feat: RawStream #6231
feat: RawStream #6231
Conversation
📝 WalkthroughWalkthroughAdds end-to-end RawStream support: a RawStream class and seroval plugins, framed multiplexed server encoder, client frame decoder and fetcher integration, E2E helpers/routes/UI, and unit + E2E tests for serialization, framing, decoding, multiplexing, hints, and edge cases. Changes
Sequence Diagram(s)mermaid rect rgba(0,128,96,0.06) rect rgba(0,64,192,0.06) note over Decoder,App: END/ERROR frames close or error per-stream readers Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
|
View your CI Pipeline Execution ↗ for commit 296827b
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
packages/router-core/src/ssr/serializer/RawStream.ts (2)
94-108: Consider adding brief comments explaining the empty catch blocks.The empty catch blocks are intentional for handling closed streams, but a brief inline comment would clarify intent for maintainers.
🔎 Suggested improvement
next(base64: string) { try { controller.enqueue(base64ToUint8Array(base64)) - } catch { - // Stream may be closed + } catch { + // Intentionally ignored: stream may already be closed by consumer } },
302-314: Sync parse returns empty stream - document the limitation.The sync parse method returns an empty stream since synchronous stream parsing isn't supported. Consider adding a warning log or throwing a more descriptive error if sync mode is attempted in production, as this could lead to silent data loss.
packages/router-core/tests/RawStream.test.ts (1)
201-232: Remove debug console.log statements from tests.The
console.logstatements were likely added during development/debugging. They should be removed to keep test output clean.🔎 Suggested cleanup
const serialized = await toCrossJSONAsync(data, { refs, plugins: [rpcPlugin], }) - console.log('Serialized CrossJSON:', JSON.stringify(serialized, null, 2)) - // Verify we collected the stream expect(collectedStreams.size).toBe(1) const streamId = Array.from(collectedStreams.keys())[0]! - console.log('Collected stream ID:', streamId) // Create getOrCreateStream function const getOrCreateStream = (id: number) => { @@ -218,13 +215,6 @@ const deserialized = fromCrossJSON(serialized, { refs: new Map(), plugins: [deserializePlugin], }) as any - console.log('Deserialized result:', deserialized) - console.log('rawData type:', typeof deserialized.rawData) - console.log( - 'rawData is ReadableStream:', - deserialized.rawData instanceof ReadableStream, - ) - expect(deserialized.message).toBe('test') expect(deserialized.rawData).toBe(testStream) })packages/start-client-core/tests/frame-decoder.test.ts (1)
8-40: Consider extracting shared frame encoding test utilities.The frame encoding helpers (
encodeFrame,encodeJSONFrame,encodeChunkFrame,encodeEndFrame,encodeErrorFrame) are likely duplicated inpackages/start-server-core/tests/frame-protocol.test.ts. Consider extracting these to a shared test utilities file to ensure consistency and reduce maintenance burden.🔎 Potential location for shared utilities
Create a shared test utilities file (e.g.,
packages/start-test-utils/frame-helpers.ts) and import from both test files:// In both test files: import { encodeFrame, encodeJSONFrame, encodeChunkFrame, encodeEndFrame, encodeErrorFrame } from '@tanstack/start-test-utils/frame-helpers'e2e/react-start/basic/src/routes/raw-stream/ssr-single.tsx (1)
1-1: Remove unused import.The
Awaitimport from@tanstack/react-routeris not used in this file.🔎 Proposed fix
-import { createFileRoute, Await } from '@tanstack/react-router' +import { createFileRoute } from '@tanstack/react-router'e2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx (1)
53-59: Consider adding error handling for stream consumption.The stream consumption in
useEffectdoesn't handle potential errors. If the stream fails, the component will silently remain in the loading state.🔎 Proposed fix to add error handling
React.useEffect(() => { const consumeStream = createStreamConsumer() - consumeStream(rawData).then((content) => { - setStreamContent(content) - setIsConsuming(false) - }) + consumeStream(rawData) + .then((content) => { + setStreamContent(content) + setIsConsuming(false) + }) + .catch((err) => { + console.error('Stream consumption error:', err) + setIsConsuming(false) + }) }, [rawData])packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
352-435: Significant code duplication withprocessServerFnResponse.
processFramedResponse(lines 357-435) is nearly identical toprocessServerFnResponse(lines 269-351). The only differences are the input type (ReadableStream<string>vsResponse) and the initial setup. Consider extracting the shared NDJSON parsing logic into a common helper.🔎 Proposed refactor to extract shared logic
// Extract the common NDJSON line processing logic async function processNDJSONStream({ reader, onMessage, onError, }: { reader: ReadableStreamDefaultReader<string> onMessage: (msg: any) => any onError?: (msg: string, error?: any) => void }) { let buffer = '' let firstRead = false let firstObject: any while (!firstRead) { const { value, done } = await reader.read() if (value) buffer += value if (buffer.length === 0 && done) { throw new Error('Stream ended before first object') } if (buffer.endsWith('\n')) { const lines = buffer.split('\n').filter(Boolean) const firstLine = lines[0] if (!firstLine) throw new Error('No JSON line in the first chunk') firstObject = JSON.parse(firstLine) firstRead = true buffer = lines.slice(1).join('\n') } else { const newlineIndex = buffer.indexOf('\n') if (newlineIndex >= 0) { const line = buffer.slice(0, newlineIndex).trim() buffer = buffer.slice(newlineIndex + 1) if (line.length > 0) { firstObject = JSON.parse(line) firstRead = true } } } } // Process rest asynchronously ;(async () => { try { while (true) { const { value, done } = await reader.read() if (value) buffer += value const lastNewline = buffer.lastIndexOf('\n') if (lastNewline >= 0) { const chunk = buffer.slice(0, lastNewline) buffer = buffer.slice(lastNewline + 1) const lines = chunk.split('\n').filter(Boolean) for (const line of lines) { try { onMessage(JSON.parse(line)) } catch (e) { onError?.(`Invalid JSON line: ${line}`, e) } } } if (done) break } } catch (err) { onError?.('Stream processing error:', err) } })() return onMessage(firstObject) }packages/start-client-core/src/constants.ts (1)
69-69: Unnecessary empty export statement.The
export {}at the end is redundant since the file already has multiple exports. This appears to be a leftover artifact.🔎 Proposed fix
-export {}e2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx (1)
31-35: Module-level variable mutation is safe but could be clearer.The
offsetvariable is reused at lines 31-35 and 44-48 during module initialization. While this works correctly since it's synchronous initialization code, using a separate variable or an IIFE would make the intent clearer.🔎 Alternative using IIFE for clarity
-const TEXT_EXPECTED = new Uint8Array( - TEXT_CHUNKS.reduce((acc, c) => acc + c.length, 0), -) -let offset = 0 -for (const chunk of TEXT_CHUNKS) { - TEXT_EXPECTED.set(chunk, offset) - offset += chunk.length -} +const TEXT_EXPECTED = (() => { + const result = new Uint8Array( + TEXT_CHUNKS.reduce((acc, c) => acc + c.length, 0), + ) + let offset = 0 + for (const chunk of TEXT_CHUNKS) { + result.set(chunk, offset) + offset += chunk.length + } + return result +})()e2e/react-start/basic/src/routes/raw-stream/client-call.tsx (1)
43-43: Consider movingcreateStreamConsumer()insiderunTestor usinguseMemo.The
consumeStreamfunction is created once when the component mounts. SincecreateStreamConsumerreturns a new function with its ownTextDecoder, this should work correctly. However, if the decoder needed to maintain state across calls, this pattern could cause issues. For clarity, consider either:
- Moving the creation inside
runTest(creates fresh decoder per test)- Using
React.useMemoto make the intent explicitThis is a minor point since the current implementation works correctly.
packages/start-server-core/src/frame-protocol.ts (1)
101-101: Unusual type assertion on boolean.The
as booleancast on a boolean literal is unusual. This might be to prevent TypeScript from narrowing the type, but a simpler approach would be to just declare it aslet cancelled = false.🔎 Suggested simplification
- let cancelled = false as boolean + let cancelled = falseIf the
as booleanis intentional to prevent const narrowing issues in closures, consider adding a comment explaining why.e2e/react-start/basic/src/raw-stream-fns.ts (1)
360-408: Consider extracting the stream unwrapping logic.The pattern for extracting the actual stream from
RawStream | ReadableStream<Uint8Array>is duplicated acrosscreateStreamConsumer,consumeBinaryStream, andcollectBytes.🔎 Optional: Extract shared helper
+// Helper to unwrap RawStream to ReadableStream +function unwrapStream( + stream: ReadableStream<Uint8Array> | RawStream, +): ReadableStream<Uint8Array> { + return stream instanceof RawStream + ? stream.stream + : (stream as ReadableStream<Uint8Array>) +} + export async function consumeBinaryStream( stream: ReadableStream<Uint8Array> | RawStream, ): Promise<number> { - // Handle both RawStream (from type system) and ReadableStream (runtime) - const actualStream = - stream instanceof RawStream - ? stream.stream - : (stream as ReadableStream<Uint8Array>) + const actualStream = unwrapStream(stream) const reader = actualStream.getReader()This is optional since the duplication is in a test utility file where explicitness can aid readability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
e2e/react-start/basic/src/raw-stream-fns.tse2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/__root.tsxe2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-single.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/tests/raw-stream.spec.tspackages/router-core/src/index.tspackages/router-core/src/ssr/serializer/RawStream.tspackages/router-core/src/ssr/serializer/seroval-plugins.tspackages/router-core/src/ssr/serializer/transformer.tspackages/router-core/tests/RawStream.test.tspackages/start-client-core/src/client-rpc/frame-decoder.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-client-core/src/constants.tspackages/start-client-core/src/index.tsxpackages/start-client-core/tests/frame-decoder.test.tspackages/start-server-core/src/frame-protocol.tspackages/start-server-core/src/server-functions-handler.tspackages/start-server-core/tests/frame-protocol.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/ssr/serializer/transformer.tse2e/react-start/basic/tests/raw-stream.spec.tse2e/react-start/basic/src/routes/__root.tsxpackages/router-core/src/ssr/serializer/seroval-plugins.tse2e/react-start/basic/src/routes/raw-stream/index.tsxpackages/start-client-core/src/index.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxpackages/router-core/src/index.tse2e/react-start/basic/src/routes/raw-stream.tsxpackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/tests/frame-protocol.test.tse2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-single.tsxpackages/start-client-core/src/constants.tspackages/start-client-core/src/client-rpc/frame-decoder.tse2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxpackages/start-server-core/src/server-functions-handler.tspackages/start-server-core/src/frame-protocol.tspackages/router-core/tests/RawStream.test.tspackages/start-client-core/tests/frame-decoder.test.tspackages/router-core/src/ssr/serializer/RawStream.tse2e/react-start/basic/src/raw-stream-fns.tse2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/ssr/serializer/transformer.tse2e/react-start/basic/tests/raw-stream.spec.tse2e/react-start/basic/src/routes/__root.tsxpackages/router-core/src/ssr/serializer/seroval-plugins.tse2e/react-start/basic/src/routes/raw-stream/index.tsxpackages/start-client-core/src/index.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxpackages/router-core/src/index.tse2e/react-start/basic/src/routes/raw-stream.tsxpackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/tests/frame-protocol.test.tse2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-single.tsxpackages/start-client-core/src/constants.tspackages/start-client-core/src/client-rpc/frame-decoder.tse2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxpackages/start-server-core/src/server-functions-handler.tspackages/start-server-core/src/frame-protocol.tspackages/router-core/tests/RawStream.test.tspackages/start-client-core/tests/frame-decoder.test.tspackages/router-core/src/ssr/serializer/RawStream.tse2e/react-start/basic/src/raw-stream-fns.tse2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx
🧠 Learnings (9)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-single.tsxe2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxpackages/router-core/tests/RawStream.test.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routeTree.gen.tspackages/start-server-core/src/server-functions-handler.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxpackages/start-client-core/src/index.tsxe2e/react-start/basic/src/routes/raw-stream.tsx
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Use file-based routing in `src/routes/` directories or code-based routing with route definitions
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/index.tsxe2e/react-start/basic/src/routes/raw-stream.tsx
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream.tsxe2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-12-24T22:47:44.320Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6211
File: e2e/react-start/i18n-paraglide/src/server.ts:6-6
Timestamp: 2025-12-24T22:47:44.320Z
Learning: In TanStack Router projects using `inlang/paraglide-js`, the callback passed to `paraglideMiddleware` should use `() => handler.fetch(req)` (referencing the outer `req`) instead of `({ request }) => handler.fetch(request)`. This is intentional because the router needs the untouched URL to perform its own rewrite logic with `deLocalizeUrl`/`localizeUrl`. The middleware's processed request would delocalize the URL and interfere with the router's rewrite handling.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/src/server-functions-handler.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
e2e/react-start/basic/src/routeTree.gen.ts
🧬 Code graph analysis (10)
packages/router-core/src/ssr/serializer/transformer.ts (3)
packages/router-core/src/index.ts (1)
RawStream(436-436)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)packages/start-client-core/src/index.tsx (1)
RawStream(5-5)
e2e/react-start/basic/tests/raw-stream.spec.ts (1)
packages/router-core/src/ssr/serializer/RawStream.ts (5)
test(236-238)test(264-266)test(298-300)test(388-390)test(433-436)
packages/router-core/src/ssr/serializer/seroval-plugins.ts (3)
packages/router-core/src/index.ts (2)
defaultSerovalPlugins(433-433)RawStreamSSRPlugin(437-437)packages/router-core/src/ssr/serializer/ShallowErrorPlugin.ts (1)
ShallowErrorPlugin(12-43)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStreamSSRPlugin(294-366)
e2e/react-start/basic/src/routes/raw-stream/client-call.tsx (1)
e2e/react-start/basic/src/raw-stream-fns.ts (15)
createStreamConsumer(335-358)consumeBinaryStream(360-379)collectBytes(382-408)compareBytes(411-444)TEST7_EXPECTED(50-50)TEST8_EXPECTED(57-57)TEST9_EXPECTED(65-65)TEST10_EXPECTED(69-69)TEST11_EXPECTED(75-75)TEST12_STREAM_A_EXPECTED(245-247)TEST12_STREAM_B_EXPECTED(248-250)TEST13_EXPECTED(262-264)TEST14_STREAM_A_EXPECTED(285-287)TEST14_STREAM_B_EXPECTED(288-290)TEST14_STREAM_C_EXPECTED(291-293)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (3)
packages/start-client-core/src/constants.ts (2)
TSS_CONTENT_TYPE_FRAMED(12-12)validateFramedProtocolVersion(56-68)packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
createFrameDecoder(33-237)packages/router-core/src/index.ts (1)
createRawStreamDeserializePlugin(439-439)
e2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx (3)
packages/router-core/src/ssr/serializer/RawStream.ts (5)
stream(246-248)stream(274-276)stream(330-344)stream(401-404)RawStream(36-45)packages/router-core/src/ssr/serializer/transformer.ts (2)
stream(173-175)stream(207-209)e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsx (1)
Route(114-149)
e2e/react-start/basic/src/routes/raw-stream/ssr-single.tsx (2)
e2e/react-start/basic/src/raw-stream-fns.ts (2)
encode(41-41)createStreamConsumer(335-358)packages/router-core/src/ssr/serializer/RawStream.ts (5)
stream(246-248)stream(274-276)stream(330-344)stream(401-404)RawStream(36-45)
packages/start-server-core/src/server-functions-handler.ts (5)
packages/router-core/src/index.ts (1)
createRawStreamRPCPlugin(438-438)packages/router-core/src/ssr/serializer/RawStream.ts (5)
createRawStreamRPCPlugin(382-419)stream(246-248)stream(274-276)stream(330-344)stream(401-404)packages/start-server-core/src/frame-protocol.ts (2)
createMultiplexedStream(94-186)TSS_CONTENT_TYPE_FRAMED_VERSIONED(18-18)packages/start-client-core/src/constants.ts (2)
TSS_CONTENT_TYPE_FRAMED_VERSIONED(37-37)X_TSS_SERIALIZED(7-7)packages/start-client-core/src/index.tsx (2)
TSS_CONTENT_TYPE_FRAMED_VERSIONED(87-87)X_TSS_SERIALIZED(91-91)
packages/router-core/tests/RawStream.test.ts (1)
packages/router-core/src/ssr/serializer/RawStream.ts (7)
stream(246-248)stream(274-276)stream(330-344)stream(401-404)RawStream(36-45)createRawStreamRPCPlugin(382-419)createRawStreamDeserializePlugin(427-449)
e2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx (3)
e2e/react-start/basic/src/raw-stream-fns.ts (2)
encode(41-41)createStreamConsumer(335-358)e2e/react-start/basic/src/routes/raw-stream/ssr-single.tsx (1)
Route(27-40)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (37)
packages/router-core/src/ssr/serializer/RawStream.ts (4)
36-45: LGTM! Clean marker class implementation.The
RawStreamclass correctly wraps aReadableStream<Uint8Array>with an encoding hint, providing a clear API for distinguishing binary vs text-optimized serialization.
57-79: LGTM! Robust base64 conversion with stack overflow protection.The chunked approach in
uint8ArrayToBase64correctly handles large byte arrays by processing in 32KB segments, and the|| 1fallback properly handles empty arrays.
382-419: LGTM! Clean separation of server and client responsibilities.The RPC plugin correctly registers streams during serialization and outputs a JSON placeholder with the stream ID. The error in
deserializeprovides clear guidance about usingcreateRawStreamDeserializePluginon the client.
427-449: LGTM! Minimal deserialize-only plugin.The plugin correctly returns
falsefromtest()since the client never serializes RawStreams, and properly delegates stream reconstruction to the provided callback.packages/router-core/src/ssr/serializer/transformer.ts (1)
11-11: LGTM! Type extensions for RawStream serialization.Adding
RawStreamandUint8ArraytoDefaultSerializablecorrectly extends the type system to recognize these types as natively serializable, enabling type-safe usage throughout the router.Also applies to: 25-26
e2e/react-start/basic/src/routes/__root.tsx (1)
176-183: LGTM! Navigation entry for Raw Stream tests.The new link follows the established pattern for navigation items in the root document and correctly uses
activePropsfor consistent styling.packages/router-core/src/ssr/serializer/seroval-plugins.ts (1)
6-11: LGTM! Correct plugin ordering with clear documentation.The comment on line 8 clearly explains why
RawStreamSSRPluginmust precedeReadableStreamPlugin- sinceRawStreamwraps aReadableStream, the more specific plugin must match first to prevent incorrect serialization.e2e/react-start/basic/src/routes/raw-stream/index.tsx (1)
1-63: LGTM! Well-structured index page for RawStream tests.The component provides clear navigation to all test sub-routes with descriptive labels. The file-based route definition follows established patterns.
packages/router-core/src/index.ts (1)
435-446: LGTM! Complete public API exports for RawStream feature.All necessary values and types are exported, following the established pattern of separating value exports from type exports. This provides a clean public API surface for consumers.
e2e/react-start/basic/src/routes/raw-stream.tsx (1)
1-65: LGTM! Clean layout route with navigation.The layout route properly uses
Outletfor nested route rendering and provides consistent navigation with active state highlighting. The structure follows TanStack Router conventions.packages/router-core/tests/RawStream.test.ts (3)
9-45: LGTM! Comprehensive class tests.Good coverage of the
RawStreamconstructor behavior including default hint, explicit binary hint, and text hint options.
47-117: LGTM! Thorough RPC plugin tests.Tests correctly verify the callback invocation, serialization format (including the
tss/RawStreamtag), and unique ID assignment for multiple streams.
174-237: LGTM! Valuable round-trip integration test.The round-trip test validates the complete serialize/deserialize flow using both plugins together, which is essential for verifying the feature works end-to-end.
packages/start-client-core/src/index.tsx (1)
5-6: LGTM! Public API additions for RawStream support.The new exports for RawStream, OnRawStreamCallback, and framing protocol constants are appropriate additions to the public API surface for enabling binary streaming functionality.
Also applies to: 86-96
packages/start-server-core/src/server-functions-handler.ts (1)
173-267: LGTM! Clean RawStream integration with proper fallback handling.The implementation correctly:
- Collects raw streams via the RPC plugin callback
- Short-circuits to plain JSON when done and no raw streams exist
- Creates multiplexed framed streams when raw streams are present
- Maintains the existing NDJSON streaming path as a fallback
The plugin ordering (RawStreamRPCPlugin before default SSR plugins) ensures RawStream instances are intercepted early in the serialization process.
packages/start-server-core/tests/frame-protocol.test.ts (1)
1-338: LGTM! Comprehensive test coverage for frame protocol.The test suite thoroughly validates:
- Individual frame encoding functions (JSON, CHUNK, END, ERROR)
- Multiplexed stream creation with various scenarios
- Error handling and cancellation behavior
- Interleaving of multiple concurrent streams
e2e/react-start/basic/tests/raw-stream.spec.ts (1)
1-615: LGTM! Comprehensive end-to-end test coverage.This test suite provides excellent coverage of RawStream functionality across:
- Client RPC calls with various streaming scenarios
- SSR loader streaming with direct and client-side navigation
- Text and binary hint handling
- Multiplexed concurrent streams
- Cross-navigation compatibility
The use of hydration waits and appropriate timeouts ensures reliable test execution.
e2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx (1)
27-46: LGTM!The route definition correctly combines immediate data, deferred promises, and RawStream to test the mixed SSR streaming scenario. The loader returns appropriate test data with proper timing delays for verification.
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (2)
191-218: LGTM!The framed response handling correctly:
- Validates protocol version compatibility
- Checks for response body presence
- Creates the frame decoder and raw stream deserialize plugin
- Wires up the deserialization pipeline with proper plugin ordering
66-69: LGTM!The accept header correctly prioritizes the framed content type for RawStream support while maintaining backward compatibility with NDJSON and JSON responses.
packages/start-client-core/src/constants.ts (1)
11-68: LGTM!The framing protocol constants and utilities are well-defined:
FrameTypeenum with properas consttyping- Header size correctly matches the frame format specification
- Protocol version validation provides clear error messages for version mismatches
- Version parsing regex correctly handles the
v=<number>formatpackages/start-client-core/src/client-rpc/frame-decoder.ts (2)
33-237: LGTM!The frame decoder implementation is well-designed with several performance optimizations:
- Buffer list avoids O(n²) concatenation
- Manual byte reading avoids DataView allocation per frame
- Proper unsigned 32-bit handling with
>>> 0- Lazy stream creation via
getOrCreateStream- Proper cleanup of stream controllers while preserving streams for consumption
- Robust error propagation to all active streams
166-198: Potential edge case: CHUNK frames after END/ERROR could recreate streams.After an END or ERROR frame, the stream controller is deleted but the stream remains in the
streamsmap. However, if a malformed server sends a CHUNK frame after END,ensureController(line 168) will callgetOrCreateStreamwhich only checksstreams.get(id). Since the stream still exists, it won't recreate it, butstreamControllers.get(id)will returnundefined(since it was deleted), so the chunk will be silently dropped.This behavior is actually safe (chunks are dropped after stream closure), but you might want to add a comment clarifying this is intentional behavior for protocol violations.
e2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx (1)
95-119: LGTM!The route correctly tests RawStream with the
hint: 'binary'option for both text and pure binary data. The loader returns appropriate test data for verifying binary hint behavior.e2e/react-start/basic/src/routes/raw-stream/client-call.tsx (1)
35-411: LGTM!Comprehensive client-side test harness for RawStream RPC functionality covering:
- Single and multiple streams
- JSON/raw stream ordering scenarios
- Large binary data handling
- Mixed streaming with immediate, deferred, and raw data
- Hint parameter variations (text/binary)
- Multiplexing scenarios (interleaved, burst-pause-burst, concurrent streams)
The test structure with individual buttons and result displays provides good isolation and visibility for e2e testing.
packages/start-server-core/src/frame-protocol.ts (1)
94-185: LGTM!The multiplexed stream implementation is well-designed:
- Proper active pump tracking for completion detection
- Fatal error handling for JSON stream vs non-fatal ERROR frames for raw streams
- Graceful cancellation with flag-based pump termination
- Concurrent pumping with proper reader lock cleanup
- Good comment explaining why
reader.cancel()isn't called in the cancel handlere2e/react-start/basic/src/routeTree.gen.ts (1)
1-1204: Skipping review of auto-generated file.This file is auto-generated by TanStack Router and should not be manually modified. The changes correctly reflect the new raw-stream routes added in the PR.
e2e/react-start/basic/src/raw-stream-fns.ts (10)
4-17: LGTM!The
createDelayedStreamhelper correctly implements the ReadableStream pattern with async start for test fixtures. The logic is clean and appropriate for E2E test scenarios.
43-76: LGTM!The test data exports comprehensively cover various encoding scenarios (pure text, pure binary, mixed content). The use of
concatBytesbefore its declaration works correctly due to JavaScript function hoisting.
78-87: LGTM!The
concatByteshelper is correctly implemented with proper pre-allocation and offset tracking. This is the standard efficient pattern for byte array concatenation.
89-147: LGTM!Tests 1-4 cover essential streaming scenarios: single stream, multiple streams, JSON-first completion, and raw-first completion. The implementations correctly use the
createServerFn().handler()pattern andRawStreamwrapper.
149-180: LGTM!Tests 5-6 appropriately test large binary data (3KB across 3 chunks) and mixed streaming with deferred promises. The binary test pattern using
i % 256creates a predictable, verifiable byte sequence.
182-225: LGTM!The hint test functions (Tests 7-11) provide excellent coverage of the encoding hint system, testing various combinations of text/binary hints with text/binary/mixed data content. The naming convention is clear and self-documenting.
227-293: LGTM!The multiplexing test data (Tests 12-14) is well-designed to verify correct interleaving behavior under various timing scenarios: two-stream interleaving, burst-pause-burst patterns, and three concurrent streams with different speeds. The timing patterns will effectively test the multiplexing implementation.
295-329: LGTM!The multiplexing server functions correctly implement the test scenarios using
createVariableDelayStreamwith the predefined test data. The return object property names (streamA/streamB,fast/slow/burst) are descriptive and align with the test objectives.
335-358: LGTM!The
createStreamConsumerfactory correctly handles bothRawStreamandReadableStream<Uint8Array>types. UsingTextDecoder.decode(value, { stream: true })is the right approach to handle multi-byte UTF-8 characters that may be split across chunks.
410-444: LGTM!The
compareBytesfunction provides useful diagnostic information for test assertions, including the exact mismatch index and both array lengths. The distinction between-1(length mismatch) and a non-negative index (byte mismatch) is clear and documented.
| try { | ||
| // Try UTF-8 decode first | ||
| const text = decoder.decode(value, { stream: true }) | ||
| if (text.length > 0) { | ||
| stream.next(text) | ||
| } | ||
| } catch { | ||
| // UTF-8 decode failed, fallback to base64 | ||
| stream.next({ $b64: uint8ArrayToBase64(value) }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/router-core/src/ssr/serializer/RawStream.ts | head -250 | tail -100Repository: TanStack/router
Length of output: 3228
🏁 Script executed:
# Search for TextDecoder initialization and usage in the file
rg -n "TextDecoder\|decoder" packages/router-core/src/ssr/serializer/RawStream.ts -B2 -A2Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there are any tests or documentation about decoder state handling
find . -type f -name "*.ts" -o -name "*.js" | xargs rg "decoder.*state\|fatal.*true" 2>/dev/null | head -20Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Search for any error handling or state management documentation
web_search "TextDecoder fatal mode state after error throw exception"Repository: TanStack/router
Length of output: 109
🏁 Script executed:
# Check if there are tests for toTextStream function
fd -e test.ts -e test.tsx -e spec.ts | xargs rg -l "toTextStream\|RawStream" 2>/dev/nullRepository: TanStack/router
Length of output: 43
🏁 Script executed:
# Look for any comments or documentation about the decoder state issue
rg -n "decoder.*state\|partial.*byte\|accumulated\|stream.*true" packages/router-core/src/ssr/serializer/ -B2 -A2Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check the entire RawStream.ts file to see full context
wc -l packages/router-core/src/ssr/serializer/RawStream.tsRepository: TanStack/router
Length of output: 115
🏁 Script executed:
# Read the full RawStream.ts file to understand the complete context
cat packages/router-core/src/ssr/serializer/RawStream.tsRepository: TanStack/router
Length of output: 13142
🌐 Web query:
TextDecoder fatal mode state persistence after exception JavaScript MDN
💡 Result:
Short answer: In fatal mode a decoding error causes a TypeError to be thrown, and the decoder's internal state is not automatically reset by that exception — the TextDecoder keeps its decoder/stream state (including any buffered partial bytes when using stream:true) according to the Encoding Standard. See the spec and MDN for the decode/error behavior. [1][2]
Sources
- WHATWG Encoding Standard — TextDecoder.decode() (describe streaming, decoder state and that a fatal error causes an error to be thrown). [1]
- MDN TextDecoder: fatal / decode pages. [2]
TextDecoder state persists after base64 fallback, causing subsequent UTF-8 chunks to fail.
When UTF-8 decode fails with fatal: true and the code falls back to base64 (line 217), the TextDecoder instance retains its internal buffer containing any partial UTF-8 bytes from the failed chunk. This means subsequent chunks—even valid UTF-8—will attempt to decode the buffered invalid bytes combined with new data, causing those chunks to fail as well.
The decoder should be recreated after a fallback, or UTF-8 decoding should be abandoned for the remainder of the stream once a fatal error occurs. Currently, the single decoder instance continues to attempt UTF-8 decoding with corrupted internal state.
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/serializer/RawStream.ts around lines 209 to 218,
the TextDecoder is left in a poisoned state after a fatal UTF-8 decode and then
a base64 fallback is emitted; recreate the decoder (or stop attempting UTF-8
decoding for the rest of the stream) immediately after catching the decode error
so subsequent chunks are not tainted by the decoder's internal buffer.
Concretely: inside the catch, after emitting the base64 object, reset the
decoder variable to a new TextDecoder instance (or flip a flag to skip future
UTF-8 decode attempts) so that future chunks are decoded cleanly.
There was a problem hiding this comment.
Finding: TextDecoder poisoning after fatal UTF-8 decode error
Location: packages/router-core/src/ssr/serializer/RawStream.ts lines 209-218
Recommendation: Reset the TextDecoder after catching a decode error to prevent subsequent chunks from being tainted by the decoder's internal buffer.
Investigation Results:
I investigated this concern by testing the actual behavior of TextDecoder with fatal: true in Node.js. The hypothesis was that after a fatal decode error, the decoder's internal buffer would retain partial multi-byte sequences that would corrupt subsequent decodes.
Test scenarios executed:
- Valid → Invalid → Valid UTF-8 chunks
- Partial multi-byte sequence → Error → New valid sequence
- Incomplete emoji (4-byte) → Invalid continuation → Complete new sequence
Findings:
In all test cases, the Node.js TextDecoder implementation automatically clears its internal buffer after a fatal error. Subsequent valid UTF-8 chunks decode correctly without needing to recreate the decoder.
Example test:
const decoder = new TextDecoder('utf-8', { fatal: true });
decoder.decode(new Uint8Array([0xF0, 0x9F]), { stream: true }); // partial emoji
try { decoder.decode(new Uint8Array([0x41]), { stream: true }); } catch(e) {} // error
const result = decoder.decode(new Uint8Array([0xC4, 0x80]), { stream: true }); // Ā
console.log(result); // Output: "Ā" (correctly decoded, buffer was cleared)
Conclusion:
The WHATWG TextDecoder spec behavior in modern runtimes (Node.js, browsers) clears internal state after fatal errors. The suggested fix would add unnecessary overhead (creating a new TextDecoder on every error) without providing any practical benefit.
Recommendation: No change needed. The current implementation is correct for all tested runtimes.
There was a problem hiding this comment.
@schiller-manuel, thank you for the thorough investigation! Your empirical testing clearly demonstrates that the concern about TextDecoder state persistence was based on an incorrect understanding of how TextDecoder actually behaves in modern runtimes.
Your test results confirm that the WHATWG TextDecoder implementation automatically clears its internal buffer after a fatal error, making the suggested fix unnecessary. The current implementation in toTextStream() is correct as-is—the decoder can safely continue to be used for subsequent chunks after catching a decode error and falling back to base64.
I appreciate you taking the time to validate the actual runtime behavior rather than just accepting the theoretical concern. The code is working as intended.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
e2e/react-start/basic/src/routes/raw-stream/client-call.tsx (2)
42-42: Consider using a more specific type instead ofany.The
resultsstate is typed asRecord<string, any>, which bypasses TypeScript's type safety. While this is common in test code, consider defining a union type of possible result shapes or usingunknownwith type guards.🔎 Suggested type improvement
+type TestResult = + | { error: string } + | Record<string, unknown> + -const [results, setResults] = React.useState<Record<string, any>>({}) +const [results, setResults] = React.useState<Record<string, TestResult>>({})Note: Using
unknowninstead ofanywould require type guards when accessing result properties, but provides better type safety.
47-65: Consider stronger typing for the test runner.The
runTestfunction usesanyfor both the function return type and the processor result. While acceptable for test code, usingunknownwould provide better type safety without significantly increasing complexity.🔎 Suggested type improvement
async function runTest( testName: string, - fn: () => Promise<any>, - processor: (result: any) => Promise<any>, + fn: () => Promise<unknown>, + processor: (result: unknown) => Promise<unknown>, ) { setLoading((prev) => ({ ...prev, [testName]: true })) try { const result = await fn() const processed = await processor(result) setResults((prev) => ({ ...prev, [testName]: processed })) } catch (error) { setResults((prev) => ({ ...prev, [testName]: { error: String(error) }, })) } finally { setLoading((prev) => ({ ...prev, [testName]: false })) } }e2e/react-start/basic/src/raw-stream-fns.ts (2)
40-41: Export statement referencesconcatBytesbefore it's defined.Line 41 exports
concatBytes, but the function is not defined until lines 78-87. While JavaScript hoisting allows this to work, it reduces code readability and could be confusing. Consider either moving the export after the function definition or moving the function definition before the export.🔎 Proposed refactor
Option 1: Move export after all helper definitions
-// Export helpers for use in components and SSR routes -export { encode, createDelayedStream, concatBytes } - // Expected data for hint tests - defined here for both server and client verification // Test 7: Text hint with pure text export const TEST7_CHUNKS = [ encode('Hello, '), encode('World! '), encode('This is text.'), ] export const TEST7_EXPECTED = concatBytes(TEST7_CHUNKS) // ... (other TEST constants) // Helper to concatenate byte arrays function concatBytes(chunks: Array<Uint8Array>): Uint8Array { const totalLength = chunks.reduce((acc, c) => acc + c.length, 0) const result = new Uint8Array(totalLength) let offset = 0 for (const chunk of chunks) { result.set(chunk, offset) offset += chunk.length } return result } + +// Export helpers for use in components and SSR routes +export { encode, createDelayedStream, concatBytes }Option 2: Move concatBytes definition before the export
+// Helper to concatenate byte arrays +function concatBytes(chunks: Array<Uint8Array>): Uint8Array { + const totalLength = chunks.reduce((acc, c) => acc + c.length, 0) + const result = new Uint8Array(totalLength) + let offset = 0 + for (const chunk of chunks) { + result.set(chunk, offset) + offset += chunk.length + } + return result +} + // Export helpers for use in components and SSR routes export { encode, createDelayedStream, concatBytes } // Expected data for hint tests - defined here for both server and client verification // Test 7: Text hint with pure text export const TEST7_CHUNKS = [ encode('Hello, '), encode('World! '), encode('This is text.'), ] export const TEST7_EXPECTED = concatBytes(TEST7_CHUNKS) // ... (other TEST constants) - -// Helper to concatenate byte arrays -function concatBytes(chunks: Array<Uint8Array>): Uint8Array { - const totalLength = chunks.reduce((acc, c) => acc + c.length, 0) - const result = new Uint8Array(totalLength) - let offset = 0 - for (const chunk of chunks) { - result.set(chunk, offset) - offset += chunk.length - } - return result -}
385-394: Reconsider the ESLint disable comment.The comment
@typescript-eslint/no-unnecessary-conditionis disabled for thewhile (true)pattern. This is a common idiom for consuming streams, and the condition inif (done) breakis necessary. However, with React 19, you might consider using async iteration instead, which is cleaner and doesn't require ESLint disables.🔎 Alternative using async iteration
export function createStreamConsumer() { const decoder = new TextDecoder() return async function consumeStream( stream: ReadableStream<Uint8Array> | RawStream, ): Promise<string> { // Handle both RawStream (from type system) and ReadableStream (runtime) const actualStream = stream instanceof RawStream ? stream.stream : (stream as ReadableStream<Uint8Array>) - const reader = actualStream.getReader() const chunks: Array<string> = [] - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - while (true) { - const { done, value } = await reader.read() - if (done) break + for await (const value of actualStream) { chunks.push(decoder.decode(value, { stream: true })) } return chunks.join('') } }Note: This requires the stream to be async-iterable, which ReadableStream is in modern environments.
packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
161-203: Consider adding validation or logging for unknown frame types.The switch statement has no
defaultcase, so unknown frame types are silently ignored. This could hide protocol errors or lead to silent data loss if the protocol evolves.🔎 Suggested enhancement
Add a default case to detect unknown frame types:
switch (type) { case FrameType.JSON: jsonController.enqueue(textDecoder.decode(payload)) break case FrameType.CHUNK: { const ctrl = ensureController(streamId) if (ctrl) { ctrl.enqueue(payload) } break } case FrameType.END: { const ctrl = ensureController(streamId) if (ctrl) { try { ctrl.close() } catch { // Already closed } streamControllers.delete(streamId) } break } case FrameType.ERROR: { const ctrl = ensureController(streamId) if (ctrl) { const message = textDecoder.decode(payload) ctrl.error(new Error(message)) streamControllers.delete(streamId) } break } + + default: + // Unknown frame type - log or throw for debugging + console.warn(`Unknown frame type: ${type}`) + break }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/react-start/basic/src/raw-stream-fns.tse2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-single.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/tests/raw-stream.spec.tspackages/start-client-core/src/client-rpc/frame-decoder.tspackages/start-client-core/src/client-rpc/serverFnFetcher.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/start-client-core/src/client-rpc/serverFnFetcher.ts
- e2e/react-start/basic/tests/raw-stream.spec.ts
- e2e/react-start/basic/src/routes/raw-stream/ssr-single.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/raw-stream-fns.tspackages/start-client-core/src/client-rpc/frame-decoder.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsxe2e/react-start/basic/src/raw-stream-fns.tspackages/start-client-core/src/client-rpc/frame-decoder.ts
🧠 Learnings (2)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/src/routes/raw-stream/client-call.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsxe2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsx
🧬 Code graph analysis (4)
e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsx (2)
e2e/react-start/basic/src/raw-stream-fns.ts (5)
encode(41-41)concatBytes(41-41)createDelayedStream(41-41)collectBytes(418-444)compareBytes(447-480)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)
e2e/react-start/basic/src/routes/raw-stream/ssr-multiple.tsx (5)
e2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx (1)
Route(10-29)e2e/react-start/basic/src/routes/raw-stream/ssr-single.tsx (1)
Route(10-23)e2e/react-start/basic/src/routes/raw-stream/ssr-text-hint.tsx (1)
Route(34-69)e2e/react-start/basic/src/raw-stream-fns.ts (3)
createDelayedStream(41-41)encode(41-41)createStreamConsumer(371-394)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)
e2e/react-start/basic/src/routes/raw-stream/ssr-mixed.tsx (3)
e2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx (1)
Route(22-46)e2e/react-start/basic/src/raw-stream-fns.ts (3)
createDelayedStream(41-41)encode(41-41)createStreamConsumer(371-394)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)
e2e/react-start/basic/src/routes/raw-stream/ssr-binary-hint.tsx (2)
e2e/react-start/basic/src/raw-stream-fns.ts (5)
encode(41-41)concatBytes(41-41)createDelayedStream(41-41)collectBytes(418-444)compareBytes(447-480)packages/router-core/src/ssr/serializer/RawStream.ts (1)
RawStream(36-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (5)
packages/start-client-core/src/client-rpc/frame-decoder.ts (5)
1-15: LGTM: Efficient constant reuse.The cached
TextDecoderand sharedEMPTY_BUFFERare good optimizations that avoid repeated allocations during frame processing.
42-47: LGTM: Definite assignment assertion is safe here.The
!assertion onjsonControlleris justified because thestartcallback executes synchronously duringReadableStreamconstruction, ensuring the controller is assigned before the IIFE can access it.
49-84: LGTM: Proper lazy stream creation with race condition handling.The
ensureControllerpattern correctly handles cases whereENDorERRORframes arrive before deserialization creates the stream viagetOrCreateStream.
94-117: LGTM: Efficient buffer management strategy.The buffer list approach avoids O(n²) concatenation, and using
slice()inconsumeBytescorrectly releases memory of consumed portions for GC, trading a copy for memory efficiency.
207-237: LGTM: Proper cleanup and error propagation.The cleanup logic correctly:
- Closes all streams on normal completion
- Propagates errors to all active streams
- Releases the reader lock in the
finallyblock- Preserves the streams map for application consumption
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
378-395: Unhandled promise rejection in background processing IIFE.The async IIFE starting at line 378 catches errors internally and logs them, but if an unexpected error occurs before the
tryblock or during iteration setup, it could result in an unhandled promise rejection. Consider adding a.catch()at the IIFE invocation.🔎 Suggested fix
// Process remaining frames asynchronously (for streaming refs like RawStream) ;(async () => { try { // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition while (true) { const { value, done } = await reader.read() if (done) break if (value) { try { onMessage(JSON.parse(value)) } catch (e) { onError?.(`Invalid JSON: ${value}`, e) } } } } catch (err) { onError?.('Stream processing error:', err) } - })() + })().catch((err) => { + onError?.('Unhandled stream processing error:', err) + })packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
49-51: Consider usinglet cancelled: boolean = falsewithout the type assertion.The
as booleanassertion is unusual here. A simplelet cancelled = falsewould be cleaner and TypeScript would correctly infer it asboolean.🔎 Suggested fix
- let cancelled = false as boolean + let cancelled = falsepackages/start-server-core/src/frame-protocol.ts (1)
101-101: Same style nit:as booleantype assertion is unnecessary.🔎 Suggested fix
- let cancelled = false as boolean + let cancelled = false
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/router-core/src/ssr/serializer/RawStream.tspackages/router-core/tests/RawStream.test.tspackages/start-client-core/src/client-rpc/frame-decoder.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-client-core/tests/frame-decoder.test.tspackages/start-server-core/src/frame-protocol.tspackages/start-server-core/src/server-functions-handler.tspackages/start-server-core/tests/frame-protocol.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/start-server-core/tests/frame-protocol.test.ts
- packages/start-server-core/src/server-functions-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-client-core/tests/frame-decoder.test.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/router-core/tests/RawStream.test.tspackages/router-core/src/ssr/serializer/RawStream.tspackages/start-server-core/src/frame-protocol.tspackages/start-client-core/src/client-rpc/frame-decoder.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-client-core/tests/frame-decoder.test.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/router-core/tests/RawStream.test.tspackages/router-core/src/ssr/serializer/RawStream.tspackages/start-server-core/src/frame-protocol.tspackages/start-client-core/src/client-rpc/frame-decoder.ts
🧠 Learnings (2)
📚 Learning: 2025-12-24T22:47:44.320Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6211
File: e2e/react-start/i18n-paraglide/src/server.ts:6-6
Timestamp: 2025-12-24T22:47:44.320Z
Learning: In TanStack Router projects using `inlang/paraglide-js`, the callback passed to `paraglideMiddleware` should use `() => handler.fetch(req)` (referencing the outer `req`) instead of `({ request }) => handler.fetch(request)`. This is intentional because the router needs the untouched URL to perform its own rewrite logic with `deLocalizeUrl`/`localizeUrl`. The middleware's processed request would delocalize the URL and interfere with the router's rewrite handling.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/RawStream.test.ts
🧬 Code graph analysis (3)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (5)
packages/start-server-core/src/frame-protocol.ts (1)
TSS_CONTENT_TYPE_FRAMED(17-17)packages/start-client-core/src/constants.ts (2)
TSS_CONTENT_TYPE_FRAMED(12-12)validateFramedProtocolVersion(56-68)packages/start-client-core/src/index.tsx (2)
TSS_CONTENT_TYPE_FRAMED(86-86)validateFramedProtocolVersion(94-94)packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
createFrameDecoder(39-388)packages/router-core/src/index.ts (1)
createRawStreamDeserializePlugin(439-439)
packages/router-core/tests/RawStream.test.ts (1)
packages/router-core/src/ssr/serializer/RawStream.ts (7)
stream(258-260)stream(286-288)stream(342-356)stream(413-417)RawStream(36-45)createRawStreamRPCPlugin(394-435)createRawStreamDeserializePlugin(443-464)
packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
packages/start-client-core/src/constants.ts (3)
FRAME_HEADER_SIZE(31-31)FrameType(17-26)FrameType(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (24)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
191-218: LGTM! Framed response handling is well-structured.The framing branch correctly:
- Validates protocol version before processing
- Guards against missing response body
- Creates the frame decoder and wires up the deserialize plugin
- Properly orders plugins with RawStream plugin first
packages/start-client-core/src/client-rpc/frame-decoder.ts (5)
17-21: Good hardening limits for DoS protection.The defined limits (16MiB payload, 32MiB buffer, 1024 streams, 100k frames) provide reasonable protection against resource exhaustion attacks while allowing for realistic workloads.
83-117: Stream lifecycle handling is well-designed.The
getOrCreateStreamfunction correctly:
- Returns existing streams
- Returns an immediately-closed stream for cancelled stream IDs (preventing consumer hangs)
- Enforces the MAX_STREAMS limit
- Sets up cleanup on cancel
142-196: Header parsing with fast/slow path optimization is efficient.The fast path (header fits in first chunk) avoids allocation, while the slow path correctly handles headers spanning multiple chunks. The bitwise operations for big-endian parsing are correct with
>>> 0to ensure unsigned interpretation.
254-272: Stream ID validation enforces protocol conventions correctly.The validation ensures JSON frames use streamId 0 and raw frames use non-zero IDs, which prevents protocol confusion attacks.
362-384: Robust cleanup in error and finally blocks.The error handling correctly propagates errors to both jsonController and all stream controllers, and the finally block properly releases the reader lock. The try-catch wrappers around controller operations handle the case where streams may already be closed.
packages/start-client-core/tests/frame-decoder.test.ts (4)
8-40: Well-designed test helper functions.The
encodeFrame,encodeJSONFrame,encodeChunkFrame,encodeEndFrame, andencodeErrorFramehelpers provide a clean API for constructing test frames with correct header encoding.
44-176: Comprehensive error condition testing.Tests cover:
- Unknown frame type rejection
- Invalid streamId for both raw and JSON frames
- Oversized payload detection
- Incomplete frame detection
- Cancellation propagation
- Stream count limits
- Buffer size limits
These tests validate the hardening measures in the decoder.
462-509: Important race condition test for late consumers.The "should preserve stream after END frame for late consumers" test validates that streams remain accessible even if the deserializer calls
getOrCreateStreamafter all frames (including END) have been processed. This is critical for correct RawStream deserialization.
336-361: Thorough header splitting tests.Testing header splits at every byte boundary ensures the slow path for multi-chunk headers is fully exercised. This is important for correctness with small network chunks or chunked transfers.
packages/start-server-core/src/frame-protocol.ts (4)
31-50: Efficient header encoding avoiding DataView allocation.Writing header bytes directly with bitwise operations matches the client decoder's expectations and avoids per-frame DataView allocation overhead. The big-endian encoding is correct.
79-83: Good error message extraction with fallback.The
encodeErrorFramefunction safely extracts the error message from Error instances or converts other values to strings, with a fallback for null/undefined.
98-136: Well-structured stream lifecycle management.The
activePumpscounter andcancelledflag ensure:
- All pumps must complete before closing the output stream
- Cancellation propagates to all underlying readers
- Safe enqueue/error/close helpers prevent exceptions after cancellation
162-164: JSON stream errors are correctly treated as fatal.Unlike raw stream errors (which emit ERROR frames), JSON stream errors call
safeErrorto error the entire multiplexed response. This is appropriate since JSON carries the main response payload.packages/router-core/src/ssr/serializer/RawStream.ts (6)
36-45: Clean RawStream class design with hint option.The class correctly defaults to
'binary'hint and exposes both the underlying stream and hint as readonly properties.
60-75: Robust base64 encoding with Node Buffer optimization.The implementation:
- Uses Node's Buffer when available for speed
- Falls back to chunked
String.fromCharCode+btoain browsers- Uses 32KB chunks to avoid stack overflow with
apply
197-240: Text mode streaming with UTF-8 first, base64 fallback.The
toTextStreamfunction correctly:
- Uses
TextDecoderwithfatal: trueandstream: truefor multi-byte handling- Falls back to
{ $b64: ... }wrapper when UTF-8 decoding fails- Flushes the decoder on stream end to handle any buffered bytes
394-435: RPC plugin correctly assigns sequential stream IDs.The plugin maintains its own counter (
nextStreamId) independent of seroval internals, ensuring predictable stream ID assignment. Bothasyncandstreamparse methods register the stream and return the ID.
443-463: Deserialize plugin correctly wires up stream reconstruction.The
createRawStreamDeserializePlugincreates a plugin that:
- Never matches during serialization (
test: () => false)- Calls
getOrCreateStreamwith the node'sstreamIdduring deserializationThis cleanly separates client-side (deserialize only) from server-side (serialize only) concerns.
162-168: Factory functions are syntactically valid and self-contained for HTML injection.The
FACTORY_BINARYandFACTORY_TEXTstring constants contain complete, self-contained JavaScript that can be safely injected into HTML script tags. Both correctly implement base64 decoding and stream construction logic with proper error handling.packages/router-core/tests/RawStream.test.ts (4)
9-45: Comprehensive RawStream class tests.Tests verify:
- Stream wrapping preserves the original stream reference
instanceofcheck works correctly- Default hint is
'binary'- Both
'binary'and'text'hint options are accepted
47-117: Good RPC plugin behavior tests.Tests verify:
onRawStreamcallback receives stream ID and stream reference- Serialized output contains the expected tag and streamId
- Multiple streams get unique IDs
146-148: Direct plugin.deserialize access is acceptable for unit testing.Accessing
(plugin as any).deserialize(node, {})directly bypasses seroval's normal flow but is appropriate for isolated unit testing of the deserialize behavior.
174-227: Round-trip test validates end-to-end workflow.The test correctly:
- Serializes a data object containing a RawStream with the RPC plugin
- Verifies the stream was collected
- Creates a deserialize plugin with access to collected streams
- Deserializes and verifies the original stream reference is restored
This validates the complete serialization/deserialization cycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/start-server-core/src/frame-protocol.ts (3)
31-50: Consider validating streamId bounds.The function correctly encodes big-endian values. However,
streamIdis typed asnumberwhich could be negative or exceed uint32 max (4,294,967,295). Negative numbers or values > 2³²-1 would produce unexpected byte sequences due to JavaScript's bitwise operation semantics.If invalid streamIds can reach this function from external callers, consider adding validation or documenting the expected range.
🔎 Optional validation
export function encodeFrame( type: FrameType, streamId: number, payload: Uint8Array, ): Uint8Array { + if (streamId < 0 || streamId > 0xffffffff || !Number.isInteger(streamId)) { + throw new RangeError(`streamId must be a uint32, got: ${streamId}`) + } const frame = new Uint8Array(FRAME_HEADER_SIZE + payload.length)
62-83: StreamId 0 is reserved for JSON but not validated in chunk/end/error helpers.
encodeJSONFramecorrectly hardcodes streamId 0 for JSON frames. However,encodeChunkFrame,encodeEndFrame, andencodeErrorFrameaccept any streamId including 0, which could cause protocol confusion on the client if accidentally passed.Consider whether validation is needed here or if the caller (
createMultiplexedStream) is the appropriate enforcement point.
101-101: Redundant type assertion.The
as booleanassertion is unnecessary since TypeScript already infersfalseas typeboolean. This can be simplified:- let cancelled = false as boolean + let cancelled = false
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-server-core/src/frame-protocol.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-server-core/src/frame-protocol.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/frame-protocol.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (4)
packages/start-server-core/src/frame-protocol.ts (4)
1-26: Well-documented protocol with good performance optimizations.The frame format documentation is clear, and the module-level caching of
TextEncoderandEMPTY_PAYLOADavoids unnecessary allocations.
144-166: Correct async pump implementation with proper cancellation handling.The pump correctly checks the
cancelledflag after each await, handles errors as fatal for the JSON stream, and ensures cleanup viafinally. The reader cancel callback is properly registered for coordinated cancellation.
169-197: Good non-fatal error handling for raw streams.The design correctly allows other streams to continue when one errors. The ERROR frame implicitly terminates that stream, so no separate END frame is needed. This provides good fault isolation.
199-215: Concurrent pump orchestration with proper cancellation.The fire-and-forget pattern for starting pumps is appropriate here since each pump has comprehensive internal error handling. The cancel handler correctly propagates cancellation to all underlying readers, ensuring prompt termination of ongoing work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/start-client-core/src/client-rpc/frame-decoder.ts (1)
49-49: Remove redundant type assertion.The
as booleanassertion is unnecessary since TypeScript already infersfalseas typeboolean.🔎 Proposed fix
- let cancelled = false as boolean + let cancelled = false
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-client-core/src/client-rpc/frame-decoder.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-client-core/src/client-rpc/frame-decoder.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-client-core/src/client-rpc/frame-decoder.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (12)
packages/start-client-core/src/client-rpc/frame-decoder.ts (12)
1-22: LGTM! DoS protections properly implemented.The hardening limits address the previous review's concerns about unbounded frame sizes. The constants are well-chosen for typical use cases, and the shared TextDecoder and EMPTY_BUFFER optimizations are good practices.
23-41: LGTM! Well-designed public interface.The
FrameDecoderResultinterface provides a clean separation between JSON chunks and raw stream access, making it easy for consumers to work with multiplexed responses.
54-77: LGTM! Proper stream cancellation handling.The cancellation logic correctly propagates errors to all active streams and cleans up resources. The try-catch blocks with ignores are appropriate here since errors during cleanup should not prevent other cleanup actions.
79-117: LGTM! Robust stream lifecycle management.The handling of already-cancelled streams (lines 91-96) is particularly important—it prevents consumers from hanging when requesting a stream that already received END/ERROR frames. The MAX_STREAMS limit properly guards against resource exhaustion.
119-128: LGTM! Correct handling of stream lifecycle.The
ensureControllerpattern ensures streams are created when processing CHUNK/END/ERROR frames, even if the deserialize plugin hasn't requested them yet. This is correct—when the plugin later callsgetOrCreateStream, lines 91-96 will appropriately return an already-closed stream for END/ERROR cases.
142-196: LGTM! Efficient header parsing with proper optimizations.The fast-path optimization (lines 152-167) avoids unnecessary allocations for the common case where the header fits in a single chunk. The big-endian parsing and unsigned integer conversion (
>>> 0) are correctly implemented.
198-226: LGTM! Efficient buffer extraction.The function correctly flattens buffer chunks on demand and properly manages partial chunk consumption (lines 217-221). The EMPTY_BUFFER optimization for zero-length payloads is a nice touch.
228-246: LGTM! Proper buffer overflow protection.The validation at lines 239-243 prevents buffer exhaustion by enforcing the MAX_BUFFERED_BYTES limit before accepting new data. The ESLint disables are justified for the intentional
while (true)pattern.
247-289: LGTM! Comprehensive validation addresses DoS concerns.All the necessary validations are present:
- Frame type validation (lines 255-262)
- Stream ID convention enforcement (lines 264-273)
- Payload size limit (lines 275-279)
- Frame count limit (lines 284-288)
This fully addresses the past review's concerns about DoS attacks via malformed frames.
290-340: LGTM! Correct frame processing for all types.Each frame type is handled appropriately:
- JSON frames are decoded and enqueued with error tolerance
- CHUNK frames ensure the stream exists before enqueuing
- END/ERROR frames properly update
cancelledStreamIdsand clean up controllersThe error message decoding in ERROR frames (line 333) allows the server to provide meaningful error information.
342-386: LGTM! Thorough cleanup and error propagation.The validation at line 343 ensures no incomplete frames are left, which could indicate a protocol error. The error propagation logic (lines 363-377) correctly notifies all active streams, and the finally block guarantees reader lock cleanup regardless of outcome.
388-389: LGTM! Clean return of decoder result.The function returns the properly configured
FrameDecoderResultwith both JSON chunk stream and stream getter function.
In CI, hydration takes longer so reader.read() can hang if called before seroval's stream factory connects callbacks. 100ms delay ensures stream is ready to be consumed.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.