# Frontend Review and Recommendations

Date: 2025-08-12T11:32:16-05:00
Scope: `frontend/` of `chatterbox-test` monorepo

---

## Summary
- Static vanilla JS frontend served by `frontend/start_dev_server.py` interacting with FastAPI backend under `/api`.
- Solid feature set (speaker management, dialog editor, per-line generation, full dialog generation, save/load) with robust error handling.
- Key issues: inconsistent API trailing slashes, Jest/babel-jest version/config mismatch, minor state duplication, alert/confirm UX, overly dark border color, token in `package.json` repo URL.

---

## Findings

- **Framework/structure**
  - `frontend/` is static vanilla JS. Main files:
    - `index.html`, `js/app.js`, `js/api.js`, `js/config.js`, `css/style.css`.
    - Dev server: `frontend/start_dev_server.py` (CORS, env-based port/host).

- **API client vs backend routes (trailing slashes)**
  - Frontend `frontend/js/api.js` currently uses:
    - `getSpeakers()`: `${API_BASE_URL}/speakers/` (trailing).
    - `addSpeaker()`: `${API_BASE_URL}/speakers/` (trailing).
    - `deleteSpeaker()`: `${API_BASE_URL}/speakers/${speakerId}/` (trailing).
    - `generateLine()`: `${API_BASE_URL}/dialog/generate_line`.
    - `generateDialog()`: `${API_BASE_URL}/dialog/generate`.
  - Backend routes:
    - `backend/app/routers/speakers.py`: `GET/POST /` and `DELETE /{speaker_id}` (no trailing slash on delete when prefixed under `/api/speakers`).
    - `backend/app/routers/dialog.py`: `/generate_line` and `/generate` (match frontend).
  - Tests in `frontend/tests/api.test.js` expect no trailing slashes for `/speakers` and `/speakers/{id}`.
  - Implication: Inconsistent trailing slashes can cause test failures and possible 404s for delete.

- **Payload schema inconsistencies**
  - `generateDialog()` JSDoc shows `silence` as `{ duration_ms: 500 }` but backend expects `duration` (seconds). UI also uses `duration` seconds.

- **Form fields alignment**
  - Speaker add uses `name` and `audio_file` which match backend (`Form` and `File`).

- **State management duplication in `frontend/js/app.js`**
  - `dialogItems` and `availableSpeakersCache` defined at module scope and again inside `initializeDialogEditor()`, creating shadowing risk. Consolidate to a single source of truth.

- **UX considerations**
  - Heavy use of `alert()`/`confirm()`. Prefer inline notifications/banners and per-row error chips (you already render `item.error`).
  - Add global loading/disabled states for long actions (e.g., full dialog generation, speaker add/delete).

- **CSS theme issue**
  - `--border-light` is `#1b0404` (dark red); semantically a light gray fits better and improves contrast harmony.

- **Testing/Jest/Babel config**
  - Root `package.json` uses `jest@^29.7.0` with `babel-jest@^30.0.0-beta.3` (major mismatch). Align versions.
  - No `jest.config.cjs` to configure `transform` via `babel-jest` for ESM modules.

- **Security**
  - `package.json` `repository.url` embeds a token. Remove secrets from VCS immediately.

- **Dev scripts**
  - Only `"test": "jest"` present. Add scripts to run the frontend dev server and test config explicitly.

- **Response handling consistency**
  - `generateLine()` parses via `response.text()` then `JSON.parse()`. Others use `response.json()`. Standardize for consistency.

---

## Recommended Actions (Phase 1: Quick wins)

- **Normalize API paths in `frontend/js/api.js`**
  - Use no trailing slashes:
    - `GET/POST`: `${API_BASE_URL}/speakers`
    - `DELETE`: `${API_BASE_URL}/speakers/${speakerId}`
    - Keep dialog endpoints unchanged.

- **Fix JSDoc for `generateDialog()`**
  - Use `silence: { duration: number }` (seconds), not `duration_ms`.

- **Refactor `frontend/js/app.js` state**
  - Remove duplicate `dialogItems`/`availableSpeakersCache` declarations. Choose module-scope or function-scope, and pass references.

- **Improve UX**
  - Replace `alert/confirm` with inline banners near `#results-display` and per-row error chips (extend existing `.line-error-msg`).
  - Add disabled/loading states for global generate and speaker actions.

- **CSS tweak**
  - Set `--border-light: #e5e7eb;` (or similar) to reflect a light border.

- **Harden tests/Jest config**
  - Align versions: either Jest 29 + `babel-jest` 29, or upgrade both to 30 stable together.
  - Add `jest.config.cjs` with `transform` using `babel-jest` and suitable `testEnvironment`.
  - Ensure tests expect normalized API paths (recommended to change code to match tests).

- **Dev scripts**
  - Add to root `package.json`:
    - `"frontend:dev": "python3 frontend/start_dev_server.py"`
    - `"test:frontend": "jest --config ./jest.config.cjs"`

- **Sanitize repository URL**
  - Remove embedded token from `package.json`.

- **Standardize response parsing**
  - Switch `generateLine()` to `response.json()` unless backend returns `text/plain`.

---

## Backend Endpoint Confirmation

- `speakers` router (`backend/app/routers/speakers.py`):
  - List/Create: `GET /`, `POST /` (when mounted under `/api/speakers` → `/api/speakers/`).
  - Delete: `DELETE /{speaker_id}` (→ `/api/speakers/{speaker_id}`), no trailing slash.
- `dialog` router (`backend/app/routers/dialog.py`):
  - `POST /generate_line`, `POST /generate` (mounted under `/api/dialog`).

---

## Proposed Implementation Plan

- **Phase 1 (1–2 hours)**
  - Normalize API paths in `api.js`.
  - Fix JSDoc for `generateDialog`.
  - Consolidate dialog state in `app.js`.
  - Adjust `--border-light` to light gray.
  - Add `jest.config.cjs`, align Jest/babel-jest versions.
  - Add dev/test scripts.
  - Remove token from `package.json`.

- **Phase 2 (2–4 hours)**
  - Inline notifications and comprehensive loading/disabled states.

- **Phase 3 (optional)**
  - ESLint + Prettier.
  - Consider Vite migration (HMR, proxy to backend, improved DX).

---

## Notes
- Current local time captured for this review: 2025-08-12T11:32:16-05:00.
- Frontend config (`frontend/js/config.js`) supports env overrides for API base and dev server port.
- Tests (`frontend/tests/api.test.js`) currently assume endpoints without trailing slashes.