docs: warn about react_component helper collision with react-rails (#3143)#3160
docs: warn about react_component helper collision with react-rails (#3143)#3160
Conversation
Incremental migrations from react-rails were breaking silently as soon as react_on_rails was added: both gems define react_component with incompatible signatures, and Rails helper auto-loading makes the second-defined one win. Un-migrated views then raise ArgumentError. Extend the migration guide with a preflight callout and a new "Coexistence" section that explains the collision, shows how to detect affected call sites, and documents two paths: - migrate all call sites in one PR (recommended), or - preserve legacy react-rails semantics via a prepended shim and use an explicit react_on_rails_component alias for migrated mounts. Addresses #3143. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds a “Coexistence” section to the React Rails migration guide documenting that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: b9f78c1339
ℹ️ 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.
🧹 Nitpick comments (1)
docs/oss/migrating/migrating-from-react-rails.md (1)
169-169: Optional: consider "impractical" for conciseness.Minor style suggestion: "If a single-PR migration is impractical" reads slightly more concise than "is not practical."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/migrating/migrating-from-react-rails.md` at line 169, Replace the phrase "is not practical" with the more concise "is impractical" in the sentence that mentions keeping react-rails' react_component semantics and introducing a separate helper for migrated mounts; locate the line containing the string "If a single-PR migration is not practical, you can keep `react-rails`'s `react_component` semantics..." and update it to read "If a single-PR migration is impractical, you can keep `react-rails`'s `react_component` semantics..." to apply the style suggestion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/oss/migrating/migrating-from-react-rails.md`:
- Line 169: Replace the phrase "is not practical" with the more concise "is
impractical" in the sentence that mentions keeping react-rails' react_component
semantics and introducing a separate helper for migrated mounts; locate the line
containing the string "If a single-PR migration is not practical, you can keep
`react-rails`'s `react_component` semantics..." and update it to read "If a
single-PR migration is impractical, you can keep `react-rails`'s
`react_component` semantics..." to apply the style suggestion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee864d01-fb5a-4998-874f-0c973080d51b
📒 Files selected for processing (1)
docs/oss/migrating/migrating-from-react-rails.md
Code Review — docs: warn about react_component helper collision with react-rails (#3143)Overall: Good docs addition that addresses a real, reproducible migration footgun. The explanation of why the collision happens is accurate, and Option A/B structure is clear. A few issues worth addressing before merge. Issues1. Monkey-patch lines belong in an initializer, not a helper file (medium)The shim file is placed in ```ruby …are global monkey-patches that must run before any view renders. In development, Zeitwerk loads Rails'
2. "silently" is misleading (minor)
The failure is not silent — it raises 3.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Greptile SummaryThis docs-only PR adds a preflight warning and a new "Coexistence" section to Confidence Score: 5/5Safe to merge; docs-only change with one minor P2 suggestion about a superfluous All findings are P2 (style/best-practice). The shim logic, ancestor-chain explanation, and Line 207 of Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Add react_on_rails to Gemfile] --> B{Legacy react_component calls in views?}
B -- No --> C[Add gem, migrate freely]
B -- Yes --> D{Can migrate all call sites in one PR?}
D -- Yes --> E[Option A: Convert all calls to options-hash form in same PR]
E --> F[No collision — only react_on_rails helper active]
D -- No --> G[Option B: Shim approach]
G --> H[Create app/helpers/react_on_rails_coexistence.rb]
H --> I[Create config/initializers/react_on_rails_coexistence.rb]
I --> J[to_prepare: prepend LegacyReactComponent to ReactOnRailsHelper]
J --> K[to_prepare: include ReactOnRailsCoexistence into ActionView::Base]
K --> L{View calls react_component?}
L -- legacy call --> M[LegacyReactComponent delegates to React::Rails::ViewHelper]
L -- migrated call --> N[use react_on_rails_component alias]
N --> O[Calls ReactOnRails::Helper#react_component directly]
M --> P[Incremental migration — remove shim when done]
O --> P
Reviews (2): Last reviewed commit: "docs: address PR review feedback on reac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Clarify that the helper collision is driven by engine initializer load order, not file order in app/helpers/ - Forward &block in the LegacyReactComponent shim so react-rails block-form calls keep working (react-rails' react_component accepts a block for mount-tag content) - Split Option B into a helper file + initializer; apply the prepend/include inside Rails.application.config.to_prepare so patches run once at boot and are reapplied after dev reloads (avoiding Zeitwerk autoload timing issues and duplicate ancestors) - Reword "happens silently" to clarify that ArgumentError is raised at render time; Rails gives no boot-time warning - Add a comment in the react_on_rails_component snippet explaining why the unbound method is fetched from ReactOnRails::Helper (not ReactOnRailsHelper) to avoid infinite recursion - Add Action Mailer and Rails engines to the Option B known limitations alongside gem-provided engines Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review feedback addressed in edc08c2All seven inline review comments on this PR have been addressed and the threads resolved. MUST-FIX (3)
OPTIONAL (3)
Rationale for summary comments not replied-to individually
Thanks to everyone for the reviews. |
|
To use Codex here, create an environment for this repo. |
|
I'll trigger a re-review to verify the updated commit. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edc08c2699
ℹ️ 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".
Clarify that 2-argument legacy calls like `react_component "Foo", props` do not raise — they are silently accepted and drop props — while only 3+ argument calls raise ArgumentError. The previous wording implied every un-migrated view would fail loudly, which could mislead readers doing staged migrations into missing silent prop loss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review feedback addressed in 054c6eaAddressed
Not addressed (rationale)
Thanks. |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
Happy to accept the "is not practical" call — the phrasing is fine as-is. ✏️ Learnings added
🧠 Learnings used |
Review: docs —
|
Address remaining PR review feedback on the react-rails coexistence section: - Replace the inaccurate "rename react_on_rails_component back to react_component" cleanup step with the actual procedure (update each call site and delete the two shim files). - Note that react_on_rails wins the helper collision in practice because of initializer sort order, but that teams should not rely on ordering to resolve the conflict. - Tighten "is not practical" to "is impractical". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review feedback addressed in 5149483All remaining review threads on this PR are now resolved. Addressed
Already addressed in 054c6ea
All changes are documentation-only. |
|
Tip For best results, initiate chat on the files or code changes.
The migration guide is in good shape — the two failure-mode split, the 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
…tion - Split the long "Why it collides" paragraph into two so the failure-mode bullets open their own paragraph (review #3106012646). - Document bind(self)'s precondition (self must include React::Rails::ViewHelper) and mention bind_call as a Ruby 2.7+ alternative (review #3106012927). - Note in the to_prepare block that Ruby skips duplicate prepend/include so reloads don't accumulate ancestors (review #3106013062). - Add an explicit "two-rename" callout so readers expect call sites to flip name twice across the migration lifecycle (review #3105700862). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review round-upI (Claude Code) addressed the seven open review threads on the coexistence section. Three were already covered by Already addressed in db2273b
Fixed in fdcd292
All seven threads have been resolved. CodeRabbit's most recent review reported "No actionable comments" — no further changes from that bot. |
Review: docs — react_component helper collision during staged migrationOverviewSolid, well-structured addition. The preflight warning (item 7 in the checklist) and the silent-failure description (two-argument form silently drops props) are the most valuable parts — that failure mode is genuinely non-obvious. The explanation of Rails helper-precedence mechanics is accurate. A few things worth addressing before merge: Issues1. bind_call mentioned in comment but not used The shim's inline comment says "On Ruby 2.7+, .bind_call(self, ...) skips the BoundMethod allocation" but then calls .bind(self).call(...). React on Rails already requires Ruby 2.7+, so if bind_call is preferable it should be used; otherwise the hint is misleading. The same pattern appears in react_on_rails_component. Either switch both to bind_call or drop the comment. 2. Shim file in app/helpers/ loaded via bare require Putting the module in app/helpers/react_on_rails_coexistence.rb means Zeitwerk registers ReactOnRailsCoexistence as an autoload constant for that path. The initializer's require races Zeitwerk to define it, which usually wins in practice but is fragile — in eager-load mode (production) Zeitwerk's eager loader may reopen or conflict with the constant. A cleaner home is lib/react_on_rails_coexistence.rb (outside Zeitwerk's autoload paths), or just define the module inline in the initializer since it's migration-only scaffolding anyway. 3. Detection command only scans app/views Apps using ViewComponent, Phlex, or Cells keep React mounts outside app/views. A note to also check app/components (or omit the path to scan the whole project) would make the audit more complete. 4. Untested shim (acknowledged in PR description) Worth surfacing this caveat in the docs themselves with a brief callout: "This pattern is adapted from a real migration; test it in your specific app before relying on it in production." Strengths
|
Address review feedback on the react_component collision section: - Define the ReactOnRailsCoexistence module directly in the initializer instead of splitting across app/helpers/ and config/initializers/. This keeps the shim out of Zeitwerk's autoload paths and avoids bookkeeping confusion in production eager-load mode. - Switch both helpers from .bind(self).call(...) to .bind_call(self, ...) so the code matches the Ruby 2.7+ guidance mentioned in the comment. React on Rails requires Ruby 3.0+, so bind_call is always available. - Broaden the detection grep to include app/components and note that users with Phlex/mailer/engine mounts should expand the path further or drop it entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review round-upI (Claude Code) addressed the four open review threads from the 2026-04-19 10:02 review — all resolved in What changed
All four threads have been resolved. |
ReviewOverall: High-quality docs addition that addresses a real, under-documented pain point. The technical analysis of why the collision happens is accurate (confirmed against the actual helper module structure: What works well
Issues1. Wording inconsistency in preflight item 7 (inline, line 15) 2. 3. Shim code is unverified (inline, lines 205–210) 4. Option B double-rename friction is understated (inline, line 221) Minor nits (non-blocking)
|
- Fix preflight item 7 wording: "second-loaded" implied gem/load order, but body correctly explains it's Rails helper module precedence. - Add Ruby 2.7+ caveat with `bind(self).call(...)` fallback for the `UnboundMethod#bind_call` calls in the shim. - Add a callout marking the shim as community-sourced and not covered by the react_on_rails test suite; recommend staging verification. - Foreground the two project-wide rename passes as the first bullet in Known limitations of Option B, removing the now-duplicate trailing Note. - Include `app/mailers` in the detection grep example so it matches the surrounding prose. - Qualify "Rails gives no boot-time warning" with "As of Rails 7/8" to future-proof the claim.
Review round-upI (Claude Code) addressed the four open review threads and both summary-review nits from the 2026-04-23 review — all resolved in Addressed (inline threads)
Addressed (summary-review minor nits)
All four inline threads are resolved. |
|
Good docs addition addressing a real silent-failure footgun. Structure and A/B option split are clear. A few issues in the Option B shim to address before merge. |
- Detection commands: added `app/helpers` to the `rg`/`grep` path list so view-helper wrappers that call `react_component` from Ruby are not missed. - Shim inline comment: spelled out that `bind_call` raises `TypeError` at render time when `self` doesn't include `React::Rails::ViewHelper`, and pointed readers at the Known Limitations section. - Known Limitations: upgraded the "will not help" bullet to make explicit that unsupported view contexts (Rails engines, ViewComponent, Action Mailer, gem engines) hard-fail with `TypeError` rather than degrade gracefully, and added a new bullet on cleanup ordering: delete `config/initializers/react_on_rails_coexistence.rb` in the same commit that drops `react-rails` from the `Gemfile`, otherwise any request that still routes through the legacy delegate raises `NameError: uninitialized constant React::Rails::ViewHelper` at render time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review round-upI (Claude Code) addressed the three open review threads from the 2026-04-23 02:21 review — all resolved in Addressed
Lefthook (trailing newlines, Lychee link check, Prettier) passes locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e272eca41
ℹ️ 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 is in good shape after multiple rounds of iteration. The collision explanation is accurate, the Option A/B structure is clear, and the Known Limitations section is comprehensive. One technical gap remains before merge. Remaining concern:
|
1 participant
Summary
react-railsbreak silently as soon asreact_on_railsis added because both gems define areact_componentview helper with incompatible signatures, and Rails helper auto-loading lets the second-defined one win. Un-migrated views then raiseArgumentErrorat render time.docs/oss/migrating/migrating-from-react-rails.mdwith a preflight callout and a new Coexistence: keeping both gems installed during a staged migration section covering:rg/grepto audit legacy positional calls before adding the gem,react-railssemantics via a prependedLegacyReactComponentshim and expose an explicitreact_on_rails_component(...)alias for migrated mounts (the pattern thehackclub/hcbmigration actually used),.claude/docs/changelog-guidelines.md(docs fixes are excluded).Why docs, not a code change
The issue proposes either (1) avoid taking over the default helper when
react-railsis detected, or (2) document the collision. Option 1 would mean changing the semantics of a public view helper based on runtime gem detection — brittle and a backwards-compat risk for everyone who already migrated. The reporter ended up using exactly the two shims now documented in Option B, so codifying that path is the pragmatic fix and leaves room for a future product-level coexistence mode if demand warrants it.Test plan
npx prettier --check docs/oss/migrating/migrating-from-react-rails.mdpasses.#legacy-compatibility-fixes-that-often-make-migration-one-shotmatches an existing heading in the same file.react-rails+react_on_railsapp (not verified in this PR — the pattern is lifted from the reporter's working migration).Fixes #3143.
🤖 Generated with Claude Code
Note
Low Risk
Low risk: documentation-only changes. Main risk is misleading guidance for coexistence shims if copied verbatim, but no runtime code is changed in this repo.
Overview
Adds a new preflight warning and an expanded coexistence section in
migrating-from-react-rails.mdexplaining thatreact-railsandreact_on_railsboth definereact_componentwith incompatible signatures, which can cause runtimeArgumentErroror silent prop loss during staged migrations.Provides quick audit commands to find legacy call sites and documents two mitigation paths: migrate all mounts to the options-hash form in the same PR, or temporarily add an app-level initializer shim that preserves legacy
react_componentbehavior and introduces an explicitreact_on_rails_componentalias (with limitations and cleanup guidance).Reviewed by Cursor Bugbot for commit 5394237. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit