Fix RSC streaming: use React's flush() signal instead of setTimeout(0)#3196
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
size-limit report 📦
|
Code Review: Fix RSC Streaming with React's
|
Greptile SummaryThis PR fixes RSC streaming granularity by replacing two sources of unwanted batching: (1) Confidence Score: 5/5Safe to merge — no P0/P1 issues found; the flush signal approach is correctly wired and buffer draining on stream close is properly handled. Both changes are small and targeted. The React flush() hook is correctly assigned before any async rendering begins (React schedules work asynchronously so the assignment races safely). The setImmediate yield is correctly scoped to update chunks only and does not interfere with the chunk timeout mechanism. Remaining buffers are reliably drained by endResultStream() on htmlStream close. No P0 or P1 issues were identified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant NDJSON as NDJSON Stream
participant HIR as handleIncrementalRenderStream
participant React as React Renderer
participant htmlStream as htmlStream (PassThrough)
participant inject as injectRSCPayload (flush())
participant client as Result Stream / Client
NDJSON->>HIR: update chunk (setProp A)
HIR->>React: onUpdateReceived(A)
HIR->>HIR: await setImmediate (yield)
Note over React: performWork runs in gap<br/>pingedTasks → flushCompletedQueues
React->>htmlStream: write(HTML_A)
React->>htmlStream: flush() signal
htmlStream->>inject: flush() called
inject->>client: emit(rscInit + HTML_A + rscPayload)
NDJSON->>HIR: update chunk (setProp B)
HIR->>React: onUpdateReceived(B)
HIR->>HIR: await setImmediate (yield)
Note over React: separate performWork cycle
React->>htmlStream: write(HTML_B)
React->>htmlStream: flush() signal
htmlStream->>inject: flush() called
inject->>client: emit(HTML_B + rscPayload)
htmlStream->>inject: close event
inject->>inject: await rscPromise (all RSC settled)
inject->>client: endResultStream() → flush() remaining data + end()
Reviews (1): Last reviewed commit: "Fix RSC streaming: use React's flush() s..." | Re-trigger Greptile |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // | ||
| // See: https://github.com/facebook/react/pull/21625 | ||
| // See: https://github.com/facebook/react/blob/main/packages/react-server/src/ReactServerStreamConfigNode.js#L31-L38 | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
This works correctly — React checks if (destination.flush) destination.flush() at the end of each flushCompletedQueues cycle, so duck-typing the property onto htmlStream is the right approach.
One risk worth noting: this couples behavior to React's internal streaming convention, not a public Node.js stream contract. If React adds version-gating or changes the call site in a future major, flushing silently stops working (streaming degrades to one big chunk but nothing crashes). Consider adding a comment like:
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| // Verified against React 18.3 / 19.x ReactServerStreamConfigNode internals. | |
| // Re-verify this signal on major React version upgrades. | |
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
The yield is correct and necessary, but two things worth noting:
-
Overhead accumulates: each
setImmediateround-trip is ~0.1–1ms. For a page with N async props this adds N event-loop ticks to end-to-end processing. The streaming granularity win clearly outweighs this (per the PR benchmarks), but it should be documented so future maintainers know the tradeoff is intentional. -
The yield runs even on the last update chunk — a minor inefficiency since there's no subsequent React render to unblock. Not a bug, just a small unnecessary tick at stream end.
Suggested comment addition:
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); | |
| // Tradeoff: adds one event-loop tick per update chunk. For N async props | |
| // this costs N setImmediate round-trips (~0.1ms each), but the streaming | |
| // granularity gain far outweighs it (see PR #3196 benchmarks). | |
| // eslint-disable-next-line no-await-in-loop | |
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); |
![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: c254687136
ℹ️ 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".
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | ||
| flush(); | ||
| }; |
There was a problem hiding this comment.
injectRSCPayload now depends on destination.flush() as its flush trigger, but safePipe(...) starts source.pipe(htmlStream) before this callback is installed. If the source writes and calls flush() synchronously during pipe() (the shell-ready React path), that first flush signal is lost, so the initial buffered HTML/RSC data is held until a later cycle or final close, which undermines progressive first-chunk streaming and can hurt FCP on single-cycle renders. Install htmlStream.flush (and the data listener) before invoking safePipe.
Useful? React with 👍 / 👎.
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
c254687 to
f0c3755
Compare
|
|
||
| validate_packages_installed!(has_base_package, has_pro_package) | ||
| validate_no_duplicate_packages!(has_base_package, has_pro_package) | ||
| # validate_no_duplicate_packages!(has_base_package, has_pro_package) # temporarily disabled for yalc dev |
There was a problem hiding this comment.
Critical: Development artifact must not be merged.
This line disables a safety validation that prevents users from accidentally installing both react-on-rails and react-on-rails-pro simultaneously — a misconfiguration that causes hard-to-debug runtime errors.
yalc is a local package development tool (like npm link). The validation was disabled so the author could test against a local build of the pro package alongside the base package, but this change has no place in production code.
| # validate_no_duplicate_packages!(has_base_package, has_pro_package) # temporarily disabled for yalc dev | |
| validate_no_duplicate_packages!(has_base_package, has_pro_package) |
Please restore this line before merging.
| // | ||
| // See: https://github.com/facebook/react/pull/21625 | ||
| // See: https://github.com/facebook/react/blob/main/packages/react-server/src/ReactServerStreamConfigNode.js#L31-L38 | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
Reliance on undocumented / internal React API.
This monkey-patches a .flush() method onto htmlStream and relies on React calling destination.flush() at the end of flushCompletedQueues. While facebook/react#21625 confirms this pattern exists today, it is not part of React's public API contract for renderToPipeableStream.
Two concerns:
-
Silent regression risk: If React removes or renames
flushBuffered/destination.flush()in a future version, flushing will stop happening entirely without any error — the output stream will just fill up and be flushed only at stream-end. This is the kind of breakage that will be very hard to diagnose. -
No fallback: Consider logging a warning (dev-only) if
htmlStream.flushis never called, so developers know if the React version they upgraded to no longer triggers this hook.
At minimum, pin a comment referencing the exact React source commit so future maintainers can verify the behavior still holds during React upgrades.
| // separate output chunk per resolved Suspense boundary. | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); |
There was a problem hiding this comment.
Performance trade-off worth documenting; consider error-path behavior.
Each NDJSON update chunk now incurs an extra setImmediate round-trip, so a stream with N async props adds N event-loop yields. For real-world pages with tens or hundreds of concurrent Suspense boundaries this is intentional (that's the whole point), but it may add noticeable latency on non-streaming use-cases where all props resolve synchronously before any React flush fires.
Two suggestions:
-
Error path skips the yield — when
onUpdateReceivedthrows, thecatchblock is entered and thesetImmediatenever runs. That means on a fast sequence of erroring chunks there is no yield between them. This is probably fine (no React work to process on the error path), but worth a short comment to make the intent explicit. -
Consider a helper —
await new Promise<void>((resolve) => { setImmediate(resolve); })appears (or will appear) in more than one place. A named one-liner likeconst yieldToEventLoop = () => new Promise<void>((resolve) => setImmediate(resolve))at the module level makes the intent immediately clear at each call site without repeating the boilerplate.
Code ReviewOverviewThis PR fixes progressive RSC streaming by replacing the coarse
The problem statement is well-diagnosed and the benchmark data is convincing (+172% → +23% FCP on the product page). Issues🔴 Must-fix before merge
🟡 Should address
The
🟢 Suggestions / minor
const yieldToEventLoop = () => new Promise<void>((resolve) => setImmediate(resolve));This makes the call site ( No new tests for the new flush/yield behaviors The existing tests in
Given that this is precisely the behavior being fixed, having regression tests here would be valuable to prevent the fix from silently reverting. SummaryThe core approach is sound — using React's own signal for flush granularity is strictly better than |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0c3755634
ℹ️ 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".
|
|
||
| validate_packages_installed!(has_base_package, has_pro_package) | ||
| validate_no_duplicate_packages!(has_base_package, has_pro_package) | ||
| # validate_no_duplicate_packages!(has_base_package, has_pro_package) # temporarily disabled for yalc dev |
There was a problem hiding this comment.
Disabling validate_no_duplicate_packages! removes the guard that this class explicitly promises (validate_version_and_package_compatibility! documents raising when both react-on-rails and react-on-rails-pro are installed), so a misconfigured Pro app can now boot in an unsupported dependency state instead of failing fast with remediation steps. This regression makes package/gem compatibility errors much harder to diagnose and allows invalid setups to reach runtime.
Useful? React with 👍 / 👎.
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
f0c3755 to
df793ad
Compare
| // Yield to the event loop after each update chunk so React's | ||
| // setImmediate(performWork) can fire. Without this, all setProp() | ||
| // calls batch into React's pingedTasks and are processed in a | ||
| // single performWork → single flushCompletedQueues, merging all | ||
| // resolved Suspense boundaries into one chunk. | ||
| // With this yield, each setProp triggers its own performWork → | ||
| // flushCompletedQueues → destination.flush(), producing a | ||
| // separate output chunk per resolved Suspense boundary. | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
The inline setImmediate promise is correct but worth extracting as a named utility — await yieldToEventLoop() at the call site is self-documenting and avoids the boilerplate if this pattern is needed again elsewhere:
| // Yield to the event loop after each update chunk so React's | |
| // setImmediate(performWork) can fire. Without this, all setProp() | |
| // calls batch into React's pingedTasks and are processed in a | |
| // single performWork → single flushCompletedQueues, merging all | |
| // resolved Suspense boundaries into one chunk. | |
| // With this yield, each setProp triggers its own performWork → | |
| // flushCompletedQueues → destination.flush(), producing a | |
| // separate output chunk per resolved Suspense boundary. | |
| // eslint-disable-next-line no-await-in-loop | |
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); | |
| // Yield to the event loop after each update chunk so React's | |
| // setImmediate(performWork) can fire. Without this, all setProp() | |
| // calls batch into React's pingedTasks and are processed in a | |
| // single performWork → single flushCompletedQueues, merging all | |
| // resolved Suspense boundaries into one chunk. | |
| // With this yield, each setProp triggers its own performWork → | |
| // flushCompletedQueues → destination.flush(), producing a | |
| // separate output chunk per resolved Suspense boundary. | |
| // Note: adds one event loop tick per update chunk (N chunks = N extra ticks). | |
| // eslint-disable-next-line no-await-in-loop | |
| await new Promise<void>((resolve) => setImmediate(resolve)); |
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | ||
| if (flushFallbackTimeout) { | ||
| clearTimeout(flushFallbackTimeout); | ||
| flushFallbackTimeout = null; | ||
| } | ||
| flush(); | ||
| }; |
There was a problem hiding this comment.
Silent stall risk for RSC-only data: when RSC payload chunks arrive and are pushed into rscPayloadBuffers (line 314), no flush is triggered — data waits for the next htmlStream.flush() from React. In the common case this is fine because RSC resolves around the same time as its Suspense boundary. But if RSC data arrives after the final HTML render cycle (i.e., React has stopped calling flush()) and before htmlStream.close, those RSC chunks sit buffered until endResultStream(). This is ultimately safe but can add noticeable latency for late-resolving RSC payloads. A scheduleFlushFallback() call after pushing to rscPayloadBuffers would bound that latency to one event loop tick.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df793ad583
ℹ️ 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".
| * Primary: React calls this at the end of each render cycle. | ||
| * Cancels the fallback timer and flushes immediately. | ||
| */ | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
htmlStream.flush is registered only after safePipe(pipeableHtmlStream, htmlStream, ...) has already started source.pipe(destination). If React writes shell output and calls destination.flush() synchronously during pipe() (the shell-ready fast path), that first render-cycle flush signal is dropped, so buffered HTML/RSC data waits for the fallback timer or final close instead of flushing immediately, which collapses early progressive chunks and can regress FCP on fast renders.
Useful? React with 👍 / 👎.
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
df793ad to
b53288b
Compare
Code ReviewOverall assessment: The fix is well-reasoned and the approach is sound. The dual-mechanism design (primary React signal + setTimeout fallback) is a smart defensive pattern. A few issues worth addressing before merge. What this PR doesTwo coordinated changes to fix RSC streaming granularity:
Both changes are correctly explained in the PR description and inline comments. Issues1. Missing test coverage for the primary flush path (medium)The existing tests in
A test simulating React's flush signal would be: it('flushes on React destination.flush() signal, not just setTimeout', async () => {
const htmlPassThrough = new PassThrough();
// ... setup ...
const resultStream = injectRSCPayload(htmlPassThrough, rscRequestTracker, 'node');
const chunks: string[] = [];
resultStream.on('data', (chunk: Buffer) => chunks.push(chunk.toString()));
// Simulate React: write HTML, then call flush()
htmlPassThrough.push(Buffer.from('<div>first</div>'));
(htmlPassThrough as PassThrough & { flush?(): void }).flush?.();
htmlPassThrough.push(Buffer.from('<div>second</div>'));
(htmlPassThrough as PassThrough & { flush?(): void }).flush?.();
htmlPassThrough.push(null);
await new Promise<void>(resolve => resultStream.once('end', resolve));
// Should produce at least 2 separate chunks (one per flush signal)
expect(chunks.length).toBeGreaterThanOrEqual(2);
});2. Fallback timer cancelled on early return — RSC data can be held longer than expected (low, but non-obvious)In
The data will eventually flush when HTML arrives (which re-calls 3.
|
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
Consider importing setImmediate from timers/promises instead of wrapping it manually:
| } catch (err) { | |
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); |
import { setImmediate as setImmediateP } from 'timers/promises';
// ...
await setImmediateP();This removes the manual new Promise wrapper and is marginally cleaner to read. timers/promises is available in all Node.js versions this package targets.
| if (flushFallbackTimeout) { | ||
| clearTimeout(flushFallbackTimeout); | ||
| flushFallbackTimeout = null; | ||
| } | ||
|
|
||
| // STREAMING CONSTRAINT: Don't start until we have HTML content | ||
| // This ensures the first chunk always contains HTML, which is required | ||
| // for proper page rendering and prevents empty initial chunks | ||
| if (!hasReceivedFirstHtmlChunk && htmlBuffers.length === 0) { | ||
| flushTimeout = null; | ||
| return; |
There was a problem hiding this comment.
The fallback timer is cancelled unconditionally before the early-return guard. If this flush() is triggered by React's destination.flush() signal while RSC init data is already buffered but no HTML has arrived yet, the timer is cancelled and no reschedule happens — the RSC init data sits until HTML arrives and calls scheduleFlushFallback() again.
In practice this is not a data-loss bug (HTML will eventually arrive), but it means RSC init scripts can be held an extra write cycle longer than necessary. Consider only cancelling the timer if you're actually going to flush:
| if (flushFallbackTimeout) { | |
| clearTimeout(flushFallbackTimeout); | |
| flushFallbackTimeout = null; | |
| } | |
| // STREAMING CONSTRAINT: Don't start until we have HTML content | |
| // This ensures the first chunk always contains HTML, which is required | |
| // for proper page rendering and prevents empty initial chunks | |
| if (!hasReceivedFirstHtmlChunk && htmlBuffers.length === 0) { | |
| flushTimeout = null; | |
| return; | |
| const flush = () => { | |
| // STREAMING CONSTRAINT: Don't start until we have HTML content | |
| // This ensures the first chunk always contains HTML, which is required | |
| // for proper page rendering and prevents empty initial chunks | |
| if (!hasReceivedFirstHtmlChunk && htmlBuffers.length === 0) { | |
| return; | |
| } | |
| // Cancel any pending fallback timer since we're flushing now | |
| if (flushFallbackTimeout) { | |
| clearTimeout(flushFallbackTimeout); | |
| flushFallbackTimeout = null; | |
| } | |
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
b53288b to
bc78a8d
Compare
Code ReviewOverviewThis PR fixes a real and well-diagnosed RSC streaming regression: RSC shell + all resolved Suspense content were collapsing into one giant chunk, hurting FCP on fast-query pages. The two-part fix ( Issues1. Relies on an undocumented React internal (medium risk) The 2. Node 22 ships 3. Per-chunk Adding one full event-loop yield after every update chunk means N chunks causes N idle event-loop cycles before processing completes. For high-frequency update streams (e.g. 50+ small NDJSON chunks), the cumulative latency adds up. Worth documenting the tradeoff, or skipping the yield on the last chunk when there is no more work pending. 4. Test weakening without a replacement assertion (minor) The test strips 5. The Positive notes
SummaryThe fix is sound and the performance numbers are compelling. The main risks are the undocumented React API dependency (mitigated by the fallback) and the missing positive streaming-granularity assertion in tests. Items 2 and 4 are the highest priority before merge; 1, 3, and 5 are lower priority but worth tracking. |
| // eslint-disable-next-line no-await-in-loop | ||
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
Node 22 has timers/promises which gives a cleaner spelling for this pattern:
| }); | |
| await import('timers/promises').then(({ setImmediate: si }) => si()); |
Or add an import at the top of the file:
import { setImmediate as yieldToEventLoop } from 'timers/promises';then replace the whole block with await yieldToEventLoop();. This avoids the manual Promise wrapping and makes the intent ("yield to event loop") clear from the name.
| // eslint-disable-next-line no-await-in-loop | ||
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
Throughput concern: this yields once per update chunk unconditionally. If a render produces N update chunks in rapid succession, you pay N full event-loop cycles even for chunks where no React work is actually waiting. For the common case this is fine, but for high-frequency streams it could add measurable latency.
One mitigation: skip the yield on the very last chunk (when the read loop is about to exit anyway), since there is nothing left to batch at that point. Another option: only yield if the next iteration is also going to call onUpdateReceived (peek ahead), but that complicates the loop.
| * Primary: React calls this at the end of each render cycle. | ||
| * flush() cancels the fallback timer internally. | ||
| */ | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
This relies on React's undocumented flushBuffered(destination) → destination.flush() internal convention. The fallback in scheduleFlushFallback is a good safety net, but note the fallback fires once per data event — not once per React render cycle. So if React ever stops calling destination.flush(), streaming reverts to the old per-tick batching (all Suspense boundaries merged again) without any error or warning.
Consider adding a short comment here documenting that degradation explicitly, so a future maintainer investigating "streaming is back to one big chunk" knows exactly where to look:
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| // Called by React at the end of each flushCompletedQueues cycle (React PR #21625). | |
| // If React stops calling this, streaming granularity silently degrades to one | |
| // chunk per event-loop tick (the scheduleFlushFallback path). No error is thrown. | |
| flush(); | |
| }; |
| // RSC Flight payload <script> tags contain the serialized React tree which | ||
| // includes fallback text as data (e.g., "Loading branch1..." inside a JSON | ||
| // string). We only want to assert that the fallback is not rendered as HTML. | ||
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); |
There was a problem hiding this comment.
The script-tag stripping is correct. However, the suite now has no assertion that verifies the core fix — that resolved Suspense boundaries arrive in separate chunks. If the setImmediate yield is accidentally removed, all the negative assertions below will still pass because content just moves to a later (merged) chunk.
Consider adding a positive structural assertion, e.g.:
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); | |
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); | |
| // Verify progressive streaming: there should be more than 2 chunks, meaning | |
| // resolved Suspense boundaries arrived in separate chunks rather than all batched together. | |
| expect(chunks.length).toBeGreaterThan(2); |
| flushTimeout = null; | ||
| }; | ||
|
|
||
| const endResultStream = () => { |
There was a problem hiding this comment.
The old if (flushTimeout) clearTimeout(flushTimeout); made it obvious that a pending timer was being cancelled on stream close. Now that cleanup lives inside flush(), this is less visible. A one-line comment here would preserve that intent:
| const endResultStream = () => { | |
| const endResultStream = () => { | |
| // flush() also cancels any pending flushFallbackTimeout | |
| flush(); | |
| if (!resultStream.writableEnded) { | |
| resultStream.end(); | |
| } | |
| }; |
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
bc78a8d to
d0724ba
Compare
Code Review: Fix RSC streaming with React flush() signalOverviewThis PR addresses a real performance problem: fast DB queries caused all resolved Suspense boundaries to merge into a single wire-format chunk, defeating progressive streaming and making RSC FCP worse than SSR. The two-part fix is well-diagnosed and well-explained. What's Good
Concerns1. Regression:
|
| if (!resultStream.writableEnded) { | ||
| resultStream.end(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The original endResultStream explicitly cancelled the timer before calling flush(). That guard was removed here, and the removal is benign in the current event-loop ordering (by the time this runs, flush() will either find data and cancel the timer itself, or the timer has already fired with empty buffers). But it's a silent safety-net regression.
| }; | |
| const endResultStream = () => { | |
| if (flushFallbackTimeout) { | |
| clearTimeout(flushFallbackTimeout); | |
| flushFallbackTimeout = null; | |
| } | |
| flush(); | |
| if (!resultStream.writableEnded) { | |
| resultStream.end(); | |
| } | |
| }; |
| // eslint-disable-next-line no-await-in-loop | ||
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); |
There was a problem hiding this comment.
Consider extracting this as a named utility at the module level so the eslint-disable and the intent are co-located once, rather than inline in the hot path:
// At module level:
const yieldToEventLoop = () => new Promise<void>((resolve) => { setImmediate(resolve); });
// Then here:
await yieldToEventLoop();Also worth a short note that this uses the real Node.js setImmediate (worker scope), not the VM-sandboxed stub from vm.ts — the two look similar to readers unfamiliar with the boundary.
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); | ||
| expect(secondChunkHtml).not.toContain('<p>Header for AsyncComponentsTreeForTesting</p>'); | ||
| expect(secondChunkHtml).not.toContain('<p>Footer for AsyncComponentsTreeForTesting</p>'); | ||
| expect(secondChunkHtml).not.toContain('Loading branch1...'); | ||
| expect(secondChunkHtml).not.toContain('Loading branch2...'); | ||
| expect(secondChunkHtml).not.toContain('branch1 (level 0)'); |
There was a problem hiding this comment.
The test fix (stripping <script> tags before asserting HTML absence) is correct — RSC Flight payloads embed fallback text as JSON, causing false positives.
However, this test only validates a correctness property (shell content isn't replicated in subsequent chunks). It doesn't assert the streaming granularity improvement this PR fixes. A regression that re-merges all Suspense resolutions back into one giant chunk would not fail this test.
Consider adding an assertion like:
// Each resolved Suspense boundary should arrive as its own chunk.
// The component has 4 async branches → at least 5 chunks (shell + 4 resolutions).
expect(chunks.length).toBeGreaterThanOrEqual(5);The timing test already has chunks.length >= 5, but making it explicit here too would protect the granularity contract going forward.
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
732e85f to
f1bc59c
Compare
Code Review: Fix RSC streaming with React's flush() signalOverviewThis PR fixes RSC streaming granularity by replacing Strengths
Issues1. Leaked fallback timer on stream end (medium severity)
If const endResultStream = () => {
if (flushFallbackTimeout) {
clearTimeout(flushFallbackTimeout);
flushFallbackTimeout = null;
}
flush();
if (!resultStream.writableEnded) {
resultStream.end();
}
};2. Skipped integration tests without replacement coverage (medium severity)Two Ruby integration tests in 3. Dependency on undocumented React internal (informational)The 4.
|
|
|
||
| const endResultStream = () => { | ||
| if (flushTimeout) clearTimeout(flushTimeout); | ||
| flush(); |
There was a problem hiding this comment.
The fallback timer is no longer explicitly cancelled before calling flush(). In the old code endResultStream did if (flushTimeout) clearTimeout(flushTimeout) before calling flush(). Now flush() only cancels flushFallbackTimeout when totalSize > 0 — the two early-return guards above the cancel logic mean a pending timer survives if buffers happen to be empty at this point.
The timer will harmlessly fire after resultStream.end() (finding no data), but it leaks a timer through stream close. Suggest cancelling unconditionally here:
| flush(); | |
| const endResultStream = () => { | |
| if (flushFallbackTimeout) { | |
| clearTimeout(flushFallbackTimeout); | |
| flushFallbackTimeout = null; | |
| } | |
| flush(); | |
| if (!resultStream.writableEnded) { | |
| resultStream.end(); | |
| } | |
| }; |
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
This pattern is correct but utils.ts:176 already notes that timers/promises should be used when Node 16 is the minimum (same reason setTimeout was discussed there). setImmediate from timers/promises avoids the manual wrapper and removes the need for eslint-disable-next-line no-await-in-loop:
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); | |
| await setImmediate(); |
(with import { setImmediate } from 'timers/promises'; at the top of the file)
| # which is not compatible with the length-prefixed wire format introduced in | ||
| # PR #3195. These tests need to be rewritten to use real React components | ||
| # with async props instead of raw stream fixtures. | ||
| it "sends stream values and receives them in the response", skip: "Needs rewrite: getStreamValues not in dummy bundle, raw stream incompatible with length-prefixed protocol (#3195)" do |
There was a problem hiding this comment.
These two tests are the only integration-level coverage for the incremental rendering feature, which is exactly what this PR changes. Skipping them (rather than rewriting them) leaves the critical path untested. Since the skip says "Needs rewrite: ... incompatible with length-prefixed protocol (#3195)", could the rewrites be included here or tracked in a dedicated issue linked from this PR? Merging with permanently skipped tests against a feature under active change is a coverage risk.
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
f1bc59c to
d8b0fcb
Compare
Code ReviewOverviewThis PR fixes a real performance regression in RSC streaming by addressing two root causes: (1) Strengths
IssuesHigh — Skipped tests leave a coverage gapTwo integration tests in Medium — React internal API dependency needs a version guard
Medium — No unit tests for the new primary flush path
Low — Type assertion for
|
| * Verified against React 18.3 / 19.x ReactServerStreamConfigNode internals. | ||
| * Re-verify this signal on major React version upgrades. | ||
| */ | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
The type assertion PassThrough & { flush?: () => void } types flush as optional (?), but we immediately assign it — so it's always defined after this line. The ? is misleading.
More importantly, directly mutating a property on a PassThrough instance via type assertion is fragile: if a future version of Node.js adds a flush method with a different signature to PassThrough or its prototype chain, this silently overrides it. A safer approach is a thin wrapper stream:
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| class FlushablePassThrough extends PassThrough { | |
| flush() { | |
| flush(); | |
| } | |
| } | |
| const htmlStreamFlushable = new FlushablePassThrough(); |
Or, if changing the stream type is too invasive, at least document the override risk explicitly here and use a non-optional type:
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| (htmlStream as PassThrough & { flush: () => void }).flush = () => { | |
| flush(); | |
| }; |
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
The correctness of this yield depends on FIFO ordering of setImmediate callbacks: React schedules setImmediate(performWork) during setProp() (called inside onUpdateReceived), so React's callback is queued before this setImmediate(resolve). In the same event loop pass the FIFO ordering guarantees React's performWork fires first.
This works today, but the comment should explicitly state the ordering dependency so it's not accidentally broken — e.g., by inserting any async work between onUpdateReceived and this yield that itself uses setImmediate:
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); | |
| // ORDERING INVARIANT: React's setImmediate(performWork) is queued | |
| // inside onUpdateReceived (via setProp → pingedTasks ping). Since | |
| // setImmediate callbacks fire in FIFO order, React's performWork | |
| // runs before this resolve, producing a separate flushCompletedQueues | |
| // cycle per Suspense boundary. Do not insert any setImmediate-based | |
| // async work between onUpdateReceived and this yield or the ordering | |
| // guarantee breaks. | |
| // eslint-disable-next-line no-await-in-loop | |
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); |
|
|
||
| describe "render_code_with_incremental_updates" do | ||
| it "sends stream values and receives them in the response" do | ||
| # These tests reference ReactOnRails.getStreamValues() which is not defined |
There was a problem hiding this comment.
Skipping these two tests reduces coverage of the bidirectional streaming path, which is exactly what this PR modifies. The skip message is clear, but please open a tracking issue before merging so this rewrite doesn't get lost. The skip: annotation alone is easy to overlook in future PRs.
Also worth noting: if getStreamValues is unavailable in the dummy bundle now (before this PR), these tests were likely already failing or being skipped in CI. If they were passing before, the skip should document what changed.
| // RSC Flight payload <script> tags contain the serialized React tree which | ||
| // includes fallback text as data (e.g., "Loading branch1..." inside a JSON | ||
| // string). We only want to assert that the fallback is not rendered as HTML. | ||
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); |
There was a problem hiding this comment.
The lazy [\s\S]*? is correct here since escapeScript() in injectRSCPayload.ts escapes all </script> occurrences inside payloads as <\/script>, so the first unescaped </script> always closes the intended tag.
Consider a brief comment to make this non-obvious invariant explicit for future readers:
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); | |
| // Strip <script> tags before asserting on HTML. RSC Flight payloads are | |
| // serialized inside <script> tags and may contain fallback strings as data | |
| // (e.g. "Loading branch1..."). The lazy regex is safe here because | |
| // escapeScript() ensures no literal </script> appears inside a payload. | |
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); |
The shell HTML + all resolved Suspense boundaries were being merged into a single wire-format message, defeating progressive streaming. On the product page this caused RSC FCP (370ms) to be 172% worse than SSR (136ms). Root cause: two issues combined to prevent progressive streaming. 1. injectRSCPayload.ts used setTimeout(flush, 0) to batch HTML chunks from React's 4KB internal buffer (which can split tags mid-attribute). But setTimeout(0) batches by event loop tick — too coarse — merging multiple render cycles into one output chunk. 2. handleIncrementalRenderStream.ts processed all NDJSON update chunks in a tight while loop. All setProp() calls batched into React's pingedTasks before setImmediate(performWork) could fire, producing one flushCompletedQueues call with everything merged. Fix: - Replace setTimeout(0) with React's destination.flush() signal as the primary flush mechanism, with setTimeout(0) kept as a fallback in case flush() is never called (e.g., future React version changes). React calls flushBuffered(destination) → destination.flush() at the end of every flushCompletedQueues cycle. This is the exact signal that a complete render batch is done — all HTML elements are whole, no partial tags. See facebook/react#21625. - Yield to the event loop (setImmediate) between NDJSON update chunks so each setProp triggers its own performWork → flushCompletedQueues → flush(), producing a separate output chunk per Suspense boundary. Both changes are needed together: flush() alone doesn't help because without yields all setProp calls batch into one performWork. Yields alone don't help because setTimeout(0) still merges everything. Closes #3194 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
fix/rsc-streaming-flush
branch
from
d8b0fcb to
8bf2f6b
Compare
Note on skipped integration testsThe two These tests were broken by PR #3195 ("Unify incremental rendering to use length-prefixed protocol"), which removed the The fixture fix is non-trivial because it affects both:
This should be addressed in a follow-up PR to #3195, not here. |
Code ReviewOverviewThis PR fixes RSC streaming granularity by replacing the monolithic The PR description is detailed and benchmarks are convincing. Overall the approach is sound. What's Good
Issues and Suggestions1. No unit test for the new primary flush mechanism The 2. Skipped Ruby specs need a follow-up issue The two skipped tests in 3. React internal API — document the verification procedure The comment says "Re-verify this signal on major React version upgrades" but doesn't say how to verify. The concrete check is: confirm 4. Minor: inline The VerdictThe core changes are correct and well-reasoned. The dual-mechanism flush approach is a good defensive pattern given the reliance on a React internal. Main gaps: no unit test for the new primary flush wiring, and no tracking issue for the skipped Ruby specs. Suggest opening that tracking issue before merging. |
| * Verified against React 18.3 / 19.x ReactServerStreamConfigNode internals. | ||
| * Re-verify this signal on major React version upgrades. | ||
| */ | ||
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { |
There was a problem hiding this comment.
The duck-typed cast is the right approach here, but consider noting how to verify this on React upgrades directly beside the cast:
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| // Verification: in ReactServerStreamConfigNode.js, confirm flushBuffered() still | |
| // calls dest.flush?.() (or equivalent). If the call site disappears, the fallback | |
| // setTimeout handles it — zero silent regression, just slightly worse granularity. | |
| (htmlStream as PassThrough & { flush?: () => void }).flush = () => { | |
| flush(); | |
| }; |
This makes the "re-verify on upgrade" note actionable rather than advisory.
| await new Promise<void>((resolve) => { | ||
| setImmediate(resolve); | ||
| }); |
There was a problem hiding this comment.
This allocates a new Promise on every update chunk. Consider a module-level helper to improve readability and avoid the repeated eslint-disable comment:
| await new Promise<void>((resolve) => { | |
| setImmediate(resolve); | |
| }); | |
| await yieldToEventLoop(); |
With a module-level constant before the function:
const yieldToEventLoop = () => new Promise<void>((resolve) => { setImmediate(resolve); });
| // RSC Flight payload <script> tags contain the serialized React tree which | ||
| // includes fallback text as data (e.g., "Loading branch1..." inside a JSON | ||
| // string). We only want to assert that the fallback is not rendered as HTML. | ||
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); |
There was a problem hiding this comment.
The non-greedy regex is correct here. One edge case worth documenting: escapeScript() already converts </script> to <\/script> inside script content, so the regex won't accidentally terminate early on embedded script-close sequences. A brief comment would help future readers:
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); | |
| // Strip <script> tags from the chunk before checking for rendered HTML. | |
| // RSC Flight payload <script> tags contain the serialized React tree which | |
| // includes fallback text as data (e.g., "Loading branch1..." inside a JSON | |
| // string). We only want to assert that the fallback is not rendered as HTML. | |
| // Note: escapeScript() converts </script> → <\/script> inside content, so | |
| // this regex won't terminate early on embedded script-close sequences. | |
| const secondChunkHtml = chunks[1].replace(/<script[\s\S]*?<\/script>/g, ''); |
|
|
||
| describe "render_code_with_incremental_updates" do | ||
| it "sends stream values and receives them in the response" do | ||
| # Broken by PR #3195 (length-prefixed protocol): fixture bundle writes raw |
There was a problem hiding this comment.
Skipping rather than removing is the right call since these tests represent intended behavior. However, without a tracking issue these will silently accumulate. Suggest adding a GitHub issue link to the skip message so it's actionable:
| # Broken by PR #3195 (length-prefixed protocol): fixture bundle writes raw | |
| skip: "Pre-existing: raw fixture incompatible with length-prefixed protocol — track in <issue-url>" do |
Even a placeholder is better than only a PR reference, since PRs get closed and lose visibility.
* Bump react_on_rails to v16.5.1 with RSC streaming flush fix Pins react-on-rails JS packages to the v16.5.1-rsc-flush-fix tag on the builds repo and lets Gemfile.lock resolve the Ruby gems to the matching tip of upcoming-v16.3.0 (commit 03e7787e). The upstream fix (shakacode/react_on_rails#3196) replaces the setTimeout(0) chunk-batching in injectRSCPayload with React's destination.flush() signal plus a setTimeout fallback, eliminating the ~400ms FCP regression that was batching RSC shell and resolved content into one chunk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use react-on-rails-rsc perf/rsc-revive-model-walk branch Switches react-on-rails-rsc from the 19.0.5-rc.1 release to the perf/rsc-revive-model-walk branch (resolved at 67b27f08) for the RSC model-walk performance improvements. Pinned via pnpm-lock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#3196) ## Summary - Replace `setTimeout(flush, 0)` in `injectRSCPayload.ts` with React's `destination.flush()` signal for per-render-cycle flushing - Yield to the event loop (`setImmediate`) between NDJSON update chunks in `handleIncrementalRenderStream.ts` so each async prop triggers a separate React render cycle ## Problem The RSC shell HTML + all resolved Suspense boundary content were being merged into a single wire-format message, defeating progressive streaming. On pages with fast DB queries (product, blog, restaurant search), RSC FCP was **worse** than SSR. **Product page before fix:** | Metric | SSR | RSC | |--------|-----|-----| | FCP | 136ms | 370ms (+172%) | | First wire msg | 70KB | 124KB (shell + all resolved + Flight data) | **Product page after fix (with 3ms sleep workaround confirming the diagnosis):** | Metric | SSR | RSC | |--------|-----|-----| | FCP | 140ms | 172ms (+23%) | | First wire msg | 70KB | 38KB (shell + skeletons only) | ## Root Cause Two issues combined: 1. **`injectRSCPayload.ts`**: `setTimeout(flush, 0)` batches by event loop tick. When multiple Suspense boundaries resolve in the same tick, all HTML merges into one output. 2. **`handleIncrementalRenderStream.ts`**: The `while` loop processes all NDJSON update chunks synchronously. Each `setProp()` adds to React's `pingedTasks`, but React only calls `setImmediate(performWork)` on the first ping. All props batch into one `performWork` → one `flushCompletedQueues` → one giant chunk. ## Fix **`injectRSCPayload.ts`**: React calls `flushBuffered(destination)` → `destination.flush()` at the end of every `flushCompletedQueues` cycle ([React source](https://github.com/facebook/react/blob/main/packages/react-server/src/ReactServerStreamConfigNode.js#L31-L38), [PR facebook/react#21625](facebook/react#21625)). By adding `.flush()` to `htmlStream`, we flush buffered chunks at exactly the right granularity — per render cycle, not per event loop tick. This also solves the partial HTML tag problem (React's 4KB buffer can split tags mid-attribute) without the over-batching of `setTimeout(0)`. **`handleIncrementalRenderStream.ts`**: `await setImmediate` after each update chunk gives React's `performWork` a chance to run between `setProp()` calls, producing separate `flushCompletedQueues` → `flush()` cycles per Suspense boundary. Both changes are needed together — see the issue for detailed explanation of why each alone is insufficient. ## Verification Test script rendering 4 large Suspense boundaries (>4KB each): ``` Without flush(): 147 chunks, 84% contain partial HTML tags With flush(): 5 flush cycles, 0% partial tags Without flush: [Hero+Fallbacks+Details] → [Reviews] → [Related] → [Recommended] With flush: [Hero+Fallbacks] → [Details] → [Reviews] → [Related] → [Recommended] ``` ## Test plan - [ ] Existing streaming tests pass - [ ] Product page RSC: first wire-format message contains only shell + skeletons (not resolved content) - [ ] Blog page RSC: FCP is faster or equal to SSR (was 28% slower) - [ ] Dashboard RSC: no regression (queries are slow enough that streaming already worked) - [ ] No partial HTML tags in any output chunk (verified by test script) Closes #3194 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#3196) ## Summary - Replace `setTimeout(flush, 0)` in `injectRSCPayload.ts` with React's `destination.flush()` signal for per-render-cycle flushing - Yield to the event loop (`setImmediate`) between NDJSON update chunks in `handleIncrementalRenderStream.ts` so each async prop triggers a separate React render cycle ## Problem The RSC shell HTML + all resolved Suspense boundary content were being merged into a single wire-format message, defeating progressive streaming. On pages with fast DB queries (product, blog, restaurant search), RSC FCP was **worse** than SSR. **Product page before fix:** | Metric | SSR | RSC | |--------|-----|-----| | FCP | 136ms | 370ms (+172%) | | First wire msg | 70KB | 124KB (shell + all resolved + Flight data) | **Product page after fix (with 3ms sleep workaround confirming the diagnosis):** | Metric | SSR | RSC | |--------|-----|-----| | FCP | 140ms | 172ms (+23%) | | First wire msg | 70KB | 38KB (shell + skeletons only) | ## Root Cause Two issues combined: 1. **`injectRSCPayload.ts`**: `setTimeout(flush, 0)` batches by event loop tick. When multiple Suspense boundaries resolve in the same tick, all HTML merges into one output. 2. **`handleIncrementalRenderStream.ts`**: The `while` loop processes all NDJSON update chunks synchronously. Each `setProp()` adds to React's `pingedTasks`, but React only calls `setImmediate(performWork)` on the first ping. All props batch into one `performWork` → one `flushCompletedQueues` → one giant chunk. ## Fix **`injectRSCPayload.ts`**: React calls `flushBuffered(destination)` → `destination.flush()` at the end of every `flushCompletedQueues` cycle ([React source](https://github.com/facebook/react/blob/main/packages/react-server/src/ReactServerStreamConfigNode.js#L31-L38), [PR facebook/react#21625](facebook/react#21625)). By adding `.flush()` to `htmlStream`, we flush buffered chunks at exactly the right granularity — per render cycle, not per event loop tick. This also solves the partial HTML tag problem (React's 4KB buffer can split tags mid-attribute) without the over-batching of `setTimeout(0)`. **`handleIncrementalRenderStream.ts`**: `await setImmediate` after each update chunk gives React's `performWork` a chance to run between `setProp()` calls, producing separate `flushCompletedQueues` → `flush()` cycles per Suspense boundary. Both changes are needed together — see the issue for detailed explanation of why each alone is insufficient. ## Verification Test script rendering 4 large Suspense boundaries (>4KB each): ``` Without flush(): 147 chunks, 84% contain partial HTML tags With flush(): 5 flush cycles, 0% partial tags Without flush: [Hero+Fallbacks+Details] → [Reviews] → [Related] → [Recommended] With flush: [Hero+Fallbacks] → [Details] → [Reviews] → [Related] → [Recommended] ``` ## Test plan - [ ] Existing streaming tests pass - [ ] Product page RSC: first wire-format message contains only shell + skeletons (not resolved content) - [ ] Blog page RSC: FCP is faster or equal to SSR (was 28% slower) - [ ] Dashboard RSC: no regression (queries are slow enough that streaming already worked) - [ ] No partial HTML tags in any output chunk (verified by test script) Closes #3194 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
setTimeout(flush, 0)ininjectRSCPayload.tswith React'sdestination.flush()signal for per-render-cycle flushingsetImmediate) between NDJSON update chunks inhandleIncrementalRenderStream.tsso each async prop triggers a separate React render cycleProblem
The RSC shell HTML + all resolved Suspense boundary content were being merged into a single wire-format message, defeating progressive streaming. On pages with fast DB queries (product, blog, restaurant search), RSC FCP was worse than SSR.
Product page before fix:
Product page after fix (with 3ms sleep workaround confirming the diagnosis):
Root Cause
Two issues combined:
injectRSCPayload.ts:setTimeout(flush, 0)batches by event loop tick. When multiple Suspense boundaries resolve in the same tick, all HTML merges into one output.handleIncrementalRenderStream.ts: Thewhileloop processes all NDJSON update chunks synchronously. EachsetProp()adds to React'spingedTasks, but React only callssetImmediate(performWork)on the first ping. All props batch into oneperformWork→ oneflushCompletedQueues→ one giant chunk.Fix
injectRSCPayload.ts: React callsflushBuffered(destination)→destination.flush()at the end of everyflushCompletedQueuescycle (React source, PR facebook/react#21625). By adding.flush()tohtmlStream, we flush buffered chunks at exactly the right granularity — per render cycle, not per event loop tick. This also solves the partial HTML tag problem (React's 4KB buffer can split tags mid-attribute) without the over-batching ofsetTimeout(0).handleIncrementalRenderStream.ts:await setImmediateafter each update chunk gives React'sperformWorka chance to run betweensetProp()calls, producing separateflushCompletedQueues→flush()cycles per Suspense boundary.Both changes are needed together — see the issue for detailed explanation of why each alone is insufficient.
Verification
Test script rendering 4 large Suspense boundaries (>4KB each):
Test plan
Closes #3194
🤖 Generated with Claude Code