fix(tasks): add Assign Tasks button for stuck pending tasks #250
Conversation
Fixes Issue #248 where tasks remained stuck in "pending" state with "Assigned to: Unassigned" for late-joining users or after execution failures. Changes: - Fix LeadAgent._assign_and_execute_task() to set assigned_to field before status transitions (root cause of display bug) - Add POST /api/projects/{id}/tasks/assign endpoint for manual trigger - Add tasksApi.assignPending() frontend API client method - Add "Assign Tasks" banner to TaskList when pending unassigned tasks exist - Add comprehensive tests for all new functionality The button appears contextually only when tasks are stuck, and auto-hides when tasks get assigned via WebSocket updates.
WalkthroughAdds a project-scoped POST endpoint to trigger assignment of pending unassigned tasks, updates LeadAgent to persist Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Web UI (TaskList)
participant API as Backend API
participant DB as Database
participant BG as Background Task Runner
participant LeadAgent as Lead Agent
User->>Frontend: Click "Assign Tasks" button
Frontend->>API: POST /api/projects/{id}/tasks/assign
API->>DB: Validate project, user, phase
API->>DB: Count pending unassigned tasks
alt pending tasks exist and no execution in progress
API->>BG: Schedule start_development_execution (background)
API->>Frontend: Return success + pending_count + message
BG->>LeadAgent: start_development_execution
LeadAgent->>DB: Update task -> set assigned_to = agent_id
LeadAgent->>DB: Update task -> set status = in_progress
LeadAgent->>DB: Continue task execution...
else no pending tasks
API->>Frontend: Return success (pending_count = 0)
end
Frontend->>Frontend: Update banner/UI state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)codeframe/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
Comment |
Add Assign Tasks button in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| // Assignment state (Issue #248 fix) | ||
| const [isAssigning, setIsAssigning] = useState(false); | ||
| const [assignmentError, setAssignmentError] = useState<string | null>(null); |
There was a problem hiding this comment.
assignmentError persists across banner hide/show, so a past error is shown when new pending tasks appear. Consider resetting assignmentError when hasPendingUnassigned changes (e.g., clear on hide or when it becomes true) so the banner starts clean.
🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
| // Success - the WebSocket will update the task list as agents are assigned | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Failed to assign tasks'; |
There was a problem hiding this comment.
handleAssignTasks surfaces generic error.message and misses server messages from error.response?.data?.message. Consider preferring the response message (with a safe runtime check) so users see meaningful errors.
| const errorMessage = error instanceof Error ? error.message : 'Failed to assign tasks'; | |
| const errorMessage = ((error as any)?.response?.data?.message as string) ?? (error instanceof Error ? error.message : 'Failed to assign tasks'); |
🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review: PR #250 - Assign Tasks Button FixOverall assessment: LGTM with 1 blocking issue and a few suggestions ✅ Strengths
🚫 Blocking Issuetasks.py:455-467 - Misleading success response when API key missing When # Current behavior - misleading:
if api_key:
background_tasks.add_task(...)
logger.info(f"✅ Scheduled task assignment...")
else:
logger.warning(f"⚠️ ANTHROPIC_API_KEY not configured...") # Nothing happens!
return TaskAssignmentResponse(
success=True, # ❌ Misleading!
pending_count=pending_count,
message=f"Assignment started for {pending_count} pending task(s)." # ❌ False!
)Suggested fix: if api_key:
background_tasks.add_task(...)
logger.info(f"✅ Scheduled task assignment...")
return TaskAssignmentResponse(
success=True,
pending_count=pending_count,
message=f"Assignment started for {pending_count} pending task(s)."
)
else:
logger.warning(f"⚠️ ANTHROPIC_API_KEY not configured...")
return TaskAssignmentResponse(
success=False,
pending_count=pending_count,
message="Cannot assign tasks: ANTHROPIC_API_KEY not configured"
)📝 Suggestions1. Prevent duplicate assignment triggers (Low priority)Currently, a user could click "Assign Tasks" multiple times before the first execution completes, triggering multiple background executions. Consider adding a flag to track if assignment is in progress: # In assign_pending_tasks():
# Check if there are already tasks in assigned or in_progress state
executing_or_assigned = [
t for t in tasks
if t.status in [TaskStatus.ASSIGNED, TaskStatus.IN_PROGRESS]
]
if executing_or_assigned:
return TaskAssignmentResponse(
success=False,
pending_count=pending_count,
message="Assignment already in progress. Wait for current tasks to complete."
)2. Frontend: Consider optimistic update (Nice to have)The frontend relies on WebSocket updates to refresh task status. Consider an optimistic update to show "Assigning..." status immediately on pending tasks for better UX: // After calling assignPending(), temporarily mark tasks as assigning
const handleAssignTasks = useCallback(async () => {
setIsAssigning(true);
setAssignmentError(null);
try {
const response = await tasksApi.assignPending(projectId);
if (response.data.success) {
// Optional: Mark pending tasks as "assigning" locally for immediate feedback
// WebSocket will eventually update with actual agent IDs
}
} ...
}, [projectId]);3. Add logging for edge case (Debugging)In if pending_count == 0:
logger.debug(
f"assign_pending_tasks called for project {project_id} but found 0 pending unassigned tasks. "
f"Total tasks: {len(tasks)}, statuses: {[t.status for t in tasks]}"
)
return TaskAssignmentResponse(...)✅ Code Quality Notes
Test Coverage✅ Unit tests cover:
✅ Integration tests verify:
Verdict: Fix the blocking issue and this is ready to merge. The suggestions are optional improvements for future iterations. |
|
I've reviewed PR #250 and posted a detailed code review as a comment on the PR. Review SummaryOverall Assessment: LGTM with 1 blocking issue Blocking Issue Foundtasks.py:455-467 - When Key Strengths
Suggestions
The PR is well-structured with good test coverage. Fixing the blocking issue about the misleading success response when the API key is missing would make this ready to merge. |
![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
🤖 Fix all issues with AI agents
In @codeframe/ui/routers/tasks.py:
- Around line 453-473: The response always indicates success even when
ANTHROPIC_API_KEY is missing; update the logic around api_key and the
TaskAssignmentResponse so that when api_key is falsy the endpoint returns
success=False (or an error code) and a clear message like "ANTHROPIC_API_KEY not
configured - assignment not started" instead of "Assignment started...";
concretely, in the block using api_key and background_tasks (references:
api_key, start_development_execution, TaskAssignmentResponse, logger),
move/modify the return to occur after the api_key check so the success and
message reflect whether background_tasks.add_task was called (return
success=True only when api_key truthy and task scheduled, otherwise return
success=False with the warning message).
🧹 Nitpick comments (3)
codeframe/agents/lead_agent.py (1)
1873-1876: Consider combining into a single atomic update.The fix correctly sets
assigned_tobefore the status change, which addresses Issue #248. However, two separateupdate_taskcalls introduce a race window where another process could read inconsistent state between the updates. Combining them into a single call improves atomicity and reduces database round-trips.♻️ Suggested improvement
- # Update task with assigned agent and status (Issue #248 fix) - # Set assigned_to BEFORE status change so UI shows assignment immediately - self.db.update_task(task.id, {"assigned_to": agent_id}) - self.db.update_task(task.id, {"status": "in_progress"}) + # Update task with assigned agent and status atomically (Issue #248 fix) + # Both fields updated together so UI shows assignment immediately + self.db.update_task(task.id, {"assigned_to": agent_id, "status": "in_progress"})web-ui/src/components/TaskList.tsx (1)
221-233: Consider combining derived state into a single memo.Both
hasPendingUnassignedandpendingUnassignedCountiterate overprojectTaskswith the same filter logic. Combining them reduces redundant iterations.♻️ Suggested improvement
- // Check if there are pending unassigned tasks (Issue #248 fix) - const hasPendingUnassigned = useMemo(() => { - return projectTasks.some( - (task) => task.status === 'pending' && !task.agent_id - ); - }, [projectTasks]); - - // Count of pending unassigned tasks - const pendingUnassignedCount = useMemo(() => { - return projectTasks.filter( - (task) => task.status === 'pending' && !task.agent_id - ).length; - }, [projectTasks]); + // Pending unassigned task stats (Issue #248 fix) + const { hasPendingUnassigned, pendingUnassignedCount } = useMemo(() => { + const pendingUnassigned = projectTasks.filter( + (task) => task.status === 'pending' && !task.agent_id + ); + return { + hasPendingUnassigned: pendingUnassigned.length > 0, + pendingUnassignedCount: pendingUnassigned.length, + }; + }, [projectTasks]);tests/integration/test_multi_agent_execution.py (1)
724-783: Makeget_or_create_agentmocking argument-driven (not call-order) to avoid brittle tests.
Right now the “first call worker / second call review” assumption will break if_assign_and_execute_task()adds one moreget_or_create_agent()call internally.Proposed change
- # Also mock the review agent to avoid review step - with patch.object(lead_agent.agent_pool_manager, "get_or_create_agent") as mock_get_agent: - # First call returns worker agent, second call returns review agent - mock_get_agent.side_effect = ["test-agent-001", "review-agent-001"] + # Mock worker vs review agent selection based on requested agent type (more robust than call-order) + def get_agent_side_effect(agent_type, *args, **kwargs): + return "review-agent-001" if agent_type == "review" else "test-agent-001" + + mock_pool.get_or_create_agent.side_effect = get_agent_side_effect
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codeframe/agents/lead_agent.pycodeframe/ui/routers/tasks.pytests/integration/test_multi_agent_execution.pytests/ui/test_assign_pending_tasks.pyweb-ui/src/components/TaskList.tsxweb-ui/src/lib/api.ts
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development
Use React 18 and Next.js 14 for frontend development
Use Tailwind CSS for styling with Nova design system template
Use shadcn/ui components from @/components/ui/ for UI elements
Use Hugeicons (@hugeicons/react) for all icons, never mix with lucide-react
Use Nova color palette variables (bg-card, text-foreground, etc.) instead of hardcoded color values
Use cn() utility for conditional CSS classes in React components
Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Store auth tokens in localStorage with key 'auth_token' and include token in API requests via Authorization header
Files:
web-ui/src/lib/api.tsweb-ui/src/components/TaskList.tsx
web-ui/src/lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement WebSocket automatic reconnection with exponential backoff (1s → 30s) and full state resync
Files:
web-ui/src/lib/api.ts
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development
Use FastAPI framework for backend API development
Use SQLite with async support (aiosqlite) for database operations
Use tiktoken for token counting
Run linting with ruff check .
Import and use domain-specific repositories from codeframe/persistence/repositories/ for data access instead of direct database calls
Files:
codeframe/agents/lead_agent.pycodeframe/ui/routers/tasks.py
web-ui/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.{ts,tsx}: Use React Context with useReducer for centralized state management in Dashboard
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Files:
web-ui/src/components/TaskList.tsx
🧠 Learnings (5)
📚 Learning: 2025-12-17T19:21:40.014Z
Learnt from: frankbria
Repo: frankbria/codeframe PR: 128
File: tests/agents/test_bottleneck_detection.py:486-500
Timestamp: 2025-12-17T19:21:40.014Z
Learning: In tests/agents/test_bottleneck_detection.py, test_skip_agents_below_threshold should be async and mock _get_agent_workload to return workload below AGENT_OVERLOAD_THRESHOLD (5) while providing non-empty tasks list to prevent early return in detect_bottlenecks().
Applied to files:
tests/integration/test_multi_agent_execution.py
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/reducers/**/*.{ts,tsx} : Use useReducer with 13+ action types for agent state management; implement timestamp conflict resolution with last-write-wins strategy
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Applied to files:
web-ui/src/components/TaskList.tsx
🧬 Code graph analysis (1)
web-ui/src/components/TaskList.tsx (1)
web-ui/src/lib/api.ts (1)
tasksApi(148-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: Macroscope - Correctness Check
- GitHub Check: opencode-review
🔇 Additional comments (11)
web-ui/src/lib/api.ts (1)
153-167: LGTM!The new
assignPendingmethod follows established patterns in this file. The response type matchesTaskAssignmentResponsefrom the backend, and the JSDoc documentation clearly explains the purpose and usage.web-ui/src/components/TaskList.tsx (3)
182-184: LGTM!State management for the assignment flow is clean and follows React patterns. The separation of loading (
isAssigning) and error (assignmentError) states enables good UX feedback.
236-251: LGTM!The handler properly manages loading state, error handling, and cleanup via
finally. Relying on WebSocket updates for task list refresh (rather than manual refetch) is the correct approach given the existing architecture.
301-327: LGTM!The banner implementation follows the Nova design system with proper color variables, includes accessible disabled states, handles singular/plural grammar, and provides clear error feedback. Good use of
data-testidattributes for testing.codeframe/ui/routers/tasks.py (2)
380-384: LGTM!The
TaskAssignmentResponsemodel is well-defined and matches the frontend's expected response structure.
418-451: LGTM!Validation logic follows established patterns in this file. The early return for zero pending tasks avoids unnecessary background task scheduling.
tests/ui/test_assign_pending_tasks.py (4)
17-27: LGTM!Good fixture design that ensures deterministic tests by clearing the API key by default. Tests that need API key behavior explicitly set it via
patch.dict.
60-182: LGTM!Comprehensive test coverage for success and error paths. Tests properly verify both response values and side effects (background task scheduling).
184-217: Test correctly validates current behavior, but may need update.This test validates the current implementation where
success=Trueis returned even when the API key is missing. If the endpoint is updated to returnsuccess=Falsewhen the API key is not configured (as suggested in the tasks.py review), this test's assertion on line 210 will need to change toassert response.success is False.
219-277: LGTM!Thorough test for counting logic with mixed task states. The response model and function signature tests provide good regression protection against accidental API changes.
tests/integration/test_multi_agent_execution.py (1)
827-897: The test assertion is already correct and resilient; the suggested changes are unnecessary.The implementation at line 1876 uses a hard-coded string
{"status": "in_progress"}, not an enum, and this matches the test's assertion. Checking for bothTaskStatus.IN_PROGRESS.valueand the string adds complexity without benefit. Additionally,assigned_tois updated only once per execution (line 1875), so capturing the "first" occurrence is defensive for a non-existent issue. The test as written correctly validates the execution order and handles the actual implementation behavior.Likely an incorrect or invalid review comment.
Phase 1 fix for concurrent execution issue: - Backend: Check for in_progress tasks before scheduling new execution - Frontend: Disable button when tasks are already running - Frontend: Show "Running..." state and updated message when execution active - Add 2 new tests for execution blocking behavior
| tasks = db.get_project_tasks(project_id) | ||
| pending_unassigned = [ | ||
| t for t in tasks | ||
| if t.status == TaskStatus.PENDING and not t.assigned_to |
There was a problem hiding this comment.
t.status may be a string, so comparing to TaskStatus members (TaskStatus.PENDING / TaskStatus.IN_PROGRESS) will fail and skip tasks. Consider normalizing t.status to a TaskStatus before filtering, or compare to .value if statuses are stored as strings.
🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web-ui/src/components/TaskList.tsx (1)
221-238: Consider combininghasPendingUnassignedandpendingUnassignedCountinto a single memo.Both memos iterate over
projectTaskswith the same filter condition. Combining them into one pass would avoid redundant iteration.♻️ Suggested optimization
- // Check if there are pending unassigned tasks (Issue #248 fix) - const hasPendingUnassigned = useMemo(() => { - return projectTasks.some( - (task) => task.status === 'pending' && !task.agent_id - ); - }, [projectTasks]); - - // Count of pending unassigned tasks - const pendingUnassignedCount = useMemo(() => { - return projectTasks.filter( - (task) => task.status === 'pending' && !task.agent_id - ).length; - }, [projectTasks]); + // Count and check for pending unassigned tasks (Issue #248 fix) + const pendingUnassignedCount = useMemo(() => { + return projectTasks.filter( + (task) => task.status === 'pending' && !task.agent_id + ).length; + }, [projectTasks]); + + const hasPendingUnassigned = pendingUnassignedCount > 0;tests/ui/test_assign_pending_tasks.py (1)
219-245: Test name and assertion may be misleading.The test is named
test_assign_pending_tasks_only_counts_unassigned, but it also implicitly tests the "blocked when in_progress" behavior due to theIN_PROGRESStask at line 230. The assertion at lines 243-244 checks for "in progress" in the message, which is about blocking, not counting.Consider splitting this into two separate tests or renaming to clarify its scope.
♻️ Suggested clarification
Option 1: Remove the
IN_PROGRESStask from this test to purely test counting:mock_db.get_project_tasks.return_value = [ Task(id=1, project_id=1, title="Task 1", status=TaskStatus.PENDING, assigned_to=None), # Count this Task(id=2, project_id=1, title="Task 2", status=TaskStatus.PENDING, assigned_to="agent-1"), # Already assigned - Task(id=3, project_id=1, title="Task 3", status=TaskStatus.IN_PROGRESS, assigned_to=None), # Not pending + Task(id=3, project_id=1, title="Task 3", status=TaskStatus.COMPLETED, assigned_to="agent-1"), # Not pending ]Option 2: Rename the test to reflect its actual behavior, e.g.,
test_assign_pending_tasks_counts_unassigned_and_respects_in_progress.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codeframe/ui/routers/tasks.pytests/ui/test_assign_pending_tasks.pyweb-ui/src/components/TaskList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- codeframe/ui/routers/tasks.py
🧰 Additional context used
📓 Path-based instructions (2)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development
Use React 18 and Next.js 14 for frontend development
Use Tailwind CSS for styling with Nova design system template
Use shadcn/ui components from @/components/ui/ for UI elements
Use Hugeicons (@hugeicons/react) for all icons, never mix with lucide-react
Use Nova color palette variables (bg-card, text-foreground, etc.) instead of hardcoded color values
Use cn() utility for conditional CSS classes in React components
Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Store auth tokens in localStorage with key 'auth_token' and include token in API requests via Authorization header
Files:
web-ui/src/components/TaskList.tsx
web-ui/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.{ts,tsx}: Use React Context with useReducer for centralized state management in Dashboard
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Files:
web-ui/src/components/TaskList.tsx
🧠 Learnings (4)
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/reducers/**/*.{ts,tsx} : Use useReducer with 13+ action types for agent state management; implement timestamp conflict resolution with last-write-wins strategy
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/TaskList.tsx
📚 Learning: 2026-01-11T23:33:31.883Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.883Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Applied to files:
web-ui/src/components/TaskList.tsx
🧬 Code graph analysis (2)
web-ui/src/components/TaskList.tsx (1)
web-ui/src/lib/api.ts (1)
tasksApi(148-168)
tests/ui/test_assign_pending_tasks.py (1)
codeframe/ui/routers/tasks.py (2)
assign_pending_tasks(388-486)TaskAssignmentResponse(380-384)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
🔇 Additional comments (9)
web-ui/src/components/TaskList.tsx (4)
14-14: LGTM!The
tasksApiimport correctly references the new API module for the assignment feature.
182-184: LGTM!The assignment state variables follow proper patterns for managing async operation lifecycle (loading + error states).
240-256: LGTM with a minor observation.The handler correctly manages loading state, clears previous errors, and handles both API-level failures (
!success) and network exceptions. TheuseCallbackdependency array is correct.One consideration: Axios errors may contain less user-friendly messages (e.g., "Request failed with status code 500"). If improved error messaging is desired, you could extract the server's error detail from
error.response?.data?.detail.
306-336: Well-implemented assignment banner.The banner correctly:
- Uses Nova design system variables (
bg-muted/50,text-foreground, etc.) per coding guidelines- Provides clear contextual messaging for in-progress vs. waiting states
- Disables the button appropriately during assignment or when tasks are running
- Displays errors prominently when they occur
The grammar handling for singular/plural ("task is" vs "tasks are") is a nice touch for UX polish.
tests/ui/test_assign_pending_tasks.py (5)
17-57: Well-structured test fixtures.The fixtures are clean, documented, and follow good practices:
mock_background_taskscorrectly clearsANTHROPIC_API_KEYfor deterministic behavior- Using
spec=BackgroundTasksensures mock integrity- Clear setup for the "happy path" in
mock_db
247-304: LGTM!The tests for execution-in-progress blocking are comprehensive:
test_assign_pending_tasks_blocked_when_execution_in_progresscorrectly verifies that the background task is NOT scheduled when tasks are already runningtest_assign_pending_tasks_allowed_when_no_execution_in_progressconfirms execution proceeds normally otherwiseBoth tests have clear assertions covering
success,pending_count, message content, andadd_taskinvocation.
307-322: LGTM!Basic structural validation of the response model. This ensures the model contract is maintained.
325-339: LGTM!The signature test is a useful contract verification that catches accidental breaking changes to the endpoint interface.
60-61: Good test coverage for the new endpoint.The test suite covers:
- Success paths (pending tasks exist, no pending tasks)
- Error paths (wrong phase, not found, access denied)
- Edge cases (missing API key, counting logic, concurrent execution blocking)
- Model structure and function signature validation
This provides solid confidence for the Issue #248 fix.
Code Review Summary🔴 Blocking Issue (NOT YET FIXED)**** - Misleading response when API key missing The endpoint returns with message "Assignment started..." even when is not configured and no background task is scheduled. This will confuse users who think assignment is happening when nothing will occur. # Current (lines 482-486):
return TaskAssignmentResponse(
success=True, # ❌ Misleading!
pending_count=pending_count,
message=f"Assignment started for {pending_count} pending task(s)."
)Fix required: Move the return statement inside the block and return when API key is missing. ✅ Issues Already AddressedThe following improvements from previous reviews have been implemented:
|
Code Review CompleteReviewed PR #250 and posted a comprehensive review comment at: 🔴 Blocking Issue Found
This issue was raised by all previous reviewers (macroscopeapp, coderabbitai, github-actions) but has not been fixed in the current code. ✅ Issues Already Addressed
📊 Overall Assessment
Status: 🔴 1 blocking issue prevents merge - Fix API key response logic at tasks.py:467-486 |
- Return success=False when API key missing (blocking issue) - Add ASSIGNED status check to prevent duplicate triggers - Add debug logging for zero pending tasks case - Add test for ASSIGNED status blocking
Code Review Summary✅ Blocking Issue FIXEDThe API key handling issue raised by previous reviewers has been successfully addressed in commit Fix Verified (tasks.py:476-500):
📊 Overall Assessment
Status: ✅ Ready to merge - All blocking issues resolved 📝 Optional Improvements (Non-blocking)1. Frontend Error Handling EnhancementLocation: Currently uses generic error messages. Could extract server messages for better user feedback: const errorMessage = ((error as any)?.response?.data?.message as string)
?? (error instanceof Error ? error.message : 'Failed to assign tasks');2. Performance Optimization (Minor)Location: Two memos iterate const pendingUnassignedCount = useMemo(() => {
return projectTasks.filter(
(task) => task.status === 'pending' && !task.agent_id
).length;
}, [projectTasks]);
const hasPendingUnassigned = pendingUnassignedCount > 0;✅ Strengths
Verdict: ✅ LGTM - The PR is well-structured and addresses all requirements. The optional improvements above are nice-to-haves for future iterations. |
Review CompleteI've reviewed PR #250 and posted my feedback as a comment on the PR. SummaryStatus: ✅ Ready to merge Key Findings
Optional Improvements (Non-blocking)
Test Coverage
The PR is well-structured with good test coverage and all blocking issues resolved. The suggested improvements are nice-to-haves for future iterations. |
Summary
Fixes #248 - Tasks stuck in "pending" state with "Assigned to: Unassigned" for late-joining users
assigned_tofield when assigning tasks (root cause of display bug)POST /api/projects/{id}/tasks/assignChanges
codeframe/agents/lead_agent.pyassigned_tobefore status update in_assign_and_execute_task()codeframe/ui/routers/tasks.pyassign_pending_tasksendpoint andTaskAssignmentResponsemodelweb-ui/src/lib/api.tstasksApi.assignPending()methodweb-ui/src/components/TaskList.tsxtests/ui/test_assign_pending_tasks.pytests/integration/test_multi_agent_execution.pyassigned_tofixRoot Cause Analysis
The original
_assign_and_execute_task()never settask.assigned_to, so the UI correctly showed "Unassigned" even during active execution. This was a pre-existing bug that the original plan didn't identify.Test plan
Summary by CodeRabbit
New Features
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.