Fix rspack configuration not applying to all environments #2275
Conversation
## Problem When using `--rspack` flag with the generator or `bin/switch-bundler`, only the `default` section of shakapacker.yml was updated. The environment sections (development, test, production) retained `assets_bundler: webpack` because: 1. `YAML.load` expands `<<: *default` inheritance into explicit copies 2. Modifying `config["default"]` doesn't affect the already-expanded copies 3. `YAML.dump` writes back with all sections having explicit values Additionally, `bin/switch-bundler` crashed with `Psych::AliasesNotEnabled` on YAML files containing anchors/aliases. ## Solution Replace YAML parse/dump approach with regex replacement (Thor's `gsub_file` pattern) which: - Preserves file structure (comments, anchors, aliases) - Updates `assets_bundler` in ALL sections, not just default - Avoids YAML parsing issues entirely This follows the standard Rails generator pattern for modifying config files. ## Files Changed - `base_generator.rb`: Use `gsub_file` instead of YAML.load/dump - `bin/switch-bundler` template: Use regex replacement, remove yaml require 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughThe changes replace YAML-based configuration parsing with string-based text replacements using regex and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (1)
react_on_rails/lib/generators/react_on_rails/base_generator.rb (1)
301-315: LGTM! Regex-based replacement correctly preserves file structure.The regex patterns appropriately match configuration lines while preserving whitespace, comments, and file structure. The approach correctly updates all occurrences across all environment sections (not just
default), which fixes the original issue.Minor note: The patterns
["']?webpack["']?and["']?babel["']?match quotes independently, which could theoretically match mismatched quotes like"webpack'. For more robustness, consider paired matching, though the current pattern is adequate for valid YAML.Optional: More robust regex pattern
- /^(\s*assets_bundler:\s*)["']?webpack["']?(\s*(?:#.*)?)$/, + /^(\s*assets_bundler:\s*)(?:"webpack"|'webpack'|webpack)(\s*(?:#.*)?)$/, '\1rspack\2' ) - /^(\s*webpack_loader:\s*)["']?babel["']?(\s*(?:#.*)?)$/, + /^(\s*webpack_loader:\s*)(?:"babel"|'babel'|babel)(\s*(?:#.*)?)$/, '\1swc\2'This explicitly matches paired quotes or no quotes, preventing edge cases with malformed YAML.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: For infrastructure/config changes, perform comprehensive local testing including: finding all affected files with grep, testing build pipeline, running relevant specs, and linting everything before pushing
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: In IDE configuration, exclude these directories to prevent slowdowns: /coverage, /tmp, /gen-examples, /packages/react-on-rails/lib, /node_modules, /react_on_rails/spec/dummy/node_modules, /react_on_rails/spec/dummy/tmp, /react_on_rails/spec/dummy/app/assets/webpack, /react_on_rails/spec/dummy/log, /react_on_rails/spec/dummy/e2e/playwright-report, /react_on_rails/spec/dummy/test-results
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/config/webpack/**/*.js : Use `namedExport: false` and `exportLocalsConvention: 'camelCase'` for CSS Modules loader options in Shakapacker 9.0+ if existing code uses `import styles from './file.module.css'` pattern
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/*.rb : Run `bundle exec rubocop` (MANDATORY) before every commit to ensure zero offenses
Applied to files:
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/*.rb : Run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (3)
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler (1)
50-68: LGTM! Implementation correctly mirrors base_generator.rb approach.The regex-based replacement logic is implemented correctly:
- Dynamic pattern building with controlled string interpolation is safe
- Replacement string escaping (
"\\1#{var}\\2") is correct- File I/O operations are appropriate for a standalone script
- Logic for determining old vs. new bundler/loader values is sound
The implementation maintains consistency with the generator approach while adapting it for a script context.
react_on_rails/lib/generators/react_on_rails/base_generator.rb (2)
295-318: The regex patterns handle various YAML formats correctly.The implementation successfully replaces all occurrences of
webpackwithrspackandbabelwithswcwhile preserving:
- Inline comments
- YAML anchors and aliases
- Quote styles (
"webpack",'webpack',webpack)- Indentation and whitespace variations
295-318: Verify locally by runningbundle exec rubocopon this file before committing.The Ruby file must pass all rubocop checks as per the project's mandatory pre-commit requirements. Note that git pre-commit hooks automatically run RuboCop with auto-fix enabled, but ensure local verification passes first.
PR Review: Fix rspack configuration not applying to all environments✅ Strengths1. Correct Problem DiagnosisThe root cause analysis is excellent. YAML.load expands anchor aliases ( 2. Appropriate SolutionUsing regex replacement (Thor's
3. Well-Crafted RegexThe pattern is robust with proper anchoring, indentation handling, quote flexibility, and comment preservation.
|
Code Review for PR #2275: Fix rspack configuration not applying to all environmentsSummaryThis PR fixes two critical bugs in the Rspack support feature by replacing YAML parse/dump operations with regex replacements. The approach is sound and follows Rails generator patterns. Overall, this is a well-executed fix that addresses real issues, but there are some considerations around testing and edge cases. ✅ Strengths
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐ (4/5) | Clean, follows Rails patterns |
| Correctness | ⭐⭐⭐⭐⭐ (5/5) | Fixes both reported bugs correctly |
| Test Coverage | ⭐⭐⭐ (3/5) | Missing automated tests for environments |
| Documentation | ⭐⭐⭐⭐ (4/5) | Good PR description, missing CHANGELOG |
Overall: 4.3/5 — Solid fix with minor improvements needed
✅ Action Items Before Merge
Must fix:
- ❗ Add CHANGELOG.md entry (user-visible bug fix per CLAUDE.md)
Should add:
2.
3.
Nice to have:
4. 💡 Make bin/switch-bundler more robust by matching any bundler value
5. 💡 Add example comment showing what the regex matches
📋 Recommendation
Approve with requested changes ✅
The core fix is correct and well-implemented. The bugs are real, the solution is appropriate, and the code follows Rails patterns.
Great work identifying and fixing these bugs! The regex approach is the right solution for preserving YAML structure.
Code Review - PR #2275Thank you for this fix! I've reviewed the changes and they effectively address the reported bugs. Here's my detailed feedback: ✅ Strengths1. Root Cause Analysis
2. Solution Design
3. Code Quality
4. Testing Documentation
🔍 Observations & Suggestions1. Regex Pattern Edge CasesThe current regex patterns work well for standard cases: /^(\s*assets_bundler:\s*)["']?webpack["']?(\s*(?:#.*)?)$/Potential edge cases to consider:
Recommendation: These edge cases are unlikely in real-world Shakapacker configs, but consider adding a comment noting the assumption of block-style YAML formatting. 2. Error Handling in
|
Code Review: Fix rspack configuration not applying to all environmentsGreat work identifying and fixing these bugs! The approach is solid, but I have some important feedback on the implementation. 🟢 Strengths
🔴 Critical Issues1. Regex patterns may not match all valid YAML formatsThe regex patterns assume specific formatting: /^(\s*assets_bundler:\s*)["\']?webpack["\']?(\s*(?:#.*)?)$/Potential issues:
Recommendation: Test with different YAML formatting variations: # Test cases:
assets_bundler: webpack
assets_bundler:webpack
assets_bundler: "webpack"
assets_bundler: 'webpack'
assets_bundler: webpack # comment
assets_bundler: webpack#comment (no space)2. Missing test coverage for the actual bugLooking at
Current test (line 162-212) only checks:
Recommendation: Add a test like: it "configures rspack in all environment sections" do
assert_file "config/shakapacker.yml" do |content|
# Verify default section
expect(content).to match(/default:.*assets_bundler:\s*rspack/m)
# Verify development section
expect(content).to match(/development:.*assets_bundler:\s*rspack/m)
# Verify test section
expect(content).to match(/test:.*assets_bundler:\s*rspack/m)
# Verify production section
expect(content).to match(/production:.*assets_bundler:\s*rspack/m)
# Verify YAML anchors are preserved
expect(content).to include("<<: *default")
end
end3. bin/switch-bundler: Hardcoded old/new bundler valuesIn old_bundler = @target_bundler == "rspack" ? "webpack" : "rspack"
new_content = content.gsub(
/^(\s*assets_bundler:\s*)["\']?#{old_bundler}["\']?(\s*(?:#.*)?)$/,
"\\1#{@target_bundler}\\2"
)Problem: This assumes the current bundler is the opposite of the target. What if:
Recommendation: Make the replacement more flexible: # Replace any bundler value, not just the opposite one
new_content = content.gsub(
/^(\s*assets_bundler:\s*)["\']?\w+["\']?(\s*(?:#.*)?)$/,
"\\1#{@target_bundler}\\2"
)🟡 Minor Issues4. Removed YAML require but not referenced in commentsIn 5. Generator uses Thor's gsub_file, script uses manual File.read/writeFor consistency, both should use the same approach. Since 6. No validation that the replacement succeededAfter the regex replacement, there's no check to ensure it actually matched and replaced something. If the regex doesn't match (due to unexpected formatting), the operation silently does nothing. Recommendation: Add validation: unless new_content.include?("assets_bundler: #{@target_bundler}")
warn "⚠️ Warning: Could not find assets_bundler setting to update"
end📋 Testing RecommendationsBefore merging, manually test these scenarios:
🎯 SummarySeverity breakdown:
Recommendation: Add comprehensive tests before merging. The fix is heading in the right direction, but needs more robust regex patterns and test coverage to ensure it works reliably across different YAML formatting styles. Great work on the investigation and fix! The approach is much better than YAML parsing. Just needs more comprehensive testing and slightly more flexible regex patterns. 🚀 |
Code Review: Rspack Configuration FixThis PR fixes two important bugs in the Rspack support feature. The solution replaces YAML parsing with regex-based replacement, which is the correct approach. Strengths
Issues & Recommendations1. CRITICAL: Missing Test CoveragePer CLAUDE.md requirements, these changes need automated tests for:
2. Regex Pattern RobustnessIn bin/switch-bundler, the pattern only matches the old bundler value. If a user has mixed values, not all sections will update. Consider matching ANY bundler value (webpack|rspack) regardless of current state. 3. Testing RequirementsPer CLAUDE.md: Test fresh install, bin/switch-bundler in both directions, and verify YAML anchors/comments are preserved. Checklist Before Merge
Overall, this is a well-reasoned fix. Main gaps are test coverage and some regex edge cases. Great work on the analysis! |
Summary
Fixes two bugs in the Rspack support feature (PR #1852):
bin/switch-bundlercrashes withPsych::AliasesNotEnabledon YAML files containing anchors/aliases--rspackflag andbin/switch-bundleronly updatedefaultsection - environment sections retainwebpackRoot Cause
Both the generator and
bin/switch-bundlerusedYAML.load→ modify default →YAML.dumpwhich:YAML.loadexpands<<: *defaultinheritance into explicit copiesconfig["default"]doesn't affect already-expanded environment sectionsYAML.dumpwrites back with all sections having explicit values (only default was changed)Solution
Replace YAML parse/dump with regex replacement (Thor's
gsub_filepattern):assets_bundlerin all sections, not just defaultTesting
Before (reproduce bugs)
After (with this fix)
Test plan
--rspackconfigures rspack in all environmentsbin/switch-bundler rspackupdates all environment sectionsbin/switch-bundler webpackupdates all environment sections🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.