fix(integration): capture on confirmed success, and stop over-flagging person processing - #388
Open
sarahxsanders wants to merge 4 commits into
Open
Conversation
… intent The capture step told the agent to instrument "the real user action — the click or submit handler". That draws the line at handler-vs-render and never at intent-vs-outcome, so a completion event landed at the top of the submit handler and a form the server rejected still counted as a success. Inflated event counts, and the inflation lands on the metrics people steer by. Completion events now belong in the branch that runs after the awaited response confirms success. Capturing the attempt as well is still fine where the drop-off matters, under its own name — one name must never mean both. Also closes an adjacent gap on the server side. Backend SDKs process a person profile on every capture carrying a distinct id, and the runtime metadata they attach overwrites what the browser set on that person, so an event captured from a Linux host rewrites a macOS user's profile. Identified events also cost more than anonymous ones. Server-side business events now pass $process_person_profile false unless they deliberately update the person. audit-identify already stated both rules, but only as after-the-fact checks — the write-time prompts never got told, so the wizard could generate code its own audit would flag. The wording here is lifted from that audit reference so the two surfaces agree. Generated-By: PostHog Desktop Task-Id: 4fd61d6e-eb4e-498e-abfd-1d0b60d95a63
…tent Two fixes to the same skill. The ledger was never seeded. audit-events' overview claimed the wizard pre-seeded it, but only `audit all` gets that treatment — `wizard audit events` resolves through the generic skill program, which seeds nothing. With no ledger, audit_resolve_checks rejects every id as unknown, so no check in the skill could report anything. Step 1 now seeds it itself, the way audit-attribution already does, and the overview says so instead of the opposite. Adds capture-fires-on-success to audit-events and to the comprehensive audit. It flags completion-named events that fire before the awaited response resolves, or outside the success branch, and names the branch the call belongs in. This is the detection side of the capture-timing rule the integration skills now write against — it matters because fixing the write-time prompt only helps future runs, and anyone who already ran the wizard has no other way to find the over-reporting sitting in their data. Scoped to completion-named events on purpose. A blanket "not in a success branch" rule would flag every legitimate intent event and make the check noise. Generated-By: PostHog Desktop Task-Id: 4fd61d6e-eb4e-498e-abfd-1d0b60d95a63
The success-branch rule assumed there is always an awaited response to branch on. With a Server Action driven by useActionState, a form action prop, or a mutation hook, the client never gets one — the outcome arrives as state. The existing React rule forbids reacting to state in an effect, so between the two rules the agent had no legal placement left, in the framework the wizard is used with most. Capture belongs inside the action itself, on the server, right after the mutation succeeds: that is where the outcome is known, and it needs no client state. Staying in the browser is the fallback, and then firing off the returned success state is correct rather than the effect anti-pattern — called out explicitly so the two rules stop contradicting each other. Splits the React commandment back into two entries instead of one overloaded line, so the original useEffect rule reads unchanged and the timing rule can carry its own exception. The audit checks pick up the same shape. A capture in the submit path of an action-driven form has no await beside it to notice, so it would otherwise slip past a reviewer looking only for misplaced awaits. Generated-By: PostHog Desktop Task-Id: 4fd61d6e-eb4e-498e-abfd-1d0b60d95a63
…up and billing A live run against a Django SaaS app put `$process_person_profile: false` on all 13 server captures, including user_registered, subscription_started, subscription_plan_changed and subscription_canceled, with no identify() call anywhere. The project would create no person profiles at all — no cohorts, no person-property filters, no lifecycle insights, no funnel from first visit to paying. Cheaper events, and nothing left to analyse. The wording invited it. "Server-side business events that are not meant to update person properties, which is most of them" reads as a default, and "business event" covers a subscription as readily as a cron job. The rule now splits on what the event is for. Events that only record that something happened — a row written, a job run, a webhook received — take the flag. Events that decide who the person is or what they are worth do not, and are paired with the identify()/$set that records the change. Blanketing every capture is called out as its own mistake. audit-identify graded the same way in one direction only: it penalised a missing flag and never a flag on everything, and its own worked example was subscription_upgraded — an event that should reach the person. That check now reads both ways. Generated-By: PostHog Desktop Task-Id: 4fd61d6e-eb4e-498e-abfd-1d0b60d95a63
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a wizard branch:
Add Results will be posted here when complete. |
2 tasks
Member
|
@sarahxsanders looks solid to me. let's run a few CI jobs beginning of next week |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two instrumentation-quality bugs from @raquelmsmith, plus the audit checks that catch them.
Events fired on submit intent, so a form the server rejected still counted as a success. Fixed: completion events go in the branch that runs after the response confirms success.
Server Action forms had nowhere legal to capture from.
useActionStatenever hands the client a response, and the React rule forbids reacting to state in an effect. Fixed: capture inside the action on the server; client fallback fires off the returned success state.Backend SDKs overwrote person properties and billed as identified. Fixed: server events that only record that something happened pass
$process_person_profile: false.First version of that rule went too far — it flagged signup and subscription too, leaving no person profiles at all. Fixed: person-defining events are let through and paired with
identify()/$set.wizard audit eventscouldn't resolve a single check. Nothing seeded its ledger, so every id came back unknown. Fixed: step 1 seeds its own.audit-identifyonly ever penalised a missing flag, never a flag on everything. Fixed: that check reads both ways now.Also adds
capture-fires-on-successto the audits, so people can find intent-fired events in code the wizard already wrote.Testing
Four live wizard runs (Next.js, Fastify, Django ×2) via
--local-context-mill;pnpm build && pnpm testgreen and warlock clean. On the Django app the flag went from 13/13 captures to 3/12 — only the activity events keep it — andidentify()/$setwent 0 → 8.Checklist
pnpm testpassingLand PostHog/wizard#1211 first — it seeds the new check id.
Created with PostHog Desktop