Skip to content

Commit f871452

Browse files
frankbriafrankbria
authored
feat: Task Board bulk stop, reset & state management (#340)
## Summary - Single-task Stop (IN_PROGRESS) and Reset (FAILED) buttons on task cards - Bulk stop/reset with checkbox selection and confirmation dialogs - Column-level Select All checkboxes - Per-task loading states with accessible spinners - Context-aware batch action bar (shows actions based on selected task statuses) - View Execution navigation for IN_PROGRESS tasks - Error banner with role="alert" and dismiss button - Race condition mitigation: task IDs frozen at dialog-open time - 303 tests passing ## Acceptance Criteria (Issue #340) - [x] Clicking an IN_PROGRESS task navigates to the Execution Monitor - [x] IN_PROGRESS tasks show a Stop action in the task card - [x] FAILED tasks show a Reset to Ready action in the task card - [x] Users can select multiple tasks via checkboxes - [x] Bulk action bar appears with context-appropriate actions - [x] Confirmation dialogs for destructive bulk actions (stop, reset) - [x] All actions work through the existing v2 API Closes #340
1 parent bd05c0e commit f871452

15 files changed

Lines changed: 1265 additions & 63 deletions

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

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
# Code Review Report: Task Board Bulk Stop, Reset & State Management
2+
3+
**Date:** 2026-02-19
4+
**Reviewer:** Code Review Agent
5+
**Component:** Task Board (web-ui/src/components/tasks/)
6+
**PR:** #389 (feature/issue-340-task-board-bulk-actions)
7+
**Files Reviewed:** 6 production files, 5 test files
8+
**Ready for Production:** Yes
9+
10+
## Executive Summary
11+
12+
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.
13+
14+
**Critical Issues:** 0
15+
**Major Issues:** 0 (all resolved)
16+
**Minor Issues:** 2 (accepted trade-offs, documented below)
17+
**Positive Findings:** 8
18+
19+
---
20+
21+
## Review Context
22+
23+
**Code Type:** UI Components (React/Next.js)
24+
**Risk Level:** Low-Medium (UI-only, calls existing backend APIs)
25+
**Business Constraints:** Standard feature, no performance-critical requirements
26+
27+
### Review Focus Areas
28+
29+
- ✅ A04 Insecure Design — Workflow race conditions, state consistency
30+
- ✅ Reliability — Error handling, async cleanup, loading states
31+
- ✅ Maintainability — Type safety, test coverage, code patterns
32+
- ❌ A01 Access Control — Skipped, frontend doesn't enforce auth
33+
- ❌ A03 Injection — Skipped, no raw user input to backend
34+
- ❌ LLM/ML Security — Not applicable
35+
36+
---
37+
38+
## Priority 1 Issues - Critical
39+
40+
None.
41+
42+
---
43+
44+
## Priority 2 Issues - Major (All Resolved)
45+
46+
### 1. Loading state cleared before SWR refresh (RESOLVED)
47+
**Location:** `TaskBoardView.tsx:237-268`
48+
**Fix:** Wrapped `handleConfirmAction` in try/finally, loading flags reset in finally block.
49+
50+
### 2. Missing try/finally could leave dialog stuck (RESOLVED)
51+
**Location:** `TaskBoardView.tsx:237-268`
52+
**Fix:** Same try/finally restructure.
53+
54+
### 3. selectedTasks scope inconsistency with filters (RESOLVED)
55+
**Location:** `TaskBoardView.tsx:83-86`
56+
**Fix:** `selectedTasks` now derives from `data?.tasks` instead of `filteredTasks`.
57+
58+
### 4. Tests relied on mock-specific data-testid (RESOLVED)
59+
**Location:** `TaskCard.test.tsx`
60+
**Fix:** Replaced `getByTestId('icon-Loading03Icon')` with `getByRole('status', { name: /loading/i })`.
61+
62+
### 5. SWR revalidation race condition on confirmation count (RESOLVED)
63+
**Location:** `TaskBoardView.tsx:227-235`
64+
**Fix:** Task IDs frozen into `confirmAction` state at dialog-open time.
65+
66+
### 6. BulkActionType included unreachable 'execute' variant (RESOLVED)
67+
**Location:** `BulkActionConfirmDialog.tsx:15`
68+
**Fix:** Narrowed type to `'stop' | 'reset'`, removed dead config entry.
69+
70+
### 7. Missing test for partial failure in batch operations (RESOLVED)
71+
**Location:** `TaskBoardView.test.tsx`
72+
**Fix:** Added test that mocks `stopExecution` rejection and verifies error banner.
73+
74+
---
75+
76+
## Priority 3 Issues - Minor
77+
78+
### 1. Unsafe error cast `err as ApiError`
79+
**Location:** `TaskBoardView.tsx:173, 194`
80+
**Category:** Type Safety
81+
82+
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.
83+
84+
**Decision:** Accepted. Functionally correct. Fixing only in new code would be inconsistent.
85+
86+
### 2. handleClearSelection fires on partial failure
87+
**Location:** `TaskBoardView.tsx:265`
88+
**Category:** UX
89+
90+
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.
91+
92+
**Decision:** Accepted trade-off for v1. Retaining selection on partial failure would require tracking per-task success/failure state.
93+
94+
---
95+
96+
## Positive Findings
97+
98+
1. **Race condition mitigation**: Freezing `taskIds` into `confirmAction` at dialog-open time isolates batch from SWR revalidation mid-flight.
99+
2. **Promise.allSettled**: Correct choice for bulk ops — partial failures handled gracefully.
100+
3. **AlertDialog over Dialog**: Proper Radix primitive for destructive confirmations (focus trap, explicit dismiss).
101+
4. **Accessibility**: `role="alert"` on error banner, `role="status"` on spinner, dismiss button, keyboard nav preserved, ARIA labels on all interactive elements.
102+
5. **e.preventDefault() on AlertDialogAction**: Prevents auto-close before async confirm completes.
103+
6. **Per-task loading state**: `loadingTaskIds` Set prevents double-clicks and gives clear visual feedback.
104+
7. **useCallback/useMemo throughout**: Handlers are stable references, derived state is memoized.
105+
8. **Comprehensive test coverage**: 302 tests covering happy paths, error paths, loading states, accessibility, and now partial failures.
106+
107+
---
108+
109+
## Action Items Summary
110+
111+
### Immediate (Before Merge)
112+
All resolved.
113+
114+
### Short-term (Backlog)
115+
1. Consider retaining selection on partial bulk-action failure
116+
2. Consider a shared `extractErrorDetail(err)` utility for safer error extraction
117+
118+
### Long-term (Backlog)
119+
1. Shift-click range selection for task checkboxes (deferred from issue #340)
120+
121+
---
122+
123+
## Conclusion
124+
125+
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.
126+
127+
**Recommendation:** Deploy

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ module.exports = {
4040
// Task Board components
4141
PlayCircleIcon: createIconMock('PlayCircleIcon'),
4242
LinkCircleIcon: createIconMock('LinkCircleIcon'),
43+
ViewIcon: createIconMock('ViewIcon'),
4344
Search01Icon: createIconMock('Search01Icon'),
4445
CheckListIcon: createIconMock('CheckListIcon'),
4546
// Execution Monitor components

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

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
import { render, screen } from '@testing-library/react';
2+
import userEvent from '@testing-library/user-event';
3+
import { BatchActionsBar } from '@/components/tasks/BatchActionsBar';
4+
import type { Task } from '@/types';
5+
6+
function makeTask(overrides: Partial<Task> = {}): Task {
7+
return {
8+
id: 'task-1',
9+
title: 'Test Task',
10+
description: 'A test task',
11+
status: 'READY',
12+
priority: 1,
13+
depends_on: [],
14+
...overrides,
15+
};
16+
}
17+
18+
const defaultProps = {
19+
selectionMode: true,
20+
onToggleSelectionMode: jest.fn(),
21+
selectedCount: 0,
22+
strategy: 'serial' as const,
23+
onStrategyChange: jest.fn(),
24+
onExecuteBatch: jest.fn(),
25+
onClearSelection: jest.fn(),
26+
isExecuting: false,
27+
selectedTasks: [] as Task[],
28+
onStopBatch: jest.fn(),
29+
onResetBatch: jest.fn(),
30+
isStoppingBatch: false,
31+
isResettingBatch: false,
32+
};
33+
34+
beforeEach(() => {
35+
jest.clearAllMocks();
36+
});
37+
38+
describe('BatchActionsBar', () => {
39+
it('shows Batch button when not in selection mode', () => {
40+
render(<BatchActionsBar {...defaultProps} selectionMode={false} />);
41+
expect(screen.getByRole('button', { name: /batch/i })).toBeInTheDocument();
42+
});
43+
44+
it('shows Cancel button when in selection mode', () => {
45+
render(<BatchActionsBar {...defaultProps} selectionMode={true} />);
46+
expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
47+
});
48+
49+
it('shows Execute button with count when READY tasks are selected', () => {
50+
const selectedTasks = [
51+
makeTask({ id: 't1', status: 'READY' }),
52+
makeTask({ id: 't2', status: 'READY' }),
53+
];
54+
render(
55+
<BatchActionsBar
56+
{...defaultProps}
57+
selectedCount={2}
58+
selectedTasks={selectedTasks}
59+
/>
60+
);
61+
expect(screen.getByRole('button', { name: /execute 2/i })).toBeInTheDocument();
62+
});
63+
64+
it('shows Stop button with count when IN_PROGRESS tasks are selected', () => {
65+
const selectedTasks = [
66+
makeTask({ id: 't1', status: 'IN_PROGRESS' }),
67+
makeTask({ id: 't2', status: 'IN_PROGRESS' }),
68+
makeTask({ id: 't3', status: 'READY' }),
69+
];
70+
render(
71+
<BatchActionsBar
72+
{...defaultProps}
73+
selectedCount={3}
74+
selectedTasks={selectedTasks}
75+
/>
76+
);
77+
expect(screen.getByRole('button', { name: /stop 2/i })).toBeInTheDocument();
78+
});
79+
80+
it('shows Reset button with count when FAILED tasks are selected', () => {
81+
const selectedTasks = [
82+
makeTask({ id: 't1', status: 'FAILED' }),
83+
makeTask({ id: 't2', status: 'FAILED' }),
84+
makeTask({ id: 't3', status: 'FAILED' }),
85+
];
86+
render(
87+
<BatchActionsBar
88+
{...defaultProps}
89+
selectedCount={3}
90+
selectedTasks={selectedTasks}
91+
/>
92+
);
93+
expect(screen.getByRole('button', { name: /reset 3/i })).toBeInTheDocument();
94+
});
95+
96+
it('shows multiple action buttons for mixed selection', () => {
97+
const selectedTasks = [
98+
makeTask({ id: 't1', status: 'READY' }),
99+
makeTask({ id: 't2', status: 'IN_PROGRESS' }),
100+
makeTask({ id: 't3', status: 'FAILED' }),
101+
];
102+
render(
103+
<BatchActionsBar
104+
{...defaultProps}
105+
selectedCount={3}
106+
selectedTasks={selectedTasks}
107+
/>
108+
);
109+
expect(screen.getByRole('button', { name: /execute 1/i })).toBeInTheDocument();
110+
expect(screen.getByRole('button', { name: /stop 1/i })).toBeInTheDocument();
111+
expect(screen.getByRole('button', { name: /reset 1/i })).toBeInTheDocument();
112+
});
113+
114+
it('calls onStopBatch when Stop button is clicked', async () => {
115+
const user = userEvent.setup();
116+
const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
117+
render(
118+
<BatchActionsBar
119+
{...defaultProps}
120+
selectedCount={1}
121+
selectedTasks={selectedTasks}
122+
/>
123+
);
124+
await user.click(screen.getByRole('button', { name: /stop 1/i }));
125+
expect(defaultProps.onStopBatch).toHaveBeenCalledTimes(1);
126+
});
127+
128+
it('calls onResetBatch when Reset button is clicked', async () => {
129+
const user = userEvent.setup();
130+
const selectedTasks = [makeTask({ id: 't1', status: 'FAILED' })];
131+
render(
132+
<BatchActionsBar
133+
{...defaultProps}
134+
selectedCount={1}
135+
selectedTasks={selectedTasks}
136+
/>
137+
);
138+
await user.click(screen.getByRole('button', { name: /reset 1/i }));
139+
expect(defaultProps.onResetBatch).toHaveBeenCalledTimes(1);
140+
});
141+
142+
it('disables Stop button when isStoppingBatch is true', () => {
143+
const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
144+
render(
145+
<BatchActionsBar
146+
{...defaultProps}
147+
selectedCount={1}
148+
selectedTasks={selectedTasks}
149+
isStoppingBatch={true}
150+
/>
151+
);
152+
expect(screen.getByRole('button', { name: /stop/i })).toBeDisabled();
153+
});
154+
155+
it('disables Reset button when isResettingBatch is true', () => {
156+
const selectedTasks = [makeTask({ id: 't1', status: 'FAILED' })];
157+
render(
158+
<BatchActionsBar
159+
{...defaultProps}
160+
selectedCount={1}
161+
selectedTasks={selectedTasks}
162+
isResettingBatch={true}
163+
/>
164+
);
165+
expect(screen.getByRole('button', { name: /reset/i })).toBeDisabled();
166+
});
167+
168+
it('shows strategy selector only when READY tasks are selected', () => {
169+
const selectedTasks = [makeTask({ id: 't1', status: 'IN_PROGRESS' })];
170+
render(
171+
<BatchActionsBar
172+
{...defaultProps}
173+
selectedCount={1}
174+
selectedTasks={selectedTasks}
175+
/>
176+
);
177+
// Strategy selector should not appear when no READY tasks
178+
expect(screen.queryByText('Serial')).not.toBeInTheDocument();
179+
});
180+
181+
it('hides action buttons when no tasks are selected', () => {
182+
render(<BatchActionsBar {...defaultProps} selectedCount={0} selectedTasks={[]} />);
183+
expect(screen.queryByRole('button', { name: /execute/i })).not.toBeInTheDocument();
184+
expect(screen.queryByRole('button', { name: /stop/i })).not.toBeInTheDocument();
185+
expect(screen.queryByRole('button', { name: /reset/i })).not.toBeInTheDocument();
186+
});
187+
});

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { render, screen } from '@testing-library/react';
2+
import userEvent from '@testing-library/user-event';
3+
import { BulkActionConfirmDialog } from '@/components/tasks/BulkActionConfirmDialog';
4+
5+
const defaultProps = {
6+
open: true,
7+
onOpenChange: jest.fn(),
8+
actionType: 'stop' as const,
9+
taskCount: 3,
10+
onConfirm: jest.fn(),
11+
isLoading: false,
12+
};
13+
14+
beforeEach(() => {
15+
jest.clearAllMocks();
16+
});
17+
18+
describe('BulkActionConfirmDialog', () => {
19+
it('renders stop confirmation with correct title and description', () => {
20+
render(<BulkActionConfirmDialog {...defaultProps} actionType="stop" taskCount={2} />);
21+
expect(screen.getByText('Stop Tasks')).toBeInTheDocument();
22+
expect(screen.getByText(/stop 2 running task\(s\)/i)).toBeInTheDocument();
23+
});
24+
25+
it('renders reset confirmation with correct title and description', () => {
26+
render(<BulkActionConfirmDialog {...defaultProps} actionType="reset" taskCount={4} />);
27+
expect(screen.getByText('Reset Tasks')).toBeInTheDocument();
28+
expect(screen.getByText(/reset 4 failed task\(s\)/i)).toBeInTheDocument();
29+
});
30+
31+
it('calls onConfirm when confirm button is clicked', async () => {
32+
const user = userEvent.setup();
33+
render(<BulkActionConfirmDialog {...defaultProps} />);
34+
await user.click(screen.getByRole('button', { name: /confirm/i }));
35+
expect(defaultProps.onConfirm).toHaveBeenCalledTimes(1);
36+
});
37+
38+
it('calls onOpenChange(false) when cancel button is clicked', async () => {
39+
const user = userEvent.setup();
40+
render(<BulkActionConfirmDialog {...defaultProps} />);
41+
await user.click(screen.getByRole('button', { name: /cancel/i }));
42+
expect(defaultProps.onOpenChange).toHaveBeenCalledWith(false);
43+
});
44+
45+
it('disables confirm button when isLoading is true', () => {
46+
render(<BulkActionConfirmDialog {...defaultProps} isLoading={true} />);
47+
const confirmBtn = screen.getByRole('button', { name: /confirm/i });
48+
expect(confirmBtn).toBeDisabled();
49+
});
50+
51+
it('does not render when open is false', () => {
52+
render(<BulkActionConfirmDialog {...defaultProps} open={false} />);
53+
expect(screen.queryByText('Stop Tasks')).not.toBeInTheDocument();
54+
});
55+
56+
it('shows destructive styling for stop action confirm button', () => {
57+
render(<BulkActionConfirmDialog {...defaultProps} actionType="stop" />);
58+
const confirmBtn = screen.getByRole('button', { name: /confirm/i });
59+
expect(confirmBtn).toHaveClass('bg-destructive');
60+
});
61+
});

0 commit comments

Comments
 (0)