Preserve deep-linked report route during onboarding guard redirect#86390
Preserve deep-linked report route during onboarding guard redirect#86390
Conversation
When the OnboardingGuard fires a REDIRECT, handleNavigationGuards was replacing the entire navigation state with [Home, OnboardingModalNavigator]. This discarded any existing fullscreen routes, including a deep-linked report that the user was trying to access before signing in. Now we separate the redirect state into fullscreen (base) and modal routes. If the current state already has fullscreen routes and the redirect adds modal routes, we preserve the existing fullscreen routes and layer the modal routes on top. This means the navigation state becomes [ReportsSplitNavigator(deep-linked report), OnboardingModalNavigator] instead of [Home, OnboardingModalNavigator], so when onboarding completes and the modal is dismissed, the user lands on their deep-linked report. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Hold on #85660 (comment) |
|
@MelvinBot please merge with |
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23635905944 |
|
@MelvinBot please merge with the |
…nkDuringOnboardingRedirect-v2 Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Successfully merged |
|
The failing check ( Analysis: The test Evidence: This PR only changes Re-running the failing job should resolve this. |
|
@MelvinBot mark this PR as ready |
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23638630176 |
|
@MelvinBot mark this PR as ready |
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23638673642 |
|
@MelvinBot mark this PR as ready |
|
I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here). Failure details: https://github.com/Expensify/Expensify/actions/runs/23639248265 |
|
@MelvinBot mark this PR as ready |
|
Done! This PR has been marked as ready for review. |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
|
@bernhardoj 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] |
![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: ed1c51ff6e
ℹ️ 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".
| if (redirectRoute && state.routes[state.index]?.name === redirectRoute.name) { | ||
| return state; |
There was a problem hiding this comment.
This early return only compares the top-level route name, so redirects to different onboarding steps are treated as duplicates once OnboardingModalNavigator is focused. OnboardingGuard computes step-specific redirect paths (e.g., purpose vs employees) but they share the same root navigator name; returning state here skips the reset and also bypasses downstream action handling, which can leave users stuck on an outdated onboarding step instead of being redirected to the newly required one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah I really think this is unneeded as well as wrong.
There was a problem hiding this comment.
We need this, otherwise multiple onboarding modal will be added when we refresh the app. (multiple navigation actions are fired).
I wanna test comparing findFocusedRoute(state).name === findFocusedRoute(redirectState).name, but got distracted with other things and forget to come back to this 🤦
There was a problem hiding this comment.
Wrong this change BTW caused a regression. If you proceed up to accounting stage of onboarding(selecting manage teams) and deeplink to purpose page you are now not correctly redirected to oboarding-account page.
There was a problem hiding this comment.
I wanna test comparing findFocusedRoute(state).name === findFocusedRoute(redirectState).name, but got distracted with other things and forget to come back to this
Yep, that's why I say this ^. If we use findFocusedRoute(state).name === findFocusedRoute(redirectState).name, then that won't happen. But there is another problem.
When we open onboarding/purpose by changing the URL, the root state will be [Home, OnboardingModalNavigator (with the onboarding purpose screen)], then the REDIRECT guard will add another OnboardingModalNavigator, so we have 2 OnboardingModalNavigator on the stack, which we don't want.
| // append only the redirect target on top of the existing routes so the user returns | ||
| // to them after the redirect screen is dismissed. Otherwise (fresh app with no stack), | ||
| // use the full redirect state which includes the base route (e.g., Home). | ||
| const routes = hasExistingFullScreenRoute && redirectRoute ? [...state.routes, redirectRoute] : redirectState.routes; |
There was a problem hiding this comment.
This condition preserves the existing stack for any redirect as long as a fullscreen route exists, but OnboardingGuard also issues non-modal redirects to ROUTES.HOME for completed users. In those cases the new behavior pushes Home on top of prior routes instead of replacing the stack, so back navigation can return users to pre-redirect screens that the guard intended to exit. The stack-preservation path should be gated to modal redirect targets only.
Useful? React with 👍 / 👎.
|
The failing checks are unrelated to this PR's changes.
A re-run of the test job should resolve the |
|
@MelvinBot can you create an ad-hoc build, please? |
|
Sorry, creating ad-hoc builds is outside my capabilities. You can trigger an ad-hoc build manually through the GitHub Actions workflow or by asking someone with access to the build pipeline. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@m-natarajan hi, the previous PR was reverted because of this regression: #86258. I recreated the same PR, but I still can't reproduce the regression. Can you use the ad-hoc build and see if you can reproduce the regression, please? Thanks! |
|
@bernhardoj Expensify logo not displayed during onboarding in the Ad-hoc build, Tried multiple times signing up for new account as well. Screen.Recording.2026-03-30.At.17.06.17.mp4 |
|
@m-natarajan thanks! |
|
@MelvinBot merge main |
|
I can only make code changes (including merging main) when directed by the PR author, an assignee, or an existing co-author. To have me merge main into this branch, the PR assignee ( |
|
@MelvinBot please merge main |
…nkDuringOnboardingRedirect-v2 Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Merged |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| return state; | ||
| } | ||
|
|
||
| const hasExistingFullScreenRoute = state.routes.some((route) => isFullScreenName(route.name)); |
There was a problem hiding this comment.
Instead of using some I think it is more logical to check on the focused route only.
There was a problem hiding this comment.
I thought about that too, but if in some way, we have stack like this, [Home, any RHP], then the focused route will be the any RHP, and another fullscreen will be added to the stack.
|
The test step should specify a specific onboarding flow b/c for manage teams choice I saw that you are navigated to the admins room of the workspace. |
I did not reproduce it on the branch. However, I was a bit doubtful of the code in terms of what we are trying to achieve and when I saw melvin's break down here, I thought maybe there is some race condition that will make the bug occur on some conditions. So I think we should update the logic, I think, in terms of what we want to achieve, the logic should be: |
Yeah. Also, after completing the onboarding, we won't be redirected to Home if we aren't merging public email with work email. As the comment said:
But after this commit, we navigate the user to Home. App/src/libs/navigateAfterOnboarding.ts Lines 70 to 75 in 0eda3f5 But I say let's come back to this after we find the best solution for the first issue. |
I am not at all suggesting we should always redirect them to the deeplinked route. I am just suggesting a test step change. |
|
So regarding the other comment I assume you 're 🤔 of a better solution. |
|
Yeah, but nothing come to my mind yet. @MelvinBot please merge with main |
…nkDuringOnboardingRedirect-v2 Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Merged |
|
So, we have 2 cases:
I think the problem here is the 1st case. Let's say we have a guard that prevents users from accessing the create expense pages if the subscription has expired, and wants to redirect them to the restricted action RHP. If we use the code in No problem here since we RESET the whole state. But if we use the code in this PR (+ comparing the focused route), the state becomes because we preserve the existing state (and now we have 2 RightModalNavigator). Now, for the 2nd case, Let's say we have this state: If we use the code on If we use the code in this PR, then the previous screen is preserved. This is really tricky. |
4 participants
Explanation of Change
This is a re-creation of the reverted PR #85556 with identical changes.
When an unauthenticated user visits a direct report deep link (e.g.,
/r/7075912447943023), signs in as a new account, and gets redirected to onboarding, the deep-linked report route is lost. This happens becausehandleNavigationGuardsinRootStackRouter.tscompletely replaces the navigation state when the OnboardingGuard fires a REDIRECT — the state becomes[Home, OnboardingModalNavigator]instead of preserving the existing report route.This PR modifies
handleNavigationGuardsso that when a REDIRECT adds a modal route (like the OnboardingModalNavigator), the existing fullscreen/base routes (like the deep-linked report) are preserved instead of being replaced with Home. The navigation state becomes[ReportsSplitNavigator(deep-linked report), OnboardingModalNavigator], so when onboarding completes and the modal is dismissed, the user lands on their deep-linked report.When no existing fullscreen routes are present or the redirect doesn't include modal routes, the behavior falls back to the previous logic.
Fixed Issues
$ #85242
PROPOSAL: #85242 (comment)
Tests
https://dev.new.expensify.com/r/7075912447943023while signed outOffline tests
N/A - This change affects navigation state during the sign-in/onboarding flow which requires network connectivity.
QA Steps
https://staging.new.expensify.com/r/7075912447943023while signed outPR 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