From 833dd90bcdbed1f078fd9a552d0594bd1d942007 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:04:43 -0700 Subject: [PATCH] fix(codex): stop the restart card from parking focus on Restart (#10870) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a Codex restart notice appears, the card moved keyboard focus to its default "Restart" button. The card is mounted per WORKTREE — Terminal.tsx renders it as a sibling of TabGroupSplitLayout — so `root.parentElement` is the whole worktree surface and shouldFocusMobileDriverAction's xterm branch matches ANY terminal in it. A notice raised for pane A therefore yanked focus out of a different, perfectly healthy pane B that the user was typing in. The focused control was destructive: Restart calls queueCodexPaneRestarts over every stale pty in the worktree, so the next Space or Enter of the user's prose silently restarted those Codex sessions and lost their state — the same class of loss #10757 is about. Verified empirically: focus moves from TEXTAREA.xterm-helper-textarea to BUTTON 'Restart' 3/3, and Space/Enter changed the pane's ptyId, i.e. genuinely killed and relaunched the session. Focus the role="dialog" root instead, with tabIndex={-1} so it is programmatically focusable but out of the Tab order. The card is still announced (role + aria-live + labelledby/describedby) and Tab from it still reaches Restart and Keep, so keyboard and AT discoverability are preserved; the only thing lost is the ability to fire a session-killing action by accident. shouldFocusMobileDriverAction is untouched, so MobileDriverOverlay is unchanged. Both new tests are mutation-proved: restoring restartRef.current?.focus() fails exactly those two and leaves the other 11 green. Known residual, deliberately not fixed: focus still leaves a healthy pane for the dialog. That interrupts typing but no longer destroys anything; narrowing to the stale pane needs a DOM-to-ptyId mapping the card does not have today. Not verified: no live app run. Refs #10863 --- .../src/components/CodexRestartChip.tsx | 22 +++- .../src/components/codex-restart-chip.test.ts | 111 +++++++++++++++--- 2 files changed, 109 insertions(+), 24 deletions(-) diff --git a/src/renderer/src/components/CodexRestartChip.tsx b/src/renderer/src/components/CodexRestartChip.tsx index a7537343494..92f19f41144 100644 --- a/src/renderer/src/components/CodexRestartChip.tsx +++ b/src/renderer/src/components/CodexRestartChip.tsx @@ -151,10 +151,17 @@ function LoudRestartOverlay({ const titleId = useId() const bodyId = useId() const rootRef = useRef(null) - const restartRef = useRef(null) - // Why: focus Restart only when the user isn't typing elsewhere; unconditional - // autoFocus would steal keys from an active composer or terminal input. + // Why: move focus to the card only when the user isn't typing elsewhere; + // unconditional autoFocus would steal keys from an active composer. + // + // The target is the dialog itself, never Restart. This card is mounted per + // WORKTREE (a sibling of the split layout), so its focus scope is every + // terminal in the worktree — a notice for one pane lands while the user may + // be typing in a different, perfectly healthy pane. With Restart focused, the + // next Space/Enter of their prose queued a restart of every stale pane here + // and destroyed those sessions. Focus must not land on a destructive action + // the user never aimed at. See #10863. useEffect(() => { if (!isVisible) { return @@ -165,7 +172,7 @@ function LoudRestartOverlay({ } const paneScope = root.parentElement if (shouldFocusMobileDriverAction(document.activeElement, document.body, paneScope)) { - restartRef.current?.focus() + root.focus() } }, [isVisible, noticeKey]) @@ -173,10 +180,13 @@ function LoudRestartOverlay({
@@ -207,7 +217,7 @@ function LoudRestartOverlay({ - diff --git a/src/renderer/src/components/codex-restart-chip.test.ts b/src/renderer/src/components/codex-restart-chip.test.ts index 5f3a8f5b14f..a23ed4d78e7 100644 --- a/src/renderer/src/components/codex-restart-chip.test.ts +++ b/src/renderer/src/components/codex-restart-chip.test.ts @@ -234,15 +234,13 @@ describe('CodexRestartChip helpers', () => { }, ptyIdsByTabId: { 'tab-1': ['pty-1'] } }) - useAppStore - .getState() - .markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) + useAppStore.getState().markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) await act(async () => { root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) }) @@ -292,15 +290,13 @@ describe('CodexRestartChip helpers', () => { }, ptyIdsByTabId: { 'tab-1': ['pty-1'] } }) - useAppStore - .getState() - .markCodexRestartNotices([ - { - ptyId: 'pty-1', - previousAccountLabel: 'old@example.com', - nextAccountLabel: 'new@example.com' - } - ]) + useAppStore.getState().markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) await act(async () => { root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) }) @@ -369,3 +365,82 @@ describe('CodexRestartChip helpers', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']?.restartRequested).toBe(true) }) }) + +describe('CodexRestartChip focus target', () => { + function renderWithFocusedSiblingPane(): HTMLTextAreaElement { + // Mirrors Terminal.tsx, which mounts the chip as a SIBLING of the split + // layout — so the chip's parentElement is the whole worktree surface and + // its focus scope covers every pane in it, not just the stale one. + const splitLayout = document.createElement('div') + const healthyPaneInput = document.createElement('textarea') + healthyPaneInput.className = 'xterm-helper-textarea' + splitLayout.appendChild(healthyPaneInput) + container.appendChild(splitLayout) + healthyPaneInput.focus() + + useAppStore.setState({ + tabsByWorktree: { + 'worktree-1': [ + { + id: 'tab-1', + worktreeId: 'worktree-1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: null + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] } + }) + useAppStore + .getState() + .markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) + return healthyPaneInput + } + + it('never parks focus on Restart, so a stray keystroke cannot kill the session', async () => { + renderWithFocusedSiblingPane() + + await act(async () => { + root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) + }) + + // Regression (#10863): focus used to land on Restart. The card is worktree + // scoped, so it fires while the user is typing in a DIFFERENT, healthy pane + // — and the next Space/Enter of their prose restarted every stale pane here. + const restartButton = Array.from(container.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Restart' + ) + expect(restartButton).toBeDefined() + expect(document.activeElement).not.toBe(restartButton) + expect((document.activeElement as HTMLElement | null)?.getAttribute('role')).toBe('dialog') + + // The keystroke that used to destroy the session now does nothing. + await act(async () => { + ;(document.activeElement as HTMLElement | null)?.click() + }) + expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({}) + }) + + it('still moves focus off the terminal so the card is reachable by keyboard', async () => { + const healthyPaneInput = renderWithFocusedSiblingPane() + + await act(async () => { + root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) + }) + + // Why: the fix must not turn into "never take focus" — assistive tech and + // keyboard users still need to land on the dialog and Tab to its actions. + expect(document.activeElement).not.toBe(healthyPaneInput) + expect(document.activeElement).toBe(container.querySelector('[role="dialog"]')) + }) +})