Skip to content

fix(routing): stop a navigation mid-route-resolution unmounting the app - #64

Merged
UberMouse merged 1 commit into
masterfrom
fix-route-event-drift-crash
Aug 25, 2026
Merged

fix(routing): stop a navigation mid-route-resolution unmounting the app#64
UberMouse merged 1 commit into
masterfrom
fix-route-event-drift-crash

Conversation

@UberMouse

@UberMouse UberMouse commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

The bug

buildRootComponent kept the active route in React state and the routing events it was matched from in a ref:

const [activeRoute, setActiveRoute] = useState<AnyRoute | undefined>(undefined);
const activeRouteEventsRef = useRef<RoutingEvent<AnyRoute>[]>([]);

Both are written by the same code path, but they're read at different times. The route-resolution useEffect is a passive effect, so anything that navigates between the commit and that effect running moves the ref on while the effect still closes over the previous route:

const activeRoutesEvent = activeRouteEventsRef.current.find(
  (e) => e.type === activeRoute.event
);
assertIsDefined(activeRoutesEvent);   // 💥

A slotted child that only mounts in response to a routing event does exactly this. getViewForInterpreter replays the active route events to it from a mount effect, and React runs passive effects child-first — so a child that navigates in response to its own route runs before the root's effect for the route that mounted it.

find then returns undefined, assertIsDefined throws out of a passive effect, and because there is no error boundary above a routing root, React unmounts the entire tree:

Error: Expected 'val' to be defined, but received undefined
    at assertIsDefined
    at commitHookEffectListMount
    ...
The above error occurred in the <XstateTreeRootComponent> component

Every interpreter in the tree stops and nothing remounts. The app is simply gone.

How it shows up in practice

Found in Koordinates codebase. A cold deep link which opens a detail dialog whose machine normalises the URL back to another as it opens. That normalisation runs from the dialog's mount-time route replay — the exact ordering above — and took the whole page down a few hundred ms after the dialog appeared.

It's only latent there because that app's root is currently a v4 routing root. It reproduces on every deep-link run once the root is v5.

The fix

Store the route and the events it was matched from as one state value, so they cannot drift:

interface ActiveRoute {
  route: AnyRoute;
  events: RoutingEvent<AnyRoute>[];
}

The context still exposes a ref — descendants mounting part way through a navigation genuinely do need the latest events, not the ones from the render they mounted in — but the resolution effect now reads its events out of the same state value as its route.

The assert is replaced with a console.error + early return. It's unreachable now that the two are set together, but this effect runs on every navigation and should not be able to take the application down; a thrown assert here is a crash, not a diagnostic.

Drive-by: positional route↔event pairing

Immediately after the assert, routes were paired to events by index:

for (let i = 0; i < routes.length; i++) {
  routeEventPairs.push([routes[i], activeRouteEventsRef.current[i]]);
}

routes is built by walking up the parent chain and unshifting, so it's root-first. handleLocationChange builds its parent events by walking up and pushing, so result.events is [parent, grandparent, ..., root, matched] — the opposite order for the parent portion.

For R -> C -> L that gives R ← C's event, C ← R's event, L ← L's event. Every redirect above the leaf received the wrong event once a route was more than two levels deep.

This was not observable, because handleLocationChange gives every parent event the leaf's full params and an empty query, so the parent events are interchangeable in content — which is also why the existing 3-deep asyncRouteRedirects spec passes either way. But it's the same fragility that made the assert necessary in the first place, so it's now matched by event type, with "no event for this route" treated as a skip.

Test

src/tests/navigationDuringRouteResolution.spec.tsx reproduces the production shape: a routing root, a child invoked into a slot by a routing event, and that child navigating from its mount-time route replay.

It fails on master with the exact production stack (assertIsDefinedcommitHookEffectListMount) and passes with this change.

Verification

  • npx jest128 passed, 28 suites, no regressions
  • npm run lint -- --fix — 0 errors
  • npm run build — clean
  • npm run api-extractor -- --localpublic API surface unchanged (ActiveRoute is internal)

`buildRootComponent` kept the active route in state and the routing events it
was matched from in a ref. They are written by the same code path, but read at
different times: the route-resolution effect is a passive effect, so anything
that navigates between the commit and that effect moves the ref on while the
effect still sees the previous route.

A slotted child that only mounts in response to a routing event does exactly
that. `getViewForInterpreter` replays the active route events to it from a
mount effect, and passive effects run child-first, so a child that navigates in
response to its own route runs before the root's effect for the route that
mounted it. The `find` for the active route's event then returned undefined and
`assertIsDefined` threw out of the effect. There is no error boundary above a
routing root, so React unmounted the whole tree and nothing remounted.

Store the route and its events as a single state value so they cannot drift,
and treat a missing event as a skipped redirect pass rather than a throw - an
effect that runs on every navigation should not be able to take the app down.

Also pair routes to events by event type instead of by index. `routes` is built
by walking up the parent chain (root first) while `handleLocationChange` returns
the parent events in the opposite order, so the positional pairing handed the
wrong event to every `redirect` above the leaf once a route was more than two
levels deep. Parent routing events currently carry identical params/query/meta,
so this was not observable, but it was the same fragility that made the assert
necessary in the first place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a React/XState routing-root crash where a navigation that occurs between a commit and the root’s passive route-resolution effect could desynchronize the “active route” from the “events it was matched from”, causing an assertion to throw in a passive effect and unmount the entire app. The fix makes the active route and its matched routing events a single, atomic state value and makes redirect resolution resilient to missing event pairing.

Changes:

  • Store the matched route and its matched routing events together in a single ActiveRoute state value to prevent state/ref drift during passive-effect timing.
  • Pair routes to routing events by event type (not by index) and skip redirect resolution safely if the active-route event can’t be found (log + early return instead of throwing).
  • Add a regression test reproducing “child navigates during mount-time route replay” to ensure the app is not torn down.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/xstateTree.tsx Makes active route + matched events atomic and fixes redirect route↔event pairing to prevent crashes and mis-association in deep route chains.
src/tests/navigationDuringRouteResolution.spec.tsx Adds a regression test covering navigation that occurs during root route-resolution (passive effect ordering) to prevent full-tree unmounts.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@UberMouse
UberMouse merged commit 68d783d into master Aug 25, 2026
2 checks passed
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 5.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants