Skip to content

Commit c645eee

Browse files
authored
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
1 parent ed7e95d commit c645eee

2 files changed

Lines changed: 13 additions & 7 deletions

File tree

src/McpContext.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -695,10 +695,13 @@ export class McpContext implements Context {
695695
);
696696
});
697697

698+
// Only fall back when the selected page is actually gone. Gating on
699+
// `isClosed()` instead of `#pages` membership avoids silently swapping a
700+
// live page that is momentarily missing from the snapshot, e.g., a
701+
// `devtools://` page, which is selectable but filtered out of `#pages` above.
698702
this.#selectedPageFallback = undefined;
699703
if (
700-
(!this.#selectedPage ||
701-
this.#pages.indexOf(this.#selectedPage.pptrPage) === -1) &&
704+
(!this.#selectedPage || this.#selectedPage.pptrPage.isClosed()) &&
702705
this.#pages[0]
703706
) {
704707
// Record the automatic change so the response can surface it. Skipped on

tests/McpContext.test.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,13 @@ describe('McpContext', () => {
175175
});
176176
});
177177

178-
it('reports the fallback when the selected page is missing from the list', async () => {
178+
it('keeps a still-open selected page that is missing from the list', async () => {
179179
await withMcpContext(async (_response, context) => {
180180
const page = await context.newPage();
181181
assert.ok(context.isPageSelected(page.pptrPage));
182182

183-
// A live page that is temporarily missing from the pages list.
183+
// A live page that is temporarily missing from the pages list must keep
184+
// its selection — only a genuinely closed page is replaced.
184185
const pages = await context.browser.pages();
185186
const stub = sinon
186187
.stub(context.browser, 'pages')
@@ -191,9 +192,11 @@ describe('McpContext', () => {
191192
stub.restore();
192193
}
193194

194-
const fallback = context.getSelectedPageFallback();
195-
assert.ok(fallback, 'fallback should be reported');
196-
assert.strictEqual(fallback.wasClosed, false);
195+
assert.ok(
196+
context.isPageSelected(page.pptrPage),
197+
'a still-open page should keep its selection',
198+
);
199+
assert.strictEqual(context.getSelectedPageFallback(), undefined);
197200
});
198201
});
199202

0 commit comments

Comments
 (0)