From bb0e9ee95a98ed4ec2df8b8abb1887f003a16753 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 19 Aug 2026 22:43:29 -0500 Subject: [PATCH] fix(desktop): confirm dialogs take focus so Enter confirms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delete-session dialog opted out of Radix's autofocus, which left focus on the sidebar row that opened it — Enter re-activated the row instead of confirming, and ConfirmDialog's Enter handler never saw the key. ConfirmDialog now focuses its own Confirm button on open. The existing Enter test fired the key at the dialog node, so it passed over the bug; it now fires at whatever actually holds focus. --- .../sidebar/session-actions-menu.test.tsx | 11 +++++-- .../app/chat/sidebar/session-actions-menu.tsx | 10 +----- .../src/components/ui/confirm-dialog.tsx | 33 ++++++++++++------- apps/desktop/src/components/ui/dialog.tsx | 4 ++- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx b/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx index 60de5f5ca5..21e276bb04 100644 --- a/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx +++ b/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx @@ -247,12 +247,19 @@ describe('SessionActionsMenu', () => { expect(await screen.queryByRole('dialog')).toBeNull() expect(onDelete).not.toHaveBeenCalled() - // Re-open and confirm with Enter: the delete call fires. + // Re-open and confirm with Enter at wherever focus actually is. Firing on + // the dialog node would pass even when the menu leaves focus on the row + // trigger — where Enter re-activates the row instead of confirming. fireEvent.pointerDown(trigger, { button: 0, pointerType: 'mouse' }) fireEvent.pointerUp(trigger, { button: 0, pointerType: 'mouse' }) fireEvent.click(trigger) fireEvent.click(await screen.findByRole('menuitem', { name: /delete/i })) - fireEvent.keyDown(await screen.findByRole('dialog'), { key: 'Enter' }) + + const reopened = await screen.findByRole('dialog') + // eslint-disable-next-line no-restricted-globals -- asserting real focus requires the live document + await waitFor(() => expect(reopened.contains(document.activeElement)).toBe(true)) + // eslint-disable-next-line no-restricted-globals -- asserting real focus requires the live document + fireEvent.keyDown(document.activeElement!, { key: 'Enter' }) expect(await screen.findByText('Session deleted')).toBeTruthy() expect(onDelete).toHaveBeenCalledTimes(1) diff --git a/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx b/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx index 2550800e72..70962073db 100644 --- a/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx +++ b/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx @@ -22,14 +22,7 @@ import { Codicon } from '@/components/ui/codicon' import { ColorSwatches } from '@/components/ui/color-swatches' import { ConfirmDialog } from '@/components/ui/confirm-dialog' import { CopyButton } from '@/components/ui/copy-button' -import { - Dialog, - DialogContent, - DialogFooter, - DialogHeader, - DialogTitle, - preventCloseButtonAutoFocus -} from '@/components/ui/dialog' +import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog' import { Input } from '@/components/ui/input' import { renameSession } from '@/hermes' import { useI18n } from '@/i18n' @@ -580,7 +573,6 @@ function DeleteSessionDialog({ open, onOpenChange, onConfirm, sessionTitle }: De doneLabel={r.deleted} onClose={() => onOpenChange(false)} onConfirm={onConfirm} - onOpenAutoFocus={preventCloseButtonAutoFocus} open={open} title={r.deleteTitle} /> diff --git a/apps/desktop/src/components/ui/confirm-dialog.tsx b/apps/desktop/src/components/ui/confirm-dialog.tsx index d4d5fc327d..3d60069c4b 100644 --- a/apps/desktop/src/components/ui/confirm-dialog.tsx +++ b/apps/desktop/src/components/ui/confirm-dialog.tsx @@ -1,5 +1,5 @@ import type { ReactNode } from 'react' -import { useEffect, useState } from 'react' +import { useEffect, useRef, useState } from 'react' import { ActionStatus } from '@/components/ui/action-status' import { Button } from '@/components/ui/button' @@ -28,15 +28,12 @@ interface ConfirmDialogProps { destructive?: boolean /** Close as soon as onConfirm resolves — for optimistic actions that finish in the background. */ dismissOnConfirm?: boolean - /** Focus control for dialogs with no input. Pass `preventCloseButtonAutoFocus` - * so opening doesn't land focus on the close/cancel button (which would pop - * its tooltip with no pointer near it). */ - onOpenAutoFocus?: (event: Event) => void } -// Shared confirmation dialog: Enter confirms (from anywhere in the dialog), -// Esc/Cancel/backdrop dismiss. Owns the pending → done → close beat and inline -// error, so callers pass only an async onConfirm that does the work. +// Shared confirmation dialog: opens focused on Confirm, Enter confirms (from +// anywhere in the dialog), Esc/Cancel/backdrop dismiss. Owns the pending → done +// → close beat and inline error, so callers pass only an async onConfirm that +// does the work. export function ConfirmDialog({ open, onClose, @@ -48,10 +45,10 @@ export function ConfirmDialog({ doneLabel, cancelLabel, destructive = false, - dismissOnConfirm = false, - onOpenAutoFocus + dismissOnConfirm = false }: ConfirmDialogProps) { const { t } = useI18n() + const confirmRef = useRef(null) const [status, setStatus] = useState<'done' | 'idle' | 'saving'>('idle') const [error, setError] = useState(null) const busy = status === 'saving' || status === 'done' @@ -109,7 +106,14 @@ export function ConfirmDialog({ void run() } }} - onOpenAutoFocus={onOpenAutoFocus} + onOpenAutoFocus={event => { + // Focus must land inside the dialog or the handler above never sees + // the key: it stays on whatever opened the dialog (a menu item, a + // sidebar row) and Enter re-triggers that instead. Radix's default + // would take the X — confirm is the button Enter maps to. + event.preventDefault() + confirmRef.current?.focus() + }} > {title} @@ -127,7 +131,12 @@ export function ConfirmDialog({ -