fix: scope RSC client reference discovery#3219
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:
WalkthroughGenerator and webpack/rspack templates now define a scoped ChangesRSC clientReferences scoping
Sequence DiagramsequenceDiagram
participant Generator
participant File as "webpack config file"
participant Shakapacker
participant RSCPlugin as "RSCWebpackPlugin"
Generator->>File: open/read file
alt existing RSCWebpackPlugin found without clientReferences
Generator->>File: rewrite plugin constructor to add clientReferences
else no RSCWebpackPlugin
Generator->>File: insert imports for Shakapacker & path
Generator->>Shakapacker: use config.source_path
Generator->>File: insert rscClientReferences + new RSCWebpackPlugin({..., clientReferences})
end
File->>RSCPlugin: RSCWebpackPlugin configured with scoped clientReferences
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 scopes the Confidence Score: 5/5Safe to merge — changes are consistent across all code paths, no logic errors or regressions identified. All six changed files are internally consistent: both template (.tt) and existing-config transform paths inject the same scoped rscClientReferences object, config is never double-imported (server template already has it; client template adds it only in the RSC block), and specs validate the new content. No P0 or P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Generator invoked with RSC] --> B{New install or existing config?}
B -- New install --> C[Render .js.tt templates]
B -- Existing config --> D[update_webpack_configs_for_rsc]
C --> E[clientWebpackConfig.js.tt\nInjects: config from shakapacker\n resolve from path\n RSCWebpackPlugin\n rscClientReferences scoped to config.source_path]
C --> F[serverWebpackConfig.js.tt\nInjects: resolve from path\n RSCWebpackPlugin\n rscClientReferences scoped to config.source_path]
D --> G[update_client_webpack_config_for_rsc\ngsub: add RSCWebpackPlugin import +\n shakapacker config + rscClientReferences]
D --> H[update_server_webpack_config_for_rsc\ngsub: add RSCWebpackPlugin import +\n resolve + rscClientReferences]
E --> I[RSCWebpackPlugin with clientReferences\ndirectory: resolve config.source_path\nrecursive: true\ninclude: js,ts,jsx,tsx]
F --> I
G --> I
H --> I
I --> J[Webpack scans only source_path\nSkips vendor/bundle in CI]
Reviews (1): Last reviewed commit: "Scope RSC client references to app sourc..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThe PR resolves a webpack compilation issue where RSCWebpackPlugin's default recursive project root scan could inadvertently include vendored gem templates in CI environments. Changes include updating documentation, modifying webpack config generators and templates to explicitly compute and pass Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
Code Review — PR #3219: Scope RSC client reference discoverySummaryCorrect fix for a real CI reliability issue. Scoping Issues1.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code ReviewGood, targeted fix. The root cause (unscoped Summary
Issues1. Test gap — webpack server config (not in diff, so inline comment not possible)
Suggested addition to the test at line 99: it "adds RSCWebpackPlugin to serverWebpackConfig" do
assert_file "config/webpack/serverWebpackConfig.js" do |content|
expect(content).to include("RSCWebpackPlugin")
expect(content).to include("react-on-rails-rsc/WebpackPlugin")
expect(content).to include("clientReferences: rscClientReferences") # add
expect(content).to include("directory: resolve(config.source_path)") # add
end
endSimilarly 2. Possible duplicate
|
![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: d49943e2f3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08e341ea21
ℹ️ 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".
![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.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Server config missing duplicate
resolveimport guard- The server RSC webpack transform now skips injecting the path resolve import when it already exists and has regression coverage for that case.
Or push these changes by commenting:
@cursor push f73f903f50
Preview (f73f903f50)
diff --git a/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb b/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
--- a/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
+++ b/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
@@ -385,11 +385,11 @@
# Add RSCWebpackPlugin import after bundler require
server_injected_imports = [
"\\1",
- "const { resolve } = require('path');",
+ ("const { resolve } = require('path');" unless path_resolve_imported?(content)),
"const { RSCWebpackPlugin } = require('react-on-rails-rsc/WebpackPlugin');",
"",
rsc_client_references_js
- ].join("\n")
+ ].compact.join("\n")
gsub_file(
config_path,
diff --git a/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb b/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
--- a/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
+++ b/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
@@ -199,6 +199,38 @@
end
end
+ context "when the server webpack config already imports path resolve" do
+ before(:all) do
+ prepare_destination
+ simulate_existing_rails_files(package_json: true)
+ simulate_npm_files(package_json: true)
+ simulate_existing_file("config/initializers/react_on_rails_pro.rb", <<~RUBY)
+ ReactOnRailsPro.configure do |config|
+ config.server_renderer = "NodeRenderer"
+ end
+ RUBY
+ simulate_existing_file("Procfile.dev", "rails: bin/rails s\n")
+ simulate_pro_webpack_files
+ simulate_existing_file(
+ "config/webpack/serverWebpackConfig.js",
+ "const { resolve } = require('path');\n#{pro_server_webpack_content}"
+ )
+
+ Dir.chdir(destination_root) do
+ run_generator(["--force"])
+ end
+ end
+
+ it "does not inject a duplicate resolve import" do
+ assert_file "config/webpack/serverWebpackConfig.js" do |content|
+ expect(content.scan("const { resolve } = require('path');").length).to eq(1)
+ expect(content).to include("clientReferences: rscClientReferences")
+ expect(content).to include("directory: resolve(config.source_path)")
+ expect(content).to include("isServer: true")
+ end
+ end
+ end
+
context "when Pro is installed with a canonical legacy hello_world layout" do
before(:all) do
prepare_destinationYou can send follow-ups to the cloud agent here.
Code Review: PR #3219 — Scope RSC client reference discoveryOverviewThis PR correctly fixes the root cause of issue #3201: What's Good
Issues & SuggestionsBug: Server config injection lacks the
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a87cc3ce
ℹ️ 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 ReviewOverviewThis PR fixes a real CI bug (#3201) where the RSCWebpackPlugin, when The approach is sound: generate the scoped references once at the module level, share them between server and client plugin calls, guard against duplicate imports when rewriting existing configs, and document the risk clearly. The new test matrix is comprehensive. Issues & SuggestionsBug risk:
|
Code ReviewOverviewThis PR scopes RSC webpack plugin's client-reference scanning to The core approach — computing Issues FoundExisting apps with old generator output are not auto-upgradedBoth return if content.include?("RSCWebpackPlugin")This means users who previously ran the generator (which produced Aliased Shakapacker import causes a duplicate
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 184460aa25
ℹ️ 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".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Partial gsub can produce undefined variable reference
- Guarded the plugin rewrite on successful client-reference setup and added verification/tests so customized configs warn instead of receiving undefined references.
Or push these changes by commenting:
@cursor push 39dc680411
Preview (39dc680411)
diff --git a/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb b/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
--- a/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
+++ b/react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
@@ -489,6 +489,7 @@
missing = []
missing << "RSCWebpackPlugin in serverWebpackConfig.js" unless content.include?("RSCWebpackPlugin")
missing << "rscBundle parameter in serverWebpackConfig.js" unless content.include?("rscBundle")
+ missing.concat(check_rsc_client_references(content, "serverWebpackConfig.js"))
missing
end
@@ -497,7 +498,10 @@
return [] unless File.exist?(path)
content = File.read(path)
- content.include?("RSCWebpackPlugin") ? [] : ["RSCWebpackPlugin in clientWebpackConfig.js"]
+ missing = []
+ missing << "RSCWebpackPlugin in clientWebpackConfig.js" unless content.include?("RSCWebpackPlugin")
+ missing.concat(check_rsc_client_references(content, "clientWebpackConfig.js"))
+ missing
end
def check_rsc_scob_config
@@ -521,7 +525,7 @@
def update_existing_rsc_webpack_config(config_path, content, is_server:)
return if content.include?("clientReferences: rscClientReferences")
- add_rsc_client_references_setup(config_path, content, is_server: is_server)
+ return unless add_rsc_client_references_setup(config_path, content, is_server: is_server)
gsub_file(
config_path,
@@ -549,9 +553,25 @@
%r{(const commonWebpackConfig = require\('\./commonWebpackConfig'\);)}
end
+ return false unless content.match?(import_pattern)
+
gsub_file(config_path, import_pattern, injected_imports)
+ true
end
+ def check_rsc_client_references(content, filename)
+ return [] unless content.include?("RSCWebpackPlugin")
+
+ missing = []
+ unless content.include?("clientReferences: rscClientReferences")
+ missing << "clientReferences option in #{filename}"
+ end
+ if content.include?("clientReferences: rscClientReferences") && !content.include?("const rscClientReferences =")
+ missing << "rscClientReferences definition in #{filename}"
+ end
+ missing
+ end
+
def shakapacker_config_imported?(content)
commonjs_named_imported?(content, "shakapacker", "config")
end
diff --git a/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb b/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
--- a/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
+++ b/react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
@@ -303,6 +303,68 @@
end
end
+ context "when existing RSC webpack configs have customized import anchors" do
+ before(:all) do
+ prepare_destination
+ simulate_existing_rails_files(package_json: true)
+ simulate_npm_files(package_json: true)
+ simulate_existing_file("config/initializers/react_on_rails_pro.rb", <<~RUBY)
+ ReactOnRailsPro.configure do |config|
+ config.server_renderer = "NodeRenderer"
+ end
+ RUBY
+ simulate_existing_file("Procfile.dev", "rails: bin/rails s\n")
+ simulate_pro_webpack_files
+ simulate_existing_file("config/webpack/serverWebpackConfig.js", <<~JS)
+ const { RSCWebpackPlugin } = require('react-on-rails-rsc/WebpackPlugin');
+ const webpack = require('webpack');
+
+ const configureServer = (rscBundle = false) => {
+ const serverWebpackConfig = { plugins: [] };
+
+ if (!rscBundle) {
+ serverWebpackConfig.plugins.push(new RSCWebpackPlugin({ isServer: true }));
+ }
+
+ return serverWebpackConfig;
+ };
+
+ module.exports = { default: configureServer };
+ JS
+ simulate_existing_file("config/webpack/clientWebpackConfig.js", <<~JS)
+ const { RSCWebpackPlugin } = require('react-on-rails-rsc/WebpackPlugin');
+
+ const configureClient = () => {
+ const clientConfig = { plugins: [] };
+ clientConfig.plugins.push(new RSCWebpackPlugin({ isServer: false }));
+
+ return clientConfig;
+ };
+
+ module.exports = configureClient;
+ JS
+
+ Dir.chdir(destination_root) do
+ run_generator(["--force"])
+ end
+ end
+
+ it "does not add undefined rscClientReferences references" do
+ assert_file "config/webpack/serverWebpackConfig.js" do |content|
+ expect(content).not_to include("clientReferences: rscClientReferences")
+ expect(content).not_to include("const rscClientReferences =")
+ end
+ assert_file "config/webpack/clientWebpackConfig.js" do |content|
+ expect(content).not_to include("clientReferences: rscClientReferences")
+ expect(content).not_to include("const rscClientReferences =")
+ end
+
+ warning_text = GeneratorMessages.messages.join("\n")
+ expect(warning_text).to include("clientReferences option in serverWebpackConfig.js")
+ expect(warning_text).to include("clientReferences option in clientWebpackConfig.js")
+ end
+ end
+
context "when the client webpack config uses aliased imports" do
before(:all) do
prepare_destinationYou can send follow-ups to the cloud agent here.
Code Review — PR #3219: Scope RSC client reference discoveryOverviewThis PR fixes a real CI problem (#3201) cleanly: by scoping I found four issues worth addressing before merge — one is a silent failure path. Issues1. Silent no-op when user has a customised
|
Code Review: PR #3219 — Scope RSC Client Reference DiscoveryOverviewThis PR fixes a real CI pain point (#3201): without a scoped The scope is substantial: ~900 lines of Ruby for the JS state-machine scanner and migration logic, plus ~1,700 lines of new tests. The tests are exceptionally thorough — CRLF handling, commented-out anchors, nested template literals, pretend mode, double-import guards, etc. Strengths
Issues & SuggestionsMisleading method name Suggestion: rename to Duplicated O(n×m) comment
ESM import limitation not surfaced clearly in the warning
Minor Notes
SummaryThis is well-designed migration tooling with strong test coverage and appropriate safety guardrails. The items above are refinements rather than blockers. The naming issue with |
- Make `rsc_plugin_is_server_match?` depth-aware via `js_top_level_position?` so a
nested `metadata: { isServer: true }` inside another option's value is no longer
routed into the wrong `is_server:` partition bucket. Previously the section was
marked safe for the mismatched bucket, the splice helper correctly bailed (it is
already depth-aware), and `warn_missing_rsc_plugin_target` then fired with a
misleading "no plugin options with isServer: <value> could be rewritten" warning
for a config that was actually fine. Partition and splice now agree on what
counts as a top-level `isServer:` pair.
- Replace the side-effecting `any?` block in `rsc_client_references_setup_anchor_match`
with an explicit `each` + early `return` so the "first code-position match wins"
intent is obvious without an outer-scope capture.
- Document the block-comment exit contract on `advance_js_scan_state` (callers
whose post-call branch inspects `char` as a significant character must guard on
`prev_state == :block_comment`) and add an inline note at the one caller that
relies on this invariant implicitly (`matching_js_closing_brace`).
Adds a regression test that bucketing a section with `metadata: { isServer: true }`
plus top-level `isServer: false` lands the section only in the `is_server: false`
partition, never `is_server: true`.
Round 3 review addressedPushed commit addressing the three open review threads:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22b805141
ℹ️ 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 ReviewOverviewThis PR fixes a real CI reliability problem: without scoped The significant addition is a migration path for existing webpack configs — a ~927-line lightweight JavaScript scanner/rewriter embedded in the Rails generator. That is the part that deserves careful scrutiny. Strengths
IssuesMedium — Direct
|
- Rename `content_before_rsc_setup_anchor` to `content_through_rsc_setup_anchor` and add a doc comment explaining the inclusive slice — callers (`rsc_setup_blocked_by_later_imports?`, `inject_rsc_*_imports`, import-detection helpers) need the anchor itself in the returned content to check whether required imports appear up to and including the anchor line. - Drop the redundant O(n × m) note inside `rsc_plugin_option_sections_partition`; the wrapper `rsc_plugin_option_sections` already carries the perf-cost note for the public surface. - Add an inline comment at `return false if rewrites.empty?` documenting that the sole caller (`update_existing_rsc_webpack_config`) is responsible for warning via `warn_missing_rsc_plugin_target`, so the silent return is intentional. - Expand `warn_missing_rsc_client_references_anchor` to mention that the generator looks for the CommonJS `require`-style anchor and that ESM `import` syntax is unsupported, since "config converted to ESM" is the most common reason users hit this warning.
Round 4 review addressedPushed commit addressing the four new threads opened against round 3:
Verification:
|
- Tolerate a single trailing comma between an RSCWebpackPlugin options object
and the closing `)` so configs formatted with Prettier's `trailingComma: "all"`
(`new RSCWebpackPlugin({...},)`) are no longer marked unparseable. A second
comma still bails — that would be invalid JS. Adds a regression test that the
partition routes a trailing-comma section into the `:safe` bucket.
- Fix dead-code fallback in `splice_client_references_at_close_brace`: the
previous indent regex `/\A[ \t]*/` (zero-or-more) always matched at least the
empty string, so the `|| " "` fallback could never fire. Switched to
`/\A[ \t]+/` (one-or-more) so an unindented last line correctly hits the
fallback instead of producing a zero-width indent.
Round 5 review addressedRound 5 produced 6 new threads — 2 substantive bugs, 1 intentional-as-designed, and 3 that were already resolved by round 4. Pushed commit with the 2 fixes plus reply-only resolution on the rest. Fixed:
Reply-only resolutions:
Verification:
|
Code Review — PR #3219: Fix RSC client reference scopeOverviewThis PR fixes a real CI regression (#3201) where
Code qualityStrengths
Concerns1. O(n²×m) actual complexity (comment at line 699 says O(n×m)) The noted complexity is understated. 2. The 3. When the last line of the plugin options body starts at column 0 (no indentation before the closing brace), 4. ESM config detection happens late When a config uses ESM ( SecurityNo concerns — this is a local config generator with no network I/O. VerdictThe fix is correct, the test coverage is thorough, and the failure modes are all handled gracefully. The concerns above are minor; items 1 and 3 are the most actionable. Overall this is a well-crafted PR. |
| trailing = body[/\s*\z/] | ||
| content = body[0...(body.length - trailing.length)] | ||
| content_without_comments = rsc_plugin_options_without_comments(content).rstrip | ||
| separator = content_without_comments.end_with?(",") ? " " : ", " |
There was a problem hiding this comment.
When the last non-trailing-whitespace line of the options body starts at column 0 (i.e. no indentation before the closing-brace line), content[last_line_start..][/\A[ \t]*/] returns "" and the || " " fallback injects 2-space indentation unconditionally. In a tab-indented or 4-space project this produces a cosmetically mismatched line that a linter may flag.
A small improvement: prefer the indentation of the nearest non-blank line above rather than hard-coding " " as the default, or at least document the 2-space assumption in the comment above rsc_client_references_js.
| # appears inside a function body (which does not produce a module-scope binding). | ||
| next false unless js_top_level_position?(content, Regexp.last_match.begin(0)) | ||
|
|
||
| bindings = captures.first |
There was a problem hiding this comment.
binding.start_with?("#{binding_name} =") requires a space before =. The pattern const { config=fn() } = require('shakapacker') (no space, which JS style guides discourage but do not forbid) would not be detected, causing the generator to inject a duplicate const { config } = require('shakapacker');. This is a very minor edge case — virtually no real webpack config omits the space — but it's worth a note since the import-deduplication logic elsewhere handles whitespace variation explicitly.
Code Review: Fix RSC client reference scopeOverviewThis PR fixes a real CI pain point (Issue #3201) where omitting The approach is sound: update the generator templates for new installs, and add a migration path for existing configs. The migration path includes a lightweight Ruby-based JS scanner to safely navigate comments, strings, and brace nesting — with clearly documented limitations and extensive safety checks. Test coverage is exceptional — 25+ new spec contexts covering edge cases including CRLF line endings, aliased imports, nested objects, comment-only matches, pretend mode, and unparseable regex literals. IssuesPotential binding-conflict false negatives
Both conflict-detection helpers use patterns like: pattern = /^[ \t]*(?:(?:const|let|var)\s+config\b|function\s+config\s*\()/This matches const { config } = require('some-internal-config-package');then This is a real-world edge case (unlikely in webpack configs), but closing the gap would prevent silent corruption. Multiple content scans within
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96e630f6bc
ℹ️ 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 { config /* primary */ } = require('shakapacker')`) are also unsupported: | ||
| # they leave their text in the captured binding so the exact `config` match fails. | ||
| # All of these shapes are vanishingly rare in real webpack configs. | ||
| binding == binding_name || binding.start_with?("#{binding_name} =") |
There was a problem hiding this comment.
commonjs_named_imported? only accepts config or config = ..., so valid self-alias forms like const { config: config } = require('shakapacker') (and similarly resolve: resolve) are treated as missing imports. The migration then injects const { config } = ... / const { resolve } = ..., which redeclares an already-declared identifier and makes the generated webpack config fail to parse with Identifier has already been declared when rerunning the generator on such customized configs.
Useful? React with 👍 / 👎.
| # gsub_file call, and pretend mode is already handled above. | ||
| File.write(full_path, content) | ||
| say_status(:rewrite, config_path, :green) | ||
| end |
There was a problem hiding this comment.
The pattern here (and the analogous one in top_level_resolve_binding?) only matches the const config = ... form, not the destructured form const { config } = require('some-other-package').
If a user has a non-shakapacker package that destructures config, shakapacker_config_imported? returns false (wrong package) and top_level_config_binding? also returns false (pattern misses { config }), so the generator injects const { config } = require('shakapacker') and silently shadows the existing binding.
Extending the pattern to also match destructured declarations would close the gap:
| end | |
| def top_level_config_binding?(content) | |
| pattern = /^[ \t]*(?:(?:const|let|var)\s+(?:config\b|\{[^}]*\bconfig\b)|function\s+config\s*\()/ | |
| content.to_enum(:scan, pattern).any? do | |
| js_top_level_position?(content, Regexp.last_match.begin(0)) | |
| end | |
| end |
Same fix applies to top_level_resolve_binding? — const { resolve } = require('non-path-package') would slip past it today.
- Correct the partition complexity note from O(n × m) to O(n² × m): each
`js_code_position?` call walks from index 0, so every plugin hit triggers a
fresh O(n) rescan. The cost stays acceptable for small webpack configs, but
the note now reflects the actual cost so future contributors don't layer
additional O(n) callers under the impression only one linear pass exists.
- Add a unit-level invariant test for `advance_js_scan_state`'s
`block_comment_exit_guard`: when `*/` is consumed, the returned state must be
nil and the returned index must point at the closing `/`. Several callers
(`matching_js_closing_brace`, `js_code_position?`, `js_top_level_position?`)
rely on this without an explicit `prev_state == :block_comment` guard, so a
regression here would silently misroute downstream `char == '{' / '}' / ')'`
checks. The unit test catches the break at the helper boundary instead of
forcing the diagnosis through a downstream caller misbehaving.
| # NOTE: internally rescans from index 0 for each `new RSCWebpackPlugin(` hit, so this is | ||
| # O(n × m) in content size × plugin count. Acceptable for small webpack configs; carry | ||
| # scanner state forward before adopting this pattern for larger shared inputs. | ||
| def rsc_plugin_option_sections(content, is_server:) |
There was a problem hiding this comment.
The O(n × m) comment is accurate and fine for typical webpack config sizes. Worth noting for future maintainers: rsc_plugin_option_sections_partition is called on the same content string three separate times within update_existing_rsc_webpack_config — via rsc_plugin_sections_safe_to_rewrite?, rsc_plugin_uses_scoped_client_references?, and rewritable_rsc_plugin? — plus once more after a disk re-read in rewrite_rsc_plugin_client_references. If this method is ever extended to batch-migrate many files, threading the partition result through rather than recomputing would be the natural fix.
Round 6 review addressedPushed commit addressing 2 new threads:
Verification:
|
| end | ||
| end | ||
|
|
||
| def inject_rsc_server_imports(config_path, content, existing_imports_content) |
There was a problem hiding this comment.
The asymmetry vs inject_rsc_client_imports (which includes shakapacker_config_import_statement) is intentional and correct — the server anchor already requires config to be in scope. A brief comment here would make the omission self-documenting so a future reader doesn't add it thinking it was forgotten:
| def inject_rsc_server_imports(config_path, content, existing_imports_content) | |
| def inject_rsc_server_imports(config_path, content, existing_imports_content) | |
| # `shakapacker_config_import_statement` is intentionally omitted: the server anchor | |
| # (`config.assets_bundler === 'rspack'`) requires `config` to already be imported, | |
| # so a missing import is caught earlier by `shakapacker_config_blocker_reason`. | |
| replace_rsc_client_references_setup_anchor(config_path, content, is_server: true) do |anchor| |
1 participant
Summary
Fixes #3201
Test plan
Note
Medium Risk
Medium risk because it changes generator rewrite behavior for existing webpack configs and adds a custom lightweight JS parser that could miss edge cases and alter config files unexpectedly.
Overview
Scopes RSC client-reference discovery to application source. Generated RSC webpack configs now pass
clientReferencesderived from Shakapackerconfig.source_path(viarscClientReferences) for both client and serverRSCWebpackPlugininstances, avoiding scans of unrelated project files (e.g.,vendor/bundle) in CI.The
react_on_rails:rscsetup generator now also upgrades existing webpack configs that already includeRSCWebpackPluginby injecting the scopedrscClientReferenceshelper (when safe) and rewriting plugin option objects to addclientReferences: rscClientReferences, with expanded verification/warnings for non-CommonJS or hard-to-parse configs. Templates, docs, and generator specs were updated accordingly.Reviewed by Cursor Bugbot for commit 4122dc9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Bug Fixes
Tests