fix: ai plugin and mcp issue#1586
Conversation
WalkthroughConsolidates chat UI into a single scrollable container with exposed chatContainerRef and mcpDrawerPosition; switches message rendering to incremental renderContent entries and contentRenderers (including LoadingRenderer); updates Markdown rendering to hljs core + markdown-it with theme support; adds MCP position prop; removes mock MCP servers and adjusts tool metadata/types. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Main as Main.vue (UI)
participant Utils as mcp/utils.ts
participant LLM as LLM
participant Tool as MCP Tool
Note over Main: User submits prompt → message appended with renderContent=[loading]
U->>Main: submit prompt
Main->>Utils: sendMcpRequest(messages)
Utils->>LLM: fetchLLM(formatMessages(messages))
LLM-->>Utils: assistant response (may include tool call or markdown)
alt assistant requests tool
Utils->>Tool: execute(toolCall)
Tool-->>Utils: tool result
Utils->>Utils: append tool result to message.renderContent (status=success)
Utils->>LLM: fetchLLM(with tool messages)
LLM-->>Utils: assistant follow-up
end
Utils->>Main: update message.renderContent incrementally (uses LoadingRenderer)
Main->>Main: auto-scroll via chatContainerRef (nextTick)
Utils->>Main: finalize messages.at(-1)!.content from last renderContent entry and remove internal renderContent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/plugins/robot/src/mcp/utils.ts (2)
63-65: Sanitize history messages before subsequent LLM callsIn tool-call rounds, you pass raw
historyMessages(which can include UI-only fields likerenderContent) tofetchLLM. UseformatMessages(...)here too to avoid leaking non-spec fields to the LLM API.- const historyMessages = contextMessages?.length ? contextMessages : toRaw(messages.slice(0, -1)) - const toolMessages: LLMMessage[] = [...historyMessages, res.choices[0].message] as LLMMessage[] + const history = contextMessages?.length ? contextMessages : messages.slice(0, -1) + const historyForLLM = formatMessages(history as unknown as LLMMessage[]) + const toolMessages: LLMMessage[] = [...historyForLLM, res.choices[0].message]
107-121: Type mismatch and unsafe assignment tocontent
sendMcpRequesttakesLLMMessage[], but later usesrenderContentwhich is only onRobotMessage. This will not type-check and risks runtime errors.- Assigning
messages.at(-1)!.contentfromrenderContent.at(-1)!.contentcan assign a non-string (e.g., a tool result object) when the final assistant message has no markdown content.Update the function to accept
RobotMessage[], and safely derive the final string content:-export const sendMcpRequest = async (messages: LLMMessage[], options: RequestOptions = {}) => { +export const sendMcpRequest = async (messages: RobotMessage[], options: RequestOptions = {}) => { if (messages.length < 1) { return } const tools = await useMcpServer().getLLMTools() requestOptions = options - const res = await fetchLLM(formatMessages(messages.slice(0, -1)), tools, options) + const res = await fetchLLM(formatMessages(messages.slice(0, -1) as unknown as LLMMessage[]), tools, options) const hasToolCall = res.choices[0].message.tool_calls?.length > 0 if (hasToolCall) { await handleToolCall(res, tools, messages) - messages.at(-1)!.content = messages.at(-1)!.renderContent.at(-1)!.content + const lastMsg = messages.at(-1)! + const lastItem = lastMsg.renderContent?.at(-1) + if (lastItem && lastItem.type === 'markdown' && typeof lastItem.content === 'string') { + lastMsg.content = lastItem.content + } else if (typeof res.choices[0].message.content === 'string') { + lastMsg.content = res.choices[0].message.content + } else { + lastMsg.content = '' + } } else { messages.at(-1)!.content = res.choices[0].message.content } }
🧹 Nitpick comments (14)
packages/plugins/robot/src/mcp/types.ts (1)
14-14: Avoid adding non-spec fields to OpenAI tool definitions
RequestTool.function.titleis not part of the OpenAI tool/function-call schema. Keeping it in the type can mislead future code to serialize and send it, causing API rejections. Since we don’t actually populate it in convertMCPToOpenAITools, prefer removing it (or clearly documenting it as “UI-only, never serialized”).Apply this diff to drop the field:
export interface RequestTool { type: 'function' function: { name: string description: string - title?: string parameters: { type: 'object' required?: string[] properties: Record<packages/plugins/robot/src/mcp/useMcp.ts (2)
58-58: CoerceisToolsEnabledto a strict boolean
some(...)may never run (undefined chaining), yieldingundefined. Coerce to boolean to avoid ternary/boolean logic surprises in consumers.-const isToolsEnabled = computed(() => getEngineServer()?.tools?.some((tool) => tool.enabled)) +const isToolsEnabled = computed(() => !!getEngineServer()?.tools?.some((tool) => tool.enabled))
81-90: Prevent duplicate server entries when addingGiven
inUseMcpServersis initialized with the engine server, pushing again on “add” can create duplicates if the UI ever firesplugin-addfor it. Guard against duplicates.if (added) { - const newServer: PluginInfo = { + const newServer: PluginInfo = { ...server, id: server.id || `mcp-server-${Date.now()}`, enabled: true, added: true, expanded: false, tools: server.tools || [] } - inUseMcpServers.value.push(newServer) + if (!inUseMcpServers.value.some((s) => s.id === newServer.id)) { + inUseMcpServers.value.push(newServer) + }packages/plugins/robot/src/mcp/utils.ts (1)
34-40: Narrow the type ofparseArgsto reduce accidental misuseWhen JSON.parse fails, you return the original string. That string is later sent to
callTool, which declaresargs: Record<string, unknown>. Consider returningunknownand normalizing to an object to keep the wire format consistent, or updatecallTool’s signature to acceptunknown.For example:
-const parseArgs = (args: string) => { +const parseArgs = (args: string): unknown => { try { return JSON.parse(args) } catch (error) { - return args + // Fallback: preserve the raw string in a stable envelope + return { __raw: args } } }And adapt tool invocation/rendering accordingly.
packages/plugins/robot/src/mcp/MarkdownRenderer.vue (2)
75-84: Avoid querying the whole document when highlightingRe-highlighting on theme change is fine, but searching
documentmay touch unrelated code blocks. Limit to this component’s root via a ref.Example (outside selected lines for clarity):
- Template:
<div ref="root" ... />- Script:
const root = ref<HTMLElement>()root.value?.querySelectorAll('pre code')...
If you want, I can provide a concrete patch.Also applies to: 86-93
31-44: Minor typing improvement for theme propUse PropType for stricter typing, avoids relying on a function-cast to String.
+import type { PropType } from 'vue' ... - theme: { - type: String as () => 'light' | 'dark', + theme: { + type: String as PropType<'light' | 'dark'>, default: 'dark' },packages/plugins/robot/src/Main.vue (8)
83-83: Import nextTick (used by scrollContent/watch fix)Needed for reliable post-DOM update scrolling.
-import { ref, onMounted, watchEffect, type CSSProperties, h, resolveComponent, computed, watch } from 'vue' +import { ref, onMounted, watchEffect, type CSSProperties, h, resolveComponent, computed, watch, nextTick } from 'vue'
56-58: Avoid double auto-scroll strategiesYou both:
- Manually scroll via chatContainerRef, and
- Use TrBubbleList’s autoScroll.
These can fight each other or cause redundant work. Prefer one approach.
If you keep the container-based approach, remove autoScroll:
-<tr-bubble-list :items="activeMessages" :roles="roles" autoScroll></tr-bubble-list> +<tr-bubble-list :items="activeMessages" :roles="roles"></tr-bubble-list>
39-59: Make the container actually scrollableSince scrollContent scrolls robot-chat-container-content, ensure it’s a scroll container. Otherwise, setting scrollTop has no effect.
Add CSS (outside selected lines) to the scoped style:
.robot-chat-container-content { display: flex; flex-direction: column; min-height: 0; /* allow child to shrink in flex context */ height: 100%; overflow: auto; }Please verify visually that:
- The content area gets a scrollbar when overflowed,
- scrollContent moves to bottom as messages stream in.
205-223: Unify loading state across MCP and HTTP flows; always scroll after completionrequestLoading is only toggled for the MCP path, leading to inconsistent UX. Also, scrolling after completion is useful in the HTTP path too.
Suggestion (outside selected lines; illustrative):
// At function start: requestLoading.value = true // In HTTP branch: getMetaApi(META_SERVICE.Http) .post(...) // existing then/catch... .finally(async () => { inProcesing.value = false requestLoading.value = false await scrollContent() })This keeps the sender’s loading UI consistent and ensures the viewport stays pinned to the bottom in both flows.
128-129: Type the ref for better TS safetyMinor, but helpful for IDE/TS:
-const chatContainerRef = ref(null) +const chatContainerRef = ref<HTMLElement | null>(null)
198-201: Normalize message helpers to the new shapeBoth getAiRespMessage and getMessage can provide renderContent by default to simplify downstream rendering logic.
// getAiRespMessage -const getAiRespMessage = (role = 'assistant', content) => ({ - role, - content -}) +const getAiRespMessage = (role = 'assistant', content) => ({ + role, + content, + renderContent: content +}) // getMessage -const getMessage = (content) => ({ - role: 'user', - content -}) +const getMessage = (content) => ({ + role: 'user', + content, + renderContent: content +})If you intend only assistant to use renderContent, skip the user change; otherwise it keeps things uniform and MarkdownRenderer can still handle both roles consistently.
Also applies to: 266-269
436-446: Add CSS var fallback for drawer width; verify variable scopeWhen not fullscreen you use right: 'var(--tr-container-width)'. If the popup is appended to body, that CSS var may be undefined outside the container scope.
Apply this diff to provide a robust fallback:
- ...(fullscreen.value ? { left: 0 } : { right: 'var(--tr-container-width)' }) + ...(fullscreen.value ? { left: 0 } : { right: 'var(--tr-container-width, 400px)' })Please verify in devtools that the MCP drawer aligns flush with the container edge in both fullscreen and non-fullscreen modes.
134-135: Typo in asset filename: defaultAvator → defaultAvatar
Our repo-wide search confirms the file and all references are consistently spelled “defaultAvator.png,” and there is no “defaultAvatar.png.” To correct this typo and avoid future confusion, please:• Rename the asset
- packages/design-core/public/img/defaultAvator.png → defaultAvatar.png
• Update every import/path- packages/toolbars/collaboration/src/Main.vue (watchEffect at line 78; userHead at line 84)
- packages/plugins/robot/src/Main.vue (avatarUrl.value at line 134)
Once renamed, rebuild or restart your dev server to verify the image loads as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/plugins/robot/src/Main.vue(12 hunks)packages/plugins/robot/src/mcp/MarkdownRenderer.vue(1 hunks)packages/plugins/robot/src/mcp/McpServer.vue(4 hunks)packages/plugins/robot/src/mcp/types.ts(3 hunks)packages/plugins/robot/src/mcp/useMcp.ts(2 hunks)packages/plugins/robot/src/mcp/utils.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-15T02:19:06.755Z
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/DesignCanvas.vue:0-0
Timestamp: 2025-01-15T02:19:06.755Z
Learning: In Vue components using message subscriptions from opentiny/tiny-engine-meta-register, always clean up subscriptions in the onUnmounted hook using useMessage().unsubscribe() to prevent memory leaks.
Applied to files:
packages/plugins/robot/src/Main.vue
📚 Learning: 2025-01-14T06:49:00.797Z
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered and available throughout Vue components without requiring explicit imports.
Applied to files:
packages/plugins/robot/src/Main.vue
🧬 Code Graph Analysis (2)
packages/plugins/robot/src/mcp/useMcp.ts (2)
packages/register/src/common.ts (1)
getMetaApi(20-30)packages/register/src/constants.ts (1)
META_SERVICE(1-23)
packages/plugins/robot/src/mcp/utils.ts (1)
packages/plugins/robot/src/mcp/types.ts (1)
LLMMessage(30-34)
🔇 Additional comments (7)
packages/plugins/robot/src/mcp/types.ts (2)
38-40: Model shift to structured rendering looks goodChanging
RobotMessage.contenttostringand introducingrenderContent?: BubbleContentItem[]aligns with the new rendering pipeline. This pairs correctly with Main.vue’scustomContentField: 'renderContent'.
71-71: Addingtitleto McpTool improves UI/UX for tool displayOptional
titleenables localized or friendly naming while preservingnameas the stable identifier. Good addition.packages/plugins/robot/src/mcp/useMcp.ts (1)
26-33: Tool display name formatting is sensiblePreferring
title（name）when available reads well in Chinese UI while keeping the uniquenamevisible. No issues.packages/plugins/robot/src/mcp/utils.ts (1)
9-16: Formatting outbound messages before LLM calls is correctThe new
formatMessagesstrips UI-only fields (e.g., renderContent) before sending to the LLM. Good defensive step.packages/plugins/robot/src/mcp/McpServer.vue (1)
8-21: Popup positioning prop passthrough aligns with Main.vueBinding
:popup-config="props.position"is the right hook for externalizing drawer placement. Once the default fix above is applied, this is good to go.If you want, I can scan for all usages to ensure they pass a
positionprop where necessary and won’t rely on the broken default. Let me know and I’ll generate a script.packages/plugins/robot/src/Main.vue (2)
71-71: LGTM: mcp-server now accepts a position propBinding mcpDrawerPosition keeps the popup aligned with the container. Nice.
519-550: LGTM: Markdown/table/list styling upgradesThe table borders, zebra striping, hover effect, and list padding greatly improve readability. Font-size unification to 14px matches the PR goals.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
packages/plugins/robot/src/mcp/utils.ts (1)
63-80: Bug: toolMessages constructed from raw messages; include only role/content and preserve tool_callsCurrently historyMessages (RobotMessage[]) are spread directly into toolMessages, which leaks non-LLM fields (e.g., renderContent) into the backend. Also, res.choices[0].message is pushed as-is with optional role. Shape should be normalized: use formatMessages for history and construct an assistant message preserving tool_calls.
Apply this diff:
- const historyMessages = contextMessages?.length ? contextMessages : toRaw(messages.slice(0, -1)) - const toolMessages: LLMMessage[] = [...historyMessages, res.choices[0].message] as LLMMessage[] + const historyMessages = contextMessages?.length ? contextMessages : toRaw(messages.slice(0, -1)) + const baseMessages = formatMessages(historyMessages as unknown as LLMMessage[]) + const assistantMsg: LLMMessage = { + role: (res.choices[0].message.role as string) || 'assistant', + content: res.choices[0].message.content || '', + tool_calls: res.choices[0].message.tool_calls + } + const toolMessages: LLMMessage[] = [...baseMessages, assistantMsg] @@ - const toolCallResult = await useMcpServer().callTool(name, parsedArgs) + const toolCallResult = await useMcpServer().callTool(name, parsedArgs) toolMessages.push({ type: 'text', - content: toolCallResult.content, + content: typeof toolCallResult?.content === 'string' ? toolCallResult.content : JSON.stringify(toolCallResult ?? {}), role: 'tool', tool_call_id: tool.id }) @@ - const newResp = await fetchLLM(toolMessages, tools) + const newResp = await fetchLLM(toolMessages, tools)Also applies to: 92-95
packages/plugins/robot/src/mcp/useMcp.ts (1)
124-129: EnsurelistToolsalways returns anMcpListToolsResponseto match its signature
The current implementation can resolve toundefineddue to optional chaining, violating thePromise<McpListToolsResponse>return type. Wrap the call in anawaitand provide a{ tools: [] }fallback.• packages/plugins/robot/src/mcp/useMcp.ts – update the
listToolsfunction-const listTools = async (): Promise<McpListToolsResponse> => - getMetaApi(META_SERVICE.McpService)?.getMcpClient()?.listTools() +const listTools = async (): Promise<McpListToolsResponse> => { + const resp = await getMetaApi(META_SERVICE.McpService) + ?.getMcpClient() + ?.listTools() + return resp ?? { tools: [] } +}packages/plugins/robot/src/Main.vue (2)
205-224: Bug: computed used as truthy object; must use .value; also mirror renderContent on errors
- if (useMcpServer().isToolsEnabled) always enters the MCP branch; should be .isToolsEnabled.value.
- In catch/finally paths, update renderContent so assistant bubble renders error text.
Apply this diff:
- if (useMcpServer().isToolsEnabled) { + if (useMcpServer().isToolsEnabled.value) { try { requestLoading.value = true await scrollContent() // eslint-disable-line await sendMcpRequest(messages.value, { model: selectedModel.value.value, headers: { Authorization: `Bearer ${tokenValue.value || import.meta.env.VITE_API_TOKEN}` } }) } catch (error) { - messages.value[messages.value.length - 1].content = '连接失败' + const failed = '连接失败' + const last = messages.value[messages.value.length - 1] + last.content = failed + last.renderContent = [{ type: 'markdown', content: failed }] } finally { inProcesing.value = false requestLoading.value = false await scrollContent() // eslint-disable-line } return }
233-246: Mirror renderContent in HTTP branch success/error pathsKeeps assistant rendering consistent with customContentField='renderContent'.
- messages.value[messages.value.length - 1].content = replyWithoutCode + const last = messages.value[messages.value.length - 1] + last.content = replyWithoutCode + last.renderContent = [{ type: 'markdown', content: replyWithoutCode }] @@ - .catch(() => { - messages.value[messages.value.length - 1].content = '连接失败' + .catch(() => { + const failed = '连接失败' + const last = messages.value[messages.value.length - 1] + last.content = failed + last.renderContent = [{ type: 'markdown', content: failed }]
♻️ Duplicate comments (5)
packages/plugins/robot/src/Main.vue (5)
83-83: Import nextTick for robust scrolling (paired with scroll fix below)Add nextTick to ensure DOM updates before scrolling.
Apply this diff:
-import { ref, onMounted, watchEffect, type CSSProperties, h, resolveComponent, computed, watch } from 'vue' +import { ref, onMounted, watchEffect, type CSSProperties, h, resolveComponent, computed, watch, nextTick } from 'vue'
249-253: Ensure DOM update before scrolling; improve scroll robustnessAwait nextTick and use a local var.
-const scrollContent = async () => { - if (chatContainerRef.value) { - chatContainerRef.value.scrollTop = chatContainerRef.value.scrollHeight - } -} +const scrollContent = async () => { + await nextTick() + const el = chatContainerRef.value as HTMLElement | null + if (el) { + el.scrollTop = el.scrollHeight + } +}
306-306: Assistant placeholder must set renderContent; remove stray name propWithout renderContent, the assistant bubble can appear blank.
-messages.value.push({ role: 'assistant', content: '好的，正在执行相关操作，请稍等片刻...', name: 'AI' }) +messages.value.push({ + role: 'assistant', + content: '好的，正在执行相关操作，请稍等片刻...', + renderContent: [{ type: 'markdown', content: '好的，正在执行相关操作，请稍等片刻...' }] +})
432-435: watch sources are non-reactive; callback won’t re-runPass functions (reactive getters) instead of evaluated values.
-watch([activeMessages.value.length, activeMessages.value.at(-1)?.renderContent?.length], async () => { - await scrollContent() -}) +watch( + [() => activeMessages.value.length, () => activeMessages.value.at(-1)?.renderContent?.length ?? 0], + async () => { + await scrollContent() + } +)
198-201: Default assistant message should include renderContent (markdown item)Prevents empty assistant bubbles when customContentField='renderContent'.
-const getAiRespMessage = (role = 'assistant', content) => ({ - role, - content -}) +const getAiRespMessage = (role = 'assistant', content: string) => ({ + role, + content, + renderContent: [{ type: 'markdown', content }] +})
🧹 Nitpick comments (5)
packages/plugins/robot/src/mcp/types.ts (2)
14-14: Good addition: optional title on RequestTool.functionThis enables localized/display-friendly tool names. Ensure it’s propagated when converting MCP tools to OpenAI tools (see convertMCPToOpenAITools in useMcp.ts).
71-72: Good: McpTool gains optional titleMatches UI’s “title（name）” display. Also consider carrying this into RequestTool.function.title during conversion so LLM UIs can surface friendlier labels.
packages/plugins/robot/src/mcp/utils.ts (1)
113-118: Correctly formats initial LLM call; minor safety on final content assignmentInitial call uses formatMessages — good. For the final assignment below, guard against non-markdown last entries.
Apply this diff to pick the last markdown item’s content or fallback to the plain response:
- messages.at(-1)!.content = messages.at(-1)!.renderContent.at(-1)!.content + const rc = messages.at(-1)!.renderContent || [] + const lastMarkdown = [...rc].reverse().find((it: any) => it?.type === 'markdown') + messages.at(-1)!.content = lastMarkdown?.content ?? res.choices?.[0]?.message?.content ?? ''packages/plugins/robot/src/mcp/useMcp.ts (1)
37-52: Propagate title into OpenAI tool function metadataYou’ve added title to types but it’s not forwarded. This helps downstream UIs and logs.
Apply this diff:
return mcpTools.map((tool: McpTool) => ({ type: 'function', function: { name: tool.name, - description: tool.description || '', + description: tool.description || '', + title: tool.title, parameters: { type: 'object', properties: Object.fromEntries( Object.entries(tool.inputSchema?.properties || {}).map(([key, prop]: [string, any]) => [key, { ...prop }]) ), required: tool.inputSchema?.required || [] } } })) as RequestTool[]packages/plugins/robot/src/Main.vue (1)
128-129: Type chatContainerRef for safer DOM accessAvoid implicit any and enable editor intellisense.
-const chatContainerRef = ref(null) +const chatContainerRef = ref<HTMLElement | null>(null)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/plugins/robot/src/Main.vue(12 hunks)packages/plugins/robot/src/mcp/MarkdownRenderer.vue(1 hunks)packages/plugins/robot/src/mcp/McpServer.vue(4 hunks)packages/plugins/robot/src/mcp/types.ts(3 hunks)packages/plugins/robot/src/mcp/useMcp.ts(2 hunks)packages/plugins/robot/src/mcp/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugins/robot/src/mcp/McpServer.vue
- packages/plugins/robot/src/mcp/MarkdownRenderer.vue
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-15T02:19:06.755Z
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/DesignCanvas.vue:0-0
Timestamp: 2025-01-15T02:19:06.755Z
Learning: In Vue components using message subscriptions from opentiny/tiny-engine-meta-register, always clean up subscriptions in the onUnmounted hook using useMessage().unsubscribe() to prevent memory leaks.
Applied to files:
packages/plugins/robot/src/Main.vue
📚 Learning: 2025-01-14T06:49:00.797Z
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered and available throughout Vue components without requiring explicit imports.
Applied to files:
packages/plugins/robot/src/Main.vue
🧬 Code Graph Analysis (2)
packages/plugins/robot/src/mcp/useMcp.ts (2)
packages/register/src/common.ts (1)
getMetaApi(20-30)packages/register/src/constants.ts (1)
META_SERVICE(1-23)
packages/plugins/robot/src/mcp/utils.ts (1)
packages/plugins/robot/src/mcp/types.ts (1)
LLMMessage(30-34)
⏰ 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: push-check
🔇 Additional comments (8)
packages/plugins/robot/src/mcp/utils.ts (3)
9-16: formatMessages: simple, correct shaping for LLM inputKeeps only role/content which most backends expect.
52-60: Render pipeline initialization looks correctInitializing renderContent and appending initial markdown content is consistent with the new Bubble renderer.
86-91: Status and result plumbing looks goodMarks the last tool block as success and replaces content with params/result bundle — aligns with renderers expecting structured tool output.
packages/plugins/robot/src/mcp/useMcp.ts (1)
26-34: UI tool naming via optional title is solidDisplaying “title（name）” improves clarity while preserving identity.
packages/plugins/robot/src/Main.vue (4)
39-59: Unified scrollable container and conditional rendering look goodSingle container with welcome and message states improves UX and makes auto-scroll easier to reason about.
422-429: roles config aligns with new render pipelineUsing MarkdownRenderer and customContentField='renderContent' is consistent with the types and utils changes.
436-445: mcpDrawerPosition computed is sensibleAnchoring left in fullscreen and right to container otherwise matches the PR objective “全屏时 MCP 弹窗位置改为侧边”.
519-550: Markdown/table/list styling update looks goodReadable defaults (14px, paddings, table borders/zebra) align with “支持 markdown 样式 / 代码块高亮 / 字体大小统一为 14px”.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/plugins/robot/src/mcp/useMcp.ts (1)
90-93: Do not force-enable all tools on server add; respect server-reported statusesCalling
updateEngineServer(newServer, added)afterupdateEngineTools()will enable every tool regardless of its reportedstatus. This contradicts the fetched metadata.if (server.id === ENGINE_MCP_SERVER.id) { await updateEngineTools() - updateEngineServer(newServer, added) }If you need a “toggle all” action, trigger
updateEngineServeronly from that UI action, not on add.packages/plugins/robot/src/Main.vue (1)
225-244: Bug: computed ref used as boolean — MCP branch always taken
useMcpServer().isToolsEnabledis a ref; without.valuethe condition is always truthy. This forces the MCP path even when no tools are enabled.- if (useMcpServer().isToolsEnabled) { + if (useMcpServer().isToolsEnabled.value) {Optional: initialize once to avoid repeatedly creating the composable:
// near other consts in setup() const mcp = useMcpServer() // then: if (mcp.isToolsEnabled.value) { /* ... */ }
♻️ Duplicate comments (5)
packages/plugins/robot/src/mcp/MarkdownRenderer.vue (1)
10-11: Remove global highlight.js theme import to avoid overriding theme-scoped stylesThis import applies the light theme globally and conflicts with the theme-scoped CSS you already import in the Less block.
-import 'highlight.js/styles/github.css'packages/plugins/robot/src/Main.vue (4)
166-173: Nice: scroll now awaits DOM updateSwitching to nextTick + null-checked element eliminates race conditions and flakiness.
247-254: Mirror renderContent when updating assistant replies and errorsPrevents empty assistant bubbles when only
contentis set.- messages.value[messages.value.length - 1].content = replyWithoutCode + messages.value[messages.value.length - 1].content = replyWithoutCode + messages.value[messages.value.length - 1].renderContent = replyWithoutCode setContextSession() @@ - .catch(() => { - messages.value[messages.value.length - 1].content = '连接失败' + .catch(() => { + messages.value[messages.value.length - 1].content = '连接失败' + messages.value[messages.value.length - 1].renderContent = '连接失败'Also add the same mirroring in the MCP catch at Line 237:
- } catch (error) { - messages.value[messages.value.length - 1].content = '连接失败' + } catch (error) { + messages.value[messages.value.length - 1].content = '连接失败' + messages.value[messages.value.length - 1].renderContent = '连接失败'Also applies to: 261-266
320-321: Populate placeholder assistant’s renderContent and drop stray nameAligns with roles.assistant.customContentField and avoids unused prop.
- messages.value.push({ role: 'assistant', content: '好的，正在执行相关操作，请稍等片刻...', name: 'AI' }) + messages.value.push({ + role: 'assistant', + content: '好的，正在执行相关操作，请稍等片刻...', + renderContent: '好的，正在执行相关操作，请稍等片刻...' + })
218-221: Default assistant message to renderContent to match customContentFieldAssistant bubbles use customContentField='renderContent'. Ensure it’s always present.
-const getAiRespMessage = (role = 'assistant', content) => ({ - role, - content -}) +const getAiRespMessage = (role = 'assistant', content) => ({ + role, + content, + renderContent: content +})
🧹 Nitpick comments (9)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (2)
5-5: Validate non-empty page id.Guard against empty ids to fail fast at validation.
Apply:
- id: z.string().describe('The id of the page') + id: z.string().min(1, 'page id is required').describe('The id of the page')
18-33: Wrap switchPage in try/catch and return Markdown for better UXThe pipeline already recognizes
type: 'markdown'(see other plugins), so you can safely return a formatted code block instead of plain text.– File:
packages/plugins/page/src/mcp/tools/editSpecificPage.ts(around lines 18–33)
Apply the following diff:await switchPage(id) const res = { status: 'success', message: `Page now can be edited.`, data: {} } return { content: [ { type: 'text', text: JSON.stringify(res) } ] }becomes
+ try { + await switchPage(id) + const res = { + status: 'success', + message: `Page "${id}" is now editable.`, + data: {} + } + return { + content: [ + { + type: 'markdown', + text: `✅ 成功：已在画布中打开并可编辑页面：${id}\n\n\`\`\`json\n${JSON.stringify(res, null, 2)}\n\`\`\`` + } + ] + } + } catch (e) { + const errMsg = e instanceof Error ? e.message : String(e) + const res = { + status: 'error', + message: `Failed to switch page: ${id}`, + error: errMsg + } + return { + content: [ + { + type: 'markdown', + text: `❌ 切换页面失败：${id}\n\n\`\`\`json\n${JSON.stringify(res, null, 2)}\n\`\`\`` + } + ] + } + }packages/plugins/robot/src/mcp/MarkdownRenderer.vue (2)
36-38: Use PropType for union prop and (optionally) add a runtime validatorImproves typing and prevents accidental non-light/dark values at runtime.
- theme: { - type: String as () => 'light' | 'dark', - default: 'light' - }, + theme: { + type: String as import('vue').PropType<'light' | 'dark'>, + default: 'light', + validator: (v: string) => v === 'light' || v === 'dark' + },Note: If preferred, import
PropTypeat the top:-import { computed } from 'vue' +import { computed } from 'vue' +import type { PropType } from 'vue'
66-67: Option precedence: consider protecting highlight/html defaultsCurrently
...props.optionscan override highlight/html. If you don't intend that, spread first so your defaults win.-const markdownIt = new MarkdownIt({ - html: true, - breaks: true, - typographer: true, - highlight: (str: string, lang: string) => { +const markdownIt = new MarkdownIt({ + ...props.options, + html: true, + breaks: true, + typographer: true, + highlight: (str: string, lang: string) => { // ... - }, - ...props.options + } })packages/plugins/robot/src/mcp/LoadingRenderer.vue (1)
1-3: Use imported asset binding for robustness; avoid brittle relative URLImporting the asset ensures correct bundler handling across environments. Also replace inline styles with a class if desired.
+<script setup lang="ts"> +import loadingImg from '../../assets/loading.webp' +</script> <template> - <img src="../../assets/loading.webp" alt="loading" style="width: 24px; height: 24px" /> + <img :src="loadingImg" alt="loading" style="width: 24px; height: 24px" /> </template>packages/plugins/robot/src/mcp/useMcp.ts (3)
26-33: Avoid merging title into name; keep semantic fields separate and format in UIMixing title (human label) into name (identifier) can leak UI wording into identifiers and complicate downstream logic. Preserve both and add a
displayNamefor UI if needed.- const engineTools = tools.map((tool) => ({ - id: tool.name, - name: tool.title ? `${tool.title} ${tool.name}` : tool.name, - description: tool.description, - enabled: tool.status === 'enabled' - })) + const engineTools = tools.map((tool) => ({ + id: tool.name, + name: tool.name, + title: tool.title, // keep semantic label + displayName: tool.title ? `${tool.title} ${tool.name}` : tool.name, // UI only + description: tool.description, + enabled: tool.status === 'enabled' + }))Adjust UI renderers to use
displayNamewhere appropriate.
124-129: Fix return type of listTools; handle undefined client gracefullyThe current signature promises a Promise but may return undefined if the client is absent.
-const listTools = async (): Promise<McpListToolsResponse> => - getMetaApi(META_SERVICE.McpService)?.getMcpClient()?.listTools() +const listTools = async (): Promise<McpListToolsResponse | undefined> => { + const client = getMetaApi(META_SERVICE.McpService)?.getMcpClient?.() + return client?.listTools() +}Optionally also refine
callTool’s return type instead of|| {}.
58-59: Expose boolean accessor or ensure.valueusage
isToolsEnabledis a computed ref—callers must use.valueto read the boolean. We found one direct misuse in your code:• packages/plugins/robot/src/Main.vue:226
- if (useMcpServer().isToolsEnabled) { + if (useMcpServer().isToolsEnabled.value) {To avoid accidental omissions, consider one of the following optional refactors:
- Export a plain function
isToolsEnabled()that returnsbooleaninstead of the ref.- Rename the ref to
isToolsEnabledRefto make its reactive nature explicit.packages/plugins/robot/src/Main.vue (1)
446-449: Make watcher async and await scroll for deterministic behaviorEnsures scroll occurs after reactive updates settle.
-watch([() => activeMessages.value.length, () => activeMessages.value.at(-1)?.renderContent?.length ?? 0], () => { - scrollContent() -}) +watch( + [() => activeMessages.value.length, () => activeMessages.value.at(-1)?.renderContent?.length ?? 0], + async () => { + await scrollContent() + } +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts(1 hunks)packages/plugins/robot/src/Main.vue(13 hunks)packages/plugins/robot/src/mcp/LoadingRenderer.vue(1 hunks)packages/plugins/robot/src/mcp/MarkdownRenderer.vue(1 hunks)packages/plugins/robot/src/mcp/McpServer.vue(4 hunks)packages/plugins/robot/src/mcp/useMcp.ts(2 hunks)packages/plugins/robot/src/mcp/utils.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugins/robot/src/mcp/utils.ts
- packages/plugins/robot/src/mcp/McpServer.vue
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-15T02:19:06.755Z
Learnt from: yy-wow
PR: opentiny/tiny-engine#940
File: packages/canvas/DesignCanvas/src/DesignCanvas.vue:0-0
Timestamp: 2025-01-15T02:19:06.755Z
Learning: In Vue components using message subscriptions from opentiny/tiny-engine-meta-register, always clean up subscriptions in the onUnmounted hook using useMessage().unsubscribe() to prevent memory leaks.
Applied to files:
packages/plugins/robot/src/Main.vue
📚 Learning: 2025-01-14T06:49:00.797Z
Learnt from: gene9831
PR: opentiny/tiny-engine#1011
File: packages/configurator/src/router-select-configurator/RouterSelectConfigurator.vue:63-73
Timestamp: 2025-01-14T06:49:00.797Z
Learning: In the tiny-engine project, the SvgIcon component is globally registered and available throughout Vue components without requiring explicit imports.
Applied to files:
packages/plugins/robot/src/Main.vue
🧬 Code Graph Analysis (1)
packages/plugins/robot/src/mcp/useMcp.ts (2)
packages/register/src/common.ts (1)
getMetaApi(20-30)packages/register/src/constants.ts (1)
META_SERVICE(1-23)
⏰ 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: push-check
🔇 Additional comments (3)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (2)
13-14: No changes needed foreditSpecificPage.ts– using.shapeis consistent across all MCP toolsI’ve verified that every MCP tool in
packages/pluginspassesinputSchema: inputSchema.shapeand then usesz.infer<typeof inputSchema>in its callback (e.g. in addPage, delPage, getPageList, all i18n and materials tools). This matches the established pattern, so unless the MCP tool registry has changed to expect full Zod schemas instead of shape objects, no action is required here.• If the registry was updated to accept full Zod schemas (
z.ZodType), convert all tools to passinputSchemadirectly (and similarly foroutputSchema).
• Otherwise, keep using.shapeas-is.
9-9: No stale references found; tool name is unique and correctly normalized
- Searched for “Edit_page_in_canvas” (with or without trailing dot): no matches
- Searched for “edit_page_in_canvas”: only found in
packages/plugins/page/src/mcp/tools/editSpecificPage.ts(its own definition)- No other tool exports use the same name
All references have been updated; no breakage expected.
packages/plugins/robot/src/Main.vue (1)
436-442: Roles configuration looks solidUsing MarkdownRenderer with customContentField for assistant is consistent with the new render pipeline.
2 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
优化代码，修复AI插件与MCP问题：
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements
Style