Pro RSC migration 2/3: switch SSR to the Pro Node renderer on webpack

[ihabadham]

Summary

Routes server rendering through the Pro Node renderer and flips the asset bundler to webpack. Wires the renderer in dev (Procfile.dev), CI (rspec_test.yml), and production (Control Plane Dockerfile CMD + env).

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

References this PR follows

• Pro documentation in the React on Rails repo — docs/oss/configuration/configuration-pro.md for initializer fields, docs/oss/building-features/node-renderer/container-deployment.md for the single-container deploy pattern, docs/oss/building-features/node-renderer/js-configuration.md for launcher options.
• Pro dummy app — the test application at react_on_rails_pro/spec/dummy/ in shakacode/react_on_rails. Referenced for the initializer shape, serverWebpackConfig.js Pro transforms, and the launcher defaults.
• RSC marketplace demo — the deployed reference app at shakacode/react-on-rails-demo-marketplace-rsc. Referenced for the Control Plane single-container Dockerfile CMD pattern and the app.yml renderer env block.
• cpflow (control-plane-flow) — shakacode/control-plane-flow. The docs/secrets-and-env-values.md guide and lib/core/template_parser.rb document the {{APP_SECRETS}} template variable used in app.yml.

Changes

Bundler

• shakapacker.yml: rspack → webpack. Tactical reversal of Upgrade React on Rails/Shakapacker and standardize on Rspack v2 #702 — rspack 2.0.0-beta.7's webpack-compat layer doesn't support the APIs upstream RSCWebpackPlugin needs. Flips back when shakacode/react_on_rails_rsc#29 ships a release.
• bundlerUtils.js: returns @rspack/core or webpack based on config (was rspack-only).
• serverWebpackConfig.js: Pro transforms per the :pro generator's update_webpack_config_for_pro and the Pro dummy's serverWebpackConfig.js — target: 'node', node: false, libraryTarget: 'commonjs2', extractLoader, babel SSR caller, destructured exports, RSCWebpackPlugin({ isServer: true, clientReferences: [...] }).

Pro runtime

• initializers/react_on_rails_pro.rb: NodeRenderer-only config (RSC fields in Sub-PR 3). renderer_use_fallback_exec_js = false so any renderer issue raises loudly instead of silently degrading to ExecJS.
• renderer/node-renderer.js: launcher with integer env parsing, CI worker cap, additionalContext: { URL, AbortController } (without URL, react-router-dom's NavLink throws ReferenceError: URL is not defined at SSR time).
• package.json: react-on-rails-pro-node-renderer 16.6.0, react-on-rails-rsc 19.0.4 (Pro peer dep), react + react-dom pinned to 19.0.4 per the RSC CVE floor.

Dev / CI / Production wiring

• Procfile.dev: node-renderer process name (matches the Pro dummy's Procfile.dev).
• .github/workflows/rspec_test.yml: starts the renderer with PID liveness + port-ready checks + log capture on failure.
• .controlplane/Dockerfile CMD: runs the renderer and Rails in one container with wait -n ; exit 1. Option 1 "Single Container" from container-deployment.md; same pattern as the marketplace demo's Dockerfile.
• .controlplane/templates/app.yml: renderer env (RENDERER_PORT, RENDERER_LOG_LEVEL, RENDERER_WORKERS_COUNT, RENDERER_URL) + cpln://secret/{{APP_SECRETS}}.* references for RENDERER_PASSWORD and REACT_ON_RAILS_PRO_LICENSE. {{APP_SECRETS}} is cpflow's template variable for {APP_PREFIX}-secrets.
• .controlplane/templates/rails.yml: memory 512Mi → 1Gi and capacityAI: false → true. Gives the single container headroom for Rails + renderer without matching the marketplace demo's 2Gi static baseline (tutorial's Rails surface is smaller); capacityAI adjusts CPU/memory within the replica based on observed usage.
• .env.example: RENDERER_URL, RENDERER_PASSWORD, REACT_ON_RAILS_PRO_LICENSE.

Deploy prerequisite (manual, one-time)

Before deploying this change to Control Plane, create three Secrets across the two orgs with keys RENDERER_PASSWORD (strong random) and REACT_ON_RAILS_PRO_LICENSE (JWT from https://pro.reactonrails.com/):

Org	Secret name	Apps using it
shakacode-open-source-examples-production	react-webpack-rails-tutorial-production-secrets	production
shakacode-open-source-examples-staging	react-webpack-rails-tutorial-staging-secrets	staging
shakacode-open-source-examples-staging	qa-react-webpack-rails-tutorial-secrets	all qa-* review apps (shared)

Review apps share one secret because {{APP_SECRETS}} expands to {APP_PREFIX}-secrets and the qa-apps config has match_if_app_name_starts_with: true, so the prefix is qa-react-webpack-rails-tutorial for every PR.

cpln secret create --org <org> --name <secret-name> --type opaque \
  --data '{"RENDERER_PASSWORD":"...","REACT_ON_RAILS_PRO_LICENSE":"..."}'

Without this, the workload resolves cpln://secret/... → fails → crashloops.

Stack context

• Sub-PR 1 (Pro RSC migration 1/3: Swap gem and npm to react_on_rails_pro #726, merged): gem + npm package migration.
• Sub-PR 2 (this PR): NodeRenderer + webpack + deploy infra.
• Sub-PR 3: RSC demo (upstream RSCWebpackPlugin({ isServer: false }) on client, rscWebpackConfig.js derived from serverWebpackConfig(true), initializer RSC fields, server-components page + routes + 'use client' directives).
• Post-merge follow-up: flip shakapacker.yml back to rspack once shakacode/react_on_rails_rsc#29 ships a release.

Test plan

• bundle exec rake shakapacker:compile clean webpack build
• Renderer + Rails → GET / HTTP 200; Rails log has Node Renderer responded; renderer log shows [SERVER] RENDERED lines — SSR routes through the Pro path, not ExecJS.
• Renderer killed → HTTP 500 with HTTPX::ConnectionError (Connection refused). No ExecJS fallback (confirms renderer_use_fallback_exec_js = false is load-bearing).
• Wrong password → HTTP 500 with 401: Wrong password from the renderer.
• CI green on base PR [DO NOT MERGE] Pro RSC migration: stacked base PR (tracks sub-PRs) #727.
• Control Plane staging deploy (requires the Secret creation step above).

🤖 Generated with Claude Code

[coderabbitai]

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: 1fd61952-bc61-44bd-a43a-36e8e78176bf

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/node-renderer

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

🚀 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.

---

[claude]

Code Review — PR #728: Pro RSC migration 2/3 (SSR → Node renderer + webpack)

Overview

This PR wires up the Pro Node renderer as the SSR backend and switches the asset bundler from rspack back to webpack (scoped tactical reversal while the native rspack RSC plugin is in progress). The overall approach is sound and the PR is well-documented. A few targeted issues to address before merging.

---

Issues

🔴 Breaking change: bundlerUtils.js silently falls back to webpack for invalid assets_bundler values

File: config/webpack/bundlerUtils.js

_cachedBundler = config.assets_bundler === 'rspack' ? require('@rspack/core') : require('webpack');

The old code threw an explicit, actionable error for any non-rspack value (including typos like rpack). The new code silently returns webpack for any non-rspack value — including invalid values. The test case 'throws when assets_bundler is invalid' was removed rather than updated to reflect the new behavior.

Risk: A mis-typed assets_bundler in shakapacker.yml would silently build with webpack, producing a confusing debug experience. Suggest throwing on values that are neither 'rspack' nor 'webpack':

if (config.assets_bundler !== 'rspack' && config.assets_bundler !== 'webpack') {
  throw new Error(`Invalid assets_bundler: "${config.assets_bundler}". Expected "webpack" or "rspack".`);
}
_cachedBundler = config.assets_bundler === 'rspack' ? require('@rspack/core') : require('webpack');

And add a test for the invalid case.

---

🟡 babelLoader.options.caller replaces instead of merges

File: config/webpack/serverWebpackConfig.js

babelLoader.options = babelLoader.options || {};
babelLoader.options.caller = { ssr: true };

This overwrites any existing caller configuration. If shakapacker or a plugin already sets caller fields, they'll be silently lost. Safer to spread:

babelLoader.options.caller = { ...babelLoader.options.caller, ssr: true };

---

🟡 CJS module.exports = { default: ... } is an unusual pattern

File: config/webpack/serverWebpackConfig.js → config/webpack/webpackConfig.js

Exporting a CJS module with a key named default is unconventional (that naming belongs to ES modules). webpackConfig.js correctly imports it as const { default: serverWebpackConfig } = require(...), but the pattern will confuse anyone reading it cold. Since this is CJS throughout, using a named export is cleaner:

// serverWebpackConfig.js
module.exports = { configureServer, extractLoader };

// webpackConfig.js
const { configureServer: serverWebpackConfig } = require('./serverWebpackConfig');

---

🟡 workersCount: 0 is permitted

File: renderer/node-renderer.js

workersCount: parseIntegerEnv('NODE_RENDERER_CONCURRENCY', 3, { min: 0 }),

min: 0 means NODE_RENDERER_CONCURRENCY=0 is accepted. Whether the renderer library handles 0 workers gracefully (e.g., single-threaded mode) should be verified; if it does, add a comment clarifying the intent. If not, min: 1 is safer.

---

🟢 CI: implicit password coordination between renderer and Rails initializer

File: .github/workflows/rspec_test.yml

The CI step starts node renderer/node-renderer.js without setting RENDERER_PASSWORD, so both the Node renderer and the Rails initializer fall back to local-dev-renderer-password. This works, but it's invisible from the CI config alone — a future reader adding env vars might break authentication silently. A brief inline comment in the workflow (or an explicit env: RENDERER_PASSWORD: local-dev-renderer-password) would make the coupling visible.

---

🟢 RSCWebpackPlugin is imported unconditionally but conditionally used

File: config/webpack/serverWebpackConfig.js

const { RSCWebpackPlugin } = require('react-on-rails-rsc/WebpackPlugin');
// ...
if (!rscBundle) {
  serverWebpackConfig.plugins.push(new RSCWebpackPlugin({ isServer: true }));
}

The require at module load time means an import cost on every build, including when rscBundle=true. Not a significant perf issue, but if Sub-PR 3 is the only caller that passes true, it's worth noting that the dependency is always loaded. This is fine as-is since react-on-rails-rsc is a declared dependency.

---

Positive highlights

• The parseIntegerEnv helper is clean and well-tested with edge cases (empty string, floats, out-of-range).
• CI liveness check (PID alive + port-ready) is robust — it fails loudly instead of letting a silent renderer crash cause flaky tests.
• renderer_use_fallback_exec_js = false is the right call — fail fast in any renderer issue rather than silently degrading.
• additionalContext: { URL, AbortController } is a well-documented fix for the NavLink SSR crash.
• .env.example is complete and clearly explains which vars are required in production vs dev.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 758a2c7bd4

ℹ️ 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".

[claude]

PR Review findings for Pro RSC Migration 2/3: (1) Bug in renderer/node-renderer.js - parseIntegerEnv uses min:0 for NODE_RENDERER_CONCURRENCY allowing workersCount:0 which would hang the renderer; bump to min:1. (2) serverWebpackConfig.js overwrites babelLoader.options.caller rather than merging - should spread existing value. (3) The module.exports={default:configureServer,extractLoader} pattern is unusual for CJS; prefer module.exports=configureServer; module.exports.extractLoader=extractLoader. (4) target:'node' without version pin could cause unexpected polyfills; consider 'node18'. (5) extractLoader helper has no unit tests. Minor: getBundler() silently falls back to webpack for unknown assets_bundler values; a warning would help. Security looks good - ENV.fetch(RENDERER_PASSWORD) correctly raises KeyError at boot in production rather than falling back to a dev default.

[claude]

Code Review — PR #728: Pro RSC migration 2/3 (SSR → Node renderer + webpack)

Overall this is a well-structured, well-commented atomic migration. The stacking rationale is clear, the Pro webpack transforms follow the upstream dummy pattern, and the CI wiring for the renderer is thoughtful. A few issues worth addressing before merge:

---

Bugs / Correctness

• min: 0 for workersCount (renderer/node-renderer.js:36) — zero workers makes the renderer boot but never serve requests. Inline comment with fix.
• babelLoader.options.caller replacement (serverWebpackConfig.js:157) — clobbers any existing caller properties; should spread. Inline comment with fix.

---

Code Quality

• module.exports = { default: configureServer, extractLoader } (serverWebpackConfig.js:179-182) — using default as a CJS property name is non-idiomatic and forces every caller to destructure { default: fn }. The standard pattern module.exports = fn; module.exports.extractLoader = extractLoader is cleaner and keeps existing call sites working without changes. Inline comment with suggestion.
• Silent fallback in getBundler() (bundlerUtils.js:21) — any unrecognised assets_bundler value (typo, future third bundler) silently resolves to webpack. The old guard that threw on unknown values was a better safety net. Inline comment with suggested addition.

---

Security

• Empty-string RENDERER_PASSWORD (node-renderer.js:5) — RENDERER_PASSWORD="" in a staging/non-production env will silently fall through to the dev default password due to the || short-circuit. Minor, but worth tightening. Inline comment with suggestion.

---

CI / Operations

• Renderer starts as NODE_ENV=test (.github/workflows/rspec_test.yml:90) — the workflow-level env is test; Procfile.dev uses development. Makes CI and local dev diverge if the renderer library branches on NODE_ENV. Inline comment.
• Renderer logs not captured on mid-test crash (.github/workflows/rspec_test.yml:107) — logs are only dumped if the renderer fails to start; a crash during the rspec run loses all output. Suggest adding an if: failure() step to dump /tmp/node-renderer.log.

---

Minor / Nits

• package.json pins react-on-rails-rsc to exact 19.0.4 (no ^), but the PR description says ^19.0.4. Intentional pin or discrepancy?
• Test for returns webpack when assets_bundler is undefined (bundlerUtils.spec.js) confirms the silent-fallback behaviour noted above. If the validation guard is added, this test would need updating to expect the throw.

---

🤖 Generated with Claude Code

[claude]

Code Review: Pro RSC migration 2/3

Overall this is a solid, well-documented PR. The architecture decisions are sound and the test plan is thorough. A few items worth addressing before merge:

Signal handling in the single-container CMD (medium)

The Dockerfile CMD runs bash as PID 1. When Control Plane sends SIGTERM to stop the container, bash receives it and exits — but it does not forward SIGTERM to its backgrounded children (the renderer and Thruster/Rails). The kernel then SIGKILLs everything after the grace period. This means Rails never gets a chance to drain in-flight connections.

For a tutorial app this may be acceptable, but for staging/production it can cause dropped requests on redeploy. The fix is a small trap in the CMD:

CMD ["bash", "-c", "
  trap 'kill $RENDERER_PID $RAILS_PID 2>/dev/null' TERM INT;
  node renderer/node-renderer.js & RENDERER_PID=$! ;
  bundle exec thrust bin/rails server & RAILS_PID=$! ;
  wait -n ; exit 1
"]

Or better yet, extract this to .controlplane/start.sh — the one-liner is already hard to read.

No renderer log capture if it crashes mid-test (low)

The CI "Start Node renderer" step captures crash-on-startup correctly, but exits (and loses $RENDERER_PID) before the test suite runs. If the renderer crashes at request time during rspec, the test failures will say Connection refused with no renderer output. Adding a if: failure() step that prints the renderer log file (if you redirect its stdout to a file) would make these failures much easier to diagnose.

Rails.env.local? requires Rails 7.1+ (low)

Rails.env.local? (returns true for development + test) was added in Rails 7.1. If this branch will ever be backported or run on an older Rails, use Rails.env.development? || Rails.env.test? instead. Inline comment added below.

devtool: 'eval' on the server bundle (informational)

eval source maps work by wrapping each module in an eval() call. The Pro Node renderer evaluates the bundle in a Node.js VM context — if that VM has --disallow-code-generation-from-strings set (or equivalent), eval-based source maps will throw at runtime. cheap-module-source-map (or false for production) avoids this. Not a current issue since the renderer appears to use a standard VM, but worth knowing if you ever harden the sandbox.

Minor

• RENDERER_PID and RAILS_PID are assigned in the Dockerfile CMD but wait -n doesn't reference them — they only serve as documentation. Either drop them or add them to a trap (see above) so they earn their place.
• module.exports = { default: configureServer, extractLoader } is a valid CommonJS pattern and the consumer destructures it correctly (const { default: fn } = require(...)), but it looks like an ES module leak. A note in the file or a rename (e.g. configureServer at the export site) would reduce future confusion.

[ihabadham]

/deploy-review-app

[github-actions]

🏗️ Building Docker image for PR #728, commit b0a8dff

📝 View Build Logs

🎮 Control Plane Console

[ihabadham]

/deploy-review-app

[github-actions]

🚀 Deploying to Control Plane...

⏳ Waiting for deployment to be ready...

📝 View Deploy Logs
🎮 Control Plane Console

[github-actions]

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #728, commit b000c55

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

[claude]

Code Review: PR #728 — Pro RSC migration 2/3: switch SSR to the Pro Node renderer on webpack

Overview

This PR wires the React on Rails Pro Node renderer into Rails SSR, flips the asset bundler back to webpack (tactical regression while awaiting rspack RSC plugin support), and updates dev/CI/production plumbing accordingly. The description is thorough and the implementation is well-commented. A few correctness and safety issues are worth addressing before merge.

---

Issues

High priority

1. wait -n requires bash 4.3+ — Dockerfile must guarantee this
The CMD uses wait -n (exit when the first child finishes), which was introduced in bash 4.3. The Dockerfile doesn't pin or verify a bash version. If the base image ships an older bash or /bin/bash is actually dash/ash (common in Alpine-derived images), the container will crash at startup with a confusing error rather than a process failure. Verify the base image ships bash ≥ 4.3 or replace with a portable init-process pattern (e.g., tini).

2. RENDERER_PID and RAILS_PID are dead variables in the Dockerfile CMD
Both PIDs are captured but never read again. wait -n waits for any background child, not those specific PIDs, so storing them does nothing. Remove them or document why they might be needed.

3. getBundler() silently falls back to webpack for any invalid assets_bundler value
The old code threw explicitly when assets_bundler !== 'rspack'. The new code falls through to require('webpack') for any non-'rspack' value, including typos like 'webapck'. The corresponding test for this case was deleted without replacement. Either add validation that throws for values other than 'rspack'/'webpack', or add an explicit test documenting the intentional fallback behavior.

4. CI log capture on renderer startup failure is promised but not implemented
The PR description says "log capture on failure" is part of the CI changes. The current step only prints "see output above" and exits — it doesn't capture or upload renderer logs. If the renderer process exits silently (wrong Node version, missing bundle, etc.), there will be nothing to see "above". Consider redirecting stdout/stderr to a file at startup and using actions/upload-artifact on failure, or at minimum cat the log file in a subsequent always-run step.

---

Medium priority

5. rscBundle parameter logic feels inverted
The signature is configureServer(rscBundle = false) and the RSCWebpackPlugin is added when !rscBundle. So calling configureServer(true) ("I want the RSC bundle") omits the RSC plugin. This is intentional per Sub-PR 3 design, but the name is misleading. Consider renaming to isRscClientBundle or adding a short inline comment on the condition explaining the intent.

6. additionalContext: { URL, AbortController } relies on Node.js globals that may not exist
AbortController is only a Node.js global from v15.0+. If the renderer runs on an older Node.js, the reference throws ReferenceError: AbortController is not defined at module load time — before the helpful startup error message. Adding an engines field to package.json would make the version requirement explicit and fail fast with a clear message.

7. babelLoader.options mutation may affect the shared config object
babelLoader.options.caller = { ssr: true } directly mutates the loader config returned by commonWebpackConfig(). If that factory ever returns a cached or shared object, this mutation could cause issues. The existing code already mutates cssLoader.options.modules, so this follows the established pattern, but it is worth noting the footgun for Sub-PR 3 if it calls configureServer multiple times.

---

Low priority

8. Removed test for invalid assets_bundler with no replacement
The test it('throws when assets_bundler is invalid') was removed. The new behavior (silent fallback to webpack) should have a test documenting the intention, to prevent someone from re-adding the guard in the future thinking it was an oversight.

9. extractLoader exported as public module API
This helper looks like an internal implementation detail of serverWebpackConfig.js. Exporting it (presumably for Sub-PR 3's RSC config) couples the two PRs at the API surface. If it's only used by Sub-PR 3, consider keeping it private until that PR adds it, or add a comment on the export documenting the intended consumer.

---

Positives

• The renderer startup check in CI (PID liveness + port readiness) is well-structured.
• The parseIntegerEnv helper with range validation is clean and avoids silent truncation.
• renderer_use_fallback_exec_js = false is explicitly documented and the test plan validates it.
• The TODO comment in shakapacker.yml clearly tracks the tactical regression with a specific upstream issue reference.
• The Control Plane secret setup instructions in the PR description are unusually thorough — consider copying a condensed version into .controlplane/README.md or a deploy runbook so it is not buried in the PR history.

[claude]

Any value that isn't 'rspack' — including typos like 'webapck' or an empty string — silently falls through to require('webpack'). The old code threw explicitly; the new code is less strict.

Consider guarding the two valid values:

Suggested change

	_cachedBundler = config.assets_bundler === 'rspack' ? require('@rspack/core') : require('webpack');
	if (config.assets_bundler === 'rspack') {
	_cachedBundler = require('@rspack/core');
	} else if (!config.assets_bundler || config.assets_bundler === 'webpack') {
	_cachedBundler = require('webpack');
	} else {
	throw new Error(
	`Invalid assets_bundler: "${config.assets_bundler}". Expected "webpack" or "rspack".`,
	);
	}

[claude]

AbortController is only a Node.js global from v15.0+. Referencing it here (at module load time, not lazily) means an older Node.js runtime throws ReferenceError: AbortController is not defined before the helpful startup error messages can run.

Adding an engines field to package.json would make the minimum Node.js version explicit and surface this constraint early:

"engines": { "node": ">=18.0.0" }

Alternatively, a guard at the top of this file:

if (typeof AbortController === 'undefined') {
  throw new Error('Node.js >=15.0.0 is required (AbortController global missing)');
}

[claude]

The condition if (!rscBundle) means the RSCWebpackPlugin is added for the default SSR bundle and skipped when rscBundle = true. The parameter name implies the opposite — "if I'm building the RSC bundle, add the plugin". A clearer name (or a short comment) would help avoid confusion in Sub-PR 3:

Suggested change

	// Scope client-reference discovery to the app source dir. Without this,
	if (!rscBundle) {
	// RSCWebpackPlugin does server-side client-reference discovery.
	// Skip it for the dedicated RSC client bundle (Sub-PR 3) where the
	// client-side plugin variant is used instead.

[claude]

The PR description promises "log capture on failure", but when the renderer exits unexpectedly or times out the error message is just "see output above" with no actual log capture. In GitHub Actions each step runs in a fresh shell, so "above" is only what the renderer printed to stdout/stderr before backgrounding — which may be empty if it died during startup.

Consider capturing renderer output to a file and uploading it as an artifact on failure:

Suggested change

	echo "Node renderer failed to start within 30 seconds (see output above)."
	echo "Node renderer failed to start within 30 seconds."
	kill $RENDERER_PID 2>/dev/null || true
	exit 1

And add a subsequent step:

      - name: Upload renderer logs on failure
        if: failure()
        uses: actions/upload-artifact@v4
        with:
          name: renderer-logs
          path: /tmp/node-renderer.log

(requires redirecting node renderer/node-renderer.js output to /tmp/node-renderer.log at startup)

[claude]

RENDERER_PID and RAILS_PID are captured but never read — wait -n waits for any background child, not those specific PIDs, so they do nothing today.

More importantly, wait -n requires bash ≥ 4.3. If the base image doesn't ship that version (or /bin/bash is dash/ash), the container will crash at startup with a confusing syntax error rather than a process failure. Worth verifying the base image guarantees bash 4.3+, or consider a portable init approach (e.g. tini).

Suggested cleanup (assuming bash 4.3+ is confirmed):

Suggested 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"]

[claude]

The test for an invalid assets_bundler value was removed without replacement. The new behavior — silently falling back to webpack for any non-'rspack' value — is a deliberate design decision, but it is now undocumented in the test suite. A future reader could reasonably re-add a throw guard thinking the lack of validation was an oversight.

Consider adding an explicit test after this block:

it('falls back to webpack when assets_bundler is an unrecognized value', () => {
  mockConfig.assets_bundler = 'invalid-bundler';
  jest.doMock('shakapacker', () => ({ config: mockConfig }));
  const utils = require('../../../config/webpack/bundlerUtils');

  // Intentional: any non-rspack value resolves to webpack (tactical, see shakapacker.yml TODO)
  const bundler = utils.getBundler();
  expect(bundler.DefinePlugin.name).toBe('MockWebpackDefinePlugin');
});

(Or change this to toThrow if stricter validation is added to bundlerUtils.js.)

[ihabadham]

Thanks for the review. All 9 items are either reference-matched or previously-settled themes — running the same reference-check methodology used across this PR:

High priority

1. bash 4.3+ for wait -n — Our Dockerfile uses ruby:$RUBY_VERSION-slim (Debian). Marketplace demo reference (react-on-rails-demo-marketplace-rsc) uses ruby:3.3.0-slim with the same wait -n pattern. Debian slim ships bash 5.1/5.2. Alpine concern is theoretical for this base.
2. Dead PID vars — Settled multiple times. Marketplace demo + hichee production Dockerfile both have the same unused PID captures. Deploy just validated the pattern works end-to-end on CP.
3. bundlerUtils silent fallback — Settled. No reference has a getBundler() helper. Fallback handles shakapacker's optional assets_bundler key; test at bundlerUtils.spec.js:58 covers the undefined case.
4. CI log capture — Stale PR body wording. Code was intentionally aligned with Pro CI (pro-integration-tests.yml) in commit 55b8fb2f; Pro CI runs renderer with no redirect, logs interleave with job stdout.

Medium priority

5. rscBundle naming — Settled. Pro dummy spec/dummy/config/webpack/serverWebpackConfig.js uses the same parameter shape; docs document it in docs/oss/migrating/rsc-preparing-app.md:244.
6. AbortController on Node 15+ — Tutorial pinned to Node 22 (.tool-versions / engines); Pro dummy uses bare { URL, AbortController } verbatim at line 58.
7. babel caller mutation — Pro dummy does the same mutation at line 128. Reference match.

Low priority

8. Removed invalid-bundler test — Intentionally updated to reflect new behavior (the spec now includes returns webpack when assets_bundler is undefined).
9. extractLoader export — Pro dummy exports it (line 154); Pro docs document this as the "new Pro config exports" shape in docs/oss/migrating/rsc-preparing-app.md:142.

Deploy status: Review app built and deployed successfully to CP. End-to-end SSR validated via logs: License validated, Node renderer listening on both workers, [ReactOnRailsPro] Node Renderer responded on GET /. The wait -n bash pattern works on Debian-slim.

[github-actions]

✅ Review app for PR #728 was successfully deleted

View Completed Delete Logs

Control Plane Organization