fix: dont show survey on url change, only hide it

PostHog · Feb 23, 2025 · 3070995 · 3070995
1 parent 60c8e27
commit 3070995
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 77 deletions.
diff --git a/src/__tests__/extensions/surveys.test.ts b/src/__tests__/extensions/surveys.test.ts
@@ -5,8 +5,8 @@ import {
     renderFeedbackWidgetPreview,
     renderSurveysPreview,
     SurveyManager,
+    useHideSurveyOnURLChange,
     usePopupVisibility,
-    useToggleSurveyOnURLChange,
 } from '../../extensions/surveys'
 import { createShadow } from '../../extensions/surveys/surveys-utils'
 import { Survey, SurveyQuestionType, SurveyType } from '../../posthog-surveys-types'
@@ -798,7 +798,7 @@ describe('usePopupVisibility URL changes should hide surveys accordingly', () =>
     })
 })
 
-describe('useToggleSurveyOnURLChange', () => {
+describe('useHideSurveyOnURLChange', () => {
     let originalLocationHref: string
     let originalPushState: typeof window.history.pushState
     let originalReplaceState: typeof window.history.replaceState
@@ -846,7 +846,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -872,7 +872,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -900,7 +900,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -928,7 +928,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -956,7 +956,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -988,7 +988,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -1020,7 +1020,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         const { unmount } = renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -1043,67 +1043,6 @@ describe('useToggleSurveyOnURLChange', () => {
         expect(mockSetSurveyVisible).not.toHaveBeenCalled()
     })
 
-    it('should show survey when URL changes to match conditions', () => {
-        const survey = {
-            id: 'test-survey',
-            conditions: {
-                url: 'https://example.com/target-path',
-                urlMatchType: 'exact' as const,
-                events: null,
-                actions: null,
-            },
-        }
-
-        // Start with a non-matching URL
-        Object.defineProperty(window, 'location', {
-            value: { href: 'https://example.com/initial-path' },
-            writable: true,
-        })
-
-        renderHook(() =>
-            useToggleSurveyOnURLChange({
-                survey,
-                removeSurveyFromFocus: mockRemoveSurveyFromFocus,
-                setSurveyVisible: mockSetSurveyVisible,
-                isPreviewMode: false,
-            })
-        )
-
-        // Initial mount should not trigger any visibility changes
-        expect(mockSetSurveyVisible).not.toHaveBeenCalled()
-        expect(mockRemoveSurveyFromFocus).not.toHaveBeenCalled()
-
-        // Change to another non-matching URL
-        act(() => {
-            Object.defineProperty(window, 'location', {
-                value: { href: 'https://example.com/another-wrong-path' },
-                writable: true,
-            })
-            window.dispatchEvent(new Event('popstate'))
-        })
-
-        // Survey should remain hidden
-        expect(mockSetSurveyVisible).toHaveBeenLastCalledWith(false)
-        expect(mockRemoveSurveyFromFocus).toHaveBeenCalledWith('test-survey')
-
-        // Reset mocks for clarity
-        mockSetSurveyVisible.mockClear()
-        mockRemoveSurveyFromFocus.mockClear()
-
-        // Change URL to match the survey conditions
-        act(() => {
-            Object.defineProperty(window, 'location', {
-                value: { href: 'https://example.com/target-path' },
-                writable: true,
-            })
-            window.dispatchEvent(new Event('popstate'))
-        })
-
-        // Survey should become visible
-        expect(mockSetSurveyVisible).toHaveBeenCalledWith(true)
-        expect(mockRemoveSurveyFromFocus).not.toHaveBeenCalled()
-    })
-
     it('should keep survey visible when URL still matches after navigation', () => {
         const survey = {
             id: 'test-survey',
@@ -1116,7 +1055,7 @@ describe('useToggleSurveyOnURLChange', () => {
         }
 
         renderHook(() =>
-            useToggleSurveyOnURLChange({
+            useHideSurveyOnURLChange({
                 survey,
                 removeSurveyFromFocus: mockRemoveSurveyFromFocus,
                 setSurveyVisible: mockSetSurveyVisible,
@@ -1129,7 +1068,6 @@ describe('useToggleSurveyOnURLChange', () => {
         })
 
         expect(mockRemoveSurveyFromFocus).not.toHaveBeenCalled()
-        expect(mockSetSurveyVisible).toHaveBeenCalledWith(true)
     })
 })
 

diff --git a/src/extensions/surveys.tsx b/src/extensions/surveys.tsx
@@ -498,9 +498,9 @@ type UseHideSurveyOnURLChangeProps = {
  *
  * This separation of concerns means:
  * 1. Initial URL matching is done by `getActiveMatchingSurveys` before displaying the survey
- * 2. Subsequent URL changes are handled here to hide/show the survey as the user navigates
+ * 2. Subsequent URL changes are handled here to hide the survey as the user navigates
  */
-export function useToggleSurveyOnURLChange({
+export function useHideSurveyOnURLChange({
     survey,
     removeSurveyFromFocus,
     setSurveyVisible,
@@ -517,7 +517,6 @@ export function useToggleSurveyOnURLChange({
                 setSurveyVisible(false)
                 return removeSurveyFromFocus(survey.id)
             }
-            setSurveyVisible(true)
         }
 
         // Listen for browser back/forward browser history changes
@@ -637,7 +636,7 @@ export function usePopupVisibility(
         }
     }, [])
 
-    useToggleSurveyOnURLChange({
+    useHideSurveyOnURLChange({
         survey,
         removeSurveyFromFocus,
         setSurveyVisible: setIsPopupVisible,
@@ -880,7 +879,7 @@ export function FeedbackWidget({
         }
     }, [])
 
-    useToggleSurveyOnURLChange({
+    useHideSurveyOnURLChange({
         survey,
         removeSurveyFromFocus,
         setSurveyVisible: setIsFeedbackButtonVisible,