fix(web): don't round unrepresentable numbers, and reset app-switch drafts
Review round 2. Three findings, all real. 1. Precision. Mantine's `NumberInput` returns the raw *string* rather than a number once the value reaches `Number.MAX_SAFE_INTEGER` (see its `isValidNumber` guard), precisely because JS cannot hold it. Parsing that turned `90071992547409910` into `90071992547409904` and would have sent the server a number the user never typed. An inspector must not misreport what it transmits, so such input now reports no value — which is also what this field did before #1888, so nothing regresses. Only the integer part overflows, so long decimals stay parsed. 2. `AppDetailPanel` has the same reuse pattern as `ToolDetailPanel`: `AppsScreen.handleSelect` swaps `selectedAppName` + `formValues` in place without remounting, so switching between apps with a same-named number field could retain an equal-valued draft. It now passes `resetKey` too. 3. An exponent was never reachable: `NumberInput` masks input through `NumericFormat`, which rejects `e`. Dropped that claim from the doc comment rather than implying `"1e"` is a case being handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
This commit is contained in:
parent
24a9786337
commit
6fd03d6de4
4 changed files with 84 additions and 5 deletions
|
|
@ -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("");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -336,6 +336,34 @@ describe("SchemaForm", () => {
|
|||
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
|
||||
|
|
|
|||
|
|
@ -53,10 +53,26 @@ function toEnumData(
|
|||
|
||||
/**
|
||||
* Interpret whatever Mantine's `NumberInput` reported as the JSON value for the
|
||||
* field. It emits a `number` once the text parses cleanly, and the **raw string**
|
||||
* while it does not — `""` when cleared, but also the in-progress `"1."`, `"-"`,
|
||||
* and `"1e"`. Anything that is not a finite number becomes `undefined`, which is
|
||||
* how an absent optional argument is represented everywhere else in this form.
|
||||
* 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") {
|
||||
|
|
@ -66,7 +82,12 @@ function toNumericValue(raw: string | number): number | undefined {
|
|||
return undefined;
|
||||
}
|
||||
const parsed = Number(raw);
|
||||
return Number.isFinite(parsed) ? parsed : undefined;
|
||||
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 {
|
||||
|
|
|
|||
Loading…
Reference in a new issue