fix(app-router): reuse metadata rewrite prefetches

[james-elicx]

Summary

• resolve App Router rewrite aliases when looking up prefetch/navigation cache entries
• consume alternate cached RSC URLs while normalizing alias response URLs back to the visible route
• dedupe the visible rewrite alias probe so metadata-prefetch navigations stay request-free

Validation

• vp test run tests/prefetch-cache.test.ts tests/hybrid-client-route-owner.test.ts tests/link-navigation.test.ts
• vp check --fix packages/vinext/src/shims/link.tsx tests/e2e/app-router/nextjs-compat/segment-cache-metadata.browser.spec.ts
• vp run vinext#build
• PLAYWRIGHT_PROJECT=app-router-chrome-browser-specific pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/segment-cache-metadata.browser.spec.ts
• targeted Next.js E2E test/e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts passed

Independent review loop completed with no findings.

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

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

npm i https://pkg.pr.new/vinext@2455

commit: a761e60

[github-actions]

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]

Review: reuse metadata rewrite prefetches

Reviewed the alias-resolution changes across link.tsx, navigation.ts, app-browser-entry.ts, and hybrid-client-route-owner.ts. The core mechanism — resolving rewrite aliases so a prefetch cached under the rewrite destination can be reused for a navigation to the visible source URL — is implemented correctly and consistently across the prefetch path (link.tsx), the lookup helpers (navigation.ts), and the navigation consume path (app-browser-entry.ts). Type check, lint, and the three targeted test suites (link-navigation, prefetch-cache, hybrid-client-route-owner — 115 tests) all pass.

A few points worth confirming before merge.

1. mode === "full" no longer caches the warmed loading shell

packages/vinext/src/shims/link.tsx:634-654

The fetchPromise condition changed from (mode === "full" || ...) to mode !== "full" && (mode === "full-after-shell" || __prefetchInlining), and a new parallel branch fires fetchLoadingShell(false) for mode === "full".

Previously, the mode === "full" path fetched the loading shell and stored it in the prefetch cache (prefetchRscResponse(..., { optimisticRouteShell: true })), then awaited it before fetching the full payload. The new path calls fetchLoadingShell(false), whose cacheForReuse === false branch does a plain fetch(...).arrayBuffer() and discards the result — it is never inserted into the prefetch cache.

Net effect for mode === "full":

• Intended win: the full payload resolves immediately instead of being gated behind the shell.
• Possible regression: a later shell-mode prefetch of the same URL can no longer reuse a warmed optimistic shell, since it is no longer cached.

If dropping the cached optimistic shell for full mode is deliberate (server cache warming is the only goal here), this is fine — but it's an observable behavior change beyond the PR's stated scope and isn't covered by an assertion. Consider a comment noting the shell is intentionally fire-and-forget for full, or a test asserting the shell is not cached.

2. Rewrite-resolution logic is duplicated

packages/vinext/src/shims/internal/hybrid-client-route-owner.ts:72-123

resolveHybridClientRewriteHref reimplements the exact beforeFiles → afterFiles → fallback chain already in resolveHybridClientRouteOwner (lines 137-180), including the document/isDynamic short-circuits. The two must stay in lockstep — if one is updated (e.g. a new rewrite phase or a matching-order fix) and the other isn't, the alias href used for cache lookup will silently diverge from the owner decision.

Not a correctness bug today (I verified they match), but the duplication is a maintenance hazard. Extracting a shared "walk the rewrite chain" helper that both call would remove the drift risk.

3. pingVisibleLinks now gated + deferred

packages/vinext/src/server/app-browser-entry.ts:1105-1119

The re-ping is now (a) skipped when previousMountedSlotsHeader === nextMountedSlotsHeader and (b) deferred via setTimeout(0). This looks correct: the ping only re-prefetches already-visible links whose cache keys are invalidated by a mounted-slots change, and initial prefetch is driven by the IntersectionObserver (setVisibleLinkPrefetch), not this ping — so skipping it when slots are unchanged (including initial hydration with no slots, where both sides are null) doesn't drop the first prefetch. Worth double-checking the setTimeout(0) deferral doesn't reorder a ping after a rapid subsequent navigation in a way the cleanup doesn't cover; the clearTimeout on unmount handles the common case.

4. App Router prefetches bypass requestIdleCallback

packages/vinext/src/shims/link.tsx:709-715

All prefetches now run immediately when hasAppNavigationRuntime() is true, where previously low-priority (viewport) prefetches were deferred to idle. This is intentional (docstring updated, test added) and matches Next.js segment-cache eagerness, but it does run counter to the AGENTS.md guidance about keeping the common request path lightweight during initial load. Flagging so it's a conscious tradeoff rather than an accidental one.

Nits

• tests/link-navigation.test.ts waitForFetchCall has no explicit return at the end of the loop (returns undefined on timeout). Fine for the test, but the caller then reads shellFetchCall?.[1] and would assert undefined === LOADING_SHELL → a confusing failure rather than a clear timeout. A throw new Error("timed out waiting for fetch call") would give a better failure message.

Overall the change is well-tested and the alias reuse is correctly wired end-to-end. Main thing to confirm is item #1 (dropped shell caching for full mode) is intended.

[ask-bonk]

Review posted to PR #2455. Summary of my findings:

Review complete

The core change — resolving rewrite aliases so a prefetch cached under the rewrite destination is reused when navigating to the visible source URL — is correctly wired end-to-end across link.tsx (prefetch), navigation.ts (lookup/consume helpers), and app-browser-entry.ts (navigation consume + response-URL normalization). Type check, lint, and all 3 targeted test suites (115 tests) pass.

4 points flagged (none blocking):

1. mode === "full" no longer caches the warmed loading shell (link.tsx:634-654) — the biggest finding. Previously the full path cached the optimistic shell (optimisticRouteShell: true) and awaited it before fetching the full payload. Now the full payload fetches immediately (intended win) but the parallel shell fetch discards its result and is never cached, so a later shell-mode prefetch of the same URL can't reuse it. Likely intended, but it's an unasserted behavior change beyond the PR's stated scope.

2. Duplicated rewrite-resolution logic (hybrid-client-route-owner.ts:72-123) — the new resolveHybridClientRewriteHref reimplements the exact beforeFiles/afterFiles/fallback chain from resolveHybridClientRouteOwner. They match today but must stay in lockstep; a shared helper would remove drift risk.

3. pingVisibleLinks gating + setTimeout(0) deferral (app-browser-entry.ts:1105-1119) — verified correct; initial prefetch is driven by the IntersectionObserver, not this ping, so skipping on unchanged slots (incl. initial hydration) is safe.

4. App Router prefetches bypass requestIdleCallback (link.tsx:709-715) — intentional and tested, but runs counter to AGENTS.md lightweight-path guidance; flagged as a conscious tradeoff.

Plus one test nit (waitForFetchCall returns undefined on timeout instead of throwing a clear error).

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: reuse metadata rewrite prefetches (PR #2455)

Re-reviewed the alias-resolution changes across link.tsx, navigation.ts, app-browser-entry.ts, and hybrid-client-route-owner.ts. The core mechanism — resolving rewrite aliases so a prefetch cached under the rewrite destination is reused for a navigation to the visible source URL — is wired correctly and consistently end-to-end. Type check, lint, and the 3 targeted suites (prefetch-cache, hybrid-client-route-owner, link-navigation — 115 tests) all pass locally.

I want to correct two claims in the earlier automated review that don't match the code as committed, then flag the points that do stand.

Corrections to the prior review

mode === "full" still caches the warmed loading shell. The prior review stated the full path now calls fetchLoadingShell(false) and discards the shell. It actually calls fetchLoadingShell(true) (link.tsx:651), and the cacheForReuse === true branch (link.tsx:543-573) inserts the shell into the prefetch cache via prefetchRscResponse(..., { optimisticRouteShell: true }) exactly as before. The only observable change is ordering: the full payload now fetches immediately instead of being gated behind an awaited shell, and the shell warms in parallel (fire-and-forget). tests/link-navigation.test.ts:1646-1674 asserts this new ordering (full payload = call 0, shell fetched afterward). So this is not a dropped-cache regression — it is an intentional, tested reordering.

waitForFetchCall does throw on timeout. The prior nit claimed it returns undefined on timeout, producing a confusing undefined === LOADING_SHELL failure. The helper at tests/link-navigation.test.ts:201-211 ends with throw new Error("Timed out waiting for matching fetch call"), so the failure message is already clear. No change needed.

Points that stand

1. Duplicated rewrite-chain logic (maintenance hazard, not a bug). resolveHybridClientRewriteHref (hybrid-client-route-owner.ts:72-123) reimplements the same beforeFiles → afterFiles → fallback walk as resolveHybridClientRouteOwner (:137-180). They agree today (verified), but with one deliberate divergence: on a document rewrite the owner returns "document" while the href resolver returns didRewrite ? currentHref : null. Both call sites are safe with this divergence:

• In link.tsx:478-482, the owner check runs first and early-returns on "document", so the href resolver's beforeFiles-document branch is effectively unreachable there.
• In app-browser-entry.ts:1770-1777, a null result just yields an empty additionalPrefetchRscUrls, degrading gracefully to exact-URL lookup.

Still, the two walkers must stay in lockstep; a future edit to one phase in only one function would silently desync the alias key from the owner decision. Extracting a shared "walk the rewrite chain" helper would remove the drift risk. Worth a follow-up, not blocking.

2. Double hasPrefetchCacheEntryForNavigation call on the prefetch hot path. link.tsx:602-610 calls the helper once for the exact URL, then again with additionalRscUrls when the first misses. If the exact entry exists but is expired, the first call deletes it (with notifyInvalidation defaulting to true), then the second call re-scans the cache. This is not a correctness bug — notifyPrefetchInvalidated clears onInvalidateCallbacks after firing (navigation.ts:540-544) and the entry is already removed, so no double-notify — but it is a redundant full cache scan on the common viewport-prefetch path. Consider computing the alias match in a single lookup pass (the helper already accepts additionalRscUrls, so the exact+alias check could be folded into one call and hasExactNavigationCacheEntry derived from the matched cacheKey).

3. pingVisibleLinks gating + setTimeout(0) deferral — verified correct. app-browser-entry.ts:1105-1119 skips the re-ping when previousMountedSlotsHeader === nextMountedSlotsHeader and defers via setTimeout(0). getMountedSlotsHeader() reads the persisted global before setMountedSlotsHeader(next) overwrites it, so previous is captured correctly; on initial hydration both are null and the ping is skipped, which is safe because first-paint prefetch is driven by the IntersectionObserver (setVisibleLinkPrefetch), not this ping. The clearTimeout cleanup covers unmount/rapid re-render.

4. App Router prefetches now bypass requestIdleCallback. link.tsx:709-715 runs all prefetches immediately when hasAppNavigationRuntime() is true; previously low-priority viewport prefetches deferred to idle. Intentional (docstring updated, tests/link-navigation.test.ts:1326-1352 added) and matches Next.js segment-cache eagerness. Combined with point 3, note that a mounted-slots change now triggers an immediate synchronous re-prefetch of every visible app link inside the deferred ping — bounded by visible-link count, but flagging it as a conscious tradeoff against the AGENTS.md "keep the common request path lightweight" guidance.

Verdict

No blocking issues. The alias reuse is correctly implemented and well-tested. The earlier review's headline finding (#1) does not reproduce against the committed code. Remaining items are a maintenance-hazard follow-up (duplicated walker) and a minor hot-path optimization (double cache scan).

[ask-bonk]

Review posted to PR #2455.

Summary

I re-reviewed the alias-reuse changes. The core mechanism — resolving rewrite aliases so a prefetch cached under the rewrite destination is reused for a navigation to the visible source URL — is correctly wired across link.tsx (prefetch), navigation.ts (lookup/consume), and app-browser-entry.ts (navigate consume + response-URL normalization). Type check, lint, and all 3 targeted suites (115 tests) pass.

Corrected two factual errors in the earlier automated review:

1. mode === "full" still caches the loading shell. The prior review claimed the full path calls fetchLoadingShell(false) and discards the shell. It actually calls fetchLoadingShell(true) (link.tsx:651), which caches via prefetchRscResponse(..., { optimisticRouteShell: true }) exactly as before. The only change is ordering (full payload fetches immediately, shell warms in parallel) — and it's asserted by tests/link-navigation.test.ts. Not a regression.

2. waitForFetchCall does throw on timeout (tests/link-navigation.test.ts:211), contradicting the prior nit that claimed it returns undefined.

Points that stand (none blocking):

• Duplicated rewrite-chain walker in hybrid-client-route-owner.ts — a maintenance hazard; both call sites are safe today but the two walkers must stay in lockstep. Follow-up refactor suggested.
• Double hasPrefetchCacheEntryForNavigation call on the viewport-prefetch hot path (link.tsx:602-610) — not a correctness bug (no double-notify), but a redundant cache scan that could be folded into one lookup.
• pingVisibleLinks gating + setTimeout(0) — verified correct.
• App Router prefetches bypass requestIdleCallback — intentional and tested; flagged as a conscious tradeoff, especially combined with the now-immediate re-ping of visible links on mounted-slots changes.

Verdict: no blocking issues; the PR's headline concern from the prior review does not reproduce against the committed code.

github run

[NathanDrake2406]

Requesting changes on structure.

The alias reuse direction looks right, but resolveHybridClientRewriteHref duplicates the rewrite traversal already owned by resolveHybridClientRouteOwner: beforeFiles, the dynamic-route gate, afterFiles, fallback, and document bailout now exist in two separate flows.

That makes the owner decision and the cache-alias href drift-prone. These are not separate concepts; they are two facts produced by the same route-resolution walk. Can we extract a single resolver that walks rewrites once and returns both the owner and the rewritten href, then have resolveHybridClientRouteOwner and the cache-alias path consume that shared result?

Something like:

type HybridClientRouteResolution =
  | { kind: "document" }
  | {
      kind: "route";
      owner: HybridClientOwner | null;
      href: string;
      rewritten: boolean;
    };

That would remove the duplicated state machine and make future rewrite-order changes much safer.

I also think prefetchUrl is carrying too much of this policy directly now: rewrite alias resolution, alias cache candidates, shell/full fetch orchestration, and the alias HEAD probe are all embedded in the same already-busy function. After the shared route-resolution helper exists, I’d prefer the cache-candidate construction to move behind a small helper as well, so link.tsx is not manually threading additionalRscUrls as an ad-hoc side channel.

[james-elicx]

I think this PR covers the segment-cache metadata failures from run 28478866791 / report job 84413308650. The report has exactly the two non-cacheComponents failures in test/e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts: dynamic content + dynamic metadata with a full prefetch, and runtime-prefetchable content + dynamic metadata with a runtime prefetch. This PR adds the matching dynamic-head/runtime-prefetchable-head metadata-prefetch regression coverage and its validation notes say the exact targeted upstream metadata E2E passed.

[james-elicx]

Backlog mapping for GitHub Actions run 28478866791 / job 84413308650: this PR appears to own the App Router metadata rewrite/prefetch failures in test/e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts.

Per instruction I am skipping a duplicate implementation because this PR is by james-elicx. Remaining work looks like addressing review feedback and rerunning the exact metadata targeted Next.js E2E proof.

[github-actions]

Performance benchmarks

Compared b7ac399 against base ebf8e59 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	125.0 KB	125.6 KB	⚫ +0.4%
Client entry size (gzip)	vinext	119.4 KB	119.8 KB	⚫ +0.3%
Dev server cold start	vinext	2.54 s	2.51 s	⚫ -1.3%
Production build time	vinext	3.12 s	3.10 s	⚫ -0.6%
RSC entry closure size (gzip)	vinext	96.8 KB	96.8 KB	⚫ -0.0%
Server bundle size (gzip)	vinext	163.2 KB	163.2 KB	⚫ -0.0%

View detailed results and traces

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