Polish RSC generator install follow-ups#3664
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCentralizes package-manager-aware command generation for generator warnings/rescue paths, consolidates RSC pin messaging, and adds stale-inactive RSC plugin detection and client-reference normalization with matching spec updates. ChangesRSC Generator Package-Manager Awareness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 polishes the RSC generator install flow with three focused changes: package-manager-aware manual recovery commands (replacing hardcoded
Confidence Score: 4/5Changes are confined to generator messaging, dependency-install logic, and RSC config heuristics; no runtime app code or auth paths are touched, making this safe to merge. The only notable issue is that when an RSC version-pin install fails, the user receives two consecutive warnings that both print the same manual install command. A separate test for rspack-specific RSC pin failure now exercises the same unified code path as the non-rspack test, which could mislead future maintainers. The Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review — Polish RSC generator install follow-upsOverviewThis PR does four things well:
The overall direction is solid and the new test coverage is good (package-manager variants, comment/string exclusion, mixed-plugin detection). Issues1. Stale TODO comment —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review: Polish RSC generator install follow-upsOverviewThis PR makes three related improvements to the RSC generator:
Overall this is clean, well-motivated work. The test coverage is solid. One UX nit and one style observation below. IssuesDuplicate manual install command in failure outputWhen the RSC package install fails and
The pin-failed warning is complete instructions on its own. The generic warning after it is redundant and will confuse users who see the same command recommended twice for the same failure. Simplest fix — guard the generic warning so it only fires when there is no pin-specific warning already covering the failure: GeneratorMessages.add_warning(rsc_dependency_pin_failed_warning) if used_version_pins
unless used_version_pins
GeneratorMessages.add_warning(<<~MSG.strip)
⚠️ Failed to add React Server Components dependencies.
You can install them manually by running:
#{manual_add_packages_command(manual_install_packages)}
MSG
endOr drop the trailing manual-command sentence from Minor: duplicated
|
justin808
force-pushed
the
jg-codex/follow-up-3640-3646-rsc-generator
branch
from
ca43f11 to
57d03d8
Compare
Code ReviewOverall: Clean, well-scoped follow-up. Generalizing hardcoded Strengths
IssuesMinor — unnecessary
|
justin808
force-pushed
the
jg-codex/follow-up-3640-3646-rsc-generator
branch
from
57d03d8 to
bf167dd
Compare
justin808
force-pushed
the
jg-codex/follow-up-3640-3646-rsc-generator
branch
from
bf167dd to
53627c9
Compare
|
Update pushed in What changed:
Validation:
CI restarted for the rebased branch and is currently queued/pending. |
Code ReviewOverviewSolid cleanup PR that addresses two gaps from the previous RSC generator work (#3640, #3646):
What's good
Minor issue (see inline)Both Nit
VerdictApprove with minor fix — the hardcoded |
Code Review: Polish RSC generator install follow-upsThis is a well-structured PR with clear goals. The changes are low-risk (generator/install-time only), test coverage is solid, and the simplifications are sensible. A few observations below. What this does
Strengths
Issues and Observations1.
|
Code ReviewOverviewThis PR polishes the RSC generator install experience in three focused areas: (1) package-manager-aware manual recovery commands replacing hardcoded npm; (2) simplified RSC install logic that drops the unversioned latest fallback; (3) improved inactive-plugin detection that ignores comments and strings. All three are well-scoped and low-risk. StrengthsPackage manager awareness - replacing 14+ hardcoded npm install / npm uninstall strings with manual_add_packages_command / manual_remove_packages_command helpers is clean and correct. The helpers delegate to fallback_package_manager which already defaults to npm for unrecognized managers, so package_manager_commands.fetch(package_manager) cannot raise in practice. RSC install simplification - dropping the Webpack/Rspack branch and the unversioned latest retry simplifies add_rsc_dependencies significantly. The reasoning (both bundlers need the same pin until 19.0.5 is stable) is sound and clearly documented in the PR and in rsc_dependency_pin_info. Regex comment fix - the old comment on rsc_plugin_commonjs_import_regex said the regex was single-line-only; the new comment is accurate. [^}]* in Ruby does match newlines (only . skips them by default), so the regex has always handled multiline destructuring. Now the comment says so. Inactive-plugin detection - inactive_rsc_plugin_symbol_in_js_code? is the right guard for the stale-symbol report. Using js_code_position? (not js_top_level_position?) for the new InactivePlugin(...) invocation check is intentional and correct: plugin instantiation can appear inside callback functions, not just at module scope. Test coverage - new specs directly exercise the three changed behaviours (pnpm/yarn command strings, pin-failure output, comment/string ignoring). The renaming of the old test to keeps-the-pin correctly reflects the new intent. Issues and observations1. Triple-message verbosity on install failure In add_rsc_dependencies, rsc_dependency_pin_info is emitted before the install attempt (line 413). On failure the user sees three messages: the pre-install info, the pin-failed warning, then the main failure warning. The info message was designed for the success path; showing it before a failed install is confusing. Consider emitting it only on success, inside the success branch of add_packages(rsc_packages). 2. --save-exact with range specifiers in recovery commands manual_add_packages_command always delegates to build_install_args, which adds --save-exact. Some recovery messages pass range-specified packages, e.g. react@~19.0.4 and react-dom@~19.0.4 from warn_about_react_version_for_rsc. Running npm install --save-exact react@~19.0.4 resolves the range and pins the exact resolved version to package.json, discarding the intended range. For RSC pinned packages this is correct; for advisory minimum-version messages it is at best surprising. This is a pre-existing pattern, but the PR makes it more visible. 3. Hardcoded 19.0.5 in rsc_dependency_pin_info The stable target version is mentioned literally in the string rather than via a constant. RSC_PACKAGE_VERSION_PIN already holds the prerelease string. If 19.0.5 ships under a different tag this message will be stale. Consider a constant (e.g. RSC_STABLE_PACKAGE_VERSION) or an explicit TODO comment. Minor, but the message is user-visible. 4. Pin-failed warning emitted for all errors in the rescue path In the rescue block, rsc_dependency_pin_failed_warning is added unconditionally (when used_version_pins) even for failures unrelated to the pin (e.g. a network timeout). This can mislead users into thinking the pin is the root cause. Folding the pin note into the main error message, or making the warning conditional on absence of an explicit exception message, would reduce noise. SummaryThe core changes are correct, well-tested, and a genuine improvement. The main suggestion worth acting on before merge is the info-message ordering (issue 1). Items 2-4 are cosmetic and can be addressed in a follow-up. |
Code Review: Polish RSC generator install follow-upsOverall: Solid cleanup. The package-manager-aware messaging is a meaningful improvement for yarn/pnpm/bun users, dropping the A few items worth discussing: Verbosity on install failureWhen
Messages 2 and 3 could be consolidated — the pin failure warning's "left the version pin in package.json" sentence and the manual install command belong in the same block. As-is, some UIs that display generator messages sequentially will show the explanation separated from the remediation by an unrelated message.
|
Code Review — Polish RSC Generator Install Follow-upsOverall assessment: Solid, well-motivated cleanup. The package-manager-aware command helpers eliminate a whole class of "why does it tell me to run npm when I'm using pnpm?" UX bugs, and the RSC retry-removal simplifies a subtle, hard-to-test code path. Test coverage is comprehensive. A few things worth looking at below. Findings
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code ReviewOverall this is a clean, well-scoped follow-up. The package-manager-aware commands, unified RSC failure messaging, and improved inactive-plugin detection (real JS only, ignoring comments/strings) are all solid. Tests are comprehensive and the logic changes are covered. A few things worth addressing: Issues1. Non-obvious rescue fallback in rsc_packages = RSC_DEPENDENCIES
used_version_pins = false
say "Installing React Server Components dependencies..."
rsc_packages, used_version_pins = rsc_packages_with_versionThe first two lines look like dead code — they're immediately overwritten on the happy path. In fact they are the rescue-block fallbacks: if 2. Indentation inconsistency in " #{manual_add_packages_command(rsc_packages)}"Other warning messages in the same file indent the recovery command with 4–12 spaces inside a Additionally, when All the other 3. package_version.start_with?("~", "^")Only Nits
Overall: Low-risk, well-tested, and a meaningful UX improvement. The rescue-fallback issue (#1) is the only thing I'd want addressed before merge — it's a correctness-silent footgun that could lose the fallback behaviour in a future refactor. |
Pro Node Renderer Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 1/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 2/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
1 participant
Summary
--rscinstall usesreact-on-rails-rsc@19.0.5-rc.6until stable19.0.5is published and tagged latest.Design Calls
--rscinstalls keep the prereleasereact-on-rails-rscpin for now, including Webpack projects. The generator no longer retries an unversionedlatestfallback because npm latest may not include Rspack support yet.new InactivePlugin(...)invocations, alongside the active plugin.normalize_rsc_plugin_import_for_active_bundlernow derives inactive plugin class and import path from the module-level helper methods instead of caller-supplied parameters.Tests
git diff --checkbundle exec rspec spec/react_on_rails/generators/install_generator_spec.rb:2000 spec/react_on_rails/generators/rsc_generator_spec.rb:2559bundle exec rspec spec/react_on_rails/generators/js_dependency_manager_spec.rb spec/react_on_rails/generators/generator_helper_spec.rbbundle exec rspec spec/react_on_rails/generators/install_generator_spec.rb:2013 spec/react_on_rails/generators/install_generator_spec.rb:2025bundle exec rspec spec/react_on_rails/generators/rsc_generator_spec.rb:2581 spec/react_on_rails/generators/rsc_generator_spec.rb:2604bundle exec rspec spec/react_on_rails/generators/rsc_generator_spec.rb:1114bundle exec rubocop lib/generators/react_on_rails/generator_helper.rb lib/generators/react_on_rails/js_dependency_manager.rb lib/generators/react_on_rails/rsc_setup.rb lib/generators/react_on_rails/rsc_setup/client_references.rb spec/react_on_rails/generators/generator_helper_spec.rb spec/react_on_rails/generators/install_generator_spec.rb spec/react_on_rails/generators/js_dependency_manager_spec.rb spec/react_on_rails/generators/rsc_generator_spec.rbCloses #3640
Closes #3646
Note
Low Risk
Changes are limited to install-time generators and warning text; the main behavioral shift is refusing an unversioned RSC fallback, which is intentional and covered by updated specs.
Overview
Generator install UX now surfaces package-manager-aware manual recovery commands (npm, yarn, pnpm, bun) for add/remove/install failures instead of always showing
npm install. Fallback installs drop--save-exact/--exactwhen specs use semver ranges (~,^) so manual and CLI recovery match real constraints.RSC JS dependencies: every
--rscrun logs whyreact-on-rails-rscis pinned to the prerelease (19.0.5-rc.6). On install failure the generator keeps the pin and stops retrying unversionedlatest(avoids replacing a good pin with an incompatible package). Pin-failure copy is unified viarsc_dependency_failure_messageand applies to Webpack and Rspack.RSC webpack migration/verification: inactive bundler plugin detection uses real JS (CommonJS/ESM imports and
new InactivePlugin(...)) and ignores comments/strings. Config checks can report stale inactive plugins when the active plugin is already present. Import normalization no longer takes inactive class/path from callers; multiline CommonJSrequiredestructuring is documented as supported.Reviewed by Cursor Bugbot for commit f17926a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests