[DO NOT MERGE] Pro RSC migration: stacked base PR (tracks sub-PRs)#727
[DO NOT MERGE] Pro RSC migration: stacked base PR (tracks sub-PRs)#727
Conversation
First of three stacked sub-PRs enabling React Server Components in this tutorial, since `enable_rsc_support` is a Pro-only configuration. Pure dependency swap — ExecJS server rendering and existing behavior unchanged. - Gemfile: `react_on_rails 16.6.0.rc.0` → `react_on_rails_pro 16.6.0` - package.json: `react-on-rails 16.6.0-rc.0` → `react-on-rails-pro 16.6.0` - shakapacker: `10.0.0.rc.0` → `10.0.0` stable (lockstep with the Pro gem) - First-party imports: `'react-on-rails'` → `'react-on-rails-pro'` across 10 files (matches `react_on_rails :pro` generator's `update_imports_to_pro_package`) - `config/webpack/commonWebpackConfig.js`: alias `'react-on-rails$'` → `'react-on-rails-pro'` as a third-party shim. Without it, `rescript-react-on-rails`'s `import ReactOnRails from "react-on-rails"` resolves to core, core and Pro coexist in the bundle, and SSR fails with the ExecJS error `Cannot mix react-on-rails (core) with react-on-rails-pro` (verified by repro). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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:
|
Code Review — Sub-PR 1: Dependency Swap (react-on-rails → react-on-rails-pro)OverviewThis PR is the integration base branch for the React on Rails Pro + RSC migration. The diff from Sub-PR 1 (#726) performs a clean dependency swap: ✅ What's done well
|
| // needed for the third-party exact-specifier case — hence the `$` suffix (no | ||
| // sub-path rewrites, since Pro's package exports differ from core's). | ||
| alias: { | ||
| 'react-on-rails$': 'react-on-rails-pro', |
There was a problem hiding this comment.
The $-suffixed alias correctly avoids rewriting sub-path imports (e.g. react-on-rails/node_package/…), but it carries an implicit assumption: that every API rescript-react-on-rails calls at the exact-specifier level is present and compatible in react-on-rails-pro. If that has been verified against the actual call sites in rescript-react-on-rails, a short note to that effect here (or in the PR description) would save a future maintainer from re-investigating when something mysteriously breaks under RSC.
| rails (>= 5.2) | ||
| rainbow (~> 3.0) | ||
| shakapacker (>= 6.0) | ||
| react_on_rails_pro (16.6.0) |
There was a problem hiding this comment.
Note that react_on_rails_pro depends on react_on_rails (= 16.6.0) (line 335 below), so both gems are present in the final bundle — core as a transitive dep, Pro as the direct dep. This is intentional (Pro wraps Core), but developers who grep Gemfile.lock for react_on_rails will find two entries. A brief comment in Gemfile above the react_on_rails_pro declaration would prevent confusion.
| actionview (>= 5.0.0) | ||
| activesupport (>= 5.0.0) | ||
| json (2.19.1) | ||
| jwt (2.10.2) |
There was a problem hiding this comment.
jwt is used by Pro for license validation and NodeRenderer JWT authentication. Ensure the REACT_ON_RAILS_PRO_LICENSE env var is present in all CI environments and in .env.example (the PR description says this is Sub-PR 2's responsibility, but CI runs against this base branch first — the secret needs to be in place before this branch's CI suite is exercised).
…#728) * Switch SSR to the Pro Node renderer on webpack Second of three stacked sub-PRs in the Pro RSC migration. Routes all server rendering through the Pro Node renderer (port 3800) instead of ExecJS, and flips the asset bundler from rspack to webpack — scoped reversal of #702, needed because rspack 2.0.0-beta.7's webpack compatibility layer doesn't cover the APIs upstream RSCWebpackPlugin requires. We flip back to rspack once shakacode/react_on_rails_rsc#29 ships a native rspack RSC plugin. The bundler flip and NodeRenderer wiring ship atomically: the server bundle produced by the Pro webpack transforms (target: 'node' + libraryTarget: 'commonjs2') is not evaluable by ExecJS, so the initializer pointing server_renderer at the NodeRenderer must land at the same time. Key changes: - config/shakapacker.yml: assets_bundler: rspack → webpack - config/webpack/bundlerUtils.js: return @rspack/core or webpack based on the shakapacker setting (was rspack-only and threw otherwise); spec updated in parallel - config/webpack/serverWebpackConfig.js: Pro transforms per the :pro generator's update_webpack_config_for_pro and the marketplace/dummy references — target: 'node' + node: false, libraryTarget: 'commonjs2', extractLoader helper, babelLoader.options.caller = { ssr: true }, destructured module.exports so Sub-PR 3's rscWebpackConfig.js can derive from serverWebpackConfig(true). RSCWebpackPlugin({ isServer: true }) when !rscBundle emits the server manifest; inert until Sub-PR 3 activates RSC support - config/initializers/react_on_rails_pro.rb: NodeRenderer-only config (no RSC fields — those move in Sub-PR 3) - renderer/node-renderer.js: launcher with strict integer env parsing, CI worker cap, and additionalContext: { URL, AbortController } so react-router-dom's NavLink.encodeLocation does not throw "ReferenceError: URL is not defined" at SSR time - Procfile.dev: renderer: NODE_ENV=development node renderer/node-renderer.js - package.json: react-on-rails-pro-node-renderer 16.6.0 and react-on-rails-rsc ^19.0.4 (Pro peer dep; required for the RSCWebpackPlugin import) - .gitignore: /renderer/.node-renderer-bundles/ - .env.example: REACT_ON_RAILS_PRO_LICENSE, RENDERER_PASSWORD, and REACT_RENDERER_URL with dev vs prod guidance - .github/workflows/rspec_test.yml: start the Node renderer before rspec with PID liveness and port-ready checks plus log capture on failure Verified locally: webpack build compiles cleanly. `bin/rails s` on 3000 with `node renderer/node-renderer.js` on 3800 renders GET / at HTTP 200; Rails log shows "Node Renderer responded" and the renderer log emits "[SERVER] RENDERED Footer to dom node with id: ..." — confirming SSR went through the Pro path rather than falling back to ExecJS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Tighten Pro webpack + initializer setup from reference audit Four fixes from auditing Sub-PR 2 against the Pro dummy, the :pro generator, and the Pro configuration docs. - config/initializers/react_on_rails_pro.rb: renderer_password now raises in non-local envs instead of falling back to the dev string. Previously any production deploy that forgot to set the env var would silently run with a known-public password; now it fails loudly. Matches the safer pattern from PR #723's final state. - config/webpack/serverWebpackConfig.js: pass clientReferences to RSCWebpackPlugin({ isServer: true }), matching the Pro dummy's serverWebpackConfig at `react_on_rails_pro/spec/dummy/config/webpack/ serverWebpackConfig.js`. Without it, the plugin may walk into node_modules and hit unlodaed .tsx source files and re-scan modules we don't need. Locks client-ref discovery to client/app/**. - config/webpack/serverWebpackConfig.js: drop publicPath from the server-bundle output. Server bundles are loaded by the Node renderer via the filesystem, never served over HTTP — the URL is unused. Matches the Pro dummy's comment. - package.json: pin react-on-rails-rsc to 19.0.4 stable (was ^19.0.4 range) and add "node-renderer" npm script as a convenience shortcut for `node renderer/node-renderer.js`. - .github/workflows/rspec_test.yml: set RENDERER_PASSWORD explicitly in CI to the shared dev default so both the initializer and the launcher use the same concrete value, avoiding silent drift if either side's default is ever touched. Re-verified: webpack build clean; renderer + Rails boot; GET / returns HTTP 200 with "Node Renderer responded" in the Rails log and "[SERVER] RENDERED" in the renderer log, confirming SSR still goes through the Pro path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Align renderer_password initializer with Pro docs + source Previous commit added Rails.env-branching + explicit raise in non-local envs. That duplicates what Pro's configuration.rb already does at boot (validate_renderer_password_for_production) and diverges from the documented pattern in pro/node-renderer.md and pro/installation.md. Revert to the simple ENV.fetch with a dev default. Pro handles the prod-enforcement itself — if RENDERER_PASSWORD is unset in a production-like env, Pro raises at boot with a helpful error message pointing at exactly this fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Pin react and react-dom to 19.0.4 Journey plan's Sub-PR 1 KEEP table explicitly called for the 19.0.4 pin from PR #723's final state — missed in the Sub-PR 1 migration commit. 19.0.4 is the minimum React version documented as required for React Server Components (per the Pro RSC tutorial doc, CVE-safe floor). Tightens from the inherited "^19.0.0" range. Boot-verified: webpack build clean; renderer + Rails boot; GET / returns HTTP 200 with "Node Renderer responded" in Rails log and "[SERVER] RENDERED" in renderer log — SSR path unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Rename Procfile.dev renderer process to node-renderer Match the Pro dummy's Procfile.dev and PR #723's naming. Marketplace uses `renderer:`, but the dummy is the canonical Pro reference and aligns with how the :pro generator's `add_pro_to_procfile` names it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Default renderer logLevel to 'info' Pro docs (docs/oss/building-features/node-renderer/container-deployment.md) prescribe `logLevel: 'info'` as the general-renderer-logs default, and the Pro dummy's renderer/node-renderer.js also uses 'info'. The inherited 'debug' default is too verbose for normal operation (emits the full VM context setup messages like "Adding Buffer, TextDecoder, ..."). Debug remains reachable via RENDERER_LOG_LEVEL=debug env var as docs describe for active debugging (docs/oss/building-features/node-renderer/ debugging.md). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Trim over-verbose comments across Sub-PR 2 files Per the repo's CLAUDE.md guidance ("default to writing no comments; only when the WHY is non-obvious"), stripping comments that explain WHAT the code does or reference the current-PR context. Kept (non-obvious WHYs): the `additionalContext: { URL, AbortController }` reason (react-router-dom NavLink url-is-not-defined), the CI worker-count cap reason, `clientReferences` scoping rationale, `libraryTarget: 'commonjs2'` requirement for the renderer, `target: 'node'` fix for SSR-breaking libs, the renderer-fallback-disabled reason, the renderer-password-must-match pointer, the bundler-utils dual-support blurb, and the shakapacker.yml tactical-reversal tag. Dropped (WHAT explanations, redundant with identifiers): JSDoc blocks describing `configureServer` and `extractLoader`, per-config-key descriptions in the renderer launcher, Procfile.dev process-purpose comment, verbose `.env.example` prose, initializer multi-line explanations of each field. No code changes — comments only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Restore license setup guidance in initializer comments Over-trimmed in the previous cleanup commit. The longer wording has non-obvious info (where to get the JWT, which envs skip license, what Pro does when no license is set) that readers genuinely benefit from — beyond what the generator template's one-line version captures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Wire the Pro Node renderer for Control Plane deploys Sub-PR 2 already wires dev (Procfile.dev) and CI (rspec_test.yml). Without this commit, merging base → master with the renderer active on the Rails side but no renderer process in the deployed container would break the live tutorial (Rails tries to hit localhost:3800, nothing listens, HTTPX::ConnectionError → 500 with renderer_use_fallback_exec_js = false). Matches the react-server-components-marketplace-demo deploy pattern (Option 1 "Single Container" from docs/oss/building-features/ node-renderer/container-deployment.md): - .controlplane/Dockerfile: CMD starts the Node renderer and Rails in the same container with `wait -n ; exit 1` so any child exit restarts the whole workload. Preserves the existing Thruster HTTP/2 proxy for Rails — the only deviation from marketplace's literal CMD. - .controlplane/templates/app.yml: add RENDERER_PORT, RENDERER_LOG_LEVEL, RENDERER_WORKERS_COUNT, RENDERER_URL as plain env, plus RENDERER_PASSWORD and REACT_ON_RAILS_PRO_LICENSE as Control Plane secret references keyed by {{APP_NAME}}-secrets (matches the repo's existing {{APP_NAME}} templating convention in DATABASE_URL / REDIS_URL). - .controlplane/templates/rails.yml: bump memory 512Mi → 2Gi. The container now runs two processes; 512Mi OOMs fast. 2Gi matches the marketplace demo's rails.yml. - config/initializers/react_on_rails_pro.rb: read ENV["RENDERER_URL"] instead of ENV["REACT_RENDERER_URL"]. Aligns with docs/oss/ configuration/configuration-pro.md (which uses RENDERER_URL), the Pro dummy's initializer, and the marketplace initializer — all of which use RENDERER_URL without a REACT_ prefix. The REACT_ prefix was inherited from PR #723 and is non-canonical. - .env.example: matching REACT_RENDERER_URL → RENDERER_URL rename. Prerequisite before first deploy (outside PR scope, one-time per app per org, performed manually via cpln or the Control Plane UI): create a Secret named `<app-name>-secrets` with keys RENDERER_PASSWORD (strong random) and REACT_ON_RAILS_PRO_LICENSE (JWT from pro.reactonrails.com). Affects react-webpack-rails-tutorial-production, react-webpack-rails-tutorial-staging, and any qa-* review apps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use {{APP_SECRETS}} for renderer + license secret references {{APP_NAME}}-secrets expanded to a per-app secret name, which would require a new Control Plane Secret for every review app PR — wrong per cpflow's own conventions. cpflow exposes {{APP_SECRETS}} (lib/core/template_parser.rb:49, lib/core/config.rb:51-52) which expands to `{APP_PREFIX}-secrets`. Per our controlplane.yml, APP_PREFIX is: - `react-webpack-rails-tutorial-production` for the prod app - `react-webpack-rails-tutorial-staging` for the staging app - `qa-react-webpack-rails-tutorial` for all qa-* review apps (because match_if_app_name_starts_with: true) So review apps all share `qa-react-webpack-rails-tutorial-secrets` instead of each PR needing its own. Three secrets total across two orgs instead of one per PR. Matches the `{APP_PREFIX}-secrets` default documented at shakacode/control-plane-flow/docs/secrets-and-env-values.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix env var name mismatch: NODE_RENDERER_CONCURRENCY → RENDERER_WORKERS_COUNT The launcher was reading NODE_RENDERER_CONCURRENCY (inherited from PR #723's f55dcc2), but app.yml sets RENDERER_WORKERS_COUNT (canonical per marketplace + Pro renderer library). Result: prod/staging would ignore the deployed value and always use the launcher default. RENDERER_WORKERS_COUNT is the canonical env var name per: - docs/oss/building-features/node-renderer/js-configuration.md "workersCount (default: process.env.RENDERER_WORKERS_COUNT ...)" - packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts:200 "workersCount: env.RENDERER_WORKERS_COUNT ? parseInt(...) : defaultWorkersCount()" - react-server-components-marketplace-demo/node-renderer.js NODE_RENDERER_CONCURRENCY was a non-canonical name invented by PR #723. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Right-size rails workload: 1Gi + capacityAI Previous commit bumped memory 512Mi → 2Gi matching the marketplace demo, but cherry-picked only the memory dimension: CPU stayed at 300m and capacityAI stayed false. Net result was a wasteful fixed 2Gi allocation with no autoscale — the pattern Justin flagged. Better right-size for this tutorial (smaller Rails surface than the marketplace RSC demo): - memory: 2Gi → 1Gi. Enough headroom for Rails + the Pro Node renderer in one container without reserving capacity that won't be used. - capacityAI: false → true. Adjusts CPU/memory within the single replica based on observed usage, so the 1Gi/300m baseline grows if actual workload warrants it. Matches the marketplace demo's capacityAI posture without copying its oversized static baseline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Replace org.yml placeholder with renderer + license schema Document the actual keys the qa-* dictionary needs (RENDERER_PASSWORD, REACT_ON_RAILS_PRO_LICENSE) instead of the unused SOME_ENV placeholder, and warn against re-applying the template after real values are populated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix stale Rspack comments in Procfile.dev The rspack→webpack bundler flip in this PR left two comment lines referring to Rspack that now misdescribe the active bundler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Treat blank RENDERER_PASSWORD as unset in Rails initializer ENV.fetch returns "" when the env var is set to empty string, while the JS renderer's `process.env.RENDERER_PASSWORD || fallback` treats "" as falsy. Result: copying .env.example verbatim (ships with a blank RENDERER_PASSWORD= line) leaves Rails sending "" in the auth header while the renderer expects the dev default, silently failing SSR auth. Switch to ENV["RENDERER_PASSWORD"].presence || default so blank values route to the fallback on both sides. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document RENDERER_PORT, LOG_LEVEL, WORKERS_COUNT in .env.example app.yml and the renderer launcher read three additional env vars that .env.example didn't mention, leaving developers with no single place to see which renderer knobs exist. Add them commented-out with their defaults and brief descriptions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Derive RSCWebpackPlugin clientReferences from config.source_path Previously hardcoded path.resolve(__dirname, '../../client/app'), which would silently point at the wrong directory if shakapacker.yml's source_path were ever changed. The same file already derives the server-bundle entry's path from config.source_path (line 33); apply the same pattern here so the two stay in sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Dump Node renderer log on CI rspec failure The renderer startup step only cats /tmp/node-renderer.log when the startup wait loop times out (30s). If the renderer starts fine but crashes mid-test, the log is never surfaced, making the rspec failure impossible to debug from the Actions UI. Add an `if: failure()` step after rspec that cats the log so any crash during the test run is visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Forward SIGTERM to children in Dockerfile CMD Bash as PID 1 doesn't forward signals to its children by default, so Control Plane's rolling-deploy SIGTERM never reaches Puma or the Node renderer's cluster manager. Both handle SIGTERM automatically (drain in-flight requests, stop accepting new ones), but only if they receive the signal. Without a trap, the graceful period expires and SIGKILL drops in-flight requests on every rolling deploy. Add `trap 'kill -TERM $RENDERER_PID $RAILS_PID' TERM INT` before `wait -n`. Uses the PID captures that were previously assigned but unused, turning dead code into a real graceful-shutdown mechanism aligned with the graceful-shutdown section of container-deployment.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "Forward SIGTERM to children in Dockerfile CMD" This reverts commit 1bd5e86. The trap addition was incomplete. On the shutdown signal (Control Plane sends SIGINT per docs.controlplane.com/reference/workload/termination.md), the trap fires, `wait -n` returns 130, and `exit 1` then runs unconditionally. The container exits with code 1 on every rolling deploy instead of a code that reflects the signal-initiated shutdown. More importantly, the trap was trying to solve a problem Control Plane already handles. CP's default preStop hook runs `sh -c "sleep 45"` before any signal reaches PID 1, and the sidecar stops accepting new inbound connections ~80 seconds ahead of termination. That sleep plus the routing drain is the graceful-shutdown window; signal forwarding inside the container is marginal on top of it. Match the reference deployments verbatim: the react-on-rails-demo- marketplace-rsc Dockerfile and the hichee production Dockerfile both use this exact bash-c pattern without a trap. If deeper graceful shutdown is ever needed, the right tool is extending preStop or switching to a process manager (overmind, tini), not a bash trap on wait -n. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Guard against dev-default RENDERER_PASSWORD in production The Pro renderer's JS side raises if RENDERER_PASSWORD is unset in production, but accepts the literal "local-dev-renderer-password" value. A CP secret misconfigured to that string (easy to happen when copying from .env.example) would let both sides "match" while running with no real authentication. Split the branch by Rails.env.local?: - dev/test keeps the .presence || default fallback so blank env vars still work for local development - production fetches strict and raises if blank, unset, or the literal dev default Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fold CI workers cap into parseIntegerEnv default The post-hoc `if (process.env.CI && env == null) config.workersCount = 2` block mutated an already-constructed config and used a narrower definition of "unset" (`== null`) than parseIntegerEnv's own check (treats "" as unset too). Folding the CI default into the second argument of parseIntegerEnv: workersCount: parseIntegerEnv('RENDERER_WORKERS_COUNT', process.env.CI ? 2 : 3, { min: 0 }) keeps the same behaviour for explicit values, uses one definition of "unset" consistently, and drops the mutation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document rscBundle parameter's Sub-PR 3 role The parameter is unused until Sub-PR 3 wires rscWebpackConfig.js to call configureServer(true). Adding a short comment so it isn't mistaken for dead code during review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "Document rscBundle parameter's Sub-PR 3 role" This reverts commit a7734472b8e9e2bf80088a85ef0b7b9dd76b44a0. The `rscBundle = false` parameter shape follows the documented Pro pattern: docs/oss/migrating/rsc-preparing-app.md:244 and docs/pro/react-server-components/upgrading-existing-pro-app.md:106 both name it by parameter name and show the exact `if (!rscBundle) { ... RSCWebpackPlugin ... }` guard. The Pro dummy (react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js) uses the same shape with no inline comment. The reverted commit restated what the docs already cover and added a Sub-PR 3 reference that would rot once Sub-PR 3 merges. Code follows documented conventions; readers who need context can read the doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Loosen react pin from exact 19.0.4 to ~19.0.4 The exact pin locked out every 19.0.x patch including 19.0.5 and future security patches within the 19.0 line. Pro's own install docs (docs/pro/react-server-components/upgrading-existing-pro-app.md:26-28 and create-without-ssr.md:37) prescribe `react@~19.0.4` — tilde range that keeps the CVE floor while allowing 19.0.x patches. react-on-rails-rsc@19.0.4's peer dep is `react: ^19.0.3`, so 19.0.5 satisfies it. After this change, yarn resolves react to 19.0.5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Trim renderer_password initializer comment The previous comment restated in English what the code below already expressed. Cut to the one non-obvious WHY (why the prod branch exists despite Pro's own JS-side guard). Method names + the raise message cover the rest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Align CI renderer log handling with Pro CI Pro's own integration CI (react_on_rails_pro/.github/workflows/ pro-integration-tests.yml) runs the renderer with `pnpm run node-renderer &` — no log redirect — so renderer output interleaves with job stdout and is always visible without a special dump step. Our workflow inherited a redirect-to-/tmp/node-renderer.log pattern from PR #723 plus a follow-up if-failure cat step (commit 7dea094) that dumped the file after rspec. Both existed to work around the redirect; neither was in any reference. Drop the redirect and the associated cat calls (startup-failure cat, timeout cat, post-rspec failure dump). Renderer logs now appear inline with job output, same shape as Pro CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Reframe shakapacker bundler note as TODO "Tactical:" read as a weird label; "TODO:" is the standard signal that the current state is meant to be reverted once the upstream blocker (shakacode/react_on_rails_rsc#29) ships. Same content, leads with the action. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Remove RENDERER_PASSWORD prod-default guard The guard added in 0eb94af raised at every Rails boot in production, which breaks the Docker build step that runs `bin/rails react_on_rails:locale` during image bake (RENDERER_PASSWORD isn't in the build environment — secrets are mounted at runtime only, not at build time). Deploy on Control Plane fails before it can even produce an image. Reference check on the guard: zero of Pro dummy, marketplace demo (react-on-rails-demo-marketplace-rsc), Justin's PR 723, and hichee have a "raise on literal dev-default value" guard. Each uses ENV.fetch with a default or hardcodes the password. The guard was a reviewer-driven divergence with no reference backing. Revert the prod branch. Keep the .presence fallback from 7254b1a (that one responds to our specific .env.example shape, not a reference divergence). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — PR #727 (Pro RSC Migration: Stacked Base)This is a thorough, well-documented migration. The inline rationale throughout is a strong signal that the tradeoffs (webpack-over-rspack temporarily, React version pin, single-container deployment) were thought through deliberately. Below is a full pass with a few items worth resolving before the stack lands on OverviewThe PR correctly splits work across three sub-PRs and integrates them here:
The overall approach aligns with canonical Pro references ( Issues1.
|
| const babelLoader = extractLoader(rule, 'babel-loader'); | ||
| if (babelLoader) { | ||
| babelLoader.options = babelLoader.options || {}; | ||
| babelLoader.options.caller = { ssr: true }; |
There was a problem hiding this comment.
This fully replaces caller rather than merging into it. If babel-loader was configured upstream with other caller fields (e.g. { name: 'webpack', supportsStaticESM: true }), those are silently dropped.
| babelLoader.options.caller = { ssr: true }; | |
| babelLoader.options.caller = { ...babelLoader.options.caller, ssr: true }; |
| } | ||
|
|
||
| _cachedBundler = require('@rspack/core'); | ||
| _cachedBundler = config.assets_bundler === 'rspack' ? require('@rspack/core') : require('webpack'); |
There was a problem hiding this comment.
This silently falls back to webpack for any non-'rspack' value — including typos or genuinely invalid strings. The previous code threw an explicit error for invalid values; that test was removed without a replacement.
This is defensible as intentional (webpack is the desired fallback while rspack support is paused), but consider either documenting it explicitly or adding a warning log for unexpected values to make misconfiguration easier to detect:
| _cachedBundler = config.assets_bundler === 'rspack' ? require('@rspack/core') : require('webpack'); | |
| _cachedBundler = | |
| config.assets_bundler === 'rspack' | |
| ? require('@rspack/core') | |
| : (() => { | |
| if (config.assets_bundler && config.assets_bundler !== 'webpack') { | |
| console.warn( | |
| `[bundlerUtils] Unknown assets_bundler "${config.assets_bundler}", falling back to webpack.`, | |
| ); | |
| } | |
| return require('webpack'); | |
| })(); |
| # docs/oss/building-features/node-renderer/container-deployment.md). | ||
| # `wait -n` exits on first child failure → container exits → Control Plane | ||
| # restarts the whole unit. Keeps the existing Thruster HTTP/2 proxy for Rails. | ||
| CMD ["bash", "-c", "node renderer/node-renderer.js & RENDERER_PID=$! ; bundle exec thrust bin/rails server & RAILS_PID=$! ; wait -n ; exit 1"] |
There was a problem hiding this comment.
wait -n (wait for the first child to exit) requires bash ≥ 4.3. Debian/Ubuntu base images ship bash 5.x so this should be fine, but worth a quick bash --version sanity check against the actual base image.
Also, RENDERER_PID and RAILS_PID are assigned but never used again in this one-liner — they can be dropped without any behavioral change:
| CMD ["bash", "-c", "node renderer/node-renderer.js & RENDERER_PID=$! ; bundle exec thrust bin/rails server & RAILS_PID=$! ; wait -n ; exit 1"] | |
| CMD ["bash", "-c", "node renderer/node-renderer.js & bundle exec thrust bin/rails server & wait -n ; exit 1"] |
| // Expose globals the VM sandbox doesn't auto-provide but that downstream | ||
| // deps rely on during SSR. Without URL, react-router-dom's NavLink throws | ||
| // `ReferenceError: URL is not defined` via encodeLocation. | ||
| additionalContext: { URL, AbortController }, |
There was a problem hiding this comment.
URL and AbortController are Node.js globals as of Node 18 (both) and Node 15 (AbortController). If there's any chance of Node 16 being in use, AbortController would be undefined and reactOnRailsProNodeRenderer would receive undefined silently — or throw a confusing error at SSR time rather than at startup.
Since the project already requires modern Node for React 19 / RSC, consider making the minimum Node version requirement explicit — either via engines in package.json or an upfront check here:
| additionalContext: { URL, AbortController }, | |
| additionalContext: { URL, AbortController }, |
(No code change needed if Node 18+ is already enforced elsewhere — just make sure it's documented.)
This is the base/integration branch for the React on Rails Pro + React Server Components migration. Sub-PRs squash-merge into this branch as they're reviewed; only when all sub-PRs have landed and the full migration is complete does this PR merge to
masteras one atomic unit.Keep this PR open as Draft until the stack is complete. Please do not auto-merge.
Goal
Migrate this tutorial to React on Rails Pro so React Server Components can be demoed here.
enable_rsc_supportis Pro-only, so demoing RSC inherently pulls Pro into the tutorial app-wide — this is the entry ticket for the RSC demo, not scope drift.Supersedes PR #723
PR #723 bundled the full migration (gem swap + NodeRenderer + RSC demo + a custom Rspack RSC plugin) in a single branch. This stacked series splits that work into focused, independently-reviewable sub-PRs and realigns several choices with the canonical Pro references (
react_on_rails_pro/spec/dummy,react-server-components-marketplace-demo).Sub-PR plan
rescript-react-on-rails. Pure dependency swap — ExecJS unchanged.server_renderer = "NodeRenderer", renderer URL/password, etc.),renderer/node-renderer.jslauncher,Procfile.deventry, and the Pro webpack transforms from the:progenerator'supdate_webpack_config_for_pro(target: 'node',node: false,extractLoaderhelper, Babel SSR caller,libraryTarget: 'commonjs2').shakapacker.ymlflips to webpack here (tactical, reversed whenshakacode/react_on_rails_rsc#29ships). Handles the production license story (.env.example, CI secret).RSCWebpackPluginviareact-on-rails-rsc/WebpackPluginin both client and RSC bundles;rscWebpackConfig.jsderived fromserverWebpackConfig(true)per canonical refs. RSC initializer fields (enable_rsc_support,rsc_bundle_js_file,rsc_payload_generation_url_path). Demo page + routes +'use client'directives on existing client components. No custom plugin, no separatersc-bundle.jssource, no manualrsc-client-components.jsside-import — upstream plugin'sclientReferenceshandles client-ref inclusion.Post-merge follow-up
Once upstream
shakacode/react_on_rails_rsc#29shipsRSCRspackPluginin a release, flipshakapacker.ymlback to rspack and swaprequire('react-on-rails-rsc/WebpackPlugin')→require('react-on-rails-rsc/RspackPlugin'). Small diff — the manifest contract is shared between the two plugins.CI note
The repo's
js_test.yml,lint_test.yml, andrspec_test.ymlworkflows only trigger on PRs targetingmaster, so sub-PRs (which target this base branch) didn't get the full suite. This PR is where the full suite runs across all the accumulated sub-PR changes before anything lands onmaster.🤖 Generated with Claude Code