fix(app-router): reuse runtime segment prefetch payloads#2476
Conversation
…he-prefetch-scheduling-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx
…he-prefetch-scheduling-28478866791
…he-prefetch-scheduling-28478866791
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: reuse runtime segment prefetch payloads
Reviewed the three-hunk diff in packages/vinext/src/shims/link.tsx. The change is small, well-scoped, and correct. Targeted tests pass (42 passed | 18 skipped) and vp check is clean on the touched file.
What the change does (confirmed correct)
Hunk 1 (cacheForNavigation: mode === "segment") — This makes the segment-phase prefetch entry navigation-reusable while keeping route-tree at false, matching the PR description.
I traced the reuse mechanism end to end to confirm this actually works:
- The segment request bakes
Next-Router-Segment-Prefetch: /_pageinto the RSC cache-busting hash (createCacheBustingInput,app-rsc-cache-busting.ts:172), so the segment entry's exactrscUrldiffers from a navigation request's URL. - Navigation reuse still succeeds via the fuzzy-match loop in
findPrefetchCacheEntryForNavigation(navigation.ts:455-465), which comparesnormalizeRscCacheLookupUrl(...)afterstripRscCacheBustingSearchParam(navigation.ts:395-403). So the differing/_pagehash does not block the match. - Reuse is only safe because vinext serves the full unified page payload for
/_page(the marker is only a scheduler-phase discriminator, per the comment atlink.tsx:527-530), not a partial Next.js segment payload. If that ever changes, this line would need to revert tofalse.
I also verified the surrounding conditionals are unaffected by flipping segment to true:
isOptimisticRouteShellPrefetch(link.tsx:509-510) staysfalsefor segment because of themode !== "segment"guard.shouldSendSegmentPrefetchHeaders(link.tsx:521) staysfalse; the/_pageheader still comes from the dedicatedelse if (mode === "segment")branch.- The new-for-segment navigation-freshness gate (
link.tsx:554-559) now short-circuits a segment prefetch when a navigation-reusable entry already exists — this is the desired behavior (a full/nav entry supersedes a segment one).
Hunks 2 & 3 (void prefetchUrl(...)) — Correct floating-promise cleanup at link.tsx:772 and link.tsx:1005. I checked all four prefetchUrl call sites: the remaining unvoided one at link.tsx:963 is properly chained with .catch().finally(), so the file is now consistent.
One gap worth noting (non-blocking)
There is no test asserting the core behavioral change. The existing scheduling tests (tests/link-navigation.test.ts:1320, :1429) assert the /_page request headers but not the resulting entry.cacheForNavigation for the segment-phase entry. The nearby full/loading-shell tests already assert cacheForNavigation (:1780, :1845), so an equivalent assertion for the segment phase — ideally a round-trip showing a segment prefetch is consumed by a subsequent navigation — would lock in this fix and guard against a future regression that silently flips it back to false. The linked Next.js E2E (prefetch-runtime.test.ts) covers this at the integration level, but a unit assertion here would be cheaper insurance.
No correctness blockers. LGTM.
|
Review posted to PR #2476. SummaryReviewed the single-file, 3-hunk change in Findings:
One non-blocking gap: No unit test asserts the core behavioral change — existing tests check the Verification: targeted tests pass (42 passed, 18 skipped), |
|
Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the App Router fully-static runtime-prefetch row from Exact row:
Since this PR is already James-owned, I am skipping duplicate implementation work for this row. I am not counting broader cacheComponents/use-cache rows here. |
4c23523 to
f584994
Compare
…he-prefetch-runtime-remaining-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
…-scheduling-28478866791' into codex/fix-segment-cache-prefetch-runtime-remaining-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Summary
Next.js parity
Targets the App Router Segment Cache behavior covered by:
test/e2e/app-dir/segment-cache/prefetch-runtime/prefetch-runtime.test.tsThis fixes the run 28478866791 / job 84413308650 row:
runtime prefetching > can completely prefetch a page that is fully staticStacked on #2465 because this row depends on the route-tree -> segment-prefetch scheduling path.
Validation
vp test run tests/link-navigation.test.ts -t "Link prefetch scheduling|runtime-prefetch|segment|route-tree|full prefetch"— 42 passed, 18 skippedvp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.ts— cleanREPO="$(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/prefetch-runtime/prefetch-runtime.test.ts— 1 passed, 33 skipped