fix(replay): Adjust slow/multi click handling

mydea · mydea · commit 2921c5eff23d · 2023-06-22T14:59:52.000+02:00
diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts
@@ -58,3 +58,105 @@ sentryTest('captures multi click when not detecting slow click', async ({ getLoc
     },
   ]);
 });
+
+sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page }) => {
+  if (shouldSkipReplayTest()) {
+    sentryTest.skip();
+  }
+
+  const reqPromise0 = waitForReplayRequest(page, 0);
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    return route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify({ id: 'test-id' }),
+    });
+  });
+
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  await page.goto(url);
+  await reqPromise0;
+
+  let lastMultiClickSegmentId: number | undefined;
+
+  const reqPromise1 = waitForReplayRequest(page, (event, res) => {
+    const { breadcrumbs } = getCustomRecordingEvents(res);
+
+    const check = !lastMultiClickSegmentId && breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick');
+    if (check) {
+      lastMultiClickSegmentId = event.segment_id;
+    }
+    return check;
+  });
+
+  const reqPromise2 = waitForReplayRequest(page, (event, res) => {
+    const { breadcrumbs } = getCustomRecordingEvents(res);
+
+    const check =
+      !!lastMultiClickSegmentId &&
+      event.segment_id > lastMultiClickSegmentId &&
+      breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick');
+    if (check) {
+      lastMultiClickSegmentId = event.segment_id;
+    }
+    return check;
+  });
+
+  await page.click('#mutationButtonImmediately', { clickCount: 4 });
+
+  await new Promise(resolve => setTimeout(resolve, 1001));
+
+  await page.click('#mutationButtonImmediately', { clickCount: 2 });
+
+  const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1);
+  const { breadcrumbs: breadcrumb2 } = getCustomRecordingEvents(await reqPromise2);
+
+  const slowClickBreadcrumbs = breadcrumbs
+    .concat(breadcrumb2)
+    .filter(breadcrumb => breadcrumb.category === 'ui.multiClick');
+
+  expect(slowClickBreadcrumbs).toEqual([
+    {
+      category: 'ui.multiClick',
+      type: 'default',
+      data: {
+        clickCount: 6,
+        metric: true,
+        node: {
+          attributes: {
+            id: 'mutationButtonImmediately',
+          },
+          id: expect.any(Number),
+          tagName: 'button',
+          textContent: '******* ******** ***********',
+        },
+        nodeId: expect.any(Number),
+        url: 'http://sentry-test.io/index.html',
+      },
+      message: 'body > button#mutationButtonImmediately',
+      timestamp: expect.any(Number),
+    },
+    {
+      category: 'ui.multiClick',
+      type: 'default',
+      data: {
+        clickCount: 2,
+        metric: true,
+        node: {
+          attributes: {
+            id: 'mutationButtonImmediately',
+          },
+          id: expect.any(Number),
+          tagName: 'button',
+          textContent: '******* ******** ***********',
+        },
+        nodeId: expect.any(Number),
+        url: 'http://sentry-test.io/index.html',
+      },
+      message: 'body > button#mutationButtonImmediately',
+      timestamp: expect.any(Number),
+    },
+  ]);
+});
diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts
@@ -42,8 +42,6 @@ export const CONSOLE_ARG_MAX_SIZE = 5_000;
 export const SLOW_CLICK_THRESHOLD = 3_000;
 /* For scroll actions after a click, we only look for a very short time period to detect programmatic scrolling. */
 export const SLOW_CLICK_SCROLL_TIMEOUT = 300;
-/* Clicks in this time period are considered e.g. double/triple clicks. */
-export const MULTI_CLICK_TIMEOUT = 1_000;
 
 /** When encountering a total segment size exceeding this size, stop the replay (as we cannot properly ingest it). */
 export const REPLAY_MAX_EVENT_BUFFER_SIZE = 20_000_000; // ~20MB
diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts
@@ -33,7 +33,6 @@ export class ClickDetector implements ReplayClickDetector {
   private _clicks: Click[] = [];
   private _teardown: undefined | (() => void);
 
-  private _multiClickTimeout: number;
   private _threshold: number;
   private _scollTimeout: number;
   private _timeout: number;
@@ -51,7 +50,6 @@ export class ClickDetector implements ReplayClickDetector {
   ) {
     // We want everything in s, but options are in ms
     this._timeout = slowClickConfig.timeout / 1000;
-    this._multiClickTimeout = slowClickConfig.multiClickTimeout / 1000;
     this._threshold = slowClickConfig.threshold / 1000;
     this._scollTimeout = slowClickConfig.scrollTimeout / 1000;
     this._replay = replay;
@@ -126,13 +124,6 @@ export class ClickDetector implements ReplayClickDetector {
       return;
     }
 
-    const click = this._getClick(node);
-
-    if (click) {
-      // this means a click on the same element was captured in the last 1s, so we consider this a multi click
-      return;
-    }
-
     const newClick: Click = {
       timestamp: breadcrumb.timestamp,
       clickBreadcrumb: breadcrumb,
@@ -150,22 +141,14 @@ export class ClickDetector implements ReplayClickDetector {
 
   /** Count multiple clicks on elements. */
   private _handleMultiClick(node: HTMLElement): void {
-    const click = this._getClick(node);
-
-    if (!click) {
-      return;
-    }
-
-    click.clickCount++;
+    this._getClicks(node).forEach(click => {
+      click.clickCount++;
+    });
   }
 
-  /** Try to get an existing click on the given element. */
-  private _getClick(node: HTMLElement): Click | undefined {
-    const now = nowInSeconds();
-
-    // Find any click on the same element in the last second
-    // If one exists, we consider this click as a double/triple/etc click
-    return this._clicks.find(click => click.node === node && now - click.timestamp < this._multiClickTimeout);
+  /** Get all pending clicks for a given node. */
+  private _getClicks(node: HTMLElement): Click[] {
+    return this._clicks.filter(click => click.node === node);
   }
 
   /** Check the clicks that happened. */
@@ -182,13 +165,6 @@ export class ClickDetector implements ReplayClickDetector {
         click.scrollAfter = click.timestamp <= this._lastScroll ? this._lastScroll - click.timestamp : undefined;
       }
 
-      // If an action happens after the multi click threshold, we can skip waiting and handle the click right away
-      const actionTime = click.scrollAfter || click.mutationAfter || 0;
-      if (actionTime && actionTime >= this._multiClickTimeout) {
-        timedOutClicks.push(click);
-        return;
-      }
-
       if (click.timestamp + this._timeout <= now) {
         timedOutClicks.push(click);
       }
@@ -269,6 +245,10 @@ export class ClickDetector implements ReplayClickDetector {
 
   /** Schedule to check current clicks. */
   private _scheduleCheckClicks(): void {
+    if (this._checkClickTimeout) {
+      clearTimeout(this._checkClickTimeout);
+    }
+
     this._checkClickTimeout = setTimeout(() => this._checkClicks(), 1000);
   }
 }
diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts
@@ -7,7 +7,6 @@ import { logger } from '@sentry/utils';
 import {
   BUFFER_CHECKOUT_TIME,
   MAX_SESSION_LIFE,
-  MULTI_CLICK_TIMEOUT,
   SESSION_IDLE_EXPIRE_DURATION,
   SESSION_IDLE_PAUSE_DURATION,
   SLOW_CLICK_SCROLL_TIMEOUT,
@@ -175,7 +174,6 @@ export class ReplayContainer implements ReplayContainerInterface {
           timeout: slowClickTimeout,
           scrollTimeout: SLOW_CLICK_SCROLL_TIMEOUT,
           ignoreSelector: slowClickIgnoreSelectors ? slowClickIgnoreSelectors.join(',') : '',
-          multiClickTimeout: MULTI_CLICK_TIMEOUT,
         }
       : undefined;
 
diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts
@@ -476,5 +476,4 @@ export interface SlowClickConfig {
   timeout: number;
   scrollTimeout: number;
   ignoreSelector: string;
-  multiClickTimeout: number;
 }
diff --git a/packages/replay/test/unit/coreHandlers/handleClick.test.ts b/packages/replay/test/unit/coreHandlers/handleClick.test.ts
@@ -26,7 +26,6 @@ describe('Unit | coreHandlers | handleClick', () => {
           timeout: 3_000,
           scrollTimeout: 200,
           ignoreSelector: '',
-          multiClickTimeout: 1_000,
         },
         mockAddBreadcrumbEvent,
       );
@@ -72,113 +71,6 @@ describe('Unit | coreHandlers | handleClick', () => {
       expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1);
     });
 
-    test('it groups multiple clicks together', async () => {
-      const replay = {
-        getCurrentRoute: () => 'test-route',
-      } as ReplayContainer;
-
-      const mockAddBreadcrumbEvent = jest.fn();
-
-      const detector = new ClickDetector(
-        replay,
-        {
-          threshold: 1_000,
-          timeout: 3_000,
-          scrollTimeout: 200,
-          ignoreSelector: '',
-          multiClickTimeout: 1_000,
-        },
-        mockAddBreadcrumbEvent,
-      );
-
-      const breadcrumb1: Breadcrumb = {
-        timestamp: BASE_TIMESTAMP / 1000,
-        data: {
-          nodeId: 1,
-        },
-      };
-      const breadcrumb2: Breadcrumb = {
-        timestamp: BASE_TIMESTAMP / 1000 + 0.2,
-        data: {
-          nodeId: 1,
-        },
-      };
-      const breadcrumb3: Breadcrumb = {
-        timestamp: BASE_TIMESTAMP / 1000 + 0.6,
-        data: {
-          nodeId: 1,
-        },
-      };
-      const breadcrumb4: Breadcrumb = {
-        timestamp: BASE_TIMESTAMP / 1000 + 2,
-        data: {
-          nodeId: 1,
-        },
-      };
-      const breadcrumb5: Breadcrumb = {
-        timestamp: BASE_TIMESTAMP / 1000 + 2.9,
-        data: {
-          nodeId: 1,
-        },
-      };
-      const node = document.createElement('button');
-      detector.handleClick(breadcrumb1, node);
-
-      detector.handleClick(breadcrumb2, node);
-
-      detector.handleClick(breadcrumb3, node);
-
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0);
-
-      jest.advanceTimersByTime(2_000);
-
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0);
-
-      detector.handleClick(breadcrumb4, node);
-      detector.handleClick(breadcrumb5, node);
-
-      jest.advanceTimersByTime(1_000);
-
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1);
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, {
-        category: 'ui.slowClickDetected',
-        type: 'default',
-        data: {
-          // count is not actually correct, because this is identified by a different click handler
-          clickCount: 1,
-          endReason: 'timeout',
-          nodeId: 1,
-          route: 'test-route',
-          timeAfterClickMs: 3000,
-          url: 'http://localhost/',
-        },
-        message: undefined,
-        timestamp: expect.any(Number),
-      });
-
-      jest.advanceTimersByTime(2_000);
-
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(2);
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, {
-        category: 'ui.slowClickDetected',
-        type: 'default',
-        data: {
-          // count is not actually correct, because this is identified by a different click handler
-          clickCount: 1,
-          endReason: 'timeout',
-          nodeId: 1,
-          route: 'test-route',
-          timeAfterClickMs: 3000,
-          url: 'http://localhost/',
-        },
-        message: undefined,
-        timestamp: expect.any(Number),
-      });
-
-      jest.advanceTimersByTime(5_000);
-      expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(2);
-    });
-
     test('it captures clicks on different elements', async () => {
       const replay = {
         getCurrentRoute: () => 'test-route',
@@ -193,7 +85,6 @@ describe('Unit | coreHandlers | handleClick', () => {
           timeout: 3_000,
           scrollTimeout: 200,
           ignoreSelector: '',
-          multiClickTimeout: 1_000,
         },
         mockAddBreadcrumbEvent,
       );
@@ -247,7 +138,6 @@ describe('Unit | coreHandlers | handleClick', () => {
           timeout: 3_000,
           scrollTimeout: 200,
           ignoreSelector: '',
-          multiClickTimeout: 1_000,
         },
         mockAddBreadcrumbEvent,
       );
@@ -304,7 +194,6 @@ describe('Unit | coreHandlers | handleClick', () => {
             timeout: 3_000,
             scrollTimeout: 200,
             ignoreSelector: '',
-            multiClickTimeout: 1_000,
           },
           mockAddBreadcrumbEvent,
         );
@@ -437,7 +326,6 @@ describe('Unit | coreHandlers | handleClick', () => {
             timeout: 3_000,
             scrollTimeout: 200,
             ignoreSelector: '',
-            multiClickTimeout: 1_000,
           },
           mockAddBreadcrumbEvent,
         );

Original file line number	Diff line number	Diff line change
`@@ -476,5 +476,4 @@ export interface SlowClickConfig {`
`476`	`476`	`timeout: number;`
`477`	`477`	`scrollTimeout: number;`
`478`	`478`	`ignoreSelector: string;`
`479`		`- multiClickTimeout: number;`
`480`	`479`	`}`