fix(insights): prevent duplicate events

in sendEvent, we mark internal events (click and conversion), and prevent them being sent if there was a custom click right before. Note that this doesn't yet work with bindEvent, as it's higher in the bubbling tree
algolia · Mar 3, 2023 · 2b84e99 · 2b84e99
1 parent bb4c356
commit 2b84e99
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 81 deletions.
diff --git a/packages/instantsearch.js/src/components/Hits/Hits.tsx b/packages/instantsearch.js/src/components/Hits/Hits.tsx
@@ -56,7 +56,7 @@ const Hits = ({
             rootProps={{
               className: cssClasses.item,
               onClick: () => {
-                sendEvent('click', hit, 'Hit Clicked');
+                sendEvent('click:internal', hit, 'Hit Clicked');
               },
             }}
             key={hit.objectID}

diff --git a/packages/instantsearch.js/src/components/Hits/__tests__/Hits.test.tsx b/packages/instantsearch.js/src/components/Hits/__tests__/Hits.test.tsx
@@ -28,7 +28,7 @@ describe('Hits', () => {
 
     expect(props.sendEvent).toHaveBeenCalledTimes(1);
     expect(props.sendEvent).toHaveBeenLastCalledWith(
-      'click',
+      'click:internal',
       props.hits[0],
       'Hit Clicked'
     );

diff --git a/packages/instantsearch.js/src/components/InfiniteHits/InfiniteHits.tsx b/packages/instantsearch.js/src/components/InfiniteHits/InfiniteHits.tsx
@@ -88,7 +88,7 @@ const InfiniteHits = ({
             rootProps={{
               className: cssClasses.item,
               onClick: () => {
-                sendEvent('click', hit, 'Hit Clicked');
+                sendEvent('click:internal', hit, 'Hit Clicked');
               },
             }}
             key={hit.objectID}

diff --git a/packages/instantsearch.js/src/components/InfiniteHits/__tests__/InfiniteHits.test.tsx b/packages/instantsearch.js/src/components/InfiniteHits/__tests__/InfiniteHits.test.tsx
@@ -28,7 +28,7 @@ describe('InfiniteHits', () => {
 
     expect(props.sendEvent).toHaveBeenCalledTimes(1);
     expect(props.sendEvent).toHaveBeenLastCalledWith(
-      'click',
+      'click:internal',
       props.hits[0],
       'Hit Clicked'
     );

diff --git a/packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts b/packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts
@@ -41,12 +41,17 @@ const buildPayloads = ({
   methodName: 'sendEvent' | 'bindEvent';
   args: any[];
   instantSearchInstance: InstantSearch;
-}): InsightsEvent[] => {
+}): {
+  payloads: InsightsEvent[];
+  eventModifier?: string;
+  preventNextInternalEvent?: boolean;
+} => {
   // when there's only one argument, that means it's custom
   if (args.length === 1 && typeof args[0] === 'object') {
-    return [args[0]];
+    return { payloads: [args[0]] };
   }
-  const eventType: string = args[0];
+  const [eventType, eventModifier]: [string, string] = args[0].split(':');
+
   const hits: Hit | Hit[] | EscapedHits = args[1];
   const eventName: string | undefined = args[2];
   if (!hits) {
@@ -57,7 +62,7 @@ const buildPayloads = ({
   `
       );
     } else {
-      return [];
+      return { payloads: [] };
     }
   }
   if ((eventType === 'click' || eventType === 'conversion') && !eventName) {
@@ -70,15 +75,15 @@ const buildPayloads = ({
   `
       );
     } else {
-      return [];
+      return { payloads: [] };
     }
   }
   const hitsArray: Hit[] = Array.isArray(hits)
     ? removeEscapedFromHits(hits)
     : [hits];
 
   if (hitsArray.length === 0) {
-    return [];
+    return { payloads: [] };
   }
   const queryID = hitsArray[0].__queryID;
   const hitsChunks = chunk(hitsArray);
@@ -91,58 +96,69 @@ const buildPayloads = ({
 
   if (eventType === 'view') {
     if (instantSearchInstance.status !== 'idle') {
-      return [];
+      return { payloads: [] };
     }
-    return hitsChunks.map((batch, i) => {
-      return {
-        insightsMethod: 'viewedObjectIDs',
-        widgetType,
-        eventType,
-        payload: {
-          eventName: eventName || 'Hits Viewed',
-          index,
-          objectIDs: objectIDsByChunk[i],
-        },
-        hits: batch,
-      };
-    });
+    return {
+      payloads: hitsChunks.map((batch, i) => {
+        return {
+          insightsMethod: 'viewedObjectIDs',
+          widgetType,
+          eventType,
+          payload: {
+            eventName: eventName || 'Hits Viewed',
+            index,
+            objectIDs: objectIDsByChunk[i],
+          },
+          hits: batch,
+        };
+      }),
+      eventModifier,
+    };
   } else if (eventType === 'click') {
-    return hitsChunks.map((batch, i) => {
-      return {
-        insightsMethod: 'clickedObjectIDsAfterSearch',
-        widgetType,
-        eventType,
-        payload: {
-          eventName,
-          index,
-          queryID,
-          objectIDs: objectIDsByChunk[i],
-          positions: positionsByChunk[i],
-        },
-        hits: batch,
-      };
-    });
+    return {
+      payloads: hitsChunks.map((batch, i) => {
+        return {
+          insightsMethod: 'clickedObjectIDsAfterSearch',
+          widgetType,
+          eventType,
+          payload: {
+            eventName,
+            index,
+            queryID,
+            objectIDs: objectIDsByChunk[i],
+            positions: positionsByChunk[i],
+          },
+          hits: batch,
+        };
+      }),
+      eventModifier,
+      preventNextInternalEvent: eventModifier !== 'internal',
+    };
   } else if (eventType === 'conversion') {
-    return hitsChunks.map((batch, i) => {
-      return {
-        insightsMethod: 'convertedObjectIDsAfterSearch',
-        widgetType,
-        eventType,
-        payload: {
-          eventName,
-          index,
-          queryID,
-          objectIDs: objectIDsByChunk[i],
-        },
-        hits: batch,
-      };
-    });
+    return {
+      payloads: hitsChunks.map((batch, i) => {
+        return {
+          insightsMethod: 'convertedObjectIDsAfterSearch',
+          widgetType,
+          eventType,
+          payload: {
+            eventName,
+            index,
+            queryID,
+            objectIDs: objectIDsByChunk[i],
+          },
+          hits: batch,
+        };
+      }),
+      eventModifier,
+      preventNextInternalEvent: eventModifier !== 'internal',
+    };
   } else if (__DEV__) {
     throw new Error(`eventType("${eventType}") is not supported.
     If you want to send a custom payload, you can pass one object: ${methodName}(customPayload);
     `);
   } else {
-    return [];
+    return { payloads: [] };
   }
 };
 
@@ -160,14 +176,28 @@ export function createSendEventForHits({
   index: string;
   widgetType: string;
 }): SendEventForHits {
+  let shouldSendInternalEvent = true;
+
   const sendEventForHits: SendEventForHits = (...args: any[]) => {
-    const payloads = buildPayloads({
-      widgetType,
-      index,
-      methodName: 'sendEvent',
-      args,
-      instantSearchInstance,
-    });
+    const { payloads, eventModifier, preventNextInternalEvent } = buildPayloads(
+      {
+        widgetType,
+        index,
+        methodName: 'sendEvent',
+        args,
+        instantSearchInstance,
+      }
+    );
+
+    if (eventModifier === 'internal' && !shouldSendInternalEvent) {
+      // don't send internal events, but still send the next one
+      shouldSendInternalEvent = true;
+      return;
+    } else if (eventModifier === 'internal') {
+      shouldSendInternalEvent = true;
+    } else if (preventNextInternalEvent) {
+      shouldSendInternalEvent = false;
+    }
 
     payloads.forEach((payload) =>
       instantSearchInstance.sendEventToInsights(payload)
@@ -186,7 +216,7 @@ export function createBindEventForHits({
   instantSearchInstance: InstantSearch;
 }): BindEventForHits {
   const bindEventForHits: BindEventForHits = (...args: any[]) => {
-    const payloads = buildPayloads({
+    const { payloads } = buildPayloads({
       widgetType,
       index,
       methodName: 'bindEvent',

diff --git a/packages/instantsearch.js/src/widgets/hits/__tests__/hits-integration-test.ts b/packages/instantsearch.js/src/widgets/hits/__tests__/hits-integration-test.ts
@@ -123,7 +123,7 @@ describe('hits', () => {
       );
     });
 
-    test('sends a default `click` event when clicking on a hit', async () => {
+    test.only('sends a default `click` event when clicking on a hit', async () => {
       const { search } = createInstantSearch();
       const { insights, onEvent } = createInsightsMiddlewareWithOnEvent();
 
@@ -190,8 +190,8 @@ describe('hits', () => {
 
       fireEvent.click(getByText(container, 'title 1'));
 
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
       expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'click',
         hits: [
@@ -242,8 +242,8 @@ describe('hits', () => {
       onEvent.mockClear();
 
       fireEvent.click(getByText(container, 'title 2'));
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
       expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'conversion',
         hits: [
@@ -289,9 +289,9 @@ describe('hits', () => {
       onEvent.mockClear();
 
       fireEvent.click(getByText(container, 'title 1'));
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
-      expect(onEvent.mock.calls[1][0]).toEqual({
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
+      expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'click',
         hits: [
           {
@@ -342,9 +342,9 @@ describe('hits', () => {
 
       fireEvent.click(getByText(container, 'title 2'));
 
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
-      expect(onEvent.mock.calls[1][0]).toEqual({
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
+      expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'conversion',
         hits: [
           {

diff --git a/...es/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts b/...es/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts
@@ -434,9 +434,9 @@ describe('infiniteHits', () => {
 
       fireEvent.click(getByText(container, 'title 1'));
 
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
-      expect(onEvent.mock.calls[onEvent.mock.calls.length - 1][0]).toEqual({
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
+      expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'click',
         hits: [
           {
@@ -486,9 +486,9 @@ describe('infiniteHits', () => {
 
       fireEvent.click(getByText(container, 'title 2'));
 
-      // The default `click` one + the custom one
-      expect(onEvent).toHaveBeenCalledTimes(2);
-      expect(onEvent.mock.calls[onEvent.mock.calls.length - 1][0]).toEqual({
+      // The custom one only
+      expect(onEvent).toHaveBeenCalledTimes(1);
+      expect(onEvent.mock.calls[0][0]).toEqual({
         eventType: 'conversion',
         hits: [
           {

diff --git a/packages/react-instantsearch-hooks-web/src/ui/Hits.tsx b/packages/react-instantsearch-hooks-web/src/ui/Hits.tsx
@@ -65,7 +65,7 @@ export function Hits<THit extends Hit>({
             key={hit.objectID}
             className={cx('ais-Hits-item', classNames.item)}
             onClick={() => {
-              sendEvent('click', hit, 'Hit Clicked');
+              sendEvent('click:internal', hit, 'Hit Clicked');
             }}
           >
             <HitComponent hit={hit} sendEvent={sendEvent} />

diff --git a/packages/react-instantsearch-hooks-web/src/ui/__tests__/Hits.test.tsx b/packages/react-instantsearch-hooks-web/src/ui/__tests__/Hits.test.tsx
@@ -211,7 +211,7 @@ describe('Hits', () => {
 
     expect(props.sendEvent).toHaveBeenCalledTimes(1);
     expect(props.sendEvent).toHaveBeenLastCalledWith(
-      'click',
+      'click:internal',
       props.hits[0],
       'Hit Clicked'
     );