Fix pack generation in bin/dev by preventing Bundler auto-exec interception #2085
Conversation
WalkthroughThe PR fixes a bug where pack generation fails when running Changes
Sequence DiagramsequenceDiagram
participant Caller
participant PackGenerator
participant BundlerAPI
participant System
Caller->>PackGenerator: run_via_bundle_exec(silent: false)
PackGenerator->>PackGenerator: with_unbundled_context(&block)
alt Bundler defined
alt with_unbundled_env available
PackGenerator->>BundlerAPI: Bundler.with_unbundled_env
activate BundlerAPI
BundlerAPI->>System: system("bundle", "exec", "rake", ...)
System-->>BundlerAPI: success
deactivate BundlerAPI
else with_clean_env fallback
PackGenerator->>BundlerAPI: Bundler.with_clean_env
activate BundlerAPI
BundlerAPI->>System: system("bundle", "exec", "rake", ...)
System-->>BundlerAPI: success
deactivate BundlerAPI
else neither available
PackGenerator->>System: system("bundle", "exec", "rake", ...)
System-->>PackGenerator: success
end
else Bundler not defined
PackGenerator->>System: system("bundle", "exec", "rake", ...)
System-->>PackGenerator: success
end
PackGenerator-->>Caller: completion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #2085✅ Overall AssessmentThis PR successfully fixes the pack generation failure in 🎯 Strengths
🔍 Code Quality ObservationsGood:
Potential Consideration:
🧪 Test CoverageCurrent Coverage: The existing test suite in
Gap Identified: The new
Example test structure: context "when calling bundle exec from within Bundler context" do
it "unwraps the Bundler context before executing" do
stub_const("Bundler", Module.new)
allow(Bundler).to receive(:respond_to?).with(:with_unbundled_env).and_return(true)
allow(Bundler).to receive(:with_unbundled_env).and_yield
described_class.generate(verbose: false)
expect(Bundler).to have_received(:with_unbundled_env)
end
end🔒 Security & PerformanceSecurity: ✅ No security concerns. The change only affects how subprocesses are spawned. Performance: ✅ Negligible impact. The unbundling operation is lightweight and only affects the 📋 Checklist Items (from CLAUDE.md)
📝 Recommendations
🎓 Educational NoteThis is an excellent example of a subtle Bundler interaction bug. The issue occurs because:
This pattern is worth documenting in the project's knowledge base for future reference. Overall: LGTM with minor suggestions 👍 The core fix is solid and addresses the reported issue. The suggestions are minor enhancements rather than blockers. |
Code Review - PR #2085✅ SummaryThis PR successfully fixes a real bug where pack generation fails when running 🎯 Strengths1. Correct Root Cause Analysis
2. Excellent Test Coverage
3. Good Code Quality
4. Proper Changelog Entry
🔍 Observations & Suggestions1. Code Duplication Opportunity (Minor) The
Note: The Question: Should these be consolidated into a shared module/concern? Or is the duplication acceptable given the small size? If you want to DRY this up, consider: # lib/react_on_rails/dev/bundler_context.rb
module ReactOnRails
module Dev
module BundlerContext
def with_unbundled_context(&block)
if defined?(Bundler)
if Bundler.respond_to?(:with_unbundled_env)
Bundler.with_unbundled_env(&block)
elsif Bundler.respond_to?(:with_clean_env)
Bundler.with_clean_env(&block)
else
yield
end
else
yield
end
end
end
end
endThen include it in both classes. However, this is optional - the current duplication is minimal and may be acceptable. 2. Inconsistency with process_manager.rb
Recommendation: Either:
✅ Security & PerformanceSecurity: ✅ No concerns
Performance: ✅ No concerns
🧪 Testing VerificationQuestion: Have you manually tested this with:
The automated tests look excellent, but a quick manual verification would confirm the fix works end-to-end. 📝 Final RecommendationApprove with optional follow-up: ✅ Merge as-is - The fix is correct and well-tested 🔄 Optional follow-up: Consider DRYing up the Great work identifying and fixing this subtle Bundler interaction issue! 🚀 |
Code Review - PR #2085SummaryThis PR fixes a critical bug where pack generation fails when running ✅ Strengths1. Excellent Problem Analysis
2. Consistent with Existing Patterns
3. Backward Compatibility Handled Well
4. Comprehensive Test Coverage
5. Proper Changelog Entry
🔍 Observations & Suggestions1. Code Duplication (Minor)The
Suggestion for future refactoring (not blocking for this PR):
The current approach is acceptable, especially for a bug fix. The duplication could be addressed in a follow-up refactoring PR. 2. Test OrganizationThe new tests are well-organized and placed in an appropriate context block. The use of 3. Security Considerations✅ No security concerns identified:
4. Performance Considerations✅ Minimal performance impact:
🎯 Testing RecommendationsWhile the unit tests are comprehensive, consider these additional manual tests:
📋 Checklist
🚀 RecommendationAPPROVE - This PR is ready to merge once CI passes. The fix is:
The minor code duplication noted above is acceptable for a bug fix and can be addressed in a future refactoring if desired. Great work on the thorough analysis and implementation! The detailed issue description and comprehensive test coverage make this an exemplary bug fix PR. |
…eption When running bin/dev, pack generation was failing with "Could not find command 'react_on_rails:generate_packs'" because Bundler was intercepting the system call and trying to execute it as a gem executable command instead of passing it through to rake. The fix wraps the bundle exec subprocess call with Bundler.with_unbundled_env to prevent Bundler from intercepting when already running inside a Bundler context. Added with_unbundled_context helper method that supports both new (with_unbundled_env) and legacy (with_clean_env) Bundler APIs for backwards compatibility. Fixes #2084 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Added comprehensive test coverage for the with_unbundled_context wrapper to verify the Bundler API compatibility chain: * Tests with_unbundled_env (modern Bundler API) * Tests with_clean_env (legacy Bundler API) * Tests fallback when neither method is available * Tests execution when Bundler is not defined - Updated CHANGELOG.md with bug fix entry for issue #2084 All 18 examples in pack_generator_spec.rb pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
jg-/fix-pack-gen-bundler
branch
from
4e98029 to
86594f2
Compare
Code Review SummaryThis PR successfully fixes the Bundler auto-exec interception issue in pack generation. The implementation is solid, well-tested, and follows established patterns in the codebase. ✅ ✅ Strengths1. Correct Root Cause AnalysisThe PR correctly identifies that Bundler intercepts 2. Follows Existing PatternsThe
3. Excellent Backwards CompatibilityThe
4. Comprehensive Test CoverageThe test suite covers all code paths:
The tests properly use 5. Clear Documentation
💡 Minor Observations1. Code Duplication with process_manager.rbThe
Analysis: This is acceptable because:
Recommendation: If a third file needs this pattern, consider extracting to 2. Test Assertions Could Be More ExplicitIn lines 217-236 of the test file, the test verifies Current behavior: ✅ Correct (the yield ensures it) 3. Changelog Entry FormattingThe changelog entry follows the required format correctly:
🔒 Security ConsiderationsNo security concerns identified:
⚡ Performance ConsiderationsMinimal impact:
🧪 Test CoverageCoverage: Excellent ✅ The new tests cover:
Test quality:
📝 RecommendationsBefore Merge
Future Considerations
🎯 ConclusionVerdict: Ready to merge ✅ This PR:
The implementation is production-ready and aligns with the project's quality standards per CLAUDE.md. Review conducted using React on Rails project guidelines from CLAUDE.md |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/react_on_rails/dev/pack_generator.rb (1)
143-172: Bundler unbundling helper looks correct; consider DRY’ing withprocess_managerlater
run_via_bundle_execnow correctly executes the rake task insidewith_unbundled_context, andwith_unbundled_context’swith_unbundled_env/with_clean_env/yield fallback mirrors the existing pattern inlib/react_on_rails/dev/process_manager.rb. Return values fromsystempropagate as before, sorun_pack_generationsemantics are preserved. If this pattern continues to be reused, you might eventually want a shared dev helper module to avoid duplication betweenPackGeneratorandProcessManager. Based on learnings.CHANGELOG.md (1)
30-32: Wording tweak to match legacy Bundler fallback behaviorThe implementation wraps the
bundle execcall in a helper that prefersBundler.with_unbundled_envbut also falls back toBundler.with_clean_env(or a no-op) for older Bundler versions. Consider updating the text to something like “wraps the bundle exec call in an unbundled Bundler context (viaBundler.with_unbundled_envorBundler.with_clean_envwhen available)” so the changelog precisely reflects the behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)lib/react_on_rails/dev/pack_generator.rb(1 hunks)spec/react_on_rails/dev/pack_generator_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
lib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rb
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/dev/pack_generator_spec.rb
🧬 Code graph analysis (2)
lib/react_on_rails/dev/pack_generator.rb (2)
lib/react_on_rails/dev/process_manager.rb (1)
with_unbundled_context(120-129)spec/react_on_rails/dev/pack_generator_spec.rb (5)
respond_to?(219-221)respond_to?(241-243)respond_to?(263-265)with_unbundled_env(223-225)with_clean_env(245-247)
spec/react_on_rails/dev/pack_generator_spec.rb (1)
lib/react_on_rails/dev/pack_generator.rb (2)
generate(27-175)generate(29-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (1)
spec/react_on_rails/dev/pack_generator_spec.rb (1)
210-293: Thorough coverage of Bundler context unwrapping behaviorThese examples nicely exercise all branches of
with_unbundled_context:with_unbundled_env,with_clean_env, and the plain-yield paths (including whenBundleris undefined), while still asserting thesysteminvocation and user-facing output. This gives good confidence thatrun_via_bundle_execbehaves correctly across Bundler versions and environments.
Summary
Fixes #2084 - Pack generation now works correctly when running
bin/devfrom within a Bundler context.Problem
When running
bin/dev, pack generation was failing with:The pack generation actually succeeded (note the ✅), but then Bundler tried to execute
bundle react_on_rails:generate_packsas a gem executable command instead ofbundle exec rake react_on_rails:generate_packs, which failed.Root Cause
In
lib/react_on_rails/dev/pack_generator.rb, therun_via_bundle_execmethod uses:When this is called from within an existing Bundler context (which is the case when running
bin/dev), Bundler intercepts thesystemcall and tries to parse the arguments. It sees "react_on_rails:generate_packs" and attempts to execute it as a gem executable via Bundler's auto-exec feature, rather than passing it through to rake.Solution
The fix wraps the
bundle execsubprocess call withBundler.with_unbundled_envto prevent Bundler from intercepting when already running inside a Bundler context:Added a
with_unbundled_contexthelper method that supports both new (with_unbundled_env) and legacy (with_clean_env) Bundler APIs for backwards compatibility.Testing
process_manager.rbImpact
This fix affects all users of
bin/devin react_on_rails when:🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.