mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(codex): stop the restart card from parking focus on Restart (#10870)
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
This commit is contained in:
@@ -151,10 +151,17 @@ function LoudRestartOverlay({
|
||||
const titleId = useId()
|
||||
const bodyId = useId()
|
||||
const rootRef = useRef<HTMLDivElement>(null)
|
||||
const restartRef = useRef<HTMLButtonElement>(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({
|
||||
<div
|
||||
ref={rootRef}
|
||||
role="dialog"
|
||||
// Why: programmatically focusable so the card can take focus itself, but
|
||||
// out of the Tab order — Tab from here still reaches Restart/Keep.
|
||||
tabIndex={-1}
|
||||
aria-live="assertive"
|
||||
aria-labelledby={titleId}
|
||||
aria-describedby={bodyId}
|
||||
className="pointer-events-none absolute inset-0 z-50 flex items-center justify-center p-6"
|
||||
className="pointer-events-none absolute inset-0 z-50 flex items-center justify-center p-6 outline-none"
|
||||
>
|
||||
<div className="pointer-events-auto flex w-full max-w-[30rem] flex-col gap-3 rounded-lg border border-border bg-card p-6 pb-5 text-card-foreground shadow-xs">
|
||||
<div className="flex items-start gap-3">
|
||||
@@ -207,7 +217,7 @@ function LoudRestartOverlay({
|
||||
<Button type="button" variant="outline" size="sm" onClick={onDismiss}>
|
||||
{translate('auto.components.CodexRestartChip.6133594b12', 'Keep old account')}
|
||||
</Button>
|
||||
<Button ref={restartRef} type="button" variant="default" size="sm" onClick={onRestart}>
|
||||
<Button type="button" variant="default" size="sm" onClick={onRestart}>
|
||||
<RefreshCw />
|
||||
{translate('auto.components.CodexRestartChip.c72a5fb234', 'Restart')}
|
||||
</Button>
|
||||
|
||||
@@ -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"]'))
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user