Skip to content

fix(response): keep content-length header for Uint8Array responses#1504

Merged
pi0 merged 2 commits into
h3js:mainfrom
g1mn:fix/uint8array-content-length
Jul 22, 2026
Merged

fix(response): keep content-length header for Uint8Array responses#1504
pi0 merged 2 commits into
h3js:mainfrom
g1mn:fix/uint8array-content-length

Conversation

@g1mn

@g1mn g1mn commented Jul 22, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Part of #1477 (2 of 2 — split per @pi0's request; companion to #1503).

prepareResponse clears event[kEventRes] before prepareResponseBody runs (capturing prepared headers pre-clear to avoid duplication, #1259). The Uint8Array branch then set content-length via event.res.headers.set(...), which lazily recreated a new H3EventResponse that nothing reads — so the header was dropped and event.res got repopulated. This returns the header on the prepareResponseBody result instead, so it is actually applied and event.res stays cleared.

app.get("/bin", () => new Uint8Array([1, 2, 3]));
// before: no content-length   after: content-length: 3, event.res not repopulated

Test (middleware.test.ts, describeMatrix): a Uint8Array response keeps its content-length and does not duplicate headers when passed through a second toResponse() pass. pnpm test is green.

Drafted with AI assistance; reviewed and verified against the test suite by me.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed response handling for binary data so the correct content-length is preserved.
    • Prevented stale headers from being duplicated when responses are processed multiple times.
    • Ensured existing Set-Cookie headers remain intact without duplication.

@g1mn @claude
fix(response): keep content-length header for Uint8Array responses
188bcda 
prepareResponse() clears event[kEventRes] before calling prepareResponseBody()
to avoid duplicating the response (h3js#1259), capturing preparedHeaders from the
pre-clear object. The Uint8Array branch then called
event.res.headers.set("content-length", ...), which lazily recreated a brand
new H3EventResponse post-clear that nothing reads — the header was silently
dropped, and event[kEventRes] was left repopulated, undermining the h3js#1259
anti-duplication fix for this branch.

content-length is now returned on prepareResponseBody's own result so the
existing header-merge logic in prepareResponse picks it up, and event.res is
no longer touched (so it stays cleared).

Split out of h3js#1503 per maintainer feedback (one PR per issue). Addresses the
second bug in h3js#1477; the sync-throw bug is handled separately in
fix/response-prepare-error-handling.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@g1mn
g1mn requested a review from pi0 as a code owner July 22, 2026 17:09
@g1mn g1mn mentioned this pull request Jul 22, 2026
Merged
@coderabbitai

coderabbitai Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13221f68-2b76-400d-bcf9-752d4022b46b

📥 Commits

Reviewing files that changed from the base of the PR and between 188bcda and d3e22e3.

📒 Files selected for processing (1)
  • src/response.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/response.ts
📝 Walkthrough

Walkthrough

The Uint8Array response path now returns content-length through prepared headers instead of mutating staged response headers. Middleware regression coverage verifies byte length and preservation of existing Set-Cookie headers across repeated response processing.

Changes

Response header preservation

Layer / File(s) Summary
Prepared response headers
src/response.ts		 Uint8Array responses include a new Headers object containing content-length derived from the byte length.
Middleware regression validation
test/middleware.test.ts		 The regression test verifies correct content-length and non-duplicated Set-Cookie headers with onResponse() processing.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • h3js/h3 issue 1477 — The change directly covers Uint8Array content-length loss and stale response-header mutation.

Suggested reviewers: pi0

Poem

A bunny packed bytes in a neat little heap,
While headers stayed tidy instead of asleep.
Cookies hopped once, not twice in the queue,
Length matched every byte as it should do.
“Hop to the tests!” said the rabbit with cheer.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: preserving content-length for Uint8Array responses.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pi0
Merge remote-tracking branch 'origin/main' into pr-1504/fix/uint8arra…
d3e22e3 
…y-content-length

# Conflicts:
#	test/bench/bundle.test.ts
@pi0
pi0 merged commit 5cba776 into h3js:main Jul 22, 2026
7 checks passed
@g1mn g1mn mentioned this pull request Jul 22, 2026
Closed
@g1mn
g1mn deleted the fix/uint8array-content-length branch July 22, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pi0 pi0 pi0 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@g1mn @pi0