Speed up marquee and double-click element selection in design canvas - #5095
Conversation
Both gestures did work proportional to the whole screen rather than to what the gesture touched, because the bulk selection paths call the bridge's full getElementInfo(), which snapshots portable computed styles for an element and its entire subtree. A drill-in is a point query, so filter candidates to the pointer's containment chain before building element info. A marquee freezes layout, so measure candidate bounds and build element info once per gesture, rAF-coalesce moves, and pool the passive selection overlays instead of rebuilding them per frame. The drill-in case was also failing outright: at ~1192ms it never beat the 400ms reply timeout, so double-click silently did nothing on a large screen while the abandoned pass still ran and the host re-requested every tick. Back off retries for a screen that timed out (no-iframe still retries) and raise the timeout, since the area query still collects a whole screen. Measured on a 1309-node screen, before/after with the same harness: marquee drag scripting 3415ms -> ~630ms; getBoundingClientRect per frame 455 -> 65; getComputedStyle per frame 259 -> 39; drill-in collect 1192ms -> ~85ms, building 2 element infos instead of 1200. No message shape, ElementInfo payload, drill-in walk semantics, container-first marquee rule, or one-drag-one-undo behaviour changes.
builder-io-integration
Bot
changed the title
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
PR #5095 optimizes design-canvas selection by narrowing point queries before expensive element-info construction, caching marquee bounds and element info per gesture, coalescing pointer moves with animation frames, pooling passive overlays, and extending selectable-rect reply handling. The overall direction is sound and the generated bridge plus targeted regression tests improve coverage of the intended performance wins. This is standard risk because the changes alter shared canvas hit-testing, gesture lifecycle, and asynchronous selection coordination.
Key Findings
- 🔴 HIGH — Point-scoped collection mixes viewport-space input with document-space bounds in scrolled iframe documents, so drill-in can miss the visible target.
- 🟡 MEDIUM — Marquee finalization can commit before slow selectable-rect replies arrive, so the longer timeout still produces partial/empty final selections for normal quick releases.
- 🟡 MEDIUM — Board candidate collection is started only after all screen collections settle, allowing a slow screen timeout to delay board candidates beyond the gesture's finalization window.
The rAF latest-event handling, per-gesture memoization, passive overlay reuse, and generated output are otherwise coherent. 🧪 Browser testing: Will run after this review (PR touches UI code).
| var bounds = selectableBounds(el); | ||
| var scrollX = window.scrollX || window.pageXOffset || 0; | ||
| var scrollY = window.scrollY || window.pageYOffset || 0; | ||
| var tolerance = 1; | ||
| return ( | ||
| point.x >= bounds.left + scrollX - tolerance && |
There was a problem hiding this comment.
🔴 Point filtering uses the wrong coordinate space for scrolled documents
atPoint arrives in iframe viewport/local coordinates, while selectableBounds(el) is converted to document coordinates by adding window.scrollX/Y. When the iframe document is scrolled, visible targets are compared against mismatched coordinates and can be filtered out before drill-in receives their element info. Keep both the point and bounds in client space, or add the same scroll offset to the point before filtering.
Additional Info
Reported by 2 of 3 code-review agents; one classified it high and one medium.
There was a problem hiding this comment.
Required — not fixing: verified the premise is wrong, and added a regression test proving it.
atPoint does not arrive in viewport coordinates. The host derives it by inverting the exact mapping it applied to info.boundingRect, and rectInfoForElement reports that rect in document space:
return {
x: rect.x + (window.scrollX || window.pageXOffset || 0),
y: rect.y + (window.scrollY || window.pageYOffset || 0),
...So both sides are document space, which is exactly why the filter adds scrollX/scrollY to the client-space selectableBounds — it is converting to match the point, not introducing a mismatch.
Rather than argue it, selection-performance.bridge.spec.ts now has agrees with the reported boundingRect space when the document is scrolled: it scrolls the document to y=420, picks a deep on-screen target, uses its reported rect centre as atPoint, and asserts the target comes back.
I also confirmed the test is not vacuous. Forcing the bug you describe (scrollX = 0; scrollY = 0, i.e. comparing in viewport space) makes it fail with precisely the symptom in this comment — the element under the pointer filtered out and the wrong ones returned:
AssertionError: expected [ 'card-24', 'title-24' ] to include 'action-0'
With the shipped code it passes. Good catch to check, but the space is consistent and is now pinned.
| // A drill-in narrows the collect to the pointer's containment chain and answers | ||
| // in ~100ms, but an area query (the marquee) still builds info for every | ||
| // candidate on the screen, which measures near a second on a large generated | ||
| // document. Giving up at 400ms there did not make the marquee faster — it made | ||
| // in-screen layers unselectable while the abandoned pass still ran. | ||
| const SELECTABLE_RECTS_REPLY_TIMEOUT_MS = 2_000; |
There was a problem hiding this comment.
🟡 Longer timeout still allows marquee finalization to drop slow replies
The marquee commits its final selection and clears the drag state on mouseup, so replies that arrive during the new 2-second window are discarded once the gesture has ended. A normal quick release can therefore still produce a partial or empty selection on large documents; either finalize after in-flight collections settle or explicitly define a cancellation/fallback path that does not imply the longer timeout makes the final result reliable.
Additional Info
Confirmed by one code-review agent; follows the finalization and stale-drag guards in the modified flow.
There was a problem hiding this comment.
Optional — skipping: accurate description, but it is pre-existing behaviour that this PR strictly improves rather than causes.
Dropping late replies after the gesture ends is the state !== marqueeState guard in reportLayerSelection, which predates this change. What matters is whether a reply lands before mouseup at all:
- Before: the collect took ~943ms against a 400ms timeout, so on a large screen it was always abandoned. In-screen layer candidates never arrived, no matter how slowly you released.
- After: the same collect completes at ~943ms inside the 2000ms window, so it now succeeds and the candidates are applied mid-drag.
So the window where a quick release loses candidates got strictly smaller. There is no release timing that regressed.
On the suggested fix — finalising after in-flight collections settle — I looked at it and do not think it is a win here. It would delay the committed selection behind a round-trip the user cannot see, make mouseup feel laggy, and open a race with the next gesture that the current marqueeState identity check exists to prevent. Trading a missing-candidate case for a stale-selection case is not obviously better.
The real fix is removing the ~943ms itself: a marquee is an area query and cannot be point-filtered like the drill-in, so candidate collection still builds full getElementInfo for every candidate on the screen. Lean descriptors plus hydrate-on-select (agent-native:measure-selection already exists) would collapse it, but that makes setSelectedElement async and has to be reconciled with the PF10 signature dedup and the history coalescer — too much risk to bundle into a perf fix. It is called out as the top remaining item in the PR description.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
…bd02258cc0924dd1891d
…ill-in Review follow-ups on the selection performance change: - The board candidate request was only issued after awaiting every screen's round-trip, so one slow screen could burn the whole reply timeout before the board — whose own iframe is ready — was even asked. Start both and await together. - Add a regression test for point-filtered collection against a scrolled document. getElementInfo reports boundingRect in document space (client rect plus scroll) and the host derives atPoint by inverting that same mapping, so the filter adds the scroll offset to match. The test fails if that offset is dropped, which is the coordinate-space mismatch review flagged. - oxfmt on the changed files.
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental Code Review Summary
The latest commit starts board and screen candidate collection concurrently, eliminating the previously reported board-behind-screen-timeout issue; that review thread has been resolved. The added scrolled-document regression coverage is useful, but it directly exercises the bridge rather than the full host-to-bridge conversion path. No new production regressions were confirmed in the updated async coordination, timeout bookkeeping, correlation handling, or generated bridge output.
The two other prior findings remain open and were intentionally not reposted: the viewport/document coordinate mismatch in scrolled iframe drill-in, and marquee finalization before slow replies settle. This remains standard risk because the PR changes shared canvas hit-testing and asynchronous gesture coordination. 🧪 Browser testing: Will run after this review (PR touches UI code).
|
There was a problem with your request, please try again later. Error id: |
3 participants
Summary
Fixes slow, buggy marquee (rubber-band) select and double-click element selection on the design canvas by cutting down redundant work in the selection bridge and its host-side coordination.
Problem
Selecting elements on the canvas was reported as very slow: marquee drag was laggy while dragging, and double-click took a perceptible amount of time to resolve the target element. On large generated screens (thousands of candidate nodes), the selectable-rects collection built full
getElementInfosnapshots — including computed styles for an element and its whole subtree — for every candidate on every event, and marquee drags rebuilt passive selection overlays and re-measured bounds on every raw mousemove instead of once per frame.Solution
getElementInfocollection to only the candidates relevant to the current operation (point-based filtering for drill-in/pick, cached per-gesture bounds/info for marquee), and batch marquee move handling to animation frames instead of firing on every raw pointer event.getBoundingClientRectreads for non-primary overlays.Key Changes
collectSelectableElementInfosnow accepts an optionalatPoint, filtering candidates via a newdocumentSpaceBoundsContainPointcheck before building full element info;readSelectablePointvalidates/throws on malformed points rather than silently widening the collection.activeMarqueeSelectionnow trackscandidateBounds(measured once per gesture) and a per-gestureinfoCacheMap sogetElementInfois built once per element instead of once per element per tick.beginMarqueeSelection'sonMovenow queues the latest move event and flushes viarequestAnimationFrame(flushMarqueeMove), withonUpforce-flushing and canceling any pending frame so the gesture always ends with exactly onefinalreport.syncPassiveSelectionOverlayPoolandsamePassiveSelectionElementsreuse existing overlay DOM nodes and skip repositioning entirely when the hit-set hasn't changed, avoiding per-frame create/append/remove churn and forced reflows;positionOverlayskipsgetBoundingClientRectfor non-primary overlays.MultiScreenCanvas.requestSelectableElementInfosandcollectLayerMarqueeCandidatesaccept an optional board-spaceatPoint, converted per-frame into screen-local space viaboardPointToScreenLocalPoint, and drill-in now tracks per-screen timeouts (timedOutScreenIds) separately from "no iframe yet" so a slow screen isn't re-queried every tick.SELECTABLE_RECTS_REPLY_TIMEOUT_MSraised from 400ms to 2000ms to accommodate area queries on large generated documents.selection-performance.bridge.spec.tsasserts bounded work (computed-style read counts, element-info payload size) rather than elapsed time for both point-scoped collection and marquee dragging, pluscoordinate-transforms.spec.tspinning that board/screen-local point conversion is the exact inverse used by rect conversion..generated/bridge/editor-chrome.generated.tsregenerated from the updatededitor-chrome.bridge.ts.To clone this PR locally use the Github CLI with command
gh pr checkout 5095You can tag me at @BuilderIO for anything you want me to fix or change