From 8c99efca209ed03a39dca2cd0c422500b07ac1bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Wed, 22 Apr 2026 17:30:59 +0800 Subject: [PATCH] Prevent destructive desktop actions from bypassing confirmation The desktop settings and sidebar still had deletion flows that either used browser-native confirms or deleted immediately. This change consolidates destructive confirmations behind a shared dialog component, applies it to provider deletion, plugin uninstall, adapter unbind, and sidebar session deletion, and adds regression coverage so delete actions require an explicit second confirmation before mutating state. Constraint: Other in-progress desktop work in the tree had to stay out of this commit Constraint: Existing MCP and task confirmations needed to keep their current behavior Rejected: Leave confirmations embedded per-page with browser dialogs | inconsistent UX and easy to regress Rejected: Add confirmations only to provider deletion | leaves other destructive desktop flows unsafe Confidence: high Scope-risk: narrow Reversibility: clean Directive: Any new desktop delete, uninstall, or unbind action should use the shared ConfirmDialog instead of browser-native dialogs or one-click deletion Tested: bun run test src/components/layout/Sidebar.test.tsx src/__tests__/generalSettings.test.tsx; bun run lint Not-tested: Manual desktop click-through of every destructive action after this refactor --- .../src/__tests__/generalSettings.test.tsx | 88 ++++++++++++++++--- .../src/components/layout/Sidebar.test.tsx | 15 +++- desktop/src/components/layout/Sidebar.tsx | 29 ++++-- .../src/components/plugins/PluginDetail.tsx | 28 ++++-- .../src/components/shared/ConfirmDialog.tsx | 49 +++++++++++ desktop/src/i18n/locales/en.ts | 1 + desktop/src/i18n/locales/zh.ts | 1 + desktop/src/pages/AdapterSettings.tsx | 36 +++++++- desktop/src/pages/Settings.tsx | 34 ++++++- 9 files changed, 246 insertions(+), 35 deletions(-) create mode 100644 desktop/src/components/shared/ConfirmDialog.tsx diff --git a/desktop/src/__tests__/generalSettings.test.tsx b/desktop/src/__tests__/generalSettings.test.tsx index c53c2996..a101cbcb 100644 --- a/desktop/src/__tests__/generalSettings.test.tsx +++ b/desktop/src/__tests__/generalSettings.test.tsx @@ -1,11 +1,30 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { fireEvent, render, screen } from '@testing-library/react' +import { fireEvent, render, screen, within } from '@testing-library/react' import '@testing-library/jest-dom' import { Settings } from '../pages/Settings' import { useSettingsStore } from '../stores/settingsStore' import { useUIStore } from '../stores/uiStore' import { useUpdateStore } from '../stores/updateStore' +import type { SavedProvider } from '../types/provider' + +const MOCK_DELETE_PROVIDER = vi.fn() +const providerStoreState = { + providers: [] as SavedProvider[], + activeId: null, + presets: [], + isLoading: false, + isPresetsLoading: false, + fetchProviders: vi.fn(), + fetchPresets: vi.fn(), + deleteProvider: MOCK_DELETE_PROVIDER, + activateProvider: vi.fn(), + activateOfficial: vi.fn(), + testProvider: vi.fn(), + createProvider: vi.fn(), + updateProvider: vi.fn(), + testConfig: vi.fn(), +} vi.mock('../api/agents', () => ({ agentsApi: { @@ -14,19 +33,7 @@ vi.mock('../api/agents', () => ({ })) vi.mock('../stores/providerStore', () => ({ - useProviderStore: () => ({ - providers: [], - activeId: null, - isLoading: false, - fetchProviders: vi.fn(), - deleteProvider: vi.fn(), - activateProvider: vi.fn(), - activateOfficial: vi.fn(), - testProvider: vi.fn(), - createProvider: vi.fn(), - updateProvider: vi.fn(), - testConfig: vi.fn(), - }), + useProviderStore: () => providerStoreState, })) vi.mock('../pages/AdapterSettings', () => ({ @@ -64,6 +71,21 @@ vi.mock('../components/chat/CodeViewer', () => ({ describe('Settings > General tab', () => { beforeEach(() => { + MOCK_DELETE_PROVIDER.mockReset() + providerStoreState.providers = [] + providerStoreState.activeId = null + providerStoreState.presets = [] + providerStoreState.isLoading = false + providerStoreState.isPresetsLoading = false + providerStoreState.fetchProviders = vi.fn() + providerStoreState.fetchPresets = vi.fn() + providerStoreState.activateProvider = vi.fn() + providerStoreState.activateOfficial = vi.fn() + providerStoreState.testProvider = vi.fn() + providerStoreState.createProvider = vi.fn() + providerStoreState.updateProvider = vi.fn() + providerStoreState.testConfig = vi.fn() + useSettingsStore.setState({ locale: 'en', skipWebFetchPreflight: true, @@ -111,6 +133,44 @@ describe('Settings > General tab', () => { }) }) +describe('Settings > Providers tab', () => { + beforeEach(() => { + MOCK_DELETE_PROVIDER.mockReset() + providerStoreState.providers = [ + { + id: 'provider-1', + name: 'MiniMax-M2.7-highspeed(openai)', + presetId: 'custom', + apiKey: '***', + baseUrl: 'https://api.minimaxi.com', + apiFormat: 'openai_chat', + models: { + main: 'MiniMax-M2.7-highspeed', + haiku: '', + sonnet: '', + opus: '', + }, + notes: '', + }, + ] + }) + + it('requires confirmation before deleting a provider', async () => { + render() + + fireEvent.click(screen.getAllByText('Delete')[0]!) + + expect(MOCK_DELETE_PROVIDER).not.toHaveBeenCalled() + expect(screen.getByRole('dialog')).toBeInTheDocument() + expect(screen.getByText('Delete provider "MiniMax-M2.7-highspeed(openai)"? This cannot be undone.')).toBeInTheDocument() + + const dialog = screen.getByRole('dialog') + fireEvent.click(within(dialog).getByRole('button', { name: 'Delete' })) + + expect(MOCK_DELETE_PROVIDER).toHaveBeenCalledWith('provider-1') + }) +}) + describe('Settings > About tab', () => { beforeEach(() => { useUIStore.setState({ pendingSettingsTab: 'about' }) diff --git a/desktop/src/components/layout/Sidebar.test.tsx b/desktop/src/components/layout/Sidebar.test.tsx index c98ef8fe..f380c64a 100644 --- a/desktop/src/components/layout/Sidebar.test.tsx +++ b/desktop/src/components/layout/Sidebar.test.tsx @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react' import '@testing-library/jest-dom' vi.mock('./ProjectFilter', () => ({ @@ -17,6 +17,7 @@ vi.mock('../../i18n', () => ({ 'sidebar.noMatching': 'No matching sessions', 'sidebar.sessionListFailed': 'Session list failed', 'common.retry': 'Retry', + 'common.cancel': 'Cancel', 'common.delete': 'Delete', 'common.rename': 'Rename', 'sidebar.timeGroup.today': 'Today', @@ -25,6 +26,7 @@ vi.mock('../../i18n', () => ({ 'sidebar.timeGroup.last30days': 'Last 30 Days', 'sidebar.timeGroup.older': 'Older', 'sidebar.missingDir': 'Missing', + 'sidebar.confirmDelete': 'Delete this session? This cannot be undone.', 'sidebar.collapse': 'Collapse sidebar', 'sidebar.expand': 'Expand sidebar', } @@ -121,7 +123,7 @@ describe('Sidebar', () => { expect(useTabStore.getState().tabs).toEqual([]) }) - it('removes the matching tab when deleting a session from the sidebar', async () => { + it('requires confirmation before deleting a session from the sidebar', async () => { deleteSession.mockResolvedValue(undefined) useSessionStore.setState({ sessions: [ @@ -146,8 +148,15 @@ describe('Sidebar', () => { fireEvent.contextMenu(screen.getByRole('button', { name: /Open Session/ })) + fireEvent.click(screen.getByRole('button', { name: 'Delete' })) + + expect(deleteSession).not.toHaveBeenCalled() + const dialog = screen.getByRole('dialog') + expect(dialog).toBeInTheDocument() + expect(screen.getByText('Delete this session? This cannot be undone.')).toBeInTheDocument() + await act(async () => { - fireEvent.click(screen.getByRole('button', { name: 'Delete' })) + fireEvent.click(within(dialog).getByRole('button', { name: 'Delete' })) }) await waitFor(() => { diff --git a/desktop/src/components/layout/Sidebar.tsx b/desktop/src/components/layout/Sidebar.tsx index 85ad50b8..50c0eb65 100644 --- a/desktop/src/components/layout/Sidebar.tsx +++ b/desktop/src/components/layout/Sidebar.tsx @@ -3,6 +3,7 @@ import { useSessionStore } from '../../stores/sessionStore' import { useUIStore } from '../../stores/uiStore' import { useTranslation } from '../../i18n' import { ProjectFilter } from './ProjectFilter' +import { ConfirmDialog } from '../shared/ConfirmDialog' import type { SessionListItem } from '../../types/session' import { useTabStore, SETTINGS_TAB_ID, SCHEDULED_TAB_ID } from '../../stores/tabStore' import { useChatStore } from '../../stores/chatStore' @@ -29,6 +30,7 @@ export function Sidebar() { const disconnectSession = useChatStore((s) => s.disconnectSession) const [searchQuery, setSearchQuery] = useState('') const [contextMenu, setContextMenu] = useState<{ id: string; x: number; y: number } | null>(null) + const [pendingDeleteSessionId, setPendingDeleteSessionId] = useState(null) const [renamingId, setRenamingId] = useState(null) const [renameValue, setRenameValue] = useState('') @@ -67,12 +69,18 @@ export function Sidebar() { setContextMenu({ id, x: e.clientX, y: e.clientY }) }, []) - const handleDelete = useCallback(async (id: string) => { + const handleDelete = useCallback((id: string) => { setContextMenu(null) - await deleteSession(id) - disconnectSession(id) - closeTab(id) - }, [closeTab, deleteSession, disconnectSession]) + setPendingDeleteSessionId(id) + }, []) + + const confirmDelete = useCallback(async () => { + if (!pendingDeleteSessionId) return + await deleteSession(pendingDeleteSessionId) + disconnectSession(pendingDeleteSessionId) + closeTab(pendingDeleteSessionId) + setPendingDeleteSessionId(null) + }, [closeTab, deleteSession, disconnectSession, pendingDeleteSessionId]) const handleStartRename = useCallback((id: string, currentTitle: string) => { setContextMenu(null) @@ -351,6 +359,17 @@ export function Sidebar() { )} + + setPendingDeleteSessionId(null)} + onConfirm={confirmDelete} + title={t('common.delete')} + body={pendingDeleteSessionId ? t('sidebar.confirmDelete') : ''} + confirmLabel={t('common.delete')} + cancelLabel={t('common.cancel')} + confirmVariant="danger" + /> ) } diff --git a/desktop/src/components/plugins/PluginDetail.tsx b/desktop/src/components/plugins/PluginDetail.tsx index ef0acfe2..ab91091f 100644 --- a/desktop/src/components/plugins/PluginDetail.tsx +++ b/desktop/src/components/plugins/PluginDetail.tsx @@ -4,6 +4,7 @@ import { useSessionStore } from '../../stores/sessionStore' import { useTranslation } from '../../i18n' import { useUIStore } from '../../stores/uiStore' import { Button } from '../shared/Button' +import { ConfirmDialog } from '../shared/ConfirmDialog' import type { PluginCapabilityKey } from '../../types/plugin' import { SETTINGS_TAB_ID, useTabStore } from '../../stores/tabStore' import { useSkillStore } from '../../stores/skillStore' @@ -36,6 +37,7 @@ export function PluginDetail() { const selectServer = useMcpStore((s) => s.selectServer) const t = useTranslation() const [actionKey, setActionKey] = useState(null) + const [showUninstallDialog, setShowUninstallDialog] = useState(false) const activeSession = sessions.find((session) => session.id === activeSessionId) const currentWorkDir = activeSession?.workDir || undefined @@ -98,11 +100,6 @@ export function PluginDetail() { } } - const confirmUninstall = () => { - const label = t('settings.plugins.confirmUninstall', { name: selectedPlugin.name }) - return window.confirm(label) - } - const openSettingsTab = (tab: 'skills' | 'agents' | 'mcp') => { useUIStore.getState().setPendingSettingsTab(tab) useTabStore.getState().openTab(SETTINGS_TAB_ID, 'Settings', 'settings') @@ -290,8 +287,7 @@ export function PluginDetail() { size="sm" loading={isApplying && actionKey === 'uninstall'} onClick={() => { - if (!confirmUninstall()) return - void runAction('uninstall', () => uninstallPlugin(selectedPlugin.id, selectedPlugin.scope, false, currentWorkDir)) + setShowUninstallDialog(true) }} > {t('settings.plugins.uninstall')} @@ -483,6 +479,24 @@ export function PluginDetail() { + + { + if (isApplying && actionKey === 'uninstall') return + setShowUninstallDialog(false) + }} + onConfirm={async () => { + setShowUninstallDialog(false) + await runAction('uninstall', () => uninstallPlugin(selectedPlugin.id, selectedPlugin.scope, false, currentWorkDir)) + }} + title={t('settings.plugins.uninstall')} + body={t('settings.plugins.confirmUninstall', { name: selectedPlugin.name })} + confirmLabel={t('settings.plugins.uninstall')} + cancelLabel={t('common.cancel')} + confirmVariant="danger" + loading={isApplying && actionKey === 'uninstall'} + /> ) } diff --git a/desktop/src/components/shared/ConfirmDialog.tsx b/desktop/src/components/shared/ConfirmDialog.tsx new file mode 100644 index 00000000..6284547d --- /dev/null +++ b/desktop/src/components/shared/ConfirmDialog.tsx @@ -0,0 +1,49 @@ +import { Modal } from './Modal' +import { Button } from './Button' + +type ConfirmDialogProps = { + open: boolean + onClose: () => void + onConfirm: () => void | Promise + title: string + body: string + confirmLabel: string + cancelLabel: string + confirmVariant?: 'primary' | 'danger' + loading?: boolean +} + +export function ConfirmDialog({ + open, + onClose, + onConfirm, + title, + body, + confirmLabel, + cancelLabel, + confirmVariant = 'danger', + loading = false, +}: ConfirmDialogProps) { + return ( + {} : onClose} + title={title} + width={460} + footer={( + <> + + + + )} + > +

+ {body} +

+ + ) +} diff --git a/desktop/src/i18n/locales/en.ts b/desktop/src/i18n/locales/en.ts index 3a129ac9..33004574 100644 --- a/desktop/src/i18n/locales/en.ts +++ b/desktop/src/i18n/locales/en.ts @@ -24,6 +24,7 @@ export const en = { 'sidebar.noMatching': 'No matching sessions', 'sidebar.sessionListFailed': 'Session list failed to load', 'sidebar.missingDir': 'missing dir', + 'sidebar.confirmDelete': 'Delete this session? This cannot be undone.', 'sidebar.allProjects': 'All projects', 'sidebar.other': 'Other', 'sidebar.timeGroup.today': 'Today', diff --git a/desktop/src/i18n/locales/zh.ts b/desktop/src/i18n/locales/zh.ts index 30b7b962..ca792fbb 100644 --- a/desktop/src/i18n/locales/zh.ts +++ b/desktop/src/i18n/locales/zh.ts @@ -26,6 +26,7 @@ export const zh: Record = { 'sidebar.noMatching': '没有匹配的会话', 'sidebar.sessionListFailed': '会话列表加载失败', 'sidebar.missingDir': '目录缺失', + 'sidebar.confirmDelete': '确定要删除这个会话吗？此操作不可撤销。', 'sidebar.allProjects': '所有项目', 'sidebar.other': '其他', 'sidebar.timeGroup.today': '今天', diff --git a/desktop/src/pages/AdapterSettings.tsx b/desktop/src/pages/AdapterSettings.tsx index 5526234f..bc7e6769 100644 --- a/desktop/src/pages/AdapterSettings.tsx +++ b/desktop/src/pages/AdapterSettings.tsx @@ -4,6 +4,7 @@ import { useTranslation } from '../i18n' import { Input } from '../components/shared/Input' import { Button } from '../components/shared/Button' import { DirectoryPicker } from '../components/shared/DirectoryPicker' +import { ConfirmDialog } from '../components/shared/ConfirmDialog' type ImTab = 'feishu' | 'telegram' @@ -37,6 +38,8 @@ export function AdapterSettings() { // Pairing const [pairingCode, setPairingCode] = useState(null) const [isGenerating, setIsGenerating] = useState(false) + const [pendingUnbind, setPendingUnbind] = useState<{ platform: 'telegram' | 'feishu'; userId: string | number } | null>(null) + const [isUnbinding, setIsUnbinding] = useState(false) useEffect(() => { fetchConfig() @@ -114,10 +117,20 @@ export function AdapterSettings() { }, [generatePairingCode]) const handleUnbind = useCallback(async (platform: 'telegram' | 'feishu', userId: string | number) => { - if (!confirm(t('settings.adapters.unbindConfirm'))) return - await removePairedUser(platform, userId) - await fetchConfig() - }, [removePairedUser, fetchConfig, t]) + setPendingUnbind({ platform, userId }) + }, []) + + const confirmUnbind = useCallback(async () => { + if (!pendingUnbind) return + setIsUnbinding(true) + try { + await removePairedUser(pendingUnbind.platform, pendingUnbind.userId) + await fetchConfig() + setPendingUnbind(null) + } finally { + setIsUnbinding(false) + } + }, [pendingUnbind, removePairedUser, fetchConfig]) // Collect all paired users across platforms const allPairedUsers = [ @@ -344,6 +357,21 @@ export function AdapterSettings() { )} + + { + if (isUnbinding) return + setPendingUnbind(null) + }} + onConfirm={confirmUnbind} + title={t('settings.adapters.unbind')} + body={t('settings.adapters.unbindConfirm')} + confirmLabel={t('settings.adapters.unbind')} + cancelLabel={t('common.cancel')} + confirmVariant="danger" + loading={isUnbinding} + /> ) } diff --git a/desktop/src/pages/Settings.tsx b/desktop/src/pages/Settings.tsx index dfc85afc..643d894c 100644 --- a/desktop/src/pages/Settings.tsx +++ b/desktop/src/pages/Settings.tsx @@ -3,6 +3,7 @@ import { useSettingsStore } from '../stores/settingsStore' import { useProviderStore } from '../stores/providerStore' import { useTranslation } from '../i18n' import { Modal } from '../components/shared/Modal' +import { ConfirmDialog } from '../components/shared/ConfirmDialog' import { Input } from '../components/shared/Input' import { Button } from '../components/shared/Button' import type { PermissionMode, EffortLevel, ThemeMode } from '../types/settings' @@ -116,6 +117,8 @@ function ProviderSettings() { const t = useTranslation() const [editingProvider, setEditingProvider] = useState(null) const [showCreateModal, setShowCreateModal] = useState(false) + const [pendingDeleteProvider, setPendingDeleteProvider] = useState(null) + const [isDeletingProvider, setIsDeletingProvider] = useState(false) const [testResults, setTestResults] = useState>({}) useEffect(() => { @@ -130,8 +133,20 @@ function ProviderSettings() { const handleDelete = async (provider: SavedProvider) => { if (activeId === provider.id) return - if (!window.confirm(t('settings.providers.confirmDelete', { name: provider.name }))) return - await deleteProvider(provider.id).catch(console.error) + setPendingDeleteProvider(provider) + } + + const confirmDelete = async () => { + if (!pendingDeleteProvider) return + setIsDeletingProvider(true) + try { + await deleteProvider(pendingDeleteProvider.id) + setPendingDeleteProvider(null) + } catch (error) { + console.error(error) + } finally { + setIsDeletingProvider(false) + } } const handleTest = async (provider: SavedProvider) => { @@ -281,6 +296,21 @@ function ProviderSettings() { {editingProvider && ( setEditingProvider(null)} mode="edit" provider={editingProvider} presets={presets} /> )} + + { + if (isDeletingProvider) return + setPendingDeleteProvider(null) + }} + onConfirm={confirmDelete} + title={t('common.delete')} + body={pendingDeleteProvider ? t('settings.providers.confirmDelete', { name: pendingDeleteProvider.name }) : ''} + confirmLabel={t('common.delete')} + cancelLabel={t('common.cancel')} + confirmVariant="danger" + loading={isDeletingProvider} + /> ) }