fix(pages): defer router query until ready#2539
Conversation
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11cfe69d73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const shouldDeferQuery = | ||
| _ssrCtx.navigationIsReady === false && (_ssrCtx.isFallback === true || isAutoExportDynamic); |
There was a problem hiding this comment.
When this server branch returns an empty query for a pre-ready auto-export dynamic or fallback render, the browser hydration path still initializes PagesRouterProvider through the client branch of getPathnameAndQuery, which always merges window.location.search and route params into query even while isPagesRouterReady() is false. For a page like /posts/[id]?view=full that renders router.query, the server now ships {} but the first client render hydrates with { id: "42", view: "full" }, causing the hydration mismatch this change is trying to avoid; please defer the client snapshot under the same readiness condition or keep the SSR snapshot aligned.
Useful? React with 👍 / 👎.
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.
Reviewed the change. The core fix is correct and matches Next.js's render.tsx carve-out (query = {} for isAutoExport || isFallback renders, per issue #1971's citations). I verified:
- Dev/prod parity holds. The SSR branch reads
autoExportfrom_ssrCtx.nextData(populated viabuildPagesReadinessNextData→applySSRContext({ nextData: pagesNextData })inpages-page-handler.ts:488), and the fallback republish atpages-page-handler.ts:672setsisFallback: true, whichshouldDeferPagesRouterQuerykeys off (it does not depend onnextDatafor the fallback path). ✅ - Client/SSR alignment. The Codex P2 comment about the client hydration path leaking
{ id, view }while SSR ships{}is addressed by this PR's second commit — the client branch now gates the same merge behindshouldDeferPagesRouterQuery(pathname, isPagesRouterReady(), nextData), andpagesRouterReadyinitializes tofalsefor pre-ready auto-export dynamic routes (router.ts:460). The added client-hydration test exercises this. ✅ - Tests pass.
vitest run tests/shims.test.ts -t "pre-ready"→ 4 passed. ✅
Two non-blocking observations below.
| ): boolean { | ||
| const isAutoExportDynamic = | ||
| nextData?.autoExport === true && extractRouteParamNames(routePattern).length > 0; | ||
| return !isReady && (nextData?.isFallback === true || isAutoExportDynamic); |
There was a problem hiding this comment.
Non-blocking scope note: shouldDeferPagesRouterQuery gates the auto-export case on extractRouteParamNames(routePattern).length > 0 (dynamic only). In Next.js render.tsx, the query = {} reset fires for isAutoExport || isFallback where isAutoExport is not gated on the route being dynamic — so a static auto-export page loaded with a query string (e.g. /about?foo=bar) also renders with query = {} while isReady === false in Next.js, whereas here it would not defer.
This matches issue #1971's stated scope (auto-export dynamic routes) and is consistent with the sibling deferral in getPagesNavigationContext (line 1004-1005) and _buildClientPagesNavigationContext (line 1004), so it's a pre-existing, deliberate scoping choice rather than a regression. Flagging only so it's a conscious decision — if strict parity on static auto-export + querystring is desired, that would be a follow-up touching all these sites together.
| nextData: RouteQueryNextData | undefined, | ||
| ): boolean { | ||
| const isAutoExportDynamic = | ||
| nextData?.autoExport === true && extractRouteParamNames(routePattern).length > 0; |
There was a problem hiding this comment.
Minor maintainability: the isAutoExportDynamic predicate (nextData?.autoExport === true && extractRouteParamNames(...).length > 0) is now duplicated in at least four places — here, _buildClientPagesNavigationContext (line 1004-1005), getPagesNavigationContext (line 1076-1077), and referenced by resolvePagesNavigationPathname. Since these must stay in lockstep to avoid reintroducing a hydration mismatch (a risk explicitly called out in the resolvePagesNavigationPathname doc comment), consider extracting a single isAutoExportDynamicRoute(routePattern, nextData) helper and reusing it. Not required for this PR, but it reduces the chance a future edit updates one site and not the others.
|
Review posted successfully. Summary of my review of PR #2539: Verdict: Approve (with two non-blocking notes)The fix correctly implements the Next.js What I verified:
Two non-blocking observations posted inline:
|
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.
Reviewed the change against the checked-out branch. The core fix is correct and matches Next.js's render.tsx carve-out (query = {} for isAutoExport || isFallback renders while isReady === false). I verified locally:
- Targeted tests pass.
vitest run tests/shims.test.ts -t "pre-ready"→ 4 passed;tests/pages-router.test.ts+tests/entry-templates.test.ts→ 392 passed;tsc --noEmitclean. - Shared runtime state is coherent across module instances.
pagesRouterInitialStatelives on the global-symbolrouterRuntimeState(router.ts:446/487), so duplicatednext/routerchunks observe the same flag — no split-state hydration hazard. - State transitions flip correctly.
markPagesRouterReadyclearspagesRouterInitialStatefor non-fallback renders before the early-return (router.ts:1390), so the ready timer re-reads the merged query even for already-ready routes; fallback/auto-export paths flip it viamarkPagesRouterLiveStateon the hydrationRouter.replace. The reactivePagesRouterProvider(router.ts:3335) re-renders after that flip, resolving the mismatch. - Dev/prod parity holds. The SSR deferral reads
autoExport/isFallbackfrom request-scopednextData, and the fallback republish path is covered independently. Both dev-server and pages-page-handler apply the samegetPagesInitialRouterQuerygate.
Two non-blocking notes below.
| if (isFallback) return {}; | ||
| if (nextData?.autoExport === true) return {}; | ||
| return nextData?.gsp === true ? params : query; | ||
| } |
There was a problem hiding this comment.
Non-blocking (DRY): getPagesInitialRouterQuery and getPagesSerializedRouterQuery have byte-identical bodies — same guards, same gsp ? params : query result. The only difference is the name and declared return type. Since they must stay in lockstep (the runtime SSR snapshot and the serialized __NEXT_DATA__.query have to agree to avoid a hydration mismatch — the exact class of bug this PR fixes), consider collapsing them into one function so a future edit can't diverge one from the other.
If the intent is that they may diverge later (e.g. serialized query needs different escaping/shaping than the live SSR query), a short comment on each explaining the expected future difference would prevent someone from "deduplicating" them incorrectly.
| window.location.search, | ||
| nextData, | ||
| ); | ||
| if (!initialRouterIsReady) { |
There was a problem hiding this comment.
Behavior-expansion note (non-blocking): the hydration Router.replace(..., { _h: 1 }) was previously gated on nextData.isFallback; it now fires for every !initialRouterIsReady case — including auto-export dynamic routes and any route loaded with a query string. This is the correct parity behavior (Next.js runs the same _h: 1 query-update replace on hydration for not-ready routes), and it's what makes the client query converge to the merged value. But it does mean a plain auto-export dynamic page (no data functions) now goes through navigateClient on first load where it previously did not.
Worth confirming that path short-circuits cleanly for a pure auto-export page (no getStaticProps/getServerSideProps) rather than issuing a stray _next/data fetch — dataTarget?.dataKind should be neither static nor server for those, but it's the one runtime side effect this change broadens. An E2E assertion that no extra network request fires on hydration of a non-fallback auto-export dynamic page would lock this down.
|
Review posted successfully. Summary of my review of PR #2539: Verdict: Approve (two non-blocking notes)The fix correctly implements the Next.js What I verified locally (checked-out branch):
Two non-blocking observations posted inline:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed the checked-out branch. The core fix is correct and matches Next.js's render.tsx carve-out: query = {} for isAutoExport || isFallback renders while isReady === false, resolving the parity gap in #1971.
Verified locally:
- Targeted tests pass:
vitest -t "pre-ready"(4),tests/pages-page-handler.test.ts+tests/entry-templates.test.ts(67), and the fulltests/pages-router.test.ts(351). pnpm run checkclean (format, lint, type — 1085 files).- Dev/prod parity holds. Both
dev-server.tsandpages-page-handler.tsapply the samegetPagesInitialRouterQuerygate for the live SSR snapshot, and both serialize viagetPagesSerializedRouterQuery. The fallback republish path is covered independently. - Shared runtime state is coherent.
pagesRouterInitialStatelives on the global-symbolrouterRuntimeState, so duplicatednext/routerchunks observe the same flag.initializePagesRouterReadyFromNextDataruns before the hydrationRouter.replace({_h:1}), andmarkPagesRouterReadyclears the flag before its early return — the state transitions line up. - No stray data fetch for pure auto-export dynamic pages. The broadened
_h:1replace routes a no-data auto-export page throughnavigateClientNoData(dataKind is neitherstaticnorserver), so hydration doesn't issue a_next/datafetch. The prior reviewers' suggested E2E assertion would be a nice guardrail but the code path is correct.
No blocking issues. Two non-blocking notes below (one is a behavior expansion worth calling out explicitly).
| if (isFallback) return {}; | ||
| if (nextData?.autoExport === true) return {}; | ||
| return nextData?.gsp === true ? params : query; | ||
| } |
There was a problem hiding this comment.
Non-blocking (DRY / correctness hazard): getPagesInitialRouterQuery and getPagesSerializedRouterQuery are byte-identical — same guards, same isFallback / autoExport / gsp branches, same result. They must stay in lockstep, because the live SSR router.query (from getPagesInitialRouterQuery) and the serialized __NEXT_DATA__.query (from getPagesSerializedRouterQuery) have to agree — a divergence reintroduces the exact hydration mismatch this PR fixes.
Consider collapsing them into one function so a future edit can't drift one from the other:
| } | |
| export function getPagesInitialRouterQuery<T extends Record<string, unknown>>( | |
| query: T, | |
| params: Record<string, unknown>, | |
| nextData: PagesRouterQueryNextData | undefined, | |
| isFallback: boolean, | |
| ): T | Record<string, unknown> { | |
| if (isFallback) return {}; | |
| if (nextData?.autoExport === true) return {}; | |
| return nextData?.gsp === true ? params : query; | |
| } | |
| // Serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it is | |
| // intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
If they are expected to diverge later (e.g. serialized query needs different shaping/escaping than the live SSR query), a short comment on each stating the expected future difference would prevent someone from "deduplicating" them incorrectly.
| props: freshRenderProps, | ||
| page: patternToNextFormat(route.pattern), | ||
| query: params, | ||
| query: getPagesSerializedRouterQuery( |
There was a problem hiding this comment.
Behavior-expansion note (non-blocking): this path (and the equivalent at ~L1871, plus pages-page-handler.ts) now serializes the full merged query into __NEXT_DATA__.query for gssp/gip routes, where previously it serialized params only (the old query: params). This is closer to Next.js — its __NEXT_DATA__.query for gssp/gip pages includes route params and search params — so it's an improvement, and the new shims.test.ts case ("serializes the full initial query for ordinary server renders") locks it in.
Worth flagging only because it's a change beyond the stated PR scope (auto-export/fallback empty-query). I confirmed the client stays consistent: with pagesRouterInitialState true, getPathnameAndQuery returns copySerializedPagesQuery(nextData.query) directly, so the hydrated snapshot equals the serialized query for gssp — no mismatch. No action needed; just documenting the widened behavior.
|
Review posted successfully. Summary of my review of PR #2539: Verdict: Approve (two non-blocking notes)The fix correctly implements the Next.js What I verified locally on the checked-out branch:
Two non-blocking inline comments posted:
No blocking issues found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
The core empty-query deferral for auto-export dynamic / fallback renders is correct and matches Next.js render.tsx. The shared runtime-state coherence, the _h:1 hydration replace, and the pre-ready/ready snapshot tests all look sound. Prior reviewers already covered the DRY concern on the two getPages*RouterQuery helpers.
However, I believe there is a real dev/prod parity gap in the widened __NEXT_DATA__.query serialization behavior that the current tests do not catch. Details inline. This is exactly the class of divergence AGENTS.md calls out as high-risk, so flagging it as blocking-until-confirmed rather than a nit.
| props: options.props ?? { pageProps: options.pageProps }, | ||
| page: options.routePattern, | ||
| query: options.params, | ||
| query: getPagesSerializedRouterQuery( |
There was a problem hiding this comment.
Dev/prod parity gap (likely a hydration mismatch in production).
This new getPagesSerializedRouterQuery(options.query ?? options.params, ...) widens serialized __NEXT_DATA__.query from params-only to the full merged query — but only when a query is actually passed in. The production caller renderPagesPageResponse (this file, ~L499) builds the script without forwarding options.query:
const nextDataScript = buildPagesNextDataScript({
buildId: options.buildId,
...
params: options.params, // <-- no `query:` here
...
});
so options.query ?? options.params falls back to params. Meanwhile the dev path (dev-server.ts) calls buildPagesNextDataScript/serialize with the full merged query. Result: dev serializes the full query for gssp/gip routes, production serializes params-only.
Why it bites gssp/gip specifically (autoExport/gsp get {}/params regardless):
- Server React render uses
ssrCtx.query = getPagesInitialRouterQuery(query, params, nextData, false)= the fullqueryfor gssp/gip, so SSR HTML rendersrouter.query = { id, ...search }. - Client hydration with
pagesRouterInitialState === truereturnscopySerializedPagesQuery(nextData.query)directly. In productionnextData.queryis now{ id }only. { id, view }(server) vs{ id }(client) => React hydration mismatch for e.g./posts/42?view=fullin production.
Fix: forward options.query in the renderPagesPageResponse call to buildPagesNextDataScript (~L499). Note L539 in this same function already forwards options.query ?? options.params to the _document context, so the omission is internally inconsistent too. renderPagesIsrHtml in pages-page-data.ts:650 has the same missing forward (harmless for gsp today, but worth aligning).
| expect(nextData.__vinext).toMatchObject({ | ||
| routeUrl: "/posts/001?draft=1", | ||
| routeParams: { id: "001" }, | ||
| }); |
There was a problem hiding this comment.
Since renderNextData already captures the full serialized __NEXT_DATA__ object and the request carries a query string (/posts/001?draft=1), this test is one assertion away from locking down the parity gap flagged in pages-page-response.ts. For the gssp case, adding:
expect(nextData.query).toEqual({ id: "001", draft: "1" });would fail today (production serializes { id: "001" } only) and guards against the SSR-vs-hydration query divergence. A matching expect(nextData.query).toEqual({ id: "001" }) on the gsp case would pin the params-only behavior there.
| routeUrl || currentUrl, | ||
| routeUrl ? currentUrl : undefined, | ||
| fallbackRoute || routeUrl || currentUrl, | ||
| fallbackRoute || routeUrl ? currentUrl : undefined, |
There was a problem hiding this comment.
Minor readability: fallbackRoute || routeUrl ? currentUrl : undefined relies on || binding tighter than the ternary. It is correct ((fallbackRoute || routeUrl) ? currentUrl : undefined), but reads ambiguously next to line 313 where fallbackRoute || routeUrl || currentUrl is a value rather than a condition. Consider parenthesizing the condition:
| fallbackRoute || routeUrl ? currentUrl : undefined, | |
| await Router.replace( | |
| fallbackRoute || routeUrl || currentUrl, | |
| (fallbackRoute || routeUrl) ? currentUrl : undefined, | |
| { _h: 1, scroll: false }, | |
| ); |
| nextConfig?.clientTraceMetadata, | ||
| nextConfig?.htmlLimitedBots, | ||
| nextConfig?.reactStrictMode === true, | ||
| nextConfig?.buildId ?? process.env.__VINEXT_BUILD_ID ?? "development", |
There was a problem hiding this comment.
Behavior note (non-blocking): the dev __NEXT_DATA__.buildId now defaults to "development" where it was previously process.env.__VINEXT_BUILD_ID (typically undefined in dev). Client-side navigations build /_next/data/<buildId>/... fetch URLs from this, so worth confirming a dev-mode data navigation still resolves with buildId === "development" (or that data URLs aren't keyed on it in dev).
|
Review posted successfully. SummaryI reviewed PR #2539 ( Main finding — dev/prod parity gap (potential production hydration mismatch)The PR widens the serialized Consequence for gssp/gip routes loaded with a query string (e.g.
I suggested forwarding Minor notes
Verdict: not a rubber-stamp approval — the parity gap should be confirmed or fixed before merge, since it's the exact SSR-vs-hydration query divergence this PR is trying to eliminate. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD 916cc42 — four commits newer than the last automated round (2136078). The new commits (preserve route and cache ordering, order invalidation replacements, bypass ISR for header overrides, preserve streamed no-store responses) had not been reviewed before, so I focused there and re-verified the two long-standing ISR concerns.
The four new commits are correct
- Monotonic mutation timestamp (
1af2030/8dce219) —MemoryCacheHandlerandKVCacheHandlernow allocate a strictly-increasing timestamp forset/revalidateTagviaMath.max(now, last + 1). This is a genuine correctness fix:get()evicts whenrevalidatedAt >= entry.lastModified(cache-handler.ts:240), so aset()landing in the same millisecond as a precedingrevalidateTag()was previously evicted immediately. Both handlers are covered by a same-ms test. Sound. - App-relative asPath history normalization (
1af2030) —getCurrentHistoryAsPathnow usesremoveNavigationLocalePrefix(state.as)instead ofgetPagesRouterVisiblePath(state.as). Correct:state.asis already app-relative (basePath-stripped), so re-stripping basePath would corrupt an asPath whose first segment equalsbasePath. The new i18n-sticky-locale test pins this. - Bypass ISR for header overrides (
691bac6) — when middleware overrides request headers (buildRequestHeadersFromMiddlewareResponsereturns non-null), ISR read/write is disabled (isrGet→null,isrSet→noop,isrRevalidateSeconds→null) and the response is forcedno-store. This matches Next.js: a getServerSideProps/getStaticProps render that depends on middleware-injected request headers is request-specific and must not be CDN-shared. Coherent. - Preserve streamed no-store responses (
916cc42) — theno-storere-wrap now copies__vinextStreamedHtmlResponseonto the newResponse, matching the established pattern inpages-request-pipeline.ts:798-802. Without it, the streaming marker would be lost when the response is rebuilt and the adapter would buffer instead of stream. Correct.
Verified locally on this HEAD
tests/pages-page-handler.test.ts+tests/kv-cache-handler.test.ts+tests/isr-cache.test.ts(165),tests/pages-router-i18n-sticky-locale.test.ts+pages-page-response+pages-page-data+pages-request-pipeline(210),shims -t "pre-ready"(4).- The prior bundle-budget blocker stays resolved —
tests/pages-router.test.ts -t "produces client bundle"passes. tsc --noEmit -p packages/vinextclean.- Core router-query deferral, dev/prod query-serialization parity, and buildId default all remain correct.
The two ISR-isolation concerns from prior rounds are still open
The four new commits refine cache ordering and the request-header-override case, but neither of the two substantive ISR concerns flagged in the 81f0612/2136078 rounds is addressed. 691bac6 narrows one slice (request-header overrides bypass ISR), but a plain NextResponse.next() match, a response-header set, or a rewrite still triggers full origin-isolation + no-store. Details inline. These are the highest-risk part of the diff and unrelated to the stated fix.
Process (as every prior round noted)
The PR is scoped/titled as router-query deferral for #1971 but now spans ~40 commits and 37 files including middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert. Maintainer judgment.
| }): boolean { | ||
| return ( | ||
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true | ||
| ); |
There was a problem hiding this comment.
Still open (flagged at 81f0612 and 2136078): isolation is much broader than the rewrite-aliasing case it targets. shouldIsolatePagesMiddlewareIsr isolates on plain initialMiddlewareMatched === true, which is set on any matcher hit (pages-request-pipeline.ts:337 — result.matched === true), including middleware that only returns nothing or calls NextResponse.next() (middleware-runtime.ts:361-373).
The new 691bac6 commit narrows one slice — when middleware overrides request headers, ISR is now bypassed — but a plain pass-through match, a response-header set, or a rewrite still triggers origin-isolation.
Effect by adapter:
- default in-process (
ownsBackgroundRevalidation === true): previously not isolated → now isolated for every matched getStaticProps page. - Cloudflare CDN (
false): isolated either way.
With the common broad matcher (/((?!api|_next|favicon\.ico|...).*)), essentially every getStaticProps page behind that middleware routes ISR reads/writes to origin-only and emits no-store (pages-page-response.ts), so it is never CDN-cacheable. Next.js keeps getStaticProps pages CDN-cacheable even when non-request-varying middleware matched the path.
Keep the adapter gate for the plain-matched branch and only unconditionally isolate when the match can actually vary by request:
| ); | |
| return ( | |
| input.initialMiddlewareMatchCanVaryByRequest === true || | |
| (input.initialMiddlewareMatched === true && !getCdnCacheAdapter().ownsBackgroundRevalidation) | |
| ); |
(If broadening is deliberate for the rewrite-aliasing fix, isolating by visible pathname — see the pages-page-handler.ts comment — fixes aliasing without the CDN fallout.)
| ? (router: string, _pathname: string) => | ||
| pageIsrCacheKey( | ||
| `${router}:middleware`, | ||
| `${routerAsPath}::route=${encodeURIComponent(routeUrl)}`, |
There was a problem hiding this comment.
Still open (flagged at 81f0612 and 2136078): the isolated ISR cache key includes the query string → unbounded growth. routerAsPath retains parsed.search (via normalizeVisiblePagesUrl), and the composed key ${routerAsPath}::route=${encodeURIComponent(routeUrl)} embeds it directly. The new pages-page-handler.test.ts pins this — /visible-a?draft=1 and /visible-b?draft=2 are asserted to produce distinct keys (L500-501).
Consequence: a matched-middleware getStaticProps page hit with ?utm_source=a, ?utm_source=b, … creates a distinct origin cache entry per query string, each with its own background render — defeating ISR dedup and letting an unauthenticated client grow the in-memory origin store without bound (memory-exhaustion / cache-stampede vector). Next.js keys SSG/ISR by normalized pathname, not query.
The path-based tag already strips the query (getPagesPathCacheTags), and the on-demand invalidation at L609 uses the query-stripped tag — so the tag and the key disagree on query handling, which is what makes targeted invalidation fragile. The rewrite-aliasing case you want (/rewritten vs /mw-rewrite-query → /ssr) only needs isolation by visible pathname. Strip the query:
| `${routerAsPath}::route=${encodeURIComponent(routeUrl)}`, | |
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath.split("?")[0] : routeUrl; |
(and drop the query from the composed key at L615 the same way)
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type. They must stay in lockstep — the live SSR router.query and the serialized __NEXT_DATA__.query have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
|
Review posted successfully. SummaryI reviewed PR #2539 at HEAD The four new commits are all correct
Verified locally
Two ISR-isolation concerns from prior rounds remain open (inline, non-blocking)
Plus the long-standing DRY hazard ( Verdict: no merge-blocking correctness bug on this HEAD; the two ISR concerns are flagged for maintainer judgment. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD b30d60f — one commit newer than the last automated round (916cc42). The new commit (fix(pages): publish fallback hydration before data) had not been reviewed before, so I focused there and re-verified the two long-standing ISR concerns.
The new commit is correct
b30d60f calls _markPagesRouterHydrated() for fallback routes immediately after hydrationCommitted, before the _h:1 Router.replace (pages-client-entry.ts:296-298). I traced the chain:
- For a fallback page the
PagesRouterHydrationMarkereffect only firesmarkPagesRouterHydrated()whenisReady === true(router.ts:1454-1457), butisReadystays false until the_h:1replace's asyncrunNavigateClientdata fetch completes and reachesmarkPagesRouterReady()(router.ts:3298, non-shallow fallback path). So without this commit,__NEXT_HYDRATED/__NEXT_HYDRATED_CBwould not fire until the fallback data resolves — coupling the hydration signal to data loading. Next.js sets the hydration marker at hydration-commit time, independent of fallback data. - The early call is safe:
markPagesRouterHydratedis idempotent (window.__NEXT_HYDRATED === trueguard,router.ts:1445), andwindow.next.router.isReadystill gates onisPagesRouterReady() && __NEXT_HYDRATED(router.ts:4000), so it correctly stays false for the pre-ready fallback window. The_habort guard (router.ts:3258) can't trip during the single initial hydration replace, so readiness still resolves. entry-templates.test.tspins the new import + call in the generated entry.
Verified locally on this HEAD
tests/entry-templates.test.ts(13),shims -t "pre-ready"(4) pass.- Bundle-budget test passes —
tests/pages-router.test.ts -t "produces client bundle"green, so the prior REQUEST_CHANGES blocker stays resolved. - Core router-query deferral, dev/prod query-serialization parity, and buildId default all remain correct.
The two ISR-isolation concerns from prior rounds are still open
I re-confirmed both against this HEAD. They are the highest-risk part of the diff and are unrelated to the stated router-query fix. Details inline.
Process (as every prior round noted)
The PR is scoped/titled as router-query deferral for #1971 but now spans ~41 commits and 37 files including middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert. Maintainer judgment; not a hard block.
Event is COMMENT — I do not have merge authority; the ISR findings are flagged for maintainer judgment.
| }): boolean { | ||
| return ( | ||
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true | ||
| ); |
There was a problem hiding this comment.
Still open (flagged at 81f0612, 2136078, 916cc42): isolation is much broader than the rewrite-aliasing case it targets. shouldIsolatePagesMiddlewareIsr isolates on plain initialMiddlewareMatched === true, which is set on any matcher hit (pages-request-pipeline.ts:337 — result.matched === true), including middleware that returns nothing (middleware-runtime.ts:361-362) or calls NextResponse.next() to set a header (middleware-runtime.ts:365-373).
Effect by adapter:
- default in-process (
ownsBackgroundRevalidation === true): previously not isolated → now isolated for every matched getStaticProps page. - Cloudflare CDN (
false): isolated either way.
With the common broad matcher (the repo fixture uses /((?!api|_next|favicon\.ico|...).*)), essentially every getStaticProps page behind that middleware routes ISR reads/writes to origin-only and emits no-store (pages-page-response.ts), so it is never CDN-cacheable. Next.js keeps getStaticProps pages CDN-cacheable even when non-request-varying middleware matched the path.
Keep the adapter gate for the plain-matched branch and only unconditionally isolate when the match can actually vary by request:
| ); | |
| return ( | |
| input.initialMiddlewareMatchCanVaryByRequest === true || | |
| (input.initialMiddlewareMatched === true && !getCdnCacheAdapter().ownsBackgroundRevalidation) | |
| ); |
(If broadening is deliberate for the rewrite-aliasing fix, isolating by visible pathname — see the pages-page-handler.ts comment — fixes aliasing without the CDN fallout.)
| ? (router: string, _pathname: string) => | ||
| pageIsrCacheKey( | ||
| `${router}:middleware`, | ||
| `${routerAsPath}::route=${encodeURIComponent(routeUrl)}`, |
There was a problem hiding this comment.
Still open (flagged at 81f0612, 2136078, 916cc42): the isolated ISR cache key includes the query string → unbounded growth. routerAsPath retains parsed.search (via normalizeVisiblePagesUrl, L234), and the composed key ${routerAsPath}::route=${encodeURIComponent(routeUrl)} embeds it directly.
Consequence: a matched-middleware getStaticProps page hit with ?utm_source=a, ?utm_source=b, … creates a distinct origin cache entry per query string, each with its own background render — defeating ISR dedup and letting an unauthenticated client grow the in-memory origin store without bound (memory-exhaustion / cache-stampede vector). Next.js keys SSG/ISR by normalized pathname, not query.
Note the tag and the key disagree on query handling: the path-based tag strips the query (getPagesPathCacheTags), and the on-demand invalidation at L609 uses the query-stripped tag, but the cache key keeps it — which makes targeted invalidation fragile. The isolates origin ISR entries... test only varies path+query together (/visible-a?draft=1 vs /visible-b?draft=2), so it doesn't catch the pure-query-variation growth case. The rewrite-aliasing case you want (/rewritten vs /mw-rewrite-query → /ssr) only needs isolation by visible pathname. Strip the query:
| `${routerAsPath}::route=${encodeURIComponent(routeUrl)}`, | |
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath.split("?")[0] : routeUrl; |
(and drop the query from the composed key at L615 the same way)
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query and the serialized __NEXT_DATA__.query have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
|
|
||
| if (nextData.isFallback) { | ||
| const currentUrl = window.location.pathname + window.location.search + window.location.hash; | ||
| if (nextData.isFallback === true) { |
There was a problem hiding this comment.
Nice — publishing the hydration marker for fallback here (before the async _h:1 data fetch) is the right parity call, since otherwise __NEXT_HYDRATED/__NEXT_HYDRATED_CB wouldn't fire until fallback data resolves (the non-shallow _h:1 replace only reaches markPagesRouterReady() after runNavigateClient). Worth a one-line comment noting the invariant: this is safe because markPagesRouterHydrated is idempotent and window.next.router.isReady still gates on isPagesRouterReady(), so the fallback stays not-ready until the replace completes — a future reader might otherwise worry this leaks isReady: true early.
|
Review posted successfully. SummaryI reviewed PR #2539 at HEAD Verdict: no blocking correctness bug found on this HEAD (posted as COMMENT)The new commit is correct.
Verified locally: entry-templates (13) + pre-ready (4) pass; the prior REQUEST_CHANGES bundle-budget blocker stays resolved ( Findings posted inline
Both ISR concerns are real and unresolved; flagged for maintainer judgment (not merge-blocked, as they're outside the stated router-query scope). Also reiterated the scope-creep process concern per AGENTS.md's stacked-PR guidance. |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/vinext-pages-router-isready-1971
branch
2 times, most recently
from
6410d02 to
e5eea5c
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD e5eea5c — one commit newer than the last automated round (b30d60f, which is no longer in the branch history; the branch appears to have been reworked). The new HEAD commit (fix(pages): decouple hydration from readiness) had not been reviewed before, so I focused there and re-verified the two long-standing ISR concerns.
The new commit is correct
e5eea5c reverts the isReady-gate on PagesRouterHydrationMarker so markPagesRouterHydrated() now fires unconditionally in a mount-only effect (router.ts:1454-1457), decoupling __NEXT_HYDRATED from router readiness. I traced the chain and it is sound and closer to Next.js parity:
- Next.js sets
__NEXT_HYDRATEDat hydration-commit time, independent of router readiness. The previousisReady-gated version was the divergence; this restores parity. - Readiness is still correctly separated:
window.next.router.isReadycontinues to gate onisPagesRouterReady() && __NEXT_HYDRATED(router.ts:3999-4001), so a not-ready fallback/auto-export page stays not-ready until the client entry's post-hydrationRouter.replace({ _h: 1 })flips readiness. - The marker is idempotent (
window.__NEXT_HYDRATED === trueguard,router.ts:1445), so it is safe under StrictMode double-invocation and the removed explicit fallback call in the client entry is now correctly unnecessary. waitForHydration(tests/e2e/helpers.ts) waits on both__NEXT_HYDRATEDandrouter.isReady, so E2E coverage still exercises the readiness path.
Doc-note (non-blocking): worth a one-line comment at router.ts:1454 documenting the invariant that keeps readiness separate — __NEXT_HYDRATED signals hydration commit while router.isReady still ANDs isPagesRouterReady() — so a future reader doesn't re-gate this on isReady and reintroduce the coupling this commit removed.
Verified locally on this HEAD
shims -t "pre-ready"(4),entry-templates(41),isr-cache+pages-page-handler(100),pages-router-i18n-sticky-locale(12) all pass.- The prior REQUEST_CHANGES bundle-budget blocker stays resolved —
tests/pages-router.test.ts -t "produces client bundle"passes (the live-search-query helper still lives in thevinext-runtime chunk, keeping the generated entry thin). tsc --noEmit -p packages/vinextclean.- The earlier dev/prod
asPathlocale-parity concern is resolved: both dev (dev-server.ts:755) and prod (pages-page-handler.ts:463-467vianormalizeVisiblePagesUrl→extractLocaleFromUrl(...).url) strip the locale fromasPath.
The two ISR-isolation concerns from prior rounds are still open
I re-confirmed both against this HEAD. They are the highest-risk part of the diff and unrelated to the stated router-query fix. Details inline.
Process (as every prior round noted)
The PR is scoped/titled as router-query deferral for #1971 but now spans 41 commits and 37 files including middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert. Maintainer judgment.
Event is COMMENT — I do not have merge authority; the ISR findings are flagged for maintainer judgment, not a hard block.
| initialMiddlewareMatchCanVaryByRequest?: boolean; | ||
| }): boolean { | ||
| return ( | ||
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true |
There was a problem hiding this comment.
Still open (flagged at 81f0612, 2136078, 916cc42, b30d60f): isolation is much broader than the rewrite-aliasing case it targets. shouldIsolatePagesMiddlewareIsr isolates on plain initialMiddlewareMatched === true, which is set on any matcher hit (pages-request-pipeline.ts:337 — result.matched === true), including middleware that returns nothing (middleware-runtime.ts:361-362) or calls NextResponse.next() to set a header (middleware-runtime.ts:365-373).
Effect by adapter:
- default in-process (
ownsBackgroundRevalidation === true): previously not isolated → now isolated for every matched getStaticProps page. - Cloudflare CDN (
false): isolated either way.
With the common broad matcher (the repo fixture uses /((?!api|_next|favicon\.ico|...).*)), essentially every getStaticProps page behind that middleware routes ISR reads/writes to origin-only and emits no-store, so it is never CDN-cacheable. Next.js keeps getStaticProps pages CDN-cacheable even when non-request-varying middleware matched the path.
Keep the adapter gate for the plain-matched branch and only unconditionally isolate when the match can actually vary by request:
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true | |
| input.initialMiddlewareMatchCanVaryByRequest === true || | |
| (input.initialMiddlewareMatched === true && !getCdnCacheAdapter().ownsBackgroundRevalidation) |
(If broadening is deliberate for the rewrite-aliasing fix, isolating by visible pathname — see the pages-page-handler.ts comment — fixes aliasing without the CDN fallout.)
| middlewareHeaders !== null && | ||
| middlewareHeaders !== undefined && | ||
| buildRequestHeadersFromMiddlewareResponse(request.headers, middlewareHeaders) !== null; | ||
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath : routeUrl; |
There was a problem hiding this comment.
Still open (flagged at 81f0612, 2136078, 916cc42, b30d60f): the isolated ISR cache key includes the query string → unbounded growth. routerAsPath retains parsed.search (via normalizeVisiblePagesUrl, L234), and the composed key ${routerAsPath}::route=${encodeURIComponent(routeUrl)} (L615) embeds it directly.
Consequence: a matched-middleware getStaticProps page hit with ?utm_source=a, ?utm_source=b, … creates a distinct origin cache entry per query string, each with its own background render — defeating ISR dedup and letting an unauthenticated client grow the in-memory origin store without bound (memory-exhaustion / cache-stampede vector). Next.js keys SSG/ISR by normalized pathname, not query.
Note the tag and the key disagree on query handling: the path-based tag strips the query (getPagesPathCacheTags) and the on-demand invalidation at L609 uses the query-stripped tag, but the cache key keeps it — which makes targeted invalidation fragile. The isolates origin ISR entries... test (pages-page-handler.test.ts:489) only varies path+query together (/visible-a?draft=1 vs /visible-b?draft=2), so it does not catch the pure-query-variation growth case. The rewrite-aliasing case you want (/rewritten vs /mw-rewrite-query → /ssr) only needs isolation by visible pathname. Strip the query:
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath : routeUrl; | |
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath.split("?")[0] : routeUrl; |
(and drop the query from the composed key at L615 the same way)
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query and the serialized __NEXT_DATA__.query have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
|
Review posted successfully. SummaryI reviewed PR #2539 ( Verdict: no blocking correctness bug found on this HEAD (posted as COMMENT)The new commit is correct.
Verified locally: pre-ready (4), entry-templates (41), isr-cache + pages-page-handler (100), i18n-sticky-locale (12) all pass; the prior bundle-budget blocker stays resolved; Two ISR-isolation concerns remain open (flagged inline)
Plus the long-standing byte-identical |
james-elicx
force-pushed
the
codex/vinext-pages-router-isready-1971
branch
from
e5eea5c to
1982c20
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed the checked-out branch at HEAD 1982c20 (fix(pages): decouple hydration from readiness). I independently verified the core fix, the two significant concerns resolved in earlier rounds, and re-confirmed the two ISR-isolation concerns that remain open.
The core fix is correct
The empty-router.query deferral for pre-ready auto-export dynamic / fallback renders matches Next.js render.tsx (query = {} for isAutoExport || isFallback while isReady === false). I verified:
- Hydration/readiness decoupling on this HEAD is sound.
markPagesRouterHydrated(router.ts:1444) fires unconditionally in a mount-only effect and is idempotent (window.__NEXT_HYDRATED === trueguard), matching Next.js which sets__NEXT_HYDRATEDat commit time.router.isReadyremains separately gated onisPagesRouterReady() && __NEXT_HYDRATED, so not-ready routes stay not-ready until the post-hydrationRouter.replace({ _h: 1 }). - Dev/prod query-serialization parity holds.
renderPagesPageResponseforwardsquery: options.querytobuildPagesNextDataScript(pages-page-response.ts:515), so production serializes the full merged__NEXT_DATA__.queryfor gssp/gip, matching the dev path and the server React render. - Tests pass locally.
shims -t "pre-ready"(4) and the prior REQUEST_CHANGES bundle-budget blocker stays resolved —tests/pages-router.test.ts -t "produces client bundle"passes (the live-search-query helper lives in thevinext-runtime chunk, keeping the generated entry thin).
Two ISR-isolation concerns remain open (git-confirmed as regressions in this PR)
I traced both through git history and confirmed they are introduced by this PR, not pre-existing. They are the highest-risk part of the diff and are unrelated to the stated router-query fix. Details inline. Event is COMMENT — flagging for maintainer judgment, not a hard block.
Process
As every prior round noted, the PR is scoped/titled as router-query deferral for #1971 but spans 40+ commits and 37 files including middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert.
| initialMiddlewareMatchCanVaryByRequest?: boolean; | ||
| }): boolean { | ||
| return ( | ||
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true |
There was a problem hiding this comment.
ISR isolation is much broader than the rewrite-aliasing case it targets — git-confirmed regression in this PR.
This predicate was introduced in db42197 with the adapter gate:
input.initialMiddlewareMatchCanVaryByRequest === true ||
(input.initialMiddlewareMatched === true && !getCdnCacheAdapter().ownsBackgroundRevalidation)Commit 81f0612 dropped the !ownsBackgroundRevalidation gate, so it now isolates on plain matched.
initialMiddlewareMatched is set on any matcher hit (pages-request-pipeline.ts:337 — result.matched === true), and middleware-runtime.ts returns matched: true for middleware that returns nothing (L362) or calls NextResponse.next() to set a header (L365-373). Effect by adapter:
- default in-process (
ownsBackgroundRevalidation === true,cdn-cache.ts:129): previously not isolated → now isolated for every matched getStaticProps page. - Cloudflare CDN (
false,cdn-adapter.runtime.ts:126): isolated either way.
With the extremely common broad matcher (the repo fixture uses /((?!api|_next|favicon\.ico|...).*), middleware.ts:296), essentially every getStaticProps page behind that middleware now routes ISR reads/writes to origin-only and emits no-store, so it is never CDN-cacheable. Next.js keeps getStaticProps pages CDN-cacheable even when non-request-varying middleware matched the path.
Restore the adapter gate for the plain-matched branch and only unconditionally isolate when the match can actually vary by request:
| input.initialMiddlewareMatched === true || input.initialMiddlewareMatchCanVaryByRequest === true | |
| return ( | |
| input.initialMiddlewareMatchCanVaryByRequest === true || | |
| (input.initialMiddlewareMatched === true && !getCdnCacheAdapter().ownsBackgroundRevalidation) | |
| ); |
(If broadening is deliberate for the rewrite-aliasing fix, isolating by visible pathname — see the pages-page-handler.ts comment — fixes aliasing without the CDN fallout.)
| middlewareHeaders !== null && | ||
| middlewareHeaders !== undefined && | ||
| buildRequestHeadersFromMiddlewareResponse(request.headers, middlewareHeaders) !== null; | ||
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath : routeUrl; |
There was a problem hiding this comment.
Isolated ISR cache key includes the query string → unbounded growth. routerAsPath retains parsed.search (normalizeVisiblePagesUrl, L234: stripBasePath(parsed.pathname, basePath) + parsed.search), and the composed key at L615 (${routerAsPath}::route=${encodeURIComponent(routeUrl)}) embeds it directly.
Consequence: a matched-middleware getStaticProps page hit with ?utm_source=a, ?utm_source=b, … creates a distinct origin cache entry per query string, each with its own background render — defeating ISR dedup and letting an unauthenticated client grow the in-memory origin store without bound (memory-exhaustion / cache-stampede vector). Next.js keys SSG/ISR by normalized pathname, not query.
The tag and the key disagree on query handling: the path-based tag strips the query (getPagesPathCacheTags) and the on-demand invalidation at L609 uses the query-stripped tag, but the cache key keeps it — which makes targeted invalidation fragile. The isolates origin ISR entries... test (pages-page-handler.test.ts:489) only varies path+query together (/visible-a?draft=1 vs /visible-b?draft=2), so it does not catch the pure-query-variation growth case.
The rewrite-aliasing case you want (/rewritten vs /mw-rewrite-query → /ssr) only needs isolation by visible pathname. Strip the query:
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath : routeUrl; | |
| const isrIdentityUrl = isolateMiddlewareIsr ? routerAsPath.split("?")[0] : routeUrl; |
(and drop the query from the composed key at L615 the same way)
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query (from getPagesInitialRouterQuery) and the serialized __NEXT_DATA__.query (from getPagesSerializedRouterQuery) have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
Collapse them so a future edit can't drift one from the other:
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone function body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
|
Review posted successfully. SummaryI reviewed PR #2539 at HEAD Verdict: No blocking correctness bug in the core fix; two ISR concerns flagged for maintainer judgment (COMMENT event)The core router-query fix is correct. I independently verified:
Two ISR-isolation concerns remain open — I git-confirmed both are regressions introduced by this PR:
Both came with |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD 42f47bd — one commit newer than the last automated round (1982c20). The new commit, fix(pages): preserve middleware ISR identity, directly reworks the two long-standing ISR concerns from prior rounds — and it's a good direction: it stops isolating on plain matched (introducing middlewareAffectsCacheIdentity = matched && (rewrite || middleware set headers)) and strips the query from the isolated cache key (isrIdentityUrl = visiblePathname, key uses visiblePathname/routePathname). The prior over-broad-isolation and query-keyed-growth concerns are addressed on the MISS path.
However, this commit introduces a blocking regression. It changed the shouldIsolatePagesMiddlewareIsr signature to read middlewareAffectsCacheIdentity instead of initialMiddlewareMatched, but only the MISS path in pages-page-handler.ts computes and passes that field. The three HIT/STALE/background-regen call sites in pages-page-data.ts still call shouldIsolatePagesMiddlewareIsr(options.vinext ?? {}), and options.vinext is the serialized __vinext block, which carries initialMiddlewareMatched / initialMiddlewareMatchCanVaryByRequest but never middlewareAffectsCacheIdentity (see the type in client/vinext-next-data.ts:40-54 and the serialization at pages-page-handler.ts:547-556). So on those paths, isolation now silently keys off initialMiddlewareMatchCanVaryByRequest only, dropping the rewrite / header-modifying (middlewareAffectsCacheIdentity) case entirely.
Blocking: tests/pages-page-data.test.ts fails deterministically on this HEAD
AssertionError: expected 'public, max-age=0, must-revalidate' to be 'no-store'
at tests/pages-page-data.test.ts:1125
The test keeps matched middleware cache HITs out of an active edge cache sets vinext: { hasMiddleware: true, initialMiddlewareMatched: true } and expects the HIT to be forced no-store. It now returns public, max-age=0, must-revalidate because shouldIsolatePagesMiddlewareIsr(options.vinext) at pages-page-data.ts:919 returns false (the field it needs isn't serialized). This test was not updated by the commit, and the Vitest required check will fail, blocking merge. (I ran the file: 1 failed / 61 passed, reproducible.)
Beyond the failing test, this is the exact MISS-vs-HIT cache-parity divergence AGENTS.md warns about: a rewrite-aliased getStaticProps page isolates correctly on its first (MISS) render but, once cached under the isolated key, its HIT/STALE responses no longer get no-store, and the background-regen forceOrigin at pages-page-data.ts:1017 is false when it should be true — so regeneration writes back through the wrong (shared) cache.
Suggested fix: the handler already computes isolateMiddlewareIsr (pages-page-handler.ts:606) and forwards it to renderPagesPageResponse (:862). Forward that same boolean into resolvePagesPageData and consume it at the three pages-page-data.ts sites instead of re-deriving from the serialized __vinext block — mirroring how this very commit changed renderPagesPageResponse to take an explicit isolateMiddlewareIsr option rather than calling shouldIsolatePagesMiddlewareIsr(options.nextData?.__vinext ?? {}).
What I verified is correct on this HEAD
- Core router-query deferral matches Next.js
render.tsx;pre-readytests (4) green. - Prior bundle-budget blocker stays resolved (
produces client bundlepasses). - The query-stripping fix is sound:
visiblePathname/routePathnameare both.pathname(query-dropped), so the rewrite detection isn't confused by query strings, and the new pure-query test (pages-page-handler.test.ts:508-517) asserts one key for?draft=1/?draft=2. - Pass-through
NextResponse.next()(empty staged headers, no rewrite) correctly no longer isolates on the default adapter — the flippedpreserves edge caching for matched pass-through middlewaretest passes.
Long-standing DRY note (non-blocking, open since round 1)
getPagesInitialRouterQuery and getPagesSerializedRouterQuery are still byte-identical; suggested collapsing to an alias inline.
| cached.value.cacheControl, | ||
| cachedValue.status, | ||
| ); | ||
| if (shouldIsolatePagesMiddlewareIsr(options.vinext ?? {})) { |
There was a problem hiding this comment.
Blocking — MISS-vs-HIT ISR isolation divergence introduced by 42f47bd. This (and L1017, L1033) call shouldIsolatePagesMiddlewareIsr(options.vinext ?? {}), but the new predicate signature reads middlewareAffectsCacheIdentity, which is never serialized into the __vinext block (pages-page-handler.ts:547-556; type at client/vinext-next-data.ts:40-54). So the HIT / STALE / background-regen paths now isolate on initialMiddlewareMatchCanVaryByRequest only, silently dropping the rewrite / header-modifying case that the MISS path in pages-page-handler.ts:600-608 correctly isolates.
This breaks tests/pages-page-data.test.ts:1093 (keeps matched middleware cache HITs out of an active edge cache) deterministically:
expected 'public, max-age=0, must-revalidate' to be 'no-store' (L1125)
The Vitest required check will fail.
The handler already computes isolateMiddlewareIsr (pages-page-handler.ts:606) and forwards it to renderPagesPageResponse (:862). Forward the same boolean into resolvePagesPageData and consume it here instead of re-deriving from the serialized block — mirroring how this commit changed renderPagesPageResponse to take an explicit isolateMiddlewareIsr flag:
| if (shouldIsolatePagesMiddlewareIsr(options.vinext ?? {})) { | |
| if (options.isolateMiddlewareIsr === true) { |
| routePath: options.routePattern, | ||
| routeType: "render", | ||
| }, | ||
| { forceOrigin: shouldIsolatePagesMiddlewareIsr(options.vinext ?? {}) }, |
There was a problem hiding this comment.
Same root cause as L919: forceOrigin here is derived from the serialized __vinext block, which lacks middlewareAffectsCacheIdentity, so for a rewrite/header-identity HIT this is false when the MISS path isolated it to origin — background regeneration then writes back through the shared cache instead of origin. Use the forwarded options.isolateMiddlewareIsr boolean:
| { forceOrigin: shouldIsolatePagesMiddlewareIsr(options.vinext ?? {}) }, | |
| { forceOrigin: options.isolateMiddlewareIsr === true }, |
| cached.value.cacheControl, | ||
| cachedValue.status, | ||
| ); | ||
| if (shouldIsolatePagesMiddlewareIsr(options.vinext ?? {})) { |
There was a problem hiding this comment.
Same root cause as L919/L1017 — STALE responses for a rewrite/header-identity page won't get no-store because middlewareAffectsCacheIdentity isn't in the serialized block. Use the forwarded flag:
| if (shouldIsolatePagesMiddlewareIsr(options.vinext ?? {})) { | |
| if (options.isolateMiddlewareIsr === true) { |
| @@ -632,6 +693,7 @@ export function createPagesPageHandler( | |||
| triggerBackgroundRegeneration, | |||
| vinext: serializedPagesNextData.__vinext, | |||
There was a problem hiding this comment.
resolvePagesPageData receives isrGet/isrSet (L644-653) and cacheRouteUrl (L696) computed from isolateMiddlewareIsr, but not the boolean itself — so the HIT/STALE/regen paths inside pages-page-data.ts re-derive isolation from options.vinext and get it wrong for the identity case. Forward the already-computed flag alongside vinext:
| vinext: serializedPagesNextData.__vinext, | |
| vinext: serializedPagesNextData.__vinext, | |
| isolateMiddlewareIsr, |
(and add isolateMiddlewareIsr?: boolean to ResolvePagesPageDataOptions, then consume it at the three call sites above).
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query and the serialized __NEXT_DATA__.query have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
|
Review posted successfully as REQUEST_CHANGES. SummaryI reviewed PR #2539 at HEAD Blocking finding (CI will fail)The commit changed I confirmed this concretely: Suggested fix: forward the already-computed Verified correct
Also re-flagged the long-standing byte-identical |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD 0c61578 — one commit newer than the last automated round (42f47bd, which was flagged REQUEST_CHANGES for a MISS-vs-HIT ISR isolation regression). The new commit, fix(pages): propagate middleware ISR isolation, directly addresses that blocking finding and it holds up.
The prior blocking issue is resolved
The 42f47bd round found that shouldIsolatePagesMiddlewareIsr had switched to reading middlewareAffectsCacheIdentity, but only the MISS path computed it — the HIT/STALE/background-regen sites in pages-page-data.ts still re-derived isolation from the serialized __vinext block (which never carries that field), causing tests/pages-page-data.test.ts:1125 to fail deterministically. This is fixed on HEAD. The handler now forwards an explicit isolateMiddlewareIsr boolean into resolvePagesPageData (pages-page-handler.ts:658), and the three pages-page-data.ts sites consume options.isolateMiddlewareIsr === true (L913 HIT, L1014 regen forceOrigin, L1030 STALE) instead of re-deriving. I ran the file: tests/pages-page-data.test.ts passes (95/95 with pages-page-handler).
The two long-standing ISR concerns are addressed on this HEAD
Both concerns raised across the 81f0612–1982c20 rounds are resolved by the 42f47bd/0c61578 reworks:
- Over-broad isolation —
shouldIsolatePagesMiddlewareIsr(isr-cache.ts:186) no longer isolates on plainmatched. It isolates oninitialMiddlewareMatchCanVaryByRequestORmiddlewareAffectsCacheIdentity, and the latter (pages-page-handler.ts:608) requiresmatched && (rewrite || status-override || response-headers-set). A pass-throughNextResponse.next()with no headers no longer isolates, so getStaticProps pages behind a broad matcher stay CDN-cacheable. The flippedpreserves edge caching for matched pass-through middlewaretest confirms this. - Query-keyed cache growth — the isolated key now uses
visiblePathname(.pathname, query-stripped) andmiddlewareRouteIdentity(rewrite pathname+search, orroutePathname), not the fullrouterAsPathwithparsed.search.isrIdentityUrl = visiblePathname(pages-page-handler.ts:623). A page hit with?utm_source=a/b/c…now maps to a single origin entry, and the key aligns with the query-stripped tag.
Verified locally on this HEAD
tests/pages-page-data.test.ts+tests/pages-page-handler.test.ts(95) — pass (the prior blocker).tests/isr-cache.test.ts+tests/kv-cache-handler.test.ts+tests/middleware-runtime.test.ts+tests/pages-request-pipeline.test.ts+tests/pages-page-response.test.ts(284) — pass.- Full
tests/pages-router.test.ts(352) — pass, including the bundle-budget test (produces client bundle...) that was the6277900REQUEST_CHANGES blocker. tests/shims.test.ts -t "pre-ready"(4),tests/pages-router-i18n-sticky-locale.test.ts+tests/entry-templates.test.ts+tests/middleware-runtime-trailing-slash.test.ts(66) — pass.tsc --noEmit -p packages/vinextclean;vp lintclean.- The monotonic mutation-timestamp fix (
Math.max(now, last+1)) in bothMemoryCacheHandlerandKVCacheHandleris a genuine same-msset/revalidateTagrace fix, consistently applied. middlewareMatchCanVaryByRequestcorrectly short-circuits unconditional matchers (string patterns and has/missing-free objects) tofalse.
Verdict: no blocking correctness bug found on this HEAD.
One dev/prod ISR-isolation parity gap (non-blocking, dev ISR is best-effort), the long-standing DRY hazard, and two minor notes below. Plus the scope-creep process concern every prior round flagged.
Process (as prior rounds noted)
The PR is titled/scoped as router-query deferral for #1971 but spans 43 commits and 37 files, also landing middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert. Maintainer judgment; not a merge blocker.
| const isolateMiddlewareIsr = | ||
| initialMiddlewareMatchCanVaryByRequest || | ||
| middlewareRewriteUrl !== null || | ||
| statusCode !== undefined; |
There was a problem hiding this comment.
Non-blocking dev/prod ISR-isolation parity gap. The dev isolateMiddlewareIsr here diverges from the production handler's decision (pages-page-handler.ts:608-618) in two ways:
- Dev does not include the response-header-set case. Prod isolates when
middlewareHeadershas keys (pages-page-handler.ts:612-614); dev only checksmatchCanVaryByRequest || rewrite || status. - Dev does not gate on
initialMiddlewareMatched. Prod'smiddlewareAffectsCacheIdentityrequiresinitialMiddlewareMatched === true; dev keys offmiddlewareRewriteUrl/statusCodedirectly.
In practice dev ISR collapses to the same store and just varies the Cache-Control header, so the blast radius is small — but AGENTS.md explicitly calls out dev/prod parity as high-risk. A middleware that sets a response header on a getStaticProps page would emit no-store in prod but not in dev. Worth aligning the dev computation with the prod middlewareAffectsCacheIdentity predicate (or extracting a shared helper) so the two can't drift, and adding a dev-path test for the header-set case.
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query (from getPagesInitialRouterQuery) and the serialized __NEXT_DATA__.query (from getPagesSerializedRouterQuery) have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
Collapse them so a future edit can't drift one from the other:
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
| routeUrl ? currentUrl : undefined, | ||
| { _h: 1, scroll: false }, | ||
| fallbackRoute || routeUrl || currentUrl, | ||
| fallbackRoute || routeUrl ? currentUrl : undefined, |
There was a problem hiding this comment.
Minor readability (generated template): this relies on || binding tighter than the ternary. It is correct ((fallbackRoute || routeUrl) ? currentUrl : undefined) but reads ambiguously next to L322 where fallbackRoute || routeUrl || currentUrl is a value, not a condition. Parenthesize the condition to make intent explicit:
| fallbackRoute || routeUrl ? currentUrl : undefined, | |
| fallbackRoute || routeUrl ? currentUrl : undefined, |
(Suggestion left byte-identical; the ask is only to wrap (fallbackRoute || routeUrl) in parens if you touch this line.)
| const currentValue = parsed.__vinext.initialMiddlewareMatched === true; | ||
| if (currentValue === initialMiddlewareMatched) return html; | ||
| const currentLiteral = `"initialMiddlewareMatched":${currentValue ? "true" : "false"}`; | ||
| const valueIndex = json.lastIndexOf(currentLiteral); |
There was a problem hiding this comment.
rewritePagesInitialMiddlewareMatched rewrites only the last occurrence via json.lastIndexOf(currentLiteral). This is correct today because buildPagesNextDataScript serializes user props first and the top-level __vinext block last, and JSON.stringify preserves insertion order — so the top-level __vinext.initialMiddlewareMatched is always the last "initialMiddlewareMatched":<bool> literal. It is a little fragile: if a future serialization-order change ever placed user-controlled data (which could legitimately contain the literal string) after __vinext, the wrong occurrence would be silently rewritten. Worth a one-line comment noting the reliance on __vinext being serialized last so a future reorder doesn't quietly break the rewrite target.
|
Review posted successfully. SummaryI reviewed PR #2539 at HEAD Verdict: no blocking correctness bug found on this HEAD (posted as COMMENT)The prior blocking issue is fixed. The The two long-standing ISR concerns are now addressed:
Verified locally: Non-blocking notes posted inline:
Plus the scope-creep process concern (43 commits / 37 files well beyond the stated router-query fix) flagged for maintainer judgment. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): defer router query until ready
Reviewed at HEAD 6a0bca9 — one commit newer than the last automated round (0c61578). The new commit, fix(pages): align middleware ISR identity in dev, directly targets the dev/prod ISR-isolation parity gap flagged in the previous round. I focused there and re-verified the earlier concerns.
Verdict: no blocking correctness bug found on this HEAD (posted as COMMENT)
The new commit materially narrows the dev/prod gap. Dev's isolateMiddlewareIsr now includes initialMiddlewareOverridesRequestHeaders (dev-server.ts:763-767), the field is threaded through the pipeline (pages-request-pipeline.ts) into both the internal renderPage calls and result.renderOptions, and forwarded to the dev SSR handler (index.ts:5286). The dev ISR key is now computed once up front (dev-server.ts:768-774) and reused across the HIT / STALE / regen / data-req / streamed-ISR write paths, replacing the several ad-hoc pagesIsrCacheKey(url.split("?")[0]) sites that previously ignored isolation — a real internal-consistency fix. Request-header-override renders now correctly bypass the ISR read/write in dev (isrGet → null, data-req and streamed-ISR writes skipped).
The two long-standing ISR-isolation concerns from prior rounds are resolved on this HEAD:
- Over-broad isolation —
shouldIsolatePagesMiddlewareIsr(isr-cache.ts:186) no longer isolates on plainmatched; it requiresmatchCanVaryByRequestORmiddlewareAffectsCacheIdentity(=matched && (rewrite || status || response-headers-set)). Pass-throughNextResponse.next()stays CDN-cacheable. - Query-keyed growth — the isolated key uses
visiblePathname+ query-strippedmiddlewareRouteIdentity(pages-page-handler.ts:606-608now drops.search). The new pure-rewrite-query test (pages-page-handler.test.ts:521-530) asserts one key across?variant=one/two.
Verified locally on this HEAD:
tests/pages-page-data.test.ts+tests/pages-page-handler.test.ts+tests/pages-request-pipeline.test.ts(178) pass.tests/isr-cache.test.ts+tests/kv-cache-handler.test.ts+tests/middleware-runtime*.test.ts+tests/pages-router-i18n-sticky-locale.test.ts+tests/entry-templates.test.tspass.- The prior REQUEST_CHANGES bundle-budget blocker stays resolved —
tests/pages-router.test.ts -t "produces client bundle"passes. tsc --noEmit -p packages/vinextclean;vp lintclean.- Core router-query deferral, dev/prod query-serialization parity, and buildId default all remain correct.
Notes (non-blocking)
- Dev/prod ISR-isolation parity still not fully closed. This commit aligns the request-header-override case, but dev's predicate (
dev-server.ts:763-767) still diverges from prod'smiddlewareAffectsCacheIdentity(pages-page-handler.ts:609-615): dev does not include the response-header-set case and does not gate oninitialMiddlewareMatched. A getStaticProps page behind middleware that only sets a response header would isolate +no-storein prod but not in dev. Blast radius is small (dev ISR is best-effort, single store), but per AGENTS.md dev/prod parity is high-risk — a shared predicate would prevent future drift. - DRY hazard (open since round 1).
getPagesInitialRouterQuery(pages-readiness.ts:32) andgetPagesSerializedRouterQuery(L43) are still byte-identical apart from the declared return type; they must stay in lockstep or the exact hydration mismatch this PR fixes returns. - Minor readability in the generated client entry (
pages-client-entry.ts:323).
Process (as every prior round noted)
The PR is scoped/titled as router-query deferral for #1971 but now spans 44 commits and 37 files including middleware ISR origin-isolation, forceOrigin, CDN no-store gating, middlewareMatchCanVaryByRequest, query-keyed isolation, tag invalidation, monotonic cache timestamps, and header-override ISR bypass. Per AGENTS.md's stacked-PR guidance the ISR/middleware half would ideally be split for independent review/revert. Maintainer judgment.
| initialMiddlewareMatchCanVaryByRequest || | ||
| middlewareRewriteUrl !== null || | ||
| statusCode !== undefined || | ||
| initialMiddlewareOverridesRequestHeaders; |
There was a problem hiding this comment.
Non-blocking dev/prod ISR-isolation parity gap (partially closed by this commit). This now adds initialMiddlewareOverridesRequestHeaders, aligning the request-header-override case with prod. But the dev predicate still diverges from prod's middlewareAffectsCacheIdentity (pages-page-handler.ts:609-615) in two ways:
- Dev does not include the response-header-set case. Prod isolates when the middleware staged response headers (
[...middlewareHeaders.keys()].length > 0); dev has no equivalent — it only checksmatchCanVaryByRequest || rewrite || status || overridesRequestHeaders. - Dev does not gate on
initialMiddlewareMatched. Prod'smiddlewareAffectsCacheIdentityrequiresinitialMiddlewareMatched === truebefore the rewrite/status/headers checks.
So a getStaticProps page behind middleware that only sets a response header would emit no-store in prod but not in dev. Dev ISR is best-effort (single store, just varies Cache-Control), so the blast radius is small — but AGENTS.md flags dev/prod parity as high-risk. Consider extracting a shared middlewareAffectsCacheIdentity-style predicate so the two can't drift, and add a dev-path test for the response-header-set case (there is currently none).
| return nextData?.gsp === true ? params : query; | ||
| } | ||
|
|
||
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( |
There was a problem hiding this comment.
Non-blocking DRY hazard (open since round 1). getPagesInitialRouterQuery (L32) and getPagesSerializedRouterQuery (L43) are still byte-identical apart from the declared return type — same isFallback / autoExport / gsp guards, same result. They must stay in lockstep: the live SSR router.query (from getPagesInitialRouterQuery) and the serialized __NEXT_DATA__.query (from getPagesSerializedRouterQuery) have to agree, and a silent divergence reintroduces the exact hydration mismatch this PR fixes.
Collapse them so a future edit can't drift one from the other:
| export function getPagesSerializedRouterQuery<T extends Record<string, unknown>>( | |
| // The serialized `__NEXT_DATA__.query` must equal the live SSR snapshot, so it | |
| // is intentionally the same computation. Kept as an alias so the two call sites | |
| // stay in lockstep by construction rather than by convention. | |
| export const getPagesSerializedRouterQuery = getPagesInitialRouterQuery; |
(Drop the standalone function body and export the alias. If a future divergence is intended, document the expected difference so nobody "re-deduplicates" incorrectly.)
| routeUrl ? currentUrl : undefined, | ||
| { _h: 1, scroll: false }, | ||
| fallbackRoute || routeUrl || currentUrl, | ||
| fallbackRoute || routeUrl ? currentUrl : undefined, |
There was a problem hiding this comment.
Minor readability (generated template): this relies on || binding tighter than the ternary. It is correct ((fallbackRoute || routeUrl) ? currentUrl : undefined) but reads ambiguously next to L322 where fallbackRoute || routeUrl || currentUrl is a value, not a condition. Parenthesize the condition to make intent explicit:
| fallbackRoute || routeUrl ? currentUrl : undefined, | |
| (fallbackRoute || routeUrl) ? currentUrl : undefined, |
|
Review posted successfully. SummaryI reviewed PR #2539 at HEAD Verdict: no blocking correctness bug found on this HEADThe new commit materially narrows the dev/prod gap. Dev's Both long-standing ISR concerns are resolved on this HEAD:
Verified locally: targeted test suites pass (178+ across pages/isr/middleware/pipeline), the prior bundle-budget blocker stays resolved, Notes posted (all non-blocking)
|
2 participants
Summary
useRouter().queryduring SSR for pre-ready auto-export dynamic Pages routes so it matches Next.js's{}not-ready snapshotCloses #1971
Validation
corepack pnpm exec vitest run tests/shims.test.ts -t "pre-ready auto-export" --reporter dotcorepack pnpm exec vitest run tests/shims.test.ts -t "pre-ready fallback" --reporter verbosecorepack pnpm test tests/shims.test.ts(1187/1187)corepack pnpm run check(format/lint/type)git diff --checkPreflight
gh pr list --search 1971and targetedgh search prsforisReady, auto-export dynamic routes, anduseRouter().queryterms returned no open PRs