Add updated metrics consent prompt (#632)

* Add metrics consent handling

* Remove unnecessary frontend setting

* Refactor

* Add unit test

* Refactor

* Refactor

* Refactor

* Inline handler

* Fix type in tests

* Run format

* Change to use date and make setting more generic

* Update unit tests to use date

* Change to version string from date

* nit

* nit

* Fix format

---------

Co-authored-by: Chenlei Hu <[email protected]>
Co-authored-by: huchenlei <[email protected]>

Author: christian-byrne

global.d.ts

@@ -1 +1,2 @@
 declare const __COMFYUI_VERSION__: string;
+declare const __COMFYUI_DESKTOP_VERSION__: string;

src/constants.ts

@@ -43,6 +43,7 @@ export const IPC_CHANNELS = {
   UV_INSTALL_REQUIREMENTS: 'uv-install-requirements',
   GET_WINDOW_STYLE: 'get-window-style',
   TRACK_EVENT: 'track-event',
+  SET_METRICS_CONSENT: 'set-metrics-consent',
   INCREMENT_USER_PROPERTY: 'increment-user-property',
   UV_CLEAR_CACHE: 'uv-clear-cache',
   UV_RESET_VENV: 'uv-delete-venv',

src/install/installationManager.ts

@@ -101,6 +101,7 @@ export class InstallationManager {
 
     const installWizard = new InstallWizard(installOptions, this.telemetry);
     useDesktopConfig().set('basePath', installWizard.basePath);
+    useDesktopConfig().set('versionConsentedMetrics', __COMFYUI_DESKTOP_VERSION__);
 
     const { device } = installOptions;
     if (device !== undefined) {

src/main.ts

@@ -10,7 +10,7 @@ import { InstallationManager } from './install/installationManager';
 import { AppWindow } from './main-process/appWindow';
 import { ComfyDesktopApp } from './main-process/comfyDesktopApp';
 import SentryLogging from './services/sentry';
-import { getTelemetry } from './services/telemetry';
+import { getTelemetry, promptMetricsConsent } from './services/telemetry';
 import { DesktopConfig } from './store/desktopConfig';
 import { findAvailablePort } from './utils';
 
@@ -64,8 +64,9 @@ if (!gotTheLock) {
 // Async app start
 async function startApp() {
   // Load config or exit
+  let store: DesktopConfig | undefined;
   try {
-    const store = await DesktopConfig.load(shell);
+    store = await DesktopConfig.load(shell);
     if (!store) throw new Error('Unknown error loading app config on startup.');
   } catch (error) {
     log.error('Unhandled exception during config load', error);
@@ -114,10 +115,10 @@ async function startApp() {
 
       // At this point, user has gone through the onboarding flow.
       SentryLogging.comfyDesktopApp = comfyDesktopApp;
-      if (comfyDesktopApp.comfySettings.get('Comfy-Desktop.SendStatistics')) {
-        telemetry.hasConsent = true;
-        telemetry.flush();
-      }
+      const allowMetrics = await promptMetricsConsent(store, appWindow, comfyDesktopApp);
+      telemetry.hasConsent = allowMetrics;
+      if (allowMetrics) telemetry.flush();
+
       // Construct core launch args
       const useExternalServer = devOverride('USE_EXTERNAL_SERVER') === 'true';
       // Shallow-clone the setting launch args to avoid mutation.

src/preload.ts

@@ -331,6 +331,9 @@ const electronAPI = {
   },
   /** Gets the platform reported by node.js */
   getPlatform: () => process.platform,
+  setMetricsConsent: async (consent: boolean) => {
+    await ipcRenderer.invoke(IPC_CHANNELS.SET_METRICS_CONSENT, consent);
+  },
 
   /**
    *  Interfaces related to installation / install validation

src/services/telemetry.ts

@@ -8,7 +8,11 @@ import path from 'node:path';
 import si from 'systeminformation';
 
 import { IPC_CHANNELS } from '../constants';
+import { AppWindow } from '../main-process/appWindow';
+import { ComfyDesktopApp } from '../main-process/comfyDesktopApp';
 import { InstallOptions } from '../preload';
+import { DesktopConfig } from '../store/desktopConfig';
+import { compareVersions } from '../utils';
 
 let instance: ITelemetry | null = null;
 export interface ITelemetry {
@@ -213,3 +217,30 @@ export function trackEvent(eventName: string) {
     return descriptor;
   };
 }
+
+/** @returns Whether the user has consented to sending metrics. */
+export async function promptMetricsConsent(
+  store: DesktopConfig,
+  appWindow: AppWindow,
+  comfyDesktopApp: ComfyDesktopApp
+): Promise<boolean> {
+  const isMetricsEnabled = comfyDesktopApp.comfySettings.get('Comfy-Desktop.SendStatistics') ?? false;
+  const consentedOn = store.get('versionConsentedMetrics');
+  const isOutdated = !consentedOn || compareVersions(consentedOn, '0.4.8') < 0;
+  if (!isOutdated) return isMetricsEnabled;
+
+  store.set('versionConsentedMetrics', __COMFYUI_DESKTOP_VERSION__);
+  if (isMetricsEnabled) {
+    const consentPromise = new Promise<boolean>((resolve) => {
+      ipcMain.handle(IPC_CHANNELS.SET_METRICS_CONSENT, (_event, consent: boolean) => {
+        resolve(consent);
+        ipcMain.removeHandler(IPC_CHANNELS.SET_METRICS_CONSENT);
+      });
+    });
+
+    await appWindow.loadRenderer('metrics-consent');
+    return consentPromise;
+  }
+
+  return isMetricsEnabled;
+}

src/store/desktopSettings.ts

@@ -27,4 +27,6 @@ export type DesktopSettings = {
    * - `default`: Impersonal, static, plain - default window title bar
    */
   windowStyle?: 'custom' | 'default';
+  /** The version of comfyui-electron on which the user last consented to metrics. */
+  versionConsentedMetrics?: string;
 };

src/utils.ts

@@ -171,3 +171,26 @@ export async function validateHardware(): Promise<HardwareValidation> {
     };
   }
 }
+
+const normalize = (version: string) =>
+  version
+    .split(/[+.-]/)
+    .map(Number)
+    .filter((part) => !Number.isNaN(part));
+
+export function compareVersions(versionA: string, versionB: string): number {
+  versionA ??= '0.0.0';
+  versionB ??= '0.0.0';
+
+  const aParts = normalize(versionA);
+  const bParts = normalize(versionB);
+
+  for (let i = 0; i < Math.max(aParts.length, bParts.length); i++) {
+    const aPart = aParts[i] ?? 0;
+    const bPart = bParts[i] ?? 0;
+    if (aPart < bPart) return -1;
+    if (aPart > bPart) return 1;
+  }
+
+  return 0;
+}

tests/unit/telemetry.test.ts

@@ -1,9 +1,9 @@
 import { IpcMainEvent, ipcMain } from 'electron';
 import fs from 'node:fs';
 import path from 'node:path';
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { Mock, beforeEach, describe, expect, it, vi } from 'vitest';
 
-import { MixpanelTelemetry } from '../../src/services/telemetry';
+import { MixpanelTelemetry, promptMetricsConsent } from '../../src/services/telemetry';
 import { IPC_CHANNELS } from '/src/constants';
 
 vi.mock('electron', () => ({
@@ -14,6 +14,8 @@ vi.mock('electron', () => ({
   ipcMain: {
     on: vi.fn(),
     once: vi.fn(),
+    handle: vi.fn(),
+    removeHandler: vi.fn(),
   },
 }));
 
@@ -185,3 +187,166 @@ describe('MixpanelTelemetry', () => {
     expect(telemetry['mixpanelClient']).toBe(mockInitializedClient);
   });
 });
+
+describe('promptMetricsConsent', () => {
+  let store: { get: Mock; set: Mock };
+  let appWindow: { loadRenderer: Mock };
+  let comfyDesktopApp: { comfySettings: { get: Mock } };
+
+  const versionBeforeUpdate = '0.4.1';
+  const versionAfterUpdate = '1.0.1';
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    store = { get: vi.fn(), set: vi.fn() };
+    appWindow = { loadRenderer: vi.fn() };
+    comfyDesktopApp = { comfySettings: { get: vi.fn() } };
+  });
+
+  const runTest = async ({
+    storeValue,
+    settingsValue,
+    expectedResult,
+    mockConsent,
+    promptUser,
+  }: {
+    storeValue: string | null | undefined;
+    settingsValue: boolean | null | undefined;
+    expectedResult: boolean;
+    mockConsent?: boolean;
+    promptUser?: boolean;
+  }) => {
+    store.get.mockReturnValue(storeValue);
+    comfyDesktopApp.comfySettings.get.mockReturnValue(settingsValue);
+
+    if (promptUser) {
+      vi.mocked(ipcMain.handle).mockImplementationOnce((channel, handler) => {
+        if (channel === IPC_CHANNELS.SET_METRICS_CONSENT) {
+          // @ts-expect-error - handler is a mock and doesn't implement the correct signature
+          handler(null, mockConsent);
+        }
+      });
+    }
+
+    // @ts-expect-error - store is a mock and doesn't implement all of DesktopConfig
+    const result = await promptMetricsConsent(store, appWindow, comfyDesktopApp);
+    expect(result).toBe(expectedResult);
+
+    if (promptUser) ipcMain.removeHandler(IPC_CHANNELS.SET_METRICS_CONSENT);
+  };
+
+  it('should prompt for update if metrics were previously enabled', async () => {
+    await runTest({
+      storeValue: versionBeforeUpdate,
+      settingsValue: true,
+      expectedResult: true,
+      mockConsent: true,
+      promptUser: true,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).toHaveBeenCalledWith('metrics-consent');
+    expect(ipcMain.handle).toHaveBeenCalledWith(IPC_CHANNELS.SET_METRICS_CONSENT, expect.any(Function));
+  });
+
+  it('should not show prompt if consent is up-to-date', async () => {
+    await runTest({
+      storeValue: versionAfterUpdate,
+      settingsValue: true,
+      expectedResult: true,
+    });
+    expect(store.get).toHaveBeenCalledWith('versionConsentedMetrics');
+    expect(store.set).not.toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+
+  it('should return true if consent is up-to-date and metrics enabled', async () => {
+    await runTest({
+      storeValue: versionAfterUpdate,
+      settingsValue: true,
+      expectedResult: true,
+    });
+    expect(store.set).not.toHaveBeenCalled();
+  });
+
+  it('should return false if consent is up-to-date and metrics are disabled', async () => {
+    await runTest({
+      storeValue: versionAfterUpdate,
+      settingsValue: false,
+      expectedResult: false,
+    });
+    expect(store.set).not.toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+
+  it('should return false if consent is out-of-date and metrics are disabled', async () => {
+    await runTest({
+      storeValue: versionBeforeUpdate,
+      settingsValue: false,
+      expectedResult: false,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+
+  it('should update consent to false if the user denies', async () => {
+    await runTest({
+      storeValue: versionBeforeUpdate,
+      settingsValue: true,
+      expectedResult: false,
+      mockConsent: false,
+      promptUser: true,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).toHaveBeenCalledWith('metrics-consent');
+    expect(ipcMain.handle).toHaveBeenCalledWith(IPC_CHANNELS.SET_METRICS_CONSENT, expect.any(Function));
+  });
+
+  it('should return false if previous metrics setting is null', async () => {
+    await runTest({
+      storeValue: versionBeforeUpdate,
+      settingsValue: null,
+      expectedResult: false,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+
+  it('should prompt for update if versionConsentedMetrics is undefined', async () => {
+    await runTest({
+      storeValue: undefined,
+      settingsValue: true,
+      expectedResult: true,
+      mockConsent: true,
+      promptUser: true,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).toHaveBeenCalledWith('metrics-consent');
+    expect(ipcMain.handle).toHaveBeenCalledWith(IPC_CHANNELS.SET_METRICS_CONSENT, expect.any(Function));
+  });
+
+  it('should return false if both settings are null or undefined', async () => {
+    await runTest({
+      storeValue: null,
+      settingsValue: null,
+      expectedResult: false,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+
+  it('should return false if metrics are disabled and consent is null', async () => {
+    await runTest({
+      storeValue: versionBeforeUpdate,
+      settingsValue: null,
+      expectedResult: false,
+    });
+    expect(store.set).toHaveBeenCalled();
+    expect(appWindow.loadRenderer).not.toHaveBeenCalled();
+    expect(ipcMain.handle).not.toHaveBeenCalled();
+  });
+});

vite.base.config.ts

@@ -27,6 +27,7 @@ export function getBuildConfig(env: ConfigEnv): UserConfig {
 
     define: {
       __COMFYUI_VERSION__: JSON.stringify(pkg.config.comfyVersion),
+      __COMFYUI_DESKTOP_VERSION__: JSON.stringify(process.env.npm_package_version),
     },
   };
 }