[backport cloud/1.38] fix: route gtm through telemetry entrypoint#8714
[backport cloud/1.38] fix: route gtm through telemetry entrypoint#8714benceruleanlu wants to merge 1 commit intocloud/1.38from
Conversation
dosubot
bot
added
the
size:XXL
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/07/2026, 10:24:20 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
There was a problem hiding this comment.
Pull request overview
Backport to cloud/1.38 that extends the cloud telemetry pipeline to route GTM through a centralized telemetry entrypoint, and enriches checkout/auth events with GA identity + attribution so backend can correlate events without frontend cookie parsing.
Changes:
- Introduces a telemetry registry/dispatcher with cloud-only initialization, registering Mixpanel + new GTM provider.
- Adds checkout attribution capture (GA identity + click IDs) and includes it in both
begin_checkouttelemetry and checkout POST bodies. - Refactors
ComfyWorkflowinto its own module and updates related imports.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stores/subgraphStore.ts | Updates workflow type/class imports after ComfyWorkflow extraction. |
| src/stores/firebaseAuthStore.ts | Adds user_id to auth telemetry metadata for cloud builds. |
| src/router.ts | Adds SPA page_view telemetry on route changes (cloud only). |
| src/platform/workflow/management/stores/workflowStore.ts | Removes embedded ComfyWorkflow implementation; re-exports from new module. |
| src/platform/workflow/management/stores/comfyWorkflow.ts | New ComfyWorkflow module (with lazy imports for some dependencies). |
| src/platform/telemetry/utils/checkoutAttribution.ts | New utility to read/persist click IDs and read GA identity from window.__ga_identity__. |
| src/platform/telemetry/utils/tests/checkoutAttribution.test.ts | Adds tests for checkout attribution behavior. |
| src/platform/telemetry/types.ts | Extends telemetry types (page view + begin checkout + optional provider methods) and adds dispatcher type. |
| src/platform/telemetry/providers/cloud/GtmTelemetryProvider.ts | New GTM provider pushing events to dataLayer and loading GTM script when configured. |
| src/platform/telemetry/initTelemetry.ts | Cloud-only telemetry initialization (registry + providers). |
| src/platform/telemetry/index.ts | Replaces singleton provider with registry setter/getter (useTelemetry). |
| src/platform/telemetry/TelemetryRegistry.ts | New dispatcher that fans out telemetry calls to registered providers. |
| src/platform/remoteConfig/types.ts | Adds gtm_container_id to runtime remote config typing. |
| src/platform/cloud/subscription/utils/subscriptionCheckoutUtil.ts | Adds attribution JSON body to checkout POST + tracks begin_checkout. |
| src/platform/cloud/subscription/utils/subscriptionCheckoutUtil.test.ts | Adds tests for attribution payload + begin_checkout tracking. |
| src/platform/cloud/subscription/composables/useSubscriptionCancellationWatcher.ts | Updates telemetry typing to dispatcher. |
| src/platform/cloud/subscription/composables/useSubscriptionCancellationWatcher.test.ts | Updates telemetry typing to dispatcher. |
| src/platform/cloud/subscription/composables/useSubscription.ts | Minor refactors (store reference + function style). |
| src/platform/cloud/subscription/composables/useSubscription.test.ts | Updates test harness to use effect scopes + updated mocks. |
| src/platform/cloud/subscription/components/PricingTable.vue | Tracks begin_checkout for plan changes and routes new checkout through shared util. |
| src/platform/cloud/subscription/components/PricingTable.test.ts | Adds expectation for begin_checkout tracking on plan change flow. |
| src/platform/cloud/onboarding/CloudSubscriptionRedirectView.test.ts | Adjusts mocked subscription composable shape. |
| src/main.ts | Cloud-only remote config load now also initializes telemetry before app mount. |
| global.d.ts | Adds gtm_container_id, __ga_identity__, and dataLayer to Window typings. |
| .github/workflows/ci-dist-telemetry-scan.yaml | Adds CI job to ensure dist artifacts don’t contain GTM references. |
| if (userId) { | ||
| telemetry?.trackBeginCheckout({ | ||
| user_id: userId, |
There was a problem hiding this comment.
userId from useFirebaseAuthStore() is a computed ref (ComputedRef<string | undefined>) in the real store, so this if (userId) check will always be truthy and user_id: userId will send the ref object instead of the UID string. Use userId.value (or storeToRefs) for both the guard and the user_id field.
| if (userId) { | |
| telemetry?.trackBeginCheckout({ | |
| user_id: userId, | |
| if (userId?.value) { | |
| telemetry?.trackBeginCheckout({ | |
| user_id: userId.value, |
| if (userId) { | ||
| telemetry?.trackBeginCheckout({ | ||
| user_id: userId, |
There was a problem hiding this comment.
userId from useFirebaseAuthStore() is a computed ref in the actual Pinia setup store. As written, if (userId) will always pass and user_id: userId will push a ref object to telemetry. Use userId.value (or storeToRefs(useFirebaseAuthStore())) when checking and when populating user_id.
| if (userId) { | |
| telemetry?.trackBeginCheckout({ | |
| user_id: userId, | |
| if (userId?.value) { | |
| telemetry?.trackBeginCheckout({ | |
| user_id: userId.value, |
| useFirebaseAuthStore: () => ({ | ||
| getFirebaseAuthHeader: mockGetFirebaseAuthHeader | ||
| getFirebaseAuthHeader: mockGetFirebaseAuthHeader, | ||
| userId: 'user-123' |
There was a problem hiding this comment.
This test mocks userId as a plain string, but the real useFirebaseAuthStore().userId is a computed ref. That mismatch makes it easy for ref/value bugs to slip through; mock userId as a ref/computed and assert the implementation reads userId.value.
| userId: 'user-123' | |
| userId: computed(() => 'user-123') |
| vi.mock('@/stores/firebaseAuthStore', () => ({ | ||
| useFirebaseAuthStore: vi.fn(() => ({ | ||
| getFirebaseAuthHeader: mockGetAuthHeader, | ||
| get userId() { | ||
| return mockUserId.value | ||
| } | ||
| })), |
There was a problem hiding this comment.
This test's useFirebaseAuthStore() mock exposes userId as a getter returning a string, but the real store returns a computed ref. Mocking it as a ref/computed will better match runtime behavior and will catch ref/value issues in the checkout telemetry code.
| const { useDialogService } = await import('@/services/dialogService') | ||
| return await useDialogService().prompt({ | ||
| title: t('workflowService.saveWorkflow'), | ||
| message: t('workflowService.enterFilenamePrompt'), |
There was a problem hiding this comment.
workflowService.enterFilenamePrompt doesn't exist in the locale files (only workflowService.enterFilename is defined). This will render the raw i18n key in the prompt. Either switch back to t('workflowService.enterFilename') + ':' to match existing usage, or add the new enterFilenamePrompt key to all locales.
| message: t('workflowService.enterFilenamePrompt'), | |
| message: t('workflowService.enterFilename') + ':', |
| await import('@/platform/workflow/persistence/stores/workflowDraftStore') | ||
| const draftStore = useWorkflowDraftStore() | ||
| this.content = JSON.stringify(this.activeState) | ||
| // Force save to ensure the content is updated in remote storage incase |
There was a problem hiding this comment.
Typo in comment: "incase" should be "in case".
| // Force save to ensure the content is updated in remote storage incase | |
| // Force save to ensure the content is updated in remote storage in case |
![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: d651214a31
ℹ️ 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".
| const { useDialogService } = await import('@/services/dialogService') | ||
| return await useDialogService().prompt({ | ||
| title: t('workflowService.saveWorkflow'), | ||
| message: t('workflowService.enterFilenamePrompt'), |
There was a problem hiding this comment.
promptSave() now calls t('workflowService.enterFilenamePrompt'), but this key is not defined in the locale bundle on this branch (src/locales/en/main.json only defines workflowService.enterFilename), so users will see the raw i18n key in the save dialog instead of a readable prompt whenever they save/rename workflows.
Useful? React with 👍 / 👎.
| -e '(?i)googletagmanager\.com/gtm\.js\\?id=' \ | ||
| -e '(?i)googletagmanager\.com/ns\.html\\?id=' \ |
There was a problem hiding this comment.
The new scan patterns use \?id= (googletagmanager\.com/...\\?id=), which in ripgrep regex does not match a literal ?id= URL query; as a result, GTM URL references like .../ns.html?id=... can slip past CI undetected. This weakens the guardrail the workflow is intended to enforce for telemetry-free dist assets.
Useful? React with 👍 / 👎.
|
Waiting on BE implementation |
2 participants
Backport of #8354 to
cloud/1.38.Conflict resolution:
src/platform/workflow/management/stores/workflowStore.ts: kept the PR-sideComfyWorkflowextraction tocomfyWorkflow.tsand retained target-branch behavior for the rest of the store.