perf(cookie): avoid quadratic chunked cookie parsing and header rebuilds#1472
Conversation
Read path: getChunkedCookie now parses the request cookie header once and indexes into the result instead of reparsing it per chunk. Write path: setCookie appends directly when no existing set-cookie header shares the cookie name, skipping the delete/reparse/re-append rebuild. The dedupe rebuild only runs on an actual name collision. Minimal alternative to #1368. Co-authored-by: Trivikram Kamat <[email protected]> Co-Authored-By: Claude Fable 5 <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesCookie handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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)
test/cookies.test.ts (1)
142-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
ctx.fetchfor cross-runtime URL resolution.This test calls
t.fetch("/"), but the repository requiresctx.fetchso URL resolution works consistently in both web and node test contexts.As per coding guidelines,
test/**/*.test.tsmust usectx.fetchfor URL resolution that works in both web and node test contexts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cookies.test.ts` at line 142, Update the test request in the affected cookie test from t.fetch("/") to ctx.fetch("/"), preserving the existing path and assertions so URL resolution works consistently across web and Node test contexts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/cookie-perf.test.ts`:
- Around line 17-21: Refactor the “cookie performance” test suite to use
describeMatrix for both web and node modes instead of describe. Initialize a
fresh H3 instance and real H3Event in beforeEach, track unhandled errors through
ctx.errors, and assert them in afterEach; update the tests to use these
instances and remove event as any casts while preserving the existing cookie
assertions.
---
Nitpick comments:
In `@test/cookies.test.ts`:
- Line 142: Update the test request in the affected cookie test from
t.fetch("/") to ctx.fetch("/"), preserving the existing path and assertions so
URL resolution works consistently across web and Node test contexts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bcb9c5a-b924-47a2-95cf-8070c46dcd89
📒 Files selected for processing (3)
src/utils/cookie.tstest/cookies.test.tstest/unit/cookie-perf.test.ts
| describe("cookie performance", () => { | ||
| beforeEach(() => { | ||
| parseCookieSpy.mockClear(); | ||
| parseSetCookieSpy.mockClear(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align test suite with cross-runtime testing guidelines.
The current test suite uses a standard describe block and manually mocks the event object. As per coding guidelines for test/**/*.test.ts, tests must:
- Use
describeMatrixto execute in bothwebandnodemodes. - Ensure each test has a fresh
H3instance viabeforeEachsetup. - Track unhandled errors with
ctx.errorsand auto-assert them inafterEach.
Refactoring this suite to use the matrix testing utilities and real H3Event instances will properly validate the cookie optimizations across runtimes and eliminate the need for casting event as any in your assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/cookie-perf.test.ts` around lines 17 - 21, Refactor the “cookie
performance” test suite to use describeMatrix for both web and node modes
instead of describe. Initialize a fresh H3 instance and real H3Event in
beforeEach, track unhandled errors through ctx.errors, and assert them in
afterEach; update the tests to use these instances and remove event as any casts
while preserving the existing cookie assertions.
Source: Coding guidelines
# Conflicts: # test/cookies.test.ts
Co-Authored-By: Claude Fable 5 <[email protected]>
2 participants
Summary
Minimal alternative to #1368 — same perf improvement for the chunked-cookie path used by sessions, without the WeakMap caches or set-cookie state tracking. Credit to @trivikr for identifying the issue and the original implementation (also co-authored on the commit).
setCookie()rebuilt (delete →parseSetCookieeach → re-append) the fullset-cookieheader list on every write, andgetChunkedCookie()reparsed the full requestcookieheader once per chunk — making large chunked session reads/writes quadratic in chunk count.Changes
getChunkedCookie()callsparseCookies()once and indexes into the result instead of callinggetCookie()(a full reparse) per chunk.setCookie()appends directly when no existingset-cookieheader starts withname=— a cheap conservative check that skips the parse/rebuild entirely. The dedupe rebuild still runs on an actual name collision, so overwrite semantics are unchanged. Chunk cookie names are always unique, sosetChunkedCookie()always takes the fast path.A side benefit of the fast path:
set-cookieheaders written directly tores.headers(e.g. by middleware) are no longer dropped when a latersetCookie()call appends an unrelated cookie (previously the rebuild discarded any headerparseSetCookiecouldn't parse).Tests
parseCookiecall for a chunked read; zeroparseSetCookiecalls when appending unique chunks.set-cookieheaders survive an h3-managed cookie overwrite.🤖 Generated with Claude Code
Summary by CodeRabbit
set-cookieheaders added directly to the response when overwriting a cookie with the same name.set-cookieheaders aren’t removed during cookie updates.Set-Cookiechunk output.