fix(studio): remove keyframe dragging from the timeline

[miguel-heygen]

Remove keyframe dragging from the timeline (6/8)

Deletes the timeline keyframe-drag affordance and its supporting code (−448 LOC). Pure removal; timeline still renders and selects normally.

[miguel-heygen]

• chore(studio): add anonymous usage events #1765
• fix(studio): strip a group's GSAP when ungrouping #1764
• fix(studio): remove keyframe dragging from the timeline #1763 👈 (View in Graphite)
• fix(studio): batch 3D depth commit so perspective + z don't race #1762
• feat(studio): expand sub-composition groups + children in the timeline #1761
• feat(gsap): read timelines authored inline (acorn read path) #1760
• feat(studio): element groups — studio UI #1759
• feat(studio): element groups — source mutations #1758
• main

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

[james-russo-rames-d-jusso]

Reviewed at 2f9ece73. Clean -448 LOC deletion of the timeline keyframe-drag affordance — diamonds still render, click-to-seek, and context-menu (add / remove / ease) all preserved. Caller-graph audit shows zero external consumers in hyperframes-internal or hyperframes-gemini-agent; the in-repo orphan is small. PR body matches behavior.

🟡 Concerns

• snapKeyframePctToBeat is now dead code. packages/studio/src/player/components/timelineEditing.ts:120 — still exported, zero callers in the repo after this PR (the only call site was the onSnapKeyframePct binding in Timeline.tsx that's deleted in this diff). Either delete the function + its unit tests, or keep it but at least drop the export. Same cleanup spirit as removing the keyframeMove module — leftover export is the kind of thing that ages into "why does this exist?" two quarters from now. Not a blocker; trivial follow-up.

• No assertion that the diamonds still respond to click in Timeline.test.ts. The PR drops the onPointerDown handler that was the diamond's only event-listener for drag, leaving onClick + onContextMenu. The PR body claims "click-to-seek, and offer the context menu" still work, and the code looks right, but there's no test pinning this — a future "diamonds are pointer-events: none under some condition" refactor would silently regress click-to-seek and CI wouldn't catch it. Cheap addition.

🟢 Questions

• Selection-on-drag-start side effect. onPickKeyframeElement previously selected the element on drag-start, loading its GSAP session as a side effect. Worth confirming no other interaction path (keyboard K toggle, context-menu Add/Remove) was depending on that selection happening implicitly via a diamond grab. Tests pass + the K-toggle flow goes through its own selection path, so I think this is moot — but tagging in case there's an edge case.

🔵 Nits

• The commit message + PR body are unusually good — they correctly document the why (intermittent no-op/revert from session lag + the optimistic-hold workaround) which justifies the fix: prefix on a pure deletion. No action.

• Consider a CHANGELOG.md / docs/keyboard-shortcuts.md mention if either exists for the studio — users who learned the drag will look for it. Skip if those docs don't exist.

Verdict

LGTM modulo the dead-export cleanup. No blockers. CI green on all required checks. Happy to stamp once the orphan is addressed (or deferred to a follow-up PR you can name in-thread).

— Rames D Jusso

[vanceingalls]

Verdict: LGTM with one cleanup nit. Pure removal, surgically clean: gut'd render path still works for the basic display-only case, no cross-package consumers, no orphans in TimelineEditContext / timelineCallbacks / public surface. CI green (player-perf, regression, preview-regression all pass). HEAD 2f9ece73, no prior reviewers.

Findings

🟡 Orphan helper left behind: snapKeyframePctToBeat

packages/studio/src/player/components/timelineEditing.ts:120 still exports snapKeyframePctToBeat. This PR deletes the import in Timeline.tsx (-import { snapKeyframePctToBeat } from "./timelineEditing";) and its single call site (onSnapKeyframePct wiring around the old line 488). I scanned all 430 .ts/.tsx files under packages/studio/src at HEAD 2f9ece73, plus packages/player, packages/sdk, packages/core, packages/producer — the function has zero consumers post-PR. Not a pre-existing orphan: on main and on base eg-05-3d-keyframe-fixes, Timeline.tsx imports and calls it.

Drop the function definition (lines ~113-145 in timelineEditing.ts) in this PR or queue a one-line cleanup. Since the whole point of this stack is removing keyframe drag, leaving the snap helper alive only confuses the next reader who searches "keyframe" in the player.

💭 (Verified clean — for the record)

• TimelineEditContext residue: value.onMoveKeyframe correctly dropped from the memoization dep array (line 42 in the diff); the surviving deps match the surviving TimelineEditCallbacks fields in timelineCallbacks.ts. No orphan dep.
• TimelineClipDiamonds gutted path: the +4/-125 leaves a clean display-only render — effPct collapse to kf.percentage is correct since drag state is gone; the connecting-lines x1/x2 calc is the same arithmetic minus the drag indirection. No dangling useEffect / refs / cleanup.
• onMoveKeyframe callback type: removed from the TimelineEditCallbacks interface at timelineCallbacks.ts:42, and the Timeline.tsx destructure at line 86 drops it cleanly. No external consumers — gh search code (which indexes live HEAD lazily) and a brute-force scan of all 430 studio TS files at the PR SHA both confirm zero external references to onMoveKeyframe, onDragKeyframe, onPickForDrag, onPickKeyframeElement, pickKeyframeTween, computeKeyframeMovePlan, KeyframeMovePlan.
• Public surface (packages/studio/src/index.ts): never re-exported keyframeMove or its symbols — safe.
• Sub-comp timeline expansion (#1761): the expansion already merges into the base of this stack and renders via the same TimelineClipDiamonds — the gut'd display path covers expanded children automatically; no second drag-path to also remove.
• Stack context: #1764 (next on top) is about ungrouping, #1765 is telemetry. Neither references the dropped symbols. The cleanup nit above doesn't block the stack.
• "Decorative gate" check (per my memory on dangling state): no surviving isDragging / dragState set-without-unset. The dragRef / pendingRef / dragCleanupRef are all deleted as a block.

CI

All required: regression, preview-regression, player-perf, Preview parity, Preflight (lint + format) — SUCCESS. Graphite / mergeability_check in progress (cosmetic).

Review by Via

[vanceingalls]

Verdict: COMMENTED — both R1 nits remain unaddressed, but neither is blocking.

R2 HEAD a3f0c7c4 is a rebase-only refresh (single commit, identical PR delta +4/-448, base advanced from e6bd9e44 → be4cfa43 on eg-05-3d-keyframe-fixes). No new commit was added to address either R1 nit, and Timeline.test.ts was not touched in the rebased delta.

Per-finding status

• Via R1 — snapKeyframePctToBeat orphan at packages/studio/src/player/components/timelineEditing.ts:120: NOT addressed at a3f0c7c4. The exported helper is still present, and its only prod call site (Timeline.tsx) still has the import removed by this PR. No new references show up in Timeline.tsx at HEAD. Confirmed unchanged in the rebase delta. Still a NIT — happy to land as-is or in a follow-up.
• Rames R1 — click-to-seek test gap on TimelineClipDiamonds: NOT addressed at a3f0c7c4. Timeline.test.ts has no diamond/keyframe/seek coverage at HEAD and was not modified in the rebase delta. Still a NIT — the surviving click-to-seek behavior is asserted only via the manual QA path described in the PR body.

CI

All required checks pass at a3f0c7c4 (Preflight, regression, player-perf, Preview parity, preview-regression). Graphite / mergeability_check pending, non-blocking. WIP green.

Prior reviewer state

• james-russo-rames-d-jusso: COMMENTED at 2f9ece73 — click-to-seek test gap NIT.
• vanceingalls (Via): COMMENTED at 2f9ece73 — snapKeyframePctToBeat orphan NIT.
• No R2 reviews yet from either reviewer.

LGTM to land as-is at author's discretion; both nits remain landable as follow-ups.

R2 by Via

[james-russo-rames-d-jusso]

R2 verdict: COMMENTED — both R1 nits unaddressed; PR is rebase-only refresh.

Reviewed at a3f0c7c49817189b02722386df2cdbe72e4a2782. New HEAD is a rebase of the same +4/-448 delta — no new commits added to address R1 nits. Confirmed against Via's R2 (which converged independently 30s before mine).

Per-finding status

#	R1 source	Finding	Status at a3f0c7c4
1	Via 🟡	snapKeyframePctToBeat orphan at packages/studio/src/player/components/timelineEditing.ts:120	NOT addressed. Still exported with zero consumers across packages/studio, packages/player, packages/sdk, packages/core, packages/producer (re-confirmed at HEAD via grep -rn). No tests touch it either. Trivial cleanup; can land as follow-up.
2	Rames 🟡	No click-to-seek / context-menu pin on TimelineClipDiamonds	NOT addressed. Timeline.test.ts at HEAD has no diamond / keyframe / onClickKeyframe coverage. Rebase delta didn't touch test files. Surviving paths still asserted only via manual QA per PR body.
3	Rames 💭	Implicit selection-on-drag-start side effect	N/A — moot. Confirmed at HEAD: surviving onClickKeyframe at Timeline.tsx:468-478 explicitly calls setSelectedElementId(elKey) + onSelectElement?.(el) — selection is now explicit, not implicit-via-drag. Closing this one cleanly.

R2-NEW scan

Nothing new to flag at a3f0c7c4. Caller-graph for the removed symbols (onMoveKeyframe, onDragKeyframe, onPickForDrag, pickKeyframeTween, computeKeyframeMovePlan, KeyframeMovePlan) is empty across the monorepo. TimelineEditContext memo deps + timelineCallbacks.ts interface drop are clean. No useEffect / useRef survivors from the dragRef family. Public surface (packages/studio/src/index.ts) never re-exported anything from this module — safe.

Convergence with Via

Via's R2 at a3f0c7c4 (4 min before mine) reaches identical conclusions: both nits unaddressed, both non-blocking. No daylight between our findings. I won't re-litigate the snap-helper cleanup — happy to land as-is at the author's discretion or as a one-line follow-up.

CI

Required checks green at a3f0c7c4: regression, preview-regression, Preview parity, player-perf, Preflight. Graphite / mergeability_check in_progress (non-blocking).

Net

LGTM to land. Both nits are landable as follow-ups; neither is blocking. R1 verdict stands.

R2 verification by Rames D Jusso

[vanceingalls]

Verdict: COMMENTED — LGTM-sustained. R3 is a pure rebase; both R1/R2 nits unchanged, neither blocking.

R3 HEAD 072e6f78 is a rebase-only refresh — single commit, identical message, base of eg-05-3d-keyframe-fixes advanced from be4cfa43 → acc5df1d. PR delta unchanged at +4/-448 across the same 8 files. All 8 PR-affected files are byte-identical at a3f0c7c4 vs 072e6f78 (sha256-verified per file). The R2→R3 three-dot compare diff shows additional files only because the base branch itself moved forward; none of those changes are part of this PR's surface.

Per-finding status

• Via R1/R2 — snapKeyframePctToBeat orphan at packages/studio/src/player/components/timelineEditing.ts:120: NOT addressed at 072e6f78. Exported helper still present; verified via raw-content fetch at the new HEAD. Still a NIT — landable as-is or in a follow-up.
• Rames R1/R2 — click-to-seek test gap on TimelineClipDiamonds: NOT addressed at 072e6f78. Timeline.test.ts still has no diamond/keyframe/seek coverage. Surviving click-to-seek behavior remains asserted only via the manual QA path in the PR body. Still a NIT.

CI

At 072e6f78: 13 success, 6 skipped, 6 cancelled (superseded), 1 in-flight (Graphite / mergeability_check, non-blocking). Zero failures. mergeable_state: unstable is from the pending Graphite check; required checks all green.

Prior reviewer state

• james-russo-rames-d-jusso: COMMENTED at 2f9ece73 (R1) + a3f0c7c4 (R2) — click-to-seek test gap NIT. No R3 review yet.
• vanceingalls (Via): COMMENTED at 2f9ece73 (R1) + a3f0c7c4 (R2) — snapKeyframePctToBeat orphan NIT.

LGTM-sustained to land at author's discretion. Both nits remain landable as follow-ups.

R3 by Via

[james-russo-rames-d-jusso]

R3 verdict: LGTM with NITs (sustained). Pure rebase — neither R1/R2 nit addressed.

Reviewed at HEAD 072e6f78. R2 SHA a3f0c7c4 → R3 SHA 072e6f78 is a Graphite restack from the parent stack: base eg-05-3d-keyframe-fixes advanced (be4cfa43 → acc5df1d) and the single commit rehashed. PR's incremental diff vs base is unchanged at +4/-448 across the same 8 files. The R2..R3 three-dot diff shows additional domEditingLayers.ts/useDomSelection.ts deletions, but those are base-branch deltas being absorbed into the merge-base, not this PR's surface — confirmed via git diff acc5df1d..072e6f78 showing zero changes to the cited files.

Convergent with Via's R3 posted ~10min ago.

Per-finding status at HEAD

Finding	R1	R2	R3
🟡 snapKeyframePctToBeat orphan at packages/studio/src/player/components/timelineEditing.ts:120	Open	Open	Still open. Function still exported; grep -rn snapKeyframePctToBeat returns only the definition line across all packages. Originally orphaned at PR base acc5df1d, but this PR's stated purpose ("remove keyframe dragging") is the natural home for the cleanup — the helper exists only to support keyframe-drag beat-snap.
🟡 No test pin on surviving click-to-seek path at Timeline.tsx:468-478	Open	Open	Still open. Timeline.test.ts (362 LOC) has zero matches for onClickKeyframe / keyframe / seek / click-to-seek. Surviving behavior remains asserted only via manual QA per PR body.
💭 Implicit selection side-effect on click-to-seek	Closed at R2	n/a	n/a (closed clean — setSelectedElementId called explicitly at Timeline.tsx:471).

R3-NEW: none

Reviewed Timeline.tsx / TimelineClipDiamonds.tsx / timelineCallbacks.ts / keyframeMove.ts (deletion) / StudioPreviewArea.tsx (deletion) at HEAD. No new findings. Mechanical deletion-only PR; the only delta vs R2 is base-branch absorption.

CI

All required checks success at 072e6f78 (Preflight, regression, preview-regression, Preview parity, player-perf, WIP). Graphite / mergeability_check in_progress, non-blocking.

Net

Same verdict as R1/R2: LGTM with NITs, neither blocking. Both nits remain landable as follow-ups (the orphan cleanup belongs here morally, but base-branch hoist of the keyframe-drag removal makes it defensible to land separately). Ship at author's discretion.

R3 verification by Rames D Jusso