feat(telemetry): track settings changes #6504
Merged
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
… unit test for settings telemetry trigger
dosubot
bot
added
the
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/02/2025, 06:25:45 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/02/2025, 06:41:09 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
benceruleanlu
changed the title
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.27 MB (baseline 3.27 MB) • 🔴 +104 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 724 kB (baseline 724 kB) • ⚪ 0 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 — 295 kB (baseline 295 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 11.4 kB (baseline 11.4 kB) • ⚪ 0 BStores, 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
|
benceruleanlu
force-pushed
the
chief-pig
branch
from
db10f3b to
75857b3
Compare
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: feat(telemetry): track settings changes (#6504)
Impact: 117 additions, 2 deletions across 4 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 3
- Low: 3
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 1 issue
- Code Quality: 3 issues
Key Findings
Architecture & Design
Positive: The implementation follows a good pattern by only tracking telemetry when the settings dialog is open, preventing unnecessary data collection during programmatic settings changes.
Concerns:
- The telemetry tracking is directly embedded in the settings store, violating single responsibility principle
- Error handling could be more robust with better isolation between core functionality and telemetry
Recommendation: Consider extracting telemetry to a separate service that observes settings changes through Vue's reactivity system.
Security Considerations
Critical Issue: The current implementation tracks previous_value and new_value as unknown types, which could inadvertently capture sensitive data like API keys, tokens, or personal information in telemetry systems.
Recommendation: Implement a whitelist approach for trackable settings or add data sanitization to prevent sensitive information leaks.
Performance Impact
Minor Issue: There's unnecessary computation happening on every settings change (like getSettingInfo calls) even when telemetry won't be sent.
Bundle Impact: Minimal - only adds telemetry interface and tracking logic, with good conditional compilation for OSS builds.
Integration Points
Extension Compatibility: Changes are additive and shouldn't affect existing extensions or settings functionality.
Testing: Excellent test coverage with both positive and negative test cases for telemetry triggering.
Positive Observations
- Excellent Test Coverage: Comprehensive unit tests covering both telemetry enabled/disabled scenarios
- OSS Build Safety: Proper conditional compilation ensures telemetry is excluded from open source builds
- Type Safety: Good TypeScript usage with proper interfaces and type definitions
- Error Handling: Basic error handling with try-catch blocks and console error logging
- Conditional Logic: Smart approach to only track when settings dialog is open
- Code Organization: Clean separation of telemetry types and provider implementation
Next Steps
- Address security issue - Implement data sanitization for sensitive settings before merge
- Consider architectural feedback for long-term maintainability (can be addressed in follow-up PR)
- Performance optimization - Move unnecessary computations inside conditional blocks
- Code cleanup - Remove unnecessary import reordering for cleaner diffs
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
dosubot
bot
added
size:M
dosubot
bot
added
size:L
benceruleanlu
added
needs-backport
christian-byrne
approved these changes
Nov 2, 2025
|
@benceruleanlu Backport to Please manually cherry-pick commit Conflicting files
|
benceruleanlu
added a commit
that referenced
this pull request
Nov 4, 2025
…factor dialogService telemetry import\n\n- Backport of #6504 and https://github.com/Comfy-Org/ComfyUI_frontend/pull/6511\n- Remove input_type, category, sub_category from SettingChangedMetadata\n- Replace lazy import in dialogService error dialog onClose with top-level useTelemetry()\n- Lint and typecheck pass (pnpm lint:fix && pnpm typecheck)
christian-byrne
pushed a commit
that referenced
this pull request
Nov 4, 2025
…import refactor (#6567) Backports the combined changes from the following PRs into `rh-test`: - #6504 — Settings telemetry (track `SETTING_CHANGED` on successful update) - #6511 — UI telemetry (actionbar drag handle, run button choices/multi‑batch submit, breadcrumb item/root selection) Key points - Settings telemetry added via `SettingItem.vue` after successful setting updates and wired to `TelemetryEvents.SETTING_CHANGED`. - UI telemetry wired for run/queue actions and breadcrumbs to match upstream behavior. Divergences from the source PRs - Removed `input_type`, `category`, and `sub_category` from `SettingChangedMetadata` to keep the event shape focused and consistent with downstream consumers. - Replaced lazy telemetry import in `dialogService` error dialog `onClose` handlers with a top‑level `useTelemetry()` import for clarity and to avoid unnecessary dynamic imports. - Kept a few additional telemetry events already present in this branch (error dialog actions, graph/sidebar/template interactions). Happy to trim these for a strict backport if desired. Validation - Ran `pnpm lint:fix && pnpm typecheck` successfully locally. References - Upstream PRs: #6504, #6511 - Branch: `backport-6511-6504-to-rh-test` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6567-Backport-telemetry-settings-UI-tracking-6504-6511-and-dialog-import-refactor-2a16d73d365081ce80a0f973c4483653) by [Unito](https://www.unito.io)
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
settingParameter,settingType) for readabilitySettingChangedMetadataandTelemetryEvents.SETTING_CHANGEDtrackSettingChangedin Mixpanel providerQuality
pnpm lint:fixandpnpm typecheckNotes
useDialogStore().isDialogOpen('global-settings'))useTelemetry()returns null)┆Issue is synchronized with this Notion page by Unito