bug fix: set content-type header based on response type in mapResponse and mapEarlyResponse#1766
bug fix: set content-type header based on response type in mapResponse and mapEarlyResponse#1766KnextKoder wants to merge 3 commits intoelysiajs:mainfrom
Conversation
… in mapResponse and mapEarlyResponse
… in mapResponse and mapEarlyResponse
… in mapResponse and mapEarlyResponse
WalkthroughUpdated Content-Type header assignment logic in Bun and web-standard adapters to conditionally set Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/adapter/web-standard/handler.ts (1)
182-186: Consider using lowercasecontent-typefor consistency.The header key casing differs between the pre-handleSet code (lowercase
'content-type'at line 63) and this fallback path (capitalized'Content-Type'). While HTTP headers are case-insensitive, using consistent casing improves code clarity. This is a minor nit.♻️ Suggested change for consistency
if (set.headers instanceof Headers) { - if (!set.headers.has('Content-Type')) - set.headers.set('Content-Type', 'application/json') -} else if (!set.headers['Content-Type']) - set.headers['Content-Type'] = 'application/json' + if (!set.headers.has('content-type')) + set.headers.set('content-type', 'application/json') +} else if (!set.headers['content-type']) + set.headers['content-type'] = 'application/json'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapter/web-standard/handler.ts` around lines 182 - 186, The header key casing is inconsistent: in the fallback branch you set 'Content-Type' while elsewhere you use lowercase 'content-type'; update the fallback to use lowercase 'content-type' for consistency by checking and setting set.headers (both when it's a Headers instance and when it's a plain object) using 'content-type' instead of 'Content-Type' so the code paths match the pre-handleSet usage of lowercase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/adapter/web-standard/handler.ts`:
- Around line 182-186: The header key casing is inconsistent: in the fallback
branch you set 'Content-Type' while elsewhere you use lowercase 'content-type';
update the fallback to use lowercase 'content-type' for consistency by checking
and setting set.headers (both when it's a Headers instance and when it's a plain
object) using 'content-type' instead of 'Content-Type' so the code paths match
the pre-handleSet usage of lowercase.
There was a problem hiding this comment.
Pull request overview
Fixes a bug where setting multiple cookies could prevent Content-Type from being written correctly (notably for JSON responses) by ensuring Content-Type is determined/set before cookie handling can convert set.headers into a Headers instance.
Changes:
- Pre-sets
content-typeinmapResponse/mapEarlyResponse(web-standard + bun) before callinghandleSet(set)so cookie parsing can’t blockContent-Typewrites. - Updates JSON “string looks like JSON” fallback to set
Content-Typecorrectly whenset.headersis aHeadersinstance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/adapter/web-standard/handler.ts | Pre-sets content-type before handleSet; adds Headers-aware Content-Type setting in JSON-detection branch. |
| src/adapter/bun/handler.ts | Pre-sets JSON content-type before handleSet; adds Headers-aware Content-Type setting in JSON-detection branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (response?.constructor?.name) { | ||
| case 'String': | ||
| set.headers['content-type'] = 'text/plain' | ||
| break | ||
| case 'Array': | ||
| case 'Object': | ||
| set.headers['content-type'] = 'application/json' | ||
| break | ||
| } |
There was a problem hiding this comment.
Setting set.headers['content-type'] here assumes set.headers is a plain object. However set.headers can already be a Headers instance in this codepath (e.g. if handleSet was called earlier, or after the ElysiaCustomStatusResponse branch recurses with a set whose headers were converted by handleSet). In that case, index assignment won’t write the header and the Content-Type fix won’t apply (and the post-handleSet assignment was removed). Update this block to support both Headers and plain objects (e.g. use set.headers.set('content-type', ...) when set.headers instanceof Headers).
| switch (response?.constructor?.name) { | ||
| case 'String': | ||
| set.headers['content-type'] = 'text/plain' | ||
| break | ||
| case 'Array': | ||
| case 'Object': | ||
| set.headers['content-type'] = 'application/json' | ||
| break | ||
| } |
There was a problem hiding this comment.
Same issue as mapResponse: this pre-handleSet set.headers['content-type'] write will fail if set.headers is already a Headers instance (possible when mapEarlyResponse is called with a set that has already passed through handleSet elsewhere). Since the post-handleSet content-type assignment was removed, this can leave JSON responses without Content-Type. Handle both Headers and plain header objects when setting this value.
| ) | ||
| set.headers['content-type'] = 'application/json' |
There was a problem hiding this comment.
This new pre-handleSet assignment uses index-based writes (set.headers['content-type'] = ...). set.headers can be a Headers instance in some call paths (for example when handleSet already ran before calling mapResponse, or after recursive mapResponse calls using the same set), in which case this write is ignored and JSON responses won’t get Content-Type. Update to set the header via Headers.set(...) when set.headers instanceof Headers (or use a helper that supports both shapes).
| ) | |
| set.headers['content-type'] = 'application/json' | |
| ) { | |
| if (set.headers instanceof Headers) | |
| set.headers.set('content-type', 'application/json') | |
| else (set.headers as any)['content-type'] = 'application/json' | |
| } |
| ) | ||
| set.headers['content-type'] = 'application/json' |
There was a problem hiding this comment.
Same as mapResponse: this pre-handleSet set.headers['content-type'] write assumes set.headers is a plain object. If set.headers is already a Headers instance, the assignment won’t apply and JSON responses can miss the Content-Type header. Handle the Headers case explicitly (e.g. set.headers.set('content-type', 'application/json')).
| ) | |
| set.headers['content-type'] = 'application/json' | |
| ) { | |
| if (set.headers instanceof Headers) { | |
| set.headers.set('content-type', 'application/json') | |
| } else { | |
| ;(set.headers as any)['content-type'] = 'application/json' | |
| } | |
| } |
Bug #1379
When a route handler sets two or more cookies, the content type header of the response silently becomes
text/plaininstead ofapplication/json, the bug is caused by thehandleSetfunc insrc/adapter/utils.ts.Here, when the result is an array,
parseSetCookies()replacesset.headerswith a Headers instance so that multiple set-cookie entries can be added correctly, but Headers doesn't support index based property assignment, so noContent-Typeis ever written to the response. My fix is to preset the content-type onset.headersbefore thehandleSetfunc is called, when it is guaranteed to still be a plain object. BothmapResponseandmapEarlyResponsefunctions in both the bun and web standard adapters are patched.All existing tests continue to pass.
Plus some throwaway test I used to confirm these behaviors also passed
Content-Type: application/json.value =assignment syntaxContent-Type: application/jsonContent-Type: application/jsonContent-Type: application/jsonmapEarlyResponsepathContent-Type: application/jsonapplication/jsonSummary by CodeRabbit
Bug Fixes