fix(app-router): support cached navigation stages

[james-elicx]

Summary

• learn and embed static/runtime cached-navigation stages from the authoritative server response
• preserve delayed request APIs, cache-component boundaries, interception routes, and visited-response reuse across navigation and initial HTML
• validate Next-compatible unstable_instant config at build time and when dynamically resolving active route/intercept modules

Validation

• vp test run across all 17 affected suites: 1,228/1,228 passed
• vp check
• vp run vinext#build
• vp run knip
• exact Next.js E2E, twice with concurrency 1: 9/9 passed both runs

NEXT_TEST_CONCURRENCY=1 REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/cached-navigations/cached-navigations.test.ts

Independent cumulative review converged with no findings on the committed tree.

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2870

npm i https://pkg.pr.new/create-vinext-app@2870

npm i https://pkg.pr.new/@vinext/types@2870

npm i https://pkg.pr.new/vinext@2870

commit: 851c022

[ask-bonk]

UnknownError: ProviderInitError

github run

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[github-actions]

Performance benchmarks

Compared 851c022 against base d558d62 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 3 regressed · 3 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	135.2 KB	136.5 KB	⚫ +1.0%
Client entry size (gzip)	vinext	122.7 KB	124.0 KB	⚫ +1.1%
Dev server cold start	vinext	2.28 s	2.32 s	🔴 +1.7%
Production build time	vinext	2.39 s	2.42 s	⚫ +1.3%
RSC entry closure size (gzip)	vinext	114.2 KB	118.9 KB	🔴 +4.1%
Server bundle size (gzip)	vinext	193.0 KB	197.9 KB	🔴 +2.5%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Reviewed the full 46-file diff, ran the affected unit suites locally (all green: app-segment-config, app-segment-config-validation, app-cached-navigation-runtime, app-rsc-render-mode, app-page-dispatch, app-page-element-builder, app-page-route-wiring, app-render-dependency, app-visited-response-cache, ppr-fallback-shell, thenable-params, fetch-cache, app-browser-entry, app-page-render), and cross-checked the config behavior against Next.js canary source.

Overall this is a large, carefully-tested change with good helper extraction and thorough unit coverage. The unstable_instant validation, throw-on-cachedNavigations-without-cacheComponents, and the staged wire encoding all match Next.js behavior. A few findings below.

Medium — cachedNavigations default diverges from Next.js

config/next-config.ts:1931

cachedNavigations: config.cacheComponents === true && experimental?.cachedNavigations === true,

Next.js canary auto-enables cachedNavigations whenever cacheComponents is enabled, unless the user explicitly sets it to false (see packages/next/src/server/config.ts, the "Enable cachedNavigations by default when cacheComponents is enabled" block). So a user config of just { cacheComponents: true } yields cachedNavigations: true in Next.js but false in vinext.

To match, the resolution should be roughly: enable when cacheComponents === true and experimental.cachedNavigations !== false (i.e. default-on for cache-components apps, opt-out only). The throw-on-true-without-cacheComponents is correct and matches Next exactly — this is only about the default-on case. If keeping the conservative default-off is deliberate while the feature stabilizes, please document it as a known parity gap (the surrounding appShells co-flag comment already tracks similar gaps).

Low — three exported cache helpers are production-dead

server/app-visited-response-cache.ts: canStoreCachedNavigationStage, scheduleCachedNavigationStageFills, and cancelPendingCachedNavigationStageFill are exported and unit-tested but have no production callers — the browser entry (publishEmbeddedCachedNavigationStages) inlines its own generation/fill logic instead. knip won't flag them because the tests import them. Either wire them into the runtime path they were designed for, or drop them + their tests to avoid untested-in-production surface and confusion about which code path is authoritative.

Low (perf note) — eager request-API value computation in the static stage

shims/headers.ts:988 and :1025 now compute _getReadonlyHeaders(...) / _getMutableCookies(...) before the fallback-shell check, whereas previously the suspending promise short-circuited that work. In the static stage the computed value is discarded (the API hangs), so this adds a header-copy/cookie-wrap allocation on the fallback-shell path. The getters are memoized on ctx and side-effect-free, so it's only a minor allocation, but per AGENTS.md's "keep the common request path lightweight" guidance it'd be cleaner to only compute the value on the branch that actually returns it.

Nits / confirmations (no action required)

• connection() in shims/server.ts uses a second await import("./ppr-fallback-shell.js") — consistent with the existing await import("./headers.js") in the same function, so fine.
• app-page-render.ts:938 changing createBufferedRscStream(false) → createBufferedRscStream(options.isRscRequest) correctly terminates the buffered RSC stream for static/runtime-stage RSC requests; verified this only triggers when pprFallbackShellRsc is set.
• getCachedNavigationFallbackParamNames prefix-pattern accumulation correctly mirrors the staticParamsMap keying used by app-prerender-endpoints.ts, and bare-vs-catch-all param name matching is consistent (both sides use bare names).
• Segment-config validation is duplicated (build-time transform in plugins/app-segment-config-validation.ts + runtime in app-segment-config.ts/app-page-dispatch.ts) — this is intentional and documented in the plugin's JSDoc (build-time avoids eager route imports; runtime covers dynamic/cross-file cases).

[ask-bonk]

Posted the review to PR #2870.

Summary

I reviewed the full 46-file diff for PR #2870, ran all affected unit suites locally (all passing), and cross-checked config behavior against Next.js canary source.

Findings posted:

1. Medium — cachedNavigations default diverges from Next.js (next-config.ts:1931): Next.js canary auto-enables cachedNavigations whenever cacheComponents is on (opt-out only), but vinext requires explicit experimental.cachedNavigations === true. A { cacheComponents: true } config yields true in Next.js vs false in vinext. The throw-on-missing-cacheComponents is correct and matches Next exactly; only the default-on case diverges. Suggested either matching the default-on-with-opt-out behavior or documenting it as a known parity gap.

2. Low — three production-dead exports in app-visited-response-cache.ts (canStoreCachedNavigationStage, scheduleCachedNavigationStageFills, cancelPendingCachedNavigationStageFill): exported and unit-tested but never called from runtime code (the browser entry inlines its own fill logic). knip misses them because tests import them.

3. Low (perf) — eager request-API value computation in shims/headers.ts: headers()/cookies() now compute readonly values before the fallback-shell check, allocating work that's discarded in the static stage.

Plus several confirmations that were verified as correct and need no action (the connection() dynamic import, the buffered-RSC-stream termination change, the fallback-param prefix logic, and the intentional build-time/runtime validation duplication).

The PR is well-tested and the core mechanics (staged wire encoding, unstable_instant validation, render-mode plumbing) look sound. The main item worth the author's attention before merge is the cachedNavigations default parity gap.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Re-reviewed the full tree after commit 9a295ef ("align cached navigation behavior"). That commit resolves all three findings from the prior review round:

• cachedNavigations now defaults on with cacheComponents (experimental?.cachedNavigations !== false) — matches Next.
• The three production-dead exports (canStoreCachedNavigationStage, scheduleCachedNavigationStageFills, cancelPendingCachedNavigationStageFill) and their tests were removed.
• headers() / cookies() now compute the readonly value lazily via getReadonlyHeaders / getCookieStore, so the fallback-shell (static) stage no longer allocates a discarded header copy / cookie wrapper.

Ran the affected unit suites locally — all green:
next-config, app-segment-config, app-segment-config-validation, app-cached-navigation-runtime, ppr-fallback-shell, app-render-dependency, app-page-render, app-page-route-wiring, app-rsc-render-mode, app-visited-response-cache, app-browser-entry, app-page-dispatch, app-page-element-builder, app-rsc-handler, thenable-params, fetch-cache (1,148 tests). vp lint clean on the changed source.

A few new low-severity findings below.

Low — hasRuntimeCacheTask is write-only state

shims/ppr-fallback-shell.ts:23 / :269 / :415 / :587

hasRuntimeCacheTask is declared on PprFallbackShellState, initialized to false, set to true in startTrackedTask (runtime stage + private cache), and reset in both createPprFallbackShellState and preparePprFallbackShellFinalRender — but it is never read by any production logic. Its only reads are test assertions (tests/ppr-fallback-shell.test.ts:491, tests/app-page-route-wiring.test.ts:3191). knip won't flag it because it's a struct field, not an export. Either wire it into the abort/ready gates it appears intended for, or drop the field (and the tests that assert on it) to avoid dead state that implies a behavior that doesn't exist.

Low — appShells co-flag warning no longer reflects the resolved default

config/next-config.ts:1761

The appShells validation still lists experimental.cachedNavigations as a "missing co-flag" whenever experimental?.cachedNavigations !== true. Now that cachedNavigations auto-enables with cacheComponents, a config of { cacheComponents: true, experimental: { appShells: true, prefetchInlining, varyParams, optimisticRouting } } resolves to cachedNavigations: true but the warning still tells the user it's missing. Cosmetic only (appShells is plumbing-only/no-op in vinext per #1614), but the warning text is now misleading — consider checking the resolved value instead of the raw experimental flag.

Nit — connection() fallback-shell path now bypasses suspendConnectionProbe()

shims/server.ts:1280-1289

The new delayPprFallbackShellRequestApi("connection", …) returns non-null whenever a fallback-shell state is present (connection always suspends), and the function returns early — so suspendConnectionProbe() is skipped in that case. For the pre-existing non-cached-navigation PPR path this swaps suspendConnectionProbe()'s probe.interrupt() + probe.pending for a plain shell suspense promise. If a connection probe and a fallback-shell state can ever be active simultaneously, the probe's interrupt() would no longer fire. I couldn't construct a case where both coexist and the suites are green, so flagging for author confirmation rather than as a defect — worth a one-line check that probe-time and fallback-shell suspension can't overlap.

Confirmations (no action)

• wrapCachedNavigationRuntimeValue correctly leaves functions, typed arrays, Dates, blobs, Maps/Sets-as-themselves, and non-plain objects untouched (only plain RSC objects/arrays/promises are deep-wrapped), preserving RSC value kinds — the seen WeakMap guards cycles including async promise chains.
• extractStaticExportValue correctly resolves export { local as unstable_instant } via the local binding and bails to supported: false on re-exports (statement.source !== null) and unsupported expressions — the build-time validator degrades to the runtime validator as documented.
• The throw-on-cachedNavigations-without-cacheComponents (previously a warning) now matches Next's hard error.
• Build-time (plugins/app-segment-config-validation.ts) + runtime (app-segment-config.ts) unstable_instant validation duplication is intentional and documented.

Overall the PR is in good shape and the prior-round findings are cleanly addressed. The items above are all low/nit — none block merge.

[ask-bonk]

Posted the review to PR #2870.

Summary

I re-reviewed the full tree after the new commit 9a295ef ("align cached navigation behavior"), which had landed since the prior bonk round. That commit cleanly resolves all three prior findings:

1. cachedNavigations now defaults on with cacheComponents (!== false) — matches Next.
2. The three production-dead exports + their tests were removed.
3. headers()/cookies() now compute the readonly value lazily, avoiding the discarded allocation on the static stage.

I ran all 16 affected unit suites locally (1,148 tests, all green) and vp lint (clean on changed source), then cross-checked config behavior and the RSC value-wrapping logic.

New findings posted (all low/nit — none block merge):

1. Low — hasRuntimeCacheTask is write-only state (ppr-fallback-shell.ts): set and reset but never read by production logic; only read in tests. Either wire it into the abort/ready gates it appears intended for, or drop it.

2. Low — appShells co-flag warning is now misleading (next-config.ts:1761): it still reports experimental.cachedNavigations as missing based on the raw flag, even though the resolved config now auto-enables it. Cosmetic (appShells is a no-op), but the warning text is stale.

3. Nit — connection() fallback-shell path bypasses suspendConnectionProbe() (shims/server.ts): the new delay path returns early before the probe interrupt; flagged for author confirmation that probe-time and fallback-shell suspension can't overlap (couldn't construct a failing case).

Plus confirmations that the RSC value wrapper preserves value kinds and cycles, the static-export extractor handles aliased/re-exported bindings correctly, and the throw + validation-duplication both match Next intentionally.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Re-reviewed the full tree at 401e9d5 ("address cached navigation review feedback"). That commit cleanly resolves all three findings from the previous round:

• connection() probe ordering — suspendConnectionProbe() now runs before the fallback-shell delay in shims/server.ts:1280, so the probe's interrupt() fires again. Returns null when no probe is active, so the fallback-shell branch is still reached in the normal case. New test at tests/shims.test.ts:5585 covers it. ✅
• hasRuntimeCacheTask write-only state — the field and all its assignments/tests were removed. ✅
• appShells co-flag warning — now checks the resolved cachedNavigations value (next-config.ts:1763) instead of the raw experimental flag, so the warning no longer misfires when cacheComponents auto-enables it. ✅

Ran the affected unit suites locally (fetch-cache green, 146 tests). A few new low/nit findings below — none block merge.

Low (perf) — eager error-message allocation on every uncached fetch()

shims/fetch-cache.ts:637-646

delayUncachedFetchForStaticStage builds its expression argument eagerly:

return delayPprFallbackShellRequestApi(
  "fetch",
  `fetch(${JSON.stringify(getFetchObservationUrl(input))})`,
  fetchValue,
);

But delayPprFallbackShellRequestApi returns null immediately when no fallback-shell state is active (ppr-fallback-shell.ts:294-298) — the overwhelmingly common request path. The expression string is only consumed inside createPprFallbackShellSuspensePromiseForState to build an error message, which is never reached in the null case. So the getFetchObservationUrl(input) read + JSON.stringify + template concatenation is allocated and discarded on every uncached fetch() call (5 call sites: lines 1131, 1149, 1175, 1203, 1250).

Contrast with headers()/cookies(), which pass a static string literal ("`headers()`") — no per-call allocation — and whose value computation was already made lazy in the prior review round for exactly this reason (AGENTS.md "keep the common request path lightweight" / "avoid repeated hot-path work"). Suggest either making expression a lazy () => string in delayPprFallbackShellRequestApi, or gating the string build behind a cheap getPprFallbackShellState() check inside delayUncachedFetchForStaticStage.

Low — un-drained runtimeStage.readable on early return

server/app-browser-entry.ts:905-998

publishEmbeddedCachedNavigationStages can return before it ever touches data.runtimeStage.readable:

• lines 921-927 — static stage invalid (byteLength <= 0, oversized) or generation stale
• line 933 — generation stale after static decode

On those paths the runtime stage's ReadableStream (data.runtimeStage.readable, consumed at line 964-965 via new Response(...).arrayBuffer()) is never read and never cancelled. A client-side ReadableStream backed by the RSC transport that is neither consumed nor cancel()-ed holds the underlying reader/transport open. Consider data.runtimeStage?.readable.cancel().catch(() => {}) on the early-return branches (only when data.runtimeStage !== null) so the discarded refinement stream is released.

Nit — inconsistent __NEXT_EXPERIMENTAL_CACHED_NAVIGATIONS detection idiom

server/app-browser-entry.ts:207 uses !!process.env.__NEXT_EXPERIMENTAL_CACHED_NAVIGATIONS while server/app-rsc-handler.ts:136 uses String(...) === "true" || ... === "1". Both are correct today because the DefinePlugin substitutes a boolean literal (index.ts:2393 → JSON.stringify(nextConfig.cachedNavigations)), so !!false === false and String(false) === "false" both evaluate to disabled. But the two idioms disagree if the value ever arrives as a raw string: !!"false" is true while String("false") === "true" is false. Not a live bug (the define guarantees a boolean); flagging only because the divergent idioms invite a future regression if the substitution source changes. Consider standardizing on the boolean-safe form in both places.

Confirmations (no action)

• extractStaticExportValue / extractStaticJsonValueWithBindings (build/report.ts): binding resolution isn't lost in the extractStaticJsonValue fallback — arrays/objects/identifiers are handled by the binding-aware recursion first, so the fallback is only reached for literals/template literals. The seen set correctly guards identifier cycles.
• wrapCachedNavigationRuntimeValue (app-render-dependency.tsx:104): the seen.get(value) !== undefined "was-seen" check is safe because stored values are always freshly-constructed objects (array/map/set/object/promise), never undefined. Typed arrays / Dates / non-plain objects are correctly passed through untouched.
• getCachedNavigationFallbackParamNames (app-rsc-handler.ts): the prefix-accumulation filter (once a param is unknown, all later params are fallback) correctly mirrors nested-dynamic-segment staticParamsMap keying; an empty nextCandidates degrades conservatively to "all params unknown", which is safe.
• The four remaining new exports in app-visited-response-cache.ts (startAuthoritativeCachedNavigationResponse, createCachedNavigationStageCacheKey, isCachedNavigationStagePairFresh, canPublishCachedNavigationRuntimeStage) all have production callers in app-browser-entry.ts — no dead surface.
• cachedNavigations default resolution (next-config.ts:1741) now defaults-on with cacheComponents (opt-out via !== false), matching Next canary.

Overall the PR is in good shape and the prior-round findings are all cleanly addressed. The items above are low/nit only.

[ask-bonk]

Posted the review to PR #2870.

Summary

I re-reviewed the full tree at 401e9d5. That commit cleanly resolves all three prior-round findings (connection() probe ordering, hasRuntimeCacheTask dead state, and the appShells co-flag warning). I ran the affected unit suites locally (green) and cross-checked the new logic against the design intent documented in the diff.

New findings posted (all low/nit — none block merge):

1. Low (perf) — eager error-message allocation on every uncached fetch() (shims/fetch-cache.ts:637): delayUncachedFetchForStaticStage builds `fetch(${JSON.stringify(getFetchObservationUrl(input))})` eagerly, but delayPprFallbackShellRequestApi returns null early in the common no-fallback-shell case, discarding the string. This is on the hot uncached-fetch path (5 call sites) and contradicts the same "keep the common request path lightweight" guidance the prior round applied to headers()/cookies(). Suggested a lazy expression or a cheap getPprFallbackShellState() gate.

2. Low — un-drained runtimeStage.readable on early return (app-browser-entry.ts:905): publishEmbeddedCachedNavigationStages returns before consuming/cancelling data.runtimeStage.readable on the invalid-static-stage and stale-generation paths, leaving the RSC-transport-backed stream open. Suggested cancel().catch(() => {}) on those branches.

3. Nit — inconsistent env-var detection idiom (app-browser-entry.ts:207 !! vs app-rsc-handler.ts:136 string compare): both correct today because the DefinePlugin substitutes a boolean literal, but the idioms disagree if the value ever arrives as a raw string — a latent regression risk worth standardizing.

Plus confirmations that the static-export extractor's binding resolution, the RSC value wrapper's cycle/kind handling, the fallback-param prefix accumulation, and the remaining new exports (all have production callers) are all correct.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

UnknownError: ProviderInitError

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Re-reviewed the full tree at f4d27cc ("clean up cached navigation stages"). The last two /bigbonk runs failed with ProviderInitError before reviewing this commit, so this is the first review of it. It cleanly resolves all three findings from the 401e9d5 round:

• Un-drained runtimeStage.readable on early return (app-browser-entry.ts:918-1004) — publishEmbeddedCachedNavigationStages now tracks runtimeStageConsumed (init true when there's no runtime stage, flipped true right before the arrayBuffer() read) and cancels the discarded stream in a .finally() via the new cancelCachedNavigationRuntimeStage helper. Covered by tests/app-visited-response-cache.test.ts. ✅
• Eager error-message allocation on every uncached fetch() (fetch-cache.ts:643) — the expression arg is now a lazy () => ... thunk; delayPprFallbackShellRequestApi only invokes it (ppr-fallback-shell.ts:299-302) after both early returns, so the common no-fallback-shell path returns null without building the string. New guard test at tests/ppr-fallback-shell.test.ts:33. ✅
• Inconsistent env-var idiom — app-browser-entry.ts:206-207 now uses the same === "true" || === "1" form as app-rsc-handler.ts:136-137, so the two no longer diverge if the define ever substitutes a raw string. ✅

Verified locally at f4d27cc:

• Affected unit suites green: ppr-fallback-shell, app-visited-response-cache, fetch-cache, thenable-params, app-rsc-handler, app-render-dependency, app-browser-entry, app-segment-config, app-segment-config-validation, next-config, app-cached-navigation-runtime, app-rsc-render-mode (896 tests).
• vp lint clean (full run, exit 0).

Also confirmed the two earlier-round config findings remain correctly fixed: cachedNavigations defaults on with cacheComponents and opts out only on !== false (next-config.ts:1741), and the appShells co-flag warning checks the resolved value (next-config.ts:1763).

Nit — runtimeStageConsumed is set before the read starts, not after it completes

server/app-browser-entry.ts:967-969

runtimeStageConsumed = true is assigned immediately before new Response(data.runtimeStage.readable).arrayBuffer(), so if that read rejects mid-stream, the .finally() (line 1002) sees consumed === true and skips the cancel. In practice a rejected/errored ReadableStream read already tears down the stream, so an explicit cancel() would be a no-op — this is not a leak. Flagging only for precision: the flag semantically means "we started consuming," not "we finished." No action needed unless you want the name/comment to reflect that.

Confirmations (no action)

• wrapCachedNavigationRuntimeValue (app-render-dependency.tsx:104) — functions fall through to the !isPlainRscObject branch and pass untouched; typed arrays/Dates/blobs/Maps/Sets preserve their RSC value kinds; the seen WeakMap guards cycles including async promise chains. Stored refinements are always freshly-constructed non-undefined objects, so the existing !== undefined short-circuit is safe.
• getCachedNavigationFallbackParamNames (app-rsc-handler.ts:217) — the prefix-accumulation filter (once a param is unknown, all later params are fallback) correctly mirrors nested-dynamic staticParamsMap keying; non-array generateStaticParams output bails conservatively to "all params unknown."
• getRootParam (shims/root-params.ts:79-94) — the navigation-stage delay and static-stage suspension are both gated on fallbackParamNames.has(name), consistent with the headers()/cookies()/searchParams shims.
• extractStaticExportValue / extractStaticJsonValueWithBindings (build/report.ts) — resolves export { local as unstable_instant } via the local binding, bails to supported: false on re-exports (statement.source !== null) and unsupported expressions, and guards identifier cycles with the seen Set; only const bindings are trusted.

Overall the PR is in good shape — all prior-round findings across three rounds are cleanly addressed, and I found no blocking issues on this tree. The single item above is a nit.

[ask-bonk]

Posted the review to PR #2870.

Summary

The last two /bigbonk runs failed with ProviderInitError before they could review commit f4d27cc ("clean up cached navigation stages"), so this was the first review of that commit. I reviewed the full tree at f4d27cc, ran the affected unit suites (896 tests, all green), and ran vp lint (clean).

f4d27cc cleanly resolves all three findings from the 401e9d5 round:

1. Un-drained runtimeStage.readable — now tracked via runtimeStageConsumed and cancelled in a .finally() via the new cancelCachedNavigationRuntimeStage helper (with a test).
2. Eager error-message allocation on every uncached fetch() — expression is now a lazy thunk only invoked after the early-return guards (with a test).
3. Inconsistent env-var idiom — both call sites now use the same boolean-safe === "true" || === "1" form.

I also confirmed the two earlier-round config fixes are still correct (cachedNavigations default-on with opt-out, and the appShells warning checking the resolved value).

Findings this round: one nit only — runtimeStageConsumed is set before the arrayBuffer() read starts rather than after it completes, so a mid-stream read rejection skips the explicit cancel(). Not a leak (a rejected ReadableStream read already tears down the stream); flagged only for naming/comment precision.

Plus confirmations that the RSC value wrapper, fallback-param prefix logic, getRootParam gating, and the static-export extractor are all correct. No blocking issues on this tree.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Re-reviewed the full tree at be20261 ("clarify cached stage read state"). This is the first review of the commit that landed since the previous /bigbonk round (f4d27cc). It resolves the single nit from that round:

• runtimeStageConsumed → runtimeStageReadStarted (server/app-browser-entry.ts:921/967/1003) — the flag is renamed so it now reads as "we started the runtime-stage read," which matches its actual semantics (set immediately before the arrayBuffer() read, gating the discarded-stream cancel() in .finally()). Pure clarity rename, no behavior change. ✅

Verified locally at be20261:

• Affected unit suites green: ppr-fallback-shell, app-visited-response-cache, app-page-render, app-render-dependency, app-segment-config, app-segment-config-validation, app-cached-navigation-runtime, next-config, app-rsc-render-mode, app-page-dispatch, app-page-element-builder, app-page-route-wiring, app-rsc-handler, thenable-params, fetch-cache, shims (2,223 tests).
• vp lint clean (exit 0).

Also confirmed all prior-round fixes remain intact: lazy expression thunk on the uncached-fetch() path (fetch-cache.ts:643), the boolean-safe === "true" || === "1" env-var idiom at both call sites (app-browser-entry.ts:206, app-rsc-handler.ts:136), connection() running suspendConnectionProbe() before the fallback-shell delay (shims/server.ts:1280), cachedNavigations default-on with opt-out (next-config.ts:1741), and the appShells warning checking the resolved value (next-config.ts:1763).

Low (parity) — unstable_instant schema targets a pre-unstable_samples Next.js snapshot

server/app-segment-config.ts:136 (isInstantConfig)

vinext validates unstable_instant as false or an object keyed { prefetch: "static" | "runtime", samples, from, unstable_disable*Validation }, and the whole runtime-stage detection (app-cached-navigation-runtime.ts:30 moduleUsesRuntimeCachedNavigations keys off instant.prefetch === "runtime"). Current Next.js canary's InstantConfigSchema no longer has a prefetch field inside the instant object at all — it is union([InstantConfigObjectSchema, literal(true), literal(false)]) where the object is { level?, unstable_samples? (min 1), unstable_from?, unstable_disable*Validation? } and runtime-ness is inferred from unstable_samples presence (segment-config schema in packages/next/src/build/segment-config/app/app-segment-config.ts).

So a config that current canary accepts ({ unstable_samples: [...] } or true) would be rejected by vinext's isInstantConfig, and vinext's { prefetch: "runtime" } shape is not a valid canary value. Given unstable_ churn this is a moving target, and the PR validated against a pinned local .nextjs-ref E2E snapshot whose shape matches the prefetch form — so this is very likely deliberate targeting of the Next.js version the E2E ran against, not an accidental divergence. Flagging only so it is a documented, deliberate choice: worth a short comment on isInstantConfig noting which Next.js unstable_instant revision it mirrors, since the whole feature's runtime/static branching depends on that shape.

Low (confirm) — sibling-intercept layout runtime-request-API chain is indexed by array order, applied by tree position

server/app-page-element-builder.ts:517 and :683

interceptLayoutRuntimeRequestApis = resolveCachedNavigationRuntimeModuleChain(opts.interceptLayouts, …) builds a monotonic cumulative chain over interceptLayouts in array order (each entry ORs in all prior entries' runtime-request-api flag). The sibling-intercept wrapping loop then consumes interceptLayoutRuntimeRequestApis[layoutIndex] while applying layouts in tree-position order (the sorted treePositions loop, :653-686). This is only correct if opts.interceptLayouts is already ordered outer→inner so that array index and nesting depth agree. If a deeper intercept layout can appear earlier in the array than a shallower one, its cumulative runtime flag would propagate to the wrong nesting level (a shallower layout would incorrectly inherit runtime-request-api opt-in from a deeper sibling). I couldn't construct a fixture where the array is out of tree order (the scanner appears to emit outer→inner, matching the pre-existing interceptLayoutSegments[index].length tree-position mapping) and all suites are green, so flagging for author confirmation rather than as a defect — a one-line note that interceptLayouts is guaranteed outer→inner would nail it down.

Confirmations (no action)

• wrapCachedNavigationRuntimeValue (app-render-dependency.tsx:104) — functions fall through the !isPlainRscObject branch untouched; typed arrays / Dates / blobs / Maps / Sets preserve their RSC value kinds; the seen WeakMap guards cycles (including async promise chains). Every seen.set stores a freshly-constructed non-undefined object, so the existing !== undefined short-circuit is safe.
• extractStaticExportValue / extractStaticJsonValueWithBindings (build/report.ts:176) — resolves export { local as unstable_instant } via the local const binding, bails to supported: false on re-exports (statement.source !== null, :209) and unsupported expressions, and guards identifier cycles with the seen Set; only const initializers are trusted.
• getCachedNavigationFallbackParamNames (app-rsc-handler.ts:217) — the prefix-accumulation filter (once a param is unknown, all later params are fallback) correctly mirrors nested-dynamic staticParamsMap keying; non-array generateStaticParams output bails conservatively to "all params unknown."
• getRootParam (shims/root-params.ts:79) — navigation-stage delay and static-stage suspension are both gated on fallbackParamNames.has(name), consistent with the headers()/cookies() shims.
• Body handling in renderAppPageLifecycle (app-page-render.ts:1032+) — rscResponse.body is consumed exactly once into responseWithStageHeaders; the completion-metadata branch reads responseWithStageHeaders.body (not the original) and rscResponse.body is not touched again downstream.
• Intercept-branch segment-config validation (app-page-dispatch.ts:924) omits isClientModule, consistent with resolveAppPageSegmentConfig (app-segment-config.ts:394) — both rely on the build-time transform for the client-module case, which is intentional and documented.
• The build-time validation transform (index.ts:1832) has native id + code hook filters and gates on page/layout/default files inside appDir, matching AGENTS.md's "filter Vite hooks before JS runs" guidance.

Overall the PR is in good shape — every finding across the four prior rounds is cleanly addressed and I found no blocking issues on this tree. The two items above are low-severity / confirm-only.

[ask-bonk]

Posted the review to PR #2870.

Summary

Reviewed the full 47-file diff at the current HEAD be20261 ("clarify cached stage read state") — the first /bigbonk review of the commit that landed after the previous round (f4d27cc). That commit is a pure clarity rename (runtimeStageConsumed → runtimeStageReadStarted) that resolves the single nit from the prior round; no behavior change.

I ran all 16 affected unit suites locally (2,223 tests, all green) and vp lint (clean, exit 0), and confirmed every prior-round fix remains intact (lazy fetch() expression thunk, boolean-safe env-var idiom at both sites, connection() probe ordering, cachedNavigations default-on-with-opt-out, resolved-value appShells warning).

New findings posted (both low / confirm-only — none block merge):

1. Low (parity) — unstable_instant schema (app-segment-config.ts:136): vinext validates the { prefetch: "static" | "runtime" } shape and keys runtime-stage detection off instant.prefetch === "runtime", but current Next.js canary has dropped prefetch from the instant object entirely (now unstable_samples/unstable_from + true/false). Almost certainly deliberate targeting of the pinned local E2E Next.js snapshot; flagged only to make it a documented choice (suggest a comment noting which unstable_instant revision it mirrors).

2. Low (confirm) — sibling-intercept layout runtime-request-API chain (app-page-element-builder.ts:517/683): the cumulative runtime-request-api chain is built in interceptLayouts array order but consumed while applying layouts in tree-position order — correct only if the array is guaranteed outer→inner. Couldn't construct a failing fixture (the scanner appears to emit outer→inner) and suites are green, so flagged for author confirmation.

Plus confirmations that the RSC value wrapper (cycles/value-kinds), the static-export extractor (aliased/re-exported bindings, const-only, cycle guard), the fallback-param prefix accumulation, getRootParam gating, single-consumption body handling in renderAppPageLifecycle, the intercept-branch validation, and the filtered build-time transform are all correct.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

UnknownError: ProviderInitError

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

UnknownError: ProviderInitError

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Reviewed the full 47-file tree at HEAD 0e6603e ("note cached navigation contracts") — the first /bigbonk review of the commit that landed after the previous round (be20261). The last two runs failed with ProviderInitError before reaching it.

0e6603e is a docs-only commit (two comments, no behavior change) that addresses the two low/confirm-only findings from the be20261 round:

• unstable_instant schema parity (server/app-segment-config.ts:135-136) — isInstantConfig now carries a comment: "Mirrors the unstable_instant revision in Next.js v16.2.6, the pinned deploy-suite oracle. Canary revisions may use a different unstable shape." This documents the deliberate { prefetch: "static" | "runtime" } targeting, exactly the "make it a documented choice" ask. ✅
• Sibling-intercept layout ordering (server/app-page-element-builder.ts:520-521) — the interceptLayoutRuntimeRequestApis fold now carries: "The route graph records intercept layouts outermost-to-innermost, and the generated manifest preserves that positional order alongside the segments." ✅

I independently verified the ordering claim rather than taking the comment at face value. It holds:

• interceptLayouts (ir.layoutPaths) is accumulated by collectInterceptingPages in routing/app-route-graph.ts:2798-2801, which recurses outer→inner and appends each level's own layout to the inherited ancestor list — producing [outermost, …, innermost]. The type documents the invariant at app-route-graph.ts:42.
• interceptLayoutSegments (ir.layoutSegments) is a direct .map() over layoutPaths (app-route-graph.ts:2842-2849), so it is positionally aligned by construction.
• entries/app-rsc-manifest.ts:268-293 emits both via positional moduleArray/lazyLoaderArray with no sort/reverse; app-page-dispatch.ts and app-rsc-entry.ts pass them through unchanged.
• The order-sensitive consumer — resolveCachedNavigationRuntimeModuleChain (app-cached-navigation-runtime.ts:38-47) — is a monotonic left-fold that requires index 0 to be the outermost ancestor, which is exactly what the pipeline guarantees. (The other consumer, the sibling-wrapping loop at app-page-element-builder.ts:636-690, is order-independent — it rebuilds ordering from interceptLayoutSegments[i].length — so it self-corrects regardless.)

Note: there is no interceptLayoutTreePositions array (only interceptLoadingTreePositions); layout tree position is derived at runtime from segment length. The comment's wording ("positional order alongside the segments") is accurate and does not imply otherwise.

Verified locally at 0e6603e:

• Affected suites green: app-segment-config, app-page-element-builder, thenable-params, ppr-fallback-shell, app-browser-entry, app-rsc-handler (562 + 454 tests across the runs, all pass).
• vp lint clean on the two changed source files.
• vp run vinext#build completes successfully.

Nit (no action) — getFallbackShellPromise re-fires the dynamic-boundary side effect on every fallback-key access

shims/thenable-params.ts:234-239 + :232-233 comment

The memoization comment states "Promise creation stays lazy so the dynamic-boundary side effects only fire on first actual fallback-key access." In practice the promise object is memoized (lines 251-256), but delayCachedNavigationValueForState (line 237) → markPprFallbackShellDynamicBoundaryForState (ppr-fallback-shell.ts:309) runs before the memoization check, so the side effect re-fires on every fallback-key access, not just the first. This is harmless because markPprFallbackShellDynamicBoundaryForState is idempotent (hasDynamicBoundary = true + an ignore-already-ignored-tasks loop), so it's not a defect — flagging only because the comment implies first-access-only semantics that the code doesn't actually provide. Consider tightening the comment to "the dynamic-boundary side effect is idempotent and re-applied on each fallback-key access" for precision.

Confirmations (no action)

• The runtimeStageReadStarted gate + .finally() cancel in publishEmbeddedCachedNavigationStages (app-browser-entry.ts:921/967/1003) correctly releases the discarded runtime-stage ReadableStream on all early-return branches; cancelCachedNavigationRuntimeStage (app-visited-response-cache.ts:35-40) null-guards and swallows cancel rejections.
• All prior-round fixes across the six earlier rounds remain intact: lazy fetch() expression thunk (fetch-cache.ts), boolean-safe env-var idiom at both sites, connection() probe ordering before the fallback-shell delay, cachedNavigations default-on-with-opt-out (next-config.ts:1741), and the resolved-value appShells warning (next-config.ts:1763).

Overall the PR is in good shape. Every finding across all seven review rounds is now cleanly addressed — the two remaining items from the last round are documented as deliberate choices, and I found no blocking issues on this tree. The single item above is a nit.

[ask-bonk]

Posted the review to PR #2870.

Summary

Reviewed the full 47-file tree at HEAD 0e6603e ("note cached navigation contracts") — the first /bigbonk review of that commit (the previous two runs failed with ProviderInitError). It's a docs-only commit that resolves the two remaining low/confirm-only findings from the prior round by adding documentation comments.

Verified:

• The new unstable_instant schema comment documents the deliberate { prefetch: "static" | "runtime" } targeting against the pinned Next.js v16.2.6 oracle. ✅
• The new intercept-layout ordering comment — I independently traced the pipeline (app-route-graph.ts:2798-2801 accumulation → app-rsc-manifest.ts positional emission → order-sensitive resolveCachedNavigationRuntimeModuleChain fold) and confirmed the outer→inner claim holds. ✅
• Ran the affected suites (all green), vp lint (clean), and vp run vinext#build (successful).

Findings posted: one nit only — the getFallbackShellPromise memoization comment in thenable-params.ts implies the dynamic-boundary side effect fires only on first fallback-key access, but it actually re-fires (idempotently) on every access. Not a defect; a comment-precision suggestion.

No blocking issues. Every finding across all seven review rounds is now cleanly addressed.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Reviewed the full 47-file tree at HEAD ed70871 ("clarify fallback param staging") — the first /bigbonk review of the commit that landed after the previous round (0e6603e).

ed70871 is a docs-only change (2 lines, no behavior change) that resolves the single nit from the 0e6603e round:

• getFallbackShellPromise memoization comment (shims/thenable-params.ts:232-233) — the comment previously stated the dynamic-boundary side effect "only fire[s] on first actual fallback-key access," which was inaccurate: delayCachedNavigationValueForState (line 237) → markPprFallbackShellDynamicBoundaryForState runs before the promise-memoization check, so the (idempotent) mark re-applies on every fallback-key access. The comment is now: "Hanging-promise creation stays lazy. Cached-navigation staging re-applies its idempotent dynamic-boundary mark on each fallback-key access." This matches the code exactly. ✅

Verified locally at ed70871:

• Affected unit suites green: thenable-params, app-cached-navigation-runtime, app-segment-config, app-browser-entry, app-visited-response-cache, ppr-fallback-shell, fetch-cache, next-config (712 tests, all pass).
• vp lint clean (exit 0).

I re-verified the code that comment describes: getFallbackShellPromise (thenable-params.ts:223-263) memoizes the hanging promise on fallbackShellPromise + re-derives only when the live abortController changes (warmup→final transition), while the dynamic-boundary mark is applied unconditionally through delayCachedNavigationValueForState. Since markPprFallbackShellDynamicBoundaryForState is idempotent (hasDynamicBoundary = true + ignore-already-ignored-tasks loop), re-application per access is a no-op — the revised wording is precise.

Confirmations (no action)

• All prior-round fixes across the eight earlier rounds remain intact:
  • cachedNavigations default-on with cacheComponents, opt-out only on !== false (next-config.ts:1741); throw-on-true-without-cacheComponents (next-config.ts:1824); appShells warning checks the resolved value (next-config.ts:1763).
  • Lazy fetch() expression thunk (fetch-cache.ts:643) — delayPprFallbackShellRequestApi only invokes it (ppr-fallback-shell.ts:301) after both the navigation-stage return and the null early return, so the common no-fallback-shell path never builds the string.
  • runtimeStageReadStarted gate + .finally() cancel (app-browser-entry.ts:921/967/1003) releases the discarded runtime-stage ReadableStream on all early-return branches via cancelCachedNavigationRuntimeStage.
  • unstable_instant schema documented against the pinned Next.js v16.2.6 oracle (app-segment-config.ts:137).
• wrapCachedNavigationRuntimeValue (app-render-dependency.tsx:104) — functions/typed-arrays/Dates/blobs/Maps/Sets preserve their RSC value kinds; the seen WeakMap guards cycles (including promise chains), and every seen.set stores a freshly-constructed non-undefined object so the existing !== undefined short-circuit is safe. The lazy CachedNavigationRuntimeSubtreeInvoker (:257) intentionally starts a fresh seen scope per lazily-rendered subtree, which is correct since RSC wire values serialize fresh.
• extractStaticExportValue (build/report.ts:176) — resolves export { local as unstable_instant } via the local const binding, bails to supported: false on re-exports (statement.source !== null, :209) and unsupported expressions; the build-time validation plugin (plugins/app-segment-config-validation.ts) correctly orders the client-module check before the cacheComponents check (matching validateAppRouteSegmentConfig at app-segment-config.ts:190/197).
• resolveCachedNavigationRuntimeModuleChain / moduleUsesRuntimeCachedNavigations (app-cached-navigation-runtime.ts) — the monotonic left-fold keys off instant.prefetch === "runtime", consistent with the documented schema.
• getCachedNavigationFallbackParamNames (app-rsc-handler.ts:217) — the prefix-accumulation filter mirrors nested-dynamic staticParamsMap keying; non-array generateStaticParams output bails conservatively to "all params unknown."

Overall the PR is in good shape. Every finding across all eight prior review rounds is now cleanly addressed, and I found no blocking issues on this tree. LGTM.

[ask-bonk]

Posted the review to PR #2870.

Summary

I reviewed the full 47-file tree at HEAD ed70871 ("clarify fallback param staging") — the first /bigbonk review of the commit that landed after the previous round (0e6603e).

ed70871 is a docs-only change (2 lines, no behavior change) that resolves the single nit from the prior round: the getFallbackShellPromise memoization comment in shims/thenable-params.ts now accurately states that the dynamic-boundary mark is idempotent and re-applied on each fallback-key access (previously it claimed first-access-only semantics that the code didn't provide).

Verification:

• Ran the affected unit suites locally (712 tests, all green): thenable-params, app-cached-navigation-runtime, app-segment-config, app-browser-entry, app-visited-response-cache, ppr-fallback-shell, fetch-cache, next-config.
• vp lint clean (exit 0).
• Independently re-verified the code the comment describes (the promise memoization vs. the unconditional idempotent mark), plus all prior-round fixes remain intact (config default resolution, lazy fetch() thunk, runtime-stage stream cancellation, unstable_instant schema doc), and the RSC value wrapper / static-export extractor / fallback-param logic are all correct.

Findings this round: none. Every finding across all eight prior review rounds is now cleanly addressed and I found no blocking issues on this tree. LGTM.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Reviewed the full tree at HEAD 072472d ("gate cached navigations on explicit opt-in") — the first /bigbonk review of the commit that landed after the previous round (ed70871).

072472d is a behavioral reversal of the 9a295ef round's config change, and — importantly — it is the correct one for this PR's oracle. The earlier rounds asserted that Next.js "auto-enables cachedNavigations whenever cacheComponents is on," which pushed the author to a default-on-with-opt-out resolution. I re-verified against the actual Next.js sources and that claim is version-dependent:

• Next.js v16.2.6 (packages/next/src/server/config.ts + config-shared.ts) — the pinned deploy-suite oracle this PR validated its E2E against — has no auto-enable. cachedNavigations has no default in defaultConfig and is only truthy when the user explicitly sets experimental.cachedNavigations. The only related logic is the if (result.experimental.cachedNavigations && !result.cacheComponents) throw guard.
• Next.js canary does auto-enable it (config.ts:2370-2384, "Enable cachedNavigations by default when cacheComponents is enabled"), opt-out only via an explicit false. This is the block the earlier rounds cited — but it does not exist in v16.2.6.

So 072472d correctly aligns vinext with the v16.2.6 oracle the E2E ran against, and the test comment (next-config.test.ts:2595-2596) documents this with the exact source link. This is a deliberate, version-consistent choice, not a divergence.

I verified the rest of the commit is correct:

• experimental.cacheComponents → cacheComponents deprecation move (next-config.ts:1744-1749, warning at :947) matches Next's warnOptionHasBeenMovedOutOfExperimental('cacheComponents', ...): the deprecated experimental location overrides the top-level value when present, and the resolved local is what's written into the returned config (:1941) and the build-time define (index.ts:2393).
• Genuine bug fix in the throw guard (next-config.ts:1833): it now checks the resolved cacheComponents instead of the raw config.cacheComponents. Before this commit, a config using only the deprecated experimental.cacheComponents: true + experimental.cachedNavigations: true would have thrown incorrectly; now it does not, matching Next (which evaluates the guard on the post-move result.cacheComponents).
• appShells co-flag warning (:1772) is once again consistent: with opt-in restored, listing experimental.cachedNavigations as a missing co-flag when it isn't explicitly set matches Next's requirement that all appShells co-flags be explicitly enabled.

Verified locally at 072472d:

• tests/next-config.test.ts (218 tests), tests/app-rsc-handler.test.ts + tests/app-cached-navigation-runtime.test.ts (177 tests) — all green.
• vp check and vp lint on config/next-config.ts — clean (format + lint + types).

Nit (parity, no action) — pick one Next.js version as the documented target for the flag default

The cachedNavigations default now tracks v16.2.6 (opt-in), while the unstable_instant schema comment added earlier (app-segment-config.ts) also pins v16.2.6, and AGENTS.md's general guidance is to track canary (where this flag is default-on-with-opt-out). The current choice is internally consistent and matches the E2E oracle, so no change is needed — but since canary and v16.2.6 genuinely disagree here, a one-line note on the cachedNavigations resolution (mirroring the isInstantConfig "pinned to v16.2.6 oracle" comment) would make the version target explicit and prevent a future contributor from "fixing" it back to the canary default.

Low (parity, minor) — deprecation warning fires on key-value, not key-presence

hasConfigProperty (next-config.ts:906-913) returns false when the value is undefined, so experimental.cacheComponents: undefined won't warn. Next's warnOptionHasBeenMovedOutOfExperimental uses 'cacheComponents' in config.experimental (key presence, warns even for an explicit undefined). This is consistent with vinext's existing deprecation-warning convention across the whole warnings table, so it's not a regression introduced by this PR — flagging only for completeness. No action.

Overall the PR is in good shape. 072472d is the correct resolution for the v16.2.6 oracle and cleanly fixes the deprecation-move throw guard. No blocking issues on this tree.

[ask-bonk]

Posted the review to PR #2870.

Summary

I reviewed the full tree at HEAD 072472d ("gate cached navigations on explicit opt-in") — the first /bigbonk review of the commit that landed after the previous round (ed70871).

Key finding: 072472d is a behavioral reversal of the earlier 9a295ef config change, and it's the correct one for this PR. The earlier bonk rounds pushed the author toward default-on-with-opt-out based on the claim that "Next.js auto-enables cachedNavigations with cacheComponents." I verified against actual Next.js source that this claim is version-dependent:

• v16.2.6 (this PR's pinned E2E oracle): no auto-enable, cachedNavigations is opt-in.
• canary: does auto-enable (the block the earlier rounds cited — which doesn't exist in v16.2.6).

So 072472d correctly aligns vinext with the v16.2.6 oracle the E2E validated against, and the test comment documents it with the exact source link.

I also verified the commit's other changes are correct and includes a genuine bug fix: the cachedNavigations-requires-cacheComponents throw guard now checks the resolved cacheComponents (post-deprecation-move) instead of raw config.cacheComponents, matching Next.js.

Findings posted (both no-action):

1. Nit (parity) — canary and v16.2.6 genuinely disagree on this flag's default; suggested a one-line comment pinning the version target (mirroring the isInstantConfig v16.2.6 comment) to prevent a future "fix" back to the canary default.
2. Low (parity) — the deprecation warning uses key-value (!== undefined) rather than Next's key-presence (in) check; consistent with vinext's existing convention, not a regression.

Verified locally: next-config (218), app-rsc-handler + app-cached-navigation-runtime (177) tests green; vp check/vp lint clean. No blocking issues.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Reviewed the full tree at HEAD 66cfb10 ("fix(dev): prebundle vendored RSC runtimes") — the first /bigbonk review of the commit that landed after the previous round (072472d).

66cfb10 is a dev-server dep-optimizer fix, scoped to index.ts (+ a regression test). It replaces the single react-server-dom-webpack/static.edge RSC-env include with the three vendored specifiers @vitejs/plugin-rsc/vendor/react-server-dom/{server,static,client}.edge, so the first RSC request in dev can't discover a second optimizer entry and invalidate an in-flight response. I verified it end-to-end and it's correct.

Verification

• Specifiers resolve. All three @vitejs/plugin-rsc/vendor/react-server-dom/*.edge specifiers resolve via the package's ./* → ./dist/*.js wildcard export to the real vendored files (dist/vendor/react-server-dom/{server,static,client}.edge.js). Confirmed with require.resolve in this repo's install.
• They mirror plugin-rsc's own RSC-env includes exactly. @vitejs/plugin-rsc computes reactServerDomPackageName = hasReactServerDomWebpack ? "react-server-dom-webpack" : "@vitejs/plugin-rsc/vendor/react-server-dom" and already lists ${name}/{server,static,client}.edge in the RSC env's optimizeDeps.include (plugin dist, RSC env block). So the commit is defensively re-asserting the same three entries vinext's config-merge should already carry — deduped via the new Set([...]). Harmless and belt-and-suspenders against merge-order nuances.
• hasReactServerDomWebpack interaction is safe. When react-server-dom-webpack IS a crawled framework package (real user App Router projects — vinext installs it via utils/project.ts:590 / init.ts:226), plugin-rsc's pre resolveId alias rewrites @vitejs/plugin-rsc/vendor/react-server-dom/* → react-server-dom-webpack/* during optimization, so the added specifiers still resolve to the installed package rather than 404-ing. When it's NOT present (default fixtures here), they resolve to the vendored files. Both paths are covered.
• Tests / lint. tests/app-router-dev-server.test.ts green (182 tests) — the new assertions land in the existing "pre-includes framework dependencies" test and lock the three specifiers into the resolved RSC config. vp lint packages/vinext/src/index.ts clean.

Nit (no action) — stale react-server-dom-webpack/static.edge include left behind

server/index.ts:3209

The commit rewrote the comment to describe the new vendored-specifier strategy but kept the pre-existing "react-server-dom-webpack/static.edge" entry in the same include array. In the default (no-react-server-dom-webpack) setup that specifier doesn't resolve, and it's already suppressed by OPTIONAL_OPTIMIZE_DEPS_WARNING_RE; in real user projects it does resolve. It's not wrong — just now somewhat redundant next to @vitejs/plugin-rsc/vendor/react-server-dom/static.edge (which aliases to the same package when RSDW is present). Flagging only for tidiness; you could drop it or add a one-line note that it's the RSDW-installed-project variant of the same static.edge entry. No behavior impact either way.

Confirmations (no action)

• Every finding across all nine prior review rounds remains cleanly addressed: cachedNavigations gated on explicit opt-in aligned to the v16.2.6 oracle with the resolved-cacheComponents throw-guard fix (next-config.ts:1741/:1833), the resolved-value appShells warning (next-config.ts:1772), lazy fetch() expression thunk (fetch-cache.ts:643), runtimeStageReadStarted gate + .finally() stream cancel (app-browser-entry.ts), connection() probe ordering, and the documented unstable_instant v16.2.6 schema comment.
• The change is dev-only (optimizeDeps.include) and does not touch the production build pipeline or dev/prod request parity.

Overall the PR is in good shape. 66cfb10 is a correct, well-tested, dev-only dep-optimizer hardening. No blocking issues on this tree — the single item above is a tidiness nit. LGTM.

[ask-bonk]

Posted the review to PR #2870.

Summary

I reviewed the full tree at HEAD 66cfb10 ("fix(dev): prebundle vendored RSC runtimes") — the first /bigbonk review of the commit that landed after the previous round (072472d).

66cfb10 is a dev-server dep-optimizer fix scoped to index.ts plus a regression test. It replaces the single react-server-dom-webpack/static.edge RSC-env optimizeDeps include with the three vendored specifiers @vitejs/plugin-rsc/vendor/react-server-dom/{server,static,client}.edge, so the first RSC request in dev can't discover a second optimizer entry and invalidate an in-flight response.

Verified:

• All three vendor specifiers resolve (via the package's ./* → ./dist/*.js wildcard export to the real vendored files) — confirmed with require.resolve.
• They mirror plugin-rsc's own RSC-env includes exactly (reactServerDomPackageName/{server,static,client}.edge), deduped via new Set([...]) — defensive re-assertion against config-merge ordering.
• The hasReactServerDomWebpack alias interaction is safe in both cases: aliased to the installed package in real user projects, resolved to vendored files in the default fixtures.
• tests/app-router-dev-server.test.ts green (182 tests); vp lint clean on the changed file.

Findings posted: one tidiness nit only — the pre-existing react-server-dom-webpack/static.edge include entry is left in the array alongside the new vendored static.edge specifier, making it somewhat redundant. No behavior impact.

The change is dev-only (optimizeDeps.include) and doesn't touch the production build pipeline or request parity. All findings across the nine prior review rounds remain cleanly addressed. No blocking issues — LGTM.

github run

[james-elicx]

Exact-head deploy audit for 66cfb10c5386175039110d5fcd3de5e90432a1fb: run 31429981590 finished 2625 passed / 179 failed / 631 skipped versus baseline 31350589613 at 2617/187/631. Assertion-level delta: eight directly attributable cached-navigation fail→pass recoveries, one incidental hash-scroll fail→pass, and one apparent pass→fail in segment-cache/prefetch-auto. The latter timed out at the 60s Jest limit on both CI invocations without an assertion failure; an exact-head targeted NEXT_TEST_CONCURRENCY=1 rerun passed 1/1 in 1.59s, and the immediately preceding behavioral head also passed it. Independent audit verdict: no genuine regression. Fresh Big Bonk on this head is also clean.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — PR #2870 (cached navigation stages)

Independently reviewed the full 48-file tree at HEAD 8109b96 (main merged into the PR branch). This PR has been through ~11 prior review rounds; I re-verified the previously-flagged areas remain correctly resolved and focused fresh scrutiny on stream lifecycle, generation guarding, and the build-time validation path. Ran the affected unit suites locally (all green) — app-cached-navigation-runtime, app-segment-config-validation, ppr-fallback-shell (37 tests), app-visited-response-cache, app-browser-entry (256 tests).

No blocking issues. The core mechanics are sound and well-defended. Findings below are all low / confirm-only.

Verified correct (spot-checked this round)

• Runtime-stage stream lifecycle (app-page-render.ts:700-741, app-browser-entry.ts:966-1004): the producer side closes runtimeStage.writable on both the no-prerender path (:711) and via pipeTo (:731, which closes on source end), and the error path (:733-741) wraps writable.close() in try/catch. Double resolvePartial/resolveStaleTimeSeconds on the error path is a safe no-op (already-resolved promise). The consumer side releases the discarded readable via runtimeStageReadStarted + .finally() cancel. A pipeTo rejection surfaces to the consumer's arrayBuffer() read, which is swallowed by the .catch(() => {}) at :1001. No leak on any branch.
• Stage generation guarding (app-browser-entry.ts:818-886): storeCachedNavigationStage keys both stages by fillGeneration, drops the prior runtime stage when a new static stage begins (:851), and gates runtime publication on canPublishCachedNavigationRuntimeStage matching the static base's stageGeneration (:858-867) — a fill whose static base was evicted mid-flight is correctly discarded.
• Static export extraction (build/report.ts:176-437): extractStaticExportValue resolves export { local as unstable_instant } via the local const binding (:210-211), bails supported: false on cross-file re-exports (statement.source !== null, :209), only trusts const initializers, and extractStaticJsonValueWithBindings guards identifier cycles with the seen Set (:403-408). Correctly degrades to the runtime validator as documented.
• RSC value wrapping (app-render-dependency.tsx:104-168): functions/typed-arrays/Dates/blobs/Maps/Sets preserve their RSC value kinds; the seen WeakMap guards cycles including async promise chains; every seen.set stores a freshly-constructed non-undefined object so the existing !== undefined short-circuit is safe.
• Config resolution (next-config.ts:1746-1750, :1833): cachedNavigations is opt-in gated on resolved cacheComponents (matching the v16.2.6 oracle), and the throw guard checks resolved cacheComponents (post deprecation-move). connection() runs suspendConnectionProbe() before the fallback-shell delay (server.ts:1280). Lazy fetch() expression thunk (fetch-cache.ts:643) and lazy headers()/cookies() value getters (headers.ts:990/1030) confirmed intact.

Low (nit) — app-segment-config-validation.ts:44-50 validates dynamicStaleTime conflict with a hardcoded cacheComponents: true

validateAppSegmentConfigSource reaches line 44 only after the client-module (:26) and !cacheComponents (:37) guards, so a server module with cache-components is the only path there — the hardcoded cacheComponents: true in the unstable_dynamicStaleTime conflict check (:48) is therefore correct today. But it's an implicit invariant: if the earlier guards are ever reordered or a new early-return is inserted, this call would validate against the wrong cacheComponents. Consider threading options.cacheComponents through instead of the literal, so the call is correct by construction rather than by upstream ordering. No behavior impact today.

Low (confirm) — preparePprFallbackShellFinalRender does not reset cachedNavigationRuntimeReady* fields

ppr-fallback-shell.ts:572-591 resets the cache-task/runtime-discovery counters and epoch on the warmup→final transition, but leaves cachedNavigationRuntimeReadyCleanup / cachedNavigationRuntimeReadyResolvers untouched. This appears intentional: the runtime-ready machinery only fires for the "navigation" stage, and the runtime re-render uses a fresh state from prepareCachedNavigationRuntimeRender (:375-386) rather than re-preparing the navigation state in place. So the final-render reset never needs to touch those fields. Flagging only for author confirmation that preparePprFallbackShellFinalRender is never called on a "navigation"-stage state with pending runtime-ready resolvers (which would strand them) — I couldn't construct such a path and the suites are green.

Nit — divergent moduleUsesRuntimeCachedNavigations intent vs. unstable_instant schema drift

app-cached-navigation-runtime.ts:30-35 keys runtime-stage detection off instant.prefetch === "runtime", consistent with the documented v16.2.6 isInstantConfig schema (app-segment-config.ts:137). This is already documented as pinned to v16.2.6; no action. Noting only that the whole runtime/static branching depends on this single field, so if the unstable_instant schema is ever re-synced to canary (which dropped prefetch), this detection and resolveCachedNavigationRuntimeModuleChain must move together.

Overall the PR is in good shape — thorough test coverage, careful stream and generation lifecycle handling, and all prior-round findings cleanly addressed. LGTM.

[ask-bonk]

Posted the review to PR #2870.

Summary

I independently reviewed the full 48-file tree at HEAD 8109b96 (main merged into the PR branch). Since this PR has already been through ~11 review rounds, I re-verified the previously-flagged fixes remain intact and focused fresh scrutiny on the highest-risk areas: stream lifecycle, generation guarding, and the build-time validation path. Ran the affected unit suites locally — all green (app-cached-navigation-runtime, app-segment-config-validation, ppr-fallback-shell = 37 tests; app-visited-response-cache, app-browser-entry = 256 tests).

No blocking issues. The code is well-defended and thoroughly tested.

Verified correct this round:

• Runtime-stage stream lifecycle — writable closed on all producer branches, readable cancelled on all consumer early-returns, pipeTo rejection safely surfaced and swallowed; no leak.
• Stage generation guarding — mismatched/evicted static bases correctly discard orphaned runtime refinements.
• Static export extraction — aliased/re-exported binding handling, const-only, cycle guard.
• RSC value wrapping — value-kind preservation and cycle handling.
• Config resolution, connection() probe ordering, lazy fetch()/headers()/cookies() computation.

Findings posted (all low / confirm-only):

1. Low (nit) — app-segment-config-validation.ts:48 hardcodes cacheComponents: true in the dynamicStaleTime conflict check; correct today only because of upstream guard ordering. Suggested threading options.cacheComponents for correctness-by-construction.
2. Low (confirm) — preparePprFallbackShellFinalRender doesn't reset the cachedNavigationRuntimeReady* fields; appears intentional (runtime re-render uses a fresh state), flagged for author confirmation that it's never called on a navigation-stage state with pending resolvers.
3. Nit — noting the runtime/static branching depends entirely on instant.prefetch === "runtime", so a future canary re-sync (which dropped prefetch) must move detection and the module-chain resolver together.

Overall: LGTM.

github run