From ec0b23f3cec98ea9bb4b61d54ad18bb48ee1c721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Wed, 19 Aug 2026 00:22:26 -0400 Subject: [PATCH] fix(studio): make Delete remove the whole canvas selection (#3339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(studio): delete every clip in the selection, not just the first Select all in the timeline, press Delete, and one clip disappeared while the rest stayed — still drawn as selected. The Delete hotkey built the selection set correctly and then called `elements.find(...)`, which stops at the first match, and handed that single element to a handler that deletes exactly one. The comment above it claimed the handler "expands a clip that is part of the multi-selection into an atomic delete of the whole selection (single undo)" — no such expansion existed anywhere; `useTimelineEditing` never read `selectedElementIds`. `handleTimelineElementsDelete` takes the whole selection and removes every element before saving once, so the delete is a single history entry and a single undo — what the comment already promised. The hotkey layer now takes only that plural handler, since it never deletes one element in isolation; the singular entry point stays for the context menu and clip chrome. The store drops every deleted key and clears the marquee set, rather than leaving a selection drawn around clips that no longer exist. Elements whose `sourceFile` is not the composition being edited are dropped from the pass rather than written to the wrong file. Also removes the preview's double-click-to-reset-zoom. It was a document-level capture listener, so any double-click anywhere over the viewport snapped the zoom back to fit — including double-clicks meant for the content under it. The explicit reset control beside the zoom HUD stays. Reproduced by test: restoring `elements.find` reds the new marquee case. * fix(studio): delete every canvas element in the selection, not just the primary Selecting several elements on the canvas and pressing Delete removed one of them and left the rest — still drawn as selected. The delete path only ever took the primary selection; the marquee group it belongs to was ignored. Expand the session-level delete through the group ref, the same way the other group commits already do, and let the lifecycle op remove every member under a single save so one Undo restores the whole selection. * fix(studio): let the canvas selection own Delete instead of its timeline mirror Marquee-selecting elements on the canvas and pressing Delete removed a fraction of them. The hotkey routed to the timeline delete whenever the timeline store held anything, and the timeline's copy of a canvas selection is derived and lossy by construction — a member with no timeline row of its own is dropped from it. Selecting 73 elements published 14 ids, so 14 went and 59 stayed, still drawn as selected. The canvas selection is what the user drew the marquee around, so it owns Delete whenever it holds something; the timeline path stays as the fallback for rows with no canvas node to select. Both paths already remove through the same endpoint, so this is one addressing scheme replacing two. That makes the canvas delete the path a Delete press normally takes, so it picks up the same mid-recording refusal the timeline delete has. * fix(studio): let the marquee see the whole document, not the first 80 elements Dragging a marquee over the entire canvas selected a fraction of what it covered, so Delete left most of the page behind. The hit test sourced its candidates from the layers-panel collector, which stops after 80 items — a budget for how many rows that panel is willing to render, silently reused as if it described the document. Everything past the 80th element in document order was unselectable no matter where the user dragged. The off-canvas indicators were reading the same truncated list. The cap now belongs to the panel that wants it; the collector returns everything. To pay for that, the marquee measures its candidates once when the drag passes the threshold instead of re-reading layout for every element on every pointer-move: unbounded plus per-move stalled the tab outright, and the iframe DOM does not mutate mid-drag, so one pass stays true for the gesture. On a captured page: one marquee, one Delete, 734 elements down to 81. * fix(studio): report a no-op delete instead of claiming the elements went A target the file no longer holds answers `changed: false`, which is normal for a member nested inside another member already removed. Every target answering that is not — it means the preview is describing a document the file does not have, so each removal misses and the file is written back untouched. The toast still said "Deleted 503 elements. Use Undo to restore them." That is how a delete that did nothing at all looked from the outside: press Delete, the page stays, nothing on screen explains it. Say the preview is out of date and reload it instead. * fix(studio): keep the canvas hotkeys alive across preview reloads Pressing Delete with a canvas selection did nothing at all — no removal, no toast, nothing on screen to explain it. A keypress goes to whichever document has focus, and clicking the canvas puts focus inside the preview iframe, so the app's hotkeys have to be forwarded there. They were, but only from the iframe element's ref callback, which fires when the element mounts. A preview reload keeps the same element, so the callback never runs again, and keeps the same WindowProxy, so the forwarder's identity check saw no change and skipped re-attaching — while the inner window holding the listeners had been replaced. After the first reload the canvas had no app hotkeys left. Undo and redo kept working because their forwarder re-attaches on every load, which is why this read as "only Delete is broken". Fold the app handler into that per-load forwarder so both attach in the same place, on every load, and drop the mount-only one. Window only: the history pair also listens on the document, and capture listeners on both would run the app handler twice per press. * perf(studio): stop re-probing every restored selection member on load The hash carries the whole canvas selection, and restoring it asked the server whether each member still exists in the source — one request per member, awaited one after another. A marquee over a captured page puts hundreds of members in the URL, so every later load of that URL spent hundreds of serial round trips rebuilding the selection before the canvas answered anything, keypresses included. The marquee that produced those members already skips the probe. Restoring them skips it too; only the primary, whose panel reads the flag, still pays for one. * fix(studio): delete a canvas selection in one pass and say the key landed Reproduced with a real, focus-routed keypress instead of a synthetic one: the press does reach the handler and the delete does run to completion, but at hundreds of members it takes seconds during which the canvas is unchanged and nothing acknowledges the key. Silence for that long is indistinguishable from Delete being broken, and pressing it again or reloading mid-flight lands in a worse state. Two things, one per cause. The removal now sends the whole selection in a single request against a new remove-elements route, which reads the file once, drops every member and writes once — it was a round trip AND a full rewrite of the file per element. And a multi-element delete announces itself before the work starts, so the press is visibly acknowledged instead of leaving the canvas looking untouched until it finishes. Measured on a captured page, 84 members: 933ms of serial round trips against 84 rewrites, down to 583ms and one. * refactor(studio): narrow the SDK delete targets instead of asserting them The batch SDK path guarded on every member having an hfId and then asserted it away per member. Narrow once into a string list so the guard and the values come from the same place, and drop a threaded content variable that never changed — the SDK owns the document it edits, so every member is removed against the same starting content. Also mounts the new forwarding test through the existing harness rather than repeating its setup. * fix(studio): stop Delete acting on a canvas selection the user replaced Two things the reordered Delete arbitration got wrong, both found in review. A clip with no canvas node left the canvas selection pointing at whatever was picked before it, and the canvas branch wins whenever that ref is non-null — so selecting an audio clip and pressing Delete removed the previously selected canvas element and left the clip, right after the toast said the clip was not in the preview. The timeline fallback the comment described could not be reached. Clearing that selection has to stay quiet: the clear is announced to the timeline, so echoing it would deselect the clip that was just picked. Expanding the primary to the marquee group also moved out of the delete handler and up to the Delete key. Cut copies the primary alone, so expanding for every caller put one element on the clipboard and removed every other member with it — undo brought them back, paste restored one. The rule is a named function now, so the two callers can differ without either guessing. Also throttles the off-canvas indicator rebuild, which the cap had been hiding. It walks every element in the preview and reads layout for each — measured at 6.5ms on an 825-element captured page against a 16.7ms frame — and what marks it dirty is a MutationObserver on inline style, which is how animation writes. * fix(studio): hold the canvas selection inside the timeline selection The stale-canvas-selection defect survived at the second writer. The store-driven sync bails when a member has not resolved yet and returned without touching the canvas, so a pick with no canvas node at all left the previous selection in place — and Delete acts on the canvas first, so it deleted that. Reachable from the sidebar audio and asset reveals and from an asset drop, none of which go through the handler already fixed. Clearing on every bail would be wrong: the bail exists for a member whose node is not ready, which a later run resolves, and clearing there would flicker. Only a canvas anchor that resolves OUTSIDE the current selection goes, which is the state that is dangerous rather than merely unfinished. Quietly, for the same reason as the first writer: announcing would deselect the clip just picked. The invariant is named now, since Delete depends on it: the canvas selection never points outside the current timeline selection. Also drops the x-hf-removed header, which nothing read and whose comment promised a partial-vs-no-op distinction the response cannot make, and pins the indicator throttle that was measured but uncovered. --- packages/studio-server/src/routes/files.ts | 31 ++++++ packages/studio/src/App.tsx | 20 ++-- .../src/components/editor/LayersPanel.tsx | 15 ++- .../components/editor/SnapToolbar.test.tsx | 2 +- .../editor/domEditingLayers.test.ts | 24 ++++ .../src/components/editor/domEditingLayers.ts | 6 +- .../src/components/editor/marqueeCommit.ts | 59 ++++++---- .../editor/offCanvasIndicatorRefresh.test.tsx | 6 + ...offCanvasIndicatorRefresh.throttle.test.ts | 18 +++ .../editor/offCanvasIndicatorRefresh.ts | 23 +++- .../studio/src/components/nle/NLEPreview.tsx | 22 ---- .../useAppHotkeys.previewForwarding.test.tsx | 93 ++++++++++++++++ .../studio/src/hooks/useAppHotkeys.test.ts | 89 ++++++++++++++- .../hooks/useAppHotkeys.textEditing.test.tsx | 14 ++- packages/studio/src/hooks/useAppHotkeys.ts | 104 +++++++++--------- .../studio/src/hooks/useDomEditCommits.ts | 30 ++--- .../studio/src/hooks/useDomEditPreviewSync.ts | 10 +- ...useDomEditSession.membersForDelete.test.ts | 23 ++++ .../src/hooks/useDomEditSession.test.tsx | 2 +- .../studio/src/hooks/useDomEditSession.ts | 42 ++++++- packages/studio/src/hooks/useDomEditWiring.ts | 6 +- packages/studio/src/hooks/useDomSelection.ts | 23 ++-- .../useDomSelectionSelectionGuards.test.ts | 49 +++++++++ ...seElementLifecycleOps.multiDelete.test.tsx | 100 +++++++++++++++++ .../src/hooks/useElementLifecycleOps.ts | 89 ++++++++++++--- .../hooks/useStudioUrlState.hydration.test.ts | 39 +++++++ .../studio/src/hooks/useStudioUrlState.ts | 12 +- .../studio/src/hooks/useTimelineEditing.ts | 83 +++++++++----- .../useTimelineSelectionPreviewSync.test.tsx | 32 ++++++ .../hooks/useTimelineSelectionPreviewSync.ts | 35 +++++- 30 files changed, 887 insertions(+), 214 deletions(-) create mode 100644 packages/studio/src/components/editor/offCanvasIndicatorRefresh.throttle.test.ts create mode 100644 packages/studio/src/hooks/useAppHotkeys.previewForwarding.test.tsx create mode 100644 packages/studio/src/hooks/useDomEditSession.membersForDelete.test.ts create mode 100644 packages/studio/src/hooks/useElementLifecycleOps.multiDelete.test.tsx create mode 100644 packages/studio/src/hooks/useStudioUrlState.hydration.test.ts diff --git a/packages/studio-server/src/routes/files.ts b/packages/studio-server/src/routes/files.ts index dbe2f9b91..c45641a33 100644 --- a/packages/studio-server/src/routes/files.ts +++ b/packages/studio-server/src/routes/files.ts @@ -2496,6 +2496,37 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void { ); }); + // Removing a marquee selection one element at a time meant one request and + // one rewrite of the whole file per element. A canvas selection runs to + // hundreds of members, so a single Delete press became hundreds of serial + // round trips: the file ended up correct, but only after long enough that the + // key looked like it had done nothing at all. + api.post("/projects/:id/file-mutations/remove-elements/*", async (c) => { + const ctx = await resolveFileMutationContext(c, adapter, "remove-elements"); + if ("error" in ctx) return ctx.error; + + if (!existsSync(ctx.absPath)) { + return c.json({ error: "not found" }, 404); + } + + const body = (await c.req.json().catch(() => null)) as { targets?: MutationTarget[] } | null; + const targets = body?.targets; + if (!Array.isArray(targets) || targets.length === 0) { + return c.json({ error: "targets required" }, 400); + } + + const originalContent = readFileSync(ctx.absPath, "utf-8"); + // A member nested inside one already removed simply no longer matches, which + // is a normal outcome here rather than a failure. The response says whether + // the file changed, not how many of the targets landed — so a caller can + // tell a no-op from a write, but not a partial pass from a complete one. + let next = originalContent; + for (const target of targets) { + next = removeElementFromHtml(next, target); + } + return writeIfChanged(c, ctx.project.dir, ctx.filePath, ctx.absPath, originalContent, next); + }); + api.post("/projects/:id/file-mutations/split-batch", async (c) => { const body = (await c.req.json().catch(() => null)) as { files?: unknown; diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index 199971e07..fea56e771 100644 --- a/packages/studio/src/App.tsx +++ b/packages/studio/src/App.tsx @@ -218,10 +218,9 @@ export function StudioApp() { }); const clearDomSelectionRef = useRef<() => void>(() => {}); const domEditSelectionBridgeRef = useRef(null); - const handleDomEditElementDeleteRef = useRef<(s: DomEditSelection) => Promise>( - async () => {}, - ); - const domEditDeleteBridge = (s: DomEditSelection) => handleDomEditElementDeleteRef.current(s); + type DomEditDelete = (s: DomEditSelection, o?: { expandGroup?: boolean }) => Promise; + const handleDomEditElementDeleteRef = useRef(async () => {}); + const domEditDeleteBridge: DomEditDelete = (s, o) => handleDomEditElementDeleteRef.current(s, o); const resetKeyframesRef = useRef<() => boolean>(() => false); const deleteSelectedKeyframesRef = useRef<() => void>(() => {}); const { handleCopy, handlePaste, handleCut } = useClipboard({ @@ -238,7 +237,7 @@ export function StudioApp() { previewIframeRef, }); const appHotkeys = useAppHotkeys({ - handleTimelineElementDelete: timelineEditing.handleTimelineElementDelete, + handleTimelineElementsDelete: timelineEditing.handleTimelineElementsDelete, handleTimelineElementSplit: timelineEditing.handleTimelineElementSplit, handleDomEditElementDelete: domEditDeleteBridge, domEditSelectionRef: domEditSelectionBridgeRef, @@ -282,6 +281,7 @@ export function StudioApp() { setRightCollapsed: panelLayout.setRightCollapsed, setRightPanelTab: panelLayout.setRightPanelTab, showToast, + isRecordingRef: isGestureRecordingRef, refreshPreviewDocumentVersion, queueDomEditSave: previewPersistence.queueDomEditSave, readProjectFile: fileManager.readProjectFile, @@ -298,7 +298,7 @@ export function StudioApp() { previewDocumentVersion, rightPanelTab: panelLayout.rightPanelTab, applyStudioManualEditsToPreviewRef: previewPersistence.applyStudioManualEditsToPreviewRef, - syncPreviewHistoryHotkey: appHotkeys.syncPreviewHistoryHotkey, + syncPreviewHotkeys: appHotkeys.syncPreviewHotkeys, reloadPreview, setRefreshKey, openSourceForSelection: fileManager.openSourceForSelection, @@ -365,7 +365,6 @@ export function StudioApp() { isGestureRecordingRef, }); handleToggleRecordingRef.current = handleToggleRecording; - const recordingToggle = handleToggleRecording; const canvasRectRef = useRef(null); useLayoutEffect(() => { if (gestureState !== "recording" || !previewIframe) { @@ -378,8 +377,7 @@ export function StudioApp() { (iframe: HTMLIFrameElement | null) => { previewIframeRef.current = iframe; setPreviewIframe(iframe); - appHotkeys.syncPreviewTimelineHotkey(iframe); - appHotkeys.syncPreviewHistoryHotkey(iframe); + appHotkeys.syncPreviewHotkeys(iframe); resetConsoleErrors(); refreshPreviewDocumentVersion(); }, @@ -526,7 +524,7 @@ export function StudioApp() { }} recordingState={gestureState} recordingDuration={gestureRecording.recordingDuration} - onToggleRecording={recordingToggle} + onToggleRecording={handleToggleRecording} sdkSession={sdkHandle.session} publishSdkSession={sdkHandle.publish} forceReloadSdkSession={sdkHandle.forceReload} @@ -561,7 +559,7 @@ export function StudioApp() { shouldShowSelectedDomBounds={shouldShowSelectedDomBounds} isGestureRecording={gestureState === "recording"} recordingState={gestureState} - onToggleRecording={recordingToggle} + onToggleRecording={handleToggleRecording} blockPreview={blockPreview} gestureOverlay={ gestureState === "recording" && previewIframe ? ( diff --git a/packages/studio/src/components/editor/LayersPanel.tsx b/packages/studio/src/components/editor/LayersPanel.tsx index ca215d1d7..914800e2d 100644 --- a/packages/studio/src/components/editor/LayersPanel.tsx +++ b/packages/studio/src/components/editor/LayersPanel.tsx @@ -23,6 +23,9 @@ import { useLayerReorderTimelineMirror } from "../nle/useCanvasZOrderTimelineMir import { runZLaneGesture } from "../nle/zLaneGesture"; import { useLayerRevealOverride } from "./useLayerRevealOverride"; +// Rows this panel renders before it stops. A display budget, not a document limit. +const LAYERS_PANEL_MAX_ROWS = 80; + const TAG_ICONS: Record = { video: "Vi", audio: "Au", @@ -137,11 +140,13 @@ export const LayersPanel = memo(function LayersPanel() { // A preview reload detaches the drilled-into wrapper; exit drill-in if so. if (activeGroupElement && !activeGroupElement.isConnected) setActiveGroupElement(null); - const items = collectDomEditLayerItems(root, { - activeCompositionPath: activeCompPath, - isMasterView, - activeGroupElement, - }); + const items = collectDomEditLayerItems( + root, + { activeCompositionPath: activeCompPath, isMasterView, activeGroupElement }, + // How many rows this panel is willing to render, nothing more. Hit-testing + // callers deliberately take the whole document instead. + LAYERS_PANEL_MAX_ROWS, + ); setLayers(sortLayersByZIndex(items)); }, [previewIframeRef, activeCompPath, isMasterView, activeGroupElement, setActiveGroupElement]); diff --git a/packages/studio/src/components/editor/SnapToolbar.test.tsx b/packages/studio/src/components/editor/SnapToolbar.test.tsx index 29e73b463..f364ec7f1 100644 --- a/packages/studio/src/components/editor/SnapToolbar.test.tsx +++ b/packages/studio/src/components/editor/SnapToolbar.test.tsx @@ -34,7 +34,7 @@ function AppHotkeyHarness() { const leftSidebarRef = useRef(null); useAppHotkeys({ - handleTimelineElementDelete: vi.fn(), + handleTimelineElementsDelete: vi.fn(async () => {}), handleTimelineElementSplit: vi.fn(), handleDomEditElementDelete: vi.fn(), domEditSelectionRef, diff --git a/packages/studio/src/components/editor/domEditingLayers.test.ts b/packages/studio/src/components/editor/domEditingLayers.test.ts index b81728994..c0e972dff 100644 --- a/packages/studio/src/components/editor/domEditingLayers.test.ts +++ b/packages/studio/src/components/editor/domEditingLayers.test.ts @@ -224,3 +224,27 @@ describe("buildTextFieldChildLocator", () => { expect(buildTextFieldChildLocator(fields, "missing")).toBeNull(); }); }); + +describe("collectDomEditLayerItems item budget", () => { + function documentWith(count: number): HTMLElement { + const root = document.createElement("div"); + root.setAttribute("data-composition-id", "index.html"); + for (let i = 0; i < count; i++) { + const child = document.createElement("div"); + child.id = `el-${i}`; + root.append(child); + } + return root; + } + + it("returns the whole document by default", () => { + // A default cap here silently truncated the marquee's candidate list: a drag + // over the whole canvas only ever saw the first 80 elements, so everything + // past them was unselectable and survived a Delete. + expect(collectDomEditLayerItems(documentWith(200), opts)).toHaveLength(200); + }); + + it("truncates only when a caller asks for a rendering budget", () => { + expect(collectDomEditLayerItems(documentWith(200), opts, 80)).toHaveLength(80); + }); +}); diff --git a/packages/studio/src/components/editor/domEditingLayers.ts b/packages/studio/src/components/editor/domEditingLayers.ts index 178d72f50..b1af3fb61 100644 --- a/packages/studio/src/components/editor/domEditingLayers.ts +++ b/packages/studio/src/components/editor/domEditingLayers.ts @@ -458,10 +458,14 @@ export function countDomEditChildLayers( return count; } +// Every editable element under `root`, in document order. `maxItems` is a +// caller's rendering budget, not a property of the document: hit-testing +// callers (marquee, off-canvas indicators) must see all of it, and sharing a +// truncated list left everything past the cut unselectable however far you drag. export function collectDomEditLayerItems( root: HTMLElement | null | undefined, options: DomEditContextOptions, - maxItems = 80, + maxItems = Number.POSITIVE_INFINITY, ): DomEditLayerItem[] { if (!root) return []; diff --git a/packages/studio/src/components/editor/marqueeCommit.ts b/packages/studio/src/components/editor/marqueeCommit.ts index 99fc2b061..fa731613c 100644 --- a/packages/studio/src/components/editor/marqueeCommit.ts +++ b/packages/studio/src/components/editor/marqueeCommit.ts @@ -24,16 +24,18 @@ interface MarqueeHit { } /** - * Synchronous core of the marquee: the elements whose overlay-space rect - * intersects the marquee rect. Uses the SAME `toOverlayRect` basis as the - * single-selection / group-selection boxes, so what the marquee highlights - * and selects is exactly the box the user sees when they click an element. - * Shared by the live candidate highlight (per pointer-move) and the mouse-up - * commit. No async source probe — that only happens once, on commit. + * Every element the marquee could hit, with the overlay-space rect it would be + * tested against. Uses the SAME `toOverlayRect` basis as the single-selection / + * group-selection boxes, so what the marquee highlights and selects is exactly + * the box the user sees when they click an element. + * + * Measured once per drag rather than per pointer-move: this reads layout for + * every element in the document, and a captured page has enough of them that + * doing it 60 times a second stalls the tab. The iframe DOM does not mutate + * mid-drag, so the rects it returns stay true for the whole gesture. */ // fallow-ignore-next-line complexity -function collectMarqueeHits( - rect: Rect, +function collectMarqueeCandidates( iframe: HTMLIFrameElement, overlayEl: HTMLDivElement, activeCompositionPath: string, @@ -53,35 +55,39 @@ function collectMarqueeHits( height: declH > 0 ? declH : rootEl.getBoundingClientRect().height || 1, }; - const hits: MarqueeHit[] = []; + const candidates: MarqueeHit[] = []; for (const item of items) { const el = item.element; if (!isElementComputedVisible(el)) continue; if (coversComposition(el.getBoundingClientRect(), viewport)) continue; const overlayRect = toVisibleOverlayRect(overlayEl, iframe, el); if (!overlayRect) continue; - const r: Rect = { - left: overlayRect.left, - top: overlayRect.top, - width: overlayRect.width, - height: overlayRect.height, - }; - if (!rectsOverlap(rect, r)) continue; - hits.push({ element: el, rect: r }); + candidates.push({ + element: el, + rect: { + left: overlayRect.left, + top: overlayRect.top, + width: overlayRect.width, + height: overlayRect.height, + }, + }); } - return hits; + return candidates; +} + +function hitsWithin(rect: Rect, candidates: MarqueeHit[]): MarqueeHit[] { + return candidates.filter((candidate) => rectsOverlap(rect, candidate.rect)); } async function runMarqueeIntersection( rect: Rect, - iframe: HTMLIFrameElement, - overlayEl: HTMLDivElement, + candidates: MarqueeHit[], activeCompositionPath: string, ): Promise { const isMasterView = !activeCompositionPath || activeCompositionPath === "index.html"; const hits: DomEditSelection[] = []; - for (const { element } of collectMarqueeHits(rect, iframe, overlayEl, activeCompositionPath)) { + for (const { element } of hitsWithin(rect, candidates)) { const sel = await resolveDomEditSelection(element, { activeCompositionPath, isMasterView, @@ -116,6 +122,8 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { // iframe DOM doesn't mutate during a drag, so a sync intersection per move // is cheap (clean layout → no thrash). const [candidateRects, setCandidateRects] = useState([]); + // Measured once when the drag passes the threshold and reused until it ends. + const candidatesRef = useRef(null); const commitMarquee = useCallback( async ( @@ -126,7 +134,8 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { const overlay = deps.overlayRef.current; if (!iframe || !overlay || !deps.onMarqueeSelectRef.current) return; const acp = deps.activeCompositionPathRef.current ?? "index.html"; - const hits = await runMarqueeIntersection(rect, iframe, overlay, acp); + const candidates = candidatesRef.current ?? collectMarqueeCandidates(iframe, overlay, acp); + const hits = await runMarqueeIntersection(rect, candidates, acp); deps.onMarqueeSelectRef.current(hits, additive); }, [deps.iframeRef, deps.overlayRef, deps.onMarqueeSelectRef, deps.activeCompositionPathRef], @@ -145,6 +154,7 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { const dy = m.currentY - m.startY; if (Math.hypot(dx, dy) < MARQUEE_THRESHOLD_PX) return; m.pastThreshold = true; + candidatesRef.current = null; } const rect: Rect = { left: Math.min(m.startX, m.currentX), @@ -157,7 +167,8 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { const overlay = deps.overlayRef.current; if (iframe && overlay) { const acp = deps.activeCompositionPathRef.current ?? "index.html"; - setCandidateRects(collectMarqueeHits(rect, iframe, overlay, acp).map((h) => h.rect)); + candidatesRef.current ??= collectMarqueeCandidates(iframe, overlay, acp); + setCandidateRects(hitsWithin(rect, candidatesRef.current).map((h) => h.rect)); } return; } @@ -191,6 +202,7 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { } setMarqueeRect(null); setCandidateRects([]); + candidatesRef.current = null; return; } deps.gestures.onPointerUp(event); @@ -203,6 +215,7 @@ export function useMarqueeGestures(deps: MarqueeGesturesDeps) { marqueeRef.current = null; setMarqueeRect(null); setCandidateRects([]); + candidatesRef.current = null; return; } deps.gestures.clearPointerState(deps.selectionRef); diff --git a/packages/studio/src/components/editor/offCanvasIndicatorRefresh.test.tsx b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.test.tsx index 3d2321e74..8e7528904 100644 --- a/packages/studio/src/components/editor/offCanvasIndicatorRefresh.test.tsx +++ b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.test.tsx @@ -4,6 +4,7 @@ import React, { act } from "react"; import { createRoot, type Root } from "react-dom/client"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { DomEditOverlay } from "./DomEditOverlay"; +import { RECOMPUTE_INTERVAL_MS } from "./offCanvasIndicatorRefresh"; Reflect.set(globalThis, "IS_REACT_ACT_ENVIRONMENT", true); @@ -47,7 +48,12 @@ function domRect(left: number, top: number, width: number, height: number): DOMR }; } +// The refresh rebuilds at most every RECOMPUTE_INTERVAL_MS — it walks the whole +// preview and reads layout per element, which is too much to do per frame while +// animation is writing inline styles. Waiting past that window is what makes +// consecutive frames here represent consecutive rebuilds. async function flushAnimationFrames(): Promise { + await new Promise((resolve) => setTimeout(resolve, RECOMPUTE_INTERVAL_MS + 5)); await new Promise((resolve) => { requestAnimationFrame(() => requestAnimationFrame(() => resolve())); }); diff --git a/packages/studio/src/components/editor/offCanvasIndicatorRefresh.throttle.test.ts b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.throttle.test.ts new file mode 100644 index 000000000..84a09a87b --- /dev/null +++ b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.throttle.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { RECOMPUTE_INTERVAL_MS, rebuildDue } from "./offCanvasIndicatorRefresh"; + +describe("rebuildDue", () => { + it("collapses mutations arriving inside one window into a single rebuild", () => { + // A rebuild walks the whole preview and reads layout per element, and what + // marks it dirty is a MutationObserver on inline style — which is how + // animation writes. Without this, playback pays that on nearly every frame. + const first = 1_000; + expect(rebuildDue(true, Number.NEGATIVE_INFINITY, first)).toBe(true); + expect(rebuildDue(true, first, first + RECOMPUTE_INTERVAL_MS - 1)).toBe(false); + expect(rebuildDue(true, first, first + RECOMPUTE_INTERVAL_MS)).toBe(true); + }); + + it("never rebuilds when nothing changed", () => { + expect(rebuildDue(false, Number.NEGATIVE_INFINITY, 1_000)).toBe(false); + }); +}); diff --git a/packages/studio/src/components/editor/offCanvasIndicatorRefresh.ts b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.ts index 8e3870422..d10fb3a1a 100644 --- a/packages/studio/src/components/editor/offCanvasIndicatorRefresh.ts +++ b/packages/studio/src/components/editor/offCanvasIndicatorRefresh.ts @@ -43,11 +43,29 @@ function observeDoc(doc: Document, markDirty: () => void): MutationObserver | nu return observer; } +/** + * How often the indicator geometry may be rebuilt. + * + * A rebuild walks every element in the preview and reads layout for each — + * 6.5ms on an 825-element captured page, against a 16.7ms frame. What marks it + * dirty is a MutationObserver on inline style, which is exactly how animation + * writes, so playback would pay that on close to every frame. The indicators + * are a passive affordance: refreshing them a few times a second is + * indistinguishable on screen and keeps the cost off the frame budget. + */ +export const RECOMPUTE_INTERVAL_MS = 100; + +/** Dirty, and far enough past the last rebuild to be worth paying for another. */ +export function rebuildDue(dirty: boolean, lastAt: number, now: number): boolean { + return dirty && now - lastAt >= RECOMPUTE_INTERVAL_MS; +} + export function startOffCanvasIndicatorRefresh( options: OffCanvasIndicatorRefreshOptions, ): () => void { let frame = 0; let lastCompSig = ""; + let lastRecomputeAt = Number.NEGATIVE_INFINITY; const markDirty = () => { options.dirtyRef.current = true; }; @@ -76,7 +94,10 @@ export function startOffCanvasIndicatorRefresh( if (options.dirtyRef.current) clearIndicators(options); return; } - if (!options.dirtyRef.current) return; + // Staying dirty while throttled is what makes the next eligible frame rebuild. + const now = performance.now(); + if (!rebuildDue(options.dirtyRef.current, lastRecomputeAt, now)) return; + lastRecomputeAt = now; options.dirtyRef.current = false; recomputeOffCanvasIndicators( iframe, diff --git a/packages/studio/src/components/nle/NLEPreview.tsx b/packages/studio/src/components/nle/NLEPreview.tsx index 8e634f767..3806b7782 100644 --- a/packages/studio/src/components/nle/NLEPreview.tsx +++ b/packages/studio/src/components/nle/NLEPreview.tsx @@ -354,28 +354,6 @@ export const NLEPreview = memo(function NLEPreview({ return () => document.removeEventListener("wheel", handleWheel, { capture: true }); }, [applyZoom, applyPan]); - useEffect(() => { - const viewport = viewportRef.current; - if (!viewport) return; - - const handleDblClick = (event: MouseEvent) => { - if (isPreviewAtFit(zoomRef.current)) return; - const rect = viewport.getBoundingClientRect(); - if ( - event.clientX < rect.left || - event.clientX > rect.right || - event.clientY < rect.top || - event.clientY > rect.bottom - ) { - return; - } - applyZoom(DEFAULT_PREVIEW_ZOOM); - }; - - document.addEventListener("dblclick", handleDblClick, { capture: true }); - return () => document.removeEventListener("dblclick", handleDblClick, { capture: true }); - }, [applyZoom]); - useEffect(() => { const isInsideViewport = (clientX: number, clientY: number): DOMRect | null => { const viewport = viewportRef.current; diff --git a/packages/studio/src/hooks/useAppHotkeys.previewForwarding.test.tsx b/packages/studio/src/hooks/useAppHotkeys.previewForwarding.test.tsx new file mode 100644 index 000000000..b348cceeb --- /dev/null +++ b/packages/studio/src/hooks/useAppHotkeys.previewForwarding.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment happy-dom + +import React, { act, useRef } from "react"; +import type { Root } from "react-dom/client"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { DomEditSelection } from "../components/editor/domEditing"; +import type { LeftSidebarHandle } from "../components/sidebar/LeftSidebar"; +import { usePlayerStore } from "../player/store/playerStore"; +import { useAppHotkeys } from "./useAppHotkeys"; +import { mountReactHarness } from "./domSelectionTestHarness"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const domDelete = vi.fn(async () => undefined); +let root: Root | null = null; +let sync: ((iframe: HTMLIFrameElement | null) => void) | null = null; + +function selection(): DomEditSelection { + const element = document.createElement("section"); + element.id = "card"; + return { + element, + id: "card", + selector: "#card", + selectorIndex: 0, + sourceFile: "index.html", + } as unknown as DomEditSelection; +} + +function Harness() { + const selectionRef = useRef(selection()); + const hotkeys = useAppHotkeys({ + handleTimelineElementsDelete: vi.fn(async () => undefined), + handleTimelineElementSplit: vi.fn(async () => undefined), + handleDomEditElementDelete: domDelete, + domEditSelectionRef: selectionRef, + clearDomSelectionRef: useRef<() => void>(() => undefined), + editHistory: { + undo: vi.fn(async () => ({ ok: false })), + redo: vi.fn(async () => ({ ok: false })), + state: { undo: [], redo: [] }, + }, + readOptionalProjectFile: vi.fn(async () => ""), + readProjectFile: vi.fn(async () => ""), + writeProjectFile: vi.fn(async () => undefined), + domEditSaveTimestampRef: useRef(0), + showToast: vi.fn(), + syncHistoryPreviewAfterApply: vi.fn(async () => undefined), + waitForPendingDomEditSaves: vi.fn(async () => undefined), + leftSidebarRef: useRef(null), + handleCopy: vi.fn(() => false), + handlePaste: vi.fn(() => false), + handleCut: vi.fn(() => false), + onResetKeyframes: vi.fn(() => false), + onDeleteSelectedKeyframes: vi.fn(), + onAfterUndoRedo: vi.fn(), + } as unknown as Parameters[0]); + sync = hotkeys.syncPreviewHotkeys; + return null; +} + +afterEach(() => { + if (root) act(() => root?.unmount()); + root = null; + document.body.innerHTML = ""; + usePlayerStore.getState().reset(); + domDelete.mockClear(); +}); + +describe("preview iframe hotkey forwarding", () => { + it("still delivers Delete after the preview reloads", () => { + // A reload keeps the iframe element (no ref callback) and the same + // WindowProxy (an identity check sees no change) but replaces the window + // holding the listeners. Attaching once left Delete dead inside the canvas + // after the first reload — and clicking the canvas is what puts focus there. + root = mountReactHarness(); + + const iframe = document.createElement("iframe"); + document.body.append(iframe); + act(() => sync?.(iframe)); + + // The reload: same element, a window that has lost its listeners. + act(() => sync?.(iframe)); + + const inner = iframe.contentWindow as (Window & typeof globalThis) | null; + if (!inner) throw new Error("expected an iframe window"); + inner.document.body.dispatchEvent( + new inner.KeyboardEvent("keydown", { key: "Delete", bubbles: true }), + ); + + expect(domDelete).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/studio/src/hooks/useAppHotkeys.test.ts b/packages/studio/src/hooks/useAppHotkeys.test.ts index 2920d0f30..eeeb7387e 100644 --- a/packages/studio/src/hooks/useAppHotkeys.test.ts +++ b/packages/studio/src/hooks/useAppHotkeys.test.ts @@ -22,6 +22,7 @@ const bgmElement: TimelineElement = { function callbacks() { return { handleTimelineElementDelete: vi.fn(async () => {}), + handleTimelineElementsDelete: vi.fn(async () => {}), handleTimelineElementSplit: vi.fn(async () => {}), handleDomEditElementDelete: vi.fn(async () => {}), handleUndo: vi.fn(async () => {}), @@ -69,10 +70,90 @@ describe("dispatchPlainKey — Delete arbitration", () => { const e = press("Delete"); dispatchPlainKey(e, "delete", cb); // The pre-existing contract, pinned so the new guard cannot widen. - expect(cb.handleTimelineElementDelete).toHaveBeenCalledTimes(1); + expect(cb.handleTimelineElementsDelete).toHaveBeenCalledTimes(1); + expect(cb.handleTimelineElementsDelete).toHaveBeenCalledWith([bgmElement]); expect(e.defaultPrevented).toBe(true); }); + it("deletes EVERY clip in a marquee selection, not just the first", () => { + // The reported bug: select all, press Delete, and one clip disappears while + // the rest stay — still drawn as selected. The handler used `elements.find`, + // which stops at the first match. + const clips = ["a", "b", "c"].map((id) => ({ ...bgmElement, id, key: id })); + usePlayerStore.setState({ + elements: clips, + selectedElementId: null, + selectedElementIds: new Set(["a", "b", "c"]), + }); + + const cb = callbacks(); + const e = press("Delete"); + dispatchPlainKey(e, "delete", cb); + + expect(cb.handleTimelineElementsDelete).toHaveBeenCalledTimes(1); + const [passed] = cb.handleTimelineElementsDelete.mock.calls[0] as [typeof clips]; + expect(passed.map((c) => c.key)).toEqual(["a", "b", "c"]); + expect(e.defaultPrevented).toBe(true); + }); + + it("hands a canvas selection its whole group instead of the timeline's partial copy", () => { + // The reported bug: marquee 73 elements on the canvas, press Delete, and 14 + // vanish. Only those 14 owned a timeline row, and the timeline mirror drops + // every member that does not — so deleting through it left 59 behind, still + // drawn as selected. + const clips = ["a", "b"].map((id) => ({ ...bgmElement, id, key: id })); + usePlayerStore.setState({ + elements: clips, + selectedElementId: null, + selectedElementIds: new Set(["a", "b"]), + }); + const domSelection = { selector: ".title", selectorIndex: 0, sourceFile: "index.html" }; + + const cb = callbacks(); + cb.domEditSelectionRef = { current: domSelection } as typeof cb.domEditSelectionRef; + const e = press("Delete"); + dispatchPlainKey(e, "delete", cb); + + expect(cb.handleDomEditElementDelete).toHaveBeenCalledWith(domSelection, { + expandGroup: true, + }); + expect(cb.handleTimelineElementsDelete).not.toHaveBeenCalled(); + expect(e.defaultPrevented).toBe(true); + }); + + it("asks for the whole group, so Cut can still take just the one it copied", () => { + // Expanding inside the delete handler meant every caller got the group. + // Cut copies the primary alone, so it put one element on the clipboard and + // removed every other member with it; paste brought back one. + usePlayerStore.setState({ + elements: [], + selectedElementId: null, + selectedElementIds: new Set(), + }); + const domSelection = { selector: ".title", selectorIndex: 0, sourceFile: "index.html" }; + + const cb = callbacks(); + cb.domEditSelectionRef = { current: domSelection } as typeof cb.domEditSelectionRef; + dispatchPlainKey(press("Delete"), "delete", cb); + + expect(cb.handleDomEditElementDelete).toHaveBeenCalledWith(domSelection, { expandGroup: true }); + }); + + it("includes the primary selection alongside the marquee set", () => { + const clips = ["a", "b"].map((id) => ({ ...bgmElement, id, key: id })); + usePlayerStore.setState({ + elements: clips, + selectedElementId: "b", + selectedElementIds: new Set(["a"]), + }); + + const cb = callbacks(); + dispatchPlainKey(press("Delete"), "delete", cb); + + const [passed] = cb.handleTimelineElementsDelete.mock.calls[0] as [typeof clips]; + expect(passed.map((c) => c.key).sort()).toEqual(["a", "b"]); + }); + it("leaves the clip alone when an automation range is active", () => { // The bug: this listener is on window/capture so it runs BEFORE // useAutomationSelectionKeyboard's document/capture handler. Without the @@ -82,7 +163,7 @@ describe("dispatchPlainKey — Delete arbitration", () => { const cb = callbacks(); const e = press("Delete"); dispatchPlainKey(e, "delete", cb); - expect(cb.handleTimelineElementDelete).not.toHaveBeenCalled(); + expect(cb.handleTimelineElementsDelete).not.toHaveBeenCalled(); // Must NOT be consumed: the automation handler downstream still needs it. expect(e.defaultPrevented).toBe(false); }); @@ -99,7 +180,7 @@ describe("dispatchPlainKey — Delete arbitration", () => { const e = press("Backspace"); dispatchPlainKey(e, "backspace", cb); expect(cb.onResetKeyframes).not.toHaveBeenCalled(); - expect(cb.handleTimelineElementDelete).not.toHaveBeenCalled(); + expect(cb.handleTimelineElementsDelete).not.toHaveBeenCalled(); expect(e.defaultPrevented).toBe(false); }); @@ -113,7 +194,7 @@ describe("dispatchPlainKey — Delete arbitration", () => { const e = press("Delete"); dispatchPlainKey(e, "delete", cb); expect(cb.onDeleteSelectedKeyframes).toHaveBeenCalledTimes(1); - expect(cb.handleTimelineElementDelete).not.toHaveBeenCalled(); + expect(cb.handleTimelineElementsDelete).not.toHaveBeenCalled(); expect(e.defaultPrevented).toBe(true); }); }); diff --git a/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx b/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx index d1d2b6a53..446cff336 100644 --- a/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx +++ b/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx @@ -11,7 +11,7 @@ import { useAppHotkeys } from "./useAppHotkeys"; (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; -const timelineDelete = vi.fn(async () => undefined); +const timelineDeleteMany = vi.fn(async () => undefined); const domDelete = vi.fn(async () => undefined); const keyframeDelete = vi.fn(); const textCommits = vi.fn(); @@ -74,7 +74,7 @@ function Harness() { const leftSidebarRef = useRef(null); useAppHotkeys({ - handleTimelineElementDelete: timelineDelete, + handleTimelineElementsDelete: timelineDeleteMany, handleTimelineElementSplit: vi.fn(async () => undefined), handleDomEditElementDelete: domDelete, domEditSelectionRef: selectionRef, @@ -140,7 +140,7 @@ function pressBackspace(target: HTMLElement): KeyboardEvent { beforeEach(() => { vi.useFakeTimers(); - timelineDelete.mockClear(); + timelineDeleteMany.mockClear(); domDelete.mockClear(); keyframeDelete.mockClear(); textCommits.mockClear(); @@ -200,7 +200,7 @@ describe("useAppHotkeys text-field ownership", () => { expect(document.activeElement).toBe(textarea); expect(textarea.selectionStart).toBe(4); expect(textarea.selectionEnd).toBe(4); - expect(timelineDelete).not.toHaveBeenCalled(); + expect(timelineDeleteMany).not.toHaveBeenCalled(); expect(domDelete).not.toHaveBeenCalled(); expect(keyframeDelete).not.toHaveBeenCalled(); @@ -208,8 +208,10 @@ describe("useAppHotkeys text-field ownership", () => { const canvasDelete = pressBackspace(canvas); expect(canvasDelete.defaultPrevented).toBe(true); - expect(timelineDelete).toHaveBeenCalledTimes(1); - expect(domDelete).not.toHaveBeenCalled(); + // The canvas holds a selection, so it owns the delete — the timeline's copy + // of that selection is derived and drops any member without a timeline row. + expect(domDelete).toHaveBeenCalledTimes(1); + expect(timelineDeleteMany).not.toHaveBeenCalled(); expect(keyframeDelete).not.toHaveBeenCalled(); }); }); diff --git a/packages/studio/src/hooks/useAppHotkeys.ts b/packages/studio/src/hooks/useAppHotkeys.ts index 3ae69a7e0..4325e8118 100644 --- a/packages/studio/src/hooks/useAppHotkeys.ts +++ b/packages/studio/src/hooks/useAppHotkeys.ts @@ -101,9 +101,12 @@ interface EditHistoryHandle { } interface UseAppHotkeysParams { - handleTimelineElementDelete: (element: TimelineElement) => Promise; + handleTimelineElementsDelete: (elements: TimelineElement[]) => Promise; handleTimelineElementSplit: (element: TimelineElement, splitTime: number) => Promise; - handleDomEditElementDelete: (selection: DomEditSelection) => Promise; + handleDomEditElementDelete: ( + selection: DomEditSelection, + options?: { expandGroup?: boolean }, + ) => Promise; domEditSelectionRef: React.MutableRefObject; clearDomSelectionRef: React.MutableRefObject<() => void>; editHistory: EditHistoryHandle; @@ -142,9 +145,12 @@ interface UseAppHotkeysParams { // ── Extracted keydown dispatch (pure function, no hooks) ── interface HotkeyCallbacks { - handleTimelineElementDelete: (element: TimelineElement) => Promise; + handleTimelineElementsDelete: (elements: TimelineElement[]) => Promise; handleTimelineElementSplit: (element: TimelineElement, splitTime: number) => Promise; - handleDomEditElementDelete: (selection: DomEditSelection) => Promise; + handleDomEditElementDelete: ( + selection: DomEditSelection, + options?: { expandGroup?: boolean }, + ) => Promise; handleUndo: () => Promise; handleRedo: () => Promise; handleCopy: () => boolean; @@ -322,24 +328,29 @@ export function dispatchPlainKey(event: KeyboardEvent, key: string, cb: HotkeyCa return; } } - // Delete acts on the primary selection OR the marquee multi-selection — - // the delete handler expands a clip that is part of the multi-selection - // into an atomic delete of the whole selection (single undo). - const { selectedElementId, selectedElementIds, elements } = usePlayerStore.getState(); - const selectionKeys = new Set(selectedElementIds); - if (selectedElementId) selectionKeys.add(selectedElementId); - if (selectionKeys.size > 0) { - const el = elements.find((e) => selectionKeys.has(e.key ?? e.id)); - if (el) { - event.preventDefault(); - void cb.handleTimelineElementDelete(el); - return; - } - } + // The canvas selection is what the user actually drew a marquee around, so + // it owns Delete whenever it holds something. The timeline mirror of that + // selection is derived and lossy — a member with no timeline row of its own + // is dropped from it — so deleting through the timeline removed the handful + // of clips it knew about and left every other selected element behind, + // still drawn as selected. The timeline path stays as the fallback for rows + // with no canvas node to select (audio, a comp that is not the active one). const domSel = cb.domEditSelectionRef.current; if (domSel) { event.preventDefault(); - void cb.handleDomEditElementDelete(domSel); + // The whole marquee group, not just the primary the ref holds. + void cb.handleDomEditElementDelete(domSel, { expandGroup: true }); + return; + } + // Takes the WHOLE selection: `find` returned the first match, so selecting + // every clip and pressing Delete removed exactly one of them. + const { selectedElementId, selectedElementIds, elements } = usePlayerStore.getState(); + const selectionKeys = new Set(selectedElementIds); + if (selectedElementId) selectionKeys.add(selectedElementId); + const selected = elements.filter((e) => selectionKeys.has(e.key ?? e.id)); + if (selected.length > 0) { + event.preventDefault(); + void cb.handleTimelineElementsDelete(selected); } return; } @@ -353,7 +364,7 @@ export function dispatchPlainKey(event: KeyboardEvent, key: string, cb: HotkeyCa // ── Hook ── export function useAppHotkeys({ - handleTimelineElementDelete, + handleTimelineElementsDelete, handleTimelineElementSplit, handleDomEditElementDelete, domEditSelectionRef, @@ -378,7 +389,6 @@ export function useAppHotkeys({ activeCompPath, forceReloadSdkSession, }: UseAppHotkeysParams) { - const previewHotkeyWindowRef = useRef(null); const previewHistoryCleanupRef = useRef<(() => void) | null>(null); // ── Undo / Redo ── @@ -454,7 +464,7 @@ export function useAppHotkeys({ const cbRef = useRef(null!); cbRef.current = { - handleTimelineElementDelete, + handleTimelineElementsDelete, handleTimelineElementSplit, handleDomEditElementDelete, handleUndo, @@ -492,33 +502,6 @@ export function useAppHotkeys({ // ── Preview iframe forwarding ── - const syncPreviewTimelineHotkey = useCallback( - (iframe: HTMLIFrameElement | null) => { - const nextWindow = iframeContentWindow(iframe); - if (previewHotkeyWindowRef.current === nextWindow) return; - safeRemoveListener( - previewHotkeyWindowRef.current, - "keydown", - handleAppKeyDown as EventListener, - ); - previewHotkeyWindowRef.current = nextWindow; - safeAddListener(nextWindow, "keydown", handleAppKeyDown as EventListener, true); - }, - [handleAppKeyDown], - ); - - useEffect( - () => () => { - safeRemoveListener( - previewHotkeyWindowRef.current, - "keydown", - handleAppKeyDown as EventListener, - ); - previewHotkeyWindowRef.current = null; - }, - [handleAppKeyDown], - ); - const handleHistoryHotkey = useCallback((event: KeyboardEvent) => { if (!(event.metaKey || event.ctrlKey) || shouldIgnoreHistoryShortcut(event.target)) return; handleUndoRedoKey( @@ -528,7 +511,18 @@ export function useAppHotkeys({ ); }, []); - const syncPreviewHistoryHotkey = useCallback( + /** + * Give the preview iframe the app's hotkeys, because a keypress lands in + * whichever document has focus and clicking the canvas puts focus in there. + * + * Must run on every iframe LOAD, not once when the element mounts: a reload + * keeps the same element (so no ref callback) and the same WindowProxy (so an + * identity check sees no change) while replacing the inner window that holds + * the listeners. Attaching once left Delete dead in the canvas after the first + * reload — press it with a selection and nothing happened, no toast, nothing + * to explain it — while undo/redo kept working because they re-attached here. + */ + const syncPreviewHotkeys = useCallback( (iframe: HTMLIFrameElement | null) => { previewHistoryCleanupRef.current?.(); previewHistoryCleanupRef.current = null; @@ -541,14 +535,19 @@ export function useAppHotkeys({ } if (!win && !doc) return; const handler = handleHistoryHotkey as EventListener; + const appHandler = handleAppKeyDown as EventListener; safeAddListener(win, "keydown", handler, true); + // Window only: the history pair also listens on the document, and a + // capture listener on both would run the app handler twice per press. + safeAddListener(win, "keydown", appHandler, true); doc?.addEventListener("keydown", handleHistoryHotkey, true); previewHistoryCleanupRef.current = () => { safeRemoveListener(win, "keydown", handler); + safeRemoveListener(win, "keydown", appHandler); doc?.removeEventListener("keydown", handleHistoryHotkey, true); }; }, - [handleHistoryHotkey], + [handleAppKeyDown, handleHistoryHotkey], ); useEffect( @@ -562,7 +561,6 @@ export function useAppHotkeys({ return { handleUndo, handleRedo, - syncPreviewTimelineHotkey, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, }; } diff --git a/packages/studio/src/hooks/useDomEditCommits.ts b/packages/studio/src/hooks/useDomEditCommits.ts index 71dc0cfd3..11deb45fd 100644 --- a/packages/studio/src/hooks/useDomEditCommits.ts +++ b/packages/studio/src/hooks/useDomEditCommits.ts @@ -420,20 +420,21 @@ export function useDomEditCommits({ // ── Element lifecycle (delete, z-index reorder) ── - const { handleDomEditElementDelete, handleDomZIndexReorderCommit } = useElementLifecycleOps({ - activeCompPath, - showToast, - writeProjectFile, - domEditSaveTimestampRef, - editHistory, - projectIdRef, - reloadPreview, - clearDomSelection, - onTrySdkDelete, - onReorderShadow, - forceReloadSdkSession, - commitDomEditPatchBatches, - }); + const { handleDomEditElementDelete, handleDomEditElementsDelete, handleDomZIndexReorderCommit } = + useElementLifecycleOps({ + activeCompPath, + showToast, + writeProjectFile, + domEditSaveTimestampRef, + editHistory, + projectIdRef, + reloadPreview, + clearDomSelection, + onTrySdkDelete, + onReorderShadow, + forceReloadSdkSession, + commitDomEditPatchBatches, + }); return { resolveImportedFontAsset, @@ -454,6 +455,7 @@ export function useDomEditCommits({ handleDomRotationCommit, handleDomManualEditsReset, handleDomEditElementDelete, + handleDomEditElementsDelete, handleDomZIndexReorderCommit, }; } diff --git a/packages/studio/src/hooks/useDomEditPreviewSync.ts b/packages/studio/src/hooks/useDomEditPreviewSync.ts index cb0c334fe..383c136b5 100644 --- a/packages/studio/src/hooks/useDomEditPreviewSync.ts +++ b/packages/studio/src/hooks/useDomEditPreviewSync.ts @@ -25,7 +25,7 @@ interface UseDomEditPreviewSyncParams { ) => void; buildDomSelectionFromTarget: (element: HTMLElement) => Promise; refreshPreviewDocumentVersion: () => void; - syncPreviewHistoryHotkey: (iframe: HTMLIFrameElement | null) => void; + syncPreviewHotkeys: (iframe: HTMLIFrameElement | null) => void; applyStudioManualEditsToPreviewRef: React.MutableRefObject< (iframe: HTMLIFrameElement) => Promise >; @@ -45,7 +45,7 @@ export function useDomEditPreviewSync({ refreshDomEditGroupSelectionsFromPreview, buildDomSelectionFromTarget, refreshPreviewDocumentVersion, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, applyStudioManualEditsToPreviewRef, openSourceForSelection, getSidebarTab, @@ -103,13 +103,13 @@ export function useDomEditPreviewSync({ } }; - syncPreviewHistoryHotkey(previewIframe); + syncPreviewHotkeys(previewIframe); void applyStudioManualEditsToPreviewRef.current(previewIframe); void syncSelectionFromDocument(); refreshPreviewDocumentVersion(); const handleLoad = () => { - syncPreviewHistoryHotkey(previewIframe); + syncPreviewHotkeys(previewIframe); void applyStudioManualEditsToPreviewRef.current(previewIframe); void syncSelectionFromDocument(); refreshPreviewDocumentVersion(); @@ -129,7 +129,7 @@ export function useDomEditPreviewSync({ previewIframe, refreshDomEditGroupSelectionsFromPreview, refreshPreviewDocumentVersion, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, applyStudioManualEditsToPreviewRef, gsapCacheVersion, ]); diff --git a/packages/studio/src/hooks/useDomEditSession.membersForDelete.test.ts b/packages/studio/src/hooks/useDomEditSession.membersForDelete.test.ts new file mode 100644 index 000000000..7d5da1721 --- /dev/null +++ b/packages/studio/src/hooks/useDomEditSession.membersForDelete.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from "vitest"; +import { membersForDelete } from "./useDomEditSession"; +import type { DomEditSelection } from "../components/editor/domEditingTypes"; + +const sel = (id: string) => ({ id }) as DomEditSelection; + +describe("membersForDelete", () => { + it("takes the whole group when the caller asks to expand", () => { + const group = [sel("a"), sel("b"), sel("c")]; + expect(membersForDelete(sel("a"), group, { expandGroup: true })).toEqual(group); + }); + + it("takes only the primary otherwise, so Cut removes what it copied", () => { + // Cut copies the primary alone. Expanding here put one element on the + // clipboard and removed every other member of the group with it. + const group = [sel("a"), sel("b"), sel("c")]; + expect(membersForDelete(sel("a"), group)).toEqual([sel("a")]); + }); + + it("falls back to the primary when there is no group", () => { + expect(membersForDelete(sel("a"), [], { expandGroup: true })).toEqual([sel("a")]); + }); +}); diff --git a/packages/studio/src/hooks/useDomEditSession.test.tsx b/packages/studio/src/hooks/useDomEditSession.test.tsx index e629b2d1b..2c77e887f 100644 --- a/packages/studio/src/hooks/useDomEditSession.test.tsx +++ b/packages/studio/src/hooks/useDomEditSession.test.tsx @@ -93,7 +93,7 @@ function createSessionParams( previewDocumentVersion: 0, rightPanelTab: "design", applyStudioManualEditsToPreviewRef: { current: async () => {} }, - syncPreviewHistoryHotkey: vi.fn(), + syncPreviewHotkeys: vi.fn(), reloadPreview: vi.fn(), setRefreshKey: vi.fn(), ...overrides, diff --git a/packages/studio/src/hooks/useDomEditSession.ts b/packages/studio/src/hooks/useDomEditSession.ts index dd5ca6637..5bdab03a6 100644 --- a/packages/studio/src/hooks/useDomEditSession.ts +++ b/packages/studio/src/hooks/useDomEditSession.ts @@ -20,6 +20,7 @@ import { useDomEditWiring } from "./useDomEditWiring"; import { useGsapAwareEditing } from "./useGsapAwareEditing"; import { useStudioSelectionPublisher } from "./useStudioSelectionPublisher"; import { useKeyframeEaseCommits } from "./useKeyframeEaseCommits"; +import type { DomEditSelection } from "../components/editor/domEditingTypes"; interface RecordEditInput { label: string; @@ -42,6 +43,7 @@ export interface UseDomEditSessionParams { setRightCollapsed: (collapsed: boolean) => void; setRightPanelTab: (tab: RightPanelTab) => void; showToast: (message: string, tone?: "error" | "info") => void; + isRecordingRef?: React.RefObject; refreshPreviewDocumentVersion: () => void; queueDomEditSave: (save: () => Promise) => Promise; readProjectFile: (path: string) => Promise; @@ -60,7 +62,7 @@ export interface UseDomEditSessionParams { applyStudioManualEditsToPreviewRef: React.MutableRefObject< (iframe: HTMLIFrameElement) => Promise >; - syncPreviewHistoryHotkey: (iframe: HTMLIFrameElement | null) => void; + syncPreviewHotkeys: (iframe: HTMLIFrameElement | null) => void; reloadPreview: () => void; setRefreshKey: React.Dispatch>; openSourceForSelection?: (sourceFile: string, target: PatchTarget) => void; @@ -71,6 +73,22 @@ export interface UseDomEditSessionParams { forceReloadSdkSession?: () => void; } +/** + * Which elements a delete acts on. `expandGroup` widens the primary to the + * whole marquee group, which is what the Delete key means. + * + * The caller chooses rather than the delete deciding for everyone: Cut copies + * the primary alone, so expanding for it put one element on the clipboard and + * removed every other member of the group with it. + */ +export function membersForDelete( + selection: DomEditSelection, + group: DomEditSelection[], + options?: { expandGroup?: boolean }, +): DomEditSelection[] { + return options?.expandGroup && group.length > 0 ? group : [selection]; +} + export function useDomEditSession({ projectId, activeCompPath, @@ -85,6 +103,7 @@ export function useDomEditSession({ setRightCollapsed, setRightPanelTab, showToast, + isRecordingRef, refreshPreviewDocumentVersion, queueDomEditSave, readProjectFile, @@ -101,7 +120,7 @@ export function useDomEditSession({ previewDocumentVersion, rightPanelTab, applyStudioManualEditsToPreviewRef, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, reloadPreview, setRefreshKey: _setRefreshKey, openSourceForSelection, @@ -238,7 +257,7 @@ export function useDomEditSession({ handleDomRemoveTextField, handleDomBoxSizeCommit, handleDomManualEditsReset, - handleDomEditElementDelete, + handleDomEditElementsDelete, handleDomZIndexReorderCommit, } = useDomEditCommits({ activeCompPath, @@ -332,6 +351,21 @@ export function useDomEditSession({ forceReloadSdkSession, }); + const handleDomEditElementDelete = useCallback( + async (selection: DomEditSelection, options?: { expandGroup?: boolean }) => { + // Same structural edit the timeline delete refuses mid-recording, so it + // refuses here too — this is now the path a Delete press takes whenever + // the canvas holds a selection. + if (isRecordingRef?.current) { + showToast("Cannot edit timeline while recording", "error"); + return; + } + const members = membersForDelete(selection, domEditGroupSelectionsRef.current, options); + await handleDomEditElementsDelete(members); + }, + [domEditGroupSelectionsRef, handleDomEditElementsDelete, isRecordingRef, showToast], + ); + const handleGroupSelection = useCallback(() => { const group = domEditGroupSelectionsRef.current; const single = domEditSelectionRef.current; @@ -400,7 +434,7 @@ export function useDomEditSession({ bumpGsapCache, showToast, refreshPreviewDocumentVersion, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, applyStudioManualEditsToPreviewRef, applyDomSelection, buildDomSelectionFromTarget, diff --git a/packages/studio/src/hooks/useDomEditWiring.ts b/packages/studio/src/hooks/useDomEditWiring.ts index 9b49f16fa..9b806957d 100644 --- a/packages/studio/src/hooks/useDomEditWiring.ts +++ b/packages/studio/src/hooks/useDomEditWiring.ts @@ -33,7 +33,7 @@ export interface UseDomEditWiringParams { bumpGsapCache: () => void; showToast: (message: string, tone?: "error" | "info") => void; refreshPreviewDocumentVersion: () => void; - syncPreviewHistoryHotkey: (iframe: HTMLIFrameElement | null) => void; + syncPreviewHotkeys: (iframe: HTMLIFrameElement | null) => void; applyStudioManualEditsToPreviewRef: React.MutableRefObject< (iframe: HTMLIFrameElement) => Promise >; @@ -127,7 +127,7 @@ export function useDomEditWiring({ bumpGsapCache, showToast, refreshPreviewDocumentVersion, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, applyStudioManualEditsToPreviewRef, applyDomSelection, buildDomSelectionFromTarget, @@ -264,7 +264,7 @@ export function useDomEditWiring({ refreshDomEditGroupSelectionsFromPreview, buildDomSelectionFromTarget, refreshPreviewDocumentVersion, - syncPreviewHistoryHotkey, + syncPreviewHotkeys, applyStudioManualEditsToPreviewRef, openSourceForSelection, getSidebarTab, diff --git a/packages/studio/src/hooks/useDomSelection.ts b/packages/studio/src/hooks/useDomSelection.ts index 115d54fca..62c36b527 100644 --- a/packages/studio/src/hooks/useDomSelection.ts +++ b/packages/studio/src/hooks/useDomSelection.ts @@ -29,6 +29,9 @@ export interface ApplyDomSelectionOptions { revealPanel?: boolean; additive?: boolean; preserveGroup?: boolean; + // A clear that came FROM the timeline must not be echoed back, or picking a + // clip with no canvas node would deselect the clip you just picked. + announce?: boolean; } export interface ResolveDomSelectionOptions { @@ -170,21 +173,14 @@ export function useDomSelection({ const applyDomSelection = useCallback( // fallow-ignore-next-line complexity - ( - selection: DomEditSelection | null, - options?: { - revealPanel?: boolean; - additive?: boolean; - preserveGroup?: boolean; - }, - ) => { + (selection: DomEditSelection | null, options?: ApplyDomSelectionOptions) => { if (!selection) { logSelect("clear", { hadGroup: domEditGroupSelectionsRef.current.length }); domEditSelectionRef.current = null; domEditGroupSelectionsRef.current = []; setDomEditSelection(null); setDomEditGroupSelections([]); - announceTimelineSelection([], null); + if (options?.announce !== false) announceTimelineSelection([], null); return; } @@ -393,7 +389,14 @@ export function useDomSelection({ const selection = await buildDomSelectionForTimelineElement(element); // A newer selection superseded this one while we were resolving — drop the stale result. if (seq !== timelineSelectSeqRef.current) return; - if (selection) applyDomSelection(selection); + if (selection) { + applyDomSelection(selection); + return; + } + // No canvas node (audio, a comp that is not the active one). Leaving the + // previous selection pointed the canvas at something the user did not pick, + // and Delete acts on the canvas first — so it removed that, not the clip. + applyDomSelection(null, { revealPanel: false, announce: false }); }, [applyDomSelection, buildDomSelectionForTimelineElement], ); diff --git a/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts b/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts index a61ee38fa..ca758b494 100644 --- a/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts +++ b/packages/studio/src/hooks/useDomSelectionSelectionGuards.test.ts @@ -278,3 +278,52 @@ describe("useDomSelection — marquee multi-select survives the late async prima iframe.remove(); }); }); + +describe("useDomSelection — picking a clip with no canvas node", () => { + function timelineEl(id: string): TimelineElement { + return { id, domId: id, tag: "div", start: 0, duration: 1, track: 0 }; + } + + it("drops the canvas selection without deselecting the clip", async () => { + // Delete acts on the canvas first, so a canvas selection left pointing at + // the previous element removed THAT element when the user pressed Delete + // right after picking an audio clip. Clearing it has to stay quiet, though: + // announcing the clear back would deselect the clip that was just picked. + const iframe = document.createElement("iframe"); + document.body.append(iframe); + const doc = iframe.contentDocument!; + const onCanvas = doc.createElement("div"); + onCanvas.id = "on-canvas"; + doc.body.append(onCanvas); + + const setSelectedTimelineElementId = vi.fn(); + const setTimelineSelectionSet = vi.fn(); + const harness = renderHarness({ + rightPanelTab: "design", + setRightPanelTab: vi.fn(), + iframe, + timelineElements: [timelineEl("on-canvas"), timelineEl("audio-only")], + setSelectedTimelineElementId, + setTimelineSelectionSet, + }); + + await act(async () => { + const pending = harness.current().handleTimelineElementSelect(timelineEl("on-canvas")); + deferreds.get("on-canvas")?.resolve(); + await pending; + }); + expect(harness.current().domEditSelectionRef.current).not.toBeNull(); + + setSelectedTimelineElementId.mockClear(); + setTimelineSelectionSet.mockClear(); + await act(async () => { + await harness.current().handleTimelineElementSelect(timelineEl("audio-only")); + }); + + expect(harness.current().domEditSelectionRef.current).toBeNull(); + expect(setSelectedTimelineElementId).not.toHaveBeenCalled(); + expect(setTimelineSelectionSet).not.toHaveBeenCalled(); + + harness.cleanup(); + }); +}); diff --git a/packages/studio/src/hooks/useElementLifecycleOps.multiDelete.test.tsx b/packages/studio/src/hooks/useElementLifecycleOps.multiDelete.test.tsx new file mode 100644 index 000000000..245bc5d84 --- /dev/null +++ b/packages/studio/src/hooks/useElementLifecycleOps.multiDelete.test.tsx @@ -0,0 +1,100 @@ +// @vitest-environment happy-dom + +import React, { act } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useElementLifecycleOps } from "./useElementLifecycleOps"; +import { makeLifecycleOpsParams } from "./elementLifecycleOpsTestUtils"; +import { mountReactHarness, makeSelection } from "./domSelectionTestHarness"; + +function selectionFor(id: string) { + const el = document.createElement("div"); + el.id = id; + document.body.append(el); + return { ...makeSelection(id, el), sourceFile: "index.html" }; +} + +describe("useElementLifecycleOps — deleting a canvas multi-selection", () => { + const removed: string[] = []; + const requests: string[] = []; + let changes = true; + + beforeEach(() => { + removed.length = 0; + requests.length = 0; + changes = true; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init?: RequestInit) => { + requests.push(String(url)); + const body = JSON.parse(String(init?.body ?? "{}")) as { + targets?: { id?: string; selector?: string }[]; + }; + for (const target of body.targets ?? []) { + const key = target.id ?? target.selector; + if (key) removed.push(key); + } + return { + ok: true, + json: async () => ({ changed: changes, content: "" }), + } as unknown as Response; + }), + ); + }); + afterEach(() => { + vi.unstubAllGlobals(); + document.body.innerHTML = ""; + }); + + it("removes every selected element, not just the first", async () => { + // The reported bug: select several elements on the canvas, press Delete, and + // one disappears while the rest stay — still drawn as selected. + let ops: ReturnType | null = null; + function Probe() { + ops = useElementLifecycleOps( + makeLifecycleOpsParams({ + commitDomEditPatchBatches: vi.fn(async () => ({ ok: true }) as never), + projectIdRef: { current: "p1" }, + }), + ); + return null; + } + mountReactHarness(); + + const selections = ["a", "b", "c"].map(selectionFor); + await act(async () => { + await ops!.handleDomEditElementsDelete(selections); + }); + + // The defect: only the first was ever removed. + expect(removed).toEqual(["a", "b", "c"]); + // And one request for the selection, not one per member: a canvas selection + // runs to hundreds, and a round trip each made Delete look like a no-op. + expect(requests.filter((url) => url.includes("remove-elements"))).toHaveLength(1); + }); + + it("says so when the preview is stale instead of claiming a delete", async () => { + // Every target missing means the preview is describing a document the file + // does not have. Reporting success there is what read as Delete doing + // nothing at all, with nothing on screen to explain it. + changes = false; + const showToast = vi.fn(); + let ops: ReturnType | null = null; + function Probe() { + ops = useElementLifecycleOps( + makeLifecycleOpsParams({ + commitDomEditPatchBatches: vi.fn(async () => ({ ok: true }) as never), + projectIdRef: { current: "p1" }, + showToast, + }), + ); + return null; + } + mountReactHarness(); + + await act(async () => { + await ops!.handleDomEditElementsDelete([selectionFor("a")]); + }); + + expect(showToast.mock.calls.flat().join(" ")).toContain("out of date"); + }); +}); diff --git a/packages/studio/src/hooks/useElementLifecycleOps.ts b/packages/studio/src/hooks/useElementLifecycleOps.ts index 07b98324b..94076151a 100644 --- a/packages/studio/src/hooks/useElementLifecycleOps.ts +++ b/packages/studio/src/hooks/useElementLifecycleOps.ts @@ -85,39 +85,77 @@ export function useElementLifecycleOps({ onElementDeleted, }: UseElementLifecycleOpsParams) { // fallow-ignore-next-line complexity - const handleDomEditElementDelete = useCallback( + const handleDomEditElementsDelete = useCallback( // fallow-ignore-next-line complexity - async (selection: DomEditSelection) => { + async (selections: DomEditSelection[]) => { const pid = projectIdRef.current; if (!pid) return; - const label = selection.label || selection.id || selection.selector || selection.tagName; + const [selection] = selections; + if (!selection) return; + const label = + selections.length === 1 + ? selection.label || selection.id || selection.selector || selection.tagName + : `${selections.length} elements`; + // Say the press landed before doing the work. Deleting a marquee selection + // takes seconds — reading the file, removing every member, saving, then + // reloading the preview — and until it finishes the canvas looks exactly + // like it did before. With nothing acknowledging the key, that silence is + // indistinguishable from Delete being broken, which is how it got read. + if (selections.length > 1) showToast(`Deleting ${label}...`, "info"); + // One file per pass; anything authored elsewhere is dropped rather than + // patched into the wrong document. const targetPath = selection.sourceFile || activeCompPath || "index.html"; + const sameFile = selections.filter( + (candidate) => (candidate.sourceFile || activeCompPath || "index.html") === targetPath, + ); try { const originalContent = await readProjectFileContent(pid, targetPath); - const patchTarget = buildDomEditPatchTarget(selection); - if (!patchTarget.id && !patchTarget.selector && !patchTarget.hfId) { + const patchTargets = sameFile.map((member) => buildDomEditPatchTarget(member)); + if (patchTargets.some((t) => !t.id && !t.selector && !t.hfId)) { throw new Error("Selected element has no patchable target"); } - if (onTrySdkDelete && selection.hfId) { - const handled = await onTrySdkDelete(selection.hfId, originalContent, targetPath); - if (cutoverCommittedOrThrow(handled)) { + // The SDK path can take the whole selection only when every member is + // addressable in the SDK doc; otherwise fall through to REST for all of + // them rather than deleting a subset through each route. + const hfIds = sameFile + .map((member) => member.hfId) + .filter((hfId): hfId is string => Boolean(hfId)); + if (onTrySdkDelete && hfIds.length === sameFile.length) { + let allHandled = true; + for (const hfId of hfIds) { + // The SDK owns the document it edits, so every member is removed + // against the same starting content rather than a threaded copy. + const handled = await onTrySdkDelete(hfId, originalContent, targetPath); + if (!cutoverCommittedOrThrow(handled)) { + allHandled = false; + break; + } + } + if (allHandled) { clearDomSelection(); usePlayerStore.getState().setSelectedElementId(null); - showToast(`Deleted ${label}. Use Undo to restore it.`, "info"); + showToast( + `Deleted ${label}. Use Undo to restore ${sameFile.length === 1 ? "it" : "them"}.`, + "info", + ); return; } } domEditSaveTimestampRef.current = Date.now(); + // One request for the whole selection. Removing members one at a time + // cost a round trip and a rewrite of the file EACH, and a canvas + // selection runs to hundreds of members — the file ended up correct, but + // only after long enough that Delete looked like it had done nothing. const removeResponse = await fetch( - `/api/projects/${pid}/file-mutations/remove-element/${encodeURIComponent(targetPath)}`, + `/api/projects/${pid}/file-mutations/remove-elements/${encodeURIComponent(targetPath)}`, { method: "POST", headers: { "Content-Type": "application/json", ...studioWriteHeaders() }, - body: JSON.stringify({ target: patchTarget }), + body: JSON.stringify({ targets: patchTargets }), }, ); if (!removeResponse.ok) { @@ -126,8 +164,18 @@ export function useElementLifecycleOps({ `Failed to delete element from ${targetPath}`, ); } - - const removeData = (await removeResponse.json()) as { changed?: boolean; content?: string }; + const removeData = (await removeResponse.json()) as { + changed?: boolean; + content?: string; + }; + if (!removeData.changed) { + // A member the file no longer holds simply does not match, which is + // normal for one nested inside another member already removed. Nothing + // matching at all means the preview is describing a document the file + // does not have — say so rather than reporting a delete that happened. + reloadPreview(); + throw new Error("Nothing to delete — the preview was out of date. Try again."); + } const patchedContent = typeof removeData.content === "string" ? removeData.content : originalContent; // ponytail: the server remove-element route (removeElementFromHtml) strips @@ -155,8 +203,11 @@ export function useElementLifecycleOps({ // SDK edit doesn't resurrect the deleted element. forceReloadSdkSession?.(); reloadPreview(); - onElementDeleted?.(selection); - showToast(`Deleted ${label}. Use Undo to restore it.`, "info"); + for (const member of sameFile) onElementDeleted?.(member); + showToast( + `Deleted ${label}. Use Undo to restore ${sameFile.length === 1 ? "it" : "them"}.`, + "info", + ); } catch (error) { const message = error instanceof Error ? error.message : "Failed to delete element"; showToast(message); @@ -337,8 +388,16 @@ export function useElementLifecycleOps({ [commitDomEditPatchBatches, onReorderShadow], ); + const handleDomEditElementDelete = useCallback( + async (selection: DomEditSelection) => { + await handleDomEditElementsDelete([selection]); + }, + [handleDomEditElementsDelete], + ); + return { handleDomEditElementDelete, + handleDomEditElementsDelete, handleDomZIndexReorderCommit, }; } diff --git a/packages/studio/src/hooks/useStudioUrlState.hydration.test.ts b/packages/studio/src/hooks/useStudioUrlState.hydration.test.ts new file mode 100644 index 000000000..711a471c7 --- /dev/null +++ b/packages/studio/src/hooks/useStudioUrlState.hydration.test.ts @@ -0,0 +1,39 @@ +// @vitest-environment happy-dom +import { describe, expect, it, vi } from "vitest"; +import { resolveUrlSelections } from "./useStudioUrlState"; + +describe("restoring a selection from the URL", () => { + it("probes the source for the primary only, never for the group", async () => { + // Each probe is its own request and they are awaited one after another, + // while the URL carries the whole selection — so a marquee over a captured + // page turned every later reload into hundreds of serial round trips before + // the canvas answered anything. + const doc = document.implementation.createHTMLDocument(); + const ids = ["a", "b", "c", "d"]; + for (const id of ids) { + const el = doc.createElement("div"); + el.id = id; + doc.body.append(el); + } + const probed: (boolean | undefined)[] = []; + const buildDomSelection = vi.fn( + async (element: HTMLElement, options?: { skipSourceProbe?: boolean }) => { + probed.push(options?.skipSourceProbe); + return { element, id: element.id } as never; + }, + ); + + await resolveUrlSelections({ + doc, + primaryElement: doc.getElementById("a") as HTMLElement, + selection: { sourceFile: "index.html" } as never, + group: ids.slice(1).map((id) => ({ id, sourceFile: "index.html" })) as never, + activeCompPath: "index.html", + isCurrent: () => true, + buildDomSelection, + }); + + expect(probed[0]).toBeUndefined(); + expect(probed.slice(1)).toEqual([true, true, true]); + }); +}); diff --git a/packages/studio/src/hooks/useStudioUrlState.ts b/packages/studio/src/hooks/useStudioUrlState.ts index 629f04e51..03b628c81 100644 --- a/packages/studio/src/hooks/useStudioUrlState.ts +++ b/packages/studio/src/hooks/useStudioUrlState.ts @@ -27,7 +27,7 @@ interface UseStudioUrlStateParams { applyMarqueeSelection: (selections: DomEditSelection[], additive: boolean) => void; buildDomSelectionFromTarget: ( target: HTMLElement, - options?: { preferClipAncestor?: boolean }, + options?: { preferClipAncestor?: boolean; skipSourceProbe?: boolean }, ) => Promise; applyDomSelection: ( selection: DomEditSelection | null, @@ -124,10 +124,16 @@ async function buildOptionalDomSelection( buildDomSelection: UseStudioUrlStateParams["buildDomSelectionFromTarget"], ): Promise { if (!element) return null; - return buildDomSelection(element, { preferClipAncestor: false }); + // No source probe for group members. Each one costs a request, they are + // resolved one after another, and the URL carries the whole selection — so a + // marquee over a captured page turned every reload into hundreds of serial + // round trips before the canvas answered anything, including a Delete press. + // The marquee that produced these members already skips the probe for the + // same reason; only the primary, whose panel reads the flag, still pays it. + return buildDomSelection(element, { preferClipAncestor: false, skipSourceProbe: true }); } -async function resolveUrlSelections({ +export async function resolveUrlSelections({ doc, primaryElement, selection, diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index e8f5ddc9b..f59b82c2a 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -389,44 +389,59 @@ export function useTimelineEditing({ }); // fallow-ignore-next-line complexity - const handleTimelineElementDelete = useCallback( + const handleTimelineElementsDelete = useCallback( // fallow-ignore-next-line complexity - async (element: TimelineElement) => { + async (selection: TimelineElement[]) => { if (isRecordingRef?.current) { showToast("Cannot edit timeline while recording", "error"); return; } const pid = projectIdRef.current; if (!pid) throw new Error("No active project"); - const label = getTimelineElementLabel(element); + const [element] = selection; + if (!element) return; + const label = + selection.length === 1 ? getTimelineElementLabel(element) : `${selection.length} clips`; + // One file per delete pass. Every element in a marquee selection lives in + // the composition being edited, so they share a target; anything that + // does not is dropped rather than written to the wrong file. const targetPath = element.sourceFile || activeCompPath || "index.html"; + const sameFile = selection.filter( + (candidate) => (candidate.sourceFile || activeCompPath || "index.html") === targetPath, + ); try { const originalContent = await readFileContent(pid, targetPath); - const patchTarget = buildPatchTarget(element); - if (!patchTarget) { - throw new Error(`Timeline element ${element.id} is missing a patchable target`); - } + // Remove every selected element before saving once. The server rewrites + // the file per call, so `removedContent` after the last one holds them + // all — which is what makes this a single history entry, and a single + // undo, rather than one per clip. + let removedContent = originalContent; + for (const target of sameFile) { + const patchTarget = buildPatchTarget(target); + if (!patchTarget) { + throw new Error(`Timeline element ${target.id} is missing a patchable target`); + } - const removeResponse = await fetch( - `/api/projects/${pid}/file-mutations/remove-element/${encodeURIComponent(targetPath)}`, - { - method: "POST", - headers: { "Content-Type": "application/json", ...studioWriteHeaders() }, - body: JSON.stringify({ target: patchTarget }), - }, - ); - if (!removeResponse.ok) { - throw new Error(`Failed to delete ${element.id} from ${targetPath}`); - } + const removeResponse = await fetch( + `/api/projects/${pid}/file-mutations/remove-element/${encodeURIComponent(targetPath)}`, + { + method: "POST", + headers: { "Content-Type": "application/json", ...studioWriteHeaders() }, + body: JSON.stringify({ target: patchTarget }), + }, + ); + if (!removeResponse.ok) { + throw new Error(`Failed to delete ${target.id} from ${targetPath}`); + } - const removeData = (await removeResponse.json()) as { - changed?: boolean; - content?: string; - }; - const removedContent = - typeof removeData.content === "string" ? removeData.content : originalContent; + const removeData = (await removeResponse.json()) as { + changed?: boolean; + content?: string; + }; + if (typeof removeData.content === "string") removedContent = removeData.content; + } // Content-driven duration: shrink the composition to the furthest // remaining clip end, read from the post-removal SOURCE (raw // data-duration), so deleting the last/longest clip removes trailing @@ -460,15 +475,18 @@ export function useTimelineEditing({ throw error; } + const deletedKeys = new Set(sameFile.map((te) => te.key ?? te.id)); usePlayerStore .getState() - .setElements( - timelineElements.filter((te) => (te.key ?? te.id) !== (element.key ?? element.id)), - ); + .setElements(timelineElements.filter((te) => !deletedKeys.has(te.key ?? te.id))); usePlayerStore.getState().setSelectedElementId(null); + usePlayerStore.getState().setSelectedElementIds(new Set()); forceReloadSdkSession?.(); reloadPreview(); - showToast(`Deleted ${label}. Use Undo to restore it.`, "info"); + showToast( + `Deleted ${label}. Use Undo to restore ${sameFile.length === 1 ? "it" : "them"}.`, + "info", + ); } catch (error) { const message = error instanceof Error ? error.message : "Failed to delete timeline clip"; showToast(message); @@ -488,6 +506,14 @@ export function useTimelineEditing({ ], ); + /** Single-clip delete — the context menu and clip chrome path. */ + const handleTimelineElementDelete = useCallback( + async (element: TimelineElement) => { + await handleTimelineElementsDelete([element]); + }, + [handleTimelineElementsDelete], + ); + const { handleTimelineAssetDrop, handleTimelineFileDrop, handleTimelineCompositionDrop } = useTimelineAssetDropOps({ projectIdRef, @@ -533,6 +559,7 @@ export function useTimelineEditing({ handleToggleTrackHidden, handleToggleElementHidden, handleTimelineElementDelete, + handleTimelineElementsDelete, handleTimelineElementSplit: handleRazorSplit, handleRazorSplit, handleRazorSplitAll, diff --git a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.test.tsx b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.test.tsx index 2ffc3442d..aa2922fe0 100644 --- a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.test.tsx +++ b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.test.tsx @@ -143,6 +143,38 @@ describe("useTimelineSelectionPreviewSync", () => { harness.cleanup(); }); + it("drops a canvas selection that points outside the timeline selection", async () => { + // The reveal paths (sidebar audio/asset click, asset drop) select a clip + // with no canvas node. Bailing here kept whatever the canvas held, and + // Delete acts on the canvas first — so pressing it deleted the element the + // user had selected before, and left the clip they had just picked. + const { firstSelection, timelineElements } = makeSyncFixture(); + const applyDomSelection = vi.fn(); + const applyMarqueeSelection = vi.fn(); + // clip-2 is a timeline element with no DOM node — an audio clip. + const buildDomSelectionForTimelineElement = vi.fn(async () => null); + const harness = renderHarness(); + + await harness.rerender({ + selectedElementId: "clip-2", + selectedElementIds: new Set(["clip-2"]), + timelineElements, + domEditSelection: firstSelection, + domEditGroupSelections: [firstSelection], + buildDomSelectionForTimelineElement, + applyDomSelection, + applyMarqueeSelection, + onSelectionNotFound: vi.fn(), + }); + + // Quietly: announcing the clear would deselect the clip just picked. + expect(applyDomSelection).toHaveBeenCalledWith(null, { + revealPanel: false, + announce: false, + }); + harness.cleanup(); + }); + it("warns once while retrying a timeline selection after preview refreshes", async () => { const { secondSelection, timelineElements } = makeSyncFixture(); const applyDomSelection = vi.fn(); diff --git a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts index f57ebcd2e..79752e3f6 100644 --- a/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts +++ b/packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts @@ -16,7 +16,12 @@ interface UseTimelineSelectionPreviewSyncParams { ) => Promise; applyDomSelection: ( selection: DomEditSelection | null, - options?: { revealPanel?: boolean; additive?: boolean; preserveGroup?: boolean }, + options?: { + revealPanel?: boolean; + additive?: boolean; + preserveGroup?: boolean; + announce?: boolean; + }, ) => void; applyMarqueeSelection: (selections: DomEditSelection[], additive: boolean) => void; onSelectionNotFound: () => void; @@ -48,6 +53,16 @@ function selectionIdsMatch( return currentAnchor === wantedAnchor; } +/** + * The invariant this file owes the Delete key, now that Delete prefers the + * canvas: the canvas selection never points outside the current timeline + * selection. A member still resolving has no anchor of its own yet, so it is + * not caught here — only a canvas selection that belongs to something else. + */ +function anchorIsOutsideSelection(anchor: string | null, selectedIds: string[]): boolean { + return anchor !== null && !selectedIds.includes(anchor); +} + export function useTimelineSelectionPreviewSync({ selectedElementId, selectedElementIds, @@ -112,6 +127,12 @@ export function useTimelineSelectionPreviewSync({ } let cancelled = false; + // One warning per selection, however many times the effect retries it. + const warnSelectionMissingOnce = () => { + if (missingSelectionKeyRef.current === selectedKey) return; + missingSelectionKeyRef.current = selectedKey; + onSelectionNotFound(); + }; const syncSelection = async () => { const selections: DomEditSelection[] = []; let resolvableCount = 0; @@ -128,9 +149,15 @@ export function useTimelineSelectionPreviewSync({ // Bail instead; a later effect run (on timelineElements/DOM change) applies the // full set once every resolvable member has a live node. if (selections.length < resolvableCount) { - if (missingSelectionKeyRef.current !== selectedKey) { - missingSelectionKeyRef.current = selectedKey; - onSelectionNotFound(); + warnSelectionMissingOnce(); + // Bailing keeps whatever the canvas already held, and Delete acts on the + // canvas first — so an anchor pointing OUTSIDE this selection is an + // element the user is no longer looking at, and deleting it is the + // damage. Only that goes: a member still resolving has no anchor of its + // own here and is left for the later run. Quietly, because announcing + // the clear would deselect the clip that was just picked. + if (anchorIsOutsideSelection(currentAnchor, selectedIds)) { + applyDomSelection(null, { revealPanel: false, announce: false }); } return; }