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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughUpdated Rspack package version pins from v1 to v2 prerelease ranges across the generator, bundler switcher, tests, and changelog: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR upgrades Rspack dependencies from v1 to v2 across the generator ( One minor observation: both the new "Changed" entry in the changelog (this PR) and the existing "Fixed" entry from PR 3083 both state they close Issue 3082, which is a slight duplication since an issue can only be closed once on GitHub. Confidence Score: 5/5Safe to merge — all changes are targeted version string updates with matching spec and changelog coverage. No P0 or P1 findings. All remaining observations are P2: the duplicate "Closes Issue 3082" reference in the changelog is cosmetic and does not affect runtime behavior. The version bump logic, constant definitions, and test assertions are consistent across all changed files. No files require special attention.
|
| Filename | Overview |
|---|---|
| react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb | Version strings bumped from ^1.0.0 to ^2.0.0-0 for @rspack/core and @rspack/cli, and to ^2.0.0 for @rspack/plugin-react-refresh; explanatory comments added inline. |
| react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler | RSPACK_DEPS constants updated to match the same v2 version strings as js_dependency_manager.rb. |
| react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb | Spec constant assertions updated to expect new v2 version strings; no logic changes. |
| react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb | Single string assertion updated from @rspack/core@^1.0.0 to @rspack/core@^2.0.0-0 to match the new switch-bundler template content. |
| CHANGELOG.md | New "Changed" entry added; both this entry and the existing "Fixed" entry from PR 3083 reference closing Issue 3082, creating a slight duplication. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Generator / bin/switch-bundler invoked with --rspack] --> B{Rspack flag set?}
B -- Yes --> C[Install RSPACK_DEPENDENCIES\n@rspack/core@^2.0.0-0\nrspack-manifest-plugin@^5.0.0]
B -- Yes --> D[Install RSPACK_DEV_DEPENDENCIES\n@rspack/cli@^2.0.0-0\n@rspack/plugin-react-refresh@^2.0.0\nreact-refresh]
B -- No --> E[Install Webpack dependencies]
C --> F[Run package manager install]
D --> F
E --> F
Reviews (1): Last reviewed commit: "feat: bump rspack dependencies to v2 (^2..." | Re-trigger Greptile
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
Show resolved
Hide resolved
ReviewThe changes are minimal, targeted, and correct. The version bumps in both Two items to address:
Minor observation (not a blocker):
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 29-30: Update the CHANGELOG entry titled "Bump Rspack dependencies
to v2" to include the required PR attribution footer in the exact format: [PR
NUMBER](url) by [username](url) (no leading #), appended to the end of that
entry; ensure you use the actual PR number and contributor GitHub username/URLs
and preserve any existing inline **[Pro]** prefix if applicable.
🪄 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: 7c9e6b23-e039-4c2b-8394-e66c0cc2b523
📒 Files selected for processing (5)
CHANGELOG.mdreact_on_rails/lib/generators/react_on_rails/js_dependency_manager.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundlerreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb
Review Address SummaryAddressed review feedback in 16e3c56: Fixed (2):
Skipped with rationale (1):
All 3 review threads replied to and resolved. |
Review: Bump Rspack dependencies to v2Overall this PR is clean, well-scoped, and correctly addresses the Rspack v2 upgrade. The use of the `-0` prerelease suffix is the right technique to pull in `2.0.0-rc.1` builds of `@rspack/core` and `@rspack/cli` until stable 2.0.0 ships. Asymmetric version specs — looks intentional, worth a commentThe three packages are treated differently:
This is correct, but the asymmetry isn't explained in the code. A future contributor could see `@rspack/cli@^2.0.0-0` and `@rspack/plugin-react-refresh@^2.0.0` side-by-side and wonder if the missing `-0` is a bug. The inline comments on `js_dependency_manager.rb` partly address this, but `switch-bundler` has no explanation at all. Suggestion: add a brief comment (see inline) so the intent is preserved when someone revisits this after 2.0.0 stable ships. Minor: CHANGELOG — dual "Closes" cleanupThe change from "Closes" to "See" on the PR 3083 CHANGELOG entry is the right call — only one PR should own the close. Looks good. No behaviour or security concernsAll changes are version strings in frozen constants; no runtime logic or security surface is touched. Tests are updated in lockstep. Approachable change — just the one documentation nit noted inline. |
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
Show resolved
Hide resolved
...s/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt
Outdated
Show resolved
Hide resolved
Code ReviewOverall this is a clean and well-scoped change. Three items worth addressing before merge: Issues found1. CHANGELOG attribution (must fix) 2. 3. Other anonymous default exports (quick audit) What looks good
|
size-limit report 📦
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
33-33:⚠️ Potential issue | 🟡 MinorCorrect the PR 3084 contributor attribution in this entry.
Line 33 attributes PR 3084 to
I (Claude Code), but this PR is authored byjustin808. Please update the byline to the actual contributor.As per coding guidelines: “Format CHANGELOG entries as `[PR NUMBER](url) by [username](url)` without hash before PR number.”✏️ Suggested fix
-- **Fix SSR crash with webpack 5.106.0**: Changed server webpack config devtool from `eval` to `cheap-module-source-map` to work around a webpack 5.106.0 regression where anonymous default exports from ESM packages cause `ReferenceError: __WEBPACK_DEFAULT_EXPORT__ is not defined` during server-side rendering. Also converted `scriptSanitizedVal` from anonymous to named default export to prevent the issue in the npm package. [PR 3084](https://github.com/shakacode/react_on_rails/pull/3084) by [I (Claude Code)](https://claude.ai/code). +- **Fix SSR crash with webpack 5.106.0**: Changed server webpack config devtool from `eval` to `cheap-module-source-map` to work around a webpack 5.106.0 regression where anonymous default exports from ESM packages cause `ReferenceError: __WEBPACK_DEFAULT_EXPORT__ is not defined` during server-side rendering. Also converted `scriptSanitizedVal` from anonymous to named default export to prevent the issue in the npm package. [PR 3084](https://github.com/shakacode/react_on_rails/pull/3084) by [justin808](https://github.com/justin808).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 33, Update the CHANGELOG entry for PR 3084 to correct the contributor attribution: replace the current byline "I (Claude Code)" with the actual GitHub username "justin808" and ensure the entry follows the required format `[PR 3084](https://github.com/shakacode/react_on_rails/pull/3084) by [justin808](https://github.com/justin808)`, keeping the rest of the text (the change description and links) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CHANGELOG.md`:
- Line 33: Update the CHANGELOG entry for PR 3084 to correct the contributor
attribution: replace the current byline "I (Claude Code)" with the actual GitHub
username "justin808" and ensure the entry follows the required format `[PR
3084](https://github.com/shakacode/react_on_rails/pull/3084) by
[justin808](https://github.com/justin808)`, keeping the rest of the text (the
change description and links) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e38fadab-65b8-4980-9d4e-0261160a6fd7
📒 Files selected for processing (7)
CHANGELOG.mdpackages/react-on-rails/src/scriptSanitizedVal.tsreact_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.ttreact_on_rails/spec/dummy/config/webpack/serverWebpackConfig.jsreact_on_rails/spec/react_on_rails/support/generator_spec_helper.rbreact_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.jsreact_on_rails_pro/spec/execjs-compatible-dummy/config/webpack/serverWebpackConfig.js
✅ Files skipped from review due to trivial changes (2)
- react_on_rails/spec/react_on_rails/support/generator_spec_helper.rb
- packages/react-on-rails/src/scriptSanitizedVal.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6ddb7d. Configure here.
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
Show resolved
Hide resolved
ReviewOverall this is a clean, well-commented PR. Two issues and one observation: Bug:
|
|
WIP — picking this up. |
Update generator and bin/switch-bundler Rspack dependencies from v1 to v2: - @rspack/core@^2.0.0-0 (prerelease suffix includes RC builds) - @rspack/cli@^2.0.0-0 - @rspack/plugin-react-refresh@^2.0.0 (already stable at 2.0.0) - rspack-manifest-plugin@^5.0.0 (unchanged, already compatible) The -0 prerelease suffix ensures that RC builds (currently 2.0.0-rc.1) are resolved by npm until stable 2.0.0 ships. Closes #3082 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ihabadham
force-pushed
the
jg/3082-bump-rspack-v2
branch
from
b6ddb7d to
acad445
Compare
Review: Bump Rspack dependencies to v2The change itself is clean and correct. A few notes: What looks good
Minor observation: stale PR descriptionThe PR description (and CodeRabbit walkthrough) mentions changes that are not in the actual diff — specifically:
Those look like they were part of an earlier iteration and were either moved to a separate PR or reverted before this was pushed. The description should be trimmed to match the actual five-file diff to avoid reviewer confusion (especially for someone landing on this PR without git context). One inline suggestionLeft a comment on Overall: LGTM once the description is cleaned up. The version bump is safe, well-documented, and the changed lines are exactly what you'd expect. |
![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: acad445b92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The bin/switch-bundler version-pin test only spot-checked @rspack/core and webpack. A drift in @rspack/cli or @rspack/plugin-react-refresh within the RSPACK_DEPS constant would silently pass the existing assertions. Add assertions for the remaining two rspack pins so future version changes to bin/switch-bundler cannot regress without a test failure. The sibling test "installs rspack dependencies in package.json" already checks all four rspack packages (for the generated package.json), so this also brings the switch-bundler test into consistency with the adjacent pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review: Bump Rspack dependencies to v2Overall: Clean, focused, well-documented PR. The version bump approach and test updates look correct. What's good
Pre-existing gap worth addressing
RSPACK_DEV_DEPENDENCIES = %w[
@rspack/cli@^2.0.0-0
@rspack/plugin-react-refresh@^2.0.0
react-refresh # ← explicit
].freezeBut RSPACK_DEPS = {
dev_dependencies: %w[@rspack/cli@^2.0.0-0 @rspack/plugin-react-refresh@^2.0.0]
# ^ no react-refresh
}This inconsistency predates this PR, but since the file is being touched it's a good moment to align them. Minor nitThe PR is still labelled work-in-progress — if CI is green and the change is ready, consider removing the label before merge. |
react_on_rails/lib/generators/react_on_rails/templates/base/base/bin/switch-bundler
Show resolved
Hide resolved
Address-review summaryScan scope: full PR history. The user directed me to investigate every review comment (including outdated ones marked as such by the force-push squash), so this run scanned all reviews regardless of cutoff timestamp — equivalent to PR head at triage: Mattered
SkippedPre-squash comments obsolete after the squash (5 threads, all received short rationale replies and were resolved):
Pre-squash comments declined as minor nits (2 threads, replied and resolved):
Post-push new comment (1 thread, replied and resolved):
Already resolved before this pass (3 threads, no action needed):
Bulk-skipped as status noise or historical pre-squash summaries (13 items):
Outcome
Next default scan starts after this comment. Say |
|
To use Codex here, create a Codex account and connect to github. |
|
The I'll kick off a fresh review of the current state (post-squash + 🧠 Learnings used✅ Actions performedReview triggered.
|
|
Thanks for the comprehensive summary. All the key points are addressed:
The PR is in good shape. No outstanding issues from my side. |
|
I dug this through to the canonical fix and verified it in real fresh Rails apps. Bottom lineThis PR's Rspack v2 changes work cleanly once Shakapacker is fixed upstream. The canonical fix is now in progress at: That PR now does both required upstream changes:
What I verified locallyUsing:
I ran a full fresh-app flow:
All of it succeeded. Final generated dependency state was:
ConclusionSo the clean path here is:
I do not think this PR should grow a |
2 participants
Summary
bin/switch-bundlerRspack dependencies from v1 to v2@rspack/core@^2.0.0-0and@rspack/cli@^2.0.0-0— the-0prerelease suffix ensures RC builds (currently2.0.0-rc.1) are included until stable 2.0.0 lands@rspack/plugin-react-refresh@^2.0.0— already stable at 2.0.0rspack-manifest-plugin@^5.0.0— unchanged, already compatible with Rspack v2Closes #3082
Test plan
js_dependency_manager_spec.rbpasses🤖 Generated with Claude Code
Summary by CodeRabbit