From 5ce362299c72f7d188adca608ff907b2f9e5eb73 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 7 Jul 2026 23:46:50 -0400 Subject: [PATCH] refactor(studio): single-source the timeline stacking key + guard audio reorder The element stacking key (element.key ?? id) was recomputed in four places (reorder-intent generation, row ordering, the commit-time sibling lookup via a threaded keyOf param, and resolveTimelineMove). Any drift would silently break the sibling lookup and no-op the reorder. Route all of them through the existing getTimelineElementIdentity owner, share one toStackingOrderItem mapper between row ordering and reorder intent, and drop the keyOf parameter. Also enforce the audio side-effect invariant in the single mutation owner (applyTimelineStackingReorder): dragging an audio clip has no visual layer to restack, so it never writes z-index. Covered by a new hook test. --- .../src/hooks/timelineEditingHelpers.ts | 16 ++++++--- .../src/hooks/useTimelineEditing.test.tsx | 32 +++++++++++++++-- .../studio/src/hooks/useTimelineEditing.ts | 1 - .../src/player/components/timelineEditing.ts | 14 ++++---- .../player/components/timelineTrackOrder.ts | 36 ++++++------------- .../src/player/lib/timelineElementHelpers.ts | 2 +- 6 files changed, 60 insertions(+), 41 deletions(-) diff --git a/packages/studio/src/hooks/timelineEditingHelpers.ts b/packages/studio/src/hooks/timelineEditingHelpers.ts index de0619427..3f81db565 100644 --- a/packages/studio/src/hooks/timelineEditingHelpers.ts +++ b/packages/studio/src/hooks/timelineEditingHelpers.ts @@ -6,6 +6,7 @@ import { type TimelineStackingReorderIntent, } from "../player/components/timelineEditing"; import { computeReorderZValues, getElementZIndex } from "../player/lib/layerOrdering"; +import { getTimelineElementIdentity } from "../player/lib/timelineElementHelpers"; import { saveProjectFilesWithHistory } from "../utils/studioFileHistory"; import { selectedKeyframePercentagesForElement } from "../utils/keyframeSelection"; import type { EditHistoryKind } from "../utils/editHistory"; @@ -19,9 +20,10 @@ function isHTMLElement(element: Element | null): element is HTMLElement { * Resolve a timeline vertical move to a z-index stacking reorder and commit it * through the shared layers-panel reorder path. Reads live sibling z-index from * the preview DOM, remaps with the dup-preserving reorder math, and writes only - * z-index (never data-track-index). No-op when the move isn't a reorder or the - * live siblings can't be resolved. Extracted from StudioApp's timeline hook to - * keep it under the studio 600-LOC cap. + * z-index (never data-track-index). No-op when the move isn't a reorder, the + * dragged clip is audio (no visual layer to restack), or the live siblings can't + * be resolved. Extracted from StudioApp's timeline hook to keep it under the + * studio 600-LOC cap. */ export function applyTimelineStackingReorder(input: { element: TimelineElement; @@ -31,8 +33,10 @@ export function applyTimelineStackingReorder(input: { iframe: HTMLIFrameElement | null; activeCompPath: string | null; commit: TimelineZIndexReorderCommit | null | undefined; - keyOf: (element: TimelineElement) => string; }): void { + // Audio has no visual stacking; a vertical drag on it must never write z-index. + if (input.element.tag === "audio") return; + const intent = input.stackingReorder ?? (input.targetTrack !== input.element.track @@ -44,7 +48,9 @@ export function applyTimelineStackingReorder(input: { : null); if (intent == null || intent.fromIndex === intent.toIndex) return; - const siblingByKey = new Map(input.timelineElements.map((el) => [input.keyOf(el), el])); + const siblingByKey = new Map( + input.timelineElements.map((el) => [getTimelineElementIdentity(el), el]), + ); const orderedSiblings = intent.siblingKeys .map((key) => siblingByKey.get(key) ?? null) .filter((sibling): sibling is TimelineElement => sibling != null); diff --git a/packages/studio/src/hooks/useTimelineEditing.test.tsx b/packages/studio/src/hooks/useTimelineEditing.test.tsx index b427e9706..3990efbcb 100644 --- a/packages/studio/src/hooks/useTimelineEditing.test.tsx +++ b/packages/studio/src/hooks/useTimelineEditing.test.tsx @@ -49,12 +49,17 @@ function createPreviewIframe( return iframe; } -function timelineElement(input: { id: string; track: number; zIndex: number }): TimelineElement { +function timelineElement(input: { + id: string; + track: number; + zIndex: number; + tag?: string; +}): TimelineElement { return { id: input.id, domId: input.id, hfId: `hf-${input.id}`, - tag: "div", + tag: input.tag ?? "div", start: 0, duration: 2, track: input.track, @@ -235,6 +240,29 @@ describe("useTimelineEditing timeline z-index reorder", () => { unmount(); }); + it("never writes z-index when the dragged clip is audio (no visual layer)", async () => { + const iframe = createPreviewIframe([ + { id: "front", track: 0 }, + { id: "music", track: 1 }, + ]); + const front = timelineElement({ id: "front", track: 0, zIndex: 0 }); + const music = timelineElement({ id: "music", track: 1, zIndex: 0, tag: "audio" }); + const commit = vi.fn<(entries: ZIndexEntry[]) => void>(); + const { move, unmount } = renderTimelineEditingHook({ + timelineElements: [front, music], + iframe, + onZIndexCommit: commit, + }); + + await act(async () => { + await move(music, { start: music.start, track: front.track }); + }); + + expect(commit).not.toHaveBeenCalled(); + + unmount(); + }); + it("remaps distinct z-index values onto the reordered sibling group", async () => { const iframe = createPreviewIframe([ { id: "front", track: 0, style: "position: relative; z-index: 10" }, diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index 590e0e055..fed47ffbc 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -140,7 +140,6 @@ export function useTimelineEditing({ iframe: previewIframeRef.current, activeCompPath, commit: handleDomZIndexReorderCommitRef?.current, - keyOf: (el) => el.key ?? el.id, }); if (!startChanged) return; diff --git a/packages/studio/src/player/components/timelineEditing.ts b/packages/studio/src/player/components/timelineEditing.ts index 0eee5b91c..16f6ce7a8 100644 --- a/packages/studio/src/player/components/timelineEditing.ts +++ b/packages/studio/src/player/components/timelineEditing.ts @@ -1,6 +1,7 @@ import { formatTime } from "../lib/time"; import { roundToCenti } from "../../utils/rounding"; import { resolveContextOrder, resolveStackingContextKey } from "../lib/layerOrdering"; +import { getTimelineElementIdentity } from "../lib/timelineElementHelpers"; const roundToCentiseconds = roundToCenti; @@ -32,7 +33,7 @@ export interface TimelineStackingReorderIntent { siblingKeys: string[]; } -interface TimelineStackingOrderItem { +export interface TimelineStackingOrderItem { key: string; track: number; zIndex: number; @@ -41,9 +42,9 @@ interface TimelineStackingOrderItem { compositionAncestors: readonly string[]; } -function toStackingOrderItem(element: TimelineStackingElement): TimelineStackingOrderItem { +export function toStackingOrderItem(element: TimelineStackingElement): TimelineStackingOrderItem { return { - key: element.key ?? element.id, + key: getTimelineElementIdentity(element), track: element.track, zIndex: element.zIndex ?? 0, stackingContextId: element.stackingContextId ?? null, @@ -77,8 +78,9 @@ export function resolveTimelineStackingReorderByTargetTrack(args: { }): TimelineStackingReorderIntent | null { const orderedSiblings = resolveContextSiblings(args.element, args.elements); if (orderedSiblings.length <= 1) return null; - const draggedKey = args.element.key ?? args.element.id; - const fromIndex = orderedSiblings.findIndex((sibling) => sibling.key === draggedKey); + const fromIndex = orderedSiblings.findIndex( + (sibling) => sibling.key === getTimelineElementIdentity(args.element), + ); if (fromIndex < 0) return null; const toIndex = orderedSiblings.findIndex((sibling) => sibling.track === args.targetTrack); if (toIndex < 0) return null; @@ -178,7 +180,7 @@ export function resolveTimelineMove( // stacking context (top = front), rather than changing the raw track number. if (input.stackingElement && input.stackingElements) { const orderedSiblings = resolveContextSiblings(input.stackingElement, input.stackingElements); - const draggedKey = input.stackingElement.key ?? input.stackingElement.id; + const draggedKey = getTimelineElementIdentity(input.stackingElement); const fromIndex = orderedSiblings.findIndex((sibling) => sibling.key === draggedKey); if (fromIndex >= 0 && orderedSiblings.length > 1) { const toIndex = clamp(fromIndex + deltaTrack, 0, orderedSiblings.length - 1); diff --git a/packages/studio/src/player/components/timelineTrackOrder.ts b/packages/studio/src/player/components/timelineTrackOrder.ts index dc4c151e4..f678e35eb 100644 --- a/packages/studio/src/player/components/timelineTrackOrder.ts +++ b/packages/studio/src/player/components/timelineTrackOrder.ts @@ -1,39 +1,23 @@ import { type TimelineElement } from "../store/playerStore"; -import { - resolveContextOrder, - resolveStackingContextKey, - type ContextOrderItem, -} from "../lib/layerOrdering"; +import { resolveContextOrder, resolveStackingContextKey } from "../lib/layerOrdering"; +import { getTimelineElementIdentity } from "../lib/timelineElementHelpers"; +import { toStackingOrderItem, type TimelineStackingOrderItem } from "./timelineEditing"; /** * Pure timeline track-ordering logic. Timeline rows are ordered by scoped * stacking (z-index per stacking context, top = front), with data-track-index * used only to split time-overlapping clips of equal rank onto separate rows. * Extracted from Timeline.tsx to keep the component under the studio 600-LOC cap. + * + * Key derivation and stacking-descriptor mapping are owned by timelineEditing so + * the row order here and the reorder intent there interpret every element the + * same way. */ -interface TimelineTrackOrderItem extends ContextOrderItem { - key: string; - track: number; - start: number; - duration: number; -} - -function getTimelineElementKey(element: TimelineElement): string { - return element.key ?? element.id; -} +type TimelineTrackOrderItem = TimelineStackingOrderItem & { start: number; duration: number }; function toTimelineTrackOrderItem(element: TimelineElement): TimelineTrackOrderItem { - return { - key: getTimelineElementKey(element), - track: element.track, - start: element.start, - duration: element.duration, - zIndex: element.zIndex ?? 0, - stackingContextId: element.stackingContextId ?? null, - parentCompositionId: element.parentCompositionId ?? null, - compositionAncestors: element.compositionAncestors ?? [], - }; + return { ...toStackingOrderItem(element), start: element.start, duration: element.duration }; } function timelineElementsOverlap( @@ -51,7 +35,7 @@ function trackFrontOrderIndex( for (const element of elements) { orderIndex = Math.min( orderIndex, - orderIndexByKey.get(getTimelineElementKey(element)) ?? Number.POSITIVE_INFINITY, + orderIndexByKey.get(getTimelineElementIdentity(element)) ?? Number.POSITIVE_INFINITY, ); } return orderIndex; diff --git a/packages/studio/src/player/lib/timelineElementHelpers.ts b/packages/studio/src/player/lib/timelineElementHelpers.ts index 74780f429..f58253082 100644 --- a/packages/studio/src/player/lib/timelineElementHelpers.ts +++ b/packages/studio/src/player/lib/timelineElementHelpers.ts @@ -245,7 +245,7 @@ export function buildTimelineElementIdentity(params: { return { id, key }; } -export function getTimelineElementIdentity(element: TimelineElement): string { +export function getTimelineElementIdentity(element: { key?: string | null; id: string }): string { return element.key ?? element.id; }