refactor(cli): address review findings in telemetry code

- Extract shared isDevMode() to utils/env.ts (was duplicated in dev.ts and client.ts)
- Use ui/colors.ts instead of raw ANSI escapes in telemetry notice (respects NO_COLOR)
- Derive known commands from subCommands object instead of maintaining duplicate set
- Skip telemetry on --help/--version and unknown commands
- Gate incrementCommandCount() behind shouldTrack() (no disk writes in CI)
- Add flushSync() for process.exit() paths (beforeExit doesn't fire on explicit exit)
- Remove dead trackBrowserInstall(success) param (failure path never called it)
- Remove redundant isEnabled/anonymousId caching in client.ts (config.ts cache suffices)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

packages/cli/src/cli.ts

@@ -2,56 +2,67 @@
 
 import { defineCommand, runMain } from "citty";
 import { VERSION } from "./version.js";
-import { showTelemetryNotice, flush, trackCommand, incrementCommandCount } from "./telemetry/index.js";
-
-// ---------------------------------------------------------------------------
-// Telemetry — detect command from argv, track it, flush on exit
-// ---------------------------------------------------------------------------
-
-const KNOWN_COMMANDS = new Set([
-  "init", "dev", "render", "lint", "info", "compositions",
-  "benchmark", "browser", "docs", "doctor", "upgrade", "telemetry",
-]);
-
-const commandArg = process.argv[2];
-const command = commandArg && KNOWN_COMMANDS.has(commandArg) ? commandArg : "unknown";
-
-// Show first-run notice (no-ops if already shown or telemetry disabled)
-if (command !== "telemetry") {
-  showTelemetryNotice();
-  trackCommand(command);
-  incrementCommandCount();
-}
-
-// Flush telemetry events before exit — non-blocking, 5s timeout
-process.on("beforeExit", () => {
-  flush().catch(() => {});
-});
+import {
+  showTelemetryNotice,
+  flush,
+  flushSync,
+  shouldTrack,
+  trackCommand,
+  incrementCommandCount,
+} from "./telemetry/index.js";
 
 // ---------------------------------------------------------------------------
 // CLI definition
 // ---------------------------------------------------------------------------
 
+const subCommands = {
+  init: () => import("./commands/init.js").then((m) => m.default),
+  dev: () => import("./commands/dev.js").then((m) => m.default),
+  render: () => import("./commands/render.js").then((m) => m.default),
+  lint: () => import("./commands/lint.js").then((m) => m.default),
+  info: () => import("./commands/info.js").then((m) => m.default),
+  compositions: () => import("./commands/compositions.js").then((m) => m.default),
+  benchmark: () => import("./commands/benchmark.js").then((m) => m.default),
+  browser: () => import("./commands/browser.js").then((m) => m.default),
+  docs: () => import("./commands/docs.js").then((m) => m.default),
+  doctor: () => import("./commands/doctor.js").then((m) => m.default),
+  upgrade: () => import("./commands/upgrade.js").then((m) => m.default),
+  telemetry: () => import("./commands/telemetry.js").then((m) => m.default),
+};
+
 const main = defineCommand({
   meta: {
     name: "hyperframes",
     version: VERSION,
     description: "Create and render HTML video compositions",
   },
-  subCommands: {
-    init: () => import("./commands/init.js").then((m) => m.default),
-    dev: () => import("./commands/dev.js").then((m) => m.default),
-    render: () => import("./commands/render.js").then((m) => m.default),
-    lint: () => import("./commands/lint.js").then((m) => m.default),
-    info: () => import("./commands/info.js").then((m) => m.default),
-    compositions: () => import("./commands/compositions.js").then((m) => m.default),
-    benchmark: () => import("./commands/benchmark.js").then((m) => m.default),
-    browser: () => import("./commands/browser.js").then((m) => m.default),
-    docs: () => import("./commands/docs.js").then((m) => m.default),
-    doctor: () => import("./commands/doctor.js").then((m) => m.default),
-    upgrade: () => import("./commands/upgrade.js").then((m) => m.default),
-    telemetry: () => import("./commands/telemetry.js").then((m) => m.default),
-  },
+  subCommands,
+});
+
+// ---------------------------------------------------------------------------
+// Telemetry — detect command from argv, track it, flush on exit
+// ---------------------------------------------------------------------------
+
+const commandArg = process.argv[2];
+const isHelpOrVersion = process.argv.includes("--help") || process.argv.includes("--version") || process.argv.includes("-h");
+const command = commandArg && commandArg in subCommands ? commandArg : "unknown";
+
+if (command !== "telemetry" && command !== "unknown" && !isHelpOrVersion) {
+  showTelemetryNotice();
+  trackCommand(command);
+  if (shouldTrack()) {
+    incrementCommandCount();
+  }
+}
+
+// Async flush for normal exit (beforeExit fires when the event loop drains)
+process.on("beforeExit", () => {
+  flush().catch(() => {});
+});
+
+// Sync flush for process.exit() calls (exit event only allows synchronous code)
+process.on("exit", () => {
+  flushSync();
 });
 
 runMain(main);

packages/cli/src/commands/browser.ts

@@ -48,7 +48,7 @@ async function runEnsure(): Promise<void> {
   });
 
   downloadSpinner.stop(c.success("Download complete"));
-  trackBrowserInstall(true);
+  trackBrowserInstall();
 
   console.log();
   console.log(`   ${c.dim("Source:")}  ${c.bold(result.source)}`);

packages/cli/src/commands/dev.ts

@@ -5,6 +5,7 @@ import { resolve, dirname, basename, join } from "node:path";
 import { fileURLToPath } from "node:url";
 import * as clack from "@clack/prompts";
 import { c } from "../ui/colors.js";
+import { isDevMode } from "../utils/env.js";
 
 /**
  * Check if a port is available by trying to listen on it briefly.
@@ -31,19 +32,6 @@ async function findAvailablePort(startPort: number): Promise<number> {
   return startPort; // fallback — let the server fail with a clear error
 }
 
-/**
- * Detect whether we're running from source (monorepo dev) or from the built bundle.
- * When running via tsx from source, the file is at cli/src/commands/dev.ts.
- * When running from the built bundle, the file is at cli/dist/cli.js.
- * We check the filename portion of the URL to avoid false positives from
- * directory names (e.g., /Users/someone/src/...).
- */
-function isDevMode(): boolean {
-  const url = new URL(import.meta.url);
-  // In dev mode the file is a .ts source file; in production it's a bundled .js
-  return url.pathname.endsWith(".ts");
-}
-
 export default defineCommand({
   meta: { name: "dev", description: "Start the studio for local development" },
   args: {

packages/cli/src/telemetry/client.ts

@@ -1,9 +1,7 @@
 import { readConfig, writeConfig } from "./config.js";
 import { VERSION } from "../version.js";
-
-// ---------------------------------------------------------------------------
-// PostHog configuration
-// ---------------------------------------------------------------------------
+import { c } from "../ui/colors.js";
+import { isDevMode } from "../utils/env.js";
 
 // This is a public project API key — safe to embed in client-side code.
 // It only allows writing events, not reading data.
@@ -12,21 +10,7 @@ const POSTHOG_HOST = "https://us.i.posthog.com";
 const FLUSH_TIMEOUT_MS = 5_000;
 
 // ---------------------------------------------------------------------------
-// Dev mode detection — telemetry is disabled when running from source (tsx)
-// ---------------------------------------------------------------------------
-
-function isDevMode(): boolean {
-  // In dev: files are .ts (running via tsx). In production: bundled .js
-  try {
-    const url = new URL(import.meta.url);
-    return url.pathname.endsWith(".ts");
-  } catch {
-    return false;
-  }
-}
-
-// ---------------------------------------------------------------------------
-// Lightweight PostHog client — we use the HTTP API directly to avoid
+// Lightweight PostHog client — uses the HTTP batch API directly to avoid
 // pulling in the full posthog-node SDK and its dependencies.
 // All calls are fire-and-forget with a hard timeout.
 // ---------------------------------------------------------------------------
@@ -41,44 +25,39 @@ let eventQueue: Array<{
   timestamp: string;
 }> = [];
 
-let isEnabled: boolean | null = null;
-let anonymousId: string | null = null;
+let telemetryEnabled: boolean | null = null;
 
 /**
  * Check if telemetry should be active.
  * Disabled when: dev mode, user opted out, CI environment, or HYPERFRAMES_NO_TELEMETRY set.
  */
-function shouldTrack(): boolean {
-  if (isEnabled !== null) return isEnabled;
+export function shouldTrack(): boolean {
+  if (telemetryEnabled !== null) return telemetryEnabled;
 
-  // Environment overrides
   if (process.env["HYPERFRAMES_NO_TELEMETRY"] === "1" || process.env["DO_NOT_TRACK"] === "1") {
-    isEnabled = false;
+    telemetryEnabled = false;
     return false;
   }
 
-  // CI detection
   if (process.env["CI"] === "true" || process.env["CI"] === "1") {
-    isEnabled = false;
+    telemetryEnabled = false;
     return false;
   }
 
-  // Dev mode — never phone home during development
   if (isDevMode()) {
-    isEnabled = false;
+    telemetryEnabled = false;
     return false;
   }
 
   // Placeholder API key means it hasn't been configured yet
   if (POSTHOG_API_KEY === "__POSTHOG_API_KEY__") {
-    isEnabled = false;
+    telemetryEnabled = false;
     return false;
   }
 
   const config = readConfig();
-  isEnabled = config.telemetryEnabled;
-  anonymousId = config.anonymousId;
-  return isEnabled;
+  telemetryEnabled = config.telemetryEnabled;
+  return telemetryEnabled;
 }
 
 /**
@@ -87,11 +66,6 @@ function shouldTrack(): boolean {
 export function trackEvent(event: string, properties: EventProperties = {}): void {
   if (!shouldTrack()) return;
 
-  if (!anonymousId) {
-    const config = readConfig();
-    anonymousId = config.anonymousId;
-  }
-
   eventQueue.push({
     event,
     properties: {
@@ -106,19 +80,19 @@ export function trackEvent(event: string, properties: EventProperties = {}): voi
 }
 
 /**
- * Flush all queued events to PostHog. Called on process exit.
- * Uses the /batch endpoint for efficiency.
+ * Flush all queued events to PostHog via async HTTP POST.
+ * Called before normal process exit via `beforeExit`.
  */
 export async function flush(): Promise<void> {
-  if (eventQueue.length === 0 || !shouldTrack()) {
-    eventQueue = [];
+  if (eventQueue.length === 0) {
     return;
   }
 
+  const config = readConfig();
   const batch = eventQueue.map((e) => ({
     event: e.event,
     properties: e.properties,
-    distinct_id: anonymousId,
+    distinct_id: config.anonymousId,
     timestamp: e.timestamp,
   }));
   eventQueue = [];
@@ -140,6 +114,40 @@ export async function flush(): Promise<void> {
   }
 }
 
+/**
+ * Synchronous flush for use in the `exit` event handler (which doesn't support async).
+ * Uses a synchronous XMLHttpRequest-style approach via child_process to ensure
+ * events are sent even when process.exit() is called.
+ */
+export function flushSync(): void {
+  if (eventQueue.length === 0) {
+    return;
+  }
+
+  const config = readConfig();
+  const batch = eventQueue.map((e) => ({
+    event: e.event,
+    properties: e.properties,
+    distinct_id: config.anonymousId,
+    timestamp: e.timestamp,
+  }));
+  eventQueue = [];
+
+  const payload = JSON.stringify({ api_key: POSTHOG_API_KEY, batch });
+
+  try {
+    // Spawn a detached process to send the request so we don't block exit.
+    // The subprocess inherits nothing and runs independently.
+    const { execFileSync } = require("node:child_process") as typeof import("node:child_process");
+    execFileSync(process.execPath, [
+      "-e",
+      `fetch(${JSON.stringify(`${POSTHOG_HOST}/batch/`)},{method:"POST",headers:{"Content-Type":"application/json"},body:${JSON.stringify(payload)},signal:AbortSignal.timeout(${FLUSH_TIMEOUT_MS})}).catch(()=>{})`,
+    ], { stdio: "ignore", timeout: FLUSH_TIMEOUT_MS });
+  } catch {
+    // Silently ignore
+  }
+}
+
 /**
  * Show the first-run telemetry notice if it hasn't been shown yet.
  * Returns true if the notice was shown (so callers can add spacing).
@@ -150,15 +158,11 @@ export function showTelemetryNotice(): boolean {
   const config = readConfig();
   if (config.telemetryNoticeShown) return false;
 
-  // Dynamic import to avoid pulling colors into the check path
-  const dim = (s: string) => `\x1b[2m${s}\x1b[0m`;
-  const cyan = (s: string) => `\x1b[36m${s}\x1b[0m`;
-
   console.log();
-  console.log(`  ${dim("Hyperframes collects anonymous usage data to improve the tool.")}`);
-  console.log(`  ${dim("No personal info, file paths, or content is collected.")}`);
+  console.log(`  ${c.dim("Hyperframes collects anonymous usage data to improve the tool.")}`);
+  console.log(`  ${c.dim("No personal info, file paths, or content is collected.")}`);
   console.log();
-  console.log(`  ${dim("Disable anytime:")} ${cyan("hyperframes telemetry disable")}`);
+  console.log(`  ${c.dim("Disable anytime:")} ${c.accent("hyperframes telemetry disable")}`);
   console.log();
 
   config.telemetryNoticeShown = true;

packages/cli/src/telemetry/events.ts

@@ -1,16 +1,9 @@
 import { trackEvent } from "./client.js";
 
-/**
- * Track a CLI command invocation.
- * This is the primary event — fired for every command.
- */
 export function trackCommand(command: string): void {
   trackEvent("cli_command", { command });
 }
 
-/**
- * Track a successful render completion with performance metrics.
- */
 export function trackRenderComplete(props: {
   durationMs: number;
   fps: number;
@@ -29,9 +22,6 @@ export function trackRenderComplete(props: {
   });
 }
 
-/**
- * Track a render failure (error type only, no message/stack).
- */
 export function trackRenderError(props: { fps: number; quality: string; docker: boolean }): void {
   trackEvent("render_error", {
     fps: props.fps,
@@ -40,16 +30,10 @@ export function trackRenderError(props: { fps: number; quality: string; docker:
   });
 }
 
-/**
- * Track which template was chosen during init.
- */
 export function trackInitTemplate(templateId: string): void {
   trackEvent("init_template", { template: templateId });
 }
 
-/**
- * Track browser download/ensure events.
- */
-export function trackBrowserInstall(success: boolean): void {
-  trackEvent("browser_install", { success });
+export function trackBrowserInstall(): void {
+  trackEvent("browser_install", {});
 }

packages/cli/src/telemetry/index.ts

@@ -1,5 +1,5 @@
 export { readConfig, writeConfig, incrementCommandCount, CONFIG_PATH } from "./config.js";
-export { trackEvent, flush, showTelemetryNotice } from "./client.js";
+export { trackEvent, flush, flushSync, shouldTrack, showTelemetryNotice } from "./client.js";
 export {
   trackCommand,
   trackRenderComplete,

packages/cli/src/utils/env.ts

@@ -0,0 +1,12 @@
+/**
+ * Detect whether we're running from source (monorepo dev) or from the built bundle.
+ * In dev: files are .ts (running via tsx). In production: bundled into .js by tsup.
+ */
+export function isDevMode(): boolean {
+  try {
+    const url = new URL(import.meta.url);
+    return url.pathname.endsWith(".ts");
+  } catch {
+    return false;
+  }
+}