feat(studio): keyframes flag, gesture recording + timeline/selection refinements#1561
Conversation
b46e2b6 to
a96a6cf
Compare
d9d0a45 to
ba99ae0
Compare
a96a6cf to
b56fbc8
Compare
terencecho
left a comment
There was a problem hiding this comment.
Stack-wide process note (Rames covered structure / 2 concerns on this PR)
Endorsing @rames-jusso's #1561 take in full — playerStore.ts:374-379 exposing the full Zustand store via window.__playerStore in prod is a real footgun (anyone with devtools access can call __playerStore.setState({...}) to mutate selection/elements/playing); and the clipToTweenPercentage 1-keyframe edge using a clipPct fallback IS the same pattern this PR fixes elsewhere. Both should land before merge.
Stack-wide process observation (non-blocking, easy fix for next stack)
All 9 PR descriptions across this stack are the unfilled PR template. For a stack with several PRs north of +500 LOC (and the +651/-4 core-mutations PR #1554 being load-bearing), the empty bodies mean reviewers (human and bot) and future archaeologists have nothing to anchor to — no "what / why / how / test plan" content. Commit messages cover the what well, but PR-body framing of why doesn't survive a bisect via the GitHub UI.
This stack is heavily test-backed and Graphite-anchored, so structural intent is recoverable from diff + commit messages. Not blocking — just a process nudge for the next stack.
Test-coverage map across the 9-PR stack (from my axis pass)
| PR | Files | Test files | Notes |
|---|---|---|---|
| #1553 | 2 | 0 | banner shim, n/a |
| #1554 | 5 | 2 | strong (parity + parser units) |
| #1555 | 4 | 1 | adequate |
| #1556 | 3 | 0 | CLI surface, typical no-unit |
| #1557 | 7 | 3 | strong |
| #1558 | 8 | 3 | strong on new infra |
| #1559 | 6 | 2 | strong on pure-logic helpers |
| #1560 | 5 | 0 | UI overlay, typical no-unit |
| #1561 | 19 | 5 | very strong |
— Review by tai (pr-review)
ba99ae0 to
676ba06
Compare
b56fbc8 to
878d25a
Compare
676ba06 to
f9e5f4f
Compare
878d25a to
bcdfd8e
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Concurring with the consolidated review's two author-response gates at HEAD 878d25a2 (both verified still open):
packages/studio/src/player/store/playerStore.ts:377-379—window.__playerStoreships unguarded in prod. Verified at HEAD: only the SSRtypeof window !== "undefined"guard wraps it; there is noimport.meta.env.DEVorprocess.env.NODE_ENV === "development"gate. Production users get the full Zustand mutating surface via__playerStore.setState({...}).packages/studio/src/components/editor/KeyframeNavigation.tsx:32-46—clipToTweenPercentage1-anchor edge. Verified at HEAD:if (mapped.length < 2) return clipPct;returns clip-relativeclipPctwhen only one keyframe has atweenPercentage, which is exactly the off-by-tween-offset silent no-op the PR's own comment block (:115-126) calls out as the bug pattern this PR is fixing. Identity-slope at one anchor (mapped[0].tweenPercentage! + (clipPct - mapped[0].percentage)) is the right fallback.
Two depth additions from my pass:
-
useEnableKeyframesbehavioural change is migration-safe. The diff narrows scope to the "no existing animation" branch only — thekfAnimbranch atuseEnableKeyframes.ts:126-137and the flat-anim branch are untouched. New behaviour: a single keyframe at the playhead viaresolveNewTweenRange-clamped start, matching the stated intent ("creating 0%+100% up front showed two diamonds for a single add keyframe"). Existing compositions with keyframes already enabled take the unchangedkfAnimbranch — they are not migrated.resolveNewTweenRangeships with regression coverage inuseEnableKeyframes.test.tsincluding the auto-stampdata-start="0"/data-duration=14scenario. Ungating is appropriate here because the regression risk is one-way (better, not worse) for a brand-new gesture. -
Drive-by behavioural changes verified non-regressive. (a) Timeline-element key-form fix —
buildTimelineElementKeyrecomputed withdomId/selectorto match the store's hash form; backed byuseExpandedTimelineElements.test.ts(+34) andstudioHelpers.test.ts(+32); a pure fix to a selection-highlight bug. (b)setSelectedElementIdclearingactiveKeyframePct— comment explains the diamond-click ordering invariant; sensible and contained. (c)findTimelineIdByAncestor— additive, tried only whenfindMatchingTimelineElementIdreturns null; tested. (d)StudioHeaderno longer clears DOM selection on inspector-panel collapse — strictly less destructive than before; the orphanedclearDomSelectionimport is correctly removed. None of (a-d) is a regression for prior muscle memory.
Title note stands: STUDIO_KEYFRAMES_ENABLED is pre-existing on main at manualEditingAvailability.ts:73; this PR adds use sites, not the flag itself.
Review by Via
f9e5f4f to
789f860
Compare
bcdfd8e to
cd69359
Compare
789f860 to
80f1a0f
Compare
cd69359 to
be3c288
Compare
80f1a0f to
896e1ae
Compare
be3c288 to
f8a6560
Compare
…refinements Enable-keyframes gate, recorded-gesture-stays-in-place, clipToTweenPercentage keyframe-nav, inline-expand timeline elements, and minor header/layout/store touches.
896e1ae to
c66ee48
Compare
f8a6560 to
9b9cb41
Compare
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Also fixes "Add keyframe at playhead" for array-form keyframe tweens
(keyframes: [{x,y},…]):
- readElementPosition derived the captured props from anim.properties, which
is empty for array-form keyframes, so the position came back empty and the
add silently no-oped. It now falls back to the union of the keyframe stops'
property keys (then x/y).
- The add site and the toolbar button state computed the playhead percentage
from the element clip range, not the tween range, so keyframes landed at the
wrong percentage. Both now use the tween-relative basis.
- When the playhead is outside the tween range, the button is disabled instead
of silently no-oping (or, post-basis-fix, deleting the boundary keyframe).
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Also fixes "Add keyframe at playhead" for array-form keyframe tweens
(keyframes: [{x,y},…]):
- readElementPosition derived the captured props from anim.properties, which
is empty for array-form keyframes, so the position came back empty and the
add silently no-oped. It now falls back to the union of the keyframe stops'
property keys (then x/y).
- The add site and the toolbar button state computed the playhead percentage
from the element clip range, not the tween range, so keyframes landed at the
wrong percentage. Both now use the tween-relative basis.
- When the playhead is outside the tween range, "add keyframe at playhead" now
extends the tween to reach it and adds a keyframe there, keeping the existing
keyframes at their absolute times (percentages rescale into the new range),
instead of disabling or no-oping.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Also makes "Add keyframe at playhead" do the right thing for every GSAP
animation shape, never disabling or silently no-oping:
- Array-form keyframe tweens (keyframes: [{x,y},…]): readElementPosition now
derives the captured props from the keyframe stops (top-level properties is
empty for array form), and the percentage uses the tween range, not the clip
range — so the add lands at the right spot instead of no-oping.
- Out of the tween range, the action extends the tween to reach the playhead
and adds a hold there, rescaling existing keyframes to keep their absolute
timing (was: disabled / destructive).
- Flat tweens (to/from/fromTo) convert to their natural keyframes, then take
the same add/extend path — so an out-of-range playhead extends them too.
- set() is promoted to a two-stop tween from the set's time to the playhead.
- motionPath/arc tweens add a waypoint at the on-path position (matching
segment, so the curve is preserved) instead of being linearized; outside the
range they extend their duration. A small merge threshold avoids duplicate
waypoints at the path endpoints.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Makes "Add keyframe at playhead" do the right thing for every GSAP animation
shape, never disabling or silently no-oping:
- Array-form keyframe tweens (keyframes: [{x,y},…]): readElementPosition now
derives the captured props from the keyframe stops (top-level properties is
empty for array form), and the percentage uses the tween range, not the clip
range — so the add lands at the right spot instead of no-oping.
- Out of the tween range, the action extends the tween to reach the playhead
and adds a hold there, rescaling existing keyframes to keep their absolute
timing (was: disabled / destructive).
- Flat tweens (to/from/fromTo) convert to their natural keyframes, then take
the same add/extend path.
- set() is promoted to a two-stop tween from the set's time to the playhead.
- motionPath/arc tweens add a waypoint at the on-path position (matching
segment, so the curve is preserved) instead of being linearized; outside the
range they extend their duration, with a merge threshold against duplicates.
Also fixes deep-link hydration: on a fresh full-page load the player runtime
isn't ready to honor the first requestSeek, so the one-shot seek latched without
moving the playhead, and the selection hydration (gated on the seek settling)
never ran — a URL with ?t=…&selId=… restored neither. A bounded heartbeat now
retries the seek until the player honors it, then the selection restores.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Makes "Add keyframe at playhead" do the right thing for every GSAP animation
shape, never disabling or silently no-oping:
- Array-form keyframe tweens (keyframes: [{x,y},…]): readElementPosition now
derives the captured props from the keyframe stops (top-level properties is
empty for array form), and the percentage uses the tween range, not the clip
range — so the add lands at the right spot instead of no-oping.
- Out of the tween range, the action extends the tween to reach the playhead
and adds a hold there, rescaling existing keyframes to keep their absolute
timing (was: disabled / destructive).
- Flat tweens (to/from/fromTo) convert to their natural keyframes, then take
the same add/extend path.
- set() is promoted to a two-stop tween from the set's time to the playhead.
- motionPath/arc tweens add a waypoint at the on-path position (matching
segment, so the curve is preserved) instead of being linearized; outside the
range they extend their duration, with a merge threshold against duplicates.
Also fixes deep-link hydration. A URL with ?t=…&selId=… restored neither the
playhead nor the selection on a fresh load: useStudioUrlState requests the seek
before the player runtime mounts its requestedSeekTime subscription, so the
request never reached pendingSeekRef, and initializeAdapter (which drained only
pendingSeekRef when the adapter became ready) started at 0 — which also blocked
selection hydration (gated on the seek settling). Fixed at the source:
initializeAdapter now reconciles the store's requestedSeekTime as well, so a seek
requested any time before the adapter is ready lands deterministically.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Makes "Add keyframe at playhead" do the right thing for every GSAP animation
shape, never disabling or silently no-oping:
- Array-form keyframe tweens (keyframes: [{x,y},…]): readElementPosition now
derives the captured props from the keyframe stops (top-level properties is
empty for array form), and the percentage uses the tween range, not the clip
range — so the add lands at the right spot instead of no-oping.
- Out of the tween range, the action extends the tween to reach the playhead
and adds a hold there, rescaling existing keyframes to keep their absolute
timing (was: disabled / destructive).
- Flat tweens (to/from/fromTo) convert to their natural keyframes, then take
the same add/extend path.
- set() is promoted to a two-stop tween from the set's time to the playhead.
- motionPath/arc tweens add a waypoint at the on-path position (matching
segment, so the curve is preserved) instead of being linearized; outside the
range they extend their duration, with a merge threshold against duplicates.
Also fixes deep-link hydration. A URL with ?t=…&selId=… restored neither the
playhead nor the selection on a fresh load: useStudioUrlState requests the seek
before the player runtime mounts its requestedSeekTime subscription, so the
request never reached pendingSeekRef, and initializeAdapter (which drained only
pendingSeekRef when the adapter became ready) started at 0 — which also blocked
selection hydration (gated on the seek settling). Fixed at the source:
initializeAdapter now reconciles the store's requestedSeekTime as well, so a seek
requested any time before the adapter is ready lands deterministically.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.
Consolidates the studio side of the GSAP keyframe/motion-path work into one
PR: runtime read layer + shared helpers, drag/commit/bridge editing infra,
motion-path geometry + commit helpers, on-canvas motion-path overlay, and the
keyframes flag with gesture recording + timeline/selection refinements.
Makes "Add keyframe at playhead" do the right thing for every GSAP animation
shape, never disabling or silently no-oping:
- Array-form keyframe tweens (keyframes: [{x,y},…]): readElementPosition now
derives the captured props from the keyframe stops (top-level properties is
empty for array form), and the percentage uses the tween range, not the clip
range — so the add lands at the right spot instead of no-oping.
- Out of the tween range, the action extends the tween to reach the playhead
and adds a hold there, rescaling existing keyframes to keep their absolute
timing (was: disabled / destructive).
- Flat tweens (to/from/fromTo) convert to their natural keyframes, then take
the same add/extend path.
- set() is promoted to a two-stop tween from the set's time to the playhead.
- motionPath/arc tweens add a waypoint at the on-path position (matching
segment, so the curve is preserved) instead of being linearized; outside the
range they extend their duration, with a merge threshold against duplicates.
Also fixes deep-link hydration. A URL with ?t=…&selId=… restored neither the
playhead nor the selection on a fresh load: useStudioUrlState requests the seek
before the player runtime mounts its requestedSeekTime subscription, so the
request never reached pendingSeekRef, and initializeAdapter (which drained only
pendingSeekRef when the adapter became ready) started at 0 — which also blocked
selection hydration (gated on the seek settling). Fixed at the source:
initializeAdapter now reconciles the store's requestedSeekTime as well, so a seek
requested any time before the adapter is ready lands deterministically.
Supersedes the separately-reviewed studio PRs #1557, #1558, #1559, #1560, #1561.

Stack: GSAP keyframe + motion-path editing — feature flag, gesture recording, timeline polish (top of #1553 → #1561).
What
Ties the feature together and adds the remaining tooling:
useEnableKeyframes).Why
The keyframe/motion-path surface ships behind a flag; gesture recording gives a fast way to author motion; the navigation + razor tooling rounds out the timeline editing experience.
How
useEnableKeyframes.ts: flag + enablement gate.useGestureRecording.ts: samples pointer position over the timeline, simplifies (RDP), and commits keyframes (merge into an overlapping tween, else add a new range).KeyframeNavigation.tsx: per-property diamond (add / remove / convert) + prev/next seek.useRazorSplit.ts: split a clip at the playhead.useDomSelection.ts/useStudioContextValue.ts/StudioHeader.tsx/NLELayout.tsx/RenderQueue.tsx: selection + timeline refinements (selection survives inspector collapse, etc.).Test plan