Fix generator inheriting BUNDLE_GEMFILE from parent process #2288
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGenerator subprocesses that run bundler commands now execute inside Bundler.with_unbundled_env and Gemfile/Gemfile.lock checks always target the generated app’s files at project root, preventing inheritance of the parent process's BUNDLE_GEMFILE. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / Rails generator
participant Gen as InstallGenerator
participant Bundler as Bundler.with_unbundled_env
participant Shell as subprocess (bundle / rake)
participant FS as Target app filesystem (Gemfile, Gemfile.lock)
Dev->>Gen: run react_on_rails:install
Gen->>FS: read project-root `Gemfile.lock` / `Gemfile`
Gen->>Bundler: enter unbundled block
Bundler->>Shell: run `bundle add shakapacker --strict`
Shell->>FS: update target `Gemfile`/`Gemfile.lock`
Bundler->>Shell: run `bundle install`
Bundler->>Shell: run `bundle exec rails shakapacker:install`
Shell->>FS: run shakapacker installer in target app
Bundler-->>Gen: return success/failure
Gen-->>Dev: complete with status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #2288Summary: This PR fixes a legitimate bug where the install generator inherits ✅ Strengths1. Problem Correctly Identified
2. Solution Follows Established PatternsThe fix uses
3. Minimal, Focused Change
4. Code Quality
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Fixes BUNDLE_GEMFILE inheritance from parent process by wrapping bundler commands with Bundler.with_unbundled_env.
- Correctly wraps three
system()calls withBundler.with_unbundled_envto prevent environment variable pollution - Follows the documented solution from Bundler and Rails issues
- Successfully prevents "injected gems" conflicts during generator execution
Minor improvement opportunity: The codebase has an established pattern in process_manager.rb and pack_generator.rb that uses a with_unbundled_context helper method with fallback logic for older Bundler versions. Consider using the same pattern for consistency, though Bundler.with_unbundled_env is available in all supported Bundler versions (2.x+).
Confidence Score: 4/5
- This PR is safe to merge - it solves a real bug with a well-established solution
- The fix correctly addresses the BUNDLE_GEMFILE inheritance issue using the officially documented solution. The implementation is straightforward and matches patterns already used elsewhere in the codebase. Minor style inconsistency with existing
with_unbundled_contexthelper pattern prevents a perfect score, but doesn't affect functionality. - No files require special attention - the change is minimal and localized
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| react_on_rails/lib/generators/react_on_rails/install_generator.rb | 4/5 | Wraps bundler commands with Bundler.with_unbundled_env to prevent BUNDLE_GEMFILE inheritance. Minor inconsistency with existing codebase pattern for backward compatibility. |
Sequence Diagram
sequenceDiagram
participant User
participant Rails as Rails CLI
participant Generator as InstallGenerator
participant Bundler
participant System as System Shell
User->>Rails: bundle exec rails g react_on_rails:install
Note over Rails,Generator: Parent process has<br/>BUNDLE_GEMFILE set
Rails->>Generator: run_generators()
Generator->>Generator: ensure_shakapacker_installed()
alt Shakapacker not in Gemfile
Generator->>Bundler: with_unbundled_env
Bundler->>Bundler: Clear BUNDLE_GEMFILE<br/>and other env vars
Bundler->>System: system("bundle add shakapacker --strict")
Note over System: Discovers Gemfile in<br/>current directory,<br/>NOT parent's Gemfile
System-->>Bundler: Success
Bundler-->>Generator: Success
end
Generator->>Bundler: with_unbundled_env
Bundler->>Bundler: Clear BUNDLE_GEMFILE
Bundler->>System: system("bundle install")
System-->>Bundler: Success
Bundler-->>Generator: Success
Generator->>Bundler: with_unbundled_env
Bundler->>Bundler: Clear BUNDLE_GEMFILE
Bundler->>System: system("bundle exec rails shakapacker:install")
System-->>Bundler: Success
Bundler-->>Generator: Success
Generator-->>User: Installation complete
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails/lib/generators/react_on_rails/install_generator.rb (1)
220-225: Verify that Gemfile checks work correctly when parent setsBUNDLE_GEMFILE.The methods
shakapacker_in_gemfile_text?(line 234) andshakapacker_in_lockfile?(line 220) readENV["BUNDLE_GEMFILE"]to locate the Gemfile. WhenBUNDLE_GEMFILEis set to an absolute path pointing outside the target app (e.g.,/parent/Gemfile), these methods will incorrectly check the parent's files instead of the target app's:
- If shakapacker exists in parent's Gemfile, these checks return true
ensure_shakapacker_in_gemfilereturns early without adding shakapacker to the target app's Gemfile- Later bundler commands may fail because shakapacker is missing from the target app
This is an edge case (requires
BUNDLE_GEMFILEto point elsewhere), but possible in CI environments or monorepos. Consider using explicit paths"./Gemfile"and"./Gemfile.lock"in generator context, since generators should always operate on the target application's files.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/lib/generators/react_on_rails/install_generator.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
🧠 Learnings (11)
📓 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-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 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:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to lib/react_on_rails/engine.rb : In Rails Engine code, DO NOT use `rake_tasks` block to explicitly load rake files from `lib/tasks/` - Rails::Engine automatically loads them. Only use `rake_tasks` block if tasks are in non-standard location, need programmatic generation, or need to pass context
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: In IDE configuration, exclude these directories to prevent slowdowns: /coverage, /tmp, /gen-examples, /packages/react-on-rails/lib, /node_modules, /react_on_rails/spec/dummy/node_modules, /react_on_rails/spec/dummy/tmp, /react_on_rails/spec/dummy/app/assets/webpack, /react_on_rails/spec/dummy/log, /react_on_rails/spec/dummy/e2e/playwright-report, /react_on_rails/spec/dummy/test-results
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 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:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
⏰ 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: rspec-package-tests (3.4, latest)
- GitHub Check: Greptile Review
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails/lib/generators/react_on_rails/install_generator.rb (2)
266-266: LGTM! Correctly prevents BUNDLE_GEMFILE inheritance.The
Bundler.with_unbundled_envwrapper ensures the child process discovers the correct Gemfile in the target app directory rather than inheriting the parent process'sBUNDLE_GEMFILE. This prevents "injected gems" conflicts as described in issue #2287.
277-277: LGTM! Both bundler commands properly isolated.Both
bundle installandbundle exec rails shakapacker:installare correctly wrapped withBundler.with_unbundled_env, ensuring they operate on the target app's Gemfile context rather than inheriting the parent's bundler environment.Also applies to: 284-284
Code Review - PR #2288SummaryThis PR fixes a critical bug where the install generator inherits BUNDLE_GEMFILE from the parent process, causing 'injected gems' conflicts. The fix wraps bundler commands with Bundler.with_unbundled_env to isolate the generator bundler context. ✅ Strengths1. Correct Root Cause AnalysisThe issue is well-diagnosed: system() calls inherit environment variables including BUNDLE_GEMFILE. Wrapping with Bundler.with_unbundled_env is the canonical solution per Bundler documentation. 2. Consistent with Existing PatternsThe implementation follows established patterns in process_manager.rb, pack_generator.rb, and task_helpers.rb. 3. Good Backward CompatibilityThe with_unbundled_context helper supports both modern (with_unbundled_env) and legacy (with_clean_env) Bundler APIs with graceful fallback. 4. DRY PrincipleCreating the helper avoids repeating API version checking logic three times. 5. Rake Task FixThe change from sh_in_dir to unbundled_sh_in_dir in shakapacker_examples.rake:122 is correct - example generation should use the example app's Gemfile. 🔍 Observations & Suggestions1. Minor Code DuplicationThe with_unbundled_context helper is duplicated in install_generator.rb and pack_generator.rb with slight differences. Consider extracting to a shared module (non-blocking). 2. Test Coverage GapThe existing install_generator_spec.rb does not test bundler environment isolation. Consider adding tests to verify BUNDLE_GEMFILE is not inherited (non-blocking - the fix addresses a real CI failure). 3. DocumentationThe comment in shakapacker_examples.rake:120-121 is excellent. Similar comments in install_generator.rb would help future maintainers. 🚀 Performance & Security
🎯 Final Verdict: Approve with minor suggestions ✅This PR:
Suggested follow-ups (non-blocking):
Great work fixing this issue! 🎉 |
Code Review: PR #2288 - Fix generator inheriting BUNDLE_GEMFILE from parent process✅ Overall AssessmentExcellent fix! This PR correctly addresses a real issue with Bundler environment inheritance and implements the solution consistently across the codebase. 🎯 Strengths
|
Code Review for PR #2288: Fix generator inheriting BUNDLE_GEMFILE from parent process✅ SummaryThis PR correctly addresses issue #2287 by wrapping bundler commands in 🎯 Code Quality & Best PracticesStrengths:
Minor observation:
🐛 Potential IssuesNo critical issues found. The implementation is solid. Minor edge case (already handled):
⚡ Performance Considerations
🔒 Security ConcernsNo security issues identified.
🧪 Test CoverageExcellent test coverage:
Suggestion for additional testing: 📝 Additional ObservationsRake file changes:
Documentation:
🎉 VerdictLGTM - Excellent work! This PR demonstrates:
The fix is minimal, focused, and follows Ruby/Rails best practices. No blocking issues found. 📋 Pre-merge ChecklistBefore merging, ensure:
Note: According to CLAUDE.md, you should run |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails/lib/generators/react_on_rails/install_generator.rb (1)
262-272: Add explicitrequire "bundler"at the top of the file.The
ensure_shakapacker_in_gemfilemethod (lines 262-272) andinstall_shakapackermethod (lines 274-291) both useBundler.with_unbundled_envwithout a file-levelrequire "bundler". While this works underbundle exec, it creates brittleness for other invocation contexts. Therequire "bundler"at line 228 is scoped insideshakapacker_in_bundler_specs?and doesn't guarantee availability.Proposed fix
require "rails/generators" require "json" +require "bundler" require_relative "generator_helper" require_relative "generator_messages" require_relative "js_dependency_manager"Also consider using
system("bundle", "add", "shakapacker", "--strict")with array arguments instead of string interpolation to avoid shell invocation semantics and align with patterns elsewhere in the codebase.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1)
333-424: Solid regression coverage for #2287 (with_unbundled_env + local Gemfile/Gemfile.lock checks).
One maintainability tweak: dedupe the repeatedBUNDLE_GEMFILEsave/restore using anaroundblock (or helper) and restore viaensurefor stricter isolation. Based on learnings, stubbing/restoring env vars via hooks helps avoid test cross-talk.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/install_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rb: Runbundle exec rubocopand fix ALL violations before every commit/push
Runbundle exec rubocop(MANDATORY) before every commit to ensure zero offenses
Files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rb
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /{CHANGELOG.md,CHANGELOG_PRO.md} : Use format `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` in changelog entries (no hash in PR number)
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/*.rb : Run `bundle exec rubocop` and fix ALL violations before every commit/push
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to **/*.rb : Run `bundle exec rubocop` (MANDATORY) before every commit to ensure zero offenses
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-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to sig/react_on_rails/**/*.rbs : RBS signatures in `sig/react_on_rails/` should be added for new Ruby files in `lib/react_on_rails/`, included in Steepfile, validated with `bundle exec rake rbs:validate`, and type-checked with `bundle exec rake rbs:steep`
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.spec.js : Use Playwright E2E tests in `react_on_rails/spec/dummy/e2e/playwright/` for React component integration testing. Tests automatically start Rails server on port 5017 before running
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom Rails app commands in `e2e/playwright/app_commands/` directory with .rb files for reusable helpers in Playwright tests
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: For infrastructure/config changes, perform comprehensive local testing including: finding all affected files with grep, testing build pipeline, running relevant specs, and linting everything before pushing
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: In IDE configuration, exclude these directories to prevent slowdowns: /coverage, /tmp, /gen-examples, /packages/react-on-rails/lib, /node_modules, /react_on_rails/spec/dummy/node_modules, /react_on_rails/spec/dummy/tmp, /react_on_rails/spec/dummy/app/assets/webpack, /react_on_rails/spec/dummy/log, /react_on_rails/spec/dummy/e2e/playwright-report, /react_on_rails/spec/dummy/test-results
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 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:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/lib/generators/react_on_rails/install_generator.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:
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for user-visible changes in the Pro-only react_on_rails_pro gem and npm packages
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 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:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes in the open-source react_on_rails gem and npm package (features, bug fixes, breaking changes, deprecations, performance improvements)
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 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:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to {package.json,webpack.config.js,packages/*/package.json,react_on_rails_pro/package.json} : When resolving merge conflicts in build configuration files, verify file paths are correct by running `grep -r 'old/path' .` and test affected scripts like `pnpm run prepack` before continuing the merge
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Applies to lib/react_on_rails/engine.rb : In Rails Engine code, DO NOT use `rake_tasks` block to explicitly load rake files from `lib/tasks/` - Rails::Engine automatically loads them. Only use `rake_tasks` block if tasks are in non-standard location, need programmatic generation, or need to pass context
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
📚 Learning: 2025-12-19T18:57:59.314Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:57:59.314Z
Learning: Shakapacker configuration should be tested by creating debug scripts in dummy app root (debug-webpack-rules.js, debug-webpack-with-config.js) to inspect webpack rules before committing configuration changes
Applied to files:
react_on_rails/lib/generators/react_on_rails/install_generator.rb
⏰ 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). (7)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails/lib/generators/react_on_rails/install_generator.rb (2)
220-225: Lockfile detection change is aligned with the goal; regex is OK for Bundler lockfile “specs” lines.
This now correctly ignores inheritedBUNDLE_GEMFILEand only looks at the target app’sGemfile.lock.
234-239: Gemfile detection change is aligned with the goal; avoids inheritedBUNDLE_GEMFILE.
This is a clearer/safer check than relying on env-derived paths.
Code Review - PR #2288✅ Overall AssessmentThis is an excellent fix for a legitimate bug. The implementation is clean, well-tested, and follows existing patterns in the codebase. I recommend approval with one minor suggestion for consistency. 🎯 Strengths
💡 Suggestions1. Consider Backward Compatibility Helper (Low Priority)The existing code in def with_unbundled_context(&block)
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
endAnalysis:
Recommendation: The direct call to
This is not blocking since the current approach works correctly for all supported versions. 🔍 Code Quality NotesPositive:
Edge Cases Handled:
🧪 TestingTest Quality: Excellent The tests cover:
Suggestion for future enhancement (not blocking): 🚀 Performance & Security
📋 Checklist for Merge
🎬 RecommendationApprove and merge once CI passes. The optional consistency suggestion can be addressed in a follow-up refactor if desired, but this PR is production-ready as-is. Great work on the thorough investigation and clean implementation! 🎉 Files reviewed: |
Temporary duplicate of fix from PR #2288. Use unbundled context for shakapacker:install and generator commands to prevent inheriting parent workspace's Gemfile.lock (which has shakapacker 9.4.0). Remove this commit after PR #2288 is merged to master. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Temporary duplicate of fix from PR #2288. Use unbundled context for shakapacker:install and generator commands to prevent inheriting parent workspace's Gemfile.lock (which has shakapacker 9.4.0). Remove this commit after PR #2288 is merged to master. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review - PR #2288SummaryThis PR fixes a critical bug where the react_on_rails:install generator inherited BUNDLE_GEMFILE from the parent process, causing "injected gems" conflicts. The fix properly isolates bundler commands using Bundler.with_unbundled_env. ✅ Strengths1. Correct Root Cause IdentificationThe PR correctly identifies that system() calls inherit the parent process's environment, including BUNDLE_GEMFILE. This is a well-documented Bundler issue with established solutions. 2. Consistent with Existing PatternsThe fix follows patterns already used in the codebase:
3. Comprehensive FixThe PR addresses all instances of the issue:
4. Excellent Test CoverageThe regression tests are thorough (92 new lines):
5. Good Documentation
🔍 Observations & Minor Suggestions1. Simplified Approach (Good Decision)Commit 4 simplified from a helper method with legacy fallback to direct Bundler.with_unbundled_env usage. This is correct because:
Suggestion: Consider adding a comment mentioning the minimum Ruby/Bundler version to prevent future additions of unnecessary fallback code. 2. Test ImplementationThe tests manually restore ENV["BUNDLE_GEMFILE"] after modification. This is good hygiene and works correctly. An around block could be more RSpec-idiomatic, but this is purely stylistic. 3. Rake File ChangesThe changes to shakapacker_examples.rake are essential (lines 120, 132: sh_in_dir → unbundled_sh_in_dir). This ensures example apps use their own gem versions, not the parent workspace's. Question: Should the comments be more specific about preventing cached version conflicts to help future maintainers understand the CI failures this prevents? 🔒 Security Considerations✅ No security concerns. The changes use official Bundler API and actually improve security by ensuring commands run in the correct context. ⚡ Performance Considerations✅ Negligible performance impact. Bundler.with_unbundled_env just temporarily clears environment variables - no measurable overhead. 🐛 Potential IssuesNone identified. The fix is targeted, well-tested, and consistent with existing patterns. 📋 Pre-Merge ChecklistBased on CLAUDE.md requirements:
🎯 RecommendationAPPROVE with minor suggestions This is a high-quality fix that:
The minor suggestions above are optional improvements - the PR is ready to merge as-is once CI passes. 📚 References
Great work identifying and fixing this subtle but important bug! 🎉 |
Code Review: Fix generator inheriting BUNDLE_GEMFILE from parent process✅ Overall AssessmentExcellent work! This is a well-researched, properly documented fix for a legitimate bug. The PR follows best practices and aligns with existing patterns in the codebase. 🎯 Strengths
📝 Code QualityGenerator Changes (
Rake Task Changes (
Test Coverage:
🔍 Minor Observations
🔒 Security Considerations✅ No security concerns. The fix actually improves security by ensuring bundler operates on the intended Gemfile rather than an inherited one. ⚡ Performance Considerations✅ Negligible performance impact. 🧪 Test Coverage SuggestionsThe test coverage is already comprehensive. One optional enhancement: Optional: Add an integration test that runs the actual generator in a bundled context to verify end-to-end behavior. However, the current unit tests are sufficient for regression prevention. 📋 Recommendations
🎓 Final ThoughtsThis PR demonstrates excellent engineering practices:
Great work on tracking down this subtle environment inheritance issue! 🚀 Recommendation: APPROVE with minor suggestion to verify CI failure |
Temporary duplicate of fix from PR #2288. Use unbundled context for shakapacker:install and generator commands to prevent inheriting parent workspace's Gemfile.lock (which has shakapacker 9.4.0). Remove this commit after PR #2288 is merged to master. Co-Authored-By: Claude Opus 4.5 <[email protected]>
justin808
added
ready-to-merge
Wrap bundler commands in install_generator.rb with Bundler.with_unbundled_env to prevent BUNDLE_GEMFILE inheritance from the parent process. This fixes "injected gems" conflicts when running `bundle exec rails g react_on_rails:install`. The issue occurs because system() calls inherit the parent's BUNDLE_GEMFILE environment variable, causing bundler to use the wrong Gemfile when the generator runs bundle commands. This is a well-documented Bundler issue (rubygems/bundler#698, rails/rails#3153) and the fix follows the official solution recommended by Bundler maintainers since 2012. Fixes #2287 Co-Authored-By: Claude Opus 4.5 <[email protected]>
Change sh_in_dir to unbundled_sh_in_dir for rake shakapacker:install to ensure the generated example app uses its own Gemfile and gem versions, not the parent workspace's bundle context. This fixes CI failures caused by version mismatch when the parent workspace has a cached older version of shakapacker (e.g., 9.4.0) while the example app installs a newer version (e.g., 9.5.0). The fix follows the same pattern already used in this file for shakapacker:binstubs and react_on_rails:generate_packs commands. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract a helper method that supports both new (with_unbundled_env) and legacy (with_clean_env) Bundler APIs for consistency with the pattern used in process_manager.rb and pack_generator.rb. This ensures compatibility with older Bundler versions that don't have the with_unbundled_env method. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove with_unbundled_context helper method with legacy Bundler fallback - Use Bundler.with_unbundled_env directly (available in Bundler 2.1.0+, Ruby 3.0+ ships with Bundler 2.2.3+, so fallback is dead code) - Add regression tests verifying BUNDLE_GEMFILE isolation (#2287) - Matches the direct usage pattern in task_helpers.rb Co-Authored-By: Claude Opus 4.5 <[email protected]>
The shakapacker_in_gemfile_text? and shakapacker_in_lockfile? methods were using ENV["BUNDLE_GEMFILE"] to determine which Gemfile to check. This caused incorrect detection when BUNDLE_GEMFILE was inherited from a parent process (same root cause as the subprocess fix). Now these methods always check the local "Gemfile" and "Gemfile.lock", which is correct since generators always operate on the target Rails app. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The generator_commands (rails generate react_on_rails:install) were still using sh_in_dir which inherits the parent's bundler context. This caused version mismatches when the parent workspace had a different shakapacker version cached than the newly generated example app. Changed to unbundled_sh_in_dir to match the pattern used for shakapacker:install and generate_packs. Co-Authored-By: Claude Opus 4.5 <[email protected]>
justin808
force-pushed
the
ihabadham/fix/generator-bundle-gemfile-inheritance
branch
from
b15053d to
9b34f12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb`:
- Around line 527-542: The tests manually save and restore ENV["BUNDLE_GEMFILE"]
inside each example which is not exception-safe; extract that logic into an
RSpec hook (use around or a before/after pair) that saves original_gemfile, sets
ENV["BUNDLE_GEMFILE"] = "/fake/parent/Gemfile" for the example, yields to the
example, and then restores or deletes ENV["BUNDLE_GEMFILE"] afterwards; remove
the inline save/restore and existing manual cleanup blocks around the
Bundler.with_unbundled_env checks (references: ENV["BUNDLE_GEMFILE"],
original_gemfile, and Bundler.with_unbundled_env) and apply the same hook to the
other two affected examples to avoid duplication and ensure exception-safe
cleanup.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1)
525-543: This test validates Bundler's own behavior, not your code.
Bundler.with_unbundled_envis a well-tested stdlib method. This test adds maintenance cost without exercising any project code. Consider removing it or replacing it with a test that verifies your generator method actually invokeswith_unbundled_env(like the tests above already do).
| original_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | ||
| ENV["BUNDLE_GEMFILE"] = "/fake/parent/Gemfile" | ||
|
|
||
| bundler_env_in_block = nil | ||
| Bundler.with_unbundled_env do | ||
| bundler_env_in_block = ENV.fetch("BUNDLE_GEMFILE", nil) | ||
| end | ||
|
|
||
| expect(bundler_env_in_block).to be_nil | ||
|
|
||
| # Restore original value | ||
| if original_gemfile | ||
| ENV["BUNDLE_GEMFILE"] = original_gemfile | ||
| else | ||
| ENV.delete("BUNDLE_GEMFILE") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ENV restoration is not exception-safe — use around or before/after hooks.
If an expectation fails before the manual restore block, BUNDLE_GEMFILE stays polluted for subsequent tests. Extract the save/restore into an around hook (or before/after pair) so cleanup is guaranteed. This also removes significant duplication across the three tests.
♻️ Suggested refactor
describe "bundler environment isolation" do
let(:install_generator) { described_class.new }
+
+ around(:each) do |example|
+ original_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil)
+ example.run
+ ensure
+ if original_gemfile
+ ENV["BUNDLE_GEMFILE"] = original_gemfile
+ else
+ ENV.delete("BUNDLE_GEMFILE")
+ end
+ end
# ... (first two tests unchanged) ...
- it "Bundler.with_unbundled_env clears BUNDLE_GEMFILE in block" do
- # Set a fake BUNDLE_GEMFILE to simulate parent process context
- original_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil)
+ it "Bundler.with_unbundled_env clears BUNDLE_GEMFILE in block" do
ENV["BUNDLE_GEMFILE"] = "/fake/parent/Gemfile"
bundler_env_in_block = nil
Bundler.with_unbundled_env do
bundler_env_in_block = ENV.fetch("BUNDLE_GEMFILE", nil)
end
expect(bundler_env_in_block).to be_nil
-
- # Restore original value
- if original_gemfile
- ENV["BUNDLE_GEMFILE"] = original_gemfile
- else
- ENV.delete("BUNDLE_GEMFILE")
- end
endApply the same cleanup removal to the other two tests.
Based on learnings: "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."
Also applies to: 547-566, 570-588
🤖 Prompt for AI Agents
In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb`
around lines 527 - 542, The tests manually save and restore
ENV["BUNDLE_GEMFILE"] inside each example which is not exception-safe; extract
that logic into an RSpec hook (use around or a before/after pair) that saves
original_gemfile, sets ENV["BUNDLE_GEMFILE"] = "/fake/parent/Gemfile" for the
example, yields to the example, and then restores or deletes
ENV["BUNDLE_GEMFILE"] afterwards; remove the inline save/restore and existing
manual cleanup blocks around the Bundler.with_unbundled_env checks (references:
ENV["BUNDLE_GEMFILE"], original_gemfile, and Bundler.with_unbundled_env) and
apply the same hook to the other two affected examples to avoid duplication and
ensure exception-safe cleanup.
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 1m 48s —— View job Code Review for PR #2288Reviewing: Fix generator inheriting BUNDLE_GEMFILE from parent process SummaryThis PR correctly fixes a real issue where the React on Rails install generator inherits ✅ Strengths1. Correct Problem DiagnosisThe root cause is accurately identified: 2. Proper SolutionUsing 3. Consistent with Existing PatternsThe fix mirrors patterns already used in this codebase:
4. Comprehensive CoverageThe fix addresses all three critical locations:
Plus the rake tasks:
5. Excellent Test CoverageAdded 92 lines of regression tests covering:
6. Good Documentation
🔍 Issues & Suggestions1. Test Environment Cleanup Not Exception-Safe
|
Greptile OverviewGreptile SummaryThis PR hardens the Main issue to address before merge: Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer Shell
participant Rails as rails g react_on_rails:install
participant Gen as InstallGenerator
participant Bundler as Bundler.with_unbundled_env
participant Sys as system("bundle …")
participant FS as Gemfile/Gemfile.lock
Dev->>Rails: bundle exec rails g react_on_rails:install
Rails->>Gen: run_generators
Gen->>FS: shakapacker_in_gemfile_text?("shakapacker")
FS-->>Gen: false (not present)
Gen->>Bundler: with_unbundled_env { … }
Bundler->>Sys: system("bundle add shakapacker --strict")
Sys-->>Bundler: exit status
Bundler-->>Gen: success?
Gen->>Bundler: with_unbundled_env { … }
Bundler->>Sys: system("bundle install")
Sys-->>Bundler: exit status
Bundler-->>Gen: success?
Gen->>Bundler: with_unbundled_env { … }
Bundler->>Sys: system("bundle exec rails shakapacker:install")
Sys-->>Bundler: exit status
Bundler-->>Gen: success?
Note over Bundler: Clears inherited BUNDLE_GEMFILE
Note over Gen,FS: Always checks local Gemfile/Gemfile.lock
|
| it "clears BUNDLE_GEMFILE when running bundle install" do | ||
| # Mock the system calls to prevent actual execution | ||
| allow(install_generator).to receive(:system).and_return(true) | ||
|
|
||
| # Expect with_unbundled_env to be called for both bundle install and shakapacker:install | ||
| expect(Bundler).to receive(:with_unbundled_env).twice.and_yield | ||
|
|
||
| install_generator.send(:install_shakapacker) |
There was a problem hiding this comment.
Test assumes exact call count
install_shakapacker now wraps system("bundle install") and system("bundle exec rails shakapacker:install") in Bundler.with_unbundled_env, but this spec asserts expect(Bundler).to receive(:with_unbundled_env).twice.and_yield. If any future change adds another with_unbundled_env call inside this method (or a nested call via a helper), this will fail while behavior is still correct. Consider asserting that with_unbundled_env is used for the specific commands (e.g., by expecting the wrapped system invocations) rather than an exact .twice count.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Extract manual BUNDLE_GEMFILE save/restore into an around hook so cleanup is guaranteed even if expectations fail. Removes duplication across the three bundler isolation tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Replace exact .twice call count with have_received assertions that verify the specific commands are called, making the test resilient to future refactors (addresses Greptile review comment) - Add CHANGELOG entry for this user-visible bug fix Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 1m 49s —— View job Code Review - PR #2288Reviewing: Fix generator inheriting BUNDLE_GEMFILE from parent process SummaryThis PR correctly fixes a legitimate bug where the Recommendation: APPROVE ✅ (with one major issue to address in tests) ✅ Strengths1. Correct Problem DiagnosisThe root cause is accurately identified: 2. Proper SolutionUsing 3. Consistent with Existing PatternsThe fix mirrors patterns already used in this codebase:
4. Comprehensive CoverageThe fix addresses all three critical locations in install_generator.rb:
Plus the rake tasks in shakapacker_examples.rake:
5. Excellent Test CoverageAdded 82 lines of regression tests (lines 503-580) covering:
6. Good Documentation
🔍 Issues & Suggestions1. Test Environment Cleanup Not Exception-Safe
|
Additional Comments (1)
|
The install generator uses Bundler.with_unbundled_env in three places but relied on Rails boot process having already loaded Bundler. Add an explicit require at the top of the file to make the dependency clear and prevent a potential NameError in non-standard invocation contexts. Co-Authored-By: Claude Opus 4.6 <[email protected]>
justin808
deleted the
ihabadham/fix/generator-bundle-gemfile-inheritance
branch
Same fix as PR #2288 applied to shakapacker_in_lockfile? — always check the target app's Gemfile.lock directly instead of deriving the path from ENV["BUNDLE_GEMFILE"], which can point to a parent project's Gemfile when running generators via bundle exec. See: #2287 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Same fix as PR #2288 applied to shakapacker_in_lockfile? — always check the target app's Gemfile.lock directly instead of deriving the path from ENV["BUNDLE_GEMFILE"], which can point to a parent project's Gemfile when running generators via bundle exec. See: #2287 Co-Authored-By: Claude Opus 4.6 <[email protected]>
2 participants
Summary
install_generator.rbwithBundler.with_unbundled_envto preventBUNDLE_GEMFILEinheritance from the parent processbundle exec rails g react_on_rails:installProblem
When executing
bundle exec rails g react_on_rails:install, the generator's internal bundler commands inherit the parent process'sBUNDLE_GEMFILEenvironment variable, causing gem version conflicts with the error:Root Cause
Three
system()calls ininstall_generator.rbexecute bundler commands without isolating the bundler environment:system("bundle add shakapacker --strict")system("bundle install")system("bundle exec rails shakapacker:install")Solution
Wrap bundler commands using
Bundler.with_unbundled_env:This is the official solution documented by:
The pattern already exists in this codebase in
process_manager.rbandpack_generator.rb.Test plan
BUNDLE_GEMFILEin child processesFixes #2287
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests