diff --git a/electron/ai-edition/document-service.test.ts b/electron/ai-edition/document-service.test.ts index 2b5782aa6..6cbdb97c9 100644 --- a/electron/ai-edition/document-service.test.ts +++ b/electron/ai-edition/document-service.test.ts @@ -363,6 +363,83 @@ 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" }); + // 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(); + expect(assetB).toBeTruthy(); + + await service.saveProject({ + ...withB, + timeline: { + ...withB.timeline, + clips: [ + 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: [ + { + 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 }, + }, + // 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 }, + }, + ], + }); + + 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 }), + // 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 }), + ]); + }); + 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..e33d82662 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: { @@ -1386,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: [ { @@ -1442,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" }), diff --git a/src/lib/ai-edition/document/timeline.ts b/src/lib/ai-edition/document/timeline.ts index 7458cdc61..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) => 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 (