Simplify precompile hook and restore Pro dummy app to async loading #2053
Conversation
…ding strategy Apply the same defer loading strategy fix from commit d1a8a1a to the Pro dummy app to resolve race condition causing Playwright test timeouts. ## Problem Playwright E2E tests for streaming were timing out waiting for console message "ToggleContainer with title", indicating React components weren't hydrating. ## Root Cause The Pro dummy app was still using async: true for javascript_pack_tag while the open-source dummy app was updated to defer: true in commit d1a8a1a. This created a race condition where: - Generated component packs load asynchronously - Main client-bundle also loads asynchronously - If client-bundle executes before component registrations complete, React tries to hydrate unregistered components - ToggleContainer never hydrates, useEffect never runs, console.log never fires ## Solution 1. Changed javascript_pack_tag from async: true to defer: true in application.html.erb 2. Added precompile_hook to shakapacker.yml for pack generation 3. Added bin/shakapacker-precompile-hook script Using defer: true ensures script execution order - generated component packs load and register components before main bundle executes, preventing the race condition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove unnecessary rubocop disable/enable directives - Fix Style/SymbolArray violation in shakapacker-precompile-hook - Add explanatory comment about generated_component_packs_loading_strategy defaulting to :defer to match OSS dummy app configuration Note: The failing "React Router Sixth Page" RSpec test is a known flaky test that also fails intermittently on master. This is not a regression from the defer loading strategy changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Revert Pro dummy app back to async: true (Pro supports Selective Hydration) - Fix ReScript build to run from Rails root instead of current directory - Use File.join for proper path resolution of config files - Wrap build commands in Dir.chdir(rails_root) for correct execution - Add early Rails root resolution with proper error handling - Remove unnecessary defer strategy comment from initializer - Add blank line before main execution section for style 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Convert precompile hooks from Ruby to bash and DRY them up: **Why Bash Over Ruby:** - Simpler: No Ruby stdlib dependencies - Faster: No Ruby interpreter startup overhead - Standard: Most build hooks are shell scripts - Cross-platform: Works everywhere (sh/bash universal) - Fewer dependencies: Doesn't require Ruby loaded **Changes:** - Create shared bash script in generator templates - Fix ReScript build to run from Rails root (cd into rails_root) - Fix pack generation to run from Rails root - Use proper path resolution with Rails root for all file checks - Replace Ruby scripts in both dummy apps with bash version - All three copies identical (generator template + 2 dummy apps) **Script Features:** - Finds Rails root by walking up directory tree - Detects ReScript config (bsconfig.json or rescript.json) - Runs ReScript builds from correct directory - Detects auto_load_bundle/components_subdirectory config - Generates packs when configured - Cross-platform package manager detection (yarn/npm) - Proper error handling and exit codes - Sets REACT_ON_RAILS_SKIP_VALIDATION for build context **Testing:** - Shellcheck passes with no warnings - Script executes successfully in non-pro dummy app - ReScript builds complete successfully - Pack generation runs successfully - All files have trailing newlines 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Consolidate duplicate precompile hook logic across OSS dummy app, Pro dummy app, and generator template into a single shared Ruby implementation. Changes: - Create lib/tasks/precompile/shakapacker_precompile_hook_shared.rb with common logic - Update spec/dummy/bin/shakapacker-precompile-hook to load shared implementation - Update react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook to load shared implementation - Simplify generator template to use Ruby instead of shell script (removes ReScript build logic) Benefits: - Eliminates 342 lines of duplicate code across 3 files - Easier to maintain and update precompile hook logic in one place - Consistent behavior across all environments - Better error handling with Ruby exceptions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…n available Update the generator template to intelligently use the shared precompile hook implementation when running in the gem's development/test context, while maintaining a self-contained fallback for generated user applications. This provides the best of both worlds: - Test dummy apps and generator template all use the same shared code - Generated user apps have a standalone copy that works independently - Easier to maintain and update the precompile hook logic The hook now tries to load the shared implementation from the gem first, and falls back to its inline implementation if the gem isn't available or accessible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…mplate Address feedback to properly organize the precompile hook code: 1. Move shared implementation to spec/support/ (test-only location) - Renamed: lib/tasks/precompile/shakapacker_precompile_hook_shared.rb - To: spec/support/shakapacker_precompile_hook_shared.rb - This makes it clear the shared code is only for test dummy apps 2. Simplify generator template to be standalone - Remove logic to load shared implementation from gem - Generator template is for newly created production apps - Should be simple, self-contained, and production-ready - No references to development/testing concerns 3. Update test dummy apps to reference new location - spec/dummy/bin/shakapacker-precompile-hook - react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook - Both now load from spec/support/ The generator template is now clean and focused on production use cases, while test dummy apps share common implementation from the test support directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace overly complicated 58-line Ruby implementation with a simple 22-line bash script that just checks for auto_load_bundle configuration and runs the rake task if needed. The bash script is: - Easier to read and understand - Fewer dependencies (no Ruby parsing, no fileutils) - Standard shell script that works everywhere - Does exactly what's needed, nothing more Perfect for newly generated production apps. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Key improvements to shakapacker precompile hook: 1. **Better error messages for ReScript builds** - Fail fast when ReScript config exists but package.json is missing - Fail fast when build:rescript script is not defined - Provide actionable error messages with exact fixes needed - Add JSON parse error handling 2. **More robust config detection** - Improved regex to properly ignore commented configuration lines - Allow flexible spacing in config assignments - Prevent false positives from commented-out config 3. **Consistent error handling** - Changed warnings to errors when builds are required but missing - Exit with status 1 on all failure scenarios - Better distinction between optional vs required failures Addresses code review feedback on error handling and regex robustness. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…late The SKIP_VALIDATION environment variable is not needed in the generator template because: 1. By the time apps use this hook, packages are already installed 2. The engine's skip_version_validation? already handles edge cases: - package_json_missing? check - running_generator? check 3. This was cargo-culted from the Ruby version which needed it for different reasons (loading Rails environment directly) The shared implementation in spec/support still sets this variable because test apps have different requirements, but user-generated apps should rely on the engine's built-in validation logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Reverted from the hacky bash grep approach back to a clean Ruby script. The bash version had several problems: 1. Hacky grep parsing to detect config - fragile and error-prone 2. Duplicates logic that already exists in PacksGenerator 3. More complex (20+ lines vs 21 lines) 4. Harder to maintain The Ruby version is superior because: 1. Leverages PacksGenerator.generate_packs_if_stale which already knows when to generate vs skip 2. Simple and clean - just load Rails and call the method 3. Proper error handling with backtrace 4. Let the engine's built-in validation logic handle skipping The engine already skips validation appropriately via: - package_json_missing? check - running_generator? check So we don't need REACT_ON_RAILS_SKIP_VALIDATION here either. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Restored the original implementation which had: 1. Rainbow colored output for better UX during builds 2. ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true" because this hook loads Rails environment and validation would fail during builds The SKIP_VALIDATION is needed here because: - The hook loads Rails via require_relative "../config/environment" - This triggers the engine's validation initializer - During precompile, packages may not be fully available yet - Setting this env var prevents spurious validation failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis PR refactors the Shakapacker precompile hook mechanism by extracting complex build and pack-generation logic into a shared, reusable module. The main hook script is simplified to locate and load the shared hook, while configuration ties the hook into Shakapacker's precompile workflow. The hook automatically builds ReScript assets and generates React on Rails packs before webpack compilation. Changes
Sequence DiagramsequenceDiagram
participant Webpack as Webpack Build
participant Hook as Precompile Hook
participant SharedHook as Shared Hook Module
participant ReScript as ReScript Builder
participant Rails as Rails Generator
Webpack->>Hook: Invoke precompile hook
Hook->>Hook: Resolve gem root
Hook->>SharedHook: Load and execute
SharedHook->>SharedHook: Find Rails root
alt ReScript config exists
SharedHook->>ReScript: build_rescript_if_needed()
ReScript->>ReScript: Detect yarn/npm
ReScript->>ReScript: Run build:rescript
end
alt Auto-load or components configured
SharedHook->>Rails: generate_packs_if_needed()
Rails->>Rails: Set REACT_ON_RAILS_SKIP_VALIDATION=1
Rails->>Rails: Execute bundle exec rails react_on_rails:generate_packs
end
SharedHook-->>Webpack: Return (continue build)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
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 |
Code Review - PR #2053I've reviewed this PR and have the following feedback: ✅ Strengths
🔍 Code Quality Observations1. Path Traversal Logic (Minor) In both dummy app hooks, there's path traversal logic that is correct but fragile - if directory structure changes, it breaks. Consider adding a comment explaining why different traversal depths are needed. 2. Error Handling Inconsistency In
The 🐛 Potential Issues1. Cross-Platform Compatibility (line 62) if system("which yarn > /dev/null 2>&1")The if system("yarn", "--version", out: File::NULL, err: File::NULL)2. Missing Rainbow Dependency The generator template (line 24) uses: puts Rainbow("🔄 Running React on Rails precompile hook...").cyanBut there's no Recommendation: Either add ⚡ Performance ConsiderationsNo significant performance concerns. The refactoring actually improves performance by:
🔒 Security Review
The security comment added to 🧪 Test CoverageThe Missing: Tests specifically for the precompile hook scripts themselves Recommendation: Consider adding integration tests that execute the hook script directly and verify error handling. 🎯 RecommendationsHigh Priority:
Medium Priority: ✨ Overall AssessmentThis is a strong PR that significantly improves code quality and maintainability. The refactoring from a complex 100-line script to a clean 30-line implementation using the existing The issues identified are mostly minor and don't block the PR. The Rainbow dependency issue should be addressed before merge, but everything else is optional improvement. Recommendation: Approve with minor fixes requested (Rainbow dependency). Great work! 🚀 |
![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 (1)
spec/support/shakapacker_precompile_hook_shared.rb (1)
62-71: Misleading cross-platform claim.The comment claims "Cross-platform package manager detection" but uses the Unix-specific
whichcommand (lines 63, 65), which is not available on native Windows systems. While this works on Unix-like systems and Windows with Git Bash, it's not truly cross-platform.Consider either:
- Updating the comment to clarify "Unix-like systems" instead of "cross-platform"
- Using Ruby's
Gem.win_platform?and platform-specific detection- Using
system("yarn", "--version", out: File::NULL, err: File::NULL)which works cross-platformApply this diff for option 1 (simplest):
- # Cross-platform package manager detection + # Package manager detection (Unix-like systems)Or this diff for option 3 (more robust):
- # Cross-platform package manager detection - if system("which yarn > /dev/null 2>&1") + # Cross-platform package manager detection + if system("yarn", "--version", out: File::NULL, err: File::NULL) system("yarn", "build:rescript", exception: true) - elsif system("which npm > /dev/null 2>&1") + elsif system("npm", "--version", out: File::NULL, err: File::NULL) system("npm", "run", "build:rescript", exception: true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook(1 hunks)react_on_rails_pro/spec/dummy/config/shakapacker.yml(1 hunks)spec/dummy/bin/shakapacker-precompile-hook(1 hunks)spec/support/shakapacker_precompile_hook_shared.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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_pro/spec/dummy/config/shakapacker.ymlspec/dummy/bin/shakapacker-precompile-hookreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookspec/support/shakapacker_precompile_hook_shared.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_pro/spec/dummy/config/shakapacker.ymlspec/dummy/bin/shakapacker-precompile-hookreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookspec/support/shakapacker_precompile_hook_shared.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_pro/spec/dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookspec/support/shakapacker_precompile_hook_shared.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:
spec/dummy/bin/shakapacker-precompile-hookreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookspec/support/shakapacker_precompile_hook_shared.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_pro/spec/dummy/bin/shakapacker-precompile-hookspec/support/shakapacker_precompile_hook_shared.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
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
📚 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:
spec/support/shakapacker_precompile_hook_shared.rb
🧬 Code graph analysis (1)
spec/support/shakapacker_precompile_hook_shared.rb (2)
lib/react_on_rails/version_checker.rb (1)
package_json_path(223-225)lib/generators/react_on_rails/generator_helper.rb (1)
package_json(7-20)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (6)
react_on_rails_pro/spec/dummy/config/shakapacker.yml (1)
22-25: LGTM! Clear documentation with security guidance.The precompile hook configuration is well-documented with inline comments that appropriately emphasize security considerations for hook scripts.
spec/dummy/bin/shakapacker-precompile-hook (1)
4-19: LGTM! Clean delegation to shared implementation.The refactored hook properly locates the gem root, validates the shared hook exists, and delegates to the shared implementation. Error handling with emoji and clear messaging is a nice touch.
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook (1)
4-19: LGTM! Correct path calculation for Pro dummy app.The hook correctly calculates the gem root as four levels up from
react_on_rails_pro/spec/dummy/bin, properly accounting for the additional Pro directory level. The implementation mirrors the non-Pro version appropriately.spec/support/shakapacker_precompile_hook_shared.rb (3)
20-28: LGTM! Robust Rails root detection.The upward directory traversal approach is a standard and reliable method for locating the Rails root by finding
config/environment.rb.
85-113: LGTM! Proper pack generation with validation skip.The function correctly:
- Detects auto-pack configuration using regex that avoids commented lines
- Sets
REACT_ON_RAILS_SKIP_VALIDATION=trueas mentioned in the PR objectives- Provides appropriate error handling with informative messages
116-119: LGTM! Proper main guard pattern.The
__FILE__ == $PROGRAM_NAMEguard correctly allows the file to be loaded by other scripts (viaload) without executing the main logic, while also supporting direct execution. The ordering (ReScript build → pack generation) is logical.
Code Review - PR #2053I've reviewed the changes in this PR that improve the Shakapacker precompile hook implementation. Overall, this is a well-executed refactoring with several significant improvements. Here's my detailed feedback: ✅ Strengths1. Excellent Code SimplificationThe new generator template (
2. Smart Code Reuse with Shared ImplementationCreating
3. Improved Error HandlingThe shared implementation has robust error handling:
4. Better Documentation
🔍 Potential Issues & Recommendations1. Security: Path Traversal Risk (Medium)# spec/support/shakapacker_precompile_hook_shared.rb:20-27
def find_rails_root
dir = Dir.pwd
while dir != "/"
return dir if File.exist?(File.join(dir, "config/environment.rb"))
dir = File.dirname(dir)
end
nil
endIssue: The loop continues until filesystem root. On Windows, this could behave unexpectedly (no single root). Recommendation: Add a maximum depth limit: def find_rails_root
dir = Dir.pwd
max_depth = 10
depth = 0
while depth < max_depth
return dir if File.exist?(File.join(dir, "config/environment.rb"))
parent = File.dirname(dir)
break if parent == dir # Reached root
dir = parent
depth += 1
end
nil
end2. Cross-Platform Compatibility Issue (Low)# Line 63
if system("which yarn > /dev/null 2>&1")Issue: Recommendation: Use Ruby's built-in methods: if system("yarn", "--version", out: File::NULL, err: File::NULL)
system("yarn", "build:rescript", exception: true)
elsif system("npm", "--version", out: File::NULL, err: File::NULL)
system("npm", "run", "build:rescript", exception: true)
else
# error handling
end3. Inconsistent Error Handling for Errno::ENOENT (Low)# Lines 108-109
rescue Errno::ENOENT => e
warn "⚠️ Warning: #{e.message}"Issue: This catches ENOENT (file not found) but only warns instead of exiting. This could lead to silent failures if bundle executable is missing. Recommendation: Either exit with error code or document why this is a warning: rescue Errno::ENOENT => e
warn "⚠️ Warning: bundle command not found - #{e.message}"
warn " Skipping pack generation (bundle not available)"
# Or: exit 1 if this should be an error4. Missing Test Coverage (Medium)The shared implementation file doesn't have dedicated tests. While the integration tests cover this indirectly, unit tests would help catch edge cases. Recommendation: Add specs for:
5. Regex Pattern Complexity (Low)# Lines 95-96
return unless initializer_content.match?(/^\s*(?!#).*config\.auto_load_bundle\s*=/) ||
initializer_content.match?(/^\s*(?!#).*config\.components_subdirectory\s*=/)Issue: The negative lookahead Recommendation: Improve the regex: return unless initializer_content.match?(/^\s*[^#]*config\.auto_load_bundle\s*=/) ||
initializer_content.match?(/^\s*[^#]*config\.components_subdirectory\s*=/)🎯 Best Practices Adherence✅ RuboCop Compliance: Properly uses 🧪 Test Coverage ConsiderationsThe PR mentions a flaky test failure ("React Router Sixth Page"). Based on the code:
Recommendation: Consider adding a specific test in 📊 Performance Considerations✅ No performance concerns:
🔒 Security Assessment✅ Generally secure with minor notes:
🎯 Overall AssessmentLGTM with minor suggestions ✅ This is a high-quality refactoring that:
The issues I've raised are relatively minor and shouldn't block the PR. They can be addressed in follow-up improvements if desired. 📝 Suggested Follow-ups (Optional)
Great work on this refactoring! 🎉 |
justin808
changed the title
Code Review - PR #2053I've reviewed this PR and it's a well-structured refactoring that successfully eliminates code duplication. Here's my detailed feedback: ✅ Strengths
🔍 Critical Issue: Cross-Platform Bug in find_rails_rootLocation: spec/support/shakapacker_precompile_hook_shared.rb:20-28 Problem: The while loop uses 'while dir != "/"' which won't work on Windows (uses C:\ not /). Fix needed: Change to use 'loop do' with parent comparison (parent == dir) to detect filesystem root cross-platform. 🔍 Other Recommendations
🧪 Test CoverageMissing tests for:
Recommend adding unit tests for find_rails_root, build_rescript_if_needed, and generate_packs_if_needed. 🔒 Security Notes
🎯 SummaryVerdict: ✅ Approve with minor fixes Before merging:
Great work on this refactoring! The iterative evolution through commits shows thoughtful problem-solving. 🚀 |
Code Review for PR #2053Thank you for this refactoring! This is a well-structured PR that successfully simplifies the precompile hook implementation. Here's my detailed feedback: ✅ Strengths1. Excellent Code SimplificationThe new generator template (
2. Good DRY Principle ApplicationCreating 3. Clear Documentation ImprovementsThe Pro dummy app config ( 4. Correct Technical ChoiceRestoring 🔍 Issues FoundCRITICAL: Potential Security/Robustness Issue in Generator TemplateLocation: Issue: The template uses Problem: If this script fails to load the Rails environment (e.g., during asset precompilation in deployment where some gems might not be available in all groups), the script will crash with Recommendation: Add a fallback for Rainbow or make it conditional: begin
require 'rainbow'
USE_RAINBOW = true
rescue LoadError
USE_RAINBOW = false
end
# Then in the script:
def colored_output(text, color)
USE_RAINBOW ? Rainbow(text).send(color) : text
end
puts colored_output("🔄 Running React on Rails precompile hook...", :cyan)Alternative: Since the template already requires Rails environment, you could add a comment explaining this dependency: # Requires Rails environment which loads rainbow gem (react_on_rails dependency)
require_relative "../config/environment"MODERATE: Missing Error Context in Shared HookLocation: Issue: The error handling in rescue StandardError => e
warn "❌ Pack generation failed: #{e.message}"
exit 1
endRecommendation: Add more context to help developers debug failures: rescue StandardError => e
warn "❌ Pack generation failed: #{e.message}"
warn " Rails root: #{rails_root}"
warn " Initializer: #{initializer_path}"
warn e.backtrace.first(5).join("\n ") if ENV['VERBOSE']
exit 1
endMINOR: Inconsistent Main Execution GuardLocation: Issue: The guard Impact: This guard is ineffective - the code will always execute when loaded. Recommendation: Either:
Preferred approach: # Main execution (executed when loaded via 'load' from dummy app hooks)
build_rescript_if_needed
generate_packs_if_neededMINOR: RuboCop Disable Comments Could Be More SpecificLocation: Issue: Disabling multiple cops on one line: # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexityRecommendation: Consider refactoring def validate_rescript_package_json(rails_root)
package_json_path = File.join(rails_root, "package.json")
# validation logic...
end
def build_rescript_if_needed
rails_root = find_rails_root
return unless rails_root && rescript_config_exists?(rails_root)
validate_rescript_package_json(rails_root)
run_rescript_build(rails_root)
end🧪 Testing ConcernsMissing Test Coverage for New Shared HookObservation: The PR creates a new shared implementation but doesn't add explicit tests for:
Recommendation: Add spec tests in
📝 Minor Suggestions1. REACT_ON_RAILS_SKIP_VALIDATION DocumentationLocation: Suggestion: The comment is good, but could be clearer: # Skip package version validation during precompile hooks
# Rationale: This hook runs during the build process before full Rails initialization.
# Package validation is unnecessary since we're building the assets themselves.
# This prevents potential race conditions and reduces build time.
ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"2. File Existence Check MessagingLocation: Both dummy app hooks ( Suggestion: The error message could be more helpful: unless File.exist?(shared_hook)
warn "❌ Error: Shared precompile hook not found at #{shared_hook}"
warn " This indicates a React on Rails gem installation issue."
warn " Expected location: #{shared_hook}"
exit 1
end🚀 Performance & SecuritySecurity: ✅ Good
Performance: ✅ Good
📋 SummaryVerdict: ✅ Approve with minor suggestions This PR achieves its goals well:
Priority Fixes:
Nice-to-have:
Great work on this refactoring! The code is much cleaner and more maintainable. 🎉 Tested: Code review based on static analysis and project conventions from cc: @justin808 |
Summary
This PR improves the organization and documentation of precompile hooks across the React on Rails codebase.
Changes
1. Shared Test Helper for Dummy Apps
spec/support/shakapacker_precompile_hook_shared.rb(112 lines)2. Test Dummy App Hooks (Simple Wrappers)
Both
spec/dummy/bin/shakapacker-precompile-hookandreact_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hookare now just ~19 lines that load the shared implementation.3. Generator Template (Standalone, Production-Ready)
The
lib/generators/react_on_rails/templates/base/base/bin/shakapacker-precompile-hookis a 30-line Ruby script that:ReactOnRails::PacksGenerator.instance.generate_packs_if_staledirectlyREACT_ON_RAILS_SKIP_VALIDATION=truewith explanation4. Restored Pro Dummy App to async: true
defer: trueback toasync: truein application.html.erbasync: trueis the recommended approach for Shakapacker >= 8.2.05. Better Pro Dummy App Configuration Documentation
Architecture
Known Issues
The "React Router Sixth Page" RSpec test failure is a pre-existing flaky test that also fails intermittently on master. This is not a regression from these changes.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com