Walkthrough

Node renderer now parses worker counts via a new helper: primary RENDERER_WORKERS_COUNT, fallback NODE_RENDERER_CONCURRENCY, then default 3. Invalid inputs return null to allow fallback; CI still forces workersCount to 2. Documentation and minor formatting/directive presentation updated.

Changes

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I hop through envs, sniffing each name,
RENDERER first, then an old flame.
If numbers fail or none apply,
Three carrots wait beneath the sky.
CI taps two—off we go, nimble and spry. 🥕

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 @coderabbitai help to get the list of available commands and usage tips.}

Review: Fix #2607 – align Pro worker-count env var naming

Overall the change is clean and the backward-compatibility story is solid. A couple of issues worth addressing:

1. || 3 in the docs snippet silently swallows an explicit 0

See the inline comment on pro-quick-start.md:126. The generated template correctly handles this with a nested ternary, but the docs snippet uses Number(... ?? ...) || 3, which converts 0 back to the default 3. Keeping the two implementations inconsistent is a latent footgun for anyone who copies the docs example directly.

2. Generator spec should assert the canonical env var names

react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (lines 1173–1182) only checks expect(content).to include("workersCount:"). That assertion wouldn't have caught the original naming drift in issue #2607, and it won't catch a future rename either.

Suggested additions to the "creates node-renderer.js bootstrap file" example:

expect(content).to include("RENDERER_WORKERS_COUNT")
expect(content).to include("NODE_RENDERER_CONCURRENCY")

3. Minor – README wording could be clearer

packages/react-on-rails-pro-node-renderer/README.md:103 now reads:

Legacy NODE_RENDERER_CONCURRENCY is still supported in generated templates.

The phrase "in generated templates" implies the legacy var is only honored when using the generator, but users who hand-wrote their node-renderer.js will also benefit if they use the pattern shown in the docs. Something like "Legacy NODE_RENDERER_CONCURRENCY is still honored as a fallback" would be less ambiguous.

No security or performance concerns. The logic change is correct and the risk is low as stated.

Greptile Summary

This PR standardizes the worker-count env var naming for the Pro Node Renderer. The generated template (node-renderer.js) now uses RENDERER_WORKERS_COUNT as the canonical env var (consistent with the RENDERER_* convention used by all other renderer settings) and falls back to the legacy NODE_RENDERER_CONCURRENCY for backward compatibility. Docs and README are updated accordingly.

• The template's fallback logic uses a clear ternary chain with != null checks, correctly handling all edge cases
• The runtime library (configBuilder.ts) already reads only RENDERER_WORKERS_COUNT; the legacy fallback exists solely in the generated template for existing deployments
• Note: docs/pro/installation.md (line 256) still references only NODE_RENDERER_CONCURRENCY without the new canonical var — a follow-up update may be needed there

Confidence Score: 5/5

• This PR is safe to merge — it's a straightforward env var naming alignment with proper backward compatibility.
• All changes are documentation and template updates with no runtime library modifications. The fallback logic preserves backward compatibility. Tests pass. The change is small, well-scoped, and aligns with existing conventions.
• No files require special attention. Optionally, docs/pro/installation.md (not in this PR) could be updated for full consistency.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["workersCount resolution"] --> B{"RENDERER_WORKERS_COUNT\nset?"}
    B -- "Yes" --> C["Use Number(RENDERER_WORKERS_COUNT)"]
    B -- "No" --> D{"NODE_RENDERER_CONCURRENCY\nset? (legacy)"}
    D -- "Yes" --> E["Use Number(NODE_RENDERER_CONCURRENCY)"]
    D -- "No" --> F["Default: 3"]
    G["CI environment?"] --> H{"env.CI truthy?"}
    H -- "Yes" --> I["Override: workersCount = 2"]
    H -- "No" --> J["Keep resolved value"]

_{Last reviewed commit: 960a39e}

size-limit report 📦

Review Summary

Overall the direction is correct — using RENDERER_WORKERS_COUNT as the canonical env var with a backward-compatible fallback to NODE_RENDERER_CONCURRENCY is a clean fix for the naming drift. The parseWorkersCount helper and nullish-coalescing chain in the template are readable and correct. A few things worth addressing before merge:

Issues

1. Whitespace-only string treated as 0 workers (inline, template line 6)
Number(' ') returns 0, which passes the current validation and silently sets workersCount: 0. A .trim() guard fixes this.

2. Zero workers allowed without warning (inline, template line 8)
parsed >= 0 permits workersCount: 0, which would leave the renderer with no workers and break SSR silently. Should require >= 1, or explicitly document that 0 is an intentional sentinel.

3. No deprecation signal for legacy env var (inline, template lines 22–25)
When NODE_RENDERER_CONCURRENCY is the active fallback, nothing in the runtime tells operators to migrate. A one-time console.warn would make the migration path self-documenting in production logs.

4. Doc example diverges from template pattern (inline, quick-start lines 126–139)
The quick-start guide shows a verbose inline IIFE while the generated template uses parseWorkersCount + ??. These will drift independently. The doc should mirror the template pattern or be simplified to a single readable expression.

Minor

The README table update is accurate and the wording is clear. No issues there.

Follow-up after latest commit (3c00d20)

The latest commit addressed two issues flagged earlier:

• Whitespace-only string handled correctly now via the trim() guard
• Docs/template divergence fixed: quick-start guide now uses the same parseWorkersCount + nullish-coalescing pattern as the generated template

One issue remains in both files: the guard uses parsed >= 0, which accepts 0. If RENDERER_WORKERS_COUNT=0 or NODE_RENDERER_CONCURRENCY=0 is set, workersCount resolves to 0 and the renderer silently starts with no workers, breaking all SSR requests. Changing to parsed >= 1 (see inline comments) closes this gap with minimal risk.

If 0 is intentional (e.g. as a single-process sentinel the underlying library uses), that should be explicitly documented rather than accepted silently here.

Review Summary

The core logic change (introducing RENDERER_WORKERS_COUNT as the canonical env var with a NODE_RENDERER_CONCURRENCY fallback) is sound and the parseWorkersCount validation helper is a genuine improvement over the old unguarded Number() cast.

Bug in docs (rsc-troubleshooting.md) — The change from bare string literals to parenthesized form ('use client') in the GOOD examples introduces incorrect syntax. Bundlers (Next.js/Turbopack/webpack) detect the directive as a bare StringLiteral AST node; wrapping it in parens creates a ParenthesizedExpression that is not reliably recognized as a directive. This change appears unrelated to the PR's stated goal and should be reverted. Inline comments posted on the specific lines.

CI override silently beats the new canonical env var — The pre-existing if (env.CI) { config.workersCount = 2; } block at the bottom of the generated template unconditionally overrides workersCount, so a user who explicitly sets RENDERER_WORKERS_COUNT in their CI pipeline will be silently ignored. Now that this PR promotes RENDERER_WORKERS_COUNT as the canonical var, the CI block should either respect an explicitly set value or at least be documented with a warning. Inline comment posted on the template.

Minor: parseWorkersCount is duplicated between the generator template and pro-quick-start.md — expected for generated-file docs, but future changes to validation logic need updating in both places.

Review of PR #2611: Fix env var naming for Pro worker count

Summary

The core change — making RENDERER_WORKERS_COUNT canonical with a backward-compatible fallback to NODE_RENDERER_CONCURRENCY — is well-motivated and the implementation is solid. The parseWorkersCount helper correctly handles all edge cases: null/undefined, empty strings, floats, negatives, and explicitly 0 (single-process mode).

Bug: Unrelated 'use client' change in rsc-troubleshooting.md

The diff changes every 'use client' to ('use client') in docs/oss/migrating/rsc-troubleshooting.md. This is incorrect and unrelated to this PR's purpose. I've left an inline comment with details, but the short version:

• React/RSC bundlers detect the directive by looking for a bare StringLiteral expression statement in the AST. A ParenthesizedExpression wrapping a string is not the same AST node and may silently be ignored.
• Worse, the surrounding // GOOD labels make these the recommended forms for readers — actively misleading anyone who copies the example.

These three occurrences should be reverted to 'use client';.

Minor notes

• The CI override (if (env.CI) { config.workersCount = 2; }) still hard-codes 2 and cannot be overridden even when a developer explicitly sets RENDERER_WORKERS_COUNT=0 for single-process debugging in CI. Not introduced by this PR, but worth a follow-up issue.
• The README doc update (changing "The --pro generator uses NODE_RENDERER_CONCURRENCY instead" to "Legacy NODE_RENDERER_CONCURRENCY is still supported") is a clear improvement.

Code Review

Overall this is a clean, well-structured change. The backward-compatibility design is correct: RENDERER_WORKERS_COUNT as canonical, NODE_RENDERER_CONCURRENCY as legacy fallback, and the CI override only applies when no explicit value is set (so RENDERER_WORKERS_COUNT=0 correctly disables the CI default of 2).

Observations

Dead code in parseWorkersCount
The branch typeof value === 'string' ? value.trim() : String(value) will never reach the non-string path because the function is only called with process.env.* values, which are always strings (or undefined). The undefined case is already handled by the early null guard. The String(value) path is dead code in the generated template.

Spec coverage is presence-only
The two new spec lines assert the generated file contains specific source strings but don't exercise the runtime logic (e.g., that the fallback chain works, that 0 is accepted, that an invalid string returns null). For a generated template this is the norm, but the parseWorkersCount logic itself has no dedicated unit tests.

Pre-existing: Number(env.RENDERER_PORT) || 3800 inconsistency
Out of scope for this PR, but worth a follow-up: RENDERER_PORT=0 silently falls back to 3800 because Number('0') || 3800 === 3800. The new parseWorkersCount correctly handles 0 via >= 0, making the port parsing inconsistent by comparison.

Review

The fix is clean and correct. The parseWorkersCount helper properly handles all edge cases (null/undefined, empty string, negatives, floats, NaN), the ?? chaining correctly preserves 0 as a valid single-process value, and the CI guard now only applies when no explicit override is set — all good.

A few observations:

Missing CHANGELOG entry — This fixes a real user-facing issue (#2607) in the Pro generator template. Per the changelog guidelines, a #### Pro > ##### Fixed entry in CHANGELOG.md seems warranted.

Silent fallback on invalid value — Noted inline on parseWorkersCount. If RENDERER_WORKERS_COUNT=bad is set, it silently falls back without any log message, which can be hard to debug in production. Worth adding a console.warn here.

Pre-existing port inconsistency — Number(env.RENDERER_PORT) || 3800 uses the old falsy pattern. Port 0 would incorrectly fall back to 3800. Low severity since port 0 isn't a real use case, but it's now more visible alongside the careful parseWorkersCount approach. Could be addressed as a follow-up.

Doc/template duplication — pro-quick-start.md now mirrors the template code, which is the right approach for docs. Just worth being aware that they'll need to be kept in sync manually if the template evolves further.

Overall: good, low-risk change. Addresses the issue cleanly with backward compatibility preserved.

Review: Fix #2607 — align Pro worker-count env var naming

Overall the approach is sound: RENDERER_WORKERS_COUNT as canonical with a legacy NODE_RENDERER_CONCURRENCY fallback, proper 0-support, and validation. A few things worth addressing before merge:

Missing CHANGELOG entry
The [Unreleased] section is empty. This PR changes the behaviour of generated files (new env var precedence, CI-cap semantics) and should have an entry — tagged [Pro] — so operators upgrading know what to re-generate or watch for.

Spec coverage is thin
The two new assertions in install_generator_spec.rb only check that certain strings appear in the generated file. There are no tests for the parseWorkersCount logic itself (e.g. "0" → 0, "1.5" → null/warn, "" → null, "abc" → null/warn). If the helper ever lives in the library it could be unit-tested there; otherwise a JS test file for the template would help catch regressions.

Inline comments posted on specific lines (see thread above).

Overall: nice clean fix, just needs a CHANGELOG entry and the README/table inconsistencies addressed.

Overall the direction is right: canonicalizing RENDERER_WORKERS_COUNT, introducing a validated parseWorkersCount helper, and keeping backward-compatible fallback to NODE_RENDERER_CONCURRENCY is a clean approach. The test coverage for the new helper is solid.

Issues flagged inline:

• ReactOnRailsProNodeRenderer.ts (medium): configBuilder.ts still parses RENDERER_WORKERS_COUNT with bare parseInt, so invalid values produce NaN workers when workersCount is not set in code — inconsistent with the new validated helper.
• ReactOnRailsProNodeRenderer.ts:13 (low): Number() accepts scientific notation (1e2 = 100) and hex (0x10 = 16) as valid worker counts, which is likely unintended.
• ReactOnRailsProNodeRenderer.ts:15 (nit): Warn message logs raw value — should log trimmed normalized for clarity.
• parseWorkersCount.test.ts (low): Missing tests for 1e2 / 0x10 edge cases to lock in the intended contract.
• install_generator_spec.rb (low): Spec checks configuredWorkersCount variable usage but does not assert parseWorkersCount appears in the generated import.

Other observations:

• The CI guard if (env.CI && configuredWorkersCount == null) correctly preserves an explicit RENDERER_WORKERS_COUNT=0 (single-process mode) even in CI.
• parseWorkersCount is exported from the main package entry point so the destructured require in templates will work correctly.