Skip to content

fix(cli): snapshot shows each video's own frame, keyed by render id as render does - #5740

Open
miguel-heygen wants to merge 4 commits into
mainfrom
fix/launch-video-hoisted-ids-and-layer
Open

miguel-heygen wants to merge 4 commits into
mainfrom
fix/launch-video-hoisted-ids-and-layer

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

What

hyperframes snapshot now keys injected video frames the way render does, by the media render id (data-hf-render-id), not the element's id.

  • Two <video> elements that share an authored id get one frame each. Before, one of them showed nothing.
  • A video whose render id differs from its id keeps its injected frame visible through the visibility sync. Example: an untimed, id-less <img> earlier in the document takes hf-media-0, so a video with id="hf-media-0" gets render id hf-media-0__hf2.

Why

Render stamps a document-unique data-hf-render-id on every media element and addresses frames by it. Snapshot keyed them by video.id, so duplicate ids collapsed into one frame.

Review found a second half (thanks): syncVideoFrameVisibility still compared video.id against 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 plain id fallback for documents that were never stamped.

What changed since the first version

Main retired the product-launch-video skill (#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 in assemble-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: syncVideoFrameVisibility matches window.__hfMediaId?.(video) || video.id against the active ids.
  • engine/index.ts exports installMediaRenderIdBridge.

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 for pip__hf2 on a video with id="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 --noEmit exit 0 in packages/engine and packages/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:

Build Pixel
0.8.185 (no render ids) 253 0 0 (red, correct)
this branch, sync by raw video.id 0 0 0 (frame hidden: the regression the review found)
this branch, sync through the bridge 253 0 0

Not exercised: a render run of the same case. Render's sync-before-inject order already shows the frame there.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 169 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 id hf-media-0__hf2). Pixel at t=1: 0.8.185 253 0 0, this branch with the raw-id sync 0 0 0 (hidden), fixed 253 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.

@miguel-heygen
miguel-heygen force-pushed the fix/launch-video-hoisted-ids-and-layer branch from 09196f4 to b0f6d5a Compare October 22, 2026 15:11
@miguel-heygen miguel-heygen changed the title fix(skills): keep hoisted launch-video clips on top and apart in snapshot fix(cli): snapshot shows each video's own frame, keyed by render id as render does Oct 22, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants