Skip to content

fix: prevent onboarding crash when back-navigation empties page stack#2750

Merged
bengotow merged 2 commits into
masterfrom
claude/awesome-ritchie-zn8rl7
Jun 17, 2026
Merged

fix: prevent onboarding crash when back-navigation empties page stack#2750
bengotow merged 2 commits into
masterfrom
claude/awesome-ritchie-zn8rl7

Conversation

@bengotow

Copy link
Copy Markdown
Collaborator

Summary

Fixes Sentry issue MAILSPRING-CLIENT-2623 users affected, 59 occurrences.

Error: Cannot find component for page: undefined
Location: OnboardingRoot.render() in onboarding-root.tsx

Root Cause

OnboardingStore._onMoveToPreviousPage pops from _pageStack with no bounds check. If the action fires when only one page is on the stack (e.g. rapid back-button double-clicks, or any duplicate dispatch of moveToPreviousPage), the stack becomes empty. page() then returns undefined, and OnboardingRoot.render() throws an unhandled error instead of degrading gracefully.

The stale-closure risk in PageTopBar: closeAction captures pageDepth from props at render time. A rapid double-click could fire both handlers with pageDepth = 2 before the re-render reflecting the first pop, triggering moveToPreviousPage() twice in succession and draining the stack to zero.

Fix (two layers)

  1. onboarding-store.ts — Guard in _onMoveToPreviousPage: bail out early if the stack has only one page, making it impossible to empty the stack via back-navigation.

  2. onboarding-root.tsx — Defensive fallback in render(): if page is somehow still undefined (any future unknown cause), close/quit the window gracefully instead of throwing an unhandled exception that reaches Sentry.

Test plan

  • Launch onboarding as a new user (welcome page). Verify the X button closes the window and does not crash.
  • Navigate forward to account-choose, then rapidly double-click the back button. Verify no crash — should land on welcome page or close gracefully.
  • Normal onboarding flow completes without regression.

https://claude.ai/code/session_01PGz7rfpJU4fs2QMeT7sdYw


Generated by Claude Code

…igation

OnboardingStore._onMoveToPreviousPage had no bounds check, so rapid back-
button clicks (or any double-dispatch of the action) could pop the last
entry off _pageStack, leaving it empty. page() then returned undefined,
causing OnboardingRoot.render() to throw "Cannot find component for page:
undefined" (Sentry MAILSPRING-CLIENT-26, 23 users).

Two-layer fix:
1. Guard in _onMoveToPreviousPage: bail out if the stack has only one page,
   so the stack can never be emptied through back-navigation.
2. Defensive fallback in OnboardingRoot.render: if page is still somehow
   undefined, close/quit the window gracefully instead of throwing.

Fixes MAILSPRING-CLIENT-26

https://claude.ai/code/session_01PGz7rfpJU4fs2QMeT7sdYw
@indent-staging

indent-staging Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
PR Summary

Fixes Sentry MAILSPRING-CLIENT-26 (23 users) where rapid back-button clicks in onboarding double-dispatched moveToPreviousPage — the click handler reads stale pageDepth from React props, so two clicks at depth 2 both pass the gate — draining OnboardingStore._pageStack and causing OnboardingRoot.render() to throw Cannot find component for page: undefined. After review feedback, the defensive render-time fallback was reverted, leaving only the root-cause store fix.

  • OnboardingStore._onMoveToPreviousPage now early-returns when _pageStack.length <= 1, establishing "stack is non-empty" as a real invariant and turning stale-state rapid clicks into a safe no-op (no spurious trigger()).
  • OnboardingRoot.render is unchanged from master (the prior defensive AppEnv.quit/close fallback was reverted in commit 02784a9, preserving the loud throw for any future invalid page name).

Issues

All clear! No issues remaining. 🎉

2 issues already resolved
  • Latent: the new render fallback silently closes/quits the window for any unknown state.page, which would swallow a future OnboardingActions.moveToPage('typo') instead of surfacing the loud error the previous throw provided. No regression today — every current caller passes a literal page name that exists in PageComponents — but worth keeping in mind if you add new pages. (fixed by commit 02784a9)
  • Nit: AppEnv.quit()/AppEnv.close() are invoked from inside OnboardingRoot.render(), which is a React-purity smell — side effects belong in componentDidUpdate/componentDidMount. Harmless in practice because the store-level guard makes this branch unreachable via the primary fix, but moving the call to componentDidUpdate (and just returning null in render) would be cleaner. (fixed by commit 02784a9)

CI Checks

All CI checks passed on commit 02784a9.

Custom Rules 3 rules evaluated, 3 passed, 0 failed

Passing This is a longer title to see what happens when they are too long to fit
Passing B
Passing Ben Rule

View all rules

Comment thread app/internal_packages/onboarding/lib/onboarding-root.tsx Outdated
@bengotow

Copy link
Copy Markdown
Collaborator Author

I agree with Indent, @claude can you keep just the app/internal_packages/onboarding/lib/onboarding-store.ts change in this PR? Let's not do the defensive fallback, just the one fix should be sufficient.

@bengotow bengotow merged commit 0b43f1c into master Jun 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants