Walkthrough

Added a REACT_ON_RAILS_PRERENDER_OVERRIDE environment variable to globally override prerender behavior; RenderOptions#prerender now checks this env var (normalized "true"/"false") before per-call prerender: and initializer default. Docs and tests updated to document and validate the new precedence.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble on code beneath the moon,
A tiny var makes prerender tune.
True hops forward, False stays home,
Tests now run without an SSR comb.
Docs and specs—my carrot's in bloom.

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

The feature is well-scoped and the intent is clear. Three issues worth addressing before merge:

Must-fix

Warning floods logs on every render (see inline comment on render_options.rb)
prerender_env_override is called on every #prerender invocation, so an invalid env var value will emit a Rails.logger.warn on every component render on every request. In a busy app that is high-traffic this means thousands of warnings per minute. The env var is a boot-time global, so it should be parsed and cached once at class-load time rather than re-evaluated per render.

Should-fix

around hook scope too broad (see inline comment on the spec)
The env cleanup hook is defined at the top level of the describe block, wrapping every test in the file. It should be scoped to just the #prerender env override block.

Missing CHANGELOG entry
Per project guidelines, user-visible features need an entry under #### Added in CHANGELOG.md. This adds a new global override mechanism that operators need to know about.

Nit

The invalid-value test sets up the expect(Rails.logger).to receive(:warn) expectation after constructing opts but before calling opts.prerender. It works because the method is lazy, but adding .once and moving the expectation to the top of the test would make the contract explicit and more resilient to refactoring. See inline comment.

Greptile Summary

This PR introduces a REACT_ON_RAILS_PRERENDER_OVERRIDE environment variable that can force server-side rendering on or off globally, overriding both per-component prerender: options and the initializer default. The implementation is clean and fits naturally into the existing RenderOptions precedence chain (env → component option → config default).

Key changes:

• render_options.rb: Adds prerender_env_override private method that reads and normalises the env var, logs a warning for invalid values, and returns nil to fall through to existing logic.
• render_options_spec.rb: Adds an around hook for safe env isolation and three new examples covering the happy paths and invalid-value fallback.
• view-helpers-api.md: Documents the new env var and its precedence.

Minor concerns:

• prerender_env_override is called on every render. If the env var is set to an invalid value, Rails.logger.warn fires on every rendered component, which could flood production logs. Caching the parsed result at the class level would eliminate both the per-request overhead and the warning noise.
• In the invalid-value spec, the expect(Rails.logger).to receive(:warn) message expectation is set up after the object is instantiated but before #prerender is called — technically correct, but moving it before described_class.new would follow conventional RSpec ordering and make the test more robust against future refactors.

Confidence Score: 5/5

• This PR is safe to merge — the feature is well-scoped, correctly implemented, and fully tested.
• The core logic is simple and correct: env var lookup → normalise → return boolean or nil → fall through to existing precedence chain. The around hook correctly isolates tests. The two P2 suggestions (potential warn-flood on invalid values, and spec ordering convention) are non-blocking quality improvements that don't affect production correctness in normal use (invalid env var value is an operator error, not a code bug).
• No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["#prerender called"] --> B["prerender_env_override"]
    B --> C{"REACT_ON_RAILS_PRERENDER_OVERRIDE\nenv var set?"}
    C -- "No (nil)" --> D["return nil"]
    D --> E["retrieve_configuration_value_for(:prerender)"]
    E --> F{"options[:prerender]\nexplicitly set?"}
    F -- "Yes" --> G["Return component-level option"]
    F -- "No" --> H["Return ReactOnRails.configuration.prerender"]
    C -- "Yes" --> I{"Value is\n'true' or 'false'?"}
    I -- "Yes (case-insensitive)" --> J["Return true / false"]
    I -- "No (invalid)" --> K["Rails.logger.warn\nIgnoring ..."]
    K --> D

_{Reviews (1): Last reviewed commit: "Document prerender override in changelog" | Re-trigger Greptile}

Review Summary

The feature is well-scoped and useful for CI/test environments. The implementation is clean and the documentation is clear. One meaningful issue and a couple of minor notes:

Warning: Missing memoization causes warning spam (real bug)

prerender_env_override is invoked on every call to #prerender. If a page renders 20 components and REACT_ON_RAILS_PRERENDER_OVERRIDE is set to a typo'd value (e.g. 1 or yes), you'll get 20 identical warnings per request — potentially burying the real misconfiguration signal in production logs. See inline comment for a suggested fix using instance_variable_defined? (needed because false is a valid cached value, making simple ||= memoization incorrect).

Minor notes

• Test: Adding .once to the receive(:warn) expectation in the invalid-value test would make it self-documenting and catch regressions if the warning ever becomes de-duplicated upstream.
• Test isolation comment: The around block is necessary to protect the existing configurable_options loop tests for :prerender, but this dependency isn't obvious — a brief comment would help future maintainers understand why the hook is at the top-level describe scope.

Looks good

• Precedence logic is correct (env > component option > config default).
• raw_value.strip.downcase handles whitespace and casing nicely.
• The around block correctly restores CI env state.
• CHANGELOG and docs are updated.

Code Review

The implementation is clean and the precedence logic is correct. A few issues worth addressing.

Significant: Warning fires on every component render

prerender_env_override is called on every invocation of #prerender (once per component per request). When REACT_ON_RAILS_PRERENDER_OVERRIDE is set to an invalid value, Rails.logger.warn fires for every component rendered on every request — very noisy in production.

Suggested fixes: (1) warn once at startup via a class-level flag, or (2) memoize the parsed result on the instance since the env var will not change mid-request.

Minor: Global config mutation not cleaned up in test

In the invalid-value fallback test, ReactOnRails.configuration.prerender = true is set but never restored, risking test pollution for later examples that rely on the default config value.

Minor: Missing explicit "env var absent" test in the new describe block

The new describe "#prerender env override" group does not include a case verifying that when the env var is absent the normal configuration chain applies. It is implicitly covered by existing tests but an explicit case would make the group self-documenting.

Nit: Unrelated analysis/open-issue-deep-dive/ changes

The wave-1/2/3 markdown edits appear to be tooling churn unrelated to this feature. Consider dropping them to keep the diff focused.

Review Summary

The feature is well-scoped and the implementation is clean. Two issues worth addressing before merging:

1. Warning flood in production (see inline comment on render_options.rb)

prerender_env_override is called on every #prerender invocation. With an invalid env value, Rails.logger.warn fires on every component render, rapidly flooding production logs. The warning should fire only once per process — e.g. via a class-level flag set after the first warn.

2. Missing test coverage (see inline comment on spec)

None of the three new specs tests the env override when no explicit prerender: option is given at the component level — i.e. env overriding the config default rather than an explicit component option. Should be covered.

Minor observations

• The around block correctly handles test isolation — good practice.
• The configurable_options shared test for :prerender still passes correctly because the around block clears the env var before each example.
• ENV.fetch(key, nil) vs ENV[key] is a minor style point; both work fine.

Overall a useful escape hatch for CI/test environments. The log flooding issue is the main concern.

PR Review: Add REACT_ON_RAILS_PRERENDER_OVERRIDE global prerender override

The feature direction is sound and fills a real gap (disabling SSR in CI/test environments). The implementation is mostly clean, but there are a few concerns worth addressing before merge.

Thread Safety (High)

RenderOptions.prerender_env_override reads and writes the class-level instance variable @prerender_env_override_cache without any synchronization. In a multi-threaded server (Puma, Falcon) two threads can interleave between the read and the write. The worst case is double-parsing (idempotent), but the class-level mutable state is an unnecessary risk. ENV does not change between requests in production, so the cache buys very little. Consider either removing the cache entirely (ENV reads + one hash lookup are negligible) or protecting writes with a Mutex.

Unnecessary Caching Complexity (Medium)

The caching exists mainly to emit the invalid-value warning only once. A simpler alternative: parse the env var once at class-load time, or just let the cheap parse run on every call. The current design leaks class-level mutable state across test examples and couples tests to caching behavior (see below).

Test: .once expectation couples test to caching implementation (Medium)

The .once assertion on the Rails.logger.warn expectation passes only because the class-level cache suppresses the second parse call. If caching is ever removed or refactored, this test fails with a misleading error. The test should assert observable behavior (value falls back, warning is emitted) without pinning how many times the warning fires internally. Use .at_least(:once) or avoid calling prerender twice in the same example.

with_prerender_env_override_cleared does not reset the class cache (Low)

The helper restores the ENV variable correctly but leaves @prerender_env_override_cache populated. Tests pass today because the cache comparison re-detects any ENV change. This is a subtle dependency: a future caching refactor could silently break test isolation. An explicit cache reset in the ensure block would make the isolation explicit.

build_render_options_with_clean_prerender_env helper complexity (Low)

This helper branches on the option name to decide whether to clear the env. A single top-level around block that unconditionally clears the env and the class cache for the entire spec would achieve the same isolation without the per-call conditional.

Overall the feature is well-scoped and the test coverage is appreciated. Thread safety and test brittleness are the main things to resolve before merge.

Addressed the blocking review items in latest push:\n\n- Added class-level caching of parsing so invalid values no longer warn on every render path.\n- Kept cache keyed by raw env value so changed env values are re-parsed, while stable values avoid repeated warning/log overhead.\n- Scoped env cleanup into helper usage + block, and added explicit config restore in to avoid global-state leakage.\n- Updated invalid-value spec to assert warning fires once and added explicit no-env-override precedence coverage.\n\nLocal verification:\n- Inspecting 2 files
..

2 files inspected, no offenses detected\n- Run options: include {focus: true}

All examples were filtered out; ignoring {focus: true}
...........................

Finished in 0.00869 seconds (files took 0.82004 seconds to load)
27 examples, 0 failures

Run options: --seed 1121

Running:

Finished in 0.000455s, 0.0000 runs/s, 0.0000 assertions/s.

0 runs, 0 assertions, 0 failures, 0 errors, 0 skips

Review Summary

The feature itself is well-conceived — a global REACT_ON_RAILS_PRERENDER_OVERRIDE env var with clear precedence (env > component option > config default) is exactly the right UX for CI/test environments. The test coverage is thorough and the docs update is clear.

However, there are a few implementation concerns worth addressing before merging:

Critical: Mutex on the render hot path

PRERENDER_OVERRIDE_CACHE_MUTEX.synchronize is called on every component render. In high-concurrency Puma/multi-threaded environments, a single global Mutex on the render path is a contention bottleneck. See the inline comment on prerender_env_override — the cache re-reads ENV.fetch on every call anyway, so the mutex adds overhead without preventing ENV reads. The simplest fix is to either:

• Cache once at startup (ENV is static after boot)
• Or drop the cache/mutex entirely and just parse on each call (it's a hash lookup, effectively free)

Medium: Logger called inside mutex

Rails.logger.warn is invoked while holding PRERENDER_OVERRIDE_CACHE_MUTEX. Logger subscribers can involve I/O or their own locks, which risks deadlock. See the inline comment for a suggested refactor to emit the warning outside the critical section.

Minor: reset_prerender_env_override_cache! is public API

This method exists only to support test isolation. Leaving it as a public class method means application code could accidentally call it in production, corrupting the cache mid-flight in a multi-threaded app. Consider guarding it with a Rails.env.test? check or noting it clearly as test-only.

Minor: Inconsistent cache-reset discipline in specs

Several tests set ENV[...] directly without calling reset_prerender_env_override_cache! afterward. They pass today because the around block clears the cache first, but the pattern is fragile. See the inline comment on the spec.

Overall: Good feature, clean implementation approach — just wants a simpler/safer caching strategy before landing.

Review: Add REACT_ON_RAILS_PRERENDER_OVERRIDE global prerender override

Overall the feature is well-scoped and the implementation is correct. Four issues worth addressing before merge:

Bug — CHANGELOG entry is malformed (must fix)

The new entry was inserted between PR 2818's by keyword and its author attribution, so PR 2818 loses its author link and it visually attaches only to PR 2816. See inline comment on CHANGELOG.md.

Performance — mutex acquired on every prerender call

ENV.fetch is called inside PRERENDER_OVERRIDE_CACHE_MUTEX.synchronize on every invocation, adding lock contention per rendered component. Since env vars don't change at runtime in production, a simple double-checked locking pattern (read ENV once, cache forever, never re-enter the mutex) would be zero-overhead after the first call. See inline comment on render_options.rb.

Test — "warn once" assertion tests cache behavior, not the stated intent

The .once expectation on the invalid-value test passes because the second opts.prerender call hits the cache rather than because the logger was deduplicated. The test should use two separate RenderOptions instances to actually demonstrate that the class-level cache prevents repeated warnings across distinct objects. See inline comment on the spec.

Minor — reset_prerender_env_override_cache! leaks an implementation detail

This method is public only to enable test isolation. Worth at minimum documenting it as test-only, or moving it into a spec support helper that reopens the class, to avoid accidental use in application code.