fix(navigation): track pending pathname to fix isSameRoute race condition during rapid navigation#745
fix(navigation): track pending pathname to fix isSameRoute race condition during rapid navigation#745Divkix wants to merge 4 commits intocloudflare:mainfrom
Conversation
…tion during rapid navigation Fixes race condition where isSameRoute compared against stale window.location.pathname during rapid successive navigations (A→B→C). Previously, when navigation C started before navigation B committed, C would compare against A instead of B, causing janky transitions. Changes: - Add pendingPathname field to ClientNavigationState in navigation.ts - Export setPendingPathname() and clearPendingPathname() helpers - Update isSameRoute logic in app-browser-entry.ts to compare against: 1. pendingPathname (previous in-flight navigation) 2. cachedPathname (last committed navigation) 3. window.location.pathname (fallback) - Clear pendingPathname on navigation commit and error - Add unit tests for helper functions - Add E2E tests for rapid navigation scenarios The fix ensures correct same-route vs cross-route classification even when URL commits are deferred via useLayoutEffect. Closes cloudflare#744
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: pending pathname race condition fix
The approach — tracking the intended destination of in-flight navigations to fix stale isSameRoute comparisons — is the right idea. The core change is small and well-placed. A few issues to address though:
Bug: clearPendingPathname() in catch block can clobber active navigation
This is the main concern. When a superseded navigation errors in the catch block, clearPendingPathname() at line 778 runs unconditionally before the navId !== activeNavigationId stale check at line 781. This means a failing old navigation clears the pendingPathname that was set by the newer active navigation.
In practice, this is likely low-impact because the newer navigation already read pendingPathname before the older one's catch fires (the isSameRoute comparison at line 625-629 happens synchronously at the top of navigateRsc). But it leaves the state incorrect: if a third navigation fires after the catch clears it, it would fall through to cachedPathname instead of seeing the second navigation's pending value. The fix is simple: guard clearPendingPathname() behind the same stale check, or move it after the stale check.
Unit tests provide minimal coverage
The unit tests in navigation-pending-pathname.test.ts only verify that the functions are exported and have the correct .length. They don't test any behavior — setPendingPathname is never called on actual state, clearPendingPathname is never verified to clear anything, and the interaction with commitClientNavigationState is not tested. These are essentially no-ops as regression tests. Given that the helpers early-return in server/test environments (isServer check, no window), the functions literally do nothing when called in these tests.
The E2E tests are well-structured and cover the important scenarios. But the unit test file should either be removed (if you're relying solely on E2E coverage) or replaced with tests that actually exercise the state machine — e.g., using jsdom or mocking getClientNavigationState.
E2E test concern: page.goto() is not a client-side navigation
See inline comment on the "same-route query change" test.
| commitClientNavigationState(); | ||
| } | ||
| // Clear pending pathname on error so subsequent navigations compare correctly | ||
| clearPendingPathname(); |
There was a problem hiding this comment.
Bug: clearPendingPathname() runs unconditionally, but this catch block also handles superseded navigations (line 781 checks navId !== activeNavigationId). A superseded navigation's error handler will clear the pendingPathname that was set by the active (newer) navigation.
Should be guarded the same way as the _snapshotPending cleanup above it — only clear if this is still the active navigation:
| clearPendingPathname(); | |
| // Clear pending pathname on error so subsequent navigations compare correctly. | |
| // Only clear if this is still the active navigation — a newer navigation | |
| // has already overwritten pendingPathname with its own target. | |
| if (navId === activeNavigationId) { | |
| clearPendingPathname(); | |
| } |
Alternatively, move it after the stale check at line 781.
| import { describe, it, expect } from "vite-plus/test"; | ||
|
|
||
| describe("navigation state - pendingPathname helpers", () => { | ||
| describe("type definition", () => { | ||
| it("exports setPendingPathname function", async () => { | ||
| const nav = await import("../packages/vinext/src/shims/navigation.js"); |
There was a problem hiding this comment.
These tests only verify that the functions are exported and have the expected .length — they don't test any actual behavior. Since setPendingPathname and clearPendingPathname early-return when isServer is true or getClientNavigationState() returns null (which it does in a test environment without window), these functions are literally no-ops when called.
Either:
- Remove this file and rely on the E2E tests for coverage, or
- Replace with tests that actually exercise the state machine (mock
window, verify thatgetClientNavigationState().pendingPathnamechanges aftersetPendingPathname, verifycommitClientNavigationStateclears it, etc.)
As-is, these tests give a false sense of coverage.
| await page.click('a[href="/nav-rapid/page-b"]'); | ||
|
|
||
| // Immediately change query param (same-route navigation to B with query) | ||
| await page.goto(`${BASE}/nav-rapid/page-b?filter=test`); |
There was a problem hiding this comment.
page.goto() is a full page navigation (hard nav), not a client-side SPA navigation. This doesn't test the "same-route query change during cross-route navigation" scenario described in the test name — it tests "hard navigation interrupts a pending client-side navigation."
To actually test a same-route query change via client-side navigation, you'd need to trigger router.push('/nav-rapid/page-b?filter=test') via page.evaluate after the click, or add a link on page B that navigates to ?filter=test.
The test still has value (verifying hard nav during pending client nav doesn't break), but the name and comments are misleading.
| */ | ||
| export function setPendingPathname(pathname: string): void { | ||
| const state = getClientNavigationState(); | ||
| if (!state || isServer) return; |
There was a problem hiding this comment.
Minor: the isServer guard here is redundant. getClientNavigationState() already returns null when isServer is true (line 491), so the if (!state || isServer) return collapses to just if (!state) return — which is what clearPendingPathname uses at line 751. Consider making them consistent:
| if (!state || isServer) return; | |
| if (!state) return; |
|
Review posted successfully. Here's a summary of the feedback: Key findings
|
…dition fix This commit addresses all issues raised in PR review cloudflare#745: 1. Fix race condition in catch block (app-browser-entry.ts) - Guard clearPendingPathname() with navId === activeNavigationId check - Prevents superseded navigations from clobbering pendingPathname of active nav - Ensures state consistency when rapid successive navigations occur 2. Remove useless unit tests (navigation-pending-pathname.test.ts) - Deleted file that only tested typeof/.length, not actual behavior - Functions early-return in test environment (no window), making tests no-ops - E2E tests provide sufficient coverage for this functionality 3. Fix E2E test to use client-side navigation (rapid-navigation.spec.ts) - Changed page.goto() (hard nav) to Link click (client-side SPA nav) - Added query param link to page-b fixture - Test now correctly validates same-route query change scenario 4. Remove redundant isServer guard (navigation.ts) - getClientNavigationState() already returns null when isServer is true - Made consistent with clearPendingPathname() which only checks (!state) All review feedback items addressed: - clearPendingPathname() race condition fixed - Unit tests deleted (no false coverage) - E2E test uses correct navigation type - Code consistency improved
- Update comment to reference getClientTreeshakeConfigForVite instead of deprecated clientTreeshakeConfig
- Remove redundant not.toHaveProperty('preset') assertions from tests (toEqual already does exact matching)
Addresses review feedback from @ask-bonk[bot] on PR cloudflare#745
* fix(build): eliminate Vite 8 treeshake.preset warning Add version-gated getClientTreeshakeConfigForVite() function that returns Rollup-compatible config (with preset) for Vite 7 and Rolldown-compatible config (without preset) for Vite 8+. The treeshake.preset option is Rollup-specific and causes warnings in Vite 8 which uses Rolldown. The moduleSideEffects: 'no-external' option is valid in both bundlers and is preserved in all configurations. Changes: - Add getClientTreeshakeConfigForVite(viteMajorVersion) function - Update 4 usage sites to use version-gated function - Mark clientTreeshakeConfig as @deprecated for backward compatibility - Export getClientTreeshakeConfigForVite for testing - Add comprehensive tests for Vite 7/8/9+ compatibility - Update existing tests to expect Vite 8 format Fixes #540 * refactor(build): address PR review feedback for treeshake config Remove dead workaround plugin from playground - Deleted strip-rolldown-incompatible-rollup-options plugin from examples/app-router-playground/vite.config.ts - This workaround is no longer needed since getClientTreeshakeConfigForVite() never emits treeshake.preset for Vite 8+ Add documentation comment about Rolldown defaults - Enhanced JSDoc for getClientTreeshakeConfigForVite() to explain behavior gap - Documents how Rolldown defaults compare to Rollup's recommended preset - Clarifies that moduleSideEffects: no-external is the key optimization Fix test import style - Changed from dynamic await import() to static imports - Matches existing test patterns in build-optimization.test.ts - Removes async from test cases that don't need it Addresses review comments from PR #746 * docs(build): clarify Rolldown treeshake divergence in comments Address PR #746 review feedback from @ask-bonk: - JSDoc: Explicitly call out unknownGlobalSideEffects as a known acceptable divergence (Rolldown defaults to true vs Rollup recommended preset's false). Makes it clear this is intentional, not an oversight. - Inline comment: Note that Rolldown's built-in defaults already cover what Rollup's 'recommended' preset provides (annotations, correctContext, tryCatchDeoptimization). Provides immediate context at the code site. Both changes are documentation-only; no functional impact. * chore: address PR review feedback on treeshake config - Update comment to reference getClientTreeshakeConfigForVite instead of deprecated clientTreeshakeConfig - Remove redundant not.toHaveProperty('preset') assertions from tests (toEqual already does exact matching) Addresses review feedback from @ask-bonk[bot] on PR #745
2 participants
Fixes #744
Problem
During rapid successive navigations (e.g., user clicks A→B→C quickly), the "isSameRoute" check in app-browser-entry.ts compared against stale window.location.pathname because URL commits are deferred via useLayoutEffect.
Solution
Add pendingPathname to ClientNavigationState to track the destination URL of in-flight navigations. This allows isSameRoute to compare against the most recent pending destination instead of stale window.location.pathname.
Changes
Edge Cases Handled
Testing