fix: manage capture pageview hook lifecycle (#1408)

* fix: manage capture pageview hook lifecycle

* but not with silly un-failable test assertion

* tidy up page capture hook on stop

* fix

cypress/e2e/session-recording.cy.ts

@@ -397,10 +397,18 @@ describe('Session recording', () => {
                     // a meta and then a full snapshot
                     expect(captures[1]['properties']['$snapshot_data'][0].type).to.equal(4) // meta
                     expect(captures[1]['properties']['$snapshot_data'][1].type).to.equal(2) // full_snapshot
-                    expect(captures[1]['properties']['$snapshot_data'][2].type).to.equal(5) // custom event with options
-                    expect(captures[1]['properties']['$snapshot_data'][3].type).to.equal(5) // custom event with posthog config
+
+                    expect(captures[1]['properties']['$snapshot_data'][2].type).to.equal(5)
+                    expect(captures[1]['properties']['$snapshot_data'][2].data.tag).to.equal('$pageview')
+
+                    expect(captures[1]['properties']['$snapshot_data'][3].type).to.equal(5) // custom event with options
+                    expect(captures[1]['properties']['$snapshot_data'][3].data.tag).to.equal('$session_options')
+
+                    expect(captures[1]['properties']['$snapshot_data'][4].type).to.equal(5) // custom event with posthog config
+                    expect(captures[1]['properties']['$snapshot_data'][4].data.tag).to.equal('$posthog_config') // custom event with posthog config
+
                     const xPositions = []
-                    for (let i = 4; i < captures[1]['properties']['$snapshot_data'].length; i++) {
+                    for (let i = 5; i < captures[1]['properties']['$snapshot_data'].length; i++) {
                         expect(captures[1]['properties']['$snapshot_data'][i].type).to.equal(3)
                         expect(captures[1]['properties']['$snapshot_data'][i].data.source).to.equal(
                             6,

src/__tests__/extensions/replay/sessionrecording.test.ts

@@ -129,6 +129,8 @@ describe('SessionRecording', () => {
     let sessionIdGeneratorMock: Mock
     let windowIdGeneratorMock: Mock
     let onFeatureFlagsCallback: ((flags: string[], variants: Record<string, string | boolean>) => void) | null
+    let removeCaptureHookMock: Mock
+    let addCaptureHookMock: Mock
 
     const addRRwebToWindow = () => {
         assignableWindow.rrweb = {
@@ -173,6 +175,10 @@ describe('SessionRecording', () => {
 
         sessionManager = new SessionIdManager(config, postHogPersistence, sessionIdGeneratorMock, windowIdGeneratorMock)
 
+        // add capture hook returns an unsubscribe function
+        removeCaptureHookMock = jest.fn()
+        addCaptureHookMock = jest.fn().mockImplementation(() => removeCaptureHookMock)
+
         posthog = {
             get_property: (property_key: string): Property | undefined => {
                 return postHogPersistence?.['props'][property_key]
@@ -185,7 +191,7 @@ describe('SessionRecording', () => {
             },
             sessionManager: sessionManager,
             requestRouter: new RequestRouter({ config } as any),
-            _addCaptureHook: jest.fn(),
+            _addCaptureHook: addCaptureHookMock,
             consent: { isOptedOut: () => false },
         } as unknown as PostHog
 
@@ -308,6 +314,44 @@ describe('SessionRecording', () => {
             expect((sessionRecording as any)._startCapture).toHaveBeenCalled()
         })
 
+        it('sets the pageview capture hook once', () => {
+            expect(sessionRecording['_removePageViewCaptureHook']).toBeUndefined()
+
+            sessionRecording.startIfEnabledOrStop()
+
+            expect(sessionRecording['_removePageViewCaptureHook']).not.toBeUndefined()
+            expect(posthog._addCaptureHook).toHaveBeenCalledTimes(1)
+
+            // calling a second time doesn't add another capture hook
+            sessionRecording.startIfEnabledOrStop()
+            expect(posthog._addCaptureHook).toHaveBeenCalledTimes(1)
+        })
+
+        it('removes the pageview capture hook on stop', () => {
+            sessionRecording.startIfEnabledOrStop()
+            expect(sessionRecording['_removePageViewCaptureHook']).not.toBeUndefined()
+
+            expect(removeCaptureHookMock).not.toHaveBeenCalled()
+            sessionRecording.stopRecording()
+
+            expect(removeCaptureHookMock).toHaveBeenCalledTimes(1)
+            expect(sessionRecording['_removePageViewCaptureHook']).toBeUndefined()
+        })
+
+        it('sets the window event listeners', () => {
+            //mock window add event listener to check if it is called
+            const addEventListener = jest.fn().mockImplementation(() => () => {})
+            window.addEventListener = addEventListener
+
+            sessionRecording.startIfEnabledOrStop()
+            expect(sessionRecording['_onBeforeUnload']).not.toBeNull()
+            // we register 4 event listeners
+            expect(window.addEventListener).toHaveBeenCalledTimes(4)
+
+            // window.addEventListener('blah', someFixedListenerInstance) is safe to call multiple times,
+            // so we don't need to test if the addEvenListener registrations are called multiple times
+        })
+
         it('emits an options event', () => {
             sessionRecording.startIfEnabledOrStop()
             expect((sessionRecording as any)['_tryAddCustomEvent']).toHaveBeenCalledWith('$session_options', {

src/extensions/replay/sessionrecording.ts

@@ -116,6 +116,7 @@ export class SessionRecording {
 
     private _fullSnapshotTimer?: ReturnType<typeof setInterval>
 
+    private _removePageViewCaptureHook: (() => void) | undefined = undefined
     // if pageview capture is disabled
     // then we can manually track href changes
     private _lastHref?: string
@@ -284,16 +285,35 @@ export class SessionRecording {
         if (this.isRecordingEnabled) {
             this._startCapture()
 
+            // calling addEventListener multiple times is safe and will not add duplicates
             window?.addEventListener('beforeunload', this._onBeforeUnload)
             window?.addEventListener('offline', this._onOffline)
             window?.addEventListener('online', this._onOnline)
             window?.addEventListener('visibilitychange', this._onVisibilityChange)
 
+            if (isNullish(this._removePageViewCaptureHook)) {
+                // :TRICKY: rrweb does not capture navigation within SPA-s, so hook into our $pageview events to get access to all events.
+                //   Dropping the initial event is fine (it's always captured by rrweb).
+                this._removePageViewCaptureHook = this.instance._addCaptureHook((eventName) => {
+                    // If anything could go wrong here it has the potential to block the main loop,
+                    // so we catch all errors.
+                    try {
+                        if (eventName === '$pageview') {
+                            const href = window ? this._maskUrl(window.location.href) : ''
+                            if (!href) {
+                                return
+                            }
+                            this._tryAddCustomEvent('$pageview', { href })
+                        }
+                    } catch (e) {
+                        logger.error('Could not add $pageview to rrweb session', e)
+                    }
+                })
+            }
+
             logger.info(LOGGER_PREFIX + ' started')
         } else {
             this.stopRecording()
-            this.clearBuffer()
-            clearInterval(this._fullSnapshotTimer)
         }
     }
 
@@ -308,6 +328,12 @@ export class SessionRecording {
             window?.removeEventListener('online', this._onOnline)
             window?.removeEventListener('visibilitychange', this._onVisibilityChange)
 
+            this.clearBuffer()
+            clearInterval(this._fullSnapshotTimer)
+
+            this._removePageViewCaptureHook?.()
+            this._removePageViewCaptureHook = undefined
+
             logger.info(LOGGER_PREFIX + ' stopped')
         }
     }
@@ -661,24 +687,6 @@ export class SessionRecording {
             ...sessionRecordingOptions,
         })
 
-        // :TRICKY: rrweb does not capture navigation within SPA-s, so hook into our $pageview events to get access to all events.
-        //   Dropping the initial event is fine (it's always captured by rrweb).
-        this.instance._addCaptureHook((eventName) => {
-            // If anything could go wrong here it has the potential to block the main loop,
-            // so we catch all errors.
-            try {
-                if (eventName === '$pageview') {
-                    const href = window ? this._maskUrl(window.location.href) : ''
-                    if (!href) {
-                        return
-                    }
-                    this._tryAddCustomEvent('$pageview', { href })
-                }
-            } catch (e) {
-                logger.error('Could not add $pageview to rrweb session', e)
-            }
-        })
-
         // We reset the last activity timestamp, resetting the idle timer
         this._lastActivityTimestamp = Date.now()
         this.isIdle = false

src/posthog-core.ts

@@ -864,8 +864,8 @@ export class PostHog {
         return data
     }
 
-    _addCaptureHook(callback: (eventName: string, eventPayload?: CaptureResult) => void): void {
-        this.on('eventCaptured', (data) => callback(data.event, data))
+    _addCaptureHook(callback: (eventName: string, eventPayload?: CaptureResult) => void): () => void {
+        return this.on('eventCaptured', (data) => callback(data.event, data))
     }
 
     _calculate_event_properties(event_name: string, event_properties: Properties, timestamp?: Date): Properties {