Skip to content

feat(player): add audio-locked attribute (force-mute + hide controls)#1234

Merged
miguel-heygen merged 1 commit into
mainfrom
feat/player-audio-locked
Jun 6, 2026
Merged

feat(player): add audio-locked attribute (force-mute + hide controls)#1234
miguel-heygen merged 1 commit into
mainfrom
feat/player-audio-locked

Conversation

@xiayewang-heygen
Copy link
Copy Markdown
Contributor

What

Adds an audio-locked boolean attribute (and audioLocked JS property) to <hyperframes-player>.

Why

The existing muted attribute is user-toggleable — the controls bar has a mute button + volume slider, so a viewer can always turn sound back on. We need a hard lock for host-mandated silent playback: when a HyperFrames project is embedded in a chat host (Claude.ai / ChatGPT), audio must stay off with no way for the viewer to enable it. (hyperframes.dev mounts the player without this attribute, so it plays normally.)

Behavior

When audio-locked is set, the player:

  • forces muted on,
  • hides the volume controls (mute button + slider) so there's no UI path to unmute,
  • re-asserts mute if anything clears the muted attribute while locked (covers raw removeAttribute, stray scripts, host control).

Removing audio-locked only lifts the restriction (unhides the controls) — it does not auto-unmute; callers manage muted explicitly after unlocking.

Changes

  • hyperframes-player.ts: new observed attribute audio-locked + audioLocked accessor; _handleMutedChange / _applyAudioLock helpers (attribute handling extracted to keep attributeChangedCallback complexity in check).
  • controls.ts: ControlsOptions.audioLocked + setVolumeControlsHidden() API; hides the volume wrap.
  • controls-setup.ts: threads audioLocked through.
  • README.md: documents the attribute + property.

Tests

7 new tests (force-mute, re-assert-on-unmute via property and raw removeAttribute, control hiding in both attribute orders, unlock restore). Full player suite green (126 tests); typecheck, oxlint, oxfmt --check, and fallow audit all clean.

Companion PRs

  • pacific (@pacific/apps-sdk-widgets) maps its audioLocked widget field → this audio-locked attribute.
  • experiment-framework emits audio_locked: true in MCP HyperFrames widget data.

🤖 Generated with Claude Code

muted alone is user-toggleable via the controls bar. audio-locked is a hard
lock for host-mandated silent playback (e.g. a HyperFrames project embedded
in a chat host): it forces muted on, hides the volume controls (mute button
and slider) so the viewer has no UI path to turn sound on, and re-asserts
mute if anything clears the muted attribute while locked.

Unlocking only lifts the restriction (unhides controls); it does not
auto-unmute, so callers manage muted explicitly after unlocking.

- new observed attribute audio-locked plus audioLocked JS property
- createControls gains an audioLocked option and setVolumeControlsHidden
- attribute handling extracted into helpers to keep complexity in check
- README documents the attribute and property
- 7 new tests; full player suite green, typecheck and oxlint clean

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

audio-locked ✅

Solid implementation. The design is clean and the edge cases are handled correctly.

What's good

  • Re-assertion loop is correct: when muted is cleared while locked, _handleMutedChange re-sets the attribute with val="" not null, so the guard doesn't re-fire — no infinite loop.
  • Both init orders (lock before controls, controls before lock) covered in tests.
  • Non-destructive unlock (doesn't auto-unmute) is the right call — callers control muted explicitly after releasing the lock.
  • 7 tests with clear scenarios; 126 green is a good signal.

Nits (non-blocking)

_applyAudioLockthis.muted = true unconditionally on lock. If the player was already muted, setAttribute("muted", "") is a no-op on the attribute (same old/new), so attributeChangedCallback doesn't fire and _media.updateMuted isn't called redundantly — that's fine. But the intent is marginally clearer with a guard:

if (!this.muted) this.muted = true;

Pure style, not a bug.


setVolumeControlsHidden restores with style.display = "" which resets to browser/stylesheet default — correct for a shadow-DOM div. Just noting it's intentional in case a future reviewer questions it.


Accessibility: when locked, the volume wrap is display: none (removed from a11y tree) but there's no signal on the player element itself that audio is host-locked. Screen reader users won't know it's permanently silenced vs just muted. Low priority for this PR but worth a follow-up issue — e.g., a [audio-locked] CSS hook the host can target, or an aria-description on the player.


Overall: implementation is correct, tests are solid. ✅

@miguel-heygen miguel-heygen merged commit f39b598 into main Jun 6, 2026
39 checks passed
@miguel-heygen miguel-heygen deleted the feat/player-audio-locked branch June 6, 2026 04:17
Copy link
Copy Markdown

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Choose a reason for hiding this comment

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

Hey Xiaye — clean PR for a security-sensitive feature. Good test coverage at the layer the feature operates on (7 cases, both attribute orders, lock + unlock, re-assert via property and removeAttribute), and the _handleMutedChange short-circuit on val === null && hasAttribute("audio-locked") is the right shape to avoid the "renderer briefly receives an unmute" race — the synchronous setAttribute("muted", "") re-fire keeps _media.updateMuted from ever being called with false while locked. No blockers, one concern about threat-model clarity, two smaller things, and some questions on the stack contract.

Concerns

  • The threat model isn't pinned down in the PR, and the protection's actual depth depends on it. The lock defends robustly at the <hyperframes-player> attribute layer — UI button, slider, removeAttribute("muted"), JS property write all re-assert correctly. But:

    • The shadow root is attachShadow({ mode: "open" }) (hyperframes-player.ts:79), so player.shadowRoot is reachable from outside.
    • _media is a ParentMediaManager instance whose underlying <video> / <audio> elements (parent-media.ts:247) live as m.el references inside _media._entries — created via document.createElement against the parent document, not the shadow root. They're not enumerable from querySelectorAll-on-shadow, but they're trivially reachable as player._media._entries.forEach(e => e.el.muted = false). JS-private (_media) is not runtime-enforced — #media would be.
    • Any same-realm script (host code, another widget on the same page, a stray injection) can therefore unmute by reaching past the player-attribute layer entirely. The _handleMutedChange re-assert never fires because the player's muted attribute hasn't changed.

    If the chat-host embed model is iframe-sandboxed (Claude.ai widget iframes typically are), realm isolation makes the above moot and the current implementation is exactly the right amount of defense. Pin that down in the PR body or in a code comment near _applyAudioLock so the next reader doesn't over-trust the lock. If the embed model includes same-realm surfaces, the next iteration would want either (a) #private class fields so _media isn't reachable from outside, (b) closed shadow root (loses devtools convenience, common tradeoff), or (c) a MutationObserver / periodic re-assertion watching the underlying el.muted directly (with the obvious noise/perf cost). My read is option (a) is free defense-in-depth regardless of which threat model wins — worth doing now.

  • Silent property-setter failure is debugging-hostile for legitimate unlockers. When locked, player.muted = false no-ops silently (the synchronous re-assert flips it back inside the same tick). A host script trying to unlock + unmute in a single tick has to know the order matters — set audioLocked = false first, then muted = false — and out-of-order calls fail with no signal. Two cheap fixes: (a) emit console.warn("[hyperframes-player] muted change ignored while audio-locked") in dev (gated on process.env.NODE_ENV !== "production" or equivalent player-side flag), and/or (b) document the required ordering in the README. The PR body says "callers manage muted explicitly after unlocking" but doesn't pin the sequence; pacific#28773's integration is going to want to know.

Nits

  • audio-locked="false" LOCKS (standard HTML boolean-attribute semantics — presence wins, value ignored). Worth a single test asserting this (it("locks regardless of attribute string value")) plus a README sentence — "the attribute behaves as a boolean: set via attribute presence (<player audio-locked> or setAttribute("audio-locked", "")), unset via removeAttribute. The string value is not interpreted." Pacific#28773 has to map audio_locked === truesetAttribute and === falseremoveAttribute; if it ever passes "false" through it'll silently lock. (nit)

  • No test that proves the "media engine is never told to unmute while locked" property explicitly. The current tests verify the attribute and control hiding stay in their expected state, but the safety claim that motivates the short-circuit (no _media.updateMuted(false) reaches the renderer while locked, even transiently) is implicit. A vi.spyOn(player._media, "updateMuted"); player.audioLocked = true; player.muted = false; expect(spy).not.toHaveBeenCalledWith(false); locks the contract against a future refactor reordering operations. Cheap and high-leverage given the security framing. (nit)

  • setVolumeControlsHidden(hidden: boolean) is a public method on the controls API that's only ever called by the audio-lock path. Exposing it as a general knob invites future drift between "audio-locked" and "controls-hidden" reasons. Either (a) inline the hide/show into a new method that names the audio-lock semantic (applyAudioLock(locked)), or (b) keep setVolumeControlsHidden but document at the type-level that it's reserved for the audio-lock use case. The README's "Audio-locked" comment (controls.ts:144) is good archaeology; reinforcing it in the API surface keeps the abstraction tight. (nit)

Questions

  • Embed model: are the chat-host embeds (Claude.ai, ChatGPT) always inside iframes, or sometimes inline? Determines whether concern #1 is "harden now" or "documented assumption."
  • iOS Safari: the test suite is headless jsdom-ish. iOS Safari has a few well-known quirks around <video muted autoplay playsinline> and programmatic muted setting — were the test plan items run on a real iOS device, or is that staging work the pacific integration will pick up?
  • Pacific contract: what's the exact mapping in pacific#28773? Specifically: does it pass through audio_locked: false as setAttribute("audio-locked", "false") or as removeAttribute("audio-locked")? (See nit #1 — this is the failure mode.) Worth a contract test on the pacific side; out of scope for this PR but flagging now while it's fresh.
  • Companion-PR sequencing: the stack lands player → pacific → experiment-framework. If the player ships first and the pacific mapping hasn't gone out yet, existing pacific embeds keep working as today (no audio-locked attribute, default unlocked). Lambda/CDN cache: any caching layer between deployments that might serve a stale player without the new attribute parsing while pacific is already passing audio_locked: true? If so, pacific's audio_locked: true becomes a silent no-op until both are deployed — fine semantically (the existing muted default still mutes) but worth confirming.

What I didn't verify

  • _media.updateMuted reaching all media surfaces uniformly (only the entries-loop in parent-media.ts:107 was visible; iframe-adopted media via _adoptIframeMedia was not traced through).
  • The control-message channel — _sendControl("set-muted", { muted }) — whether the downstream renderer/worker has any independent path to the underlying media that bypasses the short-circuit (unlikely from the diff, but the wire side wasn't read).
  • Whether MutationObserver already runs anywhere on the shadow root for unrelated reasons (would be a free hook for a deeper lock without per-frame cost).
  • The existence of a non-volume-slider keyboard shortcut (e.g. "M" on the player root) — controls.ts:231's keydown handler is on volumeSlider itself, which gets display: none when locked → not focusable → keyboard tampering through the slider is covered. The player root wasn't searched exhaustively.
  • iOS Safari live behavior.
  • Pacific + experiment-framework code (out of scope).

Review by Rames D Jusso

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.

3 participants