diff --git a/desktop/src/components/chat/AssistantOutputTargetCard.test.tsx b/desktop/src/components/chat/AssistantOutputTargetCard.test.tsx index 2fd7b836..40e69906 100644 --- a/desktop/src/components/chat/AssistantOutputTargetCard.test.tsx +++ b/desktop/src/components/chat/AssistantOutputTargetCard.test.tsx @@ -107,4 +107,17 @@ describe('AssistantOutputTargetCard', () => { expect(await screen.findByText('openWith.inAppBrowser')).toBeInTheDocument() expect(screen.getByText('openWith.systemBrowser')).toBeInTheDocument() }) + + it('re-clicking the same open-with trigger TOGGLES the menu closed', async () => { + render() + const trigger = screen.getByLabelText('openWith.title') + + // 1st click → opens + fireEvent.click(trigger) + expect(await screen.findByText('openWith.inAppBrowser')).toBeInTheDocument() + + // 2nd click on the SAME trigger → closes (toggle) + fireEvent.click(trigger) + expect(screen.queryByText('openWith.inAppBrowser')).not.toBeInTheDocument() + }) }) diff --git a/desktop/src/components/chat/AssistantOutputTargetCard.tsx b/desktop/src/components/chat/AssistantOutputTargetCard.tsx index a627c232..8ddca096 100644 --- a/desktop/src/components/chat/AssistantOutputTargetCard.tsx +++ b/desktop/src/components/chat/AssistantOutputTargetCard.tsx @@ -21,7 +21,7 @@ type Props = { export function AssistantOutputTargetCard({ target, sessionId, workDir }: Props) { const t = useTranslation() - const [openWith, setOpenWith] = useState<{ items: OpenWithItem[]; anchor: DOMRect } | null>(null) + const [openWith, setOpenWith] = useState<{ items: OpenWithItem[]; anchor: DOMRect; triggerEl: HTMLElement } | null>(null) const isLocalhost = target.kind === 'localhost-url' const typeInfo = describeFileType(target.normalizedPath ?? target.href) @@ -60,7 +60,15 @@ export function AssistantOutputTargetCard({ target, sessionId, workDir }: Props) const handleOpenWith = useCallback((event: ReactMouseEvent) => { event.stopPropagation() - const rect = event.currentTarget.getBoundingClientRect() + // Toggle: a second click on the same trigger closes the menu. OpenWithMenu's + // outside-mousedown ignores the trigger, so the trigger's own click is the + // only path that can close it on re-click. + if (openWith) { + setOpenWith(null) + return + } + const triggerEl = event.currentTarget + const rect = triggerEl.getBoundingClientRect() void (async () => { await useOpenTargetStore.getState().ensureTargets() const targets = useOpenTargetStore.getState().targets @@ -77,9 +85,9 @@ export function AssistantOutputTargetCard({ target, sessionId, workDir }: Props) openTarget: (id, abs) => { void useOpenTargetStore.getState().openTarget(id, abs) }, t: (k, v) => t(k as TranslationKey, v), }) - setOpenWith({ items, anchor: rect }) + setOpenWith({ items, anchor: rect, triggerEl }) })() - }, [sessionId, t, target.href, workDir]) + }, [openWith, sessionId, t, target.href, workDir]) return (
@@ -137,7 +145,7 @@ export function AssistantOutputTargetCard({ target, sessionId, workDir }: Props) - {openWith && setOpenWith(null)} />} + {openWith && setOpenWith(null)} />}
) } diff --git a/desktop/src/components/chat/CurrentTurnChangeCard.tsx b/desktop/src/components/chat/CurrentTurnChangeCard.tsx index 5f4722db..27d29460 100644 --- a/desktop/src/components/chat/CurrentTurnChangeCard.tsx +++ b/desktop/src/components/chat/CurrentTurnChangeCard.tsx @@ -38,7 +38,7 @@ export function CurrentTurnChangeCard({ onUndo, }: CurrentTurnChangeCardProps) { const t = useTranslation() - const [openWith, setOpenWith] = useState<{ items: OpenWithItem[]; anchor: DOMRect } | null>(null) + const [openWith, setOpenWith] = useState<{ items: OpenWithItem[]; anchor: DOMRect; triggerEl: HTMLElement } | null>(null) const [showAllFiles, setShowAllFiles] = useState(false) const files = useMemo( @@ -63,7 +63,15 @@ export function CurrentTurnChangeCard({ const handleOpenWith = useCallback((event: ReactMouseEvent, fileEntry: ChangedFileEntry) => { event.stopPropagation() - const rect = event.currentTarget.getBoundingClientRect() + // Toggle: if the menu is already open, a second click on the trigger closes it + // (the OpenWithMenu's outside-mousedown handler excludes the trigger, so its + // own click is the only thing that can close it on re-click). + if (openWith) { + setOpenWith(null) + return + } + const triggerEl = event.currentTarget + const rect = triggerEl.getBoundingClientRect() void (async () => { await useOpenTargetStore.getState().ensureTargets() const targets = useOpenTargetStore.getState().targets @@ -78,9 +86,9 @@ export function CurrentTurnChangeCard({ openTarget: (id, abs) => { void useOpenTargetStore.getState().openTarget(id, abs) }, t: (k, v) => t(k as TranslationKey, v), }) - setOpenWith({ items, anchor: rect }) + setOpenWith({ items, anchor: rect, triggerEl }) })() - }, [sessionId, t]) + }, [openWith, sessionId, t]) const cardLabel = isLatest ? t('chat.turnChangesLatestCardLabel') @@ -193,7 +201,7 @@ export function CurrentTurnChangeCard({ )} - {openWith && setOpenWith(null)} />} + {openWith && setOpenWith(null)} />} ) } diff --git a/desktop/src/components/common/OpenWithMenu.test.tsx b/desktop/src/components/common/OpenWithMenu.test.tsx index 27cb52d1..a2481163 100644 --- a/desktop/src/components/common/OpenWithMenu.test.tsx +++ b/desktop/src/components/common/OpenWithMenu.test.tsx @@ -69,6 +69,65 @@ describe('OpenWithMenu', () => { expect(onClose).toHaveBeenCalledTimes(1) }) + describe('triggerEl exclusion (re-click-trigger toggle support)', () => { + it('does NOT call onClose when mousedown lands inside triggerEl', () => { + // Set up a trigger element + a child within it in the document. + const trigger = document.createElement('button') + const triggerChild = document.createElement('span') + trigger.appendChild(triggerChild) + document.body.appendChild(trigger) + const onClose = vi.fn() + render() + + // mousedown on the trigger itself + fireEvent.mouseDown(trigger) + expect(onClose).not.toHaveBeenCalled() + + // mousedown on a descendant of the trigger + fireEvent.mouseDown(triggerChild) + expect(onClose).not.toHaveBeenCalled() + + document.body.removeChild(trigger) + }) + + it('STILL calls onClose for true outside clicks (not menu, not trigger)', () => { + const trigger = document.createElement('button') + document.body.appendChild(trigger) + const outside = document.createElement('div') + document.body.appendChild(outside) + const onClose = vi.fn() + render() + + fireEvent.mouseDown(outside) + expect(onClose).toHaveBeenCalledTimes(1) + + document.body.removeChild(trigger) + document.body.removeChild(outside) + }) + + it('Escape still closes when triggerEl is provided', () => { + const trigger = document.createElement('button') + document.body.appendChild(trigger) + const onClose = vi.fn() + render() + fireEvent.keyDown(document, { key: 'Escape' }) + expect(onClose).toHaveBeenCalledTimes(1) + document.body.removeChild(trigger) + }) + + it('item click still closes the menu when triggerEl is provided', () => { + const trigger = document.createElement('button') + document.body.appendChild(trigger) + const onClose = vi.fn() + const onSelect1 = vi.fn() + render() + fireEvent.click(screen.getByText('In-app browser')) + expect(onSelect1).toHaveBeenCalledTimes(1) + expect(onClose).toHaveBeenCalledTimes(1) + document.body.removeChild(trigger) + }) + }) + it('flips above the anchor when it would overflow the viewport bottom', () => { // The trigger often sits right above the composer; the menu must not render off-screen below it. Object.defineProperty(window, 'innerHeight', { value: 300, configurable: true }) diff --git a/desktop/src/components/common/OpenWithMenu.tsx b/desktop/src/components/common/OpenWithMenu.tsx index 11a2e9cb..ae1f75de 100644 --- a/desktop/src/components/common/OpenWithMenu.tsx +++ b/desktop/src/components/common/OpenWithMenu.tsx @@ -8,6 +8,10 @@ type Props = { items: OpenWithItem[] anchor: { top: number; bottom: number; left: number; right: number } onClose: () => void + // Optional trigger element to exclude from outside-close detection. When the + // user clicks the same trigger that opened this menu, the trigger's own + // click handler is responsible for toggling — don't double-close here. + triggerEl?: HTMLElement | null } function ItemIcon({ item }: { item: OpenWithItem }) { @@ -19,7 +23,7 @@ function ItemIcon({ item }: { item: OpenWithItem }) { const MARGIN = 8 -export function OpenWithMenu({ items, anchor, onClose }: Props) { +export function OpenWithMenu({ items, anchor, onClose, triggerEl }: Props) { const ref = useRef(null) // Initial guess; corrected (before paint) by the layout effect once we can measure the menu. const [pos, setPos] = useState<{ top: number; left: number }>(() => ({ @@ -47,12 +51,20 @@ export function OpenWithMenu({ items, anchor, onClose }: Props) { }, [anchor]) useEffect(() => { - const onDown = (e: MouseEvent) => { if (!ref.current?.contains(e.target as Node)) onClose() } + const onDown = (e: MouseEvent) => { + const target = e.target as Node | null + if (!target) return + // Ignore clicks inside the menu itself, AND clicks inside the trigger + // element that opened it — the trigger's own onClick handler will toggle. + if (ref.current?.contains(target)) return + if (triggerEl && triggerEl.contains(target)) return + onClose() + } const onKey = (e: KeyboardEvent) => { if (e.key === 'Escape') onClose() } document.addEventListener('mousedown', onDown) document.addEventListener('keydown', onKey) return () => { document.removeEventListener('mousedown', onDown); document.removeEventListener('keydown', onKey) } - }, [onClose]) + }, [onClose, triggerEl]) return createPortal(