fix(app-router): avoid runtime route-tree prefetch sizing#2467
Conversation
…x-segment-cache-max-prefetch-inlining-28478866791
…x-segment-cache-max-prefetch-inlining-28478866791
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: avoid runtime route-tree prefetch sizing
Solid change — moving route-tree prefetch sizing off a real RSC render is the right call for a hot prefetch path, and the test coverage (deterministic hint trees, config normalization, the userComponentTrapModule guard) is thorough. A few things worth addressing, mostly leftover plumbing from the pre-render approach.
1. Dead renderer parameter — _renderToReadableStream is now completely unused
createRouteTreePrefetchResponse still takes _renderToReadableStream: RouteTreePrefetchRenderer (app-route-tree-prefetch.ts:399) but nothing in the module references it anymore. The caller in app-rsc-handler.ts:1066 still threads options.renderToReadableStream through, and CreateAppRscHandlerOptions.renderToReadableStream remains a required field wired from the generated entry.
Before this PR (HEAD~2) the renderer was genuinely used to measure gzip sizes by rendering components. Now it's pure dead plumbing spanning the handler options, the generated entry, and this signature. Consider dropping the parameter entirely (and the option, if nothing else uses it) so the API stops advertising a capability it no longer exercises. If there's a near-term plan to reintroduce real sizing, a short comment saying so would be better than a silently-unused param.
2. Wasted per-request work: params resolved then discarded
estimatePrefetchSize ignores its params (_params, _searchParams at app-route-tree-prefetch.ts:186-187), yet buildTree still computes them for every node on every prefetch request:
collectSearchParams(options.searchParams)(:403)resolveAppPageSegmentParams(...)per segment and slot owner (:227,:238,:255)resolveAppPageBranchParams(...)per slot child (:270)
resolveAppPageSegmentParams/resolveAppPageBranchParams do O(segments) allocation + Set construction per node (app-page-params.ts:50, :77). All of it feeds a function that throws the values away. This is exactly the kind of hot-path work AGENTS.md flags ("Keep the common request path lightweight", "Avoid repeated hot-path work"). Since sizing is now heuristic and param-independent, the params/searchParams threading through buildTree/createNode/estimatePrefetchSize can be removed.
3. Dead branch: staticPrefetchDisabled can never be true
computePrefetchHints branches on staticPrefetchDisabled = (node.prefetchHints & STATIC_PREFETCH_DISABLED) !== 0 (:294), and several code paths depend on it (:308-309, :331, :344). But prefetchHints is always initialized to 0 in createNode (:119) and nothing sets HAS_RUNTIME_PREFETCH / PREFETCH_DISABLED / STATIC_PREFETCH_DISABLED anywhere in the codebase before computePrefetchHints runs (confirmed by grep across src/). So staticPrefetchDisabled is always false and those branches are unreachable.
This is a faithful port of Next.js's algorithm, but in Next.js those bits are populated from route config like dynamic = 'force-static'. In vinext they never are, which means routes with export const dynamic/runtime-prefetch semantics won't get the static-prefetch-disabled treatment — a real parity gap, not just dead code. Either:
- wire the bits from route metadata so the branches actually fire, or
- if that's intentionally out of scope, drop the unreachable branches and add a comment/issue documenting the gap so the next reader doesn't assume it works.
4. prefetchSize export is a vinext-only convention (not a Next.js API)
explicitPrefetchSize reads module.prefetchSize with "large"/"small"/number (:176-182), and the fixtures export export const prefetchSize = "large" to force outlining. Worth a brief comment noting this is a vinext test/escape-hatch convention (Next.js sizes by rendering), so it isn't mistaken for an upstream API that users can rely on. Given vinext can't measure gzip sizes without rendering, the heuristic tradeoff itself is reasonable — just document it.
Minor
normalizePrefetchInliningConfig(next-config.ts:1349) keeps negativemaxSize/maxBundleSizeverbatim (Number.isFinite(-5)is true). Next.js clamps only non-finite values toMAX_SAFE_INTEGER; a negative threshold here would silently disable all inlining. Low priority, but a>= 0guard (mirroringstaleTimeshandling right above it) would be more robust.
None of these block correctness of the happy path (tests pass), but #1 and #3 leave misleading dead surface area and #2/#3 have real hot-path/parity implications.
|
Review posted to PR #2467. Summary of what I found: Review SummaryThe PR correctly moves route-tree prefetch sizing off a real RSC render — the right call for a hot path, with thorough test coverage. But the cleanup from the old render-based approach was left incomplete. Findings:
Targeted tests ( |
james-elicx
force-pushed
the
codex/fix-segment-cache-max-prefetch-inlining-28478866791
branch
from
7c38460 to
eb4dabc
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: avoid runtime route-tree prefetch sizing
The rewrite in eb4dabc cleanly resolves the earlier review's concerns — the render-based sizing, the dead renderToReadableStream plumbing in the handler, and the discarded params/searchParams threading are all gone. The heuristic sizing approach is a reasonable tradeoff given vinext can't measure gzip sizes without rendering, and the userComponentTrapModule test is a nice guard. Two things to address, one blocking.
1. Blocking: type error breaks CI Check
tests/app-rsc-handler.test.ts still passes renderToReadableStream to createAppRscHandler, but the option was removed from CreateAppRscHandlerOptions in this same PR. vp check fails:
× typescript(TS2353): Object literal may only specify known properties, and
'renderToReadableStream' does not exist in type 'CreateAppRscHandlerOptions<TestRoute>'.
tests/app-rsc-handler.test.ts:140:5
× typescript(TS2339): Property 'renderToReadableStream' does not exist on type
'CreateAppRscHandlerOptions<TestRoute>'.
tests/app-rsc-handler.test.ts:140:70
The runtime tests pass (the extra property is ignored at runtime), so this is purely a type failure — but it will fail the required Check job. The fix is to drop the stale option (line 140) and the now-unused import { renderToReadableStream } from "react-dom/server.edge" (line 3). I've pushed that fix.
2. Parity: negative-threshold clamping diverges from Next.js
normalizePrefetchInliningConfig (next-config.ts:1349) clamps negative thresholds to Number.MAX_SAFE_INTEGER via the >= 0 guard, and tests/next-config.test.ts now asserts this. But Next.js only clamps non-finite values — it does not special-case negatives. From packages/next/src/server/config.ts (assignDefaultsAndValidate):
result.experimental.prefetchInlining = {
// Clamp Infinity to a finite value so the config survives
// JSON.stringify (used by output: standalone).
maxSize: Number.isFinite(maxSize) ? maxSize : Number.MAX_SAFE_INTEGER,
maxBundleSize: Number.isFinite(maxBundleSize) ? maxBundleSize : Number.MAX_SAFE_INTEGER,
}A negative finite value (-1) stays -1 in Next.js, which effectively disables all inlining (nothing is < maxSize). vinext instead turns -1 into MAX_SAFE_INTEGER, which inlines everything — the opposite behavior. Per AGENTS.md, divergences from Next.js should be deliberate and documented. Since the sole purpose of the clamp is JSON-survivability of Infinity, matching Next.js (Number.isFinite(x) ? x : MAX_SAFE_INTEGER) and dropping the >= 0 guard would restore parity. If keeping the guard is intentional (defense against a footgun), a comment noting the deliberate divergence would help. The earlier suggestion to add >= 0 was actually against upstream behavior.
Everything else looks good: Infinity/partial-object/default resolution matches Next.js, the codegen embedding is correct, dev/prod parity is preserved (App Router prod server delegates to the RSC entry, and route-tree prefetch is App-Router-only so Pages Router is unaffected), and the link.tsx two-stage /_tree → /__PAGE__ gate is well-tested.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2467. I also fixed the blocking type error in the test file (this is included in the response and will be pushed automatically). SummaryThe rewrite in
Everything else (Infinity/partial/default resolution, codegen embedding, dev/prod parity, the link.tsx |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: avoid runtime route-tree prefetch sizing
Re-reviewed at a8f32b6. The two prior review rounds are fully resolved:
- Type error (was blocking):
tests/app-rsc-handler.test.tsno longer passes the removedrenderToReadableStreamoption and the unusedreact-dom/server.edgeimport is gone.vp checkon all touched files now passes (format + lint + types). - Negative-threshold parity:
a8f32b6drops the>= 0guard, sonormalizePrefetchInliningConfignow matches Next.js'sassignDefaultsAndValidateexactly (Number.isFinite(x) ? x : MAX_SAFE_INTEGER). Verified againstpackages/next/src/server/config.tson canary — a negative finite value stays negative (disabling inlining) rather than being coerced toMAX_SAFE_INTEGER(inlining everything). The test innext-config.test.tswas updated to assert the parity behavior. Good. - Dead renderer plumbing / discarded params / unreachable
StaticPrefetchDisabledbranches: all gone in theeb4dabcrewrite. The bit constants (ParentInlinedIntoSelf = 0b100000,InlinedIntoChild = 0b1000000,HeadInlinedIntoSelf = 0b10000000) matchpackages/next/src/shared/lib/app-router-types.ts.
Verification run locally:
vp check tests/app-rsc-handler.test.ts tests/next-config.test.ts packages/vinext/src/server/app-route-tree-prefetch.ts packages/vinext/src/config/next-config.ts— passvp test run tests/app-route-tree-prefetch.test.ts tests/next-config.test.ts— 221 passedvp test run tests/app-router-next-config-codegen.test.ts tests/app-rsc-handler.test.ts— 145 passed
Two non-blocking observations, both documentation-only:
1. HeadOutlined is never set on the root (untested edge case)
computePrefetchHints sets HeadInlinedIntoSelf when a page accepts the head inline, but never sets the root's HeadOutlined bit (0b100000000) in the fallback case where the head can't be inlined (inlinedBytes + HEAD_INLINE_SIZE >= maxBundleSize). Next.js sets HeadOutlined on the root so the client scheduler knows to fetch the head separately. Every fixture case ends with (+metadata) on a __PAGE__, so this path is never exercised — and with HEAD_INLINE_SIZE = 1 it only triggers under a tiny maxBundleSize. Vinext's own client also doesn't read these bits (it uses the two-stage /_tree → /__PAGE__ gate in link.tsx), so the practical impact is limited to the raw Next.js client scheduler compatibility surface. A one-line comment noting HeadOutlined is intentionally not emitted (or a fixture that forces head outlining) would close the loop.
2. Number(value) coercion diverges slightly from Next.js for non-number thresholds
normalizePrefetchInliningConfig calls Number(maxSize) before the finite check, whereas Next.js checks Number.isFinite(maxSize) on the raw value. For a stringified numeric threshold (maxSize: "512"), vinext resolves 512 while Next.js would fall through to MAX_SAFE_INTEGER. This only matters for malformed configs the schema would normally reject, and vinext is the more lenient of the two — noting for completeness, not asking for a change.
Everything else (Infinity/partial/default resolution, codegen JSON.stringify embedding with a covering test, dev/prod parity via the RSC-entry delegation, the userComponentTrapModule guard, dynamic-param serialization, parallel-slot ordering) checks out. The heuristic sizing tradeoff (size 1 for any real component, prefetchSize escape hatch for forced outlining) is reasonable given vinext can't measure gzip sizes without rendering, and it's now documented in explicitPrefetchSize.
|
Review posted to PR #2467. SummaryI re-reviewed at the current head ( Resolved:
Verified locally:
Two non-blocking, documentation-only observations:
No blocking issues. The heuristic sizing tradeoff is reasonable and now documented. |
NathanDrake2406
left a comment
NathanDrake2406
left a comment
There was a problem hiding this comment.
Review: route-tree prefetch inlining
Requesting changes on one contract issue, plus one structural cleanup I think should happen before this pattern spreads.
1. Blocking: route-tree dynamic segment siblings use the wrong data model
packages/vinext/src/server/app-route-tree-prefetch.ts:211 passes route.staticSiblings into every dynamic route-tree segment. That field is route-wide and flattened across all dynamic URL levels; the existing route graph docs say so explicitly. But TreePrefetchParam.siblings is segment-local, matching Next's dynamic segment tuple.
For a route like /[locale]/products/[id], any static sibling discovered at one dynamic level can be attached to the other dynamic segment in the route-tree response. That gives the client incorrect segment-cache metadata and makes the new helper depend on a field that was only shaped for the older full RSC payload.
Can we either emit segment-local static siblings into the route manifest and thread that into the route-tree builder, or set route-tree siblings to null until that data exists? Reusing the flattened field here is the wrong ownership boundary.
2. Structural: route-tree prefetches are being modeled as optimistic loading shells
packages/vinext/src/shims/link.tsx:543 reuses the loading-shell gate path for route-tree prefetches and stores the /_tree response with optimisticRouteShell: true. That leaks a new transport concept into unrelated optimistic-template learning: app-browser-entry.ts scans settled prefetch entries and tries to decode them as App element payloads.
It mostly avoids hard failure because the learning loop awaits Promise.allSettled, but the shape is misleading. A route-tree response is not a loading-shell payload and should not be tagged as one. I think this wants an explicit cache kind, e.g. prefetchKind: "route-tree" | "loading-shell" | "navigation", or a dedicated route-tree pending gate outside the navigation prefetch cache.
CI is green, and the earlier renderer-plumbing/type/parity issues are resolved. This review is about keeping the route-tree contract and cache model clean before more segment-cache behavior builds on top of it.
…he-max-prefetch-inlining-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx
…he-max-prefetch-inlining-28478866791
…he-max-prefetch-inlining-28478866791
…he-max-prefetch-inlining-28478866791 # Conflicts: # tests/app-rsc-handler.test.ts
…he-max-prefetch-inlining-28478866791
|
Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the App Router max prefetch inlining rows from Exact rows:
Since this PR is already James-owned, I am skipping duplicate implementation work for these rows. |
This reverts commit 68b96b5.
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: avoid runtime route-tree prefetch sizing
Re-reviewed at d738f95. The latest commit ("keep route-tree prefetch metadata scoped") resolves both blocking issues from the prior review round, and all earlier concerns (dead renderer plumbing, discarded params, type error, negative-threshold parity) remain resolved. No blocking issues.
Prior blockers — both resolved
1. Dynamic segment siblings used the wrong data model — fixed.
staticSiblings is entirely gone from the route-tree path. AppRouteTreePrefetchRoute no longer declares it, createNode/routeTreeSegment/dynamicRouteTreeSegment no longer thread it, and dynamicRouteTreeSegment now emits siblings: null with a comment explaining that vinext only has the flattened full-RSC-payload list and leaves segment-local siblings unknown until segment-local route metadata exists (app-route-tree-prefetch.ts:139-155). The route-wide staticSiblings field is now only used by the legitimate full-RSC-payload path (app-page-route-wiring.tsx, app-rsc-manifest.ts) — the incorrect reuse in the route-tree builder is removed. The handler route type (app-rsc-handler.ts:118-126) is consistent — no stale staticSiblings forwarding. The test was updated to assert siblings: null for /[slug].
2. Route-tree prefetches modeled as optimistic loading shells — fixed.
There's now an explicit PrefetchCacheKind = "loading-shell" | "navigation" | "route-tree" on the cache entry (navigation.ts:256, mirrored in next-shims.d.ts). Route-tree prefetches are tagged prefetchKind: "route-tree" with optimisticRouteShell: false (link.tsx:596-597) instead of being mislabeled as a loading shell. The optimistic-template learning loop now skips route-tree entries before trying to decode them as App element payloads (app-browser-entry.ts:506), and promotePrefetchEntriesForNavigation skips them too (link.tsx:708). Navigation consumption already excluded them via cacheForNavigation === false (navigation.ts:460); the new tag makes the intent explicit and self-documenting. New tests cover it: prefetch-cache.test.ts ("keeps route-tree prefetch responses out of navigation consumption") and link-navigation.test.ts asserts the route-tree entry has cacheForNavigation: false / optimisticRouteShell: false.
Verified locally at d738f95
vp checkon the 8 touched source/test files — pass (format + lint + types)tests/app-route-tree-prefetch.test.ts+tests/next-config.test.ts— 221 passedtests/prefetch-cache.test.ts+tests/link-navigation.test.ts— 95 passedtests/app-rsc-handler.test.ts+tests/app-router-next-config-codegen.test.ts+tests/shims.test.ts— 1323 passed
Non-blocking (documentation-only, unchanged from prior round)
HeadOutlinedroot bit never emitted.computePrefetchHintssetsHeadInlinedIntoSelfbut never sets the root'sHeadOutlinedbit in the fallback path (head can't inline). Vinext's own client doesn't read these hint bits — it uses the two-stage/_tree→ segment gate inlink.tsx— so impact is limited to raw Next.js client scheduler compatibility, and withHEAD_INLINE_SIZE = 1it only triggers under a tinymaxBundleSize. A one-line comment (or a fixture forcing head outlining) would close the loop.Number(value)coercion is slightly more lenient than Next.js for stringified thresholds (malformed-config edge case only; vinext is the more permissive of the two).
The heuristic sizing tradeoff (size 1 for any real component, prefetchSize escape hatch for forced outlining) is documented in explicitPrefetchSize, dev/prod parity holds (route-tree prefetch is App-Router-only and the prod server delegates to the RSC entry), and the bit constants match app-router-types.ts. Good to merge once CI is green.
|
Review posted to PR #2467. SummaryI re-reviewed at the current head ( Resolved blockers:
Verified locally: Two non-blocking, documentation-only observations carried over: the |
james-elicx
deleted the
codex/fix-segment-cache-max-prefetch-inlining-28478866791
branch
2 participants
Summary
experimental.prefetchInliningthresholds, includingInfinity, and pass resolved values into the App RSC handlerStacked on #2440 (
codex/fix-optimistic-routing-prefetch) to avoid bundling the prerequisite optimistic-prefetch diff.Validation
vp checkon touched source/test filesvp test run tests/app-route-tree-prefetch.test.ts tests/next-config.test.tsvp test run tests/app-rsc-handler.test.ts -t "route-tree"vp test run tests/link-navigation.test.ts -t "prefetchInlining|deduped"vp test run tests/app-router-next-config-codegen.test.ts tests/app-route-tree-prefetch.test.ts tests/next-config.test.tsvp run vinext#buildREPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/max-prefetch-inlining/max-prefetch-inlining.test.ts