Skip to content
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

Create sampling response form #246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathanArseneau
Copy link
Contributor

This address #199

Motivation and Context

The sampling response was hard coded here, The goal was to remove the stubs.

How Has This Been Tested?

I created a small MCP server to send / receive sampling request
image
image

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

Sorry, something went wrong.

@cliffhall cliffhall added enhancement New feature or request waiting on sdk Waiting for an SDK feature labels Apr 2, 2025
@cliffhall
Copy link
Contributor

cliffhall commented Apr 2, 2025

I think a JSON Editor should be an option here and not only the form.

Two reasons:

  1. A change coming down the pike is that the content field can be an array and not just a single object. Supporting this will be complicated in the form. We'll need a mechanism for adding more content blocks, turning an object into the first element of an array and allowing editing of all of them.
  2. We already have a similar problem with the Run Tool form, and this will be another form that seemed simple but is going to need a lot of work to be more valuable than the JSON editor.

Tool run form can't create new objects (in this case states)

Screenshot 2025-04-02 at 12 07 54 PM

Tool run form fortunately offers JSON editor option

Screenshot 2025-04-02 at 12 07 38 PM

Tool run form doesn't know how to handle objects

Screenshot 2025-04-02 at 12 07 30 PM

@cliffhall cliffhall added waiting on submitter Waiting for the submitter to provide more info and removed waiting on sdk Waiting for an SDK feature labels Apr 3, 2025
@nathanArseneau nathanArseneau force-pushed the sampling-form branch 2 times, most recently from d933078 to 7cdaad0 Compare April 5, 2025 15:26
@nathanArseneau
Copy link
Contributor Author

image image

@cliffhall
Copy link
Contributor

Hi @nathanArseneau. In that screenshot, I like the extra content container around its elements. But I notice the mime type field is visible when the type is text. It should only be shown if type is not text.

@cliffhall
Copy link
Contributor

cliffhall commented Apr 5, 2025

Hey @nathanArseneau. Testing this locally, the Approve and Reject buttons seem to work as before, but I'm seeing something weird when trying to switch from JSON to the Form.

The crash comes when a hook in the App.tsx gets run again, trying to fetch config. No idea what has caused that hook to fail or even be run again, since it's a no deps hook that should only run once when the App component is mounted.

The switch to form button is inside a component you're just using, but something must be different about the component where it is embedded. Can you have a look at how it is used in the ToolTab? It is not crashing there.

This may not be about your implementation, but it's kind of a show stopper for this PR. If this just flushed out another bug, we can fix it elsewhere and you can merge it into this. Or if you pin it down and it's easy enough it could just go in as an incidental fix on this PR.

Switch to Form crashes in Sampling tab

form-crash.mov

Switch to Form doesn't crash in Tools tab

form-not-crash.mov

@nathanArseneau
Copy link
Contributor Author

nathanArseneau commented Apr 5, 2025

Sure, I will be happy to investigate, and yes, it is better we solve this issue before this PR

However, when I am testing from my end, I cannot reproduce.

The closest I came to that issue was when my server went down, but that is expected behavior, and there was no error in the console.

@nathanArseneau
Copy link
Contributor Author

My apologies; that was my mistake. The sampling is wrapped in a form for semantics, and the buttons were not typed.

I did not get your error with the config, but I managed to reproduce a similar behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting on submitter Waiting for the submitter to provide more info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants