fix(cli): preserve media at snapshot end boundary

[miguel-heygen]

No description provided.

[james-russo-rames-d-jusso]

Reviewed at 55d2b9e742987f5e27f95f269922749785eb5ac5.

What this does

Fixes the hyperframes snapshot --at <exact-clip-duration> case going black. Root cause: at exact clipEnd, the renderer's inclusive media window still activates the clip, but FFmpeg cannot decode a frame at a source's exclusive duration boundary. Pre-PR the snapshot passed relTime = duration to ffmpeg -ss, which extracted zero frames.

New pure helper resolveSnapshotVideoFrameTime at snapshot.ts:65-83 returns the frame time to actually pass to FFmpeg:

• Out of window (globalTime < clipStart, > clipEnd, or relativeTime < 0) → null (drop from active set).
• At the exact clip end (|globalTime - clipEnd| <= 1e-9) → Math.max(0, Math.min(relativeTime, sourceEnd - 1/30)) — one nominal 30fps frame back from the source's decodable edge.
• Otherwise → relativeTime unchanged.

The call-site refactor at snapshot.ts:389-434 also moves the activation decision out of page.evaluate: the browser now returns the full candidate list, and the Node side filters via resolveSnapshotVideoFrameTime before scheduling the FFmpeg extract. Same active-set logic, but now unit-testable in isolation.

Three regressions at snapshot.test.ts:40-76 cover the three branches (exact clip end, in-window, past-end).

Verification

• Boundary semantics. globalTime > clipEnd is strict >, so globalTime === clipEnd falls through to the atClipEnd branch instead of being dropped — matches the "media active at inclusive end" contract the renderer holds elsewhere. ✓
• Backoff math. Math.min(relativeTime, sourceEnd - 1/30) back-offs 33.3ms from source end. Safely inside any real video source (min VFR frame delta ≪ 33ms). Math.max(0, ...) clamps against negative for pathologically-short sources (sourceDuration < 1/30 ≈ 33ms). ✓
• sourceEnd = sourceDuration > 0 ? sourceDuration : relativeTime fallback — when source duration is unavailable (v.duration === NaN or Infinity), uses the relativeTime as the reference, so the back-off degenerates to the requested time itself. Preserves callsite behavior when video hasn't reported metadata. ✓
• 1e-9 at-clip-end tolerance — appropriate for float-time comparisons. Wider than double-precision epsilon at 15-second timestamps; tighter than any frame duration. ✓
• In-window unchanged. Second test pins globalTime=7.5, clipDuration=15, relativeTime=7.5 → 7.5 untouched. So the code path only diverges at exact end boundary. ✓
• Refactor preserves per-video filtering. Old: filter inside page.evaluate. New: return all candidates, filter in Node via flatMap(candidate => frameTime === null ? [] : [{...candidate, relTime: frameTime}]). Same output set. ✓
• CI: 19/19 focused tests + typecheck/format/oxlint green at head. ✓

Concerns

• 🟢 Hardcoded 30 fps back-off. For a 60fps source, 1/30 = two frame durations back, sampling two frames before end — safe, but slightly conservative. For a 24fps source, 1/30 ≈ 0.83 frame durations back, still inside the last valid frame. 1/24 would work but the 30fps constant is fine as a shared conservative default. Could parameterize as SNAPSHOT_FRAME_BACKOFF_SECONDS = 1 / 30 at module scope for future tuning. Note-only.

• 🟢 The 1e-9 tolerance boundary is untested. Third test pins 15.001 (well outside tolerance) → null, which proves the strict > clipEnd gate. A test pinning globalTime = clipEnd + 5e-10 returning the backed-off frame would prove the tolerance is load-bearing on both sides. Non-blocking.

• 🟢 The Node-side filter is per-candidate; the page.evaluate payload is now slightly larger (includes candidates that will get dropped). At snapshot-invocation frequency this is negligible, and separating browser state-collection from Node activation logic is the right shape — activation is deterministic in Node and now unit-testable. ✓ Note-only.

• 🟢 resolveSnapshotVideoFrameTime is exported but the module doc-comment focuses on FFmpeg's decode limit — worth mentioning it also serves as the single-source activation gate for the snapshot pipeline (i.e. what makes it null-returning matters for the callsite). Wordsmith. Note-only.

Verdict framing

Clean fix on a specific, reproducible boundary bug. Extracting the activation predicate into a pure helper unit-tests the decision that used to live inside page.evaluate, which is a nice refactor beyond the strict bug fix. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Reviewing at head 55d2b9e7. Fresh checkout, no prior R1 posted at this SHA (Rames co-reviewing — no cross-post yet).

Position: RIGHT — the fix mechanism (helper predicate + inclusive clipEnd + clamp to sourceEnd - 1/30) is the correct shape for the "FFmpeg can't decode at exclusive-source-duration" boundary. Blocker below is on the regression envelope around it, not the shape.

Strengths:

• packages/cli/src/commands/snapshot.ts:63-83 — the extraction into a pure resolveSnapshotVideoFrameTime cleanly separates the boundary decision from the browser-side candidate enumeration; the helper is now unit-testable and the caller (snapshot.ts:421-431) is a tight flatMap that reads well.
• snapshot.ts:79 — the strict-less/strict-greater inclusive check (< clipStart || > clipEnd) plus 1e-9 tolerance at the clip-end equality is the right float-safety wedge for accumulated global-time arithmetic.
• snapshot.ts:84-85 — the sourceEnd - 1/30 clamp with Math.max(0, …) correctly floors negative results for near-zero source durations.

Blockers:

• blocker (Miguel R1 directive #4 — parameterization): packages/cli/src/commands/snapshot.test.ts:40-76 — the new describe("resolveSnapshotVideoFrameTime") block covers 3 of the ≥5 distinct branches the helper takes. Missing regression coverage:
  1. globalTime < clipStart → null (predicate short-circuit at head of snapshot.ts:79)
  2. relativeTime < 0 → null (defensive branch on line 79 — reachable via mediaStart > 0 + very-early global time)
  3. sourceDuration <= 0 fallback (sourceEnd = relativeTime) → snapshot.ts:84
  4. clipStart != 0 offset clip at inclusive-end (e.g. clipStart: 5, clipDuration: 10, globalTime: 15) — the realistic production shape
  5. tolerance boundary that is near-but-not-exactly clipEnd (e.g. globalTime: 15 + 5e-10, clipEnd: 15) — asserts the 1e-9 wedge does what it advertises
     Please parameterize across every branch — the current tests would pass even if branches 1/3/4 regressed to the wrong behavior. Table-driven or a .each loop keeps this compact.

Important:

• important: snapshot.ts:407-410 + snapshot.ts:79-85 — the fix quietly changes behavior on clips configured such that mediaStart + clipDuration * playbackRate > sourceDuration (e.g. playbackRate: 2, clipDuration: 10, srcDur: 15). Under the old code, activeNow = t < start + duration gated the media OFF at globalTime === clipEnd; the new helper hits the atClipEnd branch and samples sourceEnd - 1/30. The last-decodable-frame behavior is likely what you want, but it's an unadvertised behavioral delta relative to "restore boundary frame." Worth a one-line note in the PR body or the helper's JSDoc so a future reader doesn't read this as pure inclusive-only.
• important: runtime parity — packages/player/src/parent-media.ts:141,161,190 gates media with strict relTime >= m.duration (exclusive end). At globalTime === clipEnd the runtime hides the video, but this snapshot path now paints its last decodable frame. That's plausibly the desired snapshot semantics (users expect a non-blank final frame) but it means snapshot ≠ what parent-media renders in the live player at that exact instant. If parity with the runtime is required for any downstream comparator (parity-harness, preview-regression, etc.), this is a divergence to document; if snapshot semantics are intentionally distinct here, add a JSDoc line to resolveSnapshotVideoFrameTime calling that out.

Nits:

• nit: snapshot.ts:85 — the 1/30 frame-step is a fixed 30fps assumption. For higher-fps sources (60fps, 120fps) it steps back one 30fps-frame worth of source time, which is safely inside the decodable range but wastes ~1 frame of resolution at the tail. If the source's actual fps is available via <video>.getVideoPlaybackQuality() or an authoring-side data-fps, prefer that; otherwise fine.
• nit: the helper accepts clipDuration: Number.POSITIVE_INFINITY (from the browser-side duration fallback at snapshot.ts:406) → clipEnd = Infinity → Math.abs(globalTime - Infinity) <= 1e-9 is always false → atClipEnd = false → pass-through. Behaves correctly but is worth a comment near the helper: "Infinity clipDuration intentionally never hits the atClipEnd branch."

Notes:

• CI at this SHA: all required checks green (Format, Preflight, Test, Build, Typecheck, Lint, Fallow audit, CLI smoke, SDK, CodeQL). mergeStateStatus=BLOCKED is reviewer-gate only.
• Audited call-site scope: syncVideoFrameVisibility (engine, screenshotService.ts:776-843) consumes only the active.map(a => a.id) set — no independent boundary computation, no sibling site with the same bug. injectVideoFramesBatch receives the pre-extracted dataUri, so the boundary decision lives entirely in the CLI helper. No Rule-2 fanout finding for this PR.
• No prior reviews on GitHub at this head; Rames is co-reviewing per the request in #hyperframes-internal.

Verdict: REQUEST CHANGES
Reasoning: Fix approach is RIGHT and the extraction is clean, but the regression envelope skips ≥3 branches of the helper (offset-clip inclusive end, defensive relativeTime<0, sourceDuration-fallback, tolerance-wedge) — Miguel R1 directive #4 asks these to be parameterized before the fix ships so a future refactor doesn't silently regress a branch that has zero coverage today.

— Via

[vanceingalls]

Note re. overlap with @james-russo-rames-d-jusso's review (posted ~3 min before mine — parallel-reviewer race, I hadn't seen his when I drafted):

• Convergence: both of us landed on RIGHT position, 30fps-hardcode as a nit, and the 1e-9 tolerance boundary being test-uncovered.
• Delta (severity): Rames flagged the missing-branch coverage as non-blocking nits; I'm blocking on it per Miguel R1 directive feat(studio): consolidate into single OSS-ready NLE editor #4 (parameterize regression across every branch). Gap I'd want closed before merge: globalTime < clipStart null, relativeTime < 0 null, sourceDuration <= 0 fallback, clipStart != 0 offset-clip inclusive end.
• Delta (net-new): two important-severity findings Rames didn't touch —
  1. snapshot.ts:407-410 — behavioral delta at mediaStart + clipDuration * playbackRate > srcDuration clips (old strict-less gate turned OFF; new atClipEnd branch samples sourceEnd - 1/30). Likely the intended behavior but merits a JSDoc note.
  2. packages/player/src/parent-media.ts:141,161,190 — runtime uses strict relTime >= duration (exclusive-end) to hide media; snapshot now paints an inclusive-end frame. Snapshot ≠ what the live player renders at t = clipEnd. Document as intentional or reconcile.

— Via

[james-russo-rames-d-jusso]

Reviewed at 3c18d20ee — R1 (55d2b9e7) → R2 delta is +72 / -5 across two files.

R2 delta

Closes my R1 🟢 on the tolerance-boundary test and fixes an asymmetric-tolerance predicate I hadn't flagged.

1. clipEndTolerance = 1e-9 extracted as a named local at snapshot.ts:79, referenced in both the out-of-window check and the at-clip-end check.
2. Tolerance predicate corrected — the pre-PR out-of-window check was globalTime > clipEnd (strict), but the at-clip-end check was Math.abs(globalTime - clipEnd) <= 1e-9 (tolerance-aware). So a globalTime slightly over clipEnd by a float-precision amount would fall into the "past-end null" branch before reaching the at-clip-end check. R2 makes the out-of-window guard tolerance-aware too: globalTime > clipEnd + clipEndTolerance — so a clipEnd + 5e-10 input reliably hits the backed-off-frame branch.
3. 5-branch parametrized regression at snapshot.test.ts:77-134:
   • before clip start — null.
   • negative relative time — null.
   • unknown source duration (sourceDuration = 0) — falls back to relativeTime - 1/30.
   • offset clip inclusive end (clipStart=5, clipDuration=10, sourceDuration=10) — the non-zero-start case → 10 - 1/30.
   • clip end within floating-point tolerance — globalTime = 15 + 5e-10 → 10 - 1/30. This pins the exact tolerance boundary I flagged. ✓
4. Doc comment expanded at snapshot.ts:63-68:
   • "This intentionally differs from the live player's exclusive-end visibility so an explicit end-boundary review does not become blank." — makes the design intent explicit (snapshot ≠ player-exclusive-end).
   • "This also clamps clips whose configured media window extends beyond the source." — documents the Math.min(relativeTime, sourceEnd - 1/30) behavior.
   • "An infinite clip duration intentionally never enters the end-boundary branch." — documents that clipDuration = Infinity → clipEnd = Infinity → Math.abs(globalTime - Infinity) = Infinity → not-atClipEnd → returns raw relativeTime.

Verification

• Tolerance boundary now symmetric. Out-of-window check + at-clip-end check use the same clipEndTolerance; a globalTime = clipEnd + 5e-10 input consistently maps to the backed-off frame branch instead of falling through the crack. Test pins this exact case. ✓
• Non-zero-start clip case (clipStart=5, clipDuration=10, sourceDuration=10, relativeTime=10) — verifies the arithmetic works with clipStart ≠ 0. Under R1, this case was implicitly covered but not explicitly tested. ✓
• Unknown-source-duration branch — sourceDuration = 0 triggers sourceEnd = relativeTime, and the final clamp Math.min(relativeTime, relativeTime - 1/30) = relativeTime - 1/30. Correct behavior. ✓
• Infinity clipDuration branch documented but not tested. Math.abs(globalTime - Infinity) is Infinity, > 1e-9, so atClipEnd = false → returns relativeTime unchanged. Acknowledged as intentional in the comment. ✓
• CI: 24/24 focused tests + typecheck/hooks green per Magi's summary. ✓

R1 concerns status

• 🟢 (R1) Hardcoded 30fps back-off — unchanged (still note-only, non-blocking).
• 🟢 (R1) 1e-9 tolerance boundary untested → CLOSED via the 15 + 5e-10 branch of the parametrized test. Plus the extraction into a named clipEndTolerance local + the predicate-symmetry fix means the tolerance is now enforced consistently. ✓
• 🟢 (R1) Node-side filter payload size — unchanged; the refactor's shape is preserved.
• 🟢 (R1) Docstring focus — closed via the expanded comment that names the semantic divergence from the player.

Verdict framing

R2 closes the tolerance-boundary gap I flagged plus a latent asymmetric-tolerance predicate that would have been a real edge-case bug. The 5-branch parametrization + expanded documentation is exactly the level of guard-rail this helper wanted. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at 3c18d20ee. R1 (55d2b9e7, pullrequestreview-4703461523) → R2 delta: +72/-5 across snapshot.ts and snapshot.test.ts. Verifying against R1's blocker + two important findings.

Strengths (new at R2):

• packages/cli/src/commands/snapshot.ts:79-83 — extracting clipEndTolerance = 1e-9 as a named local and reusing it in both the out-of-window guard (line 80) and the at-clip-end predicate (line 83) also closes a latent asymmetric-tolerance bug that neither R1 nor I had flagged as a blocker: pre-R2, a globalTime = clipEnd + 5e-10 would trip the strict > clipEnd check and short-circuit to null before reaching the tolerance-aware at-clip-end branch. Rames caught this on his end; nice pickup.
• snapshot.ts:63-69 — the JSDoc now names both divergences the helper introduces relative to a naive read: (a) intentionally differs from the live player's exclusive-end visibility, and (b) clamps clips whose configured media window extends beyond the source. Reads as the contract, not the mechanic.
• snapshot.test.ts:77-137 — it.each-style parameterization pins each branch on one line; if a future refactor drops a branch, the failing case is unambiguous.

R1 findings status:

• ✅ [F1 — blocker, regression parameterization] — snapshot.test.ts:77-137 covers all five branches I asked for:
  1. before clip start → null (predicate short-circuit on globalTime < clipStart) ✓
  2. negative relative time → null (defensive relativeTime < 0) ✓
  3. unknown source duration (sourceDuration = 0) → falls back to sourceEnd = relativeTime, resolves to relativeTime − 1/30 ✓
  4. offset clip inclusive end (clipStart = 5, clipDuration = 10, sourceDuration = 10) → 10 − 1/30 ✓
  5. clip end within floating-point tolerance (globalTime = 15 + 5e-10) → 10 − 1/30 ✓
     The tolerance branch also functions as the regression test for the asymmetric-tolerance predicate fix. .each layout keeps the branch list scannable.
• ✅ [F2 — important, playbackRate behavioral delta / configured-window-exceeds-source] — snapshot.ts:67-68 "This also clamps clips whose configured media window extends beyond the source" is exactly the one-line note I asked for. The Math.min(relativeTime, sourceEnd − 1/30) clamp is now advertised in the contract; future readers won't mistake the at-clip-end branch for pure inclusive-end pass-through.
• ✅ [F3 — important, parent-media runtime parity gap] — snapshot.ts:64-66 "This intentionally differs from the live player's exclusive-end visibility so an explicit end-boundary review does not become blank." Verified against packages/player/src/parent-media.ts:141,161 which still gates with relTime >= m.duration (exclusive end) — divergence is real and now documented as intentional. Downstream comparators reading the helper's JSDoc will see the semantic split.

Adversarial pass:

• Documented behavior vs implementation.
  • "An infinite clip duration intentionally never enters the end-boundary branch" → clipDuration = Infinity → clipEnd = Infinity → Math.abs(globalTime − Infinity) = Infinity > 1e-9 → atClipEnd = false → returns raw relativeTime. Matches. ✓
  • "Clamps clips whose configured media window extends beyond the source" → at atClipEnd, Math.min(relativeTime, sourceEnd − 1/30) with sourceEnd = sourceDuration when > 0; ceiling at sourceDuration − 33.3ms. Matches. ✓
  • "Sample one nominal 30fps frame inside the source" → sourceEnd − 1/30 ≈ 33.3ms before source end. Matches. ✓
• New predicate coverage. The tolerance-symmetric globalTime > clipEnd + clipEndTolerance guard (line 80) has both sides pinned: 15.001 (far past tolerance) → null in the existing "does not activate media after the clip end" test, and 15 + 5e-10 (well inside tolerance) → back-off frame in the parameterized case. The gap between +1e-9 and +0.001 is huge; a granular +1.5e-9 case (just past tolerance) would tighten the assertion but is a nit at best given the current spread.
• Call-site scope unchanged. snapshot.ts:394-436 still filters via a flatMap(candidate => resolveSnapshotVideoFrameTime(...) === null ? [] : [{...candidate, relTime: frameTime}]); no new fan-out. syncVideoFrameVisibility / injectVideoFramesBatch consumers unchanged.
• No new branch lacks coverage. The R2 delta introduces one new predicate (tolerance-symmetric out-of-window guard) and one refactored local (clipEndTolerance); both are exercised.

Notes:

• CI at head: 24/24 focused tests + Format, Lint, Typecheck, Test, Build, Fallow audit, CLI smoke, SDK, CodeQL all green. mergeStateStatus = BLOCKED is reviewer-gate only.
• No new call-site or contract fan-out from R1 → R2.

Verdict: APPROVE
Reasoning: F1 blocker's five-branch regression envelope is in place; F2 and F3 doc updates name both divergences in the helper's contract; the R2 delta additionally fixes an asymmetric-tolerance predicate that would have been a real edge-case bug. Adversarial pass clean.

— Via