refactor: clarify RSC migration parser and release safeguards#3343
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:
WalkthroughTargeted maintenance: clarify node-renderer sandboxing docs, make package.json publishing atomic, extract dependency-pin warning helper (with test), refine webpack warning test wording, and refine RSC clientReferences detection and scanner comments. No runtime behavior changes. ChangesMaintenance and refinement improvements across Node-renderer, release infra, generators, tests, and RSC setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 is a documentation and naming clarification pass over
Confidence Score: 5/5Safe to merge — the only functional change is a method rename that is fully applied with no dangling references anywhere in the repo. All changes are documentation and a method rename. No logic paths are altered, the renamed predicate's call site is updated, and a repo-wide grep confirms zero remaining uses of the old name. The expanded advance_js_scan_state comment and the clarified guard rationale in add_rsc_client_references_setup are accurate descriptions of the existing code behaviour. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rewritable_rsc_plugin?] -->|calls| B["any_rsc_plugin_missing_client_references?\nrenamed from rsc_plugin_without_client_references?"]
B --> C[rsc_plugin_option_sections]
C --> D{any section missing clientReferences?}
D -->|yes| E[return true - file is rewritable]
D -->|no| F[rsc_plugin_defines_client_references?]
F -->|all configured| G[warn + return false]
F -->|none found| H[warn_missing_rsc_plugin_target + return false]
C --> JSS
subgraph JSS [advance_js_scan_state - central dispatcher]
S1[line and block comments]
S2[single / double / template-literal strings]
S3[simple dollar-brace interpolations]
end
JSS -->|brace depth confused by regex or nested templates| I[rsc_plugin_option_sections_partition checks for trailing close-paren]
I -->|missing| J[mark section unparseable - warn user]
I -->|present| K[section is parseable]
Reviews (1): Last reviewed commit: "refactor: clarify RSC migration parser h..." | Re-trigger Greptile |
Code ReviewSummary: Pure documentation and refactoring PR — one method rename and comment/doc improvements in What's good
Issues1. Missing caller in the The docblock enumerates 6 callers but 2. Minor: 'short-circuit on In
'Short-circuit' conventionally describes boolean operators ( 3. Future-expansion note: point 2 understates the change scope
Stack-based nested template literals (point 2) would also require changes to Overall this is a clean, low-risk clarification PR. The only actionable issue is the incomplete caller list; the other two are wording nits. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
2 times, most recently
from
f952cb4 to
09137c8
Compare
Code ReviewOverviewThe PR title describes an RSC migration parser refactor, but this branch carries 12 commits across 6 independent concerns, each at a different risk level:
Security:
|
size-limit report 📦
|
Code ReviewOverviewThis PR is titled as a refactor of RSC migration parser helpers, but it is actually a mixed-scope PR containing at least five distinct categories of change:
The mixed scope makes individual rollback harder and obscures the security change in review. That said, the changes themselves are generally well-executed. Notes below by area. RSC parser refactor (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Mattered
Optional
Skipped
Verification
Scan window: full PR review history through 2026-05-24T08:48:00Z. Future full-PR scans should start after this comment unless |
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
from
738dd85 to
5c13228
Compare
Review: refactor: clarify RSC migration parser helpers (#3258)Overall: Approved with one minor nit. This is a clean documentation + rename PR with no behavioral changes. All four files look correct. Rename:
|
Code Review — PR #3343OverviewThis PR is broader than its title suggests — it combines several distinct changes:
Strengths
Issues1.
|
justin808
force-pushed
the
codex/fix-rsc-client-reference-scope
branch
from
4e2b5e1 to
d74e827
Compare
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
from
900c70a to
20886c9
Compare
…elper Expand the comment on `changed = true` to spell out the trade-off the current ordering accepts: if `File.write` partially writes before raising, the `ensure` block will not restore the original content. This is rare on local filesystems for small JSON files (effectively atomic single-buffer write) and acceptable for a release-time helper, but documenting it prevents a future reader from flipping the ordering back without understanding the implication. Addresses review feedback on PR #3343.
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
from
9200fad to
c7673a6
Compare
Code ReviewOverview: This PR is a follow-up cleanup — mostly comment/documentation improvements, an atomic write safeguard in release tooling, a method rename for clarity, and a new formatter spec. No runtime behavior changes except the safer package.json write path.
|
Follow-up to PR #3219 cloud review. Internal cleanup, no behavior change. - Rename `rsc_plugin_without_client_references?` to `any_rsc_plugin_missing_client_references?` and add a docstring spelling out the any-section existential semantics — the prior name read as universal but the implementation is existential. - Consolidate the lightweight JS scanner's supported-surface notes onto `advance_js_scan_state` (the central dispatcher): which lexical constructs are tracked, which fall outside (regex literals, nested template literals), how downstream `rsc_plugin_option_sections_partition` detects the unsafe cases and warns, and what real-world expansion would look like. Replace the scattered shorter notes at `rsc_plugin_options_without_comments` and `matching_js_closing_brace` with pointers to the consolidated doc. - Document the defensive guards in `add_rsc_client_references_setup`: what bad outcome they prevent (a duplicate `const rscClientReferences` declaration → `Identifier already declared` SyntaxError at config load) and why two boolean checks are worth keeping in case a future caller bypasses `ensure_rsc_client_references_setup`. Fixes #3258 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- client_references.rb: include `last_js_code_line_start` in the `advance_js_scan_state` caller list and scope the regex-literal / template-literal limitations to callers that do not run `js_regex_literal_start?` preprocessing, since `js_code_position?` and `js_top_level_position?` handle regex literals via their own pre-pass. - client_references.rb: switch `add_rsc_client_references_setup` early returns to explicit `return false` to match the boolean-return convention used in `ensure_rsc_client_references_setup`. - configBuilder.ts: start the second `SECURITY:` block on a new line so the tag is a clear visual anchor, matching the first block. - js_dependency_manager.rb: emit a `GeneratorMessages.add_warning` from `write_versioned_package_specs_to_package_json` so users learn when the fallback wrote version pins to `package.json` and what they need to install manually. - release.rake: in `with_publishable_package_json`, only flip `changed` to true after `File.write` succeeds so a failed write does not drive the ensure-block restore against an unchanged file. - release.rake: replace `next if attempt == attempts - 1` with an `unless` guard around the retry puts+sleep block for readability. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…elper Expand the comment on `changed = true` to spell out the trade-off the current ordering accepts: if `File.write` partially writes before raising, the `ensure` block will not restore the original content. This is rare on local filesystems for small JSON files (effectively atomic single-buffer write) and acceptable for a release-time helper, but documenting it prevents a future reader from flipping the ordering back without understanding the implication. Addresses review feedback on PR #3343.
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
from
c7673a6 to
cc49b01
Compare
![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. A cloud agent has been kicked off to fix the reported issue.
Reviewed by Cursor Bugbot for commit cc49b01. Configure here.
Code Review — PR #3343OverviewSolid cleanup PR. The changes decompose cleanly into four concerns: documentation/rename, release atomicity, generator warning UX, and test hygiene. The atomic StrengthsAtomic Security comments in
Concerns1.
|
|
Mattered
Optional
Skipped
Verification
Scan window: default full-PR scan after the previous |
Code ReviewOverall: Clean refactoring and hardening PR. The changes are internally consistent, the test coverage is solid, and the documentation improvements are well-targeted. No must-fix findings. Atomic
|
| ensure | ||
| tmp.close unless tmp.closed? | ||
| File.unlink(tmp_path) if !renamed && File.exist?(tmp_path) |
There was a problem hiding this comment.
If Tempfile.create on line 984 raises (e.g. the directory isn't writable), Ruby initialises tmp and tmp_path to nil before the assignment runs. The ensure then calls nil.closed? and File.exist?(nil), both of which raise NoMethodError / TypeError, masking the original error.
Safe-navigation guards fix it:
| ensure | |
| tmp.close unless tmp.closed? | |
| File.unlink(tmp_path) if !renamed && File.exist?(tmp_path) | |
| tmp&.close unless tmp&.closed? | |
| File.unlink(tmp_path) if !renamed && tmp_path && File.exist?(tmp_path) |
1 participant
Summary
Follow-up to #3219 / fixes #3258. This PR is now rebased onto current
origin/main.RSC migration parser cleanup
rsc_plugin_without_client_references?toany_rsc_plugin_missing_client_references?and document the existential semantics.advance_js_scan_state, including caller coverage and regex/template-literal caveats.add_rsc_client_references_setupkeeps defensive duplicate-helper guards.Adjacent hardening
SECURITY:comments for CommonJS wrapping via bothadditionalContextandsupportModules.Review Follow-Ups
supportModulessecurity anchor and thepackage_json_pin_fallback_warningformatter spec.Generator tests / examples (3.2, minimum)failure was caused bynpm install shakapacker@10.1.0returningETARGETbefore that npm version was available. The registry now resolvesshakapacker@10.1.0, and the React 16 pinned example task passed locally.claude-reviewfailure is the Claude Action hitting its weekly limit (resets May 29, 3am UTC), not a repository test failure.CI / Test Plan
origin/mainnpm view shakapacker@10.1.0 version(cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_react16)(cd react_on_rails && BUNDLE_FORCE_RUBY_PLATFORM=true bundle exec rspec spec/react_on_rails/shakapacker_examples_rake_spec.rb spec/react_on_rails/release_rake_helpers_spec.rb spec/react_on_rails/generators/js_dependency_manager_spec.rb)Note
Medium Risk
Changes RSC config auto-migration control flow and npm publish package.json handling; mistakes could corrupt webpack configs or leave bad publish manifests, though new specs cover the release path.
Overview
This PR tightens RSC webpack config migration logic and documents Node renderer sandboxing, plus small release and generator hardening.
RSC clientReferences migration renames
rsc_plugin_without_client_references?toany_rsc_plugin_missing_client_references?and documents that it is an existential check (not the complement of “all plugins define clientReferences”).prepare_rsc_client_references_rewrite!replaces the old prepare helper so helper injection and rewrite decisions stay correct when some plugins already use scopedclientReferences.rsc_plugin_sections_safe_to_rewrite?now takesis_server:when counting unparseable sections. Lightweight JS scanner limits and caller contracts are centralized onadvance_js_scan_state, with extra comments on defensive duplicate-helper guards.Node renderer config comments add explicit SECURITY notes that
supportModules: trueand a plain-objectadditionalContextboth enable CommonJS wrapping and hostrequire.npm publish rewrites
package.jsonvia a same-directory temp file and atomic rename, sets the restorechangedflag only after rename succeeds, and adds a spec for failed rename cleanup.JS dependency fallback emits a clearer warning listing
name@versionpins written topackage.jsonwhen install fails. The webpack helper test description is made path-agnostic.Reviewed by Cursor Bugbot for commit 92f41ed. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation
Improvements
Tests