Skip to content

feat(studio): expose composable timeline parts - #4229

Merged
miguel-heygen merged 5 commits into
mainfrom
hfoss25/timeline-parts
Sep 21, 2026
Merged

miguel-heygen merged 5 commits into
mainfrom
hfoss25/timeline-parts

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Expose Timeline.Provider and the provider-backed timeline parts named by the composition plan.
  • Keep Timeline as the composed Studio variant.
  • Split overlay rendering into selectable context-backed parts.
  • Export TimelineTheme from the Studio package.

Before

The fixture and zoom are identical to After. This refactor has no intended visible layout change.

Before

After

The fixture and zoom are identical to Before. This refactor has no intended visible layout change.

After

Verification

  • Studio typecheck on devbox, green.
  • Focused timeline suite on devbox, 93 tests passed.
  • oxlint, green.

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.

@miguel-heygen
miguel-heygen force-pushed the hfoss25/timeline-parts branch 3 times, most recently from 7170814 to 2cb8a88 Compare September 20, 2026 22:24
@miguel-heygen miguel-heygen changed the title feat(studio): expose composable timeline parts feat(studio): expose composable timeline parts [CI] Sep 20, 2026
@miguel-heygen miguel-heygen changed the title feat(studio): expose composable timeline parts [CI] feat(studio): expose composable timeline parts Sep 20, 2026
@miguel-heygen miguel-heygen reopened this Sep 20, 2026
@miguel-heygen
miguel-heygen force-pushed the hfoss25/timeline-parts branch 3 times, most recently from 0f69249 to 0aef406 Compare September 21, 2026 02:28

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a renderSharedAutomationTimeline helper 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-22 carries the full CapCut/rigid-formation comment into resolveMultiDragPreview verbatim. That comment is the usual casualty of an extract-to-module refactor.
  • TimelineRulerPart.tsx:8-20 is prop-for-prop identical to the inline <TimelineRuler> it replaces in TimelineCanvas.tsx — same 11 props, same rowsVirtualized ? renderTimeRange : undefined conditional. Nothing to drift.
  • TimelineParts.test.tsx tests the actual claim: four parts mounted under a hand-built context with no Timeline and no Frame in 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-24 drops a guard the original had: the old code required draggedElementIdentity to be truthy before building a preview, the new resolveMultiDragPreview builds one with whatever getTimelineElementIdentity returns. Only reachable with an empty-string key/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). TimelineCanvas imports the latter, index.ts exports the former. The compiler will catch a mixed-up import, but the names invite it.
  • Timeline.Playhead renders a bare PlayheadIndicator (TimelineParts.tsx:43), while the composed canvas wraps the same indicator in a positioned div (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.
  • onRazorSplitAll now has two resolution paths in one function: the resolved bag at useTimelineProviderState.tsx:82 and editContext.onRazorSplitAll at :507. Same value today because it is not override-able; a trap the day it joins TimelineEditOverrides.
  • TimelineOverlays.tsx lost the docblock on resolveTimelineContextElement ("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 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 is delta ≥ -min(start), keeping every start ≥ 0; :98 adds 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-51replaceAll(/timeline-lanes_[^"]+/g, …) erases the whole attribute value, so a changed track index, a lane dropped from a two-id aria-controls, and a lost -automation suffix are all invisible to the comparison; anchoring to /timeline-lanes_r_\d+_/g closes it.
  • TimelineProvider.tsx:180-190 — the two tree shapes mean a host that toggles TimelineEditProvider around a live Timeline remounts 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

@miguel-heygen
miguel-heygen merged commit 0641921 into main Sep 21, 2026
52 checks passed
@miguel-heygen
miguel-heygen deleted the hfoss25/timeline-parts branch September 21, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants