[Phase 3] Task Board: bulk stop, reset, and state management actions#389
[Phase 3] Task Board: bulk stop, reset, and state management actions#389
Conversation
… FAILED tasks - Add onStop and onReset optional props to TaskCardProps - Show Stop button with Cancel01Icon (destructive ghost styling) for IN_PROGRESS tasks - Show Reset button with ArrowTurnBackwardIcon for FAILED tasks - Both buttons use e.stopPropagation() to avoid triggering card onClick - Add 7 new tests covering button visibility, callbacks, and styling
…ierarchy - Add onStop/onReset props to TaskColumn and TaskBoardContent - Implement handleStop (calls tasksApi.stopExecution) in TaskBoardView - Implement handleReset (calls tasksApi.updateStatus READY) in TaskBoardView - Pass handlers through TaskBoardContent -> TaskColumn -> TaskCard - Add 5 integration tests for stop/reset actions and error handling
…sk actions - AlertDialog-based component for execute/stop/reset bulk actions - Destructive styling for stop action confirm button - Loading state with spinner on confirm button - 8 tests covering all action types, callbacks, and states
…statuses - Add selectedTasks, onStopBatch, onResetBatch, isStoppingBatch, isResettingBatch props - Show Execute/Stop/Reset buttons with counts based on task status - Strategy selector only visible when READY tasks are selected - Destructive variant for Stop, outline for Reset - Loading spinners for each action type - 12 new tests covering all states and interactions
- Add isStoppingBatch, isResettingBatch, confirmAction state - Add selectedTasks useMemo for status-aware batch actions - Implement handleStopBatch/handleResetBatch with confirmation flow - handleConfirmAction uses Promise.allSettled for parallel API calls - Wire BulkActionConfirmDialog and enhanced BatchActionsBar props - Add 5 integration tests for full bulk action flow
- Add select-all Checkbox to column header in selection mode - Support checked, unchecked, and indeterminate states - Add onSelectAll/onDeselectAll props through TaskColumn -> TaskBoardContent -> TaskBoardView - Implement handleSelectAll/handleDeselectAll in TaskBoardView - 8 new tests for select-all behavior and edge cases
- Add isLoading prop to TaskCard, show Loading03Icon spinner instead of action buttons - Add loadingTaskIds Set state to TaskBoardView for per-task tracking - Wire loadingTaskIds through TaskBoardContent -> TaskColumn -> TaskCard - handleStop/handleReset add task to loadingTaskIds during API call - 4 new tests for loading spinner behavior across task statuses
- Add role="status" and aria-label to loading spinner in TaskCard - Freeze task IDs at dialog-open time to prevent count desync from SWR revalidation - Add role="alert" and dismiss button to error banner in TaskBoardView - Use frozen taskIds in handleConfirmAction instead of re-filtering selectedTasks
|
Caution Review failedThe pull request is closed. WalkthroughAdds per-task stop/reset actions, bulk selection and status-aware batch actions with confirmation dialog, per-task and batch loading states, aggregated error handling, routing to execution for in-progress tasks, and comprehensive tests across task components and a new BulkActionConfirmDialog. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as TaskBoardView
participant Dialog as BulkActionConfirmDialog
participant API as Tasks API
participant State as Component State
User->>UI: Select multiple tasks (checkboxes)
UI->>State: update selectedTasks
State-->>UI: render BatchActionsBar (actions shown based on statuses)
User->>UI: Click "Stop" in BatchActionsBar
UI->>State: set confirmAction='stop' and open dialog
State-->>Dialog: open(actionType='stop', taskCount)
User->>Dialog: Click Confirm
Dialog->>UI: onConfirm
UI->>State: set isStoppingBatch=true
loop parallel per task
UI->>API: POST /tasks/{id}/stop
API-->>UI: response (success/failure)
end
UI->>State: aggregate results, close dialog, clear selection, set isStoppingBatch=false
State-->>UI: updated UI (show error banner if some failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Add bulk stop/reset with confirmation, per-task stop/reset, and column select-all to the Task Board in TaskBoardView.tsxIntroduce bulk stop/reset flows with a confirmation dialog, add per-task stop/reset actions with loading states, derive selection from the full dataset, and add column-level select-all controls. Wire UI via 📍Where to StartStart with the bulk action orchestration and state in TaskBoardView.tsx, then review the confirmation UI in BulkActionConfirmDialog.tsx and the action surface in BatchActionsBar.tsx. Macroscope summarized a2fe539. |
PR #389 Review — Task Board: Bulk Stop, Reset & State ManagementThis is a well-structured addition. The component decomposition is clean, the race-condition fix (freezing task IDs at dialog-open time) is explicitly noted and correctly implemented, and test coverage is thorough. A few issues worth addressing before merge: Bug: Loading state cleared before SWR refresh in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/src/components/tasks/TaskBoardView.tsx (1)
56-85:⚠️ Potential issue | 🟠 MajorBatch operations have inconsistent scope when filters are applied.
selectedCountshowsselectedTaskIds.size(all selected items), buthandleStopBatchandhandleResetBatchfilter selections to only visible tasks viaselectedTasks. Meanwhile,handleExecuteBatchuses the fullselectedTaskIds. No selection clearing occurs when filters change, so users can end up with "5 selected" displayed but only 2 visible tasks affected by stop/reset—while execute would process all 5 hidden selections.🛠️ Suggested fix (make all operations consistent)
- const selectedTasks = useMemo( - () => filteredTasks.filter((t) => selectedTaskIds.has(t.id)), - [filteredTasks, selectedTaskIds] - ); + const selectedTasks = useMemo( + () => (data?.tasks ?? []).filter((t) => selectedTaskIds.has(t.id)), + [data?.tasks, selectedTaskIds] + );This makes stop/reset operations consistent with execute, which already operates on the full
selectedTaskIdsset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/tasks/TaskBoardView.tsx` around lines 56 - 85, The batch-action consistency bug stems from mixed use of selectedTasks (visible, from filteredTasks) and selectedTaskIds (global) across handlers: handleStopBatch and handleResetBatch currently act on selectedTasks while handleExecuteBatch uses selectedTaskIds, and no selections are cleared when filters change. Update handleStopBatch and handleResetBatch to operate on the full selectedTaskIds set (same as handleExecuteBatch) and/or centralize a utility that returns Array.from(selectedTaskIds) for batch operations; additionally, add logic in the filter/search effect to clear or reconcile selectedTaskIds when filters change so the displayed selectedCount and actionable items remain in sync (use selectedTaskIds, selectedTasks, handleStopBatch, handleResetBatch, handleExecuteBatch, and the filter effect to locate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/__tests__/components/tasks/TaskCard.test.tsx`:
- Around line 207-229: The tests that assert the loading spinner (in the specs
"shows loading spinner instead of action button when isLoading", "shows loading
spinner for READY task when isLoading", and "shows loading spinner for FAILED
task when isLoading") rely on getByTestId('icon-Loading03Icon') from the jest
SVG mock; replace those assertions to query the accessible spinner instead (use
getByRole or queryByRole with role 'status' and name/label matching /loading/i)
so renderCard(...) is validated against the real component behavior rather than
mock-only data-testid attributes, and remove any reliance on the
__mocks__/@hugeicons/react.js testid in these tests.
---
Outside diff comments:
In `@web-ui/src/components/tasks/TaskBoardView.tsx`:
- Around line 56-85: The batch-action consistency bug stems from mixed use of
selectedTasks (visible, from filteredTasks) and selectedTaskIds (global) across
handlers: handleStopBatch and handleResetBatch currently act on selectedTasks
while handleExecuteBatch uses selectedTaskIds, and no selections are cleared
when filters change. Update handleStopBatch and handleResetBatch to operate on
the full selectedTaskIds set (same as handleExecuteBatch) and/or centralize a
utility that returns Array.from(selectedTaskIds) for batch operations;
additionally, add logic in the filter/search effect to clear or reconcile
selectedTaskIds when filters change so the displayed selectedCount and
actionable items remain in sync (use selectedTaskIds, selectedTasks,
handleStopBatch, handleResetBatch, handleExecuteBatch, and the filter effect to
locate code).
- Wrap handleConfirmAction in try/finally so loading states and dialog
cleanup always execute, even if mutate() throws
- Move loading flag reset into finally block to keep spinners visible
until SWR refresh completes (prevents stale data flash)
- Remove unreachable 'execute' branch from handleConfirmAction
- Derive selectedTasks from full task list instead of filteredTasks
so bulk actions include all selected tasks regardless of active filters
- Replace getByTestId('icon-Loading03Icon') with getByRole('status')
to test actual accessibility behavior, not jest mock internals
- Remove unused checkboxes variable in TaskBoardView test
- Add JSDoc on optional onStop/onReset props in TaskCard
PR Review: Task Board Bulk Stop/Reset ActionsThis is a well-scoped implementation. The feature works within the existing API surface, the race-condition mitigation (freezing taskIds at dialog-open time in Potential Bugs1.
The ambiguity will confuse the next person wiring up a confirm-before-execute flow. 2. Error cast is unsafe In const apiErr = err as ApiError;
setActionError(apiErr.detail || 'Failed to stop task');
const detail = err \!= null && typeof err === 'object' && 'detail' in err
? String((err as ApiError).detail)
: undefined;
setActionError(detail || 'Failed to stop task');Minor Code Quality3. Before: {(task.status === 'READY' || task.status === 'BACKLOG' || task.status === 'IN_PROGRESS' || task.status === 'FAILED') && (This now covers every status that has an action. Since const hasAction =
(task.status === 'READY') ||
(task.status === 'BACKLOG') ||
(task.status === 'IN_PROGRESS' && \!\!onStop) ||
(task.status === 'FAILED' && \!\!onReset);This also documents that the button only appears when the handler is wired up. 4. In the 5. Three separate const { readyCount, inProgressCount, failedCount } = useMemo(() => {
return selectedTasks.reduce(
(acc, t) => {
if (t.status === 'READY') acc.readyCount++;
else if (t.status === 'IN_PROGRESS') acc.inProgressCount++;
else if (t.status === 'FAILED') acc.failedCount++;
return acc;
},
{ readyCount: 0, inProgressCount: 0, failedCount: 0 }
);
}, [selectedTasks]);This is a style note, not a blocker. Test Coverage Gaps6. Batch partial-failure error is not tested The 7. The What's Done Well
Overall: Approve after addressing bug #1 (type ambiguity) and bug #2 (unsafe error cast). The rest are improvements worth picking up but not blockers. |
- Remove 'execute' from BulkActionType (unreachable after earlier fix) - Remove dead execute config from ACTION_CONFIG - Add test for batch stop partial failure error message - Update BulkActionConfirmDialog tests for narrowed type - Add code review report documenting all resolved issues
Review: Task Board Bulk Stop, Reset & State ManagementOverall: The implementation is clean and well-structured. All issues flagged in the previous CodeRabbit review have been resolved — the Observations1. Single-task vs batch confirmation asymmetry
2. try {
if (confirmAction.type === 'stop') {
setIsStoppingBatch(true); // ← set after entering try
const results = await Promise.allSettled(...)There's a render cycle between 3.
4. Batch partial-failure message lacks task identity
5.
What's working well
Recommendation: Ready to merge after removing (or justifying) the |
Post-Merge Review — PR #389: Task Board Bulk State ManagementThe PR is already merged and the committed review report ( Implementation Quality: StrongThe final state of the code is clean:
Minor Observations (backlog only, not blocking)1. // TaskBoardView.tsx
isLoading={isStoppingBatch || isResettingBatch || isExecuting}The dialog is only reachable via the stop/reset path, so 2. Both loading flags reset in finally {
setIsStoppingBatch(false); // reset unconditionally
setIsResettingBatch(false); // reset unconditionally
...
}This is correct and safe — resetting a flag that was already Both issues are documented trade-offs, not defects. The implementation is solid and follows existing codebase patterns throughout. |
1 participant
Summary
Implements #340: Adds comprehensive task state management to the Task Board, including single-task and bulk stop/reset actions with confirmation dialogs.
Acceptance Criteria
Test Plan
Implementation Notes
tasksApi.stopExecution()andtasksApi.updateStatus()— no API changes requiredPromise.allSettled()for graceful partial failureCloses #340
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Documentation