feat(inbox): move the selection with the arrow keys (MUL-5622) (#6269)

Up/Down inside the inbox list scrolled the container instead of walking the
selection, so a notification could only be opened with the mouse.

The list's scroll container now owns the arrow keys: it moves the selection
by one row, scrolls the new row into view through Virtuoso (never the DOM's
scrollIntoView, which also scrolls ancestors), and claims the keypress so the
native scroll cannot pull the viewport off the selected row. Keyboard focus
is parked on the container rather than a row, because virtualization unmounts
rows as they scroll out. Scoping the handler to the container keeps Down from
swapping the row out while the user is reading the issue detail.

Co-authored-by: Bohan-J <bohan@devv.ai>
Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Bohan Jiang
2026-08-03 00:17:26 +08:00
committed by GitHub
parent 37f3bb7dd9
commit 9b013e34e8
2 changed files with 273 additions and 6 deletions

View File

@@ -0,0 +1,194 @@
import { forwardRef, useImperativeHandle } from "react";
import { fireEvent, render, screen } from "@testing-library/react";
import { beforeEach, describe, expect, it, vi } from "vitest";
import type { InboxItem } from "@multica/core/types";
import { InboxList } from "./inbox-list";
// jsdom has no layout, so the real Virtuoso measures a 0-height viewport and
// renders nothing. The mock renders every row inline and exposes the handle
// methods the list drives, so the keyboard behaviour is observable.
const scrollIntoView = vi.hoisted(() => vi.fn());
vi.mock("react-virtuoso", () => ({
Virtuoso: forwardRef(function MockVirtuoso(
{
data,
itemContent,
}: {
data: InboxItem[];
itemContent: (index: number, item: InboxItem) => React.ReactNode;
},
ref: React.Ref<unknown>,
) {
useImperativeHandle(ref, () => ({ scrollIntoView }));
return (
<div>
{data.map((item, index) => (
<div key={item.id}>{itemContent(index, item)}</div>
))}
</div>
);
}),
}));
// The row renders avatars, hover cards, and a status icon — none of which this
// file is about. Keep it a bare button carrying the two things the list reads.
vi.mock("./inbox-list-item", () => ({
InboxListItem: ({
item,
isSelected,
onClick,
}: {
item: InboxItem;
isSelected: boolean;
onClick: () => void;
}) => (
<button type="button" data-selected={isSelected} onClick={onClick}>
{item.id}
</button>
),
}));
vi.mock("../../i18n", () => ({ useT: () => ({ t: () => "Inbox" }) }));
function item(id: string, overrides: Partial<InboxItem> = {}): InboxItem {
return {
id,
workspace_id: "workspace-1",
recipient_type: "member",
recipient_id: "member-1",
actor_type: "agent",
actor_id: "agent-1",
type: "new_comment",
severity: "info",
issue_id: `issue-${id}`,
title: "Issue title",
body: null,
issue_status: null,
read: true,
archived: false,
created_at: "2026-06-15T08:00:00Z",
details: null,
...overrides,
};
}
const items = [item("a"), item("b"), item("c")];
function renderList(selectedKey: string, onSelect = vi.fn()) {
const utils = render(
<InboxList
items={items}
view="inbox"
selectedKey={selectedKey}
archivedCount={0}
onSelect={onSelect}
onAction={vi.fn()}
onOpenArchived={vi.fn()}
/>,
);
// The scroll container owns the key handler; it is the row's closest
// ancestor with an overflow style.
const scroller = utils.container.querySelector(".overflow-y-auto") as HTMLElement;
return { ...utils, scroller, onSelect };
}
/** Press a key on the list, reporting whether the native scroll was claimed. */
function press(scroller: HTMLElement, key: string, init: KeyboardEventInit = {}) {
return !fireEvent.keyDown(scroller, { key, ...init });
}
beforeEach(() => {
scrollIntoView.mockClear();
});
describe("InboxList keyboard navigation", () => {
it("moves the selection down instead of scrolling", () => {
const { scroller, onSelect } = renderList("issue-a");
const prevented = press(scroller, "ArrowDown");
expect(onSelect).toHaveBeenCalledWith(items[1]);
expect(prevented).toBe(true);
});
it("moves the selection up", () => {
const { scroller, onSelect } = renderList("issue-b");
press(scroller, "ArrowUp");
expect(onSelect).toHaveBeenCalledWith(items[0]);
});
it("enters the list from either end when nothing is selected", () => {
const down = renderList("");
press(down.scroller, "ArrowDown");
expect(down.onSelect).toHaveBeenCalledWith(items[0]);
const up = renderList("");
press(up.scroller, "ArrowUp");
expect(up.onSelect).toHaveBeenCalledWith(items[2]);
});
it("stops at the ends of the list and still claims the key", () => {
// Falling through to the native scroll at the last row would move the
// viewport away from the row that stays selected.
const { scroller, onSelect } = renderList("issue-c");
const prevented = press(scroller, "ArrowDown");
expect(onSelect).not.toHaveBeenCalled();
expect(prevented).toBe(true);
});
it("scrolls the newly selected row into view", () => {
// Virtuoso's own scrollIntoView, so a row that virtualization has not
// mounted still gets there — and only when it is off-screen.
const { scroller } = renderList("issue-a");
press(scroller, "ArrowDown");
expect(scrollIntoView).toHaveBeenCalledWith({ index: 1 });
});
it("leaves modified arrow keys alone", () => {
// Shift+Down extends a selection and Alt/Cmd+Down are OS/browser scroll
// accelerators; none of them mean "next notification".
const { scroller, onSelect } = renderList("issue-a");
expect(press(scroller, "ArrowDown", { shiftKey: true })).toBe(false);
expect(press(scroller, "ArrowDown", { metaKey: true })).toBe(false);
expect(press(scroller, "ArrowDown", { altKey: true })).toBe(false);
expect(onSelect).not.toHaveBeenCalled();
});
it("leaves the key to a text field inside the list", () => {
const { scroller, onSelect } = renderList("issue-a");
const input = document.createElement("input");
scroller.appendChild(input);
press(input, "ArrowDown");
expect(onSelect).not.toHaveBeenCalled();
});
it("ignores an arrow key that composition already owns", () => {
// During IME composition the arrow key walks the candidate list.
const { scroller, onSelect } = renderList("issue-a");
press(scroller, "ArrowDown", { keyCode: 229 });
expect(onSelect).not.toHaveBeenCalled();
});
it("focuses the container on click so the arrow keys work right after", () => {
// Safari does not focus a <button> on click, and virtualization can unmount
// the clicked row — either way the keydown would stop reaching the list.
const { scroller, onSelect } = renderList("");
fireEvent.click(screen.getByText("b"));
expect(onSelect).toHaveBeenCalledWith(items[1]);
expect(document.activeElement).toBe(scroller);
});
});

View File

@@ -1,8 +1,10 @@
"use client";
import { useCallback, useMemo, useState } from "react";
import { Virtuoso } from "react-virtuoso";
import { useCallback, useMemo, useRef, useState, type KeyboardEvent } from "react";
import { Virtuoso, type VirtuosoHandle } from "react-virtuoso";
import { Archive, ChevronRight, Inbox } from "lucide-react";
import { isEditableShortcutTarget } from "@multica/core/shortcuts";
import { isImeComposing } from "@multica/core/utils";
import type { InboxItem } from "@multica/core/types";
import type { InboxView } from "./inbox-view";
import { InboxListItem } from "./inbox-list-item";
@@ -27,8 +29,8 @@ import { useT } from "../../i18n";
*
* Known virtualization tradeoff: keyboard Tab only reaches currently-mounted
* rows; a keyboard-only user must scroll to bring off-screen rows into the
* tab order. The inbox has no custom arrow-key list navigation, so the
* practical surface is small, but it is called out for the manual pass.
* tab order. Arrow-key navigation (below) covers off-screen rows, because it
* walks the data rather than the DOM.
*/
export function InboxList({
items,
@@ -54,8 +56,70 @@ export function InboxList({
// A callback ref into state hands the element over once it mounts and
// triggers the re-render that lets Virtuoso attach to it.
const [scrollEl, setScrollEl] = useState<HTMLDivElement | null>(null);
const virtuosoRef = useRef<VirtuosoHandle>(null);
const isArchivedView = view === "archived";
// Keyboard focus for the list lives on the scroll container, not on a row:
// virtualization unmounts the row the user clicked as soon as it scrolls
// out, and focus falling back to <body> would silently stop the next arrow
// key from reaching this handler. `preventScroll` because focusing a box
// scrolls every scrollable ancestor to reveal it, which on desktop shoves
// the shell around (#3929).
const focusList = useCallback(() => {
scrollEl?.focus({ preventScroll: true });
}, [scrollEl]);
const selectItem = useCallback(
(item: InboxItem) => {
// Safari does not focus a <button> on click, so the container has to be
// focused explicitly or the arrow keys would stay dead after a click.
focusList();
onSelect(item);
},
[focusList, onSelect],
);
// Arrow keys move the selection instead of scrolling the container — what
// every mail-style list does (MUL-5622). Bound to the scroll container
// rather than the document so it only fires while focus is inside the list:
// pressing Down while reading the issue detail must not swap the row out
// from under the reader.
const handleKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
if (event.key !== "ArrowDown" && event.key !== "ArrowUp") return;
// A menu, editor, or text field that already acted on the arrow key owns
// it; so does an IME candidate list mid-composition.
if (event.defaultPrevented || isImeComposing(event)) return;
if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) return;
if (isEditableShortcutTarget(event.target)) return;
// Claim the key even at the ends of the list: falling through to the
// native scroll there would move the viewport away from the selected row.
event.preventDefault();
focusList();
const current = items.findIndex(
(item) => (item.issue_id ?? item.id) === selectedKey,
);
const step = event.key === "ArrowDown" ? 1 : -1;
// Nothing selected yet: Down enters the list at the top, Up at the bottom.
const nextIndex =
current < 0
? step === 1
? 0
: items.length - 1
: Math.min(Math.max(current + step, 0), items.length - 1);
if (nextIndex === current) return;
const nextItem = items[nextIndex];
if (!nextItem) return;
// Virtuoso's own scrollIntoView, never the DOM element's: the target row
// may not be mounted, and the native call scrolls ancestors too. It is a
// no-op while the row is already fully visible, so a selection moving
// inside the viewport does not scroll the list.
virtuosoRef.current?.scrollIntoView({ index: nextIndex });
onSelect(nextItem);
};
// The entry into the archive sits below the last row and scrolls with the
// list (same placement as chat's). Virtuoso mounts it via `components.Footer`,
// and swaps the component whenever that prop's identity changes — so both the
@@ -110,7 +174,7 @@ export function InboxList({
item={item}
view={view}
isSelected={(item.issue_id ?? item.id) === selectedKey}
onClick={() => onSelect(item)}
onClick={() => selectItem(item)}
onAction={() => onAction(item.id)}
/>
);
@@ -120,10 +184,19 @@ export function InboxList({
// never paints blank; once it's set, mount the Virtuoso with a matching
// `initialItemCount` so the measurement frame keeps those rows (MUL-4750).
return (
<div ref={setScrollEl} className="flex-1 min-h-0 overflow-y-auto">
<div
ref={setScrollEl}
// Programmatically focusable only: the rows are buttons and already
// carry their own tab stops, so a tabbable container would just add a
// redundant one.
tabIndex={-1}
onKeyDown={handleKeyDown}
className="flex-1 min-h-0 overflow-y-auto outline-none"
>
<div className="px-2 py-1">
{scrollEl ? (
<Virtuoso
ref={virtuosoRef}
customScrollParent={scrollEl}
data={items}
computeItemKey={computeItemKey}