fix(app-router): restore shallow history tree snapshots - #2885
fix(app-router): restore shallow history tree snapshots#2885james-elicx wants to merge 3 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
6397725 to
1d0dc78
Compare
|
/bigbonk review for issues |
Review: fix(app-router): restore shallow history tree snapshotsReviewed the full diff, ran the targeted unit suite ( FindingsNo blocking issues found. A few observations, none requiring a change: 1. 2. Map mutation during iteration in Dev/prod parity ✅The client history logic lives entirely in Behavioral correctness spot-checks ✅
LGTM. |
|
Review posted to PR #2885. SummaryI reviewed PR #2885 (restore shallow history tree snapshots). Verification performed:
Verdict: LGTM, no blocking issues. The reference-counted claim/release model for tree snapshots is sound, and coverage is thorough (unit + 8 new E2E scenarios ported from Next.js Two non-blocking observations flagged for future readers:
I also confirmed dev/prod parity (client history logic is shared via the browser entry the App Router prod server inherits; no Pages Router equivalent) and spot-checked the popstate short-circuit for non-regression, the same-index re-claim release, and the serialized |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
1d0dc78 to
f153c77
Compare
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
f153c77 to
c6b02d9
Compare
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): restore shallow history tree snapshots
Reviewed the full diff across all 7 source files and 4 test files. Verification performed:
tests/app-browser-history-controller.test.ts+tests/app-browser-entry.test.ts: 266 passedtests/shims.test.ts -t shallow: 5 passed (the ported Next.js shallow-routing shim cases)vp checkon the four largest changed source files: clean (format + lint + types)
Verdict: LGTM, no blocking issues. The reference-counted claim/release model is sound and the dev/prod parity story holds (client history logic lives in the shared app-browser-* helpers that the App Router prod server inherits via the built RSC entry; Pages Router has no equivalent shallow-routing path).
Correctness spot-checks that hold up
-
data?.__NAbypass parity is faithful.isAppOwnedHistoryStategates on!external && traversalIndex !== null, and the traversal index is only ever stamped by App Router bootstrap. Theshims.test.tscase that writes{ __vinext_historyIndex: 3, captured: true }and assertshistory.stateequals it verbatim confirms captured app-owned entries pass through untouched (no external marker, no re-wrapping), matching Next.js. -
No Pages Router regression from the shared patch. The patched
pushState/replaceStateis installed globally, but the App-Router-only runtime hooks are optional-chained (getNavigationRuntime()?.functions.X?.()), so they no-op without an App runtime. I confirmed Pages Router (router.ts) never stamps__vinext_historyIndex, so its redirectreplaceState(window.history.state ?? {}, ...)calls (router.ts:2249/2488/2568/2649) always take the external branch and still runcommitClientNavigationState(). The bypass path's skip of URL-notify is therefore only reached on runtime-issued traversal re-writes, where the popstate handler already drives notification (app-browser-entry.ts:2476). -
Same-URL replace now detaches a shared claimed tree. The new
replacesClaimedOrExternalTreeterm incommitNavigationHistorymakes a same-hrefreplace actually write history + release the claim at the current index, which previously fell through to the no-write branch. This is exactly what thesame-URL app replace detaches one shared pushState treeE2E test locks in. -
Graceful degradation on evicted snapshots. When
restoreTreeSnapshot()returns false (snapshot missing) or approval is rejected,restoreHistorySnapshotreturns false and the popstate handler falls through tohandlePopstate(event)for a real RSC traversal. No dead-end.
Non-blocking observations (agree with the earlier automated pass)
- Unbounded
#treeSnapshotsretention for claimed entries is deliberate, not a leak. Growth is bounded by userpushStatecount and mirrors Next.js keeping copied trees for the document lifetime. The traversal-index cache remains separately LRU-bounded. - Map mutation during iteration in
#releaseForwardTreeSnapshotClaims(deleting the current key mid-for...ofover.keys()) is well-defined per the ECMAScript Map iteration spec — safe, just subtle.
Fixture and testid dependencies (/shallow-test, push-path, data-testid=pathname, /shallow-test/sub shallow pathname, #content hash) all pre-exist and are consistent with shallow-routing semantics (no real sub/ route or anchor is required — the copied tree stays on /shallow-test).
|
Review posted to PR #2885. SummaryI reviewed PR #2885 (restore shallow history tree snapshots) in depth. Verification:
Verdict: LGTM, no blocking issues. The reference-counted claim/release model for tree snapshots is sound and thoroughly tested (unit + 6 new E2E scenarios ported from Next.js Key correctness checks I independently confirmed:
Non-blocking observations (concur with the earlier pass): unbounded I also verified all fixture/testid dependencies pre-exist and are consistent with shallow-routing semantics. |
|
Independent targeted Next.js E2E verification on immutable head
The checkout remained clean and the local/remote PR head SHA matched after the run. |
|
Independent exact-head review of I reviewed the full diff independently, including App Router raw/hash Validation considered:
I found no actionable parity gap, lifecycle bug, regression, or missing high-value test at this head. |
…fresh-pr2885-main-20260816
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
Review: fix(app-router): restore shallow history tree snapshotsReviewed the full diff (12 files, +1134/-45) at head
Verdict: LGTM — no blocking issues found. The reference-counted claim/release model for tree snapshots is sound, and the coverage (unit + 8 new E2E scenarios ported from Next.js Correctness checks I independently confirmed
Non-blocking observations (concur with prior passes)
LGTM. |
|
Review posted to PR #2885. SummaryI reviewed PR #2885 (restore shallow history tree snapshots) in depth. Verification:
Verdict: LGTM, no blocking issues. Key correctness checks I independently confirmed:
Three non-blocking observations flagged (unbounded-but-intentional snapshot retention, spec-safe Map mutation, benign |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): restore shallow history tree snapshots
Reviewed the full diff (12 files, +1134/-45) against main. Verification performed locally:
vp test run tests/app-browser-history-controller.test.ts→ 27 passedvp test run tests/app-browser-entry.test.ts tests/shims.test.ts→ 1549 passedvp checkon the 5 core changed source files → clean (format + lint + types)
Verdict: LGTM — no blocking issues found. The reference-counted claim/release model for tree snapshots is sound, and the correctness properties hold up under independent scrutiny.
Correctness checks I independently confirmed
-
isAppOwnedHistoryState/data?.__NAbypass parity is faithful. The gate!isExternalHistoryState(state) && readHistoryStateTraversalIndex(state) !== null(app-history-state.ts:294) correctly distinguishes the two re-pass cases:createExternalHistoryStatePreservingMetadataalways stamps__vinext_externalHistoryState, so re-passing a captured external shallow entry stays on the external/claim branch, while re-passing a captured app-owned entry (internal__vinext_historyIndex, no external marker) takes the verbatim bypass. Matches Next.js. -
Push index consistency.
claimCurrentHistoryTreeSnapshot("push")allocates#nextHistoryTraversalIndex + 1and thencommitHistoryTraversalIndex(historyIndex)advances the monotonic counter (app-browser-history-controller.ts:267,283), consistent withallocateNavigationHistoryTraversalIndex("push"). Monotonic (not stack-depth) indexing keepsresolveHistoryTraversalIntentback/forward detection correct across truncate-then-push. -
Same-index re-claim is release-safe.
#claimTreeSnapshotAtHistoryIndexearly-returns whenpreviousTreeSnapshotId === treeSnapshotIdand otherwise releases the prior claim before re-claiming, so a same-URL hashreplaceat the same index doesn't double-count or leak. -
App-owned replace releases the overwritten claim idempotently.
commitAppOwnedHistoryStateWrite("replace", …)reads the traversal index from the shim-capturedpreviousHistoryState(the entry being overwritten) and releases only when it was external/claimed. The push branch just truncates forward claims. All cleanup ops are idempotent. -
Graceful degradation. When
restoreTreeSnapshot()finds an evicted snapshot (or approval is rejected),restoreHistorySnapshotreturns false and the popstate handler falls through tohandlePopstate(event)for a real RSC traversal — no dead-end. -
Dev/prod parity holds. The client history logic lives in the shared
app-browser-*helpers and the patchedwindow.historyinshims/navigation.ts; the App Router prod server inherits it via the built RSC entry. The patchedpushState/replaceStateno-op their App-Router-only runtime hooks via optional chaining, so Pages Router (which never stamps__vinext_historyIndex) stays on the external branch and still runscommitClientNavigationState().
Non-blocking observations (concur with prior passes)
- Unbounded
#treeSnapshotsretention for claimed entries is intentional, not a leak — the browser gives no signal that a copied external entry is unreachable; growth is bounded by userpushStatecount and mirrors Next.js keeping copied trees for the document lifetime. - Map mutation during iteration in
#releaseForwardTreeSnapshotClaims(app-browser-history-controller.ts:338) is spec-safe (deleting the current/visited key over.keys()), just subtle. #treeSnapshotIdPendingFreshStatecan linger ifinvalidateRestorableClientState()runs without a followingrememberHistoryStateSnapshot()— benign; it only steers which id is reused on the next remember.
LGTM.
|
Review posted to PR #2885. SummaryI reviewed PR #2885 (restore shallow history tree snapshots) in depth against
Verdict: LGTM, no blocking issues found. Key correctness checks I independently confirmed:
Three non-blocking observations flagged (intentional unbounded snapshot retention, spec-safe Map mutation during iteration, benign |
1 participant
Summary
history.pushState()/replaceState()entriesFailure mapping
Fixes the non-cache failure in Next.js deploy-suite run 31439707085, job 93624401572:
test/e2e/app-dir/shallow-routing/shallow-routing.test.tsThe original targeted run reproduced the suite at 14 passed / 1 failed.
Validation
vp checkvp run knipvp run vinext#buildtests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts: 13 passed / 13 totalCI=1 PLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/advanced.spec.ts --grep "Shallow Routing" --retries=0: 11 passed / 1 pre-existing skippedscripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/shallow-routing/shallow-routing.test.ts: 15 passed / 15 totalThe initial draft CI exposed and locally reproduced a traverse-redirect regression. This head fixes it by matching Next.js
data.__NAbypass behavior for captured app-owned History state; the formerly failing test and its full 13-test file are green. Focused regressions also prove captured app-owned replace/push writes release overwritten or forward-truncated snapshot claims idempotently while preserving app-owned state.