feat(mcp): add live MCP tool progress bar for long running MCP tool calls (Issue #16934)#18604
feat(mcp): add live MCP tool progress bar for long running MCP tool calls (Issue #16934)#18604jasmeetsb wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @jasmeetsb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by integrating real-time progress indicators for MCP tool executions within the CLI. By establishing a robust event-driven mechanism from the MCP SDK to the UI, users can now observe the live status of operations, whether they are percentage-based or step-based. This change improves transparency and responsiveness for long-running tasks without altering existing user-facing APIs. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a real-time progress bar for long-running MCP tool calls, which is a great enhancement for user experience. The implementation is well-structured, spanning from the core event system to the CLI's UI layer with a new React hook and component. The changes are accompanied by a comprehensive set of new tests. I've identified one area for improvement in a useEffect hook that could be made more robust to prevent potential dependency cycles.
There was a problem hiding this comment.
Code Review
This pull request introduces a progress bar for long-running MCP tool calls, significantly enhancing user experience by providing real-time updates. While the implementation is generally well-structured, robust, and includes comprehensive unit tests, a high-severity Denial of Service vulnerability was identified. The progress bar rendering logic does not adequately validate numeric progress values from external MCP servers. This oversight could allow a malicious or misbehaving server to provide NaN values, triggering an unhandled RangeError and crashing the CLI. To address this, ensure all values used for UI rendering are finite and valid.
b877feb to
34b445a
Compare
| // Always call clearProgress unconditionally so late progress events are | ||
| // rejected even for calls that never received progress. | ||
| // | ||
| // This useEffect IS appropriate — it synchronizes the external |
There was a problem hiding this comment.
remove the use effect comment.
There was a problem hiding this comment.
done. Went further and removed the useEffect entirely, moved the clearProgress logic into the TOOL_CALLS_UPDATE messageBus handler as you suggested in your general comment.
| const { tool, invocation } = call; | ||
|
|
||
| // Set callId for MCP progress tracking | ||
| if ( |
There was a problem hiding this comment.
this is ugly. Would suggest suggest refactoring ToolInvocation to have an optional setCallId method or another mechanism to avoid having to do these checks.
There was a problem hiding this comment.
Done.
Added setCallId?(callId: string): void to the ToolInvocation interface in tools.ts.
| {message} | ||
| </Text> | ||
| )} | ||
| </Box> |
There was a problem hiding this comment.
write a couple snapshot based tests to validate this rendering logic.
There was a problem hiding this comment.
Done. Added 6 snapsht tests in ToolShared.test.tsx
| total, | ||
| message, | ||
| }) => { | ||
| const barWidth = 20; |
There was a problem hiding this comment.
make barWidth a required parameter and pass in a value dependent on the width available when displaying the tool message. Might as well make this a bit larger than 20 chars when the terminal is wide and make sure we don't have wrapping issues when it is small
There was a problem hiding this comment.
Changed it. barWidth is now a required prop. calculated in ToolMessage.tsx as 40% of terminal width, clamped between 10 and 40
| paddingX={1} | ||
| flexDirection="column" | ||
| > | ||
| {status === ToolCallStatus.Executing && mcpProgress && ( |
There was a problem hiding this comment.
add a snapshot based test that includes a progress indicator. will ensure progress indicators aren't accidentally removed as part of future UI changes.
There was a problem hiding this comment.
Done. Added a snapshot test in ToolMessage.test.tsx
|
These comments were from /review-frontend audited by Jacob. Great work on this feature! The implementation of real-time progress for MCP tools is a valuable addition. I have a few suggestions to improve code quality and align with project standards:
Please address these points to ensure the implementation is robust and follows our React patterns. |
jacob314
left a comment
There was a problem hiding this comment.
Looks good. Approved once these fairly minor comments are addressed.
34b445a to
7469700
Compare
|
Thanks for the review! I noticed that upstream #18567 deleted useToolExecutionScheduler.ts (consolidated into useToolScheduler.ts), so I'll port the progress integration to the new file and address all the feedback as part of next push. Thanks. |
527ef25 to
0b64680
Compare
|
@jacob314 - Thanks for all the feedback. I think I have addressed it all(see comments above). Upstream #18567 deleted some files, so I ported progress integration from the deleted useToolExecutionScheduler.ts to the consolidated useToolScheduler.ts. Please let me know if there is any additional feedback. |
0b64680 to
5f0157e
Compare
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { |
There was a problem hiding this comment.
did you mean to do something here?
|
This comment was created with This is a great feature, but the current implementation splits the state management across The Architectural MismatchThe PR adds
Why this is problematic
The Recommended FixPlease move this logic into
A Note on Performance (Throttling)Some MCP servers might emit Additional Minor Feedback
|
5f0157e to
237f6eb
Compare
|
@jacob314 - Pushed a new commit addressing your architectural feedback:
|
Display real-time progress bars for MCP tool operations by wiring SDK onprogress callbacks through core events to the CLI UI layer. Closes google-gemini#16934
Move MCP progress tracking into StateManager/Scheduler so all clients get progress via TOOL_CALLS_UPDATE. Delete useMCPProgress hook, fix CallableToolWithProgress cast, cap percentage at 100%.
237f6eb to
79a33f6
Compare
| /** | ||
| * Payload for the 'mcp-tool-progress' event. | ||
| */ | ||
| export interface MCPToolProgressPayload { |
There was a problem hiding this comment.
nit: comments about params being optional are not needed as they are implied from the type.
Display real-time progress bars for MCP tool operations by wiring SDK onprogress callbacks through core events to the CLI UI layer.
Closes Feature Request #16934
Summary
Adds MCP progress report support per the MCP specification (notifications/progress). MCP servers can now emit progress updates during tool execution, which are displayed as a live progress bar in the terminal. Supports both determinate (percentage with known total) and indeterminate (step count) progress modes.
Screenshot
Details
Core layer (
packages/core):mcp-client.ts: Generates aprogressTokenper tool call and passes anonprogresscallback to the MCP SDK'scallTool. Progress notifications are forwarded viacoreEvents.emitMCPToolProgress()mcp-tool.ts: Accepts progress callback from the client layer, wires it to the invocation'sexecute()method, and emits typedMCPToolProgressevents with call correlationtool-executor.ts: Assigns acallIdto MCP tool invocations before execution so progress events can be correlated to specific tool callsevents.ts: NewMCPToolProgressevent type andemitMCPToolProgresshelperCLI layer (
packages/cli):useMCPProgress.ts: Hook that subscribes toMCPToolProgresscore events and maintains aMap<callId, progress>stateuseToolExecutionScheduler.ts: Integrates progress state, passescallIdmappings andclearProgresscallback to tool message componentsToolShared.tsx: Renders a block-character progress bar with percentage and message below the tool spinnerToolMessage.tsx: Passes progress data through toToolShared15 files changed, 803 insertions(+), 8 deletions(-).
Related Issues
Closes Feature Request #16934
How to Validate
All existing tests pass (
npm test— 622 test files, 9750 tests, 0 failures across core, cli, and vscode packages). 17 new tests were added covering the progress functionality.notifications/progress(e.g. (https://mcpservers.org/servers/dazeb/mcp-everything)) and invoke one of its tools:Pre-Merge Checklist