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}
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
Outdated
Show resolved
Hide resolved
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
|
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/lib/generators/react_on_rails/generator_messages.rb
Outdated
Show resolved
Hide resolved
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
|
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
Outdated
Show resolved
Hide resolved
ReviewPrevious rounds addressed most of the feedback well — the content-comparison approach in Two remaining issues: 1.
|
Review: fix: replace marker file with Thor-idiomatic force option (#2289)This is a well-designed fix. The content-comparison approach in A few issues to flag: Test isolation: leftover directoriesThe Edge case in content comparisonIn @shakapacker_just_installed = yml_content_before != yml_content_afterIf
|
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
Outdated
Show resolved
Hide resolved
Review: Replace marker file with Thor-idiomatic force optionThe overall approach is solid — replacing a file-based sentinel with an in-memory flag and explicit option forwarding is more idiomatic Thor and eliminates the cleanup burden. The regression test for #2289 and the unit tests for copy_packer_config force behavior are good additions. Issues foundStale marker file in --pro --rspack test (spec line 675) This context still creates .shakapacker_just_installed and uses the old run_generator_test_with_args helper, which was not migrated to the new pattern. The marker file is now completely ignored by the generator, so this context is silently testing the pre-installed scenario while its comment says "just installed". The test likely still passes because configure_rspack_in_shakapacker uses gsub_file regardless of the force flag, but the scenario is not what is documented. The other rspack tests (plain --rspack and --rspack --typescript) were correctly migrated in this PR — this one was missed. Content-comparison detection has one unacknowledged edge case @shakapacker_just_installed = yml_content_before != yml_content_after works correctly for the four cases documented in the comments. The only unacknowledged case is if Shakapacker's installer normalises YAML (e.g., trailing whitespace, quoting style) without the user agreeing to overwrite — the content would differ, triggering a force-overwrite of the user's customised config. This is unlikely today but could silently break if Shakapacker's conflict-prompt behaviour changes. puts in generator action methods copy_packer_config uses raw puts. Thor generators should use say or say_status so output respects the --quiet/verbosity level. Not introduced by this PR, but the new message on line 126 has the same issue. SHAKAPACKER_YML_PATH constant declared after private Ruby's private keyword does not affect constants — SHAKAPACKER_YML_PATH is still public as InstallGenerator::SHAKAPACKER_YML_PATH. Declaring it before private (with the other class-level declarations) would make the intent clearer. force:/skip: forwarding passes nil when flags are absent options[:force] and options[:skip] return nil (not false) when neither flag was passed. Passing force: nil explicitly to invoke is harmless in practice (Thor treats it as falsy), but it differs from not passing the key at all. Minor nitThe comment on shakapacker_just_installed? (lines 65-68) says "In production, this is normally detected at runtime via @shakapacker_just_installed" but the method evaluates options first. Worth rewording to clarify that the CLI option overrides runtime detection. |
react_on_rails/lib/generators/react_on_rails/install_generator.rb
Outdated
Show resolved
Hide resolved
justin808
added
the
release:16.4.0-must-have
The .shakapacker_just_installed marker file caused copy_packer_config to skip RoR's template entirely, leaving private_output_path commented out. Replace the marker with a hidden --shakapacker-just-installed class option passed through invoke, and use template(..., force: true) to overwrite Shakapacker's defaults without prompting. Pre-installed configs still get the interactive prompt. Guard against the partial-install edge case: only set the flag when config/shakapacker.yml did not exist before running the Shakapacker installer, so a pre-existing customized config is never force-overwritten. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test contexts that simulate pre-existing shakapacker.yml (pre-installed, rspack, rspack+typescript) now use Thor's --skip flag instead of --force. This makes template() skip existing files while gsub_file patchers still run, correctly testing the "user declines overwrite" scenario. Also reverts the elsif File.exist? branch in copy_packer_config — the standard Thor behavior (prompt on conflict, respect --force/--skip) is correct for the pre-installed case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thor's invoke with an explicit options hash replaces parent options, dropping force/skip flags. This meant `rails g react_on_rails:install --skip` wouldn't propagate to BaseGenerator's file operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The hidden class_option was only declared on BaseGenerator, so passing --shakapacker-just-installed to InstallGenerator (e.g. in tests) was silently ignored. The regression test for #2289 was passing by accident due to global --force, not because the intended code path was exercised. Now InstallGenerator declares the option too, and the invoke line checks both the CLI flag and the runtime detection (@shakapacker_just_installed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ession test - install_shakapacker now returns true/false instead of nil, so the guard in ensure_shakapacker_installed (finalize_shakapacker_setup only runs when install actually succeeded) works correctly - Regression test for #2289 no longer uses run_generator_test_with_args which unconditionally appends --force; now calls run_generator directly so the test only passes because --shakapacker-just-installed enables force: true in copy_packer_config, not because --force overwrites all conflicting files globally - Add after cleanup to copy_packer_config force behavior describe block to remove shakapacker.yml left by the before hook - Add CHANGELOG entry for the #2289 fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…regression, redundant guard - Use content comparison (before/after File.read) in ensure_shakapacker_installed so @shakapacker_just_installed is set correctly in all cases: fresh install (nil→content), user said y to overwrite (old→new), and user said n to keep custom config (same→same). Previously config_existed only handled the fresh-install case and missed the y-overwrite scenario, leaving private_output_path commented after the user consented to a fresh config. - Add 4 unit tests for the ensure_shakapacker_installed detection path, covering all three scenarios plus the install failure case. This path was previously untested because install_shakapacker always returned false in the test environment. - Restore the detailed Shakapacker setup summary in the post-install message. Pass shakapacker_just_installed through helpful_message_after_installation so fresh installs show the three-checkmark summary instead of "already configured". - Remove redundant || false from just_installed assignment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract `options.shakapacker_just_installed? || @shakapacker_just_installed` into a private `shakapacker_just_installed?` helper to eliminate duplication between invoke_generators and add_post_install_message - Extract "config/shakapacker.yml" into SHAKAPACKER_YML_PATH constant to prevent the two read sites in ensure_shakapacker_installed and finalize_shakapacker_setup from diverging - Add comment explaining Dir.pwd reliance for File.exist?/File.read calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Forward force:/skip: to react_on_rails:pro and react_on_rails:rsc invocations, consistent with :base, :react_with_redux, and :react_no_redux which were updated in the same PR commit (30be511) - Remove dead destination_root stubs from detection path unit tests: ensure_shakapacker_installed uses Dir.pwd (via relative SHAKAPACKER_YML_PATH), not destination_root, so the stubs had no effect on test outcomes — confirmed by running the 4 tests with and without them (4/4 pass either way) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…etup - Add comment in finalize_shakapacker_setup documenting the nil→nil case (install_shakapacker returns true but writes nothing): treated as "no change needed" and force-overwrite is skipped — theoretically possible but extremely unlikely given how Shakapacker's installer works - Clarify --shakapacker-just-installed option comment: passing it manually overrides runtime detection and force-overwrites the yml unconditionally - Remove dead before/after blocks from copy_packer_config force behavior tests: template is fully mocked so neither the file creation nor cleanup has any effect on test outcomes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ment Ruby's private keyword does not affect constants — SHAKAPACKER_YML_PATH was accessible as InstallGenerator::SHAKAPACKER_YML_PATH regardless of placement. Moving it alongside the class_option declarations makes the intent clear. Also reword the shakapacker_just_installed class_option comment to explicitly state that the CLI flag takes precedence over the @shakapacker_just_installed ivar (via Ruby short-circuit evaluation of ||), not the other way around. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ihabadham
force-pushed
the
ihabadham/fix/replace-marker-with-force-option
branch
from
d616da8 to
5190008
Compare
Code Review: PR #2411Overall assessment: This is a solid refactor that correctly removes a fragile on-disk marker file approach in favour of in-memory state. The core logic is sound and the regression test for issue #2289 is appropriate. A few items need attention before merge. Bug: Missed marker-file cleanup in the
|
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