Conversation
package.nls.json
Outdated
| "contributes.command.pw.extension.command.inspect": "Pick locator", | ||
| "contributes.command.pw.extension.command.clearCache": "Clear cache", | ||
| "contributes.command.pw.extension.command.closeBrowsers": "Close all Playwright browsers", | ||
| "contributes.command.pw.extension.command.closeBrowsers": "Close all Playwright testing browsers", |
There was a problem hiding this comment.
Are you going to follow up with translations? I wonder what is the process for that?
There was a problem hiding this comment.
Good question. I don't think we have a set process - let me follow up with translations at the end, when everything's said and done.
| <title>Playwright</title> | ||
| </head> | ||
| <body class="settings-view"> | ||
| <section role="region" aria-label="Browsers"> |
There was a problem hiding this comment.
Perhaps the "Show browser" checkbox should be here as well?
There was a problem hiding this comment.
perhaps! I'll try it out, but that's for a different PR.
| this._onChanged = new this._vscode.EventEmitter(); | ||
| this.onChanged = this._onChanged.event; | ||
|
|
||
| this._reusedBrowser.onBackend(b => this._add(b)); |
There was a problem hiding this comment.
Instead of listening to the ReusedBrowser, I'd make this a part of it. Or do you plan for this list to aggregate multiple backends?
There was a problem hiding this comment.
Yep, the MCP server will also be contributing a DebugController, but with different behaviour than ReusedBrowser. BrowserList will aggregate both.
| } | ||
|
|
||
| export function createAction(action: ActionDescriptor, options?: { omitText?: boolean }): HTMLElement | null { | ||
| export interface BrowserEntry { |
There was a problem hiding this comment.
Shouldn't this be in BrowserList?
There was a problem hiding this comment.
no, because it's part of the communication between settingsView.ts and settingsView.script.ts
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src ${webview.cspSource}; script-src 'nonce-${nonce}';"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src 'unsafe-inline' ${webview.cspSource}; script-src 'nonce-${nonce}';"> |
There was a problem hiding this comment.
Do you really need unsafe-inline?
There was a problem hiding this comment.
yes, becaue the SVGs have <def> elements that don't work otherwise. I was unable to rewrite it so that it works without unsafe-inline. If you know a magic trick, I'd be happy to learn it!
There was a problem hiding this comment.
Well, we can use a png icon, or find a different svg. Here's an example.
Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src ${webview.cspSource}; script-src 'nonce-${nonce}';"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src 'unsafe-inline' ${webview.cspSource}; script-src 'nonce-${nonce}';"> |
There was a problem hiding this comment.
Well, we can use a png icon, or find a different svg. Here's an example.
src/browserList.ts
Outdated
| import * as vscodeTypes from './vscodeTypes'; | ||
|
|
||
| export class BrowserList { | ||
| private _state = new Map<DebugController, { id: string; name: string; channel?: string; title: string }[]>(); |
There was a problem hiding this comment.
I don't quite like that BrowserList touches DebugController, which is an internal part of it. I'd prefer for BrowserList to listen to ReusedBrowser.onBrowsersChanged event instead, and encapsulate the DebugController inside. WDYT?
There was a problem hiding this comment.
I think it's good how it is. Let me give an outlook for what i'm planning in the upcoming PRs:
ReusedBrowser won't be the only thing that contributes Debug Controllers, but the MCP integration will also contribute them. So I want all browser-related actions to live on BrowserList instead of ReusedBrowser. For that, I need DebugController to become an internal part of BrowserList. ReusedBrowser will lose a lot of responsibility to BrowserList, all the inspection and closing and recording stuff will move over to BrowserList, and ReusedBrowser ends up being just about starting/stopping the browser server.
There was a problem hiding this comment.
As discussed offline, I updated this PR so that it moves over the DebugController-related parts right away.
|
Mirroring our offline discussion here. Splitting up the responsibilities between BrowserList and ReusedBrowser is sound, but as evidenced by #681 the migration is risky. I'll close this PR and turn it into smaller chunks, in a sequence roughly like this:
|
|
here's a version of this PR that just changes the UI, and doesn't do any refactoring: #682 |
2 participants
Adds UI for showing a list of browsers, each with a "Pick Locator" and "Close" button. This uses the data from microsoft/playwright#37027. On old versions, we try to infer the browser information from the currently selected project.
While the buttons are per-browser in the UI, they currently always affect all browsers. I'll fix this in a follow-up, but it should be fine given most people only ever use a single browser at a time anways.
Screen.Recording.2025-08-14.at.15.51.47.mov