From 62b796cead91bfe31d3db25c2ebcaf57221edada Mon Sep 17 00:00:00 2001 From: Marek Dano Date: Fri, 21 Aug 2026 13:09:25 +0100 Subject: [PATCH] fix: make component drawer table rows inert, move selection to the name control Signed-off-by: Marek Dano --- .../prompts/PromptDefinitionTable.test.tsx | 13 ++-- .../prompts/PromptDefinitionTable.tsx | 52 ++++---------- .../prompts/PromptDetailsPanel.test.tsx | 2 +- .../resources/ResourcesTable.test.tsx | 47 ++++--------- src/components/resources/ResourcesTable.tsx | 60 ++++++---------- .../tools/ToolDetailsPanel.test.tsx | 17 ++--- src/components/tools/ToolsTable.test.tsx | 25 +++---- src/components/tools/ToolsTable.tsx | 68 ++++++------------- 8 files changed, 92 insertions(+), 192 deletions(-) diff --git a/src/components/prompts/PromptDefinitionTable.test.tsx b/src/components/prompts/PromptDefinitionTable.test.tsx index c8ff019..20cd6de 100644 --- a/src/components/prompts/PromptDefinitionTable.test.tsx +++ b/src/components/prompts/PromptDefinitionTable.test.tsx @@ -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(); - await user.click(screen.getByRole("cell", { name: "prompt_b" })); + await user.click(screen.getByRole("button", { name: "prompt_b" })); expect(onSelectPrompt).toHaveBeenCalledWith(b); }); @@ -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(); - // 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); }); diff --git a/src/components/prompts/PromptDefinitionTable.tsx b/src/components/prompts/PromptDefinitionTable.tsx index 5eb75e3..29e8369 100644 --- a/src/components/prompts/PromptDefinitionTable.tsx +++ b/src/components/prompts/PromptDefinitionTable.tsx @@ -64,28 +64,22 @@ export function PromptDefinitionTable({ 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" > - + -
- +
+ {truncateMiddle(prompt.id, 40)}
@@ -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" > {onEdit && ( - { - e.stopPropagation(); - onEdit(prompt); - }} - > + onEdit(prompt)}> {intl.formatMessage({ id: "prompts.details.action.edit" })} )} @@ -136,10 +122,7 @@ export function PromptDefinitionTable({ const enabled = prompt.enabled ?? true; return ( { - e.stopPropagation(); - onTogglePrompt(prompt.id, enabled); - }} + onClick={() => onTogglePrompt(prompt.id, enabled)} aria-label={intl.formatMessage( { id: enabled @@ -156,12 +139,7 @@ export function PromptDefinitionTable({ ); })()} {onDelete && ( - { - e.stopPropagation(); - onDelete(prompt); - }} - > + onDelete(prompt)}> {intl.formatMessage({ id: "prompts.details.action.delete" })} )} diff --git a/src/components/prompts/PromptDetailsPanel.test.tsx b/src/components/prompts/PromptDetailsPanel.test.tsx index 2af7ce8..7768f3e 100644 --- a/src/components/prompts/PromptDetailsPanel.test.tsx +++ b/src/components/prompts/PromptDetailsPanel.test.tsx @@ -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(); diff --git a/src/components/resources/ResourcesTable.test.tsx b/src/components/resources/ResourcesTable.test.tsx index b980857..3e5b90a 100644 --- a/src/components/resources/ResourcesTable.test.tsx +++ b/src/components/resources/ResourcesTable.test.tsx @@ -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(); - 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", () => { @@ -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(); + 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", () => { @@ -236,12 +234,11 @@ describe("ResourcesTable", () => { const resources = [createMockResource(1, { title: longTitle })]; render(); - 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 @@ -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(); - 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(); - 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(); - - const row = screen.getByText("Resource 1 Title").closest("tr")!; - row.focus(); - await user.keyboard("a"); - - expect(mockOnSelectResource).not.toHaveBeenCalled(); - }); }); describe("toggle dropdown (onToggleResource provided)", () => { diff --git a/src/components/resources/ResourcesTable.tsx b/src/components/resources/ResourcesTable.tsx index bda0e66..c52834c 100644 --- a/src/components/resources/ResourcesTable.tsx +++ b/src/components/resources/ResourcesTable.tsx @@ -57,25 +57,22 @@ export function ResourcesTable({ 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" > - + -
- +
+ {truncateMiddle(resource.uriTemplate || resource.uri, 28)}
-
- +
+ {truncateMiddle(resource.id, 18)}
@@ -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" > {onEditResource && ( - { - e.stopPropagation(); - onEditResource(resource); - }} - > + onEditResource(resource)}> {intl.formatMessage({ id: "resources.table.edit" })} )} {onToggleResource && ( { - e.stopPropagation(); - onToggleResource(resource.id, resource.enabled ?? true); - }} + onClick={() => onToggleResource(resource.id, resource.enabled ?? true)} aria-label={intl.formatMessage( { id: resource.enabled @@ -156,12 +142,7 @@ export function ResourcesTable({ )} {onDeleteResource && ( - { - e.stopPropagation(); - onDeleteResource(resource.id); - }} - > + onDeleteResource(resource.id)}> {intl.formatMessage({ id: "resources.table.delete" })} )} @@ -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" > diff --git a/src/components/tools/ToolDetailsPanel.test.tsx b/src/components/tools/ToolDetailsPanel.test.tsx index 565866b..d525f4b 100644 --- a/src/components/tools/ToolDetailsPanel.test.tsx +++ b/src/components/tools/ToolDetailsPanel.test.tsx @@ -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 () => { diff --git a/src/components/tools/ToolsTable.test.tsx b/src/components/tools/ToolsTable.test.tsx index bf2f867..2e45c12 100644 --- a/src/components/tools/ToolsTable.test.tsx +++ b/src/components/tools/ToolsTable.test.tsx @@ -111,18 +111,13 @@ describe("ToolsTable", () => { expect(idCell).toBeInTheDocument(); }); - it("calls onSelectTool when row is clicked", async () => { + it("calls onSelectTool when the name is clicked", async () => { const user = userEvent.setup(); const tools = [createMockTool(1)]; render(); - const row = screen.getByText("Display Name 1").closest("tr"); - expect(row).toBeInTheDocument(); - - if (row) { - await user.click(row); - expect(mockOnSelectTool).toHaveBeenCalledWith(tools[0]); - } + await user.click(screen.getByRole("button", { name: "Display Name 1" })); + expect(mockOnSelectTool).toHaveBeenCalledWith(tools[0]); }); it("highlights selected tool row", () => { @@ -288,12 +283,15 @@ describe("ToolsTable", () => { expect(screen.getByText("tool_b")).toBeInTheDocument(); }); - it("applies cursor-pointer class to rows", () => { + it("underlines the name button on hover instead of the whole row being clickable", () => { const tools = [createMockTool(1)]; render(); + const nameButton = screen.getByRole("button", { name: "Display Name 1" }); + expect(nameButton).toHaveClass("hover:underline"); + const row = screen.getByText("Display Name 1").closest("tr"); - expect(row).toHaveClass("cursor-pointer"); + expect(row).not.toHaveClass("cursor-pointer"); }); it("renders table with proper ARIA structure", () => { @@ -316,10 +314,9 @@ describe("ToolsTable", () => { const tools = [createMockTool(1, { displayName: longName })]; render(); - const displayName = screen.getByText(longName); - const span = displayName.closest("span"); - expect(span).toHaveClass("truncate"); - expect(span).toHaveAttribute("title", longName); + const nameButton = screen.getByRole("button", { name: longName }); + expect(nameButton).toHaveClass("truncate"); + expect(nameButton).toHaveAttribute("title", longName); // table-fixed + a percentage column width is what actually stops an // unbreakable long name from forcing the whole table to scroll. diff --git a/src/components/tools/ToolsTable.tsx b/src/components/tools/ToolsTable.tsx index 9db952e..246b1a9 100644 --- a/src/components/tools/ToolsTable.tsx +++ b/src/components/tools/ToolsTable.tsx @@ -70,28 +70,22 @@ export function ToolsTable({ onSelectTool(tool)} - tabIndex={0} - onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - onSelectTool(tool); - } - }} - 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" > - onSelectTool(tool)} + 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={tool.displayName || tool.title || tool.name} > {tool.displayName || tool.title || tool.name} - + -
- +
+ {tool.customName || tool.originalName}
-
- +
+ {truncateMiddle(tool.id, 18)}
@@ -126,11 +120,8 @@ export function ToolsTable({ variant="ghost" size="icon-xs" aria-label={intl.formatMessage({ id: "tools.table.viewSchema" })} - className="size-5 text-muted-foreground hover:text-foreground" - onClick={(e) => { - e.stopPropagation(); - handleSchemaClick(tool); - }} + className="size-5 text-muted-foreground transition-colors hover:text-foreground" + onClick={() => handleSchemaClick(tool)} > { - e.stopPropagation(); - }} + className="size-5 text-muted-foreground transition-colors hover:text-foreground" > {onEditTool && ( - { - e.stopPropagation(); - onEditTool(tool); - }} - > + onEditTool(tool)}> {intl.formatMessage({ id: "tools.table.edit" })} )} {onToggleTool && ( - { - e.stopPropagation(); - onToggleTool(tool); - }} - > + onToggleTool(tool)}> {intl.formatMessage({ id: tool.enabled ? "tools.table.deactivate" : "tools.table.activate", })} )} {onDeleteTool && ( - { - e.stopPropagation(); - onDeleteTool(tool.id); - }} - > + onDeleteTool(tool.id)}> {intl.formatMessage({ id: "tools.table.delete" })} )} @@ -201,10 +174,7 @@ export function ToolsTable({ variant="ghost" size="icon-xs" aria-label="More options" - className="size-5 text-muted-foreground hover:text-foreground" - onClick={(e) => { - e.stopPropagation(); - }} + className="size-5 text-muted-foreground transition-colors hover:text-foreground" >