fix(studio): make Delete remove the whole canvas selection (#3339)

* 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.
This commit is contained in:
Miguel Ángel
2026-08-19 00:22:26 -04:00
committed by GitHub
parent 0e3c5f6bef
commit ec0b23f3ce
30 changed files with 887 additions and 214 deletions
@@ -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<string, string> = {
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]);
@@ -34,7 +34,7 @@ function AppHotkeyHarness() {
const leftSidebarRef = useRef<LeftSidebarHandle | null>(null);
useAppHotkeys({
handleTimelineElementDelete: vi.fn(),
handleTimelineElementsDelete: vi.fn(async () => {}),
handleTimelineElementSplit: vi.fn(),
handleDomEditElementDelete: vi.fn(),
domEditSelectionRef,
@@ -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);
});
});
@@ -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 [];
@@ -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<DomEditSelection[]> {
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<Rect[]>([]);
// Measured once when the drag passes the threshold and reused until it ends.
const candidatesRef = useRef<MarqueeHit[] | null>(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);
@@ -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<void> {
await new Promise<void>((resolve) => setTimeout(resolve, RECOMPUTE_INTERVAL_MS + 5));
await new Promise<void>((resolve) => {
requestAnimationFrame(() => requestAnimationFrame(() => resolve()));
});
@@ -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);
});
});
@@ -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,
@@ -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;