feat(telemetry): instrument cloud funnel gaps + enable cloud web analytics - #12935
feat(telemetry): instrument cloud funnel gaps + enable cloud web analytics#12935deepme987 wants to merge 3 commits into
Conversation
app:subscribe_now_button_clicked was only fired by the legacy
SubscribeButton, never by the pricing table tier buttons or the
subscribe-to-run lock button - the surfaces users actually click. Add
the same trackSubscription('subscribe_clicked') capture to:
- PricingTable.handleSubscribe (both the new-subscriber and plan-change
paths), before checkout opens, carrying { tier, cycle }
- SubscribeToRun.handleSubscribeToRun, alongside the existing run-button
event
Each surface now tags a source ('pricing_table' | 'subscribe_to_run' |
'subscribe_button') so the subscribe-click funnel can be attributed by
CTA. Extend SubscriptionMetadata with tier/cycle/source; no new event
name.
…oud web analytics
📝 WalkthroughWalkthroughThree new telemetry event types are defined ( ChangesTelemetry Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: 🕵🏻 0 passed, 0 failed📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts (1)
101-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate initialization expectations to match the new defaults
Line 110-Line 113 still assert
autocapture,capture_pageview, and
capture_pageleavearefalse, but the provider now sets them totrue.
This test will fail once the type error in the provider is fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts` around lines 101 - 114, In the test for PostHog initialization (the it block that calls posthog.init with token and default config), update the three boolean properties in the expect.objectContaining call: change autocapture from false to true, capture_pageview from false to true, and capture_pageleave from false to true to match the new default values set by the provider.src/stores/authStore.ts (1)
340-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuth-error telemetry currently includes post-auth provisioning failures.
At Line 344-350,
createCustomer()runs inside the sametrythat is caught at Line 353. If auth succeeds but customer creation fails, Line 356 still emitstrackAuthError, which pollutes auth-failure analytics with downstream provisioning errors.Proposed fix
const executeAuthAction = async <T>( @@ ): Promise<T> => { loading.value = true + let result: T try { - const result = await action(auth) + result = await action(auth) + } catch (error) { + if (isCloud && options?.authError) { + useTelemetry()?.trackAuthError({ + method: options.authError.method, + is_sign_up: options.authError.isSignUp, + error_code: (error as { code?: string })?.code, + error_message: + error instanceof Error ? error.message : String(error) + }) + } + throw error + } - // Create customer if needed + try { + // Create customer if needed (do not classify as auth_error) if (options?.createCustomer) { const token = await getIdToken() if (!token) { throw new Error('Cannot create customer: User not authenticated') } await createCustomer() } return result - } catch (error) { - // Surface the auth failure leak that trackAuth (success-only) misses. - if (isCloud && options?.authError) { - useTelemetry()?.trackAuthError({ - method: options.authError.method, - is_sign_up: options.authError.isSignUp, - error_code: (error as { code?: string })?.code, - error_message: - error instanceof Error ? error.message : String(error) - }) - } - throw error } finally { loading.value = false } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/authStore.ts` around lines 340 - 365, The customer creation block within the action function (lines 344-350) shares the same try-catch error handler as the auth action call, causing createCustomer failures to incorrectly trigger trackAuthError telemetry even when authentication succeeds. To fix this, restructure the code by moving the customer creation logic into a separate try-catch block that is executed after the auth action succeeds and outside the catch block that handles authError tracking. This ensures trackAuthError only fires for actual authentication failures from the action call, not for downstream provisioning failures in createCustomer.
🧹 Nitpick comments (1)
src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts (1)
327-337: ⚡ Quick winAdd capture assertions for the other two new telemetry methods
This file adds coverage for
trackBillingCycleToggled, but the new
trackAuthErrorandtrackTemplateCategorySelectedpaths are still untested
here. Add one assertion per method to keep parity with provider changes and
reduce regression risk.
As per coding guidelines, "Write tests for all changes, especially bug fixes to catch future regressions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts` around lines 327 - 337, The test file PostHogTelemetryProvider.test.ts currently has a test case for trackBillingCycleToggled but is missing test coverage for the two other new telemetry methods: trackAuthError and trackTemplateCategorySelected. Add two new test cases following the same pattern as the existing trackBillingCycleToggled test - each should create a provider instance, call the respective method with appropriate test parameters, and assert that hoisted.mockCapture is called with the expected TelemetryEvents constant and payload data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/custom/widget/WorkflowTemplateSelectorDialog.vue`:
- Around line 549-552: The watcher on selectedNavItem in
WorkflowTemplateSelectorDialog.vue is firing telemetry for all changes to
selectedNavItem, including programmatic changes that occur when the sort order
changes (line 622 resets selectedNavItem from popular to all). Move the
trackTemplateCategorySelected telemetry call from the selectedNavItem watcher to
the actual user interaction handler (the click/select event that triggers the
category selection) instead, so telemetry only fires for intentional user
selections and not for programmatic resets triggered by sort changes.
In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.ts`:
- Around line 132-140: The PostHog SDK initialization configuration in
PostHogTelemetryProvider contains an invalid configuration key `heatmaps: true`
which is not supported by PostHog SDK v1.358.1. Replace the `heatmaps` key with
`enable_heatmaps` in the configuration object that includes other properties
like `autocapture`, `capture_pageview`, `capture_pageleave`, and `persistence`
to properly enable heatmaps functionality in PostHog.
---
Outside diff comments:
In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts`:
- Around line 101-114: In the test for PostHog initialization (the it block that
calls posthog.init with token and default config), update the three boolean
properties in the expect.objectContaining call: change autocapture from false to
true, capture_pageview from false to true, and capture_pageleave from false to
true to match the new default values set by the provider.
In `@src/stores/authStore.ts`:
- Around line 340-365: The customer creation block within the action function
(lines 344-350) shares the same try-catch error handler as the auth action call,
causing createCustomer failures to incorrectly trigger trackAuthError telemetry
even when authentication succeeds. To fix this, restructure the code by moving
the customer creation logic into a separate try-catch block that is executed
after the auth action succeeds and outside the catch block that handles
authError tracking. This ensures trackAuthError only fires for actual
authentication failures from the action call, not for downstream provisioning
failures in createCustomer.
---
Nitpick comments:
In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.ts`:
- Around line 327-337: The test file PostHogTelemetryProvider.test.ts currently
has a test case for trackBillingCycleToggled but is missing test coverage for
the two other new telemetry methods: trackAuthError and
trackTemplateCategorySelected. Add two new test cases following the same pattern
as the existing trackBillingCycleToggled test - each should create a provider
instance, call the respective method with appropriate test parameters, and
assert that hoisted.mockCapture is called with the expected TelemetryEvents
constant and payload data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f954fac-5988-4331-96f7-95adeb4eaed1
📒 Files selected for processing (11)
src/components/custom/widget/WorkflowTemplateSelectorDialog.vuesrc/platform/cloud/subscription/components/PricingTable.test.tssrc/platform/cloud/subscription/components/PricingTable.vuesrc/platform/cloud/subscription/components/SubscribeButton.vuesrc/platform/cloud/subscription/components/SubscribeToRun.test.tssrc/platform/cloud/subscription/components/SubscribeToRun.vuesrc/platform/telemetry/TelemetryRegistry.tssrc/platform/telemetry/providers/cloud/PostHogTelemetryProvider.test.tssrc/platform/telemetry/providers/cloud/PostHogTelemetryProvider.tssrc/platform/telemetry/types.tssrc/stores/authStore.ts
| watch(selectedNavItem, (to, from) => { | ||
| if (!to || to === from) return | ||
| useTelemetry()?.trackTemplateCategorySelected({ category_id: to }) | ||
| }) |
There was a problem hiding this comment.
Category-selection telemetry fires on internal sort-driven nav resets.
Line 549 watches all selectedNavItem changes, but Line 622 also sets selectedNavItem programmatically (popular → all) when sort changes. That means trackTemplateCategorySelected is emitted for non-user category selections.
Proposed fix
const selectedNavItem = ref<string | null>(initialCategory)
+const navChangeSource = ref<'user' | 'sort_sync'>('user')
watch(selectedNavItem, (to, from) => {
+ if (navChangeSource.value === 'sort_sync') {
+ navChangeSource.value = 'user'
+ return
+ }
if (!to || to === from) return
useTelemetry()?.trackTemplateCategorySelected({ category_id: to })
})
@@
} else if (source === 'sort') {
@@
if (isPopularNav && !isPopularSort) {
+ navChangeSource.value = 'sort_sync'
selectedNavItem.value = 'all'
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/custom/widget/WorkflowTemplateSelectorDialog.vue` around lines
549 - 552, The watcher on selectedNavItem in WorkflowTemplateSelectorDialog.vue
is firing telemetry for all changes to selectedNavItem, including programmatic
changes that occur when the sort order changes (line 622 resets selectedNavItem
from popular to all). Move the trackTemplateCategorySelected telemetry call from
the selectedNavItem watcher to the actual user interaction handler (the
click/select event that triggers the category selection) instead, so telemetry
only fires for intentional user selections and not for programmatic resets
triggered by sort changes.
| // Web analytics enabled so cloud.comfy.org gets heatmaps + $pageview | ||
| // (the login/onboarding pages previously had zero coverage). autocapture | ||
| // does NOT record input *values* (posthog masks them), and these defaults | ||
| // remain overridable per-environment via `serverConfig` (spread below). | ||
| autocapture: true, | ||
| capture_pageview: true, | ||
| capture_pageleave: true, | ||
| heatmaps: true, | ||
| persistence: 'localStorage+cookie', |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the posthog-js version used in this repository, what is the correct posthog.initoption to enable heatmaps? Isheatmapsvalid, or should a different key be used (for exampledisable_heatmaps)?
💡 Result:
To enable heatmaps in posthog-js, the correct configuration option to use in the posthog.init call is enable_heatmaps [1][2]. You should set this option to true within the configuration object: posthog.init('<ph_project_token>', { api_host: 'https://us.i.posthog.com', enable_heatmaps: true }) While some internal configuration references or older/alternative settings may show capture_heatmaps [3], the official and documented key for enabling this feature via initialization is enable_heatmaps [1][2]. Note that if you want to disable features, PostHog typically uses a disable_* prefix (e.g., disable_session_recording), but for enabling heatmaps, the enable_heatmaps key is the standard approach [1][3].
Citations:
- 1: https://posthog.com/docs/libraries/js/config
- 2: https://posthog.com/docs/toolbar/heatmaps
- 3: https://posthog.com/docs/references/posthog-js-1.297.0/types/PostHogConfig
🏁 Script executed:
# Check the PostHogTelemetryProvider.ts file around line 139
sed -n '130,145p' src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.ts
# Also check the PostHog version in package.json
rg '"posthog"' package.json -A 1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1058
🏁 Script executed:
# Check PostHog version
rg '"posthog"' package.json -B 1 -A 1
# Also verify the TypeScript error by checking tsconfig
fd -n tsconfigRepository: Comfy-Org/ComfyUI_frontend
Length of output: 301
🏁 Script executed:
# Check PostHog version
rg '"posthog"' package.json -B 1 -A 1
# Find tsconfig files
fd tsconfigRepository: Comfy-Org/ComfyUI_frontend
Length of output: 502
🏁 Script executed:
# Find package.json files and check PostHog version
find . -name "package.json" -type f | head -5 | xargs grep -l "posthog"
# Check the root package.json for PostHog
cat package.json | grep -A 2 -B 2 "posthog"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 252
🏁 Script executed:
# Check pnpm workspace catalog or pnpm-lock.yaml for actual PostHog version
rg 'posthog-js' pnpm-lock.yaml -A 3 | head -20
# Also check the catalog in package.json structure
cat package.json | grep -A 50 "catalog"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5781
🏁 Script executed:
# Check if there are any TypeScript definitions for PostHog to see what keys are allowed
rg "interface PostHogConfig" --type ts -A 20
# Also search for any existing usage of enable_heatmaps or heatmaps in the codebase
rg "enable_heatmaps|heatmaps" --type ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1356
🏁 Script executed:
# Check the node_modules PostHog types to see if heatmaps is a valid config key
rg "heatmaps|enable_heatmaps" node_modules/posthog-js -t ts -A 1 -B 1 2>/dev/null | head -30
# Also check what keys are actually defined in PostHogConfig
rg "PostHogConfig" node_modules/posthog-js --type ts -A 30 2>/dev/null | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check if there's a build or tsc output showing the error
rg "TS2353" . --type ts --type json 2>/dev/null | head -10
# Let's verify the exact line and surrounding context
sed -n '128,145p' src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.ts | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1249
Replace heatmaps with enable_heatmaps on line 139
The PostHog SDK (v1.358.1) does not support the heatmaps configuration key. Use enable_heatmaps: true instead to enable heatmaps in the initialization config.
🧰 Tools
🪛 GitHub Actions: CI: Dist Telemetry Scan / 0_scan.txt
[error] 139-139: TypeScript (vue-tsc) error TS2353: Object literal may only specify known properties; 'heatmaps' does not exist in type 'Partial'.
🪛 GitHub Actions: CI: Dist Telemetry Scan / scan
[error] 139-139: TypeScript (vue-tsc) error TS2353: Object literal may only specify known properties, and 'heatmaps' does not exist in type 'Partial'.
🪛 GitHub Actions: CI: OSS Assets Validation / 1_validate-fonts.txt
[error] 139-139: TypeScript (vue-tsc) failed with TS2353: Object literal may only specify known properties; property 'heatmaps' does not exist in type 'Partial'.
🪛 GitHub Actions: CI: OSS Assets Validation / validate-fonts
[error] 139-139: TypeScript (vue-tsc) error TS2353: Object literal may only specify known properties, and 'heatmaps' does not exist in type 'Partial'.
🪛 GitHub Check: scan
[failure] 139-139:
Object literal may only specify known properties, and 'heatmaps' does not exist in type 'Partial'.
🪛 GitHub Check: validate-fonts
[failure] 139-139:
Object literal may only specify known properties, and 'heatmaps' does not exist in type 'Partial'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/telemetry/providers/cloud/PostHogTelemetryProvider.ts` around
lines 132 - 140, The PostHog SDK initialization configuration in
PostHogTelemetryProvider contains an invalid configuration key `heatmaps: true`
which is not supported by PostHog SDK v1.358.1. Replace the `heatmaps` key with
`enable_heatmaps` in the configuration object that includes other properties
like `autocapture`, `capture_pageview`, `capture_pageleave`, and `persistence`
to properly enable heatmaps functionality in PostHog.
Sources: Linters/SAST tools, Pipeline failures
|
Folded into #12894 (the consolidated cloud telemetry PR): merged this branch in, unioning the new events (auth_error, billing_cycle_toggled, template_category_selected, subscribe-click coverage) with #12894's funnel events. Note: |
1 participant
Summary
Closes the cloud funnel-telemetry gaps from the funnel audit: subscribe-click coverage, billing-cycle toggle, auth errors, template-category selection, and enabling web analytics (heatmaps /
$pageview) on cloud.comfy.org.Changes
app:subscribe_now_button_clickednow fires from the real tier CTAs inPricingTable.vue(handleSubscribe, new + change) andSubscribeToRun.vue, not onlySubscribeButton.vue. Previously the event undercounted vs actual conversions because the forced post-signup pricing modal's tier buttons never fired it.app:billing_cycle_toggled {from,to}on the monthly/yearly switch (today ~96% pick monthly, ~4% annual, yearly is default — we had no visibility into the toggle).app:auth_error {method,is_sign_up,error_code,error_message}on the sign-in/up failure path (the signup leak that success-onlyapp:user_auth_completedcan't see).app:template_category_selected {category_id}when the user switches tab/category (e.g. Getting Started vs All) in the template selector.autocapture,capture_pageview,capture_pageleave, andheatmapsin the cloud app's posthog-js init. cloud.comfy.org currently emits zero$pageview/autocapture, so the login/onboarding pages have no heatmaps and there is no on-site sourcing.Review Focus
posthog_config(spread after them), so this can be tuned or disabled remotely without a deploy.$pageviewcapture — worth confirming route-change pageviews actually fire on the Vue SPA (add a routerafterEachhook if not).reasononapp:subscription_required_modal_opened(it arrives empty in prod).useSubscriptionDialogalready threads areasoninto the dialog, but themodal_openedevent and the ~10showSubscriptionDialogcall-sites don't pass it, and the reason taxonomy (subscription_required/out_of_credits/top_up_blocked) needs a product decision on what to track — left out deliberately.Companion PRs
website:cta_clicked) on the comfy.org marketing site.Comfy-Org/Comfy-Desktop.