fix(start-server-core): use headers.entries() to iterate headers correctly #5537
Conversation
…ectly The previous code used Object.entries(headers) which doesn't work on Headers objects from the Web API, resulting in no headers being set at all. Changed to use headers.entries() which correctly iterates through the Headers object and properly handles multiple headers with the same name. - Fixed setResponseHeaders to use headers.entries() instead of Object.entries() - Added logic to use .set() for first occurrence, .append() for duplicates - Added test for multiple headers with the same name
WalkthroughRefactors header application to iterate Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as requestHandler
participant Setter as setResponseHeaders
participant Resp as H3 Response
Handler->>Setter: provide Headers object
note right of Setter#lightblue: iterate headers.entries()\ntrack addedHeaderNames
loop for each header entry
alt header name not seen
Setter->>Resp: set(headerName, value)
Setter->>Setter: addedHeaderNames.add(headerName)
else header name seen
Setter->>Resp: append(headerName, value)
end
end
Setter-->>Handler: headers applied
Handler->>Resp: return Response('OK')
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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
🧹 Nitpick comments (1)
packages/start-server-core/src/request-response.ts (1)
139-149: Excellent fix for the Headers iteration bug!The change from
Object.entries(headers)toheaders.entries()correctly addresses the issue where no headers were being set. The deduplication logic properly handles multiple headers with the same name (e.g., multiple Set-Cookie headers) by using.set()for the first occurrence and.append()for subsequent ones.For stricter type safety in TypeScript strict mode, consider making the method selection more explicit:
const addedHeaderNames: Record<string, true> = {} for (const [name, value] of headers.entries()) { - const found = addedHeaderNames[name] ?? false - if (!found) { - addedHeaderNames[name] = true - } - // If header already existed in h3 event headers, it will be replaced. - // However, headers object in this invocation might have multiple instances of the same header name (.append() was used), let's allow the duplicates. - const method = found ? 'append' : 'set' - event.res.headers[method](name, value) + // If header already existed in h3 event headers, it will be replaced. + // However, headers object in this invocation might have multiple instances of the same header name (.append() was used), let's allow the duplicates. + if (addedHeaderNames[name]) { + event.res.headers.append(name, value) + } else { + addedHeaderNames[name] = true + event.res.headers.set(name, value) + } }This eliminates the bracket notation call and makes the intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-server-core/src/request-response.ts(1 hunks)packages/start-server-core/tests/request-response.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-server-core/tests/request-response.test.tspackages/start-server-core/src/request-response.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-server-core/tests/request-response.test.tspackages/start-server-core/src/request-response.ts
🧬 Code graph analysis (1)
packages/start-server-core/tests/request-response.test.ts (1)
packages/start-server-core/src/request-response.ts (4)
requestHandler(49-60)setResponseHeaders(135-150)getResponseHeaders(152-155)getResponseHeader(157-162)
🔇 Additional comments (2)
packages/start-server-core/tests/request-response.test.ts (2)
10-81: Excellent test coverage for basic header operations!These tests thoroughly validate single headers, multiple different headers, empty headers, and header replacement scenarios. The test structure is clear and assertions are appropriate.
96-101: Test assertions are appropriately flexible.The test correctly validates that both cookie values are present without making assumptions about the exact format returned by
getResponseHeader(). This approach ensures the test remains valid regardless of whether the H3 library returns comma-separated values or handles Set-Cookie specially.
rhyek
changed the title
|
thanks for the PR, we are currently reworking header handling, so either we will merge this PR or apply similar changes. let's keep this open until we are ready |
Ah, that makes sense. Was kind of hoping to get my contributor badge, but np! 😆 |
|
FYI, just getting the current headers object using Maybe tanstack should just rely on the user doing that. |
2 participants
Summary
This PR fixes a critical bug in setResponseHeaders() where no headers were being set at all.
The Bug
The previous code used Object.entries(headers) to iterate over a Headers object:
Problem: Object.entries() doesn't work on Headers objects (Web API interface). It returns an empty array, so no headers were being set.
The Fix
Changed to use the correct Web API method headers.entries():
Benefits:
Test Coverage
Added comprehensive tests in packages/start-server-core/tests/request-response.test.ts:
All tests pass ✓
Summary by CodeRabbit
Bug Fixes
Tests