feat(studio): bulk-edit easing for merged keyframes - #2693
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
0741937 to
463429b
Compare
1c92c81 to
2739a21
Compare
463429b to
4802f0e
Compare
2739a21 to
c02a44c
Compare
4802f0e to
8f5f4d2
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 8f5f4d2.
Feature completes what C1 laid the foundation for — clean plumbing from the collision list (collidingAnimationTargets on the focused segment) through AnimationCard → EaseCurveSection → the new handleUpdateSegmentEase → batched update-keyframe commits. Test coverage across the seams is thorough (AnimationCard bulk vs. single branching, EaseCurveSection copy, GsapAnimationSection element-scoping, propertyPanelFlatMotionSection forwarding, useDomEditSession batch/single/empty).
No blockers. Three inline comments below, plus a question and one observation.
Question — is commitMutation.batch(calls, options) atomic?
The bulk-edit fans out N update-keyframe mutations for the same file. useGsapScriptCommits.ts:391-397 shows .batch calls runBatchCommit(activeProjectId, activeCompPath, file, calls, options) through the file serializer — so the file lock is held across all N. But is runBatchCommit itself all-or-nothing (single read-modify-write of the source with all N patches applied atomically), or does it iterate N sub-writes under one lock? If the latter, a mid-batch failure leaves the file with some colliding tweens updated to the new ease and others still on the old — silent divergence, and the toast-less path (see observation) means the user wouldn't know. I didn't chase this into runBatchCommit; would appreciate a confirmation.
Observation — bulk-edit takes the raw commitMutation, not commitMutationSafely.
useDomEditSession.ts receives commitMutation (raw) from useGsapScriptCommits. Both the single-target and multi-target branches call the raw one, so on save failure there's no showToast(\"Couldn't save animation: …\") + telemetry flow — same as the pre-PR single-edit path, so not a regression, but the bulk path amplifies the blast radius (one failed save = N tweens silently unchanged). If atomicity above is guaranteed, this is minor; if not, worth routing bulk through the safe wrapper.
What I didn't verify:
- The
updateGsapMetapath for flat tweens (onUpdateMeta({ ease })atAnimationCard) — tested for the flat case in isolation but I didn't trace whether the collision-detection logic in C1 would ever produce afocusedSegmentfor a flat animation. Flat tweens don't go throughdeduplicateKeyframes, so theircollidingAnimationTargetsshould always be undefined; the test atAnimationCard.test.tsxline ~236 asserts this indirectly by expectingonUpdateMetato fire alone. Should be fine.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review — via
Grade: B+
Overall: CORRECT
Thesis check: The diff delivers what the title says. C2 wires the collidingAnimationTargets payload C1 already accumulates through focusedEaseSegment into AnimationCard, and routes the ease commit through a new onUpdateSegmentEase bulk callback whose backend is a single gsapCommitMutation.batch(...) producing one atomic undo entry labeled "Update segment ease". The discriminator focusedCollidingAnimationTargets.length > 1 is safe because accumulateCollidingAnimationTargets (in gsapTweenSynth.ts) only populates the array with [primary, …others] on the first collision, so length is either 0/undefined (no collision, single-id path) or ≥2 (merged, bulk path). Single-target callers (handleUpdateKeyframeEase) now flow through the same handleUpdateSegmentEase helper, collapse via calls.length === 1 to the identical prior gsapCommitMutation(sel, mut, {label: "Update keyframe ease", softReload: true}) invocation — no regression in the single-keyframe path.
P0/P1 findings: none.
P2/P3 findings:
- [P2]
packages/studio/src/components/editor/gsapAnimationCallbacks.ts:127—onUpdateSegmentEaseis passed throughwithTrackedGsapAnimationCallbacksun-wrapped (rawcallbacks.onUpdateSegmentEase), while its peersonUpdateKeyframeEase(line 142) andonSetAllKeyframeEases(line 148) both wrap withtrack("select", "Keyframe ease")/track("select", "All keyframe eases"). Telemetry gap on the parallel branch: users bulk-editing merged easing skip the design-input tracking channel.trackStudioSegmentEaseEdit({action:"commit",ease})still fires fromAnimationCard.tsx:315, so there's some signal — but the design-input funnel goes dark for exactly the new affordance.- Failure scenario: PM measures "keyframe ease selects per session" on the design-input dashboard, sees adoption of the merged-easing feature look artificially low because every multi-property commit is invisible.
- [P2]
packages/studio/src/components/editor/propertyPanelFlatMotionSection.test.tsx:743—usePlayerStore.getState().reset();at module top level (outsideafterEach/beforeEach). Contrast the siblingGsapAnimationSection.test.tsx:487wherereset()is correctly insideafterEach. Module-level reset runs once at import; if a prior file in the same worker mutated the store, the added "flat tween bulk segment" test starts with dirty state and can flake.- Failure scenario: parallel test worker reorders →
useDomEditSession.test.tsx's bulk-segment test leavesfocusedEaseSegmentnon-null → this file'ssetState({focusedEaseSegment: ...})still sets its own, but any incidental state (e.g.,selectedElementId) leaks intorenderInto'sFlatMotionSectionand changes card mount order.
- Failure scenario: parallel test worker reorders →
- [P2]
packages/studio/src/components/editor/PropertyPanel.tsx:560vspackages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx:146— elementId derivation is duplicated but not identical: PropertyPanel uses`${element.sourceFile || "index.html"}#${element.id}`(with a fallback), while the flat panel uses`${element.sourceFile}#${element.id}`(no fallback).DomEditSelection.sourceFileis typedstring(required), but empty string is not excluded by the type. If any resolver ever emits an emptysourceFile, PropertyPanel routes focus correctly ("index.html#hero") while FlatMotionSection routes to"#hero"— which will never match the writer's"index.html#hero"payload, and merged-easing silently disables in flat mode. Extract to a sharedrenderedElementIdOf(selection)helper so the two branches can't drift.
Nits:
packages/studio/src/hooks/useDomEditSession.ts— the deletion of the section comment banners (// ── Types ──,// ── Hook ──,// ── Selection ──,// ── Agent modal ──,// ── Preview interaction ──,// ── GSAP-aware geometry intercepts + animated property commit ──) is unnecessary churn in a feature PR — those banners were navigation aids in a 500-line hook. Consider reverting the cosmetic-only deletions.packages/studio/src/hooks/useDomEditSession.ts:483-486—if (calls.length === 1) { const call = calls[0]; if (call) void gsapCommitMutation(...); return; }. Theif (call)guard is dead: length-check guaranteescalls[0]is defined. Harmless but signals uncertainty about invariants that TS already proves.packages/studio/src/hooks/useDomEditSession.ts:487—void gsapCommitMutation.batch?.(calls, options). The optional chain is a silent-fail guard for a method thatuseGsapScriptCommitsunconditionally assigns (line 391 of that file). If.batchwere ever undefined at runtime, the batch commit silently does nothing (no error, no toast). Prefer removing the?.and letting TypeScript enforce presence via theCommitMutationtype.packages/studio/src/components/editor/AnimationCard.tsx:62-64—focusedCollidingAnimationTargetsis shadow state derived fromfocusedSegment.collidingAnimationTargets. Since it's only cleared ononToggle(and manual toggle-vs-refocus is a per-render decision), consider deriving viauseMemofromfocusedSegment+expandedKfPctinstead of duplicating into a separateuseState. Minor.
Positive callouts:
- Single-target path collapses through the same helper (
handleUpdateSegmentEase([{animationId, tweenPercentage}], ease)) — same undo label, same mutation shape, same softReload. No hidden divergence between the "single-keyframe-ease" and "bulk-segment-ease" flows. - Undo/redo entries are per-batch (
runBatchCommit → finalizeSuccessfulMutationfires once with the last call's selection/mutation) — a bulk edit undoes as one operation, which matches user intent. - The
elementIdfilter onGsapAnimationSection(line 60-61 diff) closes the shared-animation-id bug where a class-selector tween would light up cards for every element carrying that class. Test atGsapAnimationSection.test.tsx:530explicitly covers this. EaseCurveSection's "Applies to N properties" hint only shows whencollidingAnimationTargets.length > 1(line 425-429 ofEaseCurveSection.tsx), matching the bulk-dispatch discriminator exactly. Signal and action are consistent.- Test coverage is thorough: focus consumption per-element, bulk vs single dispatch, flat-tween meta path, single-id colliding fallback, empty-target guard, and the batch call shape (arguments passed to
.batch()verified verbatim).
Sweep-fix breadth check:
- Peer sites grepped for ease writers in
packages/studio/src/:onUpdateKeyframeEase,handleUpdateKeyframeEase,handleSetAllKeyframeEases,onUpdateMeta({ease}),onUpdateMeta({easeEach}). - Peer sites covered by PR: keyframed animation ease commit via
KeyframeEaseList → EaseCurveSection(bulk-aware); wiring plumbed throughStudioRightPanel.tsx,DomEditContext.tsx,PropertyPanel.tsx,PropertyPanelFlat.tsx,GsapAnimationSection.tsx, andAnimationCard.tsx. - Peer sites MISSED: flat-tween ease (
AnimationCard.tsx:325-348, theSelectField/EaseCurveSectionbranch that firesonUpdateMeta({ease})/onUpdateMeta({easeEach})) does not consumefocusedCollidingAnimationTargets. This is likely intentional — a truly flat tween has no per-segment ease to merge — but the PR description doesn't explicitly document this exclusion. If merged-flat-tween diamonds are a real UI state, they'd bypass bulk-easing. Confirm with a comment or add a follow-up.
Standards-lens mechanical checklist:
- Empty-count OK: PASS (
if (!selection || targets.length === 0) return;atuseDomEditSession.ts:475). - Error-boundary present: N/A (no new async component).
- Disposal ordered: PASS (
focusedCollidingAnimationTargetscleared on manual toggle). - Telemetry hooks fired: PARTIAL —
trackStudioSegmentEaseEditfires; design-inputtrack("select", "Keyframe ease")does not fire for the bulk path (see P2 #1). - No bare
as T: PASS. - No non-null
!: PASS. - No untyped-catch
.message: PASS.
— Review by Via
c02a44c to
ab97f8f
Compare
8f5f4d2 to
5197e76
Compare
ab97f8f to
7e6b3e6
Compare
5197e76 to
c3879a2
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta-reviewed 8f5f4d21..c3879a2b.
All three prior concerns are addressed, and two of them landed better than I asked for:
EaseCurveSection.tsx:427copy fixed to "Applies to N animations" with matching test update. ✅useDomEditSession.ts:487— the silent-drop.batch?.(...)is replaced with an explicitif (batch) { batch(...); return } else { serial fallback }plus a new test atuseDomEditSession.test.tsx:410thatReflect.deleteProperty(gsapCommitMutation, "batch")and asserts every tween is written. The comment ("A wrapped commit (a gesture transaction, say) carries no batch transport") captures the real case the optional-chain would have silently swallowed. Better than the "drop?." I suggested.- The
${sourceFile}#${id}string alignment is centralised intoscopedElementKey(element)inpackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:174-189. The docblock names the exact class of bug the helper is preventing ("Four call sites built this string by hand and two of them omitted the index.html fallback"), and the diff confirms that PropertyPanelFlat.tsx:258 and propertyPanelFlatMotionSection.tsx:146 were in fact building${undefined}#foofor elements with nosourceFile— those are two real prior bugs the helper closes, on top of the one I flagged.
Concerns
- Preflight (format:check) is red —
oxfmt --checkfails onpackages/studio/src/hooks/useDomEditSession.ts. A quickbun run format:fixshould clear it; the file is the very one you rewrote for the batch-fallback fix. - preview-regression is red —
PREVIEW_PARITY_RESULT: skippedcounts as failure. Stacked-PR preview parity gate; may just need a rebase / re-run once #2692's landing state settles, but flagging so it doesn't sit unaddressed.
What I didn't verify
- Any
element.sourceFile === null(not justundefined) path —scopedElementKeyuseselement.sourceFile || "index.html"which handles bothnullandundefinedidentically, but I didn't grep every producer to confirm null is unreachable.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R2 review — via
HEAD verified: c3879a2bd3c63dcdb5f17e5e65b9ca72ac84e8a5
Grade: A- (down half a notch for CI-red on the fix-touched file)
Overall: COMMENTED — one CI blocker to clear, then this is a stamp.
Delta-reviewed 8f5f4d21..c3879a2b against R1 (mine + Rames').
R1 findings delta
-
[P2 Via / telemetry gap on
onUpdateSegmentEase] — RESOLVED.packages/studio/src/components/editor/gsapAnimationCallbacks.ts:127-132now wrapsonUpdateSegmentEasewithtrack("select", "Segment ease"), matching the peersonUpdateKeyframeEase(line 148) andonSetAllKeyframeEases(line 153). Test coverage atgsapAnimationCallbacks.test.ts:83-86,123assertstrack:select:Segment easefires exactly once before the mutation. Design-input funnel no longer goes dark on the bulk path. -
[P2 Via / module-scope
reset()in flat-motion test] — RESOLVED.packages/studio/src/components/editor/propertyPanelFlatMotionSection.test.tsx:12-17movesusePlayerStore.getState().reset()intoafterEach, with a comment ("The store is module-global, so a test that parks a focused ease segment would otherwise leak it into every test that runs after it") capturing the exact failure mode. Matches the siblingGsapAnimationSection.test.tsx:36-39shape. -
[P2 Via / elementId derivation drift PropertyPanel vs. FlatMotionSection] — RESOLVED. Both call sites now delegate to
scopedElementKey(element)frompackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:166-171. Docblock (lines 161-165) explicitly names the class of bug it's preventing ("two of them omitted the index.html fallback"). Call sites:PropertyPanel.tsx:561,propertyPanelFlatMotionSection.tsx:147. Extraction fix quality is above bar — a docblock-attributed helper is what I hoped for. -
[P2 Rames / "Applies to N properties" counts animations, not properties] — RESOLVED.
EaseCurveSection.tsx:427copy now readsApplies to {n} animations, matching Rames' most-accurate suggestion. Test atEaseCurveSection.test.tsxupdated in the same commit. -
[P2 Rames /
.batch?.(...)silent-fails a real bug into no-op] — RESOLVED, and landed better than asked.useDomEditSession.ts:482-497now:if (calls.length === 1)→ singlecommitMutation(matches prior semantics).const batch = gsapCommitMutation.batch; if (batch) { void batch(calls, options); return; }— no optional chain.- Else falls back to serial
.commitMutationcalls per tween, with a comment ("A wrapped commit (a gesture transaction, say) carries no batch transport…an optional-chained call here would silently drop the whole edit instead") that captures the real case the?.would have swallowed.
New test atuseDomEditSession.test.tsx:411-422Reflect.deleteProperty(gsapCommitMutation, "batch")and asserts every tween is written via serial calls. This is Rames' Option 2, executed correctly.
-
[P2 Rames / producer↔consumer two-string scoping] — RESOLVED for the happy path.
scopedElementKey(line 166) emits${sourceFile || "index.html"}#${id}which matchesbuildTimelineElementKey's domId-happy branch intimelineElementHelpers.ts:295-296(${scope}#${params.domId}). Consumer-side drift is now closed, and the docblock names the exact bug prevented. Rames upgraded this to ✅ in his R2 and I agree.- Residual (non-blocking, nit):
buildTimelineElementKeystill has a colon-shape selector-fallback (${scope}:${selector}:${idx}, line 297) and an id-fallback (${scope}:${id}:${fallbackIndex}, line 298) thatscopedElementKeydoes not mirror. If a producer element ever reaches those branches (selector-only, no domId),focusedEaseSegment.elementIdwouldn't match the consumer's${scope}#${element.id}. In practice studio elements have domIds so this is unreachable, but a future refactor could unify both sides via a single helper if the selector-only path becomes hot.
- Residual (non-blocking, nit):
-
[Body-Q Rames / is
runBatchCommitatomic?] — NOT ADDRESSED in the commit or in comments from Miguel.useGsapScriptCommits.ts:348-359runBatchCommitposts ALL N mutations via onemutateGsapScriptBatch(pid, targetPath, mutations)request (single POST to/api/projects/.../gsap-mutations-batch/...— see line 65), and onlyfinalizeSuccessfulMutationruns once for the last call. Client-side is a single request under the same file-serializer lock; whether the server-side handler applies all N as one RMW vs. iterates N sub-writes is out of this diff's visibility. If server-side is iterative, a mid-batch failure leaves the file with some tweens on the new ease and others on the old — no toast, no revert. Not blocking, but worth a follow-up confirmation from whoever owns the batch endpoint.
Fresh-pass findings (new code the fix introduced)
- [P1] CI Preflight is red on the fix-touched file.
oxfmt --checkfails onpackages/studio/src/hooks/useDomEditSession.ts(run https://github.com/heygen-com/hyperframes/actions/runs/30405054272/job/90428578062). Onebun run format:fixshould clear it. Same call-out Rames raised in his R2 — real merge blocker, but self-healing. - [P2]
preview-regressionis red —PREVIEW_PARITY_RESULT: skippedcounts as failure. Since #2693 sits atop #2692 in the stack and #2692 just approved a minute before this HEAD, a rebase/re-run once the stack settles is likely enough. Flagged so it doesn't linger. - [P3,
scopedElementKeynit] the helper uses||(falsy) rather than??(nullish) — correct for the intent since empty-string sourceFile should also default to"index.html", but no defensive fallback for emptyelement.id(returns${scope}#). In practice unreachable, so no ask. - Fresh-pass 12-lens sweep: clean. Empty-count guard PASS (
if (!selection || targets.length === 0) return;atuseDomEditSession.ts:466). Serial-fallback branch covered by test.elementscope inuseCallbackdeps correct. No newas, no new!, no untyped catch. Fast-Refresh:gsapAnimationCallbacks.tsexports only functions/types, no components — safe. No new.map()into unbounded scroll container. Scope carve-out (single-target fast-path,calls.length === 1) is honored in code — verified atuseDomEditSession.ts:482-485matches the priorhandleUpdateKeyframeEaseshape exactly.
Cross-stack context (#2692 gate)
Base of #2693 is codex/studio-timeline-c-exact-targeting-v2 at 7e6b3e64 = #2692's tip. #2692 was just approved (2026-07-28T22:51:12Z, vanceingalls) and is MERGEABLE, so bundle-merging the stack (#2692 → #2693 → #2694 → #2695 via Graphite) is the intended landing path. scopedElementKey lives on #2693, not #2692, so the mid-stack landing gap concern doesn't apply here — the helper is visible from this PR's diff (gsapKeyframeCacheHelpers.ts new export).
Positive callouts
- Fix quality is uniformly above the ask — every P2 landed with test coverage that exercises the specific failure mode (batch-missing, module-store leak, telemetry emission, N-count copy). Nothing was rubber-stamped.
scopedElementKey's docblock names the drift bug directly ("two of them omitted the index.html fallback") — sets the discipline for future refactors and closes two prior latent bugs on top of the one my R1 flagged (per Rames' independent audit).- The batch → serial fallback path is not just tested but reasoned about in-code (the comment about wrapped commits / gesture transactions) — matches Rames' "explain the invariant" bar.
Verdict
Substantively clean, but CI red on oxfmt for the fix-touched file. Run bun run format:fix on packages/studio/src/hooks/useDomEditSession.ts, push, and this stamps once Preflight goes green.
— Review by Via
29d16df to
d93ce4c
Compare
7e6b3e6 to
e0ad04d
Compare
d93ce4c to
2c854ec
Compare
e0ad04d to
497a640
Compare
7f51aa9 to
23ab104
Compare

What
Apply ease edits to every authored animation represented by a merged timeline keyframe.
Why
When colliding keyframes render as one diamond, changing its easing must update the complete selected semantic target. Updating only one hidden animation makes the inspector and timeline disagree and can silently leave mixed easing behind.
How
This is C2 of the independent Family C draft Graphite stack.
Test plan
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.