fix(app-router): recover mismatched prefetch rewrites - #2453
Conversation
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: fix(app-router): recover mismatched prefetch rewrites
Reviewed all 18 changed files, ran vp check on the touched source files (clean) and the five relevant unit suites (210 tests passing). This is a solid, well-tested change that stays close to Next.js behavior. No blocking issues found.
What works well
- Matcher parity fix is correct.
middleware-runtime.tsnow evaluateshas/missingconditions against an unmodifiedmatcherRequest, plumbed fromapp-middleware.tsasoptions.request(original) while the user middleware still receives the Flight-strippedmiddlewareRequest. This matches Next.js, which evaluates matcher conditions before stripping router headers. Theoptions.matcherRequest ?? options.requestfallback preserves existing behavior for Pages Router and other callers that don't set it. Backed by a ported test inshims.test.tslinking to the Next.js source. - Conservative demotion.
automaticSearchPrefetchMayMismatchNavigationonly demotes cache reuse formode === "auto"query-string prefetches, andclientMiddlewareSourceMatchesfails closed (returnsfalse) on regex/group sources, which is the right direction here (only demote when confident). Note this deliberately diverges from the Pages Router copy inpages-data-target.ts:81, which fails open — the opposite default is correct because the two functions answer opposite questions. - Probe safety.
probePrefetchLoadingShellFallbackwraps the fallback probe inrunWithConnectionProbe+runWithSuppressedHookWarning+ try/catch, andextractFirstSuspenseFallbackstops at the firstSuspensefallback without descending intoconnection()-suspending children (verified by the newapp-page-probe.test.tscase). The recursion depth guards (> 100) are present in both the extract and the client-sidecreateSuspendingElementInsideFirstSuspense.
Non-blocking observations
-
Scheduling change is broader than the PR title.
prefetchUrlnow dispatches App Router prefetches immediately (bypassingrequestIdleCallback) rather than only forpriority === "high". This is intentional and tested ("dispatches visible App Router prefetches without waiting for idle time"), and dedup viagetPrefetchedUrls/hasPrefetchCacheEntryForNavigationguards against flooding — but it's a behavioral change worth calling out in the PR body since it affects every visible App Router<Link>, not just mismatched-prefetch cases. -
Multiple page entries share one probed fallback.
replaceAppPageElementWithFallbackapplies the single probed fallback to every id fromgetAppPagePayloadEntryIds(page +childrenslots + anypage-kind key). For a route with multiple distinct page/parallel-route payload entries, they'd all receive the fallback extracted from the primary page component. This is unlikely to matter for the loading-shell path (the shell is coarse-grained), and the marker-removal fallback path handles the no-replacement case correctly, but it's a mild imprecision if a route ever has genuinely divergent parallel page fallbacks. -
Logic reads implicitly for the inverted matcher case. A
has: [{type: header, key: next-router-prefetch}]matcher (middleware runs only on prefetch) correctly returnsfalsefrommiddlewareMatcherConditionsMatchNavigationbecauseclientMatcherConditionMatchesreturnsfalsefor that header in thehasloop. Correct outcome, but it happens as a side effect of the header special-case rather than an explicit branch — a short comment there would help future readers.
None of these require changes to merge. LGTM.
|
Review posted to PR #2453. SummaryI reviewed all 18 changed files in PR #2453, ran Verdict: LGTM, no blocking issues. The change is well-implemented and stays close to Next.js behavior:
I flagged three non-blocking observations:
None require changes to merge. |
fded658 to
0f4d508
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): recover mismatched prefetch rewrites
Independent review of all 18 changed files. Ran vp check on the six touched source files (clean — no format/lint/type errors) and the six relevant unit suites (1385 tests passing, incl. link-navigation, app-page-probe, app-page-dispatch, app-optimistic-routing, app-page-route-wiring, shims).
Verdict: LGTM, no blocking issues. The change is well-implemented, thoroughly tested, and stays close to Next.js behavior.
Verified correctness
-
Middleware matcher parity fix is correct and properly scoped.
middleware-runtime.tsnow evaluateshas/missingconditions against an unmodifiedmatcherRequest(plumbed fromapp-middleware.ts:249), while user middleware still receives the Flight-strippedmiddlewareRequest. Before this,missing: Next-Router-Prefetchmatchers always "matched" on prefetch requests because the header had already been stripped, matching Next.js which evaluates matcher conditions before the strip. Theoptions.matcherRequest ?? options.requestfallback preserves behavior for every other caller. ConfirmedexecuteMiddleware/applyAppMiddlewareare App-Router-only (Pages Router has its own middleware path), so dev/prod parity holds — no equivalent change is needed elsewhere. -
Always-setting
Next-Router-Prefetch: 1is more correct, not less. Previously the header was set only for optimistic shell prefetches; full prefetches omitted it, which would incorrectly satisfy amissing: Next-Router-Prefetchmatcher on full-prefetch requests. Setting it unconditionally (link.tsx:694) matches Next.js, where all router prefetch requests carry the header. Covered by newlink-navigationtests. -
Conservative demotion is correctly failing closed.
automaticSearchPrefetchMayMismatchNavigationonly demotes cache reuse formode === "auto"query-string prefetches, andclientMiddlewareSourceMatchesreturnsfalsefor any source containing regex/group syntax ((,\).clientMatcherConditionMatchesreturns"unknown"for headers it can't inspect client-side, whichmiddlewareMatcherConditionsMatchNavigationtreats as non-matching — so demotion only fires when the client is confident the matcher requires skipping prefetch. Right direction. -
Loading-shell fallback probe follows the established pattern.
probePrefetchLoadingShellFallbackwraps the probe inrunWithConnectionProbe+runWithSuppressedHookWarning+try/catch, the same guard stack used byapp-page-execution.ts. Theapp-page-probe.test.tscase verifiesextractFirstSuspenseFallbackstops at the firstSuspensefallback and does not invokeconnection()-suspending children. Recursion depth guards (> 100) are present on both the server extract and the clientcreateSuspendingElementInsideFirstSuspense.
Non-blocking observations
-
extractFirstSuspenseFallbackmanually renders nested function components (app-page-dispatch.ts:147-151) by invokingvalue.type(props). This is more aggressive than the existing probe (which invokes only the top-level page component and lets React abort).typeof value.type === "function"also matches React class components, which would misbehave if called withoutnew— but the whole probe is wrapped intry/catchand fails safe tonull, so worst case is a missed loading shell, never a crash. Consider a short comment noting the class-component caveat. -
replaceAppPageElementWithFallbackapplies one probed fallback to every page/slot entry fromgetAppPagePayloadEntryIds. For a route with genuinely divergent parallel-route page fallbacks, they'd all receive the fallback extracted from the primary page. Low-impact for the coarse loading-shell path, and the marker-removal branch handles the no-replacement case, but it's a mild imprecision. -
Scheduling change is broader than the PR title.
prefetchUrlnow dispatches App Router prefetches immediately (bypassingrequestIdleCallback) for all App Router links, not justpriority === "high"(link.tsx:840-847). Intentional and tested ("dispatches visible App Router prefetches without waiting for idle time"), with dedup viagetPrefetchedUrls/hasPrefetchCacheEntryForNavigationguarding against flooding — but worth a line in the PR body since it affects every visible App Router<Link>. -
Inverted matcher case is handled implicitly. A
has: [{type: header, key: next-router-prefetch}]matcher (runs only on prefetch) correctly yieldsfalsefrommiddlewareMatcherConditionsMatchNavigationbecauseclientMatcherConditionMatchesspecial-cases that header tofalsein thehasloop. Correct outcome, but a one-line comment on the header branch would aid future readers.
None require changes to merge.
SummaryReviewed all 18 changed files in PR #2453, ran Verdict: LGTM, no blocking issues. Key things I verified beyond the prior bot review:
I flagged four non-blocking observations: |
NathanDrake2406
left a comment
There was a problem hiding this comment.
Requesting changes on the client-side middleware matcher duplication.
The new App Router prefetch path adds another partial middleware matcher implementation in packages/vinext/src/shims/link.tsx: ClientMiddlewareMatcherObject, isClientMiddlewareMatcherObject, stripLocaleForMiddlewareMatcher, clientMiddlewareSourceMatches, and a separate has/missing condition walker. We already have the canonical middleware matcher path in packages/vinext/src/server/middleware-matcher.ts backed by packages/vinext/src/config/config-matchers.ts, and there is also a client-side Pages copy in packages/vinext/src/shims/internal/pages-data-target.ts.
I do not think link.tsx should import the server module directly, but this PR should extract the client-safe matcher subset into a shared helper and reuse it from both the App Router prefetch demotion and the Pages data target. The helper can keep the two callers' different policies explicit, e.g. fail closed for App prefetch demotion and fail open for Pages middleware data probing, while sharing matcher-object validation, locale stripping, and source matching. That preserves this PR's behavior without leaving three drift-prone matcher implementations.
|
I think this PR covers the functional failure from GH Actions run 28478866791 / job 84413308650 for Why: the PR specifically handles App Router navigations where a prefetched URL rewrites to a different route, and its validation lists the focused mismatching-prefetch E2E plus the related client navigation/prefetch tests. I did not see this as a cacheComponents/use-cache-only failure. |
…-prefetch-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
…-prefetch-28478866791
…-prefetch-28478866791
…-prefetch-28478866791 # Conflicts: # tests/link-navigation.test.ts
…-prefetch-28478866791
|
Backlog mapping for GitHub Actions run 28478866791 / job 84413308650: this PR appears to own the functional App Router failure in Per instruction I am skipping a duplicate implementation because this PR is by |
…-prefetch-28478866791 # Conflicts: # packages/vinext/src/plugins/rsc-reference-validation-normalizer.ts # packages/vinext/src/shims/link.tsx
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Run 29871622126 / job 88775791401 still fails I reverified the exact targeted Next.js v16.2.6 suite on the current-main campaign baseline ( |
|
Correction to my prior run mapping: |
|
Correction after live freshness review: this PR must not be treated as the current fix for Actions run 31290819291. Its head 18b4855 is from 2026-07-01, is currently DIRTY/conflicting with main, and has not passed the targeted Next.js E2E against the current repository state. The current-main failure remains reproducible. The older implementation also uses structural first-Suspense fallback extraction and broad payload replacement, which do not preserve the actual React partial shell in multi-boundary/static-sibling cases. I am therefore withdrawing the prior "skip duplicate" mapping. The run-312908 backlog item remains active until a current-main implementation has exact targeted E2E proof, clean independent review, Bonk, and CI. |
Summary
Validation
vp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.ts packages/vinext/src/server/app-optimistic-routing.ts packages/vinext/src/server/app-page-route-wiring.tsxvp test run tests/link-navigation.test.ts tests/app-page-probe.test.ts tests/app-page-dispatch.test.ts tests/app-optimistic-routing.test.ts tests/app-page-route-wiring.test.tsvp exec playwright test -c tests/e2e/app-router/nextjs-compat/playwright.nextjs-compat.config.ts tests/e2e/app-router/nextjs-compat/mismatching-prefetch.browser.spec.ts --project=chrome-browser-specificIndependent review loop completed with no findings after follow-up fixes.