fix(start-server-core): bump h3 from v2.0.1-rc.2 to v2.0.1-rc.6 #6161
Conversation
WalkthroughUpdates h3-v2 dependency and adds internal cookie/header utilities that merge Set-Cookie from the event into handler responses, with promise-aware attachment so both synchronous and asynchronous handler results receive merged headers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit bc6b53f
☁️ Nx Cloud last updated this comment at |
birkskyum
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/start-server-core/src/request-response.ts (1)
91-98: Consider simplifying the header manipulation logic.The code deletes all Set-Cookie headers from the response and then re-appends both the response's original cookies and the event's cookies. Since the response cookies are being re-added in the same order, the deletion step is redundant. You could simplify by just appending the event cookies directly.
🔎 Simplified approach
const responseSetCookies = getSetCookieValues(response.headers) - response.headers.delete('set-cookie') - for (const cookie of responseSetCookies) { - response.headers.append('set-cookie', cookie) - } for (const cookie of eventSetCookies) { response.headers.append('set-cookie', cookie) }Note: This assumes the intended behavior is to append event cookies after response cookies. If a different ordering is required, the current approach would be necessary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/start-server-core/package.json(1 hunks)packages/start-server-core/src/request-response.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-server-core/src/request-response.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/request-response.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace protocol
workspace:*for internal dependencies in package.json files
Files:
packages/start-server-core/package.json
🧠 Learnings (2)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-server-core/package.json
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/package.json : Use workspace protocol `workspace:*` for internal dependencies in package.json files
Applied to files:
packages/start-server-core/package.json
🔇 Additional comments (7)
packages/start-server-core/src/request-response.ts (6)
62-64: LGTM!The type correctly models the optional
getSetCookie()method that may be available on Headers objects in some runtime environments.
66-70: LGTM!The
MaybePromiseutility type andisPromiseLiketype guard are correctly implemented and follow TypeScript best practices.
72-79: LGTM!The function correctly prefers
getSetCookie()when available and falls back toheaders.get(). The fallback may only retrieve a single Set-Cookie value if multiple exist andgetSetCookie()is unavailable, but this is a known limitation of the Headers API.
101-119: LGTM with a note on error handling.The function correctly handles both synchronous and asynchronous handler results. The logic properly checks for Response instances before merging headers.
Note: If
mergeEventResponseHeadersthrows an error in the Promise chain, the Promise will reject. This is likely acceptable sinceh3_toResponsewould need to handle such errors anyway, but consider whether explicit error handling would be beneficial.
121-132: LGTM!The integration of
attachResponseHeaderscorrectly wraps the handler result before converting it to a Response viah3_toResponse. This enables automatic propagation of Set-Cookie headers from the event context to response objects for both synchronous and asynchronous handlers.
81-84: Clarify why event-context cookies are excluded from successful responses and simplify the header merge logic.The
if (response.ok) returncheck at lines 82-83 is intentional for handling non-2xx responses (redirects and errors), as shown by the e2e test "test that multiple Set-Cookie headers are not lost during redirect." However, response.ok is true for status codes 200-299, which means event-context cookies (set via h3'ssetCookie) are excluded from successful responses.Either document why 2xx responses should not merge event cookies, or remove this condition to preserve them consistently. Additionally, simplify lines 92-95: the
delete()followed by re-appending response cookies is redundant—directly append only event cookies to achieve the same result.packages/start-server-core/package.json (1)
82-82: [email protected] version is valid and not deprecated.The specified version exists in the npm registry with no deprecation flag. The pre-release progression from 2.0.1-rc.2 to 2.0.1-rc.6 is supported by available versions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-server-core/src/request-response.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-server-core/src/request-response.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/request-response.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (5)
packages/start-server-core/src/request-response.ts (5)
62-66: LGTM - Well-defined utility types.The type definitions appropriately support both modern environments with
getSetCookie()and fallback scenarios. TheMaybePromisetype enables promise-aware header attachment.
101-119: LGTM - Robust promise-aware header attachment.The implementation correctly handles both synchronous and asynchronous handler results:
- Promises are resolved before header merging
- Non-Response values pass through unmodified
- Type safety is preserved with proper generic handling
130-130: LGTM - Clean integration of header merging.The modification correctly positions header attachment before response conversion, enabling Set-Cookie propagation from the event context to the final Response. The approach maintains backward compatibility since merging is conditional on response status.
81-84: The implementation is correct. Theresponse.okcheck intentionally limits header merging to non-successful responses (3xx redirects and 4xx/5xx errors), which aligns with the test case "multiple Set-Cookie headers are preserved on redirect." Successful responses (2xx) don't require event cookie merging.
72-79: Understand Set-Cookie fallback limitations due to Fetch API constraints.The fallback at lines 77-78 uses
headers.get('set-cookie'), which returns only a single Set-Cookie value per the Fetch standard (Headers objects cannot represent multiple Set-Cookie headers). This is an API-level constraint, not a code issue—the function correctly prioritizesgetSetCookie()when available, and the fallback is the only option in environments without this method. Document this limitation if needed.
| response.headers.delete('set-cookie') | ||
| for (const cookie of responseSetCookies) { | ||
| response.headers.append('set-cookie', cookie) | ||
| } | ||
| for (const cookie of eventSetCookies) { | ||
| response.headers.append('set-cookie', cookie) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "request-response.ts" -type f | head -5Repository: TanStack/router
Length of output: 111
🏁 Script executed:
cat -n packages/start-server-core/src/request-response.ts | sed -n '80,110p'Repository: TanStack/router
Length of output: 1105
🏁 Script executed:
cat -n packages/start-server-core/src/request-response.ts | head -50Repository: TanStack/router
Length of output: 1870
🏁 Script executed:
cat -n packages/start-server-core/src/request-response.ts | sed -n '50,90p'Repository: TanStack/router
Length of output: 1509
Response headers mutation will fail at runtime for immutable Response objects.
The code directly mutates response.headers using delete() and append(). However, Fetch API Response headers are immutable when created via Response.error(), Response.redirect(), or received from fetch(), throwing a TypeError if mutation is attempted. Since the mergeEventResponseHeaders function receives a generic Response parameter with unknown origin, this approach will fail for responses with immutable headers.
Instead of mutating headers directly, create a new Response with merged headers:
const newHeaders = new Headers(response.headers)
newHeaders.delete('set-cookie')
for (const cookie of responseSetCookies) {
newHeaders.append('set-cookie', cookie)
}
for (const cookie of eventSetCookies) {
newHeaders.append('set-cookie', cookie)
}
return new Response(response.body, {
status: response.status,
statusText: response.statusText,
headers: newHeaders
})Alternatively, check if headers are mutable before attempting mutation, with fallback to response reconstruction for immutable headers.
🤖 Prompt for AI Agents
In packages/start-server-core/src/request-response.ts around lines 92 to 98, the
code mutates response.headers (delete/append) which will throw for immutable
Response headers; instead create a new Headers from response.headers, remove the
existing set-cookie entries on that new Headers, append responseSetCookies and
eventSetCookies to it, and return a new Response preserving response.body,
status, and statusText (and any other needed response options) with the merged
Headers; alternatively detect header mutability and fall back to this
reconstruction when headers are immutable.
Update from 2.0.1-rc.2 to 2.0.1-rc.6
There's a change to header merging behavior that causes test failure (title "multiple Set-Cookie headers are preserved on redirect"), starting from 2.0.1-rc.3
Implemented fix that keeps just the Set-Cookie headers in redirect, to maintain existing behavior.
Release notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.