Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions Context/Backlog/Bugs.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ High-priority bugs should be addressed before starting new features.

<!-- Add new bugs below this line -->

## 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
Expand Down Expand Up @@ -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)

Expand Down
76 changes: 76 additions & 0 deletions Context/QuickPlans/Done/2026-04-15-b009-overrides-happy-path.md
Original file line number Diff line number Diff line change
@@ -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<string, { exerciseId?: string; setScheme?: SetScheme }> }
```

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.
115 changes: 115 additions & 0 deletions Context/Reviews/0019-pr112-adapter-test-coverage-2026-04-15.md
Original file line number Diff line number Diff line change
@@ -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('<session-id>')` 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** |
70 changes: 70 additions & 0 deletions src/lib/__tests__/adapter-utils.test.ts
Original file line number Diff line number Diff line change
@@ -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\}/)
})
})
60 changes: 56 additions & 4 deletions src/lib/__tests__/supabase-adapter.test.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
Loading
Loading