diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts index 2d84eeb29ac3..82bc436f714e 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts @@ -1,7 +1,12 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../../utils/fixtures'; -import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; +import { + getCustomRecordingEvents, + shouldSkipReplayTest, + waitForReplayRequest, + waitForReplayRequests, +} from '../../../../utils/replayHelpers'; sentryTest('captures multi click when not detecting slow click', async ({ getLocalTestUrl, page }) => { if (shouldSkipReplayTest()) { @@ -58,3 +63,97 @@ sentryTest('captures multi click when not detecting slow click', async ({ getLoc }, ]); }); + +sentryTest('captures multiple multi clicks', async ({ getLocalTestUrl, page, forceFlushReplay }) => { + 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 multiClickBreadcrumbCount = 0; + + const reqsPromise = waitForReplayRequests(page, (_event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + const count = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick').length; + + multiClickBreadcrumbCount += count; + + if (multiClickBreadcrumbCount === 2) { + return true; + } + + return false; + }); + + await page.click('#mutationButtonImmediately', { clickCount: 4 }); + await forceFlushReplay(); + + // Ensure we waited at least 1s, which is the threshold to create a new ui.click breadcrumb + await new Promise(resolve => setTimeout(resolve, 1001)); + + await page.click('#mutationButtonImmediately', { clickCount: 2 }); + await forceFlushReplay(); + + const responses = await reqsPromise; + + const slowClickBreadcrumbs = responses + .flatMap(res => getCustomRecordingEvents(res).breadcrumbs) + .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/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index ec332edea74d..4860b894a836 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -101,6 +101,46 @@ export function waitForReplayRequest( ); } +/** + * Wait until a callback returns true, collecting all replay responses along the way. + * This can be useful when you don't know if stuff will be in one or multiple replay requests. + */ +export function waitForReplayRequests( + page: Page, + callback: (event: ReplayEvent, res: Response) => boolean, + timeout?: number, +): Promise { + const responses: Response[] = []; + + return new Promise(resolve => { + void page.waitForResponse( + res => { + const req = res.request(); + + const event = getReplayEventFromRequest(req); + + if (!event) { + return false; + } + + responses.push(res); + + try { + if (callback(event, res)) { + resolve(responses); + return true; + } + + return false; + } catch { + return false; + } + }, + timeout ? { timeout } : undefined, + ); + }); +} + export function isReplayEvent(event: Event): event is ReplayEvent { return event.type === 'replay_event'; } diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index 699cefe8b784..86dc228c50bf 100644 --- 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 index c5a65ddfdca0..5f7e46dbcbce 100644 --- 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 index 8fab410a0c34..c72306239533 100644 --- 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 index e2cfddd0f525..0e351c27a984 100644 --- 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 index 8b06e4683656..f2980e60df69 100644 --- 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, );