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.}

@claude review this PR

Claude finished @justin808's task —— View job

Code Review

Tasks

• Read changed files
• Review implementation in compiler.rb
• Review test coverage in compiler_spec.rb
• Review RBS type signatures
• Review documentation
• Provide comprehensive feedback

Summary

This PR successfully implements the SHAKAPACKER_SKIP_PRECOMPILE_HOOK environment variable to allow skipping the precompile hook when needed. The implementation is clean, well-tested, and properly documented.

Strengths

Excellent Implementation Quality:

• Clean, focused implementation in compiler.rb:83-88 with proper separation of concerns
• Robust test coverage with 3 comprehensive tests covering all edge cases
• Clear documentation with practical examples
• Proper RBS type signatures
• Maintains backward compatibility perfectly (default behavior unchanged)

Security Conscious:

• Uses exact string comparison (== "true") which prevents accidental activation
• No security concerns introduced

Well Documented:

• Added "Skipping the Hook" section in docs/precompile_hook.md:287-317
• Practical bin/dev example showing real-world usage
• Clear explanation of when and why to use the feature

Implementation Review

lib/shakapacker/compiler.rb (lines 83-88):

def should_run_precompile_hook?
  return false unless config.precompile_hook
  return false if ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] == "true"

  true
end

✅ Perfect implementation. The logic flow is clear and correct:

1. First checks if hook is configured
2. Then checks if skip flag is set
3. Returns true only if hook is configured and not skipped

The strict == "true" comparison is the right choice - it means only the exact string "true" will skip, and any other value (including "false", "1", empty string, or nil) will run the hook normally.

spec/shakapacker/compiler_spec.rb (lines 313-392):

✅ Excellent test coverage with 3 comprehensive tests:

1. Lines 313-334: Tests skip behavior when SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true - verifies only webpack is called (call_count == 1)
2. Lines 336-363: Tests normal behavior when env var is not set (nil) - verifies both hook and webpack are called (call_count == 2)
3. Lines 365-392: Tests normal behavior when SHAKAPACKER_SKIP_PRECOMPILE_HOOK=false - verifies both hook and webpack are called

These tests cover all the important cases and properly use RSpec mocking to verify the behavior without actually executing commands.

sig/shakapacker/compiler.rbs (line 55):

def should_run_precompile_hook?: () -> bool

✅ Type signature is correct. Takes no arguments and returns a boolean.

docs/precompile_hook.md (lines 287-317):

✅ Clear and helpful documentation including:

• When to use the skip flag
• Practical bin/dev example showing real-world pattern
• Explanation of why this is useful (preventing duplicate execution)

Minor Observations

Environment Variable Naming:
The name SHAKAPACKER_SKIP_PRECOMPILE_HOOK is clear and follows the project's naming convention. No issues here.

String Comparison:
The strict == "true" comparison is intentional and correct. This means:

• "true" → skip ✅
• "false", "1", "yes", etc. → run ✅
• unset/nil → run ✅

This is the safest approach and matches common Ruby/Rails patterns.

Test Quality:
The tests properly isolate the behavior using allow(ENV).to receive(:[]).with("SHAKAPACKER_SKIP_PRECOMPILE_HOOK").and_return(...). This is the correct way to test environment variable behavior without polluting the actual environment.

Potential Enhancements (Optional, Not Blocking)

These are completely optional suggestions for future consideration:

1. Environment variable documentation: Consider adding SHAKAPACKER_SKIP_PRECOMPILE_HOOK to any environment variables reference section if one exists in the docs.

2. Debug logging: Could optionally add a debug log message when hook is skipped:

   def should_run_precompile_hook?
     return false unless config.precompile_hook
     if ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] == "true"
       logger.debug "Skipping precompile hook (SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true)"
       return false
     end
     true
   end

   But this is purely optional - the current implementation is fine without it.

Verdict

✅ Excellent PR - Ready to Merge

This is a clean, well-implemented feature that:

• Solves the stated problem effectively
• Maintains backward compatibility
• Has comprehensive test coverage
• Is well documented with practical examples
• Introduces no security or performance concerns
• Follows the project's coding standards

The implementation is straightforward and does exactly what it needs to do without over-engineering. The test coverage is thorough, and the documentation clearly explains when and how to use the feature.

Great work! 🎉

Code Review for PR #850: Add SHAKAPACKER_SKIP_PRECOMPILE_HOOK environment variable support

I've thoroughly reviewed this PR. Overall, this is a well-implemented, focused change that cleanly solves the duplicate hook execution problem. Here's my detailed analysis:

✅ Strengths

1. Excellent Code Quality

• Clean implementation following Single Responsibility Principle
• The should_run_precompile_hook? method has clear, logical flow
• Minimal changes to existing code (only 1 line changed in compile method)
• Follows existing patterns in the codebase

2. Comprehensive Test Coverage

• Added 3 focused tests covering all scenarios:
  • Skip when SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true
  • Run when env var is not set
  • Run when SHAKAPACKER_SKIP_PRECOMPILE_HOOK=false
• Tests follow the existing test patterns perfectly (using spy, OpenStruct, etc.)
• Good use of mocking to verify call counts without actual execution

3. Proper Type Safety

• RBS signature file correctly updated with the new should_run_precompile_hook? method
• Method signature is accurate: () -> bool

4. Excellent Documentation

• The new "Skipping the Hook" section in docs/precompile_hook.md is thorough and practical
• Provides concrete examples with both Foreman and Overmind
• Explains why you'd want to skip the hook
• Clear bash script examples that developers can copy/paste

5. Security Conscious

• Reuses existing security validation from validate_precompile_hook
• No new security risks introduced
• Environment variable check is safe (only recognizes exact string "true")

🔍 Minor Observations (Not Issues)

1. Strict String Comparison

The implementation uses ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] == "true", which means:

• ✅ SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true → skips
• ❌ SHAKAPACKER_SKIP_PRECOMPILE_HOOK=1 → does NOT skip
• ❌ SHAKAPACKER_SKIP_PRECOMPILE_HOOK=yes → does NOT skip

This is actually good practice for explicit behavior, but worth documenting if users expect truthy value handling. The PR description and tests make this clear.

2. Method Placement

The should_run_precompile_hook? method is well-placed in the private section at line 83-88, right before run_precompile_hook. Good code organization.

3. Documentation Bash Examples

The bash examples in the docs extract the hook from YAML using Ruby one-liners. This is clever, but relies on the hook being defined in the development environment. Consider if a more generic approach would be helpful, or at least document that users may need to adjust for their environment.

🎯 Performance & Correctness

Performance

• ✅ No performance impact - just an early return if flag is set
• ✅ Environment variable check is O(1)
• ✅ No additional file I/O or system calls

Correctness

• ✅ Logic is correct: checks config first, then env flag
• ✅ Default behavior unchanged (backward compatible)
• ✅ Tests verify all edge cases
• ✅ Proper use of early returns for clarity

🔒 Security Review

• ✅ No command injection risks (reuses existing validate_precompile_hook)
• ✅ No path traversal issues
• ✅ Environment variable check is simple boolean comparison
• ✅ No shell execution in the new code
• ✅ No privilege escalation risks

📋 Test Coverage Analysis

The new tests at lines 313-392 cover:

1. ✅ Hook configured + skip flag = true → hook skipped (1 call)
2. ✅ Hook configured + skip flag = nil → hook runs (2 calls)
3. ✅ Hook configured + skip flag = "false" → hook runs (2 calls)

Missing test scenario (low priority):

• When config.precompile_hook is nil AND skip flag is true (should still return false)
  • However, this is already covered by the existing test at line 88-105

Test quality: Excellent use of call_count to verify behavior without relying on implementation details.

📝 Documentation Review

The documentation changes are comprehensive and helpful:

• Clear use cases listed
• Two complete bash script examples (Foreman and Overmind)
• Good explanation of the "why"
• Proper use of code blocks and formatting

Suggestion: Consider adding a note about the strict "true" check to avoid user confusion.

🎨 Code Style

Based on the project's CLAUDE.md guidelines:

• ✅ Files end with trailing newlines
• ✅ Follows existing code conventions
• ✅ No unnecessary comments
• ✅ Changes are focused and minimal
• ✅ RSpec test patterns match existing tests perfectly

✨ Best Practices Followed

1. ✅ Single Responsibility: New method does one thing well
2. ✅ Open/Closed Principle: Extended behavior without modifying existing logic
3. ✅ Fail-Safe Defaults: Hook runs by default; skipping requires explicit flag
4. ✅ Clear Naming: should_run_precompile_hook? is self-documenting
5. ✅ Test-Driven: Tests clearly verify the requirements from issue Add Precompile Hook Support with Environment-Based Skip Flag #849

📊 Overall Assessment

✅ Recommendation

APPROVE - This PR is ready to merge.

The implementation is clean, well-tested, properly documented, and solves the problem described in issue #849 without introducing any regressions or security concerns.

No changes required - This is production-ready code.

💡 Optional Future Enhancements (Not for this PR)

1. Consider supporting additional truthy values (1, yes, on) if user feedback indicates confusion
2. Add a warning log when hook is skipped to aid debugging
3. Consider adding metric/telemetry for hook execution (if that's tracked elsewhere)

These are minor enhancements that could be considered in future PRs if needed.

Great work! This PR demonstrates excellent engineering practices. 🚀

Code Review - PR #850

✅ Overall Assessment

This is a well-implemented feature that solves a real problem with duplicate precompile hook execution in multi-process setups. The code quality is high, tests are comprehensive, and documentation is thorough.

🎯 Code Quality

Strengths:

• Clean implementation: The should_run_precompile_hook? method follows the existing codebase patterns
• Simple logic: Clear boolean checks with early returns make the code easy to understand
• Minimal diff: Changes are focused and dont modify unrelated code
• Type safety: RBS signatures are properly updated

Code location reference:

• lib/shakapacker/compiler.rb:83-88 - Main implementation

🧪 Test Coverage

Excellent test coverage with 3 new tests covering all scenarios:

1. ✅ Hook skipped when SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true (lines 313-334)
2. ✅ Hook runs when env var not set (lines 336-363)
3. ✅ Hook runs when env var is "false" (lines 365-392)

Test quality:

• Properly mocked Open3 calls to avoid external dependencies
• Call counts verified to ensure correct behavior
• ENV stubbing is done correctly with and_call_original fallback

Minor observation: The tests properly verify that only the string "true" skips the hook, which is a good security practice (explicit opt-in).

🔒 Security Considerations

No security concerns identified. The implementation:

• ✅ Only checks an environment variable (no user input)
• ✅ Uses strict string comparison (== "true")
• ✅ Does not introduce any new command execution paths
• ✅ Existing validate_precompile_hook method already handles security (path traversal, shell injection protection)

⚡ Performance Considerations

No performance concerns:

• Simple environment variable check is O(1)
• Early return pattern avoids unnecessary work
• No additional system calls or I/O

📚 Documentation

Comprehensive documentation in docs/precompile_hook.md:

Strengths:

• ✅ Clear explanation of when to use the skip flag
• ✅ Multiple practical examples (Foreman, Overmind, Procfile patterns)
• ✅ Shows different integration approaches
• ✅ Includes React on Rails context note

Suggestions for improvement:

1. Bash script robustness: The documented bash examples extract YAML with eval, which works but could be documented with a safer Ruby approach as an alternative

2. Documentation location: Consider adding a brief mention in the main README or configuration guide pointing to this feature, as it is a significant capability

🐛 Potential Issues

None identified. The implementation is solid.

💡 Suggestions (Non-blocking)

1. Logging enhancement: Consider adding a debug log when the hook is skipped to help with debugging multi-process setups

2. Alternative ENV values: The current implementation only accepts "true". The explicit-only approach is good for clarity and is the right choice

✨ Best Practices Adherence

Checking against CLAUDE.md project guidelines:

• ✅ Files end with trailing newlines
• ✅ Code follows existing patterns
• ✅ Changes are focused and minimal
• ✅ No unnecessary comments
• ✅ Type signatures updated (RBS)
• ✅ Tests added for new functionality

📋 Checklist Before Merge

• Run bundle exec rspec spec/shakapacker/compiler_spec.rb to verify all tests pass
• Run bundle exec rubocop to ensure style compliance
• Consider adding debug logging when hook is skipped (optional)
• Update CHANGELOG.md per project guidelines

🎉 Conclusion

Recommendation: Approve with minor suggestions

This PR successfully implements the requested feature from issue #849. The code is clean, well-tested, and properly documented. The implementation elegantly solves the duplicate hook execution problem in multi-process development environments.

Great work! 👏

Code Review - PR #850

Summary

This PR adds support for the SHAKAPACKER_SKIP_PRECOMPILE_HOOK environment variable to allow skipping the precompile hook when needed. The implementation is clean, well-tested, and properly documented.

✅ Strengths

Code Quality

• Clean implementation: The should_run_precompile_hook? method follows a clear guard clause pattern that's easy to understand
• Proper separation of concerns: The logic is encapsulated in a private method rather than inline
• Type safety: RBS signatures are properly updated to reflect the new method

Test Coverage

• Comprehensive tests: Three new tests cover all edge cases:
  • Skipping when flag is "true"
  • Running when flag is unset (nil)
  • Running when flag is "false"
• Good test structure: Tests properly mock dependencies and verify call counts
• Consistent with existing tests: Follows the same patterns as other precompile hook tests

Documentation

• Excellent documentation: The new "Skipping the Hook" section in docs/precompile_hook.md is thorough and includes:
  • Clear use cases
  • Multiple practical examples (Foreman, Overmind, Procfile env prefix)
  • Real-world bin/dev integration patterns
  • Warning about React on Rails already implementing this pattern

🔍 Observations & Suggestions

1. Environment Variable Semantics ✅

The implementation only recognizes "true" as a skip signal. Any other value (including "false", "1", empty string) runs the hook. This is:

• Safe: Defaults to running the hook (fail-safe)
• Explicit: Users must explicitly set "true" to skip
• Well-tested: All three scenarios are covered

Consideration: Some gems use truthy checks like ["true", "1", "yes"].include?(ENV["VAR"]). The current strict approach is fine and arguably better, but worth documenting if it ever causes confusion.

2. Security - Shell Injection in Documentation ⚠️

The documentation examples use eval $HOOK which could be a security concern:

# docs/precompile_hook.md:317
eval $HOOK || exit 1

Issue: If someone configures a malicious hook in shakapacker.yml, eval will execute it with shell expansion.

Mitigation: The actual Ruby implementation (lib/shakapacker/compiler.rb) uses Open3.capture3 with proper argument parsing via Shellwords.shellsplit, which does prevent shell injection. The validation also ensures hooks are within the project root.

Recommendation: Consider updating the bash examples to avoid eval:

# Safer alternative
if [ -n "$HOOK" ]; then
  echo "Running precompile hook: $HOOK"
  # Don't use eval - run the hook via the Ruby implementation
  bundle exec ruby -r ./config/boot -e "system(Shakapacker.config.precompile_hook) or exit(1)"
fi

Or simply note that the examples are simplified and the actual hook execution happens safely via Ruby.

3. Documentation Examples Modify .env Files ⚠️

The Foreman example writes to .env:

echo "SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true" > .env.tmp
cat .env >> .env.tmp 2>/dev/null || true
mv .env.tmp .env

Concerns:

• Modifies a version-controlled file (potentially)
• Could cause git conflicts if .env is tracked
• Loses the original .env if the cat fails silently

Recommendation: Suggest using .env.local or environment variable export instead:

# Option 1: Use .env.local (not tracked)
echo "SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true" > .env.local

# Option 2: Export in the shell before foreman
export SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true
exec foreman start -f Procfile.dev

The Procfile env prefix alternative (shown later) is actually the cleanest approach.

4. Minor: RBS Signature Order

The RBS file has validate_precompile_hook listed after run_precompile_hook, but in the Ruby file it's defined after. Consider ordering them consistently for maintainability (very minor).

🐛 Potential Issues

None Found

No bugs identified. The implementation is solid.

🚀 Performance Considerations

No performance concerns. The environment variable check is O(1) and happens once per compilation.

🔒 Security Assessment

✅ Secure

The core implementation is secure:

• Uses Open3.capture3 with argument array (no shell injection)
• Validates hooks are within project root
• Prevents path traversal attacks
• Tests verify shell injection prevention

⚠️ Documentation Improvement

Only concern is the bash examples using eval could mislead users into thinking that's the recommended pattern. Consider clarifying.

📋 Changelog Compliance

Per CLAUDE.md, this is a user-visible feature and should be in the changelog. ✅

The PR description doesn't mention updating CHANGELOG.md, but according to project guidelines, you should:

• Add an entry describing this feature
• Use the format: [PR #850](https://github.com/shakacode/shakapacker/pull/850) by [justin808](https://github.com/justin808)
• Or use the /update-changelog command for guided updates

🎯 Final Verdict

Status: ✅ Approve with minor suggestions

This is a well-implemented feature that solves a real problem (issue #849). The code quality is excellent, tests are comprehensive, and documentation is thorough.

Recommended Actions Before Merge:

1. Update CHANGELOG.md - Add entry for this user-visible feature
2. Consider documentation improvements:
   • Clarify that bash examples are simplified and actual execution is secure
   • Suggest .env.local or export instead of modifying .env
   • Or just emphasize the Procfile env prefix as the recommended approach

Optional Future Enhancements:

• Could add SHAKAPACKER_SKIP_PRECOMPILE_HOOK=1 support for consistency with common environment variable patterns
• Consider a --skip-precompile-hook CLI flag as an alternative to the env var

Great work! This is a solid PR that will improve the developer experience for Shakapacker users.

Reviewed with Claude Code

Code Review - PR #850

I've reviewed this pull request thoroughly. Overall, this is a well-implemented feature with excellent documentation and comprehensive test coverage. Here's my detailed feedback:

✅ Strengths

1. Code Quality

• Clean, focused implementation with a single responsibility
• The should_run_precompile_hook? method follows Ruby conventions and is well-placed
• Proper early returns for clarity
• Consistent with existing codebase patterns

2. Test Coverage

• Excellent test coverage with 3 comprehensive scenarios
• Tests verify the exact string matching behavior ("true" vs "false" vs nil)
• Tests properly mock dependencies and verify call counts
• All edge cases are covered

3. Documentation

• Extensive documentation in docs/precompile_hook.md with multiple implementation patterns
• Clear explanation of when and why to use the feature
• Practical examples for Foreman, Overmind, and different approaches
• Helpful warnings about the exact "true" string requirement

4. Security

• No security concerns introduced
• Leverages existing security validation in validate_precompile_hook
• Environment variable check is simple string comparison (no eval or injection risks)

5. Backward Compatibility

• Perfect backward compatibility - default behavior unchanged
• Only activates when explicitly set to "true"
• Non-breaking change

📝 Minor Suggestions

1. CHANGELOG.md - Update PR number

The CHANGELOG currently has placeholder values:

- **Added `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` environment variable to skip precompile hook**. [PR #XXX](https://github.com/shakacode/shakapacker/pull/XXX) by [justin808](https://github.com/justin808).

Should be updated to:

- **Added `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` environment variable to skip precompile hook**. [PR #850](https://github.com/shakacode/shakapacker/pull/850) by [justin808](https://github.com/justin808).

2. Documentation - Minor clarification

In docs/precompile_hook.md, the note about React on Rails could be even clearer about whether users need to take action or not. Consider adding:

**Note:** The examples below show how to implement this in your custom `bin/dev` script. If you're using React on Rails v13.1.0+, the generated `bin/dev` script already implements this pattern automatically - **no action needed**.

This makes it crystal clear to React on Rails users that they don't need to do anything.

3. Type Signature - Consider adding comment

The RBS signature in sig/shakapacker/compiler.rbs could benefit from a brief comment:

# Returns true if precompile hook should run, false if disabled or skipped via ENV
def should_run_precompile_hook?: () -> bool

This is optional but helps developers understand the boolean logic at a glance.

🎯 Performance Considerations

• ✅ No performance concerns
• Environment variable lookup is O(1) and happens once per compile
• Early returns prevent unnecessary checks

🔍 Potential Issues

None found. The implementation is solid.

🧪 Testing Recommendations

The current tests are excellent. If you want to be extra thorough, you could add:

1. Edge case test - Verify behavior with edge case values like "TRUE", "1", or "yes" (should run hook since only "true" skips)
2. Integration test - Though this might be overkill, testing with actual Foreman/Overmind would be the ultimate verification

However, the current test coverage is already comprehensive and sufficient for merge.

✨ Summary

This is a high-quality pull request that solves a real problem (#849) with a simple, well-tested solution. The extensive documentation is particularly impressive and will help users adopt this feature correctly.

Recommendation: Approve with minor CHANGELOG fix

The only required change before merge is updating the placeholder PR numbers in CHANGELOG.md. Everything else is optional polish.

Great work! 🎉

Review generated with Claude Code