Walkthrough

Adds three new abstractions—RenderRequest, RenderingStrategy (with ExecJsStrategy), and JsCodeBuilder—plus Pro-specific subclasses and engine wiring; exposes module-level accessors for strategy and builder. Changes are additive; core rendering path is not yet swapped.

Changes

Sequence Diagram(s)

sequenceDiagram
  participant Helper as Helper (caller)
  participant RenderReq as RenderRequest
  participant Builder as JsCodeBuilder
  participant Strategy as RenderingStrategy
  participant Pool as ServerRenderingPool

  Helper->>+RenderReq: build render_request(...)
  RenderReq-->>-Helper: return RenderRequest
  Helper->>+Strategy: reset_if_bundle_changed()
  Strategy-->>-Helper: (maybe resets pool)
  Helper->>+Strategy: execute(render_request)
  Strategy->>+RenderReq: render_request.to_js
  RenderReq-->>-Strategy: JS string
  Strategy->>+Pool: exec_server_render_js(js, render_options)
  Pool-->>-Strategy: rendered result
  Strategy-->>-Helper: result

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add RBS type signatures for improved type safety #1945 — Overlapping RBS/signature additions for RenderingStrategy, JsCodeBuilder, and RenderRequest.

Poem

🐇 I stitched requests into tidy stacks,

Sections hum and strategies track,
Props escape, JS wraps tight,
Exec or Node — ready to write.
Hooray! — a rabbit’s little delight 🥕

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: Strategy Pattern Refactoring (Steps 1+2)

Overall this is a clean, well-scoped additive PR. The three abstractions map clearly to their purpose, the existing rendering path is untouched, and the 23 new specs are well-structured. A few things worth addressing before Step 3 wires this into production:

Bugs / correctness

1. rendering_strategy can return nil — unlike js_code_builder, which uses ||= to lazily construct a default, rendering_strategy returns @rendering_strategy directly. Any call before the engine initializer fires (e.g. unit tests that don't load the full Rails engine) produces NoMethodError: undefined method 'execute' for nil. See inline comment on configuration.rb:33.

2. validate_server_bundle_configured! uses strict == true — truthy but non-boolean prerender values silently skip validation and allow a render to proceed without a server bundle, leading to a confusing downstream error. See inline on render_request.rb:61.

Design / maintainability

3. Implicit coupling between render_call_section and wrap_in_iife in Pro builder — name: componentName and props: usedProps reference bare JavaScript identifiers that only exist because wrap_in_iife wraps the body in a specific function signature. This coupling is invisible at the Ruby level; renaming either side silently breaks the generated JS. A comment documenting the contract would help. See inline on react_on_rails_pro/js_code_builder.rb:83.

4. RenderingStrategy module interface stubs and :nocov: — the pattern is idiomatic Ruby, but be aware that include makes these stubs into real instance methods that are silently shadowed when a subclass defines the same method. If a subclass accidentally omits one, the NotImplementedError only appears at runtime, not load time. This is fine as-is; just worth documenting for future implementors.

Minor observations

• The renderingRequest JavaScript variable in rsc_payload_function_section is a node-renderer global — confirmed by cross-referencing the original ServerRenderingJsCode.generate_rsc_payload_js_function. The new builder faithfully preserves the original behaviour here.
• Engine initializer ordering relies on Pro loading after core (currently guaranteed by gem dependency order), which the ||= guard in the core initializer already handles correctly.
• Tests are thorough for the new surface; the only gap is a unit test confirming rendering_strategy returns a usable default when no engine has run (which would also catch the nil regression).

🤖 Generated with Claude Code

Greptile Summary

This PR introduces three new additive abstractions — RenderRequest, JsCodeBuilder, and RenderingStrategy — as the first two steps of a larger refactoring (#2905) to replace runtime react_on_rails_pro? checks with boot-time strategy objects. All new code is additive and correctly not yet wired into the main rendering path in helper.rb. The new Pro JsCodeBuilder is a faithful port of ServerRenderingJsCode, preserving the same IIFE signature and renderingRequest outer-scope reference used by the Node renderer.

Confidence Score: 5/5

Safe to merge — all changes are additive, existing rendering path is untouched, and new code has 23 passing specs.

All findings are P2 style/consistency suggestions (lazy-init inconsistency between the two accessors, module-level methods placed in configuration.rb, misleading method name in Pro builder). No P0/P1 defects were identified. The new code faithfully ports the existing JS generation logic and the engine initializer ordering is correct.

react_on_rails/lib/react_on_rails/configuration.rb — the rendering_strategy getter lacks a lazy default that js_code_builder has, which will need addressing before Step 3 wires the strategy into the live path.

Important Files Changed

_{Reviews (1): Last reviewed commit: "refactor: add RenderRequest, JsCodeBuild..." | Re-trigger Greptile}

Review Comment Summary

Addressed (1):

• ✅ configuration.rb:33 — Added lazy default for rendering_strategy to match js_code_builder pattern (01579a2). Threads resolved.

Declined with rationale (4):

• render_request.rb:61 — prerender == true strict check intentionally mirrors existing ServerRenderingJsCode pattern
• pro/js_code_builder.rb:83 — IIFE parameter coupling comment is a documentation nit, out of scope
• pro/js_code_builder.rb:109 — Method rename is a style preference, out of scope for additive PR
• configuration.rb:50 — Accessor placement is a code organization preference, out of scope

Duplicates (1):

• greptile lazy-init comment — duplicate of claude[bot] comment, addressed together

Review: Step 1+2 Refactoring (RenderRequest, JsCodeBuilder, RenderingStrategy)

The overall structure is clean and the strategy pattern split makes sense. The code faithfully mirrors the existing Pro/core JS generation logic. A few things worth addressing before or during Step 3:

🔴 Step 3 footgun: rails_context type mismatch

RenderRequest documents and expects rails_context to be a Hash, and rails_context_json calls .to_json on it. However, the existing call site in helper.rb:766 already serializes it:

rails_context: rails_context(server_side: true).to_json,   # already a JSON string

When Step 3 wires RenderRequest into the helper, passing the current .to_json result would double-encode the context into a JSON-escaped string literal instead of an object. The call site will need to drop the .to_json. Suggest a # NOTE(Step 3) comment in rails_context_json — or even a runtime guard (raise if rails_context.is_a?(String)) to catch the mistake early.

🟡 Redundant lazy initialization for rendering_strategy

configuration.rb defines:

def self.rendering_strategy
  @rendering_strategy ||= ReactOnRails::RenderingStrategy::ExecJsStrategy.new
end

And engine.rb then adds:

ReactOnRails.rendering_strategy ||= ReactOnRails::RenderingStrategy::ExecJsStrategy.new

Because the getter always returns a non-nil value, the ||= in the initializer is always a no-op — the getter fires first, sets @rendering_strategy, and the RHS of ||= is never evaluated. The engine initializer creates a second ExecJsStrategy object on every boot that is immediately discarded. Pick one ownership model: either the getter provides the default or the initializer does, not both.

🟡 Asymmetry: js_code_builder has no engine initializer

rendering_strategy gets an explicit boot-time initializer; js_code_builder only has the getter's ||=. If someone clears @js_code_builder = nil in a test, the getter silently re-creates a core builder — but there's no Pro initializer that would re-set the Pro builder in that scenario (since Pro's engine only runs once at boot). The asymmetry isn't harmful today (since the code is additive), but it could cause surprising test pollution once Step 3 lands.

🟡 renderingRequest global assumption undocumented (Pro JsCodeBuilder)

In the non-RSC-payload-streaming branch of rsc_payload_function_section, the generated JS references renderingRequest as a free variable:

railsContext.serverSideRSCPayloadParameters = {
  renderingRequest,   // ← where does this come from?
  rscBundleHash: ...
}

This variable is injected by the Node renderer runtime (confirmed by the existing ServerRenderingJsCode which has the same pattern). The new code should carry a comment explaining the assumption, as it's non-obvious to readers unfamiliar with the Pro Node renderer protocol.

🟢 Minor: spec for to_js has an implicit global dependency

render_request_spec.rb tests #to_js by stubbing only ReactOnRails.configuration.server_bundle_js_file, but lets ReactOnRails.js_code_builder resolve to whatever the global @js_code_builder state happens to be. Stubbing the builder explicitly would make the test hermetic and the contract clearer.

Overall this is well-structured foundational work. The concerns above are mostly prep-notes for Step 3 rather than blockers — the code is correctly marked additive-only.

Review comments addressed in c19a744

Fixed (2):

• engine.rb:112 — Removed dead engine initializer for rendering_strategy. The getter's ||= at configuration.rb:32 is now the single canonical default. Pro's engine continues to override via the setter.
• render_request.rb:41 — Added ArgumentError guard in rails_context_json to catch accidental double-encoding when rails_context is passed as a String instead of a Hash. Spec added.

Declined with rationale (4):

• js_code_builder.rb:51 (Pro) — renderingRequest free variable comment: documentation nit, follows existing ServerRenderingJsCode pattern
• render_request_spec.rb:121 — js_code_builder test isolation stub: speculative concern, no other spec modifies this global
• render_request.rb:61 — == true strict check: matches existing ServerRenderingJsCode pattern at line 23
• js_code_builder.rb:83 (Pro) — IIFE parameter coupling comment: follows existing pattern, clear from proximity

All 10 review threads resolved.

Review: RenderRequest, JsCodeBuilder, and RenderingStrategy abstractions

Good architectural direction — separating JS-code generation from execution, and replacing scattered react_on_rails_pro? checks with boot-time strategy injection, is exactly the right decomposition. The additive-only constraint makes this safe to land without touching the hot path.

A few issues worth addressing before the switchover in Step 3:

Substantive issues

1. Missing unicode escaping in rails_context_json (inline on render_request.rb)
props_string explicitly escapes U+2028/U+2029. rails_context_json does not, relying implicitly on ActiveSupport's encoder. Even if that encoder happens to escape them today, the asymmetry is confusing and fragile. Apply the same .gsub treatment.

2. renderingRequest in Pro builder is undocumented ambient state (inline on Pro js_code_builder.rb)
The non-payload RSC section emits renderingRequest as a shorthand property — but that variable is never declared in the generated IIFE. It only exists because the Node renderer injects it into the evaluation context. The original ServerRenderingJsCode has the same implicit contract, but at least there's a nearby comment explaining the pattern. The new builder needs an equivalent comment, otherwise every reader will think it's a bug.

3. Module-level accessors belong in react_on_rails.rb, not configuration.rb (inline on configuration.rb)
rendering_strategy and js_code_builder are module-level methods on ReactOnRails, not slots on the Configuration struct. Placing them in configuration.rb conflates two responsibilities. More practically: ReactOnRails.configure does not reset these ivars, so calling configure again in tests leaves stale strategies behind.

Minor issues

4. compact vs compact_blank in JsCodeBuilder#build
Empty-string sections (e.g. store_initializations = "") are not filtered by compact, producing blank lines in generated JS. compact_blank is already polyfilled in this codebase for Rails < 6.1.

5. Test isolation for RenderRequest#to_js (inline on spec)
The to_js specs exercise the real global ReactOnRails.js_code_builder without resetting it. Stub it explicitly so these specs remain correct when run in the Pro test suite or after other specs mutate the global.

6. RenderingStrategy interface contract (inline on module)
Minor doc nit: add a note that implementers must define instance methods (not class methods) to satisfy the include-based interface.

Overall this is a clean foundation for the Step 3 switchover. Issues 2 and 1 are the most important to address before wiring into the live rendering path.

Review: RenderRequest, JsCodeBuilder, and RenderingStrategy abstractions

Overall this is a clean, well-structured refactor. The additive approach is the right call — keeping the existing path intact while introducing the new abstractions side-by-side makes the transition safe. A few issues worth addressing before Step 3 wires these in:

Issues

1. RBS type mismatch: store_initializations is nullable

render_request.rbs declares attr_reader store_initializations: String but the field can be nil (the spec has a nil case and JsCodeBuilder#build_sections calls .compact specifically to handle it). Should be String?.

2. configure reset won't survive Pro test setups

When ReactOnRails.configure {} is called inside a test (a common pattern in this repo), @rendering_strategy and @js_code_builder are nilled. The lazy ||= fallback then returns a fresh ExecJsStrategy/JsCodeBuilder — not Pro's overrides — because after_initialize only fires once at boot and won't re-run. This is harmless now (additive only), but should be addressed before Step 3 or any Pro unit tests that call configure will silently use the wrong strategy. One approach: have the Pro engine expose a restore_rendering_strategy! class method callable from test helpers, or wire the accessors so configure invokes Pro's override if the Pro engine is loaded.

3. Minor thread-safety note on ||=

The ||= idiom on module instance variables (@rendering_strategy, @js_code_builder) is not atomic. In a Puma multi-threaded setup two threads could both see nil after a configure call and race to initialize the default. The worst outcome is creating two throwaway objects (last write wins, no data corruption), but it's worth noting since these are global mutable singletons.

4. Implicit renderingRequest dependency in Pro builder

In rsc_payload_function_section, the generated JS references renderingRequest — a variable that lives in the Node renderer's outer execution scope, not defined anywhere in the IIFE. This matches the existing ServerRenderingJsCode behavior exactly (so it's correct), but the implicit contract between the Ruby code generator and the Node runtime is undocumented. A short comment citing where renderingRequest comes from would prevent future confusion.

Minor

• validate_server_bundle_configured! uses render_options.prerender == true (strict equality). Consistent with the existing ServerRenderingJsCode check, so not a regression.
• props_string silently passes through a non-JSON string while rails_context_json raises on non-Hash. The asymmetry is fine for now but worth standardizing during Step 3.

What's good

• Unicode escape handling (\u2028/\u2029) is correctly applied in both props_string and rails_context_json.
• The section-based override design (build_sections) is significantly cleaner than duplicating heredocs; Pro only overrides what differs.
• rails_context_json raising ArgumentError on non-Hash prevents double-encoding bugs — good defensive check.
• 23 new specs with good coverage of edge cases (nil stores, unicode, Hash vs String props).

Review comments addressed in 6a44410

Fixed (2):

• render_request.rbs:6,13 — Fixed RBS type for store_initializations: String → String? in both attr_reader and initialize signature (@claude[bot])
• configuration.rb:29-30 — Removed premature @rendering_strategy = nil / @js_code_builder = nil reset from configure — Pro only sets these once during after_initialize, so resetting them on reconfigure would silently drop Pro behavior. Since the strategy path is additive-only, the reset was unnecessary; proper reconfigure mechanism deferred to Step 3. (@chatgpt-codex-connector[bot], @claude[bot])

Declined with rationale (3):

• js_code_builder.rb:40-64 — typeof guard on generateRSCPayload: faithful port of existing server_rendering_js_code.rb pattern; not a regression (@coderabbitai[bot])
• js_code_builder.rb:42-45 — Line length in JS heredoc: 120-char limit applies to Ruby logic, not embedded JS strings (@coderabbitai[bot])
• js_code_builder.rb:51 — renderingRequest implicit scope comment: same contract as legacy code; belongs in separate cleanup PR (@claude[bot])

All 7 review threads resolved.

To use Codex here, create an environment for this repo.

@justin808 Thanks for the detailed update!

• render_request.rbs store_initializations: String? — Correct fix; the type now matches the nullable return in both the attr_reader and initializer signature.
• configure reset removal — Good call on reverting. Resetting @rendering_strategy/@js_code_builder inside configure would silently clobber the Pro engine's after_initialize assignment on any reconfigure, which is a real footgun for Pro users. Deferring the proper reconfigure mechanism to Step 3 is the right sequencing.

Both of my declined items (typeof guard and heredoc line length) are already captured in long-term memory from the prior round — no new entries needed for those.

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

Timestamp: 2026-04-13T02:29:20.890Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/engine.rb` and `react_on_rails/lib/react_on_rails/configuration.rb` (PR `#3094`): Do NOT reset `rendering_strategy` or `js_code_builder` inside `ReactOnRails.configure`. The Pro engine sets these exactly once during `after_initialize`; resetting them on reconfigure would silently drop Pro's NodeStrategy/ProJsCodeBuilder assignments. A proper reconfigure mechanism for these accessors is deferred to Step 3 of the strategy-pattern refactor (`#2905`).

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 3094
File: react_on_rails_pro/lib/react_on_rails_pro/js_code_builder.rb:40-64
Timestamp: 2026-04-13T02:28:34.069Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/js_code_builder.rb`, the `typeof generateRSCPayload !== 'function'` guards in `rsc_payload_function_section` are an intentional faithful port of the existing pattern in `server_rendering_js_code.rb` (lines 21 and 39). Do not flag this guard as a bug in this file; any fix should be tracked as a separate issue covering both files.

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 2464
File: packages/react-on-rails-pro-node-renderer/src/worker.ts:343-393
Timestamp: 2026-02-21T20:46:38.404Z
Learning: In packages/react-on-rails-pro-node-renderer, the `Promise.all` pattern for per-bundle operations (e.g., in `/upload-assets` endpoint and `handleNewBundlesProvided` in handleRenderRequest.ts) is intentional. Copy operations like `copyUploadedAssets` are idempotent and retries are safe, so fast-fail behavior on lock acquisition or copy errors is acceptable. Switching to `Promise.allSettled` to surface partial success would require Ruby-side protocol changes.

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2674
File: react_on_rails/lib/react_on_rails/doctor.rb:2325-2345
Timestamp: 2026-03-19T16:13:07.375Z
Learning: In `react_on_rails/lib/react_on_rails/doctor.rb`, `check_rsc_payload_route` and `check_rsc_procfile_watcher` filter commented lines (`/^\s*#/`) before checking for the target string, consistent with the `config_has_async_loading_strategy?` pattern. The doctor avoids a full Rails boot for resilience and uses file-content scanning instead of actual Rails route inspection. Remaining accepted limitations are dynamic/engine-mounted routes and routes loaded via the `draw` macro (uncommon, since the generator places `rsc_payload_route` directly in `config/routes.rb`).

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 3094
File: react_on_rails_pro/lib/react_on_rails_pro/js_code_builder.rb:42-45
Timestamp: 2026-04-13T02:28:36.439Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/js_code_builder.rb` (and similar Ruby files in the react_on_rails / react_on_rails_pro codebases), the 120-character Ruby line-length limit applies only to Ruby logic lines, NOT to embedded JavaScript string content inside heredocs (`<<~JS`). Do not flag long JS string lines inside heredocs for exceeding the Ruby style limit, as splitting them would hurt readability for no functional benefit.

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/dummy/commonWebpackConfig.js : Changes to `commonWebpackConfig.js` affect all three webpack bundles (client, server, and RSC) in the Pro dummy app

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: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 2850
File: docs/oss/api-reference/rails-view-rendering-from-inline-javascript.md:0-0
Timestamp: 2026-03-26T07:01:43.653Z
Learning: In `packages/react-on-rails/src/ClientRenderer.ts`, the duplicate-render guard (which skips rendering if the DOM node is already connected/mounted) applies only to the automatic page-load rendering path. The public `ReactOnRails.render(...)` API routes through `packages/react-on-rails/src/base/client.ts` and calls `reactHydrateOrRender(...)` directly, without any deduplication guard. Therefore, repeated calls to the public `ReactOnRails.render(...)` API on the same DOM node (e.g., `#root`) WILL invoke React again — they are NOT deduplicated. Documentation stating this behavior is correct and should not be flagged.

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 2497
File: react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb:0-0
Timestamp: 2026-04-09T06:56:01.856Z
Learning: In `react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb`, config overrides for the error scenarios hub are NOT applied by mutating the ReactOnRails/ReactOnRailsPro global config singletons. Instead, the private helper `error_hub_config_value(key, default)` reads overrides directly from `params` and falls back to the live singleton value — no global state is mutated during request handling. Do not flag the absence of an `around_action` restore pattern here, as the design intentionally avoids mutation altogether.

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2773
File: react_on_rails/lib/react_on_rails/doctor.rb:1262-1265
Timestamp: 2026-03-22T23:36:07.592Z
Learning: In React on Rails, the RSC generator always installs `stream_react_component` in `app/views/`. Therefore, the view-file scan in `uses_prerender_in_views?` (in `react_on_rails/lib/react_on_rails/doctor.rb`) reliably detects RSC-enabled apps via `stream_react_component`, and there is no need to additionally scan `config/routes.rb` for `rsc_payload_route` to cover the route-driven RSC entry point.

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 2530
File: react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb:0-0
Timestamp: 2026-03-04T23:37:22.169Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb`, the `stream_view_containing_react_components` method intentionally does NOT set `Cache-Control: no-transform`. The PR `#2530` is an alternative fix for the Live streaming + compression middleware deadlock (issue `#2519`): instead of using `no-transform` to make compression middleware short-circuit, gzip is handled in-process via a custom `GzipOutputStream` class wrapping `Zlib::GzipWriter`. This keeps compression enabled for clients that support gzip while bypassing the middleware stack entirely. Do not suggest adding `Cache-Control: no-transform` to this method.

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T22:10:33.821Z
Learning: Applies to react_on_rails_pro/** : Ask before modifying the Pro package (`react_on_rails_pro/`)

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/dummy/*WebpackConfig.js : The Pro dummy app has three separate webpack configs: `clientWebpackConfig.js`, `serverWebpackConfig.js`, and `rscWebpackConfig.js`

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/dummy/spec/**/*.rb : Pro integration tests should be run with: `cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/`

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/react_on_rails_pro/**/*.rb : Pro gem unit tests should be run with: `cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/`

Review: RenderRequest, JsCodeBuilder, and RenderingStrategy abstractions

The architecture is sound and the additive approach (zero impact on the live rendering path until Step 3) is the right way to land a refactor of this scope. The unit tests are thorough and the RBS signatures are a nice touch.

Issues

1. Module-level state in the wrong file (see inline on configuration.rb)
rendering_strategy and js_code_builder are boot-time module accessors, not Configuration class members. Placing them in configuration.rb is confusing, and the lazy ||= initializer has a latent thread-safety issue in parallel test runs.

2. Hidden cross-method invariant in ReactOnRailsPro::JsCodeBuilder (see inline)
Three overrides — props_section, render_call_section, and wrap_in_iife — form an undocumented contract: wrap_in_iife introduces componentName into JS scope, props_section emits usedProps instead of props, and render_call_section references both. Overriding any one without the others silently produces broken JS. This is especially risky given Pro is designed to be further subclassed (RSC vs. non-RSC rendering modes).

3. Duplicate validation in RenderRequest#validate_server_bundle_configured! (see inline)
Fine as temp duplication, but needs a TODO so it doesn't survive into Step 3.

4. NotImplementedError messages in RenderingStrategy (see inline)
Minor wording improvement to avoid confusion when a concrete subclass mis-implements the interface.

Observations

• The renderingRequest bare variable reference in the RSC payload section of the Pro builder is intentional — the Node renderer injects it into the execution context. Worth a one-line comment since it looks like a bug on first read (the same implicit dependency exists in the old ServerRenderingJsCode, but it should be documented in the new code).
• The Pro render_call_section uses ReactOnRails['serverRenderReactComponent']({...}) bracket notation in the non-RSC path. This is a carry-over from the existing ServerRenderingJsCode, so it's consistent, but dot notation (ReactOnRails.serverRenderReactComponent) would be cleaner in the non-dynamic branch.
• The RBS for rendering_strategy returning RenderingStrategy (a module type) is correct — Steep handles module-as-type fine here.

Overall this is clean, well-tested code. The inline comments above are the main items I'd want addressed before Step 3 lands.

Review comments addressed in ea1e3ea

Fixed (1):

• js_code_builder.rb:74-95 — Documented the JS variable contract between props_section, render_call_section, and wrap_in_iife in both base and Pro JsCodeBuilder classes (@claude[bot]). Threads resolved.

Declined with rationale (4):

• configuration.rb:29-49 — Accessor placement is intentional; thread-safety is not a practical concern (@claude[bot]). Thread resolved.
• render_request.rb:61-72 — Duplication is intentional during additive phase, tracked by Refactor: Replace runtime react_on_rails_pro? checks with strategy pattern + RenderRequest + JsCodeBuilder #2905; == true matches existing codebase style (@claude[bot]). Thread resolved.
• rendering_strategy.rb:22-42 — NotImplementedError messages follow standard Ruby convention; module origin is clear from stack trace (@claude[bot]). Thread resolved.
• configuration.rb:29 — Reconfigure reset was intentionally removed (6a44410); accessors aren't wired into the main path yet (@cursor[bot]). Thread resolved.

Review: refactor — RenderRequest, JsCodeBuilder, RenderingStrategy abstractions

Overall the design is clean and the additive approach is the right call for this size of refactor. The three-abstraction split (data object / builder / strategy) mirrors the existing layering well, and the JS variable contract documentation in the builder is especially appreciated. A few things worth addressing before Step 3 wires this into the live path:

1. Pro-specific methods on core RenderRequest (important for Step 3)

RenderRequest#rsc_payload_streaming? and #streaming? are public methods on the core class, but rsc_payload_streaming? only makes sense for Pro — and ReactOnRails::ReactComponent::RenderOptions (the base class) doesn't implement it. When Step 3 connects this to the real rendering path, any code that calls render_request.rsc_payload_streaming? with a non-Pro render_options will raise NoMethodError.

The safest fix before Step 3: add def rsc_payload_streaming? = false to the base RenderOptions so it's always safe to call. See inline comment on render_request.rb:55.

2. No tests for ReactOnRailsPro::JsCodeBuilder or NodeStrategy

The PR adds 87 / 168 / 67 spec lines for the three core objects, but the Pro builder — which is the more complex of the two builders (RSC sections, pre-hook, IIFE-with-params, dynamic render function) — ships without a spec file. The Pro builder already has one non-obvious behavioral invariant (the IIFE default-parameter trick that lets the Node renderer re-invoke the serialized function with different component+props) that would benefit from regression coverage before Step 3.

Suggested specs:

• rsc_payload_function_section output when rsc_payload_streaming? is true vs false
• wrap_in_iife includes the default-parameter signature
• props_section uses usedProps / typeof props guard
• render_call_section picks the right RSC ternary for streaming vs non-streaming

3. Parity: base JsCodeBuilder vs existing ServerRenderingJsCode#render

Confirmed that the new builder's output matches the existing heredoc for both core and Pro. One minor point: the existing core ServerRenderingJsCode#render uses a <<-JS heredoc with no chomp, so the generated JS has a trailing newline; the new builder uses <<~JS.chomp throughout, producing tighter output. This is an improvement, just noting it in case any downstream consumer (Pro node renderer?) pattern-matches the raw JS string structure.

Minor / Non-blocking

• ||= race on module-level accessors — harmless in practice since Pro's after_initialize sets these before requests arrive, but see inline note on configuration.rb:33.
• RBS protected gap — build_sections and the section helpers are protected in Ruby but listed as public in the RBS; Steep won't prevent external callers. See inline on js_code_builder.rbs:4.
• renderingRequest JS scope — matches existing ServerRenderingJsCode behavior (Node renderer injects it); added a clarifying comment suggestion inline at pro/js_code_builder.rb:51.

Review comments addressed — final round

All 4 remaining review threads from the latest review have been replied to and resolved. No code changes were needed.

Declined with rationale (4):

• render_request.rb:55 — rsc_payload_streaming? NoMethodError claim is incorrect: base RenderOptions already defines the method at render_options.rb:166–169. The Pro call site is also guarded by rsc_streaming?.
• pro/js_code_builder.rb:51 — renderingRequest JS scope comment: already documented in build_sections contract comments (ea1e3ea).
• js_code_builder.rbs:4 — RBS protected gap: known RBS limitation, Ruby-level enforcement is intact.
• configuration.rb:33 — ||= thread safety: harmless in practice (stateless objects, Pro sets before first request).

All review threads on this PR are now resolved.