trainUs/docs/code-review-remediation-plan.md

# 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).