fix: remediate dashboard API integration review blockers (#459)
- Fix race condition: guard useEffect when workspaceId is null, prevent infinite loading state by setting isLoading=false on null workspace - Fix TypeScript strict typing: @Workspace() returns string|undefined, controller now matches with BadRequestException guard - Narrow details DTO type from unknown to Record<string, unknown>|null - Add error state UI for API fetch failures - Add error-path test with static mock import pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,8 +1,9 @@
|
|||||||
import { Controller, Get, UseGuards } from "@nestjs/common";
|
import { Controller, Get, UseGuards, BadRequestException } from "@nestjs/common";
|
||||||
import { DashboardService } from "./dashboard.service";
|
import { DashboardService } from "./dashboard.service";
|
||||||
import { AuthGuard } from "../auth/guards/auth.guard";
|
import { AuthGuard } from "../auth/guards/auth.guard";
|
||||||
import { WorkspaceGuard, PermissionGuard } from "../common/guards";
|
import { WorkspaceGuard, PermissionGuard } from "../common/guards";
|
||||||
import { Workspace, Permission, RequirePermission } from "../common/decorators";
|
import { Workspace, Permission, RequirePermission } from "../common/decorators";
|
||||||
|
import type { DashboardSummaryDto } from "./dto";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Controller for dashboard endpoints.
|
* Controller for dashboard endpoints.
|
||||||
@@ -25,7 +26,10 @@ export class DashboardController {
|
|||||||
*/
|
*/
|
||||||
@Get("summary")
|
@Get("summary")
|
||||||
@RequirePermission(Permission.WORKSPACE_ANY)
|
@RequirePermission(Permission.WORKSPACE_ANY)
|
||||||
async getSummary(@Workspace() workspaceId: string) {
|
async getSummary(@Workspace() workspaceId: string | undefined): Promise<DashboardSummaryDto> {
|
||||||
|
if (!workspaceId) {
|
||||||
|
throw new BadRequestException("Workspace context required");
|
||||||
|
}
|
||||||
return this.dashboardService.getSummary(workspaceId);
|
return this.dashboardService.getSummary(workspaceId);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -142,7 +142,7 @@ export class DashboardService {
|
|||||||
action: row.action,
|
action: row.action,
|
||||||
entityType: row.entityType,
|
entityType: row.entityType,
|
||||||
entityId: row.entityId,
|
entityId: row.entityId,
|
||||||
details: row.details,
|
details: row.details as Record<string, unknown> | null,
|
||||||
userId: row.userId,
|
userId: row.userId,
|
||||||
createdAt: row.createdAt.toISOString(),
|
createdAt: row.createdAt.toISOString(),
|
||||||
}));
|
}));
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ export class RecentActivityDto {
|
|||||||
action!: string;
|
action!: string;
|
||||||
entityType!: string;
|
entityType!: string;
|
||||||
entityId!: string;
|
entityId!: string;
|
||||||
details!: unknown;
|
details!: Record<string, unknown> | null;
|
||||||
userId!: string;
|
userId!: string;
|
||||||
createdAt!: string;
|
createdAt!: string;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||||
import { render, screen, waitFor } from "@testing-library/react";
|
import { render, screen, waitFor } from "@testing-library/react";
|
||||||
import DashboardPage from "./page";
|
import DashboardPage from "./page";
|
||||||
|
import { fetchDashboardSummary } from "@/lib/api/dashboard";
|
||||||
|
|
||||||
// Mock Phase 3 dashboard widgets
|
// Mock Phase 3 dashboard widgets
|
||||||
vi.mock("@/components/dashboard/DashboardMetrics", () => ({
|
vi.mock("@/components/dashboard/DashboardMetrics", () => ({
|
||||||
@@ -51,6 +52,19 @@ vi.mock("@/lib/api/dashboard", () => ({
|
|||||||
describe("DashboardPage", (): void => {
|
describe("DashboardPage", (): void => {
|
||||||
beforeEach((): void => {
|
beforeEach((): void => {
|
||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
|
vi.mocked(fetchDashboardSummary).mockResolvedValue({
|
||||||
|
metrics: {
|
||||||
|
activeAgents: 5,
|
||||||
|
tasksCompleted: 42,
|
||||||
|
totalTasks: 100,
|
||||||
|
tasksInProgress: 10,
|
||||||
|
activeProjects: 3,
|
||||||
|
errorRate: 0.5,
|
||||||
|
},
|
||||||
|
recentActivity: [],
|
||||||
|
activeJobs: [],
|
||||||
|
tokenBudget: [],
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should render the DashboardMetrics widget", async (): Promise<void> => {
|
it("should render the DashboardMetrics widget", async (): Promise<void> => {
|
||||||
@@ -87,4 +101,12 @@ describe("DashboardPage", (): void => {
|
|||||||
expect(screen.getByTestId("token-budget")).toBeInTheDocument();
|
expect(screen.getByTestId("token-budget")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should render error state when API fails", async (): Promise<void> => {
|
||||||
|
vi.mocked(fetchDashboardSummary).mockRejectedValueOnce(new Error("Network error"));
|
||||||
|
render(<DashboardPage />);
|
||||||
|
await waitFor((): void => {
|
||||||
|
expect(screen.getByText("Failed to load dashboard data")).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -15,19 +15,30 @@ export default function DashboardPage(): ReactElement {
|
|||||||
const workspaceId = useWorkspaceId();
|
const workspaceId = useWorkspaceId();
|
||||||
const [data, setData] = useState<DashboardSummaryResponse | null>(null);
|
const [data, setData] = useState<DashboardSummaryResponse | null>(null);
|
||||||
const [isLoading, setIsLoading] = useState(true);
|
const [isLoading, setIsLoading] = useState(true);
|
||||||
|
const [error, setError] = useState<string | null>(null);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
if (!workspaceId) {
|
||||||
|
setIsLoading(false);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const wsId = workspaceId;
|
||||||
let cancelled = false;
|
let cancelled = false;
|
||||||
|
setError(null);
|
||||||
|
setIsLoading(true);
|
||||||
|
|
||||||
async function loadSummary(): Promise<void> {
|
async function loadSummary(): Promise<void> {
|
||||||
try {
|
try {
|
||||||
const summary = await fetchDashboardSummary(workspaceId ?? undefined);
|
const summary = await fetchDashboardSummary(wsId);
|
||||||
if (!cancelled) {
|
if (!cancelled) {
|
||||||
setData(summary);
|
setData(summary);
|
||||||
}
|
}
|
||||||
} catch (err: unknown) {
|
} catch (err: unknown) {
|
||||||
// Log but do not crash; widgets will render with empty states
|
|
||||||
console.error("[Dashboard] Failed to fetch summary:", err);
|
console.error("[Dashboard] Failed to fetch summary:", err);
|
||||||
|
if (!cancelled) {
|
||||||
|
setError("Failed to load dashboard data");
|
||||||
|
}
|
||||||
} finally {
|
} finally {
|
||||||
if (!cancelled) {
|
if (!cancelled) {
|
||||||
setIsLoading(false);
|
setIsLoading(false);
|
||||||
@@ -62,6 +73,21 @@ export default function DashboardPage(): ReactElement {
|
|||||||
|
|
||||||
return (
|
return (
|
||||||
<div style={{ display: "flex", flexDirection: "column", gap: 16 }}>
|
<div style={{ display: "flex", flexDirection: "column", gap: 16 }}>
|
||||||
|
{error && (
|
||||||
|
<div
|
||||||
|
style={{
|
||||||
|
padding: "12px 16px",
|
||||||
|
marginBottom: 16,
|
||||||
|
background: "rgba(229,72,77,0.1)",
|
||||||
|
border: "1px solid var(--border)",
|
||||||
|
borderRadius: "var(--r)",
|
||||||
|
color: "var(--text)",
|
||||||
|
fontSize: "0.85rem",
|
||||||
|
}}
|
||||||
|
>
|
||||||
|
{error}
|
||||||
|
</div>
|
||||||
|
)}
|
||||||
<DashboardMetrics metrics={data?.metrics} />
|
<DashboardMetrics metrics={data?.metrics} />
|
||||||
<div className="dash-grid">
|
<div className="dash-grid">
|
||||||
<div style={{ display: "flex", flexDirection: "column", gap: 16, minWidth: 0 }}>
|
<div style={{ display: "flex", flexDirection: "column", gap: 16, minWidth: 0 }}>
|
||||||
|
|||||||
@@ -23,7 +23,7 @@ export interface RecentActivity {
|
|||||||
action: string;
|
action: string;
|
||||||
entityType: string;
|
entityType: string;
|
||||||
entityId: string;
|
entityId: string;
|
||||||
details: unknown;
|
details: Record<string, unknown> | null;
|
||||||
userId: string;
|
userId: string;
|
||||||
createdAt: string;
|
createdAt: string;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,12 +2,12 @@
|
|||||||
|
|
||||||
> Single-writer: orchestrator only. Workers read but never modify.
|
> Single-writer: orchestrator only. Workers read but never modify.
|
||||||
|
|
||||||
| id | status | milestone | description | pr | notes |
|
| id | status | milestone | description | pr | notes |
|
||||||
| --------- | ----------- | --------- | ------------------------------------------------------------------------------------------------------------------------------------ | ---- | ------------------------------------------- |
|
| --------- | ----------- | --------- | ------------------------------------------------------------------------------------------------------------------------------------ | ---- | ---------------------------------------------- |
|
||||||
| MS-P1-001 | done | phase-1 | Fix broken test suites: Button.test.tsx (4 fails, old Tailwind classes) + page.test.tsx (5 fails, old widget refs) | #458 | issue #457, commit 8fa0b30 |
|
| MS-P1-001 | done | phase-1 | Fix broken test suites: Button.test.tsx (4 fails, old Tailwind classes) + page.test.tsx (5 fails, old widget refs) | #458 | issue #457, commit 8fa0b30 |
|
||||||
| MS-P1-002 | done | phase-1 | Remove legacy unused dashboard widgets: DomainOverviewWidget, RecentTasksWidget, UpcomingEventsWidget, QuickCaptureWidget | #458 | issue #457, commit 8fa0b30, 5 files deleted |
|
| MS-P1-002 | done | phase-1 | Remove legacy unused dashboard widgets: DomainOverviewWidget, RecentTasksWidget, UpcomingEventsWidget, QuickCaptureWidget | #458 | issue #457, commit 8fa0b30, 5 files deleted |
|
||||||
| MS-P1-003 | done | phase-1 | Visual + theme polish: audit current vs design reference, fix gaps, verify dark/light across all components, responsive verification | #458 | issue #457, commit d97a98b, review: approve |
|
| MS-P1-003 | done | phase-1 | Visual + theme polish: audit current vs design reference, fix gaps, verify dark/light across all components, responsive verification | #458 | issue #457, commit d97a98b, review: approve |
|
||||||
| MS-P1-004 | done | phase-1 | Phase verification: all quality gates pass (lint 8/8, typecheck 7/7, test 8/8) | #458 | issue #457, merged 07f5225, issue closed |
|
| MS-P1-004 | done | phase-1 | Phase verification: all quality gates pass (lint 8/8, typecheck 7/7, test 8/8) | #458 | issue #457, merged 07f5225, issue closed |
|
||||||
| MS-P2-001 | not-started | phase-2 | Create dashboard summary API endpoint: aggregate task counts, project counts, recent activity, active jobs in single call | — | issue #459, est 20K |
|
| MS-P2-001 | done | phase-2 | Create dashboard summary API endpoint: aggregate task counts, project counts, recent activity, active jobs in single call | — | issue #459, commit e38aaa9, 7 files +430 lines |
|
||||||
| MS-P2-002 | not-started | phase-2 | Wire dashboard widgets to real API data: ActivityFeed, DashboardMetrics, OrchestratorSessions replace mock with API calls | — | issue #459, est 25K, depends MS-P2-001 |
|
| MS-P2-002 | done | phase-2 | Wire dashboard widgets to real API data: ActivityFeed, DashboardMetrics, OrchestratorSessions replace mock with API calls | — | issue #459, commit 7c762e6 + remediation |
|
||||||
| MS-P2-003 | not-started | phase-2 | Phase verification: create task via API, confirm visible in dashboard, all quality gates pass | — | issue #459, est 10K, depends MS-P2-002 |
|
| MS-P2-003 | not-started | phase-2 | Phase verification: create task via API, confirm visible in dashboard, all quality gates pass | — | issue #459, est 10K, depends MS-P2-002 |
|
||||||
|
|||||||
Reference in New Issue
Block a user