fix: Auto-remove default gem files to prevent installer conflicts #1848
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (1)
WalkthroughAdds a pre-install cleanup to the React on Rails install generator that, when Shakapacker is detected in the app, conditionally removes default Rails Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Gen as InstallGenerator
participant FS as Filesystem
participant Gem as Shakapacker Gem
Dev->>Gen: rails g react_on_rails:install
activate Gen
Note over Gen: Pre-install cleanup (conditional)
Gen->>Gen: if shakapacker_configured? then remove_default_gem_files()
par Remove default bin/dev
Gen->>FS: check `bin/dev` (exists & writable?)
alt matches Rails default
Gen->>FS: delete `bin/dev`
Note right of FS: prints removal notice
else
Note right of Gen: keep `bin/dev`
end
and Remove default shakapacker.yml
Gen->>FS: check `config/shakapacker.yml` (exists & writable?)
Gen->>Gem: read gem's default `config/shakapacker.yml`
alt contents match (trimmed)
Gen->>FS: delete `config/shakapacker.yml`
Note right of FS: prints removal notice
else
Note right of Gen: keep file
end
opt read/compare error
Note over Gen: rescue error and skip deletion
end
end
Note over Gen: Continue with remaining generator steps
Gen->>Gen: run_generators / messaging
deactivate Gen
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 ReviewI've reviewed this PR and have the following feedback: ✅ Strengths
🔍 Issues & Suggestions1. Missing Test Coverage (Critical)The PR adds significant new functionality but doesn't include any tests. Based on the test plan in the description, you should add: # spec/react_on_rails/generators/install_generator_spec.rb
context "removing default gem files" do
it "removes default bin/dev when it matches Rails default" do
# Test auto-removal of default bin/dev
end
it "preserves customized bin/dev" do
# Test that customized files are kept
end
it "removes default shakapacker.yml when it matches gem default" do
# Test auto-removal of default shakapacker.yml
end
it "preserves customized shakapacker.yml" do
# Test that customized files are kept
end
end2. Hardcoded Default Comparison (Medium)
The default Option A: Read from Rails gem (similar to shakapacker approach) def rails_default_bin_dev
rails_gem_path = Gem.loaded_specs["railties"]&.full_gem_path
return nil unless rails_gem_path
template_path = File.join(rails_gem_path, "lib/rails/generators/rails/app/templates/bin/dev.tt")
File.read(template_path).strip if File.exist?(template_path)
endOption B: Document the Rails version assumption and add a comment explaining this matches Rails 7.1+ defaults 3. File Comparison Could Be More Robust (Low)
Currently uses strict string equality after strip. Consider:
def normalize_content(content)
content.gsub(/\r\n/, "\n").strip
end
# Then use:
normalize_content(current_content) == normalize_content(default_content)4. Logging Verbosity Check (Low)
The verbose check puts Rainbow("🗑️ ...").yellow if options.verbose?5. Method Placement (Minor)The new methods are placed between # File cleanup utilities
def remove_default_gem_files
# ...
end
private
def remove_default_bin_dev
# ...
end
def remove_default_shakapacker_yml
# ...
end
def shakapacker_yml_matches_default?(config_path)
# ...
end🔒 Security Considerations✅ No security issues identified:
⚡ Performance Considerations✅ Performance impact is minimal:
📋 RecommendationsBefore Merging:
Nice to Have:
📝 Overall AssessmentVerdict: Approve with changes required This is a solid solution to a real UX problem. The implementation is safe and well-thought-out. However, test coverage is mandatory before merging to prevent regressions. The hardcoded bin/dev default is a minor concern that should at minimum be documented. Great work on the defensive approach (only removing exact matches) and user communication! 👍 🤖 Generated with Claude Code |
Code Review - PR #1848I've reviewed the changes for auto-removing default gem files. This is a well-implemented solution that addresses installation friction. Here are my findings: ✅ Strengths
🐛 Potential Issues1. File Permissions Not Checked (Medium Priority) No permission check before File.delete(bin_dev_path). Could crash on permission errors. Recommendation: Add rescue block for Errno::EACCES 2. Hardcoded Default Content (Low-Medium Priority) Rails default bin/dev is hardcoded. May become stale with Rails updates. Recommendation: Fetch dynamically or document Rails version 3. Inconsistent Verbose Logging (Low Priority) 🧪 Test Coverage GapsNo automated tests for new functionality. Recommend adding to install_generator_spec.rb:
🎯 Verdict✅ Approve with minor improvements Main concerns:
The approach is sound and safety checks are appropriate. Great work solving this installation pain point! 🚀 |
Code Review - PR #1848This PR provides a solid solution to prevent installer conflicts by automatically removing default gem files. The implementation follows defensive programming practices with safety checks to prevent accidental deletion of customized files. Critical Bug FoundMissing verbose option at line 113: The code references options.verbose? but no class_option :verbose exists in the generator. This will cause a NoMethodError at runtime when the rescue block executes. Fix: Either add the verbose class_option or remove the conditional check. RecommendationsMust Fix:
Should Add: Nice to Have: Summary
Status: Approve with required fixes (verbose option bug + test coverage) Generated with Claude Code |
Code Review - PR #1848Critical Issue: Missing verbose optionLine 113 references options.verbose? but no class_option :verbose exists. This will cause NoMethodError at runtime. Fix: Remove the conditional or add the verbose class_option. Required: Add Test CoverageNo tests exist for the new file removal functionality. Tests should cover:
Recommendations:
Verdict:Changes required before merge. The approach is sound but the verbose bug must be fixed and tests added. Generated with Claude Code |
Code Review: Auto-remove default gem files to prevent installer conflicts✅ Positive Aspects
🔍 Issues & Concerns1. Critical: Missing Test CoverageThe PR adds significant new functionality but no tests. The existing install_generator_spec.rb should include tests for:
Recommendation: Add comprehensive test coverage before merging. 2. Timing/Ordering IssueThe method is called in run_generators BEFORE invoke_generators. However, Shakapacker might be installed DURING invoke_generators (line 148). This means:
Critical Fix: Split the logic to run remove_default_shakapacker_yml AFTER ensure_shakapacker_installed is called 3. Hardcoded Default ComparisonThe remove_default_bin_dev method hardcodes the Rails default bin/dev content. This is brittle - if Rails changes this default in future versions, the comparison will fail. Better approach: Store the default in a constant with documentation about which Rails version it's from 4. Potential Platform-Specific IssuesFile comparison using == on file contents might have issues with:
Fix: Normalize file contents before comparison 🎯 Performance & Security
📋 Must Fix Before Merge
Overall, this is a valuable improvement that solves a real problem. With these fixes, it will be a solid addition. 🤖 Review generated with Claude Code |
Code ReviewOverall AssessmentThis is a well-thought-out solution to a real UX problem. The code quality is good, and the approach is safe and conservative. I have some suggestions for improvements below. Strengths
Code Quality Issues1. Missing test coverage - HIGH PRIORITYThe new methods (remove_default_gem_files, remove_default_bin_dev, remove_default_shakapacker_yml, shakapacker_yml_matches_default?) have zero test coverage. This is a significant gap. Recommendation: Add tests in spec/react_on_rails/generators/install_generator_spec.rb for all scenarios:
2. Potential security concern with file deletion - MEDIUMThe code deletes files without backup or confirmation. While the exact match check mitigates risk, there is still a chance of data loss if:
Recommendations:
3. Hard-coded default bin/dev content - MEDIUMlib/generators/react_on_rails/install_generator.rb:76-79 The default bin/dev content is hard-coded as a string. If Rails changes this default in future versions, this check will break. Recommendations:
4. Verbose mode check may never executelib/generators/react_on_rails/install_generator.rb:113 The error message only shows in verbose mode. Users may not see warnings about file verification failures unless they use --verbose. Recommendation: Always log errors (at minimum to stderr), even if not showing to user. 5. Missing edge case error handlingConsider these scenarios:
Recommendation: Wrap file deletion in error handling to prevent failures. Performance ConsiderationsMinor concern: Reading entire file contents for comparison could be slow for large files. Impact: Low - these files are typically small (<1KB). Not a blocker. Optimization (optional): Could use file checksums for faster comparison. Security AssessmentLow risk - The code:
Documentation Needs
Code StyleAppears to follow RuboCop rules - No obvious violations, but please verify by running: Per CLAUDE.md: "ALWAYS run bundle exec rubocop and fix ALL violations" Recommendations Priority
ConclusionRecommendation: Request changes for test coverage, then approve. The implementation solves a real problem and is generally well-written. However, test coverage is mandatory before merging to ensure:
Once tests are added and RuboCop passes, this is ready to merge! Generated with Claude Code |
Code Review - PR #1848Overall AssessmentThis PR successfully addresses the interactive conflict prompt issue during installation. The implementation is well-structured, safe, and follows Ruby best practices. I recommend approval with minor suggestions. Code Quality - Strengths
Potential Bugs & IssuesFile Permission Race Condition: String Comparison Edge Cases:
Security ConcernsThe code is generally secure with good path handling. Consider adding explicit validation that paths are within expected directories using File.realpath to prevent path traversal attacks. Test Coverage - CRITICAL GAPNo tests added for the new remove_default_gem_files functionality. Please add tests for:
Additional Recommendations
ConclusionRecommendation: Approve with request for test coverage The core implementation is solid. The suggested improvements are mostly defensive enhancements for edge cases. The critical gap is test coverage - please add tests before merging to ensure this behavior is protected against future regressions. Great work on identifying and solving this UX pain point! Review generated with Claude Code |
Code Review for PR #1848SummaryThis PR adds functionality to automatically remove default bin/dev and config/shakapacker.yml files before running the installer. Overall, this is a well-implemented solution that addresses a real pain point. Strengths
Code Quality IssuesMinor:
Testing Concerns - CRITICALThis PR adds 48 lines of code with NO TEST COVERAGE. Missing tests for:
Recommended test scenarios:
Security & Performance✅ No security concerns - proper path restrictions and validation RecommendationsMust Address:
Should Consider: Approval StatusGreat work identifying and solving this UX issue! |
|
Code Review: Auto-remove default gem files to prevent installer conflicts STRENGTHS:
CODE QUALITY ISSUES:
TEST COVERAGE CONCERNS: Major Issue: No tests added for the new functionality The PR adds significant new logic but no corresponding tests. You should add tests for:
SECURITY CONSIDERATIONS: Low risk - File deletion operations use File.join(destination_root, ...) which is good practice PERFORMANCE CONSIDERATIONS: Minor concern: Consider early exit if Shakapacker is not in Gemfile before checking file contents ADDITIONAL RECOMMENDATIONS:
SUMMARY: Approval Status: Conditional Approval The PR solves a real problem and the approach is sound, but it needs: MUST FIX:
SHOULD FIX:
The core logic is good, but without tests, this is too risky to merge. Generated with Claude Code |
Code Review - PR #1848I've reviewed the changes to auto-remove default gem files during installation. Overall, this is a well-thought-out solution to a real UX problem. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues & Suggestions1. File Deletion Timing Issue (lib/generators/react_on_rails/install_generator.rb:41)The remove_default_gem_files is called before invoke_generators, but looking at line 91 in base_generator.rb, shakapacker.yml gets copied during generator invocation. This means:
Recommendation: Add a comment explaining this timing is intentional, or consider if the removal should happen at a different point. 2. Security: File Deletion Without Backup (lib/generators/react_on_rails/install_generator.rb:88, 97)While the comparison logic is safe, deleting files without creating backups could be risky if:
Recommendation: Consider creating backup files before deletion or at minimum log what was removed. 3. Edge Case: File Encoding Issues (lib/generators/react_on_rails/install_generator.rb:102-103)File comparison using == is sensitive to:
Recommendation: Normalize content before comparison to handle cross-platform differences. 4. Hard-coded Default Content (lib/generators/react_on_rails/install_generator.rb:76-79)The default bin/dev content is hard-coded as a string. If Rails changes this default, the comparison will fail and the file won't be removed. Recommendation: Document where this default comes from (Rails version?) and consider extracting to a constant with a comment about maintenance. 5. Missing Test Coverage
|
Code Review for PR #1848Thank you for this improvement! The automatic removal of default gem files is a great UX enhancement. Here's my detailed review: ✅ Strengths
🔍 Code Quality & Best PracticesExcellent:
Suggestions:
🐛 Potential IssuesMinor:
🚀 Performance Considerations
🔒 Security ConcernsGood:
Recommendation:
🧪 Test CoverageMissing Tests:
Example test structure: context "with default bin/dev file" do
before(:all) do
# Create default bin/dev
run_generator_test_with_args(%w[], package_json: true)
end
it "removes default bin/dev before installation" do
# Verify bin/dev was replaced with React on Rails version
end
end📋 Additional Recommendations
🎯 SummaryOverall Assessment: ✅ Approve with Minor Suggestions The implementation is solid and solves a real problem. The main gap is test coverage. I recommend:
The code follows the project's conventions (per CLAUDE.md), and the solution is pragmatic and safe. Code Quality: ⭐⭐⭐⭐ (4/5) 🤖 Review generated with Claude Code |
Code Review - PR #1848Thanks for this PR! The implementation effectively solves the interactive prompt problem during installation. Here's my comprehensive review: ✅ Strengths
🔍 Code Quality ObservationsGood Practices:
Potential Issues:
🧪 Test Coverage ConcernsCritical: No tests were added for the new functionality. The PR should include specs for: context "removing default gem files" do
it "removes default bin/dev when it matches Rails default"
it "preserves customized bin/dev"
it "removes default shakapacker.yml when it matches gem default"
it "preserves customized shakapacker.yml"
it "handles missing shakapacker gem gracefully"
it "handles file permission errors gracefully"
endAdd these to 🔒 Security ConsiderationsLow Risk: The implementation is secure because:
⚡ Performance ConsiderationsNo concerns: File operations are minimal and only occur once during installation. 📝 Suggestions for Improvement
📋 Checklist Before Merge
🎯 Overall AssessmentRecommendation: Request changes for test coverage The implementation is sound and solves a real problem, but needs test coverage before merging. The code quality is good, following the project's conventions. Once tests are added and the minor suggestions addressed, this will be ready to merge. Priority fixes:
Great work on identifying and solving this installation UX issue! 🚀 Review generated with Claude Code |
justin808
force-pushed
the
update-generator-no-conflict-default-shakapacker-yml
branch
from
ac5c8e4 to
a9b33a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/install_generator.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
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). (10)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build-and-test
| # Rails 7+ default bin/dev content | ||
| # If Rails changes this default, this comparison will safely fail and preserve the file | ||
| default_bin_dev = <<~RUBY.strip | ||
| #!/usr/bin/env ruby | ||
| exec "./bin/rails", "server", *ARGV | ||
| RUBY | ||
|
|
||
| current_content = File.read(bin_dev_path).strip | ||
| return unless current_content == default_bin_dev | ||
|
|
||
| puts Rainbow("🗑️ Removing default bin/dev file to avoid conflicts...").yellow | ||
| FileUtils.rm_f(bin_dev_path) | ||
| end |
There was a problem hiding this comment.
Fix bin/dev default comparison
Rails’ generated bin/dev is a shell script (from railties/lib/rails/generators/rails/app/templates/bin/dev.tt), not the Ruby exec "./bin/rails", "server" snippet hard-coded here. Because of that mismatch, current_content == default_bin_dev never succeeds, so the default file is never removed and the installer still prompts. Please load and strip the actual template (or otherwise compare against the real shell script) before calling rm_f.
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/install_generator.rb around lines 80 to 92, the
code compares bin/dev to a hard-coded Ruby snippet but Rails' actual template is
a shell script; replace the hard-coded default with the real template content by
locating and reading railties' template file
(railties/lib/rails/generators/rails/app/templates/bin/dev.tt) from the
installed gem (e.g. via Gem.loaded_specs['railties'].full_gem_path +
'/lib/rails/generators/rails/app/templates/bin/dev.tt'), strip it and use that
string for the equality check; keep the existing guard and removal logic so
bin/dev is removed only when it matches the real Rails template.
a9b33a8 to
5e31d06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
80-92: Fix the bin/dev comparison to match Rails' actual template.The hard-coded default doesn't match Rails' actual
bin/devtemplate, which is a shell script generated fromrailties/lib/rails/generators/rails/app/templates/bin/dev.tt. As a result,current_content == default_bin_devalways fails and the file is never removed, defeating the PR's purpose of preventing installer conflicts.Load the actual Rails template for comparison:
def remove_default_bin_dev bin_dev_path = File.join(destination_root, "bin/dev") return unless File.exist?(bin_dev_path) return unless File.writable?(bin_dev_path) - # Rails 7+ default bin/dev content - # If Rails changes this default, this comparison will safely fail and preserve the file - default_bin_dev = <<~RUBY.strip - #!/usr/bin/env ruby - exec "./bin/rails", "server", *ARGV - RUBY + # Load actual Rails bin/dev template + railties_gem_path = Gem.loaded_specs["railties"]&.full_gem_path + return false unless railties_gem_path + + template_path = File.join(railties_gem_path, "lib/rails/generators/rails/app/templates/bin/dev.tt") + return false unless File.exist?(template_path) + + default_bin_dev = File.read(template_path).strip current_content = File.read(bin_dev_path).strip return unless current_content == default_bin_dev puts Rainbow("🗑️ Removing default bin/dev file to avoid conflicts...").yellow FileUtils.rm_f(bin_dev_path) end
🧹 Nitpick comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
104-118: Well-implemented comparison with safe error handling.The method correctly:
- Locates the shakapacker gem's default config
- Guards against missing gem or file
- Returns false on any comparison errors (safe default)
The exact content match (without stripping whitespace) is the conservative and correct approach—only removing files that are completely identical to the gem default.
If you want to be slightly more lenient and handle potential whitespace differences, consider normalizing both contents:
def shakapacker_yml_matches_default?(config_path) shakapacker_gem_path = Gem.loaded_specs["shakapacker"]&.full_gem_path return false unless shakapacker_gem_path default_config_path = File.join(shakapacker_gem_path, "lib/install/config/shakapacker.yml") return false unless File.exist?(default_config_path) - default_content = File.read(default_config_path) - current_content = File.read(config_path) + default_content = File.read(default_config_path).strip + current_content = File.read(config_path).strip current_content == default_content rescue StandardError # If we can't compare, don't delete - better safe than sorry false endHowever, the current exact-match approach is perfectly valid and arguably safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/install_generator.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
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). (2)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (3)
lib/generators/react_on_rails/install_generator.rb (3)
41-41: LGTM!The placement of
remove_default_gem_filesbeforeinvoke_generatorsensures cleanup happens before any file conflicts can occur.
70-73: LGTM!The orchestration method cleanly delegates to specific removal methods.
94-102: LGTM!The method includes proper guard clauses (existence, writability, content match) and delegates the comparison logic appropriately. The user feedback before removal is helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
94-105: The bin/dev default comparison is still incorrect.As previously flagged, the hard-coded Ruby snippet doesn't match Rails' actual
bin/devtemplate. Rails generates a shell script (fromrailties/lib/rails/generators/rails/app/templates/bin/dev.tt), not the Ruby code shown here. Because of this mismatch, the comparison will always fail, files won't be removed, and interactive prompts will still occur—defeating the purpose of this PR.You must replace the hard-coded default with the actual Rails template content by locating and reading the template file from the installed Rails gem, as detailed in the previous review comment.
🧹 Nitpick comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
117-131: Consider trimming whitespace for more reliable comparison.The current implementation performs a direct byte-for-byte comparison without normalizing whitespace or line endings. While this is safe (won't accidentally delete customized files), it may fail to detect matching defaults that have minor whitespace differences, potentially leaving default files that could still cause interactive prompts.
Consider applying this diff to normalize whitespace:
def shakapacker_yml_matches_default?(config_path) shakapacker_gem_path = Gem.loaded_specs["shakapacker"]&.full_gem_path return false unless shakapacker_gem_path default_config_path = File.join(shakapacker_gem_path, "lib/install/config/shakapacker.yml") return false unless File.exist?(default_config_path) - default_content = File.read(default_config_path) - current_content = File.read(config_path) + default_content = File.read(default_config_path).strip + current_content = File.read(config_path).strip current_content == default_content rescue StandardError # If we can't compare, don't delete - better safe than sorry false end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/install_generator.rb(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/generators/react_on_rails/install_generator.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (1)
rakelib/example_type.rb (1)
gemfile(37-39)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-and-test
🔇 Additional comments (4)
lib/generators/react_on_rails/install_generator.rb (4)
41-47: LGTM! Smart pre-installation detection.The logic correctly captures the Shakapacker configuration state before running generators, ensuring default files are only removed if Shakapacker was already present. This prevents deleting files that the generator itself will create.
82-92: Well-structured with appropriate guards.The removal logic correctly checks file existence and writability before attempting deletion, and provides user feedback. The safety depends on the accuracy of the comparison method.
107-115: LGTM! Consistent removal pattern.The Shakapacker config removal follows the same safe pattern as
remove_default_bin_dev: existence and writability checks, delegated comparison, user feedback, and safe deletion.
270-270: LGTM! More robust path resolution.Using
destination_rootfor the Gemfile path is more reliable in the generator context, ensuring the method works correctly regardless of the current working directory.
Automatically detect and remove default bin/dev (Rails) and config/shakapacker.yml (Shakapacker) files before running the React on Rails installer. This prevents interactive conflict prompts that would interrupt the installation process. The installer now: - Checks if bin/dev matches the Rails default and removes it if so - Checks if config/shakapacker.yml matches Shakapacker default and removes it - Only removes files that exactly match gem defaults (safe deletion) - Provides user feedback when removing files This ensures a smooth, non-interactive installation experience while preserving any customized versions of these files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address code review feedback to improve the file deletion logic: - Add file writability check before attempting deletion - Use atomic FileUtils.rm_f instead of File.delete to prevent race conditions - Add comment documenting Rails 7+ default bin/dev content - Remove invalid options.verbose? check - Remove redundant rescue blocks (FileUtils.rm_f handles non-existent files) These changes make the file deletion safer and more robust, handling edge cases like permission issues and race conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improve code organization by extracting the bin/dev default comparison logic into a dedicated method. This makes the code more maintainable and adds clearer documentation about why we use a hardcoded default (Rails doesn't provide a bin/dev template). The shakapacker.yml comparison already correctly fetches and compares against the actual installer template from the Shakapacker gem. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Only remove default bin/dev and config/shakapacker.yml files if Shakapacker is already configured before running the React on Rails installer. This prevents us from deleting files that we create ourselves during the installation process. The logic now: - IF Shakapacker already configured → remove default conflicting files - IF Shakapacker NOT configured → we'll install it, don't remove our own files This fixes the CI failure where shakapacker.yml was being removed after we created it, causing subsequent steps to fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Move the Shakapacker pre-configuration check to BEFORE invoking generators, but keep the file removal logic BEFORE generators run. The correct sequence is now: 1. Check if Shakapacker is configured (save as boolean) 2. If pre-configured, remove default conflicting files 3. Invoke generators (which may install Shakapacker) 4. Continue installation This prevents removing files we create ourselves while still removing conflicting default files when Shakapacker was installed beforehand. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix shakapacker detection by using destination_root for the Gemfile path instead of just "Gemfile". This ensures we check the correct Gemfile in the target Rails app, not the gem's directory. This prevents the "gem specified twice" error that occurred when Rails apps already had shakapacker in their Gemfile but our detection failed to find it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove the entire feature that attempted to auto-delete default bin/dev and config/shakapacker.yml files. This feature was causing more problems than it solved: - Difficulty detecting when Shakapacker was truly pre-configured - Race conditions and timing issues during installation - Complexity in determining which files were safe to delete The original goal was to avoid interactive prompts during installation, but the feature introduced too many edge cases and test failures. Users can manually handle any conflicts if they arise during installation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
update-generator-no-conflict-default-shakapacker-yml
branch
from
ce03cfc to
0281811
Compare
Summary
Automatically detect and remove default
bin/dev(Rails) andconfig/shakapacker.yml(Shakapacker) files before running the React on Rails installer to prevent interactive conflict prompts.Changes
remove_default_gem_filesmethod that runs before generator invocationremove_default_bin_devto check and delete Rails defaultbin/devremove_default_shakapacker_ymlto check and delete Shakapacker default configshakapacker_yml_matches_default?helper to safely compare filesProblem Solved
Previously, when running
rails generate react_on_rails:installon a Rails app with default Shakapacker installation, the generator would pause with interactive prompts asking whether to overwrite conflicting files. This interrupted automated installations and required manual intervention.Solution
The installer now:
bin/devexists and matches the Rails defaultconfig/shakapacker.ymlexists and matches Shakapacker defaultTest Plan
bin/dev- should auto-remove🤖 Generated with Claude Code
This change is
Summary by CodeRabbit