From a97a7df2f4b89ddb0f07442526406a8ae352a94d Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Fri, 26 Sep 2025 17:21:11 -0400 Subject: [PATCH 1/2] Track when telemetry settings change --- packages/telemetry/src/TelemetryService.ts | 12 ++ packages/types/src/telemetry.ts | 2 + .../telemetrySettingsTracking.spec.ts | 155 ++++++++++++++++++ src/core/webview/webviewMessageHandler.ts | 17 +- 4 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 src/core/webview/__tests__/telemetrySettingsTracking.spec.ts diff --git a/packages/telemetry/src/TelemetryService.ts b/packages/telemetry/src/TelemetryService.ts index 3dbab273148..ec844a7ce80 100644 --- a/packages/telemetry/src/TelemetryService.ts +++ b/packages/telemetry/src/TelemetryService.ts @@ -226,6 +226,18 @@ export class TelemetryService { this.captureEvent(TelemetryEventName.TITLE_BUTTON_CLICKED, { button }) } + /** + * Captures when telemetry settings are changed + * @param previousSetting The previous telemetry setting + * @param newSetting The new telemetry setting + */ + public captureTelemetrySettingsChanged(previousSetting: string, newSetting: string): void { + this.captureEvent(TelemetryEventName.TELEMETRY_SETTINGS_CHANGED, { + previousSetting, + newSetting, + }) + } + /** * Checks if telemetry is currently enabled * @returns Whether telemetry is enabled diff --git a/packages/types/src/telemetry.ts b/packages/types/src/telemetry.ts index abaceb9fbb0..7d0c932cef9 100644 --- a/packages/types/src/telemetry.ts +++ b/packages/types/src/telemetry.ts @@ -71,6 +71,7 @@ export enum TelemetryEventName { SHELL_INTEGRATION_ERROR = "Shell Integration Error", CONSECUTIVE_MISTAKE_ERROR = "Consecutive Mistake Error", CODE_INDEX_ERROR = "Code Index Error", + TELEMETRY_SETTINGS_CHANGED = "Telemetry Settings Changed", } /** @@ -199,6 +200,7 @@ export const rooCodeTelemetryEventSchema = z.discriminatedUnion("type", [ TelemetryEventName.TAB_SHOWN, TelemetryEventName.MODE_SETTINGS_CHANGED, TelemetryEventName.CUSTOM_MODE_CREATED, + TelemetryEventName.TELEMETRY_SETTINGS_CHANGED, ]), properties: telemetryPropertiesSchema, }), diff --git a/src/core/webview/__tests__/telemetrySettingsTracking.spec.ts b/src/core/webview/__tests__/telemetrySettingsTracking.spec.ts new file mode 100644 index 00000000000..a99c8862a34 --- /dev/null +++ b/src/core/webview/__tests__/telemetrySettingsTracking.spec.ts @@ -0,0 +1,155 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import { TelemetryService } from "@roo-code/telemetry" +import { TelemetryEventName, type TelemetrySetting } from "@roo-code/types" + +describe("Telemetry Settings Tracking", () => { + let mockTelemetryService: { + captureTelemetrySettingsChanged: ReturnType + updateTelemetryState: ReturnType + hasInstance: ReturnType + } + + beforeEach(() => { + // Reset mocks + vi.clearAllMocks() + + // Create mock service + mockTelemetryService = { + captureTelemetrySettingsChanged: vi.fn(), + updateTelemetryState: vi.fn(), + hasInstance: vi.fn().mockReturnValue(true), + } + + // Mock the TelemetryService + vi.spyOn(TelemetryService, "hasInstance").mockReturnValue(true) + vi.spyOn(TelemetryService, "instance", "get").mockReturnValue(mockTelemetryService as any) + }) + + describe("when telemetry is turned OFF", () => { + it("should fire event BEFORE disabling telemetry", () => { + const previousSetting = "enabled" as TelemetrySetting + const newSetting = "disabled" as TelemetrySetting + + // Simulate the logic from webviewMessageHandler + const isOptedIn = newSetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + // If turning telemetry OFF, fire event BEFORE disabling + if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + // Update the telemetry state + TelemetryService.instance.updateTelemetryState(isOptedIn) + + // Verify the event was captured before updateTelemetryState + expect(mockTelemetryService.captureTelemetrySettingsChanged).toHaveBeenCalledWith("enabled", "disabled") + expect(mockTelemetryService.captureTelemetrySettingsChanged).toHaveBeenCalledBefore( + mockTelemetryService.updateTelemetryState as any, + ) + expect(mockTelemetryService.updateTelemetryState).toHaveBeenCalledWith(false) + }) + + it("should fire event when going from unset to disabled", () => { + const previousSetting = "unset" as TelemetrySetting + const newSetting = "disabled" as TelemetrySetting + + const isOptedIn = newSetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + TelemetryService.instance.updateTelemetryState(isOptedIn) + + expect(mockTelemetryService.captureTelemetrySettingsChanged).toHaveBeenCalledWith("unset", "disabled") + }) + }) + + describe("when telemetry is turned ON", () => { + it("should fire event AFTER enabling telemetry", () => { + const previousSetting = "disabled" as TelemetrySetting + const newSetting = "enabled" as TelemetrySetting + + const isOptedIn = newSetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + // Update the telemetry state first + TelemetryService.instance.updateTelemetryState(isOptedIn) + + // If turning telemetry ON, fire event AFTER enabling + if (!wasPreviouslyOptedIn && isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + // Verify the event was captured after updateTelemetryState + expect(mockTelemetryService.updateTelemetryState).toHaveBeenCalledWith(true) + expect(mockTelemetryService.captureTelemetrySettingsChanged).toHaveBeenCalledWith("disabled", "enabled") + expect(mockTelemetryService.updateTelemetryState).toHaveBeenCalledBefore( + mockTelemetryService.captureTelemetrySettingsChanged as any, + ) + }) + + it("should not fire event when going from enabled to enabled", () => { + const previousSetting = "enabled" as TelemetrySetting + const newSetting = "enabled" as TelemetrySetting + + const isOptedIn = newSetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + // Neither condition should be met + if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + TelemetryService.instance.updateTelemetryState(isOptedIn) + + if (!wasPreviouslyOptedIn && isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + // Should not fire any telemetry events + expect(mockTelemetryService.captureTelemetrySettingsChanged).not.toHaveBeenCalled() + expect(mockTelemetryService.updateTelemetryState).toHaveBeenCalledWith(true) + }) + + it("should fire event when going from unset to enabled (telemetry banner close)", () => { + const previousSetting = "unset" as TelemetrySetting + const newSetting = "enabled" as TelemetrySetting + + const isOptedIn = newSetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + // For unset -> enabled, both are opted in, so no event should fire + if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + TelemetryService.instance.updateTelemetryState(isOptedIn) + + if (!wasPreviouslyOptedIn && isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, newSetting) + } + + // unset is treated as opted-in, so no event should fire + expect(mockTelemetryService.captureTelemetrySettingsChanged).not.toHaveBeenCalled() + }) + }) + + describe("TelemetryService.captureTelemetrySettingsChanged", () => { + it("should call captureEvent with correct parameters", () => { + // Create a real instance to test the method + const mockCaptureEvent = vi.fn() + const service = new (TelemetryService as any)([]) + service.captureEvent = mockCaptureEvent + + service.captureTelemetrySettingsChanged("enabled", "disabled") + + expect(mockCaptureEvent).toHaveBeenCalledWith(TelemetryEventName.TELEMETRY_SETTINGS_CHANGED, { + previousSetting: "enabled", + newSetting: "disabled", + }) + }) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index c3e57c67a20..e638c7b8a31 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2296,9 +2296,24 @@ export const webviewMessageHandler = async ( case "telemetrySetting": { const telemetrySetting = message.text as TelemetrySetting - await updateGlobalState("telemetrySetting", telemetrySetting) + const previousSetting = getGlobalState("telemetrySetting") || "unset" const isOptedIn = telemetrySetting !== "disabled" + const wasPreviouslyOptedIn = previousSetting !== "disabled" + + // If turning telemetry OFF, fire event BEFORE disabling + if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, telemetrySetting) + } + + // Update the telemetry state + await updateGlobalState("telemetrySetting", telemetrySetting) TelemetryService.instance.updateTelemetryState(isOptedIn) + + // If turning telemetry ON, fire event AFTER enabling + if (!wasPreviouslyOptedIn && isOptedIn && TelemetryService.hasInstance()) { + TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, telemetrySetting) + } + await provider.postStateToWebview() break } From ca16e973292a51c8247b0a9a9f024defd28436e6 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Fri, 26 Sep 2025 22:28:17 -0400 Subject: [PATCH 2/2] PR feedback --- packages/telemetry/src/TelemetryService.ts | 9 +++++++-- packages/types/src/telemetry.ts | 9 ++++++++- src/core/webview/webviewMessageHandler.ts | 5 +++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/telemetry/src/TelemetryService.ts b/packages/telemetry/src/TelemetryService.ts index ec844a7ce80..5ea4cef936f 100644 --- a/packages/telemetry/src/TelemetryService.ts +++ b/packages/telemetry/src/TelemetryService.ts @@ -1,6 +1,11 @@ import { ZodError } from "zod" -import { type TelemetryClient, type TelemetryPropertiesProvider, TelemetryEventName } from "@roo-code/types" +import { + type TelemetryClient, + type TelemetryPropertiesProvider, + TelemetryEventName, + type TelemetrySetting, +} from "@roo-code/types" /** * TelemetryService wrapper class that defers initialization. @@ -231,7 +236,7 @@ export class TelemetryService { * @param previousSetting The previous telemetry setting * @param newSetting The new telemetry setting */ - public captureTelemetrySettingsChanged(previousSetting: string, newSetting: string): void { + public captureTelemetrySettingsChanged(previousSetting: TelemetrySetting, newSetting: TelemetrySetting): void { this.captureEvent(TelemetryEventName.TELEMETRY_SETTINGS_CHANGED, { previousSetting, newSetting, diff --git a/packages/types/src/telemetry.ts b/packages/types/src/telemetry.ts index 7d0c932cef9..f8b3e207380 100644 --- a/packages/types/src/telemetry.ts +++ b/packages/types/src/telemetry.ts @@ -200,10 +200,17 @@ export const rooCodeTelemetryEventSchema = z.discriminatedUnion("type", [ TelemetryEventName.TAB_SHOWN, TelemetryEventName.MODE_SETTINGS_CHANGED, TelemetryEventName.CUSTOM_MODE_CREATED, - TelemetryEventName.TELEMETRY_SETTINGS_CHANGED, ]), properties: telemetryPropertiesSchema, }), + z.object({ + type: z.literal(TelemetryEventName.TELEMETRY_SETTINGS_CHANGED), + properties: z.object({ + ...telemetryPropertiesSchema.shape, + previousSetting: telemetrySettingsSchema, + newSetting: telemetrySettingsSchema, + }), + }), z.object({ type: z.literal(TelemetryEventName.TASK_MESSAGE), properties: z.object({ diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index e638c7b8a31..af5f9925c35 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2304,10 +2304,11 @@ export const webviewMessageHandler = async ( if (wasPreviouslyOptedIn && !isOptedIn && TelemetryService.hasInstance()) { TelemetryService.instance.captureTelemetrySettingsChanged(previousSetting, telemetrySetting) } - // Update the telemetry state await updateGlobalState("telemetrySetting", telemetrySetting) - TelemetryService.instance.updateTelemetryState(isOptedIn) + if (TelemetryService.hasInstance()) { + TelemetryService.instance.updateTelemetryState(isOptedIn) + } // If turning telemetry ON, fire event AFTER enabling if (!wasPreviouslyOptedIn && isOptedIn && TelemetryService.hasInstance()) {