Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 6 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/create-box-model-bounds.ts">
<violation number="1" location="packages/react-grab/src/utils/create-box-model-bounds.ts:13">
P2: Grid layouts are marked as supported, but gap detection uses linear adjacent-sibling math that is only valid for single-axis flex-like flow, causing incorrect inspect gap overlays on real grids.</violation>
<violation number="2" location="packages/react-grab/src/utils/create-box-model-bounds.ts:60">
P2: This gap calculation only works for forward single-axis layouts; `*-reverse` flex containers drop their gaps, and `grid` containers never get row-gap overlays.</violation>
<violation number="3" location="packages/react-grab/src/utils/create-box-model-bounds.ts:73">
P2: Gap calculation ignores reverse flex directions, causing valid gaps to be dropped for `row-reverse`/`column-reverse` layouts.</violation>
<violation number="4" location="packages/react-grab/src/utils/create-box-model-bounds.ts:93">
P1: Use the `border-*-width` longhands here; `border-width-top`/etc. are invalid, so bordered elements render the inner box-model layers as if the border were 0px.</violation>
</file>
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:3653">
P2: `inspectNavigationState` is still computed and used by the renderer/SelectionLabel, but it’s no longer passed into `<ReactGrabRenderer />`, which can break inspect ancestor navigation UI.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
commit: |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/create-box-model-bounds.ts">
<violation number="1" location="packages/react-grab/src/utils/create-box-model-bounds.ts:57">
P2: Grid gap computation uses global single-axis sorting, which can pair non-neighbor grid items and produce incorrect overlay gap rectangles.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
| fillWithHatch(context, gapPath, BOX_MODEL_GAP_HATCH_COLOR); | ||
| } |
There was a problem hiding this comment.
Gap rects not animated, visually desync during transitions
Medium Severity
Gap rects in renderInspectLayer are read directly from props.inspectBoxModel?.gaps (raw, non-animated positions), while the margin/border/padding/content layers are all smoothly animated via boxModelAnimations. When the user moves the cursor to a different flex/grid container, the gap hatching instantly jumps to the new element's positions while the content region is still lerping from the old position. This can cause gap hatching to render outside the content area during the animation transition.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e7ea6dc. Configure here.
| axis: "row" | "column", | ||
| ): GapRect[] => { | ||
| const sortedRects = [...childRects].sort((a, b) => | ||
| axis === "column" ? a.top - b.top : a.left - b.left, |
There was a problem hiding this comment.
Grid gap computation incorrectly pairs non-adjacent grid items by sorting all children globally on a single axis without checking for neighbor overlap
| } | ||
| } | ||
| fillWithHatch(context, gapPath, BOX_MODEL_GAP_HATCH_COLOR); | ||
| } |
There was a problem hiding this comment.
Gap rects in renderInspectLayer are read directly from props without animation while margin/border/padding/content layers animate smoothly, causing visual desynchronization during element transitions.
| } | ||
| } | ||
| fillWithHatch(context, gapPath, BOX_MODEL_GAP_HATCH_COLOR); | ||
| } |
There was a problem hiding this comment.
Grid gap hatching creates holes at intersections
Medium Severity
For grid containers, computeChildGaps returns gap rects from both axes — full-height vertical bands (column gaps) and full-width horizontal bands (row gaps). These rects overlap at intersection points. All gap rects are added to one Path2D and filled via fillWithHatch, which unconditionally uses the "evenodd" fill rule. At overlapping regions, the crossing count is 2 (even), so the hatching is not drawn, creating visible un-hatched holes at every grid gap intersection. The "evenodd" rule is correct for the ring paths (margin/padding) but incorrect for the gap rects, which need "nonzero".
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 17c5eae. Configure here.
| if (existingAnimation) { | ||
| updateAnimationTarget(existingAnimation, ancestorBounds); | ||
| return existingAnimation; | ||
| const layers = ["margin", "border", "padding", "content"] as const; |
There was a problem hiding this comment.
Variable shadows outer offscreen layers record
Low Severity
The const layers array on this line shadows the outer const layers: Record<LayerName, OffscreenLayer> defined at component scope (line 96). While no current code in this createEffect callback accesses the outer record, the name collision makes the code fragile — any future addition that needs the offscreen layers record inside this callback would silently reference the wrong variable.
Reviewed by Cursor Bugbot for commit 17c5eae. Configure here.
| const layers = ["margin", "border", "padding", "content"] as const; | ||
| for (const layer of layers) { |
There was a problem hiding this comment.
Inner variable 'layers' shadows outer 'layers' Record in effect callback, creating a maintenance hazard for future code modifications
|
|
||
| inspectVisible?: boolean; | ||
| inspectBounds?: OverlayBounds[]; | ||
| inspectBoxModel?: BoxModelBounds; |
There was a problem hiding this comment.
Unused inspectBounds prop after refactor to box model
Low Severity
The inspectBounds prop in OverlayCanvasProps is declared and passed through the component hierarchy but never consumed. The createEffect that previously read props.inspectBounds was replaced with one that reads props.inspectBoxModel, making the prop dead code. The inspectBounds() memo in core/index.tsx still runs createElementBounds on every ancestor each time the viewport changes, solely to power the inspectBounds().length > 0 visibility check — which could use the already-existing inspectAncestorElements().length > 0 instead, avoiding that unnecessary computation.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b9a52c1. Configure here.
|
|
||
| inspectVisible?: boolean; | ||
| inspectBounds?: OverlayBounds[]; | ||
| inspectBoxModel?: BoxModelBounds; |
There was a problem hiding this comment.
The inspectBounds prop is declared in multiple interfaces and generated/passed in the core component but never actually used in the overlay-canvas component
Show padding, margin, content, and flex/grid gap regions when shift-inspecting elements, using DevTools-style hatched patterns and rounded-rect-aware rendering.
- Use border-*-width longhands (border-top-width) instead of invalid border-width-* properties that always resolved to 0px - Sort children by visual position before computing gaps, fixing row-reverse and column-reverse flex layouts dropping all gaps - Detect gaps in both row and column axes for grid layouts
- Skip display:none and position:absolute/fixed children in computeChildGaps to prevent spurious gap rects at viewport origin - Also skip zero-sized children as a safety net - Simplify DOMMatrix.rotate() to single-arg form for Z-axis rotation
- Extract isInFlowChild and hasVisibleSize predicates for declarative child filtering in computeChildGaps - Cache isColumnAxis to reduce repeated axis checks in computeAxisGaps - Extract currentRect/nextRect locals to eliminate array index repetition - Move BoxModelLayerName type to module scope per AGENTS.md guidelines
Path2D.addPath() with a roundRect sub-path breaks the evenodd fill rule in Chromium, causing padding and margin ring fills to render as empty. Build both sub-paths directly on the same Path2D instead. Extract appendBoundsToPath as the shared primitive so buildBoundsPath and buildRingPath both delegate to it without duplicating logic.
- Parse all 4 corner radii via CSS longhands instead of using the first value from the shorthand, so elements with mixed corner radii (e.g. rounded top, square bottom) render accurately - Compute inner radii per-corner by subtracting the max of each corner's two adjacent sides (border/padding), matching the CSS spec - Change AnimatedBounds.borderRadii from single number to number[] so all overlay layers (selection, drag, grabbed, inspect) benefit
aidenybai
force-pushed
the
feat/box-model-inspect-overlay
branch
from
62eb20a to
1baf7f0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1baf7f0. Configure here.
| const paddingRadii = insetCornerRadii(outerRadii, borderSides); | ||
| const contentRadii = insetCornerRadii(paddingRadii, paddingSides); | ||
|
|
||
| const margin = outsetBounds(borderBounds, marginSides, "0px"); |
There was a problem hiding this comment.
Margin "0px" radius causes spurious corner hatching
Medium Severity
The margin bounds are hardcoded with borderRadius: "0px" (sharp corners), while the border bounds retain the element's actual border-radius. When buildRingPath(margin, border) is filled with evenodd, the corner areas between the sharp margin rect and the rounded border rect get filled with margin hatching — even when all margins are zero. Any element with border-radius and zero margins will show spurious hatch artifacts in the rounded corners. The margin box needs outset border radii computed from the border radii plus the margin thickness, matching how insetCornerRadii works for the inward layers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1baf7f0. Configure here.
Chromium clips the top-left of a ring when mixing roundRect and rect sub-paths on the same Path2D with evenodd fill. Using roundRect uniformly (even with [0,0,0,0] radii) avoids the bug entirely.
| const paddingRadii = insetCornerRadii(outerRadii, borderSides); | ||
| const contentRadii = insetCornerRadii(paddingRadii, paddingSides); | ||
|
|
||
| const margin = outsetBounds(borderBounds, marginSides, "0px"); |
There was a problem hiding this comment.
Margin bounds hardcoded with sharp corners (borderRadius: "0px") cause visual artifacts in hatching pattern when margin has rounded corners in the element
1 participant
Summary
Test plan
Note
Medium Risk
Moderate risk: replaces inspect-mode rendering with new canvas Path2D/hatch logic and adds DOM/CSS-dependent box-model calculations that could affect overlay correctness/perf across browsers.
Overview
Inspect mode overlay is replaced with a DevTools-style box model view. The overlay now renders animated
margin,padding, andcontentregions (with hatched patterns for margin and layout gaps) and respects per-corner border radii.This threads a new
inspectBoxModelshape from core → renderer →OverlayCanvas, addscreateBoxModelBounds(including flex/grid gap detection), and updates the canvas renderer to usePath2Dring fills plus cached hatch patterns. An additional Playwright e2e test asserts drag-box height growth matches auto-scroll delta; minor housekeeping updates.gitignoreand registry JSON formatting.Reviewed by Cursor Bugbot for commit 6d82e2f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Replaces inspect-mode ancestor boxes with a DevTools-style box model overlay. Shows margin (hatched), padding/content (solid), and flex/grid gaps with smooth animation and correct per-corner border radii.
New Features
createBoxModelBoundsto computemargin,border,padding,content, andgaps, and threadedinspectBoxModelfrom core → renderer → canvas.Path2D.roundRect, cached hatch patterns viaOffscreenCanvas, introduced overlay colors, and enabled independent corner radii across selection/drag/grab/inspect.Bug Fixes
border-*-widthlonghands to avoid 0px borders.roundRectand building ring subpaths on the samePath2D.Written for commit 6d82e2f. Summary will update on new commits.