Revert "Eliminate double JSON.stringify in RSC payload embedding (#2835)"#2878
Revert "Eliminate double JSON.stringify in RSC payload embedding (#2835)"#2878
Conversation
…)" This reverts commit d1d8e6f.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis PR refactors RSC payload handling in react-on-rails-pro, changing from NDJSON line-based processing to per-chunk JSON serialization. Payloads transition from chunk objects to stringified representations, console replay moves inline, and the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const finalChunk = (lastIncompleteLine + decoder.decode()).trim(); | ||
| if (finalChunk !== '') { | ||
| const payloadScript = createRSCPayloadChunk(finalChunk, rscPayloadKey, sanitizedNonce); | ||
| const decodedChunk = typeof chunk === 'string' ? chunk : decoder.decode(chunk); |
There was a problem hiding this comment.
Bug: TextDecoder reused without { stream: true } — will corrupt multibyte UTF-8 across chunk boundaries.
A single TextDecoder instance is created at line 91 and reused across every iteration of the for await loop. Without the { stream: true } option, the decoder flushes its internal buffer on every call, so a multibyte character (e.g. a 4-byte emoji or CJK codepoint) that is split across two consecutive binary chunks will be decoded as the replacement character U+FFFD in the first chunk and then corrupt the second.
| const decodedChunk = typeof chunk === 'string' ? chunk : decoder.decode(chunk); | |
| const decodedChunk = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true }); |
The corresponding test ("handles chunks that split a multibyte UTF-8 character") was removed by this revert — it should be kept (or re-added in the follow-up PR) to guard this path.
| try { | ||
| while (!done) { | ||
| const decodedValue = typeof value === 'string' ? value : decoder.decode(value, { stream: true }); | ||
| const decodedValue = typeof value === 'string' ? value : decoder.decode(value); |
There was a problem hiding this comment.
Same TextDecoder streaming bug as in injectRSCPayload.ts.
decoder.decode(value) without { stream: true } will mishandle a multibyte character split between two stream chunks.
| const decodedValue = typeof value === 'string' ? value : decoder.decode(value); | |
| const decodedValue = typeof value === 'string' ? value : decoder.decode(value, { stream: true }); |
| parseAndHandleLines(finalDecodedValue.split('\n')); | ||
| } | ||
|
|
||
| controller.close(); |
There was a problem hiding this comment.
Silent data loss: the final incomplete NDJSON line is never flushed.
When the upstream stream ends, lastIncompleteChunk may still contain a valid NDJSON line that never ended with \n. The loop exits and controller.close() is called immediately, dropping that data silently.
PR #2835 fixed this by flushing after the loop:
const finalDecodedValue = lastIncompleteChunk + decoder.decode(); // flush remaining bytes
if (finalDecodedValue.trim() !== '') {
parseAndHandleLines(finalDecodedValue.split('\n'));
}This is a pre-existing regression worth carrying forward as a fix in the follow-up PR.
| expect(result).toContain('.push('); | ||
| expect(result).toContain(JSON.stringify(payload)); |
There was a problem hiding this comment.
Weakened assertion: the two toContain checks are independent and can match anywhere in the output.
The prior assertion expect(result).toContain(\.push(${payload})`)verified that the payload appeared as the argument to.push(in a single expression. Splitting it into two separate checks means the test passes even if.push(andJSON.stringify(payload)appear in completely different parts of the output (e.g. the payload in the HTML body and.push(` in an unrelated script tag).
Consider tightening it back to:
expect(result).toContain(`.push(${JSON.stringify(payload)})`);![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: 123d474d0e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const finalChunk = (lastIncompleteLine + decoder.decode()).trim(); | ||
| if (finalChunk !== '') { | ||
| const payloadScript = createRSCPayloadChunk(finalChunk, rscPayloadKey, sanitizedNonce); | ||
| const decodedChunk = typeof chunk === 'string' ? chunk : decoder.decode(chunk); |
There was a problem hiding this comment.
The RSC payload loop decodes each binary chunk with decoder.decode(chunk) (no { stream: true }), so a UTF-8 character split across chunk boundaries is replaced with U+FFFD before being embedded into <script> tags. This causes silent data corruption for non-ASCII/emoji content in Flight payloads, because the client only ever sees the already-corrupted string.
Useful? React with 👍 / 👎.
| parseAndHandleLines(finalDecodedValue.split('\n')); | ||
| } | ||
|
|
||
| controller.close(); |
There was a problem hiding this comment.
The transformer keeps partial NDJSON in lastIncompleteChunk, but on EOF it closes immediately without parsing that buffered remainder. When the final payload line does not end with \n, the last RSC chunk is dropped, which can leave hydration with incomplete Flight data (missing final UI chunk or unresolved suspense).
Useful? React with 👍 / 👎.
size-limit report 📦
|
|
Review comment - see full text below |
|
Review: Revert PR 2835 The revert itself is clean and the intent is clear. A few pre-existing bugs that PR 2835 had fixed are being re-introduced - worth tracking so the follow-up PR can address them explicitly. Issues to carry forward in the follow-up PR
Both injectRSCPayload.ts (line 258) and transformRSCStreamAndReplayConsoleLogs.ts (line 67) share the same bug: a TextDecoder instance is constructed once and reused across iterations of an async loop, but each call uses decoder.decode(chunk) without { stream: true }. This flushes the decoder's internal buffer on every call, which corrupts any multibyte UTF-8 character (emoji, CJK, etc.) that happens to be split across two consecutive binary chunks. The fix is one word: decoder.decode(chunk, { stream: true }). PR 2835 fixed this and included a dedicated test that is now removed by this revert.
After the while loop exits, lastIncompleteChunk may still contain a valid payload line that never ended with a newline. The code jumps straight to controller.close() and the data is lost silently. PR 2835 flushed this buffer after the loop; the follow-up PR should restore that flush.
This revert removes several correctness tests worth keeping regardless of the implementation approach: handles chunks that split across JSON object boundaries; handles chunks that split a multibyte UTF-8 character; normalizes CRLF on the final buffered payload chunk; emits an error instead of embedding malformed NDJSON as invalid JavaScript. These cover real streaming failure modes independent of whether the optimization from 2835 is in place.
The two separate toContain('.push(') and toContain(JSON.stringify(payload)) checks are weaker than the original single-expression check - they can pass even if the two strings appear in unrelated parts of the output. See inline comment. The revert is appropriate and the PR description is clear. The items above are pre-existing regressions being restored; none block merging, but they should be in scope for the follow-up PR. |
Greptile SummaryThis PR is a pure git revert of merge commit Key changes (all intentional reversions):
Known regressions reintroduced (track for follow-up PR):
Confidence Score: 4/5Safe to merge as an intentional revert, but reintroduces known P1 bugs that must be addressed in the follow-up PR before the feature lands again The revert is mechanically correct. Three P1 regressions are reintroduced: silent NDJSON final-chunk data loss, UTF-8 multi-byte corruption across chunk boundaries, and a hanging ReadableStream in non-browser environments. These are pre-existing bugs being knowingly restored; merging is reasonable with eyes open if the team intends to fix them in a follow-up. packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts and packages/react-on-rails-pro/src/injectRSCPayload.ts contain the most impactful regressions Important Files Changed
Sequence DiagramsequenceDiagram
participant RSC as RSC Stream Node.js
participant Inject as injectRSCPayload
participant Browser as Browser
participant GlobalArr as window RSC Payloads array
participant Transform as transformRSCStream
participant React as createFromReadableStream
RSC->>Inject: raw chunk string or Buffer
Note over Inject: decoder.decode no stream true flag
Inject->>Inject: JSON.stringify decodedChunk
Inject->>Browser: script tag pushing stringified chunk
Browser->>GlobalArr: script executes pushes string
GlobalArr->>Transform: string array via createRSCStreamFromArray
Note over Transform: split by newline then JSON.parse each line
Note over Transform: lastIncompleteChunk never flushed at end
Transform->>React: Uint8Array html field of RSCPayloadChunk
React-->>Browser: hydrated React tree
Reviews (1): Last reviewed commit: "Revert "Eliminate double JSON.stringify ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ({ value, done } = await reader.read()); | ||
| } | ||
|
|
||
| const finalDecodedValue = lastIncompleteChunk + decoder.decode(); | ||
| if (finalDecodedValue.trim() !== '') { | ||
| parseAndHandleLines(finalDecodedValue.split('\n')); | ||
| } | ||
|
|
||
| controller.close(); |
There was a problem hiding this comment.
After the while (!done) loop ends, lastIncompleteChunk holds any data that was buffered from a line not yet terminated with \n. The loop calls controller.close() immediately without flushing this remaining buffer. If the RSC stream's final NDJSON line does not end with a newline, the data is silently lost.
PR #2835 fixed this by processing lastIncompleteChunk after the loop:
const finalDecodedValue = lastIncompleteChunk + decoder.decode();
if (finalDecodedValue.trim() !== '') {
parseAndHandleLines(finalDecodedValue.split('\n'));
}This is a known regression being reintroduced and should be tracked for the planned follow-up PR.
| start(controller) { | ||
| // Browser-only by design (callers read from window.REACT_ON_RAILS_RSC_PAYLOADS). | ||
| // If called outside the browser, close immediately to avoid hanging streams. | ||
| if (typeof window === 'undefined') { | ||
| closed = true; | ||
| controller.close(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When typeof window === 'undefined', the start callback returns early without closing controller and without assigning streamController. The code executed after the new ReadableStream(...) constructor also silently no-ops because streamController is undefined. Any consumer reading from this stream will hang indefinitely. The version from PR #2835 explicitly called controller.close() in this branch. While this path is unlikely to be exercised in practice (callers guard on window.REACT_ON_RAILS_RSC_PAYLOADS), it is worth tracking for the follow-up PR.
| const finalChunk = (lastIncompleteLine + decoder.decode()).trim(); | ||
| if (finalChunk !== '') { | ||
| const payloadScript = createRSCPayloadChunk(finalChunk, rscPayloadKey, sanitizedNonce); | ||
| const decodedChunk = typeof chunk === 'string' ? chunk : decoder.decode(chunk); |
There was a problem hiding this comment.
TextDecoder without { stream: true } corrupts multi-byte UTF-8 across chunk boundaries
decoder.decode(chunk) (without { stream: true }) signals to the TextDecoder that this is the final call, flushing its internal state. Any multi-byte UTF-8 sequence (e.g., emoji, CJK characters) that straddles two consecutive stream chunks will produce a replacement character (U+FFFD) instead of the correct code point.
PR #2835 fixed this with decoder.decode(chunk, { stream: true }) and also made the decoder local to each stream's IIFE so that concurrent streams don't share state. Both regressions are reintroduced here. The test case "handles chunks that split a multibyte UTF-8 character" was removed as part of this revert — worth noting for the follow-up PR.
| if (replayConsoleCode?.trim() !== '') { | ||
| const scriptElement = document.createElement('script'); | ||
| if (sanitizedNonce) { | ||
| scriptElement.nonce = sanitizedNonce; | ||
| } | ||
| scriptElement.textContent = replayConsoleCode; | ||
| document.body.appendChild(scriptElement); | ||
| } |
There was a problem hiding this comment.
document accessed without an SSR guard
document.createElement('script') is called here without first checking typeof document !== 'undefined'. The extracted replayConsoleLog helper in PR #2835's version of utils.ts carried an explicit guard:
if (typeof document === 'undefined') {
return;
}Although this code path lives in a .client.ts file and is unlikely to run in SSR, the defensive check is a best practice and prevents accidental crashes if the function is ever invoked in a Node.js context during testing or SSR. Worth restoring in the follow-up PR.
…ew-fixes * origin/main: Remove dependency on internal TanStack Router router.ssr flag (#2833) Revert "Eliminate double JSON.stringify in RSC payload embedding (#2835)" (#2878) Eliminate double JSON.stringify in RSC payload embedding (#2835) docs: align Pro references with canonical docs routes (#2866) docs: make Pro route entry points explicit (#2867) Bump fastify from 5.8.1 to 5.8.3 in the npm-security group across 1 directory (#2846) docs: add RoR-specific competitive landscape and template refs (#2869) Clarify streaming narrative in RSC docs (#2813) (#2814)
…)" (#2878) ## Summary - Reverts merge commit `d1d8e6f9` from PR #2835. - Restores `main` to the state before "Eliminate double JSON.stringify in RSC payload embedding". ## Why - PR #2835 was merged accidentally and needs to be backed out. - A fresh follow-up PR can be opened afterward with any remaining fixes. ## Test Plan - Not run (pure git revert of previously reviewed code). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches Pro RSC streaming/hydration by changing how payload chunks are embedded, decoded, and replayed, which could reintroduce payload overhead and affect progressive hydration/stream correctness. Mostly a revert, but it alters client/server stream contracts and related tests. > > **Overview** > Reverts the prior Pro RSC optimization that embedded parsed JSON payload objects directly into the HTML stream. > > RSC payload injection now pushes **stringified chunks** into `window.REACT_ON_RAILS_RSC_PAYLOADS` again (with the client updated to expect `string[]`), and `injectRSCPayload` drops the NDJSON line-buffering/validation logic in favor of embedding each incoming stream chunk as-is. > > Console replay handling is moved out of `utils.ts` (removing `replayConsoleLog`) and is performed inside `transformRSCStreamAndReplayConsoleLogs`, with tests and the changelog updated to reflect the revert. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 123d474. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 participant
Summary
d1d8e6f9from PR Eliminate double JSON.stringify in RSC payload embedding #2835.mainto the state before "Eliminate double JSON.stringify in RSC payload embedding".Why
Test Plan
Note
Medium Risk
Touches Pro RSC streaming/hydration by changing how payload chunks are embedded, decoded, and replayed, which could reintroduce payload overhead and affect progressive hydration/stream correctness. Mostly a revert, but it alters client/server stream contracts and related tests.
Overview
Reverts the prior Pro RSC optimization that embedded parsed JSON payload objects directly into the HTML stream.
RSC payload injection now pushes stringified chunks into
window.REACT_ON_RAILS_RSC_PAYLOADSagain (with the client updated to expectstring[]), andinjectRSCPayloaddrops the NDJSON line-buffering/validation logic in favor of embedding each incoming stream chunk as-is.Console replay handling is moved out of
utils.ts(removingreplayConsoleLog) and is performed insidetransformRSCStreamAndReplayConsoleLogs, with tests and the changelog updated to reflect the revert.Written by Cursor Bugbot for commit 123d474. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes