fix(web): reset a number field's draft text when the form changes entity
Review follow-up. The draft/value re-sync compares parsed numbers, so it cannot see a reset to an *equal* value. `ToolDetailPanel` is not keyed by tool, so its fields are reused across selections: type `-` (draft `"-"`, value `undefined`), switch to a tool with a same-named number field and no default, and the value is `undefined` on both sides — no divergence is detected and the stale `-` stays in the box for the new tool. Add a `resetKey` prop carrying the identity of whatever the form edits, and vary the number field's React key with it so no in-progress text can outlive the entity it was typed into. `ToolDetailPanel` passes the tool name; nested object forms inherit it. The schema object is no substitute — callers rebuild it every render, so its identity is unstable. The regression test drives the switch with `fireEvent` rather than `user.click`: a real click also blurs the input, and Mantine sanitizes an incomplete value on blur, which masked the defect entirely. A paired test asserts a *stable* `resetKey` does not remount, since remounting on every render would wipe the draft and reinstate #1888. 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
51bbe9996b
commit
24a9786337
3 changed files with 109 additions and 2 deletions
|
|
@ -2,7 +2,11 @@ import { useState } from "react";
|
||||||
import { describe, it, expect, vi } from "vitest";
|
import { describe, it, expect, vi } from "vitest";
|
||||||
import userEvent from "@testing-library/user-event";
|
import userEvent from "@testing-library/user-event";
|
||||||
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
|
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
|
||||||
import { renderWithMantine, screen } from "../../../test/renderWithMantine";
|
import {
|
||||||
|
fireEvent,
|
||||||
|
renderWithMantine,
|
||||||
|
screen,
|
||||||
|
} from "../../../test/renderWithMantine";
|
||||||
import { SchemaForm } from "./SchemaForm";
|
import { SchemaForm } from "./SchemaForm";
|
||||||
|
|
||||||
describe("SchemaForm", () => {
|
describe("SchemaForm", () => {
|
||||||
|
|
@ -332,6 +336,76 @@ describe("SchemaForm", () => {
|
||||||
expect(onChange).toHaveBeenLastCalledWith({ count: 15 });
|
expect(onChange).toHaveBeenLastCalledWith({ count: 15 });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
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 () => {
|
it("re-syncs the displayed text when the value changes externally", async () => {
|
||||||
const user = userEvent.setup();
|
const user = userEvent.setup();
|
||||||
const onChange = vi.fn();
|
const onChange = vi.fn();
|
||||||
|
|
|
||||||
|
|
@ -98,6 +98,11 @@ interface SchemaNumberInputProps {
|
||||||
* parses to, which leaves an external reset (a cleared form, a loaded example)
|
* 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
|
* working while an in-progress `"1."` — whose parse is `1`, matching the value we
|
||||||
* just emitted — is left alone.
|
* 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({
|
function SchemaNumberInput({
|
||||||
value,
|
value,
|
||||||
|
|
@ -129,6 +134,25 @@ export interface SchemaFormProps {
|
||||||
values: Record<string, unknown>;
|
values: Record<string, unknown>;
|
||||||
onChange: (values: Record<string, unknown>) => void;
|
onChange: (values: Record<string, unknown>) => void;
|
||||||
disabled?: boolean;
|
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 {
|
function getDefaultValue(fieldSchema: InspectorFormSchema): unknown {
|
||||||
|
|
@ -153,6 +177,7 @@ export function SchemaForm({
|
||||||
values,
|
values,
|
||||||
onChange,
|
onChange,
|
||||||
disabled = false,
|
disabled = false,
|
||||||
|
resetKey,
|
||||||
}: SchemaFormProps) {
|
}: SchemaFormProps) {
|
||||||
const properties = schema.properties ?? {};
|
const properties = schema.properties ?? {};
|
||||||
const requiredFields = schema.required ?? [];
|
const requiredFields = schema.required ?? [];
|
||||||
|
|
@ -232,7 +257,9 @@ export function SchemaForm({
|
||||||
if (fieldSchema.type === "number" || fieldSchema.type === "integer") {
|
if (fieldSchema.type === "number" || fieldSchema.type === "integer") {
|
||||||
return (
|
return (
|
||||||
<SchemaNumberInput
|
<SchemaNumberInput
|
||||||
key={fieldName}
|
// 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}
|
label={label}
|
||||||
description={description}
|
description={description}
|
||||||
withAsterisk={isRequired}
|
withAsterisk={isRequired}
|
||||||
|
|
@ -318,6 +345,8 @@ export function SchemaForm({
|
||||||
handleFieldChange(fieldName, nestedValues)
|
handleFieldChange(fieldName, nestedValues)
|
||||||
}
|
}
|
||||||
disabled={disabled}
|
disabled={disabled}
|
||||||
|
// Sub-fields belong to the same entity, so they reset with it.
|
||||||
|
resetKey={resetKey}
|
||||||
/>
|
/>
|
||||||
</IndentedStack>
|
</IndentedStack>
|
||||||
</Stack>
|
</Stack>
|
||||||
|
|
|
||||||
|
|
@ -317,6 +317,10 @@ export function ToolDetailPanel({
|
||||||
values={formValues}
|
values={formValues}
|
||||||
onChange={onFormChange}
|
onChange={onFormChange}
|
||||||
disabled={isExecuting}
|
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} />}
|
{progress && <ProgressDisplay params={progress} />}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue