Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: send survey partial responses event #1788

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore: send survey partial responses event #1788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d66679
wip
lucasheriques Mar 3, 2025
aa72a8b
merge main
lucasheriques Mar 18, 2025
056b7c6
use individual survey config instead
lucasheriques Mar 18, 2025
99e4c51
some type changes
lucasheriques Mar 18, 2025
26c9dda
some logic fixes
lucasheriques Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 37 additions & 19 deletions src/extensions/surveys.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { addEventListener } from '../utils'
import { document as _document, window as _window } from '../utils/globals'
import { createLogger } from '../utils/logger'
import { isNull, isNumber } from '../utils/type-utils'
import { uuidv7 } from '../uuidv7'
import { createWidgetShadow, createWidgetStyle } from './surveys-widget'
import { ConfirmationMessage } from './surveys/components/ConfirmationMessage'
import { Cancel } from './surveys/components/QuestionHeader'
Expand Down Expand Up @@ -723,6 +724,24 @@ export function SurveyPopup({
const shouldShowConfirmation = isSurveySent || previewPageIndex === survey.questions.length
const confirmationBoxLeftStyle = style?.left && isNumber(style?.left) ? { left: style.left - 40 } : {}

const surveyContextValue = useMemo(
() => ({
isPreviewMode,
previewPageIndex: previewPageIndex,
onPopupSurveyDismissed: () => {
dismissedSurveyEvent(survey, posthog, isPreviewMode)
onPopupSurveyDismissed()
},
isPopup: isPopup || false,
onPreviewSubmit,
onPopupSurveySent: () => {
onPopupSurveySent()
},
surveySubmissionId: uuidv7(),
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: surveyResponseId is regenerated on every render due to useMemo dependencies. Should be stored in state or ref to maintain consistency.

Suggested change
}),
surveyResponseId: useRef(uuidv7()).current,
}),

Copy link

@ioannisj ioannisj Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: surveyResponseId is regenerated on every render due to useMemo dependencies. Should be stored in state or ref to maintain consistency.

No idea if this stands but this would mean a new uuid on every render right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct, as uuidv7() is a stable function (outside of preact component lifecycle)

it would only change when SurveyPopup is unmounted then rendered again, as expected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it be that the user answers 1 question, refresh the page, generates another uuid, and answers the 2nd question?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rather, how do we keep track of the current state of responses if the user refresh the page, comes back later, etc? is that gonna be an issue? (eg, even if starting from zero, having multiple answers for the same question, with a different uuid)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

than the survey will not be shown, as after one question is answered, it's marked as if it is responded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'd have to do a more complex logic to handle a bigger user session that might involve refreshing. because, right now, if they answer one question, at this point the survey is marked as responded, so once it's gone, no longer it'll show up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like we should address then, because previously, the user had a chance to answer the full survey, even if from the very beginning (annoying but possible).
now if the user just goes to another page (that is not a match anymore and survey is closed), or refreshes the page, the user cant answer anything anymore, leading to maybe an even poorer experience than before, with lots of surveys half answered

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revisiting this discussion, so we're talking about the case:

  • survey is opened because it matches filters
  • user sends a couple of answers
  • but then he refreshes / goes to another page filters don't match (thus survey will not be shown anymore)

while it is not ideal, i'm not sure if we should optimize for this case specifically. we can though, might involve however keeping the surveyResponseId on the session data until it's fully answered.

however, it'll also require a lot of logic changes on other parts, like showing a survey that was already in progress, which is not exactly trivial.

I'd instead release this as is, get a few people to early access it, and see if this feedback will happen again. what do you think? we have plenty of customers who'd like partial responses so we can find customers to iterate on it with them

[isPreviewMode, previewPageIndex, onPopupSurveyDismissed, survey, posthog, uuidv7]
)

if (isPreviewMode) {
style = style || {}
style.left = 'unset'
Expand All @@ -731,21 +750,7 @@ export function SurveyPopup({
}

return isPopupVisible ? (
<SurveyContext.Provider
value={{
isPreviewMode,
previewPageIndex: previewPageIndex,
onPopupSurveyDismissed: () => {
dismissedSurveyEvent(survey, posthog, isPreviewMode)
onPopupSurveyDismissed()
},
isPopup: isPopup || false,
onPreviewSubmit,
onPopupSurveySent: () => {
onPopupSurveySent()
},
}}
>
<SurveyContext.Provider value={surveyContextValue}>
{!shouldShowConfirmation ? (
<Questions
survey={survey}
Expand Down Expand Up @@ -783,8 +788,14 @@ export function Questions({
survey.appearance?.backgroundColor || defaultSurveyAppearance.backgroundColor
)
const [questionsResponses, setQuestionsResponses] = useState({})
const { previewPageIndex, onPopupSurveyDismissed, isPopup, onPreviewSubmit, onPopupSurveySent } =
useContext(SurveyContext)
const {
previewPageIndex,
onPopupSurveyDismissed,
isPopup,
onPreviewSubmit,
onPopupSurveySent,
surveySubmissionId,
} = useContext(SurveyContext)
const [currentQuestionIndex, setCurrentQuestionIndex] = useState(previewPageIndex || 0)
const surveyQuestions = useMemo(() => getDisplayOrderQuestions(survey), [survey])

Expand Down Expand Up @@ -817,8 +828,15 @@ export function Questions({
setQuestionsResponses({ ...questionsResponses, [responseKey]: res })

const nextStep = getNextSurveyStep(survey, displayQuestionIndex, res)
if (nextStep === SurveyQuestionBranchingType.End) {
sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog)
const isSurveyCompleted = nextStep === SurveyQuestionBranchingType.End
sendSurveyEvent({
responses: { ...questionsResponses, [responseKey]: res },
survey,
posthog,
isSurveyCompleted: isSurveyCompleted,
surveySubmissionId,
})
if (isSurveyCompleted) {
onPopupSurveySent()
} else {
setCurrentQuestionIndex(nextStep)
Expand Down
40 changes: 34 additions & 6 deletions src/extensions/surveys/surveys-utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const window = _window as Window & typeof globalThis
const document = _document as Document
const SurveySeenPrefix = 'seenSurvey_'

const logger = createLogger('[Surveys]')
const logger = createLogger('[Surveys Utils]')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const logger = createLogger('[Surveys Utils]')
const logger = createLogger('[Surveys]')

any reason to change it? its easier to filter logs by [Surveys] rather than many other options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those logs are just not working, i was just testing it out to make sure.

the only logs who are working are from posthog-surveys

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quickfix? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that quick (really confused why it's not working) but I do wanna take a look


export const SURVEY_DEFAULT_Z_INDEX = 2147483647

Expand Down Expand Up @@ -563,19 +563,41 @@ export const createShadow = (styleSheet: string, surveyId: string, element?: Ele
return shadow
}

export const sendSurveyEvent = (
responses: Record<string, string | number | string[] | null> = {},
survey: Survey,
type SendSurveyEventArgs = {
responses: Record<string, string | number | string[] | null>
survey: Survey
posthog?: PostHog
) => {
isSurveyCompleted: boolean
surveySubmissionId: string
}

export const sendSurveyEvent = ({
responses,
survey,
posthog,
isSurveyCompleted,
surveySubmissionId,
}: SendSurveyEventArgs) => {
if (!posthog) {
logger.error('[survey sent] event not captured, PostHog instance not found.')
return
}
if (!isSurveyCompleted && !survey.enable_partial_responses) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this bit of logic here should be on the calling side

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we also check for survey complete on the callsite to fire the survey sent popup callback, so better to avoid dupe checks and keep in a single place

return
}
localStorage.setItem(getSurveySeenKey(survey), 'true')

logger.info('[PostHog Surveys] survey partial response payload', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.info('[PostHog Surveys] survey partial response payload', {
logger.info('survey partial response payload', {

createLogger already has a prefix

surveyId: survey.id,
surveyName: survey.name,
surveyCompleted: isSurveyCompleted,
surveySubmissionId,
responses,
})

posthog.capture('survey sent', {
$survey_name: survey.name,
$survey_completed: isSurveyCompleted,
$survey_id: survey.id,
$survey_iteration: survey.current_iteration,
$survey_iteration_start_date: survey.current_iteration_start_date,
Expand All @@ -585,12 +607,16 @@ export const sendSurveyEvent = (
index,
})),
sessionRecordingUrl: posthog.get_session_replay_url?.(),
$survey_submission_id: surveySubmissionId,
...responses,
$set: {
[getSurveyInteractionProperty(survey, 'responded')]: true,
},
})
window.dispatchEvent(new Event('PHSurveySent'))

if (isSurveyCompleted) {
window.dispatchEvent(new Event('PHSurveySent'))
}
}

export const dismissedSurveyEvent = (survey: Survey, posthog?: PostHog, readOnly?: boolean) => {
Expand Down Expand Up @@ -742,6 +768,7 @@ interface SurveyContextProps {
isPopup: boolean
onPreviewSubmit: (res: string | string[] | number | null) => void
onPopupSurveySent: () => void
surveySubmissionId: string
}

export const SurveyContext = createContext<SurveyContextProps>({
Expand All @@ -751,6 +778,7 @@ export const SurveyContext = createContext<SurveyContextProps>({
isPopup: true,
onPreviewSubmit: () => {},
onPopupSurveySent: () => {},
surveySubmissionId: '',
})

interface RenderProps {
Expand Down
1 change: 1 addition & 0 deletions src/posthog-surveys-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ export interface Survey {
current_iteration: number | null
current_iteration_start_date: string | null
schedule?: SurveySchedule | null
enable_partial_responses?: boolean | null
}

export interface SurveyActionType {
Expand Down