-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(aio): stop the wizard sample from dropping feedback answers #89603
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { getManualCaptureExample } from './codeExamples' | ||
|
|
||
| describe('getManualCaptureExample', () => { | ||
| it('does not mark the rating event incomplete for a rating that ends the survey', () => { | ||
| const thumbsEvent = getManualCaptureExample({ followUpEnabled: true }) | ||
| .split("posthog.capture('survey sent', {")[1] | ||
| .split('})')[0] | ||
|
|
||
| // The wizard branches a thumbs up straight to End, so it never sends a follow-up event. | ||
| // A literal `false` here leaves every thumbs up submission without a completed event, | ||
| // which hides it from surveys that do not enable partial responses. | ||
| expect(thumbsEvent).not.toMatch(/\$survey_completed: false\b/) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -47,7 +47,9 @@ const generateProps = (props: Prop[], indent = 2): string => { | |||||
| export function getManualCaptureExample({ surveyId = 'your-survey-id', followUpEnabled }: CodeExampleParams): string { | ||||||
| const thumbsProps: Prop[] = [ | ||||||
| { key: '$survey_id', value: `'${surveyId}'`, comment: 'ID for the survey you just created' }, | ||||||
| { key: '$survey_response', value: '1', comment: '1 = thumbs up, 2 = thumbs down' }, | ||||||
| followUpEnabled | ||||||
| ? { key: '$survey_response', value: 'rating' } | ||||||
| : { key: '$survey_response', value: '1', comment: '1 = thumbs up, 2 = thumbs down' }, | ||||||
| { key: '$ai_trace_id', value: 'traceId', comment: 'your generated trace ID' }, | ||||||
| ...(followUpEnabled | ||||||
| ? [ | ||||||
|
|
@@ -56,11 +58,7 @@ export function getManualCaptureExample({ surveyId = 'your-survey-id', followUpE | |||||
| value: 'submissionId', | ||||||
| comment: 'unique ID to link thumbs + follow-up', | ||||||
| }, | ||||||
| { | ||||||
| key: '$survey_completed', | ||||||
| value: 'true', | ||||||
| comment: 'or false if there is negative feedback followup', | ||||||
| }, | ||||||
| { key: '$survey_completed', value: '!expectsFollowUp' }, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user selects thumbs down but does not submit the optional follow-up, Prompt To Fix With AIThis is a comment left during a code review.
Path: products/ai_observability/frontend/feedback-view/wizard/codeExamples.ts
Line: 61
Comment:
**Optional follow-up drops ratings**
When a user selects thumbs down but does not submit the optional follow-up, `$survey_completed: !expectsFollowUp` leaves the only rating event incomplete, causing the rating to be filtered out of survey results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a good one too. Either the snippet sends the completed event on dismissal too, or the wizard creates the survey with enable_partial_responses: true, which makes the filter take the latest event per $survey_submission_id and makes false safe. The cumulative re-send is still needed either way.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — this is a valid gap in the follow-up-enabled snippet. If a user dismisses the follow-up after thumbs down, the only event remains I’d recommend adding a dismissal path that sends a final cumulative
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid, and fixed in 557a9e7. The second event now fires when the follow-up closes, not only when text is submitted, and carries On the Worth noting for anyone reading later: with partial responses off, exactly one completed event per submission is required. Marking both events complete would count the submission twice. So the |
||||||
| ] | ||||||
| : []), | ||||||
| ] | ||||||
|
|
@@ -70,10 +68,17 @@ export function getManualCaptureExample({ surveyId = 'your-survey-id', followUpE | |||||
| { key: '$ai_trace_id', value: 'traceId' }, | ||||||
| ] | ||||||
|
|
||||||
| const submissionIdLine = followUpEnabled | ||||||
| // The survey branches to the follow-up on a thumbs down and ends on a thumbs up, so the | ||||||
| // sample derives both the rating and the completion flag from one variable. Hard-coding | ||||||
| // `$survey_completed: false` would leave every thumbs up without a completed event. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rationale is also in the generated snippet on line 79 and again in the test. Which of the three is the durable home? This one also sits ten lines below the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The generated snippet is the durable home, since that is the artifact a customer reads. The TypeScript comment and the test comment are both gone. What is left sits next to what it explains: the branching note directly above |
||||||
| const followUpPreamble = followUpEnabled | ||||||
| ? `// Generate a unique ID to link \`survey sent\` events into a single user feedback event | ||||||
| const submissionId = crypto.randomUUID() | ||||||
|
|
||||||
| const rating = 2 // 1 = thumbs up, 2 = thumbs down | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could we make the placeholder look like one?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken, and applied to both variants, since the no-follow-up sample had the same problem with const rating = clickedThumbsUp ? 1 : 2 // 1 = thumbs up, 2 = thumbs down |
||||||
| // Only a thumbs down opens the follow-up, so a thumbs up completes the submission here | ||||||
| const expectsFollowUp = rating === 2 | ||||||
|
|
||||||
| ` | ||||||
| : '' | ||||||
|
|
||||||
|
|
@@ -82,14 +87,15 @@ posthog.capture('survey shown', { | |||||
| ${generateProps(surveyShownProps)} | ||||||
| }) | ||||||
|
|
||||||
| ${submissionIdLine}// When user clicks thumbs up/down, send a survey event | ||||||
| ${followUpPreamble}// When user clicks thumbs up/down, send a survey event | ||||||
| posthog.capture('survey sent', { | ||||||
| ${generateProps(thumbsProps)} | ||||||
| })` | ||||||
|
|
||||||
| if (followUpEnabled) { | ||||||
| const followUpProps: Prop[] = [ | ||||||
| { key: '$survey_id', value: `'${surveyId}'` }, | ||||||
| { key: '$survey_response', value: 'rating', comment: 're-send the thumbs response so it still shows' }, | ||||||
| { key: '$survey_response_1', value: "'the AI hallucinated hedgehogs everywhere'" }, | ||||||
| { key: '$ai_trace_id', value: 'traceId' }, | ||||||
| { | ||||||
|
|
||||||
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 assertion only rejects the literal
$survey_completed: falsein the first event, so it still passes if completion is removed or made unconditional, the rating condition is reversed, or the completed follow-up event stops repeating the rating.Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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 is a real issue!
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.
Fixed in 557a9e7.
The test now runs the generated sample against a stub
posthogand asserts the events a pasted integration would send, over three paths: thumbs up, thumbs down with text, thumbs down dismissed.Each weakness you listed was checked by mutating the source and re-running:
$survey_completed: falseexpectsFollowUp = rating === 1(reversed)$survey_response: 1