Fix thenable fallback in mapCompactResponse causing runtime crash with undefined variable #1662
Conversation
…riable - Replace mapResponse(x, set) with mapCompactResponse(x, request) in thenable handler - Fixes runtime crash when non-Promise thenables are returned (e.g., from ORMs like Drizzle) - Applied fix to both web-standard and bun adapters - Added regression test for custom thenable objects
WalkthroughPromise and thenable resolution in compact response paths now recursively use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @test/response/sse-double-wrap.test.ts:
- Line 27: The test assigns set.headers['x-custom'] = 'test' but never asserts
it; either remove that assignment or add an assertion checking the header on the
response. Locate the assignment in sse-double-wrap.test.ts (the line with
set.headers['x-custom'] = 'test') and either delete it if irrelevant, or add an
assertion that the response.headers['x-custom'] (or the framework-specific
response header accessor used in this test) equals 'test' so the assignment is
actually verified.
- Around line 43-62: The test currently misuses the Request header and conflicts
with its comment; decide to test SSE output: change the request header from
'content-type: text/event-stream' to 'Accept: text/event-stream' so the handler
sees the client wants SSE, and update the assertions for the Elysia route (the
generator-based handler) to verify SSE formatting by checking for 'data: hello'
and 'data: world' (instead of just 'hello'/'world'); alternatively, if you
prefer plain-text behavior, remove the header entirely and assert the response
does NOT contain 'data:' to confirm non-SSE formatting.
- Line 7: The test assigns set.headers.hello = 'world' but never asserts it;
either remove that unused assignment or add an assertion that the header is
present and equals "world" (e.g., assert on the response headers object or the
variable representing the response in this test) so the assignment is validated;
locate the assignment to set.headers.hello and either delete it or add a
corresponding expect/assert against response.headers['hello'] (or the test's
response variable) to confirm the header value.
🧹 Nitpick comments (1)
src/adapter/utils.ts (1)
175-185: Consider clarifying the comment.The logic correctly implements the skipFormat behavior. However, the comment on line 175 says "Check if stream is from a pre-formatted Response body," but the logic checks multiple SSE indicators beyond just pre-formatted responses (e.g.,
sse()wrapped values, generator.sse, content-type header).📝 Suggested comment improvement
- // Check if stream is from a pre-formatted Response body + // Determine if SSE formatting should be applied (skip if already pre-formatted) const isSSE = !skipFormat &&
📜 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 (3)
src/adapter/utils.ts (3)
155-156: LGTM: Clean parameter addition.The optional
skipFormatparameter is well-positioned and maintains backwards compatibility.
393-394: LGTM: Correct usage of skipFormat flag.Passing
skipFormat: truewhen streaming a pre-formatted Response body correctly prevents double-wrapping. The inline comment clearly explains the intent.
408-409: LGTM: Consistent skipFormat usage.Correctly applies the same skipFormat pattern as the earlier call site, ensuring consistent behavior when streaming pre-formatted Response bodies.
| describe('SSE - Response Double Wrapping', () => { | ||
| it('should not double-wrap SSE data when returning pre-formatted Response', async () => { | ||
| const app = new Elysia().get('/', ({ set }) => { | ||
| set.headers.hello = 'world' |
There was a problem hiding this comment.
Verify or remove unused header assignment.
set.headers.hello = 'world' is assigned but never verified in the test assertions. Either add an assertion to verify this header is present in the response, or remove the assignment if it's not relevant to the test.
✅ Proposed fix to verify the header
const response = await app.handle(new Request('http://localhost/')).then(r => r.text())
+ const headers = await app.handle(new Request('http://localhost/')).then(r => r.headers)
// Should NOT double-wrap with "data: data:"
expect(response).toBe('data: hello\n\ndata: world\n\n')
expect(response).not.toContain('data: data:')
+ expect(headers.get('hello')).toBe('world')Or remove the unused assignment:
const app = new Elysia().get('/', ({ set }) => {
- set.headers.hello = 'world'
-
return new Response('data: hello\n\ndata: world\n\n', {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @test/response/sse-double-wrap.test.ts at line 7, The test assigns
set.headers.hello = 'world' but never asserts it; either remove that unused
assignment or add an assertion that the header is present and equals "world"
(e.g., assert on the response headers object or the variable representing the
response in this test) so the assignment is validated; locate the assignment to
set.headers.hello and either delete it or add a corresponding expect/assert
against response.headers['hello'] (or the test's response variable) to confirm
the header value.
|
|
||
| it('should not double-wrap SSE when using set.headers with pre-formatted content', async () => { | ||
| const app = new Elysia().get('/', ({ set }) => { | ||
| set.headers['x-custom'] = 'test' |
There was a problem hiding this comment.
Verify or remove unused header assignment.
set.headers['x-custom'] = 'test' is assigned but never verified in the test assertions. Either add an assertion to verify this header is present in the response, or remove the assignment if it's not relevant to the test.
✅ Proposed fix to verify the header
const response = await app.handle(new Request('http://localhost/')).then(r => r.text())
+ const headers = await app.handle(new Request('http://localhost/')).then(r => r.headers)
expect(response).toBe('data: message1\n\ndata: message2\n\n')
expect(response).not.toContain('data: data:')
+ expect(headers.get('x-custom')).toBe('test')Or remove the unused assignment:
const app = new Elysia().get('/', ({ set }) => {
- set.headers['x-custom'] = 'test'
set.headers['content-type'] = 'text/event-stream'Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @test/response/sse-double-wrap.test.ts at line 27, The test assigns
set.headers['x-custom'] = 'test' but never asserts it; either remove that
assignment or add an assertion checking the header on the response. Locate the
assignment in sse-double-wrap.test.ts (the line with set.headers['x-custom'] =
'test') and either delete it if irrelevant, or add an assertion that the
response.headers['x-custom'] (or the framework-specific response header accessor
used in this test) equals 'test' so the assignment is actually verified.
| 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/', { | ||
| headers: { | ||
| 'content-type': 'text/event-stream' | ||
| } | ||
| }) | ||
| ) | ||
| .then((r) => r.text()) | ||
|
|
||
| // Generator without explicit SSE should format as plain text | ||
| expect(response).toContain('hello') | ||
| expect(response).toContain('world') | ||
| }) |
There was a problem hiding this comment.
Clarify test intent and fix request header usage.
This test has several issues:
-
Incorrect request header usage (lines 52-54): Setting
content-type: text/event-streamin the REQUEST headers for a GET request doesn't make semantic sense. Content-Type in requests typically indicates the body type for POST/PUT/PATCH. -
Contradictory comment (line 59): The comment states "Generator without explicit SSE should format as plain text," but the request explicitly sets content-type to text/event-stream.
-
Incomplete assertions (lines 60-61): The test only verifies the response contains 'hello' and 'world', but doesn't validate the actual format. It should either:
- Check for SSE format:
expect(response).toContain('data: hello')if testing SSE - Check for plain text:
expect(response).not.toContain('data:')if testing non-SSE
- Check for SSE format:
🔧 Proposed fix to clarify test intent
If testing plain text output from a generator:
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/', {
- headers: {
- 'content-type': 'text/event-stream'
- }
- })
- )
- .then((r) => r.text())
+ 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')
+ expect(response).not.toContain('data:')If testing SSE output, remove the confusing request header and explicitly configure SSE in the route handler.
🤖 Prompt for AI Agents
In @test/response/sse-double-wrap.test.ts around lines 43 - 62, The test
currently misuses the Request header and conflicts with its comment; decide to
test SSE output: change the request header from 'content-type:
text/event-stream' to 'Accept: text/event-stream' so the handler sees the client
wants SSE, and update the assertions for the Elysia route (the generator-based
handler) to verify SSE formatting by checking for 'data: hello' and 'data:
world' (instead of just 'hello'/'world'); alternatively, if you prefer
plain-text behavior, remove the header entirely and assert the response does NOT
contain 'data:' to confirm non-SSE formatting.
raunak-rpm
force-pushed
the
feature/my-contribution
branch
from
03ce9ef to
fb93ce6
Compare
2 participants
Fix thenable fallback in mapCompactResponse causing runtime crash with undefined variable
🐛 Problem Description
Critical Bug in Response Mapping
The
mapCompactResponsefunction in both adapter implementations contains a critical bug in its thenable fallback handler that causes immediate runtime crashes when custom thenable objects are returned from route handlers.Affected Files (3 files changed):
src/adapter/web-standard/handler.ts(line 539) - Fixed thenable handlersrc/adapter/bun/handler.ts(line 506) - Fixed thenable handlertest/adapter/web-standard/map-compact-response.test.ts(lines 207-229) - Added regression testProblematic Code:
Root Cause Analysis
The thenable fallback branch attempts to call
mapResponse(x, set), but thesetvariable does not exist in the current scope ofmapCompactResponse. This causes:ReferenceError: set is not definedWhen Does This Bug Occur?
This bug manifests when route handlers return custom thenable objects (objects with a
.then()method that aren't native Promises):Common Real-World Scenarios:
Example Crash Scenario:
📊 Visual Representation
Before Fix (❌ Crashes)
Function Scope at Crash Point:
After Fix (✅ Works Correctly)
Function Scope After Fix:
Complete Flow Comparison
❌ OLD CODE - Crashes on Thenables
✅ NEW CODE - Handles Thenables Correctly
Side-by-Side Code Comparison
Problems:
setdoesn't exist in scopemapResponserequiressetparameterBenefits:
requestis available in scopemapCompactResponsematches signatureReal-World Example Flow
Example: Drizzle ORM Query
What Happens Internally:
Architecture Diagram
✅ Solution
The Fix
Changed the thenable handler to recursively use
mapCompactResponseinstead ofmapResponse:Why This Solution Is Correct
requestis in scope: Unlikeset, therequestparameter is available inmapCompactResponseWhy NOT
mapResponse?Using
mapResponsewould be incorrect even ifsetwere available because:mapCompactResponseis optimized for cases where context manipulation isn't neededmapResponsewould break the compact path optimization📝 Changes Made
Code Changes (3 files)
src/adapter/web-standard/handler.ts(line 539)mapResponse(x, set)→mapCompactResponse(x, request)src/adapter/bun/handler.ts(line 506)mapResponse(x, set)→mapCompactResponse(x, request)test/adapter/web-standard/map-compact-response.test.ts(lines 213-231)Test Implementation
New Test Case:
🧪 Testing & Validation
Automated Tests
Manual Verification
Created comprehensive verification script testing:
set.headerscontextVerification Results:
No Breaking Changes
📊 Impact Assessment
Severity
🔴 Critical - Causes immediate runtime crashes
Scope
Who is affected:
When it occurs:
Risk Level
🟢 Low Risk - Fix is safe to merge
Why low risk:
Performance Impact
Neutral - No performance regression
🔍 Technical Deep Dive
Function Scope Analysis
mapCompactResponsesignature:Available variables in scope:
response- the value being mappedrequest- optional Request objectset- NOT available (not in parameters)Why
setwas referenced:Likely a copy-paste error from other mapping functions like
mapResponseormapEarlyResponse, which DO havesetin their scope:Response Mapping Architecture
Elysia uses different mapping strategies:
mapCompactResponse: Lightweight, minimal overhead (no context needed)mapResponse: Full context mapping (needssetfor headers, status, cookies)mapEarlyResponse: Early exit optimizationThe thenable handler in
mapCompactResponseshould maintain the "compact" strategy by recursively calling itself, not switching tomapResponse.🎯 Related Information
What is a Thenable?
A thenable is any object that implements a
.then()method, making it Promise-like:Examples in the wild:
Why This Matters for Elysia
Elysia is designed to be flexible and work with various async patterns. Supporting thenables (not just native Promises) is crucial for:
✨ Summary
This PR fixes a critical runtime crash in the thenable fallback handler by:
set→request)mapResponse→mapCompactResponse)Result: Applications using ORMs or libraries with custom thenables will no longer crash, and the fix follows Elysia's architectural patterns correctly.