feat(studio): square speed-curve editor, class-tween attribution, apply-to-all easing#1705
Conversation
b8e31d5 to
baf6f66
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Motion-editor refresh in one pass: square cubic-bezier ease graph with clamped-to-view handles, class/selector tween attribution via element.matches() so a gsap.from(".dot", …) shows up on every dot, "Set all…" bulk-ease control that strips per-keyframe overrides in a single mutation, per-keyframe size on animated elements (resize at playhead instead of global hold), and inferred Timing range when no clip is authored. Wiring is clean across UI → context → editing hook → studio-api update-meta → parser+acorn writer, with paired round-trip tests on both write paths and a motion-path regression test for the new requireChannels filter. The #1694 BLOCK pattern is not re-introduced.
Verdict: LGTM
Findings (numbered, with severity tag):
-
NIT —
KeyframeEaseList.tsx:69usesvalue=""on the select with<option value="" disabled>Set all…</option>. The handler short-circuits on the empty string (if (next) onApplyAll(next)), but after a successful apply the select keepsvalue=""(uncontrolled-relative-to-DOM). Re-selecting the same ease never fires — you have to pick a different one first and then pick the one you want. AE-parity is preserved (no "current state" to read), but a user who appliedpower2.out, then expected one segment, then wanted to reapplypower2.outto the whole tween would silently get nothing. Cheap fix: stash a key on the<select>and bump it afteronApplyAll, or always force the value back to "" via a controlled component. Author discretion. -
NIT —
gsapDragCommit.ts:402keyframe-collision tolerance isMath.abs(p - pct) < 0.05. With pct rounded to 0.1 precision (line 396,Math.round(((ct - ts) / td) * 1000) / 10), the tolerance window is half the grid, so an existing keyframe at p=50.0 and a freshly-derived pct=50.04 will both be tagged with{width, height}. Two same-value keyframes coexist (Set dedup keeps both since 50.0 ≠ 50.04). Functionally harmless — the same{width,height}lands in both rows — but the resulting keyframes block has a redundant entry. Author discretion; the alternative (snap-to-existing within tolerance) costs one more line. -
NIT —
EaseCurveSection.tsx:182-183clampspxto[0,1]during drag butpyonly toclampView(py)which honors the[VMIN, VMAX]view (= roughly[-0.28, 1.28]). That means a user can drag a handle's x out to the edge but never inverse it (handle1.x > handle2.x is impossible). Fine for GSAP'scustom()ease (monotonic time → progress), but worth a comment so a future contributor doesn't try to "fix" the asymmetric clamp. -
NIT —
EaseCurveSection.tsx:101clampViewreadsVMIN/VMAXderived fromHR/Sconstants but those constants live above the component scope without a clarifying group comment about why the view extends past[0,1](overshoot headroom for back/elastic). The PR body explains it, the source could too. Author discretion.
Verified:
- #1694 BLOCK transitive non-regression:
useGestureCommit.tsatbaf6f66dlines 257 and 275 emiteaseEach: "none"(not"power1.inOut"). The hardcode I blocked on two days ago was fixed on main before #1705 landed. #1705 doesn't toucheaseEachinuseGestureCommit.ts— only removes orphanededitLogcalls. - Velocity-fitter contract preservation:
fitEasesFromVelocitystill leaves linear segments'easeundefined (constant-speed → no override). Apply-to-all is an explicit user action that replaces all per-keyframe overrides witheaseEach; it's not the fitter quietly defaulting. User intent is the source of truth here, no contract violation. - Apply-to-all wiring across layers:
KeyframeEaseList.onApplyAll→AnimationCard→StudioRightPanel.onSetAllKeyframeEases→DomEditContext→useDomEditSession.handleSetAllKeyframeEases→gsapCommitMutation({type: "update-meta", updates: { easeEach, resetKeyframeEases: true }})→studio-api/routes/files.tsupdate-metatype → bothgsapParser.ts(stripKeyframeEases) andgsapWriterAcorn.ts(inline strip onkfNode.properties). End-to-end mutate path is symmetric between the two writers. - Test coverage: parser round-trip (
gsapParser.test.ts:580), acorn writer round-trip (gsapWriter.acorn.test.ts:275), motion-path regression forrequireChannels(gsapRuntimeKeyframes.test.ts:104— including the documents-the-bug negative case),resolveSelectorElementIdsselector resolution (useGsapTweenCache.test.ts). Square editor itself is interaction-only (no unit test), reasonable for an SVG drag widget. - Class-tween attribution flow:
resolveSelectorElementIdscorrectly splits comma-groups, triesquerySelectorAllper part, falls back toextractIdFromSelectoronly when the selector throws.getAnimationsForElementuses both the matcher set AND atry { element.matches(part) }guarded with a swallow comment ("tween selector isn't a valid CSS selector for matches() — skip") — correct, not a decorative gate. - Per-keyframe size: motion-path shadow bug:
useMotionPathData.ts:122now passes["x","y"]asrequireChannels, so the longer size tween can't shadow the position-only motion path past the position tween's range. Negative test ingsapRuntimeKeyframes.test.ts:130documents the bug pre-filter. - Cross-PR family non-regression (#1691/#1692/#1693/#1694/#1702/#1703): #1705 doesn't touch marquee (
#1693) selection state, doesn't modify per-keyframe ease update (#1694'shandleUpdateKeyframeEasepath is unchanged), and the newhandleSetAllKeyframeEasesis a sibling, not a wrapper, so its mutation goes through the sameupdate-metarail. - Dead-code removal:
editDebugLog.ts(no-op body) and all six call sites deleted in lockstep — no orphaned imports.
CI state at HEAD baf6f66d: all required green — Build / Lint / Typecheck / Test / CodeQL / Fallow audit / preview-regression / player-perf / Perf:{drift,fps,load,parity,scrub} / Studio: load smoke / CLI smoke / SDK / Test: runtime contract. Regression-shards 1–8 still running but not required. Mintlify Deployment SKIPPED. No required failures.
Prior reviewer state: reviews: [] at write time. No Rames / Genesis / Magi / Xiaye signal to converge with — first review on the PR.
Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
HF#1705 — motion-editor bundle: square ease editor, class-tween attribution, apply-to-all easing, per-keyframe size, inferred timing (reviewed at baf6f66d)
Five concerns + cleanup in one PR. Most of it is well-shaped (the writer/parser plumbing for resetKeyframeEases is correct, the requireChannels motion-path filter is the right level of fix, the curve editor reads cleanly). A few sharp findings below.
🛑 Blockers
• none.
• propertyPanelTimingSection.tsx — the inferred-range feature is gated upstream and won't fire on the headline case. PropertyPanel.tsx:351 only renders <TimingSection> when element.dataAttributes.start != null. For a pure-GSAP element with no data-start authored (the PR body's stated case — "no authored clip range"), TimingSection doesn't render at all, so deriveTimingFromAnimations is unreachable. As written, the inference only helps elements that have data-start AND data-duration=0/missing — a much narrower wedge. Either drop the start != null gate (and key off data-duration/animations together), or render <TimingSection> whenever there's a derivable range from animations.
• gsapDragCommit.ts:393-415 (commitKeyframedSizeFromResize) — the resize is two sequential commitMutations: {type:"delete", animationId: sizeSet.id} then {type:"add-with-keyframes", ...}. The per-file serializer prevents interleaving but the pair is not transactional. If the second call fails (network blip, validation), the prior sizeSet is already gone and the element loses its size hold silently from the user's POV (toast goes by, but state is now inconsistent until next save). Same-file lost-resize bug is plausible under flaky networks. Consider a single replace/update-with-keyframes-style mutation, or surface a clear "size change failed, original restored" recovery (re-add sizeSet on the catch path).
• EaseCurveSection.tsx:182-185 (drag-handle clamp mutates state). clampView is applied to py inside handlePointerMove, so the very first drag of a handle whose loaded y is outside [VMIN, VMAX] (e.g. an elastic/heavy back preset loaded with y > 1.28 or < -0.28) silently snaps the bezier to the clamp boundary instead of preserving fidelity. Visual clamping that mutates committed state on touch is a footgun: the display path correctly draws from un-clamped cp1.y/cp2.y (good), but the drag path loses precision. Suggest: clamp the display offset (already done via clampView on p1.y / p2.y) but on drag, derive py un-clamped and only snap when the delta from prior is small (drag was on the visible handle), or simply allow drag to extend up to a wider bound and only clamp the display.
🟡 Questions
• gsapAnimationConstants.ts:90-96 — EASE_LABELS wiped (≈30 friendly labels removed; consumers fall through to raw GSAP tokens). Five consumers (EaseCurveSection, AnimationCard, KeyframeEaseList, gsapAnimationHelpers, plus self) all guard with ?? token, so functionally safe — but this is a user-facing wording change ("Smooth slowdown" → power2.out everywhere, including the buildTweenSummary tooltip / animation card subtitle). Not in the PR title; Jake signed off? Worth a one-line note in the PR body for the next reader.
• useGsapTweenCache.ts:482 — resolveSelectorElementIds uses naive selector.split(","). Real risk is low (gsap target strings are typically .dot / #id / .a, .b), but [data-x="a,b"] or :not(.foo,.bar) (CSS4) would split mid-token. The try/catch on querySelectorAll makes this graceful (drops the broken part), but the tween would silently miss attribution. Worth a comment or a smarter split (paren/bracket-aware) when you next touch this.
• useGsapTweenCache.ts:397-406 — per-element cache-clear guard. When a class tween is removed from the file, the cached entry under ${sourceFile}#${elementId} / elementId stays put (the new guard skips clearKeyframeCacheForElement). Stale diamonds linger until the file-wide usePopulateKeyframeCacheForFile re-runs (which does clearKeyframeCacheForFile first). For most edit flows that's prompt (bumpGsapCache), but a select-then-don't-re-save flow could show ghost diamonds for an evicted tween. Bounded but worth knowing.
• gsapRuntimeBridge.ts:337-352 — commitKeyframedSizeFromResize is only invoked when resizeGroup === "size" (no scale tween). If an element has only an opacity tween and the user resizes it, pickClosestToPlayhead picks the opacity tween and creates size keyframes pinned to the opacity tween's time range. Probably fine (it's the most "active" tween for the element at that moment) but not what the PR comment "aligned to the element's existing animation" suggests at first read. Intended?
✅ Verified
• gsapWriterAcorn.ts:330-342 and gsapParser.ts:1245-1256 — resetKeyframeEases correctly strips per-keyframe ease and sets easeEach in a single updateAnimationInScript call on both writer paths. Tests at gsapWriter.acorn.test.ts:276-296 and gsapParser.test.ts:580-595 exercise the round-trip including kf property preservation.
• gsapRuntimeKeyframes.ts:252-302 (requireChannels) — filter applied at the right point (after readTween, before tween-selection), default undefined preserves back-compat for non-motion-path callers. useMotionPathData.ts:122 passes ["x","y"] explicitly. Test at gsapRuntimeKeyframes.test.ts:104-148 documents both the fix and the prior bug (nice).
• useGsapTweenCache.ts:121-148 (getAnimationsForElement + element.matches) — try/catch correctly wraps matches() so a tween with a non-CSS-parseable selector (gsap accepts more than querySelectorAll) doesn't blow up the inspector.
• The 4 hooks with deletion-only deltas (useGestureCommit.ts, useGsapAwareEditing.ts, useGsapScriptCommits.ts, useRazorSplit.ts) are all editLog() call removals; the deleted editDebugLog.ts was already a no-op stub (ponytail body-removed). useGestureCommit.ts await liveSession.commitMutation shape (the canonical fix from #1608) is intact — the deletion is only the upstream editLog("gesture", ...) line, no flow change.
• Class-tween cold-load gap (iframeRef not in usePopulateKeyframeCacheForFile deps) is covered by the separate runtime-scan effect (polls up to 5s, populates from live tween.targets() rather than selector-parsing) plus the cache-clear guard. Defensible.
What I didn't verify
• I didn't run bun test packages/studio or open the iframe in a real preview — drag-handle clamp behavior is reasoned from the code, not observed.
• I didn't audit every consumer of EASE_LABELS for design-team-facing wording (e.g. timeline tooltip, gesture-fit summary) — sampled five sites, all fall-through-safe, but Figma-vocabulary-vs-friendly-labels is a product call.
Review by Rames D Jusso
…ution, per-keyframe size & ease
Speed-curve editor: a fixed-square cubic-bezier graph (grid, linear reference,
draggable handles, live preview) for editing eases; conventional preset grid.
Class/selector tweens: attribute `gsap.from(".dot", …)`-style tweens to every
matching element so they surface in the inspector and keep their timeline
keyframe diamonds when the clip is selected.
Apply-to-all easing: a "Set all…" control sets easeEach and strips every
per-keyframe ease override in one mutation (AE select-all + F9). Implemented in
BOTH gsap writers — the acorn writer and the recast writer (the default server
path); the recast side was missing resetKeyframeEases, so "Set all" set easeEach
but left per-keyframe eases in place.
Per-keyframe size: resizing an animated element writes a width/height keyframe
at the playhead — other keyframes keep their size — instead of a global
gsap.set hold; static elements keep the simple global resize. The extra size
tween exposed a motion-path bug (the overlay read whichever tween contained the
playhead), fixed with an opt-in requireChannels filter so the path only reads
the positional tween.
Inferred Timing: derive Start/End/Duration from an element's animations when it
has no authored clip range, instead of showing 0.00s.
Ease labels now surface the raw GSAP token (power2.out, back.out, …) instead of
invented names ("Smooth slowdown") that confused authors.
Also pass the preview iframe to the inspector's animation hook so element
resolution runs, and remove the unused editDebugLog facility.
baf6f66 to
87064a4
Compare
|
Thanks both — addressed the substantive findings in Fixed
Noted in PR body
Acknowledged, not changing (author discretion)
All required checks green locally (fallow |
Summary
Motion-editor improvements, in three parts.
1. Square speed-curve editor (
EaseCurveSection)The ease curve now renders as a geometrically-square unit plot (equal X/Y scale — no horizontal stretch), with fixed overshoot headroom above 1 / below 0 for back/elastic eases. The view is fixed (no per-curve zoom), and handles clamp to the visible range so an overshoot handle never drifts off-screen on a long tangent. Adds a grid, square frame, linear-reference diagonal, larger hit-targets, hover-grow handles, a live preview dot, a cubic-bezier value readout, and conventional preset ordering (linear → in/out/in-out → back family → snappy).
2. Class / selector tween attribution in the inspector
getAnimationsForElementnow also tests each comma-part of a tween's target selector withelement.matches()(true CSS semantics). A shared class or descendant tween — e.g.gsap.from(".dot", { stagger }), one of the most common GSAP patterns — now shows in the per-element inspector for every matching element, instead of only the one whose exact selector string equalled the tween's. The hook resolves the live element from the preview iframe and passes it through.3. "Apply to all segments" per-keyframe easing
A "Set all…" control on the per-keyframe easing list applies one ease to every segment at once (the After Effects "select all keyframes → F9" workflow). It sets `easeEach` and strips all per-keyframe `ease` overrides in a single mutation via a `resetKeyframeEases` flag threaded through `update-meta` → `updateAnimationInScript`.
4. Per-keyframe size on animated elements
Resizing an animated element now writes a width/height keyframe at the playhead — other keyframes keep their size — instead of a global `gsap.set` hold. Static elements keep the simple global resize. That second tween exposed a motion-path bug (the overlay read whichever tween contained the playhead), fixed with an opt-in `requireChannels` filter so the path only ever reads the positional tween.
5. Inferred Timing range
When an element has no authored clip range, the Timing panel now infers Start/End/Duration from its animations instead of showing 0.00s.
Test plan
Follow-up (separate PR): the 3D-transform control.
6. Ease labels show GSAP tokens
Ease labels across the panel (curve presets, per-keyframe segments, animation-card subtitle, tween summary) now surface the raw GSAP token (
power2.out,back.out, …) instead of invented friendly names ("Smooth slowdown") — authors recognize the GSAP vocabulary. Every consumer readsEASE_LABELS[token] ?? token, so the map was simply emptied; re-add an entry to override a specific token.