fix(slideshow): address code-review findings #1580-1584

[vanceingalls]

What

Addresses the findings from a max-effort code review of the slideshow stack (#1580 core → #1581 player → #1582 studio → #1583 skill → #1584 examples). One fixup branch stacked on top of ss-examples; no example/skill files are touched.

15 findings fixed across @hyperframes/core, @hyperframes/player, and @hyperframes/studio, including one feature gap (branch-slide authoring) that was promoted into scope.

Why it matters

Two of these broke the slideshow's flagship paths outright:

• The standalone slideshow never loaded. tsup externalizes dependencies by default, so the new IIFE/global build (hyperframes-slideshow.global.js, loaded by the demos via <script src>) emitted an unresolved import of @hyperframes/core → threw at eval → custom element never defined.
• Presenter → audience sync was dead. present() opens the audience window at ?mode=audience, but the component only read the mode attribute — nothing mapped the query param — so the audience window ran as a second presenter and never followed.

Fixes

Player — @hyperframes/player

Fix	Detail
Bundle core into the global build	tsup.config.ts now sets noExternal: ["@hyperframes/core"] (mirrors shader-transitions bundling html2canvas); the IIFE build inlines core (verified 0 unresolved imports).
Audience mode from URL	New resolveMode() reads the mode attribute, falling back to ?mode=audience from location.search; used at every mode check. The audience window now actually enters audience mode.
Audience mirrors full position	The audience handler now calls a new SlideshowController.syncTo(sequenceId, slideIndex, fragmentIndex) instead of goToSlide(slideIndex). Branches and fragment state are mirrored statically (via resumeSlide) instead of being dropped (branches) or replayed from slide start (fragments).
Event-driven scene wait	hyperframes-player dispatches a scenes event when the runtime timeline arrives; waitForScenes resolves on that event (with the 2500 ms poll retained as a fallback) instead of pure polling, and init() now logs a loud console.error when no main-line slides resolve instead of failing silently to an empty deck.
Keyboard no longer hijacks the host page	onKey is window-level. Arrows still work when nothing is focused (freshly loaded deck), but Space/Backspace — which have strong page defaults (scroll / history) — only navigate when the slideshow actually has focus, so an embedded deck no longer kills page scrolling.
next() reveals trailing fragments	Removed the atEnd gate so remaining fragments are revealed even when playback is already at slide end (previously it skipped straight to the next slide, losing builds).
enterBranch ignores empty/unknown sequences	Prevents the soft-lock where entering a 0-slide branch left the controller with no current slide.
extractScenes hardened	Cast-free, null/non-object-guarded filter — a timeline message carrying scenes: [null] (or primitives) no longer throws and aborts timeline handling.

Core — @hyperframes/core

Fix	Detail
Strict manifest validation	isManifest now validates slideSequences is an array and that every slide / sequence element is a well-formed object (isSlideRef / isSlideSequence). A parseable-but-malformed island (e.g. slideSequences: {}) is now rejected as slideshow_invalid instead of throwing out of resolveSlideshow — which, because the linter runs rules with no per-rule try/catch, previously crashed npx hyperframes lint.
Inverted-range error	An explicit startTime >= endTime is now reported as a lint error (it was silently dropped at runtime by dropInvalidSlides while lint stayed green).
Empty hotspot-target error	A hotspot targeting a sequence with no slides is now flagged (previously passed lint, soft-locked the player at runtime).
Fragment de-duplication	resolveSlide de-dupes fragment times (new Set), fixing the strand where duplicate hold-points trapped next() on indexOf's first match.
Lint matches runtime scene durations	parseTiming now accepts data-end / data-hf-authored-end in addition to data-duration, mirroring the runtime (timeline.ts), so GSAP-/data-end-driven scenes no longer produce false slideshow_unresolved_ref errors.
Shared island constant + regex	Exported SLIDESHOW_ISLAND_TYPE and slideshowIslandRegex(flags) so the reader (core parser) and writer (studio persist) share one definition instead of two drift-prone copies.

Studio — @hyperframes/studio

Fix	Detail
Slide list reflects order	SlideList now renders main-line slides in manifest.slides order (then non-slide scenes), so the up/down reorder arrows are actually visible; membership uses a Set (O(n)).
Branch-slide authoring (#14)	New mapSlidesIn helper; setSlideNotes / addFragment / removeFragment / addHotspot / removeHotspot take an optional sequenceId, so notes / fragments / hotspots can be authored on slides inside a branch (not just the main line). BranchTree lets you select an assigned branch slide; SlideshowPanel threads selectedSequenceId through every handler.
Stable fragment keys	Fragment chips use index-based React keys (no duplicate-key collision / accidental multi-delete on duplicate times).
Shared island marker	setSlideshowManifest imports SLIDESHOW_ISLAND_TYPE / slideshowIslandRegex from core.

Testing

• core: 1931 tests pass · tsc --noEmit clean · build OK
• player: 267 tests pass · typecheck clean · tsup build OK (IIFE bundles core)
• studio: 1004 tests pass · tsc --noEmit clean · vite build OK
• lint/format: oxlint 0 warnings / 0 errors · oxfmt clean on all changed files

Added 9 tests: inverted range, fragment dedup, empty-sequence hotspot, non-array slideSequences, malformed slide, syncTo, fragment-at-end reveal, and branch-scoped note/fragment/hotspot editing.

Notes

• Pure additive fixup on top of ss-examples; the 5 underlying PRs are unchanged.
• onKey's "arrows work without a click" behavior is preserved; only Space/Backspace were tightened to focus scope.
• Branch sync (audience following a presenter into a hotspot branch) was a documented V1 cut in feat(player): slideshow controller + <hyperframes-slideshow> component #1581 — now implemented via syncTo.

🤖 Generated with Claude Code

[vanceingalls]

• fix(slideshow): address code-review findings #1580-1584 #1585 👈 (View in Graphite)
• 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
• 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]

Code Review — PR #1585

Reviewed the full diff (+925/−221 across 17 files). This is a dense, well-documented fixup PR. Findings organized by the five review axes requested.

---

1. Does it address the review findings from #1580–1584?

Yes — comprehensively. I can trace every claimed fix back to concrete code:

• Standalone build broken (tsup externalizing @hyperframes/core): fixed via noExternal: ["@hyperframes/core"] in tsup.config.ts. One-liner, correct.
• Audience mode never activating: resolveMode() now falls back to ?mode=audience from location.search. All five callsites that previously used getAttribute("mode") are updated.
• Audience mirroring only goToSlide(slideIndex): replaced with syncTo(sequenceId, slideIndex, fragmentIndex) on the controller and the BroadcastChannel handler. Tests updated to verify full-position mirroring including branches.
• Keyboard hijacking host page: Space/Backspace gated behind focused check; ArrowRight/ArrowLeft still work in ambient mode (body focus or unfocused). Tests verify both states.
• next() skipping trailing fragments: atEnd gate removed; test added for the specific "currentTime already at slide end" scenario.
• enterBranch soft-lock on empty sequence: guard added (seq.slides.length === 0), test renamed and rewritten to verify no-op behavior.
• extractScenes crash on null entries: cast-free, null-guarded type predicate. Clean.
• Strict manifest validation: isManifest now validates slideSequences as array and each element via isSlideRef/isSlideSequence. Tests for non-array slideSequences and malformed slides.
• Inverted range not flagged: endTime <= startTime error added to resolveSlide. Test covers it.
• Empty hotspot target not flagged: seq.slides.length === 0 error. Test covers it.
• Fragment dedup: new Set in resolveSlide. Test verifies [2,1,2,1,3] → [1,2,3].
• Lint parseTiming mismatch with runtime: now accepts data-end/data-hf-authored-end in addition to data-duration. Priority order mirrors what runtime timeline.ts does.
• Shared island constant: SLIDESHOW_ISLAND_TYPE + slideshowIslandRegex() exported from core, consumed by studio's setSlideshowManifest.ts. Drift-prone duplication eliminated.
• SlideList render order: now renders manifest.slides order first (via map + sceneById lookup), then non-slide scenes. Uses a Set for O(1) membership.
• Branch-slide authoring: mapSlidesIn helper threads sequenceId through setSlideNotes/addFragment/removeFragment/addHotspot/removeHotspot. BranchTree lets you select+edit branch slides. Panel threads selectedSequenceId everywhere. Tests cover branch-scoped editing including the "don't auto-add to branch" guard.
• Stable fragment keys: key={t} → key={\frag-${i}`}`. Prevents React duplicate-key issues on duplicate times (which can now exist briefly before dedup runs).

All 15 claimed fixes are present and tested (9 new tests total, matching the PR description).

---

2. Are the fixes correct and complete?

Overall very solid. A few observations:

(a) playTo render-nudge: Math.min(t + RENDER_NUDGE, slide.end) could clamp holdAt to exactly slide.end

When a fragment is at or near slide.end - RENDER_NUDGE, holdAt clamps to slide.end. The onTime handler fires when tt >= holdAt - EPS, so it fires at slide.end - EPS. Then it tries slide.fragments.indexOf(holdTarget) — if holdTarget is that near-end fragment, it should find it and advance fragmentIndex correctly. This seems fine in practice, but the intent documentation could note that holdAt !== holdTarget is the norm (it does, in the field comment — good).

(b) resolveMode() try/catch around location.search

The catch is defensive — location.search can throw in some restricted contexts (sandboxed iframes). Good practice. Returns null on failure, so it falls back to attribute-only mode. Correct.

(c) isSlideRef — the return false on the fragment/hotspot checks is at the end of the if block, not after it

Looking more carefully:

if (r["fragments"] !== undefined && !(Array.isArray(r["fragments"]) && r["fragments"].every(...)))
    return false;
if (r["hotspots"] !== undefined && !Array.isArray(r["hotspots"])) return false;
return true;

This is correct — the return false is the body of the if. The formatting puts it on the next line, but it's valid. fragments must be an array of numbers if present; hotspots must be an array if present (individual hotspot shape is not validated here, which is fine — runtime handles malformed hotspots gracefully).

(d) setSlideNotes with sequenceId — no-auto-add behavior

When sequenceId is defined and the scene isn't assigned to that branch, mapSlidesIn returns the slides unchanged (no auto-add). This is the correct behavior — you shouldn't be able to author notes on a slide that doesn't belong to the branch. The test explicitly verifies this. Good.

(e) Presenter layout rework

The presenter view now confines the player to the top 68% and puts a notes panel in the bottom 32%. The playerEl.style mutation in renderPresenter() is a side effect inside render — not ideal in a React-style world, but this is a custom element with manual DOM management, so it's consistent with the rest of the component. The updateElapsed() optimization (only updating the elapsed readout instead of full re-render every second) fixes the button-flicker bug described in the PR.

(f) Fullscreen support

New toggleFullscreen(), onFsChange listener, f/F keybinding (focus-gated), cleanup in disconnectedCallback. The audience view gets an fs-only nav cluster. Clean implementation. The fullscreenchange listener triggers a full render() to swap enter/exit glyphs — slightly heavy but acceptable given the infrequency of the event.

(g) waitForScenes — event-driven + poll fallback

Now listens for the scenes custom event from the player, with the poll as fallback. The done flag and cleanup in finish() prevent double-resolution. The player.removeEventListener("scenes", onScenes) cleanup is correct. One minor note: the scenes event listener is added with the default options (not { once: true }), but the finish guard makes this safe.

---

3. Any regressions introduced?

I don't see regressions. The changes are well-bounded:

• enterSlide behavior change (seek to first hold / midpoint instead of slide start) is a deliberate improvement, not a regression. Tests are updated to match.
• The atEnd gate removal in next() broadens behavior (reveals fragments even at end) — tested and intentional.
• resumeSlide now uses playTo() instead of seek()+pause() — this gives the render-nudge benefit consistently. Tests verify the new behavior.
• Keyboard changes narrow behavior (Space/Backspace require focus) — this is the fix, not a regression.
• The label element → div + button change in BranchItem preserves the checkbox behavior while adding click-to-select on assigned slides. The aria-label on the checkbox maintains accessibility.

---

4. Net line change analysis (+925/−221 = +704 net)

The +704 net breaks down as:

• ~219 lines: presenter-test.html (new example file — HTML + inline JS for presenter mode testing). This is test/demo infrastructure, not production complexity.
• ~200 lines: New tests (9 tests across 3 packages). Pure quality.
• ~150 lines: Fullscreen support (toggle, keybinding, SVG glyphs, audience fs-only cluster). New feature.
• ~80 lines: buildNavCluster refactor (extracted from render(), parameterized for reuse by both normal and presenter views). Net simplification — removes duplication.
• ~55 lines: Branch-slide authoring (mapSlidesIn, selectedSequenceId state, BranchItem selectability). New feature (promoted from backlog).

The remaining ~200 lines are the actual bug fixes, validation hardening, and the syncTo implementation. The deletions (-221) are genuine removals of dead/wrong code, not just moves.

Verdict: the +704 is justified. It's mostly tests, a new example, and two promoted features (fullscreen + branch authoring). The core bug fixes are compact.

---

5. Remaining issues not addressed

(a) Minor: ISLAND_RE in setSlideshowManifest.ts is now a module-level slideshowIslandRegex("gi") call

const ISLAND_RE = slideshowIslandRegex("gi");

Since slideshowIslandRegex creates a new RegExp(...), this is fine — it's evaluated once at module load. But RegExp with the g flag has stateful lastIndex. If ISLAND_RE is used with .exec() or .test() elsewhere, the shared lastIndex could cause intermittent match failures. In this file it's only used with String.replace(), which resets lastIndex per call, so it's safe here. But the export of slideshowIslandRegex is a factory (returns a new RegExp each call), so callers that need g should call the factory each time rather than caching. Worth a comment.

(b) Low priority: buildPresenterLayout uses string concatenation for HTML

This is the existing pattern throughout the component (it's a vanilla custom element, not React). Not a regression — just noting that the presenter panel HTML is getting complex enough that a template literal + data-attribute wiring approach (which is already used) is reaching its practical limit. Future consideration, not a blocker.

(c) The presenter-test.html duplicates the slideshow island from index.html

The file has a comment acknowledging this ("keep this copy in sync with the island in index.html"). This is a test harness, so the duplication is acceptable, but it's a maintenance footprint.

---

Summary

This is a thorough, well-tested fixup PR that addresses all 15 findings from the code review stack. The two showstopper bugs (broken standalone build, dead audience sync) are fixed cleanly. The promoted features (fullscreen, branch authoring) are well-scoped and tested. No regressions detected. The net +704 lines are justified by tests, examples, and new features — not accidental complexity.

I recommend approval pending a quick sanity check that the IIFE build actually bundles core (a grep for unresolved @hyperframes/core in the built output would confirm).

Review by Miga

[miguel-heygen]

LGTM

[james-russo-rames-d-jusso]

Reviewed at: 17e9ffa2707d3de74c611c5907474ce03a3c29e9 (HEAD)

Verdict: Comment — comprehensive fixup PR addressing the documented findings cleanly. Two flagged-claim/diff mismatches worth resolving; one carryover from #1583 that the docs PR should address but is reachable from here. Miga (review) verified the 15 fixes individually; I cross-checked the load-bearing ones and add a couple of cross-PR observations.

---

Verified against diff at HEAD

I traced four load-bearing claims to actual code:

1. tsup.config.ts noExternal — noExternal: ["@hyperframes/core"] present at packages/player/tsup.config.ts:7. Confirmed. Mirrors the shader-transitions precedent.
2. resolveMode() URL fallback — packages/player/src/slideshow/hyperframes-slideshow.ts:90-95 uses URLSearchParams(location.search).get("mode") as fallback to the attribute. All 5 callsites updated. Try/catch around the URL access is defensive (correct for sandboxed iframes).
3. syncTo() on controller — SlideshowController.ts:253 — handler at line 188 dispatches it on the BroadcastChannel state message; routes through resumeSlide() at line 266 which preserves the render-nudge path.
4. parseTiming matches runtime priority — slideshow.ts lint: data-duration → data-end → data-hf-authored-end. Cross-checked against packages/core/src/runtime/timeline.ts at lines 22-29 — same order. No drift.

PR body / diff mismatches (must fix or update body)

13. PR body claims "no example/skill files are touched" — the diff adds registry/examples/airbnb-deck/presenter-test.html (219 lines). Either drop it from this PR (move to #1584) or correct the PR body claim. Right now this PR also introduces an example file under registry/examples/, which contradicts the description and makes the "pure player/core/studio fix bundle" framing inaccurate. (Also: see item 17 — presenter-test.html adds a third duplicated island for airbnb-deck.)

14. Trailing fragment reveal — "tightens midpoint seek" claim is broader than tested. next()'s atEnd gate removed (verified SlideshowController.ts); test at SlideshowController.test.ts exercises "currentTime at slide end with one remaining fragment." But the new resumeSlide path (which back(), backToMain(), syncTo() all use) seeks to midpoint of [holdAt, slide.end] if no remaining fragments — that's a different behavioral change from the documented next() fix. Worth either: (a) a test naming the resume-midpoint heuristic explicitly, or (b) a note in the PR body that resume-position semantics changed too. The resume-to-midpoint is a defensible choice (avoids the "land at frame 0 with nothing visible" state) but it isn't called out.

Carryover risk

15. #1583 doc-vs-code drift still open (confirming Miga + my #1583 finding 1-2 + 6). This PR doesn't touch skills/slideshow/SKILL.md, so the two factual errors (.clip[id] claim, "strictly inside" fragments) carry over with the docs PR. If #1585 lands before #1583 is updated, the published docs will be wrong against current behavior. Recommend: either (a) #1583 fixes the doc copy and merges first, or (b) add a small doc patch to this PR. The carryover is invisible from this PR's diff but very real for consumers.

Concerns

16. SLIDESHOW_ISLAND_TYPE + factory pattern usage in setSlideshowManifest (confirming Miga 5a). The module-level const ISLAND_RE = slideshowIslandRegex("gi"); is only used with .replace() (line 61), which resets lastIndex per call — safe here. But:

• Other future callers may use .exec() or .test() and import the same ISLAND_RE. The factory pattern is the right shape; a JSDoc on slideshowIslandRegex like "Call per use site if you need a non-shared lastIndex. Safe to cache for .replace()." would prevent the next person from creating a regression.
• Worth a follow-up: lift ISLAND_RE into core too (next to slideshowIslandRegex) so all consumers share the cached .replace()-safe instance, rather than each module rolling its own.

17. presenter-test.html duplicates the slideshow island a third time. airbnb-deck now has 3 places carrying the same island: index.html, demo.html, presenter-test.html. Comment acknowledges sync risk ("keep in sync with index.html") — but a structured pattern (separate _island.json file inlined via build, or generated via a script) would be cheaper than three manual sync touchpoints. Not blocking; tracking-issue worthy.

18. fullscreenchange listener triggers full render() — hyperframes-slideshow.ts reacts to every fs change by re-rendering the whole chrome to swap one SVG glyph. Acceptable given infrequency (Miga noted) but a slightly cheaper path is chrome.querySelector('.fs-button').innerHTML = newSvg on the change. Not blocking.

Net line change verification

Miga's breakdown (+925/-221 = +704 net, broken down as ~219 presenter-test + ~200 tests + ~150 fullscreen + ~80 refactor + ~55 branch authoring + ~200 core fixes) checks out. The deletions are real (not just moves) — e.g., the goToSlide BroadcastChannel handler is fully removed, replaced by syncTo.

One nit on the +704: per item 13, the +219 from presenter-test.html is genuinely a 4th example PR scope-creep into a fix PR. If presenter-test.html is required for testing fullscreen + presenter sync, it could live as a one-time test artifact (gitignored, or under packages/player/test-fixtures/). Shipping it in registry/examples/airbnb-deck/ makes it look like a consumer-facing example.

Risk profile

• Branching reversal not present — verified that no PR in #1580-#1584 was reverted; this is pure additive on top of ss-examples.
• Tests added (9) all map to fixes — verified via spot-check on parseSlideshow.test.ts, SlideshowController.test.ts, SlideshowPanel.test.ts.
• No console.log / debug leftovers found in changed code.
• Cross-PR branching: same monolith-stack concern as #1583/#1584. If team picks the split (#1589-#1592), this PR rebases too.

What's good

• Bug ordering: standalone-build-broken + audience-sync-dead were the two showstoppers and both are fixed cleanly with minimal blast radius.
• isManifest strict validation closes the lint-crash hole (parseable-but-malformed islands threw out of resolveSlideshow and crashed hyperframes lint — verified in the test fixture).
• Branch-slide authoring (promoted from V1 cut in #1581) is well-scoped: mapSlidesIn is small, threading sequenceId through 5 ops keeps the API symmetric, and the no-auto-add behavior is correct + tested.
• keyboard scoping (Space/Backspace require focus, arrows still work unfocused) is the right behavioral split.
• Strict array/object validation on slideSequences will catch the "parseable-but-malformed" island case before runtime.

Item 13 (PR body / diff mismatch + scope creep) and 15 (cross-PR docs drift) are the items I'd want resolved before stamp. 14, 16-18 are concerns/tightening. Recommend after IIFE-bundles-core sanity grep (Miga's closing ask) + a one-line docs sync with #1583.

Review by Rames D Jusso

The base branch was changed.

[vanceingalls]

Review feedback addressed (8b6f09e)

• CodeQL fix(lint): rephrase too-large composition warnings to give actionable reasoning #638 (parseSlideshow.ts): completed the regex metacharacter escape in slideshowIslandRegex (was missing \); added JSDoc on the factory + the lastIndex caveat (Miga 5a / Rames 16).
• CodeQL fix(runtime): clear play guard after hard seek to prevent audio desync on scrub #639 + fix: render shader transitions for SDR compositions #640 and Rames items 13 / 17: removed registry/examples/airbnb-deck/presenter-test.html. It was a generated test harness (postMessage without origin check, prototype-pollution sink) that scope-crept into this fix PR and was a 3rd duplicated island. It's regenerated locally from a scratchpad script when testing presenter mode — not consumer-facing, so it no longer ships. This also restores the "no example files touched" framing.
• Rames item 15 (docs drift in skills/slideshow/SKILL.md): corrected two factual errors against current behavior — the lint rule resolves scenes by data-composition-id only (not .clip[id]), and fragment times are valid inclusive of [start, end], not "strictly inside". (This is the deliberate small doc patch Rames offered as option (b); it's the one intentional skill-file change here.)

Closing sanity check (Miga + Rames): confirmed the IIFE build bundles core — dist/slideshow/hyperframes-slideshow.global.js has 0 unresolved @hyperframes/core references and the core symbols are inlined.

Non-blocking items left as follow-ups: #14 (resume-position test/note), #16 (lift a shared .replace()-safe ISLAND_RE into core), #18 (targeted glyph swap instead of full re-render on fullscreenchange).

[james-russo-rames-d-jusso]

Reviewed at: 87e0f737baa0287aae9df11c1aa473f31f1d825f (HEAD)

Verdict: Comment — R1 items 13/15/16/17 are cleanly resolved in 8b6f09e; the CSP-safe rewrite in 0d208db is mostly clean but introduces a small UX regression. Two new findings from the post-R1 scope addition (present CLI command + SFX commits) worth surfacing.

---

R1 verification at HEAD

R1 item	Verdict	Evidence
13 — presenter-test.html scope creep	✅ resolved	File deleted in 8b6f09e (commit message cites item 13/17 + CodeQL #639/#640). Regenerate-locally pattern is the right call.
14 — resumeSlide midpoint seek undocumented	🛑 still open (minor)	SlideshowController.ts:114 retains the midpoint heuristic comment but no PR body note, no dedicated test. Defer to a follow-up; not blocking.
15 — SKILL.md docs drift	✅ resolved	SKILL.md:90 now reads "resolves scenes by data-composition-id" (no more .clip[id]). SKILL.md:154 now reads "fall within [start, end] (inclusive of both bounds)" with explicit time < start / time > end boundary. Both factual errors fixed.
16 — slideshowIslandRegex factory JSDoc	✅ resolved	parseSlideshow.ts:12-18 has a clear JSDoc explaining the lastIndex caveat + factory rationale. CodeQL #638 metachar-escape also completed (`[.*+?^${}()
17 — 3-duplicate island in airbnb-deck	✅ resolved (presenter-test removed)	Back to 2 copies (index.html + demo.html). The 3rd duplicate is gone with presenter-test.html. Tracking-issue for a build-time inline of the island is a separate concern.
18 — fullscreenchange triggers full render()	🟡 unchanged	hyperframes-slideshow.ts:528-531 still does this.render() on every fs change. Minor; not addressed. Defer.

---

New findings from post-R1 scope

19. CSP-safe hover regresses muted-button visual feedback (hyperframes-slideshow.ts:65-68) — the new CSS rule [data-hf-nav-cluster] button:hover { color: #fff !important; } overrides the muted-state color rgba(255,255,255,0.45) on hover. The previous inline onmouseover preserved muted state explicitly ('${this._muted ? "rgba(255,255,255,0.6)" : "#fff"}'). Result: hovering the speaker button erases mute-state affordance. Suggest a [data-hf-muted] [data-hf-mute]:hover { color: rgba(255,255,255,0.6) !important } override, or wire mute state via a data-attribute the hover rule can branch on. Not blocking.

20. present command's buildPresentPage reintroduces the two CodeQL classes the team just removed from presenter-test.html (packages/cli/src/commands/present.ts:230-287). Same shape:

• Prototype pollution — clips[d.name] with d.name from postMessage (line 279). d.name === "__proto__" returns Object.prototype, truthy, falls through to el.currentTime = 0 which mutates the prototype. Fix: Object.prototype.hasOwnProperty.call(clips, d.name) guard, or build clips with Object.create(null).
• postMessage without origin check (line 276). Server is localhost, but CodeQL will still flag this when the same workflow runs on the CLI package. Cheap fix: if (e.origin !== location.origin) return; at the top of the handler.

Both were the exact reasons presenter-test.html was deleted in 8b6f09e; the patterns moved into the CLI command unchanged.

21. assetContentType has the same prototype-access bug (packages/cli/src/utils/compositionServer.ts:74-77). ASSET_CONTENT_TYPES[ext] with ext === "__proto__" returns Object.prototype (truthy), so the ?? "application/octet-stream" fallback doesn't fire. Not exploitable in practice (constructing such a filename is hard) but the pattern is bad. Object.hasOwn(ASSET_CONTENT_TYPES, ext) ? ... : "application/octet-stream".

22. .prettierignore blanket-excludes registry/examples/**/*.html — the justification ("generated video-pipeline output, large + risky to reformat") makes sense for the composition body, but the slideshow island JSON inside airbnb-deck/index.html is hand-authored content that the team will edit. After this PR lands, manual island edits won't be auto-formatted, and the demos will silently drift in style. Minor; not blocking. Could be tightened later (e.g. limit to specific files, or extract the island to a sibling JSON file the build inlines).

---

Peer review status

• Miga (review) approved at the prior SHA 17e9ffa2, traced all 15 fixes individually. Her closing ask ("sanity check the IIFE bundles core") is addressed by 8b6f09e's commit message: "IIFE bundles core confirmed (0 external @hyperframes/core refs in the slideshow global build)." Layering R2 on top of her R1, not replacing it.
• github-advanced-security[bot] flagged CodeQL #638/#639/#640 at SHA 0d208db — all three are addressed by the subsequent commits (8b6f09e for #638 + #639/#640 via presenter-test.html removal). Items 20-21 above flag the same CodeQL classes re-emerging in the new present command.

What's good (post-R1)

• Author addressed R1 items 13/15/16/17 directly, with commit-message line-by-line mapping to the review items. Clean discipline.
• The play ↔ present refactor into utils/compositionServer.ts is a real extraction (shared resolveRuntimePath / resolvePlayerPath / listenOnFreePort / assetContentType / injectRuntime), not a copy. play.ts shrunk -82 / +13 lines.
• 0d208db adds the manifest version field cleanly (SLIDESHOW_MANIFEST_VERSION = 1, schema field is optional with "treat as 1 if absent" semantics) — well-set-up for future migration.
• UUID migration (crypto.randomUUID() in SlideshowPanel.tsx:319 + SlideshowSubPanels.tsx:428) is correct.
• Sibling stack drop risk: none — all merged sibling PRs (#1580-#1584, #1589-#1592) confirmed in main; this PR's commit graph is pure additive on top.

---

Items 19-22 are minor; item 14/18 unchanged from R1. Recommend: address item 20 (CodeQL classes in buildPresentPage) before stamp since the team just removed the same patterns; items 19/21/22 are tightening; items 14/18 can defer.

R2 review by Rames D Jusso

[james-russo-rames-d-jusso]

Reviewed at: e1e08f7410b256cc04339b6c4f4eee6a55244693 (HEAD)

Verdict: LGTM — every R2-NEW item resolved at the canonical shape; R1 carry-overs (14/18) also shipped with named-heuristic docs + tests. Stamp hold lifted on my end.

Per-item resolution

#	Severity (R2)	Status at R3	Evidence
14	🛑	✅ resolved	SlideshowController.ts:131-151 resumeSlide mirrors enterSlide via a shared restFrame(slide) = start + (end-start)*0.5 helper; midpoint heuristic named in code comment + dedicated test at SlideshowController.test.ts:610-623 ("Slide c has no fragments, so resumeSlide lands at its midpoint (restFrame)"). Branch tree (saved-fragment / pre-first / no-fragments) doc'd explicitly.
18	🟡	✅ resolved	hyperframes-slideshow.ts:536-545 onFsChange now swaps only the fullscreen glyph (innerHTML) + aria-label/aria-pressed on the [data-hf-fullscreen] button. SVGs hoisted to module consts ENTER_FS_SVG / EXIT_FS_SVG (lines 82-83) so the swap is allocation-free. No full chrome re-render.
19	⚠️	✅ resolved	hyperframes-slideshow.ts:73-75 higher-specificity rule [data-hf-muted] [data-hf-mute]:hover { color: rgba(255,255,255,0.6) !important; } preserves the muted affordance on hover. Beats the [data-hf-nav-cluster] button:hover { color: #fff } rule above by specificity (attr+attr vs single attr).
20	🛑 (gate)	✅ resolved	present.ts:276-292 BOTH canonical shapes shipped: if (e.origin !== location.origin) return; (line 278) AND if (!Object.prototype.hasOwnProperty.call(clips, d.name)) return; (line 283). Origin gate is same-origin (composition iframe is same-origin per the comment) — equivalent to the allowlist shape I outlined. Proto-pollution closed.
21	⚠️	✅ resolved	compositionServer.ts:74-79 assetContentType uses Object.hasOwn(ASSET_CONTENT_TYPES, ext) ? ASSET_CONTENT_TYPES[ext] : undefined with ?? "application/octet-stream" fallback. Same class as #20, same shape.
22	🟡	✅ resolved	.prettierignore replaces the blanket registry/examples/**/*.html glob with an explicit 6-file list of generated demo compositions (airbnb-deck/{index,demo}.html, startup-pitch/{index,demo}.html, slideshow-demo/index.html, warm-grain/compositions/captions.html). Hand-authored example HTML formats again. Comment names the intent.

Other sweeps

• CodeQL: gh api .../code-scanning/alerts?ref=refs/pull/1585/head&state=open → []. No open alerts on PR head; the github-advanced-security review at 08:43Z was the post-fix workflow result (no inline findings). #638/#639/#640 stay closed.
• Cumulative diff sweep (git diff origin/main...HEAD): no silent revert of sibling/merged work; the R2-fix push also re-removed presenter-test.html (a git add -A from commit 54a4460 had revived the harness and with it CodeQL #639/#640 — caught + remedied in the same push).
• Peer reviews since R2 (4531964672): none.
• No new findings introduced by the R2-fix push.

Items deferred / not in scope

None outstanding from R1/R2 — every numbered item now ✅. Ship-ready from my chair.

R3 review by Rames D Jusso

[jrusso1020]

LGTM per Rames D Jusso's R3 routing. Verified the canonical-shape fixes against the diff at HEAD e1e08f7410:

• R2 #20 (postMessage proto-pollution + missing origin check): present.ts:516 has if (e.origin !== location.origin) return; as the first thing in the handler, and present.ts:521 has if (!Object.prototype.hasOwnProperty.call(clips, d.name)) return; for the proto-pollution guard. Both canonical defenses applied at the exact right call sites; CodeQL would re-flag a hand-rolled clips[d.name] here, this shape is the one CodeQL accepts.
• R2 #21 (assetContentType same proto-access bug): compositionServer.ts:630 uses Object.hasOwn(ASSET_CONTENT_TYPES, ext) (modern static-method form, equivalent to the prior Object.prototype.hasOwnProperty.call for security but cleaner) with the octet-stream fallback preserved at :631.
• R2 #19 (CSS hover regression on mute affordance): hyperframes-slideshow.ts:73-75 higher-specificity selector [data-hf-muted] [data-hf-mute]:hover resolves the inherited-color leakage.
• R2 #22 (.prettierignore over-broad glob): explicit 6-file list now, no blanket glob.
• R1 #14 (resumeSlide midpoint-seek undocumented): SlideshowController.ts:131-151 shares the restFrame calculation with enterSlide so the seek logic isn't separately-derived. Midpoint-doc + dedicated test at :610-623 is the right shape.
• R1 #18 (fullscreenchange re-render): glyph + aria swap only at hyperframes-slideshow.ts:536-545, SVGs hoisted to module consts (minor cleanup but worth doing).

Bonus catch worth flagging publicly: RDJ noted the R2-fix push also re-removed presenter-test.html that a git add -A in 54a4460 had revived. That kind of add -A re-add of a deleted file is a perennial Graphite-stack gotcha — worth a one-line note in the stack's commit conventions to use git add <path> over git add -A when you've intentionally deleted files. Not blocking.

CI status: 36 checks pass, 0 fails. CodeQL open alerts on PR head = [] (per RDJ). Stamp matches the routing.

Stamp by Rames Jusso (per Rames D Jusso's R3 substance verification at HEAD e1e08f7410).

[miga-heygen]

Re-review — PR #1585 (post-R2/R3)

Reviewed at: e1e08f7410b256cc04339b6c4f4eee6a55244693 (HEAD)

Traced the four R2 findings (#19–#22) and the two R1 carry-overs (#14, #18) to the current diff. All resolved.

---

R2 CodeQL findings — both fixed

#20 (buildPresentPage: proto-pollution + missing origin check) — Fixed.
present.ts SFX message handler now has both canonical defenses:

• Origin gate: if (e.origin !== location.origin) return; — first line of the handler, before any data access.
• Proto-pollution guard: if (!Object.prototype.hasOwnProperty.call(clips, d.name)) return; — blocks __proto__ / constructor / toString from resolving to inherited properties on the clips object.

Both are the exact shapes CodeQL accepts. The patterns match the ones that were removed with presenter-test.html in 8b6f09e — the re-emergence in the CLI command is now closed.

#21 (assetContentType proto-access bug) — Fixed.
compositionServer.ts:74-79 uses Object.hasOwn(ASSET_CONTENT_TYPES, ext) — the modern static-method form. Functionally equivalent to Object.prototype.hasOwnProperty.call() for security purposes; an ext of "__proto__" now correctly falls through to the "application/octet-stream" default.

Other R2 items — all resolved

#	Finding	Status
19	CSS hover overrides muted-button color	Fixed — [data-hf-muted] [data-hf-mute]:hover { color: rgba(255,255,255,0.6) !important; } at higher specificity (attr+attr vs single attr) preserves the muted affordance on hover.
22	.prettierignore blanket glob	Fixed — explicit 6-file list of generated demo compositions; hand-authored example HTML is no longer excluded.

R1 carry-overs — also shipped

#	Finding	Status
14	resumeSlide midpoint-seek undocumented	Fixed — restFrame(slide) helper shared between enterSlide and resumeSlide; named in code comments; dedicated test at SlideshowController.test.ts:610-623.
18	fullscreenchange triggers full render()	Fixed — onFsChange now swaps only the SVG glyph + aria-label/aria-pressed on [data-hf-fullscreen]. SVGs hoisted to module consts (ENTER_FS_SVG / EXIT_FS_SVG).

Verdict

No open findings remain from R1, R2, or R3. Rames's R3 LGTM + jrusso1020's stamp are consistent with the diff at HEAD. Ship-ready.

Re-review by Miga