refactor: enhance plugin registration and API options handling #102
Conversation
- Updated the API to include a `registerPlugin` method, allowing for more flexible plugin management. - Refactored existing code to utilize the new plugin system, improving extensibility and maintainability. - Adjusted type definitions and interfaces to support the new plugin architecture, ensuring compatibility with existing functionality.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| const url = buildOpenFileUrl(filePath, lineNumber ?? undefined); | ||
| window.open(url, "_blank", "noopener,noreferrer"); | ||
| } | ||
| pluginRegistry.hooks.onOpenFile(filePath, lineNumber ?? undefined); |
There was a problem hiding this comment.
The original code conditionally opened the URL only when onOpenFile was not provided. Now both the hook and the fallback URL logic execute unconditionally. If a plugin provides an onOpenFile handler that handles navigation itself, this will result in opening the file twice - once via the plugin and once via window.open.
Consider preserving the original conditional behavior, or have the hook return a boolean to indicate whether the default behavior should be suppressed.
| pluginRegistry.hooks.onOpenFile(fileInfo.filePath, fileInfo.lineNumber ?? undefined); | ||
| const openFileUrl = buildOpenFileUrl( | ||
| fileInfo.filePath, | ||
| fileInfo.lineNumber, |
There was a problem hiding this comment.
Same issue here - the hook and the fallback window.open both execute unconditionally. Previously the window.open was only called when no onOpenFile callback was provided.
- Revised sections on plugin registration and lifecycle hooks to reflect recent API changes. - Added detailed examples for new plugins, including analytics and context menu actions. - Improved clarity in usage instructions and added a section on unregistering plugins.
![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 21 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/grab/README.md">
<violation number="1" location="packages/grab/README.md:603">
P2: Documentation example is missing `Content-Type` header for JSON POST request. Users copying this code may find their API requests fail because servers typically require this header to parse JSON bodies correctly.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:612">
P2: Missing `Content-Type: application/json` header in fetch request. Without this header, servers may not correctly parse the JSON body, causing this documentation example to fail for developers who copy it.</violation>
</file>
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:1340">
P1: Behavior change causes duplicate file openings. The original code conditionally called either the custom `onOpenFile` handler OR opened the URL in a browser. The new code unconditionally does both, which will open files twice when a plugin provides an `onOpenFile` hook (e.g., opening in IDE and browser simultaneously). Consider making the `window.open` conditional on whether any plugin handled the event, or provide a mechanism for hooks to indicate the event was handled.</violation>
<violation number="2" location="packages/react-grab/src/core/index.tsx:2228">
P1: Same behavior regression as above - `handleContextMenuOpen` now unconditionally opens files in browser AND calls plugin hooks, causing duplicate file openings when plugins provide custom file opening behavior.</violation>
<violation number="3" location="packages/react-grab/src/core/index.tsx:2421">
P1: `checkConnection()` results can update state after the agent provider has changed/unregistered (stale async update), and rejections are unhandled. Also, capabilities aren’t reset when the provider disappears, which can leave stale UI state. Capture the provider, verify it’s still current before updating, add a `.catch`, and reset capabilities when no provider is present.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- Added `onOpenFile` hook to allow plugins to handle file opening events, providing flexibility for custom behaviors. - Updated context menu handling to utilize the new `onOpenFile` hook, improving integration with plugins. - Refactored related code to ensure consistent handling of file opening across the application.
- Updated the logic for setting agent capabilities to use a captured provider, improving clarity and reducing redundancy. - Enhanced connection checking for the agent provider, ensuring accurate updates to the agent's connection status. - Refactored related code for better maintainability and consistency in handling agent options.
- Renamed `PluginContribution` to `PluginConfig` to better reflect its purpose. - Adjusted type definitions in the core and types files to ensure consistency with the new naming convention. - Updated imports and references throughout the codebase to align with the refactored interface.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/react-grab/src/core/plugin-registry.ts">
<violation number="1" location="packages/react-grab/src/core/plugin-registry.ts:102">
P1: Options set via `setOptions` will be lost when plugins are registered/unregistered. The `recomputeStore` function rebuilds options from `DEFAULT_OPTIONS` and `initialOptions` without preserving changes made via `setOptions`. Consider tracking direct option updates separately and merging them in `recomputeStore`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Enhanced the `setOptions` function to streamline the updating of plugin options, ensuring undefined values are ignored. - Updated the logic for merging options to include direct overrides, improving the clarity and maintainability of the code. - Refactored related code to ensure consistent handling of options within the plugin registry.
| getRequiredModifiers, | ||
| setupKeyboardEventClaimer, | ||
| } from "./keyboard-handlers.js"; | ||
| import { createAutoScroller, getAutoScrollDirection } from "./auto-scroll.js"; | ||
| import { logIntro } from "./log-intro.js"; | ||
| import { onIdle } from "../utils/on-idle.js"; | ||
| import { getScriptOptions } from "../utils/get-script-options.js"; | ||
| import { isEnterCode } from "../utils/is-enter-code.js"; | ||
|
|
||
| let hasInited = false; | ||
|
|
||
| export const init = (rawOptions?: Options): ReactGrabAPI => { | ||
| if (typeof window === "undefined") { | ||
| return createNoopApi(); | ||
| } | ||
|
|
||
| const scriptOptions = getScriptOptions(); | ||
|
|
||
| const initialOptions: Options = { | ||
| enabled: true, | ||
| activationMode: "toggle", | ||
| keyHoldDuration: DEFAULT_KEY_HOLD_DURATION_MS, | ||
| allowActivationInsideInput: true, | ||
| maxContextLines: 3, | ||
| ...scriptOptions, | ||
| ...rawOptions, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| if (plugin.theme) { | ||
| config.theme = config.theme | ||
| ? deepMergeTheme( | ||
| deepMergeTheme(DEFAULT_THEME, plugin.theme), | ||
| config.theme, | ||
| ) | ||
| : plugin.theme; | ||
| } | ||
|
|
||
| if (plugin.agent) { | ||
| config.agent = config.agent | ||
| ? { ...plugin.agent, ...config.agent } | ||
| : plugin.agent; | ||
| } | ||
|
|
||
| if (plugin.contextMenuActions) { | ||
| config.contextMenuActions = [ | ||
| ...plugin.contextMenuActions, | ||
| ...(config.contextMenuActions ?? []), | ||
| ]; | ||
| } | ||
|
|
||
| if (plugin.hooks) { | ||
| config.hooks = config.hooks | ||
| ? { ...plugin.hooks, ...config.hooks } | ||
| : plugin.hooks; | ||
| } | ||
|
|
||
| if (plugin.options) { | ||
| config.options = config.options | ||
| ? { ...plugin.options, ...config.options } | ||
| : plugin.options; | ||
| } |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| const isReleasingActivationKey = pluginRegistry.store.options.activationShortcut | ||
| ? !pluginRegistry.store.options.activationShortcut(event) | ||
| : pluginRegistry.store.options.activationKey | ||
| ? pluginRegistry.store.options.activationKey.key | ||
| ? event.key?.toLowerCase() === | ||
| optionsStore.store.activationKey.key.toLowerCase() || | ||
| keyMatchesCode(optionsStore.store.activationKey.key, event.code) | ||
| pluginRegistry.store.options.activationKey.key.toLowerCase() || | ||
| keyMatchesCode(pluginRegistry.store.options.activationKey.key, event.code) | ||
| : false | ||
| : isCLikeKey(event.key, event.code); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| const computedLabelInstances = createMemo(() => { | ||
| if (!optionsStore.store.theme.enabled) return []; | ||
| if (!optionsStore.store.theme.grabbedBoxes.enabled) return []; | ||
| if (!pluginRegistry.store.theme.enabled) return []; | ||
| if (!pluginRegistry.store.theme.grabbedBoxes.enabled) return []; | ||
| void store.viewportVersion; | ||
| const currentIds = new Set(store.labelInstances.map((i) => i.id)); | ||
| for (const cachedId of labelInstanceCache.keys()) { | ||
| if (!currentIds.has(cachedId)) { | ||
| labelInstanceCache.delete(cachedId); | ||
| } | ||
| } | ||
| return store.labelInstances.map((instance) => { | ||
| const newBounds = | ||
| instance.element && document.body.contains(instance.element) | ||
| ? createElementBounds(instance.element) | ||
| : instance.bounds; | ||
| const previousInstance = labelInstanceCache.get(instance.id); | ||
| const boundsUnchanged = | ||
| previousInstance && | ||
| previousInstance.bounds.x === newBounds.x && | ||
| previousInstance.bounds.y === newBounds.y && | ||
| previousInstance.bounds.width === newBounds.width && | ||
| previousInstance.bounds.height === newBounds.height; | ||
| if ( | ||
| previousInstance && | ||
| previousInstance.status === instance.status && | ||
| previousInstance.errorMessage === instance.errorMessage && | ||
| boundsUnchanged | ||
| ) { | ||
| return previousInstance; | ||
| } | ||
| const newCached = { ...instance, bounds: newBounds }; | ||
| labelInstanceCache.set(instance.id, newCached); | ||
| return newCached; | ||
| }); | ||
| }); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| const computedGrabbedBoxes = createMemo(() => { | ||
| if (!optionsStore.store.theme.enabled) return []; | ||
| if (!optionsStore.store.theme.grabbedBoxes.enabled) return []; | ||
| if (!pluginRegistry.store.theme.enabled) return []; | ||
| if (!pluginRegistry.store.theme.grabbedBoxes.enabled) return []; | ||
| void store.viewportVersion; | ||
| return store.grabbedBoxes.map((box) => { | ||
| if (!box.element || !document.body.contains(box.element)) { | ||
| return box; | ||
| } | ||
| return { | ||
| ...box, | ||
| bounds: createElementBounds(box.element), | ||
| }; | ||
| }); | ||
| }); | ||
|
|
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| const isReleasingModifier = | ||
| requiredModifiers.metaKey || requiredModifiers.ctrlKey | ||
| ? !event.metaKey && !event.ctrlKey | ||
| : (requiredModifiers.shiftKey && !event.shiftKey) || | ||
| (requiredModifiers.altKey && !event.altKey); |
There was a problem hiding this comment.
🟡 MEDIUM - Redundant boolean expression could be simplified
Agent: quality
Category: quality
Description:
isReleasingModifier uses ternary with similar checks for metaKey/ctrlKey vs shiftKey/altKey that could be extracted to a predicate function for clarity.
Suggestion:
Create a named predicate function 'hasAnyRequiredModifierBeenReleased(requiredModifiers, event)' to clearly express the intent.
Confidence: 75%
Rule: quality_complex_boolean_extraction
Review ID: 3e728555-eade-4788-ba18-dbf7f6af5545
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| actions.freeze(); | ||
| actions.showContextMenu(position, instance.element!); | ||
| }, 0); | ||
| }; |
There was a problem hiding this comment.
🟡 MEDIUM - Non-null assertion without proper type narrowing
Agent: typescript
Category: quality
Description:
instance.element is asserted as non-null with ! operator. If guard logic changes, this becomes a silent type safety issue.
Suggestion:
Use explicit type narrowing: if (!instance.element) return; // then instance.element is narrowed without assertion
Confidence: 72%
Rule: typescript_avoid_type_assertions
Review ID: 3e728555-eade-4788-ba18-dbf7f6af5545
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| if (!wasHandled) { | ||
| const url = buildOpenFileUrl(filePath, lineNumber ?? undefined); | ||
| window.open(url, "_blank", "noopener,noreferrer"); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Repeated bounds calculation logic
Agent: refactoring
Category: quality
Description:
Lines 1350-1365 contain duplicate allBounds.length > 1 check twice, violating DRY principle.
Suggestion:
Extract a helper function like getActiveSelectionBounds() that returns both combined bounds and selection array.
Confidence: 70%
Rule: quality_guard_clauses
Review ID: 3e728555-eade-4788-ba18-dbf7f6af5545
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| const callHookAsync = async <K extends HookName>( | ||
| hookName: K, | ||
| ...args: Parameters<NonNullable<PluginHooks[K]>> | ||
| ): Promise<void> => { | ||
| for (const { config } of plugins.values()) { | ||
| const hook = config.hooks?.[hookName] as | ||
| | ((...hookArgs: Parameters<NonNullable<PluginHooks[K]>>) => ReturnType<NonNullable<PluginHooks[K]>>) | ||
| | undefined; | ||
| if (hook) { | ||
| await hook(...args); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 MEDIUM - Type casting without validation in callHookAsync
Agent: Delegated (react, refactoring, testing)
Category: quality
Description:
callHookAsync uses complex type casting without validation. If a plugin provides a hook with wrong signature, it could cause runtime errors.
Suggestion:
Add a type guard function isValidHook() that validates the hook before casting. Log warning for invalid signatures.
Confidence: 68%
Rule: ts_type_assertion_abuse
Review ID: 3e728555-eade-4788-ba18-dbf7f6af5545
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| const pluginKeys = ["theme", "agent", "contextMenuActions"]; | ||
| const hookKeys = [ | ||
| "onActivate", "onDeactivate", "onElementHover", "onElementSelect", | ||
| "onDragStart", "onDragEnd", "onBeforeCopy", "onAfterCopy", | ||
| "onCopySuccess", "onCopyError", "onStateChange", "onPromptModeChange", | ||
| "onSelectionBox", "onDragBox", "onCrosshair", "onGrabbedBox", | ||
| "onContextMenu", "onOpenFile", "onElementLabel", | ||
| ]; |
There was a problem hiding this comment.
🔵 LOW - Magic string arrays without documentation
Agent: quality
Category: quality
Description:
hookKeys and pluginKeys arrays contain hardcoded strings for configuration keys without validation against actual types.
Suggestion:
Move to named constants with comments or generate from types to ensure sync.
Confidence: 65%
Rule: qual_magic_numbers_js
Review ID: 3e728555-eade-4788-ba18-dbf7f6af5545
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
@diffray-bot fuck off |
registerPluginmethod, allowing for more flexible plugin management.Note
Plugin-based architecture
plugin-registry: mergestheme,agent,options,contextMenuActionsand dispatcheshooks; removesoptions-storeregisterPlugin,unregisterPlugin,getPlugins;setOptionsnow updates only base optionsonOpenFileis now a hook that can intercept (returns boolean); copy flow refactored to pass hooks explicitlyUpdates across packages
registerPlugin({ name, agent: { ... } })setOptionscallbacks to plugins; new tests forregisterPluginWritten by Cursor Bugbot for commit f5553a9. This will update automatically on new commits. Configure here.
Summary by cubic
Introduce a plugin system with a new registerPlugin API to make extending React Grab modular and flexible. Core now reads theme, agent, options, and hooks from plugins, and providers were updated to use the new API.
New Features
Migration
Written for commit f5553a9. Summary will update on new commits.