feat(surveys): add configurable delay to popup surveys #1228

dmarticus · 2024-06-06T23:44:24Z

Problem

Many folks have been asking for us to add the ability to add a configurable delay to when our popup surveys appear on customer websites. Now, with this change (and the associated posthog change), they can do this!

Closes: PostHog/posthog#18102

Ships with the associated posthog changes: PostHog/posthog#22780

Significant Changes

I made two major changes to the surveys abstraction model.

I kinda pulled apart the SurveyPopup component to because I wanted to test it better. That component was kinda turning into a mega component that belied a lot of complexity, and it made me nervous to keep changing it without having tests.

So, I refactored that component into a hook (that did all of the interesting logic around setting state, mounting listeners, etc) and then a component that just used my new hook to keep track of the state and rendered whatever it needed to.

Then, I wrote a bunch of unit tests for that hook. See the usePopupVisibility method and the associated tests for how i did that. Required adding the preact testing library to the devdeps; I hope that's okay.

works a treat, though! And now it's easier to test!

I created a SurveyManager class that manages keeping track of the global state of which surveys we're evaluating a given time.

Demo

Survey popup with no delay set

Survey popup with 5s delay

Automated Tests

Wrote automated tests verifying the new API calls to add the surveyPopupDelay field to the appearance payload
TODO should probably add a story for this checkbox?

Manual Tests

Survey with no delay set appears immediately
Survey with a delay set to 0 appears immediately
Survey with a delay set to 5s appears after 5 seconds
Survey cannot be set with a delay less than 0
Saving the delay (or lack of delay) on the survey persists
Multiple surveys appear ascending order, where the survey with no delay appears first, followed by surveys with delays
Only one survey ever appears at a time.

vercel · 2024-06-06T23:44:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
posthog-js	✅ Ready (Inspect)	Visit Preview	Jun 23, 2024 3:03pm

posthog-bot · 2024-06-06T23:44:35Z

Hey @dmarticus! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

github-actions · 2024-06-06T23:46:25Z

Size Change: +4.52 kB (+0.45%)

Total Size: 1.02 MB

Filename	Size	Change
`dist/surveys-module-previews.js`	60.3 kB	+729 B (+1.22%)
`dist/surveys.js`	65 kB	+3.79 kB (+6.21%)	🔍

ℹ️ View Unchanged

Filename	Size
`dist/array.full.js`	241 kB
`dist/array.js`	138 kB
`dist/es.js`	138 kB
`dist/exception-autocapture.js`	12.2 kB
`dist/module.js`	139 kB
`dist/recorder-v2.js`	109 kB
`dist/recorder.js`	109 kB
`dist/tracing-headers.js`	8.26 kB

_{compressed-size-action}

dmarticus · 2024-06-07T23:23:30Z

src/extensions/surveys.tsx

 export const callSurveys = (posthog: PostHog, forceReload: boolean = false) => {
    posthog?.getActiveMatchingSurveys((surveys) => {
        const nonAPISurveys = surveys.filter((survey) => survey.type !== 'api')
        nonAPISurveys.forEach((survey) => {
+            if (visibleSurveys.has(survey.id)) {
+                return // skip if survey is already visible


FYI, I added this bit of code to prevent re-rendering the popup if this survey is already present on the page; I was noticing this weird behavior where we kept trying to render the survey afresh every few seconds. This change mitigates it, although it might have consequences that I don't fully understand. My UI tests seemed to work great, and it doesn't look like I regressed anything, but I wanted to call this out in case it was dangerous.

src/extensions/surveys.tsx

dmarticus · 2024-06-07T23:23:54Z

src/extensions/surveys.tsx

-    useEffect(() => {
-        if (isPreviewMode || !posthog) {
-            return
-        }
-
-        window.dispatchEvent(new Event('PHSurveyShown'))
-        posthog.capture('survey shown', {
-            $survey_name: survey.name,
-            $survey_id: survey.id,
-            $survey_iteration: survey.current_iteration,
-            $survey_iteration_start_date: survey.current_iteration_start_date,
-            sessionRecordingUrl: posthog.get_session_replay_url?.(),
-        })
-        localStorage.setItem(`lastSeenSurveyDate`, new Date().toISOString())
-
-        window.addEventListener('PHSurveyClosed', () => {
-            setIsPopupVisible(false)
-        })
-        window.addEventListener('PHSurveySent', () => {
-            if (!survey.appearance?.displayThankYouMessage) {
-                return setIsPopupVisible(false)
-            }
-
-            setIsSurveySent(true)
-
-            if (survey.appearance?.autoDisappear) {
-                setTimeout(() => {
-                    setIsPopupVisible(false)
-                }, 5000)
-            }
-        })
-    }, [])
-


refactored this into the useSurveyPopup hook.

neilkakkar

One potential bug I'm not sure of, otherwise seems solid!

neilkakkar · 2024-06-10T16:14:54Z

src/extensions/surveys.tsx

@@ -99,6 +104,7 @@ export const callSurveys = (posthog: PostHog, forceReload: boolean = false) => {
                }

                if (!localStorage.getItem(getSurveySeenKey(survey))) {
+                    visibleSurveys.add(survey.id)


I think we need to remove the survey.id when it's not visible as well?

I'd expect this set to always have one element. For cases when for example a survey is shown -> responded -> needs to show up again because config / iteration / whatever -> currently it wouldn't show up again if I'm reading things correctly.

src/extensions/surveys.tsx

neilkakkar · 2024-06-10T16:16:07Z

src/extensions/surveys.tsx

+            const timeoutId = setTimeout(() => {
+                setIsPopupVisible(true)
+
+                window.dispatchEvent(new Event('PHSurveyShown'))


can we abstract this into a local function call that does all of these things?

this is still todo :)

sorry, must've missed this! Just addressed it.

neilkakkar · 2024-06-10T16:33:55Z

src/extensions/surveys.tsx

+        window.addEventListener('PHSurveySent', handleSurveySent)
+
+        if (delay > 0) {
+            const timeoutId = setTimeout(() => {


hard for me to reason via the code, but what happens in the case when:

We have a popup with delay 5 seconds (surveyA) that is active & matching

We have a popup with delay 0 seconds (surveyB) that is active & matching

surveyA matches first, and we end up here, call setTimeout. isPopupVisible is false.

surveyB matches as well, and is shown directly, because no other survey is visible.

5 seconds later, surveyA is shown on top of surveyB. I think this will happen because the existing survey visible check happens at a higher level, and not inside the setTimeout here. I think the same problem magnifies when you have multiple surveys with different timeouts.

Or at least that's what I think will happen, haven't run this, try it out please :) I think this delay check needs to go at a higher level to prevent this. Another thing to figure out is how do we want to handle multiple delayed & non-delayed surveys matching - always show the non-delayed ones first, and only then start the timing for delayed surveys (I think I prefer this but haven't thought too much about it), or 'undefined behaviour', whichever ones comes first in the list is set to show first, so if there's a 30s delay, this one will wait 30 seconds, show up, and then any remaining non-delayed ones show up after.

This is a thoughtful comment, so I want to make sure I respond to it.

Basically, what I ended up doing was differentiating between the survey that was actively being focused on (I created an action queue of SurveyA, SurveyB, etc, ordered by the delay associated with that survey), and then visibility of the survey itself (which is handled by this usePopupVisibility hook.

Here's what happens now:

we load all surveys from the posthog SDK and throw them into a list, ordered by survey delay (where the non-delayed surveys all appear first, and then proceeding in sequential order with the longest delays at the end of this list)

we add the first item in this list to the surveysInFocus set, which is a Set that has strictly one member, and it keeps track of the survey about which we are evaluating visibility

we then use usePopupVisiblity to determine how to display that survey on the page.

once that survey has completed its popup lifecycle, we then remove it from the surveysInFocus set and proceed to the next one.

This way, handling visibility and handling which surveys to evaluate are separate processes, which prevent surveys from colliding with each other.

dmarticus · 2024-06-10T17:31:27Z

@neilkakkar thanks for the feedback; I didn't think about this critically enough from the multiple popups + repeated experience angle. Definitely some things I need to iron out there. Will do this today!

neilkakkar

Great job, I love this refactor & fixing! 🙌

src/__tests__/extensions/surveys.test.ts

neilkakkar · 2024-06-19T09:29:31Z

src/__tests__/extensions/surveys.test.ts

+        jest.useRealTimers()
+    })
+
+    test('should hide popup when PHSurveyClosed event is dispatched', () => {


love this test suite, great job!

neilkakkar · 2024-06-19T09:30:27Z

src/__tests__/extensions/surveys.test.ts

+                end_date: null,
+                targeting_flag_key: null,
+            },
+        ] as unknown as Survey[]


why do you need to coerce type here?

Ha, I was just being a bit lazy. FWIW, the actual values of the survey data don't matter than much in this specific test, and I noticed that some of the existing test code were also using type coercions to handle the fact that these test surveys didn't compile (since they no longer match the Survey type). I can clean all these test surveys up and make them match the correct schema; it's not much extra work and may save us in the future, but for these examples it didn't matter much since I'm not strictly testing the values within these surveys – I just needed some data I could pass in!

Anyway, I'll clean these up. It's a fair callout

Ah all good, upto you, no strong feelings from me here 🤷

Cleaned up the types a bit.

neilkakkar · 2024-06-19T09:38:20Z

src/extensions/surveys.tsx

+        // This is important for correctly displaying popover surveys with a delay, where we want to show them
+        // in order of their delay, rather than evaluate them all at once.
+        // NB: This set should only ever have 0 or 1 items in it at a time.
+        this.surveysInFocus = new Set<string>()


Since should always have 0 or 1 items, confusing to me that its a set and named plural, why not instead:

this.surveyInFocus: string | null = null

and whenever we're trying to add a 2nd survey in focus when there's already a survey in focus, console.error with enough debug info that if it happens to a user we can replicate using this state

Aligned, and I love that idea.

Done, let me know what you think of the error message (I added it to the addSurveyToFocus method)

error looks good! Although it's still a set 😅 - sorry if this wasn't clear in my earlier message, I was suggesting making it a string | null, instead of a set, since to me there doesn't seem to be a need for a set here

(which is what creates the confusion for me, since if its a set, I somewhat expect to see it handling more than one item!)

neilkakkar · 2024-06-19T09:39:23Z

src/extensions/surveys.tsx

-                                })
-                                selectorOnPage.setAttribute('PHWidgetSurveyClickListener', 'true')
-                            }
+    private canShowNextEventBasedSurvey = (): boolean => {


this is still pretty hacky, but agree with your decision to not fix it within this already big PR

neilkakkar · 2024-06-19T09:58:06Z

src/extensions/surveys.tsx

    return isPopupVisible ? (
        <SurveyContext.Provider
            value={{
                isPreviewMode,
                previewPageIndex: previewPageIndex,
-                handleCloseSurveyPopup: () => dismissedSurveyEvent(survey, posthog, isPreviewMode),
+                handleCloseSurveyPopup: () => {
+                    removeSurveyFromFocus(survey.id)


not a fan of having to pass on removeSurveyFromFocus everywhere, it seems very bug-prone, and remembering to do this in every new feature we add (for example, eventually in widgets)

Instead, I think you should hook into the PHSurveyClosed and PHSurveySent event listeners, where we also set isPopupVisible or not - this removeSurveyFromFocus and isPopupVisible should always be in sync (right?) , so makes sense to me to handle them in the same place.

oh, you're already doing that, do we still need this here?

Instead, I think you should hook into the PHSurveyClosed and PHSurveySent event listeners, where we also set isPopupVisible or not - this removeSurveyFromFocus and isPopupVisible should always be in sync (right?) , so makes sense to me to handle them in the same place.

They're sorta in sync: isPopupVisible handles whether the popup is literally visible or not, so it depends on things like the popup delay, the auto-disappear, etc. Whereas removeSurveyFromFocus is more about managing the surveys being evaluated – i.e. a survey can be in focus without being visible. That said, removing them should be in sync, a survey that is not in focus should never be visible. That, re: this point

Instead, I think you should hook into the PHSurveyClosed and PHSurveySent event listeners, where we also set isPopupVisible or not - this removeSurveyFromFocus and isPopupVisible should always be in sync (right?) , so makes sense to me to handle them in the same place.

I hear you on not wanting to thread a survey visibility manager through all of the components and instead try and instead try and tie them to the event listeners; I think that's a more reasonable abstraction.

Correct, that's exactly my point, removing them should be in sync, and for that we have the event listeners

neilkakkar · 2024-06-19T09:59:58Z

src/__tests__/extensions/surveys.test.ts

+        expect(surveyManager).toBeDefined()
+        expect(typeof surveyManager.getTestAPI().addSurveyToFocus).toBe('function')
+        expect(typeof surveyManager.getTestAPI().removeSurveyFromFocus).toBe('function')
+        expect(typeof surveyManager.callSurveysAndEvaluateDisplayLogic).toBe('function')


check initial value of surveysInFocus here?

src/__tests__/extensions/surveys.test.ts

neilkakkar · 2024-06-19T10:13:30Z

src/__tests__/extensions/surveys.test.ts

+        expect(handleWidgetSelectorMock).toHaveBeenCalledWith(mockSurvey)
+    })
+
+    test('callSurveysAndEvaluateDisplayLogic should not call surveys in focus', () => {


would love to see some tests around:

The sorting behaviour for delayed + non-delayed surveys (and wth add an api survey in the mix too :P )

SurveyinFocus handling for callSurveysAndEvaluateDisplayLogic - i.e. assume there is a surveyInFocus, (like in this test) - then handleSurveyPopover is not called. Then you manually call removeSurveyFromFocus and then call callSurveysAndEvaluateDisplayLogic again and this time handleSurveyPopover is called.

[For a follow up]. - I don't remember if this already exists, but would love to see some tests around the getSurveySeenKey and local storage handling as well, but it's not something you're currently touching, so happy to defer to you whenever / if you want to add these

I did the first 2 in these changes, and then I'll do the next bit in my refactor PR.

Co-authored-by: Neil Kakkar <[email protected]>

neilkakkar

🚀

Phanatic

Ship it!

src/posthog-surveys-types.ts

dmarticus added 4 commits June 6, 2024 15:26

this works a treat

b395861

this works without looping survey send events

b252c5b

some conversion magic

c0067bc

okay so this appears to work now

852f539

oops, don't need this

57c47cb

dmarticus marked this pull request as draft June 6, 2024 23:45

vercel bot deployed to Preview June 6, 2024 23:47 View deployment

dmarticus requested a review from a team June 7, 2024 04:12

added better testing, woohoo

bd6e9c0

vercel bot deployed to Preview June 7, 2024 23:06 View deployment

dmarticus mentioned this pull request Jun 7, 2024

feat(surveys): add configurable delay to popup surveys PostHog/posthog#22780

Merged

dmarticus commented Jun 7, 2024

View reviewed changes

dmarticus marked this pull request as ready for review June 7, 2024 23:24

neilkakkar reviewed Jun 10, 2024

View reviewed changes

dmarticus added 3 commits June 10, 2024 16:15

this WORKS

8f97d73

confirmed that this works for my use case

86ed4db

this works for all the use cases

ff23a5f

vercel bot deployed to Preview June 11, 2024 22:11 View deployment

bring the comments back

0959171

vercel bot deployed to Preview June 12, 2024 23:09 View deployment

dmarticus added 2 commits June 12, 2024 17:15

more docs, more tests

9b3be21

reference refactor in comment

12516b9

vercel bot deployed to Preview June 13, 2024 00:19 View deployment

tests aren't ready

2762293

vercel bot deployed to Preview June 13, 2024 04:58 View deployment

one more thing

b8f84ae

dmarticus added the bump minor Bump minor version when this PR gets merged label Jun 18, 2024

vercel bot deployed to Preview June 18, 2024 22:22 View deployment

dmarticus requested a review from a team June 19, 2024 01:06

neilkakkar reviewed Jun 19, 2024

View reviewed changes

dmarticus and others added 3 commits June 19, 2024 19:49

Update src/__tests__/extensions/surveys.test.ts

bb5312d

Co-authored-by: Neil Kakkar <[email protected]>

Update src/__tests__/extensions/surveys.test.ts

ba71533

Co-authored-by: Neil Kakkar <[email protected]>

Update src/__tests__/extensions/surveys.test.ts

6be5f55

Co-authored-by: Neil Kakkar <[email protected]>

vercel bot deployed to Preview June 19, 2024 23:53 View deployment

fixed the test

7f68ad6

vercel bot deployed to Preview June 20, 2024 00:09 View deployment

addressed the rest of neil's feedback

34ab992

vercel bot deployed to Preview June 20, 2024 16:28 View deployment

dmarticus requested a review from neilkakkar June 20, 2024 17:07

use a string | null instead of a set

3ecd876

vercel bot deployed to Preview June 20, 2024 17:21 View deployment

merge conflict resolution

547e439

vercel bot deployed to Preview June 23, 2024 15:03 View deployment

neilkakkar approved these changes Jun 24, 2024

View reviewed changes

Phanatic approved these changes Jun 24, 2024

View reviewed changes

src/posthog-surveys-types.ts Outdated Show resolved Hide resolved

dmarticus merged commit 2e26881 into main Jun 24, 2024
12 checks passed

dmarticus deleted the feat/delay-survey-popup branch June 24, 2024 19:47

This was referenced Jul 24, 2024

[Snyk] Upgrade posthog-js from 1.10.2 to 1.143.0 Bad3r/Logseq#752

Open

[Snyk] Upgrade posthog-js from 1.10.2 to 1.144.0 Bad3r/Logseq#755

Open

[Snyk] Upgrade posthog-js from 1.10.2 to 1.144.1 Bad3r/Logseq#756

Open

dearWatson mentioned this pull request Sep 1, 2024

Delay Survey popup PostHog/posthog#18102

Closed

feat(surveys): add configurable delay to popup surveys #1228

feat(surveys): add configurable delay to popup surveys #1228

Conversation

dmarticus commented Jun 6, 2024 • edited Loading

Problem

Significant Changes

Demo

Automated Tests

Manual Tests

vercel bot commented Jun 6, 2024 • edited Loading

posthog-bot commented Jun 6, 2024

github-actions bot commented Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilkakkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarticus Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

dmarticus commented Jun 10, 2024

neilkakkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarticus Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

neilkakkar left a comment

Choose a reason for hiding this comment

Phanatic left a comment

Choose a reason for hiding this comment

dmarticus commented Jun 6, 2024 •

edited

Loading

vercel bot commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

dmarticus Jun 18, 2024 •

edited

Loading

dmarticus Jun 20, 2024 •

edited

Loading