Skip to content

fix(HttpResponse): forward cookies only when response is used - #2728

Merged
kettanaito merged 1 commit into
mainfrom
fix/response-cookies-on-exhaust
Apr 29, 2026
Merged

fix(HttpResponse): forward cookies only when response is used#2728
kettanaito merged 1 commit into
mainfrom
fix/response-cookies-on-exhaust

Conversation

@kettanaito

Copy link
Copy Markdown
Member

Currently, we are forwarding the mocked response cookies onto document.cookie as a part of the HttpResponse constructor. That means that even constructing an HttpResponse instance will immediately forward its Set-Cookie onto document.cookie, even if that response never gets used (e.g. is conditional).

Changes

  • HttpResponse no longer forwards its Set-Cookie onto document.cookie in the constructor.
  • Response cookie forwarding is moved down to RequestHandler.run and applies only to the mocked response getting used.

@coderabbitai

coderabbitai Bot commented Apr 29, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Request handling now supports optional mocked responses and forwards raw Set-Cookie strings to the browser via a new exported forwardResponseCookies. The internal kSetCookie symbol was made private, with getRawSetCookie exposing stored raw cookie strings; cookie storage reads via that helper.

Changes

Cohort / File(s) Summary
Request Handler
src/core/handlers/RequestHandler.ts
Widened resolver result type to `Response
HTTP Response Decorators
src/core/utils/HttpResponse/decorators.ts
Made kSetCookie symbol private; removed headers-polyfill import/usage here; store raw set-cookie string on responses via internal symbol and added exported `getRawSetCookie(response: Response): string
Request Cookie Storage
src/core/utils/request/storeResponseCookies.ts
Replaced direct Reflect.get(..., kSetCookie) access with getRawSetCookie(response) call before passing value to cookieStore.setCookie.
Browser Test Mocks
test/browser/rest-api/request/request-cookies.mocks.ts
Modified /set-cookies MSW mock to construct an additional HttpResponse whose Set-Cookie header is set to must-not=be-set before returning the original response that echoes the request body.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant RequestHandler
    participant MockedResponse
    participant forwardResponseCookies
    participant Document

    Client->>RequestHandler: send request
    RequestHandler->>MockedResponse: invoke resolver -> Response | undefined
    alt mocked Response present
        RequestHandler->>forwardResponseCookies: forwardResponseCookies(response)
        forwardResponseCookies->>MockedResponse: read raw Set-Cookie(s)
        forwardResponseCookies->>Document: assign each to document.cookie
    end
    RequestHandler->>Client: return mocked or proxied response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I found cookies in a mocked reply,
Snuck them to document under moonlit sky,
Symbols hid the crumbs so neat,
getRawSetCookie whispers what to eat,
Forwarded treats make the browser sigh 🍪

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving cookie forwarding from HttpResponse construction to actual response usage.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem (cookies forwarded on construction) and the solution (move forwarding to RequestHandler.run).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/response-cookies-on-exhaust

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.

❤️ Share
Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 57 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@kettanaito
kettanaito force-pushed the fix/response-cookies-on-exhaust branch from 686d87d to 5afdabf Compare April 29, 2026 09:30
@pkg-pr-new

pkg-pr-new Bot commented Apr 29, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/msw@2728

commit: 5afdabf

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/core/handlers/RequestHandler.ts (1)

340-340: Prefer explicit Response narrowing before cookie forwarding.

At Line 353, the truthy check works in practice, but instanceof Response makes the contract explicit and safer for untyped consumers.

Suggested tweak
-    if (mockedResponse) {
+    if (mockedResponse instanceof Response) {
       forwardResponseCookies(mockedResponse)
     }

Also applies to: 353-355

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/handlers/RequestHandler.ts` at line 340, Replace the loose truthy
check used before cookie forwarding with an explicit Response type guard: ensure
the value returned where the code currently narrows with a truthy check is first
checked with "instanceof Response" (or a dedicated isResponse helper) before you
call response.clone()/forwardCookies or access Response properties; update the
Promise<Response | undefined> handling in RequestHandler (the code that
currently treats the resolved value as truthy) to branch on instanceof Response
and only forward cookies in that branch, returning undefined or handling other
cases otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/core/handlers/RequestHandler.ts`:
- Line 340: Replace the loose truthy check used before cookie forwarding with an
explicit Response type guard: ensure the value returned where the code currently
narrows with a truthy check is first checked with "instanceof Response" (or a
dedicated isResponse helper) before you call response.clone()/forwardCookies or
access Response properties; update the Promise<Response | undefined> handling in
RequestHandler (the code that currently treats the resolved value as truthy) to
branch on instanceof Response and only forward cookies in that branch, returning
undefined or handling other cases otherwise.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 209eb580-c94a-4edc-89ad-7c7f48544994

📥 Commits

Reviewing files that changed from the base of the PR and between 686d87d and 5afdabf.

📒 Files selected for processing (4)
  • src/core/handlers/RequestHandler.ts
  • src/core/utils/HttpResponse/decorators.ts
  • src/core/utils/request/storeResponseCookies.ts
  • test/browser/rest-api/request/request-cookies.mocks.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/browser/rest-api/request/request-cookies.mocks.ts
  • src/core/utils/request/storeResponseCookies.ts
  • src/core/utils/HttpResponse/decorators.ts

@kettanaito
kettanaito merged commit 30668e6 into main Apr 29, 2026
22 checks passed
@kettanaito
kettanaito deleted the fix/response-cookies-on-exhaust branch April 29, 2026 09:38
@kettanaito

Copy link
Copy Markdown
Member Author

Released: v2.14.0 🎉

This has been released in v2.14.0.

Get these changes by running the following command:

npm i msw@latest

Predictable release automation by Release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant