fix: propagate stopAtTop and repCountTiming across exercise transitions (#689)

[9thLevelSoftware]

Summary

Fixes per-exercise stopAtTop and repCountTiming settings 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 omitted stopAtTop and repCountTiming, causing the Kotlin copy() default behavior (retain previous value) to persist stale per-exercise settings across exercise boundaries.

Changes

Added explicit propagation of stopAtTop and repCountTiming from the next RoutineExercise at all three cross-exercise transition sites:

1. DefaultWorkoutSessionManager.proceedFromSummary — autoplay OFF path, transitions from summary to next exercise's set-ready
2. ActiveSessionEngine.startRestTimer — rest timer path, seeds params for the next set/exercise during rest
3. ActiveSessionEngine.startNextSetOrExercise — autoplay ON path, advances to next set or exercise after rest

Verification

• RoutineExercise data class (line 79 of Routine.kt) has both stopAtTop: Boolean and repCountTiming: RepCountTiming fields
• Seeding paths in RoutineFlowManager.kt (lines 845/850, 1107-1108, 1182-1183, 1427-1428) already correctly set these from RoutineExercise on initial workout setup — the bug was only in transition copy sites
• No schema, sync, portal, UI, or migration impact

Fixes #689

[Copilot]

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 stopAtTop and repCountTiming when advancing from Set Summary to the next exercise (autoplay OFF path).
• Propagates stopAtTop and repCountTiming when 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., DWSMRoutineFlowTest or similar) that:
  • Builds a routine with at least two exercises where stopAtTop/repCountTiming differ,
  • Advances from exercise 0 → exercise 1 via the relevant transition path(s),
  • Asserts the coordinator’s workoutParameters.stopAtTop and repCountTiming update to exercise 1’s values.
• (Optional) apply the one-line warmupReps simplification in ActiveSessionEngine.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.

[kilo-code-bot]

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 WorkoutParameters.copy() defaults once and decided the rest of us shouldn't have to.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

No new inline comments — the two concerns worth raising (the redundant warmupReps bodyweight conditional and the missing regression test for the autoplay OFF path) were already raised by Copilot at ActiveSessionEngine.kt:4446 and DefaultWorkoutSessionManager.kt:941. The author replied to both. I'm not going to shout into a conversation that's already happening.

One thing I did want to flag for the summary: commit 614b18ed (the third one) replaced the proceedFromSummary_… test from commit 7b2d286 with a startNextSet_… test. Net effect: the autoplay OFF path (proceedFromSummary) no longer has a dedicated regression test — only startNextSetOrExercise (autoplay ON) does, and startRestTimer is exercised indirectly. The fix at all three sites is still correct, but coverage is now narrower than what the PR description implies. Cheap fix if you want it: keep the renamed test and add the original proceedFromSummary test back alongside it.

🏆 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 proceedFromSummary test instead of adding a second one. The first commit added a test for one path, the third commit pivoted to a different path, and now the autoplay OFF transition that the PR description explicitly calls out (DefaultWorkoutSessionManager.proceedFromSummary) has no dedicated regression coverage. Easy to miss because the test count stayed the same.

📊 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)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt - 0 new issues (existing Copilot comment at line 4446 acknowledged as out-of-scope)
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/DefaultWorkoutSessionManager.kt - 0 new issues (existing Copilot comment at line 941 acknowledged; test gap noted in summary)
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/DWSMRoutineFlowTest.kt - 0 new issues (replaces prior test rather than extending coverage)

---

Ponytail: Lean already. Ship.
Ponytail net: 0 lines.

Suggested Minimal Patch: None required. Optional: keep the existing startNextSet_… test and re-add the original proceedFromSummary_… test alongside it for direct coverage of the autoplay OFF transition.

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 main}