diff --git a/Context/Backlog/Ideas.md b/Context/Backlog/Ideas.md index bc2e205..760113c 100644 --- a/Context/Backlog/Ideas.md +++ b/Context/Backlog/Ideas.md @@ -5,6 +5,14 @@ skill. Prioritize via the backlog-prioritize skill. +## Move view-local modal state into StrengthWorkoutView (P22-014) + +**Added:** 2026-04-20 +**Source:** PR #115 review finding P22-014 +**Priority:** Low + +`showAddExercise`, `showDiscardDialog`, `pendingInputs`, and `restMinimized` (4 `useState` calls) are used exclusively inside `StrengthWorkoutView` but are held in the parent route because the extraction was a pure mechanical lift. Moving these into the component cuts the prop interface from ~44 to ~36 props and removes shared-state ownership with no behavioral change. Out of scope for the extraction PR -- do as a standalone refactor. + ## ~~P17 Review: Adapter test coverage gaps (PR #107)~~ (Resolved 2026-04-15) **Added:** 2026-04-12 diff --git a/Context/Decisions/ADR-022-strength-view-mutation-orchestration-boundary.md b/Context/Decisions/ADR-022-strength-view-mutation-orchestration-boundary.md new file mode 100644 index 0000000..4b135e0 --- /dev/null +++ b/Context/Decisions/ADR-022-strength-view-mutation-orchestration-boundary.md @@ -0,0 +1,54 @@ +# ADR-022: Mutation Orchestration Boundary in StrengthWorkoutView + +**Date:** 2026-04-20 +**Status:** Proposed +**Context:** PR #115 review finding P22-015 + +--- + +## Context + +After extracting `StrengthWorkoutView` from `log.$workoutId.tsx`, the component receives both a raw hook operation (`confirmSet`) and a parent-wrapped handler (`handleConfirmSet`). The two are not interchangeable: + +- `handleConfirmSet` (strength sets): calls `setRestMinimized(false)` before confirming, normalizes raw string inputs via `parseNumericInput`, then calls `confirmSet` +- `confirmSet` (circuit, cardio, ruck sets): called directly, bypassing rest-timer minimization and input normalization + +This means the same physical action -- completing a set -- takes two different code paths depending on modality. The inconsistency is invisible from the component's prop interface. + +## Decision Needed + +Where does mutation orchestration belong: in the route or in the view? + +### Option A: Orchestration stays in the route (current state) + +The route wraps `confirmSet` into modality-specific handlers (`handleConfirmSet`, and in future `handleCardioConfirmSet`, etc.) and passes each to the view. The view is a pure presentation component that calls whatever handler it receives. + +**Pros:** View has no orchestration logic; route owns all side effects. +**Cons:** Props proliferate as modalities grow; view cannot change its own orchestration; the "two paths" bug can silently re-emerge on new modalities. + +### Option B: Orchestration moves into the view + +The view receives only `confirmSet` (raw) plus the data it needs (`setRestMinimized`). The view owns the normalization and side-effect logic per modality. + +**Pros:** All set-completion paths live together; adding a modality cannot silently bypass orchestration. +**Cons:** View has business logic; harder to test orchestration in isolation. + +### Option C: Extract a per-modality orchestration hook + +A `useSetConfirmation` hook (or similar) wraps `confirmSet` and `setRestMinimized`, encapsulates normalization, and returns modality-aware handlers. Both the route and view call into the hook rather than each other. + +**Pros:** Clean separation; testable; view and route stay thin. +**Cons:** Additional abstraction layer; only justified if orchestration complexity grows. + +## Recommendation + +**Option B** is the right default for current complexity: move `handleConfirmSet` logic into the view so all set-completion paths are colocated. Revisit Option C if a third or fourth modality requires materially different orchestration. + +This decision should be resolved before the next feature that adds a new workout modality or modifies the set-confirmation flow, to prevent the inconsistency from hardening further. + +## Consequences + +- `handleConfirmSet` is removed from the route's prop set passed to `StrengthWorkoutView` +- `setRestMinimized` remains a prop (used for rest-timer minimization side effect) +- Normalization logic (`parseNumericInput`) moves into the view's strength-set path +- The `confirmSet` prop type may need `exerciseName?: string` threading for the strength path diff --git a/Context/QuickPlans/Done/2026-04-20-split-log-workout-route.md b/Context/QuickPlans/Done/2026-04-20-split-log-workout-route.md new file mode 100644 index 0000000..40ceac2 --- /dev/null +++ b/Context/QuickPlans/Done/2026-04-20-split-log-workout-route.md @@ -0,0 +1,88 @@ +# Quick Plan: Split log.$workoutId Route into Three Components + +**Task:** Refactor `src/routes/_authenticated/log.$workoutId.tsx` (~914 lines) by extracting its two non-summary rendering paths into dedicated files. + +**Goal:** Reduce the route file to a thin orchestrator that owns shared state/hooks and delegates rendering to `EventWorkoutView`, `StrengthWorkoutView`, and (already-extracted) `WorkoutSummary`. No behavior changes. + +--- + +## Current Structure + +`ActiveWorkoutPage` contains three rendering paths via conditional branches: + +| Path | Trigger | Lines | +|------|---------|-------| +| Summary | `showSummary && summaryData?.workoutLog` | 485-497 | +| Event workout | `workoutLog.eventMetadata` | 509-567 | +| Strength/cardio/ruck | default return | 569-912 | + +`WorkoutSummary` is already its own component. Two new files needed. + +--- + +## Approach + +### Step 1: Extract `EventWorkoutView` + +Create `src/components/workout/event-workout-view.tsx`. + +Props it receives from the orchestrator: +- `workoutLog` (id, eventMetadata) +- `elapsedSeconds`, `isPauseSupported`, `isPaused` +- `handlePause`, `handleResume`, `handleFinish`, `handleDiscard` +- `isBroadcasting`, `publishFocus`, `publishUnfocus` +- `isFinishing`, `isDiscarding` +- `pageError`, `setPageError` +- `showDiscardDialog`, `setShowDiscardDialog` + +Move lines 509-567 verbatim. Define a `EventWorkoutViewProps` interface. + +### Step 2: Extract `StrengthWorkoutView` + +Create `src/components/workout/strength-workout-view.tsx`. + +Props it receives (all remaining state and handlers used in lines 569-912). Define a `StrengthWorkoutViewProps` interface. + +Key state threads to include: +- All exercise-block rendering logic (lines 636-851) +- Rest view / rest timer banner +- Add exercise sheet + discard dialog +- Footer button + onboarding hint +- Undo banner + +### Step 3: Slim the route file + +`log.$workoutId.tsx` becomes: +1. Route definition +2. All hooks + state declarations (unchanged) +3. Timer effect (unchanged) +4. Computed values and handlers (unchanged) +5. Three-branch conditional returning `WorkoutSummary`, `EventWorkoutView`, or `StrengthWorkoutView` + +Target size: ~200-250 lines (down from ~914). + +--- + +## File Changes + +| File | Change | +|------|--------| +| `src/routes/_authenticated/log.$workoutId.tsx` | Remove extracted JSX, import two new components | +| `src/components/workout/event-workout-view.tsx` | New file: EventWorkoutView component | +| `src/components/workout/strength-workout-view.tsx` | New file: StrengthWorkoutView component | + +--- + +## Verification + +- `bun run build` passes (TypeScript clean) +- `bun run test` passes +- Manual smoke: start a strength workout, start an event workout, finish to summary -- all three paths render correctly +- No behavior change: all state, handlers, and effects remain in the route component + +--- + +## Risks + +- Props list for `StrengthWorkoutView` is large (~20 props). This is acceptable since it's a view component, not an abstraction; the state stays hoisted in the route. If prop count feels unwieldy, consider a single context-bag object type. +- The `SetType` and `LoggedActivityGroupWithActivities` imports move into the new files; verify they re-export correctly from their domain modules. diff --git a/Context/Reviews/0022-pr115-split-log-workout-route-2026-04-20.md b/Context/Reviews/0022-pr115-split-log-workout-route-2026-04-20.md new file mode 100644 index 0000000..08f6d2d --- /dev/null +++ b/Context/Reviews/0022-pr115-split-log-workout-route-2026-04-20.md @@ -0,0 +1,177 @@ +# PR Review: refactor/split-log-workout-route → main + +**Date:** 2026-04-20 +**Feature:** N/A (pure extraction refactor, no feature spec) +**Branch:** refactor/split-log-workout-route +**Reviewers:** pr-review-toolkit (code-reviewer, silent-failure-hunter, type-design-analyzer) +**Status:** ✅ Resolved + +## Summary + +15 findings total across 3 files (`event-workout-view.tsx`, `strength-workout-view.tsx`, +`log.$workoutId.tsx`). The extraction is mechanically faithful — no logic leaked into views, +no Iron & Ember regressions, no silent-import drift. Primary issues are error handling gaps +carried forward from the original file (including one data-loss path in circuit completion), +log prefix inconsistencies introduced by the split, and type drift on the `confirmSet` prop. +One architectural concern about dual mutation paths and one follow-up task for state ownership. + +**Breakdown:** 13 [FIX] · 1 [TASK] · 1 [ADR] + +--- + +## Findings + +### Fix-Now + +#### [FIX] P22-001: `computeElapsed` catch logs but never calls `setPageError` +- **File:** `src/routes/_authenticated/log.$workoutId.tsx:208-211` +- **Severity:** Critical +- **Detail:** The catch block returns `0` and logs the error but does not call `setPageError`. If `startedAt` is corrupt, this fires on every interval tick and the user sees a frozen 00:00 timer with no explanation. The `Number.isFinite` guard above it (which does call `setPageError`) only catches the non-finite case — a JS exception in `computeElapsed` takes a separate code path. +- **Fix:** Add `setPageError('Workout timer data is corrupt. Please discard or reload.')` inside the catch before `return 0`. +- **Status:** ✅ Fixed +- **Resolution:** Added `setPageError('Workout timer data is corrupt. Please discard or reload.')` inside the catch before `return 0`. + +#### [FIX] P22-002: Silent data loss in circuit `onExerciseDone` guard +- **File:** `src/components/workout/strength-workout-view.tsx:227-228` +- **Severity:** Critical +- **Detail:** `if (!activity) return` when `exerciseIndex` is out of bounds silently drops the circuit rep. The user has no indication the save was skipped — their workout data is silently incomplete. Must log with `[strength-workout-view]` prefix and call `setPageError`. +- **Fix:** + ```ts + if (!activity) { + console.error('[strength-workout-view] onExerciseDone: no activity at index', { exerciseIndex, round, groupId: group.id }) + setPageError('Failed to record circuit set. Please log it manually.') + return + } + ``` +- **Status:** ✅ Fixed +- **Resolution:** Added error log and `setPageError` call before returning; guard now surfaces the failure. + +#### [FIX] P22-003: Bare `return null` guard renders blank screen +- **File:** `src/routes/_authenticated/log.$workoutId.tsx:480` +- **Severity:** High +- **Detail:** `if (!workoutLog) return null` renders a completely blank screen with no log entry and no user message. The `useEffect` redirect fires after the render, leaving a visible void window. Per error-handling conventions, guard clauses in rendering paths that can produce user-visible failure states must log. +- **Fix:** Replace with a minimal logged indicator: `console.error('[workout-page] Rendered without active workoutLog'); return
` +- **Status:** ✅ Fixed +- **Resolution:** Replaced `return null` with `console.error` + minimal surface div ``. + +#### [FIX] P22-004: `handleMarkDone` and `handleExpandDone` — silent user-action bare returns +- **File:** `src/routes/_authenticated/log.$workoutId.tsx:441-456` +- **Severity:** High +- **Detail:** Both handlers accept `activityId: string` with no guard. If `activityId` is empty or stale, `skipActivity` and `setExpandedDoneActivityIds` mutate silently with no log and no user feedback. Per error-handling conventions, user-action handlers must never silently return. +- **Fix:** Add `if (!activityId) { console.error('[workout-page] handleMarkDone called with empty activityId'); setPageError('Could not mark exercise done. Please reload.'); return }` at entry. +- **Status:** ✅ Fixed +- **Resolution:** Both handlers guard `!activityId` with `console.error` + `setPageError` before proceeding. + +#### [FIX] P22-005: `confirmSet` prop drops `exerciseName?: string` 4th parameter +- **File:** `src/components/workout/strength-workout-view.tsx:84-88` +- **Severity:** High +- **Detail:** The hook signature is `(loggedActivityId, setData, restSeconds?, exerciseName?)` but the prop interface declares only 3 parameters. Any future caller inside the view that needs to forward `exerciseName` cannot — the type silently truncates it. The return type is also widened to `Promise+ ALL EXERCISES DONE -- READY TO FINISH? +
+ )} + {loggedGroups.length === 0 && !firstWorkoutCompleted && ( +- ALL EXERCISES DONE -- READY TO FINISH? -
- )} - {loggedGroups.length === 0 && !firstWorkoutCompleted && ( -