[Phase 3] Task Board: bulk stop, reset, and state management actions

docs/code-review/2026-02-19-task-board-bulk-actions-review.md

@@ -0,0 +1,127 @@
+# Code Review Report: Task Board Bulk Stop, Reset & State Management
+
+**Date:** 2026-02-19
+**Reviewer:** Code Review Agent
+**Component:** Task Board (web-ui/src/components/tasks/)
+**PR:** #389 (feature/issue-340-task-board-bulk-actions)
+**Files Reviewed:** 6 production files, 5 test files
+**Ready for Production:** Yes
+
+## Executive Summary
+
+Well-structured feature addition that extends the Task Board with single-task and bulk stop/reset actions. Three rounds of review (CodeRabbit, Claude bot, manual) identified and resolved all issues. Final code is clean, accessible, and well-tested.
+
+**Critical Issues:** 0
+**Major Issues:** 0 (all resolved)
+**Minor Issues:** 2 (accepted trade-offs, documented below)
+**Positive Findings:** 8
+
+---
+
+## Review Context
+
+**Code Type:** UI Components (React/Next.js)
+**Risk Level:** Low-Medium (UI-only, calls existing backend APIs)
+**Business Constraints:** Standard feature, no performance-critical requirements
+
+### Review Focus Areas
+
+- ✅ A04 Insecure Design — Workflow race conditions, state consistency
+- ✅ Reliability — Error handling, async cleanup, loading states
+- ✅ Maintainability — Type safety, test coverage, code patterns
+- ❌ A01 Access Control — Skipped, frontend doesn't enforce auth
+- ❌ A03 Injection — Skipped, no raw user input to backend
+- ❌ LLM/ML Security — Not applicable
+
+---
+
+## Priority 1 Issues - Critical
+
+None.
+
+---
+
+## Priority 2 Issues - Major (All Resolved)
+
+### 1. Loading state cleared before SWR refresh (RESOLVED)
+**Location:** `TaskBoardView.tsx:237-268`
+**Fix:** Wrapped `handleConfirmAction` in try/finally, loading flags reset in finally block.
+
+### 2. Missing try/finally could leave dialog stuck (RESOLVED)
+**Location:** `TaskBoardView.tsx:237-268`
+**Fix:** Same try/finally restructure.
+
+### 3. selectedTasks scope inconsistency with filters (RESOLVED)
+**Location:** `TaskBoardView.tsx:83-86`
+**Fix:** `selectedTasks` now derives from `data?.tasks` instead of `filteredTasks`.
+
+### 4. Tests relied on mock-specific data-testid (RESOLVED)
+**Location:** `TaskCard.test.tsx`
+**Fix:** Replaced `getByTestId('icon-Loading03Icon')` with `getByRole('status', { name: /loading/i })`.
+
+### 5. SWR revalidation race condition on confirmation count (RESOLVED)
+**Location:** `TaskBoardView.tsx:227-235`
+**Fix:** Task IDs frozen into `confirmAction` state at dialog-open time.
+
+### 6. BulkActionType included unreachable 'execute' variant (RESOLVED)
+**Location:** `BulkActionConfirmDialog.tsx:15`
+**Fix:** Narrowed type to `'stop' | 'reset'`, removed dead config entry.
+
+### 7. Missing test for partial failure in batch operations (RESOLVED)
+**Location:** `TaskBoardView.test.tsx`
+**Fix:** Added test that mocks `stopExecution` rejection and verifies error banner.
+
+---
+
+## Priority 3 Issues - Minor
+
+### 1. Unsafe error cast `err as ApiError`
+**Location:** `TaskBoardView.tsx:173, 194`
+**Category:** Type Safety
+
+The `err as ApiError` pattern doesn't guarantee `.detail` exists on non-API errors (e.g., TypeError). However, the fallback string always fires for non-API errors since `undefined || 'fallback'` resolves correctly. This is also the pre-existing pattern used throughout the codebase.
+
+**Decision:** Accepted. Functionally correct. Fixing only in new code would be inconsistent.
+
+### 2. handleClearSelection fires on partial failure
+**Location:** `TaskBoardView.tsx:265`
+**Category:** UX
+
+When 3 of 5 bulk-stop calls fail, selection is cleared, losing visibility into which tasks were affected. The error message does report failure count.
+
+**Decision:** Accepted trade-off for v1. Retaining selection on partial failure would require tracking per-task success/failure state.
+
+---
+
+## Positive Findings
+
+1. **Race condition mitigation**: Freezing `taskIds` into `confirmAction` at dialog-open time isolates batch from SWR revalidation mid-flight.
+2. **Promise.allSettled**: Correct choice for bulk ops — partial failures handled gracefully.
+3. **AlertDialog over Dialog**: Proper Radix primitive for destructive confirmations (focus trap, explicit dismiss).
+4. **Accessibility**: `role="alert"` on error banner, `role="status"` on spinner, dismiss button, keyboard nav preserved, ARIA labels on all interactive elements.
+5. **e.preventDefault() on AlertDialogAction**: Prevents auto-close before async confirm completes.
+6. **Per-task loading state**: `loadingTaskIds` Set prevents double-clicks and gives clear visual feedback.
+7. **useCallback/useMemo throughout**: Handlers are stable references, derived state is memoized.
+8. **Comprehensive test coverage**: 302 tests covering happy paths, error paths, loading states, accessibility, and now partial failures.
+
+---
+
+## Action Items Summary
+
+### Immediate (Before Merge)
+All resolved.
+
+### Short-term (Backlog)
+1. Consider retaining selection on partial bulk-action failure
+2. Consider a shared `extractErrorDetail(err)` utility for safer error extraction
+
+### Long-term (Backlog)
+1. Shift-click range selection for task checkboxes (deferred from issue #340)
+
+---
+
+## Conclusion
+
+All critical and major issues from three review rounds have been resolved. The implementation follows existing codebase patterns, uses proper Shadcn/UI primitives, maintains accessibility standards, and has thorough test coverage including edge cases. Ready to merge.
+
+**Recommendation:** Deploy

web-ui/__mocks__/@hugeicons/react.js

@@ -40,6 +40,7 @@ module.exports = {
   // Task Board components
   PlayCircleIcon: createIconMock('PlayCircleIcon'),
   LinkCircleIcon: createIconMock('LinkCircleIcon'),
+  ViewIcon: createIconMock('ViewIcon'),
   Search01Icon: createIconMock('Search01Icon'),
   CheckListIcon: createIconMock('CheckListIcon'),
   // Execution Monitor components

web-ui/__tests__/components/tasks/BatchActionsBar.test.tsx

@@ -0,0 +1,187 @@
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { BatchActionsBar } from '@/components/tasks/BatchActionsBar';
+import type { Task } from '@/types';
+
+function makeTask(overrides: Partial<Task> = {}): Task {
+  return {
+    id: 'task-1',
+    title: 'Test Task',
+    description: 'A test task',
+    status: 'READY',
+    priority: 1,
+    depends_on: [],
+    ...overrides,
+  };
+}
+
+const defaultProps = {
+  selectionMode: true,
+  onToggleSelectionMode: jest.fn(),
+  selectedCount: 0,
+  strategy: 'serial' as const,
+  onStrategyChange: jest.fn(),
+  onExecuteBatch: jest.fn(),
+  onClearSelection: jest.fn(),
+  isExecuting: false,
+  selectedTasks: [] as Task[],
+  onStopBatch: jest.fn(),
+  onResetBatch: jest.fn(),
+  isStoppingBatch: false,
+  isResettingBatch: false,
+};
+
+beforeEach(() => {
+  jest.clearAllMocks();
+});
+
+describe('BatchActionsBar', () => {
+  it('shows Batch button when not in selection mode', () => {
+    render(<BatchActionsBar {...defaultProps} selectionMode={false} />);
+    expect(screen.getByRole('button', { name: /batch/i })).toBeInTheDocument();
+  });
+
+  it('shows Cancel button when in selection mode', () => {
+    render(<BatchActionsBar {...defaultProps} selectionMode={true} />);
+    expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
+  });
+
+  it('shows Execute button with count when READY tasks are selected', () => {
+    const selectedTasks = [
+      makeTask({ id: 't1', status: 'READY' }),
+      makeTask({ id: 't2', status: 'READY' }),
+    ];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={2}
+        selectedTasks={selectedTasks}
+      />
+    );
+    expect(screen.getByRole('button', { name: /execute 2/i })).toBeInTheDocument();
+  });
+
+  it('shows Stop button with count when IN_PROGRESS tasks are selected', () => {
+    const selectedTasks = [
+      makeTask({ id: 't1', status: 'IN_PROGRESS' }),
+      makeTask({ id: 't2', status: 'IN_PROGRESS' }),
+      makeTask({ id: 't3', status: 'READY' }),
+    ];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={3}
+        selectedTasks={selectedTasks}
+      />
+    );
+    expect(screen.getByRole('button', { name: /stop 2/i })).toBeInTheDocument();
+  });
+
+  it('shows Reset button with count when FAILED tasks are selected', () => {
+    const selectedTasks = [
+      makeTask({ id: 't1', status: 'FAILED' }),
+      makeTask({ id: 't2', status: 'FAILED' }),
+      makeTask({ id: 't3', status: 'FAILED' }),
+    ];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={3}
+        selectedTasks={selectedTasks}
+      />
+    );
+    expect(screen.getByRole('button', { name: /reset 3/i })).toBeInTheDocument();
+  });
+
+  it('shows multiple action buttons for mixed selection', () => {
+    const selectedTasks = [
+      makeTask({ id: 't1', status: 'READY' }),
+      makeTask({ id: 't2', status: 'IN_PROGRESS' }),
+      makeTask({ id: 't3', status: 'FAILED' }),
+    ];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={3}
+        selectedTasks={selectedTasks}
+      />
+    );
+    expect(screen.getByRole('button', { name: /execute 1/i })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: /stop 1/i })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: /reset 1/i })).toBeInTheDocument();
+  });
+
+  it('calls onStopBatch when Stop button is clicked', async () => {
+    const user = userEvent.setup();
+    const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={1}
+        selectedTasks={selectedTasks}
+      />
+    );
+    await user.click(screen.getByRole('button', { name: /stop 1/i }));
+    expect(defaultProps.onStopBatch).toHaveBeenCalledTimes(1);
+  });
+
+  it('calls onResetBatch when Reset button is clicked', async () => {
+    const user = userEvent.setup();
+    const selectedTasks = [makeTask({ id: 't1', status: 'FAILED' })];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={1}
+        selectedTasks={selectedTasks}
+      />
+    );
+    await user.click(screen.getByRole('button', { name: /reset 1/i }));
+    expect(defaultProps.onResetBatch).toHaveBeenCalledTimes(1);
+  });
+
+  it('disables Stop button when isStoppingBatch is true', () => {
+    const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={1}
+        selectedTasks={selectedTasks}
+        isStoppingBatch={true}
+      />
+    );
+    expect(screen.getByRole('button', { name: /stop/i })).toBeDisabled();
+  });
+
+  it('disables Reset button when isResettingBatch is true', () => {
+    const selectedTasks = [makeTask({ id: 't1', status: 'FAILED' })];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={1}
+        selectedTasks={selectedTasks}
+        isResettingBatch={true}
+      />
+    );
+    expect(screen.getByRole('button', { name: /reset/i })).toBeDisabled();
+  });
+
+  it('shows strategy selector only when READY tasks are selected', () => {
+    const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
+    render(
+      <BatchActionsBar
+        {...defaultProps}
+        selectedCount={1}
+        selectedTasks={selectedTasks}
+      />
+    );
+    // Strategy selector should not appear when no READY tasks
+    expect(screen.queryByText('Serial')).not.toBeInTheDocument();
+  });
+
+  it('hides action buttons when no tasks are selected', () => {
+    render(<BatchActionsBar {...defaultProps} selectedCount={0} selectedTasks={[]} />);
+    expect(screen.queryByRole('button', { name: /execute/i })).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: /stop/i })).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: /reset/i })).not.toBeInTheDocument();
+  });
+});

web-ui/__tests__/components/tasks/BulkActionConfirmDialog.test.tsx

@@ -0,0 +1,61 @@
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { BulkActionConfirmDialog } from '@/components/tasks/BulkActionConfirmDialog';
+
+const defaultProps = {
+  open: true,
+  onOpenChange: jest.fn(),
+  actionType: 'stop' as const,
+  taskCount: 3,
+  onConfirm: jest.fn(),
+  isLoading: false,
+};
+
+beforeEach(() => {
+  jest.clearAllMocks();
+});
+
+describe('BulkActionConfirmDialog', () => {
+  it('renders stop confirmation with correct title and description', () => {
+    render(<BulkActionConfirmDialog {...defaultProps} actionType="stop" taskCount={2} />);
+    expect(screen.getByText('Stop Tasks')).toBeInTheDocument();
+    expect(screen.getByText(/stop 2 running task\(s\)/i)).toBeInTheDocument();
+  });
+
+  it('renders reset confirmation with correct title and description', () => {
+    render(<BulkActionConfirmDialog {...defaultProps} actionType="reset" taskCount={4} />);
+    expect(screen.getByText('Reset Tasks')).toBeInTheDocument();
+    expect(screen.getByText(/reset 4 failed task\(s\)/i)).toBeInTheDocument();
+  });
+
+  it('calls onConfirm when confirm button is clicked', async () => {
+    const user = userEvent.setup();
+    render(<BulkActionConfirmDialog {...defaultProps} />);
+    await user.click(screen.getByRole('button', { name: /confirm/i }));
+    expect(defaultProps.onConfirm).toHaveBeenCalledTimes(1);
+  });
+
+  it('calls onOpenChange(false) when cancel button is clicked', async () => {
+    const user = userEvent.setup();
+    render(<BulkActionConfirmDialog {...defaultProps} />);
+    await user.click(screen.getByRole('button', { name: /cancel/i }));
+    expect(defaultProps.onOpenChange).toHaveBeenCalledWith(false);
+  });
+
+  it('disables confirm button when isLoading is true', () => {
+    render(<BulkActionConfirmDialog {...defaultProps} isLoading={true} />);
+    const confirmBtn = screen.getByRole('button', { name: /confirm/i });
+    expect(confirmBtn).toBeDisabled();
+  });
+
+  it('does not render when open is false', () => {
+    render(<BulkActionConfirmDialog {...defaultProps} open={false} />);
+    expect(screen.queryByText('Stop Tasks')).not.toBeInTheDocument();
+  });
+
+  it('shows destructive styling for stop action confirm button', () => {
+    render(<BulkActionConfirmDialog {...defaultProps} actionType="stop" />);
+    const confirmBtn = screen.getByRole('button', { name: /confirm/i });
+    expect(confirmBtn).toHaveClass('bg-destructive');
+  });
+});