From db8421cd09186523228353074f18abdeaf68b9c4 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 15 Jun 2023 15:48:01 +0100 Subject: [PATCH 1/8] test(remix): Update integration tests to Remix 1.17.0 (#8304) Updates the version of Remix used in integration tests to the latest 1.17.0, which is needed to test `handleError` and `defer` features. --- packages/remix/test/integration/package.json | 10 +++++----- .../test/integration/test/client/root-loader.test.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/remix/test/integration/package.json b/packages/remix/test/integration/package.json index 3899f1acddad..821a4242a04d 100644 --- a/packages/remix/test/integration/package.json +++ b/packages/remix/test/integration/package.json @@ -7,16 +7,16 @@ "start": "remix-serve build" }, "dependencies": { - "@remix-run/express": "1.9.0", - "@remix-run/node": "1.9.0", - "@remix-run/react": "1.9.0", - "@remix-run/serve": "1.9.0", + "@remix-run/express": "1.17.0", + "@remix-run/node": "1.17.0", + "@remix-run/react": "1.17.0", + "@remix-run/serve": "1.17.0", "@sentry/remix": "file:../..", "react": "^17.0.2", "react-dom": "^17.0.2" }, "devDependencies": { - "@remix-run/dev": "1.9.0", + "@remix-run/dev": "1.17.0", "@types/react": "^17.0.47", "@types/react-dom": "^17.0.17", "nock": "^13.1.0", diff --git a/packages/remix/test/integration/test/client/root-loader.test.ts b/packages/remix/test/integration/test/client/root-loader.test.ts index 774cbaa3e4c0..b23ef5639ff7 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -1,7 +1,7 @@ import { test, expect, Page } from '@playwright/test'; async function getRouteData(page: Page): Promise { - return page.evaluate('window.__remixContext.routeData').catch(err => { + return page.evaluate('window.__remixContext.state.loaderData').catch(err => { console.warn(err); return {}; From ea45d203d8b86a3b41c06d259ebf2720b6f5d8d6 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 15 Jun 2023 21:44:42 +0100 Subject: [PATCH 2/8] fix(remix): Extract deferred responses correctly in root loaders. (#8305) Co-authored-by: Abhijeet Prasad --- packages/remix/src/utils/instrumentServer.ts | 9 +++++- packages/remix/src/utils/types.ts | 9 ++++++ packages/remix/src/utils/vendor/response.ts | 17 ++++++++++- packages/remix/test/integration/app/root.tsx | 4 ++- .../routes/loader-defer-response/index.tsx | 20 +++++++++++++ .../test/client/root-loader.test.ts | 16 +++++++++++ .../integration/test/server/loader.test.ts | 28 +++++++++++++++++++ 7 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 packages/remix/test/integration/app/routes/loader-defer-response/index.tsx diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 7fbff4bb6bd8..ef5449067df9 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -27,7 +27,7 @@ import type { ServerRoute, ServerRouteManifest, } from './types'; -import { extractData, getRequestMatch, isResponse, json, matchServerRoutes } from './vendor/response'; +import { extractData, getRequestMatch, isDeferredData, isResponse, json, matchServerRoutes } from './vendor/response'; import { normalizeRemixRequest } from './web-fetch'; // Flag to track if the core request handler is instrumented. @@ -229,6 +229,13 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { const res = await origLoader.call(this, args); const traceAndBaggage = getTraceAndBaggage(); + if (isDeferredData(res)) { + return { + ...res.data, + ...traceAndBaggage, + }; + } + // Note: `redirect` and `catch` responses do not have bodies to extract if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) { const data = await extractData(res); diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/types.ts index 642f6eef76cb..74dcf10215cc 100644 --- a/packages/remix/src/utils/types.ts +++ b/packages/remix/src/utils/types.ts @@ -62,6 +62,15 @@ export interface RouteData { [routeId: string]: AppData; } +export type DeferredData = { + data: Record; + init?: ResponseInit; + deferredKeys: string[]; + subscribe(fn: (aborted: boolean, settledKey?: string) => void): () => boolean; + cancel(): void; + resolveData(signal: AbortSignal): Promise; +}; + export interface MetaFunction { (args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor; } diff --git a/packages/remix/src/utils/vendor/response.ts b/packages/remix/src/utils/vendor/response.ts index ab5e02425c8e..ae85fff74734 100644 --- a/packages/remix/src/utils/vendor/response.ts +++ b/packages/remix/src/utils/vendor/response.ts @@ -6,7 +6,7 @@ // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. -import type { ReactRouterDomPkg, RouteMatch, ServerRoute } from '../types'; +import type { DeferredData, ReactRouterDomPkg, RouteMatch, ServerRoute } from '../types'; /** * Based on Remix Implementation @@ -124,3 +124,18 @@ export function getRequestMatch(url: URL, matches: RouteMatch[]): R return match; } + +/** + * https://github.com/remix-run/remix/blob/3e589152bc717d04e2054c31bea5a1056080d4b9/packages/remix-server-runtime/responses.ts#L75-L85 + */ +export function isDeferredData(value: any): value is DeferredData { + const deferred: DeferredData = value; + return ( + deferred && + typeof deferred === 'object' && + typeof deferred.data === 'object' && + typeof deferred.subscribe === 'function' && + typeof deferred.cancel === 'function' && + typeof deferred.resolveData === 'function' + ); +} diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app/root.tsx index e7ec56171904..1e716237343c 100644 --- a/packages/remix/test/integration/app/root.tsx +++ b/packages/remix/test/integration/app/root.tsx @@ -1,4 +1,4 @@ -import { MetaFunction, LoaderFunction, json, redirect } from '@remix-run/node'; +import { MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; import { withSentry } from '@sentry/remix'; @@ -24,6 +24,8 @@ export const loader: LoaderFunction = async ({ request }) => { }; case 'json': return json({ data_one: [], data_two: 'a string' }, { headers: { 'Cache-Control': 'max-age=300' } }); + case 'defer': + return defer({ data_one: [], data_two: 'a string' }); case 'null': return null; case 'undefined': diff --git a/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx b/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx new file mode 100644 index 000000000000..b55d8dfede47 --- /dev/null +++ b/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx @@ -0,0 +1,20 @@ +import { defer, LoaderFunction } from '@remix-run/node'; +import { useLoaderData } from '@remix-run/react'; + +type LoaderData = { id: string }; + +export const loader: LoaderFunction = async ({ params: { id } }) => { + return defer({ + id, + }); +}; + +export default function LoaderJSONResponse() { + const data = useLoaderData(); + + return ( +
+

{data && data.id ? data.id : 'Not Found'}

+
+ ); +} diff --git a/packages/remix/test/integration/test/client/root-loader.test.ts b/packages/remix/test/integration/test/client/root-loader.test.ts index b23ef5639ff7..4bb5b41a82ce 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -72,6 +72,22 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a `J }); }); +test('should inject `sentry-trace` and `baggage` into root loader returning a deferred response', async ({ page }) => { + await page.goto('/?type=defer'); + + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); + + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, + }); +}); + test('should inject `sentry-trace` and `baggage` into root loader returning `null`.', async ({ page }) => { await page.goto('/?type=null'); diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 907875949f30..2545f63d6e92 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -187,4 +187,32 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }); }); + + it('correctly instruments a deferred loader', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/loader-defer-response`; + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); + const transaction = envelope[2]; + + assertSentryTransaction(transaction, { + transaction: 'root', + transaction_info: { + source: 'route', + }, + spans: [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response/index', + op: 'function.remix.loader', + }, + { + description: 'root', + op: 'function.remix.document_request', + }, + ], + }); + }); }); From dba9a3d5b7406dca4199efbe28f408f818d054e3 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 16 Jun 2023 08:48:05 +0200 Subject: [PATCH 3/8] feat(replay): Rework slow click & multi click detection (#8322) This PR reworks the slow click detection to accommodate rage click detection as well. This required substantial changes, as we need to keep track of stuff much more. Now, we keep a list of clicks that come in in a new class. We register a single set of listeners (mutation observer, click listener, scroll listener), and then try to route things to the correct clicks as much as possible. Any clicks within 1 second count as "multi click", so are not considered for slow clicks at all, but counted on the first click. After a second, a click (even on the same element) will be treated as a "new" click. ref https://github.com/getsentry/sentry-javascript/issues/8300 --- .../replay/slowClick/clickTargets/test.ts | 2 + .../replay/slowClick/multiClick/test.ts | 60 ++ .../suites/replay/slowClick/mutation/test.ts | 118 +++- .../suites/replay/slowClick/scroll/test.ts | 2 + .../suites/replay/slowClick/template.html | 4 + .../suites/replay/slowClick/timeout/test.ts | 4 + packages/replay/src/constants.ts | 2 + .../replay/src/coreHandlers/handleClick.ts | 306 ++++++++++ packages/replay/src/coreHandlers/handleDom.ts | 61 +- .../src/coreHandlers/handleSlowClick.ts | 145 ----- .../replay/src/coreHandlers/util/domUtils.ts | 38 ++ packages/replay/src/replay.ts | 31 + packages/replay/src/types/replay.ts | 9 + packages/replay/src/types/replayFrame.ts | 19 +- .../unit/coreHandlers/handleClick.test.ts | 542 ++++++++++++++++++ .../test/unit/coreHandlers/handleDom.test.ts | 2 +- .../unit/coreHandlers/handleSlowClick.test.ts | 34 -- 17 files changed, 1142 insertions(+), 237 deletions(-) create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts create mode 100644 packages/replay/src/coreHandlers/handleClick.ts delete mode 100644 packages/replay/src/coreHandlers/handleSlowClick.ts create mode 100644 packages/replay/src/coreHandlers/util/domUtils.ts create mode 100644 packages/replay/test/unit/coreHandlers/handleClick.test.ts delete mode 100644 packages/replay/test/unit/coreHandlers/handleSlowClick.test.ts diff --git a/packages/browser-integration-tests/suites/replay/slowClick/clickTargets/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/clickTargets/test.ts index dfa1581d4704..59bfb2ea26e8 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/clickTargets/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/clickTargets/test.ts @@ -65,8 +65,10 @@ import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } expect(slowClickBreadcrumbs).toEqual([ { category: 'ui.slowClickDetected', + type: 'default', data: { endReason: 'timeout', + clickCount: 1, node: { attributes: expect.objectContaining({ id, diff --git a/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts new file mode 100644 index 000000000000..2d84eeb29ac3 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/multiClick/test.ts @@ -0,0 +1,60 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; + +sentryTest('captures multi click when not detecting slow click', 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; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.multiClick'); + }); + + await page.click('#mutationButtonImmediately', { clickCount: 4 }); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + + const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick'); + + expect(slowClickBreadcrumbs).toEqual([ + { + category: 'ui.multiClick', + type: 'default', + data: { + clickCount: 4, + 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/suites/replay/slowClick/mutation/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts index 8a169a982aa8..deb394ebac2d 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts @@ -29,8 +29,6 @@ sentryTest('mutation after threshold results in slow click', async ({ getLocalTe return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); }); - // Trigger this twice, sometimes this was flaky otherwise... - await page.click('#mutationButton'); await page.click('#mutationButton'); const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); @@ -40,8 +38,71 @@ sentryTest('mutation after threshold results in slow click', async ({ getLocalTe expect(slowClickBreadcrumbs).toEqual([ { category: 'ui.slowClickDetected', + type: 'default', + data: { + endReason: 'mutation', + clickCount: 1, + node: { + attributes: { + id: 'mutationButton', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '******* ********', + }, + nodeId: expect.any(Number), + timeAfterClickMs: expect.any(Number), + url: 'http://sentry-test.io/index.html', + }, + message: 'body > button#mutationButton', + timestamp: expect.any(Number), + }, + ]); + + expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeGreaterThan(3000); + expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3100); +}); + +sentryTest('multiple clicks are counted', 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; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + }); + + void page.click('#mutationButton', { clickCount: 4 }); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + + const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + const multiClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick'); + + expect(slowClickBreadcrumbs).toEqual([ + { + category: 'ui.slowClickDetected', + type: 'default', data: { endReason: 'mutation', + clickCount: 4, node: { attributes: { id: 'mutationButton', @@ -58,6 +119,7 @@ sentryTest('mutation after threshold results in slow click', async ({ getLocalTe timestamp: expect.any(Number), }, ]); + expect(multiClickBreadcrumbs.length).toEqual(0); expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeGreaterThan(3000); expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3100); @@ -165,3 +227,55 @@ sentryTest('inline click handler does not trigger slow click', async ({ getLocal }, ]); }); + +sentryTest('mouseDown events are considered', async ({ browserName, getLocalTestUrl, page }) => { + // This test seems to only be flakey on firefox + if (shouldSkipReplayTest() || ['firefox'].includes(browserName)) { + 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; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + await page.click('#mouseDownButton'); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + + expect(breadcrumbs).toEqual([ + { + category: 'ui.click', + data: { + node: { + attributes: { + id: 'mouseDownButton', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '******* ******** ** ***** ****', + }, + nodeId: expect.any(Number), + }, + message: 'body > button#mouseDownButton', + timestamp: expect.any(Number), + type: 'default', + }, + ]); +}); diff --git a/packages/browser-integration-tests/suites/replay/slowClick/scroll/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/scroll/test.ts index f7f705ce5670..a8e59752fc4a 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/scroll/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/scroll/test.ts @@ -89,8 +89,10 @@ sentryTest('late scroll triggers slow click', async ({ getLocalTestUrl, page }) expect(slowClickBreadcrumbs).toEqual([ { category: 'ui.slowClickDetected', + type: 'default', data: { endReason: 'timeout', + clickCount: 1, node: { attributes: { id: 'scrollLateButton', diff --git a/packages/browser-integration-tests/suites/replay/slowClick/template.html b/packages/browser-integration-tests/suites/replay/slowClick/template.html index 1cf757f7b974..f49c8b1d410d 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/template.html +++ b/packages/browser-integration-tests/suites/replay/slowClick/template.html @@ -18,6 +18,7 @@ + Link Link external @@ -69,6 +70,9 @@

Bottom

console.log('DONE'); }, 3001); }); + document.getElementById('mouseDownButton').addEventListener('mousedown', () => { + document.getElementById('out').innerHTML += 'mutationButton clicked
'; + }); // Do nothing on these elements document diff --git a/packages/browser-integration-tests/suites/replay/slowClick/timeout/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/timeout/test.ts index fef742681614..7e94e0b68f15 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/timeout/test.ts +++ b/packages/browser-integration-tests/suites/replay/slowClick/timeout/test.ts @@ -38,8 +38,10 @@ sentryTest('mutation after timeout results in slow click', async ({ getLocalTest expect(slowClickBreadcrumbs).toEqual([ { category: 'ui.slowClickDetected', + type: 'default', data: { endReason: 'timeout', + clickCount: 1, node: { attributes: { id: 'mutationButtonLate', @@ -93,8 +95,10 @@ sentryTest('console.log results in slow click', async ({ getLocalTestUrl, page } expect(slowClickBreadcrumbs).toEqual([ { category: 'ui.slowClickDetected', + type: 'default', data: { endReason: 'timeout', + clickCount: 1, node: { attributes: { id: 'consoleLogButton', diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index 1801c34a4e8e..120079ebb857 100644 --- a/packages/replay/src/constants.ts +++ b/packages/replay/src/constants.ts @@ -42,3 +42,5 @@ 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; diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts new file mode 100644 index 000000000000..5a75413b00d1 --- /dev/null +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -0,0 +1,306 @@ +import type { Breadcrumb } from '@sentry/types'; + +import { WINDOW } from '../constants'; +import type { MultiClickFrame, ReplayClickDetector, ReplayContainer, SlowClickConfig, SlowClickFrame } from '../types'; +import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; +import { getClickTargetNode } from './util/domUtils'; + +type ClickBreadcrumb = Breadcrumb & { + timestamp: number; +}; + +interface Click { + timestamp: number; + mutationAfter?: number; + scrollAfter?: number; + clickBreadcrumb: ClickBreadcrumb; + clickCount: number; + node: HTMLElement; +} + +/** Handle a click. */ +export function handleClick(clickDetector: ReplayClickDetector, clickBreadcrumb: Breadcrumb, node: HTMLElement): void { + clickDetector.handleClick(clickBreadcrumb, node); +} + +/** A click detector class that can be used to detect slow or rage clicks on elements. */ +export class ClickDetector implements ReplayClickDetector { + // protected for testing + protected _lastMutation = 0; + protected _lastScroll = 0; + + private _clicks: Click[] = []; + private _teardown: undefined | (() => void); + + private _multiClickTimeout: number; + private _threshold: number; + private _scollTimeout: number; + private _timeout: number; + private _ignoreSelector: string; + + private _replay: ReplayContainer; + private _checkClickTimeout?: ReturnType; + private _addBreadcrumbEvent: typeof addBreadcrumbEvent; + + public constructor( + replay: ReplayContainer, + slowClickConfig: SlowClickConfig, + // Just for easier testing + _addBreadcrumbEvent = addBreadcrumbEvent, + ) { + // 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; + this._ignoreSelector = slowClickConfig.ignoreSelector; + this._addBreadcrumbEvent = _addBreadcrumbEvent; + } + + /** Register click detection handlers on mutation or scroll. */ + public addListeners(): void { + const mutationHandler = (): void => { + this._lastMutation = nowInSeconds(); + }; + + const scrollHandler = (): void => { + this._lastScroll = nowInSeconds(); + }; + + const clickHandler = (event: MouseEvent): void => { + if (!event.target) { + return; + } + + const node = getClickTargetNode(event); + if (node) { + this._handleMultiClick(node as HTMLElement); + } + }; + + const obs = new MutationObserver(mutationHandler); + + obs.observe(WINDOW.document.documentElement, { + attributes: true, + characterData: true, + childList: true, + subtree: true, + }); + + WINDOW.addEventListener('scroll', scrollHandler, { passive: true }); + WINDOW.addEventListener('click', clickHandler, { passive: true }); + + this._teardown = () => { + WINDOW.removeEventListener('scroll', scrollHandler); + WINDOW.removeEventListener('click', clickHandler); + + obs.disconnect(); + this._clicks = []; + this._lastMutation = 0; + this._lastScroll = 0; + }; + } + + /** Clean up listeners. */ + public removeListeners(): void { + if (this._teardown) { + this._teardown(); + } + + if (this._checkClickTimeout) { + clearTimeout(this._checkClickTimeout); + } + } + + /** Handle a click */ + public handleClick(breadcrumb: Breadcrumb, node: HTMLElement): void { + if (ignoreElement(node, this._ignoreSelector) || !isClickBreadcrumb(breadcrumb)) { + 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, + // Set this to 0 so we know it originates from the click breadcrumb + clickCount: 0, + node, + }; + this._clicks.push(newClick); + + // If this is the first new click, set a timeout to check for multi clicks + if (this._clicks.length === 1) { + this._scheduleCheckClicks(); + } + } + + /** Count multiple clicks on elements. */ + private _handleMultiClick(node: HTMLElement): void { + const click = this._getClick(node); + + if (!click) { + return; + } + + 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); + } + + /** Check the clicks that happened. */ + private _checkClicks(): void { + const timedOutClicks: Click[] = []; + + const now = nowInSeconds(); + + this._clicks.forEach(click => { + if (!click.mutationAfter && this._lastMutation) { + click.mutationAfter = click.timestamp <= this._lastMutation ? this._lastMutation - click.timestamp : undefined; + } + if (!click.scrollAfter && this._lastScroll) { + 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); + } + }); + + // Remove "old" clicks + for (const click of timedOutClicks) { + this._generateBreadcrumbs(click); + + const pos = this._clicks.indexOf(click); + if (pos !== -1) { + this._clicks.splice(pos, 1); + } + } + + // Trigger new check, unless no clicks left + if (this._clicks.length) { + this._scheduleCheckClicks(); + } + } + + /** Generate matching breadcrumb(s) for the click. */ + private _generateBreadcrumbs(click: Click): void { + const replay = this._replay; + const hadScroll = click.scrollAfter && click.scrollAfter <= this._scollTimeout; + const hadMutation = click.mutationAfter && click.mutationAfter <= this._threshold; + + const isSlowClick = !hadScroll && !hadMutation; + const { clickCount, clickBreadcrumb } = click; + + // Slow click + if (isSlowClick) { + // If `mutationAfter` is set, it means a mutation happened after the threshold, but before the timeout + // If not, it means we just timed out without scroll & mutation + const timeAfterClickMs = Math.min(click.mutationAfter || this._timeout, this._timeout) * 1000; + const endReason = timeAfterClickMs < this._timeout * 1000 ? 'mutation' : 'timeout'; + + const breadcrumb: SlowClickFrame = { + type: 'default', + message: clickBreadcrumb.message, + timestamp: clickBreadcrumb.timestamp, + category: 'ui.slowClickDetected', + data: { + ...clickBreadcrumb.data, + url: WINDOW.location.href, + route: replay.getCurrentRoute(), + timeAfterClickMs, + endReason, + // If clickCount === 0, it means multiClick was not correctly captured here + // - we still want to send 1 in this case + clickCount: clickCount || 1, + }, + }; + + this._addBreadcrumbEvent(replay, breadcrumb); + return; + } + + // Multi click + if (clickCount > 1) { + const breadcrumb: MultiClickFrame = { + type: 'default', + message: clickBreadcrumb.message, + timestamp: clickBreadcrumb.timestamp, + category: 'ui.multiClick', + data: { + ...clickBreadcrumb.data, + url: WINDOW.location.href, + route: replay.getCurrentRoute(), + clickCount, + metric: true, + }, + }; + + this._addBreadcrumbEvent(replay, breadcrumb); + } + } + + /** Schedule to check current clicks. */ + private _scheduleCheckClicks(): void { + this._checkClickTimeout = setTimeout(() => this._checkClicks(), 1000); + } +} + +const SLOW_CLICK_TAGS = ['A', 'BUTTON', 'INPUT']; + +/** exported for tests only */ +export function ignoreElement(node: HTMLElement, ignoreSelector: string): boolean { + if (!SLOW_CLICK_TAGS.includes(node.tagName)) { + return true; + } + + // If tag, we only want to consider input[type='submit'] & input[type='button'] + if (node.tagName === 'INPUT' && !['submit', 'button'].includes(node.getAttribute('type') || '')) { + return true; + } + + // If tag, detect special variants that may not lead to an action + // If target !== _self, we may open the link somewhere else, which would lead to no action + // Also, when downloading a file, we may not leave the page, but still not trigger an action + if ( + node.tagName === 'A' && + (node.hasAttribute('download') || (node.hasAttribute('target') && node.getAttribute('target') !== '_self')) + ) { + return true; + } + + if (ignoreSelector && node.matches(ignoreSelector)) { + return true; + } + + return false; +} + +function isClickBreadcrumb(breadcrumb: Breadcrumb): breadcrumb is ClickBreadcrumb { + return !!(breadcrumb.data && typeof breadcrumb.data.nodeId === 'number' && breadcrumb.timestamp); +} + +// This is good enough for us, and is easier to test/mock than `timestampInSeconds` +function nowInSeconds(): number { + return Date.now() / 1000; +} diff --git a/packages/replay/src/coreHandlers/handleDom.ts b/packages/replay/src/coreHandlers/handleDom.ts index 54ab7ec8bb09..71fb211ee3fe 100644 --- a/packages/replay/src/coreHandlers/handleDom.ts +++ b/packages/replay/src/coreHandlers/handleDom.ts @@ -3,32 +3,17 @@ import { NodeType } from '@sentry-internal/rrweb-snapshot'; import type { Breadcrumb } from '@sentry/types'; import { htmlTreeAsString } from '@sentry/utils'; -import { SLOW_CLICK_SCROLL_TIMEOUT, SLOW_CLICK_THRESHOLD } from '../constants'; -import type { ReplayContainer, SlowClickConfig } from '../types'; +import type { ReplayContainer } from '../types'; import { createBreadcrumb } from '../util/createBreadcrumb'; -import { detectSlowClick } from './handleSlowClick'; +import { handleClick } from './handleClick'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; +import type { DomHandlerData } from './util/domUtils'; +import { getClickTargetNode, getTargetNode } from './util/domUtils'; import { getAttributesToRecord } from './util/getAttributesToRecord'; -export interface DomHandlerData { - name: string; - event: Node | { target: EventTarget }; -} - export const handleDomListener: (replay: ReplayContainer) => (handlerData: DomHandlerData) => void = ( replay: ReplayContainer, ) => { - const { slowClickTimeout, slowClickIgnoreSelectors } = replay.getOptions(); - - const slowClickConfig: SlowClickConfig | undefined = slowClickTimeout - ? { - threshold: Math.min(SLOW_CLICK_THRESHOLD, slowClickTimeout), - timeout: slowClickTimeout, - scrollTimeout: SLOW_CLICK_SCROLL_TIMEOUT, - ignoreSelector: slowClickIgnoreSelectors ? slowClickIgnoreSelectors.join(',') : '', - } - : undefined; - return (handlerData: DomHandlerData): void => { if (!replay.isEnabled()) { return; @@ -43,11 +28,10 @@ export const handleDomListener: (replay: ReplayContainer) => (handlerData: DomHa const isClick = handlerData.name === 'click'; const event = isClick && (handlerData.event as PointerEvent); // Ignore clicks if ctrl/alt/meta keys are held down as they alter behavior of clicks (e.g. open in new tab) - if (isClick && slowClickConfig && event && !event.altKey && !event.metaKey && !event.ctrlKey) { - detectSlowClick( - replay, - slowClickConfig, - result as Breadcrumb & { timestamp: number }, + if (isClick && replay.clickDetector && event && !event.altKey && !event.metaKey && !event.ctrlKey) { + handleClick( + replay.clickDetector, + result as Breadcrumb & { timestamp: number; data: { nodeId: number } }, getClickTargetNode(handlerData.event) as HTMLElement, ); } @@ -118,32 +102,3 @@ function getDomTarget(handlerData: DomHandlerData): { target: Node | INode | nul function isRrwebNode(node: EventTarget): node is INode { return '__sn' in node; } - -function getTargetNode(event: Node | { target: EventTarget | null }): Node | INode | null { - if (isEventWithTarget(event)) { - return event.target as Node | null; - } - - return event; -} - -const INTERACTIVE_SELECTOR = 'button,a'; - -// For clicks, we check if the target is inside of a button or link -// If so, we use this as the target instead -// This is useful because if you click on the image in , -// The target will be the image, not the button, which we don't want here -function getClickTargetNode(event: DomHandlerData['event']): Node | INode | null { - const target = getTargetNode(event); - - if (!target || !(target instanceof Element)) { - return target; - } - - const closestInteractive = target.closest(INTERACTIVE_SELECTOR); - return closestInteractive || target; -} - -function isEventWithTarget(event: unknown): event is { target: EventTarget | null } { - return typeof event === 'object' && !!event && 'target' in event; -} diff --git a/packages/replay/src/coreHandlers/handleSlowClick.ts b/packages/replay/src/coreHandlers/handleSlowClick.ts deleted file mode 100644 index c939a990f87a..000000000000 --- a/packages/replay/src/coreHandlers/handleSlowClick.ts +++ /dev/null @@ -1,145 +0,0 @@ -import type { Breadcrumb } from '@sentry/types'; - -import { WINDOW } from '../constants'; -import type { ReplayContainer, SlowClickConfig } from '../types'; -import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; - -type ClickBreadcrumb = Breadcrumb & { - timestamp: number; -}; - -/** - * Detect a slow click on a button/a tag, - * and potentially create a corresponding breadcrumb. - */ -export function detectSlowClick( - replay: ReplayContainer, - config: SlowClickConfig, - clickBreadcrumb: ClickBreadcrumb, - node: HTMLElement, -): void { - if (ignoreElement(node, config)) { - return; - } - - /* - We consider a slow click a click on a button/a, which does not trigger one of: - - DOM mutation - - Scroll (within 100ms) - Within the given threshold time. - After time timeout time, we stop listening and mark it as a slow click anyhow. - */ - - let cleanup: () => void = () => { - // replaced further down - }; - - // After timeout time, def. consider this a slow click, and stop watching for mutations - const timeout = setTimeout(() => { - handleSlowClick(replay, clickBreadcrumb, config.timeout, 'timeout'); - cleanup(); - }, config.timeout); - - const mutationHandler = (): void => { - maybeHandleSlowClick(replay, clickBreadcrumb, config.threshold, config.timeout, 'mutation'); - cleanup(); - }; - - const scrollHandler = (): void => { - maybeHandleSlowClick(replay, clickBreadcrumb, config.scrollTimeout, config.timeout, 'scroll'); - cleanup(); - }; - - const obs = new MutationObserver(mutationHandler); - - obs.observe(WINDOW.document.documentElement, { - attributes: true, - characterData: true, - childList: true, - subtree: true, - }); - - WINDOW.addEventListener('scroll', scrollHandler); - - // Stop listening to scroll timeouts early - const scrollTimeout = setTimeout(() => { - WINDOW.removeEventListener('scroll', scrollHandler); - }, config.scrollTimeout); - - cleanup = (): void => { - clearTimeout(timeout); - clearTimeout(scrollTimeout); - obs.disconnect(); - WINDOW.removeEventListener('scroll', scrollHandler); - }; -} - -function maybeHandleSlowClick( - replay: ReplayContainer, - clickBreadcrumb: ClickBreadcrumb, - threshold: number, - timeout: number, - endReason: string, -): boolean { - const now = Date.now(); - const timeAfterClickMs = now - clickBreadcrumb.timestamp * 1000; - - if (timeAfterClickMs > threshold) { - handleSlowClick(replay, clickBreadcrumb, Math.min(timeAfterClickMs, timeout), endReason); - return true; - } - - return false; -} - -function handleSlowClick( - replay: ReplayContainer, - clickBreadcrumb: ClickBreadcrumb, - timeAfterClickMs: number, - endReason: string, -): void { - const breadcrumb = { - message: clickBreadcrumb.message, - timestamp: clickBreadcrumb.timestamp, - category: 'ui.slowClickDetected', - data: { - ...clickBreadcrumb.data, - url: WINDOW.location.href, - // TODO FN: add parametrized route, when possible - timeAfterClickMs, - endReason, - }, - }; - - addBreadcrumbEvent(replay, breadcrumb); -} - -const SLOW_CLICK_TAGS = ['A', 'BUTTON', 'INPUT']; - -/** exported for tests only */ -export function ignoreElement(node: HTMLElement, config: SlowClickConfig): boolean { - if (!SLOW_CLICK_TAGS.includes(node.tagName)) { - return true; - } - - // If tag, we only want to consider input[type='submit'] & input[type='button'] - if (node.tagName === 'INPUT' && !['submit', 'button'].includes(node.getAttribute('type') || '')) { - return true; - } - - // If tag, detect special variants that may not lead to an action - // If target !== _self, we may open the link somewhere else, which would lead to no action - // Also, when downloading a file, we may not leave the page, but still not trigger an action - if ( - node.tagName === 'A' && - (node.hasAttribute('download') || (node.hasAttribute('target') && node.getAttribute('target') !== '_self')) - ) { - return true; - } - - if (config.ignoreSelector && node.matches(config.ignoreSelector)) { - return true; - } - - return false; -} diff --git a/packages/replay/src/coreHandlers/util/domUtils.ts b/packages/replay/src/coreHandlers/util/domUtils.ts new file mode 100644 index 000000000000..6091dc7fe837 --- /dev/null +++ b/packages/replay/src/coreHandlers/util/domUtils.ts @@ -0,0 +1,38 @@ +import type { INode } from '@sentry-internal/rrweb-snapshot'; + +export interface DomHandlerData { + name: string; + event: Node | { target: EventTarget }; +} + +const INTERACTIVE_SELECTOR = 'button,a'; + +/** + * For clicks, we check if the target is inside of a button or link + * If so, we use this as the target instead + * This is useful because if you click on the image in , + * The target will be the image, not the button, which we don't want here + */ +export function getClickTargetNode(event: DomHandlerData['event'] | MouseEvent): Node | INode | null { + const target = getTargetNode(event); + + if (!target || !(target instanceof Element)) { + return target; + } + + const closestInteractive = target.closest(INTERACTIVE_SELECTOR); + return closestInteractive || target; +} + +/** Get the event target node. */ +export function getTargetNode(event: Node | { target: EventTarget | null }): Node | INode | null { + if (isEventWithTarget(event)) { + return event.target as Node | null; + } + + return event; +} + +function isEventWithTarget(event: unknown): event is { target: EventTarget | null } { + return typeof event === 'object' && !!event && 'target' in event; +} diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index a00b96f23c19..acb2980e608c 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -7,10 +7,14 @@ 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, + SLOW_CLICK_THRESHOLD, WINDOW, } from './constants'; +import { ClickDetector } from './coreHandlers/handleClick'; import { handleKeyboardEvent } from './coreHandlers/handleKeyboardEvent'; import { setupPerformanceObserver } from './coreHandlers/performanceObserver'; import { createEventBuffer } from './eventBuffer'; @@ -31,6 +35,7 @@ import type { ReplayPluginOptions, SendBufferedReplayOptions, Session, + SlowClickConfig, Timeouts, } from './types'; import { addEvent } from './util/addEvent'; @@ -60,6 +65,8 @@ export class ReplayContainer implements ReplayContainerInterface { public session: Session | undefined; + public clickDetector: ClickDetector | undefined; + /** * Recording can happen in one of three modes: * - session: Record the whole session, sending it continuously @@ -159,6 +166,22 @@ export class ReplayContainer implements ReplayContainerInterface { // ... per 5s 5, ); + + const { slowClickTimeout, slowClickIgnoreSelectors } = this.getOptions(); + + const slowClickConfig: SlowClickConfig | undefined = slowClickTimeout + ? { + threshold: Math.min(SLOW_CLICK_THRESHOLD, slowClickTimeout), + timeout: slowClickTimeout, + scrollTimeout: SLOW_CLICK_SCROLL_TIMEOUT, + ignoreSelector: slowClickIgnoreSelectors ? slowClickIgnoreSelectors.join(',') : '', + multiClickTimeout: MULTI_CLICK_TIMEOUT, + } + : undefined; + + if (slowClickConfig) { + this.clickDetector = new ClickDetector(this, slowClickConfig); + } } /** Get the event context. */ @@ -737,6 +760,10 @@ export class ReplayContainer implements ReplayContainerInterface { WINDOW.addEventListener('focus', this._handleWindowFocus); WINDOW.addEventListener('keydown', this._handleKeyboardEvent); + if (this.clickDetector) { + this.clickDetector.addListeners(); + } + // There is no way to remove these listeners, so ensure they are only added once if (!this._hasInitializedCoreListeners) { addGlobalListeners(this); @@ -766,6 +793,10 @@ export class ReplayContainer implements ReplayContainerInterface { WINDOW.removeEventListener('focus', this._handleWindowFocus); WINDOW.removeEventListener('keydown', this._handleKeyboardEvent); + if (this.clickDetector) { + this.clickDetector.removeListeners(); + } + if (this._performanceObserver) { this._performanceObserver.disconnect(); this._performanceObserver = null; diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index 2ec6e18346ee..f058b2c9011a 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -1,4 +1,5 @@ import type { + Breadcrumb, FetchBreadcrumbHint, HandlerDataFetch, ReplayRecordingData, @@ -407,8 +408,15 @@ export interface SendBufferedReplayOptions { continueRecording?: boolean; } +export interface ReplayClickDetector { + addListeners(): void; + removeListeners(): void; + handleClick(breadcrumb: Breadcrumb, node: HTMLElement): void; +} + export interface ReplayContainer { eventBuffer: EventBuffer | null; + clickDetector: ReplayClickDetector | undefined; performanceEvents: AllPerformanceEntry[]; session: Session | undefined; recordingMode: ReplayRecordingMode; @@ -468,4 +476,5 @@ export interface SlowClickConfig { timeout: number; scrollTimeout: number; ignoreSelector: string; + multiClickTimeout: number; } diff --git a/packages/replay/src/types/replayFrame.ts b/packages/replay/src/types/replayFrame.ts index f0107fdbf77a..379dbab91605 100644 --- a/packages/replay/src/types/replayFrame.ts +++ b/packages/replay/src/types/replayFrame.ts @@ -88,14 +88,28 @@ interface FocusFrame extends BaseBreadcrumbFrame { interface SlowClickFrameData extends ClickFrameData { url: string; - timeAfterClickFs: number; + route?: string; + timeAfterClickMs: number; endReason: string; + clickCount: number; } -interface SlowClickFrame extends BaseBreadcrumbFrame { +export interface SlowClickFrame extends BaseBreadcrumbFrame { category: 'ui.slowClickDetected'; data: SlowClickFrameData; } +interface MultiClickFrameData extends ClickFrameData { + url: string; + route?: string; + clickCount: number; + metric: true; +} + +export interface MultiClickFrame extends BaseBreadcrumbFrame { + category: 'ui.multiClick'; + data: MultiClickFrameData; +} + interface OptionFrame { blockAllMedia: boolean; errorSampleRate: number; @@ -118,6 +132,7 @@ export type BreadcrumbFrame = | BlurFrame | FocusFrame | SlowClickFrame + | MultiClickFrame | MutationFrame | BaseBreadcrumbFrame; diff --git a/packages/replay/test/unit/coreHandlers/handleClick.test.ts b/packages/replay/test/unit/coreHandlers/handleClick.test.ts new file mode 100644 index 000000000000..8b06e4683656 --- /dev/null +++ b/packages/replay/test/unit/coreHandlers/handleClick.test.ts @@ -0,0 +1,542 @@ +import type { Breadcrumb } from '@sentry/types'; + +import { BASE_TIMESTAMP } from '../..'; +import { ClickDetector, ignoreElement } from '../../../src/coreHandlers/handleClick'; +import type { ReplayContainer } from '../../../src/types'; + +jest.useFakeTimers(); + +describe('Unit | coreHandlers | handleClick', () => { + describe('ClickDetector', () => { + beforeEach(() => { + jest.setSystemTime(BASE_TIMESTAMP); + }); + + test('it captures a single click', 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 breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(1_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(1_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(1_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { + category: 'ui.slowClickDetected', + type: 'default', + data: { + 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(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', + } 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, + data: { + nodeId: 2, + }, + }; + const breadcrumb3: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 3, + }, + }; + const node1 = document.createElement('button'); + const node2 = document.createElement('button'); + const node3 = document.createElement('button'); + detector.handleClick(breadcrumb1, node1); + detector.handleClick(breadcrumb2, node2); + detector.handleClick(breadcrumb3, node3); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(3); + + jest.advanceTimersByTime(5_000); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(3); + }); + + test('it ignores clicks on ignored elements', 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, + data: { + nodeId: 2, + }, + }; + const breadcrumb3: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 3, + }, + }; + const node1 = document.createElement('div'); + const node2 = document.createElement('div'); + const node3 = document.createElement('div'); + detector.handleClick(breadcrumb1, node1); + detector.handleClick(breadcrumb2, node2); + detector.handleClick(breadcrumb3, node3); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + }); + + describe('mutations', () => { + let detector: ClickDetector; + let mockAddBreadcrumbEvent = jest.fn(); + + const replay = { + getCurrentRoute: () => 'test-route', + } as ReplayContainer; + + beforeEach(() => { + jest.setSystemTime(BASE_TIMESTAMP); + + mockAddBreadcrumbEvent = jest.fn(); + + detector = new ClickDetector( + replay, + { + threshold: 1_000, + timeout: 3_000, + scrollTimeout: 200, + ignoreSelector: '', + multiClickTimeout: 1_000, + }, + mockAddBreadcrumbEvent, + ); + }); + + test('it does not consider clicks with mutation before threshold as slow click', async () => { + const breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(500); + + // Pretend a mutation happend + detector['_lastMutation'] = BASE_TIMESTAMP / 1000 + 0.5; + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + }); + + test('it considers clicks with mutation after threshold as slow click', async () => { + const breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(1_000); + + // Pretend a mutation happend + detector['_lastMutation'] = BASE_TIMESTAMP / 1000 + 2; + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { + category: 'ui.slowClickDetected', + type: 'default', + data: { + clickCount: 1, + endReason: 'mutation', + nodeId: 1, + route: 'test-route', + timeAfterClickMs: 2000, + url: 'http://localhost/', + }, + message: undefined, + timestamp: expect.any(Number), + }); + + jest.advanceTimersByTime(5_000); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); + }); + + test('it caps timeout', async () => { + const breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(1_000); + + // Pretend a mutation happend + detector['_lastMutation'] = BASE_TIMESTAMP / 1000 + 5; + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(5_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { + category: 'ui.slowClickDetected', + type: 'default', + data: { + 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(1); + }); + }); + + describe('scroll', () => { + let detector: ClickDetector; + let mockAddBreadcrumbEvent = jest.fn(); + + const replay = { + getCurrentRoute: () => 'test-route', + } as ReplayContainer; + + beforeEach(() => { + jest.setSystemTime(BASE_TIMESTAMP); + + mockAddBreadcrumbEvent = jest.fn(); + + detector = new ClickDetector( + replay, + { + threshold: 1_000, + timeout: 3_000, + scrollTimeout: 200, + ignoreSelector: '', + multiClickTimeout: 1_000, + }, + mockAddBreadcrumbEvent, + ); + }); + + test('it does not consider clicks with scroll before threshold as slow click', async () => { + const breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(100); + + // Pretend a mutation happend + detector['_lastScroll'] = BASE_TIMESTAMP / 1000 + 0.15; + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + }); + + test('it considers clicks with scroll after threshold as slow click', async () => { + const breadcrumb: Breadcrumb = { + timestamp: BASE_TIMESTAMP / 1000, + data: { + nodeId: 1, + }, + }; + const node = document.createElement('button'); + detector.handleClick(breadcrumb, node); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(300); + + // Pretend a mutation happend + detector['_lastScroll'] = BASE_TIMESTAMP / 1000 + 0.3; + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(0); + + jest.advanceTimersByTime(3_000); + + expect(mockAddBreadcrumbEvent).toHaveBeenCalledTimes(1); + expect(mockAddBreadcrumbEvent).toHaveBeenCalledWith(replay, { + category: 'ui.slowClickDetected', + type: 'default', + data: { + 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(1); + }); + }); + }); + + describe('ignoreElement', () => { + it.each([ + ['div', {}, true], + ['button', {}, false], + ['a', {}, false], + ['input', {}, true], + ['input', { type: 'text' }, true], + ['input', { type: 'button' }, false], + ['input', { type: 'submit' }, false], + ['a', { target: '_self' }, false], + ['a', { target: '_blank' }, true], + ['a', { download: '' }, true], + ['a', { href: 'xx' }, false], + ])('it works with <%s> & %p', (tagName, attributes, expected) => { + const node = document.createElement(tagName); + Object.entries(attributes).forEach(([key, value]) => { + node.setAttribute(key, value); + }); + expect(ignoreElement(node, '')).toBe(expected); + }); + + test('it ignored selectors matching ignoreSelector', () => { + const button = document.createElement('button'); + const a = document.createElement('a'); + + expect(ignoreElement(button, 'button')).toBe(true); + expect(ignoreElement(a, 'button')).toBe(false); + }); + }); +}); diff --git a/packages/replay/test/unit/coreHandlers/handleDom.test.ts b/packages/replay/test/unit/coreHandlers/handleDom.test.ts index dc1fff0b5ff2..99fa5dc1e367 100644 --- a/packages/replay/test/unit/coreHandlers/handleDom.test.ts +++ b/packages/replay/test/unit/coreHandlers/handleDom.test.ts @@ -1,5 +1,5 @@ -import type { DomHandlerData } from '../../../src/coreHandlers/handleDom'; import { handleDom } from '../../../src/coreHandlers/handleDom'; +import type { DomHandlerData } from '../../../src/coreHandlers/util/domUtils'; describe('Unit | coreHandlers | handleDom', () => { test('it works with a basic click event on a div', () => { diff --git a/packages/replay/test/unit/coreHandlers/handleSlowClick.test.ts b/packages/replay/test/unit/coreHandlers/handleSlowClick.test.ts deleted file mode 100644 index 2d0922272115..000000000000 --- a/packages/replay/test/unit/coreHandlers/handleSlowClick.test.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { ignoreElement } from '../../../src/coreHandlers/handleSlowClick'; -import type { SlowClickConfig } from '../../../src/types'; - -describe('Unit | coreHandlers | handleSlowClick', () => { - describe('ignoreElement', () => { - it.each([ - ['div', {}, true], - ['button', {}, false], - ['a', {}, false], - ['input', {}, true], - ['input', { type: 'text' }, true], - ['input', { type: 'button' }, false], - ['input', { type: 'submit' }, false], - ['a', { target: '_self' }, false], - ['a', { target: '_blank' }, true], - ['a', { download: '' }, true], - ['a', { href: 'xx' }, false], - ])('it works with <%s> & %p', (tagName, attributes, expected) => { - const node = document.createElement(tagName); - Object.entries(attributes).forEach(([key, value]) => { - node.setAttribute(key, value); - }); - expect(ignoreElement(node, {} as SlowClickConfig)).toBe(expected); - }); - - test('it ignored selectors matching ignoreSelector', () => { - const button = document.createElement('button'); - const a = document.createElement('a'); - - expect(ignoreElement(button, { ignoreSelector: 'button' } as SlowClickConfig)).toBe(true); - expect(ignoreElement(a, { ignoreSelector: 'button' } as SlowClickConfig)).toBe(false); - }); - }); -}); From a4c858ff5557d8f088b30e88740a6c86377bbe93 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 16 Jun 2023 13:28:20 +0200 Subject: [PATCH 4/8] fix(core): Temporarily store debug IDs in stack frame and only put them into `debug_meta` before sending (#8347) --- packages/core/src/utils/prepareEvent.ts | 64 ++++++++++++++----- packages/core/test/lib/prepareEvent.test.ts | 71 ++++++++++++++++----- 2 files changed, 104 insertions(+), 31 deletions(-) diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index 474a95c7823b..84bfd404c56f 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -38,9 +38,9 @@ export function prepareEvent( applyClientOptions(prepared, options); applyIntegrationsMetadata(prepared, integrations); - // Only apply debug metadata to error events. + // Only put debug IDs onto frames for error events. if (event.type === undefined) { - applyDebugMetadata(prepared, options.stackParser); + applyDebugIds(prepared, options.stackParser); } // If we have scope given to us, use it as the base for further modifications. @@ -75,6 +75,14 @@ export function prepareEvent( } return result.then(evt => { + if (evt) { + // We apply the debug_meta field only after all event processors have ran, so that if any event processors modified + // file names (e.g.the RewriteFrames integration) the filename -> debug ID relationship isn't destroyed. + // This should not cause any PII issues, since we're only moving data that is already on the event and not adding + // any new data + applyDebugMeta(evt); + } + if (typeof normalizeDepth === 'number' && normalizeDepth > 0) { return normalizeEvent(evt, normalizeDepth, normalizeMaxBreadth); } @@ -121,9 +129,9 @@ function applyClientOptions(event: Event, options: ClientOptions): void { const debugIdStackParserCache = new WeakMap>(); /** - * Applies debug metadata images to the event in order to apply source maps by looking up their debug ID. + * Puts debug IDs into the stack frames of an error event. */ -export function applyDebugMetadata(event: Event, stackParser: StackParser): void { +export function applyDebugIds(event: Event, stackParser: StackParser): void { const debugIdMap = GLOBAL_OBJ._sentryDebugIds; if (!debugIdMap) { @@ -160,15 +168,39 @@ export function applyDebugMetadata(event: Event, stackParser: StackParser): void return acc; }, {}); - // Get a Set of filenames in the stack trace - const errorFileNames = new Set(); try { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion event!.exception!.values!.forEach(exception => { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion exception.stacktrace!.frames!.forEach(frame => { if (frame.filename) { - errorFileNames.add(frame.filename); + frame.debug_id = filenameDebugIdMap[frame.filename]; + } + }); + }); + } catch (e) { + // To save bundle size we're just try catching here instead of checking for the existence of all the different objects. + } +} + +/** + * Moves debug IDs from the stack frames of an error event into the debug_meta field. + */ +export function applyDebugMeta(event: Event): void { + // Extract debug IDs and filenames from the stack frames on the event. + const filenameDebugIdMap: Record = {}; + try { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + event.exception!.values!.forEach(exception => { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + exception.stacktrace!.frames!.forEach(frame => { + if (frame.debug_id) { + if (frame.abs_path) { + filenameDebugIdMap[frame.abs_path] = frame.debug_id; + } else if (frame.filename) { + filenameDebugIdMap[frame.filename] = frame.debug_id; + } + delete frame.debug_id; } }); }); @@ -176,18 +208,20 @@ export function applyDebugMetadata(event: Event, stackParser: StackParser): void // To save bundle size we're just try catching here instead of checking for the existence of all the different objects. } + if (Object.keys(filenameDebugIdMap).length === 0) { + return; + } + // Fill debug_meta information event.debug_meta = event.debug_meta || {}; event.debug_meta.images = event.debug_meta.images || []; const images = event.debug_meta.images; - errorFileNames.forEach(filename => { - if (filenameDebugIdMap[filename]) { - images.push({ - type: 'sourcemap', - code_file: filename, - debug_id: filenameDebugIdMap[filename], - }); - } + Object.keys(filenameDebugIdMap).forEach(filename => { + images.push({ + type: 'sourcemap', + code_file: filename, + debug_id: filenameDebugIdMap[filename], + }); }); } diff --git a/packages/core/test/lib/prepareEvent.test.ts b/packages/core/test/lib/prepareEvent.test.ts index d08b4ace221d..1e8b53e60f8c 100644 --- a/packages/core/test/lib/prepareEvent.test.ts +++ b/packages/core/test/lib/prepareEvent.test.ts @@ -1,14 +1,14 @@ import type { Event } from '@sentry/types'; import { createStackParser, GLOBAL_OBJ } from '@sentry/utils'; -import { applyDebugMetadata } from '../../src/utils/prepareEvent'; +import { applyDebugIds, applyDebugMeta } from '../../src/utils/prepareEvent'; -describe('applyDebugMetadata', () => { +describe('applyDebugIds', () => { afterEach(() => { GLOBAL_OBJ._sentryDebugIds = undefined; }); - it('should put debug source map images in debug_meta field', () => { + it("should put debug IDs into an event's stack frames", () => { GLOBAL_OBJ._sentryDebugIds = { 'filename1.js\nfilename1.js': 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', 'filename2.js\nfilename2.js': 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', @@ -34,35 +34,74 @@ describe('applyDebugMetadata', () => { }, }; - applyDebugMetadata(event, stackParser); + applyDebugIds(event, stackParser); - expect(event.debug_meta?.images).toContainEqual({ - type: 'sourcemap', - code_file: 'filename1.js', + expect(event.exception?.values?.[0].stacktrace?.frames).toContainEqual({ + filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', }); - expect(event.debug_meta?.images).toContainEqual({ - type: 'sourcemap', - code_file: 'filename2.js', + expect(event.exception?.values?.[0].stacktrace?.frames).toContainEqual({ + filename: 'filename2.js', debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', }); // expect not to contain an image for the stack frame that doesn't have a corresponding debug id - expect(event.debug_meta?.images).not.toContainEqual( + expect(event.exception?.values?.[0].stacktrace?.frames).not.toContainEqual( expect.objectContaining({ - type: 'sourcemap', - code_file: 'filename3.js', + filename3: 'filename3.js', + debug_id: expect.any(String), }), ); // expect not to contain an image for the debug id mapping that isn't contained in the stack trace - expect(event.debug_meta?.images).not.toContainEqual( + expect(event.exception?.values?.[0].stacktrace?.frames).not.toContainEqual( expect.objectContaining({ - type: 'sourcemap', - code_file: 'filename4.js', + filename3: 'filename4.js', debug_id: 'cccccccc-cccc-4ccc-cccc-cccccccccc', }), ); }); }); + +describe('applyDebugMeta', () => { + it("should move the debug IDs inside an event's stack frame into the debug_meta field", () => { + const event: Event = { + exception: { + values: [ + { + stacktrace: { + frames: [ + { filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa' }, + { filename: 'filename2.js', debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb' }, + { filename: 'filename1.js', debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa' }, + { filename: 'filename3.js' }, + ], + }, + }, + ], + }, + }; + + applyDebugMeta(event); + + expect(event.exception?.values?.[0].stacktrace?.frames).toEqual([ + { filename: 'filename1.js' }, + { filename: 'filename2.js' }, + { filename: 'filename1.js' }, + { filename: 'filename3.js' }, + ]); + + expect(event.debug_meta?.images).toContainEqual({ + type: 'sourcemap', + code_file: 'filename1.js', + debug_id: 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa', + }); + + expect(event.debug_meta?.images).toContainEqual({ + type: 'sourcemap', + code_file: 'filename2.js', + debug_id: 'bbbbbbbb-bbbb-4bbb-bbbb-bbbbbbbbbb', + }); + }); +}); From e6ea5375bfa45bebf6b46861169ecf848f2e7762 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 19 Jun 2023 12:53:53 +0200 Subject: [PATCH 5/8] fix(vue): Don't call `next` in Vue router 4 instrumentation (#8351) Vue Router 4 no longer exposes a `next` resolver function to call inside a `beforeEach` [router guard callback](https://router.vuejs.org/guide/advanced/navigation-guards.html#global-before-guards). Instead it will resolve the hook when the callback function returns. This PR checks if `next` is available and only calls it then. Also it adjusts the types accordingly and adds a test that previously failed. fixes #8349 --- packages/vue/src/router.ts | 9 +++++++-- packages/vue/test/router.test.ts | 28 +++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts index d8f87f14ad5d..25408f24f6b6 100644 --- a/packages/vue/src/router.ts +++ b/packages/vue/src/router.ts @@ -39,7 +39,7 @@ export type Route = { interface VueRouter { onError: (fn: (err: Error) => void) => void; - beforeEach: (fn: (to: Route, from: Route, next: () => void) => void) => void; + beforeEach: (fn: (to: Route, from: Route, next?: () => void) => void) => void; } /** @@ -129,7 +129,12 @@ export function vueRouterInstrumentation( }); } - next(); + // Vue Router 4 no longer exposes the `next` function, so we need to + // check if it's available before calling it. + // `next` needs to be called in Vue Router 3 so that the hook is resolved. + if (next) { + next(); + } }); }; } diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts index 2e8fd9a01d0d..044228181f50 100644 --- a/packages/vue/test/router.test.ts +++ b/packages/vue/test/router.test.ts @@ -9,7 +9,7 @@ const captureExceptionSpy = jest.spyOn(SentryBrowser, 'captureException'); const mockVueRouter = { onError: jest.fn void]>(), - beforeEach: jest.fn void) => void]>(), + beforeEach: jest.fn void) => void]>(), }; const mockStartTransaction = jest.fn(); @@ -324,4 +324,30 @@ describe('vueRouterInstrumentation()', () => { expect(mockStartTransaction).toHaveBeenCalledTimes(expectedCallsAmount + 1); }, ); + + it("doesn't throw when `next` is not available in the beforeEach callback (Vue Router 4)", () => { + const instrument = vueRouterInstrumentation(mockVueRouter, { routeLabel: 'path' }); + instrument(mockStartTransaction, true, true); + const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0]; + + const from = testRoutes.normalRoute1; + const to = testRoutes.namedRoute; + beforeEachCallback(to, from, undefined); + + // first startTx call happens when the instrumentation is initialized (for pageloads) + expect(mockStartTransaction).toHaveBeenLastCalledWith({ + name: '/login', + metadata: { + source: 'route', + }, + data: { + params: to.params, + query: to.query, + }, + op: 'navigation', + tags: { + 'routing.instrumentation': 'vue-router', + }, + }); + }); }); From 34bb40317bc0af261205fdddca81542748afcd56 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 19 Jun 2023 13:36:09 +0200 Subject: [PATCH 6/8] feat(replay): Stop replay when event buffer exceeds max. size (#8315) When the buffer exceeds ~20MB, stop the replay. Closes https://github.com/getsentry/sentry-javascript/issues/7657 Closes https://github.com/getsentry/team-replay/issues/94 --- packages/replay/src/constants.ts | 3 + .../src/eventBuffer/EventBufferArray.ts | 12 +++- .../EventBufferCompressionWorker.ts | 18 ++++- packages/replay/src/eventBuffer/index.ts | 8 +++ packages/replay/src/util/addEvent.ts | 5 +- .../unit/eventBuffer/EventBufferArray.test.ts | 55 ++++++++++++++- .../EventBufferCompressionWorker.test.ts | 70 ++++++++++++++++++- .../replay/test/unit/util/addEvent.test.ts | 28 ++++++++ 8 files changed, 192 insertions(+), 7 deletions(-) diff --git a/packages/replay/src/constants.ts b/packages/replay/src/constants.ts index 120079ebb857..699cefe8b784 100644 --- a/packages/replay/src/constants.ts +++ b/packages/replay/src/constants.ts @@ -44,3 +44,6 @@ export const SLOW_CLICK_THRESHOLD = 3_000; 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/eventBuffer/EventBufferArray.ts b/packages/replay/src/eventBuffer/EventBufferArray.ts index eaebd1b174e7..a7b363891026 100644 --- a/packages/replay/src/eventBuffer/EventBufferArray.ts +++ b/packages/replay/src/eventBuffer/EventBufferArray.ts @@ -1,5 +1,7 @@ +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { AddEventResult, EventBuffer, EventBufferType, RecordingEvent } from '../types'; import { timestampToMs } from '../util/timestampToMs'; +import { EventBufferSizeExceededError } from '.'; /** * A basic event buffer that does not do any compression. @@ -8,6 +10,7 @@ import { timestampToMs } from '../util/timestampToMs'; export class EventBufferArray implements EventBuffer { /** All the events that are buffered to be sent. */ public events: RecordingEvent[]; + private _totalSize = 0; public constructor() { this.events = []; @@ -30,6 +33,12 @@ export class EventBufferArray implements EventBuffer { /** @inheritdoc */ public async addEvent(event: RecordingEvent): Promise { + const eventSize = JSON.stringify(event).length; + this._totalSize += eventSize; + if (this._totalSize > REPLAY_MAX_EVENT_BUFFER_SIZE) { + throw new EventBufferSizeExceededError(); + } + this.events.push(event); } @@ -40,7 +49,7 @@ export class EventBufferArray implements EventBuffer { // events member so that we do not lose new events while uploading // attachment. const eventsRet = this.events; - this.events = []; + this.clear(); resolve(JSON.stringify(eventsRet)); }); } @@ -48,6 +57,7 @@ export class EventBufferArray implements EventBuffer { /** @inheritdoc */ public clear(): void { this.events = []; + this._totalSize = 0; } /** @inheritdoc */ diff --git a/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts b/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts index 45696ea46bc9..5b4c0eb4487a 100644 --- a/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts +++ b/packages/replay/src/eventBuffer/EventBufferCompressionWorker.ts @@ -1,7 +1,9 @@ import type { ReplayRecordingData } from '@sentry/types'; +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { AddEventResult, EventBuffer, EventBufferType, RecordingEvent } from '../types'; import { timestampToMs } from '../util/timestampToMs'; +import { EventBufferSizeExceededError } from '.'; import { WorkerHandler } from './WorkerHandler'; /** @@ -11,6 +13,7 @@ import { WorkerHandler } from './WorkerHandler'; export class EventBufferCompressionWorker implements EventBuffer { private _worker: WorkerHandler; private _earliestTimestamp: number | null; + private _totalSize = 0; public constructor(worker: Worker) { this._worker = new WorkerHandler(worker); @@ -53,7 +56,14 @@ export class EventBufferCompressionWorker implements EventBuffer { this._earliestTimestamp = timestamp; } - return this._sendEventToWorker(event); + const data = JSON.stringify(event); + this._totalSize += data.length; + + if (this._totalSize > REPLAY_MAX_EVENT_BUFFER_SIZE) { + return Promise.reject(new EventBufferSizeExceededError()); + } + + return this._sendEventToWorker(data); } /** @@ -66,6 +76,7 @@ export class EventBufferCompressionWorker implements EventBuffer { /** @inheritdoc */ public clear(): void { this._earliestTimestamp = null; + this._totalSize = 0; // We do not wait on this, as we assume the order of messages is consistent for the worker void this._worker.postMessage('clear'); } @@ -78,8 +89,8 @@ export class EventBufferCompressionWorker implements EventBuffer { /** * Send the event to the worker. */ - private _sendEventToWorker(event: RecordingEvent): Promise { - return this._worker.postMessage('addEvent', JSON.stringify(event)); + private _sendEventToWorker(data: string): Promise { + return this._worker.postMessage('addEvent', data); } /** @@ -89,6 +100,7 @@ export class EventBufferCompressionWorker implements EventBuffer { const response = await this._worker.postMessage('finish'); this._earliestTimestamp = null; + this._totalSize = 0; return response; } diff --git a/packages/replay/src/eventBuffer/index.ts b/packages/replay/src/eventBuffer/index.ts index f0eb83c68243..fe58b76f3c7b 100644 --- a/packages/replay/src/eventBuffer/index.ts +++ b/packages/replay/src/eventBuffer/index.ts @@ -1,6 +1,7 @@ import { getWorkerURL } from '@sentry-internal/replay-worker'; import { logger } from '@sentry/utils'; +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../constants'; import type { EventBuffer } from '../types'; import { EventBufferArray } from './EventBufferArray'; import { EventBufferProxy } from './EventBufferProxy'; @@ -30,3 +31,10 @@ export function createEventBuffer({ useCompression }: CreateEventBufferParams): __DEBUG_BUILD__ && logger.log('[Replay] Using simple buffer'); return new EventBufferArray(); } + +/** This error indicates that the event buffer size exceeded the limit.. */ +export class EventBufferSizeExceededError extends Error { + public constructor() { + super(`Event buffer exceeded maximum size of ${REPLAY_MAX_EVENT_BUFFER_SIZE}.`); + } +} diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index 16f653e9fc5d..79bf4ff2f362 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -2,6 +2,7 @@ import { EventType } from '@sentry-internal/rrweb'; import { getCurrentHub } from '@sentry/core'; import { logger } from '@sentry/utils'; +import { EventBufferSizeExceededError } from '../eventBuffer'; import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayFrameEvent } from '../types'; import { timestampToMs } from './timestampToMs'; @@ -56,8 +57,10 @@ export async function addEvent( return await replay.eventBuffer.addEvent(eventAfterPossibleCallback); } catch (error) { + const reason = error && error instanceof EventBufferSizeExceededError ? 'addEventSizeExceeded' : 'addEvent'; + __DEBUG_BUILD__ && logger.error(error); - await replay.stop('addEvent'); + await replay.stop(reason); const client = getCurrentHub().getClient(); diff --git a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts index 37827610e4cc..c7b0a4bd7e90 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferArray.test.ts @@ -1,4 +1,5 @@ -import { createEventBuffer } from './../../../src/eventBuffer'; +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; +import { createEventBuffer, EventBufferSizeExceededError } from './../../../src/eventBuffer'; import { BASE_TIMESTAMP } from './../../index'; const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; @@ -44,4 +45,56 @@ describe('Unit | eventBuffer | EventBufferArray', () => { expect(result1).toEqual(JSON.stringify([TEST_EVENT])); expect(result2).toEqual(JSON.stringify([])); }); + + describe('size limit', () => { + it('rejects if size exceeds limit', async function () { + const buffer = createEventBuffer({ useCompression: false }); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + // Now it should error + await expect(() => buffer.addEvent(largeEvent)).rejects.toThrowError(EventBufferSizeExceededError); + }); + + it('resets size limit on clear', async function () { + const buffer = createEventBuffer({ useCompression: false }); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + await buffer.clear(); + + await buffer.addEvent(largeEvent); + }); + + it('resets size limit on finish', async function () { + const buffer = createEventBuffer({ useCompression: false }); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + await buffer.finish(); + + await buffer.addEvent(largeEvent); + }); + }); }); diff --git a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts index 74819a2cdf75..6c3e5948fac1 100644 --- a/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts +++ b/packages/replay/test/unit/eventBuffer/EventBufferCompressionWorker.test.ts @@ -3,8 +3,9 @@ import 'jsdom-worker'; import pako from 'pako'; import { BASE_TIMESTAMP } from '../..'; +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; import { EventBufferProxy } from '../../../src/eventBuffer/EventBufferProxy'; -import { createEventBuffer } from './../../../src/eventBuffer'; +import { createEventBuffer, EventBufferSizeExceededError } from './../../../src/eventBuffer'; const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 }; @@ -146,4 +147,71 @@ describe('Unit | eventBuffer | EventBufferCompressionWorker', () => { await expect(() => buffer.addEvent({ data: { o: 3 }, timestamp: BASE_TIMESTAMP, type: 3 })).rejects.toBeDefined(); }); + + describe('size limit', () => { + it('rejects if size exceeds limit', async function () { + const buffer = createEventBuffer({ + useCompression: true, + }) as EventBufferProxy; + + expect(buffer).toBeInstanceOf(EventBufferProxy); + await buffer.ensureWorkerIsLoaded(); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + // Now it should error + await expect(() => buffer.addEvent(largeEvent)).rejects.toThrowError(EventBufferSizeExceededError); + }); + + it('resets size limit on clear', async function () { + const buffer = createEventBuffer({ + useCompression: true, + }) as EventBufferProxy; + + expect(buffer).toBeInstanceOf(EventBufferProxy); + await buffer.ensureWorkerIsLoaded(); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + await buffer.clear(); + + await buffer.addEvent(largeEvent); + }); + + it('resets size limit on finish', async function () { + const buffer = createEventBuffer({ + useCompression: true, + }) as EventBufferProxy; + + expect(buffer).toBeInstanceOf(EventBufferProxy); + await buffer.ensureWorkerIsLoaded(); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await buffer.addEvent(largeEvent); + await buffer.addEvent(largeEvent); + + await buffer.finish(); + + await buffer.addEvent(largeEvent); + }); + }); }); diff --git a/packages/replay/test/unit/util/addEvent.test.ts b/packages/replay/test/unit/util/addEvent.test.ts index 6cc2e6b6ffdf..f00dc82d338f 100644 --- a/packages/replay/test/unit/util/addEvent.test.ts +++ b/packages/replay/test/unit/util/addEvent.test.ts @@ -1,6 +1,7 @@ import 'jsdom-worker'; import { BASE_TIMESTAMP } from '../..'; +import { REPLAY_MAX_EVENT_BUFFER_SIZE } from '../../../src/constants'; import type { EventBufferProxy } from '../../../src/eventBuffer/EventBufferProxy'; import { addEvent } from '../../../src/util/addEvent'; import { setupReplayContainer } from '../../utils/setupReplayContainer'; @@ -29,4 +30,31 @@ describe('Unit | util | addEvent', () => { expect(replay.isEnabled()).toEqual(false); }); + + it('stops when exceeding buffer size limit', async function () { + jest.setSystemTime(BASE_TIMESTAMP); + + const replay = setupReplayContainer({ + options: { + useCompression: true, + }, + }); + + const largeEvent = { + data: { a: 'a'.repeat(REPLAY_MAX_EVENT_BUFFER_SIZE / 3) }, + timestamp: BASE_TIMESTAMP, + type: 3, + }; + + await (replay.eventBuffer as EventBufferProxy).ensureWorkerIsLoaded(); + + await addEvent(replay, largeEvent); + await addEvent(replay, largeEvent); + + expect(replay.isEnabled()).toEqual(true); + + await addEvent(replay, largeEvent); + + expect(replay.isEnabled()).toEqual(false); + }); }); From 1bdf2d942eabc696e4dc8e2df69b3b7c80b9e279 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 19 Jun 2023 14:56:30 +0200 Subject: [PATCH 7/8] feat(replay): Consider `window.open` for slow clicks (#8308) When a click triggers `window.open()`, do not consider it a slow click. Closes https://github.com/getsentry/sentry-javascript/issues/8301 --- .../suites/replay/slowClick/template.html | 4 ++ .../replay/slowClick/windowOpen/test.ts | 69 +++++++++++++++++++ .../replay/src/coreHandlers/handleClick.ts | 7 ++ .../src/coreHandlers/util/onWindowOpen.ts | 44 ++++++++++++ 4 files changed, 124 insertions(+) create mode 100644 packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts create mode 100644 packages/replay/src/coreHandlers/util/onWindowOpen.ts diff --git a/packages/browser-integration-tests/suites/replay/slowClick/template.html b/packages/browser-integration-tests/suites/replay/slowClick/template.html index f49c8b1d410d..19bd283db90e 100644 --- a/packages/browser-integration-tests/suites/replay/slowClick/template.html +++ b/packages/browser-integration-tests/suites/replay/slowClick/template.html @@ -19,6 +19,7 @@ + Link Link external @@ -73,6 +74,9 @@

Bottom

document.getElementById('mouseDownButton').addEventListener('mousedown', () => { document.getElementById('out').innerHTML += 'mutationButton clicked
'; }); + document.getElementById('windowOpenButton').addEventListener('click', () => { + window.open('https://example.com/', '_blank'); + }); // Do nothing on these elements document diff --git a/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts new file mode 100644 index 000000000000..4c9401234ea2 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/slowClick/windowOpen/test.ts @@ -0,0 +1,69 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; + +sentryTest('window.open() is considered for slow click', async ({ getLocalTestUrl, page, browser }) => { + 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; + + const reqPromise1 = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + // Ensure window.open() still works as expected + const context = browser.contexts()[0]; + const waitForNewPage = context.waitForEvent('page'); + + await page.click('#windowOpenButton'); + + const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1); + + // Filter out potential blur breadcrumb, as otherwise this can be flaky + const filteredBreadcrumb = breadcrumbs.filter(breadcrumb => breadcrumb.category !== 'ui.blur'); + + expect(filteredBreadcrumb).toEqual([ + { + category: 'ui.click', + data: { + node: { + attributes: { + id: 'windowOpenButton', + }, + id: expect.any(Number), + tagName: 'button', + textContent: '****** ****', + }, + nodeId: expect.any(Number), + }, + message: 'body > button#windowOpenButton', + timestamp: expect.any(Number), + type: 'default', + }, + ]); + + await waitForNewPage; + + const pages = context.pages(); + + expect(pages.length).toBe(2); + expect(pages[1].url()).toBe('https://example.com/'); +}); diff --git a/packages/replay/src/coreHandlers/handleClick.ts b/packages/replay/src/coreHandlers/handleClick.ts index 5a75413b00d1..c5a65ddfdca0 100644 --- a/packages/replay/src/coreHandlers/handleClick.ts +++ b/packages/replay/src/coreHandlers/handleClick.ts @@ -4,6 +4,7 @@ import { WINDOW } from '../constants'; import type { MultiClickFrame, ReplayClickDetector, ReplayContainer, SlowClickConfig, SlowClickFrame } from '../types'; import { addBreadcrumbEvent } from './util/addBreadcrumbEvent'; import { getClickTargetNode } from './util/domUtils'; +import { onWindowOpen } from './util/onWindowOpen'; type ClickBreadcrumb = Breadcrumb & { timestamp: number; @@ -68,6 +69,11 @@ export class ClickDetector implements ReplayClickDetector { this._lastScroll = nowInSeconds(); }; + const cleanupWindowOpen = onWindowOpen(() => { + // Treat window.open as mutation + this._lastMutation = nowInSeconds(); + }); + const clickHandler = (event: MouseEvent): void => { if (!event.target) { return; @@ -94,6 +100,7 @@ export class ClickDetector implements ReplayClickDetector { this._teardown = () => { WINDOW.removeEventListener('scroll', scrollHandler); WINDOW.removeEventListener('click', clickHandler); + cleanupWindowOpen(); obs.disconnect(); this._clicks = []; diff --git a/packages/replay/src/coreHandlers/util/onWindowOpen.ts b/packages/replay/src/coreHandlers/util/onWindowOpen.ts new file mode 100644 index 000000000000..e3b6b7ac92ed --- /dev/null +++ b/packages/replay/src/coreHandlers/util/onWindowOpen.ts @@ -0,0 +1,44 @@ +import { fill } from '@sentry/utils'; + +import { WINDOW } from '../../constants'; + +type WindowOpenHandler = () => void; + +let handlers: undefined | WindowOpenHandler[]; + +/** + * Register a handler to be called when `window.open()` is called. + * Returns a cleanup function. + */ +export function onWindowOpen(cb: WindowOpenHandler): () => void { + // Ensure to only register this once + if (!handlers) { + handlers = []; + monkeyPatchWindowOpen(); + } + + handlers.push(cb); + + return () => { + const pos = handlers ? handlers.indexOf(cb) : -1; + if (pos > -1) { + (handlers as WindowOpenHandler[]).splice(pos, 1); + } + }; +} + +function monkeyPatchWindowOpen(): void { + fill(WINDOW, 'open', function (originalWindowOpen: () => void): () => void { + return function (...args: unknown[]): void { + if (handlers) { + try { + handlers.forEach(handler => handler()); + } catch (e) { + // ignore errors in here + } + } + + return originalWindowOpen.apply(WINDOW, args); + }; + }); +} From e41ce248471ca6c7f66be6ec9384950c5ff13e53 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 19 Jun 2023 13:37:43 +0200 Subject: [PATCH 8/8] meta(changelog): Update changelog for 7.56.0 --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ceb218a3a05..da270fb1ccec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 7.56.0 + +- feat(replay): Rework slow click & multi click detection (#8322) +- feat(replay): Stop replay when event buffer exceeds max. size (#8315) +- feat(replay): Consider `window.open` for slow clicks (#8308) +- fix(core): Temporarily store debug IDs in stack frame and only put them into `debug_meta` before sending (#8347) +- fix(remix): Extract deferred responses correctly in root loaders. (#8305) +- fix(vue): Don't call `next` in Vue router 4 instrumentation (#8351) + ## 7.55.2 - fix(replay): Stop exporting `EventType` from `@sentry-internal/rrweb` (#8334)