From 339cdaf6ed598e27276a97ecf8be42946ccc9f87 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sat, 19 Sep 2026 05:03:35 -0400 Subject: [PATCH] fix(studio): a drop never shows the timeline an older state A clip drop holds preview manifests while it is in flight, writes the whole placement result to the timeline store in one update, and reloads the preview once at the end. Before, a manifest arriving mid-drop resurrected a clip the drop had just removed, and the drag preview fell back to the old state until the first step landed. Adds a test that mounts the real player hook and asserts the sequence of store states after a drop never returns to an older one. --- .../studio/src/hooks/useRazorSplit.test.ts | 59 ++++- packages/studio/src/hooks/useRazorSplit.ts | 38 +-- .../src/hooks/useTimelineDeleteOps.test.tsx | 39 ++- .../studio/src/hooks/useTimelineDeleteOps.ts | 5 +- .../studio/src/hooks/useTimelineEditing.ts | 3 +- .../components/timelineClipDragCommit.ts | 18 +- .../timelineClipDragGestureLifecycle.test.ts | 1 + .../timelinePlacementCommit.test.ts | 249 +++++++----------- .../components/timelinePlacementCommit.ts | 128 +++++++-- .../timelinePlacementMonotonic.test.ts | 155 +++++++++++ .../timelinePlacementTestHarness.ts | 162 ++++++++++++ .../src/player/hooks/useTimelinePlayer.ts | 7 +- .../src/player/lib/timelineManifestHold.ts | 16 ++ 13 files changed, 678 insertions(+), 202 deletions(-) create mode 100644 packages/studio/src/player/components/timelinePlacementMonotonic.test.ts create mode 100644 packages/studio/src/player/components/timelinePlacementTestHarness.ts create mode 100644 packages/studio/src/player/lib/timelineManifestHold.ts diff --git a/packages/studio/src/hooks/useRazorSplit.test.ts b/packages/studio/src/hooks/useRazorSplit.test.ts index 5d230fabb2..73f05109c4 100644 --- a/packages/studio/src/hooks/useRazorSplit.test.ts +++ b/packages/studio/src/hooks/useRazorSplit.test.ts @@ -9,6 +9,7 @@ import { createPersistentEditHistoryStore } from "./usePersistentEditHistory"; import { createEmptyEditHistory } from "../utils/editHistory"; import type { EditHistoryStorageAdapter } from "../utils/editHistoryStorage"; import { createSplitFetchMock, mountProbe } from "./useRazorSplit.testHelpers"; +import type { PlacementFold } from "../player/components/timelinePlacementCommit"; (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; @@ -49,11 +50,19 @@ interface SplitRequest { type SingleSplit = (element: TimelineElement, splitTime: number) => Promise; type SplitAll = (splitTime: number) => Promise; +type PlacementSplit = ( + element: TimelineElement, + splitTime: number, + fold: PlacementFold, +) => Promise; interface Harness { splitRequests: SplitRequest[]; singleRef: { current: SingleSplit | undefined }; allRef: { current: SplitAll | undefined }; + placementRef: { current: PlacementSplit | undefined }; + reloadPreview: ReturnType; + forceReloadSdkSession: ReturnType; root: ReturnType; } @@ -76,9 +85,12 @@ function mountRazorSplit(): Harness { const singleRef: { current: SingleSplit | undefined } = { current: undefined }; const allRef: { current: SplitAll | undefined } = { current: undefined }; + const placementRef: { current: PlacementSplit | undefined } = { current: undefined }; + const reloadPreview = vi.fn(); + const forceReloadSdkSession = vi.fn(); function Component() { - const { handleRazorSplit, handleRazorSplitAll } = useRazorSplit({ + const { handleRazorSplit, handleRazorSplitAll, handlePlacementSplit } = useRazorSplit({ projectId: "p1", activeCompPath: ROOT_FILE, showToast: () => {}, @@ -86,15 +98,25 @@ function mountRazorSplit(): Harness { disk[path] = content; }, recordEdit: async () => {}, - reloadPreview: () => {}, + reloadPreview, + forceReloadSdkSession, }); singleRef.current = handleRazorSplit; allRef.current = handleRazorSplitAll; + placementRef.current = handlePlacementSplit; return null; } const root = mountProbe(Component); - return { splitRequests, singleRef, allRef, root }; + return { + splitRequests, + singleRef, + allRef, + placementRef, + reloadPreview, + forceReloadSdkSession, + root, + }; } afterEach(() => { @@ -162,6 +184,37 @@ describe("useRazorSplit — sub-comp coordinate rebasing", () => { }); }); +// A drop reloads the preview once, at its end; an interior split must not reload on its own. +describe("useRazorSplit — preview reload folded into a drop", () => { + let harness: Harness; + beforeEach(() => { + harness = mountRazorSplit(); + }); + afterEach(() => { + act(() => harness.root.unmount()); + }); + + it("skips reloading the preview for a split folded into a drop, but still refreshes the sdk session", async () => { + await act(async () => { + await harness.placementRef.current!(rootElement, 4, { + coalesceKey: "clip-overwrite:1", + coalesceMs: Number.POSITIVE_INFINITY, + }); + }); + + expect(harness.reloadPreview).not.toHaveBeenCalled(); + expect(harness.forceReloadSdkSession).toHaveBeenCalledTimes(1); + }); + + it("still reloads the preview for a plain (non-drop) split", async () => { + await act(async () => { + await harness.singleRef.current!(rootElement, 4); + }); + + expect(harness.reloadPreview).toHaveBeenCalledTimes(1); + }); +}); + // ── Bug 1: split must resync the SDK session so undo isn't refused ──────────── const memoryStorage = (): EditHistoryStorageAdapter => { diff --git a/packages/studio/src/hooks/useRazorSplit.ts b/packages/studio/src/hooks/useRazorSplit.ts index 9bd1fe902f..ba1b41d67b 100644 --- a/packages/studio/src/hooks/useRazorSplit.ts +++ b/packages/studio/src/hooks/useRazorSplit.ts @@ -36,20 +36,28 @@ export function useRazorSplit({ const projectIdRef = useRef(projectId); projectIdRef.current = projectId; - const synchronize = useCallback(() => { - let failure: unknown; - try { - forceReloadSdkSession?.(); - } catch (error) { - failure = error; - } - try { - reloadPreview(); - } catch (error) { - failure ??= error; - } - if (failure) throw failure; - }, [forceReloadSdkSession, reloadPreview]); + // skipPreviewReload: true when this cut is one step folded into a drop, whose + // own single reload (after every step lands) replaces this one — otherwise the + // preview would show this step's DOM before the next step makes it stale. + const synchronize = useCallback( + (skipPreviewReload: boolean) => { + let failure: unknown; + try { + forceReloadSdkSession?.(); + } catch (error) { + failure = error; + } + if (!skipPreviewReload) { + try { + reloadPreview(); + } catch (error) { + failure ??= error; + } + } + if (failure) throw failure; + }, + [forceReloadSdkSession, reloadPreview], + ); const runCut = useCallback( async ( @@ -75,7 +83,7 @@ export function useRazorSplit({ writeProjectFile, recordEdit, observeProjectFileVersion, - synchronize, + synchronize: () => synchronize(Boolean(fold)), }); trackStudioRazorSplit({ mode, count: result.splitCount }); if (result.syncFailed) { diff --git a/packages/studio/src/hooks/useTimelineDeleteOps.test.tsx b/packages/studio/src/hooks/useTimelineDeleteOps.test.tsx index 16c6f3c922..e71b974875 100644 --- a/packages/studio/src/hooks/useTimelineDeleteOps.test.tsx +++ b/packages/studio/src/hooks/useTimelineDeleteOps.test.tsx @@ -59,6 +59,8 @@ describe("useTimelineDeleteOps: ripple undo label", () => { handleTimelineGroupMove: DeleteOpsOptions["handleTimelineGroupMove"]; showToast?: DeleteOpsOptions["showToast"]; recordEdit?: DeleteOpsOptions["recordEdit"]; + reloadPreview?: DeleteOpsOptions["reloadPreview"]; + forceReloadSdkSession?: DeleteOpsOptions["forceReloadSdkSession"]; }) { const elements = [el("hf-a", 0, 2), el("hf-b", 2, 2), el("hf-c", 4, 2)]; let hook: ReturnType | null = null; @@ -70,7 +72,8 @@ describe("useTimelineDeleteOps: ripple undo label", () => { showToast: overrides.showToast ?? vi.fn(), writeProjectFile: vi.fn().mockResolvedValue(undefined), recordEdit: overrides.recordEdit ?? vi.fn().mockResolvedValue(undefined), - reloadPreview: vi.fn(), + reloadPreview: overrides.reloadPreview ?? vi.fn(), + forceReloadSdkSession: overrides.forceReloadSdkSession, previewIframeRef: { current: null }, handleTimelineGroupMove: overrides.handleTimelineGroupMove, }); @@ -145,4 +148,38 @@ describe("useTimelineDeleteOps: ripple undo label", () => { expect.objectContaining({ coalesceKey: "clip-overwrite:3", coalesceMs: Infinity }), ); }); + + // A drop reloads the preview once, at its end; an interior remove must not reload on its own. + it("skips reloading the preview for an overwrite delete, but still refreshes the sdk session", async () => { + const handleTimelineGroupMove = vi.fn().mockResolvedValue(undefined); + const reloadPreview = vi.fn(); + const forceReloadSdkSession = vi.fn(); + const { b, getHook } = mountDeleteHarness({ + handleTimelineGroupMove, + reloadPreview, + forceReloadSdkSession, + }); + + await act(async () => { + await getHook().deleteTimelineElements([b], { + coalesceKey: "clip-overwrite:4", + coalesceMs: Number.POSITIVE_INFINITY, + }); + }); + + expect(reloadPreview).not.toHaveBeenCalled(); + expect(forceReloadSdkSession).toHaveBeenCalledTimes(1); + }); + + it("still reloads the preview for a plain (non-drop) delete", async () => { + const handleTimelineGroupMove = vi.fn().mockResolvedValue(undefined); + const reloadPreview = vi.fn(); + const { b, getHook } = mountDeleteHarness({ handleTimelineGroupMove, reloadPreview }); + + await act(async () => { + await getHook().handleTimelineElementDelete(b); + }); + + expect(reloadPreview).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/studio/src/hooks/useTimelineDeleteOps.ts b/packages/studio/src/hooks/useTimelineDeleteOps.ts index a1d2178bab..9b015611fa 100644 --- a/packages/studio/src/hooks/useTimelineDeleteOps.ts +++ b/packages/studio/src/hooks/useTimelineDeleteOps.ts @@ -223,7 +223,10 @@ export function useTimelineDeleteOps({ usePlayerStore.getState().setSelectedElementIds(new Set()); } forceReloadSdkSession?.(); - reloadPreview(); + // Folded into a drop: the drop's own single reload (after every step + // lands) replaces this one, so an interior remove never shows the + // preview a state the next step is about to make stale. + if (!overwrite) reloadPreview(); // A failed ripple already showed its own toast above; the user did one // thing (delete), so they get one message, not this generic follow-up too. if (!rippleFailed && !overwrite) { diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index cba4f45eb6..063291f2ef 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -452,8 +452,9 @@ export function useTimelineEditing({ split: handlePlacementSplit, remove: deleteTimelineElements, toast: (message) => showToast(message, "error"), + reloadPreview, }), - [handlePlacementSplit, deleteTimelineElements, showToast], + [handlePlacementSplit, deleteTimelineElements, showToast, reloadPreview], ); return { diff --git a/packages/studio/src/player/components/timelineClipDragCommit.ts b/packages/studio/src/player/components/timelineClipDragCommit.ts index 6b7f615e60..71dc85e770 100644 --- a/packages/studio/src/player/components/timelineClipDragCommit.ts +++ b/packages/studio/src/player/components/timelineClipDragCommit.ts @@ -117,6 +117,8 @@ export function persistMoveEdits( coalesceKey?: string, operation: TimelineMoveOperation = "timing", coalesceMs?: number, + /** False when the caller already wrote the end state to the store (a placement drop does). */ + updateStore = true, ): Promise { if (edits.length === 0) return Promise.resolve(true); const { updateElement, onMoveElement, onMoveElements } = deps; @@ -150,7 +152,7 @@ export function persistMoveEdits( writtenTrack == null ? e.updates : { ...e.updates, authoredTrack: writtenTrack }, ); }; - for (const e of edits) applyEdit(e); + if (updateStore) for (const e of edits) applyEdit(e); // The store above gets DISPLAY lanes; the file below gets the authored-space // track when one was resolved (see TimelineMoveEdit.persistTrack). const persistEdits = edits.map((e) => @@ -167,6 +169,7 @@ export function persistMoveEdits( // restore the preview manifest's pre-gesture lane. Reassert the durable // result after persistence, but only while this remains the latest // optimistic gesture so an older save can never clobber a newer drag. + if (!updateStore) return true; for (const e of edits) { const key = keyOf(e.element); if (isLatestTimelineOptimisticGesture(updateElement, revision, key)) applyEdit(e); @@ -174,9 +177,15 @@ export function persistMoveEdits( return true; }, (error) => { - for (const p of prev) { - if (isLatestTimelineOptimisticGesture(updateElement, revision, p.key)) { - updateElement(p.key, { start: p.start, track: p.track, authoredTrack: p.authoredTrack }); + if (updateStore) { + for (const p of prev) { + if (isLatestTimelineOptimisticGesture(updateElement, revision, p.key)) { + updateElement(p.key, { + start: p.start, + track: p.track, + authoredTrack: p.authoredTrack, + }); + } } } console.error("[Timeline] Failed to persist clip edits", error); @@ -262,6 +271,7 @@ export function commitDraggedClipMove(rawDrag: DraggedClipState, deps: DragCommi fold.coalesceKey, edits.some((e) => e.updates.track !== e.element.track) ? "lane-reorder" : "timing", fold.coalesceMs, + false, ), deps, ); diff --git a/packages/studio/src/player/components/timelineClipDragGestureLifecycle.test.ts b/packages/studio/src/player/components/timelineClipDragGestureLifecycle.test.ts index d389ff92e9..2e7540e193 100644 --- a/packages/studio/src/player/components/timelineClipDragGestureLifecycle.test.ts +++ b/packages/studio/src/player/components/timelineClipDragGestureLifecycle.test.ts @@ -189,6 +189,7 @@ describe("timeline clip drag gesture lifecycle", () => { split: vi.fn(async () => true), remove: vi.fn(async () => true), toast: vi.fn(), + reloadPreview: vi.fn(), }, }, }); diff --git a/packages/studio/src/player/components/timelinePlacementCommit.test.ts b/packages/studio/src/player/components/timelinePlacementCommit.test.ts index 6ac4207c85..c6ad394cf7 100644 --- a/packages/studio/src/player/components/timelinePlacementCommit.test.ts +++ b/packages/studio/src/player/components/timelinePlacementCommit.test.ts @@ -1,29 +1,13 @@ import { describe, expect, it, vi } from "vitest"; -import type { TimelineGroupResizeChange } from "../../hooks/useTimelineGroupEditing"; -import type { TimelineElement } from "../store/playerStore"; -import type { DragCommitDeps, TimelineMoveEdit } from "./timelineClipDragCommit"; -import type { DraggedClipState } from "./timelineClipDragTypes"; +import type { TimelineMoveEdit } from "./timelineClipDragCommit"; import type { PlaceClipResult } from "./timelinePlacement"; import { buildPlacementSteps, commitPlacementDrop, placementRefusal, runPlacementSteps, - type PlacementFold, - type PlacementOps, } from "./timelinePlacementCommit"; -import { - buildEditHistoryEntry, - createEmptyEditHistory, - hashEditHistoryContent, - pushEditHistoryEntry, - undoEditHistory, - type EditHistoryState, -} from "../../utils/editHistory"; - -function clip(id: string, start: number, duration: number, extra: Partial = {}) { - return { id, domId: id, tag: "video", start, duration, track: 1, ...extra } as TimelineElement; -} +import { clip, createFakeProject, depsOf, dragOf, type Doc } from "./timelinePlacementTestHarness"; const dragged = clip("d", 20, 2); const draggedEdit = (start: number): TimelineMoveEdit => ({ @@ -223,144 +207,6 @@ describe("placementRefusal", () => { }); }); -interface Doc { - [id: string]: { start: number; duration: number; playbackStart?: number }; -} - -/** A tiny stand-in for the project file plus the history recorder, driven by the real reducer. */ -function createFakeProject(initial: Doc) { - let doc: Doc = structuredClone(initial); - let history: EditHistoryState = createEmptyEditHistory(); - let clock = 0; - const serialize = (d: Doc) => - JSON.stringify(Object.entries(d).sort(([x], [y]) => (x < y ? -1 : 1))); - const record = (mutate: () => void, label: string, fold: PlacementFold) => { - const before = serialize(doc); - mutate(); - history = pushEditHistoryEntry( - history, - buildEditHistoryEntry({ - id: `e${clock}`, - projectId: "p", - label, - kind: "timeline", - coalesceKey: fold.coalesceKey, - coalesceMs: fold.coalesceMs, - now: (clock += 1000), - files: { "index.html": { before, after: serialize(doc) } }, - }), - ); - }; - const current = (element: TimelineElement) => { - const found = doc[element.id]; - if (!found) throw new Error(`${element.id} is not in the document`); - // The element a step hands over must describe what the document holds right now. - expect({ start: element.start, duration: element.duration }).toEqual({ - start: found.start, - duration: found.duration, - }); - return found; - }; - const ops: PlacementOps = { - split: async (element, at, fold) => { - record( - () => { - const target = current(element); - const end = target.start + target.duration; - doc[`${element.id}-split`] = { - start: at, - duration: end - at, - playbackStart: (target.playbackStart ?? 0) + (at - target.start), - }; - target.duration = at - target.start; - }, - "split", - fold, - ); - return true; - }, - remove: async (elements, fold) => { - record( - () => { - for (const element of elements) { - current(element); - delete doc[element.id]; - } - }, - "delete", - fold, - ); - return true; - }, - toast: vi.fn(), - }; - const resize = async (changes: TimelineGroupResizeChange[], fold: PlacementFold) => { - record( - () => { - for (const change of changes) { - const target = current(change.element); - target.start = change.start; - target.duration = change.duration; - if (change.playbackStart != null) target.playbackStart = change.playbackStart; - } - }, - "resize", - fold, - ); - }; - const move = async (edits: TimelineMoveEdit[], fold: PlacementFold) => { - record( - () => { - for (const edit of edits) { - const found = doc[edit.element.id]; - if (found) found.start = edit.updates.start; - else - doc[edit.element.id] = { start: edit.updates.start, duration: edit.element.duration }; - } - }, - "move", - fold, - ); - return true; - }; - return { - ops, - resize, - move, - doc: () => doc, - history: () => history, - undo: () => { - const undone = undoEditHistory( - history, - { "index.html": hashEditHistoryContent(serialize(doc)) }, - 0, - ); - if (!undone.ok) throw new Error(`undo refused: ${undone.reason}`); - return undone.filesToWrite["index.html"]; - }, - serializeInitial: () => serialize(initial), - }; -} - -function dragOf(element: TimelineElement, previewStart: number, previewTrack = 1) { - return { - element, - previewStart, - previewTrack, - insertRow: null, - } as unknown as DraggedClipState; -} - -function depsOf(elements: TimelineElement[], project: ReturnType) { - return { - elements, - trackOrder: [1], - updateElement: vi.fn(), - placementOps: project.ops, - onResizeElements: project.resize, - } as unknown as DragCommitDeps; -} - describe("commitPlacementDrop: one undo step for the move and every cut", () => { // Lane 1: a [0,4) and b [4,8) video (b reads its source from 1s at rate 1), dragged d [20,22). const a = clip("a", 0, 4); @@ -487,9 +333,10 @@ describe("runPlacementSteps failures", () => { it("names a partial apply and stops when a later step fails", async () => { const toast = vi.fn(); await runPlacementSteps(steps, { - ops: { split: async () => true, remove: async () => true, toast }, + ops: { split: async () => true, remove: async () => true, toast, reloadPreview: vi.fn() }, resize: vi.fn(), move: async () => false, + applyToStore: vi.fn(), }); expect(toast).toHaveBeenCalledWith("Overwrite partly applied, Undo restores it"); }); @@ -498,9 +345,10 @@ describe("runPlacementSteps failures", () => { const toast = vi.fn(); const move = vi.fn(async () => true); await runPlacementSteps(steps, { - ops: { split: async () => false, remove: async () => true, toast }, + ops: { split: async () => false, remove: async () => true, toast, reloadPreview: vi.fn() }, resize: vi.fn(), move, + applyToStore: vi.fn(), }); expect(toast).toHaveBeenCalledWith("Overwrite failed, nothing changed"); expect(move).not.toHaveBeenCalled(); @@ -510,12 +358,95 @@ describe("runPlacementSteps failures", () => { const toast = vi.fn(); vi.spyOn(console, "error").mockImplementation(() => undefined); await runPlacementSteps(steps, { - ops: { split: async () => true, remove: async () => true, toast }, + ops: { split: async () => true, remove: async () => true, toast, reloadPreview: vi.fn() }, resize: vi.fn(), move: async () => { throw new Error("write failed"); }, + applyToStore: vi.fn(), }); expect(toast).toHaveBeenCalledWith("Overwrite partly applied, Undo restores it"); }); }); + +// A drop reloads the preview exactly once, after every step lands, never once per step. +describe("runPlacementSteps: one reload per drop, after every step lands", () => { + it("reloads exactly once, after the move step, when the drop contains a remove", async () => { + const calls: string[] = []; + const reloadPreview = vi.fn(() => calls.push("reload")); + const remove = vi.fn(async () => { + calls.push("remove"); + return true; + }); + const move = vi.fn(async () => { + calls.push("move"); + return true; + }); + await runPlacementSteps( + [ + { kind: "remove", elements: [clip("a", 0, 4)] }, + { kind: "move", edits: [] }, + ], + { + ops: { split: async () => true, remove, toast: vi.fn(), reloadPreview }, + resize: vi.fn(), + move, + applyToStore: vi.fn(), + }, + ); + expect(calls).toEqual(["remove", "move", "reload"]); + expect(reloadPreview).toHaveBeenCalledTimes(1); + }); + + it("reloads exactly once when the drop contains a split", async () => { + const reloadPreview = vi.fn(); + const split = vi.fn(async () => true); + await runPlacementSteps( + [ + { kind: "split", element: clip("a", 0, 4), at: 2 }, + { kind: "move", edits: [] }, + ], + { + ops: { split, remove: async () => true, toast: vi.fn(), reloadPreview }, + resize: vi.fn(), + move: async () => true, + applyToStore: vi.fn(), + }, + ); + expect(reloadPreview).toHaveBeenCalledTimes(1); + }); + + it("never reloads a move/resize-only drop (a plain trim stays flash-free)", async () => { + const reloadPreview = vi.fn(); + await runPlacementSteps( + [ + { kind: "resize", changes: [] }, + { kind: "move", edits: [] }, + ], + { + ops: { split: async () => true, remove: async () => true, toast: vi.fn(), reloadPreview }, + resize: vi.fn(async () => undefined), + move: async () => true, + applyToStore: vi.fn(), + }, + ); + expect(reloadPreview).not.toHaveBeenCalled(); + }); + + it("reloads once when a step failed, because the store was already told the end state", async () => { + const reloadPreview = vi.fn(); + await runPlacementSteps( + [ + { kind: "remove", elements: [clip("a", 0, 4)] }, + { kind: "move", edits: [] }, + ], + { + ops: { split: async () => true, remove: async () => false, toast: vi.fn(), reloadPreview }, + resize: vi.fn(), + move: async () => true, + applyToStore: vi.fn(), + }, + ); + expect(reloadPreview).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/studio/src/player/components/timelinePlacementCommit.ts b/packages/studio/src/player/components/timelinePlacementCommit.ts index 05e4146eaa..f3a51300df 100644 --- a/packages/studio/src/player/components/timelinePlacementCommit.ts +++ b/packages/studio/src/player/components/timelinePlacementCommit.ts @@ -1,4 +1,5 @@ -import type { TimelineElement } from "../store/playerStore"; +import { usePlayerStore, type TimelineElement } from "../store/playerStore"; +import { holdTimelineManifests } from "../lib/timelineManifestHold"; import type { DraggedClipState } from "./timelineClipDragTypes"; import type { DragCommitDeps, TimelineMoveEdit } from "./timelineClipDragCommit"; import { canMoveTimelineElement } from "./timelineAuthoredMoveTarget"; @@ -21,6 +22,8 @@ export interface PlacementOps { split: (element: TimelineElement, at: number, fold: PlacementFold) => Promise; remove: (elements: TimelineElement[], fold: PlacementFold) => Promise; toast: (message: string) => void; + /** The one full-reload point for a drop; split/remove skip their own while folded. */ + reloadPreview: () => void; } export type PlacementStep = @@ -148,6 +151,8 @@ interface PlacementRunner { ops: PlacementOps; resize: (changes: TimelineGroupResizeChange[], fold: PlacementFold) => Promise | void; move: (edits: TimelineMoveEdit[], fold: PlacementFold) => Promise; + /** Puts the drop's end state in the store before any write starts. */ + applyToStore: (steps: readonly PlacementStep[]) => void; } function runStep(step: PlacementStep, fold: PlacementFold, run: PlacementRunner) { @@ -163,7 +168,87 @@ function runStep(step: PlacementStep, fold: PlacementFold, run: PlacementRunner) } } -/** Each step awaits the previous: the history fold needs every write to start from the last one's output. */ +/** The clip a split will add, under a stand-in id until the reload reports the real one. */ +function pendingSplitTail(el: TimelineElement, at: number): TimelineElement { + const playbackStart = hasSourcePlaybackOffset(el) + ? round3((el.playbackStart ?? 0) + (at - el.start) * (el.playbackRate ?? 1)) + : el.playbackStart; + return { + ...el, + id: `${el.id}~tail`, + key: `${keyOf(el)}~tail`, + domId: undefined, + start: at, + duration: round3(el.start + el.duration - at), + playbackStart, + }; +} + +function moveUpdate(edit: TimelineMoveEdit): Partial { + const written = + edit.persistTrack ?? + (edit.updates.track !== edit.element.track ? edit.updates.track : undefined); + return written == null ? edit.updates : { ...edit.updates, authoredTrack: written }; +} + +function resizeUpdate(change: TimelineGroupResizeChange): Partial { + return { + start: change.start, + duration: change.duration, + ...(change.playbackStart != null ? { playbackStart: change.playbackStart } : {}), + }; +} + +/** What one step changes in the store: keyed updates, removed keys, and clips it adds. */ +function stepEffects(step: PlacementStep): { + updates: Array<[string, Partial]>; + removed: string[]; + added: TimelineElement[]; +} { + switch (step.kind) { + case "remove": + return { updates: [], removed: step.elements.map(keyOf), added: [] }; + case "move": + return { + updates: step.edits.map((edit) => [keyOf(edit.element), moveUpdate(edit)]), + removed: [], + added: [], + }; + case "resize": + return { + updates: step.changes.map((change) => [keyOf(change.element), resizeUpdate(change)]), + removed: [], + added: [], + }; + case "split": + return { + updates: [[keyOf(step.element), { duration: round3(step.at - step.element.start) }]], + removed: [], + added: [pendingSplitTail(step.element, step.at)], + }; + } +} + +/** The drop's end state, written to the store in one go so no in-between state is ever shown. */ +function applyPlacementToStore(steps: readonly PlacementStep[]): void { + const removed = new Set(); + const updates = new Map>(); + const tails: TimelineElement[] = []; + for (const effects of steps.map(stepEffects)) { + for (const key of effects.removed) removed.add(key); + for (const [key, next] of effects.updates) updates.set(key, { ...updates.get(key), ...next }); + tails.push(...effects.added); + } + const { elements, setElements } = usePlayerStore.getState(); + setElements([ + ...elements + .filter((el) => !removed.has(keyOf(el))) + .map((el) => ({ ...el, ...updates.get(keyOf(el)) })), + ...tails, + ]); +} + +/** Steps run in order, each from the last write; preview manifests stay held until the final reload. */ export async function runPlacementSteps( steps: readonly PlacementStep[], run: PlacementRunner, @@ -172,20 +257,32 @@ export async function runPlacementSteps( coalesceKey: `clip-overwrite:${placementGestureSeq++}`, coalesceMs: Number.POSITIVE_INFINITY, }; - for (const [index, step] of steps.entries()) { - let applied = false; - try { - applied = (await runStep(step, fold, run)) !== false; - } catch (error) { - console.error("[Timeline] Overwrite step failed", error); + const release = holdTimelineManifests(); + try { + run.applyToStore(steps); + for (const [index, step] of steps.entries()) { + let applied = false; + try { + applied = (await runStep(step, fold, run)) !== false; + } catch (error) { + console.error("[Timeline] Overwrite step failed", error); + } + if (applied) continue; + run.ops.toast( + index > 0 + ? "Overwrite partly applied, Undo restores it" + : "Overwrite failed, nothing changed", + ); + // The store was told the drop's end state; only a reload puts back what disk holds. + run.ops.reloadPreview(); + return; } - if (applied) continue; - run.ops.toast( - index > 0 - ? "Overwrite partly applied, Undo restores it" - : "Overwrite failed, nothing changed", - ); - return; + // remove/split write to disk directly and skip their own reload while folded here. + if (steps.some((step) => step.kind === "remove" || step.kind === "split")) { + run.ops.reloadPreview(); + } + } finally { + release(); } } @@ -228,5 +325,6 @@ export function commitPlacementDrop( ops: placementOps, resize: (changes, fold) => onResizeElements(changes, fold), move, + applyToStore: applyPlacementToStore, }); } diff --git a/packages/studio/src/player/components/timelinePlacementMonotonic.test.ts b/packages/studio/src/player/components/timelinePlacementMonotonic.test.ts new file mode 100644 index 0000000000..89ffacdc56 --- /dev/null +++ b/packages/studio/src/player/components/timelinePlacementMonotonic.test.ts @@ -0,0 +1,155 @@ +// @vitest-environment happy-dom + +import React, { act, useEffect } from "react"; +import { createRoot } from "react-dom/client"; +import { runtimeProtocolMetadata } from "@hyperframes/core/runtime/protocol"; +import { afterEach, describe, expect, it } from "vitest"; +import { useTimelinePlayer } from "../hooks/useTimelinePlayer"; +import { usePlayerStore } from "../store/playerStore"; +import { commitPlacementDrop } from "./timelinePlacementCommit"; +import { clip, createFakeProject, depsOf, dragOf } from "./timelinePlacementTestHarness"; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +function Harness() { + useTimelinePlayer(); + useEffect(() => undefined); + return null; +} + +function mountPlayer() { + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => root.render(React.createElement(Harness))); + return () => act(() => root.unmount()); +} + +const manifestClip = (id: string, start: number, duration: number) => ({ + id, + label: id, + start, + duration, + track: 1, + kind: "element", + tagName: "div", + compositionId: null, + parentCompositionId: null, + compositionSrc: null, + assetUrl: null, +}); + +// The preview iframe keeps the pre-drop DOM until the drop's one reload, so its manifest lists the +// clip the drop already removed. It used to land mid-drop and put that clip back on the timeline. +function postStalePreviewManifest() { + window.dispatchEvent( + new MessageEvent("message", { + data: { + source: "hf-preview", + type: "timeline", + clips: [manifestClip("a", 0, 4), manifestClip("b", 4, 4), manifestClip("d", 20, 6)], + durationInFrames: 900, + fps: 30, + ...runtimeProtocolMetadata(30), + }, + }), + ); +} + +function recordStoreStates( + format: (el: { id: string; start: number; duration: number }) => string, +) { + const states: string[] = []; + const stop = usePlayerStore.subscribe((state) => + states.push(state.elements.map(format).join(" ")), + ); + return { states, stop }; +} + +afterEach(() => { + document.body.innerHTML = ""; + usePlayerStore.getState().reset(); +}); + +describe("a drop's timeline states never go back to an older one", () => { + it("keeps the removed clip gone when the old preview posts its manifest mid-drop", async () => { + const unmount = mountPlayer(); + const a = clip("a", 0, 4); + const b = clip("b", 4, 4); + const covering = clip("d", 20, 6); + const preDrop = [a, b, covering]; + usePlayerStore.getState().setElements(preDrop); + const project = createFakeProject({ + a: { start: 0, duration: 4 }, + b: { start: 4, duration: 4 }, + d: { start: 20, duration: 6 }, + }); + const remove = project.ops.remove; + project.ops.remove = async (elements, fold) => { + const done = await remove(elements, fold); + act(() => postStalePreviewManifest()); + return done; + }; + const { states, stop } = recordStoreStates((el) => `${el.id}@${el.start}`); + + await commitPlacementDrop( + dragOf(covering, 4), + depsOf(preDrop, project), + "overwrite", + project.move, + ); + stop(); + + expect(states.length).toBeGreaterThan(0); + expect(states.every((state) => !state.includes("b@"))).toBe(true); + expect(states.at(-1)).toContain("d@4"); + unmount(); + }); + + it("shows a split's head, the dropped clip and the new tail in the first state after the drop", async () => { + const a = clip("a", 0, 4); + const b = clip("b", 4, 4, { playbackStart: 1 }); + const dropped = clip("d", 20, 2); + const lane = [a, b, dropped]; + usePlayerStore.getState().setElements(lane); + const project = createFakeProject({ + a: { start: 0, duration: 4 }, + b: { start: 4, duration: 4, playbackStart: 1 }, + d: { start: 20, duration: 2 }, + }); + const { states, stop } = recordStoreStates((el) => `${el.id}@${el.start}+${el.duration}`); + + await commitPlacementDrop(dragOf(dropped, 1), depsOf(lane, project), "overwrite", project.move); + stop(); + + // d [1,3) inside a [0,4): head [0,1), tail [3,4), all in the very first store write. + expect(states[0]).toBe("a@0+1 b@4+4 d@1+2 a~tail@3+1"); + }); + + it("shows an insert's pushed clips, split head and tail in the first state after the drop", async () => { + const a = clip("a", 0, 4); + const b = clip("b", 4, 4); + const dropped = clip("d", 20, 2); + const lane = [a, b, dropped]; + usePlayerStore.getState().setElements(lane); + const project = createFakeProject({ + a: { start: 0, duration: 4 }, + b: { start: 4, duration: 4 }, + d: { start: 20, duration: 2 }, + }); + const { states, stop } = recordStoreStates((el) => `${el.id}@${el.start}+${el.duration}`); + + await commitPlacementDrop(dragOf(dropped, 2), depsOf(lane, project), "insert", project.move); + stop(); + + // d [2,4) inserted into a [0,4): head [0,2), tail [4,6), b pushed to [6,10). + expect(states[0]).toBe("a@0+2 b@6+4 d@2+2 a~tail@4+2"); + }); + + it("takes the preview's manifest again once the drop is over", () => { + const unmount = mountPlayer(); + act(() => postStalePreviewManifest()); + expect(usePlayerStore.getState().elements.map((el) => el.id)).toEqual(["a", "b", "d"]); + unmount(); + }); +}); diff --git a/packages/studio/src/player/components/timelinePlacementTestHarness.ts b/packages/studio/src/player/components/timelinePlacementTestHarness.ts new file mode 100644 index 0000000000..14fd74f834 --- /dev/null +++ b/packages/studio/src/player/components/timelinePlacementTestHarness.ts @@ -0,0 +1,162 @@ +import { expect, vi } from "vitest"; +import type { TimelineGroupResizeChange } from "../../hooks/useTimelineGroupEditing"; +import type { TimelineElement } from "../store/playerStore"; +import type { DragCommitDeps, TimelineMoveEdit } from "./timelineClipDragCommit"; +import type { DraggedClipState } from "./timelineClipDragTypes"; +import type { PlacementFold, PlacementOps } from "./timelinePlacementCommit"; +import { + buildEditHistoryEntry, + createEmptyEditHistory, + hashEditHistoryContent, + pushEditHistoryEntry, + undoEditHistory, + type EditHistoryState, +} from "../../utils/editHistory"; + +export function clip( + id: string, + start: number, + duration: number, + extra: Partial = {}, +) { + return { id, domId: id, tag: "video", start, duration, track: 1, ...extra } as TimelineElement; +} + +export interface Doc { + [id: string]: { start: number; duration: number; playbackStart?: number }; +} + +/** A tiny stand-in for the project file plus the history recorder, driven by the real reducer. */ +export function createFakeProject(initial: Doc) { + let doc: Doc = structuredClone(initial); + let history: EditHistoryState = createEmptyEditHistory(); + let clock = 0; + const serialize = (d: Doc) => + JSON.stringify(Object.entries(d).sort(([x], [y]) => (x < y ? -1 : 1))); + const record = (mutate: () => void, label: string, fold: PlacementFold) => { + const before = serialize(doc); + mutate(); + history = pushEditHistoryEntry( + history, + buildEditHistoryEntry({ + id: `e${clock}`, + projectId: "p", + label, + kind: "timeline", + coalesceKey: fold.coalesceKey, + coalesceMs: fold.coalesceMs, + now: (clock += 1000), + files: { "index.html": { before, after: serialize(doc) } }, + }), + ); + }; + const current = (element: TimelineElement) => { + const found = doc[element.id]; + if (!found) throw new Error(`${element.id} is not in the document`); + // The element a step hands over must describe what the document holds right now. + expect({ start: element.start, duration: element.duration }).toEqual({ + start: found.start, + duration: found.duration, + }); + return found; + }; + const ops: PlacementOps = { + split: async (element, at, fold) => { + record( + () => { + const target = current(element); + const end = target.start + target.duration; + doc[`${element.id}-split`] = { + start: at, + duration: end - at, + playbackStart: (target.playbackStart ?? 0) + (at - target.start), + }; + target.duration = at - target.start; + }, + "split", + fold, + ); + return true; + }, + remove: async (elements, fold) => { + record( + () => { + for (const element of elements) { + current(element); + delete doc[element.id]; + } + }, + "delete", + fold, + ); + return true; + }, + toast: vi.fn(), + reloadPreview: vi.fn(), + }; + const resize = async (changes: TimelineGroupResizeChange[], fold: PlacementFold) => { + record( + () => { + for (const change of changes) { + const target = current(change.element); + target.start = change.start; + target.duration = change.duration; + if (change.playbackStart != null) target.playbackStart = change.playbackStart; + } + }, + "resize", + fold, + ); + }; + const move = async (edits: TimelineMoveEdit[], fold: PlacementFold) => { + record( + () => { + for (const edit of edits) { + const found = doc[edit.element.id]; + if (found) found.start = edit.updates.start; + else + doc[edit.element.id] = { start: edit.updates.start, duration: edit.element.duration }; + } + }, + "move", + fold, + ); + return true; + }; + return { + ops, + resize, + move, + doc: () => doc, + history: () => history, + undo: () => { + const undone = undoEditHistory( + history, + { "index.html": hashEditHistoryContent(serialize(doc)) }, + 0, + ); + if (!undone.ok) throw new Error(`undo refused: ${undone.reason}`); + return undone.filesToWrite["index.html"]; + }, + serializeInitial: () => serialize(initial), + }; +} + +export function dragOf(element: TimelineElement, previewStart: number, previewTrack = 1) { + return { + element, + previewStart, + previewTrack, + insertRow: null, + } as unknown as DraggedClipState; +} + +export function depsOf(elements: TimelineElement[], project: ReturnType) { + return { + elements, + trackOrder: [1], + updateElement: vi.fn(), + placementOps: project.ops, + onResizeElements: project.resize, + } as unknown as DragCommitDeps; +} diff --git a/packages/studio/src/player/hooks/useTimelinePlayer.ts b/packages/studio/src/player/hooks/useTimelinePlayer.ts index 7794be1b04..f7fca7faed 100644 --- a/packages/studio/src/player/hooks/useTimelinePlayer.ts +++ b/packages/studio/src/player/hooks/useTimelinePlayer.ts @@ -40,6 +40,7 @@ import { normalizeToZones } from "../components/timelineZones"; import { applyPreviewAudioFlags, setPreviewPlaybackRate } from "../lib/timelineIframeHelpers"; import { scrubMusicAtSeek, stopScrubPreviewAudio } from "../lib/playbackScrub"; import { hasTimelinePerformanceFixtureLease } from "../lib/timelinePerformanceFixture"; +import { timelineManifestsHeld } from "../lib/timelineManifestHold"; import { applyCachedSourceDurations, probeMissingSourceDurations } from "../lib/mediaProbe"; import { shouldResumeForwardPlaybackAfterSeek, shouldStopAfterSeek } from "../lib/playbackSeek"; import { applyPreviewVariablesToUrl } from "../../hooks/previewVariablesStore"; @@ -64,13 +65,13 @@ export function useTimelinePlayer() { const { setIsPlaying, setCurrentTime, setDuration, requestTimelineReady, setElements } = usePlayerStore.getState(); - // The fixture lease belongs at this shared synchronization boundary so every - // iframe discovery path has the same owner for deciding whether it may write. + // The fixture lease and the drop's manifest hold belong at this shared synchronization + // boundary so every iframe discovery path has the same owner for deciding whether it may write. const syncTimelineElements = useCallback( // The lease guard adds one deliberate branch at the shared synchronization boundary. // fallow-ignore-next-line complexity (elements: TimelineElement[], nextDuration?: number) => { - if (hasTimelinePerformanceFixtureLease()) return; + if (hasTimelinePerformanceFixtureLease() || timelineManifestsHeld()) return; const state = usePlayerStore.getState(); const resolvedDuration = nextDuration ?? state.duration; // applyCachedSourceDurations re-applies the cached probe duration: re-derived diff --git a/packages/studio/src/player/lib/timelineManifestHold.ts b/packages/studio/src/player/lib/timelineManifestHold.ts new file mode 100644 index 0000000000..d1e0c5b739 --- /dev/null +++ b/packages/studio/src/player/lib/timelineManifestHold.ts @@ -0,0 +1,16 @@ +let holds = 0; + +/** While held, clip manifests from the preview are ignored; release() lets the next one in. */ +export function holdTimelineManifests(): () => void { + holds += 1; + let released = false; + return () => { + if (released) return; + released = true; + holds -= 1; + }; +} + +export function timelineManifestsHeld(): boolean { + return holds > 0; +}