feat(studio): expose composable timeline parts - #4229
Conversation
7170814 to
2cb8a88
Compare
0f69249 to
0aef406
Compare
0aef406 to
f72771d
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at f72771d5457364cbc2f237385fc0b7c95ae2d140. A careful decomposition — the parts genuinely read one shared context, and the existing suite came through untouched. Three things worth fixing before this becomes the public composition surface, none of them blocking.
Strengths
Timeline.test.ts(+22/-31) changes zero assertions. Both touched tests keep every expectation; the delta is arenderSharedAutomationTimelinehelper extracted from duplicated setup, and the optional-selection spread reproduces each caller's original state exactly. On a refactor that claims no behaviour change, an unmodified assertion set is the strongest preservation signal there is.- The multi-drag rationale moved with the code.
timelineProviderStateBuilders.ts:9-22carries the full CapCut/rigid-formation comment intoresolveMultiDragPreviewverbatim. That comment is the usual casualty of an extract-to-module refactor. TimelineRulerPart.tsx:8-20is prop-for-prop identical to the inline<TimelineRuler>it replaces inTimelineCanvas.tsx— same 11 props, samerowsVirtualized ? renderTimeRange : undefinedconditional. Nothing to drift.TimelineParts.test.tsxtests the actual claim: four parts mounted under a hand-built context with noTimelineand noFramein the tree, and the clip-menu assertion goes through the real component (role="menu").
Scope of what I ran
I could not run the studio suite here — the installed dependency set is missing @tanstack/react-virtual, which packages/studio declares, and stubbing it would have made both sides of a parity comparison render nothing. So the finding below was measured on a standalone React 19.2.6 + happy-dom rig instead of in vitest, and everything else is read from the files at this head.
important — the parity test's normalizer hides three classes of real divergence
TimelinePartsParity.test.tsx:50-52:
const normalizeMarkup = (markup: string) =>
markup.replaceAll(/timeline-lanes_[^"]+/g, "timeline-lanes");[^"]+ runs to the closing quote, so it erases the whole attribute value from timeline-lanes_ onward, not just the generated id. Measured against the id shape React 19.2.6 actually mints (timeline-lanes_r_0_-track-0), these divergences all normalize to equal and the test passes:
| divergence | normalized? |
|---|---|
different track index (-track-0 vs -track-7) |
hidden |
a lane dropped from a two-id aria-controls |
hidden |
a lost -automation suffix |
hidden |
| a difference in a different attribute (control) | caught |
The control arm matters: the comparison itself works, so this is a blind spot specific to attribute values containing a lanes id — the exact ids that encode which lane is which. Anchoring to the generated segment closes it and keeps the test passing:
markup.replaceAll(/timeline-lanes_r_\d+_/g, "timeline-lanes")Two things I checked that are fine, so they don't need changing: the two roots really do mint different ids (_r_0_ vs _r_1_, the counter is module-global, not per-root), so the normalization is load-bearing rather than dead; and replaceAll(":", "") at TimelineLanes.tsx:102 is a no-op on React 19 but harmless.
Worth stating plainly for the record, since this test is the evidence offered for "keeps working as before": both sides of it are the new code, so it cannot detect a regression against main, and it exercises 3 of the 13 exported parts (Frame, RazorGuide, Overlays). Ruler, Lanes, Playhead, EmptyState and the four menus are not in the comparison. The before/after screenshots are carrying that weight, not this test.
important — TimelineProvider returns two different tree shapes, so a host can remount the whole timeline
TimelineProvider.tsx:180-190 returns TimelineEditProvider > TimelineProviderState when no edit context is present and a bare TimelineProviderState when one is. Those are different element types at the same position, so if an ancestor TimelineEditProvider ever mounts or unmounts around a live Timeline, React unmounts and remounts TimelineProviderState — and every piece of timeline state it owns goes with it: scroll offset, zoom, selection, in-flight drag, open menus.
Inside this package nothing renders TimelineEditProvider (it is only re-exported from index.ts:171), so this cannot fire here today. It lands entirely on external hosts — which is exactly the audience Timeline.Provider is being made public for in this PR. One shape, same semantics:
const editContext = useTimelineEditContextValue();
return (
<TimelineEditProvider value={editContext ?? props}>
<TimelineProviderState {...props}>{children}</TimelineProviderState>
</TimelineEditProvider>
);important — lane-level resize/move quietly changed which handler runs
At base, TimelineCanvas read useTimelineEditContextOptional() itself and passed the raw context onResizeElement / onMoveElement down to TimelineLanes. At this head it passes props.onResizeElement / props.onMoveElement from the canvas state, which trace back to useResolvedTimelineEditCallbacks — the override-aware bag (useTimelineProviderState.tsx:76-79, :501-502).
So a host that passes onResizeElement to <Timeline> while an edit context also supplies one now gets its override honoured on lane edits, where before the context won. That is very likely a fix — useResolvedTimelineEditCallbacks.ts:17-19 says the override exists so NLELayout can rebase expanded sub-comp clips, and bypassing it on the lane path reads like the bug. But the PR body says "no intended visible layout change" and nothing covers this, so please either state it as an intentional fix or pin it with a test.
nits
timelineProviderStateBuilders.ts:23-24drops a guard the original had: the old code requireddraggedElementIdentityto be truthy before building a preview, the newresolveMultiDragPreviewbuilds one with whatevergetTimelineElementIdentityreturns. Only reachable with an empty-stringkey/id, so it is theoretical — but it is a parity delta inside the function whose job is parity.- Two exported things are now named
TimelineLanes: the context-backed part (TimelineParts.tsx:27) and the props-driven implementation (./TimelineLanes).TimelineCanvasimports the latter,index.tsexports the former. The compiler will catch a mixed-up import, but the names invite it. Timeline.Playheadrenders a barePlayheadIndicator(TimelineParts.tsx:43), while the composed canvas wraps the same indicator in a positioneddiv(TimelineCanvas.tsx:244-247). A host composing the part gets an unpositioned playhead. Fine if the host is meant to own that box — worth one line of docblock saying so.onRazorSplitAllnow has two resolution paths in one function: the resolved bag atuseTimelineProviderState.tsx:82andeditContext.onRazorSplitAllat:507. Same value today because it is not override-able; a trap the day it joinsTimelineEditOverrides.TimelineOverlays.tsxlost the docblock onresolveTimelineContextElement("The captured project session and current selection jointly own a context target") while the function itself stayed. That line is the invariant the resolver encodes.
Verdict: APPROVE
Reasoning: The decomposition is faithful where I could check it mechanically — identical ruler props, rationale carried with the moved code, and an existing assertion set that did not have to change. The three importants are a test-quality gap, a host-side hazard that cannot fire inside this package, and a behaviour change that looks like a fix; none of them is existing functionality silently broken.
— Rames Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 7a6b3a61. My approval at f72771d5 was un-pinned by the push, so this re-stamps the same work at the new head. The change since then is one comment, and I verified that rather than reading the patch.
What actually moved. merge_base(f72771d5, 7a6b3a61) == f72771d5 and the base is unchanged at c797f545, so f72771d5...7a6b3a61 is an honest diff — one commit whose parent is the previous head. Tree-level diff: a single file, timelineProviderStateBuilders.ts, +2/−10.
Proof it is comment-only, rather than an inference from the diff: I fetched the file at both heads, excised the comment block from each side (old 16,25d, new 16,17d) and compared the remainders — byte-identical, 3497 bytes each, 93 non-comment lines on both sides, and that block is the file's only comment. Every other blob in the tree is unchanged by hash, including the three files carrying my open findings (TimelinePartsParity.test.tsx, TimelineProvider.tsx, TimelineCanvas.tsx, useTimelineProviderState.tsx). So the PR-description claim that playback, editing and menu behavior are unchanged holds here in the strong form: nothing executable moved at all.
Credit where I expected a loss
A net −8 on a comment usually means deleted rationale, so I read the deleted text. It named MAGNETIC_TRACK_THRESHOLD, and that constant does not exist anywhere at this head. It was real once — timelineEditing.ts:15 (= 0.5) with a test pinning the value — and it was gone from the code before this PR opened, leaving the prose pointing at nothing. Trimming it removes a stale reference, and the substance the rest of that block carried (the translateX-not-re-layout choice, the preview-matches-commit pairing) survives in fuller and accurate form in timelineMultiDragPreview.ts:1-20, which is what the deleted line cross-referenced. This is a cleanup, not a rationale loss.
nit — the replacement sentence names the wrong axis
timelineProviderStateBuilders.ts:17 now reads "so the formation stays rigid at the lane boundary". The boundary that holds the formation rigid is time 0 on the start (x) axis — not a lane boundary. Three places at this head say so, and one of them is the call site:
timelineMultiDragPreview.ts:102— the floor isdelta ≥ -min(start), keeping every start≥ 0;:98adds that the timeline "has no fixed right wall".timelineMultiDragPreview.ts:14— the grabbed clip stops "the instant any member would cross 0".timelineClipDragPreview.ts:80-81— "…cross 0, exactly as it lands on commit. Lane changes still apply to the grabbed clip only, so only the start (x) is constrained."
The lane axis is specifically the one that does not constrain the group: track changes apply to the grabbed clip alone and passengers keep their lanes (timelineMultiDragPreview.ts:18). I also grepped for any row/track clamp on previewTrack and there is none. So the new wording inverts the axis on the exact invariant clampGroupMoveDelta exists to protect, in a file whose neighbours document it correctly — worth a word, since a later reader hunting the lane clamp will not find one.
Suggested: …follow the same clamped delta, so the formation stops together when its leftmost member reaches 0 instead of deforming.
Still open from my review at f72771d5 (unchanged — all three files are byte-identical)
TimelinePartsParity.test.tsx:50-51—replaceAll(/timeline-lanes_[^"]+/g, …)erases the whole attribute value, so a changed track index, a lane dropped from a two-idaria-controls, and a lost-automationsuffix are all invisible to the comparison; anchoring to/timeline-lanes_r_\d+_/gcloses it.TimelineProvider.tsx:180-190— the two tree shapes mean a host that togglesTimelineEditProvideraround a liveTimelineremounts all timeline state;value={editContext ?? props}unconditionally fixes it. Not reachable inside the package, only for the external hosts this PR opens the API to.- Lane-level resize/move now resolve through the override-aware callback bag rather than the raw edit context — probably a fix to the sub-composition path, but undeclared and untested.
Verdict: APPROVE
Reasoning: The delta since my last approval is provably one comment block with a byte-identical remainder, it removes a reference that had gone stale before this PR, and the one wrong clause in the replacement text is a comment-level nit. The three findings from my first pass stay open and none of them block.
— Rames Jusso
Summary
Timeline.Providerand the provider-backed timeline parts named by the composition plan.Timelineas the composed Studio variant.TimelineThemefrom the Studio package.Before
The fixture and zoom are identical to After. This refactor has no intended visible layout change.
After
The fixture and zoom are identical to Before. This refactor has no intended visible layout change.
Verification
The context value carries currentTime, so every composed part re-renders at playhead frequency during playback.
The next change to useTimelineProviderState.tsx must split the file before adding more code.
The multi-drag preview keeps the grabbed clip as a free ghost, uses a magnetic destination-lane affordance, and moves selected companions by the same clamped delta so the formation stays rigid at the lane boundary.