fix(#101): Remediate code review findings for TaskProgressWidget
- Fix CRITICAL: Replace .sort() state mutation with [...tasks].sort() - Fix CRITICAL: Replace PDA-unfriendly red colors with calm amber tones - Fix IMPORTANT: Add TaskProgressWidget + ActiveProjectsWidget to WidgetComponentType - Fix IMPORTANT: Add tests for interval cleanup, HTTP error responses, slice limit - 3 new tests added (10 total) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -40,7 +40,7 @@ function getStatusIcon(status: string): React.JSX.Element {
|
|||||||
return <CheckCircle className="w-4 h-4 text-green-500" />;
|
return <CheckCircle className="w-4 h-4 text-green-500" />;
|
||||||
case "failed":
|
case "failed":
|
||||||
case "killed":
|
case "killed":
|
||||||
return <XCircle className="w-4 h-4 text-red-500" />;
|
return <XCircle className="w-4 h-4 text-amber-500" />;
|
||||||
default:
|
default:
|
||||||
return <Clock className="w-4 h-4 text-gray-400" />;
|
return <Clock className="w-4 h-4 text-gray-400" />;
|
||||||
}
|
}
|
||||||
@@ -73,7 +73,7 @@ function getStatusColor(status: string): string {
|
|||||||
return "bg-green-50 border-green-200 dark:bg-green-950 dark:border-green-800";
|
return "bg-green-50 border-green-200 dark:bg-green-950 dark:border-green-800";
|
||||||
case "failed":
|
case "failed":
|
||||||
case "killed":
|
case "killed":
|
||||||
return "bg-red-50 border-red-200 dark:bg-red-950 dark:border-red-800";
|
return "bg-amber-50 border-amber-200 dark:bg-amber-950 dark:border-amber-800";
|
||||||
default:
|
default:
|
||||||
return "bg-gray-50 border-gray-200 dark:bg-gray-900 dark:border-gray-700";
|
return "bg-gray-50 border-gray-200 dark:bg-gray-900 dark:border-gray-700";
|
||||||
}
|
}
|
||||||
@@ -169,9 +169,9 @@ export function TaskProgressWidget({ id: _id, config: _config }: WidgetProps): R
|
|||||||
</div>
|
</div>
|
||||||
<div className="text-green-500">Done</div>
|
<div className="text-green-500">Done</div>
|
||||||
</div>
|
</div>
|
||||||
<div className="bg-red-50 dark:bg-red-950 rounded p-2">
|
<div className="bg-amber-50 dark:bg-amber-950 rounded p-2">
|
||||||
<div className="text-lg font-bold text-red-600 dark:text-red-400">{stats.failed}</div>
|
<div className="text-lg font-bold text-amber-600 dark:text-amber-400">{stats.failed}</div>
|
||||||
<div className="text-red-500">Stopped</div>
|
<div className="text-amber-500">Stopped</div>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
@@ -180,7 +180,7 @@ export function TaskProgressWidget({ id: _id, config: _config }: WidgetProps): R
|
|||||||
{tasks.length === 0 ? (
|
{tasks.length === 0 ? (
|
||||||
<div className="text-center text-gray-500 text-sm py-4">No agent tasks in progress</div>
|
<div className="text-center text-gray-500 text-sm py-4">No agent tasks in progress</div>
|
||||||
) : (
|
) : (
|
||||||
tasks
|
[...tasks]
|
||||||
.sort((a, b) => {
|
.sort((a, b) => {
|
||||||
// Active tasks first, then by spawn time
|
// Active tasks first, then by spawn time
|
||||||
const statusOrder: Record<string, number> = {
|
const statusOrder: Record<string, number> = {
|
||||||
@@ -217,7 +217,7 @@ export function TaskProgressWidget({ id: _id, config: _config }: WidgetProps): R
|
|||||||
<span>{getElapsedTime(task.spawnedAt, task.completedAt)}</span>
|
<span>{getElapsedTime(task.spawnedAt, task.completedAt)}</span>
|
||||||
</div>
|
</div>
|
||||||
{task.error && (
|
{task.error && (
|
||||||
<div className="text-xs text-red-600 dark:text-red-400 mt-1 truncate">
|
<div className="text-xs text-amber-600 dark:text-amber-400 mt-1 truncate">
|
||||||
{task.error}
|
{task.error}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|||||||
@@ -2,8 +2,8 @@
|
|||||||
* TaskProgressWidget Component Tests
|
* TaskProgressWidget Component Tests
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
import { render, screen, waitFor } from "@testing-library/react";
|
import { render, screen, waitFor, cleanup } from "@testing-library/react";
|
||||||
import { TaskProgressWidget } from "../TaskProgressWidget";
|
import { TaskProgressWidget } from "../TaskProgressWidget";
|
||||||
|
|
||||||
const mockFetch = vi.fn();
|
const mockFetch = vi.fn();
|
||||||
@@ -14,6 +14,10 @@ describe("TaskProgressWidget", (): void => {
|
|||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
afterEach((): void => {
|
||||||
|
cleanup();
|
||||||
|
});
|
||||||
|
|
||||||
it("should render loading state initially", (): void => {
|
it("should render loading state initially", (): void => {
|
||||||
mockFetch.mockImplementation(
|
mockFetch.mockImplementation(
|
||||||
() =>
|
() =>
|
||||||
@@ -148,6 +152,62 @@ describe("TaskProgressWidget", (): void => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should display error for non-ok HTTP responses", async (): Promise<void> => {
|
||||||
|
mockFetch.mockResolvedValueOnce({
|
||||||
|
ok: false,
|
||||||
|
status: 500,
|
||||||
|
} as unknown as Response);
|
||||||
|
|
||||||
|
render(<TaskProgressWidget id="task-progress-1" />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.getByText(/unable to reach orchestrator/i)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should clear interval on unmount", async (): Promise<void> => {
|
||||||
|
const clearIntervalSpy = vi.spyOn(global, "clearInterval");
|
||||||
|
|
||||||
|
mockFetch.mockResolvedValue({
|
||||||
|
ok: true,
|
||||||
|
json: () => Promise.resolve([]),
|
||||||
|
} as unknown as Response);
|
||||||
|
|
||||||
|
const { unmount } = render(<TaskProgressWidget id="task-progress-1" />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.getByText(/no agent tasks in progress/i)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
unmount();
|
||||||
|
|
||||||
|
expect(clearIntervalSpy).toHaveBeenCalled();
|
||||||
|
clearIntervalSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should limit displayed tasks to 10", async (): Promise<void> => {
|
||||||
|
const mockTasks = Array.from({ length: 15 }, (_, i) => ({
|
||||||
|
agentId: `agent-${String(i)}`,
|
||||||
|
taskId: `SLICE-${String(i).padStart(3, "0")}`,
|
||||||
|
status: "running",
|
||||||
|
agentType: "worker",
|
||||||
|
spawnedAt: new Date().toISOString(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
mockFetch.mockResolvedValueOnce({
|
||||||
|
ok: true,
|
||||||
|
json: () => Promise.resolve(mockTasks),
|
||||||
|
} as unknown as Response);
|
||||||
|
|
||||||
|
render(<TaskProgressWidget id="task-progress-1" />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
// Only 10 task cards should be rendered despite 15 tasks
|
||||||
|
const workerBadges = screen.getAllByText("Worker");
|
||||||
|
expect(workerBadges).toHaveLength(10);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("should sort active tasks before completed ones", async (): Promise<void> => {
|
it("should sort active tasks before completed ones", async (): Promise<void> => {
|
||||||
const mockTasks = [
|
const mockTasks = [
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -69,6 +69,8 @@ export type WidgetComponentType =
|
|||||||
| "CalendarWidget"
|
| "CalendarWidget"
|
||||||
| "QuickCaptureWidget"
|
| "QuickCaptureWidget"
|
||||||
| "AgentStatusWidget"
|
| "AgentStatusWidget"
|
||||||
|
| "ActiveProjectsWidget"
|
||||||
|
| "TaskProgressWidget"
|
||||||
| "StatCardWidget"
|
| "StatCardWidget"
|
||||||
| "ChartWidget"
|
| "ChartWidget"
|
||||||
| "ListWidget"
|
| "ListWidget"
|
||||||
|
|||||||
Reference in New Issue
Block a user