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

[vanceingalls]

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

[vanceingalls]

• fix(slideshow): address code-review findings #1580-1584 #1585
• test(examples): slideshow demos — airbnb deck, startup pitch, fixture #1584
• docs(skill): slideshow authoring guidance + standalone harness reference #1583
• feat(studio): slideshow branching editor panel UI #1592
• feat(studio): slideshow manifest persistence + panel helpers #1591
• feat(player): <hyperframes-slideshow> web component + presenter #1590 👈 (View in Graphite)
• feat(player): slideshow controller state machine #1589
• feat(core): slideshow schema, parser, and lint rule #1580 : 1 other dependent PR (#1581 )
• main

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

[miga-heygen]

PR Review: <hyperframes-slideshow> Web Component + Presenter

Solid piece of work — the custom element is well-structured, tests are thorough (1650 lines covering lifecycle, XSS, presenter mode, swipe, keyboard, form-control guards, epoch cancellation, phantom slide filtering), and the presenter/audience channel split via BroadcastChannel is clean. A few things worth discussing before merging:

---

1. innerHTML re-rendering on every state change

render() sets this.chrome.innerHTML = ... on every call, which tears down and rebuilds the entire chrome DOM tree + re-attaches all event listeners (hotspots, prev/next/mute buttons). For a slideshow deck with many hotspots or frequent onChange callbacks, this creates unnecessary GC pressure and can cause visual flicker.

Consider either:

• Targeted DOM updates (update counter text, toggle button visibility) for the hot path, full rebuild only when slide identity changes.
• Or at minimum, a dirty-check: skip render() if the serialized state hasn't changed.

The 1-second setInterval in presenter mode also calls render() just to update the elapsed timer — that's a full innerHTML rebuild every second for a single text node change.

2. Inline styles via template strings — CSP and maintenance

All styling is inline (style="...") with hover effects via onmouseover/onmouseout inline handlers. This has two concerns:

• CSP: Sites with Content-Security-Policy that forbid unsafe-inline will break the hover interactions. The inline style attributes may also be blocked under strict CSP.
• Maintenance: The style strings are 200+ chars each and duplicated across prev/next/mute buttons. Extracting to a shared <style> block (or the already-used injectKeyframesOnce pattern) would be cleaner.

3. Window-level event listeners

The component registers keydown and message listeners on window. If multiple <hyperframes-slideshow> elements exist on the same page, ALL of them will respond to the same keypress or postMessage. The onKey handler doesn't check if this element is the intended target — the last-connected element wins on prev()/next() calls from all of them.

Worth adding either:

• A guard that only the focused/active slideshow responds.
• Or documenting that only one <hyperframes-slideshow> per page is supported.

4. extractScenes type assertion

In runtime-message-handler.ts:

return (raw as SceneRecord[]).filter(
  (s) => typeof s.id === "string" && typeof s.start === "number" && typeof s.duration === "number",
);

The as SceneRecord[] cast happens before the filter, so if raw contains non-object items (e.g. [null, 42, "string"]), accessing .id, .start, .duration on them will silently produce undefined comparisons rather than throwing. This works by accident (the typeof checks catch it), but a more defensive approach would be:

return (raw as unknown[]).filter(
  (s): s is SceneRecord =>
    s != null && typeof s === "object" &&
    typeof (s as SceneRecord).id === "string" && ...
);

5. BroadcastChannel — no channel name namespacing

The channel name is hardcoded to "hf-slideshow". If two different slideshow decks are open in different tabs, the audience window of deck A could receive presenter commands from deck B. Consider including a deck/session identifier in the channel name (e.g. hf-slideshow:${deckId}).

6. dropInvalidSlides — sequences type reconstruction

const sequences: ResolvedSlideshow["sequences"] = {};
for (const [id, seq] of Object.entries(show.sequences)) {
  sequences[id] = { ...seq, slides: seq.slides.filter(validSlide) };
}

This spreads seq but only overrides slides. If ResolvedSlideshow["sequences"] has nested mutable state, this is a shallow copy — mutations to the returned object's inner fields would affect the original. The test verifies the top-level slides array isn't mutated, but not the sequences. Minor, but worth noting since the function's docstring says "does not mutate the input."

7. present() URL construction

const sep = location.search ? "&" : "?";
window.open(location.href + sep + "mode=audience", "_blank");

If the current URL already has mode=audience (e.g. someone clicks present on an audience window), this appends a second mode=audience param. A URLSearchParams-based approach would be more robust.

8. // fallow-ignore-next-line complexity

There are several fallow-ignore directives. Are these from a linter? If the complexity warnings are firing, it might be worth considering whether the flagged methods (init, render, onKey, onMessage, handleRuntimeMessage) can be decomposed. The render() method in particular is ~110 lines of mixed logic + template strings.

9. Mute toggle accessibility

The mute button re-renders via full innerHTML rebuild, which means screen readers lose focus tracking when the button is clicked. After clicking mute, the button element is destroyed and recreated — focus moves to the document body. Consider preserving focus on the mute button after toggle, or using setAttribute to update aria-pressed in place rather than rebuilding.

10. Missing observedAttributes / attributeChangedCallback

The component reads mode and sound attributes but doesn't declare static get observedAttributes(). This means if these attributes are changed dynamically after initial render (e.g. el.setAttribute("sound", "")), the component won't react. Currently this works because __setControllerForTest triggers a re-render, but in production use, setting sound after mount would have no visible effect until the next slide change.

---

What's good

• XSS escaping (escHtml) on all user-supplied strings in innerHTML — tested with injection payloads.
• Epoch generation counter to cancel stale async init — solves the disconnect-during-waitForScenes race cleanly.
• dropInvalidSlides as a pure function with immutable semantics — good seam for testing.
• Presenter/audience split via BroadcastChannel is a pragmatic V1 approach.
• Touch gesture filtering (dominant-axis check) prevents accidental navigation during vertical scroll.
• Form-control guard on keyboard navigation prevents arrow keys from hijacking input fields.
• prefers-reduced-motion media query for the hotspot pulse animation.
• The test coverage is genuinely thorough — lifecycle, XSS, race conditions, form controls, presenter mode, phantom slides.

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

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

Reviewed at HEAD 86eea2ba18d16c7e90fbeb8e052d647601e0a32f. Verdict: clean component layer, layered findings on top of Miga.

Part B of the 2-PR split of #1581. Reviewed alongside #1589 (controller) at Vance's request.

Split verification — coverage vs #1581

This PR adds the DOM + presenter layer on top of #1589's SlideshowController. Per-file addition counts match #1581 exactly:

File	#1590 adds	#1581 adds	Δ
slideshow/hyperframes-slideshow.ts	546	546	0
slideshow/hyperframes-slideshow.test.ts	1484	1484	0
slideshow/slideshowPresenter.ts	93	93	0
hyperframes-player.ts	9	9	0
runtime-message-handler.ts	12	12	0
runtime-message-handler.test.ts	1	1	0
tsup.config.ts	1	1	0

Combined with #1589, the split exactly reproduces #1581 (3247 LOC). Non-lossy.

Boundary check

Imports from #1589 are tight:

• import { SlideshowController, type PlayerPort } from "./SlideshowController" — type-only PlayerPort, instance-only SlideshowController.
• No re-declaration of controller types.
• dropInvalidSlides correctly lives in this PR (hyperframes-slideshow.ts), not leaked to the controller layer.
• runtime-message-handler.ts does NOT import from slideshow/ — clean. Only the test file imports dropInvalidSlides from the slideshow path, which is fine for happy-dom integration tests.

Findings (layered on Miga's pass)

Miga (review at 07:28Z) covered: innerHTML rebuild perf, CSP/inline-handler concern, window-level keydown multi-instance hazard, extractScenes cast-before-filter, BroadcastChannel namespace, dropInvalidSlides shallow-copy semantics, present() URL building, mute toggle focus loss, missing observedAttributes. Adding:

1. tsup.config.ts now bundles slideshow/hyperframes-slideshow.ts — this fixes the export hazard from #1589. Note in #1589's review: package.json adds a ./slideshow export pointing at ./dist/slideshow/… that doesn't get built until this PR's tsup change. Cumulative-diff is fine; the per-PR window is the problem. If you can hoist the package.json exports change here (and out of #1589), the split becomes self-consistent at every revision. Worth coordinating.

2. MessageHandlerCallbacks.setScenes is required, not optional. runtime-message-handler.ts adds setScenes: (scenes: SceneRecord[]) => void (no ?) to the callbacks interface. Any existing consumer of handleRuntimeMessage outside this file/PR who didn't supply setScenes will fail typecheck. Grep the repo / app for other callers — if there are none, ignore; if there are, either default-implement here or make it optional with ? so existing call sites don't have to touch.

3. ControllerLike interface has too many ? optional methods. Miga noticed this. To name them concretely: goToSlide?, enterBranch?, back?, backToMain?, dispose?. The component then null-checks every site with ?.(). Since the real SlideshowController (from #1589) implements all of these unconditionally, the optional shape is purely for test stubs. Cleaner: require them and provide a NoopController (3-line helper) for test stubs. Eliminates ~6-8 ?.() call sites and the silent-no-op risk.

4. Window-level listener concern (Miga's #3) compounds with shadowDOM absence. The component doesn't use Shadow DOM, so light-DOM styles can leak in/out AND the window keydown listener captures globally. If you ever embed two <hyperframes-slideshow> elements on one page (a docs-site comparison view, or a teacher-monitoring layout), both respond to every arrow key. A currentActive/document.activeElement.closest guard or moving the listener to this.addEventListener('keydown', …) (with tabIndex=0 already set) would scope it correctly.

5. present() security: child window inherits noopener? window.open(location.href + sep + "mode=audience", "_blank") — no noopener,noreferrer rel features. The audience window gets a reference back via window.opener, and any third-party JS on the deck URL could navigate the presenter window via that reference. Pass "noopener,noreferrer" as the third arg, or windowFeatures = "noopener". Cheap.

6. BroadcastChannel name "hf-slideshow" (Miga's #5) — concrete fix. Namespacing by deck identity is the standard pattern. Suggest:

const channelName = `hf-slideshow:${this.id || this.dataset.deckId || "default"}`;

This way two decks open in separate tabs don't cross-talk. Trivial change, prevents the "presenter for deck A drives audience of deck B" footgun.

7. extractScenes: extra failure mode beyond Miga's call-out. The function silently drops the entire list if raw isn't an array, but doesn't log/warn. If a manifest evolves to send {scenes: {…}} (object-shape) instead of an array, scenes go silently empty and the slideshow has nothing to drive. Add a one-line console.warn or expose via the callbacks so the host can detect malformed-manifest cases.

Test coverage spot-check

hyperframes-slideshow.test.ts is 1484 LOC. Lifecycle, XSS, phantom-slide filter, presenter sync, mute, keyboard, swipe, form-control guard. Genuinely thorough. The __setControllerForTest seam is pragmatic — better than fully exposing controller as public.

Review by Rames D Jusso

The base branch was changed.

[miga-heygen]

Re-review — checking prior findings against current HEAD

HEAD is b2acf349d339 — no new commits since the initial reviews (my pass + Rames'). None of the findings have been addressed yet; the code is unchanged. Tracking status below for clarity.

---

Still open

1. innerHTML full rebuild on every render() — this.chrome.innerHTML = hotspotsHtml + navClusterHtml still runs on every call. The 1-second setInterval in presenter mode still rebuilds the entire chrome DOM just to update the elapsed timer text node. This tears down and re-creates all event listeners, causes GC pressure, and breaks accessibility focus tracking (screen readers lose position when the mute button they just clicked is destroyed and recreated).

2. Inline onmouseover/onmouseout handlers — All nav buttons (prev, next, mute) still use inline event handler attributes for hover effects. Blocked by Content-Security-Policy without 'unsafe-inline'. The injectKeyframesOnce pattern already exists for shared styles — the hover states could move into that same injected <style> block using :hover selectors on [data-hf-prev], [data-hf-next], [data-hf-mute].

3. Window-level keydown/message listeners — multi-instance conflict — Listeners are registered on window with no guard for which element is the intended target. Two <hyperframes-slideshow> elements on the same page will both respond to every arrow key and every postMessage. The onKey handler could check document.activeElement?.closest('hyperframes-slideshow') === this or move the listener to this (with tabIndex=0 already set).

4. BroadcastChannel name "hf-slideshow" unhardened — Still hardcoded in slideshowPresenter.ts. Two different decks open in different tabs will cross-talk (presenter of deck A drives audience of deck B). Namespacing by deck identity (e.g. hf-slideshow:${this.id || this.dataset.deckId || 'default'}) would prevent this.

5. observedAttributes not declared — The component reads mode and sound attributes but does not declare static get observedAttributes() or implement attributeChangedCallback. Dynamic attribute changes after initial render (e.g. el.setAttribute('sound', '')) won't trigger re-renders until the next controller-driven render() call.

6. present() lacks noopener,noreferrer (Rames' finding) — window.open(location.href + sep + "mode=audience", "_blank") still has no third argument. The audience window gets window.opener reference, allowing any third-party script on the deck URL to navigate the presenter window. One-line fix: pass "noopener,noreferrer" as the third arg.

---

Severity assessment

Items 2, 3, 6 are the most actionable — small, targeted fixes with clear security or correctness impact. Items 1 and 5 are quality/robustness concerns that could ship as follow-ups. Item 4 is a V1 limitation worth documenting even if not fixed now.

No new issues found in this pass beyond what was already flagged.

Re-review by Miga