From 7e9c825af35209882563fc7183084c1875cacc1f Mon Sep 17 00:00:00 2001 From: kaivalya Date: Thu, 9 Jul 2026 15:09:19 +0200 Subject: [PATCH] F1: Same-user re-import skips existing execution logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logs whose id already exists locally were re-inserted as-is, producing a PRIMARY KEY violation per log that surfaced as errors in the import report. They are now counted as skipped (execution logs are immutable history — skip is always the right resolution). Adds the missing regression test: re-import a full export without deleting anything. Co-Authored-By: Claude Fable 5 --- docs/decisions-log.md | 6 +++++ src/composables/useJsonImport.js | 7 ++++-- tests/test_step7_import_export.py | 41 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/decisions-log.md b/docs/decisions-log.md index 29fd371..5a51a5d 100644 --- a/docs/decisions-log.md +++ b/docs/decisions-log.md @@ -2,6 +2,12 @@ Record of architectural and technical decisions made during development. +## 2026-07-09 — Same-user re-import skips existing execution logs (code review F1) + +**Decision:** In `useJsonImport.js`, execution logs whose id already exists locally (`sameUserCollisions`) are now counted as skipped instead of being re-inserted with their original id. Previously each one hit a PRIMARY KEY violation whose message ended up in the import report's error list, making a routine "re-import my own full export" look like a partially failed import. + +**Rationale:** Execution logs are immutable historical records — unlike exercises/combos/sessions there is nothing to Replace or merge, so a skip is always the correct resolution and no Replace/Skip UI is warranted. A regression test covers the exact scenario the existing round-trip test missed: re-importing a full export *without* deleting anything first must produce zero errors and no duplicate logs. + ## 2026-07-06 — Voice announcements via the Web Speech API (Step 26) **Decision:** Added `useSpeech.js`, a thin wrapper around `window.speechSynthesis` exposing `init()` (reads the new `voice_enabled` settings key, default enabled), `speak(text, { volume })` (cancels any pending utterance first, so a new announcement always preempts a stale one, and maps the current i18next language to a BCP-47 tag: `en` → `en-US`, `fr` → `fr-FR`, `da` → `da-DK`), `cancel()`, and `isSupported`. `useSessionExecution.js` calls `speak()` with the exercise title whenever a step becomes current (`loadStepState()`), and pre-announces the *next* exercise ("Next: X") in the two places the on-screen UI already does the same thing silently: 10 s before an EMOM slot ends (hooked into the existing `_emomSounds` tick handler) and the instant a TABATA rest phase opens (`_startRestPhase`). AMRAP and plain (NONE) combos only get the step-change announcement, matching their on-screen behavior of not showing a "next" preview until the step actually changes. To avoid the same exercise being spoken twice in quick succession, the queue index that was just pre-announced is tracked in a closure variable; `loadStepState()` skips the step-change announcement exactly once when it reaches that index. `speechSynthesis.cancel()` is also called on `exit()` and `_finish()` so nothing keeps talking after the user leaves the execution view. diff --git a/src/composables/useJsonImport.js b/src/composables/useJsonImport.js index a102a39..fa88583 100644 --- a/src/composables/useJsonImport.js +++ b/src/composables/useJsonImport.js @@ -538,10 +538,13 @@ export function useJsonImport() { report, errors, ) { - // Collect all logs to import from the three analysis buckets + // Same-user collisions mean the log already exists locally. Execution logs + // are immutable historical records, so re-importing them is always a skip + // (re-inserting would only produce PRIMARY KEY violations). + report.skipped += (analysis.sameUserCollisions || []).length + const logsToImport = [ ...(analysis.noCollision || []).map((log) => ({ log, action: 'insert' })), - ...(analysis.sameUserCollisions || []).map((e) => ({ log: e.imported, action: 'insert' })), ...(analysis.differentUser || []).map((e) => ({ log: e.imported, action: e.action })), ] diff --git a/tests/test_step7_import_export.py b/tests/test_step7_import_export.py index e4fde0f..dd0b524 100644 --- a/tests/test_step7_import_export.py +++ b/tests/test_step7_import_export.py @@ -16,6 +16,7 @@ Step 7 — Full Data Import/Export Tests - test_import_as_mine_combo: Import-as-mine button works on combo list - test_import_as_mine_session: Import-as-mine button works on session list - test_execution_logs_same_user_roundtrip: Execution logs survive a same-user export/import cycle +- test_execution_logs_same_user_reimport_without_delete: Re-import without delete skips logs cleanly - test_execution_log_id_remapping: Log item exercise_id is remapped when the exercise gets a new_id - test_session_with_combo_item_remapping: Session combo item_id follows remap when combo gets new_id - test_collection_session_remapping: Collection session ref follows remap when session gets new_id @@ -691,6 +692,46 @@ def test_execution_logs_same_user_roundtrip(page: Page, app_url: str): assert result["sessionExists"], "Log session_id must reference an existing session after reimport" +def test_execution_logs_same_user_reimport_without_delete(page: Page, app_url: str): + """Re-importing a full export without deleting anything reports execution + logs as skipped (no constraint errors, no duplicate logs).""" + _go_to(page, app_url, "#/exercises") + _clean_all(page) + + ex_id = _create_exercise(page, "Reimport Exercise") + session_id = _create_session( + page, + "Reimport Session", + [{"item_id": ex_id, "item_type": "exercise", "repetitions": 10, "weight": 0}], + ) + _create_execution_log(page, session_id, ex_id) + + json_str = _build_full_export_json(page) + + # Re-import immediately — everything still exists locally + _trigger_import_with_json(page, json_str) + expect(page.locator('[data-testid="import-apply"]')).to_be_visible(timeout=10000) + page.locator('[data-testid="import-apply"]').click() + expect(page.locator('[data-testid="import-report-step"]')).to_be_visible(timeout=15000) + + # No constraint errors in the report + error_count = page.locator('[data-testid^="import-report-error-"]').count() + assert error_count == 0, f"Expected no import errors, got {error_count}" + + # The colliding log is reported as skipped, not imported + expect(page.locator('[data-testid="import-report-skipped"]')).to_be_visible() + assert page.locator('[data-testid="import-report-logs-imported"]').count() == 0, ( + "Existing execution log must be skipped, not re-imported" + ) + + page.click('[data-testid="import-done"]') + page.wait_for_timeout(500) + + # No duplicate logs in the database + log_count = page.evaluate("async () => (await window.__repos.executionLogs.getAll()).length") + assert log_count == 1, f"Expected 1 execution log after reimport, got {log_count}" + + def test_execution_log_id_remapping(page: Page, app_url: str): """When an exercise gets new_id on collision, the execution log item's exercise_id is remapped.""" _go_to(page, app_url, "#/exercises")