fix(ui): resolve review feedback on the nullable-schema-type fix

Three issues from review and CI on this branch:

Greptile (P1): a nullable numeric/boolean field with no explicit default was
getting a synthetic 0/false from buildDefaultValue. Unlike a string's natural
"" default, that value looks user-provided, passes the non-empty submission
filter, and gets sent to the tool even when the field was never touched.
Nullable numeric/boolean fields with no default now resolve to undefined
instead, so an untouched field is correctly omitted from the payload.

Greptile (P2): the JSON-Schema-nullable-type explanation comment was
duplicated across both components, the shared types module, and both test
files. Extract the shared logic those comments were explaining (resolveSchemaType,
buildDefaultValue, getInitialValueForField) into one module,
mcpToolSchemaDefaults.ts, with the explanation living in exactly one place;
ToolTestPanel and MCPToolArgumentsForm both import from it instead of each
carrying their own copy.

CI (ESLint max-lines): ToolTestPanel.tsx had grown past the 800-line limit.
Extracting the ~100 lines of duplicated parsing logic into the shared module
brings it back to 781.

Added mcpToolSchemaDefaults.test.ts with direct unit tests for the extracted
logic (matching this repo's own stated pattern of unit-testing extracted
logic rather than only driving it through a render), including the
nullable-default fix. Updated the existing render-based tests: the
"renders inputs for nullable properties" test now asserts the numeric field
starts empty instead of 0, and a new test in each component's test file
asserts an untouched nullable field is omitted from the submitted values
entirely rather than sent as a synthetic 0.

Rebase note: this repo has since migrated ToolTestPanel/ToolArgumentsForm
off antd onto react-hook-form + shadcn, and off the local resolveSchemaType
duplicated in ToolTestPanel.tsx onto a shared resolveSchemaProperty in
toolCallArguments.ts (which additionally resolves anyOf/oneOf, not just
type arrays). ToolTestPanel.tsx therefore does not import
mcpToolSchemaDefaults.ts - the nullable-default-omission fix is ported
into toolCallArguments.ts's own buildDefaultValue instead, preserving this
commit's actual intent (consistent omit-if-untouched behavior across both
forms) against the current file layout. MCPToolArgumentsForm.tsx, which
was not touched by that refactor, imports mcpToolSchemaDefaults.ts as
originally written here.
This commit is contained in:
Aliaksei Venski 2026-08-25 11:31:16 +02:00
parent 6cc81584b9
commit 5b46865e7b
6 changed files with 199 additions and 94 deletions

View file

@ -160,9 +160,8 @@ describe("ToolTestPanel defaults", () => {
});
it("renders inputs for nullable (type-array) schema properties instead of leaving them blank", () => {
// JSON Schema represents an optional/nullable field as an array type, e.g. ["string", "null"] -
// the shape Pydantic's model_json_schema() emits for Optional[str]. A real Azure DevOps MCP tool
// schema (testplan_test_suite_write) declares its optional fields exactly this way.
// A real Azure DevOps MCP tool schema (testplan_test_suite_write) declares its optional
// fields exactly this way.
const schema: InputSchema = {
type: "object",
properties: {
@ -175,10 +174,35 @@ describe("ToolTestPanel defaults", () => {
renderPanel(schema);
expect(screen.getByLabelText("name")).toHaveValue("");
expect(screen.getByLabelText("parentSuiteId")).toHaveValue(0);
expect(screen.getByLabelText("parentSuiteId")).toHaveValue(null);
expect(screen.getByLabelText("active")).toBeInTheDocument();
});
it("omits an untouched nullable numeric field from the submitted payload instead of sending a synthetic 0", async () => {
const schema: InputSchema = {
type: "object",
properties: {
parentSuiteId: { type: ["integer", "null"], description: "Optional parent suite id" },
},
};
const onSubmit = vi.fn();
render(
<ToolTestPanel
tool={buildTool(schema)}
onSubmit={onSubmit}
isLoading={false}
result={null}
error={null}
onClose={vi.fn()}
/>,
);
await userEvent.click(screen.getByRole("button", { name: "Call Tool" }));
expect(onSubmit).toHaveBeenCalledWith({});
});
it("coerces a nullable integer field to a number on submit, not a string", async () => {
const schema: InputSchema = {
type: "object",

View file

@ -180,11 +180,10 @@ describe("MCPToolArgumentsForm", () => {
});
describe("MCPToolArgumentsForm nullable schema types", () => {
it("renders inputs for nullable (type-array) schema properties instead of a plain text fallback", () => {
// JSON Schema represents an optional/nullable field as an array type, e.g. ["integer", "null"] -
// the shape Pydantic's model_json_schema() emits for Optional[int]. Unlike ToolTestPanel, this
// component has a final "else" branch that falls back to a plain text Input, so the bug here
// is a wrong widget (text instead of number), not a missing one.
it("renders a number widget for nullable (type-array) schema properties instead of a plain text fallback", () => {
// Unlike ToolTestPanel, this component has a final "else" branch that falls back to a plain
// text Input, so the pre-fix bug here was a wrong widget (text instead of number), not a
// missing one; the field must also start empty, not a synthetic 0.
renderForm({
type: "object",
properties: {
@ -193,8 +192,21 @@ describe("MCPToolArgumentsForm nullable schema types", () => {
required: [],
});
// Native type="number" input: jest-dom reports an empty one as null, not "".
const input = screen.getByLabelText("parentSuiteId");
expect(input).toHaveValue(0);
expect(input).toHaveValue(null);
});
it("omits an untouched nullable numeric field from getSubmitValues instead of sending a synthetic 0", async () => {
const ref = renderForm({
type: "object",
properties: {
parentSuiteId: { type: ["integer", "null"], description: "Optional parent suite id" },
},
required: [],
});
await expect(submit(ref)).resolves.toEqual({});
});
it("coerces a nullable integer field to a number in getSubmitValues, not a string", async () => {

View file

@ -8,6 +8,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@
import { Textarea } from "@/components/ui/textarea";
import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/components/ui/tooltip";
import { MCPTool, InputSchema, InputSchemaProperty } from "./types";
import { resolveSchemaType, getInitialValueForField } from "./mcpToolSchemaDefaults";
type ToolFormValues = Record<string, unknown>;
@ -74,87 +75,6 @@ const labelFor = (key: string, prop: InputSchemaProperty, required: boolean): Re
</span>
);
const isPlainObject = (value: unknown): value is Record<string, any> =>
typeof value === "object" && value !== null && !Array.isArray(value);
// JSON Schema allows "type" to be an array (e.g. ["string", "null"]) for a nullable field, the
// shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Every branch below
// that decides a widget or a value conversion from a property's type needs the single effective
// (non-null) type, not the raw field verbatim.
function resolveSchemaType(type: InputSchemaProperty["type"] | undefined): string | undefined {
if (Array.isArray(type)) {
return type.find((t) => t !== "null") ?? type[0];
}
return type;
}
function buildArrayItems(items?: InputSchemaProperty | InputSchemaProperty[]): any[] {
if (!items) return [];
if (Array.isArray(items)) {
return items.map((item) => buildDefaultValue(item)).filter((value) => value !== undefined);
}
const itemDefault = buildDefaultValue(items);
return itemDefault !== undefined ? [itemDefault] : [];
}
function buildDefaultValue(prop?: InputSchemaProperty, overrideDefault?: any): any {
if (!prop) return undefined;
const effectiveDefault = overrideDefault !== undefined ? overrideDefault : prop.default;
const effectiveType = resolveSchemaType(prop.type);
if (effectiveType === "object") {
const base = isPlainObject(effectiveDefault) ? { ...effectiveDefault } : {};
if (prop.properties) {
Object.entries(prop.properties).forEach(([childKey, childProp]) => {
base[childKey] = buildDefaultValue(childProp, base[childKey]);
});
}
return base;
}
if (effectiveType === "array") {
if (Array.isArray(effectiveDefault)) {
const itemSchema = prop.items;
if (!itemSchema) return effectiveDefault;
if (effectiveDefault.length === 0) {
const sample = buildArrayItems(itemSchema);
return sample.length ? sample : effectiveDefault;
}
if (Array.isArray(itemSchema)) {
return effectiveDefault.map((value, index) => {
const schema = itemSchema[index] ?? itemSchema[itemSchema.length - 1];
return buildDefaultValue(schema, value);
});
}
return effectiveDefault.map((value) => buildDefaultValue(itemSchema, value));
}
if (effectiveDefault !== undefined) return effectiveDefault;
return buildArrayItems(prop.items);
}
if (effectiveDefault !== undefined) return effectiveDefault;
switch (effectiveType) {
case "integer":
case "number":
return 0;
case "boolean":
return false;
case "string":
default:
return "";
}
}
const getInitialValueForField = (prop: InputSchemaProperty): any => {
const defaultValue = buildDefaultValue(prop);
const effectiveType = resolveSchemaType(prop.type);
if (effectiveType === "object" || effectiveType === "array") {
const fallback = effectiveType === "array" ? [] : {};
return JSON.stringify(defaultValue ?? fallback, null, 2);
}
return defaultValue;
};
function convertFormValues(
values: Record<string, any>,
actualSchema: InputSchema,
@ -391,7 +311,7 @@ const MCPToolArgumentsForm = forwardRef<MCPToolArgumentsFormRef, MCPToolArgument
{...field}
type="number"
step={effectiveType === "integer" ? 1 : undefined}
value={field.value as number | string}
value={(field.value as number | string) ?? ""}
placeholder={prop.description || `Enter ${key}`}
/>
);

View file

@ -0,0 +1,60 @@
import { describe, expect, it } from "vitest";
import { buildDefaultValue, getInitialValueForField, resolveSchemaType } from "./mcpToolSchemaDefaults";
describe("resolveSchemaType", () => {
it("returns a scalar type unchanged", () => {
expect(resolveSchemaType("string")).toBe("string");
});
it("returns the first non-null entry of a nullable type array", () => {
expect(resolveSchemaType(["integer", "null"])).toBe("integer");
expect(resolveSchemaType(["null", "boolean"])).toBe("boolean");
});
it("falls back to the first entry when every entry is null", () => {
expect(resolveSchemaType(["null"])).toBe("null");
});
});
describe("buildDefaultValue", () => {
it("defaults a required (non-nullable) integer with no explicit default to 0", () => {
expect(buildDefaultValue({ type: "integer" })).toBe(0);
});
it("defaults a required (non-nullable) boolean with no explicit default to false", () => {
expect(buildDefaultValue({ type: "boolean" })).toBe(false);
});
it("defaults a required (non-nullable) string with no explicit default to an empty string", () => {
expect(buildDefaultValue({ type: "string" })).toBe("");
});
it("leaves a nullable integer with no explicit default undefined, not a synthetic 0", () => {
expect(buildDefaultValue({ type: ["integer", "null"] })).toBeUndefined();
});
it("leaves a nullable boolean with no explicit default undefined, not a synthetic false", () => {
expect(buildDefaultValue({ type: ["boolean", "null"] })).toBeUndefined();
});
it("defaults a nullable string with no explicit default to an empty string, unaffected by nullability", () => {
expect(buildDefaultValue({ type: ["string", "null"] })).toBe("");
});
it("still honors an explicit default on a nullable numeric field", () => {
expect(buildDefaultValue({ type: ["integer", "null"], default: 7 })).toBe(7);
});
});
describe("getInitialValueForField", () => {
it("passes through undefined for a nullable numeric field with no default", () => {
expect(getInitialValueForField({ type: ["integer", "null"] })).toBeUndefined();
});
it("JSON-stringifies an object field's computed default", () => {
expect(getInitialValueForField({ type: "object", properties: { a: { type: "string" } } })).toBe(
JSON.stringify({ a: "" }, null, 2),
);
});
});

View file

@ -0,0 +1,88 @@
import { InputSchemaProperty } from "./types";
// JSON Schema allows "type" to be an array (e.g. ["string", "null"]) for a nullable field, the
// shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Every branch that
// decides a widget or a value conversion from a property's type needs the single effective
// (non-null) type, not the raw field verbatim.
export function resolveSchemaType(type: InputSchemaProperty["type"] | undefined): string | undefined {
if (Array.isArray(type)) {
return type.find((t) => t !== "null") ?? type[0];
}
return type;
}
const isPlainObject = (value: unknown): value is Record<string, any> =>
typeof value === "object" && value !== null && !Array.isArray(value);
function buildArrayItems(items?: InputSchemaProperty | InputSchemaProperty[]): any[] {
if (!items) return [];
if (Array.isArray(items)) {
return items.map((item) => buildDefaultValue(item)).filter((value) => value !== undefined);
}
const itemDefault = buildDefaultValue(items);
return itemDefault !== undefined ? [itemDefault] : [];
}
export function buildDefaultValue(prop?: InputSchemaProperty, overrideDefault?: any): any {
if (!prop) return undefined;
const effectiveDefault = overrideDefault !== undefined ? overrideDefault : prop.default;
const effectiveType = resolveSchemaType(prop.type);
if (effectiveType === "object") {
const base = isPlainObject(effectiveDefault) ? { ...effectiveDefault } : {};
if (prop.properties) {
Object.entries(prop.properties).forEach(([childKey, childProp]) => {
base[childKey] = buildDefaultValue(childProp, base[childKey]);
});
}
return base;
}
if (effectiveType === "array") {
if (Array.isArray(effectiveDefault)) {
const itemSchema = prop.items;
if (!itemSchema) return effectiveDefault;
if (effectiveDefault.length === 0) {
const sample = buildArrayItems(itemSchema);
return sample.length ? sample : effectiveDefault;
}
if (Array.isArray(itemSchema)) {
return effectiveDefault.map((value, index) => {
const schema = itemSchema[index] ?? itemSchema[itemSchema.length - 1];
return buildDefaultValue(schema, value);
});
}
return effectiveDefault.map((value) => buildDefaultValue(itemSchema, value));
}
if (effectiveDefault !== undefined) return effectiveDefault;
return buildArrayItems(prop.items);
}
if (effectiveDefault !== undefined) return effectiveDefault;
// A nullable numeric/boolean field (an array "type") with no explicit default starts empty
// rather than a synthetic 0/false: unlike a string's natural "" default, that value looks
// user-provided, passes the non-empty submission filter, and gets sent to the tool even when
// the field was never touched.
const isNullable = Array.isArray(prop.type);
switch (effectiveType) {
case "integer":
case "number":
return isNullable ? undefined : 0;
case "boolean":
return isNullable ? undefined : false;
case "string":
default:
return "";
}
}
export const getInitialValueForField = (prop: InputSchemaProperty): any => {
const defaultValue = buildDefaultValue(prop);
const effectiveType = resolveSchemaType(prop.type);
if (effectiveType === "object" || effectiveType === "array") {
const fallback = effectiveType === "array" ? [] : {};
return JSON.stringify(defaultValue ?? fallback, null, 2);
}
return defaultValue;
};

View file

@ -296,8 +296,9 @@ export const handleAuth = (authType?: string | null): string => {
export interface InputSchemaProperty {
// JSON Schema allows an array here (e.g. ["string", "null"]) for a nullable/optional field —
// the shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Callers must
// resolve to a single type with resolveSchemaType before branching on it. Optional because a
// property may instead be described purely via anyOf/oneOf, with no top-level type at all.
// resolve to a single type with resolveSchemaType (mcpToolSchemaDefaults.ts / toolCallArguments.ts)
// before branching on it. Optional because a property may instead be described purely via
// anyOf/oneOf, with no top-level type at all.
type?: string | string[];
description?: string;
properties?: Record<string, InputSchemaProperty>; // For nested object properties