Walkthrough

The doctor now prefers runtime configuration from ReactOnRails.configuration (when Rails can be loaded) over regex-parsing the initializer. Added memoized helpers for accessing runtime and Pro renderer values, and updated analysis/check helpers to accept and prefer runtime-derived values with initializer parsing as fallback.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through configs, sniffed the runtime air,
No more brittle regex traps hiding there.
Memoized helpers keep secrets neat and small,
The Doctor now listens to the app above all. 🥕✨

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 upgrades the ReactOnRails::Doctor diagnostic tool to query live runtime configuration values (via ReactOnRails.configuration / ReactOnRailsPro.configuration) when the Rails environment can be loaded, falling back to regex-parsing the initializer file when it cannot. The change fixes the long-standing issue (#2771) where doctor output could diverge from what the app actually uses at runtime (e.g., config values set dynamically or by env variables).

Key changes:

• Two new memoised helpers — react_on_rails_runtime_configuration and resolved_pro_server_renderer — wrap env-load and config-access with error handling.
• All six analyze_* methods accept an optional runtime_config parameter and branch on it.
• check_server_bundle_prerender_consistency and server_bundle_filename likewise prefer runtime values.
• New specs cover runtime-over-file precedence, runtime NodeRenderer detection, and runtime vs. initializer conflicts.

Issues found:

• react_on_rails_runtime_configuration uses a trailing if for assignment, so when ensure_rails_environment_loaded returns false, @react_on_rails_runtime_configuration is never defined and the defined? memoization guard never fires — every call re-enters the method body.
• check_server_bundle_prerender_consistency still guards on File.exist?("config/initializers/react_on_rails.rb"), silently skipping the consistency check when the file is absent even though runtime config would provide all necessary values.
• analyze_component_loading_config reports random_dom_id when the value is nil (not configured) as well as false, which could produce misleading output.

Confidence Score: 3/5

• Safe to merge for most apps, but two logic gaps in the new runtime-config path mean the doctor can silently give incomplete output and has a caching bug that causes repeated work.
• The core concept is sound and tests pass. However, the memoization bug in react_on_rails_runtime_configuration (repeated method body re-entry when env load fails) and the unclosed guard in check_server_bundle_prerender_consistency (consistency check silently skipped when initializer isn't at the default path despite runtime config being available) are genuine logic issues that touch the primary user path of this PR. Neither causes data loss, but they undermine the PR's stated goal of preferring runtime values.
• react_on_rails/lib/react_on_rails/doctor.rb — specifically the new react_on_rails_runtime_configuration helper (memoization) and the guard in check_server_bundle_prerender_consistency.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Doctor check invoked] --> B{ensure_rails_environment_loaded?}
    B -- yes --> C[react_on_rails_runtime_configuration\n= ReactOnRails.configuration]
    B -- no --> D[react_on_rails_runtime_configuration = nil]

    C --> E{runtime_config present?}
    D --> E

    E -- yes --> F[Use live config values\nserver_bundle_js_file, prerender,\npool_size, timeout, etc.]
    E -- no --> G[Regex-parse initializer file\nconfig/initializers/react_on_rails.rb]

    F --> H[check_react_on_rails_initializer\ncheck_server_bundle_prerender_consistency\nserver_bundle_filename]
    G --> H

    B -- yes --> I{defined? ReactOnRailsPro?}
    I -- yes --> J[resolved_pro_server_renderer\n= ReactOnRailsPro.configuration.server_renderer]
    I -- no --> K{pro_initializer_has_node_renderer?}
    K -- yes --> L[resolved_pro_server_renderer = NodeRenderer]
    K -- no --> M[resolved_pro_server_renderer = nil]
    J --> N[check_server_rendering_engine]
    L --> N
    M --> N

_{Reviews (1): Last reviewed commit: "Use runtime React on Rails config in doc..." | Re-trigger Greptile}

test

Superseded by the follow-up comment with corrected formatting: #2823 (comment)

Addressed the Greptile findings in commit 2bd1c3a.\n\nChanges made:\n- Fixed runtime-config memoization so nil is memoized when Rails environment cannot be loaded, preventing repeated re-entry.\n- Updated the server bundle/prerender consistency check to run when runtime config is available even if config/initializers/react_on_rails.rb is missing.\n- Tightened random_dom_id runtime reporting so it is emitted only when explicitly false (not nil).\n- Added regression coverage for all three cases in doctor specs.\n\nRe-validated:\n- bundle exec rspec react_on_rails/spec/lib/react_on_rails/doctor_spec.rb\n- bundle exec rubocop react_on_rails/lib/react_on_rails/doctor.rb react_on_rails/spec/lib/react_on_rails/doctor_spec.rb

Review Summary

The direction is good — preferring runtime config over regex-parsing is more reliable and handles dynamic configuration (env-based values, computed expressions, multi-file initializers) that static parsing can never get right.

Bug

`server_bundle_filename` empty-string fallback (see inline at line 1299): when `server_bundle_js_file` is explicitly set to "" (SSR disabled), `configured_value.present?` is false so the method falls through to regex/default and returns "server-bundle.js". Downstream, `determine_server_bundle_path` will then construct a path for a file that does not exist and the doctor may report false-positive warnings about a missing bundle.

Design inconsistencies

1. Runtime path reports default values (inline at lines 843-844): `server_renderer_pool_size`, `server_renderer_timeout`, `raise_on_prerender_error`, and `enforce_private_server_bundles` are now always emitted in the runtime path even when they hold factory defaults. The fallback path only prints them when explicitly set. Upgraders will see more output in the runtime path without having changed anything.

2. `random_dom_id` runtime filter (inline at line 1018): only surfaced when == false; an explicit = true is silently skipped. A short comment stating the default value would make the intent unambiguous.

3. `analyze_development_config` runtime path: always prints trace, logging_on_server, and replay_console regardless of whether they differ from defaults, while the fallback path omits them unless explicitly set.

Minor

The `react_on_rails_runtime_configuration` memoization is correct. One edge case worth a comment: if `ReactOnRails.configuration` raises (rescue fires), `ensure_rails_environment_loaded` remains true, so `resolved_pro_server_renderer` will still attempt `ReactOnRailsPro.configuration`. Not a safety issue, just worth documenting. (See inline at line 2334.)

Tests

The new specs are well-structured and cover the key scenarios (runtime-over-file precedence, nil memoization, NodeRenderer detection). The `around { Dir.mktmpdir }` pattern is clean. One suggestion: add a test for the `server_bundle_filename` empty-string case to lock in the intended behaviour once the bug above is resolved.

Review Summary

The core idea — prefer ReactOnRails.configuration runtime values over regex-parsing the initializer file — is sound and fixes a real class of diagnostic inaccuracies. The memoization pattern using defined?(@ivar) to distinguish un-set from nil is correct and well-tested.

A few things worth addressing before merge:

Bugs / correctness

resolved_pro_server_renderer memoization order (inline comment on line 2352–2353)
The return nil unless react_on_rails_pro? guard fires before the defined?(@ivar) check. For non-Pro apps the instance variable is never written, so react_on_rails_pro? is called on every invocation. Swap the two lines so the memoized value short-circuits first.

Diagnostic output consistency

server_renderer_pool_size / server_renderer_timeout always emitted with runtime config (inline comment on line 852)
The runtime path unconditionally prints these; the regex fallback only prints them when explicitly set. At runtime they're always present (default values), so users will see extra lines they never saw before. Either guard on non-default values or document the intentional verbosity increase.

same_bundle_for_client_and_server / random_dom_id asymmetry (inline comment on lines 1023–1026)
One is shown when truthy, the other when == false. The logic is correct (both suppress their respective defaults) but the asymmetry is confusing without a comment explaining the defaults.

Scope

Unrelated TypeScript refactors (inline on create-app.ts)
The ternary-to-if-else changes in create-react-on-rails-app are functional no-ops that have nothing to do with Doctor. They add review noise and should live in a separate commit/PR.

Overall the PR is a solid improvement. The three items above are all small fixes.

size-limit report 📦

Review Summary

The core idea is solid — querying ReactOnRails.configuration at runtime is strictly more accurate than regex-parsing the initializer text, and the fallback chain (runtime → file parsing → defaults) is well-structured. The TypeScript refactors in create-react-on-rails-app are clean improvements.

Issues found

Bug — nil vs. "" conflation in analyze_server_rendering_config (see inline comment on line 800)

runtime_config.server_bundle_js_file returns nil when the user never set it (system default "server-bundle.js" applies) and "" when they explicitly disabled SSR. The current code collapses both to "disabled", which is misleading for the common case where the option is simply unconfigured. The initializer-parsing fallback handles this correctly by showing "server-bundle.js (default)" when nothing is found.

Minor — memoization bypass in resolved_pro_server_renderer (see inline comment on line 2354)

The return nil unless react_on_rails_pro? guard fires before the defined?(@resolved_pro_server_renderer) check, so every call hits react_on_rails_pro? again when Pro is inactive. Contrast with react_on_rails_runtime_configuration, which always writes through to the ivar. Low impact today since the method is called once per run, but worth aligning for consistency.

UX — spurious warning when runtime config is available but no initializer file (see inline comment on line 766)

When the Rails environment loads successfully and ReactOnRails.configuration is readable, emitting a warning because the initializer file is absent is noisy — the configuration is provably correct. Consider downgrading to add_info in that case, reserving add_warning for when runtime config is also unavailable.

Positive notes

• Memoization via defined?(@ivar) is the right pattern for caching nil; the explanatory comment helps.
• ensure_rails_environment_loaded is correctly guarded against repeated attempts via @rails_environment_attempted.
• New specs cover the key precedence scenarios: runtime over initializer, blank server_bundle_js_file, and memoization of nil on load failure.
• resolved_pro_server_renderer cleanly isolates Pro renderer detection from the check methods that consume it.

Review

Well-structured refactor that improves diagnostic accuracy by preferring runtime config over brittle regex parsing. The defined?(@ivar) memoization to cache nil is correct and well-commented. Test coverage of runtime-vs-initializer precedence is thorough.

Behavioral change worth documenting: the runtime path now omits settings at their default values (pool size, timeout, trace, logging_on_server, etc.), whereas the initializer-fallback path always showed defaults explicitly. Users auditing their full config may be surprised when entries silently disappear. Adding a note in the doctor output like 'Default-valued settings are omitted (see docs)' would make this self-documenting.

ensure_rails_environment_loaded warning deduplication confirmed: both react_on_rails_runtime_configuration and resolved_pro_server_renderer call ensure_rails_environment_loaded independently. @rails_environment_attempted is set before the require call, so if require throws, the rescue returns false but the flag stays set. The warning fires exactly once across all callers. Good design.

analyze_server_rendering_config falsy non-nil edge: the present? / nil? / else triple-branch silently maps false to the empty-string-disabled message. server_bundle_js_file should always be String or nil per the schema so this is harmless in practice, but a comment near is_a?(String) would clarify intent for future readers.

create-app.ts simplification looks good. The refactor preserves behavior correctly and the type narrowing to the literal union type is an improvement.

Addressed the outside-diff fallback warning concern from review #3996919903 in commit 7df8bb25.

Change made:

• check_server_rendering_engine now checks whether renderer_use_fallback_exec_js is actually enabled before warning when ExecJS is absent under NodeRenderer.
• If fallback is disabled, Doctor now reports that explicitly instead of showing a false warning.

Code Review

Overall well-structured. Moving Doctor diagnostics from brittle regex parsing to runtime config queries is the right call, and the fallback chain is clearly documented. Test coverage is thorough.

Correctness

1. Silent fallback when Rails is loaded but ReactOnRailsPro is not defined

In resolved_pro_server_renderer, when rails_environment_loaded is true but defined?(ReactOnRailsPro) is false, the code silently falls through to pro_initializer_has_node_renderer? (regex parsing) without warning. Rails loaded but Pro constant missing likely indicates a gem load error. The regex result could be a false positive and the user gets no signal. A distinct warning is needed here.

2. server_bundle_output_path always reported - inconsistent with other fields

Every other field in the runtime-config path suppresses output when the value matches the default (pool size, timeout, raise_on_prerender_error, etc.). But server_bundle_output_path is always printed regardless of whether it equals DEFAULT_SERVER_BUNDLE_OUTPUT_PATH. This creates noise for apps that have not changed the default.

3. Misleading label for the server_bundle_value.nil? case

When runtime config returns nil for server_bundle_js_file, the code falls back to server_bundle_filename and displays server-bundle.js (initializer/default). This label appears even when there is no initializer -- the value is just the hardcoded Ruby default. Consider distinguishing the two sub-cases in the label (e.g. (default) vs (from initializer)).

Minor

4. Non-obvious nil-guard pattern in config_path_resolver.rb lines 79-82

The two-statement guard (warn unless Rails.logger then Rails.logger&.debug) is correct but easy to misread as both statements always executing. A single if/else would make the mutual exclusion explicit.

5. .uniq is load-bearing correctness in webpack_config_candidates

A Shakapacker assets_bundler_config_path equal to a WEBPACK_DEFAULT_CONFIG_CANDIDATES entry (e.g. config/webpack/webpack.config.js) would be inserted twice and corrected only by the trailing .uniq. The existing comment describes this as accidental duplication; it would be clearer to note that standard-path Shakapacker configs intentionally rely on .uniq.

Review: Doctor runtime config migration

Overall this is a solid improvement — replacing fragile regex parsing of initializer files with ReactOnRails.configuration runtime values is the right direction, and the graceful fallback chain (runtime → initializer regex → hardcoded default) is well-structured. The memoization approach using instance_variable_defined?/defined? for nil-able results is idiomatic and correct throughout.

Issues worth fixing

1. Misleading warning text (doctor.rb:690) — see inline comment. The "ExecJS fallback is enabled but ExecJS is not available" message fires when the ExecJS gem is defined (so defined?(ExecJS) is truthy) but ExecJS.runtime is nil. ExecJS is available in that case; only the runtime is unconfigured. The message will confuse users who have ExecJS installed but no runtime gem (e.g. mini_racer).

2. resolved_webpack_config_path not memoized (config_path_resolver.rb:27) — see inline comment. shakapacker_assets_bundler_config_path was explicitly memoized to avoid re-requireing Shakapacker on each call; resolved_webpack_config_path deserves the same treatment since it's called from bundler_config_file_exists?, detect_bundler_config_path, and existing_bundler_config_paths (via detect_bundler_config_path), each of which rebuilds the candidate list and issues N×File.file? syscalls.

Worth considering

3. Dual-path boilerplate in analyze_* methods — each of the six analyze methods now has an if runtime_config … else … end mirroring every setting, plus three Rubocop cop disables. A small resolve_config_value(runtime_config, content, regex) { |cfg| ... } abstraction (suggested in the inline comment) would cut the per-method branch count roughly in half and make adding new settings much cheaper.

4. defined?(ReactOnRailsPro) doesn't guard NameError from a broken autoload — rescue already catches StandardError/LoadError; adding NameError to the rescue clause in pro_execjs_fallback_enabled? would close the last gap consistently with how resolved_pro_server_renderer is guarded.

Looks good

• TypeScript refactor in create-app.ts is clean and equivalent.
• File.exist? → File.file? is the right fix (directories would previously pass).
• nil-vs-"" semantics for server_bundle_js_file (disabled vs unset) are consistently handled in server_bundle_filename, check_javascript_bundles, analyze_server_rendering_config, and check_server_bundle_prerender_consistency.
• Test coverage is thorough — the new specs covering runtime-over-file precedence, whitespace-blank bundle values, memoization, and Pro renderer resolution are well-targeted.
• Path normalization in normalize_shakapacker_assets_bundler_config_path correctly guards the Rails.root="/" edge case.

Thanks for the detailed review — this is helpful.

Status update on the points you listed:

1. doctor.rb:690 warning wording: acknowledged. Current logic is intentionally keyed on runtime availability (defined?(ExecJS) && ExecJS.runtime) for the fallback path, but I agree the phrasing can be clearer when ExecJS is present but runtime is not configured.
2. resolved_webpack_config_path memoization: acknowledged as a valid optimization; currently left as follow-up work.
3. analyze_* dual-path boilerplate: agreed this is a refactor opportunity; deferred to keep this PR scoped to behavior fixes.
4. NameError hardening in pro_execjs_fallback_enabled?: good suggestion; also deferred for scope control.

Separately, I did address the actionable resolver bug around candidate skipping (sibling bundler candidate loss) in commit 04a8ebc9, with regression coverage in doctor_spec.