Refactor and improve performance of the component that embed RSC payload into the html stream #1738
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant injectRSCPayload
participant HTMLStream
participant OutputStream
Client->>injectRSCPayload: Call with HTMLStream and context
injectRSCPayload->>HTMLStream: Listen for data/error/end
HTMLStream-->>injectRSCPayload: On data (HTML chunk)
injectRSCPayload->>injectRSCPayload: Buffer HTML chunk
injectRSCPayload->>injectRSCPayload: Buffer RSC init/payload scripts as needed
injectRSCPayload->>injectRSCPayload: Schedule flush (setTimeout)
injectRSCPayload->>OutputStream: On flush, write init scripts, HTML, then payload scripts
HTMLStream-->>injectRSCPayload: On end/error
injectRSCPayload->>OutputStream: Final flush and cleanup
OutputStream-->>Client: Combined output stream (ordered)
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
node_package/src/injectRSCPayload.ts (2)
22-24: Consider CSP-friendly<script>helperIf your app ever enforces a Content-Security-Policy with a
script-src 'nonce-…', you’ll need a way to inject that nonce attribute. A tiny change to let the caller pass attributes (or at least a nonce) would avoid future refactors.
124-136: Large-buffer allocation may spike memory
Buffer.allocUnsafe(totalSize)copies every pending sub-buffer into one mega-buffer.
For long-running streams this can transiently double memory (buffers + combined copy).
UsingresultStream.cork()/uncork()or simplyresultStream.write()each sub-buffer avoids the extra copy while still batching under Node’s internal high-water-mark.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node_package/src/injectRSCPayload.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
🔇 Additional comments (1)
node_package/src/injectRSCPayload.ts (1)
221-229: Race: initialization script may miss the very first flush
startRSC()pushes the initialization script but does not callscheduleFlush().
If the first HTML chunk scheduled a flush beforestartRSC()executes, the flush could run with an emptyrscInitializationBuffers, violating the “initialisation-before-HTML” guarantee.rscInitializationBuffers.push(Buffer.from(initializationScript)); +scheduleFlush(); // ensure array init goes out with or before first HTML chunk
…to 15.0.0.rc.0 for improved stability and features.
| rscInitializationBuffers.length = 0; | ||
| htmlBuffers.length = 0; | ||
| rscPayloadBuffers.length = 0; |
There was a problem hiding this comment.
| rscInitializationBuffers.length = 0; | |
| htmlBuffers.length = 0; | |
| rscPayloadBuffers.length = 0; | |
| rscInitializationBuffers = []; | |
| htmlBuffers = []; | |
| rscPayloadBuffers = []; |
(and make them let) seems more natural.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
node_package/src/injectRSCPayload.ts (1)
30-32: Payload-breaking</script>escape bug is still herePrevious review already flagged that escaping the whole script string corrupts the JSON payload when the chunk itself contains
</script>. The new version has not addressed it –escapeScript()is still called afterJSON.stringify(chunk), producing the invalid sequence"</\script>", which will throw onJSON.parseand break hydration.This is the exact issue described in the past review comment; please apply that fix (escape inside the chunk before it is stringified).
🧹 Nitpick comments (1)
node_package/src/injectRSCPayload.ts (1)
137-161: Manual buffer stitching adds complexity without gain
Buffer.allocUnsafe(totalSize)+ manualcopyloops work, butBuffer.concat([...])is simpler, less error-prone, and avoids having to pre-computetotalSize:-const combinedBuffer = Buffer.allocUnsafe(totalSize); -let offset = 0; -for (const buffer of rscInitializationBuffers) { - buffer.copy(combinedBuffer, offset); - offset += buffer.length; -} -… -resultStream.push(combinedBuffer); +const combinedBuffer = Buffer.concat([ + ...rscInitializationBuffers, + ...htmlBuffers, + ...rscPayloadBuffers, +]); +resultStream.push(combinedBuffer);Unless you have a benchmark proving a meaningful win, prefer the concise approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
node_package/src/injectRSCPayload.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: build
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
| import { PassThrough } from 'stream'; | ||
| import { finished } from 'stream/promises'; | ||
| import { createRSCPayloadKey } from './utils.ts'; | ||
| import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts'; |
There was a problem hiding this comment.
Drop the “.ts” extension in local imports
The emitted JavaScript files will be named utils.js and types/index.js, so at runtime Node will fail to resolve paths that still end with “.ts”.
-import { createRSCPayloadKey } from './utils.ts';
-import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts';
+import { createRSCPayloadKey } from './utils';
+import {
+ RailsContextWithServerComponentCapabilities,
+ PipeableOrReadableStream,
+} from './types/index';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { PassThrough } from 'stream'; | |
| import { finished } from 'stream/promises'; | |
| import { createRSCPayloadKey } from './utils.ts'; | |
| import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts'; | |
| import { PassThrough } from 'stream'; | |
| import { finished } from 'stream/promises'; | |
| import { createRSCPayloadKey } from './utils'; | |
| import { | |
| RailsContextWithServerComponentCapabilities, | |
| PipeableOrReadableStream, | |
| } from './types/index'; |
🤖 Prompt for AI Agents
In node_package/src/injectRSCPayload.ts at lines 1 to 4, the local import
statements incorrectly include the ".ts" extension, which will cause runtime
resolution errors since the emitted JavaScript files have ".js" extensions.
Remove the ".ts" extension from the import paths for './utils.ts' and
'./types/index.ts' so they read './utils' and './types/index' respectively.
AbanoubGhadban
deleted the
abanoubghadban/dont-send-first-chunk-until-shell-is-rendered
branch
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit