feat(studio): add timeline keyframe retiming interactions#2685
Conversation
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 a1c9ebd1ebb2.
Retiming path routes drag gestures through the shared timing model. Two blockers on the interaction contract: the pending-retime feature this PR introduces is dead in real usage (blocker 1 — Promise<boolean> never propagates), and silent occupied-destination no-op leaves the playhead + selection stranded at the ghost drop position (blocker 2). Both compose with Family A #2673 findings on moveKeyframeInScript silent-collision.
Findings not anchored inline
🟠 Context-menu Delete / Move-to-Playhead wiring discards the new group-identity args added by this PR
packages/studio/src/player/components/TimelineOverlays.tsx:109
<KeyframeDiamondContextMenu
state={kfContextMenu}
onClose={() => setKfContextMenu(null)}
onDelete={(elId, pct) => onDeleteKeyframe?.(elId, pct)}
onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)}
onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)}
onMoveToPlayhead={
onMoveKeyframeToPlayhead
? (elId, pct) => onMoveKeyframeToPlayhead(elId, pct)
: undefined
}
...
/>PR extends KeyframeDiamondContextMenuState with propertyGroup?: string; animationId?: string and both onDelete and onMoveToPlayhead now accept (elementId, percentage, propertyGroup?, tweenPercentage?, animationId?). useTimelineKeyframeHandlers.ts populates the state with propertyGroup: target.propertyGroup, animationId: target.animationId (:74-89 of the diff). But TimelineOverlays.tsx:109,113 still wraps with 2-arg lambdas, so propertyGroup/animationId/tweenPercentage are silently DROPPED. On a property-group-aware diamond, delete/move-to-playhead falls back to resolveKeyframeTarget(pct) which resolves by clip-% only and picks arbitrarily when two animations coincide at the same clip-%.
Fix: Rest-args forwarding: onDelete={(...args) => onDeleteKeyframe?.(...args)} / onMoveToPlayhead={(...args) => onMoveKeyframeToPlayhead(...args)}. Also confirm downstream signatures accept the wider tuple.
🟢 Verified clean
- timelineKeyframeIdentity.ts (:17 new): identity model uses tuple encoding (elementId, propertyGroup, animationId, percentage) that survives the sort round-trip
- KeyframeDiamondContextMenu.tsx (+30/-4): correctly typed extension of the state and callback signatures, matches useTimelineKeyframeHandlers.ts population
- useTimelineKeyframeHandlers.ts (+46/-11): correctly passes propertyGroup + animationId + tweenPercentage into the menu state; the wiring loss is downstream in TimelineOverlays
Cross-stack notes
Two blockers here (dead Promise chain + silent-collision UX) block the exact 'occupied-destination safety' claim in the PR body. Both compose with Family A #2673 findings on moveKeyframeInScript. The context-menu arg-dropping (finding 3) is fixed in #2690's TimelineOverlays diff — worth mentioning in that PR review so it's clear the fix ships downstream. The pending-retime dead-code bug (finding 1) can only be fixed by threading Promise back through the callback chain, which affects #2686-#2690's TimelineLanes/callbacks type shape — recommend addressing here before consumers imitate the void-return pattern.
| ); | ||
| }); | ||
|
|
||
| export const TimelineClipDiamonds = memo(function TimelineClipDiamonds( |
There was a problem hiding this comment.
🔴 committed is always false in production — pending-retime feature is dead on the only wired path
onMoveKeyframe={
props.onMoveKeyframe
? (target, toClipPercentage) =>
props.onMoveKeyframe?.(props.elementId, target.percentage, toClipPercentage) ??
Promise.resolve(false)
: undefined
}TimelineLanes.tsx:77-81 (pr-2685) declares onMoveKeyframe?: (elementId, fromClipPercentage, toClipPercentage) => void — void return. useTimelineEditCallbacks.ts:155-201 (pr-2685) implements it as (_elId, fromClipPct, toClipPct) => { ... handleGsapMoveKeyframe(...); ... } — implicit void return; the actual Promise<boolean> from moveKeyframe is discarded inside a fire-and-forget branch. The compat wrapper's ?? Promise.resolve(false) fallback then fires unconditionally because props.onMoveKeyframe?.(...) returns undefined. Every retime therefore resolves committed === false and clearPending() runs on the next microtask, deleting the pending entry regardless of actual server outcome. The 'rapid second retime composes from where the first move left the keyframe' feature this PR introduces (verified by the third test with mockResolvedValue(true)) is dead in real usage — the pending state can only survive if the second onPointerDown occurs in the SAME synchronous tick, which no real user can achieve.
Fix: Propagate the underlying Promise<boolean> up: change TimelineEditCallbacks.onMoveKeyframe return type to Promise<boolean> and thread the return value through useTimelineEditCallbacks.onMoveKeyframe → TimelineLanes.onMoveKeyframe → the compat wrapper.
| key={`${i}-${kf.percentage}`} | ||
| type="button" | ||
| className="absolute" | ||
| data-keyframe-group={groupAware ? kf.propertyGroup : undefined} |
There was a problem hiding this comment.
🔴 Silent occupied-destination no-op leaves playhead + selection at ghost drop position
void onMoveKeyframe?.(fromTarget, res.toClipPct).then((committed) => {
if (!committed) clearPending();
}, clearPending);
// A retime still targeted this exact diamond — park/select it at its
// new position, same as a plain click, or a drag that actually moved
// something looks identical to one that silently did nothing.
onClickKeyframe?.({
...target,
percentage: res.toClipPct,
tweenPercentage: newTweenPct,
});The drag handler fires onClickKeyframe synchronously at res.toClipPct BEFORE the mutation promise resolves. On collision (gsapWriterAcorn.ts:1331-1332 if (dest && dest.prop !== match.prop) return script;), the server returns unchanged, committed === false, clearPending() fires — but by then the playhead has already seeked to the ghost drop position and the diamond selection points at a tween-% that doesn't exist in the source. No snap-back animation, no toast, no rejection signal. Exactly the Family A silent-reject class I flagged as needing UX surfacing (#2673 moveKeyframeInScript collision); the PR body claims occupied-destination safety but the safety is invisible.
Fix: Two options: (a) On committed === false, snap playhead + selection back to fromTarget.percentage; (b) Also surface a subtle toast / status-line message on non-commit. Composing with the blocker above (once the boolean is threaded), a single if (!committed) { restore(); toastCollision(); } closes both.
| width: x2 - x1, | ||
| height: 2, | ||
| transform: "translateY(-1px)", | ||
| background: baseColor, |
There was a problem hiding this comment.
🟠 Segment ease button unreachable via keyboard (mount gated on hover only)
<div
className="absolute"
data-keyframe-ease-segment=""
onMouseEnter={() => setHoveredSegment(i)}
onMouseLeave={() => setHoveredSegment((h) => (h === i ? null : h))}
>
{hoveredSegment === i && (
<button
type="button"
data-keyframe-ease-button=""
aria-label={`Edit ${ease} easing`}
...
>The button is conditionally mounted only while hoveredSegment === i, set exclusively via onMouseEnter/onMouseLeave on the wrapper div. No onFocus/onBlur, no tabIndex on the wrapper, no always-mounted-but-visually-hidden button. Keyboard-only + SR users cannot open the segment ease editor from the timeline row — primary keyboard-inaccessible entry point to a feature the PR positions as a first-class Figma-style interaction. Note: #2689 later moves this to CSS group-hover/focus-visible; would be cleaner to do that here.
Fix: Always mount the button; hide via CSS opacity-0 + reveal on both group-hover and focus-visible. Cover Tab-through in the test.
| // second drag composes from where the first move left the keyframe (whose | ||
| // cache entry has not rebuilt yet) instead of the stale rendered value. | ||
| const pendingRetimeRef = useRef(new Map<string, { clipPct: number; tweenPct: number }>()); | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟡 useEffect clears pending entries by clip-% only, ignoring keyframe identity
useEffect(() => {
for (const [key, pending] of pendingRetimeRef.current) {
if (keyframesData.keyframes.some((k) => Math.abs(k.percentage - pending.clipPct) < 0.2)) {
pendingRetimeRef.current.delete(key);
}
}
}, [keyframesData.keyframes]);The key map key (elementId:group:animId:pct) is not consulted — only pending.clipPct is. In group-aware mode keyframesData.keyframes merges multiple animations for a property group, so a pending entry for animation A at 75% clip-% is cleared by ANY keyframe at ~75% clip-% (including an X-anim and Y-anim keyframe both at 75%). Defeats pending composition when two animations share a lane. Not exercised at pr-2685 (property lanes not wired) but the code is being introduced pre-emptively wrong.
Fix: Parse the pending entry's identity from key (or store animationId in the pending value) and require identity-match, not just clip-% proximity.
| type="button" | ||
| className="absolute" | ||
| data-keyframe-group={groupAware ? kf.propertyGroup : undefined} | ||
| data-keyframe-percentage={ |
There was a problem hiding this comment.
🟢 data-keyframe-percentage attribute is polysemous
data-keyframe-percentage={
groupAware ? (kf.tweenPercentage ?? kf.percentage) : undefined
}Single attribute is tween-% when defined, but silently falls back to clip-% when tweenPercentage is missing (runtime-scanned keyframes with no source animationId). DOM selector/test reading this attribute cannot tell which coordinate space it's in — the two spaces are numerically identical only for keyframes whose animation window equals the clip window.
Fix: Two distinct data attributes (data-tween-percentage vs data-clip-percentage) or omit the attribute when tween-% is unknown so downstream consumers get an explicit signal.
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onContextMenuKeyframe?.(e, elementId, kf.percentage); | ||
| onContextMenuKeyframe?.(e, target); |
There was a problem hiding this comment.
🟢 onPointerCancel clears drag state but never fires the click-fallback
onPointerCancel={(e) => {
// Browser/OS cancellation (or lost capture) ends the drag without a
// pointerup, so clear the armed drag and preview or a ghost diamond
// stays stuck at the last previewed position.
if (dragRef.current?.kfKey !== kfKey) return;
dragRef.current = null;
setPreview(null);
e.currentTarget.releasePointerCapture?.(e.pointerId);
}}Correctly avoids ghost-preview stuck state, but if the user's intent was a click (d.moved never became true) and the OS cancels the pointer (touch scroll override, palm rejection, focus loss), the click is lost entirely — onClickKeyframe never fires. Acceptable trade-off but worth explicit decision.
Fix: If !d.moved at cancel time, still treat it as a click, matching the pointerup fallback branch.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at a1c9ebd1ebb2.
Retiming interactions on the diamond surface. Rames's R1 identified two blockers on the interaction contract (pending-retime Promise<boolean> is dead in real usage because the return type is void through TimelinePropertyLanes; occupied-destination silent no-op leaves playhead stranded). I want to add one lens-specific finding he did not explicitly anchor: the drag surface is keyboard-inaccessible — Lens 5 of the ten-lens editor-UI set.
Prior review
Rames D Jusso R1 blockers:
- Pending-retime dead code —
onMoveKeyframe?return type atTimelinePropertyLanesis=> void, drops thePromise<boolean>contract this PR introduces (verified atTimelinePropertyLanes.tsx:28@ #2685 head). - Occupied-destination silent no-op — playhead + selection stranded at ghost drop position.
- Context-menu
onDelete/onMoveToPlayheadwiring inTimelineOverlays.tsx:109drops the newpropertyGroup/animationId/tweenPercentageargs.
Overlap: my read of the diff confirms all three. Blocker 1 does get fixed downstream in #2689 (onMoveKeyframe?: (target, toClipPercentage) => Promise<boolean> and export of animationContributesLane) — but within this slice, TimelinePropertyLanes passing a void-returning callback to TimelineDiamondLane's Promise<boolean>-expecting slot IS a runtime issue (.then(...) on undefined), just masked because #2685 has no live wiring of TimelinePropertyLanes yet.
🟠 Additive — Lens 5: retiming drag surface is keyboard-inaccessible
packages/studio/src/player/components/TimelineClipDiamonds.tsx (TimelineDiamondLane diamond <button>)
The diamond <button> handles onPointerDown/Move/Up for retime + click, so it IS focusable and Enter/Space activate res.kind === "click". But retiming itself is mouse-only — there is no arrow-key nudge handler, no role="slider", no aria-valuenow/aria-valuemin/aria-valuemax, no Home/End handling. A keyboard-only user can Tab to a diamond and "click" it (which selects), but cannot retime — and a keyframe editor whose retime is unreachable by keyboard fails an interactive-editor accessibility contract.
Because the segment-ease <button> this PR introduces IS reachable by Tab+Enter (click-only affordance), the ease-editor side of the surface stays keyboard-accessible; the gap is specifically the drag semantics.
Suggested direction: thread arrow-left/right handlers into onKeyDown on the diamond <button> that call the same onMoveKeyframe(target, target.percentage ± step) path the drag uses; add role="slider" with aria-valuenow={kf.percentage}, aria-valuemin={0}, aria-valuemax={100}. Not required for this PR to ship, but the ten-lens set (Category A, Lens 5) says any surface responding to onPointerDown for dragging needs a keyboard equivalent — and this is a drag surface.
🟢 Verified clean
pendingRetimeRefcleanup effect's< 0.2tolerance matches the cache writers'Math.round(... * 1000) / 10precision (rounds to 0.1%) — the entry clears once the cache reflects the destination.onPointerCancelhandler correctly clearsdragRef+previewon OS-cancelled gestures. Note:releasePointerCaptureis called defensively butsetPointerCaptureis never called on this element, so the release is a no-op — harmless.- Key
${elementId}:${percentage}(viatimelineKeyframeSelectionKey) is stable identity, not driven by drag preview state — Lens 3 clean. - Fragment key
line-${i}-${prev.percentage}-${kf.percentage}uses source percentages, notrenderPct— stable during drag. <button data-keyframe-ease-button>aria-label={Edit ${ease} easing}— Rames anchors the a11y label loss in #2686; not a blocker here since a single-diamond context has no property-lane duplication yet.
Cross-stack
Rames flagged that the context-menu arg-drop (finding 3) is patched downstream in #2690's TimelineOverlays diff. Confirmed. Blocker 1 (Promise propagation) is patched in #2689's TimelinePropertyLanes update. So all three of Rames's blockers heal within the stack — but this slice on its own has a type-contract mismatch that would crash .then on any real consumer wiring, and the keyboard-accessibility gap has no downstream fix.
Verdict
Recommending COMMENTED — Lens 5 keyboard gap is a structural accessibility defect on an interactive-editor surface that no downstream slice repairs. Rames's blockers heal but shouldn't ship as intermediate broken state on a squash-then-merge stack.
— Review by Via
1c04226 to
c3219cc
Compare
a1c9ebd to
8ecae47
Compare
c3219cc to
1e99115
Compare
8ecae47 to
5816e70
Compare
Fallow audit reportFound 5 findings. Duplication (4)
Health (1)
Generated by fallow. |
3 participants
What
Adds direct timeline keyframe retiming interactions.
Why
Editors need to drag keyframes while preserving element identity, clip-relative timing, occupied-destination safety, and deterministic source updates.
How
Routes retime gestures through the shared timing model and mutation callbacks, with focused coverage for drag lifecycle and destination rules. This is B3 of the Family B Graphite stack.
Review fix folded in:
onMoveKeyframereturnsPromise<boolean>across the whole callback chain (timelineCallbacks->TimelineLanes->useTimelineEditCallbacks). This commit is whereTimelineClipDiamondsstarts awaiting the commit result, so the widening belongs here; without it the intermediate commits do not typecheck.Test plan
Every commit in the stack typechecks independently. Drag-to-retime verified in a browser: dragging a keyframe rewrites its percentage on disk while preserving both its value and its ease.
Deferred review findings
Every blocker and high finding raised on this PR is fixed in the stack. The 3 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/03-pr-2685-deferred-review-findings.md:packages/studio/src/player/components/TimelineClipDiamonds.tsx:139— useEffect clears pending entries by clip-% only, ignoring keyframe identitypackages/studio/src/player/components/TimelineClipDiamonds.tsx:424— data-keyframe-percentage attribute is polysemouspackages/studio/src/player/components/TimelineClipDiamonds.tsx:456— onPointerCancel clears drag state but never fires the click-fallback