fix(app): preserve next/dynamic CSS cascade order - #2879
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
Review:
|
|
Posted the review to PR #2879. Summary of findings: Review postedBlocking (1):
Non-blocking (should address): Nits: the bare The core approach is sound and |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/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.
Re-review: fix(app): preserve next/dynamic CSS cascade order
Re-reviewed after the earlier pass. The chromium crash from the previous review is fixed (it.runIf(HAS_CHROMIUM) + chromium.executablePath() existence check at tests/app-router-next-dynamic-css-order.test.ts:18-19), so the hard "Executable doesn't exist" failure is gone. But the deeper test-placement issue still holds, and it's now reinforced by this PR's own base commit.
Blocking
1. New test still runs a full build + preview server in the parallel unit project
tests/app-router-next-dynamic-css-order.test.ts is not in vite.config.ts's integration include, nor in the unit exclude, so it falls through to the unit project (include: ["tests/**/*.test.ts", ...]). The unit project runs files in parallel; only integration sets fileParallelism: false.
The guarded browser tests skip cleanly when no browser is present — good. But beforeAll (lines 42-54) unconditionally runs createBuilder().buildApp() + preview(), and two non-browser it(...) cases (lines 151, 179) always run. So even with no chromium, this test performs a full production build + preview server in parallel with the rest of the unit suite, against a checked-in shared fixture (tests/fixtures/next-dynamic-css) and its dist/.
This is exactly what the unit exclude comment warns against (vite.config.ts:178-184): "Integration tests: spin up Vite dev servers against shared fixture dirs. Must run serially to avoid Vite deps optimizer cache races (node_modules/.vite/*) that produce 'outdated pre-bundle' 500s. When adding a test that calls startFixtureServer() or createServer(), move it here."
Crucially, the immediate base commit of this PR — d558d62 "fix(ci): serialize tests sharing Vite optimizer cache (#2874)" — moved sass-tsconfig-paths.test.ts and tsconfig-path-alias-resolution.test.ts (both createBuilder().buildApp()-against-fixture tests) into integration for precisely this reason. Adding a new build+preview-against-shared-fixture test back into the parallel unit project directly re-introduces the race class that base commit just fixed.
Fix: add tests/app-router-next-dynamic-css-order.test.ts to the integration project include (and the unit exclude). If the parity assertions truly need a live browser as their primary signal, an tests/e2e/ Playwright test would be the more conventional home (it's currently the only Vitest file driving Playwright directly).
Non-blocking (should address)
2. Still no unit coverage for the new pure helpers
Confirmed only appGlobalCssOwnerChunkName is referenced in tests (as a config identity in build-optimization.test.ts:3592). The actual ordering/dedup logic is only exercised by the browser E2E above, which is skipped whenever browsers aren't installed — so in a browserless CI lane the new logic has effectively zero assertions. Several new functions are pure and unit-testable without a browser, per AGENTS.md ("add a focused unit test for the new helper module"):
normalizeRscAssetsManifestCssOrderSource/reorderClientReferenceCss(build/rsc-css-order.ts) — feed a manifest string + client manifest, assert reordering.inlineStyleCoversStylesheetHref,dedupeGlobalCssOwnerStylesheetLinks(server/app-inline-css-client.ts) — jsdom.createRscCssResourceCrossOriginPlugintransform — assertcrossOrigininjection for the known input.
3. createRscCssResourceCrossOriginPlugin is a brittle string patch with a silent no-op
rsc-css-resource-crossorigin.ts:24-25 does code.replace('"data-rsc-css-href": href', ...). I confirmed the target still matches @vitejs/plugin-rsc's generated ResourcesFn.toString() output (node_modules/@vitejs/plugin-rsc/dist/plugin-Cbs9j6lP.js:2072). But that string is whitespace/token-sensitive: if the plugin minifies, reindents, or renames href, the code.includes(...) guard returns null and CSS crossorigin parity silently regresses — no error, no failing test. Pin the current match with a unit test, and consider a build-time warning when the expected token is absent.
4. reorderClientReferenceCss correctness edge cases (build/rsc-css-order.ts:57,60-72)
- Returns on the first client-manifest key whose
ownFirstprefix-matchescss, then keepscss.slice(ownFirst.length)untouched (line 70). If a client reference's CSS is assembled from more than one client chunk, only the first chunk's slice is reordered. hrefMatchesFile(line 57) matches onendsWith("/" + file).fileis a manifest-relative path so collisions are less likely than a bare basename, but two entries sharing a suffix in different base directories could still cross-match in thecss.find(...)at line 68.- The
?? filefallback (line 68) inserts a bare manifest-relative path (no base prefix) when a dependency isn't found incss, which can emit a mismatched/duplicate href instead of surfacing the inconsistency.
All three are untested (see #2), so they're easy to regress.
5. installGlobalCssOwnerStylesheetDedupe() runs at module top level, unguarded (app-browser-entry.ts:206)
Called unconditionally at import time; dedupeGlobalCssOwnerStylesheetLinks() touches document.head (no typeof document guard) and installs a permanent MutationObserver on document.head's childList that lives for the page lifetime. The browser entry is browser-only today, so it's safe in practice — but the prior removeStylesheetLinksCoveredByInlineCss() was only ever invoked inside a function/effect. A one-line guard or an invariant comment would make the top-level side effect intentional rather than incidental.
6. computeDynamicImportPreloads ordering flip is global (utils/lazy-chunks.ts:115-125)
CSS emission now happens after imported-chunk recursion, so preload order changes from [own.js, own.css, imported…] to [own.js, imported…, own.css] (the build-optimization.test.ts snapshot updates accordingly: widget-helper.js now precedes widget.css). This is the correct execution order, but it changes preload ordering for every next/dynamic boundary in every app, not just this fixture. Likely fine, but please confirm it's intentional and no consumer relied on own-first CSS ordering.
Nits
app-global-css-owner.ts:103,105ends the generated owner module with a bare expression statementglobalThis[Symbol.for("vinext.css.owner")];. If it's a treeshaking side-effect marker, a one-line comment would help; as written it reads as dead code.normalizeRscAssetsManifestCssOrderSourcesilently no-ops (via theJSON.parsecatch,rsc-css-order.ts:82-86) when the RSC manifest carries runtime-placeholder hrefs (renderBuiltUrl/ asset-prefix / CDN configs) — those emit unquoted JS expressions that aren't valid JSON. Fails safe, but the CSS-order fix won't apply for those deployments; worth a comment or tracked follow-up.
The core approach (stable global-CSS ownership, graph-wide order reconciliation, browser-side dedup) is sound and vp check passes. Item #1 should be resolved before merge — it re-introduces the exact deps-optimizer race class the base commit d558d62 fixed.
|
Review posted to PR #2879. SummaryBlocking (1):
Non-blocking: Nits: bare The core approach is sound and |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(app): preserve next/dynamic CSS cascade order
Re-reviewed at 906fa66. The blocking issue from the prior three passes is now fixed — tests/app-router-next-dynamic-css-order.test.ts is added to the unit exclude (vite.config.ts:196) and the integration include (vite.config.ts:266), so it runs serially in the integration project and is skipped by the unit project. I verified both:
vp test run tests/app-router-next-dynamic-css-order.test.ts --project unit→No test files found(correctly excluded)vp test run tests/app-router-next-dynamic-css-order.test.ts --project integration→2 passed | 6 skipped(browser tests skip cleanly viait.runIf(HAS_CHROMIUM); the build + manifest-order assertions run and pass)
This resolves the deps-optimizer race class that base commit d558d62 (#2874) fixed. No blocking issues remain.
Non-blocking (should address)
1. Still no browserless unit coverage for the pure helpers
Confirmed only appGlobalCssOwnerChunkName is referenced in tests (build-optimization.test.ts:3591). In a browserless CI lane, the reorder/dedup/crossorigin logic is exercised only by the two non-browser assertions in the integration file (which read the built manifest). The functions themselves are pure and trivially unit-testable per AGENTS.md ("add a focused unit test for the new helper module"):
reorderClientReferenceCss/normalizeRscAssetsManifestCssOrderSource(build/rsc-css-order.ts) — feed a manifest string + client manifest, assert reordering (and the JSON.parse no-op path).inlineStyleCoversStylesheetHref/dedupeGlobalCssOwnerStylesheetLinks(server/app-inline-css-client.ts) — jsdom.createRscCssResourceCrossOriginPlugintransform — assertcrossOrigin: ""injection for the known input, which doubles as the pin for #2.
2. createRscCssResourceCrossOriginPlugin is a brittle string patch with a silent no-op (rsc-css-resource-crossorigin.ts:24-25)
code.replace('"data-rsc-css-href": href', ...). I re-confirmed the target still matches @vitejs/plugin-rsc's generated ResourcesFn.toString() output at plugin-Cbs9j6lP.js:2072. But if the plugin minifies, reindents, or renames href, the code.includes(...) guard returns null and CSS crossorigin parity silently regresses — no error, no failing test. Pin the current token with a unit test, and consider a build-time warning when it's absent.
3. reorderClientReferenceCss edge cases (build/rsc-css-order.ts:57,60-72)
- Returns on the first client-manifest key whose
ownFirstprefix-matchescss, keepingcss.slice(ownFirst.length)untouched (line 70). If a client reference's CSS spans more than one client chunk, only the first chunk's slice reorders. hrefMatchesFile(line 57) usesendsWith("/" + file);fileis manifest-relative so collisions are unlikely, but two entries sharing a suffix across base dirs could still cross-match.- The
?? filefallback (line 68) is effectively dead given line 64 already verifiedownFirst⊆cssanddependenciesFirstis a permutation ofownFirst— but if that invariant ever breaks it silently inserts a bare, base-less path instead of surfacing the inconsistency. A unit test would lock the invariant in.
4. installGlobalCssOwnerStylesheetDedupe() runs at module top level (app-browser-entry.ts:206)
It has an idempotency guard (GLOBAL_CSS_OWNER_DEDUPE symbol, app-inline-css-client.ts:87), so double-install is handled. But dedupeGlobalCssOwnerStylesheetLinks() touches document.head with no typeof document guard and installs a permanent MutationObserver for the page lifetime. The server browser entry is browser-only today (hydrateRoot), so it's safe in practice — but the prior removeStylesheetLinksCoveredByInlineCss() was only ever called inside a function. A one-line invariant comment (or typeof document guard) would make the top-level side effect intentional rather than incidental.
5. computeDynamicImportPreloads ordering flip is global (utils/lazy-chunks.ts:115-125)
CSS emission now happens after imported-chunk recursion, so preload order changes from [own.js, own.css, imported…] to [own.js, imported…, own.css] (the build-optimization.test.ts:1655 snapshot updates accordingly: widget-helper.js now precedes widget.css). This is the correct execution order, but it changes preload ordering for every next/dynamic boundary in every app, not just this fixture. Likely fine (mirrors static-dependency execution order), but please confirm it's intentional and no consumer relied on own-first CSS ordering.
Nits
app-global-css-owner.ts:103,105ends the generated owner module with a bare expression statementglobalThis[Symbol.for("vinext.css.owner")];. If it's a treeshaking side-effect marker, a one-line comment would help; as written it reads as dead code.normalizeRscAssetsManifestCssOrderSourcesilently no-ops (via theJSON.parsecatch,rsc-css-order.ts:82-86) when the RSC manifest carries runtime-placeholder hrefs (renderBuiltUrl/ asset-prefix / CDN configs) — those emit unquoted JS expressions that aren't valid JSON. Fails safe, but the CSS-order fix won't apply for those deployments; worth a comment or tracked follow-up.
The core approach (stable global-CSS ownership, graph-wide order reconciliation, browser-side dedup) is sound, vp check passes on the new files, and the previously blocking test-placement issue is resolved. The remaining items are non-blocking — none require a change before merge, though the unit coverage in #1 would meaningfully de-risk items #2–#3.
|
Review posted to PR #2879. SummaryRe-reviewed at Blocking: RESOLVED. The test-placement issue that failed CI in every prior pass is now fixed.
This closes the deps-optimizer race class that base commit Non-blocking (carried forward, all still valid):
Nits: bare The core approach is sound, |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(app): preserve next/dynamic CSS cascade order
Re-reviewed at 0f484b6. The new commit test(css): harden cascade order helpers addresses essentially all of the prior-pass feedback. Verified each item empirically.
Previously-blocking issue: still RESOLVED
tests/app-router-next-dynamic-css-order.test.ts remains in the unit exclude (vite.config.ts:196) and integration include (vite.config.ts:266). Confirmed:
--project unit→No test files found(excluded)--project integration→2 passed | 6 skipped(browser tests skip viait.runIf(HAS_CHROMIUM); build + manifest-order assertions run and pass)
No blocking issues remain.
Prior non-blocking items — now addressed
#1 Unit coverage for pure helpers — DONE. New tests/app-css-order-helpers.test.ts (163 lines, 9 tests, all passing in the unit project, browserless) now covers reorderClientReferenceCss, normalizeRscAssetsManifestCssOrderSource (including the runtime-expression no-op path), addRscCssResourceCrossOrigin, inlineStyleCoversStylesheetHref, removeStylesheetLinksCoveredByInlineCss, dedupeGlobalCssOwnerStylesheetLinks, and the browserless installGlobalCssOwnerStylesheetDedupe guard.
#2 Brittle crossorigin string patch — DONE. rsc-css-resource-crossorigin.ts replaced the exact-string code.replace('"data-rsc-css-href": href', ...) with a whitespace/quote/identifier-tolerant regex (RESOURCE_LINK_HREF_RE), and now throws loudly ("changed its CSS resource module shape") instead of silently no-oping when the token is absent. A test pins both the match and the throw. Confirmed the regex still matches plugin-rsc's current output at plugin-Cbs9j6lP.js:2072.
#3 reorderClientReferenceCss edge cases — DONE. Rewritten to a segment-based matcher: it walks css left-to-right, matches each client chunk's ownFirst sequence, and resolves against the exact matched segment via a positional hrefByFile map (rsc-css-order.ts:88-91) rather than a whole-list find. This eliminates the first-match-only slice, the endsWith cross-match, and the base-less ?? file fallback that the prior reviews flagged. Because reordering is a positional permutation within a matched segment, no wrong/base-less href can be emitted — worst case is a benign reorder of a coincidentally-matching run.
#4 Unguarded top-level installGlobalCssOwnerStylesheetDedupe() — DONE. Now guarded with typeof document === "undefined" || typeof MutationObserver === "undefined" (app-inline-css-client.ts:87) plus the existing idempotency symbol. A test asserts it's a no-op outside a browser.
#5 Dead-code-looking globalThis[Symbol.for(...)] marker — DONE. Now preceded by // Keep this module as the stylesheet's stable chunk owner. (app-global-css-owner.ts:103,105).
#6 computeDynamicImportPreloads global ordering flip. Unchanged and confirmed intentional — collectStaticChunkFiles emits JS-first, then imported chunks, then own CSS, with an explanatory comment (lazy-chunks.ts:112-125) matching react-loadable/Vite preload semantics. The build-optimization.test.ts:1655 snapshot reflects it. This is the correct execution order; browsers/React dedupe preloads, so the global change is safe.
Remaining (minor, non-blocking)
buildApp()returnsPromise.resolve()but does synchronous fs I/O (index.ts:6920-6937). Correct as written (the sync work completes before the resolved promise), but thePromise.resolve()wrapper is cosmetic — anasynchook with noawait, or dropping the wrapper, would read more clearly. Not a bug.normalizeRscAssetsManifestCssOrderSourcestill fails-safe (no-op) underrenderBuiltUrl/asset-prefix/CDN manifests because those emit non-JSON runtime expressions (now covered by a test asserting the verbatim pass-through). The CSS-order fix won't apply for those deployments; a tracked follow-up would be worthwhile, but the fail-safe behavior is correct.
Verdict
The core approach (stable global-CSS ownership, SCC-aware graph-order reconciliation, segment-based manifest reordering, browser-side dedup) is sound. vp check passes, the new unit tests pass in the unit project, and the integration test passes serially in the integration project. All prior blocking and non-blocking items are resolved or reduced to cosmetic notes. LGTM — mergeable.
|
Review posted to PR #2879. SummaryRe-reviewed at the latest commit Previously-blocking (test placement): still RESOLVED. Verified empirically — the build+preview test runs only in the serial Prior non-blocking items — now fixed by the new commit:
Remaining (cosmetic only): the Verdict: LGTM — mergeable. Core approach is sound, |
|
NO FINDINGS — independent closure review of exact head I reviewed the complete diff against Validation on this exact SHA:
No actionable correctness, parity, or regression gaps found. |
1 participant
Summary
Fixes the four non-deferred
next-dynamic-cssfailures from Actions run 31439707085, job 93624401572:should have correct order of styles between global and css modulesshould have correct order of styles on client component that is sharing styles with next/dynamicshould have correct order of styles on next/dynamic loaded componentshould have correct order of global styles between layout and pagesThe implementation gives shared global styles stable build ownership, preserves graph-wide CSS import order across RSC and client-reference manifests, reconciles late dynamic-owner duplicates, and keeps React stylesheet resource identity compatible across SSR and hydration. CSS-module ordering is derived from resolved targets, including extensionless aliases and transitive JavaScript import boundaries.
Validation
test/e2e/app-dir/next-dynamic-css/next-dynamic-css.test.ts— 4/4 passedvp checkvp run vinext#buildThe fixture intentionally throws during SSR to exercise client recovery, so the upstream test continues to log its expected HTTP 500 while all CSS assertions pass.