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:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds React Server Components (RSC) support: swaps to react_on_rails_pro, adds RSC webpack configs and an Rspack plugin, introduces a node renderer and RSC packs/entrypoints, adds a server-components demo (server + client pieces), and wires initializer, routes, and dev processes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Rails
participant NodeRSC as Node RSC Renderer
participant ClientLoader as Client Loader
Browser->>Rails: GET /server-components
activate Rails
Rails->>NodeRSC: Request RSC render for ServerComponentsPage
deactivate Rails
activate NodeRSC
NodeRSC->>NodeRSC: Render ServerInfo (sync)
NodeRSC->>NodeRSC: Await CommentsFeed (fetch, markdown/sanitize)
NodeRSC->>Browser: Stream RSC/Flight payload
deactivate NodeRSC
Browser->>ClientLoader: Fetch client manifest & chunks
ClientLoader->>Browser: Load TogglePanel chunk
Browser->>Browser: Hydrate TogglePanel (interactive)
sequenceDiagram
participant Bundler
participant RscConfig
participant Plugin as RspackRscPlugin
participant Output
Bundler->>RscConfig: Load rscWebpackConfig (RSC_BUNDLE_ONLY)
RscConfig->>RscConfig: Strip CSS, set react-server condition, enforce single chunk
Bundler->>Plugin: Run RspackRscPlugin during compilation
Plugin->>Plugin: Scan chunks/modules for 'use client'
Plugin->>Output: Emit react-client-manifest.json (and SSR manifest when applicable)
Output->>Bundler: RSC manifests & bundle 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}
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
Show resolved
Hide resolved
![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".
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
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}
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
- 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>
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
| 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)); | |
| } |
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
Show resolved
Hide resolved
- 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>
| // 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
client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
Review: Add React Server Components with React on Rails ProThis is a well-structured and impressive addition — the three-bundle pipeline is clearly thought out, and the custom SecurityHardcoded renderer password (must fix before any production use)
Hostname/system info exposure in Code Quality
No error boundary around
Minor Notes
Overall the architecture is solid and the RSC integration path is clearly documented. Addressing the password handling and |
Two issues: 1. RSC bundle (127 errors): Remove server-bundle-generated.js import from rsc-bundle.js. It includes RouterApp.server.jsx (traditional SSR component) that uses react-redux/react-router with client-only React APIs incompatible with the react-server condition. Client references are handled automatically by the RSC loader/plugin. 2. Server bundle (3 errors): Add Node.js builtin fallbacks (path, fs, stream) to server webpack config. react-on-rails-pro now includes RSC modules that import these builtins, but they aren't used in the traditional SSR path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| ...sanitizeHtml.defaults.allowedAttributes, | ||
| img: ['src', 'alt', 'title', 'width', 'height'], | ||
| }, | ||
| allowedSchemes: ['https', 'http', 'data'], |
There was a problem hiding this comment.
Security: data: URI scheme should be removed from allowedSchemes
Allowing data: URIs in image src attributes creates a meaningful attack surface. While modern browsers don't execute scripts from data: in img src directly, it can still be used to:
- Load arbitrary content that leaks the user's IP/cookies to an attacker's server via tracking pixels
- Render HTML or SVG payloads in some older browsers
- Bypass content security policies that block external image loading
Since this is user-generated comment content, only https should be allowed:
| allowedSchemes: ['https', 'http', 'data'], | |
| allowedSchemes: ['https'], |
Or at minimum remove data: and limit to ['https', 'http'].
| const config = { | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | ||
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | ||
| password: process.env.RENDERER_PASSWORD || 'devPassword', |
There was a problem hiding this comment.
Security: hardcoded fallback password
The || 'devPassword' fallback means if RENDERER_PASSWORD is not set in any environment (including production), the renderer accepts the well-known default password. The same default appears in config/initializers/react_on_rails_pro.rb via ENV.fetch("RENDERER_PASSWORD", "devPassword").
For a tutorial repo this is low risk, but the pattern being demonstrated is unsafe for any real deployment. Consider failing hard if the env var is missing in production:
| password: process.env.RENDERER_PASSWORD || 'devPassword', | |
| password: process.env.RENDERER_PASSWORD || (process.env.NODE_ENV === 'production' ? (() => { throw new Error('RENDERER_PASSWORD must be set in production') })() : 'devPassword'), |
Or at minimum add a warning log when the fallback is used.
| totalMemory: (os.totalmem() / (1024 * 1024 * 1024)).toFixed(1), | ||
| freeMemory: (os.freemem() / (1024 * 1024 * 1024)).toFixed(1), | ||
| cpus: os.cpus().length, | ||
| hostname: os.hostname(), |
There was a problem hiding this comment.
Info leak: os.hostname() exposes internal server hostname
os.hostname() returns the server's internal hostname (e.g. ip-10-0-1-42.us-east-1.compute.internal on AWS, or the container name in Kubernetes). This is visible to anyone who loads the /server-components page and reveals infrastructure naming conventions.
For a demo page this may be intentional to show RSC capabilities, but it's worth noting this should be stripped or masked before any real deployment. Consider adding a comment here to call this out:
| hostname: os.hostname(), | |
| hostname: os.hostname(), // NOTE: remove or mask before any real deployment — leaks internal host identity |
|
|
||
| // Target Node.js so server-only modules (os, fs, stream, etc.) resolve correctly | ||
| rscConfig.target = 'node'; | ||
| rscConfig.devtool = 'eval'; |
There was a problem hiding this comment.
eval devtool is unconditional — should be dev-only
eval is the fastest devtool option but it inlines eval() calls into the bundle, which:
- Violates
Content-Security-Policy: script-src 'self'headers (breaks CSP on any production site using it) - Is inappropriate for production builds
The server webpack config uses eval only in context where it's already a non-minified debug bundle, but the RSC bundle should respect the environment:
| rscConfig.devtool = 'eval'; | |
| rscConfig.devtool = process.env.NODE_ENV === 'production' ? 'source-map' : 'eval'; |
| 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 shared across all three build configs in the same process
useClientCache is a module singleton. When webpackConfig.js runs without *_BUNDLE_ONLY flags, all three configs (client, server, RSC) are built in the same Node.js process, and all three RspackRscPlugin instances share this same Map. If compilations overlap (parallel or interleaved), one compilation's clear() call in thisCompilation can evict entries that another concurrent compilation already populated, causing a 'use client' check to re-read disk for files that haven't changed.
The fix is to move the cache inside the class instance so each plugin has its own:
| const useClientCache = new Map(); | |
| const useClientCache = new Map(); | |
Actually, move the declaration and hasUseClientDirective function inside the class, making the cache an instance property:
class RspackRscPlugin {
constructor(options) {
// ...
this._useClientCache = new Map();
}
// ...
_hasUseClientDirective(filePath) {
if (this._useClientCache.has(filePath)) return this._useClientCache.get(filePath);
// ... rest of logic using this._useClientCache
}
| // Emit SSR manifest (maps module IDs to SSR module data) | ||
| if (!this.isServer) { | ||
| compilation.emitAsset( | ||
| this.ssrManifestFilename, |
There was a problem hiding this comment.
SSR manifest is always emitted as {}
react-ssr-manifest.json maps server module IDs to their SSR bundles so the React server renderer can resolve client references. Emitting an empty object means any server-side rendering that needs to resolve a 'use client' boundary during SSR will fail silently (or produce a blank component) rather than hydrating correctly.
This may be intentional if react-on-rails-pro doesn't currently use the SSR manifest, but it should be documented or populated the same way the client manifest is. If it truly isn't needed, remove the emission entirely to avoid confusion:
| this.ssrManifestFilename, | |
| // SSR manifest intentionally omitted — react-on-rails-pro resolves client | |
| // references through the client manifest only. |
PR Review: Add React Server Components with React on Rails ProThis is a significant, well-structured PR that adds RSC support via a custom Rspack plugin and a three-bundle build pipeline. The implementation is impressive in scope. Here are the findings: Security (requires attention)
Bugs
Minor / Good Patterns Worth Noting
Overall the RSC infrastructure is solid and the demo does a great job showcasing the donut pattern and Suspense streaming. The security items (especially |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/webpack/serverWebpackConfig.js (1)
9-23:⚠️ Potential issue | 🟡 MinorNormalize
extractLoader’s exported return contract.This helper still returns
undefined(miss) and may return astring(string loader entries), while the doc saysObject|null. Since it is exported, align runtime behavior/docs to avoid caller surprises.🩹 Proposed fix
/** * 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 + * `@returns` {string|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); - }); + return ( + rule.use.find((item) => { + const testValue = typeof item === 'string' ? item : item?.loader; + return testValue && testValue.includes(loaderName); + }) ?? null + ); }#!/bin/bash # Verify extractLoader call sites and check whether any caller assumes object-only return shape. rg -n -C3 '\bextractLoader\s*\(' rg -n -C3 '\bextractLoader\s*\([^)]*\)\.loader\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/serverWebpackConfig.js` around lines 9 - 23, The extractLoader function currently can return undefined or a raw string but the JSDoc says it returns an Object or null; update extractLoader so it always returns either null or an object with a consistent shape (e.g., { loader: string, options?: any }): ensure it checks Array.isArray(rule.use), finds the item, returns null when not found (never undefined), and when the matched item is a string wrap it into an object { loader: item }; when matched item is already an object normalize/return it (preserving loader and options). Reference the function name extractLoader and normalize its return to match the documented contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/webpack/serverWebpackConfig.js`:
- Around line 9-23: The extractLoader function currently can return undefined or
a raw string but the JSDoc says it returns an Object or null; update
extractLoader so it always returns either null or an object with a consistent
shape (e.g., { loader: string, options?: any }): ensure it checks
Array.isArray(rule.use), finds the item, returns null when not found (never
undefined), and when the matched item is a string wrap it into an object {
loader: item }; when matched item is already an object normalize/return it
(preserving loader and options). Reference the function name extractLoader and
normalize its return to match the documented contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b50aaec1-be52-474b-89d2-27a83f5cbf07
📒 Files selected for processing (3)
.gitignoreclient/app/packs/rsc-bundle.jsconfig/webpack/serverWebpackConfig.js
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- client/app/packs/rsc-bundle.js
The server bundle runs in Node.js, so use externals to resolve path, fs, stream at runtime via require() instead of replacing them with empty modules. This avoids potential runtime crashes when react-on-rails-pro RSC modules are imported transitively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| # Node renderer for server-side rendering and RSC payload generation | ||
| config.server_renderer = "NodeRenderer" | ||
| config.renderer_url = ENV["REACT_RENDERER_URL"] || "http://localhost:3800" | ||
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") |
There was a problem hiding this comment.
Security: well-known default password
ENV.fetch with a default value does not raise when the variable is missing — it silently falls back to "devPassword". Since that exact string is also committed in Procfile.dev, any deployment that forgets to set RENDERER_PASSWORD will expose a known, public default credential.
Consider using ENV.fetch without a default so it raises KeyError on startup if the variable is unset in production:
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") | |
| config.renderer_password = Rails.env.development? ? ENV.fetch("RENDERER_PASSWORD", "devPassword") : ENV.fetch("RENDERER_PASSWORD") |
Or, if a safe default for dev is acceptable, at least log a warning when the fallback is used.
| const config = { | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | ||
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | ||
| password: process.env.RENDERER_PASSWORD || 'devPassword', |
There was a problem hiding this comment.
Same hardcoded-default issue as the Rails initializer: || 'devPassword' silently uses the well-known committed string if RENDERER_PASSWORD is unset. The renderer is the HTTP endpoint that Rails calls for every SSR/RSC request, so a guessable password is a meaningful attack surface.
| password: process.env.RENDERER_PASSWORD || 'devPassword', | |
| password: process.env.RENDERER_PASSWORD ?? (() => { if (process.env.NODE_ENV === 'production') throw new Error('RENDERER_PASSWORD must be set in production'); return 'devPassword'; })(), |
Or simply document that RENDERER_PASSWORD must be set in non-dev environments and throw explicitly.
| ...sanitizeHtml.defaults.allowedAttributes, | ||
| img: ['src', 'alt', 'title', 'width', 'height'], | ||
| }, | ||
| allowedSchemes: ['https', 'http', 'data'], |
There was a problem hiding this comment.
Security: data: URI scheme allows potentially dangerous content
Permitting data: in allowedSchemes means user-supplied comment text could embed data:text/html or data:application/javascript URIs. Even though this runs server-side and the output is the sanitized HTML sent to the browser, some older browsers will navigate to or execute data:text/html URIs when set as href/src, and tracking pixels via data:image/... are trivially possible.
If the goal is to allow inline images from markdown, consider restricting to image MIME types explicitly, or drop data: entirely and rely on https for images:
| allowedSchemes: ['https', 'http', 'data'], | |
| allowedSchemes: ['https', 'http'], |
If data: images are genuinely needed, scope the restriction to the img tag only via allowedSchemesByTag.
| if (process.env.NODE_ENV !== 'production') { | ||
| await new Promise((resolve) => { | ||
| setTimeout(resolve, 800); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Artificial delay leaks into test and staging environments
The NODE_ENV !== 'production' guard means this 800 ms setTimeout fires in any non-production environment: local dev, CI test runs, staging servers that set NODE_ENV=development. That will slow down or flake automated tests, and degrade staging performance.
If the goal is purely a visual demo of Suspense streaming in local development, consider a more targeted guard, e.g. an explicit env flag:
| if (process.env.NODE_ENV !== 'production') { | |
| await new Promise((resolve) => { | |
| setTimeout(resolve, 800); | |
| }); | |
| } | |
| if (process.env.RSC_SUSPENSE_DEMO_DELAY === 'true') { | |
| await new Promise((resolve) => { | |
| setTimeout(resolve, 800); | |
| }); | |
| } |
Or remove it and rely on actual fetch latency to demonstrate streaming.
| 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); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Duplication: extractLoader is already defined (and exported) in serverWebpackConfig.js
The same function body exists verbatim in serverWebpackConfig.js. The RSC config can just import it:
| 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); | |
| }); | |
| } | |
| const { extractLoader } = require('./serverWebpackConfig'); |
Keeping one canonical copy avoids the two drifting over time.
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Potential race condition: module-level cache shared across parallel compilations
useClientCache is a module-level singleton. When all three bundles are built together (the default webpackConfig.js path), client and RSC compilations run concurrently. Each compilation calls useClientCache.clear() in its own thisCompilation hook — a clear from one compilation can evict entries that the other compilation already populated but hasn't finished reading yet.
Move the cache inside the plugin instance so each compiler has its own independent map:
| const useClientCache = new Map(); | |
| const fs = require('fs'); | |
| const { sources } = require('@rspack/core'); | |
Then in the constructor:
this.useClientCache = new Map();And update hasUseClientDirective to accept the cache as a parameter (or make it a method on the class).
Review: Add React Server Components with React on Rails ProSolid implementation overall — the three-bundle pipeline, custom Security
Correctness
Code Quality
🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7e452b77
ℹ️ 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".
| # Node renderer for server-side rendering and RSC payload generation | ||
| config.server_renderer = "NodeRenderer" | ||
| config.renderer_url = ENV["REACT_RENDERER_URL"] || "http://localhost:3800" | ||
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") |
There was a problem hiding this comment.
The renderer auth secret falls back to the hardcoded value devPassword, so any environment that forgets to set RENDERER_PASSWORD will run with a publicly known credential. This is a security regression for staging/production deployments where the Node renderer is reachable over the network (for example as a separate service), because requests can be authenticated with the default password. Consider failing fast when RENDERER_PASSWORD is missing outside development/test.
Useful? React with 👍 / 👎.
…ation Three root causes for the 37/38 rspec test failures: 1. CI missing Node renderer: The RSC branch switched SSR from ExecJS to the react-on-rails-pro NodeRenderer service (port 3800). CI never started this service, causing Net::ReadTimeout on all SSR requests. Added renderer startup step and RENDERER_PASSWORD env var to CI. 2. Server bundle externals broke in VM sandbox: The previous commit externalized Node builtins (path/fs/stream) as CommonJS requires, but the Node renderer runs bundles in a vm.createContext() sandbox where require() is unavailable. Reverted to resolve.fallback: false which stubs these unused code paths at build time instead. 3. MessageChannel undefined in VM: react-dom/server.browser.js instantiates MessageChannel at module load time. The Node renderer's VM sandbox lacks this browser global (unlike Bun/ExecJS on master). Added a BannerPlugin polyfill injected at bundle top. 4. RouterApp.server.jsx misclassified as RSC: The auto-bundling system registered it via registerServerComponent() because it lacked 'use client'. But it's a traditional SSR component (StaticRouter), not an RSC. Added 'use client' directive so it registers via ReactOnRails.register() instead. All 38 rspec tests now pass locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| config.server_renderer = "NodeRenderer" | ||
| config.renderer_url = ENV["REACT_RENDERER_URL"] || "http://localhost:3800" | ||
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") | ||
|
|
There was a problem hiding this comment.
Security: hardcoded fallback password
ENV.fetch("RENDERER_PASSWORD", "devPassword") means any deployment that forgets to set the env var silently uses a well-known default password. For production this is a real risk — use ENV.fetch("RENDERER_PASSWORD") (no default) and let it raise loudly if the variable is missing, or at minimum guard it:
| config.renderer_password = Rails.env.production? ? ENV.fetch("RENDERER_PASSWORD") : ENV.fetch("RENDERER_PASSWORD", "devPassword") |
| // sanitize-html strips any dangerous HTML before rendering. | ||
| // These libraries (combined ~200KB) never reach the client. | ||
| const rawHtml = marked.parse(comment.text || ''); | ||
| const safeHtml = sanitizeHtml(rawHtml, { |
There was a problem hiding this comment.
Security: data: URI scheme allows XSS via <img src="data:text/html,..."> in older browsers and SVG payloads
Even with sanitize-html, allowing data: in allowedSchemes for img[src] can be exploited. Restrict it:
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedSchemes: ['https', 'http'], |
If inline images are genuinely needed, scope it only to the img tag via allowedSchemesByTag rather than globally.
| }); | ||
| } | ||
|
|
||
| // Fetch comments directly from the Rails API — no client-side fetch needed |
There was a problem hiding this comment.
Security / reliability: unauthenticated internal API call with hardcoded fallback URL
The fetch to http://localhost:3000/comments.json has two issues:
- It's unauthenticated — anyone who can make the Node renderer render an arbitrary RSC payload gets a free read of all comments via SSRF escalation.
- The hardcoded port will silently fail if the Rails server runs on a different port (e.g.
PORT=3001).
Consider passing the base URL through the RSC render context rather than relying on an env var with a fragile default, and add authentication (e.g. an internal API token header).
| totalMemory: (os.totalmem() / (1024 * 1024 * 1024)).toFixed(1), | ||
| freeMemory: (os.freemem() / (1024 * 1024 * 1024)).toFixed(1), | ||
| cpus: os.cpus().length, | ||
| hostname: os.hostname(), |
There was a problem hiding this comment.
Security: os.hostname() is exposed to end users in rendered HTML
Leaking the server's hostname in a user-facing page is an information-disclosure risk — it aids reconnaissance. Consider removing or replacing with a generic label like "server" for this demo:
| hostname: os.hostname(), | |
| hostname: '[redacted]', |
| const testValue = typeof item === 'string' ? item : item?.loader; | ||
| return testValue && testValue.includes(loaderName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Code duplication: extractLoader is defined identically in both serverWebpackConfig.js and rscWebpackConfig.js
The function was already extracted and exported from serverWebpackConfig.js (module.exports = { default: configureServer, extractLoader }). Import it instead:
| } | |
| const { extractLoader } = require('./serverWebpackConfig'); |
And remove the local definition (lines 22–28).
| marked.use(gfmHeadingId()); | ||
|
|
||
| async function CommentsFeed() { | ||
| // Simulate network latency to demonstrate Suspense streaming (development only) |
There was a problem hiding this comment.
Performance: artificial delay ships to production
The await new Promise(resolve => setTimeout(resolve, 800)) guard checks NODE_ENV !== 'production', but staging environments typically run with NODE_ENV=production, meaning this delay is silently gone in staging but could still affect any non-production CI environment. More importantly, it's easy to forget this block exists and accidentally leave it in production-destined code. Consider removing it entirely before merging and using a dedicated test/demo helper if the streaming effect needs to be demonstrated.
| const { sources } = require('@rspack/core'); | ||
|
|
||
| // Cache for file 'use client' checks | ||
| const useClientCache = new Map(); |
There was a problem hiding this comment.
Memory leak: module-level useClientCache persists across watch-mode compilations
The cache is a module-level Map that is only cleared inside thisCompilation, but the module itself is loaded once and kept alive. In long-running watch-mode sessions, files that are deleted or have their 'use client' directive removed won't be evicted from the cache because the Map reference lives outside the compilation lifecycle. The thisCompilation hook clearing helps for the current run, but stale entries from previous builds can still accumulate if the same compiler instance is reused across restarts. Consider using a WeakMap keyed by compilation, or simply not caching — file reads at the top 200 bytes are cheap compared to a full bundle build.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1022234. Configure here.
| @@ -1,8 +1,10 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
'use client' not removed from SSR-only server component
Medium Severity
The PR discussion explicitly states that commit 0d8d75a "removed incorrect 'use client' from RouterApp.server.jsx", but the directive is still present. This file is a traditional SSR component (using StaticRouter, Provider, ReactOnRails.getStore) — not a client component in the RSC sense. Adding 'use client' incorrectly marks it as a client component for the RSC manifest, which could cause it to be treated as a client reference boundary if it ever gets pulled into the RSC bundle graph.
Reviewed by Cursor Bugbot for commit 1022234. Configure here.
|
Review: React Server Components with React on Rails Pro Good work overall - the three-bundle pipeline is well-structured and the RSC demo clearly illustrates the server/client component model. A few issues need attention before merging. Security (must fix)
Code quality
Minor
|
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
Introduces a new Node renderer and a third (RSC) bundle in the build pipeline, plus new routing/initializers; misconfiguration could break SSR/RSC rendering or CI builds.
Overview
Adds React Server Components (RSC) support by migrating from
react_on_railstoreact_on_rails_proand introducing a Node-based renderer process (with CI/dev wiring and bundle cache ignoring).Implements a three-bundle build (client + SSR server bundle + RSC bundle) with new
rscWebpackConfig, a customRspackRscPluginto generate RSC manifests under Rspack, updated webpack config selection/env flags, and resolver tweaks (aliases/fallbacks + SSR polyfill banner).Introduces an
/server-componentsdemo page (Rails route/controller/view + nav link) backed by new server/client component packs (rsc-bundle,rsc-client-components) and a sample RSC page that mixes server-only modules/data fetching with a small client-hydratedTogglePanel.Reviewed by Cursor Bugbot for commit 1022234. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Chores