From 3583e2c575d476e7d6ffab85744bae105f99ed46 Mon Sep 17 00:00:00 2001 From: Arham Wani Date: Sat, 8 Aug 2026 23:40:59 +0530 Subject: [PATCH 1/3] fix(timeline): preserve partially anchored regions --- electron/ai-edition/document-service.test.ts | 85 ++++++++++++++++++++ src/lib/ai-edition/document/timeline.test.ts | 23 ++++++ src/lib/ai-edition/document/timeline.ts | 2 +- 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/electron/ai-edition/document-service.test.ts b/electron/ai-edition/document-service.test.ts index 2b5782aa6..43abada37 100644 --- a/electron/ai-edition/document-service.test.ts +++ b/electron/ai-edition/document-service.test.ts @@ -363,6 +363,91 @@ describe("DocumentService", () => { expect(after.project.primaryAssetId).toBe(b.assets[1]?.id); }); + it("resequences other assets and rederives their anchored regions", async () => { + const created = await service.createProject("P"); + const withA = await service.addAsset(created.project.id, { path: "/tmp/a.mp4" }); + const withB = await service.addAsset(created.project.id, { path: "/tmp/b.mp4" }); + const assetA = withA.assets[0]?.id ?? ""; + const assetB = withB.assets[1]?.id ?? ""; + expect(assetA).toBeTruthy(); + expect(assetB).toBeTruthy(); + + await service.saveProject({ + ...withB, + timeline: { + ...withB.timeline, + clips: [ + { + id: "a_1", + assetId: assetA, + sourceStartSec: 0, + sourceEndSec: 2, + timelineStartSec: 0, + timelineEndSec: 2, + wordRefs: [], + origin: "user", + reason: "test", + }, + { + id: "b_1", + assetId: assetB, + sourceStartSec: 10, + sourceEndSec: 14, + timelineStartSec: 2, + timelineEndSec: 6, + wordRefs: [], + origin: "user", + reason: "test", + }, + { + id: "a_2", + assetId: assetA, + sourceStartSec: 2, + sourceEndSec: 3, + timelineStartSec: 6, + timelineEndSec: 7, + wordRefs: [], + origin: "user", + reason: "test", + }, + { + id: "b_2", + assetId: assetB, + sourceStartSec: 20, + sourceEndSec: 22, + timelineStartSec: 7, + timelineEndSec: 9, + wordRefs: [], + origin: "user", + reason: "test", + }, + ], + }, + zoomRanges: [ + { + id: "zoom_b_2", + clipId: "b_2", + sourceStartSec: 20.5, + sourceEndSec: 21.5, + startMs: 7500, + endMs: 8500, + depth: 3, + focus: { cx: 0.5, cy: 0.5 }, + }, + ], + }); + + const after = await service.removeAsset(created.project.id, assetA); + + expect(after.timeline.clips).toMatchObject([ + { id: "b_1", timelineStartSec: 0, timelineEndSec: 4 }, + { id: "b_2", timelineStartSec: 4, timelineEndSec: 6 }, + ]); + expect(after.zoomRanges).toEqual([ + expect.objectContaining({ id: "zoom_b_2", startMs: 4500, endMs: 5500 }), + ]); + }); + it("throws when removing a missing asset", async () => { const doc = await service.createProject("P"); await expect(service.removeAsset(doc.project.id, "asset_x")).rejects.toBeInstanceOf( diff --git a/src/lib/ai-edition/document/timeline.test.ts b/src/lib/ai-edition/document/timeline.test.ts index 527cbf836..420d571ef 100644 --- a/src/lib/ai-edition/document/timeline.test.ts +++ b/src/lib/ai-edition/document/timeline.test.ts @@ -1370,6 +1370,29 @@ describe("removeClip — delete a clip, close the gap, drop its pills", () => { expect(next.zoomRanges[0]).toMatchObject({ startMs: 2000, endMs: 4000 }); }); + it("preserves a bare clipId that is not a complete source anchor", () => { + const before = doc(); + before.zoomRanges.push( + makeZoom({ + id: "partial_anchor", + clipId: "clip_a", + sourceStartSec: undefined, + sourceEndSec: undefined, + startMs: 500, + endMs: 1500, + }), + ); + + const next = removeClip(before, "clip_a"); + + expect(next.zoomRanges.map((region) => region.id)).toEqual(["z_b", "partial_anchor"]); + expect(next.zoomRanges[1]).toMatchObject({ + clipId: "clip_a", + startMs: 500, + endMs: 1500, + }); + }); + it("drops every modifier anchored to the last remaining clip", () => { const before = makeDoc({ timeline: { diff --git a/src/lib/ai-edition/document/timeline.ts b/src/lib/ai-edition/document/timeline.ts index 7458cdc61..68bf7d43c 100644 --- a/src/lib/ai-edition/document/timeline.ts +++ b/src/lib/ai-edition/document/timeline.ts @@ -936,7 +936,7 @@ export function removeClip(document: AxcutDocument, clipId: string): AxcutDocume }, }; const withoutRemovedRegions = mapAllRegionCollections(next, (regions) => - regions.filter((region) => region.clipId !== clipId), + regions.filter((region) => !(isAnchored(region) && region.clipId === clipId)), ); return newClips.length > 0 ? rederiveRegionMs(withoutRemovedRegions, newClips) From 2d6cb748ce28c27c630fcfb8b9bb2e376171c2d3 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Thu, 20 Aug 2026 11:52:59 +0200 Subject: [PATCH 2/3] refactor(timeline): give "is this anchored?" a single answer The comment on `isAnchored` promised "one definition, so this can never be asked two different ways". There were two. `document/timeline.ts` tested `sourceStartSec !== undefined`; `timeline/timelineMap.ts` tested `typeof sourceStartSec === "number"`, and the two disagree about `null`. `null` is not exotic -- it survives any in-memory mutation that does not round-trip through zod, which is every edit the store and the agent tools make. Such a region was anchored to `removeClip` and `rederiveRegionMs` and unanchored to the export path, so `rederiveAnchoredRegion` rewrote it to `Math.max(null, clip.sourceStartSec)` -- the clip start -- while the exporter went on placing it by its raw ms. Preview and export disagreed about where the user's zoom was. `hasCompleteClipAnchor` is exported and `isAnchored` is gone, so the promise the comment makes is now true. The stricter `typeof` reading wins, which is also the one that cannot feed `null` into interval math. While the removal filter in `removeClip` was open: it now says why it does not match the trim filter three lines above, since making the two agree is the obvious cleanup that reintroduces #249, and it states the cost of keeping a partially-anchored region so that is a decision on the record rather than an accident. The redundant pre-filter on the survivors path is gone with it -- `rederiveRegionMs` already drops a strict superset, so the old shape walked all four region collections twice per delete. Co-Authored-By: Claude Opus 5 --- src/lib/ai-edition/document/timeline.ts | 63 ++++++++++++++-------- src/lib/ai-edition/timeline/timelineMap.ts | 15 +++++- 2 files changed, 53 insertions(+), 25 deletions(-) diff --git a/src/lib/ai-edition/document/timeline.ts b/src/lib/ai-edition/document/timeline.ts index 68bf7d43c..27eaf0f0e 100644 --- a/src/lib/ai-edition/document/timeline.ts +++ b/src/lib/ai-edition/document/timeline.ts @@ -8,6 +8,7 @@ import { anchoredToRawSpanSec, anchorRegionsWithDerivedMs, dropPillById, + hasCompleteClipAnchor, } from "../timeline/timelineMap"; import { dropTrimPillsByIds, trimAppliesToClip } from "../timeline/trim-mapping"; import { createId } from "./ids"; @@ -27,17 +28,6 @@ export function byStart(a: { startSec: number }, b: { startSec: number }): numbe return a.startSec - b.startSec; } -/** A region is anchored once it states WHERE IN THE SOURCE it lives. Anything missing - * a part of `{clipId, sourceStartSec, sourceEndSec}` still relies on its RAW ms. - * One definition, so "is this anchored?" can never be asked two different ways. */ -function isAnchored( - region: T, -): region is T & { clipId: string; sourceStartSec: number; sourceEndSec: number } { - return ( - !!region.clipId && region.sourceStartSec !== undefined && region.sourceEndSec !== undefined - ); -} - export interface Interval { startSec: number; endSec: number; @@ -299,7 +289,7 @@ export function rederiveRegionMs(document: AxcutDocument, clips: AxcutClip[]): A const clipById = new Map(clips.map((c) => [c.id, c])); return mapAllRegionCollections(document, (regions) => regions.flatMap((region) => { - if (!isAnchored(region)) { + if (!hasCompleteClipAnchor(region)) { return [region]; } const clip = clipById.get(region.clipId); @@ -426,7 +416,7 @@ export function applyProbedDuration( // and so an already-correct anchor is never re-minted. return mapAllRegionCollections(refreshed, (regions, prefix) => regions.flatMap((region) => - isAnchored(region) + hasCompleteClipAnchor(region) ? [region] : (anchorRegionsWithDerivedMs([region], nextClips, () => createId(prefix), @@ -507,7 +497,7 @@ function anchoredRegionsOf(document: AxcutDocument): Array<{ id: string; clipId: ]; return collections .flat() - .filter(isAnchored) + .filter(hasCompleteClipAnchor) .map((region) => ({ id: region.id, clipId: region.clipId })); } @@ -725,16 +715,18 @@ function reconcileRegionsAfterReplace( const clipById = new Map(clips.map((clip) => [clip.id, clip])); return mapAllRegionCollections(document, (regions, prefix) => regions.flatMap((region) => { - if (isAnchored(region) && surviving.has(region.clipId)) { + if (hasCompleteClipAnchor(region) && surviving.has(region.clipId)) { const clip = clipById.get(region.clipId); if (clip) return rederiveAnchoredRegion(region, clip, clips); } const reventilated = anchorRegionsWithDerivedMs([region], clips, () => createId(prefix), ) as StoredRegion[]; - const placed = reventilated.some((next) => isAnchored(next) && surviving.has(next.clipId)); + const placed = reventilated.some( + (next) => hasCompleteClipAnchor(next) && surviving.has(next.clipId), + ); if (placed) return reventilated; - return isAnchored(region) ? [] : reventilated; + return hasCompleteClipAnchor(region) ? [] : reventilated; }), ); } @@ -935,12 +927,37 @@ export function removeClip(document: AxcutDocument, clipId: string): AxcutDocume trimRanges: document.timeline.trimRanges.filter((t) => t.clipId !== clipId), }, }; - const withoutRemovedRegions = mapAllRegionCollections(next, (regions) => - regions.filter((region) => !(isAnchored(region) && region.clipId === clipId)), - ); - return newClips.length > 0 - ? rederiveRegionMs(withoutRemovedRegions, newClips) - : withoutRemovedRegions; + // The asymmetry with the trim filter three lines up is deliberate, and the obvious + // "cleanup" that makes the two match reintroduces #249. + // + // A trim's complete anchor IS a bare `clipId` -- it carries its own `startSec`/ + // `endSec` in source time (`trimAppliesToClip`), so the clip going away takes the + // trim with it. A region carrying only a `clipId` and no source range is NOT + // anchored (`hasCompleteClipAnchor`): it is still placed by its RAW ms, so the clip + // does not own it and deleting the clip must not delete it. + // + // The cost of keeping it, stated so it is a decision and not an accident: that + // region is now unreachable but immortal. With clips [0-10s] and [10-20s] and a bare + // `clipId` zoom at raw 12000-14000ms, deleting the second clip leaves the zoom off + // the end of a 10s ruler -- no pill to click, dropped by `projectRegionsToSource`, + // and re-emitted by every rederive. Hitting "Restore full timeline" then re-anchors + // it from those stale raw ms onto whatever footage now sits at 12-14s. That is the + // same treatment fully-unanchored legacy regions already get, and losing the user's + // region outright is the worse of the two. + // + // Only the last-clip case needs the filter spelled out. With survivors, + // `rederiveRegionMs` already drops every anchored region whose `clipId` is absent + // from the new clips -- a strict superset of "anchored to the one just removed" -- + // so running both walked all four region collections twice and spread the document + // twice per delete. `rederiveRegionMs` bails on an empty clip list (a guard against + // a transient wipe deleting everything), which is why the empty case is handled + // here rather than left to it. + if (newClips.length === 0) { + return mapAllRegionCollections(next, (regions) => + regions.filter((region) => !(hasCompleteClipAnchor(region) && region.clipId === clipId)), + ); + } + return rederiveRegionMs(next, newClips); } export function restoreFullTimeline(document: AxcutDocument): AxcutDocument { diff --git a/src/lib/ai-edition/timeline/timelineMap.ts b/src/lib/ai-edition/timeline/timelineMap.ts index 2a00573a6..b2b0edd2e 100644 --- a/src/lib/ai-edition/timeline/timelineMap.ts +++ b/src/lib/ai-edition/timeline/timelineMap.ts @@ -416,8 +416,19 @@ interface RegionClipAnchor { } /** True when the anchor is usable on its own (all three parts present), i.e. the - * region can be placed without consulting raw-virtual time at all. */ -function hasCompleteClipAnchor( + * region can be placed without consulting raw-virtual time at all. Anything missing + * a part of `{clipId, sourceStartSec, sourceEndSec}` still relies on its RAW ms. + * + * THE definition, so "is this anchored?" can never be asked two different ways. It + * used to be asked twice: the document layer had its own copy testing + * `sourceStartSec !== undefined`, which called a region carrying `null` anchored + * while this one called it unanchored. `null` survives any in-memory mutation that + * does not round-trip through zod, and the two answers sent the same region down + * two different paths -- `rederiveAnchoredRegion` rewrote it to `Math.max(null, ...)` + * i.e. the clip start, while the exporter went on using its raw ms. Preview and + * export disagreed. The `typeof` tests below are what make that unreachable, so + * keep them: `!== undefined` is not the same question. */ +export function hasCompleteClipAnchor( region: T, ): region is T & Required { return ( From 423a6bdebbf863798d0644e1c7144e64512305eb Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Thu, 20 Aug 2026 11:53:02 +0200 Subject: [PATCH 3/3] test(timeline): cover the branch #249 was actually about The new coverage only exercised `newClips.length > 0`. The last-clip branch is the one the issue described and nothing pinned it: with no clip left, `removeClip` skips `rederiveRegionMs` and its own filter is the only thing deciding, so that path could be refactored back to the old semantics with a green suite. It now carries a bare-`clipId` zoom that must survive. Two more regions cover the `null` halves of the anchor separately. A single region carrying two `null`s reads unanchored if only one of the two `typeof` checks is loosened, so it would have pinned neither. And the `removeAsset` characterization test pinned nothing about this fix: its only zoom was fully anchored to a surviving clip, so the old and the new filter both kept it -- it passed against `main` unmodified. It now also carries a zoom with a bare `clipId` on a removed clip, which is what actually routes the fix through `removeAsset`, and asserts the raw ms come out untouched: the region outlives the ruler rather than being deleted, which is the trade-off `removeClip` now documents. Co-Authored-By: Claude Opus 5 --- electron/ai-edition/document-service.test.ts | 80 +++++++++----------- src/lib/ai-edition/document/timeline.test.ts | 38 +++++++++- 2 files changed, 73 insertions(+), 45 deletions(-) diff --git a/electron/ai-edition/document-service.test.ts b/electron/ai-edition/document-service.test.ts index 43abada37..6cbdb97c9 100644 --- a/electron/ai-edition/document-service.test.ts +++ b/electron/ai-edition/document-service.test.ts @@ -367,6 +367,23 @@ describe("DocumentService", () => { const created = await service.createProject("P"); const withA = await service.addAsset(created.project.id, { path: "/tmp/a.mp4" }); const withB = await service.addAsset(created.project.id, { path: "/tmp/b.mp4" }); + // Four clips that differ only in id / asset / four numbers. + const clip = ( + id: string, + assetId: string, + [sourceStartSec, sourceEndSec]: [number, number], + [timelineStartSec, timelineEndSec]: [number, number], + ) => ({ + id, + assetId, + sourceStartSec, + sourceEndSec, + timelineStartSec, + timelineEndSec, + wordRefs: [], + origin: "user" as const, + reason: "test", + }); const assetA = withA.assets[0]?.id ?? ""; const assetB = withB.assets[1]?.id ?? ""; expect(assetA).toBeTruthy(); @@ -377,50 +394,10 @@ describe("DocumentService", () => { timeline: { ...withB.timeline, clips: [ - { - id: "a_1", - assetId: assetA, - sourceStartSec: 0, - sourceEndSec: 2, - timelineStartSec: 0, - timelineEndSec: 2, - wordRefs: [], - origin: "user", - reason: "test", - }, - { - id: "b_1", - assetId: assetB, - sourceStartSec: 10, - sourceEndSec: 14, - timelineStartSec: 2, - timelineEndSec: 6, - wordRefs: [], - origin: "user", - reason: "test", - }, - { - id: "a_2", - assetId: assetA, - sourceStartSec: 2, - sourceEndSec: 3, - timelineStartSec: 6, - timelineEndSec: 7, - wordRefs: [], - origin: "user", - reason: "test", - }, - { - id: "b_2", - assetId: assetB, - sourceStartSec: 20, - sourceEndSec: 22, - timelineStartSec: 7, - timelineEndSec: 9, - wordRefs: [], - origin: "user", - reason: "test", - }, + clip("a_1", assetA, [0, 2], [0, 2]), + clip("b_1", assetB, [10, 14], [2, 6]), + clip("a_2", assetA, [2, 3], [6, 7]), + clip("b_2", assetB, [20, 22], [7, 9]), ], }, zoomRanges: [ @@ -434,6 +411,18 @@ describe("DocumentService", () => { depth: 3, focus: { cx: 0.5, cy: 0.5 }, }, + // Bare `clipId`, no source range: not an anchor, so removing the asset that owns + // `a_2` must NOT take it. This is what routes #249's fix through `removeAsset` + // -- the fully-anchored zoom above survives either way, so on its own it pins + // nothing about the predicate. + { + id: "zoom_partial_a_2", + clipId: "a_2", + startMs: 6000, + endMs: 7000, + depth: 3, + focus: { cx: 0.5, cy: 0.5 }, + }, ], }); @@ -445,6 +434,9 @@ describe("DocumentService", () => { ]); expect(after.zoomRanges).toEqual([ expect.objectContaining({ id: "zoom_b_2", startMs: 4500, endMs: 5500 }), + // Survives, and keeps its raw ms untouched -- now past the end of a 6s timeline. + // That is the documented trade-off in `removeClip`: unreachable beats deleted. + expect.objectContaining({ id: "zoom_partial_a_2", startMs: 6000, endMs: 7000 }), ]); }); diff --git a/src/lib/ai-edition/document/timeline.test.ts b/src/lib/ai-edition/document/timeline.test.ts index 420d571ef..e33d82662 100644 --- a/src/lib/ai-edition/document/timeline.test.ts +++ b/src/lib/ai-edition/document/timeline.test.ts @@ -1409,6 +1409,37 @@ describe("removeClip — delete a clip, close the gap, drop its pills", () => { sourceStartSec: undefined, sourceEndSec: undefined, }), + // #249, and the branch nothing pinned: with no clip left, `removeClip` skips + // `rederiveRegionMs` entirely, so this filter is the only thing deciding. A bare + // `clipId` is not an anchor -- the region is still placed by its raw ms, so the + // clip going away must not take it. Without this case the ternary can be + // refactored back to the old semantics with a green suite. + makeZoom({ + id: "partial_zoom", + clipId: "clip_a", + sourceStartSec: undefined, + sourceEndSec: undefined, + }), + // The same region after an in-memory edit that never round-tripped through zod: + // `null`, not `undefined`. The document layer used to call this one anchored + // (`!== undefined`) while the export path called it unanchored (`typeof`), and + // the two answers moved it to two different places -- `rederiveAnchoredRegion` + // slid it to `Math.max(null, ...)`, i.e. the clip start, while the exporter kept + // using its raw ms. One predicate now. Both halves get a case, because a single + // region carrying two `null`s still reads unanchored if only one check is + // loosened, and would pin neither. + makeZoom({ + id: "null_start_zoom", + clipId: "clip_a", + sourceStartSec: null as unknown as undefined, + sourceEndSec: 1, + }), + makeZoom({ + id: "null_end_zoom", + clipId: "clip_a", + sourceStartSec: 0, + sourceEndSec: null as unknown as undefined, + }), ], annotations: [ { @@ -1465,7 +1496,12 @@ describe("removeClip — delete a clip, close the gap, drop its pills", () => { const next = removeClip(before, "clip_a"); expect(next.timeline.clips).toEqual([]); - expect(next.zoomRanges.map((region) => region.id)).toEqual(["legacy_zoom"]); + expect(next.zoomRanges.map((region) => region.id)).toEqual([ + "legacy_zoom", + "partial_zoom", + "null_start_zoom", + "null_end_zoom", + ]); expect(next.annotations).toEqual([]); expect((next.legacyEditor as { speedRegions: Array<{ id: string }> }).speedRegions).toEqual([ expect.objectContaining({ id: "legacy_speed" }),