fix(slideshow): finish remaining split-PR review findings

[vanceingalls]

Follow-up to #1585 (merged). Addresses the remaining open findings from the #1580/#1590/#1591/#1592 reviews — the items that weren't already fixed on #1585. Branches off main since the slideshow stack is merged.

Findings addressed

core (#1580)

• Duplicated isSceneLikeCompositionId — extracted to slideshow/sceneId.ts, imported by both the lint rule and the runtime scene-window computation (was a mirror-and-drift copy).
• (also on fix(slideshow): address code-review findings #1580-1584 #1585, included via cherry-pick) isManifest rejects non-object/array manifests; resolveSlideshow flags duplicate sequence ids.

player (#1590 / #1592)

• Multi-instance key conflict — when several decks share a page, onKey drops the unfocused-convenience so a key drives only the focused deck.
• Scene race with iframe hydration — if the scene timeline posts after waitForScenes times out (empty scenes → sceneId slides dropped), re-init once when it finally arrives.
• (via fix(slideshow): address code-review findings #1580-1584 #1585 cherry-pick) present() noopener,noreferrer; per-deck BroadcastChannel name; observedAttributes/attributeChangedCallback.

studio (#1591)

• Write-time validation — persistSlideshowManifest confirms the built island round-trips to a valid manifest before writing.
• reorderBranchSlide — new helper + BranchTree up/down controls (parallel to main-line reorder) with a branch-position indicator.
• (via fix(slideshow): address code-review findings #1580-1584 #1585 cherry-pick) persist no-op gate; surfaced persist failures; delete-branch confirmation.

Notes

• The redundant split PRs feat(player): slideshow controller state machine #1589–1592 should be closed — their content is in main via the combined merge; this PR + fix(slideshow): address code-review findings #1580-1584 #1585 carry the fixes.
• Findings already resolved on fix(slideshow): address code-review findings #1580-1584 #1585 and verified here: CSP handlers, manifest version, Date.now→crypto.randomUUID, float React keys, presenter 1s-timer. (unused SceneInfo was stale — it's used throughout.)
• #1590.1 (full re-render every onChange) is the component's by-design model; the timer + fs-glyph hotspots are fixed.

core 228 / player 106 / studio 46 (panel) tests pass; tsc / oxlint / oxfmt / fallow clean.

🤖 Generated with Claude Code

[miga-heygen]

Review — PR #1594: fix(slideshow): finish remaining split-PR review findings

Solid cleanup PR. Systematically ticks off the outstanding review items from the split-PR stack. Here is a per-finding scorecard and a few observations on the new code.

---

Findings addressed in this PR

Finding	Source	Status	Notes
Duplicated isSceneLikeCompositionId	#1580	Fixed	Extracted to slideshow/sceneId.ts, imported by both timeline.ts and lint rule. Single source of truth — drift eliminated.
Shallow isManifest() (passes [42, null])	#1580	Fixed	Array.isArray(v) guard added; test added for the array case.
Sequence-id collision (silent overwrite)	#1580	Fixed	resolveSlideshow now pushes an error on duplicate ids. Test added. Note: it warns but still keeps the last one — the error message says "only the last definition is kept" which is honest, but the caller doesn't get the chance to reject the manifest. Acceptable for a lint-time check; worth considering a hard throw if this ever gates a write path.
Multi-instance key conflict	#1590	Fixed	document.querySelectorAll("hyperframes-slideshow").length > 1 disables ambient keyboard handling. Functional, though it's a DOM query on every keydown — fine for a keyboard handler's cadence.
BroadcastChannel name unhardened	#1590	Fixed	slideshowChannelName() keys on location.pathname. Good. Tests updated. Exported so tests can use the same derivation.
Missing observedAttributes	#1590	Fixed	static get observedAttributes returns ["sound", "mode"]; attributeChangedCallback re-renders.
present() lacks noopener,noreferrer	#1590	Fixed	Third arg added to window.open.
No-op gate in persistSlideshowManifest	#1591	Fixed	if (after === current) return; — clean.
Silent .catch(()=>{}) on persist failures	#1591	Fixed	Both the debounced notes persist and the onPersist wrapper now console.error.
No delete confirmation on branch sequences	#1592	Fixed	window.confirm with count and label. Good UX copy.

New functionality added

• reorderBranchSlide helper + UI buttons: Refactored the swap logic into a shared swapSlide function, reused by both reorderMainLineSlide and the new reorderBranchSlide. Tests cover both directions + boundary/unknown cases. Clean.
• Slow-iframe recovery: Re-init listener when scenes arrive late (empty at first waitForScenes timeout). The gen === this.initGeneration guard is good — prevents stale listeners from re-initing after a subsequent call has already taken over.
• Write-time validation: persistSlideshowManifest round-trips the built island HTML through parseSlideshowManifest before writing. Belt-and-suspenders — catches malformed edits before they corrupt the composition file.

---

Observations / minor items

1. handleReorderBranchSlide swallows persist errors (SlideshowPanel.tsx:299):

   applyManifest(reorderBranchSlide(manifestRef.current, sequenceId, sceneId, dir)).catch(
     () => {},
   );

   This is the same .catch(() => {}) pattern that was flagged on #1591 and fixed for notes persist and the onPersist wrapper a few lines above. The applyManifest call does now have console.error inside it via the try/catch on line 226, so the outer .catch is probably a dead path — but a bare swallow is still code-smell-adjacent. Same applies to handleDeleteSequence at line 361. Consider console.error for consistency, or remove the outer .catch entirely if applyManifest is guaranteed not to reject.

2. handleDeleteSequence also swallows (SlideshowPanel.tsx:361):
   Same pattern — applyManifest(...).catch(() => {}). Same note as above.

3. Multi-instance detection queries the DOM on every keydown (hyperframes-slideshow.ts:347):

   const multiInstance = document.querySelectorAll("hyperframes-slideshow").length > 1;

   Not a perf concern at keyboard-event cadence, but if this ever matters, a static counter incremented/decremented in connectedCallback/disconnectedCallback would be O(1). Non-blocking.

4. slideshowChannelName uses location.pathname only — two decks on the same path but different query params (e.g. ?v=1 vs ?v=2) share a channel. Probably fine since the audience window inherits the presenter's full URL, but worth a comment if the assumption is intentional.

5. Duplicate sequence id: warn but keep — As noted above, the duplicate id detection in resolveSlideshow pushes an error string but still overwrites. This is fine for lint/resolution — the caller can inspect errors — but if persistSlideshowManifest ever calls resolveSlideshow as part of its validation, duplicates would persist to disk with only a console warning. Currently not an issue since the write-time validation uses parseSlideshowManifest (which doesn't check sequence id uniqueness), but worth noting for future hardening.

---

Findings from the original reviews NOT in scope for this PR (claimed resolved on #1585 or by-design)

These are noted as resolved in the PR description. I have not re-verified them here since they're on main already:

• innerHTML full rebuild every render (#1590) — described as by-design
• CSP-unsafe inline handlers (#1590) — fixed on #1585
• Missing version field on SlideshowManifest (#1591) — fixed on #1585
• Date.now() for IDs (#1592) — fixed on #1585
• Suppressed exhaustive-deps lint (#1592) — fixed on #1585
• Float values as React keys (#1592) — fixed on #1585

---

Verdict

The PR addresses the outstanding findings correctly and introduces well-structured new code (shared swapSlide, write-time validation, slow-iframe recovery). Test coverage is solid for the new paths. The few items above are minor and non-blocking.

Recommend approval once the .catch(() => {}) inconsistency in the two new handlers is addressed (items 1-2 above) — or if you're fine with it given applyManifest's internal try/catch already covers it, that's a reasonable call too.

Review by Miga

[miguel-heygen]

LGTM