diff --git a/docs/code-review-remediation-plan.md b/docs/code-review-remediation-plan.md new file mode 100644 index 0000000..3781bd8 --- /dev/null +++ b/docs/code-review-remediation-plan.md @@ -0,0 +1,218 @@ +# Code Review Remediation Plan + +> **Source:** Full-project code review of 2026-07-06 (code, tests, documentation). +> **Status:** Proposed — not yet validated. +> **Convention:** One fix at a time, user validation between each, per AGENTS.md. +> Each fix follows the standard Definition of Done (code, tests on Firefox + +> Chromium, docs/decision-log updates where relevant, manual validation). + +Fixes are numbered F1–F12 and grouped into four phases by priority. Phases 1–2 +are behaviour fixes; Phase 3 is code quality; Phase 4 is documentation drift. + +--- + +## Phase 1 — Correctness bugs + +### F1 — Same-user re-import of execution logs reports constraint errors instead of skipping + +**Priority:** High (user-visible on a common operation — re-importing a full export). + +- **Problem:** In `src/composables/useJsonImport.js` (`_importExecutionLogs`, + ~line 544), logs bucketed into `sameUserCollisions` (log id already exists + locally) are mapped to `action: 'insert'` and re-inserted with their original + id. Every one hits a PRIMARY KEY violation; the error is caught and pushed to + `report.errors`, so the import report shows constraint errors for what should + be clean skips. +- **Fix:** Treat same-user log collisions as skips: count them in + `report.skipped` and do not attempt the insert. (Execution logs are immutable + historical records — Replace/Skip resolution UI is not needed; skip is always + correct.) +- **Tests:** New test in `tests/test_step7_import_export.py` (or a new file): + export full data → re-import **without deleting anything** → assert + `report.errors` is empty, `executionLogs.skipped` equals the log count, and no + duplicate logs exist. This is the exact scenario the existing round-trip test + misses (it deletes all data before importing). +- **Files:** `src/composables/useJsonImport.js`, test file. +- **Effort:** Small. + +### F2 — Streak counts aborted sessions + +**Priority:** High (one-line fix, wrong stat displayed on Stats view). + +- **Problem:** `getDistinctSessionDays()` in + `src/db/repositories/executionLogRepository.js` has no status filter, while + every other stat query filters `status = 'completed'`. Starting a session and + immediately exiting extends the streak. +- **Fix:** Add `WHERE status = 'completed'` to the query. +- **Tests:** Extend `tests/test_step9_stats.py`: create one completed log + (today) and one aborted log (yesterday) → streak must be 1, not 2. +- **Files:** `src/db/repositories/executionLogRepository.js`, `tests/test_step9_stats.py`. +- **Effort:** Trivial. + +### F3 — DB restore copies the backup's `schema_version` into the live DB (latent) + +**Priority:** High impact, currently dormant (only triggers once `SCHEMA_VERSION > 1`). + +- **Problem:** In `src/db/worker.js` (`handleImportDb`), `tablesToRestore` + includes `schema_version`. Restoring an older-version backup (after adapt + scripts run on the temp DB) leaves the live DB with current-schema tables but + an old version stamp. Next launch reads migration-pending and re-runs + migrations against an already-adapted schema — e.g. a duplicate + `ALTER TABLE ADD COLUMN` fails and blocks the app behind the + migration-failed screen. +- **Fix (pick one during implementation):** + 1. Exclude `schema_version` from `tablesToRestore` (the live DB's stamp is + already correct by construction), **or** + 2. Re-stamp `SCHEMA_VERSION` into `schema_version` after a successful restore. + Option 1 is simpler and keeps the restore loop generic. Also add a comment in + `src/db/releases.js` explaining the interaction, so the first real migration + ships with a restore test. +- **Tests:** Hard to test end-to-end while only schema v1 exists. Minimum now: + a test asserting that after restoring a valid backup, `MAX(version)` in + `schema_version` equals `SCHEMA_VERSION` (guards option 1/2 regardless of + future versions). Note in the test docstring that a full older-version + restore test must accompany the first v2 migration. +- **Files:** `src/db/worker.js`, `src/db/releases.js`, `tests/test_step19_db_resilience.py` or `test_step2_settings.py` (restore tests live there). +- **Effort:** Small; the thinking is done, the change is a filter + comment. + +--- + +## Phase 2 — Data integrity & UX + +### F4 — Deleting an exercise silently orphans `session_item` rows and hides usage + +**Priority:** Medium (surprising data loss from the user's perspective). + +- **Problem:** `session_item.item_id` is polymorphic (no FK). Deleting an + exercise leaves orphaned `session_item` rows; `sessionRepository.getItems` + INNER JOINs on exercise, so the entries silently vanish from the session. The + delete confirmation (`src/views/ExerciseDetailView.vue:48`) warns about + execution-history cascade but not about sessions/combos that use the exercise. + Same reasoning applies to deleting a combo used in sessions, and a session + used in collections (those junctions have FK CASCADE, so no orphans, but the + usage warning is still missing). +- **Fix:** + 1. Add repository count helpers: sessions-using-exercise, + combos-using-exercise (exists via `combo_exercise`), sessions-using-combo, + collections-using-session. + 2. Extend delete confirmations to include usage counts (new i18n keys ×3 + languages). + 3. On exercise delete, also `DELETE FROM session_item WHERE item_id = ? AND + item_type = 'exercise'` (and same for combo delete) so no orphans remain. +- **Decision to validate first:** blocking vs. informative confirm — proposal: + keep it a confirm (informative), consistent with current UX. +- **Tests:** Extend `tests/test_step3_exercises.py` / `test_step4_combos.py`: + delete an exercise used in a session → confirm mentions usage; after delete, + no `session_item` rows reference it. +- **Files:** repositories, `ExerciseDetailView.vue`, `ComboDetailView.vue`, + `SessionDetailView.vue`, locale files, tests. +- **Effort:** Medium (touches several views + i18n). + +### F5 — `renameSuffix` is not atomic + +**Priority:** Medium (rare operation, but can half-rename the database). + +- **Problem:** `src/db/repositories/suffixRepository.js` runs 5 sequential + `db.run` statements. A UNIQUE-title collision on the REPLACE mid-way leaves a + partial rename with no rollback. +- **Fix:** Build the 5 statements and run them through the existing + `db.batch()` (single transaction, already rolls back on error). Surface the + failure to the caller (SettingsView) as a translated error. Optionally + pre-check for title collisions (`SELECT` for would-be-duplicate titles) to + give a friendlier message than a constraint error. +- **Tests:** Extend the suffix rename test: seed a title that will collide + after rename → assert the operation fails **and** nothing was renamed. +- **Files:** `src/db/repositories/suffixRepository.js`, `src/views/SettingsView.vue` (error display), tests, locales if new error key. +- **Effort:** Small. + +### F6 — HTML export icon fetch breaks on subdirectory deploys + +**Priority:** Low (graceful degradation — icon is just omitted). + +- **Problem:** `fetchIconAsBase64()` in `src/composables/useHtmlExport.js` + fetches `'/icon.svg'`, ignoring `base: './'`, even though `database.js` + computes `BASE_URL` for the wasm file for exactly this reason. +- **Fix:** `fetch(new URL('icon.svg', new URL(import.meta.env.BASE_URL, window.location.href)))` + (or simply `import.meta.env.BASE_URL + 'icon.svg'`). +- **Tests:** Existing HTML export test keeps passing; subdirectory deploys are + not covered by the test env — verify via `npm run preview` under a subpath as + part of manual validation. +- **Effort:** Trivial. + +--- + +## Phase 3 — Code quality (no behaviour change) + +### F7 — Deduplicate the four CRUD composables + +- **Problem:** `useExercises`, `useCombos`, `useSessions`, `useCollections` + are the same 54 lines with a different repository (~160 duplicated lines). +- **Fix:** `src/composables/createCrudComposable.js` factory + (`createCrudComposable(repository, itemsKey)`); the four composables become + one-liners re-exporting the factory result, so **no call site changes**. +- **Tests:** No new tests — the entire existing suite is the regression net. +- **Effort:** Small. Risk: low, mechanical. + +### F8 — Date formatting: Danish gets US format; inconsistent strategies + +- **Problem:** `formatDate` in `src/utils/dateFormatter.js` special-cases only + `fr`; Danish users see `mm/dd/yyyy`. `formatDateTime` mixes a manual French + format with `toLocaleString()` for everyone else. +- **Fix:** Use `Intl.DateTimeFormat(langTag)` for both, mapping app language → + locale tag (reuse the `LANG_MAP` idea from `useSpeech.js`). Keep output + deterministic per language so tests can assert it. +- **Tests:** Unit-style assertions via page.evaluate on the three languages + (extend the tests that already check date rendering). +- **Effort:** Small–medium (check which tests assert current formats). + +### F9 — Small polish (batchable in one step) + +- `getYouTubeId` (`src/utils/youtube.js`): handle `/embed/` and + `/shorts/` paths. +- `useListNavigation` JSDoc: `itemCount` is documented as a number but must be + a ref. +- `collectionRepository.getAll` N+1 count → single `LEFT JOIN … GROUP BY`. + (Other N+1 loops — `getAllWithItems`, `getRecentWithItems` — are acceptable + at local-DB scale; only do them if trivial while in the file.) +- **Tests:** Extend youtube-related test (step 13 markdown / detail views) with + embed + shorts URLs; existing collection tests cover the query change. +- **Effort:** Small. + +--- + +## Phase 4 — Documentation drift + +### F10 — `docs/plan.md` overview still says "English (default) + French" + +Danish shipped since. Update the overview paragraph. + +### F11 — `AGENTS.md` Build & Run section is incomplete + +Missing `npm run lint`, `lint:fix`, and the `test`/`test:*` scripts that exist +in `package.json` (CI runs lint, so agents/contributors should know it exists). + +### F12 — Decision log entries for the fixes above + +As Phase 1–3 fixes land, record the notable ones in `docs/decisions-log.md` +(F1, F3, F4 at minimum — they encode behaviour decisions, not just bug fixes). + +--- + +## Suggested execution order + +| Order | Fix | Why this order | +| ----- | --- | -------------- | +| 1 | F2 | Trivial, immediately user-visible stat fix | +| 2 | F1 | High-value, small, adds the missing re-import regression test | +| 3 | F3 | Small, defuses the migration time bomb before it's forgotten | +| 4 | F5 | Small, closes the only non-atomic multi-statement write | +| 5 | F6 | Trivial | +| 6 | F4 | Larger; needs a UX decision validated first | +| 7 | F7–F9 | Quality pass, no behaviour change | +| 8 | F10–F12 | Docs sweep (F12 happens incrementally alongside the fixes) | + +**Out of scope (reviewed, judged acceptable):** N+1 loops in +`getAllWithItems`-style repository methods (local DB, small data), the +`useSessionExecution` prefill-side behaviour for asymmetric exercises across +rounds, and prefill-from-aborted-runs (`getLastItemsBySession` takes the most +recent log regardless of status — arguably the right UX). diff --git a/docs/plan.md b/docs/plan.md index 233f026..21d1dd6 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1,7 +1,7 @@ # TrainUs — Implementation Plan (v3) > **Last updated:** 2026-07-06 -> **Status:** Step 24 implemented (app-update modal) — awaiting manual validation; Step 25 implemented (headset media controls) — awaiting manual validation; Step 26 implemented (voice announcements) — awaiting manual validation +> **Status:** Step 24 implemented (app-update modal) — awaiting manual validation; Step 25 implemented (headset media controls) — awaiting manual validation; Step 26 implemented (voice announcements) — awaiting manual validation. Full suite green on Firefox 2026-07-06 (374 passed, 1 skipped). ## Overview @@ -869,7 +869,7 @@ Hugo 0.123.7 static site under `website/`. FR default at `/`, EN at `/en/`. Depl ### Step 26 — Voice Announcements (Web Speech API) -**Status:** ✅ Implemented (2026-07-06) — awaiting manual validation by the user. Implemented as planned below; 9 Playwright tests passing on Firefox + Chromium. +**Status:** ✅ Implemented (2026-07-06) — awaiting manual validation by the user. Implemented as planned below; 9 Playwright tests passing on Firefox + Chromium. Full regression suite (374 passed, 1 skipped) green on Firefox 2026-07-06. **Goal:** in execution mode, a synthesized voice (SpeechSynthesis — `window.speechSynthesis`, no audio assets, supported by Firefox and Chromium) announces the exercise title at each step change. In TABATA and EMOM the _next_ exercise is pre-announced. The voice can be disabled in Settings → Sound. diff --git a/scripts/code-stats.sh b/scripts/code-stats.sh new file mode 100755 index 0000000..e63b56c --- /dev/null +++ b/scripts/code-stats.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +# TrainUs code statistics — lines of code by area, via cloc. +# Excludes generated output, dependencies, and the Hugo theme. +# Usage: ./scripts/code-stats.sh + +set -euo pipefail +cd "$(dirname "$0")/.." + +if ! command -v cloc >/dev/null 2>&1; then + echo "cloc is required (e.g. 'apt install cloc' or 'brew install cloc')" >&2 + exit 1 +fi + +echo "=== Overall (src/, tests/, website/, docs/) ===" +cloc . \ + --exclude-dir=node_modules,dist,.git,dev-dist,tmp,.venv \ + --fullpath --not-match-d='(^|/)(public|resources|themes)(/|$)' \ + --exclude-ext=json,lock + +echo +echo "=== src/ (application code) ===" +cloc src/ --exclude-ext=json + +echo +echo "=== tests/ (Playwright, excluding venv) ===" +cloc tests/ --exclude-dir=.venv --exclude-ext=json + +echo +echo "=== website/ (docs site, excluding generated/theme/deps) ===" +cloc website/ \ + --fullpath --not-match-d='(^|/)(public|resources|themes|node_modules)(/|$)' \ + --exclude-ext=json,lock + +echo +echo "=== docs/ ===" +cloc docs/ diff --git a/website/content/developers/technical.en.md b/website/content/developers/technical.en.md index a10236c..e1baef0 100644 --- a/website/content/developers/technical.en.md +++ b/website/content/developers/technical.en.md @@ -69,6 +69,19 @@ trainUs/ └── tests/ # Playwright Python test suite ``` +### Code Statistics + +Lines of code by area, measured with [cloc](https://github.com/AlDanial/cloc) (generated files, dependencies, and the Hugo theme excluded): + +| Area | Files | Code lines | Main languages | +| ------------------------ | ----: | ---------: | -------------------------------- | +| `src/` (application) | 81 | 13,973 | Vue SFC, JavaScript, CSS | +| `tests/` (Playwright) | 37 | 7,371 | Python | +| `website/` (docs site) | 49 | 4,732 | Markdown, CSS, HTML | +| `docs/` | 4 | 1,500 | Markdown | + +Within `src/`, Vue single-file components make up the bulk of application code (9,045 lines across 32 components), with 4,065 lines of plain JavaScript (composables, DB layer, utilities, router). + ### Main Thread / Worker Architecture SQLite runs in a **Web Worker** to access OPFS APIs, which are unavailable on the main thread. diff --git a/website/content/developers/technical.fr.md b/website/content/developers/technical.fr.md index e76e137..ca00333 100644 --- a/website/content/developers/technical.fr.md +++ b/website/content/developers/technical.fr.md @@ -69,6 +69,19 @@ trainUs/ └── tests/ # Suite de tests Playwright en Python ``` +### Statistiques du code + +Lignes de code par zone, mesurées avec [cloc](https://github.com/AlDanial/cloc) (fichiers générés, dépendances et thème Hugo exclus) : + +| Zone | Fichiers | Lignes de code | Langages principaux | +| -------------------------- | -------: | --------------: | --------------------------------- | +| `src/` (application) | 81 | 13 973 | Vue SFC, JavaScript, CSS | +| `tests/` (Playwright) | 37 | 7 371 | Python | +| `website/` (site de docs) | 49 | 4 732 | Markdown, CSS, HTML | +| `docs/` | 4 | 1 500 | Markdown | + +Dans `src/`, les composants Vue monofichiers constituent l'essentiel du code applicatif (9 045 lignes réparties sur 32 composants), avec 4 065 lignes de JavaScript pur (composables, couche DB, utilitaires, router). + ### Architecture thread principal / Worker SQLite tourne dans un **Web Worker** pour accéder aux API OPFS, indisponibles sur le thread principal.