[WIP] [Performance] Fix submit-to-destination span stalls caused by sliced tab state and fragile dismiss signals#93452
Draft
JakubKorytko wants to merge 4 commits into
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.
49 tasks
Contributor
Author
|
@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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.type:expense-report 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari