From 3c6c1d3f2778a2be912e05b42f094b2c3e4a3b3d Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Thu, 9 Jul 2026 15:05:02 -0400 Subject: [PATCH] refactor(studio): single-source timeline id-resolution and resize-clamp math Extract resolveTimelineIdForSelection so DOM-to-timeline id mapping lives in one place with a single sourceFile / activeCompPath / index.html fallback, fixing a sub-composition selection that previously diverged between callers. Extract shared start-trim delta helpers used by both single-clip and group resize, and remove the never-called refreshDomEditGroupSelectionsFromPreview. --- packages/studio/src/hooks/useDomSelection.ts | 101 ++++-------------- .../hooks/useTimelineSelectionPreviewSync.ts | 21 +--- .../src/player/components/timelineEditing.ts | 26 ++--- .../player/components/timelineGroupEditing.ts | 77 ++++++++++--- .../studio/src/utils/studioHelpers.test.ts | 31 ++++++ packages/studio/src/utils/studioHelpers.ts | 22 ++++ 6 files changed, 149 insertions(+), 129 deletions(-) diff --git a/packages/studio/src/hooks/useDomSelection.ts b/packages/studio/src/hooks/useDomSelection.ts index 423d4b70a..346432455 100644 --- a/packages/studio/src/hooks/useDomSelection.ts +++ b/packages/studio/src/hooks/useDomSelection.ts @@ -4,11 +4,7 @@ import { getAllPreviewTargetsFromPointer, getPreviewTargetFromPointer, } from "../utils/studioPreviewHelpers"; -import { - findMatchingTimelineElementId, - findTimelineIdByAncestor, - type RightPanelTab, -} from "../utils/studioHelpers"; +import { resolveTimelineIdForSelection, type RightPanelTab } from "../utils/studioHelpers"; import { domEditSelectionsTargetSame, domEditSelectionInGroup, @@ -96,7 +92,6 @@ export interface UseDomSelectionReturn { ) => Promise; handleTimelineElementSelect: (element: TimelineElement | null) => Promise; refreshDomEditSelectionFromPreview: (selection: DomEditSelection) => Promise; - refreshDomEditGroupSelectionsFromPreview: (selections: DomEditSelection[]) => Promise; applyMarqueeSelection: (selections: DomEditSelection[], additive: boolean) => void; } @@ -208,20 +203,24 @@ export function useDomSelection({ setRightCollapsed(false); setRightPanelTab("design"); } - const nextSelectedTimelineId = - findMatchingTimelineElementId(nextSelection, timelineElements) ?? - findTimelineIdByAncestor( - nextSelection.element, - timelineElements, - nextSelection.sourceFile || "index.html", - ); + const nextSelectedTimelineId = resolveTimelineIdForSelection( + nextSelection, + timelineElements, + activeCompPath, + ); setSelectedTimelineElementId(nextSelectedTimelineId); return; } setSelectedTimelineElementId(null); }, - [setSelectedTimelineElementId, timelineElements, setRightCollapsed, setRightPanelTab], + [ + setSelectedTimelineElementId, + timelineElements, + setRightCollapsed, + setRightPanelTab, + activeCompPath, + ], ); const clearDomSelection = useCallback(() => { @@ -401,55 +400,6 @@ export function useDomSelection({ [activeCompPath, applyDomSelection, buildDomSelectionFromTarget, previewIframeRef], ); - const refreshDomEditGroupSelectionsFromPreview = useCallback( - // fallow-ignore-next-line complexity - async (selections: DomEditSelection[]) => { - const iframe = previewIframeRef.current; - let doc: Document | null = null; - try { - doc = iframe?.contentDocument ?? null; - } catch { - return; - } - if (!doc) return; - - const nextGroup: DomEditSelection[] = []; - for (const selection of selections) { - const element = findElementForSelection(doc, selection, activeCompPath); - if (!element) continue; - const nextSelection = await buildDomSelectionFromTarget(element); - if (nextSelection) nextGroup.push(nextSelection); - } - if (nextGroup.length === 0) return; - - const currentSelection = domEditSelectionRef.current; - const nextSelection = - nextGroup.find((selection) => domEditSelectionsTargetSame(selection, currentSelection)) ?? - nextGroup[0] ?? - null; - - domEditSelectionRef.current = nextSelection; - domEditGroupSelectionsRef.current = nextGroup; - setDomEditSelection(nextSelection); - setDomEditGroupSelections(nextGroup); - - if (nextSelection) { - setSelectedTimelineElementId( - findMatchingTimelineElementId(nextSelection, timelineElements), - ); - } else { - setSelectedTimelineElementId(null); - } - }, - [ - activeCompPath, - buildDomSelectionFromTarget, - setSelectedTimelineElementId, - timelineElements, - previewIframeRef, - ], - ); - // ── Effects ── // Clear hover unconditionally on composition/project/preview change @@ -531,22 +481,14 @@ export function useDomSelection({ domEditGroupSelectionsRef.current = nextGroup; setDomEditSelection(nextSelection); setDomEditGroupSelections(nextGroup); - const nextTimelineId = - findMatchingTimelineElementId(nextSelection, timelineElements) ?? - findTimelineIdByAncestor( - nextSelection.element, - timelineElements, - nextSelection.sourceFile || "index.html", - ); + const nextTimelineId = resolveTimelineIdForSelection( + nextSelection, + timelineElements, + activeCompPath, + ); const nextTimelineIds = nextGroup - .map( - (selection) => - findMatchingTimelineElementId(selection, timelineElements) ?? - findTimelineIdByAncestor( - selection.element, - timelineElements, - selection.sourceFile || "index.html", - ), + .map((selection) => + resolveTimelineIdForSelection(selection, timelineElements, activeCompPath), ) .filter((id): id is string => Boolean(id)); if (nextTimelineIds.length > 0) { @@ -555,7 +497,7 @@ export function useDomSelection({ setSelectedTimelineElementId(null); } }, - [applyDomSelection, timelineElements, setSelectedTimelineElementId], + [applyDomSelection, timelineElements, setSelectedTimelineElementId, activeCompPath], ); // Disabled inspector effect @@ -592,7 +534,6 @@ export function useDomSelection({ buildDomSelectionForTimelineElement, handleTimelineElementSelect, refreshDomEditSelectionFromPreview, - refreshDomEditGroupSelectionsFromPreview, applyMarqueeSelection, }; } diff --git a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts index 83e710597..71861e529 100644 --- a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts +++ b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts @@ -1,7 +1,7 @@ import { useEffect, useMemo } from "react"; import type { TimelineElement } from "../player"; import type { DomEditSelection } from "../components/editor/domEditing"; -import { findMatchingTimelineElementId, findTimelineIdByAncestor } from "../utils/studioHelpers"; +import { resolveTimelineIdForSelection } from "../utils/studioHelpers"; interface UseTimelineSelectionPreviewSyncParams { selectedElementId: string | null; @@ -26,21 +26,6 @@ function orderSelectedIds(ids: Set, anchor: string | null): string[] { return [anchor, ...ordered.filter((id) => id !== anchor)]; } -function selectionTimelineId( - selection: DomEditSelection, - timelineElements: TimelineElement[], - activeCompPath: string | null, -): string | null { - return ( - findMatchingTimelineElementId(selection, timelineElements) ?? - findTimelineIdByAncestor( - selection.element, - timelineElements, - selection.sourceFile || activeCompPath || "index.html", - ) - ); -} - function selectionIdsMatch(currentIds: string[], selectedIds: string[]): boolean { if (currentIds.length !== selectedIds.length) return false; const selected = new Set(selectedIds); @@ -72,7 +57,9 @@ export function useTimelineSelectionPreviewSync({ ? [domEditSelection] : []; const currentIds = currentSelections - .map((selection) => selectionTimelineId(selection, timelineElements, activeCompPath)) + .map((selection) => + resolveTimelineIdForSelection(selection, timelineElements, activeCompPath), + ) .filter((id): id is string => Boolean(id)); if (selectedIds.length === 0) { diff --git a/packages/studio/src/player/components/timelineEditing.ts b/packages/studio/src/player/components/timelineEditing.ts index 0f07e072f..1ec873812 100644 --- a/packages/studio/src/player/components/timelineEditing.ts +++ b/packages/studio/src/player/components/timelineEditing.ts @@ -4,7 +4,11 @@ import type { StackingTimelineLayer, TimelineLayerId } from "./timelineTrackOrde import { resolveTimelineLayerStackingMove } from "./timelineLayerDrag"; import type { TimelineStackingElement, TimelineStackingReorderIntent } from "./timelineStacking"; -import { resolveTimelineMinDuration } from "./timelineGroupEditing"; +import { + applyClipStartTrimDelta, + clipStartTrimDeltaBounds, + resolveTimelineMinDuration, +} from "./timelineGroupEditing"; export { clampTimelineGroupResizeDelta, @@ -220,23 +224,15 @@ export function resolveTimelineResize( }; } - const playbackRate = Math.max(0.1, input.playbackRate ?? 1); - const maxLeftExtensionFromMedia = - input.playbackStart != null ? input.playbackStart / playbackRate : Number.POSITIVE_INFINITY; - const minDelta = -Math.min(input.start - input.minStart, maxLeftExtensionFromMedia); - const maxDelta = input.duration - minDuration; + const { minDelta, maxDelta } = clipStartTrimDeltaBounds(input, input.minStart, minDuration); const clampedDelta = clamp(deltaTime, minDelta, maxDelta); - const nextStart = roundToCentiseconds(input.start + clampedDelta); - const nextDuration = roundToCentiseconds(input.duration - clampedDelta); - const nextPlaybackStart = - input.playbackStart != null - ? roundToCentiseconds(Math.max(0, input.playbackStart + clampedDelta * playbackRate)) - : undefined; + const trimmed = applyClipStartTrimDelta(input, clampedDelta); return { - start: nextStart, - duration: nextDuration, - playbackStart: nextPlaybackStart, + start: roundToCentiseconds(trimmed.start), + duration: roundToCentiseconds(trimmed.duration), + playbackStart: + trimmed.playbackStart != null ? roundToCentiseconds(trimmed.playbackStart) : undefined, }; } diff --git a/packages/studio/src/player/components/timelineGroupEditing.ts b/packages/studio/src/player/components/timelineGroupEditing.ts index 11de9c8ed..65533df13 100644 --- a/packages/studio/src/player/components/timelineGroupEditing.ts +++ b/packages/studio/src/player/components/timelineGroupEditing.ts @@ -15,6 +15,58 @@ export function resolveTimelineMinDuration(minDuration?: number): number { return Math.max(ABSOLUTE_TIMELINE_MIN_DURATION, minDuration ?? DEFAULT_TIMELINE_MIN_DURATION); } +/** Playback rate never drops to zero (would make media-in-point math divide by ~0). */ +function resolveTimelinePlaybackRate(rate?: number): number { + return Math.max(0.1, rate ?? 1); +} + +interface TimelineStartTrimClip { + start: number; + duration: number; + playbackStart?: number; + playbackRate?: number; +} + +/** + * Delta bounds for trimming a clip's START edge (shared by single-clip and group + * resize). Left-bounded by how far the start can move toward `minStart` and by the + * media in-point (`playbackStart / playbackRate`); right-bounded by `minDuration`. + * Returned deltas are unrounded — callers round with their own centisecond helper. + */ +export function clipStartTrimDeltaBounds( + clip: TimelineStartTrimClip, + minStart: number, + minDuration: number, +): { minDelta: number; maxDelta: number } { + const playbackRate = resolveTimelinePlaybackRate(clip.playbackRate); + const maxLeftExtensionFromMedia = + clip.playbackStart != null ? clip.playbackStart / playbackRate : Number.POSITIVE_INFINITY; + return { + minDelta: -Math.min(clip.start - minStart, maxLeftExtensionFromMedia), + maxDelta: clip.duration - minDuration, + }; +} + +/** + * Apply a start-edge delta to one clip (unrounded): moves the start, shrinks the + * duration by the same amount, and shifts the media in-point by the delta scaled to + * the playback rate (clamped at 0). + */ +export function applyClipStartTrimDelta( + clip: TimelineStartTrimClip, + delta: number, +): { start: number; duration: number; playbackStart?: number } { + const playbackRate = resolveTimelinePlaybackRate(clip.playbackRate); + return { + start: clip.start + delta, + duration: clip.duration - delta, + playbackStart: + clip.playbackStart != null + ? Math.max(0, clip.playbackStart + delta * playbackRate) + : undefined, + }; +} + export interface TimelineGroupTimingMember { start: number; duration: number; @@ -70,17 +122,10 @@ export function clampTimelineGroupResizeDelta( return roundTimelineTime(Math.max(rawDelta, minDelta)); } - const minDelta = Math.max( - ...members.map((member) => { - const playbackRate = Math.max(0.1, member.playbackRate ?? 1); - const maxLeftExtensionFromMedia = - member.playbackStart != null - ? member.playbackStart / playbackRate - : Number.POSITIVE_INFINITY; - return -Math.min(member.start, maxLeftExtensionFromMedia); - }), - ); - const maxDelta = Math.min(...members.map((member) => member.duration - minDuration)); + // Rigid group: the applied delta is bounded by the most-constrained member. + const bounds = members.map((member) => clipStartTrimDeltaBounds(member, 0, minDuration)); + const minDelta = Math.max(...bounds.map((b) => b.minDelta)); + const maxDelta = Math.min(...bounds.map((b) => b.maxDelta)); return roundTimelineTime(clamp(rawDelta, minDelta, maxDelta)); } @@ -102,14 +147,12 @@ export function resolveTimelineGroupResize( }; } - const playbackRate = Math.max(0.1, member.playbackRate ?? 1); + const trimmed = applyClipStartTrimDelta(member, delta); return { - start: roundTimelineTime(member.start + delta), - duration: roundTimelineTime(member.duration - delta), + start: roundTimelineTime(trimmed.start), + duration: roundTimelineTime(trimmed.duration), playbackStart: - member.playbackStart != null - ? roundTimelineTime(Math.max(0, member.playbackStart + delta * playbackRate)) - : undefined, + trimmed.playbackStart != null ? roundTimelineTime(trimmed.playbackStart) : undefined, }; }), }; diff --git a/packages/studio/src/utils/studioHelpers.test.ts b/packages/studio/src/utils/studioHelpers.test.ts index c50abc548..1a4c0f29a 100644 --- a/packages/studio/src/utils/studioHelpers.test.ts +++ b/packages/studio/src/utils/studioHelpers.test.ts @@ -4,6 +4,7 @@ import { describe, expect, it } from "vitest"; import { findMatchingTimelineElementId, findTimelineIdByAncestor, + resolveTimelineIdForSelection, resolveTimelineSelectionSeekTime, } from "./studioHelpers"; @@ -72,3 +73,33 @@ describe("findTimelineIdByAncestor", () => { expect(findTimelineIdByAncestor(child, [], "index.html")).toBe(null); }); }); + +describe("resolveTimelineIdForSelection", () => { + const el = (over: Record) => + ({ id: "x", start: 0, duration: 1, track: 0, tag: "div", ...over }) as never; + + it("resolves an ancestor clip against activeCompPath when the selection has no sourceFile", () => { + // #card (a clip in a sub-composition) > .leaf (selected, not itself a clip) + const card = document.createElement("div"); + card.id = "card"; + const leaf = document.createElement("span"); + leaf.className = "leaf"; + card.appendChild(leaf); + + const els = [ + el({ + id: "card", + domId: "card", + key: "comps/panel.html#card", + sourceFile: "comps/panel.html", + }), + ]; + const selection = { element: leaf } as never; + + // Falling back to the active comp matches; the old index.html-only fallback would miss. + expect(resolveTimelineIdForSelection(selection, els, "comps/panel.html")).toBe( + "comps/panel.html#card", + ); + expect(resolveTimelineIdForSelection(selection, els, null)).toBe(null); + }); +}); diff --git a/packages/studio/src/utils/studioHelpers.ts b/packages/studio/src/utils/studioHelpers.ts index fc5459bc2..dc72837e6 100644 --- a/packages/studio/src/utils/studioHelpers.ts +++ b/packages/studio/src/utils/studioHelpers.ts @@ -209,6 +209,28 @@ export function findTimelineIdByAncestor( return null; } +/** + * Resolve the timeline element id for a DOM selection: direct match first, then + * nearest clip ancestor. The ancestor lookup resolves against the selection's own + * source file, falling back to the active composition path, then index.html — so a + * sub-composition selection with no explicit sourceFile resolves against the comp + * currently open, not always the root file. + */ +export function resolveTimelineIdForSelection( + selection: DomEditSelection, + elements: TimelineElement[], + activeCompPath: string | null, +): string | null { + return ( + findMatchingTimelineElementId(selection, elements) ?? + findTimelineIdByAncestor( + selection.element, + elements, + selection.sourceFile || activeCompPath || "index.html", + ) + ); +} + export function resolveTimelineSelectionSeekTime( currentTime: number, element: Pick | null | undefined,