From b11784e8e3cd01eacf60dbe32d9664e87f5cc7a2 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 5 Sep 2026 06:41:40 +0100 Subject: [PATCH] fix(maestro): mobile drawer access, focus preservation, stale rAF focus (#140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more fixes from acceptance review: - structured.ts's header omitted the .sessions-btn attach.ts's terminal header carries. The sidebar is off-canvas on mobile, so a structured session had no way to reach it — add the same button and requestToggleDrawer wiring. - ConnectionPool.reconcileViews always remounted a repaired connection with focus:false. If the mistyped pane was the one actually focused, disposing it dropped DOM focus while `focusedId` still named it. Pass `focus: this.focusedId === id` instead — `focusedId` only ever names a real focus (multiview.ts skips setFocus while previewing), so a preview or a merely-visible split neighbour still comes back unfocused. - onMount's deferred requestAnimationFrame focus had no way to tell it had been superseded by an unmount (or a remount elsewhere) before it fired, so a parked pane could steal focus a frame later. A token bumped on every mount/unmount lets the callback recognize it's stale and no-op. Regression tests: 7 new (structured.test.ts, connections.test.ts). 495 tests total. --- web/src/connections.test.ts | 54 ++++++++++++++++++++++- web/src/connections.ts | 11 ++++- web/src/structured.test.ts | 88 ++++++++++++++++++++++++++++++++++++- web/src/structured.ts | 19 +++++++- 4 files changed, 167 insertions(+), 5 deletions(-) diff --git a/web/src/connections.test.ts b/web/src/connections.test.ts index 20d03de..a9c10c6 100644 --- a/web/src/connections.test.ts +++ b/web/src/connections.test.ts @@ -9,6 +9,7 @@ import type { Session } from "./types.js"; // the pool reaches the focused connection with them. const sent: { id: string; data: string }[] = []; const disposed: string[] = []; +const mounts: { id: string; focus: boolean | undefined }[] = []; function fakeConn(kind: string, id: string) { const el = document.createElement("div"); el.dataset.kind = kind; @@ -17,7 +18,9 @@ function fakeConn(kind: string, id: string) { sessionId: id, kind, el, - onMount() {}, + onMount(opts?: { focus?: boolean }) { + mounts.push({ id, focus: opts?.focus }); + }, onUnmount() {}, focusTerminal() {}, sendInput(data: string) { @@ -39,6 +42,7 @@ afterEach(() => { vi.unstubAllGlobals(); sent.length = 0; disposed.length = 0; + mounts.length = 0; }); /** Minimal fixture for the `CatalogState` a `CatalogStore` broadcasts. */ @@ -256,4 +260,52 @@ describe("ConnectionPool: view resolution races", () => { expect(disposed).toEqual([]); }); + + it("re-focuses the fresh connection when the replaced pane was the focused session", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const pane = document.createElement("div"); + pool.mount(A, pane); + pool.setFocus(A); + mounts.length = 0; + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual(["attach:" + A]); + expect(mounts).toEqual([{ id: A, focus: true }]); + }); + + it("does not steal focus for a merely-visible pane that was never actually focused", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const paneA = document.createElement("div"); + const paneB = document.createElement("div"); + pool.mount(A, paneA); + pool.mount(B, paneB); + pool.setFocus(B); // B is the real keyboard focus; A is only visible + mounts.length = 0; + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual(["attach:" + A]); + expect(mounts).toEqual([{ id: A, focus: false }]); + }); + + it("does not focus a parked (unmounted) connection it repairs", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const pane = document.createElement("div"); + pool.mount(A, pane); + pool.setFocus(A); + pool.unmount(A); // parked — no longer visible, `focusedId` still names it + mounts.length = 0; + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual(["attach:" + A]); + expect(mounts).toEqual([]); // parked repair calls onUnmount, never onMount + }); }); diff --git a/web/src/connections.ts b/web/src/connections.ts index bcb6809..426248a 100644 --- a/web/src/connections.ts +++ b/web/src/connections.ts @@ -171,8 +171,15 @@ export class ConnectionPool { // siblings — append, never replaceChildren, or they'd all vanish. this.hidden.appendChild(fresh.el); } - if (wasMounted) fresh.onMount({ focus: false }); - else fresh.onUnmount(); + if (wasMounted) { + // `focusedId` only ever names a session the operator actually + // focused — never a merely-visible split neighbour or a preview + // (multiview.ts skips `setFocus` while previewing) — so this is + // exactly "was the keyboard actually in the pane being replaced." + fresh.onMount({ focus: this.focusedId === id }); + } else { + fresh.onUnmount(); + } } } diff --git a/web/src/structured.test.ts b/web/src/structured.test.ts index ba74be4..e5faf7d 100644 --- a/web/src/structured.test.ts +++ b/web/src/structured.test.ts @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("./api.js", async () => { const actual = await vi.importActual("./api.js"); @@ -12,6 +12,7 @@ vi.mock("./api.js", async () => { }); import { cancelStructuredTurn, resolveStructuredApproval, sendStructuredPrompt } from "./api.js"; +import { TOGGLE_DRAWER_EVENT } from "./drawer.js"; import { CatalogStore } from "./events.js"; import { createStructuredConnection, type StructuredSocketFactory } from "./structured.js"; import type { ReducedState, Session, StructuredEvent, TranscriptRow } from "./types.js"; @@ -349,6 +350,22 @@ describe("structured connection: pane lifecycle", () => { conn.dispose(); }); + it("carries the same off-canvas sessions button as the terminal attach header", () => { + const conn = mount({ sockets: [new FakeSocket()] }); + const btn = conn.el.querySelector(".sessions-btn") as HTMLButtonElement; + expect(btn).not.toBeNull(); + expect(btn.getAttribute("aria-label")).toBe("Show sessions"); + expect(btn.type).toBe("button"); + + let toggled = 0; + const onToggle = () => toggled++; + document.addEventListener(TOGGLE_DRAWER_EVENT, onToggle); + btn.click(); + document.removeEventListener(TOGGLE_DRAWER_EVENT, onToggle); + expect(toggled).toBe(1); + conn.dispose(); + }); + it("focusTerminal focuses the composer only when it is enabled", () => { const ws = new FakeSocket(); const conn = mount({ sockets: [ws] }); @@ -400,3 +417,72 @@ describe("structured connection: pane lifecycle", () => { conn.dispose(); }); }); + +describe("structured connection: deferred mount focus", () => { + let frames: FrameRequestCallback[]; + + beforeEach(() => { + vi.clearAllMocks(); + frames = []; + vi.stubGlobal("requestAnimationFrame", (cb: FrameRequestCallback) => { + frames.push(cb); + return frames.length; + }); + }); + + afterEach(() => vi.unstubAllGlobals()); + + function runFrames(): void { + const pending = frames; + frames = []; + for (const cb of pending) cb(0); + } + + it("does not focus the composer if the pane was parked before its deferred frame ran", () => { + const ws = new FakeSocket(); + const conn = mount({ sockets: [ws] }); + ws.open(); + ws.emit({ type: "transcript_snapshot", snapshot: { cursor: 0, rows: [] } }); + ws.emit({ type: "reduced_state", state: idleState }); + + conn.onMount({ focus: true }); + conn.onUnmount(); // parked again before the frame below runs + runFrames(); + + const input = conn.el.querySelector(".structured-input") as HTMLTextAreaElement; + expect(document.activeElement).not.toBe(input); + conn.dispose(); + }); + + it("does not focus the composer if it was re-mounted into a different pane before the stale frame ran", () => { + const ws = new FakeSocket(); + const conn = mount({ sockets: [ws] }); + ws.open(); + ws.emit({ type: "transcript_snapshot", snapshot: { cursor: 0, rows: [] } }); + ws.emit({ type: "reduced_state", state: idleState }); + + conn.onMount({ focus: true }); // stale frame #1 + conn.onUnmount(); + conn.onMount({ focus: false }); // the pane that actually won focus, without it + runFrames(); // both frames fire; only the latest should have any say + + const input = conn.el.querySelector(".structured-input") as HTMLTextAreaElement; + expect(document.activeElement).not.toBe(input); + conn.dispose(); + }); + + it("still focuses normally when nothing raced the frame", () => { + const ws = new FakeSocket(); + const conn = mount({ sockets: [ws] }); + ws.open(); + ws.emit({ type: "transcript_snapshot", snapshot: { cursor: 0, rows: [] } }); + ws.emit({ type: "reduced_state", state: idleState }); + + conn.onMount({ focus: true }); + runFrames(); + + const input = conn.el.querySelector(".structured-input") as HTMLTextAreaElement; + expect(document.activeElement).toBe(input); + conn.dispose(); + }); +}); diff --git a/web/src/structured.ts b/web/src/structured.ts index eee7f7b..994c176 100644 --- a/web/src/structured.ts +++ b/web/src/structured.ts @@ -12,6 +12,7 @@ import { sendStructuredPrompt, structuredWsUrl, } from "./api.js"; +import { requestToggleDrawer } from "./drawer.js"; import type { CatalogState, CatalogStore } from "./events.js"; import { paneTitleAttr, paneTitleHtml, type AttachInfo } from "./paneTitle.js"; import type { PaneConnection } from "./pane.js"; @@ -57,6 +58,7 @@ export function createStructuredConnection( el.className = "attach structured"; el.innerHTML = `
+ ${paneTitleHtml(sessionId, info)} connecting… @@ -87,6 +89,12 @@ export function createStructuredConnection( const cancelBtn = el.querySelector(".structured-cancel") as HTMLButtonElement; disconnectBtn.addEventListener("click", () => opts.onRequestClose?.(sessionId)); + // Mobile-only (CSS hides it on desktop, where the sidebar is always in + // view): the session list is an off-canvas drawer and this opens it — + // same control attach.ts's terminal header carries. + (el.querySelector(".sessions-btn") as HTMLElement).addEventListener("click", () => + requestToggleDrawer(), + ); const setConnState = (text: string, kind: "connecting" | "open" | "closed" | "error") => { stateEl.textContent = text; @@ -346,17 +354,26 @@ export function createStructuredConnection( } connect(); + // A pane can be parked again before its deferred focus frame fires (a + // preview stepped past it, a split closed it) — the frame must not focus a + // hidden textarea out from under whatever the operator moved to since. + // `mountToken` is bumped on every mount/unmount so a stale frame can tell + // it is no longer the most recent one. + let mountToken = 0; + return { sessionId, el, onMount(mountOpts?: { focus?: boolean }) { const focusOnMount = mountOpts?.focus !== false; + const token = ++mountToken; requestAnimationFrame(() => { + if (token !== mountToken) return; // parked/remounted since this was scheduled if (focusOnMount && !input.disabled) input.focus(); }); }, onUnmount() { - // Nothing to release: the composer needs no GPU context or size fit. + mountToken++; }, focusTerminal() { if (!input.disabled) input.focus();