-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
Conversation
Fixes #6373 The issue was caused by React's useEffect dependency on object references. When toolInput or toolResult props are passed as inline objects (e.g., `toolInput={{ arguments: ... }}`), a new object reference is created on every render. This triggered the useEffect to run repeatedly, sending duplicate notifications in an infinite loop. The fix uses refs to track the last sent values (via JSON stringification) and only sends notifications when the actual content changes, not just the object reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an infinite loop bug where MCP Apps sandbox bridge was repeatedly sending ui/notifications/tool-input and ui/notifications/tool-result notifications. The root cause was React useEffect dependencies triggering on object reference changes rather than value changes.
Key changes:
- Added ref-based tracking to prevent duplicate notifications using JSON stringification for deep equality comparison
- Updated initialization handler to check for duplicates before sending pending tool inputs/results
- Reset tracking refs when resource URI changes to avoid stale state
|
|
||
| // Track last sent values to prevent duplicate notifications | ||
| const lastSentToolInputRef = useRef<string | null>(null); | ||
| const lastSentToolResultRef = useRef<string | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useRef seems overkill here; if we wanted to do it this way, we can just serialize on compare
| if (!isGuestInitializedRef.current || !toolResult) return; | ||
| const serialized = JSON.stringify(toolResult); | ||
| if (serialized === lastSentToolResultRef.current) return; | ||
| lastSentToolResultRef.current = serialized; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have three times the same code, but we're only fixing it twice; presumably toolInputPartial needs this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, good catch
| }, [handleJsonRpcMessage]); | ||
|
|
||
| // Send tool input notification when it changes | ||
| // Use JSON stringification to compare values and prevent duplicate notifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the way. Can't we memoize the inputs to this component instead so the values don't change on re-rerender? that seems cleaner and easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. will pivot to that approach.
e61de76 to
496d2d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| () => toolInput, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolInput)] | ||
| ); | ||
| const memoizedToolInputPartial = useMemo( | ||
| () => toolInputPartial, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolInputPartial)] | ||
| ); | ||
| const memoizedToolResult = useMemo( | ||
| () => toolResult, |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useMemo implementation doesn't prevent the infinite loop. While useMemo tracks changes via JSON.stringify, it returns the original object reference (toolInput), which creates a new reference on every parent render. The useEffect in useSandboxBridge (line 186) still triggers repeatedly because it depends on the object reference, not the memoized value's stability. This approach adds overhead without fixing the root cause.
| () => toolInput, | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [JSON.stringify(toolInput)] | |
| ); | |
| const memoizedToolInputPartial = useMemo( | |
| () => toolInputPartial, | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [JSON.stringify(toolInputPartial)] | |
| ); | |
| const memoizedToolResult = useMemo( | |
| () => toolResult, | |
| () => (toolInput != null ? structuredClone(toolInput) : undefined), | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [JSON.stringify(toolInput)] | |
| ); | |
| const memoizedToolInputPartial = useMemo( | |
| () => (toolInputPartial != null ? structuredClone(toolInputPartial) : undefined), | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [JSON.stringify(toolInputPartial)] | |
| ); | |
| const memoizedToolResult = useMemo( | |
| () => (toolResult != null ? structuredClone(toolResult) : undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. good point from copilot @aharvard - doing it this way would not really be better than just having the JSON.stringify in the dependencies of where we use this though so I think we have to go further up the stack
toolInput={{ arguments: requestWithMeta.toolCall.value.arguments || {} }}
seems like a likely culprit. when you think about it, the conversation should be stable. it does feel iffy though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DOsinga Updated per your feedback - moved the fix higher up the stack. Converted maybeRenderMCPApp to a proper component (McpAppWrapper) and added useMemo there to memoize toolInput and toolResult before passing them down. This keeps useSandboxBridge clean and addresses the root cause at the source.
Ready for another look when you have a chance!
| [JSON.stringify(toolInput)] | ||
| ); | ||
| const memoizedToolInputPartial = useMemo( | ||
| () => toolInputPartial, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolInputPartial)] | ||
| ); | ||
| const memoizedToolResult = useMemo( | ||
| () => toolResult, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolResult)] |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.stringify is called on every render in the dependency array. For complex tool objects with nested data, this creates unnecessary serialization overhead on every render cycle, even when the objects haven't changed.
| // Memoize tool props to prevent unnecessary re-renders and duplicate notifications | ||
| // These props are often passed as inline objects which create new references on each render | ||
| const memoizedToolInput = useMemo( | ||
| () => toolInput, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolInput)] | ||
| ); | ||
| const memoizedToolInputPartial = useMemo( | ||
| () => toolInputPartial, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolInputPartial)] | ||
| ); | ||
| const memoizedToolResult = useMemo( | ||
| () => toolResult, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [JSON.stringify(toolResult)] | ||
| ); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states the solution uses refs (lastSentToolInputRef and lastSentToolResultRef) to track previously sent values in useSandboxBridge.ts, but the actual implementation uses useMemo in McpAppRenderer.tsx instead. This mismatch suggests either the wrong approach was implemented or the description needs updating.
Fixes #6373 The toolInput, toolInputPartial, toolResult, and toolCancelled props were being created as new object references on every render, causing useEffect hooks in useSandboxBridge to re-trigger unnecessarily. Memoize these props in McpAppRenderer using useMemo with JSON.stringify in the dependency array to ensure stable references when content hasn't changed.
496d2d8 to
7dac857
Compare
Fixes #6373 Memoize toolInput and toolResult in McpAppWrapper component to ensure stable object references. Send pending tool data when guest initializes to handle timing issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| // Memoize toolInput to prevent unnecessary re-renders | ||
| const toolInput = useMemo(() => ({ arguments: toolArguments || {} }), [toolArguments]); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
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.
| // 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], | |
| ); |
| } | ||
| } | ||
| return undefined; | ||
| }, [toolResponse]); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
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.
| }, [toolResponse]); | |
| }, [JSON.stringify(toolResponse?.toolResult ?? null)]); |
| toolInput, | ||
| toolResult, |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
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.
| toolInput, | |
| toolResult, |
|
Gonna merge. As a reminder, we'll be able to delete a bunch of related code when this ships in the client SDK MCP-UI-Org/mcp-ui#147 (might be available for us to use as soon as this week... gonna keep my eye on it) |
* 'main' of github.com:block/goose: Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) fix: prevent infinite loop of tool-input notifications in MCP Apps (#6374)
* main: (31 commits) added validation and debug for invalid call tool result (#6368) Update MCP apps tutorial: fix _meta structure and version prereq (#6404) Fixed fonts (#6389) Update confidence levels prompt injection detection to reduce false positive rates (#6390) Add ML-based prompt injection detection (#5623) docs: update custom extensions tutorial (#6388) fix ResultsFormat error when loading old sessions (#6385) docs: add MCP Apps tutorial and documentation updates (#6384) changed z-index to make sure the search highlighter does not appear on modal overlay (#6386) Handling special claude model response in github copilot provider (#6369) fix: prevent duplicate rendering when tool returns both mcp-ui and mcp-apps resources (#6378) fix: update MCP Apps _meta.ui.resourceUri to use nested format (SEP-1865) (#6372) feat(providers): add streaming support for Google Gemini provider (#6191) Blog: edit links in mcp apps post (#6371) fix: prevent infinite loop of tool-input notifications in MCP Apps (#6374) fix: Show platform-specific keyboard shortcuts in UI (#6323) fix: we load extensions when agent starts so don't do it up front (#6350) docs: credit HumanLayer in RPI tutorial (#6365) Blog: Goose Lands MCP Apps (#6172) Claude 3.7 is out. we had some harcoded stuff (#6197) ...
Summary
Fixes #6373
The MCP Apps sandbox bridge was sending an infinite loop of
ui/notifications/tool-inputnotifications after receiving a tool result, causing re-rendering issues.For testing, I used my demo mcp app: https://mcp-app-bench.onrender.com/mcp
Before
Note the debug log panel on the right. Tool input is received many times when it should not be.

After
Note the debug panel on the right. tool input and tool results are received once.

Root Cause
The issue was caused by two problems:
Unstable object references: When
toolInput,toolInputPartial, ortoolResultprops are passed as inline objects, a new object reference is created on every render. This triggered useEffect hooks to run repeatedly, sending duplicate notifications.Timing issue: The useEffect hooks for sending tool data run before the guest iframe sends
ui/notifications/initialized, so the initial tool data was never sent.Solution
Memoize props in
McpAppRenderer.tsxAdded
useMemowithJSON.stringifyin the dependency array fortoolInput,toolInputPartial, andtoolResultto ensure stable object references when content has not changed.Send pending data on initialization in
useSandboxBridge.tsWhen the guest sends
ui/notifications/initialized, we now check for any pendingtoolInputortoolResultand send them immediately.Testing