-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: allow empty elicitation form data when all fields are optional #926
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
fix: allow empty elicitation form data when all fields are optional #926
Conversation
|
I don't understand what happened: why did an action in an unrelated repo close this PR? |
e1e8bf9 to
4b20f15
Compare
|
@olaservo added proposed tests |
Using appropriate message/title fields, I think this looks better now: <img width="3370" height="3208" alt="image" src="https://github.com/user-attachments/assets/e9bbf906-4ba8-4563-affc-62cdc6c97342" /> Though note that in the current version of the Inspector (`0.17.2`), you cannot hit **Submit** until you fill out the field. I believe this is a bug in the Inspector, as it does not properly handle the case when all fields are optional. I put up a fix: modelcontextprotocol/inspector#926
|
@cliffhall would you be willing to take a look? |
olaservo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I tested with this tool triggering an elicitation with all optional inputs, and also tested that elicitations in the latest everything server built from main still worked after this change.
|
Thanks so much @olaservo! |
Summary
When all of the fields of the schema for an elicitation are optional, it should be possible to hit the Submit button (sending
"accept") without filling in any of the fields. Prior to this change, this would fail in the UI with:Previously, in this case,
generateDefaultValue()would produceundefinedas the default value, but as of this change, it produces{}instead. Theundefinedwould fail the check here:inspector/client/src/components/ElicitationRequest.tsx
Lines 82 to 90 in dff91a8
Now it passes.
Type of Change
Changes Made
Related Issues
Testing
Test Results and/or Instructions
Screenshots are encouraged to share your testing results for this change.
Checklist
npm run prettier-fix)Breaking Changes
Additional Context