Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
packages/react-grab/src/components/toolbar/toolbar-entry-container.tsx
Outdated
Show resolved
Hide resolved
![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 13 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/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:3898">
P2: `dismissAllPopups` correctly includes `dismissToolbarEntry()`, and `handleToggleToolbarEntry` dismisses other popups before opening — but the reverse is not true. The existing popup-opening functions (`handleToggleToolbarMenu`, `openCommentsDropdown`, `showClearPrompt`) were not updated to call `dismissToolbarEntry()`. Opening any of those while a toolbar entry dropdown is visible will leave both popups visible simultaneously.</violation>
<violation number="2" location="packages/react-grab/src/core/index.tsx:3903">
P2: Dismiss the active toolbar entry before opening a new one to ensure its `onClose` fires. Also consider adding `dismissToolbarEntry()` to the other existing popup handlers to maintain mutual exclusion.</violation>
</file>
<file name="packages/react-grab/src/components/toolbar/toolbar-content.tsx">
<violation number="1" location="packages/react-grab/src/components/toolbar/toolbar-content.tsx:296">
P2: The nullish coalescing operator (`??`) prevents clearing badges. A plugin calling `handle.setBadge(undefined)` cannot hide an initial badge because `undefined ?? fallback` returns the fallback.</violation>
</file>
<file name="packages/gym/components/toolbar-entries-provider.tsx">
<violation number="1" location="packages/gym/components/toolbar-entries-provider.tsx:99">
P1: The click handler does not actually toggle the status, contradicting the test plan.</violation>
</file>
<file name="packages/react-grab/src/core/plugin-registry.ts">
<violation number="1" location="packages/react-grab/src/core/plugin-registry.ts:199">
P2: SolidJS `setStore` deep-merges objects and does not delete missing keys. Set the removed keys to `undefined` to actually delete them from the store.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 18 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:3858">
P2: Stale `toolbarEntryOverrides` are never cleaned up on plugin unregister. The old cleanup block in `plugin-registry.ts` was removed, and the new `unregisterPlugin` code here only dismisses the active dropdown. If a plugin is unregistered and later re-registered with the same entry IDs, overrides from the previous lifecycle (set via `handle.setIcon`, `handle.setBadge`, etc.) silently persist and override the fresh entry's properties. Add cleanup of `toolbarEntryOverrides` for the removed plugin's entry IDs, either here or back in the registry's `unregister` method.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 14 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/gym/components/toolbar-entries-provider.tsx">
<violation number="1" location="packages/gym/components/toolbar-entries-provider.tsx:46">
P2: The `traverseFiber` function increments the render count for every component in the fiber tree unconditionally on every commit. This effectively counts 'components × commits' rather than identifying which components actually rendered. To accurately measure renders, verify if the fiber updated during the commit (e.g., by checking `fiber.memoizedProps !== fiber.alternate?.memoizedProps` or using `fiber.flags`).</violation>
<violation number="2" location="packages/gym/components/toolbar-entries-provider.tsx:86">
P1: When intercepting `onCommitFiberRoot`, the wrapper function explicitly forwards only `rendererID` and `fiberRoot`. If the original `onCommitFiberRoot` expects additional arguments (like `commitPriority` or `didError` in newer React versions), they will be dropped, which can break React DevTools. Use rest parameters to forward all arguments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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/gym/components/toolbar-entries-provider.tsx">
<violation number="1" location="packages/gym/components/toolbar-entries-provider.tsx:65">
P2: This props/state equality check misses real React renders, so the render monitor undercounts parent- and context-driven updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| pluginRegistry.updateToolbarEntry(entryId, { badge }), | ||
| setVisible: (isVisible) => | ||
| pluginRegistry.updateToolbarEntry(entryId, { isVisible }), | ||
| }); |
There was a problem hiding this comment.
New handle created each call defeats documented caching
Low Severity
getToolbarEntryHandle creates a brand-new object on every invocation, contradicting the PR description's claim of "cache handles per entry ID for stable identity." While currently functional (methods operate by entryId), the lack of a Map-based cache means every call to activeToolbarEntryHandle() or handleToggleToolbarEntry allocates a fresh object. If any consumer later relies on referential identity (e.g., in a reactive comparison or as a Map key), it will silently break.
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/gym/components/toolbar-entries-provider.tsx">
<violation number="1" location="packages/gym/components/toolbar-entries-provider.tsx:57">
P2: `fiber.flags > 0` is not a reliable re-render signal; static fiber flags will make the render monitor overcount memoized components.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fiber.memoizedProps !== fiber.alternate.memoizedProps || | ||
| fiber.memoizedState !== fiber.alternate.memoizedState | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Render monitor flags check overcounts fiber renders
Medium Severity
The didFiberRender function's fiber.flags && fiber.flags > 0 check is far too broad. React fiber flags is a bitfield where many bits (like Ref, Placement, ChildDeletion, ContentReset) can be set on fibers that didn't actually re-render their component function. This causes the render monitor to significantly overcount renders, directly contradicting the PR's stated fix of "count only fibers that actually re-rendered." A more targeted check like fiber.flags & 1 (the PerformedWork bit) would match only fibers whose component function was actually called.
| handle.setBadge(totalRenderCount); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Monitoring state inconsistent when devtools hook missing
Low Severity
startMonitoring sets isMonitoring = true on line 87 before checking if __REACT_DEVTOOLS_GLOBAL_HOOK__ exists on line 96. When the hook is absent, the function returns early with isMonitoring stuck as true but no actual monitoring installed. The toolbar button icon turns red (active appearance) even though nothing is being tracked, and the badge never updates — misleading the user into thinking monitoring is working.
Allow plugins to register persistent buttons in the toolbar strip via `toolbarEntries`. Entries support emoji/SVG/HTML icons, optional dropdown containers (onRender), action-only buttons (onClick), badges, visibility control, and lifecycle hooks (onOpen/onClose). Handles are cached per entry and provide full ReactGrabAPI access plus dropdown and display control. Includes gym demo with debug panel, screenshot action, and status indicator entries registered globally.
Remove redundant onOpen/onClose lifecycle hooks, eliminate toolbarEntryOverrides prop threading through 3 component layers, and drop the handle cache + stale cleanup bookkeeping.
Pre-merging overrides via object spread broke <For> referential identity and caused the container effect to re-fire onRender on every badge/icon change, resetting closure state.
Render monitor hooks into __REACT_DEVTOOLS_GLOBAL_HOOK__ to count component renders with per-component breakdown. FPS meter tracks frame timing via rAF with sparkline graph and drop detection. Test page has intentionally laggy components to exercise both.
- Add dismissToolbarEntry() to all popup openers for mutual exclusion - Fix badge clearing: use "in" check instead of ?? so setBadge(undefined) properly clears badges instead of falling through to the original value - Restore toolbarEntryOverrides cleanup on plugin unregister to prevent stale overrides persisting across re-registrations
SolidJS setStore deep-merges objects, so removed keys persist. Using reconcile ensures stale overrides are actually deleted when a plugin is unregistered. Also includes previously uncommitted README updates and script.js changes.
- Use rest params for onCommitFiberRoot to forward all arguments - Only count fibers that actually rendered (check alternate props/state) - Remove dead patchedRoots WeakSet code - Add dispose() to stop rAF loop and unhook devtools on plugin unregister
The props/state reference equality check alone misses renders triggered by context changes or parent re-renders where React sets flags on the fiber without changing memoizedProps/memoizedState references.
aidenybai
force-pushed
the
feat/toolbar-entries
branch
from
b34b49a to
191dedc
Compare
| setActiveToolbarEntryId(entryId); | ||
| openTrackedDropdown(setToolbarEntryDropdownPosition); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Calling handle.open() from onClick causes infinite recursion
Medium Severity
If a plugin's onClick callback calls handle.open() or handle.toggle(), it re-enters handleToggleToolbarEntry before setActiveToolbarEntryId has been called. Because the active ID hasn't been set yet, the guard in handle.open() (activeToolbarEntryId() !== entryId) passes again, triggering another handleToggleToolbarEntry → onClick → handle.open() cycle, resulting in a stack overflow. The same applies to action-only entries (no onRender), where onClick is called unconditionally.
Split the monolithic core/index.tsx (~5000 lines) into a plugin architecture with 7 focused plugins communicating via SharedPluginApi and interceptor chain: - keyboard-plugin: hold activation, keydown/keyup handling, Escape - pointer-plugin: mouse/touch tracking, element detection, drag, copy - navigation-plugin: arrow key navigation, action cycling - toolbar-plugin: toolbar state, activation/deactivation, freeze effects - menus-plugin: context menu, comments dropdown - prompt-plugin: prompt mode, agent integration - copy-pipeline: copy execution, clipboard, feedback labels Also fixes: - Set wasActivatedByToggle during keyboard hold activation so toggle-mode semantics (Escape to dismiss, modifier release ignored) work correctly - Add missing pointermove/pointerdown/pointerup event dispatchers to the interceptor chain (selection box rendering depended on these) - Fix flaky freeze-updates test by replacing fixed timeout with waitForFunction for element count assertion
|
|
||
| // HACK: defer to next frame so idle preview label clears visually before "copied" appears | ||
| nativeRequestAnimationFrame(() => { | ||
| if (!isElementConnected(element)) return; |
There was a problem hiding this comment.
Calling handle.open() or handle.toggle() from within an onClick callback causes infinite recursion because activeToolbarEntryId is set after onClick executes
| const didFiberRender = (fiber: FiberNode): boolean => { | ||
| if (!fiber.alternate) return true; | ||
| // Check flags for context-driven and parent-driven updates | ||
| if (fiber.flags && fiber.flags > 0) return true; |
There was a problem hiding this comment.
Overly broad fiber flags check inflates render counts
Medium Severity
The didFiberRender check fiber.flags && fiber.flags > 0 treats any fiber with any non-zero flag as having re-rendered. React fiber flags include Passive (2048, set on any component using useEffect), Ref (512), Snapshot (1024), and many others that don't indicate an actual re-render. Even React DevTools' own didFiberRender was recently fixed (facebook/react#33434) because checking just the PerformedWork flag (value 1) was insufficient — and flags > 0 is far broader than that. This causes the Render Monitor to massively over-count renders, making the tool unreliable.
There was a problem hiding this comment.
16 issues found across 13 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/plugins/toolbar-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/toolbar-plugin.ts:178">
P1: Guard `open()`/`toggle()` so action-only entries do not re-enter their own `onClick` handler.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/toolbar-plugin.ts:186">
P2: Ignore `setIcon`/`setTooltip`/`setBadge`/`setVisible` calls after an entry has been unregistered, or stale handles can resurrect overrides for reused entry IDs.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/menus-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/menus-plugin.ts:531">
P1: Preserve the full frozen selection when entering prompt mode; this collapses multi-select actions down to one element.</violation>
</file>
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1">
P2: Mirror the active toolbar-entry cleanup in `registerPlugin`. Re-registering a plugin with the same name can remove the currently open entry and leave a stale/empty dropdown mounted.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/navigation-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/navigation-plugin.ts:131">
P1: Resolve selection source from the effective selection, not `targetElement`, or keyboard navigation can leave `selectionFilePath` pointing at the previously hovered element.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/navigation-plugin.ts:353">
P2: Ignore bare Shift while prompt mode is open; otherwise typing with Shift in the prompt toggles inspect mode and can unfreeze the current selection.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/prompt-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/prompt-plugin.ts:602">
P1: This reads the current comments dropdown position instead of computing a new anchor, so toggling Comments never opens the dropdown.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/prompt-plugin.ts:617">
P2: This rereads `clearPromptPosition()` after dismissing popups, so the clear-comments confirmation never becomes visible.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/copy-pipeline.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/copy-pipeline.ts:385">
P2: Only show grabbed-box success feedback after the copy has actually succeeded.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/copy-pipeline.ts:389">
P1: Await async `onElementSelect` interceptors even when other elements still fall back to the normal copy path.</violation>
<violation number="3" location="packages/react-grab/src/core/plugins/copy-pipeline.ts:519">
P2: Don't gate copy feedback labels on `theme.grabbedBoxes.enabled`.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/keyboard-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/keyboard-plugin.ts:234">
P2: Pending hold cancellation only works for Ctrl/Meta shortcuts. Custom activation keys like `g` or `Space` keep their timer alive after another key is pressed, so the overlay can still activate unexpectedly.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/keyboard-plugin.ts:434">
P1: Releasing Shift/Alt is ignored for multi-modifier shortcuts, so combos like `Ctrl+Shift+g` stay active until Ctrl/Meta is released.</violation>
</file>
<file name="packages/react-grab/src/core/plugin-registry.ts">
<violation number="1" location="packages/react-grab/src/core/plugin-registry.ts:392">
P1: Wire `contextmenu` into the interceptor dispatcher; handlers registered through this new chain never run.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/pointer-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/pointer-plugin.ts:93">
P2: Clearing the debounced drag pointer on every move makes drag previews vanish during continuous drags.</violation>
<violation number="2" location="packages/react-grab/src/core/plugins/pointer-plugin.ts:105">
P3: These pending-selection locals are never assigned, so the default-action/context-menu path is unreachable dead code.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| open: () => { | ||
| if (activeToolbarEntryId() !== entryId) | ||
| handleToggleToolbarEntry(entryId); | ||
| }, | ||
| close: () => { | ||
| if (activeToolbarEntryId() === entryId) dismissToolbarEntry(); | ||
| }, | ||
| toggle: () => handleToggleToolbarEntry(entryId), |
There was a problem hiding this comment.
P1: Guard open()/toggle() so action-only entries do not re-enter their own onClick handler.
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/plugins/toolbar-plugin.ts, line 178:
<comment>Guard `open()`/`toggle()` so action-only entries do not re-enter their own `onClick` handler.</comment>
<file context>
@@ -0,0 +1,415 @@
+ const getToolbarEntryHandle = (entryId: string): ToolbarEntryHandle => ({
+ api,
+ isOpen: () => activeToolbarEntryId() === entryId,
+ open: () => {
+ if (activeToolbarEntryId() !== entryId)
+ handleToggleToolbarEntry(entryId);
</file context>
| open: () => { | |
| if (activeToolbarEntryId() !== entryId) | |
| handleToggleToolbarEntry(entryId); | |
| }, | |
| close: () => { | |
| if (activeToolbarEntryId() === entryId) dismissToolbarEntry(); | |
| }, | |
| toggle: () => handleToggleToolbarEntry(entryId), | |
| open: () => { | |
| const toolbarEntry = registry.store.toolbarEntries.find( | |
| (toolbarEntry) => toolbarEntry.id === entryId, | |
| ); | |
| if (toolbarEntry?.onRender && activeToolbarEntryId() !== entryId) { | |
| handleToggleToolbarEntry(entryId); | |
| } | |
| }, | |
| close: () => { | |
| if (activeToolbarEntryId() === entryId) dismissToolbarEntry(); | |
| }, | |
| toggle: () => { | |
| const toolbarEntry = registry.store.toolbarEntries.find( | |
| (toolbarEntry) => toolbarEntry.id === entryId, | |
| ); | |
| if (toolbarEntry?.onRender) { | |
| handleToggleToolbarEntry(entryId); | |
| } | |
| }, |
| onBeforePrompt?.(); | ||
| shared.preparePromptMode?.(position, element); | ||
| actions.setPointer({ x: position.x, y: position.y }); | ||
| actions.setFrozenElement(element); |
There was a problem hiding this comment.
P1: Preserve the full frozen selection when entering prompt mode; this collapses multi-select actions down to one element.
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/plugins/menus-plugin.ts, line 531:
<comment>Preserve the full frozen selection when entering prompt mode; this collapses multi-select actions down to one element.</comment>
<file context>
@@ -0,0 +1,660 @@
+ onBeforePrompt?.();
+ shared.preparePromptMode?.(position, element);
+ actions.setPointer({ x: position.x, y: position.y });
+ actions.setFrozenElement(element);
+ shared.activatePromptMode?.();
+ if (!isActivated()) {
</file context>
|
|
||
| createEffect( | ||
| on( | ||
| () => targetElement(), |
There was a problem hiding this comment.
P1: Resolve selection source from the effective selection, not targetElement, or keyboard navigation can leave selectionFilePath pointing at the previously hovered element.
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/plugins/navigation-plugin.ts, line 131:
<comment>Resolve selection source from the effective selection, not `targetElement`, or keyboard navigation can leave `selectionFilePath` pointing at the previously hovered element.</comment>
<file context>
@@ -0,0 +1,386 @@
+
+ createEffect(
+ on(
+ () => targetElement(),
+ (element) => {
+ const currentVersion = ++selectionSourceRequestVersion;
</file context>
| } | ||
| } | ||
| await waitUntilNextFrame(); | ||
| if (unhandledElements.length > 0) { |
There was a problem hiding this comment.
P1: Await async onElementSelect interceptors even when other elements still fall back to the normal copy path.
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/plugins/copy-pipeline.ts, line 389:
<comment>Await async `onElementSelect` interceptors even when other elements still fall back to the normal copy path.</comment>
<file context>
@@ -0,0 +1,774 @@
+ }
+ }
+ await waitUntilNextFrame();
+ if (unhandledElements.length > 0) {
+ await copyWithFallback(
+ unhandledElements,
</file context>
| pendingResults.push(pendingResult); | ||
| } | ||
| if (registry.store.theme.grabbedBoxes.enabled) { | ||
| showTemporaryGrabbedBox(createElementBounds(element), element); |
There was a problem hiding this comment.
P2: Only show grabbed-box success feedback after the copy has actually succeeded.
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/plugins/copy-pipeline.ts, line 385:
<comment>Only show grabbed-box success feedback after the copy has actually succeeded.</comment>
<file context>
@@ -0,0 +1,774 @@
+ pendingResults.push(pendingResult);
+ }
+ if (registry.store.theme.grabbedBoxes.enabled) {
+ showTemporaryGrabbedBox(createElementBounds(element), element);
+ }
+ }
</file context>
|
|
||
| const computedLabelInstances = createMemo(() => { | ||
| if (!registry.store.theme.enabled) return []; | ||
| if (!registry.store.theme.grabbedBoxes.enabled) return []; |
There was a problem hiding this comment.
P2: Don't gate copy feedback labels on theme.grabbedBoxes.enabled.
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/plugins/copy-pipeline.ts, line 519:
<comment>Don't gate copy feedback labels on `theme.grabbedBoxes.enabled`.</comment>
<file context>
@@ -0,0 +1,774 @@
+
+ const computedLabelInstances = createMemo(() => {
+ if (!registry.store.theme.enabled) return [];
+ if (!registry.store.theme.grabbedBoxes.enabled) return [];
+ void store.viewportVersion;
+ const currentIds = new Set(
</file context>
|
|
||
| if (!isTargetKeyCombination(event, registry.store.options)) { | ||
| if ( | ||
| (event.metaKey || event.ctrlKey) && |
There was a problem hiding this comment.
P2: Pending hold cancellation only works for Ctrl/Meta shortcuts. Custom activation keys like g or Space keep their timer alive after another key is pressed, so the overlay can still activate unexpectedly.
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/plugins/keyboard-plugin.ts, line 234:
<comment>Pending hold cancellation only works for Ctrl/Meta shortcuts. Custom activation keys like `g` or `Space` keep their timer alive after another key is pressed, so the overlay can still activate unexpectedly.</comment>
<file context>
@@ -0,0 +1,593 @@
+
+ if (!isTargetKeyCombination(event, registry.store.options)) {
+ if (
+ (event.metaKey || event.ctrlKey) &&
+ !MODIFIER_KEYS.includes(event.key) &&
+ !isEnterCode(event.code)
</file context>
| if (dragPreviewDebounceTimerId !== null) { | ||
| clearTimeout(dragPreviewDebounceTimerId); | ||
| } | ||
| setDebouncedDragPointer(null); |
There was a problem hiding this comment.
P2: Clearing the debounced drag pointer on every move makes drag previews vanish during continuous drags.
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/plugins/pointer-plugin.ts, line 93:
<comment>Clearing the debounced drag pointer on every move makes drag previews vanish during continuous drags.</comment>
<file context>
@@ -0,0 +1,696 @@
+ if (dragPreviewDebounceTimerId !== null) {
+ clearTimeout(dragPreviewDebounceTimerId);
+ }
+ setDebouncedDragPointer(null);
+ dragPreviewDebounceTimerId = window.setTimeout(() => {
+ setDebouncedDragPointer({ x: clientX, y: clientY });
</file context>
| () => isDragging(), | ||
| ); | ||
|
|
||
| let isPendingContextMenuSelect = false; |
There was a problem hiding this comment.
P3: These pending-selection locals are never assigned, so the default-action/context-menu path is unreachable dead code.
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/plugins/pointer-plugin.ts, line 105:
<comment>These pending-selection locals are never assigned, so the default-action/context-menu path is unreachable dead code.</comment>
<file context>
@@ -0,0 +1,696 @@
+ () => isDragging(),
+ );
+
+ let isPendingContextMenuSelect = false;
+ let pendingDefaultActionId: string | null = null;
+
</file context>
- Extract duplicate label fade logic from copy-pipeline and menus plugins into shared utils/label-fade-manager.ts - Remove dead _keyboardSelectedElement variable from navigation-plugin - Hoist duplicate isFromOverlay computation in keyboard-plugin - Strip excessive non-HACK comments from keyboard-plugin - Rename dragBoundsVal to firstFrozenBounds in pointer-plugin - Extract plugin priority magic numbers to constants.ts
There was a problem hiding this comment.
1 issue found across 9 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/utils/label-fade-manager.ts">
<violation number="1" location="packages/react-grab/src/utils/label-fade-manager.ts:40">
P1: Track the fade-completion timeout too; otherwise `cancel()`/`schedule()` cannot stop a pending removal after the label starts fading.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const timeoutId = window.setTimeout(() => { | ||
| timeouts.delete(instanceId); | ||
| actions.updateLabelInstance(instanceId, "fading"); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
P1: Track the fade-completion timeout too; otherwise cancel()/schedule() cannot stop a pending removal after the label starts fading.
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/label-fade-manager.ts, line 40:
<comment>Track the fade-completion timeout too; otherwise `cancel()`/`schedule()` cannot stop a pending removal after the label starts fading.</comment>
<file context>
@@ -0,0 +1,50 @@
+ const timeoutId = window.setTimeout(() => {
+ timeouts.delete(instanceId);
+ actions.updateLabelInstance(instanceId, "fading");
+ setTimeout(() => {
+ timeouts.delete(instanceId);
+ actions.removeLabelInstance(instanceId);
</file context>
The label-fade-manager extraction accidentally removed this import that is still used in 3 other places in the file.
- Close active toolbar entry before unregistering plugin (prevents stale state) - Add getPluginToolbarEntryIds to plugin registry for entry ownership lookup - Dismiss current entry before opening new one in toolbar toggle (mutual exclusion) - Make dispose cleanup unconditional in gym toolbar-entries-provider
| const didFiberRender = (fiber: FiberNode): boolean => { | ||
| if (!fiber.alternate) return true; | ||
| // Check flags for context-driven and parent-driven updates | ||
| if (fiber.flags && fiber.flags > 0) return true; |
There was a problem hiding this comment.
Render monitor overcounts due to overly broad flags check
Medium Severity
didFiberRender uses fiber.flags > 0 to detect re-renders, but this matches nearly every fiber in a committed tree — not just those that actually re-rendered. React fiber flags include Placement (2), Update (4), Passive (2048), Ref (512), etc. The flag that indicates a component actually executed its render function is PerformedWork (value 1), which requires a bitwise check like fiber.flags & 1. With > 0, the render monitor will massively overcount renders, making the devtool's data misleading.
There was a problem hiding this comment.
2 issues found across 9 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">
P2: This closes the active toolbar entry by plugin-local ID match, which breaks when multiple plugins contribute the same entry ID.</violation>
</file>
<file name="packages/gym/components/toolbar-entries-provider.tsx">
<violation number="1" location="packages/gym/components/toolbar-entries-provider.tsx:148">
P2: Keep the render-monitor cleanup conditional. The new direct `dispose: stopMonitoring` can reset `onCommitFiberRoot` during unmount even when this entry is no longer active, clobbering a newer hook wrapper.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| id: "render-monitor", | ||
| icon: ICON_RENDER, | ||
| tooltip: "Render Monitor", | ||
| dispose: stopMonitoring, |
There was a problem hiding this comment.
P2: Keep the render-monitor cleanup conditional. The new direct dispose: stopMonitoring can reset onCommitFiberRoot during unmount even when this entry is no longer active, clobbering a newer hook wrapper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/gym/components/toolbar-entries-provider.tsx, line 148:
<comment>Keep the render-monitor cleanup conditional. The new direct `dispose: stopMonitoring` can reset `onCommitFiberRoot` during unmount even when this entry is no longer active, clobbering a newer hook wrapper.</comment>
<file context>
@@ -131,9 +145,7 @@ const createRenderMonitorEntry = (): ToolbarEntry & { dispose: () => void } => {
- dispose: () => {
- if (isMonitoring) stopMonitoring();
- },
+ dispose: stopMonitoring,
onClick: (handle) => {
if (isMonitoring) {
</file context>
| dispose: stopMonitoring, | |
| dispose: () => { | |
| if (isMonitoring) stopMonitoring(); | |
| }, |
…ration - Add missing contextmenu window event listener to interceptor chain - Fix SolidJS signal tracking for isToolbarSelectHovered and hasDragPreviewBounds by hoisting signals to index.tsx (memos defined before plugin setup couldn't track signals created during setup) - Add isSelectionSuppressed check to selectionLabelVisible in navigation-plugin (toolbar hover should hide label in non-frozen mode) - Use deferHideContextMenu for onContextMenuHide renderer prop - Wire openTrackedDropdown/stopTrackingDropdownPosition via shared API for comments dropdown and clear prompt positioning - Expose getDragBounds and isDragBoxVisible via shared for getState()
… logic - Fix contextmenu handler to continue processing after clearing arrow navigation (matching main's behavior instead of returning early) - Fix openCommentsDropdown/showClearPrompt to use targeted popup dismissals instead of dismissAllPopups, which was resetting isCommentsHoverOpen and breaking pin-on-click - Increase e2e test timeouts and replace hardcoded waits with polling to reduce flakiness
| } | ||
| | undefined; | ||
|
|
||
| if (!hook) return; |
There was a problem hiding this comment.
Render monitor silently fails with stale active state
Medium Severity
startMonitoring sets isMonitoring = true before checking whether __REACT_DEVTOOLS_GLOBAL_HOOK__ exists. If the hook is absent, the function returns early but isMonitoring remains true. The toolbar button icon turns red (appears active), but no renders are ever counted. Subsequent calls to startMonitoring also bail out early because isMonitoring is already true. The flag needs to be reset to false when the hook isn't found.
| fiber.memoizedProps !== fiber.alternate.memoizedProps || | ||
| fiber.memoizedState !== fiber.alternate.memoizedState | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Fiber flags check overcounts renders significantly
Medium Severity
didFiberRender treats any non-zero fiber.flags as evidence of a render. React fiber flags include many non-render-related bits like Placement, ChildDeletion, Snapshot, Passive, etc. This causes the render monitor to massively overcount by reporting fibers that were merely placed, had children deleted, or had passive effects scheduled — not just those that actually re-rendered.
There was a problem hiding this comment.
3 issues found across 10 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/e2e/freeze-updates.spec.ts">
<violation number="1" location="packages/react-grab/e2e/freeze-updates.spec.ts:34">
P2: Replace the new fixed 500ms sleep with a state-based wait for prompt mode to exit.</violation>
</file>
<file name="packages/react-grab/src/core/plugins/pointer-plugin.ts">
<violation number="1" location="packages/react-grab/src/core/plugins/pointer-plugin.ts:606">
P2: Returning to the normal context-menu flow after clearing arrow navigation lets overlay right-clicks act on the underlying page element.</violation>
</file>
<file name="packages/react-grab/e2e/edge-cases.spec.ts">
<violation number="1" location="packages/react-grab/e2e/edge-cases.spec.ts:49">
P2: Replace this fixed sleep with a deterministic wait on the removed element or overlay state; otherwise this test will stay flaky in slower CI runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| await reactGrab.pressEscape(); | ||
| await reactGrab.page.waitForTimeout(200); | ||
| await reactGrab.page.waitForTimeout(500); |
There was a problem hiding this comment.
P2: Replace the new fixed 500ms sleep with a state-based wait for prompt mode to exit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/e2e/freeze-updates.spec.ts, line 34:
<comment>Replace the new fixed 500ms sleep with a state-based wait for prompt mode to exit.</comment>
<file context>
@@ -25,13 +25,13 @@ test.describe("Freeze Updates", () => {
await reactGrab.pressEscape();
- await reactGrab.page.waitForTimeout(200);
+ await reactGrab.page.waitForTimeout(500);
const countAfterExit = await getElementCount();
</file context>
| await reactGrab.page.waitForTimeout(500); | |
| await expect.poll(() => reactGrab.isPromptModeActive()).toBe(false); |
| event, | ||
| "data-react-grab-ignore-events", | ||
| ); | ||
| if (isFromOverlay && ctx.shared.hasArrowNavigation?.()) { |
There was a problem hiding this comment.
P2: Returning to the normal context-menu flow after clearing arrow navigation lets overlay right-clicks act on the underlying page element.
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/plugins/pointer-plugin.ts, line 606:
<comment>Returning to the normal context-menu flow after clearing arrow navigation lets overlay right-clicks act on the underlying page element.</comment>
<file context>
@@ -603,8 +603,9 @@ export const pointerPlugin: InternalPlugin = {
"data-react-grab-ignore-events",
);
- if (isFromOverlay) {
+ if (isFromOverlay && ctx.shared.hasArrowNavigation?.()) {
ctx.shared.clearArrowNavigation?.();
+ } else if (isFromOverlay) {
</file context>
| await reactGrab.waitForSelectionBox(); | ||
|
|
||
| await reactGrab.removeElement("[data-testid='toggleable-element']"); | ||
| await reactGrab.page.waitForTimeout(100); |
There was a problem hiding this comment.
P2: Replace this fixed sleep with a deterministic wait on the removed element or overlay state; otherwise this test will stay flaky in slower CI runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/e2e/edge-cases.spec.ts, line 49:
<comment>Replace this fixed sleep with a deterministic wait on the removed element or overlay state; otherwise this test will stay flaky in slower CI runs.</comment>
<file context>
@@ -46,6 +46,7 @@ test.describe("Edge Cases", () => {
await reactGrab.waitForSelectionBox();
await reactGrab.removeElement("[data-testid='toggleable-element']");
+ await reactGrab.page.waitForTimeout(100);
await reactGrab.hoverElement("li:first-child");
</file context>
| await reactGrab.page.waitForTimeout(100); | |
| await reactGrab.page.waitForFunction( | |
| () => !document.querySelector("[data-testid='toggleable-element']"), | |
| ); |
- Remove unnecessary comment in didFiberRender - Prefix blocking comment with HACK: per convention - Convert page component to arrow function - Rename `previous` to `previousCount` for clarity
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.
| if (currentFps > 0) { | ||
| minFps = Math.min(minFps, currentFps); | ||
| maxFps = Math.max(maxFps, currentFps); | ||
| } |
There was a problem hiding this comment.
FPS monitor min always reports 1
Low Severity
The minFps stat always displays 1 because minFps is updated starting from the very first measurement frame, when only a single timestamp exists in the 1-second sliding window. On the second frame currentFps is 1, so minFps immediately locks to 1 and never recovers, making the "Min" display in the FPS Monitor dropdown permanently meaningless.
1 participant
Summary
toolbarEntriesto the plugin system, allowing plugins to register persistent buttons in the toolbar strip with emoji, SVG, or HTML iconsonClickwithout dropdown) and dropdown-based (onRenderwith a framework-agnostic container)ToolbarEntryHandleper entry for dropdown control (open/close/toggle), display updates (setIcon/setBadge/setTooltip/setVisible), and fullReactGrabAPIaccessAGENTS.md compliance fixes applied
createEffectevent-bus pattern with direct cleanup inregisterPlugin/unregisterPlugincall sitesmountCleanupvariable tracking with idiomaticonCleanup()inside the effectactiveToolbarEntryHandle)entryloop/callback variables totoolbarEntryfor descriptive namingTest plan
pnpm build— no type errorspnpm typecheck— passespnpm lint— 0 warnings, 0 errors/dashboardshows 3 toolbar entry buttons/toolbar-entriesshows documentation pagehandle.setBadge(n)shows badge on buttonNote
Medium Risk
Introduces a new plugin-driven toolbar surface (buttons + dropdown rendering) that touches core overlay/toolbar rendering and dismissal logic, which could affect UX interactions. Test updates reduce flakiness but also indicate timing-sensitive behavior around overlays/freeze mode.
Overview
Adds a plugin toolbar entries API so plugins can register persistent toolbar buttons with optional badges/visibility overrides and either an
onClickaction or anonRenderdropdown panel (rendered into a raw DOM container via the newToolbarEntryContainer).Updates the core renderer/toolbar to display these entries, track the active entry/dropdown state, and suppress tooltips while a plugin dropdown is open; also exports supporting types and adds plugin priority constants.
Extends the Gym with a
ToolbarEntriesProvider(Render Monitor + FPS Monitor) and a new/toolbar-entriesperformance test page, and refreshes READMEs to document plugin toolbar entries.Includes minor CLI/test formatting cleanups and several e2e de-flakes (longer waits/polling) around element removal, focus trap prompt submission, and freeze/update behavior.
Written by Cursor Bugbot for commit 3ab872f. This will update automatically on new commits. Configure here.
Summary by cubic
Adds a plugin-driven toolbar entries API to
react-grabso plugins can add persistent toolbar buttons with optional dropdowns and live updates. Refactors the core into 7 focused internal plugins and adds Render Monitor + FPS Monitor demos in the Gym.New Features
toolbarEntriesfor plugin buttons (emoji/SVG/HTML, badges, visibility) with action (onClick) or dropdown (onRender) and per-entryToolbarEntryHandle.toggleToolbarEntry(entryId),closeToolbarEntry(). ExportToolbarEntryandToolbarEntryHandle.ToolbarEntriesProvider; new/toolbar-entriespage.Bug Fixes
toolbarEntryOverridesand cleanup via Solidreconcile;setBadge(undefined)clears badges.contextmenulistener; hoist signals to fix toolbar hover and drag preview tracking; hide selection label on toolbar hover; anchor dropdowns for comments/clear prompt; exposegetDragBounds/isDragBoxVisibleviagetState().Written for commit 3ab872f. Summary will update on new commits.