fix(studio): mute composition hover previews

[miguel-heygen]

Summary

Studio composition cards can no longer start a second audible stream while the main preview is playing. Hover previews are muted before playback begins, preserving the main player as the only audible preview source.

The regression test reproduces the prior overlap by starting an initially unmuted card player and verifies muting occurs before playback. Source report: https://heygen.slack.com/archives/C0BGC335AQY/p1784118425210739

Test plan

• bun run --cwd packages/studio test -- CompositionsTab.test.ts
• bun run --cwd packages/studio typecheck
• bunx oxlint packages/studio/src/components/sidebar/CompositionsTab.tsx packages/studio/src/components/sidebar/CompositionsTab.test.ts
• bunx oxfmt --check packages/studio/src/components/sidebar/CompositionsTab.tsx packages/studio/src/components/sidebar/CompositionsTab.test.ts

---

[james-russo-rames-d-jusso]

Reviewed at 3e637dd7.

Small, well-scoped fix: syncIframePlayback now sets player.muted = true on the shouldPlay branch before calling player.play?.(), which mutes hover-preview cards so the main player stays the only audible source.

Blockers

(none)

Concerns

(none)

Nits

🟡 Regression test doesn't pin call ordering. The test asserts player.muted === true AND player.play was called, but the mock doesn't verify the mute assignment happened before the play call. If a future refactor swapped the order (play() then muted=true), the assertion would still pass while the audible-overlap regression returned in cases where the player samples muted synchronously in play(). A vi.spyOn capturing the sequence, or a muted setter that flips a "was muted before play" flag the play mock reads, would lock the ordering in.

Green notes

🟢 __player.muted?: boolean type is right. Optional keeps the interface tolerant of iframes exposing a partial __player (or older builds that don't support mute). The unconditional assignment inside syncIframePlayback is safe — writing an unknown property is a no-op on any object shape.

🟢 Idempotent + persistent. muted=true persists across play/pause/play cycles because the same __player object lives for the iframe's lifetime; re-entering the shouldPlay branch just re-asserts it. New iframe → new __player → un-muted initial state → first play mutes it before starting audio. Correct lifecycle handling.

🟢 Single caller (line 140's retry loop). No other consumers of syncIframePlayback grepped in the studio package, so the export-widening for the test is minimal blast radius.

Verdict framing

Clean fix, correct semantics, minimal surface change. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

RIGHT direction, wrong mechanism — the fix does not actually mute the hover-preview audio. Field-reported bug is real and small in scope, but player.muted = true on the composition-iframe __player is a no-op at runtime. This should REQUEST_CHANGES until the mute path actually flips DOM <video>/<audio> state (or webAudio) inside the sub-composition iframe.

Runtime-interop lens; no prior reviews at head 3e637dd. Rames may cover the test-coverage / observability lens separately.

Findings

P0 blocker — player.muted = true is inert on the __player compat wrapper.

file:packages/studio/src/components/sidebar/CompositionsTab.tsx:97

The __player object inside the sub-composition preview iframe is constructed by createPlayerApiCompat(...) in packages/core/src/runtime/init.ts:2232-2272 and assigned at init.ts:2272. It is a plain object literal that pulls only play / pause / seek / getTime / getDuration / isPlaying and a set of no-op stubs off the underlying transport — no muted field, no muted getter/setter, no proxy. Assigning player.muted = true from the parent studio therefore adds an ordinary JavaScript property to that object that no runtime code reads.

Actual mute inside the preview iframe happens via one of two paths:

• The parent-to-iframe control bridge in packages/core/src/runtime/bridge.ts:61 ("set-muted": (data, deps) => deps.onSetMuted(Boolean(data.muted))), whose handler in packages/core/src/runtime/init.ts:2303-2312 mutates every <video> / <audio> element (el.muted = effective || el.defaultMuted) and calls webAudio.setMuted(effective).
• The <hyperframes-player> custom-element host, when the iframe is nested inside one — see packages/player/src/hyperframes-player.ts:425 (set muted(m: boolean) { ... setAttribute("muted", ...) }), which propagates via _setIframeMediaMuted at hyperframes-player.ts:534.

The CompositionsTab preview is a raw <iframe> (no <hyperframes-player> shadow host), so only the bridge path applies. player.muted = true doesn't touch the bridge.

The codebase already has the correct helper. setPreviewMediaMuted(iframe, muted) at packages/studio/src/player/lib/timelineIframeHelpers.ts:114-124 picks the right mechanism (custom-element host if present, postMessage set-muted otherwise) and is used by the main preview at packages/studio/src/player/hooks/useTimelinePlayer.ts:41,247. The fix should call this helper on the hover-play branch of syncIframePlayback instead of writing a plain property.

Sketch:

if (shouldPlay) {
  setPreviewMediaMuted(iframe, true);  // real mute via bridge
  player.play?.();
  return true;
}

P0 blocker — the regression test asserts on a mock that would pass a broken fix.

file:packages/studio/src/components/sidebar/CompositionsTab.test.ts:57-71

The test mocks __player as { muted: false, play: vi.fn() } — a plain object with a writable muted field. It verifies player.muted === true after syncIframePlayback(iframe, true). But the real runtime __player (see finding #1) has no such field on any read side. The test passes today, and it would keep passing regardless of whether the fix ever propagates to DOM media. It's a "the code did what the code did" test, not "audio was actually muted."

Under Miguel's parameterization / regression-coverage rule, this needs a test that exercises the actual mute channel — either asserting postRuntimeControlMessage(..., "set-muted", { muted: true }) is invoked before play, or asserting that <video>/<audio> elements in a JSDOM iframe end up with .muted === true after syncIframePlayback(...). The current test does neither.

Adversarial pass (assuming the mechanism is corrected)

Once setPreviewMediaMuted is in play, the remaining lifecycle/state concerns look clean:

• Re-mount / iframe reload — onLoad at CompositionsTab.tsx:203-213 already re-invokes requestIframePlaybackSync(hovered), so a fresh iframe re-mutes on the same trigger. Fine.
• Bridge-not-ready race — the runtime posts { type: "ready" } on bridge install (bridge.ts:129); if set-muted is posted before that, the parent must retry. The existing 10 × 100ms sync retry loop in requestIframePlaybackSync covers the ready-window, but it currently keys off syncIframePlayback returning true (which requires player present). With setPreviewMediaMuted, the mute call is fire-and-forget — worth confirming the mute is retried alongside play, not just done once and forgotten.
• Cleanup / unmount mid-retry — useEffect return at CompositionsTab.tsx:171-176 clears both timers. Pre-existing, not touched by this PR.
• Hover jitter during drag / rapid pointer moves — 300 ms hover-enter debounce plus single-card hovered state means only one preview plays at a time in normal use. Not a new failure mode.

Extrapolation

Searched studio for sibling surfaces that talk to the sub-composition __player:

• packages/studio/src/utils/studioPreviewHelpers.ts:339 (pauseStudioPreviewPlayback) — uses __player.pause, no mute.
• packages/studio/src/hooks/timelineTrackVisibility.ts:101-103 — uses __player.seek, no mute.
• packages/studio/src/App.tsx main preview — mutes via setPreviewMediaMuted (correct path).

No other site currently writes __player.muted. Once this PR uses the helper, the pattern is consistent across studio. If the fix ships as-is with a plain property write, that anti-pattern would risk being copied at other hover-preview sites (context menus, playlists, etc.) as new features land.

Strengths

• Clean extraction of syncIframePlayback from its enclosing hook makes the unit-testable seam. Once the mechanism is right, the shape is easy to test.
• Cited the source Slack field report in the PR body — makes the regression path traceable.
• Retry loop already exists for the "player not yet ready" ready-window; correcting the mechanism doesn't require reworking that structure.

Verdict: REQUEST CHANGES
Reasoning: The fix writes a property no runtime code reads; the regression test verifies the write, not the mute. Real bug remains open at ship. Use setPreviewMediaMuted from packages/studio/src/player/lib/timelineIframeHelpers.ts.

— Via (runtime-interop lens)

[vanceingalls]

Reconciliation with @james-russo-rames-d-jusso's LGTM (posted while I was preparing mine, so I didn't cite it in my main review).

Rames read the assignment as "writing an unknown property is a no-op on any object shape" — which is type-safe (correct), but the PR's goal is to mute audio, and I don't think it does. I verified the runtime __player (constructed by createPlayerApiCompat at packages/core/src/runtime/init.ts:2232-2272, assigned at init.ts:2272) is a plain object literal with no muted field on any read side — the actual mute path is the parent-to-iframe set-muted bridge (packages/core/src/runtime/bridge.ts:61 → init.ts:2303-2312 DOM <video>/<audio> mutation + webAudio.setMuted). The codebase already exposes the right helper: setPreviewMediaMuted at packages/studio/src/player/lib/timelineIframeHelpers.ts:114-124, used by the main preview at useTimelinePlayer.ts:247.

Rames' green note on lifecycle ("new iframe → new __player → un-muted initial state → first play mutes it before starting audio") describes what the fix intends, but the middle step "first play mutes it" doesn't currently happen because the mutation never propagates to the DOM media elements or the webAudio node.

Rames' ordering nit (Nits section) is orthogonal to my blocker and still valid — worth locking in once the mechanism is corrected.

— Via (runtime-interop lens)

[miguel-heygen]

player.muted = true on the composition-iframe __player is inert.

Addressed at eb64a8108: syncIframePlayback now calls the existing setPreviewMediaMuted bridge before player.play(), so the parent sends the real set-muted runtime control message. The regression now uses the actual postMessage channel and asserts the ordered sequence mute then play.

Validation: targeted test 7/7, Studio typecheck, formatting, lint, fallow, and tracked-artifact checks all pass.

[james-russo-rames-d-jusso]

R1 (3e637dd7) → R2 (eb64a810) delta: 3 lines in CompositionsTab.tsx + 8 lines in the regression test. Reviewed at eb64a810.

R1 nit status

🟢 Ordering is now pinned in the test. The mute call and the play call each push to a shared calls: string[] array, and expect(calls).toEqual(["mute", "play"]) locks the sequence. A future refactor that reordered them would fail loud. ✓

Delta verification

🟢 Right primitive. Swapping player.muted = true (direct property write on contentWindow.__player.muted) for setPreviewMediaMuted(iframe, true) routes through the existing runtime control channel — the same well-known postRuntimeControlMessage(..., "set-muted", { muted }) path used by useColorGradingController, useElementPicker, and hyperframes-player.ts. This is a strictly better shape than R1: same-shadow-root hosts get direct host.muted = true (fast path in setPreviewMediaMuted), and cross-origin / non-shadow-root iframes get the postMessage bridge — R1's direct property write would have silently failed on cross-origin contentWindow access (caught by the outer try/catch, but no mute).

🟢 __player.muted?: boolean removed from the interface. Correct — the bridge owns mute state now; the previous type field was documenting a property that we've stopped writing to. No stragglers reading it (grepped __player.muted in packages/: zero hits post-PR).

🟢 Receiving side already handles set-muted. packages/core/src/runtime/init.test.ts:1706-1724 verifies the iframe runtime processes {action: "set-muted", muted: bool} for both directions; hyperframes-player.ts:202 uses the same action on its side. Documented in packages/core/src/runtime/README.md:31. Well-established protocol, not a new surface.

🟢 CI. Full check rollup in progress at the head SHA; Magi confirmed targeted 7/7, studio typecheck, format/lint/fallow, hooks all green pre-push.

Verdict framing

Clean fix of the ordering concern from R1 — and the primitive upgrade (bridge vs. direct property write) makes this more correct across the cross-origin case too. LGTM from my side once the fresh CI settles.

— Review by Rames D Jusso

[vanceingalls]

R2 verify at head eb64a810.

Both R1 blockers cleanly resolved. Approving.

F1 — real bridge wire ✓

packages/studio/src/components/sidebar/CompositionsTab.tsx syncIframePlayback:

if (shouldPlay) {
  setPreviewMediaMuted(iframe, true);
  player.play?.();
  return true;
}

Mute is called BEFORE play, on the same tick. setPreviewMediaMuted (packages/studio/src/player/lib/timelineIframeHelpers.ts:114) does the right thing on both paths: shadow-DOM host → synchronous host.muted = true; plain iframe → postRuntimeControlMessage(iframe.contentWindow, "set-muted", { muted: true }) (runtimeProtocol.ts:32).

Full chain traced end-to-end at this SHA:

1. postMessage({ source: "hf-parent", type: "control", action: "set-muted", muted: true }, "*")
2. bridge.ts:61 routes "set-muted" → deps.onSetMuted(Boolean(data.muted))
3. init.ts:2303-2312 onSetMuted sets state.bridgeMuted = true, webAudio.setMuted(effective), and iterates every <video>/<audio> DOM element setting el.muted = effective || el.defaultMuted.

Audio is actually muted at the runtime, not just on the compat wrapper.

F2 — real assertion ✓

packages/studio/src/components/sidebar/CompositionsTab.test.ts:

const postMessage = vi.fn(() => calls.push("mute"));
const player = { play: vi.fn(() => calls.push("play")) };
const iframe = {
  contentWindow: { __player: player, postMessage },
  getRootNode: () => ({}),
} as unknown as HTMLIFrameElement;

expect(syncIframePlayback(iframe, true)).toBe(true);
expect(postMessage).toHaveBeenCalledWith(
  expect.objectContaining({ action: "set-muted", muted: true }),
  "*",
);
expect(calls).toEqual(["mute", "play"]);

The fixture exercises the real postMessage bridge path (getRootNode: () => ({}) bypasses the shadow-DOM shortcut, forcing postPreviewControl). The assertion is load-bearing: renaming action → type, dropping muted, reordering the play/postMessage calls, or forgetting the mute entirely all fail the test. Not aspirational — actually reproduces the failure mode R1 flagged.

Adversarial pass — clean

• Retry loop preserves mute. Every sync(remainingAttempts) iteration re-calls syncIframePlayback(iframe, true), which re-invokes setPreviewMediaMuted(iframe, true) before play. If the first attempt hits before __player exists, subsequent retries still mute before playing.
• Two hovers in succession. requestIframePlaybackSync clears the pending syncTimer first, then re-syncs from the latest hovered state. No stale unmute wins.
• Unmount cleanup. Card unmount tears the iframe out of the DOM alongside its child-frame state; nothing to leak. handleLeave takes the pause branch and never actively unmutes, which is fine — the iframe is either about to be reused for another hover (which re-mutes) or removed.
• postMessage race window. Inherent to the bridge design (postMessage is async into the child), same window that gates the main-timeline preview's mute. Not a regression introduced here; if we want to eliminate it we'd need a bridge redesign, not a fix in this PR.

CI at this SHA. Preflight (lint + format), Build, Lint, Fallow audit, Format, Test: runtime contract, SDK unit+contract+smoke, Preview parity, Test: skills all green. Regression shards + platform tests still pending — not blockers for the mechanism review.

Peer state. Rames reviewed at old head 3e637dd (COMMENTED), not yet at eb64a810. No overlap.

— Via