fix(studio): resolve recent timeline regressions

[miguel-heygen]

Summary

• stop right-click keyframe pointer/context-menu paths from seeking before the menu opens
• serialize keyframe ease once when editing an already-eased keyframe
• let SnapToolbar ignore keyboard events another handler already claimed
• render audio clips as waveforms before generic composition-preview thumbnails

Fixes #1800
Fixes #1801
Fixes #1806
Fixes #1807

Note: #1808 is an enhancement request for a keyframe-recording opt-out and is intentionally left open.

Verification

• bun run --filter @hyperframes/parsers test -- src/gsapParser.test.ts
• bun run --filter @hyperframes/parsers typecheck
• bun run --filter @hyperframes/studio test -- src/player/components/TimelineClipDiamonds.test.tsx src/player/components/Timeline.test.ts src/components/editor/SnapToolbar.test.tsx src/hooks/useRenderClipContent.test.ts
• bun run --filter @hyperframes/studio typecheck
• agent-browser harness: all 4 browser checks PASS, no browser errors

[james-russo-rames-d-jusso]

Reviewed at 80a8fa3cc13fe1ce41d873b402d23d8f25caf124.

Overall: solid multi-issue root fix. Each of #1800/#1801/#1807 lands at the correct emit/dispatch layer; #1806's mechanism holds for the reported repro but leaves a small asymmetry outside it. #1808 deferral is legitimate. Reviewed at commit above; CI is green on the completed checks (Test/Typecheck/Windows/CodeQL JS still pending at post time; mergeStateStatus=BLOCKED is REVIEW_REQUIRED, not a check failure).

Per-issue verdict

Issue	Verdict	Notes
#1800 right-click → seek → menu no-op	✅ genuinely fixed at root	TimelineClipDiamonds.tsx:181 if (e.button !== 0) return; in the click-branch is exactly the issue's suggested fix. Timeline.tsx:487-489 seek-removal in onContextMenuKeyframe is complementary defense. TimelineClipDiamonds.test.tsx correctly pins the pointerup path; Timeline.test.ts pins the contextmenu path. Both would fail without the fixes.
#1801 duplicate ease on add-keyframe merge	✅ genuinely fixed at root	buildKeyframeValueNode is the single emit chokepoint — filtering ease from properties and coalescing with the explicit ease arg fixes it for all 5 callers (addKeyframeToScript merge + non-merge, _auto backfill emit, array-form keyframe replace, and updateKeyframeInScript). Regression test asserts match-count = 1 in the emitted source, which fails without the filter.
#1806 bare s = Split + snap-toggle	⚠️ partial (see 🟠 below)	Mechanic is sound where Split preconditions ARE met; not sound where they aren't.
#1807 audio waveform never mounts	✅ genuinely fixed at root	useRenderClipContent.ts:113-118 hoists the tag==="audio" branch above the generic activePreviewUrl branch — exactly the issue's proposed fix. Sub-comp branch (compSrc) still runs first, so no interaction there.
#1808 opt-out for gesture→keyframe recording	🟢 legitimately deferred	Genuinely an enhancement (product decision, not a defect). The 4 fixes don't incidentally address it and don't introduce a new instance. The #1801 comment thread and #1808 body both come from the same user asking for the same toggle — worth acknowledging in the issue reply that fixing #1801's serializer bug closes the symptom they mostly cared about, while #1808 remains a design ask.

🟢 Strengths

• buildKeyframeValueNode fix is placed at the true root — emit layer — not at the caller that happened to trigger it. All 5 callsites benefit; no risk of the twin-ease pattern reappearing in a sibling flow.
• Regression tests are defensive, not happy-path: each one would fail cleanly if the corresponding fix were reverted (ease match-count = 1, onSeek not-called, onClickKeyframe not-called on right-button, snap onSnapChange not-called when defaultPrevented, content.type === AudioWaveform).
• Timeline.tsx seek-removal in onContextMenuKeyframe is a nice belt-and-suspenders: even if a future pointer-event refactor bypasses the button guard, the menu path can't silently seek. Selection still happens (setSelectedElementId / onSelectElement) so the menu still lands on the right target.
• Handler mechanics for #1806 are correct: useAppHotkeys attaches on window in capture phase, SnapToolbar on document in bubble phase — capture always runs first, so defaultPrevented is observable by SnapToolbar. Confirmed at useAppHotkeys.ts:461 (, true) vs SnapToolbar.tsx:82 (default bubble).

🟠 Should-fix

• #1806 — Split's preventDefault is conditional; SnapToolbar's guard passes when Split didn't preventDefault. useAppHotkeys.ts:230-249 (dispatchPlainKey, s-key branch) only calls event.preventDefault() inside the two branches where Split actually does something (splittable element + playhead inside clip range, or sub-comp #-qualified selection hint). If the user presses s with no clip selected OR playhead outside the selected clip's range OR selected element is non-splittable (canSplitElement(el) false), Split silently no-ops and SnapToolbar's defaultPrevented check evaluates false, so snap still toggles. Net effect: bare s = "snap toggle when no split is possible, otherwise split only". The issue's repro passes cleanly (selected clip + playhead inside → split-only, snap does not toggle), but users pressing s while browsing without a selected clip still see snap flip. Two options: (a) always preventDefault in the s branch of dispatchPlainKey (with a comment explaining the "reserve s for split even when it no-ops" contract); or (b) surface via a shared "claimed keys" registry rather than defaultPrevented-piggybacking. (a) is a one-liner. Worth resolving before landing so the fix matches the issue's title ("bare 's' triggers both") rather than the issue's specific repro. Also note: the new if (e.defaultPrevented) return; guard sits above the s AND g branches — any capture-phase handler that preventDefault's any key (existing or future) will silently suppress both toggles. Not load-bearing today, but a wider suppression than the fix technically needs; a if (e.defaultPrevented && (e.key === "s" || e.key === "g")) return; would tighten it.

🟡 Questions

• Existing-state cleanup for #1801. Per issue body, sources dirtied with duplicate ease before this PR persist untouched. Reader dedupes at runtime so nothing breaks, but hyperframes lint (per issue) doesn't flag it either, so those files stay quietly dirty. Is a follow-up planned to (a) add a lint rule, or (b) opportunistically normalize on next write? If neither, the "silent dirtying of design sources" concern in the issue body remains for pre-existing projects; PR body could note this as known-residual so it doesn't get forgotten.
• Sub-comp + audio interaction for #1807. The audio branch now sits above activePreviewUrl but still below the compSrc branch, which is correct in theory. Is there a test or repro shape where an audio clip carries compositionSrc? None in the diff — happy with the ordering, just checking whether that pairing is possible/expected.
• agent-browser harness. PR body cites 4/4 browser checks passing. Is that harness in CI, or run only locally? If local-only, it doesn't guard against future regressions in the way the unit tests do; if that's fine given the unit coverage is strong, no action — just want to confirm.
• #1808 in the release notes. Since both #1801's Chinese comment thread and #1808 are the same underlying user ask, it might be worth linking them explicitly in the changelog / release note so users tracking one know the other is deferred (and know the duplicate-ease symptom is gone regardless of the opt-out decision).

🛑 Blockers
None. #1806's asymmetry is a should-fix, not a blocker — the reported repro is resolved.

Multi-issue cross-cutting

• Four fixes touch four independent surfaces (parser emit, timeline pointer routing, keyboard handler coordination, clip-content dispatch). No shared root, no interaction risk between the fixes. Bundling into one PR is fine here because each is small; if they were larger I'd prefer four separate PRs for revert-clarity, but at this size the bundle reads cleanly.
• No sibling-asymmetry risk: buildKeyframeValueNode root fix propagates to all keyframe callers uniformly; the TimelineClipDiamonds pointer guard mirrors the existing pointerdown button guard right above it (symmetric).
• Existing tainted state exists only for #1801 (see 🟡 above); the other three are transient runtime state that resolves on next interaction.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 @ 9b311588 — verifying against my R1 at 80a8fa3c.

Verdict: ✅ #1806 resolved cleanly. Carry-over strengths (#1800, #1801, #1807) intact at HEAD. No new findings. Ship it.

---

#1806 — bare s key double-fire (R1: 🟠 partial → R2: ✅ resolved, PREEMPT)

R2 shape at useAppHotkeys.ts:230-233:

if (event.key === "s" && !event.altKey) {
  event.preventDefault();  // ← top-level, before any branch
  const { selectedElementId, elements, currentTime } = usePlayerStore.getState();
  ...
}

Classification: PREEMPT, not NARROW/MOVE. preventDefault() fires unconditionally at the top of the s branch, before usePlayerStore.getState() and before any selection / playhead / splittable check. The two prior preventDefault() calls inside the "actually split" and "sub-comp nudge" branches (previously my R1 concern) are now redundant and correctly removed at :243 and :249 in the diff — belt-and-suspenders would have been the NARROW shape; the R2 collapse to a single top-level call is the correct PREEMPT.

Belt-and-suspenders on the receiver side: SnapToolbar.tsx:68 also adds if (e.defaultPrevented) return; — even if a future refactor changed handler registration order, snap wouldn't misfire. Defense in depth.

Semantics check (per feedback_asymmetric_boolean_handler — did the fix silently swallow a valid intent?): Issue #1806 explicitly asks for "guard the handlers so Split doesn't also toggle snap." The fix takes that path — bare s is now Split-exclusive under Studio. The bare-s→snap-toggle pathway is intentionally removed by issue design. SnapToolbar test 1 ("toggles snap on an unclaimed S keypress") confirms the SnapToolbar listener still fires on non-preempted events, preserving the pathway for contexts where SnapToolbar mounts without useAppHotkeys.

Adjacent-key parity: f (:222), b (:259), v (:264), Escape all use unconditional top-level preventDefault — s now matches sibling shape.

Regression test at SnapToolbar.test.tsx:104-116: mounts both useAppHotkeys + SnapToolbar via renderToolbarWithAppHotkeys, dispatches bare s with default store state (no selectedElementId), asserts onSnapChange NOT called. Would correctly fail without the top-level preventDefault — the pre-R2 code returned from the s block without ever calling preventDefault on the no-selection path, so SnapToolbar's listener would have observed defaultPrevented=false and toggled snap. Test bug-pins the fix. Test 2 (pre-prevented event) additionally guards SnapToolbar's own defaultPrevented early-return.

Carry-overs from R1 — all intact at HEAD

• #1800 (right-click seek): TimelineClipDiamonds.tsx:181 onPointerUp still gates e.button !== 0 before treating as click; Timeline.tsx no longer seeks on primary-button contextmenu path. ✅
• #1801 (duplicate ease in parsed keyframes): gsapParser.ts single-chokepoint dedupe via the builtin var keys handling at :582 / property extraction at :672-707. ✅
• #1807 (audio waveform mount): useRenderClipContent.ts:113-118 el.tag === "audio" branch resolves BEFORE the generic activePreviewUrl thumbnail branch — waveform wins over drilled-composition frame. ✅

CI + merge state

• mergeable: MERGEABLE, mergeStateStatus: BLOCKED (missing required approval — expected at COMMENT lane).
• isDraft: false.
• CI: majority green; Windows render + Test + Typecheck + Build + Studio: load smoke + Preview parity in progress at time of review; CodeQL javascript-typescript in progress. No failures.
• No peer reviews since my R1 at 80a8fa3c.

Review by Rames D Jusso