diff --git a/packages/vinext/src/server/app-browser-entry.ts b/packages/vinext/src/server/app-browser-entry.ts index 56164d7ea..424a9f46f 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,6 +36,7 @@ import { replaceHistoryStateWithoutNotify, restoreRscResponse, setClientParams, + setPendingPathname, snapshotRscResponse, setMountedSlotsHeader, setNavigationContext, @@ -190,8 +193,10 @@ function drainPrePaintEffects(upToRenderId: number): void { // Winning navigation: run its actual pre-paint effect effect(); } else { - // Superseded navigation: balance its activateNavigationSnapshot() - commitClientNavigationState(); + // Superseded navigation: balance its activateNavigationSnapshot(). + // Pass undefined navId intentionally so this cleanup cannot clear + // pendingPathname owned by the current active navigation. + commitClientNavigationState(undefined); } } } @@ -200,9 +205,16 @@ function drainPrePaintEffects(upToRenderId: number): void { function createNavigationCommitEffect( href: string, historyUpdateMode: HistoryUpdateMode | undefined, + navId: number, params: Record, ): () => 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; stageClientParams(params); @@ -212,7 +224,7 @@ function createNavigationCommitEffect( pushHistoryStateWithoutNotify(null, "", href); } - commitClientNavigationState(); + commitClientNavigationState(navId); }; } @@ -551,7 +563,7 @@ async function renderNavigationPayload( const resolve = pendingNavigationCommits.get(renderId); pendingNavigationCommits.delete(renderId); if (snapshotActivated) { - commitClientNavigationState(); + commitClientNavigationState(navId); } resolve?.(); throw error; @@ -747,17 +759,24 @@ 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 elementsAtNavStart = getBrowserRouterState().elements; const mountedSlotsHeader = getMountedSlotIdsHeader(elementsAtNavStart); const cachedRoute = getVisitedResponse(rscUrl, mountedSlotsHeader, navigationKind); @@ -789,7 +808,7 @@ async function main(): Promise { cachedNavigationSnapshot, href, navId, - createNavigationCommitEffect(href, historyUpdateMode, cachedParams), + createNavigationCommitEffect(href, historyUpdateMode, navId, cachedParams), isSameRoute, toActionType(navigationKind), ); @@ -878,7 +897,7 @@ async function main(): Promise { navigationSnapshot, href, navId, - createNavigationCommitEffect(href, historyUpdateMode, navParams), + createNavigationCommitEffect(href, historyUpdateMode, navId, navParams), isSameRoute, toActionType(navigationKind), ); @@ -905,7 +924,13 @@ async function main(): Promise { // before re-throwing, so this guard correctly skips the double-decrement case. if (_snapshotPending) { _snapshotPending = false; - commitClientNavigationState(); + commitClientNavigationState(navId); + } + // 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. diff --git a/packages/vinext/src/shims/navigation.ts b/packages/vinext/src/shims/navigation.ts index 9e1adf04b..30c323574 100644 --- a/packages/vinext/src/shims/navigation.ts +++ b/packages/vinext/src/shims/navigation.ts @@ -500,6 +500,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; @@ -525,7 +527,7 @@ export function getMountedSlotsHeader(): string | null { return globalState[_MOUNTED_SLOTS_HEADER_KEY] ?? null; } -function getClientNavigationState(): ClientNavigationState | null { +export function getClientNavigationState(): ClientNavigationState | null { if (isServer) return null; const globalState = window as ClientNavigationGlobal; @@ -538,6 +540,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 @@ -768,6 +772,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; } @@ -927,7 +959,14 @@ function withSuppressedUrlNotifications(fn: () => T): T { } } -export function commitClientNavigationState(): void { +/** + * Commit pending client navigation state to committed snapshots. + * + * navId is optional: callers that don't own pendingPathname (for example, + * superseded pre-paint cleanup) may pass undefined to flush snapshot/params + * state without clearing pendingPathname owned by the active navigation. + */ +export function commitClientNavigationState(navId?: number): void { if (isServer) return; const state = getClientNavigationState(); if (!state) return; @@ -947,6 +986,18 @@ 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) + // - navId is undefined only for non-owning callers, which must not clear + // pendingPathname for an active navigation. + const canClearPendingPathname = + state.pendingPathnameNavId === null || + (navId !== undefined && state.pendingPathnameNavId === navId); + if (canClearPendingPathname) { + state.pendingPathname = null; + state.pendingPathnameNavId = null; + } const shouldNotify = urlChanged || state.hasPendingNavigationUpdate; state.hasPendingNavigationUpdate = false; diff --git a/tests/e2e/app-router/navigation-flows.spec.ts b/tests/e2e/app-router/navigation-flows.spec.ts index f5f847654..dc793db3e 100644 --- a/tests/e2e/app-router/navigation-flows.spec.ts +++ b/tests/e2e/app-router/navigation-flows.spec.ts @@ -225,4 +225,48 @@ test.describe("App Router navigation flows", () => { await page.goto(`${BASE}/client-nav-test?q=test-param`); await expect(page.locator('[data-testid="client-search-q"]')).toHaveText("test-param"); }); + + test("back/forward through rapid-nav pages maintains 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 waitForAppRouterHydration(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"); + + // 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`); + + // 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); + }); }); 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 000000000..8ea410b1e --- /dev/null +++ b/tests/e2e/app-router/rapid-navigation.spec.ts @@ -0,0 +1,94 @@ +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 }) => { + 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. + // + // Why isSameRoute === true for the second navigation: + // Both clicks run synchronously in the same microtask. The first click calls + // setPendingPathname('/nav-rapid/page-b', navId=1), storing the pending target. + // The second click then reads pendingPathname (which is '/nav-rapid/page-b'), + // compares it to its own target pathname (also '/nav-rapid/page-b'), and sees + // they match — so isSameRoute is true. This is correct behavior: the second + // navigation is a same-route query-param change, not a cross-route navigation. + 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); + }); +}); 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 000000000..cf0a6c45b --- /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 000000000..4e33d3fa8 --- /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 000000000..125b73635 --- /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 000000000..e755802ec --- /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

+ +
+ ); +}