From 6080f5ad3e57af63578606838692f6da9027b267 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Thu, 9 Jul 2026 16:53:26 -0400 Subject: [PATCH] fix(studio): propagate z-index reorder save failures and drop dead targetTrack handleDomZIndexReorderCommit no longer swallows per-entry save failures: it settles every patch, and on any rejection rolls back the eager DOM z-index/position and the optimistic store zIndex before rejecting, so a failed save cannot leave the UI showing a stacking order that never persisted or let an ordered-after timing write proceed. Also removes the dead targetTrack parameter threaded through the timeline edit helpers; vertical placement is owned by the z-index intent. --- .../src/hooks/timelineEditingHelpers.test.ts | 2 - .../src/hooks/timelineEditingHelpers.ts | 8 +- .../src/hooks/useElementLifecycleOps.ts | 38 ++++- .../src/hooks/useTimelineEditing.test.tsx | 130 ++++++++++++++++++ .../studio/src/hooks/useTimelineEditing.ts | 1 - .../src/hooks/useTimelineEditingTypes.ts | 1 + 6 files changed, 168 insertions(+), 12 deletions(-) diff --git a/packages/studio/src/hooks/timelineEditingHelpers.test.ts b/packages/studio/src/hooks/timelineEditingHelpers.test.ts index 5af7db737..0d31cb50c 100644 --- a/packages/studio/src/hooks/timelineEditingHelpers.test.ts +++ b/packages/studio/src/hooks/timelineEditingHelpers.test.ts @@ -39,7 +39,6 @@ describe("applyTimelineStackingReorder", () => { applyTimelineStackingReorder({ element: el({ id: "chip", tag: "div" }), - targetTrack: 0, stackingReorder: { contextKey: "scene", placement: { type: "above", layerId: "layer:scene:x" }, @@ -76,7 +75,6 @@ describe("applyTimelineStackingReorder", () => { applyTimelineStackingReorder({ element: el({ id: "track", tag: "audio" }), - targetTrack: 0, stackingReorder: { contextKey: "main", placement: { type: "above", layerId: "layer:main:x" }, diff --git a/packages/studio/src/hooks/timelineEditingHelpers.ts b/packages/studio/src/hooks/timelineEditingHelpers.ts index 3f2d9a15b..d2414dd45 100644 --- a/packages/studio/src/hooks/timelineEditingHelpers.ts +++ b/packages/studio/src/hooks/timelineEditingHelpers.ts @@ -33,7 +33,6 @@ function isHTMLElement(element: Element | null): element is HTMLElement { // fallow-ignore-next-line complexity export function applyTimelineStackingReorder(input: { element: TimelineElement; - targetTrack: number; stackingReorder: TimelineStackingReorderIntent | null | undefined; timelineElements: readonly TimelineElement[]; iframe: HTMLIFrameElement | null; @@ -89,12 +88,7 @@ export function applyTimelineStackingReorder(input: { } if (commitEntries.length === 0) return Promise.resolve(); - const persisted = input.commit?.(commitEntries) ?? Promise.resolve(); - const store = usePlayerStore.getState(); - for (const entry of commitEntries) { - store.updateElement(entry.key, { zIndex: entry.zIndex, hasExplicitZIndex: true }); - } - return persisted; + return input.commit?.(commitEntries) ?? Promise.resolve(); } /** diff --git a/packages/studio/src/hooks/useElementLifecycleOps.ts b/packages/studio/src/hooks/useElementLifecycleOps.ts index 05f5e3813..e59e1b042 100644 --- a/packages/studio/src/hooks/useElementLifecycleOps.ts +++ b/packages/studio/src/hooks/useElementLifecycleOps.ts @@ -160,6 +160,7 @@ export function useElementLifecycleOps({ // persistDomEditOperations → onTrySdkPersist, so it is already SDK-cut-over as setStyle. // No SDK reorder/reparent op exists; DOM sibling order stays server-authoritative if ever needed. const handleDomZIndexReorderCommit = useCallback( + // fallow-ignore-next-line complexity ( entries: Array<{ element: HTMLElement; @@ -168,6 +169,7 @@ export function useElementLifecycleOps({ selector?: string; selectorIndex?: number; sourceFile: string; + key?: string; }>, ) => { if (entries.length === 0) return Promise.resolve(); @@ -178,8 +180,15 @@ export function useElementLifecycleOps({ ); const coalesceKey = `z-reorder:${entries.map((e) => e.id ?? e.selector ?? e.element.getAttribute("data-hf-id") ?? "el").join(":")}`; const saves: Array> = []; + const rollbacks: Array<() => void> = []; for (let i = 0; i < entries.length; i++) { const entry = entries[i]; + const priorZIndex = entry.element.style.zIndex; + const priorPosition = entry.element.style.position; + const priorStoreEntry = entry.key + ? usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === entry.key) + : undefined; + let positionChanged = false; entry.element.style.zIndex = String(entry.zIndex); const patches: Array<{ type: "inline-style"; property: string; value: string }> = [ { type: "inline-style", property: "z-index", value: String(entry.zIndex) }, @@ -188,11 +197,27 @@ export function useElementLifecycleOps({ const win = entry.element.ownerDocument?.defaultView; if (win && win.getComputedStyle(entry.element).position === "static") { entry.element.style.position = "relative"; + positionChanged = true; patches.push({ type: "inline-style", property: "position", value: "relative" }); } } catch { /* cross-origin or detached — skip */ } + if (entry.key) { + usePlayerStore + .getState() + .updateElement(entry.key, { zIndex: entry.zIndex, hasExplicitZIndex: true }); + } + rollbacks.push(() => { + entry.element.style.zIndex = priorZIndex; + if (positionChanged) entry.element.style.position = priorPosition; + if (entry.key && priorStoreEntry) { + usePlayerStore.getState().updateElement(entry.key, { + zIndex: priorStoreEntry.zIndex, + hasExplicitZIndex: priorStoreEntry.hasExplicitZIndex, + }); + } + }); saves.push( commitPositionPatchToHtml( { @@ -209,12 +234,21 @@ export function useElementLifecycleOps({ coalesceKey, skipRefresh: i < entries.length - 1, }, - ).catch(() => undefined), + ), ); } // Resolves once every z-index patch is persisted so a same-file timing write // can be ordered after it (see applyTimelineStackingReorder callers). - return Promise.all(saves).then(() => undefined); + return Promise.allSettled(saves).then((settled) => { + const rejected = settled.find( + (result): result is PromiseRejectedResult => result.status === "rejected", + ); + if (rejected) { + for (const rollback of rollbacks) rollback(); + return Promise.reject(rejected.reason); + } + return undefined; + }); }, [commitPositionPatchToHtml, onReorderShadow], ); diff --git a/packages/studio/src/hooks/useTimelineEditing.test.tsx b/packages/studio/src/hooks/useTimelineEditing.test.tsx index e5b56a544..daa256a04 100644 --- a/packages/studio/src/hooks/useTimelineEditing.test.tsx +++ b/packages/studio/src/hooks/useTimelineEditing.test.tsx @@ -493,6 +493,136 @@ describe("useTimelineEditing timeline z-index reorder", () => { unmount(); }); + it("rejects and rolls back DOM and store z-index changes when a reorder save fails", async () => { + const iframe = createPreviewIframe([ + { id: "front", track: 0, style: "position: relative; z-index: 7" }, + { id: "back", track: 1, style: "position: static" }, + ]); + const front = timelineElement({ id: "front", track: 0, zIndex: 7 }); + const back = timelineElement({ id: "back", track: 1, zIndex: 0 }); + usePlayerStore.getState().setElements([ + { ...front, hasExplicitZIndex: true }, + { ...back, hasExplicitZIndex: false }, + ]); + const saveError = new Error("save failed"); + const commitPositionPatchToHtml = vi + .fn<(...args: unknown[]) => Promise>() + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(saveError); + const { move, unmount } = renderTimelineEditingHookWithLifecycle({ + timelineElements: [front, back], + iframe, + commitPositionPatchToHtml, + }); + const doc = iframe.contentDocument; + if (!doc) throw new Error("Expected iframe document"); + const frontElement = doc.getElementById("front") as HTMLElement | null; + const backElement = doc.getElementById("back") as HTMLElement | null; + if (!frontElement || !backElement) throw new Error("Expected reordered elements"); + + let rejection: unknown; + await act(async () => { + try { + await move(back, { + start: back.start, + track: back.track, + stackingReorder: { + contextKey: "root", + placement: { type: "above", layerId: "front" }, + zIndexChanges: [ + { key: "front", zIndex: 2 }, + { key: "back", zIndex: 5 }, + ], + }, + }); + } catch (error) { + rejection = error; + } + await flushAsyncWork(); + }); + + expect(rejection).toBe(saveError); + expect(frontElement.style.zIndex).toBe("7"); + expect(frontElement.style.position).toBe("relative"); + expect(backElement.style.zIndex).toBe(""); + expect(backElement.style.position).toBe("static"); + const storeEntries = usePlayerStore.getState().elements; + expect(storeEntries.find((entry) => entry.id === "front")).toMatchObject({ + zIndex: 7, + hasExplicitZIndex: true, + }); + expect(storeEntries.find((entry) => entry.id === "back")).toMatchObject({ + zIndex: 0, + hasExplicitZIndex: false, + }); + + unmount(); + }); + + it("waits for every lifecycle z-index save before resolving a reorder", async () => { + const iframe = createPreviewIframe([ + { id: "front", track: 0, style: "position: relative; z-index: 1" }, + { id: "back", track: 1, style: "position: relative; z-index: 0" }, + ]); + const front = timelineElement({ id: "front", track: 0, zIndex: 1 }); + const back = timelineElement({ id: "back", track: 1, zIndex: 0 }); + let releaseFirst!: () => void; + let releaseSecond!: () => void; + const firstSave = new Promise((resolve) => { + releaseFirst = resolve; + }); + const secondSave = new Promise((resolve) => { + releaseSecond = resolve; + }); + const commitPositionPatchToHtml = vi + .fn<(...args: unknown[]) => Promise>() + .mockReturnValueOnce(firstSave) + .mockReturnValueOnce(secondSave); + const { move, unmount } = renderTimelineEditingHookWithLifecycle({ + timelineElements: [front, back], + iframe, + commitPositionPatchToHtml, + }); + let settled = false; + + let movePromise!: Promise; + await act(async () => { + movePromise = move(back, { + start: back.start, + track: back.track, + stackingReorder: { + contextKey: "root", + placement: { type: "above", layerId: "front" }, + zIndexChanges: [ + { key: "front", zIndex: 2 }, + { key: "back", zIndex: 3 }, + ], + }, + }).then(() => { + settled = true; + }); + await flushAsyncWork(); + }); + + expect(commitPositionPatchToHtml).toHaveBeenCalledTimes(2); + expect(settled).toBe(false); + + await act(async () => { + releaseFirst(); + await flushAsyncWork(); + }); + expect(settled).toBe(false); + + await act(async () => { + releaseSecond(); + await movePromise; + await flushAsyncWork(); + }); + expect(settled).toBe(true); + + unmount(); + }); + it("keeps horizontal-only drag on the timing and GSAP shift path without z-index writes", async () => { const iframe = createPreviewIframe([{ id: "clip", track: 0 }]); const clip = timelineElement({ id: "clip", track: 0, zIndex: 0 }); diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index 9dcb59e71..b5ca68e0c 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -136,7 +136,6 @@ export function useTimelineEditing({ const reorderDone = applyTimelineStackingReorder({ element, - targetTrack: updates.track, stackingReorder: updates.stackingReorder, timelineElements, iframe: previewIframeRef.current, diff --git a/packages/studio/src/hooks/useTimelineEditingTypes.ts b/packages/studio/src/hooks/useTimelineEditingTypes.ts index 0bd0700c1..b8cc2c753 100644 --- a/packages/studio/src/hooks/useTimelineEditingTypes.ts +++ b/packages/studio/src/hooks/useTimelineEditingTypes.ts @@ -20,6 +20,7 @@ export type TimelineZIndexReorderCommit = ( selector?: string; selectorIndex?: number; sourceFile: string; + key?: string; }>, ) => Promise;