Fix invalid precompile_hook in Pro dummy #2202
Conversation
WalkthroughUpdated the shakapacker precompile hook in the dummy application to load the shared hook from the repository root instead of the gem root, enforced strict error handling with exit status 1 for missing dependencies, and added explicit hook loading at script completion. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook (2)
9-16: Path resolution to shared hook looks correct and fail‑fast behavior is appropriateThe relative ascent from
__dir__torepo_rootand the constructedshared_hookpath match the described layout (dummy app underreact_on_rails_pro, shared hook underreact_on_rails/spec/support). Exiting with status1when the hook is missing is a good fail‑fast choice for this test dummy.Only nit: this is tightly coupled to the current directory structure. If the repo layout changes often, a future refactor could derive
repo_rootby walking up until a known file (e.g., a gemspec or.gitdirectory) is found, but that’s not necessary for this fix.
4-7: Remember Pro changelog entry for this bugfixSince this change fixes a Pro dummy configuration issue, consider adding an entry to
/CHANGELOG_PRO.mdfollowing the standard PR/author format once the PR is finalized. Based on learnings, Pro-only fixes are expected to be tracked there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)`
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-10T09:27:45.093Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for Pro-only features, bug fixes, and improvements using the same format: `[PR number](URL) by [username](URL)`
📚 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_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/generators/react_on_rails/**/*.rb : Generators run in host app context, not engine context; do not assume host app structure (e.g., `app/javascript/` may not exist in older apps)
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Create corresponding RBS signature files in `sig/react_on_rails/` for new Ruby files and add them to Steepfile for type checking
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)`
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Engine code has limited access to host application; use `Rails.root` for host app paths and `ReactOnRails.configure` for engine configuration
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 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_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rbs : Add RBS signature files for all new Ruby files in `lib/react_on_rails/` and register them in Steepfile; validate with `bundle exec rake rbs:validate`
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 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_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,jsx,tsx,json,css,scss} : The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration separate from the root; CI lints both directories separately
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to spec/dummy/**/*.{spec,test}.{js,ts} : Playwright E2E tests use `cypress-on-rails` gem for Rails integration; use helpers like `app('clean')`, `appEval()`, and `appFactories()` for test setup instead of UI automation
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom app commands for Playwright E2E tests in this directory using the `command` syntax provided by `cypress-on-rails` gem
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
⏰ 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). (2)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
Code Review✅ Overall AssessmentThis is a good, targeted fix that resolves a path resolution bug in the Pro dummy app's precompile hook. The change is minimal, correct, and follows the project's conventions. 🎯 What This FixesProblem: The Pro dummy app's
Root cause: The monorepo has two top-level directories ( ✅ What's Good
📋 VerificationI verified the fix is correct: # Starting from: react_on_rails_pro/spec/dummy/bin/
# Four levels up: ../../../.. → repo root
# Path: react_on_rails/spec/support/shakapacker_precompile_hook_shared.rb ✅ EXISTSThe shared hook file exists at the new path and the logic is sound. 💡 Minor Suggestions (Optional)While not blocking, consider these follow-ups:
🔍 Alignment with CLAUDE.md Guidelines✅ No changelog needed - Internal test infrastructure fix (not user-facing) ✅ RecommendationLGTM (Looks Good To Me) - Ready to merge. This is a straightforward bug fix with no risks. Note: According to CLAUDE.md guidelines for managing file paths, this type of bug (hardcoded paths after refactors) is a known issue. The fix correctly addresses it by updating the path to match the current directory structure. |
Code Review - PR #2202: Fix invalid precompile_hook in Pro dummy✅ SummaryThis PR fixes a path resolution bug in the Pro dummy app's precompile hook. The shared hook file is correctly located in 🔍 Code Quality AssessmentStrengths:
Path Verification:
📝 Minor Observations
🧪 Testing RecommendationsPer CLAUDE.md guidelines on file path changes, please verify: # 1. Verify the new path exists (already confirmed ✅)
ls -la react_on_rails/spec/support/shakapacker_precompile_hook_shared.rb
# 2. Test that the hook loads successfully
cd react_on_rails_pro/spec/dummy
ruby -c bin/shakapacker-precompile-hook
# 3. If possible, test the actual precompile process
# (May require full Rails app setup)🎯 VerdictLGTM ✅ - This is a straightforward bug fix with correct implementation. Estimated Impact: Low risk, high value
Pro tip: When working with file paths in this monorepo, the structure is:
|
Code Review✅ SummaryThis PR fixes a critical path resolution bug in the Pro dummy app's precompile hook. The fix is correct and necessary. 🎯 What Changed
✅ Code Quality AssessmentPath Resolution Logic - Excellent
Error Handling - Good
Comments - Minor Issue (see below)
🐛 Potential IssuesNone - This is a straightforward path fix with no bugs. 🚀 Performance ConsiderationsNo impact - This is a startup hook that runs once during asset precompilation. 🔒 Security ConcernsNone - The hook:
🧪 Test CoverageImplicitly tested - This hook is exercised whenever:
The fact that CI passed after this change confirms the path is now correct. 📝 Minor Suggestion - Documentation ClarityConsider updating the comment to be more explicit: # Find the repo root directory (four levels up from react_on_rails_pro/spec/dummy/bin)
# Then navigate to react_on_rails/spec/support for the shared hook
repo_root = File.expand_path("../../../..", __dir__)
shared_hook = File.join(repo_root, "react_on_rails", "spec", "support", "shakapacker_precompile_hook_shared.rb")This makes it clearer why we need the ✅ RecommendationApprove and merge. This is a necessary bug fix with no downsides. The CI has passed, confirming the path is correct. Nice catch! This would have caused precompile failures when the hook tried to build ReScript or generate packs in the Pro dummy app. |
Code Review - PR #2202: Fix invalid precompile_hook in Pro dummy✅ Overall AssessmentThis is a clean, necessary bugfix that correctly resolves a path resolution issue in the Pro dummy app's precompile hook. 🎯 What ChangedThe Pro dummy app's
✅ Correctness VerificationPath calculation is correct:
Consistency with open-source dummy:
📋 Code QualityStrengths:
No issues found:
🧪 Testing ConsiderationsCovered by existing mechanisms:
Optional improvement for future consideration: # spec/react_on_rails/precompile_hook_spec.rb
RSpec.describe 'Precompile hooks' do
it 'open-source dummy hook finds shared implementation' do
hook_path = Rails.root.join('spec/dummy/bin/shakapacker-precompile-hook')
expect(File.exist?(hook_path)).to be true
# Verify the hook doesn't exit with error
end
it 'Pro dummy hook finds shared implementation' do
hook_path = Rails.root.join('react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook')
expect(File.exist?(hook_path)).to be true
end
endHowever, this is not required for this PR - the existing integration tests are sufficient. 📝 Documentation & ChangelogPR correctly skips changelog update:
🚀 RecommendationLGTM - Ready to merge ✅ This is a straightforward, necessary fix with:
No changes requested. |
Code Review for PR #2202SummaryThis PR fixes a path resolution issue in the Pro dummy app's shakapacker precompile hook. The hook was attempting to load a shared implementation file using an incorrect path. ✅ What's Good
📝 Observations
|
Code Review: Fix invalid precompile_hook in Pro dummySummary✅ LGTM - This is a correct and necessary bug fix. AnalysisProblem Identified: Path Resolution:
Verification:
Code Quality: Excellent
Testing RecommendationsSince this affects the build precompile process, please verify: cd react_on_rails_pro/spec/dummy
bundle exec rails assets:precompileThis should now successfully find and execute the shared hook without errors. Security: No concerns
Performance: No impact
NotesThis is a good example of the monorepo structure requiring careful path resolution. The shared file at Related Files:
Recommendation: Approve and merge. This unblocks the Pro dummy app's asset precompilation. |
Summary
The Pro dummy app was looking for a non-existent file in its
precompile_hook.Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.