diff --git a/webview-ui/src/App.tsx b/webview-ui/src/App.tsx index 3e05ce64da..1f78ba2b45 100644 --- a/webview-ui/src/App.tsx +++ b/webview-ui/src/App.tsx @@ -194,7 +194,7 @@ const App = () => { // yet (e.g. an old extension build during a mixed rollout), matching VS Code's own // default -- it must never silently disable telemetry just because the field is // momentarily undefined during hydration. - telemetryClient.updateTelemetryState( + void telemetryClient.updateTelemetryState( telemetrySetting, telemetryKey, machineId, diff --git a/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts new file mode 100644 index 0000000000..d144549b71 --- /dev/null +++ b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" + +const posthogMocks = vi.hoisted(() => ({ + init: vi.fn(), + reset: vi.fn(), + identify: vi.fn(), + capture: vi.fn(), +})) + +const factoryState = vi.hoisted(() => ({ attempts: 0 })) + +// The factory throws on its first evaluation and succeeds afterwards, which mirrors a +// transient chunk fetch failure followed by a successful retry. This lives in its own +// file because vitest caches a module factory for the whole file, so a per-test failure +// toggle would leak into the sibling specs. +vi.mock("posthog-js", () => { + factoryState.attempts += 1 + + if (factoryState.attempts === 1) { + throw new Error("Failed to fetch dynamically imported module") + } + + return { default: posthogMocks } +}) + +describe("TelemetryClient after a failed PostHog chunk load", () => { + beforeEach(() => { + vi.clearAllMocks() + factoryState.attempts = 0 + }) + + it("warns and skips init when the chunk cannot be loaded", async () => { + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + vi.resetModules() + const { telemetryClient } = await import("@src/utils/TelemetryClient") + + const enabling = telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + telemetryClient.capture("queued_before_failure") + await enabling + + expect(consoleWarn).toHaveBeenCalledWith("Failed to load PostHog:", expect.any(Error)) + expect(posthogMocks.init).not.toHaveBeenCalled() + expect(posthogMocks.capture).not.toHaveBeenCalled() + + consoleWarn.mockRestore() + }) + + /** + * Regression guard for the `posthogLoadFailed` flag. A failed load must stop queueing, + * because the queue is only drained by a successful load and would otherwise grow + * until the cap and then replay events that were captured while telemetry had no + * client at all. + * + * The assertion is falsifiable because the retry makes a client observable: if the + * flag were removed, the post-failure captures would still be in the queue when the + * retry succeeds and would be replayed by flushPendingEvents. + * + * The same retry also pins the opposite edge: the flag is cleared when a load attempt + * starts, so events captured while the retry is in flight queue up and flush on + * success. Without that reset they would be dropped for the whole retry window. + */ + it("drops events captured after a failed load but queues those captured during the retry", async () => { + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + vi.resetModules() + const { telemetryClient } = await import("@src/utils/TelemetryClient") + + // First enable: the factory throws, so the chunk load rejects. + const failedEnabling = telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + telemetryClient.capture("queued_before_failure") + await failedEnabling + + expect(consoleWarn).toHaveBeenCalledWith("Failed to load PostHog:", expect.any(Error)) + expect(posthogMocks.init).not.toHaveBeenCalled() + + // Captured after the failure. With the flag these are dropped; without it they + // would sit in the queue for the retry below to replay. + telemetryClient.capture("captured_after_failure") + + // Second enable: the factory now succeeds. The capture below runs before this + // promise resolves, which is the retry window the flag reset opens; init not + // having run yet proves the window was genuinely open. + const retrying = telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + expect(posthogMocks.init).not.toHaveBeenCalled() + telemetryClient.capture("captured_during_retry") + await retrying + + // Only the in-flight retry event may reach PostHog: the pre-failure event was + // cleared with the failed load, and the post-failure one was dropped by the flag. + expect(posthogMocks.init).toHaveBeenCalledTimes(1) + expect(posthogMocks.capture).toHaveBeenCalledTimes(1) + expect(posthogMocks.capture).toHaveBeenCalledWith( + "captured_during_retry", + expect.objectContaining({ releaseChannel: "stable" }), + ) + + consoleWarn.mockRestore() + }) +}) diff --git a/webview-ui/src/__tests__/TelemetryClient.spec.ts b/webview-ui/src/__tests__/TelemetryClient.spec.ts index 0e1617a65e..9448141731 100644 --- a/webview-ui/src/__tests__/TelemetryClient.spec.ts +++ b/webview-ui/src/__tests__/TelemetryClient.spec.ts @@ -2,15 +2,15 @@ import posthog from "posthog-js" import { telemetryClient } from "@src/utils/TelemetryClient" -vi.mock("posthog-js", () => ({ - default: { - init: vi.fn(), - reset: vi.fn(), - identify: vi.fn(), - capture: vi.fn(), - }, +const posthogMocks = vi.hoisted(() => ({ + init: vi.fn(), + reset: vi.fn(), + identify: vi.fn(), + capture: vi.fn(), })) +vi.mock("posthog-js", () => ({ default: posthogMocks })) + describe("TelemetryClient", () => { beforeEach(() => { vi.clearAllMocks() @@ -35,21 +35,23 @@ describe("TelemetryClient", () => { * Tests for the updateTelemetryState method */ describe("updateTelemetryState", () => { - it("resets PostHog when called", () => { + it("does not load PostHog when the update cannot enable telemetry", async () => { // Act - telemetryClient.updateTelemetryState("enabled") + await telemetryClient.updateTelemetryState("enabled") - // Assert - expect(posthog.reset).toHaveBeenCalled() + // Assert - no credentials means no opt-in, so the PostHog chunk must stay + // unloaded and reset() must not run against an uninitialized client. + expect(posthog.reset).not.toHaveBeenCalled() + expect(posthog.init).not.toHaveBeenCalled() }) - it("initializes PostHog when telemetry is enabled with API key and distinctId", () => { + it("initializes PostHog when telemetry is enabled with API key and distinctId", async () => { // Arrange const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" // Act - telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) + await telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) // Assert expect(posthog.init).toHaveBeenCalledWith( @@ -69,21 +71,21 @@ describe("TelemetryClient", () => { expect(posthog.identify).toHaveBeenCalled() }) - it("doesn't initialize PostHog when telemetry is disabled", () => { + it("doesn't initialize PostHog when telemetry is disabled", async () => { // Act - telemetryClient.updateTelemetryState("disabled") + await telemetryClient.updateTelemetryState("disabled") // Assert expect(posthog.init).not.toHaveBeenCalled() }) - it("initializes PostHog when telemetry is unset and credentials are present (disclosed opt-out default)", () => { + it("initializes PostHog when telemetry is unset and credentials are present (disclosed opt-out default)", async () => { // Arrange const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" // Act - telemetryClient.updateTelemetryState("unset", API_KEY, DISTINCT_ID) + await telemetryClient.updateTelemetryState("unset", API_KEY, DISTINCT_ID) // Assert expect(posthog.init).toHaveBeenCalledWith( @@ -96,15 +98,15 @@ describe("TelemetryClient", () => { ) }) - it("doesn't initialize PostHog when telemetry is unset but credentials are missing", () => { + it("doesn't initialize PostHog when telemetry is unset but credentials are missing", async () => { // Act - telemetryClient.updateTelemetryState("unset") + await telemetryClient.updateTelemetryState("unset") // Assert expect(posthog.init).not.toHaveBeenCalled() }) - it("doesn't initialize PostHog when vscode.env.isTelemetryEnabled is false, even with an explicit Accept", () => { + it("doesn't initialize PostHog when vscode.env.isTelemetryEnabled is false, even with an explicit Accept", async () => { // Regression test: the webview's own PostHog client must respect the live VS // Code global telemetry toggle the same way the extension-side gate does -- // otherwise a user's explicit Accept can still send events while VS Code's @@ -112,16 +114,16 @@ describe("TelemetryClient", () => { const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" - telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID, false) + await telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID, false) expect(posthog.init).not.toHaveBeenCalled() }) - it("initializes PostHog when vscode.env.isTelemetryEnabled is true and the user opted in", () => { + it("initializes PostHog when vscode.env.isTelemetryEnabled is true and the user opted in", async () => { const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" - telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID, true) + await telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID, true) expect(posthog.init).toHaveBeenCalledWith( API_KEY, @@ -133,13 +135,13 @@ describe("TelemetryClient", () => { ) }) - it("defaults vscodeTelemetryEnabled to true when the argument is omitted", () => { + it("defaults vscodeTelemetryEnabled to true when the argument is omitted", async () => { // Callers on an old build (or during a state-hydration race) that don't yet // pass this argument must not have telemetry silently disabled as a side effect. const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" - telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) + await telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) expect(posthog.init).toHaveBeenCalled() }) @@ -149,9 +151,9 @@ describe("TelemetryClient", () => { * Tests for the capture method */ describe("capture", () => { - it("captures events when telemetry is enabled", () => { + it("captures events when telemetry is enabled", async () => { // Arrange - set telemetry to enabled - telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + await telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") vi.clearAllMocks() // Clear previous calls // Act @@ -167,9 +169,9 @@ describe("TelemetryClient", () => { ) }) - it("doesn't capture events when telemetry is disabled", () => { + it("doesn't capture events when telemetry is disabled", async () => { // Arrange - set telemetry to disabled - telemetryClient.updateTelemetryState("disabled") + await telemetryClient.updateTelemetryState("disabled") vi.clearAllMocks() // Clear previous calls // Act @@ -183,9 +185,9 @@ describe("TelemetryClient", () => { * This test verifies that no telemetry events are captured when * the telemetry setting is unset, further documenting the expected behavior */ - it("doesn't capture events when telemetry is unset", () => { + it("doesn't capture events when telemetry is unset", async () => { // Arrange - set telemetry to unset - telemetryClient.updateTelemetryState("unset") + await telemetryClient.updateTelemetryState("unset") vi.clearAllMocks() // Clear previous calls // Act @@ -195,4 +197,130 @@ describe("TelemetryClient", () => { expect(posthog.capture).not.toHaveBeenCalled() }) }) + + /** + * The PostHog chunk is now loaded on demand, so events captured between the + * enabling updateTelemetryState() call and the chunk resolving must be replayed + * once the client is ready. These tests reload the module to get fresh + * module-level state (the loaded client and the pending queue). + */ + describe("deferred PostHog loading", () => { + async function loadFreshTelemetryClient() { + vi.resetModules() + const module = await import("@src/utils/TelemetryClient") + return module.telemetryClient + } + + it("does not load PostHog until telemetry is actually enabled", async () => { + const client = await loadFreshTelemetryClient() + + await client.updateTelemetryState("disabled", "test-key", "test-user") + + expect(posthog.init).not.toHaveBeenCalled() + expect(posthog.reset).not.toHaveBeenCalled() + }) + + it("replays events captured while the PostHog chunk was loading", async () => { + const client = await loadFreshTelemetryClient() + + // Capture happens before the enabling update resolves, which is the window + // that did not exist when posthog-js was a static import. + const enabling = client.updateTelemetryState("enabled", "test-key", "test-user") + client.capture("early_event", { property: "queued" }) + await enabling + + expect(posthog.capture).toHaveBeenCalledWith( + "early_event", + expect.objectContaining({ property: "queued", releaseChannel: "stable" }), + ) + }) + + it("replays at most MAX_PENDING_EVENTS when more are captured during the chunk load", async () => { + const client = await loadFreshTelemetryClient() + const overflowCount = 51 + + // The enabling call suspends at `await loadPostHog()`, so every capture below + // runs while the chunk is still in flight. Asserting init has not run yet + // proves the window was genuinely open, which is what lets the client exist + // later and makes the replay assertion falsifiable. + const enabling = client.updateTelemetryState("enabled", "test-key", "test-user") + expect(posthog.init).not.toHaveBeenCalled() + + for (let index = 0; index < overflowCount; index++) { + client.capture(`queued_event_${index}`) + } + + expect(posthog.capture).not.toHaveBeenCalled() + + await enabling + + const replayed = vi.mocked(posthog.capture).mock.calls.map(([eventName]) => eventName) + expect(replayed).toHaveLength(50) + expect(replayed[0]).toBe("queued_event_0") + expect(replayed.at(-1)).toBe("queued_event_49") + expect(replayed).not.toContain("queued_event_50") + }) + + it("drops queued events when telemetry is disabled before the chunk resolves", async () => { + const client = await loadFreshTelemetryClient() + + const enabling = client.updateTelemetryState("enabled", "test-key", "test-user") + client.capture("early_event") + await client.updateTelemetryState("disabled") + await enabling + + expect(posthog.capture).not.toHaveBeenCalled() + expect(posthog.init).not.toHaveBeenCalled() + }) + + it("does not replay events queued before a disable once telemetry is re-enabled", async () => { + const client = await loadFreshTelemetryClient() + + // Queue an event during the first load window, disable (which must empty the + // queue), then re-enable. Only the second load's events may be replayed. + const firstEnabling = client.updateTelemetryState("enabled", "test-key", "test-user") + client.capture("captured_before_disable") + await client.updateTelemetryState("disabled") + await firstEnabling + + await client.updateTelemetryState("enabled", "test-key", "test-user") + + const replayed = vi.mocked(posthog.capture).mock.calls.map(([eventName]) => eventName) + expect(replayed).not.toContain("captured_before_disable") + }) + + it("does not capture events while telemetry is disabled", async () => { + const client = await loadFreshTelemetryClient() + + await client.updateTelemetryState("disabled", "test-key", "test-user") + client.capture("dropped_event") + + expect(posthog.capture).not.toHaveBeenCalled() + }) + + it("identifies the user through the PostHog loaded callback", async () => { + const client = await loadFreshTelemetryClient() + + await client.updateTelemetryState("enabled", "test-key", "test-user") + + const initOptions = vi.mocked(posthog.init).mock.calls.at(-1)?.[1] + expect(typeof initOptions?.loaded).toBe("function") + + initOptions?.loaded?.(posthog) + + expect(posthog.identify).toHaveBeenCalledWith("test-user") + }) + + it("skips init when the pending enable resolves after telemetry was disabled", async () => { + const client = await loadFreshTelemetryClient() + + // Start an enable whose chunk load is still in flight, then disable before it + // resolves. The stale enable must not initialize PostHog afterwards. + const enabling = client.updateTelemetryState("enabled", "test-key", "test-user") + await client.updateTelemetryState("disabled") + await enabling + + expect(posthog.init).not.toHaveBeenCalled() + }) + }) }) diff --git a/webview-ui/src/components/common/MermaidBlock.tsx b/webview-ui/src/components/common/MermaidBlock.tsx index 346fe3c471..e8dc8273cb 100644 --- a/webview-ui/src/components/common/MermaidBlock.tsx +++ b/webview-ui/src/components/common/MermaidBlock.tsx @@ -1,5 +1,4 @@ import { useEffect, useRef, useState } from "react" -import mermaid from "mermaid" import styled from "styled-components" import { useDebounceEffect } from "@src/utils/useDebounceEffect" import { vscode } from "@src/utils/vscode" @@ -9,6 +8,25 @@ import CodeBlock from "./CodeBlock" import { MermaidButton } from "@/components/common/MermaidButton" import { getMermaidBackgroundColor, getMermaidConfig, useMermaidTheme } from "./mermaidTheme" +type MermaidClient = (typeof import("mermaid"))["default"] + +// Mermaid is a large dependency that only matters once a message actually contains a +// diagram, so it is loaded on demand instead of being part of the eager webview graph. +let mermaidModulePromise: Promise | undefined + +function loadMermaid(): Promise { + mermaidModulePromise ??= import("mermaid").then( + (module) => module.default, + (error: unknown) => { + // Drop the cached rejection so a later diagram can retry the chunk load. + mermaidModulePromise = undefined + throw error + }, + ) + + return mermaidModulePromise +} + interface MermaidBlockProps { code: string } @@ -37,22 +55,32 @@ export default function MermaidBlock({ code }: MermaidBlockProps) { containerRef.current.innerHTML = "" } - mermaid.initialize(getMermaidConfig(theme.kind)) - mermaid - .parse(code) - .then(() => { - const id = `mermaid-${Math.random().toString(36).substring(2)}` - return mermaid.render(id, code) + loadMermaid() + .then((mermaid) => { + if (renderVersion !== renderVersionRef.current) { + return + } + + mermaid.initialize(getMermaidConfig(theme.kind)) + + return mermaid.parse(code).then(() => { + const id = `mermaid-${Math.random().toString(36).substring(2)}` + return mermaid.render(id, code) + }) }) - .then(({ svg }) => { + .then((result) => { + if (!result) { + return + } + if (containerRef.current && renderVersion === renderVersionRef.current) { - containerRef.current.innerHTML = svg + containerRef.current.innerHTML = result.svg } }) .catch((err) => { if (renderVersion === renderVersionRef.current) { console.warn("Mermaid parse/render failed:", err) - setError(err.message || "Failed to render Mermaid diagram") + setError(err instanceof Error ? err.message : "Failed to render Mermaid diagram") } }) .finally(() => { diff --git a/webview-ui/src/components/common/__tests__/MermaidBlock.spec.tsx b/webview-ui/src/components/common/__tests__/MermaidBlock.spec.tsx index 428e3ae306..40f6ed927a 100644 --- a/webview-ui/src/components/common/__tests__/MermaidBlock.spec.tsx +++ b/webview-ui/src/components/common/__tests__/MermaidBlock.spec.tsx @@ -1,4 +1,4 @@ -import { act, render, waitFor } from "@testing-library/react" +import { act, render, screen, waitFor } from "@testing-library/react" import { beforeEach, describe, expect, it, vi } from "vitest" import MermaidBlock from "../MermaidBlock" @@ -9,6 +9,8 @@ const mermaidMocks = vi.hoisted(() => ({ renderDiagram: vi.fn(), })) +// Mermaid is loaded through a dynamic import, so the module factory has to serve the +// same shape the component reads from `import("mermaid").default`. vi.mock("mermaid", () => ({ default: { initialize: mermaidMocks.initialize, @@ -86,4 +88,55 @@ describe("MermaidBlock", () => { expect(queryByTestId("dark-diagram")).toBeInTheDocument() expect(queryByTestId("stale-light-diagram")).not.toBeInTheDocument() }) + + it("shows the loading placeholder until the diagram renders", async () => { + let resolveRender!: (value: { svg: string }) => void + mermaidMocks.renderDiagram.mockImplementationOnce(() => new Promise((resolve) => (resolveRender = resolve))) + + render() + + expect(screen.getByText("common:mermaid.loading")).toBeInTheDocument() + + await waitFor(() => expect(mermaidMocks.renderDiagram).toHaveBeenCalledTimes(1), { timeout: 1_500 }) + await act(async () => resolveRender({ svg: '' })) + + await waitFor(() => expect(screen.queryByText("common:mermaid.loading")).not.toBeInTheDocument()) + expect(screen.getByTestId("rendered-diagram")).toBeInTheDocument() + }) + + it("renders the error state when the mermaid chunk fails to load", async () => { + vi.resetModules() + vi.doMock("mermaid", () => { + throw new Error("Failed to fetch dynamically imported module") + }) + const { default: FreshMermaidBlock } = await import("../MermaidBlock") + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + render() + + await waitFor(() => expect(screen.getByText("common:mermaid.render_error")).toBeInTheDocument(), { + timeout: 1_500, + }) + expect(screen.queryByTestId("rendered-diagram")).not.toBeInTheDocument() + expect(mermaidMocks.initialize).not.toHaveBeenCalled() + expect(mermaidMocks.parse).not.toHaveBeenCalled() + + consoleWarn.mockRestore() + vi.doUnmock("mermaid") + vi.resetModules() + }) + + it("renders the error state when parse rejects", async () => { + mermaidMocks.parse.mockRejectedValue(new Error("Parse error on line 1")) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + render() + + await waitFor(() => expect(screen.getByText("common:mermaid.render_error")).toBeInTheDocument(), { + timeout: 1_500, + }) + expect(mermaidMocks.renderDiagram).not.toHaveBeenCalled() + + consoleWarn.mockRestore() + }) }) diff --git a/webview-ui/src/utils/TelemetryClient.ts b/webview-ui/src/utils/TelemetryClient.ts index f3a70da29f..67df5b2d7f 100644 --- a/webview-ui/src/utils/TelemetryClient.ts +++ b/webview-ui/src/utils/TelemetryClient.ts @@ -1,33 +1,105 @@ -import posthog from "posthog-js" +import type { Properties } from "posthog-js" import { type TelemetrySetting, isTelemetryOptedIn } from "@roo-code/types" +type PostHogClient = (typeof import("posthog-js"))["default"] +type CapturedEvent = { eventName: string; properties: Properties } + +// posthog-js is large and only useful once telemetry is actually enabled, so it is +// loaded on demand instead of being part of the eager webview bundle. +let posthogModulePromise: Promise | undefined +let posthogClient: PostHogClient | undefined +let posthogLoadFailed = false + +// Events captured while the PostHog chunk is still in flight. Before this change the +// import was static, so a capture() that ran right after an enabling +// updateTelemetryState() call reached PostHog synchronously; replaying the queue keeps +// that ordering without dropping events that used to be sent. +// +// OVERFLOW POLICY: this queue is a best-effort ordering buffer, not a durable outbox. +// - Past MAX_PENDING_EVENTS the oldest entries are kept and new events are dropped +// silently. There is no error and no retry; the cap exists so a pathological +// startup cannot grow the queue without bound. +// - After a failed chunk load nothing is queued (posthogLoadFailed), because there is +// no client to replay to. The flag is cleared when a later opt-in starts a retry, so +// events captured while that retry is in flight queue again and flush on success. +// - Disabling telemetry empties the queue, so a re-enable cannot replay events that +// were captured under a state the user has since turned off. +const MAX_PENDING_EVENTS = 50 +const pendingEvents: CapturedEvent[] = [] + +function loadPostHog(): Promise { + // Clear the failure flag for the retry window: while this attempt is in flight there + // is no client yet, so capture() must queue instead of dropping events. + posthogLoadFailed = false + + posthogModulePromise ??= import("posthog-js").then( + (module) => { + posthogClient = module.default + return module.default + }, + (error: unknown) => { + // Drop the cached rejection so a later opt-in can retry the chunk load. + posthogModulePromise = undefined + posthogLoadFailed = true + throw error + }, + ) + + return posthogModulePromise +} + class TelemetryClient { private static instance: TelemetryClient private static telemetryEnabled: boolean = false - public updateTelemetryState( + public async updateTelemetryState( telemetrySetting: TelemetrySetting, apiKey?: string, distinctId?: string, vscodeTelemetryEnabled: boolean = true, - ) { - posthog.reset() - - if (isTelemetryOptedIn(telemetrySetting) && apiKey && distinctId && vscodeTelemetryEnabled) { - TelemetryClient.telemetryEnabled = true - - posthog.init(apiKey, { - api_host: "https://us.i.posthog.com", - persistence: "localStorage", - loaded: () => posthog.identify(distinctId), - capture_pageview: false, - capture_pageleave: false, - autocapture: false, - }) - } else { - TelemetryClient.telemetryEnabled = false + ): Promise { + const shouldEnable = Boolean( + isTelemetryOptedIn(telemetrySetting) && apiKey && distinctId && vscodeTelemetryEnabled, + ) + + TelemetryClient.telemetryEnabled = shouldEnable + + if (!shouldEnable) { + // Nothing to reset when PostHog was never loaded: reset() before init() only + // logged a "must initialize PostHog first" error and cleared nothing. + posthogClient?.reset() + pendingEvents.length = 0 + return + } + + let client: PostHogClient + + try { + client = await loadPostHog() + } catch (error) { + console.warn("Failed to load PostHog:", error) + pendingEvents.length = 0 + return + } + + // A later updateTelemetryState() call may have disabled telemetry while the + // chunk was loading; that stale enable must not initialize PostHog. + if (!TelemetryClient.telemetryEnabled || !apiKey || !distinctId) { + return } + + client.reset() + client.init(apiKey, { + api_host: "https://us.i.posthog.com", + persistence: "localStorage", + loaded: () => client.identify(distinctId), + capture_pageview: false, + capture_pageleave: false, + autocapture: false, + }) + + this.flushPendingEvents(client) } public static getInstance(): TelemetryClient { @@ -39,17 +111,42 @@ class TelemetryClient { } public capture(eventName: string, properties?: Record) { - if (TelemetryClient.telemetryEnabled) { - try { - posthog.capture(eventName, { - appName: process.env.PKG_NAME, - appVersion: process.env.PKG_VERSION, - releaseChannel: process.env.PKG_RELEASE_CHANNEL || "stable", - ...properties, - }) - } catch (_error) { - // Silently fail if there's an error capturing an event. + if (!TelemetryClient.telemetryEnabled) { + return + } + + const event: CapturedEvent = { + eventName, + properties: { + appName: process.env.PKG_NAME, + appVersion: process.env.PKG_VERSION, + releaseChannel: process.env.PKG_RELEASE_CHANNEL || "stable", + ...properties, + }, + } + + if (!posthogClient) { + if (!posthogLoadFailed && pendingEvents.length < MAX_PENDING_EVENTS) { + pendingEvents.push(event) } + + return + } + + this.send(posthogClient, event) + } + + private flushPendingEvents(client: PostHogClient) { + for (const event of pendingEvents.splice(0)) { + this.send(client, event) + } + } + + private send(client: PostHogClient, { eventName, properties }: CapturedEvent) { + try { + client.capture(eventName, properties) + } catch (_error) { + // Silently fail if there's an error capturing an event. } } } diff --git a/webview-ui/src/utils/__tests__/TelemetryClient.spec.ts b/webview-ui/src/utils/__tests__/TelemetryClient.spec.ts index 2e28994108..5698007382 100644 --- a/webview-ui/src/utils/__tests__/TelemetryClient.spec.ts +++ b/webview-ui/src/utils/__tests__/TelemetryClient.spec.ts @@ -21,14 +21,14 @@ describe("TelemetryClient", () => { expect(telemetryClient).toBeDefined() }) - it("should have updateTelemetryState method", () => { + it("should have updateTelemetryState method", async () => { // Test if the method exists expect(typeof telemetryClient.updateTelemetryState).toBe("function") // Call it with different values to verify it doesn't throw errors - expect(() => telemetryClient.updateTelemetryState("enabled")).not.toThrow() - expect(() => telemetryClient.updateTelemetryState("disabled")).not.toThrow() - expect(() => telemetryClient.updateTelemetryState("unset")).not.toThrow() + await expect(telemetryClient.updateTelemetryState("enabled")).resolves.toBeUndefined() + await expect(telemetryClient.updateTelemetryState("disabled")).resolves.toBeUndefined() + await expect(telemetryClient.updateTelemetryState("unset")).resolves.toBeUndefined() }) it("should have capture method", () => { @@ -40,21 +40,22 @@ describe("TelemetryClient", () => { expect(() => telemetryClient.capture("test_event", { key: "value" })).not.toThrow() }) - it("should reset PostHog when updating telemetry state", () => { + it("should not touch PostHog before the chunk is loaded", async () => { // Act - telemetryClient.updateTelemetryState("enabled") + await telemetryClient.updateTelemetryState("disabled") - // Assert - expect(posthog.reset).toHaveBeenCalled() + // Assert - a disabled update must not load the PostHog chunk or reset an + // uninitialized client, which only logged a console error. + expect(posthog.reset).not.toHaveBeenCalled() }) - it("should initialize PostHog when telemetry is enabled with API key and distinctId", () => { + it("should initialize PostHog when telemetry is enabled with API key and distinctId", async () => { // Arrange const API_KEY = "test-api-key" const DISTINCT_ID = "test-user-id" // Act - telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) + await telemetryClient.updateTelemetryState("enabled", API_KEY, DISTINCT_ID) // Assert expect(posthog.init).toHaveBeenCalledWith(