[No QA] Optimize ExpenseReportListItemRow - Split ActionCell into smaller components#83079
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9712947112
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/components/SelectionListWithSections/Search/ActionCell/index.tsx
Outdated
Show resolved
Hide resolved
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Nice! This is looking good! Let's mark it as ready for review! |
|
Can we get it tested against a really high traffic account? |
|
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| enablePaymentsRoute={ROUTES.ENABLE_PAYMENTS} | ||
| onPress={(type, payAsBusiness, methodID, paymentMethod) => confirmPayment(type as ValueOf<typeof CONST.IOU.PAYMENT_TYPE>, payAsBusiness, methodID, paymentMethod)} | ||
| style={[styles.w100, shouldDisablePointerEvents && styles.pointerEventsNone]} | ||
| wrapperStyle={[styles.w100]} |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line suppresses @typescript-eslint/prefer-nullish-coalescing without an accompanying comment explaining why the logical OR (||) is preferred over nullish coalescing (??) here.
Add a comment explaining the reason, for example:
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- We intentionally use || because empty string policyID should fall through to iouReport?.policyID
policyID={policyID || iouReport?.policyID}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This is a valid comment (unless we specifically want to use iouReport?.policyID if policyID is 0 (CONST.DEFAULT_NUMBER_ID)
There was a problem hiding this comment.
TBH - this part of code was just moved - earlier it was like this in src/components/SelectionListWithSections/Search/ActionCell.tsx#165.
IMO adding a comment like a bit suggests seems bit redundant, as the code is self explanatory here. WDYT ?
There was a problem hiding this comment.
That seems like an oversight to me
There was a problem hiding this comment.
@getusha is there a reason you user || here ?
I'm not really sure, but I think policyID can somehow be an empty string.
| enablePaymentsRoute={ROUTES.ENABLE_PAYMENTS} | ||
| onPress={(type, payAsBusiness, methodID, paymentMethod) => confirmPayment(type as ValueOf<typeof CONST.IOU.PAYMENT_TYPE>, payAsBusiness, methodID, paymentMethod)} | ||
| style={[styles.w100, shouldDisablePointerEvents && styles.pointerEventsNone]} | ||
| wrapperStyle={[styles.w100]} |
There was a problem hiding this comment.
This is a valid comment (unless we specifically want to use iouReport?.policyID if policyID is 0 (CONST.DEFAULT_NUMBER_ID)
src/components/SelectionListWithSections/Search/ActionCell/actionTranslationsMap.ts
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-23.at.21.00.25.movAndroid: mWeb ChromeScreen.Recording.2026-02-23.at.20.58.32.moviOS: HybridAppScreen.Recording.2026-02-23.at.20.51.13.moviOS: mWeb SafariScreen.Recording.2026-02-23.at.20.53.48.movMacOS: Chrome / SafariScreen.Recording.2026-02-23.at.20.48.36.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.25-13 🚀
|




Explanation of Change
Move pay-specific hooks out of ActionCell into PayActionCell
The original
ActionCellcalled all pay-related hooks unconditionally at the component top level —useReportWithTransactionsAndViolations,usePolicy,useOnyx(BANK_ACCOUNT_LIST),useOnyx(chatReport), andcanIOUBePaid(×2) — for every row in the search results, even rows with aVIEW,SUBMIT, orAPPROVEaction.These hooks are now scoped to
PayActionCell, which only mounts for rows whereaction === 'pay'. This removes several Onyx subscriptions and computations per non-PAY row.Perf improvement
For the the whole
Home->ReportsnavFor

ExpenseReportListItemRowComponentThe improvement should be visible everywhere where
ActionCellwas used. Not onlyReportsMethodology
Measured on account with 10 reports with in different
CONST.SEARCH.ACTION_TYPES.Times are avg between 5 measurements for before and after.
[No QA] added as this is just a refactor.
Fixed Issues
$ #82428
PROPOSAL:
Tests
ReportspageOffline tests
N/A
QA Steps
N / A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
MacOS: Chrome / Safari