Skip to content

fix(workout): snap timers to wall clock on mobile screen wake #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rghamilton3 merged 4 commits into from
Apr 23, 2026
Merged

fix(workout): snap timers to wall clock on mobile screen wake #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a99f84
fix(workout): snap timers to wall clock on mobile screen wake
rghamilton3 Apr 23, 2026
96c38f6
fix(build): remove unused makeExercise fixture and Exercise import
rghamilton3 Apr 23, 2026
b8bed6b
fix(review): resolve P23 review findings (11 fixes, 2 tests)
rghamilton3 Apr 23, 2026
1a614a9
fix(review): verify P23 review -- fix recalcRestTimer interface comment
rghamilton3 Apr 23, 2026
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
228 changes: 228 additions & 0 deletions Context/Reviews/0022-since-beta10-2026-04-22.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
# PR Review: Changes Since v1.0.0-beta.10

**Date:** 2026-04-22
**Scope:** All changes since tag `v1.0.0-beta.10` (24 commits, 119 source files)
**Branch:** main
**Reviewers:** code-reviewer, silent-failure-hunter, pr-test-analyzer, type-design-analyzer (automated agents)
**Status:** 🟡 Partially resolved

## Summary

14 findings total: 9 [FIX], 3 [TASK], 0 [ADR], 0 [RULE]. 3 critical, 6 high, 5 medium/low.
Coverage spans F021 (gym membership), F022 (browser notifications), F023 (exercise
completion), F024 (frequent exercises), display-realtime collapse, data-mapper elimination,
workout view split, and set UX overhaul. Test suite quality is notably strong in most areas;
the main gap is `StrengthWorkoutView` (new finish guard has zero tests) and `EventWorkoutView`
(new component, no test file).

---

## Findings

### Fix-Now

#### [FIX] P22-001: `getFrequentExerciseIds` swallows RPC error with `return []`
- **File:** `src/lib/supabase-adapter.ts`
- **Severity:** Critical
- **Detail:** On RPC failure the function logs and returns `[]`, making a network error
indistinguishable from "user has no frequent exercises yet." TanStack Query reports
`isSuccess: true` with empty data; users who have frequent exercises see nothing and
receive no indication that something went wrong. Fix: `throw error` after logging so
`isError` propagates through the normal query error flow.
- **Status:** ✅ Fixed
- **Resolution:** Changed `return []` to `throw error` after logging. Updated test to assert rejection instead of empty return.

#### [FIX] P22-002: `useGymRoster` null-client branch returns partial data inside `queryFn`
- **File:** `src/hooks/use-gym-members.ts`
- **Severity:** Critical
- **Detail:** When the Supabase client is null, the `queryFn` returns augmented member
objects with `displayName: null` instead of throwing. `useQuery` reports `isSuccess: true`
with null display names on every roster member. Consumers cannot distinguish initialization
failure from "these members have no display names." A programmer-error condition should
surface as `isError`, not as silently degraded data. Fix: throw `new Error('[gym-members]
Supabase client not initialized')` so the error propagates correctly.
- **Status:** ✅ Fixed
- **Resolution:** Replaced the `return sorted.map(...)` fallback with `throw new Error('[gym-members] Supabase client not initialized')`.

#### [FIX] P22-003: Duplicate error logging at 3 sites violates hook log ownership rule
- **File:** `src/components/workout/strength-workout-view.tsx:425-444`, `src/routes/_authenticated/log.$workoutId.tsx:422-427`
- **Severity:** High
- **Detail:** Three catch blocks log the same rejection that the corresponding mutation hook
`onError` already logs: `deleteSet` (strength-workout-view:425), `removeActivity`
(strength-workout-view:438), and `handleUnconfirmSet` (log.$workoutId.tsx:422). This
produces two log lines with different `[module-name]` prefixes per failure, violating the
"Mutation Hook Log Ownership" rule in `.claude/rules/error-handling.md`. Fix: in each
catch block, keep `setPageError(...)` but drop the `console.error(...)` call -- the hook
already owns the log.
- **Status:** ✅ Fixed
- **Resolution:** Removed `console.error` from all three catch blocks; kept `setPageError` calls. Bare `catch {}` used where error param was only used for logging.

#### [FIX] P22-004: `startProgrammedWorkout` store boundary validation removed
- **File:** `src/stores/active-workout-store.ts`
- **Severity:** High
- **Detail:** The guard that threw when `workoutLog.programContext` was absent was removed
without a replacement invariant. The store no longer self-protects against non-programmed
logs being passed to `startProgrammedWorkout`. Per `.claude/rules/state-management.md`,
stores must validate at their own boundary. A future caller passing a non-programmed log
will corrupt workout state with no log or error. Fix: reinstate the `programContext` guard,
logging with `[active-workout]` prefix before returning.
- **Status:** ✅ Deferred -- finding invalid
- **Resolution:** An existing test explicitly documents that `startProgrammedWorkout` accepts logs without `programContext` (standalone template workouts). Adding the guard broke this intentional behavior. No fix applied; the finding conflicts with documented, tested behavior.

#### [FIX] P22-005: `unconfirmSet` guard clause silently returns without user-facing error
- **File:** `src/hooks/use-active-workout.ts`
- **Severity:** High
- **Detail:** When `findSetById` returns null, the handler logs `[workout] unconfirmSet: set
not found` and returns -- no toast, no error state. Per the "User-Action Guard Clauses"
rule in `.claude/rules/error-handling.md`, guard clauses in user-action handlers must
log AND set a user-facing error state. Users who tap "unconfirm" on a stale set ID get
no feedback and cannot tell whether their action registered. Fix: after the `console.error`,
surface an error via the existing page error mechanism or a toast.
- **Status:** ✅ Fixed
- **Resolution:** Changed `return` to `throw new Error('Set not found -- it may have already been removed.')`. The caller's existing catch block in `log.$workoutId.tsx` catches this and calls `setPageError`.

#### [FIX] P22-006: Missing `.refine` cross-field invariants on gym domain types
- **File:** `src/domain/types/gym.ts`
- **Severity:** High
- **Detail:** Three invariants are unenforced at the type level, inconsistent with the
`.refine` pattern already used on `WorkoutLog` (`completedAt > startedAt`):
1. `GymInvitation.usesCount <= maxUses` -- adapter bugs can silently produce over-capacity
invitations.
2. `GymInvitation.expiresAt > createdAt` -- an invite that expired before creation is
representable.
3. `GymOwnershipTransfer.proposedBy !== proposedTo` -- a self-transfer is a nonsensical
business state.
Additionally, `gymInvitationSchema` uses `token: z.string().min(1)` -- align minimum
length with the token generator (likely `min(16)` or UUID-length `min(36)`).
- **Status:** ✅ Fixed
- **Resolution:** Added `.refine` for all three invariants. Token min updated to `min(24)` to match DB constraint `char_length(token) >= 24` from `20260408000002_gym_membership_explicit.sql`.

#### [FIX] P22-007: `addExerciseToWorkout` unused `_exercise` and `_groupType` parameters
- **File:** `src/stores/active-workout-store.ts`
- **Severity:** Low
- **Detail:** Both parameters are prefixed `_` (intentionally unused) but appear on the
public action interface. Future authors reading the interface would believe they affect
store behavior; they do not -- the caller builds `newGroup` and `newActivity` before
calling the action. Fix: remove the unused parameters from the signature or use them
internally for validation/construction.
- **Status:** ✅ Fixed
- **Resolution:** Removed `exercise` and `groupType` from both the interface and implementation. Updated the call site in `use-active-workout.ts` and the test. Removed now-unused `Exercise` and `GroupType` imports from the store.

#### [FIX] P22-008: Notification preference failure silently degrades rest timer
- **File:** `src/hooks/use-active-workout.ts`
- **Severity:** Medium
- **Detail:** When notification preferences fail to load, the rest timer starts without the
`onExpired` callback -- rest-complete browser notifications are silently disabled for that
set. Athletes who rely on rest-complete notifications (e.g., phone face-down between sets)
miss them with no explanation. Fix: add a one-time toast such as "Rest notifications
unavailable -- check notification permissions" when the prefs fetch fails.
- **Status:** ✅ Fixed
- **Resolution:** Added `toast('Rest notifications unavailable -- check notification permissions.')` in the `.catch` block after starting the timer without the callback.

#### [FIX] P22-009: `pendingDirty` not cleared on `handleMarkDone`
- **File:** `src/components/workout/strength-workout-view.tsx`, `src/routes/_authenticated/__tests__/-log-workout-mark-done.test.tsx`
- **Severity:** Medium
- **Detail:** `handleMarkDone` clears `pendingInputs` for a skipped activity but does not
remove the activity's ID from `pendingDirty`. An athlete who edits a pending row and then
marks the exercise done will encounter the "You have unsaved data" finish guard dialog even
though the row is visually gone. The finish guard at line 143 checks `pendingDirty.size > 0`
with no awareness that the activity was skipped. Fix: clear the activity's ID from
`pendingDirty` in `handleMarkDone`, and add a test verifying a subsequent Finish tap
does not show the dirty dialog.
- **Status:** ✅ Fixed
- **Resolution:** Wrapped `onSkipExercise` in `strength-workout-view.tsx` to clear the activity's ID from `pendingDirty` before calling `handleMarkDone`. The regression test is tracked as S008-T in F023/Steps.md.

---

### Missing Tasks

#### [TASK] P22-010: `StrengthWorkoutView` has no test file -- finish guard untested
- **File:** `src/components/workout/strength-workout-view.tsx`
- **Severity:** Critical
- **Detail:** The `feat(workout): guard finish when pending set is dirtied` commit's core
behavior -- `handleFinishWithGuard`, `pendingDirty` accumulation, and clearing on confirm
or delete -- has zero test coverage. No test file exists for this component. Three paths
need explicit tests: (1) no dirty rows → `handleFinish()` called directly; (2) dirty row
present → `showFinishDirtyDialog` shown; (3) dirty row confirmed → subsequent Finish
proceeds without dialog. A regression here silently discards athlete set data.
- **Relates to:** F023 exercise completion signal implementation
- **Status:** ✅ Task created
- **Resolution:** Added as S008-T in Context/Features/023-Exercise-Completion-Signal/Steps.md (Phase 6).

#### [TASK] P22-011: `SetRow.onPendingDirty` prop never tested -- idempotency unverified
- **File:** `src/components/workout/__tests__/set-row.test.tsx`
- **Severity:** High
- **Detail:** `markDirty` fires `onPendingDirty` on first edit of a pending row and is
intentionally idempotent (only calls once per row lifecycle). The existing `SetRow` tests
never pass `onPendingDirty` as a prop and never assert it is called. Needed: (1)
`onPendingDirty` called exactly once on first weight/reps edit of a pending row; (2) not
called on a confirmed (read-only) row; (3) not called a second time on subsequent field
edits. Without these, a loss of the `isDirty` guard would cause per-keystroke state churn
undetected.
- **Relates to:** P22-010 (StrengthWorkoutView finish guard depends on this signal)
- **Status:** ✅ Task created
- **Resolution:** Added as S009-T in Context/Features/023-Exercise-Completion-Signal/Steps.md (Phase 6).

#### [TASK] P22-012: `EventWorkoutView` has no tests
- **File:** `src/components/workout/event-workout-view.tsx`
- **Severity:** High
- **Detail:** New component split from the workout route in this release has no test file.
The discard flow (dialog triggered via `WorkoutPausedBar.onDiscard`, Cancel vs Discard
paths), `canFinish={true}` semantics (always true, unlike `StrengthWorkoutView`'s
set-count gate), and `pageError` / `ErrorBanner` dismiss are all uncovered. Without tests,
a future unification of the two workout views could accidentally gate event workout
finishing behind a set count.
- **Status:** ✅ Task created
- **Resolution:** Added as S010-T in Context/Features/023-Exercise-Completion-Signal/Steps.md (Phase 6).

#### [TASK] P22-013: `allActivitiesDone` mixed CIRCUIT + STRENGTH finish-banner scenario untested
- **File:** `src/routes/_authenticated/__tests__/-log-workout-finish-banner.test.tsx`
- **Severity:** Medium
- **Detail:** The existing test covers CIRCUIT-only (banner shows when only CIRCUIT groups
remain). The mixed case -- one CIRCUIT group with incomplete activities plus one STRENGTH
group fully done -- is not tested. In this scenario `allActivitiesDone` should be `true`
because CIRCUIT groups are excluded from the check. If the exclusion filter regresses,
the "READY TO FINISH?" banner never appears for concurrent-training workouts. Add a test
case with at least one CIRCUIT and one STRENGTH group where all STRENGTH activities are
done.
- **Status:** ✅ Task created
- **Resolution:** Added as S011-T in Context/Features/023-Exercise-Completion-Signal/Steps.md (Phase 6).

---

## Positive Observations

- `active-workout-store-rest-expired.test.ts` is a precise behavioral specification: once-only
callback firing, non-firing on `skipRest`, cleanup/finish/discard no-call guarantees all
verified.
- `display-realtime.test.ts` covers all five reconnect error status paths, exponential
backoff, gym-switch channel teardown, and auto-`publishHello` on reconnect. Boundary
validation tests directly enforce the `state-management.md` module-boundary rule.
- `use-session-reminder-browser.test.ts` covers all six disabling conditions, once-per-day
deduplication, and Tauri no-op path. The asymmetry between rest-timer (not blocked by quiet
hours) and session-reminder (blocked) is explicitly verified.
- `RedeemInviteError` and `GymMemberCount` are well-specified types with no identified gaps.
- Data-mapper elimination is clean: no orphaned consumers, `notes`/`noteTags` removal from
`LoggedSet` is complete with no downstream breakage.

---

## Resolution Checklist

- [x] All [FIX] findings resolved (P22-001 through P22-009)
- [x] All [TASK] findings added to Steps.md or addressed (P22-010 through P22-013)
- [x] Review verified by review-verify agent

---

## Resolution Summary
**Resolved at:** 2026-04-22
**Session:** P22 batch review -- changes since v1.0.0-beta.10

| Category | Total | Resolved | Deferred |
|---|---|---|---|
| [FIX] | 9 | 8 | 1 |
| [TASK] | 4 | 4 | 0 |
| **Total** | **13** | **12** | **1** |

**Deferred:** P22-004 -- finding invalid; existing test documents that `startProgrammedWorkout` intentionally accepts standalone template workouts without `programContext`.
Loading
Loading