From 6e530bc2dd8ff5a53338dd8a90fe5d5dcc907b11 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 25 Mar 2026 23:02:33 +0000 Subject: [PATCH] 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) --- packages/cli/src/cli.ts | 89 +++++++++++++---------- packages/cli/src/commands/browser.ts | 2 +- packages/cli/src/commands/dev.ts | 14 +--- packages/cli/src/telemetry/client.ts | 104 ++++++++++++++------------- packages/cli/src/telemetry/events.ts | 20 +----- packages/cli/src/telemetry/index.ts | 2 +- packages/cli/src/utils/env.ts | 12 ++++ 7 files changed, 121 insertions(+), 122 deletions(-) create mode 100644 packages/cli/src/utils/env.ts diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 95979c4e8..2364038cb 100644 --- a/packages/cli/src/cli.ts +++ b/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); diff --git a/packages/cli/src/commands/browser.ts b/packages/cli/src/commands/browser.ts index 66603c8d8..3144abfb8 100644 --- a/packages/cli/src/commands/browser.ts +++ b/packages/cli/src/commands/browser.ts @@ -48,7 +48,7 @@ async function runEnsure(): Promise { }); downloadSpinner.stop(c.success("Download complete")); - trackBrowserInstall(true); + trackBrowserInstall(); console.log(); console.log(` ${c.dim("Source:")} ${c.bold(result.source)}`); diff --git a/packages/cli/src/commands/dev.ts b/packages/cli/src/commands/dev.ts index a5f32e5fe..dd7789b20 100644 --- a/packages/cli/src/commands/dev.ts +++ b/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 { 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: { diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index 853db9b3a..efce78136 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/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 { - 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 { } } +/** + * 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; diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index c509808c6..2bf498c8e 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/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", {}); } diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index 0caaff3df..f1c00b71b 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/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, diff --git a/packages/cli/src/utils/env.ts b/packages/cli/src/utils/env.ts new file mode 100644 index 000000000..d4a922220 --- /dev/null +++ b/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; + } +}