fix(app-router): support instant route prefetch shells#2374
fix(app-router): support instant route prefetch shells#2374james-elicx wants to merge 8 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
fea3789 to
76b8990
Compare
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
76b8990 to
0d87d61
Compare
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: instant route prefetch shells
I focused on Next.js parity (per AGENTS.md the Next.js test suite is the authoritative reference) and on the runtime correctness of the new sanitize path. Two issues look blocking, plus a smaller race concern.
1. Wrong export name — does not match Next.js (unstable_instant vs instant + prefetch) — blocking
This PR keys the whole feature off a unstable_instant page/layout export. Next.js does not use that export. The exact test this PR claims to port (test/e2e/app-dir/prefetch-true-instant/prefetch-true-instant.test.ts) opts a route into instant prefetch with:
export const instant = {
unstable_samples: [{ cookies: [{ name: 'test', value: null }] }],
}
export const prefetch = 'allow-runtime'(see app/target-page/page.tsx and app/layout-instant/layout.tsx in that fixture). There is no unstable_instant export anywhere in the Next.js source or test suite (gh api search/code?q=unstable_instant+repo:vercel/next.js → 0 results).
The consequence is a silent behavioral divergence: a real Next.js app that uses export const prefetch = 'allow-runtime' / export const instant = {...} gets none of this behavior on vinext, and a vinext app written against this PR uses an export that Next.js ignores. The PR's own e2e fixture hard-codes the invented name:
// tests/e2e/app-router/nextjs-compat/suspense-prefetch.browser.spec.ts:119
`export const unstable_instant = { prefetch: "runtime", samples: [] };\n${instantPage}`so the test passes against the wrong contract. Please align detection with the actual Next.js surface (prefetch === 'allow-runtime', optionally instant) in routeModuleHasInstant (packages/vinext/src/routing/app-route-graph.ts) and update the fixtures/tests to match.
2. sanitizeInstantShellValue throws on pending/halted RSC chunks — blocking
packages/vinext/src/server/app-optimistic-routing.ts:289-307:
if (isUnknownRecord(payload) && payload.status === "rejected") {
return createElement(OptimisticRouteSegment);
}
if (typeof initialize === "function") {
return sanitizeInstantShellValue(Reflect.apply(initialize, undefined, [payload]));
}This only special-cases status === "rejected". For any other status it calls _init(payload) (react-server-dom's readChunk). For instant shells the dynamic connection() subtree is intentionally suspended, so its serialized chunk arrives pending / blocked / halted, not fulfilled or rejected. readChunk throws the chunk itself in those states:
case "pending":
case "blocked":
case "halted":
throw chunk;(verified against the bundled react-server-dom-webpack-client.node.development.js and reproduced standalone). That throw propagates out of sanitizeInstantShellElements → createOptimisticRouteTemplate → learnOptimisticRouteTemplateFromPrefetch. It's swallowed by Promise.allSettled in learnOptimisticRouteTemplatesFromPrefetchCache (app-browser-entry.ts:508), so there's no crash — but the optimistic instant template is silently never learned for exactly the routes this feature targets (cached content + suspended dynamic content), defeating the stated goal of "preserve cached content while suspending connection() content."
The unit test masks this because it mocks the dynamic child as _payload: { status: "rejected" } and the cached child as "resolved_model" (tests/app-optimistic-routing.test.ts:307-343). Neither reflects the real pending/halted dynamic chunk. Please:
- treat pending/blocked/halted chunks as suspended segments (return
OptimisticRouteSegment) rather than calling_init, and - add a unit test that exercises a
status: "pending"(and"halted") lazy child whose_initthrows the chunk, asserting it is replaced by the suspense trigger rather than propagating.
3. Instant-mode abort scheduling depends on connection() being reached — confirm
In app-page-render.ts:719-722, instant mode does not call schedulePrefetchSuspenseShellAbort up front; scheduling only happens lazily inside suspendPrefetchSuspenseShell when connection() is encountered (prefetch-suspense-shell.ts:69). If an instant route renders without ever hitting connection() (or a dynamic API), the abort is never scheduled, wasPrefetchSuspenseShellAborted stays false, and partialShell (app-page-render.ts:841) is not set. Is that intended (a fully-cacheable instant route is a complete, non-partial response)? If so a comment would help; if not, this is a correctness gap. Either way please confirm against Next.js' behavior for an instant route with no dynamic APIs.
Process note
Per AGENTS.md ("Always verify Next.js behavior first" / "Searching the Next.js Test Suite" is a required step), issue #1 should have been caught by reading the ported fixture's exports. Recommend re-deriving the detection contract directly from prefetch-true-instant before re-requesting review.
|
Review posted to PR #2374 requesting changes. Summary of findings: Blocking issues1. Wrong export name — Next.js parity violation 2. Non-blocking3. Instant-mode abort scheduling is lazy and only fires if Both blocking issues stem from skipping the required "verify Next.js behavior first" step — the export contract was discoverable directly in the ported fixture. |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
2 times, most recently
from
f9032d8 to
3c8accc
Compare
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
3c8accc to
8a5c51f
Compare
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
8a5c51f to
65b1ff8
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells
I focused on Next.js parity (per AGENTS.md the Next.js test suite is the authoritative reference) and re-checked the two issues raised in the prior /bigbonk round. Issue #2 (sanitize throwing on suspended chunks) is now fixed. Issue #1 (wrong export contract) is still unaddressed and remains blocking — and digging into the real Next.js surface, the detection is wrong on two axes, not just the name.
1. Detection uses an invented export and the wrong semantics — blocking
routeModuleHasInstant (packages/vinext/src/routing/app-route-graph.ts:199-206) keys the entire feature off a truthy unstable_instant export:
function routeModuleHasInstant(filePath: string | null): boolean {
if (filePath === null) return false;
try {
return hasTruthyNamedExport(fs.readFileSync(filePath, "utf8"), "unstable_instant");
} catch {
return false;
}
}This does not match Next.js, and there are two distinct problems:
(a) unstable_instant is not a real Next.js export. gh api search/code?q=unstable_instant+repo:vercel/next.js → 0 results. The actual segment config exports are prefetch and instant (packages/next/src/build/segment-config/app/app-segment-config.ts:137-153, :261-281). Runtime prefetch is gated specifically on the prefetch config:
// packages/next/src/server/app-render/instant-validation/instant-config.tsx
const prefetchConfig = layoutOrPageMod ? (layoutOrPageMod as AppSegmentConfig).prefetch : undefined
if (prefetchConfig === 'allow-runtime') {
return true
}The ported test this PR claims to mirror opts a route in with export const prefetch = 'allow-runtime' + export const instant = {...}, never unstable_instant. Result: a real Next.js app using prefetch = 'allow-runtime' gets none of this behavior on vinext, and a vinext app written against this PR uses an export Next.js ignores.
(b) Even with the right name, a truthiness check is wrong. prefetch is an enum string — 'auto' | 'partial' | 'unstable_eager' | 'force-disabled' | 'allow-runtime' (app-segment-config.ts:44, schema error at :207). Only 'allow-runtime' enables instant runtime prefetch. hasTruthyNamedExport(code, "prefetch") would treat prefetch = 'auto', prefetch = 'partial', and even prefetch = 'force-disabled' as instant routes, since all are truthy strings. force-disabled is the exact opposite of the intended behavior. Detection needs an exact value match (prefetch === 'allow-runtime'), not truthiness.
Please re-derive detection directly from the prefetch-true-instant fixture: match prefetch === 'allow-runtime' (with instant as the related config), and update hasTruthyNamedExport-based detection to a value-aware check. Then fix the fixtures/tests, which currently hard-code the invented name:
packages/vinext/src/routing/app-route-graph.ts:202tests/e2e/app-router/nextjs-compat/suspense-prefetch.browser.spec.ts:119,123,266tests/app-route-graph.test.ts:104-119,254-283tests/build-report.test.ts:98-112
Because every test validates the invented contract, the suite stays green while the feature is invisible to real apps — exactly the silent-divergence failure mode AGENTS.md's "verify Next.js behavior first" step exists to prevent.
2. sanitizeInstantShellValue on suspended chunks — resolved (was blocking)
This is now correct. app-optimistic-routing.ts:307-326 special-cases pending/blocked/halted/rejected via SUSPENDED_LAZY_STATUSES and returns OptimisticRouteSegment before calling _init, so the suspended connection() subtree no longer throws out of sanitizeInstantShellElements. The new tests/app-optimistic-routing.test.ts:396-451 cases exercise pending/blocked/halted lazy children whose _init throws and assert they're replaced by the suspense trigger across page/layout/slot elements. Good — no further action here.
3. Instant-mode abort scheduling depends on connection() being reached — confirm
app-page-render.ts instant mode does not call schedulePrefetchSuspenseShellAbort up front; scheduling happens lazily inside suspendPrefetchSuspenseShell only when connection() (or a dynamic API) is hit (prefetch-suspense-shell.ts). For an instant route that renders with no dynamic API, the abort never schedules, wasPrefetchSuspenseShellAborted stays false, and partialShell is not set. If that's intentional (a fully-cacheable instant route is a complete, non-partial response), add a comment; otherwise this is a correctness gap. Please confirm against Next.js' behavior for an instant route with no dynamic APIs.
Summary
|
Posted a CHANGES_REQUESTED review to PR #2374. Findings: Blocking1. Detection uses an invented export and wrong semantics
All tests hard-code the invented name, so the suite stays green while the feature is invisible to real Next.js apps — the silent-divergence failure mode AGENTS.md's "verify Next.js behavior first" step is meant to catch. Resolved since last round2. Non-blocking3. Instant-mode abort scheduling is lazy and only fires if |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
65b1ff8 to
9faed78
Compare
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
9faed78 to
bb7cf30
Compare
|
/bigbonk review for issues |
|
UnknownError: The operation timed out. |
james-elicx
force-pushed
the
codex/fix-prefetch-true-instant-stacked
branch
from
bb7cf30 to
ac4232c
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells
Re-reviewed against the Next.js source/test suite (per AGENTS.md, that's the authoritative reference). Issue #2 stays resolved. Issue #1 — the wrong export contract — is still unaddressed after three rounds and remains blocking. Verified the real Next.js surface directly this round.
1. Detection uses an invented export and the wrong semantics — blocking
routeModuleHasInstant (packages/vinext/src/routing/app-route-graph.ts:199-206):
function routeModuleHasInstant(filePath: string | null): boolean {
if (filePath === null) return false;
try {
return hasTruthyNamedExport(fs.readFileSync(filePath, "utf8"), "unstable_instant");
} catch {
return false;
}
}Two distinct problems, both confirmed against the real Next.js source:
(a) unstable_instant is not a real Next.js export. gh api 'search/code?q=unstable_instant+repo:vercel/next.js' → 0 results. The actual segment-config exports are prefetch and instant (packages/next/src/build/segment-config/app/app-segment-config.ts). The prefetch-true-instant fixture this PR claims to mirror opts a route in with:
// app/target-page/page.tsx and app/layout-instant/layout.tsx
export const instant = {
unstable_samples: [{ cookies: [{ name: 'test', value: null }] }],
}
export const prefetch = 'allow-runtime'Note: prefetch is a top-level export whose value is 'allow-runtime' — not a prefetch: "runtime" property on an unstable_instant object (the shape this PR's tests fabricate). A real Next.js app using prefetch = 'allow-runtime' gets none of this behavior on vinext, and a vinext app written against this PR uses an export Next.js ignores.
(b) Even with the right name, a truthiness check is wrong. Runtime prefetch in Next.js is gated on an exact value match (packages/next/src/server/app-render/instant-validation/instant-config.tsx):
const prefetchConfig = layoutOrPageMod
? (layoutOrPageMod as AppSegmentConfig).prefetch
: undefined
if (prefetchConfig === 'allow-runtime') {
return true
}prefetch is an enum string — "auto" | "partial" | "unstable_eager" | "force-disabled" | "allow-runtime" (app-segment-config.ts:207). Only 'allow-runtime' opts in. hasTruthyNamedExport(code, "prefetch") would treat prefetch = 'auto', 'partial', and even prefetch = 'force-disabled' as instant routes, since all are truthy string literals (hasTruthyNamedExport returns Boolean(value) for string literals, report.ts:156-157). Treating force-disabled as instant is the exact opposite of the intended behavior. Detection needs an exact value match, not truthiness.
Suggested fix. extractExportConstString already lives in the same module (packages/vinext/src/build/report.ts:244) and gives you the value. Match prefetch === 'allow-runtime' (with instant as the related config) and update the fixtures/tests, which currently hard-code the invented name and shape:
packages/vinext/src/routing/app-route-graph.ts:195,202tests/e2e/app-router/nextjs-compat/suspense-prefetch.browser.spec.ts:119,123,266tests/app-route-graph.test.ts:104-119,254-283tests/build-report.test.ts:94-166
Because every test validates the invented unstable_instant contract, the suite stays green while the feature is invisible to real Next.js apps — exactly the silent-divergence failure mode AGENTS.md's "verify Next.js behavior first" step is meant to prevent.
2. sanitizeInstantShellValue on suspended chunks — resolved (was blocking)
Confirmed fixed. app-optimistic-routing.ts:307-326 special-cases pending/blocked/halted/rejected via SUSPENDED_LAZY_STATUSES and returns OptimisticRouteSegment before calling _init, so the suspended connection() subtree no longer throws out of sanitizeInstantShellElements. New tests at tests/app-optimistic-routing.test.ts:396-451 exercise the pending/blocked/halted paths. No further action.
3. Instant abort scheduling only fires on connection() — confirm intent
Two related observations in instant mode:
- Scheduling is lazy:
app-page-render.ts:719-722callscancelPrefetchSuspenseShellAbort(notschedulePrefetchSuspenseShellAbort) for instant shells, so the abort is only armed insidesuspendPrefetchSuspenseShellwhen reached (prefetch-suspense-shell.ts:69). An instant route that renders with no dynamic API never setspartialShell. - Narrow trigger:
prefetch-suspense-shell.ts:68only suspends instant mode onexpression === "connection()", ignoringcookies(),headers(),searchParams, etc. The fixture intentionally preservescookies()-derived "cached" content, so suspending only onconnection()may be deliberate — but please confirm this matches Next.js' instant-shell behavior for routes whose only dynamic dependency is something other thanconnection(), and add a comment documenting the intent either way.
Summary
|
Review posted to PR #2374 as CHANGES_REQUESTED. SummaryI verified all three prior findings against the actual Next.js source/test suite. Blocking1. Detection uses an invented export and wrong semantics — still unaddressed after three rounds.
Resolved2. Non-blocking3. Instant abort scheduling is lazy and the instant suspend trigger only fires on |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells
Re-reviewed against the Next.js source/test suite (per AGENTS.md, that's the authoritative reference), including the new commit 5aef816 ("close instant prefetch invalidation races"). Issue #2 stays resolved. The new race fix is sound. Issue #1 — the wrong export contract — is still unaddressed after four rounds and remains blocking. I verified the real Next.js fixture and gating logic directly this round.
1. Detection uses an invented export and the wrong semantics — blocking (unchanged)
routeModuleHasInstant (packages/vinext/src/routing/app-route-graph.ts:199-206) still keys the entire feature off a truthy unstable_instant export, and the new commit doesn't touch it:
function routeModuleHasInstant(filePath: string | null): boolean {
if (filePath === null) return false;
try {
return hasTruthyNamedExport(fs.readFileSync(filePath, "utf8"), "unstable_instant");
} catch {
return false;
}
}Two distinct problems, both confirmed against the real Next.js source this round:
(a) unstable_instant is not a real Next.js export. gh api 'search/code?q=unstable_instant+repo:vercel/next.js' → 0 results. The prefetch-true-instant fixture this PR claims to mirror opts a route in with two top-level exports — verified by reading the fixture directly:
// test/e2e/app-dir/prefetch-true-instant/app/target-page/page.tsx
// (identical exports in app/layout-instant/layout.tsx)
export const instant = {
unstable_samples: [{ cookies: [{ name: 'test', value: null }] }],
}
export const prefetch = 'allow-runtime'There is no unstable_instant export, and prefetch is a top-level string export valued 'allow-runtime' — not a prefetch: "runtime" property on an unstable_instant object (the shape this PR fabricates). A real Next.js app using prefetch = 'allow-runtime' gets none of this behavior on vinext; a vinext app written against this PR uses an export Next.js ignores.
(b) Even with the right name, a truthiness check is wrong. Runtime prefetch in Next.js is gated on an exact value match (verified in packages/next/src/server/app-render/instant-validation/instant-config.tsx):
const prefetchConfig = layoutOrPageMod
? (layoutOrPageMod as AppSegmentConfig).prefetch
: undefined
if (prefetchConfig === 'allow-runtime') {
return true
}prefetch is an enum string — 'auto' | 'partial' | 'unstable_eager' | 'force-disabled' | 'allow-runtime' (app-segment-config.ts:42-54). Only 'allow-runtime' opts in. hasTruthyNamedExport(code, "prefetch") would treat prefetch = 'auto', 'partial', and even prefetch = 'force-disabled' as instant routes (all truthy string literals — hasTruthyNamedExport returns Boolean(value) for string literals, report.ts:156-157). Treating force-disabled as instant is the exact opposite of the intended behavior. Detection needs an exact value match, not truthiness.
Suggested fix. extractExportConstString already lives in the same module (packages/vinext/src/build/report.ts:244) and returns the string value. Match prefetch === 'allow-runtime' (with instant as the related config) and update the fixtures/tests, which currently hard-code the invented name and shape:
packages/vinext/src/routing/app-route-graph.ts:195,202tests/e2e/app-router/nextjs-compat/suspense-prefetch.browser.spec.ts:51,55(and theunstable_instantstrings)tests/app-route-graph.test.tstests/build-report.test.ts:94-166(the entirehasTruthyNamedExportinstant suite asserts the inventedunstable_instant/prefetch: 'runtime'shape)
Because every test validates the invented unstable_instant contract, the suite stays green while the feature is invisible to real Next.js apps — exactly the silent-divergence failure mode AGENTS.md's "verify Next.js behavior first" step is meant to prevent. The contract was discoverable by reading the ported fixture's exports.
2. sanitizeInstantShellValue on suspended chunks — resolved (was blocking)
Confirmed still fixed. app-optimistic-routing.ts:307-326 special-cases pending/blocked/halted/rejected via SUSPENDED_LAZY_STATUSES and returns OptimisticRouteSegment before calling _init, so the suspended connection() subtree no longer throws out of sanitizeInstantShellElements. Unit tests at tests/app-optimistic-routing.test.ts exercise the suspended paths. No further action.
New commit 5aef816 (invalidation race fix) — looks correct
Reviewed the fresh commit independently:
learnOptimisticRouteTemplateFromPrefetchnow addsoptimisticRouteTemplateSourcessynchronously (rather than in the post-await.then), and gates theinstantShellcase onattachPrefetchInvalidationCallback(..., options.entry)returningtrue. The newexpectedEntryidentity check (navigation.ts:587) correctly bails (return false) when the cache entry was replaced between prefetch resolution and callback attachment, and the caller returnsfalsewithout registering a stale template.- Ordering is safe:
schedulePrefetchInvalidationusessetTimeout(delay ≥ 0), so the invalidation callback can't fire synchronously duringattachPrefetchInvalidationCallback— the template is always inoptimisticRouteTemplatesbefore any invalidation runs. TheoptimisticRouteTemplates.get(templateKey) === templateidentity guard further protects against deleting a newer template. - New regression test
tests/prefetch-cache.test.ts("does not attach invalidation ownership to a replaced prefetch entry") covers the race. Verified passing locally (vp test run tests/prefetch-cache.test.ts→ 32 passed). - The dev-only
server.watcher.on("change")handler (index.ts:3583) and theinvalidateHybridClientEntriesgating change (hasAppDir-only, app-browser entry invalidation) are in scope and consistent. No duplicate change handler.
No issues here.
3. Instant abort scheduling only fires on connection() — non-blocking (aligns with fixture)
prefetch-suspense-shell.ts:68 suspends instant mode only on expression === "connection()", ignoring cookies()/headers()/searchParams, and scheduling is lazy (app-page-render.ts calls cancelPrefetchSuspenseShellAbort, not schedulePrefetchSuspenseShellAbort, up front). Reading the real fixture, this actually matches Next.js intent: Cached uses cookies() and is preserved as cached content, while Dynamic uses connection() and is suspended. So suspending only on connection() is consistent with the fixture. Please add a short comment documenting that cookies()-derived content is intentionally treated as cacheable (not suspended) so this isn't mistaken for a gap later.
Summary
- #1 blocking — wrong export name and wrong detection semantics; re-derive the contract from
prefetch-true-instant(prefetch === 'allow-runtime', relatedinstant) usingextractExportConstString, and fix the fixtures/tests. - #2 resolved.
- New race fix — correct, no issues.
- #3 non-blocking — aligns with the fixture; add a clarifying comment.
|
Posted a CHANGES_REQUESTED review to PR #2374. SummaryReviewed the latest state including the new commit Blocking#1 — Detection uses an invented export and wrong semantics (still unaddressed after 4 rounds)
Resolved / sound
Non-blocking
|
|
Bonk clarification: this PR is intentionally verified against the deploy-suite pinned Next.js v16.2.6 checkout at
The exact pinned Next.js test passes 2/2 on head /bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells (verified against pinned Next.js v16.2.6 ee6e79b1)
Per the bonk clarification, I re-verified this PR against the deploy-suite pinned Next.js checkout ee6e79b1792a4d401ddf2480f40a83549fe8e722 (v16.2.6), not canary. I fetched that exact commit and read the fixture and gating source directly. The detection contract this PR implements is correct for the pinned target. The prior rounds' blocking finding (#1) does not apply to this version.
#1 — Export contract: correct for v16.2.6 (NOT a divergence)
The earlier reviews asserted unstable_instant is "0 results / invented" and that the real contract is export const instant = {...} + export const prefetch = 'allow-runtime'. That is the canary surface. In the pinned ee6e79b1 source the contract is exactly what this PR implements:
// test/e2e/app-dir/prefetch-true-instant/app/target-page/page.tsx (and layout-instant/layout.tsx)
export const unstable_instant = {
prefetch: 'runtime',
samples: [{ cookies: [{ name: 'test', value: null }] }],
}And the gating logic (packages/next/src/server/app-render/instant-validation/instant-config.tsx, anySegmentHasRuntimePrefetchEnabled):
const instantConfig = (layoutOrPageMod as AppSegmentConfig).unstable_instant
const hasRuntimePrefetch =
instantConfig && typeof instantConfig === 'object'
? instantConfig.prefetch === 'runtime'
: falseapp-segment-config.ts in this commit declares unstable_instant: InstantConfigSchema.optional() with a discriminated union on prefetch: 'static' | 'runtime'. There is no top-level prefetch enum and no 'allow-runtime' value in this version. So:
unstable_instantis the correct export name here.- Gating on
unstable_instant.prefetch === 'runtime'is correct.
The PR's routeModuleHasInstant (app-route-graph.ts:199-211) now matches this exactly via the new hasNamedExportObjectStringProperty(code, "unstable_instant", "prefetch", "runtime") helper added in 0f09c0a.
#1b — Truthiness concern: addressed in commit 0f09c0a
The one legitimate kernel of the prior reviews — that a bare truthiness check would over-match — has been fixed. hasNamedExportObjectStringProperty (report.ts:160-190) does an exact prefetch === 'runtime' literal match against the parsed object expression, handles aliased re-exports, unwraps TS wrappers, requires a non-computed key, and refuses to guess external re-exports. The new test at tests/app-route-graph.test.ts confirms unstable_instant = { prefetch: 'static' } correctly yields hasInstant === false — matching Next.js, where 'static' is not runtime prefetch. Verified tests/app-route-graph.test.ts + tests/build-report.test.ts pass (168/168).
#2 — sanitizeInstantShellValue on suspended chunks: resolved
SUSPENDED_LAZY_STATUSES special-cases pending/blocked/halted/rejected and returns OptimisticRouteSegment before calling _init (app-optimistic-routing.ts:307-326), with covering tests. Confirmed passing.
#3 — connection()-only suspend / lazy abort: correct, matches the fixture
suspendPrefetchSuspenseShell suspends instant mode only on connection() and preserves cookies()-derived content as cached (prefetch-suspense-shell.ts:68). This is exactly what the pinned e2e test asserts: Cached (cookies()) is included in the downgraded prefetch; Dynamic (connection()) is excluded and only resolves on the (blocked) navigation. The layout-level test confirms SubtreeHasInstant propagation, which routeHasInstant (app-route-graph.ts:213-227) implements by scanning page + ancestor layouts + parallel slots. Non-blocking nit: a one-line comment noting cookies() content is intentionally treated as cacheable would help future readers, but behavior is correct.
Verification summary
- Fetched pinned
ee6e79b1(v16.2.6); read fixture pages/layouts andinstant-config.tsx/app-segment-config.tsdirectly. - Detection, exact-value matching, suspended-chunk handling, and
connection()-only suspend all match the pinned source/fixture. tests/app-route-graph.test.ts,tests/build-report.test.ts,tests/app-optimistic-routing.test.ts,tests/prefetch-cache.test.ts,tests/link.test.tspass locally (340 assertions across these files).
Approving. The prior CHANGES_REQUESTED blocker was based on a different Next.js version; against the pinned deploy target this PR is correct, and the truthiness refinement landed in 0f09c0a. Recommend addressing the #3 comment nit at the author's discretion (non-blocking).
|
Review posted to PR #2374 as APPROVED. SummaryI verified this PR against the deploy-suite pinned Next.js checkout #1 (the standing blocker) does not apply to the pinned target. Prior reviews verified against canary, where the API is
#1b (the one legitimate kernel — truthiness over-matching) is fixed in commit #2 (suspended-chunk sanitize) resolved; #3 ( All relevant test files pass locally. Left a non-blocking nit suggesting a clarifying comment for the |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells (verified against installed Next.js v16.2.7 9bd3c26a)
I reviewed against the authoritative installed Next.js source in this repo's node_modules (next@16.2.7, build commit 9bd3c26a733901956549889f28c0047553bba622) — read instant-config.js, app-segment-config.js, and the e2e fixture directly. The detection contract this PR implements is correct for the deploy target. The prior rounds' standing blocker (#1) was verified against canary, which has a newer/different API; it does not apply here.
#1 — Export contract & semantics: correct (not a divergence)
Installed dist/esm/server/app-render/instant-validation/instant-config.js:
const instantConfig = layoutOrPageMod ? layoutOrPageMod.unstable_instant : undefined;
const hasRuntimePrefetch =
instantConfig && typeof instantConfig === 'object' ? instantConfig.prefetch === 'runtime' : false;And dist/esm/build/segment-config/app/app-segment-config.js declares unstable_instant: InstantConfigSchema.optional() with a discriminated union on prefetch: 'static' | 'runtime'. There is no top-level prefetch enum and no 'allow-runtime' value in this version.
So in the pinned/installed target:
unstable_instantis the correct export name (a real export).- Gating on
unstable_instant.prefetch === 'runtime'is correct.
routeModuleHasInstant (app-route-graph.ts:199-211) matches this exactly via hasNamedExportObjectStringProperty(code, "unstable_instant", "prefetch", "runtime"). The e2e fixture (suspense-prefetch.browser.spec.ts:110-123) uses export const unstable_instant = { prefetch: "runtime", samples: [] } with cookies() (cached) + connection() (dynamic), matching the installed fixture shape.
#1b — Truthiness over-matching: fixed
The one legitimate kernel of the prior reviews. hasNamedExportObjectStringProperty (report.ts:160-190) does an exact prefetch === 'runtime' literal match against the parsed object expression — handles aliased local re-exports, unwraps TS wrappers, requires a non-computed key, and conservatively refuses to guess external re-exports. Tests confirm prefetch: 'static' → false and prefetch: 'runtime' → true (build-report.test.ts:169-213). static is correctly excluded.
#2 — sanitizeInstantShellValue on suspended chunks: resolved
SUSPENDED_LAZY_STATUSES special-cases pending/blocked/halted/rejected and returns OptimisticRouteSegment before calling _init (app-optimistic-routing.ts:307-333). It also wraps _init in a try/catch that converts a thrown chunk/thenable into the suspense trigger — defense in depth. Covering tests pass.
#3 — connection()-only suspend / lazy abort: correct, matches the fixture
suspendPrefetchSuspenseShell suspends instant mode only on connection() and preserves cookies()-derived content as cached (prefetch-suspense-shell.ts:68-71). Abort scheduling is lazy: app-page-render.ts:719-723 uses cancelPrefetchSuspenseShellAbort up front, and the abort is only armed inside suspendPrefetchSuspenseShell when connection() is reached. For an instant route with no dynamic API, partialShell stays false → a complete instant shell. This matches the installed fixture (Cached via cookies() is included; Dynamic via connection() is suspended). Non-blocking nit: a one-line comment noting cookies()-derived content is intentionally treated as cacheable would help future readers.
New commit 7033498 (preserve instant shell HMR semantics) — looks correct
sanitizeInstantShellValuenow recurses into all element props, not justchildren(app-optimistic-routing.ts:335-342). This is needed because suspended instant content can be passed via a named prop (e.g.content), as the new test atapp-optimistic-routing.test.ts:444-466exercises. Without it, a suspended chunk nested under a non-childrenprop would have escaped sanitization.- The dev
changehandler (index.ts:3602-3610) now invalidates the app route cache only when a file's instant status flips, tracked viainstantConfigByFile. This is a deliberate, correct optimization:hasInstantis baked into the route graph at scan time, so the graph only needs rebuilding when the config flips; content edits are handled by normal Vite HMR, and structural file add/remove is handled by theadd/unlinkhandlers. TheinstantConfigByFilemap is seeded asynchronously, but the?? falsedefault makes the first post-seed flip detection safe (worst case: one extra invalidation on a genuine flip that raced the seed — harmless in dev).
Race fix 5aef816 — confirmed correct
learnOptimisticRouteTemplateFromPrefetch registers optimisticRouteTemplateSources synchronously and gates the instantShell case on attachPrefetchInvalidationCallback(..., options.entry) returning true; the expectedEntry identity guard (navigation.ts:582-590) bails when the cache entry was replaced. Instant templates are keyed by concreteHrefKey (app-optimistic-routing.ts:396-398), so they are URL-specific and won't leak across param values; lookup tries the concrete key first, then the route-level loading-shell fallback (:436-452). No issues.
Verification summary
- Read installed
next@16.2.7(9bd3c26a)instant-config.js/app-segment-config.jsand the e2e fixture directly. - Detection, exact-value matching (
runtimein,staticout), suspended-chunk handling, all-prop sanitization,connection()-only suspend, and the HMR/race fixes all match the installed source/fixture. tests/app-route-graph.test.ts(60),tests/build-report.test.ts(108),tests/app-optimistic-routing.test.ts(22),tests/link.test.ts(121),tests/prefetch-cache.test.ts(32) all pass locally.
No blocking issues. Recommend addressing the #3 comment nit at the author's discretion (non-blocking).
|
Review posted to PR #2374. SummaryI reviewed PR #2374 against the authoritative installed Next.js source ( No blocking issues found. Key findings:
All relevant test files (343 assertions) pass locally. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: instant route prefetch shells
Reviewed against the installed Next.js source in node_modules (next@16.2.7, build commit 9bd3c26a) — the deploy target — and including the newest commit 1e89c91 ("align programmatic instant prefetch") that prior bonk rounds had not seen.
Prior findings — status
#1 export contract — correct, not a divergence. I read dist/esm/server/app-render/instant-validation/instant-config.js directly. In the installed target the gate is:
const instantConfig = layoutOrPageMod ? layoutOrPageMod.unstable_instant : undefined;
const hasRuntimePrefetch =
instantConfig && typeof instantConfig === 'object' ? instantConfig.prefetch === 'runtime' : false;unstable_instant.prefetch === 'runtime' is exactly what routeModuleHasInstant (app-route-graph.ts:199-211) checks via hasNamedExportObjectStringProperty. The earlier CHANGES_REQUESTED rounds verified against canary (prefetch === 'allow-runtime'), which is a newer API that does not exist in the pinned/installed version. Not a blocker.
#1b truthiness over-matching — fixed. Exact literal match; prefetch: 'static' is correctly excluded (build-report.test.ts).
#2 sanitize on suspended chunks — resolved. SUSPENDED_LAZY_STATUSES returns OptimisticRouteSegment before _init, plus a try/catch fallback (app-optimistic-routing.ts:307-333).
#3 connection()-only suspend — non-blocking, matches fixture. cookies() content is intentionally treated as cacheable. A one-line comment is still worth adding.
New in commit 1e89c91 — incomplete Link/router.prefetch alignment (non-blocking)
The commit extracts resolveAppRoutePrefetchPolicy into client/app-route-prefetch-policy.ts and routes both <Link> and router.prefetch() through it. For instant routes the two paths now agree (prefetchInstantShell: true → INSTANT_SHELL in both), and the added test covers that — good.
But the alignment is incomplete for non-instant dynamic routes without a loading shell, where the policy returns { cacheForNavigation: false, prefetchSuspenseShell: true, optimisticRouteShell(implicit): false }. The two entry points then diverge:
<Link>(link.tsx:441-451,:532-536): treats!cacheForNavigationas an optimistic-route-shell prefetch — render mode is overridden toAPP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL, the entry is markedoptimisticRouteShell: true, and it early-returns wheninterceptionContext !== null(link.tsx:442).router.prefetch()(navigation.ts:1937-1969): sendsAPP_RSC_RENDER_MODE_PREFETCH_SUSPENSE_SHELL, never setsoptimisticRouteShell, and has no interception-context guard.
Consequences for that route class:
- The two paths request different server render modes for the identical URL, producing different RSC payloads / cache entries.
learnOptimisticRouteTemplateFromPrefetchgatesallowLoadingShellonoptimisticRouteShell === true || instantShell === true(app-browser-entry.ts:450-451). The Link-warmed entry contributes a loading-shell optimistic template; therouter.prefetch-warmed entry does not.- Missing the
interceptionContext !== nullguard meansrouter.prefetchcan warm an interception-keyed entry that the Link path deliberately skips.
This is newly introduced: before this commit router.prefetch() always did a plain full prefetch (no render mode, cacheForNavigation: true). The commit's stated goal — aligning instant prefetch — is achieved, but the shared policy now also drives the non-instant suspense/optimistic-shell branch on the router.prefetch path without replicating the Link path's optimistic-shell handling. Only the instant case is tested.
Recommend either (a) replicating the Link path's optimistic-route-shell handling (loading-shell render mode, optimisticRouteShell: true, the interceptionContext early-return) in router.prefetch, or (b) deliberately scoping router.prefetch to instant + plain-cacheable prefetch and documenting why the optimistic-shell branch is Link-only. Add a router.prefetch test for a non-instant dynamic route either way.
Verification
- Read installed
next@16.2.7instant-config.jsdirectly to confirm the export contract. tests/prefetch-cache.test.ts(33) andtests/link.test.ts(121) pass locally.
Non-blocking overall: the headline instant-prefetch feature is correct against the deploy target. The flagged item is the unfinished non-instant alignment in the latest commit.
|
Posted a review to PR #2374. Summary of what I found: Verified against the deploy target (
|
|
/bigbonk review for issues Please review exact head f5ebe12, including the final fix that preserves full non-instant router.prefetch caching. |
There was a problem hiding this comment.
Review: instant route prefetch shells (head f5ebe12)
Reviewed against the installed Next.js source (next@16.2.7, the deploy target) in node_modules, with focus on the final fix f5ebe12 ("preserve full programmatic prefetches"). No blocking issues.
Final fix f5ebe12 — correct; resolves the prior round's non-instant router.prefetch concern
The previous round flagged that commit 1e89c91 routed router.prefetch() through the shared policy and sent APP_RSC_RENDER_MODE_PREFETCH_SUSPENSE_SHELL with cacheForNavigation driven by the policy — diverging from the <Link> path and from the pre-PR behavior. f5ebe12 resolves this by scoping router.prefetch() to:
- instant routes →
APP_RSC_RENDER_MODE_PREFETCH_INSTANT_SHELL,instantShell: true,cacheForNavigation: true(the server's partial-shell header self-downgrades the entry viaprefetchRscResponsenavigation.ts:762-765, so this is self-correcting); - everything else → no render mode (plain full prefetch),
cacheForNavigation: true.
I confirmed this exactly restores the pre-PR baseline (2a4681d:navigation.ts): before this PR, router.prefetch() had no render-mode header and called prefetchRscResponse with no behavior object (defaulting to cacheForNavigation: true). So the suspense-shell branch introduced in 1e89c91 is removed, and the non-instant path is unchanged from main. This is option (b) from the prior review (deliberately scope router.prefetch to instant + plain-cacheable) and is the right call. The new test tests/prefetch-cache.test.ts:579 ("keeps ordinary router.prefetch responses available for navigation") asserts cacheForNavigation: true, instantShell: false. Passes locally.
Prior findings — status
- #1 export contract — correct, not a divergence. Read installed
dist/esm/server/app-render/instant-validation/instant-config.jsdirectly: the gate isinstantConfig.prefetch === 'runtime'onlayoutOrPageMod.unstable_instant. That is exactly whatrouteModuleHasInstant(app-route-graph.ts:199-211) checks viahasNamedExportObjectStringProperty(code, "unstable_instant", "prefetch", "runtime"). The earlier CHANGES_REQUESTED rounds verified against canary (prefetch === 'allow-runtime'), a newer API absent from the pinned/installed16.2.7. Not a blocker. - #1b truthiness over-matching — fixed.
hasNamedExportObjectStringProperty(report.ts:160-190) does an exact literal match (value.value === expectedValue), unwraps TS wrappers, requires a non-computed key, and handles aliased re-exports.prefetch: 'static'→false(tested). - #2 sanitize on suspended chunks — resolved.
SUSPENDED_LAZY_STATUSESreturnsOptimisticRouteSegmentbefore calling_init, plus a try/catch fallback that converts a thrown chunk/thenable into the suspense trigger (app-optimistic-routing.ts:307-332). Recurses into all element props, not justchildren(:335-342). - #3
connection()-only suspend — non-blocking, matches fixture.cookies()content is intentionally treated as cacheable; abort scheduling is lazy. A one-line comment documenting thecookies()-cacheable intent would still help future readers.
Non-blocking observations
-
Pre-existing parity note (not introduced by this PR): installed Next.js
router.prefetch()defaults toPrefetchKind.AUTO→FetchStrategy.PPR, which for a non-PPR route falls back toLoadingBoundary(renders only up to the first loading boundary) — i.e. a partial prefetch, not Full (app-router-instance.js:273,segment-cache/scheduler.js:444,types.js:28-37; note also the upstream comment "We don't currently offer a way to issue a runtime prefetch viarouter.prefetch()"). vinext'srouter.prefetch()does a full prefetch instead. This divergence predates this PR (it was the main baseline) andf5ebe12deliberately preserves it rather than introducing it, so it is out of scope here — but it's worth a tracked follow-up if Segment-Cache-style partial programmatic prefetch is ever in scope. -
Test coverage of the dynamic-without-shell case: the new
router.prefetchtest usesisDynamic: false, canPrefetchLoadingShell: true. The fix setscacheForNavigation: trueunconditionally, so behavior is identical for a dynamic-without-loading-shell route, but a test withisDynamic: true(the route class the prior round specifically called out) would lock in the intended "full prefetch" behavior more directly. Minor.
Verification
- Read installed
next@16.2.7instant-config.jsandapp-router-instance.js/ segment-cachetypes.js+scheduler.jsto confirm the export contract androuter.prefetchsemantics. - Confirmed
f5ebe12restores the2a4681d(pre-PR)router.prefetchfull-prefetch behavior for non-instant routes. vp checkclean onnavigation.ts+app-route-prefetch-policy.ts.- Tests pass locally:
prefetch-cache(34),link(121),app-route-graph(61),build-report(108),app-optimistic-routing(22).
No blocking issues. The final fix correctly preserves full non-instant router.prefetch caching while keeping the instant-shell path intact. Recommend the #3 comment nit and the two non-blocking items at the author's discretion.
|
Posted a review to PR #2374 at head No blocking issues. Final fix Prior findings verified resolved:
Two non-blocking notes: (1) a pre-existing parity gap — Next.js |
|
Deploy-suite run 28478866791 / job 84413308650 triage: I think this PR already covers the instant route full-prefetch failures: test/e2e/app-dir/prefetch-true-instant/prefetch-true-instant.test.ts (2 failures around dynamic content exclusion when unstable_instant is enabled). I am skipping a duplicate backlog fix for that item and leaving this note so the overlap is visible. |
|
Deploy-suite run 28478866791 / job 84413308650 backlog check: this PR appears to cover the |
|
Backlog check for deploy-suite run 28478866791 / report job 84413308650: this PR likely covers the remaining non-cacheComponents
Those line up with this PR's instant-route shell handling and its exact upstream target |
1 participant
Summary
unstable_instantexports on pages and ancestor layoutsprefetch={true}to an instant runtime shell for those routesconnection()content until the navigation response arrivesNext.js parity
Ported against:
test/e2e/app-dir/prefetch-true-instant/prefetch-true-instant.test.tsValidation
vp checkon all 19 touched source/test filesvinext,cloudflareStack
Depends on #2364 (
codex/fix-suspense-derived-prefetch) for user-authored Suspense shell prefetching.