Skip to content

[miniflare] Add local simulation for browser rendering devtools endpo…#13025

Draft
ruifigueira wants to merge 5 commits intomainfrom
rfigueira/devtools
Draft

[miniflare] Add local simulation for browser rendering devtools endpo…#13025
ruifigueira wants to merge 5 commits intomainfrom
rfigueira/devtools

Conversation

@ruifigueira
Copy link
Contributor

…ints

Fixes #[insert GH or internal issue link(s)].

Describe your change...


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because:

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2026

🦋 Changeset detected

Latest commit: e2636b6

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 2026

⚠️ Issues found

.changeset/headful-browser-rendering.md

Markdown headers (h2/h3 used — not allowed)

The changeset uses bold text (**...**) to label sections, which is fine, but it also uses **wrangler dev**: and **@cloudflare/vite-plugin** — these are bold, not headers, so they are acceptable.

However, the changeset description does not explicitly call out that this is an experimental feature. The Browser Rendering API is experimental (in local dev/preview). Per the guidelines:

When adding or changing experimental features, call this out explicitly in the changeset description.

The changeset describes new opt-in behavior (--headful flag, headful: true config) without noting whether Browser Rendering local dev support itself is experimental/beta. If this feature is experimental, the changeset should include a note such as "This is an experimental feature; opt in via --headful."

If Browser Rendering local dev is already stable, this is not an issue — but it should be verified.


.changeset/browser-rendering-devtools-endpoints.md

No issues found. The description is clear, the version type (minor — new feature/simulation) is appropriate, no forbidden headers, and it does not relate to analytics.


Summary of issues:

  1. .changeset/headful-browser-rendering.md — If Browser Rendering local dev support is an experimental feature, the changeset must explicitly note that. The guidelines require: "When adding or changing experimental features, call this out explicitly in the changeset description."

Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR adds local simulation for browser rendering devtools endpoints in miniflare, implementing ~15 new route handlers that proxy Chrome's DevTools JSON and WebSocket endpoints.

Issues

  1. Bug: proxyPageWebSocket will throw on reserved WebSocket close codes — The existing BrowserSession.fetch method has a forwardClose helper (line 92-99) that guards against reserved close codes 1005 and 1006 which throw InvalidAccessError when passed to WebSocket.close(). The new proxyPageWebSocket function directly forwards code and reason without this guard, so any Chrome page connection that closes with code 1005 or 1006 will crash the proxy.

Comment on lines +226 to +227
server.addEventListener("close", ({ code, reason }) => ws.close(code, reason));
ws.addEventListener("close", ({ code, reason }) => server.close(code, reason));
Copy link
Contributor

Choose a reason for hiding this comment

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

Reserved close codes 1005 (No Status Received) and 1006 (Abnormal Closure) are valid in CloseEvent but throw InvalidAccessError when passed to .close(). The existing BrowserSession.fetch already handles this (see forwardClose at line 92). This proxy should do the same.

Suggested change
server.addEventListener("close", ({ code, reason }) => ws.close(code, reason));
ws.addEventListener("close", ({ code, reason }) => server.close(code, reason));
server.addEventListener("close", ({ code, reason }) => {
if (code === 1005 || code === 1006) {
ws.close();
} else {
ws.close(code, reason);
}
});
ws.addEventListener("close", ({ code, reason }) => {
if (code === 1005 || code === 1006) {
server.close();
} else {
server.close(code, reason);
}
});

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 23, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR adds local simulation for browser rendering devtools endpoints in miniflare, implementing ~15 new route handlers that proxy Chrome's DevTools JSON and WebSocket endpoints.

Issue found: The new proxyPageWebSocket function at packages/miniflare/src/workers/browser-rendering/binding.worker.ts:226-227 directly forwards WebSocket close codes without guarding against reserved codes 1005 and 1006. These codes are valid in CloseEvent but throw InvalidAccessError when passed to WebSocket.close(). The existing BrowserSession.fetch already handles this correctly with a forwardClose helper (line 92-99). I posted a suggestion comment with the fix.

github run

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 23, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13025

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13025

miniflare

npm i https://pkg.pr.new/miniflare@13025

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13025

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13025

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13025

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13025

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13025

wrangler

npm i https://pkg.pr.new/wrangler@13025

commit: e2636b6

@ruifigueira ruifigueira force-pushed the rfigueira/devtools branch 3 times, most recently from 7c3a803 to 5b74cc2 Compare March 23, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

2 participants