fix(response): strip HEAD body when merging prepared headers into a mutable Response#1490
Conversation
…utable Response prepareResponse stripped the body of a HEAD (and other null-body) response on the fast path and the immutable-headers catch path, but the in-place header merge path returned the mutable Response with its body intact. app.fetch / web-mode / service-worker consumers then saw a HEAD response carrying a body (Node strips it at the socket, so it only surfaced off-Node). Merge the prepared headers first, then strip the HEAD body in a single place that also covers the in-place merge path. This dedupes the HEAD handling and keeps the bundle size unchanged. Ref h3js#1482
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesHEAD response behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/response.ts`:
- Around line 155-164: Update the successful in-place merge return path around
the HEAD-specific body stripping to use the existing nullBody logic, so 204,
304, HEAD, and all other null-body statuses consistently return a null body.
Preserve the current response metadata and return behavior for statuses that
allow bodies.
🪄 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: e03ce510-508d-4326-846c-c1a38c99a7e3
📒 Files selected for processing (2)
src/response.tstest/app.test.ts
2 participants
Addresses the "HEAD body stripping misses the in-place merge path" item from #1482.
prepareResponsestrips the body of a HEAD (or otherwise null-body) response on the fast path (!preparedHeaders) and on the immutable-headers catch path, but the in-place header merge path returned the mutableResponsewith its body intact. So when prepared headers are merged into a mutable 2xxResponseunder a HEAD request,app.fetch/ web-mode / service-worker consumers received a HEAD response carrying a body. Node strips it at the socket, so it only surfaced off-Node.This merges the prepared headers first, then strips the HEAD body in a single place that also covers the in-place merge path, deduping the HEAD handling that was previously split across two branches. Bundle size is unchanged (the guards in
test/bench/bundle.test.tsstill pass).Added a regression test in
test/app.test.ts: it sets a prepared header viaevent.res.headers, returns a mutableResponsewith a body under a HEAD request, and asserts the merged headers are present while the body is empty. It fails on thewebmatrix mode before this change and passes after; thenodemode passes either way since the runtime strips the body at the socket.Summary by CodeRabbit
HEADresponses so the body is reliably empty in all cases, while response headers remain intact.HEADresponses include the merged headers but an empty response body.