Repository navigation
Keep the current tab when opening an exitTo link from a signed-in val… - #101335
Conversation
|
@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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ab422bc42
ℹ️ 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".
| Navigation.waitForProtectedRoutes().then(() => { | ||
| Navigation.goBack(ROUTES.HOME, {waitForTransition: true}); | ||
| if (navigationRef.canGoBack()) { | ||
| Navigation.pop(navigationRef.getRootState().key); |
There was a problem hiding this comment.
I'm not questioning it, just curious why you went with this approach instead of your original proposal. In what scenario would these two approaches behave differently, so I can test it?
There was a problem hiding this comment.
Short answer on why I moved off my proposal's line.
goBack(undefined) doesn't always pop ValidateLogin. When shouldPopToSidebar is set, goBack takes the popToSidebar() branch first, and popToSidebar() clears the flag and returns without popping anything, because the top root route is ValidateLogin and not a split navigator. NavigationRoot sets that flag on any wide → narrow change while a split navigator is on top (e.g. rotating landscape → portrait with a report open), and it stays set until the next parameterless goBack().
I ran both versions through the real navigators in Jest with the flag set. The report opens on the right tab either way, so the fix for this issue holds in both, but the resulting root stack differs:
- this PR:
[TabNavigator → Inbox split: Inbox, Report] - my original line:
[TabNavigator(Inbox), ValidateLogin, TabNavigator(Inbox split: Report)]
So with the proposal's line the consumed ValidateLogin stays in the stack and the report is pushed above it in a second tab navigator: Back from the report shows the /v/ loading screen, and only the second Back returns to the report list.
Navigation.pop(navigationRef.getRootState().key) is dispatched at the root stack directly, so the flag can't redirect it and the only route it can remove is ValidateLogin. The lone-route case is unchanged: still goBack(ROUTES.HOME, {waitForTransition: true}). This is covered by removes ValidateLogin even when a pop to sidebar is pending — it fails with my original line (Expected: false Received: true) and passes with this one.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen_recording_20260922_163527.webmAndroid: mWeb ChromeScreen_recording_20260922_164025.webmiOS: HybridAppScreen.Recording.2026-09-22.at.4.34.40.PM.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.17.-.2026-09-22.at.16.14.34.movSimulator.Screen.Recording.-.iPhone.17.-.2026-09-22.at.16.22.40.movMacOS: Chrome / SafariScreen.Recording.2026-09-22.at.4.44.00.PM.mov |
QichenZhu
left a comment
There was a problem hiding this comment.
Thanks! Please record the iOS video. You can test the direct link in the Simulator by running xcrun simctl openurl booted "new-expensify://v/22983538/MZUDTGTRV?exitTo=/r/1848836441519490". Replace the URL with the one from the email.
|
Thanks @QichenZhu Added the iOS Native video using the |
|
🚧 mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.4.92-0 🚀
|
|
🤖 No help site changes are required for this PR, so I did not create a docs draft PR. This PR is a navigation bug fix, not a product change. It swaps It restores the behavior users already expect — tapping View in an email opens the report — rather than introducing new behavior. Nothing user-facing changed: no new feature, no new or renamed setting, no new button, tab, or label. How I checked the help siteI searched
No article states the old (buggy) behavior, so there is no stale text to correct and no gap to fill. Writing a new article about it would document an implementation detail, which the help authoring guidelines do not call for. One caveat: browser control was unavailable in this run, so I could not verify the fix live in NewDot web. The conclusion above comes from reading the diff, the linked issue, and the help site markdown. Since there is no docs PR, there is nothing to label
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.92-8 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
When a signed-in user opens a report link from an email while the app is on another tab,
handleExitToNavigationcalledgoBack(ROUTES.HOME), which switched the tab to Home before the report was opened. Now onlyValidateLoginis popped when there is a screen under it, so the report opens on the current tab.goBack(ROUTES.HOME)is still used whenValidateLoginis the only route.Added unit tests in
tests/navigation/HandleExitToNavigationTests.tsx.Fixed Issues
$ #100680
PROPOSAL: #100680 (comment)
Tests
Precondition: As submitter, submit an expense report so the workspace owner receives the email.
Offline tests
N/A
QA Steps
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
Android: Native
AFTER FIX:REC-20260916180935.mp4
Android: mWeb Chrome
iOS: Native
AFTER FIX:
REC-20260922124122.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari