feat: two-phase navigation commit with visited response cache

[NathanDrake2406]

Ref #639

Summary

Redesigns the App Router client-side navigation system to eliminate URL/hook desync flashes and enable instant back/forward navigation.

Core architecture changes

• Two-phase navigation commit — URL and history updates are deferred to useLayoutEffect after React commits the new tree, preventing usePathname()/useSearchParams()/useParams() from returning stale values during transitions
• Navigation render context — A React context (ClientNavigationRenderSnapshot) provides the pending URL and params to hooks during the render phase, before history is committed. This means components see consistent navigation state throughout the entire render cycle
• RSC response buffering — Cold navigation responses are fully buffered via snapshotRscResponse before passing to createFromFetch, ensuring the flight parser processes all rows in a single synchronous pass (prevents partial tree commits where list content updates before heading hooks catch up)
• Visited response cache — RSC responses are snapshotted to ArrayBuffer after each navigation and cached (30s TTL, 50 entry cap). Back/forward navigations replay from cache instantly. router.refresh() bypasses the cache
• Navigation snapshot activation counter — Hooks only prefer the render snapshot context during active transitions (tracked via ref-counted _navigationSnapshotActiveCount). After commit, hooks fall through to useSyncExternalStore so user pushState/replaceState calls are immediately reflected
• Non-blocking prefetch consumption — consumePrefetchResponse only returns settled responses synchronously, matching Next.js's segment cache behavior. Pending prefetches never block navigation (fixes Firefox nav bar hang)
• History notification suppression — pushHistoryStateWithoutNotify/replaceHistoryStateWithoutNotify wrap the original (unpatched) history methods to update the URL without triggering useSyncExternalStore subscribers prematurely
• Server action redirect fix — Immediate history.pushState before fire-and-forget __VINEXT_RSC_NAVIGATE__ with .catch cleanup, avoiding deadlock between the form's outer useTransition and renderNavigationPayload's inner startTransition

Unified navigation surface

• navigateClientSide() is now the single entry point for all client-side navigation (Link, Form, router.push/replace)
• Link and Form no longer manipulate history directly — they delegate to navigateClientSide which coordinates with __VINEXT_RSC_NAVIGATE__
• Hash-only changes, scroll restoration, and animation suppression are handled centrally in navigateImpl

Next.js parity notes

This brings vinext closer to Next.js for common navigation cases (forward nav, back/forward, prefetch reuse, shallow routing):

• Page-level caching vs Next.js's segment-level caching — vinext caches and replays the entire RSC response per URL. Simpler but coarser; a back/forward replays the whole page tree, not just changed segments
• Two-phase commit is a straightforward stage → render → commit-in-useLayoutEffect flow, vs Next.js's 2000+ line router reducer state machine
• Snapshot/restore via ArrayBuffer is minimal compared to Next.js's multi-tier Flight cache

Known limitation: CSS animation replay on navigation

vinext replaces the entire page tree on navigation, causing all CSS animations (fade-in, slide-in) to replay on freshly-mounted elements. Next.js avoids this architecturally via segment-level caching — only changed segments re-mount, so unchanged layouts/templates keep their DOM elements. This is a deeper architectural issue tracked separately from this PR.

Test plan

• pnpm test tests/shims.test.ts tests/link.test.ts tests/app-router.test.ts — all pass (1074 tests)
• pnpm run test:e2e — all pass (501 tests, 0 failures)
• pnpm run check — zero lint, type, or formatting errors
• Shallow routing: pushState/replaceState updates reflected in usePathname/useSearchParams
• Server action redirect() navigates correctly
• Cold Link navigation keeps URL state aligned with committed tree
• Animation suppression on mount-time row animations

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@643

commit: efe7f40

[NathanDrake2406]

@james-elicx should I split this out?

[james-elicx]

@james-elicx should I split this out?

Smaller PRs are generally preferred. These bigger ones start to get hard to review - I like to go through diffs myself as well as the bot...

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Two-phase navigation commit with visited response cache

This is a substantial redesign of the client-side navigation system. The core architecture — deferring URL/history updates until useLayoutEffect after React commits — is well-motivated and the right approach for preventing hook desync flashes. The visited response cache, RSC response buffering, and non-blocking prefetch consumption are all solid additions.

That said, there are several correctness concerns, a duplicated scroll restoration mechanism, and the PR size makes it harder to verify edge cases. I agree with the maintainer's suggestion to split this into smaller PRs.

High-level observations

1. _navigationSnapshotActiveCount is a module-level variable, not on the global state object. The ClientNavigationState was carefully moved to a Symbol.for-keyed global to survive multiple module instances (Vite can load navigation.ts through different resolved IDs). But _navigationSnapshotActiveCount is a plain let at module scope, so if Vite loads two copies of this module, they'll have independent counters. One copy's activateNavigationSnapshot() won't be balanced by the other copy's commitClientNavigationState() decrement, causing hooks to permanently return stale snapshot values.

2. Duplicate scroll restoration for popstate. navigation.ts has restoreScrollPosition (lines 833-861, called from its own popstate listener at line 1215 for Pages Router). app-browser-entry.ts adds restorePopstateScrollPosition (lines 334-345, called at line 619 for App Router). Both exist for different code paths, but the implementations diverge — the browser-entry version is simpler and doesn't wait for __VINEXT_RSC_PENDING__. The navigation.ts version waits but defers via microtask. This seems intentional since the browser-entry now handles scroll restoration after renderNavigationPayload resolves, but having two near-identical functions doing the same thing in two files is a maintenance hazard.

3. No cancellation of superseded navigations. If a user rapidly clicks Link A then Link B, both navigations run to completion. The _navigationSnapshotActiveCount counter handles overlapping activations, but the first navigation's renderNavigationPayload still runs updateBrowserTree and its commit effect still pushes to history. Only the later renderId resolves last, but the earlier one may also commit an intermediate URL to history. Consider adding an abort signal or navigation sequence check.

4. restoreRscResponse uses snapshot.body.slice(0) but the original ArrayBuffer is shared. ArrayBuffer.prototype.slice creates a copy, which is correct for allowing re-reads. However, the visited response cache stores the original ArrayBuffer — if snapshotRscResponse is called and the response body is consumed, the stored buffer is fine. But restoreRscResponse is called multiple times for cached entries, and each call creates a new copy. This is fine for correctness but worth noting for memory — 50 cached full-page RSC responses could be significant.

5. storePrefetchResponse is now async (breaking change for callers). The function signature changed from void to Promise<void>. The call site in link.tsx was updated to use prefetchRscResponse instead, and the old storePrefetchResponse is only used from tests. But if any external code imported it, this would be a silent behavioral change.

See inline comments for specific issues.

[ask-bonk]

This counter should live on the ClientNavigationState global object (keyed by Symbol.for), not as a module-level let. The rest of the client navigation state was moved to a global symbol specifically because Vite can load navigation.ts through multiple resolved module IDs (the virtual module resolution quirks documented in AGENTS.md). If two module instances exist, activateNavigationSnapshot() increments one copy's counter while commitClientNavigationState() decrements the other's, leaving hooks permanently stuck on the stale render snapshot.

Suggested change

	let _navigationSnapshotActiveCount = 0;
	// Stored on ClientNavigationState to survive multiple module instances.

[ask-bonk]

isThenable doesn't account for the case where value could be a thenable that isn't a native Promise (e.g., React's lazy wrappers, or createFromFetch return values). The "then" in value check would match any object with a then property (including objects with then: undefined). Consider checking that then is actually a function:

Suggested change

	function isThenable<T>(value: T | Promise<T>): value is Promise<T> {
	function isThenable<T>(value: T | Promise<T>): value is Promise<T> {
	return typeof value === "object" && value !== null && typeof (value as any).then === "function";
	}

[ask-bonk]

The comment says "requestAnimationFrame fires after the next paint" — but requestAnimationFrame fires before the next paint, not after. It's useLayoutEffect that fires before paint. The rAF callback scheduled from useLayoutEffect will fire before the following frame's paint, which means the navigation commit promise resolves ~16ms later than the actual DOM commit. If the intent is to resolve after paint, requestAnimationFrame is correct but the comment is misleading. If the intent is to resolve right after commit, you could resolve synchronously in useLayoutEffect.

[ask-bonk]

Setting a module-level variable from inside useLayoutEffect is a React anti-pattern — it creates a dependency on render timing and won't survive Strict Mode's double-invocation in development (the cleanup runs and nulls out setBrowserTreeState, then the second effect sets it again, which happens to work but is fragile). Consider using a useRef and exposing it through a stable callback ref, or storing the setter in the global ClientNavigationState object.

[ask-bonk]

The isSameRoute heuristic compares url.pathname === window.location.pathname, but at this point window.location.pathname hasn't been updated yet (the two-phase commit defers the history update). For cached routes served from the visited response cache (line 521-536), this comparison uses the old pathname from before navigation started. This means navigating A→B→A would treat the second A→B as isSameRoute=false on the first click but could incorrectly compute isSameRoute=true on a rapid second click if the first commit hasn't flushed yet.

Is this intentional? The comment says it controls startTransition usage, so a wrong value would affect UX (sync vs async update) but not correctness.

[ask-bonk]

Minor: nextNavigationRenderId (without ++) is used here instead of ++nextNavigationRenderId. This means the HMR update reuses the same renderId as the previous navigation. If a pending navigation commit for that renderId hasn't resolved yet, resolveCommittedNavigations would resolve it prematurely when the HMR update's NavigationCommitSignal mounts.

Suggested change

	nextNavigationRenderId,
	createClientNavigationRenderSnapshot(window.location.href, latestClientParams),
	++nextNavigationRenderId,
	false,

[ask-bonk]

The condition json !== state.clientParamsJson || json !== state.pendingClientParamsJson means this will set hasPendingNavigationUpdate = true even when the pending params haven't changed (because state.pendingClientParamsJson starts as null, and json !== null is always true). After the first call, subsequent calls with the same params will still set the flag because the || short-circuits on the first condition being false but the second being true (if state.pendingClientParamsJson was already set to json, the second clause is false, so it correctly skips — but only after the first call).

The real issue: on the very first replaceClientParamsWithoutNotify call, even if json === state.clientParamsJson, the function sets hasPendingNavigationUpdate = true because json !== null (since state.pendingClientParamsJson is null). Consider:

Suggested change

	if (json !== state.clientParamsJson || json !== state.pendingClientParamsJson) {
	if (json !== state.clientParamsJson && json !== state.pendingClientParamsJson) {

Using && would only set the pending update when the params actually differ from both the committed and the already-pending value.

[ask-bonk]

The getVisitedResponse function returns the cached entry for "traverse" navigations regardless of TTL (line 183-184). This means back/forward navigations never expire from the visited cache — a user could go back to a page hours later and still see the stale cached version. While the PR description mentions this is intentional for instant back/forward, it differs from Next.js which respects a 30s client-side cache TTL even for traversals (the staticPrefetchBailout behavior). The clearClientNavigationCaches on server actions helps, but navigations that don't trigger server actions won't invalidate.

Is there a plan for a separate traversal TTL or an LRU eviction for traversal entries?

[ask-bonk]

clearClientNavigationCaches() at the top of every server action callback is aggressive — it clears all prefetch and visited caches before the action even runs. If the action fails or doesn't revalidate anything, the caches are already gone. Consider clearing only after a successful action that triggers revalidation, or at least only clearing the visited cache (since prefetch data is still valid until revalidation).

[ask-bonk]

snapshot.body.slice(0) creates a full copy of the ArrayBuffer on every restoreRscResponse call. For the visited cache (50 entries, each potentially hundreds of KB for complex pages), each navigation that hits cache creates a copy. The slice is necessary because Response consumes the buffer, but consider whether a ReadableStream wrapping a Uint8Array view could avoid the copy:

// Zero-copy alternative:
const body = new ReadableStream({
  start(controller) {
    controller.enqueue(new Uint8Array(snapshot.body));
    controller.close();
  },
});
return new Response(body, { ... });

This avoids the allocation since Uint8Array(buffer) creates a view (no copy) and the Response constructor accepts ReadableStream.

[ask-bonk]

Review posted successfully. Here's a summary of the key findings:

Architecture — The two-phase navigation commit (stage → render → commit in useLayoutEffect) is a sound approach for preventing URL/hook desync flashes. The visited response cache, RSC response buffering, and non-blocking prefetch consumption are well-designed additions.

Key issues found:

1. _navigationSnapshotActiveCount lives at module scope instead of on the Symbol.for-keyed global state. This breaks if Vite loads two copies of navigation.ts — activate/deactivate calls from different module instances won't be balanced.

2. isThenable check is too loose — "then" in value matches objects with then: undefined. Should check typeof .then === "function".

3. Module-level setBrowserTreeState from useLayoutEffect is fragile under React Strict Mode's double-invocation.

4. HMR update reuses the same renderId (no ++), which could prematurely resolve pending navigation commits.

5. replaceClientParamsWithoutNotify condition uses || instead of &&, causing unnecessary hasPendingNavigationUpdate flag sets when params haven't changed.

6. Traverse navigations bypass TTL entirely — back/forward to a page hours later still serves the stale cache.

7. Aggressive cache clearing on every server action — clears all caches before the action runs, even if it fails.

8. restoreRscResponse copies the full ArrayBuffer on every call — a zero-copy ReadableStream approach could reduce memory pressure.

9. No cancellation of superseded navigations — rapid clicks push multiple URLs to history.

10. Duplicate scroll restoration logic across navigation.ts and app-browser-entry.ts — maintenance hazard.

11. Misleading comment about requestAnimationFrame firing "after" paint (it fires before).

I also agree with the maintainer's suggestion to split this into smaller PRs given the scope (~1950 additions across 19 files).

github run

[NathanDrake2406]

@james-elicx should be good now