perf: Stop amount field re-rendering on background Onyx writes#94295
Conversation
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.
|
|
Codex Review: Didn't find any major issues. 👍 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". |
2c221f0 to
1ab3ea1
Compare
|
No product review needed |
|
@Pujan92 Can you review it when you have a chance? Thanks! |
# Conflicts: # src/pages/iou/request/step/IOURequestStepAmount.tsx
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-01.at.12.09.30.mov |
Pujan92
left a comment
There was a problem hiding this comment.
Alternative approach of shifting from Onyx.ConnectWithoutView to renderless component looks good to me. I've suggested a minor change.
|
@OlimpiaZurek is out sick today, she will handle the feedback when she is back 🙏 |
tgolen
left a comment
There was a problem hiding this comment.
Thank you! This looks correct from an Onyx standpoint now 😍
Co-authored-by: Pujan Shah <shahpujan1992@gmail.com>
Resolves typecheck (Cannot find name 'Log'), ESLint (unsafe call/member access), and Prettier (missing semicolon) failures on the submit-data guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi can you check now please |
|
@VickyStash @OlimpiaZurek could you please resolve conflicts? 🙏 |
I'm on it! |
# Conflicts: # src/libs/IOUAmountSubmission.ts # src/pages/iou/request/step/IOURequestStepAmount.tsx # tests/unit/IOUAmountSubmissionTest.ts
|
Conflicts resolved |
cristipaval
left a comment
There was a problem hiding this comment.
Thanks!
On you now, @Julesssss
# Conflicts: # src/pages/iou/request/step/IOURequestStepAmount.tsx
|
🚧 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.4.29-0 🚀
|
This comment was marked as low quality.
This comment was marked as low quality.
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.31-0 🚀
|
|
🤖 Help site review: no changes required I reviewed the changes in this PR and no updates to Expensify's help site files under Why: This is a pure performance refactor with no user-facing change. It moves the submit-only Onyx subscriptions off the render path into a new renderless component ( If you believe a specific help article is affected, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.31-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This PR moves the submit-only subscriptions into a new renderless component,
AmountSubmitDataSync. It owns those subscriptions and writes the latest values into a ref that the screen's submit handler reads at click time. The form no longer subscribes to the noisy keys, so background writes don't ripple through the input while the user is typing.Results from profiling:
Total render duration: 128.6ms -> 96.6ms (-24.9%)
Fixed Issues
$ #92411
PROPOSAL:
Tests
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari