Fix workspace navigation after distance settings deep link - #98423
Fix workspace navigation after distance settings deep link#98423fedirjh wants to merge 7 commits into
Conversation
|
@marufsharifi 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-12.at.6.15.08.PM.movAndroid: mWeb ChromeScreencast.From.2026-08-12.18-01-48.webmiOS: HybridAppScreen.Recording.2026-08-12.at.6.04.47.PM.moviOS: mWeb SafariScreen.Recording.2026-08-12.at.6.08.11.PM.movMacOS: Chrome / SafariScreencast.From.2026-08-12.17-58-17.webm |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6533c3d643
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const updatedTabRoute = {...existingTabRoute, state: staleTabState} as StackNavigationState<ParamListBase>['routes'][number]; | ||
| // Drop consumed deep-link hints before remounting, or React Navigation can replay the old target over the new state. | ||
| const updatedTabRoute = {...withSanitizedDeepLinkParams(existingTabRoute, undefined), state: staleTabState} as StackNavigationState<ParamListBase>['routes'][number]; |
There was a problem hiding this comment.
Remove state-only deep-link hints before remounting
When the existing TAB_NAVIGATOR was created by a nested push, getRehydratedTabNavigatorStateAfterPush() stores the consumed target as route.params.state without a screen key (lines 176–182). This call does not remove that hint because withSanitizedDeepLinkParams() only sanitizes params containing a string screen, so creating a workspace afterward can still let React Navigation replay the previous nested workspace state over staleTabState. Sanitize the state-only form here as well before remounting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The state-only hint comes from the separate nested-push flow, while this PR addresses stale params.screen hints from the reported distance-settings flow. Expanding the shared sanitizer would increase this regression fix’s blast radius, so I propose handling the state-only form in a focused follow-up with dedicated coverage.
|
@MelvinBot Review |
ReviewSolid, well-targeted fix. The root cause — a matching workspace background rebuilt from only the split navigator's name, dropping its nested state — is correctly addressed by preserving 1. The 2. The cancel/restore path re-injects the stale hint this PR strips. At What I verified
I did not run the app; the added Jest suites ( Reply with |
|
|
||
| return { | ||
| screen: lastRoute.name, | ||
| params: lastRoute.state ? {...lastRoute.params, state: lastRoute.state} : lastRoute.params, |
There was a problem hiding this comment.
This now preserves nested state for all matching-fullscreen navigators, not just Workspace.
getMatchingFullScreenRouteParams runs for every matching-fullscreen tab switch, so Reports / Settings / Search split navigators are also affected. Before this PR their split was reset to its initial route on tab switch; now the full nested stack is restored. That's very likely the more-correct behavior and aligns this branch with the state-preserving branch just above (~L249), but the added tests only cover WORKSPACE_SPLIT_NAVIGATOR.
- Could you confirm a non-workspace matching-fullscreen RHP open (e.g. Reports/Settings) still lands on the expected background screen and doesn't jump to an unexpected deep sub-screen?
- If it checks out, consider a one-line test for a non-workspace navigator to lock the generalization in.
Explanation of Change
Opening the distance settings RHP from another tab could construct an incomplete workspace background on narrow screens. The navigation action retained the
WorkspaceSplitNavigatorname but dropped its nested state, where the policy-scoped Workspace Initial and Distance Rates routes live. Closing the RHP could therefore reveal/workspaces/undefined.This change:
WorkspaceInitialPagefrom fetching policy data, showing Not Found, or closing the RHP before a route policy ID exists.Fixed Issues
$ #98365
PROPOSAL:
Tests
/workspaces/undefined.Offline tests
/workspaces/undefined, and the app does not crash.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 stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
CleanShot.2026-08-12.at.13.55.23.mp4
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari