Repository navigation
fix(cli): snapshot shows each video's own frame, keyed by render id as render does - #5740
miguel-heygen wants to merge 4 commits into
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 169 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
Reviewed all seven changed files at 09196f468a12ba284420aa527c1f2bf555752de1 against effective base 4e6ee6d2300290b638042ec88ef5afb7a0c27408, including legacy behavior and independent adversarial passes.
One P2 correctness issue: snapshot now passes canonical render IDs into a visibility consumer that still compares raw DOM IDs. Its post-injection sync can hide a successfully extracted active video before capture. See the inline source/control example.
Source-only review: browser/media execution, tests, builds, typechecking, and CI were not run. Author-reported pixel/test results were not treated as independently verified.
— Review by tai (pr-review)
Verdict: REQUEST CHANGES
Reasoning: The canonical-ID migration does not reach the final visibility consumer, so a previously working static composition can save snapshots with its active video hidden.
| const srcDur = Number.isFinite(v.duration) && v.duration > 0 ? v.duration : 0; | ||
| return { | ||
| id: v.id, | ||
| id: v.getAttribute(renderIdAttr) || v.id, |
There was a problem hiding this comment.
[P2] Keep the final visibility sync in this ID namespace too
candidate.id now contains the render key, but syncVideoFrameVisibility still checks active.has(video.id) (screenshotService.ts:892). Snapshot invokes it after injection (lines 841–847), so it hides the correctly injected image with visibility:hidden !important before recapture/saving whenever the two IDs differ.
This also regresses static input without duplicate authored IDs: put an untimed <img src="assets/still.png"> before a timed <video id="hf-media-0" src="assets/clip.mp4" data-start="0" data-duration="4" muted>. assignMediaRenderIds gives the idless image hf-media-0 and the video hf-media-0__hf2. At t=1 with successful extraction and visible ancestors, BASE injects/syncs using the video's unique raw ID; HEAD injects through the bridge using hf-media-0__hf2, then sync rejects raw hf-media-0 and hides it.
Make visibility use the same canonical reader/fallback and cover stamping → injection → sync. The new identity test stops at mocked page opening, so it cannot catch this final-state mismatch. This is source-level control analysis, not a browser/media reproduction.
There was a problem hiding this comment.
Confirmed and fixed at b0f6d5a, at the consumer: syncVideoFrameVisibility now matches window.__hfMediaId?.(video) || video.id, the same reader injection uses, so every caller is in the render-id namespace.
- Reproduced your static case with the real CLI (id-less untimed
<img>, then a red<video id="hf-media-0">, which gets render idhf-media-0__hf2). Pixel at t=1: 0.8.185253 0 0, this branch with the raw-id sync0 0 0(hidden), fixed253 0 0. - Render never showed it because it syncs before it injects, and the injection then shows the frame. Snapshot syncs after.
- New engine test covers stamping, then injection, then sync with the real bridge. It fails without the fix (
expected 'hidden' to be 'visible'). - Rebased onto main. Main retired
product-launch-video(feat(skills)!: publish seven knowledge skills aligned with Studio and retire the workflow skills #5651), so the skill-side changes are gone and the PR is now snapshot plus engine only. Title and body updated.
…render id differs
09196f4 to
b0f6d5a
Compare
What
hyperframes snapshotnow keys injected video frames the way render does, by the media render id (data-hf-render-id), not the element'sid.<video>elements that share an authored id get one frame each. Before, one of them showed nothing.idkeeps its injected frame visible through the visibility sync. Example: an untimed, id-less<img>earlier in the document takeshf-media-0, so a video withid="hf-media-0"gets render idhf-media-0__hf2.Why
Render stamps a document-unique
data-hf-render-idon every media element and addresses frames by it. Snapshot keyed them byvideo.id, so duplicate ids collapsed into one frame.Review found a second half (thanks):
syncVideoFrameVisibilitystill comparedvideo.idagainst the active set, while injection resolves through the render-id bridge. Snapshot syncs after it injects, so a stamped video's frame was hidden again before capture. Render syncs before it injects, and the injection then shows the frame, which is why render never showed it. The sync now reads the id through the bridge's__hfMediaId, the same reader injection uses (mediaRenderIdBridge.ts), with the plainidfallback for documents that were never stamped.What changed since the first version
Main retired the
product-launch-videoskill (#5651), so this PR no longer touches it. Its two skill-side fixes went with it: isolating each frame host, and unique ids for hoisted clips inassemble-index.mjs. Nothing on main hoists frame videos any more. What remains is general: snapshot's media identity, for any composition.Related work
Refs #5651 (retired the skill this PR first patched).
How
snapshot.ts: stamps render ids on the bundled HTML (assignMediaRenderIds), installs the engine's media render-id bridge before navigating, and passes each candidate's render id to injection and sync.screenshotService.ts:syncVideoFrameVisibilitymatcheswindow.__hfMediaId?.(video) || video.idagainst the active ids.engine/index.tsexportsinstallMediaRenderIdBridge.Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
screenshotService.test.ts, new: "keeps a frame injected by render id visible through the sync, as snapshot calls them". It installs the real render-id bridge, injects a frame forpip__hf2on a video withid="pip", then syncs. Red without the engine fix (expected 'hidden' to be 'visible'); green with it, 37 passed, 3 runs in a row.videoFrameInjector.test.ts: 18 passed.snapshot.test.ts, "keys two videos that share an author id apart, as the render path does": 77 passed.tsc --noEmitexit 0 inpackages/engineandpackages/cli. Oxlint, oxfmt, the comment ratchet and citations are clean, and the commit hooks pass.Built CLI, real snapshot at t=1 of the review's case (an id-less untimed
<img>, then a red<video id="hf-media-0">filling the frame), pixel in the video's area:video.idNot exercised: a render run of the same case. Render's sync-before-inject order already shows the frame there.