Merge pull request #625 from olaservo/omit-optional-empty-strings
Update ToolsTab parameter handling to omit optional empty fields
This commit is contained in:
Cliff Hall
GitHub
commit
cd2b787066
6 changed files with 327 additions and 30 deletions
Binary file not shown.
|
|
@ -22,6 +22,8 @@ import { SESSION_KEYS, getServerSpecificKey } from "./lib/constants";
|
|||
import { AuthDebuggerState, EMPTY_DEBUGGER_STATE } from "./lib/auth-types";
|
||||
import { OAuthStateMachine } from "./lib/oauth-state-machine";
|
||||
import { cacheToolOutputSchemas } from "./utils/schemaUtils";
|
||||
import { cleanParams } from "./utils/paramUtils";
|
||||
import type { JsonSchemaType } from "./utils/jsonUtils";
|
||||
import React, {
|
||||
Suspense,
|
||||
useCallback,
|
||||
|
|
@ -777,12 +779,18 @@ const App = () => {
|
|||
lastToolCallOriginTabRef.current = currentTabRef.current;
|
||||
|
||||
try {
|
||||
// Find the tool schema to clean parameters properly
|
||||
const tool = tools.find((t) => t.name === name);
|
||||
const cleanedParams = tool?.inputSchema
|
||||
? cleanParams(params, tool.inputSchema as JsonSchemaType)
|
||||
: params;
|
||||
|
||||
const response = await sendMCPRequest(
|
||||
{
|
||||
method: "tools/call" as const,
|
||||
params: {
|
||||
name,
|
||||
arguments: params,
|
||||
arguments: cleanedParams,
|
||||
_meta: {
|
||||
progressToken: progressTokenRef.current++,
|
||||
},
|
||||
|
|
@ -793,6 +801,8 @@ const App = () => {
|
|||
);
|
||||
|
||||
setToolResult(response);
|
||||
// Clear any validation errors since tool execution completed
|
||||
setErrors((prev) => ({ ...prev, tools: null }));
|
||||
} catch (e) {
|
||||
const toolResult: CompatibilityCallToolResult = {
|
||||
content: [
|
||||
|
|
@ -804,6 +814,8 @@ const App = () => {
|
|||
isError: true,
|
||||
};
|
||||
setToolResult(toolResult);
|
||||
// Clear validation errors - tool execution errors are shown in ToolResults
|
||||
setErrors((prev) => ({ ...prev, tools: null }));
|
||||
}
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -131,14 +131,15 @@ const ToolsTab = ({
|
|||
</h3>
|
||||
</div>
|
||||
<div className="p-4">
|
||||
{error ? (
|
||||
<Alert variant="destructive">
|
||||
<AlertCircle className="h-4 w-4" />
|
||||
<AlertTitle>Error</AlertTitle>
|
||||
<AlertDescription>{error}</AlertDescription>
|
||||
</Alert>
|
||||
) : selectedTool ? (
|
||||
{selectedTool ? (
|
||||
<div className="space-y-4">
|
||||
{error && (
|
||||
<Alert variant="destructive">
|
||||
<AlertCircle className="h-4 w-4" />
|
||||
<AlertTitle>Error</AlertTitle>
|
||||
<AlertDescription>{error}</AlertDescription>
|
||||
</Alert>
|
||||
)}
|
||||
<p className="text-sm text-gray-600 dark:text-gray-400">
|
||||
{selectedTool.description}
|
||||
</p>
|
||||
|
|
@ -184,16 +185,27 @@ const ToolsTab = ({
|
|||
id={key}
|
||||
name={key}
|
||||
placeholder={prop.description}
|
||||
value={(params[key] as string) ?? ""}
|
||||
onChange={(e) =>
|
||||
setParams({
|
||||
...params,
|
||||
[key]:
|
||||
e.target.value === ""
|
||||
? undefined
|
||||
: e.target.value,
|
||||
})
|
||||
value={
|
||||
params[key] === undefined
|
||||
? ""
|
||||
: String(params[key])
|
||||
}
|
||||
onChange={(e) => {
|
||||
const value = e.target.value;
|
||||
if (value === "") {
|
||||
// Field cleared - set to undefined
|
||||
setParams({
|
||||
...params,
|
||||
[key]: undefined,
|
||||
});
|
||||
} else {
|
||||
// Field has value - keep as string
|
||||
setParams({
|
||||
...params,
|
||||
[key]: value,
|
||||
});
|
||||
}
|
||||
}}
|
||||
className="mt-1"
|
||||
/>
|
||||
) : prop.type === "object" || prop.type === "array" ? (
|
||||
|
|
@ -227,13 +239,35 @@ const ToolsTab = ({
|
|||
id={key}
|
||||
name={key}
|
||||
placeholder={prop.description}
|
||||
value={(params[key] as string) ?? ""}
|
||||
value={
|
||||
params[key] === undefined
|
||||
? ""
|
||||
: String(params[key])
|
||||
}
|
||||
onChange={(e) => {
|
||||
const value = e.target.value;
|
||||
setParams({
|
||||
...params,
|
||||
[key]: value === "" ? "" : Number(value),
|
||||
});
|
||||
if (value === "") {
|
||||
// Field cleared - set to undefined
|
||||
setParams({
|
||||
...params,
|
||||
[key]: undefined,
|
||||
});
|
||||
} else {
|
||||
// Field has value - try to convert to number, but store input either way
|
||||
const num = Number(value);
|
||||
if (!isNaN(num)) {
|
||||
setParams({
|
||||
...params,
|
||||
[key]: num,
|
||||
});
|
||||
} else {
|
||||
// Store invalid input as string - let server validate
|
||||
setParams({
|
||||
...params,
|
||||
[key]: value,
|
||||
});
|
||||
}
|
||||
}
|
||||
}}
|
||||
className="mt-1"
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -644,6 +644,7 @@ describe("ToolsTab", () => {
|
|||
description: "Tool with JSON parameters",
|
||||
inputSchema: {
|
||||
type: "object" as const,
|
||||
required: ["config", "data"], // Make them required so they render as form fields
|
||||
properties: {
|
||||
config: {
|
||||
type: "object" as const,
|
||||
|
|
@ -693,15 +694,16 @@ describe("ToolsTab", () => {
|
|||
callTool: mockCallTool,
|
||||
});
|
||||
|
||||
// Find JSON editor textareas
|
||||
// Find JSON editor textareas (should have one for each required field: config and data)
|
||||
const textareas = screen.getAllByRole("textbox");
|
||||
expect(textareas.length).toBe(2);
|
||||
|
||||
// Enter valid JSON in the first textarea
|
||||
// Enter valid JSON in each textarea
|
||||
fireEvent.change(textareas[0], {
|
||||
target: {
|
||||
value:
|
||||
'{ "config": { "setting": "value" }, "data": ["item1", "item2"] }',
|
||||
},
|
||||
target: { value: '{ "setting": "value" }' },
|
||||
});
|
||||
fireEvent.change(textareas[1], {
|
||||
target: { value: '["item1", "item2"]' },
|
||||
});
|
||||
|
||||
// Wait for debounced updates
|
||||
|
|
@ -799,9 +801,16 @@ describe("ToolsTab", () => {
|
|||
});
|
||||
|
||||
const textareas = screen.getAllByRole("textbox");
|
||||
expect(textareas.length).toBe(2);
|
||||
|
||||
// Clear the textarea (empty JSON should be valid)
|
||||
fireEvent.change(textareas[0], { target: { value: "" } });
|
||||
// Clear both textareas (empty JSON should be valid)
|
||||
fireEvent.change(textareas[0], { target: { value: "{}" } });
|
||||
fireEvent.change(textareas[1], { target: { value: "[]" } });
|
||||
|
||||
// Wait for debounced updates
|
||||
await act(async () => {
|
||||
await new Promise((resolve) => setTimeout(resolve, 350));
|
||||
});
|
||||
|
||||
// Try to run the tool
|
||||
const runButton = screen.getByRole("button", { name: /run tool/i });
|
||||
|
|
|
|||
208
client/src/utils/__tests__/paramUtils.test.ts
Normal file
208
client/src/utils/__tests__/paramUtils.test.ts
Normal file
|
|
@ -0,0 +1,208 @@
|
|||
import { cleanParams } from "../paramUtils";
|
||||
import type { JsonSchemaType } from "../jsonUtils";
|
||||
|
||||
describe("cleanParams", () => {
|
||||
it("should preserve required fields even when undefined", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: ["requiredString", "requiredNumber"],
|
||||
properties: {
|
||||
requiredString: { type: "string" },
|
||||
requiredNumber: { type: "number" },
|
||||
optionalString: { type: "string" },
|
||||
optionalNumber: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
requiredString: undefined,
|
||||
requiredNumber: undefined,
|
||||
optionalString: undefined,
|
||||
optionalNumber: undefined,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({
|
||||
requiredString: undefined,
|
||||
requiredNumber: undefined,
|
||||
// optionalString and optionalNumber should be omitted
|
||||
});
|
||||
});
|
||||
|
||||
it("should omit optional fields with empty strings", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: [],
|
||||
properties: {
|
||||
optionalString: { type: "string" },
|
||||
optionalNumber: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
optionalString: "",
|
||||
optionalNumber: "",
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({});
|
||||
});
|
||||
|
||||
it("should omit optional fields with undefined values", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: [],
|
||||
properties: {
|
||||
optionalString: { type: "string" },
|
||||
optionalNumber: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
optionalString: undefined,
|
||||
optionalNumber: undefined,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({});
|
||||
});
|
||||
|
||||
it("should omit optional fields with null values", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: [],
|
||||
properties: {
|
||||
optionalString: { type: "string" },
|
||||
optionalNumber: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
optionalString: null,
|
||||
optionalNumber: null,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({});
|
||||
});
|
||||
|
||||
it("should preserve optional fields with meaningful values", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: [],
|
||||
properties: {
|
||||
optionalString: { type: "string" },
|
||||
optionalNumber: { type: "number" },
|
||||
optionalBoolean: { type: "boolean" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
optionalString: "hello",
|
||||
optionalNumber: 42,
|
||||
optionalBoolean: false, // false is a meaningful value
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({
|
||||
optionalString: "hello",
|
||||
optionalNumber: 42,
|
||||
optionalBoolean: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("should handle mixed required and optional fields", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: ["requiredField"],
|
||||
properties: {
|
||||
requiredField: { type: "string" },
|
||||
optionalWithValue: { type: "string" },
|
||||
optionalEmpty: { type: "string" },
|
||||
optionalUndefined: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
requiredField: "",
|
||||
optionalWithValue: "test",
|
||||
optionalEmpty: "",
|
||||
optionalUndefined: undefined,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({
|
||||
requiredField: "",
|
||||
optionalWithValue: "test",
|
||||
});
|
||||
});
|
||||
|
||||
it("should handle schema without required array", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
properties: {
|
||||
field1: { type: "string" },
|
||||
field2: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
field1: "",
|
||||
field2: undefined,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({});
|
||||
});
|
||||
|
||||
it("should preserve zero values for numbers", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: [],
|
||||
properties: {
|
||||
optionalNumber: { type: "number" },
|
||||
},
|
||||
};
|
||||
|
||||
const params = {
|
||||
optionalNumber: 0,
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({
|
||||
optionalNumber: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("should handle the new undefined-first approach (no empty strings)", () => {
|
||||
const schema: JsonSchemaType = {
|
||||
type: "object",
|
||||
required: ["requiredField"],
|
||||
properties: {
|
||||
requiredField: { type: "string" },
|
||||
optionalField: { type: "string" },
|
||||
},
|
||||
};
|
||||
|
||||
// New behavior: cleared fields are undefined, never empty strings
|
||||
const params = {
|
||||
requiredField: undefined, // cleared required field
|
||||
optionalField: undefined, // cleared optional field
|
||||
};
|
||||
|
||||
const cleaned = cleanParams(params, schema);
|
||||
|
||||
expect(cleaned).toEqual({
|
||||
requiredField: undefined, // required field preserved as undefined
|
||||
// optionalField omitted entirely
|
||||
});
|
||||
});
|
||||
});
|
||||
34
client/src/utils/paramUtils.ts
Normal file
34
client/src/utils/paramUtils.ts
Normal file
|
|
@ -0,0 +1,34 @@
|
|||
import type { JsonSchemaType } from "./jsonUtils";
|
||||
|
||||
/**
|
||||
* Cleans parameters by removing undefined, null, and empty string values for optional fields
|
||||
* while preserving all values for required fields.
|
||||
*
|
||||
* @param params - The parameters object to clean
|
||||
* @param schema - The JSON schema defining which fields are required
|
||||
* @returns Cleaned parameters object with optional empty fields omitted
|
||||
*/
|
||||
export function cleanParams(
|
||||
params: Record<string, unknown>,
|
||||
schema: JsonSchemaType,
|
||||
): Record<string, unknown> {
|
||||
const cleaned: Record<string, unknown> = {};
|
||||
const required = schema.required || [];
|
||||
|
||||
for (const [key, value] of Object.entries(params)) {
|
||||
const isFieldRequired = required.includes(key);
|
||||
|
||||
if (isFieldRequired) {
|
||||
// Required fields: always include, even if empty string or falsy
|
||||
cleaned[key] = value;
|
||||
} else {
|
||||
// Optional fields: only include if they have meaningful values
|
||||
if (value !== undefined && value !== "" && value !== null) {
|
||||
cleaned[key] = value;
|
||||
}
|
||||
// Empty strings, undefined, null for optional fields → omit completely
|
||||
}
|
||||
}
|
||||
|
||||
return cleaned;
|
||||
}
|
||||
Loading…
Reference in a new issue