[GH-ISSUE #744] Race condition in isSameRoute classification causes janky transitions during rapid navigation #165

New issue

Closed

opened 2026-05-06 12:37:47 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 12:37:47 +02:00

Owner

Originally created by @Divkix on GitHub (Apr 1, 2026).
Original GitHub issue: https://github.com/cloudflare/vinext/issues/744

Problem

During rapid successive navigations (e.g., user clicks A→B→C quickly), the isSameRoute check in app-browser-entry.ts compares against stale window.location.pathname because URL commits are deferred via useLayoutEffect.

Current Behavior (lines 619-622)

const isSameRoute =
  stripBasePath(url.pathname, __basePath) ===
  stripBasePath(window.location.pathname, __basePath);

Race condition window:

Navigation 1 starts (A→B):
- renderNavigationPayload() called with isSameRoute=false (correctly: B ≠ A)
- navigationCommitEffect queues URL update via pushHistoryStateWithoutNotify()
- URL hasn't updated yet in window.location
Navigation 2 starts (B→C) before Navigation 1 commits:
- isSameRoute compares url.pathname (C) with window.location.pathname (still A)
- Result: C === A is false → classified as cross-route → synchronous updates
- Should be: C === B comparison to determine if B→C is same-route
Navigation 1 eventually commits:
- commitClientNavigationState() calls syncCommittedUrlStateFromLocation()
- state.cachedPathname updates to B
- But isSameRoute was already evaluated incorrectly

Impact

Scenario	Misclassification Result
Same-route (search param change) during cross-route transition	Uses sync updates instead of smooth `startTransition`
Cross-route during another pending navigation	May incorrectly use `startTransition` (rare but possible)

User-facing: Janky, non-smooth transitions when rapidly clicking between routes or changing filters during page loads.

Code Reference

This was acknowledged as a known limitation in PR #690 with this comment (lines 621-625):

// NB: During rapid navigations, window.location.pathname may not reflect
// the previous navigation's URL yet (URL commit is deferred). This could
// cause misclassification (synchronous instead of startTransition or vice
// versa), resulting in slightly less smooth transitions but correct behavior.

This issue tracks implementing a proper fix rather than accepting the workaround.

Root Cause

The ClientNavigationState tracks pending params but not pending pathname:

type ClientNavigationState = {
  // ...
  clientParams: Record<string, string | string[]>;
  pendingClientParams: Record<string, string | string[]> | null;  // ✅ Tracked
  cachedPathname: string;  // ❌ Only committed, no pending pathname
  // ...
}

Proposed Solution

Option A: Track Pending Pathname (Recommended)

Add pendingPathname to ClientNavigationState in navigation.ts:

type ClientNavigationState = {
  // ... existing fields
  pendingPathname: string | null;
  cachedPathname: string;
}

Flow changes:

Set pendingPathname immediately when __VINEXT_RSC_NAVIGATE__ starts

Update isSameRoute comparison:

const currentPath = state?.pendingPathname ?? 
                    state?.cachedPathname ?? 
                    window.location.pathname;
const isSameRoute = stripBasePath(url.pathname, __basePath) === 
                    stripBasePath(currentPath, __basePath);

Clear pendingPathname in commitClientNavigationState() after successful commit
Handle error cases: clear on navigation failure/supersession

Complexity: Requires restructuring navigation flow because isSameRoute is evaluated before stageClientParams() in both cached and non-cached paths.

Add a destinationPathname field set immediately when navigation starts, before any async work. This separates "where we are going" from "params we are staging".

Edge Cases to Handle

Overlapping navigations (A→B→C): When C starts, should compare against B (most recent pending), not A. The navId/activeNavigationId system already supersedes older navigations.
Navigation superseded before commit: If B→C starts but is superseded by B→D, pending pathname should update to D. Cleanup must be robust.
Error during navigation: If navigation fails, pendingPathname must be cleared in catch blocks. Current _snapshotPending pattern shows the complexity of cleanup.
Back/forward (traverse) navigation: Different code path via popstate handler. Uses window.location.href directly, less affected.

Files Affected

packages/vinext/src/shims/navigation.ts — add pendingPathname to state type, update commitClientNavigationState()
packages/vinext/src/server/app-browser-entry.ts — update isSameRoute logic, set/clear pending pathname

Why This Matters Now

UX regression: Rapid navigation is a common user pattern. The current workaround produces visible jank.
Self-contained fix: ~30 lines changed, does not depend on major refactors like #726 (layout persistence architecture).
Code health: The comment acknowledges this as debt. Better to fix properly than let it linger.

PR #690 (Firefox navigation hang fix) — introduced the workaround comment
Review comment: https://github.com/cloudflare/vinext/pull/690#discussion_r2034234561
Issue #726 — Layout persistence architecture (orthogonal to this race condition)

Testing Needed

E2E test for rapid navigation sequence (click A→B→C without waiting for intermediate commits)
Unit test for isSameRoute classification with simulated pending navigation
Verify no regression in back/forward navigation
Verify error handling clears pending state correctly

Originally created by @Divkix on GitHub (Apr 1, 2026). Original GitHub issue: https://github.com/cloudflare/vinext/issues/744 ## Problem During rapid successive navigations (e.g., user clicks A→B→C quickly), the `isSameRoute` check in `app-browser-entry.ts` compares against stale `window.location.pathname` because URL commits are deferred via `useLayoutEffect`. ### Current Behavior (lines 619-622) ```typescript const isSameRoute = stripBasePath(url.pathname, __basePath) === stripBasePath(window.location.pathname, __basePath); ``` **Race condition window:** 1. **Navigation 1 starts (A→B):** - `renderNavigationPayload()` called with `isSameRoute=false` (correctly: B ≠ A) - `navigationCommitEffect` queues URL update via `pushHistoryStateWithoutNotify()` - URL hasn't updated yet in `window.location` 2. **Navigation 2 starts (B→C) before Navigation 1 commits:** - `isSameRoute` compares `url.pathname` (C) with `window.location.pathname` (still A) - **Result:** C === A is false → classified as cross-route → synchronous updates - **Should be:** C === B comparison to determine if B→C is same-route 3. **Navigation 1 eventually commits:** - `commitClientNavigationState()` calls `syncCommittedUrlStateFromLocation()` - `state.cachedPathname` updates to B - But `isSameRoute` was already evaluated incorrectly ### Impact | Scenario | Misclassification Result | |----------|----------------------| | Same-route (search param change) during cross-route transition | Uses sync updates instead of smooth `startTransition` | | Cross-route during another pending navigation | May incorrectly use `startTransition` (rare but possible) | **User-facing:** Janky, non-smooth transitions when rapidly clicking between routes or changing filters during page loads. ### Code Reference This was acknowledged as a known limitation in PR #690 with this comment (lines 621-625): ```typescript // NB: During rapid navigations, window.location.pathname may not reflect // the previous navigation's URL yet (URL commit is deferred). This could // cause misclassification (synchronous instead of startTransition or vice // versa), resulting in slightly less smooth transitions but correct behavior. ``` This issue tracks implementing a proper fix rather than accepting the workaround. ## Root Cause The `ClientNavigationState` tracks **pending params** but **not pending pathname**: ```typescript type ClientNavigationState = { // ... clientParams: Record<string, string | string[]>; pendingClientParams: Record<string, string | string[]> | null; // ✅ Tracked cachedPathname: string; // ❌ Only committed, no pending pathname // ... } ``` ## Proposed Solution ### Option A: Track Pending Pathname (Recommended) Add `pendingPathname` to `ClientNavigationState` in `navigation.ts`: ```typescript type ClientNavigationState = { // ... existing fields pendingPathname: string | null; cachedPathname: string; } ``` **Flow changes:** 1. Set `pendingPathname` immediately when `__VINEXT_RSC_NAVIGATE__` starts 2. Update `isSameRoute` comparison: ```typescript const currentPath = state?.pendingPathname ?? state?.cachedPathname ?? window.location.pathname; const isSameRoute = stripBasePath(url.pathname, __basePath) === stripBasePath(currentPath, __basePath); ``` 3. Clear `pendingPathname` in `commitClientNavigationState()` after successful commit 4. Handle error cases: clear on navigation failure/supersession **Complexity:** Requires restructuring navigation flow because `isSameRoute` is evaluated **before** `stageClientParams()` in both cached and non-cached paths. ### Option B: Track In-Flight Navigation Destination Add a `destinationPathname` field set immediately when navigation starts, before any async work. This separates "where we are going" from "params we are staging". ## Edge Cases to Handle - [ ] **Overlapping navigations (A→B→C):** When C starts, should compare against B (most recent pending), not A. The `navId`/`activeNavigationId` system already supersedes older navigations. - [ ] **Navigation superseded before commit:** If B→C starts but is superseded by B→D, pending pathname should update to D. Cleanup must be robust. - [ ] **Error during navigation:** If navigation fails, `pendingPathname` must be cleared in catch blocks. Current `_snapshotPending` pattern shows the complexity of cleanup. - [ ] **Back/forward (traverse) navigation:** Different code path via `popstate` handler. Uses `window.location.href` directly, less affected. ## Files Affected - `packages/vinext/src/shims/navigation.ts` — add `pendingPathname` to state type, update `commitClientNavigationState()` - `packages/vinext/src/server/app-browser-entry.ts` — update `isSameRoute` logic, set/clear pending pathname ## Why This Matters Now 1. **UX regression:** Rapid navigation is a common user pattern. The current workaround produces visible jank. 2. **Self-contained fix:** ~30 lines changed, does not depend on major refactors like #726 (layout persistence architecture). 3. **Code health:** The comment acknowledges this as debt. Better to fix properly than let it linger. ## Related - PR #690 (Firefox navigation hang fix) — introduced the workaround comment - Review comment: https://github.com/cloudflare/vinext/pull/690#discussion_r2034234561 - Issue #726 — Layout persistence architecture (orthogonal to this race condition) ## Testing Needed - [ ] E2E test for rapid navigation sequence (click A→B→C without waiting for intermediate commits) - [ ] Unit test for `isSameRoute` classification with simulated pending navigation - [ ] Verify no regression in back/forward navigation - [ ] Verify error handling clears pending state correctly