more review changes, and tests

src/__tests__/posthog-surveys.test.ts

@@ -0,0 +1,261 @@
+/* eslint-disable compat/compat */
+jest.mock('../utils/logger', () => ({
+    createLogger: jest.fn().mockReturnValue({
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+    }),
+}))
+jest.useFakeTimers()
+
+import { SURVEYS } from '../constants'
+import { PostHog } from '../posthog-core'
+import { PostHogSurveys } from '../posthog-surveys'
+import { assignableWindow } from '../utils/globals'
+
+describe('PostHogSurveys', () => {
+    let mockPostHog: PostHog & {
+        get_property: jest.Mock
+        _send_request: jest.Mock
+    }
+    let surveys: PostHogSurveys
+    let mockGenerateSurveys: jest.Mock
+    let mockLoadExternalDependency: jest.Mock
+
+    beforeEach(() => {
+        // Reset mocks
+        jest.clearAllMocks()
+
+        // Mock PostHog instance
+        mockPostHog = {
+            config: {
+                disable_surveys: false,
+                token: 'test-token',
+            },
+            persistence: {
+                register: jest.fn(),
+                props: {},
+            },
+            requestRouter: {
+                endpointFor: jest.fn().mockReturnValue('https://test.com/api/surveys'),
+            },
+            _send_request: jest.fn(),
+            get_property: jest.fn(),
+        } as unknown as PostHog & {
+            get_property: jest.Mock
+            _send_request: jest.Mock
+        }
+
+        // Create surveys instance
+        surveys = new PostHogSurveys(mockPostHog as PostHog)
+
+        // Mock window.__PosthogExtensions__
+        mockGenerateSurveys = jest.fn()
+        mockLoadExternalDependency = jest.fn()
+        assignableWindow.__PosthogExtensions__ = {
+            generateSurveys: mockGenerateSurveys,
+            loadExternalDependency: mockLoadExternalDependency,
+        }
+    })
+
+    afterEach(() => {
+        // Clean up
+        delete assignableWindow.__PosthogExtensions__
+    })
+
+    describe('loadIfEnabled', () => {
+        it('should not initialize if surveys are already loaded', () => {
+            // Set surveyManager to simulate already loaded state
+            surveys['_surveyManager'] = {}
+            surveys.loadIfEnabled()
+
+            expect(mockGenerateSurveys).not.toHaveBeenCalled()
+            expect(mockLoadExternalDependency).not.toHaveBeenCalled()
+        })
+
+        it('should not initialize if already initializing', () => {
+            // Set isInitializingSurveys to true
+            surveys['_isInitializingSurveys'] = true
+            surveys.loadIfEnabled()
+
+            expect(mockGenerateSurveys).not.toHaveBeenCalled()
+            expect(mockLoadExternalDependency).not.toHaveBeenCalled()
+        })
+
+        it('should not initialize if surveys are disabled', () => {
+            mockPostHog.config.disable_surveys = true
+            surveys.loadIfEnabled()
+
+            expect(mockGenerateSurveys).not.toHaveBeenCalled()
+            expect(mockLoadExternalDependency).not.toHaveBeenCalled()
+        })
+
+        it('should not initialize if PostHog Extensions are not found', () => {
+            delete assignableWindow.__PosthogExtensions__
+            surveys.loadIfEnabled()
+
+            expect(mockGenerateSurveys).not.toHaveBeenCalled()
+            expect(mockLoadExternalDependency).not.toHaveBeenCalled()
+        })
+
+        it('should not initialize if decide server response is not ready', () => {
+            surveys.loadIfEnabled()
+
+            expect(mockGenerateSurveys).not.toHaveBeenCalled()
+            expect(mockLoadExternalDependency).not.toHaveBeenCalled()
+        })
+
+        it('should set isInitializingSurveys to false after successful initialization', () => {
+            // Set decide server response
+            surveys['_decideServerResponse'] = true
+            mockGenerateSurveys.mockReturnValue({})
+
+            surveys.loadIfEnabled()
+
+            expect(surveys['_isInitializingSurveys']).toBe(false)
+        })
+
+        it('should set isInitializingSurveys to false after failed initialization', () => {
+            // Set decide server response
+            surveys['_decideServerResponse'] = true
+            mockGenerateSurveys.mockImplementation(() => {
+                throw new Error('Test error')
+            })
+
+            expect(() => surveys.loadIfEnabled()).toThrow('Test error')
+            expect(surveys['_isInitializingSurveys']).toBe(false)
+        })
+
+        it('should set isInitializingSurveys to false when loadExternalDependency fails', () => {
+            // Set decide server response but no generateSurveys
+            surveys['_decideServerResponse'] = true
+            mockGenerateSurveys = undefined
+            assignableWindow.__PosthogExtensions__.generateSurveys = undefined
+
+            mockLoadExternalDependency.mockImplementation((_instance, _type, callback) => {
+                callback(new Error('Failed to load'))
+            })
+
+            surveys.loadIfEnabled()
+
+            expect(surveys['_isInitializingSurveys']).toBe(false)
+        })
+    })
+
+    describe('getSurveys', () => {
+        const mockCallback = jest.fn()
+        const mockSurveys = [{ id: 'test-survey' }]
+
+        beforeEach(() => {
+            mockCallback.mockClear()
+        })
+
+        it('should return cached surveys and not fetch if they exist', () => {
+            mockPostHog.get_property.mockReturnValue(mockSurveys)
+
+            surveys.getSurveys(mockCallback)
+
+            expect(mockPostHog._send_request).not.toHaveBeenCalled()
+            expect(mockCallback).toHaveBeenCalledWith(mockSurveys)
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+        })
+
+        it('should not make concurrent API calls', () => {
+            surveys['_isFetchingSurveys'] = true
+
+            surveys.getSurveys(mockCallback)
+
+            expect(mockPostHog._send_request).not.toHaveBeenCalled()
+            expect(mockCallback).toHaveBeenCalledWith([])
+        })
+
+        it('should reset _isFetchingSurveys after successful API call', () => {
+            mockPostHog._send_request.mockImplementation(({ callback }) => {
+                callback({ statusCode: 200, json: { surveys: mockSurveys } })
+            })
+
+            surveys.getSurveys(mockCallback)
+
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+            expect(mockCallback).toHaveBeenCalledWith(mockSurveys)
+            expect(mockPostHog.persistence?.register).toHaveBeenCalledWith({ [SURVEYS]: mockSurveys })
+        })
+
+        it('should reset _isFetchingSurveys after failed API call (non-200 status)', () => {
+            mockPostHog._send_request.mockImplementation(({ callback }) => {
+                callback({ statusCode: 500 })
+            })
+
+            surveys.getSurveys(mockCallback)
+
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+            expect(mockCallback).toHaveBeenCalledWith([])
+        })
+
+        it('should reset _isFetchingSurveys when API call throws error', () => {
+            mockPostHog._send_request.mockImplementation(() => {
+                throw new Error('Network error')
+            })
+
+            expect(() => surveys.getSurveys(mockCallback)).toThrow('Network error')
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+        })
+
+        it('should reset _isFetchingSurveys when request times out', () => {
+            // Mock a request that will timeout
+            mockPostHog._send_request.mockImplementation(({ callback }) => {
+                // Simulate a timeout by calling callback with status 0
+                callback({ statusCode: 0, text: 'timeout' })
+            })
+
+            surveys.getSurveys(mockCallback)
+
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+            expect(mockCallback).toHaveBeenCalledWith([])
+        })
+
+        it('should handle delayed successful responses correctly', () => {
+            const delayedSurveys = [{ id: 'delayed-survey' }]
+
+            // Mock a request that takes some time to respond
+            mockPostHog._send_request.mockImplementation(({ callback }) => {
+                setTimeout(() => {
+                    callback({
+                        statusCode: 200,
+                        json: { surveys: delayedSurveys },
+                    })
+                }, 100)
+            })
+
+            surveys.getSurveys(mockCallback)
+
+            // Initially the flag should be true
+            expect(surveys['_isFetchingSurveys']).toBe(true)
+
+            // After the response comes in
+            jest.advanceTimersByTime(100)
+
+            expect(surveys['_isFetchingSurveys']).toBe(false)
+            expect(mockCallback).toHaveBeenCalledWith(delayedSurveys)
+            expect(mockPostHog.persistence?.register).toHaveBeenCalledWith({ [SURVEYS]: delayedSurveys })
+        })
+
+        it('should set correct timeout value in request', () => {
+            surveys.getSurveys(mockCallback)
+
+            expect(mockPostHog._send_request).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    timeout: 10000,
+                })
+            )
+        })
+
+        it('should force reload surveys when forceReload is true', () => {
+            mockPostHog.get_property.mockReturnValue(mockSurveys)
+
+            surveys.getSurveys(mockCallback, true)
+
+            expect(mockPostHog._send_request).toHaveBeenCalled()
+        })
+    })
+})

src/posthog-surveys.ts

@@ -221,58 +221,49 @@ export class PostHogSurveys {
             return
         }
 
+        if (!this._decideServerResponse) {
+            logger.warn('Decide not loaded yet. Not loading surveys.')
+            return
+        }
+
+        if (this._surveyEventReceiver == null) {
+            this._surveyEventReceiver = new SurveyEventReceiver(this.instance)
+        }
+
         this._isInitializingSurveys = true
 
         try {
             const generateSurveys = phExtensions.generateSurveys
 
-            if (!this._decideServerResponse) {
-                logger.warn('Decide not loaded yet. Not loading surveys.')
-                this._isInitializingSurveys = false
-                return
-            }
-
-            if (this._surveyEventReceiver == null) {
-                this._surveyEventReceiver = new SurveyEventReceiver(this.instance)
-            }
-
             if (!generateSurveys) {
                 const loadExternalDependency = phExtensions.loadExternalDependency
 
                 if (loadExternalDependency) {
                     loadExternalDependency(this.instance, 'surveys', (err) => {
                         if (err) {
                             logger.error('Could not load surveys script', err)
-                            this._isInitializingSurveys = false
                             return
                         }
 
-                        try {
-                            if (!this._surveyManager) {
-                                // Double-check we still don't have a manager
-                                this._surveyManager = phExtensions.generateSurveys?.(this.instance)
-                            }
-                        } finally {
-                            this._isInitializingSurveys = false
+                        if (!this._surveyManager) {
+                            // Double-check we still don't have a manager
+                            this._surveyManager = phExtensions.generateSurveys?.(this.instance)
                         }
                     })
                 } else {
                     logger.error('PostHog loadExternalDependency extension not found. Cannot load remote config.')
-                    this._isInitializingSurveys = false
                 }
             } else {
-                try {
-                    if (!this._surveyManager) {
-                        // Double-check we still don't have a manager
-                        this._surveyManager = generateSurveys(this.instance)
-                    }
-                } finally {
-                    this._isInitializingSurveys = false
+                if (!this._surveyManager) {
+                    // Double-check we still don't have a manager
+                    this._surveyManager = generateSurveys(this.instance)
                 }
             }
         } catch (e) {
-            this._isInitializingSurveys = false
+            logger.error('Error initializing surveys', e)
             throw e
+        } finally {
+            this._isInitializingSurveys = false
         }
     }
 
@@ -304,6 +295,7 @@ export class PostHogSurveys {
                         `/api/surveys/?token=${this.instance.config.token}`
                     ),
                     method: 'GET',
+                    timeout: 10000, // 10 second timeout
                     callback: (response) => {
                         this._isFetchingSurveys = false
                         const statusCode = response.statusCode