refactor: clarify RSC parser helpers and release safeguards (#3258)#3343
refactor: clarify RSC parser helpers and release safeguards (#3258)#3343justin808 wants to merge 6 commits into
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 (7)
WalkthroughThis PR makes targeted improvements across multiple subsystems: Node-renderer VM sandboxing documentation, atomic file operations for npm publishing, extraction of a shared dependency warning helper, and refinement of RSC plugin detection logic to correctly handle multi-section configurations. No feature-level behavioral changes to user-facing functionality. ChangesMaintenance and refinement improvements across Node-renderer, release infra, generators, and RSC setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/react-on-rails/tests/webpackHelpers.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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
Code Review: refactor: clarify RSC migration parser helpers (#3258)Summary: Documentation/rename-only refactoring with no intended behavior changes. The overall direction is good — the rename is more precise, the consolidated scanner docs are useful, and the Issues (see inline comments)
Positives
|
- 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>
|
Mattered
Optional
Skipped
Verification
Scan window: PR review history from 2026-05-24T08:48:01Z through 2026-05-26T04:01:08Z. Future full-PR scans should start after this comment unless |
![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: 9962c16960
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Code Review - Summary: Pure refactoring/documentation pass with no behavior change to the RSC parser logic. Clean, well-reasoned PR. |
|
Code Review Summary: Pure refactoring/documentation pass with no behavior change to the RSC parser logic. The changes improve naming clarity, consolidate scanner documentation, and harden two minor edge cases in the release tooling. Clean, well-reasoned PR. RSC Migration Parser Cleanup Rename: rsc_plugin_without_client_references? -> any_rsc_plugin_missing_client_references? is a genuine improvement. The old name implied a universal check; the new name correctly signals existential semantics. The added docblock explicitly calls out the non-complement behavior when multiple plugin sections exist (one configured, one not -- both predicates can return true simultaneously). Old name is confirmed fully removed from react_on_rails/ and react_on_rails_pro/. Consolidated advance_js_scan_state docblock: The 45-line block is long but justified -- this function has 8 call sites, non-obvious limitations, a caller contract invariant (block_comment_exit_guard), and documented future-expansion requirements. The pointer approach at rsc_plugin_options_without_comments and matching_js_closing_brace is cleaner than duplicating the limitation prose. add_rsc_client_references_setup comment: Correctly explains both what the duplicate guard prevents (SyntaxError at config load) and why it is kept even though the single caller already checked. Good defensive design rationale. Release Rake Hardening with_publishable_package_json fix: Deferring changed = true until after a successful File.write is a real correctness improvement. In the original code, if File.write raised, changed was already true, causing the ensure block to attempt a redundant restore of an unchanged file. The new ordering avoids that. See inline note for a minor edge-case caveat. fetch_npm_package_metadata_with_retries restructure: Functionally identical to the prior next if attempt == attempts - 1 guard. The unless block is a valid style choice, though Ruby conventionally favors the one-liner next if form for early-exit guards. Not a blocking issue. JS Dependency Manager Warning placement: Moving add_warning after File.write (and before true) is correct -- the user-facing message now only fires when the write actually succeeded. The extracted package_json_pin_fallback_warning helper is clean and independently testable. Minor style note: other warnings in this file use heredocs. The new helper uses string concatenation. Both are fine for a single-sentence message. Node Renderer Security Comments Upgrading NOTE: to SECURITY: and naming specific built-ins (fs, child_process, os) is strictly better than the prior text. The two separate SECURITY: markers for additionalContext and supportModules correctly call out that either option independently enables CommonJS mode -- a reader scanning only one of the two options will still see the warning. Test Coverage The PR runs the 137-example RSC generator spec with 0 failures. No spec changes are needed since there is no behavior change. The test description fix in webpackHelpers.test.ts accurately reflects what the fixture tests (path-agnostic pattern matching, not webpack 4 specifically). Verdict: Approve with one minor inline observation. The rename, documentation consolidation, and release-rake hardening are all sound. No blocking issues. |
|
Mattered
Considered, kept as-is
Acknowledged
CI note: the OpenTelemetry build failures on this branch trace to the stale base ( |
justin808
changed the title
…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.
Code Review: refactor: clarify RSC parser helpers and harden release tooling (#3258)Overall: This is a low-risk, well-scoped cleanup PR. The rename, documentation consolidation on 1.
|
| // `supportModules: false`. When `supportModules: true`, the renderer wraps the bundle and | ||
| // injects the host `require` regardless of `additionalContext`. | ||
| // `supportModules: false`. | ||
| // SECURITY: When `supportModules: true`, the renderer also wraps the bundle and injects the |
There was a problem hiding this comment.
Both SECURITY: blocks currently live inside the additionalContext comment section. A reader who reads only the supportModules field docs (lines 45–52) and stops at supportModules: boolean; will not see this anchor.
Consider adding a brief pointer directly before supportModules: boolean;:
| // SECURITY: When `supportModules: true`, the renderer also wraps the bundle and injects the | |
| // SECURITY: When `supportModules: true`, the renderer also wraps the bundle and injects the | |
| // host `require` regardless of `additionalContext`. |
…and place a short forward-reference in the supportModules comment block pointing to the detailed note on additionalContext. That way both entry points surface the warning independently.
| pinned_list = versioned_packages.map { |name, version| "#{name}@#{version}" }.join(", ") | ||
| "⚠️ Package manager install failed. Wrote the following version pins to package.json " \ | ||
| "so you can rerun your package manager manually: #{pinned_list}" | ||
| end |
There was a problem hiding this comment.
The PR description explicitly calls out that package_json_pin_fallback_warning is extracted to be "independently testable," but no spec is added for it. The format "#{name}@#{version}" joined by ", " is a user-visible contract — worth a single unit test asserting the output string so a future refactor can't silently break the message.
# Example spec skeleton:
describe "#package_json_pin_fallback_warning" do
it "formats pinned packages as name@version list" do
result = subject.send(:package_json_pin_fallback_warning, [["react", "18.0.0"], ["react-dom", "18.0.0"]])
expect(result).to include("react@18.0.0, react-dom@18.0.0")
end
end
| # restore the original content; we accept this as a known limitation of the single-buffer | ||
| # write approach, since this is a release-time helper running on local disk where | ||
| # `File.write` is effectively atomic for small JSON files. | ||
| changed = true |
There was a problem hiding this comment.
The comment correctly documents the trade-off. For a release helper that runs once locally against a real package.json, a proper atomic write is low-cost and eliminates the partial-write scenario entirely:
require "tempfile"
tmpfile = Tempfile.new("package_json", File.dirname(package_json_path))
tmpfile.write("#{JSON.pretty_generate(package_json)}\n")
tmpfile.close
FileUtils.mv(tmpfile.path, package_json_path)
changed = trueFile.rename (which FileUtils.mv uses on the same filesystem) is an atomic kernel operation — the old file is either replaced fully or not at all. This removes the need for the trade-off comment entirely. Happy to leave it as-is if the current behaviour is intentional, but wanted to flag the pattern.
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
changed the title
justin808
force-pushed
the
jg/3258-rsc-migration-parser-clarity
branch
from
918acd3 to
06dc380
Compare
1 participant
Summary
Follow-up to #3219 / fixes #3258. This PR is now rebased onto
main, so the stale stacked-branch CI failure from the pre-#3420 OpenTelemetry syntax issue is no longer part of the branch.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
Test Plan
pnpm install --frozen-lockfilebundle exec rspec react_on_rails/spec/react_on_rails/release_rake_helpers_spec.rb react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb— 110 examples, 0 failures(cd react_on_rails && bundle exec rubocop)— 203 files, no offensespnpm run lintpnpm run type-checkpnpm --dir packages/react-on-rails-pro-node-renderer exec tsc --project src/tsconfig.json --noEmitpnpm exec prettier --check packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts packages/react-on-rails/tests/webpackHelpers.test.tsNote
Low Risk
Changes are mostly comments, renames, docs, and safer file I/O during publish; no runtime behavior changes to SSR auth or payment paths.
Overview
This PR tightens documentation and naming around RSC webpack migration and Node renderer config, plus small release/generator reliability fixes.
RSC generator:
rsc_plugin_without_client_references?is renamed toany_rsc_plugin_missing_client_references?with comments explaining existential semantics (not the complement of “all define clientReferences” when multiple plugins exist). Lightweight JS scanner limits and theadvance_js_scan_statecontract are centralized in one place;add_rsc_client_references_setupduplicate-guard rationale is spelled out.Node renderer:
configBuilder.tsSECURITY:notes now state explicitly thatsupportModules: trueand any plain-objectadditionalContext(including{}) wrap the bundle with hostrequire, disabling VM sandboxing.Release: Publish-time
package.jsonworkspace rewrites use a same-directory temp file + atomic rename;changedis set only after rename succeeds soensurerestore matches reality. NPM metadata retry loop uses a clearerunlessguard.Generator deps: Failed installs that fall back to writing pins now emit a formatted warning listing
name@versionpairs; specs cover that and release restore behavior.Tests: Webpack
reactDomClientWarningtest description is path-agnostic.Reviewed by Cursor Bugbot for commit 06dc380. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Chores