From 2b12c989e9ca7aa3feac4033bc6b634ae145abbe Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Mon, 21 Sep 2026 16:44:15 -0500 Subject: [PATCH] improve: one tool row for every integration tool dialog --- .../settings/CustomMcpServers.client.test.tsx | 6 +- .../components/settings/CustomMcpServers.tsx | 77 +++-------- .../IntegrationToolApprovalControls.tsx | 120 +++++++++++++++--- .../components/settings/Integrations.test.tsx | 38 +++--- .../McpToolManagementDialog.client.test.tsx | 22 +++- .../settings/McpToolManagementDialog.tsx | 70 +--------- 6 files changed, 167 insertions(+), 166 deletions(-) diff --git a/apps/web/src/components/settings/CustomMcpServers.client.test.tsx b/apps/web/src/components/settings/CustomMcpServers.client.test.tsx index 95ef96f03..acc51a0a1 100644 --- a/apps/web/src/components/settings/CustomMcpServers.client.test.tsx +++ b/apps/web/src/components/settings/CustomMcpServers.client.test.tsx @@ -376,7 +376,7 @@ describe('useCustomMcpServers', () => { expect( screen.getByRole('button', { name: 'Show less' }), ).toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: 'resolve' })).toBeChecked(); + expect(screen.getByRole('checkbox', { name: 'Resolve' })).toBeChecked(); state.tools = []; }); @@ -425,7 +425,7 @@ describe('useCustomMcpServers', () => { it('never loads or shows approval policies for non-admins or with the experiment off', async () => { state.isAdmin = false; await openToolsDialog(); - await screen.findByText('search'); + await screen.findByText('Search'); expect( screen.queryByRole('radiogroup', { name: 'Approval mode for search' }), ).not.toBeInTheDocument(); @@ -436,7 +436,7 @@ describe('useCustomMcpServers', () => { approvals.experimentEnabled = false; approvals.listEnabled.length = 0; await openToolsDialog(); - await screen.findByText('search'); + await screen.findByText('Search'); expect( screen.queryByRole('radiogroup', { name: 'Approval mode for search' }), ).not.toBeInTheDocument(); diff --git a/apps/web/src/components/settings/CustomMcpServers.tsx b/apps/web/src/components/settings/CustomMcpServers.tsx index 414e35bd8..56245c2b0 100644 --- a/apps/web/src/components/settings/CustomMcpServers.tsx +++ b/apps/web/src/components/settings/CustomMcpServers.tsx @@ -8,7 +8,6 @@ import { toast } from 'sonner'; import { Button, - Checkbox, Dialog, DialogContent, DialogDescription, @@ -745,32 +744,6 @@ function ServerFormDialog({ ); } -/** - * Some servers ship prompt-length tool descriptions. Two lines are enough to - * recognize a tool; the rest is one click away instead of burying the list. - * The toggle lives outside the row's label so it never flips the checkbox. - */ -function ToolDescription({ text }: { text: string }) { - const [expanded, setExpanded] = useState(false); - const isLong = text.length > 140; - - return ( -
-

{text}

- {isLong ? ( - - ) : null} -
- ); -} - function CustomToolManagementDialog({ server, scope, @@ -863,47 +836,27 @@ function CustomToolManagementDialog({
- {(tool) => ( - <> - { - setDisabledNames((current) => { - const next = new Set(current); - - if (checked === true) { - next.delete(tool.name); - } else { - next.add(tool.name); - } + isToolEnabled={(toolName) => !disabledNames.has(toolName)} + onToggleTool={(toolName, enabled) => + setDisabledNames((current) => { + const next = new Set(current); + + if (enabled) { + next.delete(toolName); + } else { + next.add(toolName); + } - return next; - }); - }} - className="mt-0.5" - /> -
- - {tool.description ? ( - - ) : null} -
- - )} - + return next; + }) + } + />
)} diff --git a/apps/web/src/components/settings/IntegrationToolApprovalControls.tsx b/apps/web/src/components/settings/IntegrationToolApprovalControls.tsx index 852d6c977..e2b2b672c 100644 --- a/apps/web/src/components/settings/IntegrationToolApprovalControls.tsx +++ b/apps/web/src/components/settings/IntegrationToolApprovalControls.tsx @@ -14,6 +14,7 @@ import { cn } from '@/lib/utils'; import { Badge, Ban, + Checkbox, ChevronDown, ChevronRight, CircleCheck, @@ -227,18 +228,22 @@ function IntegrationToolApprovalGroup({ * adds the per-tool mode control, the group-level control, and the save hint, * or nothing but the grouping when approvals are not active for the viewer. */ -export function IntegrationToolApprovalList({ +export function IntegrationToolApprovalList({ integrationId, + integrationName, scope, canManage, open, tools, saveNote, - rowClassName, - children, + isToolEnabled, + onToggleTool, + toggleDisabled, }: { /** The id policies are keyed on: the mount name agents see. */ integrationId: string | null; + /** Dropped from tool names that repeat it ("resend_list_domains"). */ + integrationName: string | null; /** `personal` policies apply to the viewer's own sessions only. */ scope: 'deployment' | 'personal'; /** Deployment policies are admin-managed; never load them otherwise. */ @@ -247,8 +252,10 @@ export function IntegrationToolApprovalList({ tools: T[]; /** How the dialog's own enable/disable changes are saved. */ saveNote: string; - rowClassName: string; - children: (tool: T) => ReactNode; + isToolEnabled: (toolName: string) => boolean; + /** Staged by the dialog until its Save, hence a checkbox, not a switch. */ + onToggleTool: (toolName: string, enabled: boolean) => void; + toggleDisabled?: boolean; }) { const experiment = useIntegrationToolApprovalsExperiment(); const active = experiment.enabled && canManage && integrationId != null; @@ -290,23 +297,100 @@ export function IntegrationToolApprovalList({ } : {})} > - {group.tools.map((tool) => ( -
- {children(tool)} - {active ? ( - - policies.setMode(integrationId, tool.name, mode) + {group.tools.map((tool) => { + const checkboxId = `integration-tool-${integrationId ?? 'unknown'}-${tool.name}`; + return ( +
+ + onToggleTool(tool.name, checked === true) } + className="mt-0.5" /> - ) : null} -
- ))} +
+ + {tool.description ? ( + + ) : null} +
+ {active ? ( + + policies.setMode(integrationId, tool.name, mode) + } + /> + ) : null} +
+ ); + })} ))} ); } + +type ManageableTool = GroupableTool & { description?: string | null }; + +function splitToolNameParts(name: string): string[] { + return name.split(/[-_\s]+/).filter((part) => part.length > 0); +} + +/** "resend_list_api_keys" under Resend reads as "List Api Keys". */ +function prettifyToolName( + name: string, + integrationName: string | null, +): string { + const nameParts = splitToolNameParts(name); + const integrationParts = integrationName + ? splitToolNameParts(integrationName) + : []; + const hasIntegrationPrefix = + integrationParts.length > 0 && + integrationParts.every( + (part, index) => nameParts[index]?.toLowerCase() === part.toLowerCase(), + ); + const displayParts = hasIntegrationPrefix + ? nameParts.slice(integrationParts.length) + : nameParts; + + return (displayParts.length > 0 ? displayParts : nameParts) + .map((part) => part.charAt(0).toUpperCase() + part.slice(1).toLowerCase()) + .join(' '); +} + +/** + * MCP tool descriptions are written for the model and can run to several + * paragraphs, so long ones clamp to two lines behind a toggle. + */ +function ToolDescription({ text }: { text: string }) { + const [expanded, setExpanded] = useState(false); + const isLong = text.length > 140; + + return ( +
+

{text}

+ {isLong ? ( + + ) : null} +
+ ); +} diff --git a/apps/web/src/components/settings/Integrations.test.tsx b/apps/web/src/components/settings/Integrations.test.tsx index 49a1c0e6e..8bca31a79 100644 --- a/apps/web/src/components/settings/Integrations.test.tsx +++ b/apps/web/src/components/settings/Integrations.test.tsx @@ -233,14 +233,22 @@ vi.mock('@/hooks/linear', () => ({ vi.mock('./IntegrationToolApprovalControls', () => ({ IntegrationToolApprovalList: ({ tools, - children, + isToolEnabled, + onToggleTool, }: { tools: T[]; - children: (tool: T) => ReactNode; + isToolEnabled: (toolName: string) => boolean; + onToggleTool: (toolName: string, enabled: boolean) => void; }) => (
{tools.map((tool) => ( -
{children(tool)}
+ onToggleTool(tool.name, event.target.checked)} + /> ))}
), @@ -1556,7 +1564,7 @@ describe('Integrations settings', () => { ).not.toBeNull(); }); - it('opens the manage tools dialog with prettified tool labels', () => { + it('opens the manage tools dialog with a toggle per tool', () => { state.deploymentEnablements = [{ mcpId: 'sentry', enabled: true }]; state.userConnections = [ { id: 'conn-sentry', mcpId: 'sentry', authStatus: 'authenticated' }, @@ -1582,16 +1590,8 @@ describe('Integrations settings', () => { screen.getByRole('heading', { name: 'Manage tools for Sentry' }), ).toBeInTheDocument(); expect( - screen.getByRole('button', { name: 'Disable get_sentry_resource' }), - ).toBeInTheDocument(); - expect(screen.getByText('Get Sentry Resource')).toHaveAttribute( - 'for', - expect.stringMatching(/^mcp-tool-sentry-/), - ); - expect(screen.queryByText('get_sentry_resource')).not.toBeInTheDocument(); - expect( - screen.queryByText('Inspect a Sentry resource'), - ).not.toBeInTheDocument(); + screen.getByRole('checkbox', { name: 'get_sentry_resource' }), + ).toBeChecked(); expect( screen.getByRole('button', { name: 'Save changes' }), ).toBeInTheDocument(); @@ -2632,7 +2632,7 @@ describe('Integrations settings', () => { screen.getByRole('button', { name: 'Manage Sentry tools' }), ); fireEvent.click( - screen.getByRole('button', { name: 'Disable get_sentry_resource' }), + screen.getByRole('checkbox', { name: 'get_sentry_resource' }), ); rerender(); @@ -2641,11 +2641,11 @@ describe('Integrations settings', () => { screen.getByRole('heading', { name: 'Manage tools for Sentry' }), ).toBeInTheDocument(); expect( - screen.getByRole('button', { name: 'Enable get_sentry_resource' }), - ).toBeInTheDocument(); + screen.getByRole('checkbox', { name: 'get_sentry_resource' }), + ).not.toBeChecked(); expect( - screen.getByRole('button', { name: 'Enable search_events' }), - ).toBeInTheDocument(); + screen.getByRole('checkbox', { name: 'search_events' }), + ).not.toBeChecked(); }); it('lets an admin store a voice key from the Voice card', async () => { diff --git a/apps/web/src/components/settings/McpToolManagementDialog.client.test.tsx b/apps/web/src/components/settings/McpToolManagementDialog.client.test.tsx index bee33d445..0c930cf2e 100644 --- a/apps/web/src/components/settings/McpToolManagementDialog.client.test.tsx +++ b/apps/web/src/components/settings/McpToolManagementDialog.client.test.tsx @@ -15,6 +15,7 @@ const state = vi.hoisted(() => ({ mode: string; }[], annotated: false, + searchDescription: null as string | null, policiesQueryEnabled: undefined as boolean | undefined, })); @@ -56,7 +57,7 @@ vi.mock('@/hooks/mcp-connections', () => ({ tools: [ { name: 'web_search_exa', - description: null, + description: state.searchDescription, enabled: true, readOnly: state.annotated ? true : null, }, @@ -103,6 +104,25 @@ describe('McpToolManagementDialog tool approvals', () => { Element.prototype.scrollIntoView = vi.fn(); }); + it('shows each tool as a staged checkbox with a readable name and its description', () => { + state.searchDescription = 'Search the web with Exa.'; + try { + renderDialog(); + const checkbox = screen.getByRole('checkbox', { + name: 'Web Search Exa', + }); + expect(checkbox).toBeChecked(); + expect(screen.getByText('Web Search Exa')).toHaveAttribute( + 'title', + 'web_search_exa', + ); + expect(screen.getByText('Search the web with Exa.')).toBeInTheDocument(); + expect(screen.queryByRole('switch')).not.toBeInTheDocument(); + } finally { + state.searchDescription = null; + } + }); + it('hides per-tool approval modes while the experiment is off', () => { renderDialog(); expect( diff --git a/apps/web/src/components/settings/McpToolManagementDialog.tsx b/apps/web/src/components/settings/McpToolManagementDialog.tsx index 006c72042..5c25d899b 100644 --- a/apps/web/src/components/settings/McpToolManagementDialog.tsx +++ b/apps/web/src/components/settings/McpToolManagementDialog.tsx @@ -16,9 +16,7 @@ import { DialogFooter, DialogHeader, DialogTitle, - Label, Spinner, - Switch, ToggleLeft, ToggleRight, } from '@/components/system'; @@ -40,36 +38,6 @@ type McpToolManagementDialogProps = { isAdmin?: boolean; }; -function splitToolNameParts(name: string): string[] { - return name.split(/[-_\s]+/).filter((part) => part.length > 0); -} - -function titleCaseToolNamePart(part: string): string { - return part.charAt(0).toUpperCase() + part.slice(1).toLowerCase(); -} - -function prettifyToolName( - name: string, - integrationName: string | null, -): string { - const nameParts = splitToolNameParts(name); - const integrationParts = integrationName - ? splitToolNameParts(integrationName) - : []; - const hasIntegrationPrefix = - integrationParts.length > 0 && - integrationParts.every( - (part, index) => nameParts[index]?.toLowerCase() === part.toLowerCase(), - ); - const displayParts = hasIntegrationPrefix - ? nameParts.slice(integrationParts.length) - : nameParts; - - return (displayParts.length > 0 ? displayParts : nameParts) - .map(titleCaseToolNamePart) - .join(' '); -} - function McpToolLoadErrorMessage({ integrationName, message, @@ -277,42 +245,18 @@ export function McpToolManagementDialog({
- {(tool) => { - const enabled = !normalizedDisabledToolNames.includes( - tool.name, - ); - const switchId = `mcp-tool-${mcpId ?? 'unknown'}-${tool.name}`; - - return ( -
-
- - handleToggle(tool.name, nextEnabled) - } - /> - -
-
- ); - }} - + isToolEnabled={(toolName) => + !normalizedDisabledToolNames.includes(toolName) + } + onToggleTool={handleToggle} + toggleDisabled={setDisabledTools.isPending} + />
) : null}