feat(player): slideshow controller state machine

[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
• feat(player): slideshow controller state machine #1589 👈 (View in Graphite)
• 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]

Review: feat(player) — slideshow controller state machine

Alright, I spent a good amount of time going through this one — the controller, the test suite, the type contracts, the vitest config, the whole nine yards. Here's where I landed.

---

1. State Machine Design

The state machine is well-conceived and appropriately scoped. The navigation stack (StackFrame[]) is the right primitive for branch/sub-sequence navigation — it's essentially a call stack for presentation flow, which maps cleanly to the "enter branch / back to parent" UX metaphor.

The states themselves are implicit rather than declared as an enum (there's no explicit State.PLAYING | State.PAUSED | State.HOLDING type), but that's actually fine here because the state is fully captured by the combination of holdAt (null vs number) and player state (playing vs paused). Adding a formal state enum would be over-engineering for this use case.

One thing I'd flag: The fragmentIndex = -1 sentinel value is load-bearing in multiple places — enterSlide, resumeSlide, nextStop, the fragment boundary detection in onTime. It works, but it's the kind of implicit protocol that could bite future contributors. A brief doc comment on the StackFrame.fragmentIndex field explaining the -1 = before first fragment convention already exists (line 724, nice), but consider also documenting the full lifecycle: -1 on enter → 0..n as fragments reveal → stays at last fragment index when slide ends.

2. Edge Cases

What's handled well:

• Empty sequences (no slides) — enterSlide early-returns and clears holdAt. Test coverage for this is solid (Fix #5-ctrl).
• Unknown branch IDs — enterBranch guards with if (!this.show.sequences[sequenceId]) return, no-throw. Tested.
• back() / backToMain() at root — both guard on stack.length <= 1. Good.
• Branch-edge navigation — prev at first slide of branch pops, next at last slide of branch pops. Clean, tested thoroughly.
• Fragment position restoration on back() — resumeSlide preserves the exact fragment index. This is a subtle and important detail, well-tested (Fix 8b).

What concerns me:

(a) Rapid next() calls / double-fire risk. If next() is called twice in rapid succession before onTime fires for the first hold point, the second next() will see the same fragmentIndex (since onTime hasn't advanced it yet) and compute the same nextStop target. This means the second call effectively becomes a no-op (re-plays to the same hold point), which is safe but might feel janky to a user rapidly clicking next. Not a bug, but worth noting — the play-to-hold model naturally debounces rapid input.

(b) onTime epsilon comparison. The t >= this.holdAt - EPS check (with EPS = 0.001) is correct for continuous playback, but could this fire prematurely if the player's time reporting is non-monotonic (e.g., after a seek to a position slightly before holdAt)? The seek in enterSlide goes to slide.start, and holdAt is set to the first fragment/end, so there should always be a gap. But if slide.start and fragments[0] are within 1ms of each other (which would be a degenerate manifest), the hold would fire immediately on seek. Might be worth a comment acknowledging this assumption, or clamping EPS to be less than the minimum allowed fragment gap.

(c) currentTime check in next(). Line 871: const atEnd = this.player.currentTime >= slide.end - EPS. This is evaluated at the moment next() is called. If the player is paused at a fragment hold point (normal case), currentTime will be at the fragment time, which is less than slide.end. So atEnd will be false, and hasMoreFragments determines flow. This is correct. But: if currentTime somehow overshoots slide.end (buffering jank, low-precision timer), atEnd is true AND hasMoreFragments might also be true (if fragments remain by index), and the code falls through to "advance to next slide" — which skips remaining fragments. This is probably fine in practice (the slide already played past its end), but it's a subtle interaction.

3. Integration with Controller from #1581

This PR cleanly separates concerns. The PlayerPort interface (lines 713-719) is the seam:

• seek(t), play(), pause(), currentTime, onTimeUpdate(cb) — five methods, read-only current time, one callback registration. Minimal and clean.

The controller has zero DOM awareness — it's purely a navigation state machine that commands a player through the port. The web component in #1590 will be the glue layer. This is the right separation.

The @hyperframes/core dependency for types only (ResolvedSlideshow, ResolvedSlide) is appropriate — these are shared data contracts, not runtime behavior.

4. Testability

The test suite is excellent. 574 lines of tests for 215 lines of implementation (~2.7:1 ratio) is generous. Highlights:

• fakePlayer() is a clean, minimal mock that captures the essential PlayerPort behavior plus a synthetic emit() for driving time updates. No framework overhead.
• Test factories (showAtFrag1, showAtSlide1InDeep, inBranch) reduce setup duplication while keeping each test readable.
• Named regression tests (Fix 8a, Fix 8b, Fix #5-ctrl, Fix #backToMain) are great for documenting discovered bugs and preventing regressions.
• Edge cases (empty sequences, unknown branches, single-slide shows, multi-nested branches) are all covered.

Minor test suggestion: There's no test for dispose() — specifically, that after calling dispose(), time updates no longer trigger holds/pauses. Not critical since the implementation is trivial (this.unsub()), but it'd round out the suite.

5. Over-engineering Assessment

Verdict: NOT over-engineered. The branch navigation stack is justified by the slideshow UX (sequences/sub-decks are a core feature), fragment holds are justified by the presenter reveal pattern, and the resumeSlide path is a real need (restoring position after branching). The canPrev/canNext getters are simple derived state, not premature features.

If anything, the one area that might be under-specified is the onChange callback — it fires on slide/fragment changes, but consumers can't distinguish what changed (slide advance vs. fragment reveal vs. branch enter). For a 10-PR stack this is fine to address later.

6. Nits

• vitest.config.ts line 3: new URL("..", import.meta.url).pathname — works in Node/Bun but would break in a browser context. Since this is test-only config, it's fine, but a comment noting the Node-only assumption would be kind.
• test-setup.ts: The BroadcastChannel polyfill is only needed by the component tests in #1590, not by the controller tests in this PR. Including it here is harmless (the controller tests don't touch it) but it's forward-loading infrastructure.
• PR description is the default template with no content filled in. For a 10-PR stack, descriptions matter — worth filling in.

---

Summary

Strong PR. The state machine is well-designed, the PlayerPort abstraction is clean, the test suite is thorough with good regression test naming, and the separation from the web component layer is exactly right. The edge cases I flagged (rapid calls, epsilon edge, currentTime overshoot) are low-risk in practice but worth keeping in mind as the stack builds.

I'd recommend filling in the PR description and considering a dispose() test, but neither blocks approval.

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

Review — feat(player): slideshow controller state machine

Reviewed at HEAD aa9036be83386a55c002a0901b1b5cc4cbd156cb. Verdict: clean state machine, one real split-ordering hazard in package.json, otherwise non-blocking.

This is part A of a 2-PR split of #1581. Reviewed alongside #1590 (part B) at Vance's request.

Split verification — coverage vs #1581 (the monolith)

Reads cleanly as a strict subset. Per-file addition counts vs #1581:

File	#1589 adds	#1581 adds	Δ
slideshow/SlideshowController.ts	187	187	0
slideshow/SlideshowController.test.ts	518	518	0
slideshow/test-setup.ts	39	39	0
vitest.config.ts	8	8	0
package.json	9	9	0
bun.lock	—	—	0

No drift, no rewrite. Boundary is the PlayerPort interface (5 methods) + SlideshowController class + types pulled from @hyperframes/core/slideshow. #1590 consumes this via import { SlideshowController, type PlayerPort } from "./SlideshowController" — type-only PlayerPort import, instance-only SlideshowController import. Tight, no leak.

---

Findings (layered on Miga's pass)

Miga (review at 07:28Z) covered the state-machine shape: implicit-state-via-holdAt-flag observation, fragmentIndex = -1 sentinel docs, rapid-next() debounce property, EPS edge cases, missing dispose() test. Adding:

1. (Blocker if #1589 lands without #1590) package.json exports a built file that doesn't exist in this PR. Lines 19-24 of the updated package.json add:

"./slideshow": {
  "types": "./dist/slideshow/hyperframes-slideshow.d.ts",
  "script": "./dist/slideshow/hyperframes-slideshow.global.js",
  "import": "./dist/slideshow/hyperframes-slideshow.js",
  "require": "./dist/slideshow/hyperframes-slideshow.cjs"
}

But the tsup entry for src/slideshow/hyperframes-slideshow.ts doesn't land until #1590 (its tsup.config.ts change). And hyperframes-slideshow.ts itself doesn't exist on this branch. Net: if anyone publishes / bun packs / consumes @hyperframes/player/slideshow from this revision, the export resolves to missing files. Two fixes, pick one:

• Hold the ./slideshow export entry for #1590 (clean split semantics).
• Or fold the tsup entry + hyperframes-slideshow.ts stub into this PR. Less clean.

If Vance always lands #1589+#1590 together (atomic), it's a transient-revision hazard rather than a runtime one — but Graphite stacks have a habit of getting cherry-picked or partially reverted.

2. Inverse asymmetry: enterSlide clears holdAt, resumeSlide doesn't. enterSlide (line ~808) does this.holdAt = null before the slide-lookup early-return. resumeSlide (line ~823) skips this. If resumeSlide is reached with no slide at the index (degenerate after back()/backToMain() into a malformed sequence), a previously-armed holdAt lingers and could fire on the next onTime. Inverse-operation rubric — easy mirror fix.

3. goToSlide() is implicitly sequence-local. Clamps to slidesOf(this.frame.sequenceId). No way to jump across branches. Almost certainly intentional, but worth a one-line docstring so consumers don't pattern-match it as global. Also affects ControllerLike in #1590, which exposes goToSlide? to test stubs.

4. PR description is the default template. Worth filling in even briefly for a 10-PR stack — both reviewers and git blame archaeologists thank you.

5. (Nit) test-setup.ts ships the BroadcastChannel polyfill but #1589's only test file doesn't touch it. Forward-loading by ~574 lines of tests' worth of dependency. Fine — it's setup-only, and #1590 needs it — but if you ever split further, the polyfill belongs with the component PR.

---

Architecture assessment vs monolith

The controller boundary is the genuinely valuable seam to lock in independently. PlayerPort is 5 methods (seek, play, pause, currentTime, onTimeUpdate) — small, swappable, testable. CI runs the controller suite without happy-dom, faster signal loop. This split is the better architecture if you fix the package.json ordering. If they always land atomically, the practical difference is reviewability — and 866 LOC of state machine is a much friendlier review surface than 3247 LOC of state-machine-plus-DOM.

Review by Rames D Jusso

The base branch was changed.