fix(app-router): vary segment prefetches by accessed params#2474
fix(app-router): vary segment prefetches by accessed params#2474james-elicx wants to merge 7 commits into
Conversation
|
/bigbonk review for issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
4c8d470 to
6928f9f
Compare
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
6928f9f to
9a028cb
Compare
|
/bigbonk review for issues |
commit: |
|
Review: fix(app-router): vary segment prefetches by accessed paramsFocused on correctness and CI parity. One blocking issue found (a required Vitest check will fail), plus a few soundness observations. Blocking —
|
|
Posted the review to PR #2474. Summary of findings: Review postedBlocking issue (will fail CI):
Other observations:
The core implementation (optimistic template variant keying, suspense preservation, cache-key varying) is coherent and its focused unit tests pass. The main required action is fixing the stale |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
9a028cb to
a7fe652
Compare
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed after the previous round. The prior blocking issue (tests/link.test.ts asserting the old prefetchShellFirst shape) is now fixed — the test was updated to renderLoadingShell and passes locally (121/121). The two new unit files pass too (app-optimistic-routing.test.ts 14/14, app-prefetch-vary-analysis.test.ts 6/6). No blocking issues remain; the notes below are correctness/hygiene follow-ups.
Dead build-time define — __VINEXT_PREFETCH_INLINING (introduced by this PR)
This PR removed const __prefetchInlining and its || __prefetchInlining term from the fetchPromise gate in link.tsx (confirmed in the commit diff), so the flag is no longer read anywhere in the shim (only a comment remains). But the build-time define and its type are still present:
packages/vinext/src/index.ts:1751still injectsprocess.env.__VINEXT_PREFETCH_INLININGpackages/vinext/src/global.d.ts:426still declares__VINEXT_PREFETCH_INLINING?: string
Two things to resolve:
- Confirm the behavior change is intentional. Before this PR, configuring
experimental.prefetchInliningforced the two-phase shell-then-fullfetchPromisepath even formode === "auto". Now that path is gated only onmode === "full" || mode === "full-after-shell", soprefetchInliningno longer changes prefetch behavior. If that's the intended consolidation, remove the now-dead define +global.d.tsentry; if not, the gate needs the flag back. tests/link-navigation.test.ts:1927is now a misleading stub. That test still doesvi.stubEnv("__VINEXT_PREFETCH_INLINING", "true"), but the asserted shell-only prefetch for/teams/vercel/dashboardnow happens via therequiresDynamicNavigationRequest→renderLoadingShellpath regardless of the flag. The test passes, but the stub implies a dependency that no longer exists. Drop the stub (or replace it with a comment) so the test doesn't imply flag-gated behavior.
Prefetch scheduling change: requestIdleCallback → setTimeout(fn, 0) (introduced by this PR)
prefetchUrl's low-priority scheduler changed from window.requestIdleCallback ?? ((fn) => setTimeout(fn, 100)) to (fn) => setTimeout(fn, 0) (link.tsx). This makes viewport/idle prefetches run on the very next macrotask instead of during browser idle time, which can contend with more important work on link-heavy pages. If this is deliberate (e.g. to make segment-cache reuse deterministic for the new tests), please add a one-line comment with the rationale; otherwise it looks like an accidental regression of the idle-scheduling behavior. Worth confirming whether any existing test actually required the change, or whether requestIdleCallback can be retained.
Soundness of regex-based static param analysis (analyzePrefetchVary)
The analysis infers param/searchParams access by scanning source with regexes at build time (app-browser-entry.ts). This is inherently approximate; the dangerous direction is a false negative — missing an access leads to over-sharing the segment cache and serving wrong content (a correctness bug), not just a perf regression. Confirmed gaps:
- Arrow-form
generateMetadatais not split out.extractExportedFunction/removeExportedFunctiononly matchexport (async) function generateMetadata.export const generateMetadata = async (...) => {}stays in the page body, so its accesses are attributed to the body instead of metadata. This errs toward over-varying (safe), but silently disables the metadata-vs-body split for arrow-form exports — worth a comment noting the limitation or handling the const form. staticPrefetchRegiontruncates atindexOf("connection(")with no word boundary.myConnection(, or the literal string"connection("in unrelated code, would truncate the analyzed region early and drop later param/searchParams accesses (a false negative → over-share). Comments are stripped but strings/identifiers are not. Consider\bconnection\s*\(.- Runtime vs static tracking. Next.js tracks param access at runtime via the params proxy; this is a static approximation. Please make sure the ported Next.js e2e rows cover the false-negative shapes — helper-function param reads, computed member access (
params[key]), and spread-into-helper — since those are the cases that surface as wrong cached content rather than a perf hit.
CodeQL "double unescaping" alert — false positive (confirm with a comment)
decodeSimpleJsxText (app-browser-entry.ts:280) uses a single-pass String.prototype.replace with a global regex; replacement output is not re-scanned, so &lt; → < (the test decodes runtime-prefetch fallback entities without double-decoding asserts exactly this). The CodeQL alert is a false positive. Add a one-line comment noting the single-pass intent so the alert can be dismissed with justification.
Nit
replaceInnermostSuspenseChildren(app-optimistic-routing.ts) guards recursion atdepth > 100and returns the value unchanged past that. A brief comment on why 100 and that exceeding it means "no fallback replacement" would help future readers.
|
Review posted to PR #2474. Summary of findings: Review postedThe previous round's blocking issue is resolved: Follow-ups flagged (all confirmed against the commit diff):
The core implementation (variant-keyed optimistic templates, suspense-child preservation, shared-cache-key varying) is coherent and its focused unit tests pass. |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
a7fe652 to
fa75126
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed the current commit (fa75126). Most of the earlier rounds' follow-ups are now resolved in the tree, but I found a new blocking regression in a required Vitest check that the prior rounds missed, plus notes on what is now stale.
Blocking — tests/link-navigation.test.ts regresses on this branch (required Vitest check fails)
tests/link-navigation.test.ts is not in the PR diff, but this PR changes behavior it asserts. The test passes on main and fails on this branch:
FAIL tests/link-navigation.test.ts >
Link prefetch scheduling >
uses a shell-only automatic prefetch for dynamic routes requiring fresh navigation
AssertionError: expected null to be 'prefetch-loading-shell'
- Expected: "prefetch-loading-shell"
+ Received: null
tests/link-navigation.test.ts:1949
Root cause: resolveMatchedAutoAppRoutePrefetch decoupled renderLoadingShell from the optimistic-route-shell decision. For the fixture route { patternParts: ["teams", ":team", "dashboard"], isDynamic: true, canPrefetchLoadingShell: false, requiresDynamicNavigationRequest: true } (tests/link-navigation.test.ts:59-64), the new fallback branch returns:
const hasLoadingShell = route.canPrefetchLoadingShell; // false
return {
cacheForNavigation: !hasLoadingShell && route.requiresDynamicNavigationRequest !== true, // false
renderLoadingShell: hasLoadingShell, // false
shouldPrefetch: true,
};So renderLoadingShell === false, and prefetchUrl then builds headers with renderMode: renderLoadingShell ? APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL : undefined → undefined (link.tsx:528-531). On main, this same route was an optimistic-route-shell prefetch (!cacheForNavigation) and therefore requested APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL.
This is a real behavioral change for dynamic routes that require a fresh navigation request but have no loading boundary: previously they were prefetched as a loading shell; now they are prefetched without the shell render mode. Two things to resolve before merge:
- Decide whether the behavior change is intentional. If
requiresDynamicNavigationRequestroutes with no loading boundary should still request the shell render mode, the fallback branch needs to keeprenderLoadingShell(or the render-mode gate) tied toshouldLearnOptimisticRouteShell, not only tocanPrefetchLoadingShell. - Either way,
tests/link-navigation.test.tsmust be updated in this PR so the required Vitest check is green. Since it is not in the diff, it was not re-derived against the new semantics.
I verified this by running the test on both branches:
main: passes (1 passed | 54 skipped)- this branch: fails at
tests/link-navigation.test.ts:1949
The full tests/link.test.ts, tests/app-optimistic-routing.test.ts, and tests/app-prefetch-vary-analysis.test.ts all pass (144 total), and tests/features.test.ts + tests/app-router.test.ts pass (459 total). The regression is isolated to link-navigation.test.ts.
Now-stale prior review notes (no action needed)
For the record, several items flagged in earlier rounds are not present in the current commit and need no change:
__VINEXT_PREFETCH_INLININGis not dead. The|| __prefetchInlininggate is still inlink.tsx:616, the define is still injected atindex.ts:1751-1752, and the type is still declared atglobal.d.ts:426. The flag remains consistently wired, and thelink-navigation.test.ts:1927stub is still meaningful. Earlier rounds claimed this was removed; it was not.- Scheduler is unchanged.
prefetchUrlstill useswindow.requestIdleCallback ?? ((fn) => setTimeout(fn, 100))(link.tsx:461), notsetTimeout(fn, 0). No idle-scheduling regression. staticPrefetchRegionalready uses a word boundary (\bconnection\s*\(,app-browser-entry.ts:128), with a dedicated test (does not truncate static analysis on connection substrings).decodeSimpleJsxTextalready documents its single-pass intent (app-browser-entry.ts:291), with thedecodes runtime-prefetch fallback entities without double-decodingtest — the CodeQL alert is a documented false positive.replaceInnermostSuspenseChildrenalready has thedepth > 100comment (app-optimistic-routing.ts:347).
Soundness note (non-blocking)
The regex-based static param analysis in analyzePrefetchVary errs toward over-varying for unknown/computed/helper reads and has good coverage for those shapes. The one remaining approximation: arrow-form export const generateMetadata = async (...) => {} is not split from the page body by extractExportedFunction/removeExportedFunction (only export (async) function is matched), so metadata accesses are attributed to the body. This over-varies (safe), but silently disables the metadata-vs-body split for arrow-form exports. Worth a one-line comment noting the limitation, or handling the const form.
The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based cache reuse) is coherent. The blocking item is the link-navigation.test.ts regression.
|
Review posted to PR #2474. SummaryI found one blocking regression that earlier review rounds missed: Blocking —
Verified stale (no action) — prior rounds were incorrect/outdated on these:
Non-blocking: arrow-form Other suites ( |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
fa75126 to
5ce0276
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed the current HEAD (5ce0276). The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based cache reuse, and the regex analyzePrefetchVary that over-varies on unknown/computed/helper reads) is coherent and its focused unit tests pass. But there is one blocking regression in a required Vitest check that this PR introduces and does not update.
Blocking — tests/link-navigation.test.ts regresses on this branch (required Vitest check fails)
tests/link-navigation.test.ts is not in the PR diff, but this PR changes behavior it asserts. The test passes on main and fails on this branch:
FAIL tests/link-navigation.test.ts >
Link prefetch scheduling >
uses a shell-only automatic prefetch for dynamic routes requiring fresh navigation
AssertionError: expected null to be 'prefetch-loading-shell'
- Expected: "prefetch-loading-shell"
+ Received: null
tests/link-navigation.test.ts:1949
I verified this by running the test on this branch (fails at tests/link-navigation.test.ts:1949) and confirming the test source is byte-identical on main (so it passed there).
Root cause — the render-mode header was decoupled from the optimistic-route-shell decision.
On main, the shell render mode was gated on !cacheForNavigation:
const isOptimisticRouteShellPrefetch = !autoPrefetch.cacheForNavigation;
renderMode: isOptimisticRouteShellPrefetch ? APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL : undefined,On this branch it is gated only on renderLoadingShell (link.tsx:530):
renderMode: renderLoadingShell ? APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL : undefined,For the fixture route { patternParts: ["teams", ":team", "dashboard"], isDynamic: true, canPrefetchLoadingShell: false, requiresDynamicNavigationRequest: true } (tests/link-navigation.test.ts:59-64), the new resolveMatchedAutoAppRoutePrefetch fallback branch (link.tsx:336-345) returns:
const hasLoadingShell = route.canPrefetchLoadingShell; // false
cacheForNavigation: !hasLoadingShell && route.requiresDynamicNavigationRequest !== true, // true && false => false
renderLoadingShell: hasLoadingShell, // falseSo cacheForNavigation === false (→ shouldLearnOptimisticRouteShell === true), but renderLoadingShell === false. The header therefore becomes undefined. On main, the same route was an optimistic-route-shell prefetch (!cacheForNavigation) and thus requested APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL.
This is a real behavioral change for dynamic routes that require a fresh navigation request but have no loading boundary: previously prefetched as a loading shell; now prefetched with no render-mode header. Note optimisticRouteShell: shouldLearnOptimisticRouteShell && renderLoadingShell (link.tsx:696) also collapses to false for this route, so it is no longer cached as an optimistic route shell either.
Two things to resolve before merge:
- Decide whether the behavior change is intentional. If
requiresDynamicNavigationRequestroutes with no loading boundary should still request the shell render mode (and be learned as an optimistic route shell), the render-mode gate /optimisticRouteShellflag needs to stay tied toshouldLearnOptimisticRouteShell, not only torenderLoadingShell. - Either way,
tests/link-navigation.test.tsmust be updated in this PR so the required Vitest check is green. Since it is not in the diff, it was not re-derived against the new semantics.
Verified stale (no action) — earlier rounds were incorrect on these
For the record, several items flagged in earlier rounds are not present in the current commit and need no change. I re-checked each against HEAD:
__VINEXT_PREFETCH_INLININGis not dead. The|| __prefetchInlininggate is still inlink.tsx:616, theconst __prefetchInliningread is atlink.tsx:164, the define is still injected inindex.ts, and the declaration is still inglobal.d.ts. The flag remains consistently wired; thelink-navigation.test.ts:1927stub is still meaningful. Earlier claims that this was removed were wrong.- Scheduler is unchanged.
prefetchUrlstill useswindow.requestIdleCallback ?? ((fn) => setTimeout(fn, 100))(link.tsx:461), notsetTimeout(fn, 0). No idle-scheduling regression. staticPrefetchRegionalready uses a word boundary.app-browser-entry.ts:136uses/\bconnection\s*\(/.decodeSimpleJsxTextalready documents its single-pass intent (app-browser-entry.ts:299), with thedecodes runtime-prefetch fallback entities without double-decodingtest — the CodeQL "double unescaping" alert (scanning alert #238) is a documented false positive.replaceInnermostSuspenseChildrenalready documents thedepth > 100guard (app-optimistic-routing.ts:347), as doeselementHasSuspenseFallback(app-optimistic-routing.ts:271-272).
Soundness note (non-blocking)
The regex-based static param analysis in analyzePrefetchVary (collectParamAccesses, app-browser-entry.ts:200-251) correctly over-varies on the dangerous shapes — spread, Object.keys/values/entries/assign, computed params[...]/Reflect.get, "x" in params, and passing params/await params to a helper. That is the safe direction (over-varying is a perf hit, not wrong content). The one remaining approximation: arrow-form export const generateMetadata = async (...) => {} is not split from the page body by extractExportedFunction/removeExportedFunction (they only match export (async) function generateMetadata, app-browser-entry.ts:182-198), so its accesses are attributed to the body and the metadata-vs-body split is silently skipped. This over-varies (safe) but is worth a one-line comment noting the limitation, or handling the const form.
Verification run on this branch: tests/link.test.ts (121), tests/app-optimistic-routing.test.ts (14), and tests/app-prefetch-vary-analysis.test.ts (10) all pass. The regression is isolated to tests/link-navigation.test.ts.
|
Review posted to PR #2474. SummaryI found one blocking regression in a required Vitest check that this PR introduces. Blocking —
Verified stale (no action): I re-checked every follow-up from the earlier conflicting rounds against HEAD:
Non-blocking: arrow-form The core implementation is coherent; |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
5ce0276 to
779f36a
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed at HEAD (779f36a). Rebuilt the package and ran the affected suites. The blocking regression from the earlier rounds is resolved, and I confirmed the "verified stale" items. I did find one real soundness asymmetry in the static analysis that is worth addressing (or at least documenting) because it can serve wrong cached content.
Resolved — tests/link-navigation.test.ts no longer regresses
The prior rounds flagged uses a shell-only automatic prefetch for dynamic routes requiring fresh navigation as failing on this branch. It now passes:
✓ tests/link-navigation.test.ts (55 tests | 54 skipped)
✓ uses a shell-only automatic prefetch for dynamic routes requiring fresh navigation
Root cause was fixed in resolveMatchedAutoAppRoutePrefetch (link.tsx:344): the fallback branch now returns renderLoadingShell: hasLoadingShell || requiresDynamicNavigationRequest (previously hasLoadingShell), so a dynamic route with requiresDynamicNavigationRequest: true and no loading boundary again requests APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL. tests/link.test.ts (121), tests/app-optimistic-routing.test.ts (14), and tests/app-prefetch-vary-analysis.test.ts (10) all pass; vp check is clean on the changed files.
Soundness — searchParams access detection is weaker than params (false-negative → wrong content)
This is the direction that actually matters for correctness: a missed access → over-shared segment cache → serving one URL's content for another. collectParamAccesses (app-browser-entry.ts:200) deliberately over-varies on helper-passes and awaited aliases, but sourceAccessesSearchParams (app-browser-entry.ts:272) does not have the equivalent handling, so the two are asymmetric. Confirmed by running the actual regexes:
searchParamspassed to a helper is not detected.export default function Page({ searchParams }) { return helper(searchParams); }→sourceAccessesSearchParamsreturnsfalse, soprefetchVarySearchParamsstays unset and the segment is shared across all query strings even though the helper reads them. The param path handles the analogous shape viapassesParamsToHelper; the searchParams path has no such branch.- Prop-renamed
searchParamsis not detected.export default async function Page({ searchParams: sp }) { const q = await sp; return q.foo; }→false. (For completeness, the same prop-rename gap exists forparams—{ params: p }thenawait pis also missed — becausecollectParamAccessesonly tracksconst x = await paramsaliases, not the destructured-prop rename.)
Next.js does not hit these cases because it tracks access at runtime via the params/searchParams proxy; this is a vinext-specific limitation of the build-time regex approach, and the ported e2e rows only exercise the direct await searchParams shape. Suggested fix: give sourceAccessesSearchParams the same over-vary treatment (detect helper(searchParams) and destructured-prop renames the way collectParamAccesses does), or at minimum add a comment documenting that renamed/helper-passed searchParams/params are not tracked, plus e2e coverage for those shapes.
Verified stale (no action) — earlier conflicting rounds were outdated on these
__VINEXT_PREFETCH_INLININGis not dead: the|| __prefetchInlininggate is still atlink.tsx:617, and the define/declaration remain wired.- Scheduler is unchanged —
prefetchUrlstill useswindow.requestIdleCallback ?? setTimeout(fn, 100). No idle regression. staticPrefetchRegionuses\bconnection\s*\((app-browser-entry.ts:136).decodeSimpleJsxTextis single-pass with an explaining comment (app-browser-entry.ts:299) — the CodeQL "double unescaping" alert is a documented false positive.replaceInnermostSuspenseChildrendocuments itsdepth > 100guard (app-optimistic-routing.ts).
Nit
- Arrow-form
export const generateMetadata = async (...) => {}is not split from the page body (extractExportedFunction/removeExportedFunctiononly matchexport (async) function), so its accesses are attributed to the body. This over-varies (safe), but silently disables the metadata-vs-body split for arrow-form exports — worth a one-line comment or handling the const form. This is the same class of gap as thesearchParamsnote above.
The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based reuse) is coherent, and the regression is gone. The searchParams-vs-params detection asymmetry is the one item I'd want addressed or explicitly documented before merge.
|
Review posted to PR #2474. SummaryI rebuilt the package and ran the affected suites. Findings: Resolved — the blocking regression from prior rounds is fixed.
New soundness finding (the one item worth acting on):
I confirmed both by running the actual regexes. Next.js avoids these via runtime proxy tracking, so they aren't in the ported e2e rows. Suggested: mirror the over-vary treatment or document the limitation + add e2e coverage. Verified stale (no action): Nit: arrow-form |
james-elicx
force-pushed
the
codex/fix-segment-cache-vary-params-28478866791
branch
from
779f36a to
59376b9
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed the current HEAD (59376b9, the branch has been squashed to a single commit). I rebuilt the package (vp run vinext#build — clean) and ran the affected suites. No blocking issues remain. The blocking regression and the searchParams-detection asymmetry flagged in earlier rounds are both resolved. One small non-blocking nit remains.
Resolved — prior blocking regression is gone
tests/link-navigation.test.ts > uses a shell-only automatic prefetch for dynamic routes requiring fresh navigation now passes on this branch. The fix is in resolveMatchedAutoAppRoutePrefetch (link.tsx:344): the fallback branch returns renderLoadingShell: hasLoadingShell || requiresDynamicNavigationRequest, so a dynamic route with requiresDynamicNavigationRequest: true and no loading boundary again requests APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL.
Verification on this branch:
tests/link-navigation.test.ts— 55 passtests/link.test.ts— 121 passtests/app-optimistic-routing.test.ts— 14 passtests/app-prefetch-vary-analysis.test.ts— 14 passtests/entry-templates.test.ts(40) +tests/rsc-reference-validation-compat.test.ts(5) passvp checkclean on the changed source files
Resolved — searchParams detection is no longer asymmetric with params
The previous round flagged that sourceAccessesSearchParams missed helper-passes and prop-renamed searchParams. That is fixed at HEAD (app-browser-entry.ts:298-332): the searchParams path now collects prop aliases via collectPropAliases, tracks const x = await searchParams aliases, and detects the same dangerous shapes as the params path — spread, Object.keys/values/entries/assign, Reflect.get, computed access, and helper-passes (helper(searchParams) / helper(await searchParams), line 328-330). So both params and searchParams now over-vary in the same false-negative-avoiding way. tests/app-prefetch-vary-analysis.test.ts grew to 14 tests covering these shapes.
Verified sound
sharedCacheKeynavigation reuse (navigation.ts:494-527): the cross-URL match infindPrefetchCacheEntryForNavigationonly bypasses the URL-normalization check whenoptions.sharedCacheKeyis a non-null string andentry.sharedCacheKey === options.sharedCacheKey. Correct — reuse only happens when the analysis proved the segment doesn't vary by the differing URL portion.staticPrefetchRegionuses\bconnection\s*\((word boundary present,app-browser-entry.ts:141).decodeSimpleJsxTextis single-pass with an explaining comment (app-browser-entry.ts:354) — the CodeQL "double unescaping" alert (#238) is a documented false positive.replaceInnermostSuspenseChildrendocuments itsdepth > 100guard (app-optimistic-routing.ts:322-323).
Nit (non-blocking, safe direction)
Arrow-form export const generateMetadata = async (...) => {} is still not split from the page body. extractExportedFunction / removeExportedFunction (app-browser-entry.ts:187-203) only match export (async) function generateMetadata, so for the const/arrow form metadataSource is empty and the arrow function stays in pageBodySource. I confirmed this by running the regex. The effect is that metadata param accesses get attributed to the page body (and to runtimePrefetchParamNames), which over-varies — the safe direction, never wrong content. It just silently disables the metadata-vs-body split for arrow-form exports. Worth a one-line comment noting the limitation, or handling the const form, but not a blocker.
The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based reuse, and the over-varying regex analysis) is coherent and its unit tests pass. Good to merge once the arrow-form nit is either addressed or consciously deferred.
|
Review posted to PR #2474. SummaryI reviewed the current HEAD ( Resolved since prior rounds:
Verified sound:
Test verification (all pass): Non-blocking nit: Arrow-form |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed at HEAD (35ef1e8 — a new commit fix(app-router): detect props-based prefetch vary access sits on top of the squashed 59376b9). I rebuilt the package (vp run vinext#build — clean) and ran all affected suites. No blocking issues remain.
Verified — all prior blocking items are resolved
tests/link-navigation.test.tsno longer regresses.uses a shell-only automatic prefetch for dynamic routes requiring fresh navigationpasses (55/55). The fix is inresolveMatchedAutoAppRoutePrefetch(link.tsx:344): the fallback branch returnsrenderLoadingShell: hasLoadingShell || requiresDynamicNavigationRequest, restoring the shell render mode for dynamic routes that require a fresh navigation but have no loading boundary.searchParamsdetection is symmetric withparams. The new commit givessourceAccessesSearchParams(app-browser-entry.ts:324-363) the same over-varying treatment ascollectParamAccesses: I ran the actual regexes and confirmed it now detects sync prop access ({ searchParams }→searchParams.q), renamed props ({ searchParams: sp }→sp.q), awaited aliases, and helper-passes (helper(searchParams)). All are the safe (over-vary) direction, so no missed access → no wrong cached content.
Verified sound
sharedCacheKeycross-URL reuse is correctly gated (navigation.ts:512-524). An entry is only reused across a differing URL whenoptions.sharedCacheKeyis a non-null string andentry.sharedCacheKey === options.sharedCacheKey; otherwise the normalized-URL equality still governs. Reuse only happens when the static analysis proved the segment doesn't vary by the differing URL portion.resolveAppPrefetchSharedCacheKey(navigation.ts:432-473) folds accessed vary params into the key and leaves non-accessed params as:nameplaceholders, so distinct accessed-param values produce distinct keys.- Helper-pass over-vary is safe, not overreaching.
fn(params.category)matchespassesParamsToHelper(over-vary), whileparams.categoryalone does not — correct. staticPrefetchRegionuses\bconnection\s*\((word boundary,app-browser-entry.ts:141);decodeSimpleJsxTextis single-pass with an explaining comment (app-browser-entry.ts:384-385) — the CodeQL "double unescaping" alert #238 is a documented false positive;replaceInnermostSuspenseChildrendocuments itsdepth > 100guard (app-optimistic-routing.ts).__VINEXT_PREFETCH_INLININGis not dead — the|| __prefetchInlininggate is still wired atlink.tsx:617, and the define/declaration remain consistent. (Earlier rounds that flagged this as dead and claimed the scheduler changed tosetTimeout(fn, 0)were incorrect; the scheduler is stillrequestIdleCallback ?? setTimeout(fn, 100)atlink.tsx:462.)
Scope note (non-blocking)
This PR also carries a global-error-document refactor (default-global-error-document.tsx, error-boundary.tsx, app-ssr-entry.ts) and a new rsc-reference-validation-compat.ts plugin, which are unrelated to the vary-params title. They look correct in isolation (the SSR-only server-component mirror avoids creating a plugin-rsc client reference from the SSR environment; the validation plugin only accepts a decoded key already present in plugin-rsc's allowlist), and tests/rsc-reference-validation-compat.test.ts (5) passes. Worth a one-line note in the PR body that these are bundled fixes, so reviewers aren't surprised by the diff scope.
Nit (non-blocking, safe direction)
Arrow-form export const generateMetadata = async (...) => {} is still not split from the page body — extractExportedFunction/removeExportedFunction (app-browser-entry.ts:187-203) only match export (async) function generateMetadata. I confirmed the const/arrow form yields an empty metadataSource, so metadata param accesses are attributed to the page body (and runtimePrefetchParamNames), which over-varies — the safe direction, never wrong content. It silently disables the metadata-vs-body split for arrow-form exports and has no test/comment. Worth a one-line comment noting the limitation, or handling the const form, but not a blocker.
Test verification (all pass)
link-navigation (55), link (121), app-optimistic-routing (14), app-prefetch-vary-analysis (18), entry-templates (40), rsc-reference-validation-compat (5), and app-router/features (338). vp check clean on the changed source files.
The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based reuse, and the now-symmetric over-varying regex analysis) is coherent. Good to merge once the arrow-form nit is either addressed or consciously deferred.
|
Review posted to PR #2474. SummaryRe-reviewed at HEAD ( Verified resolved:
Verified sound:
Scope note (non-blocking): the PR bundles an unrelated global-error-document refactor + a new Nit (non-blocking, safe): arrow-form All tests pass (253 in the focused suites + 338 in app-router/features); |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): vary segment prefetches by accessed params
Re-reviewed at HEAD (bfe151d, fix(app-router): tighten segment prefetch vary analysis — a new commit on top of the previously-reviewed tree). Rebuilt and ran all affected suites. No blocking issues remain.
New commit review — the connection-detection tightening is a real correctness improvement
bfe151d fixes a genuine false-positive in the static-analysis cutoff. Previously findConnectionCallIndex truncated staticPrefetchRegion at any identifier named connection( regardless of origin. Now it only treats connection() as the Server-side cutoff when the identifier actually resolves to a next/server import:
collectNextServerConnectionIdentifiers(app-browser-entry.ts:168) scansimport { connection } from "next/server"(andnext/server.js, andconnection as alias), andfindConnectionCallIndexbails early (returnsnull) when no such import exists.
I ran the actual import regex against edge cases and confirmed it behaves correctly:
import { connection } from "next/server"→connectionimport { connection } from "next/server.js"→connectionimport { connection as waitFor } from "next/server"→waitForimport { NextResponse, connection } from "next/server"→connectionimport { connection } from "./db"→ (empty — local helper no longer truncates)import type { connection } from "next/server"→ (empty — type-only import correctly ignored, sinceimport type {isn't matched byimport\s*\{)
This is the safe direction: before, a local connection() helper or a ./db import would truncate the region early and drop later param accesses (a false negative → over-shared cache → wrong content). The three new tests (does not truncate ... on local connection helpers, ... on non-Next connection imports, uses aliased next/server.js connection imports as the static analysis cutoff) lock this in.
The commit also adds two param-access shapes to collectParamAccesses:
- destructured-from-alias:
const { itemId } = resolved;whereresolved = await params(app-browser-entry.ts:399-406) - awaited-member:
const itemId = (await params).itemId;(app-browser-entry.ts:407-414)
Both are bounded by sortedKnownParams (line 569) which filters to route.params, so any over-matching stays in the over-vary (safe) direction. Verified by running the regexes.
Verified sound (re-checked at this HEAD)
sharedCacheKeycross-URL reuse is correctly gated (navigation.ts): reuse across a differing URL only happens whenoptions.sharedCacheKeyis a non-null string and exactly matchesentry.sharedCacheKey; otherwise normalized-URL equality governs.searchParamsdetection is symmetric withparams(sourceAccessesSearchParams,app-browser-entry.ts:455): prop aliases, awaited aliases, spread,Object.keys/values/entries/assign,Reflect.get, computed access, and helper-passes are all detected (over-vary direction).decodeSimpleJsxTextis single-pass with an explaining comment (app-browser-entry.ts:521) — the CodeQL "double unescaping" alert #238 is a documented false positive.replaceInnermostSuspenseChildrendocuments itsdepth > 100guard (app-optimistic-routing.ts:324).__VINEXT_PREFETCH_INLININGremains consistently wired (gate + define + declaration); scheduler unchanged.
Scope note (non-blocking)
This PR still bundles the unrelated global-error-document refactor (default-global-error-document.tsx, error-boundary.tsx, app-ssr-entry.ts) and the new dev-only rsc-reference-validation-compat.ts plugin. I re-checked the plugin: it's apply: "serve", and shouldAcceptDecodedViteRscReferenceValidation only accepts a decoded key that is already present in plugin-rsc's clientReferenceMetaMap allowlist (it just normalizes the \0 vs __x00__ encoding mismatch), so it doesn't widen the trust boundary. Correct in isolation, but worth a one-line note in the PR body that these are bundled fixes so reviewers aren't surprised by the diff scope.
Nit (non-blocking, safe direction)
Arrow-form export const generateMetadata = async (...) => {} is still not split from the page body — extractExportedFunction/removeExportedFunction (app-browser-entry.ts:273-289) only match export (async) function generateMetadata. I confirmed the const/arrow form yields an empty metadataSource, so metadata accesses are attributed to the page body (and runtimePrefetchParamNames), which over-varies — safe, never wrong content. It silently disables the metadata-vs-body split for arrow-form exports and has no test/comment. Worth a one-line comment or const-form handling, but not a blocker.
Test verification (all pass)
app-prefetch-vary-analysis (25), app-optimistic-routing (14), link (121), link-navigation (55) — 215 total; vp check clean on the changed source files.
The core implementation (variant-keyed optimistic templates, suspense-child preservation, sharedCacheKey-based reuse, and the now-tightened over-varying regex analysis) is coherent. Good to merge once the arrow-form nit is either addressed or consciously deferred.
|
Review posted to PR #2474. SummaryRe-reviewed at HEAD ( The new commit is a real correctness improvement
Verified sound at this HEAD
Non-blocking
All 215 tests in the affected suites pass; |
|
Backlog coverage check for GitHub Actions run 28478866791 / job 84413308650: this PR looks like the likely owner for the functional Please include or rerun the exact targeted wrapper for |
# Conflicts: # packages/vinext/src/index.ts # packages/vinext/src/server/app-browser-entry.ts # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
2 participants
Summary
Covers GitHub Actions run 28478866791, job 84413308650 for
test/e2e/app-dir/segment-cache/vary-params/vary-params.test.ts.Original failing rows
Validation
vp fmt --check packages/vinext/src/client/vinext-next-data.ts packages/vinext/src/entries/app-browser-entry.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-optimistic-routing.ts packages/vinext/src/shims/link.tsx packages/vinext/src/shims/navigation.ts tests/app-optimistic-routing.test.ts tests/app-prefetch-vary-analysis.test.ts tests/e2e/app-router/nextjs-compat/vary-params.browser.spec.ts tests/fixtures/app-basic/app/nextjs-compat/segment-cache-vary-paramsvp test run tests/app-optimistic-routing.test.ts tests/app-prefetch-vary-analysis.test.tsvp check tests/app-optimistic-routing.test.ts tests/app-prefetch-vary-analysis.test.tsREPO="$(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/vary-params/vary-params.test.ts(16/16 passed)