fix(utils): Avoid keeping a reference of last used event

mydea · mydea · commit 3f15aabe60f2 · 2023-10-30T10:15:17.000+01:00
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/template.html b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/template.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title></title>
+  </head>
+  <body>
+    <button id="button1" type="button">Button 1</button>
+    <button id="button2" type="button">Button 2</button>
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/test.ts b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/test.ts
@@ -0,0 +1,57 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('captures Breadcrumb for clicks & debounces them for a second', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  await page.route('**/foo', route => {
+    return route.fulfill({
+      status: 200,
+      body: JSON.stringify({
+        userNames: ['John', 'Jane'],
+      }),
+      headers: {
+        'Content-Type': 'application/json',
+      },
+    });
+  });
+
+  const promise = getFirstSentryEnvelopeRequest<Event>(page, url);
+
+  await page.click('#button1');
+  // not debounced because other target
+  await page.click('#button2');
+  // This should be debounced
+  await page.click('#button2');
+
+  // Wait a second for the debounce to finish
+  await page.waitForTimeout(1000);
+  await page.click('#button2');
+
+  await page.evaluate('Sentry.captureException("test exception")');
+
+  const eventData = await promise;
+
+  expect(eventData.exception?.values).toHaveLength(1);
+
+  expect(eventData.breadcrumbs).toEqual([
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button1[type="button"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button2[type="button"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button2[type="button"]',
+    },
+  ]);
+});
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/init.js b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/init.js
@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  defaultIntegrations: false,
+  integrations: [new Sentry.Integrations.Breadcrumbs()],
+  sampleRate: 1,
+});
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/template.html b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/template.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title></title>
+  </head>
+  <body>
+    <input id="input1" type="text" />
+    <input id="input2" type="text" />
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/test.ts b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/test.ts
@@ -0,0 +1,64 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('captures Breadcrumb for events on inputs & debounced them', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  await page.route('**/foo', route => {
+    return route.fulfill({
+      status: 200,
+      body: JSON.stringify({
+        userNames: ['John', 'Jane'],
+      }),
+      headers: {
+        'Content-Type': 'application/json',
+      },
+    });
+  });
+
+  const promise = getFirstSentryEnvelopeRequest<Event>(page, url);
+
+  await page.click('#input1');
+  // Not debounced because other event type
+  await page.type('#input1', 'John');
+  // This should be debounced
+  await page.type('#input1', 'Abby');
+  // not debounced because other target
+  await page.type('#input2', 'Anne');
+
+  // Wait a second for the debounce to finish
+  await page.waitForTimeout(1000);
+  await page.type('#input2', 'John');
+
+  await page.evaluate('Sentry.captureException("test exception")');
+
+  const eventData = await promise;
+
+  expect(eventData.exception?.values).toHaveLength(1);
+
+  expect(eventData.breadcrumbs).toEqual([
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > input#input1[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input1[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input2[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input2[type="text"]',
+    },
+  ]);
+});
diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts
@@ -12,6 +12,7 @@ import type {
 import { isString } from './is';
 import type { ConsoleLevel } from './logger';
 import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger';
+import { uuid4 } from './misc';
 import { addNonEnumerableProperty, fill } from './object';
 import { getFunctionName } from './stacktrace';
 import { supportsHistory, supportsNativeFetch } from './supports';
@@ -404,21 +405,24 @@ function instrumentHistory(): void {
 
 const DEBOUNCE_DURATION = 1000;
 let debounceTimerID: number | undefined;
-let lastCapturedEvent: Event | undefined;
+let lastCapturedEventType: string | undefined;
+let lastCapturedEventTargetId: string | undefined;
+
+type SentryWrappedTarget = EventTarget & { _sentryId?: string };
 
 /**
- * Check whether two DOM events are similar to eachother. For example, two click events on the same button.
+ * Check whether the event is similar to the last captured one. For example, two click events on the same button.
  */
-function areSimilarDomEvents(a: Event, b: Event): boolean {
+function isSimilarToLastCapturedEvent(event: Event): boolean {
   // If both events have different type, then user definitely performed two separate actions. e.g. click + keypress.
-  if (a.type !== b.type) {
+  if (event.type !== lastCapturedEventType) {
     return false;
   }
 
   try {
     // If both events have the same type, it's still possible that actions were performed on different targets.
     // e.g. 2 clicks on different buttons.
-    if (a.target !== b.target) {
+    if (!event.target || (event.target as SentryWrappedTarget)._sentryId !== lastCapturedEventTargetId) {
       return false;
     }
   } catch (e) {
@@ -486,24 +490,31 @@ function makeDOMEventHandler(handler: Function, globalListener: boolean = false)
     // Mark event as "seen"
     addNonEnumerableProperty(event, '_sentryCaptured', true);
 
+    if (event.target && !(event.target as SentryWrappedTarget)._sentryId) {
+      // Add UUID to event target so we can identify if
+      addNonEnumerableProperty(event.target, '_sentryId', uuid4());
+    }
+
     const name = event.type === 'keypress' ? 'input' : event.type;
 
     // If there is no last captured event, it means that we can safely capture the new event and store it for future comparisons.
     // If there is a last captured event, see if the new event is different enough to treat it as a unique one.
     // If that's the case, emit the previous event and store locally the newly-captured DOM event.
-    if (lastCapturedEvent === undefined || !areSimilarDomEvents(lastCapturedEvent, event)) {
+    if (!isSimilarToLastCapturedEvent(event)) {
       handler({
         event: event,
         name,
         global: globalListener,
       });
-      lastCapturedEvent = event;
+      lastCapturedEventType = event.type;
+      lastCapturedEventTargetId = event.target ? (event.target as SentryWrappedTarget)._sentryId : undefined;
     }
 
     // Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together.
     clearTimeout(debounceTimerID);
     debounceTimerID = WINDOW.setTimeout(() => {
-      lastCapturedEvent = undefined;
+      lastCapturedEventTargetId = undefined;
+      lastCapturedEventType = undefined;
     }, DEBOUNCE_DURATION);
   };
 }
