Fix parallel slot persistence and cache variants#811
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
The overall approach is sound — tracking mounted-slot state on the client, sending it to the server via X-Vinext-Mounted-Slots, and using it to vary cache keys and suppress default.tsx re-emission on soft nav. The three-pronged fix (slot persistence, cache keying, prefetch/visited-response invalidation) is coherent and well-tested.
Main observations:
- Normalization divergence between the generated entry's
__normalizeMountedSlotsHeader(splits on" ") and the typed module'snormalizeMountedSlotsHeaderinapp-elements.ts(splits on/\s+/). They should match. - Prefetch dedup key doesn't incorporate slot state (
link.tsx:132), so a URL prefetched with one set of mounted slots won't be re-prefetched if slots change.consumePrefetchResponsecatches the mismatch at consumption time, but the prefetch slot is wasted. useLayoutEffectruns the full body (re-assigning module-level refs) on everytreeState.elementschange where onlysetMountedSlotsHeaderneeds element-level tracking. Minor perf nit — could split into two effects.- Stale background regen skips HTML write for RSC requests (
app-page-cache.ts:174), which is correct but means an RSC-triggered regen won't refresh the HTML cache variant. Worth a comment noting the intentional asymmetry.
The slot persistence logic in app-page-route-wiring.tsx:460-471 reads well and handles the key cases correctly. Stale mountedSlotIds from the client are safe because they would reference slots not present in route.slots, so the for loop would never encounter them.
Test coverage for the new behavior is solid — the route-wiring tests cover mounted/not-mounted/hard-nav, the cache tests verify slot-keyed reads and stale regen, and the prefetch tests verify slot-context matching/rejection.
None of the issues above are blocking.
| const normalized = Array.from( | ||
| new Set( | ||
| raw | ||
| .split(" ") |
There was a problem hiding this comment.
The generated entry splits on " " (single space) while the typed module normalizeMountedSlotsHeader in app-elements.ts:28 splits on /\s+/ (any whitespace). With consecutive spaces, .split(" ") produces empty strings that .filter(Boolean) cleans up, so the end result is the same for well-formed input. But the implementations should match to avoid subtle divergence.
| .split(" ") | |
| .split(/\s+/) |
| setMountedSlotsHeader(null); | ||
| }; | ||
| }, [dispatchTreeState]); | ||
| }, [dispatchTreeState, treeState.elements]); |
There was a problem hiding this comment.
Adding treeState.elements to the dependency array means this layout effect re-runs on every navigation (every time elements change). The cleanup tears down dispatchBrowserRouterAction and browserRouterStateRef, then re-assigns them — all unnecessary churn. Only setMountedSlotsHeader needs to track element changes.
Consider splitting into two effects: one stable effect for the dispatch/ref setup (deps: [dispatchTreeState]) and a second for setMountedSlotsHeader(getMountedSlotIdsHeader(stateRef.current.elements)) (deps: [treeState.elements]).
| ]); | ||
| ]; | ||
|
|
||
| if (!options.isRscRequest) { |
There was a problem hiding this comment.
When an RSC request triggers stale revalidation, only the RSC cache key (with the requesting client's mounted-slot variant) is regenerated — the HTML cache key is skipped.
This is correct for the immediate request, but it means an RSC-triggered regen won't refresh the HTML cache. If the HTML cache is also stale for the same pathname, it will require its own separate request to trigger regeneration. Might be worth a brief comment noting this is an intentional asymmetry.
| entry.snapshot = await snapshotRscResponse(response); | ||
| entry.snapshot = { | ||
| ...(await snapshotRscResponse(response)), | ||
| mountedSlotsHeader, |
There was a problem hiding this comment.
The snapshot from snapshotRscResponse(response) already reads X-Vinext-Mounted-Slots from the response headers (line 365). The spread then overwrites mountedSlotsHeader with the caller-provided value. This is correct (the caller knows the request context better), but since both values should agree, the double-read is slightly surprising. A brief comment explaining why the caller's value takes precedence would help readability.
| !overrideOrPageComponent && | ||
| defaultComponent && | ||
| options.isRscRequest && | ||
| options.mountedSlotIds?.has(slotId) |
There was a problem hiding this comment.
The core slot persistence logic reads well. One thought: if a slot has both a page and a default export, the overrideOrPageComponent will be truthy and this branch is skipped — correct. And if mountedSlotIds is null (initial page load, no header sent), mountedSlotIds?.has(slotId) is undefined (falsy) and the branch is also skipped — also correct.
Looks good.
|
Review posted successfully. Here's a summary of the findings: Overall: The PR is well-structured and addresses the parallel slot persistence bugs coherently across three layers (slot rendering, cache keying, and client-side cache invalidation). Test coverage is solid. Key inline comments posted:
None of the issues are blocking. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
The approach is solid and well-structured. The three-pronged fix (slot persistence via mountedSlotIds, cache key fanning by slot state, and prefetch/visited-response invalidation) is coherent and the test coverage is thorough.
I have a few observations — one concrete divergence (normalization regex in the generated entry vs the typed module), one structural awareness note (background regen forwarding the original request), and a few smaller nits.
Summary of findings
-
Generated entry normalization regex diverges from typed module — The
__normalizeMountedSlotsHeaderin the generated entry splits on/[\\t\\r\\n ]+/(tab, CR, LF, space) while the typed module inapp-elements.tsuses/\s+/(all Unicode whitespace). For practical purposes these agree on well-formed input, but they diverge on vertical tab, form feed, NBSP, etc. Easy to align. (See inline.) -
Background regen passes original
requesttobuildPageElements— In the stale regen closure, the originalrequestis forwarded, so regen renders with the requesting client'sX-Vinext-Mounted-Slotsheader. This means the RSC variant written is client-specific rather than canonical. This is the correct behavior for refreshing the requesting client's cache variant, but worth noting the regen is slot-specific by design. -
Prefetch dedup key doesn't include slot state —
prefetchUrlinlink.tsxdeduplicates onrscUrlalone. If mounted slots change between two prefetch calls for the same URL, the second is suppressed. Caught safely at consumption time (consumePrefetchResponserejects on mismatch), so correctness is preserved but the prefetch is wasted. Not blocking. -
useLayoutEffectseparation is correct — Already split into two effects: one stable for dispatch/ref (deps:[dispatchTreeState]), one reactive for slot tracking (deps:[treeState.elements]). Good structure. -
Test coverage is strong — Route-wiring tests cover mounted/unmounted/hard-nav. Cache tests verify slot-keyed reads, stale regen writing only RSC for RSC requests, and the prefetch tests verify slot-context match/reject. Good coverage of the key behavioral boundaries.
None of the findings are blocking.
| const normalized = Array.from( | ||
| new Set( | ||
| raw | ||
| .split(/[\\t\\r\\n ]+/) |
There was a problem hiding this comment.
This regex character class /[\\t\\r\\n ]+/ in the generated code resolves to tab, CR, LF, and space — which is correct for well-formed input. However, the typed module normalizeMountedSlotsHeader in app-elements.ts:28 uses /\s+/ which also covers vertical tab, form feed, NBSP, and other Unicode whitespace.
Aligning them would prevent any exotic-whitespace divergence:
| .split(/[\\t\\r\\n ]+/) | |
| .split(/\\s+/) |
This generates .split(/\s+/) in the output, matching the typed module exactly.
| if (!options.isRscRequest) { | ||
| // HTML remains canonical across slot-state variants; only RSC cache | ||
| // entries fan out by mounted-slot header. | ||
| writes.push( |
There was a problem hiding this comment.
The asymmetry here is correct but non-obvious. When an RSC request triggers stale regen, only the RSC slot-variant cache key is refreshed. The HTML cache (if also stale) requires a separate HTML request to trigger its own regen. Consider a brief comment:
| if (!options.isRscRequest) { | |
| // HTML remains canonical across slot-state variants; only RSC cache | |
| // entries fan out by mounted-slot header. | |
| writes.push( | |
| if (!options.isRscRequest) { | |
| // HTML cache is slot-state-independent (canonical), so only refresh it | |
| // during HTML-triggered regens. RSC-triggered regens only update the | |
| // requesting client's RSC slot variant; a stale HTML cache entry will | |
| // be regenerated independently by the next full-page HTML request. |
| const [streamForClient, streamForCache] = response.body.tee(); | ||
| const htmlKey = options.isrHtmlKey(options.cleanPathname); | ||
| const rscKey = options.isrRscKey(options.cleanPathname); | ||
| const rscKey = options.isrRscKey(options.cleanPathname, null); |
There was a problem hiding this comment.
finalizeAppPageHtmlCacheResponse correctly writes the RSC cache entry with mountedSlotsHeader: null — the HTML path produces a canonical (slot-independent) RSC payload. Consistent with the overall design.
|
|
||
| schedule(() => { | ||
| if (typeof window.__VINEXT_RSC_NAVIGATE__ === "function") { | ||
| const mountedSlotsHeader = getMountedSlotsHeader(); |
There was a problem hiding this comment.
The mounted-slot header is captured here for the prefetch, but the dedup check at line 132 (prefetched.has(rscUrl)) uses rscUrl alone as the key. If slots change between two prefetch calls for the same URL, the second prefetch is suppressed and the first (now stale-slot) prefetch will be rejected at consumption time.
Correctness is preserved since consumePrefetchResponse catches the mismatch, but the prefetch network work is wasted. If you want to address this later, the simplest fix is to incorporate the slot header into the dedup key.
| if ( | ||
| !overrideOrPageComponent && | ||
| defaultComponent && | ||
| options.isRscRequest && |
There was a problem hiding this comment.
The core slot persistence logic is clean. Worth noting the three cases:
isRscRequest && mountedSlotIds?.has(slotId)→ slot already mounted, skip key so browser retains prior contentisRscRequest && !mountedSlotIds?.has(slotId)→ first entry into layout, render default.tsx!isRscRequest(hard nav / initial load) → always render default.tsx
Implicit safety: stale mountedSlotIds referencing slots not in the current route's route.slots are harmless — the for loop only iterates actual route slots, so unknown IDs in the Set are never reached.
| }, [dispatchTreeState]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| setMountedSlotsHeader(getMountedSlotIdsHeader(stateRef.current.elements)); |
There was a problem hiding this comment.
Good: this reactive effect correctly tracks element changes independently from the stable dispatch/ref setup effect above. Splitting into two effects avoids unnecessary teardown/re-setup of dispatchBrowserRouterAction and browserRouterStateRef on every navigation.
|
Review posted successfully. Here's a summary of what I submitted: PR #811 Review: Fix parallel slot persistence and cache variants The review was posted as a batch review with 6 inline comments across the key changed files. The overall assessment is positive — the three-pronged approach (slot persistence, cache key fanning, prefetch/visited-response invalidation) is coherent and well-tested. Key findings:
None of the findings are blocking. |
Align generated entry's __normalizeMountedSlotsHeader regex with the typed module in app-elements.ts (both now use /\s+/). Clarify the HTML vs RSC cache asymmetry comment in stale regen path.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
The design is coherent — tracking mounted-slot state on the client, propagating it via X-Vinext-Mounted-Slots, and using it to vary cache keys and suppress default.tsx re-emission during soft nav. The three layers (slot persistence, cache key fanning, prefetch/visited-response invalidation) fit together well and the test coverage is thorough across all three.
I have one substantive concern (background regen re-rendering with the original request, which bakes user-specific slot context into cached content) and a few smaller observations.
Substantive issue
Background regen inherits the requesting client's slot context. The renderFreshPageForCache closure in the generated entry passes the original request object to buildPageElements (line 2186). This means the background regen respects the triggering client's X-Vinext-Mounted-Slots header — the regen will continue (skip) slots that the triggering client already had mounted. The resulting RSC payload is then cached under the slot-variant key, so other clients with the same mounted-slot combination get the right thing. But if a different client triggers a stale read for the same URL with a different slot combination, they will get a cache MISS and fall through to a fresh render — the stale regen from the first client doesn't help them.
This is probably fine as an MVP (slot-variant caching is inherently per-combination), but it does mean the regen comment at line 2167 ("Use an empty headers context for background regeneration — not the original user request") is misleading — it describes the headers context correctly, but the slot context still comes from the original user request. Worth a comment clarifying the intentional design.
Smaller observations
-
normalizeMountedSlotsHeaderinapp-elements.ts:29has a redundant.map(trim)after.split(/\s+/)—split(/\s+/)never produces tokens with leading/trailing whitespace; the only values that.trim()would affect are empty strings, which.filter(Boolean)already handles. Not a bug, just dead code. -
Prefetch dedup key doesn't incorporate slot state —
link.tsx:132deduplicates onrscUrlalone. If slots change between two prefetch calls for the same URL, the second is suppressed. The mismatch is caught at consumption time (consumePrefetchResponserejects when slot headers differ), so correctness is preserved, but the first prefetch is wasted. Minor — the comment at line 128 could note this limitation. -
The
getVisitedResponseslot-mismatch path deletes the cache entry (app-browser-entry.ts:230) rather than leaving it for a future consumer with matching slots. Since visited-response entries are URL-keyed (not slot-keyed), this is probably the simplest correct behavior — but it does mean a rapid back-forward between two slot contexts will always miss the visited cache.
Overall the PR looks good. None of the above are blocking.
| const normalized = Array.from( | ||
| new Set( | ||
| raw | ||
| .split(/\\s+/) |
There was a problem hiding this comment.
Previous reviews flagged the regex divergence between this generated code and the typed module. This .split(/\\s+/) in the template string produces .split(/\s+/) in the generated output, which correctly matches app-elements.ts:28. Just confirming this is already aligned — the earlier review suggestion to change from " " to /\\s+/ appears to have been addressed (or was never an issue in the committed code).
| new Set( | ||
| header | ||
| .split(/\s+/) | ||
| .map((slotId) => slotId.trim()) |
There was a problem hiding this comment.
Nit: .map((slotId) => slotId.trim()) is a no-op here. split(/\s+/) produces tokens that never have leading/trailing whitespace — the only edge case is empty strings from leading/trailing whitespace in the input, which .filter(Boolean) already handles.
| .map((slotId) => slotId.trim()) | |
| .split(/\s+/) | |
| .filter(Boolean), |
| undefined, | ||
| new URLSearchParams(), | ||
| isRscRequest, | ||
| request, |
There was a problem hiding this comment.
The original request is passed here, so the background regen renders with the triggering client's X-Vinext-Mounted-Slots header. The result is cached under the slot-variant key, which is correct for that specific variant — but the existing comment block (lines 2167-2170) describes avoiding user-specific context only in terms of headers/cookies, not slot state. Worth a one-liner noting that slot context is intentionally inherited:
| request, | |
| isRscRequest, | |
| request, // Slot context (X-Vinext-Mounted-Slots) is intentionally | |
| // inherited from the triggering request — the regen result | |
| // is cached under the slot-variant key. |
| const cached = visitedResponseCache.get(rscUrl); | ||
| if (!cached) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The slot-mismatch path unconditionally deletes the visited-response entry. Since the visited cache is keyed by URL (not URL+slots), this means rapid navigation between two slot contexts for the same URL will always miss. This is the simplest correct behavior given the single-key structure, but if visited-response cache misses become a perf concern for parallel-route-heavy apps, the cache could be changed to store multiple slot variants per URL.
Not blocking — just noting the tradeoff.
| .then(async (response) => { | ||
| if (response.ok) { | ||
| entry.snapshot = await snapshotRscResponse(response); | ||
| entry.snapshot = { |
There was a problem hiding this comment.
Good: explicitly overriding the snapshot's mountedSlotsHeader with the caller-provided value ensures prefetch compatibility is defined by the slot context at fetch time, not by whatever header the response happens to carry. The comment on lines 424-426 explains this well.
| // already mounted on the client. Absent key means the browser retains prior | ||
| // slot content rather than replacing it. When the slot is not yet mounted | ||
| // (first entry into this layout), include the key so default.tsx renders. | ||
| if ( |
There was a problem hiding this comment.
The slot persistence logic is clean and correct. The three cases are well-handled:
- RSC + mounted →
continue(browser retains prior content) - RSC + not mounted → render
default.tsx(first entry) - Non-RSC (hard nav) → always render
default.tsx
Stale mountedSlotIds referencing slots not in route.slots are harmless since the loop only iterates actual route slots.
| ), | ||
| options.isrSet( | ||
| options.isrRscKey(options.cleanPathname), | ||
| options.isrRscKey(options.cleanPathname, options.mountedSlotsHeader), |
There was a problem hiding this comment.
Minor note: the renderFreshPageForCache closure (in the generated entry) uses isRscRequest and request from the outer scope, so the regen produces a slot-variant-specific RSC payload. The cache key at line 167 correctly uses options.mountedSlotsHeader to store it under the right variant. This means different clients with different slot combinations get independent cache entries — cache fan-out is proportional to the number of distinct slot combinations observed, which could be worth monitoring in production.
|
Review posted successfully. Here's a summary of what I submitted: Overall assessment: The PR is well-designed and none of the findings are blocking. Key findings:
|
split(/\s+/) never produces tokens with leading/trailing whitespace, so .map(trim) was a no-op in both normalizeMountedSlotsHeader and its generated counterpart. Also add a comment clarifying that background regen intentionally inherits the triggering request's slot context.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
The design is sound and well-layered: client-side slot state tracking via X-Vinext-Mounted-Slots, server-side slot-aware rendering in buildAppPageElements, and cache key fanning by slot variant. The three previous bot reviews have covered the broad strokes well. I want to focus on a few issues that were either missed or under-discussed.
Key findings
-
Stale regen dedup key is pathname-only, but cache write key includes slot state. The
scheduleBackgroundRegenerationcallback receivesoptions.cleanPathnameas its dedup key (app-page-cache.ts:163). This means if two concurrent RSC requests for the same pathname but different slot combinations both get STALE, only one regen runs. The second slot variant's cache entry stays stale until a future request triggers its own regen cycle. Probably acceptable in practice — a composite dedup key would increase concurrent regen work — but worth a comment. -
consumePrefetchResponsepermanently destroys the cache entry on slot mismatch (navigation.ts:464-469). The entry is deleted before the mismatch check, so the prefetch work is fully wasted — not just unused for this navigation, but unavailable for a future navigation that does match. Prior reviews noted this is "caught safely" but didn't flag the permanent deletion. -
Prefetch dedup key doesn't include slot state (
link.tsx:132). If mounted slots change between two prefetches of the same URL (e.g., modal open/close), the second prefetch is suppressed and the first will be rejected at consumption time. For apps with frequent slot transitions, prefetches could be persistently wasted. A lightweight fix: include the slot header in the dedup key (rscUrl + '\0' + (mountedSlotsHeader ?? '')). Fine to defer — not a correctness issue. -
buildPageElementsparameter list is at 7 positional args. This is generated code so ergonomics matter less, but it's getting close to the threshold where an options bag would be clearer. Not blocking.
Test coverage assessment
Test coverage is strong across all three layers:
app-page-route-wiring.test.tscovers mounted/unmounted/hard-nav slot persistenceapp-page-cache.test.tscovers slot-keyed RSC reads, stale regen RSC-only writes, and HTML+canonical-RSC writesprefetch-cache.test.tscovers slot match/reject inconsumePrefetchResponseapp-browser-entry.test.tscoversgetMountedSlotIdsandgetMountedSlotIdsHeaderhelpers
The normalization functions between the generated entry and app-elements.ts are correctly aligned — both produce /\s+/ in the runtime output.
None of the findings are blocking. The PR is ready to merge once CI is green.
| @@ -153,27 +162,38 @@ export async function readAppPageCacheResponse( | |||
| // the stale payload and will fall through to a fresh render. | |||
| options.scheduleBackgroundRegeneration(options.cleanPathname, async () => { | |||
There was a problem hiding this comment.
The dedup key here is options.cleanPathname alone, but the cache write at line 167 uses options.isrRscKey(options.cleanPathname, options.mountedSlotsHeader) which includes slot state. This means two concurrent RSC requests for the same URL with different slot combinations will have only one regen run (the loser's variant stays stale until its next STALE read).
This is probably the right tradeoff (avoids concurrent regen storms for slot-heavy pages), but a brief comment would help:
| options.scheduleBackgroundRegeneration(options.cleanPathname, async () => { | |
| options.scheduleBackgroundRegeneration(options.cleanPathname, async () => { | |
| // Dedup key is pathname-only: if multiple slot variants are stale | |
| // concurrently, only one regen runs. Other variants refresh on | |
| // their next STALE read. |
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) { | ||
| if ((entry.snapshot.mountedSlotsHeader ?? null) !== mountedSlotsHeader) { |
There was a problem hiding this comment.
The entry was already deleted at line 464 (unconditionally, before any checks). So a slot mismatch permanently destroys the prefetch — it's not available for a future navigation that does match the original slot context.
If intentional (simplest correct behavior), a brief comment would clarify the design:
| if ((entry.snapshot.mountedSlotsHeader ?? null) !== mountedSlotsHeader) { | |
| if ((entry.snapshot.mountedSlotsHeader ?? null) !== mountedSlotsHeader) { | |
| // Entry was already removed — slot mismatch means the prefetch was | |
| // done with stale slot context and cannot be safely reused. | |
| return null; | |
| } |
| // already mounted on the client. Absent key means the browser retains prior | ||
| // slot content rather than replacing it. When the slot is not yet mounted | ||
| // (first entry into this layout), include the key so default.tsx renders. | ||
| if ( |
There was a problem hiding this comment.
The guard conditions are exactly right, covering three cases: RSC+mounted (skip), RSC+not-mounted (render default), non-RSC (always render default). There's an implicit fourth case worth noting: when a slot has both a page and a default, overrideOrPageComponent is truthy from the page match, so this branch is never entered — the page always renders regardless of mounted state. Correct Next.js behavior.
| stripBasePath(url.pathname, __basePath) === | ||
| stripBasePath(window.location.pathname, __basePath); | ||
| const cachedRoute = getVisitedResponse(rscUrl, navigationKind); | ||
| const elementsAtNavStart = getBrowserRouterState().elements; |
There was a problem hiding this comment.
Good: capturing elementsAtNavStart before any async work ensures the mounted-slot header reflects the state at navigation start, not a potentially-mutated state from a concurrent navigation that committed during the fetch. This is the right approach for consistency between the header sent to the server and the compatibility check against prefetched responses.
|
Review posted successfully on PR #811. Here's a summary of my findings: Overall assessment: The PR is well-designed and ready to merge. The three-pronged approach (slot persistence, cache key fanning, prefetch/visited-response invalidation) is coherent, and test coverage is thorough across all layers. Key findings (none blocking):
|
Add comments explaining that pathname-only regen dedup is intentional (avoids concurrent regen storms across slot variants) and that consumePrefetchResponse unconditionally removes the entry before the slot-mismatch check (simplest correct behavior, prefetch is wasted on mismatch).
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
The design is coherent and well-implemented. The three-layer approach works correctly together:
-
Server-side slot persistence (
app-page-route-wiring.tsx) — Distinguishes mounted vs first-entry slots using theX-Vinext-Mounted-Slotsheader. Thecontinueto omit already-mounted default-only slots from the RSC payload is the right mechanism for letting the browser retain prior content. -
Cache key fanning (
app-page-cache.ts,app-rsc-entry.ts) — RSC cache keys now incorporate slot state via FNV-1a hash, while HTML cache remains canonical (slot-independent). The asymmetry between RSC and HTML regen paths is correct and well-commented. -
Client-side cache invalidation (
app-browser-entry.ts,navigation.ts,link.tsx) — Both visited-response and prefetch caches check slot compatibility before serving entries. TheelementsAtNavStartcapture pattern in the browser entry is correct for consistency between the header sent to the server and the compatibility check.
Test coverage is solid across all three layers. The normalization functions between the generated entry and app-elements.ts are correctly aligned.
None of the findings below are blocking.
Additional note: storePrefetchResponse slot-awareness gap
storePrefetchResponse (navigation.ts:340) is a legacy backward-compat function that doesn't accept a mountedSlotsHeader parameter. Entries it creates will have mountedSlotsHeader set to whatever the response's X-Vinext-Mounted-Slots header happens to be (via snapshotRscResponse), rather than the caller-controlled value that prefetchRscResponse uses. This is fine today since it's only called from tests, but if anyone calls it from production code in the future, consumePrefetchResponse will reject entries on mismatch. Consider adding a JSDoc warning that it's slot-unaware, or adding the parameter for parity.
| options.mountedSlotIds?.has(slotId) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The slot persistence logic is clean and handles all four cases correctly:
- RSC + only
default.tsx+ mounted →continue(browser retains prior content) - RSC + only
default.tsx+ not mounted → render default (first entry into layout) - RSC + has
page→ always renders page (overrideOrPageComponentis truthy, branch skipped) - Non-RSC (hard nav) → always render
default.tsx
Stale mountedSlotIds from the client referencing slots not in route.slots are harmless since the loop only iterates actual route slots. Null mountedSlotIds (initial page load, no header) correctly falls through via optional chaining.
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) { | ||
| if ((entry.snapshot.mountedSlotsHeader ?? null) !== mountedSlotsHeader) { |
There was a problem hiding this comment.
The unconditional deletion (line 464) before this slot check means a mismatched prefetch is permanently destroyed. If the user opens a modal (changing mounted slots), then immediately closes it (reverting slots) and navigates to the prefetched URL, the entry is gone even though the original slot context would have matched.
This is the simplest correct behavior. getPrefetchedUrls().delete(rscUrl) at line 465 clears the dedup guard, so a subsequent <Link> hover will re-prefetch — the waste is limited to one round-trip per slot change per URL.
Not blocking — just documenting the tradeoff for future readers.
| // the stale payload and will fall through to a fresh render. | ||
| // Dedup key is pathname-only: if multiple slot variants are stale | ||
| // concurrently, only one regen runs. Other variants refresh on | ||
| // their next STALE read. |
There was a problem hiding this comment.
The pathname-only dedup key means if two concurrent RSC requests for the same URL with different slot combinations both get STALE, only one regen runs. The second variant stays stale until its next read. The comment here correctly documents this — confirming the tradeoff (fewer concurrent regens vs. delayed freshness for non-primary variants) is reasonable.
| }, [dispatchTreeState]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| setMountedSlotsHeader(getMountedSlotIdsHeader(stateRef.current.elements)); |
There was a problem hiding this comment.
Good: splitting into two effects is the right call. The first effect (deps: [dispatchTreeState]) handles stable module-level ref setup/teardown. This reactive effect only runs when treeState.elements changes, avoiding unnecessary teardown/re-setup of dispatchBrowserRouterAction and browserRouterStateRef on every navigation.
| stripBasePath(window.location.pathname, __basePath); | ||
| const cachedRoute = getVisitedResponse(rscUrl, navigationKind); | ||
| const elementsAtNavStart = getBrowserRouterState().elements; | ||
| const mountedSlotsHeader = getMountedSlotIdsHeader(elementsAtNavStart); |
There was a problem hiding this comment.
Capturing elementsAtNavStart before any async work is the right approach. This ensures the mounted-slot header reflects the state at navigation start, not a potentially-mutated state from a concurrent navigation that committed during the fetch. The same captured value is used for both the fetch header and the prefetch compatibility check — consistent.
| ]); | ||
| ]; | ||
|
|
||
| if (!options.isRscRequest) { |
There was a problem hiding this comment.
The asymmetry between RSC-triggered and HTML-triggered regens is correct: HTML cache is canonical (slot-independent), so an RSC-triggered regen should only refresh the requesting client's RSC slot variant. The comment explains this well. One thing worth noting: the renderFreshPageForCache closure in the generated entry inherits the triggering request's slot context (via the original request object), so the regen does produce a slot-variant-specific RSC payload — correctly cached under the matching variant key.
|
The review was posted successfully. Here's a summary of what I submitted: PR #811 Review: Fix parallel slot persistence and cache variants The review was posted as a batch review with 6 inline comments across the key changed files. The overall assessment is positive — the three-pronged approach (slot persistence, cache key fanning, prefetch/visited-response invalidation) is coherent, well-tested, and correctly implemented. Key inline comments posted:
Top-level note: Flagged that None of the findings are blocking. |
|
@james-elicx Not sure why it isn't approved here. Comments are all obsevations |
Add JSDoc note that storePrefetchResponse is slot-unaware: the snapshot's mountedSlotsHeader comes from the response headers rather than the caller, so consumePrefetchResponse may reject the entry on slot context mismatch.
|
/bigbonk re-review pr for issues? any blockers? |
There was a problem hiding this comment.
Review: Fix parallel slot persistence and cache variants
This PR correctly addresses three interrelated bugs in parallel slot handling introduced in #750:
-
Slot persistence on soft navigation — The core logic in
app-page-route-wiring.tsx:464-471correctly distinguishes between already-mounted slots (omit from RSC payload so browser retains prior content) and first-entry slots (includedefault.tsx). The four cases (RSC+mounted, RSC+not-mounted, RSC+has-page, hard-nav) are all handled correctly, and stalemountedSlotIdsfrom the client are harmless since the loop only iterates actual route slots. -
Cache key fanning by slot state — RSC cache keys now incorporate the mounted-slot header via FNV-1a hash, while HTML cache remains canonical (slot-independent). The
finalizeAppPageHtmlCacheResponsecorrectly passesnullfor the slot header. The asymmetry between RSC-triggered and HTML-triggered stale regens is correct and well-commented. -
Client-side cache invalidation — Both visited-response and prefetch caches check slot compatibility before serving entries. The
elementsAtNavStartcapture inapp-browser-entry.ts:753ensures consistency between the header sent to the server and the prefetch compatibility check.
The normalization functions between the generated entry (\\s+ in template literal producing /\s+/ at runtime) and the typed module (/\s+/ directly) are correctly aligned.
Test coverage is strong across all three layers. The two-effect split in BrowserRoot is the right approach.
Non-blocking observations (already noted by prior bot reviews, documenting agreement):
- Prefetch dedup key doesn't include slot state — waste is bounded and caught at consumption.
- Visited-response cache evicts on slot mismatch rather than storing variants per URL — simplest correct behavior.
- Background regen dedup key is pathname-only — acceptable tradeoff vs concurrent regen storms.
Clean, well-structured PR. LGTM.
|
Review posted successfully. Here's a summary: Approved PR #811 — The three-pronged fix for parallel slot persistence is correct and well-tested:
Test coverage is solid across all layers. The normalization functions are correctly aligned between generated and typed code. No blocking issues found. |
2 participants
Fixes bugs in slot components introduced in #750 that #752 caught
This tightens App Router slot behavior in three places:
default.tsxfallback behavior correctly on soft navigation by distinguishing already-mounted slots from first entry into a layoutVerification:
vp test run tests/app-page-cache.test.ts tests/prefetch-cache.test.ts tests/app-browser-entry.test.ts tests/app-page-route-wiring.test.tsvp test run tests/app-router.test.ts -t "generated code|ISR cache read fires|generated handler exports async function handler"vp test run tests/entry-templates.test.ts -uvp checkRisks: