fix: route gtm through telemetry entrypoint#8354
Merged
benceruleanlu merged 48 commits intomainfrom Feb 7, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a registry-based telemetry system (Mixpanel + GTM), cloud-only init, GTM runtime/types and remote-config flag, pageview/auth/checkout tracking and attribution, TelemetryRegistry and GTM provider, checkout-attribution util and tests, subscription checkout telemetry hooks/tests, ComfyWorkflow extraction, and a CI dist telemetry scan workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Main as src/main.ts
participant Init as initTelemetry()
participant Index as telemetry index
participant Registry as TelemetryRegistry
participant Mixpanel as MixpanelProvider
participant GTM as GtmTelemetryProvider
App->>Main: boot (cloud build)
Main->>Init: dynamic import & call initTelemetry()
activate Init
Init->>Registry: create TelemetryRegistry
Registry->>Mixpanel: registerProvider(Mixpanel)
Registry->>GTM: registerProvider(GTM)
GTM->>GTM: initialize() (ensure window.dataLayer, inject GTM script)
Registry-->>Init: registry ready
Init->>Index: setTelemetryRegistry(registry)
deactivate Init
sequenceDiagram
participant User as User
participant Router as src/router.ts
participant Telemetry as TelemetryRegistry
participant GTM as GtmTelemetryProvider
participant Mixpanel as MixpanelProvider
participant DataLayer as window.dataLayer
User->>Router: navigate
Router->>Telemetry: trackPageView(title, path)
activate Telemetry
Telemetry->>GTM: trackPageView(...)
GTM->>DataLayer: push page_view event
Telemetry->>Mixpanel: trackPageView(...)
deactivate Telemetry
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
benceruleanlu
added
the
needs-backport
benceruleanlu
force-pushed
the
fix/gtm-telemetry-entrypoint
branch
from
88677aa to
563cdbf
Compare
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/05/2026, 06:56:12 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.4 kB (baseline 22.5 kB) • 🟢 -104 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 852 kB (baseline 840 kB) • 🔴 +12.3 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • 🔴 +42 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 410 kB (baseline 410 kB) • 🔴 +81 BConfiguration panels, inspectors, and settings screens
Status: 23 added / 24 removed User & Accounts — 16.1 kB (baseline 16 kB) • 🔴 +80 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 3.57 kB (baseline 3.47 kB) • 🔴 +104 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 38 kB (baseline 37.8 kB) • 🔴 +205 BReusable component library chunks
Status: 10 added / 10 removed Data & Services — 2.06 MB (baseline 2.1 MB) • 🟢 -39.9 kBStores, services, APIs, and repositories
Status: 15 added / 13 removed Utilities & Hooks — 237 kB (baseline 234 kB) • 🔴 +2.7 kBHelpers, composables, and utility bundles
Status: 20 added / 19 removed Vendor & Third-Party — 8.77 MB (baseline 9.37 MB) • 🟢 -601 kBExternal libraries and shared vendor chunks
Status: 8 added / 8 removed Other — 7.1 MB (baseline 7.08 MB) • 🔴 +12.8 kBBundles that do not match a named category
Status: 125 added / 123 removed |
dosubot
bot
added
the
size:L
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D,
christian-byrne and
shinshin86
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates Google Tag Manager (GTM) initialization and data-layer event tracking behind the telemetry entrypoint to enable tree-shaking of GTM code in non-cloud builds. The implementation uses compile-time constants (__DISTRIBUTION__) and dynamic imports to ensure GTM dependencies are excluded from OSS builds while maintaining full functionality in cloud builds.
Changes:
- Created new
src/platform/telemetry/gtm.tsmodule with GTM initialization and event pushing logic - Updated
src/platform/telemetry/index.tsto exposeinitGtm()andpushDataLayerEvent()functions that dynamically import GTM only in cloud builds - Added subscription purchase tracking with localStorage-based pending purchase tracker
- Added sign-up tracking with SHA-256 hashed user IDs for email, Google, and GitHub authentication methods
- Added page view tracking on route navigation
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/platform/telemetry/index.ts |
Added GTM module loading with dynamic imports and cloud build guards |
src/platform/telemetry/gtm.ts |
New file containing GTM initialization and dataLayer event pushing logic |
src/main.ts |
Added GTM initialization call in cloud builds |
src/router.ts |
Added page view tracking on route changes |
src/stores/firebaseAuthStore.ts |
Added sign-up tracking with hashed user IDs |
src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts |
New utility for tracking pending subscription purchases via localStorage |
src/platform/cloud/subscription/utils/subscriptionCheckoutUtil.ts |
Added call to start subscription purchase tracking |
src/platform/cloud/subscription/composables/useSubscription.ts |
Added purchase event tracking when subscription completes |
src/platform/cloud/subscription/composables/useSubscription.test.ts |
Added test coverage for purchase tracking and updated test structure with effectScope |
global.d.ts |
Added TypeScript definition for window.dataLayer |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/cloud/subscription/composables/useSubscription.test.ts (1)
101-130: Potential test isolation issue:window.dataLayernot cleaned up between tests.The
mockPushDataLayerEventimplementation (lines 115-118) pushes towindow.dataLayer, and line 248 initializes it to an empty array. However, there's no cleanup ofwindow.dataLayerinafterEach, which could cause state leakage between tests.Also,
scope?.stop()on line 107 inbeforeEachis redundant sinceafterEachalready handles scope cleanup.🧹 Proposed fix: Add dataLayer cleanup and remove redundant stop
afterEach(() => { scope?.stop() scope = undefined + window.dataLayer = undefined }) beforeEach(() => { - scope?.stop() scope = effectScope() vi.clearAllMocks()
🤖 Fix all issues with AI agents
In `@src/platform/cloud/subscription/composables/useSubscription.ts`:
- Around line 242-247: The call to trackSubscriptionPurchase is synchronous
(returns void) but is being awaited; remove the unnecessary await in the try
block (replace "await trackSubscriptionPurchase(statusData)" with
"trackSubscriptionPurchase(statusData)") so the code correctly reflects
synchronous behavior while preserving the existing try/catch error handling
around the trackSubscriptionPurchase invocation.
In `@src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts`:
- Around line 12-13: Replace the hardcoded VALID_TIERS and VALID_CYCLES arrays
in subscriptionPurchaseTracker.ts by importing exported constants from the
source-of-truth files: add export const TIER_KEYS: TierKey[] = [...] to
tierPricing.ts and export const BILLING_CYCLES: BillingCycle[] = [...] to
subscriptionTierRank.ts, then remove the local definitions of VALID_TIERS and
VALID_CYCLES and import TIER_KEYS and BILLING_CYCLES into
subscriptionPurchaseTracker.ts (use these imported symbols wherever VALID_TIERS
and VALID_CYCLES were used, preserving types and behavior).
In `@src/platform/telemetry/gtm.ts`:
- Around line 8-37: Change initGtm from a void function to return the
initialization Promise so callers can await GTM readiness: have initGtm return
initPromise (Promise<void>) when created or return a resolved Promise if already
initialized; reference the existing symbols initGtm, initPromise, isInitialized
and finalize to locate the logic that creates and resolves the promise. Also
adjust the error handler on the script element (the listener that currently
calls finalize on 'error') to still resolve for telemetry resilience but emit a
development-only warning (e.g., when NODE_ENV !== 'production' or a dev flag) so
load failures are visible during debugging.
src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts
Outdated
Show resolved
Hide resolved
src/platform/telemetry/gtm.ts
Outdated
Comment on lines
8
to
37
| export function initGtm(): void { | ||
| if (!isCloud || typeof window === 'undefined') return | ||
| if (typeof document === 'undefined') return | ||
| if (isInitialized) return | ||
|
|
||
| if (!initPromise) { | ||
| initPromise = new Promise((resolve) => { | ||
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | ||
| dataLayer.push({ | ||
| 'gtm.start': Date.now(), | ||
| event: 'gtm.js' | ||
| }) | ||
|
|
||
| const script = document.createElement('script') | ||
| script.async = true | ||
| script.src = `https://www.googletagmanager.com/gtm.js?id=${GTM_CONTAINER_ID}` | ||
|
|
||
| const finalize = () => { | ||
| isInitialized = true | ||
| resolve() | ||
| } | ||
|
|
||
| script.addEventListener('load', finalize, { once: true }) | ||
| script.addEventListener('error', finalize, { once: true }) | ||
| document.head?.appendChild(script) | ||
| }) | ||
| } | ||
|
|
||
| void initPromise | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
initGtm returns void but internal promise is never exposed to callers.
The function creates initPromise but the void initPromise on line 36 is a no-op that simply suppresses the floating-promise lint. Callers cannot await GTM initialization completion. If callers need to wait for GTM to be ready before pushing events, consider returning the promise.
Additionally, script load errors are silently swallowed (line 31 resolves on error). This is acceptable for telemetry but consider logging a warning in development for debuggability.
♻️ Optional: Return promise for callers who need to await
-export function initGtm(): void {
+export function initGtm(): Promise<void> {
if (!isCloud || typeof window === 'undefined') return
+ return Promise.resolve()
if (typeof document === 'undefined') return
+ return Promise.resolve()
if (isInitialized) return
+ return Promise.resolve()
if (!initPromise) {
initPromise = new Promise((resolve) => {
// ... existing code ...
})
}
- void initPromise
+ return initPromise
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function initGtm(): void { | |
| if (!isCloud || typeof window === 'undefined') return | |
| if (typeof document === 'undefined') return | |
| if (isInitialized) return | |
| if (!initPromise) { | |
| initPromise = new Promise((resolve) => { | |
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | |
| dataLayer.push({ | |
| 'gtm.start': Date.now(), | |
| event: 'gtm.js' | |
| }) | |
| const script = document.createElement('script') | |
| script.async = true | |
| script.src = `https://www.googletagmanager.com/gtm.js?id=${GTM_CONTAINER_ID}` | |
| const finalize = () => { | |
| isInitialized = true | |
| resolve() | |
| } | |
| script.addEventListener('load', finalize, { once: true }) | |
| script.addEventListener('error', finalize, { once: true }) | |
| document.head?.appendChild(script) | |
| }) | |
| } | |
| void initPromise | |
| } | |
| export function initGtm(): Promise<void> { | |
| if (!isCloud || typeof window === 'undefined') return Promise.resolve() | |
| if (typeof document === 'undefined') return Promise.resolve() | |
| if (isInitialized) return Promise.resolve() | |
| if (!initPromise) { | |
| initPromise = new Promise((resolve) => { | |
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | |
| dataLayer.push({ | |
| 'gtm.start': Date.now(), | |
| event: 'gtm.js' | |
| }) | |
| const script = document.createElement('script') | |
| script.async = true | |
| script.src = `https://www.googletagmanager.com/gtm.js?id=${GTM_CONTAINER_ID}` | |
| const finalize = () => { | |
| isInitialized = true | |
| resolve() | |
| } | |
| script.addEventListener('load', finalize, { once: true }) | |
| script.addEventListener('error', finalize, { once: true }) | |
| document.head?.appendChild(script) | |
| }) | |
| } | |
| return initPromise | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/telemetry/gtm.ts` around lines 8 - 37, Change initGtm from a
void function to return the initialization Promise so callers can await GTM
readiness: have initGtm return initPromise (Promise<void>) when created or
return a resolved Promise if already initialized; reference the existing symbols
initGtm, initPromise, isInitialized and finalize to locate the logic that
creates and resolves the promise. Also adjust the error handler on the script
element (the listener that currently calls finalize on 'error') to still resolve
for telemetry resilience but emit a development-only warning (e.g., when
NODE_ENV !== 'production' or a dev flag) so load failures are visible during
debugging.
![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.
Reviewed commit: 2db667bde4
ℹ️ 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".
src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts
Outdated
Show resolved
Hide resolved
dosubot
bot
added
size:XL
Member
Author
|
The dist telemetry sniffing code can be made more strict by modifying the regex. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/telemetry/TelemetryRegistry.ts`:
- Around line 109-117: The current checkForCompletedTopup uses Array.some which
short-circuits and prevents later providers from running; change it to iterate
all providers (like the dispatch() pattern) using providers.forEach, call
provider.checkForCompletedTopup?.(events) inside a try/catch for each provider,
accumulate a boolean result (e.g., orAggregate = orAggregate || result) and
return that aggregated boolean at the end; ensure you still log errors in the
catch and reference the checkForCompletedTopup method and providers collection
when making the change.
🧹 Nitpick comments (1)
src/platform/cloud/subscription/composables/useSubscription.test.ts (1)
63-77: Replaceany[]with generics for type safety in the mock.The mock uses
any[]which violates the "Never useanytype" guideline. A small generic keeps type safety without changing behavior.♻️ Typed alternative
wrapWithErrorHandlingAsync: vi.fn( - (fn, errorHandler) => - async (...args: any[]) => { + <TArgs extends unknown[], TReturn>( + fn: (...args: TArgs) => Promise<TReturn> | TReturn, + errorHandler?: (error: unknown) => void + ) => + async (...args: TArgs) => { try { return await fn(...args) } catch (error) { if (errorHandler) { errorHandler(error)
Comment on lines
+109
to
+117
| checkForCompletedTopup(events: AuditLog[] | undefined | null): boolean { | ||
| return this.providers.some((provider) => { | ||
| try { | ||
| return provider.checkForCompletedTopup?.(events) ?? false | ||
| } catch (error) { | ||
| console.error('[Telemetry] Provider dispatch failed', error) | ||
| return false | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the full TelemetryRegistry.ts file to understand context
head -n 200 src/platform/telemetry/TelemetryRegistry.ts | tail -n +90Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3651
🏁 Script executed:
# Find all call sites of checkForCompletedTopup to understand expected behavior
rg -n "checkForCompletedTopup" --type ts -B 3 -A 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 8602
🏁 Script executed:
# Check if there are other similar dispatcher methods to understand the pattern
rg -n "this\.providers\.(some|forEach|map)" --type ts -B 2 -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1403
🏁 Script executed:
# View the dispatch method and context to confirm pattern
sed -n '45,55p' src/platform/telemetry/TelemetryRegistry.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 337
🏁 Script executed:
# Check all methods that call dispatch to confirm they use forEach pattern
rg "this\.dispatch\(" src/platform/telemetry/TelemetryRegistry.ts | wc -lRepository: Comfy-Org/ComfyUI_frontend
Length of output: 72
🏁 Script executed:
# Verify no other method uses some() with providers
rg "this\.providers\.(some|map)" src/platform/telemetry/TelemetryRegistry.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 116
Use forEach instead of some() to ensure all providers execute.
checkForCompletedTopup is the only method not using the dispatch() pattern with forEach, which calls every provider. Using some() stops after the first truthy result, preventing later providers from executing their check logic and updating state. Aggregate the boolean while calling all providers.
🛠️ Proposed fix
checkForCompletedTopup(events: AuditLog[] | undefined | null): boolean {
- return this.providers.some((provider) => {
- try {
- return provider.checkForCompletedTopup?.(events) ?? false
- } catch (error) {
- console.error('[Telemetry] Provider dispatch failed', error)
- return false
- }
- })
+ let completed = false
+ this.providers.forEach((provider) => {
+ try {
+ if (provider.checkForCompletedTopup?.(events)) {
+ completed = true
+ }
+ } catch (error) {
+ console.error('[Telemetry] Provider dispatch failed', error)
+ }
+ })
+ return completed
}🤖 Prompt for AI Agents
In `@src/platform/telemetry/TelemetryRegistry.ts` around lines 109 - 117, The
current checkForCompletedTopup uses Array.some which short-circuits and prevents
later providers from running; change it to iterate all providers (like the
dispatch() pattern) using providers.forEach, call
provider.checkForCompletedTopup?.(events) inside a try/catch for each provider,
accumulate a boolean result (e.g., orAggregate = orAggregate || result) and
return that aggregated boolean at the end; ensure you still log errors in the
catch and reference the checkForCompletedTopup method and providers collection
when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/telemetry/types.ts (1)
470-493:⚠️ Potential issue | 🟡 MinorMissing
PageViewMetadataandBeginCheckoutMetadatain union type.The
TelemetryEventPropertiesunion type does not include the newly addedPageViewMetadataandBeginCheckoutMetadatatypes. If this union is used for type-checking event properties elsewhere, it should include these new metadata types for completeness.🔧 Proposed fix
export type TelemetryEventProperties = | AuthMetadata | SurveyResponses | TemplateMetadata | ExecutionContext | RunButtonProperties | ExecutionErrorMetadata | ExecutionSuccessMetadata | CreditTopupMetadata | WorkflowImportMetadata | TemplateLibraryMetadata | TemplateLibraryClosedMetadata | PageVisibilityMetadata | TabCountMetadata | NodeSearchMetadata | NodeSearchResultMetadata | TemplateFilterMetadata | SettingChangedMetadata | UiButtonClickMetadata | HelpCenterOpenedMetadata | HelpResourceClickedMetadata | HelpCenterClosedMetadata | WorkflowCreatedMetadata | EnterLinearMetadata + | PageViewMetadata + | BeginCheckoutMetadata
🧹 Nitpick comments (1)
src/platform/cloud/subscription/components/PricingTable.test.ts (1)
205-233: Consider adding telemetry assertion for new subscriber flow.This test verifies new subscribers go through the checkout flow but doesn't assert on
trackBeginCheckout. SincesubscriptionCheckoutUtil.tsnow tracks checkout for new subscribers, consider adding an assertion to verify telemetry is called withcheckout_type: 'new'.💡 Suggested addition
expect(windowOpenSpy).toHaveBeenCalledWith( 'https://checkout.stripe.com/test', '_blank' ) + + expect(mockTrackBeginCheckout).toHaveBeenCalledWith({ + user_id: 'user-123', + tier: expect.any(String), + cycle: 'yearly', + checkout_type: 'new' + }) windowOpenSpy.mockRestore()
benceruleanlu
added
the
preview-cpu
simula-r
previously approved these changes
Feb 5, 2026
Contributor
simula-r
left a comment
simula-r
left a comment
There was a problem hiding this comment.
LGTM. Nice job on this.
benceruleanlu
dismissed
christian-byrne’s stale review
PR changed direction, last review was from Jake
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
christian-byrne
approved these changes
Feb 7, 2026
|
benceruleanlu
added a commit
that referenced
this pull request
Feb 7, 2026
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
5 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.
Wire checkout attribution into GTM events and checkout POST payloads.
This updates the cloud telemetry flow so the backend team can correlate checkout events without relying on frontend cookie parsing. We now surface GA4 identity via a GTM-provided global and include attribution on both
begin_checkouttelemetry and the checkout POST body. The backend should continue to derive the Firebase UID from the auth header; the checkout POST body does not include a user ID.GTM events pushed (unchanged list, updated payloads):
page_view(page title/location/referrer as before)sign_up/loginbegin_checkoutnow includes:user_id,tier,cycle,checkout_type,previous_tier(if change flow)ga_client_id,ga_session_id,ga_session_numbergclid,gbraid,wbraidBackend-facing change:
POST /customers/cloud-subscription-checkout/:tiernow includes a JSON body with attribution fields only:ga_client_id,ga_session_id,ga_session_numbergclid,gbraid,wbraidRequired GTM setup:
window.__ga_identity__via a GTM Custom HTML tag (after GA4/Google tag) with{ client_id, session_id, session_number }. The frontend reads this to populate the GA fields.Screenshots (if applicable)
Manual Testing:
To manually test, you will need to override api/features in devtools to also return this:
┆Issue is synchronized with this Notion page by Unito
Summary by CodeRabbit
New Features
Chores
Tests
CI