test+fix(telemetry): address PR review — dev-mode gate, session-storage dedupe, payload tests

Addresses review comments on heygen-com#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.

Author: jrusso1020

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();
+    });
+  });
+});

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,

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]);
 

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<string, unknown>).VITE_HYPERFRAMES_POSTHOG_KEY;
+  } else {
+    (import.meta.env as Record<string, unknown>).VITE_HYPERFRAMES_POSTHOG_KEY = value;
+  }
+}
+
+function setNoTelemetry(value: string | undefined): void {
+  if (value === undefined) {
+    delete (import.meta.env as Record<string, unknown>).VITE_HYPERFRAMES_NO_TELEMETRY;
+  } else {
+    (import.meta.env as Record<string, unknown>).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);
+  });
+});

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();

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 */
+  }
+}