From bb773bc768051aa802dd7cfa2bc4dcf901acb360 Mon Sep 17 00:00:00 2001 From: Agastya Darma Date: Wed, 29 Jul 2026 00:00:48 +0900 Subject: [PATCH] feat(WorktreeRow): add loading state and safety guards to worktree deletion - Prevent duplicate deletion requests via in-flight ref and early returns - Show spinner during deletion; disable interactions (drag, click, context menu) - Persist "don't ask again" only on successful deletion, not on failure - Replace raw buttons with Button component for consistent loading indicator --- .../components/Sidebar/WorktreeRow.tsx | 127 +++++++++---- .../Sidebar/__tests__/WorktreeRow.test.tsx | 174 ++++++++++++++++++ 2 files changed, 261 insertions(+), 40 deletions(-) create mode 100644 src/renderer/components/Sidebar/__tests__/WorktreeRow.test.tsx diff --git a/src/renderer/components/Sidebar/WorktreeRow.tsx b/src/renderer/components/Sidebar/WorktreeRow.tsx index 5b67f7e..86e00eb 100644 --- a/src/renderer/components/Sidebar/WorktreeRow.tsx +++ b/src/renderer/components/Sidebar/WorktreeRow.tsx @@ -5,7 +5,7 @@ import { PrIcon } from './PrIcon' import { useUIStore } from '@/store/ui' import { useProjectsStore } from '@/store/projects' import { Tooltip } from '@/components/shared/Tooltip' -import { Checkbox } from '@/components/ui' +import { Button, Checkbox, Spinner } from '@/components/ui' import { ContextMenu, type ContextMenuItem } from '@/components/shared/ContextMenu' import { useSessionsForWorktree } from '@/store/sessions' import { useTranslation } from 'react-i18next' @@ -25,13 +25,19 @@ interface Props { onRegisterRef?: (el: HTMLElement | null) => void } -type RowState = { menu: { x: number; y: number } | null; showDeleteConfirm: boolean; dontAskAgain: boolean } +type RowState = { + menu: { x: number; y: number } | null + showDeleteConfirm: boolean + dontAskAgain: boolean + isDeleting: boolean +} type RowAction = | { type: 'OPEN_MENU'; x: number; y: number } | { type: 'CLOSE_MENU' } | { type: 'SHOW_DELETE_CONFIRM' } | { type: 'HIDE_DELETE_CONFIRM' } | { type: 'SET_DONT_ASK'; value: boolean } + | { type: 'SET_DELETING'; value: boolean } function rowReducer(state: RowState, action: RowAction): RowState { switch (action.type) { @@ -40,6 +46,7 @@ function rowReducer(state: RowState, action: RowAction): RowState { case 'SHOW_DELETE_CONFIRM': return { ...state, showDeleteConfirm: true } case 'HIDE_DELETE_CONFIRM': return { ...state, showDeleteConfirm: false } case 'SET_DONT_ASK': return { ...state, dontAskAgain: action.value } + case 'SET_DELETING': return { ...state, isDeleting: action.value } } } @@ -82,6 +89,7 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused const clearNewlyAdded = useUIStore((s) => s.setNewlyAddedWorktreeId) const rowRef = useRef(null) + const deleteInFlightRef = useRef(false) useEffect(() => { const el = rowRef.current @@ -103,8 +111,13 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused }, [isNew, clearNewlyAdded]) // Feature 3: worktree context menu - const [rowState, rowDispatch] = useReducer(rowReducer, { menu: null, showDeleteConfirm: false, dontAskAgain: false }) - const { menu, showDeleteConfirm, dontAskAgain } = rowState + const [rowState, rowDispatch] = useReducer(rowReducer, { + menu: null, + showDeleteConfirm: false, + dontAskAgain: false, + isDeleting: false, + }) + const { menu, showDeleteConfirm, dontAskAgain, isDeleting } = rowState // Priority: permission > working > done > active > inactive let status: WorktreeStatus = 'inactive' @@ -114,16 +127,28 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused else if (hasDoneAgent) status = 'done' else if (sessions.some((s) => s.status === 'idle')) status = 'active' - const runDeleteWorktree = () => { - removeWorktree(worktree.projectId, worktree.id).catch((err) => { + const runDeleteWorktree = async (skipFutureConfirmations = false) => { + if (deleteInFlightRef.current) return + + deleteInFlightRef.current = true + rowDispatch({ type: 'SET_DELETING', value: true }) + try { + await removeWorktree(worktree.projectId, worktree.id) + if (skipFutureConfirmations) setSkipDeleteConfirm(true) + rowDispatch({ type: 'HIDE_DELETE_CONFIRM' }) + } catch (err) { console.error('[WorktreeRow] removeWorktree failed:', err) flash('error', cleanIpcError(err, t('deleteWorktreeFailed')), 5_000) - }) + } finally { + deleteInFlightRef.current = false + rowDispatch({ type: 'SET_DELETING', value: false }) + } } const requestDeleteWorktree = () => { + if (deleteInFlightRef.current) return if (skipDeleteConfirm) { - runDeleteWorktree() + void runDeleteWorktree() } else { rowDispatch({ type: 'SHOW_DELETE_CONFIRM' }) } @@ -163,12 +188,18 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused .join(' ')} role="option" aria-selected={isSelected} + aria-busy={isDeleting} tabIndex={isFocused ? 0 : -1} - draggable={draggable} + draggable={draggable && !isDeleting} data-worktree-id={worktree.id} - onClick={() => selectWorktree(worktree.projectId, worktree.id)} - onDoubleClick={() => setMissionControlActive(false)} + onClick={() => { + if (!isDeleting) selectWorktree(worktree.projectId, worktree.id) + }} + onDoubleClick={() => { + if (!isDeleting) setMissionControlActive(false) + }} onKeyDown={(e) => { + if (isDeleting) return if (e.key === 'Enter' || e.key === ' ') { e.preventDefault() e.stopPropagation() @@ -181,6 +212,7 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused onContextMenu={(e) => { e.preventDefault() e.stopPropagation() + if (isDeleting) return rowDispatch({ type: 'OPEN_MENU', x: e.clientX, y: e.clientY }) }} > @@ -210,26 +242,32 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused
- - {worktree.isMain && ( - - {t('worktreeMainBadge')} - - )} + {isDeleting ? ( + + ) : ( + <> + + {worktree.isMain && ( + + {t('worktreeMainBadge')} + + )} - {/* Feature 1: pin / star button */} - - - + {/* Feature 1: pin / star button */} + + + + + )}
@@ -244,7 +282,12 @@ export function WorktreeRow({ worktree, dragOverId, draggingId, isNew, isFocused )} {showDeleteConfirm && ( -
rowDispatch({ type: 'HIDE_DELETE_CONFIRM' })}> +
{ + if (!isDeleting) rowDispatch({ type: 'HIDE_DELETE_CONFIRM' }) + }} + >
rowDispatch({ type: 'SET_DONT_ASK', value: checked })} + disabled={isDeleting} label={t('deleteWorktreeDontAsk')} />
- {/* autoFocus the cancel button — the safe non-destructive action */} - - +
diff --git a/src/renderer/components/Sidebar/__tests__/WorktreeRow.test.tsx b/src/renderer/components/Sidebar/__tests__/WorktreeRow.test.tsx new file mode 100644 index 0000000..2cf8274 --- /dev/null +++ b/src/renderer/components/Sidebar/__tests__/WorktreeRow.test.tsx @@ -0,0 +1,174 @@ +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Worktree } from '@/types' +import { WorktreeRow } from '../WorktreeRow' + +const mocks = vi.hoisted(() => ({ + removeWorktree: vi.fn(), + selectWorktree: vi.fn(), + setMissionControlActive: vi.fn(), + togglePinWorktree: vi.fn(), + setSkipDeleteWorktreeConfirm: vi.fn(), + setNewlyAddedWorktreeId: vi.fn(), + flash: vi.fn(), + skipDeleteWorktreeConfirm: false, +})) + +function deferred() { + let resolve!: (value: T) => void + let reject!: (reason?: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + + return { promise, resolve, reject } +} + +vi.mock('@/store/projects', () => ({ + useProjectsStore: vi.fn((selector: (state: { removeWorktree: typeof mocks.removeWorktree }) => unknown) => + selector({ removeWorktree: mocks.removeWorktree }) + ), +})) + +vi.mock('@/store/ui', () => { + const state = { + selectedWorktreeId: null, + selectWorktree: mocks.selectWorktree, + setMissionControlActive: mocks.setMissionControlActive, + pinnedWorktrees: new Set(), + togglePinWorktree: mocks.togglePinWorktree, + get skipDeleteWorktreeConfirm() { return mocks.skipDeleteWorktreeConfirm }, + setSkipDeleteWorktreeConfirm: mocks.setSkipDeleteWorktreeConfirm, + bigTerminalsByWorktree: {}, + bigTerminalStatusById: {}, + setNewlyAddedWorktreeId: mocks.setNewlyAddedWorktreeId, + } + + return { + useUIStore: vi.fn((selector: (value: typeof state) => unknown) => selector(state)), + } +}) + +vi.mock('@/store/sessions', () => ({ + useSessionsForWorktree: () => [], +})) + +vi.mock('@/store/flash', () => ({ flash: mocks.flash })) + +vi.mock('@/lib/ipc', () => ({ + cleanIpcError: (_error: unknown, fallback: string) => fallback, +})) + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})) + +vi.mock('../StatusDot', () => ({ StatusDot: () => })) +vi.mock('../PrIcon', () => ({ PrIcon: () => })) +vi.mock('@/components/shared/Tooltip', () => ({ + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, +})) + +const worktree: Worktree = { + id: 'wt-feature', + projectId: 'project-1', + branch: 'feature/loading', + path: '/repo/feature-loading', + isMain: false, + sessions: [], +} + +function renderRow() { + return render( + + ) +} + +function openDeleteDialog() { + const row = screen.getByRole('option') + fireEvent.keyDown(row, { key: 'Delete' }) + return screen.getByRole('dialog') +} + +describe('WorktreeRow deletion', () => { + afterEach(() => { + cleanup() + }) + + beforeEach(() => { + vi.clearAllMocks() + mocks.skipDeleteWorktreeConfirm = false + mocks.removeWorktree.mockResolvedValue(undefined) + }) + + it('keeps the confirmation open and shows loading state while deleting', async () => { + const removal = deferred() + mocks.removeWorktree.mockReturnValue(removal.promise) + renderRow() + + const dialog = openDeleteDialog() + const cancelButton = within(dialog).getByRole('button', { name: 'cancel' }) as HTMLButtonElement + const deleteButton = within(dialog).getByRole('button', { name: 'deleteWorktreeConfirm' }) as HTMLButtonElement + const checkbox = within(dialog).getByRole('checkbox') as HTMLInputElement + + fireEvent.click(checkbox) + fireEvent.click(deleteButton) + + await waitFor(() => expect(mocks.removeWorktree).toHaveBeenCalledWith('project-1', 'wt-feature')) + expect(mocks.setSkipDeleteWorktreeConfirm).not.toHaveBeenCalled() + expect(screen.getByRole('dialog')).toBe(dialog) + expect(cancelButton.disabled).toBe(true) + expect(deleteButton.disabled).toBe(true) + expect(checkbox.disabled).toBe(true) + expect(within(deleteButton).getByRole('status')).toBeTruthy() + + await act(async () => removal.resolve()) + + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()) + expect(mocks.setSkipDeleteWorktreeConfirm).toHaveBeenCalledWith(true) + }) + + it('does not persist the confirmation opt-out when deletion fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + mocks.removeWorktree.mockRejectedValue(new Error('deletion failed')) + renderRow() + + const dialog = openDeleteDialog() + const checkbox = within(dialog).getByRole('checkbox') as HTMLInputElement + const deleteButton = within(dialog).getByRole('button', { name: 'deleteWorktreeConfirm' }) + + fireEvent.click(checkbox) + fireEvent.click(deleteButton) + + await waitFor(() => expect(mocks.flash).toHaveBeenCalled()) + expect(screen.getByRole('dialog')).toBe(dialog) + expect(checkbox.disabled).toBe(false) + expect(mocks.setSkipDeleteWorktreeConfirm).not.toHaveBeenCalled() + + consoleError.mockRestore() + }) + + it('shows a row-level spinner when confirmation is skipped', async () => { + const removal = deferred() + mocks.removeWorktree.mockReturnValue(removal.promise) + mocks.skipDeleteWorktreeConfirm = true + renderRow() + + const row = screen.getByRole('option') + fireEvent.keyDown(row, { key: 'Delete' }) + + await waitFor(() => expect(row.getAttribute('aria-busy')).toBe('true')) + expect(within(row).getByRole('status')).toBeTruthy() + expect(row.getAttribute('draggable')).toBe('false') + + await act(async () => removal.resolve()) + + await waitFor(() => expect(row.getAttribute('aria-busy')).toBe('false')) + }) +})