fix: propagate stopAtTop and repCountTiming across exercise transitions (#689) - #690
Conversation
Three WorkoutParameters.copy() transition sites in autoplay omitted stopAtTop and repCountTiming, causing per-exercise 'end at the top' and rep count timing settings to leak from one exercise to all subsequent exercises in a routine. Added explicit propagation from the next RoutineExercise at: - DefaultWorkoutSessionManager.proceedFromSummary (autoplay OFF path) - ActiveSessionEngine.startRestTimer (rest timer → next set) - ActiveSessionEngine.startNextSetOrExercise (autoplay ON path) Fixes #689
There was a problem hiding this comment.
Pull request overview
Fixes per-exercise stopAtTop and repCountTiming leaking across exercise boundaries in autoplay routines by explicitly copying those fields from the upcoming RoutineExercise during cross-exercise transitions.
Changes:
- Propagates
stopAtTopandrepCountTimingwhen advancing from Set Summary to the next exercise (autoplay OFF path). - Propagates
stopAtTopandrepCountTimingwhen seeding parameters for the next set/exercise during rest and when advancing after rest (autoplay ON paths).
Verdict
Request changes — correctness looks good, but this regression needs an automated test to prevent reintroduction.
Correctness / Safety Findings
important: shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/DefaultWorkoutSessionManager.kt:L940: Regression-prone transition bug fixed without a regression test. Required fix: add a routine-flow test that advances across exercises (autoplay ON and/or OFF) and asserts WorkoutParameters.stopAtTop and .repCountTiming match the next RoutineExercise.
minor: shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:L4446: Redundant warmupReps bodyweight conditional in a branch that only runs for non-bodyweight. Fix: simplify to Constants.DEFAULT_WARMUP_REPS (optional cleanup).
Ponytail Review
shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:L4446: shrink — redundant warmupReps conditional (guard already ensures non-bodyweight). Replace with Constants.DEFAULT_WARMUP_REPS.
Ponytail net: 0 lines.
Suggested Minimal Patch
- Add a focused regression test in existing routine-flow coverage (e.g.,
DWSMRoutineFlowTestor similar) that:- Builds a routine with at least two exercises where
stopAtTop/repCountTimingdiffer, - Advances from exercise 0 → exercise 1 via the relevant transition path(s),
- Asserts the coordinator’s
workoutParameters.stopAtTopandrepCountTimingupdate to exercise 1’s values.
- Builds a routine with at least two exercises where
- (Optional) apply the one-line
warmupRepssimplification inActiveSessionEngine.kt.
Final Merge Guidance
Can merge after the regression test is added.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/DefaultWorkoutSessionManager.kt | Copies per-exercise stopAtTop/repCountTiming when proceeding to the next exercise from summary (autoplay OFF path). |
| shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt | Copies per-exercise stopAtTop/repCountTiming when seeding/advancing to the next set or exercise during/after rest (autoplay ON paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ropagation Issue #689: Adds a focused regression test that verifies proceedFromSummary() propagates stopAtTop and repCountTiming from the next RoutineExercise when transitioning between exercises (autoplay OFF path). Without the fix from commit 8dc3fcf, the first exercise's values would leak to all subsequent exercises. Addresses Copilot review feedback on PR #690.
Code Review Roast 🔥Verdict: No New Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I had my flamethrower warmed up and everything, and then I read the diff — six added lines, three sites, same field copy in each. Whoever wrote this clearly lost a fight with Overview
No new inline comments — the two concerns worth raising (the redundant One thing I did want to flag for the summary: commit 🏆 Best part: The fix is exactly as wide as the bug — two fields, three sites, six lines. No new helpers, no new abstractions, no "while we're in here." This is what a bugfix looks like when the author respects the codebase. 💀 Worst part: Replacing the original 📊 Overall: Like a well-trimmed hedge — small, intentional, and only the branches you actually needed to cut. Just don't tell anyone you got the test coverage wrong; nobody will notice unless they read the commit history. Files Reviewed (3 files)
Ponytail: Lean already. Ship. Suggested Minimal Patch: None required. Optional: keep the existing Final Merge Guidance: Can merge as-is. The production fix is correct at all three transition sites and the existing Copilot comments already cover the maintainability concerns. Reviewed by minimax-m3 · Input: 55.2K · Output: 9.5K · Cached: 510.7K Review guidance: REVIEW.md from base branch |
Summary
Fixes per-exercise
stopAtTopandrepCountTimingsettings leaking across exercise boundaries during autoplay routines. Previously, only the first exercise's values were preserved; all subsequent exercises inherited them.Root Cause
Three
WorkoutParameters.copy()transition sites during autoplay omittedstopAtTopandrepCountTiming, causing the Kotlincopy()default behavior (retain previous value) to persist stale per-exercise settings across exercise boundaries.Changes
Added explicit propagation of
stopAtTopandrepCountTimingfrom the nextRoutineExerciseat all three cross-exercise transition sites:DefaultWorkoutSessionManager.proceedFromSummary— autoplay OFF path, transitions from summary to next exercise's set-readyActiveSessionEngine.startRestTimer— rest timer path, seeds params for the next set/exercise during restActiveSessionEngine.startNextSetOrExercise— autoplay ON path, advances to next set or exercise after restVerification
RoutineExercisedata class (line 79 ofRoutine.kt) has bothstopAtTop: BooleanandrepCountTiming: RepCountTimingfieldsRoutineFlowManager.kt(lines 845/850, 1107-1108, 1182-1183, 1427-1428) already correctly set these fromRoutineExerciseon initial workout setup — the bug was only in transition copy sitesFixes #689