feat(mcp): add progress bar, throttling, and input validation for MCP tool progress#19772
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 for long-running Model Context Protocol (MCP) tool calls by providing a more robust and visually appealing progress reporting mechanism. It addresses potential performance issues with frequent updates and improves data integrity by validating incoming progress information, ensuring a smoother and more reliable display of tool execution status. 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
|
There was a problem hiding this comment.
Code Review
The pull request introduces a visual progress bar, throttling, and input validation for MCP tool progress reporting. The changes are well-tested with new unit tests covering various scenarios. The implementation correctly integrates the new McpProgressIndicator component and handles progress updates with throttling and validation. The ToolMessage component now uses the new progress indicator, and the Scheduler correctly processes and throttles progress events. The CoreEventEmitter also includes validation for McpProgressPayload to ensure data integrity, aligning with the principle of treating user-provided data as untrusted. Overall, the changes enhance the user experience by providing clear progress feedback and improve the robustness of the system by validating incoming progress data and preventing UI lag.
|
@jacob314 - For your review. Thanks |
96e7846 to
9e805b6
Compare
9e805b6 to
4516447
Compare
jacob314
left a comment
There was a problem hiding this comment.
This is an excellent visual improvement for long-running MCP tools! The implementation correctly calculates the progress dimensions, properly bounds values with safety checks, and implements a solid throttle in Scheduler to preserve Ink UI performance under high-frequency updates.
I’ve verified the diffs and ran npm run preflight which completed successfully with all tests passing.
Here are a few minor findings/nitpicks that do not block approval:
Minor Improvements / Nitpicks
- Unused
progressPercenttype field: Inpackages/cli/src/ui/types.tsandpackages/cli/src/ui/hooks/toolMapping.ts, theprogressPercentproperty is mapped and defined but is no longer consumed by any UI component (removed fromToolMessageandToolShared). You can safely remove it from the UI layer to clean up the type definition if desired. - Top Margin in Progress Bar Container:
McpProgressIndicatorhasmarginTop={1}on its rootBox. Because the container border box generally lacks apaddingY, this causes the progress bar to sit one line lower inside the top border compared to standardToolResultDisplaytext output. It looks fine, but could be adjusted if strict visual alignment with standard tool output is desired. - Indeterminate Jumpiness: The modulo math (
Math.floor(progress) % (barWidth + 1)) works beautifully for progress values that increment monotonically (e.g., 1, 2, 3... steps). If an MCP server sends progress as arbitrary raw byte counts without a total (e.g., jumping from 1024 to 4096), the bar will appear to jump around randomly. Given the MCP spec, this is acceptable, but something to be aware of for arbitrary tools.
Great work on the tests and defensive input validation (Number.isFinite) too!
4516447 to
1f2d3b2
Compare
|
Thanks @jacob314 |
1f2d3b2 to
11d54dc
Compare
| private isProcessing = false; | ||
| private isCancelling = false; | ||
| private readonly requestQueue: SchedulerQueueItem[] = []; | ||
| private readonly progressThrottles = new Map< |
There was a problem hiding this comment.
please revert the throttle logic from core. If there is a really a performance issue here it should be addressed on the package/cli side throttling the rendering rather than filtering the events. the logic for throttling here seems a bit fragile and hard to get right.
jacob314
left a comment
There was a problem hiding this comment.
approved once the scheduler changes are reverted. Don't think that is the right layer for UI specific throttling.
|
Some thoughts from Gemini about removing the scheduler logic: Here is what the vastly simplified code would look like in scheduler.ts: 10 } What this improves:
|
… tool progress Enhances MCP progress reporting (layered on google-gemini#19046) with: - Visual progress bar component (McpProgressIndicator) using block characters, supporting determinate (percentage) and indeterminate (step count) modes - Per-callId leading+trailing throttle (100ms) in Scheduler to prevent UI lag from high-frequency MCP server progress events - Percentage capping at 100% for misbehaving servers - Input validation in emitMcpProgress rejecting NaN/negative/Infinity - Raw progress/progressTotal fields propagated through the pipeline for bar fill computation Fixes google-gemini#16934
11d54dc to
fd451be
Compare
|
@jacob314 - Thanks for the feedback. I went with removing the throttle entirely rather than the timestamp-based drop suggested by Gemini. The drop approach simplifies the code but still puts UI-specific throttling in Core's scheduler, which doesn't fully address your feedback about it being the wrong layer. Since Ink already batches terminal writes and my manual testing with rapid events (200 at 10ms intervals) showed no lag, this can possibly be addressed later. Now the PR only addresses Visual Progress Bar and Input validation for MCP Tool call progress. |

Follow-up to #19046 to add enhancements to the long-running MCP tool call progress feature.
Fixes #16934
Summary
Adds a visual progress bar, throttling, input validation, and percentage capping to MCP tool progress reporting.
Feature Details
McpProgressIndicator, rendered inside the tool's bordered content box. Replaces feat(core): add support for MCP progress updates #19046's inline text.scheduler.tsto prevent UI lag from high-frequency progress events.Math.min(100, ...)for servers reportingprogress > total.Number.isFinite()+>= 0guard inemitMcpProgressto reject NaN/negative/Infinity values.progress/progressTotaltoExecutingToolCalland mapped through to UI, since the bar needs raw values to compute fill width.Related Issues
Fixes #16934
How to Validate
Manual
Configure an MCP server that sends progress notifications (e.g.,
@modelcontextprotocol/server-everything):{ "mcpServers": { "everything": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-everything"] } } }Run
npm startand prompt: "Use the longRunningOperation tool with duration 10 and steps 5"Testing done
Pre-Merge Checklist