Skip to content

fix(app-router): prefetch root-param segment trees - #2856

Open
james-elicx wants to merge 5 commits into
mainfrom
codex/fix-root-params-segment-prefetch-current
Open

fix(app-router): prefetch root-param segment trees#2856
james-elicx wants to merge 5 commits into
mainfrom
codex/fix-root-params-segment-prefetch-current

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • mark routes with dynamic params above their root layout in the client prefetch manifest
  • under Cache Components, gate automatic root-param prefetches through /_tree before requesting the concrete /__PAGE__ payload
  • apply the same sequence to both <Link> and imperative router.prefetch(), while preserving loading-shell behavior when Cache Components is disabled
  • port the upstream root-param segment-prefetch regression into the vinext browser suite

Current-main evidence

Fresh baseline on 99623542d failed 0/1 at the first /aaa prefetch because the response never contained Root param page content - param: aaa.

PR #2463 was inspected only for historical context. Its July 1 head is conflicting and substantially behind current main, with no current exact-target proof, so this implementation was rebuilt against the current route-tree and prefetch-cache architecture rather than reviving or stacking that branch.

Validation

  • REPO=\"$(pwd)\" NEXTJS_DIR=\"/Users/jamesanderson/Developer/vinext/.nextjs-ref\" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/vary-params/root-params-segment-prefetch.test.ts — 1/1 passed
  • PLAYWRIGHT_PROJECT=app-router-chrome-browser-specific pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/root-params-segment-prefetch.browser.spec.ts — 1/1 passed
  • vp test run tests/entry-templates.test.ts tests/link.test.ts tests/link-navigation.test.ts tests/prefetch-cache.test.ts tests/app-route-tree-prefetch.test.ts — 339/339 passed
  • vp check — passed

Two independent review findings were addressed before opening: Cache Components disabled-mode gating and router.prefetch() parity. Final rereview found no remaining issues.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Aug 10, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2856
npm i https://pkg.pr.new/create-vinext-app@2856
npm i https://pkg.pr.new/@vinext/types@2856
npm i https://pkg.pr.new/vinext@2856

commit: 1629a44

@github-actions

Copy link
Copy Markdown
Contributor
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

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: prefetch root-param segment trees

I traced the full change through both prefetch entry points (<Link> in link.tsx and router.prefetch() in navigation.ts), the policy resolver, the manifest projection, and all four modified test files. Ran the linked suites and vp check on the changed source — everything is green (339/339 unit tests, no lint/type errors).

Verdict

No blocking issues. The change is correct, well-scoped, and matches the two-stage /_tree/__PAGE__ segment-cache sequence. hasRootParams derives from computeRootParamNames (params strictly above the root layout position, app-route-graph.ts:1827), so the manifest flag is semantically accurate and not over-broad.

What I verified

  • Env gating parity. String(process.env.__NEXT_CACHE_COMPONENTS) === "true" (app-route-prefetch-policy.ts:94) matches the existing pattern in slot.tsx:68, and the build define compiles the value to a real boolean (index.ts:2361), so true"true" resolves correctly and the disabled state stays shell-only. Covered by the new "keeps root-param loading routes shell-only without Cache Components" test.
  • No one-request-per-route regression. In navigation.ts, prefetched.add(cacheKey) (2765) is followed synchronously by prefetchRscResponse(rscUrl, fetchPromise, …) (2819), which inserts a .pending cache entry immediately even though the page fetch is deferred inside the IIFE — so a concurrent navigation sees the pending entry, not a gap.
  • Route-tree entry is non-reusable. Both paths tag the /_tree entry cacheForNavigation: false / prefetchKind: "route-tree", so it feeds the tree gate without polluting navigation reuse. route-tree is a declared PrefetchCacheKind (navigation.ts:326).
  • Concrete keys. Tests assert the page request URL does not contain %5B…%5D / %5BrootParam%5D, confirming the concrete param (not the bracket pattern) keys the request.

Non-blocking observations

  1. Link vs router.prefetch route-tree asymmetry (navigation.ts:2777 vs link.tsx:719). link.tsx's route-tree branch, after awaiting /_tree, reads shellEntry.snapshot.renderedPathAndSearch and reuses a cached rendered-alias response via peekPrefetchResponseForNavigation before falling back to the page fetch. The navigation.ts branch skips that alias-reuse shortcut and always issues the /__PAGE__ fetch. For root-param routes the concrete key is already used so the alias rarely triggers, and both paths converge on the same cached navigation entry — so this is a missed micro-optimization, not a correctness gap. Worth a short comment noting the intentional divergence, or lifting the shared route-tree gate into a helper (aligns with the AGENTS.md guidance to keep this logic in server/*/shared helpers rather than duplicated inline).

  2. requiresRouteTreePrefetch?: true literal-only typing is consistent with the sibling requiresDynamicNavigationRequest?: true convention in the same file — fine, just flagging that all consumers correctly use === true guards.

Coverage note

Both client prefetch entry points that build segment-prefetch headers are updated; the server side (app-route-tree-prefetch.ts) already handles /_tree. No dev/prod parity gap since the shims are environment-agnostic. The added browser spec exercises the real build→prerender→prod-server path end to end.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2856.

Summary

I reviewed PR #2856 (fix(app-router): prefetch root-param segment trees) by tracing the change through both prefetch entry points, the policy resolver, the manifest projection, and all four test files. I also ran the linked test suites (339/339 pass) and vp check on the changed source (no lint/type errors).

Verdict: No blocking issues. The implementation is correct and well-scoped.

Key things I verified:

  • hasRootParams correctly derives from computeRootParamNames (params strictly above the root layout), so it's not over-broad
  • Env gating (String(process.env.__NEXT_CACHE_COMPONENTS) === "true") matches the existing slot.tsx pattern and the boolean build-define
  • No one-request-per-route regression — the pending cache entry is inserted synchronously even though the page fetch is deferred inside the IIFE
  • The /_tree entry is correctly marked cacheForNavigation: false so it doesn't pollute navigation reuse
  • Concrete param keys (not bracket patterns) are asserted in tests

Two non-blocking observations:

  1. A minor Link vs router.prefetch() asymmetry — link.tsx's route-tree branch reuses a rendered-path alias after /_tree while navigation.ts doesn't. Not a correctness gap (both converge on the same navigation entry), but worth a comment or a shared helper per AGENTS.md guidance.
  2. The literal-only requiresRouteTreePrefetch?: true typing is consistent with the sibling convention and all consumers use === true.

github run

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 1629a44 against base 4e95e97 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 134.8 KB 135.1 KB ⚫ +0.3%
Client entry size (gzip) vinext 122.3 KB 122.6 KB ⚫ +0.3%
Dev server cold start vinext 2.96 s 3.00 s ⚫ +1.2%
Production build time vinext 3.13 s 3.11 s ⚫ -0.7%
RSC entry closure size (gzip) vinext 114.3 KB 114.3 KB ⚫ -0.0%
Server bundle size (gzip) vinext 191.9 KB 192.5 KB ⚫ +0.3%

View detailed results and traces

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

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: prefetch root-param segment trees

I re-reviewed the current head (2b5068a, all 3 commits) end to end: both prefetch entry points (link.tsx, navigation.ts), the policy resolver, the manifest projection, the new freshness plumbing added in the third commit, and all four modified/added test files. Ran the targeted unit suites (332/332 pass) and vp check on the changed source (clean, no lint/type errors).

Verdict

No blocking issues. The change is correct and well-scoped. The two-stage /_tree/__PAGE__ sequence is applied consistently across <Link> and router.prefetch(), gated behind __NEXT_CACHE_COMPONENTS + hasRootParams, and the disabled-mode path stays shell-only.

What I verified

  • Manifest flag is semantically accurate. hasRootParams projects from route.rootParamNames (app-browser-entry.ts:121), which computeRootParamNames fills with params strictly above the root layout — not over-broad.
  • Env gating parity. String(process.env.__NEXT_CACHE_COMPONENTS) === "true" (app-route-prefetch-policy.ts:94) matches the slot.tsx pattern and the boolean build-define. Confirmed by the "keeps root-param loading routes shell-only without Cache Components" test (link.test.ts:424), which asserts requiresRouteTreePrefetch is absent and prefetchShellFirst: false when disabled.
  • cacheForNavigation override is intentional and covered. With requiresRouteTreePrefetch true, cacheForNavigation collapses to !hasSearchParams, overriding the prior loading-shell branch. This is the correct Cache-Components behavior (concrete-key navigation reuse) and is asserted in prefetch-cache.test.ts (navigationEntry.cacheForNavigation === true).
  • No one-request-per-route regression. prefetched.add(cacheKey) (navigation.ts:2788) precedes the deferred IIFE; prefetchRscResponse (2854) inserts a .pending entry synchronously, so concurrent navigations see the pending entry, not a gap.
  • Route-tree entry is non-reusable. Both paths tag the /_tree entry cacheForNavigation: false / prefetchKind: "route-tree", feeding the gate without polluting navigation reuse.
  • Freshness preservation (3rd commit) is sound. The restoredRscResponseExpiresAt WeakMap carries a reused rendered-alias's original expiresAt from restoreRscResponse (navigation.ts:1306) through snapshotRscResponse (1249), so an aliased route-tree reuse keeps its source expiry instead of being granted a fresh TTL. Keyed by single-use Response identity → no leak, GC-safe. getFreshPrefetchCacheEntry correctly evicts settled-but-expired entries (background-throttled timers) on foreground read, matching the existing hasPrefetchCacheEntryForNavigation pattern (703–705).
  • E2E coverage is real. The added browser spec builds a cacheComponents: true fixture, serves a production build, and asserts both /_tree and /__PAGE__ fire with concrete (non-%5BrootParam%5D) keys — exercising build→prerender→prod-server end to end.

Note on the prior review

ask-bonk's non-blocking observation #1 (Link reuses a rendered-path alias after /_tree but router.prefetch() doesn't) is no longer accurate at head. The third commit brought navigation.ts (2839–2849) to parity: it now reads renderedPathAndSearch and reuses via peekPrefetchResponseForNavigationrestoreRscResponse exactly like link.tsx (720–730). The two branches are now near-identical.

Non-blocking observation

  • Route-tree gate is duplicated inline in both link.tsx and navigation.ts. The two IIFEs are now nearly line-for-line identical (build /_tree headers → prefetch tree entry → await pending → re-read fresh → alias-reuse-or-fetch). Per AGENTS.md guidance to keep runtime behavior in shared server/*/helper modules rather than duplicated inline, lifting this into one helper would reduce the risk of the two paths drifting on a future fix. Not blocking — behavior is currently identical and both are covered by tests.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2856.

Summary

I reviewed PR #2856 (fix(app-router): prefetch root-param segment trees) at its current head (2b5068a, all 3 commits), tracing the change through both prefetch entry points, the policy resolver, the manifest projection, the new freshness plumbing, and all four test files. Ran the targeted unit suites (332/332 pass) and vp check on the changed source (clean).

Verdict: No blocking issues. The implementation is correct and well-scoped.

Key things I verified:

  • hasRootParams derives from computeRootParamNames (params strictly above the root layout) — not over-broad
  • Env gating matches the slot.tsx pattern; disabled-mode stays shell-only (covered by test)
  • The cacheForNavigation override under Cache Components is intentional and asserted in tests
  • No one-request-per-route regression — pending entry inserted synchronously
  • The 3rd commit's restoredRscResponseExpiresAt WeakMap correctly preserves the aliased route-tree entry's original expiry (GC-safe, keyed by single-use Response identity)
  • getFreshPrefetchCacheEntry correctly evicts settled-but-expired timer entries, matching an existing pattern
  • The added browser spec is a real build→prerender→prod-server E2E test

Notable findings beyond the prior ask-bonk review:

  1. ask-bonk's non-blocking observation Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1 is now outdated — the 3rd commit brought navigation.ts to full parity with link.tsx's rendered-path alias reuse, so the asymmetry it flagged no longer exists.
  2. One remaining non-blocking observation: the /_tree gate is now duplicated near line-for-line across link.tsx and navigation.ts; per AGENTS.md guidance, lifting it into a shared helper would reduce future drift risk. Not blocking.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

Backlog mapping for Actions run 31290819291: this PR appears to fix test/e2e/app-dir/segment-cache/vary-params/root-params-segment-prefetch.test.ts (does not encode root param placeholders in segment-prefetch responses). The exact targeted Next.js wrapper passes 1/1 at head c9c306d5cf6c0080fdff3c3e1f7c7a80c7fd72fc; the PR remains draft while its review loop completes.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: prefetch root-param segment trees (head c9c306d)

I re-reviewed at the current head, which now includes a 4th commit — refactor(app-router): share route-tree prefetch gate — that lifts the two-stage /_tree/__PAGE__ gate into a shared fetchRouteTreeGatedPrefetch helper in navigation.ts. This directly resolves the only outstanding non-blocking observation from the two prior ask-bonk reviews (the near line-for-line duplication between link.tsx and navigation.ts), and aligns with the AGENTS.md guidance to keep runtime behavior in shared modules rather than duplicated inline.

Ran the targeted unit suites (entry-templates, link, link-navigation, prefetch-cache) — 332/332 pass — and vp check on all five changed source files — clean, no lint/type errors.

Verdict

No blocking issues. The change is correct and well-scoped.

What I verified at this head

  • Duplication is genuinely gone. link.tsx no longer imports peekPrefetchResponseForNavigation or restoreRscResponse (confirmed no dangling refs), and both entry points now call the single fetchRouteTreeGatedPrefetch helper. The helper preserves the caller's fetch-scheduling/priority control via the injected fetchRouteTree/fetchFullRscPayload closures, so <Link> and router.prefetch() keep their distinct priorities (priority vs "low") while sharing freshness, dedup, and alias-reuse logic.
  • Policy override is semantically correct. With requiresRouteTreePrefetch true (__NEXT_CACHE_COMPONENTS === "true" + route.hasRootParams), cacheForNavigation collapses to !hasSearchParams (app-route-prefetch-policy.ts:104), overriding both the loading-shell and requiresDynamicNavigationRequest branches. This is the correct Cache-Components behavior — the /_tree gate resolves active parallel branches so the concrete /__PAGE__ is navigation-reusable. The !hasSearchParams short-circuit correctly keeps root-param routes with query strings non-reusable.
  • Freshness plumbing (restoredRscResponseExpiresAt WeakMap) is sound. restoreRscResponse records an aliased response's original expiresAt (navigation.ts:1306); snapshotRscResponse reads it back and injects it into the snapshot (1248-1250); resolvePrefetchedRscResponseExpiresAt then short-circuits on isCacheExpiresAt(cached.expiresAt) (507), so an aliased route-tree reuse keeps its source expiry instead of a fresh TTL. Keyed by single-use Response identity → GC-safe, no leak.
  • getFreshPrefetchCacheEntry await ordering is correct. entry.pending is assigned the full .then().catch().finally() chain (navigation.ts:1436), and .finally() populates entry.snapshot and clears pending (1477-1483). So the post-await re-read in the helper reliably sees the settled snapshot, and the expired-entry eviction (settled + past-expiry) matches the existing hasPrefetchCacheEntryForNavigation pattern.
  • No one-request-per-route regression. prefetched.add(cacheKey) (navigation.ts:2850) precedes the deferred IIFE; prefetchRscResponse inserts a .pending entry synchronously so concurrent navigations see the pending entry, not a gap. The route-tree entry is tagged cacheForNavigation: false / prefetchKind: "route-tree" and never pollutes navigation reuse.
  • Manifest flag is accurate, not over-broad. toLinkPrefetchRoute projects hasRootParams from route.rootParamNames?.length (app-browser-entry.ts:121), and rootParamNames is filled by computeRootParamNames with params strictly above the root layout.
  • Coverage is real. The added browser spec (root-params-segment-prefetch.browser.spec.ts) drives a cacheComponents: true fixture through build → prerender → prod-server and asserts both /_tree and /__PAGE__ fire with concrete (non-%5BrootParam%5D) keys. The link-navigation and prefetch-cache unit tests assert the two-stage header sequence, concrete keys, and cacheForNavigation === true; link.test.ts covers the disabled-mode shell-only path.

Non-blocking observations

  1. getFreshPrefetchCacheEntry deletes with notify=true. For the route-tree entry there are no userland onInvalidate subscribers, so this is a no-op in practice — but it's a behavioral difference from the pre-refactor inline cache.get. It matches the intent (a foreground read finding an expired entry should notify), and is consistent with other foreground eviction paths, so it's fine as-is; just noting the semantic isn't a plain read.
  2. requiresRouteTreePrefetch?: true literal-only typing stays consistent with the sibling requiresDynamicNavigationRequest?: true convention, and all consumers correctly use === true guards.

The prior review's asymmetry finding (Link alias-reuse vs router.prefetch()) is now moot — both entry points share the exact same helper, so they cannot drift.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2856.

Summary

I reviewed PR #2856 at its current head (c9c306d), which now includes a 4th commitrefactor(app-router): share route-tree prefetch gate — beyond the 3 commits the prior ask-bonk reviews covered. I traced the change through both prefetch entry points (link.tsx, navigation.ts), the policy resolver, the manifest projection, and the freshness plumbing, ran the targeted unit suites (332/332 pass), and ran vp check on all 5 changed source files (clean).

Verdict: No blocking issues. The implementation is correct and well-scoped.

Key findings:

  • The new refactor commit resolves the only outstanding non-blocking observation from the prior reviews — the duplicated /_tree gate is now a single shared fetchRouteTreeGatedPrefetch helper, and link.tsx has no dangling refs to the removed restoreRscResponse/peekPrefetchResponseForNavigation imports.
  • The policy override (cacheForNavigation!hasSearchParams under Cache Components + root params) is semantically correct and correctly keeps search-param routes non-reusable.
  • The restoredRscResponseExpiresAt WeakMap freshness preservation is sound and GC-safe.
  • getFreshPrefetchCacheEntry's await ordering reliably sees the settled snapshot because entry.pending is the full .then().catch().finally() chain.
  • No one-request-per-route regression; manifest flag derives from computeRootParamNames (not over-broad); E2E and unit coverage exercise the real two-stage sequence with concrete keys.

Two minor non-blocking notes (a notify=true semantic on the fresh-read eviction and the literal-only typing convention) — neither requires changes.

github run

@james-elicx
james-elicx marked this pull request as ready for review August 10, 2026 07:49
…-segment-prefetch-current

# Conflicts:
#	packages/vinext/src/shims/internal/app-route-prefetch-policy.ts
#	packages/vinext/src/shims/link.tsx
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review: prefetch root-param segment trees (head 1629a44)

I re-reviewed at the current head (post-merge with main), independently tracing both prefetch entry points (link.tsx, navigation.ts), the shared fetchRouteTreeGatedPrefetch helper, the policy resolver, the manifest projection, and the restoredRscResponseExpiresAt freshness plumbing. Ran the targeted unit suites (link, link-navigation, prefetch-cache, entry-templates) — 336/336 pass — and vp check on all 5 changed source files — clean, no lint/type errors.

Verdict

No blocking issues. The change is correct and well-scoped.

What I independently verified

  • Manifest flag is accurate, not over-broad. toLinkPrefetchRoute projects hasRootParams from route.rootParamNames?.length (app-browser-entry.ts:121), and computeRootParamNames (app-route-graph.ts:1827) fills it only with params strictly above the root layout position (routeSegments.slice(0, rootLayoutPosition)).

  • Search-param root-param routes stay non-reusable on BOTH paths. For /root-param/aaa?q=1 the policy yields requiresRouteTreePrefetch: true but cacheForNavigation: false. I traced both entry points for this case:

    • link.tsx: the /__PAGE__ header block (516-522) is gated on cacheForNavigation, so it correctly falls to the learning "1" header. gateViaRouteTree (656) is true, but the fetchPromise ternary (668-669) requires autoPrefetch.cacheForNavigation, so it falls to fetchFullRscPayload().
    • navigation.ts: the route-tree gate (2893-2894) requires reusable, so it also falls to fetchFullRscPayload().
    • Both converge on plain fetch, no route-tree gate — consistent. Asserted by the resolveAutoAppRoutePrefetch("/root-param/aaa?q=1") test in link.test.ts.
  • prefetchShellFirst invariant holds. The policy sets prefetchShellFirst: requiresRouteTreePrefetch || ..., so whenever requiresRouteTreePrefetch is true, prefetchShellFirst is guaranteed true — the gateViaRouteTree precondition can never be silently skipped.

  • Freshness plumbing is sound and GC-safe. restoreRscResponse records the aliased response's expiresAt in the restoredRscResponseExpiresAt WeakMap (1338-1340), snapshotRscResponse reads it back and injects it (1281-1282), and resolvePrefetchedRscResponseExpiresAt short-circuits on isCacheExpiresAt(cached.expiresAt) (507). The alias source is only reachable through peekPrefetchResponseForNavigation, which itself evicts expired entries before returning (1570-1573) — so a near-expired alias cannot leak a stale-but-fresh-TTL entry onto the concrete /__PAGE__ key. Keyed by single-use Response identity → no leak.

  • getFreshPrefetchCacheEntry await ordering is correct. entry.pending is the full .then().catch().finally() chain (1468) and .finally() clears pending + seeds snapshot (1507-1516), so the post-await re-read (1405) reliably observes the settled snapshot.

  • No one-request-per-route regression. prefetched.add(cacheKey) (2881) precedes the deferred IIFE; prefetchRscResponse inserts a .pending entry synchronously, so a concurrent navigation sees the pending entry rather than a gap. The /_tree entry is tagged cacheForNavigation: false / prefetchKind: "route-tree" and never pollutes navigation reuse.

  • Refactor removed the duplication cleanly. link.tsx no longer imports peekPrefetchResponseForNavigation/restoreRscResponse (confirmed no dangling refs); both entry points now call the single fetchRouteTreeGatedPrefetch helper while retaining their distinct fetch priorities (priority vs "low") via injected closures. This resolves the only outstanding non-blocking note from the prior reviews.

  • Dev/prod parity. All modified logic is in environment-agnostic client shims; the server /_tree handler (server/app-route-tree-prefetch.ts) is unchanged. The added browser spec exercises the real build → prerender → prod-server path end to end and asserts both /_tree and /__PAGE__ fire with concrete (non-%5BrootParam%5D) keys.

Non-blocking observations

  1. getFreshPrefetchCacheEntry deletes with notify=true. For a route-tree entry there are no userland onInvalidate subscribers, so this is a no-op in practice, and it matches the intent that a foreground read of an expired entry should notify (consistent with peekPrefetchResponseForNavigation at 1571 and findPrefetchCacheEntryForNavigation at 1638). Fine as-is; just flagging the semantic isn't a plain read.

  2. requiresRouteTreePrefetch?: true literal-only typing stays consistent with the sibling requiresDynamicNavigationRequest?: true convention, and all consumers correctly use === true guards.

The prior review's Link-vs-router.prefetch() asymmetry finding is moot — both entry points now share the exact same helper and cannot drift.

@ask-bonk

ask-bonk Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2856.

Summary

I reviewed PR #2856 (fix(app-router): prefetch root-param segment trees) at the current head (1629a44, post-merge with main). I independently traced both prefetch entry points (link.tsx, navigation.ts), the shared fetchRouteTreeGatedPrefetch helper, the policy resolver, the manifest projection, and the freshness plumbing — then ran the targeted unit suites (336/336 pass) and vp check on all 5 changed source files (clean).

Verdict: No blocking issues. The implementation is correct and well-scoped.

Beyond confirming the prior reviews, I independently verified some things they didn't spell out:

  • Search-param root-param routes (/root-param/aaa?q=1) stay non-reusable on both paths. This is the trickiest case because requiresRouteTreePrefetch is true while cacheForNavigation is false. I traced both link.tsx (the /__PAGE__ header block and the fetchPromise ternary are both gated on cacheForNavigation) and navigation.ts (route-tree gate requires reusable) — both correctly fall to a plain fetch with no route-tree gate. Covered by a link.test.ts assertion.
  • The prefetchShellFirst invariant guarantees the route-tree gate precondition can never be silently skipped when requiresRouteTreePrefetch is true.
  • The freshness WeakMap is safe against stale aliases because peekPrefetchResponseForNavigation evicts expired entries before returning, so a near-expired alias can't leak a fresh TTL onto the concrete key.
  • Dev/prod parity holds — all changes are in environment-agnostic client shims; the server /_tree handler is unchanged and the browser spec exercises the real build→prerender→prod path.

Two minor non-blocking notes (the notify=true semantic on fresh-read eviction, and the literal-only typing convention) — neither requires changes.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant