Improve pack generation to avoid unnecessary subprocess spawning

[justin808]

Summary

Refactors pack_generator.rb to intelligently choose between direct Rake task execution and bundle exec based on the current context. When running in a Rails/Bundler environment (e.g., from bin/dev), the pack generation now runs the Rake task directly, avoiding the overhead of spawning a subprocess.

Key Improvements

• Adds run_pack_generation method that detects Rails availability
• Implements run_rake_task_directly for in-process execution when Rails is available
• Falls back to run_via_bundle_exec when Rails is not available
• Includes proper silent mode handling for both execution paths
• Prevents dead code by ensuring Rails is always defined before use

Dead Code Prevention

The key improvement is that the implementation never includes the dead code that was identified in the shakapacker-9.3.0 branch (commit b716f08). Specifically, these 3 unreachable lines were never added:

# Load Rails environment if not already loaded
require File.expand_path("config/environment", Dir.pwd) unless defined?(Rails)

This code would have been dead because:

• run_rake_task_directly is only called when rails_available? returns true
• rails_available? explicitly checks defined?(Rails) and returns false if Rails is not defined
• Therefore, Rails is always defined when run_rake_task_directly executes
• The unless defined?(Rails) guard would never be true, making the require statement unreachable

Files Changed

• lib/react_on_rails/dev/pack_generator.rb (57 lines added, 2 lines modified)

Impact

• Cleaner, more maintainable code
• Better performance when running in a Rails/Bundler context (avoids unnecessary subprocess spawning)
• No functional changes to existing behavior
• No breaking changes

Testing

• Existing tests pass: bundle exec rspec spec/react_on_rails/dev/pack_generator_spec.rb
• RuboCop passes with zero offenses
• Git hooks (Prettier, trailing newlines, RuboCop) all pass

Fixes #1909

---

This change is 

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 14 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3182910 and 9c5422f.

📒 Files selected for processing (1)

• lib/react_on_rails/dev/pack_generator.rb (2 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/remove-dead-code

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

PR Review: Improve pack generation to avoid unnecessary subprocess spawning

Overview

This PR refactors pack_generator.rb to intelligently choose between direct Rake task execution and subprocess spawning based on the current context. Overall, this is a well-designed improvement that addresses the dead code issue from #1909 and optimizes performance when running in a Rails/Bundler environment.

---

✅ Strengths

1. Clean Architecture

• The separation between run_rake_task_directly and run_via_bundle_exec is clear and maintainable
• The rails_available? guard method is well-designed with multiple safety checks
• Good use of method extraction to keep code readable

2. Dead Code Prevention

• Correctly avoids the dead code pattern identified in the shakapacker-9.3.0 branch
• The rails_available? check ensures Rails is always defined before calling run_rake_task_directly
• Logic flow is sound: defined?(Rails) → rails_available? → run_rake_task_directly

3. Proper Error Handling

• Good use of rescue StandardError with appropriate error messaging
• Silent mode is properly handled in both execution paths
• Graceful fallback to bundle exec when Rails isn't available

4. Performance Optimization

• Avoids unnecessary subprocess spawning when already in a Rails context (e.g., from bin/dev)
• Smart detection of execution context with defined?(Bundler) && rails_available?

---

⚠️ Issues & Concerns

1. Critical: Test Coverage Gap ⚠️

The existing tests in spec/react_on_rails/dev/pack_generator_spec.rb are not updated to cover the new code paths:

• Tests still stub system calls with hardcoded bundle exec commands (lines 10, 17, 25)
• New methods run_pack_generation, rails_available?, run_rake_task_directly, and run_via_bundle_exec are completely untested
• No tests for the branching logic between direct execution and bundle exec
• No tests for Rails availability detection
• No tests for silent mode handling in run_rake_task_directly

Recommendation: Add comprehensive tests covering:

describe ".run_pack_generation" do
  context "when Rails is available and in Bundler context" do
    # Test direct execution path
  end
  
  context "when Rails is not available" do
    # Test bundle exec fallback
  end
end

describe ".rails_available?" do
  # Test various Rails states
end

describe ".run_rake_task_directly" do
  # Test successful execution, errors, silent mode
end

2. Potential Issue: Task Loading Side Effects

Rails.application.load_tasks unless Rake::Task.task_defined?("react_on_rails:generate_packs")

Concerns:

• load_tasks loads ALL Rake tasks from the application, not just the one needed
• Could have side effects if called multiple times or in unexpected contexts
• If load_tasks is called when tasks are already loaded but the check fails for some reason, it could reload tasks unnecessarily

Recommendation: Consider catching the task not found error instead:

begin
  task = Rake::Task["react_on_rails:generate_packs"]
rescue RuntimeError
  Rails.application.load_tasks
  task = Rake::Task["react_on_rails:generate_packs"]
end

3. Silent Mode Implementation: Global State Mutation

original_stdout = $stdout
original_stderr = $stderr
$stdout = StringIO.new
$stderr = StringIO.new

Concerns:

• Mutating global variables ($stdout, $stderr) can be problematic in multi-threaded contexts
• If an exception occurs before the ensure block (e.g., in task invocation), output could be lost
• Not thread-safe

Recommendation: Consider using capture_subprocess_io from Open3 or ensure this is only called in single-threaded contexts. The current implementation is acceptable for the bin/dev use case but worth documenting.

4. Missing StringIO Require

$stdout = StringIO.new

Issue: StringIO is used but not explicitly required at the top of the file. While it's part of Ruby's standard library and often auto-loaded, it's better to be explicit.

Recommendation: Add require "stringio" to the top of the file.

---

🔍 Code Quality Observations

1. Consistent with Existing Patterns

The code follows the repository's style guidelines:

• Uses frozen string literal
• Method visibility (private) is properly set
• Silent mode pattern matches existing code in server_manager.rb:424

2. Good Documentation

The inline comment clearly explains the optimization:

# If we're already inside a Bundler context AND Rails is available (e.g., called from bin/dev),
# we can directly require and run the task. Otherwise, use bundle exec.

---

🛡️ Security Considerations

✅ No security concerns identified

• No user input is used in system calls
• No shell injection risks (using proper Ruby method calls)
• Fallback to bundle exec is safe

---

🎯 Performance Considerations

✅ Positive Performance Impact

• Eliminates subprocess overhead when running from bin/dev
• Direct Rake task execution is significantly faster than spawning bundle exec
• Task reenabling allows multiple invocations without reload

---

📋 Before Merge Checklist

Based on CLAUDE.md requirements:

• CRITICAL: Run bundle exec rubocop and ensure zero offenses
• CRITICAL: Ensure all files end with newline character
• Add comprehensive test coverage for new methods
• Consider adding require "stringio"
• Consider alternative task loading approach to avoid load_tasks side effects
• Update CHANGELOG.md (this is a performance improvement worthy of mention)
  • Format: [PR 1915](https://github.com/shakacode/react_on_rails/pull/1915) by [justin808](https://github.com/justin808)

---

💡 Suggestions for Follow-up

1. Documentation: Consider adding a comment explaining why task.reenable is needed
2. Metrics: Could add timing logs to quantify the performance improvement
3. Edge Cases: Test behavior when Rake task doesn't exist or fails to load

---

✅ Verdict

Conditionally Approve pending:

1. Addition of comprehensive test coverage
2. RuboCop passing with zero offenses
3. Minor improvements (StringIO require, task loading consideration)

The core refactoring is solid and achieves its goals. The main concern is the lack of test coverage for the new code paths, which is essential for maintainability and preventing regressions.

Great work on identifying and eliminating the dead code! 🎉

[justin808]

Closing this PR as it's now redundant. The same refactoring was just merged in PR #1907, which implements a more robust version of the fix with better error handling and additional safety checks (like verifying BUNDLE_GEMFILE environment variable). Issue #1909 has been resolved by that PR.