docs: legacy Webpacker and migration-fit guidance (#3138)#3157
docs: legacy Webpacker and migration-fit guidance (#3138)#3157
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:
WalkthroughAdds targeted migration guidance in two docs: react-rails classifies first-targets and mandates a separate Webpacker→Shakapacker PR before running the React on Rails generator; vite-rails distinguishes Rails-owned island mounts from client-routed SPA shells and documents Vite runtime differences (e.g., Changes
Sequence Diagram(s)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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR adds two documentation sections to the React on Rails migration guides: a "Pick the right first target" section in the Confidence Score: 5/5Documentation-only PR with no code changes; safe to merge. All changes are prose additions to migration guides. Content is accurate, internally consistent, and addresses a genuine usability gap. No P0 or P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start migration] --> B{Which bundler?}
B -- Shakapacker 7+ & Rails 6+ --> C[Island mounts?]
B -- gem webpacker / Webpack 4 --> D[Preferred path: migrate bundler first]
D --> D1[PR 1: webpacker → shakapacker]
D1 --> D2[PR 2: run react_on_rails generator]
C -- Yes --> E[✅ Smoothest path - Run generator + follow steps]
C -- No, SPA shell --> F[⚠️ Not a quick first migration - Pick a Rails-owned island first]
F --> E
Reviews (1): Last reviewed commit: "docs: add legacy Webpacker and migration..." | Re-trigger Greptile |
![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: 5ae10c0265
ℹ️ 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".
Review: docs/oss/migrating — Webpacker & SPA-shell guidanceThis is a solid documentation improvement that solves a real user confusion. The two-level classification (island mounts vs SPA shells; Shakapacker-baseline vs Webpacker-era) is the right mental model to surface early, and the placement before Preflight is correct — readers need to self-classify before the how-to steps are useful. What works well
Issues to address1. Shakapacker version inconsistency (react-rails doc) "Pick the right first target" names Shakapacker 7+ as the smooth path, but "Preferred path for Webpacker-era apps" later says "auto-detection assumes Shakapacker 9.0+". Both facts are true, but a reader who is on Shakapacker 8.x will be unsure whether they're on the smooth path or need the explicit 2. Missing link to Shakapacker migration guide Step 1 of "Preferred path for Webpacker-era apps" recommends "Migrate to Shakapacker first, as its own PR" but doesn't link to the Shakapacker upgrade guide or the 3. Preflight is now partially redundant After the new sections, the first item in Preflight ("if the app still uses 4. Minor: The new SPA-shell paragraph mentions VerdictApprove with minor suggestions. None of the issues above are blockers, but the version-number ambiguity (#1) is the most likely to create real confusion and is worth a one-line fix before merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Add explicit guidance in the react-rails and vite_rails migration docs so teams pick a realistic first migration target: - react-rails: new "Pick the right first target" and "Preferred path for Webpacker-era apps" sections. Recommend migrating webpacker to shakapacker as a separate PR before adding react_on_rails, and spell out the compatibility shims required if a team intentionally stays on Webpack 4. - vite_rails: new "Two different starting points" section distinguishing Rails-owned island mounts from client-routed SPA shells. SPA shells should not be treated as a quick first migration. Fixes #3138. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf8815a09a
ℹ️ 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 is a docs-only PR adding up-front migration-target classification to both migration guides. The changes are well-motivated and address a real usability problem (teams misdiagnosing RoR as broken when the root cause is legacy bundler incompatibility or an SPA-shell architecture choice). Overall assessmentThe three-way classification (island mounts / Webpacker-era / SPA shells) gives readers a clear self-triage framework. The guidance is accurate. A few minor observations: 1. Preflight item 1 is now orphaned from the new guidanceThe existing Preflight checklist in
Now that there is a dedicated Preferred path for Webpacker-era apps section with richer guidance, the Preflight item should cross-reference it — otherwise readers who skim directly to Preflight miss the "in its own PR" emphasis and the shim list. Suggested update (easy one-liner): 1. **Webpacker vs Shakapacker**: if the app still uses `webpacker`, migrate to `shakapacker` first (see [Preferred path for Webpacker-era apps](#preferred-path-for-webpacker-era-apps)).2. Intentional but notable content duplicationThe four-bullet shim list in Preferred path for Webpacker-era apps (pinned webpack, babel config, webpack config edits, 3.
|
- Remove inaccurate claim that Webpacker-era apps can work with current React on Rails; `doctor.rb` flags Webpacker as a removed breaking change and the gemspec requires `shakapacker >= 6.0`. - Clarify Shakapacker version expectations: 7+ baseline with a note that `server_bundle_output_path` auto-detection requires 9.0+. - Link to the Shakapacker upgrade guide in the Preferred path. - Replace Preflight step 1 with a back-reference to avoid duplicating the new Preferred path section. - Note that `import.meta.glob` has no direct Webpack equivalent and must be replaced with explicit `require.context` calls. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai Thanks — acknowledged that the automated review found no actionable items on the latest commit. Addressing minor suggestions from parallel reviews in 02b66cd. |
|
@greptile-apps Thanks for the confidence-5/5 read. No P0/P1 findings here, and the minor suggestions from parallel reviews (Shakapacker version callout, upgrade guide link, |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Review checkpoint — review feedback addressed in 02b66cdAddressed by I (Claude Code) on behalf of @justin808: @chatgpt-codex-connector[bot] (P2):
@claude[bot] (inline + review summary):
All four resolved threads: #discussion_r3104807938, #discussion_r3104808202, #discussion_r3104808407, #discussion_r3104808522. |
|
To use Codex here, create an environment for this repo. |
|
Thanks for the update — good to know those were all addressed in 02b66cd. The docs changes are solid and the staged guidance (Webpacker → Shakapacker first, then React on Rails) is exactly the kind of upfront self-classification that saves teams from unnecessary debugging. LGTM. |
Review: docs: legacy Webpacker and migration-fit guidanceOverall: Solid, targeted docs improvement. The additions directly address the issue — readers can now self-classify before starting a migration, which should cut down on "React on Rails is broken" confusion caused by Webpacker/Shakapacker mismatch or wrong-first-target choices. The prose is clear and the staging advice (separate PRs for bundler vs. RoR install) is practical. What works well
Minor issuesTwo inline comments are posted:
Nit: Shakapacker upgrade link anchorLine 19 of No concerns on
|
- Specify `< 16.0` as the Webpacker-supporting pin in the Webpacker fallback guidance (v16 is the release that removed Webpacker support per CHANGELOG.md). - Fix the broken Shakapacker upgrade anchor (`#upgrading-from-webpacker` does not exist); link to the Webpacker → Shakapacker v6 upgrade doc instead. - Expand the `require.context` note in the vite_rails migration doc to call out the API differences (sync/async, eager/lazy, manual key extraction) and link to the webpack docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review checkpoint — follow-up feedback addressed in f4ed225Addressed by I (Claude Code) on behalf of @justin808: @claude[bot] (inline,
@claude[bot] (inline,
@claude[bot] (review summary nit about the Shakapacker anchor link):
Rationale replies on already-addressed threads (resolved):
All six review threads on this PR are now resolved. |
|
To use Codex here, create an environment for this repo. |
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)
docs/oss/migrating/migrating-from-react-rails.md (1)
102-105:⚠️ Potential issue | 🟠 MajorDocument both positional-call failure modes explicitly.
This section tells users to convert legacy calls, but it should explicitly warn that failure behavior differs by arity: 3+ args fail fast, while exactly 2 args can silently drop props. That silent path is easy to miss during migration.
✍️ Suggested doc patch
These commands list candidates only. Inspect each match manually and convert any legacy positional calls (for example `react_component('Post', `@props`, prerender: true)`, `react_component 'Post', `@props``, `react_component :Post, `@props``, or `react_component component_name, `@props``) to options-style props before running tests. + + Important: legacy positional calls fail in two different ways when `react_on_rails` wins helper resolution: + - Calls with 3+ positional args usually raise `ArgumentError` at render time (fast failure). + - Calls with exactly 2 positional args (`component_name`, props hash) may be accepted but drop props silently. + Treat both as required fixes before considering migration complete.Based on learnings: In
react_on_rails/lib/react_on_rails/helper.rb(line 84),react_component(component_name, options = {})causes two distinct legacy positional-call failure modes (3+ args error vs 2 args silent props drop), and migration docs should distinguish both explicitly.🤖 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` around lines 102 - 105, The docs mention converting legacy positional calls but don't call out the two distinct failure modes of react_component when called positionally; update the paragraph to explicitly warn that react_component(component_name, options = {}) in react_on_rails/lib/react_on_rails/helper.rb produces two different behaviors: calls with 3+ positional arguments raise an immediate error, whereas calls with exactly 2 positional arguments silently drop the props, so instruct users to search for positional usages like react_component('Post', `@props`, prerender: true), react_component 'Post', `@props`, react_component :Post, `@props`, or react_component component_name, `@props` and convert them to the options-style form to avoid both hard failures and silent prop loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/oss/migrating/migrating-from-react-rails.md`:
- Around line 102-105: The docs mention converting legacy positional calls but
don't call out the two distinct failure modes of react_component when called
positionally; update the paragraph to explicitly warn that
react_component(component_name, options = {}) in
react_on_rails/lib/react_on_rails/helper.rb produces two different behaviors:
calls with 3+ positional arguments raise an immediate error, whereas calls with
exactly 2 positional arguments silently drop the props, so instruct users to
search for positional usages like react_component('Post', `@props`, prerender:
true), react_component 'Post', `@props`, react_component :Post, `@props`, or
react_component component_name, `@props` and convert them to the options-style
form to avoid both hard failures and silent prop loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 956247c1-f4ef-4b9e-b967-59d2349825b3
📒 Files selected for processing (2)
docs/oss/migrating/migrating-from-react-rails.mddocs/oss/migrating/migrating-from-vite-rails.md
Review: docs — legacy Webpacker and migration-fit guidanceSummaryDocumentation-only PR that adds self-classification guidance to both migration guides so teams pick a realistic first migration target. Addresses a real gap: teams were hitting Webpacker incompatibility and concluding React on Rails was broken. Strengths
Issues (flagged inline)
Minor
Overall this is a clear improvement to the migration docs. The two inline suggestions are optional polish; the ambiguous "option 1" reference is the only one I'd call a real readability issue worth fixing before merge. |
Addresses review feedback on the Vite migration guide: - Name the "Keep the SPA shape" path explicitly in the follow-up paragraph so it does not read as ambiguous "option 1" (which could be misread as the first top-level bullet). - Move the require.context mode-argument breakdown out of the orientation section and into step 5 "Replace Vite-specific asset and env usage", which is the natural home for Vite-to-Webpack API mapping. - Add a concrete Popmenu example (110grill.com) in both migration guides so readers can see the single-top-level-component pattern in the wild. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This is a solid docs-only improvement that directly addresses the confusion described in #3138. The staged migration guidance (Webpacker → Shakapacker as its own PR, then React on Rails) is exactly the right mental model, and the self-classification sections are practical and actionable. Strengths
Issues
Minor: The SPA-shell bullet in |
Addresses PR #3157 review feedback from @claude[bot]: - react-rails.md: split the long "Client-routed SPA shells" bullet into skimmable sub-bullets (mount strategy, Popmenu callout, separate-islands note). - vite-rails.md: add a concrete before/after JS snippet showing import.meta.glob vs require.context so readers don't have to infer the API shape from prose. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review checkpoint — follow-up feedback addressed in 9f36b14Addressed by I (Claude Code) on behalf of @justin808: @claude[bot] (inline):
Already addressed on prior pass (baf4355):
All four unresolved threads are now resolved. |
Review: docs: legacy Webpacker and migration-fit guidanceOverall this is a well-targeted, accurate documentation improvement. The classification sections ("Pick the right first target" / "Two different starting points") directly address the problem from issue #3138 and the writing is clear. A few technical notes below. Verified accurate
Issues worth addressing
The before/after code block has two silent gotchas that are likely to trip up a reader doing a real migration:
These are worth calling out even briefly — see inline comment for a suggested diff. Shakapacker v6 upgrade guide link pins to
Minor observations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f36b14c66
ℹ️ 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".
Address PR #3157 review feedback: - migrating-from-react-rails.md: rephrase "does not install alongside `gem \"webpacker\"`" — the install generator does not enforce a hard install-time block; the actual constraint is unsupported configuration surfaced via `react_on_rails doctor`. - migrating-from-vite-rails.md: expand the import.meta.glob → require.context example with the key-path-format mismatch (relative to caller vs context dir) and the sync-vs-async distinction (lazy import.meta.glob vs synchronous ctx(key)). Adds an explicit lazy-equivalent recipe using dynamic import(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review checkpoint — follow-up feedback addressed in 6a86348Addressed by I (Claude Code) on behalf of @justin808: @chatgpt-codex-connector[bot] (P2, inline
@claude[bot] (inline
Rationale for the remaining items in PR review 4275625044 (not changed):
Resolving the two inline threads. |
|
To use Codex here, create an environment for this repo. |
Code Review: docs: legacy Webpacker and migration-fit guidanceOverviewDocumentation-only PR that adds self-classification guidance to both migration docs so teams can identify the correct first migration target before starting. The core content — Webpacker-era staged path, SPA vs. island distinction, Strengths
Issues Found1. The bullet describing mode values ( 2. Lazy
3. Duplicate Popmenu production reference ( The exact same sentence about Popmenu appears in both files verbatim. If the deployment details change — URL, company name, example sites — there are two places to update. A shared callout or a single canonical paragraph with a cross-link from the other file would reduce maintenance risk. Inline comment filed. Minor / Non-blocking
SummaryThe additions are well-motivated and the staged Webpacker path guidance is clearly correct. The three issues above are real — particularly the lazy module key-path mismatch, which will silently break code that copies the example verbatim — but they're all fixable with small targeted edits. The PR is close to merge-ready once those are addressed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a863487d9
ℹ️ 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".
Resolves merge conflicts in docs/oss/migrating/migrating-from-react-rails.md
by keeping both the app-shape classification ("Pick the right first target"
plus "Preferred path for Webpacker-era apps" from this branch) and the
"Choose a first slice" section and Example Migrations link from main.
Also drops the Popmenu / 110grill.com reference from the react-rails guide:
Popmenu does not migrate from react-rails, so the name-drop was off-topic
in that document.
Addresses unresolved review feedback on the lazy require.context example
in docs/oss/migrating/migrating-from-vite-rails.md:
- Rewrites the lazy example to use require.context with mode: 'lazy' so
ctx(key) returns Promise<Module>, removing the conflation between the
mode argument (discussed in text) and webpackMode magic comments (used
in the prior example). Fixes the factually wrong claim that
require.context is "not a direct replacement" for default-lazy Vite.
- Updates the "Sync vs async" bullet to explicitly point readers to the
lazy example for async semantics via the 4th 'lazy' argument.
Closes review comments discussion_r3127921272, discussion_r3127921792,
and discussion_r3127929295. discussion_r3127921912 (Popmenu duplicate)
addressed by removing the reference from the react-rails guide entirely.
Review checkpoint — follow-up feedback addressed in cd947b5Addressed by I (Claude Code) on behalf of @justin808: @claude[bot] (inline):
Merge conflicts resolved: Merged latest All four previously-unresolved review threads are now resolved. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd947b5bf8
ℹ️ 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".
Review: docs — legacy Webpacker and migration-fit guidanceOverall: Excellent addition. The "self-classify before you start" framing directly addresses the failure mode described in #3138, and the staged PR recommendation for Webpacker apps is exactly the kind of opinionated guidance this doc has been missing. Technical accuracy ✅The
One substantive concern — key-path mismatch not shown in the lazy exampleThe eager example has an explicit code comment flagging the key format difference: // ctx.keys() → ['./foo.js', ...] ← keys relative to context dir, NOT './dir/foo.js'But the lazy example omits a parallel comment. Readers who focus only on the lazy pattern (which is the default Vite shape and therefore the most common migration case) won't see the warning unless they also read the eager example and bullet point 1 above it. See the inline comment below for a suggested addition. Minor nits
Structure and cross-references ✅All anchor links for the new sections ( Test / validation ✅Prettier + lychee link checks passing per the PR description — no further CI concerns for a docs-only change. Approachable fix for a real user pain point. The one inline suggestion is the only change I'd consider before merging. |
|
Address-review checkpoint for PR #3157. Scan window: selected follow-up from user triage after reply batch Mattered
Optional
Skipped
Validation
Future full-PR scans should start after this comment unless |
|
Address-review checkpoint for PR #3157. Scan window: reran the full-PR scan after the prior checkpoint at Mattered
Optional
Skipped
Validation
Future full-PR scans should start after this comment unless |
1 participant
Summary
Adds explicit first-target guidance to the
react-railsandvite_railsmigration guides so teams pick a realistic migration slice and don't conclude "React on Rails is broken" when the real problem is legacy bundler compatibility.What changed
docs/oss/migrating/migrating-from-react-rails.mdwebpacker→shakapackeras a separate PR before addingreact_on_rails, and spells out the compatibility shims needed if a team intentionally stays on Webpack 4.docs/oss/migrating/migrating-from-vite-rails.mdWhy
From the issue: on apps like
indentlabs/notebook, the current docs don't make it obvious that legacy bundler compatibility — not React on Rails itself — is the main friction. Similarly, SPA-root Vite apps look like easy targets but are architectural rewrites. These additions let readers self-classify before they start.Validation
pnpm exec prettier --checkon both changed docs files passesFixes #3138.
🤖 Generated with Claude Code
Note
Low Risk
Low risk: documentation-only updates that don’t affect runtime behavior, but they may change user guidance around supported bundler paths and recommended upgrade sequencing.
Overview
Adds upfront decision guidance to the
react-railsandvite_railsmigration docs so teams choose a realistic first migration slice.For
migrating-from-react-rails.md, it now explicitly classifies app shapes (island mounts vs Webpacker-era vs SPA shells), adds a preferred staged path for Webpacker-era apps (migratewebpacker→shakapackerfirst, then install React on Rails, with version-pin guidance), and updates existing preflight/checklists to point to this guidance.For
migrating-from-vite-rails.md, it distinguishes island-mount vs SPA-shell starting points and expands the Vite-specific replacement section with concrete guidance for migratingimport.meta.globusage to Webpack’srequire.context(including sync vs lazy semantics).Reviewed by Cursor Bugbot for commit 7fde5c2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit