Fix SSR runtime failures for React Server Components#723
Fix SSR runtime failures for React Server Components#723
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>
- 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>
- 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>
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>
- Add react-on-rails-pro-node-renderer for SSR and RSC payload generation - Configure Pro initializer with NodeRenderer, renderer_url, password - Alias react-dom/server.browser to react-dom/server.node in server webpack config (React 19's browser build requires MessageChannel which isn't available in the Node renderer VM) - Add auto_load_bundle: false to RSC view (server components use registerServerComponent, not ror_components auto-loading) - Add node-renderer to Procfile.dev Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace hardcoded localhost URL with RAILS_INTERNAL_URL env variable - Add response.ok check to prevent silent fetch failures - Guard 800ms demo delay to non-production environments - Restrict sanitize-html img tag to explicit allowed attributes/schemes - Clear useClientCache on each compilation for correct watch mode - Remove incorrect 'use client' from server-only files - Fix import/order lint violation in rsc-client-components - Gate trace option to development environment only - Remove duplicate RspackRscPlugin from server config (RSC-only) - Fix url-loader/file-loader guard to use .includes() matching - Pass RSC config to envSpecific callback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add .node-renderer-bundles/ to .gitignore (Node renderer cache) - CommentsFeed: skip artificial delay in production, use configurable base URL, add error handling for fetch, tighten sanitize-html config - RspackRscPlugin: clear cache on each compilation for watch mode - View: use trace only in development - rscWebpackConfig: safer file-loader option merge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
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>
…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>
|
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 support: RSC webpack config and Rspack plugin, a Node renderer with CI/start step, Rails initializer/routes/view/controller for an RSC demo, migration to react-on-rails-pro, new server/client RSC components, packs/webpack changes, and development tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Browser
participant Rails as Rails Server
participant NodeRend as Node Renderer (port 3800)
participant API as Rails API (/comments.json)
Client->>Rails: GET /server-components
activate Rails
Rails->>NodeRend: Request RSC render (auth + payload)
activate NodeRend
NodeRend->>API: Fetch /comments.json
activate API
API-->>NodeRend: Return comments JSON
deactivate API
NodeRend->>NodeRend: Render ServerInfo & CommentsFeed (RSC)
NodeRend->>NodeRend: Serialize RSC stream
NodeRend-->>Rails: Streamed RSC payload
deactivate NodeRend
Rails->>Client: HTML + RSC payload + client pack tags
deactivate Rails
Client->>Client: Load client packs, hydrate TogglePanel, render streamed content
sequenceDiagram
participant Bundler as Webpack/Rspack
participant RscPlugin as RspackRscPlugin
participant FS as File System
Bundler->>FS: Read entries (client, server, rsc)
Bundler->>RscPlugin: Invoke during asset processing
activate RscPlugin
RscPlugin->>FS: Read module starts to detect "'use client'"
FS-->>RscPlugin: File contents
RscPlugin->>Bundler: Emit react-client-manifest.json / react-server-client-manifest.json
deactivate RscPlugin
Bundler->>FS: Output bundles/artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes 37/38 rspec failures on the RSC branch caused by SSR runtime errors after switching from ExecJS to the react-on-rails-pro NodeRenderer. It makes four targeted changes: starting the NodeRenderer in CI with a TCP readiness check, replacing Node-builtin Key changes:
Confidence Score: 3/5Mostly safe to merge but contains a P1 security concern in CommentsFeed.jsx that should be fixed before the RSC demo page is publicly accessible The CI infrastructure fix and four SSR fixes are well-reasoned and the 38/38 passing test result is reassuring. The incomplete MessageChannel polyfill is low risk since tests pass. However, the global data: URI scheme in sanitize-html's allowedSchemes creates an XSS vector in user-generated markdown content rendered by CommentsFeed — sanitize-html intentionally excludes data: from its defaults for this reason, and overriding that globally is a genuine security regression. client/app/bundles/server-components/components/CommentsFeed.jsx (data: URI XSS risk), config/webpack/serverWebpackConfig.js (incomplete MessageChannel polyfill) Important Files Changed
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions CI
participant NR as Node Renderer (port 3800)
participant Rails as Rails App
participant RSpec as RSpec Tests
CI->>NR: node react-on-rails-pro-node-renderer.js &
loop TCP readiness check (up to 30s)
CI->>NR: nc -z localhost 3800
NR-->>CI: connection accepted
end
CI->>RSpec: bundle exec rake ci:rspec
RSpec->>Rails: GET /server-components (RSC page)
Rails->>NR: HTTP POST /render (rsc-bundle.js)
NR->>NR: VM runs RSC bundle
NR-->>Rails: React Flight payload
Rails-->>RSpec: Rendered HTML
RSpec->>Rails: GET /comments (traditional SSR)
Rails->>NR: HTTP POST /render (server-bundle.js)
NR->>NR: VM runs server bundle
Note over NR: MessageChannel polyfill active
NR-->>Rails: Rendered HTML
Rails-->>RSpec: HTML response
Reviews (1): Last reviewed commit: "Fix SSR runtime failures: Node renderer,..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 1022234fa8
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
client/app/bundles/server-components/components/CommentsFeed.jsx (1)
59-59: Consider restrictingdata:scheme to specific MIME types.The
datascheme allows any content type. For tighter security, consider restricting to image MIME types only.🛡️ Suggested improvement
- allowedSchemes: ['https', 'http', 'data'], + allowedSchemes: ['https', 'http'], + allowedSchemesByTag: { + img: ['https', 'http', 'data'], + },Alternatively, if
data:URIs for images are not required, remove it entirely:- allowedSchemes: ['https', 'http', 'data'], + allowedSchemes: ['https', 'http'],🤖 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 59, The allowedSchemes configuration currently includes a blanket 'data' entry which permits any data: MIME type; update the configuration in CommentsFeed.jsx where allowedSchemes is defined to either remove 'data' entirely or restrict it to image MIME types only (e.g., allow only data:image/* by replacing 'data' with a constrained pattern or validated check such as permitting only data URLs that match /^data:image\/(png|jpeg|gif|webp);base64,/), ensuring the sanitizer/validator for allowedSchemes enforces that constraint.app/views/pages/server_components.html.erb (1)
1-6: Consider removing the hardcoded DOMid.The
id: "ServerComponentsPage-react-component-0"appears to be hardcoded. React on Rails typically generates unique IDs automatically. Hardcoding this could cause issues if the component is rendered multiple times on a page or if the ID generation logic changes.Unless there's a specific requirement for this exact ID (e.g., RSC payload targeting), consider omitting it to let the framework handle ID generation.
Suggested change
<%= append_javascript_pack_tag('rsc-client-components') %> <%= react_component("ServerComponentsPage", prerender: false, auto_load_bundle: false, - trace: Rails.env.development?, - id: "ServerComponentsPage-react-component-0") %> + trace: Rails.env.development?) %>🤖 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 1 - 6, The react_component invocation for ServerComponentsPage includes a hardcoded id ("ServerComponentsPage-react-component-0") which can cause collisions; remove the id option from the react_component call in the ServerComponentsPage render so React on Rails can auto-generate unique IDs (or, if a stable ID is truly required for RSC payload targeting, replace the hardcoded string with a generated unique value/prop such as a SecureRandom.uuid or a passed-in server-side identifier before rendering).client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx (1)
1-2: Intentional but confusing:'use client'in a.server.jsxfile.Per the PR objectives, this directive is intentionally added to ensure the component is registered via
ReactOnRails.register()(traditional SSR) rather thanregisterServerComponent()(RSC). This is necessary because the component uses client-only APIs likeReactOnRails.getStore().However, the filename
RouterApp.server.jsxwith a'use client'directive creates cognitive dissonance. Consider adding a comment explaining this apparent contradiction, or renaming the file to something likeRouterApp.ssr.jsxto clarify it's for traditional SSR (not RSC server components).Suggested clarifying comment
'use client'; +// NOTE: Despite the .server.jsx filename (which indicates traditional SSR with StaticRouter), +// this component needs 'use client' to prevent RSC bundler from treating it as a React Server +// Component. It uses client APIs (ReactOnRails.getStore) and is for hydration-based SSR. // Compare to ./RouterApp.client.jsx🤖 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` around lines 1 - 2, The 'use client' directive in RouterApp.server.jsx is intentionally used so the component registers with ReactOnRails via ReactOnRails.register() and can call client-only APIs like ReactOnRails.getStore(), but the .server.jsx filename is misleading; either add a clear top-of-file comment explaining this contradiction (mentioning that 'use client' is required to force traditional SSR registration with ReactOnRails.register() and to allow ReactOnRails.getStore()), or rename the file to RouterApp.ssr.jsx (and update any imports/registrations) to make its SSR intent explicit..github/workflows/rspec_test.yml (1)
86-98: Add output logging to the Node renderer process for better debugging.The Node renderer process started in background will persist across steps, but without capturing its output, it's difficult to diagnose failures if the renderer crashes between the health check and the rspec test execution. Consider using
nohupwith output redirection to preserve logs for troubleshooting:Suggested improvement
- name: Start Node renderer for SSR run: | - node react-on-rails-pro-node-renderer.js & + nohup node react-on-rails-pro-node-renderer.js > node-renderer.log 2>&1 & echo "Waiting for Node renderer on port 3800..." for i in $(seq 1 30); do if nc -z localhost 3800 2>/dev/null; then echo "Node renderer is ready" exit 0 fi sleep 1 done echo "Node renderer failed to start within 30 seconds" + echo "--- Node renderer logs ---" + cat node-renderer.log || true exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rspec_test.yml around lines 86 - 98, The Node renderer background start (react-on-rails-pro-node-renderer.js) in the "Start Node renderer for SSR" step currently discards stdout/stderr; modify the step to run the renderer with nohup (or equivalent) and redirect both stdout and stderr to a log file (e.g., nohup node react-on-rails-pro-node-renderer.js > node-renderer.log 2>&1 &) so the process output is preserved for debugging while still performing the existing health check on port 3800; ensure the log file is accessible in later steps (or optionally tail the log) so failures between the readiness check and rspec run can be diagnosed.client/app/packs/rsc-bundle.js (1)
6-7: This side-effect import does not register the stores in the RSC render path.
client/app/packs/stores-registration.jsis itself a'use client'module that only runsReactOnRails.registerStore(...)at module evaluation time. Imported from this server entry, that side effect is not part of the server render path, so the comment here overstates what is actually available. If the RSC page needs those stores, load them from a pack that is guaranteed to execute in the browser; otherwise remove this import/comment to avoid a misleading invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/packs/rsc-bundle.js` around lines 6 - 7, The import of './stores-registration' and its comment are misleading because that module is a 'use client' file which runs ReactOnRails.registerStore(...) only in the browser, so it is not executed in the RSC/server render path; either remove the import and the comment from rsc-bundle.js to avoid the incorrect invariant, or move the store registration into a client-only pack that is guaranteed to run in the browser (e.g., create or update a browser entry that imports './stores-registration' and ensure that pack is included on client pages); refer to the './stores-registration' module and the ReactOnRails.registerStore calls when making the change.config/webpack/rscWebpackConfig.js (2)
16-22: Unused helper function.
extractLoaderis defined but only used withinconfigureRsc. Consider inlining it or confirming it's needed. The function itself is clean and correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rscWebpackConfig.js` around lines 16 - 22, The helper function extractLoader is defined but only referenced inside configureRsc; remove the standalone extractLoader declaration and inline its logic directly into configureRsc (or, if you prefer to keep it, ensure it's actually reused elsewhere) by replacing calls to extractLoader(...) with the equivalent Array.isArray(rule.use) check and find logic that inspects item or item.loader for loaderName; reference the extractLoader name and configureRsc to locate where to perform the change.
47-71: Rule mutation is necessary but consider defensive coding.The
no-param-reassignlint errors are expected when modifying webpack rules. The mutations are intentional and safe sincecommonWebpackConfig()returns a fresh config each time. However, there's a potential edge case:
- Lines 65-70: The
else ifonly checksrule.use?.loaderbut some rules may haverule.loaderdirectly without ausewrapper.Consider handling direct `rule.loader` case
- } 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 }; + } 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 }; + } else if (rule.loader && (rule.loader.includes('url-loader') || rule.loader.includes('file-loader'))) { + rule.options = { ...(rule.options || {}), emitFile: 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 47 - 71, The loop in rscConfig.module.rules.forEach mutates rule.use options for url/file loaders but misses rules that define loader directly on the rule (rule.loader), so update the handler to also detect and handle a direct rule.loader case: when rule.loader (not rule.use) is a string containing 'url-loader' or 'file-loader', set/merge rule.options = { ...(rule.options || {}), emitFile: false } with type-safe checks (ensure rule.loader is a string and rule.options exists or is created) so direct-loader rules receive the same emitFile:false behavior as use-wrapped loaders.config/webpack/rspackRscPlugin.js (2)
30-32: Silent error suppression hides potential issues.The empty catch block silently swallows all file read errors. Consider logging in debug mode or at least differentiating between "file not found" (expected for generated files) and other errors.
Consider logging unexpected errors
} catch (e) { - // file doesn't exist or can't be read + // File doesn't exist or can't be read - expected for generated/virtual modules + if (e.code !== 'ENOENT' && e.code !== 'EACCES') { + // Unexpected error - log for debugging + console.warn(`RspackRscPlugin: Unexpected error reading ${filePath}:`, e.message); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 30 - 32, The empty catch in rspackRscPlugin.js silently swallows all read errors; update the catch in the file-read block (the try/catch surrounding the read operation in the Rsc plugin) to detect and ignore only the "file not found" case (e.g., error.code === 'ENOENT') while logging any other errors (use processLogger.debug or console.error depending on available logger, and include the error object) so unexpected IO issues are surfaced; ensure the catch references the thrown error (e) rather than using an unused placeholder (_).
12-36: Module-level cache could cause issues with parallel compilations.The
useClientCacheis shared across all plugin instances. If multiple compilations run in parallel (e.g., client and server builds), they share the same cache. While the cache is cleared at the start of each compilation (line 52-54), parallel compilations could interfere with each other.Also, synchronous file I/O in
hasUseClientDirectivecould slow builds with many files. Consider using async I/O or webpack's file system abstraction.Consider moving cache to instance level
class RspackRscPlugin { constructor(options) { if (!options || typeof options.isServer !== 'boolean') { throw new Error('RspackRscPlugin: isServer option (boolean) is required.'); } this.isServer = options.isServer; this.clientManifestFilename = options.isServer ? 'react-server-client-manifest.json' : 'react-client-manifest.json'; this.ssrManifestFilename = 'react-ssr-manifest.json'; + this._useClientCache = new Map(); }Then update
hasUseClientDirectiveto be an instance method usingthis._useClientCache.🤖 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 - 36, The module-level Map useClientCache and function hasUseClientDirective should be converted to per-plugin-instance state and methods: add this._useClientCache (a Map) on the plugin constructor and change hasUseClientDirective to an instance method (e.g., this.hasUseClientDirective) that reads/writes this._useClientCache instead of the module-scope useClientCache; also replace the synchronous fs.* reads inside hasUseClientDirective with asynchronous I/O or the compiler/filesystem abstraction provided by the build (e.g., use compiler.inputFileSystem/async reads) so the check is non-blocking and isolated per compilation, and remove any module-level useClientCache references so parallel builds do not share state.
🤖 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/CommentsFeed.jsx`:
- Around line 28-32: The code assigns the full JSON body to comments but the
Rails API returns { comments: [...] }, so _.orderBy is being passed an object;
update the assignment after response.json() to extract the array (e.g., const {
comments: commentsArray } = await response.json() or const commentsArray =
(await response.json()).comments || []) and then feed commentsArray into
_.orderBy and _.take; adjust references from comments to commentsArray (or
rename consistently) so sortedComments = _.orderBy(commentsArray,
['created_at'], ['desc']) and recentComments = _.take(sortedComments, 10),
handling a missing comments key by defaulting to an empty array.
In `@client/app/bundles/server-components/components/ServerInfo.jsx`:
- Around line 38-40: The JSX text in the ServerInfo component contains Prettier
formatting inconsistencies; run Prettier (or your project's formatter) to
reformat ServerInfo.jsx so the JSX text nodes and inline <code> elements are
properly spaced and wrapped according to the project's rules. Locate the
ServerInfo React component and reformat the block containing the sentence about
the Node.js os module and lodash usage so that spacing around inline code
elements and line breaks match the project's Prettier config, then commit the
formatted file.
In `@client/app/bundles/server-components/ServerComponentsPage.jsx`:
- Around line 10-127: ESLint flags the component definition and Prettier reports
formatting errors; replace the arrow function component "const
ServerComponentsPage = () => { ... }" with a function declaration "function
ServerComponentsPage() { ... }", keep the same JSX and component usages
(ServerInfo, TogglePanel, CommentsFeed, Suspense), then run your project's
formatter/Prettier on this file to fix the remaining formatting violations and
ensure the file exports the component (export default ServerComponentsPage) if
applicable.
In `@client/app/packs/stimulus-bundle.js`:
- Around line 24-28: The registerServerComponent call is using the wrong
signature by passing an options object; update the invocation of
registerServerComponent to pass only variadic component name strings (e.g., call
registerServerComponent with 'ServerComponentsPage' as a string) and remove the
rscPayloadGenerationUrlPath options object since that parameter is not supported
and defaults to 'rsc_payload/' internally.
In `@config/initializers/react_on_rails_pro.rb`:
- Around line 3-7: Replace the ENV["REACT_RENDERER_URL"] ||
"http://localhost:3800" pattern with ENV.fetch("REACT_RENDERER_URL",
"http://localhost:3800") inside the ReactOnRailsPro.configure block so
config.renderer_url uses ENV.fetch like config.renderer_password; update the
config.renderer_url assignment to call ENV.fetch("REACT_RENDERER_URL",
"http://localhost:3800") to ensure empty strings fall back to the default URL.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 94-100: The current emit block emits an empty SSR manifest; update
the logic around this.isServer / compilation.emitAsset so that
this.ssrManifestFilename (react-ssr-manifest.json) is written with the actual
module metadata object containing moduleMap, serverModuleMap, and moduleLoading
(populated from the plugin's collected module registry or the same source that
builds the client manifest) instead of {}. Locate where the plugin builds the
client manifest and mirror that collection step for server modules (or extend
that collection), then serialize the assembled metadata { moduleMap,
serverModuleMap, moduleLoading } into JSON and pass it to compilation.emitAsset
using new sources.RawSource(JSON.stringify(..., null, 2)). Ensure the names
this.ssrManifestFilename and any registry variables used for module ids are
reused so the client and SSR manifests stay consistent.
In `@config/webpack/webpackConfig.js`:
- Around line 30-36: The envSpecific callback is invoked with three configs
(clientConfig, serverConfig, rscConfig) but the environment-specific functions
only accept two params, so the RSC config is ignored; update the signatures in
the environment modules (e.g., change productionEnvOnly and testOnly to accept a
third parameter such as _rscWebpackConfig) so they match the call site
(envSpecific(clientWebpackConfig(), serverWebpackConfig(), rscWebpackConfig()))
and can modify the RSC config when needed.
In `@react-on-rails-pro-node-renderer.js`:
- Around line 4-10: The config object uses process.env values inconsistently:
port can remain a string while workersCount is coerced with Number(), which can
produce NaN; normalize both by parsing and validating their numeric environment
variables before assigning to config—convert process.env.RENDERER_PORT and
process.env.NODE_RENDERER_CONCURRENCY to integers (e.g., parseInt or Number with
fallback), ensure sensible defaults if parsing fails, and assign the
parsed/validated numbers to config.port and config.workersCount (refer to the
config object and the port and workersCount properties) so the configuration is
always numeric and predictable.
---
Nitpick comments:
In @.github/workflows/rspec_test.yml:
- Around line 86-98: The Node renderer background start
(react-on-rails-pro-node-renderer.js) in the "Start Node renderer for SSR" step
currently discards stdout/stderr; modify the step to run the renderer with nohup
(or equivalent) and redirect both stdout and stderr to a log file (e.g., nohup
node react-on-rails-pro-node-renderer.js > node-renderer.log 2>&1 &) so the
process output is preserved for debugging while still performing the existing
health check on port 3800; ensure the log file is accessible in later steps (or
optionally tail the log) so failures between the readiness check and rspec run
can be diagnosed.
In `@app/views/pages/server_components.html.erb`:
- Around line 1-6: The react_component invocation for ServerComponentsPage
includes a hardcoded id ("ServerComponentsPage-react-component-0") which can
cause collisions; remove the id option from the react_component call in the
ServerComponentsPage render so React on Rails can auto-generate unique IDs (or,
if a stable ID is truly required for RSC payload targeting, replace the
hardcoded string with a generated unique value/prop such as a SecureRandom.uuid
or a passed-in server-side identifier before rendering).
In
`@client/app/bundles/comments/startup/RouterApp/ror_components/RouterApp.server.jsx`:
- Around line 1-2: The 'use client' directive in RouterApp.server.jsx is
intentionally used so the component registers with ReactOnRails via
ReactOnRails.register() and can call client-only APIs like
ReactOnRails.getStore(), but the .server.jsx filename is misleading; either add
a clear top-of-file comment explaining this contradiction (mentioning that 'use
client' is required to force traditional SSR registration with
ReactOnRails.register() and to allow ReactOnRails.getStore()), or rename the
file to RouterApp.ssr.jsx (and update any imports/registrations) to make its SSR
intent explicit.
In `@client/app/bundles/server-components/components/CommentsFeed.jsx`:
- Line 59: The allowedSchemes configuration currently includes a blanket 'data'
entry which permits any data: MIME type; update the configuration in
CommentsFeed.jsx where allowedSchemes is defined to either remove 'data'
entirely or restrict it to image MIME types only (e.g., allow only data:image/*
by replacing 'data' with a constrained pattern or validated check such as
permitting only data URLs that match
/^data:image\/(png|jpeg|gif|webp);base64,/), ensuring the sanitizer/validator
for allowedSchemes enforces that constraint.
In `@client/app/packs/rsc-bundle.js`:
- Around line 6-7: The import of './stores-registration' and its comment are
misleading because that module is a 'use client' file which runs
ReactOnRails.registerStore(...) only in the browser, so it is not executed in
the RSC/server render path; either remove the import and the comment from
rsc-bundle.js to avoid the incorrect invariant, or move the store registration
into a client-only pack that is guaranteed to run in the browser (e.g., create
or update a browser entry that imports './stores-registration' and ensure that
pack is included on client pages); refer to the './stores-registration' module
and the ReactOnRails.registerStore calls when making the change.
In `@config/webpack/rscWebpackConfig.js`:
- Around line 16-22: The helper function extractLoader is defined but only
referenced inside configureRsc; remove the standalone extractLoader declaration
and inline its logic directly into configureRsc (or, if you prefer to keep it,
ensure it's actually reused elsewhere) by replacing calls to extractLoader(...)
with the equivalent Array.isArray(rule.use) check and find logic that inspects
item or item.loader for loaderName; reference the extractLoader name and
configureRsc to locate where to perform the change.
- Around line 47-71: The loop in rscConfig.module.rules.forEach mutates rule.use
options for url/file loaders but misses rules that define loader directly on the
rule (rule.loader), so update the handler to also detect and handle a direct
rule.loader case: when rule.loader (not rule.use) is a string containing
'url-loader' or 'file-loader', set/merge rule.options = { ...(rule.options ||
{}), emitFile: false } with type-safe checks (ensure rule.loader is a string and
rule.options exists or is created) so direct-loader rules receive the same
emitFile:false behavior as use-wrapped loaders.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 30-32: The empty catch in rspackRscPlugin.js silently swallows all
read errors; update the catch in the file-read block (the try/catch surrounding
the read operation in the Rsc plugin) to detect and ignore only the "file not
found" case (e.g., error.code === 'ENOENT') while logging any other errors (use
processLogger.debug or console.error depending on available logger, and include
the error object) so unexpected IO issues are surfaced; ensure the catch
references the thrown error (e) rather than using an unused placeholder (_).
- Around line 12-36: The module-level Map useClientCache and function
hasUseClientDirective should be converted to per-plugin-instance state and
methods: add this._useClientCache (a Map) on the plugin constructor and change
hasUseClientDirective to an instance method (e.g., this.hasUseClientDirective)
that reads/writes this._useClientCache instead of the module-scope
useClientCache; also replace the synchronous fs.* reads inside
hasUseClientDirective with asynchronous I/O or the compiler/filesystem
abstraction provided by the build (e.g., use compiler.inputFileSystem/async
reads) so the check is non-blocking and isolated per compilation, and remove any
module-level useClientCache references so parallel builds do not share state.
🪄 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: 4082de09-59c2-4c40-9a11-48ccbf0cb6ea
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
.github/workflows/rspec_test.yml.gitignoreGemfileProcfile.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.jsonreact-on-rails-pro-node-renderer.js
client/app/bundles/server-components/components/CommentsFeed.jsx
Outdated
Show resolved
Hide resolved
| 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. |
There was a problem hiding this comment.
Fix Prettier formatting issues.
Static analysis flagged formatting inconsistencies in the JSX text content. Run Prettier to auto-fix.
🔧 Suggested 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>📝 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.
| 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 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> |
🧰 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)
🤖 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
38 - 40, The JSX text in the ServerInfo component contains Prettier formatting
inconsistencies; run Prettier (or your project's formatter) to reformat
ServerInfo.jsx so the JSX text nodes and inline <code> elements are properly
spaced and wrapped according to the project's rules. Locate the ServerInfo React
component and reformat the block containing the sentence about the Node.js os
module and lodash usage so that spacing around inline code elements and line
breaks match the project's Prettier config, then commit the formatted file.
| const ServerComponentsPage = () => { | ||
| return ( | ||
| <div className="max-w-4xl mx-auto py-8 px-4"> | ||
| <header className="mb-10"> | ||
| <h1 className="text-3xl font-bold text-slate-800 mb-2"> | ||
| React Server Components Demo | ||
| </h1> | ||
| <p className="text-slate-600 text-lg"> | ||
| This page is rendered using <strong>React Server Components</strong> with React on Rails Pro. | ||
| Server components run on the server and stream their output to the client, keeping | ||
| heavy dependencies out of the browser bundle entirely. | ||
| </p> | ||
| </header> | ||
|
|
||
| <div className="space-y-8"> | ||
| {/* Server Info - uses Node.js os module (impossible on client) */} | ||
| <section> | ||
| <h2 className="text-xl font-semibold text-slate-700 mb-4 flex items-center gap-2"> | ||
| Server Environment | ||
| <span className="text-xs font-normal bg-emerald-100 text-emerald-700 px-2 py-0.5 rounded-full"> | ||
| Server Only | ||
| </span> | ||
| </h2> | ||
| <ServerInfo /> | ||
| </section> | ||
|
|
||
| {/* Interactive toggle - demonstrates mixing server + client components */} | ||
| <section> | ||
| <h2 className="text-xl font-semibold text-slate-700 mb-4 flex items-center gap-2"> | ||
| Interactive Client Component | ||
| <span className="text-xs font-normal bg-blue-100 text-blue-700 px-2 py-0.5 rounded-full"> | ||
| Client Hydrated | ||
| </span> | ||
| </h2> | ||
| <TogglePanel title="How does this work?"> | ||
| <div className="prose prose-slate max-w-none text-sm"> | ||
| <p> | ||
| This toggle is a <code>'use client'</code> component, meaning it ships JavaScript | ||
| to the browser for interactivity. But the content inside is rendered on the server | ||
| and passed as children — a key RSC pattern called the <strong>donut pattern</strong>. | ||
| </p> | ||
| <ul> | ||
| <li>The <code>TogglePanel</code> wrapper runs on the client (handles click events)</li> | ||
| <li>The children content is rendered on the server (no JS cost)</li> | ||
| <li>Heavy libraries used by server components never reach the browser</li> | ||
| </ul> | ||
| </div> | ||
| </TogglePanel> | ||
| </section> | ||
|
|
||
| {/* Async data fetching with Suspense streaming */} | ||
| <section> | ||
| <h2 className="text-xl font-semibold text-slate-700 mb-4 flex items-center gap-2"> | ||
| Streamed Comments | ||
| <span className="text-xs font-normal bg-amber-100 text-amber-700 px-2 py-0.5 rounded-full"> | ||
| Async + Suspense | ||
| </span> | ||
| </h2> | ||
| <p className="text-slate-500 text-sm mb-4"> | ||
| Comments are fetched directly on the server using the Rails API. | ||
| The page shell renders immediately while this section streams in progressively. | ||
| </p> | ||
| <Suspense | ||
| fallback={ | ||
| <div className="animate-pulse space-y-3"> | ||
| {[1, 2, 3].map((i) => ( | ||
| <div key={i} className="bg-slate-100 rounded-lg p-4"> | ||
| <div className="h-4 bg-slate-200 rounded w-1/4 mb-2" /> | ||
| <div className="h-3 bg-slate-200 rounded w-3/4" /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| } | ||
| > | ||
| <CommentsFeed /> | ||
| </Suspense> | ||
| </section> | ||
|
|
||
| {/* Architecture explanation */} | ||
| <section className="bg-slate-50 border border-slate-200 rounded-xl p-6"> | ||
| <h2 className="text-lg font-semibold text-slate-700 mb-3"> | ||
| What makes this different? | ||
| </h2> | ||
| <div className="grid md:grid-cols-2 gap-4 text-sm text-slate-600"> | ||
| <div> | ||
| <h3 className="font-medium text-slate-800 mb-1">Smaller Client Bundle</h3> | ||
| <p> | ||
| Libraries like <code>lodash</code>, <code>marked</code>, and Node.js <code>os</code> module | ||
| are used on this page but never downloaded by the browser. | ||
| </p> | ||
| </div> | ||
| <div> | ||
| <h3 className="font-medium text-slate-800 mb-1">Direct Data Access</h3> | ||
| <p> | ||
| Server components fetch data by calling your Rails API internally — no | ||
| client-side fetch waterfalls or loading spinners for initial data. | ||
| </p> | ||
| </div> | ||
| <div> | ||
| <h3 className="font-medium text-slate-800 mb-1">Progressive Streaming</h3> | ||
| <p> | ||
| The page shell renders instantly. Async components (like the comments feed) | ||
| stream in as their data resolves, with Suspense boundaries showing fallbacks. | ||
| </p> | ||
| </div> | ||
| <div> | ||
| <h3 className="font-medium text-slate-800 mb-1">Selective Hydration</h3> | ||
| <p> | ||
| Only client components (like the toggle above) receive JavaScript. | ||
| Everything else is pure HTML — zero hydration cost. | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </section> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Resolve the current ESLint/Prettier errors before merging.
Static analysis is already flagging react/function-component-definition on Line 10, and the JSX block still has multiple prettier/prettier violations. This page is not lint-clean yet, so please rewrite the component as a function declaration and run the formatter on the file.
🧰 Tools
🪛 ESLint
[error] 10-127: Function component is not a function declaration
(react/function-component-definition)
[error] 14-16: Replace ⏎··········React·Server·Components·Demo⏎········ with React·Server·Components·Demo
(prettier/prettier)
[error] 18-20: Replace ⏎··········Server·components·run·on·the·server·and·stream·their·output·to·the·client,·keeping⏎··········heavy·dependencies·out with ·Server⏎··········components·run·on·the·server·and·stream·their·output·to·the·client,·keeping·heavy·dependencies·out⏎·········
(prettier/prettier)
[error] 47-47: Insert ·to
(prettier/prettier)
[error] 48-49: Replace ·to·the·browser·for·interactivity.·But·the·content·inside·is·rendered·on·the·server⏎················and·passed·as with ·the·browser·for·interactivity.·But·the·content·inside·is·rendered·on·the·server·and·passed·as⏎···············
(prettier/prettier)
[error] 52-52: Replace The·<code>TogglePanel</code>·wrapper·runs·on·the·client·(handles·click·events) with ⏎··················The·<code>TogglePanel</code>·wrapper·runs·on·the·client·(handles·click·events)⏎················
(prettier/prettier)
[error] 69-70: Replace ⏎············The·page·shell·renders with ·The·page·shell·renders⏎···········
(prettier/prettier)
[error] 90-92: Replace ⏎············What·makes·this·different?⏎·········· with What·makes·this·different?
(prettier/prettier)
[error] 104-105: Replace ⏎················client-side·fetch with ·client-side·fetch⏎···············
(prettier/prettier)
[error] 111-112: Replace ⏎················stream·in·as·their with ·stream·in·as·their⏎···············
(prettier/prettier)
[error] 118-119: Replace ⏎················Everything·else·is·pure with ·Everything·else·is·pure⏎···············
(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/ServerComponentsPage.jsx` around lines
10 - 127, ESLint flags the component definition and Prettier reports formatting
errors; replace the arrow function component "const ServerComponentsPage = () =>
{ ... }" with a function declaration "function ServerComponentsPage() { ... }",
keep the same JSX and component usages (ServerInfo, TogglePanel, CommentsFeed,
Suspense), then run your project's formatter/Prettier on this file to fix the
remaining formatting violations and ensure the file exports the component
(export default ServerComponentsPage) if applicable.
| // Emit SSR manifest (maps module IDs to SSR module data) | ||
| if (!this.isServer) { | ||
| compilation.emitAsset( | ||
| this.ssrManifestFilename, | ||
| new sources.RawSource(JSON.stringify({}, null, 2)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What should react-ssr-manifest.json contain for React Server Components streaming SSR?
💡 Result:
react-ssr-manifest.json is a Vite-generated file used in SSR setups to map server-side module IDs to their corresponding client-side chunk files (JS/CSS/assets). This enables the server to preload the correct client assets during streaming SSR with Suspense and React.lazy, avoiding waterfalls. It contains an object where keys are normalized source module paths (e.g., "src/pages/create/create.jsx") and values are arrays of asset URLs (absolute or relative based on publicPath): { "src/pages/create/create.jsx": [ "/assets/create-Cgw3MXE6.js", "/assets/create-CgBOejMp.css" ], "src/pages/create/create.module.scss": [ "/assets/create-Cgw3MXE6.js", "/assets/create-CgBOejMp.css" ], ... } For React Server Components (RSC) streaming SSR specifically: - Standard Vite ssr-manifest.json (via build.ssrManifest: true) supports traditional SSR but lacks full context.modules for dynamic imports (Vue has it via @vitejs/plugin-vue; React needs custom plugins like vite-preload). - RSC requires additional manifests: client-manifest.json (maps client "use client" components to chunks) and server-consumer-manifest.json (formerly SSRManifest; module loading config + moduleMap for server-to-client references). These are generated by react-server-dom-webpack plugin. - In Vite RSC setups (@vitejs/plugin-rsc), use virtual:vite-rsc/assets-manifest and buildAssetsManifest for client assets in SSR. Example server usage (Express): const ssrManifest = JSON.parse(fs.readFileSync('.vite/ssr-manifest.json', 'utf-8')); To support React.lazy preloading in RSC streaming, process vite's manifest.json (with dynamicImports) or use plugins like vite-preload. Note: react-ssr-manifest.json is distinct from RSC-specific manifests; for full RSC streaming, combine with client/server manifests passed to renderToReadableStream(, clientManifest).
Citations:
- 1: https://stackoverflow.com/questions/78841536/how-to-get-context-modules-for-vite-react-ssr-manifest-json
- 2: How to get context.modules for vite react ssr-manifest.json vitejs/vite-plugin-react#356
- 3: https://tessl.io/registry/tessl/npm-react-server-dom-webpack/19.1.0/files/docs/webpack-plugin.md
- 4: https://tessl.io/registry/tessl/npm-react-server-dom-webpack/19.1.0/files/docs/server-apis.md
- 5: https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-rsc/docs/architecture.md
- 6: https://registry.npmjs.org/%40vitejs%2Fplugin-rsc
- 7: https://github.com/Daydreamer-riri/vite-react-ssg/blob/main/src/node/assets.ts
- 8: https://github.com/vitejs/vite/blob/130ef31b/packages/vite/src/node/ssr/ssrManifestPlugin.ts
🏁 Script executed:
# First, let's look at the full context of the rspackRscPlugin.js file
fd -e js -e ts -name '*rspack*' -o -name '*RSC*' -o -name '*rsc*' | head -20Repository: shakacode/react-webpack-rails-tutorial
Length of output: 312
🏁 Script executed:
# Find and examine the rspackRscPlugin.js file
find . -type f -name "rspackRscPlugin.js" -o -name "*rspackRsc*"Repository: shakacode/react-webpack-rails-tutorial
Length of output: 117
🏁 Script executed:
# Read the rspackRscPlugin.js file
cat -n ./config/webpack/rspackRscPlugin.jsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 5899
🏁 Script executed:
# Search for usage of react-ssr-manifest.json in the codebase
rg "react-ssr-manifest" --type js --type ts --type jsonRepository: shakacode/react-webpack-rails-tutorial
Length of output: 173
🏁 Script executed:
# Search for references to ssrManifestFilename
rg "ssrManifestFilename" --type js --type tsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 247
🏁 Script executed:
# Check for any SSR or manifest-related configuration
rg "ssr-manifest|ssrManifest" --type js --type ts --type json -iRepository: shakacode/react-webpack-rails-tutorial
Length of output: 247
🏁 Script executed:
# Check for any comments or documentation about the plugin
grep -r "SSR manifest\|ssr.*manifest" --include="*.md" --include="*.txt" --include="*.js"Repository: shakacode/react-webpack-rails-tutorial
Length of output: 591
🏁 Script executed:
# Look for any server-side build config or SSR setup
find . -type f \( -name "README*" -o -name "*SSR*" -o -name "*config*" \) | grep -E "(README|SSR|config)" | head -20Repository: shakacode/react-webpack-rails-tutorial
Length of output: 759
🏁 Script executed:
# Check if there are any references to building or using the manifests
rg "react-client-manifest|react-server-client-manifest" --type js --type tsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 455
🏁 Script executed:
# Look at the react-server-dom-webpack-plugin to understand expected format
cat -n node_modules/react-on-rails-rsc/dist/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js | head -100Repository: shakacode/react-webpack-rails-tutorial
Length of output: 4532
🏁 Script executed:
# Check the react-on-rails-pro file that creates SSR manifest
cat -n node_modules/react-on-rails-pro/lib/getReactServerComponent.server.js 2>/dev/null | head -50Repository: shakacode/react-webpack-rails-tutorial
Length of output: 2775
🏁 Script executed:
# Search for any documentation or examples about manifest structure in node_modules
rg "serverConsumerManifest|react-ssr-manifest" node_modules/react-on-rails-rsc node_modules/react-on-rails-pro --type js -A 5 | head -60Repository: shakacode/react-webpack-rails-tutorial
Length of output: 11492
🏁 Script executed:
# Search for what should populate the server manifest in the webpack plugin
rg "moduleMap|serverModuleMap|moduleLoading" node_modules/react-on-rails-rsc --type js -B 2 -A 2 | head -100Repository: shakacode/react-webpack-rails-tutorial
Length of output: 15978
🏁 Script executed:
# Check if there are any server build configs that might populate this
rg "serverConsumerManifest|react-ssr-manifest" config/ --type js -B 3 -A 3Repository: shakacode/react-webpack-rails-tutorial
Length of output: 535
Populate react-ssr-manifest.json with module metadata for RSC hydration.
The SSR manifest is emitted as an empty object, but React's flight protocol requires it to contain module metadata (moduleMap, serverModuleMap, moduleLoading). Without this data, buildClientRenderer() and createFromNodeStream() cannot resolve server module references during streaming SSR. The plugin's own comment indicates the intent: "maps module IDs to SSR module data", but the implementation leaves it unpopulated. Consider populating this manifest with module metadata alongside the client manifest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/rspackRscPlugin.js` around lines 94 - 100, The current emit
block emits an empty SSR manifest; update the logic around this.isServer /
compilation.emitAsset so that this.ssrManifestFilename (react-ssr-manifest.json)
is written with the actual module metadata object containing moduleMap,
serverModuleMap, and moduleLoading (populated from the plugin's collected module
registry or the same source that builds the client manifest) instead of {}.
Locate where the plugin builds the client manifest and mirror that collection
step for server modules (or extend that collection), then serialize the
assembled metadata { moduleMap, serverModuleMap, moduleLoading } into JSON and
pass it to compilation.emitAsset using new sources.RawSource(JSON.stringify(...,
null, 2)). Ensure the names this.ssrManifestFilename and any registry variables
used for module ids are reused so the client and SSR manifests stay consistent.
| const clientConfig = clientWebpackConfig(); | ||
| const serverConfig = serverWebpackConfig(); | ||
| const rscConfig = rscWebpackConfig(); | ||
| if (envSpecific) envSpecific(clientConfig, serverConfig, rscConfig); | ||
| // eslint-disable-next-line no-console | ||
| console.log('[React on Rails] Creating both client and server bundles.'); | ||
| result = [clientConfig, serverConfig]; | ||
| console.log('[React on Rails] Creating client, server, and RSC bundles.'); | ||
| result = [clientConfig, serverConfig, rscConfig]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all files that call webpackConfig() with a callback to verify they handle the new signature
rg -n "webpackConfig\(" config/webpack/ --type js -A 3Repository: shakacode/react-webpack-rails-tutorial
Length of output: 304
🏁 Script executed:
cat -n config/webpack/production.jsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 660
🏁 Script executed:
cat -n config/webpack/test.jsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 341
🏁 Script executed:
cat -n config/webpack/webpackConfig.js | head -50Repository: shakacode/react-webpack-rails-tutorial
Length of output: 2323
Update environment-specific callbacks to accept the rscConfig parameter.
The envSpecific callbacks are called with different numbers of arguments depending on the build mode. In the default case (lines 30-36), they receive 3 arguments (clientConfig, serverConfig, rscConfig), but the callbacks in production.js and test.js only accept 2 parameters. JavaScript silently ignores the extra argument, preventing environment-specific RSC configuration.
Update the callback signatures in environment files to accept the third parameter:
// config/webpack/production.js
const productionEnvOnly = (_clientWebpackConfig, _serverWebpackConfig, _rscWebpackConfig) => {
// place any code here that is for production only
};
// config/webpack/test.js
const testOnly = (_clientWebpackConfig, _serverWebpackConfig, _rscWebpackConfig) => {
// place any code here that is for test only
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/webpackConfig.js` around lines 30 - 36, The envSpecific
callback is invoked with three configs (clientConfig, serverConfig, rscConfig)
but the environment-specific functions only accept two params, so the RSC config
is ignored; update the signatures in the environment modules (e.g., change
productionEnvOnly and testOnly to accept a third parameter such as
_rscWebpackConfig) so they match the call site
(envSpecific(clientWebpackConfig(), serverWebpackConfig(), rscWebpackConfig()))
and can modify the RSC config when needed.
| const config = { | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | ||
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | ||
| password: process.env.RENDERER_PASSWORD || 'devPassword', | ||
| port: process.env.RENDERER_PORT || 3800, | ||
| supportModules: true, | ||
| workersCount: Number(process.env.NODE_RENDERER_CONCURRENCY || 3), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*react-on-rails-pro-node-renderer*" -type fRepository: shakacode/react-webpack-rails-tutorial
Length of output: 119
🏁 Script executed:
cat -n ./react-on-rails-pro-node-renderer.js | head -50Repository: shakacode/react-webpack-rails-tutorial
Length of output: 752
🏁 Script executed:
# Check if there are any other usages of config.port or config.workersCount in the codebase
rg "config\.port|config\.workersCount" --type jsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 144
🏁 Script executed:
# Test the actual behavior of Number coercion with process.env
python3 << 'EOF'
# Simulate process.env behavior - all values are strings or undefined
# Current code behavior
def test_current():
print("=== Current Code Behavior ===")
# Case 1: env var undefined
result = int(3) # Number('undefined' || 3) -> Number(3)
print(f"Number(undefined || 3) = {result}")
# Case 2: env var is invalid string
try:
result = int('abc') # Number('abc' || 3) -> Number('abc')
except:
result = "NaN"
print(f"Number('abc' || 3) = {result}")
# Case 3: env var is valid number string
result = int('8080') # Number('8080')
print(f"Number('8080') = {result}")
test_current()
print("\n=== Suggested Fix Behavior ===")
# Test Number.parseInt with ?? operator
import math
def test_suggested():
# Case 1: env var undefined
val = '' if None is None else None # simulating ?? with empty string
try:
result = int(val, 10) if val else float('nan')
except:
result = float('nan')
print(f"Number.parseInt(undefined ?? '', 10) = {result}")
# Case 2: env var is invalid string
val = 'abc'
try:
result = int(val, 10)
except:
result = float('nan')
print(f"Number.parseInt('abc', 10) = {result}")
# Case 3: env var is valid number string
val = '8080'
result = int(val, 10)
print(f"Number.parseInt('8080', 10) = {result}")
test_suggested()
print("\n=== Key Issue ===")
print("Number(undefined || 3) evaluates the || operator FIRST")
print("So if RENDERER_PORT is undefined, it uses 3 as the numeric default")
print("But if RENDERER_PORT='8080' (string), Number('8080') = 8080 works fine")
print("The real issue: Number('invalid') = NaN, and that could happen if env var is malformed")
EOFRepository: shakacode/react-webpack-rails-tutorial
Length of output: 627
🏁 Script executed:
# Check how reactOnRailsProNodeRenderer is called and if there are any type hints or documentation
rg "reactOnRailsProNodeRenderer" -A 3 -B 3 --type jsRepository: shakacode/react-webpack-rails-tutorial
Length of output: 684
Parse numeric environment variables before passing to config.
process.env values are strings. The port setting stays stringly-typed when RENDERER_PORT is set, while workersCount is coerced with Number(). This inconsistency means invalid NODE_RENDERER_CONCURRENCY becomes NaN. Normalize both values upfront so configuration is predictable across deployments.
Suggested hardening
+const rendererPort = Number.parseInt(process.env.RENDERER_PORT ?? '', 10);
+const workersCount = Number.parseInt(process.env.NODE_RENDERER_CONCURRENCY ?? '', 10);
+
const config = {
serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'),
logLevel: process.env.RENDERER_LOG_LEVEL || 'debug',
password: process.env.RENDERER_PASSWORD || 'devPassword',
- port: process.env.RENDERER_PORT || 3800,
+ port: Number.isFinite(rendererPort) ? rendererPort : 3800,
supportModules: true,
- workersCount: Number(process.env.NODE_RENDERER_CONCURRENCY || 3),
+ workersCount: Number.isFinite(workersCount) ? workersCount : 3,
};📝 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 config = { | |
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | |
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | |
| password: process.env.RENDERER_PASSWORD || 'devPassword', | |
| port: process.env.RENDERER_PORT || 3800, | |
| supportModules: true, | |
| workersCount: Number(process.env.NODE_RENDERER_CONCURRENCY || 3), | |
| const rendererPort = Number.parseInt(process.env.RENDERER_PORT ?? '', 10); | |
| const workersCount = Number.parseInt(process.env.NODE_RENDERER_CONCURRENCY ?? '', 10); | |
| const config = { | |
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | |
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | |
| password: process.env.RENDERER_PASSWORD || 'devPassword', | |
| port: Number.isFinite(rendererPort) ? rendererPort : 3800, | |
| supportModules: true, | |
| workersCount: Number.isFinite(workersCount) ? workersCount : 3, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react-on-rails-pro-node-renderer.js` around lines 4 - 10, The config object
uses process.env values inconsistently: port can remain a string while
workersCount is coerced with Number(), which can produce NaN; normalize both by
parsing and validating their numeric environment variables before assigning to
config—convert process.env.RENDERER_PORT and
process.env.NODE_RENDERER_CONCURRENCY to integers (e.g., parseInt or Number with
fallback), ensure sensible defaults if parsing fails, and assign the
parsed/validated numbers to config.port and config.workersCount (refer to the
config object and the port and workersCount properties) so the configuration is
always numeric and predictable.
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
| prerender: false, | ||
| auto_load_bundle: false, | ||
| trace: Rails.env.development?, | ||
| id: "ServerComponentsPage-react-component-0") %> |
There was a problem hiding this comment.
Hardcoded DOM id can cause collisions and is fragile
"ServerComponentsPage-react-component-0" mirrors React on Rails' auto-generated ID pattern. If the framework's ID generation logic changes, or if this component is ever rendered more than once on a page, this will silently conflict with the framework-assigned ID — potentially causing hydration failures or duplicate-ID accessibility violations.
| id: "ServerComponentsPage-react-component-0") %> | |
| trace: Rails.env.development?) %> |
Unless there is a specific reason the RSC payload route needs to target this exact id (in which case that dependency should be documented), let the framework generate it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/webpack/rscWebpackConfig.js`:
- Around line 24-27: configureRsc currently builds its RSC webpack config from
commonWebpackConfig(), which duplicates the server Node renderer and can drift;
change configureRsc to import/derive the server-side base config (the server
webpack config export, e.g. serverWebpackConfig) instead of calling
commonWebpackConfig(), then apply only RSC-specific overrides (entry, RSC loader
rules, manifest plugin adjustments) on that base; keep getBundler usage and
ensure you only mutate a shallow clone of the server config to avoid affecting
other bundles.
- Around line 45-69: The rule-pruning loop mutates each rule parameter (causing
no-param-reassign lint errors); rewrite the pass to replace
rscConfig.module.rules with rscConfig.module.rules.map(...) that returns new
rule objects—do not reassign properties on the incoming rule. For array-form
uses, produce a new use array filtered of
mini-css-extract-plugin/CssExtractRspackPlugin/cssExtractLoader/style-loader,
locate the css-loader entry in the new array and if its options.modules exists
return a cloned css-loader object with options.modules merged to include
exportOnlyLocals: true; for single-object uses (rule.use.loader matching
url-loader or file-loader) return a new rule with use.options cloned and
emitFile: false set. Ensure all objects/arrays (rule, rule.use, cssLoader,
rule.use.options) are shallow-copied rather than mutated so
rscConfig.module.rules receives a brand-new array of updated rule objects.
🪄 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: b892ca98-3eae-4264-bc23-3333f60fbb05
📒 Files selected for processing (8)
Procfile.devclient/app/bundles/server-components/components/CommentsFeed.jsxconfig/initializers/react_on_rails_pro.rbconfig/webpack/rscWebpackConfig.jsconfig/webpack/rspackRscPlugin.jsconfig/webpack/serverWebpackConfig.jsconfig/webpack/webpackConfig.jsreact-on-rails-pro-node-renderer.js
✅ Files skipped from review due to trivial changes (1)
- Procfile.dev
🚧 Files skipped from review as they are similar to previous changes (3)
- config/webpack/serverWebpackConfig.js
- react-on-rails-pro-node-renderer.js
- config/initializers/react_on_rails_pro.rb
| const configureRsc = () => { | ||
| const bundler = getBundler(); | ||
| const rscConfig = commonWebpackConfig(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Build the RSC config from the server-side base, not commonWebpackConfig().
This file is re-creating a second Node-renderer config by hand, so it can drift from config/webpack/serverWebpackConfig.js and miss future server-only runtime fixes. Extract a shared Node-side base or derive this config from the server bundle config, then layer only the RSC-specific entry / loader / manifest changes here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/webpack/rscWebpackConfig.js` around lines 24 - 27, configureRsc
currently builds its RSC webpack config from commonWebpackConfig(), which
duplicates the server Node renderer and can drift; change configureRsc to
import/derive the server-side base config (the server webpack config export,
e.g. serverWebpackConfig) instead of calling commonWebpackConfig(), then apply
only RSC-specific overrides (entry, RSC loader rules, manifest plugin
adjustments) on that base; keep getBundler usage and ensure you only mutate a
shallow clone of the server config to avoid affecting other bundles.
- Fix JSON response structure mismatch in CommentsFeed (unwrap data.comments) - Fix registerServerComponent API call (remove unsupported options object) - Enable NodeRenderer in test env by using Rails.env.local? instead of development? - Add warning for non-ENOENT errors in rspackRscPlugin _hasUseClientDirective - Avoid no-param-reassign ESLint violations in rscWebpackConfig loader pruning Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review comments addressed (248ba65)Triaged 40 review comments from 5 bots across 2 review rounds. Summary: Fixed (6 items)
Skipped with rationale (32 items)
All 40 threads replied to and resolved. |
| serverWebpackConfig.plugins.push( | ||
| new bundler.BannerPlugin({ | ||
| banner: [ | ||
| 'if(typeof MessageChannel==="undefined"){', | ||
| ' globalThis.MessageChannel=class MessageChannel{', | ||
| ' constructor(){', | ||
| ' this.port1={onmessage:null};', | ||
| ' this.port2={postMessage:function(msg){', | ||
| ' var p=this._port1;if(p.onmessage)p.onmessage({data:msg});', | ||
| ' }};', | ||
| ' this.port2._port1=this.port1;', | ||
| ' }', | ||
| ' };', | ||
| '}', | ||
| ].join('\n'), | ||
| raw: true, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
The hand-rolled MessageChannel polyfill is asymmetric — only port2.postMessage → port1.onmessage is wired up; port1.postMessage → port2.onmessage is never connected.
React's concurrent scheduler uses both ports for bi-directional task scheduling. If the VM ever calls port1.postMessage(...), port2.onmessage won't fire and scheduling will silently stall.
| serverWebpackConfig.plugins.push( | |
| new bundler.BannerPlugin({ | |
| banner: [ | |
| 'if(typeof MessageChannel==="undefined"){', | |
| ' globalThis.MessageChannel=class MessageChannel{', | |
| ' constructor(){', | |
| ' this.port1={onmessage:null};', | |
| ' this.port2={postMessage:function(msg){', | |
| ' var p=this._port1;if(p.onmessage)p.onmessage({data:msg});', | |
| ' }};', | |
| ' this.port2._port1=this.port1;', | |
| ' }', | |
| ' };', | |
| '}', | |
| ].join('\n'), | |
| raw: true, | |
| }), | |
| ); | |
| serverWebpackConfig.plugins.push( | |
| new bundler.BannerPlugin({ | |
| banner: [ | |
| 'if(typeof MessageChannel==="undefined"){', | |
| ' globalThis.MessageChannel=class MessageChannel{', | |
| ' constructor(){', | |
| ' var p1={onmessage:null};', | |
| ' var p2={onmessage:null};', | |
| ' p1.postMessage=function(msg){if(p2.onmessage)p2.onmessage({data:msg});};', | |
| ' p2.postMessage=function(msg){if(p1.onmessage)p1.onmessage({data:msg});};', | |
| ' this.port1=p1;', | |
| ' this.port2=p2;', | |
| ' }', | |
| ' };', | |
| '}', | |
| ].join('\n'), | |
| raw: true, | |
| }), | |
| ); |
| 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.
extractLoader is an exact copy of the same function defined (and exported) in serverWebpackConfig.js. The server config already exports it via module.exports.extractLoader = extractLoader.
| 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'); |
This eliminates the duplication and ensures both configs stay in sync if the logic ever changes.
| password: rendererPassword, | ||
| port: process.env.RENDERER_PORT || 3800, | ||
| supportModules: true, | ||
| workersCount: Number(process.env.NODE_RENDERER_CONCURRENCY || 3), |
There was a problem hiding this comment.
Number('abc') silently returns NaN, which the renderer will then accept as a valid workersCount. This can cause cryptic failures at startup. Add a guard:
| workersCount: Number(process.env.NODE_RENDERER_CONCURRENCY || 3), | |
| workersCount: (() => { | |
| const n = Number(process.env.NODE_RENDERER_CONCURRENCY || 3); | |
| if (!Number.isFinite(n) || n < 1) throw new Error(`Invalid NODE_RENDERER_CONCURRENCY: "${process.env.NODE_RENDERER_CONCURRENCY}"`); | |
| return n; | |
| })(), |
| 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.
Information disclosure: os.hostname() returns the actual server/container hostname, which can leak infrastructure details (e.g. pod names, EC2 instance IDs, internal DNS). This is a tutorial app so the risk is limited, but it's worth either omitting the field or replacing it with a privacy-safe placeholder in non-development environments:
| hostname: os.hostname(), | |
| hostname: process.env.NODE_ENV === 'development' ? os.hostname() : '[redacted]', |
| const safeHtml = sanitizeHtml(rawHtml, { | ||
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | ||
| allowedAttributes: { | ||
| ...sanitizeHtml.defaults.allowedAttributes, | ||
| img: ['src', 'alt', 'title', 'width', 'height'], | ||
| }, | ||
| allowedSchemes: ['https', 'http'], | ||
| }); |
There was a problem hiding this comment.
Allowing img tags with src pointing to arbitrary http/https URLs means any user-authored comment can embed external images, including tracking pixels that reveal visitor IPs to third-party servers.
Consider either:
- Dropping
imgfromallowedTags(safest for a comments feed), or - Keeping
imgbut restrictingsrcto relative paths only viaallowedSchemesByTag:
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']), | |
| allowedAttributes: { | |
| ...sanitizeHtml.defaults.allowedAttributes, | |
| img: ['src', 'alt', 'title', 'width', 'height'], | |
| }, | |
| allowedSchemes: ['https', 'http'], | |
| }); | |
| const safeHtml = sanitizeHtml(rawHtml, { | |
| allowedTags: sanitizeHtml.defaults.allowedTags, | |
| allowedAttributes: sanitizeHtml.defaults.allowedAttributes, | |
| }); |
Code Review — Fix SSR runtime failures for React Server ComponentsOverall this is solid work: the root-cause analysis in the PR description is accurate, and the fixes (fallbacks over externals, BannerPlugin polyfill, Bug — asymmetric
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
client/app/bundles/server-components/components/CommentsFeed.jsx (1)
43-48: Use destructuring and add defensive fallback for missing comments array.ESLint flags the assignment on line 44. Additionally, if
data.commentsisundefined,_.orderBywill return an empty array but_.takeon it is fine. However, using destructuring with a default is cleaner.♻️ Proposed fix
- const data = await response.json(); - const comments = data.comments; + 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 43 - 48, Replace the direct property access assignment to data.comments with destructuring that provides a default empty array and avoid reassigning variables: extract comments via const { comments = [] } = data (from the response.json() result) and then pass that comments array into _.orderBy and _.take to compute sortedComments and recentComments (referencing _.orderBy, _.take, sortedComments, recentComments) so the code is defensive when data.comments is missing and satisfies ESLint.config/webpack/rspackRscPlugin.js (2)
40-47: Only the first.jsfile per chunk is collected.The
breakon Line 45 stops after finding the first.jsfile. If a chunk emits multiple relevant.jsfiles (e.g., async chunks or split bundles), subsequent files are ignored. This may be intentional givenLimitChunkCountPlugin({ maxChunks: 1 })is used in the RSC config, but if this plugin is reused elsewhere, it could miss module-to-chunk mappings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 40 - 47, The current loop over compilation.chunks collects only the first matching .js file because of the break in the inner loop; remove the break and instead push all files that endWith('.js') and not endWith('.hot-update.js') into chunkFiles (the block iterating compilation.chunks and chunk.files that builds chunkFiles), so all emitted JS outputs for a chunk are captured; if the single-file assumption is intentional due to LimitChunkCountPlugin({ maxChunks: 1 }), add a comment or a feature flag to preserve the existing behavior when required.
92-94: Regex doesn't handle hashbang or BOM before directive.The regex pattern correctly handles comments but doesn't account for:
- Hashbang (
#!/usr/bin/env node\n) which is valid at the start of ESM files- UTF-8 BOM (
\uFEFF) which some editors prependThis is likely a minor edge case since RSC source files rarely have hashbangs, but worth noting.
♻️ Optional: Extended regex to handle edge cases
- result = /^(?:\s*(?:\/\/[^\n]*\n|\/\*[\s\S]*?\*\/))*\s*['"]use client['"]/.test(head); + // Handle BOM, hashbang, whitespace, and comments before directive + result = /^(?:\uFEFF)?(?:#![^\n]*\n)?(?:\s*(?:\/\/[^\n]*\n|\/\*[\s\S]*?\*\/))*\s*['"]use client['"]/.test(head);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/webpack/rspackRscPlugin.js` around lines 92 - 94, The regex that assigns result from head (the line with result = /.../.test(head)) currently allows only comments before "use client" and should be updated to also accept an optional UTF‑8 BOM (\uFEFF) and an optional hashbang (#!... newline) before comments/directive; modify the pattern used in the assignment to result (the regex literal that tests head) to permit an optional BOM and an optional leading hashbang line prior to the existing comment-handling and the ['"]use client['"] check so files starting with BOM or #! are correctly detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/app/bundles/server-components/components/CommentsFeed.jsx`:
- Around line 43-48: Replace the direct property access assignment to
data.comments with destructuring that provides a default empty array and avoid
reassigning variables: extract comments via const { comments = [] } = data (from
the response.json() result) and then pass that comments array into _.orderBy and
_.take to compute sortedComments and recentComments (referencing _.orderBy,
_.take, sortedComments, recentComments) so the code is defensive when
data.comments is missing and satisfies ESLint.
In `@config/webpack/rspackRscPlugin.js`:
- Around line 40-47: The current loop over compilation.chunks collects only the
first matching .js file because of the break in the inner loop; remove the break
and instead push all files that endWith('.js') and not endWith('.hot-update.js')
into chunkFiles (the block iterating compilation.chunks and chunk.files that
builds chunkFiles), so all emitted JS outputs for a chunk are captured; if the
single-file assumption is intentional due to LimitChunkCountPlugin({ maxChunks:
1 }), add a comment or a feature flag to preserve the existing behavior when
required.
- Around line 92-94: The regex that assigns result from head (the line with
result = /.../.test(head)) currently allows only comments before "use client"
and should be updated to also accept an optional UTF‑8 BOM (\uFEFF) and an
optional hashbang (#!... newline) before comments/directive; modify the pattern
used in the assignment to result (the regex literal that tests head) to permit
an optional BOM and an optional leading hashbang line prior to the existing
comment-handling and the ['"]use client['"] check so files starting with BOM or
#! are correctly detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18d17adb-65a2-4ba5-85c4-1325b3c8dc71
📒 Files selected for processing (5)
client/app/bundles/server-components/components/CommentsFeed.jsxclient/app/packs/stimulus-bundle.jsconfig/initializers/react_on_rails_pro.rbconfig/webpack/rscWebpackConfig.jsconfig/webpack/rspackRscPlugin.js
✅ Files skipped from review due to trivial changes (1)
- config/initializers/react_on_rails_pro.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- client/app/packs/stimulus-bundle.js
Automated tests can pass while bin/dev is completely broken (as seen in shakacode/react-webpack-rails-tutorial#723). This adds a structured checklist that agents must follow when changes affect how the app starts, builds, or serves. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document the three-bundle architecture (client, server SSR, RSC), Node Renderer setup requirements, VM sandbox constraints, RSC component classification rules, and common troubleshooting patterns. Update README with current version targets, RSC section, expanded config file list, and all six Procfile.dev processes. Motivated by the debugging challenges in PR #723 where undocumented constraints (VM sandbox lacks require/MessageChannel, 'use client' classification, Node renderer must run for tests) caused significant debugging time. Filed shakacode/react_on_rails#3076 with upstream doc suggestions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review: Fix SSR runtime failures for React Server ComponentsOverall this PR is well-structured and solves a real problem cleanly. The PR description is thorough and the CLAUDE.md additions are genuinely helpful for future contributors. A few issues worth addressing before merge: Security: Hardcoded password in CI workflow
RENDERER_PASSWORD: ${{ secrets.RENDERER_PASSWORD }}Add Bug:
|
| DRIVER: selenium_chrome | ||
| CHROME_BIN: /usr/bin/google-chrome | ||
| USE_COVERALLS: true | ||
| RENDERER_PASSWORD: devPassword |
There was a problem hiding this comment.
Security: this hardcodes a credential in source. Even as a dev-only password in a tutorial, it teaches poor practice. Use a GitHub Actions secret instead:
| RENDERER_PASSWORD: devPassword | |
| RENDERER_PASSWORD: ${{ secrets.RENDERER_PASSWORD }} |
Add RENDERER_PASSWORD=devPassword to this repo's Actions secrets (Settings → Secrets and variables → Actions). That way the value is injected at runtime and not committed to the repo.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
76-93:⚠️ Potential issue | 🟡 MinorFix broken Table of Contents anchors.
The TOC links on Line 91 (
#about-shakacode) and Line 93 (#open-code-of-conduct) do not resolve to headings in this README. Please remove or update them to valid anchors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 76 - 93, The table of contents contains broken anchors '#about-shakacode' and '#open-code-of-conduct' — locate the TOC entries referencing these anchors and either remove them or update them to the exact heading slugs used in the document (match the heading text/casing and spacing converted to the correct GitHub-style anchor) so the links resolve; specifically edit the TOC lines containing "[About ShakaCode](`#about-shakacode`)" and "[Open Code of Conduct](`#open-code-of-conduct`)" to the correct anchors or delete them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 19: Update the heading "Three Bundle System" to use a hyphenated compound
adjective by changing the text to "Three-Bundle System" in the CLAUDE.md heading
(locate the heading string "Three Bundle System" and replace it with
"Three-Bundle System").
---
Outside diff comments:
In `@README.md`:
- Around line 76-93: The table of contents contains broken anchors
'#about-shakacode' and '#open-code-of-conduct' — locate the TOC entries
referencing these anchors and either remove them or update them to the exact
heading slugs used in the document (match the heading text/casing and spacing
converted to the correct GitHub-style anchor) so the links resolve; specifically
edit the TOC lines containing "[About ShakaCode](`#about-shakacode`)" and "[Open
Code of Conduct](`#open-code-of-conduct`)" to the correct anchors or delete them.
🪄 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: 3d31334d-28a5-4bb6-af9a-61e284dabef4
📒 Files selected for processing (2)
CLAUDE.mdREADME.md
| bundle exec rubocop -a | ||
| ``` | ||
|
|
||
| ## Architecture: Three Bundle System |
There was a problem hiding this comment.
Use hyphenated compound modifier in heading.
Line 19 should read “Three-Bundle System” for correct compound adjective style.
Suggested edit
-## Architecture: Three Bundle System
+## Architecture: Three-Bundle System📝 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.
| ## Architecture: Three Bundle System | |
| ## Architecture: Three-Bundle System |
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: Use a hyphen to join words.
Context: ...c rubocop -a ``` ## Architecture: Three Bundle System This project builds **thr...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 19, Update the heading "Three Bundle System" to use a
hyphenated compound adjective by changing the text to "Three-Bundle System" in
the CLAUDE.md heading (locate the heading string "Three Bundle System" and
replace it with "Three-Bundle System").
| const chunks = []; | ||
| for (const file of chunkFiles) { | ||
| chunks.push(chunk.id, file); | ||
| } |
There was a problem hiding this comment.
Potential bug: chunks.push(chunk.id, file) interleaves chunk ID and filename into the same flat array, producing [chunkId, filename]. The React flight client manifest format expects chunks to be an array of chunk filenames (or URLs) only — not pairs. The standard ReactFlightWebpackPlugin outputs ["filename.js"].
If the React runtime tries to resolve chunk.id (a number) as a URL, client components will silently fail to hydrate. Double-check against the manifest format that react-on-rails-rsc consumes — it may expect just the filename:
| const chunks = []; | |
| for (const file of chunkFiles) { | |
| chunks.push(chunk.id, file); | |
| } | |
| const chunks = [...chunkFiles]; |
| 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.
Code duplication: extractLoader is defined identically in serverWebpackConfig.js (lines 9–15), which even exports it at the bottom (module.exports.extractLoader = extractLoader). Import from there instead of redeclaring:
| 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'); |
Or better yet, move the utility to bundlerUtils.js so neither config file exports implementation details of the other.
| // The RSC bundle runs server components in the Node renderer and produces | ||
| // the Flight payload that React uses to hydrate on the client. | ||
| // | ||
| // Unlike the server bundle (which uses ExecJS), the RSC bundle targets Node.js |
There was a problem hiding this comment.
Stale comment — ExecJS was removed in this PR. The server bundle now runs in the Node renderer's VM sandbox, not ExecJS.
| // Unlike the server bundle (which uses ExecJS), the RSC bundle targets Node.js | |
| // Unlike the server bundle (which runs in the Node renderer's VM sandbox), | |
| // the RSC bundle targets full Node.js and can use built-in modules (os, fs, path, etc.) directly. |
| ...sanitizeHtml.defaults.allowedAttributes, | ||
| img: ['src', 'alt', 'title', 'width', 'height'], | ||
| }, | ||
| allowedSchemes: ['https', 'http'], |
There was a problem hiding this comment.
Allowing http enables mixed-content rendering (HTTP images on an HTTPS page) and is generally discouraged. For a tutorial that others learn from, ['https'] only is a safer default:
| allowedSchemes: ['https', 'http'], | |
| allowedSchemes: ['https'], |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6429ce0cae
ℹ️ 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".
| ' this.port2={postMessage:function(msg){', | ||
| ' var p=this._port1;if(p.onmessage)p.onmessage({data:msg});', |
There was a problem hiding this comment.
The injected MessageChannel shim invokes port1.onmessage synchronously inside postMessage, but React 19's server scheduler relies on MessageChannel to schedule work on a later tick. In larger SSR/RSC streams this synchronous re-entry can recurse (postMessage immediately runs the handler, which schedules again) and lead to stack overflows or renderer stalls/timeouts. Queue the handler asynchronously (for example via setImmediate/setTimeout(0)/microtask) so the polyfill preserves actual MessageChannel behavior.
Useful? React with 👍 / 👎.
- Import react-on-rails-pro in rsc-bundle.js to initialize RSC support (resolves to ReactOnRailsRSC.js with react-server condition) - Move @rspack/dev-server to devDependencies where it belongs - Add QA_PLAYBOOK.md with manual testing checklist for all rendering modes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| 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.
Duplicate function: extractLoader is already defined and exported from serverWebpackConfig.js. Import it from there instead of redefining it here.
| }); | |
| const { extractLoader } = require('./serverWebpackConfig'); |
Then remove the local definition (lines 15–21).
| allowedSchemes: ['https', 'http'], | ||
| }); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Security / mixed content: allowedSchemes includes 'http', which permits images loaded over insecure connections. When the app is served over HTTPS this will trigger mixed-content warnings and browsers will block the image. Restrict to 'https' only.
| return ( | |
| allowedSchemes: ['https'], |
| logLevel: process.env.RENDERER_LOG_LEVEL || 'debug', | ||
| password: rendererPassword, | ||
| port: process.env.RENDERER_PORT || 3800, | ||
| supportModules: true, |
There was a problem hiding this comment.
Type bug: process.env.RENDERER_PORT is always a string when set. The || fallback only applies when it's unset/empty, so a configured port is passed as a string (e.g. "3800") while the default is the number 3800. Depending on how the renderer internally binds the port this may work, but it's fragile.
| supportModules: true, | |
| port: Number(process.env.RENDERER_PORT) || 3800, |
| @@ -82,6 +83,20 @@ jobs: | |||
| - name: Build shakapacker chunks | |||
| run: NODE_ENV=development bundle exec bin/shakapacker | |||
|
|
|||
There was a problem hiding this comment.
Observability: The renderer's stdout/stderr goes to the runner's default output mixed with the CI step log, making it hard to diagnose start-up failures. Redirect to a log file so it can be shown only on failure:
| node react-on-rails-pro-node-renderer.js > /tmp/node-renderer.log 2>&1 & |
Then add a step after the readiness check (or in an if: failure() step) to cat /tmp/node-renderer.log so failures are captured.
| arch: os.arch(), | ||
| nodeVersion: process.version, | ||
| uptime: Math.floor(os.uptime() / 3600), | ||
| totalMemory: (os.totalmem() / (1024 * 1024 * 1024)).toFixed(1), |
There was a problem hiding this comment.
Info disclosure (low, demo context): hostname leaks the server's machine name to any visitor. Fine for a tutorial/demo, but worth keeping out of any production deployment. Consider omitting it or replacing it with a sanitised label before this demo is referenced as a production template.
PR Review: Fix SSR runtime failures for React Server ComponentsOverall this is a well-structured PR. The root-cause analysis (VM sandbox has no Issues found
Inline comments posted for each item above. Other notes
|
1 participant
Summary
Fixes the 37/38 rspec test failures on the RSC branch caused by SSR runtime errors after switching from ExecJS to the react-on-rails-pro NodeRenderer.
Net::ReadTimeouton all prerendered pages. Added a startup step with TCP readiness check andRENDERER_PASSWORDenv var.path/fs/stream) as CommonJSrequire()calls, but the Node renderer'svm.createContext()sandbox has norequire. Reverted toresolve.fallback: falsewhich stubs these unused code paths at build time.react-dom/server.browser.jsinstantiatesMessageChannelat module load. The Node renderer VM lacks this global (unlike Bun used by ExecJS on master). Added aBannerPluginpolyfill.'use client'directive — The auto-bundling system misclassified this traditional SSR component (usesStaticRouter) as a React Server Component because it lacked'use client'. Added the directive so it registers viaReactOnRails.register()instead ofregisterServerComponent().All 38 rspec tests pass locally.
Test plan
bundle exec rspec— all 38 tests pass (0 failures)bundle exec rubocop— no offenses🤖 Generated with Claude Code
Note
Medium Risk
Medium risk due to new SSR/RSC runtime dependencies (Node renderer, bundle splitting, polyfills) and changes to the build pipeline that could affect rendering and CI stability.
Overview
Enables React on Rails Pro NodeRenderer and React Server Components. Adds
react_on_rails_proconfiguration (react_on_rails_pro.rb), a Node renderer launcher (react-on-rails-pro-node-renderer.js), and updates CI to start the renderer (with password) and wait for port readiness before running RSpec.Introduces a third Shakapacker/Rspack bundle for RSC. Adds
rscWebpackConfig.js, wires it intowebpackConfig.jswithRSC_BUNDLE_ONLY, and adds a Rspack-compatibleRspackRscPluginto emit React Flight manifests; also updates client/server bundler configs with fallbacks and aMessageChannelpolyfill for the VM-based SSR environment.Adds an RSC demo route and client/server registration changes. Creates
/server-componentspage + navigation link, adds RSC entry packs (rsc-bundle.js,rsc-client-components.js), and updates multiple components/packs to usereact-on-rails-proand explicit'use client'directives to avoid RSC misclassification.Reviewed by Cursor Bugbot for commit f09b789. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit