139 lines
6.0 KiB
Markdown
139 lines
6.0 KiB
Markdown
# 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.
|