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
CursorMcp and ClaudecodeMcp.
- Fix
ClaudecodeMcp.fromFile: Pass the global parameter to the ClaudecodeMcp constructor in the fromFile method, matching the correct pattern used in CursorMcp.fromFile.
- Harmonize error handling: Consider adding consistent JSON parse error handling to
ClaudecodeMcp as well, using the same shared helper.
Related PR: #1273
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.tsThe same JSON parse-with-descriptive-error pattern is repeated three times in
CursorMcp: in the constructor, infromFile, and infromRulesyncMcp. Each uses an identical try/catch block: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
ClaudecodeMcpdoes not wrap itsJSON.parsecalls in try/catch at all (its constructor usesJSON.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.fromFiledoes not passglobalto constructor (Medium)File:
src/features/mcp/claudecode-mcp.tsIn
ClaudecodeMcp.fromFile, theglobalflag is not passed to the constructor. SinceisDeletable()depends onthis.global, this meansisDeletable()would always returntruefor instances created viaClaudecodeMcp.fromFile, even in global mode where it should returnfalse.The new
CursorMcp.fromFilein PR #1273 correctly passesglobalto the constructor, highlighting the inconsistency in the existingClaudecodeMcpimplementation.Solution / Next Steps
CursorMcpandClaudecodeMcp.ClaudecodeMcp.fromFile: Pass theglobalparameter to theClaudecodeMcpconstructor in thefromFilemethod, matching the correct pattern used inCursorMcp.fromFile.ClaudecodeMcpas well, using the same shared helper.Related PR: #1273