fix(telemetry): remove redundant run tracking; keep click analytics + single execution event #6518
Conversation
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
dosubot
bot
added
the
size:M
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/03/2025, 02:33:48 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/03/2025, 02:45:13 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.27 MB (baseline 3.27 MB) • 🔴 +169 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 725 kB (baseline 725 kB) • 🔴 +24 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 293 kB (baseline 293 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.6 kB (baseline 12.5 kB) • 🔴 +111 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10.4 kB (baseline 11.4 kB) • 🟢 -1 kBStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
DrJKL
previously approved these changes
Nov 1, 2025
DrJKL
previously approved these changes
Nov 1, 2025
benceruleanlu
added
needs-backport
benceruleanlu
changed the title
dosubot
bot
added
size:L
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary: - Remove trackRunTriggeredViaKeybinding() and trackRunTriggeredViaMenu() methods - Remove duplicate tracking calls from keybindings, menu, and legacy UI - Keep only trackWorkflowExecution() in commands for centralized tracking Problem: The previous implementation added tracking at multiple levels: 1. Keybindings tracked, then called command 2. Menu tracked, then called command 3. Command also tracked This caused double (or triple) tracking for the same user action. Solution: - Commands already call trackWorkflowExecution() when executing - Keybindings and menu items call commands, so they inherit tracking - Legacy UI buttons call app.queuePrompt() directly, so they keep tracking - No need to distinguish button/keybinding/menu - all are workflow executions Files changed: - src/services/keybindingService.ts: Remove tracking before command execution - src/stores/menuItemStore.ts: Remove tracking before command execution - src/scripts/ui.ts: Change trackRunTriggeredViaMenu to trackWorkflowExecution - src/platform/telemetry/types.ts: Remove unused methods and event constants - src/platform/telemetry/providers/cloud/MixpanelTelemetryProvider.ts: Remove implementations
The Vue queue button was calling both trackRunButton() and then executing the command which calls trackWorkflowExecution(), causing double-tracking. Now all paths (button, keybinding, menu) go through the command as the single centralized gateway for tracking.
Corrected the tracking to use trackRunButton() (app:run_button_click) instead of trackWorkflowExecution() (execution_start). Key difference: - trackRunButton() = User initiates a run (button/keybinding/menu) - trackWorkflowExecution() = Execution actually starts (WebSocket event) Now all paths converge on trackRunButton() in the command: 1. Button → command → trackRunButton() → app.queuePrompt() 2. Keybinding → command → trackRunButton() → app.queuePrompt() 3. Menu → command → trackRunButton() → app.queuePrompt() 4. Legacy UI → trackRunButton() → app.queuePrompt() (bypasses command) Also added missing tracking to Comfy.QueueSelectedOutputNodes command.
Following VS Code's command pattern, commands can now accept optional
arguments that get passed through from execute().
Changes:
- ComfyCommand.function now accepts ...args: unknown[]
- commandStore.execute() now accepts ...args and passes them through
- Queue commands (QueuePrompt, QueuePromptFront, QueueSelectedOutputNodes)
now accept optional metadata: { subscribe_to_run?: boolean }
- Vue queue button passes { subscribe_to_run: false } to track properly
- Keybindings and menu items continue to work without passing args
Benefits:
- Maximally flexible like VS Code
- Non-breaking (existing callers work unchanged)
- Commands opt-in to args they need
- No forced positional parameters
- Extensible for future metadata needs
Reference: VS Code's executeCommand uses this same pattern
(/tmp/vscode/src/vs/platform/commands/common/commands.ts:26)
Following VS Code's actual implementation, commands now use generics with proper type widening when stored. Changes: - ComfyCommand and ComfyCommandImpl are now generic: ComfyCommand<TArgs> - Storage uses default (unknown[]): Map<string, ComfyCommandImpl> - Queue commands are properly typed with specific metadata signature - Commands array uses type assertion to widen to ComfyCommand[] - Extension commands wrap functions to match signature - All formatting and linting applied This approach: - Provides type safety at definition time - Allows flexible args per command - Storage naturally erases types (like VS Code) - No casts needed in command bodies - Extensible for future args Verified against VS Code source: /tmp/vscode/src/vs/platform/commands/common/commands.ts All checks passed: ✅ Lint ✅ Format ✅ Typecheck ✅ Unit tests (2800+ tests)
…) and restore single execution_start; remove duplicate pre-tracking
christian-byrne
force-pushed
the
fix/simplify-run-telemetry
branch
3 times, most recently
from
6033d7b to
395d2f0
Compare
Restores the queue_run_multiple_batches_submitted tracking that was accidentally removed during conflict resolution. This tracking was added in PR #6511 to track when users submit multiple batches. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
christian-byrne
force-pushed
the
fix/simplify-run-telemetry
branch
from
395d2f0 to
e026c25
Compare
Changes execute signature from execute(commandId, errorHandler?, ...metadata)
to execute(commandId, { errorHandler?, metadata? })
This eliminates awkward undefined placeholders and makes the API more extensible.
- Update ComfyCommand interface with concrete metadata signature - Remove type assertion from command registration - Remove redundant isCloud checks around telemetry calls
Use proper TypeScript type for error parameters instead of any
|
@christian-byrne Backport to Please manually cherry-pick commit Conflicting files
|
christian-byrne
added a commit
that referenced
this pull request
Nov 3, 2025
… single execution event (#6518) Deduplicates workflow run telemetry and keeps a single source of truth for execution while retaining click analytics and attributing initiator source. - Keep execution tracking in one place via `trackWorkflowExecution()` - Keep click analytics via `trackRunButton(...)` - Attribute initiator with `trigger_source` = 'button' | 'keybinding' | 'legacy_ui' - Remove pre‑tracking from keybindings to avoid double/triple counting - Update legacy UI buttons to emit both click + execution events (they bypass commands) PR #6499 added tracking at multiple layers: 1) Keybindings tracked via a dedicated method and then executed a command 2) Menu items tracked via a dedicated method and then executed a command 3) Commands also tracked execution Because these ultimately trigger the same command path, this produced duplicate (sometimes triplicate) events per user action and made it hard to attribute initiator precisely. - Remove redundant tracking from keybindings (and previous legacy menu handler) - Commands now emit both: - `trackRunButton(...)` (click analytics, includes `trigger_source` when provided) - `trackWorkflowExecution()` (single execution start; includes the last `trigger_source`) - Legacy UI buttons (which call `app.queuePrompt(...)` directly) now also emit both events with `trigger_source = 'legacy_ui'` - Add `ExecutionTriggerSource` type and wire `trigger_source` through provider so `EXECUTION_START` matches the most recent click intent - `RUN_BUTTON_CLICKED` (click analytics) - Emitted when a run is initiated via: - Button: `trigger_source = 'button'` - Keybinding: `trigger_source = 'keybinding'` - Legacy UI: `trigger_source = 'legacy_ui'` - `EXECUTION_START` (execution lifecycle) - Emitted once per run at start; includes `trigger_source` matched to the click intent above - Paired with `EXECUTION_SUCCESS` / `EXECUTION_ERROR` from execution handlers - ✅ Accurate counts by removing duplicated run events - ✅ Clear initiator attribution (button vs keybinding vs legacy UI) - ✅ Separation of “intent” (click) vs. “lifecycle” (execution) - ✅ Simpler implementation and maintenance - `src/services/keybindingService.ts`: Route run commands with `trigger_source = 'keybinding'` - `src/components/actionbar/ComfyRunButton/ComfyQueueButton.vue`: Send `trigger_source = 'button'` to commands - `src/scripts/ui.ts`: Legacy queue buttons emit `trackRunButton({ trigger_source: 'legacy_ui' })` and `trackWorkflowExecution()` - `src/composables/useCoreCommands.ts`: Commands emit `trackRunButton(...)` + `trackWorkflowExecution()`; accept telemetry metadata - `src/platform/telemetry/types.ts`: Add `ExecutionTriggerSource` and optional `trigger_source` in click + execution payloads - `src/platform/telemetry/providers/cloud/MixpanelTelemetryProvider.ts`: Carry `trigger_source` from click → execution and reset after use - `src/stores/commandStore.ts`: Allow commands to receive args (for telemetry metadata) - `src/extensions/core/groupNode.ts`: Adjust command function signatures to new execute signature - Reverts the multi‑event approach from #6499 - Keeps `trackWorkflowExecution()` as the canonical execution event while preserving click analytics and initiator attribution with `trackRunButton(...)` ┆Issue is synchronized with this Notion page by Unito --------- Co-authored-by: Christian Byrne <c.byrne@comfy.org> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Benjamin Lu <benjaminlu1107@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
christian-byrne
added a commit
that referenced
this pull request
Nov 3, 2025
…keep click analytics + single execution event (#6552) ## Summary Manual backport of #6518 to the `rh-test` branch. Deduplicates workflow run telemetry and keeps a single source of truth for execution while retaining click analytics and attributing initiator source. - Keep execution tracking in one place via `trackWorkflowExecution()` - Keep click analytics via `trackRunButton(...)` - Attribute initiator with `trigger_source` = 'button' | 'keybinding' | 'legacy_ui' - Remove pre-tracking from keybindings to avoid double/triple counting - Update legacy UI buttons to emit both click + execution events ## Backport Notes This backport required manual conflict resolution in: - `src/components/actionbar/ComfyRunButton/ComfyQueueButton.vue` - Added batchCount tracking and trigger_source metadata - `src/composables/useCoreCommands.ts` - Added error handling and execution tracking - `src/platform/telemetry/providers/cloud/MixpanelTelemetryProvider.ts` - Updated trackRunButton signature with trigger_source support Additionally added: - `trackUiButtonClicked` method to TelemetryProvider interface - `UiButtonClickMetadata` type definition - `UI_BUTTON_CLICKED` event constant All conflicts resolved intelligently to maintain the intent of the original PR while adapting to the rh-test branch codebase. ## Original PR - Original PR: #6518 - Original commit: 6fe88db ## Testing - ✅ Typecheck passed - ✅ Pre-commit hooks passed (lint, format) - ✅ All conflicts resolved ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6552-Backport-to-rh-test-fix-telemetry-remove-redundant-run-tracking-keep-click-analytics-2a06d73d365081f78e4ad46a16be69f1) by [Unito](https://www.unito.io) --------- Co-authored-by: Christian Byrne <c.byrne@comfy.org> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Benjamin Lu <benjaminlu1107@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Christian Byrne <chrbyrne96@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Deduplicates workflow run telemetry and keeps a single source of truth for execution while retaining click analytics and attributing initiator source.
trackWorkflowExecution()trackRunButton(...)trigger_source= 'button' | 'keybinding' | 'legacy_ui'Problem
PR #6499 added tracking at multiple layers:
Because these ultimately trigger the same command path, this produced duplicate (sometimes triplicate) events per user action and made it hard to attribute initiator precisely.
Solution
trackRunButton(...)(click analytics, includestrigger_sourcewhen provided)trackWorkflowExecution()(single execution start; includes the lasttrigger_source)app.queuePrompt(...)directly) now also emit both events withtrigger_source = 'legacy_ui'ExecutionTriggerSourcetype and wiretrigger_sourcethrough provider soEXECUTION_STARTmatches the most recent click intentTelemetry behavior after this change
RUN_BUTTON_CLICKED(click analytics)trigger_source = 'button'trigger_source = 'keybinding'trigger_source = 'legacy_ui'EXECUTION_START(execution lifecycle)trigger_sourcematched to the click intent aboveEXECUTION_SUCCESS/EXECUTION_ERRORfrom execution handlersBenefits
Files Changed (high level)
src/services/keybindingService.ts: Route run commands withtrigger_source = 'keybinding'src/components/actionbar/ComfyRunButton/ComfyQueueButton.vue: Sendtrigger_source = 'button'to commandssrc/scripts/ui.ts: Legacy queue buttons emittrackRunButton({ trigger_source: 'legacy_ui' })andtrackWorkflowExecution()src/composables/useCoreCommands.ts: Commands emittrackRunButton(...)+trackWorkflowExecution(); accept telemetry metadatasrc/platform/telemetry/types.ts: AddExecutionTriggerSourceand optionaltrigger_sourcein click + execution payloadssrc/platform/telemetry/providers/cloud/MixpanelTelemetryProvider.ts: Carrytrigger_sourcefrom click → execution and reset after usesrc/stores/commandStore.ts: Allow commands to receive args (for telemetry metadata)src/extensions/core/groupNode.ts: Adjust command function signatures to new execute signatureRelated
trackWorkflowExecution()as the canonical execution event while preserving click analytics and initiator attribution withtrackRunButton(...)┆Issue is synchronized with this Notion page by Unito