Refactor: Extract modules from core/index.tsx, eliminate silent error swallowing#250
Merged
Refactor: Extract modules from core/index.tsx, eliminate silent error swallowing#250
Conversation
Contributor
|
Error agent completed without reporting progress |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@react-grab/cli
grab
@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: |
![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}
Contributor
There was a problem hiding this comment.
7 issues found across 19 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/toolbar-position.ts">
<violation number="1" location="packages/react-grab/src/utils/toolbar-position.ts:193">
P2: Add `TOOLBAR_SNAP_MARGIN_PX` to bounds to prevent the collapsed toolbar from ignoring the snap margin.</violation>
</file>
<file name="packages/react-grab/src/core/label-manager.ts">
<violation number="1" location="packages/react-grab/src/core/label-manager.ts:76">
P1: The inner fade completion timeout is untracked, causing a timer leak on bulk clear. Fixing this also requires updating hover logic to prevent zombie labels.</violation>
</file>
<file name="packages/react-grab/src/utils/create-toolbar-drag.ts">
<violation number="1" location="packages/react-grab/src/utils/create-toolbar-drag.ts:106">
P1: Velocity becomes stale if the pointer stops moving before release, causing incorrect snapping velocity.</violation>
<violation number="2" location="packages/react-grab/src/utils/create-toolbar-drag.ts:156">
P1: Snap animation timeouts are not cleared on new drags, which can incorrectly interrupt the new drag session.</violation>
<violation number="3" location="packages/react-grab/src/utils/create-toolbar-drag.ts:162">
P1: `didDragOccur` remains stuck as `true` if a drag occurs on a non-button area, causing the next button click to be silently ignored.</violation>
<violation number="4" location="packages/react-grab/src/utils/create-toolbar-drag.ts:178">
P1: Missing `pointercancel` listener leaves the drag state permanently stuck if interrupted by the system.</violation>
<violation number="5" location="packages/react-grab/src/utils/create-toolbar-drag.ts:182">
P1: `stopImmediatePropagation` is called unconditionally, breaking normal event delegation and outside-click detection for regular clicks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
|
|
||
| const handlePointerDown = (event: PointerEvent) => { | ||
| if (config.isCollapsed()) return; |
Contributor
There was a problem hiding this comment.
P1: Snap animation timeouts are not cleared on new drags, which can incorrectly interrupt the new drag session.
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/create-toolbar-drag.ts, line 156:
<comment>Snap animation timeouts are not cleared on new drags, which can incorrectly interrupt the new drag session.</comment>
<file context>
@@ -0,0 +1,203 @@
+ };
+
+ const handlePointerDown = (event: PointerEvent) => {
+ if (config.isCollapsed()) return;
+
+ const containerRef = config.getContainerRef();
</file context>
Suggested change
| if (config.isCollapsed()) return; | |
| clearTimeout(snapAnimationTimeout); | |
| setIsSnapping(false); | |
| if (config.isCollapsed()) return; |
| const rect = containerRef?.getBoundingClientRect(); | ||
| if (!rect) return; | ||
|
|
||
| const currentVelocity = velocity(); |
Contributor
There was a problem hiding this comment.
P1: Velocity becomes stale if the pointer stops moving before release, causing incorrect snapping velocity.
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/create-toolbar-drag.ts, line 106:
<comment>Velocity becomes stale if the pointer stops moving before release, causing incorrect snapping velocity.</comment>
<file context>
@@ -0,0 +1,203 @@
+ const rect = containerRef?.getBoundingClientRect();
+ if (!rect) return;
+
+ const currentVelocity = velocity();
+ const snap = getSnapPosition(
+ rect.left,
</file context>
| }; | ||
|
|
||
| window.addEventListener("pointermove", handleWindowPointerMove); | ||
| window.addEventListener("pointerup", handleWindowPointerUp); |
Contributor
There was a problem hiding this comment.
P1: Missing pointercancel listener leaves the drag state permanently stuck if interrupted by the system.
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/create-toolbar-drag.ts, line 178:
<comment>Missing `pointercancel` listener leaves the drag state permanently stuck if interrupted by the system.</comment>
<file context>
@@ -0,0 +1,203 @@
+ };
+
+ window.addEventListener("pointermove", handleWindowPointerMove);
+ window.addEventListener("pointerup", handleWindowPointerUp);
+ };
+
</file context>
| const rect = containerRef?.getBoundingClientRect(); | ||
| if (!rect) return; | ||
|
|
||
| pointerStartPosition = { x: event.clientX, y: event.clientY }; |
Contributor
There was a problem hiding this comment.
P1: didDragOccur remains stuck as true if a drag occurs on a non-button area, causing the next button click to be silently ignored.
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/create-toolbar-drag.ts, line 162:
<comment>`didDragOccur` remains stuck as `true` if a drag occurs on a non-button area, causing the next button click to be silently ignored.</comment>
<file context>
@@ -0,0 +1,203 @@
+ const rect = containerRef?.getBoundingClientRect();
+ if (!rect) return;
+
+ pointerStartPosition = { x: event.clientX, y: event.clientY };
+
+ dragOffset = {
</file context>
Suggested change
| pointerStartPosition = { x: event.clientX, y: event.clientY }; | |
| didDragOccur = false; | |
| pointerStartPosition = { x: event.clientX, y: event.clientY }; |
Comment on lines
+182
to
+189
| (callback: () => void) => (event: MouseEvent) => { | ||
| event.stopImmediatePropagation(); | ||
| if (didDragOccur) { | ||
| didDragOccur = false; | ||
| return; | ||
| } | ||
| callback(); | ||
| }; |
Contributor
There was a problem hiding this comment.
P1: stopImmediatePropagation is called unconditionally, breaking normal event delegation and outside-click detection for regular clicks.
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/create-toolbar-drag.ts, line 182:
<comment>`stopImmediatePropagation` is called unconditionally, breaking normal event delegation and outside-click detection for regular clicks.</comment>
<file context>
@@ -0,0 +1,203 @@
+ };
+
+ const createDragAwareHandler =
+ (callback: () => void) => (event: MouseEvent) => {
+ event.stopImmediatePropagation();
+ if (didDragOccur) {
</file context>
Suggested change
| (callback: () => void) => (event: MouseEvent) => { | |
| event.stopImmediatePropagation(); | |
| if (didDragOccur) { | |
| didDragOccur = false; | |
| return; | |
| } | |
| callback(); | |
| }; | |
| (callback: () => void) => (event: MouseEvent) => { | |
| if (didDragOccur) { | |
| event.preventDefault(); | |
| event.stopImmediatePropagation(); | |
| didDragOccur = false; | |
| return; | |
| } | |
| callback(); | |
| }; |
| const centeredX = expandedPosition.x + xOffset; | ||
| const clampedX = clampToRange( | ||
| centeredX, | ||
| viewport.offsetLeft, |
Contributor
There was a problem hiding this comment.
P2: Add TOOLBAR_SNAP_MARGIN_PX to bounds to prevent the collapsed toolbar from ignoring the snap margin.
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/toolbar-position.ts, line 193:
<comment>Add `TOOLBAR_SNAP_MARGIN_PX` to bounds to prevent the collapsed toolbar from ignoring the snap margin.</comment>
<file context>
@@ -102,6 +102,125 @@ export const getRatioFromPosition = (
+ const centeredX = expandedPosition.x + xOffset;
+ const clampedX = clampToRange(
+ centeredX,
+ viewport.offsetLeft,
+ viewport.offsetLeft + viewport.width - collapsedWidth,
+ );
</file context>
Contributor
There was a problem hiding this comment.
2 issues found across 4 files (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/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:621">
P2: The inner fade-completion timeout is untracked and cannot be cancelled, causing orphaned timer leaks during cleanup.</violation>
<violation number="2" location="packages/react-grab/src/core/index.tsx:640">
P2: Hovering an error or fading label permanently cancels its fade, leaving it stuck on the screen.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…s the application. Replace PANEL_STYLES with "bg-white" in multiple components to standardize appearance.
…ents for enhanced clarity and maintainability. Adjust event handling in context menus and toolbars to streamline user interactions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… swallowing
- Extract label lifecycle management into `core/label-manager.ts` (create, fade, hover, timer cleanup)
- Extract dropdown position tracking into `core/dropdown-controller.ts` (RAF-based tracking, anchor computation)
- Create `utils/log-recoverable-error.ts` to replace 12 silent `catch {}` blocks across session.ts, history-storage.ts, freeze-updates.ts, copy-html/styles plugins, and core/index.tsx
- Create `utils/generate-id.ts` to consolidate 4 duplicate ID generators (label, session, history, grabbed-box)
- Create `utils/get-element-center.ts` to eliminate repeated getBoundsCenter(createElementBounds(element)) pattern
- Rename detectionState fields for clarity (lastTime→lastDetectionTimestamp, latestX→latestPointerX, etc.)
- Fix: labelManager.clearAll() now cancels orphaned fade timers that previously leaked when instances were cleared
583/584 e2e tests pass (1 pre-existing flaky). 0 lint errors, 0 type errors.
Made-with: Cursor
…viewport test - Inline label-manager.ts and dropdown-controller.ts back into core/index.tsx — they created DI interfaces for code with exactly one consumer - Keep the actual improvements: cancelAllLabelFades() for orphaned timer cleanup, clearAllLabels() for combined clear+cancel - Fix flaky "should handle elements outside viewport" test: use scrollIntoViewIfNeeded() instead of fixed scrollPage(1000) which was viewport-size-dependent 584/584 e2e tests pass, 0 flaky. Made-with: Cursor
aidenybai
force-pushed
the
refactor/code-quality-audit
branch
from
900d31d to
e5ab8e1
Compare
Made-with: Cursor
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.
| ): string => { | ||
| actions.clearLabelInstances(); | ||
| const instanceId = generateLabelId(); | ||
| cancelAllLabelFades(); |
There was a problem hiding this comment.
createLabelInstance duplicates clearAllLabels logic inline
Low Severity
createLabelInstance manually calls actions.clearLabelInstances() then cancelAllLabelFades(), duplicating what the newly introduced clearAllLabels() helper does (in reversed order). All four other call sites correctly use clearAllLabels(). If clearAllLabels gains additional cleanup logic later, createLabelInstance won't benefit and the two will silently diverge.
Additional Locations (1)
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Comment on lines
+620
to
+624
| labelFadeTimeouts.delete(instanceId); | ||
| actions.updateLabelInstance(instanceId, "fading"); | ||
| setTimeout(() => { | ||
| removeLabelInstance(instanceId); | ||
| }, FADE_COMPLETE_BUFFER_MS); |
Contributor
There was a problem hiding this comment.
Suggested change
| labelFadeTimeouts.delete(instanceId); | |
| actions.updateLabelInstance(instanceId, "fading"); | |
| setTimeout(() => { | |
| removeLabelInstance(instanceId); | |
| }, FADE_COMPLETE_BUFFER_MS); | |
| actions.updateLabelInstance(instanceId, "fading"); | |
| const fadeCompleteTimeoutId = window.setTimeout(() => { | |
| removeLabelInstance(instanceId); | |
| }, FADE_COMPLETE_BUFFER_MS); | |
| labelFadeTimeouts.set(instanceId, fadeCompleteTimeoutId); |
Inner fade-completion timeout is untracked and cannot be cancelled, creating orphaned timer leaks
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
core/label-manager.ts— moves label create/fade/hover/timer cleanup out of the 4000-lineinit()closure into a focused module with explicitdispose()for timer cleanupcore/dropdown-controller.ts— moves RAF-based position tracking, anchor computation, andgetNearestEdgeinto a reusable controllercatch {}blocks — createsutils/log-recoverable-error.ts(dev-onlyconsole.warn) and replaces all swallowed errors insession.ts,history-storage.ts,freeze-updates.ts,copy-html.ts,copy-styles.ts, andcore/index.tsxutils/generate-id.ts— replacesgenerateLabelId,generateSessionId,generateHistoryItemId, and inline grabbed-box ID generationutils/get-element-center.ts— eliminates repeatedgetBoundsCenter(createElementBounds(element))pattern (3 call sites)detectionStatefields for clarity:lastTime→lastDetectionTimestamp,latestX/Y→latestPointerX/Y,pendingScheduledAt→pendingDetectionScheduledAtlabelManager.clearAll()now cancels pending fade timers when label instances are bulk-cleared (previously, timers would fire on already-removed instances)Net result: -221 lines from
core/index.tsx, +359 lines across 5 focused new files.Test plan
pnpm typecheck— 0 errorspnpm lint— 0 warnings, 0 errorspnpm format— cleanpnpm test— 583/584 e2e tests pass (1 pre-existing flaky: viewport edge case timeout)Made with Cursor
Note
Medium Risk
Touches core overlay interaction code (label lifecycle/timers, pointer detection, and cleanup) and storage/session handling; behavior should be unchanged but regressions could affect selection/copy UX. Adds new logging paths that may surface previously swallowed errors in development.
Overview
Removes the “Primitives” documentation sections from the root and package READMEs.
Standardizes ephemeral ID creation via new
utils/generate-id.ts, introducesutils/get-element-center.ts, and updates core selection/copy flows to use them (including label and grabbed-box IDs and element-center calculations).Replaces multiple silent
catch {}blocks across agent session storage, history storage, freeze/unfreeze updates, core actions, and copy plugins withutils/log-recoverable-error.ts(dev-only warnings), and adds label fade timer management/cleanup (cancelAllLabelFades/clearAllLabels) to prevent orphaned timers.Stabilizes an e2e viewport edge case by scrolling the footer into view with
scrollIntoViewIfNeeded()before hovering.Written by Cursor Bugbot for commit 3cf2716. This will update automatically on new commits. Configure here.
Summary by cubic
Refactors core overlay logic and error handling to improve reliability and maintainability, standardizes UI constants/styles, and removes the Primitives docs. Reverts label/dropdown over-abstractions back into
core/index.tsxwhile keeping better cleanup.Refactors
core/index.tsx(keptcancelAllLabelFadesandclearAllLabels).utils/log-recoverable-error.tsacross session/history storage, plugins, freeze/unfreeze, and core.utils/generate-id.ts; addedutils/get-element-center.tsand used in core.README.md,packages/grab/README.md, andpackages/react-grab/README.md.Bug Fixes
scrollIntoViewIfNeeded().registerOverlayDismiss(enabled only for the context menu).resolveBooleanEnableddeclaration from a stash merge.Written for commit 3cf2716. Summary will update on new commits.