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

[miguel-heygen]

Selecting elements in Studio and pressing Delete left most of them behind, and at large selections looked like the key did nothing at all. Several independent defects stacked on the same gesture.

The selection was never the whole selection

The marquee could only see the first 80 elements. Its hit test drew candidates from the layers-panel collector, which stops after 80 items — a budget for how many rows that panel renders, reused as if it described the document. Everything past the 80th element in document order was unselectable no matter where you dragged, so "select all" never selected all. The off-canvas indicators read the same truncated list. The cap now belongs to the panel that asks for it; the collector returns the whole document.

To afford that, the marquee measures its candidates once when the drag passes the threshold instead of reading layout for every element on every pointer-move. Unbounded plus per-move stalls the tab outright; the iframe DOM does not mutate mid-drag, so one pass stays true for the gesture.

Delete acted on a fraction of it

The canvas selection did not own Delete. The hotkey routed to the timeline delete whenever the timeline store held anything, and the timeline's copy of a canvas selection is lossy by construction — announceTimelineSelection drops every member with no timeline row of its own. Both paths remove through the same endpoint, so this replaces two addressing schemes with one; the timeline path stays as the fallback for rows with no canvas node (audio, an inactive comp).

The canvas delete only took the primary, ignoring the marquee group it belonged to. The timeline delete only took the first clip, because it resolved the selection with find(). Every member is now removed under a single save, so one ⌘Z restores the whole selection.

And then it looked broken even when it worked

Reproduced with a real, focus-routed keypress rather than a synthetic one: the press reaches the handler and the delete runs to completion, but at hundreds of members it took seconds during which the canvas was unchanged and nothing acknowledged the key. Silence that long is indistinguishable from Delete being broken — and pressing again or reloading mid-flight lands somewhere worse.

• One request for the whole selection. It was a round trip and a full rewrite of the file per element. A new remove-elements route reads once, drops every member, writes once. Measured on 84 members: 933ms across 84 rewrites, down to 583ms and one.
• The press is acknowledged before the work starts, so a multi-element delete says what it is doing.
• Restoring a selection no longer re-probes every member. The hash carries the whole selection and hydrating it asked the server whether each member still exists — one request each, serially. Every later load of such a URL paid it again. The marquee that produced those members already skips the probe; restoring them does too.
• A no-op delete says so instead of reporting "Deleted 503 elements" when nothing matched.

Also

Removes the document-level dblclick listener that reset preview zoom. Double-clicking anywhere in the canvas — including into a text element to edit it — silently threw away the zoom you had set. The explicit reset control is unchanged.

Verified

End to end in Studio on a captured page, driving a real keypress:

	before	after
marquee over the whole canvas	80 selected, whatever the drag covered	442
one Delete	most of the page left behind	734 elements → 81, canvas clear
feedback	none until it finished	"Deleting 442 elements…" → "Deleted 442 elements."

Selecting a clip in the timeline and deleting it still works; it goes through the canvas path now and removes the same row.

Tests

• domEditingLayers — the collector returns the whole document by default and truncates only when a caller passes a budget.
• useAppHotkeys — a canvas selection gets its whole group instead of the timeline's partial copy; Delete over a multi-clip timeline selection removes all of them; hotkeys survive a preview reload.
• useElementLifecycleOps.multiDelete — every member is removed, in one request, and a stale preview is reported rather than claimed as a delete.
• useStudioUrlState.hydration — restoring a selection probes the primary only.

Each was mutation-checked: restoring the old behaviour turns the matching test red.

[jrusso1020]

Reviewed at e1a80900. Four real defects fixed on one gesture, and the marquee cap diagnosis is exactly right. One blocker: the Delete reorder makes the fallback its own comment promises unreachable, and in that state Delete removes an element the user is not looking at.

Audited end-to-end: useAppHotkeys.ts (dispatch + preview forwarding), useElementLifecycleOps.ts, useDomEditSession.ts, useTimelineEditing.ts, marqueeCommit.ts, domEditingLayers.ts, LayersPanel.tsx, studio-server/routes/files.ts (new route only), plus the untouched neighbours the arbitration depends on — useDomSelection.ts, useTimelineSelectionPreviewSync.ts, useClipboard.ts, EditorShell.tsx, useCanvasContextMenuState.ts, offCanvasIndicatorRefresh.ts. Read for context: the four test files, useDomEditCommits.ts / useDomEditWiring.ts / useDomEditPreviewSync.ts (mechanical rename). Not executed: I did not run the suites locally, so everything below is a source read; CI at this head is the execution evidence.

CI, verified rather than inferred: all 8 required contexts green at e1a80900 (Build, Test, Typecheck, Test: runtime contract, regression, Semantic PR title, Tests on windows-latest, Render on windows-latest). BLOCKED is the reviewer gate alone; base is current (behind_by: 0). No prior human review — the only review on the PR is the CodeQL bot at the first commit 107d1ca0, on useTimelineEditing.ts (pre-existing: the same remove-element fetch with the same encodeURIComponent(targetPath) URL exists on main; the diff moved it into a loop, it did not introduce the pattern).

Strengths

• The cap diagnosis is the good kind of finding: a number that described one panel's rendering budget being read as a property of the document. Re-homing it as LAYERS_PANEL_MAX_ROWS (LayersPanel.tsx:27,148) with the invariant written onto the collector puts the constant where its meaning lives. I checked the enumeration is complete — the collector has exactly three non-test callers, and pre-PR both hit-testing ones silently took 80 (marqueeCommit.ts:48, offCanvasIndicatorGeometry.ts:60, neither passing a budget), so the body's claim that the off-canvas indicators read the same truncated list holds at source.
• Single-undo atomicity is real, not just asserted. useTimelineEditing.ts:409-433 loops the removals and saves once after the loop, and the canvas path sends one request for the whole selection — so one ⌘Z restores the set. That is the property most easily lost when a single-item handler grows a loop.
• The preview-reload fix is a second genuine bug, and its test pins the mechanism rather than the symptom. useAppHotkeys.previewForwarding.test.tsx re-syncs the same iframe element twice and asserts Delete still lands — reproducing the "same element, same WindowProxy, replaced inner window" case precisely, which is the only shape under which an identity check silently no-ops. The window-only attach with the double-fire reasoning stated inline (useAppHotkeys.ts:531-533) is the right call and explains itself.
• The dblclick removal left nothing dead — I checked, because a removed listener usually strands its helpers: isPreviewAtFit and DEFAULT_PREVIEW_ZOOM still serve the HUD and the explicit reset control (NLEPreview.tsx:230,251,494-498), and no test covered the removed behaviour.

blocker — Delete now acts on a stale canvas selection, and the fallback the comment promises cannot be reached

useAppHotkeys.ts:332-337 gives the canvas selection Delete whenever domEditSelectionRef.current is non-null, and the comment above it scopes the exception precisely: "The timeline path stays as the fallback for rows with no canvas node to select (audio, a comp that is not the active one)." That fallback (:340-346) is unreachable, because nothing clears the canvas selection when a timeline clip fails to resolve to a canvas node — which is the same condition the comment names:

• useDomSelection.ts:396 — if (selection) applyDomSelection(selection);. buildDomSelectionForTimelineElement (:357-381) returns null when findElementForTimelineElement finds nothing, so selecting such a clip leaves the previous canvas selection in place rather than clearing it.
• useTimelineSelectionPreviewSync.ts:130-135 — the same case bails out early and calls onSelectionNotFound(), which is EditorShell.tsx:115-117: a toast and nothing else ("The selected clip is not available in the preview yet.").

So: click any element on the canvas, then select an audio clip (or a clip in a non-active comp) in the timeline, press Delete. Pre-PR the timeline branch ran first and deleted that clip. Now domSel is still the earlier canvas element, so the canvas element is deleted and the selected clip stays — and the user has just been told the clip is not in the preview, which makes the wrong deletion read as unrelated. App.tsx:312 mirrors the session's domEditSelection straight into the ref the hotkey reads, so there is no second gate between those two facts.

This is a reorder of existing code, not new coverage, so it is a regression rather than an incomplete new capability — and the damage lands on an element the user is not looking at.

Not pinned by the tests, in the direction that matters: useAppHotkeys.test.ts:37 defaults the ref to {current: null} (timeline path) and :113 sets it agreeing with the timeline selection (canvas path). Across the three useAppHotkeys test files nothing sets a canvas selection that disagrees with the current timeline selection, which is the only state this bug needs.

Worth fixing as arbitration rather than ordering: ask whether the canvas selection corresponds to the current timeline selection (or clear it when a timeline selection fails to resolve) instead of "non-null wins". Ordering alone cannot express the distinction the comment is already making.

blocker — Cut copies the primary and deletes the whole marquee group

useDomEditSession.ts:354-355 expands to the group and discards its argument: const members = group.length > 0 ? group : [selection]. For the Delete key that is the intent. For Cut it is not — useClipboard.ts:200-216:

const copied = handleCopy();          // :201  → copies ONE element (:113-125, the primary's outerHTML)
...
await handleDomEditElementDelete(domSelection);   // :215 → deletes ALL N

So ⌘X over a marquee selection puts one element on the clipboard and removes every member. Paste restores one. Reachable exactly when the primary has no timeline row — so selectedElementId is null and Cut falls past its timeline branch (:205-211) — which is the population this PR exists for (a marquee over a captured page). Undo does recover it in one step, and the toast names the real count, so this is loud rather than silent; the clipboard is what is wrong.

Related and worth deciding deliberately: Cut and Delete now disagree about which selection owns the operation. Cut checks the timeline first (useClipboard.ts:205), Delete now checks the canvas first (useAppHotkeys.ts:333). Pre-PR both checked the timeline first. Both read the same ref (App.tsx:230,244).

Expanding at the Delete call site instead of inside the shared handler fixes both halves and leaves every other caller addressing what it passed. (I checked the other callers: the canvas context menu is safe, because right-clicking a different element reselects non-additively and collapses the group to that element first — useCanvasContextMenuState.ts:79-82 → applyDomSelection nextGroup = [selection].)

important — the cap came off the off-canvas indicators with no cost control, unlike its sibling caller

The PR states the cost problem correctly for the marquee — "Unbounded plus per-move stalls the tab outright" — and solves it by measuring once per drag. The other newly-uncapped hit-testing caller did not get the same treatment:

• offCanvasIndicatorGeometry.ts:60 calls the collector with no budget, so it now walks the whole document (domEditingLayers.ts:468).
• Per item it does getComputedStyle (:67 → isElementComputedVisible → domEditingElement.ts:35) and a rect read (:72 orientedGroupAwareOverlayRect), plus getDirectLayerChildren per node inside the walk itself.
• It runs from a requestAnimationFrame loop started unconditionally on overlay mount (DomEditOverlay.tsx:236, offCanvasIndicatorRefresh.ts:61,92), recomputing on any frame where the dirty flag is set (:79-81) — and dirty is set by a MutationObserver on the preview document filtering ["style","class","transform","width","height","data-hidden"] with subtree: true, childList: true (:39). Inline style/transform writes are how the preview animates, so during playback that is dirty on essentially every frame, with no playback gate anywhere in the path.

Napkin, on this PR's own measured page (442 elements): ~442 style resolutions plus 442 rect reads per animating frame where it was ≤80. At a rough 5-50µs per element that is ~2-22ms against a 16.7ms budget, versus ~0.4-4ms before — i.e. it can plausibly consume the frame on its own. I have not measured this; it is a source read plus an estimate, and I would not treat the numbers as more than a reason to measure. Two things genuinely mitigate it and belong in the same breath: the dirty gate means a static canvas costs nothing extra, and sigRef still suppresses the React re-render (though not the walk). The fix pattern is already in this PR one file over.

nit — x-hf-removed is dead, and its comment claims a capability no caller has

files.ts:2537 is the only occurrence of x-hf-removed in the tree — nothing reads it. The comment at :2519-2521 says removed "reports how many actually left so the caller can tell a partial pass from a no-op", and the client reads only changed / content off the JSON body (useElementLifecycleOps.ts:167-177). So a pass where 1 of 442 targets matched sets changed: true and reports "Deleted 442 elements" — the partial case the comment describes is exactly the one that cannot be distinguished. Separately, c.header() runs after writeIfChanged has already built the response (:2529-2538), so the header may not reach a caller that did want it.

Deciding not to surface partial counts is fine; the comment asserting the caller can tell is the part to fix, since it is what stops the next reader from looking.

nit — the test fixture still advertises the removed callback

useAppHotkeys.test.ts:26 keeps handleTimelineElementDelete alongside the new handleTimelineElementsDelete. It is no longer on HotkeyCallbacks, and excess-property checking does not fire because callbacks()'s result is not a fresh literal at the call site — so the fixture now describes an interface the hook does not have.

Verdict: REQUEST CHANGES
Reasoning: The Delete reorder makes the timeline fallback unreachable in exactly the state its own comment carves out, and deletes an unrelated canvas element instead; Cut deletes N while copying 1. Both are in modified code paths with no test pinning the disagreeing-selection state. Everything else here is solid work and the cap diagnosis is genuinely good.

— Rames Jusso

[miguel-heygen]

Both blockers fixed, and the perf note turned out to be measurable — thank you, the audio-clip path is one I would not have found.

Delete acting on an element you're not looking at. You were right that nothing clears the canvas selection, and right about where: useDomSelection.ts resolved a timeline pick and only applied a result when one came back, so an unresolvable pick left the previous canvas selection in place. There's a trap in the obvious fix, though — clearing announces to the timeline, so clearing on that path would have deselected the clip you just picked. The clear now skips the announce, which is the loop the option exists to break. Test asserts both halves: the canvas selection goes, the timeline setters are untouched. Mutation-checked in both directions — leaving it in place, and announcing the clear.

Cut copying one and deleting N. Expansion moved to the Delete key, as you suggested. The rule is a named function (membersForDelete) taking the caller's intent, so Cut and Delete can differ without either guessing. I found the first version of this untested — reverting the expansion guard didn't turn anything red — so the pure rule exists partly to make that reachable. It's mutation-checked now.

The off-canvas indicators. Measured on the 825-element captured page: a rebuild is 6.5ms against a 16.7ms frame, versus 0.1ms when it was capped at 80. And the observer that marks it dirty filters on style, so animation writes trigger it — your read was exactly right. The rebuild is now throttled to at most every 100ms; the indicators are a passive affordance, so the staleness isn't visible, and the cost stops scaling with frame rate. The existing transition tests now wait past that window, which makes the throttle legible rather than incidental. Being precise about coverage: the throttle itself is not pinned by a test, only the correctness it protects.

What I could not verify. The browser harness I used to drive the marquee stopped producing a selection partway through this session. I isolated it by stashing my changes — it fails without them too — so it's the harness, not the code, but it means these three fixes rest on unit tests with mutation checks rather than a live run. Worth knowing before you re-review.

One flag: this is now 1025 changed lines, over the repo's cap. The marquee cap, the hotkey forwarding, the URL-hydration perf, and the batch delete are each independently revertable. I'd rather cut it than have you review it at this size — say the word and I'll stack them.

[jrusso1020]

Re-review at ef2b2745 (my earlier CHANGES_REQUESTED was at e1a80900). Both original blockers are genuinely fixed and the perf note is now measured rather than estimated. One thing is still open, and it is the first blocker's defect class surviving at the other writer of the same state — so I'm keeping this at REQUEST_CHANGES rather than re-opening the same argument.

What holds

The announce: false diagnosis is the good part. The obvious fix here is wrong, and you named why before I did: clearing the canvas selection announces back to the timeline, so clearing on that path would deselect the clip just picked. useDomSelection.ts:29-33 adds the option, :176-183 honours it, :389-399 uses it on exactly the unresolvable path. The test asserts both halves at once — canvas selection null, setSelectedTimelineElementId and setTimelineSelectionSet untouched (useDomSelectionSelectionGuards.test.ts:324-328) — which is what makes it a guard instead of a demonstration.

membersForDelete is the right shape and the right location. useDomEditSession.ts:82-88 is pure and named, and moving the choice to the caller means useClipboard.ts:31 can declare a narrow local type with no options parameter at all — Cut's intent is now structural rather than a comment. Enumerated every current caller and they are all correct: useAppHotkeys.ts:342 expands, useClipboard.ts:215 does not, and DomEditOverlay.tsx:561-564 → PreviewOverlays.tsx:222 passes the session handler with no options so the overlay affordance takes the primary only — which matches main, where the expansion never existed. Not a regression; worth a separate decision on whether that affordance should expand.

The measurement replaces my napkin. 6.5ms across 825 elements against a 16.7ms frame is the number my estimate was standing in for, and the mechanism you confirmed (the observer filters on style, which is how animation writes) is the part that made it per-frame. The throttle reads correctly: lastRecomputeAt starts at Number.NEGATIVE_INFINITY so the first rebuild is immediate, and dirty is deliberately not cleared while throttled (offCanvasIndicatorRefresh.ts:97-101), so a mutation arriving inside the window rebuilds on the next eligible frame instead of being dropped.

blocker — the same defect survives at the second writer of the canvas selection

useDomSelection.handleTimelineElementSelect is one of two places that mirror a timeline pick onto the canvas. The other is the store-driven effect, and it still returns without clearing — useTimelineSelectionPreviewSync.ts:130-136:

if (selections.length < resolvableCount) {
  if (missingSelectionKeyRef.current !== selectedKey) {
    missingSelectionKeyRef.current = selectedKey;
    onSelectionNotFound();
  }
  return;                 // canvas selection left pointing at the previous pick
}

An audio clip is resolvable as a timeline element and unresolvable as a DOM node, so resolvableCount is 1 and selections.length is 0, and it takes this branch. It never reaches the selections.length === 0 clear at :143-144 — that one is only reachable when resolvableCount is 0 too, i.e. when the selected id matches no timeline element at all.

It matters because not every writer of the store selection goes through the handler you fixed. Timeline clip clicks do (timelineClipGestureHandlers.ts:218-219 calls setSelectedElementId and onSelectElement), and the timeline marquee does on pointer-up (useTimelineRangeSelection.ts:463). These three do not:

• AudioRow.tsx:60-71 — clicking a sidebar audio asset that is already in the project reveals its clip: setSelectedElementId(clipKey) + requestTimelineFocus, no onSelectElement.
• AssetCard.tsx:152-163 — same shape for visual assets, which reaches the other half of your carve-out (a clip in a composition that is not the active one).
• useTimelineAssetDropOps.ts:197 — selectHost after an insert, so dropping an audio asset selects a clip with no canvas node.

Sequence:

1. select a text element on the canvas
2. in the sidebar Audio list, click an audio asset already used in the project — the timeline scrolls to its clip, the clip becomes the selected element, and the toast says it is not available in the preview
3. press Delete — the text element goes, the audio clip stays

Same direction as before: the reorder is what turns a stale pointer into data loss, so this lands on the PR rather than being pre-existing. To be straight about my own verification: this is a static trace of the writers, not a live run — I did not drive the browser either, so your harness caveat applies to me as well.

Two things suggest the second writer simply wasn't in scope rather than being considered and dismissed. useTimelineSelectionPreviewSync.ts:18-21 still declares its own narrowed structural copy of the applyDomSelection signature (revealPanel, additive, preserveGroup), so the new announce option is not reachable from there at all. And the one existing test that reaches the branch — useTimelineSelectionPreviewSync.test.tsx:146 — asserts onSelectionNotFound fired once and nothing about the canvas selection, so the line is covered for its toast and uncovered for its consequence.

On the fix: "clear when you bail" is the wrong shape, because the bail exists for a real transient — a member whose DOM node is not ready yet, which a later run resolves, and clearing there would flicker. The narrow version is already computable from what the effect has: currentAnchor (:91-93) is the timeline id the current canvas selection resolves to, so when it is non-null and absent from selectedIds, the canvas is pointing outside the selection and can be cleared with the same quiet announce: false. That leaves in-flight members alone and removes only the dangerous state. The invariant worth writing down once, now that Delete prefers the canvas: the canvas selection never points outside the current timeline selection.

notes

• Your coverage statement checks out. Nothing pins RECOMPUTE_INTERVAL_MS, so setting it to 0 keeps the suite green — offCanvasIndicatorRefresh.test.tsx waits past the window rather than asserting it. Cheapest pin is a fake clock showing two mutations inside one window collapse to a single rebuild.
• Throttle staleness, checked: worst case is one 100ms window of lag after a comp-rect change (:89-92 marks dirty, the gate defers it). Passive affordance, and player-perf is green at this head.
• Carried from the last pass and still open as a nit: studio-server/src/routes/files.ts:2519-2538 sets x-hf-removed and its comment says a caller can tell a partial pass from a no-op, but that header is the only occurrence in the tree.

CI

7 of the 8 required contexts are green at ef2b2745; Test was re-queued at 03:48Z and is still running, so I'm not calling the head green. Tests on windows-latest passed, as did regression, Build, Typecheck, Test: runtime contract, Render on windows-latest and Semantic PR title.

On the size

Your 1025 figure is right (816 additions, 209 deletions). I could not find a required check or a documented diff-size cap that it violates — File size check is per-file and green — so if the cap is a convention you're carrying, you know it better than I can see it. My recommendation is to keep it whole: it has been read end to end at this size twice now, and re-cutting a reviewed branch into four is where work quietly gets dropped between the pieces. If you do split it, split off the marquee cap, the hotkey forwarding and the URL-hydration perf and leave the Delete arbitration plus this blocker as the PR I've already reviewed, rather than re-cutting all of it.

Verdict: REQUEST CHANGES
Reasoning: Both original blockers are fixed and fixed well, but the stale-canvas-selection defect survives at useTimelineSelectionPreviewSync.ts:130-136, reachable from the sidebar reveal and the asset drop, with the same outcome of deleting an element the user is not looking at.

— Rames Jusso

[miguel-heygen]

Second blocker fixed at a4dedf05, and you were right about the shape of it — I fixed one writer and left the other holding the same stale pointer.

The second writer. Traced it and it lands exactly as you described: resolvableCount is 1 and selections.length is 0 for an audio clip, so it takes the bail and never reaches the clear at :143. The three paths that skip handleTimelineElementSelect check out too — AudioRow.tsx:60-71, AssetCard.tsx:152-163 and useTimelineAssetDropOps.ts:197 all set the store selection without onSelectElement.

I took your narrow version rather than "clear when you bail", for the reason you gave: the bail exists for a member whose node is not ready, and clearing there would flicker. Only currentAnchor resolving outside selectedIds is dropped, quietly. The structural copy of the applyDomSelection signature at :18-21 now carries announce — you were right that its absence is why this wasn't reachable from here.

Mutation-checked in both directions, which is what convinced me the guard is narrow enough: removing the clear reds the new test, and clearing unconditionally reds your transient-retry case ("warns once while retrying a timeline selection after preview refreshes"). That second one is the useful half — it means the test suite now holds the line between "dangerous" and "merely unfinished" rather than me asserting it.

The invariant is written down at the predicate, in your words: the canvas selection never points outside the current timeline selection.

Both notes cleared. x-hf-removed is gone along with the comment that promised a distinction the response can't make — you flagged it twice and were right both times; the counting was left over from a draft where the client read it. And the throttle is pinned now via rebuildDue — two mutations inside one window collapse to one rebuild, and setting the interval to 0 reds it.

On the merge: Miguel has asked me to admin-merge this now, so it will land with your review open rather than waiting for your re-review. That's his call and I've told him what it means — I'm noting it here so it doesn't arrive silently, since you said you own this thread and Vai may be looking in parallel. Everything above is pushed and green; if anything in the fix reads wrong to you post-merge I'll turn it around immediately as a follow-up.

Thank you for three passes that each found something real. The two blockers were both in reordered code, which is the part I was least likely to catch myself.