fix(app-router): full prefetch static loading routes#2446
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — LGTM
Focused, correct fix. Verified locally: vp check clean on the 3 changed files, and vp test run tests/link.test.ts tests/link-navigation.test.ts → 176 passed.
What the change does
resolveMatchedAutoAppRoutePrefetch now returns cacheForNavigation: !hasLoadingShell || !route.isDynamic. The only behavioral delta is static routes with a loading boundary (e.g. /settings): they flip from shell-only (cacheForNavigation: false) to full prefetch (true). The truth table is unchanged for every other combination:
| Route | isDynamic | hasLoadingShell | cacheForNavigation | prefetchShellFirst |
|---|---|---|---|---|
/about |
false | false | true | true |
/settings |
false | true | true (was false) | true |
/blog/:slug |
true | true | false | false |
/products/:id |
true | false | true | false |
This matches Next.js: a fully-static route is fully prefetchable even with a loading.js boundary, while dynamic loading-boundary routes stay shell-only so loading.js can still commit on navigation.
Things I checked
- Downstream flow (
automode): withcacheForNavigation: true,isOptimisticRouteShellPrefetchisfalse, sorenderModeisundefined(full RSC), and neither the render-mode header norNEXT_ROUTER_PREFETCH_HEADERis set. The shell-firstfetchPromisebranch (link.tsx:540–544) is gated onmode === "full" | "full-after-shell" | __prefetchInlining, soautomode goes straight to a full fetch. Matches the new test's assertions (optimisticRouteShell: false, no prefetch/render-mode headers). - Single source of truth: the loading-shell decision lives only in
resolveMatchedAutoAppRoutePrefetch; no duplicated prefetch logic in server entries, so no dev/prod parity gap to sync. prefetchShellFirststilltruefor static routes is harmless inautomode (branch not taken). Under__VINEXT_PREFETCH_INLINING=truea static loading-shell route inautomode will now take the shell-first path (previously short-circuited bycacheForNavigation: false); this is consistent with the Segment CacheprefetchInliningcontract rather than a regression.
Minor notes (non-blocking)
- The new
linkPrefetchRoutesentry (metadata-await-promise/nested) is a synthetic manifest entry for the isolated unit test — no real fixture/loading.jsis required since fetch is stubbed. That's fine for this unit test. - Tests are well-labeled and the Next.js parity references are included. Nice.
No correctness or parity issues found. Approving.
|
Review posted to PR #2446. SummaryReviewed PR #2446 ( The change is a one-line logic fix in What I verified:
The new synthetic |
|
This looks like it covers the run 28478866791 / job 84413308650 failure for |
|
Backlog mapping for GitHub Actions run 28478866791 / job 84413308650: this PR appears to own the App Router static loading-boundary/full-prefetch failures in Per instruction I am skipping a duplicate implementation because this PR is by |
|
Run 29871622126 / job 88775791401 still fails |
38c752d to
af91241
Compare
|
Refreshed onto current main Current-main exact proof reproduced the owned failure: Focused validation: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Updated the independent-review blocker at The full-prefetch decision no longer treats a fixed filesystem pathname as proof of a static render. Automatic loading-boundary prefetches now use the RSC response policy as the authority: Regression coverage includes a fixed Validation:
Ready for a fresh independent review of this exact head; no Bonk/ready transition requested yet. |
|
Updated the branch after the independent review findings. The cache-policy concern was resolved against the Next.js oracle rather than by adding vinext-only public-cache proof headers. Next's Segment Cache explicitly treats a Full prefetch as a navigation issued ahead of time and intentionally stores/reuses dynamic Full responses; it does not reject them based on
The revised behavior is:
Validation at
|
|
Fresh-review P1 fixes are now pushed at What changed:
New regression coverage verifies:
Validation:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — LGTM (no issues found)
Re-reviewed the current head 2464ce6, which has grown well past the original one-line fix. Verified locally: vp check clean on all 5 changed files, and the focused suites pass — tests/link.test.ts (127), tests/link-navigation.test.ts (74), tests/prefetch-cache.test.ts (43), tests/entry-templates.test.ts (48), plus tests/app-router.test.ts + tests/features.test.ts (349) with no regressions. 292/292 across the four directly-touched files.
What the change now does
The policy no longer trusts filesystem shape (isDynamic, fixed pathname, or generateStaticParams) as proof of staticness. Instead:
AppRoutePrefetchPolicysplits the oldcacheForNavigationboolean into three concepts:fetchFullPayload(issue a Full request),cacheForNavigationOnComplete(become reusable only after EOF snapshot), and a residualcacheForNavigationthat startsfalsefor every newly-started Full request.- Automatic loading-boundary routes carry
automaticFullPrefetchProbe: "loading-shell". The probe fetches the loading shell, andresponseProvesAutomaticFullPrefetchEligibilitygates the Full request on positive shared-cache evidence (s-maxage>0orX-Vinext-Cache: HIT|STALE|STATIC). No proof → the Full request throws and the entry self-deletes, leaving only the loading shell. - A completed Full prefetch flips to
cacheForNavigation: trueafter its snapshot reaches EOF, so a click never joins a still-open stream.findProvisionalFullPrefetchEntryForNavigationdedupesfull-after-shellintent against a pending provisional entry.
Correctness checks I ran through
- Reject/throw path is clean. The
!eligiblethrow rejectsfetchPromise;prefetchRscResponse's.catchcallsdeletePrefetchCacheEntry, so ineligible Full attempts don't linger. The separately-cached loading shell survives and remains the navigation path. ✓ - Alias ordering. In the
.then,entry.cacheForNavigation = true(line 1069) runs beforeaddRenderedPathAndSearchPrefetchAlias(line 1071), so thecacheForNavigation === falseearly-return at nav.ts:1120 doesn't suppress the alias. ✓ promotePrefetchEntriesForNavigationnow skipscacheForNavigationOnComplete === trueentries (link.tsx:966), so a provisional Full entry can't be force-promoted mid-stream. ✓learnOptimisticRouteTemplatesFromPrefetchCacheskips ineligible probe shells (automaticFullPrefetchEligible === false), so a live RSC stream stays authoritative. The=== falseguard is scoped — non-probe shells keepautomaticFullPrefetchEligible === undefinedand are unaffected. ✓s-maxageparsing./^\s*s-maxage=(\d+)\s*$/iper comma-split directive correctly handlespublic, s-maxage=Nands-maxage=N, stale-while-revalidate, and correctly treatss-maxage=0as not eligible (Number("0") > 0is false). ✓- Single source of truth. The policy lives only in the client link shim; no server-side (
prod-server,worker-entry) duplicate to keep in sync. The only server-side touch is thelearnOptimistic…guard, which is the correct counterpart. ✓ - Build-time cost.
hasGenerateStaticParamsdoes syncreadFileSync+ AST parse of the page and each layout, but only for dynamic routes and only during manifest generation (not per request), short-circuiting on the page file. Acceptable.
Parity note
The behavioral premise — a completed Full prefetch is reusable even for no-store/dynamic responses — is validated against the Next.js oracle: test/e2e/app-dir/navigation/navigation.test.ts reports 47/50 with both owned async-metadata rows (shows a fallback when prefetch was pending / ...completed) passing; the three remaining failures are the separately-owned #2104 hash/query rows.
Minor / non-blocking
responseProvesAutomaticFullPrefetchEligibilityjoinscache-control+cdn-cache-controlwith,then re-splits on,. That's fine for the current regexes (each directive is matched in isolation) but is slightly indirect; a small comment noting the join is only to pool both header sources for the same per-directive scan would help future readers.- The synthetic
metadata-await-promise/nested/personalized-loading/generated-posts/:slugmanifest entries in the unit tests have no real fixtures, which is fine since fetch is stubbed and the E2E suite covers the real wiring.
No correctness or parity issues found. Approving.
|
Review posted to PR #2446. SummaryRe-reviewed the current head What I verified
Minor / non-blocking
Approved. |
|
Fresh exact-head targeted verification after the final
The exact functional row mapped to #2446 remains fixed at the current live source head. |
1 participant
Summary
Next.js parity
Targets the App Router navigation behavior covered by:
test/e2e/app-dir/navigation/navigation.test.tsThis fixes the case where a visible static link with a loading boundary has already completed prefetch: navigation should use the prefetched full RSC payload rather than showing the fallback again.
Validation
vp test run tests/link.test.ts -t "automatic full RSC prefetch|full-prefetches static routes|distinguishes automatic prefetch"vp test run tests/link-navigation.test.ts -t "full-prefetches visible static links with loading boundaries"vp check tests/link.test.ts tests/link-navigation.test.ts packages/vinext/src/shims/link.tsxREPO="/Users/jamesanderson/.codex/worktrees/async-metadata-prefetch-fallback/vinext" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/navigation/navigation.test.ts(50/50 passed)