refactor: update tool registration to prevent duplicates #4561
Conversation
|
""" Walkthrough本次更改修改了 Changes
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
packages/ai-native/src/browser/mcp/mcp-server.feature.registry.ts (1)
47-52: 代码更改有效地防止了工具重复注册此更改通过检查已存在的同名工具并进行替换而非简单追加，成功地防止了工具列表中出现重复项。这是一个很好的实现，可以确保每个工具名称在注册表中的唯一性。
不过，我注意到
getMCPTool方法（第42-44行）在查找工具时考虑了serverName参数，而此处只比较工具名称。为了保持一致性，可以考虑在检查重复时也将serverName纳入考虑：- const existingIndex = this.tools.findIndex((t) => t.name === tool.name); + // 考虑serverName进行更精确的工具匹配 + const existingIndex = this.tools.findIndex((t) => { + return getToolName(t.name, t.serverName || BUILTIN_MCP_SERVER_NAME) === + getToolName(tool.name, tool.serverName || BUILTIN_MCP_SERVER_NAME); + });这样修改可以确保工具的唯一性更加精确，与
getMCPTool方法的查找逻辑保持一致。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ai-native/src/browser/mcp/mcp-server.feature.registry.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build-windows
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4561 +/- ##
==========================================
- Coverage 52.64% 52.63% -0.02%
==========================================
Files 1684 1685 +1
Lines 104044 104064 +20
Branches 22577 22576 -1
==========================================
- Hits 54773 54769 -4
- Misses 40956 40973 +17
- Partials 8315 8322 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/next |
|
🎉 PR Next publish successful! 3.8.3-next-1747818383.0 |
|
/next |
|
🎉 PR Next publish successful! 3.9.1-next-1747829468.0 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ai-native/__test__/node/mcp-server.sse.test.ts (1)
52-53: 记录为什么禁用测试很好，但可以提供更多详细信息添加TODO注释解释为什么跳过测试是一个好的做法，但建议提供更多关于具体需要修改的内容的详细信息。这将帮助其他开发人员理解为什么这个测试不再有效以及如何修复它。
建议:
- 添加具体的MCP SDK版本号信息
- 简要说明哪些API或行为发生了变化
- 如果可能，链接到相关的任务或问题追踪
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ai-native/__test__/node/mcp-server.sse.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build-windows
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ai-native/src/browser/mcp/tools/handlers/ListDir.ts (1)
93-93: 建议：同样使用path.join处理此处的路径连接为了保持一致性，建议此处也使用
path.join代替字符串拼接。- const filePath = `${absolutePath}/${uri.displayName}`; + const filePath = path.join(absolutePath, uri.displayName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ai-native/src/browser/mcp/tools/handlers/ListDir.ts(2 hunks)packages/ai-native/src/browser/mcp/tools/handlers/ReadFile.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build-windows
🔇 Additional comments (4)
packages/ai-native/src/browser/mcp/tools/handlers/ListDir.ts (2)
2-2: 导入了path模块以改进路径处理引入
path模块是一个很好的改进，它允许使用更健壮的路径连接方法。
70-70: 改进了路径连接方式将字符串拼接改为
path.join是一个很好的改进，这样可以：
- 处理不同操作系统的路径分隔符
- 避免路径分隔符重复
- 处理路径末尾斜杠等边缘情况
packages/ai-native/src/browser/mcp/tools/handlers/ReadFile.ts (2)
4-4: 导入了path模块以改进路径处理引入
path模块是一个很好的改进，注意这里从@opensumi/ide-core-common导入，而ListDir.ts从@opensumi/ide-core-browser导入。这可能是基于各文件的不同依赖关系，但值得注意保持一致性。
110-110: 改进了路径连接方式将字符串拼接改为
path.join是一个很好的改进，可以更安全地处理文件路径，避免因路径分隔符导致的问题。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts (1)
30-34: 错误消息语言不一致代码中的注释和日志消息使用中文，但抛出的错误消息使用英文，应保持语言一致性。
应用此差异修复语言不一致问题：
- throw new Error("can't find project dir"); + throw new Error('无法找到项目目录');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ai-native/src/browser/mcp/tools/createNewFileWithText.ts(2 hunks)packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ai-native/src/browser/mcp/tools/createNewFileWithText.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts (1)
packages/ai-native/src/browser/types.ts (1)
MCPLogger(355-357)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build-windows
🔇 Additional comments (3)
packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts (3)
1-7: 导入声明合理且符合预期导入的依赖项都是实现文件创建功能所必需的，包括依赖注入、URI处理、文件服务和工作区服务。
9-22: 依赖注入模式实现正确类使用标准的依赖注入模式，所有必要的服务都已正确注入。注释清晰地说明了类的用途。
37-39: 路径构建改进很好使用
path.join进行路径拼接是一个很好的改进，提高了跨平台兼容性，符合 AI 总结中提到的路径处理标准化。
packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts
Show resolved
Hide resolved
packages/ai-native/src/browser/mcp/tools/handlers/CreateNewFileWithText.ts
Outdated
Show resolved
Hide resolved
ensorrow
force-pushed
the
refactor/tool-override
branch
from
c7eb9a8 to
d14998b
Compare
Types
Background or solution
refactor: update tool registration to prevent duplicates
Changelog
refactor: update tool registration to prevent duplicates
Summary by CodeRabbit