fix(app-router): preserve active slots across segment-cache refresh

[james-elicx]

Summary

• preserve active parallel-slot state across App Router segment-cache refresh and prefetch paths
• include mounted-slot context in navigation, prefetch, and server-action refresh requests
• keep committed navigation snapshots reusable without losing active slot information

Validation

• vp check on touched source/test groups
• focused Vitest coverage for mounted slots, action refresh, browser commit/cache state, and Link active-slot prefetch headers
• targeted Next.js e2e: test/e2e/app-dir/segment-cache/refresh/segment-cache-refresh.test.ts (3/3 passed)
• independent review: NO FINDINGS

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: preserve active slots across segment-cache refresh

Reviewed the full diff (23 files). The core mechanism — a new X-Vinext-Mounted-Slot-Active-Routes side-channel header that carries slotId=routeId pairs so the server can rerender/preserve active parallel slots during refresh, prefetch, and action-rerender — is coherent, well-tested, and centralized behind normalizeRscRequest (single ingestion point shared by dev and prod RSC handlers, so dev/prod parity holds). Type check and the touched Vitest files (app-browser-entry, app-page-element-builder, app-server-action-execution, link-navigation, entry-templates) all pass.

No blocking issues found. A few observations, roughly in priority order:

1. resolveRouteSlotId fallback diverges from the canonical slot-id computation (potential silent miss)

app-page-element-builder.ts:575-586 computes a slot id when slot.id is absent:

const layoutIndex = slot.layoutIndex && slot.layoutIndex >= 0 ? slot.layoutIndex : 0;
const treePosition = route.layoutTreePositions?.[layoutIndex] ?? 0;

The canonical wiring path (app-page-route-wiring.tsx:513) uses slot.layoutIndex >= 0 ? slot.layoutIndex : layoutEntries.length - 1 and resolves the tree path from the actual resolved layout entry. When layoutIndex < 0 these disagree (0 vs last layout), so activeSlotRoutes.get(slotId) would silently miss and the active slot would not be preserved. In practice slot.id is populated from the route graph (AppRouteGraphParallelSlot.id is "Required"), so the fallback is rarely hit — but the manifest emits id: slot.id ?? null, so the null path is reachable. Consider either asserting slot.id is present here or making the fallback match the wiring's layoutEntries.length - 1 semantics. Also note slot.layoutIndex && ... treats layoutIndex === 0 as falsy; it lands on the right answer by coincidence, but typeof slot.layoutIndex === "number" && slot.layoutIndex >= 0 would be clearer.

2. renderObservation metadata is cast without validation, and its consumer can throw

app-elements-wire.ts:860-862 accepts renderObservation with only a shallow typeof === "object" check, then casts to RenderObservation. This is inconsistent with every adjacent field (slotBindings, interception, cacheEntryReuseProof) which have strict parsers. The consumer hasObservedDynamicRenderWork (app-browser-entry.ts) then does observation.dynamicFetches.length and observation.requestApis.some(...) unguarded, which would throw a TypeError on a malformed object. It is self-generated RSC metadata (not a cross-origin header) and the call sites are inside the snapshot-store try/catch, so severity is low — but given the strictness of the surrounding parsers this is worth tightening for consistency.

3. Behavioral change: dynamicOnly revalidation no longer clears the client navigation cache

app-browser-action-result.ts and app-browser-server-action-client.ts change the gate from revalidation !== "none" to only clearing on staticAndDynamic. This is intentional (clearing the whole nav cache would defeat active-slot preservation) and the test was updated to expect false for dynamicOnly. Flagging it as the most consumer-visible semantic change so a reviewer confirms it matches Next.js segment-cache handoff behavior for revalidateTag/revalidatePath on dynamic content. Worth a link to the Next.js reference in the commit/PR body per repo convention.

4. Behavioral change: non-HMR active-slot preservation is loosened to any layout in the target's layoutIds

app-browser-visible-commit.ts:168-178 previously required the slot owner's layout to be a bfcache-compatible preserved element (unchanged bfcache id). For the non-HMR/non-refresh lane it now only requires targetLayoutIdSet.has(ownerLayoutId). This is the crux of the fix and is covered by the new tests, but it does allow preserving slot content when the owner layout id matches even if its bfcache id changed. Reviewers should confirm this can't retain semantically-stale slot content across a navigation that re-renders the owning layout.

5. Perf: App Router viewport prefetches now bypass idle scheduling

link.tsx changes prefetchUrl so if (priority === "high" || hasAppNavigationRuntime()) startPrefetch() runs immediately; only Pages Router low-priority fallbacks keep requestIdleCallback. Combined with useEffect → useLayoutEffect for the intersection-observer registration, this shifts more work onto the synchronous commit path. It appears necessary so a visible-link ping seeds the cache before refresh, but it is a real change to main-thread scheduling for viewport prefetches — worth a note in the PR body.

6. Minor: redundant header sets and a no-op wrapper

• In navigation.ts (_appRouter.prefetch) and link.tsx, mountedSlotActiveRoutesHeader is passed to createRscRequestHeaders({ mountedSlotActiveRoutesHeader }) and then set again via explicit headers.set(VINEXT_MOUNTED_SLOT_ACTIVE_ROUTES_HEADER, ...). createRscRequestHeaders already sets it (app-rsc-cache-busting.ts:297-302), so the explicit set is dead code. (It mirrors the pre-existing mountedSlotsHeader pattern, so it is consistent, just redundant.)
• resolveActionRerenderRenderMode() (app-server-action-execution.ts:385) just returns the APP_RSC_RENDER_MODE_ACTION_RERENDER_PRESERVE_UI constant with no added logic; the previous inline constant was clearer.

None of these block merge. Items 1 and 2 are the ones I would most want addressed (or explicitly acknowledged as safe) before shipping.

[ask-bonk]

Review posted to PR #2461.

Summary

I reviewed all 23 changed files in PR #2461. The core mechanism — a new X-Vinext-Mounted-Slot-Active-Routes header carrying slotId=routeId pairs so the server can preserve/rerender active parallel slots during refresh, prefetch, and action-rerender — is coherent, well-tested, and correctly centralized (single ingestion via normalizeRscRequest, so dev/prod parity holds). Type check and all touched Vitest files pass.

No blocking issues. Findings, in priority order:

1. resolveRouteSlotId fallback diverges from the canonical slot-id computation in app-page-route-wiring.tsx (0 vs layoutEntries.length - 1 when layoutIndex < 0), which could silently miss active-slot preservation. Rarely hit since slot.id is normally populated, but the manifest emits id ?? null so the fallback is reachable.

2. renderObservation cast without validation — inconsistent with strict parsers on all adjacent metadata fields; the consumer accesses .dynamicFetches.length/.requestApis.some() unguarded (low severity: self-generated, inside try/catch).

3. dynamicOnly revalidation no longer clears the client nav cache — intentional behavioral change worth confirming against Next.js.

4. Non-HMR active-slot preservation loosened to any layout in the target's layoutIds — the crux of the fix; reviewers should confirm it can't retain stale slot content.

5. App Router viewport prefetches now bypass idle scheduling (plus useEffect→useLayoutEffect) — a real main-thread scheduling shift.

6. Minor: redundant mountedSlotActiveRoutesHeader header sets and a no-op resolveActionRerenderRenderMode() wrapper.

Items 1 and 2 are the ones I'd most want addressed or explicitly acknowledged as safe before shipping.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review: preserve active slots across segment-cache refresh (post-f374819)

Re-reviewed after the two hardening commits. The two high-priority items from the prior review are now resolved, and I found one additional cache-correctness concern worth confirming before merge.

Resolved since last review

1. resolveRouteSlotId fallback now matches the canonical wiring. app-page-element-builder.ts:575-590 now uses typeof slot.layoutIndex === "number" && slot.layoutIndex >= 0 and falls back to Math.max((layoutPositions?.length ?? 1) - 1, 0) (last layout), matching app-page-route-wiring.tsx:513 (layoutEntries.length - 1). Since layoutEntries[i].treePath === createAppPageTreePath(routeSegments, layoutTreePositions[i] ?? 0) (wiring createAppPageLayoutEntries, line 360-372), the two slot-id computations now agree in the layoutIndex < 0 fallback. Good.

2. renderObservation is now validated. app-elements-wire.ts:806-825 (parseRenderObservationMetadata) checks Array.isArray on requestApis/dynamicFetches/cacheTags/pathTags and isUnknownRecord/typeof on the rest before casting, so the downstream observation.dynamicFetches.length / observation.requestApis.some(...) accesses in app-browser-entry.ts can no longer throw on a malformed object. Consistent with the adjacent strict parsers now.

New finding (worth confirming before merge)

The ISR RSC cache key does not incorporate mountedSlotActiveRoutesHeader, but that header can change the cached RSC output in cacheable render modes.

• app-page-cache.ts:311-318 builds the ISR key from cleanPathname + (mountedSlotsHeader, renderMode, interceptionContext) only — mountedSlotActiveRoutesHeader is not threaded in.
• isr-cache.ts:401-419 (appIsrRscKey) hashes slots:<mountedSlotsHeader>, the render-mode variant, and source:<interceptionContext> into the key variant — but not the active-routes header.
• The _rsc cache-busting search param does include VINEXT_MOUNTED_SLOT_ACTIVE_ROUTES_HEADER (app-rsc-cache-busting.ts:180), so the client prefetch cache and any URL-keyed CDN layer disambiguate correctly. But the server ISR key is built from cleanPathname (search params stripped), so that disambiguation does not reach the ISR store.
• The header is consumed on cacheable paths, not just refresh/action-rerender. In app-page-element-builder.ts:531-540, the !shouldRerenderActiveMountedSlots(renderMode) branch (navigation / PREFETCH_LOADING_SHELL) sets preserveMountedContent: true when activeRouteId === targetRouteId || !slotHasTargetPage. That flag then causes the slot to be omitted from the RSC payload in app-page-route-wiring.tsx:817-823 (preserveMountedContent && isRscRequest && mountedSlotIds.has(slotId)), which is a distinct output from the non-preserve case for a slot that does have a page.
• The header is emitted on prefetch/navigation requests (link.tsx:498-514, navigation.ts:1977-1987) and is set whenever there are active slot bindings (app-browser-entry.ts:1128-1131), so it is routinely present.

Net: two cacheable requests with identical cleanPathname / mountedSlotsHeader / renderMode / interceptionContext but different mountedSlotActiveRoutesHeader collide on the same ISR key while potentially rendering different RSC (one omits an active slot's page, the other includes it). That is a stale-slot / cache-poisoning shape for apps using parallel routes with active slots.

Neither isr-cache.ts nor app-page-cache.ts was touched in this PR, so this looks like the header was threaded through element building without extending the ISR key. Suggested fixes, in order of preference:

• Add mountedSlotActiveRoutesHeader to appIsrRscKey's variant (mirroring the existing slots: variant), and thread it through readAppPageCacheResponse / the key builder — analogous to how mountedSlotsHeader is already handled.
• Or, if active-slot preservation is intended to be dynamic-only, skip the ISR cache read/write when mountedSlotActiveRoutesHeader is present.

Please confirm which of these matches the intended behavior, and add a focused isr-cache/app-page-cache test asserting the key varies with the active-routes header. If Next.js's segment cache keys these variants separately (it uses the router state tree in the cache key), aligning with that would be the reference.

Carried-forward observations (from prior review, non-blocking)

3. dynamicOnly revalidation no longer clears the client nav cache — intentional; still worth a Next.js reference link in the PR body.
4. Non-HMR active-slot preservation loosened to targetLayoutIdSet.has(ownerLayoutId) (app-browser-visible-commit.ts:176) — the crux of the fix; covered by tests.
5. Viewport prefetches now bypass idle scheduling + useEffect→useLayoutEffect (link.tsx) — real main-thread scheduling shift; note it in the PR body.
6. Redundant explicit headers.set(VINEXT_MOUNTED_SLOT_ACTIVE_ROUTES_HEADER, ...) after already passing mountedSlotActiveRoutesHeader into createRscRequestHeaders (link.tsx:512-514, 568-573; navigation.ts:1986-1987) — dead code, since app-rsc-cache-busting.ts:297-302 already sets it. And resolveActionRerenderRenderMode() (app-server-action-execution.ts:25) is a no-op wrapper over a constant.

Item under "New finding" is the one I'd want addressed or explicitly acknowledged as safe before shipping. Type check and the touched Vitest files (app-page-element-builder, app-elements, app-browser-entry, app-server-action-execution, link-navigation) all pass locally.

[ask-bonk]

Review posted to PR #2461.

Summary

Re-reviewed after the two hardening commits (f374819). The two high-priority items from the previous review are now resolved, and I identified one new cache-correctness concern.

Resolved

1. resolveRouteSlotId fallback now matches the canonical wiring (Math.max((layoutPositions?.length ?? 1) - 1, 0) ≈ layoutEntries.length - 1, plus typeof slot.layoutIndex === "number"). Verified the two slot-id computations agree because layoutEntries[i].treePath === createAppPageTreePath(routeSegments, layoutTreePositions[i] ?? 0).
2. renderObservation is now validated via parseRenderObservationMetadata (Array.isArray checks on the arrays the consumer indexes), so the downstream accesses can no longer throw.

New finding (the one to address before merge)

The ISR RSC cache key omits mountedSlotActiveRoutesHeader, but that header can change the cached RSC output in cacheable render modes.

• app-page-cache.ts:311-318 builds the ISR key from cleanPathname + (mountedSlotsHeader, renderMode, interceptionContext) — no active-routes header.
• appIsrRscKey (isr-cache.ts:401-419) hashes slots:, render-mode, and source: variants but not the active-routes header.
• The _rsc cache-busting param does include it (app-rsc-cache-busting.ts:180), so client/CDN keying is fine — but that's stripped from cleanPathname, so it never reaches the ISR store.
• The header is consumed on the cacheable navigation/prefetch path (app-page-element-builder.ts:531-540 → preserveMountedContent → slot omission in app-page-route-wiring.tsx:817-823), and is routinely emitted on prefetch/navigation.

Net: two cacheable requests differing only in mountedSlotActiveRoutesHeader collide on the same ISR key while producing different RSC — a stale-slot shape for parallel-route apps. Neither cache file was touched in the PR, consistent with the header being threaded through element building without extending the ISR key. Suggested either adding it to the key variant or skipping ISR when the header is present, plus a focused cache test.

Items 3-6 from the prior review were carried forward as non-blocking. Type check and all touched Vitest files pass locally.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review: preserve active slots across segment-cache refresh (post-f6259ba)

Re-reviewed after the cache-keying commit. The one blocking finding from the previous review is now resolved, and I found no new blocking issues. Type check passes and all touched Vitest files pass locally (isr-cache, app-page-cache, app-page-element-builder, app-browser-entry, app-elements, app-server-action-execution, link-navigation — 597 tests).

Resolved since last review

The ISR RSC cache key now incorporates mountedSlotActiveRoutesHeader. Commit f6259ba threads the header the full length of the cacheable path and keys on it:

• appIsrRscKey (isr-cache.ts:404-424) now normalizes the header via normalizeMountedSlotActiveRoutesHeader and adds an active:<hash> variant alongside the existing slots: / source: / render-mode variants. The new isr-cache.test.ts case confirms the key is order-insensitive (normalized) and varies with a different active-route mapping.
• The header is forwarded through every layer: entry (app-rsc-entry.ts:862) → dispatchAppPage (app-page-dispatch.ts:633,1072) → renderAppPageLifecycle (app-page-render.ts:862) → both the cache read key (app-page-cache.ts:319) and the cache write key (app-page-cache.ts:416, app-page-cache-finalizer.ts:262). The read/write keys now agree, so no read/write skew.
• Verified against the two ways the shared appIsrRscKey is invoked in the generated entry: the full-arg call (isrRscKey: __isrRscKey) forwards all five positional args, and the single-arg seedMemoryCacheFromPrerender call (app-rsc-entry.ts:676) is a build-time full-page prerender that never carries an active-slot header, so keying on the base variant there is correct.
• Client/CDN keying already included the header via the _rsc cache-busting hash (app-rsc-cache-busting.ts:180), so both the URL-keyed and server ISR layers are now consistent.

The stale-slot / cache-poisoning shape flagged previously (two cacheable requests differing only in the active-routes header colliding on one ISR key) can no longer occur.

Also confirmed the action-rerender path does not reintroduce the issue: action rerenders use ACTION_RERENDER_PRESERVE_UI, so shouldRerenderActiveMountedSlots returns true and slots are re-rendered rather than omitted, and action responses are POSTs that don't hit the ISR store.

Carried-forward observations (non-blocking, unchanged)

3. dynamicOnly revalidation no longer clears the client nav cache (app-browser-action-result.ts, app-browser-server-action-client.ts) — intentional; still worth a Next.js reference link in the PR body.
4. Non-HMR active-slot preservation is gated by action.reuseCurrentBfcacheIds && lane !== "refresh" && targetLayoutIdSet.has(ownerLayoutId) (app-browser-visible-commit.ts:168-177). Re-read this path: it only reuses the previous slot render when the owning layout is still part of the target tree and the new route doesn't override the slot — which matches Next.js segment-cache behavior for active parallel slots. Covered by the new tests.
5. Viewport prefetches bypass idle scheduling + useEffect→useLayoutEffect (link.tsx) — real main-thread scheduling shift; note it in the PR body.
6. Redundant explicit headers.set(VINEXT_MOUNTED_SLOT_ACTIVE_ROUTES_HEADER, ...) after already passing the value into createRscRequestHeaders (link.tsx:512-514,568-573; navigation.ts:1986-1987) — dead code since app-rsc-cache-busting.ts:298-301 sets it. Mirrors the pre-existing mountedSlotsHeader pattern, so it's consistent, just redundant. And resolveActionRerenderRenderMode() (app-server-action-execution.ts) is a no-op wrapper over a constant.

Verdict

No blocking issues. The core mechanism (a normalized X-Vinext-Mounted-Slot-Active-Routes side channel, ingested once in normalizeRscRequest for dev/prod parity, cardinality-bounded to 16 sorted slotId=routeId pairs) is coherent, well-tested, and now correctly participates in the ISR cache key. Items 3-6 remain optional cleanup / PR-body documentation.

[ask-bonk]

Review posted to PR #2461.

Summary

Re-reviewed after the new cache-keying commit (f6259ba). The one blocking finding from the previous review is now resolved, and I found no new blocking issues. Type check passes and all 597 touched Vitest tests pass locally.

Resolved

The ISR RSC cache key now incorporates mountedSlotActiveRoutesHeader. I verified the fix is complete and correct:

• appIsrRscKey adds a normalized active:<hash> variant (isr-cache.ts:404-424).
• The header is threaded through every layer of the cacheable path: entry → dispatchAppPage → renderAppPageLifecycle → both the cache read and write keys, so read/write keys agree.
• Confirmed the build-time prerender-seed call correctly uses the base key (no header present there), and client/CDN _rsc keying already included the header — so both layers are consistent.
• Confirmed the action-rerender path doesn't reintroduce the issue (re-renders slots rather than omitting them; POSTs don't hit ISR).

The stale-slot / cache-poisoning collision flagged previously can no longer occur.

Non-blocking (carried forward)

Items 3-6 remain: the intentional dynamicOnly nav-cache behavior change, the active-slot preservation loosening (verified benign, matches Next.js segment-cache semantics), the viewport-prefetch scheduling shift, and redundant header sets / a no-op wrapper — all optional cleanup or PR-body documentation.

Verdict: no blocking issues.

github run

[james-elicx]

Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the App Router refresh behavior rows from test/e2e/app-dir/segment-cache/refresh/segment-cache-refresh.test.ts.

Exact rows:

• segment cache (refresh) > router.refresh() refreshes both cached and dynamic data
• segment cache (refresh) > Server Action refresh() refreshes dynamic data only, not cached
• segment cache (refresh) > re-navigation to a fully static page does not overwrite dynamic slots with default content

Since this PR is already James-owned, I am skipping duplicate implementation work for these rows.

[james-elicx]

Deploy-suite run 31290819291 maps all three current failures in test/e2e/app-dir/segment-cache/refresh/segment-cache-refresh.test.ts to this existing James-owned fix:

• segment cache (refresh) > router.refresh() refreshes both cached and dynamic data
• segment cache (refresh) > Server Action refresh() refreshes dynamic data only, not cached
• segment cache (refresh) > re-navigation to a fully static page does not overwrite dynamic slots with default content

The targeted Next.js v16.2.6 validation on this PR already passed this exact upstream file 3/3, so I am skipping duplicate implementation work and leaving its branch untouched.

[james-elicx]

This PR is now conflicted and its implementation predates the current App Router cache architecture. I opened #2873 as the reviewed current-main replacement for the same segment-cache/refresh failures; its targeted Next.js E2E passes 3/3. I’ll leave this PR untouched while #2873 completes its gates.