diff --git a/apps/code/src/renderer/features/inbox/components/InboxSignalsTab.tsx b/apps/code/src/renderer/features/inbox/components/InboxSignalsTab.tsx index 726e6f177..f391ecad0 100644 --- a/apps/code/src/renderer/features/inbox/components/InboxSignalsTab.tsx +++ b/apps/code/src/renderer/features/inbox/components/InboxSignalsTab.tsx @@ -14,6 +14,7 @@ import { } from "@features/inbox/hooks/useInboxBulkActions"; import { useInboxDeepLinkListSync } from "@features/inbox/hooks/useInboxDeepLinkListSync"; import { useInboxEngagementTracker } from "@features/inbox/hooks/useInboxEngagementTracker"; +import { useInboxKeyboardNavigation } from "@features/inbox/hooks/useInboxKeyboardNavigation"; import { useInboxAvailableSuggestedReviewers, useInboxReportsInfinite, @@ -235,9 +236,6 @@ export function InboxSignalsTab() { (s) => s.toggleReportSelection, ); const selectRange = useInboxReportSelectionStore((s) => s.selectRange); - const selectExactRange = useInboxReportSelectionStore( - (s) => s.selectExactRange, - ); const clearSelection = useInboxReportSelectionStore((s) => s.clearSelection); const [dismissReport, setDismissReport] = useState(null); @@ -577,52 +575,12 @@ export function InboxSignalsTab() { } }, [focusListPane, showTwoPaneLayout]); - // Tracks the cursor position for keyboard navigation (the "moving end" of - // Shift+Arrow selection). Separated from `lastClickedId` which acts as the - // anchor so that the anchor stays fixed while the cursor extends the range. - const keyboardCursorIdRef = useRef(null); + const { navigateReport } = useInboxKeyboardNavigation({ reports }); - const navigateReport = useCallback( + const handleNavigate = useCallback( (direction: 1 | -1, shift: boolean) => { - const list = reportsRef.current; - if (list.length === 0) return; - - // Determine cursor position — the item to navigate away from - const cursorId = - keyboardCursorIdRef.current ?? - (selectedReportIdsRef.current.length > 0 - ? selectedReportIdsRef.current[ - selectedReportIdsRef.current.length - 1 - ] - : null); - const cursorIndex = cursorId - ? list.findIndex((r) => r.id === cursorId) - : -1; - const nextIndex = - cursorIndex === -1 - ? 0 - : Math.max(0, Math.min(list.length - 1, cursorIndex + direction)); - const nextId = list[nextIndex].id; - - if (shift) { - // Anchor is the store's lastClickedId — the point where shift-selection started. - // selectExactRange replaces the selection with the exact range from anchor to cursor, - // so reversing direction correctly contracts the selection. - const anchor = - useInboxReportSelectionStore.getState().lastClickedId ?? nextId; - setPendingInboxOpenMethod("keyboard"); - selectExactRange( - anchor, - nextId, - list.map((r) => r.id), - ); - keyboardCursorIdRef.current = nextId; - } else { - setPendingInboxOpenMethod("keyboard"); - setSelectedReportIds([nextId]); - keyboardCursorIdRef.current = nextId; - } - + const nextId = navigateReport(direction, shift); + if (!nextId) return; const container = leftPaneRef.current; const row = container?.querySelector( `[data-report-id="${nextId}"]`, @@ -630,14 +588,12 @@ export function InboxSignalsTab() { const stickyHeader = container?.querySelector( "[data-inbox-sticky-header]", ); - if (!row) return; - const stickyHeaderHeight = stickyHeader?.offsetHeight ?? 0; row.style.scrollMarginTop = `${stickyHeaderHeight}px`; row.scrollIntoView({ block: "nearest" }); }, - [setSelectedReportIds, selectExactRange], + [navigateReport], ); // Window-level keyboard handler so arrow keys work regardless of which @@ -658,10 +614,10 @@ export function InboxSignalsTab() { if (e.key === "ArrowDown") { e.preventDefault(); - navigateReport(1, e.shiftKey); + handleNavigate(1, e.shiftKey); } else if (e.key === "ArrowUp") { e.preventDefault(); - navigateReport(-1, e.shiftKey); + handleNavigate(-1, e.shiftKey); } else if ( e.key === "Escape" && selectedReportIdsRef.current.length > 0 @@ -672,7 +628,7 @@ export function InboxSignalsTab() { }; window.addEventListener("keydown", handler); return () => window.removeEventListener("keydown", handler); - }, [navigateReport, clearSelection]); + }, [handleNavigate, clearSelection]); const searchDisabledReason = !hasReports && !searchQuery.trim() diff --git a/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.test.tsx b/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.test.tsx new file mode 100644 index 000000000..35cb1d8df --- /dev/null +++ b/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.test.tsx @@ -0,0 +1,494 @@ +import { useInboxReportSelectionStore } from "@features/inbox/stores/inboxReportSelectionStore"; +import type { SignalReport } from "@shared/types"; +import { + act, + fireEvent, + render, + renderHook, + screen, +} from "@testing-library/react"; +import { useEffect } from "react"; +import { beforeEach, describe, expect, it } from "vitest"; +import { useInboxKeyboardNavigation } from "./useInboxKeyboardNavigation"; + +function makeReport(id: string): SignalReport { + return { + id, + title: id, + summary: null, + status: "potential", + total_weight: 0, + signal_count: 0, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + artefact_count: 0, + }; +} + +const REPORTS: SignalReport[] = ["a", "b", "c", "d", "e"].map(makeReport); + +function getSelection() { + return useInboxReportSelectionStore.getState().selectedReportIds; +} + +function getLastClicked() { + return useInboxReportSelectionStore.getState().lastClickedId; +} + +/** Mimic InboxSignalsTab.handleReportClick — plain click. */ +function plainClick(id: string) { + act(() => { + useInboxReportSelectionStore.getState().setSelectedReportIds([id]); + }); +} + +/** Mimic InboxSignalsTab.handleReportClick — cmd-click. */ +function cmdClick(id: string) { + act(() => { + useInboxReportSelectionStore.getState().toggleReportSelection(id); + }); +} + +/** Mimic InboxSignalsTab.handleReportClick — shift-click. */ +function shiftClick(id: string, reports: SignalReport[] = REPORTS) { + act(() => { + useInboxReportSelectionStore.getState().selectRange( + id, + reports.map((r) => r.id), + ); + }); +} + +describe("useInboxKeyboardNavigation", () => { + beforeEach(() => { + useInboxReportSelectionStore.setState({ + selectedReportIds: [], + lastClickedId: null, + }); + }); + + describe("arrow navigation from an empty selection", () => { + it.each<[1 | -1, string]>([ + [1, "ArrowDown"], + [-1, "ArrowUp"], + ])("%s selects the first report when nothing is selected", (direction) => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + act(() => { + result.current.navigateReport(direction, false); + }); + + expect(getSelection()).toEqual(["a"]); + }); + + it("returns null when the list is empty", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: [] }), + ); + + let nextId: string | null = "unset"; + act(() => { + nextId = result.current.navigateReport(1, false); + }); + + expect(nextId).toBeNull(); + expect(getSelection()).toEqual([]); + }); + }); + + describe("arrow navigation after a click (regression for cursor drift)", () => { + it("ArrowDown after clicking a report selects the next report below it", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + // Walk the cursor down to "d" via the keyboard. + act(() => { + result.current.navigateReport(1, false); // a + result.current.navigateReport(1, false); // b + result.current.navigateReport(1, false); // c + result.current.navigateReport(1, false); // d + }); + expect(getSelection()).toEqual(["d"]); + + // Now click report "b" — this is the scenario from the bug report. + plainClick("b"); + expect(getSelection()).toEqual(["b"]); + + // ArrowDown should land on "c" (neighbour of the clicked report), + // NOT on "e" (where the keyboard cursor previously left off). + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["c"]); + }); + + it("ArrowUp after clicking a report selects the previous report above it", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + // Drift the keyboard cursor first. + act(() => { + result.current.navigateReport(1, false); // a + result.current.navigateReport(1, false); // b + }); + + // Click somewhere else. + plainClick("d"); + + act(() => { + result.current.navigateReport(-1, false); + }); + + expect(getSelection()).toEqual(["c"]); + }); + + it("ArrowDown after cmd-clicking a new report continues from the cmd-clicked id", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + // Keyboard-select "a". + act(() => { + result.current.navigateReport(1, false); + }); + expect(getSelection()).toEqual(["a"]); + + // Cmd-click "c" — extends the selection AND moves the click anchor to "c". + cmdClick("c"); + expect(getSelection()).toEqual(["a", "c"]); + expect(getLastClicked()).toBe("c"); + + // ArrowDown should navigate from "c" (the cmd-clicked id), not from "a". + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["d"]); + }); + + it("ArrowDown after shift-clicking continues from the shift-clicked id", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + // Plain-click "a" to set an anchor. + plainClick("a"); + + // Shift-click "c" — selects range a..c, anchor moves to c. + shiftClick("c"); + expect(getSelection()).toEqual(["a", "b", "c"]); + expect(getLastClicked()).toBe("c"); + + // ArrowDown should navigate from "c". + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["d"]); + }); + + it("ArrowDown after clearing selection starts from the top", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + act(() => { + result.current.navigateReport(1, false); // a + result.current.navigateReport(1, false); // b + }); + act(() => { + useInboxReportSelectionStore.getState().clearSelection(); + }); + + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["a"]); + }); + }); + + describe("arrow navigation bounds", () => { + it.each<[1 | -1, string]>([ + [1, "e"], + [-1, "a"], + ])( + "direction %i at the boundary stays on the same report", + (direction, reportId) => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick(reportId); + + act(() => { + result.current.navigateReport(direction, false); + }); + + expect(getSelection()).toEqual([reportId]); + }, + ); + }); + + describe("shift+arrow range extension", () => { + it("shift+ArrowDown after clicking extends a range from the clicked report", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick("b"); + + act(() => { + result.current.navigateReport(1, true); + }); + + expect(getSelection()).toEqual(["b", "c"]); + // Anchor stays put even as the cursor walks. + expect(getLastClicked()).toBe("b"); + }); + + it("shift+ArrowDown walks the cursor without disturbing the anchor", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick("b"); + + act(() => { + result.current.navigateReport(1, true); + result.current.navigateReport(1, true); + result.current.navigateReport(1, true); + }); + + expect(getSelection()).toEqual(["b", "c", "d", "e"]); + expect(getLastClicked()).toBe("b"); + }); + + it("shift+ArrowUp contracts a range when reversing direction", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick("b"); + + act(() => { + result.current.navigateReport(1, true); // b..c + result.current.navigateReport(1, true); // b..d + result.current.navigateReport(-1, true); // b..c + }); + + expect(getSelection()).toEqual(["b", "c"]); + }); + + it("plain arrow after shift+arrow restarts navigation from the cursor", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick("b"); + act(() => { + result.current.navigateReport(1, true); // b..c, cursor=c + result.current.navigateReport(1, true); // b..d, cursor=d + }); + expect(getSelection()).toEqual(["b", "c", "d"]); + + // Plain ArrowDown — selection collapses to the next item after the cursor. + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["e"]); + }); + + it("click after shift+arrow re-seats the cursor at the clicked report", () => { + const { result } = renderHook(() => + useInboxKeyboardNavigation({ reports: REPORTS }), + ); + + plainClick("b"); + act(() => { + result.current.navigateReport(1, true); // cursor=c + result.current.navigateReport(1, true); // cursor=d + }); + + // Click "a" — the cursor should re-seat there. + plainClick("a"); + + act(() => { + result.current.navigateReport(1, false); + }); + + expect(getSelection()).toEqual(["b"]); + }); + }); +}); + +/** + * Test harness that wires the hook the same way `InboxSignalsTab` does: + * a window-level keydown handler, and row click handlers that mirror the + * production plain/cmd/shift dispatch into the selection store. + * + * Lets us drive the real bug scenario via `fireEvent.click` + `fireEvent.keyDown`. + */ +function TestInbox({ reports }: { reports: SignalReport[] }) { + const { navigateReport } = useInboxKeyboardNavigation({ reports }); + const selectedIds = useInboxReportSelectionStore((s) => s.selectedReportIds); + + useEffect(() => { + const handler = (e: KeyboardEvent) => { + if (e.key === "ArrowDown") { + e.preventDefault(); + navigateReport(1, e.shiftKey); + } else if (e.key === "ArrowUp") { + e.preventDefault(); + navigateReport(-1, e.shiftKey); + } + }; + window.addEventListener("keydown", handler); + return () => window.removeEventListener("keydown", handler); + }, [navigateReport]); + + const handleClick = (id: string, e: React.MouseEvent) => { + const store = useInboxReportSelectionStore.getState(); + if (e.shiftKey) { + store.selectRange( + id, + reports.map((r) => r.id), + ); + } else if (e.metaKey) { + store.toggleReportSelection(id); + } else { + store.setSelectedReportIds([id]); + } + }; + + return ( +
    + {reports.map((r) => ( +
  • + +
    • + ))} +
+ ); +} + +function getSelectedTestIds() { + return Array.from( + document.querySelectorAll('[data-selected="true"]'), + ).map((el) => el.dataset.testid?.replace(/^report-/, "") ?? ""); +} + +describe("inbox keyboard navigation — full event pipeline", () => { + beforeEach(() => { + useInboxReportSelectionStore.setState({ + selectedReportIds: [], + lastClickedId: null, + }); + }); + + // The literal scenario from the bug report: click a report, hit ArrowDown, + // and it should select the report below — not the report below wherever the + // keyboard cursor previously was. + it("ArrowDown after clicking a report selects the report below it", () => { + render(); + + // Drift the keyboard cursor down to "d". + fireEvent.keyDown(window, { key: "ArrowDown" }); + fireEvent.keyDown(window, { key: "ArrowDown" }); + fireEvent.keyDown(window, { key: "ArrowDown" }); + fireEvent.keyDown(window, { key: "ArrowDown" }); + expect(getSelectedTestIds()).toEqual(["d"]); + + // Click "b". + fireEvent.click(screen.getByRole("button", { name: "b" })); + expect(getSelectedTestIds()).toEqual(["b"]); + + // ArrowDown should now select "c", not "e". + fireEvent.keyDown(window, { key: "ArrowDown" }); + + expect(getSelectedTestIds()).toEqual(["c"]); + }); + + it("ArrowUp after clicking a report selects the report above it", () => { + render(); + + fireEvent.keyDown(window, { key: "ArrowDown" }); + fireEvent.keyDown(window, { key: "ArrowDown" }); + expect(getSelectedTestIds()).toEqual(["b"]); + + fireEvent.click(screen.getByRole("button", { name: "d" })); + + fireEvent.keyDown(window, { key: "ArrowUp" }); + + expect(getSelectedTestIds()).toEqual(["c"]); + }); + + it("Shift+ArrowDown after clicking extends a range from the clicked report", () => { + render(); + + fireEvent.click(screen.getByRole("button", { name: "b" })); + fireEvent.keyDown(window, { key: "ArrowDown", shiftKey: true }); + fireEvent.keyDown(window, { key: "ArrowDown", shiftKey: true }); + + expect(getSelectedTestIds()).toEqual(["b", "c", "d"]); + }); + + it("Shift+ArrowUp contracts the range when direction reverses", () => { + render(); + + fireEvent.click(screen.getByRole("button", { name: "b" })); + fireEvent.keyDown(window, { key: "ArrowDown", shiftKey: true }); // b..c + fireEvent.keyDown(window, { key: "ArrowDown", shiftKey: true }); // b..d + fireEvent.keyDown(window, { key: "ArrowUp", shiftKey: true }); // b..c + + expect(getSelectedTestIds()).toEqual(["b", "c"]); + }); + + it("Cmd+click moves the keyboard cursor to the cmd-clicked report", () => { + render(); + + fireEvent.keyDown(window, { key: "ArrowDown" }); // a + fireEvent.click(screen.getByRole("button", { name: "c" }), { + metaKey: true, + }); + expect(getSelectedTestIds()).toEqual(["a", "c"]); + + fireEvent.keyDown(window, { key: "ArrowDown" }); + + expect(getSelectedTestIds()).toEqual(["d"]); + }); + + it("Shift+click moves the anchor to the shift-clicked report", () => { + render(); + + fireEvent.click(screen.getByRole("button", { name: "a" })); + fireEvent.click(screen.getByRole("button", { name: "c" }), { + shiftKey: true, + }); + expect(getSelectedTestIds()).toEqual(["a", "b", "c"]); + + fireEvent.keyDown(window, { key: "ArrowDown" }); + + expect(getSelectedTestIds()).toEqual(["d"]); + }); + + it("ArrowDown from an empty list does nothing", () => { + render(); + + fireEvent.keyDown(window, { key: "ArrowDown" }); + + expect(getSelectedTestIds()).toEqual([]); + }); +}); diff --git a/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.ts b/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.ts new file mode 100644 index 000000000..ae0d38421 --- /dev/null +++ b/apps/code/src/renderer/features/inbox/hooks/useInboxKeyboardNavigation.ts @@ -0,0 +1,92 @@ +import { useInboxReportSelectionStore } from "@features/inbox/stores/inboxReportSelectionStore"; +import { setPendingInboxOpenMethod } from "@features/inbox/utils/pendingInboxOpenMethod"; +import type { SignalReport } from "@shared/types"; +import { useCallback, useEffect, useRef } from "react"; + +interface UseInboxKeyboardNavigationArgs { + reports: SignalReport[]; +} + +interface UseInboxKeyboardNavigationResult { + /** Move the keyboard cursor up/down. Returns the id of the report that is now the cursor, or null when the list is empty. */ + navigateReport: (direction: 1 | -1, shift: boolean) => string | null; +} + +/** + * Keyboard navigation for the inbox signal list. + * + * Maintains the keyboard "cursor" — the moving end of arrow-key navigation — + * and syncs it to `lastClickedId` whenever the user clicks a report. Without + * that sync, the cursor would drift: after clicking report B the next arrow + * press would navigate from wherever the keyboard previously left off, not + * from B. + * + * Shift+Arrow deliberately walks the cursor without touching `lastClickedId`, + * so the anchor stays fixed while the selection extends. + */ +export function useInboxKeyboardNavigation({ + reports, +}: UseInboxKeyboardNavigationArgs): UseInboxKeyboardNavigationResult { + const reportsRef = useRef(reports); + reportsRef.current = reports; + + // The moving end of arrow-key navigation. Distinct from the store's + // `lastClickedId` (the anchor) so Shift+Arrow can extend a range while + // keeping the anchor fixed. + const keyboardCursorIdRef = useRef( + useInboxReportSelectionStore.getState().lastClickedId, + ); + + // Sync the cursor to `lastClickedId` so any click (plain, cmd, shift, or + // checkbox toggle) re-seats the keyboard cursor. Shift+Arrow leaves + // `lastClickedId` unchanged (it's the anchor), so the cursor walks freely + // in that one case. + useEffect(() => { + return useInboxReportSelectionStore.subscribe((state, prev) => { + if (state.lastClickedId !== prev.lastClickedId) { + keyboardCursorIdRef.current = state.lastClickedId; + } + }); + }, []); + + const navigateReport = useCallback( + (direction: 1 | -1, shift: boolean): string | null => { + const list = reportsRef.current; + if (list.length === 0) return null; + + const store = useInboxReportSelectionStore.getState(); + const cursorId = keyboardCursorIdRef.current; + const cursorIndex = cursorId + ? list.findIndex((r) => r.id === cursorId) + : -1; + const nextIndex = + cursorIndex === -1 + ? 0 + : Math.max(0, Math.min(list.length - 1, cursorIndex + direction)); + const nextId = list[nextIndex].id; + + if (shift) { + const anchor = store.lastClickedId ?? nextId; + setPendingInboxOpenMethod("keyboard"); + store.selectExactRange( + anchor, + nextId, + list.map((r) => r.id), + ); + // selectExactRange keeps lastClickedId as the anchor, so the + // subscription above won't fire — track the moving end ourselves. + keyboardCursorIdRef.current = nextId; + } else { + setPendingInboxOpenMethod("keyboard"); + store.setSelectedReportIds([nextId]); + // setSelectedReportIds with a single id updates lastClickedId, so the + // subscription will set keyboardCursorIdRef. No need to do it here. + } + + return nextId; + }, + [], + ); + + return { navigateReport }; +}