Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Procfile.dev
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,6 @@ rails: bundle exec thrust bin/rails server -p 3000
wp-client: RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server
# Server webpack watcher for SSR bundle
wp-server: SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch
# RSC webpack watcher for React Server Components bundle
wp-rsc: RSC_BUNDLE_ONLY=yes bin/shakapacker --watch
node-renderer: NODE_ENV=development node renderer/node-renderer.js
2 changes: 2 additions & 0 deletions app/controllers/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def simple; end

def rescript; end

def server_components; end

private

def set_comments
Expand Down
5 changes: 5 additions & 0 deletions app/views/pages/server_components.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<%= 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?) %>

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?) %>

Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

import React from 'react';
import PropTypes from 'prop-types';
import BaseComponent from 'libs/components/BaseComponent';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,14 @@ function NavigationBar(props) {
Rescript
</a>
</li>
<li>
<a
className={navItemClassName(pathname === paths.SERVER_COMPONENTS_PATH)}
href={paths.SERVER_COMPONENTS_PATH}
>
RSC Demo
</a>
</li>
<li>
<a
className={navItemClassName(false)}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

// eslint-disable-next-line max-classes-per-file
import React from 'react';
import request from 'axios';
Expand Down
1 change: 1 addition & 0 deletions client/app/bundles/comments/constants/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export const RESCRIPT_PATH = '/rescript';
export const SIMPLE_REACT_PATH = '/simple';
export const STIMULUS_PATH = '/stimulus';
export const RAILS_PATH = '/comments';
export const SERVER_COMPONENTS_PATH = '/server-components';
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

// Wrapper for ReScript component to work with react_on_rails auto-registration
// react_on_rails looks for components in ror_components/ subdirectories

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

import { Provider } from 'react-redux';
import React from 'react';
import ReactOnRails from 'react-on-rails-pro';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

// Top level component for client side.
// Compare this to the ./ServerApp.jsx file which is used for server side rendering.

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use client';

// Compare to ./RouterApp.server.jsx
import { Provider } from 'react-redux';
import React from 'react';
Expand Down
Original file line number Diff line number Diff line change
@@ -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 👍 / 👎.

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.


// Compare to ./RouterApp.client.jsx
import { Provider } from 'react-redux';
import React from 'react';
Expand Down
129 changes: 129 additions & 0 deletions client/app/bundles/server-components/components/CommentsFeed.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
// Server Component - fetches comments directly from the Rails API on the server.
// Uses marked for markdown rendering. Both fetch and marked stay server-side.

import React from 'react';
import { Marked } from 'marked';
import { gfmHeadingId } from 'marked-gfm-heading-id';
import sanitizeHtml from 'sanitize-html';
import _ from 'lodash';
import TogglePanel from './TogglePanel';

const marked = new Marked();
marked.use(gfmHeadingId());

function resolveRailsBaseUrl() {
if (process.env.RAILS_INTERNAL_URL) {
return process.env.RAILS_INTERNAL_URL;
}

// Local defaults are okay in development/test, but production should be explicit.
if (process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') {
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 👍 / 👎.

}

async function CommentsFeed() {
// Simulate network latency only when explicitly enabled for demos.
if (process.env.RSC_SUSPENSE_DEMO_DELAY === 'true') {
await new Promise((resolve) => {
setTimeout(resolve, 800);
});
}

let recentComments = [];
try {
// Fetch comments directly from the Rails API — no client-side fetch needed
const baseUrl = resolveRailsBaseUrl();
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 5000);
const response = await fetch(`${baseUrl}/comments.json`, { signal: controller.signal });
clearTimeout(timeoutId);
Comment on lines +39 to +42
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);
}

if (!response.ok) {
throw new Error(`Failed to fetch comments: ${response.status} ${response.statusText}`);
}
const data = await response.json();
const comments = data.comments;

// Use lodash to process (stays on server)
const sortedComments = _.orderBy(comments, ['created_at'], ['desc']);
recentComments = _.take(sortedComments, 10);
} catch (error) {
// eslint-disable-next-line no-console
console.error('CommentsFeed failed to load comments', error);
return (
<div className="bg-rose-50 border border-rose-200 rounded-lg p-6 text-center">
<p className="text-rose-700">Could not load comments right now. Please try again later.</p>
</div>
);
}

if (recentComments.length === 0) {
return (
<div className="bg-amber-50 border border-amber-200 rounded-lg p-6 text-center">
<p className="text-amber-700">
No comments yet. Add some comments from the{' '}
<a href="/" className="underline font-medium">
home page
</a>{' '}
to see them rendered here by server components.
</p>
</div>
);
}

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'],

// Render markdown on the server using marked + sanitize-html.
// sanitize-html strips any dangerous HTML before rendering.
// These libraries (combined ~200KB) never reach the client.
const rawHtml = marked.parse(comment.text || '');
const safeHtml = sanitizeHtml(rawHtml, {
allowedTags: sanitizeHtml.defaults.allowedTags.concat(['img']),
allowedAttributes: {
...sanitizeHtml.defaults.allowedAttributes,
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'],

Comment on lines +88 to +90
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'],


return (
<div
key={comment.id}
className="bg-white border border-slate-200 rounded-lg p-4 shadow-sm hover:shadow-md transition-shadow"
>
<div className="flex items-center justify-between mb-2">
<span className="font-semibold text-slate-800">{comment.author}</span>
<span className="text-xs text-slate-400">
{new Date(comment.created_at).toLocaleDateString('en-US', {
month: 'short',
day: 'numeric',
year: 'numeric',
hour: '2-digit',
minute: '2-digit',
})}
</span>
</div>
<TogglePanel title="Show rendered markdown">
{/* Content is sanitized via sanitize-html before rendering */}
{/* eslint-disable-next-line react/no-danger */}
<div
className="prose prose-sm prose-slate max-w-none"
dangerouslySetInnerHTML={{ __html: safeHtml }}
/>
</TogglePanel>
<p className="text-slate-600 text-sm mt-1">{comment.text}</p>
</div>
);
})}
<p className="text-xs text-slate-400 text-center pt-2">
{recentComments.length} comment{recentComments.length !== 1 ? 's' : ''} rendered on the server using{' '}
<code>marked</code> + <code>sanitize-html</code> (never sent to browser)
</p>
</div>
);
}

export default CommentsFeed;
58 changes: 58 additions & 0 deletions client/app/bundles/server-components/components/ServerInfo.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Server Component - uses Node.js os module, which only exists on the server.
// This component and its dependencies are never sent to the browser.

import React from 'react';
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() {

const serverInfo = {
platform: os.platform(),
arch: os.arch(),
nodeVersion: process.version,
uptime: Math.floor(os.uptime() / 3600),
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)',

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.

};

// Using lodash on the server — this 70KB+ library stays server-side
const infoEntries = _.toPairs(serverInfo);
const grouped = _.chunk(infoEntries, 4);

const labels = {
platform: 'Platform',
arch: 'Architecture',
nodeVersion: 'Node.js',
uptime: 'Uptime (hrs)',
totalMemory: 'Total RAM (GB)',
freeMemory: 'Free RAM (GB)',
cpus: 'CPU Cores',
hostname: 'Hostname',
};

return (
<div className="bg-gradient-to-br from-emerald-50 to-teal-50 border border-emerald-200 rounded-xl p-6">
<p className="text-xs text-emerald-600 mb-4 font-medium">
This data comes from the Node.js <code className="bg-emerald-100 px-1 rounded">os</code> module
— it runs only on the server. The <code className="bg-emerald-100 px-1 rounded">lodash</code> library
used to format it never reaches the browser.
</p>
<div className="grid md:grid-cols-2 gap-x-8 gap-y-1">
{grouped.map((group) => (
<div key={group.map(([k]) => k).join('-')} className="space-y-1">
{group.map(([key, value]) => (
<div key={key} className="flex justify-between py-1.5 border-b border-emerald-100 last:border-0">
<span className="text-sm text-emerald-700 font-medium">{labels[key] || key}</span>
<span className="text-sm text-emerald-900 font-mono">{value}</span>
</div>
))}
</div>
))}
</div>
</div>
);
}

export default ServerInfo;
40 changes: 40 additions & 0 deletions client/app/bundles/server-components/components/TogglePanel.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use client';

import React, { useState } from 'react';
import PropTypes from 'prop-types';

const TogglePanel = ({ title, children }) => {
const [isOpen, setIsOpen] = useState(false);

return (
<div className="border border-slate-200 rounded-lg overflow-hidden">
<button
type="button"
onClick={() => setIsOpen((prev) => !prev)}
className="w-full flex items-center justify-between px-4 py-2.5 bg-slate-50 hover:bg-slate-100 transition-colors text-left"
>
<span className="text-sm font-medium text-slate-700">{title}</span>
<svg
className={`w-4 h-4 text-slate-400 transition-transform ${isOpen ? 'rotate-180' : ''}`}
fill="none"
viewBox="0 0 24 24"
stroke="currentColor"
>
<path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M19 9l-7 7-7-7" />
</svg>
</button>
{isOpen && (
<div className="px-4 py-3 bg-white">
{children}
</div>
)}
</div>
);
};

TogglePanel.propTypes = {
title: PropTypes.string.isRequired,
children: PropTypes.node.isRequired,
};

export default TogglePanel;
Loading
Loading