Conversation
Draw on elements using Alt+drag or the toolbar draw button to circle/arrow select them and open the comment prompt. Adds stroke gesture classification, overlay canvas rendering, and e2e tests for the full drawing flow. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This run croaked 😵 The workflow encountered an error before any progress could be reported. Please check the workflow run logs for details. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/copilot
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
@react-grab/utils
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 9 files
Prompt for AI agents (all 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/components/overlay-canvas.tsx">
<violation number="1" location="packages/react-grab/src/components/overlay-canvas.tsx:393">
P2: Add a check for `props.freeformStrokeVisible` at the beginning of `renderFreeformLayer` to ensure the stroke is hidden when the prop is false.</violation>
<violation number="2" location="packages/react-grab/src/components/overlay-canvas.tsx:593">
P1: Remove the `else if` block that continuously sets `shouldContinueAnimating = true` when drawing to prevent an infinite `requestAnimationFrame` loop and high CPU usage.</violation>
</file>
<file name="packages/react-grab/e2e/freeform-drawing.spec.ts">
<violation number="1" location="packages/react-grab/e2e/freeform-drawing.spec.ts:6">
P2: Missing test coverage for the draw mode toolbar button. The tests currently only cover the 'Alt' key activation path.</violation>
<violation number="2" location="packages/react-grab/e2e/freeform-drawing.spec.ts:117">
P2: This test completely duplicates the logic from 'should enter comment mode via arrow gesture' (lines 7-29). Consider moving the `isOverlayVisible` assertion into the first test and removing this duplicate to reduce maintenance burden.</violation>
</file>
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:490">
P1: Freeform timers `freeformCleanupTimerId` and `freeformIdleTimerId` are never cleared in `onCleanup`, leaking timers on component disposal. Both should be cleared alongside the other timers in the existing cleanup block.</violation>
<violation number="2" location="packages/react-grab/src/core/index.tsx:1528">
P1: `deactivateRenderer` clears draw mode but does not call `cancelFreeformDraw()`. A mid-stroke deactivation leaves `document.body.style.userSelect = "none"` unreset, freeform signals in a dirty state, and pending timers that will fire on a deactivated renderer.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| freeformFadeOpacity = 1; | ||
| shouldContinueAnimating = true; | ||
| } | ||
| } else if ( |
There was a problem hiding this comment.
P1: Remove the else if block that continuously sets shouldContinueAnimating = true when drawing to prevent an infinite requestAnimationFrame loop and high CPU usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/components/overlay-canvas.tsx, line 593:
<comment>Remove the `else if` block that continuously sets `shouldContinueAnimating = true` when drawing to prevent an infinite `requestAnimationFrame` loop and high CPU usage.</comment>
<file context>
@@ -517,6 +576,28 @@ export const OverlayCanvas: Component<OverlayCanvasProps> = (props) => {
+ freeformFadeOpacity = 1;
+ shouldContinueAnimating = true;
+ }
+ } else if (
+ props.freeformStrokeVisible &&
+ props.freeformStrokePoints &&
</file context>
| ); | ||
| const [freeformStrokeCompletedAt, setFreeformStrokeCompletedAt] = | ||
| createSignal<number | null>(null); | ||
| let freeformCleanupTimerId: ReturnType<typeof setTimeout> | null = null; |
There was a problem hiding this comment.
P1: Freeform timers freeformCleanupTimerId and freeformIdleTimerId are never cleared in onCleanup, leaking timers on component disposal. Both should be cleared alongside the other timers in the existing cleanup block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/core/index.tsx, line 490:
<comment>Freeform timers `freeformCleanupTimerId` and `freeformIdleTimerId` are never cleared in `onCleanup`, leaking timers on component disposal. Both should be cleared alongside the other timers in the existing cleanup block.</comment>
<file context>
@@ -467,6 +477,29 @@ export const init = (rawOptions?: Options): ReactGrabAPI => {
+ );
+ const [freeformStrokeCompletedAt, setFreeformStrokeCompletedAt] =
+ createSignal<number | null>(null);
+ let freeformCleanupTimerId: ReturnType<typeof setTimeout> | null = null;
+ let freeformIdleTimerId: ReturnType<typeof setTimeout> | null = null;
+
</file context>
| arrowNavigator.clearHistory(); | ||
| keyboardSelectedElement = null; | ||
| isPendingContextMenuSelect = false; | ||
| setIsDrawMode(false); |
There was a problem hiding this comment.
P1: deactivateRenderer clears draw mode but does not call cancelFreeformDraw(). A mid-stroke deactivation leaves document.body.style.userSelect = "none" unreset, freeform signals in a dirty state, and pending timers that will fire on a deactivated renderer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/core/index.tsx, line 1528:
<comment>`deactivateRenderer` clears draw mode but does not call `cancelFreeformDraw()`. A mid-stroke deactivation leaves `document.body.style.userSelect = "none"` unreset, freeform signals in a dirty state, and pending timers that will fire on a deactivated renderer.</comment>
<file context>
@@ -1491,6 +1525,7 @@ export const init = (rawOptions?: Options): ReactGrabAPI => {
arrowNavigator.clearHistory();
keyboardSelectedElement = null;
isPendingContextMenuSelect = false;
+ setIsDrawMode(false);
if (wasDragging) {
document.body.style.userSelect = "";
</file context>
| const context = layer.context; | ||
| context.clearRect(0, 0, canvasWidth, canvasHeight); | ||
|
|
||
| const strokes = props.freeformStrokePoints; |
There was a problem hiding this comment.
P2: Add a check for props.freeformStrokeVisible at the beginning of renderFreeformLayer to ensure the stroke is hidden when the prop is false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/components/overlay-canvas.tsx, line 393:
<comment>Add a check for `props.freeformStrokeVisible` at the beginning of `renderFreeformLayer` to ensure the stroke is hidden when the prop is false.</comment>
<file context>
@@ -366,6 +383,46 @@ export const OverlayCanvas: Component<OverlayCanvasProps> = (props) => {
+ const context = layer.context;
+ context.clearRect(0, 0, canvasWidth, canvasHeight);
+
+ const strokes = props.freeformStrokePoints;
+ if (!strokes || strokes.length === 0) return;
+ if (freeformFadeOpacity <= 0) return;
</file context>
| const strokes = props.freeformStrokePoints; | |
| if (!props.freeformStrokeVisible) return; | |
| const strokes = props.freeformStrokePoints; |
| expect(state.isActive).toBe(false); | ||
| }); | ||
|
|
||
| test("should keep overlay active in comment mode after freeform gesture", async ({ |
There was a problem hiding this comment.
P2: This test completely duplicates the logic from 'should enter comment mode via arrow gesture' (lines 7-29). Consider moving the isOverlayVisible assertion into the first test and removing this duplicate to reduce maintenance burden.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/e2e/freeform-drawing.spec.ts, line 117:
<comment>This test completely duplicates the logic from 'should enter comment mode via arrow gesture' (lines 7-29). Consider moving the `isOverlayVisible` assertion into the first test and removing this duplicate to reduce maintenance burden.</comment>
<file context>
@@ -0,0 +1,168 @@
+ expect(state.isActive).toBe(false);
+ });
+
+ test("should keep overlay active in comment mode after freeform gesture", async ({
+ reactGrab,
+ }) => {
</file context>
| @@ -0,0 +1,168 @@ | |||
| import { test, expect } from "./fixtures.js"; | |||
There was a problem hiding this comment.
P2: Missing test coverage for the draw mode toolbar button. The tests currently only cover the 'Alt' key activation path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/e2e/freeform-drawing.spec.ts, line 6:
<comment>Missing test coverage for the draw mode toolbar button. The tests currently only cover the 'Alt' key activation path.</comment>
<file context>
@@ -0,0 +1,168 @@
+const FREEFORM_IDLE_TIMEOUT_MS = 600;
+const GESTURE_SETTLE_BUFFER_MS = 300;
+
+test.describe("Freeform Drawing", () => {
+ test("should enter comment mode via arrow gesture", async ({ reactGrab }) => {
+ await reactGrab.activate();
</file context>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestions:
- When deactivateRenderer() is called, active freeform drawing sessions are not cancelled, leaving timers running and potentially causing memory leaks
- The pointercancel event listener is missing the capture: true option while pointerdown and pointerup use it, causing inconsistent event handling phases
| ): StrokePoint => ({ | ||
| x: clientX, | ||
| y: clientY, | ||
| pressure: pressure || FREEFORM_STROKE_DEFAULT_PRESSURE, |
There was a problem hiding this comment.
| pressure: pressure || FREEFORM_STROKE_DEFAULT_PRESSURE, | |
| pressure: pressure ?? FREEFORM_STROKE_DEFAULT_PRESSURE, |
Pressure value of 0 incorrectly replaced with default pressure in createStrokePoint function
![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 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| arrowNavigator.clearHistory(); | ||
| keyboardSelectedElement = null; | ||
| isPendingContextMenuSelect = false; | ||
| setIsDrawMode(false); |
There was a problem hiding this comment.
Deactivation doesn't cancel freeform timers or state
High Severity
deactivateRenderer sets isDrawMode(false) but never calls cancelFreeformDraw(). If the renderer deactivates (via keyboard shortcut, toggle button, etc.) while a freeform session is active or during the 600ms idle timeout window, freeformIdleTimerId keeps running and eventually fires finalizeFreeformSession. That calls handleFreeformCircle/handleFreeformArrow, which invoke actions.freeze(), enterCommentModeForElement, and other store mutations on a deactivated renderer — leaving the app in an inconsistent state. Additionally, document.body.style.userSelect may remain "none" since the drag-specific cleanup guard (wasDragging) doesn't cover freeform drawing.
Additional Locations (1)
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
|
|
||
| freeformIdleTimerId = setTimeout(() => { | ||
| freeformIdleTimerId = null; | ||
| finalizeFreeformSession(); |
There was a problem hiding this comment.
[🟡 Medium]
The idle timeout finalizes the freeform session unconditionally, even if the renderer was deactivated after pointer-up (for example via toolbar toggle) but before the 600ms idle delay expires. That allows a canceled gesture to still freeze elements and enter prompt mode after the user has explicitly turned the overlay off. ```ts
// packages/react-grab/src/core/index.tsx
freeformIdleTimerId = setTimeout(() => {
freeformIdleTimerId = null;
finalizeFreeformSession();
}, FREEFORM_IDLE_TIMEOUT_MS);
```suggestion
if (isActivated()) finalizeFreeformSession(); else cancelFreeformDraw();
1 participant
Summary
Test plan
pnpm testto verify e2e tests passMade with Cursor
Summary by cubic
Add freeform drawing to select elements and open the comment prompt. Draw a circle to multi-select or an arrow to select one element, using Alt+drag or the new Draw toolbar button.
New Features
Bug Fixes
Written for commit 413a2cb. Summary will update on new commits.
Note
Medium Risk
Touches core pointer event handling and overlay state transitions, so regressions could affect drag/select behavior and activation/deactivation flow; changes are contained and covered by new e2e tests.
Overview
Adds a freeform drawing interaction (Alt-drag or new toolbar Draw toggle) that records pointer strokes, classifies them as
arrowvscircle, and uses the result to select/freeze either a single element (arrow endpoint) or multiple elements within the stroke bounding box (circle) before entering comment prompt mode.Updates the overlay renderer to draw and fade pressure-weighted strokes on a new
freeformcanvas layer, and suppresses crosshair/selection/label rendering while a freeform session is active to avoid UI conflicts.Introduces
StrokePointtyping, new freeform-related constants, aclassify-stroke-gestureutility, a newIconDraw, and adds Playwright e2e coverage for gesture recognition, overlay visibility behavior, and insufficient-point discards; also adjusts click/cancel handling so capture-phase deactivation doesn’t interrupt freeform sessions.Written by Cursor Bugbot for commit 413a2cb. This will update automatically on new commits. Configure here.