From cd5bdfd534b2b0c824fbb8b9035eaf976c14fdec Mon Sep 17 00:00:00 2001 From: Lew Gordon <167105523+lewgordon-amplitude@users.noreply.github.com> Date: Mon, 16 Dec 2024 09:26:36 -0800 Subject: [PATCH] fix(session-replay-browser): prevent finder errors from affecting session replay (#934) * fix(session-replay-browser): only setup hooks if interaction config is enabled * fix(session-replay-browser): prevent finder errors from affecting session replay capture --- .../session-replay-browser/src/hooks/click.ts | 11 +++- .../src/session-replay.ts | 26 ++++---- .../test/hooks/click.test.ts | 64 ++++++++++++++++++- .../test/session-replay.test.ts | 35 ++++++++++ 4 files changed, 119 insertions(+), 17 deletions(-) diff --git a/packages/session-replay-browser/src/hooks/click.ts b/packages/session-replay-browser/src/hooks/click.ts index a70abd6be..bd57a2911 100644 --- a/packages/session-replay-browser/src/hooks/click.ts +++ b/packages/session-replay-browser/src/hooks/click.ts @@ -4,6 +4,7 @@ import { SessionReplayEventsManager as AmplitudeSessionReplayEventsManager } fro import { PayloadBatcher } from 'src/track-destination'; import { finder } from '../libs/finder'; import { getGlobalScope } from '@amplitude/analytics-client-common'; +import type { Logger } from '@amplitude/analytics-types'; // exported for testing export type ClickEvent = { @@ -67,8 +68,8 @@ export const clickBatcher: PayloadBatcher = ({ version, events }) => { return { version, events: Object.values(reduced) }; }; -export const clickHook: (options: Options) => mouseInteractionCallBack = - ({ eventsManager, sessionId, deviceIdFn }) => +export const clickHook: (logger: Logger, options: Options) => mouseInteractionCallBack = + (logger, { eventsManager, sessionId, deviceIdFn }) => (e) => { if (e.type !== MouseInteractions.Click) { return; @@ -93,7 +94,11 @@ export const clickHook: (options: Options) => mouseInteractionCallBack = const node = record.mirror.getNode(e.id); let selector; if (node) { - selector = finder(node as Element); + try { + selector = finder(node as Element); + } catch (err) { + logger.debug('error resolving selector from finder'); + } } const { left: scrollX, top: scrollY } = utils.getWindowScroll(globalScope as unknown as Window); diff --git a/packages/session-replay-browser/src/session-replay.ts b/packages/session-replay-browser/src/session-replay.ts index 97708603d..7fb99c5de 100644 --- a/packages/session-replay-browser/src/session-replay.ts +++ b/packages/session-replay-browser/src/session-replay.ts @@ -321,7 +321,20 @@ export class SessionReplay implements AmplitudeSessionReplay { return; } this.stopRecordingEvents(); - const { privacyConfig } = config; + const { privacyConfig, interactionConfig } = config; + + const hooks = interactionConfig?.enabled + ? { + mouseInteraction: + this.eventsManager && + clickHook(this.loggerProvider, { + eventsManager: this.eventsManager, + sessionId, + deviceIdFn: this.getDeviceId.bind(this), + }), + scroll: this.scrollHook, + } + : {}; this.loggerProvider.log(`Session Replay capture beginning for ${sessionId}.`); this.recordCancelCallback = record({ @@ -339,16 +352,7 @@ export class SessionReplay implements AmplitudeSessionReplay { } }, inlineStylesheet: config.shouldInlineStylesheet, - hooks: { - mouseInteraction: - this.eventsManager && - clickHook({ - eventsManager: this.eventsManager, - sessionId, - deviceIdFn: this.getDeviceId.bind(this), - }), - scroll: this.scrollHook, - }, + hooks, maskAllInputs: true, maskTextClass: MASK_TEXT_CLASS, blockClass: BLOCK_CLASS, diff --git a/packages/session-replay-browser/test/hooks/click.test.ts b/packages/session-replay-browser/test/hooks/click.test.ts index 731c75128..cbd66f359 100644 --- a/packages/session-replay-browser/test/hooks/click.test.ts +++ b/packages/session-replay-browser/test/hooks/click.test.ts @@ -1,3 +1,6 @@ +/* eslint-disable @typescript-eslint/no-unsafe-call */ +/* eslint-disable @typescript-eslint/no-unsafe-member-access */ +/* eslint-disable @typescript-eslint/no-unsafe-return */ /* eslint-disable @typescript-eslint/no-unsafe-assignment */ import * as AnalyticsClientCommon from '@amplitude/analytics-client-common'; import { MouseInteractions } from '@amplitude/rrweb-types'; @@ -5,10 +8,32 @@ import { SessionReplayEventsManager } from '../../src/typings/session-replay'; import { UUID } from '@amplitude/analytics-core'; import { ClickEvent, ClickEventWithCount, clickBatcher, clickHook, clickNonBatcher } from '../../src/hooks/click'; import { record, utils } from '@amplitude/rrweb'; +import type { Logger } from '@amplitude/analytics-types'; +import { finder } from '../../src/libs/finder'; jest.mock('@amplitude/rrweb'); +jest.mock('../../src/libs/finder'); describe('click', () => { + const mockLoggerProvider: Logger = { + error: jest.fn(), + log: jest.fn(), + disable: jest.fn(), + enable: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + }; + + // Need to mock this in one function, but we want to use the real function everywhere else. + // If there is a better way to do this in Jest, please refactor this! + const mockFinder = finder as jest.Mock; + beforeEach(() => { + mockFinder.mockImplementation((arg) => { + const fn = jest.requireActual('../../src/libs/finder'); + return fn.finder(arg); + }); + }); + describe('clickHook', () => { const mockEventsManager: jest.Mocked> = { sendStoredEvents: jest.fn(), @@ -39,7 +64,7 @@ describe('click', () => { const deviceId = UUID(); const sessionId = Math.round(Math.random() * 1_000_000); - const hook = clickHook({ + const hook = clickHook(mockLoggerProvider, { deviceIdFn: () => deviceId, eventsManager: mockEventsManager, sessionId: sessionId, @@ -65,7 +90,7 @@ describe('click', () => { }); test('do nothing if no window given', () => { mockGlobalScope(undefined); - const hook = clickHook({ + const hook = clickHook(mockLoggerProvider, { deviceIdFn: () => deviceId, eventsManager: mockEventsManager, sessionId: sessionId, @@ -82,7 +107,7 @@ describe('click', () => { mockGlobalScope({ location: undefined, }); - const hook = clickHook({ + const hook = clickHook(mockLoggerProvider, { deviceIdFn: () => deviceId, eventsManager: mockEventsManager, sessionId: sessionId, @@ -156,6 +181,39 @@ describe('click', () => { selector: 'div', }); }); + test('no selector info on finder failure', () => { + const mockFinder = finder as jest.Mock; + mockFinder.mockImplementation(() => { + throw new Error(''); + }); + + (record.mirror.getNode as jest.Mock).mockImplementation(() => { + const ele = document.createElement('div'); + document.body.appendChild(ele); + return ele; + }) as any; + const hook = clickHook(mockLoggerProvider, { + deviceIdFn: () => deviceId, + eventsManager: mockEventsManager, + sessionId: sessionId, + }); + hook({ + id: 1234, + type: MouseInteractions.Click, + x: 3, + y: 3, + }); + expect(jest.spyOn(mockEventsManager, 'addEvent')).toHaveBeenCalledTimes(1); + expect(JSON.parse(mockEventsManager.addEvent.mock.calls[0][0].event.data)).toStrictEqual({ + x: 3, + y: 3, + viewportHeight: 768, + viewportWidth: 1024, + pageUrl: 'http://localhost/', + timestamp: expect.any(Number), + type: 'click', + }); + }); }); describe('clickBatcher', () => { diff --git a/packages/session-replay-browser/test/session-replay.test.ts b/packages/session-replay-browser/test/session-replay.test.ts index 5a0a6f06c..bf5e5e810 100644 --- a/packages/session-replay-browser/test/session-replay.test.ts +++ b/packages/session-replay-browser/test/session-replay.test.ts @@ -967,6 +967,41 @@ describe('SessionReplay', () => { recordArg?.errorHandler && recordArg?.errorHandler(error); }).toThrow(error); }); + + test('should not add hooks if interaction config is not enabled', async () => { + const sessionReplay = new SessionReplay(); + await sessionReplay.init(apiKey, mockOptions).promise; + sessionReplay.recordEvents(); + const recordArg = record.mock.calls[0][0]; + const error = new Error('test') as Error & { _external_?: boolean }; + error._external_ = true; + expect(recordArg?.hooks).toStrictEqual({}); + }); + + test('should add hooks if interaction config is enabled', async () => { + // enable interaction config + getRemoteConfigMock = jest.fn().mockImplementation((namespace: string, key: keyof SessionReplayRemoteConfig) => { + if (namespace === 'sessionReplay' && key === 'sr_interaction_config') { + return { + enabled: true, + }; + } + return; + }); + jest.spyOn(RemoteConfigFetch, 'createRemoteConfigFetch').mockResolvedValue({ + getRemoteConfig: getRemoteConfigMock, + metrics: {}, + }); + + const sessionReplay = new SessionReplay(); + await sessionReplay.init(apiKey, mockOptions).promise; + sessionReplay.recordEvents(); + const recordArg = record.mock.calls[0][0]; + const error = new Error('test') as Error & { _external_?: boolean }; + error._external_ = true; + expect(recordArg?.hooks?.mouseInteraction).toBeDefined(); + expect(recordArg?.hooks?.scroll).toBeDefined(); + }); }); describe('getDeviceId', () => {