af5535dfcc
Persiste proposal, specs delta (auth, api-client, access-requests, platform-shell, testing), design con 8 ADRs, tasks (27 items en 10 fases), verify-report (28 tests, 0 críticos, 0 warnings tras el cierre) y archive-report final. Cierra el ciclo SDD del cleanup de deuda técnica del console.
442 lines
27 KiB
Markdown
442 lines
27 KiB
Markdown
# 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.
|