Review: reuse retained interception history

Reviewed the full diff (10 files). The core change is sound: it replaces the prior "kick off async RSC traverse, then synchronously supersede it via a commit-version race" pattern with an explicit, restore-first path in the popstate handler, plus two new runtime functions (hasRestorableHistoryTarget / navigateRestorableHistoryTarget) that let <Link> skip prefetch and reuse the immediately-forward history entry. Tests pass (273), type check and lint are clean. Parity with the described Next.js behavior (first intercept fetches, repeated back/forward reuses history without network) looks correct.

A few observations, none blocking:

1. findCurrentIndex returns the first match, but the caller requires currentHistoryIndex + 1

packages/vinext/src/server/app-history-state.ts:51-66 iterates snapshots and returns the first historyIndex whose state matches the predicate. In hasRestorableHistoryTarget/navigateRestorableHistoryTarget (app-browser-entry.ts:2038-2060) the result is only accepted when it equals currentHistoryIndex + 1.

If the same href exists at multiple history indices (e.g. A → B → A produces two snapshots whose href is A), and the first match in Map-insertion order is not the forward entry but a later snapshot is, findCurrentIndex returns the earlier index and the +1 check fails. This is a false negative — it just degrades to the prior fetch/normal-navigation path, so it is not a correctness bug. But if you want the optimization to be robust to duplicate-href history entries, consider having findCurrentIndex accept the target index (or return all matches) rather than the first predicate hit. Worth a comment at minimum so a future reader knows the first-match semantics are intentional.

2. hasRestorableHistoryTarget check runs after createRscRequestUrl in the prefetch path

packages/vinext/src/shims/link.tsx:469-472 awaits createRscRequestUrl(fullHref, headers) and then checks hasRestorableHistoryTarget. Since the check only depends on fullHref, moving it above the createRscRequestUrl/header construction (e.g. right after interceptionContext/shouldPrefetch gating) would skip the wasted URL/header work on the common "already retained" path. Aligns with the AGENTS.md guidance to keep the request path lightweight. Minor.

3. In-flight navigateRsc is not aborted on a synchronous restore (parity preserved)

When tryRestoreHistorySnapshot wins, the new popstate path returns early and never calls navigate, so activeNavigationAbortController?.abort() (app-browser-entry.ts:1486) is not invoked for any in-flight RSC traverse. The in-flight navigation is still discarded because tryRestoreHistorySnapshot calls beginNavigation() (bumping activeNavigationId), causing the older navigate's commit to be classified no-commit. So behavior matches the old commit-version race (which also did not abort the network request). Confirmed parity, just flagging for awareness — the network request for a superseded traverse is not cancelled, only its commit is dropped.

4. beginNavigation() is called even when the restore fails

tryRestoreHistorySnapshot (app-browser-entry.ts:2086-2102) calls browserNavigationController.beginNavigation() before checking restoreHistoryStateSnapshot. On failure it returns false and the handler falls through to navigate, which calls beginNavigation() again. Because activeNavigationId is strictly monotonic and the navigate path re-captures the latest id, the extra increment is harmless. Noting it because it is a behavior change from the old ordering (restore previously ran after navigate); the double-increment-on-failure is benign but non-obvious.

Nits

• isSnapshotTargetHref import in app-browser-entry.ts:76 is out of alphabetical order (inserted before createSnapshotPathAndSearch). Lint doesn't flag it, so purely cosmetic.

Overall this looks correct and well-tested. Items 1 and 2 are the most worth addressing (a comment on the first-match semantics and hoisting the cheap check before createRscRequestUrl).

Re-review: reuse retained interception history (after 937ba69, dc0801a)

Re-reviewed the full 10-file diff after the two follow-up commits. The approach is meaningfully better than the version I reviewed earlier, and both substantive items from my first pass are now resolved. Tests pass (274 across the three named files), vp check on the touched files is clean.

Prior findings — status

1. findCurrentIndex first-match semantics — RESOLVED. The index lookup is now HistoryStateSnapshotCache.hasCurrentAtIndex (app-history-state.ts:51-63), which reads this.#snapshots.get(historyIndex) directly from a Map<number, snapshot>. There is no longer a first-predicate-match scan, so the duplicate-href false-negative I described can't occur. Nice.

2. Cheap check hoisted before createRscRequestUrl — RESOLVED. hasRestorableHistoryTarget now runs at link.tsx:458-463, ahead of getPrefetchInterceptionContext, createRscRequestHeaders, and the awaited createRscRequestUrl (link.tsx:480). The common retained path no longer pays for URL/header construction.

The restore-first popstate refactor (tryRestoreHistorySnapshot runs before navigate() in app-browser-popstate.ts:70-73, replacing the old commit-version race) is also a clear readability/robustness win and preserves the "no refetch on back" parity.

New observations (none blocking)

1. onRouterTransitionStart fires twice for a successful restorable Link click. When navigateRestorableHistoryTarget wins, navigateClientSide has already fired notifyAppRouterTransitionStart(fullHref, "push") (navigation.ts:1651); the helper then calls window.history.forward(), whose popstate fires notifyAppRouterTransitionStart(href, "traverse") again (app-browser-entry.ts:2088). In the normal push path, navigate() does not produce a popstate, so the hook fires once — this restorable path is a new double-notify ("push" then "traverse") for a single user click. Worth confirming against Next.js; instrumentation consumers (analytics/transition timing) may double-count. If Next reports once here, consider suppressing the line-1651 notify when the restorable path will be taken, or having the restorable path own the single notify.

2. pendingRetainedHistoryScrollPosition is consumed by the next popstate regardless of source. tryRestoreHistorySnapshot reads and clears the module-level scroll position at the top of every popstate (app-browser-entry.ts:2096-2097), before confirming the restore even succeeds. If another history traversal is dispatched in the task gap between navigateRestorableHistoryTarget's history.forward() and its popstate (e.g. a fast browser back), that intervening popstate consumes the retained scroll and applies it to the wrong entry, and the intended forward popstate then falls back to the snapshot's own scroll. Very narrow race and consistent with other module-level state in this file (synchronousPopstateScrollRestoreNavigationId), so not a regression in kind — flagging for awareness. Tagging the pending value with the expected target index (and ignoring it if the popstate index doesn't match) would close it.

Nits

• Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2101-2128): the two restoreSynchronousPopstateScrollPosition(...) calls have identical deps objects and differ only in the final state argument (historyState vs the synthesized {__vinext_scrollX, __vinext_scrollY}). Compute the state once into a local and call the helper a single time.
• Import order (app-browser-entry.ts:76): isSnapshotTargetHref is inserted before createSnapshotPathAndSearch, out of alphabetical order. Lint doesn't flag it; purely cosmetic.

Overall this is correct, well-tested, and an improvement over the earlier revision. Item 1 (the double onRouterTransitionStart) is the only one I'd recommend confirming against Next.js before merge; the rest are minor.

Re-review: reuse retained interception history (after 538f4fc)

Reviewed the new commit 538f4fc ("invalidate orphaned history snapshots") on top of the prior two passes, plus re-confirmed the carryover items. Tests pass locally (vp test run tests/shims.test.ts tests/link-navigation.test.ts → 1196 passed). The PR remains correct and well-tested.

New commit (538f4fc) — analysis

The commit wires an invalidateRestorableHistory runtime function into the patched window.history.pushState (navigation.ts:2153) so an external/user pushState clears the retained App Router snapshots. This is the right call: a browser pushState truncates the forward history stack, so the retained forward snapshot at currentIndex + 1 becomes stale and must not be reused by navigateRestorableHistoryTarget.

I checked two things that could have made this risky and both are fine:

1. Internal history writes do not falsely invalidate. vinext's own navigation uses pushHistoryStateWithoutNotify / replaceHistoryStateWithoutNotify (navigation.ts:1453-1473), which call state.originalPushState directly and bypass the patched wrapper. The other in-tree window.history.pushState call sites (link.tsx:233 applyPagesNavigationFallback, form.tsx:472, router.ts:2306) are all Pages Router paths, where invalidateRestorableHistory is an unregistered no-op. So only genuine external pushState triggers invalidation. Good.

2. replaceState intentionally not invalidated — correct. A replaceState only rewrites the current entry's URL and leaves forward entries intact, so the retained forward snapshot at currentIndex + 1 stays valid. Not invalidating there is the right asymmetry, not an oversight. (Worth a one-line comment on the patched pushState explaining why only push invalidates, so a future reader doesn't "fix" the perceived asymmetry by adding it to replaceState too.)

Carryover — the one item still worth confirming before merge

onRouterTransitionStart fires twice for a successful restorable Link click (with mismatched navigationType). This was flagged in the previous re-review and is still present:

• navigateClientSide fires notifyAppRouterTransitionStart(fullHref, "push") at navigation.ts:1651, before it calls navigateRestorableHistoryTarget at navigation.ts:1681.
• navigateRestorableHistoryTarget calls window.history.forward() (app-browser-entry.ts:2070), whose popstate handler unconditionally fires notifyAppRouterTransitionStart(window.location.href) (app-browser-popstate.ts:69), wired to "traverse" (app-browser-entry.ts:2088-2089).

Net effect: a single user click on a restorable <Link> emits onRouterTransitionStart(href, "push") then onRouterTransitionStart(href, "traverse"). The normal push path (navigate() with no popstate) emits it once. Instrumentation consumers (analytics, transition timing) will double-count and see a contradictory type pair for one interaction. Since this is the user-facing instrumentation contract, I'd confirm against Next.js whether a reused-history Link click reports once (traverse) or twice. If once, either suppress the line-1651 notify when the restorable path will be taken, or have navigateRestorableHistoryTarget own the single notify. This is the only item I'd resolve (or explicitly accept) before merge.

Minor (non-blocking)

1. pendingRetainedHistoryScrollPosition consumed by the next popstate regardless of source (app-browser-entry.ts:2097-2098). It's read+cleared at the top of every popstate before the restore is even confirmed. An interleaved traversal in the task gap between history.forward() and its popstate would apply the retained scroll to the wrong entry. Very narrow, consistent with the existing module-level synchronousPopstateScrollRestoreNavigationId pattern. Tagging the pending value with the expected target index (and ignoring it on index mismatch) would close it.

2. Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2102-2129): the two restoreSynchronousPopstateScrollPosition(...) calls share an identical deps object and differ only in the final state argument. Compute the state once (retainedHistoryScrollPosition === null ? historyState : { __vinext_scrollX, __vinext_scrollY }) and call the helper once.

3. Import order (app-browser-entry.ts:76): isSnapshotTargetHref is inserted before createSnapshotPathAndSearch, out of alphabetical order. Lint doesn't flag it; cosmetic.

Verdict

The new commit is sound and the push/replace asymmetry is deliberate and correct. The double onRouterTransitionStart is the only thing I'd recommend confirming against Next.js before merge; everything else is minor cleanup.

Re-review: reuse retained interception history (after 45ee5e4)

Reviewed the new commit 45ee5e4 ("preserve retained navigation lifecycle") on top of the prior three passes. This commit directly resolves the one carryover item I'd been flagging as the only thing to confirm before merge. Tests pass locally (260 across app-browser-entry.test.ts + link-navigation.test.ts), and vp check on the four touched files is clean.

Carryover — RESOLVED

Double onRouterTransitionStart for a restorable Link click — fixed. The popstate handler now skips the "traverse" notify when a retained navigation is in flight (app-browser-popstate.ts:71-74, gated on the new shouldSuppressTransitionStart dep wired to pendingRetainedHistoryNavigation !== null at app-browser-entry.ts:2114). A restorable Link click now emits a single notifyAppRouterTransitionStart(fullHref, "push") from navigateClientSide (navigation.ts:1651) and no longer double-fires "traverse" from the history.forward()-triggered popstate. This is the right fix and matches the "report once" expectation.

The lifecycle rework is also a genuine correctness improvement: navigateRestorableHistoryTarget now returns a Promise<void> | null that navigateClientSide awaits (navigation.ts:1679-1687), so the navigation promise no longer resolves before the traversal actually commits. The promise is settled from both popstate branches via settleRetainedHistoryNavigation (app-browser-popstate.ts:77, 95), and pendingRetainedHistoryNavigation is cleared inside the settle callback (app-browser-entry.ts:2104-2109), so there's no leak across the success or fallthrough path. The commitHashNavigation change to invalidate restorable state (app-browser-entry.ts:2043-2046) is also correct — a hash-only push truncates the forward stack, so the retained forward snapshot must not be reused.

New observations (none blocking)

1. Awaiting navigateRestorableHistoryTarget introduces a hang risk if history.forward() produces no popstate. This is the one behavior change in this commit I'd want explicitly accepted. Previously navigateClientSide returned synchronously after the === true fire-and-forget path (538f4fc:navigation.ts:1679-1685); now it awaits a promise that only resolves when the popstate fires and settleRetainedHistoryNavigation runs. navigateRestorableHistoryTarget guards on hasRestorableSnapshotAtIndex(currentIndex + 1, ...) before calling history.forward(), and the known forward-stack-truncation vectors are all covered (internal push overwrites the snapshot at currentIndex+1; external pushState invalidates via 538f4fc; hash push invalidates via this commit). So in practice the snapshot's presence implies a real browser forward entry. But if the retained snapshot and the actual browser forward stack ever desync (e.g. browser-side history-length eviction of the forward entry while the snapshot survives), history.forward() is a no-op, the popstate never fires, and the awaited promise — now feeding navigateClientSide / the caller's transition — hangs indefinitely. The old code degraded to a dropped commit; the new code degrades to a stuck navigation. A microtask/next-frame fallback that resolves (or falls through to a normal navigate) if no popstate arrives would close this. At minimum, worth a comment documenting the invariant that "a retained snapshot at currentIndex+1 implies a live browser forward entry" so the dependency is explicit.

2. pendingRetainedHistoryNavigation is consumed by the next popstate regardless of source (carryover, still present). tryRestoreHistorySnapshot reads pendingRetainedHistoryNavigation?.scrollPosition (app-browser-entry.ts:2116-2117) and shouldSuppressTransitionStart keys off the same module-level value, both before the popstate is confirmed to be the one history.forward() triggered. A fast interleaved traversal in the task gap between history.forward() and its popstate would consume the retained scroll/suppression on the wrong entry. Very narrow, consistent with the existing synchronousPopstateScrollRestoreNavigationId pattern. Tagging the pending value with the expected target index (and ignoring it on index mismatch) would close both this and the suppression mis-targeting.

Test gap

The new app-browser-entry.test.ts:5913 test covers the suppress-and-settle path only for a successful restore. It doesn't cover the case where shouldSuppressTransitionStart() is true but tryRestoreHistorySnapshot returns false — there the notify is skipped and the handler falls through to a real RSC navigate() traverse that fires no transition-start at all (the only notify for the whole interaction is the earlier "push" from navigateClientSide). That's arguably acceptable (one notify per user intent), but it's an untested branch with a deliberate-looking type semantics; a focused test asserting the single-notify + settle-via-real-navigation behavior would lock it in.

Verdict

The carryover double-notify item is cleanly resolved and the await-the-traversal rework is more correct than the fire-and-forget version. The only thing I'd recommend explicitly accepting (or guarding) before merge is the new hang-on-missing-popstate risk in item 1; everything else is minor/optional.

Re-review: reuse retained interception history (after 886cca6)

Reviewed the new commit 886cca6 ("serialize retained history traversals") on top of the prior four passes, and re-confirmed the carryover hang risk. Tests pass locally (vp test run tests/link-navigation.test.ts tests/app-browser-entry.test.ts → 261 passed), and vp check on the three touched source files is clean.

New commit (886cca6) — analysis

The commit serializes overlapping retained-history traversals. navigateRestorableHistoryTarget now returns Promise<boolean> | null (was Promise<void> | null), and navigateClientSide only treats a true resolution as "handled" — a false falls through to a normal RSC navigate(). When a retained traversal is already in flight, the new call short-circuits to pendingNavigation.promise.then(() => pendingNavigation.href === href) instead of issuing a second history.forward().

This is the right shape for the bug it targets. Without it, a second restorable click during an in-flight history.forward() would fire a second history.forward() against a browser that hasn't committed the first traversal yet, and the second pending state would clobber the first's resolve/scrollPosition. Serializing onto the in-flight promise and only claiming true when the destination matches is correct: the in-flight nav is guaranteed to land on its href (it was gated on hasRestorableSnapshotAtIndex(currentIndex+1, isSnapshotTargetHref(...href))), so a same-href second click is genuinely satisfied by it, and a different-href second click correctly waits and then does a normal navigate from the committed base. I traced both the A→A (coalesce) and A→B (wait-then-navigate) paths and the settle/clear bookkeeping (settleRetainedHistoryNavigation resolves once and nulls pendingRetainedHistoryNavigation in both popstate branches) and found no double-settle or leak.

Carryover — hang-on-missing-popstate is now amplified by serialization (item 1)

The prior review flagged that awaiting navigateRestorableHistoryTarget hangs if history.forward() never produces a popstate (a desync between the retained snapshot at currentIndex+1 and the browser's actual forward stack). This commit does not address that, and it widens the blast radius: pendingRetainedHistoryNavigation is cleared only inside settleRetainedHistoryNavigation, which runs only from the popstate handler (app-browser-entry.ts:2110-2118). So if the first retained traversal hangs, the module-level pendingRetainedHistoryNavigation is never nulled, and every subsequent retained navigation now takes the pendingNavigation !== null branch (app-browser-entry.ts:2062-2064) and chains onto the same never-resolving promise — so they all hang too, not just the first. Previously a hang was confined to a single navigation; now one stuck traversal can wedge all future restorable Link clicks until a full reload.

The known forward-stack-truncation vectors are still all covered (internal push overwrites the snapshot, external pushState invalidates via 538f4fc, hash push invalidates via 45ee5e4), so in practice the snapshot's presence implies a live forward entry and this stays theoretical. But given the serialized amplification, a guard is more warranted than before: resolve (or fall through to a normal navigate) if no popstate arrives within a microtask/requestAnimationFrame, and on that timeout null pendingRetainedHistoryNavigation so it can't wedge the queue. At minimum, document the invariant "a retained snapshot at currentIndex+1 implies a live browser forward entry, therefore history.forward() always yields a popstate" next to the window.history.forward() call so the dependency is explicit.

Test gap (item 2)

The two added Link-level tests mock navigateRestorableHistoryTarget to return Promise.resolve(true) / Promise.resolve(false) / null, so they verify only navigateClientSide's consumption of the boolean — they do not exercise the commit's actual new code, the pendingNavigation !== null serialization branch in navigateRestorableHistoryTarget itself (app-browser-entry.ts:2061-2064). That branch — second call with matching href resolving true, second call with mismatched href resolving false, and the single-history.forward() guarantee — is the headline behavior of this commit and is currently uncovered. A focused test (calling the registered runtime function twice with a stubbed history controller / history.forward) would lock in the serialization contract.

Minor / carryover (non-blocking)

1. pendingRetainedHistoryNavigation consumed by the next popstate regardless of source (carryover). shouldSuppressTransitionStart and the scroll read in tryRestoreHistorySnapshot (app-browser-entry.ts:2123,2126) both key off the module-level value before confirming the popstate is the one history.forward() triggered. A fast interleaved traversal in the task gap would consume the suppression/scroll on the wrong entry. The serialization narrows the intent overlap but does not tag by target index; tagging the pending value with the expected currentIndex+1 (and ignoring a popstate whose committed index doesn't match) would close this.

2. Rapid same-href double-click coalesces to a single forward entry. Two <Link href="/A"> clicks while the first is in flight resolve the second as true (handled) without producing a second history entry. A normal push-path double-click would push twice. Narrow rapid-double-click case; flagging as a deliberate-looking divergence rather than a bug.

3. Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2130-2157): the two restoreSynchronousPopstateScrollPosition(...) calls share an identical deps object and differ only in the final state argument. Compute the state once and call the helper once.

Verdict

The serialization is correctly shaped and resolves the overlapping-traversal race; the href-match true/false contract is sound and the settle bookkeeping is leak-free. No blocking issues. The one item I'd recommend addressing (or explicitly accepting) before merge is the now-amplified hang-on-missing-popstate risk — because a single wedged traversal can now block all subsequent restorable navigations — plus a direct test for the new serialization branch.

Re-review: reuse retained interception history (after c321d17)

Reviewed the new commit c321d17 ("match retained history fragments") plus the b75781d format-only follow-up, on top of the prior five passes. Tests pass locally (vp test run tests/link-navigation.test.ts tests/app-browser-entry.test.ts tests/app-browser-history-controller.test.ts → 277 passed), and vp check on the three touched source files is clean.

New commit (c321d17) — analysis, correct

The commit adds a hash field to ClientNavigationRenderSnapshot (navigation.ts:1129,1173) and introduces isSnapshotExactTargetHref (app-browser-navigation-controller.ts:260-272), which the two retained-history entry points now use in place of isSnapshotTargetHref (app-browser-entry.ts:2055,2071). The new helper is isSnapshotTargetHref(...) && targetUrl.hash === snapshot.hash.

This closes a real fragment-aliasing gap. isSnapshotTargetHref compares only path+search (createBasePathStrippedPathAndSearch vs createSnapshotPathAndSearch), so a retained forward snapshot for /page#section previously matched a no-hash /page Link click. navigateRestorableHistoryTarget would then history.forward() onto /page#section — the wrong URL — instead of falling back to a real navigation to /page. Requiring the snapshot's hash to match the target's fixes that.

Worth noting the net behavior given the two callers: both hasRestorableHistoryTarget (app-browser-entry.ts:2050) and navigateRestorableHistoryTarget (app-browser-entry.ts:2066) already early-out when targetUrl.hash !== "", so by the time isSnapshotExactTargetHref runs the target hash is always "". The new check therefore effectively reduces to "the retained forward snapshot must itself have no hash" — which is exactly the case that needed rejecting. So the guard is meaningful, not redundant, even though the targetUrl.hash === snapshot.hash form reads more general than the call sites exercise. The direct unit test (app-browser-history-controller.test.ts:434-440) covers the hash-present, hash-mismatch, and exact-match cases against isSnapshotExactTargetHref directly, which is the right level for this commit. Good.

Carryover — hang-on-missing-popstate (still unaddressed, still amplified)

This is the one item I keep flagging and it is unchanged by c321d17. Re-confirming it is still live:

• navigateRestorableHistoryTarget sets module-level pendingRetainedHistoryNavigation and calls window.history.forward() (app-browser-entry.ts:2080-2087), returning a promise resolved only by settleRetainedHistoryNavigation.
• settleRetainedHistoryNavigation runs only from the popstate handler (app-browser-popstate.ts:77, 95), and it is the only place that nulls pendingRetainedHistoryNavigation (app-browser-entry.ts:2110-2112).
• navigateClientSide awaits that promise (navigation.ts:1686).

If history.forward() ever fails to produce a popstate (a desync between the retained snapshot at currentIndex+1 and the browser's actual forward stack), the awaited promise never resolves and pendingRetainedHistoryNavigation is never nulled. Because of the serialization branch from 886cca6 (app-browser-entry.ts:2061-2064), every subsequent restorable navigation then chains onto the dead promise — so one wedged traversal blocks all future restorable Link clicks until a reload, not just the first.

The known forward-stack-truncation vectors are all covered (internal push overwrites the snapshot; external pushState invalidates via 538f4fc; hash push invalidates via 45ee5e4; and c321d17 now additionally rejects hash-aliased forward entries, which tightens the snapshot↔browser correspondence further). So this stays theoretical in practice. But given the amplification, I'd still recommend either a requestAnimationFrame/microtask fallback that resolves the promise and nulls pendingRetainedHistoryNavigation if no popstate arrives, or — at minimum — a comment next to window.history.forward() documenting the invariant "a retained snapshot at currentIndex+1 implies a live browser forward entry, therefore history.forward() always yields a popstate." This is the only item I'd resolve or explicitly accept before merge.

Test gap (carryover)

The serialization branch added in 886cca6 (navigateRestorableHistoryTarget's pendingNavigation !== null short-circuit, app-browser-entry.ts:2061-2064) is still uncovered. The Link-level tests (link-navigation.test.ts:1693, 1719) mock navigateRestorableHistoryTarget to return Promise.resolve(true|false)/null, so they verify only navigateClientSide's consumption of the boolean, not the in-flight-coalescing path inside the registered runtime function. A direct test calling the runtime function twice (matching-href → resolves true, mismatched-href → resolves false, with a single history.forward()) would lock in the serialization contract.

Minor / carryover (non-blocking)

1. pendingRetainedHistoryNavigation consumed by the next popstate regardless of source. shouldSuppressTransitionStart (app-browser-entry.ts:2123) and the scroll read in tryRestoreHistorySnapshot (app-browser-entry.ts:2125-2126) both key off the module-level value before confirming the popstate is the one history.forward() triggered. A fast interleaved traversal in the task gap would consume the suppression/scroll on the wrong entry. Tagging the pending value with the expected currentIndex+1 and ignoring a popstate whose committed index doesn't match would close this.
2. Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2130-2157): the two restoreSynchronousPopstateScrollPosition(...) calls share an identical deps object and differ only in the final state argument. Compute the state once (retainedHistoryScrollPosition === null ? historyState : { __vinext_scrollX, __vinext_scrollY }) and call the helper once.
3. Import order (app-browser-entry.ts:76): isSnapshotExactTargetHref is inserted before createSnapshotPathAndSearch, out of alphabetical order. Lint doesn't flag it; cosmetic.

Verdict

c321d17 is a correct, well-targeted fix for fragment aliasing in retained-history reuse, with appropriate direct test coverage. No blocking issues. The carryover hang-on-missing-popstate risk (amplified by the 886cca6 serialization) remains the one thing I'd recommend guarding or explicitly accepting before merge, alongside a direct test for the serialization branch; everything else is minor cleanup.

Re-review: reuse retained interception history (after 08e3622)

Reviewed the new commit 08e3622 ("invalidate branched history snapshots") on top of the prior seven passes. Tests pass locally (vp test run tests/app-browser-history-controller.test.ts tests/app-browser-entry.test.ts tests/link-navigation.test.ts → 278 passed across the three files).

New commit (08e3622) — analysis

The commit fixes a genuine forward-branch bug. Two related changes:

1. Push index now branches at current + 1, not next + 1. allocateNavigationHistoryTraversalIndex("push") was #nextHistoryTraversalIndex + 1 (highest known index + 1) and is now (#current ?? #next) + 1 (app-browser-history-controller.ts:139). This is the correct browser semantics: a push from a back-navigated entry truncates the forward stack and the new entry takes current + 1, not highest + 1. The new #commitPushedHistoryTraversalIndex (:158-163) then sets #next = index (overwriting any higher value) rather than Math.max, which correctly models the truncation. I confirmed traverse commits are unaffected: a popstate traverse passes navigationKind="traverse" / historyUpdateMode=undefined (app-browser-popstate.ts:86, app-browser-entry.ts:1484-1485), so it never enters the === "push" branch and still goes through the targetHistoryIndex path at :315-317.

2. Branching pushes invalidate retained snapshots. #invalidateBranchedForwardHistory() (:165-172) clears retained state when #current < #next (i.e. there are higher forward entries that the push is about to orphan). Gating on #current < #next is right — a straight-line forward push (#current === #next) does not invalidate.

The direct test (tests/app-browser-history-controller.test.ts:189-220) covers the branching case and is at the right level. The approach is sound and fixes a real correctness gap.

Item 1 (should address before merge): bfcache version is stamped before the branch invalidation bumps it

In the push branch of commitNavigationHistory, the order is:

• :290 — historyState is built with bfcacheVersion: this.#restorableClientState.currentBfcacheVersion (read as version N)
• :304 — #invalidateBranchedForwardHistory() runs, and on a branching push that calls invalidateClientState() → #invalidateBfcacheIds() → currentBfcacheVersion becomes N+1
• :305 — #pushHistoryState(historyState, ...) persists the entry to window.history stamped with version N (the pre-bump value)

So on a branching push, the freshly-created history entry is persisted with a stale bfcache version (N) while the live controller version is N+1. The in-memory snapshot is fine — rememberHistoryStateSnapshot runs later in the layout effect (app-browser-entry.ts:1028) and stamps N+1 — but the persisted window.history.state carries N. When the user later traverses back to this just-created entry, isCurrentBfcacheVersion(historyState) reads the persisted N, compares against the live version (≥ N+1), and returns false. That drops restoredBfcacheIds and sets reuseCurrentBfcacheIds = false (app-browser-entry.ts:1528-1539), forcing the traverse to treat a cleanly-created entry as bfcache-invalidated. Correctness-safe (it degrades to a refetch rather than a wrong restore), but it's an unintended perf regression on exactly the branched entry this commit creates.

Fix is small: build historyState (or at least read currentBfcacheVersion) after #invalidateBranchedForwardHistory() runs, so the persisted entry carries N+1. The same ordering exists in commitHashOnlyNavigation (:242 reads the version, :251 invalidates, :252 pushes) and should be fixed in the same pass.

Item 2 (worth a comment / accept): invalidation is full-clear, not forward-only

#invalidateBranchedForwardHistory() calls invalidateClientState(), which does a full #snapshots.clear() plus the version bump (app-history-state.ts:160-163). A browser forward-branch only truncates the forward stack; the backward entries remain valid and restorable. By clearing all snapshots, this drops the still-valid backward snapshots too, so a subsequent back navigation refetches instead of restoring. There is no forward-only truncation API on HistoryStateSnapshotCache today (only clear() and per-index delete()), so the full clear is an implementation limitation rather than a scoped choice. Correctness-safe, but more aggressive than Next.js's forward-only truncation. If forward-only deletion is feasible (delete keys > #current), that would preserve back-restore; otherwise a one-line comment noting the deliberate over-invalidation would keep a future reader from assuming it's a bug.

Carryover — hang-on-missing-popstate (documented, accepted)

Unchanged by this commit and remains the documented-invariant resolution from bb8b4f9 (app-browser-entry.ts:2086-2090). Note that 08e3622 actually tightens the backing invariant: a branching soft push now invalidates the retained forward snapshot before the push commits, closing one more snapshot/forward-stack desync vector. The residual is still that the mitigation is a comment, not a requestAnimationFrame fall-through guard, and pendingRetainedHistoryNavigation is nulled only from the popstate handler — so a desync would still wedge subsequent restorable navigations. Theoretical given the now-broader truncation coverage.

Test gap (carryover, still open)

The 886cca6 serialization branch — navigateRestorableHistoryTarget's pendingNavigation !== null short-circuit — remains uncovered by a direct test (the Link-level tests mock the runtime function). Separately, the new 08e3622 test asserts the forward snapshot is dropped (hasRestorableSnapshotAtIndex(1, …) === false) but does not assert the bfcache-version ordering of the persisted entry; a test reading the pushed window.history.state's bfcache version after a branching push would have caught Item 1.

Verdict

08e3622 fixes a real forward-branch index/invalidation bug and is well-targeted. No blocking correctness issues — both Item 1 and Item 2 are correctness-safe and degrade only to extra refetches. I'd recommend fixing the bfcache-version stamping order (Item 1) before merge since it silently regresses bfcache reuse on the branched entry and is a one-line reorder in two functions; Item 2 and the carryovers are optional.

Re-review: reuse retained interception history (after 8592582)

Reviewed the new commit 8592582 ("invalidate history on native hash changes") on top of the prior eight passes, and re-confirmed the outstanding carryovers. The commit adds a window.addEventListener("hashchange", ...) listener (app-browser-entry.ts:2184-2187) wired through a new createNativeHashChangeHandler helper (app-browser-popstate.ts:35-37) that calls historyController.invalidateRestorableClientState() on every native hashchange.

New commit (8592582) — purpose is sound, but the invalidation is unconditional

The motivation is correct and closes a real gap. vinext's own hash navigations go through pushHistoryStateWithoutNotify / replaceHistoryStateWithoutNotify (navigation.ts:1455-1473), and the patched pushState already invalidates retained history (navigation.ts:2158). But a native anchor click (<a href="#section"> that vinext does not intercept) updates the URL without calling pushState, so it never hit the existing invalidation. Such a click branches the forward history stack, orphaning the retained forward snapshot at currentIndex+1, and navigateRestorableHistoryTarget could then history.forward() onto a stale entry. The new hashchange listener closes that vector. Good.

Item 1 (should address or explicitly accept before merge): hashchange also fires on hash-crossing back/forward traversals, where invalidation is wrong

hashchange is not specific to forward-branching navigations. Per the HTML spec, a history traversal (back/forward) whose old and new URLs differ only in fragment fires popstate then hashchange. So pressing back/forward across a hash boundary — /page#a ↔ /page#b, or /page ↔ /page#a — runs:

1. popstate → isSameAppRoutePopstateTarget(href) is true (path+search match, hash ignored), so the fast path commits the traversal index and restores scroll and returns (app-browser-entry.ts:2176-2181).
2. hashchange → the new handler unconditionally calls invalidateRestorableClientState().

A back/forward traversal does not truncate the history stack, so the retained snapshots are still valid — but step 2 wipes them anyway. Worse, invalidateClientState() is a full #snapshots.clear() plus a bfcache version bump (app-history-state.ts:160-163). The version bump is session-wide: after one hash-crossing back/forward, every persisted history entry's bfcacheVersion reads as stale, so isCurrentBfcacheVersion returns false on subsequent traversals → restoredBfcacheIds dropped / reuseCurrentBfcacheIds = false → refetch. So a single legitimate hash back/forward degrades bfcache reuse for the whole session, not just the retained interception snapshots.

This is correctness-safe (it degrades to refetch, never a wrong restore), but it's a broad perf regression triggered by an ordinary user action (back/forward to a #anchor), and it can undercut the retained-history reuse this PR is adding whenever the page mixes hash navigation with intercepted Link navigation.

The handler needs to distinguish a forward-branching native hash change (invalidate) from a popstate-driven hash change (do not invalidate). Options:

• Set a short-lived "popstate in progress" flag in the popstate listener and have the hashchange handler skip invalidation when it's set (popstate fires synchronously before hashchange, so a flag cleared on a microtask/next-frame works).
• Or, since the retained-reuse path early-outs on targetUrl.hash !== "" and vinext's own hash writes bypass the listener, only the forward-branching case actually orphans a snapshot — gate the invalidation on currentHistoryIndex < nextHistoryTraversalIndex (the same #invalidateBranchedForwardHistory predicate already used for soft pushes) so a non-branching traversal is a no-op.

At minimum, if this is intended (e.g. judged acceptable because hash + retained-interception co-occurrence is rare), it warrants a comment explaining that hash-crossing traversals deliberately over-invalidate.

Item 2 (test gap): the new test does not exercise the actual concern

tests/app-browser-entry.test.ts:5888-5896 only asserts that createNativeHashChangeHandler(fn)() calls fn once — i.e. it verifies the trivial wrapper forwards the call. The handler has no logic to distinguish branching from non-branching hash changes, so the test can't catch the over-invalidation in Item 1. A test that drives a hash-crossing popstate followed by hashchange and asserts retained snapshots survive (or are dropped, if over-invalidation is accepted) would lock in the intended semantics. The test's it(...) description ("when a fragment navigation branches history") also overstates what is verified — the handler invalidates on all hashchanges, branching or not.

Carryovers (unchanged by this commit)

1. Prior Item 1 — bfcache version stamped before branch invalidation — STILL OPEN. app-browser-history-controller.ts was not touched after 08e3622. In both commitNavigationHistory (:290 reads currentBfcacheVersion, :304 invalidates) and commitHashOnlyNavigation (:241-242 reads, :251 invalidates), a branching push persists window.history.state with the pre-bump version, so a later traverse back to the freshly-created entry reads it as bfcache-invalid and refetches. One-line reorder in both functions (read the version after #invalidateBranchedForwardHistory()). Correctness-safe, perf-only.

2. Hang-on-missing-popstate remains the documented-invariant resolution from bb8b4f9 (app-browser-entry.ts:2086-2090). 8592582 arguably tightens the backing invariant slightly (native hash branches now invalidate the forward snapshot), but the mitigation is still a comment, not a requestAnimationFrame fall-through guard, and pendingRetainedHistoryNavigation is nulled only from the popstate handler.

3. 886cca6 serialization branch (navigateRestorableHistoryTarget's pendingNavigation !== null short-circuit) is still uncovered by a direct test.

4. Minor: the pendingRetainedHistoryNavigation cross-popstate consumption race, the duplicated branch in tryRestoreHistorySnapshot, and the out-of-order import are all unchanged.

Verdict

8592582 correctly identifies and closes the native-anchor-click branching gap, but the listener is unconditional and also fires on hash-crossing back/forward traversals — where it over-invalidates and (via the bfcache version bump) regresses bfcache reuse session-wide for an ordinary user action. I'd recommend gating the invalidation on the branching condition (or a popstate-in-progress flag) before merge, and strengthening the test beyond the wrapper-forwarding assertion. The prior bfcache-version-stamping carryover (Item 1 from the 08e3622 review) also remains open — both are perf regressions, not correctness bugs.

Re-review: reuse retained interception history (after 629dbad, e46451c, 1ff3956)

Reviewed the three new commits that landed after the prior nine passes, and re-confirmed the carryovers. Tests pass locally (vp test run tests/app-browser-entry.test.ts tests/app-browser-history-controller.test.ts tests/link-navigation.test.ts → 282 passed; tests/shims.test.ts → 1144 passed), and vp check on the three touched source files is clean.

629dbad ("stamp branched history bfcache version") — RESOLVES prior Item 1

This is the one item I'd flagged as worth fixing before merge in the 08e3622/8592582 passes, and it's now fixed correctly. #invalidateBranchedForwardHistory() is moved above the bfcacheVersion read in both functions:

• commitNavigationHistory (app-browser-history-controller.ts:288-299): the invalidation now runs before historyState is built, so the persisted entry carries the post-bump version.
• commitHashOnlyNavigation (:237-249): same reorder. Note bfcacheIds is still read before the bump (:234-236) while bfcacheVersion is read after (:245) — that's correct, not a mismatch: the entry keeps the ids that were valid pre-branch and stamps them with the new version, which is exactly what a freshly-created branched entry should carry.

The new test assertion readHistoryStateBfcacheVersion(store.pushed[0]?.state) === 1 (tests/app-browser-history-controller.test.ts:221) directly locks in the persisted-version semantics that the prior review noted was missing. With this, a later traverse back to the branched entry reads it as bfcache-current and no longer forces a spurious refetch. Good — this carryover is closed.

e46451c ("preserve history on hash traversal") — RESOLVES 8592582 Item 1

The unconditional hashchange invalidation flagged last pass is now gated. The popstate listener sets suppressNextHashChangeInvalidation = true and clears it on setTimeout(..., 0) (app-browser-entry.ts:2175-2179); createNativeHashChangeHandler skips invalidation when the flag is set (app-browser-popstate.ts:39-42). This is the "popstate-in-progress flag" approach I suggested, and the timing is sound: per the HTML spec a hash-crossing traversal queues popstate then hashchange in the same task, so the flag is set synchronously before the suppressed hashchange and cleared on the next macrotask. A genuine forward-branching native anchor click (no preceding popstate) still invalidates. The added test covers both the suppressed and non-suppressed paths (tests/app-browser-entry.test.ts:5901-5910).

One narrow residual (non-blocking, no action needed): the flag is armed after every popstate, not just hash-crossing ones, and stays armed until the next macrotask. So a native forward-branching anchor hashchange that happens to fire in the same task as an unrelated (non-hash) popstate would be wrongly suppressed (a missed invalidation → stale forward snapshot → degrades to a history.forward() onto a wrong entry, then the existing snapshot guards fall back to a real navigate). This requires a popstate and a synchronous anchor activation to interleave within one task, which isn't a realistic user-interaction sequence — flagging only for completeness.

1ff3956 ("settle retained history lifecycle") — fixes a real coalescing bug

Two changes, both correct:

1. resolveCoalescedRetainedNavigation (app-browser-popstate.ts:45-51) replaces the inline pendingNavigation.promise.then(() => pendingNavigation.href === href) in the serialization branch (app-browser-entry.ts:2066-2072). The old form returned true for a same-href coalesced click regardless of whether the in-flight retained navigation actually succeeded — so a second same-href click could claim "handled" even when the first traversal fell through to a real navigate (resolved false). The new form is pendingHref === requestedHref && handled, which only reports handled when the in-flight nav both matched the href and resolved true. Given settleRetainedHistoryNavigation resolves true on synchronous restore (app-browser-popstate.ts:99) and true/false from the fallthrough pendingNavigation (:117, :127-130), the && handled propagation is correct. This is a genuine correctness improvement over what I reviewed in 886cca6.

2. shouldHandleSameRoutePopstate (:53-55) extracts the !hasPendingRetainedNavigation guard on the same-route popstate fast path into a tested helper (app-browser-entry.ts:2187-2190). This prevents a pending retained traversal's popstate from being short-circuited by the hash-only fast path before tryRestoreHistorySnapshot/settle runs. Behavior is unchanged from the prior inline guard; it's now covered by tests/app-browser-entry.test.ts:5960.

The new resolveCoalescedRetainedNavigation test (:5915) also partially closes the long-standing serialization test-gap carryover: the core coalescing logic (false is preserved for a matching href) now has direct coverage. The pendingNavigation !== null branch inside navigateRestorableHistoryTarget that calls the helper still isn't exercised end-to-end via the registered runtime with a live pending state, but the extracted helper carrying the actual logic is now tested at the right level.

Carryovers (unchanged, all non-blocking)

1. 08e3622 Item 2 — full-clear over-invalidation. #invalidateBranchedForwardHistory() → invalidateClientState() still does a full #snapshots.clear() (app-history-state.ts:160-163), dropping still-valid backward snapshots on a forward branch. Correctness-safe (degrades to refetch); there's no forward-only truncation API on the cache today, so it remains an accepted implementation limitation. A one-line comment noting the deliberate over-invalidation would still help a future reader.
2. Hang-on-missing-popstate remains the documented-invariant resolution from bb8b4f9 (app-browser-entry.ts:2094-2098). 629dbad/e46451c arguably tighten the backing invariant further (branched pushes and native hash branches now invalidate the forward snapshot), but the mitigation is still a comment, not a requestAnimationFrame fall-through guard, and pendingRetainedHistoryNavigation is nulled only from the popstate handler.
3. Minor: the pendingRetainedHistoryNavigation cross-popstate consumption race, the duplicated branch in tryRestoreHistorySnapshot (:2143-2169), and the out-of-order import (app-browser-entry.ts:76) are all unchanged.

Verdict

The three new commits resolve the two open "should address before merge" items (bfcache-version stamping and the unconditional hashchange invalidation) and additionally fix a real coalescing-correctness bug in the serialization branch. No blocking issues. The PR is correct, well-tested (282 + 1144 passing), and ready; the remaining carryovers are all correctness-safe and optional (full-clear comment, requestAnimationFrame hang guard, and the minor nits).

Re-review: reuse retained interception history (after f46e144)

Reviewed the new commit f46e144 ("serialize retained traversal barriers") on top of the prior ten passes, and re-confirmed the carryovers. Tests pass locally (vp test run tests/link-navigation.test.ts tests/app-browser-entry.test.ts → 266 passed) and vp check on the three touched source files is clean.

New commit (f46e144) — analysis, correctly shaped

The commit adds a new waitForPendingRestorableHistory runtime function and has navigateClientSide await it on the non-push path (navigation.ts:1650-1656), before stageAppNavigationFailureTarget / notifyAppRouterTransitionStart. The function returns the in-flight pendingRetainedHistoryNavigation.promise (or null when none is in flight) (app-browser-entry.ts:2064-2067).

This closes a real ordering gap. navigateRestorableHistoryTarget is push-only (navigation.ts:1688-1691), so before this commit a replace/router.replace() issued while a retained history.forward() traversal was still in flight would race ahead of the traversal's still-pending popstate commit — the replace would replaceHistoryStateWithoutNotify / navigateRsc against the pre-traversal entry, then the traversal would commit on top, leaving the two navigations interleaved. Awaiting the pending traversal first serializes them: the replace now runs only after the traversal commits and settles. I traced this:

• The replace path can't self-deadlock — a replace Link/router.replace never calls the push-only navigateRestorableHistoryTarget, so it only ever waits on a previously-started traversal from a different navigation.
• The two notifyAppRouterTransitionStart calls (the traversal's "traverse"/suppressed, then the replace's "replace") are two distinct user intents, so this is not a double-notify of the kind fixed in 45ee5e4.
• refresh() issues through rscNavigate directly (navigation.ts:1863), not navigateClientSide, so it's unaffected by the new mode !== "push" gate.

The added Link-level test (link-navigation.test.ts:1771) covers the intended behavior — the replace click defers navigate until waitForPendingRestorableHistory settles — at the right level.

Carryover — hang-on-missing-popstate, now widened to replace navigations (the one item to accept or guard)

This commit further amplifies the documented hang-on-missing-popstate invariant. waitForPendingRestorableHistory returns the same pendingRetainedHistoryNavigation.promise that is settled only from the popstate handler via settleRetainedHistoryNavigation (app-browser-entry.ts:2127-2135), and pendingRetainedHistoryNavigation is nulled only there (:2129) — invalidateRestorableClientState() clears the snapshot cache but does not null the pending value (confirmed at app-browser-history-controller.ts:203 / the invalidateRestorableHistory seam at :2063).

So the blast radius of a single wedged history.forward() (snapshot ↔ forward-stack desync) now extends one step further:

• 886cca6 made every subsequent restorable push chain onto the dead promise.
• f46e144 now makes every subsequent replace / router.replace() await it too — so a stuck traversal also wedges replace navigations, not just restorable Link pushes.

This stays theoretical given the now-broad truncation coverage (internal push overwrites the snapshot; external pushState via 538f4fc; hash push via 45ee5e4; native hash branch via 8592582; branched push via 08e3622; fragment aliasing rejected via c321d17), and the invariant is documented at app-browser-entry.ts:2098-2102. But each commit that makes more navigation paths depend on the popstate-only settlement raises the cost of the documented-comment-only mitigation. The defense-in-depth fix remains the same as flagged across the last several passes: a requestAnimationFrame/microtask fall-through in navigateRestorableHistoryTarget that resolves the promise and nulls pendingRetainedHistoryNavigation if no popstate arrives, which would convert all of these (push-coalesce, replace-await) hang vectors into graceful fall-throughs at once. At minimum, the waitForPendingRestorableHistory seam deserves a one-line note that it inherits the same forward-stack invariant.

Test gap (carryover, partially closed)

The 886cca6 serialization branch inside navigateRestorableHistoryTarget (the pendingNavigation !== null short-circuit, app-browser-entry.ts:2069-2076) is still not exercised end-to-end with a live pending state — the Link tests mock the runtime function. 1ff3956 added direct coverage of the extracted resolveCoalescedRetainedNavigation helper, which carries the actual coalescing logic, so the most important slice is now tested; the new f46e144 test similarly mocks waitForPendingRestorableHistory and verifies only navigateClientSide's consumption, not the registered seam reading pendingRetainedHistoryNavigation.

Minor / carryover (non-blocking)

1. Full-clear over-invalidation (08e3622 Item 2): #invalidateBranchedForwardHistory() → invalidateClientState() does a full #snapshots.clear(), dropping still-valid backward snapshots on a forward branch. Correctness-safe; no forward-only truncation API today. A one-line comment would help.
2. pendingRetainedHistoryNavigation cross-popstate consumption race: shouldSuppressTransitionStart and the scroll read in tryRestoreHistorySnapshot key off the module-level value before confirming the popstate is the one history.forward() triggered.
3. Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2147-2174) and the out-of-order isSnapshotExactTargetHref import (:76).

Verdict

f46e144 correctly serializes a real replace-vs-retained-traversal ordering race and is well-targeted. No blocking issues — the PR is correct and well-tested (266 passing). The only thing worth explicitly accepting or guarding before merge is that this commit widens the documented hang-on-missing-popstate failure mode to also wedge replace navigations (not just restorable pushes); a single requestAnimationFrame fall-through that nulls pendingRetainedHistoryNavigation would close all of those vectors at once. Everything else is the unchanged minor cleanup.

Re-review: reuse retained interception history (after 4d08897)

Reviewed the new commit 4d08897 ("verify retained traversal popstate") on top of the prior eleven passes, and re-confirmed the carryovers. Tests pass locally (vp test run tests/app-browser-entry.test.ts → 213; tests/link-navigation.test.ts tests/app-browser-history-controller.test.ts → 71) and vp check on the three touched source files is clean.

New commit (4d08897) — analysis, resolves the cross-popstate consumption race

This commit directly closes the long-standing pendingRetainedHistoryNavigation cross-popstate consumption race carryover I'd been flagging since the 45ee5e4 pass. Two coordinated changes:

1. Tags the pending value with its expected target index. pendingRetainedHistoryNavigation now carries expectedHistoryIndex: currentHistoryIndex + 1 (app-browser-entry.ts:2096), set at the moment navigateRestorableHistoryTarget issues history.forward().

2. Verifies the popstate before consuming the retained navigation. The popstate listener now checks isExpectedRetainedPopstate (app-browser-popstate.ts:57-67) — requiring both actualHistoryIndex === expectedHistoryIndex and a normalized full-href match (new URL(expectedHref, actualHref).href === actualHref) — before the retained traversal is allowed to consume the popstate. If an unrelated popstate fires in the task gap between history.forward() and its own popstate, the handler now nulls pendingRetainedHistoryNavigation and resolves the pending promise with false (app-browser-entry.ts:2195-2207), so navigateClientSide's await retainedHistoryNavigation (navigation.ts:1693) falls through to a real RSC navigate instead of mis-applying the suppression/scroll/settle meant for the retained entry.

I traced the four paths and they're all correct:

• Expected popstate (index + href match) → rejection block skipped, pendingRetainedHistoryNavigation stays set, shouldHandleSameRoutePopstate(true) → false → handlePopstate restores and settleRetainedHistoryNavigation resolves true. Unchanged, correct.
• Unrelated popstate (mismatch) → resolve false + null, then shouldHandleSameRoutePopstate(false) → true, the unrelated popstate is handled normally. No double-settle: handlePopstate → settleRetainedHistoryNavigation re-reads the now-null pending value and returns early (app-browser-entry.ts:2132-2134). Correct.
• No leak / no double-resolve — the promise is settled exactly once in each branch.
• Index source is sound — vinext push navigations always stamp traversalIndex onto window.history.state (app-browser-history-controller.ts:247,297), so the forward entry the snapshot points at carries currentIndex+1; readHistoryStateTraversalIndex(event.state) (app-browser-entry.ts:2200) reads that back, and c321d17 already guarantees the snapshot is hash-free, so the full-href equality holds for a genuine match.

This is the "tag the pending value with the expected target index (and ignore a popstate whose committed index doesn't match)" resolution I'd suggested across the last several passes. I consider the cross-popstate consumption race closed.

Carryover — hang-on-missing-popstate (narrowed, not closed)

The commit narrows but does not fully close the documented hang-on-missing-popstate invariant. It now handles the case where a different popstate fires while a retained traversal is in flight (that popstate triggers the rejection → resolve false → null), which means an interleaved traversal can no longer wedge the pending value. But the zero-popstate case — history.forward() producing no popstate at all (a true snapshot/forward-stack desync) — is still only guarded by the documented invariant (app-browser-entry.ts:2102-2106); nothing resolves or nulls pendingRetainedHistoryNavigation in that case, so the 886cca6 push-coalesce and f46e144 replace-await amplifications still apply to a fully-stuck forward. A requestAnimationFrame/microtask fall-through that resolves false and nulls the pending state would convert this last vector into a graceful fall-through; the documented-invariant-only mitigation remains acceptable given the now-broad truncation coverage (internal push, external pushState via 538f4fc, hash push via 45ee5e4, native hash branch via 8592582, branched push via 08e3622, fragment aliasing rejected via c321d17).

Test gap (this commit + carryover)

The new test (tests/app-browser-entry.test.ts:5927) covers isExpectedRetainedPopstate only as a pure helper, and only the all-differ (false) and all-match (true) cases. The && means a partial match must also reject, but neither partial case is exercised:

• index matches, href differs (e.g. a same-index entry the browser landed on with a different URL)
• href matches, index differs

A test for each partial case would lock in that both conditions are required. Separately, the integration path — the popstate listener actually rejecting an unrelated popstate and resolving the retained promise false so the caller falls through — is still only verified indirectly. And the 886cca6 navigateRestorableHistoryTarget pendingNavigation !== null serialization branch remains uncovered end-to-end (the Link tests mock the seam; 1ff3956 covers the extracted resolveCoalescedRetainedNavigation helper directly).

Minor / carryover (non-blocking)

1. Double onRouterTransitionStart on the rejection fall-through. A rejected retained click fires "push" (from navigateClientSide, navigation.ts:1660) then "traverse" (from the fall-through handlePopstate/same-route branch). This is the genuine-traversal fall-through case, so the second "traverse" notify is defensible (matches the accepted semantics where a fall-through real navigation owns its own transition) — flagging only because it's a path the 45ee5e4 suppression no longer covers once the pending value is nulled.
2. Full-clear over-invalidation (08e3622 Item 2): #invalidateBranchedForwardHistory() → invalidateClientState() does a full #snapshots.clear(), dropping still-valid backward snapshots on a forward branch. Correctness-safe; no forward-only truncation API today. A one-line comment would help.
3. Duplicated branch in tryRestoreHistorySnapshot (app-browser-entry.ts:2151-2178): the two restoreSynchronousPopstateScrollPosition(...) calls share an identical deps object and differ only in the final state argument. Compute the state once and call the helper once.
4. Import order (app-browser-entry.ts:76): isSnapshotExactTargetHref is out of alphabetical order. Lint doesn't flag it; cosmetic.

Verdict

4d08897 is a correct, well-targeted fix that closes the cross-popstate consumption race (the standing minor carryover) by tagging the retained navigation with its expected index+href and verifying the popstate before consuming it. No blocking issues — the PR is correct and well-tested. The remaining items are all optional: the zero-popstate hang vector is narrowed but still relies on the documented invariant (a requestAnimationFrame fall-through would close it for good), plus the partial-match test gap and the unchanged minor cleanup.

Re-review: reuse retained interception history (after 41d6d13)

Reviewed the new commit 41d6d13 ("cancel invalidated retained traversal") on top of the prior twelve passes, and re-confirmed the carryovers. Tests pass locally (vp test run tests/app-browser-entry.test.ts → 214; tests/link-navigation.test.ts tests/app-browser-history-controller.test.ts → 71) and vp check on the two touched source files is clean.

New commit (41d6d13) — resolves the hang-on-missing-popstate carryover

This commit closes the long-running hang-on-missing-popstate item I've been flagging since the 45ee5e4 pass — the one residual that every subsequent commit (886cca6 push-coalesce, f46e144 replace-await) kept amplifying. The fix is the defense-in-depth I'd recommended: invalidateRestorableClientState no longer just clears the snapshot cache, it now also nulls and settles the in-flight retained navigation.

A new invalidateRestorableHistory helper (app-browser-entry.ts:418-423) snapshots pendingRetainedHistoryNavigation, nulls the slot, then calls cancelPendingRetainedNavigation(retainedNavigation?.resolve) (app-browser-popstate.ts:69-73), which resolves the pending promise with false. It's now wired into every invalidation seam: the cache-clear path (clearClientNavigationCaches → invalidateRestorableHistory, :428), commitHashNavigation (:2060), the registered invalidateRestorableHistory runtime function (:2074), and the native hashchange handler (:2230). Previously these only called historyController.invalidateRestorableClientState() and left pendingRetainedHistoryNavigation dangling.

The net effect: any invalidation that fires while a retained history.forward() is in flight now unblocks the awaiting caller (the push-path await retainedHistoryNavigation and the f46e144 waitForPendingRestorableHistory replace-await), resolving false so they fall through to a real navigate instead of hanging on a promise that the popstate handler can no longer settle (because the slot is nulled). This converts the previously-theoretical wedge into a graceful fall-through for all the invalidation-triggered vectors.

I traced the ordering for double-settle and orphaned-popstate safety, and it's correct:

• No double-settle. After invalidateRestorableHistory nulls the slot, if the (now-orphaned) history.forward() popstate eventually fires, settleRetainedHistoryNavigation re-reads pendingRetainedHistoryNavigation (now null) and returns early (:2140-2142); the rejection block at :2204-2215 also reads null and is skipped. Even if both resolves reached the same promise, Promise resolution is idempotent, so the false settlement stands. No leak, no double-resolve.
• Orphaned popstate handled normally. Once the slot is nulled, shouldHandleSameRoutePopstate(false) → true, so the orphaned forward popstate is processed as an ordinary same-route traversal (:2217). No hang, no mis-applied scroll/suppression.
• refresh() interaction is benign. router.refresh() → clearNavigationCaches → invalidateRestorableHistory now also cancels an in-flight retained traversal. That's correct: the awaiting push resolves false and falls through to a real navigate, and the orphaned forward popstate is handled as a normal traversal.

The added test (tests/app-browser-entry.test.ts:5917-5923) asserts cancelPendingRetainedNavigation(fn) calls fn(false) — the right unit-level check for the new helper.

Residual on the new commit (non-blocking)

The true zero-popstate desync is still only narrowed, not fully closed. This commit closes every invalidation-triggered hang vector, but the pure case — history.forward() producing no popstate and nothing subsequently triggering an invalidation — still leaves pendingRetainedHistoryNavigation set with no resolver ever firing. In that scenario the awaiting caller still hangs and (via the serialization branch) can still wedge subsequent restorable navigations until something calls invalidateRestorableHistory or a reload occurs. The documented invariant (:2110-2114) plus the now-broad truncation coverage keeps this theoretical, and this commit meaningfully shrinks the window (any cache clear / hash nav / external pushState now rescues a stuck slot). A requestAnimationFrame/microtask fall-through in navigateRestorableHistoryTarget that resolves false + nulls the slot if no popstate arrives within a frame would be the only thing that closes it unconditionally — but given how much this commit narrows it, I'd consider the documented-invariant resolution acceptable.

Test gap on the new helper's integration. The added test covers cancelPendingRetainedNavigation as a pure helper only. The actual behavior this commit adds — that invalidateRestorableHistory resolves a live pendingRetainedHistoryNavigation and unblocks waitForPendingRestorableHistory / the push-await — isn't exercised end-to-end (no test sets a pending slot, calls invalidateRestorableHistory, and asserts the awaited promise resolves false). A focused test driving that sequence would lock in the rescue contract.

Carryovers (unchanged, all non-blocking)

1. Full-clear over-invalidation (08e3622 Item 2): invalidateRestorableClientState → invalidateClientState() still does a full #snapshots.clear() (app-history-state.ts:160-163), dropping still-valid backward snapshots on a forward branch. Correctness-safe; no forward-only truncation API today. A one-line comment would help a future reader.
2. Cross-popstate consumption race — closed in 4d08897 via the expectedHistoryIndex + href verification; remains closed.
3. 886cca6 serialization branch (navigateRestorableHistoryTarget's pendingNavigation !== null short-circuit, :2080-2086) is still not exercised end-to-end with a live pending slot; 1ff3956 covers the extracted resolveCoalescedRetainedNavigation helper directly, which is the most important slice.
4. Minor: the duplicated branch in tryRestoreHistorySnapshot (:2159-2186) — the two restoreSynchronousPopstateScrollPosition(...) calls share an identical deps object and differ only in the final state argument; and the out-of-alphabetical-order isSnapshotExactTargetHref import (:76). Both cosmetic.

Verdict

41d6d13 is a correct, well-targeted fix that resolves the headline carryover — the invalidation seams now settle and null the in-flight retained navigation, so every invalidation-triggered hang vector becomes a graceful fall-through with no double-settle or leak. No blocking issues — the PR is correct and well-tested. The only remaining items are optional: the pure zero-popstate vector is now strongly narrowed but still relies on the documented invariant (a requestAnimationFrame fall-through would close it for good), an end-to-end test for the new rescue path, and the unchanged minor cleanup (full-clear comment, duplicated branch, import order).