Fix infinite onboarding navigation loop crash on iOS#88014
Conversation
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
So it's the combination: the original guard lacking the idempotency check + the partial fix not covering RESET actions. The crash only affects a narrow population (HybridApp users who completed HybridApp onboarding but not NewDot onboarding), which is why it took a month to surface. @mountiny Looking at the Sentry logs and the fix, this investigation seems accurate. |
|
Confirmed in the adhoc this is fixed for the customer on narrow web |
melvin-bot
Bot
requested review from
abdulrahuman5196 and
heyjennahay
and removed request for
a team
|
@abdulrahuman5196 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] |
mountiny
requested review from
Krishna2323
and removed request for
abdulrahuman5196
![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: bb1b80a6e1
ℹ️ 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".
| const isAlreadyOnOnboarding = state.routes.some((route) => route.name === NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR); | ||
| if (isAlreadyOnOnboarding) { | ||
| return {type: 'ALLOW'}; |
There was a problem hiding this comment.
This check assumes that having OnboardingModalNavigator anywhere in state.routes means the user is currently on onboarding, but stacks can contain that route while another screen is focused (the adapted onboarding reset state includes both HOME and onboarding). In that state, any non-RESET action (for example a NAVIGATE to HOME/deep link) bypasses onboarding because shouldPreventReset() only handles RESET and this branch returns ALLOW, so users who still require onboarding can escape the flow. The guard should only allow when onboarding is actually focused (or when the action explicitly targets onboarding).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@mountiny this seems like a valid concern. I think we should check if an onboarding screen is actually focused instead:
const currentFocusedRoute = findFocusedRoute(state);
if (isOnboardingFlowName(currentFocusedRoute?.name)) {
return {type: 'ALLOW'};
}Both imports are already in the file. This way the guard only skips the redirect when the user is genuinely viewing onboarding, not just when it's somewhere in the stack.
There was a problem hiding this comment.
We can also add a check for the action target, not just the current state. There's a window where a RESET action targets onboarding (e.g. from useNavigationResetOnLayoutChange) but the state hasn't committed yet — so the focused route check alone could miss it.
const currentFocusedRoute = findFocusedRoute(state);
const isResettingToOnboarding = action.type === CONST.NAVIGATION_ACTIONS.RESET && !!action.payload && isOnboardingFlowName(findFocusedRoute(action.payload as NavigationState)?.name);
if (isOnboardingFlowName(currentFocusedRoute?.name) || isResettingToOnboarding) {
return {type: 'ALLOW'};
}This pattern is used in MigratedUserWelcomeModalGuard.
App/src/libs/Navigation/guards/MigratedUserWelcomeModalGuard.ts
Lines 66 to 98 in 9bcf248
There was a problem hiding this comment.
@MelvinBot investigate the concern raised by chatgpt-codex-connector, and also check whether both of my comments above are valid and regression-free.
There was a problem hiding this comment.
The codex-connector concern is valid, and both of Krishna2323's suggestions are correct improvements.
The concern (codex-connector):
The current check (state.routes.some) returns ALLOW whenever OnboardingModalNavigator is anywhere in state.routes. But React Navigation's state.index determines which route is focused — the onboarding navigator can be in the stack while another screen (e.g. HOME) is actually focused. In that case, the guard would incorrectly ALLOW, letting the user bypass onboarding.
Comment 1 (focused-route check) — valid, no regression:
findFocusedRoute drills into nested navigators and returns the deepest focused screen. isOnboardingFlowName checks against SCREENS.ONBOARDING values (the actual screen names like OnboardingPurpose, etc). This correctly identifies when the user is viewing onboarding, not just when the navigator exists somewhere in the stack. The infinite loop is still broken (when onboarding IS focused → ALLOW), but the escape-hatch bug is fixed (when HOME is focused → falls through to REDIRECT).
Comment 2 (RESET-in-flight check) — valid, no regression:
This covers the timing window where a RESET action targets onboarding but the state hasn't committed yet — so the focused-route check alone would miss it. This is exactly the pattern used in MigratedUserWelcomeModalGuard.isNavigatingToMigratedUserModal() (lines 66-72), which checks both isOnModal and isResettingToModal. Following the established codebase pattern here is the right call.
Both changes together make the guard strictly more precise without regressing the infinite-loop fix.
There was a problem hiding this comment.
Thanks for the detailed analysis. A few points:
Re comment 1 (findFocusedRoute + isOnboardingFlowName): The current check state.routes[state.index]?.name already verifies the focused route — it just does it at the navigator level. OnboardingModalNavigator only appears at state.index when onboarding is focused, so the result is identical. Using findFocusedRoute would be a stylistic consistency with shouldPreventReset, but not a correctness difference.
Re comment 2 (RESET-in-flight): This doesn't apply here. The guard's REDIRECT is converted to a RESET and processed by the base StackRouter (line 108 of RootStackRouter.ts), not the custom router — so guards are not re-entered during redirect processing. The next time the guard runs, the state already has OnboardingModalNavigator focused. The MigratedUserWelcomeModalGuard needs this pattern because of its hasRedirectedToMigratedUserModal flag; our guard is purely state-driven and doesn't have that timing window.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_native.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_native.mp4iOS: mWeb Safariios_safari.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
Good catch — tightened the check from |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Apart from the failing checks, this LGTM. |
The OnboardingGuard would unconditionally REDIRECT to the onboarding route whenever shouldSkipOnboarding was false, even when the user was already on the OnboardingModalNavigator. Each REDIRECT produced a CommonActions.reset that changed the navigation state, which triggered downstream effects dispatching further actions, re-entering the guard, and looping until React hit maximum update depth. Add a check for OnboardingModalNavigator already being present in the navigation state routes before issuing a REDIRECT. This makes the guard idempotent: once the user is on onboarding, subsequent evaluations return ALLOW instead of producing redundant resets. Made-with: Cursor
Check state.routes[state.index] instead of .some() so the guard only ALLOWs when OnboardingModalNavigator is actually focused, not merely present in the stack. Adds a test for the unfocused edge case. Made-with: Cursor
Made-with: Cursor
mountiny
force-pushed
the
fix-onboarding-nav-loop-APP-7FR
branch
from
797d876 to
6a4298b
Compare
|
@srikarparsi 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] |
|
|
||
| // If the OnboardingModalNavigator is the currently focused route, the user is already | ||
| // on the onboarding flow. Redirecting again would produce a redundant state reset that | ||
| // triggers further actions, creating an infinite navigation loop (APP-7FR). |
There was a problem hiding this comment.
NAB - What is APP-7FR? Not sure it's relevant to add if it's not explained anywhere.
There was a problem hiding this comment.
It is the issue id in Sentry
I agree it does not have to be there, but this is causing crashes for users in production so I would still go ahead and merge now
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.60-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The fix is a purely internal navigation guard change (idempotency check in |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.60-22 🚀
|
5 participants
Explanation of Change
Root cause: The
OnboardingGuardunconditionally returnsREDIRECTwhenshouldSkipOnboardingisfalse, even when the user is already on theOnboardingModalNavigator. EachREDIRECTproduces aCommonActions.reset()that changes the navigation state, which triggersonStateChange→ Onyx writes → re-renders → re-evaluation of the guard → anotherREDIRECT→ another reset — an infinite loop that crashes with "Maximum update depth exceeded."Fix: Add an idempotency check before the
REDIRECTpath — if theOnboardingModalNavigatoris already present instate.routes, returnALLOWinstead of issuing a redundant redirect. This guarantees the guard reaches a stable state: the first evaluation redirects to onboarding, and all subsequent evaluations pass through without repeating the reset.The fix is minimal (8 lines including comment) and localized to
OnboardingGuard.evaluate(). It does not change behavior for users who are not yet on onboarding — they still get redirected as before.Evidence from Sentry: The crash breadcrumbs show rapid, repeated navigation to
OnboardingModalNavigator, consistent with this loop. VL logs confirm[OnboardingGuard] Redirecting to onboarding routefiring repeatedly with all boolean conditions empty (serializedfalse/undefined).Fixed Issues
$ #88018
Tests
Confirmed by supportalling to user account that I can reproduce the issue for in production narrow view and I cannot reproduce on this build
Offline tests
N/A — the navigation guard logic is purely client-side and does not depend on network state.
QA Steps
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