From 0730f88dc773afe6f4dfb1f3f5f7d8173a4e46d0 Mon Sep 17 00:00:00 2001
From: ukimsanov <ular.kimsanov@heygen.com>
Date: Mon, 20 Apr 2026 10:30:29 -0400
Subject: [PATCH] fix(cli): address snapshot PR review feedback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Incorporates four review points on #348:

1. Fix typecheck error — cast the dynamic `@hyperframes/engine` import to a
   typed shape and drop the `as typeof x` cast on a `null`-unioned variable
   that TS rejected. CI `Typecheck` was failing on main because the cast
   conflicted with the declared union.

2. Clear stale injected <img> overlays — always call
   `syncVideoFrameVisibility(page, activeIds)` on every seek (including
   `active.length === 0`), so injected frames from a previous timestamp
   don't leak into later snapshots. The runtime's visibility toggles act
   on the native <video> but not on its injected sibling, which Copilot
   correctly flagged as a leakage source.

3. URL-decode the served video path before resolving to a filesystem path.
   Files whose names contain spaces or other URL-encoded characters were
   skipped because `new URL().pathname` preserves `%20` while the file
   server decodes inbound requests and the file on disk lives at the
   decoded name.

4. Mirror `packages/core/src/runtime/media.ts` media math so clips with a
   non-1 `defaultPlaybackRate` get the correct active window and relTime.
   Specifically: clamp `defaultPlaybackRate` to [0.1, 5], apply
   `(t - start) * rate + mediaStart`, and adjust the duration-fallback
   branch by `/ rate` when only source duration is known.

5. Kill FFmpeg on a 30s timeout so a pathological clip cannot wedge
   snapshot indefinitely. Matches the default in
   `@hyperframes/engine`'s `runFfmpeg`.

Re-verified against the same 4-timestamp A/B on launch-video-2:
MD5s match the ffmpeg-from-render ground truth
  12.5s → ef9684e36fea53a0db7adf7cfcaacad3  (Stripe)
  16.0s → 487494ca16344d55d7181408dc439a56  (Framer)
  20.5s → 8835c34ad2a45755a1c98a7e079427a1  (HeyGen 3D)
  32.5s → 34dc9450f2bd661c12039d7aa82a30b0  (GitHub finale)

No-video projects (basecamp-tour, linear-brand, commissioned/github)
still produce identical output to the pre-fix path. Latency unchanged.

Made-with: Cursor
---
 packages/cli/src/commands/snapshot.ts | 190 ++++++++++++++++----------
 1 file changed, 118 insertions(+), 72 deletions(-)

diff --git a/packages/cli/src/commands/snapshot.ts b/packages/cli/src/commands/snapshot.ts
index f88fcab76..bd8be250f 100644
--- a/packages/cli/src/commands/snapshot.ts
+++ b/packages/cli/src/commands/snapshot.ts
@@ -11,6 +11,12 @@ import type { Example } from "./_examples.js";
 const __filename = fileURLToPath(import.meta.url);
 const __dirname = dirname(__filename);
 
+/** Maximum time a single-frame FFmpeg extract is allowed to run. Mirrors the
+ * default applied by `@hyperframes/engine`'s `runFfmpeg` so a pathological
+ * clip (corrupt media, stalled network mount, codec edge case) cannot wedge
+ * `hyperframes snapshot` indefinitely. */
+const FFMPEG_EXTRACT_TIMEOUT_MS = 30_000;
+
 /**
  * Extract a single frame from a video file at `timeSeconds` via FFmpeg.
  * Used to work around Chrome-headless's inability to reliably seek
@@ -23,32 +29,45 @@ async function extractVideoFrameToBuffer(
   const tmp = mkdtempSync(join(tmpdir(), "hf-snapshot-frame-"));
   const outPath = join(tmp, "frame.png");
   try {
-    const result = await new Promise<{ code: number | null; stderr: string }>((resolvePromise) => {
-      // `-ss` before `-i` performs a fast keyframe seek; adequate for snapshot accuracy
-      // (±1 frame) and orders of magnitude faster than the decode-and-scan alternative.
-      const ff = spawn("ffmpeg", [
-        "-hide_banner",
-        "-loglevel",
-        "error",
-        "-ss",
-        String(Math.max(0, timeSeconds)),
-        "-i",
-        videoPath,
-        "-frames:v",
-        "1",
-        "-q:v",
-        "2",
-        "-y",
-        outPath,
-      ]);
-      let stderr = "";
-      ff.stderr.on("data", (d: Buffer) => {
-        stderr += d.toString();
-      });
-      ff.on("close", (code) => resolvePromise({ code, stderr }));
-      ff.on("error", () => resolvePromise({ code: null, stderr: "ffmpeg spawn failed" }));
-    });
-    if (result.code !== 0 || !existsSync(outPath)) return null;
+    const result = await new Promise<{ code: number | null; stderr: string; timedOut: boolean }>(
+      (resolvePromise) => {
+        // `-ss` before `-i` performs a fast keyframe seek; adequate for snapshot accuracy
+        // (±1 frame) and orders of magnitude faster than the decode-and-scan alternative.
+        const ff = spawn("ffmpeg", [
+          "-hide_banner",
+          "-loglevel",
+          "error",
+          "-ss",
+          String(Math.max(0, timeSeconds)),
+          "-i",
+          videoPath,
+          "-frames:v",
+          "1",
+          "-q:v",
+          "2",
+          "-y",
+          outPath,
+        ]);
+        let stderr = "";
+        let timedOut = false;
+        const timer = setTimeout(() => {
+          timedOut = true;
+          ff.kill("SIGTERM");
+        }, FFMPEG_EXTRACT_TIMEOUT_MS);
+        ff.stderr.on("data", (d: Buffer) => {
+          stderr += d.toString();
+        });
+        ff.on("close", (code) => {
+          clearTimeout(timer);
+          resolvePromise({ code, stderr, timedOut });
+        });
+        ff.on("error", () => {
+          clearTimeout(timer);
+          resolvePromise({ code: null, stderr: "ffmpeg spawn failed", timedOut });
+        });
+      },
+    );
+    if (result.code !== 0 || result.timedOut || !existsSync(outPath)) return null;
     return readFileSync(outPath);
   } finally {
     try {
@@ -228,12 +247,20 @@ async function captureSnapshots(
       // already extracts each frame via FFmpeg and injects it as an <img> sibling
       // over the <video>. We reuse that same primitive here so `snapshot` and
       // `render` behave identically for timed <video data-start> elements.
-      let injectVideoFramesBatch:
-        | ((page: any, updates: Array<{ videoId: string; dataUri: string }>) => Promise<void>)
-        | null = null;
+      type InjectFn = (
+        page: unknown,
+        updates: Array<{ videoId: string; dataUri: string }>,
+      ) => Promise<void>;
+      type SyncVisibilityFn = (page: unknown, activeVideoIds: string[]) => Promise<void>;
+      let injectVideoFramesBatch: InjectFn | null = null;
+      let syncVideoFrameVisibility: SyncVisibilityFn | null = null;
       try {
-        const engine = await import("@hyperframes/engine");
-        injectVideoFramesBatch = engine.injectVideoFramesBatch as typeof injectVideoFramesBatch;
+        const engine = (await import("@hyperframes/engine")) as {
+          injectVideoFramesBatch: InjectFn;
+          syncVideoFrameVisibility: SyncVisibilityFn;
+        };
+        injectVideoFramesBatch = engine.injectVideoFramesBatch;
+        syncVideoFrameVisibility = engine.syncVideoFrameVisibility;
       } catch {
         // Engine unavailable in this install — snapshot will still run, and
         // compositions without <video data-start> get exactly the old behaviour.
@@ -271,70 +298,89 @@ async function captureSnapshots(
         // Without this, Chrome-headless renders them blank/first-frame because
         // it silently drops programmatic `currentTime` writes during capture.
         // No-op when the composition has no timed videos (basecamp, linear, etc.)
-        if (injectVideoFramesBatch) {
+        if (injectVideoFramesBatch && syncVideoFrameVisibility) {
+          // Mirror the runtime's media math in packages/core/src/runtime/media.ts
+          // so clips with non-1 `defaultPlaybackRate` get the right active
+          // window and the right `relTime`:
+          //   playbackRate = clamp(defaultPlaybackRate, 0.1, 5) — default 1
+          //   duration fallback = (sourceDuration - mediaStart) / playbackRate
+          //   relTime = (t - start) * playbackRate + mediaStart
+          //   active  = t >= start && t < start+duration && relTime >= 0
           const active = await page.evaluate((t: number) => {
             return Array.from(document.querySelectorAll("video[data-start]"))
               .map((el) => {
                 const v = el as HTMLVideoElement;
                 const start = parseFloat(v.dataset.start ?? "0") || 0;
+                const rawRate = v.defaultPlaybackRate;
+                const playbackRate =
+                  Number.isFinite(rawRate) && rawRate > 0 ? Math.max(0.1, Math.min(5, rawRate)) : 1;
+                const mediaStart =
+                  parseFloat(v.dataset.playbackStart ?? v.dataset.mediaStart ?? "0") || 0;
                 const rawDuration = parseFloat(v.dataset.duration ?? "");
                 const srcDur = Number.isFinite(v.duration) && v.duration > 0 ? v.duration : 0;
                 const duration =
                   Number.isFinite(rawDuration) && rawDuration > 0
                     ? rawDuration
                     : srcDur > 0
-                      ? srcDur
+                      ? Math.max(0, (srcDur - mediaStart) / playbackRate)
                       : Number.POSITIVE_INFINITY;
-                const mediaStart =
-                  parseFloat(v.dataset.playbackStart ?? v.dataset.mediaStart ?? "0") || 0;
+                const relTime = (t - start) * playbackRate + mediaStart;
+                const activeNow = t >= start && t < start + duration && relTime >= 0 && !!v.id;
                 return {
                   id: v.id,
                   src: v.currentSrc || v.src,
-                  start,
-                  duration,
-                  mediaStart,
+                  relTime,
+                  active: activeNow,
                 };
               })
-              .filter(
-                (entry) =>
-                  entry.id && entry.src && t >= entry.start && t < entry.start + entry.duration,
-              );
+              .filter((entry) => entry.active && entry.src);
           }, time);
 
-          if (active.length > 0) {
-            const updates: Array<{ videoId: string; dataUri: string }> = [];
-            for (const v of active) {
-              // The page-served URL (http://127.0.0.1:PORT/relative/path.mp4)
-              // maps 1:1 to <projectDir>/relative/path.mp4. Reconstruct the
-              // filesystem path from the URL pathname.
-              let filePath: string | null = null;
-              try {
-                const url = new URL(v.src);
-                const candidate = resolve(projectDir, url.pathname.replace(/^\//, ""));
-                const rel = relative(projectDir, candidate);
-                if (!rel.startsWith("..") && !isAbsolute(rel) && existsSync(candidate)) {
-                  filePath = candidate;
-                }
-              } catch {
-                /* unresolvable src (e.g. blob:, data:) — skip */
+          const updates: Array<{ videoId: string; dataUri: string }> = [];
+          for (const v of active) {
+            // The page-served URL (http://127.0.0.1:PORT/relative/path.mp4)
+            // maps 1:1 to <projectDir>/relative/path.mp4. decodeURIComponent
+            // the pathname — the file server decodes inbound requests, so a
+            // file with spaces in its path lives at the decoded name on disk
+            // while `new URL().pathname` preserves the %-encoding.
+            let filePath: string | null = null;
+            try {
+              const url = new URL(v.src);
+              const decodedPath = decodeURIComponent(url.pathname).replace(/^\//, "");
+              const candidate = resolve(projectDir, decodedPath);
+              const rel = relative(projectDir, candidate);
+              if (!rel.startsWith("..") && !isAbsolute(rel) && existsSync(candidate)) {
+                filePath = candidate;
               }
-              if (!filePath) continue;
-              const relTime = Math.max(0, time - v.start + v.mediaStart);
-              const png = await extractVideoFrameToBuffer(filePath, relTime);
-              if (!png) continue;
-              updates.push({
-                videoId: v.id,
-                dataUri: `data:image/png;base64,${png.toString("base64")}`,
-              });
+            } catch {
+              /* unresolvable src (e.g. blob:, data:) — skip */
             }
+            if (!filePath) continue;
+            const png = await extractVideoFrameToBuffer(filePath, Math.max(0, v.relTime));
+            if (!png) continue;
+            updates.push({
+              videoId: v.id,
+              dataUri: `data:image/png;base64,${png.toString("base64")}`,
+            });
+          }
+
+          // Always run the visibility sync — even when `active` is empty and
+          // no new updates were injected. Without this, stale __render_frame__
+          // <img> overlays left by a previous seek (where different clips were
+          // active) remain visible in later snapshots, because the runtime's
+          // visibility toggles act on the <video> element but not its injected
+          // <img> sibling.
+          try {
             if (updates.length > 0) {
-              try {
-                await injectVideoFramesBatch(page, updates);
-              } catch {
-                // If injection fails, fall through to the plain screenshot — no worse
-                // than pre-fix behaviour.
-              }
+              await injectVideoFramesBatch(page, updates);
             }
+            await syncVideoFrameVisibility(
+              page,
+              active.map((a) => a.id),
+            );
+          } catch {
+            // If either step fails, fall through to the plain screenshot —
+            // no worse than the pre-fix behaviour.
           }
         }