Hide legacy Redux install generator path#4277
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:
WalkthroughThe Redux install option is hidden from generator help and usage text. Legacy warnings are emitted for install and standalone Redux generator paths, and the Tailwind validation text now includes legacy and default install commands. Specs and the changelog were updated. ChangesRedux Generator Deprecation (V17)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
+ci-status |
|
+ci-force-full |
CI StatusHead SHA: Only the required gate is active unless hosted CI is requested. |
Greptile SummaryThis PR hides the legacy Redux installer path from normal generator guidance. The main changes are:
Confidence Score: 4/5The legacy Redux recovery path needs a fix before merging.
react_on_rails/lib/generators/react_on_rails/install_generator.rb; react_on_rails/lib/generators/react_on_rails/react_with_redux_generator.rb Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
github-actions
Bot
added
force-full-hosted-ci
Force-Full Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 209470deb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
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)
800-819: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the recovery command faithful to the requested install path.
Line 818 now drops
--redux, but the generator still materially branches on that flag in Lines 245-267 and inreact_on_rails/lib/generators/react_on_rails/base_generator.rb. Ifshakapacker:installfails before the child generators run, the suggested rerun command replays the default HelloWorld flow instead of the originally requested legacy Redux scaffold. Preserve the legacy flag here, or append an explicit follow-up step to runrails generate react_on_rails:react_with_reduxso recovery still reaches the same result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@react_on_rails/lib/generators/react_on_rails/install_generator.rb` around lines 800 - 819, The recovery command in recovery_install_command is missing the legacy --redux path, so reruns can fall back to the default HelloWorld scaffold instead of the originally requested Redux setup. Update the flag assembly to preserve the explicit redux choice alongside the existing bundler, RSC, pro, and agent-files options, or otherwise ensure the suggested recovery flow invokes react_with_redux. Keep the suggested command faithful to the same install path chosen by options handling in InstallGenerator and BaseGenerator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 44-45: The changelog bullet in CHANGELOG.md is missing the
repository’s standard PR-link suffix. Update this existing entry to match the
surrounding changelog format by appending the required PR reference and author
attribution text, keeping the current summary intact while adding the usual “[PR
…](…) by […](…).” trailer.
---
Outside diff comments:
In `@react_on_rails/lib/generators/react_on_rails/install_generator.rb`:
- Around line 800-819: The recovery command in recovery_install_command is
missing the legacy --redux path, so reruns can fall back to the default
HelloWorld scaffold instead of the originally requested Redux setup. Update the
flag assembly to preserve the explicit redux choice alongside the existing
bundler, RSC, pro, and agent-files options, or otherwise ensure the suggested
recovery flow invokes react_with_redux. Keep the suggested command faithful to
the same install path chosen by options handling in InstallGenerator and
BaseGenerator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca352ccc-25fa-4049-9ce2-37a84c2fbc18
📒 Files selected for processing (6)
CHANGELOG.mdreact_on_rails/lib/generators/USAGEreact_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rbreact_on_rails/lib/generators/react_on_rails/react_with_redux_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
💤 Files with no reviewable changes (1)
- react_on_rails/lib/generators/USAGE
justin808
force-pushed
the
codex/v17-redux-generator-policy-4272-4273
branch
from
209470d to
ba301d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@react_on_rails/lib/generators/react_on_rails/install_generator.rb`:
- Around line 792-794: Update the guidance text in the install generator so the
suggested legacy scaffold command uses the gem convention with bundle exec.
Locate the user-facing message in the install generator around the
react_with_redux reference and change the command example to include bundle exec
for the rails generate invocation.
- Around line 789-795: The warning in install_generator currently sends users
with `--redux --tailwind` to the standalone `ReactWithReduxGenerator`, but that
path does not accept Tailwind. Update the message logic around the legacy Redux
warning to branch on `use_tailwind?`: either keep Tailwind users on the install
generator path or explicitly note that Tailwind must not be combined with the
standalone Redux generator. Make sure the text in
`GeneratorMessages.add_warning` matches the actual validation in
`ReactWithReduxGenerator#validate_standalone_tailwind`.
In `@react_on_rails/lib/generators/react_on_rails/react_with_redux_generator.rb`:
- Around line 162-168: The install command examples in the generator text should
be updated to use the repo-standard Ruby invocation. In the ReactOnRails
generator message, adjust both the legacy Redux/Tailwind and default Tailwind
install examples so they are prefixed with bundle exec, keeping the references
in react_with_redux_generator consistent with the gem’s command usage
guidelines.
In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb`:
- Around line 4510-4514: The spec is asserting against rendered output even
though `add_legacy_redux_install_warning` only queues a message. Update the
example in `install_generator_spec.rb` to assert on the queued buffer exposed by
`GeneratorMessages.messages`, or explicitly call the print/render path before
checking `GeneratorMessages.output`. Use the existing
`add_legacy_redux_install_warning` and `GeneratorMessages` symbols to keep the
test aligned with the method’s actual behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b8299d0-0b2d-48f6-9438-fb0a63bae172
📒 Files selected for processing (6)
CHANGELOG.mdreact_on_rails/lib/generators/USAGEreact_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/install_generator.rbreact_on_rails/lib/generators/react_on_rails/react_with_redux_generator.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
💤 Files with no reviewable changes (1)
- react_on_rails/lib/generators/USAGE
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/lib/generators/react_on_rails/base_generator.rb
justin808
force-pushed
the
codex/v17-redux-generator-policy-4272-4273
branch
from
ba301d9 to
55e928a
Compare
|
Address-review checkpoint after the Fixed and resolved review threads for:
Validation rerun locally:
Current blocker is external: |
|
Code review posted via inline comments below. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review — PR #4277: Hide legacy Redux install generator pathOverview: This PR hides the `--redux` / `-R` flag from public generator help text, adds legacy deprecation warnings when the hidden path is used, and adds focused specs for the new policy. The approach is sound, but there are three issues worth addressing before merge. Finding 1 — HIGH:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264b3cc249
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code ReviewThis PR cleanly demotes the Redux generator path from first-class to legacy: hiding it from help text, adding targeted deprecation warnings, and narrowing the public surface area. The refactor of Finding 1 — Legacy warning is silently dropped on infrastructure errors (medium)
A user who types Fix: call Finding 2 —
|
justin808
force-pushed
the
codex/v17-redux-generator-policy-4272-4273
branch
from
74c43d5 to
de095e2
Compare
Review: Hide legacy Redux install generator pathOverall the design is solid. The layered Issues1. Hardcoded recovery command drops 2. Subtle retry behavior in Positive observations
|
|
test |
|
Code Review Summary - PR 4277: Hide legacy Redux install generator path. Well-structured deprecation shim. The _once_safely guard pattern is sound defensive programming, the error-first message ordering is correctly enforced, and the test suite covers the key scenarios (retry on failure, idempotency, ordering assertions, suppression when invoked-by-install). The overall approach - hide from help, keep the path functional, emit a prominent advisory - is the right gradualism for a breaking-ish change. Architecture: PR consolidates all action methods under a single run_generator public method (making helpers private). Strictly cleaner but behavioral change - subclasses overriding previously-public action methods will no longer have those called automatically by Rails. Bug fix: The ensure block silently fixes a pre-existing bug where print_generator_messages was never called when the standalone react_with_redux generator ran. Nits (see inline comments): (1) product_stack_install_flag ends with explicit nil return - redundant. (2) Legacy docs URL inlined as local variable - a private constant would be easier to maintain. (3) String-index ordering assertions in specs correct but could use a comment. No security or performance concerns. LGTM with the nits above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea62f23f9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [ | ||
| "Existing apps that need the legacy Redux scaffold can use the hidden react_with_redux generator. " \ | ||
| "That generator is also legacy and emits its own warning:", | ||
| "rails generate react_on_rails:react_with_redux" |
There was a problem hiding this comment.
When rails generate react_on_rails:install --redux --typescript is used without Tailwind, this warning tells users to switch to rails generate react_on_rails:react_with_redux, but that generator's :typescript option defaults to false and uses it to choose the .jsx/.js scaffold. In a TypeScript app, following the emitted command creates JavaScript Redux files instead of the TypeScript scaffold the original install requested; include --typescript in this command when options.typescript? is true.
Useful? React with 👍 / 👎.
ReviewThis PR hides the legacy I traced the full warning/printing control flow (install →
No correctness, security, or performance issues survived review. Nothing to flag. |
Review summaryReviewed the generator changes that hide the legacy Code quality / correctness
Issues found (posted inline)
Tests/rubocop weren't re-run here (sandboxed environment blocked |
justin808
left a comment
justin808
left a comment
There was a problem hiding this comment.
Submitting pending review replies from the stacked PR closeout.
justin808
left a comment
justin808
left a comment
There was a problem hiding this comment.
Submitting pending review replies from the stacked PR closeout.
…st-surface-4274 * origin/main: Docs: frame Redux as legacy shared-store guidance (#4278) Hide legacy Redux install generator path (#4277) Add typed Rails action helper (#4260) Add Rails response type generation (#4259) Remove stale Pro RSpec encoding workaround (#4291) [Pro] Fix Gemfile loader source encoding under C locale (#4281) # Conflicts: # react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
…ered-rsc-rendering * origin/main: Adopt agent-workflow binstubs (.agents/bin/ + AGENTS.md pointer) (#4264) [codex] Wire release harnesses into CI (#4266) Document bounded coordination audit fallback (#4283) Docs: frame Redux as legacy shared-store guidance (#4278) Hide legacy Redux install generator path (#4277) Add typed Rails action helper (#4260) Add Rails response type generation (#4259) Remove stale Pro RSpec encoding workaround (#4291) [Pro] Fix Gemfile loader source encoding under C locale (#4281) Fail fast for RSC on Rspack v1 (#4289) [codex] Document agent workflow trust boundary (#4288) Fix precompile hook forcing UTF-8 onto non-UTF-8 (national) locales (#4244) Regenerate Pro llms bundle (#4280)
1 participant
Why
React on Rails V17 should steer new applications toward the default component/RSC path instead of presenting Redux as normal new-app guidance. This keeps the legacy Redux scaffold available for existing scripted installs and direct generator use while making clear that runtime Redux APIs remain supported for compatibility.
Closes #4272.
Closes #4273.
What changed
react_on_rails:install --redux/-Roption from public generator help and usage text.install.react_on_rails:react_with_reduxavailable, but warn that it is a hidden legacy generator path and not recommended for new apps.--reduxfrom recovery guidance so failed new-app installs do not recommend Redux.Validation
.agents/bin/agent-workflow-seam-doctor.agents/skills/pr-batch/bin/pr-security-preflight --repo shakacode/react_on_rails 4272 4273bundle exec rspec spec/react_on_rails/generators/install_generator_spec.rb:4395 spec/react_on_rails/generators/install_generator_spec.rb:4544 spec/react_on_rails/generators/install_generator_spec.rb:4569 spec/react_on_rails/generators/install_generator_spec.rb:4751 spec/react_on_rails/generators/install_generator_spec.rb:4765 spec/react_on_rails/generators/install_generator_spec.rb:4785 spec/react_on_rails/generators/install_generator_spec.rb:4794 spec/react_on_rails/generators/install_generator_spec.rb:4806fromreact_on_rails/(8 examples)BUNDLE_GEMFILE=../Gemfile bundle exec rubocop lib/generators/react_on_rails/install_generator.rb lib/generators/react_on_rails/react_with_redux_generator.rb spec/react_on_rails/generators/install_generator_spec.rbfromreact_on_rails/git diff --checklychee 0.23.0onPATHCI recommendation
Generator output and install behavior changed, so this PR should get force-full hosted CI rather than docs-only or no hosted CI.