Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request integrates Google Tag Manager (GTM) for analytics tracking across the application. It adds GTM initialization, dataLayer window property, and implements event tracking for page views, user sign-ups, and subscription purchases through both GTM and localStorage-based purchase tracking. Changes
Sequence DiagramssequenceDiagram
participant Browser
participant App
participant GTM as GTM Module
participant DataLayer
participant GTMScript as GTM Script
App->>GTM: initGtm()
GTM->>DataLayer: Create dataLayer[]
GTM->>Browser: Inject GTM script tag
Browser->>GTMScript: Load script
GTMScript->>DataLayer: Initialize GTM
GTMScript-->>GTM: Load complete
GTM-->>App: Promise resolved
sequenceDiagram
participant Browser as Browser/User
participant App as App Logic
participant Router as Router
participant DataLayer as GTM DataLayer
participant SubCheckout as Subscription Checkout
participant Storage as localStorage
participant SubStatus as Subscription Status
Browser->>Router: Navigate
Router->>Router: pushPageView()
Router->>DataLayer: page_view event
Browser->>SubCheckout: Initiate checkout
SubCheckout->>SubCheckout: Get checkout URL
SubCheckout->>Storage: startSubscriptionPurchaseTracking()
Storage-->>Storage: Store pending purchase
SubCheckout->>Browser: Open checkout URL
Browser->>SubStatus: Subscription completes (later)
SubStatus->>Storage: getPendingSubscriptionPurchase()
Storage-->>SubStatus: Return pending data
SubStatus->>DataLayer: purchase event (with price/tier)
SubStatus->>Storage: clearPendingSubscriptionPurchase()
Suggested reviewers
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 |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/27/2026, 08:26:39 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 23.5 kB (baseline 22.8 kB) • 🔴 +726 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 960 kB (baseline 960 kB) • 🔴 +103 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • 🔴 +36 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 470 kB (baseline 470 kB) • 🔴 +72 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 24 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.88 kB (baseline 2.83 kB) • 🔴 +48 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 9 added / 9 removed Data & Services — 3.2 MB (baseline 3.19 MB) • 🔴 +3.4 kBStores, services, APIs, and repositories
Status: 9 added / 9 removed Utilities & Hooks — 25.4 kB (baseline 25.2 kB) • 🔴 +265 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 8 added / 8 removed Other — 6.55 MB (baseline 6.55 MB) • 🔴 +2.43 kBBundles that do not match a named category
Status: 92 added / 89 removed |
dosubot
bot
added
the
size:M
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds Google Tag Manager (GTM) integration for cloud distribution builds to track user analytics. The implementation injects GTM scripts during the build process and tracks page views on route changes and signup events for new users.
Changes:
- GTM script injection in vite.config.mts for cloud builds with configurable enablement via environment variable
- Page view tracking in router.ts that pushes location and title data on route changes
- Signup event tracking in firebaseAuthStore.ts with privacy-preserving SHA-256 hashed user IDs for email, Google, and GitHub authentication methods
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vite.config.mts | Adds GTM script and noscript iframe injection via Vite plugin for cloud builds, with ENABLE_GTM flag for manual override |
| src/stores/firebaseAuthStore.ts | Implements signup event tracking with SHA-256 hashed user IDs and dataLayer helper functions |
| src/router.ts | Adds page_view event tracking on route changes using router.afterEach hook |
| global.d.ts | Adds TypeScript type definition for window.dataLayer array |
src/stores/firebaseAuthStore.ts
Outdated
Comment on lines
84
to
86
| const pushDataLayerEvent = (event: Record<string, unknown>) => { | ||
| if (!isCloud || typeof window === 'undefined') return | ||
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) |
There was a problem hiding this comment.
The dataLayer initialization pattern window.dataLayer ?? (window.dataLayer = []) is duplicated between this file and src/router.ts:42. Consider extracting this into a shared utility function to maintain consistency and reduce duplication. For example, create a helper function like getDataLayer() that both files can import.
Suggested change
| const pushDataLayerEvent = (event: Record<string, unknown>) => { | |
| if (!isCloud || typeof window === 'undefined') return | |
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | |
| const getDataLayer = (): unknown[] | undefined => { | |
| if (!isCloud || typeof window === 'undefined') return | |
| // Initialize dataLayer if it does not exist yet | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| const dataLayer = (window as any).dataLayer ?? ((window as any).dataLayer = []) | |
| return dataLayer | |
| } | |
| const pushDataLayerEvent = (event: Record<string, unknown>) => { | |
| const dataLayer = getDataLayer() | |
| if (!dataLayer) return |
Member
Author
There was a problem hiding this comment.
I think it's fine for now just two callers
![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: bb0abe7417
ℹ️ 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/router.ts
Outdated
Comment on lines
42
to
46
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | ||
| dataLayer.push({ | ||
| event: 'page_view', | ||
| page_location: window.location.href, | ||
| page_title: document.title |
There was a problem hiding this comment.
Because pushPageView sends window.location.href as page_location, any invite tokens in the query string are forwarded to GTM. The cloud invite flow explicitly accepts /?invite=TOKEN and only removes it later in useInviteUrlLoader (so the token is still present on the first navigation), which means the raw invite token will be sent to third‑party analytics on the initial page_view. To avoid leaking sensitive invite codes, consider sanitizing page_location (e.g., remove invite and other preserved query params) before pushing to dataLayer.
Useful? React with 👍 / 👎.
![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: 1
🤖 Fix all issues with AI agents
In `@src/router.ts`:
- Around line 39-48: pushPageView may read document.title too early when called
from router.afterEach causing the old title to be pushed; change pushPageView
(and its caller in router.afterEach) to prefer a provided title param (e.g.,
to.meta.title) and/or defer reading document.title via Vue.nextTick before
building the dataLayer object so the new page title is available. Update the
pushPageView signature to accept an optional title and in router.afterEach pass
to.meta.title if present, otherwise call pushPageView inside nextTick to read
document.title.
Comment on lines
39
to
48
| function pushPageView(): void { | ||
| if (!isCloud) return | ||
|
|
||
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | ||
| dataLayer.push({ | ||
| event: 'page_view', | ||
| page_location: window.location.href, | ||
| page_title: document.title | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Potential timing issue with document.title.
When pushPageView is called in router.afterEach, document.title may still reflect the previous page's title since Vue Router's navigation completes before the new component is mounted and updates the title.
Consider using to.meta.title if available, or deferring the push with nextTick to allow the DOM to update:
🔧 Suggested fix using nextTick
+import { nextTick } from 'vue'
+
function pushPageView(): void {
if (!isCloud) return
- const dataLayer = window.dataLayer ?? (window.dataLayer = [])
- dataLayer.push({
- event: 'page_view',
- page_location: window.location.href,
- page_title: document.title
+ nextTick(() => {
+ const dataLayer = window.dataLayer ?? (window.dataLayer = [])
+ dataLayer.push({
+ event: 'page_view',
+ page_location: window.location.href,
+ page_title: document.title
+ })
})
}📝 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
| function pushPageView(): void { | |
| if (!isCloud) return | |
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | |
| dataLayer.push({ | |
| event: 'page_view', | |
| page_location: window.location.href, | |
| page_title: document.title | |
| }) | |
| } | |
| import { nextTick } from 'vue' | |
| function pushPageView(): void { | |
| if (!isCloud) return | |
| nextTick(() => { | |
| const dataLayer = window.dataLayer ?? (window.dataLayer = []) | |
| dataLayer.push({ | |
| event: 'page_view', | |
| page_location: window.location.href, | |
| page_title: document.title | |
| }) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@src/router.ts` around lines 39 - 48, pushPageView may read document.title too
early when called from router.afterEach causing the old title to be pushed;
change pushPageView (and its caller in router.afterEach) to prefer a provided
title param (e.g., to.meta.title) and/or defer reading document.title via
Vue.nextTick before building the dataLayer object so the new page title is
available. Update the pushPageView signature to accept an optional title and in
router.afterEach pass to.meta.title if present, otherwise call pushPageView
inside nextTick to read document.title.
Member
Author
There was a problem hiding this comment.
ahh, we'll see what marketing/gtm wants. we pagetitle is practically only ever going to be ComfyUI because we only change it for workflow name, unsaved indicator, and node/progress status.
Contributor
There was a problem hiding this comment.
@benceruleanlu, understood! That makes sense—if the title is mostly static, the timing issue becomes less critical. It's good to verify with your marketing/GTM team whether they need route-specific titles or if the current approach works for their tracking needs. Feel free to follow up if they have specific requirements! 👍
✏️ Learnings added
Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 8311
File: src/router.ts:39-48
Timestamp: 2026-01-26T20:01:11.087Z
Learning: In ComfyUI, the page title (document.title) is mostly static and remains "ComfyUI" throughout the application. It only changes for workflow name, unsaved indicator, and node/progress status, not for different routes or pages.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Member
Author
|
During the frontend standup, the team (Christian and Jacob) wanted that no line of this Google related analytics code to be available on the local build, and proposed injecting this during build time cloud-side. Or just making sure it's fully tree shaken for local builds. |
Member
Author
|
Because iscloud is gating, this won't be available in the code in local builds, but will still be present in sorucemaps. I don't think there's much we can do about its presence in sourcemaps. |
dosubot
bot
removed
the
size:M
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb0abe7417
ℹ️ 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".
Comment on lines
+44
to
+46
| event: 'page_view', | ||
| page_location: window.location.href, | ||
| page_title: document.title |
There was a problem hiding this comment.
pushPageView forwards document.title as page_title. In this app, document.title is derived from the active workflow filename and node execution text via useBrowserTabTitle (see src/composables/useBrowserTabTitle.ts), which are user-provided values. On cloud builds this will send workflow names and progress text to GTM/GA on every route change, which can leak sensitive user content into analytics. Consider omitting page_title or replacing it with a sanitized/static value such as the route name before pushing to dataLayer.
Useful? React with 👍 / 👎.
benceruleanlu
added
needs-backport
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/platform/cloud/subscription/composables/useSubscription.test.ts`:
- Around line 214-221: Replace the hardcoded string
'pending_subscription_purchase' in the test with the exported STORAGE_KEY from
subscriptionPurchaseTracker to avoid key drift; import STORAGE_KEY from
"src/platform/cloud/subscription/utils/subscriptionPurchaseTracker" at the top
of useSubscription.test.ts and use STORAGE_KEY in the localStorage.setItem call
(the test code block around localStorage.setItem(...) and the STORAGE_KEY symbol
identify where to change).
In `@src/platform/cloud/subscription/composables/useSubscription.ts`:
- Around line 104-145: The three utilities—buildApiUrl, pushDataLayerEvent, and
trackSubscriptionPurchase—are currently declared as const arrow functions;
change each to a standard function declaration (e.g., function buildApiUrl(path:
string) { ... }) to follow the project's TypeScript guideline against function
expressions; preserve all current logic, parameter types, and surrounding scope
(including use of getComfyApiBaseUrl, __GTM_ENABLED__, window/dataLayer access,
t, getTierPrice, getPendingSubscriptionPurchase,
clearPendingSubscriptionPurchase, and CloudSubscriptionStatusResponse) so
behavior and references remain identical.
In `@src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts`:
- Around line 32-68: Move the localStorage.getItem(STORAGE_KEY) call inside the
existing try block in getPendingSubscriptionPurchase and wrap both getItem and
JSON.parse with a try/catch so any storage exceptions are handled; create a
small helper function safeRemove() that calls
localStorage.removeItem(STORAGE_KEY) inside a try/catch and use safeRemove in
all places that currently call localStorage.removeItem (including the catch in
getPendingSubscriptionPurchase and inside clearPendingSubscriptionPurchase);
keep all existing validation logic (VALID_TIERS, VALID_CYCLES, MAX_AGE_MS) and
return behavior unchanged, only adding the guarded reads/removals to prevent
exceptions in restricted storage modes.
In `@src/stores/firebaseAuthStore.ts`:
- Around line 84-107: The GTM tracking helpers (pushDataLayerEvent, hashSha256,
trackSignUp) must be converted to function declarations and made best-effort so
they never block auth: add an early guard in trackSignUp that returns
immediately if !__GTM_ENABLED__ or typeof window === 'undefined', wrap the body
of trackSignUp in try-catch to swallow and log any errors so failures don’t
reject login/register promises, add a TextEncoder existence check in hashSha256
(return undefined if missing) and ensure pushDataLayerEvent similarly guards for
__GTM_ENABLED__ and window before accessing window.dataLayer; update references
to these functions accordingly.
In `@vite.config.mts`:
- Around line 54-63: Change ENABLE_GTM so GTM can only be active for cloud
builds and never in local/dev; compute it as true only when DISTRIBUTION ===
'cloud' and !IS_DEV, and allow explicit opt‑out via process.env.ENABLE_GTM ===
'false' but do not allow process.env.ENABLE_GTM === 'true' to force GTM on
non‑cloud builds—update the ENABLE_GTM expression (which currently uses
ENABLE_GTM, DISTRIBUTION, IS_DEV and process.env.ENABLE_GTM) to enforce
"cloud-only" and "no GA in dev" semantics while still honoring an explicit
opt‑out.
src/platform/cloud/subscription/composables/useSubscription.test.ts
Outdated
Show resolved
Hide resolved
src/platform/cloud/subscription/utils/subscriptionPurchaseTracker.ts
Outdated
Show resolved
Hide resolved
Contributor
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 (1)
src/platform/cloud/subscription/composables/useSubscription.test.ts (1)
76-92: Add cleanup forwindow.dataLayerandlocalStorageinbeforeEach.The test at line 210 sets
window.dataLayer = [], butbeforeEachdoesn't reset it. This could cause test pollution if tests run in different orders or if previous tests leave data behind.♻️ Proposed fix
beforeEach(() => { vi.clearAllMocks() mockIsLoggedIn.value = false mockTelemetry.trackSubscription.mockReset() mockTelemetry.trackMonthlySubscriptionCancelled.mockReset() + window.dataLayer = undefined + localStorage.removeItem('pending_subscription_purchase') window.__CONFIG__ = { subscription_required: true } as typeof window.__CONFIG__
🤖 Fix all issues with AI agents
In `@src/platform/cloud/subscription/composables/useSubscription.ts`:
- Line 238: The call to trackSubscriptionPurchase(statusData) should be wrapped
in a try-catch so analytics errors cannot interrupt the subscription status
flow; locate the invocation of trackSubscriptionPurchase in useSubscription.ts
and surround it with a try { await trackSubscriptionPurchase(statusData) } catch
(err) { /* log error via existing logger or console.error without rethrowing */
} ensuring you await it if it returns a promise and do not let the catch rethrow
so the main fetch flow continues.
In `@src/platform/telemetry/gtm.ts`:
- Around line 25-32: The current finalize handler marks isInitialized = true for
both load and error events and swallows failures; change this so the 'load'
event sets isInitialized = true and resolves, while the 'error' event uses a
separate handler that logs the failure (e.g., via console.warn or your
telemetry/logger) including the Event/error details and then resolves (still
best-effort). Update the event wiring from using finalize for both events to use
finalize (or onLoad) for 'load' and onError (or finalizeError) for 'error',
ensuring you still resolve the initialization promise but do not falsely mark
success without logging; reference isInitialized, finalize, and the
script.addEventListener(...) calls to locate where to change.
Comment on lines
+25
to
+32
| const finalize = () => { | ||
| isInitialized = true | ||
| resolve() | ||
| } | ||
|
|
||
| script.addEventListener('load', finalize, { once: true }) | ||
| script.addEventListener('error', finalize, { once: true }) | ||
| document.head?.appendChild(script) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Script load errors are silently swallowed.
When GTM script fails to load (e.g., network error, blocked by ad-blocker), finalize still sets isInitialized = true. This silently succeeds, which is reasonable for best-effort analytics, but consider logging the error for observability.
♻️ Optional: Add error logging
const finalize = () => {
isInitialized = true
resolve()
}
script.addEventListener('load', finalize, { once: true })
- script.addEventListener('error', finalize, { once: true })
+ script.addEventListener('error', (e) => {
+ console.warn('GTM script failed to load:', e)
+ finalize()
+ }, { once: true })
document.head?.appendChild(script)📝 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
| const finalize = () => { | |
| isInitialized = true | |
| resolve() | |
| } | |
| script.addEventListener('load', finalize, { once: true }) | |
| script.addEventListener('error', finalize, { once: true }) | |
| document.head?.appendChild(script) | |
| const finalize = () => { | |
| isInitialized = true | |
| resolve() | |
| } | |
| script.addEventListener('load', finalize, { once: true }) | |
| script.addEventListener('error', (e) => { | |
| console.warn('GTM script failed to load:', e) | |
| finalize() | |
| }, { once: true }) | |
| document.head?.appendChild(script) |
🤖 Prompt for AI Agents
In `@src/platform/telemetry/gtm.ts` around lines 25 - 32, The current finalize
handler marks isInitialized = true for both load and error events and swallows
failures; change this so the 'load' event sets isInitialized = true and
resolves, while the 'error' event uses a separate handler that logs the failure
(e.g., via console.warn or your telemetry/logger) including the Event/error
details and then resolves (still best-effort). Update the event wiring from
using finalize for both events to use finalize (or onLoad) for 'load' and
onError (or finalizeError) for 'error', ensuring you still resolve the
initialization promise but do not falsely mark success without logging;
reference isInitialized, finalize, and the script.addEventListener(...) calls to
locate where to change.
Myestery
approved these changes
Jan 27, 2026
| await import('@/platform/remoteConfig/refreshRemoteConfig') | ||
| await refreshRemoteConfig({ useAuth: false }) | ||
|
|
||
| const { initGtm } = await import('@/platform/telemetry/gtm') |
Member
Author
|
I think the failing tests is part of the CI being broken on main? The snapshot seems to actually be wrong, and the actual version is right. 
Going to merge |
Member
Author
|
Well, it passed on retry, but it's definitely a bad snapshot... We can fix it later |
|
benceruleanlu
added
preview
preview-cpu
|
1 similar comment
|
benceruleanlu
added a commit
that referenced
this pull request
Jan 28, 2026
This reverts commit 788f508.
This was referenced Jan 28, 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
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
Add GTM injection for cloud distribution builds and push SPA page view + signup events.
Changes
page_viewtodataLayeron cloud route changes (page_location + page_title)sign_uptodataLayerafter successful account creation (email/google/github)Review Focus
Screenshots (if applicable)