fix: prevent URL/content mismatch on rapid Pages Router navigation#782
fix: prevent URL/content mismatch on rapid Pages Router navigation#782NathanDrake2406 wants to merge 7 commits intocloudflare:mainfrom
Conversation
Replace the boolean `_navInProgress` guard in navigateClient() with a monotonically increasing navigation ID + AbortController. The old guard silently dropped the second navigation when two push() calls overlapped, leaving the URL bar showing B while page content showed A. Now each navigateClient() call: - Aborts the previous in-flight fetch via AbortController - Checks after every await whether a newer navigation has superseded it - Throws NavigationCancelledError (with .cancelled = true) when stale Callers (push/replace in both hook and singleton, plus popstate) catch the error and emit routeChangeError for cancelled navigations, matching Next.js behavior. routeChangeComplete only fires for navigations that actually render. Also fixes: failed navigations (non-OK response, missing data) now emit routeChangeError instead of silently emitting routeChangeComplete before the hard redirect.
commit: |
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
|
@copilot review |
|
/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: Pages Router concurrent navigation fix
The core approach — replacing a boolean _navInProgress guard with a monotonic navigation ID + AbortController — is the right fix. The old guard silently dropped the second navigation, leaving URL and content out of sync. The new pattern correctly lets the last navigation win and cancels stale ones. Good reference to Next.js's clc / getCancelledHandler pattern.
The tests are well-structured with proper cleanup in finally blocks and cover the key scenarios (overlapping push, overlapping push+replace, failed navigation, event emission correctness). I have a few concerns below, one of which is a potential user-visible bug.
Key concern: URL bar left pointing at cancelled navigation's target
When two rapid push() calls overlap, the sequence is:
push("/page-a")→pushState("/page-a")→navigateClient("/page-a")push("/page-b")→pushState("/page-b")→navigateClient("/page-b")- Navigation A is cancelled, throws
NavigationCancelledError - Navigation B completes, content renders for
/page-b
This works correctly because B's pushState overwrites A's before B's navigateClient finishes. But consider a different scenario: A is cancelled and B also fails (network error, non-OK response). Now the URL bar shows /page-b but the page content is still from the original / page. The window.location.href = url assignment inside navigateClient will trigger a hard navigation for B's failure, but A's pushState has already polluted the history stack with an entry for /page-a that was never rendered.
This is a pre-existing issue (not introduced by this PR), and Next.js has the same pattern where changeState is called before fetching. Noting it for awareness rather than blocking.
Nit: duplicated error-handling blocks
The identical try/catch pattern for navigateClient appears in 4 places: useRouter().push, useRouter().replace, Router.push, Router.replace. This is a maintenance burden — if the error handling needs to change (e.g., adding logging, reverting pushState on cancel), all 4 must be updated in sync. Consider extracting a helper. Not blocking, but worth considering as a follow-up.
| return; | ||
| const err = new Error(`Navigation failed: ${res.status} ${res.statusText}`); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Ordering concern: window.location.href = url triggers a hard navigation, then throw err fires. In a real browser, throw would likely still execute because location.href assignment doesn't synchronously unload the page. But the thrown error propagates to the caller, which emits routeChangeError — is that the intended behavior?
In the old code, these were early returns, so the caller proceeded to emit routeChangeComplete (which was wrong, hence the fix). The new throw correctly prevents routeChangeComplete, but the caller now emits routeChangeError and the page is doing a hard navigation. This double-action seems intentional (the error event lets listeners know what happened before the page unloads), but it means routeChangeError listeners must be synchronous to observe the event before the hard nav triggers. Worth a brief comment explaining this contract.
packages/vinext/src/shims/router.ts
Outdated
| await navigateClient(full); | ||
| } catch (err: unknown) { | ||
| const routeProps = { shallow: false }; | ||
| if (err instanceof NavigationCancelledError) { |
There was a problem hiding this comment.
When NavigationCancelledError is caught, the method returns true (success). This means calling code like const ok = await router.push('/page-a') gets true even though the navigation was cancelled and the content never loaded for /page-a.
Next.js's router.push also returns true for cancelled navigations (because the intent was replaced by a newer navigation, not because it failed), so this matches. But it might be worth a comment here explaining the rationale, since return true for a cancelled navigation is counterintuitive.
There was a problem hiding this comment.
The semantics are now documented in runNavigateClient()'s return type and JSDoc — "cancelled" is explicitly distinguished from "completed" and "failed", making the caller's intent clearer than a boolean.
| restoreScrollPosition(e.state); | ||
| window.dispatchEvent(new CustomEvent("vinext:navigate")); | ||
| }, | ||
| (err: unknown) => { |
There was a problem hiding this comment.
Nit: the if branch and the fallthrough do the exact same thing (routerEvents.emit("routeChangeError", ...)). The instanceof check here doesn't change behavior — both paths emit the same event. If the intent is to differentiate in the future (e.g., log non-cancellation errors, or attempt a hard navigation fallback for non-cancel errors), this makes sense as a structural placeholder. But as-is, it could be simplified to a single routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false }) without the if.
Same applies to the 4 identical try/catch blocks in push/replace.
There was a problem hiding this comment.
This is now moot — the instanceof check lives in runNavigateClient() once, not 4 times. The three-way return type ("completed" | "cancelled" | "failed") makes the branching explicit.
| if (_navInProgress) return; | ||
| _navInProgress = true; | ||
| // Cancel any in-flight navigation (abort its fetch, mark it stale) | ||
| _activeAbortController?.abort(); |
There was a problem hiding this comment.
Good: aborting the previous controller before creating a new one is the correct order. If the abort handler in the previous navigation's fetch runs synchronously (some environments do this), it will see _navigationId already incremented and correctly throw NavigationCancelledError.
One edge case to consider: if navigateClient is called with the same URL (e.g., user double-clicks a link), the current navigation aborts itself. This is arguably correct (the second call takes over), but it does mean the first fetch is wasted even though it was fetching the same resource. Not a bug, just a minor inefficiency — and it matches Next.js behavior.
| (globalThis as any).window = win; | ||
|
|
||
| // Two deferred fetches so we control resolution order | ||
| const fetchA = createDeferred<Response>(); |
There was a problem hiding this comment.
The test resolves fetchB before fetchA, which is the scenario where the winning navigation's response arrives first. It would also be valuable to test the reverse: resolve fetchA first (stale response arrives before the winning one). The cancel mechanism should still work because assertStillCurrent() fires after each await, but the timing is different — the stale navigation gets further through the pipeline before being killed.
Not blocking, but would strengthen confidence in the assertStillCurrent checkpoint coverage.
|
|
||
| /** | ||
| * Create a deferred promise for controlling fetch timing. | ||
| */ |
There was a problem hiding this comment.
The mock fetch doesn't check _init.signal — it ignores the AbortController's signal entirely. This means the tests don't actually verify that the AbortController integration works (i.e., that a real fetch would throw AbortError when the signal fires). The deferred pattern tests the navigation ID mechanism, but the abort signal path is untested.
Consider adding a test where the mock fetch respects the signal:
| */ | |
| globalThis.fetch = async (_url: any, _init: any) => { | |
| fetchCount++; | |
| if (fetchCount === 1) { | |
| // Simulate a real fetch that rejects on abort | |
| return new Promise<Response>((resolve, reject) => { | |
| fetchA.promise.then(resolve, reject); | |
| _init?.signal?.addEventListener('abort', () => { | |
| reject(new DOMException('The operation was aborted.', 'AbortError')); | |
| }); | |
| }); | |
| } | |
| return fetchB.promise; | |
| }; |
|
Review submitted successfully. Here's a summary of the feedback: Overall: The core fix is correct and well-motivated. Replacing the boolean Key inline comments:
|
…ch blocks - Add comment to !res.ok path explaining the window.location.href + throw ordering contract: the assignment schedules navigation as a task, so synchronous routeChangeError listeners observe the error before unload. Document the contract requirement: listeners MUST be synchronous. - Simplify all 4 push/replace catch blocks: extract routerEvents.emit() before the instanceof check so the emit is unconditional and the if-branch only determines the return value. Add clarifying comment explaining why cancelled navigations return true (superseded, not failed — matches Next.js behaviour). - Simplify popstate .then() rejection handler: remove redundant if/instanceof guard since both paths emitted identical arguments; add comment noting there is no return-value distinction in a void handler.
Two new tests for the Pages Router concurrent navigation suite: 1. "abort signal fires when navigation is superseded" — uses a signal-aware fetch mock where the first fetch rejects with AbortError when its signal is aborted. Verifies the AbortError → NavigationCancelledError conversion path end-to-end (i.e. the abort signal integration actually works, not just the assertStillCurrent checkpoint). 2. "stale response arriving first" variant — fetchA (stale) resolves before fetchB (winner). Exercises assertStillCurrent() checkpoints when the stale navigation gets further through the pipeline (past the fetch, into HTML parsing) before being cancelled. Verifies stale data never reaches the DOM.
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the vinext Pages Router shim where rapid overlapping navigations (e.g., multiple push() calls) could leave the browser URL showing one route while the rendered content (and __NEXT_DATA__) reflected another. It replaces a boolean re-entrancy guard with a monotonic navigation ID and an AbortController to ensure “last navigation wins”, and aligns route event semantics more closely with Next.js by emitting routeChangeError for cancellations and failures.
Changes:
- Replace
_navInProgresswith_navigationId+AbortControllerinnavigateClient()to cancel/ignore stale navigations. - Emit
routeChangeErrorfor superseded navigations (.cancelled = true) and for failed navigations (non-OK / missing__NEXT_DATA__). - Add a new test suite validating concurrent navigation behavior and event emission.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
packages/vinext/src/shims/router.ts |
Implements navigation cancellation via ID + abort, and updates push/replace/popstate to emit routeChangeError on rejection. |
tests/shims.test.ts |
Adds concurrent navigation tests covering “last wins”, cancellation semantics, and failure event behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -393,28 +456,31 @@ async function navigateClient(url: string): Promise<void> { | |||
|
|
|||
| if (!pageModuleUrl) { | |||
| window.location.href = url; | |||
| return; | |||
| throw new Error("Navigation failed: no page module URL found"); | |||
| } | |||
|
|
|||
| // Validate the module URL before importing — defense-in-depth against | |||
| // unexpected __NEXT_DATA__ or malformed HTML responses | |||
| if (!isValidModulePath(pageModuleUrl)) { | |||
| console.error("[vinext] Blocked import of invalid page module path:", pageModuleUrl); | |||
| window.location.href = url; | |||
| return; | |||
| throw new Error("Navigation failed: invalid page module path"); | |||
| } | |||
|
|
|||
| // Dynamically import the new page module | |||
| const pageModule = await import(/* @vite-ignore */ pageModuleUrl); | |||
| assertStillCurrent(); | |||
|
|
|||
| const PageComponent = pageModule.default; | |||
|
|
|||
| if (!PageComponent) { | |||
| window.location.href = url; | |||
| return; | |||
| throw new Error("Navigation failed: page module has no default export"); | |||
| } | |||
|
|
|||
| // Import React for createElement | |||
| const React = (await import("react")).default; | |||
| assertStillCurrent(); | |||
|
|
|||
| // Re-render with the new page, loading _app if needed | |||
| let AppComponent = window.__VINEXT_APP__; | |||
| @@ -433,6 +499,7 @@ async function navigateClient(url: string): Promise<void> { | |||
| } | |||
| } | |||
| } | |||
| assertStillCurrent(); | |||
|
|
|||
| let element; | |||
| if (AppComponent) { | |||
| @@ -448,12 +515,11 @@ async function navigateClient(url: string): Promise<void> { | |||
| element = wrapWithRouterContext(element); | |||
|
|
|||
| root.render(element); | |||
| } catch (err) { | |||
| console.error("[vinext] Client navigation failed:", err); | |||
| routerEvents.emit("routeChangeError", err, url, { shallow: false }); | |||
| window.location.href = url; | |||
| } finally { | |||
| _navInProgress = false; | |||
| // Clean up the abort controller if this navigation is still the active one | |||
| if (navId === _navigationId) { | |||
| _activeAbortController = null; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
navigateClient() no longer has a catch that falls back to hard navigation on unexpected failures. Several operations here can still throw (e.g. JSON.parse(match[1]), dynamic import(pageModuleUrl), import('react'), root.render), but in those cases the caller currently only emits routeChangeError and returns false — leaving the URL already updated via pushState/replaceState/popstate while the old content stays rendered. Consider reintroducing a catch in navigateClient (or adding equivalent fallback in every caller) that, for non-cancel errors, sets window.location.href = url before rethrowing so the browser reliably lands on the correct page.
There was a problem hiding this comment.
Fixed. runNavigateClient() now does window.location.href = fullUrl for non-cancel errors, restoring the hard-navigation recovery that the old code had.
tests/shims.test.ts
Outdated
| const completedUrls: string[] = []; | ||
| Router.events.on("routeChangeComplete", (...args: unknown[]) => { | ||
| completedUrls.push(String(args[0])); | ||
| }); | ||
|
|
There was a problem hiding this comment.
These tests attach Router.events handlers but never detach them. Since routerEvents is a module-level singleton and the module is cached across tests, listeners from earlier cases will accumulate and continue firing in later tests (and across the whole shims test suite), which can cause leaks/flakiness. Suggest storing the handler functions and calling Router.events.off(...) in the test finally blocks, or using vi.resetModules() + fresh import per test to isolate the event emitter.
There was a problem hiding this comment.
These listeners are already cleaned up in the finally block — see lines 8606-8608 where Router.events.off("routeChangeComplete", onRouteChangeComplete) runs. All 7 tests have equivalent cleanup.
tests/shims.test.ts
Outdated
| const errors: Array<{ err: unknown; url: string }> = []; | ||
| Router.events.on("routeChangeError", (...args: unknown[]) => { | ||
| errors.push({ err: args[0], url: String(args[1]) }); | ||
| }); |
There was a problem hiding this comment.
This Router.events.on("routeChangeError", ...) handler is never removed. Because Router.events is a module-level singleton, handlers will accumulate across test cases when the module is cached. Please call Router.events.off(...) in finally (or reset modules) to keep tests isolated and avoid leaks.
tests/shims.test.ts
Outdated
| const completedUrls: string[] = []; | ||
| const errorUrls: string[] = []; | ||
| Router.events.on("routeChangeComplete", (...args: unknown[]) => { | ||
| completedUrls.push(String(args[0])); | ||
| }); | ||
| Router.events.on("routeChangeError", (...args: unknown[]) => { | ||
| errorUrls.push(String(args[1])); | ||
| }); |
There was a problem hiding this comment.
These Router.events listeners are never detached. With a cached router module, they will accumulate across tests and continue firing later, which can lead to leaks/flakiness. Prefer removing them in finally via Router.events.off(...) (or vi.resetModules() per test).
tests/shims.test.ts
Outdated
| const completedUrls: string[] = []; | ||
| Router.events.on("routeChangeComplete", (...args: unknown[]) => { | ||
| completedUrls.push(String(args[0])); | ||
| }); | ||
| const errors: Array<{ err: unknown; url: string }> = []; | ||
| Router.events.on("routeChangeError", (...args: unknown[]) => { | ||
| errors.push({ err: args[0], url: String(args[1]) }); | ||
| }); |
There was a problem hiding this comment.
These Router.events listeners are never removed. Since routerEvents is a singleton in the cached router module, listeners will stack up across tests; please detach them in finally with Router.events.off(...) (or reset modules between tests).
tests/shims.test.ts
Outdated
|
|
||
| const errors: Array<{ err: unknown; url: string }> = []; | ||
| Router.events.on("routeChangeError", (...args: unknown[]) => { | ||
| errors.push({ err: args[0], url: String(args[1]) }); | ||
| }); |
There was a problem hiding this comment.
This Router.events.on("routeChangeError", ...) handler is not removed, so it will persist into later tests when the router module is cached. Suggest calling Router.events.off(...) in finally (or using vi.resetModules()) to prevent listener accumulation.
tests/shims.test.ts
Outdated
| const completedUrls: string[] = []; | ||
| Router.events.on("routeChangeComplete", (...args: unknown[]) => { | ||
| completedUrls.push(String(args[0])); | ||
| }); |
There was a problem hiding this comment.
This Router.events.on("routeChangeComplete", ...) handler is never detached. Because Router.events is a singleton, the handler will persist and fire in later tests. Consider removing it in finally (Router.events.off) or resetting the router module between tests.
Store each Router.events handler in a named variable before the try block and call Router.events.off() in the finally block. Prevents handlers from accumulating on the module-level routerEvents singleton across the 6 concurrent navigation tests.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Pages Router concurrent navigation fix
The core approach is sound — replacing a boolean _navInProgress guard with a monotonic navigation ID + AbortController correctly implements "last navigation wins" semantics. The assertStillCurrent() checkpoints after every await boundary mirror the Next.js clc / getCancelledHandler pattern. The tests are well-structured with proper cleanup.
I have one real bug to flag (early __NEXT_DATA__ write visible to stale navigations), a regression in error recovery for popstate, and a maintainability suggestion for the duplicated try/catch blocks.
Summary of findings
-
Bug: stale navigation can write
__NEXT_DATA__before cancellation is detected (router.ts:444) —window.__NEXT_DATA__is assigned between two async boundaries without a prior staleness check. If a stale navigation'sres.text()resolves before the winning navigation, it writes stale__NEXT_DATA__that persists until the winning navigation overwrites it. Any code reading__NEXT_DATA__in that window (e.g.,routeChangeErrorlisteners) sees inconsistent state. Fix: defer the write to just beforeroot.render(). -
Regression: popstate handler lost hard-navigation fallback — The old
navigateClienthad a catch-all that didwindow.location.href = urlfor unexpected errors. Now those errors propagate to callers. The 4push/replacesites catch them, but the popstate handler only emitsrouteChangeErrorand stops — leaving URL and content out of sync on unexpected failures (JSON parse, import errors, etc.). -
Maintenance: identical 12-line try/catch duplicated 4 times —
useRouter().push,useRouter().replace,Router.push,Router.replaceall have the same error-handling block. Should be extracted into a helper. -
Test quality is good — The 7 new tests cover the key scenarios well, including the signal-aware abort test and the stale-response-first test that were requested in the previous review. The
finallyblocks properly clean upwindowandfetch.
| * Matches Next.js's convention of an Error with `.cancelled = true`. | ||
| */ | ||
| class NavigationCancelledError extends Error { | ||
| cancelled = true; |
There was a problem hiding this comment.
Nit: consider exporting NavigationCancelledError (or at least the type) so consumers can distinguish cancellations from real errors in routeChangeError listeners without relying on duck-typing .cancelled. Next.js doesn't export a named class for this, so duck-typing is the documented pattern — but exporting the class would be a nice DX improvement.
Not blocking.
There was a problem hiding this comment.
Agreed this would be nice DX. Keeping duck-typing for now to match Next.js's convention — consumers already check .cancelled. Can revisit as a follow-up if there's demand.
| } | ||
|
|
||
| const html = await res.text(); | ||
| assertStillCurrent(); |
There was a problem hiding this comment.
Bug (low-severity): window.__NEXT_DATA__ is written on line 444 (below), between this assertStillCurrent() and the next one on line 472. Everything between lines 433–471 is synchronous so there's no microtask interleaving within this function, but the issue is:
- Stale navigation A's fetch resolves → passes
assertStillCurrent()here - Synchronously writes
window.__NEXT_DATA__ = nextDataA(line 444) - Hits
await import(pageModuleUrl)on line 471 — yields to microtask queue - Winning navigation B's microtasks run, eventually overwriting
__NEXT_DATA__
Between steps 2 and 4, __NEXT_DATA__ holds stale data. Any code reading it during that window (analytics, routeChangeError listeners from the stale nav's cancellation on line 472) sees inconsistent state.
Fix: Defer window.__NEXT_DATA__ = nextData until just before root.render(element) (around line 516), after all assertStillCurrent() checks have passed. Keep const nextData = JSON.parse(...) and const { pageProps } = nextData.props where they are — just delay the global write.
There was a problem hiding this comment.
Fixed. Deferred the window.__NEXT_DATA__ write to just before root.render(), after all assertStillCurrent() checkpoints have passed. Added a regression test that asserts __NEXT_DATA__ does not reflect the cancelled route's data at the moment routeChangeError fires.
| // Both real errors and cancellations emit routeChangeError. The popstate | ||
| // handler has no return value so there is no true/false distinction here. | ||
| routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false }); | ||
| }, |
There was a problem hiding this comment.
Regression: The popstate rejection handler emits routeChangeError but doesn't fall back to hard navigation for non-cancellation errors.
The old code had window.location.href = url inside navigateClient()'s catch block, which recovered from unexpected failures (JSON parse errors, import failures, etc.) by doing a full page load. With this PR, those errors propagate out as throws. The 4 push/replace call sites catch them and return false, but the popstate handler only emits the event and stops — leaving the browser showing the popstate target URL with the old rendered content.
Consider adding a hard-navigation fallback for non-cancel errors here:
(err: unknown) => {
routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false });
if (!(err instanceof NavigationCancelledError)) {
window.location.href = browserUrl;
}
},The same gap exists in the 4 push/replace catch blocks — they return false but don't trigger a hard navigation, so the user is stuck on the old content with a new URL. In the old code, navigateClient handled this internally. Might be worth restoring that fallback inside navigateClient's own error path (for non-cancel errors) rather than relying on every caller to do it.
There was a problem hiding this comment.
Fixed. The popstate rejection handler now does window.location.href = browserUrl for non-cancel errors. Same fallback is in the new runNavigateClient() helper used by push/replace.
packages/vinext/src/shims/router.ts
Outdated
| // Cancelled means a newer navigation superseded this one — not a failure | ||
| // from the caller's perspective, so return true (matches Next.js behaviour). | ||
| if (err instanceof NavigationCancelledError) { | ||
| return true; |
There was a problem hiding this comment.
This identical 12-line try/catch block is duplicated 4 times: useRouter().push (here), useRouter().replace (line 701), Router.push (line 900), and Router.replace (line 961).
Consider extracting a helper:
async function runNavigateClient(
full: string,
resolved: string,
): Promise<{ ok: true } | { ok: false; cancelled: boolean }> {
try {
await navigateClient(full);
return { ok: true };
} catch (err: unknown) {
routerEvents.emit("routeChangeError", err, resolved, { shallow: false });
if (!(err instanceof NavigationCancelledError)) {
window.location.href = full; // hard-navigation fallback
}
return { ok: false, cancelled: err instanceof NavigationCancelledError };
}
}This also gives a natural place to add the hard-navigation fallback for non-cancel errors (addressing point 2 above).
Not blocking, but strongly recommended before this file grows further.
There was a problem hiding this comment.
Done. Extracted runNavigateClient() returning "completed" | "cancelled" | "failed" so callers can distinguish all three outcomes. The 4 push/replace sites are now 3 lines each instead of 12.
tests/shims.test.ts
Outdated
| // complete) but NOT page-b (silently dropped by _navInProgress guard). | ||
| // After the fix, only page-b should render. | ||
| // The last render should NOT have been for page-a's data | ||
| expect(win.__NEXT_DATA__.page).toBe("/page-b"); |
There was a problem hiding this comment.
Good test. One subtlety: this assertion passes even with the early-write issue on router.ts:444, because navigation B completes last and overwrites A's __NEXT_DATA__. To catch the early-write bug, you'd need to observe __NEXT_DATA__ at the moment navigation A is cancelled.
Consider adding an assertion inside an error handler:
const onError = (...args: unknown[]) => {
// At the moment of cancellation, __NEXT_DATA__ should NOT reflect the cancelled route
expect(win.__NEXT_DATA__.page).not.toBe("/page-a");
};
Router.events.on("routeChangeError", onError);This would fail today (demonstrating the early-write issue) and would pass after deferring the __NEXT_DATA__ assignment. Not blocking for this PR, but would be valuable to add alongside the fix.
There was a problem hiding this comment.
Added this test — it asserts inside a routeChangeError handler that __NEXT_DATA__.page is not the cancelled route's value. Passes now that the write is deferred.
packages/vinext/src/shims/router.ts
Outdated
| // listeners still run — and observe the error — before the page unloads. | ||
| // Contract: routeChangeError listeners MUST be synchronous; async listeners | ||
| // will not fire before the navigation completes. | ||
| window.location.href = url; |
There was a problem hiding this comment.
Good comment explaining the window.location.href + throw interaction. One small addition worth noting: the callers catch this error and return false, but they don't prevent the hard navigation already scheduled by window.location.href = url. So the caller gets false AND a hard navigation happens. This is correct (caller learns it failed, browser recovers), but callers should not attempt their own hard-navigation fallback since one is already in flight.
Worth adding a note like: // Callers must NOT schedule a second hard navigation — this assignment already queues one.
There was a problem hiding this comment.
Added a note: Callers (runNavigateClient) must NOT schedule a second hard navigation — this assignment already queues one.
|
The review was submitted successfully. Here's a summary of what I posted: Review submitted on PR #7826 inline comments across
|
…per, add hard-nav fallback - Defer window.__NEXT_DATA__ assignment until just before root.render() so a stale navigation can never briefly pollute the global between assertStillCurrent() checkpoints. - Extract runNavigateClient() helper to deduplicate the identical 12-line try/catch block across useRouter().push/replace and Router.push/replace. - Add hard-navigation fallback (window.location.href) for non-cancel errors in both the helper and the popstate handler, fixing a regression where unexpected errors (JSON parse, import failure) left URL and content out of sync. - Add test asserting __NEXT_DATA__ is not stale at the moment routeChangeError fires for a cancelled navigation.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Pages Router concurrent navigation fix
The approach is correct and well-executed. Replacing the boolean _navInProgress with a monotonic navigation ID + AbortController is the right fix for this class of race condition, and the runNavigateClient() extraction addresses the review feedback from previous rounds cleanly.
The test suite is solid — 8 tests covering the key scenarios with proper cleanup. The deferred-promise pattern for controlling fetch timing is the right way to test concurrent async behavior.
I have one real bug to flag (double hard-navigation for known failure modes) and two minor observations.
Issue: double window.location.href assignment for known failure paths
navigateClient() sets window.location.href = url for several known failure modes (non-OK response, missing __NEXT_DATA__, no page module URL, invalid module path, no default export) and then throws. The caller runNavigateClient() catches non-cancel errors and also sets window.location.href = fullUrl. This means two hard navigations are scheduled for the same URL.
In practice this is harmless (the second assignment is a no-op since the first already queued the navigation), but the comment on line 427 explicitly says "Callers must NOT schedule a second hard navigation — this assignment already queues one." The code contradicts its own documented contract.
The fix is straightforward: runNavigateClient() should skip the window.location.href assignment when navigateClient() already did it. Options:
- Tag the thrown errors with a
hardNavScheduled = trueproperty sorunNavigateClientcan check - Move all
window.location.hrefassignments out ofnavigateClient()and intorunNavigateClient()exclusively (cleaner separation) - Accept the duplication and update the comment to say "may be assigned twice — second is a no-op"
Not blocking (functionally harmless), but it should be cleaned up since the comment was specifically added to warn against this exact thing.
Minor: missing blank line between tests
Lines 8903-8904 — the last two tests in the describe block are not separated by a blank line, unlike all the other tests. Trivial formatting nit.
| // response, missing __NEXT_DATA__), but unexpected errors (dynamic import | ||
| // failure, etc.) would otherwise leave URL and content out of sync. | ||
| if (typeof window !== "undefined") { | ||
| window.location.href = fullUrl; |
There was a problem hiding this comment.
Bug (low severity): This line sets window.location.href = fullUrl for every non-cancel error. But navigateClient() already sets window.location.href = url for all its known failure modes (non-OK response at line 428, missing __NEXT_DATA__ at line 439, no module URL at line 461, invalid path at line 469, no default export at line 480) before throwing.
This means known failures trigger two window.location.href assignments. The second is a no-op in practice, but it contradicts the comment on line 427 which explicitly says callers must NOT schedule a second hard navigation.
The window.location.href fallback here is still valuable for unexpected errors (e.g., JSON.parse throws, import() rejects with a non-abort error) where navigateClient didn't schedule a hard nav. But as written, it fires unconditionally.
Consider guarding it:
| window.location.href = fullUrl; | |
| // Genuine error (network, parse, import failure): fall back to a hard | |
| // navigation so the browser lands on the correct page. navigateClient | |
| // already assigns window.location.href for known failure modes (non-OK | |
| // response, missing __NEXT_DATA__), but unexpected errors (dynamic import | |
| // failure, etc.) would otherwise leave URL and content out of sync. | |
| // The duplicate assignment for known failures is a harmless no-op. | |
| if (typeof window !== "undefined") { | |
| window.location.href = fullUrl; | |
| } |
Or, longer term, consolidate all hard-nav assignments here and remove them from navigateClient() — let it be a pure "try or throw" function.
| const navId = ++_navigationId; | ||
|
|
||
| /** Check if this navigation is still the active one. If not, throw. */ | ||
| function assertStillCurrent(): void { |
There was a problem hiding this comment.
Good: the closure captures navId by value, so each navigation has a stable snapshot to compare against. The inner function is also nicely minimal — no allocation, just an integer comparison and throw.
One thought: if you ever need to expose cancellation status to external code (e.g., for testing or debugging), consider returning a boolean from assertStillCurrent() instead of throwing, and let the caller decide whether to throw. But for now the throw pattern is the right call — it's simpler and matches the Next.js clc pattern.
| console.error("[vinext] Client navigation failed:", err); | ||
| routerEvents.emit("routeChangeError", err, url, { shallow: false }); | ||
| window.location.href = url; | ||
| } finally { |
There was a problem hiding this comment.
Subtle but important: there is no assertStillCurrent() between the last checkpoint (line 505, after _app import) and this window.__NEXT_DATA__ write + root.render(). The gap covers:
React.createElement(AppComponent, ...)orReact.createElement(PageComponent, ...)wrapWithRouterContext(element)window.__NEXT_DATA__ = nextDataroot.render(element)
All of these are synchronous, so there's no microtask interleaving between them — a newer navigation can't start between lines 507 and 523. This is fine today, but it's a fragile invariant. If wrapWithRouterContext or element construction ever becomes async (e.g., lazy component resolution), the gap would become a real bug.
Worth a brief comment noting the synchronous invariant:
| } finally { | |
| // Commit __NEXT_DATA__ only after all assertStillCurrent() checks have passed, | |
| // so a stale navigation can never pollute the global. | |
| // NOTE: Everything from the last assertStillCurrent() (line ~505) through | |
| // root.render() is synchronous — no microtask yield, so no interleaving risk. | |
| window.__NEXT_DATA__ = nextData; |
| return { promise, resolve, reject }; | ||
| } | ||
|
|
||
| it("last push() wins when two overlap — superseded navigation does not render", async () => { |
There was a problem hiding this comment.
The first test doesn't clean up event listeners because it doesn't attach any — good. But it also doesn't reset the module-level _navigationId and _activeAbortController between tests. Since the router module is cached via import(), these module-level variables persist across all tests in the describe block.
This isn't a bug today because the monotonic counter works correctly regardless of its starting value, and the abort controller is cleaned up in navigateClient's finally block. But if a test ever leaves a navigation in-flight (e.g., by not awaiting Promise.allSettled), the stale controller could interfere with the next test.
The current tests all properly await Promise.allSettled, so this is fine — just noting the coupling for future test authors.
| globalThis.fetch = originalFetch; | ||
| } | ||
| }); | ||
| it("__NEXT_DATA__ is not stale when routeChangeError fires for a cancelled navigation", async () => { |
There was a problem hiding this comment.
Nit: missing blank line separating this test from the previous one (all other tests in the block have one). Trivial.
|
Review submitted successfully. Here's a summary of what I posted: 5 inline comments across Key findings:
|
Summary
_navInProgressguard innavigateClient()with a navigation ID counter + AbortController, fixing a race condition where rapidpush()calls left the URL bar showing one page while the content showed anotherrouteChangeErrorwith.cancelled = trueinstead of silently completing, matching Next.js behavior__NEXT_DATA__) now emitrouteChangeErrorinstead of incorrectly emittingrouteChangeCompletebefore the hard redirectHow it works
Each
navigateClient()call increments a monotonic_navigationIdand aborts the previous fetch viaAbortController. After everyawaitboundary (fetch, res.text, dynamic import, React import, app import), the function checks whether it is still the active navigation. If a newer one has started, it throwsNavigationCancelledError. Callers catch this and emitrouteChangeErrorinstead ofrouteChangeComplete.This mirrors the
clc(component load cancel) pattern in Next.js's Pages Router (getCancelledHandlerinpackages/next/src/shared/lib/router/router.ts).Test plan
push()calls: last navigation wins,__NEXT_DATA__reflects the winning pagerouteChangeCompletedoes not fire for the superseded navigationrouteChangeErrorfires with.cancelled = truefor the superseded navigationrouteChangeError, notrouteChangeCompletereplace()also cancels a supersededpush()