refactor: clean up page snapshot generation (#2348)
- remove obsolete devtools page detection step - move isolated context processing out of the browser pages fetching - move filtering of pages out of getPages(). That should just be a getter.
This commit is contained in:
+58
-103
@@ -145,7 +145,7 @@ export class McpContext implements Context {
|
||||
if (!page) {
|
||||
return;
|
||||
}
|
||||
this.#createMcpPage(page);
|
||||
void this.#createMcpPage(page);
|
||||
} catch (err) {
|
||||
this.logger?.('Error handling targetcreated', err);
|
||||
}
|
||||
@@ -298,13 +298,13 @@ export class McpContext implements Context {
|
||||
} else {
|
||||
page = await this.browser.newPage({background});
|
||||
}
|
||||
const mcpPage = this.#createMcpPage(page);
|
||||
const mcpPage = await this.#createMcpPage(page);
|
||||
await this.createPagesSnapshot();
|
||||
this.selectPage(mcpPage);
|
||||
return mcpPage;
|
||||
}
|
||||
async closePage(pageId: number): Promise<void> {
|
||||
if (this.getPages().length === 1) {
|
||||
if (this.#mcpPages.size === 1) {
|
||||
throw new Error(CLOSE_PAGE_ERROR);
|
||||
}
|
||||
const page = this.getPageById(pageId);
|
||||
@@ -341,6 +341,10 @@ export class McpContext implements Context {
|
||||
return this.#options.performanceCrux;
|
||||
}
|
||||
|
||||
getPages(): McpPage[] {
|
||||
return Array.from(this.#mcpPages.values());
|
||||
}
|
||||
|
||||
getSelectedMcpPage(): McpPage {
|
||||
const page = this.#selectedPage;
|
||||
if (!page) {
|
||||
@@ -355,11 +359,7 @@ export class McpContext implements Context {
|
||||
}
|
||||
|
||||
getPageById(pageId: number): McpPage {
|
||||
// Resolve only among listed pages (`getPages()`), so an id that
|
||||
// `list_pages` never showed cannot be targeted (e.g. a `devtools://`
|
||||
// frontend, which the listing excludes unless `experimentalDevToolsDebugging`
|
||||
// is set).
|
||||
const page = this.getPages().find(mcpPage => mcpPage.id === pageId);
|
||||
const page = this.#mcpPages.values().find(mcpPage => mcpPage.id === pageId);
|
||||
if (!page) {
|
||||
throw new Error('No page found');
|
||||
}
|
||||
@@ -426,27 +426,46 @@ export class McpContext implements Context {
|
||||
return this.#serviceWorkerConsoleCollector.getData(extensionId);
|
||||
}
|
||||
|
||||
#createMcpPage(page: Page): McpPage {
|
||||
#getBrowserContextToNameMap(): Map<BrowserContext, string> {
|
||||
// Build a reverse lookup from BrowserContext instance → name.
|
||||
const contextToName = new Map<BrowserContext, string>();
|
||||
for (const [name, ctx] of this.#isolatedContexts) {
|
||||
contextToName.set(ctx, name);
|
||||
}
|
||||
const defaultCtx = this.browser.defaultBrowserContext();
|
||||
// Auto-discover BrowserContexts not in our mapping (e.g., externally
|
||||
// created incognito contexts) and assign generated names.
|
||||
const knownContexts = new Set(this.#isolatedContexts.values());
|
||||
for (const ctx of this.browser.browserContexts()) {
|
||||
if (ctx !== defaultCtx && !ctx.closed && !knownContexts.has(ctx)) {
|
||||
const name = `isolated-context-${this.#nextIsolatedContextId++}`;
|
||||
this.#isolatedContexts.set(name, ctx);
|
||||
contextToName.set(ctx, name);
|
||||
}
|
||||
}
|
||||
return contextToName;
|
||||
}
|
||||
|
||||
async #createMcpPage(page: Page): Promise<McpPage> {
|
||||
let mcpPage = this.#mcpPages.get(page);
|
||||
if (!mcpPage) {
|
||||
mcpPage = new McpPage(page, this.#nextPageId++, {
|
||||
locatorClass: this.#locatorClass,
|
||||
hasNetworkBlockOrAllowlist: this.#hasNetworkBlockOrAllowlist,
|
||||
isolatedContextName: this.#getBrowserContextToNameMap().get(
|
||||
page.browserContext(),
|
||||
),
|
||||
});
|
||||
this.#mcpPages.set(page, mcpPage);
|
||||
void mcpPage.init();
|
||||
await mcpPage.init();
|
||||
}
|
||||
return mcpPage;
|
||||
}
|
||||
|
||||
async createPagesSnapshot(): Promise<Page[]> {
|
||||
const {pages: allPages, isolatedContextNames} =
|
||||
await this.#fetchBrowserPages();
|
||||
const allPages = await this.#fetchBrowserPages();
|
||||
|
||||
for (const page of allPages) {
|
||||
const mcpPage = this.#createMcpPage(page);
|
||||
mcpPage.isolatedContextName = isolatedContextNames.get(page);
|
||||
}
|
||||
await Promise.allSettled(allPages.map(page => this.#createMcpPage(page)));
|
||||
|
||||
// Prune orphaned #mcpPages entries (pages that no longer exist).
|
||||
const currentPages = new Set(allPages);
|
||||
@@ -457,7 +476,7 @@ export class McpContext implements Context {
|
||||
}
|
||||
}
|
||||
|
||||
const pages = this.getPages();
|
||||
const pages = Array.from(this.#mcpPages.values());
|
||||
|
||||
// Only fall back when the selected page is actually gone. Gating on
|
||||
// `isClosed()` instead of `pages` membership avoids silently swapping a
|
||||
@@ -477,19 +496,18 @@ export class McpContext implements Context {
|
||||
this.selectPage(pages[0]);
|
||||
}
|
||||
|
||||
await this.detectOpenDevToolsWindows();
|
||||
|
||||
return pages.map(p => p.pptrPage);
|
||||
}
|
||||
|
||||
async #fetchBrowserPages(): Promise<{
|
||||
pages: Page[];
|
||||
isolatedContextNames: Map<Page, string>;
|
||||
}> {
|
||||
const defaultCtx = this.browser.defaultBrowserContext();
|
||||
const allPages = await this.browser.pages(
|
||||
this.#options.experimentalIncludeAllPages,
|
||||
);
|
||||
async #fetchBrowserPages(): Promise<Page[]> {
|
||||
const allPages = (
|
||||
await this.browser.pages(this.#options.experimentalIncludeAllPages)
|
||||
).filter(page => {
|
||||
return (
|
||||
this.#options.experimentalDevToolsDebugging ||
|
||||
!page.url().startsWith('devtools://')
|
||||
);
|
||||
});
|
||||
|
||||
const allTargets = this.browser.targets();
|
||||
const extensionTargets = allTargets.filter(target => {
|
||||
@@ -499,78 +517,24 @@ export class McpContext implements Context {
|
||||
);
|
||||
});
|
||||
|
||||
for (const target of extensionTargets) {
|
||||
// Right now target.page() returns null for popup and side panel pages.
|
||||
let page = await target.page();
|
||||
if (!page) {
|
||||
// We need to cache pages instances for targets because target.asPage()
|
||||
// returns a new page instance every time.
|
||||
page = this.#extensionPages.get(target) ?? null;
|
||||
if (!page) {
|
||||
try {
|
||||
page = await target.asPage();
|
||||
this.#extensionPages.set(target, page);
|
||||
} catch (e) {
|
||||
this.logger?.('Failed to get page for extension target', e);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (page && !allPages.includes(page)) {
|
||||
allPages.push(page);
|
||||
}
|
||||
}
|
||||
|
||||
// Build a reverse lookup from BrowserContext instance → name.
|
||||
const contextToName = new Map<BrowserContext, string>();
|
||||
for (const [name, ctx] of this.#isolatedContexts) {
|
||||
contextToName.set(ctx, name);
|
||||
}
|
||||
|
||||
// Auto-discover BrowserContexts not in our mapping (e.g., externally
|
||||
// created incognito contexts) and assign generated names.
|
||||
const knownContexts = new Set(this.#isolatedContexts.values());
|
||||
for (const ctx of this.browser.browserContexts()) {
|
||||
if (ctx !== defaultCtx && !ctx.closed && !knownContexts.has(ctx)) {
|
||||
const name = `isolated-context-${this.#nextIsolatedContextId++}`;
|
||||
this.#isolatedContexts.set(name, ctx);
|
||||
contextToName.set(ctx, name);
|
||||
}
|
||||
}
|
||||
|
||||
// Map each page to its isolated context name (if any).
|
||||
const isolatedContextNames = new Map<Page, string>();
|
||||
for (const page of allPages) {
|
||||
const ctx = page.browserContext();
|
||||
const name = contextToName.get(ctx);
|
||||
if (name) {
|
||||
isolatedContextNames.set(page, name);
|
||||
}
|
||||
}
|
||||
|
||||
return {pages: allPages, isolatedContextNames};
|
||||
}
|
||||
|
||||
async detectOpenDevToolsWindows() {
|
||||
this.logger?.('Detecting open DevTools windows');
|
||||
|
||||
await Promise.all(
|
||||
Array.from(this.#mcpPages.values()).map(async mcpPage => {
|
||||
const page = mcpPage.pptrPage;
|
||||
// Prior to Chrome 144.0.7559.59, the command fails,
|
||||
// Some Electron apps still use older version
|
||||
// Fall back to not exposing DevTools at all.
|
||||
await Promise.allSettled(
|
||||
extensionTargets.map(async target => {
|
||||
try {
|
||||
if (await page.hasDevTools()) {
|
||||
mcpPage.devToolsPage = await page.openDevTools();
|
||||
} else {
|
||||
mcpPage.devToolsPage = undefined;
|
||||
let page = await target.page();
|
||||
if (!page) {
|
||||
page = await target.asPage();
|
||||
}
|
||||
} catch {
|
||||
mcpPage.devToolsPage = undefined;
|
||||
this.#extensionPages.set(target, page);
|
||||
if (page && !allPages.includes(page)) {
|
||||
allPages.push(page);
|
||||
}
|
||||
} catch (e) {
|
||||
this.logger?.('Failed to get page for extension target', e);
|
||||
}
|
||||
}),
|
||||
);
|
||||
|
||||
return allPages;
|
||||
}
|
||||
|
||||
getExtensionServiceWorkers(): ExtensionServiceWorker[] {
|
||||
@@ -583,15 +547,6 @@ export class McpContext implements Context {
|
||||
return this.#extensionServiceWorkerMap.get(extensionServiceWorker.target);
|
||||
}
|
||||
|
||||
getPages(): McpPage[] {
|
||||
return Array.from(this.#mcpPages.values()).filter(mcpPage => {
|
||||
return (
|
||||
this.#options.experimentalDevToolsDebugging ||
|
||||
!mcpPage.pptrPage.url().startsWith('devtools://')
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
async saveTemporaryFile(
|
||||
data: Uint8Array<ArrayBufferLike>,
|
||||
filename: string,
|
||||
|
||||
+35
-10
@@ -72,7 +72,6 @@ export class McpPage implements ContextPage {
|
||||
|
||||
// Metadata
|
||||
isolatedContextName?: string;
|
||||
devToolsPage?: Page;
|
||||
#devtoolsUniverse?: TargetUniverse;
|
||||
|
||||
// Dialog
|
||||
@@ -93,12 +92,14 @@ export class McpPage implements ContextPage {
|
||||
options: {
|
||||
hasNetworkBlockOrAllowlist: boolean;
|
||||
locatorClass: typeof Locator;
|
||||
isolatedContextName?: string;
|
||||
},
|
||||
) {
|
||||
this.#hasNetworkBlockOrAllowlist = options.hasNetworkBlockOrAllowlist;
|
||||
this.#locatorClass = options.locatorClass;
|
||||
this.pptrPage = page;
|
||||
this.id = id;
|
||||
this.isolatedContextName = options.isolatedContextName;
|
||||
this.#dialogHandler = (dialog: Dialog): void => {
|
||||
this.#dialog = dialog;
|
||||
};
|
||||
@@ -121,21 +122,31 @@ export class McpPage implements ContextPage {
|
||||
}
|
||||
|
||||
async init(): Promise<void> {
|
||||
if (this.#devtoolsUniverse) {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
this.#devtoolsUniverse = await createTargetUniverse(this.pptrPage);
|
||||
} catch (e) {
|
||||
logger?.('Failed to initialize DevTools universe', e);
|
||||
}
|
||||
await Promise.allSettled([
|
||||
this.#initDevToolsUniverseNoThrow(),
|
||||
this.#initFocusEmulationNoThrow(),
|
||||
]);
|
||||
}
|
||||
|
||||
async #initFocusEmulationNoThrow(): Promise<void> {
|
||||
// We emulate a focused page for all pages to support multi-agent workflows.
|
||||
void this.pptrPage.emulateFocusedPage(true).catch(error => {
|
||||
logger?.('Error turning on focused page emulation', error);
|
||||
});
|
||||
}
|
||||
|
||||
async #initDevToolsUniverseNoThrow(): Promise<void> {
|
||||
if (this.#devtoolsUniverse) {
|
||||
return undefined;
|
||||
}
|
||||
try {
|
||||
const session = await this.pptrPage.createCDPSession();
|
||||
this.#devtoolsUniverse = await createTargetUniverse(session);
|
||||
} catch (e) {
|
||||
logger?.('Failed to initialize DevTools universe', e);
|
||||
}
|
||||
}
|
||||
|
||||
get devtoolsUniverse(): TargetUniverse | undefined {
|
||||
return this.#devtoolsUniverse;
|
||||
}
|
||||
@@ -184,6 +195,20 @@ export class McpPage implements ContextPage {
|
||||
return this.networkCollector.getData(includePreservedRequests);
|
||||
}
|
||||
|
||||
async getDevToolsPage(): Promise<Page | undefined> {
|
||||
try {
|
||||
if (await this.pptrPage.hasDevTools()) {
|
||||
return await this.pptrPage.openDevTools();
|
||||
}
|
||||
return undefined;
|
||||
} catch {
|
||||
// Prior to Chrome 144.0.7559.59, the command fails,
|
||||
// Some Electron apps still use older version
|
||||
// Fall back to not exposing DevTools at all.
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
|
||||
getConsoleData(
|
||||
includePreservedMessages?: boolean,
|
||||
): Array<ConsoleMessage | Error | DevTools.AggregatedIssue | UncaughtError> {
|
||||
@@ -481,7 +506,7 @@ export class McpPage implements ContextPage {
|
||||
async getDevToolsData(): Promise<DevToolsData> {
|
||||
try {
|
||||
logger?.('Getting DevTools UI data');
|
||||
const devtoolsPage = this.devToolsPage;
|
||||
const devtoolsPage = await this.getDevToolsPage();
|
||||
if (!devtoolsPage) {
|
||||
logger?.('No DevTools page detected');
|
||||
return {};
|
||||
|
||||
@@ -214,7 +214,6 @@ export class ToolHandler {
|
||||
);
|
||||
const context = await this.getContext();
|
||||
logger?.(`${this.tool.name} context: resolved`);
|
||||
await context.detectOpenDevToolsWindows();
|
||||
const response = this.serverArgs.slim
|
||||
? new SlimMcpResponse(this.serverArgs)
|
||||
: new McpResponse(this.serverArgs);
|
||||
|
||||
@@ -8,7 +8,6 @@ import {DevTools} from '../third_party/index.js';
|
||||
import type {
|
||||
CDPSession,
|
||||
ConsoleMessage,
|
||||
Page,
|
||||
Protocol,
|
||||
} from '../third_party/index.js';
|
||||
|
||||
@@ -134,7 +133,7 @@ export interface TargetUniverse {
|
||||
}
|
||||
|
||||
export async function createTargetUniverse(
|
||||
page: Page,
|
||||
session: CDPSession,
|
||||
): Promise<TargetUniverse> {
|
||||
const settingStorage = new DevTools.Common.Settings.SettingsStorage({});
|
||||
const universe = new DevTools.Foundation.Universe.Universe({
|
||||
@@ -148,7 +147,6 @@ export async function createTargetUniverse(
|
||||
overrideAutoStartModels: new Set([DevTools.DebuggerModel]),
|
||||
});
|
||||
|
||||
const session = await page.createCDPSession();
|
||||
const connection = new PuppeteerDevToolsConnection(session);
|
||||
|
||||
const targetManager = universe.context.get(DevTools.TargetManager);
|
||||
|
||||
@@ -97,7 +97,7 @@ describe('McpContext', () => {
|
||||
async (_response, context) => {
|
||||
const page = await context.newPage();
|
||||
await context.createPagesSnapshot();
|
||||
assert.ok(page.devToolsPage);
|
||||
assert.ok(await page.getDevToolsPage());
|
||||
|
||||
// A devtools page is tracked but excluded from the listing, so its id
|
||||
// must not resolve through `getPageById()` (which backs `select_page`
|
||||
|
||||
@@ -47,7 +47,6 @@ describe('ToolHandler', () => {
|
||||
const mockContext = sinon.createStubInstance(McpContext);
|
||||
const mockPage = sinon.createStubInstance(McpPage);
|
||||
mockContext.getSelectedMcpPage.returns(mockPage);
|
||||
mockContext.detectOpenDevToolsWindows.resolves();
|
||||
|
||||
const toolMutex = new Mutex();
|
||||
const serverArgs = parseArguments('1.0.0', ['node', 'script.js'], {
|
||||
@@ -86,7 +85,6 @@ describe('ToolHandler', () => {
|
||||
};
|
||||
|
||||
const mockContext = sinon.createStubInstance(McpContext);
|
||||
mockContext.detectOpenDevToolsWindows.resolves();
|
||||
|
||||
const toolMutex = new Mutex();
|
||||
const serverArgs = parseArguments('1.0.0', ['node', 'script.js'], {
|
||||
@@ -129,7 +127,6 @@ describe('ToolHandler', () => {
|
||||
};
|
||||
|
||||
const mockContext = sinon.createStubInstance(McpContext);
|
||||
mockContext.detectOpenDevToolsWindows.resolves();
|
||||
|
||||
const toolMutex = new Mutex();
|
||||
const serverArgs = parseArguments('1.0.0', ['node', 'script.js'], {
|
||||
|
||||
@@ -23,7 +23,9 @@ describe('createTargetUniverse', () => {
|
||||
|
||||
it('works with a real browser', async () => {
|
||||
await withBrowser(async (browser, page) => {
|
||||
const targetUniverse = await createTargetUniverse(page);
|
||||
const targetUniverse = await createTargetUniverse(
|
||||
await page.createCDPSession(),
|
||||
);
|
||||
|
||||
assert.notStrictEqual(targetUniverse, null);
|
||||
});
|
||||
@@ -31,7 +33,9 @@ describe('createTargetUniverse', () => {
|
||||
|
||||
it('ignores pauses', async () => {
|
||||
await withBrowser(async (browser, page) => {
|
||||
const targetUniverse = await createTargetUniverse(page);
|
||||
const targetUniverse = await createTargetUniverse(
|
||||
await page.createCDPSession(),
|
||||
);
|
||||
assert.ok(targetUniverse);
|
||||
const model = targetUniverse.target.model(DevTools.DebuggerModel);
|
||||
assert.ok(model);
|
||||
@@ -50,7 +54,9 @@ describe('createTargetUniverse', () => {
|
||||
server.addHtmlRoute('/test', html`<div>Test</div>`);
|
||||
|
||||
await withBrowser(async (browser, page) => {
|
||||
const targetUniverse = await createTargetUniverse(page);
|
||||
const targetUniverse = await createTargetUniverse(
|
||||
await page.createCDPSession(),
|
||||
);
|
||||
assert.ok(targetUniverse);
|
||||
|
||||
const networkManager = targetUniverse.target.model(
|
||||
|
||||
Reference in New Issue
Block a user