docs(planning): JSON render-request body migration plan (#3280)

[AbanoubGhadban]

Planning artifact for #3280 — docs only, no code change. Lives in internal/planning/ per repo convention (alongside MONOREPO_MERGER_PLAN.md, js-pro-package-separation-plan.md, etc.).

Companion to experiment PR #3293. This is the production-quality follow-up plan the issue asked for once we knew the numbers moved: it records the change, the full benchmark results, the operational problems hit while running the experiment, the deep compatibility/protocol findings, and a phased production implementation plan.

Supersedes the earlier (closed) #3298, which targeted upcoming-v16.3.0 and put the file under react_on_rails_pro/. This one targets main and uses internal/planning/.

What's in internal/planning/json-render-body-migration-plan.md

• The change — the 9-line render_code conditional and why no renderer change is needed (Fastify default JSON parser; extractBodyArrayField already handles both key and key[]; auth reads req.body.password either way). Includes the runtime instrumentation proof (Ruby↔Node byte-for-byte match on 3 payload sizes).
• Performance numbers — sequential sweep, isolation, autocannon, web-vitals, three-methodology robustness (sequential / paired-curl / parallel-A/B), the perf(pro): enable TCP_NODELAY on httpx (remove 40ms HTTP/2 tail) #3217 (TCP_NODELAY) orthogonality re-verification, and byte-equivalence. Headline: mega page 396 → 92 ms wall (−304), 112.8 → 9.8 ms GC (−103), ~2× the acceptance bar.
• 13 operational problems faced — each with the workaround used.
• Production plan — compat matrix + protocol-version gating, multipart preservation, streaming out-of-scope, tests, docs/changelog, defensive audit, sequencing vs perf(pro): enable TCP_NODELAY on httpx (remove 40ms HTTP/2 tail) #3217.

Links

Issue	#3280
Experiment PR (demo)	#3293
Experiment commit	352743ef1
Demo-app harness commit	b90977ef3
Demo-app branch	https://github.com/AbanoubGhadban/rsc-benchmar/tree/experiment/3280-json-render-body
Orthogonal PR	#3217

🤖 Generated with Claude Code

Summary by CodeRabbit

• Documentation
  • Added planning document for a render request body format experiment: includes experiment summary, benchmarking and correctness results, operational findings, and a staged production implementation, testing, and rollout plan.

[coderabbitai]

Walkthrough

This PR adds a production planning document for switching steady-state render request bodies from form-encoded to JSON (when no bundle is attached), recording the demo, verification, benchmark results, operational issues, and a production rollout plan with protocol-version gating and required test/docs updates.

Changes

JSON Render-Body Migration Planning

Layer / File(s)	Summary
Experiment Overview and Demo Verification internal/planning/json-render-body-migration-plan.md	Introduces the proposed wire-level change from form-encoded to JSON for steady-state render requests. Documents the experimental demo approach (Ruby conditional: multipart when send_bundle true, otherwise use HTTPX json:), Fastify JSON parsing compatibility, and runtime verification via content-type and byte-matching checks.
Performance Validation and Operational Learning internal/planning/json-render-body-migration-plan.md	Reports benchmarking results (sequential sweeps, isolation, concurrent load, web-vitals), cross-method agreement, robustness checks vs the orthogonal TCP_NODELAY change, and correctness via byte-equivalent HTML. Catalogs operational/environmental issues encountered during experiments.
Production Implementation Roadmap internal/planning/json-render-body-migration-plan.md	Specifies protocol compatibility/version gating, keep multipart for send_bundle: true, defer streaming conversions, enumerates required Ruby and Node test updates (including JSON-body and byte-equivalence assertions), documentation/changelog tasks, and sequencing guidance relative to related PRs.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related issues

• #3280 — Documents and formalizes the same render-request body change and demo/experiment described in this planning document.

Poem

A rabbit scribbles a JSON plan with glee,
Forms folded, now data flows free,
Benchmarks hum, byte-matches shine,
Steady requests step in a straight line,
Hops of joy for a migration small and spry 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically identifies the main change: adding a JSON render-request body migration plan documentation. It directly maps to the only file added and accurately summarizes the content.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs/3280-json-render-body-plan

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR adds a docs-only planning artifact at internal/planning/json-render-body-migration-plan.md that records the completed AI-agent experiment for #3280 and lays out the work required for a production-quality implementation.

• The experiment result: a 9-line conditional in request.rb routing send_bundle: false paths through HTTPX's json: option; no Node-side change needed because Fastify's built-in JSON parser and extractBodyArrayField's dual-key check already handle both encodings. Headline: median wall 396 \u2192 92 ms, GC 112.8 \u2192 9.8 ms on the mega-payload page.
• The production plan: covers backward compatibility via a protocolVersion gate (\u00a74.1), multipart preservation for send_bundle: true (\u00a74.2), streaming paths out of scope (\u00a74.3), test requirements (\u00a74.4), changelog/docs (\u00a74.5), a defensive audit (\u00a74.6), and sequencing against perf(pro): enable TCP_NODELAY on httpx (remove 40ms HTTP/2 tail) #3217 (\u00a74.7).
• 13 operational problems from the experiment run are catalogued so the production implementation can budget for them.

Confidence Score: 4/5

Docs-only change; no executable code is modified. Safe to merge as a planning record.

The document is thorough and well-structured, with three independent methodologies supporting the core performance claim. All three comments are about completeness gaps in the planning text: an unexplained web vitals anomaly in the baseline data, an optional re-measurement that should be a hard gate before shipping, and a missing version number for the protocol bump. None affect the correctness of the proposal itself.

The web vitals table in §2.4 and the hello_server footnote merit a second look before the plan is used as the basis for a production PR.

Important Files Changed

Filename	Overview
internal/planning/json-render-body-migration-plan.md	New planning document recording the JSON render-body experiment results, 13 operational problems, and a phased production implementation plan. Three minor gaps: baseline web vitals appear anomalous for mega_benchmark and are unexplained, the hello_server TTFB re-measurement is optional rather than required, and the protocol version bump is not scoped to a specific version.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[render_code called] --> B{send_bundle?}
    B -- true --> C["perform_request form multipart"]
    B -- false --> D["perform_request json application/json"]
    D --> E{protocolVersion gate met?}
    E -- yes --> F[Fastify default JSON body parser]
    E -- no --> G[Fall back to form-encoded body]
    F --> H[req.body read agnostically]
    G --> H
    C --> I[multipart handler]
    I --> H
    H --> J[extractBodyArrayField checks key and key-array]
    J --> K[Byte-equivalent HTML response]

Loading

_{Reviews (1): Last reviewed commit: "docs(planning): JSON render-request body..." | Re-trigger Greptile}

[greptile-apps]

Baseline web vitals appear anomalous for mega_benchmark — plan should acknowledge this

The mega_benchmark_traditional row shows baseline total page load of 240 ms with a 233 ms TTFB, implying the browser received and fully rendered 2.18 MB of HTML in only 7 ms — not physically plausible. The after-measurement (553 ms total, 135 ms TTFB → 418 ms remaining load) is far more credible for a 2.18 MB document. This looks like a baseline measurement artifact rather than an after-state regression, but the plan never says so. Similarly, heavy_benchmark_traditional load doubles (37 → 79 ms) without any note. The "Go" verdict and §2.5's "three methodologies agree" claim could be misread by implementers who see the web vitals table without this explanation — the footnote only addresses hello_server. A short note clarifying that the load-time increases reflect a plausible baseline anomaly would prevent this misreading.

[greptile-apps]

hello_server re-measurement should be required, not conditional

The footnote phrases the ≥10-iteration re-measurement as "if it matters," but hello_server shows an ~20× TTFB increase (6 → 118.9 ms) on an allegedly untouched streaming path. Even if the root cause is cold-cache noise, a production plan that ships with an unresolved 20× regression on any endpoint — even a likely false positive — leaves reviewers without a clear "we checked and it's fine" sign-off. Consider promoting this to an explicit pre-ship gate in §4 rather than leaving it optional.

[greptile-apps]

Protocol version bump semantics are underspecified

§4.1 says to bump protocolVersion and gate JSON on renderers at/above the bump, but doesn't specify whether the bump is minor (2.0.0 → 2.1.0) or major (2.0.0 → 3.0.0). Sending a different Content-Type is a wire-breaking change for any renderer that doesn't have Fastify's default JSON parser (e.g., the pre-Fastify Express path listed as ✗ _risk_ in the compatibility matrix). Calling out the intended version string would prevent the production PR from having to relitigate this decision.

[claude]

Code Review (PR 3299 - docs only, no code change)

Verified against current source: extractBodyArrayField dual-key check, Fastify JSON parser never removed, bodyLimit 100MB, protocol version 2.0.0 matches both sides, password auth via timingSafeEqual, /asset-exists already uses json: keyword, common_form_data returns only JSON-serializable types. All claims accurate.

Four gaps to address before the implementation PR:

1. PROTOCOL VERSION GATING MECHANISM (section 4.1) -- The plan says "gate on protocolVersion, send form below the bump, JSON at/above." But checkProtocolVersionHandler.ts uses a strict equality check -- the gem cannot query the renderer version before choosing encoding. A renderer on 2.0.0 already rejects a gem on 2.1.0 with 412 (version mismatch), so "send form to old renderers" is unreachable regardless of encoding. Clarify: Option A (simpler) -- always send JSON after the version bump, document minimum supported renderer version; or Option B -- add a GET /status capability endpoint for the gem to discover the renderer version at startup. Option A matches current protocol behavior.

2. JSON.generate FAILURE MODE (sections 4.2/4.6) -- The plan warns against "silently" routing a Pathname through JSON.generate, but JSON.generate on a Pathname raises TypeError immediately -- not silent. The real risk is get_form_body_for_file returning a String (file contents), making it JSON-serializable but wrong. The regression test should assert Content-Type is multipart/form-data when send_bundle is true, not just that the call succeeds.

3. hello_server TTFB SPIKE (section 2.4 footnote 1) -- A 20x TTFB inflation (6ms to 119ms) on the streaming SSR path dismissed as "likely noise, re-measure if it matters" is too passive for a production plan. Require a clean web-vitals run with 10+ iterations on streaming paths before merging the implementation PR.

4. WAF/PROXY OPERATOR NOTE (section 4.5) -- The content-type change is visible to any proxy or WAF between Rails and the renderer. A WAF allowing only multipart/form-data or application/x-www-form-urlencoded would start blocking requests. Migration note should call this out for operators who restrict that internal traffic.

Minor: Section 1 references utils.rb:167 for common_form_data but it is at line 165. Section 4.4 Ruby spec should verify perform_request is called with the json: keyword argument, not just inspect the serialized body bytes.

Overall: benchmarking solid, compatibility analysis accurate, risk items identified. Resolve the protocol gating mechanism question (item 1) before writing the implementation PR.

[claude]

Protocol version gating mechanism needs clarification.

The plan says "gate on protocolVersion — bump it and have the gem send form to renderers below the bump, JSON at/above." But checkProtocolVersionHandler.ts uses a strict equality check — the gem cannot discover the renderer version before choosing its encoding format. A renderer still on 2.0.0 already rejects any gem sending protocolVersion: 2.1.0 with HTTP 412, regardless of content-type. So "send form to old renderers" is unreachable in practice.

Two concrete options to resolve this:

• Option A (simpler): Always send JSON once the gem bumps the protocol version. Old renderers are already broken by the 412 version-mismatch rejection. Document the minimum supported renderer version. Consistent with current protocol behavior.
• Option B (graceful degradation): Add a GET /status (or equivalent) capability endpoint the gem queries once at startup, then stores and uses to choose encoding. More complex but allows rolling upgrades.

Option A is the path of least resistance given the existing strict-equality version gate.

[claude]

Failure mode is loud (TypeError), not silent — reframe the test goal.

JSON.generate on a hash containing a Pathname raises TypeError: no implicit conversion of Pathname into String immediately. There is no silent failure path via the Pathname value itself.

The actual silent failure risk is if get_form_body_for_file is ever changed to return the file contents as a String — that value is JSON-serializable, so json: form would succeed, but the renderer would receive the full bundle JS embedded as a JSON string field rather than as a multipart file upload, and the rendering would silently misbehave.

The regression test should therefore assert that when send_bundle: true, the outgoing request's Content-Type header is multipart/form-data, not just that no exception is raised.

[claude]

hello_server TTFB anomaly needs a mandatory re-measurement, not an optional one.

The footnote says "re-measure with iters≥10 if it matters." A 20x TTFB inflation on the streaming SSR path (6 ms → 119 ms) should be treated as a blocker requiring proactive clearance rather than something to revisit only "if it matters." The autocannon result (120→115 ms, flat) is encouraging but covers aggregate throughput, not per-page first-byte latency under realistic conditions.

Suggest strengthening the production plan to require: a clean Playwright web-vitals run with ≥10 iterations on hello_server and mega_benchmark (streaming paths) before the implementation PR is marked ready for merge, with the results included in the PR description.

[claude]

Add a WAF/proxy operator note here.

The content-type change from application/x-www-form-urlencoded to application/json on the render endpoint is observable at the network level. Any operator who has:

• a WAF allowlisting specific content-types on the internal Rails↔renderer path
• a reverse proxy (nginx, HAProxy, etc.) with content-type filtering
• network-level logging/IDS inspecting that traffic

…will see requests start failing or triggering alerts after this change. The CHANGELOG entry should include a migration callout along the lines of: "If you have a WAF or proxy restricting Content-Type on the renderer endpoint, add application/json to the allowlist."

[claude]

Minor: utils.rb:167 — in the current checkout common_form_data is defined at line 165, not 167. Worth correcting before the implementation PR references it as a starting point.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30231ea303

ℹ️ 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".

[chatgpt-codex-connector]

Avoid using protocolVersion as the JSON feature gate

When the production PR follows this recommendation for mixed deployments, a new gem talking to an old renderer will not fall back cleanly: the gem includes its ReactOnRailsPro::PROTOCOL_VERSION in every request body, and the renderer rejects any mismatch with HTTP 412 before handling the render. If the protocol is bumped to select JSON vs form, the first fallback form request to an old renderer still carries the new protocol value and is rejected, so the plan needs a real capability discovery/config mechanism or an explicit old-protocol fallback.

Useful? React with 👍 / 👎.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/planning/json-render-body-migration-plan.md (1)

1-378: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix Prettier formatting violations before merge.

The CI pipeline reports that this markdown file has formatting issues. As per coding guidelines, Prettier must be used for all markdown files.

Run the following to fix:

prettier --write internal/planning/json-render-body-migration-plan.md

Then commit the changes. As per coding guidelines, Prettier is required for markdown files matching **/*.md.

🤖 Prompt for 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.

In `@internal/planning/json-render-body-migration-plan.md` around lines 1 - 378,
Summary: Prettier formatting violations in the "Production Migration Plan — JSON
render-request body (`#3280`)" markdown need fixing. Fix: run Prettier
(project-configured prettier) to format that markdown document (the migration
plan whose heading/title is "Production Migration Plan — JSON render-request
body (`#3280`)"), review the formatted diff to ensure only whitespace/format
changes, then commit the resulting changes; ensure CI passes. Location hints:
look for the markdown file containing the document title and the experiment diff
section in the repo.

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

Outside diff comments:
In `@internal/planning/json-render-body-migration-plan.md`:
- Around line 1-378: Summary: Prettier formatting violations in the "Production
Migration Plan — JSON render-request body (`#3280`)" markdown need fixing. Fix:
run Prettier (project-configured prettier) to format that markdown document (the
migration plan whose heading/title is "Production Migration Plan — JSON
render-request body (`#3280`)"), review the formatted diff to ensure only
whitespace/format changes, then commit the resulting changes; ensure CI passes.
Location hints: look for the markdown file containing the document title and the
experiment diff section in the repo.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fe079ca8-6318-4d98-9f40-c44d3a02918d

📥 Commits

Reviewing files that changed from the base of the PR and between e88bc4c and 30231ea.

📒 Files selected for processing (1)

• internal/planning/json-render-body-migration-plan.md

[claude]

Code Review for PR 3299 - docs-only planning document. Overall: thorough and well-organized. See inline comments for specific issues.

[claude]

Code review top-level summary (see inline comments for detail): Docs-only planning artifact -- thorough and well-organized. Four issues: (1) Load-time regressions in s2.4 web vitals table (heavy +114%, mega +130%) are not flagged or explained -- re-measure at n>=10 before the production PR ships. (2) Two stale line numbers: utils.rb:167 should be 165 for common_form_data; s4.2 request.rb:189-197 maps to add_bundle_to_form, not populate_form_with_bundle_and_assets (165-187), and form_with_code is at 158-163. (3) s4.4 spec-update list cites ~:123-135 but misses the identical assertion at line 153 in a second test case. (4) s4.6 defensive audit underspecified -- suggest JSON.generate(common_form_data) unit test or a Sorbet sig as the concrete guard.

[claude]

Stale line number: utils.rb:167 should be 165. def self.common_form_data is currently at line 165 in react_on_rails_pro/lib/react_on_rails_pro/utils.rb.

[claude]

The line range here points to the wrong function. In the current request.rb:

• form_with_code is at lines 158–163
• populate_form_with_bundle_and_assets (which form_with_code calls when send_bundle: true) is at lines 165–187
• add_bundle_to_form (which actually has the Pathname/IO :body entries) starts at line 189

So request.rb:189-197 is add_bundle_to_form, not populate_form_with_bundle_and_assets. The conceptual chain (form_with_code → populate_form_with_bundle_and_assets → add_bundle_to_form) is correct; only the cited line range needs updating.

[claude]

The spec range ~:123-135 only covers the first test case. There is a second identical assertion at line 153 in request_spec.rb (the "raises duplicate bundle upload error" test case) that also asserts body.to_s.include("renderingRequest=console.log"). Both will need updating when the production implementation switches to JSON.

[claude]

The load-time column shows significant regressions that are not mentioned anywhere in the document:

• heavy_benchmark_traditional: 37 → 79 ms (+114%)
• mega_benchmark_traditional: 240 → 553 ms (+130%)

The hello_server TTFB spike has a footnote; these do not. At n=3 this is very likely noise, but without a note the reader cannot tell whether it was investigated. Recommend either adding a footnote explaining why these are noise (e.g., renderer GC or JIT warmup on first full-page load) or, better, requiring a re-run at ≥10 iterations as a prerequisite in the production PR rather than leaving it as optional ("if it matters").