From 8388f161865134f53d55b8e4c4c3cf5296d515de Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 07:04:35 +0200 Subject: [PATCH] Keep Add Project keyboard selection visible (#7081) * fix(app): scroll the Add Project results to the keyboard-selected row The results list had no scroll ref, so ArrowUp/ArrowDown moved the selection off-screen; the autocomplete reveal math is now shared through useRevealActiveItem. * test(app): verify the directory selected beyond the results viewport Check scrolling in both directions and across wrapping, then confirm Enter opens the selected directory. The previous regression used an aria-selected attribute that result buttons do not render. Co-authored-by: superheher * test(app): verify scroll reveal through browser behavior Keep pure offset checks and cover keyboard navigation, wrapping, and replacing search results in the real Add Project flow. * fix(app): measure retained Add Project rows when revealing selection * fix(app): measure scroll content through a native View reference --------- Co-authored-by: superheher Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- .../app/e2e/browser/add-project-flow.spec.ts | 114 ++++++++++++++++- .../e2e/support/helpers/add-project-flow.ts | 23 ++++ .../app/src/components/add-project-flow.tsx | 119 +++++++++++------- .../components/ui/autocomplete-utils.test.ts | 30 +---- .../src/components/ui/autocomplete-utils.ts | 25 ---- .../app/src/components/ui/autocomplete.tsx | 4 +- .../src/components/ui/scroll-reveal.test.ts | 48 +++++++ .../app/src/components/ui/scroll-reveal.ts | 110 ++++++++++++++++ 8 files changed, 372 insertions(+), 101 deletions(-) create mode 100644 packages/app/src/components/ui/scroll-reveal.test.ts create mode 100644 packages/app/src/components/ui/scroll-reveal.ts diff --git a/packages/app/e2e/browser/add-project-flow.spec.ts b/packages/app/e2e/browser/add-project-flow.spec.ts index 9fd7f9413d..f66b963d5f 100644 --- a/packages/app/e2e/browser/add-project-flow.spec.ts +++ b/packages/app/e2e/browser/add-project-flow.spec.ts @@ -1,6 +1,6 @@ import { randomUUID } from "node:crypto"; -import { mkdtemp, rm, stat } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { mkdir, mkdtemp, rm, stat } from "node:fs/promises"; +import { homedir, tmpdir } from "node:os"; import path from "node:path"; import { test, expect } from "../support/fixtures"; import { @@ -12,12 +12,15 @@ import { chooseAddProjectMethod, expectAddProjectPage, expectNewWorkspaceForAddedProject, + expectAddProjectResultVisible, + navigateAddProjectResults, openAddProjectFlow, openAddProjectHostSelection, } from "../support/helpers/add-project-flow"; import { gotoAppShell } from "../support/helpers/app"; import { addConnectedHostAndReload, + addConnectedHostsAndReload, addOfflineHostAndReload, waitForConnectedHost, } from "../support/helpers/hosts"; @@ -214,6 +217,60 @@ test.describe("Add Project command-center flow", () => { }); }); + test("filtered hosts keep the keyboard-selected row visible", async ({ page }) => { + const hosts: IsolatedHostDaemon[] = []; + try { + for (let index = 1; index <= 3; index += 1) { + hosts.push(await startIsolatedHostDaemon(`scroll-host-${index}`)); + } + await page.setViewportSize({ width: 1280, height: 360 }); + await gotoAppShell(page); + await addConnectedHostsAndReload( + page, + hosts.map((host, index) => ({ + serverId: host.serverId, + port: host.port, + label: index < 2 ? `Fleet 0${index + 1}` : "Other host", + })), + { primaryLabel: "Fleet 00" }, + ); + for (const host of hosts) { + await waitForConnectedHost(page, { + serverId: host.serverId, + endpoint: `localhost:${host.port}`, + }); + } + await openAddProjectHostSelection(page); + await expect( + addProjectFlow(page).getByRole("button").filter({ hasText: "Other host" }), + ).toBeVisible(); + + await addProjectFlowInput(page).fill("Fleet"); + await expect(addProjectFlowHost(page, hosts[2]!.serverId)).toHaveCount(0); + await navigateAddProjectResults(page, "ArrowDown", 2); + await expectAddProjectResultVisible(page, "Fleet 02"); + + // Filtering out the first host moves retained rows to new positions. + await addProjectFlowInput(page).fill("scroll-host"); + await expect(addProjectFlowHost(page, getServerId())).toHaveCount(0); + await expectAddProjectResultVisible(page, "Fleet 01"); + await navigateAddProjectResults(page, "ArrowDown", 2); + await expectAddProjectResultVisible(page, "Other host"); + await navigateAddProjectResults(page, "ArrowDown", 1); + await expectAddProjectResultVisible(page, "Fleet 01"); + + await addProjectFlowInput(page).fill("Fleet"); + await expectAddProjectResultVisible(page, "Fleet 00"); + await navigateAddProjectResults(page, "ArrowDown", 2); + await expectAddProjectResultVisible(page, "Fleet 02"); + await page.keyboard.press("Enter"); + await expectAddProjectPage(page, "method"); + await expect(addProjectFlow(page)).toContainText("Fleet 02"); + } finally { + await Promise.all(hosts.map((host) => host.close())); + } + }); + test("keyboard directory search adds the selected Project", async ({ page, projectPickerFixture, @@ -241,6 +298,59 @@ test.describe("Add Project command-center flow", () => { await expectProjectHasNoWorkspaces(projectId); }); + test("keyboard navigation keeps the selected row inside the visible results", async ({ + page, + }) => { + const parentDirectory = await mkdtemp(path.join(homedir(), "paseo-e2e-scroll-")); + const rowPath = (index: number) => + path.join(parentDirectory, `scroll-probe-${String(index).padStart(2, "0")}`); + let projectId: string | null = null; + try { + await Promise.all(Array.from({ length: 16 }, (_, index) => mkdir(rowPath(index)))); + await gotoAppShell(page); + await openAddProjectFlow(page); + await chooseAddProjectMethod(page, "directory-search"); + await addProjectFlowInput(page).fill(path.basename(parentDirectory)); + const results = page.getByTestId("add-project-flow-results"); + await expect(results.getByRole("button", { name: /scroll-probe-/ })).toHaveCount(16, { + timeout: 30_000, + }); + + await navigateAddProjectResults(page, "ArrowDown", 12); + await expectAddProjectResultVisible(page, rowPath(11)); + await navigateAddProjectResults(page, "ArrowUp", 12); + await expectAddProjectResultVisible(page, parentDirectory); + await navigateAddProjectResults(page, "ArrowUp", 1); + await expectAddProjectResultVisible(page, rowPath(15)); + await navigateAddProjectResults(page, "ArrowDown", 1); + await expectAddProjectResultVisible(page, parentDirectory); + + await navigateAddProjectResults(page, "ArrowDown", 12); + await expectAddProjectResultVisible(page, rowPath(11)); + await addProjectFlowInput(page).fill(`${path.basename(parentDirectory)}probe`); + await expect(results.getByRole("button")).toHaveCount(16); + await expectAddProjectResultVisible(page, rowPath(0)); + await navigateAddProjectResults(page, "ArrowDown", 12); + await expectAddProjectResultVisible(page, rowPath(12)); + await addProjectFlowInput(page).fill(path.basename(parentDirectory)); + await expect(results.getByRole("button")).toHaveCount(17); + await expectAddProjectResultVisible(page, parentDirectory); + await navigateAddProjectResults(page, "ArrowDown", 12); + await expectAddProjectResultVisible(page, rowPath(11)); + await page.keyboard.press("Enter"); + projectId = await expectOpenedProject(page, "scroll-probe-11"); + await expectNewWorkspaceForAddedProject(page, { + serverId: getServerId(), + projectId, + projectName: "scroll-probe-11", + projectPath: rowPath(11), + }); + } finally { + if (projectId) await removeCreatedProject(rowPath(11), projectId); + await rm(parentDirectory, { recursive: true, force: true }); + } + }); + test("a complete repository URL remains selectable without a GitHub search result", async ({ page, }) => { diff --git a/packages/app/e2e/support/helpers/add-project-flow.ts b/packages/app/e2e/support/helpers/add-project-flow.ts index bb88264d37..666469540e 100644 --- a/packages/app/e2e/support/helpers/add-project-flow.ts +++ b/packages/app/e2e/support/helpers/add-project-flow.ts @@ -90,3 +90,26 @@ export async function expectNewWorkspaceForAddedProject( { timeout: 30_000 }, ); } + +export async function navigateAddProjectResults( + page: Page, + key: "ArrowDown" | "ArrowUp", + count: number, +): Promise { + for (let index = 0; index < count; index += 1) await page.keyboard.press(key); +} + +export async function expectAddProjectResultVisible(page: Page, text: string): Promise { + const results = page.getByTestId("add-project-flow-results"); + const row = results.getByRole("button").filter({ has: page.getByText(text, { exact: true }) }); + await expect + .poll(async () => { + const viewport = await results.boundingBox(); + const bounds = await row.boundingBox(); + if (!viewport || !bounds) return false; + return ( + bounds.y >= viewport.y - 1 && bounds.y + bounds.height <= viewport.y + viewport.height + 1 + ); + }) + .toBe(true); +} diff --git a/packages/app/src/components/add-project-flow.tsx b/packages/app/src/components/add-project-flow.tsx index d874a73618..c331f237c5 100644 --- a/packages/app/src/components/add-project-flow.tsx +++ b/packages/app/src/components/add-project-flow.tsx @@ -19,6 +19,7 @@ import { useRef, useState, type ComponentType, + type RefObject, } from "react"; import { Modal, @@ -71,6 +72,7 @@ import { type ProjectPickerOption, } from "@/components/project-picker-options"; import { Shortcut } from "@/components/ui/shortcut"; +import { useRevealActiveItem } from "@/components/ui/scroll-reveal"; import { useKeyboardShortcutsAvailable } from "@/keyboard/availability"; import { getIsElectronRuntime } from "@/constants/layout"; import { isNative, isWeb } from "@/constants/platform"; @@ -252,7 +254,17 @@ function pathTestId(path: string): string { return `add-project-flow-path-${encodeURIComponent(path)}`; } -function FlowRow({ option, active }: { option: FlowRowOption; active: boolean }) { +function FlowRow({ + option, + active, + activeItemRef, + onRowLayout, +}: { + option: FlowRowOption; + active: boolean; + activeItemRef: RefObject; + onRowLayout: () => void; +}) { const accessibilityState = useMemo( () => ({ disabled: option.disabled === true, selected: active }), [active, option.disabled], @@ -270,6 +282,8 @@ function FlowRow({ option, active }: { option: FlowRowOption; active: boolean }) accessibilityRole="button" accessibilityState={accessibilityState} disabled={option.disabled} + ref={active ? activeItemRef : undefined} + onLayout={active ? onRowLayout : undefined} onPress={option.select} style={rowStyle} testID={option.testID} @@ -384,6 +398,7 @@ export function AddProjectFlow({ request, onClose }: AddProjectFlowProps) { ); const setHasHydratedWorkspaces = useSessionStore((store) => store.setHasHydratedWorkspaces); const inputRef = useRef(null); + const resultsRef = useRef(null); const submissionInFlightRef = useRef(false); const browseInFlightRef = useRef(false); const query = page.kind === "new-directory-name" || page.kind === "method" ? "" : page.query; @@ -715,6 +730,13 @@ export function AddProjectFlow({ request, onClose }: AddProjectFlowProps) { ]); const activeIndex = rows.length === 0 ? 0 : Math.min(page.activeIndex, rows.length - 1); + const rowListKey = useMemo(() => rows.map((row) => row.id).join("\u0000"), [rows]); + const { activeItemRef, contentRef, onItemLayout, onViewportLayout, onScroll } = + useRevealActiveItem({ + activeIndex, + listKey: rowListKey, + scrollRef: resultsRef, + }); const createDirectory = useCallback(async () => { if (page.kind !== "new-directory-name" || !client) return; const name = page.name.trim(); @@ -905,53 +927,64 @@ export function AddProjectFlow({ request, onClose }: AddProjectFlowProps) { ) : null} - {preview ? ( - - {shortenPath(preview)} - - ) : null} - {isSubmitting ? ( - - {progressText(page)} - - ) : null} - {!isSubmitting && page.error ? ( - - {page.error} - - ) : null} - {!isSubmitting && queryError ? ( - - {queryError} - - ) : null} - {!isSubmitting && loading ? ( - - Loading... - - ) : null} - {!isSubmitting && - (!loading || page.kind === "github-search") && - (!queryError || page.kind === "github-search") - ? rows.map((option, index) => ( - - )) - : null} - {!isSubmitting && - !loading && - !queryError && - rows.length === 0 && - page.kind !== "new-directory-name" ? ( - - {emptyText(page, host ?? null)} - - ) : null} + + {preview ? ( + + {shortenPath(preview)} + + ) : null} + {isSubmitting ? ( + + {progressText(page)} + + ) : null} + {!isSubmitting && page.error ? ( + + {page.error} + + ) : null} + {!isSubmitting && queryError ? ( + + {queryError} + + ) : null} + {!isSubmitting && loading ? ( + + Loading... + + ) : null} + {!isSubmitting && + (!loading || page.kind === "github-search") && + (!queryError || page.kind === "github-search") + ? rows.map((option, index) => ( + + )) + : null} + {!isSubmitting && + !loading && + !queryError && + rows.length === 0 && + page.kind !== "new-directory-name" ? ( + + {emptyText(page, host ?? null)} + + ) : null} + diff --git a/packages/app/src/components/ui/autocomplete-utils.test.ts b/packages/app/src/components/ui/autocomplete-utils.test.ts index dd67c23b57..ecf046a8aa 100644 --- a/packages/app/src/components/ui/autocomplete-utils.test.ts +++ b/packages/app/src/components/ui/autocomplete-utils.test.ts @@ -1,10 +1,6 @@ import { describe, expect, it } from "vitest"; -import { - getAutocompleteFallbackIndex, - getAutocompleteScrollOffset, - orderAutocompleteOptions, -} from "./autocomplete-utils"; +import { getAutocompleteFallbackIndex, orderAutocompleteOptions } from "./autocomplete-utils"; const OPTIONS = ["alpha", "beta", "gamma"]; @@ -28,27 +24,3 @@ describe("getAutocompleteFallbackIndex", () => { expect(getAutocompleteFallbackIndex(3, "below-input")).toBe(0); }); }); - -describe("getAutocompleteScrollOffset", () => { - it("scrolls up when the active item is above the viewport", () => { - expect( - getAutocompleteScrollOffset({ - currentOffset: 120, - viewportHeight: 80, - itemTop: 90, - itemHeight: 20, - }), - ).toBe(90); - }); - - it("scrolls down when the active item is below the viewport", () => { - expect( - getAutocompleteScrollOffset({ - currentOffset: 0, - viewportHeight: 100, - itemTop: 150, - itemHeight: 24, - }), - ).toBe(74); - }); -}); diff --git a/packages/app/src/components/ui/autocomplete-utils.ts b/packages/app/src/components/ui/autocomplete-utils.ts index f8250856df..175cf6b62d 100644 --- a/packages/app/src/components/ui/autocomplete-utils.ts +++ b/packages/app/src/components/ui/autocomplete-utils.ts @@ -29,28 +29,3 @@ export function getAutocompleteNextIndex(args: { }): number { return getNextActiveIndex(args); } - -export function getAutocompleteScrollOffset(args: { - currentOffset: number; - viewportHeight: number; - itemTop: number; - itemHeight: number; -}): number { - if (args.viewportHeight <= 0) { - return args.currentOffset; - } - - const itemBottom = args.itemTop + args.itemHeight; - const viewportTop = args.currentOffset; - const viewportBottom = args.currentOffset + args.viewportHeight; - - if (args.itemTop < viewportTop) { - return Math.max(0, args.itemTop); - } - - if (itemBottom > viewportBottom) { - return Math.max(0, itemBottom - args.viewportHeight); - } - - return args.currentOffset; -} diff --git a/packages/app/src/components/ui/autocomplete.tsx b/packages/app/src/components/ui/autocomplete.tsx index cc684f8b41..7b5b99b316 100644 --- a/packages/app/src/components/ui/autocomplete.tsx +++ b/packages/app/src/components/ui/autocomplete.tsx @@ -13,7 +13,7 @@ import { StyleSheet, useUnistyles } from "react-native-unistyles"; import { useTranslation } from "react-i18next"; import { File, Folder } from "lucide-react-native"; import type { Theme } from "@/styles/theme"; -import { getAutocompleteScrollOffset } from "./autocomplete-utils"; +import { getScrollOffsetToRevealItem } from "./scroll-reveal"; export interface AutocompleteOption { id: string; @@ -146,7 +146,7 @@ export function Autocomplete({ return; } - const nextOffset = getAutocompleteScrollOffset({ + const nextOffset = getScrollOffsetToRevealItem({ currentOffset: scrollOffsetRef.current, viewportHeight: viewportHeightRef.current, itemTop: layout.top, diff --git a/packages/app/src/components/ui/scroll-reveal.test.ts b/packages/app/src/components/ui/scroll-reveal.test.ts new file mode 100644 index 0000000000..b793b2f46b --- /dev/null +++ b/packages/app/src/components/ui/scroll-reveal.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { getScrollOffsetToRevealItem } from "./scroll-reveal"; + +describe("getScrollOffsetToRevealItem", () => { + it("keeps the offset when the item is already inside the viewport", () => { + expect( + getScrollOffsetToRevealItem({ + currentOffset: 40, + viewportHeight: 200, + itemTop: 100, + itemHeight: 40, + }), + ).toBe(40); + }); + + it("scrolls up to the item top when the item is above the viewport", () => { + expect( + getScrollOffsetToRevealItem({ + currentOffset: 300, + viewportHeight: 200, + itemTop: 240, + itemHeight: 40, + }), + ).toBe(240); + }); + + it("scrolls down just far enough to reveal the item bottom", () => { + expect( + getScrollOffsetToRevealItem({ + currentOffset: 0, + viewportHeight: 200, + itemTop: 380, + itemHeight: 40, + }), + ).toBe(220); + }); + + it("stays put before the viewport has been measured", () => { + expect( + getScrollOffsetToRevealItem({ + currentOffset: 120, + viewportHeight: 0, + itemTop: 400, + itemHeight: 40, + }), + ).toBe(120); + }); +}); diff --git a/packages/app/src/components/ui/scroll-reveal.ts b/packages/app/src/components/ui/scroll-reveal.ts new file mode 100644 index 0000000000..50194c3ac8 --- /dev/null +++ b/packages/app/src/components/ui/scroll-reveal.ts @@ -0,0 +1,110 @@ +import { useCallback, useLayoutEffect, useRef, type RefObject } from "react"; +import type { + LayoutChangeEvent, + NativeScrollEvent, + NativeSyntheticEvent, + ScrollView, + View, +} from "react-native"; + +export function getScrollOffsetToRevealItem(args: { + currentOffset: number; + viewportHeight: number; + itemTop: number; + itemHeight: number; +}): number { + if (args.viewportHeight <= 0) { + return args.currentOffset; + } + + const itemBottom = args.itemTop + args.itemHeight; + const viewportTop = args.currentOffset; + const viewportBottom = args.currentOffset + args.viewportHeight; + + if (args.itemTop < viewportTop) { + return Math.max(0, args.itemTop); + } + + if (itemBottom > viewportBottom) { + return Math.max(0, itemBottom - args.viewportHeight); + } + + return args.currentOffset; +} + +export interface RevealActiveItemHandlers { + activeItemRef: RefObject; + contentRef: RefObject; + onItemLayout: () => void; + onViewportLayout: (event: LayoutChangeEvent) => void; + onScroll: (event: NativeSyntheticEvent) => void; +} + +/** + * Reveals the keyboard-selected row using its current position in the scroll + * content. Measuring on selection/list changes also covers retained rows that + * move or keep their size without emitting another onLayout event on web. + * A list replacement resets to the top only when selection restarts there. + * Bottom-anchored lists share only getScrollOffsetToRevealItem. + */ +export function useRevealActiveItem(options: { + activeIndex: number; + listKey: string; + scrollRef: RefObject; +}): RevealActiveItemHandlers { + const { activeIndex, listKey, scrollRef } = options; + const activeItemRef = useRef(null); + const contentRef = useRef(null); + const viewportHeightRef = useRef(0); + const offsetRef = useRef(0); + const measurementRef = useRef(0); + + const reveal = useCallback(() => { + const request = ++measurementRef.current; + const item = activeItemRef.current; + const scroll = scrollRef.current; + const content = contentRef.current; + if (!item || !scroll || !content) return; + + item.measureLayout(content, (_left, top, _width, height) => { + if (request !== measurementRef.current || activeItemRef.current !== item) return; + const nextOffset = getScrollOffsetToRevealItem({ + currentOffset: offsetRef.current, + viewportHeight: viewportHeightRef.current, + itemTop: top, + itemHeight: height, + }); + if (Math.abs(nextOffset - offsetRef.current) < 1) return; + offsetRef.current = nextOffset; + scroll.scrollTo({ y: nextOffset, animated: false }); + }); + }, [scrollRef]); + + const prevListKeyRef = useRef(listKey); + useLayoutEffect(() => { + if (prevListKeyRef.current !== listKey && activeIndex === 0) { + offsetRef.current = 0; + scrollRef.current?.scrollTo({ y: 0, animated: false }); + } + prevListKeyRef.current = listKey; + reveal(); + return () => { + // Measurements are asynchronous; a previous selection must not scroll us. + measurementRef.current += 1; + }; + }, [activeIndex, listKey, reveal, scrollRef]); + + const onViewportLayout = useCallback( + (event: LayoutChangeEvent) => { + viewportHeightRef.current = event.nativeEvent.layout.height; + reveal(); + }, + [reveal], + ); + + const onScroll = useCallback((event: NativeSyntheticEvent) => { + offsetRef.current = event.nativeEvent.contentOffset.y; + }, []); + + return { activeItemRef, contentRef, onItemLayout: reveal, onViewportLayout, onScroll }; +}