perf: remove draftTransactions collection subscriptions from expense creation flow#84601
Conversation
…ransactions-collection-subscriptions
…ransactions-collection-subscriptions
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
shubham1206agra
and removed request for
a team
|
@shubham1206agra 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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9fe4f4904
ℹ️ 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".
| report, | ||
| reportDraft, |
There was a problem hiding this comment.
IOURequestStartPage now reads report/reportDraft from props instead of subscribing to ${ONYXKEYS.COLLECTION.REPORT}${reportID}, but this screen is mounted directly by MoneyRequestModalStackNavigator (no withWritableReportOrNotFound wrapper injects those props), so in normal navigation these values are undefined. In non-global flows (e.g., create/submit expense from an existing chat/policy), this makes the page behave like global create (isFromGlobalCreate becomes true) and initializes the draft without the report/policy context, which can route users through the wrong flow defaults and validations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and reverted. IOURequestStartPage is not wrapped with withWritableReportOrNotFound, so the report prop was always undefined. Restored useOnyx subscription for both report and reportDraft.
| currentUserAccountIDParam, | ||
| currentUserEmailParam, | ||
| transactionViolations, | ||
| transactionViolations: getAllTransactionViolations(), |
There was a problem hiding this comment.
@BartekObudzinski Same here, please don't rely on impure functions.
There was a problem hiding this comment.
Good catch, restored useOnyx subscriptions for both transactionViolations and transactionDrafts. Also removed the getDraftTransactionByID impure helper and switched back to validTransactionDraftsSelector for direct lookup.
| policyRecentlyUsedCurrencies: policyRecentlyUsedCurrencies ?? [], | ||
| existingTransactionDraft, | ||
| draftTransactionIDs, | ||
| draftTransactionIDs: draftTransactionIDs ?? [], |
There was a problem hiding this comment.
@BartekObudzinski Same here, please don't rely on impure functions.
| ...props | ||
| }: AccessOrNotFoundWrapperProps) { | ||
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); | ||
| const [reportFromOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportProp ? undefined : reportID}`); |
There was a problem hiding this comment.
This will make it report_undefined when reportProp is present. I think that's not what is wanted, right?
There was a problem hiding this comment.
Reverted this change entirely. The report prop optimization was not worth the complexity since IOURequestStartPage already needs its own subscription anyway.
…erve pure function pattern
…ransactions-collection-subscriptions # Conflicts: # src/components/FloatingCameraButton/BaseFloatingCameraButton.tsx # src/hooks/useReceiptScanDrop.tsx # src/pages/inbox/sidebar/FloatingActionButtonAndPopover.tsx # src/pages/iou/request/IOURequestStartPage.tsx
|
Looks like the scroll-and-highlight feature is not working properly on mWeb Screen.Recording.2026-03-13.at.12.46.19.AM.mov |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Changes LGTM aside from @ShridharGoel's comment above
|
@ShridharGoel Added the improvement metrics to the description. The ManualOpenCreateExpense metric improves by approximately 12% with these subscription removals. |
|
@ShridharGoel Looking into the scroll-and-highlight issue on mWeb. This PR only modifies Onyx subscription patterns (replacing full collection subscriptions with selectors and removing unused ones), so the scroll-and-highlight regression is likely coming from a different change that got merged into the branch. Will investigate and report back. |
Screen.Recording.2026-03-13.at.09.08.04.movI could not reproduce same results on staging and dev my branch |
|
Just checked on staging, and was able to repro the same issue there as well. So, not related to this change. |
|
@ShridharGoel @marcaaron Are we good to go? |
|
Was waiting for this |
|
@ShridharGoel Sorry Didn't see let me verify again |
…to perf/remove-draft-transactions-collection-subscriptions
|
@ShridharGoel ready |
|
@ShridharGoel @marcaaron the pr is ready |
|
LGTM other than the conflicts. |
…ransactions-collection-subscriptions
|
@marcaaron done |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @marcaaron 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/marcaaron in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
6 participants
Explanation of Change
Reduces unnecessary Onyx subscriptions during the expense creation mount path. Removing these subscriptions improves the ManualOpenCreateExpense metric by approximately 12%.
Subscription removal (from earlier commit):
useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT)full collection subscriptions from 8 components/hooksdraftTransactionsparameter removed frominitMoneyRequest,clearMoneyRequest, andstartMoneyRequestsince the underlying functions already read from module-level Onyx cacheAdditional optimizations:
useReceiptScanDrop: replaced fullTRANSACTION_DRAFTcollection subscription with IDs-only selectorinitMoneyRequest: acceptsdraftTransactionIDsinstead of full collection, avoids deserializing all draft transaction objectsFixed Issues
$ #84925
PROPOSAL:
Tests
Offline tests
QA Steps
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-10.at.15.26.08.mov
Android: mWeb Chrome
Screen.Recording.2026-03-10.at.15.28.48.mov
iOS: Native
Screen.Recording.2026-03-10.at.15.35.22.mov
iOS: mWeb Safari
Screen.Recording.2026-03-10.at.15.31.27.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-10.at.15.18.49.mov