Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 135 additions & 0 deletions clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { useState } from "react";
import { describe, it, expect, vi } from "vitest";
import userEvent from "@testing-library/user-event";
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
Expand Down Expand Up @@ -228,6 +229,140 @@ 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("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 = {
Expand Down
87 changes: 81 additions & 6 deletions clients/web/src/components/groups/SchemaForm/SchemaForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -49,6 +51,79 @@ function toEnumData(
return values;
}

/**
* 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.
*/
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);
Comment thread
cliffhall marked this conversation as resolved.
return Number.isFinite(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.
*/
function SchemaNumberInput({
value,
onChange,
...inputProps
}: SchemaNumberInputProps) {
const [draft, setDraft] = useState<string | number>(value ?? "");

useValueChange(value, (next) => {
if (!Object.is(toNumericValue(draft), next)) {
setDraft(next ?? "");
}
});
Comment thread
cliffhall marked this conversation as resolved.

return (
<NumberInput
{...inputProps}
value={draft}
onChange={(next) => {
setDraft(next);
onChange(toNumericValue(next));
}}
/>
);
}

export interface SchemaFormProps {
schema: InspectorFormSchema;
values: Record<string, unknown>;
Expand Down Expand Up @@ -156,19 +231,19 @@ export function SchemaForm({
// number or integer
if (fieldSchema.type === "number" || fieldSchema.type === "integer") {
return (
<NumberInput
<SchemaNumberInput
key={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)}
/>
);
}
Expand Down
Loading