fix(pages): keep charset first in custom document heads - #2909
Conversation
commit: |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/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 charset-ordering fix. The approach is sound and matches Next.js behavior:
- Prod (
pages-page-response.ts): insertingssrHeadHTMLright after the opening<head>(via a function replacer) is the right call — using a function replacer avoids$&/$1special-pattern injection from user head HTML, and the/<head(?:\s[^>]*)?>/iregex correctly avoids matching<header>and handles attributed<head data-theme="...">. - Dev (
dev-server.ts): wrapping the collected head inSSR_HEAD_START/ENDmarkers so it survives Vite's HTML transform and then hoisting it ahead of Vite's prepended client scripts is a clean solution to the dev-vs-prod parity problem. - Test coverage is good: charset/viewport ordering is asserted on the dev path (
renders next/head tags in SSR HTML <head>runs againstcreateServer), the prod build path, and thedocument-childordering is covered in both dev and prod via theit.each(["dev", "prod"])document tests. The new unit tests inpages-page-response.test.tsdirectly cover the custom-Document-children and no-Document font-ordering cases.
A few non-blocking observations below. No blocking issues found — dev/prod parity is maintained and the ordering matches the Next.js test/e2e/next-head expectations cited in the tests.
|
Review posted successfully. Here's my summary: Review of PR #2909 —
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix. The approach is sound and dev/prod parity is well maintained. Verified:
- Prod custom-Document (
pages-page-response.ts): two-step replace —ssrHeadHTMLinserted after the opening<head>(function replacer avoids$&/$1special-pattern injection from user head HTML), then font/asset/tail tags before</head>. The/<head(?:\s[^>]*)?>/iregex correctly handles<head data-theme="...">and does not match<header>. - Dev (
dev-server.ts): wrapping the collected head inSSR_HEAD_START/ENDmarkers so it survivestransformIndexHtml, then hoisting ahead of Vite's prepended client scripts, is a clean solution. The no-op empty-headHTMLcase is harmless. - Ordering parity between the four head-injection sites (dev/prod × Document/no-Document) is consistent: charset-bearing
ssrHeadHTML/headHTMLfirst, then font, then generated assets, then tail/styles. - Test coverage is thorough: the dev path is genuinely exercised (
renders next/head tags in SSR HTML <head>runs againststartFixtureServer, i.e. dev), the prod build path is covered end-to-end, anddocument-childordering is verified in both dev and prod.
No blocking issues. Two non-blocking observations inline.
|
Review posted successfully. Here's my summary: Review of PR #2909 —
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix end-to-end. The approach is sound and dev/prod parity is well maintained. I confirm the earlier findings and have nothing blocking to add.
Verified correct:
- Prod (
pages-page-response.ts): two-step replace insertsssrHeadHTMLright after the opening<head>via a function replacer (avoids$&/$1special-pattern injection from user head HTML), then fonts/assets/tailHeadHTMLbefore</head>. The/<head(?:\s[^>]*)?>/iregex correctly matches<head data-theme="dark">and does not match<header>(after<headit requires\sor>, and<header>haser>). Since__NEXT_MAIN__is replaced with the body marker before this runs, the only<head>in the string is the document's, so no risk of matching a<head>in rendered body content. - Dev (
dev-server.ts): wrapping the collected head inSSR_HEAD_START/ENDcomment markers so it survivestransformIndexHtml, then hoisting the block ahead of Vite's prepended dev client scripts and custom Document children, is a clean solution. Vite injects its HMR client near the top of<head>(before the START marker, which sits at the old</head>position), so the move correctly places the collected head first. head.tsshim: splitting the single[children]effect into a mount/unmount cleanup ([]) plus a set-only update effect ([children]) is the right fix — the old combined effect ran cleanup on every children change, which removed and re-appended even unchanged tags to the end of<head>on every Fast Refresh. The full-projection reconcile in_syncClientHeadmeans dropping cleanup from the update path is safe.- Test coverage: genuinely exercises dev (
renders next/head tags in SSR HTML <head>runs against a dev server), the prod build path end-to-end, custom-Document-children ordering in both dev and prod, and the new e2eafter-documentHMR assertion directly covers the appended-changed-tag behavior of the shim change.
Two non-blocking observations below (both previously noted).
|
Review posted successfully to PR #2909. SummaryVerdict: No blocking issues. The fix is well-designed and dev/prod parity is maintained. I independently verified the correctness of all three moving parts: What the PR does correctly:
Two non-blocking observations posted inline (both consistent with prior bonk reviews):
|
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix. The core approach is sound and I confirm the previously-noted points: the prod function-replacer insert after <head> avoids $&/$1 injection, the /<head(?:\s[^>]*)?>/i regex won't match <header> and handles attributed heads, and — importantly — in both dev and prod the head regex runs after __NEXT_MAIN__ is replaced with the body/stream marker, so the body content isn't in the string yet and there's no risk of matching a <head> inside rendered body. The head.ts effect split (mount-only cleanup + set-only update) is the right fix for the Fast Refresh re-append behavior.
One dev/prod parity divergence worth calling out that the prior reviews missed, plus the pre-existing malformed-Document edge case. Both non-blocking.
|
Review posted successfully to PR #2909. SummaryVerdict: No blocking issues. The charset-ordering fix is well-designed and I verified the correctness of all three moving parts. Verified correct:
One new finding the prior bonk reviews missed (posted inline, non-blocking):
Plus the pre-existing edge case ( |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix end-to-end. The approach is sound and I ran the targeted suites locally — tests/pages-page-response.test.ts (56 passed), tests/head.test.ts (53 passed), and both new pages-router.test.ts cases (renders next/head tags in SSR HTML <head>, places client trace metadata after custom Document children in dev and prod) all pass.
Verified correct:
- Prod (
pages-page-response.ts) — Two-step replace:ssrHeadHTMLinserted after the opening<head>via a function replacer (safe against$&/$1special-pattern injection from user head HTML), then fonts/assets/tailHeadHTMLbefore</head>. The/<head(?:\s[^>]*)?>/iregex handles<head data-theme="dark">and won't match<header>. Because__NEXT_MAIN__→ body marker runs at line 335 before the head regex, no rendered body content is in the string — no risk of matching a stray<head>in body. - Dev (
dev-server.ts) — TheSSR_HEAD_START/ENDmarker + hoist approach cleanly survivestransformIndexHtmland reorders the collected head ahead of Vite's prepended dev scripts and custom Document children. The head regex runs on the pre-split shell (still holdingSTREAM_BODY_MARKER), so no body content is present. head.tsshim — Splitting the combined[children]effect into a mount-only cleanup ([]) + set-only update ([children]) is the right fix: the old combined effect ran cleanup on every render, re-appending even unchanged tags to the end of<head>on every Fast Refresh. The fullisEqualNodereconcile in_syncClientHeadmakes dropping cleanup from the update path safe.- dev/prod trace-metadata parity — This PR actually fixes the divergence flagged in earlier reviews: dev now routes trace HTML through
getTailHeadHTML, so trace<meta>lands at the head tail in both dev and prod. The new dev+prod parity test locks this in. 👍
No blocking issues. Two non-blocking observations below.
|
Review posted successfully to PR #2909. SummaryVerdict: No blocking issues. The charset-ordering fix is well-designed, and I verified all four moving parts and ran the targeted suites locally (all pass). Verified correct:
Two non-blocking observations posted inline:
Tests run locally: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix end-to-end. The approach is sound, dev/prod parity is well maintained, and I ran tests/head.test.ts (57) and tests/pages-page-response.test.ts (56) locally — all pass.
Verified correct:
- Prod (
pages-page-response.ts) — Function-replacer insert after the opening<head>avoids$&/$1special-pattern injection from user head HTML;/<head(?:\s[^>]*)?>/ihandles<head data-theme="dark">and won't match<header>.__NEXT_MAIN__→bodyMarkerruns before the head regex, so no rendered body content is in scope. - Dev (
dev-server.ts) — The per-response-UUIDSSR_HEAD_START/ENDmarkers + hoist survivetransformIndexHtmland reorder the collected head ahead of Vite's prepended dev scripts. Using a UUID rather than a fixed sentinel is the right call, and the newpreserves inline Head content containing the old fixed dev markertest locks that in. head.tsshim — Splitting the combined effect into mount-only cleanup ([]) + set-only update (no-deps) correctly fixes the Fast Refresh re-append bug;isEqualHeadNode+document.titleownership faithfully porthead-manager.ts.
No blocking issues. One divergence from the Next.js reference worth noting inline, plus confirmation of the pre-existing malformed-Document edge case already raised in prior reviews.
|
Review posted successfully to PR #2909. SummaryVerdict: No blocking issues. The charset-ordering fix is well-designed and dev/prod parity is maintained. I read all four modified source files, cross-referenced Next.js's Verified correct:
Three non-blocking observations posted inline:
All are non-blocking; the previously flagged trace-metadata dev/prod parity concern is actually fixed by this PR (dev now routes trace HTML through |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the charset-ordering fix end-to-end. I verified all four moving parts against the Next.js head-manager.ts source and ran the targeted suites locally — tests/head.test.ts (57) and tests/pages-page-response.test.ts (56) both pass, and vp check on the three source files is clean.
Verified correct:
- Prod (
pages-page-response.ts) — Function-replacer insert after the opening<head>avoids$&/$1special-pattern injection from user head HTML;/<head(?:\s[^>]*)?>/ihandles<head data-theme="...">and won't match<header>. Crucially, the head regex runs whilehtmlstill holds thebodyMarkerplaceholder — the real body stream is spliced in later atmarkerIndex(line 636) — so there is no risk of matching a stray<head>inside rendered body content. - Dev (
dev-server.ts) — Per-response-UUIDSSR_HEAD_START/ENDcomment markers survivetransformIndexHtml, and the hoist correctly reorders the collected head ahead of Vite's prepended dev scripts and custom Document children. The UUID (vs a fixed sentinel) is the right call and the newpreserves inline Head content containing the old fixed dev markertest locks it in. Trace-metadata dev/prod parity is now aligned — dev routes trace HTML throughgetTailHeadHTML, so it lands at the head tail in both modes, matching Next.js. The error-page manual-default-Document branch also gets trace metadata, andclientTraceMetadatais plumbed througherrorPageContextto everyrenderErrorPagecall. head.tsshim — Splitting the combined effect into mount-only cleanup ([]) + set-only update (no deps) correctly fixes the Fast Refresh re-append bug;isEqualHeadNode, thedocument.titleownership, and the[meta, base, link, style, script]type-ordering faithfully porthead-manager.ts(confirmed against canary source). Thedelete+setin_updateClientHeadInstancereproduces Next.js's last-writer-wins dedupe via Map iteration order.
No blocking issues. Two small deliberate divergences from Next.js and one pre-existing edge case, all non-blocking, noted inline.
|
Review posted successfully to PR #2909. SummaryVerdict: No blocking issues. The charset-ordering fix is well-designed and dev/prod parity is maintained. I verified all four moving parts against the Next.js Verified correct:
Two non-blocking observations posted inline:
These corroborate the prior bonk reviews. The fix is sound and ready. |
2 participants
Summary
next/headblock immediately after the opening<head>tag in production Pages responses, ahead of custom Document childrenThis fixes the remaining failure reported after #1677: custom Document children and development scripts could still appear before
<meta charset="utf-8">even though the head collector returned the tags in the correct order.Testing
pnpm exec vp check packages/vinext/src/server/dev-server.ts packages/vinext/src/server/pages-page-response.ts tests/pages-page-response.test.ts tests/pages-router.test.tspnpm test tests/head.test.ts tests/pages-page-response.test.tspnpm test tests/pages-router.test.ts -t "renders next/head tags in SSR HTML <head>"pnpm test tests/pages-router.test.ts -t "serves pages from production build end-to-end"Closes #1569