feat(player): add audio-locked attribute (force-mute + hide controls)#1234
Conversation
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>
miguel-heygen
left a comment
There was a problem hiding this comment.
audio-locked ✅
Solid implementation. The design is clean and the edge cases are handled correctly.
What's good
- Re-assertion loop is correct: when
mutedis cleared while locked,_handleMutedChangere-sets the attribute withval=""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
mutedexplicitly after releasing the lock. - 7 tests with clear scenarios; 126 green is a good signal.
Nits (non-blocking)
_applyAudioLock → this.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. ✅
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
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), soplayer.shadowRootis reachable from outside. _mediais aParentMediaManagerinstance whose underlying<video>/<audio>elements (parent-media.ts:247) live asm.elreferences inside_media._entries— created viadocument.createElementagainst the parent document, not the shadow root. They're not enumerable fromquerySelectorAll-on-shadow, but they're trivially reachable asplayer._media._entries.forEach(e => e.el.muted = false). JS-private (_media) is not runtime-enforced —#mediawould 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
_handleMutedChangere-assert never fires because the player'smutedattribute 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
_applyAudioLockso the next reader doesn't over-trust the lock. If the embed model includes same-realm surfaces, the next iteration would want either (a)#privateclass fields so_mediaisn't reachable from outside, (b) closed shadow root (loses devtools convenience, common tradeoff), or (c) aMutationObserver/ periodic re-assertion watching the underlyingel.muteddirectly (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. - The shadow root is
-
Silent property-setter failure is debugging-hostile for legitimate unlockers. When locked,
player.muted = falseno-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 — setaudioLocked = falsefirst, thenmuted = false— and out-of-order calls fail with no signal. Two cheap fixes: (a) emitconsole.warn("[hyperframes-player] muted change ignored while audio-locked")in dev (gated onprocess.env.NODE_ENV !== "production"or equivalent player-side flag), and/or (b) document the required ordering in the README. The PR body says "callers managemutedexplicitly 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>orsetAttribute("audio-locked", "")), unset viaremoveAttribute. The string value is not interpreted." Pacific#28773 has to mapaudio_locked === true→setAttributeand=== false→removeAttribute; 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. Avi.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) keepsetVolumeControlsHiddenbut 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 programmaticmutedsetting — 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: falseassetAttribute("audio-locked", "false")or asremoveAttribute("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-lockedattribute, 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 passingaudio_locked: true? If so, pacific'saudio_locked: truebecomes a silent no-op until both are deployed — fine semantically (the existingmuteddefault still mutes) but worth confirming.
What I didn't verify
_media.updateMutedreaching all media surfaces uniformly (only the entries-loop inparent-media.ts:107was visible; iframe-adopted media via_adoptIframeMediawas 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
MutationObserveralready 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 onvolumeSlideritself, which getsdisplay: nonewhen 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
What
Adds an
audio-lockedboolean attribute (andaudioLockedJS property) to<hyperframes-player>.Why
The existing
mutedattribute 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-lockedis set, the player:mutedon,mutedattribute while locked (covers rawremoveAttribute, stray scripts, host control).Removing
audio-lockedonly lifts the restriction (unhides the controls) — it does not auto-unmute; callers managemutedexplicitly after unlocking.Changes
hyperframes-player.ts: new observed attributeaudio-locked+audioLockedaccessor;_handleMutedChange/_applyAudioLockhelpers (attribute handling extracted to keepattributeChangedCallbackcomplexity in check).controls.ts:ControlsOptions.audioLocked+setVolumeControlsHidden()API; hides the volume wrap.controls-setup.ts: threadsaudioLockedthrough.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, andfallow auditall clean.Companion PRs
@pacific/apps-sdk-widgets) maps itsaudioLockedwidget field → thisaudio-lockedattribute.audio_locked: truein MCP HyperFrames widget data.🤖 Generated with Claude Code