fix(studio): player UX — keyboard timeline, honest waveform, keyframe menu actions - #1967
Conversation
7702c3f to
f64a854
Compare
4937e31 to
1af29ed
Compare
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
PR Review — fix(studio): player UX — keyboard timeline, honest waveform, keyframe menu actions
VERDICT: Approve (comment-only per process)
Summary
Strong PR that addresses three interconnected gaps across 17 files: (1) dead features that were fully wired through Timeline.tsx props but never rendered (Change Ease, Copy Properties), (2) a dishonest waveform fallback that fabricated plausible-looking peaks on decode failure, and (3) keyboard inaccessibility of timeline clips, keyframe diamonds, and several menus. The changes are well-scoped for a PR 6/7 in a stack and each fix is independently motivated.
What works well
- Honest waveform is the headline improvement. The old
fakePeaksfunction generated sine-wave-modulated data with a seeded PRNG that looked convincingly like a real waveform — exactly the kind of thing a user would trim or beat-align against. Replacing it with a dashed flat line + "waveform unavailable" text, cached viadecodeFailedSet, is the right call. The refetch-loop prevention via the Set +decodeErrorstate pair is clean. - Dead feature resurrection done right.
onChangeEaseandonCopyPropertieswere declared in the props interface, threaded throughTimeline.tsx, but never destructured or rendered. The new context menu properly wires them with current-ease checkmark display, collapsible ease list, and async copy with inline status feedback. menuKeyboardNav.ts— compact shared utility following APG menu patterns (arrow keys, Home/End, focus-on-mount, focus-restore-on-unmount). Correctly scopes its listener to the menu container so it can't conflict with the seek bar's Home/End handler. Good that it filters for:not(:disabled)items.event.detail === 0guard on keyframe diamondonClickis the textbook technique for keyboard-only activation on elements that also haveonPointerUphandlers. Prevents the double-fire correctly.- Beat dot delete migration from double-click to ⌥-click is a genuine UX safety fix — a stuttered drag attempt on a 12px target (now 24px, also good) absolutely could register as a double-click.
Findings
Nit — aria-valuenow removal (PlayerControls.tsx):
The hardcoded aria-valuenow={0} is removed, leaving useSeekBarDrag.ts:145 as the sole writer via setAttribute. This is correct since the dynamic value overwrites the static one on every seek. One subtlety: before the first seek fires, there's no aria-valuenow on the element at all. ARIA spec says aria-valuenow is required for role="slider", and the initial state is conceptually "0". In practice screen readers handle the omission gracefully since aria-valuemin={0} implies the starting value, so this is fine — just noting the spec-letter gap.
Nit — onCopyProperties type broadening (KeyframeDiamondContextMenu.tsx):
The prop type changed from (elementId, percentage) => void to => Promise<boolean> | boolean | void. The Timeline.tsx callsite returns copyTextToClipboard(...) which is Promise<boolean>, so the union is well-motivated. The result === false check in handleCopyProperties correctly distinguishes "returned false" from "returned void/undefined" (void falls through to the success path). This is the right behavior: callers that don't return anything are assumed to succeed.
Nit — ShortcutsPanel now uses useContextMenuDismiss for its panel (ShortcutsPanel.tsx):
Nice consolidation. Worth noting that useContextMenuDismiss adds both mousedown and keydown listeners on document, while the old code only had mousedown. The new behavior adds Escape-to-close, which the PR description calls out. Focus-in on open (panelBodyRef.current?.focus()) and focus-restore on close (via the cleanup return) are correct and make the panel keyboard-navigable. The triggerRef on the button ensures focus returns to the trigger, not the panel.
Observation — maxHeight on keyframe context menu:
The ease list can get tall (20+ GSAP ease options). The max-h-40 on the inner scroll container plus overflow-y-auto + maxHeight: calc(100vh - ...) on the outer menu is a double-scroll prevention pattern. Confirmed: the outer maxHeight accommodates the non-ease items, and the inner max-h-40 (160px) keeps the ease sub-list scrollable independently. This is correct — the menu itself won't overflow the viewport.
Observation — motion-reduce:animate-none additions:
Applied to both AudioWaveform and VideoThumbnail shimmer states. Good accessibility practice.
Ponytail lens
This PR is the kind of work that separates a real product from a demo. Fabricating waveform data when decode fails is the sort of "works in the demo" shortcut that causes real user harm downstream — someone trims audio to a beat that doesn't exist, publishes, and gets a jarring cut. The explicit degraded state ("waveform unavailable" with a dashed line) respects the user's intelligence. The dead features being wired-but-never-rendered suggests a rushed PR boundary in the original work — this PR cleans that up without fanfare. The keyboard accessibility additions are thorough without being over-engineered: menuKeyboardNav.ts is 47 lines, covers the APG essentials, and is already shared across three menus. No complaints.
Diff stats
+464 / -141 across 17 files (1 new: menuKeyboardNav.ts)
CI: all required checks pass (preflight, preview parity, player-perf, regression).
Review by Miga
🤖 Generated with Claude Code
f64a854 to
97e5054
Compare
1af29ed to
81d53b6
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack review for PR 6/7.
Audited: packages/studio/src/player/components/AudioWaveform.tsx, menuKeyboardNav.ts, KeyframeDiamondContextMenu.tsx, Player.tsx, PlayerControls.tsx, TimelineClipDiamonds.tsx, and the related menu/timeline wiring at head 81d53b6.
The waveform change is the right product call: decode failures now render an explicit unavailable state and cache failure by URL instead of fabricating plausible peaks. The menu keyboard helper is compact, scoped to menu containers, filters disabled items, and restores focus. The keyframe menu re-surfaces previously threaded-but-unrendered actions without broadening the timeline mutation surface more than necessary.
Miga’s ARIA nit on the initial seekbar aria-valuenow is valid but not blocking for this delta.
Verdict: APPROVE
Reasoning: The audited player changes remove a misleading waveform fallback and complete menu/keyframe interactions without introducing a blocking accessibility or state-management regression.
— Magi
97e5054 to
9a6ccd3
Compare
81d53b6 to
1ff38da
Compare
9a6ccd3 to
61c893b
Compare
1ff38da to
044743d
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 044743d.
Building on Miga's LGTM rollup + Magi's direct approval, layering the following net-new items. Miga's aria-valuenow first-state nit, onCopyProperties union-type note, and useContextMenuDismiss consolidation observation still hold at HEAD; no need to re-litigate them.
The waveform fix is the right call — fabricated peaks were a real product-harm surface, not a demo shortcut. Below are second-pass observations focused on the tests / telemetry / APG-completeness lens the peer reviews didn't fully cover.
Should-fix
SpeedMenu claims role="menu" semantics without arrow-key navigation. packages/studio/src/player/components/SpeedMenu.tsx:43-54 — the refactor added role="menu" on the popover and role="menuitemradio" + aria-checked on the options, but did NOT wire useMenuKeyboardNav (unlike the sibling ClipContextMenu / KeyframeDiamondContextMenu in this same PR). Users landing on the menu with keyboard get Tab-through but no arrow-key nav, Home/End, or focus-on-mount. Per APG the menu role expects arrow-key traversal — either add useMenuKeyboardNav(speedMenuContainerRef) (or a menu-body ref) or downgrade to role="group" so the semantics match the actual behavior. Same 3-line integration shape as the sibling menus in this diff.
Silent failure paths lack telemetry. The PR adds five new user-visible degraded states, none of which emit telemetry — the team gets no signal on how often these fire at scale (see the observability rubric on honest-gating PRs):
AudioWaveform.tsx— the.catchatdecodeFailed.set(cacheKey, Date.now())swallows the decode error with notrackStudioEventcall. This is the headline product fix; knowing decode-failure rate is load-bearing for prioritizing a real fallback (or investigating spikes).Player.tsx—handleErrorsetsloadErrorfor composition-load failures, no telemetry.Player.tsx—handleContinueAnyway— no signal on how often users bail out of the 3s asset wait (also a proxy for asset-pipeline health at the top of the funnel).EditModal.tsx—setCopyError(true)on clipboard failure, no telemetry.VideoThumbnail.tsx—setFailed(true), no telemetry.
SpeedMenu.tsx already uses trackStudioEvent("playback", …) in this same file, and the sibling captions PR (#1968) emits studio_caption_autosave_failed for exactly this shape. Not blocking merge, but the value of the honest fallback grows a lot when it's measurable — one follow-up commit would close the loop.
Nits
No unit tests for menuKeyboardNav.ts. New shared helper used in two menus, no sibling menuKeyboardNav.test.ts alongside the other .test.ts files in packages/studio/src/player/components/. The arrow-key / Home/End / focus-restore semantics are easy to regress silently (a stray .focus() call anywhere on the page would defeat the restore). A small test would pin it.
AudioWaveform.tsx — 30s failure TTL is untested. hasRecentDecodeFailure mixes deterministic behavior with wall-clock (DECODE_RETRY_MS = 30_000); a mock-timer test would pin both the initial suppress and the 30s re-attempt cycle.
menuKeyboardNav.ts is APG-partial. Arrow keys + Home/End only; no first-letter (typeahead) search and no roving-tabindex management (items keep default tabindex so each is a Tab stop). Fine for the two internal menus, but worth calling out if this helper gets adopted more widely — spec-completeness gap.
TimelineClip.tsx — nested interactive elements. The <div role="button" tabIndex={0}> contains focusable keyframe diamond <button> children. Nested interactive roles are an ARIA anti-pattern — AT can enumerate/announce inconsistently across NVDA/JAWS/VoiceOver. The e.target !== e.currentTarget guard prevents the JS bubble but not the AT confusion. Pragmatic tradeoff for this PR; worth a follow-up to move the clip focus/role into a leaf child.
— Review by Rames D Jusso
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
LGTM. Originally-clean rubber-stamp on top of the peer reviews already on file.
Diff scope confirmed clean: 17 files, +464/-141. Audited the load-bearing behavior:
- AudioWaveform —
fakePeaksgone; failure cache (decodeFailed+ 30s TTL) prevents refetch-loop; thecacheKeyeffect correctly re-syncspeaks/decodeErroron URL swap so a stale error can't pin the new source. Dashed flat-line + "waveform unavailable" is the right honest degraded state. - KeyframeDiamondContextMenu — Change Ease and Copy Properties re-surfaced;
onCopyPropertieswidened toPromise<boolean> | boolean | voidandTimeline.tsxreturnscopyTextToClipboard(...);result === falsebranch distinguishes failure from void. - menuKeyboardNav — listener scoped to menu container (no conflict with the SeekBar Home/End handler), disabled-item filter, focus-restore in cleanup.
- TimelineClipDiamonds —
event.detail === 0guard gates keyboard-only activation without double-firing withonPointerUp. - BeatStrip — delete migrated off double-click to ⌥-click; hit target 12→24 px; sensible safety fix.
- Player — retry + Continue-anyway paths added;
playerElRefnulled on unmount only if it still points at this player (crossfade-safe).
CI green: player-perf, preview-regression (incl. Preflight + Preview parity), regression all pass at 044743d; Graphite mergeability_check in progress but non-blocking. reviewDecision APPROVED (Magi at 81d53b6).
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Approve. Independent read at exact head. Comprehensive Studio UX polish:
- Fake waveform peaks replaced with explicit "waveform unavailable" degraded state — no fabricated data the user might trim or beat-align against. Decode failure cache with 30s TTL prevents refetch loops.
- Beat delete moved from double-click to ⌥-click — prevents stuttered drag from destroying beats.
- APG menu keyboard nav (ArrowUp/Down/Home/End, focus restore on unmount) via new
useMenuKeyboardNavhook, applied to clip, keyframe, speed, and shortcuts menus. - Player error/retry state with "Continue anyway" escape hatch after 3s asset wait.
- Session-storage draft persistence for edit popover prompts.
- a11y improvements throughout (role/aria-label/aria-pressed/focus-visible on timeline clips and keyframe diamonds).
All additive UI changes, no regression risk. Clean.
— Miga
61c893b to
4671960
Compare
The base branch was changed.
…-delete gesture Player and timeline fixes from the studio UX review, reconciled against six weeks of main. Honest media states: - AudioWaveform no longer falls back to synthesised sine-wave peaks when a decode fails. The failure propagates and the clip renders a dashed flat line + "waveform unavailable" instead of a plausible waveform an author would trim and beat-align against. Main's thumbnail scheduler already caches the failure with a TTL, so this neither refetch-loops nor pins the degraded state past a transient error. - VideoThumbnail renders a static "no preview" placeholder on a failed decode rather than resolving to an empty box. Keyframe context menu, restored: - "Edit Ease…" (showing the current ease) and "Copy Properties" (async, "Copied!"/"Copy failed") were plumbed but never rendered. Edit Ease routes to the same focused-ease-segment path a segment click takes, so the menu advertises the editor that exists instead of growing a second one; it is offered only for a keyframe that names a tween to focus. Copy Properties matches the keyframe cache on clip-% with the same tolerance main's move-to-playhead uses. Every row is a role="menuitem" with arrow-key navigation and focus handling via the new useMenuKeyboardNav helper, and a separator now isolates "Delete All Keyframes" from the single delete. Error prevention: - Beat dots: hit target 12→24px (WCAG 2.5.8), and delete moves off double-click to ⌥-click — a stuttered drag reads as a double-click and would destroy the beat. ⌥ starts no drag, so a slipped ⌥-drag abandons instead of deleting. - ShortcutsPanel moves focus into the panel on open and returns it to the trigger on close; SpeedMenu's trigger is labelled and reports its popup. Superseded by main, deliberately dropped: the seek-slider keyboard and aria-valuenow fixes (the transport no longer owns a seek bar), the Player load-error inline retry (main's reports the actual message and retries with a cache-busting src), TimelineClip keyboard selection (main renders a native button, and this PR's onKeyDown would have preventDefault'ed the synthesized click), the keyframe-diamond keyboard guard and label (both already on main, with a richer label), and the waveform's own cache/failure maps (main's scheduler owns that). TimelineOverlays.tsx is a main-side file edited to thread the two restored menu actions; BeatStrip.test.tsx tracks the new gesture and hit target. Restacked onto main now that PRs 1962-1966 have squash-merged, so this carries only its own changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
044743d to
457f7e5
Compare
…t gating Caption-editing fixes from the studio UX review. This surface held five of the thirteen criticals; the theme is that the editing UI shipped ahead of its apply/persist pipeline, so several controls mutated an in-memory model with no downstream effect, and the mode itself could never be exited. Mode trap: caption edit mode auto-activated on detection and had no exit — `setEditMode(false)` and `reset()` had zero call sites, so the caption overlay replaced normal element editing for the rest of the session, even after switching compositions. The store now resets on composition change (flushing the last debounced edit first), an "Editing captions · Exit" pill sits on the preview, and a re-enter button appears once dismissed. Honest gating of dead surfaces: the Animation tab (31 presets × duration/ease/stagger/intensity) edited state that was never applied to playback nor serialized — wiring it needs a CaptionOverride schema extension in packages/core plus a runtime engine, so the tab is now visibly disabled with an amber "isn't applied to playback or saved yet" notice instead of silently discarding work. Timing edge-drags moved a block that never changed playback and never saved; the handles are gone and the blocks remain as select/seek targets. Double-click split desynced the overlay↔DOM index mapping, so split is out until regeneration exists. Undo: store-level undo/redo (cap 50, 800ms coalescing by edit target) across all ten mutations, with ⌘Z/⇧⌘Z intercepted while caption mode is active and reapplied to the live iframe. Previously ⌘Z reverted an unrelated file edit while the bad caption drag persisted. Autosave: save failures, including non-2xx, raise a persistent "not saved — Retry" banner; the code's own comment called this a data-loss path and it was telemetry-only. Debounced saves flush on unmount instead of being discarded, `beforeunload` flushes and warns while pending, and corrupt overrides JSON is distinguished from a missing file. Input safety and a11y: arrow-key nudge no longer hijacks arrows inside form inputs; numeric fields commit finite values only (typing "-" used to inject NaN into gsap and persist null); "Mixed" shows on multi-select divergence; Escape cancels an in-flight drag and restores the pre-drag transform; ⌘A selects all; caption blocks are keyboard-selectable with a playhead line and click-to-seek (CaptionTimeline's `onSeek` prop existed but nothing passed it); 24px hit areas around the 8px handles; a hint when no boxes are visible; visible input focus styles; tablist semantics. Perf: the 66ms getBoundingClientRect polling loop is replaced with event-driven updates (player-store subscription, preview messages, ResizeObserver, rAF-coalesced); the interval now runs only during playback. Reconciled against main: StudioPreviewArea.tsx was deleted by the Studio revamp (#2291), so the mode pill, the sync-error banner and the re-enter button move to its successor, nle/PreviewOverlays.tsx, and the caption track's onSeek is wired in EditorShell. The per-keyframe onChangeKeyframeEase change that also lived in that file is dropped: main removed the prop, and #1967 now routes the diamond menu's ease action to the focused-ease-segment editor instead. Restacked onto main now that PRs 1962-1967 have squash-merged, so this carries only its own changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t gating (#1968) Caption-editing fixes from the studio UX review. This surface held five of the thirteen criticals; the theme is that the editing UI shipped ahead of its apply/persist pipeline, so several controls mutated an in-memory model with no downstream effect, and the mode itself could never be exited. Mode trap: caption edit mode auto-activated on detection and had no exit — `setEditMode(false)` and `reset()` had zero call sites, so the caption overlay replaced normal element editing for the rest of the session, even after switching compositions. The store now resets on composition change (flushing the last debounced edit first), an "Editing captions · Exit" pill sits on the preview, and a re-enter button appears once dismissed. Honest gating of dead surfaces: the Animation tab (31 presets × duration/ease/stagger/intensity) edited state that was never applied to playback nor serialized — wiring it needs a CaptionOverride schema extension in packages/core plus a runtime engine, so the tab is now visibly disabled with an amber "isn't applied to playback or saved yet" notice instead of silently discarding work. Timing edge-drags moved a block that never changed playback and never saved; the handles are gone and the blocks remain as select/seek targets. Double-click split desynced the overlay↔DOM index mapping, so split is out until regeneration exists. Undo: store-level undo/redo (cap 50, 800ms coalescing by edit target) across all ten mutations, with ⌘Z/⇧⌘Z intercepted while caption mode is active and reapplied to the live iframe. Previously ⌘Z reverted an unrelated file edit while the bad caption drag persisted. Autosave: save failures, including non-2xx, raise a persistent "not saved — Retry" banner; the code's own comment called this a data-loss path and it was telemetry-only. Debounced saves flush on unmount instead of being discarded, `beforeunload` flushes and warns while pending, and corrupt overrides JSON is distinguished from a missing file. Input safety and a11y: arrow-key nudge no longer hijacks arrows inside form inputs; numeric fields commit finite values only (typing "-" used to inject NaN into gsap and persist null); "Mixed" shows on multi-select divergence; Escape cancels an in-flight drag and restores the pre-drag transform; ⌘A selects all; caption blocks are keyboard-selectable with a playhead line and click-to-seek (CaptionTimeline's `onSeek` prop existed but nothing passed it); 24px hit areas around the 8px handles; a hint when no boxes are visible; visible input focus styles; tablist semantics. Perf: the 66ms getBoundingClientRect polling loop is replaced with event-driven updates (player-store subscription, preview messages, ResizeObserver, rAF-coalesced); the interval now runs only during playback. Reconciled against main: StudioPreviewArea.tsx was deleted by the Studio revamp (#2291), so the mode pill, the sync-error banner and the re-enter button move to its successor, nle/PreviewOverlays.tsx, and the caption track's onSeek is wired in EditorShell. The per-keyframe onChangeKeyframeEase change that also lived in that file is dropped: main removed the prop, and #1967 now routes the diamond menu's ease action to the focused-ease-segment editor instead. Restacked onto main now that PRs 1962-1967 have squash-merged, so this carries only its own changes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 participants
Summary
Player + timeline fixes from the studio UX review. Standouts: the keyframe context menu dropped two fully-wired actions (Change Ease and Copy Properties were plumbed all the way through
Timeline.tsxbut never rendered), and a failed waveform decode silently rendered fabricated sine-wave peaks that users would then trim against.Changes
Restored dead features:
Honest media states:
errorevent shows an inline error + Retry (was: silently cleared the loading overlay, leaving a blank preview); asset-wait overlay logs the promised debug line and offers "Continue anyway" after 3s.Keyboard access:
role="button", focusable, Enter/Space selection, focus ring (selection gates all downstream keyframe/property editing and was mouse-only).onClickgated onevent.detail === 0so pointer paths don't double-fire), labeled "Keyframe at N%".aria-haspopup/aria-expanded,menuitemradio, CSS hover, visible disabled state. ShortcutsPanel: Escape closes (a keyboard-help panel keyboard users couldn't dismiss), focus in/restore, scoped section titles disambiguate duplicate K/Del bindings, new "⇧Click — split all tracks" row (the gesture existed but was documented nowhere). Context menus get menu roles + arrow-key nav via a small shared helper. Seek slider: Home/End + fixedaria-valuenowclobber.Error-prevention:
Craft: clip hover 100→150ms; seek-thumb hover scale 1.25→1.10; overlay fade-out ease-in.
Verification
src/player/: 249 pass / 0 fail; oxlint 0 errors across all 65 player files; tsc cleanStack
PR 6/7 of the studio UX-review fixes.
🤖 Generated with Claude Code