feat(studio): add keyframe ease editor#2674
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7512a12 to
bd750ff
Compare
a115a6d to
99c7665
Compare
bd750ff to
982bebe
Compare
982bebe to
eddcc10
Compare
99c7665 to
9a94e04
Compare
9a94e04 to
786802f
Compare
eddcc10 to
8232ac4
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (with notes)
Rule applied: RIGHT-direction non-blocker findings → APPROVE (per COMMENTED-vs-APPROVE gate). Editor composition is clean; noted items are one React idiom nudge and one cross-PR confirmation.
Multi-reviewer inventory: parallel review with Rames. No other review at submission time. Size-exception justification in the PR body is reasonable — splitting further would introduce a non-typechecking intermediate branch.
What holds up
- Single-source-of-truth for mode defaults —
DEFAULT_EASE_BY_MODEinEaseCurveSection.tsx:82is the same set of strings that #2672's runtime parses. Switching modes emits a well-formed token that survives the round-trip. resolveEditableCurve+resolveEditorLabel— clean fall-through from preset → spring → wiggle → custom → catalog → raw. No hidden mutation, no useEffect for state derivation. Derived-during-render, correct.- Drag geometry —
handlePointerMoveclampspxto[0,1](monotonicity guarantee for a valid time curve) andpyto a wider[DRAG_VMIN, DRAG_VMAX]for overshoot fidelity, with the display handle re-clamped to view viaclampView. The two-tier clamp (drag vs display) is the correct model — commits keep the authored value even when the visual handle rides the edge. EaseBezierFieldinEaseParamFields.tsx:38— useskey={text}on the<input>to reset internal state when the tuple changes from a drag/preset. That's the React-recommended pattern for prop-driven state resets. Nicely done.- Wiggle default amplitudes centralized —
WIGGLE_DEFAULT_AMPLITUDEinEaseParamFields.tsx:15mirrors the runtime defaults in #2672'swiggleEase.ts:39. Contract kept in sync via co-authoring on the same PR family. - Accessibility —
role="group",aria-label,aria-pressed,aria-haspopup,aria-expandedon the mode toggle + dropdown. Good. - Test coverage — 51 focused tests, covering preset selection, mode switching, drag commit, spring/wiggle field parsing, and telemetry event emission. Solid.
Nits / follow-ups (non-blocking)
-
EaseCurveSection.tsx:229—useEffect(() => { setDraft(null); }, [ease])— this is the derived-state-via-useEffect pattern the React docs recommend replacing with a "prev-prop compare during render" reducer. The comment argues cogently against clearing onpointerUp(flicker on the stale-prop frame) — that reasoning is correct. But the derive-during-render alternative is idiomatic and avoids the extra effect trip:const [prevEase, setPrevEase] = useState(ease); const [draft, setDraft] = useState<Pts | null>(null); if (ease !== prevEase) { setPrevEase(ease); setDraft(null); }
Same commit-flicker fix, no effect. Small win. Not blocking.
-
EaseCurveSection.tsx:30—EasePresetGridrendersEASE_PRESETS.filter(...).map(...)insidemax-h-56 overflow-y-auto. Static preset library so unbounded-list virtualization concern is N/A right now, but if the preset set grows beyond ~50 entries per kind considerreact-windowor the Studio virtualizer. -
EaseCurveSection.tsx:88—EaseModeToggleusesrole="group"witharia-pressedon each button. Preferrole="radiogroup"+role="radio"semantics for mutually-exclusive mode selection — screen readers announce "1 of 3" navigation. Follow-up.
Cross-PR confirmation (from my #2673 review)
moveKeyframeInScript behavior in #2673 changed occupied-destination from OVERWRITE to NO-OP. The ease editor doesn't wire retimes directly (it commits ease strings only), so this PR is not affected. But if the timeline drag caller elsewhere in Studio uses moveKeyframeInScript, please confirm the drag surface handles the no-op instead of silently swallowing.
hyperframes conventions check
- Named exports throughout (
export function ...,export const ...). ✓ - Studio uses non-prefixed Tailwind + hardcoded neutral-palette dark theme (
bg-neutral-900/50,border-white/10) — consistent with the rest ofpackages/studio. Not the pacifictw-*+ semantic-token convention, but this repo does not follow that convention. Fine. - English strings inline (no i18n indirection). Consistent with rest of studio. Fine.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 8232ac4361e95141e7c9a322be8a7604d411174b.
The Studio ease editor — biggest slice in the stack (+1338/-251). Pure SVG (no canvas — clean on the HTML-in-canvas bias). No new dependencies, bundle-neutral. Unit tests are thorough for presets, mode switching, and local round-trip. Adversarial gaps concentrate in the interactive layer: per-keystroke commits, incorrect ARIA semantics, keyboard-inaccessible drag, and a key={text} remount storm.
No blockers. Everything below is quality/UX/A11Y — fixable in a fast round.
Cross-cutting theme: interactive layer commits too eagerly
Three of the most impactful concerns share the same shape:
- Per-keystroke
onInput(EaseParamFields.tsx:98/129/164) — typing "0.42" mutates the animation three times. <input key={text}>remount storm (EaseParamFields.tsx:67) — drag frame → parent commit → new prop → input DOM node destroyed + rebuilt every mousemove.- Draft never clears on same-string commit (
EaseCurveSection.tsx:259) —useEffect([ease])misses when parent equality-checks.
Each is fixable individually; the underlying pattern is "commit is cheap, let parent re-render normalize". At the ease-editor scale it isn't — parent's AnimationCard.onUpdateMeta triggers a full card re-render + trackStudioSegmentEaseEdit telemetry on every commit. Debounce/blur-commit + stable input via ref both close the loop.
Cross-cutting theme: accessibility gaps at the interactive surfaces
aria-haspopup=\"listbox\"(EaseCurveSection.tsx:143) but popover contents are<button>grid → SR announces phantom listbox. Also no outside-click / Escape / focus trap.- Curve drag handles (
EaseCurveSection.tsx:277/432) — pointer-only, zero keyboard path.role=\"slider\"+ arrow-nudge is the standard fix. - Double-labelled inputs (
EaseParamFields.tsx:88-96) —<label>Bouncenext toaria-label=\"Spring bounce\"→ SR reads only the aria-label. Pick one. - Focus not announced on mode switch (
EaseCurveSection.tsx:92-114) — Curve→Spring rewrites the editor; no aria-live, no focus move.
Cross-cutting theme: silent-fail on grammar mismatches
Complements the concerns I raised on #2672 about the ease-grammar silent-catch chain:
- Hold ease silently rewrites to custom(...) on drag (
EaseCurveSection.tsx:176) — Hold semantics destroyed with no confirmation. - Invalid bezier paste no-op (
EaseParamFields.tsx:51) — no aria-live, no error state, no Y-clamp. - Wiggle count intermediate
1(EaseParamFields.tsx:129) — typing over "12" hitswiggle(1, ...)for a frame.
These are the client-side siblings of #2672's silent-catch chain — user takes an action, no signal on failure, animation renders wrong.
Contract dependencies on #2672 / #2673
- Consumes
parseSpringBounce,parseWiggleEase,evaluateSpringEase,evaluateWiggleEase,WiggleEaseConfigfrom@hyperframes/core/{spring,wiggle}-ease— the very subpaths I flagged as prematurely public in #2672 land HERE, so that argument's a wash: keep the subpaths, but consumer discipline (only import what's needed) applies. - Emits
custom(M0,0 C... 1,1)strings — parser #2673 must round-trip these losslessly throughresolveEaseCurveTuple. Local tests verify commit-side; NO integration test drives commit → save → reload. Please add one — this is the exact cross-slice seam where subtle grammar drift bites. EASE_CURVESingsapAnimationConstants.tsuses raw tuples that #2673's parsers must accept;easePresetLibrary.test.ts:60imports parser source directly to avoid stale dist — good, but exposes lockstep dependency.
🟢 Verified
- Pointer capture on transparent hit-area circle (
:406-411) — GC'd on unmount, no listener leak. - Preset-grid keys use stable
preset.id. EASE_PRESETStyped viasatisfies— no exported-type leakage.- Bundle-neutral: no new deps.
- Telemetry payload: no PII (
easeis a syntactic token, not user content). EaseModeToggleskipsonCommitwhen clicking the active mode (:104) — no accidental default clobber.- Canvas-HTML: clean — pure SVG, no HTML nested inside canvas or vice-versa.
- Flicker relevance:
EasePresetGridre-filtersEASE_PRESETSper render (:29) — acceptable (module-scoped array, stablepreset.idkeys). NOT the main flicker concern; theEaseBezierField key={text}remount storm is.
Nits
- Dead telemetry action:
events.tsdeclaresaction: \"open\" | \"commit\"but only\"commit\"is emitted. Either wire the\"open\"ping on dropdown toggle or drop the union. - No error boundary around the curve viz — a
NaNcoordinate from a malformedcustom(...)renders visually broken but athrowwould crash the wholeAnimationCard. WrapEaseCurveSection.
Solid UI slice, and the mode-toggle + preset-grid mechanics read clean. The interactive-layer concerns above are the ones worth folding in — most are one-line fixes at the callsites, and closing them locks the UX contract cleanly for downstream consumers.
786802f to
d84e999
Compare
8232ac4 to
c253dec
Compare
|
Review follow-up: numeric draft commits, stable inputs, accessible validation, menu keyboard/dismissal behavior, radiogroup semantics, keyboard sliders, Hold safety, same-string draft clearing, and save/reload integration coverage are now included. I did not add a component-local error boundary: malformed input is now converted to finite validated state without throwing, while a boundary would not catch event-handler failures and would add a second fallback owner. |
The base branch was changed.
vanceingalls
left a comment
There was a problem hiding this comment.
R2 Verdict: APPROVE (adversarial re-review, max tier)
Reviewed at c253dec23b0a6f1ccfe49c17371fdc9823f73b4c.
The review-hardening pass has landed — Rames' R1 concerns on eager onInput commits, key={text} remount storm, listbox/menu ARIA, keyboard-inaccessible drag, silent Hold rewrite, invalid-bezier no-op, and same-string draft-clear all read as addressed. My R1's P2 on the useEffect([ease]) derived-state clear also stands as intentional (belt+suspenders with queueMicrotask) since same-string commits still need it.
The residual findings below are all quality/UX — no correctness blockers.
P2 — parity gap in numeric-commit error surface (Editor-UI Lens H + I)
EaseParamFields.tsx:133-144 (NumericCommitInput.commit) silently reverts draft to String(value) on empty submit or out-of-range value with no user feedback. Its sibling on the same input surface, EaseBezierField (EaseParamFields.tsx:60-66), sets an inline aria-live error ("Enter four finite numbers" / "Y values must be between -1 and 2"). Same file, same surface, different feedback pattern — typing 5 into the 0-1 wiggle amplitude field silently snaps back with no explanation.
NumericCommitInput is called 3× (SpringBounceField:176, WiggleField count :204, amplitude :234) — all three inherit the silent-catch. Suggest either a passed-in error slot (or onValidationError) so callers can show inline feedback, or at minimum a visual pulse on the revert. Fix-shape: mirror EaseBezierField's useState<string|null> error + aria-live <p>.
P2 — sibling instances of R1's derived-state antipattern
R1 (mine, dismissed) called out one useEffect(() => setDraft(null), [ease]) at EaseCurveSection.tsx:347. The same "sync local edit-buffer from prop via effect" pattern now appears three more times on this input surface:
EaseParamFields.tsx:51-54—EaseBezierField(input-value + error reset)EaseParamFields.tsx:132—NumericCommitInput(draft reset)
Idiomatic React (per You Might Not Need an Effect) is either key={value} reset at the callsite or a useRef last-seen-value guard. Not a correctness bug — controlled-input edit-buffers are a legitimate escape hatch — but flagging because Miguel's precedent from the sibling ease-grammar PRs treats this pattern as worth documenting/collapsing across the file.
P3 — no Escape-to-cancel mid-drag (Adversarial Lens A)
EaseCurveSection.tsx:373-401 — once handlePointerDown sets draggingRef.current, the drag can only be resolved by pointerup / pointercancel, both of which commit via handlePointerUp. Pressing Escape mid-drag is a no-op — the document-level Escape listener is only mounted while the preset dropdown is open (:161), and the handle circle's onKeyDown (:403) filters everything but arrow keys. Studio's usual gesture contract: Escape cancels drag and reverts to pre-drag values. Fix-shape: capture pre-drag tuple on pointerdown, listen for Escape during drag, restore tuple + clear draggingRef.
P3 — mode-toggle wipes custom curve unconditionally (Adversarial Lens B)
EaseCurveSection.tsx:111-113 — EaseModeToggle.onClick commits DEFAULT_EASE_BY_MODE[candidateMode] when switching, so Curve (hand-tuned bezier) → Spring → Curve returns you to power2.out, not your previous custom curve. No "modified" indicator on the toggle, no "restore custom" affordance. Low-impact but easily confusing on a discovery/experiment workflow. Suggest either a per-mode remembered-last-value ref, or at minimum a confirm on discard when the current curve differs from DEFAULT_EASE_BY_MODE[mode].
P3 — arrow-key nudge = one commit per keypress (Adversarial Lens C)
EaseCurveSection.tsx:403-411 — handleKeyDown calls onCustomEaseCommit synchronously on every arrow-key press. If the parent's undo scaffolding treats each onUpdateMeta as an entry (AnimationCard.tsx:301), a sustained Shift+ArrowRight becomes N undo entries per drag-equivalent motion. Drag commits once (pointerup), keyboard commits N — asymmetry. If undo does coalesce upstream, ignore.
Adversarial lenses fired
Editor-UI (of 12): h (sibling helpers on same surface), i (parity audit: bezier vs numeric feedback).
R2 (of 5): A (drag correctness — Escape cancel), B (preset switching state hygiene), C (undo integration — per-nudge coalescing).
Lenses D (live-preview perf), E (cross-mode timing-editor drift) — inspected, nothing actionable. Live-preview curvePathFor recomputes 64 samples on wiggle/spring per render, but the render window is small and driven by handle drag only. No timing-editor cross-ref surfaces in this diff.
— Via

What
Add the complete Studio keyframe ease editor: preset selection, Curve/Spring/Wiggle modes, parameter fields, editable curve visualization, and telemetry.
This is Family A, PR 3 of 4. Prerequisite:
codex/studio-timeline-a-parser-intent-v2. Next:codex/studio-timeline-a-array-timing-v2.Replacement review history: #2572 and #2562. Golden reference: #2387.
Why
Users need one coherent editor for every easing mode. The controls and inspector consume the same preset and curve contracts, so publishing only one half creates an invalid intermediate UI and a non-typechecking branch.
How
The editor centralizes preset metadata, curve SVG rendering, parameter controls, inspector composition, and analytics events.
Review hardening makes numeric inputs draft locally and commit once on blur/Enter, preserves a stable Bezier input node, validates malformed/out-of-range curves with an accessible error, implements menu dismissal/focus/arrow-key semantics, exposes mutually exclusive modes as radios, makes Bezier handles keyboard-operable sliders, prevents Hold from being rewritten by drag, and clears same-string drag drafts. A writer-plus-parser integration test verifies custom ease commit → save → reload.
This PR is the recorded Family A size exception: splitting it further would introduce temporary plumbing, duplicate APIs, and an independently broken intermediate branch.
Test plan
main