fix(host): harden the browser dialog against review round-2 races
- Dismissal (Escape/mask) is ignored while adoption is busy: the owner's in-flight createWorkspace must not land behind an apparent cancel. - Every parent control goes inert while the nested create dialog is open (Modal traps no focus, so Shift-Tab/AT could close, adopt, or retarget underneath the child). - Creation settlements are gated on an open-generation ref: a create that resolves or rejects after the flow closed (and possibly reopened) can no longer relist the stale target or surface its alert in the fresh dialog. - The keyless snapshot waits for the Open button's enabled state before clicking — on slow runners the selection's child listing was still in flight and the click landed on a disabled button.
This commit is contained in:
@@ -202,6 +202,11 @@ it('adopts a directory through the composed in-app browse flow and lands in its
|
|||||||
// row's name span is stable (clicks bubble to the row button).
|
// row's name span is stable (clicks bubble to the row button).
|
||||||
fireEvent.click(await within(dialog).findByText('Documents', {}, { timeout: 10_000 }))
|
fireEvent.click(await within(dialog).findByText('Documents', {}, { timeout: 10_000 }))
|
||||||
fireEvent.click(await within(dialog).findByText('project', {}, { timeout: 10_000 }))
|
fireEvent.click(await within(dialog).findByText('project', {}, { timeout: 10_000 }))
|
||||||
|
// Open disables while the selection's child listing is in flight; wait for
|
||||||
|
// the enabled state or the click lands on a dead button on slow runners.
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(within(dialog).getByRole<HTMLButtonElement>('button', { name: '打开' }).disabled).toBe(false)
|
||||||
|
}, { timeout: 10_000 })
|
||||||
fireEvent.click(within(dialog).getByRole('button', { name: '打开' }))
|
fireEvent.click(within(dialog).getByRole('button', { name: '打开' }))
|
||||||
await findHeroComposer()
|
await findHeroComposer()
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
|
|||||||
@@ -112,6 +112,9 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
const [creatingFolder, setCreatingFolder] = useState(false)
|
const [creatingFolder, setCreatingFolder] = useState(false)
|
||||||
const [createError, setCreateError] = useState<string | null>(null)
|
const [createError, setCreateError] = useState<string | null>(null)
|
||||||
const requestSeq = useRef(0)
|
const requestSeq = useRef(0)
|
||||||
|
// Bumped on every open/close edge: settlements from a previous open (a
|
||||||
|
// pending creation included) must never mutate a reopened dialog.
|
||||||
|
const openGeneration = useRef(0)
|
||||||
// Deep ancestry overflows the trail; keep its tail (the current directory
|
// Deep ancestry overflows the trail; keep its tail (the current directory
|
||||||
// and the edit zone beside it) in view whenever the chain changes.
|
// and the edit zone beside it) in view whenever the chain changes.
|
||||||
const crumbTrailRef = useRef<HTMLSpanElement | null>(null)
|
const crumbTrailRef = useRef<HTMLSpanElement | null>(null)
|
||||||
@@ -164,10 +167,12 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
// Every open starts fresh at the Host home directory; closing invalidates
|
// Every open starts fresh at the Host home directory; closing invalidates
|
||||||
// any in-flight response so a late arrival cannot repopulate a closed dialog.
|
// any in-flight response so a late arrival cannot repopulate a closed dialog.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
openGeneration.current += 1
|
||||||
if (open) {
|
if (open) {
|
||||||
setParent(null)
|
setParent(null)
|
||||||
setSelected(null)
|
setSelected(null)
|
||||||
setChild(null)
|
setChild(null)
|
||||||
|
setCreatingFolder(false)
|
||||||
navigate()
|
navigate()
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -190,7 +195,11 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
if (name === '') return
|
if (name === '') return
|
||||||
setCreatingFolder(true)
|
setCreatingFolder(true)
|
||||||
setCreateError(null)
|
setCreateError(null)
|
||||||
|
const generation = openGeneration.current
|
||||||
createDirectory(targetPath, name).then((createdPath) => {
|
createDirectory(targetPath, name).then((createdPath) => {
|
||||||
|
// A settlement from a closed (possibly reopened) flow must not touch
|
||||||
|
// the fresh dialog or issue a relist against the stale target.
|
||||||
|
if (generation !== openGeneration.current) return
|
||||||
setCreatingFolder(false)
|
setCreatingFolder(false)
|
||||||
setFolderDraft(null)
|
setFolderDraft(null)
|
||||||
// Land like a right-column pick (figma 802:57446 → 813:23278 flow): the
|
// Land like a right-column pick (figma 802:57446 → 813:23278 flow): the
|
||||||
@@ -210,6 +219,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
setError(failureText(reason))
|
setError(failureText(reason))
|
||||||
})
|
})
|
||||||
}, (reason: unknown) => {
|
}, (reason: unknown) => {
|
||||||
|
if (generation !== openGeneration.current) return
|
||||||
setCreatingFolder(false)
|
setCreatingFolder(false)
|
||||||
setCreateError(failureText(reason))
|
setCreateError(failureText(reason))
|
||||||
})
|
})
|
||||||
@@ -226,14 +236,20 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
|
|
||||||
if (!open) return null
|
if (!open) return null
|
||||||
const twoPane = selected !== null
|
const twoPane = selected !== null
|
||||||
|
// The nested create dialog owns the interaction while open: Modal has no
|
||||||
|
// focus trap, so every parent control goes inert (Shift-Tab or AT must not
|
||||||
|
// close, adopt, or retarget underneath the child).
|
||||||
|
const parentInert = busy || folderDraft !== null
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<Modal
|
<Modal
|
||||||
open={open}
|
open={open}
|
||||||
// Escape and mask reach every mounted Modal's document listener; while
|
// Escape and mask reach every mounted Modal's document listener; while
|
||||||
// the nested create dialog is up, only that topmost dialog may close
|
// the nested create dialog is up only that topmost dialog may close
|
||||||
// (its own guard keeps an in-flight creation open).
|
// (its own guard keeps an in-flight creation open), and an in-flight
|
||||||
onClose={() => { if (folderDraft === null) onClose() }}
|
// adoption pins the flow — dismissing it would leave the owner's
|
||||||
|
// createWorkspace to land after an apparent cancel.
|
||||||
|
onClose={() => { if (folderDraft === null && !busy) onClose() }}
|
||||||
title={t('browser.title')}
|
title={t('browser.title')}
|
||||||
className={clsx(css.dialog)}
|
className={clsx(css.dialog)}
|
||||||
headless
|
headless
|
||||||
@@ -251,7 +267,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
className={css.crumb}
|
className={css.crumb}
|
||||||
disabled={busy}
|
disabled={parentInert}
|
||||||
onClick={() => { navigate(crumb.path) }}
|
onClick={() => { navigate(crumb.path) }}
|
||||||
>
|
>
|
||||||
{crumb.name}
|
{crumb.name}
|
||||||
@@ -264,7 +280,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
type="button"
|
type="button"
|
||||||
className={css.crumbEditZone}
|
className={css.crumbEditZone}
|
||||||
aria-label={t('browser.editPath')}
|
aria-label={t('browser.editPath')}
|
||||||
disabled={parent === null || busy}
|
disabled={parent === null || parentInert}
|
||||||
/* v8 ignore next -- narrowing guard: the zone disables while the level is null. */
|
/* v8 ignore next -- narrowing guard: the zone disables while the level is null. */
|
||||||
onClick={() => { if (parent !== null) setPathDraft(selected?.path ?? parent.path) }}
|
onClick={() => { if (parent !== null) setPathDraft(selected?.path ?? parent.path) }}
|
||||||
/>
|
/>
|
||||||
@@ -299,7 +315,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
<LevelColumn
|
<LevelColumn
|
||||||
entries={parent.entries}
|
entries={parent.entries}
|
||||||
selectedPath={selected?.path ?? null}
|
selectedPath={selected?.path ?? null}
|
||||||
busy={busy}
|
busy={parentInert}
|
||||||
onPick={select}
|
onPick={select}
|
||||||
wide={!twoPane}
|
wide={!twoPane}
|
||||||
/>
|
/>
|
||||||
@@ -309,7 +325,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
<LevelColumn
|
<LevelColumn
|
||||||
entries={child.entries}
|
entries={child.entries}
|
||||||
selectedPath={null}
|
selectedPath={null}
|
||||||
busy={busy}
|
busy={parentInert}
|
||||||
onPick={advance}
|
onPick={advance}
|
||||||
wide={false}
|
wide={false}
|
||||||
/>
|
/>
|
||||||
@@ -321,7 +337,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
<Button
|
<Button
|
||||||
variant="outline"
|
variant="outline"
|
||||||
icon={<IconPlusOutline16 size={14} />}
|
icon={<IconPlusOutline16 size={14} />}
|
||||||
disabled={parent === null || busy || loading || folderDraft !== null}
|
disabled={parent === null || loading || parentInert}
|
||||||
onClick={() => {
|
onClick={() => {
|
||||||
setFolderDraft('')
|
setFolderDraft('')
|
||||||
setCreateError(null)
|
setCreateError(null)
|
||||||
@@ -330,11 +346,11 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen,
|
|||||||
{t('browser.newFolder')}
|
{t('browser.newFolder')}
|
||||||
</Button>
|
</Button>
|
||||||
<span className={css.footerGap} />
|
<span className={css.footerGap} />
|
||||||
<Button variant="outline" className={clsx(css.footerAction)} disabled={busy} onClick={onClose}>{t('browser.cancel')}</Button>
|
<Button variant="outline" className={clsx(css.footerAction)} disabled={parentInert} onClick={onClose}>{t('browser.cancel')}</Button>
|
||||||
<Button
|
<Button
|
||||||
variant="primary"
|
variant="primary"
|
||||||
className={clsx(css.footerAction)}
|
className={clsx(css.footerAction)}
|
||||||
disabled={targetPath === null || loading || busy}
|
disabled={targetPath === null || loading || parentInert}
|
||||||
/* v8 ignore next -- narrowing guard: Open disables while no target exists. */
|
/* v8 ignore next -- narrowing guard: Open disables while no target exists. */
|
||||||
onClick={() => { if (targetPath !== null) onOpen(targetPath) }}
|
onClick={() => { if (targetPath !== null) onOpen(targetPath) }}
|
||||||
>
|
>
|
||||||
|
|||||||
@@ -264,6 +264,65 @@ describe('DirectoryBrowser', () => {
|
|||||||
expect(screen.getByRole<HTMLButtonElement>('button', { name: 'browser.newFolder' }).disabled).toBe(false)
|
expect(screen.getByRole<HTMLButtonElement>('button', { name: 'browser.newFolder' }).disabled).toBe(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('ignores dismissal while adoption is busy', async () => {
|
||||||
|
const b = mount({ busy: true })
|
||||||
|
await waitFor(() => { expect(screen.getByRole('dialog')).toBeTruthy() })
|
||||||
|
fireEvent.keyDown(document, { key: 'Escape' })
|
||||||
|
expect(b.onClose).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('makes every parent control inert while the nested create dialog is open', async () => {
|
||||||
|
mount()
|
||||||
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
fireEvent.click(screen.getByRole('button', { name: 'browser.newFolder' }))
|
||||||
|
// Modal traps no focus: Shift-Tab/AT reach the parent, so closing,
|
||||||
|
// adopting, and retargeting must all disable underneath the child. Both
|
||||||
|
// dialogs carry a cancel: the parent's disables, the child's stays live.
|
||||||
|
const cancels = screen.getAllByRole<HTMLButtonElement>('button', { name: 'browser.cancel' })
|
||||||
|
expect(cancels.map(button => button.disabled).sort()).toEqual([false, true])
|
||||||
|
expect(screen.getByRole<HTMLButtonElement>('button', { name: 'browser.open' }).disabled).toBe(true)
|
||||||
|
expect(screen.getByRole<HTMLButtonElement>('button', { name: 'browser.editPath' }).disabled).toBe(true)
|
||||||
|
for (const row of screen.getAllByRole<HTMLButtonElement>('listitem')) {
|
||||||
|
expect(row.disabled).toBe(true)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
it('drops a creation failure that lands after the flow closed and reopened', async () => {
|
||||||
|
let rejectCreate!: (reason: unknown) => void
|
||||||
|
const createDirectory = vi.fn(() => new Promise<string>((_settle, reject) => { rejectCreate = reject }))
|
||||||
|
const b = mount({ createDirectory })
|
||||||
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
fireEvent.click(screen.getByRole('button', { name: 'browser.newFolder' }))
|
||||||
|
fireEvent.change(screen.getByLabelText('browser.folderName'), { target: { value: 'slow' } })
|
||||||
|
fireEvent.click(screen.getByRole('button', { name: 'browser.create' }))
|
||||||
|
b.view.rerender(<DirectoryBrowser {...b.props} open={false} />)
|
||||||
|
b.view.rerender(<DirectoryBrowser {...b.props} open />)
|
||||||
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
// The stale failure must not surface an alert inside the fresh flow.
|
||||||
|
await act(async () => { rejectCreate(new Error('too late')) })
|
||||||
|
expect(screen.queryByText('too late')).toBeNull()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('drops a creation that settles after the flow closed and reopened', async () => {
|
||||||
|
let settleCreate!: (path: string) => void
|
||||||
|
const createDirectory = vi.fn(() => new Promise<string>((settle) => { settleCreate = settle }))
|
||||||
|
const b = mount({ createDirectory })
|
||||||
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
fireEvent.click(screen.getByRole('button', { name: 'browser.newFolder' }))
|
||||||
|
fireEvent.change(screen.getByLabelText('browser.folderName'), { target: { value: 'slow' } })
|
||||||
|
fireEvent.click(screen.getByRole('button', { name: 'browser.create' }))
|
||||||
|
b.view.rerender(<DirectoryBrowser {...b.props} open={false} />)
|
||||||
|
b.view.rerender(<DirectoryBrowser {...b.props} open />)
|
||||||
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
const listCallsBefore = b.listDirectory.mock.calls.length
|
||||||
|
// The stale settlement must not relist the old target or reopen the
|
||||||
|
// nested dialog's state inside the fresh flow.
|
||||||
|
await act(async () => { settleCreate(`${HOME}/slow`) })
|
||||||
|
expect(b.listDirectory.mock.calls.length).toBe(listCallsBefore)
|
||||||
|
expect(screen.queryByLabelText('browser.folderName')).toBeNull()
|
||||||
|
expect(screen.getByText('Documents')).toBeTruthy()
|
||||||
|
})
|
||||||
|
|
||||||
it('creates a folder through the nested dialog and lands with it selected', async () => {
|
it('creates a folder through the nested dialog and lands with it selected', async () => {
|
||||||
const b = mount()
|
const b = mount()
|
||||||
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() })
|
||||||
|
|||||||
Reference in New Issue
Block a user