From 413321e261febabb8766a4d1cb1caa7782009664 Mon Sep 17 00:00:00 2001 From: creatixchu Date: Wed, 29 Jul 2026 23:56:21 +0800 Subject: [PATCH] =?UTF-8?q?fix(host):=20review=20round=2013=20=E2=80=94=20?= =?UTF-8?q?probe=20never=20gates=20a=20landing;=20relist=20supersession=20?= =?UTF-8?q?covered,=20false=20ignores=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/client/DirectoryBrowser.module.css | 8 ++-- .../src/client/DirectoryBrowser.tsx | 38 +++++++++------ .../tests/directory-browser.spec.tsx | 47 +++++++++++++++++++ 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/packages/host/directory-picker-browse/src/client/DirectoryBrowser.module.css b/packages/host/directory-picker-browse/src/client/DirectoryBrowser.module.css index 176bca7737..6cce6fabd0 100644 --- a/packages/host/directory-picker-browse/src/client/DirectoryBrowser.module.css +++ b/packages/host/directory-picker-browse/src/client/DirectoryBrowser.module.css @@ -128,8 +128,8 @@ flex-direction: column; flex: 1 1 0; min-height: 0; - /* Right inset is slimmer than the left: the trailing column's own 8px - * scrollbar clearance makes up the optical difference. */ + /* Right inset is slimmer than the left: the trailing column's own + * scrollbar clearance (see .column) makes up the optical difference. */ padding: 16px 16px 16px 24px; } @@ -144,8 +144,8 @@ flex: 1 1 0; min-height: 0; /* 12px of row gap on each side of the divider; the left side reads wider - * by the column's trailing 8px scrollbar clearance, which is deliberate — - * the thumb needs that room, the right pane's rows do not. */ + * by the column's trailing scrollbar clearance (see .column) — the thumb + * needs that room, the right pane's rows do not. */ gap: 12px; overflow-x: auto; scrollbar-width: none; diff --git a/packages/host/directory-picker-browse/src/client/DirectoryBrowser.tsx b/packages/host/directory-picker-browse/src/client/DirectoryBrowser.tsx index fd4e7c7b39..e27a72a8e5 100644 --- a/packages/host/directory-picker-browse/src/client/DirectoryBrowser.tsx +++ b/packages/host/directory-picker-browse/src/client/DirectoryBrowser.tsx @@ -272,6 +272,20 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen, const refocusEditZone = useRef(false) const editZoneRef = useRef(null) + /** + * Whether the focused element sits among the miller rows — probed before + * a landing replaces the row nodes, to decide focus parking. A probe + * only: it never gates the landing itself (a torn-down ref in a close + * race merely skips the parking). + * @returns true when `document.activeElement` is inside the miller row. + */ + const focusInMillerRows = useCallback((): boolean => { + const rowHost = millerRowRef.current + /* v8 ignore next -- close-race guard: the commit-to-effect window is not deterministically reproducible. */ + if (rowHost === null) return false + return rowHost.contains(document.activeElement) + }, []) + /** * Launch a follow-up listing under the CURRENT supersession seq: a newer * intent aborts it like the leg it continues, and it supersedes nothing. @@ -312,11 +326,10 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen, // The landing replaces every row key; a slow jump leaves the OLD // rows tabbable meanwhile (parentInert excludes loading), so focus // may live among them. With no selection yet the edit zone is the - // park target (body-guarded, like every other exit). - const rowHost = millerRowRef.current - /* v8 ignore next -- close-race guard: the commit-to-effect window is not deterministically reproducible. */ - if (rowHost === null) return - if (rowHost.contains(document.activeElement)) refocusEditZone.current = true + // park target (body-guarded, like every other exit). The probe never + // gates the commit below — stranding the dialog in loading over a + // focus check would be far worse than a skipped parking. + if (focusInMillerRows()) refocusEditZone.current = true setParent(target) setSelected(null) setChild(null) @@ -338,13 +351,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen, // The upgrade replaces every committed row node; if focus lives // among them (Tab reached the rows during the parent leg), arm the // refocus effect so it re-parks on the re-selected row. - const rowHost = millerRowRef.current - // A close race can clear the ref before the close effect's - // supersede runs (commit precedes passive effects): drop the - // upgrade, the dialog is going away. - /* v8 ignore next -- close-race guard: the commit-to-effect window is not deterministically reproducible. */ - if (rowHost === null) return - if (rowHost.contains(document.activeElement)) refocusPick.current = true + if (focusInMillerRows()) refocusPick.current = true setParent(parentLevel) setSelected(match) setChild(target) @@ -358,7 +365,7 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen, setLoading(false) setError(failureText(reason)) }) - }, [launchListing, continueScan]) + }, [launchListing, continueScan, focusInMillerRows]) /** * Close the nested create dialog. Its unmount drops focus to body (the @@ -486,13 +493,14 @@ export function DirectoryBrowser({ open, listDirectory, createDirectory, onOpen, const { seq, scan } = launchListing(targetPath) setLoading(true) scan.then((level) => { - /* v8 ignore next -- same fence as navigate/select; the modal blocks superseding input */ + // The nested dialog closed before this relist launched, so the card + // is interactive meanwhile: a pick or crumb jump supersedes it. if (seq !== requestSeq.current) return setParent(level) setLoading(false) select({ name, path: createdPath, hidden: false }) }, (reason: unknown) => { - /* v8 ignore next -- same fence as navigate/select; the modal blocks superseding input */ + // Same interactive-window fence as the success branch above. if (seq !== requestSeq.current) return setLoading(false) setError(failureText(reason)) diff --git a/packages/host/directory-picker-browse/tests/directory-browser.spec.tsx b/packages/host/directory-picker-browse/tests/directory-browser.spec.tsx index e0e05436a0..03db2e0b8b 100644 --- a/packages/host/directory-picker-browse/tests/directory-browser.spec.tsx +++ b/packages/host/directory-picker-browse/tests/directory-browser.spec.tsx @@ -269,6 +269,53 @@ describe('DirectoryBrowser', () => { expect(document.activeElement).toBe(screen.getByRole('button', { name: 'browser.editPath' })) }) + it('a pick during the post-create relist supersedes it: late settlements drop', async () => { + const settlers: { resolve: (value: DirectoryListing) => void; reject: (reason: unknown) => void }[] = [] + const listDirectory = vi.fn(async (path?: string) => { + // Explicit HOME requests are the relists; the initial open lists home + // through the absent-path form. + if (path === HOME) { + return new Promise((resolve, reject) => { settlers.push({ resolve, reject }) }) + } + return listingFor(path) + }) + mount({ listDirectory }) + await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() }) + fireEvent.click(screen.getByRole('button', { name: 'browser.newFolder' })) + fireEvent.change(screen.getByLabelText('browser.folderName'), { target: { value: 'ghost' } }) + fireEvent.click(screen.getByRole('button', { name: 'browser.create' })) + // The nested dialog is gone while the relist hangs; the card is + // interactive, and picking a row supersedes the relist. + await waitFor(() => { expect(settlers).toHaveLength(1) }) + fireEvent.click(rowButton(screen.getByRole('listitem'))) + await waitFor(() => { expect(columns()).toHaveLength(2) }) + // The stale relist settles late and must not land or select ghost. + await act(async () => { settlers[0]!.resolve({ ...listingFor(HOME), entries: [] }) }) + expect(within(columns()[0]!).getByText('Documents')).toBeTruthy() + expect(rowButton(within(columns()[0]!).getByRole('listitem')).getAttribute('aria-current')).toBe('true') + }) + + it('a rejection of a superseded post-create relist is equally silent', async () => { + const settlers: { resolve: (value: DirectoryListing) => void; reject: (reason: unknown) => void }[] = [] + const listDirectory = vi.fn(async (path?: string) => { + if (path === HOME) { + return new Promise((resolve, reject) => { settlers.push({ resolve, reject }) }) + } + return listingFor(path) + }) + mount({ listDirectory }) + await waitFor(() => { expect(screen.getByRole('listitem')).toBeTruthy() }) + fireEvent.click(screen.getByRole('button', { name: 'browser.newFolder' })) + fireEvent.change(screen.getByLabelText('browser.folderName'), { target: { value: 'ghost' } }) + fireEvent.click(screen.getByRole('button', { name: 'browser.create' })) + await waitFor(() => { expect(settlers).toHaveLength(1) }) + fireEvent.click(rowButton(screen.getByRole('listitem'))) + await waitFor(() => { expect(columns()).toHaveLength(2) }) + await act(async () => { settlers[0]!.reject(new Error('late')) }) + expect(screen.queryByRole('alert')).toBeNull() + expect(columns()).toHaveLength(2) + }) + it('a failed create relist parks focus on the edit zone with the error shown', async () => { let relists = 0 const listDirectory = vi.fn(async (path?: string) => {