Skip to content

Commit 684b0b0

Browse files
feat(telemetry): register client + deployment platform axes on PostHog (#13469)
## Problem Filtering PostHog for the three product surfaces — **desktop local**, **desktop cloud**, **web cloud** — currently requires a different hack per pipe. For this repo's cloud build, the desktop-embedded frontend and a plain browser are indistinguishable except by sniffing `Electron` in `$raw_user_agent` (~124K desktop-cloud vs ~844K web-cloud execution events/week get separated that way today). ## Change Register the two standardized platform axes as PostHog super properties at SDK init in `PostHogTelemetryProvider`: - **`client`** — which surface emitted the event: `'desktop'` when the desktop preload bridge (`window.__comfyDesktop2`) is present, else `'web'`. The bridge is injected by Electron before any page script runs, so detection is deterministic — unlike the existing utm-based `source_app` attribution, which only covers sessions that *entered* via a desktop link. - **`deployment`** — which backend runs the work: pinned to `'cloud'`. The register happens before the pre-init event queue flushes, so events captured during the posthog-js dynamic-import window carry the axes too. ## Why pinning `deployment: 'cloud'` is safe (including embedded-in-desktop) The cloud bundle also runs **embedded in Comfy Desktop** — a cloud install loads this same bundle in Electron, where `isCloud` and the host bridge are both true. Two things happen there: 1. `main.ts` runs `initHostTelemetry()` *after* `initTelemetry()`, and (when remote config `enable_telemetry` is on) it **replaces** the registry with `HostTelemetrySink` — so tracked events (`execution_start`, …) route through the desktop main process, bypassing this provider. Those are tagged the same `client`/`deployment` values main-side from the install's source category ([Comfy-Desktop#1229](Comfy-Org/Comfy-Desktop#1229)). 2. posthog-js keeps capturing independently of the registry (pageviews, web vitals, identify) — those are what these super properties cover in the embedded case, and `deployment: 'cloud'` is correct for them because the cloud bundle always talks to the cloud backend regardless of embedding; the embedding itself is what `client: 'desktop'` captures. The only way a cloud build runs against a non-cloud backend is a dev setup, where `window.__CONFIG__.posthog_project_token` is absent (injected by the cloud server) and the provider disables itself before registering anything. The locally-served frontend (desktop/localhost builds) never runs this provider: `__DISTRIBUTION__` is a compile-time define, so the `initTelemetry()` call folds away, with a runtime `IS_CLOUD_BUILD` guard as backstop. With both PRs, the three platforms become clean property filters: | Surface | Filter | |---|---| | Desktop local | `client=desktop, deployment=local` | | Desktop cloud | `client=desktop, deployment=cloud` | | Web cloud | `client=web, deployment=cloud` | ## Testing - `vitest run` on `PostHogTelemetryProvider.test.ts` — 45 passing, including new coverage: web default, bridge-present → `client=desktop`, and register-before-queue-flush ordering. The two desktop-entry tests that asserted `register` is never called were narrowed to assert no `source_app` register call. - `pnpm typecheck` + eslint/oxlint on touched files — clean. Ref [MAR-51](https://linear.app/comfyorg/issue/MAR-51/foundation-desktop-sdk-dual-send-to-posthog-alongside-mixpanel) --------- Co-authored-by: AustinMroz <austin@comfy.org>
1 parent 95b121b commit 684b0b0

2 files changed

Lines changed: 65 additions & 2 deletions

File tree

src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,46 @@ describe('PostHogTelemetryProvider', () => {
195195
})
196196
})
197197

198+
describe('platform axes (client / deployment)', () => {
199+
afterEach(() => {
200+
delete window.__comfyDesktop2
201+
})
202+
203+
it('registers client=web and deployment=cloud in a plain browser', async () => {
204+
createProvider()
205+
await vi.dynamicImportSettled()
206+
207+
expect(hoisted.mockRegister).toHaveBeenCalledWith({
208+
client: 'web',
209+
deployment: 'cloud'
210+
})
211+
})
212+
213+
it('registers client=desktop when the desktop preload bridge is present', async () => {
214+
window.__comfyDesktop2 = {
215+
isRemote: () => false,
216+
Telemetry: { capture: vi.fn() }
217+
}
218+
createProvider()
219+
await vi.dynamicImportSettled()
220+
221+
expect(hoisted.mockRegister).toHaveBeenCalledWith({
222+
client: 'desktop',
223+
deployment: 'cloud'
224+
})
225+
})
226+
227+
it('registers platform axes before flushing pre-init queued events', async () => {
228+
const provider = createProvider()
229+
provider.trackSignupOpened()
230+
await vi.dynamicImportSettled()
231+
232+
const registerOrder = hoisted.mockRegister.mock.invocationCallOrder[0]
233+
const captureOrder = hoisted.mockCapture.mock.invocationCallOrder[0]
234+
expect(registerOrder).toBeLessThan(captureOrder)
235+
})
236+
})
237+
198238
describe('desktop entry capture', () => {
199239
function setLocation(search: string): void {
200240
Object.defineProperty(window.location, 'search', {
@@ -208,20 +248,28 @@ describe('PostHogTelemetryProvider', () => {
208248
setLocation('')
209249
})
210250

251+
// The platform-axes register (client/deployment) always fires, so these
252+
// assert no register call carrying desktop-entry attribution props.
253+
function desktopEntryRegisterCalls(): unknown[][] {
254+
return hoisted.mockRegister.mock.calls.filter(
255+
([props]) => props && 'source_app' in (props as Record<string, unknown>)
256+
)
257+
}
258+
211259
it('does not register desktop props when utm_source is absent', async () => {
212260
setLocation('')
213261
createProvider()
214262
await vi.dynamicImportSettled()
215263

216-
expect(hoisted.mockRegister).not.toHaveBeenCalled()
264+
expect(desktopEntryRegisterCalls()).toHaveLength(0)
217265
})
218266

219267
it('does not register desktop props when utm_source is not comfy.desktop', async () => {
220268
setLocation('?utm_source=google&desktop_device_id=should-be-ignored')
221269
createProvider()
222270
await vi.dynamicImportSettled()
223271

224-
expect(hoisted.mockRegister).not.toHaveBeenCalled()
272+
expect(desktopEntryRegisterCalls()).toHaveLength(0)
225273
})
226274

227275
it('registers source_app and desktop_device_id when arriving from desktop', async () => {

src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ export class PostHogTelemetryProvider implements TelemetryProvider {
143143
before_send: createPostHogBeforeSend()
144144
})
145145
this.isInitialized = true
146+
// Before flushEventQueue so pre-init events also carry the
147+
// platform super properties.
148+
this.registerPlatformProps()
146149
this.flushEventQueue()
147150
this.registerDesktopEntryProps()
148151

@@ -285,6 +288,18 @@ export class PostHogTelemetryProvider implements TelemetryProvider {
285288
)
286289
}
287290

291+
private registerPlatformProps(): void {
292+
if (!this.posthog) return
293+
try {
294+
this.posthog.register({
295+
client: window.__comfyDesktop2 ? 'desktop' : 'web',
296+
deployment: 'cloud'
297+
})
298+
} catch (error) {
299+
console.error('Failed to register platform props:', error)
300+
}
301+
}
302+
288303
private registerDesktopEntryProps(): void {
289304
if (!this.posthog) return
290305
const props = readDesktopEntryProps()

0 commit comments

Comments
 (0)