-
Notifications
You must be signed in to change notification settings - Fork 79
Description
Background
During the review of PR #1273 (Add Cursor MCP support for global mode), two medium-severity issues were identified. These are not blockers for the PR itself but represent technical debt that should be addressed to improve code quality and prevent potential bugs.
Details
1. DRY Violation: Duplicated JSON parsing with error handling (Medium)
File: src/features/mcp/cursor-mcp.ts
The same JSON parse-with-descriptive-error pattern is repeated three times in CursorMcp: in the constructor, in fromFile, and in fromRulesyncMcp. Each uses an identical try/catch block:
try {
json = JSON.parse(fileContent);
} catch (error) {
throw new Error(
`Failed to parse Cursor MCP config at ${join(paths.relativeDirPath, paths.relativeFilePath)}: ${formatError(error)}`,
{ cause: error },
);
}This should be extracted into a private static helper method to consolidate the error message format in one place and make future changes easier.
Note: The existing ClaudecodeMcp does not wrap its JSON.parse calls in try/catch at all (its constructor uses JSON.parse(this.fileContent || "{}") bare), so there is a divergence in error handling strategy between the two sibling classes. Ideally this should be resolved codebase-wide.
2. Pre-existing Bug: ClaudecodeMcp.fromFile does not pass global to constructor (Medium)
File: src/features/mcp/claudecode-mcp.ts
In ClaudecodeMcp.fromFile, the global flag is not passed to the constructor. Since isDeletable() depends on this.global, this means isDeletable() would always return true for instances created via ClaudecodeMcp.fromFile, even in global mode where it should return false.
The new CursorMcp.fromFile in PR #1273 correctly passes global to the constructor, highlighting the inconsistency in the existing ClaudecodeMcp implementation.
Solution / Next Steps
- Extract JSON parsing helper: Create a shared private static method (or a utility function) for JSON parsing with descriptive error messages, and use it across both
CursorMcpandClaudecodeMcp. - Fix
ClaudecodeMcp.fromFile: Pass theglobalparameter to theClaudecodeMcpconstructor in thefromFilemethod, matching the correct pattern used inCursorMcp.fromFile. - Harmonize error handling: Consider adding consistent JSON parse error handling to
ClaudecodeMcpas well, using the same shared helper.
Related PR: #1273