feat: Task Board View with Kanban & Batch Execution (#331)#338
Conversation
Implements the Phase 3 Task Board page with full Kanban board, filtering, batch execution controls, and task detail modal. Components: - TaskBoardView: Main orchestrator with SWR data fetching, filtering, and batch state - TaskBoardContent: 6-column Kanban layout (Backlog→Done) with responsive grid - TaskColumn: Status column with count badge and empty state - TaskCard: Interactive card with status badge, dependency indicator, a11y keyboard support - TaskDetailModal: Dialog with task metadata, Execute/Mark Ready actions - TaskFilters: Search with 300ms debounce + status pill toggles - BatchActionsBar: Selection mode toggle, strategy picker, batch execute Also adds: - /tasks page route with workspace hydration guard - Tasks nav link enabled in AppSidebar - tasksApi extensions (getAll, getOne, updateStatus, startExecution, executeBatch) - Batch execution types (BatchRequest, BatchStrategy, TaskListResponse) - 3 new Shadcn UI components (Checkbox, Input, Select) - 46 tests across 4 test suites (TaskCard, TaskBoardView, TaskDetailModal, TaskFilters)
WalkthroughAdds a Task Board feature with Kanban UI (board, columns, cards, filters, batch actions, detail modal), new task API client methods and types, UI primitives (Checkbox/Input/Select), tests, icon mock exports, and enables the Tasks sidebar/page with workspace hydration handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as TasksPage
participant Board as TaskBoardView
participant API as tasksApi
participant Backend as /api/v2/tasks
User->>Page: Navigate to /tasks
Page->>Board: render with workspacePath
Board->>API: SWR GET /api/v2/tasks (workspacePath)
API->>Backend: GET /api/v2/tasks?workspace_path=...
Backend-->>API: Task[]
API-->>Board: data
Board->>Board: group by status, render TaskBoardContent
User->>Board: Filter / search / toggle selection / open modal / execute
Board->>API: POST /api/v2/tasks/{id}/execute or POST /api/v2/batches/run or PATCH /api/v2/tasks/{id}
API->>Backend: execute/update request
Backend-->>API: execution/update response
API-->>Board: result
Board->>Board: mutate/revalidate SWR, update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 Kanban Task Board with batch execution on /tasks and include estimated_hours and ISO8601 timestamps in
|
Code Review: Task Board View ImplementationThis is a well-structured Phase 3 feature implementation. Here's my detailed review: ✅ Strengths
🔍 Critical Issues1. API Contract Mismatch - updateStatus (CRITICAL) Location: The Fix: Update return type or handle missing fields in components. 2. Missing Backend Endpoint (CRITICAL) Location: Cannot find
Fix: Verify endpoint exists or update frontend.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web-ui/src/components/tasks/TaskBoardView.tsx`:
- Around line 127-145: handleExecuteBatch currently starts the batch via
tasksApi.executeBatch and resets selection but never navigates to the Execution
Monitor; change it to capture the executeBatch response (const resp = await
tasksApi.executeBatch(...)), extract resp.batch_id (or resp.batchId), call
router.push(`/executions/${batchId}`) after successful start (you can navigate
immediately or after await mutate()), and only then clear selection and
setSelectionMode(false); update references in handleExecuteBatch to use the
returned batch_id and ensure a router (useRouter or history) is available in the
component before calling router.push.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 94-107: The modal-level error can persist after a failed
handleMarkReady call; clear the error at the start of handleMarkReady by calling
setError('') (or null) before initiating the API call so any previous error does
not remain after a subsequent success; update the handleMarkReady function
(which calls tasksApi.updateStatus, setIsUpdating, setTask, and onStatusChange)
to reset error state at the beginning and preserve the existing
try/catch/finally behavior.
🧹 Nitpick comments (9)
web-ui/__tests__/components/tasks/TaskBoardView.test.tsx (1)
156-166: Fragile filter-pill selector relies on Badge internal markup.
btn.querySelector('div')distinguishes filter pills from other buttons by assuming Badge renders a<div>. If Badge's markup changes,readyFilterwill beundefinedand the non-null assertion on line 161 will throw a misleading error instead of a clear test failure.Consider using a
data-testidon the filter pill buttons inTaskFilters.tsx, or at minimum add a descriptive assertion before the!:Suggested improvement
const readyFilter = filterButtons.find( (btn) => btn.textContent === 'Ready' && btn.querySelector('div') ); - await user.click(readyFilter!); + expect(readyFilter).toBeDefined(); + await user.click(readyFilter!);A more robust approach would be adding
data-testid="status-filter-READY"to the filter pill buttons inTaskFilters.tsxand querying by that.web-ui/src/components/tasks/TaskBoardContent.tsx (1)
37-48: Tasks with statuses outside COLUMN_ORDER are silently dropped.The
if (grouped[task.status])guard on line 43 means any task whose status isn't in the 6-itemCOLUMN_ORDER(e.g.,MERGEDor a future status) will be silently excluded from the board. This is likely intentional for the Kanban view, but worth a brief inline comment so future maintainers don't wonder why tasks go missing.Optional: Add clarifying comment
for (const task of tasks) { + // Only include tasks whose status matches a visible Kanban column; + // other statuses (e.g. MERGED) are intentionally excluded. if (grouped[task.status]) { grouped[task.status].push(task); } }web-ui/src/components/tasks/BatchActionsBar.tsx (1)
83-90: Minor: native<button>for "Clear" vs<Button>used elsewhere.The "Clear" control uses a raw
<button>while the rest of the bar uses the<Button>component. This is likely intentional for the link-style appearance, but worth noting it lacks thetype="button"attribute, which means it defaults totype="submit"inside a<form>.Add explicit type to prevent accidental form submission if ever wrapped in a form
{selectedCount > 0 && ( <button + type="button" onClick={onClearSelection} className="text-xs text-muted-foreground hover:text-foreground" > Clear </button> )}web-ui/src/components/tasks/TaskCard.tsx (2)
10-30: Duplicated status mappings — extract to a shared module.
STATUS_BADGE_VARIANTandSTATUS_LABELare identically defined in bothTaskCard.tsxandTaskDetailModal.tsx(lines 24–42). Consider extracting them to a shared file (e.g.,task-utils.tsor a constants file) to avoid maintaining two copies.
77-79:as nevercast on Badge variant is fragile.This bypasses type-checking entirely. If the Badge
variantprop is a union, you could typeSTATUS_BADGE_VARIANTwith the actual variant union (e.g., import theBadgeProps["variant"]type from the badge component) to get compile-time safety.web-ui/src/types/index.ts (1)
86-100: Consider usingBatchStrategy/TaskStatusinstead ofstringfor response fields.
StartExecutionResponse.strategy(Line 90) andTaskStartResponse.status(Line 98) are typed asstring, while the corresponding request types use the stricter union types. If the backend is guaranteed to return one of the known values, narrowing these would provide better type safety downstream.Proposed tighter typing
export interface StartExecutionResponse { success: boolean; batch_id: string; task_count: number; - strategy: string; + strategy: BatchStrategy; message: string; } export interface TaskStartResponse { success: boolean; run_id: string; task_id: string; - status: string; + status: TaskStatus; message: string; }web-ui/src/components/tasks/TaskFilters.tsx (2)
36-46: Sync effect can trigger an unnecessary debounce cycle.When
searchQuerychanges from the parent (Line 44–46),localQueryupdates, which fires the debounce effect (Line 36–41), callingonSearchChangeagain with the same value after 300ms. This is a no-op but still schedules an unnecessary timer.Not a bug — the value is idempotent — but if it becomes a concern, you could guard the debounce to skip when localQuery matches the incoming prop.
66-69: Addtype="button"to pill buttons.Same as the Clear button in
BatchActionsBar— these<button>elements default totype="submit"and could cause unintended form submissions if the component is ever placed inside a<form>.Proposed fix
<button key={value} + type="button" onClick={() => onStatusFilter(isActive ? null : value)}Also apply to the Clear button on Line 85:
<button + type="button" onClick={() => onStatusFilter(null)}web-ui/src/components/tasks/TaskBoardView.tsx (1)
46-64: Client-side filtering works but doesn't scale.All tasks are fetched in a single call and filtered in
useMemo. This is fine for moderate task counts, but if workspaces grow to thousands of tasks, consider passing the status filter totasksApi.getAll(which already accepts an optionalstatusparameter) to offload filtering to the server.Not a problem today — just an architectural note for future scale.
| const handleExecuteBatch = useCallback(async () => { | ||
| if (selectedTaskIds.size === 0) return; | ||
| setIsExecuting(true); | ||
| setActionError(null); | ||
| try { | ||
| await tasksApi.executeBatch(workspacePath, { | ||
| task_ids: Array.from(selectedTaskIds), | ||
| strategy: batchStrategy, | ||
| }); | ||
| setSelectionMode(false); | ||
| setSelectedTaskIds(new Set()); | ||
| await mutate(); | ||
| } catch (err) { | ||
| const apiErr = err as ApiError; | ||
| setActionError(apiErr.detail || 'Failed to start batch execution'); | ||
| } finally { | ||
| setIsExecuting(false); | ||
| } | ||
| }, [workspacePath, selectedTaskIds, batchStrategy, mutate]); |
There was a problem hiding this comment.
Missing navigation to Execution Monitor after batch execution.
The PR objectives state: "Execute Batch action that starts a batch run and navigates to the Execution Monitor." Currently, handleExecuteBatch resets selection and revalidates but doesn't navigate. The executeBatch response includes a batch_id that could be used for navigation (e.g., router.push(\/executions/${batchId}`)`).
Is this intentionally deferred, or should it be added now?
🤖 Prompt for AI Agents
In `@web-ui/src/components/tasks/TaskBoardView.tsx` around lines 127 - 145,
handleExecuteBatch currently starts the batch via tasksApi.executeBatch and
resets selection but never navigates to the Execution Monitor; change it to
capture the executeBatch response (const resp = await
tasksApi.executeBatch(...)), extract resp.batch_id (or resp.batchId), call
router.push(`/executions/${batchId}`) after successful start (you can navigate
immediately or after await mutate()), and only then clear selection and
setSelectionMode(false); update references in handleExecuteBatch to use the
returned batch_id and ensure a router (useRouter or history) is available in the
component before calling router.push.
| const handleMarkReady = async () => { | ||
| if (!task) return; | ||
| setIsUpdating(true); | ||
| try { | ||
| const updated = await tasksApi.updateStatus(workspacePath, task.id, 'READY'); | ||
| setTask(updated); | ||
| onStatusChange(); | ||
| } catch (err) { | ||
| const apiErr = err as ApiError; | ||
| setError(apiErr.detail || 'Failed to update status'); | ||
| } finally { | ||
| setIsUpdating(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Error from handleMarkReady may persist as stale state across interactions.
If handleMarkReady fails (Line 103), the error is set but only cleared on the next open/close cycle (Line 69). A successful subsequent action (like executing) won't clear this modal-level error. Consider clearing error at the start of handleMarkReady:
Proposed fix
const handleMarkReady = async () => {
if (!task) return;
setIsUpdating(true);
+ setError(null);
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleMarkReady = async () => { | |
| if (!task) return; | |
| setIsUpdating(true); | |
| try { | |
| const updated = await tasksApi.updateStatus(workspacePath, task.id, 'READY'); | |
| setTask(updated); | |
| onStatusChange(); | |
| } catch (err) { | |
| const apiErr = err as ApiError; | |
| setError(apiErr.detail || 'Failed to update status'); | |
| } finally { | |
| setIsUpdating(false); | |
| } | |
| }; | |
| const handleMarkReady = async () => { | |
| if (!task) return; | |
| setIsUpdating(true); | |
| setError(null); | |
| try { | |
| const updated = await tasksApi.updateStatus(workspacePath, task.id, 'READY'); | |
| setTask(updated); | |
| onStatusChange(); | |
| } catch (err) { | |
| const apiErr = err as ApiError; | |
| setError(apiErr.detail || 'Failed to update status'); | |
| } finally { | |
| setIsUpdating(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 94 - 107, The
modal-level error can persist after a failed handleMarkReady call; clear the
error at the start of handleMarkReady by calling setError('') (or null) before
initiating the API call so any previous error does not remain after a subsequent
success; update the handleMarkReady function (which calls tasksApi.updateStatus,
setIsUpdating, setTask, and onStatusChange) to reset error state at the
beginning and preserve the existing try/catch/finally behavior.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| import type { Task, TaskStatus } from '@/types'; | ||
|
|
||
| /** Column display order matches the task lifecycle. */ | ||
| const COLUMN_ORDER: TaskStatus[] = [ |
There was a problem hiding this comment.
🟡 Medium
tasks/TaskBoardContent.tsx:8 MERGED is a valid TaskStatus but is missing from COLUMN_ORDER, so merged tasks are silently dropped. Consider adding 'MERGED' to the array.
🚀 Want me to fix this? Reply ex: "fix it for me".
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| onClick(task.id); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🟡 Medium
tasks/TaskCard.tsx:55 The onKeyDown handler intercepts Enter/Space from nested buttons and checkbox, breaking their keyboard accessibility. Consider checking e.target === e.currentTarget before handling.
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onClick(task.id); | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.target === e.currentTarget && (e.key === 'Enter' || e.key === ' ')) { | |
| e.preventDefault(); | |
| onClick(task.id); | |
| } | |
| }} |
🚀 Want me to fix this? Reply ex: "fix it for me".
Backend TaskResponse was missing estimated_hours, created_at, and updated_at fields that the core Task model has and the frontend expects. Added these fields to TaskResponse and all 3 serialization points. Also made created_at/updated_at optional in the frontend Task type for resilience.
PR Review: Phase 3 Task Board ImplementationThis is a comprehensive and well-structured implementation of the Task Board feature. The code quality is excellent overall, with good separation of concerns, accessibility features, and extensive test coverage. Here are my findings: ✅ Strengths
🔍 Issues Found1. Memory Leak Risk in TaskFilters (Medium Priority)File: The timeout cleanup in the useEffect could potentially be called with an undefined timer: return () => clearTimeout(timerRef.current); // timerRef.current might be undefinedFix: Ensure timer is defined before clearing: return () => {
if (timerRef.current) {
clearTimeout(timerRef.current);
}
};2. Type Assertion Could Hide Type Errors (Low Priority)File: variant={STATUS_BADGE_VARIANT[task.status] as never}Using Recommendation: Define proper Badge variant types or add runtime validation. 3. API Error Handling Could Be More Specific (Low Priority)File: Error handling casts all errors to ApiError without checking: const apiErr = err as ApiError;Recommendation: Add type guard or instanceof check before casting. 4. Potential Race Condition in TaskDetailModal (Low Priority)File: The Recommendation: Consider using AbortController for proper request cancellation. 🛡️ Security Considerations
📊 Performance Considerations
🧪 Test Coverage AssessmentExcellent coverage with proper testing of:
Missing coverage:
📝 Code Style & Best Practices
🚀 Recommendations
📋 SummaryThis PR is ready to merge after addressing the TaskFilters cleanup issue. The implementation is solid, well-tested, and follows the project's architectural patterns. The team has done excellent work on accessibility and code organization. Approval Status: Approved pending minor fix ✅ |
Radix Dialog requires DialogTitle to always be present for screen reader accessibility. Added a hidden DialogTitle for loading and error states where the task data isn't available yet.
Follow-Up Review: All Critical Issues Addressed ✅Thank you for the prompt fixes! I've reviewed the three commits since my initial review, and I'm pleased to confirm that all the issues I raised have been properly addressed. ✅ Issues Resolved1. API Contract Mismatch - FIXED (commit 54bdd84)Original Issue: TaskResponse lacked Resolution: ✅ Properly added to TaskResponse model in estimated_hours: Optional[float] = None
created_at: Optional[str] = None
updated_at: Optional[str] = NoneAll three API endpoints ( 2. Missing Backend Endpoint - CONFIRMED EXISTSOriginal Issue: I couldn't find Resolution: ✅ Endpoint exists at @router.get("", response_model=TaskListResponse)This was my error - the endpoint was always there. Apologies for the confusion! 3. Accessibility Enhancement - EXCELLENT FIX (commit 0985b35)New Issue Found: Radix Dialog requires DialogTitle for screen reader accessibility in all states Resolution: ✅ Added sr-only DialogTitle/DialogDescription for loading/error states in 🔍 Outstanding Items from Previous ReviewMinor Issues (non-blocking)
🎯 Final AssessmentStatus: APPROVED ✅ All critical issues have been resolved. The remaining items are minor code quality suggestions that can be addressed in future iterations. The implementation is:
Recommendation: Merge immediately. This is production-ready code. Excellent work on the quick turnaround! The attention to accessibility (DialogTitle fix) shows strong attention to detail. 🚀 |
Backend api_error() returns {error, code, detail} objects as HTTPException
detail. The axios interceptor's normalizeErrorDetail only handled strings
and validation arrays, passing objects through unmodified. This caused
"Objects are not valid as React child" when the error object was rendered.
Now extracts the human-readable .error or .detail string from structured
error objects.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 66-71: In TaskDetailModal's useEffect, the early-return branch
currently clears task and error but not isLoading, which can leave a stale
spinner; update that branch to also call setIsLoading(false) so isLoading is
reset whenever the modal closes or taskId is falsy, and ensure the fetch-start
path still sets setIsLoading(true) and the fetch finally path clears it as
before (referencing useEffect, setTask, setError, setIsLoading, and the
cancelled flag).
🧹 Nitpick comments (1)
web-ui/src/components/tasks/TaskDetailModal.tsx (1)
136-136:as nevercast bypasses Badge variant type-safety.If
Badgeexposes its variant union (e.g., viaVariantProps), you can type the map values directly and drop the cast. This keeps the compiler checking that every mapped value is actually a valid variant.Example
-const STATUS_BADGE_VARIANT: Record<TaskStatus, string> = { +import type { VariantProps } from 'class-variance-authority'; +// Assuming badgeVariants is exported from Badge +type BadgeVariant = NonNullable<VariantProps<typeof badgeVariants>['variant']>; + +const STATUS_BADGE_VARIANT: Record<TaskStatus, BadgeVariant> = { BACKLOG: 'backlog', ... };Then at the call site:
-<Badge variant={STATUS_BADGE_VARIANT[task.status] as never}> +<Badge variant={STATUS_BADGE_VARIANT[task.status]}>
| useEffect(() => { | ||
| if (!open || !taskId) { | ||
| setTask(null); | ||
| setError(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stale isLoading can cause a permanent spinner.
The early-return branch resets task and error but not isLoading. If the dialog closes while a fetch is in-flight, the cancelled flag prevents the .finally() from clearing isLoading. On the next open with taskId: null (or a rapid close/reopen cycle), isLoading remains true with no pending fetch, rendering an infinite spinner.
Proposed fix
useEffect(() => {
if (!open || !taskId) {
setTask(null);
setError(null);
+ setIsLoading(false);
return;
}🤖 Prompt for AI Agents
In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 66 - 71, In
TaskDetailModal's useEffect, the early-return branch currently clears task and
error but not isLoading, which can leave a stale spinner; update that branch to
also call setIsLoading(false) so isLoading is reset whenever the modal closes or
taskId is falsy, and ensure the fetch-start path still sets setIsLoading(true)
and the fetch finally path clears it as before (referencing useEffect, setTask,
setError, setIsLoading, and the cancelled flag).
Code Review: Task Board View ImplementationThis is an excellent Phase 3 implementation that delivers a fully-featured Kanban board UI. The code quality is high, architecture is sound, and test coverage is comprehensive. Here's my detailed feedback: ✅ Strengths1. Architecture & Design Patterns
2. Accessibility (WCAG 2.1 Compliance) ✨
3. API Integration
4. Test Coverage (46 new tests)
5. User Experience
🔍 Issues & Recommendations🔴 Critical Issues1. Race Condition in TaskDetailModal The cleanup function in the useEffect sets Location: web-ui/src/components/tasks/TaskDetailModal.tsx:89-91 Current: return () => {
cancelled = true;
};Fix: return () => {
cancelled = true;
setIsLoading(false);
};🟡 Medium Priority2. Selection State Lost on Batch Error In Location: TaskBoardView.tsx:136-137 Issue: Lines execute regardless of try/catch outcome. Fix: Move 3. Debounce Ineffective Due to Unstable Dependency
Fix Options:
🟢 Minor Improvements4. Type Assertion Could Be Safer Using 5. Missing ARIA for Selection Mode Screen reader users aren't notified when selection mode activates. Add aria-live region. 6. Backend: Missing Transaction Safety Between 🎯 Security Considerations7. Rate Limiting on Batch Execution The 8. Input Validation on Task IDs No validation on task_id format. Consider max length and UUID format validation to prevent memory exhaustion. 📊 Performance9. SWR Revalidation Strategy Current uses default SWR settings. For frequently-changing task boards, consider:
✨ Nice-to-Haves (Future)
🎓 Commendations
SummaryOverall Grade: A- This is high-quality, production-ready code. The critical race condition should be fixed before merge (simple one-line fix). Architecture follows Phase 3 guidelines perfectly. Recommendation: ✅ Approve with minor fixes Fix the race condition cleanup, preserve selection on batch error, and this is ready to merge. Great work - this sets a strong foundation for Phase 3 web UI rebuild! 🚀 |
1 participant
Summary
New Components (7)
TaskBoardViewTaskBoardContentTaskColumnTaskCardTaskDetailModalTaskFiltersBatchActionsBarAlso Includes
/taskspage route with workspace hydration guardtasksApiextensions:getAll,getOne,updateStatus,startExecution,executeBatchBatchRequest,BatchStrategy,TaskListResponseCloses #331
Test plan
next build) —/tasksroute registered/tasks, verify Kanban columns render with mock/real APISummary by CodeRabbit
New Features
Tests
Chores