fix(studio): close the review findings that survived the stack

Selector reads now go through one inverse of `idSelector`. Every writer emits
`[id="01-hook-hero"]` for an id a `#id` selector can't address, but the readers
still matched `#id` only, so the post-commit keyframe-cache refresh, the AST
load and the remove-all-keyframes clear all silently skipped exactly the ids
`idSelector` was added to support.

A keyframe merged from two tweens with different eases kept whichever ease
iterated last. Readers that don't check `easeAmbiguous` showed a curve from a
different animation than an edit would target, so the ambiguous flag now clears
`ease` instead of leaving an arbitrary one behind.

One tolerance for "the playhead is on this keyframe". The motion-path drag used
0.05% while the toolbar and the playhead apply used 1%, so a drag that landed a
fraction of a percent off an authored waypoint skipped the update-point branch
and appended a near-duplicate. `buildTemporalArcKeyframes` now owns the
invariant and replaces any keyframe inside the tolerance, rather than trusting
each caller's own pre-check.

The pending-retime bookkeeping matches on keyframe identity, not just on
"something is near that percentage" — an evenly spaced row cleared the entry off
an unrelated sibling. The neighbour clamp composes pending destinations in
before sorting, so a second drag can't cross a neighbour that already moved.

Also: `keyframeCache`/`gsapAnimations` setters return the same state for a write
that changes nothing (every no-op re-rendered every subscriber), the auto-expand
set drops clips that left the source so an undo/paste under the same id expands
again, `invalidateGsapCache` has a stable identity instead of re-creating the
whole timeline edit context each render, the studio test hook deletes its window
key rather than leaving it enumerable as undefined, and the past-last-row
extrapolation documents why it uses TRACK_H where the pre-first-row branch uses
row 0's own height.

Covers `idFromSelector` round-trips, the insert boundary band across plain,
expanded and unusable row heights, and the collapsed selection key for a
colon-bearing element id.
This commit is contained in:
Miguel Angel Simon Sierra
2026-07-28 00:40:46 +02:00
parent 675cbe194d
commit b8ff8bf0f3
12 changed files with 136 additions and 42 deletions
@@ -58,17 +58,29 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
// Pending retime destination (clip + tween %) per keyframe key, so a rapid
// 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 }>());
const pendingRetimeRef = useRef<Map<string, { clipPct: number; tweenPct: number }> | null>(null);
// Lazy: `useRef(new Map())` allocates a Map on every render and throws all but
// the first away, once per mounted lane.
pendingRetimeRef.current ??= new Map();
const pendingRetimes = pendingRetimeRef.current;
useEffect(() => {
// Clear a pending entry once the authoritative cache reflects a keyframe at
// ~its destination. Match by tolerance, not equality: cache writers round
// clip %s, so an exact check would leak an entry after every successful retime.
for (const [key, pending] of pendingRetimeRef.current) {
if (keyframesData.keyframes.some((k) => Math.abs(k.percentage - pending.clipPct) < 0.2)) {
pendingRetimeRef.current.delete(key);
}
// Clear a pending entry once the authoritative cache reflects THAT keyframe
// at ~its destination. Match by tolerance, not equality: cache writers round
// clip %s, so an exact check would leak an entry after every successful
// retime. Match by identity too: a bare "some keyframe is near that %" test
// cleared the entry whenever an unrelated sibling happened to sit there,
// which is easy to hit on an evenly spaced row.
const pendingEntries = pendingRetimeRef.current;
if (!pendingEntries) return;
for (const [key, pending] of pendingEntries) {
const settled = keyframesData.keyframes.some(
(k) =>
timelineKeyframeSelectionKey(elementId, keyframeTarget(k)) === key &&
Math.abs(k.percentage - pending.clipPct) < 0.2,
);
if (settled) pendingEntries.delete(key);
}
}, [keyframesData.keyframes]);
}, [keyframesData.keyframes, elementId]);
// Visual-only preview of the dragged diamond's clip-% — no runtime/GSAP hold
// (that optimistic hold was the #1763 flake). The atomic move-keyframe commit
// on drop re-keys the diamond from source.
@@ -190,9 +202,19 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
const target = keyframeTarget(kf);
const kfKey = timelineKeyframeSelectionKey(elementId, target);
// Clamp against this keyframe's own tween, not the whole merged row.
const siblingRow = siblingRowOf(kf);
const siblingClipPcts = siblingRow.map((k) => k.percentage);
const siblingIndex = siblingRow.indexOf(kf);
// Compose each sibling's pending destination in first: clamping against
// cached positions while the dragged keyframe reads its pending one let
// a second drag cross a neighbour that had already moved past it.
const siblingRow = siblingRowOf(kf)
.map((k) => ({
keyframe: k,
clipPct:
pendingRetimes.get(timelineKeyframeSelectionKey(elementId, keyframeTarget(k)))
?.clipPct ?? k.percentage,
}))
.sort((a, b) => a.clipPct - b.clipPct);
const siblingClipPcts = siblingRow.map((s) => s.clipPct);
const siblingIndex = siblingRow.findIndex((s) => s.keyframe === kf);
// While dragging this diamond, render it at the live preview clip-%.
const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage;
// Center the marker's non-overlapping hit region ON its keyframe %, so
@@ -216,7 +238,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
startX: e.clientX,
lastX: e.clientX,
index: siblingIndex,
fromClipPct: pendingRetimeRef.current.get(kfKey)?.clipPct ?? kf.percentage,
fromClipPct: pendingRetimes.get(kfKey)?.clipPct ?? kf.percentage,
moved: false,
};
}
@@ -309,7 +331,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
// For a rapid second retime the diamond still renders the stale cache
// position, so identify the FROM keyframe by the pending (already-moved)
// position; the mutation locates the source keyframe by this identity.
const pendingBefore = pendingRetimeRef.current.get(kfKey);
const pendingBefore = pendingRetimes.get(kfKey);
const fromTarget = pendingBefore
? {
...target,
@@ -318,10 +340,10 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
}
: target;
const pending = { clipPct: res.toClipPct, tweenPct: newTweenPct };
pendingRetimeRef.current.set(kfKey, pending);
pendingRetimes.set(kfKey, pending);
const clearPending = () => {
if (pendingRetimeRef.current.get(kfKey) === pending) {
pendingRetimeRef.current.delete(kfKey);
if (pendingRetimes.get(kfKey) === pending) {
pendingRetimes.delete(kfKey);
}
};
// A rejected drop (the destination time is already occupied) snaps
@@ -32,6 +32,13 @@ describe("timeline keyframe selection identity", () => {
expect(timelineKeyframeTargetFromSelectionKey("comp#a", key)).toBeNull();
});
// Timeline.tsx still writes the collapsed form for clip-lane shift-clicks, and
// an element id can itself contain a colon — the split has to be the LAST one.
it("splits the collapsed key at the last colon so a colon-bearing id survives", () => {
expect(timelineKeyframeTargetFromSelectionKey("a:b", "a:b:40")).toEqual({ percentage: 40 });
expect(timelineKeyframeTargetFromSelectionKey("a", "a:b:40")).toBeNull();
});
it("retains the collapsed key fallback and rejects malformed percentages", () => {
expect(timelineKeyframeTargetFromSelectionKey("comp#a", "comp#a:30")).toEqual({
percentage: 30,
@@ -1,5 +1,8 @@
import { describe, it, expect } from "vitest";
import {
CLIP_Y,
INSERT_BOUNDARY_BAND,
getTimelineInsertBoundaryBand,
RULER_H,
TRACK_H,
LANE_H,
@@ -225,6 +228,7 @@ describe("getTimelineScrubTime", () => {
clientX: 500,
viewportLeft: 0,
scrollLeft: 0,
contentOrigin: GUTTER + TRACKS_LEFT_PAD,
pixelsPerSecond: 0,
duration: 10,
}),
@@ -232,3 +236,25 @@ describe("getTimelineScrubTime", () => {
expect(at(origin + 250, Number.NaN)).toBe(0);
});
});
// The only production hook keeping resolveInsertRow's band aligned with the
// rendered clip inset once rows can be taller than TRACK_H. Pinned directly so a
// change to CLIP_Y or the invalid-height fallback can't silently drift it.
describe("getTimelineInsertBoundaryBand", () => {
it("matches the fixed band for a plain track row", () => {
expect(getTimelineInsertBoundaryBand(TRACK_H)).toBe(INSERT_BOUNDARY_BAND);
expect(getTimelineInsertBoundaryBand(TRACK_H)).toBe(CLIP_Y / TRACK_H);
});
it("shrinks as the row grows, so the band stays CLIP_Y pixels tall", () => {
const expanded = TRACK_H + 2 * LANE_H;
expect(getTimelineInsertBoundaryBand(expanded)).toBeCloseTo(CLIP_Y / expanded, 10);
expect(getTimelineInsertBoundaryBand(expanded)).toBeLessThan(INSERT_BOUNDARY_BAND);
});
it("falls back to the plain-track band for a height that is not usable", () => {
for (const height of [0, -10, Number.NaN]) {
expect(getTimelineInsertBoundaryBand(height)).toBe(INSERT_BOUNDARY_BAND);
}
});
});
@@ -104,6 +104,10 @@ function getTimelineRowOffset(row: number, rowHeights: readonly number[]): numbe
const offsets = getTimelineRowOffsets(rowHeights);
if (row <= 0) return row * getTimelineRowHeight(0, rowHeights);
if (row >= rowHeights.length) {
// Deliberately TRACK_H, not the last row's height: rows past the end do not
// exist yet, and a row created by dropping there starts unexpanded. The
// pre-first-row branch above uses row 0's concrete height instead because
// that row DOES exist — the pointer is in the top pad above a real lane.
return (offsets[rowHeights.length] ?? 0) + (row - rowHeights.length) * TRACK_H;
}
const wholeRow = Math.floor(row);
@@ -24,6 +24,12 @@ export function useAutoExpandKeyframedClips(gsapAnimations: Map<string, GsapAnim
} else {
seen.current.source = gsapAnimations;
}
// Drop clips that are no longer in the source at all. Without this the set
// is append-only, so a clip deleted and reinserted under the same id (undo,
// paste) is remembered as already-expanded and never auto-expands again.
for (const key of seen.current.clips) {
if (!gsapAnimations.has(key)) seen.current.clips.delete(key);
}
const fresh: string[] = [];
for (const [key, animations] of gsapAnimations) {
if (seen.current.clips.has(key)) continue;