From ebcc81ef9a7d09cfa499949206872a1bcc3c7c62 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Thu, 13 Aug 2026 04:41:48 -0700 Subject: [PATCH] [eric] browser: the caret is handed back once when the agent stops, not once per command (ENG-252) --- frontend/src/shared/browserCommandHandler.ts | 6 +- frontend/src/shared/caretHandback.test.ts | 86 ++++++++++++++++++++ frontend/src/shared/caretHandback.ts | 67 +++++++++++++++ 3 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 frontend/src/shared/caretHandback.test.ts create mode 100644 frontend/src/shared/caretHandback.ts diff --git a/frontend/src/shared/browserCommandHandler.ts b/frontend/src/shared/browserCommandHandler.ts index d874ee41..fb6caeab 100644 --- a/frontend/src/shared/browserCommandHandler.ts +++ b/frontend/src/shared/browserCommandHandler.ts @@ -1,4 +1,5 @@ import { getWebview, findWebviewByDomain, hasDomReady, markDomReady, isPendingLoad, wakePendingLoad, clearPendingLoad, type BrowserWebview } from './browserRegistry'; +import { scheduleCaretHandback } from './caretHandback'; import { shouldSelfHealClick } from './selfHealClick'; import { focusGuestForKeys } from './focusGuestForKeys'; import { FP_EXPR, clickEffect } from './clickEffect'; @@ -2248,7 +2249,10 @@ async function handleBrowserCommand(data: Record) { await runBrowserCommand(request_id, action, browser_id, tab_id, params); } finally { inflightCommands.delete(request_id); - restoreFocus(); + // Coalesced, not immediate: a run is many commands, and restoring after each one handed the + // caret back and took it again dozens of times, which is ENG-252's actual complaint. The user + // still gets it back, once, when the agent stops. + scheduleCaretHandback(restoreFocus); } } diff --git a/frontend/src/shared/caretHandback.test.ts b/frontend/src/shared/caretHandback.test.ts new file mode 100644 index 00000000..7394db00 --- /dev/null +++ b/frontend/src/shared/caretHandback.test.ts @@ -0,0 +1,86 @@ +// Run: npm test +// +// ENG-252. Measured from control flow: an agent run of N browser commands hands the caret back N +// times, because capture/restore brackets each command. Each one is correct; the user's caret is +// unusable. These tests pin the coalescing that fixes the frequency. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + scheduleCaretHandback, + cancelCaretHandback, + caretHandbackCount, + resetCaretHandbackCount, + caretHandbackPending, + HANDBACK_IDLE_MS, +} from './caretHandback.ts'; + +// A controllable clock: real timers would make this a sleep test. +function fakeTimers() { + let now = 0; + let seq = 0; + const jobs = new Map void }>(); + const set = ((fn: () => void, ms: number) => { seq += 1; jobs.set(seq, { at: now + ms, fn }); return seq; }) as unknown as typeof setTimeout; + const clear = ((id: number) => { jobs.delete(id); }) as unknown as typeof clearTimeout; + const advance = (ms: number) => { + now += ms; + for (const [id, j] of [...jobs]) if (j.at <= now) { jobs.delete(id); j.fn(); } + }; + return { set, clear, advance, pendingJobs: () => jobs.size }; +} + +test('a run of 20 commands hands the caret back ONCE, not 20 times', () => { + resetCaretHandbackCount(); + const t = fakeTimers(); + let restored = 0; + for (let i = 0; i < 20; i += 1) { + scheduleCaretHandback(() => { restored += 1; }, HANDBACK_IDLE_MS, t.set, t.clear); + t.advance(50); // commands arrive faster than the idle window + } + assert.equal(restored, 0, 'handed the caret back mid-run, which is the bug'); + t.advance(HANDBACK_IDLE_MS + 10); + assert.equal(restored, 1, `handed back ${restored} times for one run`); + assert.equal(caretHandbackCount(), 1); +}); + +test('a single command still hands the caret back', () => { + resetCaretHandbackCount(); + const t = fakeTimers(); + let restored = 0; + scheduleCaretHandback(() => { restored += 1; }, HANDBACK_IDLE_MS, t.set, t.clear); + t.advance(HANDBACK_IDLE_MS + 10); + assert.equal(restored, 1, 'the user must still get their caret back after one command'); +}); + +test('the LATEST restore wins, so a stale captured element is never used', () => { + resetCaretHandbackCount(); + const t = fakeTimers(); + const order: string[] = []; + scheduleCaretHandback(() => order.push('first'), HANDBACK_IDLE_MS, t.set, t.clear); + t.advance(50); + scheduleCaretHandback(() => order.push('second'), HANDBACK_IDLE_MS, t.set, t.clear); + t.advance(HANDBACK_IDLE_MS + 10); + assert.deepEqual(order, ['second'], `ran ${JSON.stringify(order)}`); +}); + +test('two runs separated by an idle gap hand back twice', () => { + resetCaretHandbackCount(); + const t = fakeTimers(); + let restored = 0; + const cmd = () => scheduleCaretHandback(() => { restored += 1; }, HANDBACK_IDLE_MS, t.set, t.clear); + cmd(); cmd(); t.advance(HANDBACK_IDLE_MS + 10); + cmd(); cmd(); t.advance(HANDBACK_IDLE_MS + 10); + assert.equal(restored, 2, 'each distinct run gets its own handback'); +}); + +test('cancel drops the pending handback and leaves no timer', () => { + resetCaretHandbackCount(); + const t = fakeTimers(); + let restored = 0; + scheduleCaretHandback(() => { restored += 1; }, HANDBACK_IDLE_MS, t.set, t.clear); + assert.equal(caretHandbackPending(), true); + cancelCaretHandback(t.clear); + assert.equal(caretHandbackPending(), false); + t.advance(HANDBACK_IDLE_MS + 10); + assert.equal(restored, 0, 'a cancelled handback must not fire'); + assert.equal(t.pendingJobs(), 0, 'a cancelled handback must not leak a timer'); +}); diff --git a/frontend/src/shared/caretHandback.ts b/frontend/src/shared/caretHandback.ts new file mode 100644 index 00000000..f4a26d0e --- /dev/null +++ b/frontend/src/shared/caretHandback.ts @@ -0,0 +1,67 @@ +// When should the user's caret be handed back? (ENG-252, 5th filing) +// +// The restore itself has existed since ENG-226 and works. Traced from control flow: the socket +// delivers ONE command per message, and `handleBrowserCommand` brackets each one with +// capture/restore, while every keyboard primitive inside takes host focus. So an agent run of N +// commands is literally steal, restore, steal, restore, N times. Every command behaves correctly +// and the caret is unusable, which is why four fixes "passed" and the user kept reopening it. +// +// The defect is the FREQUENCY, so the fix is fewer handbacks, not better ones: coalesce them, and +// give the caret back once the agent has actually stopped touching the browser. +// +// Deliberately NOT the more aggressive option (agent owns the caret for a whole run, user cannot +// type at all until it finishes). That is a product judgement about a resource measured to be +// singular, and it belongs to a human. This keeps the existing end state and only removes the +// thrash, so it cannot make the current behaviour worse. + +export const HANDBACK_IDLE_MS = 400; + +interface Pending { + restore: () => void; + timer: ReturnType; +} + +let p_pending: Pending | null = null; +let p_handbacks = 0; + +/** How many times the caret was actually handed back. The number ENG-252 is about. */ +export function caretHandbackCount(): number { + return p_handbacks; +} + +export function resetCaretHandbackCount(): void { + p_handbacks = 0; +} + +/** + * Schedule the caret handback, superseding any already-pending one. + * + * Called at the end of every browser command. A run of back-to-back commands therefore schedules N + * times and hands back ONCE, after the agent goes quiet for HANDBACK_IDLE_MS. The restore closure + * is the newest one, so it reflects the most recent capture rather than a stale element. + */ +export function scheduleCaretHandback( + restore: () => void, + idleMs: number = HANDBACK_IDLE_MS, + setTimer: typeof setTimeout = setTimeout, + clearTimer: typeof clearTimeout = clearTimeout, +): void { + if (p_pending) clearTimer(p_pending.timer); + const timer = setTimer(() => { + p_pending = null; + p_handbacks += 1; + restore(); + }, idleMs); + p_pending = { restore, timer }; +} + +/** Drop any pending handback without running it, for teardown. */ +export function cancelCaretHandback(clearTimer: typeof clearTimeout = clearTimeout): void { + if (!p_pending) return; + clearTimer(p_pending.timer); + p_pending = null; +} + +export function caretHandbackPending(): boolean { + return p_pending !== null; +}