Derive Pro RSC client refs from the RSC graph#3556
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:
WalkthroughUpgrades react-on-rails-rsc to 19.0.5-rc.6, removes the local patch, generates a server-component registration-entry, runs a discovery precompile to emit rsc-client-references.json, and wires generators/webpack/precompile/CI/tests to consume and validate the manifest-driven client-reference flow. ChangesRSC Manifest-Driven Client Reference Discovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Code Review Overview This PR introduces RSC client-reference discovery driven by the actual RSC module graph rather than broad filesystem scanning:
The design is sound and cleanly solves the unrelated use-client file contamination problem. Blocker The PR is labelled do-not-merge appropriately. The react-on-rails-rsc pin in package.json must be updated to point to the upstream shakacode/react_on_rails_rsc repo (not the contributor personal fork) before this can land. See inline comment on package.json:72. Issues (inline comments posted) Blocker — package.json:72: react-on-rails-rsc pinned to a personal fork commit; supply-chain and stability risk. Medium — packs_generator.rb:391: server_component_registration_entries called twice inside server_component_registration_entry_content; the full component-discovery pipeline runs 4x per call. Fix: capture into a local variable once. Medium — packs_generator.rb:428: generated_nonentrypoints_directory_path calls FileUtils.mkdir_p as a side-effect, meaning every read-only path query (staleness check, cleanup enumeration) silently creates the directory. Medium — client_references.rb:1196-1199: generated_rsc_client_references_defined? relies on three plain String#include? checks. This is the same commented-out-code false-positive the regex+js_top_level_position? approach was written to prevent. Medium — rscManifestClientReferences.js:48: Dummy file duplicates the JSON-loading logic from the generated template with no shared source; the two can silently diverge. Low — rscManifestClientReferences.js:9: __dirname-relative path to ssr-generated/ ties the file to its two-levels-deep position. The generated template uses resolve() (CWD-relative) which is more portable. Low — dummy/package.json:107: build:dev:watch skips the ssr-generated cleanup that build:dev and build:test include, so watch mode can reuse stale discovery output. What looks good
|
size-limit report 📦
|
|
RSC-side update: I opened maintainer-owned draft replacement shakacode/react_on_rails_rsc#47 for shakacode/react_on_rails_rsc#46 because direct push to the contributor fork branch was rejected. shakacode/react_on_rails_rsc#47 carries shakacode/react_on_rails_rsc#46 plus review fixes for same-line |
|
shakacode/react_on_rails_rsc#47 is merged. |
Resolves the claude-review comments on PR #3556: - Repoint react-on-rails-rsc to the upstream shakacode repo at the same commit instead of a contributor fork. The upstream PR-head ref already contains that commit and GitHub archives are deterministic per SHA, so the codeload tarball is byte-identical and the lockfile integrity hash is unchanged. Keeps the install path off a personal fork while the dep is still pinned pending react_on_rails_rsc#46. - packs_generator: capture server_component_registration_entries once in server_component_registration_entry_content instead of invoking the component-discovery pipeline four times per call. - packs_generator: move FileUtils.mkdir_p out of the generated_nonentrypoints_directory_path accessor into a dedicated ensure_nonentrypoints_directory!, called only before the actual file writes, so read-only callers (staleness checks, cleanup enumeration) no longer create the directory as a side effect. - dummy rscManifestClientReferences: resolve the refs JSON relative to process.cwd() (the Rails root) like the generated template, instead of coupling to this file's location via __dirname. - client_references generator: detect the generated manifest fallback structurally via the module-scope fallbackRscClientReferences literal rather than brittle content.include? substring guards on the env-var and JSON filename, which coupled detection to template internals and added no safety. Adds regression tests locking the structural behavior. - dummy package.json: add the public/webpack + ssr-generated cleanup step to build:dev:watch so a stale rsc-client-references.json is not reused. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Code Review: PR 3556 - Derive Pro RSC client refs from the RSC graph. Full review text follows in replies. |
Overview - This PR replaces broad filesystem scanning for RSC clientReferences with a precise two-phase build: (1) pack generation emits server-component-registration-entry.js for only registered server components, (2) a precompile-hook discovery webpack build walks the RSC import graph and emits rsc-client-references.json via RSCReferenceDiscoveryPlugin, (3) client/server webpack configs read that JSON. The fallback-to-directory-scan behaviour preserves backward compatibility. ### Issue: Performance regression in staleness check - server_component_registration_entries is called on every generated_files_present_and_up_to_date? check (every Rails request in development). The method is not memoized and its call chain triggers multiple Dir.glob calls per invocation via components_for_server_registration being called twice inside server_component_registration_entries (once directly and once through server_component_names_for_registration). That is 4+ filesystem scans per staleness check, and the method is also called in build_expected_files_set and create_server_component_registration_entry. Memoizing components_for_server_registration would fix this. ### Issue: registerServerComponent call formatting - entries.keys.join(',newline') produces registerServerComponent({ComponentA, newline ComponentB}); - opening brace on the first name, no trailing comma, closing brace on the last. Valid JS but visually jarring. Consider join(', ') or a proper multi-line form. ### Issue: Git-pinned pre-release dependency - react-on-rails-rsc is pinned to commit a7f3838 (v19.0.5-rc.3). Noted as temporary in the PR description, but flagging as a merge blocker. ### Design positives - Clean separation of FileUtils.mkdir_p from generated_server_bundle_file_path into ensure_nonentrypoints_directory! is a clear improvement. Detection strategy in generated_rsc_client_references_defined? using the structural fallbackRscClientReferences literal (not string-matching env-var names) is correct and well-documented. ### Minor - Duplicated JSON-reading logic between client_references.rb generated template and rscManifestClientReferences.js - a cross-reference comment would help. - rsc_manifest_registration_entry_path? reads like a path accessor but returns a boolean; consider valid_rsc_registration_entry_path?. - No test for the rsc_client_references_defined? short-circuit in generated_rsc_client_references_defined?. |
Addresses the remaining review comment on PR #3556 (duplicated logic between the Pro dummy's hand-written rscManifestClientReferences.js and the inline block the RSC setup generator emits via rsc_client_references_js). Rather than extract a shared package module — which would force the generated end-user config into an opaque require(), add a public react-on-rails export, and rewrite the migration detection to match a magic require path (re-introducing the brittleness just removed from generated_rsc_client_references_defined?) — this keeps both copies but makes drift fail CI: - Document the dummy file as an intentional mirror of the generator's resolution contract (override env var, default manifest path, { refs: [...] } shape, fallback ordering, precompile-hook error), and align its broad-fallback include regex to the template's (js|mjs|cjs|ts|mts|cts|jsx|tsx). - Add a dummy-side Jest contract test exercising every branch of the resolver (env-var override, default manifest, invalid payload, discovery/bundle fallback, missing-manifest hint, no-manifest fallback). - Add a generator-side RSpec assertion that the emitted config carries the same contract tokens, so a change to either side breaks a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
💡 Codex Review Check the rspack RSC config path before falling back
In rspack projects the generator writes ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 similar comments
💡 Codex Review Check the rspack RSC config path before falling back
In rspack projects the generator writes ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
💡 Codex Review Check the rspack RSC config path before falling back
In rspack projects the generator writes ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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: 267d8fc988
ℹ️ 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".
| }; | ||
|
|
||
| const rscConfigSupportsDiscovery = () => { | ||
| const rscWebpackConfig = resolve('config/webpack/rscWebpackConfig.js'); |
There was a problem hiding this comment.
In rspack projects the generator writes rscWebpackConfig.js under config/rspack/ via destination_config_path, but the emitted resolver always probes config/webpack/rscWebpackConfig.js. When a current rspack RSC app has a server-component registration entry but no generated manifest (for example after cleaning ssr-generated and running bin/shakapacker directly), rscConfigSupportsDiscovery() incorrectly returns false and silently falls back to the broad client-reference scan instead of enforcing the discovery manifest. That leaves rspack apps on the old behavior even though their generated configs can produce graph-derived refs.
Useful? React with 👍 / 👎.
|
Review sweep after |
Code Review — PR #3556: Derive Pro RSC client refs from the RSC graphOverviewThis PR wires RSC client-reference discovery into the precompile/build flow so the webpack configs use a point-in-time manifest ( The architecture is sound and the test coverage (unit, generator contract, and integration specs) is thorough. Comments below range from a real correctness concern down to style/doc nits. Correctness
E2E CSS FOUC assertion loosened to accept multiple patterns The test now accepts either a Design / ArchitectureTwo implementations of the same resolution cascade (generator IIFE vs Pro dummy function)
The IIFE requires Path resolution inconsistency between the template and the Pro dummy
Performance
In the generated IIFE (module-scope evaluation), this runs once at startup — fine. In the Pro dummy SecurityNo shell injection risks. Subprocess invocations ( Test CoverageCoverage is strong. A few gaps:
Nits
SummaryThe core approach is correct, the contract-test strategy (pinning both sides of the generator/dummy mirror) is a clever way to prevent drift, and the double recursion guard (
Generated with Claude Code |
| next unless stripped_body.match?(/\bdirectory\s*:\s*resolve\(\s*config\.source_path\s*\)/) | ||
|
|
||
| range_end = close_brace + 1 | ||
| range_end += 1 if content[range_end] == ";" |
There was a problem hiding this comment.
The semicolon-skipping logic here is subtle: when the original object literal ends with ;, range_end moves one past it, so the replacement text (rsc_client_references_js, which itself ends with })();) lands correctly without a double semicolon. This is correct — but the edge case where the original literal has no trailing semicolon (const rscClientReferences = { ... }\n) is untested. In that case range_end stays at close_brace + 1 and the replacement still works fine, but a unit test directly on replace_legacy_rsc_client_references_setup for both shapes (with and without ;) would prevent a future regression from going unnoticed.
| range_end += 1 if content[range_end] == ";" | |
| range_end = close_brace + 1 | |
| # Skip a trailing semicolon so the replacement (which already ends with `);`) | |
| # lands without a double semicolon or a missing one. | |
| range_end += 1 if content[range_end] == ";" | |
| return [match.begin(0), range_end] |
| overrides.each_key { |key| original[key] = ENV.fetch(key, nil) } | ||
| overrides.each { |key, value| ENV[key] = value } |
There was a problem hiding this comment.
The two-pass iteration (first collect originals, then apply overrides) is intentional here: the "preserves setup failures while cleaning any captured env keys" test below verifies that a bad object that raises during each_key still leaves original in a defined state so the ensure block can safely iterate it. This is correct and the separation is actually important for the test.
One small nit: the comment on the original = {} line above (outside the begin) already explains the invariant; the code is clear as-is.
| function rscConfigSupportsDiscovery() { | ||
| const rscWebpackConfig = path.resolve('config/webpack/rscWebpackConfig.js'); | ||
| const precompileHook = path.resolve('bin/shakapacker-precompile-hook'); | ||
|
|
||
| return ( | ||
| fileContainsAll(rscWebpackConfig, ['RSC_REFERENCE_DISCOVERY_BUILD', 'RSCReferenceDiscoveryPlugin']) && | ||
| fileContainsAll(precompileHook, [ | ||
| 'generate_rsc_manifest_client_references_if_needed', | ||
| 'RSC_REFERENCE_DISCOVERY_BUILD', | ||
| ]) |
There was a problem hiding this comment.
rscConfigSupportsDiscovery reads two files from disk each time it is called. Because rscManifestClientReferences is a function (called per webpack compile), this branch is reached on every invocation when the default manifest is absent but the registration entry is present — the "old tooling" upgrade path.
In practice this path should only be hit by apps that have not yet re-run rails g react_on_rails:rsc, so it is not hot. Worth a short comment so future readers don't prematurely add memoization (or, conversely, don't accidentally move this into a hot path without thinking):
| function rscConfigSupportsDiscovery() { | |
| const rscWebpackConfig = path.resolve('config/webpack/rscWebpackConfig.js'); | |
| const precompileHook = path.resolve('bin/shakapacker-precompile-hook'); | |
| return ( | |
| fileContainsAll(rscWebpackConfig, ['RSC_REFERENCE_DISCOVERY_BUILD', 'RSCReferenceDiscoveryPlugin']) && | |
| fileContainsAll(precompileHook, [ | |
| 'generate_rsc_manifest_client_references_if_needed', | |
| 'RSC_REFERENCE_DISCOVERY_BUILD', | |
| ]) | |
| // Checks whether the app's RSC webpack config and precompile hook both contain the markers | |
| // required to produce the discovery manifest. Reading the two files here is intentionally | |
| // un-memoized: this function is only reached on the "old tooling" fallback path (registration | |
| // entry exists, but no manifest yet), which is a one-time transition state, not a hot path. | |
| function rscConfigSupportsDiscovery() { |
| create_server_pack(verbose: verbose) if ReactOnRails.configuration.server_bundle_js_file.present? | ||
| create_server_component_registration_entry(verbose: verbose) if ReactOnRails::Utils.rsc_support_enabled? |
There was a problem hiding this comment.
Both create_server_pack (via generated_server_pack_file_content) and create_server_component_registration_entry (via server_component_registration_entries) independently call components_for_server_registration, which runs an un-memoized Dir.glob. This means every generate_packs call with RSC enabled does two filesystem scans of the component tree.
The staleness check path (stale_or_missing_packs?) avoids this by computing the map once and passing it through. The same pattern could be applied here — compute components = components_for_server_registration once and pass it to both callers. Non-blocking since this is tracked in #3642.
| const rscWebpackConfigPath = path.resolve(__dirname, '../config/webpack/rscWebpackConfig.js'); | ||
|
|
||
| describe('rscWebpackConfig discovery build contract', () => { | ||
| const source = fs.readFileSync(rscWebpackConfigPath, 'utf8'); |
There was a problem hiding this comment.
Reading the source file at describe-time (outside beforeEach) is intentional here — this is a static source analysis test, not a runtime execution test. A short comment would prevent a future reader from thinking this is a mistake or adding unnecessary beforeEach setup:
| const source = fs.readFileSync(rscWebpackConfigPath, 'utf8'); | |
| // Static analysis of the compiled source — read once at describe-time. | |
| const source = fs.readFileSync(rscWebpackConfigPath, 'utf8'); |
Pro Node Renderer Benchmark Summary
▲/▼ change vs baseline · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 2/2) Benchmark Summary
▲/▼ change vs baseline · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Core Benchmark Summary
▲/▼ change vs baseline · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 1/2) Benchmark Summary
▲/▼ change vs baseline · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
* origin/main: Derive Pro RSC client refs from the RSC graph (#3556) Docs: follow up v15 anchor and Rails 5.2 flag # Conflicts: # CHANGELOG.md
…lement * origin/main: Migrate OSS dummy client examples to TypeScript (#3606) Docs: use async props pattern in metadata and render-function examples (#3672) Require PR numbers in squash merge titles (#3684) Improve create-app smoke diagnostics (#3599) Suppress regression issues when only /posts_page: Pro regresses (#3668) Harden ShakaPerf artifact setup Polish RSC generator install follow-ups Harden renderer teardown polish fix(benchmark): harden target monitoring Docs: clarify rails new JavaScript skip flag (#3666) Tighten benchmark summary annotations Decouple Shakapacker config helpers from SystemChecker Derive Pro RSC client refs from the RSC graph (#3556) Docs: follow up v15 anchor and Rails 5.2 flag # Conflicts: # react_on_rails/spec/dummy/client/app/startup/ManualRenderApp.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
Manual verification: reproduced the gap and confirmed the fix ✅Reproduced via the merged OSS regression spec ( Reproduction
Results
The 13 pre-fix failures are exactly the new server-component-registration / RSC-graph-classification cases. CaveatOSS-spec level for the generator behavior (server registration entry derivation, staleness fast path, cleanup) plus the Pro dummy config test confirming the manifest-backed client-reference helper consumes discovered refs JSON. I did not run a full Pro RSC discovery build end-to-end ( |
2 participants
Summary
react-on-rails-rsc@19.0.5-rc.6from npmrscClientReferenceshelpers to the manifest-backed helper during migrationSupports #3553.
RSC package dependency: stay on
react-on-rails-rsc@19.0.5-rc.6. I found no current gap that requires an rc.7. This PR should land after #3590 and preferably after #3616 so the generator and optional peer-range story are already inmain.Architecture decisions
rscClientReferenceshelpers are not treated as fully migrated. The generator upgrades them to the manifest-backed helper so upgraded apps get exact RSC graph references instead of silently keeping broad scanning.fileContainsAllchecks read each inspected file once per check. This avoids repeated disk reads without memoizing across long-running webpack config evaluation, where setup files may change while a developer is iterating.build:dev:watchdeletes stale development webpack output andssr-generatedbefore running the precompile hook. It deliberately does not delete production output.Verification
Review-fix commit
9d414eabe530902c8b5073a92b69f61dc4f40501:pnpm install --frozen-lockfilebundle exec rubocop react_on_rails/lib/generators/react_on_rails/rsc_setup/client_references.rb react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb react_on_rails/spec/support/shakapacker_precompile_hook_shared.rbpnpm exec prettier --check react_on_rails_pro/spec/dummy/tests/rsc-manifest-client-references.test.js react_on_rails_pro/spec/dummy/tests/package-scripts.test.js react_on_rails_pro/spec/dummy/config/webpack/rscManifestClientReferences.js react_on_rails_pro/spec/dummy/package.jsonbundle exec rspec react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rbbundle exec rspec react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rbpnpm --dir react_on_rails_pro/spec/dummy run test:js -- --runInBand tests/rsc-manifest-client-references.test.js tests/package-scripts.test.jsgit diff --checkEarlier implementation verification retained from the original PR:
cd react_on_rails/spec/dummy && bundle exec rspec spec/packs_generator_spec.rbbundle exec rspec spec/requests/rsc_use_client_css_manifest_spec.rbfromreact_on_rails_pro/spec/dummyNODE_CONDITIONS=react-server pnpm --dir packages/react-on-rails-pro exec jest tests/manifestLoaderServerRSC.rsc.test.ts --runInBandpnpm --dir packages/react-on-rails-pro exec jest tests/manifestLoader.test.ts tests/resolveCssHrefs.test.ts --runInBandpnpm run lintpnpm start format.listDifferentbundle exec rubocopfromreact_on_rails/bundle exec rubocop --ignore-parent-exclusionfromreact_on_rails_pro/bundle exec rubocop benchmarkspnpm list react-on-rails-rsc --depth 0 -rpnpm run build:devfromreact_on_rails_pro/spec/dummypnpm run build:testfromreact_on_rails_pro/spec/dummypnpm run build:clientfromreact_on_rails_pro/spec/dummypnpm run build:serverfromreact_on_rails_pro/spec/dummySummary by CodeRabbit
New Features
Bug Fixes
Chores
Follow-up
Review follow-ups
Non-blocking review nits are tracked in #3642, including unresolved threads
PRRT_kwDOAnNnU86HSorq,PRRT_kwDOAnNnU86HSpqC, andPRRT_kwDOAnNnU86HSp95. The two version-specific discovery-plugin error-message threads were addressed in the latest push and may remain visible only as outdated unresolved threads until reviewer cleanup.