Add React Server Components with React on Rails Pro#722
Add React Server Components with React on Rails Pro#722
Conversation
Upgrade from react_on_rails to react_on_rails_pro gem (16.5.1) and
corresponding npm packages. Add RSC infrastructure with a demo page
at /server-components that showcases:
- Server components using Node.js os module and lodash (never shipped
to client)
- Async data fetching with Suspense streaming (comments from Rails API)
- Interactive client components ('use client' TogglePanel) mixed with
server-rendered content (donut pattern)
- Markdown rendering with marked + sanitize-html on server only
Key changes:
- Three-bundle build: client, server, and RSC bundles via Rspack
- Custom RspackRscPlugin for manifest generation (the standard
RSCWebpackPlugin uses webpack internals incompatible with Rspack)
- 'use client' directives on all existing client component entry points
- Alias react-on-rails to react-on-rails-pro in webpack resolve to
handle third-party packages (rescript-react-on-rails)
- Dedicated rsc-bundle.js entry with registerServerComponent
- RSC payload route and client-side registration
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
WalkthroughAdds React Server Components (RSC) support: swaps to Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Rails
participant RSCServer as RSC Renderer (Node)
participant ClientBundle as Client Loader
Browser->>Rails: GET /server-components
activate Rails
Rails->>Rails: pages#server_components
Rails->>RSCServer: Render ServerComponentsPage (RSC)
deactivate Rails
activate RSCServer
RSCServer->>RSCServer: Render ServerInfo (sync)
RSCServer->>RSCServer: Await CommentsFeed (fetch, parse)
RSCServer->>RSCServer: Mark TogglePanel as client component
RSCServer->>Browser: Stream RSC payload (serialized)
deactivate RSCServer
Browser->>ClientBundle: Fetch client manifest & chunks
Browser->>ClientBundle: Load TogglePanel chunk
ClientBundle->>Browser: Hydrate TogglePanel (interactive)
Browser->>Browser: Display fully interactive page
sequenceDiagram
participant Build as Bundler
participant RscConfig as rscWebpackConfig
participant Common as commonWebpackConfig
participant Plugin as RspackRscPlugin
participant Output as Build Output
Build->>RscConfig: Load RSC config (RSC_BUNDLE_ONLY)
RscConfig->>Common: Derive base config & adjust rules
RscConfig->>RscConfig: Remove CSS emitters, set react-server condition
RscConfig->>RscConfig: Inject react-on-rails-rsc loader
Build->>Plugin: Run RspackRscPlugin during compilation
Plugin->>Plugin: Scan modules for 'use client' directive
Plugin->>Output: Emit react-client-manifest.json (and SSR manifest)
Output->>Build: RSC bundle and manifests produced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 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 |
PR Review: Add React Server Components with React on Rails ProThis is a well-structured PR that introduces RSC support cleanly. The three-bundle pipeline design and the custom Critical
Bugs
Debugging artifacts left in production code
Minor / Cleanup
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
Critical: hardcoded localhost URL will break in all non-local environments.
This URL is baked into the server component bundle and will fail in staging, Docker, CI, and production. The server-side fetch should use a relative path (if the RSC renderer runs inside the same Rails process) or read the host from an environment variable:
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const baseUrl = process.env.RAILS_INTERNAL_URL || 'http://localhost:3000'; | |
| const response = await fetch(`${baseUrl}/comments.json`); |
Or, if the RSC renderer is co-located with Rails, consider calling the model/service directly instead of going through HTTP.
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); |
There was a problem hiding this comment.
Missing error handling — if the Rails server is unreachable or returns a non-OK status, this will throw an unhandled error that crashes the RSC render stream. Wrap in try/catch and return a meaningful error UI:
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const comments = await response.json(); | |
| let comments = []; | |
| try { | |
| const baseUrl = process.env.RAILS_INTERNAL_URL || 'http://localhost:3000'; | |
| const response = await fetch(`${baseUrl}/comments.json`); | |
| if (!response.ok) throw new Error(`HTTP ${response.status}`); | |
| comments = await response.json(); | |
| } catch (err) { | |
| return ( | |
| <div className="bg-red-50 border border-red-200 rounded-lg p-6 text-center"> | |
| <p className="text-red-700">Could not load comments: {err.message}</p> | |
| </div> | |
| ); | |
| } |
|
|
||
| async function CommentsFeed() { | ||
| // Simulate network latency to demonstrate streaming | ||
| await new Promise((resolve) => setTimeout(resolve, 800)); |
There was a problem hiding this comment.
Debugging artifact — artificial delay should be removed before merge.
This 800 ms setTimeout was added to simulate streaming but it runs in every environment, including production. It adds latency for all users.
| await new Promise((resolve) => setTimeout(resolve, 800)); |
(Delete this line entirely.)
| <%= append_javascript_pack_tag('rsc-client-components') %> | ||
| <%= react_component("ServerComponentsPage", | ||
| prerender: false, | ||
| trace: true, |
There was a problem hiding this comment.
trace: true emits internal rendering diagnostics visible in the HTML response. This should be gated to development only:
| trace: true, | |
| trace: Rails.env.development?, |
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Stale cache bug in watch mode. This module-level Map is allocated once when the plugin module is first require()d and is never cleared between incremental builds. If a developer adds or removes a 'use client' directive from a file while the watcher is running, the cached result will shadow the change until the process is restarted.
Clear the cache on each compilation start so watch mode stays correct:
| const useClientCache = new Map(); | |
| const useClientCache = new Map(); |
And inside apply, before iterating chunks:
compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => {
// Clear per-build so watch-mode changes to 'use client' are picked up
useClientCache.clear();
// ... rest of hook
| @@ -1,8 +1,10 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Semantically contradictory directive. This file is the server-side SSR variant of RouterApp (note .server.jsx) and is imported by the server bundle for ExecJS rendering — it should never run on the client. Adding 'use client' here tells the RSC bundler to treat it as a client component boundary, which conflicts with its purpose.
If the intent is only to satisfy the RSC bundler scanning all files, consider whether this file actually needs the directive at all, or whether it should remain server-only and be excluded from RSC manifest scanning.
| const safeHtml = sanitizeHtml(rawHtml, { | ||
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | ||
| }); |
There was a problem hiding this comment.
Allowing <img> without restricting the src attribute means user-supplied comment text can render <img src="http://internal-service">, which probes internal network hosts from the server side during RSC rendering (SSRF). Since comment text is user-controlled, consider either dropping img from the allowlist or adding an allowedAttributes restriction:
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | |
| }); | |
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | |
| allowedAttributes: { | |
| ...sanitizeHtml.defaults.allowedAttributes, | |
| img: ['alt'], // omit src to prevent SSRF from server-side rendering | |
| }, | |
| }); |
![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: 4d09e13058
ℹ️ 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".
| // RSC: Generate react-server-client-manifest.json for SSR component resolution | ||
| serverWebpackConfig.plugins.push(new RspackRscPlugin({ isServer: true })); |
There was a problem hiding this comment.
This adds RspackRscPlugin({ isServer: true }) to the SSR server config even though the RSC config also emits the same react-server-client-manifest.json into ssr-generated, and the default build runs both compilers together (config/webpack/webpackConfig.js). Because the two compilers produce different module sets (the SSR bundle does not include TogglePanel), whichever compiler writes last wins, so RSC requests can intermittently fail to resolve client components after a full build/watch cycle.
Useful? React with 👍 / 👎.
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
The comments feed fetches from http://localhost:3000/comments.json, which only works in a local dev setup on that exact port. In production, CI, or any non-default port/container deployment, this will fail with connection errors and break the streamed comments section. The server component should derive the base URL from runtime request/environment config instead of hardcoding localhost.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces React Server Components (RSC) support via
Confidence Score: 3/5Not safe to merge until the hardcoded localhost URL and missing error handling in CommentsFeed are fixed Two P1 defects exist in CommentsFeed.jsx: a hardcoded localhost URL that breaks all non-local deployments and a missing response.ok guard that causes silent crashes on API errors. These are on the primary path of the new RSC demo feature. All other changes (webpack configs, plugin, Rails routes, component structure) are well-structured and low risk. client/app/bundles/server-components/components/CommentsFeed.jsx requires fixes to the fetch call before merge Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant Rails
participant RSCRenderer as RSC Node Renderer
participant API as Rails API
Browser->>Rails: GET /server-components
Rails-->>Browser: HTML shell + JS bundles (client + rsc-client-components)
Browser->>Rails: GET /rsc_payload/ServerComponentsPage
Rails->>RSCRenderer: Render ServerComponentsPage (rsc-bundle.js)
RSCRenderer->>API: fetch /comments.json (server-side, hardcoded localhost:3000)
API-->>RSCRenderer: JSON comments array
RSCRenderer-->>Rails: React Flight payload (streamed chunks)
Rails-->>Browser: Streamed RSC payload
Browser->>Browser: Hydrate TogglePanel ('use client')
Note over Browser: ServerInfo and CommentsFeed<br/>ship zero client JS
Reviews (1): Last reviewed commit: "Add React Server Components with React o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
fetch('http://localhost:3000/comments.json') will fail in any environment other than a local dev machine on port 3000 — staging, CI, Docker, and production will all break. The base URL should come from an environment variable or Rails configuration.
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const response = await fetch(`${process.env.RAILS_BASE_URL || 'http://localhost:3000'}/comments.json`); |
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); |
There was a problem hiding this comment.
If the Rails API returns a non-200 status (e.g. 500, 503), response.json() is still called and may return an error body object rather than an array, causing _.orderBy to receive invalid input and the component to throw inside the Suspense boundary with no user-visible message.
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const comments = await response.json(); | |
| const response = await fetch('http://localhost:3000/comments.json'); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`); | |
| } | |
| const comments = await response.json(); |
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
useClientCache is a module-level Map that lives for the full lifetime of the Rspack process. In watch/HMR mode, if a developer adds or removes a 'use client' directive, the stale cached value will be used for all subsequent incremental builds and the manifest won't update until the watcher is restarted. Consider hooking into compilation.fileDependencies to invalidate affected entries, or at minimum document this limitation.
| const safeHtml = sanitizeHtml(rawHtml, { | ||
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | ||
| }); |
There was a problem hiding this comment.
img allowed in sanitized HTML without restricting src
Adding img to allowedTags without also restricting the src attribute lets any commenter embed arbitrary external image URLs (tracking pixels, SSRF probes, mixed-content issues). Provide allowedAttributes to constrain which attributes img may carry.
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | |
| }); | |
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | |
| allowedAttributes: { | |
| ...sanitizeHtml.defaults.allowedAttributes, | |
| img: ['src', 'alt', 'title', 'width', 'height'], | |
| }, | |
| allowedSchemes: ['https', 'http'], | |
| }); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
Hardcoded localhost URL breaks non-local deployments
High Severity
The CommentsFeed server component fetches from a hardcoded http://localhost:3000/comments.json URL. This project is live at www.reactrails.com per the README, so this fetch will fail in any deployed environment where the server isn't reachable at localhost:3000. The RSC server component would get a connection error or wrong response, causing the comments section to break entirely.
Reviewed by Cursor Bugbot for commit 4d09e13. Configure here.
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Module-level cache causes stale results in watch mode
Medium Severity
The useClientCache Map is module-scoped and never cleared between compilations. In watch mode (used by wp-client, wp-server, and wp-rsc in Procfile.dev), adding or removing a 'use client' directive from a file won't be reflected in the RSC manifest because the stale cached value persists. This would cause confusing development issues where client/server component boundaries don't update until the watcher is restarted.
Reviewed by Cursor Bugbot for commit 4d09e13. Configure here.
- Move eslint-disable after 'use client' directive in SimpleCommentScreen - Add no-promise-executor-return disable for setTimeout in CommentsFeed - Replace array index key with semantic key in ServerInfo - Add PropTypes to TogglePanel component - Fix import ordering in stimulus-bundle.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
Bug: hardcoded localhost URL will break in any non-local environment.
The URL is baked in as http://localhost:3000, so this component will fail on staging, production, CI, or any Docker/container setup where Rails isn't running on that exact host/port. The RSC bundle runs in the Node renderer process on the same server as Rails, so an internal/relative call is appropriate here.
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const baseUrl = process.env.RAILS_INTERNAL_URL || 'http://localhost:3000'; | |
| const response = await fetch(`${baseUrl}/comments.json`); |
Or, if react_on_rails_pro exposes a Rails API helper, use that directly to avoid the network hop altogether.
| marked.use(gfmHeadingId()); | ||
|
|
||
| async function CommentsFeed() { | ||
| // Simulate network latency to demonstrate streaming |
There was a problem hiding this comment.
Demo-only delay left in the component — should not ship as-is.
An 800 ms forced delay every time this component renders is fine for a live demo but would noticeably degrade real-world page load. Consider removing it before merging, or guarding it behind an env flag:
| // Simulate network latency to demonstrate streaming | |
| // Simulate network latency to demonstrate streaming (remove in production) | |
| if (process.env.NODE_ENV === 'development' && process.env.RSC_SIMULATE_LATENCY) { | |
| // eslint-disable-next-line no-promise-executor-return | |
| await new Promise((resolve) => setTimeout(resolve, 800)); | |
| } |
| @@ -1,8 +1,10 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
'use client' on a server-only file is semantically wrong and may misclassify this module.
RouterApp.server.jsx is the SSR rendering entry; it runs in the Node renderer (ExecJS / server bundle), not in the React Server Components runtime. Adding 'use client' causes the RSC WebpackLoader and RspackRscPlugin to treat it as a client boundary, which is the opposite of its intent.
This also contradicts the .server.jsx naming convention. If the goal was just to satisfy a linter that flags missing directives on RSC-adjacent files, this file should be left without any directive — or annotated with a comment explaining it is a legacy SSR file unrelated to RSC.
| @@ -1,5 +1,7 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
'use client' on the server bundle registration file is a semantic mismatch.
serverRegistration.jsx is the entry point for the server bundle — its whole job is to register components for server-side rendering. Marking it 'use client' tells the RSC WebpackLoader that every module reachable from this file is a client boundary, which is incorrect and could cause the RSC manifest to over-include modules that should stay server-only.
Remove this directive; it should not be present here.
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Module-level cache is never invalidated — stale results during --watch mode.
useClientCache persists across all compilations within the same process lifetime. If a developer adds or removes a 'use client' directive from a file during a watch session, the cached result will never be refreshed and the manifest will silently reflect the old state until the process is restarted.
Hook into the compiler to clear the cache entry when a file changes:
compiler.hooks.watchRun.tap('RspackRscPlugin', (compiler) => {
const changedFiles = compiler.modifiedFiles || new Set();
for (const filePath of changedFiles) {
useClientCache.delete(filePath);
}
});Place this inside apply() alongside the thisCompilation hook.
| const { getBundler } = require('./bundlerUtils'); | ||
| const { RspackRscPlugin } = require('./rspackRscPlugin'); | ||
|
|
||
| function extractLoader(rule, loaderName) { |
There was a problem hiding this comment.
extractLoader is duplicated — it already exists (and is exported) in serverWebpackConfig.js.
serverWebpackConfig.js exports this exact same function:
module.exports = { default: configureServer, extractLoader };Import it instead of redefining it:
| function extractLoader(rule, loaderName) { | |
| const { extractLoader } = require('./serverWebpackConfig'); |
Then remove lines 16–22 here.
- Use single-line comment eslint-disable before 'use client' directive (file-level rules must be disabled before line 1) - Suppress react/no-danger for sanitized HTML in CommentsFeed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| // Emit SSR manifest (maps module IDs to SSR module data) | ||
| if (!this.isServer) { | ||
| compilation.emitAsset( |
There was a problem hiding this comment.
SSR manifest is always emitted as an empty object — is this intentional?
The react-ssr-manifest.json is supposed to map module IDs to their SSR bundle equivalents so the React flight client can resolve client components during hydration. Emitting {} is a valid placeholder if react-on-rails-pro doesn't read this file at runtime, but if it does rely on it the empty manifest will silently break client component hydration without a clear error.
Please add a comment clarifying whether this is a known limitation/TODO, or whether react-on-rails-pro derives this information from the client manifest alone.
| // These libraries (combined ~200KB) never reach the client. | ||
| const rawHtml = marked.parse(comment.text || ''); | ||
| const safeHtml = sanitizeHtml(rawHtml, { | ||
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), |
There was a problem hiding this comment.
img added to allowedTags without restricting attributes — allows external image loads.
sanitize-html strips event handler attributes (onerror, etc.) by default, so this isn't a direct XSS vector. However, allowing unrestricted img tags lets comment authors embed arbitrary external images, which can be used for IP/user-agent tracking pixels, or to trigger unintended outbound requests from the server when the RSC renderer fetches the HTML.
Consider restricting img to a allowlist of safe origins, or at minimum also setting allowedAttributes to block src on img so only data URIs or relative paths are permitted:
const safeHtml = sanitizeHtml(rawHtml, {
allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']),
allowedAttributes: {
...sanitizeHtml.defaults.allowedAttributes,
img: ['alt', 'title'], // omit src to block external images
},
});
Code ReviewGreat direction — RSC support on top of React on Rails Pro is a meaningful addition and the three-bundle architecture is well-structured. A few issues need attention before merging: Bugs / Must-Fix1. Hardcoded 2. 3. Code Quality4. 5. Artificial 800 ms delay in Security / Minor6. Unrestricted 7. SSR manifest always emitted as Observations (no action needed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
config/webpack/commonWebpackConfig.js (1)
11-17: Consider using a broader alias pattern for complete coverage.The current aliases handle exact imports and the
/node_packagesubpath, but third-party packages might import other subpaths (e.g.,react-on-rails/someOtherPath). A regex-based alias could provide more comprehensive coverage.♻️ Alternative: broader alias pattern
If you encounter issues with other subpath imports, consider:
alias: { 'react-on-rails$': 'react-on-rails-pro', - 'react-on-rails/node_package': 'react-on-rails-pro/node_package', + 'react-on-rails/': 'react-on-rails-pro/', },Note: The trailing slash pattern behavior varies between webpack/rspack versions. Test this if you encounter additional subpath resolution issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/commonWebpackConfig.js` around lines 11 - 17, The current webpack alias object only maps exact imports ('react-on-rails$' and 'react-on-rails/node_package') and misses other subpath imports; update the alias entries in the alias object to use a regex/broader pattern so any import of react-on-rails and its subpaths (e.g., react-on-rails/... ) is redirected to react-on-rails-pro and its corresponding subpath, replacing or supplementing the existing keys in the alias object to ensure complete coverage across imports; test subpath resolution behavior in your webpack/rspack version after changing the alias.config/webpack/rspackRscPlugin.js (1)
20-24: Synchronous file I/O may impact build performance at scale.Using
fs.openSync/readSync/closeSyncblocks the event loop. While caching mitigates repeated reads, the initial scan of many files could slow builds. This is acceptable for most projects but worth noting for large codebases.For future optimization, consider using
fs.promiseswithPromise.allif build times become a concern, though this would require restructuring theprocessAssetshook to be async.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 20 - 24, The current synchronous file reads (fs.openSync, fs.readSync, fs.closeSync) in the RSC scanning logic block that reads the first ~200 bytes of filePath block the event loop and can degrade large builds; change this to use fs.promises (e.g., fs.promises.open andfilehandle.read or fs.promises.readFile with a slice) and perform reads concurrently with Promise.all so many files are scanned in parallel, and update the surrounding processAssets hook (or the function that calls this scan) to be async/await-compatible so it can await the promise-based reads without blocking; keep the existing cache semantics and only switch the low-level I/O to the promise-based APIs while preserving the logic that checks for 'use client' at the top of filePath.package.json (1)
132-132: Upgradeeslint-plugin-react-hooksto the latest version for React 19 compatibility.The current version
^4.6.0does not fully support React 19's new hooks likeuseActionState,useFormStatus, anduseOptimistic. The latest stable version (7.0.1) provides complete support for React 19, including validation rules for React Compiler features introduced in React 19.📦 Suggested upgrade
- "eslint-plugin-react-hooks": "^4.6.0", + "eslint-plugin-react-hooks": "^7.0.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 132, Update the eslint-plugin-react-hooks dependency in package.json to the latest compatible release (replace the "eslint-plugin-react-hooks": "^4.6.0" entry) so the project uses the React 19–compatible validator (e.g., 7.0.1); modify the package.json dependency line for eslint-plugin-react-hooks and then run your package manager (npm/yarn/pnpm) to install and update lockfiles, and run the linter to confirm no new rule errors from the updated plugin.app/controllers/pages_controller.rb (1)
41-41: Skipset_commentsfor the newserver_componentsaction.Line 41 introduces an action with no use of
@comments, but it still incursbefore_action :set_comments. Consider excluding this action to avoid unnecessary DB work.♻️ Proposed change
- before_action :set_comments + before_action :set_comments, except: %i[server_components]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/pages_controller.rb` at line 41, The new controller action server_components is being run through the before_action :set_comments even though it does not use `@comments`; update the before_action declaration to skip set_comments for this action by adding an except/only exemption (e.g., modify the before_action :set_comments line to exclude :server_components) so the database work is avoided when server_components is invoked.app/views/pages/server_components.html.erb (1)
2-4: Gate tracing by environment.Line 4 uses
trace: trueunconditionally. Prefer enabling trace only in development to reduce noise and overhead.♻️ Proposed change
<%= react_component("ServerComponentsPage", prerender: false, - trace: true, + trace: Rails.env.development?, id: "ServerComponentsPage-react-component-0") %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/pages/server_components.html.erb` around lines 2 - 4, The react_component call currently hardcodes trace: true; change it to enable tracing only in development by replacing the literal with a conditional using the Rails environment (e.g., set trace to Rails.env.development? or equivalent), updating the react_component("ServerComponentsPage", prerender: false, trace: ...) invocation so trace is true only when Rails.env.development? and false otherwise.client/app/bundles/server-components/components/TogglePanel.jsx (1)
10-29: Expose expanded/collapsed state to assistive tech.Please add
aria-expanded(and ideallyaria-controls) on the toggle button so screen readers get the panel state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/bundles/server-components/components/TogglePanel.jsx` around lines 10 - 29, The TogglePanel component's toggle button needs ARIA attributes so assistive tech can read its state: update the button (where setIsOpen is used and isOpen is referenced) to include aria-expanded={isOpen} and aria-controls pointing to the panel's id, and give the panel div (the conditional container that renders {children}) a matching id; implement the id either by accepting a prop (e.g., panelId) or generating a stable id inside the component (e.g., React's useId) and use that id on the div so aria-controls links to it.config/webpack/rscWebpackConfig.js (2)
89-90: Don't replace the inheritedoptimizationobject.This drops whatever
commonWebpackConfig()already set up. Onlyminimizeneeds to change here, so merge into the existing object instead of resetting it wholesale.Suggested diff
- rscConfig.optimization = { minimize: false }; + rscConfig.optimization = { + ...(rscConfig.optimization || {}), + minimize: false, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rscWebpackConfig.js` around lines 89 - 90, The code currently overwrites the inherited optimization object (rscConfig.optimization = { minimize: false }), dropping settings established by commonWebpackConfig(); instead merge into the existing object by preserving rscConfig.optimization and only changing minimize (e.g., assign rscConfig.optimization = { ...rscConfig.optimization, minimize: false } or set rscConfig.optimization.minimize = false) so other optimization defaults remain intact; update the rscWebpackConfig code that sets rscConfig.optimization accordingly.
29-33: Mirror the server-bundle path diagnostics here.
config/webpack/serverWebpackConfig.js:52-68derives the expected bundle path fromconfig.source_pathandconfig.source_entry_path. Hard-codingclient/app/packs/rsc-bundle.jsmakes this failure misleading for any non-default Shakapacker layout.Suggested diff
const rscEntry = rscConfig.entry['rsc-bundle']; if (!rscEntry) { + const sourcePath = config.source_path || 'client/app'; + const entryPath = config.source_entry_path || 'packs'; + const fullPath = `${sourcePath}/${entryPath}/rsc-bundle.js`; throw new Error( - 'RSC bundle entry not found. Ensure client/app/packs/rsc-bundle.js exists.', + `RSC bundle entry 'rsc-bundle' not found.\n` + + `Expected file: ${fullPath}\n` + + `Current source_path: ${config.source_path}\n` + + `Current source_entry_path: ${config.source_entry_path}\n` + + `Verify:\n` + + `1. The rsc-bundle.js file exists at the expected location\n` + + `2. nested_entries is configured correctly in shakapacker.yml\n` + + `3. The file is properly exported from your entry point`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rscWebpackConfig.js` around lines 29 - 33, The error thrown when rscEntry is missing should mirror the server-bundle diagnostics by deriving the expected RSC bundle path from the same config values instead of hard-coding "client/app/packs/rsc-bundle.js"; update the check around rscEntry (rscEntry and rscConfig.entry) to compute the expected path using config.source_path and config.source_entry_path (same approach used in serverWebpackConfig's bundle path logic) and include that computed path in the thrown Error message so the failure message accurately reflects non-default Shakapacker layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx`:
- Line 3: ESLint is still failing because the single-line disable at the top
doesn't cover both class declarations in SimpleCommentScreen.jsx; either add a
file-level directive (/* eslint-disable max-classes-per-file */) at the very top
of SimpleCommentScreen.jsx so it applies to all class declarations in the file,
or move one of the class declarations into its own module and import it (i.e.,
split the two class declarations into separate files) and remove the inline
disable; locate the two class declarations in SimpleCommentScreen.jsx and choose
one of these fixes to resolve the rule failure.
In
`@client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx`:
- Line 1: Remove the top-level 'use client' directive from RouterApp.server.jsx
so the module is treated as a server component (it currently imports server-only
APIs like react-router-dom/server); open the file and delete the "'use client'"
line and ensure no client-only hooks or APIs are used elsewhere in the module
(check for references inside RouterApp or any exported server functions) so the
import of react-router-dom/server remains valid.
In `@client/app/bundles/server-components/components/CommentsFeed.jsx`:
- Around line 19-21: In CommentsFeed.jsx replace the hardcoded
'http://localhost:3000/comments.json' fetch with an environment-aware or
relative URL (e.g., use a relative path '/comments.json' or a runtime config
variable like process.env.NEXT_PUBLIC_API_URL) and add response status handling
before parsing: check response.ok in the function that calls fetch and throw or
handle an error when it's false so you don't call response.json() on error
responses; adjust any surrounding error-handling logic in the CommentsFeed
component to surface or recover from that thrown error.
- Line 16: The Promise executor in CommentsFeed.jsx uses an implicit-return
arrow that returns setTimeout (await new Promise((resolve) =>
setTimeout(resolve, 800))), triggering the no-promise-executor-return lint rule;
change the executor to a block-bodied function and call setTimeout inside it
(i.e., replace the inline implicit-return with a brace-enclosed function that
invokes setTimeout and calls resolve), so the Promise executor does not return
the timer value.
In `@client/app/bundles/server-components/components/ServerInfo.jsx`:
- Around line 43-45: Replace the unstable array-index key used in the
grouped.map call (key={gi}) with a stable identifier derived from the group's
contents: inside the ServerInfo component replace the gi key with a
deterministic value (for example build a key from the group's item keys like
group.map(([k])=>k).join('-') or use the first item's key group[0][0] if
present). Update the grouped.map(...) element (where gi is currently used) to
use this computed stableKey so React reconciliation is stable and lint warnings
are resolved.
In `@client/app/bundles/server-components/components/TogglePanel.jsx`:
- Around line 5-34: Import PropTypes from 'prop-types' and add propTypes on the
TogglePanel component to satisfy the react/prop-types lint rule: add
"TogglePanel.propTypes = { title: PropTypes.node.isRequired, children:
PropTypes.node }" (and optionally defaultProps if desired). This change should
be made near the TogglePanel definition (reference: TogglePanel component, props
title and children) and ensure the PropTypes import is included at the top of
the file.
In `@client/app/packs/rsc-client-components.js`:
- Around line 7-8: The ESLint import/order violation is caused by importing the
relative module TogglePanel before the external package ReactOnRails; fix it by
reordering the imports so ReactOnRails is imported first and TogglePanel second
(i.e., move the ReactOnRails import above the TogglePanel import in
rsc-client-components.js), then re-run the linter to confirm the import/order
rule passes.
In `@config/webpack/rscWebpackConfig.js`:
- Around line 65-66: The guard that checks for url/file loaders in
rscWebpackConfig.js is too strict (it compares rule.use.loader using ===), so
emitFile=false is not applied when loader identifiers are resolved to full
paths; update the condition in the block that sets rule.use.options.emitFile to
use substring matching (e.g., use .includes('url-loader') ||
.includes('file-loader')) or call the existing extractLoader() helper to detect
loader identity, ensuring the check matches resolved loader paths and reliably
sets emitFile=false for the url-loader/file-loader cases.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 12-35: The module-level cache useClientCache can become stale
during watch/dev builds; update the plugin's apply method to clear
useClientCache at the start of each compilation (e.g., register a hook on
compiler.hooks.compilation or compiler.hooks.watchRun depending on the plugin
entry) so hasUseClientDirective always re-reads changed files between rebuilds;
locate useClientCache and hasUseClientDirective and add the cache.clear() call
inside the apply's compilation/watch hook to invalidate cached results on each
new build.
In `@config/webpack/webpackConfig.js`:
- Around line 23-33: The RSC-only branch never applies env-specific adjustments
and doesn't expose the rsc config for modification; update the RSC_BUNDLE_ONLY
branch so after creating rscConfig via rscWebpackConfig() you call envSpecific
appropriately (e.g., envSpecific(null, null, rscConfig) or extend envSpecific to
accept a third rscConfig arg) before assigning result = rscConfig, ensuring
rscConfig is passed to envSpecific exactly like clientConfig and serverConfig
are in the multi-config path (symbols: rscWebpackConfig, envSpecific,
clientWebpackConfig, serverWebpackConfig, result).
---
Nitpick comments:
In `@app/controllers/pages_controller.rb`:
- Line 41: The new controller action server_components is being run through the
before_action :set_comments even though it does not use `@comments`; update the
before_action declaration to skip set_comments for this action by adding an
except/only exemption (e.g., modify the before_action :set_comments line to
exclude :server_components) so the database work is avoided when
server_components is invoked.
In `@app/views/pages/server_components.html.erb`:
- Around line 2-4: The react_component call currently hardcodes trace: true;
change it to enable tracing only in development by replacing the literal with a
conditional using the Rails environment (e.g., set trace to
Rails.env.development? or equivalent), updating the
react_component("ServerComponentsPage", prerender: false, trace: ...) invocation
so trace is true only when Rails.env.development? and false otherwise.
In `@client/app/bundles/server-components/components/TogglePanel.jsx`:
- Around line 10-29: The TogglePanel component's toggle button needs ARIA
attributes so assistive tech can read its state: update the button (where
setIsOpen is used and isOpen is referenced) to include aria-expanded={isOpen}
and aria-controls pointing to the panel's id, and give the panel div (the
conditional container that renders {children}) a matching id; implement the id
either by accepting a prop (e.g., panelId) or generating a stable id inside the
component (e.g., React's useId) and use that id on the div so aria-controls
links to it.
In `@config/webpack/commonWebpackConfig.js`:
- Around line 11-17: The current webpack alias object only maps exact imports
('react-on-rails$' and 'react-on-rails/node_package') and misses other subpath
imports; update the alias entries in the alias object to use a regex/broader
pattern so any import of react-on-rails and its subpaths (e.g.,
react-on-rails/... ) is redirected to react-on-rails-pro and its corresponding
subpath, replacing or supplementing the existing keys in the alias object to
ensure complete coverage across imports; test subpath resolution behavior in
your webpack/rspack version after changing the alias.
In `@config/webpack/rscWebpackConfig.js`:
- Around line 89-90: The code currently overwrites the inherited optimization
object (rscConfig.optimization = { minimize: false }), dropping settings
established by commonWebpackConfig(); instead merge into the existing object by
preserving rscConfig.optimization and only changing minimize (e.g., assign
rscConfig.optimization = { ...rscConfig.optimization, minimize: false } or set
rscConfig.optimization.minimize = false) so other optimization defaults remain
intact; update the rscWebpackConfig code that sets rscConfig.optimization
accordingly.
- Around line 29-33: The error thrown when rscEntry is missing should mirror the
server-bundle diagnostics by deriving the expected RSC bundle path from the same
config values instead of hard-coding "client/app/packs/rsc-bundle.js"; update
the check around rscEntry (rscEntry and rscConfig.entry) to compute the expected
path using config.source_path and config.source_entry_path (same approach used
in serverWebpackConfig's bundle path logic) and include that computed path in
the thrown Error message so the failure message accurately reflects non-default
Shakapacker layouts.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 20-24: The current synchronous file reads (fs.openSync,
fs.readSync, fs.closeSync) in the RSC scanning logic block that reads the first
~200 bytes of filePath block the event loop and can degrade large builds; change
this to use fs.promises (e.g., fs.promises.open andfilehandle.read or
fs.promises.readFile with a slice) and perform reads concurrently with
Promise.all so many files are scanned in parallel, and update the surrounding
processAssets hook (or the function that calls this scan) to be
async/await-compatible so it can await the promise-based reads without blocking;
keep the existing cache semantics and only switch the low-level I/O to the
promise-based APIs while preserving the logic that checks for 'use client' at
the top of filePath.
In `@package.json`:
- Line 132: Update the eslint-plugin-react-hooks dependency in package.json to
the latest compatible release (replace the "eslint-plugin-react-hooks": "^4.6.0"
entry) so the project uses the React 19–compatible validator (e.g., 7.0.1);
modify the package.json dependency line for eslint-plugin-react-hooks and then
run your package manager (npm/yarn/pnpm) to install and update lockfiles, and
run the linter to confirm no new rule errors from the updated plugin.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 362cf633-c476-4648-9bdf-6c7c981ca0ac
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
GemfileProcfile.devapp/controllers/pages_controller.rbapp/views/pages/server_components.html.erbclient/app/bundles/comments/components/Footer/ror_components/Footer.jsxclient/app/bundles/comments/components/NavigationBar/NavigationBar.jsxclient/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsxclient/app/bundles/comments/constants/paths.jsclient/app/bundles/comments/rescript/ReScriptShow/ror_components/RescriptShow.jsxclient/app/bundles/comments/startup/App/ror_components/App.jsxclient/app/bundles/comments/startup/ClientRouterAppExpress.jsxclient/app/bundles/comments/startup/NavigationBarApp/ror_components/NavigationBarApp.jsxclient/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.client.jsxclient/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsxclient/app/bundles/comments/startup/serverRegistration.jsxclient/app/bundles/server-components/ServerComponentsPage.jsxclient/app/bundles/server-components/components/CommentsFeed.jsxclient/app/bundles/server-components/components/ServerInfo.jsxclient/app/bundles/server-components/components/TogglePanel.jsxclient/app/libs/requestsManager.jsclient/app/packs/rsc-bundle.jsclient/app/packs/rsc-client-components.jsclient/app/packs/stimulus-bundle.jsclient/app/packs/stores-registration.jsconfig/initializers/react_on_rails_pro.rbconfig/routes.rbconfig/webpack/clientWebpackConfig.jsconfig/webpack/commonWebpackConfig.jsconfig/webpack/rscWebpackConfig.jsconfig/webpack/rspackRscPlugin.jsconfig/webpack/serverWebpackConfig.jsconfig/webpack/webpackConfig.jspackage.json
...t/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
Outdated
Show resolved
Hide resolved
| @@ -1,8 +1,10 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) .server files that declare 'use client' (expected: no matches):"
rg -n --iglob '*.server.js' --iglob '*.server.jsx' --iglob '*.server.ts' --iglob '*.server.tsx' "^\s*['\"]use client['\"]\s*;?"
echo
echo "2) Server-only imports in .server files (context check):"
rg -n --iglob '*.server.js' --iglob '*.server.jsx' --iglob '*.server.ts' --iglob '*.server.tsx' "react-router-dom/server|node:|\\bos\\b"Repository: shakacode/react-webpack-rails-tutorial
Length of output: 444
Remove 'use client' from this server entry file.
The 'use client' directive on line 1 causes this module to be treated as a client component during RSC processing, but the file imports server-only APIs like react-router-dom/server (line 6), creating an incompatibility.
Proposed fix
-'use client';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'use client'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx`
at line 1, Remove the top-level 'use client' directive from RouterApp.server.jsx
so the module is treated as a server component (it currently imports server-only
APIs like react-router-dom/server); open the file and delete the "'use client'"
line and ensure no client-only hooks or APIs are used elsewhere in the module
(check for references inside RouterApp or any exported server functions) so the
import of react-router-dom/server remains valid.
|
|
||
| async function CommentsFeed() { | ||
| // Simulate network latency to demonstrate streaming | ||
| await new Promise((resolve) => setTimeout(resolve, 800)); |
There was a problem hiding this comment.
Fix lint failure in Promise executor callback.
Line 16 triggers no-promise-executor-return; wrap setTimeout in braces.
Proposed fix
- await new Promise((resolve) => setTimeout(resolve, 800));
+ await new Promise((resolve) => {
+ setTimeout(resolve, 800);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise((resolve) => setTimeout(resolve, 800)); | |
| await new Promise((resolve) => { | |
| setTimeout(resolve, 800); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 16-16: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
🪛 GitHub Check: test (22.x, 3.4.6)
[failure] 16-16:
Return values from promise executor functions cannot be read
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/bundles/server-components/components/CommentsFeed.jsx` at line 16,
The Promise executor in CommentsFeed.jsx uses an implicit-return arrow that
returns setTimeout (await new Promise((resolve) => setTimeout(resolve, 800))),
triggering the no-promise-executor-return lint rule; change the executor to a
block-bodied function and call setTimeout inside it (i.e., replace the inline
implicit-return with a brace-enclosed function that invokes setTimeout and calls
resolve), so the Promise executor does not return the timer value.
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); | ||
|
|
There was a problem hiding this comment.
Avoid hardcoded localhost and handle non-2xx responses.
Line 19 hardcodes a dev-only origin, and Line 20 parses JSON without checking response.ok. This will fail in non-local environments and produce brittle runtime errors.
Proposed fix
- const response = await fetch('http://localhost:3000/comments.json');
- const comments = await response.json();
+ const commentsUrl = process.env.RSC_COMMENTS_URL;
+ if (!commentsUrl) {
+ throw new Error('RSC_COMMENTS_URL is required for server component fetches.');
+ }
+ const response = await fetch(commentsUrl);
+ if (!response.ok) {
+ throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`);
+ }
+ const comments = await response.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const comments = await response.json(); | |
| const commentsUrl = process.env.RSC_COMMENTS_URL; | |
| if (!commentsUrl) { | |
| throw new Error('RSC_COMMENTS_URL is required for server component fetches.'); | |
| } | |
| const response = await fetch(commentsUrl); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`); | |
| } | |
| const comments = await response.json(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/bundles/server-components/components/CommentsFeed.jsx` around
lines 19 - 21, In CommentsFeed.jsx replace the hardcoded
'http://localhost:3000/comments.json' fetch with an environment-aware or
relative URL (e.g., use a relative path '/comments.json' or a runtime config
variable like process.env.NEXT_PUBLIC_API_URL) and add response status handling
before parsing: check response.ok in the function that calls fetch and throw or
handle an error when it's false so you don't call response.json() on error
responses; adjust any surrounding error-handling logic in the CommentsFeed
component to surface or recover from that thrown error.
| import TogglePanel from '../bundles/server-components/components/TogglePanel'; | ||
| import ReactOnRails from 'react-on-rails-pro'; |
There was a problem hiding this comment.
Fix import/order lint violation.
Line 8 package import should come before the relative import to satisfy ESLint ordering.
♻️ Proposed change
-import TogglePanel from '../bundles/server-components/components/TogglePanel';
import ReactOnRails from 'react-on-rails-pro';
+import TogglePanel from '../bundles/server-components/components/TogglePanel';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import TogglePanel from '../bundles/server-components/components/TogglePanel'; | |
| import ReactOnRails from 'react-on-rails-pro'; | |
| import ReactOnRails from 'react-on-rails-pro'; | |
| import TogglePanel from '../bundles/server-components/components/TogglePanel'; |
🧰 Tools
🪛 ESLint
[error] 8-8: react-on-rails-pro import should occur before import of ../bundles/server-components/components/TogglePanel
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/packs/rsc-client-components.js` around lines 7 - 8, The ESLint
import/order violation is caused by importing the relative module TogglePanel
before the external package ReactOnRails; fix it by reordering the imports so
ReactOnRails is imported first and TogglePanel second (i.e., move the
ReactOnRails import above the TogglePanel import in rsc-client-components.js),
then re-run the linter to confirm the import/order rule passes.
| } else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) { | ||
| rule.use.options.emitFile = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the rscWebpackConfig.js file around lines 65-66
cat -n config/webpack/rscWebpackConfig.js | sed -n '55,75p'Repository: shakacode/react-webpack-rails-tutorial
Length of output: 1136
🏁 Script executed:
# Find and examine the extractLoader function
fd -t f rscWebpackConfig.js -x cat -n {} | head -50Repository: shakacode/react-webpack-rails-tutorial
Length of output: 2381
🏁 Script executed:
# Check the commonWebpackConfig.js file around lines 64-67
cat -n config/webpack/commonWebpackConfig.js | sed -n '60,70p'Repository: shakacode/react-webpack-rails-tutorial
Length of output: 488
🏁 Script executed:
# Search for extractLoader function definition to understand loader detection
rg -n "extractLoader" config/webpack/ -A 10 -B 2Repository: shakacode/react-webpack-rails-tutorial
Length of output: 3038
🏁 Script executed:
# Search for how loaders are being matched/detected across webpack configs
rg -n "\.includes\(" config/webpack/ | grep -i loaderRepository: shakacode/react-webpack-rails-tutorial
Length of output: 928
The file/url-loader guard is too strict to fire reliably.
extractLoader() here and other loader checks throughout the config use .includes() for loader matching, which indicates loader identifiers are resolved paths, not bare names. Using strict equality === 'url-loader' / === 'file-loader' here will fail to match, leaving emitFile = false unapplied so the RSC build continues emitting assets.
Suggested diff
- } else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
- rule.use.options.emitFile = false;
+ } else if (
+ rule.use?.loader &&
+ (rule.use.loader.includes('url-loader') || rule.use.loader.includes('file-loader'))
+ ) {
+ rule.use.options = { ...(rule.use.options || {}), emitFile: false };
}🧰 Tools
🪛 ESLint
[error] 66-66: Assignment to property of function parameter 'rule'.
(no-param-reassign)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/rscWebpackConfig.js` around lines 65 - 66, The guard that
checks for url/file loaders in rscWebpackConfig.js is too strict (it compares
rule.use.loader using ===), so emitFile=false is not applied when loader
identifiers are resolved to full paths; update the condition in the block that
sets rule.use.options.emitFile to use substring matching (e.g., use
.includes('url-loader') || .includes('file-loader')) or call the existing
extractLoader() helper to detect loader identity, ensuring the check matches
resolved loader paths and reliably sets emitFile=false for the
url-loader/file-loader cases.
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); | ||
|
|
||
| function hasUseClientDirective(filePath) { | ||
| if (useClientCache.has(filePath)) return useClientCache.get(filePath); | ||
|
|
||
| let result = false; | ||
| try { | ||
| // Read the first ~200 bytes — 'use client' must be at the very top of the file | ||
| const fd = fs.openSync(filePath, 'r'); | ||
| const buf = Buffer.alloc(200); | ||
| fs.readSync(fd, buf, 0, 200, 0); | ||
| fs.closeSync(fd); | ||
|
|
||
| const head = buf.toString('utf-8'); | ||
| // Check for 'use client' as the first statement (with or without semicolons/quotes) | ||
| result = /^(?:\s*\/\/[^\n]*\n)*\s*['"]use client['"]/.test(head); | ||
| } catch (_) { | ||
| // file doesn't exist or can't be read | ||
| } | ||
|
|
||
| useClientCache.set(filePath, result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Cache may return stale results during watch/dev mode.
The useClientCache is a module-level Map that persists across rebuilds. If a developer adds or removes a 'use client' directive during development, the cached result won't reflect the change until the process restarts.
🔧 Suggested fix: Clear cache on new compilation
Add cache invalidation in the apply method:
apply(compiler) {
+ // Clear cache at the start of each compilation to handle file changes in watch mode
+ compiler.hooks.thisCompilation.tap('RspackRscPlugin-ClearCache', () => {
+ useClientCache.clear();
+ });
+
compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => {Alternatively, integrate the cache clearing into the existing hook:
compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => {
+ useClientCache.clear();
compilation.hooks.processAssets.tap(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/rspackRscPlugin.js` around lines 12 - 35, The module-level
cache useClientCache can become stale during watch/dev builds; update the
plugin's apply method to clear useClientCache at the start of each compilation
(e.g., register a hook on compiler.hooks.compilation or compiler.hooks.watchRun
depending on the plugin entry) so hasUseClientDirective always re-reads changed
files between rebuilds; locate useClientCache and hasUseClientDirective and add
the cache.clear() call inside the apply's compilation/watch hook to invalidate
cached results on each new build.
| } else if (process.env.RSC_BUNDLE_ONLY) { | ||
| const rscConfig = rscWebpackConfig(); | ||
| // eslint-disable-next-line no-console | ||
| console.log('[React on Rails] Creating only the RSC bundle.'); | ||
| result = rscConfig; | ||
| } else { | ||
| // default is the standard client and server build | ||
| const clientConfig = clientWebpackConfig(); | ||
| const serverConfig = serverWebpackConfig(); | ||
| const rscConfig = rscWebpackConfig(); | ||
| if (envSpecific) envSpecific(clientConfig, serverConfig); | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
envSpecific is skipped for RSC config paths.
Line 23-27 never calls envSpecific, and Line 32 does not expose rscConfig. This makes RSC bundles miss environment-specific adjustments.
Proposed fix (backward-compatible)
} else if (process.env.RSC_BUNDLE_ONLY) {
const rscConfig = rscWebpackConfig();
+ if (envSpecific) envSpecific(rscConfig, null, rscConfig);
// eslint-disable-next-line no-console
console.log('[React on Rails] Creating only the RSC bundle.');
result = rscConfig;
} else {
@@
- if (envSpecific) envSpecific(clientConfig, serverConfig);
+ if (envSpecific) envSpecific(clientConfig, serverConfig, rscConfig);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (process.env.RSC_BUNDLE_ONLY) { | |
| const rscConfig = rscWebpackConfig(); | |
| // eslint-disable-next-line no-console | |
| console.log('[React on Rails] Creating only the RSC bundle.'); | |
| result = rscConfig; | |
| } else { | |
| // default is the standard client and server build | |
| const clientConfig = clientWebpackConfig(); | |
| const serverConfig = serverWebpackConfig(); | |
| const rscConfig = rscWebpackConfig(); | |
| if (envSpecific) envSpecific(clientConfig, serverConfig); | |
| // eslint-disable-next-line no-console | |
| } else if (process.env.RSC_BUNDLE_ONLY) { | |
| const rscConfig = rscWebpackConfig(); | |
| if (envSpecific) envSpecific(rscConfig, null, rscConfig); | |
| // eslint-disable-next-line no-console | |
| console.log('[React on Rails] Creating only the RSC bundle.'); | |
| result = rscConfig; | |
| } else { | |
| const clientConfig = clientWebpackConfig(); | |
| const serverConfig = serverWebpackConfig(); | |
| const rscConfig = rscWebpackConfig(); | |
| if (envSpecific) envSpecific(clientConfig, serverConfig, rscConfig); | |
| // eslint-disable-next-line no-console |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/webpackConfig.js` around lines 23 - 33, The RSC-only branch
never applies env-specific adjustments and doesn't expose the rsc config for
modification; update the RSC_BUNDLE_ONLY branch so after creating rscConfig via
rscWebpackConfig() you call envSpecific appropriately (e.g., envSpecific(null,
null, rscConfig) or extend envSpecific to accept a third rscConfig arg) before
assigning result = rscConfig, ensuring rscConfig is passed to envSpecific
exactly like clientConfig and serverConfig are in the multi-config path
(symbols: rscWebpackConfig, envSpecific, clientWebpackConfig,
serverWebpackConfig, result).
| await new Promise((resolve) => setTimeout(resolve, 800)); | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); |
There was a problem hiding this comment.
Bug: hardcoded localhost URL breaks all non-local environments
This URL is hardcoded to localhost:3000, which will fail in staging, production, CI, or any environment where the Rails server is not on port 3000 of the same host.
In a server component, the URL should be constructed from an environment variable or the railsContext (which includes host/port). For example:
| const response = await fetch('http://localhost:3000/comments.json'); | |
| const apiBase = process.env.RAILS_RELATIVE_URL_ROOT | |
| ? `http://localhost:${process.env.PORT || 3000}` | |
| : `http://localhost:${process.env.PORT || 3000}`; | |
| const response = await fetch(`${apiBase}/comments.json`); |
Or more robustly, pass the base URL via an environment variable (API_BASE_URL) so it can be overridden per environment.
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); |
There was a problem hiding this comment.
Bug: missing HTTP error handling
response.json() will throw a parse error on non-2xx responses (e.g. Rails returns an HTML 500 page). The component will crash without a user-visible error, bypassing the Suspense fallback.
| const comments = await response.json(); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`); | |
| } | |
| const comments = await response.json(); |
| async function CommentsFeed() { | ||
| // Simulate network latency to demonstrate streaming | ||
| // eslint-disable-next-line no-promise-executor-return | ||
| await new Promise((resolve) => setTimeout(resolve, 800)); |
There was a problem hiding this comment.
Demo artifact: artificial 800ms delay should not ship
This delay was added to demonstrate Suspense streaming, which is a reasonable tutorial goal, but it will make every page load noticeably slower in any real use. If this demo is meant to be a tutorial reference that people copy from, an unconditional 800ms penalty is a bad pattern to model.
Consider making it conditional on a query param or dev-only env var, or at minimum include a prominent warning comment that this should be removed in real usage.
| // These libraries (combined ~200KB) never reach the client. | ||
| const rawHtml = marked.parse(comment.text || ''); | ||
| const safeHtml = sanitizeHtml(rawHtml, { | ||
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), |
There was a problem hiding this comment.
Security: allowing img tags enables remote content injection
Adding img to allowedTags means comment authors can embed arbitrary remote images, which:
- Leaks the reader's IP address to any third-party host (pixel tracking)
- In combination with
srcattributes pointing to internal services, can be used as a probe for SSRF — the server fetches the RSC payload, not the browser, so an attacker-controlled<img src="http://169.254.169.254/...">may trigger a server-side request in some RSC runtime configurations
Unless image embedding in comments is an intentional feature, remove img from the allowed list. If images are needed, allowlist only relative paths or a known CDN via allowedAttributes.
| @@ -1,5 +1,7 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Semantically incorrect 'use client' on a server registration file
serverRegistration.jsx is the SSR entry point — it runs in the Node/ExecJS server renderer to register React components for server-side rendering. Adding 'use client' here tells the RSC bundler to treat this module as a client boundary and include it in the client manifest, which is the opposite of what a server registration file should be.
The same concern applies to RouterApp.server.jsx (also marked 'use client' in this PR) — a file explicitly named .server. should not carry a client directive.
These directives appear to have been added globally to all entry points, but the server registration files should be excluded. The RSC bundle's rsc-bundle.js already handles the correct server/client split.
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Bug: module-level cache causes stale results in watch mode
useClientCache is a module-level Map. In --watch mode, the plugin instance is reused across incremental builds, so the cache is never cleared. If a developer removes the 'use client' directive from a file, the cache will return the stale true result from the prior build, and the component will incorrectly remain in the client manifest until the build process is restarted.
Move the cache inside the apply method (per-build scope), or clear it in a compiler.hooks.watchRun or compiler.hooks.beforeCompile hook:
apply(compiler) {
compiler.hooks.beforeCompile.tap('RspackRscPlugin', () => {
useClientCache.clear();
});
// ...
}
| const { getBundler } = require('./bundlerUtils'); | ||
| const { RspackRscPlugin } = require('./rspackRscPlugin'); | ||
|
|
||
| function extractLoader(rule, loaderName) { |
There was a problem hiding this comment.
Duplication: extractLoader is copy-pasted from serverWebpackConfig.js
The identical function was added to serverWebpackConfig.js in this PR (with a JSDoc comment) and is now duplicated here without the doc. Extract it to a shared utility, e.g. config/webpack/loaderUtils.js, and import it in both configs.
File-level ESLint rules require block comment /* */ syntax, not single-line //. Update RspackRscPlugin regex to also recognize block comments before 'use client' directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: Add React Server Components with React on Rails ProOverall this is a solid RSC integration — the three-bundle pipeline, custom Bugs / Security1. Hardcoded 2. Missing HTTP error check in 3. 4. Code Quality5. 6. 7. Unconditional 800ms artificial delay (CommentsFeed.jsx line 17) Minor / Informational
|
| // Fetch comments directly from the Rails API — no client-side fetch needed | ||
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); | ||
|
|
There was a problem hiding this comment.
Bug: hard-coded localhost URL breaks non-local environments.
This URL is baked in at build time, so the RSC demo will silently fail in staging, CI, or any Docker-based setup where Rails is not on localhost:3000.
Consider using a relative URL (Rails server-side fetch can resolve relative paths via the react_on_rails_pro request context), or at minimum an environment variable:
| const baseUrl = process.env.RAILS_API_BASE_URL || 'http://localhost:3000'; | |
| const response = await fetch(`${baseUrl}/comments.json`); |
If react-on-rails-rsc injects a request context you can derive the base URL from, that would be the cleanest approach.
| const response = await fetch('http://localhost:3000/comments.json'); | ||
| const comments = await response.json(); | ||
|
|
||
| // Use lodash to process (stays on server) |
There was a problem hiding this comment.
Missing fetch error handling — if the Rails server is down or returns a non-2xx status, response.json() will throw (or return an error body). The unhandled rejection will crash the RSC render rather than showing a graceful fallback.
| // Use lodash to process (stays on server) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`); | |
| } | |
| const comments = await response.json(); |
Alternatively, wrap the entire fetch block in a try/catch and return a fallback UI.
| key={comment.id} | ||
| className="bg-white border border-slate-200 rounded-lg p-4 shadow-sm hover:shadow-md transition-shadow" | ||
| > | ||
| <div className="flex items-center justify-between mb-2"> | ||
| <span className="font-semibold text-slate-800">{comment.author}</span> |
There was a problem hiding this comment.
sanitizeHtml adds img tags with no src restriction.
sanitizeHtml.defaults.allowedTags + img uses the default allowed attributes for img, which includes an unrestricted src. This allows commenters to embed arbitrary external images — both a tracking-pixel vector and a potential SSRF indicator (the server fetches nothing here, but browsers will).
If the goal is just to render user markdown safely, dropping img from the allowed list is the safer default for a tutorial:
| key={comment.id} | |
| className="bg-white border border-slate-200 rounded-lg p-4 shadow-sm hover:shadow-md transition-shadow" | |
| > | |
| <div className="flex items-center justify-between mb-2"> | |
| <span className="font-semibold text-slate-800">{comment.author}</span> | |
| const safeHtml = sanitizeHtml(rawHtml); |
Or, if images are intentional, restrict src to safe origins:
allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']),
allowedAttributes: {
...sanitizeHtml.defaults.allowedAttributes,
img: [{ name: 'src', multiple: false, values: [] }], // or a specific allowlist
},
| @@ -1,8 +1,10 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
'use client' on a .server.jsx file is semantically contradictory.
RouterApp.server.jsx is used for SSR (server-side rendering via ExecJS). Adding 'use client' here means the RSC bundler will treat it as a client boundary and refuse to import server-only modules from it. The intent of this directive was presumably to prevent the RSC webpack loader from incorrectly treating it as a server component — but the right fix is to ensure the RSC bundle's WebpackLoader excludes files not under the server-components tree, rather than adding 'use client' to existing SSR entry points.
The same concern applies to serverRegistration.jsx — a file named "server registration" that now carries 'use client'.
| if (!Array.isArray(rule.use)) return null; | ||
| return rule.use.find((item) => { | ||
| const testValue = typeof item === 'string' ? item : item?.loader; | ||
| return testValue && testValue.includes(loaderName); | ||
| }); | ||
| } | ||
|
|
||
| const configureRsc = () => { | ||
| const bundler = getBundler(); |
There was a problem hiding this comment.
extractLoader is duplicated verbatim in serverWebpackConfig.js.
Both files define the same function (lines 1120–1130 in the diff for serverWebpackConfig.js). serverWebpackConfig.js already exports it — this file should import it instead:
| if (!Array.isArray(rule.use)) return null; | |
| return rule.use.find((item) => { | |
| const testValue = typeof item === 'string' ? item : item?.loader; | |
| return testValue && testValue.includes(loaderName); | |
| }); | |
| } | |
| const configureRsc = () => { | |
| const bundler = getBundler(); | |
| const { extractLoader } = require('./serverWebpackConfig'); |
(The default export wrapping added to serverWebpackConfig.js was presumably done in anticipation of this, but the import never happened.)
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Module-level useClientCache is never cleared between watch rebuilds.
The Map lives at module scope, so across incremental rebuilds in --watch mode a file that had its 'use client' directive added or removed after the first build will continue to use the cached (stale) result.
The simplest fix is to clear the cache at the start of each compilation:
apply(compiler) {
compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => {
useClientCache.clear(); // invalidate per build
// ...
});
}Alternatively, scope the cache to the RspackRscPlugin instance rather than the module.
Review: Add React Server Components with React on Rails ProSolid work adding RSC support — the three-bundle pipeline, the custom Bugs / Correctness
Security
Architecture / Maintainability
Minor
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
config/webpack/rspackRscPlugin.js (1)
13-16:⚠️ Potential issue | 🟡 MinorClear
useClientCacheon each compilation to avoid stale manifest decisions.
useClientCachepersists across rebuilds, so flipping'use client'during watch mode can leave stale results until restart.🔧 Suggested patch
apply(compiler) { compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => { + useClientCache.clear(); compilation.hooks.processAssets.tap(Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 13 - 16, The Map useClientCache is persisting across rebuilds causing stale results; modify the plugin to clear useClientCache at the start of each compilation/rebuild (e.g., in the plugin hook that runs per compilation/watch run) so hasUseClientDirective reads fresh data; locate the useClientCache and hasUseClientDirective symbols and add a call to useClientCache.clear() in the per-compilation hook (compiler/compilation/watchRun) so toggling 'use client' during watch mode updates correctly.
🧹 Nitpick comments (1)
config/webpack/rspackRscPlugin.js (1)
104-106: Avoid blanket exclusion ofnode_modulesin client-reference discovery.This can drop valid third-party components that correctly declare
'use client', causing missing manifest entries when used from server components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 104 - 106, The current unconditional skip "if (resource.includes('node_modules')) return;" drops third-party files that may legitimately be client components; change it to only skip node_modules files that do NOT declare the client directive by reading the file contents (using the existing resource path) and checking for a top-of-file "use client" directive (e.g., a regex for /^(['"])use client\1/ at the start). In other words, in the client-reference discovery logic replace the blanket exclusion of resource.includes('node_modules') with a conditional that reads the file and only returns when the file is in node_modules and does not contain a "use client" directive (keep referencing the same resource variable used in this snippet).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/app/bundles/server-components/components/ServerInfo.jsx`:
- Around line 37-48: Prettier flagged formatting in the ServerInfo component:
the paragraph text wrapping around the em-dash and a JSX attribute line-break in
the mapped elements (the JSX that renders grouped.map and the inner div/span
attributes) are misformatted; run your formatter (npx prettier --write
client/app/bundles/server-components/components/ServerInfo.jsx) or reflow the
paragraph so the em-dash and inline <code> elements are on the same logical line
and ensure each JSX attribute stays on the same line for the small spans/divs
(keys: grouped, labels, the mapped arrow functions rendering group.map and the
inner divs/spans) to satisfy ESLint/Prettier rules.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 19-25: The current try block opens a file descriptor (fd) then
calls fs.readSync and fs.closeSync, but if fs.readSync throws the fd is not
closed; refactor the logic in rspackRscPlugin.js to declare fd in an outer
scope, wrap the open/read in try and ensure fs.closeSync(fd) is invoked in a
finally block (only if fd is a valid descriptor), so the file descriptor is
always cleaned up even on errors when reading; update the block around fd,
fs.openSync, fs.readSync and fs.closeSync accordingly.
---
Duplicate comments:
In `@config/webpack/rspackRscPlugin.js`:
- Around line 13-16: The Map useClientCache is persisting across rebuilds
causing stale results; modify the plugin to clear useClientCache at the start of
each compilation/rebuild (e.g., in the plugin hook that runs per
compilation/watch run) so hasUseClientDirective reads fresh data; locate the
useClientCache and hasUseClientDirective symbols and add a call to
useClientCache.clear() in the per-compilation hook
(compiler/compilation/watchRun) so toggling 'use client' during watch mode
updates correctly.
---
Nitpick comments:
In `@config/webpack/rspackRscPlugin.js`:
- Around line 104-106: The current unconditional skip "if
(resource.includes('node_modules')) return;" drops third-party files that may
legitimately be client components; change it to only skip node_modules files
that do NOT declare the client directive by reading the file contents (using the
existing resource path) and checking for a top-of-file "use client" directive
(e.g., a regex for /^(['"])use client\1/ at the start). In other words, in the
client-reference discovery logic replace the blanket exclusion of
resource.includes('node_modules') with a conditional that reads the file and
only returns when the file is in node_modules and does not contain a "use
client" directive (keep referencing the same resource variable used in this
snippet).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d9fdfe1-ad04-455f-bcbc-3f1073f7bdd1
📒 Files selected for processing (6)
client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsxclient/app/bundles/server-components/components/CommentsFeed.jsxclient/app/bundles/server-components/components/ServerInfo.jsxclient/app/bundles/server-components/components/TogglePanel.jsxclient/app/packs/stimulus-bundle.jsconfig/webpack/rspackRscPlugin.js
✅ Files skipped from review due to trivial changes (1)
- client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx
| <p className="text-xs text-emerald-600 mb-4 font-medium"> | ||
| This data comes from the Node.js <code className="bg-emerald-100 px-1 rounded">os</code> module | ||
| — it runs only on the server. The <code className="bg-emerald-100 px-1 rounded">lodash</code> library | ||
| used to format it never reaches the browser. | ||
| </p> | ||
| <div className="grid md:grid-cols-2 gap-x-8 gap-y-1"> | ||
| {grouped.map((group) => ( | ||
| <div key={group.map(([k]) => k).join('-')} className="space-y-1"> | ||
| {group.map(([key, value]) => ( | ||
| <div key={key} className="flex justify-between py-1.5 border-b border-emerald-100 last:border-0"> | ||
| <span className="text-sm text-emerald-700 font-medium">{labels[key] || key}</span> | ||
| <span className="text-sm text-emerald-900 font-mono">{value}</span> |
There was a problem hiding this comment.
Fix Prettier formatting violations.
ESLint/Prettier flagged formatting issues on lines 38-40 (text wrapping around em-dash) and line 46 (JSX attribute formatting). Run your formatter to resolve these before merging.
Suggested formatting fix
<p className="text-xs text-emerald-600 mb-4 font-medium">
- This data comes from the Node.js <code className="bg-emerald-100 px-1 rounded">os</code> module
- — it runs only on the server. The <code className="bg-emerald-100 px-1 rounded">lodash</code> library
- used to format it never reaches the browser.
+ This data comes from the Node.js{' '}
+ <code className="bg-emerald-100 px-1 rounded">os</code> module — it runs only on the server.
+ The <code className="bg-emerald-100 px-1 rounded">lodash</code> library used to format it
+ never reaches the browser.
</p>
<div className="grid md:grid-cols-2 gap-x-8 gap-y-1">
{grouped.map((group) => (
<div key={group.map(([k]) => k).join('-')} className="space-y-1">
{group.map(([key, value]) => (
- <div key={key} className="flex justify-between py-1.5 border-b border-emerald-100 last:border-0">
+ <div
+ key={key}
+ className="flex justify-between py-1.5 border-b border-emerald-100 last:border-0"
+ >
<span className="text-sm text-emerald-700 font-medium">{labels[key] || key}</span>Alternatively, run npx prettier --write client/app/bundles/server-components/components/ServerInfo.jsx to auto-fix.
🧰 Tools
🪛 ESLint
[error] 38-38: Insert ·—·it
(prettier/prettier)
[error] 39-39: Replace ·—·it·runs·only·on·the·server.·The·<code·className="bg-emerald-100·px-1·rounded">lodash</code>·library with ·runs·only·on·the·server.·The·<code·className="bg-emerald-100·px-1·rounded">lodash</code>·library·used
(prettier/prettier)
[error] 40-40: Delete ·used
(prettier/prettier)
[error] 46-46: Replace ·key={key}·className="flex·justify-between·py-1.5·border-b·border-emerald-100·last:border-0" with ⏎················key={key}⏎················className="flex·justify-between·py-1.5·border-b·border-emerald-100·last:border-0"⏎··············
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/app/bundles/server-components/components/ServerInfo.jsx` around lines
37 - 48, Prettier flagged formatting in the ServerInfo component: the paragraph
text wrapping around the em-dash and a JSX attribute line-break in the mapped
elements (the JSX that renders grouped.map and the inner div/span attributes)
are misformatted; run your formatter (npx prettier --write
client/app/bundles/server-components/components/ServerInfo.jsx) or reflow the
paragraph so the em-dash and inline <code> elements are on the same logical line
and ensure each JSX attribute stays on the same line for the small spans/divs
(keys: grouped, labels, the mapped arrow functions rendering group.map and the
inner divs/spans) to satisfy ESLint/Prettier rules.
| try { | ||
| // Read the first ~200 bytes — 'use client' must be at the very top of the file | ||
| const fd = fs.openSync(filePath, 'r'); | ||
| const buf = Buffer.alloc(200); | ||
| fs.readSync(fd, buf, 0, 200, 0); | ||
| fs.closeSync(fd); | ||
|
|
There was a problem hiding this comment.
Guard file descriptor cleanup with finally to prevent FD leaks on read errors.
If fs.readSync throws after fs.openSync, fs.closeSync is skipped.
🛠️ Suggested patch
try {
// Read the first ~200 bytes — 'use client' must be at the very top of the file
- const fd = fs.openSync(filePath, 'r');
- const buf = Buffer.alloc(200);
- fs.readSync(fd, buf, 0, 200, 0);
- fs.closeSync(fd);
+ const fd = fs.openSync(filePath, 'r');
+ try {
+ const buf = Buffer.alloc(200);
+ fs.readSync(fd, buf, 0, 200, 0);
+
+ const head = buf.toString('utf-8');
+ // Check for 'use client' as the first statement.
+ // Allow comments (single-line // or block /* */) before the directive.
+ result = /^(?:\s*(?:\/\/[^\n]*\n|\/\*[\s\S]*?\*\/))*\s*['"]use client['"]/.test(head);
+ } finally {
+ fs.closeSync(fd);
+ }
-
- const head = buf.toString('utf-8');
- // Check for 'use client' as the first statement.
- // Allow comments (single-line // or block /* */) before the directive.
- result = /^(?:\s*(?:\/\/[^\n]*\n|\/\*[\s\S]*?\*\/))*\s*['"]use client['"]/.test(head);
} catch (_) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/rspackRscPlugin.js` around lines 19 - 25, The current try
block opens a file descriptor (fd) then calls fs.readSync and fs.closeSync, but
if fs.readSync throws the fd is not closed; refactor the logic in
rspackRscPlugin.js to declare fd in an outer scope, wrap the open/read in try
and ensure fs.closeSync(fd) is invoked in a finally block (only if fd is a valid
descriptor), so the file descriptor is always cleaned up even on errors when
reading; update the block around fd, fs.openSync, fs.readSync and fs.closeSync
accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae420af2b8
ℹ️ 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 useClientCache = new Map(); | ||
|
|
||
| function hasUseClientDirective(filePath) { | ||
| if (useClientCache.has(filePath)) return useClientCache.get(filePath); |
There was a problem hiding this comment.
The useClientCache map is process-global and never invalidated, so in watch mode the plugin can keep returning stale directive results for a file path after edits. If a developer adds or removes 'use client' and recompiles, the manifest can remain out of sync until the watcher is restarted, leading to missing or incorrect client component resolution. Clear this cache at the start of each compilation (or key by content/mtime) to keep manifests accurate.
Useful? React with 👍 / 👎.
| const buf = Buffer.alloc(200); | ||
| fs.readSync(fd, buf, 0, 200, 0); |
There was a problem hiding this comment.
Directive detection reads only the first 200 bytes, so files with a longer leading comment/header before 'use client' are misclassified as server modules. In that case the module is omitted from the emitted RSC manifest and runtime resolution of that client component fails. The parser should scan the full prologue (or full file) rather than hard-capping at 200 bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Server and RSC bundles overwrite same manifest file
- Removed
RspackRscPlugin({ isServer: true })from the server webpack config so only the RSC compilation emitsreact-server-client-manifest.jsonand no cross-process overwrite occurs.
- Removed
Or push these changes by commenting:
@cursor push c304ccb508
Preview (c304ccb508)
diff --git a/config/webpack/serverWebpackConfig.js b/config/webpack/serverWebpackConfig.js
--- a/config/webpack/serverWebpackConfig.js
+++ b/config/webpack/serverWebpackConfig.js
@@ -5,7 +5,6 @@
const { config } = require('shakapacker');
const commonWebpackConfig = require('./commonWebpackConfig');
const { getBundler } = require('./bundlerUtils');
-const { RspackRscPlugin } = require('./rspackRscPlugin');
/**
* Extract a specific loader from a webpack rule's use array.
@@ -169,9 +168,6 @@
// If using the React on Rails Pro node server renderer, uncomment the next line
// serverWebpackConfig.target = 'node'
- // RSC: Generate react-server-client-manifest.json for SSR component resolution
- serverWebpackConfig.plugins.push(new RspackRscPlugin({ isServer: true }));
-
return serverWebpackConfig;
};You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit ae420af. Configure here.
| // serverWebpackConfig.target = 'node' | ||
|
|
||
| // RSC: Generate react-server-client-manifest.json for SSR component resolution | ||
| serverWebpackConfig.plugins.push(new RspackRscPlugin({ isServer: true })); |
There was a problem hiding this comment.
Server and RSC bundles overwrite same manifest file
High Severity
Both serverWebpackConfig.js and rscWebpackConfig.js add new RspackRscPlugin({ isServer: true }), which emits react-server-client-manifest.json to the same ssr-generated/ output directory. In watch mode, the wp-server and wp-rsc processes run independently and non-deterministically overwrite each other's manifest. The two manifests contain different module IDs for the same client components, so whichever write wins leaves the other bundle's references broken. The RSC renderer likely needs module IDs from the RSC compilation specifically, meaning a server-bundle rebuild silently corrupts the RSC manifest.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ae420af. Configure here.
|
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
Or push these changes by commenting: Preview (538b2a7fa2)diff --git a/client/app/bundles/server-components/components/CommentsFeed.jsx b/client/app/bundles/server-components/components/CommentsFeed.jsx
--- a/client/app/bundles/server-components/components/CommentsFeed.jsx
+++ b/client/app/bundles/server-components/components/CommentsFeed.jsx
@@ -11,12 +11,25 @@
const marked = new Marked();
marked.use(gfmHeadingId());
+function commentsApiUrl() {
+ if (process.env.RSC_COMMENTS_URL) return process.env.RSC_COMMENTS_URL;
+
+ if (process.env.PRODUCTION_HOST) {
+ const host = process.env.PRODUCTION_HOST.replace(/\/$/, '');
+ const baseUrl = host.match(/^https?:\/\//) ? host : `https://${host}`;
+ return `${baseUrl}/comments.json`;
+ }
+
+ const port = process.env.PORT || '3000';
+ return `http://127.0.0.1:${port}/comments.json`;
+}
+
async function CommentsFeed() {
// Simulate network latency to demonstrate streaming
await new Promise((resolve) => setTimeout(resolve, 800));
// Fetch comments directly from the Rails API — no client-side fetch needed
- const response = await fetch('http://localhost:3000/comments.json');
+ const response = await fetch(commentsApiUrl());
const comments = await response.json();
// Use lodash to process (stays on server)
diff --git a/config/webpack/rspackRscPlugin.js b/config/webpack/rspackRscPlugin.js
--- a/config/webpack/rspackRscPlugin.js
+++ b/config/webpack/rspackRscPlugin.js
@@ -48,6 +48,7 @@
apply(compiler) {
compiler.hooks.thisCompilation.tap('RspackRscPlugin', (compilation) => {
+ useClientCache.clear();
compilation.hooks.processAssets.tap(
{
name: 'RspackRscPlugin',
diff --git a/config/webpack/serverWebpackConfig.js b/config/webpack/serverWebpackConfig.js
--- a/config/webpack/serverWebpackConfig.js
+++ b/config/webpack/serverWebpackConfig.js
@@ -8,21 +8,6 @@
const { RspackRscPlugin } = require('./rspackRscPlugin');
/**
- * Extract a specific loader from a webpack rule's use array.
- *
- * @param {Object} rule - Webpack rule with a use array
- * @param {string} loaderName - Substring to match against loader names
- * @returns {Object|null} The matching loader entry, or null
- */
-function extractLoader(rule, loaderName) {
- if (!Array.isArray(rule.use)) return null;
- return rule.use.find((item) => {
- const testValue = typeof item === 'string' ? item : item?.loader;
- return testValue && testValue.includes(loaderName);
- });
-}
-
-/**
* Generates the server-side rendering (SSR) bundle configuration.
*
* This creates a separate bundle optimized for server-side rendering:
@@ -175,4 +160,4 @@
return serverWebpackConfig;
};
-module.exports = { default: configureServer, extractLoader };
+module.exports = configureServer;
diff --git a/config/webpack/webpackConfig.js b/config/webpack/webpackConfig.js
--- a/config/webpack/webpackConfig.js
+++ b/config/webpack/webpackConfig.js
@@ -2,7 +2,7 @@
// https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/blob/master/config/webpack/webpackConfig.js
const clientWebpackConfig = require('./clientWebpackConfig');
-const { default: serverWebpackConfig } = require('./serverWebpackConfig');
+const serverWebpackConfig = require('./serverWebpackConfig');
const rscWebpackConfig = require('./rscWebpackConfig');
const webpackConfig = (envSpecific) => {You can send follow-ups to the cloud agent here. |
1 participant
Summary
react_on_railstoreact_on_rails_pro(16.5.1) with React 19.0.4 andreact-on-rails-rsc/server-componentsshowcasing server components with async data fetching, Suspense streaming, and interactive client componentsRspackRscPluginfor manifest generation since the standardRSCWebpackPluginuses webpack internals incompatible with RspackRSC Demo Features
osmodule andlodash(never shipped to client browser)Suspensestreaming — comments fetched directly from Rails API on server'use client'TogglePanel) mixed with server-rendered content (donut pattern)marked+sanitize-html(~200KB of libraries that stay server-side)Infrastructure Changes
rscWebpackConfig.js— RSC bundle targeting Node.js withreact-on-rails-rsc/WebpackLoaderrspackRscPlugin.js— Lightweight Rspack-compatible alternative to the webpack-onlyRSCWebpackPlugin'use client'directives added to all existing client component entry pointsreact-on-rails→react-on-rails-profor third-party compatibilityrsc-bundle.jsentry withregisterServerComponentserver/client registrationrsc_payload_routeadded to routes for RSC payload deliveryTest plan
spec/requests/server_render_check_spec.rb)/server-componentsto verify RSC demo page renders/rsc_payload/ServerComponentsPage🤖 Generated with Claude Code
Note
Medium Risk
Adds a new RSC runtime path and introduces a third (Node-targeted) bundle plus custom manifest generation, which can break builds/SSR if misconfigured. Also switches core React-on-Rails integration to
react_on_rails_pro, affecting app-wide component registration and bundling.Overview
Adds React Server Components support using
react_on_rails_pro. The Rails app now configures RSC support, exposes anrsc_payload_route, and adds a new/server-componentsdemo page rendered viareact_component("ServerComponentsPage")plus a dedicated client chunk for RSC client components.Upgrades and refactors the JS/Ruby integration. Replaces
react_on_railswithreact_on_rails_proacross the app, pins React to19.0.4, addsreact-on-rails-rsc, and updates many components/entrypoints with'use client'directives for RSC compatibility.Introduces a 3-bundle build pipeline (client/server/RSC). Adds
rscWebpackConfig.js, extendswebpackConfig.jsandProcfile.devto build/watch an RSC bundle, and adds a customRspackRscPluginto emit RSC manifests under Rspack, plus resolver aliasing/fallbacks to keep client builds from pulling Node-only deps.Reviewed by Cursor Bugbot for commit ae420af. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Chores