fix: replace marker file with Thor-idiomatic force option (#2289)#2411
fix: replace marker file with Thor-idiomatic force option (#2289)#2411
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:
WalkthroughReplaces the on-disk marker file Changes
Sequence DiagramsequenceDiagram
participant User as User
participant InstallGen as InstallGenerator
participant BaseGen as BaseGenerator
participant Template as shakapacker.yml.tt
participant Config as config/shakapacker.yml
User->>InstallGen: run react_on_rails:install
InstallGen->>InstallGen: config_existed = File.exist?("config/shakapacker.yml")
alt fresh install
InstallGen->>InstallGen: set `@shakapacker_just_installed` = true
else pre-installed
InstallGen->>InstallGen: set `@shakapacker_just_installed` = false
end
InstallGen->>BaseGen: invoke generators (pass shakapacker_just_installed, force, skip, typescript)
BaseGen->>BaseGen: evaluate shakapacker_just_installed?
alt true
BaseGen->>Template: apply template (force: true)
Template->>Config: overwrite/update (ensure version-aware settings)
else false
BaseGen->>Template: apply template (no force)
Template->>Config: preserve existing file
end
BaseGen->>User: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
Greptile SummaryReplaces the Key changes:
This ensures RoR's version-aware template (with Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[rails generate react_on_rails:install] --> B{config/shakapacker.yml exists?}
B -->|Yes| C[config_existed = true]
B -->|No| D[config_existed = false]
C --> E[install_shakapacker runs]
D --> E
E --> F[Set @shakapacker_just_installed = !config_existed]
F --> G[Pass shakapacker_just_installed flag to base_generator]
G --> H{shakapacker_just_installed?}
H -->|true| I[template with force: true]
H -->|false| J[template without force - Thor prompts user]
I --> K[RoR template applies: private_output_path uncommented, precompile_hook configured]
J --> L[User decides: overwrite, skip, or diff]
K --> M[gsub_file for additional config: rspack, precompile_hook]
L --> M
M --> N[Generation complete]
Last reviewed commit: f93a161 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
If This is a pre-existing pattern (the old marker file would also have been created in this scenario), so it's not a regression. But you could guard against it: This would require |
![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
🤖 Fix all issues with AI agents
In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb`:
- Around line 233-263: The test never triggers
InstallGenerator.finalize_shakapacker_setup because it simulates existing
shakapacker files (so shakapacker_configured? returns true) and the CLI flag
--shakapacker-just-installed isn't effective; to fix, change the before(:all)
setup for this context so ensure_shakapacker_installed actually runs — either
remove the simulated files ("config/shakapacker.yml", "bin/shakapacker",
"config/webpack/webpack.config.js") so InstallGenerator will proceed to call
finalize_shakapacker_setup and set `@shakapacker_just_installed`, or explicitly
set `@shakapacker_just_installed` = true before invoke so BaseGenerator receives
shakapacker_just_installed: true and copy_packer_config will use force: true;
also confirm the test does not rely on run_generator_test_with_args always
adding --force which masks the bug.
There was a problem hiding this comment.
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)
177-187:⚠️ Potential issue | 🟡 Minor
finalize_shakapacker_setupruns even wheninstall_shakapackerfails under--ignore-warnings.If
install_shakapackerfails and--ignore-warningsis set, the error handler returns without raising, and execution falls through tofinalize_shakapacker_setup. This prints "✅ Shakapacker installed successfully!" and sets@shakapacker_just_installed, which could lead to aforce: trueoverwrite of a config that wasn't actually freshly generated.Consider guarding finalize on install success:
Proposed fix
- install_shakapacker - finalize_shakapacker_setup(config_existed) + install_shakapacker + return unless shakapacker_configured? + + finalize_shakapacker_setup(config_existed)
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 2m 24s —— View job PR Review Complete ✅SummaryThis PR successfully fixes issue #2289 by replacing the fragile marker file approach with Thor-idiomatic option passing. The implementation is well-designed and the previous review concerns from CodeRabbit have been properly addressed in commit f93a161. Verdict: Ready to merge ✅ All CI checks pass, good test coverage, clean implementation. What This PR DoesProblem: When Root Cause: The Fix: Replace marker file with in-memory boolean flag:
Code Quality Assessment✅ Strengths
Issues & Observations
|
ihabadham
force-pushed
the
ihabadham/fix/replace-marker-with-force-option
branch
from
f93a161 to
5a98064
Compare
Review: fix: replace marker file with Thor-idiomatic force option (#2289)The approach here is well-reasoned — replacing a filesystem side-channel (marker file) with in-process state is a clear improvement, and forwarding Issues1. In 2. Hidden While 3. Redundant just_installed = options.shakapacker_just_installed? || @shakapacker_just_installed || false
4. Test isolation: The MinorThe regression test for #2289 exercises the Overall direction is correct; the marker-file approach was fragile. Items 1 and 4 are the most actionable before merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/lib/generators/react_on_rails/generator_messages.rb (1)
145-152: Consider also checking for config/shakapacker.yml before reporting “already configured.”If the bin scripts exist but the config file was deleted or never created, the status message will be misleading. Including the config file in the check reduces false positives.
Suggested tweak
- if File.exist?("bin/shakapacker") && File.exist?("bin/shakapacker-dev-server") + if File.exist?("bin/shakapacker") && + File.exist?("bin/shakapacker-dev-server") && + File.exist?("config/shakapacker.yml") "\n📦 #{Rainbow('Shakapacker already configured ✓').green}#{version_warning}" else "\n📦 #{Rainbow('Shakapacker setup may be incomplete').yellow}#{version_warning}" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/generator_messages.rb` around lines 145 - 152, The status message in build_shakapacker_status_section can be misleading because it only checks for bin/shakapacker and bin/shakapacker-dev-server; update the conditional to also verify the presence of the Shakapacker config (e.g. File.exist?("config/shakapacker.yml")) so the "already configured" branch only runs when both bin scripts and the config file exist; keep the appended version_warning (from check_shakapacker_version_warning) unchanged and adjust the if condition in build_shakapacker_status_section accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/generators/react_on_rails/generator_messages.rb`:
- Around line 145-152: The status message in build_shakapacker_status_section
can be misleading because it only checks for bin/shakapacker and
bin/shakapacker-dev-server; update the conditional to also verify the presence
of the Shakapacker config (e.g. File.exist?("config/shakapacker.yml")) so the
"already configured" branch only runs when both bin scripts and the config file
exist; keep the appended version_warning (from
check_shakapacker_version_warning) unchanged and adjust the if condition in
build_shakapacker_status_section accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
react_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/generator_messages.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails/lib/generators/react_on_rails/base_generator.rb
- react_on_rails/lib/generators/react_on_rails/install_generator.rb
Review: Replace marker file with Thor-idiomatic force optionOverall: The approach is sound - moving from a filesystem marker file to an in-memory boolean eliminates the surface-area risks (leftover files, concurrent runs, partial cleanup) and aligns with Thor idioms. The bug fix for #2289 is correct. A few issues worth addressing. 1. Runtime detection path is untested (main gap)The regression test for #2289 uses the hidden CLI flag A unit test for 2. Edge case: Shakapacker installer may re-create a pre-existing configIf This tradeoff is reasonable (protecting potential customizations), but it is worth documenting with a comment that this known edge case exists. 3. UX regression: fresh-install summary loses detailThe removed branch in 4. Minor: trailing
|
PR Review: Replace Marker File with Thor-idiomatic Force OptionOverall assessment: The approach is sound and the root-cause fix is correct. Replacing the marker-file mechanism with content comparison in Issues1. The PR correctly forwards 2. Duplicated
3.
4. The content-capture code in 5. The comment acknowledges this ("In production, this is normally detected at runtime"), but exposing a hidden CLI option specifically to make integration tests easier is a design smell. The option is partly redundant: the unit tests for What's done well
|
ReviewPrevious rounds addressed most of the feedback well — the content-comparison approach in Two remaining issues: 1.
|
|
Code Review - PR 2411 The overall approach is a clear improvement over the marker-file pattern. Good changelog and solid test coverage. ISSUE 1 - Content-comparison false-negative in finalize_shakapacker_setup: The detection sets @shakapacker_just_installed = (yml_content_before != yml_content_after). This misses the case where a user whose existing file is already byte-for-byte identical to what Shakapacker would write, even if they accepted the conflict prompt. In that scenario @shakapacker_just_installed is false, copy_packer_config runs without force, and the user gets an unexpected conflict prompt. Bug 2289 does not recur (they are prompted) but the clean fresh-install UX degrades silently. A simpler alternative: set @shakapacker_just_installed = true unconditionally in finalize_shakapacker_setup, which is only reached when install_shakapacker returns true. The content-diff approach aims to preserve user customizations when they decline Shakapacker overwrite prompt, but introduces a silent regression path worth documenting as an explicit trade-off. ISSUE 2 - --shakapacker-just-installed bypasses shakapacker_configured?: If passed via CLI, shakapacker_just_installed? returns true regardless of whether ensure_shakapacker_installed ran (which only fires when !shakapacker_configured?). Running the generator with this flag on an app with Shakapacker already fully configured will silently force-overwrite config/shakapacker.yml without any prompt. The code comment discloses this, but it is a destructive footgun for any user who discovers the flag. Consider adding a warning when used with a pre-configured install, or restricting it to test environments. ISSUE 3 (minor) - puts vs Thor say: New lines in copy_packer_config and finalize_shakapacker_setup use raw puts rather than Thor say/say_status. Thor say respects --quiet and the generator output stream. POSITIVES:
|
…arity Assign the heredoc to `base` before calling `.chomp + version_warning` so the method chain is visually separate from the heredoc body. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
2 participants
Summary
Fixes #2289 — when
rails generate react_on_rails:installruns on a fresh Rails app (Shakapacker not pre-installed),private_output_path: ssr-generatedstays commented out inconfig/shakapacker.yml.Root cause: The
.shakapacker_just_installedmarker file approach causedcopy_packer_configto skip RoR'sshakapacker.yml.tttemplate entirely, so the<% if shakapacker_version_9_or_higher? %>conditional that uncommentsprivate_output_pathnever executed.Fix: Replace the marker file with Thor-idiomatic
force: trueoption:config_existed = File.exist?("config/shakapacker.yml")before runninginstall_shakapackershakapacker_just_installed: !config_existedtobase_generatorviainvokeoptionscopy_packer_configusestemplate(..., force: true)for fresh installs (safe to overwrite Shakapacker defaults), and plaintemplate(...)for pre-existing configs (prompts user)This also fixes a secondary edge case: in partial-install scenarios (customized config exists but
shakapacker_configured?returns false), the old approach would silently force-overwrite user customizations.Changes
install_generator.rbconfig_existedcheck before install, passes boolean via invoke options, removed marker file creationclass_option :shakapacker_just_installedso tests can pass the flag via CLIforce/skipflags to invoked sub-generators (Thor's explicit options hash was dropping them)base_generator.rb— Hiddenclass_option :shakapacker_just_installed, conditionalforce: trueincopy_packer_configgenerator_messages.rb— Removed dead marker file branch frombuild_shakapacker_status_sectioninstall_generator_spec.rb— Regression test for Generator skips private_output_path configuration when Shakapacker is freshly installed #2289, unit test for force/non-force behavior,--skipflag for pre-existing config testsRelated
config/webpack/instead ofconfig/rspack/SKIPenv var to Shakapacker installerTest plan
private_output_path: ssr-generateduncommented in generated config🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests