ai_synth/docs/audit/frontend-report.md

# Frontend Audit Report (SolidJS)

**Date:** 2026-03-26
**Scope:** `frontend/src/` -- SolidJS + Tailwind CSS v4 application
**Files reviewed:** 30+ files across pages, components, API modules, utils, i18n, and types

---

## Executive Summary

The frontend is well-structured for a learning project of this scope. It demonstrates solid understanding of SolidJS fundamentals: lazy loading, proper `Show`/`For` usage, context-based auth, typed i18n, and a clean API client layer. The codebase is consistent in style and convention.

That said, several structural issues deserve attention. The Settings page has grown to 1026 lines and mixes concerns that should be decomposed. Some `setTimeout` calls lack `onCleanup` guards, creating potential memory leaks. A handful of SolidJS-specific anti-patterns (destructured reactive props, `children: any`) reduce type safety and could cause subtle bugs. The i18n system, while type-safe at the key level, has an XSS surface via `innerHTML` usage in one location. None of these are critical in a single-tenant self-hosted deployment, but they represent the most impactful areas for improvement.

**Overall quality: Good.** The top priorities are decomposing the Settings page, adding missing `onCleanup` guards, and fixing the `innerHTML` usage.

---

## SolidJS Patterns

### Reactive Primitives

**Signals and derived state -- well done.** The AuthContext correctly models derived signals (`isAuthenticated`, `isAdmin`) as plain functions over `user()`, avoiding redundant signals. The `createResource` usage in ApiKeyManager and Settings for provider/key data is appropriate.

**`createEffect` usage -- mostly correct, one concern.** Effects in GenerateSynthesis for auto-redirect and error handling are clean. However, the effect in Settings.tsx (line 81-93) that checks provider availability reads both `providers()` and `settings()`, which means it will re-fire whenever either signal changes -- including when the user changes an unrelated setting field. This is harmless (idempotent) but wasteful. Using `on()` to track only `providers` would be more precise:

```ts
// Current: re-fires on any settings change
createEffect(() => {
  const providerList = providers();
  const currentSettings = settings();  // unintended dependency
  ...
});

// Better: only fires when providers load
createEffect(on(() => providers(), (providerList) => { ... }));
```

**`createResource` vs `onMount` inconsistency.** Some data fetching uses `createResource` (ApiKeyManager, Settings providers), while most pages use `onMount` + manual signal (Home, Sources, LlmLogs, GenerateSynthesis). This is not wrong, but `createResource` would provide free loading/error states and refetch capabilities. The inconsistency suggests no team-wide convention was established.

### Component Composition

**`Show` and `For` -- correctly used throughout.** The callback form of `Show` (e.g., `<Show when={user()}>{(currentUser) => ...}`) is used in Navbar and MobileMenu, which is the correct SolidJS pattern for narrowing nullable values. `For` is used for all list rendering, never `.map()`.

**Step status rendering in GenerateSynthesis -- could use `Switch/Match`.** Lines 335-357 use three consecutive `<Show>` blocks for the three possible step statuses. This is semantically an exclusive match and would be clearer as:

```tsx
<Switch>
  <Match when={status() === 'done'}>
    <CheckCircle ... />
  </Match>
  <Match when={status() === 'in-progress'}>
    <Loader2 ... />
  </Match>
  <Match when={status() === 'pending'}>
    <Circle ... />
  </Match>
</Switch>
```

The current approach renders correctly because only one condition is true at a time, but `Switch/Match` makes the mutual exclusion explicit and avoids evaluating all three conditions.

### Performance Concerns

**`completedSteps()` in GenerateSynthesis is computationally expensive per call.** This derived function (lines 126-160) iterates over all events and performs multiple `findIndex` calls on every render triggered by new SSE events. For the typical 4-step pipeline this is negligible, but the algorithmic complexity is O(steps * events). If the event stream grew large, this would become visible. Consider memoizing with `createMemo`.

**`For` with reactive lookup in ApiKeyManager.** The `getKeyForProvider` function (line 33) scans the `apiKeys()` resource on every call. Since it is called inside the `For` loop's render function, it re-evaluates whenever `apiKeys` changes, which is correct. However, the returned value is not a signal -- it is a plain value computed from reactive data accessed during render. This works in SolidJS because `For` tracks the parent signal, but it means the entire provider list re-renders when any key changes. For 3 providers this is negligible.

**No unnecessary re-renders detected.** The project correctly avoids the React-style anti-pattern of destructuring signals at the component top level. Signal reads are deferred to JSX expressions (e.g., `{settings().theme}` rather than `const theme = settings().theme` outside JSX).

---

## Component Architecture

### Oversized Components

**Settings.tsx (1026 lines) is the primary concern.** This single component handles:
- Settings form (theme, age, items, articles, batch size, history days)
- Provider selection with auto-detection and warning
- Brave Search key management (save, test, delete, toggle)
- Search agent behavior textarea
- Category list with add/remove
- Rate limit configuration
- JSON export/import with optional API key inclusion
- API key manager delegation

This should be decomposed into at least 5 sub-components:
1. `SettingsForm` -- basic numeric/text fields
2. `ProviderSelector` -- provider dropdown, model dropdowns, web search badge
3. `BraveSearchSection` -- key management + toggle (this is essentially a duplicate of ProviderKeyCard logic)
4. `CategoryManager` -- category list CRUD
5. `RateLimitSection` -- rate limit inputs and reset

The Brave Search key management (lines 57-63, 134-179, 582-678) duplicates the same save/test/delete pattern already implemented in `ProviderKeyCard` within `ApiKeyManager.tsx`. This is a clear DRY violation -- Brave Search should be treated as another provider key card.

**Other pages are appropriately sized.** Home (240 lines), GenerateSynthesis (404 lines), Sources (~300 lines), and LlmLogs (159 lines) are all reasonable for their complexity.

### State Management

**Auth context is clean and minimal.** It provides the right abstraction surface (`user`, `loading`, `isAuthenticated`, `isAdmin`, `logout`, `refreshUser`). The `refreshUser` function is available but appears unused in the current codebase -- verify if this is needed.

**Toast context is well-implemented.** The module-level `activeTimers` Map for managing auto-dismiss timers is pragmatic. The `aria-live="polite"` on the toast container is a good accessibility touch.

**No global state beyond auth and toast.** Settings and other data are fetched per-page via `onMount`. This is appropriate for the application's complexity level -- introducing a global store would be premature.

**The `deleteTimers` signal in Home.tsx stores `setTimeout` handles in a reactive signal.** This works but is unusual. The timers are stored so they can be cleared on the second (confirming) click. However, these timers are never cleaned up on component unmount -- if the user navigates away while a delete confirmation is pending, the timeout callback will still fire and call `setDeletingId` on an unmounted component. This should use `onCleanup`.

### Reusability

**`Button` component exists but is rarely used.** A well-typed `Button` component with variant support (`primary`, `secondary`, `danger`), loading state, and icon slot lives in `components/ui/Button.tsx`. However, across the pages audited, buttons are overwhelmingly created with inline Tailwind classes rather than using this component. This leads to significant class string duplication. For example, the indigo primary button pattern appears in at least 12 places with minor variations.

**`LoadingSpinner` is consistently reused** across all pages -- good.

**`ProviderKeyCard` is a good reusable pattern** but its logic is duplicated for Brave Search in Settings.tsx as noted above.

**No shared form input components.** Every input field repeats the full Tailwind class string: `"shadow-sm focus:ring-indigo-500 focus:border-indigo-500 block w-full sm:text-sm border-gray-300 rounded-md py-2 px-3 border"`. A `TextInput`, `NumberInput`, and `Select` component would reduce duplication and ensure visual consistency.

---

## Type Safety

**Overall: strong.** The codebase makes effective use of TypeScript across all layers.

**Strengths:**
- All API request/response types are defined in `types.ts` with proper interfaces
- The `isApiError` type guard is used consistently for error handling
- The i18n system uses `TranslationKey` (derived from `keyof typeof fr`) to make translation keys type-checked at compile time
- `satisfies` is used in the API client for throw expressions (line 60 of client.ts) and API calls (syntheses.ts line 86)
- Props interfaces are defined for all components

**Issues:**

1. **`children: any` in Layout and AdminLayout.** Both layout components type their children prop as `any` instead of using `ParentComponent` or `JSX.Element`. This loses type safety on what can be passed as children. The fix is trivial: use `ParentComponent` as done in App.tsx's `ProtectedLayout`.

2. **Non-null assertion in ApiKeyManager.** Line 249 uses `props.apiKey!.key_prefix` inside a `<Show when={isConfigured()}>` block. While logically safe (the Show guard ensures the value exists), the compiler cannot verify this. Using `Show`'s callback form (`{(key) => key().key_prefix}`) would eliminate the assertion.

3. **`ApiClient.request` returns `undefined as T` for 204 responses (line 76 of client.ts).** This is a type lie -- the caller may not expect `undefined`. The methods that return `void` (like `delete`) work correctly because `undefined` satisfies `void`, but if a caller expected a non-void return from a 204, it would fail at runtime. Consider constraining 204-returning methods to `Promise<void>` at the call site.

4. **`isApiError` type guard is loose.** It checks for the presence of `status` and `message` properties but does not verify their types. Any object with these two keys would pass. Adding `typeof error.status === 'number'` would tighten it.

5. **`DEFAULT_SETTINGS` contains hardcoded French text** (category names, search behavior). This is a content/i18n boundary violation -- if the app were ever localized, these defaults would need to come from the i18n system.

---

## Code Organization

### File Structure -- Good

The project follows a clear convention:
- `pages/` for route-level components
- `components/` for shared UI
- `api/` for HTTP communication
- `utils/` for pure helpers
- `contexts/` for global state
- `i18n/` for translations
- `types.ts` as the single type definition file

This separation is clean and navigable.

### Import Patterns -- Consistent

All imports use the `~/` path alias (configured in vite.config.ts), avoiding fragile relative paths. Imports are grouped logically (solid-js, router, icons, local modules) though not enforced by a linter rule.

### Separation of Concerns -- Mostly Good

**API layer is well-isolated.** The `client.ts` singleton handles all HTTP concerns (CSRF headers, credentials, 401 redirect, error parsing). Individual API modules (`auth.ts`, `settings.ts`, etc.) are thin wrappers that only map routes to typed calls. This is excellent.

**Business logic leaks into pages.** The Settings page contains import/export logic (JSON serialization, file download, file parsing), Brave Search key management, and category validation. These should be extracted into utility functions or a settings service module.

**The `syntheses.ts` API module contains `triggerDownload` and `fetchFile` utility functions** that are not synthesis-specific. These should live in a shared `utils/download.ts` module so other features (e.g., CSV export in Sources) could reuse them.

### Missing Error Boundaries Per-Route

The application has a single top-level `ErrorBoundary` wrapping the entire router. An error in any page will replace the entire app with the error screen. Consider wrapping each route's content with a more localized error boundary that preserves the navigation bar.

---

## UI/UX Patterns

### Error Handling -- Consistent But Could Be Unified

Every page implements its own error display pattern with slight variations:
- Home: red banner with `border border-red-200`
- GenerateSynthesis: red banner with `border-l-4 border-red-400`
- LlmLogs: red box with `border border-red-200`

A shared `ErrorAlert` component would standardize these.

Error handling across the codebase consistently uses the `isApiError` type guard to surface server messages or fall back to i18n strings. This is a good pattern.

**`console.error` usage in Sources.tsx** (lines 103, 174) and ArticleHistory.tsx (line 104) should be replaced with user-visible error feedback (toast or inline message). Errors silently logged to the console are invisible to the user.

### Loading States -- Good

All pages that fetch data show `<LoadingSpinner />` during loading via `<Show when={!loading()} fallback={<LoadingSpinner />}>`. The spinner supports `fullPage` and `size` variants.

### Form Handling

**No form validation library.** Settings and Sources pages implement validation inline. This is acceptable at current scale but becomes harder to maintain as forms grow. The Settings page in particular has no client-side validation -- invalid values (e.g., negative `max_age_days`) are sent to the server.

**Controlled inputs use `value` + `onInput`.** This is the correct SolidJS pattern (not `onChange` which fires on blur in some browsers). Numeric inputs use `parseInt(...) || defaultValue` for fallback, which is pragmatic but means a user typing "0" gets the default instead.

**The import flow in Settings does not auto-save.** After importing a JSON file, the user sees a success message saying "N'oubliez pas d'enregistrer" (don't forget to save). This is a potential UX pitfall -- users may import and navigate away, losing changes.

### Accessibility

**Good:**
- `aria-label` on icon-only buttons (mobile menu toggle, show/hide key)
- `aria-live="polite"` on toast container
- `aria-hidden="true"` on decorative icons
- `role="alert"` on toast items
- Semantic HTML (`<nav>`, `<main>`, `<label for="...">`)
- Focus ring styles on all interactive elements

**Needs attention:**
- The `<details>` elements in LlmLogs use `list-none` to hide the default marker but provide no accessible name or ARIA attributes.
- The delete confirmation pattern (click once to arm, click again to confirm) has no visible text change on Home page cards -- only a color shift and a tooltip change. Users relying on screen readers would miss this state change.
- No skip-to-content link.
- Color alone is used to distinguish toast types (success=green, error=red, info=blue). The icons provide a secondary cue, which is good, but there is no text prefix like "Error: " or "Success: ".

### Security Concern: innerHTML Usage

**GenerateSynthesis.tsx line 249** uses `innerHTML` to render a translated string:

```tsx
<p innerHTML={t('generate.description', { days: ..., theme: ... })} />
```

The `theme` value comes from `settings().theme`, which is user-controlled content stored in the database. If a user sets their theme to `<img src=x onerror=alert(1)>`, this would execute arbitrary JavaScript. In a single-tenant self-hosted app this is self-XSS (the user can only attack themselves), but it is still a bad practice. Replace `innerHTML` with safe text interpolation, or sanitize the value.

---

## Prioritized Recommendations

### Priority 1 -- Fix Bugs and Security Issues

1. **Remove `innerHTML` usage in GenerateSynthesis.tsx.** Replace with safe text rendering. The translation string uses `{days}` and `{theme}` placeholders -- restructure it to use separate `<span>` elements or a template that does not require HTML parsing.

2. **Add `onCleanup` for `setTimeout` in Home.tsx.** The `deleteTimers` signal stores timeout handles that are never cleared on unmount. Add an `onCleanup` that iterates and clears all active timers.

3. **Add `onCleanup` for `setTimeout` in ArticleHistory.tsx** (line 95). The 3-second confirmation timeout is not cleaned up on unmount.

4. **Add `onCleanup` for `setTimeout` in SynthesisDetail.tsx** (line 150). The 5-second email success timeout is not cleaned up on unmount.

### Priority 2 -- Structural Improvements

5. **Decompose Settings.tsx** into sub-components (ProviderSelector, BraveSearchSection, CategoryManager, RateLimitSection). Target: each sub-component under 200 lines.

6. **Eliminate Brave Search key management duplication** by reusing `ProviderKeyCard` from ApiKeyManager.tsx or extracting the shared pattern.

7. **Replace `children: any` with `ParentComponent`** in Layout.tsx and AdminLayout.tsx.

8. **Use the existing `Button` component** more broadly, or remove it if the team prefers inline styling. Current inconsistency is confusing.

### Priority 3 -- Code Quality

9. **Standardize data fetching.** Pick either `createResource` or `onMount` + signals and apply consistently. `createResource` is recommended for simple GET-on-mount patterns.

10. **Extract shared form input components** (`TextInput`, `NumberInput`, `Select`) to reduce Tailwind class duplication.

11. **Create a shared `ErrorAlert` component** for consistent error display across pages.

12. **Replace `console.error` calls** in Sources.tsx and ArticleHistory.tsx with user-visible feedback (toast or inline error).

13. **Use `Switch/Match`** instead of multiple `<Show>` blocks for mutually exclusive conditions (GenerateSynthesis step icons).

### Priority 4 -- Polish

14. **Move `triggerDownload` and `fetchFile`** from `api/syntheses.ts` to a shared utility module.

15. **Tighten the `isApiError` type guard** to check `typeof status === 'number'`.

16. **Extract business logic** from Settings.tsx (JSON export/import, category validation) into dedicated utility functions.

17. **Consider route-level error boundaries** to preserve the navbar on page errors.

18. **Add i18n-aware defaults** for `DEFAULT_SETTINGS` so category names and search behavior are not hardcoded in French.

19. **Narrow the `createEffect` in Settings.tsx** (line 81) to track only `providers()` using `on()`.