diff --git a/web/app/components/tools/mcp/__tests__/index.spec.tsx b/web/app/components/tools/mcp/__tests__/index.spec.tsx index abf847d3806..0d8934d2636 100644 --- a/web/app/components/tools/mcp/__tests__/index.spec.tsx +++ b/web/app/components/tools/mcp/__tests__/index.spec.tsx @@ -13,6 +13,8 @@ type MockDetail = MockProvider | undefined // Mock dependencies const mockRefetch = vi.fn() +const mockUpdateMCP = vi.fn() +const mockDeleteMCP = vi.fn() const mockUseAllToolProviders = vi.fn() let mockProviders: MockProvider[] = [] let mockIsLoadingToolProviders = false @@ -29,6 +31,13 @@ vi.mock('@/service/use-tools', () => ({ refetch: mockRefetch, } }, + useUpdateMCP: () => ({ + mutateAsync: mockUpdateMCP, + }), + useDeleteMCP: () => ({ + mutateAsync: mockDeleteMCP, + isPending: false, + }), })) vi.mock('@/context/permission-state', async () => { @@ -72,13 +81,13 @@ vi.mock('../provider-card', () => ({ default: ({ data, handleSelect, - onUpdate, - onDeleted, + onEdit, + onDelete, }: { data: MockProvider handleSelect: (id: string) => void - onUpdate: (id: string) => void - onDeleted: () => void + onEdit: (id: string) => void + onDelete: (id: string) => void }) => { const displayName = typeof data.name === 'string' ? data.name : Object.values(data.name)[0] return ( @@ -86,10 +95,10 @@ vi.mock('../provider-card', () => ({ - - @@ -102,12 +111,16 @@ vi.mock('../detail/provider-detail', () => ({ detail, onHide, onUpdate, + onEdit, + onDelete, isTriggerAuthorize, onFirstCreate, }: { detail: MockDetail onHide: () => void onUpdate: () => void + onEdit: (id: string) => void + onDelete: (id: string) => void isTriggerAuthorize: boolean onFirstCreate: () => void }) => { @@ -126,6 +139,12 @@ vi.mock('../detail/provider-detail', () => ({ + + @@ -134,6 +153,34 @@ vi.mock('../detail/provider-detail', () => ({ }, })) +vi.mock('../modal', () => ({ + default: ({ + show, + data, + onConfirm, + onHide, + }: { + show: boolean + data?: MockProvider + onConfirm: (form: { name: string; server_url: string }) => void + onHide: () => void + }) => + show ? ( +
+
{data?.name as string}
+ + +
+ ) : null, +})) + describe('MCPList', () => { beforeEach(() => { vi.clearAllMocks() @@ -142,6 +189,8 @@ describe('MCPList', () => { mockIsLoadingToolProviders = false mockConsoleState.workspacePermissionKeys = ['mcp.manage'] mockRefetch.mockResolvedValue(undefined) + mockUpdateMCP.mockResolvedValue({ result: 'success' }) + mockDeleteMCP.mockResolvedValue({ result: 'success' }) }) afterEach(() => { @@ -379,31 +428,49 @@ describe('MCPList', () => { mockProviders = [{ id: '1', name: 'Provider 1', type: 'mcp' }] }) - it('should call refetch and set provider after update', async () => { + it('should open only the edit dialog when edit is selected from a card', async () => { render() - const updateBtn = screen.getByTestId('update-btn-1') + fireEvent.click(screen.getByTestId('edit-btn-1')) - await act(async () => { - fireEvent.click(updateBtn) - vi.advanceTimersByTime(10) - await Promise.resolve() - }) + expect(screen.getByRole('dialog', { name: 'Edit MCP' })).toBeInTheDocument() + expect(screen.queryByTestId('detail-panel')).not.toBeInTheDocument() + }) - expect(mockRefetch).toHaveBeenCalled() + it('should replace detail with the edit dialog and restore detail on cancel', () => { + render() + + fireEvent.click(screen.getByText('Provider 1')) + expect(screen.getByTestId('detail-panel')).toBeInTheDocument() + + fireEvent.click(screen.getByTestId('edit-detail')) + expect(screen.getByRole('dialog', { name: 'Edit MCP' })).toBeInTheDocument() + expect(screen.queryByTestId('detail-panel')).not.toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + expect(screen.queryByRole('dialog', { name: 'Edit MCP' })).not.toBeInTheDocument() + expect(screen.getByTestId('detail-panel')).toBeInTheDocument() }) it('should show detail panel with trigger authorize after update', async () => { render() - const updateBtn = screen.getByTestId('update-btn-1') + const updateBtn = screen.getByTestId('edit-btn-1') + + fireEvent.click(updateBtn) await act(async () => { - fireEvent.click(updateBtn) + fireEvent.click(screen.getByRole('button', { name: 'Save' })) vi.advanceTimersByTime(10) await Promise.resolve() }) + expect(mockUpdateMCP).toHaveBeenCalledWith({ + name: 'Updated MCP', + server_url: 'https://updated.com', + provider_id: '1', + }) + expect(mockRefetch).toHaveBeenCalled() expect(screen.getByTestId('detail-panel')).toBeInTheDocument() expect(screen.getByTestId('trigger-authorize')).toHaveTextContent('true') }) @@ -414,17 +481,62 @@ describe('MCPList', () => { mockProviders = [{ id: '1', name: 'Provider 1', type: 'mcp' }] }) - it('should call refetch after delete', async () => { + it('should replace detail with delete confirmation and restore detail on cancel', () => { render() - const deleteBtn = screen.getByTestId('delete-btn-1') + fireEvent.click(screen.getByText('Provider 1')) + expect(screen.getByTestId('detail-panel')).toBeInTheDocument() + + fireEvent.click(screen.getByTestId('delete-detail')) + expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() + expect(screen.queryByTestId('detail-panel')).not.toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: 'common.operation.cancel' })) + expect(screen.getByTestId('detail-panel')).toBeInTheDocument() + }) + + it('should restore detail when the delete dialog requests close', () => { + render() + + fireEvent.click(screen.getByText('Provider 1')) + fireEvent.click(screen.getByTestId('delete-detail')) + expect(screen.queryByTestId('detail-panel')).not.toBeInTheDocument() + + fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + + expect(screen.getByTestId('detail-panel')).toBeInTheDocument() + }) + + it('should delete from a card without selecting it', async () => { + render() + + fireEvent.click(screen.getByTestId('delete-btn-1')) + expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() await act(async () => { - fireEvent.click(deleteBtn) + fireEvent.click(screen.getByRole('button', { name: 'common.operation.confirm' })) vi.advanceTimersByTime(10) + await Promise.resolve() }) + expect(mockDeleteMCP).toHaveBeenCalledWith('1') expect(mockRefetch).toHaveBeenCalled() + expect(screen.queryByTestId('detail-panel')).not.toBeInTheDocument() + }) + + it('should keep delete confirmation open when deletion fails', async () => { + mockDeleteMCP.mockResolvedValue({ result: 'error' }) + render() + + fireEvent.click(screen.getByTestId('delete-btn-1')) + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'common.operation.confirm' })) + await Promise.resolve() + }) + + expect(mockRefetch).not.toHaveBeenCalled() + expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() }) }) diff --git a/web/app/components/tools/mcp/__tests__/modal.spec.tsx b/web/app/components/tools/mcp/__tests__/modal.spec.tsx index 3f1d3304fde..1fc627308b7 100644 --- a/web/app/components/tools/mcp/__tests__/modal.spec.tsx +++ b/web/app/components/tools/mcp/__tests__/modal.spec.tsx @@ -47,6 +47,7 @@ describe('MCPModal', () => { it('should render create title when no data is provided', () => { render(, { wrapper: createWrapper() }) expect(screen.getByText('tools.mcp.modal.title'))!.toBeInTheDocument() + expect(screen.getByRole('dialog')).toHaveAccessibleName('tools.mcp.modal.title') }) it('should render edit title when data is provided', () => { @@ -188,6 +189,15 @@ describe('MCPModal', () => { expect(onHide).toHaveBeenCalled() }) + it('should call onHide when the dialog requests close', () => { + const onHide = vi.fn() + render(, { wrapper: createWrapper() }) + + fireEvent.keyDown(document, { key: 'Escape', code: 'Escape' }) + + expect(onHide).toHaveBeenCalledTimes(1) + }) + it('should have confirm button disabled when form is empty', () => { render(, { wrapper: createWrapper() }) diff --git a/web/app/components/tools/mcp/__tests__/provider-card.spec.tsx b/web/app/components/tools/mcp/__tests__/provider-card.spec.tsx index 99d5e282245..216449a16a2 100644 --- a/web/app/components/tools/mcp/__tests__/provider-card.spec.tsx +++ b/web/app/components/tools/mcp/__tests__/provider-card.spec.tsx @@ -1,57 +1,12 @@ import type { ReactNode } from 'react' import type { ToolWithProvider } from '@/app/components/workflow/types' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' -import { fireEvent, screen, waitFor } from '@testing-library/react' +import { fireEvent, screen } from '@testing-library/react' import * as React from 'react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { render } from '@/test/console/render' import MCPCard from '../provider-card' -// Mutable mock functions -const mockUpdateMCP = vi.fn().mockResolvedValue({ result: 'success' }) -const mockDeleteMCP = vi.fn().mockResolvedValue({ result: 'success' }) - -// Mock the services -vi.mock('@/service/use-tools', () => ({ - useUpdateMCP: () => ({ - mutateAsync: mockUpdateMCP, - }), - useDeleteMCP: () => ({ - mutateAsync: mockDeleteMCP, - }), -})) - -// Mock the MCPModal -type MCPModalForm = { - name: string - server_url: string -} - -type MCPModalProps = { - show: boolean - onConfirm: (form: MCPModalForm) => void - onHide: () => void -} - -vi.mock('../modal', () => ({ - default: ({ show, onConfirm, onHide }: MCPModalProps) => { - if (!show) return null - return ( -
- - -
- ) - }, -})) - // Mock the OperationDropdown type OperationDropdownProps = { onEdit: () => void @@ -59,30 +14,44 @@ type OperationDropdownProps = { onOpenChange: (open: boolean) => void } -vi.mock('../detail/operation-dropdown', () => ({ - default: ({ onEdit, onRemove, onOpenChange }: OperationDropdownProps) => ( -
- - -
- ), -})) +vi.mock('../detail/operation-dropdown', async () => { + const { createPortal } = await import('react-dom') + + return { + default: ({ onEdit, onRemove, onOpenChange }: OperationDropdownProps) => ( + <> +
+ +
+ {createPortal( + <> + + + , + document.body, + )} + + ), + } +}) const mockConsoleState = vi.hoisted(() => ({ workspacePermissionKeys: ['mcp.manage'] as string[], @@ -151,20 +120,11 @@ describe('MCPCard', () => { const defaultProps = { data: createMockData(), handleSelect: vi.fn(), - onUpdate: vi.fn(), - onDeleted: vi.fn(), + onEdit: vi.fn(), + onDelete: vi.fn(), } - const getDeleteConfirmButton = () => - screen.getByRole('button', { name: 'common.operation.confirm' }) - const getDeleteCancelButton = () => - screen.getByRole('button', { name: 'common.operation.cancel' }) - beforeEach(() => { - mockUpdateMCP.mockClear() - mockDeleteMCP.mockClear() - mockUpdateMCP.mockResolvedValue({ result: 'success' }) - mockDeleteMCP.mockResolvedValue({ result: 'success' }) mockConsoleState.workspacePermissionKeys = ['mcp.manage'] }) @@ -264,11 +224,9 @@ describe('MCPCard', () => { wrapper: createWrapper(), }) - const card = screen.getByText('Test MCP Server').closest('[class*="cursor-pointer"]') - if (card) { - fireEvent.click(card) - expect(handleSelect).toHaveBeenCalledWith('mcp-1') - } + fireEvent.click(screen.getByRole('button', { name: /Test MCP Server/ })) + + expect(handleSelect).toHaveBeenCalledWith('mcp-1') }) }) @@ -336,185 +294,43 @@ describe('MCPCard', () => { expect(screen.queryByTestId('operation-dropdown')).not.toBeInTheDocument() }) - it('should stop propagation when clicking on dropdown container', () => { + it('should not select the card when clicking the dropdown icon', () => { const handleSelect = vi.fn() render(, { wrapper: createWrapper(), }) - // Click on the dropdown area (which should stop propagation) - const dropdown = screen.getByTestId('operation-dropdown') - const dropdownContainer = dropdown.closest('[class*="absolute"]') - if (dropdownContainer) { - fireEvent.click(dropdownContainer) - // handleSelect should NOT be called because stopPropagation - expect(handleSelect).not.toHaveBeenCalled() - } + fireEvent.click(screen.getByTestId('operation-icon')) + + expect(handleSelect).not.toHaveBeenCalled() + }) + + it('should request edit without selecting the card', () => { + const handleSelect = vi.fn() + const onEdit = vi.fn() + render(, { + wrapper: createWrapper(), + }) + + fireEvent.click(screen.getByTestId('edit-btn')) + + expect(onEdit).toHaveBeenCalledWith('mcp-1') + expect(handleSelect).not.toHaveBeenCalled() }) }) - describe('Update Modal', () => { - it('should open update modal when edit button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Click the edit button - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - // Modal should be shown - await waitFor(() => { - expect(screen.getByTestId('mcp-modal')).toBeInTheDocument() - }) - }) - - it('should close update modal when close button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Open the modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-modal')).toBeInTheDocument() + describe('Delete Action', () => { + it('should request delete without selecting the card', () => { + const handleSelect = vi.fn() + const onDelete = vi.fn() + render(, { + wrapper: createWrapper(), }) - // Close the modal - const closeBtn = screen.getByTestId('modal-close-btn') - fireEvent.click(closeBtn) + fireEvent.click(screen.getByTestId('remove-btn')) - await waitFor(() => { - expect(screen.queryByTestId('mcp-modal')).not.toBeInTheDocument() - }) - }) - - it('should call updateMCP and onUpdate when form is confirmed', async () => { - const onUpdate = vi.fn() - render(, { wrapper: createWrapper() }) - - // Open the modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-modal')).toBeInTheDocument() - }) - - // Confirm the form - const confirmBtn = screen.getByTestId('modal-confirm-btn') - fireEvent.click(confirmBtn) - - await waitFor(() => { - expect(mockUpdateMCP).toHaveBeenCalledWith({ - name: 'Updated MCP', - server_url: 'https://updated.com', - provider_id: 'mcp-1', - }) - expect(onUpdate).toHaveBeenCalledWith('mcp-1') - }) - }) - - it('should not call onUpdate when updateMCP fails', async () => { - mockUpdateMCP.mockResolvedValue({ result: 'error' }) - const onUpdate = vi.fn() - render(, { wrapper: createWrapper() }) - - // Open the modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-modal')).toBeInTheDocument() - }) - - // Confirm the form - const confirmBtn = screen.getByTestId('modal-confirm-btn') - fireEvent.click(confirmBtn) - - await waitFor(() => { - expect(mockUpdateMCP).toHaveBeenCalled() - }) - - // onUpdate should not be called because result is not 'success' - expect(onUpdate).not.toHaveBeenCalled() - }) - }) - - describe('Delete Confirm', () => { - it('should open delete confirm when remove button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Click the remove button - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - // Confirm dialog should be shown - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() - }) - }) - - it('should close delete confirm when cancel button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Open the confirm dialog - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() - }) - - // Cancel - fireEvent.click(getDeleteCancelButton()) - - await waitFor(() => { - expect(screen.queryByText('tools.mcp.delete')).not.toBeInTheDocument() - }) - }) - - it('should call deleteMCP and onDeleted when delete is confirmed', async () => { - const onDeleted = vi.fn() - render(, { wrapper: createWrapper() }) - - // Open the confirm dialog - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() - }) - - // Confirm delete - fireEvent.click(getDeleteConfirmButton()) - - await waitFor(() => { - expect(mockDeleteMCP).toHaveBeenCalledWith('mcp-1') - expect(onDeleted).toHaveBeenCalled() - }) - }) - - it('should not call onDeleted when deleteMCP fails', async () => { - mockDeleteMCP.mockResolvedValue({ result: 'error' }) - const onDeleted = vi.fn() - render(, { wrapper: createWrapper() }) - - // Open the confirm dialog - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete')).toBeInTheDocument() - }) - - // Confirm delete - fireEvent.click(getDeleteConfirmButton()) - - await waitFor(() => { - expect(mockDeleteMCP).toHaveBeenCalled() - }) - - // onDeleted should not be called because result is not 'success' - expect(onDeleted).not.toHaveBeenCalled() + expect(onDelete).toHaveBeenCalledWith('mcp-1') + expect(handleSelect).not.toHaveBeenCalled() }) }) }) diff --git a/web/app/components/tools/mcp/detail/__tests__/content.spec.tsx b/web/app/components/tools/mcp/detail/__tests__/content.spec.tsx index 506c917236a..3c5809a662b 100644 --- a/web/app/components/tools/mcp/detail/__tests__/content.spec.tsx +++ b/web/app/components/tools/mcp/detail/__tests__/content.spec.tsx @@ -10,8 +10,6 @@ import MCPDetailContent from '../content' // Mutable mock functions const mockUpdateTools = vi.fn().mockResolvedValue({}) const mockAuthorizeMcp = vi.fn().mockResolvedValue({ result: 'success' }) -const mockUpdateMCP = vi.fn().mockResolvedValue({ result: 'success' }) -const mockDeleteMCP = vi.fn().mockResolvedValue({ result: 'success' }) const mockInvalidateMCPTools = vi.fn() const mockInvalidateAllMCPTools = vi.fn() const mockOpenOAuthPopup = vi.fn() @@ -44,12 +42,6 @@ vi.mock('@/service/use-tools', () => ({ mutateAsync: mockAuthorizeMcp, isPending: mockIsAuthorizing, }), - useUpdateMCP: () => ({ - mutateAsync: mockUpdateMCP, - }), - useDeleteMCP: () => ({ - mutateAsync: mockDeleteMCP, - }), })) // Mock OAuth hook @@ -58,37 +50,6 @@ vi.mock('@/hooks/use-oauth', () => ({ openOAuthPopup: (...args: OAuthArgs) => mockOpenOAuthPopup(...args), })) -// Mock MCPModal -type MCPModalData = { - name: string - server_url: string -} - -type MCPModalProps = { - show: boolean - onConfirm: (data: MCPModalData) => void - onHide: () => void -} - -vi.mock('../../modal', () => ({ - default: ({ show, onConfirm, onHide }: MCPModalProps) => { - if (!show) return null - return ( -
- - -
- ) - }, -})) - // Mock OperationDropdown vi.mock('../operation-dropdown', () => ({ default: ({ onEdit, onRemove }: { onEdit: () => void; onRemove: () => void }) => ( @@ -179,6 +140,8 @@ describe('MCPDetailContent', () => { const defaultProps = { detail: createMockDetail(), onUpdate: vi.fn(), + onEdit: vi.fn(), + onDelete: vi.fn(), onHide: vi.fn(), isTriggerAuthorize: false, onFirstCreate: vi.fn(), @@ -188,8 +151,6 @@ describe('MCPDetailContent', () => { // Reset mocks mockUpdateTools.mockClear() mockAuthorizeMcp.mockClear() - mockUpdateMCP.mockClear() - mockDeleteMCP.mockClear() mockInvalidateMCPTools.mockClear() mockInvalidateAllMCPTools.mockClear() mockOpenOAuthPopup.mockClear() @@ -197,8 +158,6 @@ describe('MCPDetailContent', () => { // Reset mock return values mockUpdateTools.mockResolvedValue({}) mockAuthorizeMcp.mockResolvedValue({ result: 'success' }) - mockUpdateMCP.mockResolvedValue({ result: 'success' }) - mockDeleteMCP.mockResolvedValue({ result: 'success' }) // Reset state mockToolsData = { tools: [] } @@ -500,170 +459,29 @@ describe('MCPDetailContent', () => { }) }) - describe('Update MCP Modal', () => { - it('should open update modal when edit button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-update-modal'))!.toBeInTheDocument() - }) - }) - - it('should close update modal when close button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Open modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-update-modal'))!.toBeInTheDocument() - }) - - // Close modal - const closeBtn = screen.getByTestId('modal-close-btn') - fireEvent.click(closeBtn) - - await waitFor(() => { - expect(screen.queryByTestId('mcp-update-modal')).not.toBeInTheDocument() - }) - }) - - it('should call updateMCP when form is confirmed', async () => { - const onUpdate = vi.fn() - render(, { + describe('Edit MCP Flow', () => { + it('should request editing the current provider', () => { + const onEdit = vi.fn() + render(, { wrapper: createWrapper(), }) - // Open modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) + fireEvent.click(screen.getByTestId('edit-btn')) - await waitFor(() => { - expect(screen.getByTestId('mcp-update-modal'))!.toBeInTheDocument() - }) - - // Confirm form - const confirmBtn = screen.getByTestId('modal-confirm-btn') - fireEvent.click(confirmBtn) - - await waitFor(() => { - expect(mockUpdateMCP).toHaveBeenCalledWith({ - name: 'Updated MCP', - server_url: 'https://updated.com', - provider_id: 'mcp-1', - }) - expect(onUpdate).toHaveBeenCalled() - }) - }) - - it('should not call onUpdate when updateMCP fails', async () => { - mockUpdateMCP.mockResolvedValue({ result: 'error' }) - const onUpdate = vi.fn() - render(, { - wrapper: createWrapper(), - }) - - // Open modal - const editBtn = screen.getByTestId('edit-btn') - fireEvent.click(editBtn) - - await waitFor(() => { - expect(screen.getByTestId('mcp-update-modal'))!.toBeInTheDocument() - }) - - // Confirm form - const confirmBtn = screen.getByTestId('modal-confirm-btn') - fireEvent.click(confirmBtn) - - await waitFor(() => { - expect(mockUpdateMCP).toHaveBeenCalled() - }) - - expect(onUpdate).not.toHaveBeenCalled() + expect(onEdit).toHaveBeenCalledWith('mcp-1') }) }) - describe('Delete MCP Flow', () => { - it('should open delete confirm when remove button is clicked', async () => { - render(, { wrapper: createWrapper() }) - - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete'))!.toBeInTheDocument() - }) - }) - - it('should close delete confirm when cancel is clicked', async () => { - render(, { wrapper: createWrapper() }) - - // Open confirm - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete'))!.toBeInTheDocument() - }) - - // Cancel - fireEvent.click(getCancelButton()) - - await waitFor(() => { - expect(screen.queryByText('tools.mcp.delete')).not.toBeInTheDocument() - }) - }) - - it('should call deleteMCP when delete is confirmed', async () => { - const onUpdate = vi.fn() - render(, { + describe('Delete MCP Action', () => { + it('should request delete for the current provider', () => { + const onDelete = vi.fn() + render(, { wrapper: createWrapper(), }) - // Open confirm - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) + fireEvent.click(screen.getByTestId('remove-btn')) - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete'))!.toBeInTheDocument() - }) - - // Confirm delete - fireEvent.click(getConfirmButton()) - - await waitFor(() => { - expect(mockDeleteMCP).toHaveBeenCalledWith('mcp-1') - expect(onUpdate).toHaveBeenCalledWith(true) - }) - }) - - it('should not call onUpdate when deleteMCP fails', async () => { - mockDeleteMCP.mockResolvedValue({ result: 'error' }) - const onUpdate = vi.fn() - render(, { - wrapper: createWrapper(), - }) - - // Open confirm - const removeBtn = screen.getByTestId('remove-btn') - fireEvent.click(removeBtn) - - await waitFor(() => { - expect(screen.getByText('tools.mcp.delete'))!.toBeInTheDocument() - }) - - // Confirm delete - fireEvent.click(getConfirmButton()) - - await waitFor(() => { - expect(mockDeleteMCP).toHaveBeenCalled() - }) - - expect(onUpdate).not.toHaveBeenCalled() + expect(onDelete).toHaveBeenCalledWith('mcp-1') }) }) diff --git a/web/app/components/tools/mcp/detail/__tests__/operation-dropdown.spec.tsx b/web/app/components/tools/mcp/detail/__tests__/operation-dropdown.spec.tsx index 64ac62962c0..d7b3f3627bd 100644 --- a/web/app/components/tools/mcp/detail/__tests__/operation-dropdown.spec.tsx +++ b/web/app/components/tools/mcp/detail/__tests__/operation-dropdown.spec.tsx @@ -123,10 +123,9 @@ describe('OperationDropdown', () => { describe('Rendering', () => { it('should render trigger button with more icon', () => { render() - const button = screen.getByTestId('dropdown-trigger') + const button = screen.getByRole('button', { name: 'common.operation.more' }) expect(button).toBeInTheDocument() - const svg = button?.querySelector('svg') - expect(svg).toBeInTheDocument() + expect(button.querySelector('.i-ri-more-fill')).toBeInTheDocument() }) it('should render medium size by default', () => { diff --git a/web/app/components/tools/mcp/detail/content.tsx b/web/app/components/tools/mcp/detail/content.tsx index c800a581372..c7f87b7b54e 100644 --- a/web/app/components/tools/mcp/detail/content.tsx +++ b/web/app/components/tools/mcp/detail/content.tsx @@ -1,5 +1,5 @@ 'use client' -import type { ComponentProps, FC } from 'react' +import type { FC } from 'react' import type { ToolWithProvider } from '../../../workflow/types' import { AlertDialog, @@ -17,7 +17,7 @@ import { Tooltip, TooltipContent, TooltipTrigger } from '@langgenius/dify-ui/too import { useBoolean } from 'ahooks' import copy from 'copy-to-clipboard' import * as React from 'react' -import { useCallback, useEffect } from 'react' +import { useCallback, useEffect, useRef } from 'react' import { useTranslation } from 'react-i18next' import ActionButton from '@/app/components/base/action-button' import Icon from '@/app/components/plugins/card/base/card-icon' @@ -25,34 +25,30 @@ import { useCanManageMCP } from '@/app/components/tools/hooks/use-tool-permissio import { openOAuthPopup } from '@/hooks/use-oauth' import { useAuthorizeMCP, - useDeleteMCP, useInvalidateAllMCPTools, useInvalidateMCPTools, useMCPTools, - useUpdateMCP, useUpdateMCPTools, } from '@/service/use-tools' -import MCPModal from '../modal' import ListLoading from './list-loading' import OperationDropdown from './operation-dropdown' import ToolItem from './tool-item' type Props = Readonly<{ detail: ToolWithProvider - onUpdate: (isDelete?: boolean) => void + onUpdate: () => void + onEdit: (providerID: string) => void + onDelete: (providerID: string) => void onHide: () => void isTriggerAuthorize: boolean onFirstCreate: () => void }> -type MCPModalConfirmPayload = Parameters['onConfirm']>[0] -type MutationResult = { - result?: string -} - const MCPDetailContent: FC = ({ detail, onUpdate, + onEdit, + onDelete, onHide, isTriggerAuthorize, onFirstCreate, @@ -89,16 +85,7 @@ const MCPDetailContent: FC = ({ updateTools, ]) - const { mutateAsync: updateMCP } = useUpdateMCP({}) - const { mutateAsync: deleteMCP } = useDeleteMCP({}) - - const [isShowUpdateModal, { setTrue: showUpdateModal, setFalse: hideUpdateModal }] = - useBoolean(false) - - const [isShowDeleteConfirm, { setTrue: showDeleteConfirm, setFalse: hideDeleteConfirm }] = - useBoolean(false) - - const [deleting, { setTrue: showDeleting, setFalse: hideDeleting }] = useBoolean(false) + const hasTriggeredAuthorizeRef = useRef(false) const handleOAuthCallback = useCallback(() => { if (!canManageMCP) return @@ -131,36 +118,12 @@ const MCPDetailContent: FC = ({ onUpdate, ]) - const handleUpdate = useCallback( - async (data: MCPModalConfirmPayload) => { - if (!canManageMCP || !detail) return - const res = (await updateMCP({ - ...data, - provider_id: detail.id, - })) as MutationResult - if (res.result === 'success') { - hideUpdateModal() - onUpdate() - handleAuthorize() - } - }, - [canManageMCP, detail, updateMCP, hideUpdateModal, onUpdate, handleAuthorize], - ) - - const handleDelete = useCallback(async () => { - if (!canManageMCP || !detail) return - showDeleting() - const res = (await deleteMCP(detail.id)) as MutationResult - hideDeleting() - if (res.result === 'success') { - hideDeleteConfirm() - onUpdate(true) - } - }, [canManageMCP, detail, showDeleting, deleteMCP, hideDeleting, hideDeleteConfirm, onUpdate]) - useEffect(() => { - if (isTriggerAuthorize) handleAuthorize() - }, []) + if (!isTriggerAuthorize || hasTriggeredAuthorizeRef.current) return + + hasTriggeredAuthorizeRef.current = true + handleAuthorize() + }, [handleAuthorize, isTriggerAuthorize]) if (!detail) return null const identifierLabel = t(($) => $['mcp.identifier'], { ns: 'tools' }) @@ -215,7 +178,10 @@ const MCPDetailContent: FC = ({
{canManageMCP && ( - + onEdit(detail.id)} + onRemove={() => onDelete(detail.id)} + /> )} $['operation.close'], { ns: 'common' })} @@ -336,37 +302,6 @@ const MCPDetailContent: FC = ({
)} - {canManageMCP && isShowUpdateModal && ( - - )} - !open && hideDeleteConfirm()} - > - -
- - {t(($) => $['mcp.delete'], { ns: 'tools' })} - -
- {t(($) => $['mcp.deleteConfirmTitle'], { ns: 'tools', mcp: detail.name })} -
-
- - - {t(($) => $['operation.cancel'], { ns: 'common' })} - - - {t(($) => $['operation.confirm'], { ns: 'common' })} - - -
-
!open && hideUpdateConfirm()} diff --git a/web/app/components/tools/mcp/detail/operation-dropdown.tsx b/web/app/components/tools/mcp/detail/operation-dropdown.tsx index aa3975cfa7b..0a9ae500ba0 100644 --- a/web/app/components/tools/mcp/detail/operation-dropdown.tsx +++ b/web/app/components/tools/mcp/detail/operation-dropdown.tsx @@ -7,7 +7,6 @@ import { DropdownMenuItem, DropdownMenuTrigger, } from '@langgenius/dify-ui/dropdown-menu' -import { RiDeleteBinLine, RiEditLine, RiMoreFill } from '@remixicon/react' import * as React from 'react' import { useTranslation } from 'react-i18next' import ActionButton from '@/app/components/base/action-button' @@ -26,14 +25,18 @@ const OperationDropdown: FC = ({ inCard, onOpenChange, onEdit, onRemove } + $['operation.more'], { ns: 'common' })} + className="data-popup-open:bg-state-base-hover" + /> } > - + - +
{t(($) => $['mcp.operation.edit'], { ns: 'tools' })}
@@ -42,7 +45,7 @@ const OperationDropdown: FC = ({ inCard, onOpenChange, onEdit, onRemove } className="data-highlighted:bg-state-destructive-hover data-highlighted:text-text-destructive" onClick={onRemove} > - +
{t(($) => $['mcp.operation.remove'], { ns: 'tools' })}
diff --git a/web/app/components/tools/mcp/detail/provider-detail.tsx b/web/app/components/tools/mcp/detail/provider-detail.tsx index 9e0ba7c6eab..4f8bd83452c 100644 --- a/web/app/components/tools/mcp/detail/provider-detail.tsx +++ b/web/app/components/tools/mcp/detail/provider-detail.tsx @@ -16,6 +16,8 @@ import MCPDetailContent from './content' type Props = Readonly<{ detail?: ToolWithProvider onUpdate: () => void + onEdit: (providerID: string) => void + onDelete: (providerID: string) => void onHide: () => void isTriggerAuthorize: boolean onFirstCreate: () => void @@ -24,15 +26,12 @@ type Props = Readonly<{ const MCPDetailPanel: FC = ({ detail, onUpdate, + onEdit, + onDelete, onHide, isTriggerAuthorize, onFirstCreate, }) => { - const handleUpdate = (isDelete = false) => { - if (isDelete) onHide() - onUpdate() - } - if (!detail) return null return ( @@ -57,7 +56,9 @@ const MCPDetailPanel: FC = ({ diff --git a/web/app/components/tools/mcp/index.tsx b/web/app/components/tools/mcp/index.tsx index d0e55942f2e..1b3338297cf 100644 --- a/web/app/components/tools/mcp/index.tsx +++ b/web/app/components/tools/mcp/index.tsx @@ -1,15 +1,27 @@ 'use client' +import type { ComponentProps } from 'react' import type { ToolsContentInset } from '../content-inset' import type { ToolWithProvider } from '@/app/components/workflow/types' +import { + AlertDialog, + AlertDialogActions, + AlertDialogCancelButton, + AlertDialogConfirmButton, + AlertDialogContent, + AlertDialogDescription, + AlertDialogTitle, +} from '@langgenius/dify-ui/alert-dialog' import { cn } from '@langgenius/dify-ui/cn' import { useEffect, useMemo, useState } from 'react' +import { useTranslation } from 'react-i18next' import { STEP_BY_STEP_TOUR_TARGETS } from '@/app/components/step-by-step-tour/target-registry' import { useCanManageMCP } from '@/app/components/tools/hooks/use-tool-permissions' import ToolCardSkeletonGrid from '@/app/components/tools/provider/tool-card-skeleton' -import { useAllToolProviders } from '@/service/use-tools' +import { useAllToolProviders, useDeleteMCP, useUpdateMCP } from '@/service/use-tools' import { toolsContentInsetClassNames, toolsUnifiedContentFrameClassName } from '../content-inset' import NewMCPCard from './create-card' import MCPDetailPanel from './detail/provider-detail' +import MCPModal from './modal' import MCPCard from './provider-card' type Props = Readonly<{ @@ -20,6 +32,11 @@ type Props = Readonly<{ showCreateCard?: boolean }> +type MCPModalConfirmPayload = Parameters['onConfirm']>[0] +type MutationResult = { + result?: string +} + const MCPList = ({ searchText, contentInset = 'default', @@ -27,6 +44,7 @@ const MCPList = ({ onCreatedProviderHandled, showCreateCard = true, }: Props) => { + const { t } = useTranslation() const canManageMCP = useCanManageMCP() const { data: list = [] as ToolWithProvider[], isLoading, refetch } = useAllToolProviders() const [isTriggerAuthorize, setIsTriggerAuthorize] = useState(false) @@ -43,10 +61,15 @@ const MCPList = ({ }, [list, searchText]) const [currentProviderID, setCurrentProviderID] = useState() + const [editingProviderID, setEditingProviderID] = useState() + const [deletingProviderID, setDeletingProviderID] = useState() - const currentProvider = useMemo(() => { - return list.find((provider) => provider.id === currentProviderID) - }, [list, currentProviderID]) + const currentProvider = list.find((provider) => provider.id === currentProviderID) + const editingProvider = list.find((provider) => provider.id === editingProviderID) + const deletingProvider = list.find((provider) => provider.id === deletingProviderID) + const detailProvider = editingProvider || deletingProvider ? undefined : currentProvider + const { mutateAsync: updateMCP } = useUpdateMCP({}) + const { mutateAsync: deleteMCP, isPending: isDeleting } = useDeleteMCP({}) const handleCreate = async (provider: ToolWithProvider) => { if (!canManageMCP) return @@ -80,12 +103,42 @@ const MCPList = ({ } }, [canManageMCP, createdProviderId, onCreatedProviderHandled, refetch]) - const handleUpdate = async (providerID: string) => { + const handleEdit = (providerID: string) => { if (!canManageMCP) return + setEditingProviderID(providerID) + } + + const handleEditConfirm = async (form: MCPModalConfirmPayload) => { + if (!canManageMCP || !editingProvider) return + + const res = (await updateMCP({ + ...form, + provider_id: editingProvider.id, + })) as MutationResult + if (res.result !== 'success') return + await refetch() // update list - setCurrentProviderID(providerID) + setCurrentProviderID(editingProvider.id) setIsTriggerAuthorize(true) + setEditingProviderID(undefined) + } + + const handleDelete = (providerID: string) => { + if (!canManageMCP) return + + setDeletingProviderID(providerID) + } + + const handleDeleteConfirm = async () => { + if (!canManageMCP || !deletingProvider) return + + const res = (await deleteMCP(deletingProvider.id)) as MutationResult + if (res.result !== 'success') return + + await refetch() + setCurrentProviderID(undefined) + setDeletingProviderID(undefined) } const contentPaddingClassName = toolsContentInsetClassNames[contentInset] const contentFrameClassName = cn(contentPaddingClassName, toolsUnifiedContentFrameClassName) @@ -111,24 +164,60 @@ const MCPList = ({ > )) )} - {currentProvider && ( + {detailProvider && ( setCurrentProviderID(undefined)} onUpdate={refetch} + onEdit={handleEdit} + onDelete={handleDelete} isTriggerAuthorize={isTriggerAuthorize} onFirstCreate={() => setIsTriggerAuthorize(false)} /> )} + {editingProvider && ( + setEditingProviderID(undefined)} + /> + )} + {deletingProvider && ( + !open && setDeletingProviderID(undefined)}> + +
+ + {t(($) => $['mcp.delete'], { ns: 'tools' })} + + + {t(($) => $['mcp.deleteConfirmTitle'], { ns: 'tools', mcp: deletingProvider.name })} + +
+ + + {t(($) => $['operation.cancel'], { ns: 'common' })} + + + {t(($) => $['operation.confirm'], { ns: 'common' })} + + +
+
+ )} ) } diff --git a/web/app/components/tools/mcp/modal.tsx b/web/app/components/tools/mcp/modal.tsx index 4e48f90918d..8400446102f 100644 --- a/web/app/components/tools/mcp/modal.tsx +++ b/web/app/components/tools/mcp/modal.tsx @@ -4,18 +4,16 @@ import type { AppIconSelection } from '@/app/components/base/app-icon-picker' import type { ToolWithProvider } from '@/app/components/workflow/types' import type { AppIconType } from '@/types/app' import { Button } from '@langgenius/dify-ui/button' -import { Dialog, DialogContent } from '@langgenius/dify-ui/dialog' +import { Dialog, DialogContent, DialogTitle } from '@langgenius/dify-ui/dialog' import { Input } from '@langgenius/dify-ui/input' import { SegmentedControl, SegmentedControlItem } from '@langgenius/dify-ui/segmented-control' import { Switch } from '@langgenius/dify-ui/switch' import { toast } from '@langgenius/dify-ui/toast' -import { RiCloseLine, RiEditLine } from '@remixicon/react' import { useSuspenseQuery } from '@tanstack/react-query' import { useHover } from 'ahooks' import { useTranslation } from 'react-i18next' import AppIcon from '@/app/components/base/app-icon' import AppIconPicker from '@/app/components/base/app-icon-picker' -import { Mcp } from '@/app/components/base/icons/src/vender/other' import { MCPAuthMethod } from '@/app/components/tools/types' import { systemFeaturesQueryOptions } from '@/features/system-features/client' import { shouldUseMcpIconForAppIcon } from '@/utils/mcp' @@ -150,13 +148,13 @@ const MCPModalContent: FC = ({ data, onConfirm, onHide }) className="absolute top-5 right-5 z-10 cursor-pointer border-none bg-transparent p-1.5 focus-visible:ring-1 focus-visible:ring-components-input-border-active focus-visible:outline-hidden" onClick={onHide} > -