feat(studio): slideshow manifest persistence + panel helpers

[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 👈 (View in Graphite)
• 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]

Review: slideshow manifest persistence + panel helpers

Clean, well-scoped PR. The split into three layers (pure transforms, persistence util, React hook) is well-reasoned and each layer earns its keep. A few observations:

---

1. Persistence design — solid

The <script type="application/hyperframes-slideshow+json"> island approach is the right call. It lives in the HTML document like a standard JSON-LD island, doesn't touch the SDK element tree, and the "strip all stale islands → insert one fresh island" strategy in persistSlideshowManifest is defensive without being over-engineered. The comment about <script> nodes having no hf-id clearly justifies why this bypasses setText dispatch.

The < / > escaping in buildSlideshowIslandHtml correctly prevents </script> breakout, and the round-trip through JSON.parse restores the original values. The test for this ("does NOT embed a literal </script> inside the JSON body") is well-crafted.

Exporting persistSdkSerialize from sdkCutover.ts with a clear ponytail comment explaining why (third caller — island write) is the right aperture. No unnecessary abstraction layer introduced.

2. Panel helpers — genuinely reusable

slideshowPanelHelpers.ts is 195 lines of pure functions with zero React, zero side-effects. This is the ideal shape for manifest transforms: each function takes a SlideshowManifest and returns a new one. They'll compose trivially with whatever state management lands later (Zustand, useReducer, etc.).

A few notes:

• toggleMainLineSlide: Appending to the end on add is fine for now, but worth considering whether insert-at-cursor or insert-at-scene-timeline-position would be more intuitive once the UI is wired up. Not a blocker — just a future consideration.

• setSlideNotes: The auto-add behavior (creates a slide entry if one doesn't exist when setting notes) is a nice touch that prevents "set notes → nothing happened because slide wasn't added" confusion.

• pruneHotspots: Smart to cascade-delete hotspot references when deleting a sequence. The nested traversal through both main-line slides and sequence slides is thorough.

• addFragment: The Set + sort dedup is correct but worth noting: Set deduplication uses strict equality, which is fine for number[] as long as fragments are always exact values (not floating-point approximations). If fragment times ever come from timeline scrubbing with sub-millisecond precision, you might want an epsilon-based dedup. Very minor — unlikely to bite in practice.

3. Data integrity — the important question

Corrupt/partial saves: The PR inherits persistSdkSerialize's write path, which is a single writeProjectFile call. If that write is interrupted (browser crash, tab close), you get a partial file. This is the same risk surface as every other SDK cutover path, so it's not new here — but since the slideshow manifest is authored data (notes, hotspots, branch sequences), data loss stings more than losing a style change. No blocker, but worth thinking about whether the save-queue's coalesce infra (which this PR correctly threads via coalesceKey) could also provide a write-ahead log or at least an in-memory rollback snapshot.

Migration path: parseSlideshowManifest does a minimal structural check (isManifest — is it an object with a slides array?) and then trusts the shape. If a future PR adds required fields to SlideshowManifest, old islands will parse but silently lack those fields. Consider a version field in the manifest now (even just version: 1) so future migrations can detect and upgrade. This is the cheapest thing to add early and the most painful thing to retrofit later.

No validation on persist: buildSlideshowIslandHtml serializes whatever SlideshowManifest it receives — it doesn't validate that sceneId values actually reference existing scenes, that hotspot targets reference existing sequences, etc. The parser side (resolveSlideshow) does validate these, which is good, but persisting an invalid manifest means the error surfaces on the next load, not at write time. Worth considering a validate-before-persist guard, at least in dev mode.

4. Integration with branching editor (#1582)

The branch-sequence helpers (createSequence, renameSequence, deleteSequence, assignToBranch) map cleanly to the SlideSequence type from core. The hotspot helpers (addHotspot, removeHotspot) reference sequences by ID, which is the right coupling level. The cascade-delete in deleteSequence (removing targeting hotspots) prevents dangling references.

One gap: there's no reorderBranchSlide equivalent for branch sequences — you can reorder main-line slides but not slides within a branch. Intentional deferral, or oversight?

5. Minor items

• SceneInfo interface in slideshowPanelHelpers.ts (lines 10-15) is defined but never used in this PR. If it's consumed by the panel component in a later PR in the stack, that's fine — but if not, it's dead code.
• The fallow-ignore-next-line complexity comment on reorderMainLineSlide — this function is straightforward (swap two array elements with bounds checking). Is fallow flagging it due to the early returns? Seems like a false positive worth noting in case the lint rule needs tuning.
• The fallow-ignore-next-line unused-export on the re-export in studioHelpers.ts — is the COMPOSITION_ROOT_OPEN_TAG_RE re-export actually consumed anywhere, or is this just suppressing a lint warning for a genuinely unused export?
• PR description is the default template with no sections filled in. For a 10-PR stack, even a one-liner per section ("Persistence layer for slideshow manifest; pure transform helpers for the panel") helps reviewers navigate.

Verdict

Well-structured, good test coverage for the persistence path (especially the </script> breakout and double-island collapse tests), clean separation of concerns. The main suggestion is adding a version field to the manifest now before it ships — everything else is minor or future-facing.

Would approve with the version field added (or a convincing argument for why it's unnecessary).

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

HEAD reviewed: bc2d796ad2de451bfb9181095c2c8de1a4a456fd

Verdict: Clean lower-half of the #1582 split — persistence layer is well-scoped, helpers are pure, tests are thorough. A few substantive concerns inherited from the parent monolith review; posted in full there (#1582) and on the UI half (#1592). Layering Miga's review.

Split verification — coverage of #1582

Compared gh pr diff 1582 vs gh pr diff 1591 + gh pr diff 1592 (the two split halves) file-by-file. Split is byte-identical, non-lossy:

• Per-file diffs match exactly between #1582 and the corresponding split half (all 11 files).
• Files in this PR (#1591): slideshowPanelHelpers.ts, useSlideshowPersist.ts, sdkCutover.ts, setSlideshowManifest.ts, setSlideshowManifest.test.ts, studioHelpers.ts — 6 files, +488/-3.
• Files in #1592: App.tsx, StudioRightPanel.tsx, SlideshowPanel.tsx, SlideshowSubPanels.tsx, SlideshowPanel.test.ts — 5 files, +1448/-4.
• 6+5 = 11 files = #1582's 11.
• 488+1448 = 1936 = #1582's 1936 additions (exact).
• 3+4 = 7 = #1582's 7 deletions (exact).

No file is in both halves. No file is missing from the union. The split is a mechanical separation, not a refactor — every helper, persist hook, and SDK-cutover export in #1591 is identical to the version in #1582.

Blockers

None.

Concerns (apply to this PR specifically)

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

Every other caller in sdkCutover.ts does if (after === before) return false before invoking persistSdkSerialize (e.g. sdkCutover.ts:198-199). The slideshow persist always replaces the island and writes, even when the resulting HTML is identical to current. Result: re-typed-same-key notes (post-debounce), toggle-then-untoggle a checkbox, reorder out of bounds — all produce a disk write, undo entry, and preview reload.

Fix: compare after === current immediately before calling persistSdkSerialize; bail if equal. Cheap, matches the sibling shape.

2. useSlideshowPersist reads originalContent per-call → race with concurrent writers — useSlideshowPersist.ts:39

const originalContent = await readProjectFile(path);
await persistSlideshowManifest({ manifest, sdkSession, originalContent, ... });

The originalContent is the undo before baseline. Other cutover writers (e.g. sdkCutoverPersist) take originalContent from their caller, where it was snapshotted before sdkSession.dispatch(...) mutated state. Here, the read happens fresh every persist call. In the single-writer studio model this is fine, but if any DOM-edit or design-panel write lands between readProjectFile and writeProjectFile, the undo entry's before will be that interleaved write's contents, not what the user actually saw before they typed. The sdkSelfWriteRegistry (hooks/sdkSelfWriteRegistry.ts:11-30) suppresses self-echo reloads but doesn't serialize concurrent writers — the race is theoretical today but worth a comment in useSlideshowPersist documenting the single-writer assumption.

3. Manifest version field — concur with Miga

SlideshowManifest (packages/core/src/slideshow/slideshow.types.ts:3-6) has no version field. Adding version?: 1 now + threading into buildSlideshowIslandHtml costs ~3 lines and converts a future migration from "best-effort inference" to switch (manifest.version ?? 0). Cheapest to add before any author hand-edits a manifest in the wild. The fact that this PR introduces the persistence side of the island makes it the right moment to bake the version in.

4. Date.now() collisions silently drop user edits in helpers — slideshowPanelHelpers.ts:80, 158 (concur Miga with sharper consequence)

createSequence silently returns the manifest unchanged when an id already exists. addHotspot does the same. A double-click within 1ms (programmatic burst, fast trackpad double-tap) produces a duplicate seq-${Date.now()} ID; the second click is dropped with no UI feedback. The .catch(() => {}) in the call site doesn't help — the helper returns the unchanged manifest by design, not throwing. The ID generation lives in #1592's UI (SlideshowPanel.tsx:298, SlideshowSubPanels.tsx:385), but the silent-rejection helper contract lives here. Suggest at minimum a console.warn when the helper hits the duplicate-ID branch — converts a silent no-op into an observable signal.

5. pruneHotspots traversal — confirm intent on nested branch hotspots — slideshowPanelHelpers.ts:117-122

deleteSequence prunes hotspots from manifest.slides AND from seq.slides for every remaining sequence. Looks right. Worth a regression test that explicitly covers "delete sequence A; hotspots targeting A on a slide inside sequence B are pruned" — I see the helper tests cover the main-line cascade but didn't trace whether the cross-branch cascade has a dedicated test. (Tests live in #1592's test file at SlideshowPanel.test.ts — the test gate is on the wrong side of the split: pure-helper tests test the helpers shipped in this PR, but ride in the UI PR. See item 7.)

Nits

6. The "ponytail: third caller" comment is misleading — sdkCutover.ts:147

persistSdkSerialize has 5 callers internal to sdkCutover.ts (the various sdk*Persist flavors) plus the new external caller in setSlideshowManifest.ts. "Third caller" reads as if a public-API count was being tracked; the more accurate phrasing is "first external caller" or "first cross-module caller." Not load-bearing.

7. Helper unit tests ship in #1592, not here

The pure-function tests at SlideshowPanel.test.ts (460 lines) exercise the helpers introduced in this PR, but the test file lives in #1592 because it co-locates with the panel. If #1591 lands and #1592 stalls, the helpers are untested at merge. Two options:

• Move slideshowPanelHelpers.test.ts into #1591 (tests follow code).
• Document that #1591 cannot merge independently of #1592 (Graphite already enforces this; just worth being explicit).

The persistence-side tests (setSlideshowManifest.test.ts, 141 lines) DO ship in #1591 — those are the ones that cover the </script> breakout and double-island collapse. Good.

8. PR description is the default template

For a 10-PR stack where the cross-compare relies on each PR's intent being explicit, the default ## What / ## Why / ## How / ## Test plan template makes review harder. A one-liner per section would let me confirm "this is the persistence layer; UI lives in #1592; ss-player-b base" without piecing it together from the branch names. (Same nit on #1592.)

Questions

9. Why the asymmetric slideshow lint rule from #1580 is silenced by safeParseManifest here?

safeParseManifest catches and warns on parse failure (SlideshowPanel.tsx:48), returning { slides: [] }. The lint rule from #1580 flags slideshow_invalid for malformed islands. If a malformed island opens in studio, the user sees the empty-slides state silently (no toast, no inline warning) and their next edit overwrites the malformed island with the new manifest — losing whatever the malformed content was trying to be. Intentional (user is editing; the malformed thing is being replaced anyway), or should studio surface the lint finding to the user before they edit?

What I didn't verify

• Whether the coalesceKey in recordEdit actually collapses to ONE undo entry per burst as the doc-comment claims, or N entries sharing a key. The PR description claims debounced-notes = "one undo entry per burst" — I trusted that.
• Whether the sdkSelfWriteRegistry hash window correctly identifies the slideshow persist write as a self-echo vs an external change. The persistSdkSerialize path calls markSelfWrite(targetPath, after) (sdkCutover.ts:160); the slideshow island write goes through the same path, so it should be tagged. But I didn't trace the useSdkSession.ts:150 "single writer" comment to confirm the slideshow path doesn't bypass it.
• Concurrency with writeProjectFile if it's not already serialized at the studio shell level. The single-writer assumption in (2) is the place to check.

Review by Rames D Jusso

The base branch was changed.

[miga-heygen]

Re-review — 4c57d68 (post-feedback HEAD)

PR scope expanded: now includes the SlideshowController state machine and <hyperframes-slideshow> web component (2 new commits from the #1581 split), plus the original studio persistence layer. Reviewed the new code; focusing this comment on the original findings and whether they've been addressed.

---

Status of prior findings

1. Missing version field on SlideshowManifest — STILL OPEN

SlideshowManifest in @hyperframes/core/slideshow still has no version field. The type isn't touched in this PR, so it wasn't added. Both Rames and I flagged this. The argument remains: adding version?: 1 now (3 lines — type field + serialize it in buildSlideshowIslandHtml + thread into parseSlideshowManifest) converts future migrations from best-effort inference to a switch on a known field. The persistence island ships in this PR — once manifests are on disk in the wild, retrofitting a version field becomes a migration problem instead of a definition problem.

2. No reorderBranchSlide helper — STILL OPEN (acknowledged deferral)

reorderMainLineSlide exists but no equivalent for branch-sequence slides. If this is an intentional deferral for a later PR, a // TODO: or a brief note in the PR description would prevent this from being re-flagged.

3. SceneInfo interface defined but unused — STILL PRESENT

SceneInfo (slideshowPanelHelpers.ts:10-15) is exported but never referenced in any file in this PR. If it's consumed by the panel component in #1592, that's fine — but it rides in the wrong PR. Either move it to #1592 where its consumer lives, or add a comment noting which downstream PR consumes it.

4. No write-time validation of manifest integrity — STILL OPEN

buildSlideshowIslandHtml serializes whatever SlideshowManifest it receives. resolveSlideshow validates on the read side (and the new console.warn for manifest errors at hyperframes-slideshow.ts:885 is good), but a bad manifest can still be persisted and the error only surfaces on the next load. Not a blocker — the read-side validation is the critical path — but a validateBeforePersist guard (even dev-only via process.env.NODE_ENV) would catch issues at write time rather than next-session-open time.

5. Missing no-op gate in persistSlideshowManifest — STILL OPEN (Rames' finding)

persistSlideshowManifest (setSlideshowManifest.ts:62-78) still always strips + re-inserts the island and writes, even when the resulting HTML is byte-identical to current. Every other persistSdkSerialize caller does if (after === before) return before invoking the shared writer. The comment on persistSdkSerialize itself says "which also did the no-op check" — but this caller doesn't honor that contract.

The fix is ~2 lines: after computing after (line ~78), add if (after === current) return; before the persistSdkSerialize call. This prevents undo entries, disk writes, and preview reloads on re-typed-same-value notes, toggle-then-untoggle checkboxes, and out-of-bounds reorders.

6. Silent duplicate-ID rejection in helpers — STILL SILENT (Rames' finding #4)

createSequence and addHotspot silently return the manifest unchanged when a duplicate ID is passed. No console.warn was added. The ID generation lives in #1592 but the silent-rejection contract lives here. A console.warn on the duplicate-ID branch converts a silent no-op into an observable signal — useful for catching double-click / programmatic-burst race conditions.

7. useSlideshowPersist single-writer assumption undocumented — STILL UNDOCUMENTED (Rames' finding #2)

The originalContent = await readProjectFile(path) call happens fresh per persist, creating a theoretical race with concurrent writers. The coalesceKey mitigates rapid-fire writes but doesn't serialize against other cutover writers. A comment documenting the single-writer assumption (as Rames suggested) would prevent future developers from accidentally breaking this invariant.

---

New code observations (SlideshowController + web component)

The new player-side code is well-structured. A few notes:

• Controller state machine — clean stack-based navigation with proper fragment handling. The resumeSlide path that preserves fragmentIndex on back() is the right design. Tests are thorough (574 lines covering linear nav, branching, fragment sequencing, edge cases).

• initInFlight re-entrancy guard — good. The generation counter (initGeneration) correctly handles disconnect-during-async-init. The try/finally ensures initInFlight is always cleared.

• waitForScenes timeout fallback — the 2500ms timeout with graceful [] fallback is sensible. The aborted callback parameter for early bail-out on disconnect is a nice touch.

• BroadcastChannel catch — the catch {} at slideshowPresenter.ts for unsupported environments is appropriate (feature detection, not error swallowing).

• Global _keyframesInjected flag — works for single-document use but would break if the element is used across documents (e.g. iframes). Not a concern for the current use case but worth noting if <hyperframes-slideshow> is ever embedded in a multi-document context.

---

Summary

Finding	Status
version field on manifest	Open
reorderBranchSlide	Open (deferral OK if noted)
SceneInfo unused	Open
Write-time validation	Open (non-blocking)
No-op gate in persist	Open
Silent duplicate-ID rejection	Open
Single-writer assumption docs	Open

The no-op gate (#5) and version field (#1) are the two I'd want addressed before merge. The rest are either non-blocking or can ride in follow-ups.

Re-review by Miga