-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(start-core): use headers.entries() instead of Object.entries #6276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis PR fixes an issue where Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 f7b6a5a
☁️ Nx Cloud last updated this comment at |
|
Didn't notice another PR for that issue. Will close that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/react-start/server-functions-global-middleware/src/start.ts (1)
6-11: Remove unused imports.The imports
getResponseHeadersandsetResponseHeaderare not used anywhere in this file. OnlygetRequest(used intrackMiddlewareExecution) andsetResponseHeaders(used inheaderMiddleware) are actually needed.🔎 Proposed fix to remove unused imports
import { getRequest, - getResponseHeaders, - setResponseHeader, setResponseHeaders, } from '@tanstack/react-start/server'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/react-start/server-functions-global-middleware/src/start.tse2e/react-start/server-functions-global-middleware/tests/global-middleware.spec.tspackages/start-server-core/src/request-response.ts
🧰 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.tse2e/react-start/server-functions-global-middleware/tests/global-middleware.spec.tse2e/react-start/server-functions-global-middleware/src/start.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.tse2e/react-start/server-functions-global-middleware/tests/global-middleware.spec.tse2e/react-start/server-functions-global-middleware/src/start.ts
🧠 Learnings (1)
📚 Learning: 2025-12-24T22:47:44.320Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6211
File: e2e/react-start/i18n-paraglide/src/server.ts:6-6
Timestamp: 2025-12-24T22:47:44.320Z
Learning: In TanStack Router projects using `inlang/paraglide-js`, the callback passed to `paraglideMiddleware` should use `() => handler.fetch(req)` (referencing the outer `req`) instead of `({ request }) => handler.fetch(request)`. This is intentional because the router needs the untouched URL to perform its own rewrite logic with `deLocalizeUrl`/`localizeUrl`. The middleware's processed request would delocalize the URL and interfere with the router's rewrite handling.
Applied to files:
e2e/react-start/server-functions-global-middleware/src/start.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 (4)
packages/start-server-core/src/request-response.ts (1)
211-211: Excellent fix for Headers iteration!This correctly uses the Headers API's
entries()method instead ofObject.entries(). The bug occurred becauseObject.entries(headers)returns an empty array when passed a Headers instance (which is not a plain object), whereasheaders.entries()properly iterates over the header entries.e2e/react-start/server-functions-global-middleware/tests/global-middleware.spec.ts (1)
132-139: Well-structured test for the header middleware fix.The test appropriately validates that response headers set via
setResponseHeadersin middleware are correctly applied. The use of lowercase'x-header-middleware'in the assertion is correct since HTTP headers are case-insensitive and Playwright normalizes them to lowercase.e2e/react-start/server-functions-global-middleware/src/start.ts (2)
57-63: Perfect demonstration of the setResponseHeaders fix.This middleware correctly demonstrates the issue that was fixed: creating a Headers instance and passing it to
setResponseHeaders. With the fix inrequest-response.ts, this now properly iterates the headers usingheaders.entries().
95-95: LGTM!The middleware is properly registered and will be executed for all requests, allowing the test to verify that headers are correctly set.
There was a bug when setting headers with
setResponseHeadersdid not change them. The root cause was thatObject.entiresreturned empty array instead of actual headers. The fix is to useheaders.entries()instead.This PR covers the issue with e2e test and fixes it.
Fixes #5407
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.