-
Notifications
You must be signed in to change notification settings - Fork 100
chore: browserserver #643
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
chore: browserserver #643
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 enhances the ReusedBrowser class to first attempt connecting to an existing browser server before spawning a new one, and wires up the transport close event in BackendClient.
- Introduces
_connectToBrowserServerto probe and attach to a running server on localhost:14518 - Updates startup flow to fallback to launching a new server only if no existing server is found
- Hooks
this._transport.oncloseto fire the internal close event inBackendClient
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/reusedBrowser.ts | Added connection-attempt logic and helper method |
| src/backend.ts | Bound transport.onclose to _onCloseEvent.fire() |
Comments suppressed due to low confidence (1)
src/reusedBrowser.ts:198
- [nitpick] The new helper method lacks JSDoc or comments explaining its behavior and expected return values; adding documentation will improve maintainability.
private async _connectToBrowserServer(): Promise<Backend | null> {
| if (typeof json.wsEndpointPath !== 'string') | ||
| return null; | ||
| const client = new Backend(this._vscode); | ||
| await client._connect(baseURL.toString()); |
Copilot
AI
Jun 16, 2025
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.
The returned wsEndpointPath from the /json endpoint is never used; instead of connecting to baseURL directly, construct the WebSocket URL with json.wsEndpointPath (e.g. await client._connect(new URL(json.wsEndpointPath, baseURL).toString())).
| await client._connect(baseURL.toString()); | |
| await client._connect(new URL(json.wsEndpointPath, baseURL).toString()); |
|
|
||
| private async _connectToBrowserServer(): Promise<Backend | null> { | ||
| try { | ||
| const baseURL = new URL('http://localhost:14518'); |
Copilot
AI
Jun 16, 2025
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.
[nitpick] Hardcoding the browser server URL and port limits flexibility; consider extracting the host/port into a config or environment variable.
| const baseURL = new URL('http://localhost:14518'); | |
| const host = process.env.BROWSER_SERVER_HOST || 'localhost'; | |
| const port = process.env.BROWSER_SERVER_PORT || '14518'; | |
| const baseURL = new URL(`http://${host}:${port}`); |
| await client._connect(baseURL.toString()); | ||
| return client; | ||
| } catch (e) { | ||
| console.error('Failed to connect to browser server:', e); |
Copilot
AI
Jun 16, 2025
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.
[nitpick] Using console.error directly may not integrate with the project's logging framework; consider using a centralized logger for consistency.
|
revision 2: microsoft/playwright#36382 |
Counterpart of microsoft/playwright#36327.