Skip to content

fix(app-router): prefetch root-param route trees - #2463

Draft
james-elicx wants to merge 1 commit into
mainfrom
codex/fix-root-params-segment-prefetch-28478866791
Draft

fix(app-router): prefetch root-param route trees#2463
james-elicx wants to merge 1 commit into
mainfrom
codex/fix-root-params-segment-prefetch-28478866791

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • emit root-param route-tree metadata to the App Router Link prefetch manifest
  • request segment route-tree prefetches for root-param links without encoding placeholder segments
  • keep viewport visibility gated through IntersectionObserver while scheduling the route-tree task immediately with low fetch priority

Validation

  • vp test run tests/link.test.ts tests/link-navigation.test.ts -t "root-param|Link App Router prefetch mode|route-tree segment prefetch"
  • vp check on changed root-param files
  • targeted Next.js e2e: test/e2e/app-dir/segment-cache/vary-params/root-params-segment-prefetch.test.ts
  • independent final re-review: NO FINDINGS

@pkg-pr-new

pkg-pr-new Bot commented Jul 1, 2026

Copy link
Copy Markdown

Open in StackBlitz

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

commit: 4fa8cfa

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@github-actions

github-actions Bot commented Jul 1, 2026

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 Jul 1, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review — PR #2463 (prefetch root-param route trees)

Reviewed the 6 changed files. The change is well-tested (unit + navigation + a ported Next.js e2e) and the core wiring is correct: the manifest now flags root-param routes via shouldPrefetchRouteTree, resolveAutoAppRoutePrefetch returns a route-tree branch, and prefetchUrl sets Next-Router-Prefetch: 1 + Next-Router-Segment-Prefetch: /_tree, schedules immediately (viewport-gated), and skips the shell-first second fetch. Navigation reuse works because normalizeRscCacheLookupUrl strips the cache-busting param, so the route-tree entry (cacheForNavigation: true) is still matched on click.

A couple of non-blocking observations:

1. pingVisibleLinkPrefetches drops the immediate schedule for route-tree links (minor)

packages/vinext/src/shims/link.tsx:779

function pingVisibleLinkPrefetches(): void {
  for (const instance of visibleLinkPrefetches) {
    if (instance.isVisible && instance.routerMode === "app") {
      prefetchUrl(instance.href, instance.mode, "low", instance.pagesRouteHref);
    }
  }
}

Unlike setVisibleLinkPrefetch (link.tsx:761), the re-ping path does not pass instance.prefetchRouteTree ? "immediate" : "idle". On a re-ping (e.g. after a navigation invalidates the prefetch cache), a root-param route-tree prefetch falls back to idle scheduling instead of immediate. The request headers are still correct because prefetchUrl re-resolves via resolveAutoAppRoutePrefetch, so this is a scheduling-priority inconsistency only, not a correctness bug. Consider forwarding the same schedulePriority here for parity with the initial reveal.

2. prefetchRouteTree scheduling flag isn't gated on mode === "auto" (minor)

packages/vinext/src/shims/link.tsx:1087

const prefetchRouteTree =
  routerMode === "app" && resolveAutoAppRoutePrefetch(hrefToPrefetch).prefetchRouteTree;

This is computed from resolveAutoAppRoutePrefetch regardless of the link's actual prefetchMode. For a root-param link with prefetch={true} (mode "full") or unstable_dynamicOnHover ("full-after-shell"), the instance still gets prefetchRouteTree: true, so it is scheduled "immediate" — even though prefetchUrl then takes the non-auto branch and never sends the /_tree headers. Net effect: an explicit-full root-param link skips idle scheduling for no route-tree benefit. Gating this on prefetchMode === "auto" would keep the immediate-schedule optimization scoped to where it actually applies.

Note (not a regression): server ignores the segment header

The server derives renderMode solely from VINEXT_RSC_RENDER_MODE_HEADER (app-rsc-request-normalization.ts:140); Next-Router-Segment-Prefetch only feeds the cache-busting hash (app-rsc-cache-busting.ts:172). So a /_tree prefetch returns a full navigation render rather than a tree-only payload. That matches the existing "unified route payload" tradeoff already documented at link.tsx:585-588, and the e2e asserts the full body content, so this is expected behavior for now — flagging only so it's a deliberate, documented choice.

Both flagged items are minor and don't block; targeted unit tests (tests/link.test.ts, tests/link-navigation.test.ts) pass locally.

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2463. Summary of findings:

Review Summary

The change correctly wires root-param route-tree prefetching end-to-end. Core mechanism is sound and well-tested. I found 2 minor, non-blocking issues and 1 documented-tradeoff note:

1. pingVisibleLinkPrefetches drops immediate scheduling (link.tsx:779) — the re-ping path doesn't forward instance.prefetchRouteTree ? "immediate" : "idle" like the initial reveal does (link.tsx:761). Headers are still correct (re-resolved), so it's a scheduling-priority inconsistency only.

2. prefetchRouteTree flag not gated on mode === "auto" (link.tsx:1087) — computed via resolveAutoAppRoutePrefetch regardless of the link's actual mode. A prefetch={true} root-param link gets scheduled "immediate" even though prefetchUrl won't send /_tree headers in full mode. Minor wasted immediate-schedule.

3. Note (expected): the server ignores Next-Router-Segment-Prefetch for render mode — it only feeds the cache-busting hash. A /_tree prefetch returns a full render, consistent with the existing "unified route payload" tradeoff at link.tsx:585-588 and asserted by the e2e.

Both flagged items are minor. Targeted unit tests (tests/link.test.ts — 121 passed, tests/link-navigation.test.ts — 56 passed) pass locally.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

This appears to cover the current run 28478866791 / job 84413308650 failure in test/e2e/app-dir/segment-cache/vary-params/root-params-segment-prefetch.test.ts:

  • segment cache - root params segment prefetch > does not encode root param placeholders in segment-prefetch responses

Since this PR is already owned by james-elicx, I am treating it as the owner for that failure class and skipping duplicate implementation work. The remaining coordinator risk is the current draft/dirty/red state rather than missing ownership.

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