fix(pages): preserve missing page props on errors#2568
Conversation
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 |
|
/bigbonk review for issues |
|
UnknownError: The operation timed out. |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
Readiness is blocked on exact head
PR remains draft; all other CI jobs are green. |
|
/bigbonk review for issues |
|
UnknownError: The operation timed out. |
|
/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.
Bonk review — PR #2568
Reviewed the diff against Next.js packages/next/src/pages/_error.tsx (canary). The port is faithful and the fix correctly addresses the root cause. Ran vp check on all 6 changed source files (clean) and the targeted suites (tests/shims.test.ts -t "next/error shim" → 25 passed, tests/pages-page-handler.test.ts + tests/entry-templates.test.ts → 67 passed, tests/pages-router.test.ts -t "falls back to pages/_error for route misses" → 1 passed).
Overall this is solid, well-tested work with strong Next.js parity. The findings below are minor.
1. No end-to-end prod test for the framework error page when no _error/404/500 exists (medium)
The new behavior — serving next/error as the default /_error route in production — is only unit-tested at the string level (tests/entry-templates.test.ts asserts the generated import * as ErrorPageModule from "next/error" and "/_error": () => import("next/error") lines). The existing falls back to pages/_error for route misses prod build test still uses a real pages/_error.tsx, so it does not exercise the new no-file path through a full buildPagesFixtureToOutDir + startProdServer flow.
Worth adding a prod build test that omits _error/404/500 and asserts a 404/500 renders the framework error document, so the framework-error-page path is covered end-to-end (including client hydration).
2. Framework error route has filePath: null, so no preload/<script> tag for its client chunk (low)
pages-server-entry.ts:304 builds _errorPageRoute with filePath: ${errorAssetPathJson}, which is null when no user _error file exists. Downstream:
pages-page-handler.ts:759(if (route.filePath) pageModuleIds.push(route.filePath)) skips the framework error module, socollectAssetTagsemits no preload/script for it.pages-page-handler.ts:512-514resolveClientModuleUrl(manifest, null, ...)returnsundefined.
Client hydration should still work because pages-client-entry.ts:106 registers "/_error": () => import("next/error") in the loader map, so the browser dynamically imports the framework chunk at hydration. But this differs from a user-authored _error (which gets a preload) and adds a round trip. This appears to be acceptable/intended, but the lack of an integration test (finding 1) means prod hydration of the framework error page is unverified. Please confirm hydration works in a real prod build.
3. Dev/prod divergence in how a custom _app omitting pageProps is handled (low — confirm intended)
- Prod (
pages-page-handler.ts:663-670) guards withrenderProps.pageProps !== undefinedbefore injectingstatusCode, preserving an_app.getInitialPropsresult that intentionally omitspageProps. - Dev (
dev-server.ts:2129-2153) fully replacesrenderPropswithappInitialPropswhenever_apphasgetInitialProps, and only injectsstatusCodeintoerrorPropswhen_apphas none.
Both paths preserve the "missing pageProps" case, but via different mechanisms. The no-page-props e2e passes, so this is likely fine — flagging so the parity is a deliberate choice rather than incidental.
Nits
error.tsx:41andnext-shims.d.tsboth use theerr.statusCode!non-null assertion, sogetInitialProps({ err: {} })returnsstatusCode: undefinedeven thoughErrorProps.statusCodeis typednumber. This matches Next.js exactly (same!), and the added test asserts it — no change needed, just noting the inherited unsoundness.- The two
ErrorPageContextshapes diverge intentionally: the runtime one inerror.tsx({ err?, req?, res? }) is a narrow subset of the.d.tsNextPageContext. Fine given theasNextPageContextcast in tests, but the duplicated-but-different name could confuse future readers.
Nice work on the hostname fallback (req.url → host header) — verified path-only and protocol-relative URLs throw and are safely caught, so there's no crash risk on typical Node path-only req.url.
|
Review posted to PR #2568. SummaryI reviewed PR #2568 ( Findings (all minor — no blockers):
Plus two nits (inherited |
|
Bonk completed on exact head
The dev/prod implementation difference for preserving an All 64 exact-head CI checks are green and merge state is |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Bonk review — PR #2568
Re-reviewed the exact head (4ea7045) after the production hydration test was added. I diffed shims/error.tsx against the Next.js packages/next/src/pages/_error.tsx and _app.tsx references, traced the dev/prod status-code flow, and re-ran the targeted suites. The fix is correct and the earlier "no end-to-end prod fallback test" gap is now closed by tests/e2e/pages-router-prod/default-error.browser.spec.ts, which builds a fixture with no _error/404/500, asserts 404/500 render the framework next/error document, and confirms hydration (waitForHydration) with no unexpected console errors.
Verification I ran (all green):
vp checkon the 5 changed source files + the 4 changed/added test files — cleantests/shims.test.ts -t "next/error shim"→ 25 passedtests/pages-page-handler.test.ts+tests/entry-templates.test.ts→ 67 passedtests/pages-router.test.ts -t "falls back to pages/_error for route misses"→ 1 passed
The res.statusCode = renderStatusCode set before getInitialProps (prod createPageReqRes, dev line 2118) is the right mechanism — it feeds the framework error page's getErrorInitialProps the correct status, matching Next.js where the response status is set before the error render. I confirmed the final HTTP status comes from the explicit statusCode: renderStatusCode passed to renderPagesPageResponse, not the mutated reqRes.res.statusCode, so there's no risk of the mutation leaking into the wrong response status.
Findings below are minor — no blockers.
1. Dev-path renderErrorPage change has no direct dev integration test (low)
The prod fallback path is now covered end-to-end, but the symmetric dev-server change in server/dev-server.ts:2094-2154 (loading next/error when no _error file exists via errorAssetPath ?? "next/error", plus running a custom _app.getInitialProps and preserving an envelope that omits pageProps) is only exercised indirectly. The PR description references tests/pages-router.test.ts -t "routes throwing .* through the error page once in dev", but no test matching that name exists on this branch (grep for error page once in dev / renderErrorPage finds only pages-page-handler.test.ts and pages-request-pipeline.test.ts, neither of which covers the new dev framework-error/_app.getInitialProps path). Given the repo's dev/prod parity rule (AGENTS.md), a focused dev integration test for the no-_error-file + custom-_app case would lock in the parity the prod test now guards. I traced the dev logic and believe it's correct (see below), so this is a coverage gap, not a defect.
2. Dead-ish branch in pages-server-entry.ts (nit)
ErrorPageModule is now import * as ErrorPageModule from "next/error" in the no-user-file case, so it's always a truthy module namespace. The generated _errorPageRoute = ErrorPageModule ? {...} : null ternary (pages-server-entry.ts:304) can therefore never take the null branch anymore. Harmless, but the : null fallback is now unreachable — worth simplifying or dropping a comment so a future reader doesn't assume the null path is still live.
3. Wasted ErrorComponent.getInitialProps call in dev when custom _app supplies props (nit)
In dev-server.ts, initialErrorProps/errorProps (line 2119-2128) are computed unconditionally, then discarded whenever AppComponent has getInitialProps and returns an envelope (renderProps = appInitialProps, line 2153). Correct behavior — a well-behaved _app that calls App.getInitialProps({ Component: ErrorComponent, ctx }) re-runs the error component's getInitialProps and gets statusCode from the now-set res.statusCode, matching Next.js _app.tsx's loadGetInitialProps(Component, ctx). But the eager compute is redundant on that branch; consider computing errorProps lazily only for the no-_app-getInitialProps path.
Verified, no action needed
- statusCode not dropped in dev. For the three non-
no-page-propscombinations (no_app,_appwithout gIP,_appwith gIP that returns{ pageProps }),statusCodereaches the rendered error props. Only the intentionalno-page-propsenvelope omits it, which is the documented fix. No regression. err.statusCode!unsoundness inerror.tsx:41/next-shims.d.ts(typedstatusCode: numberbut returnsundefinedforerr: {}) matches Next.js exactly and is asserted by the new test.- Hostname fallback (
req.url→Hostheader) safely try/catches path-only and protocol-relative URLs; the new test covers both the absolute-URL andHost-header branches. - Prod
renderProps.pageProps !== undefinedguard correctly preserves a custom_appenvelope withoutpagePropswhile still injectingstatusCodefor user_errorpages that didn't set it.
Solid, well-tested work with strong Next.js parity. The only thing I'd add before calling it done is finding #1 (a dev integration test to match the new prod one).
|
Review posted to PR #2568. SummaryI re-reviewed the exact head ( Verification (all green): Findings (all minor — no blockers):
Verified with no action needed: statusCode is not dropped in dev (only the intentional no-page-props envelope omits it), the Solid, well-tested work with strong Next.js parity. The only follow-up worth doing before calling it done is finding #1. |
|
Exact-head readiness remains blocked at Bonk confirms the implementation is correct, the production framework-error/hydration gap is closed, all 64 CI checks are green, and merge state is The other two findings (the now-unreachable |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Bonk review — PR #2568
Reviewed the exact head (02ce049) against Next.js packages/next/src/pages/_error.tsx (v16.3.0-canary.80), traced the dev/prod status-code and SSR-context flow, and re-ran the targeted suites. The fix is correct and the previously-flagged dev integration gap is now closed — tests/e2e/pages-router-prod/default-error-dev.browser.spec.ts exercises the no-_error-file + custom-_app.getInitialProps path end-to-end, including the concurrent-request isolation case.
Verification I ran (all green):
tests/shims.test.ts -t "next/error shim"→ 25 passedtests/pages-page-handler.test.ts+tests/entry-templates.test.ts→ 67 passed
The next/error port is faithful: statusCodes table, styles, the getInitialProps/origGetInitialProps statics, the displayName, and the client-exception <h2> branch all match upstream. The hostname divergence (req.url/Host header vs Next.js's getRequestMeta(req, 'initURL')) is a reasonable, documented substitute and only affects the client-exception (falsy-statusCode) message, which never renders server-side. The concurrency fix is well-designed: the route-miss caller (dev-server.ts:722-740) is correctly wrapped in its own runWithRequestContext because it sits before the main request's ALS scope at line 763, while the matched-route callers inherit that outer scope — so the finally { setSSRContext(null) } cleanup is always per-request-isolated. The default-error-dev concurrency test (/missing-a with a 50ms delay racing /missing-b) locks this in.
Findings below are all minor — no blockers.
1. Dev framework-error fallback tail is now effectively unreachable (low)
renderErrorPage's post-loop fallback (dev-server.ts:2375-2391) renders the hand-authored 404/500 body when no candidate matched. But the _error candidate now imports errorAssetPath ?? "next/error" (line 2127), and next/error always exposes a truthy default, so the loop reaches its return on the _error candidate for every 404/500/other status. The tail is only reachable if the next/error import itself throws (caught at line 2364). That makes it a genuine safety net rather than dead code, but the comment above it still describes it as the "no custom error page found" path, which no longer matches reality. Worth a one-line comment update so a future reader doesn't assume the default-body path is the normal no-_error behavior (it isn't — next/error is).
2. Unreachable : null branch in pages-server-entry.ts:304 (nit — still open)
Same nit Bonk flagged on the prior head and still unaddressed: ErrorPageModule is now always import * as ErrorPageModule from "next/error" (or the user file) in the generated server entry, so _errorPageRoute = ErrorPageModule ? {...} : null can never take the null branch. Harmless, but the : null fallback is dead — simplify or drop a comment.
3. Eager errorProps compute discarded on the custom-_app branch (nit — still open)
In dev-server.ts, initialErrorProps/errorProps (lines 2166-2175) are computed unconditionally, then renderProps is replaced with appInitialProps whenever _app has getInitialProps (line 2199). Correct behavior (a well-behaved _app re-runs the error component's gIP via AppTree/Component), but the eager compute is wasted on that branch. Consider computing lazily only for the no-_app-gIP path.
Verified, no action needed
- statusCode flow (prod):
createPageReqRessetsres.statusCode = renderStatusCodebeforegetInitialProps(pages-page-handler.ts:568), so the framework error'sgetErrorInitialPropsreads the correct 404/500. The final HTTP status still comes from the explicitstatusCode: renderStatusCodearg, not the mutatedres.statusCode. renderProps.pageProps !== undefinedguard correctly preserves a custom_appenvelope that omitspagePropswhile still injectingstatusCodefor user_errorpages.forceReady=trueon_initializePagesRouterReadyFromNextData(router.ts:1369, dev script line 2269) defaults tofalse, so existing callers are unaffected; the error page needs it because/_errorwon't satisfygetPagesNavigationIsReadyFromSerializedStateagainst the real URL. Matches Next.js's ready-on-error-page behavior.err.statusCode!unsoundness inerror.tsx:41matches Next.js exactly and is asserted by the newgetInitialProps({ err: {} })test.- Server/client hydration parity for the no-
pagePropsenvelope: both server render and the dev hydration script (line 2283) passpageProps: undefinedto the custom_app, so hydration is consistent.
Solid, well-tested work with strong Next.js parity. The remaining items are all cosmetic; I see no correctness blocker.
|
Review posted to PR #2568. SummaryI reviewed the exact head ( The fix is correct and the previously-flagged dev integration gap is now closed by Key things I verified:
Findings (all minor — no blockers):
No correctness blocker. Solid, well-tested work with strong Next.js parity. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Bonk review — PR #2568
Re-reviewed the new exact head (4234482), which lands two follow-up commits on top of the last-reviewed 02ce049. The delta is small and addresses the two open nits from the prior review. The fix remains correct; both previously-open nits are now resolved, and no new issues were introduced.
Verification I ran (all green):
vp check packages/vinext/src/server/dev-server.ts packages/vinext/src/entries/pages-server-entry.ts— clean (format + lint + type)vp test run tests/pages-page-handler.test.ts tests/shims.test.ts tests/entry-templates.test.ts→ 1,265 passed
What changed since 02ce049
1. pages-server-entry.ts:304 — dead : null branch removed (prior nit #2, now fixed).
_errorPageRoute is now an unconditional object literal instead of ErrorPageModule ? {...} : null. Since ErrorPageModule is always a truthy namespace import (user file or import * as ErrorPageModule from "next/error"), the ternary was unreachable. Correct simplification.
2. dev-server.ts renderProps restructure (prior nit around the discarded initializer).
renderProps is no longer eagerly initialized to { pageProps: errorProps }. It's now assigned in exactly one of two branches:
_appwithgetInitialProps:renderProps = appInitialProps ?? {}- otherwise:
renderProps = { pageProps: errorProps }
I traced the downstream consumers and confirmed parity is preserved:
- Server render (
createErrorElement, line 2223): whenFinalAppis falsy it renderscreateElement(FinalComponent, errorProps)(statusCode present); whenFinalAppexists it spreadsrenderProps. __NEXT_DATA__(line 2254) serializesrenderProps, and the hydration script (lines 2285-2286) readsprops.pagePropsfor both the_appand no-_appbranches. Server and client render the same props in all three non-no-page-propscombinations, and the intentionalno-page-propsenvelope (appInitialPropswithoutpageProps) is preserved verbatim — matching the prodrenderProps.pageProps !== undefinedguard inpages-page-handler.ts:663-670.- TS definite-assignment holds (both branches assign; early returns on
res.headersSentoccur before first use).
3. Dev fallback-tail comment updated (prior review finding #1, now fixed).
The comment at dev-server.ts:2377 now correctly describes the post-loop fallback as a defensive net for a missing/invalid framework error module, since the _error candidate always resolves next/error (which always exposes a truthy default) inside the loop. The stale "no custom error page found → default body" comment is gone.
Remaining item (nit — non-blocking, unchanged)
errorProps/initialErrorProps (dev-server.ts:2166-2175) are still computed eagerly and discarded on the custom-_app-with-gIP branch (that branch re-runs the error component's gIP via App.getInitialProps({ Component: ErrorComponent, ctx })). Behavior is correct; the eager compute is a cheap, harmless redundancy. Not worth a follow-up on its own.
Verified, no action needed (carried from prior reviews, re-confirmed on this head)
next/errorport fidelity (status table, styles,getInitialProps/origGetInitialPropsstatics, client-exception<h2>branch).- statusCode flow:
res.statusCodeset beforegetInitialProps; final HTTP status from the explicitstatusCodearg, not the mutatedres.statusCode. - Concurrency isolation (route-miss caller wrapped in its own
runWithRequestContext;finally { setSSRContext(null) }per-request-isolated) — covered by thedefault-error-devrace test. err.statusCode!unsoundness matches Next.js exactly and is asserted by tests.
No correctness blocker. The two open nits from the last pass are addressed and the change is safe. This looks ready.
|
Review posted to PR #2568. SummaryI re-reviewed the new exact head ( Verification (all green):
What changed since last review:
Remaining: one unchanged nit (eager No correctness blocker. The change is safe and looks ready. |
…ge-props-28938793088
|
/bigbonk review for issues |
There was a problem hiding this comment.
Bonk review — PR #2568
Re-reviewed the exact head (8876103, which merges main into the branch). I diffed shims/error.tsx against Next.js packages/next/src/pages/_error.tsx (v16.3.0-canary.80), re-traced the dev/prod status-code and SSR-context flow, and re-ran the targeted suites. Prior reviews focused on dev/prod render behavior; this pass turned up one real client/server parity bug in the production client entry that the earlier passes missed.
Verification I ran (all green):
vp checkon all 7 changed source files — cleanvp test run tests/pages-page-handler.test.ts tests/shims.test.ts tests/entry-templates.test.ts→ 1,265 passed
1. Custom pages/_error is ignored by the production client loader map (medium — correctness)
pages-client-entry.ts:105-107 adds a hardcoded framework loader for /_error:
if (!pageRoutes.some((route) => route.pattern === "/_error")) {
loaderEntries.push(' "/_error": () => import("next/error")');
}The guard is intended to skip the framework loader when the app ships a custom _error, but it never fires: _error is a reserved page name (RESERVED_PAGE_NAMES in routing/pages-router.ts:25) and is excluded from pageRoutes by design, so pageRoutes.some(r => r.pattern === "/_error") is always false. The next/error loader is therefore pushed unconditionally — even when the user has pages/_error.tsx.
I confirmed this empirically by generating the client entry for a fixture with a custom pages/_error.tsx:
=== /_error loader lines (custom _error present) ===
"/_error": () => import("next/error")
Impact: window.__VINEXT_PAGE_LOADERS__["/_error"] maps to the framework component. On client-side navigation that resolves to /_error, shims/router.ts:2342-2349 prefers the loader map over the serialized pageModuleUrl ("Prefer the generated route loader when it exists, even on the HTML fallback path"). So an app with a custom _error would client-navigate into the framework next/error page instead of its own — a client/server divergence (the server entry correctly uses the user's errorFilePath at pages-server-entry.ts:94-98; initial hydration also uses the correct pageModuleUrl, so only client-side nav to /_error is affected).
This is also a dev/prod parity gap: the dev path resolves the custom file correctly (dev-server.ts:2246 — errorAssetPath ? createPagesDevModuleUrl(...) : "next/error"), so dev and prod would load different /_error components for the same custom-error app.
Suggested fix: resolve the user file the same way the other entries do and only fall back to the framework component when it's absent. findFileWithExts is already imported (used for _app at line 67):
const errorFilePath = findFileWithExts(pagesDir, "_error", fileMatcher);
loaderEntries.push(
errorFilePath
? ` "/_error": () => import(${JSON.stringify(errorFilePath)})`
: ` "/_error": () => import("next/error")`,
);The existing entry-templates.test.ts case only covers the _error-absent path (clientCode contains import("next/error")). A companion assertion for the custom-_error case (loader points to the user file, not next/error) would lock this in.
2. Server/client prop shape differs for the no-pageProps envelope in dev (low)
In dev, for a custom _app that omits pageProps:
- Server render (
dev-server.ts:2223-2228):createElement(FinalApp, { ...renderProps, Component, router })→ nopagePropskey at all. - Hydration script (
dev-server.ts:2285):React.createElement(AppComponent, { ...props, Component, pageProps: props.pageProps, router })→pageProps: undefinedexplicitly present.
The default-error-dev e2e passes and reports no console errors, so React tolerates this (a missing key vs undefined read the same when the app destructures pageProps). Flagging only for symmetry — the prod path passes the same renderProps on both sides.
Verified, no action needed (re-confirmed on this head)
next/errorport fidelity:statusCodestable,styles,getInitialProps/origGetInitialPropsstatics,displayName, client-exception<h2>branch all match upstream. The hostname substitution (req.url→Hostheader vs Next.js'sgetRequestMeta(req, 'initURL')) safely try/catches path-only and protocol-relative URLs.- statusCode flow:
res.statusCodeset beforegetInitialProps; final HTTP status comes from the explicitstatusCodearg, not the mutatedres.statusCode. - Prod
renderProps.pageProps !== undefinedguard (pages-page-handler.ts:663-670) correctly preserves a custom_appenvelope withoutpagePropswhile still injectingstatusCodefor the framework/default error page — covered by the new unit test. - Concurrency isolation: the route-miss caller is wrapped in its own
runWithRequestContext(it sits before the outer request ALS scope atdev-server.ts:762), sofinally { setSSRContext(null) }is per-request-isolated. The/missing-avs/missing-brace test locks this in. err.statusCode!unsoundness matches Next.js exactly and is asserted by tests.next-shims.d.tsdeclarations match the runtime; theErrorPageContext(.d.ts, fullNextPageContextshape) vs the narrow runtime{ err?, req?, res? }divergence is intentional.
Finding #1 is a genuine correctness/parity bug for apps with a custom pages/_error.tsx and should be fixed before merge. The rest of the change is solid, well-tested work with strong Next.js parity.
|
Review posted to PR #2568. SummaryI re-reviewed the exact head ( Finding #1 (medium — correctness): In I confirmed empirically that generating the client entry for a fixture with a custom Also flagged one low-severity dev prop-shape asymmetry (missing Verification: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Bonk review — PR #2568
Re-reviewed the exact head (67d074f1, fix(pages): load custom error page on navigation), which lands one follow-up commit on top of the last-reviewed 8876103. The single medium-severity correctness bug from the prior review (finding #1 — custom pages/_error ignored by the production client loader map) is now fixed, and the follow-up introduces no new issues.
Verification (all green)
vp check packages/vinext/src/entries/pages-client-entry.ts tests/entry-templates.test.ts tests/e2e/pages-router-prod/custom-error-navigation.browser.spec.ts— clean (format + lint + type)vp test run tests/entry-templates.test.ts tests/pages-page-handler.test.ts tests/shims.test.ts→ 1,266 passed
What changed since 8876103
1. pages-client-entry.ts:106-110 — custom _error now honored (prior finding #1, fixed).
The unreachable pageRoutes.some(r => r.pattern === "/_error") guard is replaced with a real file resolution:
const errorFilePath = findFileWithExts(pagesDir, "_error", fileMatcher);
loaderEntries.push(
errorFilePath !== null
? ` "/_error": () => import(${JSON.stringify(errorFilePath)})`
: ' "/_error": () => import("next/error")',
);This is resolved identically to the server entry (pages-server-entry.ts:77), so the client loader map (window.__VINEXT_PAGE_LOADERS__["/_error"]) now points to the user file when present. Since shims/router.ts:2342-2349 prefers the loader map on client navigation, an app with a custom pages/_error.tsx now client-navigates into its own component instead of the framework next/error. This also restores dev/prod parity — the dev path (dev-server.ts:2246-2248, errorModuleSource) already resolves the custom file the same way.
I re-confirmed the fix empirically: the added entry-templates.test.ts case generates the client entry for a fixture with a custom _error.page.tsx (under pageExtensions: ["page.tsx", "tsx"]) and asserts the loader points to the user file and does not contain import("next/error").
2. Path convention is correct. findFileWithExts builds its result with path.join (pathslash in source), so the emitted import(...) specifier is a canonical forward-slash id, matching how the other pageRoutes loaders serialize r.filePath. No slash-convention divergence.
3. New e2e coverage. custom-error-navigation.browser.spec.ts builds a real prod fixture with a custom pages/_error.tsx (with getInitialProps), clicks a link to /missing, and asserts the user's data-testid="custom-error" renders with the correct statusCode (404) — locking the client-navigation path end-to-end. The fixture follows the established sibling-test pattern (default-error.browser.spec.ts).
Verified, no action needed (re-confirmed on this head)
- Server/client now resolve
_errorvia the identicalfindFileWithExts(pagesDir, "_error", fileMatcher)call — no residual divergence between the two entries. - Prod
renderProps.pageProps !== undefinedguard (pages-page-handler.ts:663-670) still preserves a custom_appenvelope withoutpagePropswhile injectingstatusCodefor the framework/default error page. - statusCode flow:
res.statusCodeset beforegetInitialProps(createPageReqRes); final HTTP status from the explicitstatusCodearg. next/errorport fidelity, concurrency isolation (route-miss caller wrapped in its ownrunWithRequestContext), anderr.statusCode!upstream-matching unsoundness — all unchanged and still correct.
Remaining (nit — non-blocking, unchanged)
errorProps/initialErrorProps in dev are still computed eagerly and discarded on the custom-_app-with-getInitialProps branch. Behavior is correct; the eager compute is a cheap, harmless redundancy. Not worth a dedicated follow-up.
No correctness blocker. The medium bug from the prior pass is resolved with matching unit + e2e coverage, and dev/prod/server/client now agree on the _error component. This looks ready.
|
Review posted to PR #2568. SummaryI re-reviewed the exact head ( Verification (all green):
What the fix does: Remaining: one unchanged nit (eager dev No correctness blocker. This looks ready. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| createValidFileMatcher(nextConfig.pageExtensions), | ||
| ); | ||
|
|
||
| expect(clientCode).toContain(`"/_error": () => import(${JSON.stringify(errorFilePath)})`); |
|
Independent Codex review — NO FINDINGS Reviewed exact head |
…ge-props-28938793088 # Conflicts: # packages/vinext/src/server/pages-page-handler.ts # packages/vinext/src/shims/next-shims.d.ts
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
Bonk review — PR #2568Re-reviewed the exact head ( Verification (all green)
Refactor fidelity — verified faithfulI traced every option branch against the pre-refactor inline scripts:
All branches match the originals. The Security improvement (worth calling out)The extraction changes module-specifier serialization from raw string interpolation ( The remaining CodeQL alert on Minor behavioral deltas on the error path (both benign)The unified helper is now used for the error path, which slightly widens two behaviors vs. the old inline error script — neither is a regression:
Verified, no action needed (carried from prior passes, re-confirmed on this head)
Remaining (nit — non-blocking, unchanged)Dev No correctness blocker. The refactor is a faithful, well-tested extraction that also hardens module-specifier escaping. This looks ready. |
|
Review posted to PR #2568. SummaryI reviewed the exact head ( The refactor is a faithful, well-tested extraction — no correctness blocker. Key findings:
Verification: |
2 participants
Summary
next/errorpage as the default Pages Router/_errorroute in production and client loader manifests_app.getInitialPropsresult that intentionally omitspagePropsinstead of synthesizing error props into it_app.getInitialPropsgetInitialPropsand client-exception rendering semanticsRoot cause
Vinext returned a hand-authored 404 document whenever an app did not define
pages/404orpages/_error. That bypassed the app's custom_app, so theno-page-propsfixture never exercised the same error render as Next.js. The production handler also unconditionally createdpageProps.statusCode, masking a custom_app.getInitialPropsresult that omittedpageProps.Next.js parity
Fixes run
28938793088against Next.jsv16.3.0-canary.80(9dd6d677b63b249584c8ff0bccffcc6a65288683):test/e2e/no-page-props/no-page-props.test.tsshould load 404 page correctlyshould navigate between pages correctlyReference:
test/e2e/no-page-props/no-page-props.test.tspackages/next/src/shared/lib/utils.ts(loadGetInitialProps)packages/next/src/pages/_error.tsxValidation
vp test run tests/pages-page-handler.test.ts tests/shims.test.ts tests/entry-templates.test.ts— 1,256 passedvp test run tests/pages-router.test.ts -t "falls back to pages/_error for route misses"— 1 passedvp test run tests/pages-router.test.ts -t "routes throwing .* through the error page once in dev|falls back to pages/_error for route misses"— 3 passedvp checkfor all 8 changed files — cleanREPO="$PWD" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref-v16.3.0-canary.80" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/no-page-props/no-page-props.test.ts— 5 passed, 0 failedBonk has not been requested yet; this is ready for independent review.