From fa3848a33d868f1b9796212bdc4efba4487ac14b Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Thu, 9 Jul 2026 15:03:15 -0400 Subject: [PATCH] fix(studio): keep the timeline multi-selection through a group edit setSelectedElementId no longer resets the selection set when re-selecting an element that is already a member: DOM-to-selection sync echoes fire on every pointer move during a group drag and were collapsing the set to the grabbed clip. Also drops the duplicate clearSelectedElementIds action in favor of clearSelection, which the marquee now uses to clear on an empty drag. --- .../components/useTimelineMarqueeSelection.ts | 2 +- .../src/player/store/playerStore.test.ts | 23 +++++++++++-------- .../studio/src/player/store/playerStore.ts | 22 ++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/packages/studio/src/player/components/useTimelineMarqueeSelection.ts b/packages/studio/src/player/components/useTimelineMarqueeSelection.ts index d61e06bb7..520865677 100644 --- a/packages/studio/src/player/components/useTimelineMarqueeSelection.ts +++ b/packages/studio/src/player/components/useTimelineMarqueeSelection.ts @@ -231,7 +231,7 @@ export function useTimelineMarqueeSelection({ setMarqueeRect(null); if (!active.started) { - usePlayerStore.getState().setSelection([]); + usePlayerStore.getState().clearSelection(); return true; } diff --git a/packages/studio/src/player/store/playerStore.test.ts b/packages/studio/src/player/store/playerStore.test.ts index 0338dd60e..abf415cbe 100644 --- a/packages/studio/src/player/store/playerStore.test.ts +++ b/packages/studio/src/player/store/playerStore.test.ts @@ -249,6 +249,18 @@ describe("usePlayerStore", () => { expect(state.selectedElementId).toBe("el-3"); }); + it("re-selecting a current member keeps the multi-selection and moves the anchor", () => { + const store = usePlayerStore.getState(); + store.setSelection(["el-1", "el-2", "el-3"], "el-1"); + // A DOM->selection sync echo during a group drag re-selects the grabbed + // member; this must NOT collapse the set to that single element. + store.setSelectedElementId("el-2"); + + const state = usePlayerStore.getState(); + expect([...state.selectedElementIds]).toEqual(["el-1", "el-2", "el-3"]); + expect(state.selectedElementId).toBe("el-2"); + }); + it("clearing single selection empties the set", () => { const store = usePlayerStore.getState(); store.setSelection(["el-1", "el-2"], "el-2"); @@ -275,19 +287,12 @@ describe("usePlayerStore", () => { expect(state.selectedElementId).toBe("el-2"); }); - it("clearSelection and clearSelectedElementIds empty the set and anchor", () => { + it("clearSelection empties the set and the anchor", () => { const store = usePlayerStore.getState(); store.setSelection(["el-1", "el-2"], "el-2"); store.clearSelection(); - let state = usePlayerStore.getState(); - expect([...state.selectedElementIds]).toEqual([]); - expect(state.selectedElementId).toBeNull(); - - store.setSelection(["el-3"], "el-3"); - store.clearSelectedElementIds(); - - state = usePlayerStore.getState(); + const state = usePlayerStore.getState(); expect([...state.selectedElementIds]).toEqual([]); expect(state.selectedElementId).toBeNull(); }); diff --git a/packages/studio/src/player/store/playerStore.ts b/packages/studio/src/player/store/playerStore.ts index b3b884d44..24ec3a258 100644 --- a/packages/studio/src/player/store/playerStore.ts +++ b/packages/studio/src/player/store/playerStore.ts @@ -146,7 +146,6 @@ interface PlayerState { addSelectedElementId: (id: string) => void; toggleSelectedElementId: (id: string) => void; clearSelection: () => void; - clearSelectedElementIds: () => void; /** Keyframe data per element id, populated from parsed GSAP animations. */ keyframeCache: Map; @@ -300,7 +299,6 @@ export const usePlayerStore = create((set, get) => ({ return resolveElementSelection(next, s.selectedElementId); }), clearSelection: () => set({ selectedElementId: null, selectedElementIds: new Set() }), - clearSelectedElementIds: () => set({ selectedElementId: null, selectedElementIds: new Set() }), keyframeCache: new Map(), setKeyframeCache: (elementId, data) => @@ -412,20 +410,30 @@ export const usePlayerStore = create((set, get) => ({ setBeatDragging: (dragging) => set({ beatDragging: dragging }), setElements: (elements) => set({ elements }), setSelectedElementId: (id) => - set((s) => + set((s) => { + // Re-selecting an element that is already part of an active multi-selection + // (e.g. a DOM->selection sync echo while a group drag re-patches the preview) + // updates the anchor WITHOUT collapsing the set. A genuinely new element + // (not currently in the set) replaces the whole selection with itself. + const keepSet = id != null && s.selectedElementIds.size > 1 && s.selectedElementIds.has(id); + const selectedElementIds = keepSet + ? s.selectedElementIds + : id + ? new Set([id]) + : new Set(); // Selecting a different element drops any active keyframe selection — otherwise // a stale activeKeyframePct from a prior diamond click would force the next drag // to "modify" a keyframe on the new element. A diamond click sets the pct AFTER // calling setSelectedElementId, so this never clobbers a genuine keyframe select. - id !== s.selectedElementId + return id !== s.selectedElementId ? { selectedElementId: id, - selectedElementIds: id ? new Set([id]) : new Set(), + selectedElementIds, activeKeyframePct: null, motionPathArmed: false, } - : { selectedElementId: id, selectedElementIds: id ? new Set([id]) : new Set() }, - ), + : { selectedElementId: id, selectedElementIds }; + }), updateElement: (elementId, updates) => set((state) => ({ elements: state.elements.map((el) =>