Walkthrough

Tracks incomplete Shakapacker setup in the install generator to suppress final success/quick-start messaging when Shakapacker install is skipped or fails; adds an invoked_by_install option to the Redux generator to suppress its standalone messages; exposes detect_package_manager and updates tests accordingly.

Changes

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Installer as InstallGenerator
  participant Shaka as ShakapackerTask
  participant ReduxGen as ReactWithReduxGenerator
  participant Messages as GeneratorMessages

  Installer->>Shaka: check/install shakapacker
  alt Shakapacker installed
    Shaka-->>Installer: success
    Installer->>ReduxGen: invoke (invoked_by_install: true) [if redux selected]
    ReduxGen-->>Installer: returns (no standalone redux messages)
    Installer->>Messages: add_post_install_message (normal success flow)
  else Shakapacker skipped/failed
    Shaka-->>Installer: failed/skipped
    Installer->>Installer: set `@shakapacker_setup_incomplete` = true
    Installer->>ReduxGen: invoke (invoked_by_install: true) [if redux selected]
    ReduxGen-->>Installer: returns (no standalone redux messages)
    Installer->>Messages: add_post_install_message -> emits incomplete_installation_message
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through scripts and set a little flag,
To hush the party when a pack went snag.
If Shakapacker missed its mark, I warn and pause,
I nibble bugs and cheer for safer cause. 🥕

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 @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR fixes #2600 by tracking when automatic Shakapacker installation fails during react_on_rails:install (via a @shakapacker_setup_incomplete instance variable) and replacing the misleading success banner with an explicit "installation incomplete" warning that lists concrete next steps. Two regression tests are added to verify the new behavior.

• Introduces @shakapacker_setup_incomplete flag set when install_shakapacker returns false, and a shakapacker_setup_incomplete? predicate to query it
• add_post_install_message now short-circuits with a warning when the flag is set, suppressing the success banner and ./bin/dev guidance
• The incomplete_installation_message provides numbered remediation steps using the detected package manager
• Gap: The --redux code path bypasses add_post_install_message entirely (line 107: unless options.redux?), so ReactWithReduxGenerator#add_redux_specific_messages can still emit the success banner after a shakapacker failure with --redux --ignore-warnings

Confidence Score: 3/5

• The PR correctly fixes the primary (non-Redux) install flow but leaves the --redux variant exposed to the same misleading success banner.
• The core logic is sound and well-tested for the non-Redux path. However, the --redux --ignore-warnings code path still bypasses the new incomplete-installation check, which means the original bug (Generator still shows overall success banner after failed Shakapacker install with --ignore-warnings #2600) can still manifest when Redux is enabled. This is a meaningful gap in the fix.
• react_on_rails/lib/generators/react_on_rails/install_generator.rb — line 107 where add_post_install_message is skipped for Redux installs

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_generators] --> B{prerequisites met OR ignore_warnings?}
    B -->|No| Z[Add error, skip install]
    B -->|Yes| C[invoke_generators]
    C --> D[ensure_shakapacker_installed]
    D --> E{shakapacker_configured?}
    E -->|Yes| F[Continue]
    E -->|No| G[install_shakapacker]
    G -->|Success| H[finalize_shakapacker_setup]
    G -->|Failure| I["@shakapacker_setup_incomplete = true"]
    H --> F
    I --> F
    F --> J{options.redux?}
    J -->|Yes| K[invoke ReactWithReduxGenerator]
    J -->|No| L[invoke ReactNoReduxGenerator]
    K --> M[add_redux_specific_messages]
    M --> N["Shows success banner ⚠️ even if incomplete"]
    L --> O[add_post_install_message]
    O --> P{shakapacker_setup_incomplete?}
    P -->|Yes| Q["Shows incomplete warning ✅"]
    P -->|No| R["Shows success banner ✅"]

_{Last reviewed commit: 9a4eb58}

Review

The fix is well-scoped and logic is correct for the non-Redux path. A few issues worth addressing:

Bug: Redux + Shakapacker failure still shows success banner

When rails generate react_on_rails:install --redux is run and install_shakapacker returns false, the incomplete-installation message is never shown. The flow: (1) invoke_generators sets @shakapacker_setup_incomplete = true; (2) add_post_install_message unless options.redux? is skipped; (3) ReactWithReduxGenerator#add_redux_specific_messages runs independently and emits the success banner regardless. This PR fixes the non-Redux path but the Redux path has the same bug as #2600. The flag lives on the InstallGenerator instance and the Redux generator has no access to it. Suggested fix: forward it as a generator option like shakapacker_just_installed and guard add_redux_specific_messages on it.

Potentially misleading step 4 in the incomplete-installation message

Step 4 suggests rails generate react_on_rails:install --skip. In Rails generators, --skip means skip any file that already exists and is not a RoR-specific option. After manually fixing Shakapacker, users may need --force to overwrite files the Shakapacker installer placed, or a plain re-run with no flag. The wording could mislead users into thinking --skip has a special meaning here.

Minor: redundant !! in shakapacker_setup_incomplete?

The predicate uses !!@shakapacker_setup_incomplete. Since the flag is always assigned true or false (and nil before ensure_shakapacker_installed runs, where !!nil == false is correct anyway), the double-negation adds noise. @shakapacker_setup_incomplete == true is clearer.

Missing test: Redux + Shakapacker failure path

No test verifies that --redux + a failing install_shakapacker suppresses the success banner. Once the Redux gap is fixed, a test analogous to the new failure spec should cover that path.

Review

The core fix (tracking @shakapacker_setup_incomplete and suppressing the success banner) is correct and well-motivated. Test coverage for the new state is solid. A few issues worth addressing:

Blocking

('use client') in docs is not a valid React directive (inline comments on rsc-troubleshooting.md).
The change replaces bare string literals 'use client' with parenthesised expressions ('use client'). Per the ECMAScript directive-prologue grammar, only a bare string literal as an expression statement qualifies. The parenthesised form is a ParenthesizedExpression and will not be recognised as a use-client boundary by React / bundlers. The "GOOD" code examples now demonstrate incorrect usage. This looks like an accidental formatter change — please revert to 'use client'.

Non-blocking

GeneratorMessages.send(:detect_package_manager) vs direct call (inline comment on install_generator.rb:335).
react_with_redux_generator.rb already calls GeneratorMessages.detect_package_manager directly. Using send here is inconsistent and unnecessary — either call it directly or surface it as public.

detect_package_manager never returns nil (inline comment on install_generator.rb:334–336).
The implementation always returns a string (defaults to "npm"), so the package_manager ? ... : "install JavaScript dependencies" guard is dead code. The spec testing the nil branch stubs an impossible runtime state. Worth aligning the implementation and the guard.

Overall: solid fix. Minor concerns posted as inline comments.

Review Summary

The fix is correct and well-scoped. Tracking @shakapacker_setup_incomplete and gating the success banner on it is the right approach for the issue. The Redux deduplication via invoked_by_install: is clean. A few observations:

Logic / Design

add_bin_scripts runs on failed setup (see inline): When Shakapacker fails, bin/dev is still created on disk, and the incomplete-install message then says "Avoid running ./bin/dev". This is called out in the warning copy ("Some generator files may have been partially created"), so it's not a bug, but the intent isn't obvious from the code. Worth a comment at the call site.

--force in step 4 of the incomplete message: Re-running with --force would overwrite any customizations a user made to files that were partially generated. This is the right suggestion for most users, but phrasing it as "add --force if needed" rather than just --force is good — no change needed, just worth confirming that's intentional.

Pretend mode + incomplete setup: In --pretend mode with an unconfigured Shakapacker, @shakapacker_setup_incomplete is explicitly set to false before the pretend early-return, so pretend runs will still show the success banner. That seems correct (pretend simulates success).

Code Style

• public :detect_package_manager after a private block works but is harder to follow than moving the method above private (inline comment posted).
• @shakapacker_setup_incomplete == true strict comparison is unnecessary given the ivar is always initialized before use (inline comment posted).

Tests

Good coverage of the new paths. One minor note: the new unit-test specs ("shows incomplete-installation guidance...", "incomplete-installation guidance uses detected package manager...") rely on before { GeneratorMessages.clear } from the enclosing describe block — that's fine, just worth keeping in mind if those tests are ever moved.

Overall this is a solid, low-risk fix. The three inline comments are all minor.

Review: Fix #2600 – incomplete install message after Shakapacker failure

Overall this is a clean, well-scoped fix. The @shakapacker_setup_incomplete flag approach is clear, the bun.lock additions are consistent across all three detection sites, the invoked_by_install flag elegantly prevents duplicate Redux messages, and the test coverage is solid. A few things worth noting:

Logic gap: gemfile-add failure path doesn't set the flag directly

ensure_shakapacker_in_gemfile failure (with --ignore-warnings) doesn't set @shakapacker_setup_incomplete itself — the flag only gets set later when install_shakapacker inevitably fails too. This works in practice but is an indirect dependency. See inline comment on lines 262–266.

Minor duplication between recovery_working_tree_note and recovery_working_tree_step

The two helpers carry identical prose; the only difference is the numbered prefix and .chomp. If the wording changes it'll need to be updated in two places. See inline comment on lines 349–363.

detect_package_manager is now public but has an implicit Dir.pwd dependency

The method was moved out of private intentionally (needed by install_generator.rb), but the dependency on Dir.pwd being set to the Rails root is invisible from the call site. A brief comment on the method would help future callers.

Test: ignore_warnings: true is load-bearing in the gemfile-error spec

Without that option the method raises Thor::Error and the assertions never run. A comment would save the next reader from wondering if they can simplify the options hash.

None of these are blockers — the fix is correct and the regression tests cover the critical path.

Review: Fix #2600 — incomplete install message after Shakapacker failure

Overall this is a well-structured fix with good test coverage. The core logic (tracking @shakapacker_setup_incomplete and suppressing the success banner) is sound, and the bun.lock support added consistently across all three package-manager detection methods is a nice cleanup.

A few things worth addressing:

Minor issues

Test regex excludes bun (see inline comment) — two existing test expectations use /bundle && (npm|yarn|pnpm) install/ which would fail in a Bun project.

--pro dropped silently when --rsc is also specified (see inline comment) — recovery_install_command excludes --pro when use_rsc? is true. If --pro --rsc is a supported combination, users would be told to re-run without --pro, getting a different install result.

install_shakapacker is still attempted when ensure_shakapacker_in_gemfile returns false — this appears intentional (tested), but worth a comment in the code since it's not obvious why you'd try to install when the Gemfile step failed. Users who manually added shakapacker already would benefit from this, but the reasoning isn't stated.

Positive notes

• shakapacker_setup_incomplete? using == true instead of a truthy check is a clean guard against nil-vs-false ambiguity.
• The ensure_shakapacker_in_gemfile boolean return is a cleaner API than the previous implicit nil.
• invoked_by_install: true on the Redux generator is the right way to avoid duplicate post-install messages without special-casing in the caller.