feat: add prehit spatial index as safe fallback accelerator#292
Open
feat: add prehit spatial index as safe fallback accelerator#292
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
aidenybai
force-pushed
the
feat/prehit-spatial-index
branch
from
a000158 to
eb6950a
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/react-grab/src/utils/get-element-at-position.ts">
<violation number="1" location="packages/react-grab/src/utils/get-element-at-position.ts:77">
P1: The new spatial-index fallback can return stale hits because the index is never rebuilt after viewport/layout changes, so this branch may bypass `elementsFromPoint()` with outdated geometry.</violation>
</file>
<file name="packages/react-grab/src/utils/is-valid-grabbable-element.ts">
<violation number="1" location="packages/react-grab/src/utils/is-valid-grabbable-element.ts:94">
P1: This overlay heuristic now rejects legitimate empty absolute/sticky controls that `elementFromPoint()` correctly hit, so selection can fall through to the wrong element underneath.</violation>
</file>
<file name="packages/react-grab/src/utils/element-at-point-index.ts">
<violation number="1" location="packages/react-grab/src/utils/element-at-point-index.ts:59">
P2: Spatial index geometry is snapshotted once and never invalidated, so hits can become stale after layout or DOM changes.</violation>
<violation number="2" location="packages/react-grab/src/utils/element-at-point-index.ts:221">
P1: Recheck the candidate's current bounds before accepting a cached tree hit. Otherwise layout changes can return an element from its old location and skip `elementsFromPoint`.</violation>
<violation number="3" location="packages/react-grab/src/utils/element-at-point-index.ts:227">
P1: Don't return the fixed-element shortcut before comparing overlapping fixed candidates by full stacking order.</violation>
</file>
<file name="packages/react-grab/src/utils/is-decorative-overlay.ts">
<violation number="1" location="packages/react-grab/src/utils/is-decorative-overlay.ts:15">
P2: Include `position: fixed` here; otherwise small empty fixed overlays can still block fallback selection and get treated as grabbable.</violation>
<violation number="2" location="packages/react-grab/src/utils/is-decorative-overlay.ts:16">
P2: Raw `tagName` comparison can miss SVG/non-HTML elements and misclassify them as decorative overlays.</violation>
</file>
<file name="packages/react-grab/src/utils/compare-stacking-order.ts">
<violation number="1" location="packages/react-grab/src/utils/compare-stacking-order.ts:87">
P2: Tie-breaking equal `z-index` branches by DOM order misorders positioned `z-index:auto` elements.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| result = topElement; | ||
| } else { | ||
| } else if (isElementAtPointIndexReady()) { | ||
| const spatialResult = queryElementAtPointIndex(clientX, clientY); |
Contributor
There was a problem hiding this comment.
P1: The new spatial-index fallback can return stale hits because the index is never rebuilt after viewport/layout changes, so this branch may bypass elementsFromPoint() with outdated geometry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/utils/get-element-at-position.ts, line 77:
<comment>The new spatial-index fallback can return stale hits because the index is never rebuilt after viewport/layout changes, so this branch may bypass `elementsFromPoint()` with outdated geometry.</comment>
<file context>
@@ -64,30 +65,22 @@ export const getElementAtPosition = (clientX: number, clientY: number): Element
result = topElement;
- } else {
+ } else if (isElementAtPointIndexReady()) {
+ const spatialResult = queryElementAtPointIndex(clientX, clientY);
+ if (spatialResult && isValidGrabbableElement(spatialResult)) {
+ result = spatialResult;
</file context>
| @@ -0,0 +1,252 @@ | |||
| import { HilbertRTree } from "./hilbert-r-tree.js"; | |||
Contributor
There was a problem hiding this comment.
P2: Spatial index geometry is snapshotted once and never invalidated, so hits can become stale after layout or DOM changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/utils/element-at-point-index.ts, line 59:
<comment>Spatial index geometry is snapshotted once and never invalidated, so hits can become stale after layout or DOM changes.</comment>
<file context>
@@ -0,0 +1,252 @@
+ observer.unobserve(entry.target);
+
+ const targetElement = entry.target as HTMLElement;
+ const boundingRect = entry.boundingClientRect;
+ if (boundingRect.width === 0 || boundingRect.height === 0) continue;
+ if (!isValidGrabbableElement(targetElement)) continue;
</file context>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb6950a. Configure here.
| if (PAINT_CONTAIN_VALUES.has(keyword)) return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Duplicate hasPaintContainment in two new files
Low Severity
hasPaintContainment is independently implemented in both compare-stacking-order.ts and element-at-point-index.ts with different logic. One uses String.includes("paint") while the other splits by space and checks a Set. They produce the same results for valid CSS contain values but diverge on edge cases, creating a maintenance risk where a fix to one copy could be missed in the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eb6950a. Configure here.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| if (fixedHit) { | ||
| if (visibleCandidates.length === 0) return fixedHit; | ||
| visibleCandidates.push(fixedHit); | ||
| } |
Contributor
There was a problem hiding this comment.
Multiple fixed elements at the same point are not compared using proper CSS stacking order rules, causing incorrect element selection
| const candidate = currentIndex.elements[hitIndex]; | ||
| if (!candidate.isConnected) continue; | ||
| if (!isVisibleAtPoint(candidate, clientX, clientY)) continue; | ||
| visibleCandidates.push(candidate); |
Contributor
There was a problem hiding this comment.
Spatial index returns candidates with stale bounding box coordinates, causing incorrect element hits when element bounds change after index creation
aidenybai
force-pushed
the
feat/prehit-spatial-index
branch
from
eb6950a to
2c5469c
Compare
aidenybai
force-pushed
the
feat/prehit-spatial-index
branch
from
2c5469c to
7d979fc
Compare
aidenybai
force-pushed
the
feat/prehit-spatial-index
branch
from
7d979fc to
0463f03
Compare
Add a pre-indexed element hit-testing system that replaces the expensive elementsFromPoint scan when elementFromPoint returns a non-grabbable element (decorative overlays, dev tools, etc.). Safety model: browser-native elementFromPoint is always tried first and trusted for all CSS edge cases. The spatial index only runs as a fallback, replacing the O(n) elementsFromPoint call with an O(log n) R-tree query + stacking order sort. Infrastructure: - HilbertRTree: packed R-tree with Hilbert curve ordering for spatial queries - compareStackingOrder: CSS stacking context comparator (z-index, opacity, transform, contain, backdrop-filter, perspective, clip-path) - isVisibleAtPoint: ancestor clip-chain walker with WeakMap cache - isDecorativeOverlay: filters empty positioned elements (now also used in isValidGrabbableElement for all detection paths) - Fixed elements cached with viewport rects and z-indexes at build time Includes 57 new e2e tests covering overflow clipping, CSS containment, stacking order, inline elements, decorative overlays, fixed position, transforms, dynamic DOM, and fixed-only pages.
aidenybai
force-pushed
the
feat/prehit-spatial-index
branch
from
0463f03 to
37c17a0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
elementFromPointreturns a non-grabbable element (decorative overlays, dev tool canvases, etc.)elementFromPointis always tried first — the browser handles all CSS edge cases (clip-path, pointer-events, stacking contexts). The spatial index only runs as a tier-2 fallback, replacing the expensiveelementsFromPointO(n) scan with an O(log n) R-tree queryisDecorativeOverlayintoisValidGrabbableElementso empty positioned divs are filtered universally, not just by the indexDetection pipeline
Test plan
elementFromPointreturns overlay/non-grabbableNote
Medium Risk
Changes core element hit-testing logic by adding a new spatial index fallback and new overlay filtering, which could affect what element is selected in edge CSS/stacking cases despite keeping
elementFromPointas the primary path.Overview
Adds a new activation-scoped tier-2 hit-testing path: when
elementFromPointreturns a non-grabbable element,getElementAtPositionnow queries a prebuiltelement-at-pointspatial index (Hilbert R-tree) before falling back toelementsFromPoint.The index is built on activation and destroyed on deactivation/cleanup, accounts for overflow/contain clipping, separately tracks
position: fixedelements, and resolves overlaps via a newcompareStackingOrderhelper. Empty positioned “decorative overlay” elements are now filtered universally viaisValidGrabbableElement.Expands the e2e app with new sections (overflow clipping, CSS containment, stacking order, overlays, inline elements, transform/opacity) and adds a large new Playwright suite (
element-detection.spec.ts) covering these and additional dynamic/edge scenarios.Reviewed by Cursor Bugbot for commit 37c17a0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a pre-indexed spatial hit-testing fallback in
react-grabto speed up element detection whenelementFromPointhits a non‑grabbable overlay, cutting worst‑case scans from O(n) to O(log n) without changing normal behavior. The index builds on activation and is torn down on deactivation and cleanup.IntersectionObserver(10,000px root margin), cachesposition: fixedelements with viewport rects/z-index, and ranks candidates withcompareStackingOrder(z-index, transforms, opacity, containment, clip-path) plus clip‑chain visibility checks.elementFromPoint→ spatial index (only if top hit isn’t grabbable and index is ready) →elementsFromPoint; index rebuilds each activation and is destroyed on deactivation and cleanup.isValidGrabbableElementacross all paths; adds 57 E2E tests covering overflow/containment clipping, stacking, inline elements, overlays, transforms, fixed‑only pages, modals, zero‑size/invisible elements, animations, dynamic DOM, and reactivation rebuilds.Written for commit 37c17a0. Summary will update on new commits.