Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: short name for mcp server in cursor and bump version to 1.0.5 #1478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

utkarsh-dixit
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Mar 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 24, 2025 6:13am

@@ -199,7 +199,7 @@ function saveMcpConfig(url: string, clientType: string, mcpUrl: string, command:
}

try {
const newKey = url.split('/').slice(3).join('/').replace(/\//g, '-');
const newKey = url.split('/').slice(3)[0] + '_composio';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change breaks URL parsing by only taking the first segment after the domain instead of the full path. This will cause incorrect configuration keys for nested paths.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const newKey = url.split('/').slice(3)[0] + '_composio';
const newKey = url.split('/').slice(3).join('/') + '_composio';

@@ -199,7 +199,7 @@ function saveMcpConfig(url: string, clientType: string, mcpUrl: string, command:
}

try {
const newKey = url.split('/').slice(3).join('/').replace(/\//g, '-');
const newKey = url.split('/').slice(3)[0] + '_composio';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding validation for the URL structure before accessing the first segment. The current implementation might throw an error if url.split('/').slice(3) returns an empty array. A safer approach would be:

const segments = url.split('/').slice(3);
if (segments.length === 0) {
    throw new Error('Invalid URL format');
}
const newKey = segments[0] + '_composio';

@@ -199,7 +199,7 @@ function saveMcpConfig(url: string, clientType: string, mcpUrl: string, command:
}

try {
const newKey = url.split('/').slice(3).join('/').replace(/\//g, '-');
const newKey = url.split('/').slice(3)[0] + '_composio';

cursorConfig.mcpServers[newKey] = sseConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a check for potential key collisions. Since we're now using only the first segment of the URL path, there's a possibility that different URLs with the same first segment could overwrite each other's configurations. Example:

if (cursorConfig.mcpServers[newKey]) {
    console.log(chalk.yellow(`⚠️ Warning: Overwriting existing configuration for ${newKey}`));
}

@shreysingla11
Copy link
Collaborator

Code Review Summary

Changes Overview

  • Version bump from 1.0.4 to 1.0.5 ✅
  • Simplified MCP server key generation for cursor client ✅

Potential Issues

  1. URL Validation: The new key generation method lacks validation for URL structure
  2. Key Collision Risk: Using only the first segment of the URL path could lead to key collisions

Suggestions

  1. Add URL structure validation
  2. Add warning for potential key overwrites
  3. Consider adding a unique identifier (like timestamp or hash) if key collisions become an issue

Overall Assessment

The changes are generally good and improve the user experience by creating shorter, more manageable server keys. The version bump is appropriate for this feature addition. However, adding the suggested validations would make the code more robust.

Rating: 7/10 - Good changes but needs additional error handling and validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants