diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b613699d..96aa1a999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Enter that confirms a typed character no longer saves a name, a rule or a wizard step + +Japanese, Chinese and Korean are typed through an input method, where Enter confirms the character +being built. In three fields that Enter also acted: editing a coworker's name or title saved it with +the character still unconfirmed, the new-coworker wizard moved on to its next step, and a boundary +rule was saved into the policy in force. Those fields now wait for the character, the way the chat +composer already does, and an ordinary Enter works as before. + ### A built-in coworker can be edited where the deployment's own Bot is on localhost A coworker created as Built in is stored pointing at the managed Bot's address. Editing its name, diff --git a/app/src/components/agents/agent-dialog.tsx b/app/src/components/agents/agent-dialog.tsx index cb95e1a23..25ff53b7a 100644 --- a/app/src/components/agents/agent-dialog.tsx +++ b/app/src/components/agents/agent-dialog.tsx @@ -70,6 +70,7 @@ import { updateAgentMutationOptions, } from "@/lib/agents/mutations"; import { type AgentProfile, agentQueryOptions } from "@/lib/agents/queries"; +import { isComposing } from "@/lib/composing"; import { agentPluginsQueryOptions } from "@/lib/plugins/queries"; import { readToolName } from "@/lib/plugins/tool-name"; @@ -438,7 +439,9 @@ function EditableTextItem({ autoFocus onChange={(event) => setDraft(event.target.value)} onKeyDown={(event) => { - if (event.key === "Enter") { + // Not the Enter that confirms a composed character: that one would save the value + // before the person has finished typing it. + if (event.key === "Enter" && !isComposing(event)) { event.preventDefault(); void submit(); } diff --git a/app/src/components/agents/create-agent-dialog.tsx b/app/src/components/agents/create-agent-dialog.tsx index eecc93675..ff06cf169 100644 --- a/app/src/components/agents/create-agent-dialog.tsx +++ b/app/src/components/agents/create-agent-dialog.tsx @@ -38,6 +38,7 @@ import { type ConnectionVerdict, testAgentConnection, } from "@/lib/agents/queries"; +import { isComposing } from "@/lib/composing"; import { queryClient } from "@/query-client"; /** @@ -254,12 +255,15 @@ function CreateAgentWizard({ * questionnaire's own submit path refuses any item it does not consider answered, and it * cannot see these fields: the identity inputs are this dialog's own, not registered * answers. Running first and preventing default also keeps the primitive's Enter - * handling out of the way; a textarea keeps Enter for its line breaks. + * handling out of the way; a textarea keeps Enter for its line breaks. The Enter that + * confirms a composed character is left alone, as the primitive leaves it: it finishes a + * character, not the step. */ onKeyDown={(event) => { if ( event.key === "Enter" && !event.shiftKey && + !isComposing(event) && event.target instanceof HTMLInputElement ) { event.preventDefault(); diff --git a/app/src/lib/composing.ts b/app/src/lib/composing.ts new file mode 100644 index 000000000..eb82d238e --- /dev/null +++ b/app/src/lib/composing.ts @@ -0,0 +1,16 @@ +import type { KeyboardEvent } from "react"; + +/** + * Whether a keydown belongs to a character an input method is still composing. + * + * Japanese, Chinese and Korean are typed through an input method, and Enter is how the character + * being built is confirmed. That press still arrives as a keydown with `key === "Enter"`: Chromium + * marks it `isComposing`, and WebKit sends it after `compositionend` with the key code 229 instead. + * A field that acts on Enter without asking this acts on text the person has not finished typing. + * + * The same two checks the libraries under this app already make: `prompt-area`, which draws the chat + * composer, and the questionnaire primitive both skip a keydown either one describes. + */ +export function isComposing(event: KeyboardEvent): boolean { + return event.nativeEvent.isComposing || event.keyCode === 229; +} diff --git a/app/src/routes/_authed/admin/boundaries.tsx b/app/src/routes/_authed/admin/boundaries.tsx index e96fe7d91..1511713a0 100644 --- a/app/src/routes/_authed/admin/boundaries.tsx +++ b/app/src/routes/_authed/admin/boundaries.tsx @@ -4,6 +4,7 @@ import { useState } from "react"; import { PageSection, PageShell } from "@/components/layout/page-shell"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; +import { isComposing } from "@/lib/composing"; import { saveActionPolicyMutationOptions } from "@/lib/computers/mutations"; import { type ActionPolicy, @@ -231,7 +232,9 @@ function BoundariesPage() { setTested(null); }} onKeyDown={(event) => { - if (event.key === "Enter") addRule(draft); + // Not the Enter that confirms a composed character, which would put a half-typed + // rule into the policy in force. + if (event.key === "Enter" && !isComposing(event)) addRule(draft); }} placeholder='tool.name == "computer_click" && contains(element.name, "submit")' value={draft} diff --git a/app/tests/composing-enter.test.tsx b/app/tests/composing-enter.test.tsx new file mode 100644 index 000000000..b0ea8fba4 --- /dev/null +++ b/app/tests/composing-enter.test.tsx @@ -0,0 +1,214 @@ +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + expect, + test, +} from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + createMemoryHistory, + createRootRoute, + createRouter, + RouterProvider, +} from "@tanstack/react-router"; +import { act, cleanup, fireEvent, render } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { ComponentType, ReactNode } from "react"; +import { AgentDialog } from "@/components/agents/agent-dialog"; +import { CreateAgentDialog } from "@/components/agents/create-agent-dialog"; +import { type AgentProfile, agentKeys } from "@/lib/agents/queries"; +import { computerKeys } from "@/lib/computers/queries"; +import { Route as BoundariesRoute } from "@/routes/_authed/admin/boundaries"; + +/** + * The Enter that confirms a character an input method is composing is not an Enter. + * + * Japanese, Chinese and Korean are typed through an input method (IME), and Enter is how the + * character being built is confirmed. That press still arrives as a keydown with `key === "Enter"`. + * Chromium marks it `isComposing`, and WebKit sends it after `compositionend` with the key code 229. + * The chat composer already skips it: `prompt-area` checks `isComposing` on every Enter it handles. + * Three text fields in the app acted on it instead, each with a text field's text still unconfirmed: + * a coworker's name saved in place, the new-coworker wizard moving on to its next step, and a + * boundary rule saved into the policy in force. + * + * THE HARNESS IS THIS REPOSITORY'S: `GlobalRegistrator` in `beforeAll`/`afterAll`, `cleanup` in + * `afterEach`, queries off `render()`'s own return, and a `QueryClient` with `retry: false`. Each + * screen is its real component, drawn inside a router of one route rather than through its own + * route singleton, so nothing here is left pointing another file's router at a decoy. + */ + +beforeAll(() => GlobalRegistrator.register()); +afterEach(cleanup); +afterAll(() => GlobalRegistrator.unregister()); + +const originalFetch = global.fetch; + +/** Every write a screen sent, as `METHOD path`, so "nothing was saved" is an assertion. */ +let writes: { request: string; body: unknown }[] = []; + +const PROFILE: AgentProfile = { + id: "expenses", + name: "Expenses", + title: "Finance Operations", + roleDescription: "Review receipts.", + avatarSeed: "expenses", + visibility: "private", + endpoint: null, + builtIn: true, + hasAuth: false, + hasCallbackToken: false, + hidden: false, + systemOwned: false, + canManage: true, + mine: true, +}; + +const POLICY = { mode: "enforce", deny: [], allow: [] }; + +beforeEach(() => { + writes = []; + global.fetch = Object.assign( + async ( + path: Parameters[0], + init?: Parameters[1], + ) => { + const method = init?.method ?? "GET"; + const body = + typeof init?.body === "string" ? JSON.parse(init.body) : undefined; + if (method !== "GET") writes.push({ request: `${method} ${path}`, body }); + const json = (value: unknown) => Response.json(value); + if (path === "/api/computers/policy") { + return json({ policy: method === "PUT" ? body : POLICY }); + } + if (path === "/api/agents/capabilities") { + return json({ capabilities: { builtInAvailable: true } }); + } + if (path === `/api/agents/${PROFILE.id}`) { + return json({ agent: method === "PATCH" ? PROFILE : PROFILE }); + } + return new Response(null, { status: 404 }); + }, + { preconnect: originalFetch.preconnect }, + ); +}); + +afterEach(() => { + global.fetch = originalFetch; +}); + +/** A screen drawn inside a router of one route, for the `Link` and `useNavigate` it holds. */ +function draw(screen: ReactNode, seed?: (client: QueryClient) => void) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + seed?.(queryClient); + const router = createRouter({ + history: createMemoryHistory({ initialEntries: ["/"] }), + routeTree: createRootRoute({ component: () => screen }), + }); + return render( + + + , + ); +} + +/** Both shapes the confirming Enter arrives in: Chromium's, then WebKit's. */ +async function confirmComposedCharacter(field: Element) { + await act(async () => { + fireEvent.keyDown(field, { key: "Enter", isComposing: true }); + fireEvent.keyDown(field, { key: "Enter", keyCode: 229 }); + }); +} + +/** A person replacing what a field holds, one key at a time. */ +async function type(field: Element, value: string) { + const user = userEvent.setup({ document: field.ownerDocument }); + await user.clear(field); + await user.type(field, value); +} + +/** Long enough for a write the keydown started to have reached `fetch`. */ +async function settle() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); +} + +test("a coworker's name is not saved by the Enter that confirms a composed character", async () => { + const view = draw( + {}} open />, + (client) => client.setQueryData(agentKeys.detail(PROFILE.id), PROFILE), + ); + + fireEvent.click(await view.findByRole("button", { name: "Edit name" })); + const field = view.getByDisplayValue(PROFILE.name); + await type(field, "経費"); + + await confirmComposedCharacter(field); + await settle(); + expect(writes).toEqual([]); + expect(view.getByDisplayValue("経費")).toBeTruthy(); + + // An ordinary Enter still saves, once. + await act(async () => { + fireEvent.keyDown(field, { key: "Enter", keyCode: 13 }); + }); + await settle(); + expect(writes.map((write) => write.request)).toEqual([ + `PATCH /api/agents/${PROFILE.id}`, + ]); + expect(writes[0]?.body).toMatchObject({ name: "経費" }); +}); + +test("the new-coworker wizard does not move on from the Enter that confirms a composed character", async () => { + const view = draw( + {}} onCreated={() => {}} open />, + ); + + const name = await view.findByLabelText("Name"); + await type(name, "経費"); + await type(view.getByLabelText("Title"), "Finance Operations"); + await type(view.getByLabelText("Role"), "Review receipts."); + + await confirmComposedCharacter(name); + await settle(); + expect(view.getByText("Step 1 of 3")).toBeTruthy(); + + // An ordinary Enter still means Continue. + await act(async () => { + fireEvent.keyDown(name, { key: "Enter", keyCode: 13 }); + }); + expect(await view.findByText("Step 2 of 3")).toBeTruthy(); +}); + +test("a boundary rule is not saved by the Enter that confirms a composed character", async () => { + const Boundaries = BoundariesRoute.options.component as ComponentType; + const view = draw(, (client) => + client.setQueryData(computerKeys.policy(), POLICY), + ); + + const field = await view.findByLabelText("A rule, written in CEL"); + const rule = 'contains(element.name, "送信")'; + await type(field, rule); + + await confirmComposedCharacter(field); + await settle(); + expect(writes).toEqual([]); + expect(view.getByDisplayValue(rule)).toBeTruthy(); + + // An ordinary Enter still adds the rule, once. + await act(async () => { + fireEvent.keyDown(field, { key: "Enter", keyCode: 13 }); + }); + await settle(); + expect(writes).toEqual([ + { + request: "PUT /api/computers/policy", + body: { ...POLICY, deny: [rule] }, + }, + ]); +});