feat: add MCP progress notification support for long-running tools (#2) #905
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull Request Overview
This PR adds MCP (Model Context Protocol) progress notification support for long-running tools, enabling clients to receive real-time updates during operations like browser installation to prevent timeouts and improve user experience.
Key changes:
- Adds progress notification callback system to the ServerBackend interface
- Implements periodic progress updates in the browser_install tool with indeterminate progress cycling
- Updates Response class to support sending progress notifications to MCP clients
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/mcp/server.ts | Adds ProgressCallback type and progress notification handling in MCP request handler |
| src/browserServerBackend.ts | Updates callTool method to accept and pass progress callback to Response |
| src/response.ts | Adds sendProgress method to enable tools to send progress notifications |
| src/tools/install.ts | Implements periodic progress updates with cycling indeterminate progress |
| src/mcp/proxyBackend.ts | Updates callTool signature to accept progress callback (unused) |
| src/loopTools/main.ts | Updates callTool signature to accept progress callback (unused) |
| tests/install.spec.ts | Adds tests for progress notification functionality and backward compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/tools/install.ts
Outdated
| }); | ||
| }); | ||
| } finally { | ||
| clearInterval(progressInterval); |
There was a problem hiding this comment.
clearInterval is called with potentially undefined value. When progressInterval is undefined (when response.sendProgress is falsy), clearInterval(undefined) is called, which is safe but could be more explicit.
| clearInterval(progressInterval); | |
| if (progressInterval !== undefined) { | |
| clearInterval(progressInterval); | |
| } |
Copilot uses AI. Check for mistakes.
|
@pavelfeldman Was hoping you or another team member might be able to give this a review. |
src/tools/install.ts
Outdated
| else | ||
| reject(new Error(`Failed to install browser: ${output.join('')}`)); | ||
|
|
||
| // Send periodic progress notifications (indeterminate progress) |
There was a problem hiding this comment.
What's is the value of the progress if it is timer-based? Does your client time out? What is the client. It is always best to start with filing the issue. If now adequate progress is needed and you are only fighting the timeout, you can implement it in server.ts entirely w/o touching anything else.
There was a problem hiding this comment.
@pavelfeldman I moved the implementation entirely in to server.ts now, let me know what you think!
VikashLoomba
force-pushed
the
main
branch
4 times, most recently
from
ed456c4 to
0aa307f
Compare
Implement server-level progress notifications that automatically send periodic updates when a client provides a progressToken, preventing timeouts during long-running operations like browser installation.
| progress: progressValue, | ||
| }, | ||
| }); | ||
| progressValue = (progressValue + 1) % 100; |
There was a problem hiding this comment.
This is unexpected, why going back?
| serverDebug('Progress notification failed:', error); | ||
| } | ||
| }; | ||
| progressTimeout = setTimeout(sendPeriodicProgress, 5000); |
There was a problem hiding this comment.
Avoid passing async methods into sync handers, remove async/await and do extra.sendNotification().then(...).catch(logUnhandledError); fire-and-forget style
| try { | ||
| return await backend.callTool(request.params.name, request.params.arguments || {}); | ||
| } catch (error) { | ||
| return { | ||
| content: [{ type: 'text', text: '### Result\n' + String(error) }], | ||
| isError: true, | ||
| }; | ||
| } finally { | ||
| if (progressTimeout !== null) |
There was a problem hiding this comment.
You don't need if, clearTimeout tolerates cleared and null timers.
|
Closing as stale |
Add automatic progress notifications for timeout prevention
Implement server-level progress notifications that automatically send
periodic updates when a client provides a progressToken, preventing
timeouts during long-running operations like browser installation.
Fixes #911