Pin @babel/preset-react to v7 in generator e2e template#1167
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 SummaryThis PR fixes a broken CI job caused by upstream version drift:
Confidence Score: 5/5A one-line dependency pin that unblocks CI with no logic or behavior changes to the Shakapacker runtime. The change is a targeted version constraint on a single test-template dependency. It mirrors a pin already present in spec/dummy, is consistent with the app's @babel/core ^7 baseline, and carries a clear explanatory comment. No runtime code is touched. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "Pin @babel/preset-react to v7 in generat..." | Re-trigger Greptile |
Code ReviewOverview: This is a minimal, targeted fix for a legitimate CI breakage caused by upstream dependency drift — What's good
Minor observationsComment verbosity: The three-line comment describes the failure scenario in detail. A tighter version conveys the same non-obvious constraint in one line and stays closer to the project's "no unnecessary comments" style: # Pin to v7: @babel/preset-react@8 requires @babel/core@^8, but this app uses ^7 (ERESOLVE).
package_json.manager.add(["react", "react-dom", "@babel/preset-react@^7"])That said, a pinning comment is warranted here — this is exactly the kind of hidden external constraint that would puzzle a future reader — so this is just a style nit. Slight inconsistency with
VerdictSolid, necessary fix. The approach is correct and the scope is appropriately tight. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Address-review summaryScan scope: full PR history; no previous address-review summary was present. Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
ReviewCorrect and necessary fix. The root cause analysis in the PR description is accurate: npm resolving an unpinned One minor consistency nit (see inline): the chosen floor Out-of-scope observation (pre-existing, not introduced here): |
ReviewThis PR bumps One issue worth fixing: the added comment doesn't match the diff. The PR description says the old code had no version constraint ( The functional change (bumping the minimum from 7.17.0 → 7.18.6) is correct and safe — it aligns with |
ReviewThis PR bumps 1. Root cause doesn't match the diffThe PR description says the original code was unpinned (
This is worth clarifying because if the actual root cause was something different, this fix may not prevent a recurrence. 2.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/generator_specs/e2e_template/template.rb (1)
35-35: 📐 Maintainability & Code Quality | 🔵 TrivialEnsure file ends with a trailing newline.
The file may be missing a trailing newline character. As per coding guidelines, all files must end with a trailing newline.
🤖 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 `@spec/generator_specs/e2e_template/template.rb` at line 35, The template file may be missing a trailing newline at the end of the file. Update the end of the template so it always terminates with a newline character, keeping the change within the e2e template content itself.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@spec/generator_specs/e2e_template/template.rb`:
- Line 35: The template file may be missing a trailing newline at the end of the
file. Update the end of the template so it always terminates with a newline
character, keeping the change within the e2e template content itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e673d29e-247f-4ccb-a0f0-9bdf7f141141
📒 Files selected for processing (1)
spec/generator_specs/e2e_template/template.rb
justin808
force-pushed
the
jg/fix-generator-babel-preset-react-v7
branch
from
31d71f5 to
f603c56
Compare
|
The fix is correct and the version alignment with `spec/dummy` is the right approach. A couple of notes: PR description inaccuracy: The description states that `@babel/preset-react` was installed "with no version constraint", showing `@babel/preset-react` without a pin. However, the actual diff shows the old code already had `@babel/preset-react@^7.17.0`. The real gap was that `react` and `react-dom` were unpinned (no version specifier at all), which allowed npm to resolve them to a version that conflicted with other deps. The description should be corrected so the team has an accurate record of the root cause. React 19 forward-compatibility: Pinning `^18.3.1` keeps the e2e template fixed on React 18. Once React 19 becomes the npm default, this template (and CI) won't exercise React 19 compatibility. That may be intentional for stability — just worth noting as a conscious trade-off since the template now needs a manual bump when the project is ready to validate React 19. Minor style nit (inline): the single-line array was reformatted to multi-line and a comment was added; both are extraneous relative to the version-pin task per the project's "keep changes focused and minimal" rule. |
|
The fix is correct and addresses the root cause: pinning One small nit on the added comment (see inline): it explains what was done rather than why the version was constrained, which is the part that would be non-obvious to a future reader. |
The generator specs' react e2e template installed @babel/preset-react with no version constraint. @babel/preset-react@8 was published requiring @babel/core ^8 as a peer, but Shakapacker's generated app uses @babel/core ^7. npm therefore resolved the unpinned add to v8 and failed with ERESOLVE, which aborted the whole `npm add` and left react and react-dom uninstalled. The subsequent bin/shakapacker build then failed with "Module not found: Can't resolve 'react'", breaking the Generator specs job on main and every PR. Pin to @babel/preset-react@^7 to match the app's @babel/core ^7, matching the existing pin already used in spec/dummy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg/fix-generator-babel-preset-react-v7
branch
from
2c5b03e to
a12f7e8
Compare
ReviewThe fix is correct and the version choices align with Root cause description is inaccurateThe PR description says The code change is still correct — the description just doesn't match the diff. Stale comment riskThe inline comment |
ReviewThis PR pins Minor observation on the PR description: The body says the root cause was that Unexplained minor tightening: No bugs, no removed invariants, no cross-file impact found. LGTM. |
Review: Pin @babel/preset-react to v7 in generator e2e templateThe fix is correct and the direction is right — aligning the e2e template's dependencies with `spec/dummy/package.json` (which already uses exactly these version ranges) is the right call, and this is test-only with no production impact. One thing worth clarifying: the PR description says the original file had `@babel/preset-react` with no version constraint, but the actual diff shows it already had `@babel/preset-react@^7.17.0` — a v7 pin that would have blocked v8 resolution entirely. The real fix that stops the CI failure is the new react/react-dom pins (`@^18.3.1`), which prevent npm from resolving those to React 19. The babel preset bump from `^7.17.0` → `^7.18.6` is a nice alignment with `spec/dummy` but wasn't the root cause of the breakage. Findingsspec/generator_specs/e2e_template/template.rb:8-9 — Comment describes a non-issue and omits the actual issue The comment "pin to v7 to match the generated app's @babel/core" implies the v7 pin is new, but the original already had Minor: comment style differs from the file's established pattern Every other comment in the file is a single-line section label ( Overall: approve with the comment nit addressed. The version pins themselves are correct and consistent with the rest of the test suite. |
ReviewThe fix is correct and well-targeted. The three pinned versions ( One minor note on the PR description: it states that On the added comments: the two comment lines explaining why versions are locked (react-dom 19 incompatibility, babel peer dep mismatch) are worth keeping. Without them, a future maintainer would likely see bare LGTM. |
## Problem `i18n` **1.15.0** (published 2026-06-18) calls `Fiber[:i18n_config]` (Fiber storage, Ruby 3.2+) but its gemspec declares `required_ruby_version >= 3.1`. So Bundler happily installs it on **Ruby 3.1**, where loading Active Support then crashes: ``` i18n-1.15.0/lib/i18n.rb:58:in `config': undefined method `[]' for Fiber:Class (NoMethodError) current = Fiber[:i18n_config] || self.config = I18n::Config.new ``` This turns **every Ruby 3.1 matrix job red** on `main` (the full matrix runs on pushes / `workflow_dispatch`): - **Generator specs** — `rails new` crashes before any app is created, failing all 77 examples and fail-fast-cancelling the rest of the matrix. - **Ruby based checks** — Active Support boot crashes the same way. Ruby 2.7/3.0 are unaffected (Bundler skips 1.15.0 via `required_ruby_version` and falls back to 1.14.x); Ruby 3.2+ has `Fiber[]` so 1.15.0 works there. ## Fix Pin `i18n < 1.15` **only for Ruby < 3.2**, mirroring the existing `concurrent-ruby` pin: - The affected gemfiles (`6.1`, `7.0`, `7.1`, `7.2` — the ones that run on Ruby 3.1) so the repo bundle's `rails`/Active Support load cleanly. - The generated base Rails app in the generator spec, so the app itself boots on Ruby 3.1. Ruby 3.2+ continues to use `i18n` 1.15.x. No production code or published-gem dependencies change — gemfiles and test setup only, so no CHANGELOG entry. ## Relationship to #1167 This is **stacked on #1167** ("Pin `@babel/preset-react` to v7"). That PR fixes a *separate* fresh upstream breakage (`@babel/preset-react@8` requires `@babel/core@^8` + Node ≥22.18) which is what makes the **PR minimal matrix** (`3.4/8.0`, `2.7/7.0`) red. This PR fixes the **full-matrix-only** Ruby 3.1 breakage. Merge #1167 first; GitHub will then retarget this PR to `main`. Verified via a full-matrix `workflow_dispatch` run on this branch (which includes Ruby 3.1). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > CI and test Gemfile pins only; no runtime or published gem dependency changes. > > **Overview** > **Unblocks Ruby 3.1 CI** by keeping Bundler from resolving `i18n` 1.15.0, which calls `Fiber[]` (Ruby 3.2+) and crashes Rails boot on 3.1. > > The Rails **6.1 / 7.0 / 7.1 / 7.2** gemfiles now include `gem "i18n", "< 1.15" if RUBY_VERSION < "3.2"`, following the same pattern as the existing `concurrent-ruby` pin. The generator spec appends the same constraint to the generated base app’s Gemfile when Ruby is below 3.2 so `rails new` and generator examples boot on 3.1. Ruby 3.2+ is unchanged and can still use `i18n` 1.15.x. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 84fffa6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved app generation reliability on older Ruby versions by avoiding problematic dependency releases that could prevent newly created apps from starting. * **Tests** * Updated the generator test setup to account for Ruby-version-specific behavior during app creation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Stamps the **`v10.2.0`** release section in `CHANGELOG.md` and adds the user-visible entries that were still missing for PRs merged since `v10.1.0`. Header format matches the repo convention (`## [v10.2.0] - July 3, 2026`) and is parseable by `rakelib/release.rake`'s `extract_changelog_section` (`## [v<npm-version>]`), so `bundle exec rake release` / `sync_github_release` will pick up the notes automatically. ### Changelog changes - **Version header**: inserted `## [v10.2.0] - July 3, 2026` immediately after `## [Unreleased]`; all accumulated entries now live under it, and `## [Unreleased]` is empty. - **Compare links**: `[unreleased]` now compares `v10.2.0...main`; added `[v10.2.0]: …/compare/v10.1.0...v10.2.0`. ### New entries added (were missing) | PR | Section | Note | | --- | --- | --- | | [#1187](#1187) | Added | Babel 8 peer dependency support + preset option compatibility | | [#1184](#1184) | Added | Folded into the #695 AI-prompt entry (gates the React on Rails section on app detection) | | [#1142](#1142) | Fixed | Rspack dev-server config no longer loads in static watch mode (fixes #1137) | ### Already documented (carried into v10.2.0) `#1180`, `#695`, `#1141`, `#1150`, `#1179`, `#1192`, `#1127`, `#1178`, `#1161`, `#1147`. ### Reviewed and intentionally excluded (not user-visible) Docs: `#1145`, `#1148`, `#1152`, `#1155`, `#1183`, `#1188`, `#1189`, `#1193`. CI: `#1151`, `#1168`, `#1171`. Tests / fixtures: `#1128`, `#1154`, `#1167`, `#1186`. Workflow / agent tooling: `#1153`, `#1176`, `#1182`. (`#1107` is already documented under `## [v10.1.0]`.) ## Next step After merge, run the repo's release task (no args) — it reads `v10.2.0` from the changelog and creates the GitHub release from this section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Problem
The Generator specs job is failing on
mainand every open PR (e.g. #1161), with the react e2e build erroring:Root cause
spec/generator_specs/e2e_template/template.rbinstalled@babel/preset-reactwith no version constraint:@babel/preset-react@8.0.1(nowlatest) requires@babel/core@^8as a peer, but Shakapacker's generated app uses@babel/core@^7. npm resolved the unpinned add to v8 and failed with:Because the
npm addaborted,react/react-domwere never installed, so the subsequentbin/shakapackerbuild couldn't resolvereact. This is purely upstream dependency drift — the same job passed days ago with no source change.Fix
Pin the template's preset to the v7 line, matching the app's
@babel/core@^7(and the existing@babel/preset-react@^7.18.6pin already used inspec/dummy):Verified via
npm install --dry-runagainst a@babel/core@^7.29.7app: the pin resolves to@babel/preset-react@7.29.7(peer@babel/core@^7.0.0-0) cleanly.🤖 Generated with Claude Code
Note
Low Risk
Test-only generator template change; no runtime or production dependency behavior.
Overview
Fixes Generator specs / React e2e failures caused by npm resolving an unpinned
@babel/preset-reactto v8, which peers on@babel/core@^8while generated apps stay on Babel 7 —npm addaborts andreact/react-domnever install.The e2e generator template now installs
@babel/preset-react@^7(aligned withspec/dummy) and documents the peer mismatch in a short comment.Reviewed by Cursor Bugbot for commit 3e94156. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit