fix(studio): settle boundary retimes, delete every keyframed tween, tighten the test locks

Review follow-ups on the expanded keyframe lanes.

Writer:
- `onMoveKeyframe`'s flat-tween boundary branch answered `true` the moment it
  dispatched update-meta, so a rejected write left the diamond parked at its drop
  position. `observeGsapMutation` now resolves to whether the mutation landed and
  the boundary branch returns it, matching the other branches.
- "Delete All Keyframes" cleared only the first keyframed tween on the layer, so
  a layer with position AND opacity keyframes kept half of them. It now walks
  every keyframed tween, serially, through the clicked element's selection.
- The post-convert lookup in `commitFlatViaKeyframes` matched by target selector,
  which picks an arbitrary tween when a target carries several. Match by id first.

Interaction and a11y:
- A rejected retime whose commit settled after a newer drag reverted the
  selection to its own source keyframe, undoing a retime the user could see. The
  revert now only runs while it is still the lane's latest gesture.
- Diamonds key on the authored identity instead of index plus rendered clip-%, so
  a neighbour's retime no longer remounts the button mid-drag.
- The disclosure caret gets `aria-controls` on an always-mounted lanes container,
  and both it and the property-group toggle grow to the 24x24 WCAG 2.2 minimum.
- `LayerDisclosureRow` takes the same adaptive `columnWidth` as its sibling lane
  rows instead of hardcoding LABEL_COL_W over the canvas.

Test locks:
- The timeline callbacks harness resolves a DISTINCT selection per element, so
  the clicked-element writes are actually pinned; three assertions that passed
  either way now name the clicked element's selection.
- New: null-selection aborts every mutation, delete-all covers both tweens, a
  rejected boundary retime reports `false`, and a stale revert leaves selection.
- The playhead-percentage assertion checks 25, not `expect.any(Number)` (which
  also accepts NaN); ease segments assert their label ORDER, not just that the
  three curves differ; the collapsed-diamond callback asserts the whole target.
- Dropped a duplicate `selection override` describe left by a rebase.

Author: miguel-heygen

packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx

@@ -16,11 +16,11 @@ const mocks = vi.hoisted(() => ({
     handleGsapMoveKeyframeToPlayhead: vi.fn(),
     handleGsapMoveKeyframe: vi.fn().mockResolvedValue(true),
     handleGsapResizeKeyframedTween: vi.fn().mockResolvedValue(true),
-    handleGsapUpdateMeta: vi.fn(),
+    handleGsapUpdateMeta: vi.fn().mockResolvedValue(true),
     handleGsapAddKeyframe: vi.fn(),
     handleGsapAddKeyframeBatch: vi.fn().mockResolvedValue(undefined),
     handleGsapConvertToKeyframes: vi.fn(),
-    handleGsapRemoveAllKeyframes: vi.fn(),
+    handleGsapRemoveAllKeyframes: vi.fn().mockResolvedValue(true),
     handleGsapDeleteAnimation: vi.fn(),
     buildDomSelectionForTimelineElement: vi.fn(),
   },
@@ -116,6 +116,19 @@ function renderCallbacks(): { callbacks: TimelineEditCallbacks; unmount: () => v
   return { callbacks, unmount: () => act(() => root.unmount()) };
 }
 
+// One selection PER element, so a callback that resolves the selection for the
+// wrong element gets a visibly different object. A single mockResolvedValue
+// hands every element the same selection, which passes just as happily when the
+// write is committed through whatever happens to be selected.
+function selectionForElement(el: TimelineElement): {
+  id: string;
+  selector: string;
+  sourceFile: string;
+} {
+  if (el.id === "box") return mocks.selection;
+  return { id: el.id, selector: `#${el.id}`, sourceFile: el.sourceFile ?? "index.html" };
+}
+
 function arrangeClickedCircle(): {
   circle: TimelineElement;
   selection: { id: string; selector: string; sourceFile: string };
@@ -128,19 +141,19 @@ function arrangeClickedCircle(): {
     domId: "circle",
     sourceFile: "scenes/main.html",
   };
-  const selection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" };
   usePlayerStore.setState({
     elements: [element, circle],
     gsapAnimations: new Map([[elementKey, [otherKeyframedAnimation]]]),
   });
-  mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(selection);
-  return { circle, selection };
+  return { circle, selection: selectionForElement(circle) };
 }
 
 beforeEach(() => {
   vi.clearAllMocks();
   mocks.animations = [flatAnimation];
-  mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(mocks.selection);
+  mocks.actions.buildDomSelectionForTimelineElement.mockImplementation((el: TimelineElement) =>
+    Promise.resolve(selectionForElement(el)),
+  );
   usePlayerStore.setState({
     currentTime: 0.5,
     elements: [element],
@@ -212,6 +225,27 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
     view.unmount();
   });
 
+  it("reports an unsettled flat-boundary retime as uncommitted", async () => {
+    mocks.actions.handleGsapUpdateMeta.mockResolvedValueOnce(false);
+    const view = renderCallbacks();
+
+    // The diamond snaps back on `false`. Answering `true` the moment update-meta
+    // was dispatched left a rejected boundary drag rendered at its drop position.
+    await expect(
+      view.callbacks.onMoveKeyframe?.(
+        "box",
+        {
+          percentage: 0,
+          propertyGroup: "position",
+          tweenPercentage: 0,
+          animationId: flatAnimation.id,
+        },
+        25,
+      ),
+    ).resolves.toBe(false);
+    view.unmount();
+  });
+
   it("refuses a non-selected element flat boundary instead of deleting the tween", async () => {
     const circle: TimelineElement = {
       ...element,
@@ -242,7 +276,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
       otherFlatAnimation.id,
       0,
       undefined,
-      mocks.selection,
+      selectionForElement(circle),
     );
     expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
     view.unmount();
@@ -276,7 +310,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
       otherKeyframedAnimation.id,
       100,
       undefined,
-      mocks.selection,
+      selectionForElement(circle),
     );
     expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
     view.unmount();
@@ -298,6 +332,65 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
     view.unmount();
   });
 
+  it("deletes all keyframes on every keyframed tween of the layer, not just the first", async () => {
+    const opacityAnimation: GsapAnimation = {
+      ...otherKeyframedAnimation,
+      id: "circle-to-0-visual",
+      propertyGroup: "visual",
+    };
+    const { circle } = arrangeClickedCircle();
+    usePlayerStore.setState({
+      gsapAnimations: new Map([
+        ["scenes/main.html#circle", [otherKeyframedAnimation, opacityAnimation]],
+      ]),
+    });
+    const view = renderCallbacks();
+
+    await act(async () => {
+      view.callbacks.onDeleteAllKeyframes?.(circle);
+      await Promise.resolve();
+      await Promise.resolve();
+      await Promise.resolve();
+    });
+
+    expect(mocks.actions.handleGsapRemoveAllKeyframes.mock.calls.map((call) => call[0])).toEqual([
+      otherKeyframedAnimation.id,
+      opacityAnimation.id,
+    ]);
+    view.unmount();
+  });
+
+  it("aborts every mutation when the clicked element resolves no selection", async () => {
+    const { circle } = arrangeClickedCircle();
+    mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(null);
+    const view = renderCallbacks();
+
+    await act(async () => {
+      view.callbacks.onDeleteAllKeyframes?.(circle);
+      view.callbacks.onMoveKeyframeToPlayhead?.(circle, {
+        percentage: 100,
+        propertyGroup: "position",
+        tweenPercentage: 100,
+        animationId: otherKeyframedAnimation.id,
+      });
+      view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", {
+        percentage: 100,
+        propertyGroup: "position",
+        tweenPercentage: 100,
+        animationId: otherKeyframedAnimation.id,
+      });
+      await Promise.resolve();
+      await Promise.resolve();
+    });
+
+    // No selection for the clicked element means there is nothing safe to write
+    // to: falling back to the current selection would edit a different file.
+    expect(mocks.actions.handleGsapRemoveAllKeyframes).not.toHaveBeenCalled();
+    expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).not.toHaveBeenCalled();
+    expect(mocks.actions.handleGsapRemoveKeyframe).not.toHaveBeenCalled();
+    view.unmount();
+  });
+
   it("moves a keyframe to the playhead through the clicked non-selected element's identity", async () => {
     const { circle, selection } = arrangeClickedCircle();
     const view = renderCallbacks();
@@ -398,7 +491,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
       otherFlatAnimation.id,
       0,
       undefined,
-      mocks.selection,
+      selectionForElement(circle),
     );
     expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
     view.unmount();

packages/studio/src/components/nle/useTimelineEditCallbacks.ts

@@ -194,10 +194,18 @@ export function useTimelineEditCallbacks({
         // than deleting the whole animation — deleting strands a stale GSAP base
         // that the next drag adds to, flinging the element off-screen.
         const elementKey = getTimelineElementIdentity(element);
-        const anim = resolveElementAnimations(elementKey).find((animation) => animation.keyframes);
-        if (!anim) return;
-        void buildDomSelectionForTimelineElement(element).then((selection) => {
-          if (selection) handleGsapRemoveAllKeyframes(anim.id, selection);
+        // Every keyframed tween on the layer, not just the first: a layer with
+        // position AND opacity keyframes left the second one keyframed, so
+        // "Delete All Keyframes" visibly did half the job.
+        const anims = resolveElementAnimations(elementKey).filter(
+          (animation) => animation.keyframes,
+        );
+        if (anims.length === 0) return;
+        void buildDomSelectionForTimelineElement(element).then(async (selection) => {
+          if (!selection) return;
+          // Serial: each removal rewrites the same source file, so dispatching
+          // them together would have the later writes read a pre-edit document.
+          for (const anim of anims) await handleGsapRemoveAllKeyframes(anim.id, selection);
         });
       },
       onDeleteKeyframe: (elId, keyframe) => {
@@ -287,12 +295,14 @@ export function useTimelineEditCallbacks({
           // keyframes form as a side effect of a pure position/duration change, so
           // dispatch update-meta and leave the tween as the author wrote it.
           if (decision.pctRemap.length === 0) {
-            handleGsapUpdateMeta(
+            // Report the write's real settlement, like every other branch here:
+            // answering `true` while the meta update is still in flight tells the
+            // diamond the retime landed, so a rejected write never snaps back.
+            return handleGsapUpdateMeta(
               target.animId,
               { position: decision.position, duration: decision.duration },
               sel,
             );
-            return true;
           }
           return handleGsapResizeKeyframedTween(
             target.animId,

packages/studio/src/hooks/gsapDragPositionCommit.ts

@@ -210,8 +210,13 @@ async function commitFlatViaKeyframes(
       { label: "Convert to keyframes for drag", skipReload: true, coalesceKey },
     );
     const fresh = callbacks.fetchAnimations ? await callbacks.fetchAnimations() : [];
+    // By id first: a target with several tweens (two `to`s on the same selector)
+    // matches the selector lookup on whichever one happens to be first, and the
+    // extend-and-add below would then rewrite a tween the drag never touched.
     const converted =
-      fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ?? anim;
+      fresh.find((a) => a.id === anim.id && a.keyframes) ??
+      fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ??
+      anim;
     const convertedStart = resolveTweenStart(converted) ?? ts;
     const convertedDur = resolveTweenDuration(converted) || td;
     await extendTweenAndAddKeyframe(

packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx

@@ -4,6 +4,7 @@ import { createRoot } from "react-dom/client";
 import { describe, expect, it, vi } from "vitest";
 import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
 import type { DomEditSelection } from "../components/editor/domEditingTypes";
+import { usePlayerStore } from "../player/store/playerStore";
 import { useGsapSelectionHandlers } from "./useGsapSelectionHandlers";
 
 (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
@@ -80,7 +81,11 @@ describe("useGsapSelectionHandlers save failures", () => {
       makeParams({ updateGsapMeta: vi.fn().mockRejectedValue(error), showToast }),
     );
 
-    act(() => rendered.handlers().handleGsapUpdateMeta("anim-1", { duration: 2 }));
+    // Braces, not a bare arrow: the handler returns its settlement promise now,
+    // and returning a thenable from act() turns it into an un-awaited async act.
+    act(() => {
+      void rendered.handlers().handleGsapUpdateMeta("anim-1", { duration: 2 });
+    });
     await flushRejection();
 
     expect(showToast).toHaveBeenCalledWith("Couldn't save animation: write failed", "error");
@@ -135,12 +140,24 @@ describe("useGsapSelectionHandlers selection override", () => {
   it("computes the playhead percentage from the passed animation, not the selection's", () => {
     const moveKeyframe = vi.fn();
     const selection = makeSelection();
-    const animation = { id: "anim-1", keyframes: { keyframes: [] } } as unknown as GsapAnimation;
+    // The passed tween runs 2s→6s, so the playhead at 3s is 25% into IT. Without
+    // the animation the handler falls back to the selection's own element window
+    // (0s→1s here), which reads the same playhead as 100%. Asserting the exact
+    // 25 is what separates the two; `expect.any(Number)` even accepts the NaN a
+    // missing window would produce.
+    const animation = {
+      id: "anim-1",
+      position: 2,
+      resolvedStart: 2,
+      duration: 4,
+      keyframes: { keyframes: [] },
+    } as unknown as GsapAnimation;
+    usePlayerStore.setState({ currentTime: 3 });
     const rendered = renderHandlers(makeParams({ moveKeyframe, selectedGsapAnimations: [] }));
 
     rendered.handlers().handleGsapMoveKeyframeToPlayhead("anim-1", 50, selection, animation);
 
-    expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, expect.any(Number));
+    expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, 25);
     rendered.unmount();
   });
 });
@@ -163,20 +180,3 @@ describe("useGsapSelectionHandlers retime settlement", () => {
     withSelection.unmount();
   });
 });
-
-describe("useGsapSelectionHandlers selection override", () => {
-  it("aborts on an explicit null override instead of writing to the current selection", () => {
-    const removeKeyframe = vi.fn();
-    const rendered = renderHandlers(makeParams({ removeKeyframe }));
-
-    // Explicit null: the caller resolved a selection for its own element and
-    // found none, so the write must not land on the selected element.
-    rendered.handlers().handleGsapRemoveKeyframe("anim-1", 50, undefined, null);
-    expect(removeKeyframe).not.toHaveBeenCalled();
-
-    // Omitted override: falls back to the current selection as before.
-    rendered.handlers().handleGsapRemoveKeyframe("anim-1", 50);
-    expect(removeKeyframe).toHaveBeenCalledOnce();
-    rendered.unmount();
-  });
-});

packages/studio/src/hooks/useGsapSelectionHandlers.ts

@@ -150,12 +150,24 @@ export function useGsapSelectionHandlers({
     [showToast],
   );
 
+  // Resolves to whether the mutation landed. Callers that only fire-and-forget
+  // can ignore it (the rejection is always handled here), but a caller that
+  // reports a commit result to the UI has to await the real settlement instead
+  // of assuming success the moment it dispatched.
   const observeGsapMutation = useCallback(
-    (mutation: Promise<void>, selection: DomEditSelection, mutationType: string, label: string) => {
-      void mutation.catch((error) => {
-        trackGsapHandlerFailure(error, selection, mutationType, label);
-      });
-    },
+    (
+      mutation: Promise<void>,
+      selection: DomEditSelection,
+      mutationType: string,
+      label: string,
+    ): Promise<boolean> =>
+      mutation.then(
+        () => true,
+        (error: unknown) => {
+          trackGsapHandlerFailure(error, selection, mutationType, label);
+          return false;
+        },
+      ),
     [trackGsapHandlerFailure],
   );
 
@@ -174,8 +186,8 @@ export function useGsapSelectionHandlers({
       selectionOverride?: DomEditSelection | null,
     ) => {
       const sel = resolveWriteSelection(selectionOverride);
-      if (!sel) return;
-      observeGsapMutation(
+      if (!sel) return Promise.resolve(false);
+      return observeGsapMutation(
         updateGsapMeta(sel, animId, updates),
         sel,
         "update-meta",
@@ -419,8 +431,8 @@ export function useGsapSelectionHandlers({
   const handleGsapRemoveAllKeyframes = useCallback(
     (animId: string, selectionOverride?: DomEditSelection | null) => {
       const selection = resolveWriteSelection(selectionOverride);
-      if (!selection) return;
-      observeGsapMutation(
+      if (!selection) return Promise.resolve(false);
+      return observeGsapMutation(
         removeAllKeyframes(selection, animId),
         selection,
         "remove-all-keyframes",