docs: experiment plan for decoupling props from JS source (#3281)#3297
Conversation
Document the experiment findings and implementation plan for sending component props as a separate JSON field instead of embedding them in the IIFE source string. Microbenchmarks show JSON.parse is ~1.8x faster than V8's JS parser for large payloads, and end-to-end benchmarks show ~18ms (14%) improvement on 1.1MB props. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a planning document specifying how to decouple large React component props from the JavaScript IIFE source string in React on Rails. The plan includes performance benchmarks, implementation steps across Ruby and Node code paths, and a verification checklist. ChangesProps-Decoupling Design Plan (Issue
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/docs/analysis/3281-decouple-props-from-js-source.md:
- Around line 48-57: The fenced code block containing the call chain
("rails_helper.rb ... vm.ts: runInVM") is missing a language tag and triggers
markdownlint MD040; update that fenced block in
.claude/docs/analysis/3281-decouple-props-from-js-source.md by adding an
appropriate language identifier (e.g., text) after the opening ``` so the block
becomes ```text and linting will pass, leaving the existing lines
(rails_helper.rb → ... → vm.ts: runInVM) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed091043-84c4-4d3d-9520-d906e8a2735a
📒 Files selected for processing (1)
.claude/docs/analysis/3281-decouple-props-from-js-source.md
| ``` | ||
| rails_helper.rb (react_component / stream_react_component) | ||
| → ReactOnRails::ServerRenderingJsCode.server_rendering_component_js_code | ||
| → ReactOnRailsPro::ServerRenderingJsCode.render (builds IIFE with embedded props) | ||
| → ReactOnRails::ServerRenderingPool#exec_server_render_js | ||
| → ReactOnRailsPro::Request#form_with_code (HTTP POST to Node renderer) | ||
| → Node: worker.ts route handler | ||
| → handleRenderRequest.ts | ||
| → vm.ts: runInVM (vm.runInContext with the IIFE) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block at Line 48.
This block is missing a language identifier, which will keep tripping markdownlint (MD040).
Suggested fix
-```
+```text
rails_helper.rb (react_component / stream_react_component)
→ ReactOnRails::ServerRenderingJsCode.server_rendering_component_js_code
→ ReactOnRailsPro::ServerRenderingJsCode.render (builds IIFE with embedded props)
→ ReactOnRails::ServerRenderingPool#exec_server_render_js
→ ReactOnRailsPro::Request#form_with_code (HTTP POST to Node renderer)
→ Node: worker.ts route handler
→ handleRenderRequest.ts
→ vm.ts: runInVM (vm.runInContext with the IIFE)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/docs/analysis/3281-decouple-props-from-js-source.md around lines 48
- 57, The fenced code block containing the call chain ("rails_helper.rb ...
vm.ts: runInVM") is missing a language tag and triggers markdownlint MD040;
update that fenced block in
.claude/docs/analysis/3281-decouple-props-from-js-source.md by adding an
appropriate language identifier (e.g., text) after the openingso the block becomestext and linting will pass, leaving the existing lines
(rails_helper.rb → ... → vm.ts: runInVM) unchanged.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01006d5365
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) | ||
| form["props"] = props | ||
| Thread.current[:ror_decoupled_props] = nil |
There was a problem hiding this comment.
When the renderer replies with STATUS_SEND_BUNDLE because the VM does not have the bundle yet, both NodeRenderingPool#eval_js and StreamRequest#each_chunk call form_with_code a second time with send_bundle: true. Clearing the thread-local here means that retry sends the new globalThis.__rorpProps-based rendering request without the separate props field, so first render after a renderer restart/deploy fails or renders with undefined props. Keep the props available until the whole render attempt finishes, and only clear it from an outer ensure.
Useful? React with 👍 / 👎.
| #{generate_rsc_payload_js_function(render_options)} | ||
| #{ssr_pre_hook_js} | ||
| #{redux_stores} | ||
| var usedProps = typeof props === 'undefined' ? globalThis.__rorpProps : props; |
There was a problem hiding this comment.
When renderer_use_fallback_exec_js is enabled, NodeRenderingPool#eval_js rescues renderer failures and executes the same js_code through RubyEmbeddedJavaScript, but that path never receives the new form props field or injects context.__rorpProps. Replacing the inline props with globalThis.__rorpProps therefore makes any ExecJS fallback render pass undefined props; the plan needs either an ExecJS injection path or a fallback-safe inline source.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds a documentation-only analysis plan for decoupling component props from the IIFE source string sent to the Node renderer, targeting a ~14% (18 ms) latency reduction for large-props renders by replacing V8's JS parser with the faster
Confidence Score: 3/5Safe to merge as documentation, but the plan has two logic defects in the proposed code snippets that would produce silent wrong-rendering bugs if implemented as written. The plan is well-researched and the benchmark methodology is sound, but two proposed code snippets have correctness problems: the falsy thread-local check silently drops props (producing undefined usedProps with no error), and the incremental-render path is left unspecified despite being called out as needing the same treatment. These affect two distinct render paths and would be non-obvious to catch during implementation. .claude/docs/analysis/3281-decouple-props-from-js-source.md — specifically the form_with_code Ruby snippet, the vm.ts cleanup block, and the incremental-render section. Important Files Changed
Sequence DiagramsequenceDiagram
participant Rails as Rails (server_rendering_js_code.rb)
participant Thread as Thread.current
participant Request as request.rb
participant Node as Node worker.ts
participant VM as vm.ts
Rails->>Thread: "[:ror_decoupled_props] = props_string"
Rails->>Request: form_with_code(iife_without_props)
Request->>Thread: read [:ror_decoupled_props]
Thread-->>Request: props_string
Request->>Thread: "[:ror_decoupled_props] = nil"
Request->>Node: "POST { renderingRequest: iife, props: propsJson }"
Node->>VM: runInVM(iife, bundlePath, cluster, propsJson)
VM->>VM: "context.__rorpProps = JSON.parse(propsJson)"
VM->>VM: vm.runInContext(iife) reads globalThis.__rorpProps
VM->>VM: [finally] delete context.__rorpProps
VM-->>Node: RenderCodeResult
Node-->>Rails: rendered HTML
Reviews (1): Last reviewed commit: "docs: add experiment plan for decoupling..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const runInVM = async ( | ||
| renderingRequest: string, | ||
| bundleFilePath: string, | ||
| vmCluster?: typeof cluster, |
There was a problem hiding this comment.
The plan acknowledges that render_code_with_incremental_updates / build_initial_incremental_request "needs the same thread-local treatment" but, unlike all six files in the implementation table, provides no code snippet for it. An implementer following this plan would have a complete recipe for five paths and then hit a gap on the sixth, increasing the risk of this path being skipped or handled inconsistently. The plan should include the equivalent of the form_with_code treatment for build_initial_incremental_request, or explicitly call out that the incremental path still embeds props inline and remains unoptimized.
| #{async_props_setup_js(render_options)} | ||
| return ReactOnRails[#{render_function_name}]({ | ||
| name: componentName, | ||
| domNodeId: #{render_options.dom_id.to_json}, | ||
| props: usedProps, | ||
| trace: #{render_options.trace}, | ||
| railsContext: railsContext, | ||
| throwJsErrors: #{ReactOnRailsPro.configuration.throw_js_errors}, | ||
| renderingReturnsPromises: #{ReactOnRailsPro.configuration.rendering_returns_promises}, | ||
| generateRSCPayload: typeof generateRSCPayload !== 'undefined' ? generateRSCPayload : undefined, | ||
| }); |
There was a problem hiding this comment.
The form_with_code snippet uses if (props = Thread.current[:ror_decoupled_props]). If props_string is nil (or the thread-local was already cleared by the ensure branch before this executes), the condition is false, form["props"] is never set, and propsJson arrives at vm.ts as undefined. Because the IIFE now reads globalThis.__rorpProps instead of embedding the value, usedProps becomes undefined and the render silently produces wrong output rather than an error. Consider using an explicit nil-check (unless Thread.current[:ror_decoupled_props].nil?) and adding a defensive fallback or explicit error if __rorpProps is absent when the plan assumes it will be present.
| ``` | ||
|
|
||
| ### 4. Node: `packages/react-on-rails-pro-node-renderer/src/worker.ts` | ||
|
|
||
| In the `/bundles/:bundleTimestamp/render/:renderRequestDigest` route handler, extract `body.props`: | ||
|
|
||
| ```typescript | ||
| const { renderingRequest, props: propsJson } = body as Record<string, unknown>; | ||
| // ... pass to handleRenderRequest: | ||
| propsJson: typeof propsJson === 'string' ? propsJson : undefined, | ||
| ``` |
There was a problem hiding this comment.
= undefined leaves the large parsed object reachable until GC; prefer delete
The finally block sets context.__rorpProps = undefined. This re-assigns the property slot but leaves the property itself present on the object, meaning the reference to the large parsed props object persists on the VM context until the next GC cycle. For a 1.1 MB payload in a high-throughput renderer, using delete context.__rorpProps removes the property entirely and allows the GC to reclaim the object sooner.
| #{rsc_params} | ||
| #{generate_rsc_payload_js_function(render_options)} | ||
| #{ssr_pre_hook_js} |
There was a problem hiding this comment.
Thread.current[:ror_decoupled_props] stores the value per OS thread. Fiber-based servers like Falcon multiplex many requests over a small number of threads using fibers; a fiber running a request never inherits thread-local values set by a different fiber on the same thread, and two concurrent requests on the same thread could see each other's values. The plan should note this limitation and, if Falcon compatibility is a goal, consider Fiber[:ror_decoupled_props] (Ruby 3.0+) instead.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review: Decouple Props from JS Source (#3297)This is a documentation-only PR adding an implementation plan for a well-motivated performance optimization. The V8 compile cache analysis is technically sound, the benchmark methodology is correctly validated (the initial flawed benchmark with cached source strings is a common trap, and it's good that it was caught and corrected), and the 14% end-to-end improvement is compelling. A few concerns to address before or during implementation: 1. Thread-local variable: implicit coupling and lifecycle risksThe plan routes
2. No error handling for
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ```ruby | ||
| def render(props_string, rails_context, redux_stores, react_component_name, render_options) | ||
| Thread.current[:ror_decoupled_props] = props_string |
There was a problem hiding this comment.
The thread-local stash works in Puma's threaded model, but the lifecycle needs more care. If form_with_code is called more than once during a single render (e.g., an internal retry), the second call will see nil here (cleared on line 113) and send no props field — causing the Node renderer to execute the IIFE with globalThis.__rorpProps set to undefined.
The plan should guarantee that Thread.current[:ror_decoupled_props] is set immediately before the call chain that leads to form_with_code, and that the call chain cannot repeat without a fresh set. If retries are possible, either re-set the thread-local before each attempt or pass props explicitly.
| // ... existing context setup ... | ||
| try { | ||
| if (propsJson) { | ||
| context.__rorpProps = JSON.parse(propsJson); |
There was a problem hiding this comment.
JSON.parse(propsJson) throws a SyntaxError on malformed input (truncated HTTP body, serialization bug). This propagates uncaught out of runInVM. The plan should specify the error-handling contract: catch and re-throw with a diagnostic message (e.g., "Failed to parse props JSON for component: ...") so server-side rendering failures are debuggable rather than surfacing as an opaque Node crash.
| ## Streaming and Incremental Paths | ||
|
|
||
| - **Streaming render** (`render_code_as_stream`): Also calls `form_with_code`, so props will be decoupled automatically | ||
| - **Incremental render** (`render_code_with_incremental_updates`): Uses `build_initial_incremental_request` which calls `form_with_code` with JS code that also embeds props — needs the same thread-local treatment |
There was a problem hiding this comment.
This says the incremental path "needs the same thread-local treatment" but provides no implementation. Since build_initial_incremental_request is a separate code path from form_with_code, it either needs its own thread-local stash (and server_rendering_js_code.rb's render must set it before that path too), or the plan should explain why the same Thread.current[:ror_decoupled_props] set at the top will still be alive when build_initial_incremental_request calls form_with_code. Please flesh this out or add an explicit follow-up task.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| ## Microbenchmark Results | ||
|
|
||
| A standalone microbenchmark (`/tmp/bench-json-vs-js-v2.mjs`) compared parsing 1.1 MB of JSON data as a JS object literal vs. `JSON.parse()`: |
There was a problem hiding this comment.
The benchmark script lives at /tmp/bench-json-vs-js-v2.mjs which is ephemeral. Consider committing it under internal/benchmarks/ or linking to a gist so the results can be reproduced and re-run when Node/V8 versions change.
| def form_with_code(js_code, send_bundle) | ||
| form = common_form_data | ||
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) |
There was a problem hiding this comment.
The thread-local is consumed (read + cleared) inside form_with_code, but the ensure cleanup in step 3 also sets it to nil. That's fine for the happy path. However, if form_with_code is called multiple times within a single render cycle (e.g., retry logic or the incremental path that builds an initial request and then later sends updates), the second call will find the thread-local already nil and silently skip the props field — causing a runtime error in the Node VM where globalThis.__rorpProps is undefined.
Consider making the consume-once behaviour explicit (raise if it was already nil when a decoupled render was expected) or document the invariant that form_with_code is called exactly once per render().
| ) => { | ||
| // ... existing context setup ... | ||
| try { | ||
| if (propsJson) { |
There was a problem hiding this comment.
JSON.parse(propsJson) will throw a SyntaxError if the payload is malformed, and that exception will bubble out of runInVM uncaught. The existing try/finally block only handles context cleanup; it doesn't catch parse errors. This should be wrapped in its own try/catch with a descriptive error message (e.g. "Failed to parse decoupled props: ...") so rendering errors surface clearly instead of appearing as generic VM failures.
if (propsJson) {
try {
context.__rorpProps = JSON.parse(propsJson);
} catch (e) {
throw new Error(`Failed to parse decoupled props payload: ${(e as Error).message}`);
}
}
|
|
||
| ## RSC Compatibility | ||
|
|
||
| The `generateRSCPayload` function modifies the IIFE's trailing `()` to pass `(componentName, propsString)` and runs on another bundle's VM context. With this change: |
There was a problem hiding this comment.
The type of the second argument passed by generateRSCPayload needs clarification. The plan says it calls the IIFE with (componentName, propsString) — but is propsString here a JSON string or a parsed JS object?
- If it's a JSON string, then
usedProps = propsgives the IIFE a string, not an object — likely a regression. - If it's a parsed object, the name
propsStringis misleading and should bepropsObjectin the docs.
Please spell out which type RSC passes so implementors don't have to reverse-engineer it from the existing source.
| ## Streaming and Incremental Paths | ||
|
|
||
| - **Streaming render** (`render_code_as_stream`): Also calls `form_with_code`, so props will be decoupled automatically | ||
| - **Incremental render** (`render_code_with_incremental_updates`): Uses `build_initial_incremental_request` which calls `form_with_code` with JS code that also embeds props — needs the same thread-local treatment |
There was a problem hiding this comment.
The incremental render path is flagged as needing "the same thread-local treatment" but the implementation is left unspecified. Since this is a planning doc meant to guide the implementation PR, the gap should be filled here: which method sets Thread.current[:ror_decoupled_props] for the incremental path, and does build_initial_incremental_request also call form_with_code directly, or does it go through a different code path?
Without this detail, the implementor may overlook the incremental path and ship a partial fix.
Code ReviewThis is a well-researched planning document with solid benchmark methodology. The V8 compile-cache insight and the corrected benchmark (using unique UUIDs per iteration) are particularly good. A few issues worth resolving before the implementation PR lands: File path discrepancyThe test plan checkbox in the PR description references Deployment / backward-compatibility strategy missingThis change is a protocol change between two independently deployed components (Ruby gem + Node renderer package). The plan has no rollout strategy:
The plan should address whether this needs a coordinated release, a negotiation header, or a compatibility check in the Node handler. See inline comment on the
|
Code Review: Plan - Decouple Props from JS Source StringThis is a well-researched planning document for a meaningful performance optimization. The benchmark methodology note (correcting for V8's compile cache skewing the initial benchmark) is particularly good, and the architecture walk-through is accurate and helpful. The RSC compatibility analysis is thoughtful. Below are issues to address before the implementation PR. Critical / Must Fix Before Implementation1. Backward-compatibility gap during rolling deploys The plan has no strategy for the window between deploying a new Rails version and deploying a new Node renderer. During that window, Rails sends 2. Nil props_string breaks silently
3. Incremental render path is mentioned but not designed The Streaming and Incremental Paths section says Should Fix4. Missing error handling around JSON.parse in vm.ts
5. Awkward positional-parameter ordering in runInVM Adding 6. Microbenchmark script not in the repository The benchmark is referenced at Nice to Have7. Benchmark statistical rigor The end-to-end table reports only means. For a 14% improvement claim, include standard deviation (or p95) so reviewers can judge statistical confidence. 8. RSC props format — string vs. parsed object The RSC section says 9. PR description links to the wrong file path The test plan in the PR description points to |
| def form_with_code(js_code, send_bundle) | ||
| form = common_form_data | ||
| form["renderingRequest"] = js_code | ||
| if (props = Thread.current[:ror_decoupled_props]) |
There was a problem hiding this comment.
The if (props = ...) guard is falsy when props_string is nil. A component rendered with no props (nil props_string) would silently skip sending the props field, causing the Node renderer to evaluate globalThis.__rorpProps as undefined and fail. Consider always sending the field:
| if (props = Thread.current[:ror_decoupled_props]) | |
| form["props"] = Thread.current[:ror_decoupled_props] || "{}" | |
| Thread.current[:ror_decoupled_props] = nil |
Or store props_string.to_s (converts nil to "") and always send, then skip JSON.parse on empty string on the Node side.
| ) => { | ||
| // ... existing context setup ... | ||
| try { | ||
| if (propsJson) { |
There was a problem hiding this comment.
if (propsJson) is falsy for an empty string. An empty string is not valid component props JSON, but more importantly this pattern silently does nothing for edge cases. Prefer an explicit null-check and add try-catch for parse failures:
| if (propsJson) { | |
| if (propsJson != null) { | |
| try { | |
| context.__rorpProps = JSON.parse(propsJson); | |
| } catch (e) { | |
| throw new Error(`Failed to parse propsJson for render: ${e instanceof Error ? e.message : String(e)}`); | |
| } | |
| } |
| In `runInVM`, accept optional `propsJson`, inject parsed props into the VM context before executing the IIFE: | ||
|
|
||
| ```typescript | ||
| const runInVM = async ( |
There was a problem hiding this comment.
Adding propsJson as a 4th positional param after an already-optional vmCluster is awkward — callers that want to pass propsJson without vmCluster must write runInVM(req, path, undefined, propsJson). Consider restructuring to an options object:
interface RunInVMOptions {
vmCluster?: typeof cluster;
propsJson?: string;
}
const runInVM = async (
renderingRequest: string,
bundleFilePath: string,
options: RunInVMOptions = {},
) => { ... }
|
|
||
| ## Streaming and Incremental Paths | ||
|
|
||
| - **Streaming render** (`render_code_as_stream`): Also calls `form_with_code`, so props will be decoupled automatically |
There was a problem hiding this comment.
The incremental render path is flagged as needing "the same thread-local treatment" but no code sketch is provided. Unlike the streaming path (which already flows through form_with_code), build_initial_incremental_request may have a different code path for how it embeds props. This is a gap in the plan — either add a Section 3.5 with the concrete changes or open a follow-up issue and link it here so it isn't forgotten.
|
|
||
| ## Microbenchmark Results | ||
|
|
||
| A standalone microbenchmark (`/tmp/bench-json-vs-js-v2.mjs`) compared parsing 1.1 MB of JSON data as a JS object literal vs. `JSON.parse()`: |
There was a problem hiding this comment.
The microbenchmark script is at /tmp/bench-json-vs-js-v2.mjs — a throwaway temp path. Since this plan document will live in the repo, the benchmark should too. Consider committing it to internal/planning/benchmarks/bench-json-vs-js.mjs so any contributor can re-run it and verify the numbers.
|
|
||
| ## End-to-End Benchmark Results | ||
|
|
||
| Benchmarked the full React on Rails Pro rendering pipeline (30 runs, 15 warmup, Rails + Node renderer, mega benchmark page with ~1.1 MB props): |
There was a problem hiding this comment.
The table reports only means (132.4 ms vs 113.9 ms). With 30 runs that's a reasonable sample, but without a standard deviation it's hard to tell if the 18.5 ms delta is outside the noise floor. Please add stddev (or p95) for both baseline and experiment — e.g. "132.4 ± 8.2 ms" — so the 14% claim carries statistical weight.
1 participant
Summary
Context
V8 cannot cache the compiled IIFE across requests because
domNodeIdis a random UUID per render, making every source string unique. This means V8 re-parses the full source (including large props) from scratch on every request. By decoupling props and usingJSON.parse()on the Node side, we leverage V8's optimized JSON parser.Key findings
Test plan
.claude/docs/analysis/3281-decouple-props-from-js-source.md🤖 Generated with Claude Code
Summary by CodeRabbit