fix(studio): target colliding keyframes exactly

[miguel-heygen]

What

Make merged timeline diamonds preserve the exact animation and tween identities they represent.

Why

Multiple animations can share an element, property, and percentage. Resolving edits from only those display values can target the wrong authored tween, which makes selection, deletion, and easing mutations nondeterministic.

How

• Carry stable animation/tween identities through merged keyframe selections.
• Resolve timeline operations against those exact identities.
• Keep property-lane and merged-diamond selection semantics aligned.

This is C1 of the independent Family C draft Graphite stack. Its review base is the immutable, PR-less Family B baseline; Family B does not need to merge for this PR to be reviewed.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Validated on the exact Family C tip with 94 focused tests, the full Studio suite (2,885 passed; 18 todos), Studio Server files tests (67 passed), both package typechecks, oxfmt, oxlint, diff checks, file-size gates, and Fallow with zero introduced findings.

[miguel-heygen]

• fix(studio): switch keyframe ease modes optimistically #2695
• fix(studio): publish keyframe cache refresh atomically #2694
• feat(studio): bulk-edit easing for merged keyframes #2693
• fix(studio): target colliding keyframes exactly #2692 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewed at c02a44c.

Clean foundation refactor — replacing the arbitrary easeAmbiguous boolean with an ordered collidingAnimationTargets list is exactly the right shape for the C2 bulk-edit feature. Migration is complete (grepped: the only remaining easeAmbiguous reference is a regression-guard test at gsapKeyframeCacheHelpers.test.ts:365, and everything downstream — TimelineDiamondKeyframe, keyframeSlice.focusedEaseSegment, keyframeTarget(), useTimelineKeyframeHandlers — is threaded through consistently). Tests exercise first-seen order, dedup by animationId, and the "single animation → no collisions" case.

No blockers. Two concerns inline (one about the intermediate stack-only state, one about a tweenPercentage guard edge case), plus a couple of nits below.

Nits (not worth inline comments):

• The existing regression-guard test at gsapKeyframeCacheHelpers.test.ts:365 (expect(parentKeyframes.some((keyframe) => "easeAmbiguous" in keyframe)).toBe(false)) will silently keep passing forever now that the field is gone from the type system. If it's meant to guard against reintroducing the old boolean, a code comment saying so would be helpful; otherwise it's dead assertion weight.
• The prior code around deduplicateKeyframes had a rich comment explaining WHY the "arbitrary winner ease" was dropped (if (existing.easeAmbiguous) delete existing.ease). The new code silently keeps last-write-wins ease (if (kf.ease) existing.ease = kf.ease;) at gsapTweenSynth.ts:71, which is only safe because C2 will apply the same ease to every colliding target. A one-line comment noting "arbitrary winner is fine now — C2 bulk-edit applies the same curve to every colliding tween" would keep the invariant discoverable when someone reads C1 alone.

What I didn't verify:

• Whether any runtime code path can emit a same-clip-% keyframe with animationId defined but tweenPercentage undefined. See the inline concern on the guard at line 41.

— Review by Rames D Jusso

[vanceingalls]

R1 review — via

Grade: B+
Overall: PARTIAL (correct plumbing; the "bulk-edit" behavior asserted in the diff comments only lands with C2 downstream — C1 alone is a UX regression on colliding merged rows)

Thesis check: The PR is titled "target colliding keyframes exactly." What it actually does at C1 is carry per-collision identity through the merged-keyframe pipeline: replaces the boolean easeAmbiguous flag with a collidingAnimationTargets: AnimationKeyframeTarget[] array populated by a new accumulateCollidingAnimationTargets helper inside deduplicateKeyframes (packages/studio/src/hooks/gsapTweenSynth.ts:30-56), then threads that field through TimelineDiamondKeyframe → keyframeTarget() → TimelineKeyframeTarget → focusedEaseSegment (timelineDiamondTypes.ts:18,119, timelineKeyframeIdentity.ts:8, useTimelineKeyframeHandlers.ts:68, keyframeSlice.ts:19,42,49). The collision class targeted is cross-animation same-clip-% collision (multiple GSAP tweens landing on the same merged diamond). The exact-identity design is sound.

P0/P1 findings:

• [P1] packages/studio/src/player/components/TimelineDiamondConnectors.tsx:87 + packages/studio/src/components/editor/AnimationCard.tsx:26,64,292-295 + GsapAnimationSection.tsx:58 + propertyPanelFlatMotionSection.tsx:184 — the "the button now bulk-edits both rather than being hidden" behavior asserted in the test comment at TimelineClipDiamonds.test.tsx:672-675 and the "resolve edits from only those display values can target the wrong authored tween" claim in the PR body is not implemented in this PR. AnimationCard.focusedSegment is typed { tweenPercentage: number } | null — it never reads collidingAnimationTargets. The card opens only for the AnimationCard whose id matches focusedEaseSegment.animationId (the arbitrary primary — whichever tween iterated first in deduplicateKeyframes and won existing.animationId). Its ease commit at line 292 calls onUpdateKeyframeEase(animation.id, pct, ease) — one animation. Net effect at C1 head: the ease button on a colliding merged segment now shows (previously hidden by the !kf.easeAmbiguous guard, TimelineDiamondConnectors.tsx old L58) but clicking it edits the ease of exactly one arbitrary animation. That is precisely the "nondeterministic easing mutation" bug the PR body says it fixes. C2 (PR #2693, feat(studio): bulk-edit easing for merged keyframes) is where the actual consumer lands. Recommend either landing C1+C2 atomically or gating the show-button behavior on a downstream check.
  • Failure scenario: user has two GSAP tweens on #hero colliding at clip% 50 with different eases (power2.in and power2.out); the merged diamond shows one button. User clicks, ease editor opens on hero-position, user sets ease to sine.inOut. hero-visual's tween keeps power2.out. User sees the merged diamond's curve display flip to sine.inOut (because existing.ease was overwritten last), but the runtime animation still plays two different curves on the two properties. Now indistinguishable from a "worked" edit.

P2/P3 findings:

• [P2] packages/studio/src/hooks/gsapTweenSynth.ts:71 — if (kf.ease) existing.ease = kf.ease; replaces the previous "delete ease when ambiguous" honesty. Before the PR, on a colliding merge existing.ease was deleted so TimelineDiamondConnectors.tsx:64 const ease = kf.ease ?? globalEase; fell back to globalEase — a deterministic display. Now the merged diamond's rendered MiniCurveSvg shows whichever ease the last-iterated colliding tween carried — arbitrary. Even after C2 makes the button bulk-edit, the displayed curve preview on a colliding segment with mixed source eases has no single truthful value; a "mixed" indicator (or the old globalEase fallback when collidingAnimationTargets is set) would be more honest than an arbitrary curve.
• [P3] packages/studio/src/hooks/gsapTweenSynth.ts:39-46 — accumulateCollidingAnimationTargets short-circuits when primaryId === incoming.animationId, but the outer deduplicateKeyframes still merges existing.properties and overwrites existing.ease from the same-animation second tween. If a single animation has two tweens landing on the same clip% (possible via the Math.round(... * 100000) / 1000 rounding in toClipPercentage, gsapShared.ts:540), the merged row's tweenPercentage stays pointed at the first tween. Ease edits and delete-by-target then reach only the first internal tween; the second is silently orphaned. Not new to this PR (pre-existing rounding), but the identity-first framing of this PR is exactly the place to note that within-animation collision has the same "target one arbitrary" property.

Nits:

• packages/studio/src/hooks/gsapTweenSynth.ts:30 — accumulateCollidingAnimationTargets is exported but has no external call site (only used at line 70 in the same file). Fallow flagged this at major severity; the Fallow audit CI check on this PR is FAILURE for exactly this. Either drop the export or lift it to a named callable used from the tests.
• packages/studio/src/player/components/useTimelineKeyframeHandlers.test.tsx:54-89 — the two new tests (the pre-existing "tracks opening..." and the added "focuses a merged segment with its colliding animation targets") duplicate a 16-line function Harness() block. Fallow flagged this. Extract a small mountHarness() helper.
• packages/studio/src/hooks/gsapTweenSynth.ts:50-55 — the "seed the list with the primary on first collision" branch is inlined and a little dense. A named local like const seeded = collisionTargets?.length ? collisionTargets : [{ animationId: primaryId, tweenPercentage: keyframe.tweenPercentage! }]; would read cleaner.

Positive callouts:

• Single-owner discipline: consolidating dedup into deduplicateKeyframes and removing the sibling byPct writer in gsapKeyframeCacheHelpers.ts closes the drift the comment update calls out. Same-primitive fan-in is exactly the right shape.
• Test suite proves the invariants that matter for C1 in isolation: first-seen order preserved (gsapTweenSynth.test.ts "records each animation's tween percentage in first-seen order"), 3-way collision dedup ("deduplicates three colliding animations while preserving first-seen order"), within-animation exclusion ("leaves the collision set undefined within a single animation"), and end-to-end propagation through the cache writer (gsapKeyframeCacheHelpers.test.ts "records colliding animation targets with their own tween percentages") and the click handler (useTimelineKeyframeHandlers.test.tsx "focuses a merged segment with its colliding animation targets").
• Type refactor is total: no lingering easeAmbiguous at the PR head (verified via HEAD-SHA fetch).

Sweep-fix breadth check:
Peer sites doing same-% keyframe dedup / merge in the studio tree:

• packages/studio/src/hooks/gsapTweenSynth.ts deduplicateKeyframes — covered (primary site).
• packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts (previous sibling byPct writer) — covered by removal on this PR head; now delegates to deduplicateKeyframes.
• packages/studio/src/hooks/useGsapTweenCache.ts:410,519 — two more deduplicateKeyframes call sites, both automatically inherit the new collision-list semantics.
  Peer sites doing keyframe-target resolution that could target the arbitrary primary on merged rows (out of scope for C1 but worth naming for the stack):
• packages/studio/src/components/nle/useTimelineEditCallbacks.ts:66-87 resolveTimelineKeyframeTarget — returns { animId, tweenPct } (single) from kf.animationId; consumed by delete + move keyframe callbacks. Deletion on a colliding merged row will still remove one arbitrary tween. Not a new regression (pre-existing) but the "selection, deletion, and easing mutations" list in the PR body implies scope beyond easing; deletion still targets the arbitrary primary. Follow-up ticket recommended.
• Peer sites MISSED that C1 should have touched: none — the plumbing site set is complete for the "carry identity through" claim.

Standards-lens mechanical:

• Named exports only: pass (AnimationKeyframeTarget, accumulateCollidingAnimationTargets, no default exports introduced).
• File-size cap: pass (all touched files well under 600 lines).
• import type: pass (import type { AnimationKeyframeTarget } from "../../hooks/gsapTweenSynth"; in timelineDiamondTypes.ts:7, timelineKeyframeIdentity.ts:1, keyframeSlice.ts:3).
• No useEffect for state syncing: pass (no new effects added; AnimationCard's existing focus effect is unchanged).
• Fast Refresh — component files export only components: pass (no new helper exports from .tsx component files; the helper lives in gsapTweenSynth.ts).
• List virtualization: N/A (no unbounded scroll containers added).
• Fallow audit: FAIL — 1 major (unused export accumulateCollidingAnimationTargets) + 2 minor (test-file duplication).

Collision-predicate audit:

• OLD predicate (gsapTweenSynth.ts old L23-L36): if (existing.animationId !== undefined && kf.animationId !== undefined && existing.animationId !== kf.animationId) { existing.easeAmbiguous = true; } — plus a downstream if (existing.easeAmbiguous) delete existing.ease;. Cross-animation collision recorded as a single boolean, ease dropped, button hidden downstream.
• NEW predicate (packages/studio/src/hooks/gsapTweenSynth.ts:38-55): accumulateCollidingAnimationTargets(existing, kf) — early-out on primaryId === undefined || keyframe.tweenPercentage === undefined || incoming.animationId === undefined || incoming.tweenPercentage === undefined || primaryId === incoming.animationId, then dedupe-by-animationId inside the accumulated list, then append the incoming target (seeding the list with the primary on first collision). Ease overwrite unconditional (if (kf.ease) existing.ease = kf.ease;, line 71).
• Break assessment:
  • Merged-keyframe case (same time, different properties): dedup preserves properties merge and now records identity for both — correct.
  • Same-animation multi-tween at same clip%: primaryId === incoming.animationId skips accumulation but outer dedup still merges properties + overwrites ease. Row's tweenPercentage stays pinned to first-seen — pre-existing sharp edge, not regressed.
  • Nearby-but-not-colliding (different clip% after rounding): byPct.get(kf.percentage) misses, incoming becomes its own row — unchanged.
  • Floating-point time: comparison is exact via Map<number, T> key equality on kf.percentage values pre-rounded by toClipPercentage (Math.round(... * 100000) / 1000, gsapShared.ts:540). Deterministic if inputs are consistent — no float-tolerance issue in the predicate itself.

— Review by Via

[vanceingalls]

R2 review — via

HEAD verified: 7e6b3e64258d72ee6831842450c0fe114d2c6ca4 (R1 was at c02a44c, force-pushed 22:01Z + 22:34Z on 2026-07-28).
Grade: A (up from R1 B+)
Overall: APPROVE — all R1 P1/P2 concerns closed; Rames' tweenPercentage-guard concern closed at a stronger level than requested (type enforcement, not runtime guard).

R1 findings delta:

• [P1 stack-shape mid-stack landing]: RESOLVED at gsapTweenSynth.ts:78 + TimelineDiamondConnectors.tsx:87-89. The inline ease button is now gated on (kf.collidingAnimationTargets?.length ?? 0) <= 1 && kf.animationId !== undefined. Because accumulateCollidingAnimationTargets seeds the list with the primary on first collision (line 46-50), a real cross-animation collision always yields length >= 2, so the button hides — matching Rames' explicit suggestion in the R1 inline. C1 no longer regresses UX mid-stack if #2693 is delayed.
• [P2 last-write-wins ease]: RESOLVED at gsapTweenSynth.ts:78. if ((existing.collidingAnimationTargets?.length ?? 0) > 1) delete existing.ease; restores the honest "drop ease on collision" behaviour, so MiniCurveSvg falls back to globalEase on colliding segments instead of showing an arbitrary curve.
• [Rames' P2 — tweenPercentage short-circuit guard]: RESOLVED at a stronger level than the inline suggested. Instead of adding a targeted test or inverting the guard, Miguel introduced SourcedGsapPercentageKeyframe in packages/parsers/src/gsapSerialize.ts:104-107 with both animationId: string and tweenPercentage: number REQUIRED, then narrowed deduplicateKeyframes<T extends MergeableKeyframe> and the callsite type at useGsapTweenCache.ts:270. The runtime guards (existing.tweenPercentage === undefined, incoming.tweenPercentage === undefined) are gone from gsapTweenSynth.ts:30-52 because they can't fire — the type refuses an incomplete keyframe at the merge boundary. Every push site now must provide both (useGsapTweenCache.ts:286-292 supplies tweenPercentage: k.percentage + animationId: anim.id explicitly).
• [Nit — unused export]: RESOLVED. accumulateCollidingAnimationTargets is function accumulateCollidingAnimationTargets(...) at gsapTweenSynth.ts:30 — no export keyword. Only remaining Fallow findings are 2 minor test-duplication + 1 minor high-CRAP on the connector arrow. No major findings.

Fresh-pass findings (at new HEAD):

• [Nit — test-file duplication] (useTimelineKeyframeHandlers.test.tsx:54,76): the 16-line Harness() block was flagged as a Fallow minor in R1 and is still present after the fix pass. Not blocking — separate cleanup is fine.
• [Nit — high-CRAP score] (TimelineDiamondConnectors.tsx:49): the .map arrow ticked over the Fallow CRAP threshold (37.1 vs. 30) because the collision gate added a branch to an already-dense render. Not blocking.
• [Observation — pre-plumbing without C1 consumer] (keyframeSlice.ts:42,49 + useTimelineKeyframeHandlers.ts:68): focusedEaseSegment.collidingAnimationTargets is threaded through the slice + setter, but the only writer (segment click) is now unreachable on collisions since the button hides. No C1 reader consumes the field either (grepped: no focusedEaseSegment.collidingAnimationTargets reads outside the slice type). This is expected pre-plumbing for #2693 — flagging so the reader lands there. Test coverage for the click-with-colliding-target still passes because tests call the handler directly.
• [Follow-up carried forward — delete-target-arbitrary] (useTimelineEditCallbacks.ts:66-87): out of scope for C1 (pre-existing), still worth a follow-up ticket since deletion on a colliding merged row still removes one arbitrary tween. Not a C1 regression.

Adversarial checks at new HEAD:

• easeAmbiguous fully purged from the repo (search/code at HEAD: 0 hits).
• Scope carve-out honored: the type-narrowing forces the invariant at the merge boundary, not at a downstream gate that could be bypassed.
• Runtime scan path (useGsapTweenCache.ts:419-441) writes directly to setKeyframeCache without going through deduplicateKeyframes, so unattributed runtime keyframes never violate the MergeableKeyframe requirement. Correct separation.
• Same-animation collision (primaryId === incoming.animationId early-out at line 43): collidingAnimationTargets stays undefined → ease follows last-write-wins on existing.ease = kf.ease (line 79). Same behaviour as pre-PR for within-animation dedup; the sharp edge noted in R1 P3 is unchanged and pre-existing.

Positive callouts:

• Type-level enforcement of the identity invariant (SourcedGsapPercentageKeyframe requiring both fields, MergeableKeyframe extending it, deduplicateKeyframes<T extends MergeableKeyframe>) is a stronger fix than either R1 recommended. The old "runtime guard that silently swallows incomplete keyframes" failure mode is now a compile-time error.
• Doc comment on SourcedGsapPercentageKeyframe names the exact failure mode ("silently treating that as 'no collision' is how an edit lands on the wrong tween") — future readers won't be tempted to relax the requirement.
• The <= 1 gate on the button (rather than !kf.collidingAnimationTargets?.length) is defensive against the edge case where the collision seed only has the primary and no incoming (length === 1) — the button still shows in that trivially-non-colliding case.
• Test rewrite from easeAmbiguous booleans to collidingAnimationTargets arrays validates the new invariant end-to-end, including first-seen order and 3-way dedup.

Overall — clean fix, addressed both reviewers' concerns, no new P1s introduced. Approving. The remaining Fallow minors and pre-plumbing observation are follow-up material, not blockers.

— Review by Via

[james-russo-rames-d-jusso]

Delta-reviewed c02a44ca..7e6b3e64.

Both prior concerns cleanly resolved, and the fix is stronger than the one I proposed — the attribution invariant now lives in the type system instead of a runtime guard, so an unattributed keyframe cannot even reach the merge:

• SourcedGsapPercentageKeyframe (packages/parsers/src/gsapSerialize.ts:97) requires animationId + tweenPercentage as non-optional fields. deduplicateKeyframes<T extends MergeableKeyframe> and accumulateCollidingAnimationTargets(keyframe: AnimationKeyframeTarget, incoming: AnimationKeyframeTarget) no longer accept the previously-optional shape, so the old runtime short-circuit is gone by construction. The docblocks name the exact failure mode ("silently treating that as 'no collision' is how an edit lands on the wrong tween") — nice.
• The C1 landing state is now honestly safe: the merged-collision keyframe drops its ease in deduplicateKeyframes (packages/studio/src/hooks/gsapTweenSynth.ts:73) and the inline ease button is hidden where (kf.collidingAnimationTargets?.length ?? 0) > 1 (packages/studio/src/player/components/TimelineDiamondConnectors.tsx:87). Test flip in TimelineClipDiamonds.test.tsx (shows → hides, 2 → 1 buttons) matches the deliberate semantics change; C2 re-enables the button behind the bulk-edit path.

Concerns

• CI Fallow audit is red — one fallow/high-crap-score on the marker-map arrow at TimelineDiamondConnectors.tsx:49 (CRAP 37.1 vs 30.0, cyclomatic 11) plus two fallow/code-duplication on useTimelineKeyframeHandlers.test.tsx:54,76 (16-line Harness clone). The CRAP score is a direct consequence of the added (kf.collidingAnimationTargets?.length ?? 0) <= 1 && kf.animationId !== undefined branch; extracting the segment-button subtree into a small <InlineEaseButton /> component would drop the arrow back below threshold and split the duplicated Harness in the tests. Either way the audit is currently a required check.

What I didn't verify

• Peer-callsite audit of the new SourcedGsapPercentageKeyframe / GsapKeyframesData<K> public-API additions across hyperframes-internal — the generic default keeps existing callers source-compatible, but any TypeDoc / DTS consumer will now see the type arg.

— Review by Rames D Jusso