adhit-r / adhit-r/fairmind

Fix ApiResponse<T> handling + wire up broken compliance-automation feature (~19 TS errors)

Aperta
#162 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
bug frontend help wanted typescript
Lingua principale
Python
Stelle
9
Fork
12
Merge medio
2h 23m
PR unite (30g)
2

Descrizione

## Summary

The frontend has **~19 TypeScript errors** caused by inconsistent handling of the `ApiResponse` wrapper type. The errors cluster into two categories:

1. **Half-built `compliance-automation` feature** — the service file (`compliance-automation-service.ts`) imports a non-existent `api` export and exports `scheduleAPI`/`violationAPI`/`reportAPI` while consumers (`ScheduleManager.tsx`, `ViolationAlerts.tsx`, `compliance-automation/page.tsx`) call methods on a `complianceAutomationService` object that doesn't exist. The whole feature needs to be wired up correctly.
2. **Admin pages and auth hook** — call sites that forget to unwrap `ApiResponse` via `.data` (or that wrongly unwrap raw data as if it were an `ApiResponse`).

This is a great intermediate-level contribution: real bugs, well-scoped, no infra needed, and the fix teaches the codebase's API client conventions.

## Background

Our `apiClient` returns a wrapped response shape:

```typescript
// apps/frontend/src/lib/api/api-client.ts
export interface ApiResponse {
success: boolean
data?: T
message?: string
error?: string
}
```

Every call site is supposed to check `response.success` first, then read `response.data`. About 19 sites get this wrong in two opposite ways.

## The Two Patterns

### Pattern A — Forgot to unwrap (admin pages, useAuthentik, compliance-automation/page.tsx)

```typescript
// BEFORE - wrong: response is ApiResponse<{users: User[]}>, not {users: User[]}
const response = await apiClient.get<{ users: User[] }>(API_ENDPOINTS.admin.users)
setUsers(response.users) // TS2339: Property 'users' does not exist on type 'ApiResponse<...>'

// AFTER
const response = await apiClient.get<{ users: User[] }>(API_ENDPOINTS.admin.users)
if (response.success && response.data) {
setUsers(response.data.users)
} else {
setError(response.error ?? 'Failed to load users')
}
```

### Pattern B — Treats raw data as if it were a wrapper (ScheduleManager, ViolationAlerts)

```typescript
// BEFORE - wrong: complianceAutomationService.listSchedules() returns raw data, not ApiResponse
const res = await complianceAutomationService.listSchedules()
if (res.success && res.data) { // TS2339: Property 'success' does not exist
setSchedules(res.data)
}

// AFTER (option 1: change consumer to match service contract)
const res = await complianceAutomationService.listSchedules()
setSchedules(res.schedules) // res is { schedules: ComplianceSchedule[]; total; offset; limit }

// AFTER (option 2: change service to use apiClient's ApiResponse wrapper for consistency)
// - see "Recommended approach" below
```

## Recommended Approach

**Unify on `apiClient` + `ApiResponse`.** The codebase's other services use `apiClient` directly and expose typed methods that return `ApiResponse`. The compliance-automation service is the odd one out:

1. Rewrite `apps/frontend/src/lib/api/compliance-automation-service.ts`:
- Replace `import { api } from '@/lib/api/api-client'` with `import { apiClient } from '@/lib/api/api-client'`.
- Export a single `complianceAutomationService` object with methods (`listSchedules`, `createSchedule`, `listViolations`, `acknowledgeViolation`, `listReports`, etc.) that all return `ApiResponse`.
- The current `scheduleAPI`/`violationAPI`/`reportAPI`/`remediationAPI` namespacing can be kept as a thin wrapper if you prefer, but consumers need a single import.
2. Update consumers (`ScheduleManager.tsx`, `ViolationAlerts.tsx`, `compliance-automation/page.tsx`) to use the new `ApiResponse`-shaped return - `if (res.success && res.data) { ... }`.
3. Fix the admin pages and `useAuthentik.ts` to properly unwrap `response.data` before reading inner properties.

## Files to Fix

| # | File | Line | Error |
|---|------|------|-------|
| 1 | `apps/frontend/src/lib/api/compliance-automation-service.ts` | 12 | TS2305: `'api'` not exported from api-client |
| 2 | `apps/frontend/src/components/compliance-automation/ScheduleManager.tsx` | 30 | TS2339: `success` does not exist on `{data: ComplianceSchedule[]}` |
| 3 | `apps/frontend/src/components/compliance-automation/ScheduleManager.tsx` | 45 | TS2322: `string` not assignable to `'daily' \| 'weekly' \| 'monthly'` (state needs literal type) |
| 4 | `apps/frontend/src/components/compliance-automation/ScheduleManager.tsx` | 49 | TS2339: `success` does not exist on `ComplianceSchedule` |
| 5 | `apps/frontend/src/components/compliance-automation/ViolationAlerts.tsx` | 22 | TS2339: `success`/`data` do not exist on raw violations payload |
| 6 | `apps/frontend/src/components/compliance-automation/ViolationAlerts.tsx` | 23 | TS2339: same |
| 7 | `apps/frontend/src/app/(dashboard)/compliance-automation/page.tsx` | 41 | TS2345: `ApiResponse` not assignable to `SystemStatus \| null` |
| 8 | `apps/frontend/src/app/(dashboard)/compliance-automation/page.tsx` | 223 | TS2339: `schedules` does not exist on `ApiResponse` |
| 9 | `apps/frontend/src/app/(dashboard)/compliance-automation/page.tsx` | 299 | TS2339: `violations` does not exist on `ApiResponse` |
| 10 | `apps/frontend/src/app/(dashboard)/compliance-automation/page.tsx` | 384 | TS2339: `reports` does not exist on `ApiResponse` |
| 11 | `apps/frontend/src/app/(dashboard)/admin/audit-log/page.tsx` | 56 | TS2339: `logs` does not exist on `ApiResponse<{logs: AuditLog[]}>` |
| 12 | `apps/frontend/src/app/(dashboard)/admin/registrations/page.tsx` | 46 | TS2339: `requests` does not exist on `ApiResponse<{requests: ...}>` |
| 13 | `apps/frontend/src/app/(dashboard)/admin/users/page.tsx` | 40 | TS2339: `users` does not exist on `ApiResponse<{users: User[]}>` |
| 14 | `apps/frontend/src/lib/api/hooks/useAuthentik.ts` | 181 | TS2339: `access_token` does not exist on `ApiResponse` |
| 15 | `apps/frontend/src/lib/api/hooks/useAuthentik.ts` | 181 | TS2339: `refresh_token` does not exist on `ApiResponse` |
| 16 | `apps/frontend/src/lib/api/hooks/useAuthentik.ts` | 181 | TS2339: `user` does not exist on `ApiResponse` |
| 17 | `apps/frontend/src/lib/api/hooks/useAuthentik.ts` | 194 | TS2345: `ApiResponse` not assignable to `AuthTokenResponse` |

## Acceptance Criteria

- [ ] `cd apps/frontend && npx tsc --noEmit` reports **zero** errors in any file listed above
- [ ] `compliance-automation-service.ts` uses `apiClient` (not the non-existent `api`)
- [ ] All admin pages handle `response.success === false` (show an error state, not crash)
- [ ] No `as any` / `as ApiResponse` casts used to silence errors
- [ ] No runtime behavior change in successful-path responses - only error handling and types
- [ ] If you change the compliance-automation service shape, update *both* `ScheduleManager.tsx` and `ViolationAlerts.tsx` to match

## How to Reproduce

```bash
git clone https://github.com/adhit-r/fairmind.git
cd fairmind/apps/frontend
npm install
npx tsc --noEmit
```

You should see 19 errors in the files listed above.

## How to Test

1. **Type check (the gate):** `npx tsc --noEmit` - must pass.
2. **Manual smoke (optional, needs a running backend):** Log in, navigate to `/dashboard/admin/users`, `/dashboard/admin/audit-log`, `/dashboard/admin/registrations`, and `/dashboard/compliance-automation`. Pages should render data when the backend responds, and show an error state (not a blank screen) when it doesn't.
3. If you can't run the backend, just the type check is acceptable for review.

## Estimated Effort

2-4 hours for someone comfortable with TypeScript generics and React state management. The compliance-automation rewrite is the largest piece (~1-2h); admin pages and useAuthentik are smaller mechanical fixes (~1h).

## Why This Is a Good Contribution

- **Real bugs, not busywork.** Several of these would cause runtime crashes today if anyone tried to use the affected pages.
- **Self-contained.** Frontend only, no backend changes required.
- **Teaches the codebase's conventions.** You'll understand our API client pattern after fixing this.
- **Maintainer support.** Comment on this issue with questions and a maintainer will respond within 48 hours.

## Help / Questions

Comment on this issue and a maintainer will respond. If you're new to the repo, check `CONTRIBUTING.md` first. Please claim the issue (comment "I'd like to take this") before starting so we don't have duplicate work.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.