Repository navigation
fix(app-router): preserve layouts across soft navigation - #2316
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: preserve layouts across soft navigation
Solid, well-tested change. The bfcache-identity–keyed preservation in reduceApprovedVisibleCommitState and the mergeElements prev-wins-for-preserved-ids change are coherent and well covered by the new reducer fixtures. One concern worth a closer look before merge, plus a couple of minor notes.
Concern: snapshot-counter release in the synchronous popstate restore path
packages/vinext/src/server/app-browser-entry.ts:365
The synchronous restore now calls commitClientNavigationState(navId). The old code called commitClientNavigationState() with no navId (so shouldReleaseSnapshot was false). Passing navId flips shouldReleaseSnapshot to true, so this now releases a navigationSnapshotActiveCount slot.
The problem is that the synchronous restore path never acquires a snapshot. restoreHistorySnapshotVisibleState (app-browser-navigation-controller.ts:626) commits via dispatchSynchronousVisibleCommit and does not call activateNavigationSnapshot() — unlike renderNavigationPayload, which pairs every activateNavigationSnapshot() (line 768) with a later commitClientNavigationState(navId) (the established balance, see app-browser-entry.ts:567).
Trace of the new flow:
- A prior navigation is in flight → it called
activateNavigationSnapshot()(count = 1). - popstate fires;
restoreHistoryStateSnapshot(...)runs and, viaonApprovedBeforeCommit, callsabortSupersededNavigation().AbortController.abort()only aborts the fetch — it does not synchronously release that navigation's snapshot. commitClientNavigationState(navId)(line 365) decrements the count (count = 0), releasing a snapshot it never acquired — i.e. the prior navigation's snapshot.- The aborted navigation later settles and releases its own snapshot through the commit/drain lifecycle (
drainPrePaintEffects→commitClientNavigationState(..., { releaseSnapshot: true })).
That's a double-release. In the two-navigation case the state.navigationSnapshotActiveCount > 0 guard at navigation.ts:1416 masks it (it won't go negative). But with a third concurrent navigation in flight, the count is decremented one too many times, which can make useSearchParams/useParams/usePathname stop reading the render snapshot one transition early (navigation.ts:1314/1338/1364).
Note that in the old code the release was correctly deferred: handlePopstate ran first and started the async traverse that activated the snapshot, and commitClientNavigationState() (no navId) intentionally did not release — the async no-commit path released it later. The new ordering removed that async activation but kept (and strengthened) the release.
Could you confirm the intended snapshot accounting here? Options:
- If the synchronous restore should own a snapshot slot, call
activateNavigationSnapshot()before the commit so the release is balanced. - If it should not touch the counter at all (it doesn't activate one), revert to
commitClientNavigationState()withoutnavId(or passnavIdonly for thependingPathnameclearing while keepingreleaseSnapshot: false).
A focused unit test asserting navigationSnapshotActiveCount returns to its starting value after a synchronous popstate restore that supersedes an in-flight navigation would lock this down. The current tests don't exercise the counter on this path.
Minor
app-browser-visible-commit.ts:163-170:action.slotBindings.find(...)inside thepreservePreviousSlotIds.filter(...)is O(n·m). Fine for typical slot counts, but per the perf guidance in AGENTS.md aMap<slotId, binding>built once would avoid the nested scan. Not blocking.- Fixture
action-revalidate/actions.tscallsrevalidateTag("action-revalidate-layout"), but nothing registers that cache tag, so the tag invalidation has no tagged data to clear. The e2e assertion still passes becausecrypto.randomUUID()re-renders on any refresh — so the test validates "fresh layout committed after a revalidating action," not tag-specific invalidation. Worth a comment so the intent is clear, or tag some cached data in the layout.
Looks good
- bfcache-identity gating of
preserveElementIds(mismatched/refresh-lane layouts correctly fall back to the freshnextvalue) — verified by the new reducer fixtures. resolveServerActionOperationLanecentralization and therefreshlane for revalidating server actions.mergeElementsprev-wins change is correctly bounded to the single production caller, which now passes the filteredpreserveElementIds.
|
Review posted to PR #2316. SummaryI reviewed the layout-persistence changes and posted a review with one notable concern and two minor notes. Main concern — snapshot-counter double-release in the synchronous popstate restore path ( The synchronous restore now calls Minor notes:
Verified clean: bfcache-identity gating of |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: layout preservation across soft navigation
The main concern from the prior review (snapshot-counter double-release in the synchronous popstate restore path) has been resolved by the follow-up commit e9b11d1 ("fix(navigation): balance history restore snapshots"), which postdates that review. Verified the fix and the rest of the change.
Resolved: snapshot-counter accounting
app-browser-entry.ts:365 now calls commitClientNavigationState(navId, { releaseSnapshot: false }), and commitClientNavigationState changed its guard to options?.releaseSnapshot ?? navId !== undefined (navigation.ts:1415). This keeps the navId-driven pendingPathname clearing while explicitly not releasing a snapshot the synchronous restore never acquired — exactly the "pass navId but keep releaseSnapshot: false" option suggested previously.
I confirmed the new test in tests/shims.test.ts actually locks this down: reverting the guard to the old navId !== undefined || options?.releaseSnapshot === true makes it fail (expected '/restored' to be '/in-flight'), and it passes on the current code. So useUntrackedPathname correctly keeps reading the in-flight render snapshot after a history-restore URL commit. No double-release.
Verified clean
mergeElementsprev-wins change (slot.tsx:377): bounded to the singleapp-browser-visible-commit.tscaller, andpreserveElementIdsis only non-empty underreuseCurrentBfcacheIds && lane !== "refresh"plus the bfcache-identity filter. Coherent with the layout-preservation goal and covered byslot.test.ts.resolveServerActionOperationLane(app-browser-action-result.ts): revalidating actions now take therefreshlane, which emptiespreserveElementIdsatapp-browser-visible-commit.ts:155so fresh layout output is committed. Non-revalidating actions stayserver-action. Lane values matchOperationLane.- Unit suites pass:
tests/shims.test.ts,tests/app-browser-entry.test.ts(211),tests/slot.test.ts(25).vp checkclean on all 7 changed source files.
Minor (non-blocking, still open)
app-browser-visible-commit.ts:164:action.slotBindings.find(...)inside thepreservePreviousSlotIds.filter(...)is O(n·m). AMap<slotId, binding>built once would avoid the nested scan per AGENTS.md perf guidance. Fine at typical slot counts.tests/fixtures/.../action-revalidate/layout.tsx+actions.ts: the layout renderscrypto.randomUUID()with no cached/tagged data registered for"action-revalidate-layout", sorevalidateTagActionhas nothing tag-specific to invalidate — the e2e passes because any refresh re-renders the UUID. The test validates "fresh layout committed after a revalidating action," not tag-scoped invalidation. A clarifying comment (or tagging real cached data) would make the intent explicit.
Both are minor and don't block. The core change is correct and well-tested.
|
Review posted to PR #2316. SummaryThe PR adds a follow-up commit Main concern — resolved: The snapshot-counter double-release in the synchronous popstate restore path is fixed. Verified clean:
Two minor non-blocking notes still open (carried from the prior review): the O(n·m) The core change is correct and well-tested. |
…arity # Conflicts: # packages/vinext/src/server/app-browser-visible-commit.ts
|
Fixed the post-merge HMR regression from the visible-commit conflict resolution. The reducer now refreshes planner-approved layout/page elements during HMR replace commits so fresh server-component errors reach the dev overlay, while still preserving the current route shell and slot-owner layouts needed for named parallel-route slot state. Validated locally with:
|
Summary
revalidatePath, andrevalidateTagcoverageNext.js parity impact
Targeted
test/e2e/app-dir/app/index.test.tsimproves from 94 passed / 9 failed / 5 skipped to 97 passed / 6 failed / 5 skipped, fixing same-layout rerender, soft back, and soft forward.Overlap
This is intended as a narrower, reviewed replacement/superseding attempt for draft PR #2215, and overlaps #2101 history restoration. It should not merge alongside those overlapping implementations.
Validation