From b62eb0211f87224b7eb63434f10d785593fe173b Mon Sep 17 00:00:00 2001 From: Steve White Date: Tue, 12 Aug 2025 12:16:23 -0500 Subject: [PATCH] =?UTF-8?q?feat(frontend):=20Phase=201=20=E2=80=93=20norma?= =?UTF-8?q?lize=20speakers=20endpoints,=20fix=20API=20docs=20and=20JSON=20?= =?UTF-8?q?parsing,=20consolidate=20state=20in=20app.js,=20tweak=20CSS=20b?= =?UTF-8?q?order=20color,=20align=20jest/babel-jest=20+=20add=20jest.confi?= =?UTF-8?q?g.cjs,=20add=20dev=20scripts,=20sanitize=20repo=20URL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .note/review-20250812.md | 138 +++++++++++++++++++++++++++++++++++++++ frontend/css/style.css | 2 +- frontend/js/api.js | 24 ++----- frontend/js/app.js | 3 - jest.config.cjs | 9 +++ package.json | 8 ++- 6 files changed, 160 insertions(+), 24 deletions(-) create mode 100644 .note/review-20250812.md create mode 100644 jest.config.cjs diff --git a/.note/review-20250812.md b/.note/review-20250812.md new file mode 100644 index 0000000..f972e71 --- /dev/null +++ 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. diff --git a/frontend/css/style.css b/frontend/css/style.css index b56396b..a718222 100644 --- 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; diff --git a/frontend/js/api.js b/frontend/js/api.js index 85549a0..288ca23 100644 --- 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} 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?" } * ] * } diff --git a/frontend/js/app.js b/frontend/js/app.js index 9eda494..f2506aa 100644 --- 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(); diff --git a/jest.config.cjs b/jest.config.cjs new file mode 100644 index 0000000..26555fa --- /dev/null +++ b/jest.config.cjs @@ -0,0 +1,9 @@ +// jest.config.cjs +module.exports = { + testEnvironment: 'node', + transform: { + '^.+\\.js$': 'babel-jest', + }, + moduleFileExtensions: ['js', 'json'], + roots: ['/frontend/tests', ''], +}; diff --git a/package.json b/package.json index c70d033..e6c432d 100644 --- 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" } }