Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ interface Window {
badge?: string
}
}
dataLayer?: Array<Record<string, unknown>>
}

interface Navigator {
Expand Down
15 changes: 15 additions & 0 deletions src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ function getBasePath(): string {

const basePath = getBasePath()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Strip invite tokens from page_location before GTM push

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 👍 / 👎.

Comment on lines +44 to +46

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid sending user workflow titles to GTM

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 👍 / 👎.

})
}
Comment on lines +40 to +48
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


const router = createRouter({
history: isFileProtocol
? createWebHashHistory()
Expand Down Expand Up @@ -93,6 +104,10 @@ installPreservedQueryTracker(router, [
}
])

router.afterEach(() => {
pushPageView()
})

if (isCloud) {
const { flags } = useFeatureFlags()
const PUBLIC_ROUTE_NAMES = new Set([
Expand Down
32 changes: 32 additions & 0 deletions src/stores/firebaseAuthStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,31 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => {

const buildApiUrl = (path: string) => `${getComfyApiBaseUrl()}${path}`

const pushDataLayerEvent = (event: Record<string, unknown>) => {
if (!isCloud || typeof window === 'undefined') return
const dataLayer = window.dataLayer ?? (window.dataLayer = [])
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now just two callers

dataLayer.push(event)
}

const hashSha256 = async (value: string): Promise<string | undefined> => {
if (typeof crypto === 'undefined' || !crypto.subtle) return
const data = new TextEncoder().encode(value)
const hash = await crypto.subtle.digest('SHA-256', data)
return Array.from(new Uint8Array(hash))
.map((b) => b.toString(16).padStart(2, '0'))
.join('')
}

const trackSignUp = async (method: 'email' | 'google' | 'github') => {
const userId = currentUser.value?.uid
const hashedUserId = userId ? await hashSha256(userId) : undefined
pushDataLayerEvent({
event: 'sign_up',
method,
...(hashedUserId ? { user_id: hashedUserId } : {})
})
}

// Providers
const googleProvider = new GoogleAuthProvider()
googleProvider.addScope('email')
Expand Down Expand Up @@ -347,6 +372,7 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => {
method: 'email',
is_new_user: true
})
await trackSignUp('email')
}

return result
Expand All @@ -365,6 +391,9 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => {
method: 'google',
is_new_user: isNewUser
})
if (isNewUser) {
await trackSignUp('google')
}
}

return result
Expand All @@ -383,6 +412,9 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => {
method: 'github',
is_new_user: isNewUser
})
if (isNewUser) {
await trackSignUp('github')
}
}

return result
Expand Down
38 changes: 37 additions & 1 deletion vite.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ const DISTRIBUTION: 'desktop' | 'localhost' | 'cloud' =
? 'cloud'
: 'localhost'

const ENABLE_GTM =
process.env.ENABLE_GTM === 'true' || (!IS_DEV && DISTRIBUTION === 'cloud')
const GTM_CONTAINER_ID = 'GTM-NP9JM6K7'
const GTM_SCRIPT = `(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
})(window,document,'script','dataLayer','${GTM_CONTAINER_ID}');`
const GTM_NO_SCRIPT = `<iframe src="https://www.googletagmanager.com/ns.html?id=${GTM_CONTAINER_ID}" height="0" width="0" style="display:none;visibility:hidden"></iframe>`

// Nightly builds are from main branch; RC/stable builds are from core/* branches
// Can be overridden via IS_NIGHTLY env var for testing
const IS_NIGHTLY =
Expand Down Expand Up @@ -416,7 +426,33 @@ export default defineConfig({
}
})
]
: [])
: []),
// Google Tag Manager (cloud distribution only)
{
name: 'inject-gtm',
transformIndexHtml: {
order: 'post',
handler(html) {
if (!ENABLE_GTM) return html

return {
html,
tags: [
{
tag: 'script',
children: GTM_SCRIPT,
injectTo: 'head-prepend'
},
{
tag: 'noscript',
children: GTM_NO_SCRIPT,
injectTo: 'body-prepend'
}
]
}
}
}
}
],

build: {
Expand Down
Loading