perf: optimize ManualOpenCreateExpense with policy selector and reduced subscriptions#84286
Conversation
|
Thanks for the help @BartekObudzinski let me know when this is ready for review! 🙇 |
…expense-optimization-clean # Conflicts: # src/libs/actions/IOU/index.ts
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@jayeshmangwani 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] |
|
@marcaaron Ready to review! |
| @@ -219,7 +222,7 @@ function IOURequestStartPage({ | |||
|
|
|||
| const onTabSelected = useCallback( | |||
| (newIouType: IOURequestType) => { | |||
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This Tab.setSelectedTab(CONST.TAB.IOU_REQUEST_TYPE, newIouType) call is redundant. OnyxTabNavigator already calls Tab.setSelectedTab(id, newSelectedTab) in its screenListeners.state handler (OnyxTabNavigator.tsx line 190) before invoking the onTabSelected callback (line 192). This means by the time onTabSelected runs, the Onyx key is already updated. The duplicate call triggers a second Onyx.merge for the same key with the same value on every tab switch, which is unnecessary work.
Remove the Tab.setSelectedTab call from onTabSelected since OnyxTabNavigator already handles persisting the selected tab:
const onTabSelected = useCallback(
(newIouType: IOURequestType) => {
resetIOUTypeIfChanged(newIouType);
},
[resetIOUTypeIfChanged],
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — removed the duplicate Tab.setSelectedTab call. OnyxTabNavigator already persists the selected tab before invoking onTabSelected, so our call was redundant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac68bdf260
ℹ️ 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".
| : getReportOption(participant, privateIsArchived, policy, currentUserPersonalDetails.accountID, personalDetails, reportAttributesDerived); | ||
| }) ?? [], | ||
| [transaction?.participants, iouType, personalDetails, reportAttributesDerived, reportDrafts, privateIsArchivedMap, policy, currentUserPersonalDetails.accountID], | ||
| [transaction?.participants, iouType, personalDetails, reportAttributesDerived, privateIsArchivedMap, policy, currentUserPersonalDetails.accountID], |
There was a problem hiding this comment.
Dropping reportDrafts from this memoized participants computation means updates to ONYXKEYS.COLLECTION.REPORT_DRAFT no longer invalidate the list, even though getReportOption() resolves destination data through draft reports. In flows where a destination is initially a draft report (optimistic) and then gets updated/promoted while the confirmation page is open, the row can stay stale (including draft-derived display/disabled state) until some unrelated dependency changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This removal was intentional (originally done by @OlimpiaZurek in the first draft). getReportOption calls getReportOrDraftReport internally, which falls back to the module-level allReportsDraft variable (populated via Onyx.connect in ReportUtils.ts) when the param is not passed. All other callers of getReportOption across the codebase already omit this parameter and rely on the same fallback.
There was a problem hiding this comment.
This removal was intentional (originally done by @OlimpiaZurek in the first draft). getReportOption calls getReportOrDraftReport internally, which falls back to the module-level allReportsDraft variable (populated via Onyx.connect in ReportUtils.ts) when the param is not passed. All other callers of getReportOption across the codebase already omit this parameter and rely on the same fallback.
@BartekObudzinski That goes against our efforts to remove impure functions. We should not rely on the module level onyx.connect in this case. Can you please raise a revert as we will eventually remove the module level subscription
There was a problem hiding this comment.
These changes LGTM. @Julesssss all yours.
There was a problem hiding this comment.
No product review required on this one.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @Julesssss 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.3.35-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.35-1 🚀
|
Explanation of Change
Reduces unnecessary Onyx re-renders in the Create Expense flow (ManualOpenCreateExpense span) by trimming heavy subscriptions:
Policy collection selector (
iouRequestPolicyCollectionSelectorinPolicy.ts): FiltersONYXKEYS.COLLECTION.POLICYto only the ~15 fields needed by IOURequestStartPage consumers. WithuseOnyxdeep-equal memoization, this prevents re-renders when unrelated policy fields change (accounting settings, tax rates, report fields, etc.).Derive
selectedTabdirectly: ReplaceduseState(lastSelectedTab)+ syncinguseEffectwith a simpleconst selectedTab = lastSelectedTab. Tab writes go throughTab.setSelectedTab()directly to Onyx, eliminating one render cycle on mount and tab switch.NVP selector for
SCAN_TEST_TOOLTIP: Replaced fullNVP_DISMISSED_PRODUCT_TRAININGsubscription with a boolean selector that extracts just theSCAN_TEST_TOOLTIPfield. Prevents re-renders when other product training dismissals change.Remove
reportDraftscollection subscription from IOURequestStepConfirmation:getReportOrDraftReport()already falls back to the module-levelallReportsDraftvariable when no param is passed. All other callers ofgetReportOptionalready omit this parameter.Minor cleanups: Extract magic number
2toPER_DIEM_TAB_INDEXconstant. Remove stalecurrentDate = ''default (now uses??instead of||).PErformance gain: Reduced about ~20ms on localtesting the span
Fixed Issues
$ #84789
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: Native
Screen.Recording.2026-03-06.at.11.17.35.mov
Android: mWeb Chrome
Screen.Recording.2026-03-06.at.11.20.12.mov
iOS: Native
Screen.Recording.2026-03-06.at.11.10.23.mov
iOS: mWeb Safari
Screen.Recording.2026-03-06.at.11.14.19.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-06.at.11.07.26.mov