fix(web): add Home to location dropdown, stop auto-selecting on descend
Two folder-picker UX bugs: 1. Once on a Windows drive (C:, D:), there was no way back to ~ from the dropdown — only drives were listed. Replace the "Drive" placeholder with a real "Home" option (value ~) so the dropdown always offers every root. The select's value reflects browsePath, so it reads "Home" at ~ and the drive letter inside a drive. Renamed the aria label from "Drive" to "Location" to match. 2. Double-clicking a folder to descend was silently re-selecting the folder you'd just navigated into — if it wasn't a git repo, the modal flashed a red "not a git repository" warning for every non-repo folder a user passed through. Same flaw fired on breadcrumb clicks, drive switches, and history nav. Root cause: browse() defaulted selectedPath to the navigation target. Changed the default to "" — selection is now only ever set by explicit user intent (clicking a row, the "this folder" row, or pressing Enter on a typed location). Callers that genuinely seed a selection (reset, refresh, location-input submit) already pass selectedPath explicitly, so they're unaffected. Also dropped the now-redundant selectedPath from the drive-switch call. Tests: 29/29 passing. Added two tests for the Home option (it's present and picking it routes to ~; the select value tracks browsePath) and one for the descend-doesn't-select behavior. Updated two pre-existing tests that asserted the old auto-select contract with comments explaining the new one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
b0188d68ea
commit
be29941e98
|
|
@ -151,15 +151,20 @@ export function DirectoryBrowser({ browser }: DirectoryBrowserProps) {
|
|||
</button>
|
||||
{browser.roots.length > 0 ? (
|
||||
<select
|
||||
aria-label="Drive"
|
||||
value={browser.selectedRootPath}
|
||||
aria-label="Location"
|
||||
// When on home, no drive root matches → the bare select shows "Home" as the
|
||||
// current value. On a drive, selectedRootPath is the drive; picking "Home"
|
||||
// (value="~") routes back to ~.
|
||||
value={browser.browsePath === "~" ? "~" : browser.selectedRootPath}
|
||||
onChange={(event) => {
|
||||
const nextPath = event.target.value;
|
||||
if (nextPath) void browser.browse(nextPath, { selectedPath: nextPath });
|
||||
if (!nextPath) return;
|
||||
if (nextPath === "~") void browser.browse("~");
|
||||
else void browser.browse(nextPath);
|
||||
}}
|
||||
className="add-project-modal__drive-select"
|
||||
>
|
||||
<option value="">Drive</option>
|
||||
<option value="~">Home</option>
|
||||
{browser.roots.map((root) => (
|
||||
<option key={root.path} value={root.path}>
|
||||
{root.label}
|
||||
|
|
|
|||
|
|
@ -129,7 +129,7 @@ describe("AddProjectModal", () => {
|
|||
|
||||
render(<AddProjectModal open onClose={vi.fn()} />);
|
||||
|
||||
const driveSelect = await screen.findByLabelText(/drive/i);
|
||||
const driveSelect = await screen.findByLabelText(/location/i);
|
||||
fireEvent.change(driveSelect, { target: { value: "D:\\" } });
|
||||
|
||||
await waitFor(() =>
|
||||
|
|
@ -137,7 +137,11 @@ describe("AddProjectModal", () => {
|
|||
`/api/filesystem/browse?path=${encodeURIComponent("D:\\")}`,
|
||||
),
|
||||
);
|
||||
await waitFor(() => expect(screen.getAllByText("D:\\").length).toBeGreaterThan(0));
|
||||
// Drive switch navigates but no longer auto-selects (selection is an explicit user
|
||||
// action — see useDirectoryBrowser). The location input is the canonical "where we are".
|
||||
await waitFor(() =>
|
||||
expect((screen.getByLabelText(/folder path/i) as HTMLInputElement).value).toBe("D:\\"),
|
||||
);
|
||||
expect(await screen.findByRole("button", { name: /projects/i })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -281,6 +281,110 @@ describe("DirectoryBrowser", () => {
|
|||
expect(browser.reset).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not auto-select the descended folder on double-click", async () => {
|
||||
const fetchMock = vi
|
||||
.fn()
|
||||
// initial reset → home
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
json: async () => ({
|
||||
entries: [{ name: "workspace", isDirectory: true, isGitRepo: false, hasLocalConfig: false }],
|
||||
roots: [],
|
||||
}),
|
||||
})
|
||||
// descend into workspace (not a git repo)
|
||||
.mockResolvedValueOnce({
|
||||
ok: true,
|
||||
json: async () => ({
|
||||
entries: [],
|
||||
current: { isGitRepo: false, hasLocalConfig: false },
|
||||
roots: [],
|
||||
}),
|
||||
});
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
|
||||
render(<Harness />);
|
||||
|
||||
const row = (await screen.findByText("workspace")).closest("button");
|
||||
expect(row).not.toBeNull();
|
||||
fireEvent.doubleClick(row!);
|
||||
|
||||
// After descending, the current-folder row exists but must NOT be selected — the
|
||||
// user navigated, they didn't pick. Otherwise the modal flashes a red
|
||||
// "not a git repository" warning for every non-repo folder they pass through.
|
||||
await waitFor(() => {
|
||||
const current = screen.queryByRole("button", { name: "workspace, current folder" });
|
||||
expect(current).not.toBeNull();
|
||||
expect(current?.className).not.toContain("is-selected");
|
||||
});
|
||||
});
|
||||
|
||||
it("offers Home in the location dropdown and browses back to ~ when picked", () => {
|
||||
const browser = {
|
||||
browsePath: "C:\\Users",
|
||||
selectedBrowsePath: "",
|
||||
setSelectedBrowsePath: vi.fn(),
|
||||
directoryEntries: [],
|
||||
currentDirectory: null,
|
||||
roots: [
|
||||
{ label: "C:", path: "C:\\" },
|
||||
{ label: "D:", path: "D:\\" },
|
||||
],
|
||||
selectedRootPath: "C:\\",
|
||||
locationInput: "C:\\Users",
|
||||
setLocationInput: vi.fn(),
|
||||
loading: false,
|
||||
error: null,
|
||||
parentPath: "C:\\",
|
||||
canGoBack: true,
|
||||
canGoForward: false,
|
||||
browse: vi.fn(),
|
||||
goBack: vi.fn(),
|
||||
goForward: vi.fn(),
|
||||
goUp: vi.fn(),
|
||||
refresh: vi.fn(),
|
||||
reset: vi.fn(),
|
||||
} satisfies UseDirectoryBrowser;
|
||||
|
||||
render(<DirectoryBrowser browser={browser} />);
|
||||
|
||||
const select = screen.getByLabelText("Location") as HTMLSelectElement;
|
||||
// Home is the first option so it acts as the route back to ~ from any drive.
|
||||
expect(Array.from(select.options).map((o) => o.value)).toEqual(["~", "C:\\", "D:\\"]);
|
||||
fireEvent.change(select, { target: { value: "~" } });
|
||||
|
||||
expect(browser.browse).toHaveBeenCalledWith("~");
|
||||
});
|
||||
|
||||
it("shows Home as the selected location when browsePath is ~", () => {
|
||||
const browser = {
|
||||
browsePath: "~",
|
||||
selectedBrowsePath: "",
|
||||
setSelectedBrowsePath: vi.fn(),
|
||||
directoryEntries: [],
|
||||
currentDirectory: null,
|
||||
roots: [{ label: "C:", path: "C:\\" }],
|
||||
selectedRootPath: "",
|
||||
locationInput: "~",
|
||||
setLocationInput: vi.fn(),
|
||||
loading: false,
|
||||
error: null,
|
||||
parentPath: null,
|
||||
canGoBack: false,
|
||||
canGoForward: false,
|
||||
browse: vi.fn(),
|
||||
goBack: vi.fn(),
|
||||
goForward: vi.fn(),
|
||||
goUp: vi.fn(),
|
||||
refresh: vi.fn(),
|
||||
reset: vi.fn(),
|
||||
} satisfies UseDirectoryBrowser;
|
||||
|
||||
render(<DirectoryBrowser browser={browser} />);
|
||||
|
||||
expect((screen.getByLabelText("Location") as HTMLSelectElement).value).toBe("~");
|
||||
});
|
||||
|
||||
it("handles keyboard navigation when focus is on an ancestor container", async () => {
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
|
|
|
|||
|
|
@ -74,7 +74,9 @@ describe("useDirectoryBrowser", () => {
|
|||
|
||||
expect(result.current.browsePath).toBe("~");
|
||||
expect(result.current.locationInput).toBe("~");
|
||||
expect(result.current.selectedBrowsePath).toBe("~");
|
||||
// History-replay navigation no longer carries a selection — selecting is an explicit
|
||||
// user action (click a row, type a path) so descend/back/forward leave selection clear.
|
||||
expect(result.current.selectedBrowsePath).toBe("");
|
||||
expect(result.current.canGoForward).toBe(true);
|
||||
act(() => result.current.goForward());
|
||||
await waitFor(() => expect(result.current.browsePath).toBe("~/a"));
|
||||
|
|
|
|||
|
|
@ -82,7 +82,10 @@ export function useDirectoryBrowser(): UseDirectoryBrowser {
|
|||
setBrowseEntries([]);
|
||||
setCurrentDirectory(null);
|
||||
setRoots([]);
|
||||
setSelectedBrowsePath(selectedPath ?? path);
|
||||
// Navigating to `path` (descend, breadcrumb, drive switch) must not auto-select it —
|
||||
// a selection should only ever come from explicit user intent. Callers that DO want
|
||||
// to seed a selection (refresh, reset, typed-path Enter) pass `selectedPath` explicitly.
|
||||
setSelectedBrowsePath(selectedPath ?? "");
|
||||
}, []);
|
||||
|
||||
const browse = useCallback(
|
||||
|
|
@ -116,7 +119,7 @@ export function useDirectoryBrowser(): UseDirectoryBrowser {
|
|||
const mode = options?.mode ?? "push";
|
||||
setBrowsePath(path);
|
||||
setLocationInput(path);
|
||||
setSelectedBrowsePath(options?.selectedPath ?? path);
|
||||
setSelectedBrowsePath(options?.selectedPath ?? "");
|
||||
setBrowseEntries(body?.entries ?? []);
|
||||
setCurrentDirectory(body?.current ?? null);
|
||||
setRoots(body?.roots ?? []);
|
||||
|
|
|
|||
Loading…
Reference in New Issue