Fix/689 stop at top per exercise leak - #692
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
…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.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * stopAtTop and repCountTiming values with the next RoutineExercise's values. | ||
| */ | ||
| @Test | ||
| fun startNextSet_propagatesPerExerciseStopAtTopAndRepCountTiming() = runTest { |
There was a problem hiding this comment.
🔥 The Roast: Commit 3 "autoplay coverage" is actually a bait-and-switch — it removed the proceedFromSummary regression test from commit 2 and replaced it with a startNextSet test that exercises the same startNextSetOrExercise copy block. Net regression coverage of #689 is unchanged: exactly one of three fixed transition sites is defended. The other two — DWSM.proceedFromSummary (line 940) and ActiveSessionEngine.startRestTimer (line 4447) — are now running naked through production with nothing but the author's good intentions protecting them. If someone reverts just one of those two lines in three months, CI will smile and ship a regression right back into users' workouts.
🩹 The Fix: Keep this test (it covers the autoplay-ON path). Then add a second test that drives the proceedFromSummary site (call harness.dwsm.proceedFromSummary() while WorkoutState.SetSummary is active with _currentSetIndex at the last set) and asserts the next exercise's stopAtTop/repCountTiming propagate. That's the minimal addition to put regression bars back on all three sites named in the PR description.
The startRestTimer site is harder to drive from a unit harness, so an integration-level test is acceptable there — but at minimum the proceedFromSummary site deserves a direct regression test since commit 2 had one and commit 3 silently deleted it.
📏 Severity: warning
Code Review Roast 🔥Verdict: Warning Found | Recommendation: Add regression test for Overview
Issue Details (click to expand)
🏆 Best part: The fix at line 4855 of 💀 Worst part: The PR description names three sites; the post-commit-3 test covers one of them — and not even a new one, just a swap. Net regression coverage of #689 is exactly what it was before commit 2 added a test: nil for two of three sites. The commit message "cover autoplay per-exercise timing transition" is doing a lot of verbal heavy lifting that the diff doesn't back up. 📊 Overall: The bug fix itself is tight and correct — six lines, three sites, two fields, matching the established project pattern. The test churn across commits 2 and 3 is what drags this from "clean fix" to "needs one more test before it ships." Add the Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 48.6K · Output: 8K · Cached: 415.6K Review guidance: REVIEW.md from base branch |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 614b18ed0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assertEquals(RepCountTiming.TOP, harness.dwsm.coordinator.workoutParameters.value.repCountTiming) | ||
|
|
||
| // Simulate a completed rest countdown advancing from exercise 0 to exercise 1. | ||
| harness.setActiveSummaryCountdownSeconds(0) |
There was a problem hiding this comment.
Setting summaryCountdownSeconds to 0 disables autoplay (SettingsManager.autoplayEnabled is derived from summaryCountdownSeconds != 0). Consequently, startNextSet() follows the manual enterSetReady path, which already copies stopAtTop and repCountTiming in RoutineFlowManager; this test therefore still passes if the new autoplay propagation in ActiveSessionEngine is removed. Use a nonzero countdown, as the existing autoplay tests do, so this regression test actually exercises and protects the changed production path.
Useful? React with 👍 / 👎.
|
@codex Address all comments |
Summary
Testing
|
No description provided.