From a7137b744609c0631525cbe561db9ecacff5b402 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 02:08:18 +0900 Subject: [PATCH 1/3] perf(webview): load mermaid and posthog-js on demand instead of eagerly What: - MermaidBlock.tsx: replace the static `import mermaid from "mermaid"` with a cached `import("mermaid")` behind a module-level promise. The render effect awaits the chunk, keeps the existing render-version guard, and falls into the existing error state when the import rejects. - TelemetryClient.ts: replace the static `import posthog from "posthog-js"` with a cached dynamic import that only runs once telemetry is actually enabled. `updateTelemetryState` becomes async; `capture` queues events while the chunk is in flight and replays them after `init`, matching the ordering the previous static import gave for free. `reset()` is now only called on a loaded client, which removes the "must initialize PostHog first" console error the old unconditional `reset()` produced. - App.tsx: mark the now-async `updateTelemetryState` call with `void`. Why: - mermaid (80.1 MB installed) and posthog-js (25.8 MB installed) were both in the eager webview graph. Chart rendering is rare per message, and telemetry is dead weight for every user who has not opted in. - Entry chunk `assets/index.js` drops from 6,183,743 to 5,710,029 bytes (-473,714 bytes, -7.66%). Mermaid now lands in three lazy chunks (mermaid.core 94,159 + mermaid 29,549 + mermaid-parser.core 16,626 bytes) and posthog-js in one (module-CGGd6Piz.js, 307,685 bytes). Neither is referenced from index.html, so nothing preloads them. Impact: - Webview startup no longer parses mermaid or posthog-js. A mermaid block shows the existing loading placeholder while its chunk loads; a failed chunk load shows the existing error state. - Telemetry behavior is unchanged for enabled users: events captured in the load window are replayed after init. Disabled and unset-without-credentials users never download the PostHog chunk at all. --- webview-ui/src/App.tsx | 2 +- .../TelemetryClient.load-failure.spec.ts | 41 +++++ .../src/__tests__/TelemetryClient.spec.ts | 148 ++++++++++++++---- .../src/components/common/MermaidBlock.tsx | 48 ++++-- .../common/__tests__/MermaidBlock.spec.tsx | 55 ++++++- webview-ui/src/utils/TelemetryClient.ts | 139 ++++++++++++---- .../utils/__tests__/TelemetryClient.spec.ts | 21 +-- 7 files changed, 373 insertions(+), 81 deletions(-) create mode 100644 webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts 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..f4081d51b3 --- /dev/null +++ b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts @@ -0,0 +1,41 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" + +const posthogMocks = vi.hoisted(() => ({ + init: vi.fn(), + reset: vi.fn(), + identify: vi.fn(), + capture: vi.fn(), +})) + +// A failing factory makes the client's `import("posthog-js")` reject, which is how a +// failed chunk fetch surfaces at runtime. 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", () => { + throw new Error("Failed to fetch dynamically imported module") +}) + +describe("TelemetryClient with an unloadable PostHog chunk", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("warns, skips init, and stops queueing events", async () => { + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + 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() + + // A failed load must not keep queueing events forever. + telemetryClient.capture("after_failure") + expect(posthogMocks.capture).not.toHaveBeenCalled() + + consoleWarn.mockRestore() + }) +}) diff --git a/webview-ui/src/__tests__/TelemetryClient.spec.ts b/webview-ui/src/__tests__/TelemetryClient.spec.ts index 0e1617a65e..297fe1d836 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,88 @@ 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("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 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..2823a8ff1a 100644 --- a/webview-ui/src/utils/TelemetryClient.ts +++ b/webview-ui/src/utils/TelemetryClient.ts @@ -1,33 +1,91 @@ -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. +const MAX_PENDING_EVENTS = 50 +const pendingEvents: CapturedEvent[] = [] + +function loadPostHog(): Promise { + 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 +97,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( From 2a89c9d7c9f8d64eb4f08dde26913edfbe9fe80a Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 02:58:36 +0900 Subject: [PATCH 2/3] test(webview): cover the PostHog pending-queue overflow and failure guards What: - TelemetryClient.spec.ts: add "replays at most MAX_PENDING_EVENTS when more are captured during the chunk load". It starts an enabling update, asserts init has not run (proving the load window is open), captures 51 events, then asserts exactly 50 are replayed in order (queued_event_0..queued_event_49) and queued_event_50 is absent. - TelemetryClient.spec.ts: add "does not replay events queued before a disable once telemetry is re-enabled", which pins the `pendingEvents.length = 0` clear in the disable branch. - TelemetryClient.load-failure.spec.ts: the mock factory now throws on its first evaluation and succeeds afterwards, so a transient chunk failure is followed by an observable retry. The "stops queueing" case is split into a plain warn/skip-init case and a new case that captures after the failure, retries the load, and asserts nothing is replayed. - TelemetryClient.ts: document the overflow policy next to MAX_PENDING_EVENTS (silent drop past the cap, no queueing after a failed load, queue cleared on disable). Why: - The overflow branch was implemented but no test exercised it, so a cap of 51 or a `<` to `<=` boundary flip survived the suite. - The previous "stops queueing" spec was unfalsifiable: with a permanently throwing factory no client can ever exist, so `capture` was never called regardless of whether posthogLoadFailed existed. Deleting the flag kept the spec green. The retrying factory makes a client observable, so the flag is now load-bearing for the assertion. Impact: - Verified by mutation: MAX_PENDING_EVENTS 50 -> 51 fails the overflow spec, `<` -> `<=` fails it, deleting `!posthogLoadFailed` fails the new failure spec, and deleting the disable-branch clear fails the re-enable spec. All four mutants were applied, observed failing, and reverted. - No production behavior change; the only source edit is a comment. --- .../TelemetryClient.load-failure.spec.ts | 62 ++++++++++++++++--- .../src/__tests__/TelemetryClient.spec.ts | 42 +++++++++++++ webview-ui/src/utils/TelemetryClient.ts | 9 +++ 3 files changed, 104 insertions(+), 9 deletions(-) diff --git a/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts index f4081d51b3..60bc1bee82 100644 --- a/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts +++ b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts @@ -7,21 +7,31 @@ const posthogMocks = vi.hoisted(() => ({ capture: vi.fn(), })) -// A failing factory makes the client's `import("posthog-js")` reject, which is how a -// failed chunk fetch surfaces at runtime. 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. +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", () => { - throw new Error("Failed to fetch dynamically imported module") + factoryState.attempts += 1 + + if (factoryState.attempts === 1) { + throw new Error("Failed to fetch dynamically imported module") + } + + return { default: posthogMocks } }) -describe("TelemetryClient with an unloadable PostHog chunk", () => { +describe("TelemetryClient after a failed PostHog chunk load", () => { beforeEach(() => { vi.clearAllMocks() + factoryState.attempts = 0 }) - it("warns, skips init, and stops queueing events", async () => { + 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") @@ -32,8 +42,42 @@ describe("TelemetryClient with an unloadable PostHog chunk", () => { expect(posthogMocks.init).not.toHaveBeenCalled() expect(posthogMocks.capture).not.toHaveBeenCalled() - // A failed load must not keep queueing events forever. - telemetryClient.capture("after_failure") + 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. + */ + it("does not queue events captured after a failed load, even when a later load succeeds", 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, so a client exists and the queue is + // flushed. Only the pre-failure event may be gone already (it was cleared), and + // the post-failure event must never reach PostHog. + await telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + + expect(posthogMocks.init).toHaveBeenCalledTimes(1) expect(posthogMocks.capture).not.toHaveBeenCalled() consoleWarn.mockRestore() diff --git a/webview-ui/src/__tests__/TelemetryClient.spec.ts b/webview-ui/src/__tests__/TelemetryClient.spec.ts index 297fe1d836..9448141731 100644 --- a/webview-ui/src/__tests__/TelemetryClient.spec.ts +++ b/webview-ui/src/__tests__/TelemetryClient.spec.ts @@ -235,6 +235,32 @@ describe("TelemetryClient", () => { ) }) + 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() @@ -247,6 +273,22 @@ describe("TelemetryClient", () => { 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() diff --git a/webview-ui/src/utils/TelemetryClient.ts b/webview-ui/src/utils/TelemetryClient.ts index 2823a8ff1a..982b7c6670 100644 --- a/webview-ui/src/utils/TelemetryClient.ts +++ b/webview-ui/src/utils/TelemetryClient.ts @@ -15,6 +15,15 @@ let posthogLoadFailed = false // 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 at all (posthogLoadFailed), because +// there is no client to replay to until a later opt-in retries the load. +// - 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[] = [] From 602b2549f29fcd1d0748506e8ce24922c6334d22 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 07:20:40 +0900 Subject: [PATCH 3/3] fix(webview): queue telemetry during a PostHog retry window What: clear posthogLoadFailed when loadPostHog starts a new attempt, so capture() queues events while the retry chunk is in flight instead of dropping them; extend the load-failure spec to pin both edges of the flag. Why: after a failed chunk load the flag stayed set, so every event captured between a later opt-in and the retry resolving was silently dropped even though the retry succeeded and would have flushed a queue. Impact: events captured during a retry window now reach PostHog on success. --- .../TelemetryClient.load-failure.spec.ts | 25 ++++++++++++++----- webview-ui/src/utils/TelemetryClient.ts | 9 +++++-- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts index 60bc1bee82..d144549b71 100644 --- a/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts +++ b/webview-ui/src/__tests__/TelemetryClient.load-failure.spec.ts @@ -54,8 +54,12 @@ describe("TelemetryClient after a failed PostHog chunk load", () => { * 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("does not queue events captured after a failed load, even when a later load succeeds", async () => { + 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") @@ -72,13 +76,22 @@ describe("TelemetryClient after a failed PostHog chunk load", () => { // would sit in the queue for the retry below to replay. telemetryClient.capture("captured_after_failure") - // Second enable: the factory now succeeds, so a client exists and the queue is - // flushed. Only the pre-failure event may be gone already (it was cleared), and - // the post-failure event must never reach PostHog. - await telemetryClient.updateTelemetryState("enabled", "test-key", "test-user") + // 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).not.toHaveBeenCalled() + expect(posthogMocks.capture).toHaveBeenCalledTimes(1) + expect(posthogMocks.capture).toHaveBeenCalledWith( + "captured_during_retry", + expect.objectContaining({ releaseChannel: "stable" }), + ) consoleWarn.mockRestore() }) diff --git a/webview-ui/src/utils/TelemetryClient.ts b/webview-ui/src/utils/TelemetryClient.ts index 982b7c6670..67df5b2d7f 100644 --- a/webview-ui/src/utils/TelemetryClient.ts +++ b/webview-ui/src/utils/TelemetryClient.ts @@ -20,14 +20,19 @@ let posthogLoadFailed = false // - 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 at all (posthogLoadFailed), because -// there is no client to replay to until a later opt-in retries the load. +// - 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