From 850c32811e4ebb2c1d393b5c8431bd78dfdd5b8a Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Mon, 17 Nov 2025 14:35:59 +0100 Subject: [PATCH] refactor: clear issue aggregator on page navigation --- src/McpContext.ts | 5 + src/PageCollector.ts | 209 +++++++++++++++++++++++++----------- tests/PageCollector.test.ts | 4 - tests/tools/console.test.ts | 47 +++++++- tests/utils.ts | 6 +- 5 files changed, 198 insertions(+), 73 deletions(-) diff --git a/src/McpContext.ts b/src/McpContext.ts index 76b70515..1c3c988d 100644 --- a/src/McpContext.ts +++ b/src/McpContext.ts @@ -155,6 +155,11 @@ export class McpContext implements Context { await this.#consoleCollector.init(); } + dispose() { + this.#networkCollector.dispose(); + this.#consoleCollector.dispose(); + } + static async from( browser: Browser, logger: Debugger, diff --git a/src/PageCollector.ts b/src/PageCollector.ts index 62b28055..cff2148d 100644 --- a/src/PageCollector.ts +++ b/src/PageCollector.ts @@ -4,8 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { + AggregatedIssue, + Common, +} from '../node_modules/chrome-devtools-frontend/mcp/mcp.js'; import { - type AggregatedIssue, IssueAggregatorEvents, IssuesManagerEvents, createIssuesFromProtocolIssue, @@ -14,7 +17,12 @@ import { import {FakeIssuesManager} from './DevtoolsUtils.js'; import {logger} from './logger.js'; -import type {CDPSession, ConsoleMessage} from './third_party/index.js'; +import type { + CDPSession, + ConsoleMessage, + Protocol, + Target, +} from './third_party/index.js'; import { type Browser, type Frame, @@ -79,22 +87,31 @@ export class PageCollector { this.addPage(page); } - this.#browser.on('targetcreated', async target => { - const page = await target.page(); - if (!page) { - return; - } - this.addPage(page); - }); - this.#browser.on('targetdestroyed', async target => { - const page = await target.page(); - if (!page) { - return; - } - this.cleanupPageDestroyed(page); - }); + this.#browser.on('targetcreated', this.#onTargetCreated); + this.#browser.on('targetdestroyed', this.#onTargetDestroyed); + } + + dispose() { + this.#browser.off('targetcreated', this.#onTargetCreated); + this.#browser.off('targetdestroyed', this.#onTargetDestroyed); } + #onTargetCreated = async (target: Target) => { + const page = await target.page(); + if (!page) { + return; + } + this.addPage(page); + }; + + #onTargetDestroyed = async (target: Target) => { + const page = await target.page(); + if (!page) { + return; + } + this.cleanupPageDestroyed(page); + }; + public addPage(page: Page) { this.#initializePage(page); } @@ -210,68 +227,130 @@ export class PageCollector { export class ConsoleCollector extends PageCollector< ConsoleMessage | Error | AggregatedIssue > { + #subscribedPages = new WeakMap(); + override addPage(page: Page): void { - const subscribed = this.storage.has(page); super.addPage(page); - if (!subscribed) { - void this.subscribeForIssues(page); + if (!this.#subscribedPages.has(page)) { + const subscriber = new PageIssueSubscriber(page); + this.#subscribedPages.set(page, subscriber); + void subscriber.subscribe(); } } - async subscribeForIssues(page: Page) { - const seenKeys = new Set(); - const mockManager = new FakeIssuesManager(); - const aggregator = new IssueAggregator(mockManager); - aggregator.addEventListener( + protected override cleanupPageDestroyed(page: Page): void { + super.cleanupPageDestroyed(page); + this.#subscribedPages.get(page)?.unsubscribe(); + this.#subscribedPages.delete(page); + } +} + +class PageIssueSubscriber { + #issueManager = new FakeIssuesManager(); + #issueAggregator = new IssueAggregator(this.#issueManager); + #seenKeys = new Set(); + #seenIssues = new Set(); + #page: Page; + #session: CDPSession; + + constructor(page: Page) { + this.#page = page; + // @ts-expect-error use existing CDP client (internal Puppeteer API). + this.#session = this.#page._client() as CDPSession; + } + + #resetIssueAggregator() { + this.#issueManager = new FakeIssuesManager(); + if (this.#issueAggregator) { + this.#issueAggregator.removeEventListener( + IssueAggregatorEvents.AGGREGATED_ISSUE_UPDATED, + this.#onAggregatedissue, + ); + } + this.#issueAggregator = new IssueAggregator(this.#issueManager); + + this.#issueAggregator.addEventListener( IssueAggregatorEvents.AGGREGATED_ISSUE_UPDATED, - event => { - const withId = event.data as WithSymbolId; - // Emit aggregated issue only if it's a new one - if (withId[stableIdSymbol]) { - return; - } - page.emit('issue', event.data); - }, + this.#onAggregatedissue, ); + } + async subscribe() { + this.#resetIssueAggregator(); + this.#page.on('framenavigated', this.#onFrameNavigated); + this.#session.on('Audits.issueAdded', this.#onIssueAdded); try { - // @ts-expect-error use existing CDP client (internal Puppeteer API). - const session = page._client() as CDPSession; - session.on('Audits.issueAdded', data => { - try { - const inspectorIssue = data.issue; - // @ts-expect-error Types of protocol from Puppeteer and CDP are - // incomparable for InspectorIssueCode, one is union, other is enum. - const issue = createIssuesFromProtocolIssue(null, inspectorIssue)[0]; - if (!issue) { - logger('No issue mapping for for the issue: ', inspectorIssue.code); - return; - } - - const primaryKey = issue.primaryKey(); - if (seenKeys.has(primaryKey)) { - return; - } - seenKeys.add(primaryKey); - - mockManager.dispatchEventToListeners( - IssuesManagerEvents.ISSUE_ADDED, - { - issue, - // @ts-expect-error We don't care that issues model is null - issuesModel: null, - }, - ); - } catch (error) { - logger('Error creating a new issue', error); - } - }); - - await session.send('Audits.enable'); + await this.#session.send('Audits.enable'); } catch (error) { logger('Error subscribing to issues', error); } } + + unsubscribe() { + this.#seenKeys.clear(); + this.#seenIssues.clear(); + this.#page.off('framenavigated', this.#onFrameNavigated); + this.#session.off('Audits.issueAdded', this.#onIssueAdded); + if (this.#issueAggregator) { + this.#issueAggregator.removeEventListener( + IssueAggregatorEvents.AGGREGATED_ISSUE_UPDATED, + this.#onAggregatedissue, + ); + } + void this.#session.send('Audits.disable').catch(() => { + // might fail. + }); + } + + #onAggregatedissue = ( + event: Common.EventTarget.EventTargetEvent, + ) => { + if (this.#seenIssues.has(event.data)) { + return; + } + this.#seenIssues.add(event.data); + this.#page.emit('issue', event.data); + }; + + // On navigation, we reset issue aggregation. + #onFrameNavigated = (frame: Frame) => { + // Only split the storage on main frame navigation + if (frame !== frame.page().mainFrame()) { + return; + } + this.#seenKeys.clear(); + this.#seenIssues.clear(); + this.#resetIssueAggregator(); + }; + + #onIssueAdded = (data: Protocol.Audits.IssueAddedEvent) => { + try { + const inspectorIssue = data.issue; + // @ts-expect-error Types of protocol from Puppeteer and CDP are + // incomparable for InspectorIssueCode, one is union, other is enum. + const issue = createIssuesFromProtocolIssue(null, inspectorIssue)[0]; + if (!issue) { + logger('No issue mapping for for the issue: ', inspectorIssue.code); + return; + } + + const primaryKey = issue.primaryKey(); + if (this.#seenKeys.has(primaryKey)) { + return; + } + this.#seenKeys.add(primaryKey); + this.#issueManager.dispatchEventToListeners( + IssuesManagerEvents.ISSUE_ADDED, + { + issue, + // @ts-expect-error We don't care that issues model is null + issuesModel: null, + }, + ); + } catch (error) { + logger('Error creating a new issue', error); + } + }; } export class NetworkCollector extends PageCollector { diff --git a/tests/PageCollector.test.ts b/tests/PageCollector.test.ts index e0b3a2a0..06438183 100644 --- a/tests/PageCollector.test.ts +++ b/tests/PageCollector.test.ts @@ -12,7 +12,6 @@ import type { HTTPRequest, Page, Target, - CDPSession, Protocol, } from 'puppeteer-core'; import sinon from 'sinon'; @@ -60,9 +59,6 @@ function getMockPage(): Page { mainFrame() { return mainFrame; }, - createCDPSession() { - return Promise.resolve(cdpSession as unknown as CDPSession); - }, ...mockListener(), // @ts-expect-error internal API. _client() { diff --git a/tests/tools/console.test.ts b/tests/tools/console.test.ts index 5a6f7b8c..770ccb4f 100644 --- a/tests/tools/console.test.ts +++ b/tests/tools/console.test.ts @@ -41,15 +41,14 @@ describe('console', () => { }); }); - it('lists issues messages', async () => { + it('lists issues', async () => { await withBrowser(async (response, context) => { const page = await context.newPage(); const issuePromise = new Promise(resolve => { - page.on('issue', () => { + page.once('issue', () => { resolve(); }); }); - await page.setContent(''); await issuePromise; await listConsoleMessages.handler({params: {}}, response, context); @@ -63,6 +62,48 @@ describe('console', () => { }); }); + it('lists issues after a page reload', async () => { + await withBrowser(async (response, context) => { + const page = await context.newPage(); + const issuePromise = new Promise(resolve => { + page.once('issue', () => { + resolve(); + }); + }); + + await page.setContent(''); + await issuePromise; + await listConsoleMessages.handler({params: {}}, response, context); + { + const formattedResponse = await response.handle('test', context); + const textContent = formattedResponse[0] as {text: string}; + assert.ok( + textContent.text.includes( + `msgid=1 [issue] An element doesn't have an autocomplete attribute (count: 1)`, + ), + ); + } + + const anotherIssuePromise = new Promise(resolve => { + page.once('issue', () => { + resolve(); + }); + }); + await page.reload(); + await page.setContent(''); + await anotherIssuePromise; + { + const formattedResponse = await response.handle('test', context); + const textContent = formattedResponse[0] as {text: string}; + assert.ok( + textContent.text.includes( + `msgid=2 [issue] An element doesn't have an autocomplete attribute (count: 1)`, + ), + ); + } + }); + }); + it('work with primitive unhandled errors', async () => { await withBrowser(async (response, context) => { const page = await context.newPage(); diff --git a/tests/utils.ts b/tests/utils.ts index 99f82b05..a1f97b90 100644 --- a/tests/utils.ts +++ b/tests/utils.ts @@ -18,6 +18,7 @@ import {McpResponse} from '../src/McpResponse.js'; import {stableIdSymbol} from '../src/PageCollector.js'; const browsers = new Map(); +let context: McpContext | undefined; export async function withBrowser( cb: (response: McpResponse, context: McpContext) => Promise, @@ -48,7 +49,10 @@ export async function withBrowser( }), ); const response = new McpResponse(); - const context = await McpContext.from( + if (context) { + context.dispose(); + } + context = await McpContext.from( browser, logger('test'), {