From 6046a3ee03825106f0d84a7676cab9da358a020d Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Thu, 8 Jan 2026 15:02:21 +0100 Subject: [PATCH] refactor: change to page ids --- docs/tool-reference.md | 4 ++-- src/McpContext.ts | 22 +++++++++++++++----- src/McpResponse.ts | 4 +--- src/tools/ToolDefinition.ts | 5 +++-- src/tools/pages.ts | 14 ++++++------- tests/McpResponse.test.js.snapshot | 2 +- tests/index.test.ts | 4 ++-- tests/tools/pages.test.ts | 32 +++++++++++++++--------------- 8 files changed, 48 insertions(+), 39 deletions(-) diff --git a/docs/tool-reference.md b/docs/tool-reference.md index 618a1f486..8a5191ec6 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -130,7 +130,7 @@ **Parameters:** -- **pageIdx** (number) **(required)**: The index of the page to close. Call [`list_pages`](#list_pages) to list pages. +- **pageId** (number) **(required)**: The ID of the page to close. Call [`list_pages`](#list_pages) to list pages. --- @@ -172,7 +172,7 @@ **Parameters:** -- **pageIdx** (number) **(required)**: The index of the page to select. Call [`list_pages`](#list_pages) to get available pages. +- **pageId** (number) **(required)**: The ID of the page to select. Call [`list_pages`](#list_pages) to get available pages. - **bringToFront** (boolean) _(optional)_: Whether to focus the page and bring it to the top. --- diff --git a/src/McpContext.ts b/src/McpContext.ts index 11bb3d971..aa848493d 100644 --- a/src/McpContext.ts +++ b/src/McpContext.ts @@ -111,6 +111,9 @@ export class McpContext implements Context { #geolocationMap = new WeakMap(); #dialog?: Dialog; + #pageIdMap = new WeakMap(); + #nextPageId = 1; + #nextSnapshotId = 1; #traceResults: TraceResult[] = []; @@ -246,11 +249,11 @@ export class McpContext implements Context { this.#consoleCollector.addPage(page); return page; } - async closePage(pageIdx: number): Promise { + async closePage(pageId: number): Promise { if (this.#pages.length === 1) { throw new Error(CLOSE_PAGE_ERROR); } - const page = this.getPageByIdx(pageIdx); + const page = this.getPageById(pageId); await page.close({runBeforeUnload: false}); } @@ -327,15 +330,18 @@ export class McpContext implements Context { return page; } - getPageByIdx(idx: number): Page { - const pages = this.#pages; - const page = pages[idx]; + getPageById(pageId: number): Page { + const page = this.#pages.find(p => this.#pageIdMap.get(p) === pageId); if (!page) { throw new Error('No page found'); } return page; } + getPageId(page: Page): number | undefined { + return this.#pageIdMap.get(page); + } + #dialogHandler = (dialog: Dialog): void => { this.#dialog = dialog; }; @@ -417,6 +423,12 @@ export class McpContext implements Context { this.#options.experimentalIncludeAllPages, ); + for (const page of allPages) { + if (!this.#pageIdMap.has(page)) { + this.#pageIdMap.set(page, this.#nextPageId++); + } + } + this.#pages = allPages.filter(page => { // If we allow debugging DevTools windows, return all pages. // If we are in regular mode, the user should only see non-DevTools page. diff --git a/src/McpResponse.ts b/src/McpResponse.ts index f18a12f45..6db0d1831 100644 --- a/src/McpResponse.ts +++ b/src/McpResponse.ts @@ -385,12 +385,10 @@ Call ${handleDialog.name} to handle it before continuing.`); if (this.#includePages) { const parts = [`## Pages`]; - let idx = 0; for (const page of context.getPages()) { parts.push( - `${idx}: ${page.url()}${context.isPageSelected(page) ? ' [selected]' : ''}`, + `${context.getPageId(page)}: ${page.url()}${context.isPageSelected(page) ? ' [selected]' : ''}`, ); - idx++; } response.push(...parts); } diff --git a/src/tools/ToolDefinition.ts b/src/tools/ToolDefinition.ts index d017640cd..f3eaaa7eb 100644 --- a/src/tools/ToolDefinition.ts +++ b/src/tools/ToolDefinition.ts @@ -89,10 +89,11 @@ export type Context = Readonly<{ getSelectedPage(): Page; getDialog(): Dialog | undefined; clearDialog(): void; - getPageByIdx(idx: number): Page; + getPageById(pageId: number): Page; + getPageId(page: Page): number | undefined; isPageSelected(page: Page): boolean; newPage(): Promise; - closePage(pageIdx: number): Promise; + closePage(pageId: number): Promise; selectPage(page: Page): void; getElementByUid(uid: string): Promise>; getAXNodeByUid(uid: string): TextSnapshotNode | undefined; diff --git a/src/tools/pages.ts b/src/tools/pages.ts index 8b2402894..cfdf99db0 100644 --- a/src/tools/pages.ts +++ b/src/tools/pages.ts @@ -31,10 +31,10 @@ export const selectPage = defineTool({ readOnlyHint: true, }, schema: { - pageIdx: zod + pageId: zod .number() .describe( - `The index of the page to select. Call ${listPages.name} to get available pages.`, + `The ID of the page to select. Call ${listPages.name} to get available pages.`, ), bringToFront: zod .boolean() @@ -42,7 +42,7 @@ export const selectPage = defineTool({ .describe('Whether to focus the page and bring it to the top.'), }, handler: async (request, response, context) => { - const page = context.getPageByIdx(request.params.pageIdx); + const page = context.getPageById(request.params.pageId); context.selectPage(page); response.setIncludePages(true); if (request.params.bringToFront) { @@ -59,15 +59,13 @@ export const closePage = defineTool({ readOnlyHint: false, }, schema: { - pageIdx: zod + pageId: zod .number() - .describe( - 'The index of the page to close. Call list_pages to list pages.', - ), + .describe('The ID of the page to close. Call list_pages to list pages.'), }, handler: async (request, response, context) => { try { - await context.closePage(request.params.pageIdx); + await context.closePage(request.params.pageId); } catch (err) { if (err.message === CLOSE_PAGE_ERROR) { response.appendResponseLine(err.message); diff --git a/tests/McpResponse.test.js.snapshot b/tests/McpResponse.test.js.snapshot index 61a417c81..5f5f79045 100644 --- a/tests/McpResponse.test.js.snapshot +++ b/tests/McpResponse.test.js.snapshot @@ -83,7 +83,7 @@ Testing 2 exports[`McpResponse > list pages 1`] = ` # test response ## Pages -0: about:blank [selected] +1: about:blank [selected] `; exports[`McpResponse > returns correctly formatted snapshot for a simple tree 1`] = ` diff --git a/tests/index.test.ts b/tests/index.test.ts index f31d1a84c..5970e890b 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -51,7 +51,7 @@ describe('e2e', () => { content: [ { type: 'text', - text: '# list_pages response\n## Pages\n0: about:blank [selected]', + text: '# list_pages response\n## Pages\n1: about:blank [selected]', }, ], }); @@ -72,7 +72,7 @@ describe('e2e', () => { content: [ { type: 'text', - text: '# list_pages response\n## Pages\n0: about:blank [selected]', + text: '# list_pages response\n## Pages\n1: about:blank [selected]', }, ], }); diff --git a/tests/tools/pages.test.ts b/tests/tools/pages.test.ts index b10c1d9bf..46cbc2ad0 100644 --- a/tests/tools/pages.test.ts +++ b/tests/tools/pages.test.ts @@ -32,13 +32,13 @@ describe('pages', () => { describe('new_page', () => { it('create a page', async () => { await withMcpContext(async (response, context) => { - assert.strictEqual(context.getPageByIdx(0), context.getSelectedPage()); + assert.strictEqual(context.getPageById(1), context.getSelectedPage()); await newPage.handler( {params: {url: 'about:blank'}}, response, context, ); - assert.strictEqual(context.getPageByIdx(1), context.getSelectedPage()); + assert.strictEqual(context.getPageById(2), context.getSelectedPage()); assert.ok(response.includePages); }); }); @@ -47,9 +47,9 @@ describe('pages', () => { it('closes a page', async () => { await withMcpContext(async (response, context) => { const page = await context.newPage(); - assert.strictEqual(context.getPageByIdx(1), context.getSelectedPage()); - assert.strictEqual(context.getPageByIdx(1), page); - await closePage.handler({params: {pageIdx: 1}}, response, context); + assert.strictEqual(context.getPageById(2), context.getSelectedPage()); + assert.strictEqual(context.getPageById(2), page); + await closePage.handler({params: {pageId: 2}}, response, context); assert.ok(page.isClosed()); assert.ok(response.includePages); }); @@ -57,7 +57,7 @@ describe('pages', () => { it('cannot close the last page', async () => { await withMcpContext(async (response, context) => { const page = context.getSelectedPage(); - await closePage.handler({params: {pageIdx: 0}}, response, context); + await closePage.handler({params: {pageId: 1}}, response, context); assert.deepStrictEqual( response.responseLines[0], `The last open page cannot be closed. It is fine to keep it open.`, @@ -71,24 +71,24 @@ describe('pages', () => { it('selects a page', async () => { await withMcpContext(async (response, context) => { await context.newPage(); - assert.strictEqual(context.getPageByIdx(1), context.getSelectedPage()); - await selectPage.handler({params: {pageIdx: 0}}, response, context); - assert.strictEqual(context.getPageByIdx(0), context.getSelectedPage()); + assert.strictEqual(context.getPageById(2), context.getSelectedPage()); + await selectPage.handler({params: {pageId: 1}}, response, context); + assert.strictEqual(context.getPageById(1), context.getSelectedPage()); assert.ok(response.includePages); }); }); it('selects a page and keeps it focused in the background', async () => { await withMcpContext(async (response, context) => { await context.newPage(); - assert.strictEqual(context.getPageByIdx(1), context.getSelectedPage()); + assert.strictEqual(context.getPageById(2), context.getSelectedPage()); assert.strictEqual( - await context.getPageByIdx(0).evaluate(() => document.hasFocus()), + await context.getPageById(1).evaluate(() => document.hasFocus()), false, ); - await selectPage.handler({params: {pageIdx: 0}}, response, context); - assert.strictEqual(context.getPageByIdx(0), context.getSelectedPage()); + await selectPage.handler({params: {pageId: 1}}, response, context); + assert.strictEqual(context.getPageById(1), context.getSelectedPage()); assert.strictEqual( - await context.getPageByIdx(0).evaluate(() => document.hasFocus()), + await context.getPageById(1).evaluate(() => document.hasFocus()), true, ); assert.ok(response.includePages); @@ -115,8 +115,8 @@ describe('pages', () => { it('throws an error if the page was closed not by the MCP server', async () => { await withMcpContext(async (response, context) => { const page = await context.newPage(); - assert.strictEqual(context.getPageByIdx(1), context.getSelectedPage()); - assert.strictEqual(context.getPageByIdx(1), page); + assert.strictEqual(context.getPageById(2), context.getSelectedPage()); + assert.strictEqual(context.getPageById(2), page); await page.close();