fix: fix streaming#7497
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds SSR-aware response types and normalization, refactors server-SSR lifecycle and script buffering, implements a backpressure-safe transformStreamWithRouter (fast/main paths and HTML boundary detection), wires abort/cleanup into React/Solid/Vue/start-server flows, strengthens query SSR teardown, and adds extensive tests/benchmarks. ChangesSSR Streaming, Abort Handling & Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
|
View your CI Pipeline Execution ↗ for commit ef37315
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview6 package(s) bumped directly, 21 bumped as dependents. 🟩 Patch bumps
|
schiller-manuel
force-pushed
the
fix-streaming
branch
from
65d9a9b to
b744547
Compare
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/vue-router/tests/renderRouterToStream.test.tsx (1)
40-52: 💤 Low valueConsider adding
await router.serverSsr!.dehydrate()inbuildRouter.The bot test (line 70) manually calls
dehydrate(), butbuildRouterdoesn't include it. For consistency with the pattern across all framework tests, consider including it in the helper or documenting why it's only needed for the bot test.🤖 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/vue-router/tests/renderRouterToStream.test.tsx` around lines 40 - 52, The helper buildRouter sets up server SSR utils but does not call the server-side dehydrate step, causing inconsistent setup vs tests that call router.serverSsr!.dehydrate() manually; update buildRouter to await router.serverSsr!.dehydrate() after attachRouterServerSsrUtils({ router, manifest: undefined }) and before returning the router (or document in a comment why a specific test should call dehydrate itself), so tests can rely on buildRouter returning a fully dehydrated server router.packages/react-router/tests/renderRouterToStream.test.tsx (1)
23-34: 💤 Low valueConsider adding
await router.serverSsr!.dehydrate()for completeness.The
buildRouterhelper loads the router but doesn't calldehydrate(). While some tests (like line 32 in the setup throw test'sbuildRoutercall) do call it separately, other tests rely onbuildRouterdirectly. This inconsistency could lead to flaky behavior if future tests expect dehydration to have occurred.Looking at the Solid/Vue test files, they also have this pattern inconsistently applied.
🤖 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/react-router/tests/renderRouterToStream.test.tsx` around lines 23 - 34, The helper buildRouter does not consistently dehydrate the server SSR state; update buildRouter to call and await router.serverSsr!.dehydrate() after attachRouterServerSsrUtils({ router, manifest: undefined }) and after await router.load(), so that the router is always dehydrated before return; locate the function buildRouter and add the await router.serverSsr!.dehydrate() invocation (using the existing router.serverSsr property) prior to returning the router.packages/solid-router/tests/renderRouterToStream.test.tsx (1)
34-46: 💤 Low valueConsider adding
await router.serverSsr!.dehydrate()for test consistency.Similar to the React tests,
buildRouterdoesn't calldehydrate(). While this may work for the current tests, adding it would match the pattern in other test files and ensure the router is in the expected SSR state.🤖 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/solid-router/tests/renderRouterToStream.test.tsx` around lines 34 - 46, Add a call to dehydrate the server SSR state in buildRouter: after attaching SSR utils with attachRouterServerSsrUtils({ router, manifest: undefined }) and finishing router.load(), invoke await router.serverSsr!.dehydrate() so the router is put into the same SSR-dehydrated state used by other tests; reference the buildRouter function and the router.serverSsr!.dehydrate() method when making this change.packages/start-server-core/src/createStartHandler.ts (1)
214-217: 🏗️ Heavy liftReplace
TODO/anymiddleware contracts with concrete types.
executeMiddlewarenow sits on a critical SSR cleanup path, but still accepts/returnsTODO(any) in core control flow. Tightening these types would prevent accidental invalid response/context shapes from bypassing cleanup logic.As per coding guidelines
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 524-532, 648-656
🤖 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/start-server-core/src/createStartHandler.ts` around lines 214 - 217, The executeMiddleware signature (and analogous spots at the other occurrences) uses TODO/any for middleware and ctx; replace these with concrete, strict types: define a RequestContext (or reuse existing context type used across the SSR pipeline) for ctx, a Middleware type like (ctx: RequestContext) => Promise<void | HandlerCallbackResult | Response> (or synchronous equivalents), and ensure executeMiddleware returns Promise<{ ctx: RequestContext; response: HandlerCallbackResult }>; update any intermediate helper types (e.g., HandlerCallbackResult) so their shape is explicit (status, headers, body, etc.), and propagate these concrete types into the other two locations referenced so the cleanup path enforces correct shapes rather than any/ TODO.packages/start-server-core/tests/createStartHandler.test.ts (1)
72-82: ⚡ Quick winReduce repeated
as anycasts in new SSR test helpers.Please introduce small helper types (typed router/typed SSR response) so ownership tests keep compile-time guarantees without pervasive
anycasts.As per coding guidelines
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 106-107, 129-130, 189-190, 217-218, 251-252, 277-278, 304-305
🤖 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/start-server-core/tests/createStartHandler.test.ts` around lines 72 - 82, Replace the repeated `as any` casts by introducing small test-only types and helper factories: add a TypedTestRouter (or TestRouter) interface representing the minimal router shape used by attachRouterServerSsrUtils and createSsrStreamResponse, and a helper function (e.g., makeTestRouter()) that returns a strongly-typed instance; likewise add a TypedSsrResponse or makeTestSsrResponse(stream) helper that returns Response with the proper typed serverSsr attached. Update calls to attachRouterServerSsrUtils({ router: router as any, ... }) and createSsrStreamResponse(router as any, ...) to use the new test helpers or typed variables (router: TestRouter) so the code at symbols attachRouterServerSsrUtils and createSsrStreamResponse no longer needs `as any`; apply the same pattern to the other occurrences listed (lines ~106-107, 129-130, 189-190, 217-218, 251-252, 277-278, 304-305).packages/router-core/tests/transformStreamWithRouter.test.ts (1)
185-199: 🏗️ Heavy liftConsolidate typed test fakes to remove repeated
as anycasts.The suite is strong, but repeated
anycasts weaken strict-mode guarantees. A small typed test harness (typed fake router + typed transform wrappers) would keep the same coverage without type escapes.As per coding guidelines
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 223-226, 946-950
🤖 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 185 - 199, Tests repeatedly use `as any` casts (weakening strict-mode guarantees) around the test router and transform wrappers; update the test harness to provide properly typed fakes instead of casting. Concretely, add a typed helper for building SSR routers (refactor createRealSsrRouter to accept and return precise generics) and create typed transform wrappers used in transformStreamWithRouter.test so callers of BaseRootRoute, BaseRoute, createTestRouter and createMemoryHistory no longer require `as any`; update usages at the noted locations (including the other occurrences around lines 223-226 and 946-950) to use the new typed helpers and remove the `as any` casts. Ensure the new helpers' signatures reflect the real route/component types so the TypeScript compiler enforces strict-mode types across the tests.packages/router-core/tests/ssr-server-cleanup.test.ts (1)
18-18: ⚡ Quick winReduce
anyusage in new test helpers/calls to keep strict typing coverage meaningful.Please replace
Record<string, any>andas anycasts here withunknown/narrowed helper types (e.g., typed fake router/stream aliases). This keeps tests aligned with strict-mode guarantees.As per coding guidelines
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 364-370
🤖 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/ssr-server-cleanup.test.ts` at line 18, The test helper buildRouter currently types its parameter as Record<string, any> and uses as any casts; replace that broad any with a stricter type (e.g., unknown or a narrow helper interface like DehydratedState | undefined) and update callers to narrow/validate that data before use; also remove any "as any" casts in this file (and the other occurrences around the 364-370 area) by introducing small test-only types or typed fake router/stream aliases (e.g., FakeRouter, FakeStream) and use type guards or explicit conversion helpers to map unknown to those types so the tests remain strictly typed under TS strict mode while preserving the same runtime behavior.
🤖 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 `@e2e/react-start/streaming-ssr/tests/query-heavy.spec.ts`:
- Around line 144-145: The test currently checks for the broad substring
'slow-async' which can match static markup; update the assertions in
query-heavy.spec.ts to assert against a concrete payload-specific token from the
stream (e.g., the exact streamed value token emitted for the slow async query)
instead of the generic 'slow-async' substring; specifically, change the two
expectations that use html.indexOf('slow-async') to search for the exact payload
token string (still using html and endIndex) and assert that its index is >= 0
and < endIndex so the streamed payload ordering is validated precisely.
In `@packages/router-core/src/ssr/handlerCallback.ts`:
- Around line 45-57: The dispose function currently returns immediately after a
successful await on response.body!.cancel(reason), which prevents
router.serverSsr?.cleanup() from running; modify dispose (the async dispose
method that checks disposed) to always invoke router.serverSsr?.cleanup() after
attempting response.body!.cancel(reason) — e.g., remove the premature return
and/or use try/finally so that regardless of successful cancel or thrown errors,
router.serverSsr?.cleanup() is executed (keep the disposed guard as-is).
In `@packages/router-core/src/ssr/transformStreamWithRouter.ts`:
- Around line 21-24: Update the transformReadableStreamWithRouter signature to
type routerStream as ReadableStream<Uint8Array | string> (and adjust any other
file functions that currently use untyped ReadableStream similarly), update
related types such as TransformStreamWithRouterOptions to reflect the stricter
union where used, and remove the ArrayBufferView cast-paths inside the
implementation; instead branch on chunk type (string vs Uint8Array) and handle
conversions explicitly (e.g., TextEncoder/decoder or direct passthrough) so no
unsafe ArrayBufferView casts remain and TypeScript strict mode is satisfied.
In `@packages/router-core/tests/closing-tag-detection.bench.ts`:
- Around line 266-269: The function findBodyEndTagNativeLowerThenLastOpenSlash
currently searches for the literal lowercase BODY_END_TAG in the original string
and skips the fallback, causing mixed-case tags like </BODY> to be missed;
change it to perform a case-insensitive lastIndexOf by searching on a lowercased
copy of the input (e.g., const lower = str.toLowerCase(); const index =
lower.lastIndexOf(BODY_END_TAG)) and then return index === -1 ?
findBodyEndTagLastOpenSlash(str) : index so the index stays valid and mixed-case
tags are found.
In `@packages/start-server-core/src/createStartHandler.ts`:
- Around line 223-233: The setResponse helper currently overwrites the
module-scoped streamResponse when an SsrResponse with serverSsrCleanup ===
'stream' arrives, leaking the previous stream; make setResponse async, check if
streamResponse exists and call its disposal/cleanup method (or await any
provided cleanup promise) before assigning the new stream, retain the
isSsrResponse and serverSsrCleanup checks, and update all call sites to await
setResponse(...) so disposal completes before continuing.
---
Nitpick comments:
In `@packages/react-router/tests/renderRouterToStream.test.tsx`:
- Around line 23-34: The helper buildRouter does not consistently dehydrate the
server SSR state; update buildRouter to call and await
router.serverSsr!.dehydrate() after attachRouterServerSsrUtils({ router,
manifest: undefined }) and after await router.load(), so that the router is
always dehydrated before return; locate the function buildRouter and add the
await router.serverSsr!.dehydrate() invocation (using the existing
router.serverSsr property) prior to returning the router.
In `@packages/router-core/tests/ssr-server-cleanup.test.ts`:
- Line 18: The test helper buildRouter currently types its parameter as
Record<string, any> and uses as any casts; replace that broad any with a
stricter type (e.g., unknown or a narrow helper interface like DehydratedState |
undefined) and update callers to narrow/validate that data before use; also
remove any "as any" casts in this file (and the other occurrences around the
364-370 area) by introducing small test-only types or typed fake router/stream
aliases (e.g., FakeRouter, FakeStream) and use type guards or explicit
conversion helpers to map unknown to those types so the tests remain strictly
typed under TS strict mode while preserving the same runtime behavior.
In `@packages/router-core/tests/transformStreamWithRouter.test.ts`:
- Around line 185-199: Tests repeatedly use `as any` casts (weakening
strict-mode guarantees) around the test router and transform wrappers; update
the test harness to provide properly typed fakes instead of casting. Concretely,
add a typed helper for building SSR routers (refactor createRealSsrRouter to
accept and return precise generics) and create typed transform wrappers used in
transformStreamWithRouter.test so callers of BaseRootRoute, BaseRoute,
createTestRouter and createMemoryHistory no longer require `as any`; update
usages at the noted locations (including the other occurrences around lines
223-226 and 946-950) to use the new typed helpers and remove the `as any` casts.
Ensure the new helpers' signatures reflect the real route/component types so the
TypeScript compiler enforces strict-mode types across the tests.
In `@packages/solid-router/tests/renderRouterToStream.test.tsx`:
- Around line 34-46: Add a call to dehydrate the server SSR state in
buildRouter: after attaching SSR utils with attachRouterServerSsrUtils({ router,
manifest: undefined }) and finishing router.load(), invoke await
router.serverSsr!.dehydrate() so the router is put into the same SSR-dehydrated
state used by other tests; reference the buildRouter function and the
router.serverSsr!.dehydrate() method when making this change.
In `@packages/start-server-core/src/createStartHandler.ts`:
- Around line 214-217: The executeMiddleware signature (and analogous spots at
the other occurrences) uses TODO/any for middleware and ctx; replace these with
concrete, strict types: define a RequestContext (or reuse existing context type
used across the SSR pipeline) for ctx, a Middleware type like (ctx:
RequestContext) => Promise<void | HandlerCallbackResult | Response> (or
synchronous equivalents), and ensure executeMiddleware returns Promise<{ ctx:
RequestContext; response: HandlerCallbackResult }>; update any intermediate
helper types (e.g., HandlerCallbackResult) so their shape is explicit (status,
headers, body, etc.), and propagate these concrete types into the other two
locations referenced so the cleanup path enforces correct shapes rather than
any/ TODO.
In `@packages/start-server-core/tests/createStartHandler.test.ts`:
- Around line 72-82: Replace the repeated `as any` casts by introducing small
test-only types and helper factories: add a TypedTestRouter (or TestRouter)
interface representing the minimal router shape used by
attachRouterServerSsrUtils and createSsrStreamResponse, and a helper function
(e.g., makeTestRouter()) that returns a strongly-typed instance; likewise add a
TypedSsrResponse or makeTestSsrResponse(stream) helper that returns Response
with the proper typed serverSsr attached. Update calls to
attachRouterServerSsrUtils({ router: router as any, ... }) and
createSsrStreamResponse(router as any, ...) to use the new test helpers or typed
variables (router: TestRouter) so the code at symbols attachRouterServerSsrUtils
and createSsrStreamResponse no longer needs `as any`; apply the same pattern to
the other occurrences listed (lines ~106-107, 129-130, 189-190, 217-218,
251-252, 277-278, 304-305).
In `@packages/vue-router/tests/renderRouterToStream.test.tsx`:
- Around line 40-52: The helper buildRouter sets up server SSR utils but does
not call the server-side dehydrate step, causing inconsistent setup vs tests
that call router.serverSsr!.dehydrate() manually; update buildRouter to await
router.serverSsr!.dehydrate() after attachRouterServerSsrUtils({ router,
manifest: undefined }) and before returning the router (or document in a comment
why a specific test should call dehydrate itself), so tests can rely on
buildRouter returning a fully dehydrated server router.
🪄 Autofix (Beta)
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
Run ID: 252d919e-92b8-4f8b-b32d-6675ad3b362e
📒 Files selected for processing (37)
.changeset/fuzzy-jobs-eat.mde2e/react-start/streaming-ssr/tests/query-heavy.spec.tse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/index-BwsVWHwV.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/index-ElfLiipA.csse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/index-rPfEOKs3.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/lazy-page.lazy-BzoMuhUY.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/lazy-with-loader-page.lazy-BzLmn584.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/normal-page-BPOVyYTF.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/page-with-search-BhJv4yJI.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/scroll-block-BAas_Ct-.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/virtual-page.lazy-BCqvdivF.jse2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/index.htmlpackages/react-router/src/ssr/renderRouterToStream.tsxpackages/react-router/tests/renderRouterToStream.test.tsxpackages/router-core/src/router.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/handlerCallback.tspackages/router-core/src/ssr/server.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/src/ssr/transformStreamWithRouter.tspackages/router-core/tests/closing-tag-detection.bench.tspackages/router-core/tests/ssr-server-cleanup.test.tspackages/router-core/tests/transformStreamBackpressure.perf.test.tspackages/router-core/tests/transformStreamWithRouter.test.tspackages/router-core/vite.config.tspackages/router-ssr-query-core/package.jsonpackages/router-ssr-query-core/src/index.tspackages/router-ssr-query-core/tests/index.test.tspackages/router-ssr-query-core/vite.config.tspackages/solid-router/src/ssr/renderRouterToStream.tsxpackages/solid-router/tests/renderRouterToStream.test.tsxpackages/start-server-core/src/createStartHandler.tspackages/start-server-core/tests/createStartHandler.test.tspackages/start-server-core/tests/fixtures/start-manifest.tspackages/start-server-core/vite.config.tspackages/vue-router/src/ssr/renderRouterToStream.tsxpackages/vue-router/tests/renderRouterToStream.test.tsx
💤 Files with no reviewable changes (9)
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/normal-page-BPOVyYTF.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/page-with-search-BhJv4yJI.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/lazy-with-loader-page.lazy-BzLmn584.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/lazy-page.lazy-BzoMuhUY.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/index-ElfLiipA.css
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/virtual-page.lazy-BCqvdivF.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/index-rPfEOKs3.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/assets/scroll-block-BAas_Ct-.js
- e2e/vue-router/scroll-restoration-sandbox-vite/dist-hash/index.html
Comment on lines
21
to
+24
| export function transformReadableStreamWithRouter( | ||
| router: AnyRouter, | ||
| routerStream: ReadableStream, | ||
| opts?: TransformStreamWithRouterOptions, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Tighten stream chunk typing to avoid unsafe casts in the core transform path.
These signatures use untyped ReadableStream, which forces downstream coercion. Please type the stream as ReadableStream<Uint8Array | string> (or the exact supported union) and remove the ArrayBufferView cast path where possible.
As per coding guidelines **/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.
Also applies to: 199-203, 781-785
🤖 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 21 -
24, Update the transformReadableStreamWithRouter signature to type routerStream
as ReadableStream<Uint8Array | string> (and adjust any other file functions that
currently use untyped ReadableStream similarly), update related types such as
TransformStreamWithRouterOptions to reflect the stricter union where used, and
remove the ArrayBufferView cast-paths inside the implementation; instead branch
on chunk type (string vs Uint8Array) and handle conversions explicitly (e.g.,
TextEncoder/decoder or direct passthrough) so no unsafe ArrayBufferView casts
remain and TypeScript strict mode is satisfied.
Comment on lines
+266
to
+269
| function findBodyEndTagNativeLowerThenLastOpenSlash(str: string): number { | ||
| const index = str.lastIndexOf(BODY_END_TAG) | ||
| return index === -1 ? findBodyEndTagLastOpenSlash(str) : index | ||
| } |
Contributor
There was a problem hiding this comment.
Case-insensitive “last </body>” detection is incorrect in mixed-case input.
This shortcut returns the last lowercase </body> and skips fallback, so a later uppercase </BODY> is missed.
Suggested fix
function findBodyEndTagNativeLowerThenLastOpenSlash(str: string): number {
- const index = str.lastIndexOf(BODY_END_TAG)
- return index === -1 ? findBodyEndTagLastOpenSlash(str) : index
+ // Keep this variant correct for mixed-case tails; "native lowercase first"
+ // can miss a later uppercase </BODY>.
+ return findBodyEndTagLastOpenSlash(str)
}🤖 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/closing-tag-detection.bench.ts` around lines 266 -
269, The function findBodyEndTagNativeLowerThenLastOpenSlash currently searches
for the literal lowercase BODY_END_TAG in the original string and skips the
fallback, causing mixed-case tags like </BODY> to be missed; change it to
perform a case-insensitive lastIndexOf by searching on a lowercased copy of the
input (e.g., const lower = str.toLowerCase(); const index =
lower.lastIndexOf(BODY_END_TAG)) and then return index === -1 ?
findBodyEndTagLastOpenSlash(str) : index so the index stays valid and mixed-case
tags are found.
Comment on lines
+223
to
+233
| const setResponse = (response: TODO) => { | ||
| if (isSsrResponse(response)) { | ||
| if (response.serverSsrCleanup === 'stream') { | ||
| streamResponse = response | ||
| } | ||
| ctx.response = response.response | ||
| return | ||
| } | ||
|
|
||
| ctx.response = response | ||
| } |
Contributor
There was a problem hiding this comment.
Dispose previously tracked stream before overwriting streamResponse.
When a second streaming SsrResponse is assigned, the previous tracked stream handle is overwritten without disposal. That can leak SSR cleanup ownership for the earlier stream in the same middleware chain.
Proposed direction
- const setResponse = (response: TODO) => {
+ const setResponse = async (response: TODO) => {
if (isSsrResponse(response)) {
if (response.serverSsrCleanup === 'stream') {
+ if (streamResponse && streamResponse !== response) {
+ await streamResponse.dispose('middleware stream replaced')
+ }
streamResponse = response
}
ctx.response = response.response
return
}
ctx.response = response
}Then await setResponse(...) at call sites.
🤖 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/start-server-core/src/createStartHandler.ts` around lines 223 - 233,
The setResponse helper currently overwrites the module-scoped streamResponse
when an SsrResponse with serverSsrCleanup === 'stream' arrives, leaking the
previous stream; make setResponse async, check if streamResponse exists and call
its disposal/cleanup method (or await any provided cleanup promise) before
assigning the new stream, retain the isSsrResponse and serverSsrCleanup checks,
and update all call sites to await setResponse(...) so disposal completes before
continuing.
Collaborator
Author
There was a problem hiding this comment.
there can only be a single SSR response per request
Contributor
There was a problem hiding this comment.
@schiller-manuel, understood — since the architecture guarantees at most one SSR response per request, the overwrite scenario I was worried about can't occur. My concern doesn't apply here. Thanks for the clarification!
Merging this PR will not alter performance
Comparing 
Footnotes
|
schiller-manuel
force-pushed
the
fix-streaming
branch
from
f680d15 to
ef37315
Compare
schiller-manuel
force-pushed
the
fix-streaming
branch
from
ef37315 to
0c0a6cc
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #7402
Summary by CodeRabbit
Bug Fixes
New Features
Tests