Skip to content

Pro RSC migration 3/3: React Server Components demo on webpack#729

Open
ihabadham wants to merge 13 commits intoihabadham/feature/pro-rsc/basefrom
ihabadham/feature/pro-rsc/rsc-demo
Open

Pro RSC migration 3/3: React Server Components demo on webpack#729
ihabadham wants to merge 13 commits intoihabadham/feature/pro-rsc/basefrom
ihabadham/feature/pro-rsc/rsc-demo

Conversation

@ihabadham
Copy link
Copy Markdown
Collaborator

Summary

Adds a React Server Components demo page at /server-components via the upstream RSCWebpackPlugin (react-on-rails-rsc/WebpackPlugin), riding on top of Sub-PR 2's NodeRenderer + webpack setup.

Part 3 of a stacked PR series. Targets ihabadham/feature/pro-rsc/base, not master.

References this PR follows

  • Pro docs: docs/oss/migrating/rsc-preparing-app.md (Steps 4-5), docs/pro/react-server-components/upgrading-existing-pro-app.md, docs/pro/react-server-components/how-react-server-components-work.md, docs/oss/migrating/rsc-component-patterns.md
  • Pro dummy app: react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js (RSC bundle shape, loader placement), spec/dummy/config/webpack/clientWebpackConfig.js (RSCWebpackPlugin({ isServer: false }) pattern)
  • RSC marketplace demo: react-on-rails-demo-marketplace-rsc/config/webpack/rscWebpackConfig.js (simpler resolve config without React path aliases), Procfile.dev (RSC_BUNDLE_ONLY=yes watcher)
  • Justin's PR Fix SSR runtime failures for React Server Components #723: source of server-components/ demo components (ServerComponentsPage, ServerInfo, CommentsFeed, TogglePanel), routes + controller + view

Changes

Initializer

  • config/initializers/react_on_rails_pro.rb: adds enable_rsc_support = true, rsc_bundle_js_file = 'rsc-bundle.js', rsc_payload_generation_url_path = 'rsc_payload/'. On top of Sub-PR 2's NodeRenderer config.

Webpack

  • config/webpack/clientWebpackConfig.js: adds RSCWebpackPlugin({ isServer: false, clientReferences: [...] }) so the client bundle emits react-client-manifest.json with client component chunk entries.
  • config/webpack/rscWebpackConfig.js: new file. Derives from serverWebpackConfig(true) via entry rename, adds the react-server resolve condition, aliases react-dom/server: false. Loader placement follows the Pro docs' prescribed pattern — handles both SWC (function-form rule.use) and Babel (array-form rule.use) transpilers.
  • config/webpack/webpackConfig.js: adds RSC_BUNDLE_ONLY env gate matching the existing SERVER_BUNDLE_ONLY/CLIENT_BUNDLE_ONLY pattern. Default build now compiles all three bundles (client + server + RSC).

Components

  • 'use client' directive added to 9 files per Pro docs' Step 5 rule (entry points using hooks, <Provider>, Redux, client APIs):
    • App.jsx, NavigationBarApp.jsx, RouterApp.client.jsx, RouterApp.server.jsx (SSR wrapper — NOT a React Server Component despite the filename), SimpleCommentScreen.jsx, Footer.jsx, RescriptShow.jsx, stores-registration.js, stimulus-bundle.js.
  • New RSC demo files under client/app/bundles/server-components/:
    • ror_components/ServerComponentsPage.jsx — Server Component entry point (placed in ror_components/ so auto-bundling registers it via registerServerComponent).
    • components/ServerInfo.jsx, components/CommentsFeed.jsx — Server Components.
    • components/TogglePanel.jsx — Client Component ('use client') demonstrating the boundary.

Routes / View

  • config/routes.rb: adds rsc_payload_route (Pro's RSC Flight endpoint) + get "server-components".
  • app/controllers/pages_controller.rb: server_components action.
  • app/views/pages/server_components.html.erb: react_component("ServerComponentsPage", prerender: false, auto_load_bundle: true, trace: Rails.env.development?). auto_load_bundle: true pairs with auto-discovery — no manual registerServerComponent call needed.

Dev / CI

  • Procfile.dev: wp-rsc process runs RSC_BUNDLE_ONLY=yes bin/shakapacker --watch alongside existing client/server watchers.
  • NavigationBar.jsx + paths.js: adds "RSC Demo" nav link to the new page.

What we explicitly DON'T do (diverges from Justin's PR #723)

  • No rspackRscPlugin.js (187 lines of custom rspack plugin). We use the upstream react-on-rails-rsc/WebpackPlugin on webpack instead.
  • No client/app/packs/rsc-bundle.js separate source file. RSC bundle reuses server-bundle.js via entry rename.
  • No client/app/packs/rsc-client-components.js manual side-import. Upstream plugin's AsyncDependenciesBlock handles client-ref chunk inclusion.
  • No manual registerServerComponent in stimulus-bundle.js. Auto-bundling's react_on_rails:generate_packs produces the registration file (generated/ServerComponentsPage.js with registerServerComponent("ServerComponentsPage")) because ServerComponentsPage.jsx lives in a ror_components/ directory.
  • No MessageChannel BannerPlugin polyfill. Webpack with target: 'node' (set in Sub-PR 2) avoids the underlying issue.

Acceptance criteria

Manual QA on the deployed review app (/deploy-review-app comment in this PR). Sub-PR 2 validated its deploy via the same flow.

  • Review app builds clean (Docker build succeeds with 0 errors)
  • GET / still returns 200 with full SSR content (existing pages unchanged)
  • GET /server-components returns 200 with RSC content streamed via Flight protocol
  • GET /rsc_payload/ServerComponentsPage returns valid Flight payload
  • Browser console on /server-components has no errors during page load and hydration
  • RSC-rendered ServerInfo + CommentsFeed content appears in the DOM
  • TogglePanel (client component inside server component tree) is interactive (click toggles state)
  • Rails log shows [ReactOnRailsPro] Node Renderer responded for the RSC render path
  • Renderer log shows RSC Flight stream generation for ServerComponentsPage

Stack context

ihabadham and others added 11 commits April 23, 2026 20:56
Add the three RSC fields per the marketplace demo initializer
(react-on-rails-demo-marketplace-rsc/config/initializers/
react_on_rails_pro.rb):

- enable_rsc_support = true
- rsc_bundle_js_file = "rsc-bundle.js"
- rsc_payload_generation_url_path = "rsc_payload/"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RSCWebpackPlugin({ isServer: false }) on the client bundle scans for
'use client' files and adds them as entry points so they appear in the
client manifest (react-client-manifest.json). Without this, client
components referenced in RSC payloads wouldn't have matching chunks
in the client bundle.

clientReferences scoped to config.source_path, consistent with the
server bundle's scoping in serverWebpackConfig.js.

Reference: Pro dummy clientWebpackConfig.js:16-24.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Derives from serverWebpackConfig(true) — inherits target:'node',
libraryTarget:'commonjs2', CSS filtering, and all server transforms.
Adds three RSC-specific pieces:

1. RSC WebpackLoader pushed into the babel rule's use array (runs
   before babel per right-to-left order) to detect 'use client'
   directives in raw source and replace client exports with
   registerClientReference proxies.

2. react-server resolve condition so React's RSC-specific entry
   points are used.

3. react-dom/server aliased to false (RSC bundles generate Flight
   payloads, not HTML; importing react-dom/server causes a runtime
   error).

Loader placement follows Pro dummy pattern (push into rule.use) per
docs/oss/migrating/rsc-preparing-app.md:167-195. NOT marketplace's
enforce:'post' which runs after transpilation and can miss directive
AST nodes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add RSC_BUNDLE_ONLY env gate alongside the existing SERVER_BUNDLE_ONLY
and CLIENT_BUNDLE_ONLY gates. Procfile.dev will use
RSC_BUNDLE_ONLY=yes bin/shakapacker --watch to build the RSC bundle
separately during development.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RSC auto-classification: files without 'use client' are registered as
Server Components via registerServerComponent(). All existing
components are Client Components (hooks, Redux, Router, event
handlers), so they need the directive to preserve current behavior.

Entry points (7 ror_components/ files):
- App.jsx, NavigationBarApp.jsx, RouterApp.client.jsx,
  RouterApp.server.jsx (SSR wrapper, NOT a Server Component),
  SimpleCommentScreen.jsx, Footer.jsx, RescriptShow.jsx

Pack entry files (2):
- stores-registration.js, stimulus-bundle.js

Per docs/oss/migrating/rsc-preparing-app.md Step 5 and
docs/pro/react-server-components/create-without-ssr.md:52.
Matches Justin's PR 723 final state exactly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- rsc_payload_route in routes.rb enables the Flight protocol endpoint
  for client-side RSC payload fetching.
- get "server-components" route maps to pages#server_components.
- View uses prerender: false (RSC components are streamed via the
  payload route, not traditional SSR prerender) and
  auto_load_bundle: false (ServerComponentsPage is not in
  ror_components/, so auto-discovery doesn't find its pack).
- trace: Rails.env.development? gates server-timing headers to dev.

Reference: Justin's PR 723 commits 4d09e13 + 0d8d75a.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Server Components (no 'use client'):
- ServerComponentsPage.jsx: demo container showing RSC streaming
- components/ServerInfo.jsx: displays server environment info
- components/CommentsFeed.jsx: async data fetch with timeout,
  env-gated delay, img sanitization, data.comments unwrap

Client Component ('use client'):
- components/TogglePanel.jsx: interactive panel demonstrating
  'use client' boundary within a server component tree

Salvaged from Justin's PR 723 final state per the journey report
KEEP table. CommentsFeed specifically from commit f008295
(has the fetch timeout + sanitization fixes from review).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Procfile.dev: wp-rsc process runs RSC_BUNDLE_ONLY=yes bin/shakapacker
  --watch alongside existing client, server, and renderer processes.
- paths.js: SERVER_COMPONENTS_PATH constant.
- NavigationBar.jsx: "RSC Demo" link in the nav bar.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The default branch (no env vars) runs during bin/shakapacker for
production/CI builds. Without the RSC config in the array, the RSC
bundle only gets built when RSC_BUNDLE_ONLY is set (dev watchers).
Production deploys + CI would miss it.

The *_BUNDLE_ONLY gates remain for dev Procfile processes (each
watcher builds one bundle in isolation).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Justin's PR used manual registerServerComponent() in stimulus-bundle.js
because his custom rspackRscPlugin didn't integrate with the auto-bundling
flow. With the upstream RSCWebpackPlugin, auto-bundling works: the
generate_packs task scans ror_components/ directories, classifies files
without 'use client' as Server Components, and generates the registration
file in generated/ServerComponentsPage.js automatically.

Moved from: bundles/server-components/ServerComponentsPage.jsx
Moved to:   bundles/server-components/ror_components/ServerComponentsPage.jsx

Updated relative imports (./components/ -> ../components/) and flipped the
view from auto_load_bundle: false to true. No manual registration, no
stimulus-bundle.js modification. Matches the Pro dummy pattern where
server component sources sit in the auto-discovered directory.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous implementation only handled Array.isArray(rule.use) and
only looked for babel-loader. The tutorial uses swc as its transpiler
(shakapacker.yml: javascript_transpiler: swc), which makes Shakapacker
generate rule.use as a FUNCTION, not an array. The RSC loader was
therefore never attached to the transpilation rule — 'use client'
files were left untransformed in the RSC bundle, producing 134
webpack warnings (export 'useState' not found in 'react' etc.) and
setting up a runtime error when the RSC renderer would try to call
client components directly instead of emitting client references.

Follow the pattern from docs/oss/migrating/rsc-preparing-app.md:167
verbatim, which handles both forms and both loader names.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 43601667-8c98-44c7-a965-d47a70fa4a20

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ihabadham/feature/pro-rsc/rsc-demo

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ihabadham
Copy link
Copy Markdown
Collaborator Author

/deploy-review-app

@github-actions
Copy link
Copy Markdown

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:

/deploy-review-app

Deploy your PR branch for testing

/delete-review-app

Remove the review app when done

/help

Show detailed instructions, environment setup, and configuration options.


@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #729, commit 649e0bd

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

Comment on lines +7 to +10
'rsc-bundle': rscConfig.entry['server-bundle'],
};
rscConfig.entry = rscEntry;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RSC config silently sets entry['rsc-bundle'] to undefined if serverWebpackConfig(true) produces no server-bundle entry (e.g. missing server-bundle.js pack file). serverWebpackConfig.js itself guards against this with a descriptive throw; the same guard should be applied here to make the failure message actionable instead of a cryptic webpack error downstream.

Suggested change
'rsc-bundle': rscConfig.entry['server-bundle'],
};
rscConfig.entry = rscEntry;
const serverBundleEntry = rscConfig.entry['server-bundle'];
if (!serverBundleEntry) {
const sourcePath = require('shakapacker').config.source_path || 'client/app';
throw new Error(
`RSC bundle entry 'server-bundle' not found in server webpack config.\n` +
`Expected server-bundle.js at ${sourcePath}/packs/server-bundle.js\n` +
`Verify nested_entries and source_entry_path in shakapacker.yml.`,
);
}
const rscEntry = {
'rsc-bundle': serverBundleEntry,
};
rscConfig.entry = rscEntry;

img: ['src', 'alt', 'title', 'width', 'height'],
},
allowedSchemes: ['https', 'http'],
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowedSchemes applies to every URL-type attribute (including img[src]). Allowing http exposes a few risks in production: mixed-content warnings in browsers, potential tracking pixels from plain-HTTP origins, and SSRF-adjacent issues if a proxy ever fetches these URLs server-side. Since the sanitisation already runs server-side, restricting to https only costs nothing at runtime.

Suggested change
});
allowedSchemes: ['https'],

totalMemory: (os.totalmem() / (1024 * 1024 * 1024)).toFixed(1),
freeMemory: (os.freemem() / (1024 * 1024 * 1024)).toFixed(1),
cpus: os.cpus().length,
hostname: os.hostname(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.hostname() is included in the RSC Flight payload and therefore visible in the raw HTTP response body, the browser DevTools network panel, and any CDN cache. Even for a demo, leaking the actual server hostname in production is worth suppressing — it aids host enumeration. Consider either omitting it from the serverInfo object or masking it to a fixed string outside development:

Suggested change
hostname: os.hostname(),
hostname: process.env.NODE_ENV === 'development' ? os.hostname() : '(hidden in production)',

prerender: false,
auto_load_bundle: true,
trace: Rails.env.development?,
id: "ServerComponentsPage-react-component-0") %>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit id: is unusual here — React on Rails auto-generates stable IDs. Hardcoding "ServerComponentsPage-react-component-0" means a second react_component call on the same page (e.g. if this partial is ever included twice, or if a test helper renders it multiple times) would produce a duplicate HTML id, breaking DOM queries and accessibility tooling. Unless this specific ID is required by the Pro RSC runtime, dropping the parameter lets Rails generate it automatically.

Suggested change
id: "ServerComponentsPage-react-component-0") %>
<%= react_component("ServerComponentsPage",
prerender: false,
auto_load_bundle: true,
trace: Rails.env.development?) %>

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review — PR #729: RSC Demo via RSCWebpackPlugin (webpack)

Overview

Well-structured third leg of the RSC stacked series. The approach — deriving the RSC bundle from serverWebpackConfig(true), wiring the upstream RSCWebpackPlugin on both client and RSC bundles, and relying on auto-bundling (ror_components/) for component registration — is clean and avoids the 187-line custom rspack plugin from PR #723. The SWC/Babel dual-form loader injection in rscWebpackConfig.js is the most complex piece and appears correct.


Issues (inline comments posted)

Severity File Issue
Medium config/webpack/rscWebpackConfig.js:7–10 Missing entry-existence guard — silently assigns undefined as the RSC entry if server-bundle is absent; serverWebpackConfig.js throws a descriptive error for the same case
Low client/app/bundles/server-components/components/CommentsFeed.jsx:90 allowedSchemes: ['https', 'http']http enables mixed-content image URLs in production; restrict to ['https']
Low client/app/bundles/server-components/components/ServerInfo.jsx:17 os.hostname() appears in the Flight payload and is therefore visible in raw HTTP responses; mask it outside development
Nit app/views/pages/server_components.html.erb:5 Hardcoded id: is unusual — React on Rails auto-generates stable IDs, and the explicit value would produce a duplicate id if the partial is ever rendered more than once per page

Observations (no action required, but worth noting)

'use client' in pack entry files (stimulus-bundle.js, stores-registration.js) — unusual placement since these are pack entry points, not component files. The commit message explains it correctly: without the directive, RSC auto-bundling would classify them as Server Components and break. The intent is clear; a short inline comment on each file would help future readers who don't have the PR history.

Default build now compiles three bundles — the final commit correctly includes the RSC config in the default webpackConfig.js array so CI/production builds emit rsc-bundle.js. This is the right call, but it adds a non-trivial build-time cost. Worth a note in the README or Procfile.dev comments so that local contributors who don't need RSC can understand how to skip it (RSC_BUNDLE_ONLY watcher aside, there's no NO_RSC_BUNDLE skip gate for the full build).

marked module-scope singletonnew Marked() + marked.use(gfmHeadingId()) at module scope is fine because gfmHeadingId() extension is stateless. Just flagging it was considered.

RouterApp.server.jsx gets 'use client' — the PR description explains this correctly (it's an SSR wrapper, not a React Server Component despite the filename). A one-line comment in the file noting this would prevent future confusion.


Summary

The core implementation is solid and follows the Pro docs pattern faithfully. The four inline comments above cover the notable gaps — the missing webpack entry guard is the most actionable fix before merge; the others are low/nit. No test coverage concerns given this is a demo page with manual QA acceptance criteria.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR wires up the upstream RSCWebpackPlugin (react-on-rails-rsc/WebpackPlugin) to add a React Server Components demo at /server-components, completing the three-part RSC migration series. The implementation covers a new rscWebpackConfig.js derived from the server bundle, RSCWebpackPlugin({ isServer: false }) in the client bundle, 'use client' directives on the nine existing entry-point components, and four new RSC demo components demonstrating the server/client component boundary.

Confidence Score: 5/5

PR is safe to merge; all remaining findings are P2 style improvements.

No P0 or P1 issues found. The webpack loader ordering is correct (right-to-left execution places the RSC loader before babel/swc), RSC bundle output path aligns with the initializer config, and the 'use client' directive placement follows the documented Pro pattern. The two P2 items (mixed-content http scheme and missing clearTimeout in error path) are minor style improvements that do not affect correctness of the demo.

CommentsFeed.jsx — minor: allowedSchemes includes http, and clearTimeout is not called on fetch error paths.

Important Files Changed

Filename Overview
config/webpack/rscWebpackConfig.js New RSC webpack config derived from serverWebpackConfig(true); correctly appends react-on-rails-rsc/WebpackLoader to JS rules (right-to-left execution makes it run before babel/swc), adds react-server resolve condition, and aliases react-dom/server to false. Handles both array-form (Babel) and function-form (SWC) rule.use shapes.
client/app/bundles/server-components/components/CommentsFeed.jsx Async server component fetching /comments.json with AbortController timeout; uses marked+sanitize-html for server-side markdown rendering. Two P2 issues: allowedSchemes includes 'http' (mixed-content risk on HTTPS), and clearTimeout is not called in the error path.
client/app/bundles/server-components/ror_components/ServerComponentsPage.jsx RSC entry component; sync arrow function wrapping async children (ServerInfo, CommentsFeed) with Suspense. Minor: defined as sync arrow function where async function style would match its children and leave room for future top-level awaits.
config/webpack/clientWebpackConfig.js Adds RSCWebpackPlugin({ isServer: false }) to emit react-client-manifest.json; client references scoped to config.source_path matching the server bundle pattern. Clean addition.
config/webpack/webpackConfig.js Adds RSC_BUNDLE_ONLY env gate matching the existing SERVER_BUNDLE_ONLY/CLIENT_BUNDLE_ONLY pattern; default build now includes all three bundles. Clean, consistent change.
config/initializers/react_on_rails_pro.rb Adds enable_rsc_support, rsc_bundle_js_file, and rsc_payload_generation_url_path to ReactOnRailsPro config. Straightforward and aligns with the webpack output path.
config/routes.rb Adds rsc_payload_route helper and GET /server-components route. rsc_payload_route is placed at the top per Pro convention.
app/views/pages/server_components.html.erb Minimal ERB view calling react_component with auto_load_bundle: true for auto-discovery; prerender: false is correct for RSC (server rendering is via Flight protocol, not traditional SSR).

Sequence Diagram

sequenceDiagram
    participant Browser
    participant Rails
    participant NodeRenderer
    participant RSCBundle as RSC Bundle (rsc-bundle.js)
    participant RailsAPI as Rails API (/comments.json)

    Browser->>Rails: GET /server-components
    Rails->>Browser: HTML shell (react_component auto_load_bundle)

    Browser->>Rails: GET /rsc_payload/ServerComponentsPage (Flight)
    Rails->>NodeRenderer: Render RSC payload request
    NodeRenderer->>RSCBundle: Execute ServerComponentsPage
    RSCBundle->>RailsAPI: fetch /comments.json (server-side)
    RailsAPI-->>RSCBundle: JSON comments
    RSCBundle-->>NodeRenderer: React Flight payload (streamed)
    NodeRenderer-->>Rails: Flight stream
    Rails-->>Browser: RSC Flight payload

    Browser->>Browser: Hydrate client components (TogglePanel)
    Note over Browser: ServerInfo + CommentsFeed = pure HTML<br/>TogglePanel = hydrated JS island
Loading

Reviews (1): Last reviewed commit: "Wire RSC loader for both SWC and Babel t..." | Re-trigger Greptile

Comment on lines +88 to +90
},
allowedSchemes: ['https', 'http'],
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 http scheme allows mixed-content images

allowedSchemes includes 'http', so user-submitted markdown with ![img](http://...) will produce <img src="http://..."> in the sanitized HTML. When the app is served over HTTPS (as it will be on any review/production deployment), browsers block those images as mixed-content and display broken image placeholders. Since this is a demo of server-side rendering, broken inline images would undermine the demo. Restricting to ['https'] is the safe default.

Suggested change
},
allowedSchemes: ['https', 'http'],
});
allowedSchemes: ['https'],

Comment on lines +39 to +42
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 5000);
const response = await fetch(`${baseUrl}/comments.json`, { signal: controller.signal });
clearTimeout(timeoutId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 clearTimeout not reached on network errors

clearTimeout(timeoutId) is only called on the happy path. If fetch() rejects for a reason other than the 5-second abort (e.g., a DNS error or connection refusal before the timeout fires), the timeout callback will still fire 5 seconds later and call controller.abort() on a request that has already failed. While harmless in Node.js, the dangling timer can cause confusing noise in logs or slightly delay garbage collection of the controller. A try/finally pattern is cleaner:

const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 5000);
try {
  const response = await fetch(`${baseUrl}/comments.json`, { signal: controller.signal });
  // ...
} finally {
  clearTimeout(timeoutId);
}

import CommentsFeed from '../components/CommentsFeed';
import TogglePanel from '../components/TogglePanel';

const ServerComponentsPage = () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 ServerComponentsPage is a sync arrow function, not async

ServerInfo and CommentsFeed are both async Server Components (they await data), and both are consumed here. React's RSC runtime handles async children via Suspense correctly, so this works. However, ServerComponentsPage being a sync arrow function means any future top-level await added to this component would require a refactor to async function. Declaring it as async function ServerComponentsPage() { ... } (matching the style of its children) would make the intent clear and is consistent with the rest of the RSC tree.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 649e0bdb48

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@@ -1,3 +1,5 @@
'use client';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove 'use client' from server-only router entry

Marking this .server.jsx module as a client component is unsafe because it imports react-router-dom/server, and the client build is now configured to discover 'use client' files across the whole client/app tree via RSCWebpackPlugin (clientReferences with recursive: true). That means this server-only file can be pulled into client reference/manifest processing, which can introduce server-only dependencies into the client pipeline and fail or regress builds depending on resolver behavior.

Useful? React with 👍 / 👎.

ihabadham and others added 2 commits April 24, 2026 23:00
The renderer's stubTimers default of true replaces setTimeout with a
no-op inside the VM. React's RSC server renderer uses setTimeout
internally for Flight-protocol yielding, so stubbing it makes the RSC
stream open without ever emitting a chunk. The request reaches the
worker, the worker holds the accepted socket, but no data flows.
Fastify eventually closes the idle connection at keepAliveTimeout
(~72s), HTTPX retries once by its retries plugin, and Rails sees
HTTPX::Connection::HTTP2::GoawayError after ~144s.

Non-RSC SSR is unaffected because it doesn't rely on setTimeout for
its async scheduling — only RSC's streaming path hits this.

Verified by running a second renderer alongside on another port with
RENDERER_STUB_TIMERS=false: the stuck path returned a full 9.7KB RSC
payload for ServerComponentsPage in 422ms, vs. the default renderer
timing out on the same request.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this flag, console.* calls made inside async Server Components
are captured by the renderer's per-request sharedConsoleHistory but not
replayed back to Rails' logs. Any error-path logging from an async
component (for example, a catch block that console.errors before
returning an error fallback div) disappears, making runtime failures
invisible.

The generator template, RORP spec dummy, and every maintained RSC demo
set this to true for the same reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #729, commit fd9faf1

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review: Pro RSC migration 3/3

This is a well-structured implementation of RSC on webpack. The PR description is thorough and the divergence from Justin's #723 approach (using upstream RSCWebpackPlugin instead of a custom rspack plugin) is the right call for maintainability. A few issues worth addressing before merge:


🔴 Security

ServerInfo.jsx exposes sensitive infrastructure data
os.hostname(), free/total RAM, and Node.js version are exposed in the UI. This is fine for a local demo but it is a real information-disclosure risk if this page is ever deployed without authentication. At minimum, add a comment or a NODE_ENV !== 'production' guard, or wrap the page in an auth check at the route level.

sanitize-html allows http:// for image src
The allowedSchemes: ['https', 'http'] applies to all tags including img. On an HTTPS page this allows mixed-content images and enables trivial tracking pixels embedded via user-submitted markdown. Should restrict image sources to https only (see inline comment).


🟡 Performance / Build time

Default build now compiles 3 bundles
webpackConfig.js now builds client + server + RSC by default. Every developer who runs a plain shakapacker compile (e.g. in CI) now pays for the RSC bundle even if they never touch RSC. The SERVER_BUNDLE_ONLY / CLIENT_BUNDLE_ONLY gates exist precisely to avoid this. Consider whether the default should remain [clientConfig, serverConfig] and only add the RSC bundle when RSC_BUNDLE_ONLY is set or a new INCLUDE_RSC_BUNDLE=yes flag is enabled.


🟡 Code clarity

RouterApp.server.jsx + 'use client'
A file named .server.jsx with 'use client' as its first line will confuse every future contributor who touches it. The PR body explains the reason, but that explanation should live in a comment inside the file itself.

'use client' on webpack pack entry points
stores-registration.js and stimulus-bundle.js are webpack entry points, not React module boundaries. Adding 'use client' there is non-standard usage of the RSC directive. It works because the RSC compiler treats the directive as a module-level client boundary marker, but it is not the intended usage pattern. A comment explaining why this is necessary (i.e., so the RSC plugin marks the entire dependency graph of these packs as client-side) would help future maintainers.


🟢 Missing test coverage

The new server_components controller action has no spec. Given the existing pages_spec.rb only tests the Git SHA display, a minimal request spec confirming the action returns 200 would be a low-effort addition.


Minor

  • ServerInfo is declared async but contains no await. The keyword is harmless but misleading; a plain function is clearer.
  • The hardcoded id: "ServerComponentsPage-react-component-0" in server_components.html.erb is fine for a single-use demo view but will collide if the component is ever rendered more than once on a page.

Overall the webpack config plumbing (rscWebpackConfig.js, the loader wrapper, RSC_BUNDLE_ONLY gate) is clean and the RSC component patterns (server/client boundary with TogglePanel, Suspense around the async data fetch) demonstrate the feature well.


return (
<div className="space-y-3">
{recentComments.map((comment) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allowedSchemes list applies to every permitted attribute including img[src]. Allowing http:// enables mixed-content images on HTTPS pages and makes it trivial to embed tracking pixels via user-submitted markdown. Restrict image sources to https only:

Suggested change
{recentComments.map((comment) => {
allowedSchemes: ['https'],

If http:// must stay for non-image tags, use per-tag scheme overrides:

allowedSchemesByTag: { img: ['https'] },
allowedSchemes: ['https', 'http'],

totalMemory: (os.totalmem() / (1024 * 1024 * 1024)).toFixed(1),
freeMemory: (os.freemem() / (1024 * 1024 * 1024)).toFixed(1),
cpus: os.cpus().length,
hostname: os.hostname(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.hostname() reveals an internal server hostname that an attacker can use to fingerprint or target infrastructure. Combined with Node version, free RAM, and CPU count, this is meaningful information disclosure if the page is ever reachable without authentication.

For a demo that ships to a public review app, consider either gating the component behind Rails.env.development? (passed as a prop from the server), omitting hostname, or adding a note in the view that the page must sit behind auth before production use.

import os from 'os';
import _ from 'lodash';

async function ServerInfo() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServerInfo has no await and therefore doesn't need to be async. Declaring it async causes React to suspend on the returned microtask even though the data is already synchronous, adding a small unnecessary scheduling round-trip.

Suggested change
async function ServerInfo() {
function ServerInfo() {

@@ -1,3 +1,5 @@
'use client';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A file named .server.jsx with 'use client' at the top is a contradiction that will confuse future contributors unfamiliar with this RSC setup. The .server suffix suggests a React Server Component, but this file is actually the SSR wrapper for the React Router client app (it runs in Node via the classic SSR path, not the RSC flight renderer).

Please add a short inline explanation, e.g.:

Suggested change
'use client';
'use client';
// NOTE: Despite the .server.jsx filename this is NOT a React Server Component.
// It is the classic SSR entry for React Router and uses Redux Provider, both
// of which are client-only APIs. 'use client' marks it as a client-reference
// boundary so the RSC compiler keeps it out of the server component graph.

@@ -1,3 +1,5 @@
'use client';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'use client' on a webpack pack entry point is non-standard RSC usage — the directive is designed to mark React module boundaries, not bundle entry points. It works here because the RSC Webpack plugin treats the directive as a signal to keep the entire dependency graph of this module on the client side, but it isn't documented behavior.

A brief comment prevents future maintainers from removing what looks like a misplaced directive:

Suggested change
'use client';
'use client';
// Required so the RSC WebpackPlugin marks this pack and its full dependency
// graph as client-only. Without it, the RSC compiler would attempt to trace
// into ReactOnRails registration code that uses browser APIs.

<%= react_component("ServerComponentsPage",
prerender: false,
auto_load_bundle: true,
trace: Rails.env.development?,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding the element id is fragile — it will silently create duplicate IDs if the helper is ever called more than once (e.g. if this partial is rendered in a layout that includes it twice, or in a test that renders the view multiple times). React on Rails generates a unique ID automatically when none is provided; consider removing this option and letting the helper handle it:

Suggested change
trace: Rails.env.development?,
trace: Rails.env.development?) %>

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd9faf10e7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

return 'http://localhost:3000';
}

throw new Error('RAILS_INTERNAL_URL must be set outside development/test');
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Provide a production fallback for Rails API base URL

resolveRailsBaseUrl throws whenever NODE_ENV is not development/test and RAILS_INTERNAL_URL is unset, so in any production deployment missing that env var the comments section always enters the catch path and never renders data. Because this commit does not add any default/config wiring for RAILS_INTERNAL_URL, /server-components will silently degrade to the error panel instead of showing comments in those environments.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant