refactor: clear issue aggregator on page navigation

Author: OrKoN

src/PageCollector.ts

@@ -4,8 +4,8 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import type {AggregatedIssue} from '../node_modules/chrome-devtools-frontend/mcp/mcp.js';
 import {
-  type AggregatedIssue,
   IssueAggregatorEvents,
   IssuesManagerEvents,
   createIssuesFromProtocolIssue,
@@ -14,7 +14,11 @@ 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,
+} from './third_party/index.js';
 import {
   type Browser,
   type Frame,
@@ -210,68 +214,92 @@ export class PageCollector<T> {
 export class ConsoleCollector extends PageCollector<
   ConsoleMessage | Error | AggregatedIssue
 > {
+  #subscribedPages = new WeakSet<Page>();
+  #issueManager = new FakeIssuesManager();
+  #seenKeys = new Set<string>();
+  #seenIssues = new Set<AggregatedIssue>();
+
   override addPage(page: Page): void {
-    const subscribed = this.storage.has(page);
     super.addPage(page);
-    if (!subscribed) {
-      void this.subscribeForIssues(page);
-    }
+    void this.subscribeForIssues(page);
   }
 
-  async subscribeForIssues(page: Page) {
-    const seenKeys = new Set<string>();
-    const mockManager = new FakeIssuesManager();
-    const aggregator = new IssueAggregator(mockManager);
-    aggregator.addEventListener(
+  #resetIssueAggregator(page: Page) {
+    this.#issueManager = new FakeIssuesManager();
+    new IssueAggregator(this.#issueManager).addEventListener(
       IssueAggregatorEvents.AGGREGATED_ISSUE_UPDATED,
       event => {
-        const withId = event.data as WithSymbolId<AggregatedIssue>;
-        // Emit aggregated issue only if it's a new one
-        if (withId[stableIdSymbol]) {
+        if (this.#seenIssues.has(event.data)) {
           return;
         }
+        this.#seenIssues.add(event.data);
         page.emit('issue', event.data);
       },
     );
+  }
 
+  async subscribeForIssues(page: Page) {
+    if (this.#subscribedPages.has(page)) {
+      return;
+    }
+    this.#subscribedPages.add(page);
+    this.#resetIssueAggregator(page);
     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);
-        }
-      });
-
+      page.on('framenavigated', this.#onFrameNavigated);
+      session.on('Audits.issueAdded', this.#onIssueAdded);
       await session.send('Audits.enable');
     } catch (error) {
       logger('Error subscribing to issues', error);
     }
   }
+
+  // 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(frame.page());
+  };
+
+  #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);
+    }
+  };
+
+  protected override cleanupPageDestroyed(page: Page): void {
+    super.cleanupPageDestroyed(page);
+    this.#subscribedPages.delete(page);
+    page.off('framenavigated', this.#onFrameNavigated);
+  }
 }
 
 export class NetworkCollector extends PageCollector<HTTPRequest> {

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() {

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<void>(resolve => {
-          page.on('issue', () => {
+          page.once('issue', () => {
             resolve();
           });
         });
-
         await page.setContent('<input type="text" name="username" />');
         await issuePromise;
         await listConsoleMessages.handler({params: {}}, response, context);
@@ -63,6 +62,49 @@ describe('console', () => {
       });
     });
 
+    it('lists issues after a page reload', async () => {
+      await withBrowser(async (response, context) => {
+        const page = await context.newPage();
+        const issuePromise = new Promise<void>(resolve => {
+          page.once('issue', () => {
+            resolve();
+          });
+        });
+
+        await page.setContent('<input type="text" name="username" />');
+        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<void>(resolve => {
+          page.once('issue', () => {
+            resolve();
+          });
+        });
+        await page.reload();
+        await page.setContent('<input type="text" name="username" />');
+        await anotherIssuePromise;
+        {
+          const formattedResponse = await response.handle('test', context);
+          const textContent = formattedResponse[0] as {text: string};
+          console.log(textContent.text);
+          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();