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 13 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
|
/bigbonk review |
There was a problem hiding this comment.
Review: pending pathname race condition fix (round 2)
The core approach is sound — tracking the intended destination of in-flight navigations via pendingPathname to fix stale isSameRoute comparisons during rapid navigation. The catch block bug from the previous review has been addressed (now guarded with navId === activeNavigationId). A few remaining issues:
PR description claims unit tests that don't exist
The PR body says "Add unit tests for helper functions" and "Unit tests: 4 tests for helper functions," but no unit test file is present in the diff. This appears to reference a navigation-pending-pathname.test.ts file that was either removed or never committed. The PR description should be updated to match what's actually included.
E2E test concern: "same-route query change" test doesn't test rapid navigation
The test on line 46 claims to test "same-route query change during cross-route navigation" but it can't work as described — see inline comment.
Minor: redundant isServer guard
See inline comment on setPendingPathname.
| await page.click('a[href="/nav-rapid/page-b"]'); | ||
|
|
||
| // Immediately change query param via client-side navigation (same-route nav to B with query) | ||
| await page.click('a[href="/nav-rapid/page-b?filter=test"]'); |
There was a problem hiding this comment.
This page.click targets a[href="/nav-rapid/page-b?filter=test"], but that link only exists on Page B (the "Add Filter" link). Page A only has links to /nav-rapid/page-b and /nav-rapid/page-c.
So this click can only succeed after Page B has fully rendered from the click on line 60, meaning both navigations run sequentially — not rapidly. This test doesn't actually exercise the "same-route query change during cross-route navigation" scenario it describes.
To actually test rapid same-route-then-query navigation, the ?filter=test link needs to be accessible before Page B renders. Options:
- Add the
?filter=testlink to Page A as well - Use
page.evaluate(() => window.__VINEXT_RSC_NAVIGATE__('/nav-rapid/page-b?filter=test'))immediately after the click on line 60 to trigger a programmatic client-side navigation without waiting for the DOM
| */ | ||
| export function setPendingPathname(pathname: string): void { | ||
| const state = getClientNavigationState(); | ||
| if (!state) return; |
There was a problem hiding this comment.
Minor: the isServer guard here is redundant. getClientNavigationState() on line 739 already returns null when isServer is true (line 491), so this collapses to just if (!state) return — which is what clearPendingPathname uses at line 749. Consider making them consistent:
| if (!state) return; | |
| if (!state) return; |
|
Review posted successfully. Here's a summary of the feedback: Core approach is sound. The Three remaining issues:
|
- Add 'Go to B with Filter' link to Page A for proper rapid nav testing - Add 4 unit tests for setPendingPathname/clearPendingPathname helpers
|
/bigbonk review |
There was a problem hiding this comment.
Review: pending pathname race condition fix (round 3)
The core approach is correct and the implementation is clean. The catch-block bug from round 1 has been properly addressed, and the E2E test fixture for the "same-route query change" scenario was fixed by adding the ?filter=test link to Page A (so the second click can fire before Page B renders). Two minor items remain.
Remaining issues
-
Redundant
isServerguard insetPendingPathname— Flagged in both prior reviews and still unaddressed. One-line fix. -
Unit tests are still no-ops — The tests in
shims.test.tsonly verify the functions don't throw on the server. Since both functions early-return whengetClientNavigationState()returnsnull(which it always does on the server), these tests assert thatreturn;doesn't throw. That's trivially true for any function. They add no regression coverage for the actual pending pathname state machine.The tests aren't wrong — they're just not useful. If you want to keep them, that's fine, but they shouldn't be counted as behavioral coverage in the PR description. The E2E tests are doing the real work here.
What looks good
- The
pendingPathname → cachedPathname → window.location.pathnamefallback chain is correct.cachedPathnameis the base-path-stripped last-committed URL, which is the right intermediate source. - Reading state before calling
setPendingPathname(line 621 before line 632) ensures the current navigation compares against the previous pending target, not its own. - The catch-block guard (
navId === activeNavigationId) correctly prevents a failing superseded navigation from clobbering the active navigation's pending pathname. commitClientNavigationState()clearingpendingPathnameon success is the right place — it's wherecachedPathnamegets synced fromwindow.location, so the pending value is no longer needed.- The superseded-navigation early-return points (lines 646, 655, 696, 734, 740) correctly do not clear
pendingPathname, since the newer navigation has already overwritten it.
| */ | ||
| export function setPendingPathname(pathname: string): void { | ||
| const state = getClientNavigationState(); | ||
| if (!state) return; |
There was a problem hiding this comment.
Still unaddressed from both prior reviews: getClientNavigationState() already returns null when isServer is true (line 491), so the isServer check here is redundant. clearPendingPathname at line 749 only checks if (!state) — these should be consistent.
| if (!state) return; | |
| if (!state) return; |
tests/shims.test.ts
Outdated
|
|
||
| // On server, getClientNavigationState returns null | ||
| // This should not throw and should be a no-op | ||
| expect(() => setPendingPathname("/some-path")).not.toThrow(); |
There was a problem hiding this comment.
This test verifies that calling a function which does if (!state) return; doesn't throw — that's trivially true for any function with a guard clause. It doesn't exercise the pending pathname state machine at all.
Not blocking, but these shouldn't be described as meaningful unit test coverage in the PR description. The E2E tests are where the real coverage lives.
|
Review posted successfully. Here's a summary: PR #745 — Round 3 Review The core fix is sound: tracking Two minor items remain:
|
Update test descriptions to acknowledge these are smoke tests for server-side safety, not behavioral unit tests. The real coverage is in E2E tests. Addresses review feedback from PR cloudflare#745.
Prevents stale pendingPathname from affecting isSameRoute calculations during rapid successive navigations (A→B→C). When a new navigation to a different route starts, clear the pending pathname so subsequent navigations compare against the correct current location. Fixes cloudflare#744
The fix for issue cloudflare#744 introduced a race condition where clearing pendingPathname when navigating to a different route broke the navigation chain during rapid successive navigations (A→B→C). Instead of conditionally clearing, we now always overwrite the pending pathname with the new target. This ensures subsequent navigations always compare against the most recent in-flight target without race conditions. Fixes CI failure in E2E app-router tests.
… pathname state Remove the navId === undefined check from commitClientNavigationState condition to ensure only the navigation that set pendingPathname (or a newer navigation) can clear it. This prevents superseded navigations called via drainPrePaintEffects from clearing state belonging to active navigations during rapid successive clicks (A→B→C pattern). Fixes race condition in rapid-navigation E2E tests where URL ended up at intermediate state instead of final target.
Divkix
force-pushed
the
fix/744-pending-pathname-race
branch
from
2616cad to
6299b3d
Compare
… pathname state The fix for issue cloudflare#744 introduced a race condition where superseded navigations could clear pending pathname state belonging to newer navigations during rapid successive clicks (A→B→C pattern). Changes: - Remove navId === undefined check from commitClientNavigationState() condition to ensure only the navigation that set pendingPathname (or a newer one) can clear it - Fix E2E tests to use synchronous clicking via page.evaluate() to avoid React re-render race conditions where clicks land on wrong elements during DOM transitions - Add page-specific data-testid attributes to fixture links for more robust test selectors Fixes rapid-navigation E2E test failures where URL ended up at intermediate state (page-b?filter=test) instead of final target (page-c).
Divkix
force-pushed
the
fix/744-pending-pathname-race
branch
from
1f62b07 to
cbd5049
Compare
Rapid back-to-back client-side navigations (clicking link B then immediately link C in a single page.evaluate()) abort the first navigation, causing page.waitForURL() to fail with ERR_ABORTED since it waits for a 'load' navigation event. Use expect(page).toHaveURL() instead, which polls the URL without requiring a navigation event — correct for soft client-side navigations. Fixes CI failure on fix/744-pending-pathname-race branch.
|
Finally CI passes |
|
@Divkix hey, you wanna take a crack at phase 4? #726 (comment) It should be clean once #750 gets merged. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: pending pathname race condition fix
The core approach is correct — tracking pendingPathname on the client navigation state to prevent stale isSameRoute comparisons during rapid A→B→C navigations. The navId-based ownership model for clearing pendingPathname is a solid design that prevents superseded navigations from clobbering active state.
The previous review rounds caught the important bugs (catch-block clobbering, E2E fixture links). What remains are two minor items — one is a real (if low-risk) correctness issue, the other a consistency nit.
Overall the PR is in good shape. The E2E tests are well-structured and cover the important scenarios.
| @@ -767,6 +796,12 @@ async function main(): Promise<void> { | |||
| _snapshotPending = false; | |||
| commitClientNavigationState(); | |||
There was a problem hiding this comment.
This commitClientNavigationState() call (without navId) runs when _snapshotPending is true in the outer catch. It happens to be safe because commitClientNavigationState(undefined) won't match any pendingPathnameNavId that's a number — the condition state.pendingPathnameNavId === navId will be number === undefined which is false.
But this is fragile and relies on an implicit type mismatch. If someone later changes the navId parameter type or the clearing condition, this could silently start clobbering pending pathnames from other navigations.
Since navId is available in scope here, it would be safer and more consistent to pass it:
| commitClientNavigationState(); | |
| commitClientNavigationState(navId); |
This makes the intent explicit and matches the pattern used in createNavigationCommitEffect, renderNavigationPayload, and updateBrowserTree.
| */ | ||
| export function setPendingPathname(pathname: string, navId: number): void { | ||
| const state = getClientNavigationState(); | ||
| if (!state) return; |
There was a problem hiding this comment.
Flagged in all three prior review rounds and still unaddressed. getClientNavigationState() returns null when isServer is true (line 491), so the isServer check is redundant. clearPendingPathname at line 755 only checks if (!state) return — these two functions should be consistent.
| if (!state) return; | |
| if (!state) return; |
| '[data-testid="page-a-link-to-b-filter"]', | ||
| ) as HTMLElement; | ||
| if (linkB) linkB.click(); | ||
| if (linkFilter) linkFilter.click(); |
There was a problem hiding this comment.
This test is improved from the earlier version (the ?filter=test link is now on Page A), but the scenario it exercises is subtle: both clicks happen synchronously in the same microtask, so the second navigateRsc call runs immediately after the first increments activeNavigationId. The first navigation's setPendingPathname('/nav-rapid/page-b', 1) is called, then the second navigation reads pendingPathname (which is /nav-rapid/page-b, base-path-stripped) and compares against its own target (/nav-rapid/page-b). So isSameRoute is true for the second nav, which is correct — it's a same-route query-param change.
The test validates the outcome correctly, but a brief inline comment explaining why the second navigation sees isSameRoute === true (pendingPathname from the first click matches the second click's pathname) would help future readers understand what this is actually testing.
| test("A→B→C rapid navigation completes smoothly", async ({ page }) => { | ||
| // Collect console errors during the test | ||
| const errors: string[] = []; | ||
| page.on("console", (msg) => { | ||
| if (msg.type() === "error") { | ||
| errors.push(msg.text()); | ||
| } | ||
| }); | ||
|
|
||
| // Start at page A | ||
| await page.goto(`${BASE}/nav-rapid/page-a`); | ||
| await expect(page.locator("h1")).toHaveText("Page A"); | ||
| await waitForHydration(page); | ||
|
|
||
| // Click B then immediately click C (before B fully commits) | ||
| // Use a single page.evaluate() to click both links atomically. | ||
| // This avoids React re-render issues and prevents "execution context destroyed" errors in CI. | ||
| await page.evaluate(() => { | ||
| const linkB = document.querySelector('[data-testid="page-a-link-to-b"]') as HTMLElement; | ||
| const linkC = document.querySelector('[data-testid="page-a-link-to-c"]') as HTMLElement; | ||
| if (linkB) linkB.click(); | ||
| if (linkC) linkC.click(); | ||
| }); | ||
|
|
||
| // Use toHaveURL (polling) instead of waitForURL (navigation event) because | ||
| // rapid back-to-back client-side navigations abort the first navigation, | ||
| // causing waitForURL to fail with ERR_ABORTED. | ||
| await expect(page).toHaveURL(`${BASE}/nav-rapid/page-c`, { timeout: 10_000 }); | ||
| await expect(page.locator("h1")).toHaveText("Page C"); | ||
|
|
||
| const navigationErrors = errors.filter( | ||
| (e) => e.includes("navigation") || e.includes("vinext") || e.includes("router"), | ||
| ); | ||
| expect(navigationErrors).toHaveLength(0); |
There was a problem hiding this comment.
Nit: Tests 1 and 4 ("A→B→C rapid navigation" and "interrupted navigation") are functionally identical — they both click B then C atomically from Page A and assert the final URL is /nav-rapid/page-c. The only difference is test 4 also checks a window.__NAV_MARKER__ to verify no full-page reload.
Consider merging these into a single test, or adding the marker check to test 1 and removing test 4. Having two tests that exercise the exact same code path (with the same clicks, same assertions on URL and heading) adds maintenance surface without additional coverage.
|
Review posted successfully. Here's a summary of the feedback: Overall: The PR is in good shape. The 4 comments posted:
|
3 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