# Design — `console-tech-debt-cleanup`

> **Phase**: design (HOW at architectural level)
> **Project**: omnioilpersonal
> **Surface**: `services/frontend-console`
> **Approach**: targeted refactor over the existing layered structure (`features/<domain>/{api,pages,stores}` + `lib/api` + `app/{layouts,router}`). No new architecture, no new layers — eliminate duplications, tighten contracts, install the safety net.

---

## 0. Architecture context (recap)

`frontend-console` already follows the standard React + Vite + Zustand + React Query layout:

```
src/
  app/
    layouts/PlatformLayout.tsx   <- shell (sidebar + Outlet + Toaster)
    router/{index.tsx, ProtectedRoute.tsx}
  features/
    auth/        (stores, api, schemas, pages)
    access-requests/  (api, pages)
  lib/
    api/client.ts       <- apiClient + SessionCoordinator (the choke point)
    utils/cn.ts
  components/ui/...
```

The runtime contract that `apiClient` talks to (Rust backend in `services/backend-api`) is the source of truth. We do NOT change the contract; we change how the frontend models it.

Backend login response shape (verified): `services/backend-api/src/handlers/auth.rs:232-242` returns

```json
{
  "access_token": "...",
  "token": "...",          // legacy duplicate, comment "Compatibilidad con clientes legacy"
  "role": "...",
  "email": "...",
  "projects": [...]
}
```

Same dual emission for refresh at `auth.rs:500-509`. **The backend will keep emitting both `access_token` and `token` for the foreseeable future** — that is a backend decision, not ours. Our job here is to stop the frontend from caring about `token`.

---

## 1. ADR-001 — Single auth token field: `accessToken`

**Decision**: drop the legacy `token` field from `useAuthStore` state. `accessToken` becomes the single source of truth. Components, route guards, and effects read `accessToken` only.

**Rationale**:
- Today `auth-store.ts:32,39,44` stores both `accessToken` AND `token` (mirrored). Consumers split: `ProtectedRoute.tsx:9-29` and `LoginPage.tsx:16,24,29` read `state.token`, while `client.ts:31` reads `state.accessToken`. Two fields, two readers, same value — pure duplication that drifted because nobody noticed they were equal.
- The backend already keeps the dual emission for legacy clients. We do not need a second field on the frontend to "track legacy"; the fallback lives on the wire.
- Frontend-admin (the reference app) already uses only `accessToken` for live state. Confirmed at `services/frontend-admin/src/lib/api/client.test.ts:124,146` — it asserts `state.accessToken === 'fresh-token'` and that on logout `accessToken: null, refreshToken: null, token: null`. Note: frontend-admin still keeps `token: null` in the store as a vestigial slot. We will go further and **remove the slot entirely** since console has no such legacy consumers outside the two files we control.

**Migration plan** (execution belongs to tasks, not design):
1. **Audit step before any rename** — run a regex sweep:
   - `useAuthStore.*\.token\b` and `state\.token` for selector reads
   - `\['token'\]` and `\["token"\]` for bracket-string reads (THE landmine)
   - `token:` inside object literals destructured from the store
2. Replace selectors: `state.token` → `state.accessToken`. Update dependency arrays accordingly.
3. Remove the `token` slot from `AuthState`, the `set({ token: ... })` mirror writes, and the initial `token: null`.
4. TypeScript will catch every typed read site at build. Bracket-string reads are NOT caught by TS — that's why step 1 is mandatory before rename.

**Rejected alternatives**:
- *Keep both fields, document `accessToken` as canonical*: doesn't fix the bug, just adds a comment. Future refactors will keep splitting.
- *Rename to `token` instead of `accessToken`*: contradicts the backend field name and the rest of the codebase (admin uses `accessToken`).

**Risk**: A bracket-string read like `useAuthStore.getState()['token']` would silently start returning `undefined`. Mitigation = step 1 audit. Acceptance criterion = grep returns zero matches before merge.

---

## 2. ADR-002 — `LoginResponse` type matches the runtime exactly

**Decision**: redefine `LoginResponse` in `src/features/auth/api/auth-api.ts` as

```ts
export interface LoginResponse {
  access_token: string         // required — backend always sends it
  refresh_token?: string       // optional in JSON body (backend prefers HttpOnly cookie)
  role: string
  email: string
  projects?: Array<{ id: string; name: string }>  // present in admin runtime; opaque to console
}
```

No `token`. No `??` fallback in `LoginPage.onSubmit`.

**Runtime contract source**: `services/backend-api/src/handlers/auth.rs:232-242` (login) and `:500-509` (refresh). The fields confirmed present: `access_token`, `token` (legacy mirror — we choose to ignore it), `role`, `email`, `projects`. `refresh_token` is NOT in the JSON body for login (it goes into the `Set-Cookie` header at line 223-225); it IS present in some refresh paths. Marking it optional in our type is honest.

**Rationale**:
- Today `auth-api.ts:4-9` declares `token: string` REQUIRED and `access_token?` OPTIONAL — the exact opposite of the runtime. The current code only works because `LoginPage.tsx:45` does `response.access_token ?? response.token`, papering over the lying type.
- A type that lies about the runtime is worse than no type. It actively misleads autocomplete and code review.

**Where verified**: `services/backend-api/src/handlers/auth.rs:232-242`. Documenting this in the spec so the next refactor doesn't have to re-derive it.

**Rejected alternatives**:
- *Keep `token` required for backward compat with the legacy emission*: we already chose not to consume the legacy field (ADR-001). Keeping it in the type just to "match the wire" leaks legacy concerns into TS.
- *Make everything optional*: then `LoginPage` must handle every undefined case at runtime. That's defensive coding against our own backend.

---

## 3. ADR-003 — Split `apiClient`: `apiClient<T>` + `apiClientVoid`

**Decision**: option **(a)** from the proposal. Two named exports:

```ts
export async function apiClient<T>(endpoint: string, options?: FetchOptions): Promise<T>
export async function apiClientVoid(endpoint: string, options?: FetchOptions): Promise<void>
```

`apiClient<T>` throws if the response is 204 (no body to parse — caller asked for a body). `apiClientVoid` returns `void` and ignores the body for 2xx, including 204.

Both share an internal `executeWithRefresh(endpoint, options): Promise<Response>` that owns the 401 → refresh → retry flow currently embedded in `apiClient`. The two public functions are thin wrappers that decide what to do with the final `Response`.

**Rationale**:
- Today `client.ts:96,111` returns `null as T` for 204. Every caller that types `<{ ok: boolean }>` gets `null` at runtime, which the type system happily lies about. The "fix" in the test (`client.test.ts:144` — `.rejects.toThrow`) only catches the rejection path, not the success-with-204 path.
- Option (a) makes the void-vs-body decision explicit at the **call site**, where the caller already knows whether the endpoint returns a body.
- Mechanical migration: every existing call returns a body today (login, listAccessRequests, approve/reject/review). Zero call sites need to switch to `apiClientVoid` in this change. We add the function for **future** mutation endpoints that return 204.

**Rejected alternatives**:
- *(b) Generic with overload `apiClient<T | void>`*: TypeScript overloads on generics are fragile and confuse autocomplete. Caller still has to remember which overload they invoked.
- *(c) Always return `T | null` and let callers narrow*: every caller writes `if (data === null) ...` for endpoints that NEVER return null. That's the worst-of-both-worlds — keeps the lie AND adds boilerplate.
- *Throw on 204 inside `apiClient<T>` only, no second function*: works but leaves no migration path for legitimate void endpoints; first call site that needs 204 will reintroduce the `null as T` cast.

**Internal shape** (illustrative — implementation lives in tasks/apply):

```ts
async function executeWithRefresh(endpoint, options): Promise<Response> {
  const { response } = await executeRequest(endpoint, options)
  if (response.status === 401 && !options._retry && !isBypassedEndpoint(endpoint)) {
    const refreshed = await SessionCoordinator.refresh()
    if (!refreshed) {
      useAuthStore.getState().logout()
      redirectToLogin()
      throw new Error('Sesion expirada')
    }
    const { response: retried } = await executeRequest(endpoint, { ...options, token: refreshed, _retry: true })
    if (!retried.ok) {
      // same fail-open behavior as today
      const err = await retried.json().catch(() => ({ error: 'Request failed' }))
      throw new Error(err.error ?? `HTTP ${retried.status}`)
    }
    return retried
  }
  if (!response.ok) {
    const err = await response.json().catch(() => ({ error: 'Request failed' }))
    throw new Error(err.error ?? err.message ?? `HTTP ${response.status}`)
  }
  return response
}

export async function apiClient<T>(endpoint, options = {}): Promise<T> {
  const r = await executeWithRefresh(endpoint, options)
  if (r.status === 204) throw new Error(`apiClient: endpoint ${endpoint} returned 204; use apiClientVoid`)
  return r.json() as Promise<T>
}

export async function apiClientVoid(endpoint, options = {}): Promise<void> {
  await executeWithRefresh(endpoint, options)
}
```

The 204-throws-in-`apiClient` is a **dev-time guard**. No production endpoint we currently call returns 204; if one ever does, it's a contract violation we want to see loudly, not silently coerce to null.

---

## 4. ADR-004 — `ProtectedRoute` first-load: persisted `user` as the heuristic

**Decision**: drive the "should we attempt a refresh on cold load?" decision off the persisted `user` slot. Logic:

```
on mount:
  if accessToken exists in memory: render <Outlet/> (or redirect on bad role) — done
  else if persisted user exists (rehydrated from localStorage): show "Restaurando sesion" + call refresh()
  else: render <Navigate to="/login"/> immediately — no refresh attempt, no flash
```

Today `ProtectedRoute.tsx:9-34` ALWAYS attempts a refresh when there's no token, regardless of whether the user ever logged in. That produces the "Restaurando sesion..." flash for first-time visitors.

**Rationale**:
- The persist allowlist (`auth-store.ts:54`) already saves `user` to localStorage. If `user` rehydrated → there was a prior session → refresh might restore it (cookie still valid). If `user` did not rehydrate → cold visitor → no point hitting `/auth/refresh`.
- Zero changes to `partialize` — the heuristic uses what's already persisted.
- Eliminates the spurious refresh request AND the cosmetic flash in one move.

**Implementation sketch**:

```tsx
const accessToken = useAuthStore(s => s.accessToken)
const user        = useAuthStore(s => s.user)
const hasHydrated = useAuthStore.persist.hasHydrated()
// 1. wait for hydration before deciding
const [phase, setPhase] = useState<'pending'|'restoring'|'ready'>(hasHydrated ? 'ready' : 'pending')

useEffect(() => {
  if (!hasHydrated) return
  if (accessToken) { setPhase('ready'); return }
  if (!user)       { setPhase('ready'); return }   // cold visitor — no refresh
  setPhase('restoring')
  let cancelled = false
  SessionCoordinator.refresh().finally(() => { if (!cancelled) setPhase('ready') })
  return () => { cancelled = true }
}, [hasHydrated, accessToken, user])

if (phase !== 'ready') return <RestoringSession />
if (!accessToken || !user)            return <Navigate to="/login" replace />
if (!PLATFORM_ROLES.has(user.role))   return <Navigate to="/login" replace />
return <Outlet />
```

(Actual code goes in tasks; this sketch is the contract design enforces.)

**Rejected alternatives**:
- *Persist `accessToken` in localStorage*: leaks JWT to XSS. The whole point of the cookie-based refresh is that the access token lives in memory only.
- *Skip refresh attempt entirely on every cold load*: breaks the legitimate "I closed the tab, came back, refresh cookie still valid" path.
- *Add a `hasEverLoggedIn` boolean to the store*: redundant — `user` already serves that role.

---

## 5. ADR-005 — `ErrorBoundary` placement: inside `PlatformLayout`, around `<Outlet />`

**Decision**: a class-based error boundary lives inside `PlatformLayout.tsx`, wrapping ONLY the `<Outlet />`. Sidebar, logout button, and Toaster stay outside the boundary so they survive a page-level crash.

**Rationale**:
- A render error in `AccessRequestsPage` (or any future child route) should NOT take down the shell. The user must still be able to navigate elsewhere or sign out.
- Wrapping at the `RouterProvider` level would crash the whole app — same regression as today.
- The login route is a single form with no children; React Router's natural error handling is enough. No boundary on `/login`.

**Library choice**: native React class component. No dependency added. Frontend-console's only state libs are React Query + Zustand — both have their own error surfaces (`isError`, error boundaries via `ErrorBoundary` is the standard React pattern). Adding `react-error-boundary` would be one more devdep for a 30-line class.

**Shape**:

```tsx
class ShellErrorBoundary extends React.Component<{children: React.ReactNode}, {error: Error|null}> {
  state = { error: null as Error | null }
  static getDerivedStateFromError(error: Error) { return { error } }
  componentDidCatch(error: Error, info: React.ErrorInfo) {
    // log to console for now — we don't have Sentry/observability in console yet
    console.error('[ShellErrorBoundary]', error, info)
  }
  render() {
    if (this.state.error) {
      return <FallbackPanel error={this.state.error} onReset={() => this.setState({ error: null })} />
    }
    return this.props.children
  }
}
```

The fallback shows the error message + a "Reintentar" button that resets the boundary state. This forces a remount of the route subtree.

**Rejected alternatives**:
- *Boundary at `RouterProvider` level*: kills the shell — rejected.
- *Per-route boundary*: more flexible but premature. Console has 2 routes today.
- *Add `react-error-boundary`*: extra dep for negligible ergonomic gain at this surface size.

---

## 6. ADR-006 — `AccessRequestsPage` error UX: inline card + soft-fail stat cards

**Decision**:
- When `requestsQuery.isError` is true, replace the table region (the `<Card className="overflow-hidden">` in `AccessRequestsPage.tsx:176-213`) with an inline error card containing the error message and a "Reintentar" button that calls `requestsQuery.refetch()`.
- When `allRequestsQuery.isError` is true, the four `StatCard` components show `0` (using `?? 0` on `getStatusCount`). Stats failing must NOT block the table.
- The detail panel (`RequestDetail`) keeps rendering its empty state when `selected` is null — no change.
- Loading state stays as today (centered spinner). Error state replaces what would otherwise be a silently-empty table.

**Rationale**:
- Today both queries fail invisibly (`AccessRequestsPage.tsx:71-72`: `requestsQuery.data ?? []`). The user sees an empty table and no signal that the network failed. That's a correctness bug for a page that approves/rejects platform access.
- Stats and the table are independent fetches with independent failure modes. Coupling them (e.g., refusing to render the table because stats failed) is the wrong UX.
- `refetch` on the inline button is the canonical React Query recovery path. No need to invent retry logic.

**Shape** (illustrative):

```tsx
{requestsQuery.isLoading ? (
  <Spinner />
) : requestsQuery.isError ? (
  <Card>
    <CardContent className="space-y-3 py-10 text-center">
      <p className="text-sm text-red-300">{(requestsQuery.error as Error).message}</p>
      <Button variant="secondary" onClick={() => requestsQuery.refetch()}>Reintentar</Button>
    </CardContent>
  </Card>
) : (
  <TableAndDetail ... />
)}
```

**Rejected alternatives**:
- *Toast on every fetch error*: users lose the recovery affordance the moment the toast dismisses. Also noisy.
- *Use the global ErrorBoundary*: the boundary is for render errors, not for query failures. Mixing them blurs the contract — users would see "something went wrong" for a transient network blip.
- *Block the page on stat failure*: stats are a side panel. Failing them should not break the primary workflow.

---

## 7. ADR-007 — Vitest scaffolding: mirror frontend-admin's inline `vite.config.ts` test block

**Decision**: do NOT add a separate `vitest.config.ts`. Mirror the frontend-admin pattern, which puts the `test:` block inside `vite.config.ts` (verified at `services/frontend-admin/vite.config.ts:28-33`). Add the test setup file at `src/test/setup.ts` (mirror of admin's `services/frontend-admin/src/test/setup.ts`).

**Final config additions** (delta against current state):

`services/frontend-console/vite.config.ts` — add test block:
```ts
test: {
  environment: 'jsdom',
  setupFiles: './src/test/setup.ts',
  restoreMocks: true,
  clearMocks: true,
},
```

`services/frontend-console/src/test/setup.ts` — verbatim copy of admin's setup. Stubs `localStorage`/`sessionStorage`, sets `IS_REACT_ACT_ENVIRONMENT`, clears storage `beforeEach`, unstubs globals `afterEach`.

`services/frontend-console/package.json` — add:
- `"test": "vitest run"` to scripts
- devDeps: `vitest@^3.2.4`, `jsdom@^26.1.0` (matching admin versions to avoid two majors in one repo)
- The proposal also mentions `@testing-library/react`, `@testing-library/user-event`, `@vitest/coverage-v8`. Frontend-admin does NOT have these — its tests are pure logic + RTL-free. **Recommendation: only add what we will actually use in this change**. The test priorities (next ADR) cover store + coordinator + apiClient + a page. The page test (`AccessRequestsPage`) needs RTL. Coverage is nice-to-have, not required to land tests.
  - **Add now**: `@testing-library/react@^16`, `@testing-library/dom@^10`, `@testing-library/user-event@^14`
  - **Defer**: `@vitest/coverage-v8` until someone asks for coverage gating
  - **Defer**: `msw` — for the page test we mock the api module directly (`vi.mock('@/features/access-requests/api/access-requests-api')`), simpler than network interception

**Rejected alternatives**:
- *Separate `vitest.config.ts`*: divergent from admin without a reason. Two configs to maintain.
- *Add `msw` upfront*: heavier setup; the api modules in console are thin enough that module-level mocking is sufficient and faster.
- *Add coverage from day one*: coverage is meaningless until there's a baseline of tests AND a target threshold. Premature.

**Diff snapshot for tasks phase** (so the breakdown is mechanical):

```
M services/frontend-console/package.json     (scripts.test, devDependencies)
M services/frontend-console/vite.config.ts   (+ test block)
+ services/frontend-console/src/test/setup.ts
+ services/frontend-console/src/features/auth/stores/auth-store.test.ts
+ services/frontend-console/src/lib/api/client.test.ts
+ services/frontend-console/src/features/access-requests/pages/AccessRequestsPage.test.tsx
```

---

## 8. ADR-008 — Test ordering (TDD-first sequencing)

**Decision**: tests land BEFORE the corresponding refactor, in this exact order:

1. **`auth-store.test.ts`** — `setSession` writes `accessToken`, `refreshToken`, `user`; `logout` clears all of them; `isAuthenticated()` reflects state. **Pure unit, no I/O, no jsdom features beyond what setup.ts provides.** This test must PASS against the current store (with the legacy `token` field still present). After ADR-001 lands, the test still passes — we only assert on `accessToken`, never on `token`.
2. **`SessionCoordinator.refresh` dedupe** — race test using mocked `fetch`: two concurrent `refresh()` calls, assert one network call, both promises resolve to the same token. Mirror admin's `client.test.ts:44-86`. Must PASS against current code (the dedupe already works — this test pins the behavior so the apiClient split doesn't break it).
3. **`apiClient` 401 retry path** — mock `fetch` to return 401, then 200 on the refresh, then 200 on the retry. Assert headers, the `Authorization: Bearer fresh-token` on retry, and final body. Mirror admin's `client.test.ts:88-126`. Also assert: `apiClient<{ok:boolean}>` on a 204 throws (this is the new contract from ADR-003 — test goes red until ADR-003 ships).
4. **`AccessRequestsPage.test.tsx`** — happy path render with mocked api module: status filter changes, refetch button calls both queries, approve/reject mutations show success toast, **AND** the new error UI from ADR-006 renders when the query mock rejects. Some assertions go red until ADR-006 ships.

**Rationale**:
- Tests 1-2 pin existing correct behavior FIRST. They protect against accidental regressions during the rename in ADR-001.
- Tests 3-4 contain assertions that intentionally fail today and pass once the refactor lands. That is the TDD signal. Strict TDD mode is active for this project — this is non-negotiable.
- This ordering means the rename in ADR-001 has a green safety net before it runs, exactly as the proposal demands.

**Rejected alternatives**:
- *All tests after the refactor*: defeats the point of strict TDD. Also leaves the rename uncovered during the highest-risk window.
- *Page test first*: too much surface; would require the apiClient split AND the error UI AND mocks all at once. Bottom-up is faster.

---

## 9. Component & data-flow map (post-change)

```
┌─────────────────────────────────────────────────────────────────┐
│                      RouterProvider (root)                      │
│  ┌───────────────────────────┐    ┌──────────────────────────┐  │
│  │     /login → LoginPage    │    │  /app/platform/* tree    │  │
│  │  reads:  accessToken,user │    │  ProtectedRoute (gate)   │  │
│  │  writes: setSession()     │    │  reads: accessToken,user │  │
│  │                           │    │         persist.hasHyd.. │  │
│  └───────────────────────────┘    │  decides: redirect /     │  │
│                                   │           restoring /    │  │
│                                   │           render Outlet  │  │
│                                   │  ┌────────────────────┐  │  │
│                                   │  │   PlatformLayout   │  │  │
│                                   │  │  sidebar + logout  │  │  │
│                                   │  │  ┌──────────────┐  │  │  │
│                                   │  │  │ ErrorBoundary │ │  │  │
│                                   │  │  │  <Outlet/>    │ │  │  │
│                                   │  │  └──────────────┘  │  │  │
│                                   │  └────────────────────┘  │  │
│                                   └──────────────────────────┘  │
└─────────────────────────────────────────────────────────────────┘

         useAuthStore (Zustand, persist:{ user })
            ▲          ▲          ▲           ▲
            │          │          │           │
       ProtectedR.  LoginPage  PlatformL.  apiClient
                                            (reads accessToken)
                                              │
                                              ▼
                              ┌──────────────────────────┐
                              │  apiClient<T> | apiClientVoid  
                              │   ↓ executeWithRefresh    │
                              │   ↓ 401 → SessionCoord.   │
                              │            ↓ /auth/refresh│
                              └──────────────────────────┘
                                              │
                                              ▼
                                  backend-api (Rust)
```

**Integration points** (unchanged contracts):
- `POST /auth/login` → returns `{access_token, role, email, projects, refresh_token?}` (cookie also set)
- `POST /auth/refresh` → returns `{access_token, role, email}` (rotated cookie)
- `GET /platform/access-requests` → `AccessRequest[]`
- `POST /platform/access-requests/:id/{review,approve,reject}` → `AccessRequest`

None of these contracts change in this refactor.

---

## 10. Architectural risks & assumptions

| ID | Risk / assumption | Severity | Mitigation |
|----|-------------------|----------|------------|
| R1 | Bracket-string read of `state['token']` exists somewhere TS can't see | medium | Mandatory regex audit (ADR-001 step 1) before rename; CI grep guard optional |
| R2 | Backend stops sending `access_token` and only sends `token` | low | Backend code (`auth.rs:232-236`) sends both today and the comment marks `token` as the legacy one; type contract aligned with what's primary |
| R3 | `ProtectedRoute` heuristic mis-detects "cold visitor" if `user` got persisted but cleared via direct localStorage manipulation | low | Acceptable — reduces to current behavior (refresh attempt) |
| R4 | `apiClient<T>` 204 throw breaks an existing call we missed | low | Grep: every existing call site expects a body. Add a smoke pass during apply |
| R5 | RTL test for `AccessRequestsPage` is flaky on async mutations | medium | Use `findBy*` queries + `waitFor`; mock api at the module boundary, not the network |
| R6 | Vitest version drift between admin and console | low | Pin both to `^3.2.4` (admin's version) |
| R7 | ErrorBoundary swallows real bugs in dev | low | `componentDidCatch` logs to console; React's default red overlay still surfaces in dev mode |

**Unresolved decisions**: none. All eight items have a concrete decision above.

**Assumptions requiring validation in the apply phase**:
- A1: No bracket-string `'token'` reads exist in the console source tree (validate via grep before rename).
- A2: No current caller of `apiClient<T>` hits a 204 endpoint (validate by inspecting all four call sites: `loginApi`, `listAccessRequests`, `markAccessRequestInReview`, `approveAccessRequest`, `rejectAccessRequest`, `SessionCoordinator.performRefresh`).

---

## 11. Out of scope (explicit non-goals for this design)

- Refresh token rotation rework
- Migrating frontend-admin or landing apps
- E2E tests
- Coverage gating
- Observability (Sentry, etc.) wiring into the ErrorBoundary
- Removing the legacy `token` emission from the backend
- Project/tenant selection state (admin has it; console does not need it yet)

These belong in future changes if/when the need is real.