Skip to content

Support Progress Flow for McpClient #389

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

Closed

Conversation

134130
Copy link
Contributor

@134130 134130 commented Jul 12, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

  • No

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

@134130 134130 marked this pull request as draft July 12, 2025 01:38
@tzolov
Copy link
Contributor

tzolov commented Jul 15, 2025

@134130 looks like this work overlaps with the #300?

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

@134130 ,
I left few comments. Also please do not try to format the McpSchema. Apply only your changes.
Also it looks like it would be easier to start over in a new PR on top of the latest origin/main and apply copy on only the meaningful changes from (to replace #389 and #300)
What do you think?

* @param progressNotification The progress notification to send
* @return A Mono that completes when the notification has been sent
*/
public Mono<Void> progressNotification(McpSchema.ProgressNotification progressNotification) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be mirrored in the McpSyncServerExchange

@@ -23,6 +23,7 @@
import io.modelcontextprotocol.util.Assert;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import reactor.util.annotation.Nullable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the reactor.util.annotation.Nullable;

@134130
Copy link
Contributor Author

134130 commented Jul 15, 2025

@tzolov Thank you for your comment!

Yes, the PR depends on #300 and will be rebased after the #300 is merged.

I've tried to separate the PR for Server and Client implementation, as the #298 task list.

How do you think about this? Do you think separating the PR is meaningful? And do you want a new PR with closing #300 and #389?

About McpSchema formatting

Of course! I see the #392, and I can rework to reduce the changes.

@tzolov
Copy link
Contributor

tzolov commented Jul 16, 2025

And do you want a new PR with closing #300 and #389?

Yes, please create a new PR closing #300 and #389. Given the significant code changes, it'll be simpler to copy the important changes into a clean PR rather than handling multiple merges and conflicts. This will also make review easier and faster.
@134130 Thanks for your help - we're almost there!

@134130 134130 mentioned this pull request Jul 17, 2025
9 tasks
@134130 134130 closed this Jul 17, 2025
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.

2 participants