2025-08-14 15:44:25 +00:00
6 changed files with 160 additions and 24 deletions
--- a/.note/review-20250812.md
+++ b/.note/review-20250812.md
@ -0,0 +1,138 @@
+# 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.
--- a/frontend/css/style.css
+++ b/frontend/css/style.css
@ -24,7 +24,7 @@
    --text-blue-darker: #205081;
    
    /* Border Colors */
-    --border-light: #1b0404;
+    --border-light: #e5e7eb;
    --border-medium: #cfd8dc;
    --border-blue: #b5c6df;
    --border-gray: #e3e3e3;
--- a/frontend/js/api.js
+++ b/frontend/js/api.js
@ -10,7 +10,7 @@ const API_BASE_URL = API_BASE_URL_WITH_PREFIX;
 * @throws {Error} If the network response is not ok.
 */
 export async function getSpeakers() {
-    const response = await fetch(`${API_BASE_URL}/speakers/`);
+    const response = await fetch(`${API_BASE_URL}/speakers`);
    if (!response.ok) {
        const errorData = await response.json().catch(() => ({ message: response.statusText }));
        throw new Error(`Failed to fetch speakers: ${errorData.detail || errorData.message || response.statusText}`);
@ -26,12 +26,12 @@ export async function getSpeakers() {
 * Adds a new speaker.
 * @param {FormData} formData - The form data containing speaker name and audio file.
 *                               Example: formData.append('name', 'New Speaker');
- *                                        formData.append('audio_sample_file', fileInput.files[0]);
+ *                                        formData.append('audio_file', fileInput.files[0]);
 * @returns {Promise<Object>} A promise that resolves to the new speaker object.
 * @throws {Error} If the network response is not ok.
 */
 export async function addSpeaker(formData) {
-    const response = await fetch(`${API_BASE_URL}/speakers/`, {
+    const response = await fetch(`${API_BASE_URL}/speakers`, {
        method: 'POST',
        body: formData, // FormData sets Content-Type to multipart/form-data automatically
    });
@ -86,7 +86,7 @@ export async function addSpeaker(formData) {
 * @throws {Error} If the network response is not ok.
 */
 export async function deleteSpeaker(speakerId) {
-    const response = await fetch(`${API_BASE_URL}/speakers/${speakerId}/`, {
+    const response = await fetch(`${API_BASE_URL}/speakers/${speakerId}`, {
        method: 'DELETE',
    });
    if (!response.ok) {
@ -124,18 +124,8 @@ export async function generateLine(line) {
        const errorData = await response.json().catch(() => ({ message: response.statusText }));
        throw new Error(`Failed to generate line audio: ${errorData.detail || errorData.message || response.statusText}`);
    }
-    
-    const responseText = await response.text();
-    console.log('Raw response text:', responseText);
-    
-    try {
-        const jsonData = JSON.parse(responseText);
-        console.log('Parsed JSON:', jsonData);
-        return jsonData;
-    } catch (parseError) {
-        console.error('JSON parse error:', parseError);
-        throw new Error(`Invalid JSON response: ${responseText}`);
-    }
+    const data = await response.json();
+    return data;
 }

 /**
@ -146,7 +136,7 @@ export async function generateLine(line) {
 *     output_base_name: "my_dialog",
 *     dialog_items: [
 *       { type: "speech", speaker_id: "speaker1", text: "Hello world.", exaggeration: 1.0, cfg_weight: 2.0, temperature: 0.7 },
- *       { type: "silence", duration_ms: 500 },
+ *       { type: "silence", duration: 0.5 },
 *       { type: "speech", speaker_id: "speaker2", text: "How are you?" }
 *     ]
 *   }
--- a/frontend/js/app.js
+++ b/frontend/js/app.js
@ -140,9 +140,6 @@ async function initializeDialogEditor() {
    const zipArchivePlaceholder = document.getElementById('zip-archive-placeholder');
    const resultsDisplaySection = document.getElementById('results-display');

-    let dialogItems = [];
-    let availableSpeakersCache = []; // Cache for speaker names and IDs
-
    // Load speakers at startup
    try {
        availableSpeakersCache = await getSpeakers();
--- a/jest.config.cjs
+++ b/jest.config.cjs
@ -0,0 +1,9 @@
+// jest.config.cjs
+module.exports = {
+  testEnvironment: 'node',
+  transform: {
+    '^.+\\.js$': 'babel-jest',
+  },
+  moduleFileExtensions: ['js', 'json'],
+  roots: ['<rootDir>/frontend/tests', '<rootDir>'],
+};
--- a/package.json
+++ b/package.json
@ -5,11 +5,13 @@
  "main": "index.js",
  "type": "module",
  "scripts": {
-    "test": "jest"
+    "test": "jest",
+    "test:frontend": "jest --config ./jest.config.cjs",
+    "frontend:dev": "python3 frontend/start_dev_server.py"
  },
  "repository": {
    "type": "git",
-    "url": "https://oauth2:78f77aaebb8fa1cd3efbd5b738177c127f7d7d0b@gitea.r8z.us/stwhite/chatterbox-ui.git"
+    "url": "https://gitea.r8z.us/stwhite/chatterbox-ui.git"
  },
  "keywords": [],
  "author": "",
@ -17,7 +19,7 @@
  "devDependencies": {
    "@babel/core": "^7.27.4",
    "@babel/preset-env": "^7.27.2",
-    "babel-jest": "^30.0.0-beta.3",
+    "babel-jest": "^29.7.0",
    "jest": "^29.7.0"
  }
 }