Fix #2549: clean stale webpack config on --rspack install#2597
Fix #2549: clean stale webpack config on --rspack install#2597
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ERB-aware cleanup to the install generator: when run with --rspack, the generator detects stock Changes
Sequence DiagramsequenceDiagram
participant Generator as ReactOnRails Generator
participant Cleaner as Cleanup Logic
participant ERB as ERB Template Engine
participant FS as File System
Generator->>Cleaner: copy_webpack_config (--rspack) calls cleanup
Cleaner->>FS: check for `config/webpack/` directory
alt config/webpack exists
Cleaner->>FS: list entries in config/webpack/
loop for each entry
Cleaner->>ERB: render managed template for entry
ERB-->>Cleaner: rendered template content
Cleaner->>FS: read actual entry content
Cleaner->>Cleaner: compare contents -> removable?
end
alt all entries removable
Cleaner->>FS: remove `config/webpack/` directory
else non-removable entries present
Cleaner->>Generator: warn and preserve custom files
end
end
Generator->>FS: generate/ensure `config/rspack/` files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474a07528d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds a Two behavioral issues were found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cleanup_stale_webpack_config_dir_for_rspack] --> B{using_rspack?}
B -- No --> Z[Return early - no-op]
B -- Yes --> C{Dir.exist? config/webpack}
C -- No --> Z
C -- Yes --> D[List non-hidden entries]
D --> E{entries empty?}
E -- Yes --> Z
E -- No --> F[For each entry: removable_webpack_entry?]
F --> G{entry in REMOVABLE_WEBPACK_FILES?}
G -- No --> H[non_removable]
G -- Yes --> I{Is it a file?}
I -- No --> H
I -- Yes --> J{entry == webpack.config.js OR generateWebpackConfigs.js}
J -- Yes --> K[standard_shakapacker_config? OR react_on_rails_config?]
K -- True --> L[removable]
K -- False --> H
J -- No --> M[MANAGED_WEBPACK_FILE_TEMPLATES lookup]
M --> N{template found?}
N -- No --> H
N -- Yes --> O[rendered_template_for_cleanup - renders with CURRENT options]
O --> P[content_matches_template?]
P -- Match --> L
P -- No match --> H
H --> Q{All entries removable?}
L --> Q
Q -- Yes --> R[remove_dir config/webpack]
Q -- No --> S[say_status warning - keep directory]
style H fill:#f96,stroke:#333
style J fill:#ff9,stroke:#333
style O fill:#ff9,stroke:#333
classDef bug fill:#f96
class K bug
Last reviewed commit: 474a075 |
PR Review: Fix stale webpack config cleanup on --rspack install |
PR Review: Fix stale webpack config cleanup on --rspack installOverall: The approach is well-conceived. Template-content matching to distinguish stock files from user-customised ones is the right strategy. The code is clean and includes useful specs. A few correctness issues are worth addressing before merging. CriticalOption-dependent template rendering can mis-classify stock files as custom
Concrete failure scenario: a user originally installed with The same issue applies to Possible mitigations:
Minor
Matching on The removal-path test is implicit The spec at line 559 asserts the directory no longer exists, but it would also pass if the directory was never created. A more explicit test would assert the directory existed with at least one stock file before the generator ran, or would spy on Custom file warning does not say which stock files would have been cleaned When custom files are detected, the warning only lists the unknown entries. The user cannot tell that the stock files sitting next to the custom ones would have been removed if no custom files were present--making it harder to decide whether to clean up manually. Intent of the hidden-file filter deserves a comment
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
react_on_rails/lib/generators/react_on_rails/base_generator.rb (2)
335-344: Consider extracting the shared config hash.The
confighash on line 338 duplicates the one defined incopy_webpack_config(line 134). If the message changes in one place, they could diverge.♻️ Optional: Extract shared config to a constant or method
+ TEMPLATE_CONFIG = { + message: "// The source code including full typescript support is available at:" + }.freeze + def copy_webpack_config cleanup_stale_webpack_config_dir_for_rspack say "Adding #{using_rspack? ? 'Rspack' : 'Webpack'} config" base_path = "base/base" base_files = %w[babel.config.js config/webpack/clientWebpackConfig.js # ... - config = { - message: "// The source code including full typescript support is available at:" - } base_files.each do |file| - template("#{base_path}/#{file}.tt", destination_config_path(file), config) + template("#{base_path}/#{file}.tt", destination_config_path(file), TEMPLATE_CONFIG) endAnd in
rendered_template_for_cleanup:def rendered_template_for_cleanup(template_path) `@rendered_template_cache` ||= {} `@rendered_template_cache`[template_path] ||= begin - config = { message: "// The source code including full typescript support is available at:" } template_content = File.read(File.join(self.class.source_root, template_path)) ERB.new(template_content, trim_mode: "-").result(binding) end endNote: The
TEMPLATE_CONFIGconstant would be accessible viabindingwhen rendering templates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/base_generator.rb` around lines 335 - 344, The config hash used in rendered_template_for_cleanup is duplicated from copy_webpack_config; extract that shared hash into a single source (e.g., a constant TEMPLATE_CONFIG or a private method template_config) and replace the inline hashes in both rendered_template_for_cleanup and copy_webpack_config to reference it; ensure the chosen constant/method name (TEMPLATE_CONFIG or template_config) is in scope so ERB.new(...).result(binding) can access it when rendering templates.
320-326: Clarify or removegenerateWebpackConfigs.jshandling.The case branch handles
generateWebpackConfigs.jsalongsidewebpack.config.js, but both are compared against the samewebpack.config.js.tttemplate. SincegenerateWebpackConfigs.jscontent won't matchwebpack.config.js.tt, it will always be treated as non-removable (triggering a warning).If this is intentional future-proofing, a comment would help clarify. If
generateWebpackConfigs.jsis not expected to exist in the wild, consider removing it fromREMOVABLE_WEBPACK_FILESto avoid confusion.🔧 Optional: Remove generateWebpackConfigs.js if unused
- REMOVABLE_WEBPACK_FILES = (MANAGED_WEBPACK_FILE_TEMPLATES.keys + - %w[webpack.config.js generateWebpackConfigs.js]).freeze + REMOVABLE_WEBPACK_FILES = (MANAGED_WEBPACK_FILE_TEMPLATES.keys + + %w[webpack.config.js]).freezeAnd update the case statement:
- when "webpack.config.js", "generateWebpackConfigs.js" + when "webpack.config.js"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/base_generator.rb` around lines 320 - 326, The case branch handling "generateWebpackConfigs.js" alongside "webpack.config.js" uses the same template check (rendered_template_for_cleanup with "base/base/config/webpack/webpack.config.js.tt") so "generateWebpackConfigs.js" will never match and will always be treated non-removable; either remove "generateWebpackConfigs.js" from REMOVABLE_WEBPACK_FILES and delete its case handling, or explicitly handle it by adding a separate template or matching logic for "generateWebpackConfigs.js" (update the case in the method that branches on entry and adjust calls to rendered_template_for_cleanup/content_matches_template?/standard_shakapacker_config? accordingly), or if intentional, add a clear comment above the case explaining the future-proofing intent referencing entry, rendered_template_for_cleanup, and REMOVABLE_WEBPACK_FILES so maintainers understand why it’s present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/generators/react_on_rails/base_generator.rb`:
- Around line 335-344: The config hash used in rendered_template_for_cleanup is
duplicated from copy_webpack_config; extract that shared hash into a single
source (e.g., a constant TEMPLATE_CONFIG or a private method template_config)
and replace the inline hashes in both rendered_template_for_cleanup and
copy_webpack_config to reference it; ensure the chosen constant/method name
(TEMPLATE_CONFIG or template_config) is in scope so ERB.new(...).result(binding)
can access it when rendering templates.
- Around line 320-326: The case branch handling "generateWebpackConfigs.js"
alongside "webpack.config.js" uses the same template check
(rendered_template_for_cleanup with
"base/base/config/webpack/webpack.config.js.tt") so "generateWebpackConfigs.js"
will never match and will always be treated non-removable; either remove
"generateWebpackConfigs.js" from REMOVABLE_WEBPACK_FILES and delete its case
handling, or explicitly handle it by adding a separate template or matching
logic for "generateWebpackConfigs.js" (update the case in the method that
branches on entry and adjust calls to
rendered_template_for_cleanup/content_matches_template?/standard_shakapacker_config?
accordingly), or if intentional, add a clear comment above the case explaining
the future-proofing intent referencing entry, rendered_template_for_cleanup, and
REMOVABLE_WEBPACK_FILES so maintainers understand why it’s present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8860f048-6739-497c-92fc-79d353cfe68a
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
|
test comment |
AGENTS.mdInstructions for AI coding agents working on the React on Rails codebase. React on Rails is a Ruby gem + npm package that integrates React with Ruby on Rails, providing server-side rendering (SSR) via Node.js or ExecJS. This is a monorepo: the open-source gem lives at Canonical Agent Policy
Other agent-facing docs (for example Commands# Install dependencies
bundle && pnpm install
# Build TypeScript → JavaScript
pnpm run build
# Lint (MANDATORY before every commit)
bundle exec rubocop # Ruby — must pass with zero offenses
pnpm run lint # JS/TS via ESLint
pnpm start format.listDifferent # Check Prettier formatting
rake lint # All linting (Ruby + JS + formatting)
# Auto-fix formatting
rake autofix # Preferred for all formatting
# Run tests
rake run_rspec:gem # Ruby unit tests (gem code)
rake run_rspec:dummy # Ruby integration tests (dummy Rails app)
pnpm run test # JavaScript/TypeScript tests
rake # Full suite (lint + all tests except examples)
# Type checking
pnpm run type-check # TypeScript
bundle exec rake rbs:validate # RBS signatures
# Additional test subsets
rake run_rspec # All Ruby tests
rake all_but_examples # All tests except generated examples
rake run_rspec:shakapacker_examples_basic # Single example test
# Full initial setup
bundle && pnpm install && rake shakapacker_examples:gen_all && rake node_package && rake
# CI/workflow linting
actionlint # GitHub Actions lint
yamllint .github/ # YAML lint (do NOT run RuboCop on .yml files)
# Dependency version updates
rake shakapacker:update_version[9.6.1] # Update shakapacker across the monorepoUpdating ShakapackerUse The task handles Ruby version switching for apps that require a different Ruby version (set Testing
Run specific test files: bundle exec rspec react_on_rails/spec/react_on_rails/path/to/spec.rb
cd react_on_rails/spec/dummy && bundle exec rspec spec/path/to/spec.rbProject Structure
Code StyleRuby (RuboCop)Line length max 120 characters. Run Line length — break long chains: # Bad
content = pack_content.gsub(/import.*from.*['"];/, "").gsub(/ReactOnRails\.register.*/, "")
# Good
content = pack_content.gsub(/import.*from.*['"];/, "")
.gsub(/ReactOnRails\.register.*/, "")Named subjects in RSpec: # Bad
subject { instance.method_name(arg) }
# Good
subject(:method_result) { instance.method_name(arg) }Security violations — scope disable comments tightly: # rubocop:disable Security/Eval
expect { evaluate(sanitized_content) }.not_to raise_error
# rubocop:enable Security/EvalJavaScript/TypeScriptPrettier handles all formatting. Never manually format — run Git WorkflowBranch naming: Commit messages: Explain why, not what. One logical change per commit. PR creation: Use Review WorkflowFor small, focused PRs (roughly 5 files changed or fewer and one clear purpose):
BoundariesAlways
Ask First
Never
Key Concept: File Suffixes vs. RSC DirectiveReact on Rails has two independent systems that both use "client" and "server" terminology. Do not confuse them. 1. Bundle Placement (
|
Change LogAll notable changes to this project's source code will be documented in this file. Items under
Migration instructions for the major updates can be found here. Some smaller migration information can be found here. Want to Save Time Updating?If you need help upgrading For an overview of working with us, see our Client Engagement Model article and how we bill for time. If you think ShakaCode can help your project, click here to book a call with Justin Gordon, the creator of React on Rails and Shakapacker. ContributorsPlease follow the recommendations outlined at keepachangelog.com. Please use the existing headings and styling as a guide. VersionsUnreleased16.4.0.rc.9 - 2026-03-12Improved
Fixed
Changed
ProAdded
Changes since the last non-beta release. Added
Fixed
ProFixed
Improved
Fixed
Added
Changed
Improved
Fixed
16.3.0 - 2026-02-05Changed
Fixed
ProChanged
Added
16.2.1 - 2026-01-18Fixed
Developer (Contributors Only)
16.2.0 - 2026-01-14Breaking Changes
Migration Guide: To migrate to React on Rails Pro:
Note: If you're not using any of the Pro-only methods listed above, no changes are required.
Added
Changed
Improved
Fixed
Security
Documentation
Developer (Contributors Only)
ProIncludes all features from the React on Rails Pro 4.0.0 release series (previously released as 4.0.0-rc.6 through 4.0.0-rc.15). For pre-monorepo Pro history, see the archived Pro CHANGELOG. Breaking Changes
Added
Changed
Fixed
Security
Deprecated
16.1.1 - 2025-09-24Bug Fixes
16.1.0 - 2025-09-23New Features
Deprecations
API Improvements
Security Enhancements
Pro License Features
Generator Improvements
Bug Fixes
Code Improvements
16.0.0 - 2025-09-16React on Rails v16 is a major release that modernizes the library with ESM support, removes legacy Webpacker compatibility, and introduces significant performance improvements. This release builds on the foundation of v14 with enhanced RSC (React Server Components) support and streamlined configuration. See Release Notes for complete migration guide. Major Enhancements🚀 React Server Components (RSC) -- Requires React on Rails Pro
⚡ Performance & Loading Strategy
Developer Experience
Breaking Changes🔧 Webpacker Support Removed
📦 Package System Modernization
⚡ Configuration API Changes
🔄 Async API Changes
🏗️ Runtime Suggested Versions
🎯 Generator Improvements
[15.0.0] - 2025-08-28 - RETRACTEDThe 14.2.0 - 2025-03-03Added
Fixed
Changed
14.1.1 - 2025-01-15Fixed
14.1.0 - 2025-01-06Fixed
Added
Changed
14.0.5 - 2024-08-20Fixed
14.0.4 - 2024-07-02ImprovedChanged
14.0.3 - 2024-06-28Fixed
14.0.2 - 2024-06-11Fixed14.0.1 - 2024-05-16Fixed
14.0.0 - 2024-04-03Major bump because dropping support for Ruby 2.7 and deprecated Removed
Fixed
Added
13.4.0 - 2023-07-30Fixed
ImprovedAddedChanged
13.3.5 - 2023-05-31Fixed
13.3.4 - 2023-05-23Added
Removed13.3.3 - 2023-03-21Fixed13.3.2 - 2023-02-24Fixed
13.3.1 - 2023-01-30Added
Fixed13.3.0 - 2023-01-29Fixed
Added
13.2.0 - 2022-12-23Fixed
Added13.1.0 - 2022-08-20Improved
Fixed
13.0.2 - 2022-03-09Fixed
13.0.1 - 2022-02-09Improved13.0.0 - 2022-02-08Breaking
Fixed
12.6.0 - 2022-01-22Added
12.5.2 - 2021-12-29Fixed
12.5.1 - 2021-12-27Fixed
12.5.0 - 2021-12-26Added
Changed
12.4.0 - 2021-09-22Added
12.3.0 - 2021-07-26Added
12.2.0 - 2021-03-25Added
12.1.0 - 2021-03-23Added
Fixed
12.0.4 - 2020-11-14Fixed
12.0.3 - 2020-09-20Fixed
12.0.2 - 2020-07-09Fixed12.0.1 - 2020-07-09Fixed
12.0.0 - 2020-07-08For upgrade instructions, see the upgrading guide. Major Improvements
BREAKING CHANGEIn order to solve the issues regarding React Hooks compatibility, the number of parameters See docs/guides/upgrading-react-on-rails Other Updates
11.3.0 - 2019-05-24Added
11.2.2 - 2018-12-24Improved
11.2.1 - 2018-12-06MIGRATION for v11.2
Improved
Changed
11.2.0 - 2018-12-06Do not use. Unpublished. Caused by an issue with the release script. 11.1.8 - 2018-10-14Improved
11.1.7 - 2018-10-10Fixed
11.1.6 - 2018-10-05Fixed
11.1.5 - 2018-10-03Fixed
11.1.4 - 2018-09-12Fixed
11.1.3 - 2018-08-26Fixed
11.1.2 - 2018-08-18Fixed
11.1.1 - 2018-08-09Fixed
11.1.0 - 2018-08-07Added
Fixed
11.0.10 - 2018-07-22Fixed
11.0.9 - 2018-06-24
11.0.8 - 2018-06-15Fixed
Changed
11.0.7 - 2018-05-16Fixed11.0.6 - 2018-05-15Changed
11.0.5 - 2018-05-11Changed11.0.4 - 2018-05-3Changed
11.0.3 - 2018-04-24Fixed
11.0.2 - 2018-04-24Fixed11.0.1 - 2018-04-23AddedFixed11.0.0 - 2018-04-21MIGRATION for v11
Enhancements: Better Error Messages, Support for React on Rails Pro
Fixes
10.1.4 - 2018-04-11Fixed
10.1.3 - 2018-02-28Fixed
10.1.2 - 2018-02-27Fixed
10.1.1 - 2018-01-26Fixed10.1.0 - 2018-01-23Added
Fixed
10.0.2 - 2017-11-10Fixed
10.0.1 - 2017-10-28Fixed
10.0.0 - 2017-10-08Created
Deprecated
9.0.3 - 2017-09-20Improved9.0.2 - 2017-09-10Fixed9.0.1 - 2017-09-10Fixed9.0.0 - 2017-09-06Updated React on Rails to depend on rails/webpacker. PR 908 by justin808. 9.0 from 8.x. Upgrade InstructionsMoved to our documentation. 8.0.7 - 2017-08-16Fixed
8.0.6 - 2017-07-19Fixed
8.0.5 - 2017-07-04Fixed
Note: 8.0.4 skipped. 8.0.3 - 2017-06-19Fixed
8.0.2 - 2017-06-04Fixed
8.0.1 - 2017-05-30Fixed8.0.0 - 2017-05-29
8.0.0-beta.3 - 2017-05-27Changed
8.0.0-beta.2 - 2017-05-08ChangedRemoved unnecessary values in default paths.yml files for generators. #834 by justin808. 8.0.0-beta.1 - 2017-05-03AddedSupport for WebpackerLite in the generators. #822 by kaizencodes and justin808. ChangedBreaking change is that the default value of symlink_non_digested_assets_regex has changed from this 7.0.4 - 2017-04-277.0.3 - 2017-04-27Same as 7.0.1. 7.0.2 - 2017-04-27Accidental release of beta gem here 7.0.1 - 2017-04-27Fixed7.0.0 - 2017-04-25ChangedFixed6.10.1 - 2017-04-23Fixed
6.10.0 - 2017-04-13Added
Fixed6.9.3 - 2017-04-03Fixed6.9.2 - 2017-04-02Changed
Fixed
6.9.1 - 2017-03-30Fixed6.9.0 - 2017-03-29FixedChanged
Added
6.8.2 - 2017-03-24Fixed
6.8.1 - 2017-03-21Fixed
6.8.0 - 2017-03-06Added6.7.2 - 2017-03-05Improved
6.7.1 - 2017-02-28No changes other than a test fix. 6.7.0 - 2017-02-28IMPORTANT
Commenting out this line addresses the issue: Added
Fixed
6.6.0 - 2017-02-18Added6.5.1 - 2017-02-11Fixed
6.5.0 - 2017-01-31Added
Fixed
6.4.2 - 2017-01-17Fixed
6.4.1 - 2017-1-17No changes. 6.4.0 - 2017-1-12Possible Breaking Change
FixedAdded
6.3.5 - 2017-1-6Fixed
6.3.4 - 2016-12-25Fixed6.3.3 - 2016-12-25Fixed
6.3.2 - 2016-12-5Fixed
6.3.1 - 2016-11-30Changed6.3.0 - 2016-11-30Changed
6.2.1 - 2016-11-19
6.2.0 - 2016-11-19Changed
6.1.2 - 2016-10-24Fixed
6.1.1 - 2016-09-09Fixed
6.1.0 - 2016-08-21Added
Fixed
6.0.5 - 2016-07-11Added6.0.4 - 2016-06-13Fixed6.0.3 - 2016-06-07Fixed
6.0.2 - 2016-06-06Fixed
6.0.1 - 2016-05-27Fixed
6.0.0 - 2016-05-25Breaking Changes
Here is the addition to the generated config file: # This configures the script to run to build the production assets by webpack. Set this to nil
# if you don't want react_on_rails building this file for you.
config.build_production_command = "npm run build:production"
# If you are using the ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config)
# with rspec then this controls what npm command is run
# to automatically refresh your webpack assets on every test run.
config.npm_build_test_command = "npm run build:test"Fixed
Added
Changed
Removed
5.2.0 - 2016-04-08Added
Removed
5.1.1 - 2016-04-04Fixed
Added
5.1.0 - 2016-04-03AddedAll 5.1.0 changes can be found in #362 by justin808.
5.0.0 - 2016-04-01Added
Changed
Breaking Changes
4.0.3 - 2016-03-17Fixed
4.0.2 - 2016-03-17Fixed
4.0.1 - 2016-03-16Fixed
4.0.0 - 2016-03-14Added
Changed
Fixed
Breaking Changes
3.0.6 - 2016-03-01Fixed3.0.5 - 2016-02-26Fixed3.0.4 - 2016-02-25Fixed
3.0.3 - 2016-02-21Fixed
3.0.2 - 2016-02-15Fixed
3.0.1 - 2016-02-15Fixed
3.0.0 - 2016-02-15Fixed
Added
Breaking Change
Migration Steps v2 to v3
RSpec.configure do |config|
# Ensure that if we are running js tests, we are using latest webpack assets
ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config)
2.3.0 - 2016-02-01Added
2.2.0 - 2016-01-29Added
2.1.1 - 2016-01-28Fixed
Added
2.1.0 - 2016-01-26Added
Changed
Fixed
2.0.2
2.0.1
2.0.0
Best done with Object destructing: Previously, you used This would pollute the global namespace. See details in the README.md for more information.
such as:
Migration Steps v1 to v2
1.2.2Fixed
1.2.0Added
Fixed
Removed
1.1.1 - 2015-11-28Added
Fixed
|
|
PR Review: Fix 2549 - Clean stale webpack config on --rspack install The approach is well-designed overall: conservative deletion (only removes known stock files), content-matching for safety, caching rendered templates, and a clear warning when custom files are present. However, there is a critical logic bug that will cause the legacy generateWebpackConfigs.js test to fail, plus a few smaller issues worth addressing. CRITICAL BUG: generateWebpackConfigs.js is never removable In removable_webpack_entry? (around line 321 of base_generator.rb), generateWebpackConfigs.js is grouped with webpack.config.js in the case/when clause and compared against the webpack.config.js.tt template and the standard Shakapacker configs. But the content of generateWebpackConfigs.js (the serverClientOrBoth pattern from legacy RoR versions) looks nothing like webpack.config.js. Neither comparison will ever be true, so the method returns false, making the file permanently non-removable. This means the test removes legacy generateWebpackConfigs.js along with stale config/webpack directory will fail: generateWebpackConfigs.js lands in non_removable_entries, the directory is preserved, and the assertion expect(File).not_to exist(destination_root + config/webpack) fails. Fix: Map generateWebpackConfigs.js to ServerClientOrBoth.js.tt in MANAGED_WEBPACK_FILE_TEMPLATES (it is the legacy name for the same content), then let the else branch in removable_webpack_entry? handle the content check: MINOR: dotfiles in config/webpack are silently deleted The non-removable check excludes dotfiles, which avoids false positives. But remove_dir deletes the entire directory including dotfiles like .gitkeep without mentioning them. A user who added a .gitkeep to track the directory in git would lose it silently. Consider warning about dotfiles present, or including them in the non-removable entries check. MINOR: raise in before(:all) for precondition assertion Using raise inside before(:all) causes every example in that context to report an unexpected error rather than a clear setup failure. Prefer moving the precondition check into its own it block or a helper so failures produce an actionable RSpec message. MINOR: Full generator context exposed via binding in ERB In rendered_template_for_cleanup, .result(binding) passes the generators entire method context into the ERB template. This is intentional (templates call use_pro?, use_rsc?, etc.) and not a security issue since templates are source-controlled, but a typo in a template that happens to be valid Ruby will silently evaluate rather than raising. Worth documenting or using a tighter binding if templates expand in the future. |
|
Overall this is a well-structured solution. The conservative approach (preserve on any doubt, warn rather than delete) is the right default for a generator touching user files. The new specs cover the main paths well. A few things worth addressing: 1. Dotfile-only directories are silently skipped
2.
3. Template rendering is options-dependent — not tested for the
4. Using Minor nit: |
| def add_documentation_reference(*args) | ||
| generator.__send__(:add_documentation_reference, *args) | ||
| end | ||
|
|
||
| def use_pro? | ||
| generator.__send__(:use_pro?) | ||
| end | ||
|
|
||
| def use_rsc? | ||
| generator.__send__(:use_rsc?) | ||
| end | ||
|
|
||
| def shakapacker_version_9_or_higher? | ||
| generator.__send__(:shakapacker_version_9_or_higher?) | ||
| end |
There was a problem hiding this comment.
Using __send__ to forward to private generator methods works, and failures are caught by the StandardError rescue in rendered_template_for_cleanup (treating the file as non-removable — safe). However, if any of these private methods are later renamed, the breakage is silent: the cleanup silently becomes maximally conservative without any indication why. A test that exercises the happy-path ERB rendering path (not just the failure sentinel) would catch rename-regressions earlier. Alternatively, a short comment cross-referencing the rescue contract here would help future maintainers understand the intentional coupling.
| def stale_webpack_config_dir | ||
| File.join(destination_root, "config/webpack") | ||
| end |
There was a problem hiding this comment.
The string "config/webpack" is hardcoded in two places: here as the full-path base, and as the local webpack_config_relative_dir in cleanup_stale_webpack_config_dir_for_rspack. If the path ever needs to change, both must be updated in sync. Consider deriving one from the other:
| def stale_webpack_config_dir | |
| File.join(destination_root, "config/webpack") | |
| end | |
| def stale_webpack_config_dir | |
| File.join(destination_root, stale_webpack_config_relative_dir) | |
| end | |
| def stale_webpack_config_relative_dir | |
| "config/webpack" | |
| end |
Then cleanup_stale_webpack_config_dir_for_rspack can use webpack_config_relative_dir = stale_webpack_config_relative_dir and avoid the duplication.
| def rendered_template_for_cleanup(template_path) | ||
| @rendered_template_cache ||= {} | ||
| @rendered_template_cache[template_path] ||= begin |
There was a problem hiding this comment.
The ||= pattern works correctly here because TEMPLATE_RENDER_FAILED is a truthy Object instance, so the block is never re-executed once it has been cached. However, there is one subtle gap: if File.read returns an empty string (a zero-byte template), @rendered_template_cache[template_path] would store "". In Ruby "" is truthy, so ||= would not re-run — this is correct. But content_matches_template?("", "") would return true, meaning a zero-byte file would be treated as removable. This edge case is benign for real templates, but worth a brief note if this cache is ever reused for other template types.
Review SummaryThis is a well-designed, conservative implementation of the stale webpack config cleanup. The overall approach - only deleting files that exactly match known generated content, warning and preserving everything else - is the right call for a destructive generator step. The test coverage is thorough and the intent is clearly documented. Strengths
Issues / SuggestionsSee inline comments, summarized here:
None of these are blockers - the logic is sound and the implementation is well-thought-out. |
Add PR #2597 (clean stale webpack config on --rspack install) to changelog and stamp 16.4.0.rc.10 version header. Also update the /update-changelog skill to: - Auto-commit, push, and open a PR after stamping a version - Ask for user confirmation on ambiguous version bumps Co-Authored-By: Claude Opus 4.6 <[email protected]>
Master's PR #2597 added strip_comments: keyword to standard_shakapacker_config?. The copy_webpack_main_config call site passes strip_comments: true, so test stubs must match. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
## Summary - **Add PR #2597** (clean stale webpack config on `--rspack` install) to changelog under `#### Fixed` - **Stamp `16.4.0.rc.10`** version header with today's date - **Improve `/update-changelog` skill**: auto-commit/push/PR after stamping, ask for confirmation on ambiguous version bumps ## Test plan - [ ] Verify CHANGELOG.md formatting and version links are correct - [ ] Verify `/update-changelog` skill reads correctly for future invocations 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only updates to the changelog and the Claude `/update-changelog` command instructions; no runtime code paths changed. > > **Overview** > Stamps a new `### [16.4.0.rc.10] - 2026-03-14` section in `CHANGELOG.md`, folding in the `--rspack` stale `config/webpack/` cleanup fix and updating the compare links. > > Updates the `/update-changelog` command docs to (1) require explicit user confirmation when the suggested bump type is ambiguous and (2) switch release/rc/beta mode guidance from manual “commit/push” steps to **automatically creating a branch, committing, pushing, and opening a PR** after stamping. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 596cc76. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ruby 3.4 heredoc compatibility, pnpm workspace setup guard, SSR error serialization, CSS module SSR behavior, stale webpack cleanup, and private path handling on fresh installs. * **New Features** * Explicit version stamping for changelogs (supports .rc/.beta), clearer prerelease/RC/Beta flows, automated changelog PR creation, and consolidation of duplicate changelog headings. * **Tests** * Added/updated tests to verify changelog consolidation and ordering. * **Chores** * Improved bump confirmation messaging and expanded workflow documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
Master's PR #2597 added strip_comments: keyword to standard_shakapacker_config?. The copy_webpack_main_config call site passes strip_comments: true, so test stubs must match. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
## Summary - remove stale files when running - only remove when files are known stock/generated webpack config files - preserve directory and warn when custom files are present - add generator specs for both cleanup and preservation paths Fixes #2549 ## Testing - Inspecting 2 files .. 2 files inspected, no offenses detected - Run options: include {focus: true, locations: {"./react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb" => [559, 632]}} .. Finished in 44.88 seconds (files took 1.28 seconds to load) 2 examples, 0 failures Run options: --seed 2821 # Running: Finished in 0.000584s, 0.0000 runs/s, 0.0000 assertions/s. 0 runs, 0 assertions, 0 failures, 0 errors, 0 skips <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moderate risk because the generator now conditionally deletes `config/webpack/` based on file/content matching, which could remove expected files if detection is wrong, though it attempts to preserve custom/unknown files and warns instead. > > **Overview** > Fixes `--rspack` installs leaving behind stale `config/webpack/` by adding a cleanup step that **removes the directory only when it contains known stock/generated webpack configs**; otherwise it preserves the directory and emits a warning listing custom/unknown files. > > The generator now tracks managed webpack template files and renders templates via `ERB` to verify file contents before deletion, and specs add coverage for both the removal path and the preservation path when custom webpack files are present. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 474a075. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Conservative, automatic cleanup of stale Webpack config when switching to Rspack, while preserving user-customized files and using template-aware comparisons to avoid removing modified configs. * Automatic generation or replacement of Rspack configuration where appropriate. * **Tests** * Added comprehensive tests validating cleanup, preservation, legacy-file removal, and Rspack generation across transition scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - **Add PR #2597** (clean stale webpack config on `--rspack` install) to changelog under `#### Fixed` - **Stamp `16.4.0.rc.10`** version header with today's date - **Improve `/update-changelog` skill**: auto-commit/push/PR after stamping, ask for confirmation on ambiguous version bumps ## Test plan - [ ] Verify CHANGELOG.md formatting and version links are correct - [ ] Verify `/update-changelog` skill reads correctly for future invocations 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only updates to the changelog and the Claude `/update-changelog` command instructions; no runtime code paths changed. > > **Overview** > Stamps a new `### [16.4.0.rc.10] - 2026-03-14` section in `CHANGELOG.md`, folding in the `--rspack` stale `config/webpack/` cleanup fix and updating the compare links. > > Updates the `/update-changelog` command docs to (1) require explicit user confirmation when the suggested bump type is ambiguous and (2) switch release/rc/beta mode guidance from manual “commit/push” steps to **automatically creating a branch, committing, pushing, and opening a PR** after stamping. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 596cc76. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ruby 3.4 heredoc compatibility, pnpm workspace setup guard, SSR error serialization, CSS module SSR behavior, stale webpack cleanup, and private path handling on fresh installs. * **New Features** * Explicit version stamping for changelogs (supports .rc/.beta), clearer prerelease/RC/Beta flows, automated changelog PR creation, and consolidation of duplicate changelog headings. * **Tests** * Added/updated tests to verify changelog consolidation and ordering. * **Chores** * Improved bump confirmation messaging and expanded workflow documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
## Summary - remove stale files when running - only remove when files are known stock/generated webpack config files - preserve directory and warn when custom files are present - add generator specs for both cleanup and preservation paths Fixes #2549 ## Testing - Inspecting 2 files .. 2 files inspected, no offenses detected - Run options: include {focus: true, locations: {"./react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb" => [559, 632]}} .. Finished in 44.88 seconds (files took 1.28 seconds to load) 2 examples, 0 failures Run options: --seed 2821 # Running: Finished in 0.000584s, 0.0000 runs/s, 0.0000 assertions/s. 0 runs, 0 assertions, 0 failures, 0 errors, 0 skips <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moderate risk because the generator now conditionally deletes `config/webpack/` based on file/content matching, which could remove expected files if detection is wrong, though it attempts to preserve custom/unknown files and warns instead. > > **Overview** > Fixes `--rspack` installs leaving behind stale `config/webpack/` by adding a cleanup step that **removes the directory only when it contains known stock/generated webpack configs**; otherwise it preserves the directory and emits a warning listing custom/unknown files. > > The generator now tracks managed webpack template files and renders templates via `ERB` to verify file contents before deletion, and specs add coverage for both the removal path and the preservation path when custom webpack files are present. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 474a075. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Conservative, automatic cleanup of stale Webpack config when switching to Rspack, while preserving user-customized files and using template-aware comparisons to avoid removing modified configs. * Automatic generation or replacement of Rspack configuration where appropriate. * **Tests** * Added comprehensive tests validating cleanup, preservation, legacy-file removal, and Rspack generation across transition scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - **Add PR #2597** (clean stale webpack config on `--rspack` install) to changelog under `#### Fixed` - **Stamp `16.4.0.rc.10`** version header with today's date - **Improve `/update-changelog` skill**: auto-commit/push/PR after stamping, ask for confirmation on ambiguous version bumps ## Test plan - [ ] Verify CHANGELOG.md formatting and version links are correct - [ ] Verify `/update-changelog` skill reads correctly for future invocations 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only updates to the changelog and the Claude `/update-changelog` command instructions; no runtime code paths changed. > > **Overview** > Stamps a new `### [16.4.0.rc.10] - 2026-03-14` section in `CHANGELOG.md`, folding in the `--rspack` stale `config/webpack/` cleanup fix and updating the compare links. > > Updates the `/update-changelog` command docs to (1) require explicit user confirmation when the suggested bump type is ambiguous and (2) switch release/rc/beta mode guidance from manual “commit/push” steps to **automatically creating a branch, committing, pushing, and opening a PR** after stamping. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 596cc76. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ruby 3.4 heredoc compatibility, pnpm workspace setup guard, SSR error serialization, CSS module SSR behavior, stale webpack cleanup, and private path handling on fresh installs. * **New Features** * Explicit version stamping for changelogs (supports .rc/.beta), clearer prerelease/RC/Beta flows, automated changelog PR creation, and consolidation of duplicate changelog headings. * **Tests** * Added/updated tests to verify changelog consolidation and ordering. * **Chores** * Improved bump confirmation messaging and expanded workflow documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
Summary
Fixes #2549
Testing
..
2 files inspected, no offenses detected
..
Finished in 44.88 seconds (files took 1.28 seconds to load)
2 examples, 0 failures
Run options: --seed 2821
Running:
Finished in 0.000584s, 0.0000 runs/s, 0.0000 assertions/s.
0 runs, 0 assertions, 0 failures, 0 errors, 0 skips
Note
Medium Risk
Moderate risk because the generator now conditionally deletes
config/webpack/based on file/content matching, which could remove expected files if detection is wrong, though it attempts to preserve custom/unknown files and warns instead.Overview
Fixes
--rspackinstalls leaving behind staleconfig/webpack/by adding a cleanup step that removes the directory only when it contains known stock/generated webpack configs; otherwise it preserves the directory and emits a warning listing custom/unknown files.The generator now tracks managed webpack template files and renders templates via
ERBto verify file contents before deletion, and specs add coverage for both the removal path and the preservation path when custom webpack files are present.Written by Cursor Bugbot for commit 474a075. Configure here.
Summary by CodeRabbit
New Features
Tests