Cycles #144
Cycles #144
Conversation
|
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/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
commit: |
- Improved the structure of the action cycle context by integrating the `handleActionCycleCopy` and `handleActionCyclePrompt` functions for better clarity and maintainability. - Updated the context-building logic to ensure consistent retrieval of element properties and streamlined the action context creation process. - Adjusted the handling of frozen elements and selection bounds to improve functionality and user experience.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium Priority - The action cycling feature is well-architected but has several issues that could cause bugs in edge cases. Most critical: the global wheel event handler prevents default behavior unconditionally when action cycle is active, which will break normal page scrolling. Also found potential race conditions in timeout cleanup and missing error handling in async action execution.
| const handleActionCycleWheel = (event: WheelEvent) => { | ||
| const isActionCycleActive = actionCycleActiveIndex() !== null; | ||
| if (!isActionCycleActive) return; | ||
| if (!canCycleActions()) return; | ||
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| actionCycleScrollCycler.handleWheel(event); | ||
| }; |
There was a problem hiding this comment.
The wheel event handler prevents default unconditionally when action cycle is active (line 2222), which will break normal page scrolling anywhere on the page. This should only preventDefault if the wheel event is actually over the selection label overlay, not globally. Consider checking if the event target is within the action cycle UI before calling preventDefault.
| actionCycleIdleTimeoutId = window.setTimeout(() => { | ||
| actionCycleIdleTimeoutId = null; | ||
| const activeIndex = actionCycleActiveIndex(); | ||
| const items = actionCycleItems(); | ||
| if (activeIndex === null || items.length === 0) return; | ||
| const selectedItem = items[activeIndex]; | ||
| if (!selectedItem) return; | ||
| const action = getActionById(selectedItem.id); | ||
| if (!action) { | ||
| resetActionCycle(); | ||
| return; | ||
| } | ||
| const context = getActionCycleContext(); | ||
| if (!context || !resolveActionEnabled(action, context)) { | ||
| resetActionCycle(); | ||
| return; | ||
| } | ||
| resetActionCycle(); | ||
| const result = action.onAction(context); | ||
| if (result instanceof Promise) { | ||
| void result; | ||
| } | ||
| }, ACTION_CYCLE_IDLE_TRIGGER_MS); |
There was a problem hiding this comment.
Potential race condition: If the action's onAction throws synchronously or returns a rejected promise, the voided promise on line 2157 will silently swallow the error. This makes debugging very difficult. Recommend adding proper error handling:
const result = action.onAction(context);
if (result instanceof Promise) {
result.catch((error) => {
console.error('Action cycle action failed:', error);
// Optionally show error feedback to user
});
}| const actionCycleScrollCycler = createScrollCycler({ | ||
| thresholdPx: ACTION_CYCLE_SCROLL_THRESHOLD_PX, | ||
| throttleMs: ACTION_CYCLE_INPUT_THROTTLE_MS, | ||
| lineHeightPx: ACTION_CYCLE_SCROLL_LINE_HEIGHT_PX, | ||
| onStep: handleActionCycleInput, | ||
| }); |
There was a problem hiding this comment.
actionCycleScrollCycler is created once at module initialization but never cleaned up. While it doesn't hold external resources, for consistency with other cleanup, consider resetting its internal state in onCleanup or when action cycle is reset.
| const applyActionCycleItems = ( | ||
| cycleItems: ActionCycleItem[], | ||
| direction: "forward" | "backward", | ||
| ): boolean => { | ||
| if (cycleItems.length === 0) return false; | ||
| setActionCycleItems(cycleItems); | ||
|
|
||
| const currentIndex = actionCycleActiveIndex(); | ||
| const isCurrentIndexValid = | ||
| currentIndex !== null && currentIndex < cycleItems.length; | ||
| const stepOffset = direction === "forward" ? 1 : -1; | ||
|
|
||
| let nextIndex: number; | ||
| if (!isCurrentIndexValid) { | ||
| nextIndex = direction === "forward" ? 0 : cycleItems.length - 1; | ||
| } else { | ||
| nextIndex = | ||
| (currentIndex + stepOffset + cycleItems.length) % cycleItems.length; | ||
| } | ||
|
|
||
| setActionCycleActiveIndex(nextIndex); | ||
| scheduleActionCycleActivation(); | ||
| return true; |
There was a problem hiding this comment.
Edge case bug: If cycleItems changes between when this function is called and when setActionCycleActiveIndex is called (e.g., due to an action being dynamically disabled), nextIndex could be out of bounds. Consider validating nextIndex < cycleItems.length before calling setActionCycleActiveIndex.
| export const isElementConnected = ( | ||
| element: Element | null | undefined, | ||
| ): element is Element => | ||
| Boolean(element?.isConnected ?? element?.ownerDocument?.contains(element)); |
There was a problem hiding this comment.
The fallback to ownerDocument.contains is problematic. If isConnected is false, checking ownerDocument.contains could give a different result (e.g., if element is in a detached subtree). Recommend simplifying to just trust isConnected:
export const isElementConnected = (
element: Element | null | undefined,
): element is Element =>
element?.isConnected ?? false;The ownerDocument.contains fallback is only needed for very old browsers that don't support isConnected, which is unlikely in this codebase.
| const now = Date.now(); | ||
| if ( | ||
| now - lastStepTimestamp < throttleMs || | ||
| accumulatedDelta < thresholdPx | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The throttle check uses Date.now() which can be affected by system clock changes. For timing-sensitive UX like scroll handling, consider using performance.now() instead for monotonic time.
| const getActionCycleContext = (): ContextMenuActionContext | undefined => { | ||
| const element = selectionElement(); | ||
| if (!element) return undefined; | ||
|
|
||
| const fallbackBounds = selectionBounds(); | ||
|
|
||
| return buildActionContext({ | ||
| element, | ||
| filePath: store.selectionFilePath ?? undefined, | ||
| lineNumber: store.selectionLineNumber ?? undefined, | ||
| tagName: getTagName(element) || undefined, | ||
| componentName: selectionComponentName(), | ||
| position: store.pointer, | ||
| performWithFeedbackOptions: { | ||
| fallbackBounds, | ||
| fallbackSelectionBounds: fallbackBounds ? [fallbackBounds] : [], | ||
| }, | ||
| shouldDeferHideContextMenu: false, | ||
| onBeforePrompt: resetActionCycle, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
If selectionElement() becomes null between when availableActionCycleItems is computed and when this context is used (e.g., element removed from DOM), this will return undefined. The calling code at line 2150 checks for this, but there's still a TOCTOU (time-of-check-time-of-use) gap. Consider caching the element reference at the start of the activation flow.
| if (actionCycleIdleTimeoutId) { | ||
| window.clearTimeout(actionCycleIdleTimeoutId); | ||
| } |
There was a problem hiding this comment.
Good cleanup! However, consider also clearing the action cycle state here (calling resetActionCycle()) to ensure no orphaned state remains after cleanup.
| <BottomSection> | ||
| <div class="flex flex-col w-[calc(100%+16px)] -mx-2 -my-1.5"> | ||
| <For each={actionCycleItems()}> | ||
| {(item, itemIndex) => ( | ||
| <div | ||
| data-react-grab-action-cycle-item={item.label.toLowerCase()} | ||
| class="contain-layout flex items-center justify-between w-full px-2 py-1 transition-colors" | ||
| classList={{ | ||
| "bg-black/5": | ||
| itemIndex() === actionCycleActiveIndex(), | ||
| "rounded-b-[6px]": | ||
| itemIndex() === actionCycleItems().length - 1, | ||
| }} | ||
| > | ||
| <span class="text-[13px] leading-4 font-sans font-medium text-black"> | ||
| {item.label} | ||
| </span> | ||
| <Show when={item.shortcut}> | ||
| <span class="text-[11px] font-sans text-black/50 ml-4"> | ||
| {formatShortcut(item.shortcut!)} | ||
| </span> | ||
| </Show> | ||
| </div> | ||
| )} | ||
| </For> | ||
| </div> | ||
| </BottomSection> | ||
| </Show> |
There was a problem hiding this comment.
The action cycle UI is rendered using For which creates/destroys elements on each cycle. If items are reordered frequently (e.g., actions being enabled/disabled), this could cause flashing. Consider using Index instead of For if items have stable order, or add a key function based on item.id.
| const handleActionCycleKey = (event: KeyboardEvent): boolean => { | ||
| if (event.code !== "KeyC") return false; | ||
| if (event.altKey || event.repeat) return false; | ||
| if (isKeyboardEventTriggeredByInput(event)) return false; | ||
| if (!handleActionCycleInput("forward")) return false; | ||
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (event.metaKey || event.ctrlKey) { | ||
| event.stopImmediatePropagation(); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
The KeyC handler doesn't check if event.metaKey || event.ctrlKey before the main logic, but then conditionally calls stopImmediatePropagation based on those modifiers (lines 2204-2206). This creates inconsistent behavior - either KeyC should always require a modifier, or the stopImmediatePropagation logic should be unconditional. Currently, pressing 'C' alone will cycle actions but won't stop event propagation, which could trigger other 'C' key handlers.
![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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| if (actionCycleIdleTimeoutId) { | ||
| window.clearTimeout(actionCycleIdleTimeoutId); | ||
| } |
There was a problem hiding this comment.
| if (actionCycleIdleTimeoutId) { | |
| window.clearTimeout(actionCycleIdleTimeoutId); | |
| } | |
| resetActionCycle(); |
Cleanup handler only clears actionCycleIdleTimeoutId but doesn't reset actionCycleItems and actionCycleActiveIndex signals, violating the cleanup invariant
|
|
||
| accumulatedDelta += Math.abs(normalizedDelta); | ||
|
|
||
| const now = Date.now(); |
There was a problem hiding this comment.
| const now = Date.now(); | |
| const now = performance.now(); |
Using Date.now() for scroll throttling timing can break if system clock adjusts backwards, causing incorrect throttle behavior
| resetActionCycle(); | ||
| const result = action.onAction(context); | ||
| if (result instanceof Promise) { | ||
| void result; |
There was a problem hiding this comment.
| void result; | |
| result.catch((error) => { console.error("[react-grab] Action error:", error); }); |
Promise rejection from action.onAction() is silently swallowed with void operator, preventing error visibility and making debugging difficult
| if (isKeyboardEventTriggeredByInput(event)) return false; | ||
| if (!handleActionCycleInput("forward")) return false; | ||
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (event.metaKey || event.ctrlKey) { | ||
| event.stopImmediatePropagation(); | ||
| } |
There was a problem hiding this comment.
| if (isKeyboardEventTriggeredByInput(event)) return false; | |
| if (!handleActionCycleInput("forward")) return false; | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| if (event.metaKey || event.ctrlKey) { | |
| event.stopImmediatePropagation(); | |
| } | |
| if (event.metaKey || event.ctrlKey) return false; | |
| if (isKeyboardEventTriggeredByInput(event)) return false; | |
| if (!handleActionCycleInput("forward")) return false; | |
| event.preventDefault(); | |
| event.stopPropagation(); |
The handleActionCycleKey function intercepts Cmd+C (Mac) and Ctrl+C (Windows/Linux) shortcuts, preventing normal copy operations from working
Summary by cubic
Add action cycling to the selection label so you can quickly switch actions with the mouse wheel or the C key and auto-run the chosen action after a short idle.
New Features
Refactors
Written for commit 32df084. Summary will update on new commits.
Note
Medium Risk
Touches core input/event handling and action execution timing (new global
wheel/KeyCbehavior and auto-trigger), which could cause UX regressions or unexpected action runs if edge cases slip through.Overview
Adds an action cycling UX on the selection label: users can press
Cor use the mouse wheel to cycle through a fixed set of actions (e.g. copy/comment/screenshot/open), see the active choice inline, and have the selected action auto-run after a short idle.Refactors action plumbing by extracting
resolveActionEnabled, centralizing action-context construction (includingperformWithFeedbackfallbacks), and tightening overlay event handling so the activation shortcut no longer cancels prompt/input mode when the React Grab textarea is focused. Improves DOM liveness checks by switching manydocument.containscalls toisElementConnected, and consolidates cache invalidation intoclearAllCaches.Written by Cursor Bugbot for commit 32df084. This will update automatically on new commits. Configure here.