fix(studio): clips inside nested compositions sit at their master time - #4681
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Review at 075cd12b6199e342d0dde6bfe2db80870a36b268.
Verdict: REQUEST_CHANGES
The read side is right. The timeline now places nested rows where the runtime plays them, at depth two and three, and I checked that against the runtime's own clip manifest. The write side was not updated to match. When you drag or trim a nested row, the master time it now shows gets written into a local data-start, so the enclosing host's offset is counted twice. A 1 s nudge on the logo row in the example moves it 3 s. Before this PR the row was drawn in the wrong place, but a drag moved playback by exactly the drag distance. That round trip is what regresses here.
Findings
Blocking
1. Move and trim write master time into a composition-local attribute.
handleTimelineElementMove (useTimelineEditing.ts) writes updates.start straight to data-start, both on the live preview (patchIframeDomTiming) and in the file (buildTimelineMoveTimingPatch, or the SDK setTiming path). handleTimelineElementResize does the same with start on a head trim. updates.start is the dropped row position (commitDraggedClipMove: drag.previewStart). After this PR that value is master time. But data-start on a nested element is relative to its parent composition, as resolveStartForElement reads it. Nothing in the path subtracts the parent's master start.
The rows affected are reachable. buildMissingCompositionElements adds nested hosts such as logo as master rows, and they have a domId, so getTimelineEditCapabilities gives canMove: true.
Repro with the PR's own fixture (main 0 s > intro 2 s > logo 3 s > badge 1 s), run in jsdom against this head:
ROW BEFORE 5 DROPPED AT 6 ROW AFTER 8 PLAYS AT 8
PATCHED FILE <div data-composition-id="intro" data-duration="11"><div id="logo" ... data-start="6" ...>
So the clip jumps 2 s past where it was dropped. It also grows the intro's data-duration from 10 to 11, because setCompositionDurationToContent now sees 6 + 5 in the intro's file. With the two view files reverted to the merge base, the same steps round-trip: row 3, dropped at 4, redrawn at 4, and playback moved by 1 s.
Suggested fix: keep the enclosing composition's master start on nested rows. You can get it from the same resolver, as resolveStartForElement(parentComposition). Subtract it before any start write: move, head trim, and the SDK setTiming call. Then add a test that drags a depth-2 and a depth-3 row and asserts the redrawn row equals the drop point and the written attribute is local. The existing expandedParentStart / clipTimingStart pair is the same idea from the old expanded-row design. It may be reusable, but it also switches on other branches (razorSplitTransaction, timelineZMirror), so a dedicated field is probably safer.
Non-blocking
buildMissingCompositionEntrystill getstiming.durationfromreadClipTimingwith the Studio reference resolver, so a host sized bydata-end="<ref>"gets its length from local starts while its start is now master. For siblings under the same parent the two agree. Across parents they can differ. This is an edge case and not a regression. It is worth a follow-up so the start and the length come from one resolver.
Claims checked
- "Each level below the first dropped its parent's offset." Confirmed. At the merge base the fixture gives logo 3 and badge 1. The runtime manifest (
collectRuntimeTimelinePayload) gives logo 5 and badge 6. At this head the timeline gives 5 and 6, which matches the runtime exactly. - "Adding only the nearest parent's offset fails the depth-3 case." Confirmed by mutation. With the start replaced by local start plus the nearest host's local start, the depth-3 test fails on badge.
- "Two more tests ... fail without that setting." Confirmed. With
includeAuthoredTimingAttrs: falseat both call sites, both referenced-scene tests fail. - "Both places that build master rows use it." Confirmed.
parseTimelineFromDOMandbuildMissingCompositionEntryare the only row builders insrc/playerthat read a start from the DOM. Manifest rows already carry runtime starts. - The runtime and producer use the same resolver with authored timing attributes on (
init.tscreateTimingResolver(true),timeline.tsmanifest). So the new timeline value is the frame time the renderer uses. The resolver ignores a composition host's playback rate, and so does the runtime, so the two agree there too. - "Existing callers are unaffected" by the
Pick<RuntimeTimelineLike, "duration">widening. Confirmed. Core and Studio typecheck clean. - Drill-in: with a sub-composition opened on its own, the times stay local (logo 3, badge 4). They match the runtime manifest for that document.
Tests run
- Touched Studio tests at this head: 51/51 pass.
- Reverted the two Studio source files to the merge base: all 4 new tests fail as described, and the rest pass.
- Merged with current
main(303c8e92, which includes the nested-drill playhead restore and the scene swap): clean merge, and none of the touched files changed onmainsince the base. On the merged tree, the full Studio suite passes (551 files, 5955 tests). CorestartResolver,compositionReadinessandtimelinetests pass (132).tsc --noEmitis clean for Studio and core. oxlintandoxfmt --checkon the five changed files: clean.- The scratch round-trip test above was run from outside the tree and is not part of the PR.
Checks
All 10 required contexts for main pass at this head: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest, Studio and player captures, Test reachability. The other checks pass or are skipped by path filter.
Gate
reviewDecision: REVIEW_REQUIRED, mergeStateStatus: BLOCKED, mergeable: MERGEABLE. There are no reviews or comments yet. The branch rules need one approval. No auto-merge is set.
— Rames
|
Thanks, the round trip was the thing to check. Fixed at 24b9ba1:
Tests at depth 2 and 3 cover the drag round trip, head trim, group move and resize, the scale window, duplicate, keyframe basis and drag, the whole-film video and the clamp; each fails without its conversion. The before/after drag captures are in the PR body. The host-length point stays a follow-up: core's duration resolver does not read |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 24b9ba1bfa3eb004226ca07ae592911bd81dc2fe. My REQUEST_CHANGES at 075cd12b asked for move, head trim and the SDK setTiming call to stop writing master time into a composition-local data-start. That is fixed for moves, the SDK call, group moves, in-range head trims, the razor split and duplicate. One head-trim case is still wrong, and it is new in this revision.
Verdict: REQUEST_CHANGES
Findings
Blocking
1. A head trim past the host's start moves the clip's tail.
toAuthoredStart now clamps to the host start (24b9ba1). That is right for a drop, where persistMoveEdits clamps first so the store, the preview and the file agree. A head trim goes a different way. computeResizePreview still uses minStart: 0 in master time, and commitResizePointerUp puts the unclamped previewStart into the store. handleTimelineElementResize then clamps the start inside toAuthoredStart but writes updates.duration unchanged. So the start stops at the host, the length keeps the overshoot, and the end moves right by the overshoot.
Repro with the PR's own harness (main 0 > intro 2 > logo 3 > badge 1), trimming the logo's head from 5 s to 1 s with the end held at 10 s (start: 1, duration: 9):
written: data-start="0" data-duration="9"
store row until reload: 1 to 10
row after reparse: 2 to 11, plays 2 to 11
So the row is drawn at 1 to 10, then jumps to 2 to 11 on reload, and the clip now ends 1 s later than where the trim left it. The group resize path does the same (data-start="0" data-duration="9"). The GSAP scale window gets newStart: -1 from toCompositionTime, which is not clamped, while the attribute says 0, so the tweens and the clip also disagree.
The rows that can reach this are the ones this PR is about. Nested hosts from buildMissingCompositionElements have no kind, so canSeedPlaybackStart is false and nothing stops the head at 0 before the host.
Suggested fix: give the trim the same floor the drop has. Either pass the row's host start as minStart in computeResizePreview and the group resize preview, or, when the commit clamps the start, shorten the duration by the same amount so the end stays put. Then add a trim test that goes past the host start and checks the row, the written start and length, and the scale window.
Non-blocking
- The duplicate lane check converts every row in the target file to local time (
useClipboard.ts, the.map((el) => ({ ...el, start: toAuthoredStart(el, el.start) }))). No test covers it. With that line removed, all tests still pass. A test with a clip already in the lane after the original would pin it. - In the real manifest path, a depth-3 clip that is not a host (the badge) is not a row at all: the manifest filter drops it because its parent
logois a manifest composition, and the missing-host pass only adds hosts. The depth-3 badge tests run through theparseTimelineFromDOMfallback. I checked a depth-3 host (starinsidelogo) through the real manifest path: it sits at 6.5, has host start 5, and a +1 s drag writes 2.5 and plays at 7.5. So the fix holds on the path the app uses. It may still be worth one test on that path. - My earlier note on
buildMissingCompositionEntry(length fromreadClipTiming, start from the core resolver) is deferred with a reason in the PR conversation. That is fine as a follow-up.
Prior blocker, checked end to end
- Depth 2 drag: logo 5 to 6 writes
data-start="4"inintro.html, the live node gets 4, the row redraws at 6, it plays at 6, andintro'sdata-durationstays 10. - Depth 3 drag: badge 6 to 7 writes 2 in
logo.html, redraws and plays at 7, andlogostays 5. - Real app path (runtime manifest, the manifest filter, then
buildMissingCompositionElements), run in jsdom: intro 2 (host start 0), logo 5 (host start 2), star 6.5 (host start 5). A +1 s drop on each writes 3, 4 and 2.5, and each plays at the drop point. I also tried a host whose id differs from its inner root, and a host with no composition id. In both, the inner clip is filtered out of the root rows, so no row is left with a master start and no host start. - Head trim inside the host: logo 5 to 5.5 writes 3.5 and 4.5, and the scale window is 3/5 to 3.5/4.5 in local time. This part is fixed. Only the overshoot case above is not.
- SDK
setTiming: the test hands the SDK 2 for a clip at master 4 inside a host at 2. - Drill-in: with a sub-composition opened on its own, host start is 0, so writes stay as before.
Other write sites
- Razor split:
buildCutTargetgoes throughtoAuthoredStartfor both the split time and the element start. A legacy whole-film video keeps master time on both, which is consistent. - Duplicate: the anchor is converted into the target file's clock. Covered by a test that fails without it.
- Keyframes:
resolveClipTimingBasisand the lanes useparentCompositionStart. For hosts from the missing pass, the basis is 5 - 2 = 3, the same as before this PR. For top-level manifest rows the field is unset and the old parent-row branch still applies. - Gap close goes through
persistMoveEdits, so it gets the drop clamp. - Snapping and lane placement stay in master time, which is what the rows use.
- Playback-rate hosts: the resolver and the runtime both ignore a host's rate, so the read and the write agree.
timelineZMirrornow keys onexpandedHostKey. Neither field is set on current rows, so this is no change.- Merge
ead8e814:git show --remerge-diffis empty, so there were no conflict edits. It also merges cleanly with currentmain(001d001c), where the only touched file that changed is a test file.
Mutation tests
I reverted each fix hunk one at a time and ran the touched tests:
| Reverted | Result |
|---|---|
move authoredStart (live, file, SDK) |
4 fail |
| SDK move only | 1 fails |
resize authoredStart |
1 fails |
| resize file patch helper | 2 fail |
| resize scale window | 2 fail |
| group move file patch | 1 fails |
| group move live attribute | 1 fails |
| group resize scale window | 2 fail |
| razor split | 3 fail |
drop clamp in persistMoveEdits |
1 fails |
clamp in toAuthoredStart |
1 fails |
root-time video flag in parseTimelineFromDOM |
2 fail |
| host start on DOM rows | 13 fail |
| host start on missing-host rows | 1 fails |
| duplicate anchor | 1 fails |
| duplicate lane check | 0 fail |
| keyframe basis branch | 4 fail |
toCompositionTime ignoring host start |
10 fail |
core isRootGlobalMediaStart always false (core rebuilt) |
2 fail |
Studio resolves @hyperframes/core through dist in vitest, so the core mutation needed a core rebuild. It was rebuilt again after the revert.
Claims checked
- "A drop before the host's start lands at the host's start: the row, the live preview, the file and playback all agree." True for a drop. Not true for a head trim, see the blocker.
- "One conversion subtracts it before any start is written: a move (the live preview, the file and the SDK call), a head trim ..., a group move or resize, and a razor split." Confirmed for each call site in source and by mutation. The head trim uses the conversion but not the matching length change.
- "A video or audio clip whose start already counts from the whole film keeps it as written." Confirmed:
voat 7 dragged to 8 writes 8 and plays at 8, and its scale window uses the host clock (5/2 to 5.5/1.5). - "Every row now carries the master start of the composition its
data-startcounts from." In effect, yes. Manifest rows leave the field unset, and that reads as 0. They are all top-level after the filter, so 0 is right.
Tests run
- Touched Studio tests (13 files): 372/372 pass. Core
startResolver,mediaTimingandtimelinetests: 109/109 pass. - Full Studio suite at this head: 554 files pass, 1 skipped; 5987 tests pass, 16 todo.
tsc --noEmitis clean for Studio and core.oxlintandoxfmt --checkon the 26 changed files: clean.- The trim repro and the real-path round trip above ran from temporary files that are not part of the PR.
Checks
73 checks at this head: 71 pass and 2 are skipped by path filter. All 10 required contexts pass: Build, Render on windows-latest, Semantic PR title, Studio and player captures, Test, Test reachability, Test: runtime contract, Tests on windows-latest, Typecheck, regression.
Gate
reviewDecision: CHANGES_REQUESTED (my review at 075cd12b), mergeStateStatus: BLOCKED, mergeable: MERGEABLE. No auto-merge is set.
— Rames
|
Fixed at eb4ab7f:
|
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at eb4ab7faf8650558166fa1aeb15fe3270c4edc4a. My REQUEST_CHANGES at 24b9ba1b had one blocker: a head trim past the host's start kept the overshoot in the length, so the clip's end moved. Two commits since then (6e25218e, eb4ab7fa) give the trim and the drop the same floor in the preview. The blocker is fixed on every path the app uses, and I found no new blocker.
Verdict: APPROVE
Findings
Blocking
None.
Non-blocking
1. A host authored before its enclosing host's start is kept there on screen, but every write moves it to the host's start.
clampToHostStart now floors at min(hostStart, element.start), so a row that already sits before its host is not pulled forward when grabbed. The write side still can't express that start. toAuthoredStart returns max(0, masterTime - hostStart), and the runtime clamps a literal data-start at 0 anyway. So the store keeps the early start and the file gets 0.
Such a row can only come from a relative start. Repro in the PR's harness, with star set to data-start="badge - 4" (plays at 4, host logo at 5):
lane-only move: file data-start="0", store row 4, after reparse 5, plays 5
move 4 -> 4.5: file data-start="0", store row 4.5, after reparse 5, plays 5
tail trim to 1.5: file data-start="0", after reparse 5, plays 5
At 24b9ba1b the drag floor was the host start, so the 4 to 4.5 move was drawn at 5 and landed at 5. Now it is drawn at 4.5 and lands at 5. The lane-only and tail-trim cases wrote 0 at 24b9ba1b too. This is rare (a relative start with a negative offset that lands before its host), so it doesn't block. Either floor at the host start again, since that is where any literal write will land, or leave data-start alone when the start didn't change. The new test "leaves a clip authored before its host's start in place when grabbed" checks the store start but not the written attribute, so it doesn't catch this.
2. A drop onto an empty main track shows the ghost at 0 and lands at the host's start.
resolveDragLandingStart runs after the new floor, and resolveMainTrackDropStart returns 0 when the clip lands on track 0 and no other clip is on that track. For logo (host at 2) dragged from lane 1 onto an empty lane 0, computeDragPreview gives previewStart: 0, and persistMoveEdits then clamps the store and the file to 2. The row, the file and playback agree, so only the ghost is wrong while dragging. It's a small edge case, but it goes against the commit's "stops at its host's start before snapping". Clamping the result of resolveDragLandingStart to the same floor would close it.
3. Three new hunks have no test. Reverting each one alone leaves every test passing:
- the snap bounds floor in
computeResizePreview(const floor = clampToHostStart(resize.element, 0)). The code is right: with it reverted, a snap target just below the host's start is accepted and the trim overshoots again, which is the old blocker by another route. A trim test with a snap target below the floor would pin it. clampTimelineGroupMoveDeltaintimelineGroupEditing.tsusingminStart.- the
Math.max(0, ...)intoAuthoredStart.
4. The resize handler still doesn't hold the end on its own. handleTimelineElementResize called with start: 1, duration: 9 on logo still writes data-start="0" data-duration="9" and a scale window with newStart: -1. No caller does that now. The only caller is the pointer gesture, and its preview is floored. If you want the handler safe on its own, shorten the duration by the amount it clamps.
Prior blocker, checked end to end
This is the harness from my last review (main 0 > intro 2 > logo 3 > badge 1), trimming the logo's head from 5 s toward 1 s with the end held at 10. The drag goes through computeResizePreview and then the handler, as a pointer-up does.
- Manifest path (the row from
buildMissingCompositionElements, nokind, the case from the blocker): preview2 / 8. Writtendata-start="0" data-duration="8". Store row 2 to 10, row after reparse 2 to 10, plays at 2. GSAP scale window 3/5 to 0/8. The end stays at 10. - Group resize, same row plus a top-level member at 12: the rigid delta clamps to -3, so logo is
2 / 8and the other member is9 / 8. Written0 / 8, scale window 3/5 to 0/8. - DOM-parsed row (
kind: "composition"): the head can't extend past the in-point at all, same as onmain, so nothing overshoots. - Depth 3,
badge(host at 5, start 6, end 8): a -1 or -2 trim stops at5 / 3. Written0 / 3, plays at 5, scale window 1/2 to 0/3.
New code, other cases
- Trim inside the host: logo 5 to 5.5 writes 3.5 and 4.5, plays at 5.5, scale window 3/5 to 3.5/4.5.
- Tail trims: logo end 10 to 11 writes 3 and 6. End 10 to 8 writes 3 and 3. The start is unchanged in both.
- Drop: logo -4 stops at 2, badge -3 stops at 5, and the depth-3 host
star(manifest path) -3 stops at 5. The preview, the store and the file agree for each. - Group drop, logo and badge, grabbing logo at -4: badge has 1 s of room, so the delta clamps to -1 (logo 4, badge 5).
- Root-time video
vo: its floor is 0 (its own clock), so nothing changed there. A head trim can't extend past its in-point. - Drill-in with host start 0: the floor is
max(min(0, start), 0) = 0, the same asmain. - The duplicate lane check from my last review is now covered: with the conversion removed, the new clipboard test fails.
Mutation tests
I reverted each new hunk and ran the 17 related test files (the 14 touched test files, plus timelineGroupEditing, timelineEditing and useRazorSplit.test.tsx):
| Reverted | Result |
|---|---|
trim preview minStart floor |
1 fails |
| trim snap bounds floor | 0 fail |
group resize member minStart |
1 fails |
group resize delta bounds use minStart |
1 fails |
clampTimelineGroupMoveDelta uses minStart |
0 fail |
drag minStart into resolveTimelineMove |
1 fails |
| a lone clip goes through the group clamp | 1 fails |
| group drag floors ignored | 2 fail |
| snap indicator kept after the clamp | 1 fails |
clampToHostStart own-start min |
1 fails |
toAuthoredStart Math.max(0, ...) |
0 fail |
clampGroupMoveDelta uses minStart |
2 fail |
resolveTimelineMove uses minStart |
1 fails |
all trim hunks together (6e25218e) |
2 fail |
all drop hunks together (eb4ab7fa) |
4 fail |
| everything | 6 fail |
| duplicate lane check (from last round) | 1 fails |
Claims checked
- "Your round-2 head-trim blocker and the drop floor fixed (4 tests fail without)." The blocker is fixed, see above. Reverting the drop-floor hunks fails 4 tests, as claimed. Reverting the trim hunks fails the 2 new trim tests.
- "A dropped nested clip stops at its host's start before snapping." True for a plain drag and a snap target below the floor. Not true in the ghost for a drop onto an empty main track (non-blocking 2).
Tests run
- The 17 related Studio test files: 479/479 pass.
- Full Studio suite: 554 files pass, 1 skipped. 5995 tests pass, 16 todo.
tsc --noEmitis clean for Studio and core.oxlintandoxfmt --checkon the 31 changed files: clean.- The repros above ran from temporary test files. Those files and all mutations have been removed, and the tree is clean.
Checks
73 checks at this head: 71 pass and 2 are skipped by path filter. All 10 required checks pass: Build, Render on windows-latest, Semantic PR title, Studio and player captures, Test, Test reachability, Test: runtime contract, Tests on windows-latest, Typecheck, regression.
No main merges since 24b9ba1b, so there is nothing for --remerge-diff to show. The branch still merges cleanly with current main (5606b310).
Gate
reviewDecision: CHANGES_REQUESTED (my reviews at 075cd12b and 24b9ba1b), mergeStateStatus: BLOCKED, mergeable: MERGEABLE. No auto-merge is set. This approval replaces those reviews.
— Rames
What
On the master timeline, a clip inside a nested sub-composition was drawn at its start time inside its parent instead of the time it plays. With an intro at 2 s on the master and a logo at 3 s inside the intro, the logo was drawn at 3 to 8 s but plays at 5 to 10 s. Each level below the first dropped its parent's offset.
Studio built those rows from the clip's own start, which is relative to its parent. It now asks the same start-time rule the runtime uses to play the composition, which adds every parent's start at every depth. Both places that build master rows use it: the pass that adds nested composition clips, and the fallback that reads the timeline from the document. It also reads a scene's authored length the way the runtime does, so a scene that starts after another one (
data-start="intro + 2") lands where it plays.@hyperframes/core's start-time resolver now accepts any timeline registry whose entries have a duration, which is all it reads, so Studio's own timeline type fits, and it now also returns an element's host start (resolveHostStartForElement); the rule for whole-film media starts is shared asisRootGlobalMediaStart. Existing callers are unaffected.Writes go the other way. Every row now carries the master start of the composition its
data-startcounts from, and one conversion subtracts it before any start is written: a move (the live preview, the file and the SDK call), a head trim (including the GSAP scale window), a group move or resize, and a razor split. So a nested row dragged to 6 s on the master timeline is written as its local start and plays at 6 s; before this change it would have been written as 6 inside the intro and played at 8. A video or audio clip whose start already counts from the whole film keeps it as written.Verification
data-startis local, it plays there, and the parent's length does not grow. Top-level rows, tail trims, group moves, the SDK path, the razor split and a whole-film-time video are covered too. They fail without the conversion.Before
before-nested.mp4
before-drag.mp4
After
after-nested.mp4
after-drag.mp4