Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature implementation with good UX patterns. Found 3 critical issues: memory leak in recentElementMap, potential ID collision in rapid operations, and missing sync between storage and state on item removal. Review comments below detail each concern and suggest fixes.
| import { MAX_RECENT_ITEMS } from "../constants.js"; | ||
| import type { RecentItem } from "../types.js"; | ||
|
|
||
| let recentItems: RecentItem[] = []; |
There was a problem hiding this comment.
Memory leak risk: This module-level variable persists for the entire application lifetime. The recentElementMap in core/index.tsx stores DOM element references keyed by these item IDs. When elements are removed from the DOM but still referenced in recentElementMap, they cannot be garbage collected.
Problem scenario:
- User copies element A (stored in recentElementMap)
- Element A is removed from DOM
- recentItems still contains the item (up to 20 items retained)
- recentElementMap still holds the detached element reference
- Element cannot be GC'd → memory leak
Suggested fix: Implement a cleanup mechanism that periodically removes disconnected elements from recentElementMap, or use WeakMap if the reference pattern allows.
| const generateRecentItemId = (): string => | ||
| `recent-${Date.now()}-${Math.random().toString(36).substring(2, 9)}`; |
There was a problem hiding this comment.
ID collision risk: Date.now() returns milliseconds. If addRecentItem() is called multiple times within the same millisecond (possible with rapid copy operations or programmatic calls), the random suffix (7 chars) is the only uniqueness guarantee.
Probability: With base-36 encoding and 7 characters, collision probability is ~1 in 78 billion per millisecond window. However:
- Multiple rapid copies in the same millisecond are plausible
- The deduplication logic (lines 690-704 in core/index.tsx) relies on unique IDs to map elements
- A collision would cause
recentElementMap.set()to overwrite the wrong element mapping
Suggested fix: Add a monotonic counter as a tie-breaker: recent-${Date.now()}-${counter++}-${Math.random()...}
There was a problem hiding this comment.
State synchronization issue: This deduplication logic calls removeRecentItem() which updates the module-level recentItems array in recent-storage.ts, but doesn't call setRecentItems() to update the local reactive state.
Problem: The recentItems() signal used throughout this component (line 689, 716, 3308, 3349, 3363, 3390, 3527) won't reflect the removal. This causes:
- Dropdown shows stale items until next copy operation
- Hover handlers may reference wrong elements
- "Copy all" includes deduplicated items
Suggested fix:
if (shouldDedup) {
const updated = removeRecentItem(existingItemId);
setRecentItems(updated);
recentElementMap.delete(existingItemId);
break;
}| // HACK: Delay mousedown/touchstart listener to avoid catching the triggering click | ||
| const frameId = requestAnimationFrame(() => { | ||
| window.addEventListener("mousedown", handleClickOutside, { | ||
| capture: true, | ||
| }); | ||
| window.addEventListener("touchstart", handleClickOutside, { | ||
| capture: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Timing edge case: The requestAnimationFrame delay avoids catching the triggering click, but creates a race condition:
- User clicks "Recent" button at T=0
- Dropdown becomes visible at T=0
onMountruns at T=0- Event listeners are added at T=16ms (next frame)
- If user clicks elsewhere between T=0 and T=16ms, no dismissal occurs
Impact: Low probability but possible on fast interactions. The dropdown would stay open until the next click.
Alternative approach: Check event timestamp and ignore events within a threshold of mount time, eliminating the rAF delay:
const mountTime = Date.now();
const handleClickOutside = (event: MouseEvent | TouchEvent) => {
if (Date.now() - mountTime < 50) return; // Ignore first 50ms
// ... rest of handler
};
window.addEventListener('mousedown', handleClickOutside, { capture: true });| const handleRecentItemSelect = (item: RecentItem) => { | ||
| dismissRecentDropdown(); | ||
|
|
||
| const element = recentElementMap.get(item.id); |
There was a problem hiding this comment.
Missing element lifecycle check: When selecting a recent item, the code retrieves the element from recentElementMap but doesn't verify it still exists in the map before use.
Scenario:
- Recent item exists in dropdown
- User calls
handleRecentClear()(line 3398) which clears recentElementMap - Simultaneously/immediately after, user's click on an item triggers
handleRecentItemSelect() recentElementMap.get(item.id)returnsundefined- Code proceeds with
elementbeingundefined
Current protection: Line 3324 checks isElementConnected(element) which handles undefined gracefully (returns false). Line 3332 also checks. So this is defensive, not broken.
Recommendation: Consider making the element retrieval more explicit for clarity:
const element = recentElementMap.get(item.id);
if (!element || !isElementConnected(element)) return;This makes the intentionality clear to future maintainers.
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@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.
1 issue found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:716">
P2: Prune `recentElementMap` when recent items are truncated; otherwise dropped items still keep DOM element references alive, leading to a memory leak.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
…nstants - Fix e2e test selector to use fill="currentColor" instead of fill="#404040" to match IconInboxUnread component - Fix font family name from "Inter Variable" to "Inter" to match Google Fonts registration - Remove unused TOOLBAR_DOCK_PREVIEW_SNAP_DURATION_MS and TOOLBAR_DOCK_PREVIEW_SIDE_ROTATION_DEG constants
…out on drag start
…nsition-all The class string 'grid transition-all transition-transform ...' caused twMerge to keep only transition-transform, breaking grid-cols and opacity animations. Removed transition-transform since transition-all is needed for the button container animations and already includes transform transitions.
When handlePointerDown clears the snapAnimationTimeout to avoid unintended snaps, it now also calls setIsSnapping(false) to prevent the snap state from getting permanently stuck. This ensures getTransitionClass returns the correct transition class for subsequent toolbar operations instead of always returning the 300ms snap transition.
| if (toggleAnchorLockAnimationFrame === undefined) return; | ||
| cancelAnimationFrame(toggleAnchorLockAnimationFrame); | ||
| toggleAnchorLockAnimationFrame = undefined; | ||
| }; |
There was a problem hiding this comment.
Dead toggle anchor lock infrastructure never assigned
Low Severity
toggleAnchorLockAnimationFrame is declared and has a clearToggleAnchorLockAnimationFrame helper, which is actively called in handleToggleEnabled and onCleanup, but the variable is never actually assigned a requestAnimationFrame ID. All clear calls are no-ops. Similarly, toggleEnabledButtonRef is bound via ref but never read. This appears to be scaffolding for a toggle anchor-locking feature that was wired up but never completed.
Additional Locations (1)
| export const OVERLAY_FILL_COLOR_DEFAULT = `rgba(${GRAB_PURPLE_RGB}, 0.08)`; | ||
| export const FROZEN_GLOW_COLOR = `rgba(${GRAB_PURPLE_RGB}, 0.15)`; | ||
| export const FROZEN_GLOW_EDGE_PX = 50; | ||
| export const TOOLBAR_ACTIVE_ACCENT_COLOR = "#FF96EC"; |
There was a problem hiding this comment.
Unused exported constant TOOLBAR_ACTIVE_ACCENT_COLOR
Low Severity
TOOLBAR_ACTIVE_ACCENT_COLOR is exported from constants.ts but is never imported or referenced anywhere in the codebase. It appears to be dead code introduced in this commit.
3 participants
Note
Medium Risk
Large UI/interaction refactor across the overlay toolbar and keyboard handlers; risk is mainly regressions in dragging/snap behavior and shortcut/event handling rather than data/security concerns.
Overview
Adds richer History (Recent) interactions: the dropdown UI is redesigned with icon actions + tooltips, empty-state messaging, and full keyboard navigation (ArrowUp/Down highlight + Enter select), plus a new
Rshortcut to open it while the overlay is active. Clearing is renamed/rewired toclearRecentItems/onRecentClearAll, the dropdown is dismissed on overlay deactivation, and overlay keyboard handling now defers Enter/Escape/arrow-navigation when history is open.Refactors the toolbar drag/dock system to support bottom snapping, side docking with vertical layout, dock previews, and release/transition animations; exposes
data-react-grab-toolbar-orientationfor tests and reads persisted edge state for snap assertions. Updates icons (newIconCopy/IconTrash, revisedIconSelect/IconInbox, active-capableIconComment), tweaks unread indicator detection in e2e, and switches overlay/site typography to Inter with improved font smoothing and a newanimate-toolbar-dock-shiftanimation.Written by Cursor Bugbot for commit 730303a. This will update automatically on new commits. Configure here.
Summary by cubic
Adds a “Recent” history with a toolbar inbox for up to 20 past copies, plus a refined draggable toolbar with dock previews, release animations, side‑vertical layouts, bottom snap, and a new active accent color. Updates dark‑mode icon styling and switches to Inter with improved font smoothing across the overlay and site.
New Features
Bug Fixes
Written for commit 730303a. Summary will update on new commits.