Merge pull request #1972 from modelcontextprotocol/v2/fix/1888-float-parameter-entry

fix(web): keep the text being typed in a schema number field (#1888)

clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx

@@ -149,4 +149,28 @@ describe("AppDetailPanel", () => {
     await user.click(screen.getByRole("button", { name: /open app/i }));
     expect(onOpenApp).toHaveBeenCalledTimes(1);
   });
+
+  it("drops a previous app's in-progress number text when the app changes", async () => {
+    const user = userEvent.setup();
+    // AppsScreen swaps selectedAppName + formValues in place rather than
+    // remounting this panel, so two apps exposing a same-named number field
+    // share the field component. Both values here are undefined, which the
+    // draft/value re-sync cannot distinguish — only the resetKey identity can.
+    const numberFieldTool = (name: string): Tool => ({
+      name,
+      title: name,
+      inputSchema: {
+        type: "object",
+        properties: { scale: { type: "number", title: "Scale" } },
+      },
+    });
+    const { rerender } = renderWithMantine(
+      <AppDetailPanel {...baseProps} tool={numberFieldTool("app_a")} />,
+    );
+    const input = () => screen.getByLabelText(/Scale/) as HTMLInputElement;
+    await user.type(input(), "-");
+    expect(input().value).toBe("-");
+    rerender(<AppDetailPanel {...baseProps} tool={numberFieldTool("app_b")} />);
+    expect(input().value).toBe("");
+  });
 });

clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx

@@ -76,6 +76,12 @@ export function AppDetailPanel({
           values={formValues}
           onChange={onFormChange}
           disabled={isOpening}
+          // Like ToolDetailPanel, this panel is reused across app selections
+          // rather than remounted (AppsScreen.handleSelect swaps
+          // selectedAppName + formValues in place), so the form needs the app
+          // tool's name to drop another app's in-progress field text. See
+          // SchemaFormProps.resetKey.
+          resetKey={tool.name}
         />
 
         <OpenAppButton

clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx

@@ -1,7 +1,12 @@
+import { useState } from "react";
 import { describe, it, expect, vi } from "vitest";
 import userEvent from "@testing-library/user-event";
 import type { InspectorFormSchema } from "../../../utils/jsonUtils";
-import { renderWithMantine, screen } from "../../../test/renderWithMantine";
+import {
+  fireEvent,
+  renderWithMantine,
+  screen,
+} from "../../../test/renderWithMantine";
 import { SchemaForm } from "./SchemaForm";
 
 describe("SchemaForm", () => {
@@ -228,6 +233,238 @@ describe("SchemaForm", () => {
     expect(lastCall.count).toBeUndefined();
   });
 
+  describe("number fields (#1888)", () => {
+    // The real callers keep the form values in state and feed them back in, so
+    // the bug only reproduces against a genuinely controlled SchemaForm: an
+    // uncontrolled render never rewrites the box and would pass either way.
+    function ControlledSchemaForm({
+      schema,
+      initialValues = {},
+      onChange,
+    }: {
+      schema: InspectorFormSchema;
+      initialValues?: Record<string, unknown>;
+      onChange: (values: Record<string, unknown>) => void;
+    }) {
+      const [values, setValues] =
+        useState<Record<string, unknown>>(initialValues);
+      return (
+        <SchemaForm
+          schema={schema}
+          values={values}
+          onChange={(next) => {
+            setValues(next);
+            onChange(next);
+          }}
+        />
+      );
+    }
+
+    const numberSchema: InspectorFormSchema = {
+      type: "object",
+      properties: {
+        divisor: { type: "number", title: "Divisor" },
+      },
+    };
+
+    it("lets a decimal be typed all the way through", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input, "1.5");
+      expect(input.value).toBe("1.5");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: 1.5 });
+    });
+
+    it("keeps the trailing decimal point visible mid-entry", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input, "1.");
+      // The point survives on screen even though "1." parses to plain 1.
+      expect(input.value).toBe("1.");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: 1 });
+    });
+
+    it("keeps a trailing zero after the decimal point", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input, "1.50");
+      expect(input.value).toBe("1.50");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: 1.5 });
+    });
+
+    it("reports a lone minus sign as no value while leaving it typed", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input, "-");
+      expect(input.value).toBe("-");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: undefined });
+      await user.type(input, "2.5");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: -2.5 });
+    });
+
+    it("rejects a decimal point in an integer field", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      const schema: InspectorFormSchema = {
+        type: "object",
+        properties: {
+          count: { type: "integer", title: "Count" },
+        },
+      };
+      renderWithMantine(
+        <ControlledSchemaForm schema={schema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Count/) as HTMLInputElement;
+      await user.type(input, "1.5");
+      expect(input.value).toBe("15");
+      expect(onChange).toHaveBeenLastCalledWith({ count: 15 });
+    });
+
+    it("reports no value for a magnitude JS cannot hold exactly", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      // Past Number.MAX_SAFE_INTEGER, Mantine stops emitting a number and hands
+      // back the raw string to avoid destroying precision. Number() would round
+      // this to ...904, so parsing it would send a value the user never typed.
+      await user.type(input, "90071992547409910");
+      expect(input.value).toBe("90071992547409910");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: undefined });
+    });
+
+    it("still parses a long decimal, which stays exactly representable", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      renderWithMantine(
+        <ControlledSchemaForm schema={numberSchema} onChange={onChange} />,
+      );
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      // Guards the safe-integer check against over-rejecting: the fractional
+      // digits are not what overflows, so this must not be dropped.
+      await user.type(input, "3.14159265358979");
+      expect(onChange).toHaveBeenLastCalledWith({ divisor: 3.14159265358979 });
+    });
+
+    it("drops in-progress text when resetKey moves the form to another entity", async () => {
+      const user = userEvent.setup();
+      // Both "tools" expose the same-named number field with no default, so the
+      // value is `undefined` before and after the switch. The value comparison
+      // sees no divergence, and only resetKey can tell the field to start over.
+      const schema: InspectorFormSchema = {
+        type: "object",
+        properties: { divisor: { type: "number", title: "Divisor" } },
+      };
+      function Harness() {
+        const [tool, setTool] = useState("tool-a");
+        const [values, setValues] = useState<Record<string, unknown>>({});
+        return (
+          <>
+            <button
+              type="button"
+              onClick={() => {
+                setTool("tool-b");
+                // What ToolsScreen does on select: replace the form values.
+                setValues({});
+              }}
+            >
+              Switch tool
+            </button>
+            <SchemaForm
+              schema={schema}
+              values={values}
+              onChange={setValues}
+              resetKey={tool}
+            />
+          </>
+        );
+      }
+      renderWithMantine(<Harness />);
+      const input = () => screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input(), "-");
+      expect(input().value).toBe("-");
+      // fireEvent, not user.click: a real click also blurs the input, and
+      // Mantine sanitizes an incomplete value on blur — which would mask
+      // whether the switch itself cleared the draft. This drives the state
+      // change without the blur, isolating the reset to resetKey.
+      fireEvent.click(screen.getByRole("button", { name: "Switch tool" }));
+      expect(input().value).toBe("");
+    });
+
+    it("keeps in-progress text across re-renders of the same entity", async () => {
+      const user = userEvent.setup();
+      // The counterpart to the test above: a stable resetKey must NOT remount
+      // the field, or every keystroke would wipe the draft and reinstate #1888.
+      const schema: InspectorFormSchema = {
+        type: "object",
+        properties: { divisor: { type: "number", title: "Divisor" } },
+      };
+      function Harness() {
+        const [values, setValues] = useState<Record<string, unknown>>({});
+        return (
+          <SchemaForm
+            schema={schema}
+            values={values}
+            onChange={setValues}
+            resetKey="tool-a"
+          />
+        );
+      }
+      renderWithMantine(<Harness />);
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      await user.type(input, "1.5");
+      expect(input.value).toBe("1.5");
+    });
+
+    it("re-syncs the displayed text when the value changes externally", async () => {
+      const user = userEvent.setup();
+      const onChange = vi.fn();
+      function Harness() {
+        const [values, setValues] = useState<Record<string, unknown>>({
+          divisor: 1.5,
+        });
+        return (
+          <>
+            <button type="button" onClick={() => setValues({ divisor: 42 })}>
+              Load example
+            </button>
+            <SchemaForm
+              schema={numberSchema}
+              values={values}
+              onChange={(next) => {
+                setValues(next);
+                onChange(next);
+              }}
+            />
+          </>
+        );
+      }
+      renderWithMantine(<Harness />);
+      const input = screen.getByLabelText(/Divisor/) as HTMLInputElement;
+      expect(input.value).toBe("1.5");
+      await user.click(screen.getByRole("button", { name: "Load example" }));
+      expect(input.value).toBe("42");
+    });
+  });
+
   it("falls back to empty/const labels for oneOf items missing const and title", () => {
     const onChange = vi.fn();
     const schema: InspectorFormSchema = {

clients/web/src/components/groups/SchemaForm/SchemaForm.tsx

@@ -8,7 +8,9 @@ import {
   Text,
   TextInput,
 } from "@mantine/core";
+import { useState } from "react";
 import { ClearButton } from "../../elements/ClearButton/ClearButton";
+import { useValueChange } from "../../../hooks/useValueChange";
 import type { InspectorFormSchema } from "../../../utils/jsonUtils";
 
 const FieldLabel = Text.withProps({
@@ -49,11 +51,129 @@ function toEnumData(
   return values;
 }
 
+/**
+ * Interpret whatever Mantine's `NumberInput` reported as the JSON value for the
+ * field. Anything that is not a finite number becomes `undefined`, which is how
+ * an absent optional argument is represented everywhere else in this form.
+ *
+ * `NumberInput` emits a `number` only when the text both parses *and* is exactly
+ * representable; otherwise it hands back the **raw string** (see its
+ * `isValidNumber` guard). Two quite different situations produce a string, and
+ * they are treated differently here:
+ *
+ * 1. **Mid-entry text** — `""` when cleared, plus `"1."`, `"1.50"`, and a lone
+ *    `"-"`. These are parsed: `"1."` really does mean `1`. (Note that an
+ *    exponent is *not* in this set — `NumberInput` masks input through
+ *    `NumericFormat`, which rejects `e` outright, so `"1e"` can never be typed.)
+ * 2. **Values JS cannot hold exactly** — anything at or beyond
+ *    `Number.MAX_SAFE_INTEGER`. `Number("90071992547409910")` silently yields
+ *    `90071992547409904`, so parsing here would send the server a number the
+ *    user never entered. An inspector must not misreport what it transmits, so
+ *    these report no value instead — which is also what this field did with such
+ *    input before #1888, making it no regression. Preserving them properly needs
+ *    an exact-serialization path down the whole `tools/call` chain, which is a
+ *    separate concern from being able to type a decimal.
+ */
+function toNumericValue(raw: string | number): number | undefined {
+  if (typeof raw === "number") {
+    return Number.isFinite(raw) ? raw : undefined;
+  }
+  if (raw.trim() === "") {
+    return undefined;
+  }
+  const parsed = Number(raw);
+  if (!Number.isFinite(parsed)) {
+    return undefined;
+  }
+  // Case 2 above. The integer part is what overflows exact representation; the
+  // fractional digits are bounded by the same guard and stay lossless.
+  return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined;
+}
+
+interface SchemaNumberInputProps {
+  label: string;
+  description?: string;
+  withAsterisk: boolean;
+  disabled: boolean;
+  value: number | undefined;
+  min?: number;
+  max?: number;
+  allowDecimal: boolean;
+  onChange: (value: number | undefined) => void;
+}
+
+/**
+ * A `NumberInput` that keeps the text the user is typing, not just the number it
+ * currently parses to.
+ *
+ * Driving `NumberInput` directly off the parent's numeric value makes a decimal
+ * impossible to enter (#1888): typing `.` after `1` produces the unparseable
+ * string `"1."`, the numeric value stays `1`, and the controlled `value` prop
+ * immediately rewrites the box back to `"1"` — so the `.` vanishes and `1.5` can
+ * never be reached. Trailing zeros (`"1.50"`) and a lone leading `"-"` fail the
+ * same way.
+ *
+ * So the raw text is held here as the source of truth for what is *displayed*,
+ * while the parent still only ever sees a `number | undefined`. The two are
+ * re-synced only when the parent's value genuinely diverges from what the draft
+ * parses to, which leaves an external reset (a cleared form, a loaded example)
+ * working while an in-progress `"1."` — whose parse is `1`, matching the value we
+ * just emitted — is left alone.
+ *
+ * That value comparison cannot see a reset to an *equal* value, so the caller is
+ * additionally expected to vary this component's React key via `SchemaForm`'s
+ * `resetKey` when it switches which entity the form edits. See the note on that
+ * prop for the case it covers.
+ */
+function SchemaNumberInput({
+  value,
+  onChange,
+  ...inputProps
+}: SchemaNumberInputProps) {
+  const [draft, setDraft] = useState<string | number>(value ?? "");
+
+  useValueChange(value, (next) => {
+    if (!Object.is(toNumericValue(draft), next)) {
+      setDraft(next ?? "");
+    }
+  });
+
+  return (
+    <NumberInput
+      {...inputProps}
+      value={draft}
+      onChange={(next) => {
+        setDraft(next);
+        onChange(toNumericValue(next));
+      }}
+    />
+  );
+}
+
 export interface SchemaFormProps {
   schema: InspectorFormSchema;
   values: Record<string, unknown>;
   onChange: (values: Record<string, unknown>) => void;
   disabled?: boolean;
+  /**
+   * Stable identity of whatever this form is editing — a tool name, a request
+   * id. Pass it whenever the same mounted form is reused for a *different*
+   * entity, which is the case for the Tools tab: `ToolDetailPanel` is not keyed
+   * by tool, so selecting another tool re-renders the same field components.
+   *
+   * It exists because the number field's draft/value re-sync compares parsed
+   * numbers, and so cannot detect a reset to an equal value. Type `-` (draft
+   * `"-"`, value `undefined`), then switch to a tool with a same-named number
+   * field and no default: the value is `undefined` on both sides, no divergence
+   * is seen, and the stale `-` would otherwise be left in the box for the new
+   * tool to continue from. Varying `resetKey` remounts the field instead, so no
+   * in-progress text can outlive the entity it was typed into.
+   *
+   * Omit it when the form is mounted fresh per entity (the elicitation panels),
+   * where unmounting already discards the draft. The schema object itself is no
+   * substitute — callers rebuild it every render, so its identity is unstable.
+   */
+  resetKey?: string;
 }
 
 function getDefaultValue(fieldSchema: InspectorFormSchema): unknown {
@@ -78,6 +198,7 @@ export function SchemaForm({
   values,
   onChange,
   disabled = false,
+  resetKey,
 }: SchemaFormProps) {
   const properties = schema.properties ?? {};
   const requiredFields = schema.required ?? [];
@@ -156,19 +277,21 @@ export function SchemaForm({
     // number or integer
     if (fieldSchema.type === "number" || fieldSchema.type === "integer") {
       return (
-        <NumberInput
-          key={fieldName}
+        <SchemaNumberInput
+          // 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}`}
           label={label}
           description={description}
           withAsterisk={isRequired}
           disabled={disabled}
-          value={(rawValue as number) ?? ""}
+          value={rawValue as number | undefined}
           min={fieldSchema.minimum}
           max={fieldSchema.maximum}
-          onChange={(val) => {
-            const numericValue = typeof val === "string" ? undefined : val;
-            handleFieldChange(fieldName, numericValue);
-          }}
+          // An `integer` field rejects the decimal point outright rather than
+          // accepting a value the schema forbids.
+          allowDecimal={fieldSchema.type === "number"}
+          onChange={(val) => handleFieldChange(fieldName, val)}
         />
       );
     }
@@ -243,6 +366,8 @@ export function SchemaForm({
                 handleFieldChange(fieldName, nestedValues)
               }
               disabled={disabled}
+              // Sub-fields belong to the same entity, so they reset with it.
+              resetKey={resetKey}
             />
           </IndentedStack>
         </Stack>

clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx

@@ -317,6 +317,10 @@ export function ToolDetailPanel({
             values={formValues}
             onChange={onFormChange}
             disabled={isExecuting}
+            // This panel is reused across tool selections rather than remounted,
+            // so the form needs the tool name to drop another tool's
+            // in-progress field text. See SchemaFormProps.resetKey.
+            resetKey={name}
           />
 
           {progress && <ProgressDisplay params={progress} />}