chore: simplify signal handling while recording (#32624)

packages/playwright-core/src/server/codegen/language.ts

@@ -20,6 +20,7 @@ import type * as types from '../types';
 import type { ActionInContext, LanguageGenerator, LanguageGeneratorOptions } from './types';
 
 export function generateCode(actions: ActionInContext[], languageGenerator: LanguageGenerator, options: LanguageGeneratorOptions) {
+  actions = collapseActions(actions);
   const header = languageGenerator.generateHeader(options);
   const footer = languageGenerator.generateFooter(options.saveStorage);
   const actionTexts = actions.map(a => languageGenerator.generateAction(a)).filter(Boolean);
@@ -83,3 +84,19 @@ export function toClickOptionsForSourceCode(action: actions.ClickAction): types.
     options.position = action.position;
   return options;
 }
+
+function collapseActions(actions: ActionInContext[]): ActionInContext[] {
+  const result: ActionInContext[] = [];
+  for (const action of actions) {
+    const lastAction = result[result.length - 1];
+    const isSameAction = lastAction && lastAction.action.name === action.action.name && lastAction.frame.pageAlias === action.frame.pageAlias && lastAction.frame.framePath.join('|') === action.frame.framePath.join('|');
+    const isSameSelector = lastAction && 'selector' in lastAction.action && 'selector' in action.action && action.action.selector === lastAction.action.selector;
+    const shouldMerge = isSameAction && (action.action.name === 'navigate' || (action.action.name === 'fill' && isSameSelector));
+    if (!shouldMerge) {
+      result.push(action);
+      continue;
+    }
+    result[result.length - 1] = action;
+  }
+  return result;
+}

packages/playwright-core/src/server/codegen/types.ts

@@ -36,7 +36,7 @@ export type ActionInContext = {
   frame: FrameDescription;
   description?: string;
   action: actions.Action;
-  committed?: boolean;
+  timestamp: number;
 };
 
 export interface LanguageGenerator {

packages/playwright-core/src/server/recorder/contextRecorder.ts

@@ -18,7 +18,7 @@ import type * as channels from '@protocol/channels';
 import type { Source } from '@recorder/recorderTypes';
 import { EventEmitter } from 'events';
 import * as recorderSource from '../../generated/recorderSource';
-import { eventsHelper, isUnderTest, monotonicTime, quoteCSSAttributeValue, type RegisteredListener } from '../../utils';
+import { eventsHelper, monotonicTime, quoteCSSAttributeValue, type RegisteredListener } from '../../utils';
 import { raceAgainstDeadline } from '../../utils/timeoutRunner';
 import { BrowserContext } from '../browserContext';
 import type { ActionInContext, FrameDescription, LanguageGeneratorOptions, Language, LanguageGenerator } from '../codegen/types';
@@ -27,7 +27,6 @@ import type { Dialog } from '../dialog';
 import { Frame } from '../frames';
 import { Page } from '../page';
 import type * as actions from './recorderActions';
-import { performAction } from './recorderRunner';
 import { ThrottledFile } from './throttledFile';
 import { RecorderCollection } from './recorderCollection';
 import { generateCode } from '../codegen/language';
@@ -48,7 +47,6 @@ export class ContextRecorder extends EventEmitter {
   private _lastPopupOrdinal = 0;
   private _lastDialogOrdinal = -1;
   private _lastDownloadOrdinal = -1;
-  private _timers = new Set<NodeJS.Timeout>();
   private _context: BrowserContext;
   private _params: channels.BrowserContextRecorderSupplementEnableParams;
   private _delegate: ContextRecorderDelegate;
@@ -150,9 +148,6 @@ export class ContextRecorder extends EventEmitter {
   }
 
   dispose() {
-    for (const timer of this._timers)
-      clearTimeout(timer);
-    this._timers.clear();
     eventsHelper.removeEventListeners(this._listeners);
   }
 
@@ -162,11 +157,11 @@ export class ContextRecorder extends EventEmitter {
     page.on('close', () => {
       this._collection.addRecordedAction({
         frame: this._describeMainFrame(page),
-        committed: true,
         action: {
           name: 'closePage',
           signals: [],
-        }
+        },
+        timestamp: monotonicTime()
       });
       this._pageAliases.delete(page);
     });
@@ -184,12 +179,12 @@ export class ContextRecorder extends EventEmitter {
     } else {
       this._collection.addRecordedAction({
         frame: this._describeMainFrame(page),
-        committed: true,
         action: {
           name: 'openPage',
           url: page.mainFrame().url(),
           signals: [],
-        }
+        },
+        timestamp: monotonicTime()
       });
     }
   }
@@ -220,54 +215,24 @@ export class ContextRecorder extends EventEmitter {
     return this._params.testIdAttributeName || this._context.selectors().testIdAttributeName() || 'data-testid';
   }
 
-  private async _performAction(frame: Frame, action: actions.PerformOnRecordAction) {
-    // Commit last action so that no further signals are added to it.
-    this._collection.commitLastAction();
-
+  private async _createActionInContext(frame: Frame, action: actions.Action): Promise<ActionInContext> {
     const frameDescription = await this._describeFrame(frame);
     const actionInContext: ActionInContext = {
       frame: frameDescription,
       action,
       description: undefined,
+      timestamp: monotonicTime()
     };
-
     await this._delegate.rewriteActionInContext?.(this._pageAliases, actionInContext);
+    return actionInContext;
+  }
 
-    const callMetadata = await this._collection.willPerformAction(actionInContext);
-    if (!callMetadata)
-      return;
-    const error = await performAction(callMetadata, this._pageAliases, actionInContext).then(() => undefined).catch((e: Error) => e);
-    await this._collection.didPerformAction(callMetadata, actionInContext, error);
-    if (error)
-      actionInContext.committed = true;
-    else
-      this._setCommittedAfterTimeout(actionInContext);
+  private async _performAction(frame: Frame, action: actions.PerformOnRecordAction) {
+    await this._collection.performAction(await this._createActionInContext(frame, action));
   }
 
   private async _recordAction(frame: Frame, action: actions.Action) {
-    // Commit last action so that no further signals are added to it.
-    this._collection.commitLastAction();
-
-    const frameDescription = await this._describeFrame(frame);
-    const actionInContext: ActionInContext = {
-      frame: frameDescription,
-      action,
-      description: undefined,
-    };
-
-    await this._delegate.rewriteActionInContext?.(this._pageAliases, actionInContext);
-
-    this._setCommittedAfterTimeout(actionInContext);
-    this._collection.addRecordedAction(actionInContext);
-  }
-
-  private _setCommittedAfterTimeout(actionInContext: ActionInContext) {
-    const timer = setTimeout(() => {
-      // Commit the action after 5 seconds so that no further signals are added to it.
-      actionInContext.committed = true;
-      this._timers.delete(timer);
-    }, isUnderTest() ? 500 : 5000);
-    this._timers.add(timer);
+    this._collection.addRecordedAction(await this._createActionInContext(frame, action));
   }
 
   private _onFrameNavigated(frame: Frame, page: Page) {

packages/playwright-core/src/server/recorder/recorderCollection.ts

@@ -19,13 +19,14 @@ import type { Frame } from '../frames';
 import type { Page } from '../page';
 import type { Signal } from './recorderActions';
 import type { ActionInContext } from '../codegen/types';
-import type { CallMetadata } from '@protocol/callMetadata';
-import { createGuid } from '../../utils/crypto';
 import { monotonicTime } from '../../utils/time';
-import { mainFrameForAction, traceParamsForAction } from './recorderUtils';
+import { callMetadataForAction } from './recorderUtils';
+import { serializeError } from '../errors';
+import { performAction } from './recorderRunner';
+import type { CallMetadata } from '@protocol/callMetadata';
+import { isUnderTest } from '../../utils/debug';
 
 export class RecorderCollection extends EventEmitter {
-  private _lastAction: ActionInContext | null = null;
   private _actions: ActionInContext[] = [];
   private _enabled: boolean;
   private _pageAliases: Map<Page, string>;
@@ -38,7 +39,6 @@ export class RecorderCollection extends EventEmitter {
   }
 
   restart() {
-    this._lastAction = null;
     this._actions = [];
     this.emit('change');
   }
@@ -51,98 +51,73 @@ export class RecorderCollection extends EventEmitter {
     this._enabled = enabled;
   }
 
-  async willPerformAction(actionInContext: ActionInContext): Promise<CallMetadata | null> {
-    if (!this._enabled)
-      return null;
-    const { callMetadata, mainFrame } = this._callMetadataForAction(actionInContext);
-    await mainFrame.instrumentation.onBeforeCall(mainFrame, callMetadata);
-    this._lastAction = actionInContext;
-    return callMetadata;
-  }
-
-  private _callMetadataForAction(actionInContext: ActionInContext): { callMetadata: CallMetadata, mainFrame: Frame } {
-    const mainFrame = mainFrameForAction(this._pageAliases, actionInContext);
-    const { action } = actionInContext;
-    const callMetadata: CallMetadata = {
-      id: `call@${createGuid()}`,
-      apiName: 'frame.' + action.name,
-      objectId: mainFrame.guid,
-      pageId: mainFrame._page.guid,
-      frameId: mainFrame.guid,
-      startTime: monotonicTime(),
-      endTime: 0,
-      type: 'Frame',
-      method: action.name,
-      params: traceParamsForAction(actionInContext),
-      log: [],
-    };
-    return { callMetadata, mainFrame };
-  }
-
-  async didPerformAction(callMetadata: CallMetadata, actionInContext: ActionInContext, error?: Error) {
-    if (!this._enabled)
-      return;
-
-    if (!error)
-      this._actions.push(actionInContext);
-
-    const mainFrame = mainFrameForAction(this._pageAliases, actionInContext);
-    callMetadata.endTime = monotonicTime();
-    await mainFrame.instrumentation.onAfterCall(mainFrame, callMetadata);
-
-    this.emit('change');
+  async performAction(actionInContext: ActionInContext) {
+    await this._addAction(actionInContext, async callMetadata => {
+      await performAction(callMetadata, this._pageAliases, actionInContext);
+    });
   }
 
   addRecordedAction(actionInContext: ActionInContext) {
-    if (!this._enabled)
-      return;
-    const action = actionInContext.action;
-
-    const lastAction = this._lastAction && this._lastAction.frame.pageAlias === actionInContext.frame.pageAlias ? this._lastAction.action : undefined;
-    if (lastAction && action.name === 'navigate' && lastAction.name === 'navigate' && action.url === lastAction.url) {
-      // Already at a target URL.
-      return;
-    }
-
-    if (lastAction && action.name === 'fill' && lastAction.name === 'fill' && action.selector === lastAction.selector)
-      this._actions.pop();
-
-    this._lastAction = actionInContext;
+    if (['openPage', 'closePage'].includes(actionInContext.action.name)) {
       this._actions.push(actionInContext);
       this.emit('change');
+      return;
+    }
+    this._addAction(actionInContext).catch(() => {});
   }
 
-  commitLastAction() {
+  private async _addAction(actionInContext: ActionInContext, callback?: (callMetadata: CallMetadata) => Promise<void>) {
     if (!this._enabled)
       return;
-    const action = this._lastAction;
-    if (action)
-      action.committed = true;
+
+    const { callMetadata, mainFrame } = callMetadataForAction(this._pageAliases, actionInContext);
+    await mainFrame.instrumentation.onBeforeCall(mainFrame, callMetadata);
+    this._actions.push(actionInContext);
+    this.emit('change');
+    const error = await callback?.(callMetadata).catch((e: Error) => e);
+    callMetadata.endTime = monotonicTime();
+    callMetadata.error = error ? serializeError(error) : undefined;
+    await mainFrame.instrumentation.onAfterCall(mainFrame, callMetadata);
   }
 
   signal(pageAlias: string, frame: Frame, signal: Signal) {
     if (!this._enabled)
       return;
 
-    if (this._lastAction && !this._lastAction.committed) {
-      this._lastAction.action.signals.push(signal);
-      this.emit('change');
-      return;
-    }
-
     if (signal.name === 'navigation' && frame._page.mainFrame() === frame) {
+      const timestamp = monotonicTime();
+      const lastAction = this._actions[this._actions.length - 1];
+      const signalThreshold = isUnderTest() ? 500 : 5000;
+
+      let generateGoto = false;
+      if (!lastAction)
+        generateGoto = true;
+      else if (lastAction.action.name !== 'click' && lastAction.action.name !== 'press')
+        generateGoto = true;
+      else if (timestamp - lastAction.timestamp > signalThreshold)
+        generateGoto = true;
+
+      if (generateGoto) {
         this.addRecordedAction({
           frame: {
             pageAlias,
             framePath: [],
           },
-        committed: true,
           action: {
             name: 'navigate',
             url: frame.url(),
             signals: [],
           },
+          timestamp
         });
       }
+      return;
+    }
+
+    if (this._actions.length) {
+      this._actions[this._actions.length - 1].action.signals.push(signal);
+      this.emit('change');
+      return;
+    }
   }
 }

packages/playwright-core/src/server/recorder/recorderUtils.ts

@@ -22,6 +22,7 @@ import type { Frame } from '../frames';
 import type * as actions from './recorderActions';
 import { toKeyboardModifiers } from '../codegen/language';
 import { serializeExpectedTextValues } from '../../utils/expectUtils';
+import { createGuid, monotonicTime } from '../../utils';
 
 export function metadataToCallLog(metadata: CallMetadata, status: CallLogStatus): CallLog {
   let title = metadata.apiName || metadata.method;
@@ -59,7 +60,7 @@ export function mainFrameForAction(pageAliases: Map<Page, string>, actionInConte
   const pageAlias = actionInContext.frame.pageAlias;
   const page = [...pageAliases.entries()].find(([, alias]) => pageAlias === alias)?.[0];
   if (!page)
-    throw new Error('Internal error: page not found');
+    throw new Error(`Internal error: page ${pageAlias} not found in [${[...pageAliases.values()]}]`);
   return page.mainFrame();
 }
 
@@ -129,3 +130,22 @@ export function traceParamsForAction(actionInContext: ActionInContext) {
     }
   }
 }
+
+export function callMetadataForAction(pageAliases: Map<Page, string>, actionInContext: ActionInContext): { callMetadata: CallMetadata, mainFrame: Frame } {
+  const mainFrame = mainFrameForAction(pageAliases, actionInContext);
+  const { action } = actionInContext;
+  const callMetadata: CallMetadata = {
+    id: `call@${createGuid()}`,
+    apiName: 'frame.' + action.name,
+    objectId: mainFrame.guid,
+    pageId: mainFrame._page.guid,
+    frameId: mainFrame.guid,
+    startTime: monotonicTime(),
+    endTime: 0,
+    type: 'Frame',
+    method: action.name,
+    params: traceParamsForAction(actionInContext),
+    log: [],
+  };
+  return { callMetadata, mainFrame };
+}

tests/library/inspector/cli-codegen-1.spec.ts

@@ -706,7 +706,7 @@ var page1 = await page.RunAndWaitForPopupAsync(async () =>
     expect(popup.url()).toBe('about:blank');
   });
 
-  test('should assert navigation', async ({ page, openRecorder }) => {
+  test('should attribute navigation to click', async ({ page, openRecorder }) => {
     const recorder = await openRecorder();
 
     await recorder.setContentAndWait(`<a onclick="window.location.href='about:blank#foo'">link</a>`);
@@ -720,24 +720,42 @@ var page1 = await page.RunAndWaitForPopupAsync(async () =>
     ]);
 
     expect.soft(sources.get('JavaScript')!.text).toContain(`
-  await page.getByText('link').click();`);
+  await page.goto('about:blank');
+  await page.getByText('link').click();
+
+  // ---------------------
+  await context.close();`);
 
     expect.soft(sources.get('Playwright Test')!.text).toContain(`
-  await page.getByText('link').click();`);
+  await page.goto('about:blank');
+  await page.getByText('link').click();
+});`);
 
     expect.soft(sources.get('Java')!.text).toContain(`
-      page.getByText("link").click();`);
+      page.navigate(\"about:blank\");
+      page.getByText(\"link\").click();
+    }`);
 
     expect.soft(sources.get('Python')!.text).toContain(`
-    page.get_by_text("link").click()`);
+    page.goto("about:blank")
+    page.get_by_text("link").click()
+
+    # ---------------------
+    context.close()`);
 
     expect.soft(sources.get('Python Async')!.text).toContain(`
-    await page.get_by_text("link").click()`);
+    await page.goto("about:blank")
+    await page.get_by_text("link").click()
+
+    # ---------------------
+    await context.close()`);
 
     expect.soft(sources.get('Pytest')!.text).toContain(`
+    page.goto("about:blank")
     page.get_by_text("link").click()`);
 
     expect.soft(sources.get('C#')!.text).toContain(`
+await page.GotoAsync("about:blank");
 await page.GetByText("link").ClickAsync();`);
 
     expect(page.url()).toContain('about:blank#foo');