Skip to content

Commit

Permalink
fix(replay): Adjust slow/multi click handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@mydea
mydea committed Jun 22, 2023
1 parent da2487e commit 3f3300e
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 147 deletions.
102 changes: 102 additions & 0 deletions packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
]);
});
2 changes: 0 additions & 2 deletions packages/replay/src/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
40 changes: 10 additions & 30 deletions packages/replay/src/coreHandlers/handleClick.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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. */
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/replay/src/replay.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down
1 change: 0 additions & 1 deletion packages/replay/src/types/replay.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,5 +476,4 @@ export interface SlowClickConfig {
timeout: number;
scrollTimeout: number;
ignoreSelector: string;
multiClickTimeout: number;
}
112 changes: 0 additions & 112 deletions packages/replay/test/unit/coreHandlers/handleClick.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ describe('Unit | coreHandlers | handleClick', () => {
timeout: 3_000,
scrollTimeout: 200,
ignoreSelector: '',
multiClickTimeout: 1_000,
},
mockAddBreadcrumbEvent,
);
Expand Down Expand Up @@ -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',
Expand All @@ -193,7 +85,6 @@ describe('Unit | coreHandlers | handleClick', () => {
timeout: 3_000,
scrollTimeout: 200,
ignoreSelector: '',
multiClickTimeout: 1_000,
},
mockAddBreadcrumbEvent,
);
Expand Down Expand Up @@ -247,7 +138,6 @@ describe('Unit | coreHandlers | handleClick', () => {
timeout: 3_000,
scrollTimeout: 200,
ignoreSelector: '',
multiClickTimeout: 1_000,
},
mockAddBreadcrumbEvent,
);
Expand Down Expand Up @@ -304,7 +194,6 @@ describe('Unit | coreHandlers | handleClick', () => {
timeout: 3_000,
scrollTimeout: 200,
ignoreSelector: '',
multiClickTimeout: 1_000,
},
mockAddBreadcrumbEvent,
);
Expand Down Expand Up @@ -437,7 +326,6 @@ describe('Unit | coreHandlers | handleClick', () => {
timeout: 3_000,
scrollTimeout: 200,
ignoreSelector: '',
multiClickTimeout: 1_000,
},
mockAddBreadcrumbEvent,
);
Expand Down

0 comments on commit 3f3300e

Please sign in to comment.