Merge branch 'v2/main' into v2/feat/2018-custom-authorization-params
This commit is contained in:
commit
55dbac5e95
10 changed files with 582 additions and 20 deletions
|
|
@ -173,4 +173,30 @@ describe("AppDetailPanel", () => {
|
|||
rerender(<AppDetailPanel {...baseProps} tool={numberFieldTool("app_b")} />);
|
||||
expect(input().value).toBe("");
|
||||
});
|
||||
|
||||
// #2020: `hasMissingRequiredFields` sees a field with no value, but text a
|
||||
// field could not turn into a value looks identical to it — both report
|
||||
// `undefined` — so an optional argument was dropped from the call with no
|
||||
// gate. The form reports draft validity for this reason.
|
||||
it("disables Open App while a field holds text it cannot send", async () => {
|
||||
const user = userEvent.setup();
|
||||
const jsonFieldTool: Tool = {
|
||||
name: "chart_app",
|
||||
title: "Chart App",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { series: { type: "array", title: "Series" } },
|
||||
},
|
||||
};
|
||||
renderWithMantine(<AppDetailPanel {...baseProps} tool={jsonFieldTool} />);
|
||||
const openApp = screen.getByRole("button", { name: /open app/i });
|
||||
expect(openApp).not.toBeDisabled();
|
||||
|
||||
const jsonInput = screen.getByLabelText(/Series/);
|
||||
await user.type(jsonInput, "x");
|
||||
expect(openApp).toBeDisabled();
|
||||
|
||||
await user.clear(jsonInput);
|
||||
expect(openApp).not.toBeDisabled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { Button, Divider, ScrollArea, Stack, Text } from "@mantine/core";
|
||||
import { useState } from "react";
|
||||
import { MdPlayArrow } from "react-icons/md";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { SchemaForm } from "../SchemaForm/SchemaForm";
|
||||
|
|
@ -58,7 +59,12 @@ export function AppDetailPanel({
|
|||
/* v8 ignore next -- unreachable: Tool.inputSchema is always an object */
|
||||
const formSchema = toFormSchema(inputSchema) ?? {};
|
||||
const hasErrors = hasMissingRequiredFields(formSchema, formValues);
|
||||
const disabled = isOpening || hasErrors;
|
||||
// A field holding text it could not turn into a value (unparseable JSON, an
|
||||
// unrepresentable number) reports `undefined`, which is indistinguishable
|
||||
// from empty once it reaches `formValues` — so the form reports it directly
|
||||
// and an optional argument can no longer be dropped silently (#2020).
|
||||
const [hasInvalidDraft, setHasInvalidDraft] = useState(false);
|
||||
const disabled = isOpening || hasErrors || hasInvalidDraft;
|
||||
const hasFields = hasInputFields(tool);
|
||||
|
||||
return (
|
||||
|
|
@ -82,6 +88,7 @@ export function AppDetailPanel({
|
|||
// tool's name to drop another app's in-progress field text. See
|
||||
// SchemaFormProps.resetKey.
|
||||
resetKey={tool.name}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
/>
|
||||
|
||||
<OpenAppButton
|
||||
|
|
|
|||
|
|
@ -126,4 +126,32 @@ describe("ElicitationFormPanel", () => {
|
|||
await user.type(screen.getByLabelText("Host"), "l");
|
||||
expect(onChange).toHaveBeenCalledWith({ host: "l" });
|
||||
});
|
||||
|
||||
// #2020: text a field cannot turn into a value reports `undefined`, so it is
|
||||
// indistinguishable from an empty field in `values` — Submit stayed enabled
|
||||
// and the answer went out without the property.
|
||||
it("disables Submit while a field holds text it cannot send", async () => {
|
||||
const user = userEvent.setup();
|
||||
const numberRequest: ElicitRequestFormParams = {
|
||||
message: "Please provide the port.",
|
||||
requestedSchema: {
|
||||
type: "object" as const,
|
||||
properties: { port: { type: "number" as const, title: "Port number" } },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<ElicitationFormPanel {...baseProps} request={numberRequest} />,
|
||||
);
|
||||
const submit = screen.getByRole("button", { name: "Submit" });
|
||||
expect(submit).toBeEnabled();
|
||||
|
||||
// Past MAX_SAFE_INTEGER the field reports no value rather than a rounded
|
||||
// one, so the answer would go out without the property.
|
||||
const input = screen.getByLabelText(/Port number/);
|
||||
await user.type(input, "90071992547409910");
|
||||
expect(submit).toBeDisabled();
|
||||
|
||||
await user.clear(input);
|
||||
expect(submit).toBeEnabled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import {
|
|||
Stack,
|
||||
Text,
|
||||
} from "@mantine/core";
|
||||
import { useState } from "react";
|
||||
import type { ElicitRequestFormParams } from "@modelcontextprotocol/client";
|
||||
import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js";
|
||||
import {
|
||||
|
|
@ -88,8 +89,13 @@ export function ElicitationFormPanel({
|
|||
busy = false,
|
||||
}: ElicitationFormPanelProps) {
|
||||
const requestedSchema = request.requestedSchema as InspectorFormSchema;
|
||||
// Text a field could not turn into a value never reaches `values`, so the
|
||||
// form has to report it for Submit to see it at all (#2020).
|
||||
const [hasInvalidDraft, setHasInvalidDraft] = useState(false);
|
||||
const submitDisabled =
|
||||
busy || hasMissingRequiredFields(requestedSchema, values);
|
||||
busy ||
|
||||
hasMissingRequiredFields(requestedSchema, values) ||
|
||||
hasInvalidDraft;
|
||||
return (
|
||||
<Stack gap="md">
|
||||
<QuotedMessage>{formatQuoted(request.message)}</QuotedMessage>
|
||||
|
|
@ -101,6 +107,7 @@ export function ElicitationFormPanel({
|
|||
values={values}
|
||||
onChange={onChange}
|
||||
disabled={busy}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
/>
|
||||
</FieldScroll>
|
||||
<WarningAlert>{formatWarning(serverName)}</WarningAlert>
|
||||
|
|
|
|||
|
|
@ -161,4 +161,70 @@ describe("InlineElicitationRequest", () => {
|
|||
);
|
||||
expect(screen.getByText("Host")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// #2020: this inline form had no submit gate at all — neither the missing
|
||||
// required answer that `ElicitationFormPanel` already checked, nor text a
|
||||
// field could not turn into a value (which never reaches `values`).
|
||||
describe("submit gating (#2020)", () => {
|
||||
const requiredRequest = {
|
||||
message: "Please provide your name.",
|
||||
requestedSchema: {
|
||||
type: "object" as const,
|
||||
properties: { name: { type: "string" as const, title: "Name" } },
|
||||
required: ["name"],
|
||||
},
|
||||
} satisfies ElicitRequest["params"];
|
||||
|
||||
const numberRequest = {
|
||||
message: "Please provide the port.",
|
||||
requestedSchema: {
|
||||
type: "object" as const,
|
||||
properties: { port: { type: "number" as const, title: "Port number" } },
|
||||
},
|
||||
} satisfies ElicitRequest["params"];
|
||||
|
||||
it("disables Submit while a required answer is missing", () => {
|
||||
renderWithMantine(
|
||||
<InlineElicitationRequest
|
||||
{...baseProps}
|
||||
request={requiredRequest}
|
||||
values={{}}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled();
|
||||
});
|
||||
|
||||
it("enables Submit once the required answer is given", () => {
|
||||
renderWithMantine(
|
||||
<InlineElicitationRequest
|
||||
{...baseProps}
|
||||
request={requiredRequest}
|
||||
values={{ name: "Ada" }}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled();
|
||||
});
|
||||
|
||||
it("disables Submit while a field holds text it cannot send", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(
|
||||
<InlineElicitationRequest
|
||||
{...baseProps}
|
||||
request={numberRequest}
|
||||
values={{}}
|
||||
/>,
|
||||
);
|
||||
const submit = screen.getByRole("button", { name: "Submit" });
|
||||
expect(submit).toBeEnabled();
|
||||
|
||||
// Past MAX_SAFE_INTEGER the field reports no value rather than a rounded
|
||||
// one, so the answer would go out without the property.
|
||||
const input = screen.getByLabelText(/Port number/);
|
||||
await user.type(input, "90071992547409910");
|
||||
expect(submit).toBeDisabled();
|
||||
|
||||
await user.clear(input);
|
||||
expect(submit).toBeEnabled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -13,7 +13,11 @@ import type {
|
|||
ElicitRequestFormParams,
|
||||
} from "@modelcontextprotocol/client";
|
||||
import type { PendingRequestOrigin } from "@inspector/core/mcp/types.js";
|
||||
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
|
||||
import { useState } from "react";
|
||||
import {
|
||||
hasMissingRequiredFields,
|
||||
type InspectorFormSchema,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { SchemaForm } from "../SchemaForm/SchemaForm";
|
||||
import { MrtrOriginNote } from "../../elements/MrtrOriginNote/MrtrOriginNote";
|
||||
|
||||
|
|
@ -86,6 +90,19 @@ export function InlineElicitationRequest({
|
|||
onSubmit,
|
||||
onCancel,
|
||||
}: InlineElicitationRequestProps) {
|
||||
// Same gate as `ElicitationFormPanel`, which this is the inline form of: a
|
||||
// missing required answer, or a field holding text it could not turn into a
|
||||
// value, must not be submittable — neither is visible in `values` (#2020).
|
||||
const [hasInvalidDraft, setHasInvalidDraft] = useState(false);
|
||||
const formValues = values ?? {};
|
||||
const submitDisabled =
|
||||
isFormMode(request) &&
|
||||
(hasMissingRequiredFields(
|
||||
request.requestedSchema as InspectorFormSchema,
|
||||
formValues,
|
||||
) ||
|
||||
hasInvalidDraft);
|
||||
|
||||
return (
|
||||
<RequestContainer>
|
||||
<Stack gap="sm">
|
||||
|
|
@ -100,8 +117,9 @@ export function InlineElicitationRequest({
|
|||
{isFormMode(request) && (
|
||||
<SchemaForm
|
||||
schema={request.requestedSchema as InspectorFormSchema}
|
||||
values={values ?? {}}
|
||||
values={formValues}
|
||||
onChange={onChange}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
/>
|
||||
)}
|
||||
|
||||
|
|
@ -120,7 +138,7 @@ export function InlineElicitationRequest({
|
|||
<ActionsRow>
|
||||
<CompactButton onClick={onCancel}>Cancel</CompactButton>
|
||||
{isFormMode(request) && (
|
||||
<Button size="xs" onClick={onSubmit}>
|
||||
<Button size="xs" onClick={onSubmit} disabled={submitDisabled}>
|
||||
Submit
|
||||
</Button>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -1284,3 +1284,196 @@ describe("SchemaForm nullable unions", () => {
|
|||
expect(screen.getByLabelText(/Mixed/).tagName).toBe("TEXTAREA");
|
||||
});
|
||||
});
|
||||
|
||||
// #2020: a field holding text it cannot turn into a value reports `undefined`,
|
||||
// which is exactly what an *empty* field reports. That makes the two states
|
||||
// indistinguishable to the caller, so invalid text in an optional field was
|
||||
// submittable and simply arrived at the server absent. The form reports draft
|
||||
// validity directly so a submit gate can see what `values` cannot.
|
||||
describe("SchemaForm draft validity (#2020)", () => {
|
||||
const jsonSchema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
config: { type: "array", title: "Config" },
|
||||
},
|
||||
};
|
||||
|
||||
function ValidityHarness({
|
||||
schema,
|
||||
onValidityChange,
|
||||
resetKey,
|
||||
}: {
|
||||
schema: InspectorFormSchema;
|
||||
onValidityChange: (hasInvalidDraft: boolean) => void;
|
||||
resetKey?: string;
|
||||
}) {
|
||||
const [values, setValues] = useState<Record<string, unknown>>({});
|
||||
return (
|
||||
<SchemaForm
|
||||
schema={schema}
|
||||
values={values}
|
||||
onChange={setValues}
|
||||
resetKey={resetKey}
|
||||
onValidityChange={onValidityChange}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
it("reports an empty optional field as valid", () => {
|
||||
const onValidityChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<ValidityHarness
|
||||
schema={jsonSchema}
|
||||
onValidityChange={onValidityChange}
|
||||
/>,
|
||||
);
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
it("reports an unparseable JSON draft, then clears it once the text parses", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
renderWithMantine(
|
||||
<ValidityHarness
|
||||
schema={jsonSchema}
|
||||
onValidityChange={onValidityChange}
|
||||
/>,
|
||||
);
|
||||
|
||||
const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement;
|
||||
await user.type(jsonInput, "[[1,");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
await user.type(jsonInput, "2]");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
it("reports a number this client cannot send exactly, and says so on the field", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
divisor: { type: "number", title: "Divisor" },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<ValidityHarness schema={schema} onValidityChange={onValidityChange} />,
|
||||
);
|
||||
|
||||
// Past MAX_SAFE_INTEGER the field reports no value rather than a rounded
|
||||
// one, so — like the JSON editor — the text on screen would otherwise be
|
||||
// submitted as an absent argument.
|
||||
const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
|
||||
await user.type(input, "90071992547409910");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
expect(screen.getByText(/this field will be omitted/)).toBeInTheDocument();
|
||||
|
||||
await user.clear(input);
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
expect(
|
||||
screen.queryByText(/this field will be omitted/),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("stays invalid while any one field is invalid", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
first: { type: "array", title: "First" },
|
||||
second: { type: "array", title: "Second" },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<ValidityHarness schema={schema} onValidityChange={onValidityChange} />,
|
||||
);
|
||||
|
||||
await user.type(screen.getByLabelText(/First/), "x");
|
||||
await user.type(screen.getByLabelText(/Second/), "y");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
// One of the two recovering is not enough.
|
||||
await user.clear(screen.getByLabelText(/First/));
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
await user.clear(screen.getByLabelText(/Second/));
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
it("surfaces a nested object's invalid draft through the outer form", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
outer: {
|
||||
type: "object",
|
||||
title: "Outer",
|
||||
properties: {
|
||||
config: { type: "array", title: "Config" },
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<ValidityHarness schema={schema} onValidityChange={onValidityChange} />,
|
||||
);
|
||||
|
||||
await user.type(screen.getByLabelText(/Config/), "x");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
});
|
||||
|
||||
it("clears a stale invalid draft when the form moves to another entity", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
const { rerender } = renderWithMantine(
|
||||
<ValidityHarness
|
||||
schema={jsonSchema}
|
||||
onValidityChange={onValidityChange}
|
||||
resetKey="first_tool"
|
||||
/>,
|
||||
);
|
||||
|
||||
await user.type(screen.getByLabelText(/Config/), "x");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
// Switching entities remounts the field, discarding its draft — so text
|
||||
// typed for the previous one must not keep the new one blocked.
|
||||
rerender(
|
||||
<ValidityHarness
|
||||
schema={jsonSchema}
|
||||
onValidityChange={onValidityChange}
|
||||
resetKey="second_tool"
|
||||
/>,
|
||||
);
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
it("reports valid when the form unmounts holding an invalid draft", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onValidityChange = vi.fn();
|
||||
const { unmount } = renderWithMantine(
|
||||
<ValidityHarness
|
||||
schema={jsonSchema}
|
||||
onValidityChange={onValidityChange}
|
||||
/>,
|
||||
);
|
||||
|
||||
await user.type(screen.getByLabelText(/Config/), "x");
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(true);
|
||||
|
||||
unmount();
|
||||
expect(onValidityChange).toHaveBeenLastCalledWith(false);
|
||||
});
|
||||
|
||||
it("renders without a validity callback", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(
|
||||
<SchemaForm schema={jsonSchema} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
await user.type(screen.getByLabelText(/Config/), "x");
|
||||
expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ import {
|
|||
Text,
|
||||
TextInput,
|
||||
} from "@mantine/core";
|
||||
import { useState } from "react";
|
||||
import { useCallback, useEffect, useRef, useState } from "react";
|
||||
import { ClearButton } from "../../elements/ClearButton/ClearButton";
|
||||
import { useValueChange } from "../../../hooks/useValueChange";
|
||||
import type {
|
||||
|
|
@ -169,13 +169,47 @@ function isSameJson(a: unknown, b: unknown): boolean {
|
|||
return serializeJson(a) === serializeJson(b);
|
||||
}
|
||||
|
||||
/**
|
||||
* How a field tells the enclosing `SchemaForm` whether the text it is holding
|
||||
* can be sent. Keyed by field name rather than closed over per field so the
|
||||
* reporter is referentially stable, which is what keeps the effect below from
|
||||
* re-subscribing on every render.
|
||||
*/
|
||||
type DraftValidityReporter = (fieldName: string, isValid: boolean) => void;
|
||||
|
||||
/**
|
||||
* Publish a field's draft validity to the enclosing `SchemaForm`.
|
||||
*
|
||||
* Reported from an effect rather than from the change handler because validity
|
||||
* also moves when the *parent* replaces the value — a cleared form or a loaded
|
||||
* example re-syncs the draft and makes invalid text valid again without the
|
||||
* user touching the field. Reporting during that render instead would mean
|
||||
* updating another component mid-render, which React rejects.
|
||||
*
|
||||
* The cleanup reports the field as valid again, so a field that goes away —
|
||||
* unmounted with the panel, or remounted under a new `resetKey` when the form
|
||||
* moves to another entity — cannot leave a stale draft blocking submission.
|
||||
*/
|
||||
function useDraftValidity(
|
||||
fieldName: string,
|
||||
isValid: boolean,
|
||||
onValidityChange: DraftValidityReporter,
|
||||
): void {
|
||||
useEffect(() => {
|
||||
onValidityChange(fieldName, isValid);
|
||||
return () => onValidityChange(fieldName, true);
|
||||
}, [fieldName, isValid, onValidityChange]);
|
||||
}
|
||||
|
||||
interface SchemaJsonFieldProps {
|
||||
fieldName: string;
|
||||
label: string;
|
||||
description?: string;
|
||||
withAsterisk: boolean;
|
||||
disabled: boolean;
|
||||
value: unknown;
|
||||
onChange: (value: unknown) => void;
|
||||
onValidityChange: DraftValidityReporter;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -200,18 +234,18 @@ interface SchemaJsonFieldProps {
|
|||
* it cannot send, and `undefined` is how "no value supplied" is spelled
|
||||
* everywhere else in this form.
|
||||
*
|
||||
* That leaves invalid text and an empty field indistinguishable *to the
|
||||
* parent*, so the field says so itself via `error`. A **required** field is
|
||||
* already gated — its value is `undefined`, which `hasMissingRequiredFields`
|
||||
* treats as missing — but an **optional** one would otherwise be dropped from
|
||||
* the submission with no indication. Disabling Execute/Open/Submit on an
|
||||
* invalid optional draft needs a validity channel out of this component and
|
||||
* into all four callers, which is a wider change than this fix; tracked
|
||||
* separately.
|
||||
* That leaves invalid text and an empty field indistinguishable *to the value*,
|
||||
* so the field reports the difference two other ways: it says so on itself via
|
||||
* `error`, and it reports draft validity up through `onValidityChange` so the
|
||||
* enclosing form's caller can disable Execute/Open App/Submit (#2020). Without
|
||||
* the second channel an **optional** field's invalid text is simply dropped
|
||||
* from a submission the user was allowed to make.
|
||||
*/
|
||||
function SchemaJsonField({
|
||||
fieldName,
|
||||
value,
|
||||
onChange,
|
||||
onValidityChange,
|
||||
...inputProps
|
||||
}: SchemaJsonFieldProps) {
|
||||
const [draft, setDraft] = useState(() =>
|
||||
|
|
@ -230,11 +264,11 @@ function SchemaJsonField({
|
|||
|
||||
// Text that is present but does not parse yields no value, so the field would
|
||||
// otherwise submit as absent while the user is still looking at what they
|
||||
// typed. Saying so on the field is what keeps that from being silent; see the
|
||||
// note on `SchemaJsonField` for the submit-gating half, which is not local to
|
||||
// this component.
|
||||
// typed. Saying so on the field keeps that from being silent; reporting it
|
||||
// upward is what keeps it from being submittable.
|
||||
const hasInvalidDraft =
|
||||
draft.trim() !== "" && parseJsonDraft(draft) === undefined;
|
||||
useDraftValidity(fieldName, !hasInvalidDraft, onValidityChange);
|
||||
|
||||
return (
|
||||
<SchemaJsonInput
|
||||
|
|
@ -253,7 +287,24 @@ function SchemaJsonField({
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a number field's draft is text that cannot be sent.
|
||||
*
|
||||
* Anything non-empty that `toNumericValue` declines is invalid: a half-typed
|
||||
* `"-"`, and — the case that matters — an integer past `MAX_SAFE_INTEGER`,
|
||||
* which is deliberately dropped rather than silently rounded (see
|
||||
* `toNumericValue`). Both would otherwise submit as an absent argument while
|
||||
* the text sits in the box.
|
||||
*/
|
||||
function hasInvalidNumericDraft(draft: string | number): boolean {
|
||||
if (typeof draft === "number") {
|
||||
return false;
|
||||
}
|
||||
return draft.trim() !== "" && toNumericValue(draft) === undefined;
|
||||
}
|
||||
|
||||
interface SchemaNumberInputProps {
|
||||
fieldName: string;
|
||||
label: string;
|
||||
description?: string;
|
||||
withAsterisk: boolean;
|
||||
|
|
@ -263,6 +314,7 @@ interface SchemaNumberInputProps {
|
|||
max?: number;
|
||||
allowDecimal: boolean;
|
||||
onChange: (value: number | undefined) => void;
|
||||
onValidityChange: DraftValidityReporter;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -289,8 +341,10 @@ interface SchemaNumberInputProps {
|
|||
* prop for the case it covers.
|
||||
*/
|
||||
function SchemaNumberInput({
|
||||
fieldName,
|
||||
value,
|
||||
onChange,
|
||||
onValidityChange,
|
||||
...inputProps
|
||||
}: SchemaNumberInputProps) {
|
||||
const [draft, setDraft] = useState<string | number>(value ?? "");
|
||||
|
|
@ -301,10 +355,19 @@ function SchemaNumberInput({
|
|||
}
|
||||
});
|
||||
|
||||
// Same split as the JSON field: text this client cannot send reports no value,
|
||||
// so it has to say so on the field and report it upward, or the argument is
|
||||
// dropped from a submission the user was allowed to make (#2020).
|
||||
const invalidDraft = hasInvalidNumericDraft(draft);
|
||||
useDraftValidity(fieldName, !invalidDraft, onValidityChange);
|
||||
|
||||
return (
|
||||
<NumberInput
|
||||
{...inputProps}
|
||||
value={draft}
|
||||
error={
|
||||
invalidDraft ? "Not a number — this field will be omitted" : undefined
|
||||
}
|
||||
onChange={(next) => {
|
||||
setDraft(next);
|
||||
onChange(toNumericValue(next));
|
||||
|
|
@ -337,6 +400,21 @@ export interface SchemaFormProps {
|
|||
* substitute — callers rebuild it every render, so its identity is unstable.
|
||||
*/
|
||||
resetKey?: string;
|
||||
/**
|
||||
* Called with `true` while any field is holding text that cannot be sent —
|
||||
* unparseable JSON, or a number this client cannot represent exactly. Both
|
||||
* report their value as `undefined`, which is indistinguishable from an empty
|
||||
* field once it reaches `values`, so a caller cannot derive this from the
|
||||
* values it already has (#2020).
|
||||
*
|
||||
* Gate the submit action on it: `hasMissingRequiredFields` covers a field with
|
||||
* *no* value, and this covers a field whose visible text produced none. An
|
||||
* empty optional field is unaffected — it stays valid and submittable.
|
||||
*
|
||||
* The callback is read through a ref, so an inline closure is fine — it is
|
||||
* called when the answer changes, not on every render.
|
||||
*/
|
||||
onValidityChange?: (hasInvalidDraft: boolean) => void;
|
||||
}
|
||||
|
||||
function getDefaultValue(fieldSchema: InspectorFormSchema): unknown {
|
||||
|
|
@ -362,10 +440,59 @@ export function SchemaForm({
|
|||
onChange,
|
||||
disabled = false,
|
||||
resetKey,
|
||||
onValidityChange,
|
||||
}: SchemaFormProps) {
|
||||
const properties = schema.properties ?? {};
|
||||
const requiredFields = schema.required ?? [];
|
||||
|
||||
// The names of fields currently holding unsendable text. Held here rather
|
||||
// than in each field because only the form sees them all, and only the form
|
||||
// knows when the last one cleared.
|
||||
const [invalidFields, setInvalidFields] = useState<ReadonlySet<string>>(
|
||||
() => new Set(),
|
||||
);
|
||||
|
||||
// Stable so a field's reporting effect subscribes once, not per render. The
|
||||
// updater returns the previous set unchanged when nothing moved, which is
|
||||
// what makes a nested form's inline callback safe to call repeatedly.
|
||||
const reportFieldValidity = useCallback<DraftValidityReporter>(
|
||||
(fieldName, isValid) => {
|
||||
setInvalidFields((previous) => {
|
||||
if (previous.has(fieldName) === !isValid) {
|
||||
return previous;
|
||||
}
|
||||
const next = new Set(previous);
|
||||
if (isValid) {
|
||||
next.delete(fieldName);
|
||||
} else {
|
||||
next.add(fieldName);
|
||||
}
|
||||
return next;
|
||||
});
|
||||
},
|
||||
[],
|
||||
);
|
||||
|
||||
// Read through a ref so the callback's identity is not a dependency. It has
|
||||
// to be one or the other, and a *stable* dependency is what this needs: a
|
||||
// nested form is handed a fresh closure every render, and re-running the
|
||||
// effect for that alone would toggle the parent's state (cleanup, then
|
||||
// re-report), re-render this form, and loop forever.
|
||||
const notifyValidity = useRef(onValidityChange);
|
||||
useEffect(() => {
|
||||
notifyValidity.current = onValidityChange;
|
||||
});
|
||||
|
||||
// The cleanup mirrors the one in `useDraftValidity`: a form that is no longer
|
||||
// rendered holds no drafts, so an unmounted nested form must not leave the
|
||||
// outer one blocked. It runs in the same commit as the re-report, so the
|
||||
// transient `false` is never rendered.
|
||||
const hasInvalidDraft = invalidFields.size > 0;
|
||||
useEffect(() => {
|
||||
notifyValidity.current?.(hasInvalidDraft);
|
||||
return () => notifyValidity.current?.(false);
|
||||
}, [hasInvalidDraft]);
|
||||
|
||||
function handleFieldChange(fieldName: string, fieldValue: unknown) {
|
||||
onChange({ ...values, [fieldName]: fieldValue });
|
||||
}
|
||||
|
|
@ -452,6 +579,7 @@ export function SchemaForm({
|
|||
// The only field holding local state, so the only one that has to be
|
||||
// remounted when `resetKey` says the form moved to another entity.
|
||||
key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`}
|
||||
fieldName={fieldName}
|
||||
label={label}
|
||||
description={description}
|
||||
withAsterisk={isRequired}
|
||||
|
|
@ -463,6 +591,7 @@ export function SchemaForm({
|
|||
// accepting a value the schema forbids.
|
||||
allowDecimal={fieldSchema.type === "number"}
|
||||
onChange={(val) => handleFieldChange(fieldName, val)}
|
||||
onValidityChange={reportFieldValidity}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
|
@ -541,6 +670,11 @@ export function SchemaForm({
|
|||
disabled={disabled}
|
||||
// Sub-fields belong to the same entity, so they reset with it.
|
||||
resetKey={resetKey}
|
||||
// A nested form's invalid draft is the outer form's invalid draft,
|
||||
// so it reports through the same channel under this field's name.
|
||||
onValidityChange={(nestedInvalid) =>
|
||||
reportFieldValidity(fieldName, !nestedInvalid)
|
||||
}
|
||||
/>
|
||||
</IndentedStack>
|
||||
</Stack>
|
||||
|
|
@ -553,12 +687,14 @@ export function SchemaForm({
|
|||
// Holds local draft state, so — like the number field — it has to be
|
||||
// remounted when `resetKey` says the form moved to another entity.
|
||||
key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`}
|
||||
fieldName={fieldName}
|
||||
label={label}
|
||||
description={description}
|
||||
withAsterisk={isRequired}
|
||||
disabled={disabled}
|
||||
value={rawValue}
|
||||
onChange={(val) => handleFieldChange(fieldName, val)}
|
||||
onValidityChange={reportFieldValidity}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -218,7 +218,13 @@ describe("ToolDetailPanel", () => {
|
|||
});
|
||||
|
||||
it("renders the Execute Tool button enabled when not executing", () => {
|
||||
renderWithMantine(<ToolDetailPanel {...baseProps} tool={simpleTool} />);
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel
|
||||
{...baseProps}
|
||||
tool={simpleTool}
|
||||
formValues={{ message: "hi" }}
|
||||
/>,
|
||||
);
|
||||
const button = screen.getByRole("button", { name: "Execute Tool" });
|
||||
expect(button).toBeInTheDocument();
|
||||
expect(button).not.toBeDisabled();
|
||||
|
|
@ -231,6 +237,7 @@ describe("ToolDetailPanel", () => {
|
|||
<ToolDetailPanel
|
||||
{...baseProps}
|
||||
tool={simpleTool}
|
||||
formValues={{ message: "hi" }}
|
||||
onExecute={onExecute}
|
||||
/>,
|
||||
);
|
||||
|
|
@ -502,4 +509,61 @@ describe("ToolDetailPanel", () => {
|
|||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
// #2020: Execute used to be gated on `isExecuting` alone — this panel never
|
||||
// called `hasMissingRequiredFields`, unlike the App and elicitation panels —
|
||||
// so arguments that could not be sent were executed anyway and simply arrived
|
||||
// absent at the server.
|
||||
describe("submit gating (#2020)", () => {
|
||||
const optionalJsonTool: Tool = {
|
||||
name: "record_shipment",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
// An array of no declared item type renders through the JSON editor,
|
||||
// which is where a draft can fail to parse.
|
||||
properties: { payload: { type: "array", title: "Payload" } },
|
||||
},
|
||||
};
|
||||
|
||||
it("disables Execute while a required argument has no value", () => {
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel {...baseProps} tool={simpleTool} formValues={{}} />,
|
||||
);
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Execute Tool" }),
|
||||
).toBeDisabled();
|
||||
});
|
||||
|
||||
it("enables Execute once the required argument is filled", () => {
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel
|
||||
{...baseProps}
|
||||
tool={simpleTool}
|
||||
formValues={{ message: "hi" }}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Execute Tool" }),
|
||||
).not.toBeDisabled();
|
||||
});
|
||||
|
||||
it("disables Execute while an optional field holds text it cannot send", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel {...baseProps} tool={optionalJsonTool} />,
|
||||
);
|
||||
const execute = screen.getByRole("button", { name: "Execute Tool" });
|
||||
expect(execute).not.toBeDisabled();
|
||||
|
||||
// The draft never reaches `formValues` — the field reports `undefined`
|
||||
// for text it cannot parse — so only the form's validity channel makes
|
||||
// this visible to the gate.
|
||||
const jsonInput = screen.getByLabelText(/Payload/);
|
||||
await user.type(jsonInput, "x");
|
||||
expect(execute).toBeDisabled();
|
||||
|
||||
await user.clear(jsonInput);
|
||||
expect(execute).not.toBeDisabled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -19,7 +19,10 @@ import type {
|
|||
ToolAnnotations,
|
||||
} from "@modelcontextprotocol/client";
|
||||
import { resolveDisplayLabel } from "../../../utils/toolUtils";
|
||||
import { toFormSchema } from "../../../utils/jsonUtils";
|
||||
import {
|
||||
hasMissingRequiredFields,
|
||||
toFormSchema,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { getMirroredHeaderParams } from "@inspector/core/json/xMcpHeader.js";
|
||||
import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge";
|
||||
import { ProgressDisplay } from "../../elements/ProgressDisplay/ProgressDisplay";
|
||||
|
|
@ -213,6 +216,10 @@ export function ToolDetailPanel({
|
|||
// a prior tool's hidden state doesn't carry over — mirrors how ToolsScreen
|
||||
// clears formValues on change.
|
||||
const [descriptionOpen, setDescriptionOpen] = useState(true);
|
||||
// Text a field is holding that it could not turn into a value — unparseable
|
||||
// JSON, or a number this client cannot send exactly. Reported by the form
|
||||
// because the draft lives inside the field and never reaches `formValues`.
|
||||
const [hasInvalidDraft, setHasInvalidDraft] = useState(false);
|
||||
const [prevToolName, setPrevToolName] = useState(name);
|
||||
if (name !== prevToolName) {
|
||||
setPrevToolName(name);
|
||||
|
|
@ -245,6 +252,15 @@ export function ToolDetailPanel({
|
|||
(taskSupport === "required" ||
|
||||
((taskSupport === "optional" || modernTasks) && runAsTask));
|
||||
|
||||
// Two distinct reasons the arguments aren't sendable, and this panel used to
|
||||
// check neither — Execute was gated on `isExecuting` alone, so a required
|
||||
// argument left empty and a field full of unparseable text were both
|
||||
// executable, and simply arrived at the server absent (#2020).
|
||||
const executeDisabled =
|
||||
isExecuting ||
|
||||
hasMissingRequiredFields(formSchema, formValues) ||
|
||||
hasInvalidDraft;
|
||||
|
||||
return (
|
||||
<PanelStack>
|
||||
<PinnedHeader>
|
||||
|
|
@ -321,6 +337,7 @@ export function ToolDetailPanel({
|
|||
// so the form needs the tool name to drop another tool's
|
||||
// in-progress field text. See SchemaFormProps.resetKey.
|
||||
resetKey={name}
|
||||
onValidityChange={setHasInvalidDraft}
|
||||
/>
|
||||
|
||||
{progress && <ProgressDisplay params={progress} />}
|
||||
|
|
@ -341,7 +358,7 @@ export function ToolDetailPanel({
|
|||
<Button
|
||||
size="md"
|
||||
onClick={() => onExecute(effectiveRunAsTask)}
|
||||
disabled={isExecuting}
|
||||
disabled={executeDisabled}
|
||||
loading={isExecuting}
|
||||
>
|
||||
Execute Tool
|
||||
|
|
|
|||
Loading…
Reference in a new issue