Fix precompile hook detection to match generator template #2282
Conversation
The detection logic was looking for `react_on_rails:generate_packs` rake task pattern, but the generator template uses `generate_packs_if_stale` Ruby method directly. This mismatch caused the detection to always return false for apps created with the generator. Updated the regex pattern to match both valid approaches: - The rake task: `react_on_rails:generate_packs` - The Ruby method: `generate_packs_if_stale` Fixes #2279 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdd a regex constant and helper methods to ReactOnRails::PackerUtils to detect Shakapacker precompile hooks that invoke pack generation (including Changes
Sequence Diagram(s)(omitted — changes are localized detection logic and tests, not multi-component control-flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Code Review - PR #2282SummaryThis PR fixes a critical bug where ✅ Positives1. Root Cause AnalysisExcellent investigation! The PR correctly identifies:
2. Minimal, Focused FixThe solution is elegantly simple:
3. Clear DocumentationThe inline comment clearly explains both patterns: # Matches both:
# - The rake task: react_on_rails:generate_packs
# - The Ruby method: generate_packs_if_stale (used by generator template)4. Real-World TestingThe PR includes manual testing with a fresh Rails 8.0.4 app, which is exactly what CLAUDE.md requires:
|
Greptile SummaryFixed
The mismatch was causing false negatives for apps created with the generator, as the detection only looked for the rake task pattern while the generator template uses the more efficient Ruby method approach (no shell overhead). Issue found: Regex lacks word boundaries, which could match unintended substrings. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Rails App
participant RoR as ReactOnRails
participant Utils as PackerUtils
participant Hook as precompile_hook script
participant Generator as PacksGenerator
Note over App,Generator: Detection Flow
App->>RoR: Check if hook configured
RoR->>Utils: shakapacker_precompile_hook_configured?()
Utils->>Utils: extract_precompile_hook()
Utils-->>Utils: Returns "bin/shakapacker-precompile-hook"
Utils->>Utils: hook_contains_generate_packs?(hook_value)
Utils->>Utils: Check direct command with GENERATE_PACKS_PATTERN
Note over Utils: Pattern: /(react_on_rails:generate_packs|generate_packs_if_stale)/
Utils-->>Utils: No match (it's a script path)
Utils->>Utils: resolve_hook_script_path(hook_value)
Utils-->>Utils: Returns Rails.root/bin/shakapacker-precompile-hook
Utils->>Hook: Read script contents
Hook-->>Utils: Returns script with "generate_packs_if_stale"
Utils->>Utils: Match against GENERATE_PACKS_PATTERN
Note over Utils: ✅ Matches "generate_packs_if_stale"
Utils-->>RoR: true (hook configured)
RoR-->>App: Skip redundant generation
Note over App,Generator: Hook Execution Flow
App->>Hook: Execute precompile_hook
Hook->>Generator: ReactOnRails::PacksGenerator.instance.generate_packs_if_stale
Generator-->>Generator: Check if packs are stale
Generator-->>Hook: Generate packs if needed
Hook-->>App: Complete
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
react_on_rails/lib/react_on_rails/packer_utils.rb (2)
231-237: Mark helper method as private.The
resolve_hook_script_pathmethod is an internal helper forhook_contains_generate_packs?and should not be part of the public API.🔎 Proposed fix to mark method as private
Add a
privatedeclaration before the helper methods:end + private + def self.resolve_hook_script_path(hook_value) # Hook value might be a script path relative to Rails root return nil unless defined?(Rails) && Rails.respond_to?(:root)Note: This requires also moving
shakapacker_precompile_hook_valueafter the private declaration if it should remain public, or before if you want it to be public.
239-247: Fix critical RBS type signature errors insig/react_on_rails/packer_utils.rbs.Two issues found in the RBS signatures:
shakapacker_precompile_hook_valuereturn type is incorrect: Currently declared as() -> Stringbut the implementation returnsnilwhen Shakapacker is not defined or on error (lines 242, 246). Should be() -> String?.
resolve_hook_script_pathis missing from RBS: This is a public method (lines 231-237) but has no type signature. Should add:def self.resolve_hook_script_path: (untyped hook_value) -> Pathname?Update
sig/react_on_rails/packer_utils.rbs:
- Change line 10 from
def self.shakapacker_precompile_hook_value: () -> Stringtodef self.shakapacker_precompile_hook_value: () -> String?- Add missing signature:
def self.resolve_hook_script_path: (untyped hook_value) -> Pathname?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/lib/react_on_rails/packer_utils.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/react_on_rails/packer_utils.rb
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Learnt from: 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
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: 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.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Git pre-commit hooks automatically run RuboCop (auto-fix), ESLint (auto-fix), Prettier (auto-format), and trailing newline checks on all changed files before each commit
📚 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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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/react_on_rails/packer_utils.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). (5)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails/lib/react_on_rails/packer_utils.rb (2)
217-217: LGTM!The use of
GENERATE_PACKS_PATTERNcorrectly detects both the rake task and Ruby method invocations in direct hook commands.
225-225: LGTM!The pattern matching in script contents correctly detects both pack generation patterns, with appropriate error handling that fails safe.
Address review feedback: - Add \b word boundaries to GENERATE_PACKS_PATTERN to prevent false positives on substrings like 'my_generate_packs_if_stale_helper' - Add test for direct command containing generate_packs_if_stale - Add tests for script file reading path (both patterns) - Add test for script file not existing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - Fix precompile hook detection to match generator template✅ Overall AssessmentExcellent fix! This PR correctly addresses the mismatch between the detection logic and the generator template. The implementation is clean, well-tested, and follows best practices. 🎯 Strengths
📋 Minor Suggestions (Optional)
🔍 Security & Performance
🧪 Test CoverageThe test coverage is excellent. The new tests cover:
The only thing missing is a test for the error rescue block (e.g., file read permission errors), but this is not critical since the error handling is defensive and returns 📝 DocumentationThe PR description is exemplary:
This follows all the best practices from CLAUDE.md. ✅ RecommendationAPPROVE - This PR is ready to merge once CI passes. The fix is:
Great work @ihabadham! This is a textbook example of how to fix a bug: identify the root cause, implement the minimal correct fix, test thoroughly, and document clearly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/packer_utils_spec.rb (1)
173-225: Excellent test coverage for script-file detection scenarios.The new context block thoroughly covers:
- Script containing
generate_packs_if_stalemethod- Script containing rake task
- Script without pack generation
- Non-existent script file
The mocking structure is clean and appropriate.
💡 Optional: Consider adding test for file read errors
The implementation includes error handling (rescue block), so testing that
File.readerrors are swallowed and return false would verify the fail-safe behavior:it "returns false when script file cannot be read" do allow(script_full_path).to receive(:file?).and_return(true) allow(File).to receive(:exist?).with(script_full_path).and_return(true) allow(File).to receive(:read).with(script_full_path).and_raise(Errno::EACCES, "Permission denied") expect(described_class.shakapacker_precompile_hook_configured?).to be false end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/lib/react_on_rails/packer_utils.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/spec/react_on_rails/packer_utils_spec.rb
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
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 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`
📚 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/spec/react_on_rails/packer_utils_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/packer_utils_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/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/packer_utils_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 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/spec/react_on_rails/packer_utils_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/spec/react_on_rails/packer_utils_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/spec/react_on_rails/packer_utils_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 /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/spec/react_on_rails/packer_utils_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/e2e/**/*.spec.js : Use `app('clean')`, `appEval(code)`, `appFactories(options)`, and `appScenario(name)` helpers from cypress-on-rails in Playwright tests for Rails integration
Applied to files:
react_on_rails/spec/react_on_rails/packer_utils_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/packer_utils_spec.rb
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
react_on_rails/spec/react_on_rails/packer_utils_spec.rb
📚 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:
react_on_rails/spec/react_on_rails/packer_utils_spec.rb
🧬 Code graph analysis (1)
react_on_rails/spec/react_on_rails/packer_utils_spec.rb (1)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
shakapacker_precompile_hook_configured?(178-192)
⏰ 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: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (3)
react_on_rails/spec/react_on_rails/packer_utils_spec.rb (3)
5-5: RuboCop pragmas are appropriate for comprehensive test coverage.The disable/enable pair for
Metrics/ModuleLengthis properly placed and reasonable for a test file with extensive coverage. Ensurebundle exec rubocopis run before committing to verify no other violations are introduced.Also applies to: 287-287
159-164: LGTM! Test correctly verifies detection ofgenerate_packs_if_stale.The test appropriately covers the new detection pattern for Ruby method invocation, complementing the existing rake task test.
144-253: Test suite successfully covers the enhanced precompile hook detection.The expanded test coverage correctly verifies:
- Detection of both
react_on_rails:generate_packsandgenerate_packs_if_stalepatterns- Direct hook command scenarios
- Script-file-based hook scenarios
- Error cases (missing Shakapacker, config errors, missing files)
The tests align well with the PR objectives to fix detection for generator-template-created hooks.
The method can return nil when Shakapacker is not defined or on error, but the RBS signature declared it as returning String. Updated to String? to match the actual implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code ReviewOverall Assessment✅ LGTM - This is a well-executed fix with excellent problem analysis, comprehensive testing, and clean implementation. Strengths1. Problem Analysis 🔍
2. Solution Design ✨
3. Test Coverage ✅Comprehensive test coverage covering all scenarios:
The tests use realistic code samples (heredoc with actual script content) rather than just mock strings, which increases confidence. 4. Type Safety 🔒
5. Manual Testing 🧪
Code QualityMaintainability
Error Handling
Performance
Potential Concerns1. Pattern Specificity
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/sig/react_on_rails/packer_utils.rbs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
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 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
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.
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Git pre-commit hooks automatically run RuboCop (auto-fix), ESLint (auto-fix), Prettier (auto-format), and trailing newline checks on all changed files before each commit
📚 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/sig/react_on_rails/packer_utils.rbs
📚 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/sig/react_on_rails/packer_utils.rbs
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (1)
react_on_rails/sig/react_on_rails/packer_utils.rbs (1)
10-10: LGTM: Nullable return type correctly reflects implementation behavior.The change from
StringtoString?appropriately handles cases where the precompile hook value may be absent (missing configuration, missing script files, or read errors).
| def self.precompile?: () -> bool | ||
| def self.shakapacker_precompile_hook_configured?: () -> bool | ||
| def self.shakapacker_precompile_hook_value: () -> String | ||
| def self.shakapacker_precompile_hook_value: () -> String? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Extract public class methods from the Ruby implementation
echo "=== Public class methods in packer_utils.rb ==="
rg -n "^\s*def self\." react_on_rails/lib/react_on_rails/packer_utils.rb
echo ""
echo "=== Current RBS signatures ==="
cat react_on_rails/sig/react_on_rails/packer_utils.rbs
echo ""
echo "=== Checking for resolve_hook_script_path method ==="
rg -n "resolve_hook_script_path" react_on_rails/lib/react_on_rails/packer_utils.rbRepository: shakacode/react_on_rails
Length of output: 2308
🏁 Script executed:
#!/bin/bash
# Validate RBS signatures
cd react_on_rails
bundle exec rake rbs:validate
# Run Steep type-checking
bundle exec rake rbs:steepRepository: shakacode/react_on_rails
Length of output: 154
Add RBS signature for the public resolve_hook_script_path method.
The resolve_hook_script_path(hook_value) method is defined as public at line 231 in lib/react_on_rails/packer_utils.rb but is missing from the RBS signature file. Add the corresponding method signature to sig/react_on_rails/packer_utils.rbs.
🤖 Prompt for AI Agents
In react_on_rails/sig/react_on_rails/packer_utils.rbs around line 10, add an RBS
signature for the public class method resolve_hook_script_path taking a
hook_value and returning an optional string; specifically, declare def
self.resolve_hook_script_path: (String?) -> String? so the public method defined
at lib/react_on_rails/packer_utils.rb line 231 is represented in the signature
file.
There was a problem hiding this comment.
resolve_hook_script_path is only called internally by hook_contains_generate_packs? (line 220). It's not part of the intended public API.
There was a problem hiding this comment.
@ihabadham, thank you for the clarification! You're absolutely right—if resolve_hook_script_path is internal and not part of the public API, it shouldn't be included in the RBS signature file.
To make the internal nature explicit in the Ruby code, consider marking it as private:
class << self
private
def resolve_hook_script_path(hook_value)
# implementation
end
endThis way, Ruby will enforce that it can only be called from within the module, making the intention clear to future maintainers.
🧠 Learnings used
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 sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Learnt from: 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: 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
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
There was a problem hiding this comment.
valid suggestion, but out of scope for this PR, no?
There was a problem hiding this comment.
@ihabadham, you're absolutely right—marking it as private would be out of scope for this PR, which is focused on fixing the hook detection logic. That's a good refactoring for a separate change if needed in the future, but not necessary here. 👍
🧠 Learnings used
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 sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: 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
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Code Review for PR #2282✅ SummaryThis PR successfully fixes the precompile hook detection issue by updating the regex pattern to match both the rake task approach and the Ruby method approach used by the generator template. The implementation is solid with excellent test coverage. 🎯 Strengths
💡 Suggestions1. Consider Adding Edge Case TestsWhile test coverage is good, consider these additional scenarios: # Test with whitespace variations
" generate_packs_if_stale " # Should match
"generate_packs_if_stale()" # Should match (method call with parens)
"#generate_packs_if_stale" # Should NOT match (commented out)The current regex ( 2. Documentation EnhancementConsider adding a code comment explaining why both patterns exist: # Regex pattern to detect pack generation in hook scripts
# Matches both:
# - The rake task: react_on_rails:generate_packs (used by some legacy setups)
# - The Ruby method: generate_packs_if_stale (used by generator template - more efficient, no shell overhead)
GENERATE_PACKS_PATTERN = /\b(react_on_rails:generate_packs|generate_packs_if_stale)\b/This helps future maintainers understand the rationale. 3. Minor: RuboCop Disable CommentThe test file adds 🔒 Security ReviewNo security concerns identified.
⚡ Performance ConsiderationsNo performance concerns.
📋 Testing ValidationExcellent real-world testing:
🎓 Best Practices AlignmentFollows project conventions from CLAUDE.md:
📝 Final RecommendationAPPROVE ✅ This is a well-implemented fix with excellent test coverage and clear documentation. The changes are minimal, focused, and backward-compatible. The PR successfully resolves issue #2279 without introducing any regressions. Minor suggestions above are optional enhancements - they don't block approval. The PR is ready to merge as-is. Great work on the thorough investigation and fix! 🎉 |
Summary
shakapacker_precompile_hook_configured?method to properly detect hooks created by the generator templateProblem
The detection logic was looking for the
react_on_rails:generate_packsrake task pattern, but the generator template (bin/shakapacker-precompile-hook) uses thegenerate_packs_if_staleRuby method directly. This mismatch caused the detection to always returnfalsefor apps created with the generator.Root Cause Analysis
Investigation revealed this was a historical divergence:
generate_packs_if_staleRuby methodreact_on_rails:generate_packsrake taskThe Ruby method approach is actually more efficient (no shell overhead), so the template is correct. The detection logic needed to be updated to recognize both valid patterns.
Solution
Added a
GENERATE_PACKS_PATTERNconstant that matches both valid approaches:react_on_rails:generate_packs(used by test/dummy implementation)generate_packs_if_stale(used by generator template)Test Plan
rails generate react_on_rails:installshakapacker_precompile_hook_configured?returnstrueBefore fix:
After fix:
Fixes #2279
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.