fix(cli): surface video frame-injection failures in snapshot output#2255
Conversation
a7caaab to
1d2aca6
Compare
cedfe17 to
7afc1c4
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
#2255 — surface video frame-injection failures in snapshot output
Verdict: LGTM 🟢
Three previously-silent failures now surface warnings:
- Engine unavailable at import → warns that
<video>elements will appear black (was silent, two wild Windows reports) - Partial frame extraction (some active videos couldn't be extracted at seek time) → warns with count
syncVideoFrameVisibilityinjection failure → warns instead of silently falling through
All use console.warn with actionable context ("verify media via a draft render's extracted frames instead"). No behavior change for the happy path — these only fire when something goes wrong that previously produced black frames silently.
Review by Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed current head 7afc1c4. Engine-unavailable, partial extraction, and injection exceptions now produce specific warnings while preserving snapshot continuation; no sensitive path data is added. Required checks are green. APPROVE.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 7afc1c4.
All three silent-degradation paths in snapshot's video frame-injection get surfaced. The observability shape matches the rest of the command (console warn, not a structured event) so it's consistent with the existing surface — dashboards that scrape stderr will see it.
- Path 1 (
@hyperframes/engineunavailable): warns once. Users know their<video data-start>will be black; comment now says why (chrome-headless ignores programmatic currentTime writes). - Path 2 (partial extraction,
updates.length < active.length): warns per seek time with N/M. Also correctly fires when 0/N extracted, which is the worst-visibility subcase. - Path 3 (
syncVideoFrameVisibilitythrows): warns instead of silent fall-through. Covers both theupdates.length > 0inject branch and the[]clear-stale-overlays branch — the message says "video frame injection failed" for both, which is a slight semantic drift for the clear-overlays branch, but the aggregate user-visible outcome ("videos will appear black/stale") is the same in both branches so I don't think it's worth splitting the message.
LGTM from my side.
jrusso1020
left a comment
There was a problem hiding this comment.
Approving per the channel stamp-flow: Rames-D and Magi both cleared, CI clean at 7afc1c4.
1d2aca6 to
32228cf
Compare
7afc1c4 to
20241bc
Compare
32228cf to
d9e6f7d
Compare
20241bc to
2682fe4
Compare
d9e6f7d to
01350bf
Compare
2682fe4 to
314db4d
Compare
01350bf to
b3d851d
Compare
314db4d to
edc0a16
Compare
The base branch was changed.

What
Surfaces three previously-silent degradation paths in
snapshot's video frame-injection: (1) warns when@hyperframes/engineis unavailable (injection wholly unavailable), (2) warns when some-but-not-all active<video>frames could be extracted at a given seek time, (3) warns instead of silently swallowing injection failures.Why
Two wild Windows reports: system-Chrome snapshots omitted direct
<video>media while overlay HTML rendered correctly; seven non-overlapping<video>clips on one track rendered black in both snapshot and check with only one clip ever painting.snapshotDOES have an FFmpeg-based frame-injection path, but it degraded silently in all three of these spots — users had no signal that what they were looking at was black/stale.Validation
Existing
snapshot.test.tssuite (12 tests) green — these are new warning paths with no prior coverage, consistent with the rest of the command's console-output surface (not unit-tested). CLI build clean.🤖 Generated with Claude Code