feat(player): slideshow controller + <hyperframes-slideshow> component

[vanceingalls]

Slideshow mode — 2/5: player controller & <hyperframes-slideshow>

The navigation layer. A framework-agnostic controller drives the existing <hyperframes-player> to make a continuous timeline behave as discrete slides, wrapped by a new web component with full presentation chrome. Builds on #1580.

Architecture

Three small units: a DOM-free SlideshowController (testable without a browser) drives the player through a minimal PlayerPort; the <hyperframes-slideshow> web component is thin glue (chrome + input + adapter); the player element is untouched except for an additive scenes getter.

Key changes

• SlideshowController.ts — next/prev/goToSlide; fragment hold-points (play-to-hold so each reveal animates in); a branch nav stack (enterBranch/back/backToMain, breadcrumb, per-sequence counter); resumeSlide to restore exact position; canPrev/canNext + nextSlide getters; dispose. Branch-edge nav: prev on a branch's first slide and next past its last slide return to the parent.
• hyperframes-slideshow.ts — one frosted control capsule bottom-right (mute + counter + conditional prev/next that hide when there's nowhere to go); floating pulse-glow hotspot overlays (region %, prefers-reduced-motion-aware, XSS-escaped); window-level keyboard (←/→/Space/Backspace) + touch swipe (only on dominant-horizontal); postMessage embed API (next/prev/goto/back); presenter mode (dual-window BroadcastChannel sync — audience follows, presenter shows next-slide + notes + elapsed timer); an optional sound attribute renders a mute toggle that emits hf-sound / reflects data-hf-muted (audio playback stays external). Init defers to a macrotask and waits for scenes (epoch-guarded against disconnect/reconnect races); invalid zero-length slides are dropped.
• hyperframes-player.ts + runtime-message-handler.ts — scenes getter populated from the runtime timeline message via a new setScenes callback.
• slideshowPresenter.ts — SlideshowChannel, buildPresenterLayout, formatElapsed.
• tsup gains a slideshow entry (dist/slideshow/… + ./slideshow package export); vitest config + test-setup (happy-dom, in-memory BroadcastChannel polyfill); declares @hyperframes/core and imports the /slideshow subpath.

Testing

~108 vitest tests across the controller and component (nav, fragments, branch stack, conditional buttons, hotspots, presenter sync, mute), DOM-driven under happy-dom.

Stack

core (#1580) → player → studio (#1582) → skill (#1583) → examples (#1584).

🤖 Generated with Claude Code

[vanceingalls]

• feat(studio): slideshow branching editor panel #1582
• feat(player): slideshow controller + <hyperframes-slideshow> component #1581 👈 (View in Graphite)
• feat(core): slideshow schema, parser, and lint rule #1580 : 1 other dependent PR (#1589 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

Review: feat(player): slideshow controller + component

Solid PR. The controller/component split is clean and the test coverage is thorough. I have a few findings organized by category — nothing blocking, but a couple worth discussing before merge.

---

Architecture (controller/component split) — looks good

The three-unit split (DOM-free SlideshowController, thin <hyperframes-slideshow> glue, untouched player element with additive scenes getter) is well-designed. The PlayerPort interface is the right abstraction — it keeps the controller testable without a browser and makes the player dependency swappable. The ControllerLike interface on the component side is equally clean.

One observation: ControllerLike has several optional methods (goToSlide?, enterBranch?, back?, backToMain?, dispose?) — this makes the test stubs flexible but also means the component has to null-check every call site with ?.(). If these are always present on the real SlideshowController, consider making them required in the interface and providing a NoopController for test stubs instead.

---

State management — generally solid, one race condition concern

Fragment hold-point model: The holdAt + onTime() pattern is clever — play-to-hold with the fragment index advancing only when the hold fires, not when next() is called. This avoids the classic off-by-one where the UI updates before playback catches up. Tests cover this thoroughly (Fix 8a/8b).

Branch stack: The StackFrame[] model with enterBranch/back/backToMain is clean. resumeSlide preserving the exact parent position is a nice touch.

Potential race: In onTime(), the hold-point check uses t >= this.holdAt - EPS with EPS = 0.001. If timeupdate fires rapidly (e.g., high-frequency timer or seek), could onTime fire twice for the same hold point? The this.holdAt = null on line ~939 clears it, but there's a window between the check and the clear where a second call could enter. In practice timeupdate events are usually 4-15Hz so this is unlikely, but a guard like const hold = this.holdAt; this.holdAt = null; if (hold === null) return; (move the null-set before the work) would be belt-and-suspenders.

---

Performance — a few things to watch

1. render() rebuilds innerHTML on every controller change. For a presentation with frequent fragment reveals, this means full DOM teardown + rebuild of the chrome overlay on every step. The chrome is small (a few buttons + counter), so this is probably fine in practice, but worth noting. If performance profiling ever shows jank during rapid navigation, consider diffing just the counter text and button visibility instead of full innerHTML replacement.

2. Event listeners re-attached on every render. Each render() call does this.chrome.innerHTML = ... then re-queries and addEventListeners for mute/prev/next/hotspots. The old listeners are implicitly GC'd when the old DOM nodes are replaced, so no leak — but it's worth a comment explaining this is intentional.

3. waitForScenes polling at 100ms intervals for up to 2500ms. This is fine for init, but if scenes never arrive (e.g., composition with no scenes), that's 25 setTimeout calls. Not a problem, just noting the bound.

4. presenterInterval = setInterval(() => this.render(), 1000) — re-renders the entire presenter layout every second just to update the elapsed timer. Consider updating only the timer text node instead of rebuilding the full presenter grid.

---

Accessibility — good foundation, a few gaps

Positive:

• aria-label on prev/next/mute buttons
• aria-pressed on mute toggle
• aria-label with slide counter on the counter span
• Form-control guard on keydown (Fix 6) prevents stealing input from text fields
• prefers-reduced-motion disables hotspot pulse animation

Gaps:

• No ARIA live region for slide transitions. Screen reader users won't know when the slide changes. Consider aria-live="polite" on the counter or an aria-roledescription="slideshow" on the container.
• The hotspot buttons have aria-label but no role="button" (they're <button> elements so this is implicit — fine).
• Keyboard navigation only supports arrow keys and space/backspace. No support for Home/End (first/last slide), Escape (exit branch), or number keys (jump to slide N). These are nice-to-haves, not blockers, but worth considering for the presentation use case.
• The tabIndex = 0 in connectedCallback makes the element focusable, but the keyboard handler is on window, not on the element. This means arrows work even when a different slideshow element is focused. If there are ever multiple slideshows on one page, they'd all respond to the same keypress.

---

Security — handled well

• escHtml() properly escapes &, <, >, " for attribute and text injection. Tests verify XSS vectors in hotspot IDs (Fix 1) and presenter notes (Fix 2).
• buildPresenterLayout in slideshowPresenter.ts has its own esc() function — consider importing the shared escHtml to avoid having two slightly different implementations (the presenter one doesn't escape ").

---

Lifecycle / init robustness — impressive

The init dance is thorough:

• Macrotask defer for parser-appended children (Fix Bug 1)
• initGeneration epoch counter to cancel stale inits on disconnect/reconnect (Fix 4/6/9)
• initInFlight guard with try/finally so malformed manifests don't leave the flag stuck (Fix 2)
• waitForReady with timeout so a player that never fires ready doesn't hang forever (Fix 5)
• disconnectedCallback nulls controller/offChange to prevent double-dispose (Fix 14)
• Chrome nulled on disconnect so reconnect re-appends (Fix 1)

This is well-engineered and the test coverage proves each edge case.

---

Minor nits

1. fallow-ignore-next-line comments (lines 201, 886, etc.) — are these from a custom linter? If so, consider whether the complexity warnings are worth suppressing or if the flagged methods could be simplified. The render() method in particular is ~120 lines of inline HTML template construction.

2. extractScenes in runtime-message-handler.ts casts raw as SceneRecord[] before filtering, which means the filter callback accesses .id, .start, .duration on potentially non-object array elements. If the array contains primitives (e.g., [null, 42]), accessing .id on them won't crash (returns undefined, fails the typeof check) but the cast is misleading. Consider (raw as unknown[]).filter(...) with a type guard instead.

3. Inline styles everywhere. The chrome and presenter layouts use extensive inline style attributes. This works but makes it hard to theme or override. If theming becomes a requirement, consider CSS custom properties or a <style> block in the chrome container (the --hf-slideshow-accent var on hotspots is a good start).

4. onmouseover/onmouseout inline handlers on buttons (lines ~3055-3066) — these are effectively inline JS strings in the HTML. They work but break CSP script-src policies that don't include 'unsafe-inline'. Since you're already attaching click handlers via addEventListener, consider doing the same for hover states, or use CSS :hover pseudo-class instead.

---

Verdict

Clean architecture, thorough testing (~108 tests), careful lifecycle management, and good security hygiene. The controller/component separation is textbook. The main areas for improvement are minor: innerHTML-based rendering performance (fine for now), accessibility live regions, and the inline event handlers for hover styles. None are blocking.

I'd recommend approval after considering the CSP concern with inline onmouseover/onmouseout handlers — that one could bite in production depending on the CSP policy of embedding pages.

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

Review — feat(player): slideshow controller + <hyperframes-slideshow> component

Reviewed at HEAD 2f9a23748034030ebec4776a382264f207bcaef3. Verdict: looks good, non-blocking findings layered on top of Miga's pass.

I read this PR alongside its alternative split form (#1589 + #1590) at Vance's request. Math: 866 (#1589) + 2381 (#1590) = 3247 (#1581) — exact, and per-file additions match exactly across all 12 files. The split is non-lossy.

Miga (review at 07:28Z) covered the major shape: innerHTML rebuild hot path, inline-handler CSP risk, accessibility live-region gap, BroadcastChannel namespacing, multi-instance window-listener concern, extractScenes cast-before-filter. I won't re-litigate those. Below are additions specific to this monolith form.

---

Additions to Miga's review

1. onTime() hold-clear ordering is already belt-and-suspenders. Miga flagged a potential race between the t >= holdAt - EPS check and holdAt = null. Reading SlideshowController.ts carefully: the code already does const hold = this.holdAt; this.holdAt = null; before the fragment-advance work (lines ~939 in the monolith). The reentrancy window Miga described is already closed. The remaining concern (rapid timeupdate fires) is fine because holdAt is null after the first check.

2. Inverse-asymmetry between enterSlide and resumeSlide (controller). enterSlide clears this.holdAt = null before the slide lookup; resumeSlide does not. If resumeSlide is called with an out-of-range (index, fragmentIndex) (degenerate manifest after a back()/backToMain() into a sequence whose slide was filtered), the early-return on if (!slide) return leaves a previously-set holdAt armed. Next onTime could fire a stale hold on a slide we just left. Unlikely in practice but trivially fixable: move this.holdAt = null above the slide-lookup in resumeSlide to mirror enterSlide. (Inverse-operation tolerance rubric.)

3. goToSlide() is sequence-local only. It clamps to slidesOf(this.frame.sequenceId). There's no API to jump across branches (e.g. goTo("intro", 3)). Probably intentional — branch nav is supposed to go through enterBranch — but worth a one-line docstring noting "current sequence only" so future consumers don't pattern-match it as a global jump.

4. CSP / inline-handler note: this is a worth-fixing concern, not a nit. The onmouseover/onmouseout inline strings on the chrome buttons will fail under any host page with Content-Security-Policy: script-src 'self' (no unsafe-inline). Hyperframes is shipped as an embeddable web component — assume hosts have strict CSP. Swap for :hover CSS pseudo-class or addEventListener('mouseenter'/'mouseleave'). Same pattern already used for click listeners, so it's a one-line swap.

5. escHtml vs slideshowPresenter.esc() divergence. Miga noted these are two slightly different implementations. The presenter esc() doesn't escape " — which is fine for text-node insertion but not for attribute values. If buildPresenterLayout ever puts an escaped string inside style="..." or data-x="...", that's a latent attribute-injection seam. Consolidate.

---

Architecture verdict (vs split form)

The 3-unit shape (DOM-free controller + thin component glue + additive player getter) is clean either way. The monolith is reviewable in one pass; the split (#1589 controller + #1590 component) cleanly separates the state-machine layer from DOM. For the split, see my note on a real package.json export hazard at #1589 — the slideshow export entry lands without the corresponding tsup entry/build output. That's the single load-bearing reason to prefer the monolith if you're ever going to land #1589 alone, but Vance has confirmed both PRs land together, so it's not a blocker for the split path either.

If I were picking, I'd lean split — the PlayerPort boundary is the genuinely interesting seam to lock in independently, and #1589's controller is unit-testable without happy-dom (faster CI, easier reasoning). The monolith form is fine and shippable; the split is incrementally better.

Review by Rames D Jusso

The base branch was changed.