Add some stats + plan after code review
This commit is contained in:
parent
1424029eae
commit
13f3b305bd
218
docs/code-review-remediation-plan.md
Normal file
218
docs/code-review-remediation-plan.md
Normal file
|
|
@ -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/<id>` and
|
||||
`/shorts/<id>` 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).
|
||||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
36
scripts/code-stats.sh
Executable file
36
scripts/code-stats.sh
Executable file
|
|
@ -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/
|
||||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue