feat(studio): add timeline property lanes#2686
Conversation
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 c8708ec567ee.
New per-property lane surface. Pure SVG, no HTML-in-canvas concern. Two orange concerns: memo defeated by fresh keyframesData literal per render (every playhead tick re-renders every lane), and per-property claim invisible to assistive tech (role="group" + property-scoped aria labels missing). One correctness note on globalEase-from-first-animation disagreeing with the ease editor for multi-animation groups.
Findings not anchored inline
🟠 Diamond and segment-ease buttons in property lanes carry no property in their accessible name
packages/studio/src/player/components/TimelineClipDiamonds.tsx:478
// diamond button: only title, no aria-label
<button ... title={`${kf.percentage}%`} .../>
// segment-ease button:
aria-label={`Edit ${ease} easing`}Diamond button has only title={${kf.percentage}%} and no aria-label; segment-ease button (:274) has aria-label={Edit ${ease} easing} — mentions ease curve name but not property or timestamp. Not diff-changed, but the diff introduces a NEW context that makes labeling insufficient: when the same button component now appears once per property in per-property lanes (TimelinePropertyLanes.tsx:139-158), a keyboard/AT user tabbing through a clip with Position + Visual + Scale lanes hears three identical 50% buttons and three identical Edit power2.out easing buttons with no way to distinguish them. Combined with the missing lane group role, the per-property claim is invisible to AT.
Fix: Include property in the aria-label — e.g. Edit ${property} ${ease} easing at ${percentage}%. Pass a property prop down from TimelinePropertyLanes to the segment-ease + diamond render.
🟢 Verified clean
- Property lane geometry math (top/height offsets) is derived from timelineLayout constants — deterministic
- Segment-ease button routing via animationId is correct — click opens the right animation's ease editor
- sourceGroups uses stable property-group classification from classifyPropertyGroup
Cross-stack notes
The perf + a11y findings compound in #2687 (which adds track headers) and #2690 (which wires everything into Timeline). #2687's own hoveredGroup state (see #2687 finding 5) is another parent-render trigger that cascades through this un-memoized lane path. Suggest addressing finding 1 (memo defeat) before Family B tip — otherwise every playhead tick re-renders every lane in every expanded clip.
| suppressClickRef, | ||
| }: TimelinePropertyLanesProps) { | ||
| if (clipWidthPx < 20 || clipDuration <= 0) return null; | ||
| const lanes = getTimelinePropertyLanes(animations, clipStart, clipDuration); |
There was a problem hiding this comment.
🟠 TimelineDiamondLane's memo is defeated per render — every parent re-render fully re-renders every lane (flicker-lens)
const lanes = getTimelinePropertyLanes(animations, clipStart, clipDuration);
// ...
<TimelineDiamondLane
keyframesData={{ format: "percentage", keyframes }}
...
/>getTimelinePropertyLanes(animations, clipStart, clipDuration) is called inline (:120, no useMemo), returning a fresh array whose entries each contain a fresh keyframes array. Then at :140, keyframesData={{ format: "percentage", keyframes }} allocates a fresh object literal per lane per render. TimelineDiamondLane is memo(...) (TimelineClipDiamonds.tsx:114) with default shallow-equal props — so keyframesData (fresh object) always compares unequal, and every lane's diamonds + segment strip fully rebuild on every parent re-render. The collapsed-row equivalent (TimelineClipDiamonds.tsx:511-514) forwards its keyframesData prop through unchanged, so upstream stability was preserved; the new property-lane path breaks that. HF flicker lens: playhead moves, hovers, or any timeline-wide state tick will re-render every diamond in every expanded clip.
Fix: useMemo(() => getTimelinePropertyLanes(animations, clipStart, clipDuration), [animations, clipStart, clipDuration]), and hoist the { format, keyframes } wrapper into that memo output.
| if (lanes.length === 0) return null; | ||
| return ( | ||
| <> | ||
| {lanes.map(({ group, animations: groupAnimations, keyframes }, laneIndex) => ( |
There was a problem hiding this comment.
🟠 Property-lane containers convey identity only via data- attributes, not to assistive tech*
<div
data-property-group={group}
data-timeline-property-lane=""
data-timeline-lane-top={...}
className="absolute"
style={{ ... }}
>Lane wrapper has no role="group", no aria-label, no aria-labelledby. PR body claims property lanes solve Clip-level summary alone cannot expose which property owns a keyframe, but the accessible tree currently exposes nothing more than the collapsed row did. SR users tabbing across Position and Visual lanes hear only the diamond button labels (see next finding), with no announcement that a group boundary was crossed.
Fix: Add role="group" + aria-label={${group} keyframes} on this div.
| * — tracked per-clip so a later user collapse sticks and never bounces back open | ||
| * (and clips added later still auto-expand). | ||
| */ | ||
| export function useAutoExpandKeyframedClips(gsapAnimations: Map<string, GsapAnimation[]>): void { |
There was a problem hiding this comment.
🟡 "Later user collapse sticks and never bounces back open" only holds within one hook-component lifetime
// ref-scoped bookkeeping
const seen = useRef<{ projectId: ...; source: ...; clips: Set<string> }>({
projectId: null,
source: null,
clips: new Set(),
});
// ...
expandClips(fresh)Invariant relies entirely on the useRef at :17 to remember which clips have already been auto-expanded. expandClips in keyframeSlice.ts:81-87 is ADDITIVE — always re-adds passed ids, never respecting the store's current absence. If the hosting component unmounts and remounts (route change, conditional render, parent key change), seen.current.clips resets to new Set(), the effect walks every clip that contributes a lane, calls expandClips(fresh) for all of them, and every previously user-collapsed clip pops back open. Test file covers project-switch and same-project source-swap paths but never a hook remount, so the invariant looks stronger in the test than at runtime.
Fix: Persist the "auto-expanded once per (projectId, clipId)" bookkeeping into the store (co-located with expandedClipIds) so the guarantee survives component remounts.
| > | ||
| <TimelineDiamondLane | ||
| keyframesData={{ format: "percentage", keyframes }} | ||
| globalEase={ |
There was a problem hiding this comment.
🟡 globalEase fallback for a group is taken from animations[0] regardless of which animation a keyframe belongs to
globalEase={groupAnimations[0]?.keyframes?.easeEach ?? groupAnimations[0]?.ease ?? "none"}Read once per lane and then applied to every keyframe in that lane that lacks explicit .ease (TimelineClipDiamonds.tsx:234 const ease = kf.ease ?? globalEase;). A property group can contain multiple animations (sourceGroups: :51-60 collects GsapAnimation[] per group), each keyframe carries animationId (:81). If group position has two tweens with different eases (power1.out on A, power2.in on B) and a keyframe on B has no per-keyframe ease, the MiniCurveSvg drawn on its segment ease button will show A's ease curve — even though clicking correctly routes to B via animationId. Contract drift vs Family A #2674's ease editor: editor opens on B's ease, timeline-side preview shows A's — the two components disagree about what curve is being edited.
Fix: Look up globalEase per keyframe via kf.animationId → its animation's ease/easeEach, or attach the resolved ease to the keyframe entry at synthesis time.
| const seen = useRef({ projectId, source: gsapAnimations, clips: new Set<string>() }); | ||
| useEffect(() => { | ||
| if (!STUDIO_KEYFRAMES_ENABLED) return; | ||
| if (seen.current.projectId !== projectId) { |
There was a problem hiding this comment.
🟢 Project switch that reuses the previous project's Map identity silently leaves new project fully collapsed
if (seen.current.projectId !== projectId) {
const sourceChanged = seen.current.source !== gsapAnimations;
seen.current = { projectId, source: gsapAnimations, clips: new Set() };
if (!sourceChanged) return;
}When projectId changes but the same Map instance is passed, the ref is reset AND the effect returns without expanding — so the new project renders with NO auto-expansion until a subsequent render supplies a distinct Map. Test at useAutoExpandKeyframedClips.test.tsx:60-64 documents and pins this behavior. In real usage the caller almost certainly produces a fresh Map per project, but if any consumer memoizes the Map across projects, the timeline for the freshly opened project will be silently unexpandable until the next data change.
Fix: Unconditionally expand after a projectId change rather than gating on sourceChanged.
|
|
||
| /** The tween's editable keyframes: its real keyframes, or the start→end pair | ||
| * synthesized for a flat tween. Empty for a tween that animates nothing. */ | ||
| function animationKeyframes(animation: GsapAnimation) { |
There was a problem hiding this comment.
🟢 synthesizeFlatTweenKeyframes runs twice per flat-tween animation per render
// animationKeyframes helper called from animationContributesLane (:54) AND groupKeyframes (:73)animationKeyframes (:41-43) is called first by animationContributesLane (via sourceGroups :54) to decide whether the animation contributes a lane, and again by groupKeyframes (:73) to produce the diamonds. For any flat-tween animation both invocations hit synthesizeFlatTweenKeyframes(animation) — a second synthesis pass with no cache. On its own not a bug, but combined with the memo-defeat this compounds.
Fix: A single Map<GsapAnimation, TimelineDiamondKeyframe[]> local to getTimelinePropertyLanes (inside a useMemo) would eliminate both this and finding 1.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at c8708ec567ee.
Per-property lane surface. Rames's R1 caught two orange concerns (memo defeated by a fresh keyframesData literal per render, and the per-property claim being invisible to AT because diamond + segment-ease aria-label don't include the property name — three identical "50%" buttons in a Position + Visual + Scale clip). I want to add one Lens 6 (cross-slice pattern propagation) finding on the callback type contract that isn't repaired within this slice.
Prior review
Rames D Jusso R1: (1) memo defeat on keyframesData literal, (2) diamond + segment-ease aria-label missing property, (3) globalEase-from-first-animation disagrees with the ease editor for multi-animation groups. Overlap: I verified findings 2 and 3 in the diff; finding 1 (perf) I did not benchmark but the code shape is consistent with Rames's analysis (fresh { format: "percentage", keyframes } object per parent render defeats memo).
🟠 Additive — Lens 6: onMoveKeyframe type-contract drops the Promise<boolean> return
packages/studio/src/player/components/TimelinePropertyLanes.tsx:28
onMoveKeyframe?: (target: TimelineKeyframeTarget, toClipPercentage: number) => void;But TimelineDiamondLane (from #2685) declares onMoveKeyframe?: (target, toClipPercentage) => Promise<boolean> and calls onMoveKeyframe?.(fromTarget, res.toClipPct).then((committed) => …). When TimelinePropertyLanes passes its void-returning onMoveKeyframe to TimelineDiamondLane, TypeScript's covariant return check should reject the assignment (void is not assignable to Promise<boolean> in strict mode). Since the PR passes typecheck, either bivariance is on or the mismatch is elided — either way the runtime contract is: if a real consumer wired a void-returning callback here, TimelineDiamondLane would call .then(undefined) on the result and throw. This is why the pending-retime path in #2685 is dead code Rames flagged.
Fixed at #2689 — that slice widens the type to => Promise<boolean> and exports animationContributesLane (used by #2690). But this slice on its own has a broken type contract, and stacked-PR merge order matters if any intermediate consumer lands.
🟢 Verified
sourceGroupsiteration usesclassifyPropertyGroupfor stable classification (property→group).- Lane geometry math (
getTimelineLaneTop(laneIndex)+LANE_Hheight) is derived fromtimelineLayoutconstants only — deterministic and testable. animationContributesLanegate correctly requirespropertyGroup && animationKeyframes(animation).length > 0— a flat tween of pure non-numeric props ({ backgroundColor: "#fff" }) resolves to[]viasynthesizeFlatTweenKeyframesnumeric-only filter in #2683 → lane is correctly skipped.groupKeyframestweenPercentage: keyframe.percentagepreserves the original tween-relative % (as the retime mutation keys on it) whilepercentageis clip-relative for rendering — the two-percentage contract stays clean.- Segment-ease button routing via
animationId(viakeyframeTarget(kf, groupAware=true)fromTimelineClipDiamonds) opens the correct animation's ease editor.
Cross-stack
Perf + a11y compound in #2687 (hoveredGroup local state) and #2690 (lane render on every playhead tick). Rames noted this in his write-up.
Verdict
Recommending COMMENTED — Rames's memo + a11y findings are correctness-adjacent (silently degrades AT support for the exact "per-property" claim in the PR body), and the callback-type drift IS a defect in isolation even though #2689 heals it.
— Review by Via
a1c9ebd to
8ecae47
Compare
c8708ec to
c4d4c12
Compare
8ecae47 to
5816e70
Compare
c4d4c12 to
a9bdade
Compare
Fallow audit reportFound 3 findings. Duplication (2)
Health (1)
Generated by fallow. |
3 participants
What
Adds expandable per-property keyframe lanes and segment easing controls.
Why
A clip-level summary alone cannot expose which property owns a keyframe or which segment ease is being edited.
How
Renders property-specific diamonds, connecting segments, accessible ease controls, and navigation affordances from the canonical lane model. This is B4 of the Family B draft Graphite stack.
Test plan
Exact Family B tip validation: 2,865 Studio tests, 67 Studio Server route 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 4 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/04-pr-2686-deferred-review-findings.md:packages/studio/src/player/components/useAutoExpandKeyframedClips.ts:14— "Later user collapse sticks and never bounces back open" only holds within one hook-component lifetimepackages/studio/src/player/components/TimelinePropertyLanes.tsx:141— globalEase fallback for a group is taken from animations[0] regardless of which animation a keyframe belongs topackages/studio/src/player/components/useAutoExpandKeyframedClips.ts:20— Project switch that reuses the previous project's Map identity silently leaves new project fully collapsedpackages/studio/src/player/components/TimelinePropertyLanes.tsx:41— synthesizeFlatTweenKeyframes runs twice per flat-tween animation per render