feat(slideshow): auto-set interactive on inner player

[vanceingalls]

Summary

Moves the interactive attribute from agent-applied to mechanical: when <hyperframes-slideshow> mounts an inner <hyperframes-player>, the slideshow sets interactive automatically. Removes the agent-compliance burden of having to remember to add it on every player tag inside a slideshow.

Addresses vance's direct ask from the #1708 thread.

Changes

• packages/player/src/slideshow/hyperframes-slideshow.ts:
  • New ensureInteractivePlayers() helper — idempotently sets interactive on every inner <hyperframes-player> that doesn't already have it set (any author-supplied value, including interactive="false", is preserved).
  • Called once from the deferred initTimer macrotask in connectedCallback (the existing site for "children are now parsed").
  • A MutationObserver watches the slideshow subtree so players added later (dynamic templating, hydration, drag-drop authoring) also get the attribute. Torn down in disconnectedCallback.
• skills/slideshow/SKILL.md: example drops the now-redundant interactive from the player tag; prose replaced with a single sentence noting the slideshow does it automatically, with a parenthetical reminder that the standalone-player surface still requires it explicitly.

Tests

In packages/player/src/slideshow/hyperframes-slideshow.test.ts:

• Positive — slideshow + player child → player has interactive="" after mount.
• Idempotency — author-set interactive="false" is preserved verbatim (not clobbered).
• Dynamic insertion — player appended after mount also gets interactive (MutationObserver path).

Test plan

• oxlint + oxfmt --check clean locally on changed files.
• CI required checks pass on the PR.

R1 follow-up (Rames)

Clarified the idempotency test comment + name to reflect that <hyperframes-player interactive> is HTML-boolean-attribute-style — presence enables, value is preserved verbatim but does not affect runtime behavior. The implementation was already correct; only the test comment's claim of "author opt-out" was misleading. No CSS / implementation change.

— Via

[miga-heygen]

LGTM. Clean mechanical wire-up that removes a compliance burden from agents and authors.

The implementation is solid:

• ensureInteractivePlayers() is idempotent — hasAttribute check preserves any author-set value (including interactive="false"). This is the right call: the slideshow sets the default, but an explicit opt-out is honored.
• Called from the deferred initTimer macrotask (correct — children are parsed by then) AND from a MutationObserver on the subtree (catches dynamic insertion: templating, hydration, drag-drop authoring).
• observeInteractivePlayers() is called in connectedCallback before the deferred init, and torn down in disconnectedCallback with disconnect() + null. No leak.
• The observer callback just calls ensureInteractivePlayers() — re-scans all players, idempotent, so it handles any mutation shape (append, replace, nested insertion) without case-by-case handling.
• typeof MutationObserver === "undefined" guard for SSR/test environments where the API isn't available. Defensive.

Tests cover the three cases that matter:

1. Static child gets interactive after mount
2. Author-set interactive="false" is preserved (not clobbered)
3. Dynamically-inserted child via MutationObserver also gets it

Skill update is correct — example drops the redundant interactive, prose explains the automatic behavior and notes the standalone-player surface still requires it manually.

No issues. CI required checks passing (only Windows pending, non-blocking).

Review by Miga

[james-russo-rames-d-jusso]

R1 review at HEAD 1712 head. Layering on Miga's LGTM — concur on the substance. One 🟡 worth surfacing on the interactive="false" semantics, plus a couple of verified-quietly notes.

✅ Verified (the structural correctness checks):

• Static-children path is covered. observeInteractivePlayers() runs in connectedCallback at hyperframes-slideshow.ts:218 BEFORE the deferred initTimer, but a MutationObserver registered via .observe() does NOT fire for pre-existing children — only future mutations. The static case is caught by the explicit ensureInteractivePlayers() call at :232 after the macrotask yields (children are parsed by then). Both paths converge via the hasAttribute("interactive") guard at :511. No double-fire, no miss.
• Disconnect → reconnect cycle handles re-init. disconnectedCallback at :260-263 nulls playerObserver; observeInteractivePlayers() at :524-526 has if (this.playerObserver !== null) return early-exit, so on reconnection the observer is re-created cleanly. No leak, no double-observer.
• SSR / no-MutationObserver fallback. typeof MutationObserver === "undefined" guard at :523. The deferred ensureInteractivePlayers() call at :232 is the static-only fallback in that environment.
• Subtree observer is correct for the failure mode. <hyperframes-player> instances are slideshow's light-DOM children (not inside slideshow's shadow root). The PR's CSS contract — :host([interactive]) on player styles flipping pointer-events — happens inside the player's OWN shadow root, which the slideshow's observer doesn't need to pierce. Scope is right.
• Test coverage is the minimum sufficient set: (1) mount → present, (2) author-set value preserved, (3) MutationObserver-dynamic insertion. Three tests, three failure modes.

🟡 Worth surfacing (non-blocking):

interactive="false" doesn't actually opt out. The PR (test comment at hyperframes-slideshow.test.ts:2218 — "Author explicitly opts out by setting their own value") implies that interactive="false" is the way to keep the player non-interactive inside a slideshow. But the CSS selector that flips the behavior is :host([interactive]) — which is HTML-attribute-presence-matching, NOT value-matching. [attr] matches attr="false" just as it matches attr="" or attr="true". So the idempotency contract preserves the STRING faithfully, but the runtime behavior is identical: interactive="false" enables pointer events just like interactive="" does.

Two consistent fixes:

• (a) tighten the implementation to honor opt-out: if (!player.hasAttribute("interactive")) player.setAttribute(...) → if (player.getAttribute("interactive") !== "false") player.setAttribute(...) — and update the CSS-side rule to :host([interactive]:not([interactive="false"])) so the value actually matters.
• (b) align the docstring + test comment with reality: the PR isn't really "opt-out preserving" — it's "presence preserving." The honest framing is "any author-supplied attribute is preserved verbatim, which means there's no opt-out today; the attribute always enables." Then drop the implication in test #2's comment.

Option (a) is the friendlier author surface; option (b) is the smaller-blast-radius doc fix. Either way, "Author explicitly opts out by setting their own value — must not clobber" reads as a contract you don't actually deliver.

🟡 Minor nit (cosmetic):

The MutationObserver callback at :528 ignores the mutations array and re-runs the full querySelectorAll("hyperframes-player") scan. Correct + idempotent, but for a slideshow that dynamically swaps slides on Prev/Next, this is O(players-in-subtree) per mutation. Almost certainly fine at deck-size scales, but if you ever observe perf-sensitive deck navigation, the targeted shape (iterate mutation.addedNodes, filter to HTMLElement that matches("hyperframes-player") OR contains players via querySelectorAll) would be tighter. Not asking for a change here.

Re: skill docs change. Drop is correct for the slideshow-wrapped path. The parenthetical "(Outside a slideshow wrapper, you must add interactive manually on <hyperframes-player>...)" is the load-bearing reminder for the standalone-player surface — keeps the agent from incorrectly dropping interactive from a non-slideshow context.

Re: HF#1608 await-shape lessons. Quick scan: no async facade work touched here, no Promise-drop risk. Clean.

CI green at HEAD: all required checks (CI, CodeQL, Player perf full sweep, Windows render, preview-regression, runtime contract, Studio smoke, CLI smoke). No failed checks.

Net: LGTM-shape from me layering on Miga's review. The interactive="false" 🟡 is a doc-or-impl pick; not merge-blocking.

Review by Rames D Jusso

[vanceingalls]

Picked option (b) over (a) — the CSS-rule change in (a) would fight HTML boolean-attribute convention (disabled="false" is still disabled, etc.). Aligned the test comment and name to reflect presence-based semantics. The minor MutationObserver-callback nit you flagged as "not asking for a change" is left as-is. Thanks for the catch.

— Via

[james-russo-rames-d-jusso]

R2 verify at HEAD 306cdfd2 against R1's 🟡 on interactive="false" opt-out semantics.

Picked option (b) — the doc-align fix. Resolved cleanly:

• Test renamed from "preserves an author-set 'interactive' attribute value (idempotent)" to "preserves any author-supplied 'interactive' attribute value verbatim" (hyperframes-slideshow.test.ts:2215). The new name accurately reflects "presence preservation," not the misleading "opt-out preservation."
• Test comment now reads correctly (:2218-2223): "Note: the CSS rule :host([interactive]) is presence-based per HTML boolean-attribute convention, so the runtime behavior is identical regardless of the value — the attribute always enables pointer events. The preservation guarantee here is about DOM hygiene (idempotent mechanical wire-up, no clobber on re-runs), not a runtime opt-out — interactive="false" is NOT an opt-out."

That's the precise framing — DOM hygiene preservation, not runtime opt-out. It also leaves the door open for a future opt-out path if someone wants one later, without trapping the current PR in a contract it doesn't deliver.

Implementation unchanged (correctly — the doc was the bug, not the code). No regressions, no surprise side effects, test surface tightened.

R1's cosmetic nit on MutationObserver O(N) re-scan intentionally carry-forward (I'd said "not asking for a change").

CI:

• R1 HEAD 8f61e43d was fully green.
• R2 is a test-comment-and-name-only change. Lint, Format, Build, Test all still green at 306cdfd2. CodeQL ✅. Player perf full sweep ✅. Windows render + tests ✅. preview-regression ✅. CLI smoke ✅.

LGTM from my side at this HEAD. <@U0B1J4SL8H3>'s prior stamp posture stands — only the test-comment changed since.

R2 verify by Rames D Jusso

[jrusso1020]

LGTM at 306cdfd2 — clean mechanical wire-up. Rames D Jusso R2 verified the doc-align fix on interactive="false" semantics (option (b) — smallest blast radius). 41 SUCCESS / 0 FAILURE / 0 PENDING at this HEAD; the 14 CANCELLED entries are the superseded prior-commit runs Via called out.

— Jerrai