refactor(studio): give every keyframe diamond one identity

The collapsed clip row dropped a keyframe's property group and animation id
before handing it to a callback, so the same keyframe hashed to a different
selection key than the expanded property lane did. Selecting a diamond in one
view left it unselected in the other, and retime/delete on the collapsed row
lost the animation id they use to pick between two animations that collide at
one percentage.

Diamonds now always carry their full identity, the collapsed shim just curries
the element id, and Timeline reuses useTimelineKeyframeHandlers instead of its
own inline copy of the same three handlers.

Neighbour geometry moves into one marker record per diamond, which drops the
index-lookup non-null assertions the connector pass needed.

packages/studio/src/player/components/Timeline.tsx

@@ -28,6 +28,7 @@ import { useResolvedTimelineEditCallbacks } from "./useResolvedTimelineEditCallb
 import type { TimelineProps } from "./TimelineTypes";
 import { useTrackGapMenu } from "./useTrackGapMenu";
 import { useTimelineGapHighlights } from "./useTimelineGapHighlights";
+import { useTimelineKeyframeHandlers } from "./useTimelineKeyframeHandlers";
 import { useStudioPlaybackContextOptional } from "../../contexts/StudioContext";
 
 // Re-export pure utilities so existing imports from "./Timeline" still resolve.
@@ -262,6 +263,17 @@ export const Timeline = memo(function Timeline({
   const selectedKeyframes = usePlayerStore((s) => s.selectedKeyframes);
   const toggleSelectedKeyframe = usePlayerStore((s) => s.toggleSelectedKeyframe);
 
+  const { onClickKeyframe, onShiftClickKeyframe, onContextMenuKeyframe } =
+    useTimelineKeyframeHandlers({
+      expandedElements,
+      keyframeCache,
+      onSelectElement,
+      onSeek,
+      setSelectedElementId,
+      setKfContextMenu,
+      toggleSelectedKeyframe,
+    });
+
   const selectedElement = useMemo(
     () =>
       expandedElements.find((element) => (element.key ?? element.id) === selectedElementId) ?? null,
@@ -496,40 +508,10 @@ export const Timeline = memo(function Timeline({
           selectedKeyframes={selectedKeyframes}
           currentTime={currentTime}
           beatAnalysis={adjustedBeatAnalysis}
-          onClickKeyframe={(el, pct) => {
-            usePlayerStore.getState().clearSelectedKeyframes();
-            const elKey = el.key ?? el.id;
-            setSelectedElementId(elKey);
-            onSelectElement?.(el);
-            // Select the clicked diamond (matches shift-click); cleared above so this single-selects.
-            toggleSelectedKeyframe(`${elKey}:${pct}`);
-            const absTime = el.start + (pct / 100) * el.duration;
-            onSeek?.(absTime);
-            const kfData = keyframeCache?.get(elKey);
-            const kf = kfData?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.5);
-            usePlayerStore.getState().setActiveKeyframePct(kf?.tweenPercentage ?? null);
-          }}
-          onShiftClickKeyframe={(elId, pct) => {
-            toggleSelectedKeyframe(`${elId}:${pct}`);
-          }}
+          onClickKeyframe={onClickKeyframe}
+          onShiftClickKeyframe={onShiftClickKeyframe}
           onMoveKeyframe={onMoveKeyframe}
-          onContextMenuKeyframe={(e, elId, pct) => {
-            const el = expandedElements.find((x) => (x.key ?? x.id) === elId);
-            if (!el) return;
-            setSelectedElementId(elId);
-            onSelectElement?.(el);
-            const kfData = keyframeCache.get(elId);
-            const kf = kfData?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.2);
-            setKfContextMenu({
-              x: e.clientX + 4,
-              y: e.clientY + 2,
-              element: el,
-              elementId: elId,
-              percentage: pct,
-              tweenPercentage: kf?.tweenPercentage,
-              currentEase: kf?.ease ?? kfData?.ease,
-            });
-          }}
+          onContextMenuKeyframe={onContextMenuKeyframe}
           onContextMenuClip={(e, el) => {
             e.preventDefault();
             setSelectedElementId(el.key ?? el.id);

packages/studio/src/player/components/TimelineClipDiamonds.test.tsx

@@ -4,6 +4,7 @@ import React, { act } from "react";
 import { createRoot } from "react-dom/client";
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { TimelineClipDiamonds, TimelineDiamondLane } from "./TimelineClipDiamonds";
+import { timelineKeyframeSelectionKey } from "./timelineKeyframeIdentity";
 
 (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
 
@@ -90,7 +91,57 @@ describe("TimelineClipDiamonds", () => {
       diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0 }));
     });
 
-    expect(onClickKeyframe).toHaveBeenCalledWith(50);
+    expect(onClickKeyframe).toHaveBeenCalledWith(
+      "clip-1",
+      expect.objectContaining({ percentage: 50 }),
+    );
+    act(() => root.unmount());
+  });
+
+  // The collapsed clip row and the expanded property lanes read the same cache,
+  // so a keyframe that carries a property group has to hash to the same key in
+  // both — otherwise collapsing a track silently drops the selection.
+  it("keys a grouped keyframe the same way collapsed as expanded", () => {
+    const groupedKeyframe = {
+      percentage: 50,
+      tweenPercentage: 25,
+      propertyGroup: "position",
+      animationId: "anim-1",
+      properties: { x: 100 },
+    };
+    const sharedKey = timelineKeyframeSelectionKey("clip-1", groupedKeyframe);
+    const host = document.createElement("div");
+    document.body.append(host);
+    const root = createRoot(host);
+    const onClickKeyframe = vi.fn();
+    act(() => {
+      root.render(
+        <TimelineClipDiamonds
+          keyframesData={{ format: "percentage", keyframes: [groupedKeyframe] }}
+          clipWidthPx={200}
+          clipHeightPx={48}
+          accentColor="#4ba3d2"
+          isSelected
+          currentPercentage={-10}
+          elementId="clip-1"
+          selectedKeyframes={new Set([sharedKey])}
+          onClickKeyframe={onClickKeyframe}
+        />,
+      );
+    });
+    const diamond = host.querySelector<HTMLButtonElement>('button[title="50%"]');
+    // Highlighted from the shared key alone (the playhead is off-clip here).
+    expect(diamond?.querySelector("path:last-child")?.getAttribute("fill")).toBe("#4ba3d2");
+
+    act(() => {
+      diamond?.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0 }));
+    });
+    expect(onClickKeyframe).toHaveBeenCalledWith("clip-1", {
+      percentage: 50,
+      tweenPercentage: 25,
+      propertyGroup: "position",
+      animationId: "anim-1",
+    });
     act(() => root.unmount());
   });
 

packages/studio/src/player/components/TimelineClipDiamonds.tsx

@@ -44,9 +44,13 @@ interface TimelineClipDiamondsProps {
   currentPercentage: number;
   elementId: string;
   selectedKeyframes: ReadonlySet<string>;
-  onClickKeyframe?: (percentage: number) => void;
-  onShiftClickKeyframe?: (elementId: string, percentage: number) => void;
-  onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void;
+  onClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
+  onShiftClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
+  onContextMenuKeyframe?: (
+    e: React.MouseEvent,
+    elementId: string,
+    keyframe: TimelineKeyframeTarget,
+  ) => void;
   /** Drag-to-retime: move a keyframe to a new time, preserving its value + ease.
    *  `keyframe` identifies the dragged keyframe (clip-relative percentage plus
    *  whatever animation identity the row carries); `toClipPercentage` is the
@@ -105,18 +109,21 @@ type DragState = {
   cancelled?: boolean;
 };
 
-function keyframeTarget(
-  keyframe: TimelineDiamondKeyframe,
-  groupAware: boolean,
-): TimelineKeyframeTarget {
-  return groupAware
-    ? {
-        percentage: keyframe.percentage,
-        tweenPercentage: keyframe.tweenPercentage,
-        propertyGroup: keyframe.propertyGroup,
-        animationId: keyframe.animationId,
-      }
-    : { percentage: keyframe.percentage };
+/**
+ * The full identity of a diamond, used by every callback and by the selection
+ * key. Collapsed clip rows and expanded property lanes read the same cache, so
+ * they must hash a shared keyframe to the same key: dropping the group here for
+ * the collapsed row would leave a diamond selected in one view and unselected in
+ * the other, and would strip the animation id the retime/delete mutations use to
+ * pick between two animations that collide at one percentage.
+ */
+function keyframeTarget(keyframe: TimelineDiamondKeyframe): TimelineKeyframeTarget {
+  return {
+    percentage: keyframe.percentage,
+    tweenPercentage: keyframe.tweenPercentage,
+    propertyGroup: keyframe.propertyGroup,
+    animationId: keyframe.animationId,
+  };
 }
 
 export const TimelineDiamondLane = memo(function TimelineDiamondLane({
@@ -216,16 +223,21 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
   // Clip-%s of the sorted keyframes — the neighbour clamp (preview + drop) needs
   // the whole row to bound the dragged diamond between its immediate siblings.
   const sortedClipPcts = sorted.map((k) => k.percentage);
-  const sortedCenterXs = sorted.map((keyframe) =>
-    Math.max(0, Math.min(clipWidthPx, (keyframe.percentage / 100) * clipWidthPx)),
-  );
-  const markerMetrics = sortedCenterXs.map((centerX, index) => {
-    const previousGap = index > 0 ? centerX - sortedCenterXs[index - 1]! : Infinity;
-    const nextGap =
-      index < sortedCenterXs.length - 1 ? sortedCenterXs[index + 1]! - centerX : Infinity;
+  const centerXOf = (percentage: number) =>
+    Math.max(0, Math.min(clipWidthPx, (percentage / 100) * clipWidthPx));
+  // One record per diamond, carrying its own geometry, so the connector and
+  // button passes below read neighbours as values instead of index lookups.
+  const markers = sorted.map((keyframe, index) => {
+    const centerX = centerXOf(keyframe.percentage);
+    const previous = sorted[index - 1];
+    const next = sorted[index + 1];
+    const previousGap = previous ? centerX - centerXOf(previous.percentage) : Infinity;
+    const nextGap = next ? centerXOf(next.percentage) - centerX : Infinity;
     const nearestGap = Math.max(1, Math.min(previousGap, nextGap));
     const hitWidth = Math.min(diamondSize, nearestGap);
     return {
+      keyframe,
+      centerX,
       hitWidth,
       visualSize: hitWidth === diamondSize ? diamondSize : Math.max(2, hitWidth - 2),
     };
@@ -248,25 +260,24 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
         pointerEvents: "none",
       }}
     >
-      {sorted.map((kf, i) => {
-        if (i === 0) return null;
-        const prev = sorted[i - 1]!;
-        const x1 = sortedCenterXs[i - 1]!;
-        const x2 = sortedCenterXs[i]!;
+      {markers.map((marker, i) => {
+        const previous = markers[i - 1];
+        if (!previous) return null;
+        const kf = marker.keyframe;
+        const x1 = previous.centerX;
+        const x2 = marker.centerX;
         if (x2 - x1 < 1) return null;
-        const connectorLeft = x1 + markerMetrics[i - 1]!.visualSize / 2;
-        const connectorWidth =
-          x2 - x1 - markerMetrics[i - 1]!.visualSize / 2 - markerMetrics[i]!.visualSize / 2;
-        // Group-aware target for the ease button: the segment ease is
-        // per-keyframe (each keyframe carries its own animationId/tweenPercentage).
-        // On a merged inline row the button is hidden where the segment is
-        // ambiguous (two source animations collide at this % with different
-        // eases; see easeAmbiguous) or the keyframe has no source animation id
-        // (runtime-scanned) so there is no tween to target.
-        const target = keyframeTarget(kf, true);
+        const connectorLeft = x1 + previous.visualSize / 2;
+        const connectorWidth = x2 - x1 - previous.visualSize / 2 - marker.visualSize / 2;
+        // The ease button targets one segment, so it needs the keyframe's own
+        // animationId/tweenPercentage. On a merged inline row the button is
+        // hidden where the segment is ambiguous (two source animations collide
+        // at this % with different eases; see easeAmbiguous) or the keyframe has
+        // no source animation id (runtime-scanned) so there is no tween to target.
+        const target = keyframeTarget(kf);
         const ease = kf.ease ?? globalEase;
         return (
-          <Fragment key={`line-${i}-${prev.percentage}-${kf.percentage}`}>
+          <Fragment key={`line-${i}-${previous.keyframe.percentage}-${kf.percentage}`}>
             <div
               className="absolute"
               data-keyframe-connector={groupAware ? "" : undefined}
@@ -334,8 +345,9 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
         );
       })}
 
-      {sorted.map((kf, i) => {
-        const target = keyframeTarget(kf, groupAware);
+      {markers.map((marker, i) => {
+        const kf = marker.keyframe;
+        const target = keyframeTarget(kf);
         const kfKey = timelineKeyframeSelectionKey(elementId, target);
         // While dragging this diamond, render it at the live preview clip-%.
         const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage;
@@ -344,7 +356,6 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
         // The 0% diamond's left half lands in the reserved left gutter (the
         // content origin is inset past the label column, Figma-style) so it stays
         // fully visible instead of being clipped by the sticky label column.
-        const marker = markerMetrics[i]!;
         const leftPx = (renderPct / 100) * clipWidthPx - marker.hitWidth / 2;
         const isKfSelected = selectedKeyframes.has(kfKey);
         const atPlayhead = isSelected && Math.abs(kf.percentage - currentPercentage) < 0.5;
@@ -565,12 +576,10 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds(
     <TimelineDiamondLane
       {...props}
       globalEase={props.keyframesData.ease}
-      onClickKeyframe={(target) => props.onClickKeyframe?.(target.percentage)}
-      onShiftClickKeyframe={(target) =>
-        props.onShiftClickKeyframe?.(props.elementId, target.percentage)
-      }
+      onClickKeyframe={(target) => props.onClickKeyframe?.(props.elementId, target)}
+      onShiftClickKeyframe={(target) => props.onShiftClickKeyframe?.(props.elementId, target)}
       onContextMenuKeyframe={(e, target) =>
-        props.onContextMenuKeyframe?.(e, props.elementId, target.percentage)
+        props.onContextMenuKeyframe?.(e, props.elementId, target)
       }
       onMoveKeyframe={
         props.onMoveKeyframe

packages/studio/src/player/components/TimelineLanes.tsx

@@ -72,9 +72,13 @@ export interface TimelineLaneBaseProps {
   keyframeCache?: Map<string, KeyframeCacheEntry>;
   selectedKeyframes: Set<string>;
   currentTime: number;
-  onClickKeyframe?: (element: TimelineElement, percentage: number) => void;
-  onShiftClickKeyframe?: (elementId: string, percentage: number) => void;
-  onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void;
+  onClickKeyframe?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void;
+  onShiftClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
+  onContextMenuKeyframe?: (
+    e: React.MouseEvent,
+    elementId: string,
+    keyframe: TimelineKeyframeTarget,
+  ) => void;
   onMoveKeyframe?: (
     elementId: string,
     keyframe: TimelineKeyframeTarget,
@@ -474,7 +478,9 @@ export function TimelineLanes({
                             }
                             elementId={elementKey}
                             selectedKeyframes={selectedKeyframes}
-                            onClickKeyframe={(pct) => onClickKeyframe?.(previewElement, pct)}
+                            onClickKeyframe={(_elId, keyframe) =>
+                              onClickKeyframe?.(previewElement, keyframe)
+                            }
                             onShiftClickKeyframe={onShiftClickKeyframe}
                             onContextMenuKeyframe={onContextMenuKeyframe}
                             onMoveKeyframe={onMoveKeyframe}

packages/studio/src/player/components/TimelinePropertyLanes.test.tsx

@@ -424,7 +424,10 @@ describe("TimelinePropertyLanes", () => {
     act(() => {
       diamonds[1]?.dispatchEvent(new MouseEvent("pointerup", { bubbles: true, button: 0 }));
     });
-    expect(onClickKeyframe).toHaveBeenCalledWith(50);
+    expect(onClickKeyframe).toHaveBeenCalledWith(
+      "clip-1",
+      expect.objectContaining({ percentage: 50 }),
+    );
     act(() => root.unmount());
   });
 });