Improve on tool input handling and add tests #159

olaservo · 2025-02-27T16:20:24Z

Motivation and Context

The ToolTab JSON input handling still has some quirks and testing it manually isn't very sustainable. This PR adds the Jest testing framework to the client side of the MCP Inspector, enabling unit testing for the more complex client-side utilities and components.

The intent here is to improve maintainability by:

Extracting utility functions from components into dedicated utility files
- jsonPathUtils.ts: Handles JSON path operations (updating and retrieving values)
- schemaUtils.ts: Handles schema-related operations (generating default values, formatting labels)
Adding unit tests for these utilities
Improving the DynamicJsonForm component with better error handling and state management, for better handling of edge cases (empty objects, missing schema properties).
Enhancing the JsonEditor component with better state management and formatting capabilities
Address this issue: Cannot pass an integer correctly #154

Another option to consider is using a JSON editing library which handles more of this stuff out of the box. I prefer to avoid adding libraries which might be overkill and introduce their own overhead. But it's still an option, if trying to cover all the more complex tool input testing scenarios doesn't seem sustainable in the long term.

How Has This Been Tested?

The PR includes new Jest tests for the utility functions that were extracted from components:

Tests for jsonPathUtils (updateValueAtPath, getValueAtPath)
Tests for schemaUtils (generateDefaultValue, formatFieldLabel, validateValueAgainstSchema)

I also updated the GitHub workflow to run these tests automatically during CI.

After these changes I also confirmed that I could update JSON fields for object tool inputs, which appears to be broken right now.

Breaking Changes

None.

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

…o/inspector into handle-empty-json-fields

olaservo · 2025-03-11T03:12:06Z

Hi @jspahrsummers @jerome3o-anthropic @ashwin-ant hoping to get a review on this one - sorry it got big, a few related issues + tests are bundled together here. Thanks!

jspahrsummers

Just a few questions, but nothing blocking—feel free to merge at will!

jspahrsummers · 2025-03-13T13:29:53Z

.github/workflows/main.yml

+        run: |
+          cd client
+          npm test


TIOLI:

Suggested change

run: |

cd client

npm test

working-directory: ./client

run: npm test

jspahrsummers · 2025-03-13T13:30:58Z

client/src/utils/__mocks__/DynamicJsonForm.ts

@@ -0,0 +1,18 @@
+// Mock for DynamicJsonForm that only exports the types needed for tests


What happens when importing the actual component in tests? Just wondering if we need this file or if it's a structural choice.

It wasn't actually needed anymore, I had temporarily created it while sorting out some test details but tests should run fine without it.

jspahrsummers · 2025-03-13T13:35:05Z

client/src/components/ToolsTab.tsx

-                            value={(params[key] as JsonValue) ?? {}}
+                            value={
+                              (params[key] as JsonValue) ??
+                              (prop.type === "array" ? [] : {})


Should this use generateDefaultValue?

I went back and forth on this and ended up using generateDefaultValue here, with the intention of using the tool schema defaults rather than forcing any specific defaults here.

jspahrsummers · 2025-03-13T13:35:45Z

client/src/components/DynamicJsonForm.tsx

+  // Create a ref to store the timeout ID
+  const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
+
+  // Create a debounced function to update parent state


Just curious, why?

I added a better clarifying comment here. I added this debounce logic to better handle validation while typing values in JSON editor fields, since validation was getting triggered either immediately or had to be triggered on some other action like switching modes. (Edited)

jspahrsummers · 2025-03-13T13:37:28Z

client/src/components/DynamicJsonForm.tsx

+            onChange={(e) => {
+              const val = e.target.value;
+              if (!val && !propSchema.required) {
+                handleFieldChange(path, undefined);


Why don't we want to use generateDefaultValue for all of these?

For some fields I wanted to allow for clearing the field (i.e. setting to undefined) when its not required by the schema.
Intention is to preserve the user's ability to have an empty string input rather than forcing a default value.

…ields

jspahrsummers

LGTM!

yGuy · 2025-04-23T15:08:50Z

client/src/utils/schemaUtils.ts

  }

  if (!schema.required) {
+    if (schema.type === "array") return [];


@olaservo - I think this resulted in a regression. I have a schema where an option is an array that is not required. But if you specify it, it should have at least one element.

Like this:

ids: z .array(z.number()) .min(1) .optional() .describe("An optional array of ID#s to search for"),

With the change here, I don't see how you can not specify an array in the UI. This worked before, AFAICT, but broke with this change.

What do you think?

Improve on tool input handling and add tests

olaservo added 14 commits February 26, 2025 09:50

Start adding changes to address json fields

592daca

Resolve issues where JSON fields are not being rendered in form mode

0e29e2c

Extract functions

0b105b2

Handle edge case and add tests for functions

0747479

Merge branch 'main' into handle-empty-json-fields

ede1ea0

Add client tests to workflow

d4a64fb

Test workflow in my branch

90ce628

Remove comment ans trigger workflow run

426fb87

Fix formatting

238c228

Fix path to client

8ac7ef0

Remove console.warn and extra comments to reduce code noise

720480c

Fix formatting

d1f5b3b

Revert to only run on main

abd4877

Remove some fluff

6ec82e2

olaservo changed the title ~~Handle empty json fields~~ Improve on tool input handling and add tests Feb 28, 2025

olaservo mentioned this pull request Feb 28, 2025

Handle integer input #156

Closed

9 tasks

olaservo added 2 commits February 27, 2025 21:33

Default to nulls and update tests

44982e6

Fix formatting

82bbe58

olaservo marked this pull request as ready for review February 28, 2025 05:10

olaservo added 10 commits February 28, 2025 06:26

Remove unused function plus tests

a1eb343

Fix formatting

0e50b68

Fix issue where array type defaults to object

36aa731

Fix formatting

e7f55f0

Always use JSON mode if the schema type is object and has no properties

b01e386

Fix formatting

06773bb

Merge branch 'main' into handle-empty-json-fields

1ae77e9

Use debounce instead

f9b105c

Merge branch 'handle-empty-json-fields' of https://github.com/olaserv…

dd02b69

…o/inspector into handle-empty-json-fields

Fix formatting

00836db

olaservo requested a review from jspahrsummers March 5, 2025 03:57

olaservo requested review from ashwin-ant and jerome3o-anthropic March 5, 2025 03:57

olaservo added 2 commits March 5, 2025 07:52

Merge branch 'main' into handle-empty-json-fields

4efe7d7

Remove duplicate react-dialog from merge

b9b116a

olaservo mentioned this pull request Mar 5, 2025

Enhance tool call input #130

Closed

Merge branch 'main' into handle-empty-json-fields

1ff410c

Merge branch 'main' into handle-empty-json-fields

1f214de

jspahrsummers previously approved these changes Mar 13, 2025

View reviewed changes

olaservo added 5 commits March 16, 2025 15:24

Remove unneeded DynamicJsonForm.tsx

a3740c4

Use working-directory instead of cd to client

7c4ed6a

Add comments explaining extra parsing logic

e1b015e

Use generateDefaultValue for object and array defaults

50a65d0

Fix formatting

cae7c76

olaservo dismissed jspahrsummers’s stale review via cae7c76 March 16, 2025 22:34

olaservo added 5 commits March 16, 2025 15:42

Update package lock after re-running npm install

28978ea

Merge branch 'main' into handle-empty-json-fields

50131c6

Generate empty objects and arrays for non required object and array f…

7ddba51

…ields

Fix formatting

a85d5e7

Simplify check for defaults and add another test

c463dc5

jspahrsummers approved these changes Mar 19, 2025

View reviewed changes

olaservo merged commit dfc9cf7 into modelcontextprotocol:main Mar 21, 2025
2 checks passed

olaservo mentioned this pull request Mar 21, 2025

feat: Add utility function to escape Unicode characters in tool results #193

Merged

1 task

gerrywastaken mentioned this pull request Mar 23, 2025

Cannot pass an integer correctly #154

Closed

yGuy reviewed Apr 23, 2025

View reviewed changes

IgnacioC44 referenced this pull request in MCPJam/inspector Jun 21, 2025

Merge pull request #159 from olaservo/handle-empty-json-fields

505f818

Improve on tool input handling and add tests

		@@ -0,0 +1,18 @@
		// Mock for DynamicJsonForm that only exports the types needed for tests

Improve on tool input handling and add tests #159

Improve on tool input handling and add tests #159

Uh oh!

Conversation

olaservo commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Uh oh!

olaservo commented Mar 11, 2025

Uh oh!

jspahrsummers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olaservo Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspahrsummers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olaservo commented Feb 27, 2025 •

edited

Loading

olaservo Mar 16, 2025 •

edited

Loading