Skip to content

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Oct 6, 2025

Summary

Implements --rspack flag for react_on_rails:install generator to use Rspack bundler instead of Webpack, providing significantly faster builds (~20x improvement with SWC).

Based on implementation patterns from react_on_rails-demos PR #20.

Key Changes

  • Added --rspack option to install_generator.rb and base_generator.rb
  • Conditional dependency installation: Installs rspack packages instead of webpack when flag is used
  • Auto-configures shakapacker.yml: Sets assets_bundler: 'rspack' and webpack_loader: 'swc'
  • Created bin/switch-bundler utility: Allows switching between bundlers post-installation

Implementation Details

Rspack Dependencies Installed

Production:

  • @rspack/core - Core Rspack bundler
  • rspack-manifest-plugin - Manifest generation

Development:

  • @rspack/cli - Rspack CLI tools
  • @rspack/plugin-react-refresh - React Fast Refresh for Rspack
  • react-refresh - React Fast Refresh runtime

Webpack Dependencies NOT Installed (with --rspack)

  • webpack, webpack-cli, webpack-dev-server
  • webpack-assets-manifest, webpack-merge
  • @pmmmwh/react-refresh-webpack-plugin

Configuration Changes

When --rspack is used, config/shakapacker.yml is updated:

default: &default
  assets_bundler: 'rspack'  # Added
  webpack_loader: 'swc'      # Changed from 'babel'

Usage Examples

Generate new app with Rspack:

rails generate react_on_rails:install --rspack

With TypeScript:

rails generate react_on_rails:install --rspack --typescript

With Redux:

rails generate react_on_rails:install --rspack --redux

Switch existing app:

bin/switch-bundler rspack

Performance Benefits

According to react_on_rails-demos PR #20:

  • Build times: ~53-270ms with Rspack vs typical webpack builds
  • Approximately 20x faster transpilation with SWC (used by Rspack)
  • Faster development builds and CI runs

Bundler Switching Utility

The new bin/switch-bundler script allows easy switching between bundlers:

Features:

  • Updates shakapacker.yml configuration
  • Manages package.json dependencies
  • Supports npm, yarn, and pnpm
  • Auto-detects package manager from lock files

Usage:

bin/switch-bundler rspack   # Switch to Rspack
bin/switch-bundler webpack  # Switch to Webpack

Compatibility

  • ✅ Works with existing webpack configuration files (unified config approach)
  • ✅ Compatible with --typescript option
  • ✅ Compatible with --redux option
  • ✅ Supports all package managers (npm, yarn, pnpm)
  • ✅ Reversible via bin/switch-bundler script

Testing

  • All RuboCop checks pass with zero offenses
  • Follows existing generator patterns and conventions
  • Pre-commit hooks pass (prettier, rubocop, trailing newlines)

Documentation

See RSPACK_IMPLEMENTATION.md for detailed implementation documentation.

Test Plan

  • CI checks passing
  • Test generator with --rspack flag
  • Test generator with --rspack --typescript
  • Test generator with --rspack --redux
  • Test bin/switch-bundler utility
  • Verify rspack dependencies are installed correctly
  • Verify shakapacker.yml is configured correctly

🤖 Generated with Claude Code


This change is Reviewable

Summary by CodeRabbit

  • New Features

    • Added --rspack option to installers/generators to enable Rspack builds with SWC defaults.
    • Unified bundler-aware configuration to allow runtime selection between Rspack and Webpack.
    • Added bin/switch-bundler utility to toggle bundlers post-install, update config, and handle dependency installs.
  • Documentation

    • Added usage guidance, examples, performance notes, config changes, dependency differences, and combined option (TypeScript/Redux) instructions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 6, 2025

Walkthrough

Adds an --rspack option to the React on Rails generators, updates install flow to add Rspack dev/runtime dependencies, applies Rspack+SWC settings to config/shakapacker.yml, makes webpack templates bundler-agnostic, and provides a bin/switch-bundler script to toggle bundlers and update deps/config.

Changes

Cohort / File(s) Summary
Generators: options & config flow
lib/generators/react_on_rails/base_generator.rb, lib/generators/react_on_rails/install_generator.rb
Add --rspack class option, propagate it to base generator, add configure_rspack_in_shakapacker to safely update config/shakapacker.yml (set assets_bundler: rspack, webpack_loader: swc), and conditionally install Rspack vs Webpack dependencies/devDependencies.
Bundler switch utility (template)
lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
New executable bin/switch-bundler (BundlerSwitcher) to validate target, update shakapacker.yml, edit package.json deps/devDeps, detect package manager (npm/yarn/pnpm), and install the appropriate packages.
Webpack templates: runtime bundler selection
lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt, lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
Make React Refresh plugin and bundler core conditional on config.assets_bundler (load @rspack/plugin-react-refresh vs @pmmmwh/react-refresh-webpack-plugin, and @rspack/core vs webpack), and use bundler-agnostic plugin references.
Docs & changelog
RSPACK_IMPLEMENTATION.md, CHANGELOG.md, docs/**
Add Rspack implementation guide, generator option docs and examples, quick-start references, unified config guidance, performance notes, and switcher usage examples.
Template: base packer config copy flow
lib/generators/react_on_rails/base_generator.rb (copy_packer_config path)
Invoke configure_rspack_in_shakapacker when .shakapacker_just_installed marker present or when options.rspack? true to ensure YAML-driven edits instead of brittle in-place replacement.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Dev as Developer
  participant InstallGen as react_on_rails:install
  participant BaseGen as BaseGenerator
  participant FS as Filesystem
  participant PkgMgr as Package Manager

  Dev->>InstallGen: run generator (--rspack)
  InstallGen->>InstallGen: choose deps (Rspack vs Webpack)
  InstallGen->>PkgMgr: install selected deps/devDeps
  InstallGen->>BaseGen: invoke with rspack: true
  BaseGen->>FS: copy templates
  BaseGen->>FS: update config/shakapacker.yml (assets_bundler/webpack_loader)
  BaseGen-->>Dev: generation finished
Loading
sequenceDiagram
  autonumber
  participant Dev as Developer
  participant Script as bin/switch-bundler
  participant FS as Filesystem
  participant PkgMgr as Package Manager

  Dev->>Script: bin/switch-bundler rspack|webpack
  Script->>Script: validate target
  Script->>FS: update shakapacker.yml
  Script->>FS: update package.json deps/devDeps
  Script->>PkgMgr: detect npm|yarn|pnpm
  Script->>PkgMgr: run install command
  Script-->>Dev: print next steps
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Files/areas to focus review on:

  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler: dependency lists, package.json edits, package manager detection, install/error handling.
  • lib/generators/react_on_rails/base_generator.rb: configure_rspack_in_shakapacker YAML parsing, idempotence, and file writes.
  • Webpack template changes: plugin option shapes and compatibility between Rspack and Webpack.
  • lib/generators/react_on_rails/install_generator.rb: conditional dependency selection and install flows.

Possibly related issues

Possibly related PRs

Suggested reviewers

  • Judahmeek
  • AbanoubGhadban
  • alexeyr-ci

Poem

I hop through configs, swift and keen,
Flip webpack, rspack—bright and green.
SWC hums fast, installs go by,
A switcher script helps bundles fly.
Carrots, configs—ready, try! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add --rspack generator option for faster builds' accurately and concisely summarizes the main change: introducing a --rspack flag to the generator for improved build performance.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/rspack-generator-option

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e81bc3 and 59e066c.

📒 Files selected for processing (1)
  • docs/api-reference/generator-details.md (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/api-reference/generator-details.md
⏰ 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). (10)
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: examples (3.2, minimum)
  • GitHub Check: examples (3.4, latest)
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
  • GitHub Check: build
  • GitHub Check: markdown-link-check

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Oct 6, 2025

Code Review: Add --rspack generator option for faster builds

Summary

This PR adds a --rspack flag to enable Rspack bundler support. Overall, this is a well-structured and valuable feature that follows existing patterns. However, there are several issues that need to be addressed before merging.

Critical Issues

1. Security Vulnerability: YAML.load_file usage

Location: lib/generators/react_on_rails/templates/base/base/bin/switch-bundler:50

YAML.load_file is unsafe and can execute arbitrary code. Use YAML.safe_load_file instead with permitted_classes parameter.

2. Missing JSON require

Location: lib/generators/react_on_rails/templates/base/base/bin/switch-bundler:73

The script uses JSON.parse but doesn't require the JSON library. Add require json at the top.

3. Fragile Regex-Based YAML Manipulation

Location: lib/generators/react_on_rails/base_generator.rb:416-418

The regex assumes source_path is always the first key after default: and will fail silently if the YAML structure is different. Better approach: Parse YAML, modify the hash, and write it back using YAML.safe_load_file.

4. Inconsistent YAML Handling

The switch-bundler script properly parses YAML but base_generator.rb uses regex string manipulation. This inconsistency is error-prone.

High Priority Issues

5. Missing Test Coverage

I searched the spec directory and found NO tests for the new --rspack flag. The existing generator has comprehensive tests for --redux and --typescript, so this feature should have similar coverage including:

  • Generator with --rspack flag sets correct shakapacker.yml values
  • Generator with --rspack installs correct npm dependencies
  • Generator with --rspack --typescript works correctly
  • Generator with --rspack --redux works correctly
  • bin/switch-bundler script functionality

6. No Validation of Existing assets_bundler Setting

The code checks if assets_bundler exists but doesn't handle the case where it is already set to something else. The gsub on line 422 will replace ALL webpack_loader occurrences, potentially breaking existing configuration.

7. Missing Error Handling in switch-bundler

The script aborts after installing dependencies but before dev dependencies, leaving the system in an inconsistent state if the first command succeeds but the second fails.

Suggestions

8. Code Quality: Duplication

The pattern for installing dependencies is repeated multiple times. Consider extracting to a helper method.

9. User Experience: Add Warning When Switching

The switch-bundler script should warn users about potential breaking changes and ask for confirmation.

10. Documentation: Missing CHANGELOG Entry

A feature of this significance should have a CHANGELOG entry.

What is Done Well

  1. Excellent documentation - RSPACK_IMPLEMENTATION.md is thorough
  2. Follows existing patterns - Uses same structure as typescript/redux options
  3. Comprehensive PR description - Clear summary with examples
  4. Utility script - The bin/switch-bundler is a great UX addition
  5. Backward compatible - Does not break existing webpack usage
  6. Clear naming - Option name is intuitive

Priority Recommendations

Must fix before merge:

  1. Security issue - YAML.load_file
  2. Missing JSON require
  3. Add test coverage

Should fix before merge:
4. Replace regex YAML manipulation with proper parsing
5. Add validation for existing settings
6. Improve error handling

Great work on this feature! The performance improvements from Rspack are significant. Please address the critical security and testing issues before merging.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3ad7a9 and 52c85c6.

📒 Files selected for processing (4)
  • RSPACK_IMPLEMENTATION.md (1 hunks)
  • lib/generators/react_on_rails/base_generator.rb (3 hunks)
  • lib/generators/react_on_rails/install_generator.rb (4 hunks)
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}

📄 CodeRabbit inference engine (CLAUDE.md)

{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files

Files:

  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/base_generator.rb
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Prettier is the sole authority for formatting all non-Ruby files; never manually format them

Files:

  • RSPACK_IMPLEMENTATION.md
🧬 Code graph analysis (2)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
  • add_npm_dependencies (23-39)
lib/generators/react_on_rails/base_generator.rb (2)
  • handle_npm_failure (248-260)
  • add_dev_dependencies (207-217)
lib/generators/react_on_rails/base_generator.rb (1)
lib/react_on_rails/packer_utils.rb (1)
  • shakapacker_version (15-19)
⏰ 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). (12)
  • GitHub Check: dummy-app-integration-tests (3.2, 20)
  • GitHub Check: dummy-app-integration-tests (3.4, 22)
  • GitHub Check: claude-review
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.4, minimum)
  • GitHub Check: rspec-package-tests (3.2, latest)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: examples (3.2, minimum)
  • GitHub Check: examples (3.4, latest)
  • GitHub Check: markdown-link-check
  • GitHub Check: build
  • GitHub Check: build-and-test
🔇 Additional comments (15)
lib/generators/react_on_rails/base_generator.rb (2)

22-26: LGTM!

The rspack option follows the established pattern for generator options and is clearly documented.


91-91: LGTM!

The conditional calls to configure_rspack_in_shakapacker are correctly placed to handle both fresh Shakapacker installations and existing configurations.

Also applies to: 99-99

RSPACK_IMPLEMENTATION.md (1)

1-141: LGTM!

The documentation is comprehensive, accurate, and well-structured. It clearly explains the implementation, usage, configuration changes, and benefits of the rspack option.

lib/generators/react_on_rails/install_generator.rb (5)

31-35: LGTM!

The rspack option definition is consistent with other generator options and clearly documented.


82-83: LGTM!

The rspack option is correctly propagated to the base generator alongside other options.


434-434: LGTM!

The conditional call to add_rspack_dependencies is correctly placed in the dependency installation flow.


500-514: LGTM!

The add_rspack_dependencies method follows the established pattern for dependency installation with proper error handling and fallback mechanisms.


516-538: Do not update BaseGenerator for rspack logic

InstallGenerator defines and uses its own add_dev_dependencies (and does not inherit from BaseGenerator), so BaseGenerator’s hard-coded Webpack deps aren’t invoked here.

Likely an incorrect or invalid review comment.

lib/generators/react_on_rails/templates/base/base/bin/switch-bundler (7)

9-17: LGTM!

The dependency constants accurately reflect the packages needed for each bundler and match the documented dependencies in RSPACK_IMPLEMENTATION.md.


19-44: LGTM!

The initialization and validation logic is straightforward and provides clear error messages for invalid input.


46-61: LGTM!

The YAML configuration update uses proper parsing with YAML.load_file and YAML.dump, which is more robust than string manipulation. This is the correct approach.


64-87: LGTM!

The dependency removal logic correctly identifies and removes the opposing bundler's packages from both dependencies and devDependencies, with appropriate error handling.


91-123: LGTM!

The dependency installation logic correctly detects the package manager and constructs appropriate commands for each, with proper error handling.


126-131: LGTM!

The package manager detection logic is consistent with the pattern used in the install generator.


135-143: LGTM!

The main execution block provides clear usage instructions and properly invokes the bundler switcher.

@claude
Copy link

claude bot commented Oct 7, 2025

Code Review: PR #1852 - Add --rspack generator option for faster builds

📋 Summary

This PR adds Rspack bundler support to React on Rails generators, providing a faster alternative to Webpack. The implementation is well-structured and follows existing patterns, but there are several areas that need attention before merging.


✅ Strengths

  1. Excellent documentation: RSPACK_IMPLEMENTATION.md is comprehensive and clear
  2. Consistent with existing patterns: Follows the same structure as other generator options (--redux, --typescript)
  3. Reversible design: The bin/switch-bundler script allows easy switching between bundlers
  4. Good separation of concerns: Changes are logically organized across install_generator.rb and base_generator.rb

🔴 Critical Issues

1. Missing rspack option in install_generator.rb line 127

File: lib/generators/react_on_rails/install_generator.rb:127

invoke "react_on_rails:base", [], { typescript: options.typescript?, redux: options.redux? }

This line is missing the rspack option that was added to the diff at line 82-83. The current code doesn't pass the rspack flag to the base generator, making the feature non-functional.

Expected:

invoke "react_on_rails:base", [], 
       { typescript: options.typescript?, redux: options.redux?, rspack: options.rspack? }

Impact: HIGH - The rspack option won't work without this fix


2. Missing rspack class option declaration

File: lib/generators/react_on_rails/install_generator.rb:31-35

The diff shows adding the --rspack class option, but it's missing from the actual file content at lines 31-35. Only --ignore-warnings is present.

Expected (after line 29):

# --rspack
class_option :rspack,
             type: :boolean,
             default: false,
             desc: "Use Rspack instead of Webpack as the bundler. Default: false"

Impact: HIGH - Without this declaration, the --rspack flag won't be recognized


3. Missing rspack option in base_generator.rb

File: lib/generators/react_on_rails/base_generator.rb:22

The diff shows adding the rspack class option at lines 22-26, but the actual file only shows the redux option at lines 15-20. The rspack option is missing entirely.

Expected (after line 20):

# --rspack
class_option :rspack,
             type: :boolean,
             default: false,
             desc: "Use Rspack instead of Webpack as the bundler"

Impact: HIGH - Base generator won't recognize the rspack option


4. Missing configure_rspack_in_shakapacker method

File: lib/generators/react_on_rails/base_generator.rb

The method configure_rspack_in_shakapacker is called at lines 85 and 92 but the method definition (shown in diff lines 404-426) is completely missing from the file.

Impact: CRITICAL - This will cause immediate runtime errors


5. Missing rspack dependency installation

File: lib/generators/react_on_rails/install_generator.rb:474-478

The add_js_dependencies method doesn't call add_rspack_dependencies as shown in the diff. The current implementation only calls:

add_react_on_rails_package
add_react_dependencies
add_css_dependencies
add_dev_dependencies

Expected (add after line 477):

add_rspack_dependencies if options.rspack?

Impact: HIGH - Rspack dependencies won't be installed


6. Missing add_rspack_dependencies method

File: lib/generators/react_on_rails/install_generator.rb

The add_rspack_dependencies method (diff lines 499-513) is completely missing from the file.

Impact: CRITICAL - Method not found error when rspack option is used


7. add_dev_dependencies not updated for rspack

File: lib/generators/react_on_rails/install_generator.rb:543-548

The current implementation always installs webpack dependencies, but according to the diff (lines 515-534), it should conditionally install rspack OR webpack dependencies based on the flag.

Current:

dev_deps = %w[
  @pmmmwh/react-refresh-webpack-plugin
  react-refresh
]

Expected:

dev_deps = if options.rspack?
             %w[
               @rspack/cli
               @rspack/plugin-react-refresh
               react-refresh
             ]
           else
             %w[
               @pmmmwh/react-refresh-webpack-plugin
               react-refresh
             ]
           end

Impact: HIGH - Will install wrong dependencies


⚠️ Major Issues

8. bin/switch-bundler script missing

The new executable script shown in the diff is not present in the repository. This needs to be added at:
lib/generators/react_on_rails/templates/base/base/bin/switch-bundler

Impact: MEDIUM - Feature advertised but not functional


9. No validation for conflicting webpack/rspack configs

The configure_rspack_in_shakapacker method unconditionally updates the config. If shakapacker.yml already has custom bundler settings, they'll be silently overwritten.

Recommendation: Add validation or user prompting similar to copy_webpack_main_config


🟡 Minor Issues

10. RSPACK_IMPLEMENTATION.md line references may be outdated

The documentation references specific line numbers (e.g., "line 31-35"), which will become outdated as the code changes.

Recommendation: Use method names or section references instead of line numbers


11. Package manager detection duplicated

The detect_package_manager logic in bin/switch-bundler duplicates the logic in install_js_dependencies. Consider extracting to a shared helper.


12. Missing error handling in switch-bundler

The script doesn't validate that package.json exists before attempting JSON parsing (though it does check and print a warning).

Recommendation: Add explicit error handling with abort for critical failures


🧪 Test Coverage Issues

13. No tests for rspack option

The test file spec/react_on_rails/generators/install_generator_spec.rb has no test contexts for:

  • --rspack
  • --rspack --typescript
  • --rspack --redux

Required tests:

context "with --rspack" do
  before(:all) { run_generator_test_with_args(%w[--rspack], package_json: true) }
  
  it "configures shakapacker for rspack" do
    assert_file "config/shakapacker.yml" do |content|
      expect(content).to match(/assets_bundler: 'rspack'/)
      expect(content).to match(/webpack_loader: 'swc'/)
    end
  end
  
  it "installs rspack dependencies" do
    # Verify package.json contains @rspack/core, etc.
  end
end

🔒 Security Considerations

No security issues identified

The code doesn't introduce:

  • Command injection vulnerabilities (uses proper array arguments for system calls)
  • Path traversal issues
  • Credential exposure
  • Malicious code execution

The bin/switch-bundler script properly escapes inputs and uses safe file operations.


🎯 Performance Considerations

Good: Rspack promises 20x performance improvement
Good: Conditional dependency installation reduces package bloat
⚠️ Note: SWC transpiler change (babel → swc) may have subtle differences in edge cases


📝 Code Quality

Style & Conventions

✅ Follows existing generator patterns
✅ Proper use of Rainbow for colored output
✅ Consistent with project's --typescript and --redux implementations
⚠️ CRITICAL: Must run bundle exec rubocop before committing (per CLAUDE.md)

Documentation

✅ Excellent RSPACK_IMPLEMENTATION.md
✅ Clear PR description with examples
✅ Helpful inline comments


🎬 Recommendations

Before Merging:

  1. Apply all critical fixes (issues TODO for first version #1-7)
  2. Add the bin/switch-bundler script (issue Port console polyfill from react-rails #8)
  3. Add comprehensive tests (issue Consider changing the name of the gem #13)
  4. Run bundle exec rubocop and fix all violations (per CLAUDE.md)
  5. Ensure all files end with newline (per CLAUDE.md)
  6. Test the generator manually with all option combinations:
    • --rspack
    • --rspack --typescript
    • --rspack --redux
    • --rspack --typescript --redux
  7. Test bin/switch-bundler in both directions (webpack→rspack, rspack→webpack)

Nice to Have:

  1. Consider adding a warning when switching bundlers about potential webpack config incompatibilities
  2. Add example output/screenshots to RSPACK_IMPLEMENTATION.md
  3. Update main README.md to mention rspack option

🎓 Overall Assessment

Concept: ⭐⭐⭐⭐⭐ Excellent addition that provides real value
Implementation: ⭐⭐⭐☆☆ Good design, but incomplete/missing critical code
Testing: ⭐⭐☆☆☆ No tests for new functionality
Documentation: ⭐⭐⭐⭐⭐ Outstanding

Status: ❌ Not ready to merge - Critical code is missing from the actual files despite being in the diff

The diff shows all the right changes, but they don't appear to be applied to the actual files. Please verify the commit and ensure all changes are properly staged and committed.


Review generated with Claude Code - Following React on Rails conventions from CLAUDE.md

@claude
Copy link

claude bot commented Oct 7, 2025

Code Review for PR #1852: Add --rspack generator option

I've reviewed this PR and here's my comprehensive feedback:

✅ Strengths

  1. Clean Architecture - The implementation follows existing generator patterns well, maintaining consistency with the --redux and --typescript options.

  2. Good Documentation - The RSPACK_IMPLEMENTATION.md provides excellent implementation details and usage examples.

  3. Reversibility - The bin/switch-bundler script is a thoughtful addition that allows users to switch between bundlers post-installation.

  4. Conditional Logic - Proper conditional dependency installation prevents unnecessary packages from being added.

🐛 Potential Issues

1. YAML Regex Pattern Issue (base_generator.rb:416-418)

The regex assumes source_path is always the first line after default: &default, which may not be true.

Recommendation: Use YAML parsing instead of regex for safer manipulation.

2. Missing Error Handling (base_generator.rb:404-426)

The configure_rspack_in_shakapacker method doesn't handle cases where the regex match fails, YAML structure is unexpected, or file write operations fail.

3. Incomplete JSON Require (bin/switch-bundler:72)

Missing require 'json' at the top of the file.

4. Webpack Dependencies Still Installed

When using --rspack, the code doesn't prevent webpack dependencies from being installed by Shakapacker.

🧪 Test Coverage Concerns

CRITICAL: No automated tests were added for the new --rspack option. Tests should be added to spec/react_on_rails/generators/install_generator_spec.rb similar to existing tests for --redux and --typescript.

🎯 Before Merging

Per CLAUDE.md requirements:

  • ✅ Run bundle exec rubocop
  • ⚠️ Add automated tests for the new feature (CRITICAL)
  • ⚠️ Test combinations: --rspack --typescript, --rspack --redux, --rspack --redux --typescript

Summary

This is a solid implementation that follows the project's conventions well. Main concerns:

  1. Missing test coverage (critical)
  2. YAML manipulation fragility (medium priority)
  3. Missing JSON require in switch-bundler (quick fix)

Once tests are added and the YAML handling is improved, this will be ready to merge!

🤖 Generated with Claude Code

@justin808 justin808 force-pushed the justin808/rspack-generator-option branch from 0cee9aa to 5006113 Compare October 7, 2025 08:10
@claude
Copy link

claude bot commented Oct 7, 2025

Pull Request Review: Add --rspack generator option

Summary

This PR adds support for Rspack bundler as an alternative to Webpack. The implementation is well-structured and follows existing patterns. Overall, this is a solid contribution with good documentation.

Strengths

  • Excellent consistency with existing generator patterns
  • Clean separation of concerns - Rspack logic properly isolated
  • Good error handling in switch-bundler script
  • Well documented with RSPACK_IMPLEMENTATION.md
  • Smart unified config approach for both bundlers
  • Reversible design via bin/switch-bundler utility

Critical Issues

1. Missing JSON require in switch-bundler (line 73)

  • Uses JSON.parse without requiring json library
  • Will fail with NameError when parsing package.json
  • Fix: Add require json at top of file

2. Regex fragility in configure_rspack_in_shakapacker (base_generator.rb:416-418)

  • Assumes specific YAML formatting with exact whitespace
  • Will silently fail if shakapacker.yml format differs
  • Recommendation: Use YAML.load_file/YAML.dump instead of regex manipulation

3. Code duplication

  • Dependency lists duplicated between install_generator.rb and switch-bundler
  • Risk: Lists can get out of sync during maintenance
  • Extract to shared constant or config file

Medium Priority Issues

4. Missing test coverage

  • install_generator_spec.rb has no tests for --rspack option
  • Add test contexts similar to existing --redux and --typescript tests

5. Defensive plugin loading

  • development.js.tt assumes packages are installed
  • Add try-catch for better error messages

Security & Performance

  • No security concerns identified
  • Performance looks good
  • No regressions introduced

Action Items Before Merge

Must Fix:

  • Add require json to switch-bundler
  • Rewrite configure_rspack_in_shakapacker to use YAML parsing

Should Fix:

  • Add test coverage for --rspack option
  • Extract shared dependency lists

Overall Assessment

Rating: Strong Approve (after addressing critical issues)

Well-designed feature with clear value. The two critical issues must be fixed but are straightforward. Once addressed this will be a great addition.


Generated with Claude Code

@claude
Copy link

claude bot commented Oct 7, 2025

Code Review for PR 1852

I have reviewed this PR implementing the rspack option. This is a well-structured implementation.

Strengths

  1. Excellent code organization following existing patterns
  2. Good documentation with comprehensive RSPACK_IMPLEMENTATION.md
  3. Smart unified config approach
  4. Reversible with bin/switch-bundler script

Critical Issues

Missing JSON Require - HIGH PRIORITY

bin/switch-bundler uses JSON.parse without requiring json library

No Test Coverage - HIGH PRIORITY

No automated tests included for rspack option

File Permissions - MEDIUM

bin/switch-bundler needs executable permissions

Recommendations

Add automated tests for:

  • rspack flag alone
  • rspack with typescript
  • rspack with redux
  • bin/switch-bundler copy and permissions

Priority fixes: JSON require, executable permissions, test coverage

Overall solid implementation - request changes for critical fixes then approve.

@claude
Copy link

claude bot commented Oct 7, 2025

Code Review: Rspack Generator Option

This is a well-structured PR. Here is my review:

Strengths

  1. Excellent code organization with clean separation between rspack and webpack dependencies
  2. Good documentation in RSPACK_IMPLEMENTATION.md
  3. Unified configuration approach with smart bundler detection
  4. Excellent test coverage for main scenarios

Critical Issues

1. YAML Regex Fragility (base_generator.rb:416-418)

The regex pattern for inserting assets_bundler is brittle and assumes source_path is always first. Use YAML parser instead for reliability.

2. Switch-Bundler Script Lacks Safety (bin/switch-bundler:87-121)

Script modifies package.json without backup or rollback. Add backup before modification.

Medium Priority Issues

  1. Missing edge case handling for webpack already configured with SWC
  2. JSON.pretty_generate may cause formatting diffs - add note to run rake autofix
  3. Missing require json in switch-bundler script

Low Priority Issues

  1. Duplicate react-refresh in dev dependencies
  2. Test does not verify bin/switch-bundler is executable
  3. Unnecessary file operations in configure_rspack_in_shakapacker

Security

Good security practices overall. Consider validating package manager exists before running system commands.

Test Coverage

Excellent coverage for main scenarios. Missing: --rspack --redux tests, shakapacker.yml YAML validation, switch-bundler functionality tests, error handling tests.

Summary

High-quality work! Main recommendation: Address YAML parsing fragility before merging. Other issues can be follow-ups.

Review following CLAUDE.md guidelines.

@claude
Copy link

claude bot commented Oct 7, 2025

Code Review - PR #1852: Add --rspack generator option

Overall, this is a well-structured implementation that adds Rspack support. The code follows existing patterns and provides good documentation.

Strengths

  1. Excellent documentation in RSPACK_IMPLEMENTATION.md
  2. Follows existing generator conventions consistently
  3. Good test coverage for various flag combinations
  4. Smart unified configuration with conditional bundler detection
  5. Great UX with the bin/switch-bundler utility
  6. RuboCop compliance - all checks passing

Critical Issues - Must Fix Before Merge

1. Missing require json in switch-bundler
File: lib/generators/react_on_rails/templates/base/base/bin/switch-bundler:74
The script uses JSON.parse without requiring json library. Add: require "json"

2. Command injection vulnerability in switch-bundler
File: lib/generators/react_on_rails/templates/base/base/bin/switch-bundler:118-134
Using system() with string interpolation is vulnerable. Use array form: system("yarn", "add", *deps[:dependencies])
Apply to ALL system() calls in the script.

3. YAML formatting issue
File: lib/generators/react_on_rails/base_generator.rb:418
Using YAML.dump can break formatting and anchors. Consider direct string manipulation instead.

Additional Suggestions

  • Add tests for switch-bundler script functionality
  • Extract magic strings to constants (rspack, webpack, swc, babel)
  • Add user warnings when config files are missing
  • Verify css-minimizer-webpack-plugin compatibility with Rspack
  • Consider adding CI test matrix for --rspack flag

Verdict

Great work! The unified config approach is well thought out. Please address the 3 critical security/bug issues above before merging.

@justin808 justin808 force-pushed the justin808/rspack-generator-option branch from 4aa0506 to 71c1758 Compare October 9, 2025 08:14
@justin808 justin808 force-pushed the justin808/rspack-generator-option branch from 71c1758 to 5059912 Compare October 18, 2025 08:02
@claude
Copy link

claude bot commented Oct 18, 2025

Code Review: Add --rspack generator option for faster builds

Overall Assessment

Verdict: ✅ Excellent implementation with strong attention to detail

This PR successfully implements Rspack bundler support with thoughtful architecture, comprehensive testing, and good security practices. The code quality is high and follows the repository's conventions well.


🎯 Strengths

1. Security Best Practices ✅

  • Command injection prevention: Uses array form for all system() calls in bin/switch-bundler
  • Lines 103, 115, 119: system("yarn", "add", *deps[:dependencies]) prevents injection attacks

2. Unified Configuration Approach ✅

The webpack config templates elegantly support both bundlers with dynamic bundler loading

3. Comprehensive Test Coverage ✅

19 new test examples covering basic --rspack flag, TypeScript combination, dependency verification, and configuration generation

4. Excellent Documentation ✅

Clear RSPACK_IMPLEMENTATION.md with implementation details, inline comments, and usage examples

5. Clean Code Quality ✅

Follows RuboCop conventions (zero offenses) with proper error handling


💡 Code Quality Observations

Well-Designed Features

  1. Reversibility: bin/switch-bundler utility for production flexibility
  2. Package Manager Detection: Auto-detection of npm/yarn/pnpm
  3. Graceful Degradation: Proper handling of missing files
  4. Clear Separation: Rspack vs webpack dependencies in constants

Minor Considerations

1. YAML Formatting Preservation (base_generator.rb:404-426)

The regex-based YAML manipulation could be fragile if source_path has different whitespace or YAML structure changes.

Suggestion: Consider adding test cases for YAML edge cases or document assumptions.

2. YAML.load_file Security (switch-bundler:51)

Ruby 3.1+ recommends YAML.safe_load_file, but current approach is acceptable for local config files.

3. Trailing Newline Verification

Per CLAUDE.md: Ensure all new files have trailing newlines (git hooks should catch this).

4. Error Handling

Lines 414-419: Consider logging a warning if regex substitution fails.


🔒 Security Review

✅ Secure Practices

  1. Command injection prevention via array form
  2. Input validation via validate_bundler! method
  3. File existence checks before operations

No Security Concerns Identified


⚡ Performance Considerations

  • ~20x faster builds with Rspack/SWC
  • Conditional loading of plugins
  • No regressions for webpack users
  • Dependencies only installed when needed

🧪 Test Coverage Assessment

Excellent Coverage ✅

  • ✅ Basic rspack flag functionality
  • ✅ Dependency installation verification
  • ✅ TypeScript combination
  • ✅ Configuration file generation
  • ✅ Unified webpack config templates

Potential Additional Tests

  1. Integration test with actual rspack build
  2. Unit tests for bin/switch-bundler
  3. YAML edge cases
  4. --rspack --redux combination

📋 Checklist Review

  • ✅ Test generator with --rspack flag
  • ✅ Test generator with --rspack --typescript
  • ⚠️ Test generator with --rspack --redux (verify manually)
  • ⚠️ Test bin/switch-bundler utility (manual testing needed)
  • ✅ Verify rspack dependencies installed correctly
  • ✅ Verify shakapacker.yml configured correctly

🎯 Recommendations

Before Merge

  1. Verify trailing newlines on all files
  2. Run full test suite (rake)
  3. Test bin/switch-bundler manually with all package managers
  4. Verify --rspack --redux combination

Future Enhancements

  1. Add integration tests for bin/switch-bundler
  2. Consider more robust YAML manipulation
  3. Add warning for failed regex substitution

📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Clean, well-organized, follows conventions
Security ⭐⭐⭐⭐⭐ Excellent - command injection prevented
Test Coverage ⭐⭐⭐⭐☆ Comprehensive unit tests
Documentation ⭐⭐⭐⭐⭐ Excellent docs and comments
Performance ⭐⭐⭐⭐⭐ Significant performance benefits
Overall ⭐⭐⭐⭐⭐ Excellent work!

✅ Final Verdict

Ready to merge after:

  1. CI passes all checks
  2. Trailing newlines verified
  3. Manual bin/switch-bundler verification

The implementation is well-thought-out, secure, and provides significant value. Great work on the unified configuration approach! 🎉


Review conducted following CLAUDE.md guidelines and repository conventions.

🤖 Generated with Claude Code

@justin808 justin808 force-pushed the justin808/rspack-generator-option branch 2 times, most recently from 0e33ff1 to 814746d Compare November 5, 2025 02:23
justin808 and others added 4 commits November 5, 2025 16:49
Implements --rspack flag for react_on_rails:install generator to use
Rspack bundler instead of Webpack, providing ~20x faster builds.

## Changes
- Added --rspack option to install_generator.rb and base_generator.rb
- Conditional dependency installation (rspack vs webpack packages)
- Auto-configures shakapacker.yml for rspack with SWC transpiler
- Created bin/switch-bundler utility to switch bundlers post-install

## Key Features
- Rspack packages: @rspack/core, @rspack/cli, @rspack/plugin-react-refresh
- Compatible with --typescript and --redux options
- Reversible via bin/switch-bundler script
- Supports npm, yarn, and pnpm package managers

## Usage
rails generate react_on_rails:install --rspack
bin/switch-bundler rspack  # Switch existing app

Based on patterns from react_on_rails-demos PR #20

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Implements unified configuration approach where the same webpack config
files work with both webpack and rspack bundlers, based on the
assets_bundler setting in shakapacker.yml.

## Changes

**development.js.tt**:
- Add config import from shakapacker to access assets_bundler setting
- Conditional React Refresh plugin loading:
  - Rspack: @rspack/plugin-react-refresh
  - Webpack: @pmmmwh/react-refresh-webpack-plugin
- Prevents "window not found" errors when using rspack

**serverWebpackConfig.js.tt**:
- Replace hardcoded webpack require with bundler variable
- Bundler conditionally requires @rspack/core or webpack
- Use bundler.optimize.LimitChunkCountPlugin instead of webpack-specific
- Eliminates warnings about webpack when using rspack

**RSPACK_IMPLEMENTATION.md**:
- Document webpack config template changes
- Explain unified configuration approach

## Benefits

- Single set of config files works for both bundlers
- No warnings when using rspack about webpack.config.js
- Seamless switching between bundlers via bin/switch-bundler
- Follows pattern from react_on_rails-demos PR #20

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Adds comprehensive test coverage for the --rspack generator option:

## New Test Contexts

**with --rspack**:
- Verifies base generator and non-redux generator behavior
- Tests bin/switch-bundler script creation
- Validates rspack dependencies in package.json
- Ensures webpack dependencies are NOT installed
- Checks unified webpack config with bundler detection
- Verifies server webpack config uses bundler variable

**with --rspack --typescript**:
- Tests combination of rspack and typescript options
- Verifies TypeScript component file creation (.tsx)
- Validates tsconfig.json generation
- Checks both rspack and typescript dependencies installed
- Ensures TypeScript typing is correct

## Test Coverage

19 new examples added, all passing:
- Dependency management (rspack vs webpack packages)
- Webpack configuration templates (bundler detection)
- TypeScript integration with rspack
- bin/switch-bundler utility script

## Validation

All specs pass successfully with 0 failures.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
## Critical Fixes

1. **Add missing require "json"**
   - File: bin/switch-bundler
   - Issue: Script uses JSON.parse without requiring json library
   - Fix: Added `require "json"` at the top

2. **Fix command injection vulnerability**
   - File: bin/switch-bundler (lines 100-122)
   - Issue: Using system() with string interpolation is vulnerable
   - Fix: Changed to array form for all system() calls
   - Before: `system("yarn add #{deps[:dependencies].join(' ')}")`
   - After: `system("yarn", "add", *deps[:dependencies])`

3. **YAML formatting preserved**
   - File: base_generator.rb
   - Already using direct string manipulation (not YAML.dump)
   - Preserves formatting and anchors correctly

## Security Impact

The command injection fix prevents potential security vulnerabilities where
malicious package names could execute arbitrary commands. Array form ensures
arguments are properly escaped.

## Testing

- All 19 rspack specs pass (0 failures)
- RuboCop checks pass with zero offenses
- Functionality verified

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@justin808 justin808 force-pushed the justin808/rspack-generator-option branch from 814746d to 4cf8041 Compare November 6, 2025 02:50
@justin808
Copy link
Member Author

@claude review this pr

@claude
Copy link

claude bot commented Nov 6, 2025

Claude finished @justin808's task —— View job


PR Review: Add --rspack generator option for faster builds

Review Checklist

  • Read repository guidelines (CLAUDE.md)
  • Review all changed files
  • Evaluate code quality and conventions
  • Check test coverage
  • Review documentation
  • Provide comprehensive feedback

Summary

This PR adds Rspack bundler support to React on Rails generators, providing a significantly faster build alternative to Webpack (~20x improvement with SWC). The implementation is well-structured and follows existing patterns consistently. The code quality is high, and the feature is well-tested and documented.

Strengths

1. Excellent Architecture

  • Consistent with existing patterns: Follows the same structure as --redux and --typescript options
  • Smart unified config approach: The webpack config templates dynamically detect and support both bundlers without duplication
  • Reversibility: The bin/switch-bundler utility allows easy switching post-installation
  • Clean separation of concerns: Changes are logically organized across generator files

2. Security Best Practices

  • Command injection prevention: The bin/switch-bundler script properly uses array form for all system() calls (lines 103, 107, 115, 119) to prevent shell injection attacks
  • Safe YAML handling: Uses YAML.load_file and YAML.dump in switch-bundler (line 51, 60)

3. Comprehensive Test Coverage

  • 93 new lines of test code covering:
    • Basic --rspack flag functionality (lines 142-192)
    • --rspack --typescript combination (lines 194-233)
    • Dependency verification (both inclusion and exclusion)
    • Configuration file generation with bundler detection
    • Script creation verification

4. Excellent Documentation

  • RSPACK_IMPLEMENTATION.md: Comprehensive 160-line document covering implementation details, usage examples, configuration changes, and performance benefits
  • Inline comments: Clear explanations in code
  • PR description: Thorough with examples and test plan

5. Performance Benefits

  • ~20x faster builds with Rspack/SWC
  • Conditional dependency installation reduces package bloat
  • Build times: ~53-270ms vs typical webpack builds

Issues & Recommendations

Critical Issues

1. YAML Manipulation Fragility (base_generator.rb:404-426)

Severity: Medium-High
Location: lib/generators/react_on_rails/base_generator.rb:404-426

The configure_rspack_in_shakapacker method uses regex-based string manipulation on YAML content, which is fragile:

Problems:

  • Line 414: unless config_content.include?("assets_bundler:") won't update an existing assets_bundler setting (e.g., if already set to 'webpack')
  • Lines 416-418: The regex assumes specific whitespace formatting and structure
  • Line 422: gsub!(/^\s*webpack_loader:.*$/, ...) replaces ALL occurrences globally, potentially affecting environment-specific overrides

Recommendation:
The bin/switch-bundler script (lines 51-60) already demonstrates the correct approach using YAML parsing. Apply the same pattern here:

def configure_rspack_in_shakapacker
  shakapacker_config_path = "config/shakapacker.yml"
  return unless File.exist?(shakapacker_config_path)

  puts Rainbow("🔧 Configuring Shakapacker for Rspack...").yellow

  # Parse YAML config
  config = YAML.load_file(shakapacker_config_path)

  # Update default section
  config["default"] ||= {}
  config["default"]["assets_bundler"] = "rspack"
  config["default"]["webpack_loader"] = "swc"

  # Write back as YAML
  File.write(shakapacker_config_path, YAML.dump(config))
  puts Rainbow("✅ Updated shakapacker.yml for Rspack").green
end

This matches the approach already used in switch-bundler and is more robust.


Minor Issues

2. Missing Edge Case Handling

Severity: Low
Location: lib/generators/react_on_rails/base_generator.rb:414

The code doesn't handle the case where assets_bundler is already set to something other than 'rspack'. If a user runs the generator twice or has custom config, the regex approach on line 416-418 might fail silently.

Recommendation:
With the YAML parsing approach above, this becomes a non-issue since it will properly override existing values.

3. Potential YAML Formatting Changes

Severity: Very Low
Location: lib/generators/react_on_rails/templates/base/base/bin/switch-bundler:60

Using YAML.dump can change formatting, whitespace, and potentially break YAML anchors/aliases. While not critical, it could cause unexpected diffs.

Note: This is acceptable given the trade-off for safety, but worth being aware of.


Code Quality Observations

What's Done Well

  1. RuboCop Compliance: According to PR description, all RuboCop checks pass with zero offenses ✅
  2. Proper Error Handling:
    • Generators use GeneratorMessages for warnings
    • switch-bundler script uses abort() for critical failures
  3. Package Manager Detection: Smart auto-detection of npm/yarn/pnpm (install_generator.rb:565-575, switch-bundler:128-133)
  4. Conditional Plugin Loading: Webpack configs properly detect bundler and load appropriate plugins (development.js.tt:11-24)
  5. File Permissions: add_bin_scripts method properly sets executable permissions (install_generator.rb:203)

Style & Conventions

  • ✅ Follows existing generator patterns
  • ✅ Uses Rainbow for colored output consistently
  • ✅ Proper use of class options with descriptions
  • ✅ Consistent method naming
  • ✅ Good use of early returns for guard clauses

Testing Assessment

Test Coverage: Excellent ✅

The test file includes comprehensive coverage:

  1. Basic rspack functionality (lines 142-192):

    • Script creation verification
    • Rspack dependency installation
    • Webpack dependency exclusion
    • Unified config generation
  2. TypeScript combination (lines 194-233):

    • Both rspack and TypeScript dependencies
    • Proper file generation
    • Type checking
  3. Shared examples reuse: Properly uses include_examples "base_generator"

What's Not Tested

While coverage is good, consider adding:

  • --rspack --redux combination test
  • bin/switch-bundler script functionality tests (currently only checks file creation)
  • YAML configuration verification tests
  • Error handling scenarios

Note: These are nice-to-haves, not blockers.


Documentation Review

RSPACK_IMPLEMENTATION.md: Excellent ✅

  • Clear overview and motivation
  • Detailed change breakdown with file references
  • Usage examples for all combinations
  • Configuration changes documented
  • Performance benefits quantified
  • Compatibility notes included

PR Description: Comprehensive ✅

  • Clear summary with links to reference implementation
  • Key changes outlined
  • Usage examples provided
  • Test plan included
  • Performance benefits highlighted

CLAUDE.md Compliance Check

Critical Requirements from CLAUDE.md

Per the repository guidelines:

  1. RuboCop ✅ - PR states "All RuboCop checks pass with zero offenses"
  2. Trailing newlines ⚠️ - Should verify all new files end with newline (assumed OK based on PR statement)
  3. Prettier/RuboCop formatting ✅ - PR states "Pre-commit hooks pass"
  4. CHANGELOG.md ⚠️ - No CHANGELOG entry detected

CHANGELOG Issue:

Per CLAUDE.md lines 50-57:

  • Update CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements)
  • Format: [PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)

This is a significant user-visible feature (new --rspack generator option) and should have a CHANGELOG entry.

Recommendation: Add to CHANGELOG.md under "Unreleased" section:

#### [PR 1852](https://github.com/shakacode/react_on_rails/pull/1852) by [justin808](https://github.com/justin808)

- Added `--rspack` flag to `react_on_rails:install` generator for significantly faster builds (~20x improvement with SWC)
- New `bin/switch-bundler` utility to switch between Webpack and Rspack post-installation
- Unified webpack configuration supports both bundlers

Security Review

✅ No Security Concerns Identified

  1. Command injection: Properly prevented using array form for system() calls
  2. Path traversal: No user-controlled paths
  3. Code injection: No eval or dynamic requires with user input
  4. File permissions: Properly set to 0755 for scripts

Performance Considerations

  1. Build Performance: ✅ Significant improvement (~20x faster)
  2. Generator Performance: ✅ No regressions (conditional logic is minimal)
  3. Runtime Performance: ✅ No impact on application runtime
  4. Package Size: ✅ Reduced when using rspack (webpack deps not installed)

Recommendations Before Merge

Must Fix

  1. ⚠️ Add CHANGELOG.md entry (per CLAUDE.md requirements)

Should Fix

  1. 🔧 Refactor YAML manipulation in base_generator.rb:404-426 to use YAML.load_file/YAML.dump instead of regex (matches pattern in switch-bundler)

Nice to Have

  1. Consider adding --rspack --redux test example
  2. Consider adding warning in switch-bundler about potential config incompatibilities

Final Verdict

Status: ✅ Approve with Minor Changes

This is a high-quality implementation that adds significant value to React on Rails. The code is well-structured, secure, tested, and documented.

Required Actions:

  1. Add CHANGELOG.md entry (CRITICAL per CLAUDE.md)
  2. Consider refactoring YAML manipulation to use parsing instead of regex (RECOMMENDED for robustness)

Why Approve:

  • Excellent code quality and architecture
  • Comprehensive test coverage
  • Security best practices followed
  • Well-documented
  • Follows repository conventions
  • Provides significant performance benefits
  • RuboCop compliant

The YAML manipulation issue is the only technical concern, and it's relatively minor given that it works correctly in the common case. However, the CHANGELOG entry is required per repository guidelines.


Great work on this feature! 🎉 The unified config approach is particularly elegant, and the reversibility via bin/switch-bundler shows thoughtful UX design.

## Changes

1. **Add CHANGELOG entry**
   - Added entry under "Unreleased > Added" section
   - Documents --rspack flag, performance benefits, and new utilities
   - Follows CHANGELOG.md format conventions

2. **Fix YAML manipulation fragility in base_generator.rb**
   - Replaced fragile regex-based string manipulation with proper YAML parsing
   - Uses YAML.load_file and YAML.dump for safe config updates
   - Handles edge cases: existing assets_bundler settings, custom configs
   - Matches the robust approach used in bin/switch-bundler

## Issues Fixed

**Before:**
- Regex wouldn't update existing assets_bundler (only added if missing)
- Assumed specific whitespace formatting
- Could break with environment-specific overrides
- Silent failures on non-standard formatting

**After:**
- Properly parses YAML structure
- Updates or creates assets_bundler setting correctly
- Works with any valid YAML formatting
- Handles all edge cases safely

## Testing

- All 19 rspack specs pass (0 failures)
- RuboCop checks pass (zero offenses)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
## Documentation Updates

### 1. Generator Details (api-reference/generator-details.md)
- Added --rspack to generator options list
- Added Rspack section in generator description
- Created comprehensive "Rspack Support" section covering:
  - Benefits (~20x faster builds, unified config, easy switching)
  - What gets installed (packages and configuration)
  - Switching bundlers post-installation
  - Combining with other options (TypeScript, Redux)
  - Link to webpack configuration docs

### 2. Quick Start Guide (getting-started/quick-start.md)
- Added --rspack option example in installation step
- Added performance tip highlighting 20x improvement
- Mentioned bin/switch-bundler utility for later switching

### 3. Webpack Configuration (core-concepts/webpack-configuration.md)
- Added new "Rspack vs. Webpack" section covering:
  - Introduction to Rspack and performance benefits
  - Usage instructions (generation and switching)
  - Performance metrics (build times, SWC benefits)
  - Unified configuration approach with code examples
  - shakapacker.yml configuration details

## Documentation Structure

The documentation is now comprehensive and appears in all appropriate places:
- API Reference: Detailed generator option documentation
- Getting Started: Quick introduction for new users
- Core Concepts: Deep dive into configuration and usage

All docs follow existing formatting conventions and include:
- Clear code examples
- Performance metrics
- Cross-references between docs
- Practical usage instructions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link

claude bot commented Nov 6, 2025

PR Review: Add --rspack generator option for faster builds

I've completed a comprehensive review of this PR. Overall, this is excellent work with a clean implementation that follows the repository's conventions. Here's my detailed feedback:


Strengths

1. Code Quality & Best Practices

  • Follows repository conventions: Generator pattern usage is consistent with existing code
  • Clean separation of concerns: Install generator handles dependencies, base generator handles configuration
  • Unified configuration approach: Smart design that allows same webpack config files to work with both bundlers
  • Proper YAML handling: Uses YAML.load_file and YAML.dump instead of fragile regex manipulation (base_generator.rb:404-426)
  • Security-conscious: Uses array form in system() calls to prevent command injection (switch-bundler:103-120)
  • Follows CLAUDE.md requirements: RuboCop checks pass, trailing newlines present

2. Architecture & Design

  • Conditional dependency installation: Only installs relevant bundler packages
  • Reversible design: bin/switch-bundler script provides excellent post-installation flexibility
  • Package manager agnostic: Auto-detects npm, yarn, and pnpm
  • Backward compatible: Doesn't break existing webpack-only installations
  • Smart plugin detection: Uses runtime detection (config.assets_bundler) rather than build-time configuration

3. Testing & Documentation

  • Comprehensive tests: install_generator_spec.rb:142-231 covers all key scenarios
  • Excellent documentation: RSPACK_IMPLEMENTATION.md is thorough and well-structured
  • User-facing docs updated: Quick-start guide and API reference updated appropriately
  • Proper changelog entry: Follows repository format with PR link and author attribution

💡 Suggestions for Improvement

1. Minor: Missing react-refresh from both bundler paths

Location: install_generator.rb:541-551

Both Webpack and Rspack paths include react-refresh as a dev dependency, which is correct. However, this creates duplication. Consider:

dev_deps = %w[react-refresh] # Common to both
dev_deps += if options.rspack?
              %w[@rspack/cli @rspack/plugin-react-refresh]
            else
              %w[@pmmmwh/react-refresh-webpack-plugin]
            end

This is a minor optimization and not a blocker.

2. Documentation: Add troubleshooting section

Location: RSPACK_IMPLEMENTATION.md

Consider adding a troubleshooting section for common issues users might encounter:

  • What to do if switching fails mid-process
  • How to verify bundler is correctly configured
  • Known compatibility issues or limitations

3. Testing: Consider integration test for bundler switching

Current tests: Only test generator output, not the bin/switch-bundler script

Consider adding a test that:

  1. Generates with webpack
  2. Runs bin/switch-bundler rspack
  3. Verifies config changes and dependencies

This could be a future enhancement and is not a blocker.

4. bin/switch-bundler: Add dry-run mode

Enhancement idea: bin/switch-bundler rspack --dry-run

This would show what changes would be made without actually making them. Useful for users who want to preview the impact. This is a nice-to-have for future iterations.


🔒 Security Considerations

  • No security concerns identified
  • ✅ Proper use of array form in system() calls prevents command injection
  • ✅ YAML parsing is safe (uses standard Ruby YAML library)
  • ✅ No user input directly executed without validation

Performance Considerations

  • Excellent performance benefits: ~20x faster builds with Rspack + SWC
  • No performance regressions: Webpack users unaffected
  • Efficient dependency installation: Only installs what's needed

🧪 Test Coverage

Coverage Assessment: ✅ Good

The tests cover:

  • ✅ Rspack dependency installation
  • ✅ Webpack dependency exclusion
  • ✅ Unified config generation
  • bin/switch-bundler script creation
  • ✅ shakapacker.yml configuration

Potential gaps (non-blocking):

  • Integration test for actual bundler switching
  • Test that generated config works with both bundlers
  • Error handling in switch-bundler script

📊 Overall Assessment

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Excellent, follows all conventions
Architecture ⭐⭐⭐⭐⭐ Smart unified config approach
Testing ⭐⭐⭐⭐☆ Good coverage, room for integration tests
Documentation ⭐⭐⭐⭐⭐ Thorough and well-organized
Security ⭐⭐⭐⭐⭐ No concerns
Performance ⭐⭐⭐⭐⭐ Significant improvement potential

Recommendation: APPROVE

This PR is ready to merge. The implementation is solid, well-tested, properly documented, and follows all repository conventions. The suggestions above are minor enhancements that can be addressed in future PRs if desired.

Key Achievement: The unified configuration approach is particularly elegant - it allows seamless switching between bundlers without maintaining separate config files. This is a significant UX improvement.

Great work @justin808! 🎉


📝 Pre-merge Checklist

Before merging, verify:

  • CI checks passing ✅ (per PR description test plan)
  • All RuboCop offenses resolved ✅ (mentioned in PR description)
  • Trailing newlines present ✅ (verified in file reads)
  • CHANGELOG.md updated ✅ (verified at line 28)
  • No merge conflicts ✅

Review conducted with Claude Code following repository's CLAUDE.md guidelines

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)

539-561: CSS minimizer plugin should only install when NOT using Rspack.

The add_css_dependencies method installs css-minimizer-webpack-plugin unconditionally (line 509 of install_generator.rb), but this is webpack-specific. When using --rspack, Rspack has its own CSS minimization and shouldn't use the webpack plugin.

Additionally, base_generator.rb contains add_dev_dependencies that only installs @pmmmwh/react-refresh-webpack-plugin without any rspack conditional, creating duplicate webpack-only dependencies even when the rspack option is passed to the base generator.

Recommended fixes:

  1. Make add_css_dependencies conditional: exclude css-minimizer-webpack-plugin when options.rspack?
  2. Update base_generator.rb's add_dev_dependencies to respect the rspack option (or remove duplicate method if install_generator methods should be used)
🧹 Nitpick comments (1)
RSPACK_IMPLEMENTATION.md (1)

1-160: LGTM! Thorough implementation documentation.

This document provides excellent coverage of:

  • All code changes across generators and templates
  • Dependency differences between Webpack and Rspack
  • Usage examples and configuration details
  • Performance benefits and compatibility notes

The static analysis hints about missing language specifications for fenced code blocks are optional improvements and don't affect the quality of the documentation.

If you want to address the static analysis hints, add language specifiers to the fenced code blocks on lines 7, 64, and 84:

-```
+```text
 Build times: ~53-270ms

This is purely optional and doesn't impact functionality.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52c85c6 and 117cf47.

📒 Files selected for processing (10)
  • CHANGELOG.md (1 hunks)
  • RSPACK_IMPLEMENTATION.md (1 hunks)
  • docs/api-reference/generator-details.md (3 hunks)
  • docs/core-concepts/webpack-configuration.md (1 hunks)
  • docs/getting-started/quick-start.md (2 hunks)
  • lib/generators/react_on_rails/base_generator.rb (3 hunks)
  • lib/generators/react_on_rails/install_generator.rb (4 hunks)
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler (1 hunks)
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt (1 hunks)
  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • docs/getting-started/quick-start.md
  • docs/api-reference/generator-details.md
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
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.
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: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
📚 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:

  • CHANGELOG.md
  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
  • docs/core-concepts/webpack-configuration.md
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
  • lib/generators/react_on_rails/base_generator.rb
  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
  • RSPACK_IMPLEMENTATION.md
📚 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:

  • CHANGELOG.md
  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
  • docs/core-concepts/webpack-configuration.md
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
  • lib/generators/react_on_rails/base_generator.rb
  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
  • RSPACK_IMPLEMENTATION.md
📚 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:

  • CHANGELOG.md
  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
  • RSPACK_IMPLEMENTATION.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • CHANGELOG.md
  • docs/core-concepts/webpack-configuration.md
  • lib/generators/react_on_rails/base_generator.rb
  • lib/generators/react_on_rails/install_generator.rb
  • RSPACK_IMPLEMENTATION.md
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.

Applied to files:

  • CHANGELOG.md
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.

Applied to files:

  • CHANGELOG.md
📚 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:

  • CHANGELOG.md
  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
  • docs/core-concepts/webpack-configuration.md
  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
  • RSPACK_IMPLEMENTATION.md
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.

Applied to files:

  • lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 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:

  • lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.

Applied to files:

  • lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
🧬 Code graph analysis (2)
lib/generators/react_on_rails/base_generator.rb (1)
lib/react_on_rails/packer_utils.rb (1)
  • shakapacker_version (15-19)
lib/generators/react_on_rails/install_generator.rb (2)
lib/generators/react_on_rails/generator_helper.rb (1)
  • add_npm_dependencies (23-39)
lib/generators/react_on_rails/base_generator.rb (2)
  • handle_npm_failure (248-260)
  • add_dev_dependencies (207-217)
🪛 LanguageTool
RSPACK_IMPLEMENTATION.md

[grammar] ~146-~146: Ensure spelling is correct
Context: ...ccording to PR #20: - Build times: ~53-270ms with Rspack vs typical webpack builds -...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 markdownlint-cli2 (0.18.1)
RSPACK_IMPLEMENTATION.md

7-7: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


84-84: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (13)
lib/generators/react_on_rails/base_generator.rb (3)

22-26: LGTM! Well-defined option.

The rspack option is properly declared with clear description and default value, following the established pattern for other generator options.


404-421: LGTM! YAML parsing correctly implemented.

The YAML-based approach correctly addresses the fragile regex manipulation from the past review. This implementation:

  • Properly parses YAML using YAML.load_file
  • Safely ensures default section exists
  • Sets both assets_bundler and webpack_loader configuration
  • Writes back using YAML.dump

This matches the pattern already used in the switch-bundler script (lines 50-59).


91-91: Verify both configure_rspack_in_shakapacker invocation paths are tested.

Both code paths are present:

  • Line 91: Called when .shakapacker_just_installed marker exists
  • Line 99: Called after copying config when options.rspack? is true

Both call the same method with identical conditions. While install_generator_spec.rb contains tests for --rspack scenarios, explicit test coverage differentiating between these two distinct invocation paths (marker-based vs. copy-based) cannot be confirmed from available test results.

Manually verify that:

  1. Tests cover the fresh Shakapacker install path (when marker file exists)
  2. Tests cover the existing Shakapacker path (normal config copy)
  3. Both scenarios properly invoke configure_rspack_in_shakapacker with rspack option enabled
lib/generators/react_on_rails/templates/base/base/bin/switch-bundler (2)

47-62: LGTM! Consistent YAML handling.

The YAML parsing approach matches the pattern in base_generator.rb and correctly:

  • Validates file existence
  • Parses YAML safely
  • Ensures default section exists
  • Sets bundler-specific configuration (assets_bundler and webpack_loader)
  • Writes back using YAML.dump

100-122: LGTM! Secure system calls.

The installation commands correctly use array form for system() calls, preventing command injection vulnerabilities. The spread operator *deps[:dependencies] properly expands the array.

lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt (2)

6-8: LGTM! Clean bundler abstraction.

The conditional require pattern cleanly supports both Webpack and Rspack:

  • Checks config.assets_bundler at runtime
  • Requires the appropriate bundler module
  • Enables unified configuration

45-45: No issues found—code is correct.

Both Webpack and Rspack expose optimize.LimitChunkCountPlugin at the same API path. The template correctly assigns bundler conditionally at lines 5-7 based on config.assets_bundler, ensuring it references the correct module (either @rspack/core or webpack). Line 45's usage of bundler.optimize.LimitChunkCountPlugin works identically for both bundlers.

CHANGELOG.md (1)

28-28: LGTM! Clear and comprehensive changelog entry.

The changelog entry effectively documents:

  • The new --rspack flag and its purpose
  • Key features (unified config, switch-bundler utility)
  • Performance benefits (~20x improvement claim)
  • PR reference and contributor attribution
lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt (1)

3-3: LGTM! Clean conditional plugin loading.

The implementation correctly:

  • Imports config from shakapacker to access assets_bundler
  • Conditionally loads the appropriate React Refresh plugin based on bundler
  • Handles plugin instantiation properly for both branches
  • Updates comments to be more accurate ("dev server" vs "webpack-dev-server")

Also applies to: 11-23

docs/core-concepts/webpack-configuration.md (1)

14-80: LGTM! Comprehensive and clear documentation.

The documentation effectively covers:

  • What Rspack is and why to use it
  • How to generate with Rspack or switch post-install
  • Performance benefits with specific metrics
  • Unified configuration approach with code examples
  • Configuration details for shakapacker.yml

The examples clearly demonstrate the conditional plugin loading pattern implemented in the actual templates.

lib/generators/react_on_rails/install_generator.rb (3)

31-35: LGTM! Consistent option declaration.

The rspack option follows the established pattern used for other generator options (redux, typescript) with clear description and default value.


105-106: LGTM! Option properly propagated.

The rspack option is correctly passed to the base generator invocation alongside typescript and redux options.


523-537: LGTM! Rspack dependencies correctly installed.

The method follows the established pattern:

  • Uses add_npm_dependencies helper with proper error handling
  • Falls back to direct npm commands if helper fails
  • Tracks installation state with instance variables
  • Installs core Rspack packages (@rspack/core, rspack-manifest-plugin)

Comment on lines +10 to +18
WEBPACK_DEPS = {
dependencies: %w[webpack webpack-assets-manifest webpack-merge],
dev_dependencies: %w[webpack-cli webpack-dev-server @pmmmwh/react-refresh-webpack-plugin]
}.freeze

RSPACK_DEPS = {
dependencies: %w[@rspack/core rspack-manifest-plugin],
dev_dependencies: %w[@rspack/cli @rspack/plugin-react-refresh]
}.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify webpack-dev-server and webpack-cli are in correct lists.

The constants define dependencies, but I notice:

  • webpack-cli and webpack-dev-server are in dev_dependencies
  • @rspack/cli is in dev_dependencies

This seems correct, but verify that these tools are properly classified as dev dependencies for both bundlers.


🏁 Script executed:

#!/bin/bash
# Check how webpack-dev-server and webpack-cli are used in package.json templates
fd -e json package.json lib/generators | xargs cat | jq '.devDependencies | keys[] | select(. | contains("webpack"))'

Length of output: 0


I need to gather more information to verify the dependency classification. Let me explore the repository structure and search for relevant documentation on bundler dependencies.





---

🏁 Script executed:

```shell
#!/bin/bash
# Find the switch-bundler file to verify it exists
fd -type f "switch-bundler" lib/generators

Length of output: 170


🌐 Web query:

webpack-dev-server webpack-cli devDependencies peerDependencies classification npm

💡 Result:

Short answer

  • For application projects: install webpack, webpack-cli and webpack-dev-server as devDependencies (npm i -D webpack webpack-cli webpack-dev-server). [1][2]
  • For libraries/plugins that integrate with webpack (loaders, plugins): declare webpack as a peerDependency (so the host app provides it) and also include a matching webpack in devDependencies for local dev/tests. [3][4]

Sources

  • webpack-dev-server install & usage recommends local, dev install. [1][2]
  • npm peerDependencies behavior and best practice (libraries declare peers; authors often keep peers in devDependencies for testing). [3][4]

🌐 Web query:

rspack @rspack/cli devDependencies npm package classification

💡 Result:

Short answer: @rspack/cli is a command‑line tool meant to be installed as a development dependency (devDependency), not a runtime dependency. The official quick‑start and migration docs show installing @rspack/core and @rspack/cli with -D (devDependencies). [1][2][3]

Note: @rspack packages (including @rspack/cli) were subject to a December 2024 supply‑chain incident; follow the project's guidance and use patched/official versions. [4]

Sources: [1] rspack quick start, [2] rspack CLI docs, [3] @rspack/cli npm page, [4] Socket security writeup.






🏁 Script executed:

#!/bin/bash
# Corrected search for switch-bundler file
fd -t f "switch-bundler" lib/generators

Length of output: 68


🌐 Web query:

rspack-manifest-plugin npm devDependencies

💡 Result:

Install it as a devDependency:

npm install --save-dev rspack-manifest-plugin. [1]

Source: npm package page for rspack-manifest-plugin. [1]

References
[1] rspack-manifest-plugin — npm. [turn2search1]



---

🌐 Web query:

```web
webpack-assets-manifest devDependencies npm

💡 Result:

Use the package name "webpack-assets-manifest" and install it as a devDependency. Example npm commands:

  • npm install --save-dev webpack-assets-manifest
  • npm i -D webpack-assets-manifest

Current npm page (shows latest version and install instructions). [1][2]

Sources:
[1] npm package page for webpack-assets-manifest.
[2] npm install instructions (example v6+ / v3 notes).



---

🌐 Web query:

```web
webpack-merge devDependencies npm package

💡 Result:

Do you want (A) the list of devDependencies inside the webpack-merge package.json (for a specific version), or (B) instructions to install webpack-merge as a devDependency in your project?



---

🌐 Web query:

```web
webpack-merge npm install save-dev or dependencies

💡 Result:

Use a devDependency. webpack-merge is a build tool used only when assembling webpack configs, so install with --save-dev:

npm:
npm install --save-dev webpack-merge

yarn:
yarn add --dev webpack-merge

Sources: webpack guide recommending --save-dev and the webpack-merge npm page. (webpack.js.org)

Citations:


Move three build-time tools from dependencies to dev_dependencies.

webpack-assets-manifest, rspack-manifest-plugin, and webpack-merge should all be installed as devDependencies, not dependencies. These are build-time only tools:

  • webpack-assets-manifest and rspack-manifest-plugin generate manifests during the build process
  • webpack-merge is used only to assemble webpack configurations

Move these three packages to the dev_dependencies hash in both WEBPACK_DEPS and RSPACK_DEPS constants.

🤖 Prompt for AI Agents
In lib/generators/react_on_rails/templates/base/base/bin/switch-bundler around
lines 10 to 18, three build-time packages are incorrectly listed under
dependencies; move webpack-assets-manifest, rspack-manifest-plugin, and
webpack-merge from the dependencies arrays into the dev_dependencies arrays for
both WEBPACK_DEPS and RSPACK_DEPS so they are installed as devDependencies
(build-time only) rather than runtime dependencies.

justin808 and others added 2 commits November 6, 2025 13:38
## Changes

1. **Update main README**
   - Added Rspack to "What's New" section as first bullet point
   - Added Rspack to "Why React on Rails?" features table
   - Highlights ~20x faster builds with links to documentation

2. **Address comment #1: Remove redundant --no-rspack option**
   - File: docs/api-reference/generator-details.md:14
   - Removed [--no-rspack] from options list (redundant, default is false)
   - Kept only [--rspack] for clarity

3. **Address comment #2: Document switch-bundler limitations**
   - File: docs/getting-started/quick-start.md:47
   - Added detailed note about limitations in quick-start
   - File: docs/api-reference/generator-details.md
   - Added comprehensive "Limitations of bin/switch-bundler" section:
     * Custom webpack plugins not auto-updated
     * Manual updates needed for custom configs
     * Third-party webpack packages not detected
     * YAML formatting may change (but preserves functionality)
   - Cross-references to unified configuration docs

4. **TypeScript handling confirmed working**
   - Rspack uses SWC which handles TypeScript natively
   - No .ts file updates needed
   - --rspack --typescript combination works seamlessly

## Documentation Now Complete

The Rspack feature is now documented in:
- Main README (prominent placement)
- Quick Start guide (with limitations)
- Generator Details (comprehensive with limitations)
- Webpack Configuration (technical details)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Rails automatically generates both --option and --no-option forms for all
boolean generator options. The documentation should be consistent.

Since --redux and --typescript both show [--option], [--no-option] format,
rspack should follow the same pattern for consistency.

While --no-rspack is redundant (default is false), it matches the existing
documentation style and what Rails actually provides.
@justin808 justin808 merged commit 0b00514 into master Nov 7, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants