From e53fd038a890c7956893e73c2fdcae830030e54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 23 Jul 2026 14:45:49 +0100 Subject: [PATCH] fix(windows): list logical drives in folder picker (#1870) --- .github/workflows/backend-tests.yml | 7 +- .../task/handlers/repository_handlers.go | 7 +- .../task/handlers/repository_handlers_test.go | 25 +++ .../task/service/directory_listing.go | 22 ++- .../task/service/directory_listing_test.go | 19 ++ .../service/directory_listing_windows_test.go | 33 ++++ .../internal/task/service/directory_roots.go | 13 ++ .../task/service/directory_roots_other.go | 7 + .../task/service/directory_roots_windows.go | 24 +++ apps/web/components/folder-picker.test.tsx | 163 ++++++++++++++++++ apps/web/components/folder-picker.tsx | 65 +++++-- apps/web/lib/api/domains/fs-api.ts | 1 + 12 files changed, 360 insertions(+), 26 deletions(-) create mode 100644 apps/backend/internal/task/service/directory_listing_windows_test.go create mode 100644 apps/backend/internal/task/service/directory_roots.go create mode 100644 apps/backend/internal/task/service/directory_roots_other.go create mode 100644 apps/backend/internal/task/service/directory_roots_windows.go create mode 100644 apps/web/components/folder-picker.test.tsx diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 6d63e0a2ff..a2dd6da7c9 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -255,5 +255,8 @@ jobs: # Keep this targeted: the service package has broad subprocess coverage, # while this native test is the Windows path-semantics regression. - - name: Test explicit local repository paths - run: go test -race -v -run '^TestExplicitLocalRepositoryWindowsPathSemantics$' ./internal/task/service + - name: Test Windows path semantics + run: >- + go test -race -v + -run '^(TestExplicitLocalRepositoryWindowsPathSemantics|TestListDirectory.*|TestDriveRootsFromMask|TestSplitAbsForRoot)$' + ./internal/task/service diff --git a/apps/backend/internal/task/handlers/repository_handlers.go b/apps/backend/internal/task/handlers/repository_handlers.go index 03bbbe41ac..2ac007dcca 100644 --- a/apps/backend/internal/task/handlers/repository_handlers.go +++ b/apps/backend/internal/task/handlers/repository_handlers.go @@ -159,9 +159,10 @@ func (h *RepositoryHandlers) httpListDirectory(c *gin.Context) { entries = append(entries, gin.H{"name": e.Name, "path": e.Path}) } c.JSON(http.StatusOK, gin.H{ - "path": result.Path, - "parent": result.Parent, - "entries": entries, + "path": result.Path, + "parent": result.Parent, + "entries": entries, + "choosable": result.Choosable, }) } diff --git a/apps/backend/internal/task/handlers/repository_handlers_test.go b/apps/backend/internal/task/handlers/repository_handlers_test.go index 6e82205451..25468b554f 100644 --- a/apps/backend/internal/task/handlers/repository_handlers_test.go +++ b/apps/backend/internal/task/handlers/repository_handlers_test.go @@ -91,6 +91,31 @@ func TestHTTPListRepositoryBranchesUsesRepositoryIdentity(t *testing.T) { } } +func TestHTTPListDirectoryIncludesChoosableContract(t *testing.T) { + router, _ := newRepositoryHTTPTestRouter(t) + request := httptest.NewRequest( + http.MethodGet, + "/api/v1/fs/list-dir?path="+url.QueryEscape(t.TempDir()), + nil, + ) + response := httptest.NewRecorder() + + router.ServeHTTP(response, request) + + if response.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body = %s", response.Code, http.StatusOK, response.Body.String()) + } + var body struct { + Choosable *bool `json:"choosable"` + } + if err := json.Unmarshal(response.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body.Choosable == nil || !*body.Choosable { + t.Fatalf("choosable = %v, want true; body = %s", body.Choosable, response.Body.String()) + } +} + func TestHTTPListBranchesRejectsRepositoryFromAnotherWorkspace(t *testing.T) { router, repo, svc := newRepositoryHTTPTestRouterWithService(t) for _, workspaceID := range []string{"ws-a", "ws-b"} { diff --git a/apps/backend/internal/task/service/directory_listing.go b/apps/backend/internal/task/service/directory_listing.go index fe259cc5f6..19d3660fa6 100644 --- a/apps/backend/internal/task/service/directory_listing.go +++ b/apps/backend/internal/task/service/directory_listing.go @@ -16,12 +16,13 @@ type DirectoryEntry struct { } // DirectoryListing is the result of ListDirectory: the absolute path that was -// listed, the parent (empty when at the filesystem root), and the immediate -// subdirectory children sorted alphabetically. +// listed, whether it may be selected, the parent (empty when at the filesystem +// root), and the immediate subdirectory children sorted alphabetically. type DirectoryListing struct { - Path string - Parent string - Entries []DirectoryEntry + Path string + Parent string + Entries []DirectoryEntry + Choosable bool } // ListDirectory returns the immediate subdirectories of path. When path is @@ -41,6 +42,10 @@ type DirectoryListing struct { // // Hidden (".") directories are excluded. func (s *Service) ListDirectory(ctx context.Context, path string) (DirectoryListing, error) { + if listing, handled, err := listVirtualDirectoryRoot(path); handled { + return listing, err + } + abs, err := resolveListingPath(path) if err != nil { return DirectoryListing{}, err @@ -53,9 +58,10 @@ func (s *Service) ListDirectory(ctx context.Context, path string) (DirectoryList _ = ctx return DirectoryListing{ - Path: abs, - Parent: parentPath(abs), - Entries: collectSubdirs(abs, entries), + Path: abs, + Parent: parentPath(abs), + Entries: collectSubdirs(abs, entries), + Choosable: true, }, nil } diff --git a/apps/backend/internal/task/service/directory_listing_test.go b/apps/backend/internal/task/service/directory_listing_test.go index 5bb1d0d35d..7f898417aa 100644 --- a/apps/backend/internal/task/service/directory_listing_test.go +++ b/apps/backend/internal/task/service/directory_listing_test.go @@ -49,6 +49,22 @@ func TestListDirectory_ListsImmediateSubdirsOnly(t *testing.T) { } } +func TestDriveRootsFromMask(t *testing.T) { + got := driveRootsFromMask((1 << 2) | (1 << 4)) + want := []DirectoryEntry{ + {Name: `C:\`, Path: `C:\`}, + {Name: `E:\`, Path: `E:\`}, + } + if len(got) != len(want) { + t.Fatalf("drive roots = %+v, want %+v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("drive root[%d] = %+v, want %+v", i, got[i], want[i]) + } + } +} + func TestListDirectory_DefaultsToHome(t *testing.T) { svc := &Service{} got, err := svc.ListDirectory(context.Background(), "") @@ -148,4 +164,7 @@ func TestListDirectory_ParentEmptyAtFilesystemRoot(t *testing.T) { if got.Parent != "" { t.Errorf("expected empty parent at %q, got %q", target, got.Parent) } + if !got.Choosable { + t.Errorf("filesystem root %q should be choosable", target) + } } diff --git a/apps/backend/internal/task/service/directory_listing_windows_test.go b/apps/backend/internal/task/service/directory_listing_windows_test.go new file mode 100644 index 0000000000..0762a19f9f --- /dev/null +++ b/apps/backend/internal/task/service/directory_listing_windows_test.go @@ -0,0 +1,33 @@ +//go:build windows + +package service + +import ( + "context" + "strings" + "testing" +) + +func TestListDirectoryWindowsVirtualRootListsLogicalDrives(t *testing.T) { + got, err := (&Service{}).ListDirectory(context.Background(), "/") + if err != nil { + t.Fatalf("ListDirectory virtual root: %v", err) + } + if got.Path != "/" { + t.Errorf("Path = %q, want virtual root /", got.Path) + } + if got.Parent != "" { + t.Errorf("Parent = %q, want empty", got.Parent) + } + if got.Choosable { + t.Error("Windows virtual root must not be choosable") + } + if len(got.Entries) == 0 { + t.Fatal("virtual root returned no logical drives") + } + for _, entry := range got.Entries { + if !strings.HasSuffix(entry.Path, `:\`) { + t.Errorf("drive path = %q, want native absolute drive root", entry.Path) + } + } +} diff --git a/apps/backend/internal/task/service/directory_roots.go b/apps/backend/internal/task/service/directory_roots.go new file mode 100644 index 0000000000..589c037d4d --- /dev/null +++ b/apps/backend/internal/task/service/directory_roots.go @@ -0,0 +1,13 @@ +package service + +func driveRootsFromMask(mask uint32) []DirectoryEntry { + roots := make([]DirectoryEntry, 0, 26) + for drive := 0; drive < 26; drive++ { + if mask&(1< void) | undefined; + +vi.mock("@kandev/ui/popover", () => ({ + Popover: ({ + children, + onOpenChange, + }: { + children: ReactNode; + onOpenChange: (open: boolean) => void; + }) => { + setPopoverOpen = onOpenChange; + return
{children}
; + }, + PopoverTrigger: ({ children }: { children: ReactElement<{ onClick?: () => void }> }) => + cloneElement(children, { onClick: () => setPopoverOpen?.(true) }), + PopoverContent: ({ children }: { children: ReactNode }) =>
{children}
, +})); + +vi.mock("@kandev/ui/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, +})); + +vi.mock("@/lib/api/domains/fs-api", async (importOriginal) => { + const original = await importOriginal(); + return { ...original, listDirectory: vi.fn() }; +}); + +const mockedListDirectory = vi.mocked(listDirectory); +const BUTTON_ROLE = "button"; +const TRIGGER_TEST_ID = "folder-picker-trigger"; +const VIRTUAL_ROOT = "/"; +const DRIVE_E_ROOT = "E:\\"; +const SUCCESS_PATH = "E:\\Success"; +const SUCCESS_NAME = "Success"; + +afterEach(() => { + cleanup(); + mockedListDirectory.mockReset(); + setPopoverOpen = undefined; +}); + +describe("FolderPicker", () => { + it("preserves the separator when displaying the POSIX root", () => { + render(); + + expect(screen.getByTestId(TRIGGER_TEST_ID).textContent).toBe("/"); + }); + + it("preserves the separator when displaying a Windows drive root", () => { + render(); + + expect(screen.getByTestId(TRIGGER_TEST_ID).textContent).toBe("C:\\"); + }); + + it("builds Windows breadcrumbs with a virtual root and native drive paths", async () => { + mockedListDirectory + .mockResolvedValueOnce( + listing(VIRTUAL_ROOT, false, [{ name: DRIVE_E_ROOT, path: DRIVE_E_ROOT }]), + ) + .mockResolvedValueOnce(listing("E:\\Projects\\Kandev", true)) + .mockResolvedValueOnce(listing(VIRTUAL_ROOT, false)); + + render(); + fireEvent.click(screen.getByTestId(TRIGGER_TEST_ID)); + fireEvent.click(await screen.findByRole(BUTTON_ROLE, { name: DRIVE_E_ROOT })); + + await screen.findByRole(BUTTON_ROLE, { name: "Kandev" }); + expect(screen.getByRole(BUTTON_ROLE, { name: VIRTUAL_ROOT })).toBeTruthy(); + expect(screen.getByRole(BUTTON_ROLE, { name: DRIVE_E_ROOT })).toBeTruthy(); + expect(screen.getByRole(BUTTON_ROLE, { name: "Projects" })).toBeTruthy(); + + fireEvent.click(screen.getByRole(BUTTON_ROLE, { name: VIRTUAL_ROOT })); + expect(mockedListDirectory).toHaveBeenLastCalledWith(VIRTUAL_ROOT); + }); + + it("clears a stale selectable listing when navigation fails", async () => { + mockedListDirectory + .mockResolvedValueOnce(listing("C:\\", true, [{ name: "Denied", path: "C:\\Denied" }])) + .mockRejectedValueOnce(new Error("failed to list directory")); + + render(); + fireEvent.click(screen.getByTestId(TRIGGER_TEST_ID)); + fireEvent.click(await screen.findByRole(BUTTON_ROLE, { name: "Denied" })); + + await screen.findByTestId("folder-picker-error"); + await waitFor(() => + expect(screen.getByTestId("folder-picker-choose").hasAttribute("disabled")).toBe(true), + ); + expect(screen.queryByRole(BUTTON_ROLE, { name: "C:\\" })).toBeNull(); + }); + + it("ignores an old success after a value change starts a successor load", async () => { + const oldLoad = deferred(); + const successorLoad = deferred(); + mockedListDirectory + .mockReturnValueOnce(oldLoad.promise) + .mockReturnValueOnce(successorLoad.promise); + + const { rerender } = render(); + fireEvent.click(screen.getByTestId(TRIGGER_TEST_ID)); + await waitFor(() => expect(mockedListDirectory).toHaveBeenCalledWith("")); + + rerender(); + await waitFor(() => expect(mockedListDirectory).toHaveBeenCalledWith(SUCCESS_PATH)); + await act(async () => successorLoad.resolve(listing(SUCCESS_PATH, true))); + await waitFor(() => + expect(screen.getAllByRole(BUTTON_ROLE, { name: SUCCESS_NAME })).toHaveLength(2), + ); + + await act(async () => oldLoad.resolve(listing("C:\\Stale", true))); + expect(screen.queryByRole(BUTTON_ROLE, { name: "Stale" })).toBeNull(); + expect(screen.getAllByRole(BUTTON_ROLE, { name: SUCCESS_NAME })).toHaveLength(2); + }); + + it("ignores an old error after close and reopen completes a successor load", async () => { + const oldLoad = deferred(); + const successorLoad = deferred(); + mockedListDirectory + .mockReturnValueOnce(oldLoad.promise) + .mockReturnValueOnce(successorLoad.promise); + + render(); + fireEvent.click(screen.getByTestId(TRIGGER_TEST_ID)); + await waitFor(() => expect(mockedListDirectory).toHaveBeenCalledTimes(1)); + + act(() => setPopoverOpen?.(false)); + act(() => setPopoverOpen?.(true)); + await waitFor(() => expect(mockedListDirectory).toHaveBeenCalledTimes(2)); + await act(async () => successorLoad.resolve(listing(SUCCESS_PATH, true))); + await screen.findByRole(BUTTON_ROLE, { name: SUCCESS_NAME }); + + await act(async () => oldLoad.reject(new Error("stale failure"))); + expect(screen.queryByTestId("folder-picker-error")).toBeNull(); + expect(screen.getByRole(BUTTON_ROLE, { name: SUCCESS_NAME })).toBeTruthy(); + expect(screen.getByTestId("folder-picker-choose").hasAttribute("disabled")).toBe(false); + }); +}); + +function listing( + path: string, + choosable: boolean, + entries: DirectoryListing["entries"] = [], +): DirectoryListing { + return { path, parent: "", entries, choosable }; +} + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} diff --git a/apps/web/components/folder-picker.tsx b/apps/web/components/folder-picker.tsx index b4b0f4c880..2e563f9a1b 100644 --- a/apps/web/components/folder-picker.tsx +++ b/apps/web/components/folder-picker.tsx @@ -1,6 +1,6 @@ "use client"; -import { Fragment, useCallback, useEffect, useState } from "react"; +import { Fragment, useCallback, useEffect, useRef, useState } from "react"; import { IconFolder, IconBox, IconChevronRight } from "@tabler/icons-react"; import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; @@ -59,7 +59,7 @@ export function FolderPicker({ value, onChange, placeholder }: FolderPickerProps )} void load(p)} />