fix(app-router): reuse force-stale full prefetches#2473
Conversation
|
/bigbonk review for issues |
commit: |
|
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
…routing-prefetch # Conflicts: # packages/vinext/src/shims/link.tsx
|
/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 — force-stale full prefetch reuse
I traced the visited-response cache changes across app-visited-response-cache.ts, app-browser-entry.ts, and link.tsx. Overall the prefetch-reuse plumbing is coherent and the cache-key normalization/_rsc fallback lookup is correct (separator \0 + null-context handling matches createAppPayloadCacheKey). Targeted tests pass and vp check is clean on the touched files. A few things worth a second look before merge:
1. mountedSlotsMatch change silently loosens navigation-time reuse (not just prefetch)
app-browser-entry.ts:702-703
mountedSlotsMatch:
match.entry.elements !== undefined || match.entry.mountedSlotsHeader === mountedSlotsHeader,This facts object feeds classifyVisitedResponseCacheCandidate, which runs on the navigation path (app-browser-entry.ts:1749), not only during prefetch. Previously an entry whose mountedSlotsHeader did not match the current navigation's mounted slots was evicted (mountedSlotsMismatch). Now, any entry that carries decoded elements is treated as a slot match regardless of the incoming mountedSlotsHeader.
Entries only get elements when they were captured with null mounted slots (app-browser-entry.ts:2169-2189, gated on getMountedSlotIdsHeader(...) === null). So the new behavior is: a null-slot cached tree can now be reused to satisfy a navigation that expects a non-null mountedSlotsHeader, where before it was evicted. That is a real behavioral change on the soft-nav path and it is not mentioned in the PR summary (which only describes prefetch reuse/expiry) and has no dedicated navigation-path test. Please confirm this is intended and matches Next.js segment-cache handoff, and add a navigation test asserting the reuse-vs-evict decision for the elements-present / slot-mismatch case so this doesn't silently regress.
2. hasVisitedResponseCacheEntryForPrefetch mutates despite the has* predicate name
app-visited-response-cache.ts:130-155
The function is named like a pure predicate but has three side effects: it deletes stale entries (fine), and on a hit it does delete + set to LRU-promote the entry. A has* name that mutates shared module state is a maintenance trap — a future caller will reasonably assume it is read-only. Consider renaming to something intent-revealing (e.g. claimVisitedResponseForPrefetch / reserveVisitedResponseForPrefetch) or splitting the freshness probe from the promotion. Compare with hasPrefetchCacheEntryForNavigation (navigation.ts:464), which only deletes stale entries and never re-promotes — the asymmetry is easy to trip over.
3. parseVisitedResponseCacheKey duplicates wire-format knowledge
app-visited-response-cache.ts:82-94 hard-codes the \0 separator and null-context semantics that are owned by app-elements-wire.ts (APP_INTERCEPTION_SEPARATOR, appendInterceptionContext, parsePathWithInterception). This is currently correct, but it's a second copy of the cache-key format that will drift if the wire encoding ever changes (e.g. a different separator or an escaped context). Prefer decoding through an AppElementsWire helper so there is a single source of truth, or at minimum add a comment cross-referencing appendInterceptionContext so the coupling is explicit.
Minor / confirm-only
- When
hasVisitedResponseCacheEntryForPrefetchreturnstrueinlink.tsx:532-537, the code returns beforeprefetched.add(cacheKey), so the intersection observer will re-run this check on every future intersection for the same anchor. That's harmless (it's a Map lookup + freshness check) but means the visited-cache lookup is not memoized per anchor — fine to leave, just noting it. - The
prefetchfreshness branch (isVisitedResponseCacheEntryFresh, lines 61-63) deliberately uses staticPREFETCH_CACHE_TTLfromcreatedAtand ignores the entry's ownexpiresAt/dynamic stale. That is the intended force-stale semantic and the test covers the boundary; just calling it out since it diverges from every other navigation kind which keys offexpiresAt.
Nothing here is a hard blocker, but item #1 is the one I'd want an explicit answer on before merge given it changes soft-navigation cache reuse beyond the stated scope of the PR.
|
Review posted to PR #2473. Summary of what I found: Behavioral concern (main item to confirm before merge):
Maintainability: Confirm-only notes: the per-anchor lookup re-runs on every intersection (harmless), and the The correctness plumbing (cache-key normalization, |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review — force-stale full prefetch reuse
The three commits on this PR resolve the concerns from my previous review. Confirming the current state and noting a couple of smaller items.
Previously-flagged items — now resolved
-
mountedSlotsMatchloosening is now scoped to prefetch only. ✅ The navigation path (readVisitedResponseCacheCandidate,app-browser-entry.ts:703) now callsisVisitedResponseCacheEntryCompatibleForNavigation, which keeps strictmountedSlotsHeader === mountedSlotsHeaderequality. The loosenedelements !== undefined || …check lives only inisVisitedResponseCacheEntryCompatibleForPrefetch, consumed exclusively by the prefetch claim path. The new unit test (app-visited-response-cache.test.ts, "keeps navigation mounted-slot matching strict for entries carrying decoded elements") asserts exactly this split (…ForNavigation(entry, "slot:modal") === false,…ForPrefetch(...) === true). This was the one item I wanted an explicit answer on, and it's now correct. -
has*predicate no longer mutates. ✅ Renamed toclaimVisitedResponseCacheEntryForPrefetch(app-visited-response-cache.ts:131), which is intent-revealing about the delete-stale + LRU-promote side effects. -
Wire-format duplication removed. ✅
findVisitedResponseCacheEntrydecodes keys throughAppElementsWire.decodeCacheKey(newparseAppPayloadCacheKey,app-elements-wire.ts:337), so the\0separator / null-context semantics have a single source of truth. The_rscnormalization reusesstripRscCacheBustingSearchParam, so it stays consistent with the encoder.
Correctness — looks sound
_rscnormalized fallback is safe. The O(n) scan is bounded (MAX_VISITED_RESPONSE_CACHE_SIZE = 50) and only runs on an exact-key miss. It re-checksinterceptionContext, and mounted-slot compatibility is enforced separately by each caller. Visited entries are only stored from navigation-mode responses (storeVisitedResponseSnapshot, never shell mode), so there's no cross-render-mode collision risk from dropping the_rschash.deleteVisitedResponsefallback parity. It now deletes via the samefindVisitedResponseCacheEntryused at read time, so the no-commit eviction atapp-browser-entry.ts:1868targets the same entry the candidate was read from. Consistent.- Expiry mechanism. "Expire once the prefetch settles" is implemented as static
PREFETCH_CACHE_TTL-from-createdAtfreshness inisVisitedResponseCacheEntryFresh(prefetchbranch); once past TTL the claim deletes the entry and returnsfalse, so a fresh prefetch is issued. Covered by "expires visited response cache reuse … after the static stale time".
Minor / confirm-only
resolveFullAppRoutePrefetch(href)default change. When the route trie is unavailable or unmatched,prefetchShellFirstnow defaults tofalse(was unconditionallytrue). For the intended client path this is correct (only shell-first whencanPrefetchLoadingShell), and it matches the reworked tests (/blog/:slugshell-first vs/products/:iddirect). Just flagging that the fallback for an unmatched route changed from shell-first to direct-full — intended, but worth a mental note since it also alters behavior for routes not present in__VINEXT_LINK_PREFETCH_ROUTES__.- Prefetch claim isn't memoized on the anchor. On a claim hit,
link.tsx:539-544returns beforeprefetched.add(cacheKey), so the intersection observer re-runs the (cheap, bounded) lookup on every future intersection for that anchor. Harmless, same as noted before.
Validation
tsc --noEmiton the vinext package: clean.tests/app-visited-response-cache.test.ts: 10/10 pass.
No blockers. The scope concern from the prior review is resolved and covered by a dedicated navigation-vs-prefetch test. LGTM.
|
Review posted to PR #2473. Summary of what I found: The three commits resolved all concerns from the earlier review:
Correctness verified:
Minor confirm-only notes: the Validation: typecheck clean, 10/10 cache unit tests pass. No blockers — LGTM. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — force-stale full prefetch reuse
I re-traced the visited-response-cache changes independently (app-visited-response-cache.ts, app-browser-entry.ts, link.tsx, app-elements-wire.ts) and confirm the three concerns from the earlier reviews are genuinely resolved:
- Navigation slot matching stays strict.
readVisitedResponseCacheCandidate(app-browser-entry.ts:703) usesisVisitedResponseCacheEntryCompatibleForNavigation(exactmountedSlotsHeaderequality); the loosenedelements !== undefined || …check lives only inisVisitedResponseCacheEntryCompatibleForPrefetch, consumed by the prefetch claim path. Covered by a dedicated navigation-vs-prefetch unit test. claimVisitedResponseCacheEntryForPrefetchis now correctly named for its delete-stale + LRU-promote side effects (app-visited-response-cache.ts:131).- Wire-format single source of truth.
findVisitedResponseCacheEntrydecodes keys viaAppElementsWire.decodeCacheKey/parseAppPayloadCacheKeyinstead of re-hard-coding the\0separator.
Correctness — confirmed sound
_rscnormalized fallback is safe. The scan is bounded (MAX_VISITED_RESPONSE_CACHE_SIZE = 50), only runs on an exact-key miss, re-checksinterceptionContext, and mounted-slot compatibility is enforced per-caller. The stripped_rschash covers headers that don't alter navigation-mode payload content, and visited entries are only stored from navigation-mode responses, so there's no cross-render-mode collision.deleteVisitedResponse(app-browser-entry.ts:1868) re-runsfindVisitedResponseCacheEntrywith the same key inputs, so no-commit eviction targets the same entry the candidate was read from — consistent even across therenderNavigationPayloadawait.- Prefetch freshness intentionally keys off static
PREFETCH_CACHE_TTL-from-createdAt(the "expire once the prefetch settles" force-stale semantic), diverging from theexpiresAt-based navigation kinds. Deliberate and test-covered.
Targeted tests pass (tests/app-visited-response-cache.test.ts + tests/app-router-rsc-plugin.test.ts: 18/18) and vp check is clean on the touched files.
Scope — the one thing I'd want confirmed before merge
The PR summary describes only force-stale prefetch reuse (commits 70167db, 88bd954, cc86cb8, all confined to the visited-response cache). But the head also carries 04c2073 "fix(app-router): keep dev RSC client references valid", which is an orthogonal fix:
- new
createRscReferenceValidationAliasPlugin+ dev client-reference warmup inrsc-client-reference-loaders.ts - splits
default-global-error.tsxinto a"use client"wrapper + a plaindefault-global-error-render.tsx(SSR entry now imports the non-client render directly) - adds
vinext/shims/default-global-errorto the optimizeDeps exclude list
This is 313/208 of the diff and shares no code path with the visited-response cache. It has its own unit test and typechecks clean, but:
- It's not mentioned in the PR summary or the "Next.js parity" section, and its commit has no body explaining the motivation or the dev failure it fixes.
- Because auto-changesets are generated from commit subjects, this
fix(app-router):commit will produce a changelog entry under this PR that readers won't be able to trace back to the stated scope.
Not a correctness blocker — but please either (a) split 04c2073 into its own PR, or (b) update the PR summary to describe the dev RSC client-reference fix (what dev scenario broke, why the global-error shim split is the fix) so the change is reviewable and the changelog entry is honest. Given branch protection and the stacked-PR guidance in AGENTS.md, splitting is preferable if the two fixes are truly independent.
Minor / confirm-only
resolveFullAppRoutePrefetch(href)default change. For routes not present in__VINEXT_LINK_PREFETCH_ROUTES__(or when the trie is unavailable),prefetchShellFirstnow defaults tofalse(was unconditionallytrue). Intended per the reworkedlink-navigationtests, just flagging that it also alters behavior for unmatched routes.warmupRscDevClientReferencesswallows all errors (.catch(() => {})) and the transform proceeds regardless. Best-effort warmup is reasonable, but a silent failure here would leave the original dev symptom unfixed with no signal. Consider adebug-level log on failure.- Prefetch claim isn't memoized on the anchor — on a claim hit
link.tsxreturns beforeprefetched.add(cacheKey), so the observer re-runs the (cheap, bounded) lookup on every future intersection. Harmless.
Core force-stale logic LGTM. The scope/commit-hygiene item is the only thing I'd resolve before merge.
|
Review posted to PR #2473. Summary of what I found: Force-stale prefetch reuse (the stated PR scope) — correct. The three concerns from the earlier review rounds are genuinely resolved, and I independently re-verified:
Main item to confirm before merge — scope creep. The head carries a fourth, orthogonal commit Minor/confirm-only: the No correctness blocker — the only thing I'd resolve before merge is the commit-hygiene/scope item. |
…fetch' into codex/fix-segment-cache-force-stale-stacked-28478866791 # Conflicts: # packages/vinext/src/index.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — force-stale full prefetch reuse (independent re-trace)
I re-traced the visited-response-cache changes end to end (app-visited-response-cache.ts, app-elements-wire.ts, app-browser-entry.ts, link.tsx) and independently confirm the three concerns from the earlier rounds are genuinely resolved, plus verified the correctness of the new plumbing.
Resolved / verified sound
- Navigation slot matching stays strict.
readVisitedResponseCacheCandidate(app-browser-entry.ts:693-706) feedsmountedSlotsMatchviaisVisitedResponseCacheEntryCompatibleForNavigation(exactmountedSlotsHeader === mountedSlotsHeader). The loosenedentry.elements !== undefined || …predicate lives only inisVisitedResponseCacheEntryCompatibleForPrefetch(app-visited-response-cache.ts:124-129), consumed exclusively byclaimVisitedResponseCacheEntryForPrefetch. The dedicated unit test asserts the split (…ForNavigation(entry,"slot:modal") === false,…ForPrefetch === true). This was the one scope-creep item that warranted an explicit answer, and it's correct. claim*is honestly named for its mutations.claimVisitedResponseCacheEntryForPrefetch(app-visited-response-cache.ts:131-155) deletes-stale + LRU-promotes, and the name now signals the side effects.- Wire-format single source of truth.
findVisitedResponseCacheEntrydecodes keys viaAppElementsWire.decodeCacheKey→parseAppPayloadCacheKey(app-elements-wire.ts:337-347), reusingparsePathWithInterception. No more hard-coded\0in the cache module.
Correctness — confirmed
_rscnormalized fallback is safe.normalizeVisitedResponseCacheLookupUrlstrips only the_rsccache-busting param (stripRscCacheBustingSearchParam), which hashes request headers (prefetch/segment/state-tree/next-url/interception/mounted-slots/render-mode —app-rsc-cache-busting.ts:180-190). The fallback scan re-checksinterceptionContextper entry, mounted-slot compatibility is enforced by each caller, and visited entries are stored only from navigation-mode responses (storeVisitedResponseSnapshot), so dropping the_rschash can't collide across render modes or the other variance vectors. The scan is bounded byMAX_VISITED_RESPONSE_CACHE_SIZE = 50and runs only after an exact-key miss.- No-commit eviction targets the read entry.
deleteVisitedResponse(app-browser-entry.ts:736-738) re-runsfindVisitedResponseCacheEntry, so theno-commitpath at:1868deletes the same entry the candidate was read from, even across the render await. - Force-stale expiry semantics. Prefetch freshness keys off static
PREFETCH_CACHE_TTL-from-createdAt(isVisitedResponseCacheEntryFresh,prefetchbranch) rather thanexpiresAt; once past TTL the claim deletes and returnsfalse, re-issuing a fresh prefetch. Test-covered at the boundary (now + PREFETCH_CACHE_TTLtrue /+1false).
Targeted tests pass locally: tests/app-visited-response-cache.test.ts 10/10 and tests/app-router-rsc-plugin.test.ts 8/8.
The one thing still worth resolving before merge — scope
The PR summary describes only force-stale prefetch reuse, but the head still carries the orthogonal 04c2073 "fix(app-router): keep dev RSC client references valid" (createRscReferenceValidationAliasPlugin was added in #2440's base, and this branch adds warmupRscDevClientReferences + the default-global-error.tsx → default-global-error-render.tsx "use client" split + the optimizeDeps exclude). This shares no code path with the visited-response cache, has no commit body, and isn't in the summary or parity section. Because auto-changesets derive from commit subjects, it will emit a fix(app-router): changelog entry under this PR that readers can't trace to the stated scope. Per AGENTS.md stacked-PR guidance, either split it out or extend the summary to describe the dev failure it fixes.
Minor / confirm-only (non-blocking)
warmupRscDevClientReferencesswallows all errors (.catch(() => {}),rsc-client-reference-loaders.ts:86) and the transform proceeds regardless. Reasonable as best-effort, but a silent failure would leave the original dev symptom unfixed with no signal — consider adebug-level log on failure.resolveFullAppRoutePrefetch(href)default flipped shell-first → direct-full for routes absent from__VINEXT_LINK_PREFETCH_ROUTES__/ when the trie is unavailable (prefetchShellFirstnowfalseunlesscanPrefetchLoadingShell === true). Intended per the reworkedlink-navigationtests, just noting it also changes unmatched-route behavior.- Prefetch claim isn't memoized on the anchor — on a claim hit
link.tsx:552-557returns beforeprefetched.add(cacheKey), so the observer re-runs the (cheap, bounded) lookup on each future intersection. Harmless.
Core force-stale logic LGTM. No correctness blocker; the commit-hygiene/scope item is the only thing I'd resolve before merge.
|
Review posted to PR #2473. Summary of my independent findings: Force-stale prefetch reuse (the stated PR scope) — correct. I re-traced the full flow and independently verified all three previously-flagged concerns are genuinely resolved:
Correctness confirmed: the Main item to confirm before merge — scope creep. The head still carries the orthogonal Minor/confirm-only: silent error-swallow in No correctness blocker — the commit-hygiene/scope item is the only thing worth resolving before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — force-stale full prefetch reuse (independent re-trace)
I re-traced the visited-response-cache changes end to end and confirm the three concerns from the prior rounds are genuinely resolved (strict navigation slot matching via isVisitedResponseCacheEntryCompatibleForNavigation, honestly-named claimVisitedResponseCacheEntryForPrefetch, and wire-format decode routed through AppElementsWire.decodeCacheKey). vp check is clean and 77/77 targeted tests pass locally.
I did find one new behavioral issue that the earlier reviews did not surface, plus confirm the scope item.
1. Normalized _rsc fallback can cross-evict a valid mounted-slot variant on navigation
app-visited-response-cache.ts:82-105 — findVisitedResponseCacheEntry falls back to a normalized scan that strips the _rsc cache-busting param. But _rsc hashes VINEXT_MOUNTED_SLOTS_HEADER (app-rsc-cache-busting.ts:34-44), so two visited entries for the same path + interception context that differ only by mounted slots normalize to the same URL. The fallback returns the first one in Map iteration order, and mounted-slot compatibility is only checked by the caller after the lookup.
On the navigation path this is not harmless:
readVisitedResponseCacheCandidate(app-browser-entry.ts:694-709) returns that fallback entry withmountedSlotsMatch: false(strict equality — correct).applyVisitedResponseCacheCandidateDecision(app-browser-entry.ts:727-733) then evictscandidate.cacheKeyon a slot mismatch.
So a navigation for mounted-slot variant B (same path/context) can now delete the cached entry for variant A, whenever B's own exact key misses. Before this PR, navigation used an exact-key visitedResponseCache.get(...) keyed on the _rsc-inclusive URL, so variant B simply missed and left variant A's entry intact for a later matching navigation to reuse. The normalized fallback turns that clean miss into a destructive cross-variant eviction.
The prefetch claim path is fine — claimVisitedResponseCacheEntryForPrefetch returns false without deleting on a compatibility miss (app-visited-response-cache.ts:149-151), so only the navigation eviction path regresses.
This is an edge case (requires ≥2 coexisting mounted-slot variants for one path/context — parallel routes / intercepting modals), and there's no test covering multiple normalized-collision entries. Recommend one of:
- have the normalized fallback prefer a mounted-slot-compatible entry (pass the wanted
mountedSlotsHeaderintofindVisitedResponseCacheEntryand skip mismatches during the scan), or - scope the eviction so a normalized-fallback slot-mismatch does not delete the entry it happened to land on.
Either way, please add a navigation test with two same-path entries differing only by mounted slots, asserting that reusing/evicting one doesn't clobber the other.
2. default-global-error client/render split — parity confirmed
default-global-error-render.tsx (non-"use client") still carries onClick={handleBackClick} and window.history usage (lines 155-162, 190). Rendered through the SSR entry (app-ssr-entry.ts:55 now imports default-global-error-render), the onClick is dropped in plain SSR — same non-interactive fallback shell you'd get SSR-rendering the client component's HTML, so no parity regression. The "use client" wrapper (default-global-error.tsx) keeps the client boundary for the RSC/hydration path. This looks correct.
3. Scope — the orthogonal dev RSC fix is now documented in the summary
The head still carries 04c2073 "fix(app-router): keep dev RSC client references valid" (dev warmup + default-global-error split + optimizeDeps exclude), which shares no code path with the visited-response cache. The PR summary now describes it, which addresses the changelog-traceability concern from earlier rounds. Still worth confirming whether splitting per AGENTS.md stacked-PR guidance is preferable, but it's no longer an undocumented change.
Minor / confirm-only
warmupRscDevClientReferencesswallows all errors (.catch(() => {}),rsc-client-reference-loaders.ts) and proceeds. Reasonable as best-effort, but a silent failure would leave the original dev symptom unfixed with no signal — consider adebug-level log.resolveFullAppRoutePrefetch(href)default flipped shell-first → direct-full for routes absent from__VINEXT_LINK_PREFETCH_ROUTES__. Intended per the reworkedlink-navigationtests, just noting the unmatched-route behavior change.- Prefetch claim isn't memoized on the anchor — the (cheap, bounded) lookup re-runs on each intersection. Harmless.
Core force-stale prefetch logic LGTM. Item #1 is the one I'd want addressed (or explicitly accepted as an acceptable edge case) before merge, since it changes navigation-time cache eviction beyond the stated prefetch scope.
|
Review posted to PR #2473. Summary of my independent findings: Verified resolved (from prior rounds): strict navigation slot matching, honestly-named New issue not surfaced in earlier reviews (item #1): The normalized Confirmed sound: the Scope: the orthogonal Minor/confirm-only: silent error-swallow in Core force-stale logic LGTM; item #1 is the one worth addressing or explicitly accepting before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — force-stale full prefetch reuse (item #1 follow-up)
I re-traced the visited-response-cache changes end to end and independently verified the correctness item #1 from my previous review round — the normalized _rsc cross-eviction — is now genuinely fixed by the new commit 9c11137 "fix(app-router): scope visited response fallback by slots".
Item #1 (cross-variant eviction) — resolved ✅
findVisitedResponseCacheEntry now takes a VisitedResponseCacheLookupOptions bag (mountedSlotsHeader + isEntryCompatible) and applies the compatibility predicate inside the normalized fallback scan (app-visited-response-cache.ts:110-116). Both navigation callers thread isVisitedResponseCacheEntryCompatibleForNavigation through:
readVisitedResponseCacheCandidate(app-browser-entry.ts:682-685)deleteVisitedResponse(app-browser-entry.ts:739-748), which backs the no-commit eviction at:1878
So the destructive scenario I flagged is closed: a navigation for mounted-slot variant B whose exact key misses can no longer land the normalized fallback on variant A and evict it — variant A is now skipped as slot-incompatible, the fallback returns null, and A's entry is left intact for a later matching navigation. The read path and the no-commit delete path use the identical key inputs (rscUrl, requestInterceptionContext, mountedSlotsHeader), so they target the same entry across the render await.
The new regression test (tests/app-visited-response-cache.test.ts, "keeps normalized _rsc lookup scoped to compatible mounted-slot variants") covers exactly this: two same-path entries (slot:modal:/ and slot:drawer:/) that normalize to the same URL, asserting a lookup for slot:drawer:/ returns the drawer entry and a delete for a third slot:sidebar:/ variant returns false and clobbers neither existing entry. 11/11 pass; vp check clean on both touched files.
Prefetch claim path — still safe
claimVisitedResponseCacheEntryForPrefetch now also runs the fallback scan through isVisitedResponseCacheEntryCompatibleForPrefetch, so the scan prefers a slot-compatible entry rather than the first Map-iteration hit. It still returns false without deleting on an incompatibility miss (:169-171), so it never destructively evicts a foreign slot variant. The elements-present loosening only permits reuse of a null-slot tree for prefetch, never a cross-slot delete.
Minor / confirm-only (unchanged, non-blocking)
- The redundant
isVisitedResponseCacheEntryCompatibleForPrefetchre-check atapp-visited-response-cache.ts:169is now only meaningful for the exact-key hit (the scan filter already covers fallback matches). Harmless. warmupRscDevClientReferencesstill swallows all errors (.catch(() => {})); consider adebug-level log so a silent warmup failure doesn't quietly re-surface the original dev symptom.- Prefetch claim isn't memoized on the anchor — the cheap, bounded lookup re-runs on each intersection. Harmless.
All previously-flagged concerns (strict navigation slot matching, honestly-named claim*, wire-format single source of truth via AppElementsWire.decodeCacheKey, and now the normalized-fallback cross-eviction) are resolved and test-covered. No correctness blocker remains. LGTM.
|
Review posted to PR #2473. SummaryThe head advanced to commit Verified resolved:
No correctness blocker remains. The only leftover notes are non-blocking (silent warmup error-swallow, a now-redundant re-check line, non-memoized prefetch claim). LGTM. |
|
Correction to the previous truncated mapping comment. Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the App Router force-stale navigation/prefetch row from Exact row:
Since this PR is already James-owned, I am skipping duplicate implementation work for this row. |
…-7139 # Conflicts: # packages/vinext/src/plugins/rsc-reference-validation-normalizer.ts # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
1 participant
Summary
_rscvisited-response fallback lookups to compatible mounted-slot variants so one slot variant cannot evict anotherNext.js parity
Targets the App Router Segment Cache behavior covered by:
test/e2e/app-dir/segment-cache/force-stale/force-stale.test.tsThis is stacked on #2440 because the force-stale row depends on visible App Router prefetches starting before browser idle.
Validation
vp check packages/vinext/src/server/app-visited-response-cache.ts packages/vinext/src/server/app-browser-entry.ts tests/app-visited-response-cache.test.tsvp test run tests/app-visited-response-cache.test.ts— 11 passedvp test run tests/app-router-rsc-plugin.test.ts tests/app-visited-response-cache.test.ts tests/link-navigation.test.ts tests/app-ssr-light-runtime.test.ts— 84 passed3099— HTTP 200 after Vite startupREPO="/Users/jamesanderson/.codex/worktrees/segment-cache-force-stale-28478866791/vinext" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/force-stale/force-stale.test.ts— 2/2 passedcreate-next-app (ubuntu-latest)/create-next-app (windows-latest)— passed ond3bd8b44d301c662b323a7ca2cf38742636f853a; rerunning on latest head