fix(app-route): merge middleware headers with override semantics by NathanDrake2406 · Pull Request #789 · cloudflare/vinext

NathanDrake2406 · 2026-04-08T07:36:20Z

Summary

update applyRouteHandlerMiddlewareContext to merge middleware response headers with Next-like semantics
treat set-cookie and vary as additive headers (append)
treat all other headers as singular override headers (set), so middleware can override route handler values like cache-control
add regression coverage for singular override and additive merge behavior

Verification

vp test run tests/app-route-handler-response.test.ts tests/app-route-handler-cache.test.ts tests/app-route-handler-execution.test.ts tests/app-route-handler-policy.test.ts tests/app-route-handler-runtime.test.ts

pkg-pr-new · 2026-04-08T07:37:01Z

npm i https://pkg.pr.new/vinext@789

commit: ae02936

Copilot

Pull request overview

This PR updates app route handler middleware header application to match Next-like merge semantics, allowing middleware to override singular response headers while preserving additive multi-value headers.

Changes:

Update applyRouteHandlerMiddlewareContext to set() most middleware headers (override semantics) while append()ing set-cookie and vary.
Update/extend unit tests to cover singular override behavior and additive merge behavior for Set-Cookie and Vary.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
tests/app-route-handler-response.test.ts	Adjusts existing expectations for singular header overrides and adds a regression test for additive `Set-Cookie`/`Vary` behavior.
packages/vinext/src/server/app-route-handler-response.ts	Implements override semantics for most middleware headers while preserving additive behavior for `set-cookie` and `vary`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T07:40:35Z

  const responseHeaders = new Headers(response.headers);
  if (middlewareContext.headers) {
    for (const [key, value] of middlewareContext.headers) {
-      responseHeaders.append(key, value);
+      const lowerKey = key.toLowerCase();
+      if (lowerKey === "set-cookie" || lowerKey === "vary") {
+        responseHeaders.append(key, value);
+      } else {
+        responseHeaders.set(key, value);
+      }
    }


The header merge logic here duplicates mergeMiddlewareResponseHeaders() in server/app-page-response.ts (same set-cookie/vary append vs other headers set semantics). To avoid the two implementations drifting over time, consider reusing the shared helper (or extracting a small shared utility) rather than re-implementing the merge loop in this file.

Copilot · 2026-04-08T07:40:36Z

+  it("appends additive middleware headers for Set-Cookie and Vary", async () => {
+    const response = new Response("hello", {
+      status: 200,
+      headers: [
+        ["vary", "RSC, Accept"],
+        ["set-cookie", "existing=1; Path=/"],
+      ],
+    });
+
+    const middlewareHeaders = new Headers();
+    middlewareHeaders.append("vary", "Next-Router-State-Tree");
+    middlewareHeaders.append("set-cookie", "mw=1; Path=/");
+
+    const result = applyRouteHandlerMiddlewareContext(response, {
+      headers: middlewareHeaders,
+      status: null,
+    });
+
+    expect(result.headers.get("vary")).toBe("RSC, Accept, Next-Router-State-Tree");
+    expect(result.headers.getSetCookie()).toEqual(["existing=1; Path=/", "mw=1; Path=/"]);
+    await expect(result.text()).resolves.toBe("hello");
+  });


The new additive behavior is only exercised with a single middleware set-cookie value. It would be valuable to add a regression case where middlewareHeaders.append('set-cookie', ...) is called multiple times to ensure applyRouteHandlerMiddlewareContext() preserves multiple middleware cookies as distinct Set-Cookie headers (i.e., they don’t get flattened into a single comma-separated value).

james-elicx · 2026-04-08T08:18:13Z

/bigbonk review

ask-bonk

Good change — extracting the shared helper and fixing the route handler path to use set() instead of append() for singular headers is the right call. Tests are solid and cover both override and additive semantics.

Bug: `buildAppPageHtmlResponse` still uses the old `append`-only loop

buildAppPageHtmlResponse in app-page-response.ts:233-237 was not updated and still uses the old append-only loop for middleware headers:

if (options.middlewareContext.headers) {
  for (const [key, value] of options.middlewareContext.headers) {
    headers.append(key, value);
  }
}

This means middleware can't override singular headers (like Cache-Control) on HTML page responses — the exact same bug this PR fixes for route handlers. Since the whole point of extracting mergeMiddlewareResponseHeaders into a shared module was to avoid divergence, this callsite should use the shared helper too:

mergeMiddlewareResponseHeaders(headers, options.middlewareContext.headers);

Per the AGENTS.md guidance: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as follow-ups — fix them in the same PR."

ask-bonk · 2026-04-08T08:20:11Z

@@ -0,0 +1,19 @@
+const ADDITIVE_RESPONSE_HEADER_NAMES = new Set(["set-cookie", "vary"]);
+
+export function mergeMiddlewareResponseHeaders(


Nit: the old code in app-page-response.ts had a useful JSDoc comment explaining the merge contract that got dropped during the move. Since this is now the canonical shared implementation, it'd be worth carrying that doc forward:

Suggested change

export function mergeMiddlewareResponseHeaders(

/**

* Merge middleware response headers into a target Headers object.

*

* Set-Cookie and Vary are accumulated (append) since multiple sources can

* contribute values. All other headers use set() so middleware owns singular

* response headers like Cache-Control.

*/

export function mergeMiddlewareResponseHeaders(

ask-bonk · 2026-04-08T08:20:11Z

-    }
-  }
-}
+export { mergeMiddlewareResponseHeaders };


The re-export keeps the public API surface stable for existing consumers (entry templates import from this module). Good.

ask-bonk · 2026-04-08T08:20:19Z

Review posted. Here's a summary of my feedback:

Overall: Good change. The extraction of mergeMiddlewareResponseHeaders into a shared module and fixing the route handler to use set() (override) instead of append() for singular headers is correct. Tests cover both override and additive semantics well.

Key finding — bug in scope of this PR:

buildAppPageHtmlResponse in app-page-response.ts:233-237 still uses the old append-only loop for middleware headers. This is the same bug being fixed for route handlers, and per the repo's guidelines ("fix the same bug in other files in the same PR"), it should be updated to call the shared mergeMiddlewareResponseHeaders helper.

Minor nit: The JSDoc comment from the original function was dropped during the extraction to the new module. Worth carrying it forward since this is now the canonical implementation.

github run

james-elicx · 2026-04-08T09:11:13Z

/bigbonk review

ask-bonk

Clean PR. The extraction of the merge logic into middleware-response-headers.ts is well done, and the fix correctly applies to both the route handler and the HTML page response paths. Tests cover the key behaviors (singular override, additive Set-Cookie/Vary, multiple Set-Cookie preservation). The ask-bonk concern about buildAppPageHtmlResponse still using the old append-only loop is already addressed in this diff — line 233 now calls the shared helper.

One minor nit below, otherwise LGTM.

ask-bonk · 2026-04-08T09:12:43Z

-    }
-  }
-}
+export { mergeMiddlewareResponseHeaders };


Nit: this re-export means consumers that import from app-page-response.ts still get the function, which is good for backward compat. But it also means there are now two valid import paths for the same function (app-page-response and middleware-response-headers). If new code in the codebase accidentally imports from here instead of the canonical module, there's no signal that this is just a re-export.

Consider adding a brief comment:

Suggested change

export { mergeMiddlewareResponseHeaders };

/** @see middleware-response-headers.ts — re-exported for backward compatibility with entry templates */

export { mergeMiddlewareResponseHeaders };

Very minor — not blocking.

ask-bonk · 2026-04-08T09:12:49Z

Review submitted. Here's a summary:

Approved with one minor nit.

The PR correctly extracts the middleware header merge logic into a shared middleware-response-headers.ts module and fixes both the route handler (app-route-handler-response.ts) and the HTML page response (app-page-response.ts) paths to use set() for singular headers and append() for Set-Cookie/Vary. The earlier reviewer concern about buildAppPageHtmlResponse still using the old append-only loop is already addressed in this diff.

The only suggestion was adding a brief comment on the re-export in app-page-response.ts:178 to clarify it exists for backward compatibility with entry templates — not blocking.

github run

fix(app-route): merge middleware headers with override semantics

9321f46

Copilot AI review requested due to automatic review settings April 8, 2026 07:36

Copilot started reviewing on behalf of NathanDrake2406 April 8, 2026 07:36 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

refactor(app-route): share middleware header merge helper

92bd009

ask-bonk Bot reviewed Apr 8, 2026

View reviewed changes

fix(app-page): apply shared middleware header merge to HTML

ae02936

ask-bonk Bot approved these changes Apr 8, 2026

View reviewed changes

james-elicx merged commit fc8b0d8 into cloudflare:main Apr 8, 2026
24 checks passed

NathanDrake2406 deleted the fix/bug4-route-handler-merge branch April 22, 2026 13:14

		@@ -0,0 +1,19 @@
		const ADDITIVE_RESPONSE_HEADER_NAMES = new Set(["set-cookie", "vary"]);

		export function mergeMiddlewareResponseHeaders(

-export function mergeMiddlewareResponseHeaders(
+/**
+ * Merge middleware response headers into a target Headers object.
+ *
+ * Set-Cookie and Vary are accumulated (append) since multiple sources can
+ * contribute values.  All other headers use set() so middleware owns singular
+ * response headers like Cache-Control.
+ */
+export function mergeMiddlewareResponseHeaders(

	export { mergeMiddlewareResponseHeaders };
	/** @see middleware-response-headers.ts — re-exported for backward compatibility with entry templates */
	export { mergeMiddlewareResponseHeaders };

Conversation

NathanDrake2406 commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Uh oh!

pkg-pr-new Bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

james-elicx commented Apr 8, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Bug: buildAppPageHtmlResponse still uses the old append-only loop

Uh oh!

ask-bonk Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Apr 8, 2026

Uh oh!

james-elicx commented Apr 8, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NathanDrake2406 commented Apr 8, 2026 •

edited

Loading

pkg-pr-new Bot commented Apr 8, 2026 •

edited

Loading

Bug: `buildAppPageHtmlResponse` still uses the old `append`-only loop