diff --git a/packages/vinext/src/server/app-browser-entry.ts b/packages/vinext/src/server/app-browser-entry.ts index dd74e35e..06d60b37 100644 --- a/packages/vinext/src/server/app-browser-entry.ts +++ b/packages/vinext/src/server/app-browser-entry.ts @@ -23,10 +23,12 @@ import { notifyAppRouterTransitionStart } from "../client/instrumentation-client import { __basePath, activateNavigationSnapshot, + clearPendingPathname, commitClientNavigationState, consumePrefetchResponse, createClientNavigationRenderSnapshot, getClientNavigationRenderContext, + getClientNavigationState, getPrefetchCache, getPrefetchedUrls, pushHistoryStateWithoutNotify, @@ -34,8 +36,9 @@ import { replaceHistoryStateWithoutNotify, restoreRscResponse, setClientParams, - snapshotRscResponse, + setPendingPathname, setNavigationContext, + snapshotRscResponse, toRscUrl, type CachedRscResponse, type ClientNavigationRenderSnapshot, @@ -171,8 +174,15 @@ function drainPrePaintEffects(upToRenderId: number): void { function createNavigationCommitEffect( href: string, historyUpdateMode: HistoryUpdateMode | undefined, + navId: number, ): () => void { return () => { + // Only update URL if this is still the active navigation. + // A newer navigation would have incremented activeNavigationId. + if (navId !== activeNavigationId) { + return; + } + const targetHref = new URL(href, window.location.origin).href; if (historyUpdateMode === "replace" && window.location.href !== targetHref) { @@ -181,7 +191,7 @@ function createNavigationCommitEffect( pushHistoryStateWithoutNotify(null, "", href); } - commitClientNavigationState(); + commitClientNavigationState(navId); }; } @@ -334,6 +344,7 @@ function updateBrowserTree( renderId: number, useTransitionMode: boolean, snapshotActivated = false, + navId?: number, ): void { const setter = getBrowserTreeStateSetter(); @@ -351,7 +362,11 @@ function updateBrowserTree( const resolve = pendingNavigationCommits.get(renderId); pendingNavigationCommits.delete(renderId); if (snapshotActivated) { - commitClientNavigationState(); + if (navId !== undefined) { + commitClientNavigationState(navId); + } else { + commitClientNavigationState(); + } } resolve?.(); }; @@ -383,6 +398,7 @@ function renderNavigationPayload( navigationSnapshot: ClientNavigationRenderSnapshot, prePaintEffect: (() => void) | null = null, useTransition = true, + navId?: number, ): Promise { const renderId = ++nextNavigationRenderId; queuePrePaintNavigationEffect(renderId, prePaintEffect); @@ -396,13 +412,17 @@ function renderNavigationPayload( // Wrap updateBrowserTree in try-catch to ensure counter is decremented // if a synchronous error occurs before the async promise chain is established. try { - updateBrowserTree(payload, navigationSnapshot, renderId, useTransition, true); + updateBrowserTree(payload, navigationSnapshot, renderId, useTransition, true, navId); } catch (error) { // Clean up pending state and decrement counter on synchronous error. pendingNavigationPrePaintEffects.delete(renderId); const resolve = pendingNavigationCommits.get(renderId); pendingNavigationCommits.delete(renderId); - commitClientNavigationState(); + if (navId !== undefined) { + commitClientNavigationState(navId); + } else { + commitClientNavigationState(); + } resolve?.(); throw error; // Re-throw to maintain error propagation } @@ -613,19 +633,26 @@ async function main(): Promise { try { const url = new URL(href, window.location.origin); const rscUrl = toRscUrl(url.pathname + url.search); - // Use startTransition for same-route navigations (searchParam changes) - // so React keeps the old UI visible during the transition. For cross-route - // navigations (different pathname), use synchronous updates — React's - // startTransition hangs in Firefox when replacing the entire tree. - // 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. - const isSameRoute = - stripBasePath(url.pathname, __basePath) === + + // Get navigation state for comparison + const navState = getClientNavigationState(); + + // Compare against previous pending navigation, then committed, then window.location + // This ensures correct isSameRoute classification during rapid successive navigations + const currentPath = + navState?.pendingPathname ?? + navState?.cachedPathname ?? stripBasePath(window.location.pathname, __basePath); + + const targetPath = stripBasePath(url.pathname, __basePath); + const isSameRoute = targetPath === currentPath; + + // Set this navigation as the pending pathname, overwriting any previous. + // Pass navId so only this navigation (or a newer one) can clear it later. + setPendingPathname(url.pathname, navId); + const cachedRoute = getVisitedResponse(rscUrl, navigationKind); - const navigationCommitEffect = createNavigationCommitEffect(href, historyUpdateMode); + const navigationCommitEffect = createNavigationCommitEffect(href, historyUpdateMode, navId); if (cachedRoute) { // Check stale-navigation before and after createFromFetch. The pre-check @@ -660,6 +687,7 @@ async function main(): Promise { cachedNavigationSnapshot, navigationCommitEffect, isSameRoute, + navId, ); } finally { // Always clear _snapshotPending so the outer catch does not @@ -744,6 +772,7 @@ async function main(): Promise { navigationSnapshot, navigationCommitEffect, isSameRoute, + navId, ); } finally { // Always clear _snapshotPending after renderNavigationPayload returns or @@ -767,6 +796,12 @@ async function main(): Promise { _snapshotPending = false; commitClientNavigationState(); } + // 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(navId); + } // Don't hard-navigate to a stale URL if this navigation was superseded by // a newer one — the newer navigation is already in flight and would be clobbered. if (navId !== activeNavigationId) return; diff --git a/packages/vinext/src/shims/navigation.ts b/packages/vinext/src/shims/navigation.ts index bb1c1ab1..121b7c12 100644 --- a/packages/vinext/src/shims/navigation.ts +++ b/packages/vinext/src/shims/navigation.ts @@ -474,6 +474,8 @@ type ClientNavigationState = { clientParamsJson: string; pendingClientParams: Record | null; pendingClientParamsJson: string | null; + pendingPathname: string | null; + pendingPathnameNavId: number | null; originalPushState: typeof window.history.pushState; originalReplaceState: typeof window.history.replaceState; patchInstalled: boolean; @@ -486,7 +488,7 @@ type ClientNavigationGlobal = typeof globalThis & { [_CLIENT_NAV_STATE_KEY]?: ClientNavigationState; }; -function getClientNavigationState(): ClientNavigationState | null { +export function getClientNavigationState(): ClientNavigationState | null { if (isServer) return null; const globalState = window as ClientNavigationGlobal; @@ -499,6 +501,8 @@ function getClientNavigationState(): ClientNavigationState | null { clientParamsJson: "{}", pendingClientParams: null, pendingClientParamsJson: null, + pendingPathname: null, + pendingPathnameNavId: null, // NB: These capture the currently installed history methods, not guaranteed // native ones. If a third-party library (analytics, router) has already patched // history methods before this module loads, we intentionally preserve that @@ -729,6 +733,34 @@ export function getClientParams(): Record { return getClientNavigationState()?.clientParams ?? _fallbackClientParams; } +/** + * Set the pending pathname for client-side navigation. + * Strips the base path before storing. Associates the pathname with the given navId + * so only that navigation (or a newer one) can clear it. + */ +export function setPendingPathname(pathname: string, navId: number): void { + const state = getClientNavigationState(); + if (!state) return; + state.pendingPathname = stripBasePath(pathname, __basePath); + state.pendingPathnameNavId = navId; +} + +/** + * Clear the pending pathname, but only if the given navId matches the one + * that set it, or if pendingPathnameNavId is null (no active owner). + * This prevents superseded navigations from clearing state belonging to newer navigations. + */ +export function clearPendingPathname(navId: number): void { + const state = getClientNavigationState(); + if (!state) return; + // Only clear if this navId is the one that set the pendingPathname, + // or if pendingPathnameNavId is null (no owner) + if (state.pendingPathnameNavId === null || state.pendingPathnameNavId === navId) { + state.pendingPathname = null; + state.pendingPathnameNavId = null; + } +} + function getClientParamsSnapshot(): Record { return getClientNavigationState()?.clientParams ?? _EMPTY_PARAMS; } @@ -888,7 +920,7 @@ function withSuppressedUrlNotifications(fn: () => T): T { } } -export function commitClientNavigationState(): void { +export function commitClientNavigationState(navId?: number): void { if (isServer) return; const state = getClientNavigationState(); if (!state) return; @@ -908,6 +940,13 @@ export function commitClientNavigationState(): void { state.pendingClientParams = null; state.pendingClientParamsJson = null; } + // Clear pending pathname when navigation commits, but only if: + // - The navId matches the one that set pendingPathname + // - No newer navigation has overwritten pendingPathname (pendingPathnameNavId === null or matches) + if (state.pendingPathnameNavId === null || state.pendingPathnameNavId === navId) { + state.pendingPathname = null; + state.pendingPathnameNavId = null; + } const shouldNotify = urlChanged || state.hasPendingNavigationUpdate; state.hasPendingNavigationUpdate = false; diff --git a/tests/e2e/app-router/rapid-navigation.spec.ts b/tests/e2e/app-router/rapid-navigation.spec.ts new file mode 100644 index 00000000..963d6383 --- /dev/null +++ b/tests/e2e/app-router/rapid-navigation.spec.ts @@ -0,0 +1,177 @@ +import { test, expect } from "@playwright/test"; + +const BASE = "http://localhost:4174"; + +/** + * Wait for the RSC browser entry to hydrate. The browser entry sets + * window.__VINEXT_RSC_ROOT__ after hydration completes. + */ +async function waitForHydration(page: import("@playwright/test").Page) { + await expect(async () => { + const ready = await page.evaluate(() => !!(window as any).__VINEXT_RSC_ROOT__); + expect(ready).toBe(true); + }).toPass({ timeout: 10_000 }); +} + +test.describe("rapid navigation", () => { + 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); + }); + + test("same-route query change during cross-route navigation", async ({ page }) => { + 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); + + // Navigate to B then immediately change query param (same-route nav) + // Use a single page.evaluate() to click both links atomically. + await page.evaluate(() => { + const linkB = document.querySelector('[data-testid="page-a-link-to-b"]') as HTMLElement; + const linkFilter = document.querySelector( + '[data-testid="page-a-link-to-b-filter"]', + ) as HTMLElement; + if (linkB) linkB.click(); + if (linkFilter) linkFilter.click(); + }); + + // Should settle on B with query param + await expect(page.locator("h1")).toHaveText("Page B"); + await expect(page).toHaveURL(`${BASE}/nav-rapid/page-b?filter=test`); + + // Verify no navigation-related errors + const navigationErrors = errors.filter( + (e) => e.includes("navigation") || e.includes("vinext") || e.includes("router"), + ); + expect(navigationErrors).toHaveLength(0); + }); + + test("rapid back-forward navigation maintains state consistency", async ({ page }) => { + 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); + + // Navigate A -> B + await page.click('[data-testid="page-a-link-to-b"]'); + await expect(page.locator("h1")).toHaveText("Page B"); + + // Navigate B -> C + await page.click('[data-testid="link-to-c"]'); + await expect(page.locator("h1")).toHaveText("Page C"); + + // Rapid back navigation + await page.goBack(); + await page.goBack(); + + // Should be back at A + await expect(page.locator("h1")).toHaveText("Page A"); + await expect(page).toHaveURL(`${BASE}/nav-rapid/page-a`); + + // Rapid forward navigation + await page.goForward(); + await page.goForward(); + + // Should be back at C + await expect(page.locator("h1")).toHaveText("Page C"); + await expect(page).toHaveURL(`${BASE}/nav-rapid/page-c`); + + // Verify no navigation-related errors + const navigationErrors = errors.filter( + (e) => e.includes("navigation") || e.includes("vinext") || e.includes("router"), + ); + expect(navigationErrors).toHaveLength(0); + }); + + test("interrupted navigation does not leave partial state", async ({ page }) => { + 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); + + // Set marker to detect full page reload + await page.evaluate(() => { + (window as any).__NAV_MARKER__ = "started-at-a"; + }); + + // Start navigation to B but immediately interrupt with navigation to C + // Use a single page.evaluate() to click both links atomically. + 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 — rapid client-side + // navigations abort the first nav, causing ERR_ABORTED with waitForURL. + await expect(page).toHaveURL(`${BASE}/nav-rapid/page-c`, { timeout: 10_000 }); + await expect(page.locator("h1")).toHaveText("Page C"); + + // Verify no full page reload happened (marker should survive) + const marker = await page.evaluate(() => (window as any).__NAV_MARKER__); + expect(marker).toBe("started-at-a"); + + // Verify we're on the correct final URL + await expect(page).toHaveURL(`${BASE}/nav-rapid/page-c`); + + // Verify no navigation-related errors + const navigationErrors = errors.filter( + (e) => e.includes("navigation") || e.includes("vinext") || e.includes("router"), + ); + expect(navigationErrors).toHaveLength(0); + }); +}); diff --git a/tests/fixtures/app-basic/app/nav-rapid/layout.tsx b/tests/fixtures/app-basic/app/nav-rapid/layout.tsx new file mode 100644 index 00000000..cf0a6c45 --- /dev/null +++ b/tests/fixtures/app-basic/app/nav-rapid/layout.tsx @@ -0,0 +1,3 @@ +export default function NavRapidLayout({ children }: { children: React.ReactNode }) { + return
{children}
; +} diff --git a/tests/fixtures/app-basic/app/nav-rapid/page-a/page.tsx b/tests/fixtures/app-basic/app/nav-rapid/page-a/page.tsx new file mode 100644 index 00000000..4e33d3fa --- /dev/null +++ b/tests/fixtures/app-basic/app/nav-rapid/page-a/page.tsx @@ -0,0 +1,26 @@ +import Link from "next/link"; + +export default function PageA() { + return ( +
+

Page A

+ +
+ ); +} diff --git a/tests/fixtures/app-basic/app/nav-rapid/page-b/page.tsx b/tests/fixtures/app-basic/app/nav-rapid/page-b/page.tsx new file mode 100644 index 00000000..125b7363 --- /dev/null +++ b/tests/fixtures/app-basic/app/nav-rapid/page-b/page.tsx @@ -0,0 +1,22 @@ +import Link from "next/link"; + +export default function PageB() { + return ( +
+

Page B

+ +
+ ); +} diff --git a/tests/fixtures/app-basic/app/nav-rapid/page-c/page.tsx b/tests/fixtures/app-basic/app/nav-rapid/page-c/page.tsx new file mode 100644 index 00000000..e755802e --- /dev/null +++ b/tests/fixtures/app-basic/app/nav-rapid/page-c/page.tsx @@ -0,0 +1,18 @@ +import Link from "next/link"; + +export default function PageC() { + return ( +
+

Page C

+ +
+ ); +} diff --git a/tests/shims.test.ts b/tests/shims.test.ts index 9b86462d..920a7326 100644 --- a/tests/shims.test.ts +++ b/tests/shims.test.ts @@ -338,6 +338,19 @@ describe("next/navigation shim", () => { expect(html).toContain("[]"); }); + + // Smoke tests for server-side safety + // These verify the functions don't throw on the server (getClientNavigationState returns null) + // Real pending pathname behavior is covered by E2E tests + it("setPendingPathname does not throw on server (getClientNavigationState returns null)", async () => { + const { setPendingPathname } = await import("../packages/vinext/src/shims/navigation.js"); + expect(() => setPendingPathname("/some-path", 1)).not.toThrow(); + }); + + it("clearPendingPathname does not throw on server (getClientNavigationState returns null)", async () => { + const { clearPendingPathname } = await import("../packages/vinext/src/shims/navigation.js"); + expect(() => clearPendingPathname(1)).not.toThrow(); + }); }); describe("next/headers shim", () => {