mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-27 04:56:20 +02:00
fix(editor): index @mention selection by rendered order (MUL-3607)
The @mention popup navigated and committed by indexing the flat `displayItems` array, but rendered rows in the re-bucketed order from `groupItems()` (current → recent → search → users → issues). In chat (context mode) the async server-search results are appended to the end of `displayItems` yet tagged `group:"search"`, so `groupItems()` hoists them near the top. The highlighted row and the committed item then point at different entries — you select one target but mention its neighbour (the reported "@bohan picks the next one" off-by-one). Make the flattened grouped order (`orderedItems`) the single index space for `selectedIndex`, arrow keys, Enter, and clicks, so the highlighted row is always the committed item. Plain issue-comment mentions (default mode) were already safe — no group tags means `groupItems()` is order-preserving — and stay unchanged. Adds a regression test asserting highlighted row == committed item when the list is reordered by a hoisted search result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
import { render, screen, waitFor } from "@testing-library/react";
|
||||
import { act, render, screen, waitFor } from "@testing-library/react";
|
||||
import { createRef, type ReactNode } from "react";
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { workspaceKeys } from "@multica/core/workspace/queries";
|
||||
@@ -209,6 +209,61 @@ describe("createMentionSuggestion", () => {
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
// MUL-3607: groupItems() re-buckets the list (current → recent → search →
|
||||
// users → issues), so a server "search" result that arrives at the END of
|
||||
// the data array is rendered NEAR THE TOP. Keyboard nav, Enter, and clicks
|
||||
// must follow the rendered order — otherwise the highlighted row and the
|
||||
// committed item drift apart and you mention the neighbour of who you picked.
|
||||
// (The invariant is type-agnostic; issue rows are used here because they
|
||||
// render without workspace/avatar context. The chat @bohan case is the same
|
||||
// bug — a person sitting below a hoisted search result.)
|
||||
it("commits the highlighted row, not its neighbour, when groups reorder the list", () => {
|
||||
const command = vi.fn<(item: MentionItem) => void>();
|
||||
const ref = createRef<MentionListRef>();
|
||||
|
||||
// A plain issue (issues bucket) plus a server-style search hit (search
|
||||
// bucket). Data order is [MUL-2, MUL-1] but groupItems hoists the search
|
||||
// row, so the RENDERED order is [MUL-1, MUL-2].
|
||||
const items: MentionItem[] = [
|
||||
{ id: "i-plain", label: "MUL-2", type: "issue" },
|
||||
{ id: "i-search", label: "MUL-1", type: "issue", group: "search" },
|
||||
];
|
||||
|
||||
render(
|
||||
<I18nWrapper>
|
||||
<MentionList ref={ref} items={items} query="" command={command} includeProjectSearch />
|
||||
</I18nWrapper>,
|
||||
);
|
||||
|
||||
const highlightedLabel = () => {
|
||||
const buttons = Array.from(document.querySelectorAll<HTMLButtonElement>("button"));
|
||||
return buttons.find((b) => b.classList.contains("bg-accent"))?.textContent ?? "";
|
||||
};
|
||||
|
||||
const press = (key: string) =>
|
||||
act(() => {
|
||||
ref.current?.onKeyDown({ event: new KeyboardEvent("keydown", { key }) });
|
||||
});
|
||||
|
||||
// First rendered row is the hoisted search result. Enter must commit it,
|
||||
// not the issue that sits first in the data array.
|
||||
const firstHighlighted = highlightedLabel();
|
||||
expect(firstHighlighted).toBe("MUL-1");
|
||||
press("Enter");
|
||||
expect(command).toHaveBeenCalledTimes(1);
|
||||
expect(command.mock.calls[0]?.[0]?.label).toBe(firstHighlighted);
|
||||
|
||||
command.mockClear();
|
||||
|
||||
// Arrow down one row, then Enter — still commits exactly the highlighted row.
|
||||
press("ArrowDown");
|
||||
const secondHighlighted = highlightedLabel();
|
||||
expect(secondHighlighted).toBe("MUL-2");
|
||||
press("Enter");
|
||||
expect(command).toHaveBeenCalledTimes(1);
|
||||
expect(command.mock.calls[0]?.[0]?.label).toBe(secondHighlighted);
|
||||
});
|
||||
|
||||
it("hides personal agents owned by someone else from a regular member", () => {
|
||||
const qc = fakeQc({
|
||||
members: [
|
||||
|
||||
@@ -232,9 +232,24 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
|
||||
return mergeMentionItems(items, currentServerItems).slice(0, MAX_ITEMS);
|
||||
}, [items, normalizedQuery, searchedQuery, serverItems]);
|
||||
|
||||
// Keyboard navigation and selection must index the SAME order the popup
|
||||
// renders. groupItems() re-buckets displayItems (current → recent → search
|
||||
// → users → issues), so a later-in-data item — e.g. an async server
|
||||
// "search" result appended to the end of displayItems — is hoisted into an
|
||||
// earlier bucket. Indexing the pre-group displayItems then drifts from the
|
||||
// rendered rows: the highlighted row and the committed item point at
|
||||
// different entries (you mention the neighbour of who you picked).
|
||||
// orderedItems is the flattened render order and is the single index space
|
||||
// for selectedIndex, arrow keys, Enter, and clicks.
|
||||
const groups = useMemo(() => groupItems(displayItems), [displayItems]);
|
||||
const orderedItems = useMemo(
|
||||
() => groups.flatMap((group) => group.items),
|
||||
[groups],
|
||||
);
|
||||
|
||||
useEffect(() => {
|
||||
setSelectedIndex(0);
|
||||
}, [displayItems]);
|
||||
}, [orderedItems]);
|
||||
|
||||
useEffect(() => {
|
||||
itemRefs.current[selectedIndex]?.scrollIntoView({ block: "nearest" });
|
||||
@@ -242,13 +257,13 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
|
||||
|
||||
const selectItem = useCallback(
|
||||
(index: number) => {
|
||||
const item = displayItems[index];
|
||||
const item = orderedItems[index];
|
||||
if (!item) return;
|
||||
const wsId = getCurrentWsId();
|
||||
if (wsId) recordMentionUsage(wsId, item);
|
||||
command(item);
|
||||
},
|
||||
[displayItems, command],
|
||||
[orderedItems, command],
|
||||
);
|
||||
|
||||
useImperativeHandle(ref, () => ({
|
||||
@@ -257,19 +272,19 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
|
||||
// those keys belong to the IME (Enter commits composition, etc).
|
||||
if (isImeComposing(event)) return false;
|
||||
if (event.key === "ArrowUp") {
|
||||
if (displayItems.length === 0) return true;
|
||||
if (orderedItems.length === 0) return true;
|
||||
setSelectedIndex(
|
||||
(i) => (i + displayItems.length - 1) % displayItems.length,
|
||||
(i) => (i + orderedItems.length - 1) % orderedItems.length,
|
||||
);
|
||||
return true;
|
||||
}
|
||||
if (event.key === "ArrowDown") {
|
||||
if (displayItems.length === 0) return true;
|
||||
setSelectedIndex((i) => (i + 1) % displayItems.length);
|
||||
if (orderedItems.length === 0) return true;
|
||||
setSelectedIndex((i) => (i + 1) % orderedItems.length);
|
||||
return true;
|
||||
}
|
||||
if (event.key === "Enter") {
|
||||
if (displayItems.length === 0) return true;
|
||||
if (orderedItems.length === 0) return true;
|
||||
selectItem(selectedIndex);
|
||||
return true;
|
||||
}
|
||||
@@ -277,7 +292,7 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
|
||||
},
|
||||
}));
|
||||
|
||||
if (displayItems.length === 0) {
|
||||
if (orderedItems.length === 0) {
|
||||
const isWaitingForServer =
|
||||
normalizedQuery !== "" &&
|
||||
(isSearching || searchedQuery !== normalizedQuery);
|
||||
@@ -291,8 +306,7 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
|
||||
);
|
||||
}
|
||||
|
||||
const groups = groupItems(displayItems);
|
||||
const hasContextGroups = displayItems.some((item) => item.group === "current" || item.group === "recent");
|
||||
const hasContextGroups = orderedItems.some((item) => item.group === "current" || item.group === "recent");
|
||||
const contextLayout = hasContextGroups;
|
||||
const groupLabel = (label: string): string => {
|
||||
if (label === "Current") return t(($) => $.mention.group_current);
|
||||
|
||||
Reference in New Issue
Block a user