refactor: decompose core/index.tsx, dedupe CLI knowledge, clean up E2E fixture casts#391
Open
aidenybai wants to merge 87 commits into
Open
refactor: decompose core/index.tsx, dedupe CLI knowledge, clean up E2E fixture casts#391aidenybai wants to merge 87 commits into
aidenybai wants to merge 87 commits into
Conversation
…dBox - Drop `picomatch` from root devDependencies (PR #377 intended to remove it but missed package.json). - Fold 8 duplicate `Icon{Check,Chevron,Command,Ellipsis,Loader,Retry,Return,Submit}Props` into a single shared `IconProps` in icons/types.ts. `IconSelectProps` extends it with `rotationDeg`. - Replace 3 inline `{ id; bounds; createdAt }` shapes (in `ReactGrabState`, `ReactGrabRendererProps`, and `OverlayCanvasProps`) with a new `PublicGrabbedBox = Pick<GrabbedBox, ...>` so the public shape has one canonical name. - `!!process.env.CI` -> `Boolean(...)` in playwright.config.ts per AGENTS.md V8 rules. No behavior change. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the 12 pure phase-derivation memos (isHoldingKeys, isActivated, isFrozenPhase, isDragging, isActivelyDragging, isDragRepositioning, didJustDrag, isCopying, isSelectionInteractionLocked, didJustCopy, isPromptMode, isCommentMode, isPendingDismiss) out of init() into a focused selectors.ts module. No behavior change. Part of PLAN §3.1.a (decompose core/index.tsx). index.tsx -29 lines. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the next batch of pure derivations out of init() into a new `createGrabElementSelectors` factory: - isRendererActive - targetElement - effectiveElement - selectionElement + isSelectionElementVisible (handles touch-mode + drag fallback) - frozenElementBoundsAccessors - frozenElementsBounds - selectionBounds These selectors depend on the phase selectors plus the store, with no local-signal coupling, so they're a clean extraction. The previous local-let-rec patterns (selectionElement reading effectiveElement, selectionBounds reading frozenElementsBounds, etc.) are preserved through composition inside the factory. index.tsx: 3724 -> 3623 lines (-101). No behavior change. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the toolbar dropdown-anchor tracking + dismiss out of init() into a focused controller. The rAF loop, position signal, and dispose path are all owned by the controller; init() composes it with toolbarElement (the late-bound HTMLDivElement ref) via a thin getToolbarElement closure. Drops 5 inline helpers (stopTrackingDropdownPosition, computeDropdownAnchor, openTrackedDropdown, the in-init setToolbarMenuPosition wiring, the dropdownTrackingFrameId closure var) from index.tsx. Also drops 2 now-unused imports (DropdownAnchor type, getNearestEdge util). index.tsx: 3622 -> 3579 lines (-43). No behavior change. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Drop the duplicated `hasReactGrabCode` regex array (8 patterns) and 6 near-identical `findX(projectRoot)` candidate-path helpers from transform.ts/detect.ts into a single canonical `framework-paths.ts`: - hasReactGrabCode + hasReactGrabCodeInFile - findLayoutFile, findDocumentFile, findInstrumentationFile, findIndexHtml, findEntryFile, findTanStackRootFile (now thin wrappers over a single firstExistingPath helper) - findFrameworkEntry: typed dispatch for the per-framework primary file - REACT_GRAB_DETECTION_PATHS: single source of truth for the set of files where an installation might live detect.ts no longer carries its own (subtly different - missing `href` pattern) copy of hasReactGrabCode. detectReactGrab now uses the canonical detection-paths table. transform.ts: 818 -> 708 lines (-110). detect.ts: 431 -> 395 lines (-36). +147 lines in the new framework-paths.ts module. Net: -1 line of code, but the project-layout knowledge now lives in exactly one place. All 124 CLI tests still green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Both `init` and `configure` collected the same 4 ReactGrabOptions
(activationMode, keyHoldDuration, allowActivationInsideInput,
maxContextLines) through identical `await prompts({ ... })` blocks,
each followed by the same `if (x === undefined) process.exit(1)`
cancellation guard. Move the per-option prompts into one shared module:
- promptActivationMode()
- promptKeyHoldDuration()
- promptAllowActivationInsideInput()
- promptMaxContextLines()
Each helper handles its own cancellation. Callsites collapse from
~15-line blocks to single lines:
collectedOptions.keyHoldDuration = await promptKeyHoldDuration();
Note: the activationKey prompt UI differs between the two commands
(text input in init vs autocomplete key-combo picker in configure), so
it stays per-command for now.
init.ts: 565 -> 513 lines (-52)
configure.ts: 593 -> 540 lines (-53)
+72 lines in the new option-prompts.ts module.
All 124 CLI tests still green.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the label/grabbed-box state machine out of init() into a focused manager (PLAN §3.1.b). The manager owns the two timer maps (labelFadeTimeouts, grabbedBoxTimeouts) and exposes a 6-method surface: - showTemporaryGrabbedBox - createLabelInstance - createPerElementLabelInstances - clearAllLabels - updateLabelAfterCopy - handleLabelInstanceHoverChange - dispose The 'compute mouseX offsets' algorithm that was hand-rolled in both createLabelInstance and createPerElementLabelInstances is now a single computeMouseXOffsets() helper shared by both. The dispose path now goes through labelManager.dispose() instead of three separate timer-cleanup calls inside the api.dispose closure. Also drops 3 now-unused imports (FADE_COMPLETE_BUFFER_MS, GrabbedBox type, generateId). index.tsx: 3579 -> 3423 lines (-156). Behaviour preserved: copy-feedback.spec.ts + context-menu.spec.ts (57 tests) green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Drop 32+ inline `(window as { __REACT_GRAB__?: { ... } }).__REACT_GRAB__`
casts (and the matching `__CALLBACK_HISTORY__` / `initReactGrab` casts)
spread throughout the page object. Each cast hand-rolled a narrow subset
of the API type at every callsite, with no single source of truth and a
real type bug latent in `getState` where the original cast lied about
the return type.
Replace with a single `declare global` block at the top of fixtures.ts:
declare global {
interface Window {
__REACT_GRAB__?: ReactGrabAPI;
__CALLBACK_HISTORY__?: CallbackHistoryEntry[];
initReactGrab?: (options?: Record<string, unknown>) => void;
}
}
Each `page.evaluate` now reads `window.__REACT_GRAB__` directly with
full typing. The `getState` helper now explicitly transforms the
internal API state into the fixture's serialization-safe shape (the
previous cast hid the `targetElement: Element | null` -> `boolean`
narrowing).
fixtures.ts: 1759 -> 1618 lines (-141, ~8%).
api-methods + event-callbacks (49 tests) green.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
PLAN §3.1.c code-judo: collapse `performCopyWithLabel` and
`performCopyWithPerElementLabels` from two parallel async flows into a
single `runCopyJob({ primaryElement, elements, labelInstanceIds, ... })`
helper plus thin wrappers that build their specific label strategy.
The previous duplication was in the async tail (getNearestComponentName
-> executeCopyOperation -> common .catch handler that calls
updateLabelAfterCopy on each label id and unfreezes on copying-phase
errors). Both copies now share that tail.
copy-feedback (43) + shift-multi-select (40) tests still green.
index.tsx: -2 lines (small net since both wrappers retain their
label-construction bodies), but the .catch handler now lives in one
place instead of two slightly-different copies.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
Pull viewport-sync out of init() into a self-contained observer:
- redetectElementUnderPointer: gated re-detection at the pointer position
- handleViewportChange: cache-invalidate + redetect + viewport-version bump
- scroll/resize/visualViewport listeners
- scheduleBoundsSync: rAF-throttled coarse re-sync
- The shouldRunInterval effect: 100ms ticker that pins overlay bounds while
the renderer is showing something
- Disposal of both timers in onCleanup
The controller takes (grab, phase, isEnabled, isThemeEnabled,
eventListenerManager) and owns:
- boundsRecalcIntervalId, viewportChangeFrameId, previousViewport{Width,Height}
Drops 3 now-unused imports from index.tsx (nativeCancelAnimationFrame,
nativeRequestAnimationFrame, invalidateInteractionCaches,
ZOOM_DETECTION_THRESHOLD). 39 viewport + copy-feedback specs green.
index.tsx: 3425 -> 3314 lines (-111).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull arrow-key element navigation out of init() into a self-contained controller (~96 lines moved): - arrowNavigationElements + arrowNavigationActiveIndex signals - arrowNavigator (the createArrowNavigator() spatial helper) - clearArrowNavigation, selectAndFocusElement, openArrowNavigationMenu - handleArrowNavigation (arrow keydown dispatch) - handleArrowNavigationSelect (menu item click) - arrowNavigationItems + arrowNavigationState memos The controller takes (grab, phase, effectiveElement, isShiftMultiSelecting, setKeyboardSelectedElement) and exposes its state + handlers. The keyboardSelectedElement closure flag stays in init() (other handlers also read/write it); the controller writes to it via a setter callback. The contextmenu listener's `arrowNavigationElements().length > 0` check now uses the controller's `state().isVisible` instead. Drops 3 now-unused imports (getVisibleBoundsCenter, ArrowNavigationState type, createArrowNavigator). index.tsx: 3314 -> 3218 lines (-96). 21 keyboard-navigation specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Convert single-statement `() => { return expr; }` arrows to `() => expr`
in 6 utility helpers flagged by deslop-cli:
- utils/lerp.ts: lerp
- utils/is-element-visible.ts: isElementVisible
- utils/get-script-options.ts: isObjectRecord
- utils/get-elements-in-drag.ts: hasIntersection, sortByDocumentOrder,
removeNestedElements
- primitives.ts: isFreezeActive
Pure mechanical cleanup, no behavior change.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the document-level cursor stylesheet injection out of init() into its own module. The factory takes the three phase accessors that determine the cursor (activated, copying, promptMode), owns the lazy `<style data-react-grab-cursor>` element and its CSP nonce + third- party-hide setup, and registers its own createEffect. `cursorOverride.clear()` replaces the dispose-path `setCursorOverride(null)` call. Drops 2 now-unused imports (detectCspNonce, hideFromThirdParties). index.tsx: 3218 -> 3185 lines (-33). No behavior change. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The 40-line inline helper that resolves per-element metadata and dispatches the `react-grab:element-selected` CustomEvent depends only on resolveSource / getComponentDisplayName / getTagName / PREVIEW_TEXT_MAX_LENGTH — no closure state, no signals, no actions. Move it to utils/notify-elements-selected.ts. index.tsx: 3185 -> 3145 lines (-40). Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Pull the post-copy cooldown timer + isActive flag out of init() into a focused 3-method factory: - isActive() — true while the cooldown window is open - start() — begin/restart the FEEDBACK_DURATION_MS cooldown - clear() — immediately end + cancel pending timer Replaces two closure vars (isCopyFeedbackCooldownActive, copyFeedbackCooldownTimerId) and two inline helpers with a single typed value. Aliases preserve the existing call-site names so no other code needed changes. index.tsx: 3145 -> 3128 lines (-17). Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The two residual mutable closure flags (keyboardSelectedElement, pendingDefaultActionId) shared across init's subsystems now live in a single small controller. Their lifecycle (set / take / peek) is visible from one place rather than scattered across 6 callbacks. Each callback into a subsystem now forwards directly to the controller's bound method, so there's no more 'let x: T | null = null; ... x = newValue' patchwork in init(). index.tsx: 704 -> 688 (-16). 56 keyboard-nav + context-menu specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The calculateDragDistance/Rectangle 1-line wrappers were thin closures over store.dragStart that only the drag-handlers controller needs. Move them inside the controller so init() doesn't need the wrappers, the utility imports, or the store destructure. index.tsx: 688 -> 679 (-9). 15 drag-selection specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The 1-line alias for dragPreviewDebounce.schedule is gone — drag-handlers already received the full dragPreviewDebounce controller and calls .schedule directly now. index.tsx: 679 -> 677 (-2). Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…§9 item) The two large multi-line error message templates (Next Pages Router fallback + TanStack Start fallback) — each ~25 lines of escaped JSX inside a TS file — now live in a dedicated manual-setup-messages.ts module. The bodies are plain string literals (joined with '\n') instead of a forest of concatenated quoted strings with embedded newlines, which is easier to keep in sync with the docs. transform.ts: 708 -> 685 (-23). 124/124 CLI tests green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The toolbarElement closure flag in init() existed solely as the late-bound reference passed to createToolbarMenuController via a getToolbarElement callback. The controller now owns the reference internally and exposes setToolbarElement(), which the mount-renderer wires directly into onToolbarRef. No more closure, no more getter callback. index.tsx: 677 -> 672 (-5). 44 toolbar specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The last two closure flags in init() (disposed + disposeRenderer) are paired — they together represent the renderer's lifecycle state. Folded into a small controller exposing isDisposed/markDisposed/ getDisposeRenderer/setDisposeRenderer. mount-renderer and build-public-api now bind methods directly. This leaves zero hand-rolled `let` flags in init() — every mutable piece of coordination state is now owned by a typed controller. index.tsx: 672 -> 668 (-4). 24 api-methods specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ion (PLAN §9.4)
Two duplicate {x: number; y: number} type aliases pointed at the
same shape:
- get-bounds-center.ts: local BoundsCenter
- get-elements-in-drag.ts: local SamplePoint
Both now use the canonical Position from types.ts. Same shape, one
name, fewer locally-defined types to keep mental track of.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…PLAN §9.4)
Two duplicate type aliases with identical {pageX, pageY, width, height}
shape:
- store.ts: local FrozenDragRect
- create-bounds-from-drag-rect.ts: local DragRectWithPageCoords
Both now use the exported DragRectWithPageCoords from
create-bounds-from-drag-rect.ts. The field/setter names
(frozenDragRect, setFrozenDragRect) keep their meaning — only the
type behind them is now canonical.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The local BaseBounds interface in create-bounds-from-drag-rect.ts had the exact same shape as the exported DragRect type in types.ts. Use DragRect for the 2 callsites that needed it — one less locally-defined type alias. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… (PLAN §9.4) The local Bounds interface in combine-bounds.ts had the exact same shape as the exported DragRect type. The function is generic (`combineBounds<T extends DragRect>`) so callers can still pass any shape that extends it. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… fixtures (PLAN §9.4 A3) 6 inline copies of the canonical DragRect shape across fixtures.ts replaced with a single import of DragRect from src/types.js. Same structural type, one less hand-rolled inline definition per use. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… A3)
AnimatedBounds.current / .target are typed as the canonical DragRect
instead of two more inline {x;y;width;height} copies.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The 8 inline calls to isEventFromOverlay(event, 'data-react-grab-ignore-events') across pointer-listeners + keyboard-listeners now go through a tiny bound helper. The 'data-react-grab-ignore-events' attribute literal lives in one place instead of being spelled out 8 times. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… (PLAN §5.1) The 'store.contextMenuPosition !== null' predicate was duplicated 12 times across the listeners and controllers. Now it lives in one place as an isContextMenuOpen accessor on GrabPhaseSelectors, and each consumer destructures it like the other phase booleans. Files touched: pointer-listeners, keyboard-listeners, activation-key-handlers, arrow-navigation-controller, overlay-visibility, drag-handlers, selectors. 35 context-menu + arrow-nav specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ptions The optional onBeforePrompt callback was declared, threaded through the implementation, and called — but no callsite ever passed it (both callsites in menu-handlers and context-menu-action-context only set onBeforeCopy/customEnterPromptMode). Dead surface gone. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ion (PLAN §6.1) The two buildActionContext callers (default-action runPendingDefaultAction and the regular context-menu actions row) used disjoint subsets of the 11-field options bag — different optional callbacks and different shouldDeferHideContextMenu values. Now they're modeled as a discriminated union with mode: 'default-action' | 'context-menu'. - mode: 'default-action' requires performWithFeedbackOptions (the fallback bounds for the synchronous fire); shouldDeferHideContextMenu is no longer needed because the context menu was never shown. - mode: 'context-menu' accepts optional onBeforeCopy and customEnterPromptMode callbacks; the hide path always defers to the next microtask so the click that fires the action doesn't fall through. The type now tells you up-front which callbacks each path needs, instead of leaving callers to memorize the implicit "set X if Y" pattern. 35 context-menu specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
drag-preview-debounce's local DebouncedPointer interface was the same shape as the canonical Position type from types.ts. One less locally-defined alias to track. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
3 single-statement arrow function bodies '() => { return X; }' converted
to expression form '() => X':
- cli/src/utils/prompts.ts: prompts wrapper
- cli/src/utils/install.ts: getPackagesToInstall
- react-grab/src/core/plugin-registry.ts: getPluginNames
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ations The 'data-react-grab' string and its ATTRIBUTE_NAME alias was re-declared in 3 spec files (toolbar-selection-hover.spec.ts, freeze-animations.spec.ts, and the canonical fixtures.ts). Now lives in one place — fixtures.ts exports it, the 2 specs import it. 27 specs green. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The import was unused and held in place only by a 'void' suppression comment claiming 're-export type sigs'. Neither was true — the function isn't re-exported and isn't referenced. Consolidated the remaining createPageRectFromBounds into a single import line. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…cate aliases Every module in core/ that derives a GrabStoreHandle from createGrabStore was redeclaring `type GrabStoreHandle = ReturnType<typeof createGrabStore>` locally — 27 copies of the same type alias. The store module now exports the type directly and each consumer imports it instead of re-deriving. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…erHandle types Three more type aliases (each derived from a factory's ReturnType) had local copies scattered across modules: - PluginRegistry (13 modules) - AutoScroller (2 modules; existing private interface promoted to export) - EventListenerManagerHandle (6 modules) Each is now exported from its owning module and imported by consumers, following the same pattern as GrabStoreHandle. 18 files affected. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…rchestrator The interface was declared identically in both copy-orchestrator.ts and label-instance-manager.ts. Promote label-instance-manager's copy to exported and have copy-orchestrator import it. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
3 inline Promise<{x;y;w;h}> function returns in e2e/fixtures.ts now
use the canonical DragRect.
2 inline {id; bounds; createdAt} ReactGrabState.grabbedBoxes +
ReactGrabRendererProps.grabbedBoxes now use the canonical
PublicGrabbedBox type (already defined later in the same file).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The perf-fixtures PerfGridCenter type was the exact same shape as Position. Aliased to it so the canonical name carries the meaning and perf-bench consumers still see the existing PerfGridCenter identifier. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
![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.
Reviewed by Cursor Bugbot for commit 033f191. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 92 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/overlay-effects.ts">
<violation number="1" location="packages/react-grab/src/core/overlay-effects.ts:67">
P1: Activation freeze side effects are not tied to effect disposal. If `dispose()` runs while active, global pseudo-state/animation freeze may remain applied because unfreeze is only in the deactivation branch.</violation>
</file>
<file name="packages/cli/src/utils/option-prompts.ts">
<violation number="1" location="packages/cli/src/utils/option-prompts.ts:15">
P3: Cancel check dead code here. Shared `prompts` already exits on cancel before this branch can run. Remove local guard to keep one cancel path.</violation>
</file>
<file name="packages/cli/src/utils/framework-paths.ts">
<violation number="1" location="packages/cli/src/utils/framework-paths.ts:73">
P2: Detection list misses JS/JSX Vite entries. Existing install in `main.jsx`/`index.js` can be missed, then CLI may try to install again. Reuse the shared Vite candidate list.</violation>
</file>
<file name="packages/react-grab/src/core/copy-feedback-cooldown.ts">
<violation number="1" location="packages/react-grab/src/core/copy-feedback-cooldown.ts:35">
P2: Stale timeout callback can end new cooldown early. Guard callback with timer-id check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // On deactivation: undo each in reverse. | ||
| createEffect( | ||
| on(isActivated, (activated, previousActivated) => { | ||
| if (activated && !previousActivated) { |
Contributor
There was a problem hiding this comment.
P1: Activation freeze side effects are not tied to effect disposal. If dispose() runs while active, global pseudo-state/animation freeze may remain applied because unfreeze is only in the deactivation branch.
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/overlay-effects.ts, line 67:
<comment>Activation freeze side effects are not tied to effect disposal. If `dispose()` runs while active, global pseudo-state/animation freeze may remain applied because unfreeze is only in the deactivation branch.</comment>
<file context>
@@ -0,0 +1,98 @@
+ // On deactivation: undo each in reverse.
+ createEffect(
+ on(isActivated, (activated, previousActivated) => {
+ if (activated && !previousActivated) {
+ freezePseudoStates(grab.pointer().x, grab.pointer().y);
+ freezeGlobalAnimations();
</file context>
…riminated union (PLAN §3.8)
Three flags previously encoded the same 'what does the next click do?'
state machine in mutually-exclusive branches:
- store.pendingCommentMode: boolean
- coordination-flags.pendingDefaultActionId: string | null
- isPendingContextMenuSelect signal
Now collapsed into one type-enforced union on the store:
type ActivationIntent =
| { kind: 'default' }
| { kind: 'comment' }
| { kind: 'context-menu'; actionId: string };
Migration touched ~10 files, ~52 read sites + ~10 write sites:
- store.ts: replaced field + action; deactivate resets via union
- coordination-flags.ts: dropped pendingDefaultActionId half
- comment-mode-handlers.ts: setActivationIntent in the toolbar
default-action branches
- menu-handlers.ts: runPendingDefaultAction reads intent + resets
- drag-handlers.ts: 8 read sites updated; 2 consume-intent branches
inline the reset
- drag-selectors.ts: single read-site update
- selectors.ts: isCommentMode now reads activationIntent
- build-public-api.ts: api.activate calls resetActivationIntent
- activation-lifecycle.ts: clearPendingContextMenuSelect renamed
clearActivationIntent
- index.tsx: dropped isPendingContextMenuSelect signal + plumbing
Mutual exclusion is now type-enforced (setActivationIntent always
replaces). 181/181 focused E2E specs (prompt-mode, context-menu,
activation, shift-multi-select, drag-selection, api-methods, toolbar)
green; full suite pending.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 11 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/overlay-effects.ts">
<violation number="1" location="packages/react-grab/src/core/overlay-effects.ts:67">
P1: Activation freeze side effects are not tied to effect disposal. If `dispose()` runs while active, global pseudo-state/animation freeze may remain applied because unfreeze is only in the deactivation branch.</violation>
</file>
<file name="packages/react-grab/src/core/menu-handlers.ts">
<violation number="1" location="packages/react-grab/src/core/menu-handlers.ts:193">
P1: The pending default-action check now relies on `activationIntent`, but that intent is cleared before dispatch in existing click/drag handlers, so configured default actions stop auto-running and fall back to opening the context menu.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }, 0); | ||
| }; | ||
|
|
||
| const hasPendingDefaultAction = () => store.activationIntent.kind === "context-menu"; |
Contributor
There was a problem hiding this comment.
P1: The pending default-action check now relies on activationIntent, but that intent is cleared before dispatch in existing click/drag handlers, so configured default actions stop auto-running and fall back to opening the context menu.
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/menu-handlers.ts, line 193:
<comment>The pending default-action check now relies on `activationIntent`, but that intent is cleared before dispatch in existing click/drag handlers, so configured default actions stop auto-running and fall back to opening the context menu.</comment>
<file context>
@@ -188,7 +190,7 @@ export const createMenuHandlers = (input: MenuHandlersInput): MenuHandlers => {
};
- const hasPendingDefaultAction = () => peekPendingDefaultActionId() !== null;
+ const hasPendingDefaultAction = () => store.activationIntent.kind === "context-menu";
const openContextMenuOrRunPendingDefault = (element: Element, position: Position) => {
</file context>
5 issues flagged across cubic + cursor-bugbot reviews: 1. **P1 (regression I introduced)** menu-handlers.ts:193 — drag handlers were resetting activationIntent BEFORE dispatching to openContextMenuOrRunPendingDefault, which then saw kind='default' and opened the regular context menu instead of running the configured default action. Fix: don't pre-reset; let runPendingDefaultAction own the read+reset atomically. Add a reset on the wasIntercepted branch so plugin interception doesn't leave the intent armed. 2. **P1 (pre-existing latent)** overlay-effects.ts:67 — activation freeze side effects (pseudo-state freeze, WAAPI animation freeze) leak onto the host page if api.dispose() runs while the overlay is active. Both unfreeze fns are idempotent, so add them to init-cleanup. 3. **P2** framework-paths.ts:73 — REACT_GRAB_DETECTION_PATHS only included 4 of the 8 Vite entry variants from VITE_ENTRY_CANDIDATES (missed main.jsx/main.js/index.jsx/index.js). Reuse the canonical table. 4. **P2 (defense-in-depth)** copy-feedback-cooldown.ts:35 — capture the timer id at scheduling time and guard the callback against firing for a stale handle. 5. **P3** option-prompts.ts:15 — drop the dead exitOnCancel guard. The shared prompts wrapper already process.exit(0)s on cancel, so the check could never trigger. 6. **Low (bugbot)** fixtures.ts:1119 — getState would throw if state was missing because state?.labelInstances.map(...) is missing the second optional chain. Add ?. before .map. All checks remain green; 118 focused E2E specs + 124 CLI tests verify the menu-handlers regression is fixed. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
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
2 participants
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.
Thermo-nuclear maintainability cleanup. All changes are pure refactors — no behavior changes. 86 commits, 538/538 E2E green, 124/124 CLI green.
Stats
packages/react-grab/src/core/index.tsxpackages/react-grab/src/utils/freeze-updates.tspackages/react-grab/e2e/fixtures.tspackages/cli/src/utils/transform.tspackages/cli/src/utils/detect.tspackages/cli/src/commands/init.tspackages/cli/src/commands/configure.ts48 new focused modules created.
core/index.tsxis now a clean composition root with zero hand-rolled mutable state — every mutable coordination value is owned by a typed controller.core/index.tsxdecomposition (PLAN §3.1) — 43 sub-modulesEvent handler controllers / listener registrars
drag-handlers.ts(520) — Pointer/click clusterkeyboard-listeners.ts(321) — keydown + keyup listener bodiesactivation-key-handlers.ts(300) — 4 activation key handlerspointer-listeners.ts(277) — 8 pointer/click cluster listenersmount-renderer.tsx(261) — Dynamic-import + 60-prop ReactGrabRenderermenu-handlers.ts(209) — 7 menu-lifecycle handlersprompt-mode-handlers.ts(130) — 5 prompt-mode UI handlerswindow-focus-listeners.ts(116) — visibilitychange/blur/focus/focusincomment-mode-handlers.ts(98) — handleToggleActive, enterCommentMode, handleCommentState controllers
Selectors / derivations
Side-effect bundles
Pure utilities
utils/notify-elements-selected.ts,utils/drag-geometry.tsfreeze-updates.ts4-job split (PLAN §3.7) — 6 sub-modulesfreeze-updates.ts(orchestrator)freeze/dispatcher-patch.tspatchDispatcherwraps useState/etcfreeze/hook-queue.tsfreeze/context-dependency.tsfreeze/pending-update-chain.tsfreeze/state.tsfreeze/types.tsCLI dedup (PLAN §3.3, §3.4 + §9 item 9)
cli/src/utils/framework-paths.ts— single canonical module for project-layout conventions.cli/src/utils/option-prompts.ts— 4 shared per-option prompts.cli/src/utils/manual-setup-messages.ts— two large multi-line manual-setup error messages extracted fromtransform.ts.E2E fixture cleanup (PLAN §3.2 partial)
Dropped 32+ inline
(window as { ... }).__REACT_GRAB__casts. Replaced with onedeclare global { interface Window { ... } }block. The latent type bug ingetStateis now fixed. Folded 9 inline{x;y;width;height}shapes (interface props + Promise return types) into the canonicalDragRect. Consolidated 3 dupATTRIBUTE_NAMEconst declarations across specs into one fixture export.Type-shape canonicalization (PLAN §9.4 + A3)
BoundsCenter+SamplePoint+DebouncedPointer+PerfGridCenter→ canonicalPosition(4 dup aliases)FrozenDragRect→ canonicalDragRectWithPageCoords(store + util alignment)BaseBounds+ localBounds→ canonicalDragRect(3 callsites){x;y;width;height}inoverlay-canvas.tsx→DragRect(2 sites){x;y;width;height}ine2e/fixtures.ts→DragRect(9 sites){id; bounds; createdAt}intypes.ts→PublicGrabbedBox(2 sites)Factory-derived type exports (post-decomp grime)
The decomposition created 48 redundant local type aliases (each sub-module had its own
type X = ReturnType<typeof factoryX>). Now exported once from their owning modules:GrabStoreHandlefromstore.ts— 27 callsitesPluginRegistryfromplugin-registry.ts— 13 callsitesAutoScrollerfromauto-scroll.ts— 2 callsites (existing private interface promoted)EventListenerManagerHandlefromevents.ts— 6 callsitesPerElementLabelEntryfromlabel-instance-manager.ts— 2 callsitesPredicate consolidation (PLAN §5.1)
isEventFromIgnoredOverlay(event)helper replaces 8 inlineisEventFromOverlay(event, 'data-react-grab-ignore-events')callsites.isContextMenuOpenphase selector replaces 12 inlinestore.contextMenuPosition !== nullcallsites.Type-contract improvements (PLAN §6.1)
BuildActionContextOptionsis now a discriminated union bymode: "default-action" | "context-menu". The two callers now declare which path they're on explicitly, and the type tells you up-front which callbacks each path needs.onBeforePromptoption (was threaded through but never set).Micro-cleanups (PLAN §9 appendix)
picomatchdev-dep.{ id; bounds; createdAt }shapes → canonicalPublicGrabbedBox.Icon*Propsinterfaces → one sharedIconProps.!!process.env.CI→Boolean(...).Verification
pnpm buildpnpm typecheck(4 packages)pnpm lint(0 errors, 0 warnings)pnpm test:cli— 124/124deslop-cliclean except for 2 PLAN-flagged-as-deferred semantic-name pairs (StackContextOptions/GenerateSnippetOptions, ArrowNavigationItem/TagDisplayOutput)What's deferred to future PRs
getStatebug, the inline shape duplicates, and the dupATTRIBUTE_NAMEdeclarations are fixed.SelectionLabelInstancediscriminated union — risky because the renderer reads many fields withinstance.x ?? defaultpatterns regardless of status.ReactGrabRendererPropsis still flat. Bigger blast radius.activationIntentdiscriminated union — touches multiple flag cascades in drag/click flow; risky.callHookXconsolidation — examined; the 5 variants have distinct return semantics (sync void, sync boolean, async, sync reduce, async reduce). Consolidating them would add more complexity than it removes.isNextProjectglobal check to a per-frameisSymbolicatedcheck would lose line/column info on client-side Next.js frames (which aren't symbolicated server-side). Behavior change, not refactor.?? null/?? undefinedsweep — sampled them; most are legitimatenull↔undefinedcoercions.Summary by cubic
Refactored
react-grabfor maintainability: split the core into focused modules, deduped CLI detection/prompts/messages, and simplified E2E fixtures/types. Also fixed an activation-intent regression and added cleanup/detection guards.Refactors
core/index.tsxinto targeted controllers/selectors (keyboard, pointer/drag, activation/hold, overlay visibility/effects, toolbar/menu, viewport/focus sync, renderer host/mount, plugin state bridge, and public API); split React update-freeze into an orchestrator plusfreeze/*.notify-elements-selected, addedisEventFromIgnoredOverlay, consolidated shift-multi-select and arrow navigation, added cursor override and Enter blocker, and centralized init/cleanup.build-public-api,mount-renderer, context-menu action-context, selection-source sync, prompt/comment-mode handlers, action-context builder, and window-focus listeners.utils/framework-paths.ts, shared option prompts inutils/option-prompts.ts, manual-setup text inutils/manual-setup-messages.ts; expanded Vite entry detection variants; removed duplicate logic indetect.ts/transform.ts.windowglobal, fixedgetStateserialization, exportedATTRIBUTE_NAME, folded inline shapes to canonicalDragRect/Position, usedPublicGrabbedBoxfor grabbed boxes; sharedIconProps.GrabStoreHandle,PluginRegistry,AutoScroller,EventListenerManagerHandle, andPerElementLabelEntry; addedisContextMenuOpenselector; convertedBuildActionContextOptionsto a discriminated union; replaced three click-intent flags withActivationIntent; removed deadonBeforePrompt.Boolean(process.env.CI), and removed dead imports.picomatchfrom the root.Bug Fixes
api.dispose()to avoid leaks if disposed while active.main.{js,jsx}/index.{js,jsx}) and dropped a redundant cancel guard in shared prompts.getStateto prevent a crash when state is undefined.Written for commit c02bb45. Summary will update on new commits. Review in cubic