Skip to content

Commit fe3edff

Browse files
pratikbuildspratikbuilds
committed
fix(agent): keep a stray Enter from deciding an approval
Approve, Approve always and Deny did not set a type, so inside a form they were submit buttons: Enter in a text field activated Approve, and aria-disabled did not stop it. They are now type="button". After a decision in a group, focus moved to the next waiting call's Approve button, so the repeat of a held Enter approved a call the person never chose. Focus now moves to that call's card, which is named by the question and the tool ("Needs your approval calendar__invite") and does nothing on Enter. The e2e scenario holds Enter across the decision to pin it.
1 parent 5fc8778 commit fe3edff

5 files changed

Lines changed: 75 additions & 17 deletions

File tree

‎e2e/tool-approval.spec.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,23 @@ test("in a group, keyboard decisions keep focus inside it", async ({
7474
() => (window as Window & { bodyFrames?: number }).bodyFrames,
7575
);
7676

77-
await page.keyboard.press("Enter");
77+
// A held Enter: the first press decides, the repeat lands on what has focus.
78+
await page.keyboard.down("Enter");
7879
await expect(page.getByRole("status", { name: "Events" })).toHaveText(
7980
"mail__send:approved:once",
8081
);
8182
await expect(waiting).toHaveCount(1);
8283
await expect(
83-
waiting.getByRole("button", { name: "Approve", exact: true }),
84+
page.getByRole("region", { name: "Needs your approval calendar__invite" }),
8485
).toBeFocused();
86+
await page.keyboard.down("Enter");
87+
await page.keyboard.up("Enter");
88+
await expect(waiting).toHaveCount(1);
89+
await expect(page.getByRole("status", { name: "Events" })).toHaveText(
90+
"mail__send:approved:once",
91+
);
8592

93+
await waiting.getByRole("button", { name: "Approve", exact: true }).focus();
8694
await page.keyboard.press("Enter");
8795
await expect(waiting).toHaveCount(0);
8896
const header = page.locator("[data-slot='tool-group-header']");

‎src/ui/tool-approval.test.tsx‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,13 @@ describe("ToolApproval", () => {
9494
);
9595
const section = mounted.container.querySelector("section");
9696
expect(section?.className).toContain("bg-attention-wash");
97-
expect(section?.getAttribute("aria-labelledby")).not.toBeNull();
97+
// Named by the question and the call, so two waiting cards differ.
98+
const name = section
99+
?.getAttribute("aria-labelledby")
100+
?.split(" ")
101+
.map((id) => document.getElementById(id)?.textContent)
102+
.join(" ");
103+
expect(name).toBe("Needs your approval mail__send");
98104
expect(mounted.container.textContent).toContain("Needs your approval");
99105
expect(mounted.container.textContent).toContain("mail__send");
100106
expect(mounted.container.textContent).toContain('"to": "team"');
@@ -121,6 +127,30 @@ describe("ToolApproval", () => {
121127
mounted.unmount();
122128
});
123129

130+
test("inside a form, the buttons decide without submitting it", () => {
131+
const decisions: ToolApprovalDecision[] = [];
132+
let submits = 0;
133+
const mounted = mount(
134+
<form
135+
onSubmit={(event) => {
136+
event.preventDefault();
137+
submits += 1;
138+
}}
139+
>
140+
<input aria-label="Note" />
141+
<ToolApproval part={tool()} onDecide={(d) => decisions.push(d)} />
142+
</form>,
143+
);
144+
for (const [name] of cases) {
145+
const element = button(mounted.container, name);
146+
expect(element.type).toBe("button");
147+
act(() => element.click());
148+
}
149+
expect(submits).toBe(0);
150+
expect(decisions.length).toBe(3);
151+
mounted.unmount();
152+
});
153+
124154
test("an error is announced", () => {
125155
const mounted = mount(
126156
<ToolApproval

‎src/ui/tool-approval.tsx‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ export function ToolApproval({
107107
}: ToolApprovalProps) {
108108
const labels = { ...LABELS, ...labelOverrides };
109109
const titleId = useId();
110+
const nameId = useId();
110111
const inputTitleId = useId();
111112
const outcomeId = useId();
112113
const awaiting = isAwaitingApproval(part);
@@ -165,7 +166,9 @@ export function ToolApproval({
165166
>
166167
{awaiting ? (
167168
<section
168-
aria-labelledby={titleId}
169+
data-slot="tool-approval-card"
170+
aria-labelledby={`${titleId} ${nameId}`}
171+
tabIndex={-1}
169172
className="flex flex-col gap-3 rounded-lg border border-primary-emphasis bg-attention-wash p-4 text-foreground"
170173
>
171174
<header className="flex min-w-0 flex-wrap items-center gap-2">
@@ -176,7 +179,10 @@ export function ToolApproval({
176179
<p id={titleId} className="font-semibold">
177180
{labels.title}
178181
</p>
179-
<code className="ms-auto min-w-0 truncate font-mono text-xs">
182+
<code
183+
id={nameId}
184+
className="ms-auto min-w-0 truncate font-mono text-xs"
185+
>
180186
{part.name}
181187
</code>
182188
</header>
@@ -205,6 +211,7 @@ export function ToolApproval({
205211
)}
206212
<div className="flex flex-wrap items-center gap-2">
207213
<Button
214+
type="button"
208215
size="sm"
209216
aria-disabled={disabled || undefined}
210217
onClick={decide({ outcome: "approved", scope: "once" })}
@@ -213,6 +220,7 @@ export function ToolApproval({
213220
{labels.approve}
214221
</Button>
215222
<Button
223+
type="button"
216224
size="sm"
217225
variant="outline"
218226
aria-disabled={disabled || undefined}
@@ -222,6 +230,7 @@ export function ToolApproval({
222230
{labels.approveAlways}
223231
</Button>
224232
<Button
233+
type="button"
225234
size="sm"
226235
variant="outline"
227236
aria-disabled={disabled || undefined}

‎src/ui/tool-group.tsx‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,10 @@ export type ToolGroupProps = Omit<
6262
* Tool calls that ran at the same time: one row saying so, the calls beneath.
6363
*
6464
* When a decision settles a call while focus was inside and nothing else has
65-
* taken it, focus moves to the next waiting call's first button, else to the
66-
* decided call's outcome, else to the group's row, instead of dropping to
67-
* the page.
65+
* taken it, focus moves to the next waiting call's card, else to the decided
66+
* call's outcome, else to the group's row, instead of dropping to the page.
67+
* The card, not its Approve button: a held Enter repeats onto whatever has
68+
* focus, and would decide a call the person never chose.
6869
*/
6970
export function ToolGroup({
7071
part,
@@ -235,7 +236,7 @@ export function ToolGroup({
235236

236237
const WAITING = "[data-slot='tool-approval'][data-status='pending']";
237238

238-
/** The first waiting call after the decided one, else the first waiting call. */
239+
/** The card of the first waiting call after the decided one, else of the first. */
239240
function nextWaiting(
240241
root: HTMLElement,
241242
decided: HTMLElement | null,
@@ -250,5 +251,9 @@ function nextWaiting(
250251
Node.DOCUMENT_POSITION_FOLLOWING) !==
251252
0,
252253
);
253-
return (after ?? cards[0])?.querySelector<HTMLElement>("button") ?? null;
254+
return (
255+
(after ?? cards[0])?.querySelector<HTMLElement>(
256+
"[data-slot='tool-approval-card']",
257+
) ?? null
258+
);
254259
}

‎src/ui/tool.test.tsx‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,14 @@ describe("ToolGroup", () => {
448448
act(() => approve.click());
449449
}
450450

451+
function cardOf(container: HTMLElement, name: string): Element {
452+
const card = [
453+
...container.querySelectorAll("[data-slot='tool-approval-card']"),
454+
].find((element) => element.textContent?.includes(name));
455+
if (card === undefined) throw new Error(`expected ${name}'s card`);
456+
return card;
457+
}
458+
451459
function resolvedLine(container: HTMLElement, name: string): Element {
452460
const line = [
453461
...container.querySelectorAll("[data-slot='tool-approval']"),
@@ -477,15 +485,15 @@ describe("ToolGroup", () => {
477485
mounted.unmount();
478486
});
479487

480-
test("with another call waiting, focus moves to that call's first button", () => {
488+
test("with another call waiting, focus moves to that call's card, not a button", () => {
481489
const mounted = mount(
482490
<Host initial={batch([waitingCall("a"), waitingCall("c")])} />,
483491
);
484492
decideByKeyboard(approveIn(mounted.container, "mail__a"));
485493
expect(mounted.container.textContent).toContain("Approved once");
486-
expect(document.activeElement).toBe(
487-
approveIn(mounted.container, "mail__c"),
488-
);
494+
// A held Enter repeats onto the focused element; on the card it decides nothing.
495+
expect(document.activeElement).toBe(cardOf(mounted.container, "mail__c"));
496+
expect(document.activeElement?.tagName).not.toBe("BUTTON");
489497
mounted.unmount();
490498
});
491499

@@ -500,9 +508,7 @@ describe("ToolGroup", () => {
500508
/>,
501509
);
502510
decideByKeyboard(approveIn(mounted.container, "mail__b"));
503-
expect(document.activeElement).toBe(
504-
approveIn(mounted.container, "mail__c"),
505-
);
511+
expect(document.activeElement).toBe(cardOf(mounted.container, "mail__c"));
506512
mounted.unmount();
507513
});
508514

0 commit comments

Comments
 (0)