NO MERGE: Replacement for #2835 (reapply RSC payload embedding optimization)#2879
NO MERGE: Replacement for #2835 (reapply RSC payload embedding optimization)#2879
Conversation
WalkthroughThis PR embeds already-serialized NDJSON RSC lines directly into inline Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PreloadedStore as Preloaded\nPayloads
participant Stream
participant Parser as NDJSON\nParser
participant Utils as replayConsoleLog
participant DOM
Client->>PreloadedStore: read RSCPayloadChunk[]
PreloadedStore->>Stream: createReadableStream(Uint8Array chunks)
Stream->>Parser: decode incremental bytes -> split NDJSON lines
Parser->>Utils: extract consoleReplayScript (if any)
Utils->>DOM: inject <script nonce?> with replay code
Parser->>Client: push parsed RSC chunks into React model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR reapplies the RSC payload embedding optimization from #2835, eliminating the double Key changes:
Confidence Score: 5/5Safe to merge — optimization is well-scoped, HTML-injection safety is preserved by the existing escapeScript helper, and the test suite covers all meaningful edge cases. No P0 or P1 findings. The optimization correctly relies on the JSON-is-a-strict-subset-of-JS property; escapeScript continues to neutralize the only two HTML-parser threats inside a script tag. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "Reapply "Eliminate double JSON.stringify..." | Re-trigger Greptile |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
size-limit report 📦
|
ReviewThe optimization is well-motivated (~24% payload reduction is meaningful) and the implementation is clean. The new NDJSON line-buffering logic correctly handles chunk boundary splits and multibyte UTF-8, and the test coverage for those edge cases (boundary splits, CRLF, malformed JSON) is solid. Breaking change — requires coordinated deployment The format of
With a rolling deployment (old and new pods serving simultaneously), mismatched versions produce a silent blank render — Recommendation: document this as a hard version-lock between the server-side and client-side packages, and consider adding a runtime format-detection guard (e.g. Console log replay ordering change Previously Escaping correctness The Minor items (see inline comments)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 34-35: Update the changelog entry in CHANGELOG.md so it points to
the actual landing PR 2879 instead of PR 2835: replace the PR reference text
"[PR 2835](https://github.com/shakacode/react_on_rails/pull/2835) by
[justin808](https://github.com/justin808)" with the correct reference to PR 2879
and the appropriate author/PR URL (e.g., "[PR
2879](https://github.com/shakacode/react_on_rails/pull/2879) by <author>")
ensuring the displayed PR number and link match the merged PR that shipped the
change.
In `@packages/react-on-rails-pro/src/getReactServerComponent.client.ts`:
- Around line 128-133: The current override of payloads.push replaces the array
storage and prevents new chunks from being retained, which breaks multiple
consumers; fix by preserving the backing array: capture the original push (e.g.,
const originalPush = payloads.push.bind(payloads)) and replace payloads.push
with a wrapper that calls originalPush(...chunks) to store them, then iterates
chunks.forEach(handleChunk), and returns the originalPush result; reference the
payloads variable and handleChunk function when making the change so preloaded
chunks are both emitted and kept for future readers.
🪄 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: e4719a3d-9efd-4ad9-b99b-d2c7da5feef5
📒 Files selected for processing (8)
CHANGELOG.mdpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.tspackages/react-on-rails-pro/src/utils.tspackages/react-on-rails-pro/tests/RSCRequestTracker.test.tspackages/react-on-rails-pro/tests/injectRSCPayload.test.tspackages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
…lacement-2835-main * origin/main: (44 commits) Fix Content-Length mismatch and null renderingRequest errors in node renderer (#3069) Improve memory debugging docs with simpler heap snapshot approach (#3072) Enforce strict version matching in doctor and recommend doctor on errors (#3070) Remove immediate_hydration feature from codebase (#2834) Fix infinite fork loop when node renderer worker fails to bind port (#2881) Fix TanStack Router SSR hydration mismatch in Pro async path (#2932) Improve node renderer error messages for malformed render requests (#3068) Add interactive mode prompt to create-react-on-rails-app (#3063) docs: replace hardcoded version numbers with unversioned install commands (#2893) Remove Contributing section from docs sidebar (#3064) Consolidate docs comparison pages into single evaluation entry (#3065) Docs route cleanup: canonicalize worst verbose URL slugs (#3067) Add agent summary blocks to high-value docs pages (#3066) Fix brittle positional assertions in create-app tests (#2923) Auto-resolve renderer password from ENV in Rails Pro (#2921) Standardize bundle env vars from =yes to =true (#2925) Fix Pro generator multiline and template-literal rewrites (#2918) Fix spec/dummy Procfile.dev SERVER_BUNDLE_ONLY to match template convention (#2922) Add llms docs entry points (#2916) Bump version to 16.6.0.rc.0 ... # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/RSCRequestTracker.test.ts (1)
255-255: Byte-size comments are now approximate after NDJSON wrapping.Since each chunk is wrapped as JSON plus
\n, totals are slightly above the stated 32KB/64KB/128KB. Consider wording these as “~32KB/~64KB/~128KB” to keep comments precise.Also applies to: 280-280, 336-336, 360-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro/tests/RSCRequestTracker.test.ts` at line 255, Update the inline comments that state exact chunk sizes around calls to pushNDJSONChunks in RSCRequestTracker.test to indicate approximate sizes (e.g., change "32KB/64KB/128KB" to "~32KB/~64KB/~128KB") because NDJSON wrapping adds extra bytes; locate the comments adjacent to the pushNDJSONChunks(...) calls (the ones using chunkSize 'z' and the other pushNDJSONChunks invocations) and adjust the wording in each occurrence so the comments reflect approximate totals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-on-rails-pro/tests/RSCRequestTracker.test.ts`:
- Line 255: Update the inline comments that state exact chunk sizes around calls
to pushNDJSONChunks in RSCRequestTracker.test to indicate approximate sizes
(e.g., change "32KB/64KB/128KB" to "~32KB/~64KB/~128KB") because NDJSON wrapping
adds extra bytes; locate the comments adjacent to the pushNDJSONChunks(...)
calls (the ones using chunkSize 'z' and the other pushNDJSONChunks invocations)
and adjust the wording in each occurrence so the comments reflect approximate
totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21a970cd-f6f6-4b64-b536-a51c27842eee
📒 Files selected for processing (7)
CHANGELOG.mdpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/tanstack-router/index.tspackages/react-on-rails-pro/src/tanstack-router/serverRender.tspackages/react-on-rails-pro/tests/RSCRequestTracker.test.tspackages/react-on-rails-pro/tests/injectRSCPayload.test.tspackages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-on-rails-pro/tests/injectRSCPayload.test.ts
- packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
- packages/react-on-rails-pro/src/getReactServerComponent.client.ts
Code ReviewSummaryThe optimization is sound: embedding validated JSON directly as a JavaScript expression (instead of double-stringifying it) saves one full round-trip through the serializer/deserializer on both server and client. The performance gains (~38KB, ~24% reduction) are plausible and well-explained. Security Analysis: JSON-as-JS EmbeddingThe comment in
The Issues1.
|
| const previousPush = payloads.push.bind(payloads); | ||
| payloads.forEach(handleChunk); | ||
| // eslint-disable-next-line no-param-reassign | ||
| payloads.push = (...chunks) => { | ||
| payloads.push = (...chunks: RSCPayloadChunk[]) => { | ||
| const newLength = previousPush(...chunks); | ||
| chunks.forEach(handleChunk); | ||
| return chunks.length; | ||
| return newLength; | ||
| }; |
There was a problem hiding this comment.
The payloads.push override is permanent and never cleaned up. Two concerns:
-
React Strict Mode double-invocation: In React 18 Strict Mode, effects (and the
startcallback path that triggers this) can be invoked twice. IfcreateFromPreloadedPayloadsis called twice with the samepayloadsarray, the override chains —previousPushin the second call captures the already-wrapped push from the first. Each subsequentpayloads.push(chunk)then calls both handlers. -
No cleanup on stream cancel: If the consumer cancels the stream (component unmounts mid-hydration),
payloads.pushremains overridden for the lifetime of the page.
Consider adding a cancel handler that restores the original push and prevents future handler invocations:
| const previousPush = payloads.push.bind(payloads); | |
| payloads.forEach(handleChunk); | |
| // eslint-disable-next-line no-param-reassign | |
| payloads.push = (...chunks) => { | |
| payloads.push = (...chunks: RSCPayloadChunk[]) => { | |
| const newLength = previousPush(...chunks); | |
| chunks.forEach(handleChunk); | |
| return chunks.length; | |
| return newLength; | |
| }; | |
| const previousPush = payloads.push.bind(payloads); | |
| payloads.forEach(handleChunk); | |
| // eslint-disable-next-line no-param-reassign | |
| payloads.push = (...chunks: RSCPayloadChunk[]) => { | |
| const newLength = previousPush(...chunks); | |
| chunks.forEach(handleChunk); | |
| return newLength; | |
| }; | |
| streamController = controller; |
cancel() {
closed = true;
// Restore the original push so late-arriving script tags don't
// accumulate handlers on a stream that's no longer reading.
payloads.push = previousPush;
},(Requires hoisting previousPush out of start so cancel can reference it.)
| try { | ||
| JSON.parse(jsonLine); | ||
| } catch { | ||
| throw new Error(`Malformed NDJSON line in RSC stream: ${jsonLine.slice(0, 100)}`); | ||
| } |
There was a problem hiding this comment.
JSON.parse runs on every NDJSON line purely for validation, then discards the result. For high-throughput RSC streams (many small chunks), this is a full parse-plus-GC per line on the server critical path.
The upstream RSC stream is NDJSON by contract (React generates valid JSON). If the validation is specifically guarding against malformed input from untrusted sources, it belongs here. But if this is defensive coding against bugs in React's own serializer, the overhead may not be worth it in production.
One middle ground: run the validation only in development (process.env.NODE_ENV !== 'production'), since broken RSC serialization would also manifest as hydration errors on the client anyway.
| try { | |
| JSON.parse(jsonLine); | |
| } catch { | |
| throw new Error(`Malformed NDJSON line in RSC stream: ${jsonLine.slice(0, 100)}`); | |
| } | |
| try { | |
| JSON.parse(jsonLine); | |
| } catch { | |
| throw new Error(`Malformed NDJSON line in RSC stream: ${jsonLine.slice(0, 100)}`); | |
| } |
Could become:
if (process.env.NODE_ENV !== 'production') {
try {
JSON.parse(jsonLine);
} catch {
throw new Error(`Malformed NDJSON line in RSC stream: ${jsonLine.slice(0, 100)}`);
}
}This trades a production-only performance win for dev-time error clarity. Worth deciding explicitly rather than leaving the validation always-on by default.
| const finalDecodedValue = lastIncompleteChunk + decoder.decode(); | ||
| if (finalDecodedValue.trim() !== '') { | ||
| parseAndHandleLines(finalDecodedValue.split('\n')); | ||
| } |
There was a problem hiding this comment.
Good fix — this handles the case where the RSC stream ends without a trailing \n, which would otherwise silently drop the last NDJSON object.
One edge case: if the stream ends with only whitespace (e.g., a trailing \r\n), finalDecodedValue.trim() !== '' correctly skips it. But parseAndHandleLines internally does line.trim() !== '' filtering, so even without the outer trim() guard it would be a no-op. The outer check is still good as an early-exit optimization. ✓
| } catch { | ||
| throw new Error(`Malformed NDJSON line in RSC stream: ${jsonLine.slice(0, 100)}`); | ||
| } | ||
| return createScriptTag(`(${cacheKeyJSArray(cacheKey)}).push(${jsonLine})`, sanitizedNonce); |
There was a problem hiding this comment.
Security note (informational, not blocking): The claim "Valid JSON is a strict subset of JavaScript expressions" is accurate for ES2019+ environments. Two specific sequences that could break HTML parsing are handled correctly by escapeScript():
</script>→<\/script>: HTML parser doesn't see the close tag; JS engine evaluates\/as/. ✓<!--→<\!--: HTML parser sees no comment opener; JS engine evaluates\!as!(unrecognized escape — only numeric legacy octal escapes are banned in strict mode, so\!is valid in both strict and sloppy mode). ✓
U+2028 / U+2029 (LINE SEPARATOR / PARAGRAPH SEPARATOR): valid JSON, valid in ES2019+ JS string literals. All supported browsers handle this. ✓
The approach is safe. The comment explaining it is accurate and sufficient.
2 participants
Summary
Important
Commit
3a208537Reapply "Eliminate double JSON.stringify in RSC payload embedding (Eliminate double JSON.stringify in RSC payload embedding #2835)" (Revert "Eliminate double JSON.stringify in RSC payload embedding (#2835)" #2878)Note
Medium Risk
Changes how RSC Flight data is streamed and embedded into HTML by injecting raw JSON as executable JS expressions and adding NDJSON line buffering/validation; mistakes here could break hydration or introduce XSS-like issues if assumptions about JSON safety are violated.
Overview
Reduces React Server Components (Pro) HTML-stream payload size by eliminating a second
JSON.stringifywhen embedding RSC Flight chunks;injectRSCPayloadnow pushes each NDJSON object directly intowindow.REACT_ON_RAILS_RSC_PAYLOADSas a JS expression (withJSON.parsevalidation) instead of a quoted string.Updates client hydration to consume preloaded payloads as parsed
RSCPayloadChunkobjects (noJSON.parsestep), adds robust NDJSON buffering for split JSON boundaries and multibyte UTF-8 splits, and centralizes console-log replay logic via a newreplayConsoleLogutility. Tests are updated/expanded to cover boundary-splitting, CRLF normalization, malformed NDJSON erroring, and the new.push({ ... })embedding format.Reviewed by Cursor Bugbot for commit 50ff949. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit