[Performance] Fix submit-to-destination span stalls caused by sliced tab state and fragile dismiss signals#93452
Conversation
getTopmostFullScreenRoute now falls back to preserved state when live state exists but can't resolve a focused route, and to params.screen as a last resort. getTabStateWithFocusedTarget rebuilds missing tabs with normalized routeNames, stripped history, and stale: true so TabRouter rehydrates properly with valid route keys. navigateAfterExpenseCreate no longer infers Search root when the navigator state is missing - only trusts explicit SCREENS.SEARCH.ROOT. Also tightens SPLIT_TO_SIDEBAR type access via a helper that preserves the original strongly-typed key check.
setDismissOnlyPendingActionForCurrentRoute now omits the destination report ID when Search is the topmost fullscreen route, preventing the span from waiting for a report that will never become visible.
…ading Replace handleLocationPermissionSubmit with dispatchSubmitHandler that routes GPS permission callbacks through the same handler selection as onConfirm, threading locationPermissionGranted into every handler. Gate getTopmostReportParams on isReportTopmostSplitNavigator to avoid trusting pre-inserted report state from unfocused tabs. Add recovery logging when Search dismiss finds RHP still on top.
Add array overload to useSubmitToDestinationVisible so a single hook call can handle both FOCUS and LAYOUT triggers with a shared guard. Consolidate SearchMoneyRequestReportPage from two separate hook calls into one [LAYOUT, FOCUS] call, preserving both completion signals.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
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 |
|
Oh you got codex already 😁 |
There was a problem hiding this comment.
Pull request overview
This PR targets submit-time navigation/visibility stalls in the expense submit flow by hardening navigation state handling (especially when tab state is lazily/sliced) and making the “submit-to-destination-visible” span end reliably across dismiss + reveal flows.
Changes:
- Reconstructs/normalizes missing tab navigator state during tab-target pre-insert/replace actions, including preserved-state/
params.screenfallbacks for “topmost fullscreen route” detection. - Improves dismiss-and-reveal reliability (e.g., report pre-insert gated on the focused Reports tab; dismiss-only follow-up action omits reportID when returning to Search).
- Extends submit-to-destination-visible span ending to support dual triggers (LAYOUT + FOCUS) and refactors orchestrator handler dispatch to thread GPS permission results through all handlers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/submitDismissStrategiesTest.ts | Adds coverage for clearing reportID on dismiss-only returns to Search. |
| tests/unit/Navigation/getTopmostFullScreenRouteTest.ts | Updates mocks to align with the new navigationRef import path. |
| tests/unit/Navigation/getTabStateWithFocusedTargetTest.ts | Adds unit tests for reconstructing a full tab state from sliced state and focusing a target tab. |
| tests/unit/isSearchTopmostFullScreenRouteTest.ts | Adds tests validating Search detection via live state → preserved state → params.screen fallback. |
| tests/unit/getSubmitHandlerTest.ts | Adds coverage for handler selection when Search isn’t topmost but dismiss-from-search is allowed. |
| tests/ui/TimeExpenseConfirmationTest.tsx | Updates navigationRef mocking to the default-exported module shape. |
| tests/ui/components/IOURequestStepConfirmationPageTest.tsx | Updates navigationRef mocking to the default-exported module shape. |
| src/pages/Search/SearchMoneyRequestReportPage.tsx | Switches span-ending trigger from LAYOUT-only to [LAYOUT, FOCUS]. |
| src/pages/iou/request/step/confirmation/SubmitExpenseOrchestrator.tsx | Refactors submit handler dispatch; adds report pre-insert gating and Search dismiss recovery; threads GPS permission flag through all handlers. |
| src/pages/iou/request/step/confirmation/submitDismissStrategies.ts | Omits reportID for dismiss-only when returning to Search; updates pending follow-up action selection accordingly. |
| src/libs/Navigation/helpers/navigateAfterExpenseCreate.ts | Refactors Search navigator state retrieval and follow-up action derivation for post-create navigation. |
| src/libs/Navigation/helpers/getTopmostFullScreenRoute.ts | Adds fallback chain (live → preserved → params.screen) for identifying the active fullscreen tab route. |
| src/libs/Navigation/AppNavigator/createRootStackNavigator/GetStateForActionHandlers.ts | Introduces getTabStateWithFocusedTarget to reconstruct missing tabs and normalize tab state during pre-insert/replace. |
| src/hooks/useSubmitToDestinationVisible.ts | Adds support for multiple triggers with a shared end-guard to prevent double-ending spans. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 982972dd97
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
JakubKorytko
changed the title
Explanation of Change
Fixes six
submit-to-destination-visiblespan failures identified from 11 Sentry traces across iOS HybridApp, Android HybridApp, and Web standalone. The common root cause is lazy/sliced tab navigator state being unavailable at submit-time decision points, combined with fragile post-dismiss visibility signals.Issue 01 - Global create falls back to default handler (trace
ce57bcb9, Android, ~6.1s): TheREPLACE_FULLSCREEN_UNDER_RHPdispatch fails becauseexistingTabState.routesonly contains[Home]when the target isSearchFullscreenNavigator. Fix:getTabStateWithFocusedTargetnow reconstructs missing tabs frombuildTabNavigatorNestedState, normalizesrouteNamestoTAB_SCREENS, strips stalehistory, and marks the statestale: trueso TabRouter rehydrates with valid route keys.Issue 02 - Report pre-insert lands on Inbox (traces
ae1d8901,3779d897,e7e05ac2, iOS/Android, 6.5–34.8s): Thereport_pre_inserthandler dismisses but the user lands on Inbox becausegetTopmostReportParamscan match a report in an unfocused Reports tab. Fix:dismissAfterEnsuringDestinationReportIsPreInsertednow gates the pre-insert check onisReportTopmostSplitNavigator()and falls back torevealRouteBeforeDismissingModalwhen the tab isn't focused.Issue 03 - Search report visibility depends on layout re-firing (traces
b593642b,b0153d43, Web Firefox/Safari, 17.8–32.0s):SearchMoneyRequestReportPageused only theLAYOUTtrigger, which doesn't re-fire when the page is merely uncovered after being covered by RHP screens. Fix:useSubmitToDestinationVisiblenow supports[LAYOUT, FOCUS]dual triggers with a sharedhasEndedRefguard.Issue 04 - Dismiss-only span waits for report (trace
5bee564c, Android, ~28.3s): Thedismiss_modalhandler storesDISMISS_MODAL_ONLYwith a destination report ID even when returning to Search. Fix:setDismissOnlyPendingActionForCurrentRouteomits the report ID when Search is the topmost fullscreen route.Issue 05 - Search dismiss leaves create flow open (trace
1ffba096, Web Chrome, ~17.9s):dismissModalfired andcreateTransactionsucceeded, but the RHP create stack remained visible. Fix:handleSearchDismissnow verifies the RHP is gone after the first dismissafterTransitionand performs a recovery dismiss with logging if it's still on top.Issue 06 - Global create scan from Spend misidentifies Search context (traces
d0c23d02,2a89ae3d,9789e05e, iOS HybridApp, 44.5–50.6s):isSearchTopmostFullScreenRoute()returns false because live tab state is sliced. Fix:getTopmostFullScreenRoutenow falls back through live state → preserved state →params.screenhint.navigateAfterExpenseCreateno longer infers Search root from missing navigator state.Additionally, the submit orchestrator is refactored: GPS permission callbacks now route through
dispatchSubmitHandler(same handler selection asonConfirm), threadinglocationPermissionGrantedinto all handlers to prevent silent coordinate loss.Fixed Issues
$ TBA
PROPOSAL: N/A
Tests
Issue 01 - Global create from Home falls back to default handler (narrow layout, Android/iOS)
+FAB →Create expenseManualSubmitToDestinationVisiblespan end log thatfast_path_handlerissearch_pre_insertorsearch_dismiss, notdefaultIssue 02 - Report pre-insert lands on Inbox instead of destination report (narrow layout, iOS/Android)
ManualSubmitToDestinationVisiblespan end log thatfast_path_handlerisreport_pre_insertIssue 03 - Search report visibility depends on layout re-firing (wide web, Firefox/Safari)
Add expense(if the report does not have that option, choose a different report)Create expenseManualSubmitToDestinationVisiblespan ends promptly when the Search money-request report is revealed - it should not wait for a data/layout change to re-fireonLayoutIssue 04 - Dismiss-only span waits for report instead of ending on Search (narrow layout, Android)
Note: Not currently reproducible with a verified UI flow. The tested UI flows route through
search_dismissand the span ends normally. This issue requiresfast_path_handler: dismiss_modalwhile Search is already the topmost fullscreen route and a destination report ID is present.from:... to:...)ManualSubmitToDestinationVisiblespan ends on the Search returnIssue 05 - Search dismiss leaves create flow open (wide web, Chrome)
ManualSubmitToDestinationVisiblespan ends without the user having to manually back outIssue 06 - Global create scan from Spend misidentifies Search context (narrow layout, iOS HybridApp)
ManualSubmitToDestinationVisiblespan end log thatfast_path_handlerissearch_dismiss, notdefaultGPS permission threading (all layouts)
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.mov
Android: mWeb Chrome
iOS: Native
iOS.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Web.mov