From da38de1b124a9193e599817f45063c40a16490ec Mon Sep 17 00:00:00 2001 From: James Date: Wed, 20 May 2026 18:29:01 +0000 Subject: [PATCH] =?UTF-8?q?test+fix(telemetry):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20dev-mode=20gate,=20session-storage=20dedupe,=20payl?= =?UTF-8?q?oad=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review comments on #982: - studio shouldTrack(): adds VITE_HYPERFRAMES_NO_TELEMETRY (mirrors CLI's HYPERFRAMES_NO_TELEMETRY) and import.meta.env.DEV gates so dev / CI studio builds don't pollute production telemetry. shouldTrack() is now exported for testability. - App.tsx session dedupe: moves the once-per-session check from a useRef (which resets on HMR / remount) to sessionStorage via new hasFiredSessionStart / markSessionStartFired helpers in config.ts. - studioRenderTelemetry.ts: documents why `workers` is intentionally omitted from emitStudioRenderError (studio renders don't accept a user-supplied worker count, so early failures genuinely don't know one). - client.ts flush(): documents fire-and-forget no-retry design so future hands don't accidentally add retry logic that double-counts. Tests: - studioRenderTelemetry.test.ts (8 tests): perfPayload mapping for every RenderPerfSummary field, undefined-perf path, missing-extract path, zero-elapsed edge case, error event shape. - studio/telemetry/events.test.ts (4 tests): pin event names (studio_session_start, studio_render_start) and payload shape. - studio/telemetry/client.test.ts (9 tests): shouldTrack() returns false for non-phc_ key, opt-out, doNotTrack, build-time env, vite dev mode; memoization. --- .../src/server/studioRenderTelemetry.test.ts | 175 ++++++++++++++++++ .../cli/src/server/studioRenderTelemetry.ts | 4 + packages/studio/src/App.tsx | 13 +- packages/studio/src/telemetry/client.test.ts | 100 ++++++++++ packages/studio/src/telemetry/client.ts | 27 ++- packages/studio/src/telemetry/config.ts | 25 +++ packages/studio/src/telemetry/events.test.ts | 57 ++++++ 7 files changed, 393 insertions(+), 8 deletions(-) create mode 100644 packages/cli/src/server/studioRenderTelemetry.test.ts create mode 100644 packages/studio/src/telemetry/client.test.ts create mode 100644 packages/studio/src/telemetry/events.test.ts diff --git a/packages/cli/src/server/studioRenderTelemetry.test.ts b/packages/cli/src/server/studioRenderTelemetry.test.ts new file mode 100644 index 000000000..bce04fb74 --- /dev/null +++ b/packages/cli/src/server/studioRenderTelemetry.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import type { RenderPerfSummary } from "@hyperframes/producer"; + +// Mock `../telemetry/events.js` so we can capture trackRenderComplete / +// trackRenderError calls and verify the payload mapping without firing +// network requests. +const trackRenderComplete = vi.fn(); +const trackRenderError = vi.fn(); +vi.mock("../telemetry/events.js", () => ({ + trackRenderComplete: (...args: unknown[]) => trackRenderComplete(...args), + trackRenderError: (...args: unknown[]) => trackRenderError(...args), +})); + +// Imported after the mock is registered so the module picks up the mocked +// trackRenderComplete / trackRenderError. +const { emitStudioRenderComplete, emitStudioRenderError } = + await import("./studioRenderTelemetry.js"); + +const opts = { + fps: { num: 30, den: 1 } as const, + quality: "standard", +}; + +const fullPerf: RenderPerfSummary = { + renderId: "r-1", + totalElapsedMs: 5000, + fps: 30, + quality: "standard", + workers: 4, + chunkedEncode: false, + chunkSizeFrames: null, + compositionDurationSeconds: 10, + totalFrames: 300, + resolution: { width: 1920, height: 1080 }, + videoCount: 1, + audioCount: 0, + stages: { + compileMs: 100, + videoExtractMs: 200, + audioProcessMs: 50, + captureMs: 4000, + encodeMs: 500, + assembleMs: 150, + }, + videoExtractBreakdown: { + resolveMs: 10, + hdrProbeMs: 20, + hdrPreflightMs: 30, + hdrPreflightCount: 1, + vfrProbeMs: 40, + vfrPreflightMs: 50, + vfrPreflightCount: 2, + extractMs: 60, + cacheHits: 3, + cacheMisses: 4, + }, + tmpPeakBytes: 1024, + captureAvgMs: 13, + capturePeakMs: 25, +}; + +describe("studioRenderTelemetry", () => { + beforeEach(() => { + trackRenderComplete.mockClear(); + trackRenderError.mockClear(); + }); + + describe("emitStudioRenderComplete", () => { + it("tags the event with source: 'studio' and fps as a number", () => { + emitStudioRenderComplete(opts, 5000, fullPerf); + expect(trackRenderComplete).toHaveBeenCalledOnce(); + const payload = trackRenderComplete.mock.calls[0]![0]; + expect(payload.source).toBe("studio"); + expect(payload.fps).toBe(30); + expect(payload.quality).toBe("standard"); + expect(payload.docker).toBe(false); + expect(payload.gpu).toBe(false); + }); + + it("maps every RenderPerfSummary field to the expected payload key", () => { + emitStudioRenderComplete(opts, 5000, fullPerf); + const p = trackRenderComplete.mock.calls[0]![0]; + expect(p.durationMs).toBe(5000); + expect(p.workers).toBe(4); + expect(p.compositionDurationMs).toBe(10_000); + expect(p.compositionWidth).toBe(1920); + expect(p.compositionHeight).toBe(1080); + expect(p.totalFrames).toBe(300); + // speedRatio = compositionDurationMs / elapsedMs = 10000 / 5000 = 2 + expect(p.speedRatio).toBe(2); + expect(p.captureAvgMs).toBe(13); + expect(p.capturePeakMs).toBe(25); + expect(p.tmpPeakBytes).toBe(1024); + // stages + expect(p.stageCompileMs).toBe(100); + expect(p.stageVideoExtractMs).toBe(200); + expect(p.stageAudioProcessMs).toBe(50); + expect(p.stageCaptureMs).toBe(4000); + expect(p.stageEncodeMs).toBe(500); + expect(p.stageAssembleMs).toBe(150); + // video-extract breakdown + expect(p.extractResolveMs).toBe(10); + expect(p.extractHdrProbeMs).toBe(20); + expect(p.extractHdrPreflightMs).toBe(30); + expect(p.extractHdrPreflightCount).toBe(1); + expect(p.extractVfrProbeMs).toBe(40); + expect(p.extractVfrPreflightMs).toBe(50); + expect(p.extractVfrPreflightCount).toBe(2); + // `extractMs` on RenderPerfSummary maps to `extractPhase3Ms` on the event + // (named for legacy reasons — see packages/cli/src/commands/render.ts). + expect(p.extractPhase3Ms).toBe(60); + expect(p.extractCacheHits).toBe(3); + expect(p.extractCacheMisses).toBe(4); + }); + + it("omits all perf-derived fields when perfSummary is undefined", () => { + emitStudioRenderComplete(opts, 5000, undefined); + const p = trackRenderComplete.mock.calls[0]![0]; + // Identity fields still present + expect(p.source).toBe("studio"); + expect(p.fps).toBe(30); + expect(p.durationMs).toBe(5000); + // Perf-derived fields all undefined + expect(p.workers).toBeUndefined(); + expect(p.compositionDurationMs).toBeUndefined(); + expect(p.totalFrames).toBeUndefined(); + expect(p.speedRatio).toBeUndefined(); + expect(p.stageCompileMs).toBeUndefined(); + expect(p.extractResolveMs).toBeUndefined(); + }); + + it("omits videoExtractBreakdown fields when only the breakdown is absent", () => { + const perfNoExtract: RenderPerfSummary = { ...fullPerf, videoExtractBreakdown: undefined }; + emitStudioRenderComplete(opts, 5000, perfNoExtract); + const p = trackRenderComplete.mock.calls[0]![0]; + expect(p.workers).toBe(4); + expect(p.extractResolveMs).toBeUndefined(); + expect(p.extractCacheHits).toBeUndefined(); + }); + + it("leaves speedRatio undefined when elapsedMs is zero", () => { + emitStudioRenderComplete(opts, 0, fullPerf); + const p = trackRenderComplete.mock.calls[0]![0]; + expect(p.speedRatio).toBeUndefined(); + }); + }); + + describe("emitStudioRenderError", () => { + it("tags with source: 'studio' and forwards failedStage + elapsedMs", () => { + emitStudioRenderError(opts, 1200, "encode", new Error("boom")); + expect(trackRenderError).toHaveBeenCalledOnce(); + const p = trackRenderError.mock.calls[0]![0]; + expect(p.source).toBe("studio"); + expect(p.fps).toBe(30); + expect(p.quality).toBe("standard"); + expect(p.docker).toBe(false); + expect(p.failedStage).toBe("encode"); + expect(p.elapsedMs).toBe(1200); + expect(p.errorMessage).toBe("boom"); + }); + + it("stringifies non-Error throwables", () => { + emitStudioRenderError(opts, 100, undefined, "string error"); + expect(trackRenderError.mock.calls[0]![0].errorMessage).toBe("string error"); + }); + + it("does not include a workers field on the error event payload", () => { + // Documented behavior: studio renders don't request a worker count, + // and the early-failure path doesn't have perfSummary to read it from. + emitStudioRenderError(opts, 100, undefined, new Error("x")); + const p = trackRenderError.mock.calls[0]![0]; + expect(p.workers).toBeUndefined(); + }); + }); +}); diff --git a/packages/cli/src/server/studioRenderTelemetry.ts b/packages/cli/src/server/studioRenderTelemetry.ts index 868d54d3b..3dbd6d432 100644 --- a/packages/cli/src/server/studioRenderTelemetry.ts +++ b/packages/cli/src/server/studioRenderTelemetry.ts @@ -87,6 +87,10 @@ export function emitStudioRenderError( failedStage: string | undefined, err: unknown, ): void { + // `workers` is intentionally omitted: studio renders don't accept a + // user-supplied worker count (the producer picks its default), so on early + // failures we genuinely don't know one. The CLI side has the value from + // `options.workers` even before `job.perfSummary` exists; studio doesn't. trackRenderError({ fps: fpsToNumber(opts.fps), quality: opts.quality, diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index 0b008de93..c5827742b 100644 --- a/packages/studio/src/App.tsx +++ b/packages/studio/src/App.tsx @@ -48,19 +48,20 @@ import { readStudioUrlStateFromWindow, } from "./utils/studioUrlState"; import { trackStudioSessionStart } from "./telemetry/events"; +import { hasFiredSessionStart, markSessionStartFired } from "./telemetry/config"; export function StudioApp() { const { projectId, resolving, waitingForServer } = useServerConnection(); const initialUrlStateRef = useRef(readStudioUrlStateFromWindow()); - // Fire once per browser session to mark a "studio open" event so we can - // separate studio sessions from CLI invocations in product analytics. - // `has_project` lets us tell scratch-open from project-context-open. - const sessionFiredRef = useRef(false); + // Fire once per browser tab session — sessionStorage-backed so HMR + // remounts, route changes, and any future StudioApp remount within the + // same tab don't refire `studio_session_start`. `has_project` lets us + // tell scratch-open from project-context-open. useEffect(() => { - if (sessionFiredRef.current) return; if (resolving || waitingForServer) return; - sessionFiredRef.current = true; + if (hasFiredSessionStart()) return; + markSessionStartFired(); trackStudioSessionStart({ has_project: projectId != null }); }, [projectId, resolving, waitingForServer]); diff --git a/packages/studio/src/telemetry/client.test.ts b/packages/studio/src/telemetry/client.test.ts new file mode 100644 index 000000000..3f5e1c4fe --- /dev/null +++ b/packages/studio/src/telemetry/client.test.ts @@ -0,0 +1,100 @@ +// @vitest-environment happy-dom + +import { describe, expect, it, vi, beforeEach } from "vitest"; + +// `shouldTrack()` reads `POSTHOG_API_KEY` from module-level const that's +// evaluated at module load time, so changing `import.meta.env` after import +// has no effect on the key. Each test resets module cache and re-imports. + +const OPT_OUT_KEY = "hyperframes-studio:telemetryDisabled"; + +function setKey(value: string | undefined): void { + if (value === undefined) { + delete (import.meta.env as Record).VITE_HYPERFRAMES_POSTHOG_KEY; + } else { + (import.meta.env as Record).VITE_HYPERFRAMES_POSTHOG_KEY = value; + } +} + +function setNoTelemetry(value: string | undefined): void { + if (value === undefined) { + delete (import.meta.env as Record).VITE_HYPERFRAMES_NO_TELEMETRY; + } else { + (import.meta.env as Record).VITE_HYPERFRAMES_NO_TELEMETRY = value; + } +} + +function setDev(value: boolean): void { + (import.meta.env as { DEV: boolean }).DEV = value; +} + +async function loadShouldTrack(): Promise<() => boolean> { + vi.resetModules(); + const mod = await import("./client"); + return mod.shouldTrack; +} + +describe("studio client shouldTrack", () => { + beforeEach(() => { + setDev(false); + setKey("phc_test_key"); + setNoTelemetry(undefined); + localStorage.clear(); + vi.unstubAllGlobals(); + }); + + it("returns true when key is configured, not in dev mode, and no opt-outs", async () => { + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(true); + }); + + it("returns false when API key does not start with phc_", async () => { + setKey("not_a_real_key"); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false when API key is empty string", async () => { + setKey(""); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false when user has opted out via localStorage", async () => { + localStorage.setItem(OPT_OUT_KEY, "1"); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false when navigator.doNotTrack is '1'", async () => { + vi.stubGlobal("navigator", { ...navigator, doNotTrack: "1" }); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false when VITE_HYPERFRAMES_NO_TELEMETRY=1 at build time", async () => { + setNoTelemetry("1"); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false when VITE_HYPERFRAMES_NO_TELEMETRY='true'", async () => { + setNoTelemetry("true"); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("returns false in vite dev mode", async () => { + setDev(true); + const shouldTrack = await loadShouldTrack(); + expect(shouldTrack()).toBe(false); + }); + + it("memoizes its decision after the first call", async () => { + const shouldTrack = await loadShouldTrack(); + const first = shouldTrack(); + // Flip an underlying input — memoized return must not change. + localStorage.setItem(OPT_OUT_KEY, "1"); + expect(shouldTrack()).toBe(first); + }); +}); diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index a4a327c49..388925b18 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -38,9 +38,28 @@ function isApiKeyConfigured(): boolean { return POSTHOG_API_KEY.startsWith("phc_"); } -function shouldTrack(): boolean { +// VITE_HYPERFRAMES_NO_TELEMETRY mirrors the CLI's HYPERFRAMES_NO_TELEMETRY=1 +// opt-out so HeyGen's own dev/CI builds can suppress telemetry from the studio +// bundle the same way. Vite injects it at build time. Accepts "1" or "true". +function isBuildTimeOptOut(): boolean { + const v = import.meta.env.VITE_HYPERFRAMES_NO_TELEMETRY as string | undefined; + return v === "1" || v === "true"; +} + +// `import.meta.env.DEV` is true under `vite dev` / `vite preview`. Auto-suppress +// so developers running `hyperframes preview` don't pollute production telemetry. +function isViteDevMode(): boolean { + return import.meta.env.DEV === true; +} + +export function shouldTrack(): boolean { if (telemetryEnabled !== null) return telemetryEnabled; - telemetryEnabled = isApiKeyConfigured() && !isOptedOut() && !isDoNotTrackOn(); + telemetryEnabled = + isApiKeyConfigured() && + !isBuildTimeOptOut() && + !isViteDevMode() && + !isOptedOut() && + !isDoNotTrackOn(); return telemetryEnabled; } @@ -63,6 +82,10 @@ export function trackEvent(event: string, properties: EventProperties = {}): voi showNoticeOnce(); } +// Fire-and-forget: the queue is cleared before `send()` resolves, so a network +// failure drops the batch rather than retrying. Matches the CLI client's +// design. Do NOT add retry logic here — a retry without cross-batch dedup +// would risk double-counting events on transient PostHog 5xx responses. function flush(): void { if (eventQueue.length === 0) return; const distinctId = getAnonymousId(); diff --git a/packages/studio/src/telemetry/config.ts b/packages/studio/src/telemetry/config.ts index 62dc85994..658cb2b3b 100644 --- a/packages/studio/src/telemetry/config.ts +++ b/packages/studio/src/telemetry/config.ts @@ -51,3 +51,28 @@ export function markNoticeShown(): void { /* ignore */ } } + +// Session-scoped (cleared when the tab closes) so HMR remounts and +// route-level remounts within one tab don't refire `studio_session_start`. +// Uses sessionStorage directly because the dedupe is per-tab, not per-browser. +const SESSION_FIRED_KEY = "hyperframes-studio:sessionStartFired"; + +function safeSessionStorage(): Storage | null { + try { + return typeof sessionStorage === "undefined" ? null : sessionStorage; + } catch { + return null; + } +} + +export function hasFiredSessionStart(): boolean { + return safeSessionStorage()?.getItem(SESSION_FIRED_KEY) === "1"; +} + +export function markSessionStartFired(): void { + try { + safeSessionStorage()?.setItem(SESSION_FIRED_KEY, "1"); + } catch { + /* ignore */ + } +} diff --git a/packages/studio/src/telemetry/events.test.ts b/packages/studio/src/telemetry/events.test.ts new file mode 100644 index 000000000..376fd80e1 --- /dev/null +++ b/packages/studio/src/telemetry/events.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; + +// Mock client.trackEvent so we can assert event names and payloads without +// firing network requests or relying on memoized shouldTrack() state. +const trackEvent = vi.fn(); +vi.mock("./client", () => ({ + trackEvent: (...args: unknown[]) => trackEvent(...args), +})); + +const { trackStudioSessionStart, trackStudioRenderStart } = await import("./events"); + +describe("studio telemetry events", () => { + beforeEach(() => { + trackEvent.mockClear(); + }); + + it("trackStudioSessionStart emits 'studio_session_start' with has_project", () => { + trackStudioSessionStart({ has_project: true }); + expect(trackEvent).toHaveBeenCalledOnce(); + expect(trackEvent).toHaveBeenCalledWith("studio_session_start", { has_project: true }); + }); + + it("trackStudioSessionStart preserves false for has_project (scratch open)", () => { + trackStudioSessionStart({ has_project: false }); + expect(trackEvent).toHaveBeenCalledWith("studio_session_start", { has_project: false }); + }); + + it("trackStudioRenderStart emits 'studio_render_start' with all render opts", () => { + trackStudioRenderStart({ + fps: 30, + quality: "standard", + format: "mp4", + resolution: "landscape", + composition: "intro.html", + }); + expect(trackEvent).toHaveBeenCalledOnce(); + expect(trackEvent).toHaveBeenCalledWith("studio_render_start", { + fps: 30, + quality: "standard", + format: "mp4", + resolution: "landscape", + composition: "intro.html", + }); + }); + + it("trackStudioRenderStart leaves optional fields undefined when omitted", () => { + trackStudioRenderStart({ fps: 60, quality: "high", format: "webm" }); + const payload = trackEvent.mock.calls[0][1]; + expect(payload).toEqual({ + fps: 60, + quality: "high", + format: "webm", + resolution: undefined, + composition: undefined, + }); + }); +});