refactor(dashboard): turn emitAnnotate into a request/response API (#40907)

Author: Skn0tt

packages/playwright-core/src/tools/backend/devtools.ts

@@ -22,7 +22,7 @@ import { libPath } from '../../package';
 import { defineTabTool, defineTool } from './tool';
 import { elementSchema, optionalElementSchema } from './snapshot';
 
-import type { SubmittedAnnotationFrame } from '@dashboard/dashboardChannel';
+import type { AnnotateResult } from '../dashboard/dashboardController';
 
 const resume = defineTool({
   capability: 'devtools',
@@ -157,11 +157,12 @@ const annotate = defineTabTool({
       response.addTextResult('No annotations were submitted.');
       return;
     }
-    const { frames, feedback } = JSON.parse(text) as { frames: SubmittedAnnotationFrame[]; feedback: string };
-    if (!frames || frames.length === 0) {
+    const result = JSON.parse(text) as AnnotateResult;
+    if (result.type !== 'submitted' || result.frames.length === 0) {
       response.addTextResult('No annotations were submitted.');
       return;
     }
+    const { frames, feedback } = result;
     const date = new Date();
     if (feedback)
       response.addTextResult(feedback);

packages/playwright-core/src/tools/dashboard/dashboardApp.ts

@@ -32,7 +32,7 @@ import { RegistrySessionProvider } from './registrySessionProvider';
 import { IdentitySessionProvider } from './identitySessionProvider';
 
 import type * as api from '../../..';
-import type { SubmittedAnnotationFrame } from '@dashboard/dashboardChannel';
+import type { AnnotateResult } from './dashboardController';
 import type { SessionProvider } from './sessionProvider';
 
 // HMR: build-time flag — `true` in watch builds, `false` in release. esbuild
@@ -43,7 +43,7 @@ declare const __PW_HMR__: boolean;
 type DashboardServer = {
   url: string;
   reveal: (options: DashboardOptions) => Promise<void>;
-  triggerAnnotate: (socket: net.Socket) => Promise<void>;
+  triggerAnnotate: (signal: AbortSignal) => Promise<AnnotateResult>;
   close: () => Promise<void>;
 };
 
@@ -53,18 +53,6 @@ async function startDashboardServer(provider: SessionProvider, options: Dashboar
 
   const connections = new Set<DashboardConnection>();
   let connectionLanded = new ManualPromise<void>();
-  const waitingSockets = new Set<net.Socket>();
-
-  const submitAnnotation = (frames: SubmittedAnnotationFrame[], feedback: string) => {
-    if (waitingSockets.size === 0)
-      return;
-    const payload = JSON.stringify({ frames, feedback });
-    for (const socket of waitingSockets) {
-      socket.write(payload);
-      socket.end();
-    }
-    waitingSockets.clear();
-  };
 
   httpServer.createWebSocket(() => {
     let connection: DashboardConnection;
@@ -75,7 +63,7 @@ async function startDashboardServer(provider: SessionProvider, options: Dashboar
         connectionLanded = new ManualPromise<void>();
     }, () => {
       connectionLanded.resolve();
-    }, submitAnnotation);
+    });
     connections.add(connection);
     return connection;
   });
@@ -108,24 +96,15 @@ async function startDashboardServer(provider: SessionProvider, options: Dashboar
     }));
   };
 
-  const triggerAnnotate = async (socket: net.Socket) => {
-    waitingSockets.add(socket);
-    const cleanup = () => {
-      if (!waitingSockets.delete(socket))
-        return;
-      if (waitingSockets.size === 0) {
-        for (const connection of connections)
-          connection.emitCancelAnnotate();
-      }
-    };
-    socket.on('close', cleanup);
-    socket.on('error', cleanup);
-
+  const triggerAnnotate = async (cancellation: AbortSignal): Promise<AnnotateResult> => {
     await connectionLanded;
-    if (waitingSockets.size === 0)
-      return;
-    for (const connection of connections)
-      connection.emitAnnotate();
+    if (cancellation.aborted || connections.size === 0)
+      return { type: 'cancelled' };
+    // Multiple dashboard connections is theoretical today (one UI per daemon), server mode does not support annotate.
+    // If two ever land, the first to submit wins but the losers stay in
+    // annotation mode until their UI reloads — revisit if that becomes a real
+    // scenario.
+    return await Promise.race([...connections].map(c => c.emitAnnotate({ signal: cancellation })));
   };
 
   const close = () => httpServer.stop();
@@ -364,10 +343,14 @@ async function startApp(server: net.Server, options: DashboardOptions) {
       }
       const { page, server: dashboard } = await statePromise;
       if (parsed.annotate) {
+        const cancellation = new AbortController();
+        socket.on('close', () => cancellation.abort());
+        socket.on('error', () => cancellation.abort());
         try {
           await page?.bringToFront();
           await dashboard.reveal(parsed);
-          await dashboard.triggerAnnotate(socket);
+          const result = await dashboard.triggerAnnotate(cancellation.signal);
+          socket.end(JSON.stringify(result));
         } catch (e) {
           socket.end(e);
         }

packages/playwright-core/src/tools/dashboard/dashboardController.ts

@@ -32,6 +32,10 @@ import type { SubmittedAnnotationFrame, Tab } from '@dashboard/dashboardChannel'
 import type { BrowserDescriptor } from '../../serverRegistry';
 import type { SessionProvider } from './sessionProvider';
 
+export type AnnotateResult =
+  | { type: 'submitted', frames: SubmittedAnnotationFrame[], feedback: string }
+  | { type: 'cancelled' };
+
 export class DashboardConnection implements Transport {
   sendEvent?: (method: string, params: any) => void;
   close?: () => void;
@@ -40,20 +44,18 @@ export class DashboardConnection implements Transport {
   private _attachedPage: AttachedPage | undefined;
   private _onclose: () => void;
   private _onconnected?: () => void;
-  private _onAnnotationSubmit?: (frames: SubmittedAnnotationFrame[], feedback: string) => void;
   private _pushTabsScheduled = false;
   private _visible = true;
   private _pendingReveal: { sessionName?: string; workspaceDir?: string; pageId?: string; done: ManualPromise<void> } | undefined;
-  private _annotateWaitingForAttach = false;
+  private _pendingAnnotate: { resolve: (result: AnnotateResult) => void; dispose: () => void } | undefined;
 
   _recordingDir: string;
   _streams = new Map<string, { handle: fs.promises.FileHandle; path: string }>();
 
-  constructor(provider: SessionProvider, onclose: () => void, onconnected?: () => void, onAnnotationSubmit?: (frames: SubmittedAnnotationFrame[], feedback: string) => void) {
+  constructor(provider: SessionProvider, onclose: () => void, onconnected?: () => void) {
     this._provider = provider;
     this._onclose = onclose;
     this._onconnected = onconnected;
-    this._onAnnotationSubmit = onAnnotationSubmit;
     this._recordingDir = fs.mkdtempSync(path.join(os.tmpdir(), 'playwright-recordings-'));
   }
 
@@ -84,6 +86,7 @@ export class DashboardConnection implements Transport {
     // Reject any in-flight reveal so callers awaiting it don't hang.
     this._pendingReveal?.done.reject(new Error('Dashboard connection closed'));
     this._pendingReveal = undefined;
+    this._resolvePendingAnnotate({ type: 'cancelled' });
     for (const stream of this._streams.values()) {
       void stream.handle.close()
           .catch(() => {})
@@ -193,11 +196,11 @@ export class DashboardConnection implements Transport {
   }
 
   async submitAnnotation(params: { frames: SubmittedAnnotationFrame[]; feedback: string }) {
-    this._onAnnotationSubmit?.(params.frames, params.feedback);
+    this._resolvePendingAnnotate({ type: 'submitted', frames: params.frames, feedback: params.feedback });
   }
 
   async cancelAnnotation() {
-    this._onAnnotationSubmit?.([], '');
+    this._resolvePendingAnnotate({ type: 'cancelled' });
   }
 
   async reveal(params: { path: string }) {
@@ -245,18 +248,43 @@ export class DashboardConnection implements Transport {
     this.sendEvent?.('frame', { data, viewportWidth, viewportHeight });
   }
 
-  emitAnnotate() {
+  emitAnnotate({ signal }: { signal: AbortSignal }): Promise<AnnotateResult> {
+    return new Promise<AnnotateResult>(resolve => {
+      if (signal.aborted) {
+        resolve({ type: 'cancelled' });
+        return;
+      }
+      // Latest emitAnnotate supersedes any in-flight one on the same connection.
+      this._resolvePendingAnnotate({ type: 'cancelled' });
+      const onAbort = () => {
+        if (this._pendingAnnotate !== pending)
+          return;
+        this._pendingAnnotate = undefined;
+        pending.dispose();
+        this.sendEvent?.('cancelAnnotate', {});
+        resolve({ type: 'cancelled' });
+      };
+      const pending: NonNullable<typeof this._pendingAnnotate> = {
+        resolve,
+        dispose: () => signal.removeEventListener('abort', onAbort),
+      };
+      this._pendingAnnotate = pending;
+      signal.addEventListener('abort', onAbort);
+      this._tryFireAnnotate();
+    });
+  }
+
+  private _tryFireAnnotate() {
     // Defer until a page is attached so the client can fetch a screenshot.
-    if (!this._attachedPage) {
-      this._annotateWaitingForAttach = true;
+    if (!this._pendingAnnotate || !this._attachedPage)
       return;
-    }
     this.sendEvent?.('annotate', {});
   }
 
-  emitCancelAnnotate() {
-    this._annotateWaitingForAttach = false;
-    this.sendEvent?.('cancelAnnotate', {});
+  private _resolvePendingAnnotate(result: AnnotateResult) {
+    this._pendingAnnotate?.resolve(result);
+    this._pendingAnnotate?.dispose();
+    this._pendingAnnotate = undefined;
   }
 
   artifactsDirFor(context: api.BrowserContext): string {
@@ -326,10 +354,8 @@ export class DashboardConnection implements Transport {
       attached.dispose();
       throw e;
     }
-    if (this._annotateWaitingForAttach && this._attachedPage === attached) {
-      this._annotateWaitingForAttach = false;
-      this.sendEvent?.('annotate', {});
-    }
+    if (this._attachedPage === attached)
+      this._tryFireAnnotate();
   }
 
   _handleAttachedPageClose(context: api.BrowserContext) {