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. |
|
@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] |
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.
Compare full redirect target before short-circuiting
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.
Preserve stack only for modal redirects
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. |
|
@MelvinBot let's update the code from line 97 to 116 with this code: Then, please add this right above the |
Co-authored-by: bernhardoj <nicholausbernhard@gmail.com> Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Done! I've applied both changes:
Commit: 71716c7 |
|
@MelvinBot please update the unit test |
Co-authored-by: bernhardoj <nicholausbernhard@gmail.com> Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Updated the unit test to match the new modal-only redirect preservation logic:
Commit: 4b4ac60 |
Prevent duplicate navigation state when the guard fires a REDIRECT to a route that is already focused (e.g. OnboardingModalNavigator). Return the current state unchanged instead of creating a new route. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
…oardingRedirect-v2' into claude-preserveDeepLinkDuringOnboardingRedirect-v2 Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing check: the Note: the remote branch also had a commit that replaced the original failing test with a different test case. Both changes have been merged. |
|
@FitseTLT I think it's not possible to cover all cases with a generic solution, so I borrowed some code from #86856 to focus only on the modal guard. I think the reason #86258 can happen is that we append the existing routes with the onboarding navigator, and somehow, there is another modal screen with the Expensify title logo behind the onboarding navigator. Not sure why it could happen, but it's not happening anymore, and to prevent that from happening in the future, I decided to not append the existing route, but rather, conditionally put either Home or the existing full-screen route as the first route. (what's left to fix is #86390 (comment)) |
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.
|
|
@MelvinBot please revert the commit a9d22dc |
This reverts commit a9d22dc.
|
Reverted commit a9d22dc ("Fix: skip redirect when target is already the focused route") as requested. |
|
@MelvinBot let's remove navigating to HOME in |
When no specific report is found after onboarding, stop navigating to HOME and let the modal dismissal handle the default behavior. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Removed the |
|
@MelvinBot please also update the test step to include selecting "Something else" option for onboarding, before step 4. |
|
Updated the Tests and QA Steps sections to include selecting the "Something else" option for onboarding as step 4, before completing the onboarding flow. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c58257cbd4
ℹ️ 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".
| let resetRoutes: typeof redirectState.routes = redirectState.routes; | ||
| if (isModalRedirect) { | ||
| const redirectRoute = redirectState.routes.at(-1); | ||
| const existingFullScreenRoute = state.routes.find((route) => isFullScreenName(route.name)); |
There was a problem hiding this comment.
Preserve the active fullscreen route for modal redirects
Using state.routes.find(...) here preserves the first fullscreen route in the stack, not the currently active one. This stack can contain multiple fullscreen routes (e.g., via handlePushFullscreenAction), so when a guard redirects to onboarding/migrated-user modal, the reset may become [oldFullscreen, modal] and dismissing the modal drops users onto an older screen instead of the screen they were on. Pick the last fullscreen route (the route directly under the modal) when building resetRoutes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — changed state.routes.find(...) to state.routes.findLast(...) so the most recently active fullscreen route is preserved instead of the first one in the stack.
| } else { | ||
| // Navigate to home to trigger guard evaluation | ||
| Navigation.navigate(ROUTES.HOME); |
There was a problem hiding this comment.
This is to fix what I wrote here: #86390 (comment)
(I asked if it safe to remove it here)
There was a problem hiding this comment.
But the comment above the code says:
// Navigate to home to trigger guard evaluation
So our guard code change will handle it. NO?
There was a problem hiding this comment.
That's for test drive modal guard, which doesn't exist anymore.
There was a problem hiding this comment.
I don't understand how the problem you explained here relates with the scope of this pr. Is there any specific circumstance that this code makes the expected result of this pr fail?
There was a problem hiding this comment.
Yes, the 6th step
Verify that after onboarding, you land on the deep-linked report room instead of the home page
There was a problem hiding this comment.
But without this change I was able to land on the report. Are u saying for specific onboarding flow? For instance, the manage teams flow landed on the admin's room but I thought that was expected.
There was a problem hiding this comment.
Can you test choosing Something else and complete the onboarding?
There was a problem hiding this comment.
I couldn't test because of a new bug on main here
When multiple fullscreen routes exist in the stack, find() picks the first one which may be stale. Using findLast() preserves the most recently active fullscreen route so dismissing the redirect modal returns users to the correct screen. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
@MelvinBot assign me and merge main |
|
@MelvinBot merge main |
…nkDuringOnboardingRedirect-v2 Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
Assigned |
|
MelvinBot has made 12 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
The branch is already up to date with |
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