WealthySmart/codebase-review/03-frontend.md

# Frontend Review — WealthySmart

> Hat: Senior Frontend Engineer (React 19 / TypeScript / Vite / Tailwind 4 / Hono / CopilotKit). Focus: data fetching, state, type safety, auth flow, accessibility, the Hono gateway.

## Executive Summary

WealthySmart's frontend is a well-structured React 19 + Vite + TypeScript app behind a Hono gateway server. The codebase shows good practices in component organization, Tailwind styling, and accessibility basics. The dominant weakness is the hand-rolled data-fetching layer: no request cancellation, no caching/deduplication, and inconsistent error/loading states — several pages swallow API errors into `console.error` and render empty UI. The CopilotKit integration is sophisticated but leans on heavy middleware workarounds (message dedup, snapshot reconciliation) that indicate upstream fragility and currently swallow their own errors. Auth uses httpOnly cookies (`ws_token`) over same-origin requests — a solid choice — but the 401 handling can redirect-loop and the auth probe treats network failure as logged-out.

## Strengths

- **Type safety** — comprehensive interfaces in `api.ts` for Transaction, RecurringItem, Budget types; only a couple of `any` escapes.
- **Component architecture** — clean decomposition into pages, hooks (`useBudget`), and contexts (Theme, Privacy, Auth).
- **Accessibility fundamentals** — focus rings, aria-labels, title attributes on interactive elements; privacy mode via `data-sensitive` masking; logical heading hierarchy.
- **Error messages where they exist** — `TransactionModal` handles 409 conflicts with a user-readable message.
- **Format helpers** — locale-aware CRC formatting (`formatAmount`, `formatCRC`) used consistently in most modules.
- **Hono gateway** — cleanly proxies FastAPI and hosts the CopilotKit runtime; `DeduplicateToolCallMiddleware` / `ReconcileSnapshotMiddleware` address known CopilotKit issues head-on.
- **Responsive baseline** — mobile menu, adaptive layouts; tables are the weak spot (see FE/UX findings).

## Findings

### [FE-01] No Request Cancellation in `useBudget` — High
**Location:** `src/hooks/useBudget.ts:52–59`
**Evidence:**
```typescript
useEffect(() => {
  fetchProjection();
  fetchRecurringItems();
}, [fetchProjection, fetchRecurringItems]);
```
**Why it matters:** Changing `year`/`selectedMonth` while a request is in flight lets the stale response overwrite newer state. No AbortController or request-ID tracking anywhere in the app.
**Recommendation:** Abort on dependency change (`return () => ctrl.abort()`), or adopt TanStack Query which solves this class wholesale (see FE-10).

### [FE-02] Analytics Swallows Errors, No Loading State — Medium
**Location:** `src/pages/Analytics.tsx:77–95`
**Evidence:** `Promise.all([...]).then(...).catch(console.error)` — failures leave silently empty charts; no skeleton during fetch.
**Recommendation:** `loading` / `error` / retry states with visible UI.

### [FE-03] Transaction Fetch Races in Budget Page — High
**Location:** `src/pages/Budget.tsx:44–77`
**Evidence:** `fetchTransactions` re-fires on `txSearch`/`txSource`/`selectedMonth` changes with no cancellation; last-to-resolve wins even if stale.
**Recommendation:** AbortController per request keyed to the effect; or query library.

### [FE-04] Unguarded Array Indexing in Pensions ROI Fallback — Medium
**Location:** `src/pages/Pensions.tsx:292–315`
**Evidence:** Fallback computes `chartData[len - 1][key] - chartData[len - 2][key] - ...`; with `len < 2` this silently yields 0/negative garbage.
**Recommendation:** Guard `if (len < 2) { acc[key] = 0; }` before indexing.

### [FE-05] Post-Upload Refetch Can Hang the Upload UI — Medium
**Location:** `src/pages/Pensions.tsx:343–365`
**Evidence:** `await loadData()` after upload has no timeout; "Procesando…" persists indefinitely if the backend stalls.
**Recommendation:** Timeout the refetch and surface an actionable message.

### [FE-06] `catch (err: any)` in TransactionModal — Medium
**Location:** `src/components/TransactionModal.tsx:97`
**Evidence:** `err.response?.status === 409` assumes a shape; non-HTTP errors (network) silently fall through to `console.error` with no user message.
**Recommendation:** Type-guard with the app's `ApiError`; always set a user-visible error in the else branch.

### [FE-07] localStorage Read in Effect Instead of Lazy Initializer — Low
**Location:** `src/contexts/theme-context.tsx:16–24`, `src/contexts/privacy-context.tsx:13–15`
**Evidence:** Initial theme read happens in `useEffect`, causing a flash of wrong theme on mount.
**Recommendation:** `useState(() => localStorage.getItem("theme") ?? ...)` lazy initializer.

### [FE-08] No Security Headers from the Hono Gateway — Low
**Location:** `frontend/server.ts:379–416`
**Evidence:** No CSP, `X-Frame-Options`, `X-Content-Type-Options`, or `Referrer-Policy` on served responses.
**Recommendation:** One small Hono middleware (pairs with backend SEC-07; the user-facing HTML is served from here, so this is the layer that matters most for CSP/frame protection).

### [FE-09] Coupled Effects in `useBudget` — Medium
**Location:** `src/hooks/useBudget.ts:27–35`
**Evidence:** Projection and recurring-items fetches share one effect; failure modes and re-run triggers are entangled.
**Recommendation:** One effect per concern with explicit dependencies.

### [FE-10] No Query Caching or Deduplication — High
**Location:** `src/lib/api.ts:15–66` and all consumers
**Evidence:** Every `api.get()` is a fresh fetch; parallel consumers of the same endpoint each hit the network; every page re-fetches everything on mount.
**Why it matters:** This is the root cause behind FE-01/02/03/09/24 and ARCH-13. One architectural decision fixes the whole class.
**Recommendation:** Adopt **TanStack Query**: per-key caching, dedup, cancellation, retries, `isLoading`/`isError` states, and mutation invalidation. Migrate page by page (Budget first).

### [FE-11] CopilotKit Middleware Swallows Parse Errors — Medium
**Location:** `frontend/server.ts:350–368`
**Evidence:** `catch { return; }` in `beforeRequestMiddleware` — malformed bodies silently skip orphan-tool-call pairing, producing confusing downstream runtime failures.
**Recommendation:** `console.error` with context before returning; given the documented history of card-persistence bugs (see `reference_agui_client` memory), observability here pays for itself.

### [FE-12] Month Detail Rendered Without Existence Check — Medium
**Location:** `src/pages/Budget.tsx:115–154`
**Evidence:** Month navigation renders detail UI without checking `monthDetail` exists for the selection.
**Recommendation:** `monthDetail ? <MonthlyDetail/> : loading ? <Skeleton/> : <EmptyState/>`.

### [FE-13] XSS Posture Is Fine — Watch `dangerouslySetInnerHTML` — Informational
**Location:** `src/components/TransactionList.tsx:129`, `src/components/chat/ChatCards.tsx:128`
**Evidence:** Merchant/category strings render through JSX (auto-escaped). No `dangerouslySetInnerHTML` found. Not a current bug — recorded as a guardrail since this data originates from emails (attacker-influenceable).

### [FE-14] No Runtime Validation of API Responses — Medium
**Location:** `src/lib/api.ts` and all consumers
**Evidence:** Response types are compile-time assertions only; backend schema drift produces `undefined` rendering or crashes far from the cause.
**Recommendation:** Either Zod-parse at the API-client boundary for the few critical shapes, or (better long-term) generate types from FastAPI's OpenAPI schema once ARCH-16 lands (`openapi-typescript`).

### [FE-15] Auth Probe Treats Network Failure as Logged Out — Medium
**Location:** `src/AuthContext.tsx:22–29`
**Evidence:** `.catch(() => setAuthenticated(false))` — a transient network blip on app load kicks an authenticated user to the login screen.
**Recommendation:** Distinguish network errors (retry once, or keep an "unknown" state) from a real 401.

### [FE-16] Missing `aria-pressed` on Toggle Buttons — Low
**Location:** `src/pages/Pensions.tsx:515–530` (fund visibility toggles)
**Evidence:** Toggles convey state visually only.
**Recommendation:** `aria-pressed={visibleFunds.has(key)}`; audit other toggles (privacy mode, theme).

### [FE-17] `en-US` Dates in an es-CR App — Low
**Location:** `src/pages/Analytics.tsx:238–240` (chart `labelFormatter`), also `src/lib/format.ts:19–21`
**Evidence:** Chart tooltips show "Jan", "Feb" while the page is in Spanish.
**Recommendation:** Centralize on `es-CR` (see UX-19 for the full localization sweep).

### [FE-18] `useBudget` Return Object Not Memoized — Low
**Location:** `src/hooks/useBudget.ts:86–103`
**Evidence:** Fresh object identity every render; consumers using it as a dependency re-run unnecessarily.
**Recommendation:** Wrap in `useMemo` (moot if migrated to TanStack Query).

### [FE-19] Unbounded Upload-Result List — Low
**Location:** `src/pages/Pensions.tsx:822–865`
**Evidence:** All returned snapshots render without scroll containment or truncation.
**Recommendation:** Slice to ~10 with "and N more".

### [FE-20] Dead Code: `AgentHomeClient.tsx` — Low
**Location:** `src/components/AgentHomeClient.tsx` vs `src/pages/Asistente.tsx`
**Evidence:** Two near-identical components; only `Asistente.tsx` is routed.
**Recommendation:** Delete `AgentHomeClient.tsx`.

### [FE-21] (Withdrawn) — the shadcn re-export concern was speculative; no concrete evidence found. Dropped.

### [FE-22] 401 Handler Can Redirect-Loop — Medium
**Location:** `src/lib/api.ts:48–52`
**Evidence:** On 401: call logout, `window.location.replace("/login")`. If any call from the login page itself 401s, this loops.
**Recommendation:** Skip the redirect when already on `/login`.

### [FE-23] Proxy Has No Error Handling for Backend Failures — Medium
**Location:** `frontend/server.ts:385–402`
**Evidence:** `proxyToBackend` returns `fetch(target, init)` with no try/catch; a connection refusal becomes an opaque 500.
**Recommendation:** try/catch → log → `c.json({ error: "Backend unavailable" }, 502)`.

### [FE-24] No Timeouts on Any Fetch — Medium
**Location:** All pages
**Evidence:** No `AbortSignal.timeout()` anywhere; hung backends mean indefinite spinners.
**Recommendation:** Default `AbortSignal.timeout(30_000)` in the shared `request()` helper in `api.ts` — one change covers the app.

## Quick Wins (under 1 hour each)

1. `es-CR` locale in Analytics chart labels — FE-17
2. Delete `AgentHomeClient.tsx` — FE-20
3. Security-headers middleware in `server.ts` — FE-08
4. Guard `/login` in the 401 redirect — FE-22
5. try/catch + 502 in `proxyToBackend` — FE-23
6. `AbortSignal.timeout(30000)` in the shared request helper — FE-24
7. Guard `chartData.length >= 2` in Pensions ROI — FE-04
8. `aria-pressed` on fund toggles — FE-16
9. Log CopilotKit middleware parse failures — FE-11
10. Lazy initializers for theme/privacy contexts — FE-07

**Strategic recommendation:** Most Medium/High findings here share one root cause — the absence of a query layer. Adopting TanStack Query (Plan Phase 3) retires FE-01, FE-03, FE-09, FE-10, FE-18, FE-24 and ARCH-13 in one move, and makes FE-02-style error states the default rather than per-page work.