fix: configure precompile_hook when Shakapacker is pre-installed #2280
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #2278 When Shakapacker was already installed before running the React on Rails generator, the precompile_hook setting in shakapacker.yml was not being configured, even though the bin/shakapacker-precompile-hook script was copied correctly. This caused deployment failures because the hook wouldn't run during `rails assets:precompile`. Changes: - Restructured copy_packer_config to always call configure_precompile_hook_in_shakapacker at the end - Added configure_precompile_hook_in_shakapacker method that uses gsub_file to surgically update the YAML without destroying comments - Fixed configure_rspack_in_shakapacker to use gsub_file instead of YAML.dump, which was destroying all comments and file structure - Added specs to verify precompile_hook configuration when Shakapacker is pre-installed The fix uses Thor's gsub_file method (following Rails/Shakapacker best practices) to preserve comments and YAML structure, unlike YAML.dump which strips all comments and flattens anchor/alias references. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
WalkthroughThe PR adds a new method to configure the precompile_hook in shakapacker.yml during generator execution and modifies the packer config logic to template the configuration and apply both rspack and precompile_hook settings when Shakapacker is freshly installed, addressing issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review - Code Quality Analysis✅ Overall Assessment: EXCELLENTThis PR effectively fixes issue #2278 with a clean, well-tested solution. The code quality is high and follows best practices. 🎯 Strengths1. Architecture & Design ⭐⭐⭐⭐⭐
2. Regex Pattern Quality ⭐⭐⭐⭐⭐# Excellent - captures indentation and handles optional quotes
/^(\s*)assets_bundler:\s*["']?webpack["']?\s*$/
# Good - specific to Shakapacker's default format
/^(\s*)#\s*precompile_hook:\s*~\s*$/
# Smart check - avoids overwriting existing active config
/^\s+precompile_hook:\s*['"][^'"]+['"]/)3. Test Coverage ⭐⭐⭐⭐⭐
4. Comment Preservation ⭐⭐⭐⭐⭐The shift from
🔍 Minor Suggestions for Consideration1. Regex Edge Case: Indentation ConsistencyThe precompile_hook regex expects the comment to be at root level: /^(\s*)#\s*precompile_hook:\s*~\s*$/Potential issue: In Shakapacker's default config, this line appears INDENTED under default: &default
source_path: app/javascript
# precompile_hook: ~Question: Does your regex handle this indented case? The Test to verify: content = " # precompile_hook: ~"
content.match?(/^(\s*)#\s*precompile_hook:\s*~\s*$/) # Should this match?Recommendation: The regex DOES capture leading whitespace with 2. Safety Check DocumentationThe safety check is excellent: return if content.match?(/^\s+precompile_hook:\s*['"][^'"]+['"]/)But there's a subtle difference between the two regexes:
Question: Is this intentional? Should the safety check also use Edge case: If someone has this at root level (unusual but possible): precompile_hook: 'some-custom-hook'Your safety check would MISS this (because it requires Recommendation: Change to 3. Informative Logging EnhancementCurrent logging is good but could be more informative: puts Rainbow("✅ Configured precompile_hook in shakapacker.yml").greenSuggestion: Add context about WHICH scenario happened: if content.match?(/^(\s*)#\s*precompile_hook:\s*~\s*$/)
puts Rainbow("✅ Configured precompile_hook in shakapacker.yml").green
else
puts Rainbow("ℹ️ precompile_hook already configured in shakapacker.yml").blue
endThis helps users understand whether their config was modified. 🔒 Security & Safety✅ No security concerns
📊 Performance✅ Excellent performance characteristics
🧪 Test Coverage Assessment✅ Well Covered
💭 Consider Adding (Low Priority)
Note: Some of these may be implicitly covered by existing integration tests. 📝 Code Style & Best Practices✅ Follows all CLAUDE.md guidelines:
✅ Matches project conventions:
🎯 VerdictStatus: ✅ APPROVE with minor suggestions This PR is well-architected and thoroughly tested. The minor suggestions above are optional refinements, not blockers. The core fix is solid. Recommendations before merge:
Great work on this fix! The move to Review completed by: Claude Code Agent 🤖 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails/lib/generators/react_on_rails/base_generator.rb (1)
314-333: Implementation is correct; consider edge case handling.The method correctly handles the standard Shakapacker 9.x config format. One minor consideration: if the
# precompile_hook: ~placeholder line doesn't exist in the file (custom configs or future Shakapacker versions), thegsub_filewill silently do nothing.This is likely acceptable since:
- The version guard limits this to Shakapacker 9.0+
- Custom configs may already have
precompile_hookconfigured differentlyIf explicit feedback is desired when no substitution occurs, you could check the return value or content before/after.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/config/webpack/**/*.js : Use `namedExport: false` and `exportLocalsConvention: 'camelCase'` for CSS Modules loader options in Shakapacker 9.0+ if existing code uses `import styles from './file.module.css'` pattern
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: In IDE configuration, exclude these directories to prevent slowdowns: /coverage, /tmp, /gen-examples, /packages/react-on-rails/lib, /node_modules, /react_on_rails/spec/dummy/node_modules, /react_on_rails/spec/dummy/tmp, /react_on_rails/spec/dummy/app/assets/webpack, /react_on_rails/spec/dummy/log, /react_on_rails/spec/dummy/e2e/playwright-report, /react_on_rails/spec/dummy/test-results
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: For infrastructure/config changes, perform comprehensive local testing including: finding all affected files with grep, testing build pipeline, running relevant specs, and linting everything before pushing
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/*.rb : Run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/lib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 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:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
⏰ 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). (4)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🔇 Additional comments (3)
react_on_rails/lib/generators/react_on_rails/base_generator.rb (2)
92-112: LGTM - Control flow correctly addresses the root cause of issue #2278.The restructured logic ensures
configure_precompile_hook_in_shakapackeris called unconditionally at the end, covering all scenarios: fresh Shakapacker install, pre-installed Shakapacker, or when user declines config overwrite.
299-312: Good refactor -gsub_filepreserves YAML comments and anchors.The regex correctly captures leading whitespace and handles both quoted (
"webpack") and unquoted values, ensuring indentation is preserved in the replacement.react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1)
162-234: Comprehensive test coverage for issue #2278.The test correctly simulates the scenario where Shakapacker was pre-installed by:
- Creating the marker file and realistic shakapacker.yml with commented placeholder
- Verifying the placeholder is replaced while preserving the example comment
- Confirming YAML anchors (
&default,<<: *default) and comments are preserved- Ensuring marker file cleanup
This provides good regression coverage for the fix.
Greptile SummaryFixed critical deployment issue where Key Changes:
Impact: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Generator as ReactOnRails Generator
participant FileSystem as File System
participant Shakapacker as Shakapacker Config
User->>Generator: rails generate react_on_rails:install
alt Shakapacker pre-installed
Note over Generator: .shakapacker_just_installed exists
Generator->>FileSystem: Check for marker file
FileSystem-->>Generator: File exists
Generator->>FileSystem: Delete marker file
Note over Generator: Skip copying shakapacker.yml
else Fresh install
Generator->>FileSystem: Copy shakapacker.yml from template
Note over Shakapacker: precompile_hook already configured in template
end
alt rspack option enabled
Generator->>Shakapacker: configure_rspack_in_shakapacker()
Shakapacker->>Shakapacker: gsub_file: assets_bundler → "rspack"
end
Generator->>Shakapacker: configure_precompile_hook_in_shakapacker()
alt Shakapacker < 9.0.0
Note over Generator: Skip (not supported)
else Shakapacker >= 9.0.0
Shakapacker->>Shakapacker: Check for existing active hook
alt Hook already configured
Note over Generator: Skip (already set)
else Hook commented out
Shakapacker->>Shakapacker: gsub_file: # precompile_hook: ~ → precompile_hook: 'bin/...'
Note over Shakapacker: Preserves comments and YAML anchors
end
end
Generator->>FileSystem: Copy bin/shakapacker-precompile-hook
Note over FileSystem: Hook script ready to use
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2278
When Shakapacker was already installed before running the React on Rails generator, the
precompile_hooksetting inshakapacker.ymlwas not being configured, even though thebin/shakapacker-precompile-hookscript was copied correctly.Impact: This caused deployment failures because the hook wouldn't run during
rails assets:precompile, resulting in missing component bundles in production.Changes
copy_packer_configto always callconfigure_precompile_hook_in_shakapackerat the end, regardless of which code path is takenconfigure_precompile_hook_in_shakapackermethod that usesgsub_fileto surgically update the YAML without destroying commentsconfigure_rspack_in_shakapackerto usegsub_fileinstead ofYAML.dump, which was destroying all comments and file structureprecompile_hookconfiguration when Shakapacker is pre-installedWhy
gsub_fileinstead ofYAML.dump?YAML.dumpdestroys the configuration file:&default,<<: *default) into duplicated contentUsing Thor's
gsub_file(following Rails/Shakapacker best practices) preserves comments and structure while making surgical updates.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.