🔧 fix: merge set.headers without duplicating Response

[truehazker]

Fixes #1590

Problem

When a handler returns a Response with custom headers, set.headers values were appended using headers.append(), causing duplicate header values:

.onRequest(({ set }) => {
    set.headers['Content-Type'] = 'application/json'
})
.get('/', () => new Response('data', {
    headers: { 'Content-Type': 'text/plain' }
}))

// Result: "text/plain, application/json" ❌

Solution

• Use headers.set() instead of headers.append() for regular headers
• Only add header from set.headers if Response doesn't already have it (Response takes precedence)
• Keep append() for set-cookie (multiple cookies are valid)

Changes

• src/adapter/utils.ts: Updated createResponseHandler() to check response.headers.has(key) before setting
• test/response/custom-response.test.ts: Added test for header merge behavior

Summary by CodeRabbit

• Bug Fixes
  • Fixed header merging to ensure response headers correctly take precedence over configured defaults throughout the system
  • Non-conflicting headers from configuration are now properly merged alongside response headers
  • Cookie headers are handled appropriately in all merging scenarios
  • Improved header precedence logic prevents unintended overwrites and ensures consistent behavior across different header types

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR fixes header merging behavior in createResponseHandler to prevent duplicate header values. Response headers now take precedence over set.headers for conflicting keys, while non-conflicting headers are preserved during merge operations.

Changes

Cohort / File(s)	Summary
Header Merging Logic src/adapter/utils.ts	Modified createResponseHandler to conditionally merge headers: for Headers instances, non-cookie headers are only added if absent on response; for plain objects, non-cookie headers use set with guard. Cookie headers remain in dedicated path.
Header Merge Test test/response/custom-response.test.ts	Added test case verifying Response headers take precedence over set.headers for conflicting keys while non-conflicting headers are merged together.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Focus areas:
  • Verify the header merging logic correctly handles both Headers instances and plain objects
  • Confirm Response header precedence is applied consistently across both branches
  • Check that cookie header handling remains unchanged and separate
  • Validate test assertions cover the core scenarios (precedence, non-conflicting merge, preservation)

Poem

🐰 Headers were tangled, now neat as can be,
Response takes the lead, as it should be,
No more duplicates causing a fray,
Merge conflicts solved in a proper way! 🌿

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: merge set.headers without duplicating Response' clearly summarizes the main change—fixing header merging to avoid duplicates.
Linked Issues check	✅ Passed	The PR successfully addresses all coding objectives from issue #1590: uses set() instead of append() for regular headers, checks response.headers.has(key) for precedence, keeps append() for cookies, and includes comprehensive test coverage.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing header merging behavior in createResponseHandler() and adding corresponding test coverage, with no unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

test/response/custom-response.test.ts (1)

40-63: LGTM! Test correctly validates the header merge behavior.

The test clearly verifies that Response headers take precedence over set.headers for conflicting keys (Content-Type), while non-conflicting headers from set.headers (X-Framework) are properly merged. This effectively covers the fix for issue #1590.

Optional: Consider adding edge case tests.

While the current test is solid, you might consider adding tests for:

1. Verifying behavior when set.headers is a Headers instance (not just a plain object)
2. Multiple set-cookie headers to ensure they're all appended correctly
3. Case-insensitive header name handling (e.g., content-type vs Content-Type)

These would strengthen test coverage but are not critical given the main scenario is well-tested.

src/adapter/utils.ts (1)

341-353: Correct implementation of header precedence for Headers instance.

The logic properly ensures Response headers take precedence by checking !response.headers.has(key) before merging headers from set.headers. The use of set() instead of append() prevents duplicate header values, which was the root cause of issue #1590.

Cookie handling is correct: using getSetCookie() and append() allows multiple set-cookie headers as intended.

Minor: Optional chaining is unnecessary.

At line 352, set.headers?.get(key) uses optional chaining, but set.headers is guaranteed to be a Headers instance per the condition at line 342. While harmless, you could simplify to set.headers.get(key) ?? ''.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7085776 and 2fce4b7.

📒 Files selected for processing (2)

• src/adapter/utils.ts (2 hunks)
• test/response/custom-response.test.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

test/response/custom-response.test.ts (1)

test/utils.ts (1)

• req (1-2)

src/adapter/utils.ts (1)

src/cookies.ts (1)

• set (310-320)

🔇 Additional comments (2)

src/adapter/utils.ts (2)

354-365: Correct implementation of header precedence for plain object.

This branch mirrors the Headers instance logic: set-cookie headers are always appended (lines 356-360), while other headers are only added if not already present on the Response (lines 361-365). The use of set() with the !response.headers.has(key) guard prevents the duplicate header issue.

The case-insensitive nature of the Headers API ensures proper matching even when plain object keys have different casing than Response headers.

---

341-365: Implementation successfully fixes the header duplication issue.

The changes correctly implement the merge strategy where Response headers take precedence and set.headers only fills in non-conflicting headers. This resolves issue #1590 where headers like Content-Type were being combined (e.g., "text/plain, application/json").

Key improvements:

1. Added !response.headers.has(key) checks before merging headers
2. Changed from append() to set() for non-cookie headers
3. Preserved append() for set-cookie to allow multiple cookie values
4. Consistent behavior across both branches (Headers instance and plain object)

The implementation maintains backward compatibility with existing code while fixing the reported bug.