Conversation
Add a new demo app showcasing React Server Components using: - react_on_rails 16.4.0.rc.2 with RSC support - react_on_rails_pro 16.4.0.rc.2 with Node Renderer - shakapacker 9.6.0.beta.0 - TypeScript - Rails 8.0.3 The demo includes: - HelloServer async server component with streaming - LikeButton client component demonstrating selective hydration - RSC webpack bundle configuration - Node Renderer for SSR Created with: bin/new-demo basic-v16-rsc --rsc --typescript Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Too many files changed for review. ( |
WalkthroughAdds a complete Rails 8 demo app demonstrating React Server Components (RSC) with streaming, build tooling and shakapacker/webpack configs for client/server/RSC bundles, RSC and client components, numerous dev scripts/Procfiles, Playwright E2E scaffolding, initializers, and supporting config and static assets. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Rails as Rails Server
participant RSC as React Server Component
participant NodeRenderer as Node Renderer
Browser->>Rails: GET /hello_server
activate Rails
Rails->>RSC: stream_view_containing_react_components(props)
activate RSC
RSC->>RSC: await fetchGreeting(name)
RSC-->>Rails: stream HTML fragments (server-rendered content)
deactivate RSC
Rails->>NodeRenderer: (optional) renderer requests / SSR tasks
NodeRenderer-->>Rails: renderer responses / assets
Rails-->>Browser: streamed HTML + client JS for LikeButton
deactivate Rails
Browser->>Browser: hydrate LikeButton (client interactivity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a new React Server Components (RSC) demonstration application using react_on_rails 16.4.0-rc.2, react_on_rails_pro 16.4.0-rc.2, and shakapacker 9.6.0-beta.0. The demo showcases an async server component (HelloServer) with streaming, a client component (LikeButton), and includes comprehensive webpack configuration for RSC support.
Changes:
- Created complete Rails 8 application structure with RSC-specific configuration
- Added React Server Components demo with HelloServer async component and LikeButton client component
- Configured webpack for RSC bundle generation with separate client, server, and RSC bundles
Reviewed changes
Copilot reviewed 88 out of 108 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| demos/basic-v16-rsc/package.json | Defines npm dependencies for RSC demo including react-on-rails-rsc and beta packages |
| demos/basic-v16-rsc/Gemfile | Specifies Ruby dependencies with RC versions of react_on_rails gems |
| demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js | Configures RSC-specific webpack bundle with react-server condition |
| demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js | Sets up server-side rendering webpack configuration with RSC plugin |
| demos/basic-v16-rsc/config/webpack/clientWebpackConfig.js | Configures client bundle with RSC webpack plugin |
| demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js | Orchestrates building client, server, and RSC bundles based on environment |
| demos/basic-v16-rsc/config/initializers/react_on_rails_pro.rb | Enables RSC support with bundle and payload configuration |
| demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx | Implements async server component with simulated data fetching |
| demos/basic-v16-rsc/app/javascript/src/HelloServer/components/LikeButton.tsx | Client component with 'use client' directive for interactivity |
| demos/basic-v16-rsc/app/controllers/hello_server_controller.rb | Controller using ReactOnRailsPro::Stream for RSC rendering |
| demos/basic-v16-rsc/config/shakapacker.yml | Shakapacker configuration with private_output_path for SSR bundles |
| demos/basic-v16-rsc/bin/dev | Development server script with RSC-specific default route |
Comments suppressed due to low confidence (1)
demos/basic-v16-rsc/config/routes.rb:1
- Missing documentation for the rsc_payload_route helper method. Consider adding a comment explaining that this is a React on Rails Pro helper that sets up the RSC payload generation endpoint.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@demos/basic-v16-rsc/.github/workflows/ci.yml`:
- Around line 65-80: Add a step to compile JS assets (webpack/shakapacker)
before running Rails tests so React components and assets are available; insert
a new job step between the "Setup database" and "Run tests" steps that runs the
test-environment asset compilation (e.g. RAILS_ENV=test bin/rails
assets:precompile or the project’s shakapacker/webpack compile command) and
ensure the step runs after "Install dependencies" so npm/yarn packages are
installed first.
- Around line 13-27: Update the GitHub Actions step versions to the latest major
releases: replace uses: actions/checkout@v3 with actions/checkout@v4,
ruby/setup-ruby@v1 with ruby/setup-ruby@v2 (or `@v4` if available for that
action), and actions/setup-node@v3 with actions/setup-node@v4 so the workflow
uses supported, non-deprecated action releases; ensure any version-specific
inputs remain compatible with the newer action major versions and run the CI
locally or via a PR to verify no breaking changes in the checkout, setup-ruby,
and setup-node steps.
In
`@demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx`:
- Line 14: The file's top-level import "import React from 'react'" is unused
with the new JSX transform; remove that unused import from HelloServer.tsx (the
import statement referencing React) so only needed imports remain, ensuring no
other references to the React symbol exist in the HelloServer component or its
functions before committing.
In `@demos/basic-v16-rsc/app/views/layouts/application.html.erb`:
- Line 2: The root <html> element in application.html.erb is missing a lang
attribute; update the <html> tag to include a language declaration (e.g.,
lang="en") or, if the app is internationalized, set it dynamically (e.g.,
lang="<%= I18n.locale %>") so assistive technologies receive the page
language—modify the <html> tag in application.html.erb accordingly.
- Line 21: Update the stylesheet include in the layout: replace the current call
to stylesheet_link_tag :app with stylesheet_link_tag :application so Propshaft
references the existing application.css instead of a non-existent app.css;
modify the invocation in the view where stylesheet_link_tag :app is used.
In `@demos/basic-v16-rsc/bin/shakapacker-precompile-hook`:
- Around line 26-34: Guard against Rainbow being unavailable when printing
colored messages: either explicitly require 'rainbow' with a safe rescue or wrap
all uses of Rainbow (the calls in puts Rainbow(...).cyan and warn
Rainbow(...).red) in a small helper that checks defined?(Rainbow) and falls back
to plain strings. Update the precompile hook around
ReactOnRails::PacksGenerator.instance.generate_packs_if_stale to use that helper
for both the startup message and the error message so a missing Rainbow constant
won't raise a NameError and mask the original exception.
In `@demos/basic-v16-rsc/bin/switch-bundler`:
- Line 94: The call that writes package.json uses File.write(package_json_path,
JSON.pretty_generate(package_json)) which drops the trailing newline; change the
write payload to append a single newline (e.g., use the result of
JSON.pretty_generate(package_json) plus "\n") so package_json_path is written
with a trailing newline to satisfy tooling expectations and avoid diff/lint
noise.
In `@demos/basic-v16-rsc/client/node-renderer.js`:
- Around line 37-39: The CI block unconditionally sets config.workersCount = 2,
overriding any explicit NODE_RENDERER_CONCURRENCY; change it to only set a
default when no explicit concurrency is provided—e.g., inside the env.CI branch
check for an existing value (process.env.NODE_RENDERER_CONCURRENCY or
config.workersCount) and only assign 2 if neither is set so env.CI still
triggers a safe default but respects explicit overrides.
In `@demos/basic-v16-rsc/config/initializers/react_on_rails_pro.rb`:
- Line 10: The default renderer password uses a hard-coded fallback
(config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword"));
change it so a missing RENDERER_PASSWORD raises an error outside development:
use ENV.fetch("RENDERER_PASSWORD") (no default) in production/staging and only
allow the "dev" fallback when Rails.env.development? or similar; update the
config assignment for config.renderer_password accordingly so non-development
boots fail with KeyError while development keeps the convenience default.
In `@demos/basic-v16-rsc/config/initializers/react_on_rails.rb`:
- Around line 37-51: Move config.auto_load_bundle and
config.components_subdirectory out from under the "Test Configuration" header by
inserting a clear section separator (e.g., "Component Loading" or similar) above
them so they are no longer presented as test-only settings, and update the
nearby comment that says "ALTERNATIVE APPROACH: Uncomment below" to reflect that
config.build_test_command is intentionally active (change wording to indicate
it's enabled by default or remove the "Uncomment" phrasing); ensure references
to config.build_test_command, config.auto_load_bundle, and
config.components_subdirectory are preserved and clearly grouped under
appropriate headers.
In `@demos/basic-v16-rsc/config/puma.rb`:
- Around line 27-28: The threads_count assignment should parse the ENV value
using Integer to fail fast on invalid input; replace the current
ENV.fetch("RAILS_MAX_THREADS", 3) usage so that threads_count is produced by
Integer(ENV.fetch("RAILS_MAX_THREADS", "3")) (or similar) and then call threads
threads_count, threads_count; update the code that defines threads_count and the
call to threads to use the Integer-parsed value so invalid strings raise
immediately instead of becoming 0.
In `@demos/basic-v16-rsc/config/routes.rb`:
- Around line 1-16: Add a root route so visiting "/" doesn't 404 by mapping the
app root to the existing hello_server controller action; update the routes
(Rails.application.routes.draw) to include a root route pointing to
"hello_server#index" (e.g., add root "hello_server#index") alongside the
existing get 'hello_server', to: 'hello_server#index' and rsc_payload_route.
In `@demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js`:
- Around line 23-27: The code sets rscConfig.entry = rscEntry by reusing
rscConfig.entry['server-bundle'] without verifying that key exists; add a guard
in the block that checks rscConfig.entry and that
rscConfig.entry['server-bundle'] is defined (e.g. assert or throw) before
creating rscEntry, and provide a clear error message referencing the missing
'server-bundle' entry so future changes to serverWebpackConfig fail fast; update
the logic around rscEntry, rscConfig.entry and the 'server-bundle' lookup
accordingly.
In `@demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js`:
- Around line 37-43: The comment above the default branch is outdated: it says
"default is the standard client and server build" but the code now produces
client, server, and RSC bundles; update the comment to reflect that. Edit the
comment near the else block that logs "[React on Rails] Creating client, server,
and RSC bundles." (around the console.log call) so it accurately states that the
default creates clientConfig, serverConfig, and rscConfig (and mention RSC if
desired) to match the behavior of result = [clientConfig, serverConfig,
rscConfig].
- Line 6: The environment-specific callbacks (productionEnvOnly and testOnly)
are missing the third rscConfig parameter while envSpecific calls back with
three arguments; update the function signatures in production.js and test.js to
accept (config, env, rscConfig) like the development.js pattern so RSC-specific
customization can be applied, and propagate or use rscConfig inside
productionEnvOnly/testOnly where appropriate to maintain consistent behavior
with envSpecific.
In `@demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js`:
- Around line 107-125: The code iterates rule.use and assumes each item yields a
string loader, which can throw if item.loader is missing; update both the filter
and the find that compute testValue (used by rule.use.filter and cssLoader) to
defensively handle non-string values by checking typeof testValue === 'string'
before calling match/includes (or by using a safe string coercion). Concretely,
in the rule.use.filter callback and in the cssLoader find callback, ensure
testValue is set only when typeof item === 'string' or typeof item.loader ===
'string', and short-circuit returning false when testValue is not a string so
neither testValue.match(...) nor testValue.includes(...) can be invoked on
undefined.
- Around line 44-50: Remove the redundant first filtering pass that iterates
serverWebpackConfig.module.rules.forEach and strips items matching
/mini-css-extract-plugin/ from loader.use; leave only the later comprehensive
pass that also removes style-loader (the existing logic at the second filtering
block). Specifically, delete the block that calls
serverWebpackConfig.module.rules.forEach(... loader.use = loader.use.filter(...
mini-css-extract-plugin ...)) so there is a single, correct filter run handling
both mini-css-extract-plugin and style-loader.
In `@demos/basic-v16-rsc/e2e/app_commands/activerecord_fixtures.rb`:
- Around line 16-22: Remove the dead fallback else branch that references the
non-existent Post model to avoid a potential NameError: delete the entire else
... end block (which contains logger.error and Post.create calls) and leave only
the active ActiveRecord handling; also fix the typo in the logger message by
changing "suite" to "suit" if you keep any logging text (reference symbols: the
else branch, logger.error, and Post.create).
In `@demos/basic-v16-rsc/e2e/app_commands/log_fail.rb`:
- Around line 2-5: The shell command interpolates an unsanitized `filename`
(derived via `command_options.fetch('runnable_full_title', ...)`) into a
`system` call, enabling command injection; fix by escaping the filename with
Shellwords.shellescape (or avoid shelling out entirely and implement log
extraction in Ruby using File/IO methods) before passing it to the `system`
invocation that runs "tail ... > 'log/#{filename}.log'"; also ensure any
platform-specific reverse-tail logic (the use of `tail -r`/`tac`) is handled in
the Ruby implementation or via runtime platform detection so CI/Linux behaves
correctly.
- Around line 16-19: The silent rescue around the block that populates records
(the begin ... rescue ... end wrapping records[record_class.to_s] =
record_class.limit(100).map(&:attributes)) should be changed to capture the
exception (use rescue => e) and emit a warning including the record_class name
and the exception message/backtrace (e.g., via an existing logger or
STDERR.puts) so failures to read a table are visible; keep the original behavior
of skipping the failing table but ensure the log provides context
(record_class.to_s) and e.inspect or e.message for debugging.
- Line 23: Remove the duplicate computation of filename: the expression
"filename = command_options.fetch('runnable_full_title', 'no
title').gsub(/[^[:print:]]/, '')" is already assigned earlier, so delete this
redundant reassignment and keep the original assignment (referencing the
filename variable and the command_options.fetch(...) call) to avoid
recalculation and potential inconsistencies.
In `@demos/basic-v16-rsc/e2e/playwright.config.js`:
- Line 68: The commented-out Playwright configuration line containing "use: {
..devices['Desktop Chrome'], channel: 'chrome' }" has a typo in the spread
operator; replace the two-dot "..devices" with the correct three-dot spread
"...devices" (i.e., change ..devices to ...devices) so uncommenting that line
won't cause a syntax error.
- Line 28: The hardcoded baseURL value ('baseURL' in the Playwright config) uses
port 5017 which conflicts with the root config using port 3000; update the
baseURL to read from the environment (process.env.BASE_URL) with a 3000 default
(e.g., process.env.BASE_URL || 'http://localhost:3000' or
'http://127.0.0.1:3000') so it aligns with the root playwright.config.ts and
bin/dev settings; change the baseURL entry in
demos/basic-v16-rsc/e2e/playwright.config.js accordingly.
In
`@demos/basic-v16-rsc/e2e/playwright/e2e/rails_examples/using_scenarios.spec.js`:
- Line 11: The test has its closing brace and parenthesis on the same line as
the last statement (`await page.goto("/")`), so move the closing `});` onto its
own line to match the expected formatting; locate the test function in
using_scenarios.spec.js around the `await page.goto("/")` call and insert a
newline before the `});` so the block ends with its own line.
In `@demos/basic-v16-rsc/e2e/playwright/support/index.js`:
- Around line 1-21: The top comment block in support/index.js incorrectly
references Cypress (supportFile, on.cypress.io/configuration) in a Playwright
project; update or remove that block to avoid confusion by replacing
Cypress-specific text with Playwright-centric guidance or deleting it entirely —
locate the header comment above the import './on-rails' line and either rewrite
it to mention Playwright/global test setup and relevant Playwright docs or
remove the stale comments while keeping the import intact.
In `@demos/basic-v16-rsc/e2e/playwright/support/on-rails.js`:
- Line 23: The forEach callback currently uses a ternary that returns an unused
empty object; change the callback in Object.keys(options).forEach(...) to a
void-returning form by replacing the ternary with a simple conditional: iterate
keys from Object.keys(options) and if options[key] === undefined then delete
options[key]; keep the same variables (options and key) and the forEach call so
behavior is unchanged but the callback no longer returns a value.
- Line 11: The code returns the Playwright APIResponse method reference instead
of its result — replace all occurrences of returning response.body with await
response.body() (or await response.json() if the backend returns JSON) so the
function returns the actual Buffer/parsed JSON; update the instances referenced
in this file (the returns at the spots currently using response.body on lines
corresponding to the earlier return and the two other occurrences) and ensure
callers that access body[0] operate on the Buffer/ArrayBuffer or parsed JSON
accordingly.
In `@demos/basic-v16-rsc/lefthook.yml`:
- Around line 38-55: Remove the tests-rspec and bundle-audit hook blocks from
lefthook.yml (delete the entire entries named "tests-rspec" and "bundle-audit"
including their tags, run and skip keys) so hooks no longer reference the
missing rspec and bundler-audit gems; keep the remaining hooks (e.g.,
"tests-jest") untouched and validate the YAML after removal.
- Around line 7-18: Update the lefthook.yml hook commands (rubocop, eslint,
prettier) to run under the Conductor wrapper by prefixing their existing
commands with bin/conductor-exec so they use the .tool-versions Ruby/Node
runtimes; specifically change the rubocop run to use "bin/conductor-exec bundle
exec rubocop...", the eslint run to "bin/conductor-exec npx eslint...", and the
prettier run to "bin/conductor-exec npx prettier..." (keep the same staged_files
usage and git add behavior).
In `@demos/basic-v16-rsc/package.json`:
- Around line 9-28: The package.json dependencies list mixes exact pins and
caret ranges (e.g., "css-loader": "7.1.4" vs "webpack": "^5.105.2"); update the
dependencies in package.json to use a consistent versioning strategy (either pin
all to exact versions or use caret/tilde ranges) by normalizing entries like
"css-loader", "mini-css-extract-plugin", "sass-loader", "webpack",
"webpack-cli", etc., so every dependency follows the chosen convention for
reproducible installs.
- Line 14: Remove the unnecessary "prop-types" dependency from package.json
(delete the "prop-types": "15.8.1" entry), uninstall it from the lockfile (run
npm/yarn remove so lockfile is updated), and search the codebase for any usage
or imports like "import PropTypes from 'prop-types'" and remove those prop-types
usages or replace them with TypeScript types (update any test/build scripts if
they reference prop-types).
In `@demos/basic-v16-rsc/public/406-unsupported-browser.html`:
- Around line 85-93: The CSS uses a nested `@media` inside the selector "main
article br", which may not work in older browsers targeted by this page; move
the media query out of the nested rule so the breakpoint applies universally —
i.e., create a top-level `@media` (min-width: 48em) block and place the "main
article br { display: inline; }" rule there while keeping the default "main
article br { display: none; }" outside to preserve responsive behavior.
In `@demos/basic-v16-rsc/README.md`:
- Around line 1-24: The README currently contains the stock Rails scaffold text;
replace it with concrete demo setup instructions that cover required Ruby
version, system dependencies, configuration, DB creation/migration, and the
known workaround steps mentioned in the PR (e.g., run ruby
bin/shakapacker-precompile-hook once before running bin/dev), plus how to start
the app (bin/dev), run tests, and any service or deployment notes; update the
README title and sections to include these specifics and concise command
examples so a user can get the basic demo running without reading the PR.
🧹 Nitpick comments (19)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/.github/workflows/ci.yml`: - Around line 13-27: Update the GitHub Actions step versions to the latest major releases: replace uses: actions/checkout@v3 with actions/checkout@v4, ruby/setup-ruby@v1 with ruby/setup-ruby@v2 (or `@v4` if available for that action), and actions/setup-node@v3 with actions/setup-node@v4 so the workflow uses supported, non-deprecated action releases; ensure any version-specific inputs remain compatible with the newer action major versions and run the CI locally or via a PR to verify no breaking changes in the checkout, setup-ruby, and setup-node steps. In `@demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx`: - Line 14: The file's top-level import "import React from 'react'" is unused with the new JSX transform; remove that unused import from HelloServer.tsx (the import statement referencing React) so only needed imports remain, ensuring no other references to the React symbol exist in the HelloServer component or its functions before committing. In `@demos/basic-v16-rsc/bin/shakapacker-precompile-hook`: - Around line 26-34: Guard against Rainbow being unavailable when printing colored messages: either explicitly require 'rainbow' with a safe rescue or wrap all uses of Rainbow (the calls in puts Rainbow(...).cyan and warn Rainbow(...).red) in a small helper that checks defined?(Rainbow) and falls back to plain strings. Update the precompile hook around ReactOnRails::PacksGenerator.instance.generate_packs_if_stale to use that helper for both the startup message and the error message so a missing Rainbow constant won't raise a NameError and mask the original exception. In `@demos/basic-v16-rsc/bin/switch-bundler`: - Line 94: The call that writes package.json uses File.write(package_json_path, JSON.pretty_generate(package_json)) which drops the trailing newline; change the write payload to append a single newline (e.g., use the result of JSON.pretty_generate(package_json) plus "\n") so package_json_path is written with a trailing newline to satisfy tooling expectations and avoid diff/lint noise. In `@demos/basic-v16-rsc/client/node-renderer.js`: - Around line 37-39: The CI block unconditionally sets config.workersCount = 2, overriding any explicit NODE_RENDERER_CONCURRENCY; change it to only set a default when no explicit concurrency is provided—e.g., inside the env.CI branch check for an existing value (process.env.NODE_RENDERER_CONCURRENCY or config.workersCount) and only assign 2 if neither is set so env.CI still triggers a safe default but respects explicit overrides. In `@demos/basic-v16-rsc/config/initializers/react_on_rails.rb`: - Around line 37-51: Move config.auto_load_bundle and config.components_subdirectory out from under the "Test Configuration" header by inserting a clear section separator (e.g., "Component Loading" or similar) above them so they are no longer presented as test-only settings, and update the nearby comment that says "ALTERNATIVE APPROACH: Uncomment below" to reflect that config.build_test_command is intentionally active (change wording to indicate it's enabled by default or remove the "Uncomment" phrasing); ensure references to config.build_test_command, config.auto_load_bundle, and config.components_subdirectory are preserved and clearly grouped under appropriate headers. In `@demos/basic-v16-rsc/config/routes.rb`: - Around line 1-16: Add a root route so visiting "/" doesn't 404 by mapping the app root to the existing hello_server controller action; update the routes (Rails.application.routes.draw) to include a root route pointing to "hello_server#index" (e.g., add root "hello_server#index") alongside the existing get 'hello_server', to: 'hello_server#index' and rsc_payload_route. In `@demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js`: - Around line 23-27: The code sets rscConfig.entry = rscEntry by reusing rscConfig.entry['server-bundle'] without verifying that key exists; add a guard in the block that checks rscConfig.entry and that rscConfig.entry['server-bundle'] is defined (e.g. assert or throw) before creating rscEntry, and provide a clear error message referencing the missing 'server-bundle' entry so future changes to serverWebpackConfig fail fast; update the logic around rscEntry, rscConfig.entry and the 'server-bundle' lookup accordingly. In `@demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js`: - Around line 37-43: The comment above the default branch is outdated: it says "default is the standard client and server build" but the code now produces client, server, and RSC bundles; update the comment to reflect that. Edit the comment near the else block that logs "[React on Rails] Creating client, server, and RSC bundles." (around the console.log call) so it accurately states that the default creates clientConfig, serverConfig, and rscConfig (and mention RSC if desired) to match the behavior of result = [clientConfig, serverConfig, rscConfig]. - Line 6: The environment-specific callbacks (productionEnvOnly and testOnly) are missing the third rscConfig parameter while envSpecific calls back with three arguments; update the function signatures in production.js and test.js to accept (config, env, rscConfig) like the development.js pattern so RSC-specific customization can be applied, and propagate or use rscConfig inside productionEnvOnly/testOnly where appropriate to maintain consistent behavior with envSpecific. In `@demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js`: - Around line 44-50: Remove the redundant first filtering pass that iterates serverWebpackConfig.module.rules.forEach and strips items matching /mini-css-extract-plugin/ from loader.use; leave only the later comprehensive pass that also removes style-loader (the existing logic at the second filtering block). Specifically, delete the block that calls serverWebpackConfig.module.rules.forEach(... loader.use = loader.use.filter(... mini-css-extract-plugin ...)) so there is a single, correct filter run handling both mini-css-extract-plugin and style-loader. In `@demos/basic-v16-rsc/e2e/app_commands/activerecord_fixtures.rb`: - Around line 16-22: Remove the dead fallback else branch that references the non-existent Post model to avoid a potential NameError: delete the entire else ... end block (which contains logger.error and Post.create calls) and leave only the active ActiveRecord handling; also fix the typo in the logger message by changing "suite" to "suit" if you keep any logging text (reference symbols: the else branch, logger.error, and Post.create). In `@demos/basic-v16-rsc/e2e/app_commands/log_fail.rb`: - Around line 16-19: The silent rescue around the block that populates records (the begin ... rescue ... end wrapping records[record_class.to_s] = record_class.limit(100).map(&:attributes)) should be changed to capture the exception (use rescue => e) and emit a warning including the record_class name and the exception message/backtrace (e.g., via an existing logger or STDERR.puts) so failures to read a table are visible; keep the original behavior of skipping the failing table but ensure the log provides context (record_class.to_s) and e.inspect or e.message for debugging. - Line 23: Remove the duplicate computation of filename: the expression "filename = command_options.fetch('runnable_full_title', 'no title').gsub(/[^[:print:]]/, '')" is already assigned earlier, so delete this redundant reassignment and keep the original assignment (referencing the filename variable and the command_options.fetch(...) call) to avoid recalculation and potential inconsistencies. In `@demos/basic-v16-rsc/e2e/playwright/support/index.js`: - Around line 1-21: The top comment block in support/index.js incorrectly references Cypress (supportFile, on.cypress.io/configuration) in a Playwright project; update or remove that block to avoid confusion by replacing Cypress-specific text with Playwright-centric guidance or deleting it entirely — locate the header comment above the import './on-rails' line and either rewrite it to mention Playwright/global test setup and relevant Playwright docs or remove the stale comments while keeping the import intact. In `@demos/basic-v16-rsc/lefthook.yml`: - Around line 7-18: Update the lefthook.yml hook commands (rubocop, eslint, prettier) to run under the Conductor wrapper by prefixing their existing commands with bin/conductor-exec so they use the .tool-versions Ruby/Node runtimes; specifically change the rubocop run to use "bin/conductor-exec bundle exec rubocop...", the eslint run to "bin/conductor-exec npx eslint...", and the prettier run to "bin/conductor-exec npx prettier..." (keep the same staged_files usage and git add behavior). In `@demos/basic-v16-rsc/package.json`: - Around line 9-28: The package.json dependencies list mixes exact pins and caret ranges (e.g., "css-loader": "7.1.4" vs "webpack": "^5.105.2"); update the dependencies in package.json to use a consistent versioning strategy (either pin all to exact versions or use caret/tilde ranges) by normalizing entries like "css-loader", "mini-css-extract-plugin", "sass-loader", "webpack", "webpack-cli", etc., so every dependency follows the chosen convention for reproducible installs. - Line 14: Remove the unnecessary "prop-types" dependency from package.json (delete the "prop-types": "15.8.1" entry), uninstall it from the lockfile (run npm/yarn remove so lockfile is updated), and search the codebase for any usage or imports like "import PropTypes from 'prop-types'" and remove those prop-types usages or replace them with TypeScript types (update any test/build scripts if they reference prop-types). In `@demos/basic-v16-rsc/README.md`: - Around line 1-24: The README currently contains the stock Rails scaffold text; replace it with concrete demo setup instructions that cover required Ruby version, system dependencies, configuration, DB creation/migration, and the known workaround steps mentioned in the PR (e.g., run ruby bin/shakapacker-precompile-hook once before running bin/dev), plus how to start the app (bin/dev), run tests, and any service or deployment notes; update the README title and sections to include these specifics and concise command examples so a user can get the basic demo running without reading the PR.demos/basic-v16-rsc/e2e/app_commands/activerecord_fixtures.rb (1)
16-22: Deadelsebranch references aPostmodel that likely doesn't exist in this demo.The comment on line 16 already acknowledges this can be removed. Since the RSC demo doesn't appear to have a
Postmodel, this fallback would raise aNameErrorat runtime ifActiveRecordwere somehow undefined. Consider removing it to avoid confusion.Also, minor typo on line 17: "suite" → "suit".
♻️ Suggested cleanup
-else # this else part can be removed - logger.error "Looks like activerecord_fixtures has to be modified to suite your need" - Post.create(title: 'MyCypressFixtures') - Post.create(title: 'MyCypressFixtures2') - Post.create(title: 'MyRailsFixtures') - Post.create(title: 'MyRailsFixtures2') end🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/e2e/app_commands/activerecord_fixtures.rb` around lines 16 - 22, Remove the dead fallback else branch that references the non-existent Post model to avoid a potential NameError: delete the entire else ... end block (which contains logger.error and Post.create calls) and leave only the active ActiveRecord handling; also fix the typo in the logger message by changing "suite" to "suit" if you keep any logging text (reference symbols: the else branch, logger.error, and Post.create).demos/basic-v16-rsc/e2e/app_commands/log_fail.rb (2)
16-19: Barerescueswallows all exceptions silently.A bare
rescuecatches onlyStandardErrorby default in Ruby (which is fine), but the empty body means failures are completely invisible. Consider logging a warning so debugging isn't a mystery when a table can't be read.Proposed improvement
begin records[record_class.to_s] = record_class.limit(100).map(&:attributes) - rescue + rescue StandardError => e + records[record_class.to_s] = "Error: #{e.message}" end🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/e2e/app_commands/log_fail.rb` around lines 16 - 19, The silent rescue around the block that populates records (the begin ... rescue ... end wrapping records[record_class.to_s] = record_class.limit(100).map(&:attributes)) should be changed to capture the exception (use rescue => e) and emit a warning including the record_class name and the exception message/backtrace (e.g., via an existing logger or STDERR.puts) so failures to read a table are visible; keep the original behavior of skipping the failing table but ensure the log provides context (record_class.to_s) and e.inspect or e.message for debugging.
23-23: Duplicatefilenamecomputation — already assigned on line 2.This exact expression is evaluated identically on line 2. Remove the redundant reassignment.
Proposed fix
-filename = command_options.fetch('runnable_full_title', 'no title').gsub(/[^[:print:]]/, '') File.open("#{Rails.root}/log/#{filename}.json", "w+") do |file|🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/e2e/app_commands/log_fail.rb` at line 23, Remove the duplicate computation of filename: the expression "filename = command_options.fetch('runnable_full_title', 'no title').gsub(/[^[:print:]]/, '')" is already assigned earlier, so delete this redundant reassignment and keep the original assignment (referencing the filename variable and the command_options.fetch(...) call) to avoid recalculation and potential inconsistencies.demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js (2)
37-43: Stale comment on line 38.The comment says "default is the standard client and server build" but the default now produces client, server, and RSC bundles (line 42). The
console.logon line 41 is correct; update the comment to match.Proposed fix
} else { - // default is the standard client and server build + // default is the standard client, server, and RSC build🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js` around lines 37 - 43, The comment above the default branch is outdated: it says "default is the standard client and server build" but the code now produces client, server, and RSC bundles; update the comment to reflect that. Edit the comment near the else block that logs "[React on Rails] Creating client, server, and RSC bundles." (around the console.log call) so it accurately states that the default creates clientConfig, serverConfig, and rscConfig (and mention RSC if desired) to match the behavior of result = [clientConfig, serverConfig, rscConfig].
6-6:production.jsandtest.jscallbacks don't accept therscConfigparameter.
envSpecificis invoked with three arguments (line 17), butproductionEnvOnlyandtestOnlyinproduction.js/test.jsonly declare two parameters. This won't break at runtime (JS silently drops extra args), but it means environment-specific RSC customization is silently impossible in those files. Consider updating their signatures to include the thirdrscConfigparameter for consistency withdevelopment.jspatterns.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/webpack/ServerClientOrBoth.js` at line 6, The environment-specific callbacks (productionEnvOnly and testOnly) are missing the third rscConfig parameter while envSpecific calls back with three arguments; update the function signatures in production.js and test.js to accept (config, env, rscConfig) like the development.js pattern so RSC-specific customization can be applied, and propagate or use rscConfig inside productionEnvOnly/testOnly where appropriate to maintain consistent behavior with envSpecific.demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js (1)
44-50: Duplicate mini-css-extract-plugin filtering.Lines 44–50 filter out
mini-css-extract-pluginfrom loader rules, but the same filtering (plusstyle-loaderremoval) is performed again at lines 107–115. The first pass is redundant and can be removed.Proposed fix
serverWebpackConfig.entry = serverEntry; - // Remove the mini-css-extract-plugin from the style loaders because - // the client build will handle exporting CSS. - // replace file-loader with null-loader - serverWebpackConfig.module.rules.forEach((loader) => { - if (loader.use && loader.use.filter) { - loader.use = loader.use.filter( - (item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)), - ); - } - }); - // No splitting of chunks for a server bundle🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js` around lines 44 - 50, Remove the redundant first filtering pass that iterates serverWebpackConfig.module.rules.forEach and strips items matching /mini-css-extract-plugin/ from loader.use; leave only the later comprehensive pass that also removes style-loader (the existing logic at the second filtering block). Specifically, delete the block that calls serverWebpackConfig.module.rules.forEach(... loader.use = loader.use.filter(... mini-css-extract-plugin ...)) so there is a single, correct filter run handling both mini-css-extract-plugin and style-loader.demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js (1)
23-27: Consider a guard for the expected entry key.If
serverWebpackConfigever changes its entry key away from'server-bundle', this would silently produce anundefinedentry. A small assertion would make future debugging easier.💡 Optional defensive check
+ const serverEntry = rscConfig.entry['server-bundle']; + if (!serverEntry) { + throw new Error( + 'rscWebpackConfig: expected server config to have a "server-bundle" entry' + ); + } const rscEntry = { - 'rsc-bundle': rscConfig.entry['server-bundle'], + 'rsc-bundle': serverEntry, };🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js` around lines 23 - 27, The code sets rscConfig.entry = rscEntry by reusing rscConfig.entry['server-bundle'] without verifying that key exists; add a guard in the block that checks rscConfig.entry and that rscConfig.entry['server-bundle'] is defined (e.g. assert or throw) before creating rscEntry, and provide a clear error message referencing the missing 'server-bundle' entry so future changes to serverWebpackConfig fail fast; update the logic around rscEntry, rscConfig.entry and the 'server-bundle' lookup accordingly.demos/basic-v16-rsc/e2e/playwright/support/index.js (1)
1-21: Stale Cypress references in comments — this is a Playwright support file.The comment block (Lines 1–14) references Cypress configuration,
supportFile, andhttps://on.cypress.io/configuration, but this is a Playwright project. These are leftover from the CypressOnRails generator template and could confuse contributors.Consider updating the comments to reflect the actual framework in use, or simply removing them.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/e2e/playwright/support/index.js` around lines 1 - 21, The top comment block in support/index.js incorrectly references Cypress (supportFile, on.cypress.io/configuration) in a Playwright project; update or remove that block to avoid confusion by replacing Cypress-specific text with Playwright-centric guidance or deleting it entirely — locate the header comment above the import './on-rails' line and either rewrite it to mention Playwright/global test setup and relevant Playwright docs or remove the stale comments while keeping the import intact.demos/basic-v16-rsc/README.md (1)
1-24: Replace the default Rails README with actual demo setup instructions.This is the stock Rails scaffold README. The PR description mentions specific setup steps and known workarounds (e.g., running
ruby bin/shakapacker-precompile-hookbeforebin/devon first run). Consider documenting these here so users of the demo can get started without referring to the PR.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/README.md` around lines 1 - 24, The README currently contains the stock Rails scaffold text; replace it with concrete demo setup instructions that cover required Ruby version, system dependencies, configuration, DB creation/migration, and the known workaround steps mentioned in the PR (e.g., run ruby bin/shakapacker-precompile-hook once before running bin/dev), plus how to start the app (bin/dev), run tests, and any service or deployment notes; update the README title and sections to include these specifics and concise command examples so a user can get the basic demo running without reading the PR.demos/basic-v16-rsc/package.json (2)
9-28: Inconsistent version pinning across dependencies.Some dependencies use exact versions (e.g.,
"css-loader": "7.1.4"), while others use caret ranges (e.g.,"webpack": "^5.105.2"). This is fine for a demo, but worth noting for reproducibility.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/package.json` around lines 9 - 28, The package.json dependencies list mixes exact pins and caret ranges (e.g., "css-loader": "7.1.4" vs "webpack": "^5.105.2"); update the dependencies in package.json to use a consistent versioning strategy (either pin all to exact versions or use caret/tilde ranges) by normalizing entries like "css-loader", "mini-css-extract-plugin", "sass-loader", "webpack", "webpack-cli", etc., so every dependency follows the chosen convention for reproducible installs.
14-14:prop-typesis likely unnecessary in a React 19 demo.React 19 removed
propTypessupport for function components. Since this is a new TypeScript RSC demo, TypeScript provides compile-time type checking, making this dependency redundant.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/package.json` at line 14, Remove the unnecessary "prop-types" dependency from package.json (delete the "prop-types": "15.8.1" entry), uninstall it from the lockfile (run npm/yarn remove so lockfile is updated), and search the codebase for any usage or imports like "import PropTypes from 'prop-types'" and remove those prop-types usages or replace them with TypeScript types (update any test/build scripts if they reference prop-types).demos/basic-v16-rsc/bin/shakapacker-precompile-hook (1)
26-34: Rainbow usage assumes it's loaded via Rails environment — consider a fallback.
Rainbowis used for colored output (lines 29, 32) but isn't explicitly required. It's available becausereact_on_railsdepends on it, but ifconfig/environmentloads successfully yet Rainbow is somehow unavailable (e.g., dependency changes in a future version), the rescue block itself would raise aNameError, masking the original error.A simple guard would make this more resilient:
🛡️ Optional: defensive fallback
begin - puts Rainbow("🔄 Running React on Rails precompile hook...").cyan + puts defined?(Rainbow) ? Rainbow("🔄 Running React on Rails precompile hook...").cyan : "🔄 Running React on Rails precompile hook..." ReactOnRails::PacksGenerator.instance.generate_packs_if_stale rescue StandardError => e - warn Rainbow("❌ Error in precompile hook: #{e.message}").red + warn defined?(Rainbow) ? Rainbow("❌ Error in precompile hook: #{e.message}").red : "❌ Error in precompile hook: #{e.message}" warn e.backtrace.first(5).join("\n") exit 1 end🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/bin/shakapacker-precompile-hook` around lines 26 - 34, Guard against Rainbow being unavailable when printing colored messages: either explicitly require 'rainbow' with a safe rescue or wrap all uses of Rainbow (the calls in puts Rainbow(...).cyan and warn Rainbow(...).red) in a small helper that checks defined?(Rainbow) and falls back to plain strings. Update the precompile hook around ReactOnRails::PacksGenerator.instance.generate_packs_if_stale to use that helper for both the startup message and the error message so a missing Rainbow constant won't raise a NameError and mask the original exception.demos/basic-v16-rsc/lefthook.yml (1)
7-18: Consider usingbin/conductor-execfor Ruby/Node commands if running in Conductor.The hooks invoke
bundle exec rubocopandnpxdirectly. If developers use the Conductor environment, these should be wrapped withbin/conductor-execto ensure the correct Ruby and Node versions from.tool-versionsare used. If Conductor is not used for this demo, this is fine as-is.Based on learnings: "When running pre-commit hooks (lefthook) in Conductor, use bin/conductor-exec wrapper script to ensure correct Ruby and Node versions are available."
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/lefthook.yml` around lines 7 - 18, Update the lefthook.yml hook commands (rubocop, eslint, prettier) to run under the Conductor wrapper by prefixing their existing commands with bin/conductor-exec so they use the .tool-versions Ruby/Node runtimes; specifically change the rubocop run to use "bin/conductor-exec bundle exec rubocop...", the eslint run to "bin/conductor-exec npx eslint...", and the prettier run to "bin/conductor-exec npx prettier..." (keep the same staged_files usage and git add behavior).demos/basic-v16-rsc/client/node-renderer.js (1)
37-39: CI override unconditionally replacesworkersCount.If someone explicitly sets
NODE_RENDERER_CONCURRENCYin CI, this block still overrides it to2. This is likely fine for a demo, but worth noting.Optional: respect explicit override in CI
if (env.CI) { - config.workersCount = 2; + config.workersCount = env.NODE_RENDERER_CONCURRENCY != null ? Number(env.NODE_RENDERER_CONCURRENCY) : 2; }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/client/node-renderer.js` around lines 37 - 39, The CI block unconditionally sets config.workersCount = 2, overriding any explicit NODE_RENDERER_CONCURRENCY; change it to only set a default when no explicit concurrency is provided—e.g., inside the env.CI branch check for an existing value (process.env.NODE_RENDERER_CONCURRENCY or config.workersCount) and only assign 2 if neither is set so env.CI still triggers a safe default but respects explicit overrides.demos/basic-v16-rsc/.github/workflows/ci.yml (1)
13-27: Outdated GitHub Actions versions — consider upgrading to v4.
actions/checkout@v3,ruby/setup-ruby@v1, andactions/setup-node@v3all have newer major versions available (v4). The v3 versions still work but will eventually be deprecated.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/.github/workflows/ci.yml` around lines 13 - 27, Update the GitHub Actions step versions to the latest major releases: replace uses: actions/checkout@v3 with actions/checkout@v4, ruby/setup-ruby@v1 with ruby/setup-ruby@v2 (or `@v4` if available for that action), and actions/setup-node@v3 with actions/setup-node@v4 so the workflow uses supported, non-deprecated action releases; ensure any version-specific inputs remain compatible with the newer action major versions and run the CI locally or via a PR to verify no breaking changes in the checkout, setup-ruby, and setup-node steps.demos/basic-v16-rsc/config/routes.rb (1)
1-16: No root route defined — visitors to/will get a 404.Since
hello_serveris the only page in this demo, consider adding a root route to avoid confusion for anyone launching the app and navigating tohttp://localhost:3000/.Suggested fix
- # Defines the root path route ("/") - # root "posts#index" + # Defines the root path route ("/") + root to: redirect('/hello_server')🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/routes.rb` around lines 1 - 16, Add a root route so visiting "/" doesn't 404 by mapping the app root to the existing hello_server controller action; update the routes (Rails.application.routes.draw) to include a root route pointing to "hello_server#index" (e.g., add root "hello_server#index") alongside the existing get 'hello_server', to: 'hello_server#index' and rsc_payload_route.demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx (1)
14-14: UnusedReactimport with the new JSX transform.With the modern JSX transform (required for React 19),
import React from 'react'is no longer needed unless you explicitly referenceReact(e.g.,React.createElement). This component doesn't, so the import can be removed.Trivial for a demo — feel free to ignore.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx` at line 14, The file's top-level import "import React from 'react'" is unused with the new JSX transform; remove that unused import from HelloServer.tsx (the import statement referencing React) so only needed imports remain, ensuring no other references to the React symbol exist in the HelloServer component or its functions before committing.demos/basic-v16-rsc/bin/switch-bundler (1)
94-94:JSON.pretty_generatedrops the trailing newline frompackage.json.Most JS tooling (prettier, npm, yarn) expects a trailing newline in
package.json.JSON.pretty_generatedoesn't append one, so this will cause a diff noise or linting failure.Suggested fix
- File.write(package_json_path, JSON.pretty_generate(package_json)) + File.write(package_json_path, "#{JSON.pretty_generate(package_json)}\n")🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/bin/switch-bundler` at line 94, The call that writes package.json uses File.write(package_json_path, JSON.pretty_generate(package_json)) which drops the trailing newline; change the write payload to append a single newline (e.g., use the result of JSON.pretty_generate(package_json) plus "\n") so package_json_path is written with a trailing newline to satisfy tooling expectations and avoid diff/lint noise.demos/basic-v16-rsc/config/initializers/react_on_rails.rb (1)
37-51:auto_load_bundleandcomponents_subdirectoryare placed under the "Test Configuration" section header.Lines 50-51 configure component loading, which is unrelated to testing. They sit between the "Test Configuration" header (Line 34) and the "Advanced Configuration" header (Line 52), making it look like they're part of test config. Additionally, the comment on Line 43 says "ALTERNATIVE APPROACH: Uncomment below" but
build_test_commandon Line 48 is already uncommented—so the comment is misleading.Consider adding a section separator before these two settings, and updating the comment to reflect that
build_test_commandis intentionally active.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@demos/basic-v16-rsc/config/initializers/react_on_rails.rb` around lines 37 - 51, Move config.auto_load_bundle and config.components_subdirectory out from under the "Test Configuration" header by inserting a clear section separator (e.g., "Component Loading" or similar) above them so they are no longer presented as test-only settings, and update the nearby comment that says "ALTERNATIVE APPROACH: Uncomment below" to reflect that config.build_test_command is intentionally active (change wording to indicate it's enabled by default or remove the "Uncomment" phrasing); ensure references to config.build_test_command, config.auto_load_bundle, and config.components_subdirectory are preserved and clearly grouped under appropriate headers.
justin808
changed the title
PR Review: Add RSC demo with react_on_rails 16.4.0-rc.5Great addition showcasing React Server Components! The implementation is well-structured with clear documentation and a good separation of server/client components. A few issues need attention before merge: Critical1. Nested
2. Missing The CI workflow runs Bugs3. Potential In the CSS loader filtering logic (~lines 107–125), Minor4. Outdated GitHub Actions versions All workflow steps use 5. Both 6.
Positives
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 109 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/templates/e2e.rake:1
- The relative path traverses up four directory levels, making it brittle and dependent on specific directory structure. Consider using a gem-relative path or Rails.root-based path for better maintainability. For example:
require 'shakacode_demo_common/e2e_test_runner'if the gem is properly structured, or document why this specific path structure is required.
demos/basic-v16-rsc/lib/tasks/e2e.rake:1 - Same issue as the template file - the relative path with four parent directory traversals is fragile. Consider using Rails.root or a more stable path resolution mechanism.
demos/basic-v16-rsc/e2e/playwright.config.js:1 - Corrected spread operator syntax from '..' to '...'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Review: Add RSC demo with react_on_rails 16.4.0-rc.5Great addition to the demo suite! The three-bundle architecture (client/server/rsc), streaming SSR via A few issues to address before merging: Bugs1. E2E test targets the wrong demo entirely 2. Node.js/npm version mismatch in CI 3. Unused Minor Issues4. Outdated GitHub Actions versions 5. No root route defined 6. Two separate Playwright configs with different ports |
Review: Add RSC demo with react_on_rails 16.4.0-rc.5Overall this is a well-structured demo that clearly illustrates RSC concepts — the component split between A few issues need attention before merging: Bugs (will break CI / tests)
Security
CI hygiene
Code quality
Minor
|
|
Addressed all requested review items in commit 37f4534 (CI workflow updates, RSC e2e test rewrite, renderer password hardening, middleware tightening, webpack loader guards, docs/lefthook cleanup, and related lint/syntax fixes). I also resolved all open review threads so only new feedback should remain.\n\nNote: I validated changed JS files with eslint + node syntax checks and changed Ruby files with Syntax OK; full Rails test execution wasn’t possible in this environment due missing local Rails 8 bundle/runtime. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
demos/basic-v16-rsc/.github/workflows/ci.yml (1)
82-86: JavaScript bundle build step addresses prior feedback. Minor redundancy in environment variable.The shakapacker build step properly compiles assets before tests run. However,
RAILS_ENV=testis set both in theenv:block (line 84) and inline in therun:command (line 86). The inline setting is redundant.♻️ Optional: Remove redundant inline env var
- name: Build JavaScript bundles env: RAILS_ENV: test DATABASE_URL: postgresql://postgres:postgres@localhost:5432/test - run: RAILS_ENV=test bin/shakapacker + run: bin/shakapacker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/basic-v16-rsc/.github/workflows/ci.yml` around lines 82 - 86, The Build JavaScript bundles step sets RAILS_ENV twice; remove the redundant inline prefix in the run command so the step uses the env: block value. Edit the step with name "Build JavaScript bundles" to keep env: RAILS_ENV: test and DATABASE_URL as-is and change the run from "RAILS_ENV=test bin/shakapacker" to just "bin/shakapacker" (refer to the step name and the run command to locate the change).demos/basic-v16-rsc/e2e/hello_world.spec.ts (1)
10-15: Consider usinggetByText()for consistency with modern Playwright patterns.The test correctly uses
getByRole()for the button (line 10), but then uses the legacytext=selector syntax for text assertions. UsinggetByText()would be more consistent and readable.♻️ Suggested improvement
const likeButton = page.getByRole('button', { name: '👍 Like' }); await expect(likeButton).toBeVisible(); - await expect(page.locator('text=0 likes')).toBeVisible(); + await expect(page.getByText('0 likes')).toBeVisible(); await likeButton.click(); - await expect(page.locator('text=1 like')).toBeVisible(); + await expect(page.getByText('1 like')).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/basic-v16-rsc/e2e/hello_world.spec.ts` around lines 10 - 15, Replace the legacy text= locator assertions with Playwright's getByText for consistency: instead of page.locator('text=0 likes') and page.locator('text=1 like') use page.getByText('0 likes') and page.getByText('1 like') respectively, keeping the existing await expect(...).toBeVisible() checks; the button selection via page.getByRole('button', { name: '👍 Like' }) and the likeButton.click() remain unchanged.demos/basic-v16-rsc/bin/switch-bundler (1)
52-58: Minor: Regex replacement drops optional quotes around values.The pattern captures optional quotes with
["']?but the replacement doesn't preserve them. For example,assets_bundler: "webpack"becomesassets_bundler: rspack. While YAML parses both identically, this may cause minor style drift if the original file used quoted values.This is cosmetic and unlikely to cause issues—just noting for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/basic-v16-rsc/bin/switch-bundler` around lines 52 - 58, The current regex in content.gsub captures optional quotes but the replacement uses `@target_bundler` without re-inserting them, causing quoted values like assets_bundler: "webpack" to become unquoted; update the regex in the content.gsub call (and its replacement string) to capture the optional quote as a separate group (e.g., (["'])?) and then include that backreference around `@target_bundler` in the replacement so the original quoting is preserved; adjust the capture group indexes in the replacement accordingly while keeping the existing anchors (old_bundler, `@target_bundler`) and comment-trailing group intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@demos/basic-v16-rsc/.github/workflows/ci.yml`:
- Around line 82-86: The Build JavaScript bundles step sets RAILS_ENV twice;
remove the redundant inline prefix in the run command so the step uses the env:
block value. Edit the step with name "Build JavaScript bundles" to keep env:
RAILS_ENV: test and DATABASE_URL as-is and change the run from "RAILS_ENV=test
bin/shakapacker" to just "bin/shakapacker" (refer to the step name and the run
command to locate the change).
In `@demos/basic-v16-rsc/bin/switch-bundler`:
- Around line 52-58: The current regex in content.gsub captures optional quotes
but the replacement uses `@target_bundler` without re-inserting them, causing
quoted values like assets_bundler: "webpack" to become unquoted; update the
regex in the content.gsub call (and its replacement string) to capture the
optional quote as a separate group (e.g., (["'])?) and then include that
backreference around `@target_bundler` in the replacement so the original quoting
is preserved; adjust the capture group indexes in the replacement accordingly
while keeping the existing anchors (old_bundler, `@target_bundler`) and
comment-trailing group intact.
In `@demos/basic-v16-rsc/e2e/hello_world.spec.ts`:
- Around line 10-15: Replace the legacy text= locator assertions with
Playwright's getByText for consistency: instead of page.locator('text=0 likes')
and page.locator('text=1 like') use page.getByText('0 likes') and
page.getByText('1 like') respectively, keeping the existing await
expect(...).toBeVisible() checks; the button selection via
page.getByRole('button', { name: '👍 Like' }) and the likeButton.click() remain
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
demos/basic-v16-rsc/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
demos/basic-v16-rsc/.eslintrc.jsdemos/basic-v16-rsc/.github/workflows/ci.ymldemos/basic-v16-rsc/README.mddemos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsxdemos/basic-v16-rsc/app/views/layouts/application.html.erbdemos/basic-v16-rsc/bin/shakapacker-precompile-hookdemos/basic-v16-rsc/bin/switch-bundlerdemos/basic-v16-rsc/client/node-renderer.jsdemos/basic-v16-rsc/config/initializers/cypress_on_rails.rbdemos/basic-v16-rsc/config/initializers/react_on_rails.rbdemos/basic-v16-rsc/config/initializers/react_on_rails_pro.rbdemos/basic-v16-rsc/config/puma.rbdemos/basic-v16-rsc/config/routes.rbdemos/basic-v16-rsc/config/webpack/serverWebpackConfig.jsdemos/basic-v16-rsc/e2e/app_commands/log_fail.rbdemos/basic-v16-rsc/e2e/hello_world.spec.tsdemos/basic-v16-rsc/e2e/playwright.config.jsdemos/basic-v16-rsc/e2e/playwright/e2e/rails_examples/using_scenarios.spec.jsdemos/basic-v16-rsc/e2e/playwright/support/index.jsdemos/basic-v16-rsc/e2e/playwright/support/on-rails.jsdemos/basic-v16-rsc/lefthook.ymldemos/basic-v16-rsc/lib/tasks/e2e.rakedemos/basic-v16-rsc/package.jsondemos/basic-v16-rsc/public/406-unsupported-browser.htmlpackages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/templates/e2e.rake
🚧 Files skipped from review as they are similar to previous changes (16)
- demos/basic-v16-rsc/.eslintrc.js
- demos/basic-v16-rsc/public/406-unsupported-browser.html
- demos/basic-v16-rsc/app/javascript/src/HelloServer/components/HelloServer.tsx
- demos/basic-v16-rsc/README.md
- demos/basic-v16-rsc/package.json
- demos/basic-v16-rsc/e2e/playwright/e2e/rails_examples/using_scenarios.spec.js
- demos/basic-v16-rsc/bin/shakapacker-precompile-hook
- demos/basic-v16-rsc/e2e/playwright/support/index.js
- demos/basic-v16-rsc/client/node-renderer.js
- demos/basic-v16-rsc/config/webpack/serverWebpackConfig.js
- demos/basic-v16-rsc/config/puma.rb
- demos/basic-v16-rsc/config/initializers/cypress_on_rails.rb
- demos/basic-v16-rsc/app/views/layouts/application.html.erb
- demos/basic-v16-rsc/config/routes.rb
- demos/basic-v16-rsc/lefthook.yml
- demos/basic-v16-rsc/e2e/app_commands/log_fail.rb
PR Review: Add RSC demo with react_on_rails 16.4.0-rc.5This is a well-structured addition that demonstrates React Server Components with streaming via React on Rails Pro. The architecture is sound — client/server/RSC triple webpack build, Node renderer, auto_load_bundle with A few issues worth addressing: Bugs / Code Quality1. Unused import in Line 1 imports 2. Misleading comment in The comment says "Add the RSC loader before the JS loader" but the implementation uses 3. Duplicate Playwright config files There are two Playwright config files with divergent scope:
Running Security / Reliability4. With this set to config.throw_js_errors = !Rails.env.production?Notes on Known IssuesThe PR description correctly documents the two known workarounds (generator bug #2436, first-run precompile hook issue #2438). Given that |
| @@ -0,0 +1,7 @@ | |||
| import ReactOnRails from 'react-on-rails-pro'; | |||
There was a problem hiding this comment.
Unused import — ReactOnRails is imported here but never referenced in this file. This will pull the module into scope unnecessarily and is likely a copy-paste artifact from another bundle entry point. Remove this line.
| import ReactOnRails from 'react-on-rails-pro'; | |
| import registerServerComponent from 'react-on-rails-pro/registerServerComponent/server'; |
| // Find the JS loader (could be babel-loader or swc-loader depending on configuration) | ||
| const jsLoader = extractLoader(rule, 'babel-loader') || extractLoader(rule, 'swc-loader'); | ||
| if (jsLoader) { | ||
| rule.use.push({ |
There was a problem hiding this comment.
Misleading comment: push() appends to the end of the rule.use array. Since webpack processes loaders right-to-left (last in array → runs first), this actually makes the RSC loader execute before the JS transpiler, which is the correct behaviour. But the comment says "before the JS loader", which is the opposite of what push does structurally. Consider rewording to avoid confusion:
| rule.use.push({ | |
| // Append the RSC loader so webpack (which processes loaders right-to-left) | |
| // runs it before the JS transpiler (babel-loader / swc-loader). | |
| // This loader properly excludes client components from the RSC bundle. |
| config.renderer_use_fallback_exec_js = Rails.env.development? | ||
|
|
||
| # If you want Honeybadger or Sentry on the Node renderer side to report rendering errors | ||
| config.throw_js_errors = false |
There was a problem hiding this comment.
With throw_js_errors = false, rendering errors from the Node renderer are silently swallowed in all environments including development. For a demo app this makes debugging RSC failures very difficult — a broken component will appear as empty/missing output with no error reported.
Consider enabling it for non-production environments:
| config.throw_js_errors = false | |
| config.throw_js_errors = !Rails.env.production? |
If you want errors reported via Honeybadger/Sentry in production, keep it false there only, and let the error-reporting service catch them.
| ], | ||
|
|
||
| webServer: process.env.SKIP_WEB_SERVER ? undefined : { | ||
| command: 'bin/dev', |
There was a problem hiding this comment.
The webServer.command is bin/dev, but the PR notes that pack generation fails on first run (see issue #2438) unless ruby bin/shakapacker-precompile-hook has been run beforehand. This means the webServer block will non-deterministically fail in a clean environment.
Consider either:
- Updating the command to pre-run the hook:
command: 'ruby bin/shakapacker-precompile-hook && bin/dev', or - Adding a note in the README / test plan instructing users to run the hook first before executing
npx playwright test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 109 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
demos/basic-v16-rsc/config/webpack/rscWebpackConfig.js:1
- The comment says the RSC loader should be added “before” the JS loader, but the implementation appends it via
push, which places it after existing loaders. Loader order matters in webpack; insert the RSC loader at the correct position (e.g., splice it in ahead of the matched babel/swc loader) so client components are properly excluded from the RSC bundle.
demos/basic-v16-rsc/e2e/app_commands/scenarios/basic.rb:1 - This scenario unconditionally references
Post, but this demo doesn’t show aPostmodel/migration being added in the PR. Sinceusing_scenarios.spec.jscallsappScenario('basic'), this will raiseNameErrorand fail e2e runs. Either remove/replace this line with model(s) that exist in the demo, or add the correspondingPostmodel + migration as part of the demo setup.
packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/templates/e2e.rake:1 - This template hardcodes a monorepo-relative path (
../../../../packages/...). When this generator is used outside this monorepo (or even from a different directory structure), the generated rake task will fail to load. Prefer requiring via the gem load path (e.g.,require "shakacode_demo_common/e2e_test_runner") so it works regardless of checkout layout.
demos/basic-v16-rsc/lib/tasks/e2e.rake:1 - This requires demo-common using a monorepo-specific relative path, which will break if the demo is copied out as a standalone app (which the included
README.md/CI template suggests is a supported mode). Use a normalrequire "shakacode_demo_common/e2e_test_runner"(Bundler/Gem load path) so the task works in both monorepo and standalone contexts.
demos/basic-v16-rsc/e2e/app_commands/eval.rb:1 - Executing
Kernel.evaloncommand_optionsenables arbitrary Ruby execution. Even if intended for e2e tooling, this should be explicitly constrained to test-only usage (and/or behind a strict allowlist) to prevent accidental exposure if the middleware or route is enabled in other environments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
demos/basic-v16-rsc/demo showcasing React Server Components16.4.0.rc.5+ react_on_rails_pro16.4.0.rc.5+ shakapacker9.6.0.beta.016.4.0-rc.5+ react-on-rails-pro-node-renderer16.4.0-rc.5Notes
bin/new-demo basic-v16-rsc --rsc --typescriptplus manualbundle add react_on_rails_pro(see RSC generator doesn't add react_on_rails_pro gem to Gemfile react_on_rails#2436 for the generator bug)bin/devpack generation fails on first run; workaround is to runruby bin/shakapacker-precompile-hookfirst (see bin/dev pack generation fails on first startup with RSC demo react_on_rails#2438)Test plan
cd demos/basic-v16-rsc && ruby bin/shakapacker-precompile-hook && bin/devNote
Medium Risk
Adds a brand-new demo app including custom webpack/RSC bundling, a Node SSR renderer, and CI/e2e tooling; risk is mainly around build/dev workflow correctness rather than production code.
Overview
Adds a new
demos/basic-v16-rscRails demo showcasing React Server Components with streaming SSR viareact_on_rails_pro, including aHelloServerRSC (async server component) paired with aLikeButtonclient component and a/hello_serverroute.Wires up RSC/SSR build and runtime plumbing: Shakapacker config + webpack configs to produce client, server, and
rsc-bundleoutputs, a Node renderer entrypoint (client/node-renderer.js), generated pack registration, and Procfiles/bin scripts for multi-process dev.Includes developer/CI hygiene for the demo (GitHub Actions lint/test workflow, lefthook/commitlint, prettier/eslint/rubocop configs) plus Playwright/Cypress-on-Rails e2e scaffolding and rake tasks to run tests across dev modes.
Written by Cursor Bugbot for commit 6bf28d6. Configure here.
Summary by CodeRabbit