fix(router-core): stream post-</body> render output instead of quarantining it - #8026
fix(router-core): stream post-</body> render output instead of quarantining it#8026ryansolid wants to merge 2 commits into
Conversation
…tining it transformStreamWithRouter assumed the renderer holds <body> open until the render completes (React's shape). Solid's renderToStream emits a complete document and then streams out-of-order boundary fragments after </body>; the transform quarantined all of them in a 64KB-capped tail until render + serialization finished, defeating progressive paint (fragments gated on the slowest query on the page) and killing the connection mid-response on boundaries over 64KB. Hold only the document terminator (</body>[ws]</html>, captured incrementally and case-insensitively) and emit it last; everything after it re-enters the scan-and-inject pipeline so fragments flush immediately and router scripts interleave at safe closing-tag boundaries. Unknown shapes degrade to holding just </body> — app output can no longer overflow a tail buffer, so the 64KB cap and its hard failure are removed. React's shape is byte-identical: nothing follows its terminator, and late serialization scripts still land before </body>. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe SSR stream merger replaces fixed tail buffering with incremental document-terminator matching. It streams post-body output, preserves terminator ordering, delays router HTML when partial tags exist, and adds coverage for large, split, uppercase, out-of-order, and React-shaped streams. ChangesSSR streaming changes
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AppStream
participant ReusableScanner
participant RouterHTML
participant DocumentTerminator
AppStream->>ReusableScanner: process app chunks after the document shell
ReusableScanner->>RouterHTML: flush router HTML at a safe boundary
ReusableScanner->>DocumentTerminator: retain the bounded document terminator
DocumentTerminator->>AppStream: emit the terminator after remaining fragments
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/router-core/src/ssr/transformStreamWithRouter.ts (2)
788-800: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the vestigial
combinedalias.The caller merges
leftoverintochunkStringbefore callingprocessScanChunk, socombinedis now an exact alias. UsechunkStringdirectly to avoid implying a second buffer.♻️ Proposed simplification
- const combined = chunkString - if (combined.length > MAX_LEFTOVER_CHARS) { - noteBarrierMarker(combined) - const flushUpto = combined.length - MAX_LEFTOVER_CHARS - const flushed = combined.slice(0, flushUpto) + if (chunkString.length > MAX_LEFTOVER_CHARS) { + noteBarrierMarker(chunkString) + const flushUpto = chunkString.length - MAX_LEFTOVER_CHARS + const flushed = chunkString.slice(0, flushUpto) writeChunk(flushed) - leftover = combined.slice(flushUpto) + leftover = chunkString.slice(flushUpto) } else { - leftover = combined + leftover = chunkString }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/ssr/transformStreamWithRouter.ts` around lines 788 - 800, Remove the vestigial combined alias in processScanChunk’s no-closing-tag branch and use chunkString directly for the length check, noteBarrierMarker call, slicing, and leftover assignment.
747-802: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd curly braces to the new one-line
ifbodies. The new code in this file uses one-lineifbodies in several places. The coding guidelines require braces for every control statement.
packages/router-core/src/ssr/transformStreamWithRouter.ts#L747-L802: add braces to the guard returns on lines 755, 758, 760, 762, 773, and 776, and toif (excess) processScanChunk(excess)on line 763.packages/router-core/src/ssr/transformStreamWithRouter.ts#L147-L172: add braces to the returns on lines 159, 167, and 169.packages/router-core/src/ssr/transformStreamWithRouter.ts#L730-L739: add braces toif (end === -1) return ''on line 733.packages/router-core/src/ssr/transformStreamWithRouter.ts#L949-L964: add braces toif (!chunkString) continueon line 960.As per coding guidelines: "Always use curly braces for
if,else, loops, and similar control statements. Never write one-line bodies likeif (foo) x = 1."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/ssr/transformStreamWithRouter.ts` around lines 747 - 802, Add curly braces to every single-line control body in transformStreamWithRouter.ts: in processScanChunk, wrap the guard returns and excess recursion; also brace the return guards in the 147-172 section, the end === -1 return in the boundary helper, and the !chunkString continue in the streaming loop. Preserve all existing control flow and behavior.Source: Coding guidelines
packages/router-core/tests/transformStreamWithRouter.test.ts (2)
163-184: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
collectOutputdecodes each chunk independently.
Buffer.from(...).toString('utf8')runs per chunk. A multi-byte UTF-8 sequence split across two chunks would decode as replacement characters. All current test inputs are ASCII, so no test fails today. Use a streamingTextDecoderto make the helper safe for future non-ASCII fixtures.♻️ Proposed streaming decode
function collectOutput(s: ReadableStream<Uint8Array>) { let done = false let received = '' + const decoder = new TextDecoder() const finished = (async () => { const reader = s.getReader() while (true) { const { done: d, value } = await reader.read() - if (d) break - received += Buffer.from( - value.buffer, - value.byteOffset, - value.byteLength, - ).toString('utf8') + if (d) { + break + } + received += decoder.decode(value, { stream: true }) } + received += decoder.decode() done = true })()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/transformStreamWithRouter.test.ts` around lines 163 - 184, Update collectOutput to decode the stream with a streaming TextDecoder across chunk boundaries, passing each Uint8Array chunk through incremental decoding and flushing the decoder after the read loop. Preserve the existing finished, isDone, and text behavior while ensuring split multi-byte UTF-8 sequences decode correctly.
1312-1342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the unknown-shape terminator path.
These two tests cover the split and uppercase terminators. The
-2branch ofmatchDocumentTerminatoris not covered. Two cases are worth asserting, because they define the emitted byte order:
- Content between the tags, for example
</body><div>x</div></html>.- A whitespace run longer than
MAX_TERMINATOR_WHITESPACEbetween</body>and</html>.Both take the degraded path where only
</body>is held, so</body>is emitted after</html>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/tests/transformStreamWithRouter.test.ts` around lines 1312 - 1342, Extend the terminator coverage tests around transformStreamWithRouter with unknown-shape inputs that exercise the matchDocumentTerminator -2 branch: assert content between </body> and </html> is emitted with </body> after </html>, and assert the same ordering when the whitespace run exceeds MAX_TERMINATOR_WHITESPACE. Keep the existing split and uppercase cases unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/router-core/src/ssr/transformStreamWithRouter.ts`:
- Around line 788-800: Remove the vestigial combined alias in processScanChunk’s
no-closing-tag branch and use chunkString directly for the length check,
noteBarrierMarker call, slicing, and leftover assignment.
- Around line 747-802: Add curly braces to every single-line control body in
transformStreamWithRouter.ts: in processScanChunk, wrap the guard returns and
excess recursion; also brace the return guards in the 147-172 section, the end
=== -1 return in the boundary helper, and the !chunkString continue in the
streaming loop. Preserve all existing control flow and behavior.
In `@packages/router-core/tests/transformStreamWithRouter.test.ts`:
- Around line 163-184: Update collectOutput to decode the stream with a
streaming TextDecoder across chunk boundaries, passing each Uint8Array chunk
through incremental decoding and flushing the decoder after the read loop.
Preserve the existing finished, isDone, and text behavior while ensuring split
multi-byte UTF-8 sequences decode correctly.
- Around line 1312-1342: Extend the terminator coverage tests around
transformStreamWithRouter with unknown-shape inputs that exercise the
matchDocumentTerminator -2 branch: assert content between </body> and </html> is
emitted with </body> after </html>, and assert the same ordering when the
whitespace run exceeds MAX_TERMINATOR_WHITESPACE. Keep the existing split and
uppercase cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2c3e619-1605-4063-b6ce-765084ac681d
📒 Files selected for processing (2)
packages/router-core/src/ssr/transformStreamWithRouter.tspackages/router-core/tests/transformStreamWithRouter.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/tests/transformStreamWithRouter.test.ts`:
- Around line 1272-1274: Update the test around the resolveQuery script and pl-2
template markers to first assert that both expected strings are present in
output.text(), then compare their offsets to verify the script precedes the
template.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd7b8fb6-1e83-416e-91f9-844db87a193d
📒 Files selected for processing (1)
packages/router-core/tests/transformStreamWithRouter.test.ts
| expect( | ||
| output.text().indexOf('<script>resolveQuery(2)</script>'), | ||
| ).toBeLessThan(output.text().indexOf('<template id="pl-2">')) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the injected script exists before comparing offsets.
indexOf returns -1 when the script is missing. If the template exists, this assertion still passes. Assert the expected markers are present before checking their order.
Proposed fix
+ const text = output.text()
+ expect(text).toContain('<script>resolveQuery(2)</script>')
+ expect(text).toContain('<template id="pl-2">')
expect(
- output.text().indexOf('<script>resolveQuery(2)</script>'),
+ text.indexOf('<script>resolveQuery(2)</script>'),
- ).toBeLessThan(output.text().indexOf('<template id="pl-2">'))
+ ).toBeLessThan(text.indexOf('<template id="pl-2">'))📝 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.
| expect( | |
| output.text().indexOf('<script>resolveQuery(2)</script>'), | |
| ).toBeLessThan(output.text().indexOf('<template id="pl-2">')) | |
| const text = output.text() | |
| expect(text).toContain('<script>resolveQuery(2)</script>') | |
| expect(text).toContain('<template id="pl-2">') | |
| expect( | |
| text.indexOf('<script>resolveQuery(2)</script>'), | |
| ).toBeLessThan(text.indexOf('<template id="pl-2">')) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/tests/transformStreamWithRouter.test.ts` around lines
1272 - 1274, Update the test around the resolveQuery script and pl-2 template
markers to first assert that both expected strings are present in output.text(),
then compare their offsets to verify the script precedes the template.
|
View your CI Pipeline Execution ↗ for commit c3bd362
☁️ Nx Cloud last updated this comment at |
|
FYI the "Bundle Size", "Labeler", and "PR / Preview" are expected to fail on external forks. "PR / Test" isn't. It looked like an Also I'm still trying to stabilize the codspeed benchmarks. So you can fully ignore the memory benchmarks altogether, and take the CPU benchmarks ("Simulation") with a grain of salt. |
Merging this PR will degrade performance by 23.68%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths not-found (vue) |
329.9 KB | 2,297.5 KB | -85.64% |
| ❌ | Memory | mem server error-paths redirect (solid) |
288.7 KB | 659.4 KB | -56.22% |
| ❌ | Memory | mem server serialization-payload (react) |
3.2 MB | 5.1 MB | -37.44% |
| ❌ | Memory | mem server request-churn (react) |
488.2 KB | 602.7 KB | -19% |
| ❌ | Memory | mem server error-paths redirect (react) |
203.8 KB | 219.6 KB | -7.22% |
| ❌ | Memory | mem server error-paths redirect (vue) |
293 KB | 309 KB | -5.17% |
| ❌ | Memory | mem server server-fn-churn (vue) |
263.2 KB | 276 KB | -4.64% |
| ❌ | Simulation | ssr server-fn during document ssr (react) |
69.3 ms | 71.6 ms | -3.21% |
| ❌ | Simulation | ssr server-fn multipart (solid) |
64 ms | 66.1 ms | -3.13% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
661.7 KB | 411.6 KB | +60.79% |
| ⚡ | Memory | mem client unique-location-churn (solid) |
436.9 KB | 343.8 KB | +27.09% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ryansolid:fix/stream-transform-post-body-passthrough (c3bd362) with main (af8dcb8)
2 participants
fix(router-core): stream post-
</body>render output instead of quarantining it (out-of-order SSR renderers)Problem
transformStreamWithRouterassumes the renderer holds<body>open until the renderstream completes — true for React's
renderToPipeableStream, where</body></html>are the final bytes. Solid's
renderToStreamhas the opposite shape (unchanged sinceSolid 1.x): it flushes a complete document (
…</body></html>) as the shell, thenkeeps streaming out-of-order boundary fragments (
<template>+$df()swap scripts)after
</body>. Browsers reparent trailing content into<body>; that is theprotocol.
The transform treats everything from the first
</body>onward as "tail"(
MergeState.HoldingTail→appendTail) and holds it until render andserialization finish, so its
$_TSRscript injections land before</body>. UnderSolid's shape that quarantines every boundary fragment until the end of the stream.
Two measured consequences (TanStack Start, Solid 2.0 line, production build; fixture:
three queries under separate boundaries settling at 25 ms / ~717 ms / ~1500 ms):
1. No progressive paint — every boundary is gated on the slowest query on the page.
Server byte stream, before the fix (real-Chrome UA):
$_TSR.router=blobshellquery resolution script (data on the wire at ~63 ms)renderOnlyquery scriptslowquery script</body></html>+ all three boundary<template>s + swap scriptsClient side, the
shellboundary's data was in the query cache at ~92 ms, but itsmarkup arrived at ~1538 ms — a ~1.45 s window where the fallback stayed visible with
the data already hydrated. Each boundary's content is delayed to the end of the
entire stream.
2. Hard failure above 64 KB. The tail buffer is capped (
MAX_TAIL_CHARS,64 KB). A streamed boundary rendering ~100 KB of HTML throws
SSR stream tail exceeded maximum bufferserver-side, the connection diesmid-response (
net::ERR_INCOMPLETE_CHUNKED_ENCODING), and the user is left with apermanently stuck loading fallback — the corresponding query hydrated as a pending
promise that never resolves, so the client cannot recover. Any content-heavy streamed
route hits this.
What the transform must guarantee (and why it buffered)
The tail-holding exists for exactly one reason: injected router HTML (the
$_TSRstate blob and streamed seroval script chunks) must land inside
<body>, i.e.before the document terminator. With React's shape that is nearly free — the held
tail is just the final
</body></html>. The bug is that the hold is keyed on firstsight of
</body>rather than on the terminator itself, so a renderer that keepsproducing after the terminator gets its entire remaining output quarantined.
The transform's other invariant — injected HTML only enters the stream at safe
closing-tag boundaries, never splitting app bytes mid-tag — is independent of the
tail hold and is preserved (see below).
Fix
Hold only the document terminator, and keep streaming everything after it:
</body>, the transform captures</body>[whitespace]</html>(case-insensitive, incremental across chunk splits, whitespace bounded at 256
chars) into
pendingTailand emits it last, exactly as before.pipeline (new
MergeState.PostBody): fragments flush immediately, and routerscript injections interleave with them. Scripts landing after
</body>on thewire is precisely Solid's own protocol — the parser reparents them into
<body>,and the relocated terminator means the final document is better-formed than the
renderer's raw output, not worse.
</body>don't look like[whitespace]</html>(unknownrenderer shape), the transform degrades gracefully: it holds only
</body>andstreams the rest. No path buffers unbounded app output anymore.
the scanner is at a safe boundary (no partial trailing tag held in
leftover);otherwise it queues and flushes at the next closing-tag boundary, same as before
</body>. This matters because the bounded-leftoveroverflow path can flushoutput that ends mid-element (e.g. inside
<script>text), where an immediateinjection would corrupt the stream.
Ordering guarantees, before vs. after
$_TSRscripts still always precede the emitted terminator (held tail flushedlast at
tryFinish, after any remaining router HTML). Unchanged.of the quarantine (scripts flushed immediately while all fragments were held).
Now it holds by arrival order — the seroval chunk for a settled query is injected
at the settle event, at or before the moment the renderer flushes that boundary's
fragment, and the e2e capture confirms each data script precedes its fragment on
the wire. Note the client is also order-tolerant here: fragments swap fallback
DOM without needing the data, and hydrated pending promises resolve whenever the
script executes.
The 64 KB cap
MAX_TAIL_CHARSand theSSR stream tail exceeded maximum bufferfailure areremoved, not raised: the held tail is now structurally bounded by the terminator
matcher (≤
</body>+ 256 chars whitespace +</html>). Remaining buffers, audited:pendingTailleftover(split-tag window)pendingRouterHtmlpendingWrites(downstream backpressure)App render output can no longer terminate the response.
Cross-framework safety
</body></html>are the final renderbytes, nothing follows the terminator: the capture completes, pass-through never
carries app bytes, late serialization scripts still flush before the held
terminator. A new test asserts exact output equality for a React-shaped sequence,
and it passes unchanged against the pre-fix transform (verified by stashing the
fix and re-running: the 4 Solid-shape tests fail on the old code, the React
byte-identity test passes).
@tanstack/router-coreunit suite fully green (105 files, 1575 passed,3 pre-existing expected-fails);
@tanstack/react-routerunit suite fully green;@tanstack/solid-routerunit suite fully green.tail overflow errors and runs cleanupasserted the old kill-the-connection behavior for >64 KB post-
</body>content.It is now
large post-</body> content streams through instead of erroring, whichasserts the same input survives, with the rationale in a comment.
Tests added (
packages/router-core/tests/transformStreamWithRouter.test.ts)New describe block
out-of-order (post-</body>) streaming, with the measuredtimeline as the repro description:
released before the stream ends (mid-stream assertion via a continuous
collector), that an injected data script between fragments flushes immediately and
precedes the next fragment, and that the document still ends with exactly one
</body></html>.connection survives to a clean close.
</body>…</ht…ml>) and uppercase</BODY>\n</HTML>variants — captured and emitted last, byte-exact output asserted.boundary (the stream-corruption guard).
End-to-end verification (instrumented Start app, Solid 2.0 line)
Same fixture and probes as the original investigation; the fix was applied to the
installed
router-coredist as a minimal surgical patch (identical logic) so themeasured delta is exactly this change. Baseline re-measured on the same machine
immediately before.
Server byte stream (fragment arrival):
shell(25 ms)renderOnly(~717 ms)slow(~1500 ms)</body></html>Each data script (chunks 1, 5, 9) precedes its boundary's fragment on the wire.
Client timeline (headless Chrome, real-Chrome UA):
shelldata in cacheshellboundary content renderedrenderOnlyboundary content renderedslowboundary content renderedThe ~1.45 s data-in-cache→content-visible window for the fast boundary is gone; each
boundary now paints at its own data-arrival time instead of the slowest query's.
Client
queryFnexecutions remain 0 (all DOM tokens are the SSR tokens), consoleclean apart from a favicon 404.
64 KB case (
/big, ~100 KB boundary): before — server threwSSR stream tail exceeded maximum buffer, connection died at ~98 KB read(
ERR_INCOMPLETE_CHUNKED_ENCODING), fallback stuck forever. After — responsecompletes cleanly: 394,564 bytes, all 1505
<li>rows, terminates with</body></html>, zero server errors.Notes for reviewers
findHtmlBoundary's</body>-detection is unchanged; inPostBodya literal</body>in content is treated as an ordinary closing-tag boundary rather than asecond terminator.
reserveStreamFastPath) is untouched.state as MergeState) because thestate transitions moved into helper closures and TypeScript's control-flow
narrowing otherwise pins
stateto its initial value.Summary by CodeRabbit
New Features
Bug Fixes