Add automatic precompile hook coordination in bin/dev #2092
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis PR implements automatic precompile hook coordination in React on Rails' development server manager and adds idempotent locale generation. The changes enable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant bin_dev as bin/dev
participant ServerManager as ServerManager<br/>(precompile_hook)
participant Hook as Precompile Hook<br/>(rake locale)
participant Spawner as Process Spawner<br/>(Overmind/Foreman)
participant Webpack as Webpack<br/>(skips hook)
User->>bin_dev: Start development server
bin_dev->>ServerManager: run_from_command_line(ARGV)
alt Precompile hook configured
ServerManager->>Hook: Execute precompile_hook once
activate Hook
Hook->>Hook: Check if locales outdated
alt Locales up-to-date
Hook->>Hook: Skip (idempotent)
else Locales outdated or force=true
Hook->>Hook: Generate locale files
end
deactivate Hook
Hook-->>ServerManager: ✓ Complete
end
ServerManager->>ServerManager: Set SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true
ServerManager->>Spawner: Spawn processes with env var set
activate Spawner
Spawner->>Webpack: Start webpack --watch
activate Webpack
Webpack->>Webpack: Detect SHAKAPACKER_SKIP_PRECOMPILE_HOOK
Webpack->>Webpack: Skip running precompile_hook
deactivate Webpack
deactivate Spawner
Spawner-->>User: ✓ Development environment ready
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 (2 warnings)
✅ Passed checks (3 passed)
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 ReviewThis PR successfully implements idempotent locale generation with a force flag. Overall the implementation is solid and well-tested. Here's my detailed feedback: ✅ Strengths1. Clean Implementation
2. Excellent Test Coverage
3. Good Documentation
🔍 Potential Issues & Suggestions1. Missing RBS Type Signatures (Minor)Per CLAUDE.md, this project uses RBS for type checking. The new # sig/react_on_rails/locales.rbs
module ReactOnRails
module Locales
def self.compile: (?force: bool) -> (ToJs | ToJson)
class Base
def initialize: (?force: bool) -> void
end
end
endAction: Check if 2. Race Condition in File Timestamp Checking (Low Severity)The def files_are_outdated
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
endIssue: If two processes run simultaneously, both might see Suggested improvement: Consider using a lockfile pattern for production environments: def with_generation_lock
lockfile = "\#{i18n_dir}/.generation.lock"
File.open(lockfile, File::CREAT | File::WRONLY) do |f|
f.flock(File::LOCK_EX)
yield
end
ensure
File.delete(lockfile) if File.exist?(lockfile)
endNote: This might be overkill for typical usage. If Shakapacker's 3. Test Reliability: Time-Based Testing (Low Severity)The test at
Potential issue: On very slow/busy CI systems, Suggested improvement: # More robust approach
sleep 0.1 # Increase to 100ms for filesystem consistency
# Or use explicit mtime setting without relying on current time4. Return Value Consistency (Minor)The
Suggestion: This is fine for a constructor, but consider documenting the behavior or returning 5. ENV Variable Parsing (Very Minor)In locale.rake: force = ENV["force"] == "true"This only accepts force = %w[true 1 yes].include?(ENV["force"]&.downcase)🔒 Security Considerations✅ No security concerns identified. The changes:
⚡ Performance Considerations✅ Performance impact is positive:
📋 RecommendationsHigh Priority
Medium Priority
Low Priority
🎯 Final VerdictAPPROVED ✅ - This is a well-implemented feature that solves a real problem (duplicate work in precompile hooks). The code is clean, well-tested, and the documentation is excellent. The suggestions above are minor improvements, not blockers. The implementation is production-ready as-is. Great work! 🚀 |
Code Review - PR #2092OverviewThis PR successfully implements idempotent locale generation with a force flag, addressing issue #2090. The implementation is clean, well-tested, and follows the project's conventions. Overall, this is a high-quality contribution that improves developer experience. ✅ Strengths1. Clean Architecture
2. Excellent Test Coverage
3. User Experience
4. Documentation
🔍 Minor Observations & Suggestions1. Edge Case: Empty locale_files Array (Low Priority)In Current behavior: def files_are_outdated
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
endIf Likelihood: Very low - the Suggestion: Consider adding a guard: def files_are_outdated
return true if locale_files.empty? # Force regeneration if no source files found
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
endThis is optional and could be addressed in a follow-up if deemed necessary. 2. Test Improvement: sleep 0.01 (Nitpick)
sleep 0.01 # Ensure timestamp differenceThe comment suggests this ensures timestamp difference, but the next line immediately sets Impact: None - the test works correctly. This is purely for code clarity. 3. Console Output Uses
|
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ Triggered all workflows for full CI coverage Note: Added the To disable full CI mode, use the View progress in the Actions tab. |
Code ReviewThanks for this PR! This is a solid implementation that adds idempotent behavior to locale generation. Here's my detailed feedback: ✅ Strengths1. Clean Architecture
2. User Experience
3. Test Coverage
4. Documentation
5. Type Safety
🔍 Issues & Suggestions1. Critical: Empty Array Safety
|
Code Review - PR #2092✅ Overall AssessmentThis PR implements a well-designed solution for making the locale generation task idempotent. The implementation is clean, well-tested, and follows the project's conventions. Great work! 🎯 Strengths
🔍 Code Quality Observationslib/tasks/locale.rake
lib/react_on_rails/locales/base.rb
Test Coverage
💡 Minor Suggestions (Optional)
🔒 Security Considerations✅ No security concerns identified. The force flag is read from ENV and properly sanitized with a whitelist approach. ⚡ Performance Considerations✅ Excellent: The idempotent behavior prevents unnecessary file I/O and YAML parsing when files are up-to-date. This will significantly improve performance in development workflows and CI pipelines. Timestamp checking overhead: The
📝 CHANGELOG Entry✅ The CHANGELOG entry is well-written and follows the project conventions:
✨ RBS Type SignaturesThe RBS signatures in
🧪 Testing RecommendationsBefore merging, please verify:
📚 Documentation QualityThe documentation changes in
🎉 ConclusionThis is a solid, well-implemented PR that solves a real problem (issue #2090). The code is clean, well-tested, and properly documented. The idempotent behavior will make life easier for users who want to integrate locale generation into their build pipelines. Recommendation: ✅ Approve and merge (pending CI passing) Great work, @justin808! This is exactly the kind of improvement that makes React on Rails more pleasant to use. Review generated with Claude Code - focusing on code quality, testing, security, and performance considerations. |
Code Review - PR #2092SummaryThis PR successfully makes the ✅ Strengths
🔍 Minor Issues & Suggestions1. Potential nil error in
|
Code Review: PR #2092 - Make locale generation idempotent with force flag support👋 Great work on this PR! This is a well-thought-out solution that addresses the precompile hook coordination problem. Here's my detailed review: ✅ Strengths1. Excellent Design - Idempotent by DefaultThe idempotent behavior is properly implemented in
2. Comprehensive Test Coverage
3. Proper Error Handling
4. Type Safety with RBS
5. Documentation Excellence
🔍 Observations & Minor Suggestions1. Edge Case: Empty locale_files Array ✅ Already HandledIn def files_are_outdated
return true if locale_files.empty? # ✅ Good!
# ...
endThis is correct - if there are no source files, we should still generate the locale files. 2. Force Flag Parsing 💡 Consider EnhancementIn force = %w[true 1 yes].include?(ENV["force"]&.downcase)This is good, but consider if you want to handle the case where someone passes 3. Help Flag Detection ✅ Well DoneThe early help flag detection in command = args.find { |arg| !arg.start_with?("--") && !arg.start_with?("-") }
help_requested = args.any? { |arg| HELP_FLAGS.include?(arg) }This prevents running the precompile hook when the user just wants help. Smart optimization! 4. Version Warning Logic 💡 Potential IssueIn def warn_if_shakapacker_version_too_old
return unless PackerUtils.shakapacker_version_requirement_met?("9.0.0")
return if PackerUtils.shakapacker_version_requirement_met?("9.4.0")
# ... warning
endQuestion: What happens if Shakapacker version is < 9.0.0? The warning won't show. Is this intentional?
Recommendation: Consider adding a comment explaining why versions < 9.0.0 don't trigger a warning (perhaps they don't support precompile hooks at all?). 5. Environment Variable Always Set
|
Code Review - PR #2092Thank you for this comprehensive PR! This is a well-thought-out solution to the precompile hook coordination problem. Here's my detailed review: 🟢 Strengths1. Excellent Design - Idempotent BehaviorThe idempotent locale generation is exactly the right approach. By checking timestamps and skipping when files are up-to-date, this prevents wasteful regeneration while still allowing force override when needed. 2. Great Test Coverage
3. Clear DocumentationThe i18n.md and process-managers.md updates clearly explain the new pattern with before/after examples. 4. Good Error Handling
🟡 Potential Issues & Suggestions1. Critical: Missing Edge Case in
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/react_on_rails/dev/server_manager.rb (1)
154-189: Consider more robust command detection to avoid edge cases.The current command detection logic (line 156) finds the first non-flag argument before
OptionParser.parse!executes. This can incorrectly identify option values as commands.Example edge case:
bin/dev --route dashboard # args = ["--route", "dashboard"] # command = "dashboard" (incorrect - this is an option value, not a command) # Precompile hook runs, then exits with "Unknown argument: dashboard"While this doesn't cause incorrect behavior (the hook runs, then the command exits), it wastes time running the precompile hook before failing.
Suggested improvement:
- # Get the command early to check for help/kill before running hooks - # We need to do this before OptionParser processes flags like -h/--help - command = args.find { |arg| !arg.start_with?("--") && !arg.start_with?("-") } - - # Check if help flags are present in args (before OptionParser processes them) - help_requested = args.any? { |arg| HELP_FLAGS.include?(arg) } + # Check for help flags or help/kill commands before running hooks + help_requested = args.any? { |arg| HELP_FLAGS.include?(arg) } + skip_hook_commands = %w[help kill] + has_skip_command = args.any? { |arg| skip_hook_commands.include?(arg) }Then update line 186:
- unless %w[kill help].include?(command) || help_requested + unless has_skip_command || help_requestedAnd retrieve the command after parsing (line 182):
end.parse!(args) + command = args.firstThis avoids misidentifying option values as commands and is more explicit about intent.
🧹 Nitpick comments (4)
spec/react_on_rails/locales_to_js_spec.rb (1)
54-69: Spec: good coverage for forced regeneration; could also assert default.js mtimeThis example accurately simulates “up-to-date” JS files and verifies that
force: truetriggers regeneration by checkingtranslations.jsmtime. For extra confidence, you could also assert a changed mtime fordefault.js, but that’s optional since the underlying code writes both files together.spec/react_on_rails/dev/server_manager_spec.rb (1)
272-431: Specs: comprehensive coverage of precompile hook + skip env behaviorThe new examples thoroughly exercise
run_from_command_lineacross modes (dev/static/prod/kill/help/-h) with and without a precompile hook, and correctly validate SHAKAPACKER version gating andSHAKAPACKER_SKIP_PRECOMPILE_HOOKsemantics.One minor hygiene tweak: you already
ENV.delete("SHAKAPACKER_SKIP_PRECOMPILE_HOOK")in abefore; adding a matchingafterin this describe to restore or delete the var would further guard against leakage into any future examples that might run after this block.Based on learnings
lib/react_on_rails/locales/base.rb (1)
7-21: Locales: idempotent compile + force flag design looks correctThe
compile(force:)API cleanly forwards the flag toToJs/ToJson, andBase#initialize(force:)now implements the expected behavior: skip when not forced and files aren’t obsolete, regenerate otherwise, with helpful console messages. Thefiles_are_outdatedtweak to treat an emptylocale_filesset as “outdated” also makes sense so that missing YAMLs trigger regeneration to an empty payload rather than silently skipping.One small hardening you might consider:
files_are_outdatedcurrently callslocale_files.empty?. Iflocale_fileswere evernil(for example, from an unexpectedi18n.load_pathstate), this would raise; usinglocale_files.blank?(orreturn true if !locale_files || locale_files.empty?) would make this defensive without changing the intended behavior.Also applies to: 39-50, 67-68
lib/react_on_rails/dev/server_manager.rb (1)
214-238: Consider using safer command execution to prevent potential shell injection.The hook command is executed using
system(hook_value.to_s)(line 226), which passes the command as a string to the shell. Whileshakapacker.ymlis typically developer-controlled, using the array form ofsystem()is a safer practice that avoids shell interpretation of metacharacters.Apply this improvement:
+ require "shellwords" + def run_precompile_hook_if_present hook_value = PackerUtils.shakapacker_precompile_hook_value return unless hook_value # Warn if Shakapacker version doesn't support SHAKAPACKER_SKIP_PRECOMPILE_HOOK warn_if_shakapacker_version_too_old puts Rainbow("🔧 Running Shakapacker precompile hook...").cyan puts Rainbow(" Command: #{hook_value}").cyan puts "" - unless system(hook_value.to_s) + unless system(*Shellwords.split(hook_value.to_s)) puts "" puts Rainbow("❌ Precompile hook failed!").red.bold puts Rainbow(" Command: #{hook_value}").redThis ensures that the command is parsed into separate arguments, preventing shell metacharacter interpretation and making the execution more predictable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)docs/building-features/i18n.md(1 hunks)docs/building-features/process-managers.md(1 hunks)lib/react_on_rails/dev/server_manager.rb(5 hunks)lib/react_on_rails/locales/base.rb(4 hunks)lib/tasks/locale.rake(1 hunks)sig/react_on_rails/locales.rbs(1 hunks)spec/react_on_rails/dev/server_manager_spec.rb(1 hunks)spec/react_on_rails/locales_spec.rb(1 hunks)spec/react_on_rails/locales_to_js_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
📚 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:
spec/react_on_rails/locales_to_js_spec.rbsig/react_on_rails/locales.rbsdocs/building-features/i18n.mdlib/react_on_rails/locales/base.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
spec/react_on_rails/dev/server_manager_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:
spec/react_on_rails/dev/server_manager_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/dev/server_manager_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:
spec/react_on_rails/dev/server_manager_spec.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/react_on_rails/dev/server_manager_spec.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
spec/react_on_rails/locales_spec.rbsig/react_on_rails/locales.rbslib/react_on_rails/locales/base.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:
CHANGELOG.mdsig/react_on_rails/locales.rbsdocs/building-features/i18n.mdlib/react_on_rails/locales/base.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:
sig/react_on_rails/locales.rbslib/react_on_rails/locales/base.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:
sig/react_on_rails/locales.rbsdocs/building-features/i18n.mdlib/react_on_rails/locales/base.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
sig/react_on_rails/locales.rbslib/react_on_rails/locales/base.rb
🧬 Code graph analysis (4)
spec/react_on_rails/dev/server_manager_spec.rb (1)
lib/react_on_rails/dev/server_manager.rb (1)
run_from_command_line(151-209)
spec/react_on_rails/locales_spec.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(33-71)lib/react_on_rails/locales/base.rb (1)
compile(7-22)
lib/react_on_rails/dev/server_manager.rb (1)
lib/react_on_rails/packer_utils.rb (3)
shakapacker_precompile_hook_value(235-241)shakapacker_version_requirement_met?(30-33)shakapacker_version(15-19)
lib/react_on_rails/locales/base.rb (1)
lib/react_on_rails/configuration.rb (1)
initialize(134-190)
⏰ 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: examples (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (9)
docs/building-features/i18n.md (1)
24-46: Docs: precompile_hook + force flag guidance is clear and consistentThe new recommended Shakapacker
precompile_hooksection and theforce=trueexample line up with the rake task behavior and make the idempotent-flow story easy to follow. No changes needed here.docs/building-features/process-managers.md (1)
19-58: Docs: bin/dev + precompile_hook integration matches implementationThe updated bin/dev flow, precompile hook integration note, SHAKAPACKER_SKIP_PRECOMPILE_HOOK version caveat, and Procfile/shakapacker.yml examples are all consistent with the tested server_manager behavior. Looks good.
lib/tasks/locale.rake (1)
13-20: Rake task: force flag wiring and docs look correctThe Options block clearly documents
force=true, and the%w[true 1 yes].include?(ENV["force"]&.downcase)parsing plusLocales.compile(force: force)call cleanly hook into the new API. No changes needed.CHANGELOG.md (1)
26-31: Changelog: entries accurately describe new behaviorThe Unreleased “Improved” items succinctly capture the new bin/dev precompile_hook coordination and idempotent
react_on_rails:localebehavior (includingforce=true). They match the implementation and docs.spec/react_on_rails/locales_spec.rb (1)
40-54: Specs: correctly verify force propagation to writer classesThe added examples neatly confirm that
Locales.compile(force: true)forwardsforce: trueto the appropriate writer (ToJsonvsToJs) based oni18n_output_format, keeping configuration state restored via the existing before/after hooks. Looks solid.lib/react_on_rails/dev/server_manager.rb (3)
11-12: LGTM!The HELP_FLAGS constant is well-defined and appropriately frozen.
240-257: LGTM!The version checking logic correctly identifies Shakapacker versions in the 9.0.0–9.3.x range that lack
SHAKAPACKER_SKIP_PRECOMPILE_HOOKsupport, providing clear guidance to upgrade.
184-189: Effective coordination mechanism for preventing duplicate hook execution.Setting
SHAKAPACKER_SKIP_PRECOMPILE_HOOKafter running the hook ensures spawned processes (via Procfile) will skip their hooks, preventing race conditions and duplicate work.sig/react_on_rails/locales.rbs (1)
1-46: Well-structured type signatures for the locale compilation API.The RBS signatures accurately reflect the idempotent locale generation feature:
compile(?force: bool)correctly accepts an optional force parameterBase#initialize(?force: bool)properly propagates the force flag- Subclasses
ToJsandToJsonappropriately override specific methods- Type signatures for complex hash operations (lines 26-27) accurately represent the nested translation structures
The nullable return type for
Base#file_format(line 11) versus non-nullable in subclasses (lines 35, 41) is a valid type refinement pattern, indicating that Base may be abstract or have a default implementation.
Code Review: Idempotent Locale Generation with Precompile Hook IntegrationThis is an excellent enhancement that addresses a real pain point in development workflows. The implementation is well-structured with good test coverage. Here's my detailed feedback: ✅ Strengths1. Solid Core Implementation
2. Comprehensive Test Coverage
3. Excellent Documentation
4. Type Safety
5. Smart Integration with bin/dev
🔍 Issues & ConcernsCritical: Potential Nil Comparison Bug in
|
Code Review - PR #2092: Make locale generation idempotent with force flag supportI've reviewed this PR and it looks excellent overall! The implementation is clean, well-tested, and addresses a real pain point with development workflows. Here's my detailed feedback: ✅ Strengths1. Clean Architecture
2. Excellent Test Coverage
3. Documentation
4. Type Safety
🔍 Issues & RecommendationsCritical: Missing Feature in Base ImplementationIssue: The PR diff shows changes to def initialize(force: false)
return if i18n_dir.nil?
if !force && !obsolete?
puts "Locale files are up to date, skipping generation. Use force=true to regenerate."
return
end
@translations, @defaults = generate_translations
convert
puts "Generated locale files in #{i18n_dir}"
endHowever, I notice a potential issue: Problem: The logic
Actually, the logic looks correct! The condition properly handles all cases. Minor: User Message Could Be More InformativeCurrent: puts "Locale files are up to date, skipping generation. Use force=true to regenerate."Suggestion: puts "✓ Locale files are up to date (skipping generation). Use force=true to regenerate."Rationale: The checkmark emoji makes it clearer this is a success state, not an error. The parentheses clarify what's being skipped. Minor: Consider Adding File Path Context to MessagesCurrent: puts "Generated locale files in #{i18n_dir}"Suggestion: puts "✓ Generated locale files: #{files.join(', ')}"Rationale: Users might want to know which specific files were generated, especially when troubleshooting. Code Quality: Potential Edge Case with Empty Locale FilesIn def files_are_outdated
return true if locale_files.empty?
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
endObservation: The PR adds Question: What happens if However, looking at the caller in def obsolete?
return true if exist_files.empty?
files_are_outdated
endGood! The code already handles this by returning early if ServerManager Integration: Excellent Work!The integration with
One minor suggestion: The logic for detecting help flags is slightly complex: # Get the command early to check for help/kill before running hooks
command = args.find { |arg| !arg.start_with?("--") && !arg.start_with?("-") }
# Check if help flags are present in args
help_requested = args.any? { |arg| HELP_FLAGS.include?(arg) }
unless %w[kill help].include?(command) || help_requested
run_precompile_hook_if_present
ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] = "true"
endThis works, but there's a subtle issue: if someone runs
Actually, this is correct! The logic properly handles all edge cases. Great job! ✅ 🔒 Security ReviewRake Task Parameter Parsingforce = %w[true 1 yes].include?(ENV["force"]&.downcase)✅ Good: Safe parsing of the force parameter - no shell injection risk 🎯 Performance Considerations
No performance concerns. ✅ 📋 Test Coverage AssessmentWhat's Tested:
Potential Gaps:
Suggestion: Add a simple test in it "shows skip message when files are up-to-date" do
# First generation
described_class.new
# Second call should skip and show message
expect { described_class.new }.to output(/up to date.*skipping/i).to_stdout
end
it "shows generation message after generating" do
expect { described_class.new }.to output(/Generated locale files/i).to_stdout
end🎨 Style & ConventionsFollows Project Guidelines:
RBS Type Signatures:The new
📖 Documentation Reviewi18n.md Updates:
process-managers.md Updates:
🎯 Summary & RecommendationOverall Assessment: This is a high-quality PR that solves a real problem with a clean, well-tested solution. Required Changes:None - the code is production-ready as-is. Nice-to-Have Improvements:
Verdict: ✅ APPROVEThis PR is ready to merge. The implementation is solid, well-tested, and follows all project conventions. The nice-to-have improvements can be addressed in a follow-up PR if desired. 🙏 Great Work!Special kudos for:
This will make the development workflow much smoother for React on Rails users! 🚀 |
Code Review: Idempotent Locale Generation & Precompile Hook CoordinationGreat work on this PR! The implementation is solid and addresses a real pain point with duplicate task execution. Here's my detailed feedback: ✅ Strengths
🔍 Issues Found1. Critical Bug: Potential nil comparison error in
|
The bin/dev command now automatically runs Shakapacker's precompile_hook once before starting development processes and sets SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true to prevent duplicate execution in spawned webpack processes. Key improvements: - Executes precompile_hook once before launching Procfile processes - Sets SHAKAPACKER_SKIP_PRECOMPILE_HOOK environment variable - All spawned processes inherit the variable to prevent re-execution - Displays warning for Shakapacker versions below 9.4.0 - Skips hook execution for help and kill commands This eliminates the need for: - Manual coordination in Procfile.dev - Sleep hacks to sequence tasks - Duplicate task calls across processes - Race conditions when multiple processes generate files Users can now configure expensive build tasks (locale generation, ReScript compilation, etc.) once in config/shakapacker.yml and bin/dev handles the coordination automatically. Changes: - lib/react_on_rails/dev/server_manager.rb: Added hook execution logic with version warning for Shakapacker < 9.4.0 - spec/react_on_rails/dev/server_manager_spec.rb: Added comprehensive tests for hook execution, environment variable setting, and version warnings - docs/building-features/process-managers.md: Updated documentation with precompile hook integration details and version requirements - CHANGELOG.md: Added entry for automatic precompile hook coordination Addresses #2091 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
References the idempotent locale generation feature (PR #2093) and explains how bin/dev automatically coordinates the precompile hook.
justin808
force-pushed
the
jg-/coordinate-precompile-hooks
branch
from
b7ee06b to
02b6100
Compare
justin808
changed the title
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Comprehensive PR ReviewI've completed a detailed review of this PR. Here are my findings: ✅ Overall AssessmentThis is a well-implemented PR that successfully adds automatic precompile hook coordination to 🎯 Key Strengths
🔍 Implementation ReviewCommand Detection Logic (lines 154-159)command = args.find { |arg| !arg.start_with?("--") && !arg.start_with?("-") }
help_requested = args.any? { |arg| HELP_FLAGS.include?(arg) }Good: This early detection correctly identifies commands before OptionParser processes them. Observation: The logic assumes the first non-flag argument is the command. This works correctly for all current use cases:
Environment Variable Management (lines 184-189)unless %w[kill help].include?(command) || help_requested
run_precompile_hook_if_present
ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] = "true"
endGood:
Note: Tests verify this behavior in lines 417-423 (no hook configured case). System Command Execution (line 226)unless system(hook_value.to_s)Security: Using
Verdict: Acceptable for this context. The configuration file is under version control and reviewed. Version Warning Logic (lines 241-256)def warn_if_shakapacker_version_too_old
return unless PackerUtils.shakapacker_version_requirement_met?("9.0.0")
return if PackerUtils.shakapacker_version_requirement_met?("9.4.0")
# ... warning
endAnalysis:
Question: Why is there no warning for Shakapacker < 9.0.0? Likely reason: Versions below 9.0.0 don't support Suggestion: Add a comment explaining this logic for future maintainers: # Only warn if version is between 9.0.0 and 9.3.x
# Versions below 9.0.0 don't support precompile_hook at all
return unless PackerUtils.shakapacker_version_requirement_met?("9.0.0")
return if PackerUtils.shakapacker_version_requirement_met?("9.4.0")📝 Documentation Reviewi18n.md Updates (lines 24-46)Excellent:
process-managers.md Updates (lines 19-58)Outstanding:
CHANGELOG.md EntryGood:
Minor observation: The entry is quite long (4 lines). Per CLAUDE.md changelog guidelines, this is acceptable for a significant feature, but could be slightly condensed if needed. 🧪 Test QualityThe test suite is comprehensive and well-structured: Coverage:
Test Quality:
Edge Cases Covered:
🔒 Security ReviewVerdict: No security concerns for the intended use case. Considerations:
⚡ Performance ImpactPositive:
Neutral:
📋 Potential Improvements (Optional)These are not blocking issues, just suggestions for future consideration:
🎓 Lessons from Bot ReviewsI reviewed the extensive bot feedback (coderabbitai and previous Claude reviews). Key points that were already addressed or are non-issues:
✅ Compliance with Project GuidelinesPer CLAUDE.md requirements:
Note: Per CLAUDE.md, you should verify: bundle exec rubocopThis should pass with zero offenses. (I couldn't run it without approval, but the code looks RuboCop-compliant.) 🎯 Final RecommendationStatus: ✅ APPROVED This PR is ready to merge. The implementation is solid, well-tested, and solves a real problem that users face with duplicate precompile hook execution. The code quality is high, documentation is excellent, and there are no blocking issues. Summary:
Great work on this feature! This will significantly improve the developer experience for users with expensive precompile hooks. |
This commit enhances the bin/dev precompile hook feature with better error reporting and clearer code documentation based on code review feedback. Key improvements: - Enhanced error reporting: Hook failures now display stdout and stderr output to help users debug issues, using Open3.capture3 instead of system() - Improved documentation: Added comments explaining why SHAKAPACKER_SKIP_PRECOMPILE_HOOK is always set (provides consistent signal for custom scripts) - Better code clarity: Added descriptive variable names in version check (has_precompile_hook_support, has_skip_env_var_support) - Structured changelog: Broke long entry into scannable sub-bullets - Updated tests: All 7 test cases now use Open3.capture3 with proper status mocks - Reduced complexity: Extracted error handling into separate method to fix cyclomatic complexity All tests pass (35 examples, 0 failures) and RuboCop is clean (153 files, no offenses). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit implements additional safety and robustness improvements suggested during code review. Improvements: - Safer command execution: Use Shellwords.split to prevent shell metacharacter interpretation when executing precompile hooks - Better test isolation: Add after hook to clean up SHAKAPACKER_SKIP_PRECOMPILE_HOOK env var even if tests fail - Updated test mocks: All Open3.capture3 expectations now use split arguments matching Shellwords behavior These changes are non-blocking improvements that enhance code safety without changing user-facing behavior. All tests pass (35 examples, 0 failures) and RuboCop is clean (153 files, no offenses). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Adds automatic precompile hook coordination to
bin/dev, eliminating the need for manual coordination, sleep hacks, and duplicate task calls inProcfile.dev.This PR is focused solely on the bin/dev coordination feature. The complementary idempotent locale generation feature is in PR #2093.
How It Works
When you configure a
precompile_hookinconfig/shakapacker.yml:bin/devwill now:SHAKAPACKER_SKIP_PRECOMPILE_HOOK=trueenvironment variableBefore & After
Before (manual coordination with sleep hacks):
After (automatic coordination via bin/dev):
Clean, simple, reliable - no sleep hacks required!
Key Features
Shakapacker Version Detection
SHAKAPACKER_SKIP_PRECOMPILE_HOOKsupportError Handling
Smart Skipping
bin/dev killandbin/dev helpcommandsHelp Flag Handling
-h/--helpflags earlyUse Cases
Locale generation (with PR #2093):
ReScript compilation:
Multiple tasks:
Any expensive build task that needs to run before webpack starts!
Implementation Details
Modified Files:
lib/react_on_rails/dev/server_manager.rb- Core coordination logicspec/react_on_rails/dev/server_manager_spec.rb- Comprehensive test coverage (161 lines)docs/building-features/process-managers.md- Feature documentationdocs/building-features/i18n.md- Usage example with locale generationTest Coverage:
Shakapacker Version Requirements
precompile_hookconfiguration supportSHAKAPACKER_SKIP_PRECOMPILE_HOOKenv var support (prevents duplicate execution)If you're on Shakapacker < 9.4.0,
bin/devwill warn you that the hook may run multiple times.Related PRs
react_on_rails:localesafe to call multiple times)Breaking Changes
None - this is purely additive functionality.
Testing
Run the test suite:
bundle exec rspec spec/react_on_rails/dev/server_manager_spec.rbManual testing:
🤖 Generated with Claude Code