-
Notifications
You must be signed in to change notification settings - Fork 159
chore: send survey partial responses event #1788
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.
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: +10.3 kB (+0.29%) Total Size: 3.59 MB
ℹ️ View Unchanged
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR adds support for sending partial survey responses by tracking individual question responses before survey completion, with a new enable_partial_response
flag controlling this behavior.
- Added
enable_partial_response
flag toSurvey
interface to control partial response behavior - Implemented
surveyResponseId
using uuidv7() to track responses across questions consistently - Modified
sendSurveyEvent
to conditionally send events based onenable_partial_response
andsurveyCompleted
flags - Added logging for survey response payloads to aid debugging
- Deduplication logic will be handled server-side in posthog/posthog
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
src/extensions/surveys.tsx
Outdated
|
||
const nextStep = getNextSurveyStep(survey, displayQuestionIndex, res) | ||
if (nextStep === SurveyQuestionBranchingType.End) { | ||
sendSurveyEvent({ ...questionsResponses, [responseKey]: res }, survey, posthog) | ||
sendSurveyEvent({ | ||
responses: { ...questionsResponses, [responseKey]: res }, | ||
survey, | ||
posthog, | ||
surveyCompleted: nextStep === SurveyQuestionBranchingType.End, | ||
surveyResponseId, | ||
}) | ||
onPopupSurveySent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Sending partial responses here but not in the else branch creates inconsistent behavior. Should also send partial responses when moving to next question.
onPopupSurveySent() | ||
}, | ||
surveyResponseId: uuidv7(), | ||
}), |
There was a problem hiding this comment.
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.
}), | |
surveyResponseId: useRef(uuidv7()).current, | |
}), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
I think this is the key change here. I'm okay if they are shown the survey again until they respond to the whole thing. In the edge cases listed above, they will have to go through all the questions again which is okay for me. I think it's preferable to show the survey two or more times until it's fully completed rather than not showing it again just because they accidentally refreshed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discussed offline
@@ -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]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quickfix? :)
There was a problem hiding this comment.
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
localStorage.setItem(getSurveySeenKey(survey), 'true') | ||
|
||
logger.info('[PostHog Surveys] survey partial response payload', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.info('[PostHog Surveys] survey partial response payload', { | |
logger.info('survey partial response payload', { |
createLogger already has a prefix
i think some tests around the new props would be good. |
onPopupSurveySent() | ||
}, | ||
surveyResponseId: uuidv7(), | ||
}), |
There was a problem hiding this comment.
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?
if (!posthog) { | ||
logger.error('[survey sent] event not captured, PostHog instance not found.') | ||
return | ||
} | ||
if (!isSurveyCompleted && !survey.enable_partial_responses) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
closing in favor of #1888 (not ready for review yet) too many files changed since I opened this PR, since the lines changes were few, decided to create a new one instead of handling the merge conflicts |
Changes
sends events after every survey question - logic to dedupe them will be done on posthog/posthog side
related issue: PostHog/posthog#19300
todo:
Checklist