fix(studio): author every new tween against one element - #2849
Conversation
…me percentages A ruler press with no pointer movement settled the playhead at t=0 instead of the clicked time. handlePointerUp replays pendingClientXRef, which only the pointermove path wrote, so a plain click fell back to the ref's initial 0 and overwrote the correct pointerdown seek. Seed the ref on pointerdown. The keyframe retime move branch also returned the raw quotient while the resize branch rounded to 3dp, so values like 74.81203007518799% landed in the user's source and churned the diff on every drag. Round at the point of computation so the no-op test and the written value agree.
The 24x24 WCAG 2.5.8 overlay sits on a wrapper that outranks the diamonds, so on a segment narrower than 24px it overhung them and won their hit test at fit zoom. Gate the overlay on the clear span between the two diamonds and let the button keep its 16x16 box below that. Also renames the pointer target suite to say it asserts the classes that produce the size, not the measured geometry, which happy-dom cannot see.
…ide press Split hex-draft ownership so the hex input is the sole author of its own text while editing (updateColorDraft no longer stamps a canonical hex back over every keystroke), fixing snap-back on backspace and the silent wrong-colour clobber on non-repeating hex values. Route hex typing through the shared gesture transaction so outside-click and Escape settle/cancel it like the other inspector fields, instead of relying on a private onBlur commit that never fires once the panel unmounts on outside-click.
Swap the panel's hand-rolled bubble-phase mousedown listener for the shared useContextMenuDismiss hook, which adds Escape support and fixes outside-click dismissal when a canvas gesture (e.g. marquee start) calls preventDefault on pointerdown, which otherwise suppresses the mousedown compat event entirely. Also wires up dialog ARIA (role, aria-modal, id/aria-controls) between the trigger and panel.
The panel does not trap focus and leaves the rest of the editor operable, so aria-modal would tell assistive tech the whole app is inert while it is open. role=dialog plus aria-controls and Escape is the correct non-modal disclosure shape.
The gesture resolver gated on six digits while parseCssColor accepts both lengths, so #F00 previewed as nothing and committed nothing. The old onBlur path parsed it, making this a behavioural loss rather than a pre-existing gap. Also asserts that an incomplete hex is restored on outside-click, not merely left uncommitted.
A sub-composition tween's resolvedStart is composition-local, while the timeline element resolved for it is the sub-comp HOST, whose start is main-timeline absolute. toClipPercentage subtracted the two frames from each other, so a host mounted at 1.5s cached its 0s tween at -12% and its last tween's end keyframe at 88% instead of 100%. A clip-relative percentage can never be negative. resolveClipTimingBasis now returns the clip start in the frame the tween's own times are measured in: the composition mount (expandedParentStart for an expanded child, the parent composition clip's start otherwise, 0 for a root-composition element) is subtracted, and a sub-comp inner element that falls back to its host's window starts at 0 in that window. It moves to gsapShared so the post-commit cache writer can share it instead of resolving its own basis, which also gives that writer the sub-comp host fallback it was missing.
buildExpandedElements synthesized DOM-only sub-composition children against the top-level element rather than the parentHost it resolves immediately after. Under two-level nesting every child row therefore inherited the top-level window instead of its own host's, so the rows drew at the wrong offset and duration.
The sub-composition drill-in replaced the host row with its children. Since expansion is also driven by the playhead alone (paused auto-expand), an ordinary seek into a sub-composition made the host row disappear, taking its keyframe lane with it: diamonds render per row from keyframeCache.get( elementKey), so no row means no diamonds. Reproduced live with no drag at all, seek 0 gave 3 diamonds, seek 7.68 gave 0, seek 0.2 gave 3. Make the expansion additive instead. The host row stays and its children are appended directly below it. The synthetic fractional lanes already used for children sit strictly between the host's lane and the next integer, so the host keeps its own row without colliding with anything. The time-keyed auto-expand itself is unchanged.
23bed54 to
d6aa760
Compare
91883d1 to
d32b0b7
Compare
Drilling two levels deep spared only the top-level row, so the middle host lost its row and its keyframe lane with it. Spare every host between the drilled one and the top, and anchor the children under the deepest host that actually has a row. Also stops resolveClipTimingBasis handing back a main-timeline start when a clip names a parent composition that is absent from the element list. The mount is unknowable there, so the child's own window is the only safe frame.
Two halves of one inversion in the expanded timeline lanes: the tweens
that should show were filtered out, and a tween that should not be there
was the only survivor.
Lane classification read the parser's whole-tween verdict, which is
undefined for anything spanning more than one property group. `{x,
opacity}` is the canonical HyperFrames entrance tween, so five of the
seven tweens in the swiss-grid graphics example had no caret, no
reserved row and no diamonds. Classify per property instead, through one
helper both the rendered lanes and the reserved row heights count
through so they cannot drift again.
Attribution matched an unanchored leading id, so `#stat3 .block` was
filed under `#stat3`. The child's diamonds landed on its ancestor and
collided with the ancestor's own tween at the shared percentage, which
the same-percentage merge then resolved by dropping the ease. Route
attribution through resolveSelectorElementIds, which anchors a
whole-selector id and otherwise resolves through the live preview DOM,
and anchor its no-DOM fallback so a descendant selector resolves to
nothing rather than to its ancestor. The merge rule is unchanged.
Also brings the last property-lane call site onto the shared clip timing
basis: an expanded sub-composition child's start is host-absolute while
its tweens are local to its own file.
d32b0b7 to
3140c95
Compare
d6aa760 to
9cec37c
Compare
animIdForProp matched on the parser's whole-tween propertyGroup, which is
undefined for a legacy mixed tween such as {x, opacity}. Such a tween never
matched, so an edit to either property fell through to the selection's
default animation, a different tween than the lane the user is editing.
Resolve through animationLaneGroups, the same per-keyframe helper the
rendered lanes and the reserved row heights already count groups with.
"Add keyframe at playhead" on an element with no id authored the bare class
buildStableSelector hands back, so one add on a `.group` wrote
`tl.to(".group", ...)`: a tween that animates all five siblings and that
resolveSelectorElementIds reads back as all five, collapsing their timeline
rows into one. It survived a reload, so the written file stayed un-editable.
writeTargetSelector is the write-side counterpart to selectorFromSelection
(which must keep returning the exact string findTweenAtTime compares against).
It resolves the element's own identity to a selector that addresses exactly
one element: `#id`, else `[data-hf-id="..."]`, else the selection's selector
when it is already unique, else a `:nth-child` path anchored on the nearest
identifiable ancestor (the selector + selectorIndex pair, resolved through the
DOM the index was counted in).
Applied to the two paths that author a NEW tween: the no-animation branch of
useEnableKeyframes and commitKeyframeAtTimeImpl. replace-with-keyframes still
writes the selection's own selector, since retargeting a tween the author
aimed at a whole group is a different decision from adding a keyframe.
writeTargetSelector returned the selection's bare selector whenever the structural walk failed, including when a live DOM was there to check against. An element detached between selecting and committing takes that path, so the add re-authored the exact `.group` string the function exists to replace. Return null instead: a failed walk against a live DOM is evidence, not absence of it. Callers that cannot drop a user edit opt back in with `?? selectorFromSelection` where the trade is visible. The replace-with-keyframes paths had the mirror defect. The server deletes and re-adds the tween, so their target string is a full rewrite, and they derived it from the selection: promoting a set on a tween already narrowed to `#scene > div:nth-child(3)` widened it back onto every class sibling, undoing the narrowing an earlier add had made. They now keep the tween's own authored target, matching what eight sibling commit modules already do.
…he lanes The timeline's track key is a fractional z-order sort value, and the header built its visibility label straight from it, so screen readers announced "Hide track 0.16666666666666666". A track's 1-based display row is now passed alongside the key: the row number goes in every label, the key keeps routing every callback (visibility toggle, lane context menu). The same fix covers the `Track N` fallback used when a track holds no labelled element. The layer disclosure caret's aria-controls named a div in the sticky label column. That subtree is not empty, it holds the per-lane keyframe controls, but its children are all absolutely positioned so the div computes to 0x0, and the diamonds the caret visibly reveals live on the canvas instead. The caret expands two disjoint subtrees and was naming the less useful one. TimelinePropertyLanes now renders one static wrapper (static, not relative, so it establishes no containing block and the absolutely-positioned lanes keep resolving against the track-content div with identical geometry) and takes the id. TimelineLanes mints that id, since it is the only place that sees both ends of the disclosure, and mounts the wrapper for the track's keyframe clip in both disclosure states so the reference still resolves while collapsed. TimelineLaneBaseProps moves to its own module: it is the contract shared by TimelineCanvas and TimelineLanes, and lifting it out keeps TimelineLanes.tsx well under the 600-line cap instead of pushing past it.
The timeline track key is a fractional z-order sort key: an expanded sub-composition child gets `host.track + n / (siblings + 2)`. The undo history entry for the eye toggle interpolated that key directly, so hiding an expanded child recorded "Hide track 0.16666666666666666". Give the key-to-display-row conversion a single owner (timelineTrackDisplay.ts) and route both the track header labels and the history label through it, so the two cannot drift apart again. The raw key still routes the callbacks and lookups that need it. Adds a regression test that toggles a track keyed 1 / 6; a test on track 0 formats cleanly and proves nothing.
3140c95 to
643e56b
Compare
9cec37c to
477916c
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 643e56b.
Top of the stack. The write/read pairing (writeTargetSelector for the write half + tweenTargetsElement for the read half) is the right shape, and the test surface is thorough at both the writer and the hook layer. One writer got missed, though — same class of bug the PR exists to close.
Blocker
commitCreatePath at packages/studio/src/components/editor/motionPathCommit.ts:80 is an uncovered new-tween writer — reachable from the "set destination" motion-path toolbar toggle in MotionPathOverlay.tsx:190. Traces the same shape the PR fixes for commitStaticGsapPosition et al.:
createSelector = selectorFor(selection)atMotionPathOverlay.tsx:133— falls through to baresel.selectorfor an id-less element (seemotionPathSelection.ts:9-13).- That string is passed straight to
commitCreatePath(createSelector, ...), which emits{ type: "add-motion-path", targetSelector, ... }— a fresh GSAP tween on the target. - Repro: select an id-less element that shares its class with siblings, arm "set destination", click on canvas → the new motion-path tween is authored over every sibling sharing the class. Exact bug the PR's
ensureElementAddressable/newTweenTargetmachinery exists to prevent, just from the motion-path entry point.
Neither the PR body's "nine call sites reproduced" list nor the "Investigated and deliberately excluded" list mentions this path, so I'm reading it as an oversight rather than a scope carve-out — a motion-path creation on an id-less element is authoring a new tween, not retargeting an existing one. Fix shape mirrors the other new-tween sites: run the selection through writeTargetSelector(selection) ?? createSelector at the call site in MotionPathOverlay.tsx:190, or mint an id via ensureElementAddressable if the element deserves an addressable name in the source (probably yes here — a motion-path anchor is the kind of thing an author will want to reference again).
Concerns
-
Fallback semantics deviate from
writeTargetSelector's new contract, silently.writeTargetSelectornow returnsnullwhen a live DOM is present but the structural walk fails — per its rewritten docblock, refusing to hand back the input is the whole point. Every caller here —newTweenTargetatgsapDragCommit.ts:27,addGlobalStaticSetatuseAnimatedPropertyCommit.ts:254,useAnimatedPropertyCommit.ts:517,useGestureCommit.ts:174— falls back to the same bareselectorvia?? selector. In the "no DOM to disambiguate" case that's what the shared contract asks for. In the "DOM present but structural walk failed" case, the fallback re-authors the exact bare selector the function exists to replace.commitKeyframeAtTimeImplin the base PR has an explicit comment naming this trade ("no graceful no-op to fall to"); the four sites here don't. PromotingnewTweenTargetfromgsapDragCommit.tsto a shared export ingsapShared.ts— colocated withwriteTargetSelectorand carrying the same rationale docblock — would keep the whole family on one story. -
tryGsapDragInterceptfirst-branch exact match reads as a hot-path optimization but isn't named.gsapRuntimeBridge.ts:214—posAnim && isInstantHold(posAnim) && posAnim.targetSelector === selectorshort-circuits beforefindExistingPositionWrite. When a previous write was narrowed (#scene > div:nth-child(3)) and the new drag'sselectoris still.group, exact equality fails and the generaltweenTargetsElement-driven find recovers correctly. Behaviour is fine — a one-liner comment ("exact-match fast path; general element-match happens in the fallback") saves the next reader a trip through the callee. -
First-parsed wins between overlapping tweens.
findExistingPositionWrite'sanimations.find(...)returns whichever tween the parser saw first. If a source ever holds BOTH a legacy.groupset AND a narrowed#group-2set that both match the element (mid-migration state), a nudge routes to the first-parsed. Preferring the more-specific match would be nicer UX, but this is a natural follow-up seam if a legacy migration lands, not a regression here.
Nits
-
tweenTargetsElement'stry { element.matches(targetSelector) } catch { return false }swallows any error atgsapShared.ts:315-319. Right for malformed selectors; atypeof targetSelector === "string"guard would be more explicit about what it's catching. Defensive-only — all real call sites pass a string. -
useGsapAnimationOps.addWithKeyframes(line 222) accepts a raw selector, has no callers inpackages/studiotoday (grep\.addWithKeyframes\(returns nothing outside its own definition/test), but the exposed API would bypass the write-target guard if a future caller adopts it. Either wire it throughwriteTargetSelectorinternally or delete the surface.
Confidence notes (what I didn't verify)
- Did not run the Studio suite locally; relying on the "3059 passing, 0 failures" claim in the PR body. The test surface here is thorough enough that this feels safe: every enumerated writer has a "authors against one element" pin, the retarget-existing paths have "keeps the group target" pins, and the idempotency contract has a "finds the write the first nudge authored" pin.
- The
commitCreatePathfinding is grep-verified against the tree, not repro'd in a running Studio — but the code path is a straightforward read.
Overall shape is right and the base PR's rewritten writeTargetSelector semantics + this PR's tweenTargetsElement companion are a clean pairing. The motion-path miss needs closing to hit the "every new-tween writer" bar Miguel set in the PR body.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Framing
The invariant being enforced: every mutation whose type is "add" or "add-with-keyframes" must carry a targetSelector that addresses exactly one element. writeTargetSelector is the ladder that resolves it; tweenTargetsElement is the read-half so the next nudge finds the write it just made. Retargets of existing tweens (replace-with-keyframes, update-properties, commitWholePathOffset) intentionally keep anim.targetSelector.
Verified at head 643e56b4 against the parent stack (fix/studio-track-numbers-aria). Enumerated every type: "add" / type: "add-with-keyframes" site in packages/studio/src/hooks/:
gsapDragCommit.ts— 4add, 1add-with-keyframes— routed throughnewTweenTarget(selection, selector).useAnimatedPropertyCommit.ts—addGlobalStaticSetand the same-group first-keyframe branch —writeTargetSelector(selection) ?? selector.useGestureCommit.ts— twoadd-with-keyframesbranches —writeSelector = writeTargetSelector(sel) ?? selector.useEnableKeyframes.ts:560—add-with-keyframes—writeTargetSelector(sel)with a strict null-guard that drops tohandleGsapAddAnimation("to")(safer than the?? selectorfallback, see P2 below).gsapDragPositionCommit.ts:446—add-with-keyframes— deliberately excluded, usesanim.targetSelector(convertingfrom()to keyframes carries the existing target forward).useGsapAnimationOps.ts:172"Add animation" button — routed throughensureElementAddressable(mints an id whenselection.selectoris non-unique, aborts if id-write fails).
Read-half sites updated with tweenTargetsElement: findExistingPositionWrite, findRotationSetAnimation, findSizeSetAnimation, the property panel staticWrites filter, and the gesture position lookup — every caller passes selection.element. No stray callers.
The "9 call sites" claim in the PR body checks out (I counted 10, including both useAnimatedPropertyCommit branches).
Findings
P2 — ?? selector fallback silently re-introduces the widened write on 5 sites
packages/studio/src/hooks/gsapDragCommit.ts:23-25 (newTweenTarget), useAnimatedPropertyCommit.ts:256 and :517, useGestureCommit.ts:180, and the same pattern already in gsapKeyframeCommit.ts from the parent stack.
writeTargetSelector returns null in the case that most needs the narrow write: the element+doc exist, selection.selector is a non-unique bare class, and structuralSelector returns null (parts empty, or the built structural path doesn't matchesExactlyOne). Every one of these callers then falls back to selector — which is selectorFromSelection(selection), i.e. the exact bare class the function exists to replace. The mutation writes tl.set(".group", ...) and the timeline collapses again. Same defect the PR fixes, guarded by a low-probability edge race (detached element between select and commit, shadow-root boundary, structural chain that no longer re-resolves).
writeTargetSelector's own docstring names this trade — "opt back in with ?? selectorFromSelection where the trade is visible" — so the design is intentional: for drag/resize/rotate/gesture we prefer to widen than to drop the edit. Fair. But two things aren't in the current shape:
-
No test locks the fallback in.
newTweenTarget.test.tsexercises only happy paths (id-less siblings, live DOM, structural walk succeeds). Add one test that forceswriteTargetSelectorto returnnull(detach the element from its parent before commit) and asserts the currently-intended behavior — either "widens and toasts" or "widens silently". Whichever is chosen, the test locks it and any future narrowing sees the failure surface. -
useEnableKeyframes.ts:539already uses the safer shape (if (!selector) { handleGsapAddAnimation("to"); return; }). Consider whether the 5 fallback sites can share that shape — a toast + drop is often better UX than silently animating four siblings the user didn't touch. If not, at least an observability counter on the fallback hit rate would tell you how often the trade is being made.
Same P2 I raised on #2847 for the same fallback in gsapShared.writeTargetSelector, now replicated at 5 more sites.
P2 — tweenTargetsElement widening the read-half means new individual writes update the wrong tween when a group tween exists
packages/studio/src/hooks/gsapShared.ts:304-317.
The read-half falls back to element.matches(targetSelector). Docstring names this as intentional so a deliberate .group tween still gets found. But the composition is: user has a group tween gsap.to(".group", { x: 100 }) set at 100 (all five siblings). User drags #group-2 individually.
tryGsapDragInterceptcallsfindExistingPositionWrite(resolvedAnimations, "#group-2", group-2).- String equality against
.groupfails;element.matches(".group")returnstruebecause#group-2has the.groupclass. - Returns the group
.set. Passed asexistingSettocommitStaticGsapPosition. - Since
existingSet && isInstantHold(existingSet), the commit doesupdate-propertieson the group tween — rewriting the shared.groupset's x/y. All five siblings jump.
This is documented ("still matches a deliberate group tween so merges into it keep working") but it means the invariant the PR is enforcing is only guaranteed for a NEW tween on an element with NO pre-existing group tween covering it. For elements that inherit a group tween, an individual nudge silently retargets the group. Two coverage items would tighten this:
- A test scenario where the group tween pre-exists and the user drags one sibling — assert the intended outcome (currently: group tween updates, all five siblings move) so a future "break out on individual nudge" change trips a test rather than an alarm.
- If the intent is to preserve group semantics, a follow-up note (or
writeTargetSelectordocstring extension) calling out that "author every new tween against one element" does NOT imply "every user edit lands on one element" — the two invariants come apart precisely at the group-inherited case.
Rest of the read-half looks right: element.matches with a try/catch is the correct shape for a GSAP selector string that may not be valid CSS syntax.
Nit — matchesExactlyOne now exported without a test
packages/studio/src/hooks/gsapShared.ts:276. The visibility change is what unlocks the fix in ensureElementAddressable. It's a small, well-scoped predicate but no test exercises it directly and the ensureElementAddressable test at newTweenTarget.test.ts:36-64 only asserts one axis (returns unique selector, mints id, no-op on id, no-op on unique class). A one-line test asserting matchesExactlyOne returns false for a non-existent selector and false when the query throws (":not(:foo:bar" or similar unparseable input) would lock the try/catch in.
Nit — posAnim.targetSelector === selector fast-path at gsapRuntimeBridge.ts:214
The pre-check before findExistingPositionWrite still uses raw string equality. Not a defect (falls through to the fuzzy match on miss), but the inconsistency with the surrounding code stands out; if the fast-path fails to catch the case where posAnim IS the just-authored write but selectorFromSelection and writeTargetSelector disagree, you take a slightly longer path. Consider tweenTargetsElement(posAnim.targetSelector, selector, selection.element) for consistency, or a comment noting the equality is intentional.
PR-body honesty check
- "Nine call sites reproduced" — the diff writes to 10 (
useAnimatedPropertyCommithas two, both listed under one bullet). Consistent. - "Route every path that authors a NEW tween through the existing
writeTargetSelectorladder" — verified against the enumeration above. - "Retargets of an EXISTING tween are deliberately untouched" — verified:
gsapDragCommit.ts:511,useEnableKeyframes.ts:154/263/365/428/446,useAnimatedPropertyCommit.ts:348,gsapDragPositionCommit.ts:76/329,useGestureCommit.ts:207/253,commitWholePathOffsetall keepanim.targetSelector. - "Not covered" list — verified:
commitWholePathOffset,gsapDragPositionCommit.tsfour writes,useEnableKeyframes.tsreplace-with-keyframes— all preserve existing targets. - "Tests reproduce each site through a real writer, re-parse with the real parser" — verified at
newTweenTarget.test.ts:596-614and:669-677.
Verdict
COMMENTED. Two P2 items — the fallback-widening under edge conditions on 5 new sites, and the read-half's group-tween composition — are worth addressing before merge, but neither blocks. Both are already flagged by design in the code's own comments; the ask is a locked test and a small doc/observability strengthening.
Grade: A-. Thoroughly reasoned, exhaustively tested, PR body maps to the diff, "Not covered" list is honest. The subtraction from A is the ponytail-fallback replication across 5 sites without a test that pins the chosen trade-off.
— Review by Via
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Follow-up — cross-review inventory with @james-russo-rames-d-jusso
Rames posted a review 11 seconds before mine (didn't see it before I submitted). Cross-checking; a P0 he flagged is a genuine miss on my side.
Confirmed at head 643e56b4: commitCreatePath is an uncovered new-tween writer — P0
packages/studio/src/components/editor/motionPathCommit.ts:80 — commitCreatePath(targetSelector, position, x, y, commit) emits { type: "add-motion-path", targetSelector, position, duration, x, y }. The caller at MotionPathOverlay.tsx:190 passes createSelector = selectorFor(selection), which in motionPathSelection.ts:11 is:
if (sel.id) return `#${CSS.escape(sel.id)}`;
return sel.selector ?? null;
That last rung is the exact bare-class shape the PR exists to close for every other new-tween path — an id-less .group sibling makes a motion path, add-motion-path gets authored with targetSelector: ".group", and the runtime writes the path over every sibling sharing the class. Same collapse-to-one-row shape as the sites this PR narrows for drag / resize / rotate / gesture / property panel.
Why my enumeration missed it: I searched packages/studio/src/hooks/ for type: "add" and type: "add-with-keyframes". add-motion-path is a distinct mutation kind, and the writer lives under components/editor/, outside the tree I walked. A rg 'type: "add-' over the whole package would have caught it. Adding this to my new-tween enumeration checklist.
Fix shape (same as Rames): writeTargetSelector(selection) ?? createSelector at the call site in MotionPathOverlay.tsx:190, or route through ensureElementAddressable if a motion-path anchor deserves a stable id in the source (Rames's suggestion; I agree — an author who set a motion path will likely edit it again).
Overlap inventory
| Finding | My R1 | Rames R1 | Alignment |
|---|---|---|---|
commitCreatePath motion-path writer uncovered |
Missed | P0 blocker | Rames caught, I confirm |
?? selector fallback re-authors bare class on structural-walk fail |
P2 (5 sites) | Concern (4 sites + docstring drift) | Same defect, same 5 sites (I named gsapKeyframeCommit.ts from base; he named the four in this PR) |
tweenTargetsElement group-tween composition |
P2 discussion | Not raised | My addition — flagging that the "one element" invariant unravels on inherited group tweens |
posAnim.targetSelector === selector fast-path (gsapRuntimeBridge.ts:214) |
Nit — inconsistent shape | Concern — reads as opt but unnamed | Same site, same read |
matchesExactlyOne export untested |
Nit | Not raised | My addition |
first-parsed wins in findExistingPositionWrite |
Not raised | Concern — mid-migration state | Rames's addition, valid follow-up seam |
useGsapAnimationOps.addWithKeyframes bypass surface |
Not raised | Nit | Rames's addition |
Updated verdict
REQUEST-CHANGES equivalent (this is a --comment review by convention here, but the substantive verdict is blocker-present): the motion-path writer needs closing before merge to hit the "every new-tween writer" bar the PR body sets. The two P2s from my original comment stand as follow-ups.
Grade downgrade to B — thorough on nine of ten writers; the tenth is the same class of bug and would ship the "collapse-to-one-row" defect from the motion-path entry point.
— Review by Via
|
All three fixed here rather than deferred, at da155bc. P0, motion path. Confirmed, and it was worse than the write alone. P2, the P2, Studio suite: 3081 passing, 0 failures. Typecheck, lint, format clean. PR body updated with all five bugs. |
Two latent defects in the announcement path this branch adds. - lanesId was keyed by render row alone, so a second TimelineLanes on the page (a mini-timeline beside the main one) would mint the same timeline-lanes-track-0 and every caret's aria-controls would resolve to whichever instance mounted first. The prefix now comes from useId, with the colons stripped so the id stays a legal CSS selector. - trackDisplayNumber returned trackOrder.length + 1 for a key it could not find, which is indistinguishable from a real row: the label announced a row the user can see is wrong and nothing upstream could tell it had guessed. It returns null now, and trackDisplaySuffix drops the number from the label rather than inventing one.
U3 fixed "add keyframe at playhead" widening a write to every sibling sharing a class, but wired writeTargetSelector into only two paths. The same bug was still reachable from the add-animation button, drag, resize, rotate, gesture recording, and the property panel: each derived its target from selectorFromSelection, which hands back a bare class for an id-less element, so one edit authored a tween over all five siblings and the timeline collapsed their rows into one. Route every path that authors a NEW tween through the existing ladder: - ensureElementAddressable now accepts selection.selector only when it addresses exactly one element, so the id-minting fallback right below it (previously unreachable whenever any selector was present) does the work. - gsapDragCommit's five new-tween branches go through one newTweenTarget helper; instant patches reuse the written target so the runtime moves the element the source write names. - useGestureCommit and useAnimatedPropertyCommit keep the existing selector for matching/retargeting and author new tweens with a separate write selector. Retargets of an EXISTING tween are deliberately untouched: they keep anim.targetSelector, so a tween aimed at a whole group stays aimed at it. Narrowing the write alone regressed idempotency, verified by test: the "is there already a write for this element" lookups matched targetSelector by string, so the next nudge missed the write it had just made and appended a second, conflicting one. The read half now falls back to the live DOM (tweenTargetsElement, same contract as getAnimationsForElement), which also still matches a deliberate group tween. Tests reproduce each site through a real writer, re-parse with the real parser, and resolve through resolveSelectorElementIds (what feeds the keyframe cache and the lanes), plus pins for the new-tween vs retarget-existing distinction so a future change cannot collapse the two.
The narrowing this branch adds missed the motion-path overlay, and every caller that could not narrow fell back to the exact bare class the narrowing exists to replace. - motionPathSelection.selectorFor now goes through writeTargetSelector. It feeds both the geometry read and the "set destination" write, so a class sibling measured its home off the FIRST sibling and then authored add-motion-path onto all of them. The toolbar toggle hides when no one-element form exists rather than arming a press that is dropped. - The five new-tween writers that fell back to the selection's own selector now drop the commit instead. A gesture that does not persist reverts on the next reload; a tween silently aimed at five elements does not. - tweenTargetsElement only follows the DOM to a target that matches exactly one element. A target the element merely shares with its siblings is a group tween, and these callers mutate what they find, so an individual nudge was rewriting the group's own tween and moving all five.
da155bc to
1f3fd28
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 reviewed at 1f3fd28 (delta from 643e56b).
Clean R2. Every open R1 finding closed, and Miguel's fix went further than what I'd asked for at every one.
Blocker CLOSED — motionPathSelection.selectorFor now routes through writeTargetSelector. Bonus over the R1 fix I'd suggested: MotionPathOverlay.tsx:135 guards canCreate on the narrowed selector existing, so the toolbar "set destination" toggle stays hidden rather than arming a press that would silently drop. And the docblock catches a consequence I'd missed — the OLD bare .group was ALSO the string the geometry read used, so a class-sibling read its home position off the FIRST sibling and then wrote the path onto all five. Two bugs closed by one narrowing. motionPathSelection.test.ts pins both the write-side and the null-return semantics.
Concern CLOSED (fallback semantics) — Instead of promoting newTweenTarget to a shared helper with a rationale docblock (my nit), the R2 changes the CONTRACT: newTweenTarget returns string | null, and every caller now drops the commit on null rather than falling back to selector. The rewritten writeTargetSelector docblock spells out the trade — "a gesture that does not persist reverts visibly on the next reload, where a tween silently aimed at five elements does not." useGestureCommit even surfaces a user-visible toast ("Cannot save: element has no unique selector") instead of silently dropping. Stronger fix than what R1 asked for, correctly.
Bug I missed in R1 — Worth calling out so future-me remembers: R1 read tweenTargetsElement's element.matches(targetSelector) fallback as "still matches a deliberate group tween so merges into it keep working." That's wrong: because these callers MUTATE what they find, a per-element nudge that resolves to the group tween (.group matches all five siblings via element.matches) rewrites the group's own tween and moves all five — the same class-collapse bug from a different direction. R2's fix uses matchesExactlyOne in the DOM fallback so only a write narrowed to THIS element hits, and the string-equality path keeps deliberate group edits working. The new "does not hand an individual element the group tween it merely inherits" test pins exactly this. Nice catch by Miguel.
Remaining, small:
- The
tryGsapDragIntercepttwo-tier match atgsapRuntimeBridge.ts:214still reads as a hot-path exact-match with the general fallback below. Behaviour is fine; a one-liner naming the split saves a reader the trip. Nit. - With the new
matchesExactlyOne-basedtweenTargetsElement, the "first-parsed wins between overlapping tweens" seam narrows to the legacy.groupwrite coexisting with a narrowed#group-2write for the same element (which requires bothtargetSelector === selectorin the legacy row ANDmatchesExactlyOnein the narrowed row). Only a mixed-legacy migration window; still a follow-up seam, not this PR's scope. useGsapAnimationOps.addWithKeyframesis dead inpackages/studiotoday but exposes a raw-selector API that would bypass the guard if a future caller adopts it. Delete or wire throughwriteTargetSelector. Housekeeping.
Overall: the write/read pair (writeTargetSelector + matchesExactlyOne-based tweenTargetsElement) is a load-bearing invariant now, and R2 pinned it with tests at both halves. Solid execution on a bug class that already burned QA once.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 — verifying R1 findings at the new head
Rebuilt at 1f3fd2800cd2e4df9e8a2c2b81ea11ed5b799630, three commits above R1 (643e56b4):
ba0d640fix(studio): scope lane ids per timeline and stop inventing a track row3d92436fix(studio): author every new tween against one element1f3fd28fix(studio): give the motion path and the fallbacks a one-element target
The last commit's message is the fix for the R1 blocker.
Per-finding verdict
P0 — motion-path overlay authored the bare class (commitCreatePath / MotionPathOverlay.tsx:190). RESOLVED.
motionPathSelection.ts:21-23 now routes selectorFor through the same writeTargetSelector ladder every other new-tween writer uses. Both halves of the overlay read it — the geometry measure at MotionPathOverlay.tsx:93 and the create write at :133 — so the home-position sample and add-motion-path write agree on one element. When no one-element form exists, canCreate at :138 is false and the toolbar hides "set destination" rather than arming a press that gets dropped. Test coverage is direct: motionPathSelection.test.ts mounts five class siblings, drives selectorFor on the third, asserts the result is not .group and addresses exactly one element, plus null-safety cases.
P2 #1 — five ?? selector fallbacks in the drag/property/gesture writers. RESOLVED.
All five new-tween sites in gsapDragCommit.ts (replaceKeyframedPositionHold, commitStaticGsapPosition, commitStaticGsapRotation, commitStaticGsapSize, commitKeyframedSizeFromResize) now go through one newTweenTarget helper (line 60-62) that returns writeTargetSelector(selection) and every caller gates on if (!target) return; — no ?? selector fallback. Same shape in useAnimatedPropertyCommit.ts:256 and :500, and in useGestureCommit.ts:184-188 (with an explicit toast when the recording cannot persist). Locked at newTweenTarget.test.ts and newTweenTargetHooks.test.tsx — 619 test lines that author through the real writer, re-parse with the real parser, and read attribution back through resolveSelectorElementIds. commitKeyframeAtTimeImpl also gets a "drops rather than widens" pin (gsapShared.writeTarget.test.ts:278-289).
P2 #2 — tweenTargetsElement element.matches fallback silently retargeted group tweens. RESOLVED.
Not deferred, actually fixed. The DOM fallback at gsapShared.ts:311-319 is now matchesExactlyOne, not element.matches. String equality on the target still lets the group edit its own tween ("does the selection IS the group" case), but an individual nudge on one of five .group siblings no longer inherits the group tween — it authors its own write instead. Four pinning tests at gsapShared.writeTarget.test.ts:248-275 — including the direct "does not hand an individual element the group tween it merely inherits" case — lock the outcome one way.
Fresh adversarial pass at new head
- Bonus fix I missed at R1.
ensureElementAddressableingsapScriptCommitHelpers.ts:19-40was a tenth writer site (reached from the add-animation button): pre-fix it returnedselection.selectorunconditionally, aiming the write at every sibling sharing the class. Now it accepts the selector only whenmatchesExactlyOneand falls through to id-mint otherwise. The id it writes back to the source is a stronger fix than a structural path here because every later lookup on this element becomes exact. Locked atnewTweenTarget.test.ts"ensureElementAddressable — add-animation button". - Retargets deliberately excluded and verified.
gsapDragPositionCommit.ts(three sites),commitWholePathOffset, anduseEnableKeyframes.promoteSetToKeyframeswritetargetSelector: anim.targetSelector, so a tween the author aimed at a group stays aimed at it. Consistent with the PR body's exclusion list. - One residual widening path I want on your radar (not a blocker). In
useGestureCommit.ts:216and:262thereplace-with-keyframesmutations passtargetSelector: selector(the read-side selection selector), notexistingPositionTween.targetSelector.existingPositionTweenis found viatweenTargetsElement, which the P2 #2 fix widened to include narrowed tweens — so a class sibling whose narrowed instant-hold was minted by a prior drag can now be picked up here, and the merge rewrites the tween's target from the narrowed form back to.group. Reachability: (a) prior static drag on.groupsibling → narrowed instant hold, (b) gesture recording on the same element, (c)isInstantHoldor overlapping-time branch. Existing test"leaves a merged existing group tween aimed at its whole group"pins the intended behaviour when the existing tween is genuinely the group tween (targetSelector =.group), which is consistent with the PR body's "keep existing selector for retargeting" — but the narrowed-existing case isn't covered. Two options: useexistingTweenTargetSelectorhere (the helper's own docstring warns against exactly this widening) or add a test that pins the narrowed-instant-hold case one way or the other. Non-blocking because the primary ten sites are all fixed and this requires a specific setup, but noting because it's the same class of defect the PR is closing.
Envelope
Three commits, all authored by Miguel, no Co-Authored-By in messages. CI: Preflight, Preview parity, player-perf, regression, preview-regression all pass. Perf and regression-shards matrix jobs correctly skipping. Graphite mergeability_check pending as expected for a stack top.
Verdict
Approving. The R1 blocker is closed with test coverage locking the exact bare-class shape at the overlay's selector helper. Both R1 P2s are fully resolved — the ?? selector fallbacks are replaced with a null-return + upstream gate at every site, and the element.matches fallback is narrowed to matchesExactlyOne with four tests pinning the group-exclusivity distinction. The fresh pass turned up one narrow residual worth a follow-up but no new blocker.
Grade: A. Comprehensive site coverage (10 primary writers + the bonus ensureElementAddressable catch), tests that round-trip through the real parser, and prose that names the exact failure at each site. The one point off is the useGestureCommit replace-with-keyframes residual, which is reachable through the PR's own machinery and deserves either a wire-through or a pinning test.
— Review by Via
The base branch was changed.
…en-target fix(studio): author every new tween against one element
3 participants
What
The bare-class write fixed lower in this stack for "add keyframe at playhead" was still reachable from every other writer. This closes each one, after reproducing it individually.
Bugs fixed
1. One edit authors a tween over every sibling sharing a class, reachable from the add-animation button, drag, resize, rotate, gesture recording, and the property panel. Each derived its target from
selectorFromSelection, which hands back a bare class for an id-less element, so a single edit wrote a tween covering all five siblings and the timeline collapsed their rows into one. Nine call sites reproduced:ensureElementAddressable(reached from the add-animation button)commitStaticGsapPosition,commitStaticGsapRotation,commitStaticGsapSizecommitKeyframedSizeFromResize,replaceKeyframedPositionHolduseAnimatedPropertyCommit(both the global static set and the first same-group keyframe tween)useGestureCommit(add-with-keyframes)2. The same bare-class write from the motion-path overlay, the tenth site, reached from the preview toolbar's "set destination" toggle.
selectorForreturned the selection's own selector, which the overlay uses twice:useMotionPathDatameasured the element's home position by it, andcommitCreatePathauthoredadd-motion-pathwith it. On a class sibling the destination was computed from the FIRST sibling's home position and then written onto all of them.3. A second, conflicting write is appended on the next nudge. Found by test while fixing bug 1: narrowing the write alone regressed idempotency. The "is there already a write for this element" lookups (
findExistingPositionWrite,findRotationSetAnimation,findSizeSetAnimation, the gesture position lookup, and the property panelstaticWritesfilter) all matchedtargetSelectorby string equality, so the next nudge missed the write it had just made and appended a second one that conflicted with it.4. An individual nudge silently retargets a group tween. The fix for bug 3 first read the DOM with a bare
element.matches, which is true for a target the element merely shares with its siblings. Those lookups feed mutations, so nudging one of five.groupsiblings rewrote the group's own tween and moved all five.5. A caller that could not narrow re-authored the bare class anyway. Five new-tween writers fell back to the selection's own selector when no one-element form existed, which is the exact string the narrowing exists to replace.
Why
Same class of defect as the playhead fix below: silent corruption of the user's source file from an ordinary edit gesture. Fixing only the one path the QA report named would have left nine siblings broken.
How
Route every path that authors a NEW tween through the existing
writeTargetSelectorladder:ensureElementAddressablenow acceptsselection.selectoronly when it addresses exactly one element, so the id-minting fallback directly below it does the work. That fallback already existed and was simply unreachable whenever any selector was present.gsapDragCommit's five new-tween branches go through onenewTweenTargethelper. Instant patches reuse the written target so the runtime moves the element the source write names.useGestureCommitanduseAnimatedPropertyCommitkeep the existing selector for matching and retargeting, and author new tweens with a separate write selector.motionPathSelection.selectorForgoes through the same ladder, so the overlay measures and writes against the same one element. When no one-element form exists the toolbar hides "set destination" rather than arming a press that gets dropped.The read half falls back to the live DOM (
tweenTargetsElement), which fixes idempotency. That fallback requires the target to match exactly one element: a target shared with siblings is a group tween, and only the selection that is the group itself (string equality) may edit it.Where no one-element form exists at all, every writer now drops the commit instead of falling back to the selection's selector. A gesture that does not persist reverts visibly on the next reload; a tween silently aimed at five elements does not.
useGestureCommitsurfaces a toast because a recording is expensive to redo.Retargets of an EXISTING tween are deliberately untouched. They keep
anim.targetSelector, so a tween aimed at a whole group stays aimed at it.Test plan
Tests reproduce each site through a real writer, re-parse with the real parser, and resolve through
resolveSelectorElementIds(what actually feeds the keyframe cache and the lanes), plus pins for the new-tween versus retarget-existing distinction so a future change cannot collapse the two. Bugs 2, 4 and 5 each get their own lock:motionPathSelection.test.tsfor the overlay selector, fourtweenTargetsElementcases for group exclusivity, and acommitKeyframeAtTimeImplcase asserting the commit is dropped rather than widened.Studio suite green on this branch: 3081 passing, 0 failures. Typecheck, lint and format clean.
Not covered
commitWholePathOffset, all fourgsapDragPositionCommit.tswrites, andreplace-with-keyframesinuseEnableKeyframes. Narrowing those would silently retarget a tween the author aimed at a whole group.