fix(aio): stop the wizard sample from dropping feedback answers - #89603
fix(aio): stop the wizard sample from dropping feedback answers#89603phillram wants to merge 3 commits into
Conversation
The generated manual-capture snippet split a submission's answers across two `survey sent` events: the thumbs response only on the first, the follow-up text only on the second. PostHog reads a submission's answers from the completed event, so the thumbs rating was dropped from the response and the question breakdown. The sample now derives the rating and the completion flag from one `rating` variable. The completed event re-sends the rating, and a thumbs up completes the submission on its own event, because the survey branches a thumbs up straight to End and never sends a follow-up. Re-lands #76115 with the two issues raised in review on it. Generated-By: PostHog Desktop Task-Id: a05b8ee3-88a3-42b0-bc16-07a58ca44d30
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.36 MiB · 22 files | no change | ███░░░░░░░ 30.2% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.77 MiB · 3,236 files | no change | █████████░ 90.3% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 258.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/module.js |
| 246.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.6 KiB | src/lib/api.ts |
| 95.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.25 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.25 MiB · 17 files | no change | ████░░░░░░ 39.3% of 5.72 MiB |
| Deferred (lazy) | 2.09 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 746.2 KiB | dist/toolbar/toolbar-app-JDJU4DRU.css |
| 582.8 KiB | dist/toolbar/chunk-chunk-TZGWUAXZ.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-6S5AAQW2.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-H6TI5DLW.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-4UFO5R4G.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-3XPPI6LB.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-CTG3HTDK.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +905 B (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1441.95 MiB · 🔺 +905 B (+0.0%)
Prompt To Fix All With AI### Issue 1
products/ai_observability/frontend/feedback-view/wizard/codeExamples.ts:61
**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.
### Issue 2
products/ai_observability/frontend/feedback-view/wizard/codeExamples.test.ts:12
**Test misses required behavior**
This assertion only rejects the literal `$survey_completed: false` in 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.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(aio): stop the wizard sample from dr..." | Re-trigger Greptile |
| value: 'true', | ||
| comment: 'or false if there is negative feedback followup', | ||
| }, | ||
| { key: '$survey_completed', value: '!expectsFollowUp' }, |
There was a problem hiding this 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.
Prompt To Fix With AI
This 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 $survey_completed: false, so the submission is omitted from results when partial responses are disabled.
I’d recommend adding a dismissal path that sends a final cumulative survey sent event with $survey_response: rating and $survey_completed: true. That keeps the wizard’s existing API-survey behavior unchanged. Enabling enable_partial_responses would also address the filtering, but changes survey-level semantics and should only be used if retaining abandoned submissions is intentional. The cumulative re-send is still required in either approach.
There was a problem hiding this comment.
Valid, and fixed in 557a9e7.
The second event now fires when the follow-up closes, not only when text is submitted, and carries $survey_response_1: followUpText ('' on dismissal). Every path now ends on exactly one completed event.
On the enable_partial_responses alternative: leaving it off. The wizard also renders this snippet for an existing survey the user picks from the list, and those surveys carry whatever setting they were created with. A snippet that always ends on a completed event is correct either way.
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 !expectsFollowUp shape is load-bearing, not decoration.
| // 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/) |
There was a problem hiding this comment.
This assertion only rejects the literal $survey_completed: false in 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
This is a comment left during a code review.
Path: products/ai_observability/frontend/feedback-view/wizard/codeExamples.test.ts
Line: 12
Comment:
**Test misses required behavior**
This assertion only rejects the literal `$survey_completed: false` in 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](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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.
I think this is a real issue!
There was a problem hiding this comment.
Fixed in 557a9e7.
The test now runs the generated sample against a stub posthog and 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:
| Mutation | Result |
|---|---|
rating event hard-codes $survey_completed: false |
1 failure |
| follow-up event drops the rating re-send | 2 failures |
expectsFollowUp = rating === 1 (reversed) |
3 failures |
no-follow-up variant hard-codes $survey_response: 1 |
3 failures |
| follow-up event only sent with text | 1 failure |
🦔 Hogbox preview · 🔧 building…Spinning up PostHog for this PR on a hogland hogbox — this comment updates in place when it's ready (usually a few minutes). commit |
carlos-marchal-ph
left a comment
There was a problem hiding this comment.
Approving to unblock but the issues here should be addressed :)
| // 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. |
There was a problem hiding this comment.
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 $survey_completed line it explains rather than next to it.
There was a problem hiding this comment.
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 expectsFollowUp, and the reason the closing event is mandatory directly above that event.
| ? `// 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 |
There was a problem hiding this comment.
rating is load-bearing now, expectsFollowUp and $survey_completed both derive from it. Copied verbatim, every submission reports a thumbs down and marks itself incomplete, which is the failure this PR is fixing. It also disagrees with the no-follow-up variant, which still renders $survey_response: 1.
Could we make the placeholder look like one?
| const rating = 2 // 1 = thumbs up, 2 = thumbs down | |
| const rating = clickedThumbsUp ? 1 : 2 // 1 = thumbs up, 2 = thumbs down |
There was a problem hiding this comment.
Taken, and applied to both variants, since the no-follow-up sample had the same problem with $survey_response: 1. Both now open with your line:
const rating = clickedThumbsUp ? 1 : 2 // 1 = thumbs up, 2 = thumbs down| // 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/) |
There was a problem hiding this comment.
I think this is a real issue!
| value: 'true', | ||
| comment: 'or false if there is negative feedback followup', | ||
| }, | ||
| { key: '$survey_completed', value: '!expectsFollowUp' }, |
There was a problem hiding this comment.
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.
…ssed The sample only sent its completing event when the user submitted follow-up text. A user who rated thumbs down and then closed the follow-up left one incomplete event behind, so the whole submission dropped out of Results. The follow-up event now fires when the follow-up closes, whether the user answered or dismissed it, and carries an empty answer in the dismissal case. Both snippet variants now read the rating off the click instead of hard-coding it, so a verbatim copy no longer reports the same thumb every time. The test runs the generated sample against a stub client and asserts the events a pasted integration would send, covering the thumbs up, answered follow-up, and dismissed follow-up paths. Generated-By: PostHog Desktop Task-Id: 4928fcb4-50f8-4c7e-ab85-d1f3964e8e03
Generated-By: PostHog Desktop Task-Id: 4928fcb4-50f8-4c7e-ab85-d1f3964e8e03
Problem
A user who rates an AI response thumbs down loses that rating. It shows on neither the survey response nor the question breakdown, whether they write a follow-up or dismiss it.
survey sentevent and the follow-up text on another.survey sentevent a cumulative snapshot. The snippet did not.The matching docs fix merged as PostHog/posthog.com#19172.
Re-lands #76115, which could not be updated because its branch sits on a fork.
Changes
Two alternatives were rejected. Hard-coding
$survey_completed: falseon the rating event loses more than it fixes: the wizard creates the survey asSurveyType.APIwithoutenable_partial_responses, so every thumbs-up-only submission would vanish from Results. Settingenable_partial_responses: trueon the survey also works, but the wizard renders this snippet for existing surveys the user picks too, and those carry whatever setting they were created with. Ending every path on a completed event is correct for both.Note that exactly one completed event per submission is required while partial responses are off. Marking both events complete would count the submission twice.
Generated snippet, follow-up enabled
Nothing renders differently in the wizard's own UI. The change is the text inside its code block, shown above.
How did you test this code?
codeExamples.test.tsruns the generated sample against a stub client and asserts the events a pasted integration would send.getManualCaptureExamplehad no tests before.Four cases, each catching a regression that reached review on this snippet:
$survey_completed: falsethere hides every thumbs-up submission.Each case was checked by mutating the source and confirming the test fails, including a reversed
expectsFollowUpcondition.Not run: the wizard itself. This sandbox has no dev stack, so the snippets were rendered and read through instead.
Automatic notifications
Docs update
None. PostHog/posthog.com#19172 already made the same correction to the written docs.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Opus 5 in PostHog Desktop, starting from a request to review #76115 and unblock it.
The first revision fixed the reversed thumbs value and the hard-coded incomplete flag, confirmed against posthog-js
useThumbSurveyandfeedbackSurveyWizardLogic. Review then found that a dismissed follow-up still dropped the submission, and thatconst rating = 2read as a value rather than a placeholder. Both are fixed here, along with a test rewrite that executes the sample instead of matching its text.This branch exists because #76115 is head-on-fork and no available credential could write to it.
Skills invoked:
/writing-tests,/writing-pr-descriptions.Public artifact: the original work drew on a support ticket. No ticket content, customer name, or session data appears in the diff or in this description.
Created with PostHog Desktop