Fix CSS module class name divergence with rspack SSR#2489
Fix CSS module class name divergence with rspack SSR#2489
Conversation
…h rspack The server webpack config template had two issues causing CSS module styles to silently break in production with rspack + SSR: 1. CSS extract loader filter didn't handle rspack's cssExtractLoader path, leaving extraction in the server bundle 2. CSS modules options were overwritten instead of merged, losing settings like namedExport, localIdentName, and getLocalIdent Also adds documentation to the rspack migration guide warning about contenthash divergence and providing a getLocalIdent workaround. Fixes #2488 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds deterministic CSS Modules class-name generation for SSR with Rspack (including resourceQuery in hashing), expands server-side CSS loader/plugin filtering for Rspack paths, preserves existing css-loader modules options while enabling exportOnlyLocals, and relaxes generator insertion logic and tests to handle merged/spread module option forms. Changes
Sequence Diagram(s)mermaid Dev->>Client: trigger client build with css-loader modules + getLocalIdent Dev->>Server: trigger server build with css-loader modules + exportOnlyLocals + same getLocalIdent Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (1)
51-60: Consider unifying CSS loader filtering logic.There are two loops that filter CSS-related loaders (lines 51-60 and 141-156). The first loop only checks string items and excludes
mini-css-extract-pluginandcssExtractLoader, while the second loop also handles object items and additionally excludesstyle-loader. Consider consolidating or addingstyle-loaderfiltering to the first loop for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt` around lines 51 - 60, The two filtering loops for CSS loaders are inconsistent; update the first loop (serverWebpackConfig.module.rules.forEach / loader.use.filter) to also remove style-loader and handle object-form loader entries the same way as the second loop: treat items that are strings or objects with a loader property, and filter out entries where the string or item.loader matches /mini-css-extract-plugin/, includes('cssExtractLoader') or includes('style-loader'); alternatively consolidate both loops into a single helper that performs this unified filter and call it for serverWebpackConfig.module.rules to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt`:
- Around line 151-155: The code in serverWebpackConfig.js.tt sets testValue
conditionally and then calls testValue.match(...), which can throw if testValue
is undefined; update the logic in the function/block that computes testValue
(the branch handling item as string/object loader) to ensure testValue is always
a string (e.g., initialize testValue = ''), or use safe guarding like optional
chaining before calling match, so the return expression safely checks
mini-css-extract-plugin and cssExtractLoader/style-loader without risking a
TypeError.
---
Nitpick comments:
In
`@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt`:
- Around line 51-60: The two filtering loops for CSS loaders are inconsistent;
update the first loop (serverWebpackConfig.module.rules.forEach /
loader.use.filter) to also remove style-loader and handle object-form loader
entries the same way as the second loop: treat items that are strings or objects
with a loader property, and filter out entries where the string or item.loader
matches /mini-css-extract-plugin/, includes('cssExtractLoader') or
includes('style-loader'); alternatively consolidate both loops into a single
helper that performs this unified filter and call it for
serverWebpackConfig.module.rules to keep behavior consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/migrating/migrating-from-webpack-to-rspack.mdreact_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
...s/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
Show resolved
Hide resolved
Greptile SummaryThis PR fixes two bugs in the generated
The template changes are minimal and well-targeted. One documentation inconsistency was found: the "correct" code example for preserving CSS modules config uses a stricter guard than the actual generated template. Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[serverWebpackConfig.js.tt<br>configureServer] --> B[First filter pass<br>Lines 51-60]
B --> |"Filter string loaders"| B1{Is loader string?}
B1 --> |Yes| B2{Matches mini-css-extract-plugin<br>OR cssExtractLoader?}
B2 --> |Yes| B3[Remove loader]
B2 --> |No| B4[Keep loader]
B1 --> |No| B4
A --> C[Second filter pass<br>Lines 141-156]
C --> |"Filter string & object loaders"| C1{Get testValue from<br>item or item.loader}
C1 --> C2{Matches mini-css-extract-plugin<br>OR cssExtractLoader<br>OR style-loader?}
C2 --> |Yes| C3[Remove loader]
C2 --> |No| C4[Keep loader]
C --> D[CSS modules config<br>Lines 168-173]
D --> D1{cssLoader &&<br>cssLoader.options?}
D1 --> |Yes| D2["Spread existing modules opts<br>+ exportOnlyLocals: true"]
D1 --> |No| D3[Skip]
style B3 fill:#f66,color:#fff
style C3 fill:#f66,color:#fff
style D2 fill:#6f6,color:#000
Last reviewed commit: 2714bc0 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
The "correct" example here adds an extra guard ( Consider aligning the doc with the template: |
|
test comment |
PR Review: Fix CSS module class name divergence with rspack SSRThe two template bug fixes are correct and necessary. The documentation additions are clear and well-structured. A few issues worth addressing: Issues1. Potential TypeError in existing filter logic (not introduced here, but worth noting) In the second rule.use.filter() callback (the one that also handles style-loader), testValue can be undefined if a loader item is neither a bare string nor an object with a string .loader property. Both the old testValue.match(...) and the new testValue.includes(...) will throw TypeError: Cannot read properties of undefined. The docs example wisely uses testValue?.match(...) and testValue?.includes(...) with optional chaining -- the template should match that to be safe. 2. Guard condition gap in CSS modules spread fix The guard in the template is if (cssLoader and cssLoader.options), but it does not check whether cssLoader.options.modules is actually truthy. When CSS modules are disabled via modules: false, the spread produces { exportOnlyLocals: true }, which could unexpectedly re-enable CSS modules for loaders that had them disabled. The docs example correctly uses the stricter guard: if (cssLoader and cssLoader.options and cssLoader.options.modules). 3. Hash collision risk in the getLocalIdent example .digest(base64url).slice(0, 3) is only 18 bits of entropy (64^3 = 262,144 values). With a large application CSS module classes, birthday-paradox collisions become likely (~2% with 100 classes, ~40% with 500 classes). A collision means two different CSS classes silently share the same generated identifier - the exact silent breakage this PR is trying to prevent. Increasing to .slice(0, 5) (30 bits, ~1 billion values) would make collisions practically impossible. 4. No separator in hash input relativePath + localName concatenates without a separator. The string for path foo/bar plus class Baz produces the same bytes as path foo/barB plus class az. Adding a null-byte separator eliminates this theoretical edge case. 5. Hardcoded directory depth for project root path.resolve(__dirname, ../..) assumes getLocalIdent.js will always live exactly 2 levels deep from the project root (i.e., config/webpack/). Apps with non-standard config layouts will silently compute wrong relative paths, producing different hashes between client and server. A comment noting this assumption would help users who copy the snippet. Summary
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .createHash('md5') | ||
| .update(relativePath + localName) | ||
| .digest('base64url') | ||
| .slice(0, 3); |
There was a problem hiding this comment.
Hash collision risk: .slice(0, 3) gives only 18 bits of entropy (262,144 possible values). With ~500 CSS module classes across a project, birthday-paradox collision probability approaches 40%, causing different class names to silently share the same generated identifier. Consider .slice(0, 5) (30 bits, ~1 billion values) which makes collisions negligible in practice.
There was a problem hiding this comment.
Addressed in 44b2642 — increased hash to .slice(0, 8) (48 bits of entropy) which makes collisions negligible even for large projects.
|
|
||
| const hash = crypto | ||
| .createHash('md5') | ||
| .update(relativePath + localName) |
There was a problem hiding this comment.
No separator in hash input: relativePath + localName concatenates without a separator. The string for path "foo/bar" + class "Baz" produces the same bytes as path "foo/barB" + class "az". Adding a null-byte separator — e.g., relativePath + '\0' + localName — eliminates this theoretical collision edge case.
There was a problem hiding this comment.
Addressed in 44b2642 — added null-byte separators between all hash input components: relativePath + '\0' + resourceQuery + '\0' + localName.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| cssLoader.options.modules = { | ||
| ...cssLoader.options.modules, // Preserve existing settings (namedExport, localIdentName, getLocalIdent, etc.) | ||
| exportOnlyLocals: true, | ||
| }; |
There was a problem hiding this comment.
Template change breaks Pro upgrade regex pattern match
High Severity
The template now generates a multi-line spread syntax for cssLoader.options.modules, but pro_setup.rb's add_babel_ssr_caller_to_server_config still uses a regex matching the old single-line pattern cssLoader.options.modules = { exportOnlyLocals: true };. When a user generates a non-Pro config and later runs the standalone Pro upgrade, the regex silently fails to match, so the Babel SSR caller code is never inserted. The verify_pro_webpack_transforms method doesn't check for this transform either, making the failure completely silent.
There was a problem hiding this comment.
Addressed across multiple commits:
| @@ -159,7 +166,10 @@ const configureServer = () => { | |||
| return testValue.includes('css-loader'); | |||
| }); | |||
| if (cssLoader && cssLoader.options) { | |||
There was a problem hiding this comment.
Guard condition gap: This guard checks cssLoader.options but not cssLoader.options.modules. When CSS modules are disabled via modules: false, the spread { ...false, exportOnlyLocals: true } evaluates to { exportOnlyLocals: true }, which could unexpectedly re-enable CSS modules for loaders that had them explicitly disabled. The docs example uses the stricter guard: if (cssLoader && cssLoader.options && cssLoader.options.modules) — the template should match.
There was a problem hiding this comment.
Addressed in 99d30fe — the template now uses typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {} in the spread, which safely handles false, true, and string values without re-enabling modules when disabled.
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| cssLoader.options.modules = { | ||
| ...cssLoader.options.modules, // Preserve existing settings (namedExport, localIdentName, getLocalIdent, etc.) |
There was a problem hiding this comment.
This spread is correct for the common case, but if cssLoader.options.modules is false (CSS modules explicitly disabled), { ...false } spreads to {}, and the result { exportOnlyLocals: true } could unexpectedly re-enable CSS modules. The guard on line 168 should also check cssLoader.options.modules is truthy before spreading, consistent with the docs example.
There was a problem hiding this comment.
Addressed in 99d30fe — the spread now uses typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {} which correctly handles false, true, and string values.
- Initialize testValue to '' to prevent TypeError on unexpected loader types - Add cssLoader.options.modules truthiness guard to avoid re-enabling CSS modules when explicitly disabled via modules: false - Update pro_setup.rb regex to match both old single-line and new spread syntax for cssLoader.options.modules - Update generator test fixture to match new template output - Fix getLocalIdent example: increase hash from 3 to 5 chars to reduce collision risk, add null separator between path and name, add comment about directory depth assumption Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Fix CSS module class name divergence with rspack SSRGood fixes overall — the spread-instead-of-overwrite change for A few issues worth addressing before merging: Critical: Behavior Regression in serverWebpackConfig.js.ttThe condition changed from Inconsistency Between Two Filter BlocksThe first filter block (~line 53 of the template) only removes loaders when the item is a plain string. If rspack passes Regex Fragility in pro_setup.rbThe Hash Collision Risk in getLocalIdent SnippetTruncating to 5 characters of base64url gives ~30 bits of entropy. Birthday probability of at least one collision reaches ~5% with 10,000 CSS class names — non-trivial for larger apps. 8 characters (48 bits) drops this to ~0.001%. Invalid CSS Class Names from Digit-Leading File NamesThe See inline comments for specific line references. |
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
Behavior regression: Adding && cssLoader.options.modules to the guard means exportOnlyLocals: true is never set when a project uses css-loader's automatic CSS Modules detection (via *.module.css filename pattern) without an explicit modules: { ... } options object.
The original code correctly applied exportOnlyLocals: true whenever a CSS loader with options was found. The spread can be made safe without the extra guard:
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | |
| if (cssLoader && cssLoader.options) { | |
| cssLoader.options.modules = { | |
| ...(typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {}), | |
| exportOnlyLocals: true, | |
| }; |
This preserves existing module options when they're an object, while still applying exportOnlyLocals: true even when modules wasn't previously configured.
There was a problem hiding this comment.
Addressed in 99d30fe — adopted exactly this approach. The guard checks cssLoader.options.modules (truthy), and the spread uses typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {} to safely handle all value types while still applying exportOnlyLocals: true when modules is enabled.
| if (loader.use && loader.use.filter) { | ||
| loader.use = loader.use.filter( | ||
| (item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)), | ||
| (item) => !(typeof item === 'string' && ( |
There was a problem hiding this comment.
This first filter block only removes loaders when the item is a plain string. If rspack passes cssExtractLoader as an object { loader: '...cssExtractLoader.js' }, it will slip through here. The second filter block at ~line 144 correctly handles both strings and objects — this one should do the same for consistency:
| (item) => !(typeof item === 'string' && ( | |
| loader.use = loader.use.filter((item) => { | |
| let testValue = ''; | |
| if (typeof item === 'string') { | |
| testValue = item; | |
| } else if (item && typeof item.loader === 'string') { | |
| testValue = item.loader; | |
| } | |
| return !( | |
| testValue.match(/mini-css-extract-plugin/) || | |
| testValue.includes('cssExtractLoader') | |
| ); |
There was a problem hiding this comment.
Addressed in 99d30fe — both filter blocks now handle string and object loader forms consistently. The first block (line 53-65) initializes let testValue = '' and checks typeof item.loader === 'string'.
| gsub_file( | ||
| webpack_config, | ||
| /(cssLoader\.options\.modules = \{ exportOnlyLocals: true \};\s*\n\s*\})/, | ||
| /(cssLoader\.options\.modules = \{[^}]*exportOnlyLocals: true[^}]*\};\s*\n\s*\})/, |
There was a problem hiding this comment.
The [^}]* pattern stops at the first } character, so it won't match if the modules object contains nested braces (e.g., a user who has added an inline getLocalIdent function or any other nested value). In that case, gsub_file silently does nothing and the Babel SSR caller is never inserted.
Consider using a lazy multiline match with a line-counting approach, or matching the entire block with a known structure. At minimum, add a post-call check that the expected string was actually inserted — currently there's no way to know if this substitution succeeded or was a no-op:
original = File.read(File.join(destination_root, webpack_config))
gsub_file(
webpack_config,
/(cssLoader\.options\.modules = \{[^}]*exportOnlyLocals: true[^}]*\};\s*\n\s*\})/m,
"\\1#{babel_ssr_code}"
)
new_content = File.read(File.join(destination_root, webpack_config))
unless new_content.include?("babelLoader.options.caller")
say_status :warning, "Could not insert Babel SSR caller — manual edit of #{webpack_config} required", :yellow
endNote the /m flag (Regexp::MULTILINE) which makes . match newlines and allows the pattern to span lines. Though [^}] already matches newlines in Ruby, making intent explicit helps future maintainers.
There was a problem hiding this comment.
Fixed in 74d5cd6. Changes made:
- Regex: Replaced
[^}]*with[\s\S]*?(lazy multiline) so the pattern correctly handles nested braces (e.g., inlinegetLocalIdentfunctions) /mflag: AddedRegexp::MULTILINEfor explicit intent- Post-call check: Added guard clause that warns if the substitution was a no-op, so users know manual editing is needed
RuboCop preferred guard clause style (return if ... include?) over the unless...end block, which works well here.
| .createHash('md5') | ||
| .update(`${relativePath}\0${localName}`) // Null separator prevents path/name collisions | ||
| .digest('base64url') | ||
| .slice(0, 5); |
There was a problem hiding this comment.
5 characters of base64url encoding gives 30 bits of entropy (~1 billion combinations). The birthday-attack collision probability climbs to roughly 5% once a project accumulates ~10,000 distinct CSS class names — a realistic scale for a large production app.
| .slice(0, 5); | |
| .slice(0, 8); |
8 characters (48 bits) brings the same 10,000-class collision probability down to ~0.001%.
There was a problem hiding this comment.
Addressed in 44b2642 — increased to .slice(0, 8) (48 bits) as suggested, bringing collision probability down to ~0.001% even at 10,000 classes.
There was a problem hiding this comment.
Addressed in 44b2642 — increased to .slice(0, 8) (48 bits) as suggested.
Update getLocalIdent to include context.resourceQuery in the hash, which differentiates virtual CSS modules created by CSS-in-JS tools like astroturf via matchResource (!=!) syntax. In rspack, resourcePath points to the source file rather than the virtual path, so without the query string all virtual modules from the same file collide silently. Add new section explaining the CSS-in-JS virtual module collision issue and a troubleshooting entry with detection commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add null-byte separators between hash input components to prevent ambiguous concatenation (e.g., path "foo/bar" + class "Baz" vs path "foo/barB" + class "az") - Increase hash slice from 5 to 8 characters (48 bits of entropy) to keep collision probability negligible even for large projects with ~10,000+ CSS class names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The [^}]* pattern stopped at the first } character, failing silently when the modules object contained nested braces (e.g., an inline getLocalIdent function). Switch to [\s\S]*? lazy multiline matching and add a post-call check that warns if the substitution was a no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justin808
force-pushed
the
jg/2488-fix-rspack-install-docs
branch
from
91a2a83 to
74d5cd6
Compare
PR Review: Fix CSS module class name divergence with rspack SSRThis PR correctly addresses two real bugs in rspack SSR support. The fixes are well-targeted. Positive Highlights
Issues FoundMedium: Unsafe spread when
|
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
The cssLoader.options.modules guard is truthy for any non-falsy value — including true (modules with defaults) and string modes like 'local' or 'pure'. When modules is a string, { ...cssLoader.options.modules } spreads its character indices ({0:'l', 1:'o', ...}) and silently destroys the config.
Consider:
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | |
| if (cssLoader && cssLoader.options && cssLoader.options.modules && typeof cssLoader.options.modules === 'object') { |
This ensures the spread only runs when there's an actual options object to merge into.
There was a problem hiding this comment.
Addressed in 99d30fe — the spread now uses typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {} which is functionally equivalent and handles string values like 'local' safely without needing an extra guard clause.
PR Review: Fix CSS module class name divergence with rspack SSROverall this is a solid, well-motivated fix. The two core code changes (loader filtering and options spread) are correct and the documentation is thorough. A few issues worth addressing: Bug: collision-detection grep pattern does not match getLocalIdent outputThe detection command uses the pattern grep -oP is GNU-only, fails on macOSThe pro_setup.rb regex fragility for customised configsThe updated regex Minor: first loader-filter block has an inconsistent string-only guardLines 51-59 of the template only filter The core bug fixes are correct and well-tested. The documentation improvements are a net positive even with the issues noted above. |
|
|
||
| **Cause:** Rspack sets `context.resourcePath` to the actual source file instead of the virtual matchResource path. All virtual CSS modules from the same source file produce the same hash. | ||
|
|
||
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Virtual Module Collisions](#css-in-js-virtual-module-collisions-astroturf-etc) section above. |
There was a problem hiding this comment.
Broken anchor link: #css-in-js-virtual-module-collisions-astroturf-etc doesn't exist in this document. There is no section heading matching that slug. Based on the intent, this should point to #css-modules-with-ssr-stable-class-names (the getLocalIdent code example section) or remain self-referential to the troubleshooting entry that already explains it.
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Virtual Module Collisions](#css-in-js-virtual-module-collisions-astroturf-etc) section above. | |
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS Modules with SSR: Stable Class Names](#css-modules-with-ssr-stable-class-names) section above. |
There was a problem hiding this comment.
Fixed in 8f9b793 — updated the anchor to #css-in-js-styled-components-all-share-the-same-class-name which matches the actual heading.
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
Edge case: if cssLoader.options.modules is a truthy non-object value (e.g., the string 'local' or 'global'), spreading it will produce incorrect results. { ...'local' } yields { 0: 'l', 1: 'o', 2: 'c', 3: 'a', 4: 'l' } rather than an options object.
While modules: 'local' is uncommon in practice, adding a typeof guard makes the intent explicit and prevents silent breakage:
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | |
| if (cssLoader && cssLoader.options && cssLoader.options.modules && typeof cssLoader.options.modules === 'object') { |
There was a problem hiding this comment.
Addressed in 99d30fe — the spread uses typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {} which safely handles string values like 'local' by spreading {} instead.
| @@ -51,7 +51,10 @@ const configureServer = () => { | |||
| serverWebpackConfig.module.rules.forEach((loader) => { | |||
| if (loader.use && loader.use.filter) { | |||
| loader.use = loader.use.filter( | |||
There was a problem hiding this comment.
This first filter loop (lines 51–60) only tests typeof item === 'string', so it silently skips object-form loader entries like { loader: '...cssExtractLoader...', options: {} }. The second, more complete filter further down (lines 144–156) handles both string and object loaders and is the one that actually does the heavy lifting.
This asymmetry is pre-existing, but since the cssExtractLoader addition targets Rspack and rspack may express that loader as an object (depends on shakapacker's config generation), it's worth verifying empirically that the first loop doesn't need the same object-form check. If it's always a string in rspack's rule set, a comment saying so would prevent future confusion.
There was a problem hiding this comment.
Addressed in 99d30fe — the first filter block now handles both string and object loader forms, matching the second block's behavior. Both initialize let testValue = '' and check for object loaders.
Review: Fix CSS module class name divergence with rspack SSROverall this is a solid, well-motivated fix. The core logic changes are correct and the documentation additions are genuinely useful. A few things to address: Bugs / Correctness1. Broken anchor link in docs (inline comment) — The troubleshooting entry at line 311 references 2. Spread of non-object Minor observations3. Asymmetric filter loops (inline comment) — There are two separate filter passes over 4. MD5 in the 5. 6. Documentation code snippet uses direct mutation — The SummaryThe two bugs (broken anchor and spread-type guard) are worth fixing before merge. The rest are minor polish items. |
Review: Fix CSS module class name divergence with rspack SSRThe fixes in this PR are correct and address real bugs. Two issues worth addressing before merge: Bug: Broken internal anchor link (docs)In the troubleshooting section, line 311 links to Edge case:
|
|
|
||
| **Cause:** Rspack sets `context.resourcePath` to the actual source file instead of the virtual matchResource path. All virtual CSS modules from the same source file produce the same hash. | ||
|
|
||
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Virtual Module Collisions](#css-in-js-virtual-module-collisions-astroturf-etc) section above. |
There was a problem hiding this comment.
Broken anchor — #css-in-js-virtual-module-collisions-astroturf-etc does not exist in this document. The heading that covers this topic is ### CSS-in-JS Styled Components All Share the Same Class Name (a few lines above), which generates the anchor #css-in-js-styled-components-all-share-the-same-class-name. The link silently goes nowhere.
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Virtual Module Collisions](#css-in-js-virtual-module-collisions-astroturf-etc) section above. | |
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Styled Components All Share the Same Class Name](#css-in-js-styled-components-all-share-the-same-class-name) section above. |
There was a problem hiding this comment.
Fixed in 8f9b793 — anchor corrected to #css-in-js-styled-components-all-share-the-same-class-name.
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
When cssLoader.options.modules is a boolean true (the shorthand for enabling CSS Modules with all defaults), the spread silently drops it: { ...true, exportOnlyLocals: true } → { exportOnlyLocals: true }. Functionally fine for SSR since exportOnlyLocals implies modules are active, but worth an explicit guard so the intent is clear to future readers:
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | |
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | |
| // When modules is a plain boolean `true`, spreading it gives {} — that's fine | |
| // because { exportOnlyLocals: true } still enables CSS Modules on the server. | |
| cssLoader.options.modules = { |
There was a problem hiding this comment.
Addressed in 99d30fe — the typeof check in the spread (typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {}) handles true correctly by spreading {}, and { exportOnlyLocals: true } correctly enables CSS Modules for SSR.
| } | ||
| return !(testValue.match(/mini-css-extract-plugin/) || testValue === 'style-loader'); | ||
| return !( | ||
| testValue.match(/mini-css-extract-plugin/) || |
There was a problem hiding this comment.
Minor style nit: testValue.match(/mini-css-extract-plugin/) is inconsistent with the new testValue.includes('cssExtractLoader') on the very next line. Consider making them uniform:
| testValue.match(/mini-css-extract-plugin/) || | |
| testValue.includes('mini-css-extract-plugin') || | |
| testValue.includes('cssExtractLoader') || // Rspack uses this path |
There was a problem hiding this comment.
Fixed in 8f9b793 — both filter blocks now use .includes() consistently instead of mixing .match() and .includes().
| # and new spread syntax patterns). Use [\s\S]*? for lazy multiline matching | ||
| # to support nested braces (e.g., inline getLocalIdent functions). | ||
| gsub_file( | ||
| webpack_config, |
There was a problem hiding this comment.
The updated regex uses [\s\S]*? (lazy dotall) which handles multiline and nested-brace content — a clear improvement over the old [^}]* approach. The subsequent verification step and explicit say_status :warning fallback also give users a clear signal when the transform doesn't apply, which is exactly the right approach for this kind of code-surgery pattern.
One minor note: the /m flag changes ^/$ to match line boundaries, but since the pattern uses [\s\S]*? (not .) the flag doesn't actually change the matching behaviour here. It's harmless, but slightly misleading — could be removed without any functional change.
There was a problem hiding this comment.
Good catch — removed the unnecessary /m flag in 8f9b793 since [\s\S]*? already handles multiline matching without it.
| if (loader.use && loader.use.filter) { | ||
| loader.use = loader.use.filter( | ||
| (item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)), | ||
| (item) => !(typeof item === 'string' && ( |
There was a problem hiding this comment.
This first filter block (lines 51–59) keeps the pre-existing typeof item === 'string' guard, so it only removes string-typed loader references. If rspack's cssExtractLoader ever appears as an object { loader: 'path/to/cssExtractLoader.js' }, it would slip through here.
The second, more comprehensive pass at lines 142–156 already handles both string and object forms correctly, so there's no functional regression. But having two passes with different coverage for the same loader is a latent inconsistency — worth unifying at some point, even if it's pre-existing.
There was a problem hiding this comment.
Addressed in 99d30fe — unified both filter blocks to handle string and object loader forms consistently.
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
The tightened guard (cssLoader.options.modules truthy rather than just cssLoader.options) is the right call — it correctly skips re-enabling CSS modules when they're explicitly disabled via modules: false.
One edge case worth documenting: if cssLoader.options.modules === true (boolean, enabling CSS modules with defaults), spreading it produces { ...true, exportOnlyLocals: true } → { exportOnlyLocals: true }. This is functionally correct (module defaults still apply), but the type change from true to an object is a subtle mutation that could confuse someone reading the resulting config. A comment here would help.
There was a problem hiding this comment.
Addressed in 99d30fe — the guard checks cssLoader.options.modules (truthy), and the typeof check in the spread makes the boolean true behavior explicit and safe. The code is self-documenting with the ternary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/migrating/migrating-from-webpack-to-rspack.md`:
- Line 311: The in-page anchor fragment
"#css-in-js-virtual-module-collisions-astroturf-etc" is dead; update the link in
the sentence "See the [CSS-in-JS Virtual Module
Collisions](`#css-in-js-virtual-module-collisions-astroturf-etc`) section above."
to point to the actual heading slug used in this document (or rename the target
heading to match this slug). Locate the link text and either replace the
fragment with the correct heading slug for the "CSS-in-JS Virtual Module
Collisions" section or adjust that section's heading so its generated anchor
matches this fragment (ensure the string in the link and the heading slug are
identical).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/migrating/migrating-from-webpack-to-rspack.mdreact_on_rails/lib/generators/react_on_rails/pro_setup.rb
- Make first CSS extract filter handle both string and object loader items, consistent with the second filter block (fixes rspack object loader items slipping through) - Use typeof guard on cssLoader.options.modules spread to safely handle non-object values (true, 'local', 'global') that would produce garbage when spread - Update generator spec helper to match template changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (3)
151-162: Additem &&guard for consistency with line 57.Line 154 accesses
item.loaderwithout first checking ifitemis truthy. While the earlier filtering block (line 57) defensively usesitem && typeof item.loader === 'string', this block doesn't. For consistency and defensive coding:♻️ Proposed fix
rule.use = rule.use.filter((item) => { let testValue = ''; if (typeof item === 'string') { testValue = item; - } else if (typeof item.loader === 'string') { + } else if (item && typeof item.loader === 'string') { testValue = item.loader; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt` around lines 151 - 162, The filter callback that sets testValue currently checks typeof item.loader without guarding item; update the conditional in the block that assigns testValue (inside the filter callback where testValue is initialized) to use "item && typeof item.loader === 'string'" so it mirrors the earlier defensive check and avoids accessing item.loader when item is falsy.
163-173: Same inconsistency: additem &&guard at line 168.For the same reason as above, line 168 should defensively check
itembefore accessingitem.loader:♻️ Proposed fix
const cssLoader = rule.use.find((item) => { let testValue = ''; if (typeof item === 'string') { testValue = item; - } else if (typeof item.loader === 'string') { + } else if (item && typeof item.loader === 'string') { testValue = item.loader; } return testValue.includes('css-loader'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt` around lines 163 - 173, The null/undefined access risk in the rule.use.find callback should be fixed by guarding the second branch before reading item.loader: inside the cssLoader lookup (the rule.use.find callback that currently checks "if (typeof item === 'string') ... else if (typeof item.loader === 'string')"), change the conditional to verify item exists (e.g., "else if (item && typeof item.loader === 'string')") so you don't access loader on a null/undefined item; adjust only the check in the cssLoader finder (the anonymous function passed to rule.use.find) to add the item && guard.
51-66: Consider consolidating duplicate filtering logic.The filtering at lines 51-66 removes
mini-css-extract-pluginandcssExtractLoader, while lines 147-162 removes the same loaders plusstyle-loader. Since both iterate over the sameserverWebpackConfig.module.rulesarray, this could be consolidated into a single pass at lines 147-162 (which already handles the superset of cases).If there's a specific reason for the two-phase filtering (e.g., plugin removal at line 135 needs to happen between them), a clarifying comment would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt` around lines 51 - 66, The duplicate filtering of loaders on serverWebpackConfig.module.rules (the earlier serverWebpackConfig.module.rules.forEach block that removes mini-css-extract-plugin and cssExtractLoader) should be consolidated into the later single pass that already removes the superset (mini-css-extract-plugin, cssExtractLoader and style-loader); remove the earlier forEach or merge its logic into the later pass to avoid iterating twice, and if the intermediate plugin removal (the code that removes the mini-css-extract-plugin instance) must occur between these steps, add a short clarifying comment referencing that plugin-removal block and why a two-phase approach is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt`:
- Around line 151-162: The filter callback that sets testValue currently checks
typeof item.loader without guarding item; update the conditional in the block
that assigns testValue (inside the filter callback where testValue is
initialized) to use "item && typeof item.loader === 'string'" so it mirrors the
earlier defensive check and avoids accessing item.loader when item is falsy.
- Around line 163-173: The null/undefined access risk in the rule.use.find
callback should be fixed by guarding the second branch before reading
item.loader: inside the cssLoader lookup (the rule.use.find callback that
currently checks "if (typeof item === 'string') ... else if (typeof item.loader
=== 'string')"), change the conditional to verify item exists (e.g., "else if
(item && typeof item.loader === 'string')") so you don't access loader on a
null/undefined item; adjust only the check in the cssLoader finder (the
anonymous function passed to rule.use.find) to add the item && guard.
- Around line 51-66: The duplicate filtering of loaders on
serverWebpackConfig.module.rules (the earlier
serverWebpackConfig.module.rules.forEach block that removes
mini-css-extract-plugin and cssExtractLoader) should be consolidated into the
later single pass that already removes the superset (mini-css-extract-plugin,
cssExtractLoader and style-loader); remove the earlier forEach or merge its
logic into the later pass to avoid iterating twice, and if the intermediate
plugin removal (the code that removes the mini-css-extract-plugin instance) must
occur between these steps, add a short clarifying comment referencing that
plugin-removal block and why a two-phase approach is required.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.ttreact_on_rails/spec/react_on_rails/support/generator_spec_helper.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/spec/react_on_rails/support/generator_spec_helper.rb
Code ReviewOverall, this PR correctly addresses two real bugs affecting Rspack + SSR setups:
The fixes are sound and the documentation additions are valuable. A few things worth reviewing: 1. Plugin filter still misses Rspack's CSS extract pluginThe loader filter (the focus of this PR) is now correctly handling both bundlers. But the plugin filter in the same file still only removes serverWebpackConfig.plugins = serverWebpackConfig.plugins.filter(
(plugin) =>
plugin.constructor.name \!== 'WebpackAssetsManifest' &&
plugin.constructor.name \!== 'MiniCssExtractPlugin' && // webpack only
plugin.constructor.name \!== 'ForkTsCheckerWebpackPlugin',
// missing: 'CssExtractRspackPlugin' for Rspack
);In Rspack + Shakapacker, the CSS extraction plugin registers as 2. Behavior change:
|
| }); | ||
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { |
There was a problem hiding this comment.
This guard change is correct — the old if (cssLoader && cssLoader.options) would inject exportOnlyLocals: true on any css-loader entry, erroneously enabling CSS Modules for plain (non-modules) CSS files. The new check is safe.
One edge case worth confirming: when cssLoader.options.modules is the boolean true (rather than an object), the spread ...(typeof cssLoader.options.modules === 'object' ? cssLoader.options.modules : {}) correctly spreads {} and then adds exportOnlyLocals: true. That path looks good.
There was a problem hiding this comment.
Thanks for confirming — the typeof check in the spread handles the boolean true edge case correctly as you noted.
| gsub_file( | ||
| webpack_config, | ||
| /(cssLoader\.options\.modules = \{ exportOnlyLocals: true \};\s*\n\s*\})/, | ||
| /(cssLoader\.options\.modules = \{[\s\S]*?exportOnlyLocals: true[\s\S]*?\};\s*\n\s*\})/m, |
There was a problem hiding this comment.
The /m flag in Ruby makes . match newlines, but this pattern only uses [\s\S] (not .), so the flag is a no-op. It can be safely removed.
Also a theoretical edge case: the lazy [\s\S]*? after exportOnlyLocals: true stops at the earliest };\n }. If a user's config has an inline variable with }; before the object closes (e.g., const x = { foo: 1 }; inside getLocalIdent), the match could terminate early and produce a partial replacement. The post-insertion check with say_status :warning is a good fallback — just flagging for awareness.
| /(cssLoader\.options\.modules = \{[\s\S]*?exportOnlyLocals: true[\s\S]*?\};\s*\n\s*\})/m, | |
| /(cssLoader\.options\.modules = \{[\s\S]*?exportOnlyLocals: true[\s\S]*?\};\s*\n\s*\})/, |
There was a problem hiding this comment.
Removed the /m flag in 8f9b793. Good point about the theoretical early-termination edge case — the post-insertion check with say_status :warning provides the safety net for that scenario.
| .slice(0, 8); | ||
|
|
||
| const basename = path.basename(resourcePath); | ||
| const name = basename.replace(/\.(module\.)?(scss|sass|css|tsx?|jsx?)$/, '').replace(/-styles$/, ''); |
There was a problem hiding this comment.
The extension-stripping regex doesn't cover .less or .styl/.stylus. Users with Less or Stylus CSS modules would get class names like Button.less-primary_a1b2c3d4 instead of Button-primary_a1b2c3d4.
| const name = basename.replace(/\.(module\.)?(scss|sass|css|tsx?|jsx?)$/, '').replace(/-styles$/, ''); | |
| const name = basename.replace(/\.(module\.)?(scss|sass|css|less|styl|tsx?|jsx?)$/, '').replace(/-styles$/, ''); |
There was a problem hiding this comment.
Fixed in 8f9b793 — added .less and .styl to the extension-stripping regex.
… consistency - Fix broken anchor link #css-in-js-virtual-module-collisions-astroturf-etc to point to actual heading #css-in-js-styled-components-all-share-the-same-class-name - Add .less and .styl extensions to getLocalIdent basename regex - Use .includes() consistently instead of mixing .match() and .includes() - Remove unnecessary /m flag from pro_setup.rb regex ([\s\S] already matches newlines) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewOverall, the fixes here are sound — the core logic for spreading CSS modules options instead of overwriting and detecting Rspack's Bugs1. Self-referencing documentation link (line 313) See the [CSS-in-JS Styled Components All Share the Same Class Name](#css-in-js-styled-components-all-share-the-same-class-name) section above.The anchor should point to 2. Inconsistent null guard in loader filter (template lines 154, 168) } else if (item && typeof item.loader === 'string') {But the second filter (line 154) and the } else if (typeof item.loader === 'string') { // crashes if item is nullIf any loader entry is Test Gap3. Spec helper doesn't cover the new Minor Notes
|
|
|
||
| **Cause:** Rspack sets `context.resourcePath` to the actual source file instead of the virtual matchResource path. All virtual CSS modules from the same source file produce the same hash. | ||
|
|
||
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Styled Components All Share the Same Class Name](#css-in-js-styled-components-all-share-the-same-class-name) section above. |
There was a problem hiding this comment.
Self-referencing link — this anchor points back to the current section, so clicking it does nothing useful.
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Styled Components All Share the Same Class Name](#css-in-js-styled-components-all-share-the-same-class-name) section above. | |
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS Modules with SSR: Stable Class Names](#css-modules-with-ssr-stable-class-names) section above for the full `getLocalIdent` implementation. |
| let testValue = ''; | ||
| if (typeof item === 'string') { | ||
| testValue = item; | ||
| } else if (typeof item.loader === 'string') { |
There was a problem hiding this comment.
Missing item && guard here. The first filter block at line 57 correctly uses item && typeof item.loader, but this one and the find below (line 168) do not. If a loader entry is null or an object without a loader key, item.loader will throw.
| } else if (typeof item.loader === 'string') { | |
| } else if (item && typeof item.loader === 'string') { |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (1)
51-66: Consider consolidating duplicate CSS loader filtering.The same loaders (
mini-css-extract-plugin,cssExtractLoader) are filtered in two separate loops — lines 51-66 filter rules early, then lines 147-162 filter again (withstyle-loaderadded). This duplication could be consolidated into the second, more comprehensive filter block.Not blocking since this is a generator template where robustness may be preferred over DRY.
Also applies to: 147-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt` around lines 51 - 66, The duplicate CSS loader-filtering logic should be consolidated: remove or replace the earlier serverWebpackConfig.module.rules.forEach(...) loop that filters loader.use based on testValue.includes('mini-css-extract-plugin') || testValue.includes('cssExtractLoader') and rely on the later, comprehensive filter block (the one that also filters 'style-loader'); alternatively, merge the conditions so there is a single loop that checks loader.use.filter and excludes 'mini-css-extract-plugin', 'cssExtractLoader', and 'style-loader' by examining item strings or item.loader (the same testValue logic) to avoid redundant processing while preserving the more comprehensive filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/migrating/migrating-from-webpack-to-rspack.md`:
- Around line 303-313: The self-referential anchor currently pointing to
"#css-in-js-styled-components-all-share-the-same-class-name" should be changed
to the CSS Modules section that contains the getLocalIdent example so readers
are directed to the actual solution; update the link target to the anchor for
the CSS Modules/getLocalIdent section (the section that demonstrates including
context.resourceQuery and the getLocalIdent snippet) so the reference correctly
points to the getLocalIdent solution.
---
Nitpick comments:
In
`@react_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt`:
- Around line 51-66: The duplicate CSS loader-filtering logic should be
consolidated: remove or replace the earlier
serverWebpackConfig.module.rules.forEach(...) loop that filters loader.use based
on testValue.includes('mini-css-extract-plugin') ||
testValue.includes('cssExtractLoader') and rely on the later, comprehensive
filter block (the one that also filters 'style-loader'); alternatively, merge
the conditions so there is a single loop that checks loader.use.filter and
excludes 'mini-css-extract-plugin', 'cssExtractLoader', and 'style-loader' by
examining item strings or item.loader (the same testValue logic) to avoid
redundant processing while preserving the more comprehensive filtering behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/migrating/migrating-from-webpack-to-rspack.mdreact_on_rails/lib/generators/react_on_rails/pro_setup.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
| ### CSS-in-JS Styled Components All Share the Same Class Name | ||
|
|
||
| **Symptoms:** | ||
|
|
||
| - Multiple astroturf (or similar CSS-in-JS) styled components in the same file all get identical CSS class names | ||
| - Styles look correct in development but are broken in production | ||
| - Only one component's styles survive; others are overwritten | ||
|
|
||
| **Cause:** Rspack sets `context.resourcePath` to the actual source file instead of the virtual matchResource path. All virtual CSS modules from the same source file produce the same hash. | ||
|
|
||
| **Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Styled Components All Share the Same Class Name](#css-in-js-styled-components-all-share-the-same-class-name) section above. |
There was a problem hiding this comment.
Self-referential link — should point to the solution section.
Line 313 links to #css-in-js-styled-components-all-share-the-same-class-name, which is the current section itself. The link should point to the CSS Modules section where the getLocalIdent code is provided.
📝 Suggested fix
-**Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS-in-JS Styled Components All Share the Same Class Name](`#css-in-js-styled-components-all-share-the-same-class-name`) section above.
+**Solution:** Include `context.resourceQuery` in the `getLocalIdent` hash. See the [CSS Modules with SSR: Stable Class Names](`#css-modules-with-ssr-stable-class-names`) section above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/migrating/migrating-from-webpack-to-rspack.md` around lines 303 - 313,
The self-referential anchor currently pointing to
"#css-in-js-styled-components-all-share-the-same-class-name" should be changed
to the CSS Modules section that contains the getLocalIdent example so readers
are directed to the actual solution; update the link target to the anchor for
the CSS Modules/getLocalIdent section (the section that demonstrates including
context.resourceQuery and the getLocalIdent snippet) so the reference correctly
points to the getLocalIdent solution.
| if (cssLoader && cssLoader.options) { | ||
| cssLoader.options.modules = { exportOnlyLocals: true }; | ||
| if (cssLoader && cssLoader.options && cssLoader.options.modules) { | ||
| cssLoader.options.modules = { |
There was a problem hiding this comment.
The find callback a few lines above (the cssLoader one) also uses typeof item.loader === 'string' without a prior item && guard, which is inconsistent with the first filter block at line 57 that does include item &&. If rule.use ever contains a null entry the callback will throw. Worth applying the same null guard there for consistency and safety.
…ial link - Add `item &&` null guard before accessing `item.loader` in all serverWebpackConfig files to prevent "Cannot read properties of undefined" when rule.use contains undefined/null entries - Initialize testValue to empty string to prevent undefined.includes() - Add Array.isArray guard to extractLoader in pro dummy - Fix self-referential troubleshooting link in rspack migration docs to point to the getLocalIdent implementation section instead Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review SummaryThis PR correctly addresses two real bugs in the rspack SSR setup and the documentation is thorough. The core fixes are sound. A few issues worth addressing before merge: Bugs / Inconsistencies1. Spec dummy files not updated with the CSS modules spread fix Both if (cssLoader && cssLoader.options) {
cssLoader.options.modules = { exportOnlyLocals: true };
}The template and 2.
Minor issues3. The new pattern 4. Documentation example hardcodes project-specific CSS modules options In the usage snippet: cssLoader.options.modules.namedExport = false;
cssLoader.options.modules.exportLocalsConvention = 'camelCase';These values are specific to one project's configuration. Copying them blindly could silently change CSS export behavior (e.g. Positive notes
|
| gsub_file( | ||
| webpack_config, | ||
| /(cssLoader\.options\.modules = \{ exportOnlyLocals: true \};\s*\n\s*\})/, | ||
| /(cssLoader\.options\.modules = \{[\s\S]*?exportOnlyLocals: true[\s\S]*?\};\s*\n\s*\})/, |
There was a problem hiding this comment.
The lazy [\s\S]*? matching will correctly handle the multiline spread pattern, but the tail \};\s*\n\s*\} requires that:
- The modules assignment ends with
};(no trailing comma before the}) - A newline immediately follows before the closing
}of theifblock
Prettier-formatted files typically produce }, (trailing comma) on the last property, not };. If a user's config has been auto-formatted, this could fail to match even the standard template output — the added say_status :warning will surface it, but it's worth a comment explaining why formatting divergence triggers the fallback.
| } | ||
| return !( | ||
| testValue.includes('mini-css-extract-plugin') || | ||
| testValue.includes('cssExtractLoader') // Rspack uses this path |
There was a problem hiding this comment.
cssExtractLoader is an internal rspack implementation detail (@rspack/core/dist/cssExtractLoader.js). If rspack moves or renames this file in a future version the filter silently stops working and CSS extraction leaks back into the server bundle. Worth adding a comment with the full known path and a note to check it on rspack upgrades:
| testValue.includes('cssExtractLoader') // Rspack uses this path | |
| testValue.includes('mini-css-extract-plugin') || | |
| // Rspack's internal CSS extract loader - verify path on rspack upgrades: | |
| // @rspack/core/dist/cssExtractLoader.js | |
| testValue.includes('cssExtractLoader') // Rspack uses this path |
|
|
||
| // In the CSS modules configuration loop: | ||
| if (cssLoader?.options?.modules) { | ||
| cssLoader.options.modules.namedExport = false; |
There was a problem hiding this comment.
These two lines hardcode project-specific CSS module conventions. namedExport: false and exportLocalsConvention: 'camelCase' are not universally correct defaults — users who rely on named exports or a different convention will silently regress if they copy this snippet verbatim. Consider removing them from the example entirely (they're unrelated to the getLocalIdent fix) or adding a comment like // adjust to match your existing CSS modules settings.
| if (!Array.isArray(rule.use)) return null; | ||
| return rule.use.find((item) => { | ||
| let testValue; | ||
| let testValue = ''; |
There was a problem hiding this comment.
The extractLoader function here still accesses item.loader without a null guard (item &&), unlike the updated version in the template and all the other touched files. If item is null or undefined in the use array this would throw.
| let testValue = ''; | |
| let testValue = ''; | |
| if (typeof item === 'string') { | |
| testValue = item; | |
| } else if (item && typeof item.loader === 'string') { | |
| testValue = item.loader; | |
| } |
…cases Document two rspack migration pitfalls discovered during Academia's re-land: UMD libraries (lodash/underscore) behaving differently under rspack's module interop, and modern entry points crashing on globals only exposed in legacy bundles. Kept in the Troubleshooting section to avoid distracting from the main migration flow. Closes #2488 (additional context) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
cssExtractLoaderpath when filtering CSS extract loaders for SSRexportOnlyLocals: true[contenthash]divergence and providing agetLocalIdentworkaroundDetails
The generated
serverWebpackConfig.jshad two bugs when used with rspack:mini-css-extract-plugin(webpack) but notcssExtractLoader(rspack), leaving CSS extraction in the server bundlecssLoader.options.modules = { exportOnlyLocals: true }destroyed all existing settings likenamedExport,localIdentName, andgetLocalIdent. Changed to use spread:{ ...cssLoader.options.modules, exportOnlyLocals: true }The documentation adds a new section explaining why
[contenthash]inlocalIdentNamecauses silent style breakage with rspack SSR, and provides a stablegetLocalIdentfunction as the fix.Fixes #2488
Test plan
serverWebpackConfig.jswith--rspackflag🤖 Generated with Claude Code
Note
Medium Risk
Touches SSR webpack generator logic and CSS module configuration, which can impact production rendering/styling if the loader filtering or module option merge behaves unexpectedly across bundler versions.
Overview
Fixes the generated
serverWebpackConfig.jstemplate for Rspack SSR by filtering out Rspack’scssExtractLoader(in addition tomini-css-extract-plugin/style-loader) so CSS extraction doesn’t leak into the server bundle.Adjusts SSR CSS Modules handling to preserve existing
css-loadermodule options when enablingexportOnlyLocals: true(spreading instead of overwriting), and expands the Rspack migration guide with a warning that[contenthash]-basedlocalIdentNamecan break production SSR styles plus a stablegetLocalIdentworkaround.Written by Cursor Bugbot for commit 2714bc0. Configure here.
Summary by CodeRabbit
Documentation
Configuration