Skip to content

fix(pages): preserve styled-jsx CSS ordering in SSR - #2894

Open
james-elicx wants to merge 9 commits into
mainfrom
codex/fix-streaming-ssr-styled-jsx-31439707085
Open

fix(pages): preserve styled-jsx CSS ordering in SSR#2894
james-elicx wants to merge 9 commits into
mainfrom
codex/fix-streaming-ssr-styled-jsx-31439707085

Conversation

@james-elicx

@james-elicx james-elicx commented Aug 11, 2026

Copy link
Copy Markdown
Member

Summary

  • buffer Pages Router HTML through React allReady in dev and production, matching Next.js and preventing later global CSS resources from overriding styled-jsx rules
  • collect and flush each request-local styled-jsx registry once after Suspense settles, then emit its deduplicated rules after React body stylesheet resources
  • keep ISR regeneration ordering consistent with fresh renders while preserving style-looking literals in every HTML raw-text container
  • remove stale getServerSideProps Content-Length values before Worker response merging
  • retain Pages data exports through the styled-jsx transform and provide the portable ESM styled-jsx runtime needed by generated and relocated static-export entries

Pages Router behavior change

Pages Router Suspense HTML is intentionally no longer exposed progressively: body bytes wait for renderStream.allReady. This trades Pages Router TTFB and progressive reveal for deterministic CSS ordering and Next.js parity. App Router streaming is unchanged, and Pages HEAD requests retain their headers-only fast path.

Validation

  • vp check on all changed source and test files
  • focused Pages response and data tests: 139/139 passed
  • production Pages response integration block: 10/10 passed
  • focused header merge and response-close tests: 11/11 passed
  • targeted Next.js test/e2e/streaming-ssr/index.test.ts: 5/5 passed
  • targeted Next.js test/e2e/app-dir/scss/with-styled-jsx/with-styled-jsx.test.ts: 2/2 passed across both Sass variants
  • targeted Next.js scripts rebuilt all vinext packages successfully before the upstream tests

Review

Two independent cumulative reviews returned no findings after the raw-text, Worker header, duplicate styled-jsx registry, behavior-contract, and dead HTML streaming-marker findings were addressed. The PR description was also replaced and verified from a literal body file so the documented commands and behavior remain intact.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Aug 11, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2894
npm i https://pkg.pr.new/create-vinext-app@2894
npm i https://pkg.pr.new/@vinext/types@2894
npm i https://pkg.pr.new/vinext@2894

commit: c423475

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

Comment thread packages/vinext/src/server/pages-page-data.ts Fixed
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared c423475 against base d558d62 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 135.2 KB 135.2 KB ⚫ +0.0%
Client entry size (gzip) vinext 122.7 KB 122.7 KB ⚫ 0.0%
Dev server cold start vinext 3.11 s 3.08 s ⚫ -1.1%
Production build time vinext 3.38 s 3.39 s ⚫ +0.3%
RSC entry closure size (gzip) vinext 114.2 KB 114.2 KB ⚫ -0.0%
Server bundle size (gzip) vinext 193.0 KB 193.0 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): preserve styled-jsx during streaming SSR

Reviewed the full diff plus dev/prod parity across the four request-handling paths. This is a solid, well-tested change. Verified locally: styled-jsx, pages-page-response, pages-page-data tests pass (13 + 137), the two new pages-router streaming/ISR tests pass, and vp check is clean on all six changed source files. No blocking issues found. A few notes below.

Verified correct

  • ESM conversion regexes match the real runtime. Ran convertStyledJsxRuntimeToEsm's three replacements against styled-jsx@5.1.6/dist/index/index.js — all three match, and React__default["default"] still works after swapping var React = require(...) for import * as React. The fail-loud throw when any replacement is a no-op is a reasonable guard against a future runtime layout change.
  • getRuntimeDistPath() path is correct. require.resolve("styled-jsx") resolves to .../styled-jsx/index.js, so dirname(...) + dist/index/index.js lands on the real file. Coupling to the internal dist layout is acceptable given the wrapper-shape guard.
  • Double-collect + flush is not duplicative. Initial collectPagesStyleRegistryHtml reads shell styles and flushes; the bodyAllReady continuation collects only post-flush (Suspense) styles. No double-emit.
  • disableNextSsg: true is legitimate (next/dist/build/swc/options.d.ts:27) and the new test asserts getStaticProps/getServerSideProps survive without __N_SSG/__N_SSP. Good catch — this was silently stripping Pages data exports.
  • ISR tee ordering holds. bodyStream.allReady is captured before tee(); both branches are drained (writePagesIsrCache reads the cache branch, composite stream reads the response branch), so finalShellSuffix resolves and late styles land in the cached HTML.
  • renderErrorPage refactor is behavior-preserving. The removed non-Document branch is subsumed by streamPageToResponse's default-shell path (dev-server.ts:513-529), and preserveExistingContentType: statusCode === 404 mirrors the old conditional Content-Type. Bonus: error pages now get styled-jsx too.

Non-blocking observations

  1. Dev/prod parity in how styled-jsx is loaded is structurally different. The prod entry (pages-server-entry.ts:212) statically imports styled-jsx and will hard-fail if it can't resolve, while dev (dev-server.ts:365) wraps the import in try/catch and treats it as optional. In practice this is fine because styled-jsx@5.1.6 is a hard dependency of next and vinext always requires next, so the module is always resolvable. But the two paths encode different assumptions about optionality. Worth a one-line comment on the prod static import noting why it's safe to import unconditionally (i.e., "always present via next's deps"), so a future reader doesn't "fix" the asymmetry by making prod also optional (which would silently drop styles).

  2. ISR regeneration relocates styles from before-</body> to <head>. On the initial streaming write, late/Suspense styles are injected before </body>; on regeneration, STYLED_JSX_STYLE_PATTERN strips all __jsx- styles (head and body) and refreshCachedHeadTags re-inserts the freshly collected combined run into <head> (regen uses the non-streaming renderToStringAsync, so there's no late split). Both placements are valid and the custom-document ISR test passes, so this is correct — just noting the placement is intentionally not identical between first render and regen.

  3. STYLED_JSX_STYLE_PATTERN lookahead assumes id appears before any >. <style\b(?=[^>]*\bid=["']__jsx-) relies on styled-jsx emitting id as the first attribute, which mapRulesToStyle does. Fine for the pinned runtime; just flagging it's coupled to attribute ordering.

Nothing here blocks merge. Items 1-2 are optional clarifying comments for future maintainers.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2894. Summary of my findings:

Review Summary

The PR is well-implemented and thoroughly tested. No blocking issues found. I verified locally that all relevant tests pass (13 styled-jsx, 137 pages-response/data, 5 pages-router streaming/ISR) and vp check is clean on all changed source files.

Verified correct

  • The convertStyledJsxRuntimeToEsm regexes match the real styled-jsx@5.1.6 runtime; the fail-loud guard against a future layout change is sound.
  • getRuntimeDistPath() correctly resolves .../styled-jsx/dist/index/index.js.
  • The double-collect + flush pattern (shell styles then post-flush Suspense styles) does not duplicate output.
  • disableNextSsg: true is a legitimate SWC option that fixes silent stripping of getStaticProps/getServerSideProps — a real bug the PR correctly addresses.
  • The ISR tee() ordering holds (allReady captured before tee; both branches drained).
  • The renderErrorPage refactor is behavior-preserving and additionally brings styled-jsx to error pages.

Non-blocking observations (optional clarifying comments)

  1. Structural dev/prod parity difference: prod statically imports styled-jsx (hard-fail), dev treats it as optional (try/catch). Safe in practice since styled-jsx is always a next dependency, but worth a comment explaining why the prod static import is safe.
  2. ISR regen relocates styles from before-</body> (initial streaming write) to <head> (regeneration). Both valid; noting it's intentional.
  3. STYLED_JSX_STYLE_PATTERN lookahead assumes id is the first attribute — true for the pinned runtime.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): preserve styled-jsx during streaming SSR

Reviewed the full diff, dev/prod parity across all four request-handling paths, and verified the styled-jsx runtime shape against the pinned styled-jsx@5.1.6. Ran the relevant suites locally — all pass (14 styled-jsx, 137 pages-data/response, 5 pages-router streaming/ISR/static-export) — and vp check is clean on all six changed source files. No blocking issues found.

Verified correct

  • CodeQL alert on pages-page-data.ts is a false positive. The flagged freshStyledJsxHTML/styledJsxHTML strings originate from renderToStringAsync(registry.styles()) — styled-jsx's style() renders developer-authored CSS through React's dangerouslySetInnerHTML. No request-derived/user data flows into the style content, and the injection targets (shellSuffix, cached <head>) are trusted document templates, not the untrusted style payload. This matches Next.js's own styled-jsx emission exactly. Safe to dismiss.
  • disableNextSsg: true fixes a real bug. Without it, SWC's default client-page tree-shaker replaces getStaticProps/getServerSideProps with __N_SSG/__N_SSP and strips the functions from vinext's server module graph. The new styled-jsx-streaming.tsx fixture (with getServerSideProps) plus the data-export tests confirm the functions survive. The inline comment correctly explains why this SWC option belongs here.
  • ESM runtime conversion matches the pinned runtime. Verified all three replacement regexes in convertStyledJsxRuntimeToEsm match styled-jsx@5.1.6/dist/index/index.js (client-only require, var React = require(...), and the exports.* tail). The fail-loud throw when any replacement is a no-op is a sound guard against a future layout drift.
  • getRuntimeDistPath() resolution is correct. require.resolve("styled-jsx").../styled-jsx/index.js (which itself is module.exports = require('./dist/index')), so dirname(...) + dist/index/index.js lands on the real runtime.
  • Fallback runtime is a coherent no-op. When the runtime can't be resolved, styled-jsxSTYLED_JSX_FALLBACK_RUNTIME (empty registry) and styled-jsx/style → null (default resolution). createStyleRegistry().styles() returns [], so the generated entry's static import never crashes and simply emits no styles. Consistent across both ids.
  • Double-collect + flush is not duplicative. Eager collectPagesStyleRegistryHtml reads shell styles (available after renderToReadableStream resolves the shell synchronously) and flushes → head; the bodyAllReady-gated continuation collects only post-flush Suspense styles → before </body>.
  • ISR tee ordering holds; no backpressure deadlock. bodyAllReady is captured before tee(). writePagesIsrCache drains its branch via readStreamAsText (driving the render → resolving allReady → resolving finalShellSuffix), and buildPagesCompositeStream independently drains the response branch and awaits the same finalShellSuffix. Both consumers drive rendering; nothing waits on an undrained branch.
  • renderErrorPage refactor is behavior-preserving. The removed non-Document branch is fully subsumed by streamPageToResponse's default-shell path (dev-server.ts:513-529), and preserveExistingContentType: statusCode === 404 (dev-server.ts:558) reproduces the old conditional Content-Type. Error pages now additionally get styled-jsx. Prod pages-server-entry.ts isn't an error-page path, so no parity gap introduced.
  • ISR style refresh is complete. removeStyledJsxStyles strips all __jsx- styles from both head and pre-</body> positions on every regeneration (including when the fresh render emits none), and re-inserts the freshly collected run into <head>. The isr-second-render-state.tsx dynamic z-index: ${timestamp} fixture proves styles actually refresh rather than going stale.

Non-blocking observations

  1. Dev/prod load styled-jsx with different optionality assumptions. Prod (pages-server-entry.ts:212) statically imports styled-jsx (would hard-fail if unresolvable, though the plugin's fallback runtime prevents that), while dev (dev-server.ts:365) wraps the import in try/catch as optional. In practice both are safe because styled-jsx is a hard dependency of next and the plugin supplies a fallback. A one-line comment on the prod static import noting why it's safe to import unconditionally would stop a future reader from "fixing" the asymmetry by making prod optional (which would silently drop styles).

  2. STYLED_JSX_STYLE_PATTERN lookahead assumes id is the first attribute. <style\b(?=[^>]*\bid=["']__jsx-) relies on styled-jsx emitting id before any >, which mapRulesToStyle does for the pinned runtime. Fine as-is; just coupled to attribute ordering alongside the convertStyledJsxRuntimeToEsm runtime-shape coupling (which at least fails loud).

  3. ISR regen relocates late styles from before-</body> to <head>. First render splits shell styles (head) from late Suspense styles (before </body>); regeneration uses non-streaming renderToStringAsync and folds the combined run into <head>. Both placements are valid and covered by the custom-document ISR test — noting it's an intentional, not accidental, difference.

Nothing here blocks merge. Items 1-3 are optional clarifying comments for future maintainers.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2894. Summary of findings:

Review Summary

Thorough review of the full diff, dev/prod parity across all four request-handling paths, and verification of the styled-jsx runtime shape against pinned styled-jsx@5.1.6. All relevant suites pass locally (14 styled-jsx, 137 pages-data/response, 5 pages-router streaming/ISR/static-export) and vp check is clean. No blocking issues found.

Key findings

  • CodeQL alert is a false positive. The flagged freshStyledJsxHTML/styledJsxHTML come from React-rendering registry.styles() (developer-authored CSS via styled-jsx's own dangerouslySetInnerHTML). No user/request data flows into the content, and injection targets are trusted document templates. Matches Next.js behavior exactly — safe to dismiss.
  • disableNextSsg: true fixes a real bug where SWC was silently stripping getStaticProps/getServerSideProps from vinext's server module graph. Well-covered by new tests/fixtures.
  • ESM runtime conversion regexes all match the pinned runtime, with a sound fail-loud guard against future drift.
  • ISR tee ordering holds with no backpressure deadlock — both cache-write and composite-stream branches independently drive the render and await the same finalShellSuffix.
  • renderErrorPage refactor is behavior-preserving — the removed non-Document branch is subsumed by streamPageToResponse's default-shell path.

Non-blocking observations (optional clarifying comments)

  1. Dev/prod encode different optionality assumptions for the styled-jsx import (both safe via the fallback runtime); a one-line comment on the prod static import would help.
  2. STYLED_JSX_STYLE_PATTERN is coupled to id-first attribute ordering.
  3. ISR regen intentionally relocates late styles from before-</body> to <head>.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(pages): preserve styled-jsx during streaming SSR

Independent pass over the full diff, with source-level verification against the pinned styled-jsx@5.1.6 runtime and dev/prod parity across renderPagesPageResponse, streamPageToResponse, _renderIsrPassToStringAsync, and the ISR regeneration path. Ran the relevant suites locally — all green (14 styled-jsx, 137 pages-data/response) — and vp check is clean on all six changed source files. No blocking issues found.

Verified against the actual runtime

  • JSXStyle registers styles synchronously during SSR render (styled-jsx/dist/index/index.js:472-474: if (typeof window === "undefined") { registry.add(props); return null; }). This is the load-bearing fact for the whole design: shell styles are all present when renderToReadableStream resolves the shell, so the eager collectPagesStyleRegistryHtml<head> collection is complete, and Suspense/late styles register only as their boundaries render, so gating the second collection on bodyAllReady captures exactly those. No overlap.
  • flush() clears _indices/_instancesCounts (:373-379), so the eager collect-then-flush cannot double-emit into the late collection. Confirmed.
  • registry.styles() always returns an array (mapRulesToStyle uses .map(), :288-303), so both the prod entry's styles.length > 0 guard and collectPagesStyleRegistryHtml's Array.isArray(styles) && styles.length === 0 check are safe.
  • STYLED_JSX_STYLE_PATTERN id prefix is correct: computeId returns "jsx-" + baseId (:260) and mapRulesToStyle prepends "__" (:294), yielding id="__jsx-…", which the lookahead id=["']__jsx- matches.
  • convertStyledJsxRuntimeToEsm matches the pinned runtime. The file tail is exactly exports.StyleRegistry = …; exports.createStyleRegistry = …; exports.style = JSXStyle; exports.useStyleRegistry = useStyleRegistry;\n, so all three replacements hit; the fail-loud throw on any no-op is a sound guard against future layout drift.

Verified correct

  • ISR tee() has no backpressure deadlock. bodyAllReady is captured before tee(); finalShellSuffix awaits lateStyledJsxHTML → bodyAllReady. Both consumers drive the render independently — writePagesIsrCache drains the cache branch via readStreamAsText and buildPagesCompositeStream drains the response branch — and both await the same finalShellSuffix. Neither waits on an undrained branch. The bot path buffers compositeStream (which already awaits finalShellSuffix), so late styles land in the buffered/ETag'd HTML too.
  • disableNextSsg: true fixes a real bug. Without it SWC's client-page tree-shaker rewrites getStaticProps/getServerSideProps to __N_SSG/__N_SSP and strips the functions from vinext's server module graph. The new styled-jsx-static-props/styled-jsx-streaming fixtures and the not.toContain("__N_SSP"/"__N_SSG") transform assertions lock this in.
  • renderErrorPage refactor is behavior-preserving. The removed non-Document branch is subsumed by streamPageToResponse's else default-shell path (dev-server.ts:513-529), and preserveExistingContentType: statusCode === 404 reproduces the old conditional Content-Type. Error pages additionally gain styled-jsx.
  • ISR head refresh is complete. removeStyledJsxStyles is global/unanchored so it strips __jsx- styles from both <head> and the pre-</body> region on every regen (including when the fresh render emits none), then re-inserts the fresh run into <head>. The document-isr-styled and isr-second-render-state (z-index: ${timestamp}) fixtures prove the styles actually advance and the stale ones are gone (not.toMatch(z-index: ${firstTimestamp})).
  • Fallback runtime is a coherent no-op. No nextSTYLED_JSX_FALLBACK_RUNTIME (empty registry, styles()[]) and styled-jsx/style → null; the generated entry's static import never crashes and emits no styles.

Non-blocking observations (agree with prior review)

  1. Dev/prod encode different optionality for the styled-jsx import. Prod (pages-server-entry.ts) statically imports styled-jsx; dev (dev-server.ts:365) wraps it in try/catch. Both are safe because the plugin always resolves the id (real runtime or fallback), but a one-line comment on the prod static import stating why it is unconditionally safe would stop a future reader from "fixing" the asymmetry into an optional import (which would silently drop styles).
  2. STYLED_JSX_STYLE_PATTERN and convertStyledJsxRuntimeToEsm are both coupled to the pinned runtime's shape (id-first attribute ordering / export tail). The ESM converter fails loud on drift; the style pattern does not. Acceptable given styled-jsx is pinned via next.

Nothing here blocks merge.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2894.

Summary

I performed an independent review of PR #2894 (fix(pages): preserve styled-jsx during streaming SSR), verifying the implementation against the actual pinned styled-jsx@5.1.6 runtime source rather than just reading the diff. No blocking issues found.

Key verifications (against runtime source)

  • JSXStyle registers styles synchronously during SSR render (index.js:472-474) — this is the load-bearing fact making the eager-collect (shell → <head>) + late-collect (Suspense → before </body>) split correct with no overlap.
  • flush() clears the indices (:373-379), so eager + late collection cannot double-emit.
  • registry.styles() always returns an array, so all length guards in both entries are safe.
  • STYLED_JSX_STYLE_PATTERN's __jsx- id prefix and the convertStyledJsxRuntimeToEsm export-tail regex both match the pinned runtime exactly; the fail-loud throw guards against drift.

Verified correct

  • ISR tee() has no backpressure deadlock (both branches independently drive the render; bot buffer path also awaits finalShellSuffix).
  • disableNextSsg: true fixes a real bug (SWC was stripping getStaticProps/getServerSideProps).
  • renderErrorPage refactor is behavior-preserving (removed branch subsumed by streamPageToResponse's default-shell path) and adds styled-jsx to error pages.
  • ISR head refresh removes stale head+late styles and re-inserts fresh ones.
  • Fallback runtime is a coherent no-op when next is absent.

Non-blocking (optional)

  1. Dev/prod encode different optionality assumptions for the styled-jsx import (both safe) — a one-line comment on the prod static import would help future maintainers.
  2. Regex coupling to the pinned runtime shape (acceptable; ESM converter fails loud).

Tests pass locally (14 styled-jsx, 137 pages-data/response) and vp check is clean on all six changed source files.

github run

@james-elicx
james-elicx marked this pull request as ready for review August 11, 2026 05:19
@james-elicx

Copy link
Copy Markdown
Member Author

Independent exact-head review of 7e1fd46eefdcd3c4eac98ff6a560f1e21a6258dd: NO FINDINGS.

I reviewed the full 13-file diff independently, including:

  • request-local styled-jsx registry lifetime across shell readiness, Suspense completion, body streaming, and ISR tee/cache consumption;
  • shell/late-style collection, nonce propagation, flush behavior, and cached-head regeneration/removal semantics;
  • Pages dev, production, error-page, GSSP/GSP/ISR, custom _document, and relocated static-export paths;
  • preservation of Pages data exports through the SWC styled-jsx transform and the downstream client-only server-export stripping boundary;
  • the portable ESM runtime conversion, styled-jsx/style facade, relative-root resolution, missing-runtime fallback, and wrapper-shape failure behavior;
  • the latest native load filter, including the corrected NUL-prefixed virtual IDs and its non-virtual fast-path exclusion.

Fresh validation on this immutable head:

  • focused Vitest: 535/535 passed across styled-jsx, Pages data/response, dev/prod integration, ISR refresh, and actual relocated static-export output;
  • exact targeted upstream suite: test/e2e/streaming-ssr/index.test.ts passed 5/5, retries 0/0;
  • the required package builds completed successfully as part of the targeted harness;
  • CI is fully green, local/remote head SHAs match, merge state is clean, git diff --check passed, and the worktree remained clean.

I found no actionable streaming/lifecycle gap, virtual-runtime or NUL-filter regression, dev/production/static-export divergence, or missing high-value test at this head.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx james-elicx changed the title fix(pages): preserve styled-jsx during streaming SSR fix(pages): preserve styled-jsx CSS ordering in SSR Aug 11, 2026
@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): preserve styled-jsx CSS ordering in SSR

Independent review of the full 13-file diff at head, with focus on dev/prod parity, the removed streaming marker, and the styled-jsx collection lifecycle. Ran the relevant suites locally — all green (14 styled-jsx, 139 pages-data/response, 5 pages-router streaming/ISR) — and vp check is clean on all six changed source files. No blocking issues found. A few non-blocking observations below, one of which corrects the record on prior reviews.

Verified correct

  • convertStyledJsxRuntimeToEsm matches the installed runtime. Confirmed against styled-jsx@5.1.6/dist/index/index.js: require('client-only');, var React = require('react');, and the four-line exports.* tail all match the anchored regexes, and React__default/_interopDefaultLegacy still work after swapping to import * as React. The fail-loud throw on any no-op replacement is a sound guard against future layout drift.
  • getRuntimeDistPath() resolution is correct. require.resolve("styled-jsx").../styled-jsx/index.js; dirname(...) + dist/index/index.js lands on the real runtime.
  • disableNextSsg: true is valid and fixes a real bug. Confirmed the option exists in the installed next@16.2.7 SWC options, and the new transform test asserts getServerSideProps/getStaticProps survive without __N_SSG/__N_SSP. Previously these were silently stripped from vinext's server module graph.
  • ISR head refresh is index-safe. refreshCachedHeadTags slices cachedHtml at indices computed from a length-preserving toLowerCase()/blank-out, so offsets stay valid; removeStyledJsxStyles strips __jsx- styles from both <head> and pre-</body> on every regen (including empty fresh renders).
  • renderErrorPage refactor is behavior-preserving. The removed non-Document inline-HTML branch is subsumed by streamPageToResponse's else default-shell path (dev-server.ts:498-515), and preserveExistingContentType: statusCode === 404 reproduces the old conditional Content-Type. Error pages additionally gain styled-jsx.
  • Fallback runtime is a coherent no-op when next/styled-jsx can't be resolved (styles()[], styled-jsx/style → default resolution), so the prod entry's static import { StyleRegistry, createStyleRegistry } from "styled-jsx" never crashes.

Non-blocking observations

  1. The prior reviews describe a "double-collect + flush (shell→<head>, then late→before </body>)" that does not exist in this final version. Both the dev path (dev-server.ts:407-417) and the prod path (pages-page-response.ts:610-623) do a single collect + flush after allReady, injecting all styled-jsx rules (shell and Suspense) before </body>. There is no eager <head> collection. This is fine functionally (styled-jsx styles are page-local and go after React's body stylesheet resources), but the "no double-emit" reasoning in the earlier ask-bonk/reviewer comments is verifying code that isn't here. Worth aligning the PR narrative so future readers aren't misled.

  2. The __vinextStreamedHtmlResponse marker is now dead for HTML responses, but its infrastructure and tests remain. renderPagesPageResponse no longer sets the marker (it returns a plain buffered new Response(bufferedFullHtml, ...) and deletes Content-Length upstream at pages-page-response.ts:685). Consequently:

    • isVinextStreamedHtmlResponse(...) at prod-server.ts:2318 and the shouldStripStreamLength branch at worker-utils.ts:87 are now unreachable for HTML — the buffered path (arrayBuffer → recomputed framing) handles it correctly, so this is safe.
    • The marker-propagation in pages-request-pipeline.ts:866-870 and unified-request-context.ts:324-326 now only ever copies undefined.
    • deploy.test.ts:1595 ("...only for tagged streamed Pages HTML") and the equivalent in features.test.ts:4742 still set the marker synthetically to exercise mergeHeaders, so they pass but no longer reflect a real runtime code path.

    None of this is incorrect, but it's vestigial. Consider either removing the marker machinery (and its now-synthetic tests) or leaving a comment explaining it is retained only for the merge-header unit tests, so a future reader doesn't assume HTML responses still carry it.

  3. Dev buffers the full document but still emits Transfer-Encoding: chunked (dev-server.ts:543, then a single res.write(prefix) + res.end(bufferedBody + finalSuffix)). Since the body is now fully known, this could set Content-Length instead (as prod does via sendCompressed). Valid HTTP either way; a minor dev-only efficiency/parity nit.

  4. Dev/prod encode different optionality for the styled-jsx import. Prod (pages-server-entry.ts:212) statically imports it; dev (dev-server.ts:352) wraps it in try/catch. Both are safe because the plugin always resolves the id (real runtime or fallback), but a one-line comment on the prod static import stating why it is unconditionally safe would stop a future reader from "fixing" the asymmetry into an optional import (which would silently drop styles). (Also raised in prior reviews.)

  5. removeStyledJsxStyles alternation includes <plaintext\b[^>]*>[\s\S]*$, which consumes to end-of-string once matched. React/Next never emit <plaintext>, so this is only a theoretical hazard for hand-authored cached HTML, but it means a stray <plaintext> before a styled-jsx block would swallow the rest of the document during regen. Extremely low risk given the pinned pipeline; flagging for completeness.

Nothing here blocks merge. Items 1-2 are the most useful to address so the codebase and its tests stay honest about what the streaming marker now means.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2894.

Summary

I did an independent review of PR #2894 (fix(pages): preserve styled-jsx CSS ordering in SSR), focusing on dev/prod parity, the removed streaming marker, and the styled-jsx collection lifecycle. Ran the relevant suites locally (14 styled-jsx, 139 pages-data/response, 5 pages-router streaming/ISR — all green) and vp check is clean. No blocking issues.

Verified correct

  • convertStyledJsxRuntimeToEsm regexes match the installed styled-jsx@5.1.6 runtime exactly; the fail-loud guard is sound.
  • disableNextSsg: true is a real option in next@16.2.7 and fixes silent stripping of getServerSideProps/getStaticProps.
  • ISR head refresh is index-safe (length-preserving lowercasing) and strips styled-jsx from both head and pre-</body>.
  • The renderErrorPage refactor is behavior-preserving (removed inline branch subsumed by streamPageToResponse's default-shell path).
  • The no-op fallback runtime keeps the prod entry's static styled-jsx import from ever crashing.

Non-blocking findings I raised (not covered by prior reviews)

  1. The prior reviews describe a "double-collect (shell→head, late→body)" split that does not exist in the final code — both dev and prod do a single collect after allReady, all styles injected before </body>. Worth aligning the PR narrative.
  2. The __vinextStreamedHtmlResponse marker is now dead for HTMLrenderPagesPageResponse never sets it, so shouldStripStreamLength in worker-utils.ts and the branch in prod-server.ts:2318 are unreachable, and deploy.test.ts/features.test.ts only exercise it synthetically. Safe, but vestigial; suggest removing or commenting.
  3. Dev buffers the whole document but still emits Transfer-Encoding: chunked (minor parity/efficiency nit vs prod's Content-Length).
  4. Prod's static styled-jsx import vs dev's try/catch — a one-line comment on the prod import would prevent a future "fix" that silently drops styles.
  5. removeStyledJsxStyles's <plaintext> alternation consumes to end-of-string (theoretical only).

Items 1 and 2 are the most valuable to address so the code and its tests stay honest about the streaming marker's current (non-)role.

github run

@james-elicx
james-elicx force-pushed the codex/fix-streaming-ssr-styled-jsx-31439707085 branch from 63e79a8 to c423475 Compare August 11, 2026 09:30
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): preserve styled-jsx CSS ordering in SSR

Independent review at head c423475, verifying the styled-jsx collection lifecycle, ISR tee ordering, the removed streaming marker, and dev/prod parity against the pinned styled-jsx@5.1.6 runtime source. Ran the relevant suites locally — all green (14 styled-jsx, 72 pages-page-response, 465 pages-data/router) — including the new allReady-buffering and content-length integration tests. No blocking issues found.

Verified correct

  • ESM runtime conversion matches the pinned runtime. Confirmed all three replacements in convertStyledJsxRuntimeToEsm (require('client-only'), var React = require('react'), and the four-line exports.* tail) hit against styled-jsx@5.1.6/dist/index/index.js. The fail-loud throw on any no-op is a sound guard against future layout drift. getRuntimeDistPath() resolves the runtime correctly via require.resolve + dist/index/index.js.
  • Per-request registry isolation is safe. createStyleRegistry() produces a fresh StyleSheetRegistry per request (_indices/_instancesCounts are instance state). The only module-level mutable state (var cache = {} in computeId/computeSelector) is deterministic id/selector memoization, not per-request data, so concurrent requests cannot leak styles.
  • ISR tee() ordering has no deadlock and no stale suffix. bodyAllReady is awaited before tee(), and finalShellSuffix is an eagerly-resolved Promise.resolve(injectPagesLateStyles(...)), so await options.shellSuffix in both the cache-write branch (writePagesIsrCache) and the composite branch never blocks on undrained data. Both tee branches are drained (readStreamAsText on each), so late styles land identically in the cached HTML and the served HTML.
  • disableNextSsg: true is a valid option (confirmed present in the installed next@16.2.7 SWC options.d.ts:27) and fixes a real bug — without it SWC's client-page tree-shaker rewrites getStaticProps/getServerSideProps to __N_SSG/__N_SSP and strips them from vinext's server module graph. The new styled-jsx-streaming.tsx (gSSP) + styled-jsx-static-props.tsx fixtures and the transform assertions lock this in.
  • The __vinextStreamedHtmlResponse marker is now genuinely dead and correctly removed everywhere. renderPagesPageResponse returns a plain buffered new Response(bufferedFullHtml, ...) with Content-Length deleted so the runtime recomputes framing. The marker read sites in prod-server.ts, worker-utils.ts (shouldStripStreamLength), and the copy sites in pages-request-pipeline.ts / unified-request-context.ts are all removed, and the now-synthetic deploy.test.ts/features.test.ts cases were deleted rather than left dangling. No stale source references remain (only dist/ build output, which is regenerated).
  • renderErrorPage refactor is behavior-preserving. The removed non-Document inline-HTML branch is subsumed by streamPageToResponse's default-shell path, and preserveExistingContentType: statusCode === 404 reproduces the old conditional Content-Type. Error pages additionally gain styled-jsx. The error path re-throws on allReady rejection before any headers are written, so no partial/corrupt response is emitted.
  • ISR head refresh is complete and index-safe. refreshCachedHeadTags strips all __jsx- styles from both <head> and pre-</body> on every regen (including empty fresh renders) via a length-preserving lowercase/blank-out before computing </head>/</body> offsets, then re-inserts the fresh run — consistent with the initial cache fill, which also injects late styles before </body>. The isr-second-render-state.tsx (z-index: ${timestamp}) fixture proves styles actually advance.

Non-blocking observations

  1. Dev/prod encode different optionality for the styled-jsx import. Prod (pages-server-entry.ts:212) statically imports styled-jsx; dev (dev-server.ts:352) wraps it in try/catch. Both are safe because the plugin always resolves the id (real runtime or the STYLED_JSX_FALLBACK_RUNTIME no-op), but a one-line comment on the prod static import stating why it is unconditionally safe would stop a future reader from "fixing" the asymmetry into an optional import (which would silently drop styles). (Raised in prior reviews; still applies.)

  2. Dev buffers the full document but still sends Transfer-Encoding: chunked (dev-server.ts:543, then res.write(prefix) + res.end(bufferedBody + finalSuffix)). Since the body is fully known, dev could set Content-Length like prod does. Valid HTTP either way — a minor dev-only parity/efficiency nit.

  3. STYLED_JSX_STYLE_OPEN_PATTERN / the removeStyledJsxStyles <plaintext> alternation are coupled to the pinned runtime's output shape (id-first attribute ordering; <plaintext> consuming to end-of-string). React/Next never emit <plaintext> and styled-jsx always emits id first, so both are theoretical only — flagging for completeness alongside the (fail-loud) ESM-converter coupling.

Nothing here blocks merge. All three items are optional clarifying comments for future maintainers.

@ask-bonk

ask-bonk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2894.

Summary

I did an independent review of PR #2894 (fix(pages): preserve styled-jsx CSS ordering in SSR) at head c423475, verifying against the pinned styled-jsx@5.1.6 runtime source rather than just the diff. Ran the relevant suites locally (14 styled-jsx, 72 pages-page-response, 465 pages-data/router — all green) and confirmed the ESM conversion regexes match the installed runtime. No blocking issues found.

Key verifications

  • ESM runtime conversion — all three regexes match styled-jsx@5.1.6; fail-loud guard is sound.
  • Per-request registry isolation — each request gets a fresh registry; the only module-level state (computeId/computeSelector cache) is deterministic memoization, so no cross-request style leaks.
  • ISR tee() orderingallReady awaited before tee; finalShellSuffix is an eagerly-resolved promise, so both cache-write and composite branches drain without deadlock and get identical late styles.
  • disableNextSsg: true — confirmed valid in next@16.2.7 and fixes real silent stripping of getStaticProps/getServerSideProps.
  • Dead streaming marker — genuinely removed everywhere in source (only dist/ build output retains it); synthetic tests deleted rather than left dangling.
  • renderErrorPage refactor — behavior-preserving; re-throws on allReady rejection before headers are written.
  • ISR head refresh — index-safe (length-preserving lowercasing) and consistent with the initial cache fill.

Non-blocking observations

  1. Prod static styled-jsx import vs dev try/catch — a one-line "why safe" comment would prevent a future "fix" that drops styles.
  2. Dev buffers the full document but still emits Transfer-Encoding: chunked (minor parity/efficiency nit vs prod's Content-Length).
  3. Regex coupling to the pinned runtime shape (id-first ordering, <plaintext> alternation) — theoretical only.

All items are optional clarifying comments; nothing blocks merge.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants