fix(pages): preserve prerender rewrite query parity - #2218
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/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.
Review: prerender rewrite query parity (#2218)
Solid, well-tested change overall — the server→client query-ownership transport, prototype-pollution-safe setOwnQueryValue, and the header-spoofing guard (x-vinext-resolved-query is stripped from user headers before being re-set) are all correct, and the fix is wired consistently across dev (index.ts → dev-server.ts), prod (prod-server.ts), and worker (deploy.ts) paths. But there is one blocking issue.
Blocking: a test added by this PR fails
tests/shims.test.ts → "stamps initial router state without dropping third-party history fields" fails:
Tests 1 failed | 1145 passed
expected { analyticsEntry: 'landing', … } to deeply equal ObjectContaining{ __N: true, __vinext_queryOwner: "server", url: "/", as: "/", … }
This will fail the required Vitest check.
The cause is a mismatch between the new docstring/intent and the implementation in stampInitialHistoryState (packages/vinext/src/shims/router.ts:813). The docstring now says it should "preserve fields owned by userland or third-party history integrations while ensuring the router metadata needed for back/forward and query ownership is present", and the test sets a foreign initial history.state ({ analyticsEntry, nested }) and expects router metadata (__N, key, url, as, __vinext_queryOwner) to be merged in. But the implementation still early-returns whenever any existing state is present:
const existingState = window.history.state;
if (existingState !== null && existingState !== undefined) {
routerRuntimeState.currentHistoryKey =
getRouterStateKey(existingState) ?? routerRuntimeState.currentHistoryKey;
return; // <-- never merges __N / __vinext_queryOwner / url / as
}So when third-party state is present, the entry keeps no __N, which means isNextRouterState() returns false for it and getCurrentQueryOwner() falls back to "browser" — defeating the server query-ownership the PR is trying to preserve on the very first entry.
Either:
- update
stampInitialHistoryStateto merge the router-shaped fields into a non-router existing state (preserving foreign keys) so it matches the docstring and the test, or - if the early-return is the intended behavior, drop/adjust the test and revert the docstring.
Given the PR's stated goal (preserving server query ownership across hydration and history traversal), the merge behavior described by the docstring looks like the correct fix.
Non-blocking notes
-
Header serialization is inconsistent between dev and prod.
pages-page-handler.ts:684usessafeJsonStringify(query)(HTML-escapes<,>,&) for thex-vinext-resolved-queryheader, whiledev-server.ts:1465uses plainJSON.stringify(query). The client parses both withJSON.parse, so this is functionally equivalent, butsafeJsonStringifyis intended for inline-script HTML contexts, not header values. Consider using plainJSON.stringifyin both for clarity/parity. -
initialResolvedQueryis serialized into__NEXT_DATA__for every Pages page (pages-page-handler.ts:519,dev-server.ts:1592), even pages with no rewrites/middleware where it just duplicatesquery. Consider gating it onhasRewrites || hasMiddlewareto avoid the per-page payload cost on the common path. -
Config rewrites over-record rewrite query keys. For middleware rewrites,
recordRewriteQueryKeys(rewriteUrl, originalResolvedUrl)correctly excludes unchanged keys, but config rewrites callrecordRewriteQueryKeys(rewritten)with no inherited URL, so a destination key whose value happens to equal a user-supplied search param (e.g. user?a=1, rewrite to/bar?a=1) gets hidden pre-hydration. The final value is identical, so there is no user-visible value difference, but it may cause an extra pre-ready render with the key absent. Minor parity nit. -
Redundant
extractRouteParamsFromPathcall innavigationRequiresServerQueryOwnership(router.ts:1247and again at:1253) on the navigation hot path — compute once and reuse.
|
To https://github.com/cloudflare/vinext.git |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: prerender rewrite query parity (#2218) — re-review @ fffb8fc
The previous blocking issue (stampInitialHistoryState early-returning instead of merging foreign state) has been fixed — it now does { ...existingState, ...initialState } (packages/vinext/src/shims/router.ts:822), and the "stamps initial router state without dropping third-party history fields" test passes. The newest commit (fffb8fc) also correctly defers the next/router import via await import("next/router") so the module's top-level installPagesRouterRuntime() → stampInitialHistoryState() runs after window.__NEXT_DATA__ (and thus __NEXT_DATA__.locale) is populated. Good fix.
However, that same commit introduces a new blocking failure.
Blocking: a test in this PR fails (tests/entry-templates.test.ts)
tests/entry-templates.test.ts → "gracefully skips Pages Router initialization without NEXT_DATA" now fails:
TypeError: A dynamic import callback was not specified.
Serialized Error: { code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' }
Tests 1 failed | 6 passed | 26 skipped (33)
This will fail the required Vitest check.
Cause: Commit fffb8fc changed the generated Pages client entry from a static
import Router, { ... } from "next/router" to
const { default: Router, ... } = await import("next/router")
(packages/vinext/src/entries/pages-client-entry.ts:159 and packages/vinext/src/server/dev-server.ts:1613).
The sibling test at tests/entry-templates.test.ts:1161 slices the generated initializationCode and executes it with vm.runInNewContext(...) (line 1184). That context provides _initializePagesRouterReadyFromNextData as a global but no importModuleDynamically callback, so the newly-added await import("next/router") throws ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING before the code can reach its intended "[vinext] No __NEXT_DATA__ found" path.
The PR updated the string-assertion template test (line 1135–1147) to match the new dynamic import, but missed this executed test, which still assumes the router symbols arrive via context globals rather than a dynamic import.
Fix options:
- Provide an
importModuleDynamicallyoption tovm.runInNewContext(line 1184) that resolves"next/router"to a stub exposing{ default, wrapWithRouterContext, _initializePagesRouterReadyFromNextData }, or - Restructure the test so the early
if (!nextData) returnshort-circuit is exercised without evaluating theawait import("next/router")line (e.g. assert on theconsole.errorpath differently), or - Keep the router symbols as injected context globals for this slice.
Either way the executed test must be updated to match the dynamic-import entry shape, the same way the string-assertion test was.
Non-blocking notes (carried over, still applicable)
-
Header serialization still inconsistent between dev and prod.
pages-page-handler.ts:684usessafeJsonStringify(query)for thex-vinext-resolved-queryheader whiledev-server.ts:1465uses plainJSON.stringify(query). Functionally equivalent (both parsed viaJSON.parse), butsafeJsonStringifyis meant for inline-script HTML contexts, not header values. Prefer plainJSON.stringifyin both for parity. -
initialResolvedQueryis serialized into__NEXT_DATA__for every Pages page, even pages with no rewrites/middleware where it duplicatesquery. Consider gating it onhasRewrites || hasMiddleware. -
Config rewrites over-record rewrite query keys vs middleware rewrites (no inherited URL passed to
recordRewriteQueryKeys), so a destination key whose value equals a user search param gets hidden pre-hydration. Final value is identical; may cause one extra pre-ready render. Minor parity nit.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2218. SummaryI re-reviewed PR #2218 at the current HEAD ( Good news: The prior blocking issue is resolved. New blocking issue found: That same commit introduces a failing test. I verified the failure directly (and confirmed the previously-failing shims tests now pass), and provided concrete fix options in the review. The three non-blocking notes (header serialization parity, unconditional |
|
/bigbonk review for issues |
|
error: Your local changes to the following files would be overwritten by checkout: |
|
/bigbonk review for issues |
|
error: Your local changes to the following files would be overwritten by checkout: |
|
/bigbonk review for issues |
|
error: Your local changes to the following files would be overwritten by checkout: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: prerender rewrite query parity (#2218) — re-review @ 251281a
Re-reviewed at the current HEAD (251281a), which is several commits past the previously-reviewed fffb8fc. Both prior blocking issues are resolved.
Prior blockers — fixed
-
stampInitialHistoryStateforeign-state merge —syncInitialHistoryStateFromNextDatanow merges{ ...existingState, ...initialState }(shims/router.ts:828) while preserving an existing history key. The renamed test (pages-router-i18n-sticky-locale.test.ts→ "install preserves pre-existing history fields while stamping router state") assertsforeign: truesurvives alongside__N/options.locale, and passes. -
ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSINGinentry-templates.test.ts— fixed by reverting the client entry fromawait import("next/router")back to a staticimport Router, { ... } from "next/router"(entries/pages-client-entry.ts:79), then introducing_syncInitialPagesRouterStateFromNextData()which is called explicitly after__NEXT_DATA__is populated (pages-client-entry.ts:141). This decouples the locale-dependent stamp from module-eval timing without needing a dynamic import. The fulltests/entry-templates.test.tssuite (33 tests) now passes.
I verified the affected suites locally at 251281a:
tests/entry-templates.test.ts— 33 passedtests/shims.test.ts— 1147 passedtests/query.test.ts— 25 passedtests/pages-router.test.ts— 318 passedtests/pages-router-i18n-sticky-locale.test.ts,tests/pages-request-pipeline.test.ts— passingvp checkon the five changed source files — clean (format, lint, types)
Verified correctness highlights
- Prototype-pollution-safe query handling.
setOwnQueryValueusesObject.defineProperty(utils/query.ts:14), so a malicious__proto__/constructorkey in thex-vinext-resolved-queryheader orinitialResolvedQuerycannot pollute the prototype. The header parse path (shims/router.ts:1942-1951) also type-checks each value (string or string[]) before assigning. - Header-spoof guard present in both server paths. User-supplied
x-vinext-resolved-queryis stripped before the trusted value is re-set in both prod (pages-page-handler.ts:679-684) and dev (dev-server.ts:1460-1465). - Dev/prod parity.
buildInitialPagesRouterQueryproduces the pre-readyinitialQueryidentically in prod (pages-page-handler.ts:427) and dev (dev-server.ts:585).rewriteQueryKeysflows to the prod page handler via therenderPagecallback (pages-request-pipeline.ts:664/691/699) and to dev viaindex.ts:4290— both wired.
Non-blocking notes (carried over, still applicable)
-
Header serialization inconsistent between dev and prod.
pages-page-handler.ts:684usessafeJsonStringify(query)for thex-vinext-resolved-queryheader whiledev-server.ts:1465uses plainJSON.stringify(query). Functionally equivalent (client parses both withJSON.parse), butsafeJsonStringifyis intended for inline-script HTML contexts, not header values. Prefer plainJSON.stringifyin both for parity/clarity. -
initialResolvedQueryserialized into__NEXT_DATA__.__vinextfor every Pages page (pages-page-handler.ts:519,dev-server.ts:1594), even pages with no rewrites/middleware where it duplicatesquery. Consider gating onhasRewrites || hasMiddlewareto trim the common-path payload. -
Config rewrites over-record rewrite query keys. Middleware rewrites pass the inherited URL to exclude unchanged keys (
recordRewriteQueryKeys(result.rewriteUrl, originalResolvedUrl),pages-request-pipeline.ts:413), but config rewrites callrecordRewriteQueryKeys(rewritten)with no inherited URL (:488,:568,:617,:684,:737). A destination key whose value equals a user search param gets hidden pre-hydration. Final value is identical, so no user-visible value difference, but it may cause one extra pre-ready render with the key absent. Minor parity nit. -
Redundant
extractRouteParamsFromPathcall innavigationRequiresServerQueryOwnership— computed atrouter.ts:1251and recomputed at:1257on the navigation hot path. Compute once and reuse.
Verdict
No blocking issues. The change is well-tested and consistent across dev/prod/worker. The four notes above are optional polish.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2218. SummaryI re-reviewed PR #2218 at the current HEAD ( Prior blockers — fixed:
Verified locally at Verified correctness: prototype-pollution-safe query handling ( No blocking issues found. Carried over four non-blocking notes (header serialization inconsistency, unconditional |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: prerender rewrite query parity (#2218) — re-review @ 180cd48
Re-reviewed at the current HEAD (180cd48), two commits past the previously-reviewed 251281a. Both new commits (251281a, 180cd48) are lockfile-only cleanups ("remove unrelated lockfile importer") — no source changes. Both prior blocking issues remain resolved, and no new issues were introduced.
Prior blockers — still fixed
stampInitialHistoryStateforeign-state merge —syncInitialHistoryStateFromNextDatamerges{ ...existingState, ...initialState }and preserves an existing history key (shims/router.ts:820-832). Foreign fields survive alongside__N/ query ownership.ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSINGinentry-templates.test.ts— the client entry uses a staticimportofnext/routerplus an explicit post-__NEXT_DATA__sync, so the executed-slice test no longer hits a dynamic import invm.runInNewContext.
Verified locally at 180cd48
tests/entry-templates.test.ts— 33 passedtests/shims.test.ts— 1147 passedtests/query.test.ts— 25 passedtests/pages-router.test.ts,tests/pages-request-pipeline.test.ts,tests/pages-router-i18n-sticky-locale.test.ts— 386 passed combinedvp checkon the five changed source files — clean (format, lint, types)
Verified correctness highlights
- Prototype-pollution-safe query handling.
setOwnQueryValueusesObject.defineProperty(utils/query.ts:14-20), so a malicious__proto__/constructorkey cannot pollute the prototype. The header parse path (shims/router.ts:1942-1951) type-checks each value (string or string[]) before assigning. - Header-spoof guard present in both server paths. User-supplied
x-vinext-resolved-queryis stripped before the trusted value is re-set in both prod (pages-page-handler.ts:679-684) and dev (dev-server.ts:1460-1465). - Dev/prod parity.
buildInitialPagesRouterQueryproduces the pre-readyinitialQueryidentically in prod (pages-page-handler.ts:427) and dev (dev-server.ts:585).rewriteQueryKeysflows through the sharedpages-request-pipeline.ts(:636-641,:753-756) to the prod handler and viaindex.ts:4290, keeping prod/worker/dev consistent.
Non-blocking notes (carried over, still applicable)
-
Header serialization inconsistent between dev and prod.
pages-page-handler.ts:684usessafeJsonStringify(query)for thex-vinext-resolved-queryheader whiledev-server.ts:1465uses plainJSON.stringify(query). Functionally equivalent (client parses both withJSON.parse), butsafeJsonStringifyis intended for inline-script HTML contexts, not header values. Prefer plainJSON.stringifyin both for parity/clarity. -
initialResolvedQueryserialized into__NEXT_DATA__.__vinextfor every Pages page (pages-page-handler.ts:519,dev-server.ts:1594), even pages with no rewrites/middleware where it duplicatesquery. Consider gating onhasRewrites || hasMiddlewareto trim the common-path payload. -
Config rewrites over-record rewrite query keys. Middleware rewrites pass the inherited URL to exclude unchanged keys (
recordRewriteQueryKeys(result.rewriteUrl, originalResolvedUrl),pages-request-pipeline.ts:413), but config rewrites callrecordRewriteQueryKeys(rewritten)with no inherited URL (:488,:568,:617,:684,:737). A destination key whose value equals a user search param gets hidden pre-hydration. Final value is identical (no user-visible value difference), but it may cause one extra pre-ready render with the key absent. Minor parity nit. -
Redundant
extractRouteParamsFromPathcall innavigationRequiresServerQueryOwnership— computed atshims/router.ts:1251and recomputed at:1257on the navigation hot path. Compute once and reuse.
Verdict
No blocking issues. The change is well-tested and consistent across dev/prod/worker. The four notes above are optional polish.
|
Review posted to PR #2218. SummaryRe-reviewed at HEAD Key findings:
Verdict: Well-tested, consistent across dev/prod/worker. Ready to merge; the four notes are optional polish. |
|
Worker D note for deploy-suite run 28478866791 / job 84413308650: I think this PR covers the non-dynamic SSR query leak failure (). Skipping that item and continuing with the remaining uncovered Pages prerender/GSSP/revalidation failures. |
|
Correction to my previous Worker D note: for deploy-suite run 28478866791 / job 84413308650, I think this PR covers the |
|
Pages functional remaining pass for deploy-suite run 28478866791 / job 84413308650: the TSV still includes |
|
Run 28938793088 (Next.js |
|
Backlog closure for Actions run 29871622126 / job 88775791401: this PR no longer retains a unique functional failure from that run. The static The refreshed #2454 head I am treating #2218 as superseded for this failure backlog only; its broader prerender-readiness and query-history parity cases were not separate failures in run 29871622126. |
1 participant
Summary
Next.js parity references
packages/next/src/shared/lib/router/router.ts— initialisReadycalculation and hydration query update behaviortest/e2e/prerender.test.ts— prerendered rewrite query/params behaviortest/e2e/middleware-rewrites/test/index.test.ts— rewritten Pages Router readiness and query behaviorTargeted local validation
vp test run tests/query.test.ts— 25 passedtests/pages-router.test.tsprerender/rewrite assertionstests/shims.test.tsPages Router readiness, query ownership, navigation, hash, and history assertionstests/e2e/pages-router/prerender-rewrite-query.spec.tsassertions with one Playwright workertests/e2e/pages-router-prod/prerender-rewrite-query.spec.tsassertions with one Playwright workerNo full local suite or raw Next.js E2E was run.