mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
fix(site/src/pages/AgentsPage/components/ChatMessageInput): scroll skills menu selection into view (#27345)
When navigating the slash skills menu with arrow keys, moving the highlight past the visible area did not scroll the list, so the selected skill went out of view. The menu drives cmdk in controlled mode while arrow keys are consumed by the Lexical trigger plugin, so cmdk's internal scroll-into-view never runs for keyboard navigation. This adds a layout effect in `SkillsTriggerMenu` that scrolls the highlighted item (and its group heading when it is the first item in a group) into view on keyboard-driven index changes, while skipping pointer-driven highlights to avoid hover/scroll loops, matching cmdk's own behavior. Covered by two new interaction stories that fail without the fix: `ScrollsSelectionIntoView` (menu in isolation) and `ArrowKeysScrollMenuList` (end-to-end arrow-key wrap in `ChatMessageInput`). > Opened by Mux, an AI coding agent, on Mike's behalf.
This commit is contained in:
@@ -6,6 +6,7 @@ import { COMPACT_SLASH_COMMAND } from "../../utils/slashCommands";
|
||||
import { ChatMessageInput } from "./ChatMessageInput";
|
||||
import type { SkillMetadata } from "./SkillsTriggerMenu";
|
||||
import {
|
||||
expectInsideListViewport,
|
||||
expectNoVisibleText,
|
||||
findVisibleText,
|
||||
MockSkill,
|
||||
@@ -147,6 +148,33 @@ export const ArrowKeysSelectHighlightedSkill: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
// Enough skills to overflow the menu's max height so arrow-key
|
||||
// navigation has to scroll the list.
|
||||
const manyPersonalSkills: TypesGen.UserSkillMetadata[] = Array.from(
|
||||
{ length: 15 },
|
||||
(_, index) => ({
|
||||
...MockSkill,
|
||||
id: `skill-scroll-${index}`,
|
||||
name: `skill-${String(index).padStart(2, "0")}`,
|
||||
}),
|
||||
);
|
||||
|
||||
export const ArrowKeysScrollMenuList: Story = {
|
||||
args: {
|
||||
personalSkillsOverride: manyPersonalSkills,
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
await typeInEditor(canvasElement, "/");
|
||||
const lastItem = await findVisibleText("/skill-14");
|
||||
// ArrowUp wraps the highlight to the last item, below the fold.
|
||||
await userEvent.keyboard("{ArrowUp}");
|
||||
await expectInsideListViewport(lastItem);
|
||||
// ArrowDown wraps back to the first item and its group heading.
|
||||
await userEvent.keyboard("{ArrowDown}");
|
||||
await expectInsideListViewport(await findVisibleText("Personal skills"));
|
||||
},
|
||||
};
|
||||
|
||||
export const TabSelectsSkill: Story = {
|
||||
play: async ({ canvasElement }) => {
|
||||
const editor = await typeInEditor(canvasElement, "/rev");
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { type ComponentProps, useState } from "react";
|
||||
import { expect, fn, userEvent } from "storybook/test";
|
||||
import { filterSkillsByQuery } from "../../utils/personalSkills";
|
||||
import { COMPACT_SLASH_COMMAND } from "../../utils/slashCommands";
|
||||
@@ -9,6 +10,7 @@ import {
|
||||
SkillsTriggerMenu,
|
||||
} from "./SkillsTriggerMenu";
|
||||
import {
|
||||
expectInsideListViewport,
|
||||
expectNoVisibleText,
|
||||
findVisibleText,
|
||||
MockSkills,
|
||||
@@ -146,6 +148,44 @@ export const Filtered: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
const manyPersonalSkillItems = Array.from({ length: 30 }, (_, index) =>
|
||||
createSkillMenuItem("personal", {
|
||||
name: `skill-${String(index).padStart(2, "0")}`,
|
||||
description: "",
|
||||
}),
|
||||
);
|
||||
|
||||
// cmdk scrolls the controlled highlight into view only at mount, so the
|
||||
// selection must move after mount to exercise the menu's own scrolling.
|
||||
const SelectionScrollHarness = (
|
||||
args: ComponentProps<typeof SkillsTriggerMenu>,
|
||||
) => {
|
||||
const [selectedIndex, setSelectedIndex] = useState(0);
|
||||
return (
|
||||
<>
|
||||
<button type="button" onClick={() => setSelectedIndex(29)}>
|
||||
Highlight last skill
|
||||
</button>
|
||||
<SkillsTriggerMenu
|
||||
{...args}
|
||||
selectedIndex={selectedIndex}
|
||||
onSelectedIndexChange={setSelectedIndex}
|
||||
/>
|
||||
</>
|
||||
);
|
||||
};
|
||||
|
||||
export const ScrollsSelectionIntoView: Story = {
|
||||
args: {
|
||||
personalSkills: manyPersonalSkillItems,
|
||||
},
|
||||
render: (args) => <SelectionScrollHarness {...args} />,
|
||||
play: async () => {
|
||||
await userEvent.click(await findVisibleText("Highlight last skill"));
|
||||
await expectInsideListViewport(await findVisibleText("/skill-29"));
|
||||
},
|
||||
};
|
||||
|
||||
export const SelectsByClick: Story = {
|
||||
args: {
|
||||
onSelect: fn(),
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useState } from "react";
|
||||
import { useLayoutEffect, useRef, useState } from "react";
|
||||
import {
|
||||
Command,
|
||||
CommandEmpty,
|
||||
@@ -98,16 +98,44 @@ const SkillCommandItem = ({
|
||||
value,
|
||||
selected,
|
||||
onSelect,
|
||||
consumePointerHighlight,
|
||||
}: {
|
||||
skill: SkillMenuItem;
|
||||
value: string;
|
||||
selected: boolean;
|
||||
onSelect: (skill: SkillMenuItem) => void;
|
||||
consumePointerHighlight: () => boolean;
|
||||
}) => {
|
||||
const handleSelect = () => onSelect(skill);
|
||||
const itemRef = useRef<HTMLDivElement>(null);
|
||||
|
||||
// cmdk only auto-scrolls for its own key handling; arrow keys here are
|
||||
// consumed by the Lexical trigger plugin and arrive as a controlled value
|
||||
// change, so the item scrolls itself when it becomes the highlight.
|
||||
// Pointer highlights skip scrolling, like cmdk, to avoid hover/scroll loops.
|
||||
useLayoutEffect(() => {
|
||||
if (!selected || consumePointerHighlight()) {
|
||||
return;
|
||||
}
|
||||
const item = itemRef.current;
|
||||
if (!item) {
|
||||
return;
|
||||
}
|
||||
if (item.parentElement?.firstElementChild === item) {
|
||||
// First item in a group: reveal the group heading as well. cmdk
|
||||
// renders headings internally without exposing a ref, so locate
|
||||
// it through the DOM the same way cmdk does.
|
||||
item
|
||||
.closest("[cmdk-group]")
|
||||
?.querySelector("[cmdk-group-heading]")
|
||||
?.scrollIntoView({ block: "nearest" });
|
||||
}
|
||||
item.scrollIntoView({ block: "nearest" });
|
||||
}, [selected, consumePointerHighlight]);
|
||||
|
||||
return (
|
||||
<CommandItem
|
||||
ref={itemRef}
|
||||
value={value}
|
||||
aria-selected={selected}
|
||||
className={cn(
|
||||
@@ -170,13 +198,23 @@ export const SkillsTriggerMenu = ({
|
||||
const shouldShowEmpty = allSkills.length === 0 && statusItems.length === 0;
|
||||
const selectedValue = selectedIndex >= 0 ? String(selectedIndex) : "";
|
||||
|
||||
const pointerHighlightRef = useRef(false);
|
||||
|
||||
const consumePointerHighlight = () => {
|
||||
const fromPointer = pointerHighlightRef.current;
|
||||
pointerHighlightRef.current = false;
|
||||
return fromPointer;
|
||||
};
|
||||
|
||||
const handleHighlightedValueChange = (value: string) => {
|
||||
const nextIndex = Number(value);
|
||||
if (
|
||||
Number.isInteger(nextIndex) &&
|
||||
nextIndex >= 0 &&
|
||||
nextIndex < allSkills.length
|
||||
nextIndex < allSkills.length &&
|
||||
nextIndex !== selectedIndex
|
||||
) {
|
||||
pointerHighlightRef.current = true;
|
||||
onSelectedIndexChange(nextIndex);
|
||||
}
|
||||
};
|
||||
@@ -188,6 +226,7 @@ export const SkillsTriggerMenu = ({
|
||||
value={String(index)}
|
||||
selected={index === selectedIndex}
|
||||
onSelect={onSelect}
|
||||
consumePointerHighlight={consumePointerHighlight}
|
||||
/>
|
||||
);
|
||||
|
||||
|
||||
@@ -46,3 +46,17 @@ export const expectNoVisibleText = async (text: string): Promise<void> => {
|
||||
).toBe(true);
|
||||
});
|
||||
};
|
||||
|
||||
// The 1px tolerance absorbs subpixel rounding in scrolled rects.
|
||||
export const expectInsideListViewport = async (
|
||||
element: HTMLElement,
|
||||
): Promise<void> => {
|
||||
const list = element.closest("[cmdk-list]");
|
||||
expect(list).not.toBeNull();
|
||||
await waitFor(() => {
|
||||
const listRect = (list as HTMLElement).getBoundingClientRect();
|
||||
const elementRect = element.getBoundingClientRect();
|
||||
expect(elementRect.top).toBeGreaterThanOrEqual(listRect.top - 1);
|
||||
expect(elementRect.bottom).toBeLessThanOrEqual(listRect.bottom + 1);
|
||||
});
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user