feat(studio): slideshow branching editor panel UI

[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 👈 (View in Graphite)
• feat(studio): slideshow manifest persistence + panel helpers #1591
• feat(player): <hyperframes-slideshow> web component + presenter #1590
• 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]

Slideshow Branching Editor Panel — Review

Overall this is a solid, well-structured PR. The architecture is clean: pure manifest transforms tested independently, a well-designed notes debounce controller, and presentational sub-panels that receive callbacks. A few things to address before merging.

---

Issues to address

1. Duplicate import block in SlideshowPanel.tsx

The slideshowPanelHelpers functions are imported twice — once via export { ... } from (lines 18–30) and again via a regular import { ... } from (lines 56–67, after safeParseManifest). The file will work because bundlers deduplicate, but it's confusing to read and suggests a copy-paste accident. Remove the second import block and keep only the re-export block (the named imports are already in scope from the export { ... } from syntax).

2. slideshowScenes useMemo dep array suppresses exhaustive-deps lint

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [previewIframeRef, rightPanelTab, refreshKey]);

The disable comment hides a real concern: previewIframeRef is a ref object whose identity never changes, so listing it does nothing — meanwhile the actual trigger for re-deriving scenes is the iframe's contentWindow.__clipManifest changing, which none of these deps track. The refreshKey dependency is doing the heavy lifting here, but the comment makes it look accidental. Consider either:

• Removing previewIframeRef from the dep array (it's stable) and adding a brief inline comment explaining that refreshKey is the proxy signal for manifest changes, OR
• Replacing the lint suppression with a targeted // refreshKey is the proxy for iframe content changes explanation.

3. No confirmation on destructive branch operations

deleteSequence removes the branch AND strips all hotspots targeting it — a potentially significant data loss. There's no confirmation dialog. For a v1 this might be acceptable, but consider at minimum a window.confirm or noting this as a follow-up.

4. Date.now() for sequence/hotspot IDs

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

Two rapid clicks within the same millisecond produce duplicate IDs. crypto.randomUUID() or a counter would be more robust. Low risk in practice (UI debounce makes sub-ms clicks unlikely), but worth a defensive fix.

5. Fragment key={t} uses floating point values as keys

{fragments.map((t) => (
  <span key={t} ...>

If two fragments have the same time value (shouldn't happen given dedup, but defensively), React gets confused. A stable index or ${t} string would be equivalent but more explicit.

---

Observations (non-blocking)

Architecture is strong. The makeSlideshowNotesController is a genuinely good pattern — extracting the debounce + flush-attribution logic into a pure, testable controller that the component just wires up. The stale-closure tests (typing in comp A, switching to comp B, flushing to A) prove a subtle invariant that would otherwise be a bug magnet. Well done.

Test coverage is thorough. 460 lines of pure-logic tests covering every manifest transform and the notes controller's concurrency invariants. The tests don't need React rendering, which keeps them fast and focused.

Accessibility is decent. aria-expanded, aria-pressed, aria-label on interactive elements, keyboard handlers on role="button" divs. The SectionHeader correctly uses a <button> with aria-expanded. The slide list items handle Enter/Space keyboard events. A few gaps:

• The BranchItem rename span has role="button" + tabIndex={0} + keyboard handler — good.
• The SlideList scene rows use role="button" with aria-pressed — technically aria-pressed is for toggle buttons. Since clicking selects (not toggles), aria-selected within a role="listbox" would be more semantically correct. Non-blocking.

Styling is consistent. Uses the same text-[11px], bg-neutral-800, border-neutral-700, text-neutral-400/500 tokens as the existing studio panels. The studio-accent color variable for active states matches the design system.

Performance is fine for v1. The slideshowScenes useMemo and the scenes.map() in BranchTree are O(n) where n = scene count — negligible for typical slideshow sizes (tens of scenes, not thousands). The slides.some() lookup inside each scene row is O(n*m) but again trivially small. No concerns at realistic scale.

The compHtml effect has a subtle ordering concern — it calls flush() synchronously then sets state. The flush fires persist(manifest) which is async, so in theory the new compHtml's parse could race with the old comp's persist callback. The useSlideshowPersist hook (from a prior PR in the stack) presumably handles this via the save queue's coalesce key, so it's fine in practice, but worth a comment.

---

Summary

Good PR. The duplicate import is a definite cleanup; the Date.now() ID generation is a minor robustness gap. Everything else is solid. I'd approve after the duplicate import is removed.

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

HEAD reviewed: 3f9554059cfeb40399e8363f856b5da7b84a1138

Verdict: Clean upper-half of the #1582 split — UI is well-decomposed into SubPanels with proper a11y on most interactions. Layering Miga's review with one correction and a few additional findings. Posted in full on the monolith (#1582) and persistence (#1591) too so you have the cross-cut.

Split verification — coverage of #1582

Same byte-identical split confirmation as #1591 — #1591 (488 LOC, 6 files: persistence + helpers + studioHelpers tab type) + #1592 (1448 LOC, 5 files: App + StudioRightPanel + SlideshowPanel + SlideshowSubPanels + SlideshowPanel.test) = #1582 (1936 LOC, 11 files) exactly. Per-file diff hashes match. No file in both halves; no file missing. Mechanical, non-lossy.

Blockers

None.

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

Miga says the import { ... } from "./slideshowPanelHelpers" at SlideshowPanel.tsx:56-67 is a copy-paste accident and should be removed because the export { ... } from "./slideshowPanelHelpers" re-export at lines 32-44 already brings the names in scope. It does not — please do NOT remove the local import.

export { X } from "./Y" is a re-export only. It forwards X to consumers of this module but does NOT introduce X as a local binding inside the module itself. The helpers are used as values 11 times locally: toggleMainLineSlide/reorderMainLineSlide/... at SlideshowPanel.tsx:263, 270, 278, 285, 291, 299, 306, 313, 320, 329, 336. Removing the local import would break the file at runtime with ReferenceError: toggleMainLineSlide is not defined. (TS would also error in strict mode; bundlers don't dedupe local references to non-existent bindings.)

The re-export exists so SlideshowPanel.test.ts can do import { ... } from "./SlideshowPanel" and have one import for panel + helpers (verified: the test file at lines 2-16 relies on this). The local import exists because the component itself uses the helpers. Both are load-bearing. Awkward but correct.

If you want to flatten in a follow-up, the right shape is import * as helpers from "./slideshowPanelHelpers" + every call site becomes helpers.toggleMainLineSlide(...). Not worth doing for this PR.

Concerns

2. 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]);

Miga right that previewIframeRef as a dep is a no-op (ref identity is stable). The real reactivity hangs entirely on refreshKey from useStudioPlaybackContext(). If the user opens the Slideshow tab before the iframe's player has hydrated __clipManifest, the panel shows scenes.length === 0 and won't re-derive until the next refreshKey bump (which happens on player refresh, not on first-time hydration as far as I can tell). The Slides list will show empty with no indication that the iframe just hasn't loaded yet.

Suggest: at minimum, an empty-state line when scenes.length === 0 distinguishing "no clips in the composition" from "preview still loading." Better: subscribe to whatever signal fires when __clipManifest populates (looks like the player store would have one — usePlayerStore could expose a manifestLoaded flag, or refreshKey could bump on first hydration).

3. Date.now() ID collisions silently drop the second click — SlideshowPanel.tsx:298, SlideshowSubPanels.tsx:385 (concur Miga with sharper consequence)

Miga raised the millisecond-collision risk. The user-experience hit is worse than her framing implies:

• handleCreateSequence → createSequence (helper at slideshowPanelHelpers.ts:80) silently returns the manifest unchanged when an id already exists. No throw, no warning.
• handleMakeHotspot → addHotspot (helper at slideshowPanelHelpers.ts:158) does the same.

A double-tap within 1ms — programmatic burst (autosave + user click landing in the same tick), fast trackpad — silently drops the second action. The .catch(() => {}) on the call site doesn't surface anything because the helper returns the unchanged manifest by design.

Fixes (any one):

• crypto.randomUUID() (Studio is a browser env — supported in current targets).
• Counter on a ref: const counterRef = useRef(0); const id = \seq-${Date.now()}-${counterRef.current++}``.
• At minimum: log a console.warn in the helper when it hits the duplicate-ID branch so the silent failure is observable in dev.

4. Fragment key={t} uses float values as React keys — SlideshowSubPanels.tsx

Miga noted this. Concur. The fragments array is deduped via Set (which uses strict equality on floats — same float-equality concern from #1582/#1591), but if two near-identical floats slip past dedup, React renders both with the same key and reconciliation gets confused. ${scene.id}-${t} or array index would both be safer. Same-shape fix as the helper float-equality concern.

5. No confirmation on handleDeleteSequence — destructive cascade — SlideshowPanel.tsx:313

deleteSequence cascades to remove every hotspot targeting the deleted sequence from both main-line and other sequences' slides. A misclick deletes the branch AND silently drops N hotspot bindings across the manifest. Undo recovers it, but a window.confirm("Delete branch '<label>'? This will also remove N hotspots targeting it.") (with the live count from manifest.slides.flatMap(s => s.hotspots ?? []).filter(h => h.target === id).length + sequences.flatMap(seq => seq.slides.flatMap(s => s.hotspots ?? [])).filter(h => h.target === id).length) would be the friendlier shape.

Concur Miga; called out specifically here because the destructive action surface lives in this PR.

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

Every discrete-action handler does applyManifest(...).catch(() => {}) (SlideshowPanel.tsx:263-336). A failed writeProjectFile (disk full, OPFS quota, FS permission) is silently swallowed; the in-memory manifest state shows the edit, disk doesn't. The user reloads the comp later and the edit is gone with no signal it failed.

Same concern on applyNotesManifest → the controller's persist(p.manifest).catch(() => {}) at SlideshowPanel.tsx:107, 122 swallows the notes failure equally silently.

Question: is there an app-level toast wiring elsewhere that surfaces write failures generically (AppToast is in studioHelpers.ts:7-10), or does the panel need explicit error UI here?

Nits

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

role="button" aria-pressed={isSelected} — aria-pressed is toggle-button semantics. The row's role here is "selected item in a list"; the correct shape is parent role="listbox" + child role="option" aria-selected={isSelected}. Doesn't affect mouse users; screen readers will say "pressed" instead of "selected." Miga also flagged.

8. // fallow-ignore-next-line complexity on HotspotTool and handleMakeHotspot — SlideshowSubPanels.tsx:365, 383

The component is straightforward; the handler is 5 lines. Looks like the lint rule is over-flagging. Worth tuning if it keeps tripping clean code, or annotating WHY the rule misfires here so the next reader doesn't assume there's complexity to refactor.

9. PR description is the default template

For the cross-compare with #1591 + #1582 to be fast, even a one-liner per section ("UI half of the #1582 split; persistence/helpers in #1591; ss-studio-a base") would help reviewers. The Graphite stack comment helps but doesn't say what each half contains.

Questions

10. compHtml effect ordering — is the OLD-comp flush guaranteed to write to the OLD callback?

SlideshowPanel.tsx:188-202: on compHtml change, notesCtrlRef.current.flush() fires before setManifest(parsed). The flush fires p.persist(p.manifest) where p.persist was captured when schedule() was called. So pending notes for COMP-A should write to COMP-A's onPersistNotes callback even after editingFile switches to COMP-B. But the onPersistNotes callback closes over activeCompPath from useSlideshowPersist's useCallback. If activeCompPath updates synchronously with editingFile.content, the persist closure points at COMP-B's path before the flush runs.

Miga noted the same ordering concern. Is activeCompPath derived from a different signal than editingFile.content, or do they update in the same React commit? If the latter, the flush could mis-route. Worth a comment + a unit test that explicitly covers "type in A → switch to B → expect A's file to have the notes, B's file untouched."

What I didn't verify

• The full compHtml-effect ordering interaction with the React commit phase (see Q10).
• Whether the editingFile.content lifecycle re-renders the panel cleanly when compHtml is mid-stream (e.g. a partial save reads partial bytes). safeParseManifest swallows this silently, returning { slides: [] } — same concern as #1591 about the user not seeing the parse-failure signal.
• Performance of slideshowScenes.map + scenes.map + BranchTree re-render on a slideshow with hundreds of sequences/hotspots. For v1 targets this is fine (Miga right that 10s of scenes is realistic); flagged in case the canvas-renderer ever feeds a high-cardinality clip-list back through this path.

Review by Rames D Jusso

The base branch was changed.