Walkthrough

Adds documentation and a commented scaffold supporting an extensible bin/dev precompile pattern (examples use ReactOnRails::Locales.compile), updates i18n and process-manager docs, updates the generator bin/dev template, and adds a changelog entry. No runtime behavior changes unless the scaffold is enabled.

Changes

Sequence Diagram(s)

(omitted — changes are documentation and commented scaffolding; no new runtime control flow to visualize)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• Enhancement: Extensible bin/dev precompile pattern as alternative to precompile_hook #2347 — Matches: introduces the commented run_precompile_tasks pattern and documents ReactOnRails::Locales.compile.
• Documentation: Comprehensive guide for precompile_hook configuration and alternatives shakapacker#881 — Matches: guidance about disabling precompile_hook in favor of a bin/dev-centered precompile flow.
• Coordinate assets:precompile with Shakapacker Precompile Hooks #2090 — Related: coordinates locale generation and precompile steps in bin/dev and docs.

Poem

🐰 In a burrow of scripts I softly tap,
bin/dev now holds the map and the app.
Locales sprout with a Ruby-knit cheer,
Hooks take a nap — the path is clear.
Hop on, devs — spring builds draw near!

No actionable comments were generated in the recent review. 🎉

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

Greptile Overview

Greptile Summary

Introduced extensible bin/dev precompile pattern as alternative to precompile_hook, providing better support for projects with custom build steps like ReScript and TypeScript compilation.

Key Changes

• New Documentation Guide: Created comprehensive extensible-precompile-pattern.md covering implementation, benefits, troubleshooting, and migration from precompile_hook
• Public API Documentation: Added YARD docs for ReactOnRails::Locales.compile method, documenting it as a public API with examples
• Template Enhancement: Updated bin/dev template with commented extensible pattern that users can uncomment and customize for their needs
• Cross-references: Updated i18n and process-manager docs to explain both approaches and link to new guide

Benefits of This Pattern

• Direct Ruby API access avoids shell spawning overhead
• Better version manager compatibility (mise, asdf, rbenv)
• Single location for managing multiple precompile tasks
• Cleaner Procfiles without embedded precompile logic

Documentation Quality

The documentation is thorough with clear examples, comparison tables, troubleshooting sections, and FAQ. One minor style improvement suggested for YAML formatting clarity.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk - purely additive documentation and opt-in functionality
• Score reflects that this is a documentation-focused PR introducing an optional pattern. Changes are: (1) comprehensive documentation with examples and troubleshooting, (2) YARD docs for existing public method, (3) commented-out template code users must explicitly enable, (4) updated CHANGELOG. No breaking changes, no modifications to existing runtime behavior. One minor style suggestion for YAML formatting clarity.
• No files require special attention - all changes are documentation and optional templates

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant User
    participant bindev as bin/dev
    participant Rails as Rails Environment
    participant Locales as ReactOnRails::Locales
    participant PM as Process Manager
    participant Webpack

    User->>bindev: ./bin/dev
    
    alt Extensible Pattern (Uncommented)
        bindev->>bindev: Check args (not kill/help)
        bindev->>bindev: run_precompile_tasks()
        bindev->>Rails: require "../config/environment"
        Rails-->>bindev: Environment loaded
        
        opt Custom Build Steps
            bindev->>bindev: system("yarn res:build")
        end
        
        opt Locale Generation
            bindev->>Locales: ReactOnRails::Locales.compile
            Locales->>Locales: Check if files up-to-date
            alt Files stale or missing
                Locales->>Locales: Generate translations.json/js
                Locales->>Locales: Generate default.json/js
                Locales-->>bindev: Success
            else Files up-to-date
                Locales-->>bindev: Skipped (up-to-date)
            end
        end
        
        bindev->>bindev: puts "✅"
    end
    
    bindev->>PM: ReactOnRails::Dev::ServerManager.run_from_command_line
    PM->>PM: Read Procfile.dev
    
    par Parallel Processes
        PM->>Rails: bundle exec rails s
        PM->>Webpack: bin/shakapacker-dev-server
        PM->>Webpack: bin/shakapacker --watch
    end
    
    Note over PM,Webpack: No precompile_hook<br/>called by webpack<br/>(already done)

Code Review: Extensible bin/dev Precompile Pattern

This PR introduces a well-documented alternative approach for handling precompile tasks. Overall the implementation is solid, but I have some suggestions for improvement.

✅ Strengths

1. Excellent Documentation: The new extensible-precompile-pattern.md guide is comprehensive and well-structured with clear use cases, comparison tables, and troubleshooting sections.

2. Good User Experience: The commented pattern in the bin/dev template makes it discoverable without being intrusive for users who don't need it.

3. API Documentation: Documenting ReactOnRails::Locales.compile as a public API in the i18n docs is helpful and necessary.

4. Cross-linking: Good use of references between related documentation pages.

🔍 Potential Issues & Suggestions

1. Error Handling in Example Code

In the bin/dev template (lines 52-54), there's an uncommented example that could fail silently:

# Example: Generate locales using direct Ruby API (faster than rake task)
# This avoids shell spawning and version manager PATH issues
print "   Locale generation... "
ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

Issue: If ReactOnRails::Locales.compile raises an exception, the user won't see it clearly because the output format suggests success.

Suggestion: Add error handling in examples:

print "   Locale generation... "
begin
  ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
  puts "✅"
rescue => e
  puts "❌"
  warn "Failed to compile locales: #{e.message}"
  exit(1)
end

Or at minimum, update the documentation to show best practices for error handling.

2. Inconsistency in shakapacker.yml Documentation

In extensible-precompile-pattern.md lines 75-89, the example shows:

# Note: precompile_hook is not used here because:
# - In development: bin/dev runs precompile tasks before starting processes
# - In production: build_production_command includes all build steps
# precompile_hook: (not configured)

# Optional: set to false to disable Shakapacker's own precompile behavior
# shakapacker_precompile: false

Confusion: The instructions say "Uncomment and customize the run_precompile_tasks method below" AND "Set shakapacker_precompile: false", but the YAML example shows this as optional.

Suggestion: Clarify whether shakapacker_precompile: false is required or truly optional. If users leave Shakapacker's precompile enabled while also using the bin/dev pattern, what happens? Document the interaction.

3. Missing Test Coverage

Observation: This PR adds documentation and template changes but no automated tests.

Questions:

• Should there be integration tests verifying the bin/dev pattern works as documented?
• Should there be tests ensuring ReactOnRails::Locales.compile can be called with force: true as documented?
• Are there existing tests that validate the public API surface of ReactOnRails::Locales.compile?

The PR description mentions:

## Test plan

- [ ] Documentation renders correctly on the docs site
- [ ] Code examples in documentation are syntactically correct
- [ ] Links between documentation pages work

These are all unchecked. While documentation-only PRs may not need extensive testing, the examples should at least be validated for syntax correctness.

4. Potential Race Condition in Production

In the extensible-precompile-pattern.md guide, line 66 shows:

unless ARGV.include?("kill") || ARGV.include?("-h") || ARGV.include?("--help") || ARGV.include?("help")
  run_precompile_tasks
end

Question: What happens if users run bin/dev in production? The pattern runs precompile tasks on every server start. In multi-instance deployments, this could cause:

• Concurrent writes to the same locale files
• Unnecessary duplicate work

Suggestion: Add a warning in the documentation about production usage. The pattern seems designed for development only, but this isn't explicitly stated.

5. Version Manager Edge Case

The documentation (lines 170-175) mentions version manager compatibility as a benefit:

If you experience issues where rake tasks use the wrong Ruby version (common with mise, asdf, or rbenv in non-interactive shells):

1. Use the direct Ruby API in bin/dev instead of rake tasks
2. The Rails environment loaded in bin/dev will use the correct Ruby version

Potential Issue: The bin/dev script itself uses #\!/usr/bin/env ruby. In non-interactive shells, env ruby might ALSO pick up the wrong Ruby version if the version manager hasn't shimmed the PATH yet.

Suggestion: Consider mentioning this edge case or providing a workaround (e.g., using absolute paths from version manager, or ensuring bin/dev is run from an interactive shell).

🎨 Style & Conventions

Per CLAUDE.md requirements:

✅ No trailing newline issues detected
✅ Documentation follows markdown best practices
✅ Code examples use consistent style

🔒 Security Concerns

No security issues identified. The system("yarn res:build") call in examples uses hardcoded commands (no user input interpolation).

⚡ Performance Considerations

Positive: The direct Ruby API approach (ReactOnRails::Locales.compile) avoids shell spawning overhead, which is good.

Consideration: The pattern runs precompile tasks synchronously on every bin/dev start. For projects with multiple slow precompile tasks, this could make server startup noticeably slower. Consider documenting:

• Expected startup time impact
• Alternative approaches for very slow tasks (background compilation, file watching)

📝 Documentation Rendering

Please verify the documentation renders correctly:

# Check for broken internal links
grep -o '](.*\.md)' docs/building-features/extensible-precompile-pattern.md | sed 's/](//' | sed 's/)//'

The links reference:

• ./process-managers.md ✅
• ./i18n.md ✅
• ../core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md ⚠️ (verify this exists)

🎯 Recommendations

Before Merge:

1. ✅ Check all checkboxes in the test plan
2. ⚠️ Add error handling examples or warnings about error handling
3. ⚠️ Clarify the shakapacker_precompile: false requirement
4. 📋 Add note about production usage (or explicitly mark as dev-only)
5. 🔍 Verify all documentation links are valid

Consider for Follow-up:

• Add integration test coverage for the extensible pattern
• Document interaction between shakapacker_precompile, precompile_hook, and the bin/dev pattern
• Add troubleshooting section for common errors (e.g., "LoadError: cannot load such file")

Summary

This is a valuable addition that solves real problems with version managers and complex build pipelines. The documentation quality is excellent. With minor clarifications around error handling and configuration interactions, this will be a solid feature.

Recommendation: Approve with minor changes requested.

Review performed by Claude Code following CLAUDE.md guidelines

Code Review - PR #2349: Add extensible bin/dev precompile pattern

✅ Overall Assessment

This is a well-crafted PR that adds valuable documentation and examples for handling complex precompile scenarios. The code quality is excellent, and the approach is sound. I have a few suggestions for minor improvements.

🎯 Strengths

1. Well-documented pattern: The new guide is comprehensive, clear, and provides excellent examples
2. Proper API exposure: Correctly documents ReactOnRails::Locales.compile as a public API (already implemented in lib/react_on_rails/locales/base.rb:7)
3. Good cross-referencing: Links between docs are appropriate and helpful
4. Non-breaking change: Uses commented code in template, allowing users to opt-in
5. Clear benefits comparison: Tables effectively communicate when to use each approach
6. Passes CI: All relevant checks are passing

📋 Minor Suggestions

1. Consistency in wording (Documentation)

In extensible-precompile-pattern.md:36, the comment says:

# Project-specific: Build ReScript files

But in bin/dev template at line 45, it says:

# Example: Build ReScript files

Suggestion: Use consistent language. I'd recommend "Example:" throughout since this is a template users will customize, not a project-specific implementation.

2. Error handling in bin/dev example

In extensible-precompile-pattern.md:40-42:

print "   ReScript build... "
system("yarn res:build") or exit(1)
puts "✅"

Concern: If system("yarn res:build") fails, the exit(1) will prevent the "✅" from being misleading, but there's no error message.

Suggestion: Add clearer error handling:

print "   ReScript build... "
unless system("yarn res:build")
  puts "❌"
  exit(1)
end
puts "✅"

Or at minimum, add a comment explaining the pattern:

# Uses 'or exit(1)' to halt immediately on failure
system("yarn res:build") or exit(1)

3. ReactOnRails::Locales.compile return value

Looking at lib/react_on_rails/locales/base.rb:7-22, the compile method returns either a ToJs or ToJson instance (or implicitly nil if directories don't exist).

In the documentation (extensible-precompile-pattern.md:135):

print "   Locale generation... "
ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

Question: Should users check if compile succeeded? The method doesn't raise exceptions on failure, but it may print warnings and return early.

Suggestion: Either:

• Document that compile is safe to call (prints its own messages) and always succeeds gracefully
• Or show error handling if needed

From reading base.rb:39-56, it appears the method handles all edge cases gracefully (prints warnings, returns early if files are up-to-date, etc.), so the current example is probably fine. But adding a note in the docs would help users understand the behavior:

# ReactOnRails::Locales.compile handles all edge cases gracefully:
# - Prints warnings if no locale files found
# - Skips if files are up-to-date (unless force: true)
# - Safe to call even if i18n is not configured
ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?

4. Link to actual documentation URL

In bin/dev template (line 38):

# See documentation: https://www.shakacode.com/react-on-rails/docs/building-features/extensible-precompile-pattern

Verification needed: Does this URL exist/will it exist when docs are deployed?

Suggestion: Verify the URL path matches your docs deployment structure. Based on the file location (docs/building-features/extensible-precompile-pattern.md), this looks correct, but worth confirming.

5. Test coverage consideration

The PR adds documentation for using ReactOnRails::Locales.compile as a public API, but from CLAUDE.md, this pattern should be tested.

Question: Are there existing tests that verify:

1. ReactOnRails::Locales.compile works when called directly (without rake)?
2. The force parameter works correctly?

Looking at react_on_rails/spec/react_on_rails/locales_spec.rb (found via Grep), there appear to be tests, but verify they cover the public API usage shown in the documentation.

If not, consider adding a simple integration test that:

• Calls ReactOnRails::Locales.compile directly
• Verifies files are generated
• Tests the idempotent behavior (second call skips generation)

🔒 Security Considerations

✅ No security concerns. The changes are purely documentation and commented template code.

⚡ Performance Considerations

✅ Performance improvement: The documentation correctly notes that using ReactOnRails::Locales.compile directly avoids shell spawning overhead compared to the rake task approach.

From lib/tasks/locale.rake:17-20, the rake task simply calls the same method, so the direct API call saves the rake task initialization overhead.

🧪 Test Coverage

As noted in the PR description, the test plan focuses on documentation rendering and link validation:

• ✅ Documentation renders correctly
• ✅ Code examples are syntactically correct
• ✅ Links between docs work

Additional testing suggestion: Manually verify the bin/dev template by:

# Generate a new Rails app with React on Rails
rails new test_app
cd test_app
rails generate react_on_rails:install

# Verify bin/dev contains the commented pattern
grep -A 20 'EXTENSIBLE PRECOMPILE PATTERN' bin/dev

# Uncomment the pattern and verify it works
# (This would catch syntax errors or API issues)

📝 Suggestions Summary

✅ Recommendation

Approve with minor suggestions. The PR is ready to merge after addressing the documentation clarity points (none are blocking issues).

Great work providing this alternative pattern for complex build scenarios! This will be very helpful for projects with custom requirements.

🤖 Review conducted using repository's CLAUDE.md guidelines

Pull Request Review: Extensible bin/dev Precompile Pattern

I've reviewed PR #2349 and here's my comprehensive feedback:

✅ Code Quality & Best Practices

Strengths:

• Well-structured documentation: The new extensible-precompile-pattern.md guide is thorough, well-organized, and provides clear examples for different use cases
• Backward compatibility: The feature is opt-in via commented code, preserving existing behavior and not forcing users to adopt the new pattern
• Consistent with repo patterns: Follows Ruby style conventions (frozen_string_literal, proper indentation, line length under 120 chars)
• Clear separation of concerns: Distinguishes between development (bin/dev) and production (build_production_command) precompile handling
• Idempotent design: ReactOnRails::Locales.compile properly checks if files are up-to-date before regeneration

Documentation Quality:

• Comparison tables effectively communicate trade-offs between approaches
• Before/After examples make the benefits immediately clear
• Troubleshooting section addresses common pain points (version managers, environment loading)
• Proper cross-references to related documentation

🔍 Potential Issues & Concerns

1. API Documentation Clarity (Minor)
The ReactOnRails::Locales.compile method is documented in i18n.md as a "public API" but:

• No formal public API designation in the Ruby code itself (no explicit documentation comment)
• The method already existed and has test coverage, but this PR elevates its visibility
• Recommendation: Consider adding YARD-style documentation to the method in react_on_rails/lib/react_on_rails/locales/base.rb:7-22 to formalize its public API status

2. Configuration Consistency (Minor)
In extensible-precompile-pattern.md:79-94, the guide mentions:

# Optional: set to false to disable Shakapacker's own precompile behavior
# shakapacker_precompile: false

• This appears to be placeholder/example text but there's no clarification if this is a real shakapacker config option
• Recommendation: Either remove this if it's not applicable, or add explanation about when/why to use it

3. Error Handling in bin/dev Template (Minor)
In bin/dev:41-45, the ReScript build example checks for failure with unless system() and exits, but the locale generation at line:51 doesn't check ReactOnRails::Locales.compile's return value:

ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

• Looking at the implementation in locales/base.rb:39-56, the method returns early if there are no locale files or if they're up-to-date
• Recommendation: Consider whether error conditions in ReactOnRails::Locales.compile should be surfaced. Currently, if an exception occurs during compilation, it would bubble up and stop the dev server, which is probably desired behavior. This is fine as-is, but worth noting in documentation.

4. YAML Configuration Example Placement
The shakapacker.yml example in extensible-precompile-pattern.md:82-93 suggests disabling precompile_hook but doesn't show what a typical config looks like when using the default approach

• Recommendation: Could add a brief "Before" state showing the precompile_hook configuration for comparison

🔒 Security Concerns

No significant security issues identified. The changes are documentation-heavy and the code additions are:

• Template files that users customize
• Direct API usage patterns that are already tested and in production use

⚡ Performance Considerations

Positive impacts:

• Direct Ruby API calls (ReactOnRails::Locales.compile) avoid rake task overhead and shell spawning, improving startup time
• The idempotent file checking in locales/base.rb:62-77 ensures no unnecessary regeneration work
• Consolidating precompile tasks to run once at startup (vs. before each webpack compile with precompile_hook) reduces redundant work

Potential concern:

• If users uncomment the precompile tasks, bin/dev startup will be slower, but this is expected and documented
• The pattern encourages sequential execution of precompile tasks. For projects with many tasks, this could be slow, but parallelization is outside scope

🧪 Test Coverage

Current state:

• ReactOnRails::Locales.compile has comprehensive unit tests in react_on_rails/spec/react_on_rails/locales_spec.rb:7-55
• Tests cover force parameter, output format selection, and parameter passing
• The bin/dev template changes are in generator templates, which are harder to test directly

Observations:

• No integration test verifying the full run_precompile_tasks pattern works end-to-end
• The bin/dev changes are commented out by default, so there's minimal risk
• Recommendation: Consider adding an integration test that:
  1. Generates a project with the install generator
  2. Uncomments the precompile pattern
  3. Verifies bin/dev runs without errors

However, this may be overkill given the opt-in nature and documentation focus of this PR.

📋 Documentation Completeness

Excellent documentation coverage:

• ✅ New comprehensive guide (extensible-precompile-pattern.md)
• ✅ Updates to existing docs (i18n.md, process-managers.md)
• ✅ Inline comments in bin/dev template
• ✅ Cross-references between related docs
• ✅ Examples for both JavaScript/TypeScript and Ruby code

Test plan in PR description:

• The test plan focuses on documentation rendering and links, which is appropriate for this documentation-heavy change
• All three checklist items are reasonable

🎯 Additional Recommendations

1. CHANGELOG entry: Verify that this change should be in CHANGELOG.md. Per AGENTS.md:163-166, changelog entries are for "user-visible changes only". This is definitely user-visible (new feature/pattern).

2. Link validation: Run the link checker to ensure all cross-references work:

   # From AGENTS.md workflows
   # Check that markdown links are valid

3. Consider adding a "Why not just use precompile_hook" FAQ: The comparison table is good, but a brief FAQ entry addressing "When would I NOT want to use this pattern?" could help users make informed decisions.

4. Version compatibility note: Consider adding a note about which versions of React on Rails support this pattern (looks like it's for 16.2+ based on context).

📊 Overall Assessment

This is a high-quality PR that:

• Solves a real pain point (version manager issues, complex build requirements)
• Provides clear, actionable documentation
• Maintains backward compatibility
• Follows repository conventions
• Has minimal risk (opt-in, well-tested underlying APIs)

The issues identified are minor and mostly about documentation polish rather than code correctness or safety concerns.

Recommendation: Approve with minor suggestions ✅

The PR is ready to merge as-is, though the minor suggestions above would further improve it.

Review completed using repository guidelines from CLAUDE.md and AGENTS.md

Code Review - PR #2349: Extensible bin/dev Precompile Pattern

Summary

This PR introduces an alternative approach for handling precompile tasks directly in bin/dev instead of using the precompile_hook mechanism. Overall, this is a well-crafted documentation-focused PR that provides valuable flexibility for users with complex build requirements.

✅ Strengths

Documentation Quality

• Excellent structure: The new guide (extensible-precompile-pattern.md) is comprehensive, well-organized, and follows a logical progression from overview to implementation
• Clear comparisons: The benefits comparison table effectively helps users decide which approach to use
• Practical examples: Code samples are realistic and directly applicable
• Good cross-referencing: Proper links between related documentation pages

Code Quality

• Non-invasive: Changes are entirely additive - existing functionality remains unchanged
• Well-documented API: The RDoc comments added to ReactOnRails::Locales.compile follow best practices with proper @param, @return, @raise, and @example tags
• Safe defaults: The template code is commented out by default, preventing accidental activation

Design Decisions

• Clear separation of concerns: Distinguishes when to use each approach
• Backward compatible: Doesn't break existing workflows
• Addresses real pain points: Version manager issues and shell spawning overhead are legitimate problems

💡 Suggestions for Improvement

1. Error Handling in Documentation Example

Location: docs/building-features/extensible-precompile-pattern.md:40-44

The example shows error handling for the ReScript build but not for locale compilation. Consider adding a note about when ReactOnRails::Locales.compile might raise exceptions.

# Current example silently continues if locale compilation fails
print "   Locale generation... "
ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

Suggestion: Add a comment explaining that exceptions will bubble up (as mentioned later in the doc), or show a consistent error handling pattern:

# Locale generation via direct Ruby API (faster, no shell issues)
# Note: Exceptions (missing directories, etc.) will halt execution - this is intentional
# to surface configuration issues early during development startup
print "   Locale generation... "
ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

2. Clarity on When Precompile Tasks Run

Location: docs/building-features/extensible-precompile-pattern.md:179

The benefits table mentions "Once at dev server startup" but this could be clearer. Users might wonder: what happens on code changes? What about when webpack rebuilds?

Suggestion: Add a subsection explaining the execution timing:

## Execution Timing

With this pattern, precompile tasks run:
- ✅ Once when you start `bin/dev`
- ✅ On manual restarts of `bin/dev`
- ❌ NOT on file changes during development
- ❌ NOT on webpack hot reload

For production builds, ensure tasks are included in `build_production_command`.

3. Testing Considerations

Observation: The PR description mentions documentation testing, but the pattern itself involves executable code in bin/dev.

Questions:

• Should there be integration tests verifying the template's commented code is syntactically valid?
• How can users verify their customizations work before deployment?

Suggestion: Add a "Testing" subsection:

## Testing Your Configuration

Before relying on this pattern in production:

1. **Verify syntax**: Ensure your `bin/dev` has valid Ruby syntax
   ```bash
   ruby -c bin/dev

2. Test precompile tasks: Run tasks individually to verify they work

   bundle exec rails runner 'ReactOnRails::Locales.compile(force: true)'

3. Full integration test: Start bin/dev and verify all tasks execute

### 4. Minor: Inconsistent YAML Comment Style
**Location**: `docs/building-features/extensible-precompile-pattern.md`:100

```yaml
# precompile_hook: (not configured)

This looks like it should be valid YAML but the (not configured) part might confuse users. Consider:

# precompile_hook not configured - handled by bin/dev instead

5. Security: Command Injection in Examples

Location: Multiple places showing system("yarn res:build")

While the examples are safe, users might extend this pattern with user input. Consider adding a security note:

### Security Note

When adding custom build commands, avoid using user input or environment variables directly in `system()` calls. Use array form for complex commands:

```ruby
# Safe
system(["yarn", "res:build"])

# Potentially unsafe if MY_VAR comes from untrusted source
# system("yarn res:build --flag=\#{ENV['MY_VAR']}")

## 🔍 Potential Issues

### 1. RuboCop Compliance
**Question**: Have you run `bundle exec rubocop` on the changed files? The template file `bin/dev` is executable Ruby code that should be linted.

```bash
bundle exec rubocop react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/dev

2. CHANGELOG Format

Location: CHANGELOG.md:32

The changelog entry looks good overall. Verify it matches the format specified in AGENTS.md:167:

• ✅ Uses [PR 2349] format (no hash)
• ✅ Includes author link
• ✅ Describes user-visible feature

🎯 Test Coverage

Based on the PR description, the test plan focuses on documentation:

• Documentation renders correctly on the docs site
• Code examples are syntactically correct
• Links work

Recommendation: Also verify:

• Generated bin/dev from template has valid Ruby syntax
• Commented code in template can be uncommented and runs without errors
• RDoc for ReactOnRails::Locales.compile renders correctly

📊 Performance Considerations

The PR claims performance benefits from avoiding shell spawning. This is accurate:

• ✅ Direct Ruby API calls eliminate process creation overhead
• ✅ Particularly beneficial for repeated invocations

No performance concerns with this approach.

🔒 Security Assessment

• ✅ No secrets or credentials exposed
• ✅ No SQL injection risks (no database operations)
• ✅ Template code is commented by default (safe activation)
• ⚠️ Minor: Could add guidance about safe command construction (see suggestion Add Unit tests #5)

🧪 Final Recommendation

Status: ✅ Approve with minor suggestions

This is a high-quality PR that:

• Solves real user pain points
• Maintains backward compatibility
• Provides excellent documentation
• Follows project conventions

The suggestions above are minor improvements that could be addressed in this PR or follow-ups. None are blocking issues.

Before Merge Checklist

• Run bundle exec rubocop to verify template code compliance
• Verify documentation renders correctly in the docs site preview
• Confirm all links in new documentation work

Great work on this feature! The dual-approach strategy (keeping the existing precompile_hook while offering this alternative) is exactly the right design choice. 🎉

@greptile, review this PR.
@claude, review this PR.

Claude finished @justin808's task in 2m 11s —— View job

Code Review: Add extensible bin/dev precompile pattern

Summary

This PR introduces a well-documented alternative approach for handling precompile tasks directly in bin/dev, providing more flexibility than the default precompile_hook mechanism. The implementation is solid, documentation is comprehensive, and the approach is backward-compatible.

✅ Strengths

1. Excellent Documentation Quality

• The new guide (extensible-precompile-pattern.md) is comprehensive, well-structured, and follows a logical progression
• Clear comparison tables help users decide which approach to use
• Practical examples are realistic and directly applicable
• Good cross-referencing between related docs (process-managers.md, i18n.md)

2. Non-Breaking, Opt-In Design

• All changes are additive - no existing functionality is modified
• Template code is commented out by default, preventing accidental activation
• Users can choose between default precompile_hook or the new extensible pattern

3. Solves Real Problems

• Addresses legitimate pain points: version manager compatibility (mise, asdf, rbenv)
• Provides faster execution via direct Ruby API (no shell spawning overhead)
• Simplifies complex build workflows with multiple precompile tasks

4. Well-Documented API

• RDoc comments added to ReactOnRails::Locales.compile follow best practices
• Includes proper @param, @return, @raise, and @example tags
• Makes the public API status clear and explicit

5. Good Test Coverage

• The underlying ReactOnRails::Locales.compile method has comprehensive unit tests in react_on_rails/spec/react_on_rails/locales_spec.rb
• Tests cover both output formats (JSON/JS) and the force parameter

🔍 Issues & Suggestions

1. Precompile Timing Trade-off Needs Clarification ⚠️

Issue: The comparison table at line 179 shows "Once at dev server startup" vs "Before each webpack compile" as a neutral difference, but this has important implications:

• With the extensible pattern, if a developer modifies locale YAML files while the dev server is running, they must manually re-run precompile tasks or restart bin/dev
• The default precompile_hook re-runs automatically before each webpack compile, picking up changes automatically

Impact: This is a meaningful trade-off that developers should understand when choosing between approaches.

Recommendation: Update the comparison table or add a note in the "Execution Timing" section (lines 181-192) to explicitly call out this trade-off. Something like:

**Important**: With this pattern, changes to locale YAML files (or other precompile inputs) 
while the dev server is running will require manually restarting `bin/dev` or re-running 
the precompile tasks. The default `precompile_hook` automatically picks up such changes 
before each webpack compile.

This has also been noted in previous CodeRabbit review feedback.

2. Error Handling Inconsistency in Examples (Minor)

Location: extensible-precompile-pattern.md:40-54

The ReScript build example (lines 40-45) has explicit error handling with early exit:

unless system("yarn res:build")
  puts "❌"
  exit(1)
end

But the locale generation example (lines 52-54) doesn't check for errors:

ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present?
puts "✅"

Why this works: Looking at the actual implementation in locales/base.rb, exceptions will bubble up (which is desired behavior), and the method handles edge cases gracefully.

Recommendation: The updated comment (lines 48-51) now explains this behavior, which is good. However, for consistency, consider showing both patterns or adding a brief note comparing the two error handling approaches.

3. YAML Configuration Example Could Be Clearer

Location: extensible-precompile-pattern.md:93-102

The "After" example shows:

  # precompile_hook not configured - handled by bin/dev instead

Suggestion: Make it clearer that users should either comment out or remove the existing precompile_hook line. Consider showing it more explicitly:

default: &default
  # ... other settings ...

  # When using the extensible bin/dev pattern, remove or comment out precompile_hook:
  # precompile_hook: 'bundle exec rake react_on_rails:locale'  # ← Remove this line

4. Package Manager Neutrality (Minor)

Location: Multiple places in extensible-precompile-pattern.md (lines 41, 112, 114, 135, 136)

The examples hardcode yarn commands (e.g., yarn res:build). Per repository standards, end-user documentation should be package-manager-agnostic or mention alternatives.

Recommendation: Add a note like:

# Use your project's package manager (npm, yarn, or pnpm)
unless system("yarn res:build")  # or: npm run res:build, pnpm res:build

This is a minor issue since the examples are just illustrations, but it would improve clarity.

5. Documentation Link Verification

Location: bin/dev template line 38

The template includes:

# See documentation: https://www.shakacode.com/react-on-rails/docs/building-features/extensible-precompile-pattern

Recommendation: Verify this URL will work when the docs are deployed. Based on the file location, this looks correct, but it's worth confirming before merge.

📊 Code Quality Assessment

Ruby Code Style ✅

• Follows repository conventions (frozen_string_literal, proper indentation)
• Line lengths appear to be under 120 characters
• Commented code is properly formatted

Documentation Style ✅

• Markdown is well-formatted
• Code blocks use proper syntax highlighting
• Tables are properly aligned
• Cross-references use correct relative paths

Security ✅

• No security concerns identified
• The system("yarn res:build") calls in examples use hardcoded commands (no user input interpolation)
• Template code is commented by default, preventing accidental execution

Performance ✅

• The direct Ruby API approach correctly avoids shell spawning overhead
• Idempotent file checking in locales/base.rb prevents unnecessary regeneration
• Performance benefits are accurately documented

🧪 Testing Considerations

Current Test Coverage ✅

• ReactOnRails::Locales.compile has comprehensive unit tests covering:
  • Output format selection (JSON/JS)
  • Force parameter behavior
  • Parameter passing to converter classes

Potential Additions

While not blocking for this PR, consider adding:

1. Integration test verifying the full run_precompile_tasks pattern works end-to-end
2. Test that the commented template code is syntactically valid Ruby
3. Documentation link validation in CI

However, these are nice-to-haves given the opt-in nature and existing test coverage.

📋 CHANGELOG Review ✅

The CHANGELOG entry follows repository guidelines:

• ✅ Correctly formatted: [PR 2349](https://github.com/shakacode/react_on_rails/pull/2349) by [justin808](https://github.com/justin808)
• ✅ Describes a user-visible feature (appropriate for CHANGELOG)
• ✅ Clear, concise description of the feature and its benefits

🎯 Final Recommendations

Must Address (Before Merge):

1. Clarify the precompile timing trade-off - Update documentation to explicitly explain that locale YAML changes require manual restart with this pattern (addresses CodeRabbit feedback)

Should Consider (Minor improvements):

1. Add package-manager-agnostic notes in examples
2. Clarify YAML configuration removal more explicitly
3. Verify documentation URL is correct

Nice to Have (Future work):

1. Add integration test for the full pattern
2. Add documentation link validation to CI

✅ Overall Assessment

This is a high-quality PR that provides valuable functionality for users with complex build requirements. The documentation is excellent, the implementation is sound, and the backward-compatible approach is exactly right.

Recommendation: Approve with minor documentation clarification (precompile timing trade-off)

The only blocking issue is clarifying the precompile timing trade-off in the documentation. Once that's addressed, this is ready to merge.

Review conducted following AGENTS.md and CLAUDE.md guidelines

View PR Branch |

PR Review: Extensible bin/dev Precompile Pattern (#2349)

Overall Assessment

This PR provides a well-documented alternative approach to handling precompile tasks. The implementation is solid, documentation is comprehensive, and the pattern addresses real pain points with version managers and complex build setups. Approved with minor suggestions below.

Code Quality & Best Practices ✅

Strengths:

• Excellent inline documentation in the bin/dev template with clear usage instructions
• Comprehensive RDoc documentation added to ReactOnRails::Locales.compile (base.rb:7-21)
• Follows Ruby conventions (frozen_string_literal, proper indentation)
• Code is idiomatic and readable throughout

Suggestions:

1. Minor: Error handling consistency - In the bin/dev example (line 41-45), consider documenting what happens if system("yarn res:build") returns false vs raises an exception. The locale compile call (line 58) can raise exceptions which will stop the server, but the ReScript build just exits. Both behaviors are correct, but documenting the difference would help users.

Potential Bugs/Issues 🔍

None identified. The implementation is defensive:

• Uses guard clause if ReactOnRails.configuration.i18n_dir.present? to prevent errors when i18n isn't configured
• The compile method already validates directories exist (base.rb:24-31)
• Commented-out code in template prevents accidental activation

Performance Considerations ⚡

Positive impacts:

• Direct Ruby API calls avoid shell spawning overhead (documented benefit)
• Idempotent checks prevent unnecessary regeneration
• Runs once at startup rather than on each webpack compile (for the bin/dev pattern)

Trade-off clearly documented:

• The documentation correctly notes timing differences between precompile_hook (runs before each webpack compile) vs this pattern (runs once at startup) in extensible-precompile-pattern.md:179

Security Concerns 🔒

No security issues identified.

• No user input is directly executed
• The system() call in examples uses literal strings
• File path handling goes through Rails configuration (validated)
• Template code is commented out by default (safe)

Test Coverage 📋

Current state:
This PR is primarily documentation-focused with a small code addition (RDoc comments to an existing public method).

Test coverage assessment:

• The ReactOnRails::Locales.compile method already has test coverage (it's been public API since i18n support was added)
• The bin/dev template is a generator template, tested via generator tests
• Documentation changes don't require automated tests

Recommendation:
The test plan in the PR description is appropriate. Consider manually verifying:

1. The commented code in bin/dev template can be uncommented and runs without syntax errors
2. The example in extensible-precompile-pattern.md:30-78 is copy-pasteable and functional

Documentation Quality 📚

Excellent overall. The new guide is well-structured with clear overview, step-by-step implementation, benefits comparison, troubleshooting section, and FAQ.

Minor suggestions:

1. CHANGELOG.md:30 - Consider rewording: "providing better support for projects with custom build steps" (add "projects with")

2. i18n.md:24 - The new Option A/Option B structure is clear, but consider adding a brief note about when to use which option

Code Style Compliance

Checked against AGENTS.md requirements:

• ✅ Line length under 120 chars (RuboCop will verify)
• ✅ Proper Ruby formatting
• ✅ Documentation follows conventions
• ✅ New doc file correctly placed in docs/building-features/
• ✅ CHANGELOG.md updated with correct format (no # before PR number)

Architecture & Design ⚙️

Well thought out:

• Provides flexibility without breaking existing patterns
• Opt-in design (commented by default) prevents breaking changes
• Clear migration path documented
• Addresses real user pain points (version managers, multiple build steps)

The pattern introduces two ways to accomplish the same thing, which is appropriate here because different approaches suit different project complexities.

Recommendations Summary

Must fix: None

Should consider:

1. Add brief guidance in i18n.md about when to use Option A vs Option B
2. Manually test the commented bin/dev code can be uncommented and runs

Nice to have:

1. Minor CHANGELOG wording improvement for clarity

Conclusion

This is a high-quality PR that:

• Solves a real problem (Enhancement: Extensible bin/dev precompile pattern as alternative to precompile_hook #2347)
• Provides excellent documentation
• Maintains backward compatibility
• Follows all repository conventions

The code is production-ready. Great work! 🎉

Review comment from Claude Code - detailed analysis follows in next comment

Code Review: Add extensible bin/dev precompile pattern

Summary

This PR introduces a well-designed alternative pattern for handling precompile tasks in React on Rails projects. The implementation is solid, and the documentation is thorough.

✅ Strengths

1. Excellent Documentation Structure

• The new extensible-precompile-pattern.md guide is comprehensive with clear use cases, implementation steps, and troubleshooting
• Good use of comparison tables to help users choose between approaches
• Links to related documentation are well-placed
• Code examples are practical and production-ready

2. Backward Compatibility

• Pattern is entirely opt-in (commented by default in template)
• Does not break existing precompile_hook approach
• Clear migration path documented

3. API Design

• ReactOnRails::Locales.compile is properly documented with RDoc
• Good parameter design with force: option for explicit regeneration
• Idempotent by default (safe to call multiple times)

4. Code Quality

• Template code in bin/dev follows Ruby style conventions
• Frozen string literal pragma included
• Helpful emoji indicators for task progress
• Clear comments explaining the pattern's purpose

🔍 Areas for Improvement

1. Error Handling in bin/dev Template (Minor)

Location: react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/dev:42-50

If ReactOnRails::Locales.compile raises an exception (e.g., file permission errors, malformed YAML), the success indicator will never print, but the error message could be confusing. The ReScript example properly handles this with unless system(...) exit(1) end.

Suggestion: Consider wrapping in error handling or document that exceptions are intentionally allowed to bubble up (which is mentioned later in comments but not clearly in the example).

2. Documentation: Production Build Clarity (Minor)

Location: docs/building-features/extensible-precompile-pattern.md:126-137

Add a note about locale generation in production builds. If someone uses this pattern and removes precompile_hook, they need to ensure locale generation happens in production deployments.

3. i18n.md Documentation Flow (Minor)

Location: docs/building-features/i18n.md:22-62

The section now presents "Option A" (Direct Ruby API) before "Option B" (Rake task), and then has a "Recommended" subsection about precompile_hook. This creates some confusion about which approach is actually recommended.

Suggestion: Restructure to have three distinct approaches with clearer guidance on when to use each.

4. Minor Consistency Issues

• The bin/dev template uses ReactOnRails::Locales.compile if ReactOnRails.configuration.i18n_dir.present? but the documentation example shows it without the conditional. Should align these.
• Line 58 in extensible-precompile-pattern.md: comment inside code example could be clearer

🔒 Security Assessment

No security concerns identified.

• No user input is processed in the new code
• File paths use proper Rails configuration
• No shell injection risks (uses Ruby API, not shell commands for locale generation)
• The system() call in the ReScript example is safe (no interpolation)

⚡ Performance Considerations

Positive performance impact:

1. Direct Ruby API vs. Rake Task: Eliminates shell spawning overhead
2. Idempotent Generation: Smart caching prevents unnecessary regeneration
3. One-time Execution: Running precompile tasks once at startup reduces repeated work

No performance concerns identified.

🧪 Test Coverage

This PR adds documentation and template changes but does not include tests for the new ReactOnRails::Locales.compile public API documentation.

Recommendation: Verify that existing test coverage for ReactOnRails::Locales.compile includes:

• Idempotent behavior (skips when up-to-date)
• Force regeneration with force: true
• Proper error handling for missing directories
• Behavior when i18n_dir is not configured

🎯 Final Recommendation

APPROVED with minor suggestions.

This is a well-thought-out addition that solves real problems (version manager compatibility, complex build pipelines) while maintaining backward compatibility. The documentation is thorough and the code quality is high.

Before Merging:

1. ✅ Consider error handling improvements in the bin/dev template example
2. ✅ Verify test coverage for ReactOnRails::Locales.compile force parameter
3. ✅ Clarify production deployment locale generation in the docs
4. ✅ Consider restructuring i18n.md to reduce confusion between approaches

None of these are blocking issues - they are refinements that would make an already-good PR even better.

Great work! 🚀

Test Plan Verification

Additional suggested testing:

• Verify the commented bin/dev code works when uncommented in a fresh install
• Test that removing precompile_hook and using bin/dev pattern works correctly
• Verify ReactOnRails::Locales.compile works in a non-Rails environment (bin/dev context)

Positive Patterns Worth Highlighting:

1. Progressive Disclosure: The pattern is commented by default, requiring users to actively choose it
2. Clear Benefits Table: Comparison tables help users make informed decisions
3. Version Compatibility Note: Explicitly states React on Rails 16.2+ requirement
4. Troubleshooting Section: Anticipates common issues