mirror of
https://github.com/cloudflare/vinext.git
synced 2026-09-14 19:04:59 +08:00
ef4192db49
* 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>
184 lines
7.1 KiB
TypeScript
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);
|
|
});
|
|
});
|