Tool cancellation support #1161
Conversation
dosubot
bot
added
the
size:L
WalkthroughThis change introduces tool call cancellation tracking and UI handling. A 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
1-1: Address the Prettier formatting issue.The pipeline flagged code style violations. Run the formatter to resolve:
npm run prettier-fix
🤖 Fix all issues with AI agents
In @client/src/components/ui-playground/UIPlaygroundTab.tsx:
- Around line 125-138: handleCancel currently sets isExecuting=false
unconditionally which can desync when stopFnRef.current is null; change
handleCancel so it only flips setIsExecuting(false) when a stop function was
actually invoked (i.e., check stopFnRef.current before calling it) or enqueue a
cancel request to be processed by PlaygroundMain (e.g., call a provided
queueCancel/notifyCancel callback) and let PlaygroundMain clear isExecuting via
the existing execution completion path; update the handleCancel implementation
to check stopFnRef.current, call it if present and then setIsExecuting(false),
otherwise signal/queue cancellation instead of directly setting isExecuting.
🧹 Nitpick comments (3)
client/src/components/ChatTabV2.tsx (2)
137-168: Consider stabilizinghandleStopand pruningcancelledToolIdsover time.
If this chat can run long, the Set will only grow; andhandleStopwill change whenevermessageschanges. AmessagesRef+ pruning on terminal output would keep memory and rerenders steadier.Also applies to: 435-440
137-168: Consider an explicit in-progress allowlist for resilience to future state additions.
While the current denylist logic correctly handles the four existing states ("input-streaming","input-available","output-available","output-error"), explicitly defining in-progress states would clarify intent and safeguard against future state additions requiring updates here.Suggested refactor (optional)
+ const inProgressStates = new Set(["input-streaming", "input-available"]); + const isInProgress = inProgressStates.has(toolInfo.toolState); - if ( - toolInfo.toolCallId && - toolInfo.toolState !== "output-available" && - toolInfo.toolState !== "output-error" - ) { + if (toolInfo.toolCallId && isInProgress) { inProgressToolIds.add(toolInfo.toolCallId); }client/src/components/ui-playground/PlaygroundMain.tsx (1)
374-377: Consider stabilizinghandleStopto avoid frequentonStopReadyinvocations.Since
handleStopdepends onmessages, it's recreated on every message change, triggeringonStopReadyrepeatedly. If the parent stores this function reference, it may hold stale closures or incur unnecessary re-renders.Consider using a ref to access
messageswithinhandleStop, allowing a stable callback identity:Suggested approach
+ const messagesRef = useRef(messages); + useEffect(() => { messagesRef.current = messages; }, [messages]); const handleStop = useCallback(() => { const inProgressToolIds = new Set<string>(); - for (const msg of messages) { + for (const msg of messagesRef.current) { for (const part of msg.parts ?? []) { // ... rest unchanged } } // ... stop(); - }, [messages, stop]); + }, [stop]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/src/components/ChatTabV2.tsxclient/src/components/chat-v2/thread.tsxclient/src/components/chat-v2/thread/mcp-apps-renderer.tsxclient/src/components/chat-v2/thread/message-view.tsxclient/src/components/chat-v2/thread/part-switch.tsxclient/src/components/ui-playground/PlaygroundLeft.tsxclient/src/components/ui-playground/PlaygroundMain.tsxclient/src/components/ui-playground/TabHeader.tsxclient/src/components/ui-playground/UIPlaygroundTab.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/chat-v2/thread.tsxclient/src/components/ui-playground/UIPlaygroundTab.tsxclient/src/components/chat-v2/thread/mcp-apps-renderer.tsxclient/src/components/chat-v2/thread/part-switch.tsxclient/src/components/ui-playground/PlaygroundMain.tsxclient/src/components/ChatTabV2.tsxclient/src/components/ui-playground/PlaygroundLeft.tsxclient/src/components/ui-playground/TabHeader.tsxclient/src/components/chat-v2/thread/message-view.tsx
🧬 Code graph analysis (2)
client/src/components/ui-playground/PlaygroundMain.tsx (1)
client/src/components/chat-v2/thread/thread-helpers.ts (3)
isToolPart(61-64)isDynamicTool(66-72)getToolInfo(74-99)
client/src/components/ui-playground/TabHeader.tsx (1)
client/src/components/ui/button.tsx (1)
Button(59-59)
🪛 GitHub Actions: Prettier and Build Check
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
[warning] 1-1: Code style issues found in 1 file. Run 'prettier --write' to fix formatting, e.g., 'npm run prettier-fix'.
🔍 Remote MCP
Based on the search results, I can now provide you with additional context relevant to reviewing this pull request.
Summary of Additional Context
MCP Apps Extension (SEP-1865) Background
The PR implements tool cancellation support as part of the MCP Apps Extension (SEP-1865), which is a proposed standard to allow MCP Servers to display interactive UI elements in conversational MCP clients. SEP-1865 proposes an extension to MCP that enables servers to deliver interactive user interfaces to hosts through a standardized pattern for declaring UI resources via the ui:// URI scheme, associating them with tools through metadata, and facilitating bi-directional communication between the UI and the host using MCP's JSON-RPC base protocol.
Tool Cancellation in MCP Protocol
The Model Context Protocol supports optional cancellation of in-progress requests through notification messages, where either side can send a cancellation notification to indicate that a previously-issued request should be terminated. Due to network latency, cancellation notifications may arrive after request processing has completed, and both parties must handle these race conditions gracefully.
UI Communication Pattern
Instead of inventing a custom message protocol, UI components communicate with hosts using existing MCP JSON-RPC base protocol over postMessage. This is relevant because the PR's implementation of MCPAppsRenderer sends cancellation messages through the bridge following this pattern.
Review Implications
The PR extends the chat interface to:
- Track cancelled tool execution IDs (
cancelledToolIds) across components - Detect in-progress tool calls when a stop is requested and mark them as cancelled
- Propagate cancellation state to the MCP Apps renderer to send appropriate
ui/notifications/tool-cancelledmessages to the embedded UI - Update the UI playground to expose cancellation through an
onCancelbutton
This aligns with SEP-1865's requirement for hosts to communicate tool cancellation status back to embedded MCP Apps UIs.
[::web_search::]
⏰ 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: Cursor Bugbot
🔇 Additional comments (14)
client/src/components/ChatTabV2.tsx (1)
31-35: Good cancellation state plumbing (reset included).
Nice, minimal surface-area change: track cancelled IDs in one place, clear them on reset, and thread them down to the renderer path.Also applies to: 84-86, 130-135, 558-559
client/src/components/ui-playground/PlaygroundLeft.tsx (1)
29-56: Clean onCancel wiring from sidebar to header actions.
Prop signature and forwarding look consistent with the intended “Run vs Cancel” UX.Also applies to: 58-83, 161-175
client/src/components/chat-v2/thread/message-view.tsx (1)
13-45: Good propagation of cancellation state to PartSwitch.
Optional prop keeps compatibility, and wiring covers both message branches.Also applies to: 52-76, 94-116
client/src/components/ui-playground/UIPlaygroundTab.tsx (1)
296-321: Nice end-to-end Cancel plumbing (once the stop race is handled).
TheonCancel/onStopReadythreading matches the new interaction model well.Also applies to: 340-357
client/src/components/chat-v2/thread.tsx (1)
11-27: Thread-level cancellation propagation looks solid.
Optional prop keeps compatibility while enabling deeper UI to render cancelled state.Also applies to: 29-44, 94-113
client/src/components/chat-v2/thread/part-switch.tsx (2)
50-51: Prop addition is well-structured.The optional
cancelledToolIdsprop flows cleanly through the component hierarchy.Also applies to: 66-67
139-145: Cancellation state propagation to MCPAppsRenderer is sound.The conditional check handles edge cases gracefully. Note that
ChatGPTAppRenderer(lines 187-208) does not receive equivalent cancellation props—verify whether this asymmetry is intentional or if ChatGPT Apps should also support cancellation notifications.client/src/components/ui-playground/TabHeader.tsx (1)
113-134: Clean toggle between Run and Cancel states.The conditional rendering provides clear user affordance. The destructive variant aptly signals the cancellation action's nature.
client/src/components/ui-playground/PlaygroundMain.tsx (2)
342-372: Cancellation tracking logic is well-implemented.The iteration over messages to identify in-progress tool calls is methodical. The functional state update correctly merges new cancelled IDs.
625-626: Thread correctly receives cancellation state.The
cancelledToolIdsprop propagates the cancellation context to child components for UI reflection.client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (4)
93-97: Props extend cleanly for SEP-1865 cancellation support.The optional typing permits graceful degradation when cancellation context is unavailable.
345-348: Early widget loading is a thoughtful enhancement.Fetching at
input-availableenables the widget to receive cancellation notifications during execution—a prerequisite for responsive cancellation UX.
974-992: Loading and cancellation states render distinctly.The orange-tinted cancellation block provides clear visual differentiation from the standard waiting state.
915-928: Cancellation notification dispatch is well-guarded.The deduplication via
lastToolCancelledRefand optional chaining onsendToolCancelledensure robustness. ThesendToolCancelledmethod is properly defined in@modelcontextprotocol/ext-apps/app-bridgeand the implementation correctly passes the reason parameter as expected by the API contract.
| // Ref to store the stop function from PlaygroundMain | ||
| const stopFnRef = useRef<(() => void) | null>(null); | ||
|
|
||
| // Handler to receive stop function from PlaygroundMain | ||
| const handleStopReady = useCallback((stopFn: () => void) => { | ||
| stopFnRef.current = stopFn; | ||
| }, []); | ||
|
|
||
| // Cancel handler for the sidebar | ||
| const handleCancel = useCallback(() => { | ||
| stopFnRef.current?.(); | ||
| setIsExecuting(false); | ||
| }, [setIsExecuting]); | ||
|
|
There was a problem hiding this comment.
Avoid desync: don’t force isExecuting=false unless you actually stopped something (or queue the cancel).
stopFnRef.current can be null; in that case, the UI currently claims cancellation even though the underlying execution may still be running.
One way to make cancel race-safe
import { useEffect, useCallback, useMemo, useState, useRef } from "react";
@@
const stopFnRef = useRef<(() => void) | null>(null);
+ const cancelRequestedRef = useRef(false);
@@
const handleStopReady = useCallback((stopFn: () => void) => {
stopFnRef.current = stopFn;
+ if (cancelRequestedRef.current) {
+ cancelRequestedRef.current = false;
+ stopFnRef.current?.();
+ }
}, []);
@@
const handleCancel = useCallback(() => {
- stopFnRef.current?.();
- setIsExecuting(false);
- }, [setIsExecuting]);
+ if (!stopFnRef.current) {
+ cancelRequestedRef.current = true;
+ return;
+ }
+ stopFnRef.current();
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ref to store the stop function from PlaygroundMain | |
| const stopFnRef = useRef<(() => void) | null>(null); | |
| // Handler to receive stop function from PlaygroundMain | |
| const handleStopReady = useCallback((stopFn: () => void) => { | |
| stopFnRef.current = stopFn; | |
| }, []); | |
| // Cancel handler for the sidebar | |
| const handleCancel = useCallback(() => { | |
| stopFnRef.current?.(); | |
| setIsExecuting(false); | |
| }, [setIsExecuting]); | |
| // Ref to store the stop function from PlaygroundMain | |
| const stopFnRef = useRef<(() => void) | null>(null); | |
| const cancelRequestedRef = useRef(false); | |
| // Handler to receive stop function from PlaygroundMain | |
| const handleStopReady = useCallback((stopFn: () => void) => { | |
| stopFnRef.current = stopFn; | |
| if (cancelRequestedRef.current) { | |
| cancelRequestedRef.current = false; | |
| stopFnRef.current?.(); | |
| } | |
| }, []); | |
| // Cancel handler for the sidebar | |
| const handleCancel = useCallback(() => { | |
| if (!stopFnRef.current) { | |
| cancelRequestedRef.current = true; | |
| return; | |
| } | |
| stopFnRef.current(); | |
| }, []); |
🤖 Prompt for AI Agents
In @client/src/components/ui-playground/UIPlaygroundTab.tsx around lines 125 -
138, handleCancel currently sets isExecuting=false unconditionally which can
desync when stopFnRef.current is null; change handleCancel so it only flips
setIsExecuting(false) when a stop function was actually invoked (i.e., check
stopFnRef.current before calling it) or enqueue a cancel request to be processed
by PlaygroundMain (e.g., call a provided queueCancel/notifyCancel callback) and
let PlaygroundMain clear isExecuting via the existing execution completion path;
update the handleCancel implementation to check stopFnRef.current, call it if
present and then setIsExecuting(false), otherwise signal/queue cancellation
instead of directly setting isExecuting.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Loading early allows us to send cancellation notifications to the widget during execution | ||
| useEffect(() => { | ||
| if (toolState !== "output-available") return; | ||
| if (toolState !== "input-available" && toolState !== "output-available") return; |
There was a problem hiding this comment.
Widget fetch missing output-error state causes infinite loading
Medium Severity
The rendering condition at line 976 was expanded to allow output-error state (in addition to input-available and output-available), but the widget HTML fetch condition at line 348 only triggers for input-available or output-available. If a tool's state goes directly to output-error without passing through input-available, the widget HTML fetch never starts, but rendering proceeds past the early-return check, causing the component to show "Preparing MCP App widget..." indefinitely since widgetHtml will never be populated.
Additional Locations (1)
| const handleCancel = useCallback(() => { | ||
| stopFnRef.current?.(); | ||
| setIsExecuting(false); | ||
| }, [setIsExecuting]); |
There was a problem hiding this comment.
Cancel button doesn't stop deterministic tool execution
Medium Severity
The handleCancel callback sets isExecuting to false and calls the AI streaming stop function, but during deterministic tool execution (via the Run button), there's no AI streaming to stop. The executeToolApi call in useToolExecution continues running in the background. The Cancel button appears when isExecuting is true (deterministic execution), but clicking it only hides the loading state while the API call completes and results still get injected into the chat. This creates misleading UX where users think they cancelled but the operation continues.
| // Loading early allows us to send cancellation notifications to the widget during execution | ||
| useEffect(() => { | ||
| if (toolState !== "output-available") return; | ||
| if (toolState !== "input-available" && toolState !== "output-available") return; |
There was a problem hiding this comment.
Missing cancellation ref reset on CSP mode change
Low Severity
The new lastToolCancelledRef is reset when toolCallId changes (line 934), but it's not reset in the existing CSP mode change effect (lines 450-458) alongside the other refs. When CSP mode changes, the widget reloads as a new iframe instance. The other refs (lastToolInputRef, lastToolOutputRef, lastToolErrorRef) are reset so data can be re-sent to the new widget, but lastToolCancelledRef remains true if the tool was cancelled. This prevents the cancellation notification from being re-sent to the newly loaded widget, leaving it unaware that the tool was cancelled.
Note
Introduces end-to-end tool cancellation and early widget loading for MCP Apps.
stopto record in-progress tool call IDs as cancelled; resets on chat resetcancelledToolIdsthroughChatTabV2→Thread→MessageView/PartSwitchto renderersinput-available, sendssendToolCancelled(SEP-1865), prevents duplicates, shows cancelled UI state, and tweaks loading copy to “Waiting for tool input...”stopto parent (onStopReady), adds Cancel button in sidebar header, and wires it to stop/cancel in-flight toolsWritten by Cursor Bugbot for commit b350fee. This will update automatically on new commits. Configure here.