fix(app-router): align prefetch server protocol#2318
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — fix(app-router): align prefetch server protocol
Solid, well-tested change. The render-mode plumbing, normalization, and test gating all look correct, and the unit suites I ran (app-rsc-request-normalization, app-rsc-cache-busting, link-navigation, app-page-route-wiring) pass. A few issues worth addressing — none are blocking, but the base-path/query parity gaps will silently disable the prefetch-empty optimization in real apps.
1. base-path mismatch silently disables prefetch-empty (parity)
createAppPrefetchRequestHeaders() sends two values that are normalized differently:
nextUrlcomes fromgetCurrentNextUrl()→window.location.pathname + window.location.search, which includes the configuredbasePath.prefetchRouterState.pathAndSearchcomes fromcreateSnapshotPathAndSearch/createBasePathStrippedPathAndSearch, which strips thebasePath.
In normalizeRscRequest, the empty-payload decision requires:
routerState.pathAndSearch === nextPathAndSearch && routerState.pathAndSearch === targetPathAndSearch
With any basePath set, routerState.pathAndSearch (stripped) can never equal nextPathAndSearch (derived from the unstripped Next-Url). So every same-tree prefetch falls through to prefetch-loading-shell instead of prefetch-empty. It degrades gracefully (still a valid response), but the optimization is effectively dead under basePath. Either strip the basePath from nextUrl before comparison, or compare basePath-stripped forms on the server.
2. query normalization mismatch (parity, even without basePath)
routerState.pathAndSearch re-serializes the query via URLSearchParams.toString(), while the server compares against the raw url.search / Next-Url search strings. Differences in param ordering or encoding (e.g. ?a=1&b=2 vs ?b=2&a=1, or %20 vs +) will make the equality check fail and skip prefetch-empty. Consider normalizing both sides the same way (parse → re-serialize) before comparing.
3. stale comment in consumePrefetchResponse (nit)
The delete was moved out of the unconditional position into the expired/success branches. The slot-mismatch branch comment still reads "Entry was already removed above," but the entry is no longer removed before that return. In practice this branch is dead code (findPrefetchCacheEntryForNavigation only returns slot-compatible entries), so there's no functional regression — but the comment is now misleading and should be updated or removed.
Notes (no action needed)
Next-Test-Fetch-Priorityis correctly gated behind__NEXT_TEST_MODEand thefalse-vs-"false"define encoding tree-shakes properly, so there's no production impact even though navigations now passfetchPriority: "auto".priority: "auto"on the navigationfetch()is the default, so it's a harmless no-op.- The async
void root.then(...)initialization ofinitialPrefetchRouterStatehas a correct route-manifest fallback for prefetches that fire before hydration resolves.
Heads-up
Per AGENTS.md, this PR should ideally cite the Next.js test(s) that establish the Next-Router-Prefetch / Next-Router-State-Tree / Next-Url protocol and the empty-vs-loading-shell semantics, to lock in parity. I couldn't verify against a local .nextjs-ref clone (not present in this environment).
|
Review posted to PR #2318. Summary of findings: Review SummaryThe change is well-structured and all the unit tests I ran pass ( 1. base-path mismatch silently disables 2. query normalization mismatch (parity) — client side re-serializes the query via 3. stale comment (nit) — I also confirmed the |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(app-router): align prefetch server protocol
Re-reviewed after the two follow-up commits (117fb9b keep prefetch state type internal, fef890f avoid consuming partial prefetches). The render-mode plumbing, cache-key hashing of the new Next-Router-* / Next-Url headers, and the __NEXT_TEST_MODE gating all look correct. Targeted unit suites pass locally (app-rsc-request-normalization, prefetch-cache, app-page-route-wiring — 130 tests). A few items from the prior review are still open, plus one new note on the partial-prefetch behavior change.
1. base-path mismatch silently disables prefetch-empty (parity) — still open
createAppPrefetchRequestHeaders() sends two values normalized differently:
nextUrl=getCurrentNextUrl()→window.location.pathname + window.location.search(navigation.ts:288), which includes the configuredbasePath.prefetchRouterState.pathAndSearchcomes fromcreateSnapshotPathAndSearch/createBasePathStrippedPathAndSearch(app-browser-navigation-controller.ts:237), which strips thebasePath.
On the server in normalizeRscRequest (app-rsc-request-normalization.ts:178-189):
cleanPathnameis base-path-stripped (:141), sotargetPathAndSearchis stripped.nextRequestUrl = new URL(nextUrl, url)keeps the unstrippedNext-Url, sonextPathAndSearchis unstripped.
The empty-payload decision requires routerState.pathAndSearch === nextPathAndSearch && routerState.pathAndSearch === targetPathAndSearch. With any basePath, the stripped routerState.pathAndSearch can never equal the unstripped nextPathAndSearch, so every same-tree prefetch falls through to prefetch-loading-shell instead of prefetch-empty. Degrades gracefully, but the optimization is dead under basePath. The app-basic fixture has no basePath, so no test covers this. Fix by stripping basePath from nextUrl before comparison (or comparing base-path-stripped forms on the server).
2. query normalization mismatch (parity) — still open, narrower than first stated
routerState.pathAndSearch re-serializes the query via URLSearchParams.toString(), while the server compares nextRequestUrl.search (the raw Next-Url search string). Ordering is actually preserved by both, so most cases match — but encoded spaces and valueless params still diverge:
?a=%20 → raw "?a=%20" vs re-serialized "?a=+" (DIFF)
?a → raw "?a" vs re-serialized "?a=" (DIFF)
Either case silently skips prefetch-empty. Normalize both sides identically (parse → re-serialize) before comparing.
3. stale comment in consumePrefetchResponse — still misleading after fef890f
fef890f moved the deletePrefetchCacheEntry call out of the unconditional position into the expired (navigation.ts:826) and success (:829) branches. The slot-mismatch branch (:820-824) still reads:
// Entry was already removed above. Slot mismatch means the prefetch
// used stale slot context and cannot be safely reused.
return null;But nothing is removed before this return anymore. The branch is effectively dead (findPrefetchCacheEntryForNavigation only returns slot-compatible entries), so no functional regression — but the comment is now wrong and should be updated or dropped.
Note on the partial-prefetch behavior change (fef890f)
router.prefetch() now seeds a non-consumable entry (cacheForNavigation: false, optimisticRouteShell: true), so a subsequent navigation issues a fresh request rather than consuming the partial prefetch (prefetch-cache.test.ts updated; e2e renamed to "does not consume an in-flight partial prefetch"). This looks intentional and matches the "learning-only protocol" framing — flagging it because the PR body says tests/prefetch-cache.test.ts "has no final diff," which is now stale (it does have a diff). Worth correcting the PR description.
Heads-up (process)
Per AGENTS.md, this PR should cite the Next.js test(s) that establish the Next-Router-Prefetch / Next-Router-State-Tree / Next-Url protocol and the empty-vs-loading-shell semantics. No .nextjs-ref clone is present in this environment, so I couldn't verify parity directly.
|
Review posted to PR #2318. SummaryRe-reviewed after the two follow-up commits. The core change is sound — render-mode plumbing, cache-key hashing of the new protocol headers, and 1. base-path mismatch disables 2. query normalization mismatch (parity) — Narrower than the first review claimed (ordering is preserved), but encoded spaces ( 3. stale comment — Notes: The partial-prefetch change in None of the issues are blocking; all degrade gracefully. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(app-router): align prefetch server protocol
Re-reviewed at e457634. The only new commit since the last round (e457634 test(app-router): explicitly ignore router push result) is a one-line test cleanup (void router.push(...)); it doesn't touch any of the open issues. The render-mode plumbing, cache-key hashing of the new Next-Router-* / Next-Url headers, and __NEXT_TEST_MODE gating remain correct. Targeted unit suites pass locally (app-rsc-request-normalization 64, prefetch-cache 27 — 91 tests). Three issues remain open; none are blocking (all degrade gracefully), but they silently disable the prefetch-empty optimization in real apps.
1. base-path mismatch silently disables prefetch-empty (parity) — still open
createAppPrefetchRequestHeaders() (shims/navigation.ts:291) sends two values normalized differently:
nextUrl=getCurrentNextUrl()→window.location.pathname + window.location.search(navigation.ts:288), which includes the configuredbasePath.prefetchRouterState.pathAndSearchcomes fromcreateBasePathStrippedPathAndSearch/createSnapshotPathAndSearch(app-browser-navigation-controller.ts:237-241,navigation.ts:1194-1197), both of which strip thebasePath.
On the server in normalizeRscRequest (app-rsc-request-normalization.ts:178-189):
cleanPathnameis derived from the base-path-strippedpathname(:141), sotargetPathAndSearch(:184) is stripped.nextRequestUrl = new URL(nextUrl, url)(:179) keeps the unstrippedNext-Url, sonextPathAndSearch(:183) is unstripped.
The empty-payload decision requires routerState.pathAndSearch === nextPathAndSearch && routerState.pathAndSearch === targetPathAndSearch (:186-187). With any basePath, the stripped routerState.pathAndSearch can never equal the unstripped nextPathAndSearch, so every same-tree prefetch falls through to prefetch-loading-shell instead of prefetch-empty. The app-basic fixture has no basePath, and the new normalization tests use exact-match URLs with basePath: "" (app-rsc-request-normalization.test.ts:492-538), so nothing covers this. Fix by stripping basePath from nextUrl before comparison (or comparing base-path-stripped forms on the server), and add a basePath + prefetch test.
2. query normalization mismatch (parity) — still open, narrow
routerState.pathAndSearch re-serializes the query via URLSearchParams.toString() (app-browser-navigation-controller.ts:239, navigation.ts:1195), while the server compares against the raw nextRequestUrl.search (app-rsc-request-normalization.ts:183). Ordering is preserved by both, so most cases match — but encoded spaces and valueless params still diverge:
?a=%20 → raw "?a=%20" vs re-serialized "?a=+" (DIFF)
?a → raw "?a" vs re-serialized "?a=" (DIFF)
Either case silently skips prefetch-empty. Normalize both sides identically (parse → re-serialize) before comparing.
3. stale comment in consumePrefetchResponse — still misleading
fef890f moved the deletePrefetchCacheEntry call out of the unconditional position into the expired (navigation.ts:826) and success (:829) branches. The slot-mismatch branch (:820-824) still reads:
// Entry was already removed above. Slot mismatch means the prefetch
// used stale slot context and cannot be safely reused.
return null;Nothing is removed before this return anymore. The branch is effectively dead (findPrefetchCacheEntryForNavigation only returns slot-compatible entries), so no functional regression — but the comment is now wrong and should be updated or dropped.
PR description nit
The PR body says tests/prefetch-cache.test.ts "has no final diff." It does have a diff at HEAD — the router.prefetch test was renamed to "...learning-only protocol response..." and now asserts cacheForNavigation === false, optimisticRouteShell === true, and consumePrefetchResponse(...) === null. Worth correcting the description to match fef890f's intentional non-consumable-partial-prefetch change.
Heads-up (process)
Per AGENTS.md, this PR should cite the Next.js test(s) that establish the Next-Router-Prefetch / Next-Router-State-Tree / Next-Url protocol and the empty-vs-loading-shell semantics, to lock in parity. No .nextjs-ref clone is present in this environment, so I couldn't verify against Next.js source directly.
|
Review posted to PR #2318. SummaryRe-reviewed at the current HEAD (
Also flagged that the PR body's "no final diff" claim about Targeted unit suites pass (91 tests). None of the issues are blocking — all degrade gracefully. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
/bigbonk re-review — fix(app-router): align prefetch server protocol
Re-reviewed at HEAD (43e39a8). Two new commits since the last round (e7cbfac preserve full prefetch semantics, 43e39a8 preserve explicit full prefetch requests) — and both of them resolve all three issues from the prior reviews. The change now looks correct and well-tested. Targeted suites pass locally: app-rsc-request-normalization (67), app-rsc-cache-busting (36), link-navigation (50), prefetch-cache (27), app-page-route-wiring (39). vp check is clean on the changed source files.
Previously-flagged issues — all now fixed ✅
1. base-path mismatch silently disabled prefetch-empty — FIXED in 43e39a8.
normalizeRscRequest now routes all three comparison values through the new normalizeComparablePathAndSearch helper (app-rsc-request-normalization.ts:59-67), which strips basePath (hasBasePath/stripBasePath) before comparing. The client already sends a base-path-stripped pathAndSearch, so the stripped server-side nextPathAndSearch/targetPathAndSearch can now match it under a configured basePath. Covered by the new test "infers the current tree under basePath after stripping the prefix" (/docs basePath → APP_RSC_RENDER_MODE_PREFETCH_EMPTY).
2. query normalization mismatch — FIXED in 43e39a8.
normalizeComparablePathAndSearch re-serializes every value via parsed.searchParams.toString() (:65), the same serialization the client uses in createBasePathStrippedPathAndSearch / createSnapshotPathAndSearch (URLSearchParams(...).toString()). Encoded spaces (%20 vs +) and valueless params (?a vs ?a=) now normalize identically on both sides. Covered by the new parameterized test "normalizes equivalent %s before current-tree comparison" for both encoded spaces and valueless params.
3. stale comment in consumePrefetchResponse — FIXED in e7cbfac.
The slot-mismatch branch no longer claims "Entry was already removed above"; the comment now reads "Slot mismatch means the prefetch used stale slot context and cannot be safely reused" (navigation.ts:847-850), which matches fef890f's relocation of the delete into the expired/success branches.
Review of the new full-prefetch work (e7cbfac, 43e39a8)
These commits add explicit prefetch={true} (full) support and a learning-only-entry upgrade path. They look correct:
prefetchKind: "full"→includePrefetchHeader: false, so a full prefetch omitsNext-Router-Prefetchbut still sendsNext-Router-State-Tree+Next-Url(app-rsc-cache-busting.ts:293-299). Server-side, the empty-vs-shell derivation is gated onNEXT_ROUTER_PREFETCH_HEADER === "1"(app-rsc-request-normalization.ts:182), so a full prefetch correctly staysnavigation(fetches the full payload), matching Next.js full-prefetch semantics.- The full-prefetch
_rschash differs from a navigation's (state-tree/next-url participate in the hash), butfindPrefetchCacheEntryForNavigationnormalizes the_rscparam away vianormalizeRscCacheLookupUrlbefore matching, so navigation still consumes a full prefetch entry. discardLearningOnlyPrefetchCacheEntrynow scans the cache with normalized-URL + interception-context matching instead of an exact cacheKey (navigation.ts:561-581), anddeletePrefetchCacheEntrygained anownsCacheKeyguard so it won't blow away a re-inserted entry under the same key. Reordering the discard ahead of theprefetched.hasgate inlink.tsx:475-482is sound — cacheable prefetches upgrade a learning-only entry by discarding then re-fetching.
Process note (unchanged from prior rounds)
Per AGENTS.md, the PR should still cite the Next.js test(s) establishing the Next-Router-Prefetch / Next-Router-State-Tree / Next-Url protocol and the empty-vs-loading-shell semantics. No .nextjs-ref clone is present in this environment, so I couldn't verify parity against Next.js source directly. Also note the PR body's claim that tests/prefetch-cache.test.ts "has no final diff" remains stale (it does have a diff at HEAD).
LGTM — no blocking issues. The three open items from prior reviews are all resolved and now have direct test coverage.
|
Review posted to PR #2318. SummaryI re-reviewed PR #2318 at HEAD ( Previously-flagged issues — all fixed
New full-prefetch work reviewedThe Verification
No blocking issues. Remaining notes are process-only (missing Next.js parity-test citations per AGENTS.md; stale "no final diff" claim in PR body for |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(app-router): align prefetch server protocol
Re-reviewed at HEAD (54d3794). One new commit since the last round: 54d3794 fix(app-router): guard malformed next-url inference. It is a correct, self-contained defensive fix. All three correctness issues from earlier rounds remain resolved. No blocking issues.
New commit review (54d3794) — correct ✅
Before this commit, normalizeRscRequest built nextRequestUrl = new URL(nextUrl, url) directly from the raw Next-Url header. A malformed value (e.g. http://[) would throw out of normalizeRscRequest, turning an attacker-controllable header into a 500. The commit wraps the two header-derived comparisons in tryNormalizeComparablePathAndSearch (app-rsc-request-normalization.ts:69-79) and only derives a prefetch render mode when both succeed (:211-216); otherwise it leaves renderMode = APP_RSC_RENDER_MODE_NAVIGATION, which fetches the full payload — a safe, graceful fallback. New test "ignores malformed Next-Url instead of crashing request normalization" (:543-557) covers it.
A couple of points I verified rather than flag:
targetPathAndSearchstill uses the non-tryvariant (:210). This is safe:targetUrlisnew URL(url)with onlypathname/searchParamsmutated, sotargetUrl.hrefis always a well-formed absolute URL andnew URL(href, base)cannot throw. No need to guard it.routerState.pathAndSearchis already validated to be a string starting with/inparsePrefetchRouterState(:45-52), sorouterPathAndSearchwon't realistically throw either — but routing it through thetryhelper is harmless and keeps the two header-derived values symmetric.- The guard correctly leaves the only unguarded
new URL()on attacker-controlled input (the oldnextUrlpath) closed; the rest of the function already returned400/404for malformed path input.
Previously-flagged issues — still resolved ✅
- base-path mismatch /
prefetch-empty— still fixed vianormalizeComparablePathAndSearchstrippingbasePathon all three comparands. Covered by the/docsbasePath test (:508-522). - query normalization mismatch — still fixed; both sides re-serialize via
searchParams.toString(). Parameterized%20-vs-+/ valueless-param test (:524-541). - stale
consumePrefetchResponsecomment — still fixed (navigation.ts:848reads "Slot mismatch means the prefetch used stale slot context…", no false "already removed above").
Verification
app-rsc-request-normalization(68),app-rsc-cache-busting(36),link-navigation(50),prefetch-cache(27),app-page-route-wiring(39) — all pass.vp checkclean on the changed source + test files.
Process notes (non-blocking, unchanged)
- Per AGENTS.md, the PR should still cite the Next.js test(s) that establish the
Next-Router-Prefetch/Next-Router-State-Tree/Next-Urlprotocol and the empty-vs-loading-shell semantics. No.nextjs-refclone is present in this environment, so I couldn't verify the malformed-Next-Urlfallback against Next.js source directly — worth confirming Next.js also degrades to a full fetch (rather than erroring) on a malformedNext-Url. - The PR body's claim that
tests/prefetch-cache.test.ts"has no final diff" is still stale — it has a real diff at HEAD (therouter.prefetchtest was renamed to "…learning-only protocol response…" and now assertscacheForNavigation === false,optimisticRouteShell === true, andconsumePrefetchResponse(...) === null).
LGTM — the new guard is correct and tested, and no prior issues have regressed.
|
Review posted to PR #2318. SummaryRe-reviewed at HEAD ( New commit ( I verified:
Prior issues — all still resolved: base-path Verification: 220 tests pass across the 5 targeted suites; Process notes (non-blocking): missing Next.js parity-test citations per AGENTS.md (no |
|
Deploy-suite run 28478866791 triage: this PR appears to cover the |
|
Deploy-suite backlog check for run 28478866791: this PR appears to cover the remaining non-cacheComponents/use-cache failures from |
|
Deploy-suite run 28478866791/job 84413308650 follow-up: I rechecked the open James PR coverage for the functional App Router app-prefetch cluster and I do not think this needs a duplicate implementation branch. Coverage mapping:
Skipping the requested backlog item to avoid duplicating #2318/#2229. cacheComponents/use-cache failures remain intentionally out of scope. |
|
Backlog coverage check for GitHub Actions run 28478866791 / job 84413308650: this PR already covers the non-cacheComponents/use-cache App Router prefetch layout-sharing failures in |
1 participant
Summary
Scope
This intentionally excludes the viewport scheduling changes and browser scheduling spec already covered by #2229.
tests/prefetch-cache.test.tsalso has no final diff.Validation
vp test run tests/app-page-route-wiring.test.ts tests/app-rsc-cache-busting.test.ts tests/app-rsc-request-normalization.test.ts tests/link-navigation.test.ts(186 passed)vp test run tests/features.test.ts -t 'App navigation failure handling defaults to false|Next.js test mode defaults to false'(2 passed; 188 targeted units total)PLAYWRIGHT_PROJECT=app-router vp exec playwright test tests/e2e/app-router/nextjs-compat/prefetch.spec.ts -g 'normal router prefetch sends router state and reaches loading-shell rendering' --workers=1(1 passed, port 4174 cleared first)vp check <18 changed files>vp run vinext#build