Skip to content

Conversation

@nathanArseneau
Copy link
Contributor

@nathanArseneau nathanArseneau commented Mar 17, 2025

This fixes #190

Motivation and Context

Previously, not all notifications were displayed, leading to potential missed updates and crucial information.

How Has This Been Tested?

On any notification
image
Error warning no impact / change
image

Breaking Changes

Nope

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@olaservo
Copy link
Member

Thanks for making changes. Did you get a chance to test this after switching to use the specific notification types? For a similar PR we had been using server-everything to simulate the added notifications, so I'm curious how the other types of notifications are being tested. (I'm planning to add more automated testing in the near future since it can be awkward to find the right combination of servers and conditions to test stuff like this manually.)

@cliffhall
Copy link
Member

cliffhall commented Mar 25, 2025

It looks like we may need to add handling for ToolListChangedNotificationSchema and CancelledNotificationSchema to the everything server to test these messages. We could probably find other servers to test this with, but I find none in the official servers project.

@cliffhall cliffhall assigned cliffhall and unassigned cliffhall Mar 26, 2025
cliffhall

This comment was marked as duplicate.

@cliffhall
Copy link
Member

I previously said

It looks like we may need to add handling for ToolListChangedNotificationSchema, CancelledNotificationSchema to the everything server to test these messages. We could probably find other servers to test this with, but I find none in the official servers project.

But, @olaservo you mentioned automated testing, so I'm not certain putting these in the everything server is the right approach.

Well, we could send periodic ToolListChangedNotificationSchema messages but it would be weird. We wouldn't actually change the tools, it would just prove they could get through to the inspector but also adding noise in the notifications pane.

But it would be useful to make the long running process simulation in the everything server be cancellable. Maybe that's a separate PR since the UI would need to be able to track that long running process id and give you a cancel button while it's running by monitoring progress notifications. What do you think?

@cliffhall cliffhall self-requested a review March 26, 2025 19:49
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@cliffhall cliffhall requested a review from olaservo March 26, 2025 21:44
@seuros
Copy link
Contributor

seuros commented Mar 28, 2025

can we have this merged ?

@olaservo olaservo merged commit 205e707 into modelcontextprotocol:main Mar 28, 2025
2 checks passed
@olaservo
Copy link
Member

Thanks @cliffhall for thinking through the testing approach here. I was tinkering with some sort of integration tests too earlier on which would simulate some of these scenarios.

But it would be useful to make the long running process simulation in the everything server be cancellable. Maybe that's a separate PR since the UI would need to be able to track that long running process id and give you a cancel button while it's running by monitoring progress notifications. What do you think?

Yes agree that seems like something that would be worthwhile for providing a 'live' example of cancelling, as a separate PR.

@cliffhall
Copy link
Member

Added an issue to the servers repo for handling cancellation. Will need to add one to this repo as well, for tracking the id and exposing a cancel button.

IgnacioC44 referenced this pull request in MCPJam/inspector Jun 21, 2025
Refactor notification handling to include all notifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show all notifications from the server in the UI, even if not explicitly handled

4 participants