Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 dcff88a
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![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/package.json (1)
85-85: Dependency is upgraded, but still pinned exactly.Line 85 uses an exact version (
2.0.1-rc.19). If the intent is to also unpin, switch to the repo’s preferred range style (for example^2.0.1-rc.19).Suggested change
- "h3": "2.0.1-rc.19", + "h3": "^2.0.1-rc.19",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-server-core/package.json` at line 85, The h3 dependency is pinned to an exact version ("h3": "2.0.1-rc.19"); change that entry in package.json to use the repo's preferred semver range (e.g., "h3": "^2.0.1-rc.19") so the dependency is unpinned and follow-up package installs pick up compatible updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-server-core/src/request-response.ts`:
- Around line 307-309: The getCookie implementation is converting empty-string
cookie values to undefined by using "cookies[name] || undefined"; update
getCookie to return the indexed value directly (i.e., return cookies[name]) so
that explicit empty values are preserved; locate the getCookie function where
getH3Event() and h3_parseCookies(event) are used and remove the "|| undefined"
fallback.
---
Nitpick comments:
In `@packages/start-server-core/package.json`:
- Line 85: The h3 dependency is pinned to an exact version ("h3":
"2.0.1-rc.19"); change that entry in package.json to use the repo's preferred
semver range (e.g., "h3": "^2.0.1-rc.19") so the dependency is unpinned and
follow-up package installs pick up compatible updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2a7c156-ee4d-481d-be81-705786a19f55
⛔ 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.jsonpackages/start-server-core/src/request-response.ts
| const event = getH3Event() | ||
| const cookies = h3_parseCookies(event) | ||
| return cookies[name] || undefined |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file exists and view lines 307-309
wc -l packages/start-server-core/src/request-response.ts
sed -n '305,315p' packages/start-server-core/src/request-response.tsRepository: TanStack/router
Length of output: 407
🏁 Script executed:
# Let's also look for the getCookie function definition and context
sed -n '305,320p' packages/start-server-core/src/request-response.tsRepository: TanStack/router
Length of output: 493
🏁 Script executed:
# Search for where h3_parseCookies is imported from
rg "h3_parseCookies" packages/start-server-core/src/request-response.ts -B 5 -A 5Repository: TanStack/router
Length of output: 1108
🏁 Script executed:
# Check package.json to find h3 version
rg '"h3"' packages/start-server-core/package.jsonRepository: TanStack/router
Length of output: 83
🏁 Script executed:
# Search for any tests or usage examples of getCookie with empty values
rg "getCookie" packages/start-server-core --type ts -A 3 -B 1 | head -50Repository: TanStack/router
Length of output: 1351
🏁 Script executed:
# Check if there are any comments or documentation about cookie behavior
rg "cookie" packages/start-server-core/src/request-response.ts -i -B 2 -A 2Repository: TanStack/router
Length of output: 3291
🏁 Script executed:
# Search for tests related to getCookie function
find packages/start-server-core -name "*.test.*" -o -name "*.spec.*" | xargs rg "getCookie|cookie" 2>/dev/null | head -30Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there's any TypeScript type definition or JSDoc for parseCookies
rg "parseCookies" --type ts -B 5 -A 10 | head -50Repository: TanStack/router
Length of output: 3939
🌐 Web query:
h3 parseCookies TypeScript return type empty string values
💡 Result:
In the h3 library (unjs/h3), the parseCookies(event) utility parses the Cookie header from the request and returns a Record<string, string> (object mapping cookie names to their values as strings). Cookie values are always strings, including empty strings ("") if the cookie was set with an empty value (e.g., name=; Path=/). This is standard behavior for Cookie header parsing using libraries like cookie (jshttp/cookie), which h3 relies on internally. TypeScript treats Record<string, string> as all values being non-nullable strings, so empty string values are fully compatible—no type errors occur. If no cookies are present, it returns {} (empty object), which is also valid. Examples from h3 docs and source: - parseCookies(event) → { cookieName: "cookieValue" } or { cookieName: "" } - getCookie(event, name) → string | undefined (wrapper around parseCookies) No issues or bugs reported specifically about empty string values in TypeScript context. If experiencing type mismatches, ensure proper typing: const cookies: Record<string, string> = parseCookies(event);. For source confirmation, early h3 versions used parse(req.headers.cookie || '') from 'cookie', returning Record<string, string>. Current versions maintain this signature per docs and utils pages.
Citations:
- 1: https://h3.dev/utils/cookie
- 2: https://github.com/unjs/h3/blob/10eab1b237b0e01eb9337da653faafb9b9616905/src/utils/cookie.ts
- 3: h3js/h3@088f413
- 4: https://www.h3.dev/examples/handle-cookie
- 5: https://h3.unjs.io/utils/advanced
Preserve empty cookie values in getCookie().
Line 309 uses || undefined, which converts valid empty cookie values ('') into undefined. This loses information about whether a cookie was explicitly set to an empty value versus not being set at all. Return the indexed value directly to preserve this distinction.
Suggested fix
export function getCookie(name: string): string | undefined {
const event = getH3Event()
const cookies = h3_parseCookies(event)
- return cookies[name] || undefined
+ return cookies[name]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const event = getH3Event() | |
| const cookies = h3_parseCookies(event) | |
| return cookies[name] || undefined | |
| export function getCookie(name: string): string | undefined { | |
| const event = getH3Event() | |
| const cookies = h3_parseCookies(event) | |
| return cookies[name] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-server-core/src/request-response.ts` around lines 307 - 309,
The getCookie implementation is converting empty-string cookie values to
undefined by using "cookies[name] || undefined"; update getCookie to return the
indexed value directly (i.e., return cookies[name]) so that explicit empty
values are preserved; locate the getCookie function where getH3Event() and
h3_parseCookies(event) are used and remove the "|| undefined" fallback.
|
The e2e test "un-matched malformed paths should return not found on direct navigation" is failing probably because of how h3 now handles malformed paths. I'm not sure what's the best way to continue. I will let the other more experienced contributors decide on how to continue with this PR. Also no worries if my approach is incorrect and you want to close the PR. |
|
@ulrichstark , thanks for looking into this. We need to update cookie-es to v3 (release notes) as part of this PR, since it was bumped in h3 v2.0.1-rc.19 |
ulrichstark
changed the title
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Thanks! I changed the PR title to reflect that and made one commit that removes an unnecessary |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/start-server-core/src/request-response.ts (1)
315-317:⚠️ Potential issue | 🟡 MinorDon’t collapse empty cookie values in
getCookie().Line 317 converts an explicit empty cookie into
undefined.h3’s own helper returns the parsed entry directly and documentsundefinedonly for the missing-cookie case, so this makesname=indistinguishable from “missing”. Returncookies[name]as-is. (raw.githubusercontent.com)Suggested fix
- return cookies[name] || undefined + return cookies[name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-server-core/src/request-response.ts` around lines 315 - 317, The getCookie implementation collapses explicit empty cookie values to undefined; update the function that calls getH3Event and h3_parseCookies (the getCookie helper) to return cookies[name] directly instead of using "|| undefined" so that an explicit empty string (name=) is preserved; locate the return in getCookie and remove the coalescing to undefined so it mirrors h3_parseCookies behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-server-core/src/request-response.ts`:
- Around line 295-303: The current normalization creates normalizedCookies with
a plain object literal which loses the prototype-less semantics from the
upstream parser; change the initialization of normalizedCookies to use
Object.create(null) so it remains prototype-less while you still copy/filter
values from cookies via the existing loop over Object.entries(cookies); keep the
rest of the logic the same (the variable names to update are normalizedCookies
and the loop using cookies).
---
Duplicate comments:
In `@packages/start-server-core/src/request-response.ts`:
- Around line 315-317: The getCookie implementation collapses explicit empty
cookie values to undefined; update the function that calls getH3Event and
h3_parseCookies (the getCookie helper) to return cookies[name] directly instead
of using "|| undefined" so that an explicit empty string (name=) is preserved;
locate the return in getCookie and remove the coalescing to undefined so it
mirrors h3_parseCookies behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 306bcd6e-518a-4865-a376-3d5f93faf0d2
📒 Files selected for processing (1)
packages/start-server-core/src/request-response.ts
This reverts commit c15834b.
|
I tried Nx's proposed fix for the CI failure, but that didn't turn the CI green. I reverted it and leave the rest of the PR up to other maintainers. |
|
@ulrichstark , I've submitted an issue here: |
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We replaced the bare return new Response(null, { status: 404 }) fallback with a path-sanitization step that double-encodes malformed percent sequences (% → %25) so h3 rc.19's new decodePathname/decodeURI call in the H3Event constructor no longer throws. By passing the original (unsanitized) request to the handler, the router still receives the malformed path, catches the URIError in its own findMatch, and renders the default-not-found-component with a proper 404 HTML response — restoring the behavior that existed with h3 rc.16.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/packages/start-server-core/src/request-response.ts b/packages/start-server-core/src/request-response.ts
index 2ceb0a2e50..e1dd29babb 100644
--- a/packages/start-server-core/src/request-response.ts
+++ b/packages/start-server-core/src/request-response.ts
@@ -126,10 +126,31 @@ export function requestHandler<TRegister = unknown>(
try {
h3Event = new H3Event(request)
} catch (err) {
- if (err instanceof URIError) {
+ if (!(err instanceof URIError)) {
+ throw err
+ }
+ // h3 v2 decodes the URL pathname during H3Event construction and throws
+ // URIError for malformed percent-encoded sequences (e.g. %E0%A4).
+ // Sanitize the path so H3Event can be constructed, but pass the original
+ // request to the handler so the router encounters the malformed path,
+ // catches its own URIError, and renders the not-found component with 404.
+ try {
+ const url = new URL(request.url)
+ url.pathname = url.pathname
+ .split('/')
+ .map((segment) => {
+ try {
+ decodeURIComponent(segment)
+ return segment
+ } catch {
+ return segment.replace(/%/g, '%25')
+ }
+ })
+ .join('/')
+ h3Event = new H3Event(new Request(url.toString(), request))
+ } catch {
return new Response(null, { status: 404 })
}
- throw err
}
const response = eventStorage.run({ h3Event }, () =>
Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.
Apply changes locally with:
npx nx-cloud apply-locally 5wuQ-kEUE
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
2 participants
Closes #7043
The function
h3_parseCookiesnow returnsRecord<string, string | undefined>instead ofRecord<string, string>. I handled that by filtering out theundefinedvalues to keep the function signature unchanged to avoid breaking changes. Also inlined code fromgetCookiesintogetCookieto avoid the unnecessary execution of theforloop.Summary by CodeRabbit
Chores
Bug Fixes