fix: empty-body status codes send body when using string status names#1833
fix: empty-body status codes send body when using string status names#1833alexkahndev wants to merge 2 commits intoelysiajs:mainfrom
Conversation
… using string status names
ElysiaCustomStatusResponse constructor checked `code in emptyHttpStatus` but `code` is the
string name (e.g. 'No Content') while emptyHttpStatus keys are numeric (204). Changed to
check `this.code` which is already resolved to the numeric status via StatusMap.
Also changed mapResponse undefined case to use `new Response(null, set)` instead of
`new Response('', set)` as HTTP spec requires no body for 204/205 responses.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughUpdated response mapping in Bun and Web handlers to return Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/response/no-content-status.test.ts (1)
5-43: Heee~ the tests look nice, buuut~ ( ̄∇ ̄)Your
describeblock is called'204 No Content'but you snuck in a test for205 Reset Contenttoo~! That's a bit misleading, don't you think, silly~? ♡Also,
emptyHttpStatusincludes other codes like304 Not Modified- maybe you could add coverage for that too someday~? But I gueeess this is enough for now... (´-ω-`)♻️ Suggestion for cleaner test organization~
-describe('204 No Content', () => { +describe('Empty Body Status Codes', () => { it('should return null body for status No Content', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/response/no-content-status.test.ts` around lines 5 - 43, The describe block label "'204 No Content'" is misleading because it also contains a test for the 205 "Reset Content" case (the test that defines a POST '/reset' route and calls status('Reset Content')); rename the describe to something generic like "'Empty HTTP statuses'" or split the 205 test into its own describe (e.g., "'205 Reset Content'") so the suite name matches its contents; update the describe string that wraps the tests accordingly to reflect either multiple empty statuses or move the test with status('Reset Content') into a separate describe block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapter/bun/handler.ts`:
- Line 59: The two functions mapEarlyResponse and mapCompactResponse currently
return new Response('') for falsy responses; make them consistent with
mapResponse by returning new Response(null, set as any) instead (i.e., check for
!response and return new Response(null, set as any)). Update the falsy-response
branches in mapEarlyResponse and mapCompactResponse to match the same return
pattern used by mapResponse.
In `@src/adapter/web-standard/handler.ts`:
- Line 93: Update the two remaining spots that still create an empty-body
Response with a string: in mapEarlyResponse and mapCompactResponse replace any
occurrences of new Response('') (or returning a falsy response as an empty
string body) with the same HTTP-spec-compliant pattern used at the earlier
change—return new Response(null, set as any) (or construct the Response with
null as the body and the appropriate init/options) and ensure any type
assertions match the existing handler pattern so the compiler is satisfied.
---
Nitpick comments:
In `@test/response/no-content-status.test.ts`:
- Around line 5-43: The describe block label "'204 No Content'" is misleading
because it also contains a test for the 205 "Reset Content" case (the test that
defines a POST '/reset' route and calls status('Reset Content')); rename the
describe to something generic like "'Empty HTTP statuses'" or split the 205 test
into its own describe (e.g., "'205 Reset Content'") so the suite name matches
its contents; update the describe string that wraps the tests accordingly to
reflect either multiple empty statuses or move the test with status('Reset
Content') into a separate describe block.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72d9b26b-145b-4700-8e70-739dbabe7db8
📒 Files selected for processing (4)
src/adapter/bun/handler.tssrc/adapter/web-standard/handler.tssrc/error.tstest/response/no-content-status.test.ts
…sistency
Updated all remaining `new Response('')` falsy-response branches to
`new Response(null)` in both bun and web-standard adapters, matching
the pattern already used in mapResponse.
Renamed test describe block to 'Empty HTTP statuses' to reflect
coverage of both 204 and 205.
Summary
ElysiaCustomStatusResponseconstructor checkscode in emptyHttpStatus, but when using string status names likestatus('No Content'),codeis'No Content'whileemptyHttpStatuskeys are numeric (204). The lookup always fails, sothis.responseis never cleared — resulting in a response body being sent with 204/205/304/etc.this.code in emptyHttpStatusinstead, sincethis.codeis already resolved to the numeric status viaStatusMap.mapResponse'scase undefinedto usenew Response(null, set)instead ofnew Response('', set)in both the bun and web-standard adapters, since HTTP spec requires no body for these status codes and an empty string still produces aReadableStreambody.Test plan
test/response/no-content-status.test.tswith 3 tests covering 204 (no args), 204 (with message arg), and 205Summary by CodeRabbit
Bug Fixes
Tests