perf: extract submitAmount handler and cache submit-only Onyx state#93584
Conversation
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
melvin-bot
Bot
requested review from
JmillsExpensify and
Pujan92
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, [originalCurrency]); | ||
|
|
||
| const navigateBack = () => { | ||
| const navigateBack = useCallback(() => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes closures based on their captured variables. The new useCallback wrapping around navigateBack is redundant and interferes with the compiler's optimization model.
Remove the useCallback wrapper and use a plain function:
const navigateBack = () => {
Navigation.goBack(backTo);
};Reviewed at: adb2075 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-16.at.18.48.15.mov |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
| allPersonalDetails, | ||
| conciergeReportID, | ||
| reportAttributesReports, | ||
| allReportDrafts?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT}${participant.reportID}`], |
There was a problem hiding this comment.
Just to verify, earlier the same draftReport was used here for all participants and now specific to participant reportID. Will this change create any issues?
There was a problem hiding this comment.
In a 1:1 DM or single policy expense chat the two resolve to the same draft anyway, but in a multi-participant flow this could cause an issue. So reverted this to use the same single screen derived reportDraft.
cristipaval
left a comment
cristipaval
left a comment
There was a problem hiding this comment.
On you now, @Julesssss
|
🚧 @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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.10-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site files under Why no docs changes are neededThe PR only touches internal implementation:
There are no changes to user-facing copy, feature names, tab/setting labels, buttons, or If you believe a help article should still be updated, reply with |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.12-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site files under This is a performance/internal refactor only — it has no user-facing impact:
There are no new features, no changed UI labels, tabs, settings, or buttons, and no behavioral changes that the help site documents. The expense creation/editing flows described in the docs continue to work exactly as before, so nothing in @OlimpiaZurek, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR (Note: no help site PR was created because no documentation changes are needed for this change.) |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.14-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.14-1 🚀
|
7 participants
Explanation of Change
This PR moves the submit handler into the same module that already holds the helpers, and caches the simple/derived/singleton-shaped submit-only state at module scope so it no longer triggers a
useOnyxsetup on every screen mount.IOURequestStepAmounthad ~21 submit-onlyuseOnyxsubscriptions — values consumed only inside the submit handler, never during render — yet each was a lifetime subscription created on every mount.This PR moves the submit handler into the same module that already holds the helpers, and caches the simple/derived/singleton-shaped submit-only state at module scope so it no longer triggers a
useOnyxsetup on every screen mount.Net effect: −11 mount-time Onyx subscriptions on
IOURequestStepAmount.A follow-up PR will handle the remaining collection-shaped submit data (transaction collections, NVPs, etc.) and drop the corresponding screen-side subscriptions.
Fixed Issues
$ #92411
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari