Repository navigation
fix(cli): preserve authored clip length when moving - #5315
Conversation
Signed-off-by: Rudy Celekli <rudy@gradiahq.com>
jrusso1020
left a comment
There was a problem hiding this comment.
Approving. Reviewed at daab5e74.
The bug is still on main. moveMutation checks bounds with the resolved row.duration but only rewrites data-start, so a legacy data-start/data-end clip keeps its old end and changes length. Writing the resolved duration and removing data-end follows the contract in compositionContract.ts (writers emit start + duration, data-end is derived). It also fixes a quieter case: a duration-authored clip carrying a compiler-derived data-end used to end up with a conflicting stale end after a move. Clips whose length comes from the media or the image default are left alone because durationAuthored is false for them, and the image control case covers that.
On a local merge with current main, after a full build, timeline.e2e.test.ts passes 68 of 68. With the old a2Shared.ts put back, exactly the four end-authored cases fail (applied and --plan, both directions).
A non-blocking note on the same pattern next door: trimMutation also writes data-start and data-duration without clearing data-end. Its length comes out right because the canonical duration wins, but a trimmed legacy clip is left with a conflicting data-end that lint reports. Clearing it there too, or sharing a small "write canonical timing" helper between the two, would close the whole class. Fine as a follow-up.
Need: yes. Size: ok. Reuse: follows the existing canonical timing contract. Simpler: already minimal.
— Rames
What
Preserve an end-authored clip's resolved length when moving it with the timeline CLI.
Why
The timeline reader supports older clips with
data-startanddata-end. The move command checked bounds and overlaps using the original length but only changed the start attribute. A two-second clip at 1–3 seconds moved to 2 seconds became one second long; moving a two-second clip at 4–6 seconds to 1 second made it five seconds long. The same incorrect result appeared in--plan.Related work
This is a small correction to the existing timeline move command introduced in #4221. Open #5279 concerns atomic batch undo receipts; its implementation is independent, although both PRs append CLI regressions to the same test file.
How
For a clip with an authored duration, write its already resolved duration and remove the derived/legacy end attribute alongside the new start. This follows the existing canonical timing writer's contract. Clips whose duration comes from natural media length or an image default retain their unauthored duration.
Test plan
The full CLI suite reports 4594 passed, 6 failed, 49 skipped out of 4649 collected tests. An unchanged-production comparison with the same tests and environment reports 4590 passed, 10 failed, 49 skipped, including the four timing regressions. Five exact failures match between runs: the history held-lock test, the atomic file watcher test, and three Sherpa installation tests. The sixth failure is in the unchanged real-filesystem watcher module: the new-directory case fails after the fix, while the manifest-write case fails in the comparison. That module has no dependency on the changed timeline helper. The full CLI suite is not reported as passing.
Validation covers CLI timing and persistence on macOS. Hosted upstream CI and maintainer review remain outstanding.