feat: add hooks management and translation for Claude Code plugins#42
feat: add hooks management and translation for Claude Code plugins#42
Conversation
|
Warning Rate limit exceeded@yordis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds translation, merging, and cleanup for Claude Code plugin hooks: translates per-plugin Claude hooks to Cursor format during sync, merges AIPM-managed hooks into Changes
Sequence Diagram(s)sequenceDiagram
participant SyncCmd as Sync Command
participant SyncHelper as Sync Helper (sync-strategy)
participant Translator as Hooks Translator
participant Merger as Hooks Merger
participant FS as File System
Note over SyncCmd,Merger: Plugin sync -> translate hooks -> final merge
SyncCmd->>SyncHelper: syncPluginToCursor(plugin)
SyncHelper->>FS: check for plugin `hooks.json`
alt hooks.json found
SyncHelper->>Translator: translateClaudeCodeHook(hooks.json, marketplace, plugin, pluginPath)
Translator-->>SyncHelper: CursorHooksConfig (translated)
SyncHelper->>FS: copy plugin files (exclude plugin hooks.json)
SyncHelper->>FS: remove plugin hooks.json in target
SyncHelper-->>SyncCmd: return translatedHooks
else not found
SyncHelper-->>SyncCmd: return no translatedHooks
end
SyncCmd->>Merger: mergeHooks(targetDir, collectedPluginHooks)
Merger->>FS: read `.cursor/hooks.json`
Merger->>Merger: preserve non-AIPM user hooks + append AIPM hooks
Merger->>FS: write merged `.cursor/hooks.json`
sequenceDiagram
participant UninstallCmd as Uninstall Command
participant MergerReader as Hooks Merger Reader
participant FS as File System
Note over UninstallCmd,MergerReader: Uninstall-time hook cleanup
UninstallCmd->>MergerReader: readExistingHooks(cursorDir)
MergerReader->>FS: read `.cursor/hooks.json`
alt valid hooks.json
MergerReader->>MergerReader: filter hooks where x-managedBy=="aipm" and x-hookId startsWith(pluginPrefix)
MergerReader->>FS: write filtered `.cursor/hooks.json`
else missing/invalid
MergerReader-->>UninstallCmd: no-op (graceful)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR SummaryAdds end-to-end hooks support: translate Claude Code hooks to Cursor format, merge/preserve user hooks, and clean up hooks on sync/uninstall.
Written by Cursor Bugbot for commit ce8ab82. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
yordis/feat-2
branch
from
3be2981 to
5364925
Compare
yordis
force-pushed
the
yordis/feat-2
branch
from
5364925 to
6a39b1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/helpers/sync-strategy.ts (3)
174-265: Consider extracting duplicated cleanup logic.The three branches (translation success, translation failure, no hooks.json) share nearly identical cleanup logic:
- Copy hooks directory via
syncDirectory- Delete
hooks.jsonif it exists- Remove empty hooks directory
Consider extracting this into a helper:
+async function cleanupHooksDirectory(targetHooksDir: string): Promise<void> { + const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); + if (await fileExists(copiedHooksJson)) { + await rm(copiedHooksJson); + } + try { + const remainingFiles = await readdir(targetHooksDir); + if (remainingFiles.length === 0) { + await rm(targetHooksDir, { recursive: true }); + } + } catch { + // Directory doesn't exist or can't be read - ignore + } +}Then each branch can call
await cleanupHooksDirectory(targetHooksDir).
189-197: Minor inefficiency: copying then immediately deleting hooks.json.Line 191 copies all files (including
hooks.json) viasyncDirectory(..., []), then lines 194-197 immediately delete the copiedhooks.json. Consider excludinghooks.jsonduring copy to avoid this redundant I/O:- await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); - - // Remove the original hooks.json since it's been translated and merged into .cursor/hooks.json - const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); - if (await fileExists(copiedHooksJson)) { - await rm(copiedHooksJson); - } + // Copy hooks directory excluding hooks.json (already translated) + await syncDirectoryExcluding(join(pluginPath, 'hooks'), targetHooksDir, [FILE_HOOKS_JSON]);This is a minor optimization since
hooks.jsonis typically small.
209-211: Clarify or remove misleading comment.The comment "Scripts stay in global plugin location - don't copy them" is confusing since
syncDirectoryon line 191 does copy files. If the intent is to explain that hook commands reference scripts at their original location (rather than copied scripts), consider rephrasing:- // Scripts stay in global plugin location - don't copy them + // Note: Translated hook commands reference scripts at their original plugin pathOr if the comment is no longer accurate, remove it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(1 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(1 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/constants.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-merger.test.tstests/helpers/hooks-translator.test.tstests/commands/plugin-uninstall.test.tssrc/schema.tssrc/helpers/hooks-translator.tstests/commands/sync.test.tssrc/helpers/hooks-merger.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/constants.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-merger.test.tstests/helpers/hooks-translator.test.tstests/commands/plugin-uninstall.test.tssrc/schema.tssrc/helpers/hooks-translator.tstests/commands/sync.test.tssrc/helpers/hooks-merger.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/constants.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-merger.test.tstests/helpers/hooks-translator.test.tstests/commands/plugin-uninstall.test.tssrc/schema.tssrc/helpers/hooks-translator.tstests/commands/sync.test.tssrc/helpers/hooks-merger.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/constants.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-merger.test.tstests/helpers/hooks-translator.test.tstests/commands/plugin-uninstall.test.tssrc/schema.tssrc/helpers/hooks-translator.tstests/commands/sync.test.tssrc/helpers/hooks-merger.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/helpers/hooks-merger.test.tstests/helpers/hooks-translator.test.tstests/commands/plugin-uninstall.test.tstests/commands/sync.test.ts
🧬 Code graph analysis (8)
src/commands/plugin-uninstall.ts (3)
src/constants.ts (2)
DIR_CURSOR(4-4)FILE_HOOKS_JSON(59-59)src/helpers/hooks-merger.ts (1)
readExistingHooks(13-38)src/helpers/fs.ts (1)
writeJsonFile(35-49)
tests/helpers/hooks-translator.test.ts (2)
src/schema.ts (1)
ClaudeCodeHook(196-196)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-92)countTranslatedHooks(97-103)
tests/commands/plugin-uninstall.test.ts (4)
src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/commands/plugin-uninstall.ts (1)
pluginUninstall(36-222)src/helpers/fs.ts (2)
readJsonFile(51-64)fileExists(26-33)src/schema.ts (1)
CursorHooksConfig(198-198)
src/helpers/hooks-translator.ts (3)
src/schema.ts (3)
ClaudeCodeHook(196-196)CursorHooksConfig(198-198)CursorHook(197-197)src/helpers/io.ts (1)
defaultIO(79-79)src/constants.ts (1)
CLAUDE_PLUGIN_ROOT_VAR(58-58)
tests/commands/sync.test.ts (3)
src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/schema.ts (1)
CursorHooksConfig(198-198)
src/helpers/hooks-merger.ts (4)
src/schema.ts (1)
CursorHooksConfig(198-198)src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (3)
fileExists(26-33)readJsonFile(51-64)writeJsonFile(35-49)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/sync.ts (2)
src/schema.ts (1)
CursorHooksConfig(198-198)src/helpers/hooks-merger.ts (1)
mergeHooks(89-100)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(198-198)ClaudeCodeHookSchema(160-177)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-92)countTranslatedHooks(97-103)src/helpers/io.ts (1)
defaultIO(79-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (34)
src/constants.ts (1)
54-59: LGTM!The new hook-related constants follow the existing naming conventions and provide a consistent way to reference the hooks configuration file and the Claude plugin root variable placeholder across the codebase.
src/commands/plugin-uninstall.ts (2)
11-11: LGTM!The new imports (FILE_HOOKS_JSON, writeJsonFile, readExistingHooks) are necessary for implementing the hooks cleanup functionality and properly scoped from their respective modules.
Also applies to: 16-16, 18-18
176-214: Hooks cleanup logic looks good, type guard addresses prior concern.The hook cleanup implementation correctly:
- Filters out hooks belonging to the uninstalled plugin by checking the hookIdPrefix
- Preserves user hooks and hooks from other AIPM-managed plugins
- Includes proper type guards at line 198 (
typeof hook['x-hookId'] === 'string') before calling string methods, addressing the past review commentThe logic integrates well with the hooks merger utilities and only executes when
removeFiles=true.src/commands/sync.ts (5)
9-9: LGTM!The new imports (mergeHooks, CursorHooksConfig, IntegrationConfigSchema) are appropriate for the hooks collection and merging functionality.
Also applies to: 19-19
71-72: LGTM!Early initialization of
collectedPluginHooksis appropriate to enable hook cleanup even when no plugins are enabled. The comment clearly explains the intent.
74-80: LGTM!The logic correctly handles the case when no plugins are enabled by still calling
mergeHooksto clean up hooks from previously enabled but now disabled/uninstalled plugins. The dry-run guard is properly applied.
177-183: Hooks collection now properly respects the enabled config.The code correctly checks whether hooks are enabled (line 179) before collecting
translatedHooksintocollectedPluginHooks. This addresses the past review comment concern about hooks being merged even when disabled in config.
203-207: LGTM!Always calling
mergeHooksafter processing plugins (even with an empty array) ensures proper cleanup of hooks from disabled or uninstalled plugins. The dry-run guard and comments are appropriate.tests/commands/plugin-uninstall.test.ts (5)
6-7: LGTM!The new imports (FILE_HOOKS_JSON, CursorHooksConfig, fileExists, readJsonFile) are necessary for implementing comprehensive hooks cleanup tests.
Also applies to: 9-9
644-714: LGTM!The test comprehensively verifies hook cleanup behavior when
removeFiles=true, including:
- Removal of hooks from the uninstalled plugin
- Preservation of hooks from other plugins
- Preservation of user-defined hooks
The use of type assertions (line 712) for user hooks is appropriate since they don't conform to the strict AIPM schema.
716-759: LGTM!The test correctly verifies that
hooks.jsonis not modified whenremoveFiles=false, confirming that hook cleanup is properly guarded by the removeFiles flag.
761-788: LGTM!The test properly verifies graceful handling when
hooks.jsondoesn't exist, ensuring the uninstall operation doesn't fail or create an unnecessary file.
790-864: LGTM!The test thoroughly validates robustness against malformed
hooks.jsondata with non-stringx-hookIdvalues. It confirms:
- The type guard correctly preserves malformed hooks (null, number types)
- Valid plugin hooks with string
x-hookIdare still removed- No TypeError is thrown despite malformed data
This provides excellent coverage for edge cases.
src/schema.ts (3)
157-177: LGTM!The
ClaudeCodeHookSchemaaccurately models the Claude Code hooks.json format with proper nesting and optional fields. The schema structure aligns well with the translator implementation.
179-186: LGTM!The
CursorHookSchemaproperly defines AIPM-managed hooks with strict validation usingz.literal('aipm')for thex-managedByfield. This ensures type safety and consistency across the hooks management system.
188-198: LGTM!The
CursorHooksConfigSchemacorrectly models the Cursor hooks.json structure with version enforcement (z.literal(1)) and proper record mapping for event-based hooks. The type exports follow the established pattern and provide good type safety.src/helpers/hooks-translator.ts (4)
1-3: LGTM!The imports are appropriate and necessary for the translation functionality, including the constant for variable replacement, types for validation, and IO for logging.
5-14: LGTM!The event mapping from Claude Code to Cursor hooks is reasonable. The comment on line 11 appropriately notes that
PostToolUse→afterShellExecutionis an approximation, acknowledging the semantic gap between the two event systems.
25-92: LGTM!The
translateClaudeCodeHookfunction correctly translates Claude Code hooks to Cursor format with:
- Proper event mapping via EVENT_MAP
- Graceful handling of unknown events with warnings
- Unique hook ID generation following the
aipm/{marketplace}/{plugin}/{name}pattern- Safe CLAUDE_PLUGIN_ROOT resolution using regex with proper escaping (line 75)
- Null checks preventing runtime errors
The implementation is robust and well-structured.
94-103: LGTM!The
countTranslatedHooksutility function is simple and correct, summing the lengths of all hook arrays in the configuration.tests/helpers/hooks-translator.test.ts (1)
1-272: Excellent test coverage!The test suite comprehensively covers:
- Event translation (SessionStart, Stop, UserPromptSubmit)
- Nested hooks array handling
- CLAUDE_PLUGIN_ROOT variable resolution
- Hook ID generation and format
- Optional fields handling
- Matcher field behavior
- Hook counting utility
The tests are well-structured with proper setup/teardown and realistic test data.
src/helpers/hooks-merger.ts (4)
1-5: LGTM!The imports are appropriate for the hooks merging functionality, including path utilities, constants, types, file system helpers, and IO for logging.
13-38: LGTM!The
readExistingHooksfunction appropriately:
- Reads hooks.json without strict schema validation to allow user-defined hooks
- Performs basic structural validation (version and hooks fields)
- Returns null gracefully for missing or invalid files with informative logging
- Documents the non-strict validation approach clearly (line 21)
This design correctly balances validation with flexibility for user hooks.
47-81: LGTM!The
preserveUserHooksfunction correctly implements the preserve-then-merge strategy:
- First preserves user hooks (without
x-managedBy: 'aipm')- Then merges all AIPM-managed hooks from enabled plugins
- Properly initializes and builds the result structure
- Uses appropriate filtering and spreading operations
This ensures user hooks are never lost during sync operations.
89-100: LGTM!The
mergeHooksfunction correctly orchestrates the merge workflow:
- Reads existing hooks (which may include user hooks)
- Merges using
preserveUserHooksto maintain user hooks- Writes the result without strict schema validation (line 99), appropriately documented since user hooks may not conform to the AIPM schema
The implementation ensures data integrity while allowing user customization.
tests/commands/sync.test.ts (3)
9-11: LGTM!The new imports for
FILE_HOOKS_JSON,readJsonFile,fileExists, andCursorHooksConfigare correctly sourced and align with the test requirements for validating hooks translation and cleanup.
511-598: LGTM! Comprehensive test for hooks translation.The test thoroughly validates:
- Hooks structure with version and event mappings
x-managedByandx-hookIdmetadata- Resolution of
${CLAUDE_PLUGIN_ROOT}to absolute paths- Hook scripts remain in global plugin location (not copied locally)
The path extraction regex on line 585 works for this controlled test scenario. For broader robustness in production code, consider more sophisticated path parsing if needed.
600-711: LGTM! Good coverage of hooks cleanup scenarios.The tests properly validate:
- Hooks are removed when a plugin is disabled (lines 600-676)
- All AIPM hooks are cleaned up when no plugins are enabled (lines 678-711)
The conditional checks for
beforeSubmitPromptbeing undefined (lines 667-675, 704-710) correctly handle the edge case where the entire hook array is removed.tests/helpers/hooks-merger.test.ts (4)
1-23: LGTM!Test setup follows best practices with proper cleanup in
afterEach. The temp directory pattern ensures test isolation.
62-100: LGTM! Important edge case coverage.The
as anytype assertion on line 70 is necessary sinceCursorHookSchemarequiresx-managedBy, but real user hooks may omit this field. The comment explains this well. This test validates that the merger correctly preserves such hooks, which is critical for not breaking user configurations.
141-164: LGTM!Good test pattern for simulating plugin disabling by merging an empty array. This validates that previously added hooks are correctly removed when no plugins contribute hooks.
249-261: LGTM! Good error handling validation.Testing that
readExistingHooksreturnsnullfor missing files (line 251) and invalid JSON (line 260) ensures graceful degradation without throwing exceptions that could break the sync flow.src/helpers/sync-strategy.ts (2)
1-10: LGTM!Imports are correctly updated to include the new hooks translation types and helpers.
13-21: LGTM!Adding
translatedHooks?: CursorHooksConfig | nulltoAipmPluginSyncResultis a clean way to surface translation results for downstream merging without breaking existing consumers.
yordis
force-pushed
the
yordis/feat-2
branch
from
6a39b1d to
2d74873
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/commands/sync.test.ts (1)
582-588: Path extraction regex could be more robust.The regex pattern used to extract and validate absolute paths works for common cases but could be fragile with unusual path formats or edge cases (e.g., paths with quotes, spaces, or special characters).
Consider a more robust approach for test validation:
- // Extract the path from the command and verify it's absolute - const pathMatch = command.match(/(\/[^\s"]+|"[^"]+")/); - expect(pathMatch).toBeTruthy(); - const extractedPath = pathMatch![0].replace(/^"|"$/g, ''); // Remove quotes if present - expect(extractedPath).toMatch(/^\/|^[A-Z]:/); // Should be absolute path (Unix or Windows) + // Verify the command contains the absolute plugin path + expect(command).toContain(pluginPath); + // Verify path is absolute by checking it doesn't start with relative indicators + expect(command).not.toMatch(/\.\//); + expect(command).not.toMatch(/\.\.\//);src/helpers/sync-strategy.ts (1)
165-265: Consider extracting duplicated cleanup logic.The cleanup code that removes
hooks.jsonand empty directories is repeated identically in all three branches (translation success, translation failure, no hooks.json). This duplication increases maintenance burden and the risk of inconsistency if changes are needed.Consider extracting the cleanup logic into a helper function:
/** * Cleanup after syncing hooks: remove hooks.json and empty directories */ async function cleanupHooksDirectory(targetHooksDir: string): Promise<void> { // Remove hooks.json if it exists (we don't need the original Claude Code format) const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); if (await fileExists(copiedHooksJson)) { await rm(copiedHooksJson); } // Remove empty hooks directory if it only contained hooks.json try { const remainingFiles = await readdir(targetHooksDir); if (remainingFiles.length === 0) { await rm(targetHooksDir, { recursive: true }); } } catch { // Directory doesn't exist or can't be read - ignore } }Then use it in all three branches:
const targetHooksDir = join(cursorDir, 'hooks', DIR_AIPM_NAMESPACE, marketplaceName, pluginName); const count = await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); await cleanupHooksDirectory(targetHooksDir);This would reduce duplication from ~45 lines to ~15 lines and make future maintenance easier.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(1 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(1 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/helpers/hooks-merger.ts
- src/schema.ts
- tests/helpers/hooks-translator.test.ts
- src/helpers/hooks-translator.ts
- src/commands/sync.ts
- src/commands/plugin-uninstall.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/constants.tssrc/helpers/sync-strategy.tstests/commands/sync.test.tstests/commands/plugin-uninstall.test.tstests/helpers/hooks-merger.test.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/constants.tssrc/helpers/sync-strategy.tstests/commands/sync.test.tstests/commands/plugin-uninstall.test.tstests/helpers/hooks-merger.test.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/constants.tssrc/helpers/sync-strategy.tstests/commands/sync.test.tstests/commands/plugin-uninstall.test.tstests/helpers/hooks-merger.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/constants.tssrc/helpers/sync-strategy.tstests/commands/sync.test.tstests/commands/plugin-uninstall.test.tstests/helpers/hooks-merger.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/sync.test.tstests/commands/plugin-uninstall.test.tstests/helpers/hooks-merger.test.ts
🧬 Code graph analysis (4)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(198-198)ClaudeCodeHookSchema(160-177)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-92)countTranslatedHooks(97-103)src/helpers/io.ts (1)
defaultIO(79-79)
tests/commands/sync.test.ts (3)
src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/schema.ts (1)
CursorHooksConfig(198-198)
tests/commands/plugin-uninstall.test.ts (4)
src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/commands/plugin-uninstall.ts (1)
pluginUninstall(36-226)src/helpers/fs.ts (2)
readJsonFile(51-64)fileExists(26-33)src/schema.ts (1)
CursorHooksConfig(198-198)
tests/helpers/hooks-merger.test.ts (4)
src/schema.ts (1)
CursorHooksConfig(198-198)src/helpers/hooks-merger.ts (3)
mergeHooks(93-104)preserveUserHooks(47-85)readExistingHooks(13-38)src/helpers/fs.ts (2)
readJsonFile(51-64)fileExists(26-33)src/constants.ts (1)
FILE_HOOKS_JSON(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/constants.ts (1)
54-59: LGTM!The new hook-related constants are well-named, properly documented, and follow the existing code conventions. They provide clear references for the hooks management functionality introduced in this PR.
tests/commands/plugin-uninstall.test.ts (1)
644-865: Excellent test coverage for hooks cleanup!The test suite comprehensively validates the hooks cleanup behavior during plugin uninstallation, including:
- Selective removal of plugin hooks while preserving other plugins' hooks and user hooks
- Conditional cleanup based on the
removeFilesflag- Graceful handling of missing or malformed
hooks.jsonfilesThe edge case testing for malformed data (null/non-string
x-hookIdvalues) is particularly thorough and demonstrates defensive coding practices.tests/commands/sync.test.ts (1)
511-711: Comprehensive hooks translation and cleanup tests!The test suite thoroughly validates:
- Translation from Claude Code hooks format to Cursor format, including path resolution and metadata generation
- Cleanup behavior when plugins are disabled
- Cleanup when no plugins are enabled
The tests correctly verify that hook scripts remain in the global plugin location rather than being copied to Cursor-local directories.
tests/helpers/hooks-merger.test.ts (1)
1-391: Excellent comprehensive test coverage!This test suite provides thorough validation of the hooks merger functionality, including:
- Merging hooks from multiple plugins with proper version and metadata preservation
- Preserving user hooks (both with and without
x-managedByfield)- Removing disabled plugin hooks while keeping others intact
- Robust handling of missing files and malformed data (nulls, primitives, invalid JSON)
The defensive testing for corrupted data demonstrates production-ready error handling.
src/helpers/sync-strategy.ts (2)
1-21: LGTM!The new imports support the hooks translation functionality, and the addition of the optional
translatedHooksfield toAipmPluginSyncResultis backward compatible and properly typed.
85-88: LGTM!The integration of the new
syncHooksfunction into the main sync flow is clean and straightforward, properly populating both thehooksCountandtranslatedHooksfields in the result.
yordis
force-pushed
the
yordis/feat-2
branch
from
2d74873 to
69a2c11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/helpers/sync-strategy.ts (2)
165-265: Consider extracting duplicated cleanup logic.The cleanup code for removing
hooks.jsonand empty directories is repeated three times (lines 193-207, 220-234, 245-259). Extracting this into a helper function would reduce duplication and improve maintainability.+async function cleanupHooksDirectory(targetHooksDir: string): Promise<void> { + const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); + if (await fileExists(copiedHooksJson)) { + await rm(copiedHooksJson); + } + try { + const remainingFiles = await readdir(targetHooksDir); + if (remainingFiles.length === 0) { + await rm(targetHooksDir, { recursive: true }); + } + } catch { + // Directory doesn't exist or can't be read - ignore + } +}Then replace each cleanup block with a single call to
await cleanupHooksDirectory(targetHooksDir);
212-216: Consider extracting error message for cleaner logging.Using
${error}directly in the template literal may produce[object Object]for some error types or include the full stack trace. Consider using a utility likegetErrorMessage(error)(already imported in other files) for consistent error formatting.src/helpers/hooks-merger.ts (1)
24-28: Comment says "must have version and hooks" but only checks forhooks.The validation comment mentions checking for both
versionandhooks, but line 25 only validatesrawData.hooks. Consider addingtypeof rawData.version !== 'number'to the condition for consistency with the comment, or update the comment to reflect that onlyhooksis required.- // Basic validation - must have version and hooks - if (!rawData || typeof rawData !== 'object' || !rawData.hooks) { + // Basic validation - must have hooks object (version is optional, defaults to 1 if missing) + if (!rawData || typeof rawData !== 'object' || !rawData.hooks) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(1 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(1 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/helpers/hooks-merger.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/schema.ts
- src/helpers/hooks-translator.ts
- tests/helpers/hooks-translator.test.ts
- src/commands/sync.ts
- tests/commands/plugin-uninstall.test.ts
- tests/commands/sync.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/constants.tssrc/commands/plugin-uninstall.tssrc/helpers/hooks-merger.tssrc/helpers/sync-strategy.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/constants.tssrc/commands/plugin-uninstall.tssrc/helpers/hooks-merger.tssrc/helpers/sync-strategy.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/constants.tssrc/commands/plugin-uninstall.tssrc/helpers/hooks-merger.tssrc/helpers/sync-strategy.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/constants.tssrc/commands/plugin-uninstall.tssrc/helpers/hooks-merger.tssrc/helpers/sync-strategy.ts
🧬 Code graph analysis (2)
src/helpers/hooks-merger.ts (4)
src/schema.ts (1)
CursorHooksConfig(198-198)src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (3)
fileExists(26-33)readJsonFile(51-64)writeJsonFile(35-49)src/helpers/io.ts (1)
defaultIO(79-79)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(198-198)ClaudeCodeHookSchema(160-177)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-90)countTranslatedHooks(95-101)src/helpers/io.ts (1)
defaultIO(79-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/commands/plugin-uninstall.ts (2)
192-208: Type guards properly implemented.The filtering logic now correctly guards against malformed hook entries:
- Non-object entries (null, primitives, arrays) are filtered out at lines 194-196
- The
typeof hook['x-hookId'] === 'string'check at line 202 preventsTypeErrorwhen callingstartsWith()This addresses the concerns from the previous review.
176-218: Hooks cleanup only runs whenremoveFilesis true.The hooks cleanup logic is nested inside the
if (cmd.removeFiles)block, meaning hooks for the uninstalled plugin will persist inhooks.jsonif the user runsplugin-uninstallwithout--remove-files. This may be intentional (keeping hooks as "orphaned" until files are removed), but consider whether hooks should be cleaned regardless ofremoveFilessince the plugin config is being removed.src/constants.ts (1)
54-59: LGTM!The new constants are well-organized under the "Hook-related constants" section and follow the existing naming conventions. Centralizing
FILE_HOOKS_JSONprevents magic strings across the codebase.src/helpers/sync-strategy.ts (1)
189-211: Clarify comment vs. code behavior for script copying.The comments at lines 209, 236, and 261 state "Scripts stay in global plugin location - don't copy them," but
syncDirectorywith an empty extensions array copies all files from the hooks directory totargetHooksDir. If scripts should not be copied, consider filtering them out. If the current behavior is intentional, update the comments to reflect that scripts are copied alongside the translation.src/helpers/hooks-merger.ts (2)
59-66: Type guards properly implemented.The filtering logic at lines 60-63 correctly guards against non-object hook entries before using the
inoperator, addressing the concerns from the previous review.
47-85: LGTM!The
preserveUserHooksfunction correctly:
- Filters out malformed hook entries
- Preserves user hooks (without
x-managedBy: "aipm")- Merges AIPM hooks from multiple plugins
The defensive spread pattern at line 69 ensures safety even if event names somehow appeared multiple times.
yordis
force-pushed
the
yordis/feat-2
branch
from
69a2c11 to
253ae85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/helpers/sync-strategy.ts (1)
165-265: Consider extracting duplicate cleanup logic.The three code paths (translation success, fallback, no hooks.json) all contain identical cleanup logic for removing
hooks.jsonand empty directories (lines 194-207, 221-234, 246-259). Extracting this into a helper function would improve maintainability and reduce duplication.Example refactor:
async function cleanupHooksDirectory(targetHooksDir: string): Promise<void> { // Remove hooks.json if it exists const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); if (await fileExists(copiedHooksJson)) { await rm(copiedHooksJson); } // Remove empty hooks directory if it only contained hooks.json try { const remainingFiles = await readdir(targetHooksDir); if (remainingFiles.length === 0) { await rm(targetHooksDir, { recursive: true }); } } catch { // Directory doesn't exist or can't be read - ignore } }Then call
await cleanupHooksDirectory(targetHooksDir);in each path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
-
src/commands/plugin-uninstall.ts(2 hunks) -
src/commands/sync.ts(5 hunks) -
src/constants.ts(1 hunks) -
src/helpers/hooks-merger.ts(1 hunks) -
src/helpers/hooks-translator.ts(1 hunks) -
src/helpers/sync-strategy.ts(4 hunks) -
src/schema.ts(1 hunks) -
tests/commands/plugin-uninstall.test.ts(2 hunks) -
tests/commands/sync.test.ts(2 hunks) -
tests/helpers/hooks-merger.test.ts(1 hunks) -
tests/helpers/hooks-translator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/schema.ts
- src/helpers/hooks-translator.ts
- src/constants.ts
- tests/helpers/hooks-merger.test.ts
- tests/commands/sync.test.ts
- tests/commands/plugin-uninstall.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/sync.tssrc/helpers/hooks-merger.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-translator.test.tssrc/helpers/sync-strategy.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/sync.tssrc/helpers/hooks-merger.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-translator.test.tssrc/helpers/sync-strategy.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/sync.tssrc/helpers/hooks-merger.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-translator.test.tssrc/helpers/sync-strategy.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/sync.tssrc/helpers/hooks-merger.tssrc/commands/plugin-uninstall.tstests/helpers/hooks-translator.test.tssrc/helpers/sync-strategy.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/helpers/hooks-translator.test.ts
🧬 Code graph analysis (5)
src/commands/sync.ts (3)
src/schema.ts (1)
CursorHooksConfig(198-198)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(107-118)
src/helpers/hooks-merger.ts (4)
src/schema.ts (1)
CursorHooksConfig(198-198)src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (3)
fileExists(26-33)readJsonFile(51-64)writeJsonFile(35-49)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/plugin-uninstall.ts (3)
src/constants.ts (2)
DIR_CURSOR(4-4)FILE_HOOKS_JSON(59-59)src/helpers/hooks-merger.ts (1)
readExistingHooks(13-52)src/helpers/fs.ts (1)
writeJsonFile(35-49)
tests/helpers/hooks-translator.test.ts (2)
src/schema.ts (1)
ClaudeCodeHook(196-196)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-90)countTranslatedHooks(95-101)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(198-198)ClaudeCodeHookSchema(160-177)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-90)countTranslatedHooks(95-101)src/helpers/io.ts (1)
defaultIO(79-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
tests/helpers/hooks-translator.test.ts (1)
1-344: LGTM! Excellent test coverage for hook translation logic.The test suite comprehensively covers:
- Event mapping (SessionStart → beforeSubmitPrompt, Stop → stop)
- CLAUDE_PLUGIN_ROOT variable replacement including multiple occurrences
- Special replacement characters (
$1, $ &, $$) that could corrupt paths- Nested hook arrays and hook ID generation
- Edge cases like optional fields and ignored matchers
The tests for special replacement characters (lines 307-343) are particularly valuable for preventing path corruption bugs.
src/helpers/sync-strategy.ts (1)
20-20: LGTM! Type augmentation correctly supports translated hooks.The addition of
translatedHooks?: CursorHooksConfig | nulltoAipmPluginSyncResultproperly extends the type to support the new hook translation feature. The integration at lines 85-88 correctly destructures and assigns both the count and translated hooks fromsyncHooks.Also applies to: 85-88
src/commands/sync.ts (1)
71-79: LGTM! Hook collection correctly respects the include config.The code properly addresses the previous review concern about hooks being merged when disabled. Lines 179-182 now check if hooks are enabled via
includeConfigbefore collectingtranslatedHooks, ensuring that disabled hooks are not added to the merge.The
mergeHookscalls at lines 78 and 206 correctly execute even with an empty array to clean up hooks from disabled or uninstalled plugins.Also applies to: 177-183, 203-207
src/helpers/hooks-merger.ts (3)
13-52: LGTM! Validation properly addresses past review concerns.The validation logic correctly addresses the previous review comments:
- Lines 30-34 ensure
hooksis an object and not an array, preventing TypeError when calling.filter()later- Lines 36-42 validate that all hook event values are arrays, preventing runtime errors
The non-strict parsing approach (line 22) appropriately allows user-defined hooks while still validating the core structure.
61-99: LGTM! Type guards properly prevent TypeError on malformed hooks.The type guards at lines 74-76 correctly address the previous review comment about potential TypeError when hook entries are non-object values. The guard checks for
null,undefined, primitives, and arrays before using theinoperator at line 79.The merge logic properly:
- Filters out malformed entries
- Preserves user hooks (without
x-managedBy: "aipm")- Appends all AIPM-managed hooks from enabled plugins
107-118: LGTM! Merge orchestration is clean and correct.The
mergeHooksfunction properly orchestrates the merge workflow:
- Reads existing hooks (handling missing/invalid gracefully via
readExistingHooks)- Merges user and AIPM hooks via
preserveUserHooks- Writes without strict validation to accommodate user hooks that lack AIPM metadata
The approach correctly balances validation for safety while allowing flexibility for user-defined hooks.
yordis
force-pushed
the
yordis/feat-2
branch
2 times, most recently
from
d8d607a to
9c87e56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/helpers/sync-strategy.ts (1)
214-239: Same misleading comment appears in fallback paths.The comments at lines 236 and 261 have the same issue as line 209: they claim scripts aren't copied, but
syncDirectorycopies all files. This creates unused script duplicates in the target directory across all three code paths (translation success, failure, and no hooks.json).Consider either filtering scripts during copy or updating the comments for consistency.
Also applies to: 241-264
src/commands/plugin-uninstall.ts (1)
218-220: Respect dry-run flag when writing hooks.json.The
writeJsonFilecall should respect thedryRunflag for consistency with the rest of the uninstall flow. This was flagged in previous reviews but remains unaddressed.Apply this diff to add dry-run support:
- await writeJsonFile(hooksPath, cleanedHooks); + await writeJsonFile(hooksPath, cleanedHooks, undefined, cmd.dryRun);
🧹 Nitpick comments (2)
src/helpers/sync-strategy.ts (2)
189-211: Misleading comment: scripts ARE being copied.The comment at line 209 states "Scripts stay in global plugin location - don't copy them", but the
syncDirectorycall at line 191 with an empty extensions array copies all files, including scripts. The translated hooks correctly reference the global plugin location (viaCLAUDE_PLUGIN_ROOT_VARreplacement), so the copied scripts in.cursor/hooks/aipm/marketplace/plugin/are unused duplicates.If the intent is to not copy scripts, you should either:
- Pass specific extensions to
syncDirectoryto exclude scripts, or- Update the comment to reflect that scripts are copied but not referenced by translated hooks
Consider applying this diff to filter out common script extensions:
- await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); + // Copy non-script files only (scripts stay in global plugin location) + const scriptExtensions = ['.sh', '.bash', '.js', '.ts', '.py', '.rb']; + const entries = await readdir(join(pluginPath, 'hooks'), { withFileTypes: true }); + for (const entry of entries) { + if (entry.isFile() && !scriptExtensions.some(ext => entry.name.endsWith(ext))) { + await cp(join(pluginPath, 'hooks', entry.name), join(targetHooksDir, entry.name)); + } + }Alternatively, update the comment to clarify that scripts are copied but unused.
165-265: Consider extracting common cleanup logic.All three code paths (translation success, failure, and no hooks.json) share similar cleanup logic:
- Remove hooks.json from target
- Remove empty hooks directory
This could be extracted to a helper function to reduce duplication, though the current structure is clear enough.
Example helper function:
async function cleanupHooksTarget(targetHooksDir: string): Promise<void> { // Remove hooks.json (we don't need it in target) const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); if (await fileExists(copiedHooksJson)) { await rm(copiedHooksJson); } // Remove empty hooks directory try { const remainingFiles = await readdir(targetHooksDir); if (remainingFiles.length === 0) { await rm(targetHooksDir, { recursive: true }); } } catch { // Directory doesn't exist or can't be read - ignore } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(1 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(2 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/helpers/hooks-merger.test.ts
- src/helpers/hooks-translator.ts
- tests/commands/sync.test.ts
- tests/helpers/hooks-translator.test.ts
- src/helpers/hooks-merger.ts
- src/schema.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/sync.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/plugin-uninstall.test.tssrc/constants.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/sync.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/plugin-uninstall.test.tssrc/constants.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/sync.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/plugin-uninstall.test.tssrc/constants.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/sync.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/plugin-uninstall.test.tssrc/constants.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/plugin-uninstall.test.ts
🧬 Code graph analysis (4)
src/commands/sync.ts (2)
src/schema.ts (1)
CursorHooksConfig(199-199)src/helpers/hooks-merger.ts (1)
mergeHooks(107-118)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(199-199)ClaudeCodeHookSchema(161-178)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-90)countTranslatedHooks(95-101)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/plugin-uninstall.ts (3)
src/constants.ts (5)
DIR_CURSOR(4-4)AIPM_HOOK_PREFIX(60-60)HOOK_MANAGED_BY_FIELD(61-61)HOOK_ID_FIELD(62-62)FILE_HOOKS_JSON(59-59)src/helpers/hooks-merger.ts (1)
readExistingHooks(13-52)src/helpers/fs.ts (1)
writeJsonFile(35-49)
tests/commands/plugin-uninstall.test.ts (3)
src/constants.ts (1)
FILE_HOOKS_JSON(59-59)src/helpers/fs.ts (2)
readJsonFile(51-64)fileExists(26-33)src/schema.ts (1)
CursorHooksConfig(199-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
src/constants.ts (1)
54-62: LGTM! Well-organized hook constants.The new constants are clearly documented and follow consistent naming patterns. The grouping under a dedicated comment section improves code organization.
src/commands/plugin-uninstall.ts (3)
6-6: LGTM! Imports support the new hooks cleanup flow.All new imports are properly utilized in the hooks cleanup logic added below.
Also applies to: 12-14, 19-19, 21-21
196-207: LGTM! Type guards properly address past review concerns.The type guards at lines 196-199 (checking for non-object entries) and lines 205-206 (verifying
x-hookIdis a string) correctly prevent TypeError when processing malformed hooks. This addresses the previous review comment about missing type guards.
180-217: Well-structured hooks cleanup logic.The implementation correctly:
- Constructs plugin-specific hook ID prefix for filtering
- Preserves user hooks and hooks from other plugins
- Removes empty hook event arrays from the output
- Handles edge cases with appropriate type guards
tests/commands/plugin-uninstall.test.ts (2)
6-9: LGTM! Imports support comprehensive hooks testing.The imports enable thorough testing of the hooks cleanup functionality with proper typing.
644-865: Excellent test coverage for hooks cleanup!The test suite comprehensively validates:
- Correct removal of plugin-specific hooks while preserving others
- Respecting the
removeFilesflag- Graceful handling of missing hooks.json
- Robust handling of malformed data (non-string
x-hookIdvalues)The malformed data test at lines 790-864 is particularly valuable, as it confirms the type guards prevent runtime errors while preserving invalid entries (rather than crashing).
src/commands/sync.ts (4)
9-9: LGTM! Imports enable hooks merging flow.The new imports support the centralized hooks management introduced in this PR.
Also applies to: 19-19
71-72: Good design: early initialization enables cleanup of disabled plugins.Initializing
collectedPluginHooksbefore the plugin loop allowsmergeHooksto clean up hooks from disabled or uninstalled plugins, even when no plugins are currently enabled.
177-183: LGTM! Properly respects hooks enabled/disabled configuration.The
hooksEnabledcheck at lines 179-180 correctly verifies whether hooks are enabled in the integration config before collecting translated hooks. This addresses the previous review concern about hooks being merged even when disabled.
203-207: LGTM! Final hooks merge ensures cleanup.Always calling
mergeHooksafter processing all plugins (even with an empty array) correctly ensures hooks from disabled or uninstalled plugins are removed. The dry-run check is appropriate.src/helpers/sync-strategy.ts (3)
1-11: LGTM! Imports support hooks translation flow.All new imports are properly utilized in the hooks synchronization and translation logic.
20-20: LGTM! Field enables hooks collection for later merging.The optional
translatedHooksfield allows the sync result to communicate translated hooks back to the caller, enabling centralized hooks management in the sync command.
85-88: LGTM! Clean integration of hooks translation.The
syncHookscall and result propagation cleanly integrates hooks translation into the overall plugin sync flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/helpers/sync-strategy.ts (1)
174-265: Refactor to eliminate code duplication.The three code paths (translate success, translate failure, no hooks.json) have nearly identical cleanup logic duplicated at Lines 189-210, 217-237, and 242-262. The repeated blocks include:
syncDirectorycall- Remove
hooks.jsonif exists- Remove empty directory cleanup
- Comment about scripts
Consider extracting this cleanup into a helper function to reduce duplication and improve maintainability.
Apply this refactor:
+async function cleanupHooksDirectory( + pluginPath: string, + targetHooksDir: string, + marketplaceName: string, + pluginName: string, +): Promise<number> { + // Copy the hooks directory files to the target location (excluding hooks.json since it's translated and merged) + const count = await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); + + // Remove the hooks.json since it's been translated/processed + const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); + if (await fileExists(copiedHooksJson)) { + await rm(copiedHooksJson); + } + + // Remove empty hooks directory if it only contained hooks.json + try { + const remainingFiles = await readdir(targetHooksDir); + if (remainingFiles.length === 0) { + await rm(targetHooksDir, { recursive: true }); + } + } catch { + // Directory doesn't exist or can't be read - ignore + } + + // Scripts stay in global plugin location (pluginPath/scripts/), not in hooks/ subdirectory + return count; +} + async function syncHooks( pluginPath: string, marketplaceName: string, pluginName: string, cursorDir: string, ): Promise<{ count: number; translatedHooks: CursorHooksConfig | null }> { const hooksJsonPath = join(pluginPath, 'hooks', FILE_HOOKS_JSON); + const targetHooksDir = join(cursorDir, 'hooks', DIR_AIPM_NAMESPACE, marketplaceName, pluginName); if (await fileExists(hooksJsonPath)) { try { // Translate Claude Code hooks const claudeHook = await readJsonFile(hooksJsonPath, ClaudeCodeHookSchema); const translated = translateClaudeCodeHook(claudeHook, marketplaceName, pluginName, pluginPath); - const count = countTranslatedHooks(translated); - - // Copy the hooks directory files to the target location (excluding hooks.json since it's translated and merged) - const targetHooksDir = join(cursorDir, 'hooks', DIR_AIPM_NAMESPACE, marketplaceName, pluginName); - await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); - - // Remove the original hooks.json since it's been translated and merged into .cursor/hooks.json - const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); - if (await fileExists(copiedHooksJson)) { - await rm(copiedHooksJson); - } - - // Remove empty hooks directory if it only contained hooks.json - try { - const remainingFiles = await readdir(targetHooksDir); - if (remainingFiles.length === 0) { - await rm(targetHooksDir, { recursive: true }); - } - } catch { - // Directory doesn't exist or can't be read - ignore - } - - // Scripts stay in global plugin location - don't copy them + const translatedCount = countTranslatedHooks(translated); + await cleanupHooksDirectory(pluginPath, targetHooksDir, marketplaceName, pluginName); - return { count, translatedHooks: translated }; + return { count: translatedCount, translatedHooks: translated }; } catch (error) { // If translation fails, fall back to copying hooks directory as-is defaultIO.logInfo( `⚠️ Failed to translate hooks.json for ${pluginName}@${marketplaceName}: ${error}. Copying as-is.`, ); - const targetHooksDir = join(cursorDir, 'hooks', DIR_AIPM_NAMESPACE, marketplaceName, pluginName); - const count = await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); - - // Remove hooks.json if it exists (we don't need the original Claude Code format) - const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); - if (await fileExists(copiedHooksJson)) { - await rm(copiedHooksJson); - } - - // Remove empty hooks directory if it only contained hooks.json - try { - const remainingFiles = await readdir(targetHooksDir); - if (remainingFiles.length === 0) { - await rm(targetHooksDir, { recursive: true }); - } - } catch { - // Directory doesn't exist or can't be read - ignore - } - - // Scripts stay in global plugin location - don't copy them + const count = await cleanupHooksDirectory(pluginPath, targetHooksDir, marketplaceName, pluginName); return { count, translatedHooks: null }; } } else { // No hooks.json, copy as-is (AIPM format or scripts) - const targetHooksDir = join(cursorDir, 'hooks', DIR_AIPM_NAMESPACE, marketplaceName, pluginName); - const count = await syncDirectory(join(pluginPath, 'hooks'), targetHooksDir, []); - - // Remove hooks.json if it exists (we don't need it if there's no translation) - const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); - if (await fileExists(copiedHooksJson)) { - await rm(copiedHooksJson); - } - - // Remove empty hooks directory if it only contained hooks.json - try { - const remainingFiles = await readdir(targetHooksDir); - if (remainingFiles.length === 0) { - await rm(targetHooksDir, { recursive: true }); - } - } catch { - // Directory doesn't exist or can't be read - ignore - } - - // Scripts stay in global plugin location - don't copy them + const count = await cleanupHooksDirectory(pluginPath, targetHooksDir, marketplaceName, pluginName); return { count, translatedHooks: null }; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(1 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(2 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/schema.ts
- src/helpers/hooks-translator.ts
- src/commands/plugin-uninstall.ts
- tests/commands/plugin-uninstall.test.ts
- tests/helpers/hooks-merger.test.ts
- tests/helpers/hooks-translator.test.ts
- src/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/helpers/sync-strategy.tssrc/commands/sync.tstests/commands/sync.test.tssrc/helpers/hooks-merger.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/helpers/sync-strategy.tssrc/commands/sync.tstests/commands/sync.test.tssrc/helpers/hooks-merger.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/helpers/sync-strategy.tssrc/commands/sync.tstests/commands/sync.test.tssrc/helpers/hooks-merger.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/helpers/sync-strategy.tssrc/commands/sync.tstests/commands/sync.test.tssrc/helpers/hooks-merger.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/sync.test.ts
🧬 Code graph analysis (3)
src/helpers/sync-strategy.ts (4)
src/schema.ts (2)
CursorHooksConfig(199-199)ClaudeCodeHookSchema(161-178)src/constants.ts (2)
FILE_HOOKS_JSON(59-59)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(25-90)countTranslatedHooks(95-101)
src/commands/sync.ts (2)
src/schema.ts (1)
CursorHooksConfig(199-199)src/helpers/hooks-merger.ts (1)
mergeHooks(107-118)
src/helpers/hooks-merger.ts (4)
src/schema.ts (1)
CursorHooksConfig(199-199)src/constants.ts (3)
FILE_HOOKS_JSON(59-59)HOOK_MANAGED_BY_FIELD(61-61)AIPM_HOOK_PREFIX(60-60)src/helpers/fs.ts (3)
fileExists(26-33)readJsonFile(51-64)writeJsonFile(35-49)src/helpers/io.ts (1)
defaultIO(79-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (10)
src/commands/sync.ts (3)
71-80: LGTM! Good cleanup strategy.Initializing
collectedPluginHooksearly and callingmergeHookseven with an empty array ensures proper cleanup of hooks from disabled/uninstalled plugins, even when no plugins are currently enabled.
177-183: Hooks collection correctly respects include config.The conditional logic properly checks if hooks are enabled before collecting
translatedHooks. This addresses the past review comment about hooks being merged even when disabled in config.
203-207: LGTM! Final merge step properly positioned.Calling
mergeHooksafter processing all plugins ensures that the final.cursor/hooks.jsonreflects only enabled plugins' hooks while preserving user hooks. The dry-run check is appropriate.tests/commands/sync.test.ts (3)
511-598: Comprehensive test coverage for Claude Code hooks translation.The test properly validates:
- Translation of Claude Code hooks format to Cursor format
- Metadata fields (
x-managedBy,x-hookId)- Variable substitution (
${CLAUDE_PLUGIN_ROOT})- Path absolutization
- Scripts remaining at plugin root (not copied to
.cursor/hooks/)
600-676: LGTM! Cleanup test properly validates hook removal.The test correctly verifies that disabling a plugin removes its hooks from
.cursor/hooks.jsonwhile preserving other hooks. The conditional check handles both scenarios (hooks array with filtered entries vs. completely empty/undefined).
678-711: Good edge case coverage for cleanup with no enabled plugins.The test validates that
mergeHooksis called even when no plugins are enabled (as implemented in Lines 76-79 of sync.ts), ensuring AIPM-managed hooks are removed while preserving any user hooks.src/helpers/sync-strategy.ts (1)
20-20: LGTM! Optional field properly typed.The optional
translatedHooksfield allows the result to distinguish between: translated hooks (object), failed translation (null), and pre-hooks-feature code (undefined).src/helpers/hooks-merger.ts (3)
13-52: LGTM! Robust validation addresses past review concerns.The validation at Lines 30-42 properly addresses the past review comment about missing validation. The function now:
- Validates
hooksis an object (not array)- Validates all hook event values are arrays
- Returns null for malformed structures, allowing a fresh hooks.json to be created
This prevents TypeErrors in
preserveUserHookswhen calling.filter()on hook arrays.
61-99: Defensive filtering addresses past review concerns.The guard at Lines 74-77 properly addresses the past review comment about TypeError with non-object values. The filter correctly checks for null, undefined, primitives, and arrays before using the
inoperator.The silent filtering of malformed entries is acceptable for resilience. If debugging assistance is desired, consider adding a debug log when malformed hooks are encountered, but this is optional.
107-118: LGTM! Clean orchestration with appropriate validation strategy.The function correctly orchestrates the merge flow. Skipping strict schema validation on write (Line 115-116) is appropriate since user hooks may not include AIPM-specific metadata fields like
x-managedBy.
yordis
force-pushed
the
yordis/feat-2
branch
3 times, most recently
from
92e3d91 to
fe1cadb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/schema.ts (1)
222-234: Type mismatch between schema and TypeScript type is intentional but warrants a brief note.The
CursorHooksConfigSchemaaccepts malformed entries viaCursorHookValidationSchema, but theCursorHooksConfigtype uses the strictCursorHook[]. This gap is bridged by filtering inpreserveUserHooks, but a reader might find the cast at line 24 ofhooks-merger.tsconfusing.Consider adding a brief inline comment in
readExistingHooksexplaining why the cast is safe (validation allows malformed, filtering removes them before use).src/helpers/sync-strategy.ts (1)
174-250: Significant code duplication in cleanup logic.The cleanup pattern (remove hooks.json, check empty dir, remove dir) is repeated three times (lines 191-204, 214-226, 234-246). Consider extracting a helper function to reduce duplication and maintenance burden.
+async function cleanupHooksDirectory(targetHooksDir: string): Promise<void> { + const copiedHooksJson = join(targetHooksDir, FILE_HOOKS_JSON); + if (await fileExists(copiedHooksJson)) { + await rm(copiedHooksJson); + } + + try { + const remainingFiles = await readdir(targetHooksDir); + if (remainingFiles.length === 0) { + await rm(targetHooksDir, { recursive: true }); + } + } catch { + // Ignore - directory doesn't exist or can't be read + } +}Then use it in each branch:
await cleanupHooksDirectory(targetHooksDir);src/constants.ts (1)
56-63: Hook constants are well-organized and actively used across the codebase.The new constants provide a single source of truth for hook-related magic strings and are referenced in
src/helpers/hooks-translator.tsandsrc/commands/plugin-uninstall.ts. However,src/schema.ts(lines 185-186) uses inline string literals'x-managedBy'and'x-hookId'instead of these constants, creating an inconsistency.Consider updating the schema for consistency:
// In src/schema.ts +import { AIPM_HOOK_PREFIX, HOOK_MANAGED_BY_FIELD, HOOK_ID_FIELD } from './constants'; export const AipmManagedHookSchema = z.object({ - 'x-managedBy': z.literal(AIPM_HOOK_PREFIX), - 'x-hookId': z.string(), + [HOOK_MANAGED_BY_FIELD]: z.literal(AIPM_HOOK_PREFIX), + [HOOK_ID_FIELD]: z.string(), command: z.string(), });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/commands/plugin-uninstall.ts(3 hunks)src/commands/sync.ts(5 hunks)src/constants.ts(2 hunks)src/helpers/hooks-merger.ts(1 hunks)src/helpers/hooks-translator.ts(1 hunks)src/helpers/sync-strategy.ts(4 hunks)src/schema.ts(2 hunks)tests/commands/plugin-uninstall.test.ts(2 hunks)tests/commands/sync.test.ts(2 hunks)tests/helpers/hooks-merger.test.ts(1 hunks)tests/helpers/hooks-translator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/helpers/hooks-translator.ts
- tests/helpers/hooks-translator.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/helpers/hooks-merger.tssrc/commands/sync.tstests/helpers/hooks-merger.test.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/sync.test.tssrc/schema.tssrc/constants.tstests/commands/plugin-uninstall.test.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/helpers/hooks-merger.tssrc/commands/sync.tstests/helpers/hooks-merger.test.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/sync.test.tssrc/schema.tssrc/constants.tstests/commands/plugin-uninstall.test.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/helpers/hooks-merger.tssrc/commands/sync.tstests/helpers/hooks-merger.test.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/sync.test.tssrc/schema.tssrc/constants.tstests/commands/plugin-uninstall.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/helpers/hooks-merger.tssrc/commands/sync.tstests/helpers/hooks-merger.test.tssrc/helpers/sync-strategy.tssrc/commands/plugin-uninstall.tstests/commands/sync.test.tssrc/schema.tssrc/constants.tstests/commands/plugin-uninstall.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/helpers/hooks-merger.test.tstests/commands/sync.test.tstests/commands/plugin-uninstall.test.ts
🧬 Code graph analysis (6)
src/helpers/hooks-merger.ts (4)
src/schema.ts (4)
CursorHooksConfig(231-234)CursorHooksConfigSchema(222-225)AipmManagedHookSchema(184-188)UserHookSchema(194-194)src/constants.ts (1)
FILE_HOOKS_JSON(60-60)src/helpers/fs.ts (3)
fileExists(26-33)readJsonFile(51-64)writeJsonFile(35-49)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/sync.ts (3)
src/schema.ts (1)
CursorHooksConfig(231-234)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(85-90)
tests/helpers/hooks-merger.test.ts (4)
src/schema.ts (1)
CursorHooksConfig(231-234)src/helpers/hooks-merger.ts (3)
mergeHooks(85-90)preserveUserHooks(38-77)readExistingHooks(14-29)src/helpers/fs.ts (2)
readJsonFile(51-64)fileExists(26-33)src/constants.ts (1)
FILE_HOOKS_JSON(60-60)
src/helpers/sync-strategy.ts (5)
src/schema.ts (2)
CursorHooksConfig(231-234)ClaudeCodeHookSchema(161-178)src/constants.ts (3)
DIR_HOOKS(21-21)FILE_HOOKS_JSON(60-60)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/helpers/hooks-translator.ts (2)
translateClaudeCodeHook(26-84)countTranslatedHooks(89-95)src/helpers/io.ts (1)
defaultIO(79-79)
tests/commands/sync.test.ts (3)
src/constants.ts (1)
FILE_HOOKS_JSON(60-60)src/helpers/fs.ts (2)
fileExists(26-33)readJsonFile(51-64)src/schema.ts (1)
CursorHooksConfig(231-234)
src/schema.ts (1)
src/constants.ts (1)
AIPM_HOOK_PREFIX(61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (18)
tests/commands/plugin-uninstall.test.ts (1)
643-913: LGTM! Comprehensive test coverage for hooks cleanup.The new test suite thoroughly covers:
- Hook removal when
removeFiles=true- Hook preservation when
removeFiles=false- Graceful handling of missing
hooks.json- Resilience against malformed
x-hookIdvalues (null, number)- Dry-run mode respecting hooks.json
The use of
anytype assertions for test fixtures with non-strict hook schemas is appropriate.src/commands/plugin-uninstall.ts (1)
179-211: Well-structured hooks cleanup with proper type safety.The implementation correctly addresses the previous review concerns:
- Type guards via
safeParse(lines 193-198) prevent TypeError on non-stringx-hookIdvalues- The
cmd.dryRunparameter at line 210 provides defense-in-depth, though this code path is only reachable whendryRun=falsedue to the branching at line 68The filtering logic cleanly separates AIPM-managed hooks (removed if matching the plugin prefix) from user hooks (preserved).
src/commands/sync.ts (2)
176-182: Past review concern properly addressed.The hook collection is now correctly gated by the
includeConfigsetting (line 178). Whenhooks: falseis set in the integration config, hooks won't be added tocollectedPluginHooks, ensuring they're cleaned up rather than persisted.
71-79: Good: Early initialization enables cleanup even with no enabled plugins.Initializing
collectedPluginHooksbefore the enabled plugins check (line 72) and callingmergeHooksin the early-return path (line 78) ensures proper cleanup of stale hooks when all plugins are disabled.tests/commands/sync.test.ts (2)
511-598: Thorough test for hooks translation workflow.The test comprehensively validates:
- Claude Code → Cursor format translation
${CLAUDE_PLUGIN_ROOT}replacement with absolute paths- Proper
x-managedByandx-hookIdassignment- Scripts remaining in the plugin location (not copied)
600-712: Good coverage for hooks cleanup scenarios.Both tests properly verify:
- Hooks are removed when a plugin is disabled and re-synced
- AIPM hooks are cleaned up when syncing with no enabled plugins
The conditional checks at lines 667-676 and 705-711 appropriately handle both cases where hooks might be undefined or an empty filtered array.
tests/helpers/hooks-merger.test.ts (3)
1-24: Well-structured test setup.The test harness properly creates and cleans up temporary directories for isolation. The
cleanupfunction pattern ensures proper teardown even if tests fail.
325-385: Excellent edge case coverage for malformed hook entries.This test verifies that
preserveUserHooksgracefully handles corrupted data (null, primitives, arrays) without throwing TypeError. The assertions confirm that:
- Malformed entries are filtered out
- Valid user hooks are preserved
- New AIPM hooks are added
- Old AIPM hooks are replaced
263-323: Good schema validation tests for hooks.json structure.These tests verify that
readExistingHooksproperly rejects invalid structures:
hooksas an array instead of object- Hook event values as strings/objects instead of arrays
- Mixed valid/invalid event values
This ensures the merger handles malformed files gracefully by returning null.
src/helpers/hooks-merger.ts (3)
14-29: LGTM! Resilient reading with graceful fallback.The function properly handles missing files and invalid JSON by returning
null, allowing callers to proceed with a clean slate. The try-catch around schema validation ensures malformed files don't crash the sync process.
85-90: Clean orchestration of the merge flow.The function correctly sequences read → preserve → write operations. One consideration:
writeJsonFilehere doesn't pass a schema for validation, which is intentional since the merged result may contain user hooks with arbitrary extra properties that wouldn't pass strict validation.
47-65: The edge case mentioned is not possible—schema validation is already in place upstream. TheCursorHooksConfigSchemaenforcesz.record(z.string(), z.array(...)), which meanshooks[eventName]is guaranteed to be an array. InreadExistingHooks, any JSON that violates this structure is rejected during validation (line 26), causing the function to returnnullrather than passing invalid data topreserveUserHooks. By the timeexistingHooks.hooksis accessed inpreserveUserHooks, the structure is already validated. The.filter()call cannot throw because the type system and schema both guarantee an array. No additional defensive validation is needed.Likely an incorrect or invalid review comment.
src/schema.ts (3)
201-209: Private validation schema serves resilience needs.The
CursorHookValidationSchemaunion allows parsing to succeed even with malformed entries (null, primitives, arrays), which are then filtered out inpreserveUserHooks. This is a sound design for handling user-edited files.Note: The schema is not exported (no
export), which correctly limits its use to internal validation only.
161-178: Schema structure looks correct for Claude Code hooks format.The nested structure (
hooks→ event → array of matchers → array of hook objects) matches typical Claude Code plugin hooks.json patterns.
184-194: AIPM and User hook schemas are well-defined.
AipmManagedHookSchemaenforces the required AIPM metadata fields, whileUserHookSchemawith.loose()(Zod 4 syntax) allows user hooks to have any additional properties beyond the requiredcommandfield.src/helpers/sync-strategy.ts (2)
206-228: Count semantics are now consistent.The past review flagged inconsistent
hooksCountsemantics. This has been addressed:
- Translation success: returns actual hook count via
countTranslatedHooks- Translation failure: returns
count: 0- No hooks.json: returns
count: 0This ensures
formatSyncResultdisplays accurate hook counts.
13-21: Good addition oftranslatedHooksto the result type.The optional
translatedHooksfield allows downstream code (sync.ts) to collect and merge hooks from multiple plugins, enabling the centralized hooks.json merge workflow.src/constants.ts (1)
4-9: Good addition ofas constfor type narrowing.Adding
as constto existing constants enables TypeScript to infer string literal types instead ofstring, improving type safety when these constants are used as discriminators or in template literal types.Also applies to: 21-21, 26-28, 33-33, 38-48, 53-54
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-2
branch
from
fe1cadb to
ce8ab82
Compare
1 participant
No description provided.