Files
cloudflare__vinext/tests/prefetch-cache.test.ts
T
Divanshu Chauhan (divkix) ef4192db49 fix: cross-route client navigation hangs in Firefox (#652) (#690)
* fix: cross-route client navigation hangs in Firefox (#652)

Replace flushSync-based RSC tree rendering with a two-phase navigation
commit that uses startTransition for same-route navigations and
synchronous updates for cross-route navigations. This fixes the Firefox
hang where startTransition never commits when the entire component tree
is replaced, and also resolves the Suspense double-flash from #639.

Key changes:
- navigation.ts: ClientNavigationState on Symbol.for global for module
  instance safety, render snapshot context for hook consistency during
  transitions, RSC response snapshot/restore for visited cache, unified
  navigateClientSide() entry point, history suppression helpers
- app-browser-entry.ts: BrowserRoot component with useState-managed tree,
  NavigationCommitSignal that defers URL commit to useLayoutEffect,
  visited response cache with LRU eviction, navigation ID counter for
  stale navigation bailout, manual scroll restoration
- link.tsx: remove duplicated helpers, delegate to navigateClientSide
- form.tsx: delegate App Router GET navigation to navigateClientSide

Closes #652

* fix: detect React Flight Thenables in updateBrowserTree and apply params eagerly

createFromFetch() returns a Thenable (not instanceof Promise), so the
async branch was never taken. The raw unresolved Thenable was set as
React children, causing suspension without a Suspense boundary and
empty content after cross-route navigations.

- Duck-type the .then() check to handle both Promises and Thenables
- Replace stageClientParams with applyClientParams so useSyncExternalStore
  subscribers see correct params immediately
- Remove dead stageClientParams function and replaceClientParamsWithoutNotify import

Fixes useParams E2E failures in CI (hooks.spec.ts:161, hooks.spec.ts:180).

* fix: await createFromFetch before passing RSC payload to tree

The Flight Thenable from createFromFetch was passed unresolved to
renderNavigationPayload/updateBrowserTree. The .then() callback fired
unreliably across consecutive navigations, causing empty content.

Await createFromFetch in both navigation paths (cached and fresh) so
renderNavigationPayload always receives a resolved ReactNode. Add stale
navigation checks after the new await points.

The HMR handler and server action handler already awaited correctly.

* fix: preserve app router params across cached navigation

Preserve the X-Vinext-Params header when snapshotting prefetched and visited RSC responses so cached navigations keep dynamic params intact. Stage client params until the URL commit lands so useParams, history state, and redirect handling stay aligned during App Router transitions.

* fix: address review feedback on navigation caching and history management

- Remove double history push for RSC navigations: let the commit effect
  in navigateRsc own push/replace exclusively. Also fixes a hidden bug
  where the early push broke isSameRoute detection (always true),
  negating the Firefox cross-route hang fix.
- Enforce prefetch cache size limits in prefetchRscResponse() by
  extracting shared eviction logic from storePrefetchResponse().
- Add TTL check in consumePrefetchResponse() so expired snapshots are
  discarded instead of served as stale UI.
- Prevent navigationSnapshotActiveCount leak when async payloads reject
  in updateBrowserTree by adding error handlers that balance the
  activate/commit pairing.
- Move BrowserRoot setBrowserTreeState assignment from render to
  useLayoutEffect to avoid side effects under React Strict Mode.
- Make visited response cache true LRU (delete+set on hit) and add a
  30-minute upper bound for traversal cache hits to prevent arbitrarily
  stale back/forward content.

* fix(navigation): address PR review feedback for Firefox navigation fix

Fixes the following review issues from ask-bonk:

Major fixes:
- Fix prefetch cache eviction order to add entry first, then evict (prevents
  temporary overflow past MAX_PREFETCH_CACHE_SIZE)
- Add error handling in renderNavigationPayload to ensure
  navigationSnapshotActiveCount is decremented on synchronous errors,
  preventing counter leaks

Minor fixes:
- Add cross-reference comments between Pages Router and App Router scroll
  restoration handlers (navigation.ts and app-browser-entry.ts)
- Add JSDoc to getPathnameSnapshot/getSearchParamsSnapshot explaining cached
  behavior vs live window.location reads
- Add comment documenting server action navigation flow (doesn't use
  activateNavigationSnapshot because it stays on same URL)
- Add comment explaining Map iteration safety in resolveCommittedNavigations
- Use stripBasePath in isSameRoute comparison for robust basePath handling
- Explicitly pass undefined for historyUpdateMode in redirect chains with
  explanatory comment (prevents double URL push)

All 1094 tests pass.

* fix: prevent stale navigation bailout from leaking staged params

Addresses review feedback on PR #690:
- Move stageClientParams() to after final navId check in both cached and fresh paths
- Build navigationSnapshot from local params instead of latestClientParams
- Move storeVisitedResponseSnapshot after final stale check
- Add doc comment about history method patching behavior

This fixes a race where cancelled navigations could leak their staged params
into subsequent unrelated navigations when commitClientNavigationState()
was called later without clearing the pending state.

Test: Add E2E test for cancelled slow navigation param leak scenario

* fix: remove duplicate onRouterTransitionStart call and fix formatting

The refactor to delegate Link navigation to navigateClientSide() left a
duplicate notifyAppRouterTransitionStart() call in link.tsx, causing the
callback to fire twice per push navigation.

* fix(navigation): address code review feedback on Firefox cross-route navigation hang fix

Fixes four edge cases identified in code review:

1. NavigationCommitSignal cleanup now resolves pending promises on unmount
   to prevent hanging callers if React unmounts the component (e.g., error
   boundary triggers full-page nav)

2. Added dev-mode warning for navigationSnapshotActiveCount going to 0 before
   decrement to catch activate/commit pairing bugs early

3. Changed prefetch eviction from if to while loop to match visited cache
   pattern and prevent temporary cache overflow during rapid prefetch calls

4. Moved storeVisitedResponseSnapshot below final navId check to avoid storing
   stale cache entries from superseded navigations

Also added cross-reference comments between the two popstate handlers in
app-browser-entry.ts and navigation.ts for future maintainers.

* fix(navigation): resolve infinite recursion in getServerSearchParamsSnapshot

The function getServerSearchParamsSnapshot had infinite recursion when
called on the server - it would call itself infinitely when
getClientNavigationState() returned null.

Fixed by implementing proper server context handling:
- Use _getServerContext() to access server navigation context
- Cache ReadonlyURLSearchParams wrapper using symbol-based caching
- Return cached empty instance when no context available
- Follows established patterns from getServerParamsSnapshot and
  getSearchParamsSnapshot

Fixes CI failures in PR #690 where 18 tests were failing with
"Maximum call stack size exceeded" errors during SSR.

* style(navigation): fix lint errors from stricter oxlint rules

Convert interface declarations to type aliases per consistent-type-definitions rule:
- CachedRscResponse, PrefetchCacheEntry, ClientNavigationRenderSnapshot
- VisitedResponseCacheEntry in app-browser-entry.ts

Add oxlint-disable block comments for react-hooks/rules-of-hooks violations:
- useChildSegments, useClientNavigationRenderSnapshot, usePathname
- useSearchParams, useParams

The original conditional hook pattern (if isServer return early) is intentional
and required for SSR/client duality. Server components must not call client-only
React hooks. Block-level disable comments suppress the false positives while
maintaining the correct runtime behavior.

* fix(navigation): address review feedback on Firefox cross-route navigation fix

- Fix isHashOnlyChange to strip basePath for consistent comparison with isSameRoute
- Add snapshotPending guard to prevent double-decrement in catch block
- Document storePrefetchResponse API contract (callers manage prefetchedUrls)
- Remove duplicate prefetched.add from _appRouter.prefetch (ownership now in prefetchRscResponse)
- Add invariant comment to drainPrePaintEffects explaining activate/commit pairing
- Improve test error handling in cancelled navigation test with debug logging

* fix: address PR review — prefetch dedup, comments, and test robustness

- Add dedup guard to router.prefetch() matching link.tsx pattern
  (check/add prefetchedUrls before calling prefetchRscResponse)
- Fix incorrect comments claiming prefetchRscResponse manages prefetchedUrls
- Add doc comment to storePrefetchResponse noting caller owns URL tracking
- Document module-level state assumption in app-browser-entry.ts
- Add inline comments in drainPrePaintEffects for winning/superseded pairing
- Add element visibility assertion in E2E cancelled-navigation test

* chore: review changes implemented

* Address ask-bonk review feedback: remove dead code and add defensive comments

- Remove vestigial response field from PrefetchCacheEntry (never read by consumePrefetchResponse)
- Add defensive comment for _snapshotPending guard brittleness
- Document isSameRoute race condition during rapid navigations
- Update test helper to use snapshot instead of response

All changes are non-behavioral documentation/clarity improvements.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: prevent double-decrement of navigationSnapshotActiveCount on renderNavigationPayload throw

When updateBrowserTree throws synchronously inside renderNavigationPayload,
the inner catch already calls commitClientNavigationState() and re-throws.
Previously the outer navigateRsc catch saw _snapshotPending === true and
decremented again, leaving the counter off by 1 for concurrent navigations.

Fix by wrapping each renderNavigationPayload call in try-finally that always
clears _snapshotPending, so the outer catch guard correctly skips the
already-committed case.

* docs: document latestClientParams staging invariant in stageClientParams

Per bonk review: stageClientParams diverges latestClientParams from
ClientNavigationState.clientParams between staging and commit. Server
action snapshots read latestClientParams, so add a comment clarifying
this narrow race is acceptable since the commit effect fires synchronously
in the same React commit phase.

* fix: address two blocking concerns from bonk review

1. Guard outer catch against hard-navigating to stale URL when superseded:
   - Hoist navId above the try block so it is accessible in the catch
   - Add 'if (navId !== activeNavigationId) return' before window.location.href
   - Move navId check before createFromFetch in cached route path to avoid
     wasted parse work and the same stale-URL risk on throw

2. Guard handleAsyncError in updateBrowserTree against counter corruption:
   - Add snapshotActivated parameter (default false) to updateBrowserTree
   - Only call commitClientNavigationState() when snapshotActivated is true
   - renderNavigationPayload passes true; server actions and HMR pass false
     (they never call activateNavigationSnapshot so must not decrement)
   - Update comments on server action and HMR call sites to document why

Also add a note on restoreRscResponse that reconstructed Response.url is
always empty string — the Response constructor does not accept a url option.

* fix: tighten cached-route stale-nav guards and fix stale comment

- Add navId check between cachedNavigationSnapshot creation and
  createFromFetch in the cached route path, so superseded navigations
  bail out before the async parse rather than only after
- Fix stale line reference in stageClientParams comment (533 -> 544/558)
- Document slow-route fixture delay / click timeout coupling in E2E test
  so future changes to the fixture delay don't silently break the test

* fix: stage params after _snapshotPending=true in both nav paths, fix stale comment

Move stageClientParams() to after _snapshotPending = true in both the
cached-route path and the non-cached path. This makes the intent clearer:
params are staged as part of an in-flight snapshot, not as a standalone
side-effect before the snapshot flag is set.

If renderNavigationPayload throws synchronously, its inner catch calls
commitClientNavigationState() which flushes pendingClientParams — committing
params for a route that never rendered. The hard-nav fallback in the outer
catch resets everything, so this was self-correcting, but reordering makes
the relationship explicit and guards the intent.

Also replace the stale "lines 544/558" reference in the stageClientParams
comment with the function name (registerServerActionCallback) so it doesn't
rot as the file evolves.

* fix: expand module-singleton comment, remove redundant stale-nav check, document BASE URL

- Expand the module-level state comment to call out Map fragmentation as
  the most severe consequence of multiple instances: pendingNavigationCommits
  and pendingNavigationPrePaintEffects would split across instances, causing
  drainPrePaintEffects to miss effects queued by the other instance and
  permanently leaking navigationSnapshotActiveCount.

- Remove the redundant navId !== activeNavigationId check between
  createClientNavigationRenderSnapshot (synchronous) and createFromFetch
  (async). The pre-check before createClientNavigationRenderSnapshot is
  sufficient; the meaningful post-check is after the await of createFromFetch.
  Added a comment noting the function is synchronous so no recheck is needed.

- Add a comment to BASE in navigation-regressions.spec.ts explaining it
  follows the established repo convention (same pattern in navigation.spec.ts,
  server-client-only.spec.ts, after.spec.ts, etc.).

* docs: annotate stageClientParams throw path in both nav paths

If stageClientParams throws (e.g. a param object with a throwing getter),
_snapshotPending is true but renderNavigationPayload was never called.
The outer catch then calls commitClientNavigationState() which decrements
a counter never incremented by activateNavigationSnapshot() — safe due to
the > 0 guard — but pendingClientParams and hasPendingNavigationUpdate
could persist until the hard-nav fallback resets JS state.

Add a brief NB comment on both stageClientParams call sites noting that
the outer catch hard-navigates, which resets all JS state, making this
benign in practice.

* fix: store visited response snapshot only after successful render

Move storeVisitedResponseSnapshot to after the renderNavigationPayload
try-finally block so we only cache RSC responses from navigations that
actually rendered successfully. Previously, the cache write happened
before renderNavigationPayload — if it threw, a future back/forward
navigation could replay a snapshot from a navigation that never rendered.

The hard-nav fallback in the outer catch made this self-correcting in
practice, but caching only on success is the correct invariant.

Also expand the cached-path navId comment to acknowledge the style
asymmetry vs the non-cached path, explaining why no extra check is
needed between createClientNavigationRenderSnapshot (synchronous) and
createFromFetch.

---------

Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: James <james@eli.cx>
2026-04-01 22:12:52 +01:00

184 lines
7.1 KiB
TypeScript

/**
* Prefetch cache eviction tests.
*
* Verifies that storePrefetchResponse() sweeps expired entries before
* falling back to FIFO eviction, preventing expired entries from wasting
* cache slots on link-heavy pages.
*
* The navigation module computes `isServer = typeof window === "undefined"`
* at load time, so we must set globalThis.window BEFORE importing it via
* vi.resetModules() + dynamic import().
*/
import { describe, it, expect, beforeEach, afterEach, vi } from "vite-plus/test";
type Navigation = typeof import("../packages/vinext/src/shims/navigation.js");
let storePrefetchResponse: Navigation["storePrefetchResponse"];
let getPrefetchCache: Navigation["getPrefetchCache"];
let getPrefetchedUrls: Navigation["getPrefetchedUrls"];
let MAX_PREFETCH_CACHE_SIZE: Navigation["MAX_PREFETCH_CACHE_SIZE"];
let PREFETCH_CACHE_TTL: Navigation["PREFETCH_CACHE_TTL"];
let snapshotRscResponse: Navigation["snapshotRscResponse"];
let restoreRscResponse: Navigation["restoreRscResponse"];
beforeEach(async () => {
// Set window BEFORE importing so isServer evaluates to false
(globalThis as any).window = {
__VINEXT_RSC_PREFETCH_CACHE__: new Map(),
__VINEXT_RSC_PREFETCHED_URLS__: new Set(),
location: { pathname: "/", search: "", hash: "", href: "http://localhost/" },
addEventListener: () => {},
history: { pushState: () => {}, replaceState: () => {}, state: null },
dispatchEvent: () => {},
};
vi.resetModules();
const nav = await import("../packages/vinext/src/shims/navigation.js");
storePrefetchResponse = nav.storePrefetchResponse;
getPrefetchCache = nav.getPrefetchCache;
getPrefetchedUrls = nav.getPrefetchedUrls;
MAX_PREFETCH_CACHE_SIZE = nav.MAX_PREFETCH_CACHE_SIZE;
PREFETCH_CACHE_TTL = nav.PREFETCH_CACHE_TTL;
snapshotRscResponse = nav.snapshotRscResponse;
restoreRscResponse = nav.restoreRscResponse;
});
afterEach(() => {
vi.restoreAllMocks();
delete (globalThis as any).window;
});
/** Helper: fill cache with `count` entries at a given timestamp. */
function fillCache(count: number, timestamp: number, keyPrefix = "/page-"): void {
const cache = getPrefetchCache();
const prefetched = getPrefetchedUrls();
for (let i = 0; i < count; i++) {
const key = `${keyPrefix}${i}.rsc`;
const body = `body-${i}`;
const buffer = new TextEncoder().encode(body).buffer;
cache.set(key, {
snapshot: {
buffer,
contentType: "text/x-component",
paramsHeader: null,
url: key,
},
timestamp,
});
prefetched.add(key);
}
}
describe("prefetch cache eviction", () => {
it("preserves X-Vinext-Params when replaying cached RSC responses", async () => {
const response = new Response("flight", {
headers: {
"content-type": "text/x-component; charset=utf-8",
"x-vinext-params": encodeURIComponent('{"id":"2"}'),
},
});
const snapshot = await snapshotRscResponse(response);
const restored = restoreRscResponse(snapshot);
expect(restored.headers.get("content-type")).toBe("text/x-component; charset=utf-8");
expect(restored.headers.get("x-vinext-params")).toBe(encodeURIComponent('{"id":"2"}'));
await expect(restored.text()).resolves.toBe("flight");
});
it("sweeps all expired entries before FIFO", () => {
// Use fixed arbitrary values to avoid any dependency on the real wall clock
const now = 1_000_000;
const expired = now - PREFETCH_CACHE_TTL - 1_000; // 31s before `now`
fillCache(MAX_PREFETCH_CACHE_SIZE, expired);
expect(getPrefetchCache().size).toBe(MAX_PREFETCH_CACHE_SIZE);
expect(getPrefetchedUrls().size).toBe(MAX_PREFETCH_CACHE_SIZE);
vi.spyOn(Date, "now").mockReturnValue(now);
storePrefetchResponse("/new.rsc", new Response("new"));
const cache = getPrefetchCache();
expect(cache.size).toBe(1);
expect(cache.has("/new.rsc")).toBe(true);
// All evicted entries should be removed from prefetched URL set
expect(getPrefetchedUrls().size).toBe(0);
});
it("falls back to FIFO when all entries are fresh", () => {
// Use fixed arbitrary values to avoid any dependency on the real wall clock
const now = 1_000_000;
fillCache(MAX_PREFETCH_CACHE_SIZE, now);
expect(getPrefetchCache().size).toBe(MAX_PREFETCH_CACHE_SIZE);
expect(getPrefetchedUrls().size).toBe(MAX_PREFETCH_CACHE_SIZE);
vi.spyOn(Date, "now").mockReturnValue(now);
storePrefetchResponse("/new.rsc", new Response("new"));
const cache = getPrefetchCache();
// FIFO evicted one, new one added → still at capacity
expect(cache.size).toBe(MAX_PREFETCH_CACHE_SIZE);
expect(cache.has("/new.rsc")).toBe(true);
// First inserted entry should be evicted
expect(cache.has("/page-0.rsc")).toBe(false);
// Second entry should survive
expect(cache.has("/page-1.rsc")).toBe(true);
// FIFO-evicted entry should be removed from prefetched URL set
expect(getPrefetchedUrls().size).toBe(MAX_PREFETCH_CACHE_SIZE - 1);
expect(getPrefetchedUrls().has("/page-0.rsc")).toBe(false);
});
it("sweeps only expired entries when cache has a mix", () => {
// Use fixed arbitrary values to avoid any dependency on the real wall clock
const now = 1_000_000;
const expired = now - PREFETCH_CACHE_TTL - 1_000;
const half = Math.floor(MAX_PREFETCH_CACHE_SIZE / 2);
const rest = MAX_PREFETCH_CACHE_SIZE - half;
fillCache(half, expired, "/expired-");
fillCache(rest, now, "/fresh-");
expect(getPrefetchCache().size).toBe(MAX_PREFETCH_CACHE_SIZE);
expect(getPrefetchedUrls().size).toBe(MAX_PREFETCH_CACHE_SIZE);
vi.spyOn(Date, "now").mockReturnValue(now);
storePrefetchResponse("/new.rsc", new Response("new"));
const cache = getPrefetchCache();
// expired swept, fresh kept, 1 new added
expect(cache.size).toBe(rest + 1);
expect(cache.has("/new.rsc")).toBe(true);
// All expired entries should be gone
for (let i = 0; i < half; i++) {
expect(cache.has(`/expired-${i}.rsc`)).toBe(false);
}
// All fresh entries should survive
for (let i = 0; i < rest; i++) {
expect(cache.has(`/fresh-${i}.rsc`)).toBe(true);
}
// Only fresh entries remain in prefetched URL set
expect(getPrefetchedUrls().size).toBe(rest);
});
it("does not sweep when cache is below capacity", () => {
// Use fixed arbitrary values to avoid any dependency on the real wall clock
const now = 1_000_000;
const expired = now - PREFETCH_CACHE_TTL - 1_000;
const belowCapacity = MAX_PREFETCH_CACHE_SIZE - 1;
fillCache(belowCapacity, expired);
vi.spyOn(Date, "now").mockReturnValue(now);
storePrefetchResponse("/new.rsc", new Response("new"));
const cache = getPrefetchCache();
// Below capacity — no eviction, all entries kept + 1 new
expect(cache.size).toBe(belowCapacity + 1);
// storePrefetchResponse only manages the prefetch cache — the caller
// (router.prefetch()) is responsible for adding to prefetchedUrls. So
// the new entry (/new.rsc) is NOT in prefetchedUrls here, and the count
// stays at belowCapacity (no evictions triggered).
expect(getPrefetchedUrls().size).toBe(belowCapacity);
});
});