diff --git a/Context/Backlog/Bugs.md b/Context/Backlog/Bugs.md index 68e9bd8..482e368 100644 --- a/Context/Backlog/Bugs.md +++ b/Context/Backlog/Bugs.md @@ -5,6 +5,17 @@ High-priority bugs should be addressed before starting new features. +## B009: `mapScheduledSession` overrides happy-path not covered by any test + +**Severity:** Medium +**File:** `src/lib/__tests__/supabase-adapter.test.ts` +**Detail:** All existing fixtures set `overrides: null`, so the `if (r.overrides != null)` branch in +`mapScheduledSession` is never exercised by a green test. Neither the string-parse branch (Tauri/SQLite +path) nor the object-passthrough branch (Supabase PostgREST path) has coverage. +**Fix:** Add two tests -- one with `overrides: JSON.stringify({ ... })` asserting the parsed output, one with +a pre-parsed object asserting direct passthrough. Use a valid `SessionOverrides` fixture in each. +**Origin:** P19-003 (PR #112 review, 2026-04-15) + ## ~~B001: invokeCommand does not log before re-throwing~~ (Resolved 2026-04-04) **Severity:** High @@ -36,11 +47,10 @@ High-priority bugs should be addressed before starting new features. **Severity:** Medium **Resolution:** Fixed in `5d2335c` -- disable delete-week button when block has only one week. -### B008: Gym selector shown every time a workout is started -**Added:** 2026-04-12 -**Context:** The "select gym" modal/selector appears on every workout start, even when the user has already chosen a gym in a previous session. The last-selected gym should be persisted and pre-selected so the user only needs to intervene when changing gyms. -**Related:** `src/stores/active-workout-store.ts`, gym-picker-storage, workout start flow -**Priority:** Medium +### ~~B008: Gym selector shown every time a workout is started~~ (Resolved 2026-04-14) + +**Severity:** Medium +**Resolution:** Fixed in PR #109 (gym picker fix -- last-selected gym now persisted across sessions). ### ~~B007: Active workout only shows one exercise despite multiple being added~~ (Resolved 2026-04-13) diff --git a/Context/QuickPlans/Done/2026-04-15-b009-overrides-happy-path.md b/Context/QuickPlans/Done/2026-04-15-b009-overrides-happy-path.md new file mode 100644 index 0000000..87bc0da --- /dev/null +++ b/Context/QuickPlans/Done/2026-04-15-b009-overrides-happy-path.md @@ -0,0 +1,76 @@ +# B009: mapScheduledSession Overrides Happy-Path Tests + +**Task:** Add two tests covering the `if (r.overrides != null)` branch in `mapScheduledSession` +**Goal:** Both the string-parse path (Tauri/SQLite) and the object-passthrough path (Supabase PostgREST) have green assertions +**Scope:** Tests only -- no production code changes + +--- + +## Approach + +Both tests go inside the existing `getProgramFull` describe block in +`src/lib/__tests__/supabase-adapter.test.ts`, alongside the malformed-JSON test. + +### Fixture shape + +`sessionOverridesSchema` is: +```typescript +{ activityOverrides?: Record } +``` + +Use a minimal valid override: +```typescript +const overridesObj = { activityOverrides: { 'act-001': { exerciseId: 'ex-002' } } } +``` + +### Test 1 -- string-parse path (Tauri/SQLite) + +```typescript +it('parses overrides when stored as a JSON string (Tauri/SQLite path)', async () => { + const row = { ...scheduledSessionRow, overrides: JSON.stringify(overridesObj) } + mockClient.mockResponse('programs', 'select', [programRow]) + mockClient.mockResponse('blocks', 'select', [blockRow]) + mockClient.mockResponse('block_weeks', 'select', [blockWeekRow]) + mockClient.mockResponse('scheduled_sessions', 'select', [row]) + + const result = await adapter.getProgramFull('prog-001') + + expect(result!.scheduledSessions[0].overrides).toEqual(overridesObj) +}) +``` + +### Test 2 -- object-passthrough path (Supabase PostgREST) + +```typescript +it('passes through overrides when already a parsed object (PostgREST path)', async () => { + const row = { ...scheduledSessionRow, overrides: overridesObj as unknown as string } + mockClient.mockResponse('programs', 'select', [programRow]) + mockClient.mockResponse('blocks', 'select', [blockRow]) + mockClient.mockResponse('block_weeks', 'select', [blockWeekRow]) + mockClient.mockResponse('scheduled_sessions', 'select', [row]) + + const result = await adapter.getProgramFull('prog-001') + + expect(result!.scheduledSessions[0].overrides).toEqual(overridesObj) +}) +``` + +Define `overridesObj` as a `const` at the top of the describe block (or inline in each test). + +--- + +## Verification + +```bash +bun run test src/lib/__tests__/supabase-adapter.test.ts +bun run test # full suite green +``` + +--- + +## Risks + +- `ScheduledSessionRow.overrides` is typed as `string | null`. The object-passthrough test + needs `as unknown as string` cast on the fixture (same pattern as `sessionTemplateRow.rest_between_groups`). +- If `sessionOverridesSchema.parse()` rejects the fixture shape, adjust `overridesObj` to + match the actual schema. diff --git a/Context/Reviews/0019-pr112-adapter-test-coverage-2026-04-15.md b/Context/Reviews/0019-pr112-adapter-test-coverage-2026-04-15.md new file mode 100644 index 0000000..0d5d8fb --- /dev/null +++ b/Context/Reviews/0019-pr112-adapter-test-coverage-2026-04-15.md @@ -0,0 +1,115 @@ +# PR Review: feat/p17-adapter-test-coverage → main + +**Date:** 2026-04-15 +**Feature:** Context/QuickPlans/Done/2026-04-15-p17-adapter-test-coverage.md +**Branch:** feat/p17-adapter-test-coverage +**Reviewers:** pr-test-analyzer, code-reviewer, silent-failure-hunter +**Status:** ✅ Resolved + +## Summary + +6 findings total across 2 source files. The test additions are accurate and well-structured, +but the production error handling in `mapScheduledSession` has a critical silent-failure design +problem: a single catch block conflates JSON parse errors and Zod schema errors, causing valid +overrides data to be silently dropped on schema mismatch. 4 fix-now items and 1 missing task. + +## Findings + +### Fix-Now + +#### [FIX] P19-001: `mapScheduledSession` catch conflates `SyntaxError` and `ZodError` +- **File:** `src/lib/supabase-adapter.ts:486-495` +- **Severity:** Critical +- **Detail:** The single `catch` block traps both `JSON.parse` failures (malformed data, expected) + and `sessionOverridesSchema.parse()` failures (Zod schema mismatch, a data integrity signal). + Both receive `console.warn` + `overrides = undefined`, silently dropping a user's valid + per-session overrides after any schema change or migration regression. Zod failures should + `console.error` and be treated as an integrity event, not a graceful degradation. + Split the catch: + ```typescript + } catch (err) { + if (err instanceof SyntaxError) { + console.warn(`[supabase-adapter] Failed to parse overrides JSON for session ${r.id as string}, falling back to undefined:`, err) + } else { + console.error(`[supabase-adapter] overrides schema validation failed for session ${r.id as string}. Data integrity issue:`, err) + } + overrides = undefined + } + ``` +- **Status:** ✅ Fixed +- **Resolution:** Split catch block: `z.ZodError` → `console.error` (integrity event), other errors → `console.warn` (expected bad input). Combined with P19-002 fix. + +#### [FIX] P19-002: `mapScheduledSession` uses inline `JSON.parse` instead of `parseJsonOrValue` +- **File:** `src/lib/supabase-adapter.ts:487` +- **Severity:** High +- **Detail:** Every other JSONB column in `supabase-adapter.ts` (lines 344-411: `restBetweenGroups`, + `timeCap`, `eventMetadata`, `restBetweenRounds`, `setScheme`) uses the shared `parseJsonOrValue` + utility. The `overrides` field uses a hand-rolled `typeof r.overrides === 'string' ? JSON.parse(r.overrides) : r.overrides` + inline. If `parseJsonOrValue` is updated (logging, error format, Sentry hooks), `overrides` + silently stays on the old path. Replace with `parseJsonOrValue(r.overrides as string | object, 'overrides')`. +- **Status:** ✅ Fixed +- **Resolution:** Replaced inline JSON.parse with `parseJsonOrValue(r.overrides as string | object, 'overrides')`. Combined with P19-001 catch-split fix. + +#### [FIX] P19-004: Warn spy doesn't assert session ID in the message +- **File:** `src/lib/__tests__/supabase-adapter.test.ts:1162` +- **Severity:** Medium +- **Detail:** The malformed-JSON test asserts `expect.stringContaining('[supabase-adapter]')` but + not that the session ID appears in the warn message. The production code includes the session ID + for production debuggability (`Failed to parse overrides for scheduled session ${r.id}`). If a + future refactor strips the ID, the test still passes. Add a second + `expect.stringContaining('')` matcher. +- **Status:** ✅ Fixed +- **Resolution:** Added second `expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('ss-001'), ...)` assertion. + +#### [FIX] P19-005: Warn spy cleanup is fragile if an assertion throws before `mockRestore()` +- **File:** `src/lib/__tests__/supabase-adapter.test.ts:1151-1167` +- **Severity:** Low +- **Detail:** `warnSpy.mockRestore()` is called at the end of the test body. If either + `expect(result!.scheduledSessions[0].overrides).toBeUndefined()` or the `warnSpy` + assertion throws, the spy leaks into subsequent tests. Vitest does not auto-restore spies + unless `restoreMocks: true` is configured. Wrap spy setup/restore in `try/finally`, or + move the spy to a scoped `afterEach`. +- **Status:** ✅ Fixed +- **Resolution:** Wrapped all assertions in `try/finally { warnSpy.mockRestore() }` to guarantee cleanup. + +#### [FIX] P19-006: Outer catch wraps error with `new Error(msg)` losing original error type +- **File:** `src/lib/supabase-adapter.ts:507-510` +- **Severity:** Low +- **Detail:** The outer catch block in `mapScheduledSession` uses + `throw new Error(`Failed to map scheduled session (${r.id}): ${err.message}`)` which + discards the original error's type and stack chain. Use `{ cause: err }` to preserve it: + ```typescript + throw new Error(`Failed to map scheduled session (${r.id as string})`, { cause: err }) + ``` +- **Status:** ✅ Fixed +- **Resolution:** Replaced multi-argument `new Error(msg: ${err.message})` with `new Error(msg, { cause: err })`. + +### Missing Tasks + +#### [TASK] P19-003: Overrides happy-path not covered by any test +- **File:** `src/lib/__tests__/supabase-adapter.test.ts` +- **Severity:** Medium +- **Detail:** `scheduledSessionRow.overrides` is `null` in all existing fixtures, so the + `if (r.overrides != null)` block in `mapScheduledSession` is never entered in any passing + test. Neither the string-parse branch (Tauri/SQLite path) nor the object-passthrough branch + (Supabase PostgREST path) has a green assertion. Add two tests: one with + `overrides: JSON.stringify({ ... })` asserting parsed output, one with a pre-parsed object + asserting direct passthrough. +- **Relates to:** P17-009 (fixture branch coverage) +- **Status:** ✅ Task created +- **Resolution:** Added as B009 in Context/Backlog/Bugs.md (no active Steps.md for this completed QuickPlan). + +## Resolution Checklist +- [x] All [FIX] findings resolved (P19-001, P19-002, P19-004, P19-005, P19-006) +- [x] All [TASK] findings added to backlog (P19-003 → B009) +- [x] Review verified by review-verify agent + +## Resolution Summary +**Resolved at:** 2026-04-15 +**Session:** resolve-review 0019 + +| Category | Total | Resolved | +|---|---|---| +| [FIX] | 5 | 5 | +| [TASK] | 1 | 1 | +| **Total** | **6** | **6** | diff --git a/src/lib/__tests__/adapter-utils.test.ts b/src/lib/__tests__/adapter-utils.test.ts new file mode 100644 index 0000000..9bc27d7 --- /dev/null +++ b/src/lib/__tests__/adapter-utils.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect } from 'vitest' +import { camelizeKeys, parseJsonOrValue } from '../adapter-utils' + +// =========================================================================== +// camelizeKeys +// =========================================================================== + +describe('camelizeKeys', () => { + it('converts standard snake_case keys to camelCase', () => { + expect(camelizeKeys({ created_at: 'x' })).toEqual({ createdAt: 'x' }) + }) + + it('passes through already-camelCase keys unchanged', () => { + expect(camelizeKeys({ createdAt: 'x' })).toEqual({ createdAt: 'x' }) + }) + + it('converts multiple underscores (foo_bar_baz)', () => { + expect(camelizeKeys({ foo_bar_baz: 1 })).toEqual({ fooBarBaz: 1 }) + }) + + it('transforms leading underscore because regex /_([a-z])/g matches _p in _private', () => { + // The regex /_([a-z])/g matches the `_p` in `_private`, turning it into `Private` + expect(camelizeKeys({ _private: 'x' })).toEqual({ Private: 'x' }) + }) + + it('does not transform underscores adjacent to digits', () => { + // known limitation: underscores adjacent to digits are not transformed + // /_([a-z])/g does not match `_1` because `1` is not [a-z] + expect(camelizeKeys({ supports_1rm: true })).toEqual({ supports_1rm: true }) + }) + + it('does not deep-transform nested object values', () => { + // Only top-level keys are converted; nested keys are left as-is + expect(camelizeKeys({ foo_bar: { nested_key: 1 } })).toEqual({ + fooBar: { nested_key: 1 }, + }) + }) + + it('returns an empty object unchanged', () => { + expect(camelizeKeys({})).toEqual({}) + }) +}) + +// =========================================================================== +// parseJsonOrValue +// =========================================================================== + +describe('parseJsonOrValue', () => { + it('parses a valid JSON string into an object', () => { + expect(parseJsonOrValue('{"a":1}', 'col')).toEqual({ a: 1 }) + }) + + it('returns a pre-parsed object unchanged (passthrough)', () => { + const obj = { foo: 'bar' } + expect(parseJsonOrValue(obj, 'col')).toEqual(obj) + }) + + it('returns an array input unchanged (not a string)', () => { + const arr = [1, 2, 3] + expect(parseJsonOrValue(arr as unknown as object, 'col')).toEqual(arr) + }) + + it('throws an error on malformed JSON whose message contains the column name', () => { + expect(() => parseJsonOrValue('{bad json}', 'overrides')).toThrow(/overrides/) + }) + + it('throws an error on malformed JSON whose message contains the raw value', () => { + expect(() => parseJsonOrValue('{bad json}', 'overrides')).toThrow(/\{bad json\}/) + }) +}) diff --git a/src/lib/__tests__/supabase-adapter.test.ts b/src/lib/__tests__/supabase-adapter.test.ts index 7306762..a8a76dd 100644 --- a/src/lib/__tests__/supabase-adapter.test.ts +++ b/src/lib/__tests__/supabase-adapter.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'vitest' +import { describe, it, expect, beforeEach, vi } from 'vitest' import { SupabaseAdapter } from '../supabase-adapter' import { createMockSupabaseClient, type MockSupabaseClient } from '@/test/mocks/supabase-client' import type { SupabaseClient } from '@supabase/supabase-js' @@ -161,7 +161,7 @@ const sessionTemplateRow: SessionTemplateRow = { name: 'Heavy Upper', description: 'Upper body strength', category: 'STRENGTH', - rest_between_groups: JSON.stringify({ seconds: 120 }), + rest_between_groups: { seconds: 120 } as unknown as string, time_cap: null, scoring: 'NONE', is_public: false, @@ -188,12 +188,12 @@ const templateActivityRow: ActivityRow = { activity_group_id: 'ag-001', exercise_id: 'ex-001', ordinal: 1, - set_scheme: JSON.stringify({ + set_scheme: { type: 'fixedSets', sets: 3, reps: 5, load: { type: 'absolute', weight: { value: 135, unit: 'lb' } }, - }), + } as unknown as string, notes: null, created_at: now, updated_at: now, @@ -311,6 +311,7 @@ describe('Exercise operations', () => { primary: ['QUADS', 'GLUTES'], secondary: ['HAMSTRINGS', 'CORE'], }) + expect(result[0].supports1RM).toBe(true) }) it('passes category filter to query builder', async () => { @@ -1146,6 +1147,57 @@ describe('Program operations', () => { expect(result!.blockWeeks).toEqual([]) expect(result!.scheduledSessions).toEqual([]) }) + + it('falls back to undefined overrides when overrides JSON is malformed', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const badRow = { ...scheduledSessionRow, overrides: '{not valid json' } + mockClient.mockResponse('programs', 'select', [programRow]) + mockClient.mockResponse('blocks', 'select', [blockRow]) + mockClient.mockResponse('block_weeks', 'select', [blockWeekRow]) + mockClient.mockResponse('scheduled_sessions', 'select', [badRow]) + + const result = await adapter.getProgramFull('prog-001') + + expect(result!.scheduledSessions[0].overrides).toBeUndefined() + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('[supabase-adapter]'), + expect.anything(), + ) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('ss-001'), expect.anything()) + } finally { + warnSpy.mockRestore() + } + }) + + it('parses overrides when stored as a JSON string (Tauri/SQLite path)', async () => { + const overridesObj = { activityOverrides: { 'act-001': { exerciseId: 'ex-002' } } } + const row = { ...scheduledSessionRow, overrides: JSON.stringify(overridesObj) } + mockClient.mockResponse('programs', 'select', [programRow]) + mockClient.mockResponse('blocks', 'select', [blockRow]) + mockClient.mockResponse('block_weeks', 'select', [blockWeekRow]) + mockClient.mockResponse('scheduled_sessions', 'select', [row]) + + const result = await adapter.getProgramFull('prog-001') + + expect(result!.scheduledSessions[0].overrides).toEqual(overridesObj) + }) + + it('passes through overrides when already a parsed object (PostgREST path)', async () => { + const overridesObj = { activityOverrides: { 'act-001': { exerciseId: 'ex-002' } } } + const row = { + ...scheduledSessionRow, + overrides: overridesObj as unknown as string, + } + mockClient.mockResponse('programs', 'select', [programRow]) + mockClient.mockResponse('blocks', 'select', [blockRow]) + mockClient.mockResponse('block_weeks', 'select', [blockWeekRow]) + mockClient.mockResponse('scheduled_sessions', 'select', [row]) + + const result = await adapter.getProgramFull('prog-001') + + expect(result!.scheduledSessions[0].overrides).toEqual(overridesObj) + }) }) describe('createProgramFull', () => { diff --git a/src/lib/supabase-adapter.ts b/src/lib/supabase-adapter.ts index fc067b0..3d60e42 100644 --- a/src/lib/supabase-adapter.ts +++ b/src/lib/supabase-adapter.ts @@ -484,13 +484,20 @@ export class SupabaseAdapter implements DataAdapter { let overrides: ScheduledSession['overrides'] if (r.overrides != null) { try { - const parsed = typeof r.overrides === 'string' ? JSON.parse(r.overrides) : r.overrides + const parsed = parseJsonOrValue(r.overrides as string | object, 'overrides') overrides = sessionOverridesSchema.parse(parsed) } catch (err) { - console.warn( - `[supabase-adapter] Failed to parse overrides for scheduled session ${r.id as string}, falling back to undefined:`, - err, - ) + if (err instanceof z.ZodError) { + console.error( + `[supabase-adapter] overrides schema validation failed for session ${r.id as string}. Data integrity issue:`, + err, + ) + } else { + console.warn( + `[supabase-adapter] Failed to parse overrides JSON for session ${r.id as string}, falling back to undefined:`, + err, + ) + } overrides = undefined } } @@ -505,9 +512,7 @@ export class SupabaseAdapter implements DataAdapter { overrides, } } catch (err) { - throw new Error( - `Failed to map scheduled session (${r.id as string}): ${err instanceof Error ? err.message : String(err)}`, - ) + throw new Error(`Failed to map scheduled session (${r.id as string})`, { cause: err }) } }