feat(studio): add keyframe track headers#2785
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a9bdade to
6bc8d54
Compare
7f86018 to
86e9735
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — head 86e9735.
Track-header contract is clean, the extract-to-helper is safe (both KeyframeNavigation callsite and the new lane header type-check under the generic NavigableKeyframe), and the alignment test (assertAligned) pins the pixel-perfect lane contract with #2784. Findings below are all P2/P3, non-blocking.
Adversarial lenses
Lens D — Interaction bleed with property lanes (P2). Half the controls stop propagation, half don't. Consistent stoppers: LayerDisclosureRow's caret (packages/studio/src/player/components/LayerDisclosureRow.tsx:100-104, both onPointerDown and onClick) and VisibilityButton (TimelineTrackHeader.tsx:559-563). Non-stoppers: PropertyGroupNavigation's prev/next chevrons (TimelineTrackHeader.tsx:736,745 — onClick={() => seekTo(...)} with no stopPropagation) and the toggle diamond (TimelineTrackHeader.tsx:832-836). Since sibling buttons in the same absolute-positioned label column deliberately stop propagation, clicks that leak from the chevrons/toggle up to whatever the ancestor track-row does are the likely regression path here. Match the sibling pattern.
Lens B — Truncation without tooltip fidelity (P2). Three ellipsis'd spans have no title attribute → mouse users get no full-name reveal, touch users are stuck: layer name in LayerDisclosureRow.tsx:119 (min-w-0 flex-1 truncate font-medium), track label in LegacyTrackHeader at TimelineTrackHeader.tsx:590, and the value readout at TimelineTrackHeader.tsx:841. The two <button>s in the same subtree already carry title= — adding it to the truncatable spans is a two-line fix.
Lens E — Contract with #2784 (verified clean). Header count = lane count (both derive from getTimelinePropertyLanes(animations, ...); the LegacyTrackHeader fallback fires when lanes.length === 0). [data-timeline-lane-top] offsets match TimelinePropertyLanes via getTimelineLaneTop(laneIndex) — asserted by the alignment test at TimelineTrackHeader.test.tsx:342-386. No orphan header/lane path.
Lens A — Disclosure state (deferred, not new). State ownership is upstream (isExpanded is a prop). Caret click is isolated to the button; wrapper <div> has no click handler, so header-body vs. caret can't diverge here.
Lens C — Reorder (N/A). No drag-to-reorder in this PR.
Nits (P3)
TimelineTrackHeader.tsx:519-524—visualValueReadoutshows raw values when|opacity| > 1, which for a percentage readout is a leaky formatting branch. Won't fire on valid GSAP opacity.TimelineTrackHeader.test.tsx:245-249— locks in the linear-sampling behaviour (tweenPercentage: 25→opacity: 0.25). Per the PR body, the ease-following sampler lands at the stack tip; that PR will need to update this expectation.- The 4 deferred findings enumerated in the PR body all match the diff — verified.
— 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 86e9735511ec.
R2 adversarial pass on the track-headers slice. Three orange a11y items on the interactive layer: LayerDisclosureRow trigger missing aria-expanded/aria-controls (disclosure widget's whole contract), the hover-only Eye button unmounting means keyboard users can't reach it on any lane where laneIndex !== 0, and the O(lanes × keyframes) hover-driven recompute on every pointer-enter/leave. Plus the same propertyGroup?: string widening pattern Via flagged in a different form.
🟢 Verified clean
- KeyframeNavigation.tsx: getKeyframeNavigationState generic extraction preserves prev/current/next semantics + TOLERANCE boundaries
- timelineLayout.ts: single additive LABEL_COL_W=232 constant, no behavior change
- TimelineTrackHeader.test.tsx: happy-dom coverage of disclosure + navigation
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.
6bc8d54 to
913fe08
Compare
0b9eff8 to
cf1cc8c
Compare
913fe08 to
0509de7
Compare
The prev/next keyframe chevrons and the group toggle diamond let their click bubble to the ancestor track row, so seeking to a keyframe also reselected the track. The disclosure caret and the eye already stop it; these now match. Truncated labels (layer name, track label, group label, value readout) also carry a title so the full text is reachable on hover.
The header file owned value sampling, readout formatting, lane-state resolution and the JSX at once, so a formatting change and a layout change edited the same file. Sampling and formatting now live in trackHeaderLaneValues, lane-state resolution in trackHeaderLaneState, and resolveLaneHeaderState returns only the four fields its caller reads. Also shows the track's clip count next to the track identity, which the header promised but never rendered.
LegacyTrackHeader reads as deprecated code. It is the live path for every track that has no keyframe clip to disclose, so call it PlainTrackHeader and say so in a comment.
toggleTarget is derived from expandedElement, so it can only be set when expandedElement is. The extra check read as if the two could disagree.
0509de7 to
4a8322f
Compare
cf1cc8c to
3795067
Compare
3 participants
What
Adds compact keyframe-aware track headers and lane expansion controls.
Why
Users need a clear, low-density way to understand track identity, clip count, visibility, and whether property lanes are expanded.
How
Introduces the track-header presentation and interaction contract while keeping selection and expansion state owned by the existing timeline model. This is B5 of the Family B Graphite stack.
Review fixes land in the stack tip (#2690): state-specific toggle labels (
Add <Group> keyframe/Remove <Group> keyframe), and property-value sampling that follows the segment's own ease instead of interpolating linearly. The linear sampling reported a value the element never has mid-segment, and stamped that wrong value onto keyframes inserted from the header.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 4 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/05-pr-2687-deferred-review-findings.md:packages/studio/src/player/components/timelineCallbacks.ts:74— New optional keyframe-callback params widen propertyGroup to string, diverging from PropertyGroupName used on the sibling typepackages/studio/src/player/components/TimelineTrackHeader.tsx:482— hoveredGroup useState causes full lane recompute on every pointer enter/leave (flicker-lens)packages/studio/src/player/components/LayerDisclosureRow.tsx:47— Decorative ◇ glyph labels an unlabelablepackages/studio/src/player/components/TimelineTrackHeader.tsx:491— Keyframe-layer branch renders LABEL_COL_W children inside a parent whose width can be smallerSupersedes #2687, 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:
titleattributes, plus the layer disclosure row label.The 2 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:
TimelineTrackHeader.tsxowned value sampling, readout formatting, lane-state resolution and the JSX at once, so a formatting change and a layout change edited the same file. Sampling and formatting moved totrackHeaderLaneValues, lane-state resolution totrackHeaderLaneState, andresolveLaneHeaderStatereturns only the four fields its caller reads. The header also renders the per-track clip count it previously described but never showed.LegacyTrackHeaderread as deprecated code while being the live path for every track with no keyframe clip to disclose. It is nowPlainTrackHeader, with a comment saying which tracks take it.