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