Skip to content

feat(slideshow): per-slide autoplay (manual-advance, opt-in)#1708

Open
jrusso1020 wants to merge 3 commits into
mainfrom
feat/slideshow-per-slide-autoplay
Open

feat(slideshow): per-slide autoplay (manual-advance, opt-in)#1708
jrusso1020 wants to merge 3 commits into
mainfrom
feat/slideshow-per-slide-autoplay

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

What

Opt-in per-slide autoplay for the slideshow presenter. When the presenter lands on a video slide, its <video> plays from the start. The slideshow still holds and never auto-advances — the presenter clicks Next when ready (PowerPoint/Keynote-style click-through, but the clip plays itself).

Motivation: the player renders the composition pointer-events:none, so a clip's own controls (native or custom) can't be clicked during present. Autoplay is how a video slide plays without a click.

What's in this PR (done + tested)

  • coreSlideRef.autoplay?: boolean, parsed and validated in parseSlideshow (a non-boolean autoplay rejects the manifest), carried through resolveSlideshow.
  • controller — optional PlayerPort.playSceneMedia(sceneId), fired only on forward enterSlide for autoplay slides (not resume/back/sync), so the audience — which mirrors the presenter's media events — isn't double-driven.
  • componentplaySceneDocumentMedia reaches the same-origin composition iframe, finds the scene's <video>, and asserts playback; the existing stopMedia (already wired on slide change) resets it. An autoplay token cancels a pending start when the slide changes.
  • tests — controller autoplay behavior + parser flag round-trip/validation. 131 player + 22 core slideshow tests pass; lint/format/typecheck/fallow clean.

Authoring: add "autoplay": true to a slide in the slideshow island.

⚠️ Known limitation — runtime media-start needs the player media model (@vanceingalls)

On current main, the clip↔timeline binding from #1601 keeps every clip synced and paused to the held timeline frame, which wins against playSceneMedia's play() — so the clip does not actually start on main yet (it does on the pre-#1601 player).

The correct fix is a sanctioned "let this clip free-run while the timeline holds" path in the player/runtime media controller — i.e. the media model #1601 introduced. Flagging for Vance to wire the start into that model (or rebase this onto it) when back. Everything in this PR is the stable surface that hook plugs into; it's a no-op until then, not a regression.

Relatedly, this is independent of #1706 (which fixes npx hyperframes present shipping the player bundles).

🤖 Generated with Claude Code

jrusso1020 and others added 2 commits June 24, 2026 21:04
Adds an opt-in `autoplay` flag to slideshow slides: when the presenter lands
on a video slide, its `<video>` plays from the start. The slideshow still
holds and never auto-advances — the presenter clicks Next when ready. This
covers compositions whose own controls can't be clicked (the player renders
the composition pointer-events:none).

Plumbing (done, tested):
- core: `SlideRef.autoplay?: boolean`, parsed + validated in parseSlideshow
  (a non-boolean autoplay rejects the manifest); carried through resolve.
- controller: optional `PlayerPort.playSceneMedia(sceneId)`, fired only on
  forward `enterSlide` for autoplay slides (not resume/back/sync, so the
  audience — which mirrors the presenter's media events — isn't double-driven).
- component: `playSceneDocumentMedia` reaches the same-origin composition
  iframe, finds the scene's `<video>`, and asserts playback; `stopMedia`
  (already wired on slide change) resets it. An autoplay token cancels a
  pending start when the slide changes.
- tests: controller autoplay behavior + parser flag round-trip/validation
  (131 player + 22 core slideshow tests pass).

KNOWN LIMITATION — runtime media-start needs the player media model (@vance):
On current main the clip<->timeline binding from #1601 keeps every clip synced
and *paused* to the held timeline frame, which wins against playSceneMedia's
play() — so the clip does not actually start on main yet (it does on the
pre-#1601 player). The correct fix is a sanctioned "let this clip free-run
while the timeline holds" path in the player/runtime media controller. Flagging
for Vance to wire the start into the #1601 media model (or rebase onto it) when
back. The plumbing above is the stable surface that hook plugs into.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- guard playSceneDocumentMedia behind resolveMode() !== "audience": the
  audience mirrors the presenter's media events, so it must not independently
  drive its own copy of the clip.
- drop the per-enter window pointerdown/keydown "gesture retry" listeners,
  which leaked when muted autoplay succeeded without a gesture. The poll already
  re-asserts play(), so a gesture within the window is picked up next tick.
- stop polling once the clip is advancing across two ticks (was re-asserting
  play() for the full window even after playback was confirmed).
- cancel any in-flight autoplay loop on disconnectedCallback (bump the token).
- split the poll into findSceneVideo + stepAutoplay helpers (keeps each small).
- fix the enterSlide comment: autoplay fires from enterSlide (next/prev/
  goToSlide), not resumeSlide (back/backToMain/syncTo).
- parser: isOptionalBoolean type guard instead of a one-off helper; drop `as`
  assertions in the new controller test.

131 player + 22 core slideshow tests pass; lint/format/typecheck/fallow clean.

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

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Clean, well-layered feature with solid test coverage and good defensive engineering around the timing hazards.

Architecture is right:

The three-layer split (core type → controller dispatch → component implementation) keeps each layer focused:

  • Core adds autoplay?: boolean to SlideRef with isOptionalBoolean validation — clean extraction alongside the existing isOptionalNumberArray refactor.
  • Controller fires playSceneMedia on enterSlide only — not resumeSlide, syncTo, or backToMain. This is the correct boundary: the controller knows when to play (enter), the component knows how (DOM, polling, token guard).
  • Component (HyperframesSlideshow) owns the DOM interaction, timing hazards, and presenter/audience split.

The polling approach is well-designed:

The two timing hazards (clip not in DOM yet at construction, player timeline sync pausing the clip on enter) mean a single play() loses the race. The poll-until-advancing pattern addresses both:

  • findSceneVideo returns null if the iframe DOM isn't ready → poll retries
  • void video.play().catch(() => {}) is correct since the retry loop handles failures
  • advancingTicks >= 2 confirms real playback across two ticks, avoiding false positives
  • AUTOPLAY_STEP_MS=150 / AUTOPLAY_MAX_MS=6000 → max ~40 iterations, reasonable budget

Token cancellation is solid:

autoplayToken++ in both stopDocumentMedia and disconnectedCallback ensures a pending poll from a previous slide can't replay a clip we've left. The guard if (token !== this.autoplayToken) return kills stale loops cleanly.

Presenter-only is correct:

if (this.resolveMode() === "audience") return — the audience mirrors presenter media events via wireSlideshowMedia(), so only the presenter initiates autoplay. No double-driving.

Tests cover the right cases:

  • Controller: plays on enter, skips non-autoplay slides, stops + replays on nav back, graceful when port lacks playSceneMedia
  • Core: flag parse + carry-through, non-boolean rejection

Minor observation (non-blocking):

safeId = sceneId.replace(/["\\]/g, "\\$&") in the querySelector is good defensive practice against injection. The video.muted = this._muted || video.defaultMuted respects both the slideshow mute state and the video's own default — nice detail.

No issues. CI still running.

Review by Miga

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Opt-in per-slide autoplay for the slideshow presenter: a video slide can declare "autoplay": true and its <video> will play on enter, working around the composition's intentional pointer-events:none (which makes native clip controls unclickable during present). Slideshow itself still holds — never auto-advances — and the surface is cleanly three-layered: core type + parser validation, controller playSceneMedia dispatch fired only from enterSlide, component DOM poll loop with token-cancelled re-assert. James's disclosure that runtime media-start is inert on main until the #1601 media-model wires up is accurate and clearly flagged in the PR body. 173 / 5 across 6 files; production delta ≈109 lines, test delta ≈64 lines.

Verdict: LGTM

Findings (numbered, with severity tag):

  1. NITpackages/player/src/slideshow/hyperframes-slideshow.ts:1100 (stepAutoplay): void video.play().catch(() => {}) silently swallows every play() rejection. This is intentional (autoplay-policy rejections during the user-gesture wait are expected; AbortError on timeline-sync seek is the whole reason for the poll). For the lifetime of the inert period this is correct. Once the media-model wire-up lands and play() is supposed to take, a swallowed real error (e.g. NotSupportedError on a bad source) becomes invisible. Worth a follow-up to distinguish expected rejections (AbortError, NotAllowedError) from real failures and surface the latter — but not blocking now since the loop's "advancingTicks >= 2" gate provides observability (poll just times out at 6s).

  2. NITpackages/player/src/slideshow/hyperframes-slideshow.ts:1094 (findSceneVideo): returns the first <video> inside [data-composition-id="<sceneId>"]. If a scene contains multiple video clips, only the first autoplays. The PR scope is "the slide's <video>" (singular in the doc-comment), so this matches stated intent — but worth a sentence in the SlideRef.autoplay JSDoc saying "the scene's first video element" so authors with multi-video scenes don't expect everything to play.

Verified:

  • Opt-in invariant. SlideRef.autoplay?: boolean defaults to undefined; parseSlideshow.ts isOptionalBoolean rejects non-boolean (test covers); SlideshowController.ts:124 gates dispatch on if (slide.autoplay); hyperframes-slideshow.ts:1071 audience-mode early-return. Existing slideshows without the flag stay exactly as they were.
  • Wired-but-inert framing is accurate. The component-side playSceneDocumentMedia does run on main (the controller still dispatches, the poll still loops), but video.play() loses the race against #1601's clip↔timeline binding and the poll harmlessly times out at AUTOPLAY_MAX_MS=6000. No throw, no DOM state leakage across navigation (stopDocumentMedia + autoplayToken++ on slide change and disconnectedCallback cancel pending re-asserts). Manual Next still works because nothing in this PR touches navigation.
  • Forward/resume split is correct. Controller fires from enterSlide (next / prev / goToSlide) only — resumeSlide (used by back / backToMain / syncTo to restore a saved fragmentIndex) does not call playSceneMedia. Test "stops prior media and plays again when navigating back into an autoplay slide" confirms prev() into an autoplay slide replays (because prev() calls enterSlide, not resumeSlide).
  • Audience non-double-drive. wireSlideshowMedia() (pre-existing on main, line 545) attaches a "play" event listener to every media element and postMedias it to the audience channel. So the presenter's play() propagates naturally; the audience-side if (this.resolveMode() === "audience") return correctly prevents the audience from also initiating its own poll. No conflation.
  • Token cancellation. autoplayToken++ bumped in (a) playSceneDocumentMedia start, (b) stopDocumentMedia (slide change), (c) disconnectedCallback. Stale poll closures bail on if (token !== this.autoplayToken) return. Clean.
  • Optional-port hook. PlayerPort.playSceneMedia? is optional; SlideshowController.ts:124 uses this.player.playSceneMedia?.(slide.sceneId) so a port without the hook doesn't throw. Test "does not require autoplay support on the port" covers.
  • Test count claim. James cites "131 player + 22 core slideshow tests"; this PR adds 4 new controller cases (describe("SlideshowController autoplay")) + 2 parser cases (autoplay round-trip + non-boolean rejection). The headline is total greens (not delta), and CI Test job at f8b89d04 is green, so the figure is what pnpm test prints rather than a delta claim — fine.
  • Cross-PR family. #1601 is MERGED (2026-06-20, head d31c1c7a) — so the "wins against play()" disclosure refers to current main. #1596 is OPEN but irrelevant here — James says main already supersedes its hold fix, and this PR doesn't touch the hold path. #1706 is MERGED (2026-06-24, Via LGTM'd at 109f8f5a carried through), which means npx hyperframes present ships the slideshow bundle — so once the media-model wire-up lands, the autoplay path will actually surface to users via the CLI. #1702 / #1703 are orthogonal capture/runtime layers and don't touch slide DOM or data-composition-id selectors.
  • Band-aid scan. No duplicate guards (single if (slide.autoplay) site in controller, single if (sceneAutoplay) check pathway component-side); no contradictory rules (the "never auto-advance" invariant is preserved — playSceneMedia is media-only, no next() call); no silent scope gap on per-slide (each enterSlide re-evaluates slide.autoplay); no decorative gate (the eligibility gate enrolls the right cohort, even though the play() call is currently inert against #1601).

CI state at HEAD f8b89d04: Most jobs green (CI: Lint/Format/Typecheck/Test/Build/Fallow/SDK contracts/Studio smoke/Semantic-PR-title all SUCCESS; CodeQL js-ts and Windows render shards + regression shards + Perf:drift/parity + CLI smoke + Smoke:global-install still IN_PROGRESS). No failures.

Prior reviewer state: Miga (miga-heygen) posted LGTM as COMMENTED at 2026-06-25T01:15:11Z. Covers architecture (three-layer split right), polling design (timing-hazard analysis: clip-not-in-DOM-yet + player-timeline-sync-pauses, advancingTicks >= 2 gate, ~40-iteration budget), token cancellation, presenter-only invariant, test coverage. Convergence is strong — independent finds line up: we both flag the silent .catch(() => {}) as defensible-given-current-design-but-worth-revisiting; Miga praises the safeId injection guard and video.muted = this._muted || video.defaultMuted detail. No miss-check disagreements.

Forward-coherence note for vance: the playSceneMedia port hook + playSceneDocumentMedia poll is the durable surface; when you wire the media-model "let this clip free-run while the timeline holds" path, the integration point is stepAutoplay calling video.play() (line 1100). Either replace the body to dispatch through the media model, or let the poll keep doing its thing and add the model-side concession that lets the play() actually take. Either way the controller-level gate (if (slide.autoplay)) and the parser-level isOptionalBoolean validation stay as-is.

Review by Via

@vanceingalls

vanceingalls commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

On pointer-events:none and interactive

pointer-events:none on <hyperframes-player> is intentional — it prevents stray clicks from toggling timeline playback while the presenter is navigating. When a composition needs interactive controls (native video controls, links, custom players), the interactive attribute opts out of this: it passes pointer events through to the composition iframe. So the inability to click video controls isn't a bug — it's the expected default, with interactive as the escape hatch.

The media model (#1601)

ParentMediaManager maintains mirror proxies of the iframe's <audio>/<video> elements in the parent frame. This matters because mobile browsers gate el.play() on user activation in the same frame — proxies in the parent frame inherit the user gesture from slideshow navigation. On every bridge tick (~80ms), mirrorTime syncs each proxy's currentTime to the timeline position.

The actual blocking happens inside the composition iframe's runtime. syncRuntimeMedia runs on every tick (~50ms) with playing: false when the slideshow holds, and enforces pause on any playing clip:

} else if (!params.playing && !el.paused) {
  el.pause();
}

playSceneDocumentMedia's play() fires, then the runtime's next tick pauses it. mirrorTime in ParentMediaManager is a separate concern (parent-frame proxies) — the blocking is in the iframe runtime itself.

Presenter → audience sync

wireSlideshowMedia attaches native media event listeners (play, pause, timeupdate, seeking, ended, etc.) to every media element in the composition iframes. When the presenter's media fires one of these, publishMediaState broadcasts it over BroadcastChannel('hf-slideshow:' + pathname). The audience window receives it via onRemoteMedia, which calls applyRemoteMedia — play events trigger playAudienceMediaMuted (starts muted first to comply with autoplay policy, then unlocks if permitted), pause/ended events pause directly. timeupdate is throttled to 450ms to avoid flooding the channel. This means autoplay on the presenter naturally propagates to the audience through the existing event-mirroring path — no extra wiring needed there.

What's needed to make autoplay work

The exemption needs to live in syncRuntimeMedia — a new control message from the player to the runtime flagging a specific clip as free-running, so syncRuntimeMedia skips the !params.playing && !el.paused → pause() enforcement for it. The player's playSceneMedia method sends this control, then calls play() on the clip. stopMedia on slide exit (already wired) sends the corresponding clear. The audience gets it through wireSlideshowMedia as normal — no extra work there.

The surface PR #1708 builds is exactly right — just needs that control message wired into the runtime.

@vanceingalls

Copy link
Copy Markdown
Collaborator

Authoring model question + skill guidance

One thing worth clarifying before merge: how does autoplay get set, and when should it be used?

It's a static authoring decision — the composition author writes "autoplay": true on specific slides in the island JSON before handing the deck off. The presenter just navigates normally; slides marked autoplay start their video on enter. The slideshow still holds, presenter controls pacing with Next.

The default mental model should stay "presenter always initiates playback." autoplay is the narrow exception: slides where the video is the content (a recorded demo, a product walkthrough, a testimonial clip) and forcing a manual play click after navigating is just friction. The audience gets it automatically through the existing wireSlideshowMedia event-mirroring path — no extra wiring needed.

Without guidance in the slideshow skill, agents will add autoplay: true to every video slide by default — including background clips and anything the presenter talks over. Worth adding a short section before merge covering:

  • Use it when the video is the primary content of the slide and its natural end is the cue to advance
  • Don't use it for background/ambient video, clips the presenter talks over, or slides where the presenter might want to pause mid-video
  • Audience sync is automatic through wireSlideshowMedia
  • Leaving the slide always stops the clip (stopMedia is already wired on slide exit)

Addresses review feedback on #1708:
- skill: document per-slide `autoplay` in the slideshow standalone-harness
  reference — when to use it (video is the slide's primary content, its end is
  the advance cue) vs not (background/ambient loops, footage talked over), per
  Vance's guidance, before merge.
- play() rejection is no longer blanket-swallowed: AbortError (timeline-sync
  seek interrupt) and NotAllowedError (gesture-gated autoplay) are expected and
  ignored; any other rejection is surfaced once via console.warn (Via nit 1).
- clarify in the SlideRef.autoplay doc that it plays the scene's FIRST <video>
  (Via nit 2).

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

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

R2 verify at 7aefc378dc — both R1 nits cleanly addressed, plus the autoplay skill-guidance section vance asked for landed in the same commit.

My R1 nits — resolved:

  1. video.play().catch(() => {}) swallowed all rejection types. Now hyperframes-slideshow.ts:1115-1125 distinguishes expected (AbortError from timeline-sync seek interruption, NotAllowedError from autoplay-policy gating) from real failures (NotSupportedError on bad source, decode errors), and surfaces real failures once via a state.warned latch — clean idempotent observability without spamming. Comment accurately documents both branches:

    Expected during the poll: AbortError (a timeline-sync seek interrupts the play) and NotAllowedError (autoplay gated on a user gesture). Surface anything else once — a real failure (bad src, decode) shouldn't be silent.

    The latch is per-poll-session (AutoplayPollState), so a fresh slide entry gets a fresh warn budget — correct.

  2. Multi-video scene scope clarification. SlideRef.autoplay docstring at packages/core/src/slideshow/slideshow.types.ts:22-28 now reads:

    When true, the slide's first <video> plays automatically on enter ... The slideshow still holds — it never auto-advances — so the presenter clicks Next when ready. Defaults to false. Use it when the video is the slide's primary content and its natural end is the cue to advance, not for background/ambient clips.

    The "first <video>" framing matches findSceneVideo's actual behavior (single first match). Author expectations now set correctly.

vance's autoplay-guidance ask — addressed. New "Per-slide autoplay" section at skills/slideshow/references/standalone-harness.md:112-124:

  • States the invariant: slideshow holds, autoplay only saves a manual play click.
  • JSON example with "autoplay": true + presenter notes.
  • Positive use case: video is the slide's primary content, natural end is the advance cue (cold-open promo, demo clip).
  • Negative use case: background/ambient loops, footage the presenter talks over — those need the presenter's own cue via interactive. Aligns with #1712's mechanical-interactive direction.
  • Constraint statement: "One autoplay clip per slide (the first <video> in the scene)."

Verified non-regression:

  • The component-side playSceneDocumentMedia polling structure is unchanged — same token cancellation, audience early-return, AUTOPLAY_MAX_MS timeout, advancing-ticks confirmation gate.
  • wireSlideshowMedia() audience mirror is unchanged — presenter play() still propagates via the existing "play" listener.
  • SlideshowController.ts:124 gate on if (slide.autoplay) unchanged.
  • "Wired but inert until media-model lands" framing still accurate — the play() is now slightly more verbose on real-error reporting but still loses the race vs #1601's clip↔timeline binding, so the poll still times out harmlessly at 6s on main.

CI state at HEAD 7aefc378dc: all 46 required + non-required checks SUCCESS. Zero failures, zero pending.

Prior reviewer state: Miga COMMENTED LGTM at R1 (no follow-up at R2 — no new findings to integrate), Via COMMENTED LGTM at R1 (this is the upgrade). No Rames review on this PR's record (he's on #1712 and the track-distribution Slack).

Verdict: LGTM at HEAD 7aefc378dc. Both my R1 nits resolved correctly; vance's skill-guidance ask is in; no regressions.

Stamp routing per standing rule: the actual APPROVE click goes via Vai (my GH layer = vance's identity, no direct stamps without his explicit authorization in DM). This is COMMENTED LGTM only.

R2 verify by Via

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