fix(app-router): support runtime instant prefetch shells - #2850
fix(app-router): support runtime instant prefetch shells#2850james-elicx wants to merge 12 commits into
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: runtime instant prefetch shells
Reviewed the 28 changed files, focusing on the new shell state machine, the route-graph runtime-instant detection, prefetch-policy/link/navigation wiring, the RSC finalizer, and the dev HMR change. Ran the targeted suites (app-page-render, app-route-graph, build-report, app-optimistic-routing, app-rsc-response-finalizer, link, link-navigation, prefetch-cache) plus vp check on the core changed files — all green. The core mechanism (partial-shell abort after cache tasks settle + connection() suspension, never caching aborted shells, learning exact-href optimistic templates with source-scoped invalidation) is sound. A few observations below; none are blocking, but the first two are worth a decision before merge.
1. runtimeInstantConfigDependencies is a module-scoped set that is never cleared (routing/app-route-graph.ts:255)
Entries are only ever added (:313), never removed. Across dev route-graph rebuilds this set grows monotonically and retains stale paths. If a local unstable_instant re-export chain is later removed or repointed, the old dependency path lingers, so isRuntimeInstantConfigDependency (used by the HMR change handler in index.ts:4652) keeps triggering route-graph invalidation for a file that is no longer a config dependency. Over-invalidation is safe for correctness, but it's an unbounded-growth leak on a long-lived dev server. Consider clearing/repopulating the set per buildAppRouteGraph call (mirroring the per-scan findFileProbeCache clone pattern already used a few lines below).
2. Unresolved external re-exports are conservatively classified as runtime-instant (routing/app-route-graph.ts:306-312)
Any route that does export { unstable_instant } from "@myorg/config" (Vite alias / package export the sync scanner can't resolve) is treated as hasRuntimeInstant, forcing every prefetch for that route into a non-reusable partial shell — even when the actual value is { prefetch: "static" }. This is a documented, deliberate safety default, but it can be a broad prefetch-cacheability regression for apps that centralize route config in a shared package. Worth confirming this matches the intended tradeoff, and ideally noting it in user-facing docs since the failure mode (prefetches silently stop being reused) is hard to diagnose.
3. HMR change handler invalidates on any script under app/ (index.ts:4648-4660)
fileMatcher.isPageFile(filePath) || SCRIPT_IMPORT_RE.test(filePath) matches every .ts/.tsx/.js/... file under app/, so editing an arbitrary helper or leaf component now re-runs invalidateAppRoutingModules() + invalidateAppBrowserEntry() (full route re-scan) on every save. Given AGENTS.md's emphasis on keeping the dev graph lightweight, this is a noticeable dev-loop cost. It's understandable as the safety net for re-export chains not yet recorded in runtimeInstantConfigDependencies, but it's a wide net. If perf matters, consider gating on "file is imported by a known route module" rather than "any script under app/".
4. Property-value alias not resolved in hasNamedExportObjectStringProperty (build/report.ts)
resolveLocalConstExpression follows the top-level export const alias, but the object property value is only unwrapStaticExpression'd, not alias-resolved. So const RUNTIME = "runtime"; export const unstable_instant = { prefetch: RUNTIME } won't be detected as runtime-instant. This is a conservative miss (falls back to non-instant), not a correctness bug, but it's a small parity gap if you want to advertise full static analysis of the config.
5. Pre-existing gap, not introduced here: app-only add/unlink doesn't refresh the browser entry
invalidateHybridClientEntries() early-returns when !hasPagesDir (index.ts:4417), so in an app-only project adding/removing a runtime-instant route on the add/unlink watcher paths never calls invalidateAppBrowserEntry() — the new change handler only covers change events. The prefetch route manifest (now carrying hasRuntimeInstant) can go stale until the next full reload. This predates the PR, but the PR makes the staleness more meaningful. A follow-up to invalidate the app browser entry on add/unlink in app-only mode would close it.
Nice work on the abort scheduling (two-macrotask defer + cache-task refcount + cancelPendingAbort) and on keeping instant shells out of both navigation reuse (consumeMatchedPrefetchResponse bails on cacheForNavigation === false) and the CDN cache (lockPartialShellCacheHeaders). The exact-href template learning with entry-identity-checked invalidation (attachPrefetchInvalidationCallback returning false when the entry was superseded) is the right call.
|
Review posted to PR #2850. Summary of what I did and found: Review completeI reviewed all 28 changed files, focusing on the shell state machine, route-graph runtime-instant detection, prefetch/link/navigation wiring, the RSC finalizer, and the dev HMR change. Ran the targeted test suites and Findings (none blocking, first two worth a pre-merge decision):
I also confirmed the good parts: instant shells are correctly kept out of navigation reuse ( |
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 |
Review: runtime instant prefetch shells (re-review)Re-reviewed the 40 changed files with focus on the six commits stacked on this branch, cross-checking against the earlier
Correctness spot-checks (all pass)
Verification
No blocking issues. Nice work closing the review loop — the abort scheduling (two-macrotask defer + cache-task refcount) and the entry-identity-checked optimistic-template invalidation ( |
|
Review posted to PR #2850. SummaryI re-reviewed all 40 changed files across the 6 commits, cross-checking against the earlier review cycle. All five prior-review findings have been resolved, and I found no new blocking issues. Prior findings, now fixed:
Correctness spot-checks that passed:
Verification: |
|
Mapping note for the cache-components backlog from Actions run |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: runtime instant prefetch shells (independent pass)
Reviewed all 40 changed files with focus on the new shell state machine (instant-prefetch-shell.ts), route-graph runtime-instant detection (app-route-graph.ts), the static-analysis in build/report.ts, the three cache-task refcount wrappers (fetch-cache.ts, cache.ts, cache-runtime.ts), thenable-params suspension, the RSC finalizer/render-mode plumbing, optimistic-template learning, and the dev HMR change. Ran the targeted suites and vp check on the core files — all green.
The core design is correct and I found no blocking issues. The prior review cycle's findings (unbounded config-dep set, broad HMR invalidation, property-value alias resolution, app-only add/unlink browser-entry staleness) are all resolved in the tree I reviewed.
Correctness spot-checks (all pass)
- ALS key parity.
instant-prefetch-shell.tsreads the use-cache/unstable-cache stores viaglobalThis[Symbol.for("vinext.cacheRuntime.contextAls")]/Symbol.for("vinext.unstableCache.als")— exactly the keyscache-runtime.ts:156andcache.ts:474register throughgetOrCreateAls. Thevariant !== "private"gate matchesCacheContext.variant(cache-runtime.ts:135). - Abort scheduling + cache-task refcount. The two-macrotask defer in
scheduleAfterTaskhas no TDZ issue (thefirstTimercallback is a macrotask, solet secondTimerhas executed).cancelPendingAbortat the top oftrackInstantPrefetchShellCacheTaskcorrectly prevents an abort from firing between a settled task and a newly-started one, and the re-check inside the deferred callback guardspendingCacheTasks === 0. The nestedvoid trackInstantPrefetchShellCacheTask(() => cacheWrite, "fetch")infetch-cache.tsincrements the refcount before the outerfnresolves, keeping the shell open until the cold entry is durable. dynamicAbortControllernever aborted — intentional. Theconnection()/params/private-cache hanging promises usedynamicAbortController.signalpurely as a hole marker; the render is cut byreactAbortController.abort(), matching Next.js's separate dynamic-tracking vs render-abort signals.- Instant shells excluded from reuse.
promotePrefetchEntriesForNavigationskipsinstantShell/optimisticRouteShell/partialSuspenseShell(link.tsx:847);lockPartialShellCacheHeadersforcesno-storeand strips CDN-Cache-Control/Cloudflare-CDN-Cache-Control/Cache-Tag (app-rsc-response-finalizer.ts);wasInstantPrefetchShellAbortedonly reports true when a dynamic boundary was actually hit, so fully-static instant routes still produce complete, cacheable payloads (and get promoted to a reusablenavigationentry via theVINEXT_RSC_PARTIAL_SHELL_HEADERcheck inprefetchRscResponse). - Source-scoped template invalidation.
attachPrefetchInvalidationCallbacknow takes anexpectedEntryand returnsfalseon identity mismatch/supersession;discardLearningOnlyPrefetchCacheEntryhandles the newinstant-shellkind, so a learned optimistic template is invalidated with its source prefetch. pageProps = {}fallback. Both consumers guard onEffectivePageComponent, so the empty-object branch is never read — no dropped-params regression.- Validation parity.
toLinkPrefetchRoutesthrows onunstable_instantin a Client Component module and on use withoutcacheComponents, matching Next.js validation; both are gated on the metadata flags so non-instant apps are unaffected.
Non-blocking observations
-
Unresolved external re-export → conservative
runtime(app-route-graph.tsrouteModuleExportInstantPrefetchMode). A route that doesexport { unstable_instant } from "@myorg/config"(Vite alias / package export the sync scanner can't resolve) is always classified runtime-instant, forcing non-reusable partial shells even when the real value is{ prefetch: "static" }. This is a documented, deliberate safety default carried over from the prior cycle. It remains worth a user-facing docs note, because the failure mode — prefetches silently stop being reused for that route — is hard to diagnose. Not blocking. -
Dead-in-production helper exports.
hasNamedExportObjectStringPropertyandfindNamedExternalReexportinbuild/report.tsare exported but only referenced bytests/build-report.test.ts; the route graph usesanalyzeNamedExportObjectStringPropertyexclusively. Harmless, but they read as public API. Consider marking them@internalor dropping them if the test coverage can move ontoanalyzeNamedExportObjectStringProperty. -
prefetched.has(cacheKey)freshness gate is now uniform (link.tsx:543,navigation.ts:2815). The old code let a navigation-reusable entry fall through and re-prefetch even when already prefetched; the newhasFreshPrefetchCacheEntryguard skips any fresh entry regardless ofcacheForNavigation. This is a deliberate de-dup improvement and the link/prefetch suites cover it, but it is a behavior change in the reusable path worth a second look if any caller relied on the re-prefetch-while-fresh semantics.
Verification
vp checkclean oninstant-prefetch-shell.ts,app-route-graph.ts,build/report.ts,fetch-cache.ts,cache-runtime.ts.- Targeted suites green:
app-route-graph(70),build-report(103),shims(1285),fetch-cache(145),app-page-render(67),app-rsc-response-finalizer(19),app-optimistic-routing(13),link(129),link-navigation(77),prefetch-cache(74). 1782 passing, 0 failing.
Nice work closing the review loop. The nested cache-task refcount (deferring the shell abort until cold cache writes are durable) and the entry-identity-checked optimistic-template invalidation are the right calls.
|
Review posted to PR #2850. SummaryI did an independent review of all 40 changed files, focusing on the shell state machine, route-graph runtime-instant detection, static analysis, the three cache-task refcount wrappers, thenable-params suspension, the RSC finalizer/render-mode plumbing, and the dev HMR change. Ran targeted suites and No blocking issues. The core mechanism is correct and all prior-cycle findings are resolved in the tree I reviewed. Correctness spot-checks that passed:
Three non-blocking observations flagged: (1) unresolved external re-exports conservatively classified as |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: runtime instant prefetch shells (independent pass)
Reviewed all 40 changed files with a focus on the new shell state machine (instant-prefetch-shell.ts), the route-graph runtime-instant detection (app-route-graph.ts), the static analysis in build/report.ts, the three cache-task refcount wrappers (fetch-cache.ts, cache.ts, cache-runtime.ts), thenable-params suspension, the render-mode plumbing, the RSC finalizer, optimistic-template learning, and the dev HMR change. Ran the targeted suites and vp check on the core files — all green.
No blocking issues. The core mechanism is correct, and all five prior-cycle findings are resolved in the tree I reviewed. Confirmed:
instantConfigDependenciesfresh per rebuild —buildAppRouteGraphallocates a freshSetper call (app-route-graph.ts:1270) andcaptureInstantRouteMetadatareassigns it wholesale (index.ts:1453). No monotonic growth.- HMR
changehandler narrowly gated — fires only on tracked config deps or app-router page/layout files that actually exportunstable_instant(index.ts:4725-4728), andrefreshInstantMetadataAfterChangeonly invalidates routing modules when the metadata signature actually changed (index.ts:4666-4670). Fine-grained HMR for ordinary edits is preserved. - Property-value alias resolution — both
hasNamedExportObjectStringPropertyandanalyzeNamedExportObjectStringPropertyresolve the property value viaresolveLocalConstExpression(build/report.ts:180,245). - App-only add/unlink browser-entry refresh —
invalidateAppRoutingModules()→invalidateAppBrowserEntry()runs for tracked deps on add/unlink.
Correctness spot-checks (all pass)
- Abort scheduling + cache-task refcount.
scheduleAfterTaskhas no TDZ (thefirstTimercallback is a macrotask, solet secondTimerhas executed).cancelPendingAbortclears a scheduled abort when a new cache task starts, and the re-check inside the deferred callback guardspendingCacheTasks === 0/!aborted. The nestedvoid trackInstantPrefetchShellCacheTask(() => cacheWrite, "fetch")(fetch-cache.ts:1365) increments the refcount before the outer task'sreturn responseresolves, so there is no zero-refcount window that could fire a premature abort before the cold cache write is durable. - Private-cache staging.
suspendStaticInstantPrefetchPrivateCache()is called beforetrackCacheTaskForShellsincache-runtime.ts:497, so a static shell suspends before both lookup and execution and never registers a cache task that would deadlock against its own abort. dynamicAbortControllernever aborted — intentional hole marker; the render is cut byreactAbortController.- Instant shells excluded from reuse.
promotePrefetchEntriesForNavigationskipsinstantShell/optimisticRouteShell/partialSuspenseShell(link.tsx:852-859);lockPartialShellCacheHeadersforcesno-storeand stripsCDN-Cache-Control/Cloudflare-CDN-Cache-Control/Cache-Tag, and runs after config headers so a matching header rule can't leave a stray cache tag (app-rsc-response-finalizer.ts:125,134). A complete (non-aborted) instant render is promoted to a reusablenavigationentry via theVINEXT_RSC_PARTIAL_SHELL_HEADERcheck (navigation.ts:1394-1404). - Identity-checked template invalidation.
attachPrefetchInvalidationCallbackreturnsfalseon entry supersession (navigation.ts:1060); the browser entry bails when the attach fails (server/app-browser-entry.ts). - Validation parity.
toLinkPrefetchRoutesthrows onunstable_instantin a Client Component module and on use withoutcacheComponents; both are gated so non-instant apps are unaffected. Runtime configs correctly requiresamples(app-route-graph.ts:388, covered by tests).
Non-blocking observations
-
Unresolved external re-export → conservative
runtime(app-route-graph.ts:475-481). A route that doesexport { unstable_instant } from "@myorg/config"(Vite alias / package export the sync scanner can't resolve) is always classified runtime-instant, forcing non-reusable partial shells even when the real value is{ prefetch: "static" }. Deliberate and documented; still worth a user-facing docs note because the failure mode (prefetches silently stop being reused) is hard to diagnose. Carried over from prior cycles. -
isClientReferenceAppComponentonly checks the top-level$$typeof(app-render-dependency.tsx:31-33). A client reference wrapped inReact.memo/forwardRefat a page/layout/slot boundary would not be detected, so itsparams/searchParamswould be givensuspendStaticInstantShell: trueand suspend during a static instant shell even though they are transport data for a Client Component. This is a conservative miss (over-suspends → the shell just becomes a hole for that segment), not a correctness bug, and mirrors the earlier property-value-alias parity gap. Non-blocking; note it if full parity of the static-shell client-reference fast path matters. -
export const unstable_instant = falserequirescacheComponents.hasInstantConfigis true for the explicit-disable value too (app-route-graph.test.ts:192), sotoLinkPrefetchRoutesthrows "cannot useunstable_instantwithout enablingcacheComponents" even when the author is disabling instant. Confirm this matches the target Next.js canary's validation; if Next.js allows the disable form without the flag, this is a small parity divergence. Tested and appears deliberate, so flagging only for confirmation.
Verification
vp checkclean oninstant-prefetch-shell.ts,app-page-render.ts.- Targeted suites green:
app-route-graph(70),app-page-render(67),app-rsc-response-finalizer(19),thenable-params(22),app-optimistic-routing(13),entry-templates(57),build-report(103),prefetch-cache(74). 425 passing, 0 failing.
Nice work closing the review loop. The two-macrotask defer + nested cache-task refcount and the entry-identity-checked optimistic-template invalidation are the right calls.
|
Review posted to PR #2850. SummaryI did an independent review of all 40 changed files, focusing on the shell state machine ( No blocking issues. The core mechanism is correct and all five prior-cycle findings are confirmed resolved in the current tree:
Key correctness spot-checks that passed:
Three non-blocking observations flagged for confirmation:
|
…ue-instant-current # Conflicts: # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts # tests/link.test.ts
…ue-instant-current # Conflicts: # packages/vinext/src/shims/cache-runtime.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: runtime instant prefetch shells (independent pass)
Reviewed all 40 changed files with focus on the shell state machine (instant-prefetch-shell.ts), route-graph runtime-instant detection (app-route-graph.ts), the static analysis in build/report.ts, the cache-task refcount wrappers (fetch-cache.ts, cache-runtime.ts, cache.ts), thenable-params suspension, the RSC finalizer / render-mode plumbing, client-side optimistic-template learning, and the dev HMR change. Ran targeted suites and vp check on the core files — all green.
No blocking issues. The core design is correct and the prior-cycle findings are resolved in the tree I reviewed. The hasFreshPrefetchCacheEntry gate referenced in earlier review cycles no longer exists — the current freshness gate in link.tsx:627-653 keys off hasNavigationCacheEntry, so that earlier observation is moot.
Correctness spot-checks (all pass)
- Abort scheduling + refcount.
scheduleAfterTask's two-macrotask defer has no TDZ (thefirstTimercallback runs as a macrotask, after the synclet secondTimer).cancelPendingAbortat the top oftrackInstantPrefetchShellCacheTaskclears a pending abort when a new cache task starts, and the re-check inside the deferred callback re-guardspendingCacheTasks === 0/!aborted. The nestedvoid trackInstantPrefetchShellCacheTask(() => cacheWrite, "fetch")(fetch-cache.ts:1365) increments the refcount before the outer task'sreturn responseresolves, so there is no zero-refcount window that could fire a premature abort before the cold cache write is durable. - Cache-scope deadlock avoidance. Request-data reads inside a public cache scope return
nullfromsuspendStaticInstantPrefetchRequestData(isInsideStaticRequestDataCacheScopegate), and the key-generationparamsreads happen insidecacheContextStorage.run, so the scope is active when the refcount is held. Private caches suspend beforetrackCacheTaskForShells(cache-runtime.ts:586), so a static shell never registers a cache task that would wait on its own abort. - Runtime vs static staging.
headers()/cookies()/draftMode()suspend only in thestaticstage; runtime shells intentionally admit request data and stop atconnection(). Matches the module docstring and Next's staging. - Complete-render promotion. A non-aborted instant render omits
VINEXT_RSC_PARTIAL_SHELL_HEADER, soprefetchRscResponse(navigation.ts:1408-1417) promotes it to a reusablenavigationentry; aborted shells forceno-store+ stripCDN-Cache-Control/Cloudflare-CDN-Cache-Control/Cache-TagvialockPartialShellCacheHeaders, which runs after config headers so a matching header rule can't leave a stray cache tag. - Instant shells excluded from reuse.
promotePrefetchEntriesForNavigationskipsinstantShell/optimisticRouteShell/partialSuspenseShell(link.tsx:852-858). - Identity-checked template invalidation.
attachPrefetchInvalidationCallbackreturnsfalseon entry supersession;discardLearningOnlyPrefetchCacheEntrynow also handlesinstant-shellentries. - HMR narrowly gated. The
changehandler fires only on tracked config deps or app-router page/layout files that actually exportunstable_instant, andrefreshInstantMetadataAfterChangeinvalidates routing modules only when the derived metadata signature changed (index.ts:4655-4670).instantConfigDependenciesis a freshSetper rebuild, reassigned wholesale bycaptureInstantRouteMetadata. - Client-side lazy sanitization.
sanitizeInstantShellValue's_initcall is idempotent for resolved React lazies and short-circuits suspended ones via the status check, so no shared-module mutation.
Non-blocking observations
-
Unresolved external re-export → conservative
runtime(app-route-graph.ts:475-481).export { unstable_instant } from "@myorg/config"(Vite alias / package export the sync scanner can't resolve) is always classified runtime-instant, forcing non-reusable partial shells even when the real value is{ prefetch: "static" }. Deliberate and documented; still worth a user-facing docs note since the failure mode (prefetches silently stop being reused) is hard to diagnose. Carried over from prior cycles. -
isClientReferenceAppComponentchecks only the top-level$$typeof(app-render-dependency.tsx:31-33). A client reference wrapped inReact.memo/forwardRefat a page/layout boundary is not detected, somakeComponentParamsgives itsuspendStaticInstantShell: trueand its transportparamsover-suspend into a hole during a static shell. Conservative miss (over-suspend, not incorrect output), and it mirrors thecreatePageElementisReactOwnedAppComponentgate. Non-blocking. -
extractStaticJsonValuesparse-array behavior change (build/report.ts). Array holes now pushundefinedinstead of returningUNSUPPORTED_STATIC_VALUE. For instantsamplesa hole would yield anundefinedsample that failsisInstantSampleand throws a clear validation error — arguably better than silently classifying the whole config as non-static, but it is a behavior change worth a confirming test. The new cycle-detectionvisitingset with try/finally cleanup is a nice defensive addition. -
export const unstable_instant = falsestill requirescacheComponents.hasInstantConfigis true for the explicit-disable value, sotoLinkPrefetchRoutesthrows even when the author is disabling instant. Confirm against the target Next.js canary's validation; if Next allows the disable form without the flag this is a small parity divergence. Tested and appears deliberate.
Verification
vp checkclean oninstant-prefetch-shell.ts,app-optimistic-routing.ts,thenable-params.ts.- Targeted suites green:
app-route-graph(70),build-report(103),thenable-params(22),app-page-render(67),app-optimistic-routing(13),app-rsc-response-finalizer(19),link(130),link-navigation(78),prefetch-cache(76). 578 passing, 0 failing.
Nice work closing the review loop — the two-macrotask defer + nested cache-task refcount and the entry-identity-checked optimistic-template invalidation are the right calls.
|
Review posted to PR #2850. SummaryI did an independent review of all 40 changed files, focusing on the shell state machine, route-graph runtime-instant detection, the static analysis in No blocking issues. The core mechanism is correct and prior-cycle findings are resolved in the current tree. Key correctness spot-checks that passed:
Four non-blocking observations flagged for confirmation:
I also noted that the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: runtime instant prefetch shells (independent pass)
Reviewed all 40 changed files with focus on the new shell state machine (instant-prefetch-shell.ts), the route-graph runtime-instant detection (app-route-graph.ts), the static analysis in build/report.ts, the three cache-task refcount wrappers (fetch-cache.ts, cache-runtime.ts, cache.ts), thenable-params suspension, the render-mode/finalizer plumbing, client-side optimistic-template learning + promotion, and the dev HMR change. Ran targeted suites and vp check on the core files — all green.
No blocking issues. The design is correct and the prior-cycle findings (unbounded config-dep set, broad HMR invalidation, property-value alias resolution, app-only add/unlink browser-entry refresh) are all resolved in the tree I reviewed.
Correctness spot-checks (all pass)
- Abort scheduling + refcount.
scheduleAfterTask's two-macrotask defer has no TDZ (thefirstTimercallback is a macrotask, solet secondTimerat line 54 has already executed).cancelPendingAbortat the top oftrackInstantPrefetchShellCacheTaskclears a scheduled abort when a new task starts, and the deferred callback re-guardspendingCacheTasks === 0/!aborted. The nestedvoid trackInstantPrefetchShellCacheTask(() => cacheWrite, "fetch")(fetch-cache.ts) increments the refcount before the outer task'sreturn responseresolves, so there is no zero-refcount window that could fire a premature abort before the cold cache write is durable. - Private-cache staging moved out of the tracked task. In
cache-runtime.ts,suspendStaticInstantPrefetchPrivateCache()(and the private-in-public error) now runs beforetrackCacheTaskForShells, so a static shell suspends before registering a cache task and never deadlocks against its own abort. Moving the parent-context check to the top also makes it fire even for non-serializable args — strictly more correct. - Complete-render promotion. A non-aborted instant render omits
VINEXT_RSC_PARTIAL_SHELL_HEADER, soprefetchRscResponsepromotes it to a reusablenavigationentry (cacheForNavigation = true,prefetchKind = "navigation"); aborted shells forceno-store+ stripCDN-Cache-Control/Cloudflare-CDN-Cache-Control/Cache-TagvialockPartialShellCacheHeaders, which runs after config headers so a matching header rule can't leave a stray cache tag. - Instant shells excluded from reuse.
promotePrefetchEntriesForNavigationskipsinstantShell/optimisticRouteShell/partialSuspenseShell;discardLearningOnlyPrefetchCacheEntrynow also handlesinstant-shell. - Identity-checked template invalidation.
attachPrefetchInvalidationCallbackreturnsfalseon entry supersession; the source-key set (optimisticRouteTemplateSources) is added insidelearnOptimisticRouteTemplateFromPrefetchusing the samegetOptimisticPrefetchSourceKeyderivation as the outer learning loop'soptimisticRouteTemplateLearning.delete, so keys match and no learning slot leaks. - HMR narrowly gated. The
changehandler fires only on tracked config deps or app-router page/layout files that actually exportunstable_instant, andrefreshInstantMetadataAfterChangeinvalidates routing modules only when the derived metadata signature changed.instantConfigDependenciesis a freshSetperbuildAppRouteGraph, reassigned wholesale bycaptureInstantRouteMetadata. The unconditionalinvalidateAppRouteCache()before the re-scan only clears a memo cache, so skippinginvalidateAppRoutingModules()on an unchanged signature is safe.
Non-blocking observations
-
Promoted instant entries lack
preparedElements(perf, not correctness). When a runtime instant shell is promoted to anavigationentry insideprefetchRscResponse.then(navigation.ts:1411-1418), the fixedbehaviorobject still hasprepareSnapshot === undefined(instant prefetches set it only whencacheForNavigationwas truthy at call time). So the promoted entry getsentry.snapshotbut neverentry.preparedElements, and navigation must decode the buffered RSC payload through the normal path instead of reusing pre-decoded elements. TheprepareSnapshotcomment already calls preparation "an acceleration only," so output is correct — a promoted instant navigation is just slightly slower to commit than a natively-reusablefullprefetch. Worth a follow-up if instant→navigation commit latency matters. -
Unresolved external re-export → conservative
runtime(app-route-graph.tsrouteModuleExportInstantPrefetchMode).export { unstable_instant } from "@myorg/config"(Vite alias / package export the sync scanner can't resolve) is always classified runtime-instant, forcing non-reusable partial shells even when the real value is{ prefetch: "static" }. Deliberate and documented; still worth a user-facing docs note since the failure mode (prefetches silently stop being reused) is hard to diagnose. Carried over from prior cycles. -
extractStaticJsonValuesparse-array /undefinedbehavior change is untested (build/report.ts). Array holes now pushundefined(and anundefinedidentifier resolves toundefined) instead of returningUNSUPPORTED_STATIC_VALUE. Downstream this is benign — a hole insamplesyields anundefinedelement that failsisInstantSampleand throws a clearassertValidInstantConfigerror, andexport const unstable_instant = undefinedsimilarly throws — buttests/build-report.test.tshas no case covering it. A confirming test would lock in the intended "throws a clear error" outcome. The new cycle-detectionvisitingset with try/finally cleanup is a good defensive addition. -
isClientReferenceAppComponentchecks only the top-level$$typeof(app-render-dependency.tsx:31-33). A client reference wrapped inReact.memo/forwardRefat a page/layout boundary is not detected, somakeComponentParamsgives itsuspendStaticInstantShell: trueand its transportparamsover-suspend into a hole during a static shell. Conservative miss (over-suspend, not wrong output), mirroring theisReactOwnedAppComponentgate. Non-blocking. -
export const unstable_instant = falsestill requirescacheComponents.hasInstantConfigis true for the explicit-disable value (app-route-graph.test.ts:192), sotoLinkPrefetchRoutesthrows "cannot useunstable_instant… without enablingcacheComponents" even when the author is disabling instant. Tested and appears deliberate; worth confirming against the target Next.js canary's validation in case Next allows the disable form without the flag.
Verification
vp checkclean oninstant-prefetch-shell.ts,app-route-graph.ts,build/report.ts.- Targeted suites green:
build-report(103),app-page-render(67),link-navigation(78),prefetch-cache(76). No failures.
Nice work closing the review loop — the two-macrotask defer + nested cache-task refcount and the entry-identity-checked optimistic-template invalidation are the right calls.
|
Review posted to PR #2850. SummaryI did an independent review of all 40 changed files, focusing on the shell state machine ( No blocking issues. The core mechanism is correct and all prior-cycle findings are resolved in the current tree. Key correctness spot-checks that passed:
One new observation worth a follow-up (non-blocking): promoted instant→navigation entries never get Four carried-over non-blocking notes: unresolved external re-exports conservatively classified |
# Conflicts: # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/navigation.ts
1 participant
Summary
unstable_instanton pages, layouts, slots, aliases, and local re-export chainsconnection()contentValidation
vp checktest/e2e/app-dir/prefetch-true-instant/prefetch-true-instant.test.ts— 2/2 passedFailure source: Actions run 31290819291.