fix(form): soft-navigate Pages Router submissions - #2029
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(form): soft-navigate Pages Router submissions
I reviewed the diff against main, traced the request lifecycle through app-rsc-handler.ts → app-pages-bridge.ts → pages-page-handler.ts, checked dev/prod parity, and ran the targeted unit + production-server suites. Everything passes and the change is well-scoped and well-tested. No blocking issues found.
Verified behavior
- Pre-middleware normalization is correct.
/_next/data/<buildId>/<page>.jsonis detected and normalized before middleware runs; middleware sees the basePath-prefixed page path (/docs/form-search), andcfis preserved across the request clones. Covered by the new "normalizes hybrid Pages data requests before middleware" test. x-nextjs-rewriteparity. The emitted rewrite target is the basePath-strippedresolvedUrl, matching the existing pure Pages Router pipeline (pages-request-pipeline.tsrefreshDataRewriteHeader). The client'sresolvePagesDataNavigationTargetrunsstripBasePathon it, so the values line up. Comparing againstoriginalResolvedUrl(captured before any rewrite) correctly covers both middleware and config rewrites.x-nextjs-redirectsoft-redirect protocol is emitted for config redirects (with basePath, e.g./docs/about) and middleware redirects (threaded viaisDataRequestintoapplyAppMiddleware). Both paths are tested.- Stale buildId → JSON 404 before middleware, app-route-owned data URL → JSON 404, and out-of-basePath data URLs left untouched — all covered.
- App-only apps (no pages dir) are unaffected —
pagesDataNormalizationis gated onoptions.renderPagesFallback, so the new branch never fires andisDataRequestfalls back to thex-nextjs-dataheader as before. cloneRequestWithHeadersrefactor in the bridge is behavior-preserving and additionally restorescf/redirect/signal metadata that the old hand-rolledRequestInitdropped.- buildId wiring —
process.env.__VINEXT_BUILD_IDis a Vitedefineapplied in both dev and prod (index.ts:1376), sobuildId: process.env.__VINEXT_BUILD_ID ?? nullresolves to the real id in the generated entry.
Non-blocking observations
-
Latent double-consume of the request body (not currently reachable). In
createAppRscHandler, whenpagesDataNormalization.isDataReqis true,pagesDataCandidateis used as the clone source for bothappRequest(cloneRequestWithUrl, app-rsc-handler.ts:1118) andpagesDataRequest(cloneRequestWithHeaders, :1122). On undici/Workers,new Request(src, ...)locks/transfers the source body stream, so a body-carrying request would leave the second clone with a disturbed body. This is harmless today because_next/datanavigations are always GET (thenext/formPOST case intentionally stays a native browser submit), but it's a sharp edge if_next/dataever carries a body. Cloning both fromrawRequest(or teeing) would make it robust. -
Style nit — duplicated
renderPagecall. Inapp-pages-bridge.ts:161-176theisDataRequestternary duplicates the entire five-argumentrenderPage(...)call just to append the 6th{ isDataReq: true }arg. Could collapse to a single call with a conditional final argument (e.g.renderPage(renderRequest, pagesUrl, {}, undefined, headers, isDataRequest ? { isDataReq: true } : undefined)) for readability.
Neither observation blocks merge. Nice work on the parity coverage and the Next.js-sourced e2e port.
|
Review posted to PR #2029. SummaryI reviewed PR #2029 (fix(form): soft-navigate Pages Router submissions) by tracing the full request lifecycle through Verdict: No blocking issues. The change is well-scoped and well-tested. Key things I verified:
Two non-blocking observations flagged in the review:
|
Summary
next/formsubmissions with Pages Router soft navigationformAction, client actions,replace, redirects, rewrites, basePath, history, and scroll behaviorNext.js parity
Fixes the failing Pages Router matrix in
test/e2e/next-form/default/pages-dir.test.ts.Validation
Cache Components, PPR, and resume behavior are explicitly out of scope.