fix(404): align default not-found copy with Next.js#1567
Conversation
The Pages Router previously emitted `<h1>404 - Page not found</h1>` and the App Router emitted plain-text `This page could not be found` (no period) when no user-defined 404 boundary was present. Next.js renders `This page could not be found.` (with trailing period) for both routers. Closes #1454.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good change — the scope is well-chosen, the test coverage is solid, and the new code follows established patterns (mirrors the DEFAULT_GLOBAL_ERROR_MODULE approach cleanly). Two real issues to fix, both minor.
tl;dr: Fix charSet → charset in the raw HTML template and drop the stray line-height:49px on the Pages Router h1 to match Next.js's _error.tsx exactly. Everything else looks correct.
|
|
||
| const CSS = `body{color:#000;background:#fff;margin:0}.next-error-h1{border-right:1px solid rgba(0,0,0,.3)}@media (prefers-color-scheme:dark){body{color:#fff;background:#000}.next-error-h1{border-right:1px solid rgba(255,255,255,.3)}}`; | ||
|
|
||
| const HTML = `<!DOCTYPE html><html><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><title>${STATUS}: ${MESSAGE}</title><meta name="next-head-count" content="2"/><style data-next-hide-fouc="true">body{display:none}</style><noscript data-next-hide-fouc="true"><style>body{display:block}</style></noscript></head><body><div id="__next"><div style="font-family:system-ui,"Segoe UI",Roboto,Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji";height:100vh;text-align:center;display:flex;flex-direction:column;align-items:center;justify-content:center"><div style="line-height:48px"><style>${CSS}</style><h1 class="next-error-h1" style="display:inline-block;margin:0 20px 0 0;padding-right:23px;font-size:24px;font-weight:500;vertical-align:top;line-height:49px">${STATUS}</h1><div style="display:inline-block"><h2 style="font-size:14px;font-weight:400;line-height:28px">${MESSAGE}</h2></div></div></div></div></body></html>`; |
There was a problem hiding this comment.
Bug: charSet should be charset in raw HTML.
charSet is the React JSX prop name (camelCase), which React transforms to charset when emitting HTML. But this is a hand-authored HTML string literal, not JSX — the browser receives it as-is. The HTML spec attribute is charset (all lowercase).
Browsers are forgiving here (they handle it case-insensitively), so this won't cause user-visible breakage, but it's technically malformed HTML. Since the goal is to match Next.js's _error.tsx output exactly, it should be charset.
Bug: stray line-height:49px on h1 — not present in Next.js's Pages Router _error.tsx.
Next.js's pages/_error.tsx does NOT set lineHeight on the h1 style object — the h1 inherits line-height: 48px from the parent desc div. The 49px value comes from the App Router access-error-styles.ts, not the Pages Router.
For reference, Next.js's _error.tsx styles.h1 is:
h1: {
display: 'inline-block',
margin: '0 20px 0 0',
paddingRight: 23,
fontSize: 24,
fontWeight: 500,
verticalAlign: 'top',
// NO lineHeight here
}Fix both on this line: charSet → charset, and remove line-height:49px from the h1 inline style.
| // When the app does not define `app/not-found.tsx` (and has not opted into | ||
| // `app/global-not-found.tsx`), fall back to vinext's built-in default | ||
| // not-found component so route-miss 404s render the canonical Next.js | ||
| // markup (status + "This page could not be found." message). Matches the |
There was a problem hiding this comment.
Clean. Mirrors the effectiveGlobalErrorModule pattern perfectly. The as unknown as TModule cast is unavoidable given the generic signature — same trade-off as the global error fallback.
| } | ||
| } | ||
|
|
||
| // No custom error page found — use plain text fallback |
There was a problem hiding this comment.
Nit (no action needed): The Response → Node.js ServerResponse bridge here works but is a little ceremonial. If you ever touch this again, buildDefaultPagesNotFoundResponse() could also expose the raw HTML + status directly (or the dev server could use DEFAULT_PAGES_NOT_FOUND_HTML with a manual writeHead). Not worth changing now — just noting it.
| lineHeight: "49px", | ||
| }, | ||
| h2: { | ||
| fontSize: 14, |
There was a problem hiding this comment.
App Router component looks correct — styles match Next.js's access-error-styles.ts (h1.lineHeight: "49px", h2.lineHeight: "49px", padding: "0 23px 0 0"). Note this intentionally differs from the Pages Router template (which should use 48px / no h1 lineHeight per _error.tsx). Two different Next.js upstream sources, two different vinext implementations — correct.
| }); | ||
|
|
||
| // Mirrors Next.js 16 experimental.globalNotFound behavior. | ||
| // Ported from Next.js: test/e2e/app-dir/global-not-found/{basic,both-present,not-present}. |
There was a problem hiding this comment.
Nice behavioral change. This test previously asserted null (renderer gave up), and now it correctly asserts the built-in default renders. Callers of renderNotFound no longer need their own fallback path for the "no not-found module" case — a simplification.
|
Review posted successfully. Here's a summary of the review: PR #1567 Review Summary The PR cleanly aligns vinext's default 404 copy with Next.js for both routers. Two issues found:
Everything else is solid — the App Router component correctly matches |
- Use HTML attribute name 'charset' (not the JSX 'charSet') in the raw HTML template. - Drop stray 'line-height:49px' on the Pages Router h1 — Next.js's _error.tsx inherits 48px from the parent div.
|
/bigbonk review |
There was a problem hiding this comment.
Both issues from the previous review are fixed in the follow-up commit (9adff76):
charSet→charset:pages-default-404.ts:26now has<meta charset="utf-8"/>— correct HTML attribute name.- Stray
line-height:49pxremoved from Pages Routerh1: Theh1inline style no longer includesline-height. The App Router component (default-not-found.tsx:39) correctly retainslineHeight: "49px"since that matches the upstreamaccess-error-styles.ts.
LGTM — clean fix, good test coverage, review feedback addressed.
|
Review posted. Both issues from the previous round are fixed:
Approved the PR. |
1 participant
Closes #1454
Summary
This page could not be found.to match Next.js v16.buildDefaultPagesNotFoundResponse()helper renders the canonical Next.js_error.tsxHTML body (status + message + dark-mode theme CSS) instead of the previous<h1>404 - Page not found</h1>placeholder. Wired into the production worker entry, the dev-server fallback, andpages-page-dataso all Pages Router 404 paths agree on the same body.DEFAULT_NOT_FOUND_MODULEmirrors Next.js's packagednot-found.tsx(HTTPAccessErrorFallbackwithstatus=404/message="This page could not be found."). Threaded throughapp-fallback-rendereras theeffectiveRootNotFoundModulefallback (same pattern as the existingeffectiveGlobalErrorModule) so route-miss 404s render a real HTML page even when the app does not defineapp/not-found.tsx.Test plan
tests/pages-default-404.test.tsasserting the canonical body, content-type, and removal of the old placeholder.tests/app-fallback-renderer.test.tsfor both the new default and the user-override precedence.tests/pages-page-data.test.ts404 assertion to the canonical copy.pnpm run checkpasses (format + lint + types).pnpm test tests/pages-default-404.test.ts tests/pages-page-data.test.ts tests/app-fallback-renderer.test.ts tests/http-error-responses.test.ts tests/nextjs-compat/not-found.test.ts tests/app-rsc-handler.test.ts tests/app-router.test.ts tests/app-page-request.test.ts tests/routing.test.ts tests/pages-router.test.tspasses.Notes
notFoundResponse()helper inhttp-error-responses.tsis intentionally unchanged: it mirrors Next.js'sres.end('This page could not be found')plain-text fallback (no period) used by API-route and minimal-mode paths. Only the HTML-rendered defaults are touched.