-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore: first pass at browser server #36327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new browser server that manages browser lifecycle and enables sharing between Playwright MCP, VS Code Extension, and CLI clients via HTTP endpoints.
- Adds
/json/list
and/json/launch
HTTP endpoints toWSServer
- Implements
onList
/onLaunch
inPlaywrightServer
withreuseGroup
logic - Updates CLI to POST to the browser server when
PW_BROWSER_SERVER
is set
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/playwright-core/src/server/utils/wsServer.ts | Added HTTP endpoints for listing and launching browsers and JSON body parsing helpers |
packages/playwright-core/src/remote/playwrightServer.ts | Impl. of onList /onLaunch , tracking non-testing browsers by reuseGroup and exposing WebSocket paths |
packages/playwright-core/src/remote/playwrightConnection.ts | Added keepBrowser callback to preserve reusable browsers and exported launchOptionsHash |
packages/playwright-core/src/client/browserType.ts | Adjusted connect overloads to return the correct Browser type |
packages/playwright-core/src/cli/program.ts | Uses PW_BROWSER_SERVER to POST launch requests, falling back to direct launch |
Comments suppressed due to low confidence (6)
packages/playwright-core/src/server/utils/wsServer.ts:90
- You should validate the parsed
body
(e.g. checkbrowserName
,launchOptions
presence and types) and return a clear HTTP 400 error when the payload is invalid, instead of letting JSON parsing errors or missing fields throw uncaught exceptions.
const body = await readBodyJSON(request) as LaunchRequest;
packages/playwright-core/src/server/utils/wsServer.ts:82
- Consider adding automated tests to cover the new
/json/list
endpoint to verify it returns the expected browser list structure under different conditions.
if (request.method === 'GET' && request.url === '/json/list') {
packages/playwright-core/src/remote/playwrightServer.ts:87
- There is a potential race condition if two launch requests with the same
reuseGroup
arrive concurrently: both could skip the existing browser check and launch duplicate browsers. Consider serializing or locking reuseGroup-based launches to prevent duplicates.
if (request.reuseGroup) browser = playwright.allBrowsers().find(b => b.options.name === request.browserName && this._nonTestingBrowsers.get(b)?.reuseGroup === request.reuseGroup);
packages/playwright-core/src/server/utils/wsServer.ts:56
- The
stream
namespace is used here but not imported. Please addimport type { Duplex } from 'stream';
(or similar) sostream.Duplex
is resolved correctly.
onUpgrade(request: http.IncomingMessage, socket: stream.Duplex): { error: string } | undefined;
packages/playwright-core/src/server/utils/wsServer.ts:58
- The delegate type makes
onClose
mandatory but elsewhere you callthis._delegate.onClose?.()
. Either makeonClose
optional in the type or remove the optional chaining in the invocation to keep them consistent.
onClose(): Promise<void>;
packages/playwright-core/src/cli/program.ts:483
- When POSTing JSON, include the
Content-Type: application/json
header for clarity and to ensure proper parsing on the server side.
body: JSON.stringify({
This comment has been minimized.
This comment has been minimized.
onConnection: (request: http.IncomingMessage, url: URL, ws: WebSocket, id: string) => WSConnection; | ||
onClose?(): Promise<void>; | ||
onHeaders(headers: string[]): void; | ||
onList(): any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be onGet()
and onPost()
instead. Or, as we usually do, a setRoute()
direct call.
@@ -60,14 +70,30 @@ export class WSServer { | |||
async listen(port: number = 0, hostname: string | undefined, path: string): Promise<string> { | |||
debugLogger.log('server', `Server started at ${new Date()}`); | |||
|
|||
const server = createHttpServer((request: http.IncomingMessage, response: http.ServerResponse) => { | |||
const server = createHttpServer(async (request: http.IncomingMessage, response: http.ServerResponse) => { | |||
if (request.method === 'GET' && request.url === '/json') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this endpoint makes sense for the new server.
browserName: browser.options.name, | ||
launchOptions: browser.options.originalLaunchOptions, | ||
reuseGroup: this._nonTestingBrowsers.get(browser)?.reuseGroup, | ||
wsPath: this._options.path + '?' + new URLSearchParams({ browserGuid: browser.guid }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd like different paths for different browsers.
}, | ||
|
||
onLaunch: async request => { | ||
if (!this._preLaunchedPlaywright) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the code here should be shared with what PlaywrightConnection
does. Overall, the ownership of the browsers should be in a single place, not split between server and connection. Let's land a refactoring beforehand if we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding of the Server/Connection split, this means that browser ownership should live exclusively in the Server, not in the Connection. Does that match your thinking? Happy land a refactor if you could outline some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | ||
|
||
onLaunch: async request => { | ||
if (!this._preLaunchedPlaywright) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding of the Server/Connection split, this means that browser ownership should live exclusively in the Server, not in the Connection. Does that match your thinking? Happy land a refactor if you could outline some more.
Test results for "tests 1"6 flaky39427 passed, 822 skipped Merge workflow run. |
Mirroring our offline discussion here:
|
revision 2: #36382 |
Adds a first sketch for a browser server that manages the browser lifecycle and facilitates sharing amongst Playwright MCP, the VS Code Extension and CLI clients like
npx playwright open
ornpx playwright test
.GET /json/list
gets a list of running browsersPOST /json/launch
launches a browser, if there's no other browser with the same reuseGroupHere's a demo of interop between VS Code and MCP:
Screen.Recording.2025-06-16.at.15.01.37.mov
You can also do
nxp playwright open
with thePW_BROWSER_SERVER
env var. There's no support in test runner yet.See microsoft/playwright-mcp#554 and microsoft/playwright-vscode#643 for the other side of the demo.