Skip to content

fix: prevent infinite loop of tool-input notifications in MCP Apps #6374

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
aharvard merged 3 commits into from
Jan 7, 2026
Merged

fix: prevent infinite loop of tool-input notifications in MCP Apps #6374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67f4cbb
fix: prevent infinite loop of tool-input notifications in MCP Apps
aharvard Jan 7, 2026
7dac857
fix: prevent infinite loop of MCP App notifications
aharvard Jan 7, 2026
3aa59a3
fix: prevent infinite loop of MCP App notifications
aharvard Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion ui/desktop/src/components/McpApps/useSandboxBridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,21 @@ export function useSandboxBridge(options: SandboxBridgeOptions): SandboxBridgeRe

case 'ui/notifications/initialized':
isGuestInitializedRef.current = true;
// Send any pending tool data that arrived before initialization
if (toolInput) {
sendToSandbox({
jsonrpc: '2.0',
method: 'ui/notifications/tool-input',
params: { arguments: toolInput.arguments },
});
}
if (toolResult) {
sendToSandbox({
jsonrpc: '2.0',
method: 'ui/notifications/tool-result',
params: toolResult,
});
}
break;

case 'ui/notifications/size-changed': {
Expand Down Expand Up @@ -163,7 +178,16 @@ export function useSandboxBridge(options: SandboxBridgeOptions): SandboxBridgeRe
}
}
},
[resourceHtml, resourceCsp, resolvedTheme, sendToSandbox, onMcpRequest, onSizeChanged]
[
resourceHtml,
resourceCsp,
resolvedTheme,
sendToSandbox,
onMcpRequest,
onSizeChanged,
toolInput,
toolResult,
Comment on lines +188 to +189
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

Adding toolInput and toolResult to the handleJsonRpcMessage dependency array causes the callback to be recreated on every change, which triggers the message listener to be re-registered (line 200). This is inefficient and could cause the callback to access stale values. Since these values are only used in the 'ui/notifications/initialized' case and are accessed from the closure, consider using refs for toolInput and toolResult instead, or remove them from the dependency array and accept that they'll be captured from the closure at creation time.

Suggested change
toolInput,
toolResult,

Copilot uses AI. Check for mistakes.
]
);

useEffect(() => {
Expand Down
59 changes: 43 additions & 16 deletions ui/desktop/src/components/ToolCallWithResponse.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ToolIconWithStatus, ToolCallStatus } from './ToolCallStatusIndicator';
import { getToolCallIcon } from '../utils/toolIconMapping';
import React, { useEffect, useRef, useState } from 'react';
import React, { useEffect, useRef, useState, useMemo } from 'react';
import { Button } from './ui/button';
import { ToolCallArguments, ToolCallArgumentValue } from './ToolCallArguments';
import MarkdownContent from './MarkdownContent';
Expand Down Expand Up @@ -71,12 +71,19 @@ function isEmbeddedResource(content: Content): content is EmbeddedResource {
return 'resource' in content && typeof (content as Record<string, unknown>).resource === 'object';
}

function maybeRenderMCPApp(
toolRequest: ToolRequestMessageContent,
toolResponse: ToolResponseMessageContent | undefined,
sessionId: string,
append?: (value: string) => void
): React.ReactNode {
interface McpAppWrapperProps {
toolRequest: ToolRequestMessageContent;
toolResponse?: ToolResponseMessageContent;
sessionId: string;
append?: (value: string) => void;
}

function McpAppWrapper({
toolRequest,
toolResponse,
sessionId,
append,
}: McpAppWrapperProps): React.ReactNode {
const requestWithMeta = toolRequest as ToolRequestWithMeta;
let resourceUri = requestWithMeta._meta?.['ui/resourceUri'];

Expand All @@ -87,24 +94,37 @@ function maybeRenderMCPApp(
}
}

if (!resourceUri) return null;
if (requestWithMeta.toolCall.status !== 'success') return null;
const extensionName =
requestWithMeta.toolCall.status === 'success'
? requestWithMeta.toolCall.value.name.split('__')[0]
: '';

const toolArguments =
requestWithMeta.toolCall.status === 'success'
? requestWithMeta.toolCall.value.arguments
: undefined;

const extensionName = requestWithMeta.toolCall.value.name.split('__')[0];
// Memoize toolInput to prevent unnecessary re-renders
const toolInput = useMemo(() => ({ arguments: toolArguments || {} }), [toolArguments]);
Comment on lines +107 to +108
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The useMemo dependency array for toolInput uses toolArguments, but toolArguments is extracted from toolRequest.toolCall.value.arguments which is itself an object reference that may change on every render. This doesn't actually solve the infinite loop problem. Consider using JSON.stringify(toolArguments) in the dependency array or depending on toolRequest directly with a comparison based on the stringified arguments value.

Suggested change
// Memoize toolInput to prevent unnecessary re-renders
const toolInput = useMemo(() => ({ arguments: toolArguments || {} }), [toolArguments]);
const toolArgumentsString = useMemo(
() => (toolArguments ? JSON.stringify(toolArguments) : ''),
[toolArguments],
);
// Memoize toolInput to prevent unnecessary re-renders
const toolInput = useMemo(
() => ({ arguments: toolArguments || {} }),
[toolArgumentsString],
);

Copilot uses AI. Check for mistakes.

let toolResult: CallToolResponse | undefined;
if (toolResponse) {
// Memoize toolResult to prevent unnecessary re-renders
const toolResult = useMemo(() => {
if (!toolResponse) return undefined;
const resultWithMeta = toolResponse.toolResult as ToolResultWithMeta;
if (resultWithMeta?.status === 'success' && resultWithMeta.value) {
toolResult = resultWithMeta.value;
return resultWithMeta.value;
}
}
return undefined;
}, [toolResponse]);
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The useMemo dependency array for toolResult uses toolResponse, but toolResponse is an object that may have the same content with a different reference on each render. This doesn't prevent re-renders. Consider using JSON.stringify(toolResponse) in the dependency array or extracting the actual result value and comparing that.

Suggested change
}, [toolResponse]);
}, [JSON.stringify(toolResponse?.toolResult ?? null)]);

Copilot uses AI. Check for mistakes.

if (!resourceUri) return null;
if (requestWithMeta.toolCall.status !== 'success') return null;

return (
<div className="mt-3">
<McpAppRenderer
resourceUri={resourceUri}
toolInput={{ arguments: requestWithMeta.toolCall.value.arguments || {} }}
toolInput={toolInput}
toolResult={toolResult}
extensionName={extensionName}
sessionId={sessionId}
Expand Down Expand Up @@ -181,7 +201,14 @@ export default function ToolCallWithResponse({
}
})}

{sessionId && maybeRenderMCPApp(toolRequest, toolResponse, sessionId, append)}
{sessionId && (
<McpAppWrapper
toolRequest={toolRequest}
toolResponse={toolResponse}
sessionId={sessionId}
append={append}
/>
)}
</>
);
}
Expand Down
Loading