feat(inbox): stamp inbox_client=code on inbox analytics events#3059
Conversation
Mirrors posthog's frontend/src/scenes/inbox/inboxAnalytics.ts, which stamps
an inbox_client discriminator ("cloud") on every inbox event via a central
helper so the shared PostHog project can be sliced by surface.
Do the same on the desktop Code host: inject inbox_client="code" at the
track() choke point in posthogAnalyticsImpl for the inbox event family
(Inbox viewed/report opened/closed/action/scrolled + Signal source
connected). Central injection means no call site can miss it and future
inbox events are covered automatically.
Adds INBOX_ANALYTICS_EVENT_NAMES + isInboxAnalyticsEvent to @posthog/shared
and tests covering stamped/unstamped events.
andrewm4894
added
the
Stamphog
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reviews (1): Last reviewed commit: "feat(inbox): stamp inbox_client=code on ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("track", () => { | ||
| it("stamps inbox_client on inbox events", async () => { | ||
| const { initializePostHog, track } = await loadAnalytics(); | ||
| initializePostHog(); | ||
|
|
||
| track(ANALYTICS_EVENTS.SIGNAL_SOURCE_CONNECTED, { | ||
| source_product: "github", | ||
| is_first_connection: true, | ||
| via_setup_wizard: false, | ||
| }); | ||
|
|
||
| expect(mockPosthog.capture).toHaveBeenCalledWith( | ||
| ANALYTICS_EVENTS.SIGNAL_SOURCE_CONNECTED, | ||
| expect.objectContaining({ inbox_client: "code" }), | ||
| ); | ||
| }); | ||
|
|
||
| it("does not stamp inbox_client on non-inbox events", async () => { | ||
| const { initializePostHog, track } = await loadAnalytics(); | ||
| initializePostHog(); | ||
|
|
||
| track(ANALYTICS_EVENTS.PROMPT_HISTORY_OPENED, { entry_count: 3 }); | ||
|
|
||
| expect(mockPosthog.capture).toHaveBeenCalledWith( | ||
| ANALYTICS_EVENTS.PROMPT_HISTORY_OPENED, | ||
| expect.not.objectContaining({ inbox_client: expect.anything() }), | ||
| ); | ||
| }); | ||
|
|
||
| it("does nothing before init", async () => { | ||
| const { track } = await loadAnalytics(); | ||
|
|
||
| track(ANALYTICS_EVENTS.SIGNAL_SOURCE_CONNECTED, { | ||
| source_product: "github", | ||
| is_first_connection: true, | ||
| via_setup_wizard: false, | ||
| }); | ||
|
|
||
| expect(mockPosthog.capture).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
it blocks
The suite tests only SIGNAL_SOURCE_CONNECTED to verify inbox stamping, but INBOX_ANALYTICS_EVENT_NAMES contains five other events (INBOX_VIEWED, INBOX_REPORT_OPENED, INBOX_REPORT_CLOSED, INBOX_REPORT_ACTION, INBOX_REPORT_SCROLLED). A new inbox event could be added to the set without any test coverage confirming it gets the inbox_client stamp. Using it.each (or test.each) over the full INBOX_ANALYTICS_EVENT_NAMES set — or at least the full list of event constants — would give exhaustive coverage and remove the implicit assumption that one event represents all others.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
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!
| */ | ||
| export const INBOX_ANALYTICS_EVENT_NAMES: ReadonlySet<string> = new Set([ | ||
| ANALYTICS_EVENTS.INBOX_VIEWED, | ||
| ANALYTICS_EVENTS.INBOX_REPORT_OPENED, | ||
| ANALYTICS_EVENTS.INBOX_REPORT_CLOSED, | ||
| ANALYTICS_EVENTS.INBOX_REPORT_ACTION, | ||
| ANALYTICS_EVENTS.INBOX_REPORT_SCROLLED, | ||
| ANALYTICS_EVENTS.SIGNAL_SOURCE_CONNECTED, | ||
| ]); | ||
|
|
||
| /** True when `eventName` is an inbox event that should carry `inbox_client`. */ | ||
| export function isInboxAnalyticsEvent(eventName: string): boolean { | ||
| return INBOX_ANALYTICS_EVENT_NAMES.has(eventName); | ||
| } |
There was a problem hiding this comment.
INBOX_ANALYTICS_EVENT_NAMES and EventPropertyMap can drift
The JSDoc says "Keep this in sync with the inbox entries in EventPropertyMap above," but there's no compile-time enforcement of that. If a new inbox event is added to EventPropertyMap but missed here, it will silently go out without inbox_client. A satisfies or mapped-type guard could make the compiler catch the gap. This is a maintenance footgun worth considering as the inbox event family grows.
#3059) Ports desktop PR #3059 to the mobile app: stamp a client discriminator `inbox_client="mobile"` on the inbox-family analytics events so the shared PostHog project can be sliced by surface. The mobile analytics wrapper (`useAnalytics().track`) now spreads `{ inbox_client: "mobile", ...properties }` for events in the mobile inbox set (Inbox viewed / report opened / closed / scrolled / action), with the client value first so an explicit override wins. Non-inbox events are unchanged. Mobile keeps its own event set rather than importing the shared helper, mirroring how it already owns its `ANALYTICS_EVENTS` map. Generated-By: PostHog Code Task-Id: 03d5200d-e698-4abe-8980-4bfe2290a9c7
1 participant
Summary
We want every custom inbox analytics event from the desktop Code app to carry an
inbox_client=codeproperty, so the shared PostHog project (which also receives inbox events from the web frontend and mobile) can be sliced by surface.This mirrors how
posthogdoes it infrontend/src/scenes/inbox/inboxAnalytics.ts, which stamps aninbox_clientdiscriminator ('cloud'there) on every inbox event via a central helper rather than per call site.What changed
@posthog/shared— addedINBOX_ANALYTICS_EVENT_NAMES(the inbox event family: Inbox viewed / report opened / closed / action / scrolled + Signal source connected) and anisInboxAnalyticsEvent()helper, kept next toEventPropertyMap.posthogAnalyticsImpl.ts— addedconst INBOX_CLIENT = "code"and stamp it at the singletrack()→posthog.capture()choke point for inbox events. Central injection means no call site can miss it and future inbox events are covered automatically. Spread order matches posthog's helper.inbox_client: "code", non-inbox events don't, and nothing fires before init.Notes / follow-ups
codevsdesktop: posthog'sinboxAnalytics.tsdoc comment refers to the Code app's discriminator as'desktop'. This PR usescodeper request. Each client sets its own string, so this only matters for how breakdowns read — happy to switch todesktop(and/or update the posthog comment) if we want the two repos to agree.apps/mobilesends the same inbox event names through its own tracker and currently emits noinbox_client. Out of scope here; it would sendinbox_client="mobile"via the same pattern if we want to slice it out too.Test plan
pnpm --filter @posthog/shared typecheck+buildpnpm --filter @posthog/ui test(1113 passing, incl. new cases)pnpm typecheckpassed