feat(studio): add keyframe timeline state - #2781
Conversation
5dd41e9 to
e4d7bde
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial review — head e4d7bde
Verdict: COMMENT. State-shape primitive is sound: pure setters, no Date.now/Math.random/DOM in reducers, immutable Set/Map copies on write, and the ambiguity-flag semantics are captured in two tests. Author has parked six known low/nit findings verbatim in the scratch doc, so I'm surfacing only the ones that are stack-forward defects or drift risks the deferred list didn't already own. Head SHA verified e4d7bde64f3b3e500834231134bc9fdb3858ad41; pre-existing state on main confirms selectedKeyframes:Set and keyframeCache:Map shapes are inherited, so I only fault the new additions where scope worsens the invariant.
P2
1. Selection primitives are single-key only — later stack PRs will patch this slice. packages/studio/src/player/store/keyframeSlice.ts:31-35. toggleSelectedKeyframe(key) + clearSelectedKeyframes() covers add/remove/clear. Any downstream marquee/range/select-all lands with either (a) a loop of toggleSelectedKeyframe (N re-renders, each rebuilding the Set), or (b) a slice patch that adds replaceSelectedKeyframes(keys). Since this PR is explicitly the shared primitive, add replaceSelectedKeyframes(keys: readonly string[]) and selectRangeKeyframes(keys: readonly string[]) now — Lens B / Lens E.
2. easeAmbiguous guard is duplicated verbatim across the two writers — the exact drift risk the deferred 01-pr-2683 doc calls out. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:60-72 and packages/studio/src/hooks/gsapTweenSynth.ts:15-30. Both blocks encode the same three-clause invariant on the merged keyframe. The invariant is the state contract downstream lanes read from (easeAmbiguous ⇒ suppress inline button), so a divergence between the two writers is a silent data-shape defect, not just a style nit. Extract flagAmbiguousEase(existing, incoming) and call it from both sites. Deferring this to a future PR is fine, but this is the correct PR to fix it — the slice is where the invariant becomes public API. Author flagged 🟡.
3. gsapAnimations silently diverges from keyframeCache on any anim missing propertyGroup. packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:22-25 gates the sourceAnimations write on if (anim.propertyGroup); keyframeCache writes regardless. useGsapTweenCache.ts:333-337 applies the same filter. Net effect: for an element whose only tween has no propertyGroup, keyframeCache has an entry, gsapAnimations is empty, and writeGsapAnimationsForElement(..., undefined) deletes whatever was there. The slice comment (keyframeSlice.ts:38) says "expanded property lanes read this, never keyframeCache" — so an expanded lane on that element renders nothing while the collapsed row still shows diamonds. Either (a) tighten the invariant ("no propertyGroup ⇒ don't cache") uniformly across both writers, or (b) drop the propertyGroup gate on gsapAnimations and let downstream lane renderers filter. Pick one; the current split is undefined behavior. Lens A / Lens E.
4. selectedKeyframes holds two disjoint key shapes with no discriminator — author flagged 🔵, upgrading to P2 because it's the slice's public API. packages/studio/src/player/store/keyframeSlice.ts:15. Collapsed diamonds key as element:pct, expanded as element:group:animation:clipPct. Callers passing a collapsed key against an expanded set won't error — they'll silently miss. This is a state-shape defect the stack will inherit N times. Fix now with a discriminator prefix (c: / e:) or split into two Sets before consumers land, so the top of the stack doesn't cement it.
P3
5. expandClips has no collapseClips counterpart. packages/studio/src/player/store/keyframeSlice.ts:36. Author flagged 🟢. Symmetric pair belongs in the primitive — Lens E.
6. window.__studioTest is reassigned every effect re-run. packages/studio/src/hooks/useStudioTestHooks.ts:25-46. Effect deps [applyDomSelection, buildDomSelectionFromTarget, previewIframeRef] will churn on any parent re-render that doesn't memoize the two callbacks; each churn stomps any handle a dev driver just captured. Guard with if (!window.__studioTest) or install once at module scope. Author flagged 🟢.
7. Set/Map shape is persistence-hostile; document that constraint on the slice. packages/studio/src/player/store/keyframeSlice.ts:31, 47, 49. Neither zustand/persist nor devtools middleware is currently wired (grep-verified), so this is deferred, not blocking. But two new Map/Set fields land in this PR, and any downstream PR that adds persist/devtools/SSR replay will round-trip them to {} with no warning. Add a top-of-file comment: "Set/Map by design — do not add persist middleware without a serializer." Lens D.
Adversarial lenses
- A (state-shape): #3, #4 fired — divergent cache invariants and mixed key shapes are the P2s.
- B (selection): #1 fired — no bulk/range primitive.
- C (reducer purity): clean; only #6 (window write in effect) is a side-effect boundary nit.
- D (serialization): #7 fired — deferred but worth pinning.
- E (stack-forward compat): #1, #3, #5 fired — all shape gaps the primitive should close before consumers land.
Approve after #2 (ambiguity-helper extraction) and #3 (propertyGroup gate convergence) land in-stack; #1 and #4 are worth attempting now while the primitive has zero consumers.
— Via
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 e4d7bde64f3b.
R2 adversarial pass on the foundation slice for Family B. Publishes 5 contracts (keyframeCache, gsapAnimations, focusedEaseSegment, selectedKeyframes, expandedClipIds) that all 7 downstream PRs build on. The concerns cluster around primitive-layer invariants the type system doesn't enforce — they'll propagate through B2-B8 consumers, so worth closing here. Convergent with Via's rollup on gsapAnimations/keyframeCache divergence.
🟢 Verified clean
- idSelector() correctly emits [id="..."] fallback for digit-leading/dotted/space-containing ids and escapes quotes/backslashes; SAFE_HASH_ID regex ^-?[A-Za-z_][\w-]*$ cannot match anything that would break querySelector
- deduplicateKeyframes' generic signature threads animationId/easeAmbiguous through without narrowing to GsapPercentageKeyframe
- trackStudioSegmentEaseEdit's discriminant action: 'open' | 'commit' threaded through AnimationCard + events test
- createKeyframeSlice's initial Set/Map values are fresh instances; toggle/set/expandClips setters return early with state unchanged on no-op transitions (proper Zustand shallow-eq)
Complements Via's parallel adversarial pass (Family B rollup). Where Via found 0 P1 / 17 P2 / 18 P3, the adversarial subagent pass here surfaced additional depth on the mutation-authority thread and the Promise chain.
An ungrouped tween (mixed property groups classify to propertyGroup undefined) fed keyframeCache but was skipped by every gsapAnimations writer, so the collapsed row drew diamonds the expanded lanes had no source animation to render. Drop the property-group gate at all three writers; lane consumers already filter by group. Also route the same-percentage merge in updateKeyframeCacheFromParsed through deduplicateKeyframes so the easeAmbiguous rule has one owner.
Each keyframe-cache writer re-derived a clip-relative percentage inline, and the post-commit writer rounded to 0.1% while the others used 0.001%. Selection keys embed that number, so a commit-time rewrite could orphan a live key. toClipPercentage owns the rounding, toClipKeyframes owns the whole row (percentage plus the tween percentage and animation identity the lanes read), and the parsed write reuses elementCacheKeys instead of open-coding the three key variants.
R3 review follow-ups on the keyframe cache: - clearKeyframeCacheForFile collected ids from the index.html alias prefix too, so a re-scan of one composition file wiped rows a sibling file had just written (several files re-scan concurrently). Only the file's own prefixed keys name the ids now; clearKeyframeCacheForElement still takes the alias and bare key with them. - toClipKeyframes fell back to a fixed 1s tween duration, which put a duration-less tween's keyframes at a percentage no edit path agreed with. It now spans the clip, matching resolveEditableTweenDuration. - collectAnimatableKeyframeProperties takes `object` so call sites drop their `as Record<string, unknown>` casts. Regression tests cover both fixes.
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta review from R1 head e4d7bde64 to current head d05ecb109 — three new commits (e34e529 R1 fix, acf6766 R2 refactor, d05ecb1 R3 fix). Also verified the 6 R1 findings you deferred against stack tip 6ee750fee.
Deferral to stack tip — verified
Reading the stack tip head at 6ee750fee (PR #2791), each of the 6 R1 findings you deferred does land there. Per the Graphite-stack fix-hoist model this is fine as long as #2781 doesn't merge to main ahead of the sibling PRs — the fixes are in the stack, not on this SHA. Verdict per finding:
| Finding | This PR (d05ecb109) |
Stack tip (6ee750fee) |
|---|---|---|
🟠 F#1 playerStore.reset() omits focusedEaseSegment |
still stranded (line 517-546) | fixed — focusedEaseSegment: null, added to reset |
🟠 F#2 updateKeyframeCacheFromParsed strips [id="…"] selectors |
still uses anim.targetSelector.match(/^#([\w-]+)/)?.[1] (line 21 + line 64) |
fixed — both sites now call idFromSelector(anim.targetSelector) from gsapShared |
🟡 F#3 useGsapAnimationsForElement doesn't clear stale gsapAnimations when animations become empty |
mostly OK via the allKeyframes.length === 0 clear path — but the hold-skip criteria in sourceAnimations filter and the inline for-loop differ subtly (see note below) |
fixed — both paths gated on the same isStaticPositionHold(anim) helper |
🟡 F#4 updateKeyframeCacheFromParsed wipes valid gsapAnimations for propertyGroup-less animations |
fixed here in e34e529 — property-group gate removed at all three writers, sourceAnimations.get(id) is populated for every anim that fed the cache |
(same fix carried at tip) |
🟡 F#5 deduplicateKeyframes leaves ease iteration-order-dependent after flagging easeAmbiguous |
still if (kf.ease) existing.ease = kf.ease; unconditionally |
fixed — if (existing.easeAmbiguous) delete existing.ease; else if (kf.ease) existing.ease = kf.ease; |
🟡 F#6 useStudioTestHooks cleanup = undefined leaves the key on window |
still .__studioTest = undefined; |
fixed — delete window.__studioTest; (with rationale comment) |
🟢 F#7 KeyframeSlice setters allocate a new Map on every no-op |
still unconditional new Map(...) |
fixed — early-return if (data ? state.keyframeCache.get(elementId) === data : !state.keyframeCache.has(elementId)) return state; on both setKeyframeCache and setGsapAnimations |
On F#3 specifically — the R1 concern about stale gsapAnimations survival is largely resolved at this head because the allKeyframes.length === 0 branch calls clearKeyframeCacheForElement, and that clears both keyframeCache and gsapAnimations for the element. What remains is a small divergence between the sourceAnimations filter ((animation.keyframes || synthesizeFlatTweenKeyframes(animation))) and the inline hold-skip inside the for-loop (method === "set" || (duration ?? 0) === 0 with x/y-only prop guard). A to({ duration: 0 }) with x/y props but no immediateRender: true clears the inline skip but not the synth's null-return path, so it lands in sourceAnimations but not allKeyframes. That divergence goes away at stack tip where both paths call isStaticPositionHold(anim). Corner case, not a blocker.
Merge-order coupling to flag: if #2781 merges to main ahead of the stack (e.g. bypassing Graphite's stack semantics), findings F#1, F#2, F#5, F#6, F#7 all ship live. F#2 in particular has real teeth here — idSelector() is already emitted by gsapDragCommit, useGestureCommit, and ensureElementAddressable at this SHA (see gsapDragCommit.ts:120, useGestureCommit.ts:171, gsapScriptCommitHelpers.ts:11), so a drag on a digit-leading id would leave post-commit cache stale until a full file re-scan. Trusting the stack merge sequence you've set up.
The three new commits — LGTM
-
e34e529—fix(studio): keep gsapAnimations in sync with the keyframe cache. Drops theif (anim.propertyGroup)gate at all three writers; every tween that feedskeyframeCachenow also feedsgsapAnimations. Also routes the pre-existing duplicated same-percentage-merge inupdateKeyframeCacheFromParsedthroughdeduplicateKeyframesinstead of open-coding it. Clean fix; kills my F#4 cleanly and preemptively removes an easeAmbiguous drift risk between the two writers. -
acf6766—refactor(studio): one owner for clip-relative keyframe rows. ExtractstoClipPercentage(0.001% precision,absoluteTime - clipStartnormalized againstclipDurationwith fallback for degenerate 0-duration clips) andtoClipKeyframes(percentage +tweenPercentage+propertyGroup+animationIdshape) intogsapShared.ts, then rewiresupdateKeyframeCacheFromParsed,useGsapAnimationsForElement, andusePopulateKeyframeCacheForFileto use them. Kills the 0.1% vs 0.001% precision drift between the post-commit writer and the pre-commit writers — real fix, since selection keys embed that number and a commit-time rewrite at coarser precision would orphan a live selection. Also unifies the three cache-key variants throughelementCacheKeys(targetPath, id). Tests cover both. -
d05ecb1—fix(studio): scope the per-file keyframe-cache clear to its own keys. Two real fixes:clearKeyframeCacheForFileused to walk theindex.html#alias prefix too, so a re-scan ofcomp.htmlcould delete a keyframe row that a concurrent re-scan of another composition file had just written intoindex.html#id. Now only walks the file's own prefix — regression test atgsapKeyframeCacheHelpers.test.ts:87-97. Nice — the concurrent-rescan race would have been very hard to debug from telemetry alone.toClipKeyframesfell back to a fixed1whenanim.durationwas undefined, misplacing a duration-less tween's keyframes at 25% of a 4-second clip. Now falls back toclipDuration, matchingresolveEditableTweenDuration. Regression test atgsapShared.test.ts:143-146.- Type-tightens
collectAnimatableKeyframeProperties(entry: object)and drops theas Record<string, unknown>casts at call sites.
I didn't find anything new to flag in the delta — the three commits are surgical and well-tested.
Foundational-slice lens
Per foundational-contract-deep-scrutiny — this is B1/6 (or whatever the M is) of a Family B stack targeting a Family A baseline. The primitives added here (KeyframeSlice, useStudioTestHooks, toClipPercentage, toClipKeyframes, elementCacheKeys) are the foundation the rest of the stack builds on. A defect that lives past R3 propagates through the entire stack. Looked specifically for:
- Contract seams.
KeyframeSlice.focusedEaseSegmenthas no readers at this SHA — declared here, consumed at the stack tip. Same shape forexpandedClipIds(readers arrive at #2791). Adding state ahead of consumers is fine as long as the reset/lifecycle is fixed in the same slice that adds the consumers — which is what you're doing. elementCacheKeysinvariants. ForsourceFile !== "index.html", writes go to[sourceFile#id, index.html#id, id]. Forindex.html,[index.html#id, id].writeGsapAnimationsForElement,clearKeyframeCacheForElement, and theupdateKeyframeCacheFromParsedwrite loop all go through this helper.clearKeyframeCacheForFileintentionally does NOT walkindex.html#— that's the R3 concurrent-rescan fix. Reads (readKeyframeSnapshotand callers) all usebuildCacheKey(sourceFile, elementId) = sourceFile#elementId— consistent with the writes.- Ambiguous-ease policy.
easeAmbiguousis set on any same-% collision between differentanimationIds. Downstream lanes / diamond consumers must check it before usingease. The stack-tipdeduplicateKeyframesfix (delete existing.easewhen ambiguous) is the correct enforcement point, and it's the SAMEdeduplicateKeyframesthatupdateKeyframeCacheFromParsednow calls (thanks to the R1 fix ine34e529) — so at stack tip, the ambiguous-ease drop applies everywhere it should.
What I didn't verify
- Stack tip's
focusedEaseSegmentconsumers (PropertyPanelFlat, GsapAnimationSection, propertyPanelFlatMotionSection) — trusting that they arrive together with the reset fix at #2791. - The
2,910 Studio tests, 398 Studio Server testsyou called out — trusting your local validation numbers; CI here shows the same jobs passing. - Whether other Family A PRs on
mainsince R1 have changed anything in the seams I looked at — the merge-base bump to755b68fband the small delta stat suggest no.
All 30+ CI checks green at head. LGTM from my side on the delta; the deferral shape works as long as the Graphite stack merges as a unit.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at d05ecb10910e.
Verdict
COMMENTED. R1 and R2 fixes for in-diff concerns landed cleanly; the seven residual R2 findings deferred to the stack tip are verified fixed at 6ee750fee (PR #2791). Two nits from this pass, no blockers.
Scope re-verification
728 additions / 255 deletions across 21 files; no Co-Authored-By lines in the body; head d05ecb10910e. The slice publishes exactly the five contracts R1/R2 named — selectedKeyframes, expandedClipIds, focusedEaseSegment, keyframeCache, gsapAnimations — plus their setters. keyframeSlice.ts:53-108 is the new home; playerStore.ts:96,308 fold it in via interface PlayerState extends KeyframeSlice + ...createKeyframeSlice(set). Every previous inline definition is deleted, not shadowed.
R1 follow-up verification
- #3 propertyGroup gate divergence — RESOLVED IN THIS PR.
gsapKeyframeCacheHelpers.ts:31(sourceAnimations.set(id, ...)unconditional) anduseGsapTweenCache.ts:333-338(animations.filter((a) => a.keyframes || synthesizeFlatTweenKeyframes(a))) both admit ungrouped tweens. Delta commite34e529on 2026-07-25 removes bothif (anim.propertyGroup)gates. - #2 ambiguity-helper extraction — RESOLVED DIFFERENTLY (better). Rather than lifting a
flagAmbiguousEase(existing, incoming)helper, Miguel madededuplicateKeyframes(gsapTweenSynth.ts:8-33) the sole implementer and had the second writer call it (gsapKeyframeCacheHelpers.ts:50). One owner instead of two conforming writers is the stronger consolidation. - #1 bulk selection primitive — DEFERRED WITH RATIONALE. PR body cites the unused-exports gate; verified no consumer in this PR's diff. Reasonable — the primitive can grow when B2-B8 lands the marquee.
- #4 two disjoint selection-key shapes — DEFERRED TO #2787. PR body cites
timelineKeyframeIdentity.tsJSON envelope. Not verified here, on the honor system for the stack. - #5
collapseClips— unchanged (parked 🟢). - #6
useStudioTestre-run guard — unchanged; ALSO Rames R2 #6; fixed at tip. - #7 Set/Map persist-hostile comment — unchanged (nit).
R2 follow-up verification (Rames' inline findings)
Every non-in-diff residual carries Miguel's "Addressed at stack tip in 6ee750fee (PR #2791)" reply. Verified each at that SHA:
- 🟠 R2#1
focusedEaseSegmentstranded across composition switch —playerStore.ts:534at6ee750feeincludesfocusedEaseSegment: nullinreset(). Fixed. - 🟠 R2#2
updateKeyframeCacheFromParsedregex strips[id="…"]selectors —6ee750feeaddsidFromSelectorinverse togsapShared.tsand both regex sites (gsapKeyframeCacheHelpers.ts:21,64) call it. Fixed. - 🟡 R2#3 stale
gsapAnimationsnever cleared when source empty —useGsapTweenCache.ts:308-320at6ee750feereachesclearKeyframeCacheForElement(sourceFile, elementId)on theallKeyframes.length === 0branch, which nulls both stores. Fixed. - 🟡 R2#4 wipe valid
gsapAnimationswhen propertyGroup missing — RESOLVED IN THIS PR by the propertyGroup gate removal (R1#3 fix).sourceAnimations.get(id)now always returns the anim array, sowriteGsapAnimationsForElement(..., animations.get(id))cannot passundefinedand delete a live entry. - 🟡 R2#5 iteration-order ease under
easeAmbiguous—gsapTweenSynth.ts:51-52at6ee750feereplacesif (kf.ease) existing.ease = kf.easewithif (existing.easeAmbiguous) delete existing.ease; else if (kf.ease) existing.ease = kf.ease;. Fixed. - 🟡 R2#6
useStudioTestHookscleanup= undefinedvsdelete—useStudioTestHooks.ts:60at6ee750feeusesdelete window.__studioTestwith a comment explaining why. Fixed. - 🟢 R2#7 setter allocates on no-op writes —
keyframeSlice.ts:99-115at6ee750feeshort-circuits withreturn statewhen incoming payload is identity-equal or an absent delete. Fixed.
Editor-UI lens set
- Silent-catch / error-invariant — none.
useStudioTestHooks:30-34catchesimport.meta.envaccess but that's a bundler-portability guard, not a swallowed mutation error. - Commit semantics — every setter is single-dispatch; no interleavable multi-step state transitions.
- Key stability —
selectedKeyframeskeys are content-derived (element:pct/element:group:animation:clipPct), not array indices;keyframeCachekeyed bysourceFile#elementIdand bareelementId. Stable across re-orders. - ARIA / keyboard — n/a (state slice, no rendered surface). Downstack lanes own it.
- Propagation — n/a (no DOM handlers).
- Semantic-vs-symptom — R1#3 fix (propertyGroup gate) is semantic: writes agree on which tweens the two stores admit, not a per-render patch. R2#5 fix at tip likewise (
delete existing.easeis honest about ambiguity, not a UI band-aid). - Sibling helpers / DRY —
toClipPercentage+toClipKeyframes(gsapShared.ts:225-270) collapse three previously-open-coded call sites (gsapKeyframeCacheHelpers.ts:38,useGsapTweenCache.ts:363,414). Explicit answer to the R2 rounding-drift concern. - Parity audit — collapsed diamonds read
keyframeCache; expanded property lanes readgsapAnimations. R1#3 + R2#4 fixes make the two agree on which tweens they record. Parity re-established. - Cross-mode (edit vs preview) —
useStudioTestHooksgated onimport.meta.env.DEV; never installs in production.keyframeSlice.ts:89initial isnull, no mode-specific branch. - Perf audit — the setter no-op flap (R2#7) is real at this head, addressed at tip. Non-blocker in-PR:
keyframeCache/gsapAnimationssubscribers are the timeline lanes, which already virtualize. - PR-body-vs-diff — every claim in the body maps to a code change:
- "property-group gate is gone from both writers" →
gsapKeyframeCacheHelpers.ts:31,useGsapTweenCache.ts:333-338✓ - "
toClipPercentagenow owns the rounding" →gsapShared.ts:225-233✓ - "
toClipKeyframesowns the whole row" →gsapShared.ts:241-270✓ - "parsed write reuses
elementCacheKeys" →gsapKeyframeCacheHelpers.ts:60✓ - "
window.__studioTestgated onimport.meta.env.DEV, cleared on unmount" →useStudioTestHooks.ts:29-34,47-49✓ - "Every blocker and high finding raised on this PR is fixed in the stack" → both 🟠 fixes verified at
6ee750fee✓
- "property-group gate is gone from both writers" →
- Cross-writer invariant (new axis) — see Non-blocking observations #1 below.
Standards + Spec + Precision + Round-trip
- Standards (CONTRIBUTING.md §Type-safety): avoid
as T, avoid!non-null, prefer narrowing. Diff has one bareas Tin a test fixture (see Standards lens re-run). - Spec bi-directional: every body bullet has a code anchor (above); every non-trivial code change has a body anchor.
trackStudioKeyframeLaneExpand(events.ts:59-61) is added ahead of its caller (the lane-caret handler is downstack B2) — legitimate for a shared-primitive PR. - Sibling-precision divergence:
toClipPercentageowns 0.001% (gsapShared.ts:232,Math.round(... * 100000) / 1000), and all three previously-open-coded call sites are converted to call it — the exact drift R2 named. Clean. - Middle-man wrap-unwrap:
keyframeSlice.tsre-exportsKeyframeCacheEntry;playerStore.ts:14re-re-exports it. Type identity preserved, no serialization boundary in this PR.
Fix-internal remaining-silent-X audit (six categories, adapted for feature)
- State-accumulation discard —
setKeyframeCache/setGsapAnimationscopy-on-write into a freshMap; failure would raise (no swallow), leaving prior state intact. - Return-boundary invariant on the timeline's outer render —
KeyframeCacheEntry.keyframes[]has no sortedness guarantee on the slice itself; sorting happens insidededuplicateKeyframes(gsapTweenSynth.ts:33). Consumers that read the entry directly (useGsapTweenCache.ts:363merged path) rely on the writer having sorted. Contract implicit; documented via thededuplicateKeyframesreturn. - Library defaults on the failure axis — Zustand shallow-eq is Set/Map-identity based; every write creates a new reference, which is what triggers subscribers. Correct for this slice.
- Precedence in overlap rules —
deduplicateKeyframes:29if (kf.ease) existing.ease = kf.easeis iteration-order dependent when both eases exist andeaseAmbiguousis not set (single-animation case). Rames' R2#5 catches the multi-animation case; single-animation is by definition non-conflicting so this is fine. Same logic inupdateKeyframeCacheFromParsedafter the R1#2 consolidation is now single-owner. - Session/resource ownership —
useStudioTestHooks.ts:47-49returns a cleanup that reassignsundefined, keeping the key enumerable (Rames R2#6; fixed at tip viadelete). - Discovery/enumeration completeness —
updateKeyframeCacheFromParsedstill uses^#([\w-]+)/(gsapKeyframeCacheHelpers.ts:21,64) as the reader inverse ofidSelector— attribute-selector shape is not matched; Rames R2#2 catches this (🟠), fixed at tip viaidFromSelector. In this PR alone, digit-leading ids don't get their cache refreshed post-commit.
Standards lens re-run
Mechanical grep on added lines only (from /tmp/pr2781.diff ^+ slice):
| Pattern | Count | Notes |
|---|---|---|
Bare as T (not as const, not as unknown as T) |
1 | gsapShared.test.ts:640 } as GsapAnimation; — test fixture. CONTRIBUTING.md §Type-safety asks for as unknown as GsapAnimation with a one-line comment, or Partial<GsapAnimation> if callers tolerate it. Test-file nit. |
Non-null !. / ![ / !; |
0 | — |
.message without instanceof |
0 | — |
Angle-bracket cast <T> (excluding generics) |
0 | — |
Tests
Coverage matches the added surface. New slice fields get direct-store tests (playerStore.test.ts:29-56 — toggle and idempotent set for expandedClipIds). toClipPercentage and toClipKeyframes both get unit tests (gsapShared.test.ts:82-153), including the zero-duration and duration-less clip edge cases the R2 rounding fix exists to cover. updateKeyframeCacheFromParsed gets a new test that seeds three keys and verifies clearKeyframeCacheForFile("comp.html") doesn't touch index.html#title/title (gsapKeyframeCacheHelpers.test.ts:84-100) — the exact scope-of-clear invariant Miguel introduced in the R2 fix commit. idSelector and deduplicateKeyframes generic (through animationId/easeAmbiguous) both have added tests. Undo/redo integration isn't yet applicable (the slice is a shared primitive; history integration is downstack).
Non-blocking observations
-
sourceAnimationsfilter vsallKeyframesinline filter divergence.useGsapTweenCache.ts:333-338admits an anim ifanimation.keyframes || synthesizeFlatTweenKeyframes(animation)is truthy; theallKeyframesloop at:366-375additionally skips static position holds (x/yonly,method === "set" || duration === 0). A.to("#el", { x: 100, duration: 0 })— method"to", noimmediateRender, x/y only — passes the first filter (becausesynthesizeFlatTweenKeyframesreturns non-null for non-setmethods) but is dropped by the second. Result:writeGsapAnimationsForElementwrites it, thenallKeyframes.length === 0 → clearKeyframeCacheForElementimmediately drops it. Self-correcting within one tick, but a wasted write and a briefly-observable stale reference.6ee750feecleans this up by extractingisStaticPositionHold(gsapTweenSynth.ts:18-23) and applying it to both filters. Noting for the record; not blocking here. -
Test-file
as GsapAnimationcast (gsapShared.test.ts:640). CONTRIBUTING.md §Type-safety asks for the honest form; suggestas unknown as GsapAnimationwith a one-line comment ("test fixture — only the fields used bytoClipKeyframesare relevant"), or bind the fixture through aPartial<GsapAnimation>-typed helper. Nit.
Peer state
Prior reviews: my R1 at e4d7bde (COMMENTED, 4 P2 / 3 P3), Rames' R2 at e4d7bde64f3b (COMMENTED, 7 inline findings with severities 🟠🟠🟡🟡🟡🟡🟢). Miguel replied to all 7 R2 inlines on 2026-07-27T14:48 with the identical "Addressed at stack tip in 6ee750fee" note — verified accurate at that SHA. CI is fully green at head: Test, Typecheck, Lint, Format, Fallow audit, File size check, CLI smoke (required), Perf: drift/fps/load/parity/scrub, Preview parity, Preflight, Studio: load smoke, Producer: integration/unit, Render on windows-latest, Tests on windows-latest, all shard runners — pass. Mergeable state blocked reflects stack-order coordination, not a defective PR.
Stamped. (COMMENTED — the two 🟠 R2 findings remain at this PR's head; they land fixed at the stack tip, so this PR should merge as part of the stack, not standalone. If Graphite orchestrates the stack merge atomically, the effect on main is clean.)
— Review by Via
|
Both R3 nits handled, and I agree on the merge-as-a-unit point. Nit 2 (bare Nit 1 ( While making the fixture change the
Verified at On the merge shape: agreed, this is a stack and #2781 should not go in standalone. Nothing here is being merged yet. |
…tack tip The R1/R3 residuals on this PR were fixed at the top of the stack, so they only cleared once every branch above landed. They belong here, next to the code they correct: - `idFromSelector` inverts `idSelector` for both regex readers, so the post-commit cache refresh stops skipping the CSS-unsafe ids `idSelector` exists to support. - `deduplicateKeyframes` drops `ease` when it is ambiguous; the flag was the only honest answer and the last-writer-wins curve belonged to an arbitrary colliding tween. - `isStaticPositionHold` is now the single owner of the hold skip. The `sourceAnimations` filter and the `allKeyframes` filter had diverged on whether `immediateRender` counts as a property. - The keyframe-cache setters no-op when the write changes nothing, instead of handing every subscriber a fresh Map. - `reset()` clears `focusedEaseSegment`. - The test hook `delete`s its window key rather than setting it to undefined, so feature detection still works. - The `toClipKeyframes` fixture uses `as unknown as T` with the justification CONTRIBUTING.md asks for.
Dragging the playhead to the start of the composition needed a very slow drag. The scrub surface begins GUTTER + TRACKS_LEFT_PAD px right of the viewport edge, and both scrub paths bailed out when the pointer sat left of that origin rather than clamping. So the last 80px of the drag toward zero silently did nothing: the playhead stuck at whatever the last in-range sample reported, and only a drag slow enough to sample inside the thin sliver before the origin ever reached 0. Both paths now share getTimelineScrubTime, which clamps to [0, duration]. One owner, so the live-feedback path and the committed-seek path cannot disagree about the edge again.
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed delta d05ecb109..b386b55f7 at commit b386b55f7.
The R3-residual hoist (3c7400af8) is a clean consolidation — every R1/R3 residual I flagged as "will land at stack tip" now closes here, so this slice stops depending on the rest of Family B landing. Verdicts against my R3 findings:
| R3 # | R3 finding | Status at b386b55f7 |
|---|---|---|
| F#1 | reset() didn't clear focusedEaseSegment |
✅ FIXED — playerStore.ts:534 focusedEaseSegment: null, inside the reset block |
| F#2 | Post-commit cache updater used raw /^#([\w-]+)/ regex, dropping CSS-unsafe ids |
✅ FIXED — new idFromSelector in gsapShared.ts:92-100 inverts both #id and [id="..."] shapes; both readers in gsapKeyframeCacheHelpers.ts (lines 21, 63) go through it; round-trip test at gsapShared.test.ts:158-164 covers the CSS-unsafe id classes (1box, 1"x, my.class, 01-hook-...) |
| F#3 | sourceAnimations filter and inline allKeyframes filter disagreed on immediateRender |
✅ FIXED — single owner isStaticPositionHold in gsapTweenSynth.ts:8-22 filters out immediateRender before checking property surface; both callsites in useGsapTweenCache.ts (lines 364, 465) delegate to it |
| F#5 | deduplicateKeyframes last-writer-wins on colliding ease |
✅ FIXED — gsapTweenSynth.ts:45-52 now if (existing.easeAmbiguous) delete existing.ease; else if (kf.ease) existing.ease = kf.ease; — ambiguous means "no single ease" instead of "arbitrary one of the colliding curves" |
| F#6 | Test hook set to undefined instead of delete |
✅ FIXED — useStudioTestHooks.ts:48-50 uses delete, so "__studioTest" in window no longer stays true post-unmount |
| F#7 | KeyframeSlice setters emitted new Map even on no-op | ✅ FIXED — keyframeSlice.ts:95-97 + keyframeSlice.ts:110-115 short-circuit on state.keyframeCache.get(elementId) === data (or !has(elementId) for the null-clear case) before allocating a fresh Map |
Plus the CONTRIBUTING.md-justified as unknown as T on toClipKeyframes fixture with the explanatory comment (gsapShared.test.ts:129-133) — matches the guidance from [[code-review-max-lens-gaps]].
The scrub-clamp commit (b386b55f7) is a clean fix for a real UX bug: dragging left of the scrub origin (GUTTER + TRACKS_LEFT_PAD) used to bail out instead of clamping, so the last ~80px of the drag toward t=0 silently no-op'd. Both scrub paths (useTimelinePlayhead.ts:134-140, useTimelineRangeSelection.ts:289-297) now go through getTimelineScrubTime which clamps to [0, duration] and guards !(pps > 0) + !Number.isFinite(duration) to 0. Tests at timelineLayout.test.ts:110-150 cover the origin, left-of-origin, past-end, degenerate-zoom, and NaN-duration paths. Nothing else calls the old inline formula (grepped packages/studio for - GUTTER - TRACKS_LEFT_PAD — only these two callsites).
Concerns
- 🟡 Sibling
extractIdFromSelectorinuseGsapTweenCache.ts:14-17still uses the raw/^#([\w-]+)/regex — the same anti-pattern R3-F#2 replaced elsewhere. It's called fromresolveSelectorElementIdsat line 39 (no-DOM fallback) and line 51 (querySelectorAll-throw fallback), so the impact is narrow: with a live preview iframe the DOM path resolves[id="..."]correctly, and only pre-iframe cache populate + invalid-selector recovery hit the raw regex. Still: exactly the same shape the R3 hoist just fixed, in the file family that already importsgsapShared. Suggestimport { idFromSelector } from "./gsapShared", delete the local helper, and route both fallback sites through the shared reader. Same call foruseGsapKeyframeOps.ts:341(selection.id ?? selection.selector?.match(/^#([\w-]+)/)?.[1] ?? null) — the short-circuit onselection.idmasks it in the common case but the fallback is still a raw-regex sibling.
What I didn't verify
- Whether the
useGsapTweenCache.ts:32bare-id fast-path (selector.match(/^#([\w-]+)$/)) needs symmetric handling. It's anchored^…$so[id="..."]correctly falls through to the DOM path — the fast-path is a performance optimization, not a correctness path, so this one likely doesn't matter.
Unrelated CI note
The failing Tests on windows-latest check is packages/engine/src/services/audioMixer.test.ts > processCompositionAudio > keeps the ffmpeg command line short with a large track count (regression for spawn ENAMETOOLONG). HF#2781's diff is entirely inside packages/studio/*, so this can't be a regression from this PR — looks like Windows-only flakiness on the engine's ffmpeg-command-length regression test that landed in an earlier PR (last touched 37b88688e "fix(audio): preserve causes and use portable padding" by James in #2769). Worth checking main at the same SHA to confirm preexisting rather than debugging here.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at b386b55f735a.
Verdict
APPROVE. Both R2 🟠 residuals I held on at d05ecb109 are now closed inside this PR itself — no more depending on stack-tip landings. The second delta commit (b386b55f7, scrub-clamp) is a clean net-add with adequate test coverage. Non-blocker CI failure on Tests on windows-latest is packages/engine/src/services/audioMixer.test.ts (5s test timeout on the ffmpeg-command-length regression) and this PR touches nothing under packages/engine.
Delta since R2 (d05ecb109 → b386b55f7)
Two commits, ahead_by: 2, behind_by: 0 — no rebase, so no new-base regression surface.
| SHA | Subject | Scope |
|---|---|---|
3c7400af8 |
fix(studio): close the review findings in this PR instead of at the stack tip |
Hoists the 6 fixes from #2791's tip down into this PR (KeyframeSlice + gsapShared + tests) |
b386b55f7 |
fix(studio): clamp the timeline scrub to 0 instead of dropping it |
New scrub-clamp helper + 2 caller updates + test file |
R2 🟠 residuals — resolution status at this head
Both closed in 3c7400af8.
- ✅
focusedEaseSegmentreset omission —playerStore.ts:534now includesfocusedEaseSegment: null,inside thereset()block (lines 520-546). The reset happens on composition switch and clears every stack tip's writes uniformly. No other keyframe-mutation path allocates state that needs a symmetric clear. - ✅ Post-commit id-regex reader unsafe for CSS-hostile ids — the two id extractors in
gsapKeyframeCacheHelpers.ts(lines 21, 63) now go throughidFromSelector(gsapShared.ts:94-102), which inverts BOTH the#idshape and the[id="…"]attribute-selector shape thatidSelectoremits for digit-leading / dotted / spaced ids. Round-trip test atgsapShared.test.ts:85-112covers the CSS-unsafe id classes explicitly (01-hook-hero-word,my.class,1box,1"x).
R2 non-blocker observations — status
- ✅
sourceAnimationsvsallKeyframesfilter divergence on static holds —isStaticPositionHold(gsapTweenSynth.ts:18-23) is now the single owner. It filtersimmediateRenderout of the property surface before checking. Both callsites inuseGsapTweenCache.ts(lines 364, 465) delegate to it — no inline filter left to drift. - ✅ Bare
as GsapAnimationin test fixture —gsapShared.test.ts:132-138now usesas unknown as GsapAnimationwith the CONTRIBUTING.md-justification comment at lines 128-131 (// Fixture carries only the fields the function under test reads; the double-cast is the documented way to stand in for the full runtime shape (CONTRIBUTING.md).).
Byte-clean audit on prior-audited surface
Every file I audited at d05ecb109 changed in 3c7400af8 and was re-read at b386b55f7:
| File | Delta @ new head | Notes |
|---|---|---|
keyframeSlice.ts |
+13 | No-op guards on setKeyframeCache (lines 98-105) and setGsapAnimations (lines 110-119) — bonus fix beyond the deferred residuals |
playerStore.ts |
+1 | focusedEaseSegment: null, in reset() (line 534) |
gsapShared.ts |
+21 | New idFromSelector export (lines 94-102) |
gsapShared.test.ts |
+20/-2 | Round-trip tests + fixture cast fix |
gsapTweenSynth.ts |
+25/-1 | isStaticPositionHold extracted (lines 18-23) + deduplicateKeyframes ease-drop (lines 45-52) |
useGsapTweenCache.ts |
+7/-23 | Callers moved to isStaticPositionHold |
useStudioTestHooks.ts |
+3/-1 | delete instead of undefined (line 50) |
gsapKeyframeCacheHelpers.ts |
+3/-4 | Callers moved to idFromSelector |
Adversarial audit on b386b55f7 (scrub-clamp)
Adversarial six-category on the delta:
- State discard / clamp semantics.
getTimelineScrubTime(timelineLayout.ts:334-345):Math.max(0, Math.min(duration, x / pixelsPerSecond)). Guards!(pixelsPerSecond > 0)and!Number.isFinite(duration)to0— the two degenerate cases the pre-fix inline code punted on. Tests attimelineLayout.test.ts:110-151cover origin, left-of-origin, past-end,pps=0, andNaNduration. - Return invariant. Return type
number, nevernullorNaN— every branch returns a finite number in[0, duration]. Callers can no longer decide to bail; the helper always produces a scrub target. - Library-default / silent-catch. No
try/catch, no default-arg surprises.durationis required. - Precedence — arithmetic bug audit.
clientX - viewportLeft + scrollLeft - GUTTER - TRACKS_LEFT_PADmatches the pre-fix inline form in both callers verbatim (grep confirms — the fix is a literal extract). Signs correct: content-space x = viewport-relative pointer + scroll offset - gutters. - Session / lifecycle ownership. Callers own the RAF batching (
updateScrubDrag'sseekRafRef,seekFromX's direct notify). The helper is pure — no side effects, no captured refs — souseCallbackdeps in both callers remain minimal. - Discovery completeness — parity across scrub surfaces. Grepped
packages/studiofor the raw form- GUTTER - TRACKS_LEFT_PAD— hits only the two shift-drag range-selection sites (useTimelineRangeSelection.ts:232, 318), which are a different gesture (Shift+drag time range) and intentionally useMath.max(0, x/pps)without an upper clamp.BeatStrip.tsxscrubs beat time (Math.max(0, t)), also intentionally unclamped upward. So the "cannot disagree about the edge" invariant holds for the pair Miguel names.
Standards lens re-run
- CONTRIBUTING.md guidance (test-fixture double-cast): honored at
gsapShared.test.ts:129-131. - Fallow / complexity: no new suppressions; existing
fallow-ignore-next-line complexityonsynthesizeFlatTweenKeyframes(line 60) is untouched. - No barrel files: the extract adds named exports directly; no new index files.
- File-size:
useGsapTweenCache.tsnet −16,gsapTweenSynth.ts+25 — both stay under the 600-line ceiling the header cites.
Empty count aside, no Standards-lens violations in the delta.
Concerns
- 🟡 Sibling
extractIdFromSelectorinuseGsapTweenCache.ts:18-21still uses the raw/^#([\w-]+)/regex — same anti-pattern R2-F2 replaced elsewhere, in a file that already imports fromgsapShared. Called fromresolveSelectorElementIdsat lines 39 (no-DOM fallback) and 51 (querySelectorAll-throw fallback). Impact is narrow: with a live iframe the DOM path resolves[id="…"]correctly, and only pre-iframe cache populate + invalid-selector recovery hit the raw regex. Rames independently flagged the same atb386b55f7(n=2 lens convergence). Follow-up:import { idFromSelector } from "./gsapShared", delete the local helper. Pre-existing onmain— not a regression from this PR, not blocking.
Peer state
- Rames reviewed at
b386b55f7(COMMENTED, submitted2026-07-27T17:48:42Z). Verdict table converges with mine on all six R3-numbered fixes and on the scrub-clamp audit. The 🟡 siblingextractIdFromSelectorconcern above matches his findings. - No CODEOWNER block, no CHANGES_REQUESTED review.
CI
- 39 of 40 checks pass at this head.
Tests on windows-latest(job90053346540) fails onpackages/engine/src/services/audioMixer.test.ts:626 > processCompositionAudio > keeps the ffmpeg command line short with a large track count:Test timed out in 5000ms. This PR's diff is entirely insidepackages/studio/*— zero causal relationship to apackages/engineffmpeg-spawn perf test. Prior head (d05ecb109) passed the same Windows job (job90002783496). Classic Windows-runner flake, not a regression.mainis unprotected; Graphite mergeability check passed.
Envelope hygiene
- No
Co-Authored-Bylines in the PR body or in either new commit's message. - Both new commits authored + committed by
miguel.sierra@heygen.com. - Scope of the delta stays inside
packages/studio/**— no cross-package sprawl.
Stamped.
— Review by Via
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at b386b55f735a.
Verdict
APPROVE. Both R2 🟠 residuals I held on at d05ecb109 are now closed inside this PR itself — no more depending on stack-tip landings. The second delta commit (b386b55f7, scrub-clamp) is a clean net-add with adequate test coverage. Non-blocker CI failure on Tests on windows-latest is packages/engine/src/services/audioMixer.test.ts (5s test timeout on the ffmpeg-command-length regression) and this PR touches nothing under packages/engine.
Delta since R2 (d05ecb109 → b386b55f7)
Two commits, ahead_by: 2, behind_by: 0 — no rebase, so no new-base regression surface.
| SHA | Subject | Scope |
|---|---|---|
3c7400af8 |
fix(studio): close the review findings in this PR instead of at the stack tip |
Hoists the 6 fixes from #2791's tip down into this PR (KeyframeSlice + gsapShared + tests) |
b386b55f7 |
fix(studio): clamp the timeline scrub to 0 instead of dropping it |
New scrub-clamp helper + 2 caller updates + test file |
R2 🟠 residuals — resolution status at this head
Both closed in 3c7400af8.
- ✅
focusedEaseSegmentreset omission —playerStore.ts:534now includesfocusedEaseSegment: null,inside thereset()block (lines 520-546). The reset happens on composition switch and clears every stack tip's writes uniformly. No other keyframe-mutation path allocates state that needs a symmetric clear. - ✅ Post-commit id-regex reader unsafe for CSS-hostile ids — the two id extractors in
gsapKeyframeCacheHelpers.ts(lines 21, 63) now go throughidFromSelector(gsapShared.ts:94-102), which inverts BOTH the#idshape and the[id="…"]attribute-selector shape thatidSelectoremits for digit-leading / dotted / spaced ids. Round-trip test atgsapShared.test.ts:85-112covers the CSS-unsafe id classes explicitly (01-hook-hero-word,my.class,1box,1"x).
R2 non-blocker observations — status
- ✅
sourceAnimationsvsallKeyframesfilter divergence on static holds —isStaticPositionHold(gsapTweenSynth.ts:18-23) is now the single owner. It filtersimmediateRenderout of the property surface before checking. Both callsites inuseGsapTweenCache.ts(lines 364, 465) delegate to it — no inline filter left to drift. - ✅ Bare
as GsapAnimationin test fixture —gsapShared.test.ts:132-138now usesas unknown as GsapAnimationwith the CONTRIBUTING.md-justification comment at lines 128-131 (// Fixture carries only the fields the function under test reads; the double-cast is the documented way to stand in for the full runtime shape (CONTRIBUTING.md).).
Byte-clean audit on prior-audited surface
Every file I audited at d05ecb109 changed in 3c7400af8 and was re-read at b386b55f7:
| File | Delta @ new head | Notes |
|---|---|---|
keyframeSlice.ts |
+13 | No-op guards on setKeyframeCache (lines 98-105) and setGsapAnimations (lines 110-119) — bonus fix beyond the deferred residuals |
playerStore.ts |
+1 | focusedEaseSegment: null, in reset() (line 534) |
gsapShared.ts |
+21 | New idFromSelector export (lines 94-102) |
gsapShared.test.ts |
+20/-2 | Round-trip tests + fixture cast fix |
gsapTweenSynth.ts |
+25/-1 | isStaticPositionHold extracted (lines 18-23) + deduplicateKeyframes ease-drop (lines 45-52) |
useGsapTweenCache.ts |
+7/-23 | Callers moved to isStaticPositionHold |
useStudioTestHooks.ts |
+3/-1 | delete instead of undefined (line 50) |
gsapKeyframeCacheHelpers.ts |
+3/-4 | Callers moved to idFromSelector |
Adversarial audit on b386b55f7 (scrub-clamp)
Adversarial six-category on the delta:
- State discard / clamp semantics.
getTimelineScrubTime(timelineLayout.ts:334-345):Math.max(0, Math.min(duration, x / pixelsPerSecond)). Guards!(pixelsPerSecond > 0)and!Number.isFinite(duration)to0— the two degenerate cases the pre-fix inline code punted on. Tests attimelineLayout.test.ts:110-151cover origin, left-of-origin, past-end,pps=0, andNaNduration. - Return invariant. Return type
number, nevernullorNaN— every branch returns a finite number in[0, duration]. Callers can no longer decide to bail; the helper always produces a scrub target. - Library-default / silent-catch. No
try/catch, no default-arg surprises.durationis required. - Precedence — arithmetic bug audit.
clientX - viewportLeft + scrollLeft - GUTTER - TRACKS_LEFT_PADmatches the pre-fix inline form in both callers verbatim (grep confirms — the fix is a literal extract). Signs correct: content-space x = viewport-relative pointer + scroll offset - gutters. - Session / lifecycle ownership. Callers own the RAF batching (
updateScrubDrag'sseekRafRef,seekFromX's direct notify). The helper is pure — no side effects, no captured refs — souseCallbackdeps in both callers remain minimal. - Discovery completeness — parity across scrub surfaces. Grepped
packages/studiofor the raw form- GUTTER - TRACKS_LEFT_PAD— hits only the two shift-drag range-selection sites (useTimelineRangeSelection.ts:232, 318), which are a different gesture (Shift+drag time range) and intentionally useMath.max(0, x/pps)without an upper clamp.BeatStrip.tsxscrubs beat time (Math.max(0, t)), also intentionally unclamped upward. So the "cannot disagree about the edge" invariant holds for the pair Miguel names.
Standards lens re-run
- CONTRIBUTING.md guidance (test-fixture double-cast): honored at
gsapShared.test.ts:129-131. - Fallow / complexity: no new suppressions; existing
fallow-ignore-next-line complexityonsynthesizeFlatTweenKeyframes(line 60) is untouched. - No barrel files: the extract adds named exports directly; no new index files.
- File-size:
useGsapTweenCache.tsnet −16,gsapTweenSynth.ts+25 — both stay under the 600-line ceiling the header cites.
Empty count aside, no Standards-lens violations in the delta.
Concerns
- 🟡 Sibling
extractIdFromSelectorinuseGsapTweenCache.ts:18-21still uses the raw/^#([\w-]+)/regex — same anti-pattern R2-F2 replaced elsewhere, in a file that already imports fromgsapShared. Called fromresolveSelectorElementIdsat lines 39 (no-DOM fallback) and 51 (querySelectorAll-throw fallback). Impact is narrow: with a live iframe the DOM path resolves[id="…"]correctly, and only pre-iframe cache populate + invalid-selector recovery hit the raw regex. Rames independently flagged the same atb386b55f7(n=2 lens convergence). Follow-up:import { idFromSelector } from "./gsapShared", delete the local helper. Pre-existing onmain— not a regression from this PR, not blocking.
Peer state
- Rames reviewed at
b386b55f7(COMMENTED, submitted2026-07-27T17:48:42Z). Verdict table converges with mine on all six R3-numbered fixes and on the scrub-clamp audit. The 🟡 siblingextractIdFromSelectorconcern above matches his findings. - No CODEOWNER block, no CHANGES_REQUESTED review.
CI
- 39 of 40 checks pass at this head.
Tests on windows-latest(job90053346540) fails onpackages/engine/src/services/audioMixer.test.ts:626 > processCompositionAudio > keeps the ffmpeg command line short with a large track count:Test timed out in 5000ms. This PR's diff is entirely insidepackages/studio/*— zero causal relationship to apackages/engineffmpeg-spawn perf test. Prior head (d05ecb109) passed the same Windows job (job90002783496). Classic Windows-runner flake, not a regression.mainis unprotected; Graphite mergeability check passed.
Envelope hygiene
- No
Co-Authored-Bylines in the PR body or in either new commit's message. - Both new commits authored + committed by
miguel.sierra@heygen.com. - Scope of the delta stays inside
packages/studio/**— no cross-package sprawl.
Stamped.
— Review by Via
The local extractIdFromSelector duplicated the `#id`-only regex that idFromSelector replaced, so both DOM-less paths in resolveSelectorElementIds (no-iframe fallback and querySelectorAll-throw recovery) read no id at all for the bracketed `[id="..."]` form writers emit for CSS-unsafe ids. Deleted the duplicate and imported the shared reader; both forms now resolve.
|
Consolidated in
The On the scrub clamp, verified against a live studio rather than only in unit tests. Driving the real timeline in a browser, pre-fix vs post-fix on the same three gestures:
That last row is the reported symptom exactly: the press was ignored, not clamped, so only a slow drag that happened to sample inside the 80px sliver ever landed on 0. |
3 participants
What
Adds the shared Studio state and cache primitives for keyframe-aware timeline authoring.
Why
Timeline lanes need one canonical source for discovered GSAP keyframes, selection identity, and edit state before rendering or interaction layers can safely consume them.
How
Introduces focused keyframe state/cache helpers and hooks, with identity and state transitions covered independently. This is B1 of the Family B draft Graphite stack and targets the immutable PR-less Family A review baseline.
Test plan
Exact Family B tip validation: 2,910 Studio tests, 398 Studio Server tests, both package typechecks, formatting, lint, diff check, file-size gate, and Fallow audit passed.
Deferred review findings
Every blocker and high finding raised on this PR is fixed in the stack. The 6 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/01-pr-2683-deferred-review-findings.md:packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:22— id extraction regex /^#([\w-]+)/ isn't paired with the new idSelector attribute-selector outputpackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:77— Synthesized flat-tween's top-level ease is dropped when written into keyframeCachepackages/studio/src/hooks/gsapKeyframeCacheHelpers.ts:59— Ambiguity-flag logic is inlined identically in two places — contract drift risk across foundation slicepackages/studio/src/player/store/keyframeSlice.ts:36— expandClips has no bulk-collapse counterpartpackages/studio/src/hooks/useStudioTestHooks.ts:45— window.__studioTest is reassigned every effect re-run and stomps on any prior handlepackages/studio/src/player/store/keyframeSlice.ts:26— selectedKeyframes stores two disjoint key shapes with no discriminator or helperSupersedes #2683, which was closed when
mainwas rewritten to unwind an early landing of this stack. Same head commit, same review history.R1 review follow-ups
Fixed in this PR:
gsapAnimationsno longer disagree on which tweens they admit. The property-group gate is gone from both writers, so a mixed-group tween ({ x, opacity }classifies to no group) can no longer cache diamonds with no source animation behind them.Not changed, with rationale:
replaceSelectedKeyframes/selectRangeKeyframesare skipped as YAGNI. No consumer exists in the stack and the unused-exports gate would flag both.timelineKeyframeIdentity.ts(fix(studio): harden keyframe editing semantics #2787).The 3 remaining low findings are parked in
.scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.R2 review follow-ups
Fixed in this PR:
toClipPercentagenow owns the rounding andtoClipKeyframesowns the whole row (percentage plus the tween percentage and animation identity the lanes read); the parsed write reuseselementCacheKeysinstead of open-coding its three key variants.Deliberate, stated for the record:
window.__studioTest(packages/studio/src/hooks/useStudioTestHooks.ts) is a headless-QA shortcut, not a product surface. It is gated onimport.meta.env.DEV, cleared on unmount, and never defined in a production build.