feat(slideshow): per-slide autoplay (manual-advance, opt-in)#1708
feat(slideshow): per-slide autoplay (manual-advance, opt-in)#1708jrusso1020 wants to merge 3 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
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?: booleantoSlideRefwithisOptionalBooleanvalidation — clean extraction alongside the existingisOptionalNumberArrayrefactor. - Controller fires
playSceneMediaonenterSlideonly — notresumeSlide,syncTo, orbackToMain. 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:
findSceneVideoreturnsnullif the iframe DOM isn't ready → poll retriesvoid video.play().catch(() => {})is correct since the retry loop handles failuresadvancingTicks >= 2confirms real playback across two ticks, avoiding false positivesAUTOPLAY_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
left a comment
There was a problem hiding this comment.
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):
-
NIT —
packages/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 andplay()is supposed to take, a swallowed real error (e.g.NotSupportedErroron 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). -
NIT —
packages/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 theSlideRef.autoplayJSDoc 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?: booleandefaults toundefined;parseSlideshow.tsisOptionalBooleanrejects non-boolean (test covers);SlideshowController.ts:124gates dispatch onif (slide.autoplay);hyperframes-slideshow.ts:1071audience-mode early-return. Existing slideshows without the flag stay exactly as they were. - Wired-but-inert framing is accurate. The component-side
playSceneDocumentMediadoes run on main (the controller still dispatches, the poll still loops), butvideo.play()loses the race against #1601's clip↔timeline binding and the poll harmlessly times out atAUTOPLAY_MAX_MS=6000. No throw, no DOM state leakage across navigation (stopDocumentMedia+autoplayToken++on slide change anddisconnectedCallbackcancel 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 callplaySceneMedia. Test"stops prior media and plays again when navigating back into an autoplay slide"confirmsprev()into an autoplay slide replays (becauseprev()callsenterSlide, notresumeSlide). - Audience non-double-drive.
wireSlideshowMedia()(pre-existing on main, line 545) attaches a"play"event listener to every media element andpostMedias it to the audience channel. So the presenter'splay()propagates naturally; the audience-sideif (this.resolveMode() === "audience") returncorrectly prevents the audience from also initiating its own poll. No conflation. - Token cancellation.
autoplayToken++bumped in (a)playSceneDocumentMediastart, (b)stopDocumentMedia(slide change), (c)disconnectedCallback. Stale poll closures bail onif (token !== this.autoplayToken) return. Clean. - Optional-port hook.
PlayerPort.playSceneMedia?is optional;SlideshowController.ts:124usesthis.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 atf8b89d04is green, so the figure is whatpnpm testprints rather than a delta claim — fine. - Cross-PR family. #1601 is MERGED (2026-06-20, head
d31c1c7a) — so the "wins againstplay()" 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 at109f8f5acarried through), which meansnpx hyperframes presentships 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 ordata-composition-idselectors. - Band-aid scan. No duplicate guards (single
if (slide.autoplay)site in controller, singleif (sceneAutoplay)check pathway component-side); no contradictory rules (the "never auto-advance" invariant is preserved —playSceneMediais media-only, nonext()call); no silent scope gap on per-slide (eachenterSlidere-evaluatesslide.autoplay); no decorative gate (the eligibility gate enrolls the right cohort, even though theplay()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
|
On
The media model (#1601)
The actual blocking happens inside the composition iframe's runtime. } else if (!params.playing && !el.paused) {
el.pause();
}
Presenter → audience sync
What's needed to make autoplay work The exemption needs to live in The surface PR #1708 builds is exactly right — just needs that control message wired into the runtime. |
|
Authoring model question + skill guidance One thing worth clarifying before merge: how does It's a static authoring decision — the composition author writes The default mental model should stay "presenter always initiates playback." Without guidance in the slideshow skill, agents will add
|
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
left a comment
There was a problem hiding this comment.
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:
-
video.play().catch(() => {})swallowed all rejection types. Nowhyperframes-slideshow.ts:1115-1125distinguishes expected (AbortErrorfrom timeline-sync seek interruption,NotAllowedErrorfrom autoplay-policy gating) from real failures (NotSupportedErroron bad source, decode errors), and surfaces real failures once via astate.warnedlatch — 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. -
Multi-video scene scope clarification.
SlideRef.autoplaydocstring atpackages/core/src/slideshow/slideshow.types.ts:22-28now 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 matchesfindSceneVideo'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+ presenternotes. - 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
playSceneDocumentMediapolling structure is unchanged — same token cancellation, audience early-return, AUTOPLAY_MAX_MS timeout, advancing-ticks confirmation gate. wireSlideshowMedia()audience mirror is unchanged — presenterplay()still propagates via the existing"play"listener.SlideshowController.ts:124gate onif (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 onmain.
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
What
Opt-in per-slide
autoplayfor 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)
SlideRef.autoplay?: boolean, parsed and validated inparseSlideshow(a non-booleanautoplayrejects the manifest), carried throughresolveSlideshow.PlayerPort.playSceneMedia(sceneId), fired only on forwardenterSlidefor autoplay slides (not resume/back/sync), so the audience — which mirrors the presenter's media events — isn't double-driven.playSceneDocumentMediareaches the same-origin composition iframe, finds the scene's<video>, and asserts playback; the existingstopMedia(already wired on slide change) resets it. An autoplay token cancels a pending start when the slide changes.Authoring: add
"autoplay": trueto a slide in the slideshow island.On current
main, the clip↔timeline binding from #1601 keeps every clip synced and paused to the held timeline frame, which wins againstplaySceneMedia'splay()— 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 presentshipping the player bundles).🤖 Generated with Claude Code