Add CI check for telemetry tree-shaking validation#8623
Add CI check for telemetry tree-shaking validation#8623comfy-pr-bot wants to merge 3 commits intomainfrom
Conversation
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/06/2026, 02:19:46 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
📝 WalkthroughWalkthroughAdds a CI workflow to validate telemetry code is tree-shaken from builds, plus a deliberate telemetry violation in source (new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.5 kB (baseline 22.5 kB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 840 kB (baseline 840 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 410 kB (baseline 410 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Editors & Dialogs — 3.47 kB (baseline 3.47 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
UI Components — 37.8 kB (baseline 37.8 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 2.1 MB (baseline 2.1 MB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 234 kB (baseline 234 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 9.37 MB (baseline 9.37 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.1 MB (baseline 7.1 MB) • ⚪ 0 BBundles that do not match a named category
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci-telemetry-tree-shaking.yaml:
- Around line 23-35: The script currently assumes dist/ exists which can cause
false passes; add an explicit existence check before the KEYWORDS loop (e.g.,
test for directory existence with [ -d dist ] or similar) and fail fast (echo a
clear error and exit non‑zero) if dist/ is missing or empty; update the block
around the KEYWORDS array and the for loop (referencing KEYWORDS and the grep -r
-i "$keyword" dist/ usage) so the script aborts immediately when dist/ is not
present.
|
Fixed in commit 0d1ebbac9:
The check now passes on the current codebase, as GTM was properly removed in PR #8353. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci-telemetry-tree-shaking.yaml:
- Around line 47-53: The grep block is currently only matching single-quoted
gtag patterns (e.g. "gtag('config'"/"gtag('event'") and should accept either
single or double quotes; update the -e patterns used in the grep invocation that
reference "gtag('config'" and "gtag('event'" to a quote-agnostic BRE/ERE such as
matching gtag\( ['"]? config/event (i.e. allow either ' or " after the opening
parenthesis) and remove the now-unused --exclude='mixpanel.module-*.js' from the
grep command; target the existing grep invocation and the two gtag pattern
strings to implement this change.
|
✅ Addressed in commit 239e398 Updated the grep patterns to be quote-agnostic:
The pattern gtag(.config uses BRE syntax where . matches any single character, so it will catch both gtag('config' and gtag("config") regardless of how the minifier normalizes quotes. |
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 (5)
src/locales/fa/nodeDefs.json (1)
6444-6461:⚠️ Potential issue | 🔴 CriticalFix schema inconsistency in FA locale Load3D entries.
The FA locale has three entries using empty-string keys (
"": "..."):
"clear""upload 3d model""upload extra resources"Other fields in the same Load3D node use
"name"keys, and no other locale (EN, ES, ZH) uses empty-string keys anywhere. This schema mismatch will likely prevent proper label rendering in the UI. Change these entries to use"name"keys to match the rest of the structure:"clear": { "name": "پاکسازی" }, "upload 3d model": { "name": "بارگذاری مدل سهبعدی" }, "upload extra resources": { "name": "بارگذاری منابع اضافی" }src/locales/ko/nodeDefs.json (1)
6435-6446:⚠️ Potential issue | 🟡 MinorWrap string values as objects to maintain locale schema consistency.
Lines 6435, 6445-6446 use string values instead of the
{ "name": ... }object format used for other input labels in the same node and throughout the English reference. The English nodeDefs.json uses empty objects{}for these entries, while other entries like "model_file", "image", "width", and "height" consistently use{ "name": "..." }format. These three entries should follow the same pattern:🛠️ Suggested fix
- "clear": "지우기", + "clear": { "name": "지우기" }, @@ - "upload 3d model": "3D 모델 업로드", - "upload extra resources": "추가 리소스 업로드", + "upload 3d model": { "name": "3D 모델 업로드" }, + "upload extra resources": { "name": "추가 리소스 업로드" },src/locales/es/nodeDefs.json (1)
6435-6449:⚠️ Potential issue | 🟡 MinorUse the standard
namefield for Load3D labels.Line 6435 and Lines 6445-6449 use
{}/{ "es": ... }, while other inputs in this file use{ "name": ... }. If the UI reads onlyname, these labels may be ignored or fall back to English.🛠️ Suggested alignment to the existing schema
- "clear": {}, + "clear": { + "name": "limpiar" + }, ... - "upload 3d model": { - "es": "Subir modelo 3D" - }, + "upload 3d model": { + "name": "Subir modelo 3D" + }, - "upload extra resources": { - "es": "Subir recursos adicionales" - }, + "upload extra resources": { + "name": "Subir recursos adicionales" + },src/renderer/extensions/vueNodes/widgets/components/WidgetInputNumberInput.vue (1)
240-240:⚠️ Potential issue | 🟠 MajorDecrement/increment button clicks bypass clamping logic.
The
@clickhandlers on lines 240 and 298 directly modifymodelValuewithout clamping, unlikeupdateValueBy(line 152) andupdateValue(line 70) which both clamp to[min, max]. IfmodelValueis within onestepValueof the boundary, these clicks will push the value pastmin/max.Proposed fix: use updateValueBy for button clicks
- `@click`="modelValue -= stepValue" + `@click`="updateValueBy(-stepValue)"- `@click`="modelValue += stepValue" + `@click`="updateValueBy(stepValue)"Also applies to: 298-298
src/locales/ar/nodeDefs.json (1)
13957-13999:⚠️ Potential issue | 🟡 MinorLocalize new parameter labels for Arabic UI (cfg_scale/top_k/top_p).
Thesenamevalues remain raw keys while adjacent fields are translated, which will surface mixed-language labels in the UI.🔧 Suggested localization tweak
"cfg_scale": { - "name": "cfg_scale" + "name": "مقياس CFG" }, @@ "top_k": { - "name": "top_k" + "name": "أعلى K" }, "top_p": { - "name": "top_p" + "name": "أعلى P" }
🤖 Fix all issues with AI agents
In `@src/composables/graph/useGraphNodeManager.ts`:
- Line 173: The current cast of widget.options to IWidgetOptions<unknown[]>
hides a generic mismatch; update the code to preserve the options type by making
getSharedWidgetEnhancements generic (e.g. add a type parameter T extends
IWidgetOptions<unknown> and change the widget parameter to IBaseWidget<any, any,
T>) so SharedWidgetEnhancements receives the correct options type, or
alternatively change SharedWidgetEnhancements.options to IWidgetOptions<unknown>
to match the incoming widget.options; locate getSharedWidgetEnhancements,
SharedWidgetEnhancements, IWidgetOptions and IBaseWidget in the file and apply
one of these two fixes to remove the unsafe type assertion.
In `@src/platform/settings/settingStore.ts`:
- Around line 119-122: The get function uses a non-null assertion on
getDefaultValue(key) which can be undefined; update get (and references to
settingValues.value) to remove the `!` and explicitly handle the undefined path:
compute const val = settingValues.value[key] ?? getDefaultValue(key); if val is
undefined, either throw a clear error (or a dev-mode assertion) describing the
missing registered SettingParams/defaultValue for that key, otherwise return
_.cloneDeep(val) as Settings[K]; reference function get and getDefaultValue to
locate where to add the guard so callers never receive a lied-about non-null
value.
In
`@src/renderer/extensions/vueNodes/widgets/components/audio/AudioPreviewPlayer.vue`:
- Around line 208-220: The try/catch in handleDownload won't catch async errors
because downloadFile currently returns void and swallows errors inside
downloadViaBlobFetch; update the call chain so errors propagate: change
downloadFile (and/or where it calls downloadViaBlobFetch) to return a Promise
and rethrow or let the fetch error bubble up (remove the internal .catch that
only logs), then await downloadFile(modelValue.value) inside handleDownload so
the catch block can trigger toast.add on failure; reference downloadFile,
downloadViaBlobFetch, and handleDownload to locate the changes.
🧹 Nitpick comments (13)
src/platform/workflow/persistence/composables/useWorkflowPersistence.test.ts (1)
166-168: Consider usingWorkflowDraftSnapshothere too for consistency.Line 168 uses an inline type
{ data: string; isTemporary: boolean }which is a subset ofWorkflowDraftSnapshot. Since you've already imported the type (line 7) and applied it on line 195, this cast could be unified as well.♻️ Suggested change
const drafts = JSON.parse( localStorage.getItem('Comfy.Workflow.Drafts') ?? '{}' - ) as Record<string, { data: string; isTemporary: boolean }> + ) as Record<string, WorkflowDraftSnapshot>src/services/litegraphService.ts (1)
875-886: Pre-existing@ts-expect-errorsuppressions — not introduced by this change.Lines 883 and 885 suppress type errors on
graph.add(node)andreturn node. These are pre-existing and out of scope for this PR, but worth noting as future cleanup targets.src/services/load3dService.ts (1)
167-175: Type assertionas ReturnType<T>is pragmatic but introduces a soundness gap.The generic signature is a nice improvement for call-site ergonomics. Two observations:
Mixed-caller scenario: Both
getOrCreateViewer(async, line 153) andgetOrCreateViewerSyncshare the sameviewerInstancesmap. If the async version creates an instance first (typed asReturnType<UseLoad3dViewerFn>), a subsequentgetOrCreateViewerSync<T>call for the samenode.idreturns the existing instance cast toReturnType<T>, which may not match the actual runtime type whenTis a narrower subtype.Same node, different
T: If two callers invokegetOrCreateViewerSyncwith differentTfor the same node, the second caller silently receives an instance created by the first, cast to the wrong type.Neither is likely to bite in practice today, but consider documenting the invariant (one viewer factory per node) or using a runtime guard.
.github/workflows/ci-telemetry-tree-shaking.yaml (1)
13-21: Consider adding explicitpermissionsto the job.The job only needs read access, but without an explicit
permissionsblock it inherits the repository's default token permissions (which may be broader than needed).🛡️ Suggested fix
jobs: validate-tree-shaking: runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v6src/locales/fa/nodeDefs.json (1)
13976-14017: Optional: localize the new technical labels for consistency.
Most fields in this node are Persian;cfg_scale,temperature,top_k,top_premain English. If full localization is desired, consider translating or adding Persian tooltips.scripts/diff-i18n.ts (1)
78-78:console.warnfor success/progress messages is unconventional.Lines 78, 98, and 120 use
console.warnfor non-warning informational output ("Captured current locale files…", "Wrote diff to…", "Removed temp directory"). Typicallyconsole.warnsignals a potential problem. If the intent is to write to stderr (to separate from stdout data),console.erroror a dedicated logger is more conventional; otherwiseconsole.log/console.infofits success messages better.Also applies to: 98-98, 120-120
scripts/check-unused-i18n-keys.ts (1)
166-179: Downgrade from error to warning is intentional — verify this aligns with CI expectations.Unused new i18n keys now produce
console.warnoutput and exit with code 0 instead of 1. This means the pre-commit/CI check will no longer block on unused keys. If this script is used as a git hook or CI gate, confirm that a non-blocking warning is the desired behavior — otherwise genuinely unused keys could accumulate silently.src/renderer/extensions/vueNodes/widgets/components/audio/AudioPreviewPlayer.test.ts (1)
8-12: Consider resettingmockToastAddin abeforeEachhook for robustness.Currently
mockToastAddis only reset in the error test (line 83). If tests are reordered or new tests are added that check toast calls, leaked state from prior tests could cause flaky failures.♻️ Suggested improvement
+import { afterEach, describe, expect, it, vi } from 'vitest' -import { describe, expect, it, vi } from 'vitest'Add after the mock declarations:
afterEach(() => { mockToastAdd.mockReset() })And remove the inline
vi.mocked(downloadFile).mockReset()on line 83 in favor of a centralized cleanup.Also applies to: 68-84
src/renderer/extensions/vueNodes/widgets/components/WidgetInputText.test.ts (1)
14-24: Parameter typePartial<InputTextProps>doesn't match the cast targetIWidgetOptions.The
optionsparameter is typed asPartial<InputTextProps>(PrimeVue) but cast toIWidgetOptions(litegraph). Since the widget actually consumesIWidgetOptions, consider aligning the parameter type to avoid a misleading API for future test authors.Suggested fix
+import type { InputTextProps } from 'primevue/inputtext' + const createMockWidget = ( value: string = 'default', - options: Partial<InputTextProps> = {}, + options: Partial<IWidgetOptions> = {}, callback?: (value: string) => void ): SimplifiedWidget<string> => ({ name: 'test_input', type: 'string', value, options: options as IWidgetOptions, callback })src/composables/graph/useGraphNodeManager.ts (1)
432-435: Synchronous layout initialization replacesrequestAnimationFrame— verify no downstream timing dependency.Removing the rAF deferral for non-workflow-loading node additions is a sensible simplification that eliminates a race window (node could be deleted before the rAF callback fires). The existing guard on line 404 remains as a good defensive check.
Confirm that no other code relies on the one-frame delay for position/size to settle before layout creation.
src/renderer/extensions/vueNodes/widgets/components/WidgetInputNumberInput.vue (2)
70-73: Consider extractingeffectiveMin/effectiveMaxcomputed properties.The pattern
filteredProps.value.min ?? -Number.MAX_VALUE/filteredProps.value.max ?? Number.MAX_VALUEis duplicated in four places (updateValue,canDecrement/canIncrement,updateValueBy,handleMouseMove). Centralizing these as computed properties would eliminate duplication and guard against inconsistency.Proposed refactor
Add near line 94:
+const effectiveMin = computed(() => filteredProps.value.min ?? -Number.MAX_VALUE) +const effectiveMax = computed(() => filteredProps.value.max ?? Number.MAX_VALUE)Then replace all occurrences, e.g.:
- const max = filteredProps.value.max ?? Number.MAX_VALUE - const min = filteredProps.value.min ?? -Number.MAX_VALUE - modelValue.value = Math.min(max, Math.max(min, parsed)) + modelValue.value = Math.min(effectiveMax.value, Math.max(effectiveMin.value, parsed))Also applies to: 96-103, 153-155, 175-177
194-199: Hardcoded English tooltip string should use i18n.Line 196 returns a raw English string for the tooltip. This should be localized via
vue-i18n.Proposed fix
Add a key to
src/locales/en/main.json, then:+const { t } = useI18n() + const buttonTooltip = computed(() => { if (buttonsDisabled.value) { - return 'Increment/decrement disabled: value exceeds JavaScript precision limit (±2^53)' + return t('widgetInputNumber.precisionLimitTooltip') } return null })As per coding guidelines: "Use vue-i18n in Composition API for any string literals. Place new translation entries in src/locales/en/main.json"
apps/desktop-ui/src/views/InstallView.stories.ts (1)
250-251: Accessing optionalelectronAPIwithout null check.
ElectronWindow.electronAPIis typed as optional (electronAPI?: ElectronAPI), but these lines access it without optional chaining. This works at runtime because the decorator sets it first, but strict TypeScript may flag it. Not a new issue since the PR just changed the cast type — noting for awareness.Also applies to: 269-270
| : undefined, | ||
| label: widget.label, | ||
| options: widget.options | ||
| options: widget.options as IWidgetOptions |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the IBaseWidget interface definition and its options property
rg -n "interface IBaseWidget" --type=ts -A 20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2867
🏁 Script executed:
# Search for IWidgetOptions definition and its structure
rg -n "interface IWidgetOptions" --type=ts -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1893
🏁 Script executed:
# Check the actual context of line 173 in useGraphNodeManager.ts
sed -n '165,180p' src/composables/graph/useGraphNodeManager.ts | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 673
🏁 Script executed:
# Get the function signature containing line 173
sed -n '150,175p' src/composables/graph/useGraphNodeManager.ts | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1127
🏁 Script executed:
# Check imports in the file to see which IWidgetOptions is being used
head -50 src/composables/graph/useGraphNodeManager.ts | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2114
🏁 Script executed:
# Find the SharedWidgetEnhancements interface definition
rg -n "interface SharedWidgetEnhancements" --type=ts -A 15Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1389
Type assertion reveals generic type parameter mismatch — consider preserving type information.
The cast from widget.options (typed as IWidgetOptions<unknown>) to IWidgetOptions<unknown[]>) is necessary due to the function receiving an untyped IBaseWidget. Instead of asserting, either:
-
Make the function generic to preserve the options type:
export function getSharedWidgetEnhancements<T extends IWidgetOptions<unknown>>( node: LGraphNode, widget: IBaseWidget<any, any, T> ): SharedWidgetEnhancements
-
Or update
SharedWidgetEnhancements.optionstoIWidgetOptions<unknown>to match the parameter type.
This avoids the assertion while improving type safety downstream.
🤖 Prompt for AI Agents
In `@src/composables/graph/useGraphNodeManager.ts` at line 173, The current cast
of widget.options to IWidgetOptions<unknown[]> hides a generic mismatch; update
the code to preserve the options type by making getSharedWidgetEnhancements
generic (e.g. add a type parameter T extends IWidgetOptions<unknown> and change
the widget parameter to IBaseWidget<any, any, T>) so SharedWidgetEnhancements
receives the correct options type, or alternatively change
SharedWidgetEnhancements.options to IWidgetOptions<unknown> to match the
incoming widget.options; locate getSharedWidgetEnhancements,
SharedWidgetEnhancements, IWidgetOptions and IBaseWidget in the file and apply
one of these two fixes to remove the unsafe type assertion.
| function get<K extends keyof Settings>(key: K): Settings[K] { | ||
| // Clone the value when returning to prevent external mutations | ||
| return _.cloneDeep(settingValues.value[key] ?? getDefaultValue(key)) | ||
| return _.cloneDeep(settingValues.value[key] ?? getDefaultValue(key)!) | ||
| } |
There was a problem hiding this comment.
Non-null assertion ! on getDefaultValue(key) masks a real undefined path.
If get is called for a key that has no stored value and whose SettingParams haven't been registered yet (or has no defaultValue), getDefaultValue(key) returns undefined. The ! silences TypeScript but the caller receives undefined typed as Settings[K].
This is unlikely to crash (lodash/es-toolkit cloneDeep(undefined) → undefined), but it's a type-level lie that could mislead callers. Consider a defensive guard or at minimum a dev-mode assertion so the condition is observable:
Suggested defensive guard
function get<K extends keyof Settings>(key: K): Settings[K] {
- return _.cloneDeep(settingValues.value[key] ?? getDefaultValue(key)!)
+ const value = settingValues.value[key] ?? getDefaultValue(key)
+ if (value === undefined && import.meta.env.DEV) {
+ console.warn(`Setting "${key}" accessed before registration or without a default value.`)
+ }
+ return _.cloneDeep(value) as Settings[K]
}📝 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.
| function get<K extends keyof Settings>(key: K): Settings[K] { | |
| // Clone the value when returning to prevent external mutations | |
| return _.cloneDeep(settingValues.value[key] ?? getDefaultValue(key)) | |
| return _.cloneDeep(settingValues.value[key] ?? getDefaultValue(key)!) | |
| } | |
| function get<K extends keyof Settings>(key: K): Settings[K] { | |
| // Clone the value when returning to prevent external mutations | |
| const value = settingValues.value[key] ?? getDefaultValue(key) | |
| if (value === undefined && import.meta.env.DEV) { | |
| console.warn(`Setting "${key}" accessed before registration or without a default value.`) | |
| } | |
| return _.cloneDeep(value) as Settings[K] | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/settings/settingStore.ts` around lines 119 - 122, The get
function uses a non-null assertion on getDefaultValue(key) which can be
undefined; update get (and references to settingValues.value) to remove the `!`
and explicitly handle the undefined path: compute const val =
settingValues.value[key] ?? getDefaultValue(key); if val is undefined, either
throw a clear error (or a dev-mode assertion) describing the missing registered
SettingParams/defaultValue for that key, otherwise return _.cloneDeep(val) as
Settings[K]; reference function get and getDefaultValue to locate where to add
the guard so callers never receive a lied-about non-null value.
| const handleDownload = () => { | ||
| if (!modelValue.value) return | ||
| try { | ||
| downloadFile(modelValue.value) | ||
| } catch { | ||
| toast.add({ | ||
| severity: 'error', | ||
| summary: t('g.error'), | ||
| detail: t('g.failedToDownloadFile'), | ||
| life: 3000 | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
downloadFile in cloud mode swallows async errors — toast won't fire on fetch failure.
downloadFile returns void and internally fires downloadViaBlobFetch with its own .catch() that only logs to console. If the blob fetch fails in cloud mode, this try/catch won't catch it, so the user sees no error toast. This is consistent with how downloadFile is used elsewhere (e.g., useMediaAssetActions.ts), but worth noting as a gap for the audio player UX.
🤖 Prompt for AI Agents
In
`@src/renderer/extensions/vueNodes/widgets/components/audio/AudioPreviewPlayer.vue`
around lines 208 - 220, The try/catch in handleDownload won't catch async errors
because downloadFile currently returns void and swallows errors inside
downloadViaBlobFetch; update the call chain so errors propagate: change
downloadFile (and/or where it calls downloadViaBlobFetch) to return a Promise
and rethrow or let the fetch error bubble up (remove the internal .catch that
only logs), then await downloadFile(modelValue.value) inside handleDownload so
the catch block can trigger toast.add on failure; reference downloadFile,
downloadViaBlobFetch, and handleDownload to locate the changes.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add dist/ directory existence check before running validation - Update grep patterns to focus on GTM/GA code (not Mixpanel) - Exclude source maps and credit files from search - Use more specific patterns to avoid false positives (e.g., 'toStringTag') - Mixpanel uses conditional dynamic imports and is expected in dist Addresses CodeRabbit review feedback. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Use regex pattern 'gtag(.config' to match both single and double quotes - Remove unnecessary --exclude='mixpanel.module-*.js' since we're not searching for mixpanel - Ensures validation catches GTM code regardless of minifier quote normalization Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
snomiao
force-pushed
the
feature/add-ci-telemetry-tree-shaking-validation
branch
from
239e398 to
97a038f
Compare
dosubot
bot
added
the
size:M
There was a problem hiding this comment.
Pull request overview
This PR adds a GitHub Actions workflow to prevent telemetry code (specifically Google Tag Manager and Google Analytics) from being accidentally shipped in OSS/localhost/desktop builds. The workflow was motivated by PR #8311 which added GTM but wasn't properly tree-shaken, requiring a revert in PR #8353.
Changes:
- New CI workflow that builds the project and greps dist/ for telemetry keywords
- Validates that GTM/GA code is absent from built files
- Provides clear error messages with remediation instructions when violations are found
| if grep -r -i --exclude='*.map' --exclude='CREDIT.txt' \ | ||
| -e 'googletagmanager' \ | ||
| -e 'gtm\.js' \ | ||
| -e 'analytics\.js' \ |
There was a problem hiding this comment.
The grep patterns for detecting gtag and analytics code have issues with special character handling:
-
Lines 51-52: The patterns
gtag(.configandgtag(.eventwon't match correctly because the parenthesis is a special regex character. Without the-Eflag for extended regex,grep -euses basic regex where(needs to be escaped as\(or the pattern should be simplified. -
Line 49-50: The escaped dots
\.in patterns likegtm\.jsandanalytics\.jsare unnecessary for basic grep (and may not work as intended without-E).
Consider one of these fixes:
- Add
-Eflag for extended regex and use proper escaping (e.g.,gtag\(\.config,gtag\(\.event) - Simplify patterns to avoid special characters (e.g., use
gtag.configor justgtagfor broader matching) - Use
-Fflag for fixed string matching if exact literal matches are intended
The current patterns may fail to detect GTM/GA code that should be caught.
| if grep -r -i --exclude='*.map' --exclude='CREDIT.txt' \ | |
| -e 'googletagmanager' \ | |
| -e 'gtm\.js' \ | |
| -e 'analytics\.js' \ | |
| if grep -rFi --exclude='*.map' --exclude='CREDIT.txt' \ | |
| -e 'googletagmanager' \ | |
| -e 'gtm.js' \ | |
| -e 'analytics.js' \ |
| - name: Setup frontend | ||
| uses: ./.github/actions/setup-frontend | ||
| - name: Build project | ||
| run: pnpm build |
There was a problem hiding this comment.
The workflow doesn't explicitly set the DISTRIBUTION environment variable during the build step. While the vite.config.mts defaults to 'localhost' when DISTRIBUTION is unset (which is correct for OSS builds), it's safer to be explicit to prevent future issues or changes to default behavior.
Consider adding an explicit environment variable:
- name: Build project
run: pnpm build
env:
DISTRIBUTION: localhost
This makes the intent clear and ensures the test validates the correct build configuration.
| run: pnpm build | |
| run: pnpm build | |
| env: | |
| DISTRIBUTION: localhost |
| # Check for Google Tag Manager / Google Analytics | ||
| echo "Checking for GTM/GA code..." | ||
| # Exclude source maps and credit files | ||
| if grep -r -i --exclude='*.map' --exclude='CREDIT.txt' \ | ||
| -e 'googletagmanager' \ | ||
| -e 'gtm\.js' \ | ||
| -e 'analytics\.js' \ | ||
| -e 'gtag(.config' \ | ||
| -e 'gtag(.event' \ | ||
| dist/; then | ||
| echo "❌ ERROR: Found GTM/GA code in dist files!" | ||
| echo "Google Tag Manager / Google Analytics must be properly tree-shaken from OSS builds." | ||
| FOUND_VIOLATIONS=1 | ||
| else | ||
| echo "✓ No GTM/GA code found" | ||
| fi |
There was a problem hiding this comment.
The workflow only checks for GTM/GA telemetry code but doesn't verify that Mixpanel code is properly lazy-loaded as expected. While the comment on line 25 mentions Mixpanel is expected in dist as a lazy chunk, the workflow doesn't validate this expectation.
Consider adding a check to ensure Mixpanel appears only as a dynamic import and not as a top-level import. This could prevent regressions where Mixpanel might be accidentally bundled instead of lazy-loaded:
For example, check that 'mixpanel-browser' appears only in specific lazy-loaded chunk patterns and not in main bundles.
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main.ts`:
- Line 18: Remove the test telemetry import and its invocation from the
production entry point: delete the static import line "import {
initializeAnalytics } from './test-telemetry-violation'" and remove the calls to
initializeAnalytics() (the invocation around lines 97–98) so the telemetry
module is not bundled; ensure there are no other direct references to
initializeAnalytics in src/main.ts and, if needed in future, load telemetry via
a guarded dynamic import or behind an isCloud check instead.
In `@src/test-telemetry-violation.ts`:
- Line 6: The function type for the gtag property currently uses a disallowed
any array in its rest parameter; update the signature (the gtag: (command:
string, ...args: any[]) => void declaration) to use unknown[] instead of any[]
(i.e., ...args: unknown[]) and adjust any downstream usages or type assertions
of gtag to narrow/validate the unknown values where they are consumed.
- Around line 1-18: The file src/test-telemetry-violation.ts intentionally
injects GTM/GA telemetry (symbols: initializeAnalytics, gtagId, window.gtag
usage) and must not be merged; remove this file from the branch (or move it to a
non-merge experimental branch/CI-only test) and delete any references to
initializeAnalytics from entry points such as src/main.ts so the telemetry
strings ('googletagmanager.com/gtm.js', gtag('config', ...), gtag('event', ...))
no longer appear in src/ or the build artifacts; ensure CI is validated against
the branch before merging and confirm no remaining imports or references remain.
src/main.ts
Outdated
| import '@/lib/litegraph/public/css/litegraph.css' | ||
| import router from '@/router' | ||
| import { useBootstrapStore } from '@/stores/bootstrapStore' | ||
| import { initializeAnalytics } from './test-telemetry-violation' |
There was a problem hiding this comment.
Remove test telemetry import and invocation from the production entry point.
The import on line 18 and the call on lines 97–98 inject GTM/GA telemetry code into every build. Since initializeAnalytics is statically imported (not behind a dynamic import() or any isCloud guard), the bundler will include the telemetry strings in dist/, causing the new CI workflow to fail on every subsequent push to main.
These lines must be removed before merge.
-import { initializeAnalytics } from './test-telemetry-violation'-// TEST: Intentional violation to test CI
-initializeAnalytics()
-Also applies to: 97-98
🤖 Prompt for AI Agents
In `@src/main.ts` at line 18, Remove the test telemetry import and its invocation
from the production entry point: delete the static import line "import {
initializeAnalytics } from './test-telemetry-violation'" and remove the calls to
initializeAnalytics() (the invocation around lines 97–98) so the telemetry
module is not bundled; ensure there are no other direct references to
initializeAnalytics in src/main.ts and, if needed in future, load telemetry via
a guarded dynamic import or behind an isCloud check instead.
src/test-telemetry-violation.ts
Outdated
| // TEST: This file intentionally violates telemetry tree-shaking | ||
| // It will be caught by the CI check | ||
|
|
||
| declare global { | ||
| interface Window { | ||
| gtag: (command: string, ...args: any[]) => void | ||
| } | ||
| } | ||
|
|
||
| export function initializeAnalytics() { | ||
| // This top-level import will NOT be tree-shaken | ||
| const gtagId = 'googletagmanager.com/gtm.js?id=GTM-TEST123' | ||
|
|
||
| window.gtag('config', 'GA-TEST123') | ||
| window.gtag('event', 'page_view') | ||
|
|
||
| console.warn('Analytics initialized:', gtagId) | ||
| } |
There was a problem hiding this comment.
This test violation file must not be merged into the production codebase.
This file intentionally introduces GTM/GA telemetry patterns (googletagmanager.com, gtag('config', ...), gtag('event', ...)) into src/, which will end up in dist/ and cause the new CI telemetry tree-shaking workflow to permanently fail on main. The commit message confirms this is a test artifact: "add intentional telemetry violation to test CI check."
To validate the CI workflow, either:
- Run the workflow against this branch (without merging), confirm it fails, then remove this file and the
src/main.tsreferences before merge. - Move the test to a separate, non-merge branch or a dedicated CI test matrix step that expects failure.
Additionally, shipping this file violates the repository's telemetry policy: GTM must be cloud-only and must never appear in OSS/local/desktop builds. Based on learnings, Google Tag Manager must be cloud-only and cannot be enabled in localhost or desktop distributions.
🤖 Prompt for AI Agents
In `@src/test-telemetry-violation.ts` around lines 1 - 18, The file
src/test-telemetry-violation.ts intentionally injects GTM/GA telemetry (symbols:
initializeAnalytics, gtagId, window.gtag usage) and must not be merged; remove
this file from the branch (or move it to a non-merge experimental branch/CI-only
test) and delete any references to initializeAnalytics from entry points such as
src/main.ts so the telemetry strings ('googletagmanager.com/gtm.js',
gtag('config', ...), gtag('event', ...)) no longer appear in src/ or the build
artifacts; ensure CI is validated against the branch before merging and confirm
no remaining imports or references remain.
src/test-telemetry-violation.ts
Outdated
|
|
||
| declare global { | ||
| interface Window { | ||
| gtag: (command: string, ...args: any[]) => void |
There was a problem hiding this comment.
any type violates coding guidelines.
The ...args: any[] parameter uses any, which is disallowed. If this file is kept for any reason, use unknown[] instead.
- gtag: (command: string, ...args: any[]) => void
+ gtag: (command: string, ...args: unknown[]) => voidAs per coding guidelines: "Never use any type; use proper TypeScript types."
📝 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.
| gtag: (command: string, ...args: any[]) => void | |
| gtag: (command: string, ...args: unknown[]) => void |
🤖 Prompt for AI Agents
In `@src/test-telemetry-violation.ts` at line 6, The function type for the gtag
property currently uses a disallowed any array in its rest parameter; update the
signature (the gtag: (command: string, ...args: any[]) => void declaration) to
use unknown[] instead of any[] (i.e., ...args: unknown[]) and adjust any
downstream usages or type assertions of gtag to narrow/validate the unknown
values where they are consumed.
eabf9a5 to
97a038f
Compare
dosubot
bot
added
size:M
2 participants
Summary
Adds a GitHub Actions workflow to prevent telemetry code from being accidentally shipped to OSS/local/desktop builds.
Context
Implementation
.github/workflows/ci-telemetry-tree-shaking.yamlHow It Works
The CI check searches for these patterns in the compiled dist files:
googletagmanager- GTM domain referencesgtm.js- GTM script fileanalytics.js- GA script filegtag(.config- gtag config calls (matches both single/double quotes)gtag(.event- gtag event calls (matches both single/double quotes)Example: What Gets Caught
If you accidentally add top-level telemetry code like:
The CI will FAIL with:
What's Allowed
✅ Mixpanel is allowed - it uses conditional dynamic imports and only loads in cloud builds
✅ Source maps and credit files are excluded from checks
Testing
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
┆Issue is synchronized with this Notion page by Unito