Fix SSE double-wrapping bug when returning pre-formatted Response (#1661) #1663
Conversation
…h chunked transfer Fixes elysiajs#1661 Problem: When returning a Response with pre-formatted SSE data (content-type: text/event-stream and transfer-encoding: chunked) while using set.headers, the response would be double- wrapped with 'data:' prefix, resulting in 'data: data: hello' instead of 'data: hello'. Root Cause: - handleResponse merges set.headers into the Response - Sees transfer-encoding: chunked and routes through handleStream(streamResponse(response)) - handleStream detects SSE via content-type and auto-wraps chunks with 'data: ' format - But the Response body is ALREADY formatted as SSE by the user - Result: double-wrapping Solution: Added skipFormat parameter to createStreamHandler to prevent SSE auto-formatting when streaming pre-formatted Response bodies. The skipFormat flag is set to true when: - Response has transfer-encoding: chunked - Stream comes from streamResponse(response) (already formatted by user) This ensures: - Generator functions still get SSE formatting applied - Pre-formatted Response bodies are streamed as-is - No behavioral changes for existing working code Changes: - src/adapter/utils.ts: Added skipFormat parameter to createStreamHandler - src/adapter/utils.ts: Pass skipFormat=true for Response body streams - test/response/sse-double-wrap.test.ts: Added comprehensive regression tests Testing: - All 1434 existing tests pass - 3 new tests cover the bug scenarios - Verified generator functions still work correctly
WalkthroughAdds an optional skipFormat?: boolean to createStreamHandler and propagates it through the streaming pipeline so pre-formatted SSE responses bypass automatic SSE formatting and avoid double-wrapping. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant StreamHandler as createStreamHandler
participant RespHandler as createResponseHandler
participant Downstream as DownstreamStream
Client->>Router: HTTP request
Router->>StreamHandler: handle route result (passes skipFormat?)
alt skipFormat = false
StreamHandler->>RespHandler: detect SSE -> apply auto-formatting
RespHandler->>Downstream: stream formatted SSE
else skipFormat = true
StreamHandler->>Downstream: pass through pre-formatted SSE (no extra formatting)
end
Downstream->>Client: stream chunks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/response/sse-double-wrap.test.ts (1)
43-62: Enhance test coverage for SSE-enabled generators.While this test verifies that plain generators still work, it doesn't validate that generators with explicit SSE markers are still formatted correctly. Consider adding a test case that uses SSE markers (e.g., setting
content-type: text/event-streaminset.headersor using.sseproperty) and verifies the output contains proper SSE formatting withdata:prefix.Also note: the
content-typeheader on line 53 is set on the request, not the response, so it doesn't affect SSE formatting behavior.📝 Example test to add
it('should properly format SSE for generators with SSE markers', async () => { const app = new Elysia().get('/', ({ set }) => { set.headers['content-type'] = 'text/event-stream' return (function* () { yield 'hello' yield 'world' })() }) const response = await app .handle(new Request('http://localhost/')) .then((r) => r.text()) // Should be formatted with SSE data prefix expect(response).toContain('data: hello') expect(response).toContain('data: world') })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/adapter/utils.tstest/response/sse-double-wrap.test.ts
🔇 Additional comments (6)
src/adapter/utils.ts (4)
155-156: LGTM! Clean parameter addition.The optional
skipFormatparameter is well-placed and maintains backward compatibility. The naming clearly conveys its purpose.
175-185: LGTM! Correct logic to bypass SSE formatting.The
!skipFormatcheck correctly short-circuits SSE detection whenskipFormatis true, ensuring pre-formatted Response bodies are not reformatted. The existing SSE detection logic is preserved for normal use cases.
393-394: LGTM! Correct propagation of skipFormat flag.The inline comment clearly explains that pre-formatted Response bodies should not be auto-formatted. This correctly prevents double-wrapping in the status update path.
408-409: LGTM! Correct propagation in the main streaming path.This correctly prevents SSE auto-formatting for pre-formatted Response bodies in the primary streaming path. The inline comment is helpful.
test/response/sse-double-wrap.test.ts (2)
5-23: LGTM! Comprehensive test for the primary bug scenario.This test effectively validates the main fix by reproducing all three conditions from issue #1661:
set.headers,transfer-encoding: chunked, andcontent-type: text/event-stream. The assertions correctly verify both the expected output and the absence of double-wrapping.
25-41: LGTM! Tests important header-merging variation.This test covers the scenario where
content-typeis set viaset.headersrather than directly on the Response, ensuring the fix works correctly in both header-setting patterns.
- Tests that generators using sse() helper get proper SSE formatting - Verifies no double-wrapping occurs with explicit SSE markers - Addresses CodeRabbit review suggestion for comprehensive SSE test coverage
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/response/sse-double-wrap.test.ts:
- Around line 43-62: The test sets 'content-type: text/event-stream' on the
Request which doesn't influence response formatting for a GET; update the test
"should properly format SSE for generator functions" by removing the request
header in the Request(...) call (or alternatively change the test
purpose/comment to assert response-driven SSE behavior) so that the generator
route defined with Elysia().get('/', function* () { yield 'hello'; yield 'world'
}) is validated as plain text output; adjust the comment to state that request
Content-Type is irrelevant for GET responses if you keep the header.
🧹 Nitpick comments (2)
test/response/sse-double-wrap.test.ts (2)
64-66: Consider moving dynamic import to top-level.The
ssehelper is dynamically imported on line 65, which is less conventional than importing at the file level. While functionally correct, moving it to the top-level imports (line 2) would improve consistency and readability.♻️ Proposed refactor
Move the import to the top of the file:
-import { describe, it, expect } from 'bun:test' -import { Elysia } from '../../src' +import { describe, it, expect } from 'bun:test' +import { Elysia, sse } from '../../src'Then remove the dynamic import from the test:
it('should format SSE correctly for generators with explicit SSE configuration', async () => { - const { sse } = await import('../../src') - const app = new Elysia().get('/', ({ set }) => {
1-86: Consider adding response header verification for completeness.The tests thoroughly verify response body content, but don't validate that response headers are correctly set (e.g.,
content-type: text/event-stream,transfer-encoding: chunked). Adding header assertions would make the tests more comprehensive and catch potential header-related regressions.💡 Example header verification for test 1
- const response = await app.handle(new Request('http://localhost/')).then(r => r.text()) + const response = await app.handle(new Request('http://localhost/')) + + // Verify headers + expect(response.headers.get('content-type')).toBe('text/event-stream') + expect(response.headers.get('hello')).toBe('world') + + const text = await response.text() // Should NOT double-wrap with "data: data:" - expect(response).toBe('data: hello\n\ndata: world\n\n') - expect(response).not.toContain('data: data:') + expect(text).toBe('data: hello\n\ndata: world\n\n') + expect(text).not.toContain('data: data:')
- Removed content-type header from GET request (doesn't affect response) - Request Content-Type is irrelevant for GET responses - Addresses CodeRabbit review feedback
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/response/sse-double-wrap.test.ts (1)
43-56: Strengthen assertions to verify output format.The comment states "Generator without explicit SSE should format as plain text," but the assertions only check substring presence, not the actual format. This test would pass even if the output were SSE-formatted (e.g.,
"data: hello\n\n").Consider adding assertions that verify the output is truly plain text (e.g., no "data:" prefix, no double newlines) or document why checking substring presence is sufficient.
📝 Suggested assertion improvements
it('should properly format SSE for generator functions', async () => { const app = new Elysia().get('/', function* () { yield 'hello' yield 'world' }) const response = await app .handle(new Request('http://localhost/')) .then((r) => r.text()) // Generator without explicit SSE should format as plain text expect(response).toContain('hello') expect(response).toContain('world') + // Verify it's NOT SSE formatted + expect(response).not.toContain('data: hello') + expect(response).not.toContain('data: world') })Alternatively, if generators should auto-format to SSE even without explicit
sse()calls, update the comment and verify SSE format instead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/response/sse-double-wrap.test.ts
🔇 Additional comments (2)
test/response/sse-double-wrap.test.ts (2)
5-41: LGTM! Solid regression tests for the double-wrapping fix.Both tests correctly validate the core issue: pre-formatted SSE responses with
set.headersshould not be double-wrapped. The first test uses headers in the Response constructor, while the second setscontent-typeviaset.headers— good coverage of different configuration patterns.
58-79: Test logic is sound; dynamic import is acceptable.The async generator with explicit
sse()helper correctly tests that formatted SSE data doesn't get double-wrapped. The dynamic import at line 59 isolates the dependency, which is fine, though importing at the top would be more conventional.
…tted - Added negative assertions to confirm no 'data:' prefix - Ensures generator without sse() helper outputs plain text - Test now explicitly verifies what comment claims - Addresses CodeRabbit review feedback
2 participants
Description
Fixes #1661 - SSE double-wrapping bug when returning pre-formatted Response with chunked transfer encoding.
Problem
When users return a
Responsewith pre-formatted SSE data (content-type: text/event-streamandtransfer-encoding: chunked) while usingset.headers, the response gets double-wrapped with thedata:prefix:Expected:
Got:
This only occurs when all three conditions are met:
set.headers(causes routing through handleResponse)transfer-encoding: chunkedcontent-type: text/event-streamRoot Cause
The bug chain:
handleResponsemergesset.headersinto the Responsetransfer-encoding: chunkedand routes throughhandleStream(streamResponse(response))handleStreamchecks content-type and detects SSEdata: ${chunk}\n\nformatdata: data: helloSolution
Production-level architectural fix (not a workaround):
Added
skipFormatparameter tocreateStreamHandlerto distinguish between:When
handleResponsecallshandleStreamfor a Response body, it passesskipFormat=trueto prevent auto-formatting of already-formatted content.Changes Made
src/adapter/utils.tsskipFormat?: booleanparameter tocreateStreamHandlerskipFormatbefore applying SSE formattingskipFormat=truewhen streaming Response bodies (2 locations increateResponseHandler)test/response/sse-double-wrap.test.tsTesting
✅ All 1433 existing tests pass
✅ 3 new regression tests added
✅ No breaking changes
✅ Generator SSE formatting still works
✅ Pre-formatted Response streaming works
Impact
Related Issues
Introduced in v1.3.16 per user investigation in issue comments.
This is a production-ready architectural fix, not a workaround. The solution properly distinguishes between content that needs formatting vs pre-formatted content.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.