Walkthrough

The PR adds a new method to configure the precompile_hook in shakapacker.yml during generator execution and modifies the packer config logic to template the configuration and apply both rspack and precompile_hook settings when Shakapacker is freshly installed, addressing issue #2278.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #2278: Generator doesn't configure precompile_hook in shakapacker.yml when Shakapacker is pre-installed — this PR directly addresses the issue by adding the configure_precompile_hook_in_shakapacker method and ensuring it's called during config initialization.

Possibly related PRs

• Add Shakapacker precompile hook template for generator #1916: Both PRs modify the ReactOnRails generator's handling of Shakapacker precompile hooks—adding/configuring precompile_hook in shakapacker.yml during installation.
• Refactor: Extract JS dependency management into shared module #2051: Both PRs modify Shakapacker configuration and rspack handling in the generator's config setup logic.
• Upgrade Shakapacker from 8.2.0 to 9.0.0 #1904: Both PRs configure the Shakapacker precompile_hook in config/shakapacker.yml through generator and supporting script additions.

Suggested labels

bug, review-needed

Suggested reviewers

• justin808
• alexeyr-ci2

Poem

🐰 A hook now hops where configs brew,
Shakapacker's precompile too!
With comments safe and markers gone,
The generator marches on—
One less path for bundles true! ✨

Pre-merge checks and finishing touches

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 help to get the list of available commands and usage tips.}

PR Review - Code Quality Analysis

✅ Overall Assessment: EXCELLENT

This PR effectively fixes issue #2278 with a clean, well-tested solution. The code quality is high and follows best practices.

🎯 Strengths

1. Architecture & Design ⭐⭐⭐⭐⭐

• Excellent fix location: Moving configure_precompile_hook_in_shakapacker to run unconditionally at the end of copy_packer_config ensures it handles ALL scenarios (fresh install, pre-installed, user declined overwrite)
• Consistent pattern: The rspack configuration was also improved to use gsub_file instead of YAML.dump, maintaining consistency
• Smart guard clauses: The method properly checks Shakapacker version (9.0+), file existence, and doesn't overwrite existing configurations

2. Regex Pattern Quality ⭐⭐⭐⭐⭐

# Excellent - captures indentation and handles optional quotes
/^(\s*)assets_bundler:\s*["']?webpack["']?\s*$/

# Good - specific to Shakapacker's default format
/^(\s*)#\s*precompile_hook:\s*~\s*$/

# Smart check - avoids overwriting existing active config
/^\s+precompile_hook:\s*['"][^'"]+['"]/)

3. Test Coverage ⭐⭐⭐⭐⭐

• Comprehensive scenario testing: 3 new specs cover the pre-installed Shakapacker case
• Tests verify preservation: YAML anchors, comments, and structure are all validated
• Tests the actual bug: Ensures precompile_hook is configured when marker file exists

4. Comment Preservation ⭐⭐⭐⭐⭐

The shift from YAML.dump to gsub_file is brilliant:

• Preserves user documentation in comments
• Maintains YAML anchors (&default, <<: *default)
• Keeps file formatting intact

🔍 Minor Suggestions for Consideration

1. Regex Edge Case: Indentation Consistency

The precompile_hook regex expects the comment to be at root level:

/^(\s*)#\s*precompile_hook:\s*~\s*$/

Potential issue: In Shakapacker's default config, this line appears INDENTED under default::

default: &default
  source_path: app/javascript
  # precompile_hook: ~

Question: Does your regex handle this indented case? The ^ anchor requires the line to start at column 0 (after optional whitespace), but the comment is actually indented with 2 spaces in real Shakapacker configs.

Test to verify:

content = "  # precompile_hook: ~"
content.match?(/^(\s*)#\s*precompile_hook:\s*~\s*$/)  # Should this match?

Recommendation: The regex DOES capture leading whitespace with (\s*), so it should work correctly. However, consider adding a test comment in your spec to confirm this works with indented lines as found in real Shakapacker configs.

2. Safety Check Documentation

The safety check is excellent:

return if content.match?(/^\s+precompile_hook:\s*['"][^'"]+['"]/)

But there's a subtle difference between the two regexes:

• Replacement regex: ^(\s*)# - captures ANY whitespace (0 or more)
• Safety check regex: ^\s+ - requires AT LEAST 1 whitespace

Question: Is this intentional? Should the safety check also use \s* to catch root-level configs?

Edge case: If someone has this at root level (unusual but possible):

precompile_hook: 'some-custom-hook'

Your safety check would MISS this (because it requires \s+, not \s*), and you might try to replace a non-existent commented line.

Recommendation: Change to ^\s*precompile_hook: for consistency, or document why root-level configs are intentionally not checked.

3. Informative Logging Enhancement

Current logging is good but could be more informative:

puts Rainbow("✅ Configured precompile_hook in shakapacker.yml").green

Suggestion: Add context about WHICH scenario happened:

if content.match?(/^(\s*)#\s*precompile_hook:\s*~\s*$/)
  puts Rainbow("✅ Configured precompile_hook in shakapacker.yml").green
else
  puts Rainbow("ℹ️  precompile_hook already configured in shakapacker.yml").blue
end

This helps users understand whether their config was modified.

🔒 Security & Safety

✅ No security concerns

• File operations are local to generator context
• Regex patterns are specific and safe (no ReDoS risk)
• Proper guard clauses prevent unintended overwrites

📊 Performance

✅ Excellent performance characteristics

• Early returns prevent unnecessary file operations
• Single file read + gsub is optimal
• No performance concerns

🧪 Test Coverage Assessment

✅ Well Covered

• Pre-installed Shakapacker scenario
• Comment preservation
• YAML anchor preservation
• Marker file cleanup

💭 Consider Adding (Low Priority)

1. Test with indented # precompile_hook: ~ to confirm regex works
2. Test behavior when precompile_hook already exists (should skip)
3. Test Shakapacker < 9.0 (should early return)
4. Test when file doesn't exist (should early return)

Note: Some of these may be implicitly covered by existing integration tests.

📝 Code Style & Best Practices

✅ Follows all CLAUDE.md guidelines:

• Uses gsub_file for surgical updates (Rails best practice)
• Preserves file structure and comments
• Proper guard clauses
• Clear method naming
• Comprehensive test coverage

✅ Matches project conventions:

• Consistent with configure_rspack_in_shakapacker pattern
• Uses Rainbow for colored output
• Follows Thor generator patterns

🎯 Verdict

Status: ✅ APPROVE with minor suggestions

This PR is well-architected and thoroughly tested. The minor suggestions above are optional refinements, not blockers. The core fix is solid.

Recommendations before merge:

1. ✅ Verify the indentation regex works with real Shakapacker configs (likely already does)
2. ⚠️ Consider harmonizing the safety check regex (\s* vs \s+)
3. 💡 Optional: Add more informative logging

Great work on this fix! The move to gsub_file for both rspack AND precompile_hook is a significant improvement that will prevent future issues.

Review completed by: Claude Code Agent 🤖

Greptile Summary

Fixed critical deployment issue where precompile_hook was not configured in shakapacker.yml when Shakapacker was pre-installed before running the React on Rails generator.

Key Changes:

• Restructured copy_packer_config to ensure configure_precompile_hook_in_shakapacker is always called, regardless of whether Shakapacker was just installed or pre-existing
• Added new configure_precompile_hook_in_shakapacker method that uses gsub_file to surgically replace the commented placeholder # precompile_hook: ~ with the active configuration
• Improved configure_rspack_in_shakapacker to use gsub_file instead of YAML.dump, preserving comments and YAML anchors
• Added comprehensive test coverage for the "Shakapacker pre-installed" scenario

Impact:
This fix ensures the bin/shakapacker-precompile-hook script is properly configured in all installation scenarios, preventing production deployment failures where component bundles would be missing.

Confidence Score: 5/5

• This PR is safe to merge with high confidence
• The changes are well-structured with excellent test coverage. The fix addresses a real production issue (deployment failures) with a targeted solution. The use of gsub_file over YAML.dump is a significant improvement that preserves file structure. The implementation includes proper version checks, guards against re-configuration, and handles all edge cases (fresh install, pre-installed, user-declined overwrite).
• No files require special attention

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant User
    participant Generator as ReactOnRails Generator
    participant FileSystem as File System
    participant Shakapacker as Shakapacker Config

    User->>Generator: rails generate react_on_rails:install
    
    alt Shakapacker pre-installed
        Note over Generator: .shakapacker_just_installed exists
        Generator->>FileSystem: Check for marker file
        FileSystem-->>Generator: File exists
        Generator->>FileSystem: Delete marker file
        Note over Generator: Skip copying shakapacker.yml
    else Fresh install
        Generator->>FileSystem: Copy shakapacker.yml from template
        Note over Shakapacker: precompile_hook already configured in template
    end
    
    alt rspack option enabled
        Generator->>Shakapacker: configure_rspack_in_shakapacker()
        Shakapacker->>Shakapacker: gsub_file: assets_bundler → "rspack"
    end
    
    Generator->>Shakapacker: configure_precompile_hook_in_shakapacker()
    
    alt Shakapacker < 9.0.0
        Note over Generator: Skip (not supported)
    else Shakapacker >= 9.0.0
        Shakapacker->>Shakapacker: Check for existing active hook
        
        alt Hook already configured
            Note over Generator: Skip (already set)
        else Hook commented out
            Shakapacker->>Shakapacker: gsub_file: # precompile_hook: ~ → precompile_hook: 'bin/...'
            Note over Shakapacker: Preserves comments and YAML anchors
        end
    end
    
    Generator->>FileSystem: Copy bin/shakapacker-precompile-hook
    Note over FileSystem: Hook script ready to use