feat(studio): slideshow branching editor panel

[vanceingalls]

Slideshow mode — 3/5: Studio branching editor

A new Slideshow right-panel tab in Studio to author the slideshow island visually — mark slides, edit notes/fragments, build branch sequences, and designate hotspots — persisting back into the composition HTML. Builds on #1581.

Key changes

• components/panels/SlideshowPanel.tsx + SlideshowSubPanels.tsx — slide list, slide inspector (notes + fragment hold-points marked at the playhead), branch tree (create/rename/assign), and a hotspot tool driven by the existing canvas selection.
• components/panels/slideshowPanelHelpers.ts — pure, unit-tested manifest transforms (toggle main-line slide, add/remove fragment, create/rename/delete sequence, add/remove hotspot). deleteSequence also prunes dangling hotspots that targeted it; safeParseManifest wraps the parser so a malformed island can't crash the panel.
• hooks/useSlideshowPersist.ts — persists edits through the SDK serialize→write path; notes are debounced with a coalesceKey (one undo entry per burst) and read from a ref to avoid stale-closure lost updates across composition switches.
• utils/setSlideshowManifest.ts — buildSlideshowIslandHtml (escapes </script> so manifest text can't break out of the island) + a persist helper that strips any existing islands and inserts exactly one.
• StudioRightPanel.tsx / App.tsx — new "slideshow" tab; the persist dependencies (sdkSession, recordEdit, reloadPreview, save-timestamp ref) are now required props rather than silently no-op'd.
• utils/sdkCutover.ts — exports persistSdkSerialize (now has a third caller); utils/studioHelpers.ts — RightPanelTab gains "slideshow".

Testing

Pure-helper unit tests + the island round-trip/escaping tests. tsc --noEmit clean.

Notes

The component reads the island from its own wrapper (not the composition the player loads) — known interim behaviour, documented in the skill PR (#1583); the durable fix is engine-hosted slideshow preview.

Stack

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

🤖 Generated with Claude Code

[vanceingalls]

• feat(studio): slideshow branching editor panel #1582 👈 (View in Graphite)
• feat(player): slideshow controller + <hyperframes-slideshow> component #1581
• 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]

Slideshow Branching Editor Panel — Review

Nice work. The separation between pure transform helpers, the notes-debounce controller, the persist hook, and the UI components is clean and well-tested. Here are my findings:

---

Architecture & separation of concerns — strong

The layered decomposition is genuinely good:

• slideshowPanelHelpers.ts — pure, immutable transforms. No React, no IO. Easy to test, easy to reason about.
• makeSlideshowNotesController — pure closure over pending / timer, fully testable without React or DOM. The flush-attribution invariant (notes always write to the callback that was active when they were typed, not the current one) is the hardest part of this PR and it's handled correctly.
• useSlideshowPersist — thin useCallback wrapper wiring the SDK persist path.
• SlideshowPanel / SlideshowSubPanels — presentation layer.
• setSlideshowManifest.ts — island serialization + the </script> breakout escape.

This is the right amount of layering for the problem — not over-engineered.

Things that look correct and well-handled

1. </script> breakout prevention — buildSlideshowIslandHtml escapes < and > as < / > in the JSON body. The test confirms a round-trip through parseSlideshowManifest with a notes field containing literal </script> — good edge case coverage.

2. Stale island dedup — ISLAND_RE is global, so persistSlideshowManifest strips ALL existing islands before inserting one. The "two stale islands collapse to one" test covers this. Solid.

3. deleteSequence cascading hotspot pruning — deleting a sequence prunes dangling hotspots from both main-line slides AND other sequences' slides. Tests cover both paths. This is the kind of referential-integrity concern that often gets missed.

4. manifestRef pattern — discrete handlers read from manifestRef.current (never a stale closure), and notes writes update it synchronously. This avoids the classic React state-lag bug.

5. Immutability — every transform returns a new object; the "does not mutate the input manifest" test explicitly guards this.

---

Issues and suggestions

1. Date.now() in sequence/hotspot IDs — collision risk under rapid use (low severity)

const id = `seq-${Date.now()}`;
// and
const id = `hotspot-${elementKey}-${Date.now()}`;

Date.now() has millisecond resolution. A double-click or a programmatic batch could produce duplicate IDs — and createSequence silently rejects duplicate IDs (returns the manifest unchanged), so the user would think nothing happened. Consider a monotonic counter or crypto.randomUUID() if the environment supports it.

2. slideshowScenes useMemo deps include previewIframeRef — a ref never triggers re-render (nit)

const slideshowScenes = useMemo<SceneInfo[]>(() => {
  // reads previewIframeRef.current
}, [previewIframeRef, rightPanelTab, refreshKey]);

previewIframeRef is a ref object — its identity is stable across renders, so listing it as a dependency is harmless but misleading. The actual reactivity comes from refreshKey (which is what forces the re-read). The eslint-disable comment suggests this was intentional, but a brief comment explaining that previewIframeRef is a "just in case the ref object identity ever changes" guard would prevent future confusion.

3. Fragment hold-points use exact float equality for dedup and removal

const frags = [...new Set([...(s.fragments ?? []), time])].sort((a, b) => a - b);
// and
(s.fragments ?? []).filter((f) => f !== time)

currentTime from the player store is a float. Two marks at "the same visual moment" could differ by sub-millisecond floating-point noise. If this matters for the UX (can the user click "mark" twice and get two entries at 1.500000001 and 1.499999999?), consider rounding to e.g. 2 decimal places before storing, or using an epsilon comparison for removal. If the player normalizes currentTime already, a comment would help.

4. No confirmation on deleteSequence — destructive action with cascade

handleDeleteSequence calls applyManifest(deleteSequence(...)) immediately with no confirmation dialog. Since this cascades to remove all hotspots targeting the sequence across the entire manifest, an accidental click could silently drop multiple hotspot bindings. The undo history should recover this, but a simple confirm prompt (even window.confirm) would be friendlier. Not blocking, but worth considering.

5. addHotspot only operates on main-line slides — is that intentional?

The helper maps over manifest.slides only, not manifest.slideSequences[*].slides. If a user selects a scene that's part of a branch (not the main line), the hotspot silently fails to attach. The PR description says "driven by the existing canvas selection" which implies the user has a main-line slide selected, but this could be confusing if the branch tree shows scenes with checkboxes that look selectable. A guard or a comment documenting this restriction would clarify intent.

6. persistSlideshowManifest regex escape is slightly incomplete

ISLAND_TYPE.replace(/[.+]/g, "\\$&")

This escapes . and + in the MIME type for the regex, but the actual type string application/hyperframes-slideshow+json also contains /. The / is fine inside a RegExp constructor (not a literal), so this works, but if the type ever changed to include other regex-special chars (brackets, parens) they wouldn't be escaped. Consider a general escapeRegExp utility or a comment noting the assumption.

7. Test file imports from ./SlideshowPanel via re-exports — intentional but unusual

// SlideshowPanel.tsx
export { toggleMainLineSlide, ... } from "./slideshowPanelHelpers";

The test file imports from ./SlideshowPanel rather than from ./slideshowPanelHelpers directly. The re-export comment says this is intentional, but it means the test file pulls in the full React component module (including useState, useEffect, player store imports, context imports). If any of those have side effects on import, the test could become flaky. Consider importing from ./slideshowPanelHelpers directly in the test — the helpers ARE the public API under test.

---

Summary

The code is well-structured, the edge cases that matter most (stale-closure notes, </script> breakout, dangling hotspot pruning, island dedup) are all handled correctly and tested. The issues above are low-to-medium severity — the Date.now() collision and the float-equality fragment comparison are the most likely to produce real user-facing bugs, but both have natural mitigations (undo history, player time normalization).

Recommend approval after addressing or acknowledging items 1 and 3.

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

HEAD reviewed: 67380f51257cccfff1f7ddcd81000edfe26ffe0e

Verdict: Solid panel; layering and stale-closure handling are right. A few concrete bugs/gaps worth tightening before this lands either as the monolith or as the split-stack tip (#1591+#1592). Posting the same findings on #1591 (persistence) and #1592 (UI) so you can pick the surface.

Layering Miga's review with corrections and additional findings.

---

Correction to Miga's finding #1 ("duplicate import block")

Miga claims the second import { ... } from "./slideshowPanelHelpers" block at SlideshowPanel.tsx:56-67 is redundant with the export { ... } from "./slideshowPanelHelpers" re-export at SlideshowPanel.tsx:32-44. This is incorrect — please do NOT remove the local import.

export { X } from "./Y" is a re-export only; it does NOT introduce X as a local binding in the importing module. The helpers are used as values 11 times locally at SlideshowPanel.tsx:263, 270, 278, 285, 291, 299, 306, 313, 320, 329, 336 (e.g. applyManifest(toggleMainLineSlide(manifestRef.current, sceneId))). Removing the local import would break the file with ReferenceError: toggleMainLineSlide is not defined at runtime (TS would also flag it, but the re-export-then-use pattern doesn't fail TS checks because the re-export forwards types).

The re-export block exists to give the test file (SlideshowPanel.test.ts:2-16) a single import surface for the panel + helpers; the local import block exists because the component itself consumes the helpers. Both are load-bearing. The duplication is awkward but functional. If you want to flatten, the right shape is: drop the local import, change every call site to helpers.toggleMainLineSlide(...) via a import * as helpers from "./slideshowPanelHelpers". Not blocking, not even worth doing for this PR — but worth noting in case Miga's suggestion lands and breaks the panel.

Blockers

None.

Concerns

1. persistSlideshowManifest skips the no-op gate that every sibling cutover writer applies — setSlideshowManifest.ts:62-78

Every other caller of persistSdkSerialize snapshots before = sdkSession.serialize(), applies dispatch ops, computes after = sdkSession.serialize(), and bails early if after === before (e.g. sdkCutover.ts:198-199: if (after === before) return false). The slideshow path doesn't — persistSlideshowManifest always replaces the island and writes, even when the manifest serialized identically to what's already in current. Effect: a setSlideNotes keystroke that re-types the same character at the same position still produces an undo entry + disk write + preview reload after the 450ms debounce. Same for toggling a checkbox on then off (the slides array goes empty → unchanged).

Suggest: compare after === current (the serialized HTML) immediately before the persistSdkSerialize call and bail if equal. Cheap, matches sibling shape.

2. No-op writes still bump the undo history — recordEdit unconditional

Consequence of (1): even no-op writes call recordEdit. Each keystroke during a notes burst is debounce-coalesced (good — coalesceKey: "slideshow-notes:<path>"), but a no-op discrete action (toggle + immediate untoggle, reorder out-of-bounds) still records an undo entry the user can't see any visible state change for. Both items resolved by adding the no-op gate above.

3. slideshowScenes useMemo can race the iframe's __clipManifest — StudioRightPanel.tsx:177-190

const slideshowScenes = useMemo<SceneInfo[]>(() => {
  const win = previewIframeRef.current?.contentWindow as IframeWindow | null;
  return (win?.__clipManifest?.scenes ?? []).map(...);
}, [previewIframeRef, rightPanelTab, refreshKey]);

previewIframeRef is a ref — its identity never changes (Miga right that the dep is a no-op). rightPanelTab flips when the user clicks "Slideshow". refreshKey is the only signal that bumps on iframe content change. If the user opens the slideshow tab before the iframe finishes hydrating __clipManifest, the panel shows zero scenes — and won't re-derive until refreshKey bumps (which only happens on a player refresh, not on first hydration as far as I can tell from useStudioPlaybackContext). The user sees an empty "Slides" list and assumes the feature is broken.

Suggest: either subscribe to a clip-manifest-loaded signal, or fall through to re-deriving on editingFile.content changes (which trip after compHtml loads). At minimum, a one-line empty state ("Waiting for preview to load…") when scenes.length === 0 and the iframe hasn't initialized __clipManifest would let the user know it's not a no-scenes condition.

4. Date.now() ID collision → silent no-op (sharper than Miga's framing) — SlideshowPanel.tsx:298, SlideshowSubPanels.tsx:385

Miga flagged the Date.now() collision. The user-experience hit is worse than she described:

• createSequence(manifest, id, label) — if id already exists in slideSequences, returns manifest unchanged (slideshowPanelHelpers.ts:80). The test at the helper level confirms this.
• addHotspot(manifest, sceneId, hotspot) — if hotspot.id already exists on the slide, returns s unchanged (slideshowPanelHelpers.ts:158).

A user who double-clicks "New branch" or "Make hotspot" within 1ms (programmatic burst or a fast trackpad double-tap) gets the second click silently dropped with no UI feedback. The handler calls applyManifest(...).catch(() => {}) — even the swallowed error path doesn't help here because the helper returns the same manifest by design.

Fixes (any one):

• crypto.randomUUID() if available, fallback to Date.now() + Math.random().toString(36).slice(2,8).
• Counter on a ref: const counterRef = useRef(0); const id = \seq-${Date.now()}-${counterRef.current++}``.
• At minimum: log/warn when the helper silently rejects the duplicate, so the failure is observable.

5. Float-equality fragment dedup/remove — slideshowPanelHelpers.ts:67, 84 (concur with Miga)

new Set([...fragments, time]) and filter((f) => f !== time) use strict equality on number. The player's currentTime is a float updated on requestAnimationFrame; two "mark fragment" clicks at "the same visual moment" could differ by ~1e-9. Two distinct fragments get stored at e.g. 1.500000001 and 1.499999999; removing the displayed 1.5 fails to match either. Suggest rounding to Math.round(time * 1000) / 1000 (ms precision) at the helper boundary — same answer as Miga, calling it out so it doesn't get lost.

Nits

6. The ponytail: third caller comment in sdkCutover.ts:147 is misleading — concur with intent

persistSdkSerialize has 5 internal callers in sdkCutover.ts (sdkCutoverPersist, sdkTimingPersist, sdkGsap*Persist via dispatchGsapOpAndPersist, sdkDeletePersist) plus the new external caller in setSlideshowManifest.ts. The "third caller" wording suggests an aperture decision was tracking a public-API count; the comment more accurately reads "first external caller" or "first cross-file caller." (nit — code is correct).

7. // fallow-ignore-next-line complexity on reorderMainLineSlide is a false positive (concur with Miga). Function is 7 lines with two early returns and a swap; the fallow rule is over-flagging. Same on HotspotTool (16 hooks deps but the body is trivial) and handleMakeHotspot. Worth tuning the rule if it keeps tripping clean code.

8. // fallow-ignore-next-line unused-export on studioHelpers.ts:207 (COMPOSITION_ROOT_OPEN_TAG_RE) — if it's genuinely unused, delete the re-export; if it's used externally and the rule is flagging it incorrectly, that's a rule-tuning issue. Either way the lint-suppress comment is masking something that should be answered (nit).

9. Aria role mismatch on SlideList scene rows — SlideshowSubPanels.tsx:61-66

Each row uses role="button" aria-pressed={isSelected} to convey selection. aria-pressed is for toggle-buttons; aria-selected (inside a role="listbox" parent) is the correct semantic for "selected row in a list." Doesn't affect users with default assistive tech but screen-reader output reads "pressed" instead of "selected." Quick fix.

Questions

10. Persist failure path — what does the user see?

useSlideshowPersist's callback is awaited in applyManifest (SlideshowPanel.tsx:216) but every call site does applyManifest(...).catch(() => {}). A failed writeProjectFile (disk full, FS permission, OPFS quota) is silently swallowed; the in-memory manifest state reflects the edit but disk doesn't. Next time the user reloads the comp the edit is gone. Is there an app-level toast wiring elsewhere that surfaces the write failure, or does the panel need an explicit error UI? Same question applies to applyNotesManifest → notesCtrl.schedule(..., onPersistNotes, 450) where onPersistNotes is persist(p.manifest).catch(() => {}) (SlideshowPanel.tsx:131 in the controller).

11. Manifest version field — should it land in this PR?

Miga raised this on #1591. The SlideshowManifest type at packages/core/src/slideshow/slideshow.types.ts has no version field. Adding version?: 1 now (in core + threading through buildSlideshowIslandHtml) costs ~3 lines and converts a future migration from "best-effort field-shape inference" to "switch on manifest.version ?? 0." Cheapest to add before any author hand-edits a manifest in the wild. Defer is fine if the slideshow island spec is genuinely frozen, but worth a sentence in the PR description either way.

What I didn't verify

• Whether recordEdit's coalesceKey actually collapses to a single undo entry across save-queue flushes vs producing N entries that all share a key (I trusted the doc-comment claim).
• Whether the FileManagerContext's editingFile.content updates synchronously enough that the compHtml effect's notesCtrlRef.current.flush() runs before the new comp's parse — Miga raised an ordering concern. I believe the flush fires synchronously and setManifest is async, so the OLD comp's debounced write goes through with the OLD callback (which has the OLD activeCompPath). Looks right but I didn't trace it through the React commit cycle.
• The lint rule slideshow_ codes from #1580 — safeParseManifest swallows malformed islands silently; whether the lint surface flags malformed islands at edit time so the user knows their notes won't persist is worth a check.

Review by Rames D Jusso

The base branch was changed.