fix: keep a still-open selected page instead of falling back to the first page (#2328)
Follow-up to #2304. `createPagesSnapshot()` reselects `#pages[0]` whenever the selected page is missing from `#pages`, even when that page is still open. So a live selection can be silently swapped, not only a closed one. The case that hit us is a `devtools://` page: - the server connects with `handleDevToolsAsPage: true` (`src/browser.ts`), so DevTools frontends are pages and land in `#mcpPages` - `#pages` filters `devtools://` out (`src/McpContext.ts`, unless `experimentalDevToolsDebugging` is set) - `getPageById()` (used by `select_page`) reads `#mcpPages`, not `#pages`, so a devtools page can be selected even though `list_pages` never lists it - the next snapshot finds it missing from `#pages` and, because the fallback keys on `#pages` membership rather than `isClosed()`, reselects `#pages[0]` with the page still open (`isClosed() === false`) This reproduces on a normal Chrome (details and a repro in #2304). This change gates the fallback on `isClosed()`, as proposed in #2304. A still-open selection is kept; a genuinely closed page still auto-advances, so the browser-like behavior for closed tabs is unchanged. One consequence: the "is no longer listed" wording added in #2308 becomes unreachable, since a live page missing from the list no longer triggers a fallback. I left it in place to keep this diff focused, but I'm happy to simplify it here or in a follow-up. The unit test that covered the missing-but-open case now asserts the selection is retained. Refs: #2304
This commit is contained in:
+5
-2
@@ -695,10 +695,13 @@ export class McpContext implements Context {
|
||||
);
|
||||
});
|
||||
|
||||
// Only fall back when the selected page is actually gone. Gating on
|
||||
// `isClosed()` instead of `#pages` membership avoids silently swapping a
|
||||
// live page that is momentarily missing from the snapshot, e.g., a
|
||||
// `devtools://` page, which is selectable but filtered out of `#pages` above.
|
||||
this.#selectedPageFallback = undefined;
|
||||
if (
|
||||
(!this.#selectedPage ||
|
||||
this.#pages.indexOf(this.#selectedPage.pptrPage) === -1) &&
|
||||
(!this.#selectedPage || this.#selectedPage.pptrPage.isClosed()) &&
|
||||
this.#pages[0]
|
||||
) {
|
||||
// Record the automatic change so the response can surface it. Skipped on
|
||||
|
||||
@@ -175,12 +175,13 @@ describe('McpContext', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('reports the fallback when the selected page is missing from the list', async () => {
|
||||
it('keeps a still-open selected page that is missing from the list', async () => {
|
||||
await withMcpContext(async (_response, context) => {
|
||||
const page = await context.newPage();
|
||||
assert.ok(context.isPageSelected(page.pptrPage));
|
||||
|
||||
// A live page that is temporarily missing from the pages list.
|
||||
// A live page that is temporarily missing from the pages list must keep
|
||||
// its selection — only a genuinely closed page is replaced.
|
||||
const pages = await context.browser.pages();
|
||||
const stub = sinon
|
||||
.stub(context.browser, 'pages')
|
||||
@@ -191,9 +192,11 @@ describe('McpContext', () => {
|
||||
stub.restore();
|
||||
}
|
||||
|
||||
const fallback = context.getSelectedPageFallback();
|
||||
assert.ok(fallback, 'fallback should be reported');
|
||||
assert.strictEqual(fallback.wasClosed, false);
|
||||
assert.ok(
|
||||
context.isPageSelected(page.pptrPage),
|
||||
'a still-open page should keep its selection',
|
||||
);
|
||||
assert.strictEqual(context.getSelectedPageFallback(), undefined);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user