Add Desktop telemetry event sink#12802
Open
benceruleanlu wants to merge 3 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds desktop telemetry: bridge types, a DesktopTelemetryProvider with payload sanitization and many track* methods plus tests, a desktop-only initializer wired into startup, narrows the Desktop2 model-download bridge typing, and removes cloud-only telemetry guards so events emit for all distributions. ChangesDesktop Telemetry Infrastructure
Sequence Diagram(s)sequenceDiagram
participant AppCode as Application Code
participant Provider as DesktopTelemetryProvider
participant Sanitizer as Sanitize Properties
participant DesktopHost as window.__comfyDesktop2.Telemetry
AppCode->>Provider: trackRunButton() / trackWorkflowImported() / etc.
Provider->>Sanitizer: filter properties to primitives/arrays
Sanitizer-->>Provider: sanitized properties
Provider->>DesktopHost: capture(eventName, sanitizedProperties)
DesktopHost-->>Provider: (optional response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12802 +/- ##
==========================================
- Coverage 76.30% 76.14% -0.17%
==========================================
Files 1571 1573 +2
Lines 101779 101901 +122
Branches 31295 30698 -597
==========================================
- Hits 77663 77593 -70
- Misses 23306 23500 +194
+ Partials 810 808 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Collaborator
|
I agree with the goal of routing Desktop-hosted frontend telemetry through the Desktop telemetry pipeline, but I’m concerned about extending The recent missing-model download work treated this bridge as a pragmatic escape hatch to get off DOM/aria scraping. In particular, Comfy-Desktop#1044 describes the Desktop injection path as a stopgap/backwards-compatibility safety net, and ComfyUI_frontend#12710 lists defining/documenting the FE/Desktop2 bridge contract as follow-up work. This PR adds telemetry to the same surface. Telemetry is more cross-cutting than missing-model download capability, and once FE starts calling I think we should clarify this before merging:
I’m not opposed to a preload-exposed Desktop capability API in principle. My concern is that this PR turns a temporary bridge into a de facto official API without making the ownership, compatibility, and privacy/safety contract explicit. |
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
I don't think desktop is a provider of tele.etry really. Can we just have posthog telemetry provider and have desktop opt in? Then we can remove the duplication. The global window bridge shouldn't exist either
Member
Author
|
@jaeone94 There is a misunderstanding here. The "stopgap" described in Comfy-Org/Comfy-Desktop#1044 is
On your specific questions:
Yes. Desktop 2 deliberately ships the stock released frontend — no fork, no Capability detection/versioning: key presence is the capability flag. FE calls Canonical typing: this PR already moves the bridge type out of the ad-hoc ambient declaration in Clone-safety: the FE provider filters properties to primitives and primitive arrays before invoking ( On the bridge contract, yes this wasn't documented, I added it to the PR now. |
benceruleanlu
assigned
jaeone94 and unassigned
christian-byrne,
benceruleanlu and
jaeone94
Member
Author
|
There is an interesting issue, however. These two statements cannot be true at the same time:
What we will have to concede here is that the gate on this desktop telemetry provider will hinge on window.__comfyDesktop2?.Telemetry, but that is a telemetry reference in local-git. |
Member
Author
|
@christian-byrne This is the MAR-240 direction (Jacob's EventSink proposal, confirmed with Deep): the frontend keeps the event layer, Desktop registers as the sink and relays to PostHog. Desktop is the relay because it already owns the PostHog client, the person/device identity, and the consent toggle — and the frontend it hosts is the stock build, which ships no telemetry SDK. "Have desktop opt in" is how this works: Desktop opts in by exposing The bridge is the only channel Electron offers a hosted page short of forking the frontend — same pattern v1 used with Agreed the provider surface can be changed to a thin generic-capture refactor as a follow-up, if we want this. |
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
3 times, most recently
from
22d04a1 to
1c9d778
Compare
Member
Author
|
This was @christian-byrne's verbatim review posted in Slack:
|
Member
Author
While I generally agree with this design preference, all Mixpanel, PostHog, and GTM telemetry providers already import useAppMode, I wouldn't say this PR is inventing it, and thus I don't think we should block marketing and growth on this.
The capture method is already typed in the type package, maybe more information is needed here.
This was indeed a valid type improvement, I implemented it.
It seems like branching on window.__comfyDesktop2?.Telemetry is a capability check. Adding another flag for the same thing would be redundant, since this only determines whether the Desktop transport should be registered. Desktop 2 still owns consent enforcement and decides whether telemetry actually leaves the user’s device.
This was for safety because__DISTRIBUTION__ === 'cloud' and window.__comfyDesktop2 are not actually mutually exclusive signals, but I implemented the cleanup nit by removing the cloud check.
Conceptually, yes it's not the classic case of a provider, but from the registry's perspective, it does behave that way, and that's why it's written as such. Would a rename help? Behaviorally, this is intended.
The principle is valid, but the capture method is already generic, Desktop is not defining product event types, It should not know that begin_checkout or app:run_button_clicked exist unless we deliberately decide Desktop should validate product analytics schemas. We can force this, but I would point towards this not being a correctness blocker.
Note I think the common pattern here is that a while these principles are valid or directionally correct, but none of them answers the review question of whether or not this code can merge, which is the goal of code review and what's blocking progression right now. On this point again, Desktop is not defining or mirroring product-specific telemetry methods/events. It is a generic Telemetry.capture(event, properties) method. The boundary being described here is what is already implemented.
I think we are going in circles on this one, and probably need to loop in TL @thedatalife. On the point as a whole, I think using frontend PostHog directly would move us in the wrong direction. It would require Desktop to expose/synchronize identity, consent, config, app metadata, and lifecycle behavior into the renderer, and we would still need policy around PII/rate limiting/retry semantics. That is a wider contract than capture(event, properties). The bridge keeps the boundary smaller: frontend owns event names, payloads, and emission timing; Desktop owns the telemetry client, identity, consent, persistence, retry/rate limiting, and final transport. Stitching may be possible, but it is not a substitute for keeping a single Desktop telemetry authority. On your point specifically, I think the phrase “parallel telemetry provider hierarchy” may be conflating two different layers. If you mean Desktop has its own telemetry stack and the frontend now has a When you say “what we actually have is another telemetry transport,” I agree. That is the intended shape. The current frontend telemetry abstraction models transports as providers, so adding a Desktop transport means adding a frontend provider implementation. The important boundary is that Desktop itself does not get product-specific telemetry methods or event definitions; it only exposes |
benceruleanlu
changed the base branch from
bl/comfyui-desktop-bridge-types
to
bl/desktop2-frontend-distribution
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
from
b73499c to
1db0930
Compare
dosubot
Bot
added
size:XL
benceruleanlu
changed the base branch from
bl/desktop2-frontend-distribution
to
bl/telemetry-run-button-context
dosubot
Bot
added
size:L
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
from
d6c3b0c to
48eb979
Compare
📦 Bundle: 7.45 MB gzip 🔴 +8.36 kBDetailsSummary
Category Glance App Entry Points — 46.7 kB (baseline 45.8 kB) • 🔴 +958 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.25 MB (baseline 1.25 MB) • 🔴 +151 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 95.3 kB (baseline 95.3 kB) • 🔴 +42 BTop-level views, pages, and routed surfaces
Status: 12 added / 12 removed Panels & Settings — 525 kB (baseline 523 kB) • 🔴 +2.45 kBConfiguration panels, inspectors, and settings screens
Status: 24 added / 22 removed / 2 unchanged User & Accounts — 19.9 kB (baseline 19.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 9 added / 9 removed Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 5 added / 5 removed UI Components — 57.2 kB (baseline 57.2 kB) • ⚪ 0 BReusable component library chunks
Status: 13 added / 13 removed Data & Services — 266 kB (baseline 266 kB) • 🟢 -202 BStores, services, APIs, and repositories
Status: 15 added / 14 removed / 1 unchanged Utilities & Hooks — 3.32 MB (baseline 3.32 MB) • 🟢 -1.1 kBHelpers, composables, and utility bundles
Status: 30 added / 30 removed Vendor & Third-Party — 15.3 MB (baseline 15.3 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 7 added / 7 removed / 9 unchanged Other — 10.4 MB (baseline 10.4 MB) • 🔴 +27.9 kBBundles that do not match a named category
Status: 142 added / 139 removed / 9 unchanged ⚡ Performance
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1667 passed, 0 failed · 3 flaky📊 Browser Reports
|
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
2 times, most recently
from
fff3274 to
f2b1f95
Compare
benceruleanlu
force-pushed
the
bl/telemetry-run-button-context
branch
from
128e035 to
0fcbdb0
Compare
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
from
f2b1f95 to
d2459d9
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/uy-tieu-s-projects?upgradeToPro=build-rate-limit |
dosubot
Bot
added
size:XL
benceruleanlu
force-pushed
the
bl/mar-240-frontend-event-sink
branch
from
d2459d9 to
cd1d95f
Compare
dosubot
Bot
added
size:L
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
3 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.
Summary
window.__comfyDesktop2.Telemetry.captureusing the existing event namesPaired change
Validation
pnpm typecheckpnpm format:checkpnpm lintpnpm knippnpm test:unit src/platform/missingModel/missingModelDownload.test.ts src/platform/telemetry/initDesktopTelemetry.test.ts src/platform/telemetry/providers/desktop/DesktopTelemetryProvider.test.ts.yamllintMAR-240