diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index 2d709683..5ba5d9d6 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -88,6 +88,16 @@ describe("EditReplayModal", () => { expect(screen.getByText(/must be a JSON object/i)).toBeInTheDocument(); }); + // `JSON.parse("1e400")` yields `Infinity`, which `JSON.stringify` writes back + // as `null` — so Send would dispatch a number the user never typed while the + // editor still showed what they wrote. + it("disables Send for a number that cannot survive being sent", async () => { + renderWithMantine(); + await setAceText('{"limit":1e400}'); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + expect(screen.getByText(/Numbers must be finite/)).toBeInTheDocument(); + }); + it("closes without sending when cancelled", async () => { const user = userEvent.setup(); const onSend = vi.fn(); diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx index e0476517..784db9b7 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -2,6 +2,7 @@ import { useState } from "react"; import { Button, Group, Modal, Stack, Text } from "@mantine/core"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { isJsonObject } from "../../../utils/jsonObjectDraft"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; export interface EditReplayModalProps { opened: boolean; @@ -34,7 +35,8 @@ const Footer = Group.withProps({ justify: "flex-end", gap: "sm" }); * genuinely carries none, and an empty editor wearing a red error would read as * broken. A non-object is reported separately from text that is not JSON at * all: JSON-RPC `params` must be a structured value, so `42` is a shape that - * cannot be sent however finished it is. + * cannot be sent however finished it is. A number that overflows is a third + * case, and the subtlest — see the note on that branch. */ function parseParamsDraft( text: string, @@ -51,6 +53,17 @@ function parseParamsDraft( if (!isJsonObject(parsed)) { return { ok: false, error: "Params must be a JSON object (`{ … }`)" }; } + // `JSON.parse` accepts numeric literals it cannot represent: `1e400` parses to + // `Infinity`, which `JSON.stringify` then writes as `null`. Accepting it here + // would show the user one value and send another — the one thing an inspector + // must not do. Same guard, same reason, as `parseJsonObjectDraft`. + if (!isSerializableJson(parsed)) { + return { + ok: false, + error: + "Numbers must be finite — a value like `1e400` overflows and would be sent as null", + }; + } return { ok: true, value: parsed }; } diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 6f913944..08eab255 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1899,6 +1899,143 @@ describe("SchemaForm raw JSON (#2151)", () => { expect(screen.queryByText(/Not valid JSON/)).toBeNull(); }); + // `JSON.parse("1e400")` yields `Infinity`, which `JSON.stringify` writes back + // as `null`. Submitting it would send a number the user never typed while the + // editor still showed what they wrote. + it("blocks submission for a number that cannot survive being sent", async () => { + const user = userEvent.setup(); + const onValidityChange = vi.fn(); + renderWithMantine(); + await enableRawJson(user); + + await setAceTextByLabel(/Arguments JSON/, '{"count":1e400}'); + expect(onValidityChange).toHaveBeenLastCalledWith(true); + expect(screen.getByText(/Numbers must be finite/)).toBeInTheDocument(); + }); + + // Nothing in `values` names a branch, so the picker's index is held by the + // form. Editing the discriminator in the raw document is a change to which + // branch is in effect, and it carries no other signal — without the re-derive + // the picker keeps showing the outgoing branch over the incoming values. + it("moves the branch picker when the raw document changes the discriminator", async () => { + const user = userEvent.setup(); + const union: InspectorFormSchema = { + type: "object", + anyOf: [ + { + type: "object", + title: "Email", + properties: { + kind: { type: "string", const: "email" }, + address: { type: "string", title: "Address" }, + }, + required: ["kind"], + }, + { + type: "object", + title: "SMS", + properties: { + kind: { type: "string", const: "sms" }, + phone: { type: "string", title: "Phone" }, + }, + required: ["kind"], + }, + ], + }; + renderWithMantine( + , + ); + expect( + (screen.getByRole("textbox", { name: /Variant/ }) as HTMLInputElement) + .value, + ).toBe("Email"); + + await enableRawJson(user); + await setAceTextByLabel( + /Arguments JSON/, + '{"kind":"sms","phone":"555-0100"}', + ); + + expect( + (screen.getByRole("textbox", { name: /Variant/ }) as HTMLInputElement) + .value, + ).toBe("SMS"); + + // And the widgets agree with it on the way back. + await user.click(screen.getByLabelText("Edit as JSON")); + // Anchored: the branch picker's own options carry these words too. + expect(screen.getByLabelText(/^Phone$/)).toBeInTheDocument(); + expect(screen.queryByLabelText(/^Address$/)).toBeNull(); + }); + + // The editor holds the whole arguments object, not a per-branch slice, so a + // branch move must not remount it — that would reformat the text being typed + // and drop the caret to the top. Pinned by the text surviving verbatim: a + // remount reseeds from `values`, which is pretty-printed. + it("does not reformat the draft when the branch moves under it", async () => { + const user = userEvent.setup(); + const union: InspectorFormSchema = { + type: "object", + anyOf: [ + { + type: "object", + title: "Email", + properties: { kind: { type: "string", const: "email" } }, + required: ["kind"], + }, + { + type: "object", + title: "SMS", + properties: { kind: { type: "string", const: "sms" } }, + required: ["kind"], + }, + ], + }; + renderWithMantine( + , + ); + await enableRawJson(user); + + await setAceTextByLabel(/Arguments JSON/, '{"kind":"sms"}'); + expect(getAceTextByLabel(/Arguments JSON/)).toBe('{"kind":"sms"}'); + }); + + // The discriminator is cleared and retyped on the way between branches, so + // snapping to the first branch the moment it stops matching would move the + // picker under the user mid-edit. + it("keeps the current branch while the discriminator is absent", async () => { + const user = userEvent.setup(); + const union: InspectorFormSchema = { + type: "object", + anyOf: [ + { + type: "object", + title: "Email", + properties: { kind: { type: "string", const: "email" } }, + required: ["kind"], + }, + { + type: "object", + title: "SMS", + properties: { kind: { type: "string", const: "sms" } }, + required: ["kind"], + }, + ], + }; + renderWithMantine(); + await enableRawJson(user); + expect( + (screen.getByRole("textbox", { name: /Variant/ }) as HTMLInputElement) + .value, + ).toBe("SMS"); + + await setAceTextByLabel(/Arguments JSON/, '{"note":"mid-edit"}'); + expect( + (screen.getByRole("textbox", { name: /Variant/ }) as HTMLInputElement) + .value, + ).toBe("SMS"); + }); + // #2123: switching a root union drops the outgoing branch's values. The raw // editor is seeded from what the form holds, so a round trip through it must // not resurrect them. diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 360347e4..a24649a8 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -25,6 +25,7 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { EnlargeButton } from "../../elements/EnlargeButton/EnlargeButton"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { isJsonObject } from "../../../utils/jsonObjectDraft"; +import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; import { useValueChange } from "../../../hooks/useValueChange"; import type { InspectorFormSchema, @@ -272,16 +273,29 @@ function toNumericValue(raw: string | number): number | undefined { return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined; } -/** Parse editor text, reporting `undefined` for anything that is not JSON yet. */ +/** + * Parse editor text, reporting `undefined` for anything this client cannot + * send. + * + * That covers text that is not JSON *yet* — the mid-edit state this field's + * whole draft/value split exists for — and text that parses but does not + * survive being written back out: `JSON.parse("1e400")` yields `Infinity`, + * which `JSON.stringify` writes as `null`. Both report no value, so the field + * shows its error and `onValidityChange` blocks submission, rather than sending + * a number the user never typed. Same guard, same reason, as + * `parseJsonObjectDraft` and the raw-arguments editor above. + */ function parseJsonDraft(text: string): unknown { if (text.trim() === "") { return undefined; } + let parsed: unknown; try { - return JSON.parse(text); + parsed = JSON.parse(text); } catch { return undefined; } + return isSerializableJson(parsed) ? parsed : undefined; } /** Structural equality for the draft/value re-sync, via canonical JSON. */ @@ -584,6 +598,17 @@ function parseRawArgumentsDraft( error: "Arguments must be a JSON object (`{ … }`)", }; } + // `JSON.parse` accepts numeric literals it cannot represent: `1e400` parses to + // `Infinity`, which `JSON.stringify` then writes as `null`. Accepting it here + // would show the user one value and send another — the one thing an inspector + // must not do. Same guard, same reason, as `parseJsonObjectDraft`. + if (!isSerializableJson(parsed)) { + return { + ok: false, + error: + "Numbers must be finite — a value like `1e400` overflows and would be sent as null", + }; + } return { ok: true, value: parsed }; } @@ -942,6 +967,27 @@ export function SchemaForm({ onChange({ ...values, [fieldName]: fieldValue }); } + /** + * Take the raw editor's object, and move the branch picker with it. + * + * Nothing in `values` names a branch, so the picker's index is held here and + * is only ever re-derived when something says the values changed underneath + * (`resetKey`, a rewritten union). Editing the discriminator in the raw + * document is exactly that, and it has no other signal: without this, turning + * `{"kind":"email"}` into `{"kind":"sms",…}` leaves the picker on Email, and + * switching back to the widgets renders the Email branch over SMS values — + * showing one shape while submitting another. + * + * The current branch is kept when the values identify none, rather than + * snapping to the first: the user passes through that state every time they + * clear the discriminator to retype it. + */ + function handleRawArgumentsChange(next: Record) { + onChange(next); + if (branches.length === 0) return; + setBranchIndex(selectBranchIndex(branches, next) ?? branchIndex); + } + /** * Switch branches, and move `values` with the form. * @@ -1413,13 +1459,17 @@ export function SchemaForm({ /> )} {rawJsonMode ? ( - // Keyed by the entity and branch, like every draft-holding field: text - // typed for one tool must not be left in the box for the next, and - // switching a root union prunes values the stale draft would restore. + // Keyed by the *entity* only, unlike the per-field editors, which are + // keyed by entity and branch. This editor holds the whole arguments + // object, which is not a per-branch thing: switching branches rewrites + // `values`, and the re-sync below picks that up without a remount. + // Keying it by branch too would remount it the moment an edit to the + // discriminator moved the picker — reformatting the text the user was + // typing and dropping their caret to the top.