fix(navigation): reuse router.prefetch payloads during navigation#2709
fix(navigation): reuse router.prefetch payloads during navigation#2709NathanDrake2406 wants to merge 4 commits into
Conversation
link.tsx is a "use client" React module, so its prefetch policy helpers (resolveAutoAppRoutePrefetch, resolveFullAppRoutePrefetch and their supporting trie/route-href logic) could not be reached from navigation.ts, which must stay importable without React. Move them verbatim into shims/internal/app-route-prefetch-policy.ts (same layering as internal/app-route-detection.ts) and name the previously inline policy shape AppRoutePrefetchPolicy. link.tsx re-exports canAutoPrefetchFullAppRoute and resolveAutoAppRoutePrefetch, keeping its public test surface unchanged. No behaviour change.
router.prefetch() stored every response as a learning-only cache entry (cacheForNavigation: false), so a router.prefetch(x) followed by router.push(x) always issued a second RSC request; even an in-flight programmatic prefetch was invisible to navigation. Next.js reuses router.prefetch() results (PrefetchKind.AUTO by default, kind: "full" caches the full payload), and <Link> prefetches in vinext already produce reusable entries through the shared route-policy helpers. Resolve the same policy in router.prefetch(): kind: "full" maps to the full-prefetch policy, anything else to the auto policy for the target route. Reusable prefetches send the same segment-prefetch headers as <Link>, gate re-issue on a freshness-aware navigation cache probe (discarding a stale learning-only twin first), and store the payload with navigation TTLs plus ahead-of-navigation snapshot preparation. Routes the auto policy declines (no manifest match, loading shell, search params) keep the previous learning-only fetch, preserving optimistic-route-template learning and matching Next.js AUTO semantics for dynamic routes. In-flight sharing needs no extra machinery: consumePrefetchResponseForNavigation already awaits a pending entry once the entry is no longer learning-only. Fixes cloudflare#2707
Covers issue cloudflare#2707 end to end on the /top -> /film/[imdbId] interception fixture: a programmatic router.prefetch followed by a Link click must produce exactly one /film RSC request while still rendering the intercepted film panel. A companion test without the prefetch pins the request counter so the reuse assertion cannot pass vacuously.
|
@codex review |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a587ba57b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const policy = | ||
| kind === "full" ? resolveFullAppRoutePrefetch() : resolveAutoAppRoutePrefetch(prefetchHref); |
There was a problem hiding this comment.
When a configured rewrite maps an href that also matches an App Router manifest entry to a different App route, this selects policy metadata for the source while the request and navigation resolve the destination. If the destination has a loading boundary or sets requiresDynamicNavigationRequest, the prefetch is incorrectly marked reusable and sent with full-payload headers instead of remaining a loading-shell/learning-only request. The Link path already avoids this by calling resolveHybridClientRewriteHref before resolveAutoAppRoutePrefetch; apply the same resolution here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a78870c: the prefetch closure now loads the rewrite-aware hybrid module first and resolves the policy (and the freshness-gate lookup, via additionalRscUrls) against the rewritten destination, mirroring Link's prefetchPolicyHref.
| import { | ||
| resolveAutoAppRoutePrefetch, | ||
| resolveFullAppRoutePrefetch, | ||
| } from "./internal/app-route-prefetch-policy.js"; |
There was a problem hiding this comment.
This static import makes every next/navigation consumer load and evaluate the prefetch policy and its route-trie dependencies, even when an application only uses hooks such as usePathname() and never calls router.prefetch(). Since this is a feature-specific path in a shared App Router module, load it inside the existing asynchronous prefetch closure instead so it stays off the common startup path.
AGENTS.md reference: AGENTS.md:L479-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a78870c: the policy module is now imported dynamically inside the async prefetch closure, keeping the route-trie dependencies off the next/navigation startup path.
| if (reusable && kind === "auto") { | ||
| headers.set(NEXT_ROUTER_PREFETCH_HEADER, "1"); | ||
| headers.set(NEXT_ROUTER_SEGMENT_PREFETCH_HEADER, __prefetchInlining ? "/__PAGE__" : "1"); |
There was a problem hiding this comment.
With experimental.prefetchInlining enabled, an AUTO prefetch for an eligible static route goes directly to Next-Router-Segment-Prefetch: /__PAGE__. The server computes the route tree and its inlining hints only for a preceding /_tree request, and the Link implementation deliberately gates the page request behind that stage. Programmatic prefetches therefore bypass the configured inlining protocol and lose its segment-selection benefit; use the same route-tree-first sequence here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one: in vinext the server's only special segment-prefetch handling is /_tree (isRouteTreePrefetchRequest in server/app-route-tree-prefetch.ts); a Next-Router-Segment-Prefetch: /__PAGE__ request returns the same unified route payload with or without a preceding tree request, and that unified payload is what gets cached for navigation. Link's tree-first stage exists for request-timing parity so duplicate visible links observe the full payload as pending (see the comment above gateViaRouteTree in link.tsx) — a viewport-race concern that doesn't apply to a single programmatic call. Listed as a deliberate non-goal in the PR description.
| if (hasPrefetchCacheEntryForNavigation(rscUrl, interceptionContext, mountedSlotsHeader)) { | ||
| attachPrefetchInvalidationCallback(cacheKey, options?.onInvalidate); | ||
| return; |
There was a problem hiding this comment.
When the freshness check succeeds through a normalized URL or rendered-path alias rather than the exact new key—for example, a prior <Link prefetch={true}> cached the same route under a different _rsc header variant—hasPrefetchCacheEntryForNavigation returns true, but attachPrefetchInvalidationCallback(cacheKey, ...) looks up only the absent exact key. The call then returns without registering onInvalidate, so the documented callback never fires when the reused entry expires; propagate the matched entry/key from the lookup and attach to that instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a78870c: hasPrefetchCacheEntryForNavigation now accepts onInvalidate and attaches it to whichever entry it matched (exact, normalized _rsc variant, or alias). Regression test: "attaches onInvalidate when reuse matches an entry under a different _rsc variant" in tests/prefetch-cache.test.ts.
…h reuse Address review findings on the router.prefetch reuse path: - A configured client rewrite can map the prefetched href onto a different App route. The policy previously described the source pattern, so a destination with a loading boundary or a requires-dynamic flag was incorrectly marked reusable. Load the rewrite-aware hybrid module first and resolve the policy (and the freshness-gate lookup) against the rewritten destination, mirroring Link's prefetchPolicyHref. - When the freshness gate matched a reusable entry under a normalized _rsc variant or rendered-path alias, onInvalidate was attached via an exact-key lookup that silently missed, so the documented callback never fired on expiry. hasPrefetchCacheEntryForNavigation now attaches the callback to whichever entry it matched. - Import the prefetch policy module dynamically inside the prefetch closure so its route-trie dependencies stay off the startup path of every next/navigation consumer (AGENTS.md performance guidance).
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Overview
router.prefetch()results reusable by a subsequent navigation_appRouter.prefetchresolves the same prefetch policy as<Link>instead of always storing a learning-only entryshims/internal/app-route-prefetch-policy.tssonavigation.ts(no React) andlink.tsx("use client") share one decisionrouter.prefetch(x)+router.push(x)/ Link click issues one RSC request instead of two; in-flight programmatic prefetches are shared with navigationCloses #2707.
Why
Every prefetched payload must either be safe to commit on navigation or be marked learning-only.
router.prefetch()hardcoded the learning-only shape ({ cacheForNavigation: false, optimisticRouteShell: true }), so all navigation-reuse paths (findPrefetchCacheEntryForNavigation,consumeMatchedPrefetchResponse,peekPrefetchResponseForNavigation) skipped its entries by design, and every navigation refetched. Next.js reusesrouter.prefetch()results: it defaults toPrefetchKind.AUTOand caches the full payload forkind: "full"(app-router-instance.ts).vinext already encodes exactly that reuse decision for
<Link>inresolveAutoAppRoutePrefetch/resolveFullAppRoutePrefetch, keyed off the client prefetch route manifest. The right owner of the decision is that shared policy, not a second hardcoded literal in the router, so this PR moves the helpers to an internal module and hasrouter.prefetchconsult them.What changed
router.prefetch(x)then navigate tox(route without loading shell / search params)router.prefetch(x)is still in flightconsumePrefetchResponseForNavigationpathrouter.prefetch(x, { kind: "full" })Next-Router-Prefetchsuppressed on the wire (matches<Link prefetch={true}>)getPrefetchInterceptionContext, which already keys prefetch and navigation identically (covered by the new e2e on the/top→/film/[imdbId]modal fixture)<Link>prefetching,form.tsxprefetchinglink.tsxre-exports the moved helpers so its public test surface is unchangedMaintainer review path
packages/vinext/src/shims/internal/app-route-prefetch-policy.ts: verbatim extraction fromlink.tsx(first commit is mechanical only).packages/vinext/src/shims/navigation.ts(_appRouter.prefetch): the behavioural decision — kind resolution, policy lookup, reusable vs learning branch, freshness gate (discardLearningOnlyPrefetchCacheEntry+hasPrefetchCacheEntryForNavigation), and the reusableprefetchRscResponsebehaviour object mirroring Link's.tests/prefetch-cache.test.ts: unit proof for reuse, in-flight sharing,kind: "full", and the preserved learning-only fallback.tests/e2e/app-router/router-prefetch-reuse.spec.ts: end-to-end request counting across an intercepted navigation.Validation
kind: "full"reusable on a loading-shell route withNext-Router-Prefetchabsent; loading-shell route stays learning-only under the default kind.app-router, dev):router.prefetch+ click on the film interception fixture produces exactly one/film/*RSC request and still renders the intercepted panel; a companion no-prefetch test pins the request counter.vp checkclean on all touched files.Commands
Risk / compatibility
window.next.router.prefetchgains the documented Next.jskindoption;PrefetchOptions.kindstaysunknownat the shim boundary and anything other than"full"falls back to auto, matching Next'sdefault:branch.router.prefetch). Dev-mode e2e that depend on per-navigation requests are protected by the loading-shell policy and were re-run.minimumTtlMs: 0honours server stale-times,PREFETCH_CACHE_TTLfallback; encoded intests/link-navigation.test.tsstale-time tests).link.tsx/prefetchRscResponseregion as open PRs fix(app-router): support instant route prefetch shells #2374 and fix(app-router): full prefetch static loading routes #2446. The extraction commit is a verbatim move to keep rebases cheap; this PR depends on neither.Non-goals
fetchLoadingShellForReuse/ full-after-shell sequencing,searchAgnosticShellprobes, and route-tree gating are not replicated forrouter.prefetch. Those optimise viewport/hover races that do not apply to a programmatic call; an in-flightrouter.prefetchentry is already shared with navigation by awaitingentry.pending.Router.prefetchorform.tsxprefetching.References
router.prefetch+router.pushalways double-fetchesapp-router-instance.ts(prefetch, lines 392-438)kind: "full", navigation reuse