[Metrics] Optimize submit-to-Search navigation performance#84910
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
|
@QichenZhu 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] |
Update IOURequestStepConfirmationPageTest to wait for submit actions scheduled via requestAnimationFrame. This fixes failures introduced by the deferred confirmation flow where assertions were running before requestMoney, startSplitBill, and createDistanceRequest were invoked.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
staszekscp
left a comment
There was a problem hiding this comment.
This looks great! 🎉 Could you only check the impact of the change for the mobile devices?
| lastPaymentMethod, | ||
| personalPolicyID, | ||
| customCardNames, | ||
| selectedTransactions, |
There was a problem hiding this comment.
Just one thing came into my mind - does it mean that we rerender the whole list when the selection of just one item changes 🤔
There was a problem hiding this comment.
Not exactly. Updating selectedTransactions does invalidate the list’s item render path, but because FlashList is virtualized this does not mean the whole dataset is fully re-rendered at once. In practice, it mainly re-renders the visible/recycled cells. So the effect is broader than "just one row", but narrower than "the entire list".
|
On native the navigation one does not apply but it is still (simulator so it's slower anyway) 3 259 ms -> 2 408 ms (-26%) from rendering improvements. iOS - mainmain_ios.moviOS - this branchthis_branch_ios.movAndroid - mainmain_android.movAndroid - this branchthis_branch_android.movNOTE: On the native videos, you can see that scrolling to and highlighting the newly added expense is not fully consistent. This is not unique to this branch - the same issue can also happen on main (android main video for example) - but this branch makes the timing window a bit easier to hit. This happens because the search highlight flow is currently timer-based. After submit outside Inbox, we mark the new transaction for highlight on the Search route, but that highlight state is cleared after a fixed timeout. The actual scroll/highlight happens later, once the Search list has mounted, grouped/sorted data is ready, and the list layout callback runs. If Search rendering is delayed by navigation timing, loading skeletons, or heavy list work, the highlight state can expire before the row is actually reachable, so the expense is neither scrolled into view nor highlighted. Since this PR is a performance-focused change, I think fixing that race is out of scope here. The right fix is to make the highlight lifecycle readiness-based instead of timeout-based: keep the manual highlight state until the target transaction is actually present in the rendered Search data and the list can scroll to it, then clear the flag only after the scroll/highlight has started. @mountiny may you please create an issue for that? |
|
@JakubKorytko Sounds good, could you actually create the issue for it yourself with no labels as you already have the context and reproduction? |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
The navigation seems to have become a little buggy: Screen.Recording.2026-03-12.at.6.58.45.PM.mov |
|
Quick summary of the nav fix (recent commit): The browser back/forward was broken after expense submit because The fix introduces a new Visual behavior is identical (Search still appears behind the closing RHP), no perf impact (same synchronous router operation), and all the existing optimizations are untouched. Also consolidated the two helpers into a single fix.mp4 |
|
Are you seeing the same numbers (performance wise) after the fix? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-03-13.at.1.38.29.AM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-13.at.1.20.37.AM.mov |
|
Bug: Deferring Search work makes to-do searches show the empty state before showing the items Screen.Recording.2026-03-13.at.1.41.18.AM.mov |
|
@ShridharGoel should be ready for a re-review soon 🤞 |
src/libs/Navigation/AppNavigator/createRootStackNavigator/GetStateForActionHandlers.ts
Show resolved
Hide resolved
mountiny
left a comment
There was a problem hiding this comment.
Let's give it a go, thank you!
|
🚧 @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! 🧪🧪
|
|
✋ 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/mountiny in version: 9.3.39-0 🚀
|
|
Deploy Blocker #85534 was identified to be related to this PR. |
|
Deploy Blocker #85640 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.39-3 🚀
|
Explanation of Change
Improve the submit-to-search handoff by revealing the target Search route before dismissing the confirmation modal and deferring expensive Search list work until after the first paint. The update also avoids rebuilding selection/highlight data for every list render in
SearchandSearchList.These changes improve the performance of submit action from ~1130,2ms to ~547,8ms (-51.5%) and make it a lot smoother experience on web.
On native the navigation one does not apply but it is still (simulator so it's slower anyway) 3 259 ms -> 1996 ms (-38.7%) from rendering improvements.
Main branch - current state
main.mov
This branch
this_PR.mov
Fixed Issues
$ #83634
PROPOSAL: N/A
Tests
Offline tests
N/A
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari