chore(vscode): browser_connect via MCP Switcher and out of process backend #823
Conversation
Skn0tt
changed the title
Skn0tt
changed the title
Skn0tt
changed the title
|
Can we go lower level and implement it in src/mcp? |
src/mcp/server.ts
Outdated
| onChangeProxyTarget: ServerBackend['onChangeProxyTarget']; | ||
| } | ||
|
|
||
| export class ServerBackendProxy implements ServerBackend { |
src/program.ts
Outdated
| const proxy = new ServerBackendProxy( | ||
| new BrowserServerBackend(config, factories), | ||
| { | ||
| async onChangeProxyTarget(target) { |
There was a problem hiding this comment.
I'd inline this logic into ServerBackendProxy and have it accept a list of backed factories.
| this._target.onChangeProxyTarget = this._handleChangeProxyTarget; | ||
| if (this._initializeInfo) { | ||
| old.serverClosed?.(); | ||
| await this.initialize(this._initializeInfo); |
There was a problem hiding this comment.
| await this.initialize(this._initializeInfo); | |
| await this._target.initialize?.(this._initializeInfo); |
| constructor(private readonly _targetFactories: Record<string, (options: any) => ServerBackend>) { | ||
| const defaultTargetFactory = this._targetFactories['']; | ||
| this._target = defaultTargetFactory({}); | ||
| this._target.onChangeProxyTarget = this._handleChangeProxyTarget; |
There was a problem hiding this comment.
this._handleChangeProxyTarget.bind(this) ?
| private _initializeInfo?: InitializeInfo; | ||
|
|
||
| constructor(private readonly _targetFactories: Record<string, (options: any) => ServerBackend>) { | ||
| const defaultTargetFactory = this._targetFactories['']; |
There was a problem hiding this comment.
use "default" key for the default factory?
| @@ -210,6 +210,10 @@ export class Context { | |||
| for (const page of browserContext.pages()) | |||
| this._onPageCreated(page); | |||
| browserContext.on('page', page => this._onPageCreated(page)); | |||
| browserContext.on('close', () => { | |||
| this._browserContextPromise = undefined; | |||
| this._closeBrowserContextPromise = undefined; | |||
There was a problem hiding this comment.
Maybe we should resolve this promise in case there are waiting close calls?
| ].join('\n'), | ||
| inputSchema: z.object({ | ||
| method: z.enum(factories.map(factory => factory.name) as [string, ...string[]]).default(factories[0].name).describe('The method to use to connect to the browser'), | ||
| method: z.enum(contextSwitchers.map(c => c.name) as [string, ...string[]]).describe('The method to use to connect to the browser'), | ||
| options: askForOptions ? z.any().optional().describe('options for the connection method') : z.void(), |
There was a problem hiding this comment.
Do not define this parameter if askForOptions is false
| name: factory.name, | ||
| description: factory.description, | ||
| switch: async () => { | ||
| await this._setContextFactory(factory); |
There was a problem hiding this comment.
I don't like that we now mix them and have 2 ways to switch how we connect to the browser. Maybe migrate the context factories to the backend switchers as well or what is the plan?
| this._extraEnv = env; | ||
| } | ||
|
|
||
| async startRunner(runnerParams: any, options: { onStdOut?: (chunk: Buffer | string) => void, onStdErr?: (chunk: Buffer | string) => void } = {}): Promise<ProcessExitData | undefined> { |
| const old = this._target; | ||
| old.onChangeProxyTarget = undefined; | ||
| this._target = target; | ||
| this._target.onChangeProxyTarget = this._handleChangeProxyTarget; |
There was a problem hiding this comment.
Why do we have to assign onChangeProxyTarget to the target while we could just handle that particular tool call in this class ant spare all other backends from the knowledge about onChangeProxyTarget? Basically similar to how we switch between context factories but just intercept the backend switch call here.
No description provided.