Walkthrough

Moved renderer-password validation into the main config setup, broadened the validation to consider both RAILS_ENV and NODE_ENV, clarified NODE_ENV-only behavior in Node renderer async-logging comments, and expanded tests for cross-env scenarios.

Changes

Sequence Diagram(s)

sequenceDiagram
  participant Config as ReactOnRailsPro::Configuration
  participant ENV as Environment
  participant Validator as validate_renderer_password_for_production

  Config->>Config: setup_config_values()
  Config->>Config: setup_renderer_password()
  Config->>Validator: validate_renderer_password_for_production()
  Validator->>ENV: read RAILS_ENV, NODE_ENV
  alt any present env outside {development,test}
    Validator->>Config: raise error if no RENDERER_PASSWORD
  else all present envs in {development,test}
    Validator->>Config: no-op (skip validation)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Enforce secure renderer password defaults for production-like envs #2829: Related prior PR that introduced runtime-env gating and renderer-password validation logic which this change extends with cross-environment checks and unconditional validator invocation.

Poem

🐰
I nibble configs, hop through envs so spry,
NODE and RAILS I check with twitching nose,
No mismatches pass, no secret left awry,
A careful guard where safer setup grows,
Hooray — secure boot, and off the rabbit goes! 🥕

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

Greptile Summary

This PR hardens the RENDERER_PASSWORD validation by checking both RAILS_ENV and NODE_ENV (mirroring the Node-side runtimeEnvsAllowDevelopmentDefaults()), moves the validator call to setup_config_values so it runs unconditionally, and adds a clarifying comment to defaultReplayServerAsyncOperationLogs() explaining its intentional NODE_ENV-only check. All four new cross-env test cases are well-targeted and the logic correctly fails closed when both env vars are absent.

Confidence Score: 5/5

Safe to merge — logic is correct, fail-closed, and well-covered by tests.

All findings are P2 or better. The cross-env validation logic is correct: it fails closed when both envs are absent, catches the NODE_ENV=production + RAILS_ENV=development misconfiguration, and preserves the development/test bypass. Moving the validator call to setup_config_values is a clean improvement with no behavioral regressions. Test coverage is thorough for the new scenarios.

No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[setup_config_values] --> B[setup_renderer_password]
    B --> C{renderer_password present?}
    C -- Yes --> D[Skip URL/ENV extraction]
    C -- No --> E[Extract from renderer_url]
    E --> F{Password in URL?}
    F -- Yes --> G[Set from URL]
    F -- No --> H[Set from ENV RENDERER_PASSWORD]
    D & G & H --> I[validate_renderer_password_for_production]
    I --> J{renderer_password present?}
    J -- Yes --> K[✅ Skip validation]
    J -- No --> L{node_renderer?}
    L -- No --> K
    L -- Yes --> M[Build runtime_envs from RAILS_ENV + NODE_ENV]
    M --> N{runtime_envs.any? AND all dev/test?}
    N -- Yes --> K
    N -- No --> O[🚨 Raise Error: RENDERER_PASSWORD required]

_{Reviews (1): Last reviewed commit: "fix: cross-env validation and documentat..." | Re-trigger Greptile}

size-limit report 📦

Review

The approach is sound. Moving validate_renderer_password_for_production out of setup_renderer_password and into setup_config_values is the right call — it makes enforcement unconditional regardless of how the password resolution path evolves.

Logic correctness — The new condition mirrors runtimeEnvsAllowDevelopmentDefaults() faithfully:

The fourth row is the key fix — the old code only checked RAILS_ENV so a NODE_ENV=production + RAILS_ENV=development misconfiguration would have silently passed.

Ordering — setup_renderer_password runs before validate_renderer_password_for_production in setup_config_values, so the validator always sees a fully-resolved password. The validator's own early-return on renderer_password.present? is correct defense-in-depth.

Two small items left inline (one missing test, one style nit on .freeze). Neither blocks merge.

RBS signature — Correctly adds the private method. Looks good.

Review feedback addressed in 35bfb99

All 4 review threads resolved. 52 specs passing, RuboCop clean.

@justin808 Thanks for the detailed summary! Great to see all the feedback addressed so thoroughly. A few notes on the resolutions:

1. .compact_blank fix — Good call. .compact only removes nil, while .compact_blank also strips empty strings "", which correctly mirrors Node's Boolean() filtering behavior. This is an important behavioral difference in Ruby.
2. Error matrix update — Glad the dual-env rule is now accurately reflected in the docs.
3. RAILS_ENV=nil, NODE_ENV=development test — Important edge case, good that it's covered.
4. .freeze decline — Completely reasonable; avoiding a RuboCop Performance/CollectionLiteralInLoop violation is a valid justification for keeping the extracted local variable.

52 specs passing and RuboCop clean is a solid state. 🎉

Learnt from: justin808
URL: https://github.com/shakacode/react_on_rails/pull/3090

Timestamp: 2026-04-12T07:04:58.059Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/configuration.rb`, use `.compact_blank` (not `.compact`) when filtering environment variable collections, because `.compact` only removes `nil` while `.compact_blank` also removes empty strings `""`, matching Node's `Boolean()` filtering behavior for env vars.

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/execjs-compatible-dummy/spec/**/*.rb : ExecJS dummy tests should be run with: `cd react_on_rails_pro/spec/execjs-compatible-dummy && bundle exec rspec`

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2284
File: react_on_rails/lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt:10-10
Timestamp: 2026-02-12T20:43:26.843Z
Learning: In React on Rails Pro initializer templates (e.g., `react_on_rails_pro.rb.tt`), the `config.renderer_password` uses `ENV.fetch("RENDERER_PASSWORD", "devPassword")` as a development starter configuration. This pattern is intentional: the node renderer requires explicit production deployment setup, and adding a production-only raise would break generator execution since initializers are loaded during `rails generate`. The pattern is consistent with existing manual setup documentation.

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 2857
File: README.md:0-0
Timestamp: 2026-03-26T08:32:31.881Z
Learning: In `README.md` (and related docs) for `shakacode/react_on_rails`, the Ruby minimum version (`>= 3.0`) similarly reflects the actual package validator/gemspec floor, while `CI tested: 3.2 - 3.4` describes coverage. This distinction is intentional and should not be flagged as a docs/CI mismatch.

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: The Node renderer uses JWT-based authentication with password from Rails initializer

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Review: cross-env validation and renderer password docs

Overall: solid change. The logic is correct, the Node-side mirror is precise, and moving the validator call out of setup_renderer_password is a resilience improvement. A few observations below.

Logic correctness

The Ruby implementation of validate_renderer_password_for_production now exactly mirrors runtimeEnvsAllowDevelopmentDefaults() in the Node renderer:

• Both unset → runtime_envs.any? is false → raises (fail-closed) ✓
• One dev/test, one nil → all elements allowed → returns ✓
• Mixed (e.g. staging + development) → not all allowed → raises ✓
• Both dev/test → all allowed → returns ✓

Moving the validator call

Previously, if renderer_password.present? was already true on entry to setup_renderer_password, the method returned early and the validator was never called. With the unconditional call in setup_config_values, the validator always runs (and still short-circuits immediately via return if renderer_password.present?). This closes a future-refactor trap where changes to setup_renderer_password could inadvertently skip enforcement.

Minor observations

1. .freeze on a local variable (configuration.rb line 278): allowed_envs = %w[development test].freeze — freezing a local that is never mutated or passed elsewhere is a style quirk. Could be a module constant or just an unfrozen literal. Not a bug.

2. Missing mixed-env test for the other direction: The new cross-env tests cover NODE_ENV=production + RAILS_ENV=development but not RAILS_ENV=staging + NODE_ENV=development. The logic handles it correctly, but an explicit test would document the intent for that combination (staging is a real deployment environment where this misconfiguration could occur).

3. compact_blank vs compact: Using compact_blank is the right choice — it correctly treats an empty-string env var as unset, which preserves fail-closed behavior even if RAILS_ENV="" is set.

Tests

The new cross-env specs (4 production-like + 4 dev/test cases) give thorough coverage of the documented matrix, and the migration from ENV.[] stubs to ENV.fetch stubs correctly aligns with the implementation change.

Review comments addressed (round 2)

Fixed (1):

• configuration_spec.rb:310-314 — Stubbed RENDERER_PASSWORD to nil in the dev/test shared before block so tests exercise the env matrix logic even when the env var is set locally or in CI. Matches the production-like context setup (line 182). → 5a01f45

Already resolved (4):

• Error matrix updated for dual-env rule (coderabbitai) ✅
• .compact → .compact_blank for empty-string filtering (cursor) ✅
• Test for RAILS_ENV=nil, NODE_ENV=development added (claude) ✅
• .freeze on local var justified by RuboCop cop (claude) ✅

All review threads are now resolved.

Review: fix: cross-env validation and docs for renderer password

Overall: Solid, well-reasoned PR. The logic correctly mirrors Node-side runtimeEnvsAllowDevelopmentDefaults() and the move to call validate_renderer_password_for_production unconditionally from setup_config_values is the right architectural fix.

What works well

• Logic parity: The Ruby runtime_envs.any? && runtime_envs.all? { ... } pattern is a faithful translation of the TypeScript runtimeEnvs.length > 0 && runtimeEnvs.every(...) idiom — both fail-closed when both envs are unset, both pass when every present value is development or test.
• Unconditional validator call: Previously, if renderer_password was already present when setup_renderer_password ran, the early return silently skipped validate_renderer_password_for_production. The new placement makes enforcement independent of password-resolution flow — good resilience against future refactors.
• compact_blank vs compact: Using compact_blank correctly treats empty-string RAILS_ENV="" the same as unset — fail-closed. Appropriate for a security boundary.
• Clarifying comment on defaultReplayServerAsyncOperationLogs: The comment explaining the intentional asymmetry (JS debugging concern, not a security boundary) prevents future maintainers from "fixing" it to also check RAILS_ENV.

Behavioral change worth noting in CHANGELOG

Previously: RAILS_ENV=nil + NODE_ENV=development → raised (only RAILS_ENV was checked; nil → production-like)
Now: RAILS_ENV=nil + NODE_ENV=development → does not raise (mirrors Node-side behavior)

This is intentional and correct, but it changes existing startup behavior for anyone running with RAILS_ENV unset and NODE_ENV=development. The test "does not raise when RAILS_ENV is unset and NODE_ENV is development" captures this, but a CHANGELOG entry would help operators know to expect the change.

Minor

Two specific notes left as inline comments:

1. Missing test: NODE_ENV=test + RAILS_ENV=nil — symmetric to the added NODE_ENV=development + RAILS_ENV=nil test, but not covered.
2. Error matrix: The old (any other) — RENDERER_PASSWORD required catch-all was dropped; adding it back (or an equivalent) makes the table exhaustive for cases like NODE_ENV=staging alone.