Repository navigation
perf(studio): drag and resize any number of selected clips, saved in one write per file - #4937
Conversation
Edit accuracy: accurate 2040 (base branch 2040), smooth 1600 of thoseThe gate passes. Quarantined, measured but not gated (0) |
f3acf77 to
0b2b5d9
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes for one undo regression. The batching itself checks out, and the server and writer side are byte-equivalent to main.
Blocking: Cmd+Z during a save can paint the move back over a file the server has reverted
useEditHistoryActions.ts:101-105 now confirms a painted-back pending edit only when the claim count grew after the key press (claimedSinceKey). That is wrong when the edit's history claim landed before the key but the edit is still saving.
A group move has that window:
persistServerBatchclaims history first.- The pending edit then stays open through the GSAP batch request and the preview sync (
useTimelineGroupEditing.ts~296-350).
Cmd+Z in that window goes like this:
claimedAfteris a count that already includes the move.claimedAfter(count)finds nothing newer, so the server steps back. The top entry is the move, so the move is undone.claimedSinceKeyis false, soserverSteppedShownis false andshowAgain()re-applies the move in the store.
The file is reverted, but the timeline shows the clips moved. A clip with no tweens makes the window the whole GSAP request, because its batch returns mutated: false and makes no second claim.
Repro: a unit test in useEditHistoryActions.test.tsx using the file's own mount:
beginStudioPendingEdit(() => reapply), withclaimsfixed at 8 before and after.- The server answers
{ ok: true, label: "Undid: Move", undoes: "e8" }. - Undo, then
saving.settle(saving.adopt(() => Promise.resolve())).
reapply is called once at this head. With origin/main's useEditHistoryActions.ts swapped in, the same test passes.
The hook gets the same inputs as your row "its claim never counted, the move is shown again" (7 before, 7 after), so it can't tell the two apart. The fix needs a reference point from the edit itself:
- the claim count when the pending edit began, or
- the claim id the edit recorded, compared with
result.undoes.
I haven't verified end to end whether a later preview resync repaints the timeline. The soft restore path only resyncs automation lanes, so I wouldn't count on it. This path is shared, so canvas moves and nudges that claim before they settle can hit the same case.
Server and writer: equivalent to main
- I fuzzed
retimeClipTweensInScriptagainstorigin/main'sshiftPositionsInScript/scalePositionsInScriptapplied in turn, over 9000 random scripts and 3 seeds:- Scripts used to/from/fromTo/set, keyframed tweens,
hf-holdsets, label and relative positions, missing durations, descendant and class selectors, and duplicate ids. - Each script got 1-6 mixed shift/scale retimes, including 0-duration no-ops.
- Result: 0 byte differences, both without and with
syncPositionHoldsBeforeKeyframes. About 1400 cases per seed changed the file.
- Scripts used to/from/fromTo/set, keyframed tweens,
- The fuzz also catches mutants the PR's tests miss: scale positions not rounded (225 mismatches) and the id index returning only the first match (220).
- The client groups mutations per file in
finishGroupTimingGsapFallback, one owned step per file. Files are independent, so the order across files doesn't matter.
Test gaps (not blocking)
-
The batch-route test can't fail on the batch logic. It compares a batch with single requests, but both go through
retimeRun. Each of these leftfiles.test.tsgreen (99/99):- removing the hold sync in
retimeRun - letting
retimeRunsmerge across a non-retime mutation (that one skips the mutation in between) - not crediting the sync to the run's last slot
- treating a same-values scale as live
Expected bytes, or an assertion on
#k's hold set, would give it an oracle of its own.useGsapScriptCommitssends mixed batches to this route, so the run splitting matters. - removing the hold sync in
-
The group duration sync isn't wired into any test. Removing
finishFile: syncCompositionDurationToContent(useTimelineGroupEditing.ts:179) leaves 2240 studio tests green, includinguseTimelineGroupEditing.test.tsxandsrc/player. A group move past the end would stop growing the rootdata-durationon the server path with nothing red. -
The one-store-update claim is only tested on the helper. Removing
batchElementUpdatesaround the rollback, the group resize and the drop'sapplyEditseach survives. -
Selection guards: the preflight key's
group,selectorIndexandsourceFileguards each survive removal. -
Drag offset: not passing
passengerStyletoTimelineCompactDiamondsorTimelinePropertyLanessurvives. Diamonds and lanes would stop following a dragged clip.
Keyframe cache, noted but not a regression: ownership is checked by object identity, and keepEqualEntries can split identity per key. The sequence:
- An optimistic keyframe write updates only
comp.html#box. - An unchanged re-read of
comp.htmlkeeps that key's old object but givesindex.html#box/boxnew ones. - The next
index.htmlre-read drops those two again.
Main drops them too.
Reuse:
- Moving the shift/scale bodies into
retimeClipTweensInScript, with the old functions as one-line wrappers, is the right reuse. clipRetimeOfis now the single owner of the no-op rules, so they no longer live in two places.- I found no existing deep-equal helper in studio or core, so
sameDatais justified.
Simplify:
persistTimelineBatchEdit's new optionalfinishFilehas one caller, which always passessyncCompositionDurationToContent. Syncing insidepersistTimelineBatchEditwould remove the option, and also the untested wiring above.- A store
updateElements(map)action would be simpler than the module-levelbatchElementUpdatesinterception. The current shape keeps the injectedupdateElementsignature the revision tracking relies on, so this one is a judgement call.
Tests run:
- parsers 1120,
files.test.ts99, timing sync / editing helpers / timeline editing 110. - The 13 changed client test files (352 tests) and the full studio unit suite (7217).
- Mutations on the server, writer and duration sync: 4 of 13 turned the PR's tests red. Of the rest, 2 are caught only by my fuzz, 2 make no observable difference, and the others are the gaps above.
- Mutations on the client: most turned tests red. The survivors are the gaps above, plus one that only costs performance (
withTweenIndex).
CI at this head: 76 passed, 5 skipped, nothing failing.
— Rames
…tead of remounting it
…Z right after it need not wait
…eck on a new cache version
…ays within budget
terencecho
left a comment
There was a problem hiding this comment.
Requesting changes on 2905eca5 for one undo regression that I reproduced myself; Rames Jusso's changes-requested review at this head names the same spot. The server side holds: the batch route writes the same bytes as main. Everything else I checked is non-blocking.
It touches 55 files (parsers, studio-server, studio). Not draft, not stacked: 51 commits on main, same 55 files in main...2905eca5. It is 11 behind main; GitHub reports it mergeable.
Blocking: Cmd+Z after the move's history claim has landed re-paints the move over the reverted file (useEditHistoryActions.ts:101-105)
claimedAfteris read fromeditHistory.claims()at the key press. If the group move's own claim already counted,claims()is 8 andclaimedAfteris 8. The server then steps the move back ("Undid: Move"), andclaimedSinceKeyisclaims() > claimedAfter, which is false. SoserverSteppedShownis false andputBack()(applyEdits(true)for a timeline move) runs after the server has reverted the file.- Reproduced with the PR's own hook and
beginStudioPendingEdit: pending edit saving,claimsfixed at 8, the edit adopted and settled, the server answering{ok:true, label:"Undid: Move", undoes:"e2"}. The server undo is called once withclaimedAfter: 8andreapplyfires once. The PR's two rows ("its claim counted" at 8 after the key, "never counted") both change the count after the key; neither is "already counted at the key". main's line isstepped && Boolean(pendingEditShown), so it does not re-paint here (read from the diff; I did not runmain's file against this test).- When it can happen: the move's claim lands before the pending edit settles, i.e. while the rest of the save (the GSAP batch request, the preview sync) is still in flight.
- What I did not observe: the end state in a browser.
syncHistoryPreviewAfterApplyruns after the re-paint and patches the reverted attributes back, so the preview may correct itself. The timeline store takesapplyEdits(true)and I could not confirm it re-reads. My browser run held writes 1.5 s and pressed Cmd+Z 300 ms after the drop, which is before the claim, so it did not enter this window. - Fix: give the hook a reference point from the edit itself, not from the key press: the claim count when the edit began, or the edit's claim id compared with
result.undoes. A test withclaimsfixed at 8 andclaimedAfter8 pins it.
What I verified (head tarball, deps built, NODE_ENV=test; two lanes plus my own check of the undo path)
- Server and parsers, 3 runs each:
gsapWriter.parity151/151,files.test.ts99/99, keyframe cache 25/25. Studio, the 15 touched test files: 402/402, 3 runs.tsc --noEmit(parsers, studio-server, studio),oxlintandoxfmt --checkclean. - The batch route writes
main's bytes. 3000 random route scenarios (duplicate ids, nested and class targets, keyframed tweens needing holds, mixed with other mutation types): file bytes, status, error andchangedidentical tomainin 3000/3000. 4000 parser-level scenarios and 31 edge requests (bad bodies,../, absolute path, sub-composition,<template>) identical. A 98-clip batch: 2.76 s on head against 229 s onmain, same output. 30 retimes make one hold sync (main: 30). mutationChangesfor retime slots differs in 645 of 3000 scenarios, by design (the hold re-sync is credited to the run's last slot; a formatting-only rewrite reports false). The only reader ignores retime mutations.- Cap removal: the only refusal removed is the count check (3 call sites); locked, linked and audio-group handling is untouched and already ran for 1 to 3 clips. Nothing references
MAX_HAND_EDIT_CLIPSany more. - Real browser run (built CLI, Chrome, 12-clip fixture with GSAP tweens): select-all move and select-all edge resize move every clip and tween by the same amount, root
data-durationstays in sync, and one Ctrl+Z restores everything. A 3-clip move and a 3-clip edge resize save a byte-identicalindex.htmlonmainand head; head sends 1 batch request wheremainsent 3. - Passenger styling through the real
TimelineLanes: during a drag the clip and property lane get the offset, opacity 0.85 andpointer-events: none; after the drag all of it is gone, and the clip's DOM node is the same. - 93 mutants over the new code: 45 caught, 48 survive. The survivors that matter are listed below.
Non-blocking
- Test gaps (survivors that change bytes or behaviour): (a) no test has a retime, a non-retime, then a retime in one batch; dropping the hold re-sync (135 of 400 scenarios differ), not ending a run at a non-retime mutation (49 of 400) and an off-by-one on the run index (56 of 400) all pass, because the batch test compares against single requests that run the same fold. (b) Dropping the group
finishFileroot-duration sync stays green. (c) Passengerpointer-eventsand opacity are unpinned (a mutant that lets a passenger take pointer events passes), as are the diamond and lane offsets. (d) The group-membership andpreserveGroupclauses of the selection refresh guard, and the zero-delta, unchanged-timing and non-positive-duration skips in the group builders. - A second Cmd+Z while two saves are in flight paints nothing for the older edit (probe confirmed: undo 1 returns a revert, undo 2 returns none). The fallback to an older predicted step is read from the code, not run.
- Slow server: Cmd+Z reverts at once, then the clips snap forward when the timing save and the tween save land more than the coalesce window apart (only the tween entry is undone). The final file is the same on
mainand head; it needs a slow server to see. - A group edit whose members all have non-finite timing still rewrites root
data-duration(finishFileruns once per file regardless). The drag math does not produce NaN. - One unexplained run: 1 of 5 head runs of a 3-clip resize left root
data-durationat 6.9 after the first Ctrl+Z (a second Ctrl+Z fixed it). It did not reproduce in 3 reruns andmainwas 4 of 4. I am not calling it a regression. - Dead branch:
shift-positions/scale-positionsinexecuteGsapMutationAcornis unreachable now that the fold handles them. Hold sync is still quadratic in the hold count (the remaining 2.76 s). Passenger diamonds sit at 0.85 over a 0.85 clip, as the body says.
Not exercised: a real 98-clip project (my fixture is 12 clips), the PR's timings, the Desktop host, Windows, a multi-file group edit in a browser, linked or locked clips in a selection, keyboard pickup of a large selection, Escape of a group drag, the passenger opacity visuals, the 409 race on the route.
CI. All checks at this head finished: 76 passed, 5 skipped, 0 failing, 0 pending. CI is a reference; the verdict rests on the reproduction above.
Reviewed on the PR head 2905eca5; Rames Jusso (jrusso1020) has a changes-requested review at this head. This is a review verdict, not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
2905eca to
c8e8305
Compare
… dbulk/smooth-multi-drag # Conflicts: # packages/studio/src/hooks/gsapRuntimeKeyframes.ts
01200de to
abc4e2f
Compare
…he edit that ends it
abc4e2f to
f5f20d0
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at f5f20d01. This clears my changes-requested review at 2905eca5.
History check: the branch was rebuilt. 0d19b3ec2 is my reviewed 2905eca5 rebased: the same +/- lines against its base. The merge of main (49066a50e) differs from a clean auto-merge only in the gsapRuntimeKeyframes.ts import conflict, where it keeps both imports. I reviewed the 7 commits after that.
My blocker is fixed (c8e8305a): a pending edit now records the history's claim count when it begins (claimsAtBegin). Undo checks the paint-back against that edit's own count rather than the count at the key press, and passes it to the server as claimedAfter. My repro is now a test row: claim before the key, counts 7 / 8 / 8, and the move is not painted back. Two mutations each turn it red:
- going back to the key-time count
- a
claimsAtBeginof 0
The new undo work holds up:
- One step per gesture (
1f6c4583,997263e4):timingGestureStepgives each move or resize a key of its own, held until the next edit. The timing write and its GSAP fold are then one step however slow the fold is. Before, the window was 10 s per clip id.Infinitygoes over the wire asidleMs: null, which main already treats as "hold until another key" (#4513). The z-mirror and ripple callers keep their own unique keys and get the same window from one place. - Page scope (
4fe911c0): thepageKeyScopeprefix stops a reload or a second tab from joining another page's held step. - Delete and ripple (
018871be): they share one held step. - Server (
f5f20d01):holdClaimupdateslastWriteAt, so a held step is dated by its last write. - Refactor (
a7f142bd): the single-clip move and resize move intouseTimelineClipTimingEditing. Apart from trimmed comments and the hook's parameter plumbing, every line moved unchanged.
Mutations: 10 of 11 turn tests red:
- the claim-count reference
- the clock wiring
- either window back to 10 s
- a gesture key that isn't unique
- no page scope
- the delete record on the default window
- the server's re-dating
The survivor: if timingGestureStep ignores the caller's coalesceKey, nothing fails. The z-mirror and ripple tests mock the group move and only check the key they pass, so nothing pins that the group hook uses it. A row in useTimelineGroupEditing.test.tsx that passes a key and checks the claim would cover it. Not blocking.
From my last review:
- Folding the duration sync into
persistTimelineBatchEditis in, and it's pinned now: dropping it turns a test red. - The batch-route test still can't fail on the run logic. Dropping the hold sync in
retimeRunleavesfiles.test.ts99/99. Still not blocking.
Reuse: timingGestureStep replaces three hand-rolled keys and windows (deleteGestureSeq, the per-clip keys, the 10 s fold constant), and the explicit coalesceMs plumbing goes from five signatures. That's the right direction.
Simplify: I don't see a smaller version.
Tests:
- Studio unit suite: 7267 pass. One failure,
useClipboard.source.test.ts, passes 3 of 3 on its own and the PR doesn't touch it. - studio-server history, routes and files: 269 pass.
CI and merge state: GitHub reports the PR as CONFLICTING with main (useCommitPreflightCapabilities.ts), so the CI workflows have not run at this head. Only Graphite and WIP report. The push that resolves the conflict needs CI and a fresh approval. I'll re-check the resolution then.
— Rames
…ooth-multi-drag # Conflicts: # packages/studio/src/hooks/useCommitPreflightCapabilities.ts
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at 99f9b64c. It merges main (cd0d04e4) into f5f20d01, which I approved.
How I checked the merge: I replayed f5f20d01 onto cd0d04e4 with git merge-tree. The only conflict is useCommitPreflightCapabilities.ts, and every other file merges to exactly what this head has. The resolution keeps both sides: the PR's withTweenIndex import, and main's switch from GSAP_EDIT_BLOCK_COPY to gsapEditBlockMessage, which line 80 now calls. Main's rename is already in the rest of the file, so nothing else needed to change. The PR's diff against main is the same size as before (72 files, +2171 / -933).
Tests at this head:
tsc --noEmiton studio is clean.- The full studio unit suite passes: 7479 tests, no failures. The clipboard test that failed last round passes here too.
- studio-server history and routes: 664 pass.
CI at this head: 41 passed, nothing failing, 22 still pending at post time (the 20 edit-accuracy shards, Windows studio-2, CodeQL). Typecheck, Lint and Test (studio) are green.
My review of f5f20d01 still applies, including the reuse and simplify notes.
— Rames
terencecho
left a comment
There was a problem hiding this comment.
Approving 99f9b64c, which retracts my changes-requested review on 2905eca5. The undo race I reproduced is fixed and pinned by a test, the new undo-step claims hold up where I could probe them, and the merge with main changes nothing of the PR's own.
It touches 72 files. Not draft, not stacked: a merge of main (cd0d04e4) into the approved f5f20d01. Rames Jusso (jrusso1020) approved both f5f20d01 and this head.
The blocker is fixed (useEditHistoryActions.ts)
- Each pending edit now records the claim count when it begins.
editClaimedisclaims() > claimsAtBegin, and the server undo is toldclaimedAfter = claimsAtBegin. A key-press snapshot no longer decides anything. - Truth table with the real hook (begin = 7): claim counted before the key (8 at the key) with the server stepping the move: no reapply. Claim counted while undo waits: no reapply. Claim never counted with the server stepping an older entry, or answering empty: one reapply. Claim counted but the server answer is empty, a mismatch or a failure: one reapply (the file still has the move). My old probe (count fixed at 8) now reapplies once, which is right, because that edit began at 8 and never claimed.
- The PR's tests now include "claim counted before the key" (7, 8, 8). 12 mutants of the confirmation logic, the begin snapshot, the clock wiring and the page scope were all caught.
What I verified (head tarball, deps built, NODE_ENV=test)
- The 20 changed studio test files, 3 runs: 474/474. Parsers
gsapWriter.parity151/151, studio-server history and routes 228 passed, 2 skipped.tsc --noEmitclean for parsers, studio-server and studio;oxlintandoxfmt --checkclean. - The merge changed nothing of the PR's own. The tree differs from
mainin exactly the PR's 72 files (66 changed, 6 added), and the+/-lines of every file match the approvedf5f20d01. The one conflict, the imports ofuseCommitPreflightCapabilities.ts, keeps both sides:withTweenIndexandmain's renamedgsapEditBlockMessage. - Server unchanged since my last review: 1500 of 1500 random batch-route scenarios give the same bytes, status, error and
changedas the merge-base; 2000 of 2000 at parser level.files.ts,clipTweens.tsandgsapWriterAcorn.tsare byte-identical to what I reviewed. The one server change isgroup.lastWriteAt = this.now()inholdClaim, which dates a held claim by its last write; its test fails on the base source. The history log format is untouched, so an old log reads the same (read from the diff; I did not run a separate old-log probe). - New undo-step claims on the real
ProjectHistory: a held key joins the same key at any later time; a different key, a plain claim or an outside write ends the hold; undo by id of a held claim works, andstepcommits it first. - Page scope is a random id per page mount, so a reload or a second tab cannot join another page's step (read, and a mutant that removes it is caught). A failed save claims nothing, so no key is left held.
Non-blocking
- Overlapping moves can still split. On the real
ProjectHistory, claims for move 1 timing, move 2 timing, then move 1's fold give three entries, and the first Cmd+Z undoes only the fold. In the client the timing writes are queued (enqueueEdit) but the fold runs after the queued promise resolves, outside the queue, so a second move could interleave. I reproduced the server half; I did not show the client interleaving.mainsplit the same way, but "one Cmd+Z however slow the save" is stronger than what holds for overlapping moves. - The claim count cannot tell whose claim landed. If another edit's claim lands after this edit began while this edit claims nothing (a no-op or failed save),
editClaimedis true, the server undoes the other claim, and there is no paint-back. I probed the count logic, not this case. - Test gaps (carried from last round, still true): no test has a retime, a non-retime, then a retime in one batch; dropping the hold re-sync, not ending a run at a non-retime mutation and an off-by-one on the run index all pass (they change bytes in many fuzz scenarios). Of 38 mutants over the new server and parser code, 19 were caught and 19 survive; Rames's 10 of 11 on the undo tests agree with my 12 of 12 there.
Not exercised: the end state in a browser (I could not observe it this round either), a real 98-clip project, the PR's timings, the Desktop host, Windows, a multi-file group edit, linked or locked clips, keyboard pickup of a large selection, Escape of a group drag, the 409 race on the route.
CI. All checks at this head finished: 66 passed, 14 skipped, 0 failing, 0 pending. CI is a reference; the verdict rests on the evidence above.
Reviewed on the PR head 99f9b64c. This is a review verdict, not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
What
Moving or resizing many selected clips on the timeline works at any count again: the 3-clip hand-edit cap is gone, and dropping 98 selected clips settles in about 1 s instead of 15 s.
gsap-mutationsrequest per clip, one after another, each parsing and rewriting the whole file. It now sends onegsap-mutations-batchrequest per file, and the server folds a run of clip retimes into one script parse and one hold sync. Clip lookups during the retime share one DOM walk per file instead of one selector query per clip. Ownership and rollback are unchanged: a file is one owned step.data-duration; a batch now patches every clip, then syncs once.index.htmldeleted theindex.html#idand bare-id entries that sub-composition files write for their own elements, so their keyframes vanished until each file was read again.index.htmlnow leaves those to their owner, and a file that re-reads unchanged publishes nothing.useTimelineClipTimingEditing, beside the group one.Why
Dragging 4+ clips on a launch-size project took 10 to 15 s or more to land, which is why the cap existed. Profiled on the public 98-clip launch project with every clip selected, the drop paid for one GSAP request per clip, one file parse per clip on the server, one selector query per clip per retime, one existence check per selected element, and one store update per clip, each re-rendering the timeline.
Related work
Refs #4932 (added the cap and the first bulk-action fixes). The drop is now main-thread bound (Chrome task time about equals wall time). Three follow-ups take it further, each its own PR: clips that did not change skip rendering, the post-save preview and keyframe sync stops re-publishing unchanged data, and a group move writes timing and tween positions in one save.
How
packages/studio/tests/e2e/timeline-bulk-actions.mjsgainsmove-all-small,move-all-past-endandresize-all-end(select every clip, drag, drop) and reportsdropTaskMs, the browser main-thread task time spent on the drop. "Before" is this branch's first commit (cap removed, nothing else), so both sides accept the drag. Before and after ran interleaved, one round each, five rounds, on a shared 32-core Linux box under load (load average 15 to 24).Medians of 5, measured before the rebase onto main. "Settled" is the captures' "idle". On the same box nearly idle (load 4 to 10), three runs per action at this PR's head: settled 0.83 s, 0.76 s and 0.90 s, with the save finished at 0.51 s, 0.49 s and 0.61 s. Absolute times move with the box's load (the same after build measured 0.85 to 3 s across an afternoon); compare the columns.
In a folded run of clip retimes, the per-mutation
mutationChangesflags report whether each retime moved a value, and a hold re-sync is credited to the run's last slot; single requests credit the sync to the request that ran it. The only reader of these flags ignores retime mutations.Before
Every clip selected, dragged right, dropped. The banner counts from the drop and then shows three numbers: clips placed (first frame painted after the drop, clips at their new place), saved (the last file write finished) and idle (no request in flight and no long task for 500 ms, the same "settled" as the table). Base build, shared box at load 16 to 22: placed 233 ms, saved 16.6 s, idle 18.6 s.
drag-all-before.mp4
After
Same drag on this branch, same box and load: placed 225 ms, saved 0.84 s, idle 1.42 s.
drag-all-after.mp4
While a save is in flight, undo reverts the move at once, a second drag is accepted and its save queues behind the first, and the preview plays tweens at their old times until the GSAP write lands. A follow-up writes timing and tween positions in one save.
One Cmd+Z after a group move
Three clips selected and dragged right, with every tween save held 3 s; once the save finished, Cmd+Z was pressed once. The fixture is a three-clip composition built for this capture. Before is this branch without the fix (main also sends one tween request per clip, which hides the split).
Before: the clips stay at 2.61 s while their tweens and keyframes go back to 0 s.
undo-before.webm
After: one Cmd+Z puts the clips and their tweens back at 0 s.
undo-after.webm
Test plan
foldGsapMutationIntoHistory.#idlookup comes from one DOM walk.index.htmlkeeps a sub-composition's entries.