fix(ui): keep inbox hover and j/k keyboard selection in sync across list reshapes (#9680)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Inbox is the primary triage surface; it supports both mouse hover and `j`/`k` keyboard navigation over a flat, expandable list of rows > - Hover and keyboard selection are meant to be a single shared "cursor": hovering a row and then pressing `j`/`k` should continue from the hovered row, not jump elsewhere > - A prior hover-perf rewrite moved the hovered row into a numeric `hoveredIndexRef` and nulled it whenever the list reshaped; because the inbox polls constantly, any refresh between hovering and pressing a key dropped the hovered index > - With the hovered index dropped, the next keypress fell back to selection index `0`, stranding the cursor at the top of the list instead of continuing from the hovered row > - This pull request tracks the hovered row's stable identity (a nav key) alongside the numeric index and re-anchors it by key across reshapes, mirroring the existing keyboard-selection reconciliation > - The benefit is that mouse hover and keyboard navigation stay in sync exactly as intended, even while the inbox is polling ## Linked Issues or Issue Description **Bug report** - **What happened:** In the Inbox, hovering a row with the mouse and then pressing `j`/`k` (or another keyboard shortcut) does not continue from the hovered row. If the list refreshes (which happens on the inbox's constant polling) in the moment between hovering and pressing a key, keyboard selection snaps back to the top row instead. - **Expected behavior:** The keyboard cursor should be in sync with the hovered row — pressing `j`/`k` after hovering should move relative to the row the mouse is over. - **Steps to reproduce:** 1. Open the Inbox with several rows. 2. Hover the mouse over a row partway down the list. 3. Wait for (or trigger) a background poll/refresh of the list. 4. Press `j` or `k`. 5. Observe selection jumps to the top of the list instead of continuing from the hovered row. - **Root cause:** The `[flatNavItems]` effect nulled `hoveredIndexRef` on every list reshape. Since hover also clears the keyboard selection band to `-1`, the fallback selection index resolved to `0`. ## What Changed - Hoisted `navEntryKey` to module scope so it can compute a stable, index-independent identity for a nav row from both the hover handler and the reshape effect. - Added `hoveredNavKeyRef` to track the hovered row's stable key alongside the existing numeric `hoveredIndexRef`, set whenever the pointer selects a row. - On list reshape, re-anchor the hovered index by key (find the row with the same key) instead of unconditionally dropping it; only drop the hover when the row is actually gone. This mirrors the existing `selectedIndex` key-based reconciliation. - Added a unit test that hovers a row, reshapes the list via a simulated poll, presses `j`, and asserts selection continues from the hovered row. ## Verification - `pnpm --filter ./ui exec vitest run src/pages/Inbox.test.tsx` → 15/15 passing, including the new hover→`j`/`k` sync test and the existing keyboard-nav tests. - The new test explicitly covers the reshape-during-hover path that the prior test suite had deferred to live/e2e verification. ## Risks - Low risk. The change is confined to the Inbox's in-memory hover/selection bookkeeping (two refs and one effect); it adds no new renders (hover still paints via CSS `:hover`) and touches no data fetching, routing, or persistence. Behavior is unchanged when the list does not reshape; when it does, the hover now follows the same row instead of being dropped. ## Model Used - Claude Opus 4.8 (Anthropic), model id `claude-opus-4-8`, used with extended thinking and tool use (file edits, running the UI test suite locally). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
263316609e
commit
054b076b58
|
|
@ -107,8 +107,9 @@ vi.mock("../context/SidebarContext", () => ({
|
|||
useSidebar: () => ({ isMobile: false }),
|
||||
}));
|
||||
|
||||
const generalSettingsMock = { keyboardShortcutsEnabled: false };
|
||||
vi.mock("../context/GeneralSettingsContext", () => ({
|
||||
useGeneralSettings: () => ({ keyboardShortcutsEnabled: false }),
|
||||
useGeneralSettings: () => generalSettingsMock,
|
||||
}));
|
||||
|
||||
vi.mock("../hooks/useInboxBadge", () => ({
|
||||
|
|
@ -500,6 +501,72 @@ describe("Inbox toolbar", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("keeps hover→j/k selection in sync after the list reshapes (PAP-9679)", async () => {
|
||||
routerMock.location.pathname = "/inbox/mine";
|
||||
generalSettingsMock.keyboardShortcutsEnabled = true;
|
||||
const issueA = createIssue({ id: "issue-a", identifier: "PAP-2001", title: "Sync row A" });
|
||||
const issueB = createIssue({ id: "issue-b", identifier: "PAP-2002", title: "Sync row B" });
|
||||
const issueC = createIssue({ id: "issue-c", identifier: "PAP-2003", title: "Sync row C" });
|
||||
apiMocks.issuesList.mockResolvedValue([issueA, issueB, issueC]);
|
||||
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: { queries: { retry: false, staleTime: 0, gcTime: 0 } },
|
||||
});
|
||||
const root = createRoot(container);
|
||||
|
||||
const linkOf = (row: Element): HTMLAnchorElement | null =>
|
||||
row.querySelector("a[data-inbox-issue-link]");
|
||||
// The keyboard-selected row swaps to `hover:bg-transparent`; find its index.
|
||||
const selectedRowIndex = () =>
|
||||
[...container.querySelectorAll("[data-inbox-item]")].findIndex((row) =>
|
||||
linkOf(row)?.className.includes("hover:bg-transparent"),
|
||||
);
|
||||
|
||||
try {
|
||||
await act(async () => {
|
||||
root.render(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<Inbox />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
await vi.waitFor(() => {
|
||||
expect(container.querySelectorAll("[data-inbox-item]").length).toBeGreaterThanOrEqual(3);
|
||||
});
|
||||
|
||||
// Pointer physically moves, then hovers the middle row (index 1).
|
||||
await act(async () => {
|
||||
window.dispatchEvent(new MouseEvent("mousemove", { bubbles: true }));
|
||||
const rows = container.querySelectorAll("[data-inbox-item]");
|
||||
rows[1]!.dispatchEvent(new MouseEvent("mouseover", { bubbles: true }));
|
||||
rows[1]!.dispatchEvent(new MouseEvent("mouseenter", { bubbles: false }));
|
||||
});
|
||||
|
||||
// A poll reshapes the list (row B's title changes → new nav array) before
|
||||
// the keypress. This is what used to null the hovered index and strand
|
||||
// j/k back at the top.
|
||||
apiMocks.issuesList.mockResolvedValue([issueA, { ...issueB, title: "Sync row B (updated)" }, issueC]);
|
||||
await act(async () => {
|
||||
await queryClient.invalidateQueries();
|
||||
});
|
||||
await vi.waitFor(() => {
|
||||
expect(container.textContent).toContain("Sync row B (updated)");
|
||||
});
|
||||
|
||||
// j must continue from the hovered row (index 1) → index 2, not jump to
|
||||
// the top of the list.
|
||||
await act(async () => {
|
||||
document.body.dispatchEvent(new KeyboardEvent("keydown", { key: "j", bubbles: true }));
|
||||
});
|
||||
expect(selectedRowIndex()).toBe(2);
|
||||
} finally {
|
||||
generalSettingsMock.keyboardShortcutsEnabled = false;
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps other issue archive controls enabled while one archive is pending", async () => {
|
||||
routerMock.location.pathname = "/inbox/mine";
|
||||
const issueA = createIssue({ id: "issue-a", identifier: "PAP-1001", title: "First inbox row" });
|
||||
|
|
|
|||
|
|
@ -181,6 +181,17 @@ type SectionKey =
|
|||
|
||||
/** A flat navigation entry for keyboard j/k traversal that includes expanded children. */
|
||||
type NavEntry = InboxKeyboardNavEntry;
|
||||
// Stable identity for a nav row, resilient to the numeric index shifting when
|
||||
// the inbox reshapes (archive/poll). Used to re-anchor both the keyboard
|
||||
// selection and the hovered row across list refreshes.
|
||||
const navEntryKey = (entry: NavEntry | undefined): string | null =>
|
||||
!entry
|
||||
? null
|
||||
: entry.type === "top"
|
||||
? `top:${entry.itemKey}`
|
||||
: entry.type === "child"
|
||||
? `child:${entry.issueId}`
|
||||
: `group:${entry.groupKey}`;
|
||||
type CreatorOption = {
|
||||
id: string;
|
||||
label: string;
|
||||
|
|
@ -1405,6 +1416,10 @@ export function Inbox() {
|
|||
const flatNavItems = useMemo((): NavEntry[] => {
|
||||
return buildInboxKeyboardNavEntries(groupedSections, collapsedGroupKeys, collapsedInboxParents);
|
||||
}, [collapsedGroupKeys, collapsedInboxParents, groupedSections]);
|
||||
// Read the current nav list from event handlers without recreating them (and
|
||||
// without capturing a stale array), so hover can resolve the row's key.
|
||||
const flatNavItemsRef = useRef(flatNavItems);
|
||||
flatNavItemsRef.current = flatNavItems;
|
||||
// Roll live descendant runs up to their ancestors across the loaded inbox tree
|
||||
// so a parent that is not itself live can still surface "n live below".
|
||||
const subtreeLiveCounts = useMemo(() => {
|
||||
|
|
@ -1614,9 +1629,14 @@ export function Inbox() {
|
|||
// list costs zero re-renders (hover paints via CSS `:hover`, see IssueRow).
|
||||
// Keyboard nav reads this to continue from the hovered row.
|
||||
const hoveredIndexRef = useRef<number | null>(null);
|
||||
// The hovered row's stable key, kept alongside the numeric index so a poll
|
||||
// that reshapes the list can re-anchor the hover to the same row instead of
|
||||
// dropping it (which stranded j/k back at the top — PAP-9679 regression).
|
||||
const hoveredNavKeyRef = useRef<string | null>(null);
|
||||
const setSelectedIndexFromPointer = useCallback((idx: number) => {
|
||||
if (!pointerMovedSinceKeyNavRef.current) return;
|
||||
hoveredIndexRef.current = idx;
|
||||
hoveredNavKeyRef.current = navEntryKey(flatNavItemsRef.current[idx]);
|
||||
// Drop any keyboard selection band the moment the mouse takes over, so we
|
||||
// never show two identical highlights at once. React bails out when the
|
||||
// value is already -1, so continuous hovering triggers no re-render.
|
||||
|
|
@ -1786,19 +1806,16 @@ export function Inbox() {
|
|||
// numeric index onto a neighboring row (and Enter would open the wrong
|
||||
// task). Falls back to clamping when the item is gone; never auto-selects
|
||||
// on initial load.
|
||||
const navEntryKey = (entry: NavEntry | undefined): string | null =>
|
||||
!entry
|
||||
? null
|
||||
: entry.type === "top"
|
||||
? `top:${entry.itemKey}`
|
||||
: entry.type === "child"
|
||||
? `child:${entry.issueId}`
|
||||
: `group:${entry.groupKey}`;
|
||||
const selectedNavKeyRef = useRef<string | null>(null);
|
||||
useEffect(() => {
|
||||
// A reshaped list invalidates the numeric hover index; drop it so the next
|
||||
// keypress falls back to the (key-reconciled) keyboard selection.
|
||||
hoveredIndexRef.current = null;
|
||||
// A reshaped list invalidates the numeric hover index. Re-anchor it to the
|
||||
// same row by key (the inbox polls constantly, so nulling it here silently
|
||||
// broke hover→j/k sync — PAP-9679). Drop it only when the row is gone.
|
||||
const hoveredKey = hoveredNavKeyRef.current;
|
||||
const nextHovered =
|
||||
hoveredKey === null ? -1 : flatNavItems.findIndex((entry) => navEntryKey(entry) === hoveredKey);
|
||||
hoveredIndexRef.current = nextHovered >= 0 ? nextHovered : null;
|
||||
if (nextHovered < 0) hoveredNavKeyRef.current = null;
|
||||
setSelectedIndex((prev) => {
|
||||
if (prev < 0) return resolveInboxSelectionIndex(prev, flatNavItems.length);
|
||||
const prevKey = selectedNavKeyRef.current;
|
||||
|
|
|
|||
Loading…
Reference in New Issue