diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 4cf52f5697e..ae392533618 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -104,6 +104,7 @@ type ChatEditorTestProps = { type AddWorkspaceDialogTestProps = { onClose: () => void; onAdd: (cwd: string, persist: boolean, displayName?: string) => Promise; + onSuggest?: (prefix: string) => Promise; onPick?: () => Promise; displayNameEnabled?: boolean; persistenceSupported?: boolean; @@ -7985,7 +7986,7 @@ describe('App session callbacks', () => { }, ], } as typeof mockWorkspace.capabilities; - const { container } = renderApp(); + const { container, rerender } = renderApp(); await flush(); act(() => { @@ -7998,6 +7999,16 @@ describe('App session callbacks', () => { displayNameEnabled: true, persistenceSupported: true, }); + // The dialog's fetch effect re-runs on every onSuggest identity + // change, so App must pass the memoized workspace action itself, + // not a per-render closure; pin the reference across a re-render. + expect(testState.latestAddWorkspaceDialogProps?.onSuggest).toBe( + mockWorkspaceActions.suggestWorkspacePaths, + ); + rerender(); + expect(testState.latestAddWorkspaceDialogProps?.onSuggest).toBe( + mockWorkspaceActions.suggestWorkspacePaths, + ); mockWorkspaceActions.pickWorkspaceDirectory.mockResolvedValue({ kind: 'workspace-directory-picker', selected: true, diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index b12cd27e7ea..9116c023a92 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -202,7 +202,7 @@ import { copyFromLastAssistantMessage, COPY_MESSAGES, } from './utils/copyCommand'; -import { isEditableTarget } from './utils/dom'; +import { getShadowAwareActiveElement, isEditableTarget } from './utils/dom'; import { invokeSlashCommandHandler, SLASH_COMMAND_PATTERN, @@ -3861,8 +3861,8 @@ export function App({ } // document.activeElement retargets to the shadow host in shadow-DOM // portal mode; read the focused node from the surface's own root. - const surfaceRoot = surface.getRootNode() as Document | ShadowRoot; - if (!surface.contains(surfaceRoot.activeElement)) surface.focus(); + const surfaceActive = getShadowAwareActiveElement(surface); + if (!surface.contains(surfaceActive)) surface.focus(); // Keydowns inside the sandboxed HTML preview iframe never reach the // surface's Tab-wrap handler or the window Escape handler, and a Tab // past the preview's last focusable lands focus natively outside the @@ -3922,9 +3922,7 @@ export function App({ return; } const [first, last] = getFullscreenSurfaceTabEdges(event.currentTarget); - const focused = ( - event.currentTarget.getRootNode() as Document | ShadowRoot - ).activeElement; + const focused = getShadowAwareActiveElement(event.currentTarget); if (!first || !last) { if (focused === event.currentTarget) event.preventDefault(); return; @@ -9660,9 +9658,7 @@ export function App({ setShowAddWorkspaceDialog(false)} onAdd={handleAddWorkspace} - onSuggest={(prefix) => - workspaceActions.suggestWorkspacePaths(prefix) - } + onSuggest={workspaceActions.suggestWorkspacePaths} onPick={async () => { const result = await workspaceActions.pickWorkspaceDirectory(); return result.selected ? result.path : undefined; diff --git a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx index ae2f9c9dac4..290942ba5fc 100644 --- a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx +++ b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx @@ -3,6 +3,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { I18nProvider } from '../../i18n'; +import { WebShellPortalRootContext } from '../../portalRoot'; Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); @@ -306,11 +307,13 @@ describe('AddWorkspaceDialog', () => { it('opens the system picker and fills the selected absolute path', async () => { const onPick = vi.fn().mockResolvedValue('/Users/me/code'); + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); mount( , ); @@ -321,6 +324,254 @@ describe('AddWorkspaceDialog', () => { expect(onPick).toHaveBeenCalledTimes(1); expect(input().value).toBe('/Users/me/code'); + expect(document.activeElement).not.toBe(input()); + await settle(); + expect(listbox()).toBeNull(); + }); + + it('keeps suggestions closed when a lookup finishes after blur', async () => { + let resolveSuggestions!: (value: typeof SUGGESTIONS) => void; + const onSuggest = vi.fn( + () => + new Promise((resolve) => { + resolveSuggestions = resolve; + }), + ); + mount( + , + ); + + type('/home/me/co'); + await settle(); + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + resolveSuggestions(SUGGESTIONS); + await Promise.resolve(); + }); + + expect(listbox()).toBeNull(); + }); + + it('closes the suggestion list when the input blurs', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + type('/home/me/co'); + await settle(); + expect(listbox()).not.toBeNull(); + + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + + expect(listbox()).toBeNull(); + }); + + it('cancels the pending blur dismiss when focus returns to the input', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + type('/home/me/co'); + await settle(); + expect(listbox()).not.toBeNull(); + + act(() => input().blur()); + act(() => input().focus()); + // Cross the blur timer's deadline: a still-pending timer would close + // the list and invalidate in-flight lookups via the sequence counter. + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + expect(listbox()).not.toBeNull(); + + type('/home/me/cod'); + await settle(); + expect(listbox()).not.toBeNull(); + }); + + it('opens suggestions on the first edit after a re-blur within the blur window', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + act(() => input().blur()); + act(() => input().focus()); + await act(async () => { + await vi.advanceTimersByTimeAsync(60); + }); + // Second blur while the first blur timer is still pending: it must + // cancel that timer rather than stack a second one on top of it. + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(20); + }); + act(() => input().focus()); + // Cross the first timer's original deadline: an uncancelled timer + // would have bumped the sequence counter by now. + await act(async () => { + await vi.advanceTimersByTimeAsync(40); + }); + + type('/home/me/co'); + await settle(); + + expect(listbox()).not.toBeNull(); + }); + + it('opens suggestions on the first edit after the blur dismiss fired', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + type('/home/me/co'); + await settle(); + expect(listbox()).not.toBeNull(); + + // Stay blurred past the dismiss window so the timer fires. + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + expect(listbox()).toBeNull(); + + // Returning and editing must reopen the list on the first edit. + act(() => input().focus()); + type('/home/me/cod'); + await settle(); + + expect(listbox()).not.toBeNull(); + }); + + it('keeps suggestions closed when a pre-blur lookup resolves after refocus', async () => { + let resolveSuggestions!: (value: typeof SUGGESTIONS) => void; + const onSuggest = vi.fn( + () => + new Promise((resolve) => { + resolveSuggestions = resolve; + }), + ); + mount( + , + ); + + type('/home/me/co'); + // Let the debounce fire so the lookup is in flight, then blur past the + // dismiss window so the timer fires while the lookup is pending. + await settle(); + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + + // Refocus before the stale lookup resolves: the dismiss must + // invalidate it, not let it pop the list open with zero edits. + act(() => input().focus()); + await act(async () => { + resolveSuggestions(SUGGESTIONS); + await Promise.resolve(); + }); + + expect(listbox()).toBeNull(); + }); + + it('drops stale suggestions on blur dismiss so refocus cannot reopen them', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + type('/home/me/co'); + await settle(); + expect(listbox()).not.toBeNull(); + + // Blur past the dismiss window while the second lookup is still + // debounced: the timer invalidates it, so it never refreshes the + // stale entries from the first prefix. + type('/home/me/cod'); + act(() => input().blur()); + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + + // Refocusing without editing must not reopen the stale entries. + act(() => input().focus()); + keydown('ArrowDown'); + + expect(listbox()).toBeNull(); + }); + + it('opens suggestions for a focused input in a shadow-DOM portal root', async () => { + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + const host = document.createElement('div'); + document.body.append(host); + container = host; + const shadowRoot = host.attachShadow({ mode: 'open' }); + const portalRoot = document.createElement('div'); + shadowRoot.append(portalRoot); + const dialogContainer = document.createElement('div'); + shadowRoot.append(dialogContainer); + root = createRoot(dialogContainer); + act(() => { + root!.render( + + + + + , + ); + }); + + const shadowInput = shadowRoot.querySelector( + '#add-workspace-path', + )!; + // document.activeElement retargets to the shadow host in this mode. + expect(shadowRoot.activeElement).toBe(shadowInput); + expect(document.activeElement).toBe(host); + + typeInto(shadowInput, '/home/me/co'); + await settle(); + + expect(shadowRoot.querySelector('[role="listbox"]')).not.toBeNull(); }); it('leaves the path unchanged when the system picker is cancelled', async () => { @@ -341,16 +592,204 @@ describe('AddWorkspaceDialog', () => { expect(input().value).toBe(''); }); + it('opens suggestions on the first edit after a cancelled picker', async () => { + let resolvePick!: (value: string | undefined) => void; + const onPick = vi.fn( + () => + new Promise((resolve) => { + resolvePick = resolve; + }), + ); + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + act(() => { + browseButton().click(); + }); + // Simulate the picker staying open well past the blur window. + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + await act(async () => { + resolvePick(undefined); + await Promise.resolve(); + await Promise.resolve(); + }); + + act(() => input().focus()); + type('/home/me/co'); + await settle(); + + expect(onSuggest).toHaveBeenCalledWith('/home/me/co'); + expect(listbox()).not.toBeNull(); + }); + + it('opens suggestions on the first edit after picking the typed path', async () => { + let resolvePick!: (value: string | undefined) => void; + const onPick = vi.fn( + () => + new Promise((resolve) => { + resolvePick = resolve; + }), + ); + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + type('/home/me/co'); + await settle(); + expect(listbox()).not.toBeNull(); + + act(() => { + browseButton().click(); + }); + // Browse closes the open list while the picker is up. + expect(listbox()).toBeNull(); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + await act(async () => { + // Same value as typed: setPath bails out, so no path-change effect. + resolvePick('/home/me/co'); + await Promise.resolve(); + await Promise.resolve(); + }); + + act(() => input().focus()); + type('/home/me/cod'); + await settle(); + + expect(listbox()).not.toBeNull(); + }); + + it('does not pop suggestions open on refocus when a same-value pick raced an in-flight lookup', async () => { + let resolvePick!: (value: string | undefined) => void; + const onPick = vi.fn( + () => + new Promise((resolve) => { + resolvePick = resolve; + }), + ); + let resolveSuggestions!: (value: typeof SUGGESTIONS) => void; + const onSuggest = vi.fn( + () => + new Promise((resolve) => { + resolveSuggestions = resolve; + }), + ); + mount( + , + ); + + type('/home/me/co'); + // Browse while the first lookup is still debounced, then let the + // debounce fire so the lookup is in flight while the picker is open. + act(() => { + browseButton().click(); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + await act(async () => { + resolvePick('/home/me/co'); + await Promise.resolve(); + await Promise.resolve(); + }); + + act(() => input().focus()); + await act(async () => { + resolveSuggestions(SUGGESTIONS); + await Promise.resolve(); + }); + + // The lookup predates Browse; the same-value pick must invalidate it + // so a bare refocus cannot pop the list open. + expect(listbox()).toBeNull(); + + type('/home/me/cod'); + await settle(); + await act(async () => { + // resolveSuggestions now points at the second lookup's resolver. + resolveSuggestions(SUGGESTIONS); + await Promise.resolve(); + }); + expect(listbox()).not.toBeNull(); + }); + + it('keeps the pick-triggered lookup closed until the first edit', async () => { + let resolvePick!: (value: string | undefined) => void; + const onPick = vi.fn( + () => + new Promise((resolve) => { + resolvePick = resolve; + }), + ); + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); + mount( + , + ); + + act(() => { + browseButton().click(); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + await act(async () => { + resolvePick('/Users/me/code'); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(input().value).toBe('/Users/me/code'); + // Refocusing to fine-tune the picked path while its lookup is still + // pending must not pop the list open; only a real edit may. + act(() => input().focus()); + await settle(); + expect(listbox()).toBeNull(); + + type('/Users/me/code/s'); + await settle(); + expect(listbox()).not.toBeNull(); + }); + it('shows an error when the system picker fails', async () => { const onPick = vi.fn().mockRejectedValue(new Error('boom')); + const onSuggest = vi.fn().mockResolvedValue(SUGGESTIONS); mount( , ); + // The picker rejects instantly (e.g. no zenity/osascript on a + // headless host). await act(async () => { browseButton().click(); await Promise.resolve(); @@ -360,6 +799,20 @@ describe('AddWorkspaceDialog', () => { expect(alert()?.textContent).toContain( 'Unable to open the system folder picker', ); + + // No blur timer is pending here — pickDirectory cancels it right + // after blur(). Cross the dismiss deadline anyway: even a leaked + // dismiss must not stop the first edit below from opening the list. + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + + act(() => input().focus()); + type('/home/me/co'); + await settle(); + + expect(onSuggest).toHaveBeenCalledWith('/home/me/co'); + expect(listbox()).not.toBeNull(); }); it('never queries for a non-absolute value', async () => { diff --git a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.tsx b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.tsx index 557c7fdd99e..8d13bf1df39 100644 --- a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.tsx +++ b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.tsx @@ -1,5 +1,6 @@ import { useState, useCallback, useRef, useEffect } from 'react'; import { useI18n } from '../../i18n'; +import { getShadowAwareActiveElement } from '../../utils/dom'; import { DialogShell } from './DialogShell'; import { Button } from '../ui/button'; import { @@ -72,14 +73,27 @@ export function AddWorkspaceDialog({ const listOpenRef = useRef(false); listOpenRef.current = listOpen && suggestions.length > 0; const suggestSeqRef = useRef(0); - // Set when a suggestion is accepted or the list is dismissed, so the - // path-change effect knows whether to reopen the list for that update. + // Set while Browse is in flight, so the path-change effect keeps the + // pick-triggered lookup closed until the first edit; blur dismissal + // invalidates in-flight lookups via suggestSeqRef instead. const suppressNextFetchOpenRef = useRef(false); + const blurTimeoutRef = useRef | undefined>( + undefined, + ); useEffect(() => { inputRef.current?.focus(); }, []); + const cancelBlurDismiss = useCallback(() => { + if (blurTimeoutRef.current !== undefined) { + clearTimeout(blurTimeoutRef.current); + blurTimeoutRef.current = undefined; + } + }, []); + + useEffect(() => () => cancelBlurDismiss(), [cancelBlurDismiss]); + const closeList = useCallback(() => { setListOpen(false); setHighlight(-1); @@ -104,7 +118,12 @@ export function AddWorkspaceDialog({ setSuggestions(result.suggestions); setHostSep(result.sep || '/'); setHighlight(-1); - if (openOnResult || listOpenRef.current) { + const input = inputRef.current; + if ( + input !== null && + getShadowAwareActiveElement(input) === input && + (openOnResult || listOpenRef.current) + ) { setListOpen(result.suggestions.length > 0); } }, @@ -149,22 +168,37 @@ export function AddWorkspaceDialog({ const pickDirectory = useCallback(async () => { if (!onPick) return; + inputRef.current?.blur(); + // The blur above scheduled the delayed dismiss; cancel it and apply the + // close + suppress now so the timer cannot fire after the outcome below. + cancelBlurDismiss(); + closeList(); + suppressNextFetchOpenRef.current = true; setBrowsing(true); setError(null); + let pickedPath: string | undefined; try { - const selectedPath = await onPick(); - if (selectedPath) { + pickedPath = await onPick(); + if (pickedPath && pickedPath !== path) { + // Leave the suppress flag set: the path-change effect consumes it, + // keeping the pick-triggered lookup closed until the first edit. ++suggestSeqRef.current; - setPath(selectedPath); + setPath(pickedPath); setSuggestions([]); - closeList(); + } else { + // Cancelled, failed, or same-value pick: the first edit must open. + // A same-value pick keeps the typed path, so invalidate any lookup + // already in flight from before Browse was clicked. + if (pickedPath) ++suggestSeqRef.current; + suppressNextFetchOpenRef.current = false; } } catch { + suppressNextFetchOpenRef.current = false; setError(t('sidebar.addWorkspaceBrowseError')); } finally { setBrowsing(false); } - }, [onPick, closeList, t]); + }, [onPick, path, closeList, cancelBlurDismiss, t]); const handleInputKeyDown = useCallback( (event: React.KeyboardEvent) => { @@ -277,10 +311,20 @@ export function AddWorkspaceDialog({ if (error) setError(null); }} onKeyDown={handleInputKeyDown} + onFocus={cancelBlurDismiss} onBlur={() => { // Delay so a mousedown on a suggestion wins over blur. - setTimeout(() => { - suppressNextFetchOpenRef.current = true; + cancelBlurDismiss(); + blurTimeoutRef.current = setTimeout(() => { + blurTimeoutRef.current = undefined; + // Invalidate in-flight lookups via the sequence counter + // rather than suppressing the next fetch, which would + // leak into the first edit after the user refocuses. + ++suggestSeqRef.current; + // Drop the stale entries too: the invalidated lookup + // never refreshes them, and ArrowDown would reopen + // whatever is left against the current input. + setSuggestions([]); closeList(); }, 100); }} diff --git a/packages/web-shell/client/utils/dom.ts b/packages/web-shell/client/utils/dom.ts index c990f3f3a2f..0ce90831704 100644 --- a/packages/web-shell/client/utils/dom.ts +++ b/packages/web-shell/client/utils/dom.ts @@ -5,3 +5,15 @@ export function isEditableTarget(target: EventTarget | null): boolean { 'input, textarea, select, [contenteditable="true"], .cm-editor, [data-keyboard-scope]', ); } + +// document.activeElement retargets to the shadow host when focus is inside +// a shadow root (Web Shell portal mode), so resolve the active element from +// the element's own root instead. +export function getShadowAwareActiveElement( + element: Element | null | undefined, +): Element | null { + const root = element?.getRootNode(); + return root instanceof Document || root instanceof ShadowRoot + ? root.activeElement + : null; +}