Stabilize submit-to popover host context to prevent Search row re-renders#95267
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@NicolasBonet 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83dd0f61a1
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
Please complete author checklist |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ost-context-stability
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.4.28-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site articles under This is a pure internal performance optimization with no user-facing impact:
Since the help site documents user-facing product behavior and this PR changes none of it, no documentation update is needed and I did not create a draft docs PR. @TMisiukiewicz, no help site PR was created since no documentation changes are required. If you believe a user-facing behavior did change and warrants a docs update, let me know and I'll draft one. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.28-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Pressing the chevron on the Pay split button in a Search list row opens a
ButtonWithDropdownMenupopover. That was causing everyExpenseReportListItemand everyReportSubmitToPopoverRootin the list to re-render.Root cause:
useReportSubmitToPopoversubscribes toONYXKEYS.MODAL(willAlertModalBecomeVisible). That value was a dependency of theopenReportSubmitToPopovercallback, so the callback got a new identity on every modal/popover toggle. The callback flows intoopenReportSubmitToPopoverForHost→ the memoizedhostContextValue→ theReportSubmitToPopoverHostContext.Providervalue inReportSubmitToPopoverHost, which is mounted once around the whole Search list (SearchPage.tsx). Every row'sReportSubmitToPopoverRootconsumes that context, so opening any modal — including the unrelated Pay dropdown — toggled the context reference and re-rendered all rows.Fix:
openReportSubmitToPopoveronly needs to readwillAlertModalBecomeVisibleat press time, not depend on it. The value is now mirrored into a ref (willAlertModalBecomeVisibleRef, synced viauseEffect) and read via.currentinside the callback, and dropped from the deps. The callback — and therefore the shared host context value — now stays referentially stable acrossMODALchanges, so unrelated popovers no longer re-render every Search row.Measured impact: total render time on opening the Pay dropdown dropped from 232ms → 166ms (≈28% improvement).
Fixed Issues
$ #95283
Tests
Offline tests
N/A — no network behavior changed.
QA Steps
Same as tests.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari