mirror of
https://github.com/NanmiCoder/cc-haha
synced 2026-07-16 13:03:31 +08:00
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
This commit is contained in:
parent
d8a44c0a54
commit
8c99efca20
@ -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(<Settings />)
|
||||
|
||||
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' })
|
||||
|
||||
@ -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(() => {
|
||||
|
||||
@ -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<string | null>(null)
|
||||
const [renamingId, setRenamingId] = useState<string | null>(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() {
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
|
||||
<ConfirmDialog
|
||||
open={pendingDeleteSessionId !== null}
|
||||
onClose={() => setPendingDeleteSessionId(null)}
|
||||
onConfirm={confirmDelete}
|
||||
title={t('common.delete')}
|
||||
body={pendingDeleteSessionId ? t('sidebar.confirmDelete') : ''}
|
||||
confirmLabel={t('common.delete')}
|
||||
cancelLabel={t('common.cancel')}
|
||||
confirmVariant="danger"
|
||||
/>
|
||||
</aside>
|
||||
)
|
||||
}
|
||||
|
||||
@ -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<string | null>(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() {
|
||||
</div>
|
||||
</div>
|
||||
</section>
|
||||
|
||||
<ConfirmDialog
|
||||
open={showUninstallDialog}
|
||||
onClose={() => {
|
||||
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'}
|
||||
/>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
49
desktop/src/components/shared/ConfirmDialog.tsx
Normal file
49
desktop/src/components/shared/ConfirmDialog.tsx
Normal file
@ -0,0 +1,49 @@
|
||||
import { Modal } from './Modal'
|
||||
import { Button } from './Button'
|
||||
|
||||
type ConfirmDialogProps = {
|
||||
open: boolean
|
||||
onClose: () => void
|
||||
onConfirm: () => void | Promise<void>
|
||||
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 (
|
||||
<Modal
|
||||
open={open}
|
||||
onClose={loading ? () => {} : onClose}
|
||||
title={title}
|
||||
width={460}
|
||||
footer={(
|
||||
<>
|
||||
<Button variant="secondary" onClick={onClose} disabled={loading}>
|
||||
{cancelLabel}
|
||||
</Button>
|
||||
<Button variant={confirmVariant} onClick={() => void onConfirm()} loading={loading}>
|
||||
{confirmLabel}
|
||||
</Button>
|
||||
</>
|
||||
)}
|
||||
>
|
||||
<p className="text-sm leading-6 text-[var(--color-text-secondary)]">
|
||||
{body}
|
||||
</p>
|
||||
</Modal>
|
||||
)
|
||||
}
|
||||
@ -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',
|
||||
|
||||
@ -26,6 +26,7 @@ export const zh: Record<TranslationKey, string> = {
|
||||
'sidebar.noMatching': '没有匹配的会话',
|
||||
'sidebar.sessionListFailed': '会话列表加载失败',
|
||||
'sidebar.missingDir': '目录缺失',
|
||||
'sidebar.confirmDelete': '确定要删除这个会话吗?此操作不可撤销。',
|
||||
'sidebar.allProjects': '所有项目',
|
||||
'sidebar.other': '其他',
|
||||
'sidebar.timeGroup.today': '今天',
|
||||
|
||||
@ -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<string | null>(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() {
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<ConfirmDialog
|
||||
open={pendingUnbind !== null}
|
||||
onClose={() => {
|
||||
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}
|
||||
/>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
@ -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<SavedProvider | null>(null)
|
||||
const [showCreateModal, setShowCreateModal] = useState(false)
|
||||
const [pendingDeleteProvider, setPendingDeleteProvider] = useState<SavedProvider | null>(null)
|
||||
const [isDeletingProvider, setIsDeletingProvider] = useState(false)
|
||||
const [testResults, setTestResults] = useState<Record<string, { loading: boolean; result?: ProviderTestResult }>>({})
|
||||
|
||||
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 && (
|
||||
<ProviderFormModal key={editingProvider.id} open={true} onClose={() => setEditingProvider(null)} mode="edit" provider={editingProvider} presets={presets} />
|
||||
)}
|
||||
|
||||
<ConfirmDialog
|
||||
open={pendingDeleteProvider !== null}
|
||||
onClose={() => {
|
||||
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}
|
||||
/>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user