Recover transitive CSS-only split chunk styles#184
Conversation
WalkthroughBoth the rspack and webpack RSC plugins now compute a chunk-group usage count and per-module chunk sets to recover CSS transitively imported by non-client-reference child modules when SplitChunks cache groups move that CSS into CSS-only split chunks. New fixtures and integration tests validate the fix; a changelog entry documents it. ChangesTransitive CSS recovery fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler as Webpack/Rspack Compilation
participant Plugin as RSC Plugin (buildManifest)
participant ChunkGraph as Chunk/Module Graph
participant Manifest as Client/Server Manifest
Compiler->>Plugin: emit compilation with chunk groups
Plugin->>ChunkGraph: compute chunkGroupUseCount per chunk
Plugin->>ChunkGraph: compute moduleChunks for reference module
Plugin->>ChunkGraph: traverse outgoing connections (child modules)
ChunkGraph-->>Plugin: candidate non-style child module
Plugin->>Plugin: belongsToReferenceChunkGroup(child, moduleChunks, chunkGroupUseCount)
alt shares chunk or uniquely used
Plugin->>ChunkGraph: addDirectStyleImports(child)
ChunkGraph-->>Plugin: recovered CSS from css/mini-extract module
Plugin->>Manifest: record CSS on reference entry
else shared elsewhere
Plugin->>Plugin: skip CSS to avoid contamination
end
Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c9105d2 to
032af63
Compare
Greptile SummaryThis PR recovers RSC stylesheet hints for CSS-only split chunks. The main changes are:
Confidence Score: 4/5The direct child CSS recovery works for the tested shape, but nested child-module CSS can still be omitted.
src/webpack/RSCWebpackPlugin.ts and src/react-server-dom-rspack/plugin.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client reference] --> B[Direct CSS import]
A --> C[Local child module]
C --> D[Child direct CSS import]
C -. not visited .-> E[Nested child module]
E --> F[Nested CSS-only split chunk]
B --> G[RSC manifest CSS]
D --> G
F -. missing hint .-> G
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Client reference] --> B[Direct CSS import]
A --> C[Local child module]
C --> D[Child direct CSS import]
C -. not visited .-> E[Nested child module]
E --> F[Nested CSS-only split chunk]
B --> G[RSC manifest CSS]
D --> G
F -. missing hint .-> G
Reviews (1): Last reviewed commit: "Recover transitive CSS split chunks" | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9105d266a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review triage note:
qa-evidence v1 |
032af63 to
03b7585
Compare
|
Review triage update:
qa-evidence v1 |
Review summaryThis PR extends the #112 sibling-chunk CSS recovery to follow one non-style child hop, so a client reference's local child component (e.g. Findings posted inline:
Minor, non-blocking: No issues found with null-guards, the one-hop bound (applied consistently in both files), CSS dedup, or the |
03b7585 to
fe9ad50
Compare
|
Review triage update for head
qa-evidence v1 |
Review summaryThis PR extends the RSC webpack/rspack plugins' CSS sibling-chunk recovery to follow one additional non-style hop: when a client reference's own JS chunk lost its CSS to a Verified: the one-hop child-CSS recovery is correctly scoped per client-reference module ( One real issue survived verification:
Minor/non-blocking: the two plugin files now duplicate ~40 nearly-identical lines of new traversal logic ( |
fe9ad50 to
88f7bf0
Compare
|
Final review triage update for head
qa-evidence v1 |
|
Final thread triage for head
qa-evidence v1 |
Review summaryThis PR extends the existing sibling-chunk CSS recovery ( Correctness: I traced the new Lower-severity notes (posted inline):
Nothing here should block the fix from landing; the notes are about durability/maintainability of the approach. |
| // into a CSS-only chunk). Follow the module's DIRECT `.css` | ||
| // imports to the chunk(s) that carry them, intersected with this | ||
| // chunk group, and merge that CSS. Only direct imports are | ||
| // followed, so CSS reached through a shared dependency (imported by | ||
| // another module, not the reference) is NOT picked up — the #108 | ||
| // broadcast stays fixed. | ||
| // chunk group, and merge that CSS. Also follow one non-style child | ||
| // hop when that child is bundled into one of the reference's own | ||
| // JS chunks, so a local child component's extracted stylesheet is | ||
| // recovered after a CSS-only SplitChunks merge. Shared dependency | ||
| // chunks still do not qualify, keeping the #108 broadcast fixed. |
There was a problem hiding this comment.
This comment only documents the "child bundled into one of the reference's own JS chunks" case (moduleChunks.has(depChunk) in belongsToReferenceChunkGroup below). It doesn't mention the second, less obvious branch:
if (groupChunks.has(depChunk) && (chunkGroupUseCount.get(depChunk) ?? 0) === 1) {
return true;
}That branch is what actually makes the new "keeps child-module CSS when the child is split into its own JS chunk" test pass (Panel.js ends up in its own panel.chunk.js, not the same chunk as Button.js). It's also the subtler half of this fix from a correctness standpoint: it relies on the invariant that a chunk used by exactly one chunk group in the whole compilation is "private" to that reference, which only holds because each client reference gets its own async chunk group. Worth spelling that out here (and the corresponding rationale in src/react-server-dom-rspack/plugin.ts) so a future reader doesn't have to reverse-engineer why the chunkGroupUseCount === 1 check is safe against the #108 broadcast regression.
Review summaryReviewed the transitive/child-module CSS-only SplitChunks recovery for both Correctness: The core mechanism ( Duplication (quality, non-blocking): The new logic ( Minor perf nit: In both files, Tests/fixtures: The new webpack and rspack fixtures and tests mirror each other well and cover both the "child CSS ends up in a CSS-only chunk" and "child is split into its own JS chunk" cases, plus a regression check that the existing shared-dependency exclusion (#108) and the no-warning invariant still hold. No security or blocking correctness issues found. |
88f7bf0 to
6d05731
Compare
Review summaryThis PR adds a bounded "one non-style child hop" to the existing sibling-CSS-only-chunk recovery in both Most significant finding — possible reintroduction of #108 under merged chunk-group topologies ( Secondary finding — efficiency ( Note: I initially (mistakenly) posted the same efficiency concern against the webpack file too, based on a stale local checkout of that file; I've posted a retraction inline once I re-verified against the actual PR diff — the webpack file already hoists this correctly. |
|
Final current-head review triage for
qa-evidence v1 |
Review summaryOverall this is a well-scoped, well-tested fix. I read both plugin diffs in full, cross-checked the added fixtures/tests, and reasoned through the chunk-graph logic by hand (couldn't run What looks solid:
One thing worth double-checking before merge (left as inline comments on both The The existing tests for that fixture (lines 289, 307, 417, 437 in |
Summary
RSCWebpackPluginandRSCRspackPluginwhile keeping the existing group/chunk guard that prevents shared dependency CSS broadcast.Button.js -> Panel.js -> Panel.cssunder astylesCSS-only cache group, plus a webpack server-build regression that proves shared dependency CSS-only chunks still stay excluded.Fixes #180.
Validation
yarn jest tests/webpack-plugin/plugin-integration.test.ts --runInBand -t "transitive CSS"failed becauseButton.jshadcss: []whilestyles.chunk.csswas emitted.yarn jest tests/rspack-plugin/plugin.test.ts --runInBand -t "child-module CSS-only"failed becauseButton.jshadcss: []whilestyles.chunk.csswas emitted.yarn buildpassed.yarn jest tests/webpack-plugin/plugin-integration.test.ts --runInBand -t "shared dependency|transitive CSS|server build".yarn jest tests/rspack-plugin/plugin.test.ts --runInBand -t "child-module CSS-only|child-module CSS when the child is split|CSS-only split chunk styles|scopes manifest CSS|does not attach an importing"..agents/bin/validatepassed:yarn buildplusyarn test(7 RSC suites / 15 tests; 25 non-RSC suites / 259 tests).git diff --check origin/main...HEADandgit diff --checkpassed.Review And Churn Notes
origin/main...HEAD.6d05731.QA Evidence
Summary by CodeRabbit
Bug Fixes
Tests