Skip to content

fix: make component drawer table rows inert, move selection to the name control #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marekdano wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: make component drawer table rows inert, move selection to the name control #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/components/prompts/PromptDefinitionTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ describe("PromptDefinitionTable", () => {
expect(screen.getByRole("cell", { name: "Greet User" })).toBeInTheDocument();
});

it("calls onSelectPrompt with the row's prompt when the row is clicked", async () => {
it("calls onSelectPrompt with the row's prompt when the name button is clicked", async () => {
const onSelectPrompt = vi.fn();
const user = userEvent.setup();
const a = mockPrompt({ id: "a", name: "prompt_a" });
const b = mockPrompt({ id: "b", name: "prompt_b" });
render(<PromptDefinitionTable prompts={[a, b]} onSelectPrompt={onSelectPrompt} />);

await user.click(screen.getByRole("cell", { name: "prompt_b" }));
await user.click(screen.getByRole("button", { name: "prompt_b" }));
expect(onSelectPrompt).toHaveBeenCalledWith(b);
});

Expand All @@ -68,19 +68,18 @@ describe("PromptDefinitionTable", () => {
expect(onSelectPrompt).not.toHaveBeenCalled();
});

it("selects the row via keyboard, but not when a key fires from an in-row control", async () => {
it("selects via keyboard on the name button, but not from the copy button", async () => {
const onSelectPrompt = vi.fn();
const user = userEvent.setup();
render(<PromptDefinitionTable prompts={[mockPrompt()]} onSelectPrompt={onSelectPrompt} />);

// Enter from the copy button must not bubble up into a row selection.
// Enter on the copy button must not also select the row.
screen.getByRole("button", { name: /copy prompt id/i }).focus();
await user.keyboard("{Enter}");
expect(onSelectPrompt).not.toHaveBeenCalled();

// Enter on the row itself does select it.
const dataRow = screen.getAllByRole("row")[1];
dataRow.focus();
// Enter on the name button does select it.
screen.getByRole("button", { name: "greet_user" }).focus();
await user.keyboard("{Enter}");
expect(onSelectPrompt).toHaveBeenCalledTimes(1);
});
Expand Down
52 changes: 15 additions & 37 deletions src/components/prompts/PromptDefinitionTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,28 +64,22 @@ export function PromptDefinitionTable({
<TableRow
key={prompt.id}
data-state={selectedPromptId === prompt.id ? "selected" : undefined}
onClick={() => onSelectPrompt(prompt)}
tabIndex={0}
onKeyDown={(e) => {
// Ignore keys bubbling up from in-row controls (copy / menu) so
// activating them doesn't also select the row.
if (e.target !== e.currentTarget) return;
if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
onSelectPrompt(prompt);
}
}}
className="cursor-pointer border-0 bg-neutral-50 hover:bg-neutral-50 data-[state=selected]:bg-neutral-50 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-inset dark:bg-neutral-800/50 dark:hover:bg-neutral-800/50 dark:data-[state=selected]:bg-neutral-800/50 [&>td:first-child]:rounded-l-lg [&>td:last-child]:rounded-r-lg"
className="border-0 bg-neutral-50 hover:bg-neutral-50 data-[state=selected]:bg-neutral-50 dark:bg-neutral-800/50 dark:hover:bg-neutral-800/50 dark:data-[state=selected]:bg-neutral-800/50 [&>td:first-child]:rounded-l-lg [&>td:last-child]:rounded-r-lg"
>
<TableCell className="px-4 py-3 text-sm text-foreground">
<span className="block truncate" title={prompt.displayName || prompt.name}>
<button
type="button"
onClick={() => onSelectPrompt(prompt)}
className="block max-w-full truncate rounded-sm text-left transition-colors hover:underline focus-visible:underline focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
title={prompt.displayName || prompt.name}
>
{prompt.displayName || prompt.name}
</span>
</button>
</TableCell>

<TableCell className="px-4 py-3">
<div className="flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground">
<div className="group flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground transition-colors group-hover:text-foreground">
{truncateMiddle(prompt.id, 40)}
</span>
<CopyButton
Expand All @@ -95,7 +89,7 @@ export function PromptDefinitionTable({
{ name: prompt.name },
)}
iconClassName="size-3"
className="ml-4 size-4 shrink-0 text-muted-foreground hover:text-foreground"
className="ml-4 size-4 shrink-0 text-muted-foreground transition-colors group-hover:text-foreground"
/>
</div>
</TableCell>
Expand All @@ -112,22 +106,14 @@ export function PromptDefinitionTable({
{ id: "prompts.details.moreOptionsFor" },
{ name: prompt.name },
)}
className="size-5 text-muted-foreground hover:text-foreground"
onClick={(e) => {
e.stopPropagation();
}}
className="size-5 text-muted-foreground transition-colors hover:text-foreground"
>
<MoreHorizontal className="size-4" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="end">
{onEdit && (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onEdit(prompt);
}}
>
<DropdownMenuItem onClick={() => onEdit(prompt)}>
{intl.formatMessage({ id: "prompts.details.action.edit" })}
</DropdownMenuItem>
)}
Expand All @@ -136,10 +122,7 @@ export function PromptDefinitionTable({
const enabled = prompt.enabled ?? true;
return (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onTogglePrompt(prompt.id, enabled);
}}
onClick={() => onTogglePrompt(prompt.id, enabled)}
aria-label={intl.formatMessage(
{
id: enabled
Expand All @@ -156,12 +139,7 @@ export function PromptDefinitionTable({
);
})()}
{onDelete && (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDelete(prompt);
}}
>
<DropdownMenuItem onClick={() => onDelete(prompt)}>
{intl.formatMessage({ id: "prompts.details.action.delete" })}
</DropdownMenuItem>
)}
Expand Down
2 changes: 1 addition & 1 deletion src/components/prompts/PromptDetailsPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ describe("PromptDetailsPanel", () => {
expect(screen.queryByText("Private")).not.toBeInTheDocument();

await user.click(screen.getByRole("tab", { name: /definition/i }));
await user.click(screen.getByRole("cell", { name: "prompt_b" }));
await user.click(screen.getByRole("button", { name: "prompt_b" }));

// Sidebar now reflects the row that was picked.
expect(screen.getByText("Private")).toBeInTheDocument();
Expand Down
47 changes: 15 additions & 32 deletions src/components/resources/ResourcesTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,18 +133,13 @@ describe("ResourcesTable", () => {
expect(screen.getByText("very-lo...-middle")).toBeInTheDocument();
});

it("calls onSelectResource when row is clicked", async () => {
it("calls onSelectResource when the name is clicked", async () => {
const user = userEvent.setup();
const resources = [createMockResource(1)];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const row = screen.getByText("Resource 1 Title").closest("tr");
expect(row).toBeInTheDocument();

if (row) {
await user.click(row);
expect(mockOnSelectResource).toHaveBeenCalledWith(resources[0]);
}
await user.click(screen.getByRole("button", { name: "Resource 1 Title" }));
expect(mockOnSelectResource).toHaveBeenCalledWith(resources[0]);
});

it("highlights selected resource row", () => {
Expand Down Expand Up @@ -208,12 +203,15 @@ describe("ResourcesTable", () => {
expect(tbody.children).toHaveLength(0);
});

it("applies cursor-pointer class to rows", () => {
it("underlines the name button on hover instead of the whole row being clickable", () => {
const resources = [createMockResource(1)];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const nameButton = screen.getByRole("button", { name: "Resource 1 Title" });
expect(nameButton).toHaveClass("hover:underline");

const row = screen.getByText("Resource 1 Title").closest("tr");
expect(row).toHaveClass("cursor-pointer");
expect(row).not.toHaveClass("cursor-pointer");
});

it("renders table with proper ARIA structure", () => {
Expand All @@ -236,12 +234,11 @@ describe("ResourcesTable", () => {
const resources = [createMockResource(1, { title: longTitle })];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const title = screen.getByText(longTitle);
const span = title.closest("span");
expect(span).toHaveClass("truncate");
const nameButton = screen.getByRole("button", { name: longTitle });
expect(nameButton).toHaveClass("truncate");
// The full name stays available (e.g. via native tooltip) even though
// it's visually clipped.
expect(span).toHaveAttribute("title", longTitle);
expect(nameButton).toHaveAttribute("title", longTitle);

// table-fixed + a percentage column width is what actually stops an
// unbreakable long name from forcing the whole table to scroll — a
Expand Down Expand Up @@ -411,41 +408,27 @@ describe("ResourcesTable", () => {
});

describe("keyboard selection", () => {
it("calls onSelectResource when Enter is pressed on a focused row", async () => {
it("calls onSelectResource when Enter is pressed on the focused name button", async () => {
const user = userEvent.setup();
const resources = [createMockResource(1)];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const row = screen.getByText("Resource 1 Title").closest("tr")!;
row.focus();
screen.getByRole("button", { name: "Resource 1 Title" }).focus();
await user.keyboard("{Enter}");

expect(mockOnSelectResource).toHaveBeenCalledWith(resources[0]);
});

it("calls onSelectResource when Space is pressed on a focused row", async () => {
it("calls onSelectResource when Space is pressed on the focused name button", async () => {
const user = userEvent.setup();
const resources = [createMockResource(1)];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const row = screen.getByText("Resource 1 Title").closest("tr")!;
row.focus();
screen.getByRole("button", { name: "Resource 1 Title" }).focus();
await user.keyboard(" ");

expect(mockOnSelectResource).toHaveBeenCalledWith(resources[0]);
});

it("does not call onSelectResource for an unrelated key", async () => {
const user = userEvent.setup();
const resources = [createMockResource(1)];
render(<ResourcesTable resources={resources} onSelectResource={mockOnSelectResource} />);

const row = screen.getByText("Resource 1 Title").closest("tr")!;
row.focus();
await user.keyboard("a");

expect(mockOnSelectResource).not.toHaveBeenCalled();
});
});

describe("toggle dropdown (onToggleResource provided)", () => {
Expand Down
60 changes: 19 additions & 41 deletions src/components/resources/ResourcesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,22 @@ export function ResourcesTable({
<TableRow
key={resource.id}
data-state={selectedResourceId === resource.id ? "selected" : undefined}
onClick={() => onSelectResource(resource)}
tabIndex={0}
onKeyDown={(e) => {
if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
onSelectResource(resource);
}
}}
className="cursor-pointer border-0 bg-neutral-50 hover:bg-neutral-50 data-[state=selected]:bg-neutral-50 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-inset dark:bg-neutral-800/50 dark:hover:bg-neutral-800/50 dark:data-[state=selected]:bg-neutral-800/50 [&>td:first-child]:rounded-l-lg [&>td:last-child]:rounded-r-lg"
className="border-0 bg-neutral-50 hover:bg-neutral-50 data-[state=selected]:bg-neutral-50 dark:bg-neutral-800/50 dark:hover:bg-neutral-800/50 dark:data-[state=selected]:bg-neutral-800/50 [&>td:first-child]:rounded-l-lg [&>td:last-child]:rounded-r-lg"
>
<TableCell className="px-4 py-3 text-sm text-foreground">
<span className="block truncate" title={resource.title || resource.name}>
<button
type="button"
onClick={() => onSelectResource(resource)}
className="block max-w-full truncate rounded-sm text-left transition-colors hover:underline focus-visible:underline focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
title={resource.title || resource.name}
>
{resource.title || resource.name}
</span>
</button>
</TableCell>

<TableCell className="px-4 py-3">
<div className="flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground">
<div className="group flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground transition-colors group-hover:text-foreground">
{truncateMiddle(resource.uriTemplate || resource.uri, 28)}
</span>
<CopyButton
Expand All @@ -85,21 +82,21 @@ export function ResourcesTable({
{ uri: resource.uriTemplate || resource.uri },
)}
iconClassName="size-3"
className="ml-4 size-4 shrink-0 text-muted-foreground hover:text-foreground"
className="ml-4 size-4 shrink-0 text-muted-foreground transition-colors group-hover:text-foreground"
/>
</div>
</TableCell>

<TableCell className="px-4 py-3">
<div className="flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground">
<div className="group flex min-w-0 items-center">
<span className="min-w-0 truncate font-mono text-xs text-muted-foreground transition-colors group-hover:text-foreground">
{truncateMiddle(resource.id, 18)}
</span>
<CopyButton
value={resource.id}
label={intl.formatMessage({ id: "resources.table.copyResourceId" })}
iconClassName="size-3"
className="ml-4 size-4 shrink-0 text-muted-foreground hover:text-foreground"
className="ml-4 size-4 shrink-0 text-muted-foreground transition-colors group-hover:text-foreground"
/>
</div>
</TableCell>
Expand All @@ -116,31 +113,20 @@ export function ResourcesTable({
{ id: "resources.table.moreOptionsFor" },
{ name: resource.title || resource.name },
)}
className="size-5 text-muted-foreground hover:text-foreground"
onClick={(e) => {
e.stopPropagation();
}}
className="size-5 text-muted-foreground transition-colors hover:text-foreground"
>
<MoreHorizontal className="size-4" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="end">
{onEditResource && (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onEditResource(resource);
}}
>
<DropdownMenuItem onClick={() => onEditResource(resource)}>
{intl.formatMessage({ id: "resources.table.edit" })}
</DropdownMenuItem>
)}
{onToggleResource && (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onToggleResource(resource.id, resource.enabled ?? true);
}}
onClick={() => onToggleResource(resource.id, resource.enabled ?? true)}
aria-label={intl.formatMessage(
{
id: resource.enabled
Expand All @@ -156,12 +142,7 @@ export function ResourcesTable({
</DropdownMenuItem>
)}
{onDeleteResource && (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDeleteResource(resource.id);
}}
>
<DropdownMenuItem onClick={() => onDeleteResource(resource.id)}>
{intl.formatMessage({ id: "resources.table.delete" })}
</DropdownMenuItem>
)}
Expand All @@ -176,10 +157,7 @@ export function ResourcesTable({
{ id: "resources.table.moreOptionsFor" },
{ name: resource.title || resource.name },
)}
className="size-5 text-muted-foreground hover:text-foreground"
onClick={(e) => {
e.stopPropagation();
}}
className="size-5 text-muted-foreground transition-colors hover:text-foreground"
>
<MoreHorizontal className="size-4" />
</Button>
Expand Down
17 changes: 6 additions & 11 deletions src/components/tools/ToolDetailsPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,18 +515,13 @@ describe("ToolDetailsPanel", () => {
expect(screen.getByText("tool_1")).toBeInTheDocument();
});

// Click on second tool
const tool2Row = screen.getByText("tool_2").closest("tr");
expect(tool2Row).toBeInTheDocument();
// Click on second tool's name button
await user.click(screen.getByRole("button", { name: "Display Name 2" }));

if (tool2Row) {
await user.click(tool2Row);

// Second tool details should now be shown
await waitFor(() => {
expect(screen.getByText("Display Name 2")).toBeInTheDocument();
});
}
// Second tool details should now be shown
await waitFor(() => {
expect(screen.getByText("Display Name 2")).toBeInTheDocument();
});
});

it("resets selected tool when panel closes", async () => {
Expand Down
Loading
Loading