Reduce Rspack RSC dev-server manifest footgun#4234
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
WalkthroughRSC generator docs and specs now resolve ChangesRSC generator and Rspack diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 reduces an Rspack RSC dev-server manifest footgun. The main changes are:
Confidence Score: 4/5The RSC doctor diagnostic can report success for configs that still leave the RSC client bundle lazy.
react_on_rails/lib/react_on_rails/doctor.rb Important Files Changed
Reviews (1): Last reviewed commit: "Reduce Rspack RSC dev-server manifest fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
+ci-run-hosted |
github-actions
Bot
added
the
ready-for-hosted-ci
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
Review: Reduce Rspack RSC dev-server manifest footgunOverall this is a solid, well-scoped follow-up. The two core code changes are correct:
Three things worth addressing before merge: 1. Regex can false-positive on commented-out code (doctor.rb, lines 3525–3528)
2. Success message is overconfident (doctor.rb, line 3636)
3. Missing test for the "dev config not found" branch (doctor_spec.rb, lines 5289–5299)The Everything else — changelog, USAGE, docs, generator test updates — looks good and is consistent with the changes. |
Review — Reduce Rspack RSC dev-server manifest footgunThis PR makes three targeted improvements: it replaces the fragile filesystem probe in Two confirmed issues with the new regex in 1. Regex produces false success on commented-out guards
2. Regex matches the wrong config variable The first alternative ( A minimal fix addressing both: strip comment lines and pin the LHS to RSC_RSPACK_LAZY_COMPILATION_DISABLED_PATTERN = /
^[^\/\n]*\bclientWebpackConfig\.lazyCompilation\s*=\s*false\b |
^[^\/\n]*\blazyCompilation\s*:\s*false\b
/x3. Silent skip when When |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@react_on_rails/lib/react_on_rails/doctor.rb`:
- Around line 3525-3528: The lazyCompilation detector in the doctor regex is
matching commented-out examples, so it can incorrectly report success from
non-executable text. Update the logic around
RSC_RSPACK_LAZY_COMPILATION_DISABLED_PATTERN so it ignores comments before
testing, or replace the raw-text regex with a parser-aware check that only
recognizes actual code assignments in the relevant doctor check.
- Around line 3624-3646: The Rspack lazy-compilation check in
check_rsc_rspack_lazy_compilation is hardcoded to the default config/rspack
path, so it can miss apps using a custom assets_bundler_config_path. Update the
check to resolve both the RSC webpack config and the development config from the
active bundler directory/path used by active_assets_bundler instead of fixed
config/rspack filenames. Keep the existing warning/success behavior, but ensure
File.exist? and File.read operate on the bundler-relative paths so the check
works for non-default layouts.
🪄 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: 84c3f1f1-2839-47d5-bdbe-89428c8e58ea
📒 Files selected for processing (11)
CHANGELOG.mddocs/oss/api-reference/generator-details.mddocs/oss/migrating/rsc-troubleshooting.mddocs/pro/installation.mdreact_on_rails/lib/generators/react_on_rails/rsc/USAGEreact_on_rails/lib/generators/react_on_rails/rsc_setup/client_references.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.ttreact_on_rails/lib/react_on_rails/doctor.rbreact_on_rails/spec/lib/react_on_rails/doctor_spec.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
There was a problem hiding this comment.
♻️ Duplicate comments (2)
react_on_rails/lib/react_on_rails/doctor.rb (2)
3624-3628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the Rspack files from the active bundler config directory.
This still hardcodes
config/rspack/rscWebpackConfig.jsandconfig/rspack/development.js, so apps using a non-defaultassets_bundler_config_pathwill silently skip the warning even though the same empty-manifest footgun applies. Derive both paths from the active bundler directory instead of assuming the default layout. Based on the PR objectives and stack context, RSC config discovery in this change set is meant to be bundler-relative.🤖 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 `@react_on_rails/lib/react_on_rails/doctor.rb` around lines 3624 - 3628, The Rspack/RSC config check in doctor.rb is still tied to hardcoded config/rspack paths, so it misses apps using a custom assets_bundler_config_path. Update the RSC config discovery logic around the active_assets_bundler == "rspack" guard to derive both the RSC webpack config and development config paths from the active bundler config directory instead of fixed defaults, and keep the warning in checker.add_warning tied to those resolved paths.
3525-3526: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore comments before matching the lazyCompilation assignment.
This detector still runs on raw file text, so
// clientWebpackConfig.lazyCompilation = false(or a commented example in docs/config) will report success even though lazy compilation is still enabled. Strip comments first or make the check parser-aware before applying this pattern. Based on the existing review history, this is the same still-unresolved false-positive path previously flagged.🤖 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 `@react_on_rails/lib/react_on_rails/doctor.rb` around lines 3525 - 3526, The lazyCompilation detector still matches commented text, so the RSC_RSPACK_CLIENT_LAZY_COMPILATION_DISABLED_PATTERN in doctor.rb can report a false success on raw file text. Update the detection logic around this pattern to ignore comments first or make it parser-aware before evaluating clientWebpackConfig.lazyCompilation = false, so only real assignments are treated as disabling lazy compilation.
🤖 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.
Duplicate comments:
In `@react_on_rails/lib/react_on_rails/doctor.rb`:
- Around line 3624-3628: The Rspack/RSC config check in doctor.rb is still tied
to hardcoded config/rspack paths, so it misses apps using a custom
assets_bundler_config_path. Update the RSC config discovery logic around the
active_assets_bundler == "rspack" guard to derive both the RSC webpack config
and development config paths from the active bundler config directory instead of
fixed defaults, and keep the warning in checker.add_warning tied to those
resolved paths.
- Around line 3525-3526: The lazyCompilation detector still matches commented
text, so the RSC_RSPACK_CLIENT_LAZY_COMPILATION_DISABLED_PATTERN in doctor.rb
can report a false success on raw file text. Update the detection logic around
this pattern to ignore comments first or make it parser-aware before evaluating
clientWebpackConfig.lazyCompilation = false, so only real assignments are
treated as disabling lazy compilation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a462daa7-1fd3-4165-a191-fafa25beacee
📒 Files selected for processing (2)
react_on_rails/lib/react_on_rails/doctor.rbreact_on_rails/spec/lib/react_on_rails/doctor_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/spec/lib/react_on_rails/doctor_spec.rb
Code Review — PR #4234: Reduce Rspack RSC dev-server manifest footgunThis PR follows up #4227 by making Rspack RSC dev-server setup more discoverable: it fixes the client-reference discovery path to be Two confirmed issues with the new doctor check deserve attention, plus one gap in the migration guide. Finding 1 — Doctor reports false success when the fix line is commented out The regex Suggested fix: strip single-line JS comments before matching: Finding 2 — Pattern matches the assignment regardless of which branch it appears in The doctor hint says to add the line inside the Rspack A practical minimum: require Finding 3 — Troubleshooting guide omits the The new section instructs existing Rspack RSC users to update |
Address-review summaryScan scope: full PR history through 2026-06-27T08:13:34Z. Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
|
Code review findings for PR 4234 (posting in parts due to shell escaping constraints). |
|
Code Review: PR 4234 - Three findings
|
|
Addressed the in-scope items from #4234 (comment):
|
Address-review summaryScan scope: review activity after 2026-06-27T08:14:22Z. Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
Review: Reduce Rspack RSC dev-server manifest footgunThis PR is a solid follow-up to #4227. The generator fix (switching from a filesystem 1. Doctor regex accepts unconditional placement as success
The test fixture at line 5331 in 2. Parameter-based guard silently breaks for manually-added RSC configsThe old Edge case: a developer who manually drops No blocking issues. Both items are low-to-medium severity and well within the context of a diagnostic/DX improvement PR. |
Code ReviewThis PR follows up on #4227 with three related fixes: (1) switching the dev-server lazy-compilation guard from a filesystem Bug: Pro dummy
|
|
Replying to #4234 (comment): [auto-deferred] [auto-deferred] |
|
Code review posted as inline comments. Top-level summary: clean follow-up to 4227, core fixes are correct. Two actionable items flagged inline: (1) troubleshooting doc should lead with the unconditional fix since the conditional form requires ServerClientOrBoth.js to also be updated; (2) rsc_rspack_config_directory silently skips when no rspack config is found at default paths, an info message would help non-standard setups. One informational note on the regex pattern. |
Rebased on main and regenerated llms-full after CI flagged a stale generated file. Applied reviewer-requested doc fixes, all kept faithful to the official starter (shakacode/react-on-rails-starter-tanstack), whose files each carry a REFERENCE PATTERN marker matching the snippets: - Add an Install section (mirrors the sister TanStack Router guide). - Fix the "Mistake 5" cross-link to target the section anchor. - Show apiFetch's real getCsrfToken import and ApiFetchOptions type so the snippet stands alone (matches lib/apiFetch.ts). - Note that the initialData seed carries no initialDataUpdatedAt, so the table does one background refetch on mount; document how to opt out. llms-full-pro.txt picks up one inherited line from #4234 (RSC generator text) that landed on main without a pro-file regen; the new OSS-only page does not appear in the pro bundle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Follow-up to #4227 / #4226. The original fix disabled Rspack lazy compilation for generated RSC apps, but the behavior was still easy for existing apps to miss and one helper still assumed the RSC config lived under
config/webpack.This PR:
rscWebpackConfig.jsin the active config directoryreact_on_rails:doctor:rscdiagnostics for existing Rspack RSC apps whose dev config does not disable lazy compilationlazy-compilation-proxy/POST /_rspack/lazy/trigger404 troubleshooting pathValidation
cd react_on_rails && bundle exec rspec spec/react_on_rails/generators/install_generator_spec.rb -e 'disables Rspack lazy compilation while serving RSC apps' -e 'checks RSC discovery support in the generated Rspack config directory'cd react_on_rails && bundle exec rspec spec/react_on_rails/generators/rsc_generator_spec.rb -e 'when Pro is installed on an existing rspack project'cd react_on_rails && bundle exec rspec spec/react_on_rails/generators/rsc_generator_spec.rb -e 'when an existing client RSC webpack config already references the scoped helper'cd react_on_rails && bundle exec rspec spec/lib/react_on_rails/doctor_spec.rb -e 'check_rsc_rspack_lazy_compilation' -e 'warns when the Rspack RSC config supports manifest discovery and the manifest is missing'bundle exec rubocop react_on_rails/lib/react_on_rails/doctor.rb react_on_rails/lib/generators/react_on_rails/rsc_setup/client_references.rb react_on_rails/spec/lib/react_on_rails/doctor_spec.rb react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rbpnpm start format.listDifferentgit diff --check origin/main...HEADscript/ci-changes-detector origin/main/Users/justin/.agents/skills/autoreview/scripts/autoreview --mode localclean before pushSummary by CodeRabbit
Bug Fixes
Documentation
Tests