Fix stall detection to allow failure detection on first working rep#260
Conversation
The shouldDeferStandardSetStall guard was unconditionally deferring stall detection when workingReps == 0, leaving users unprotected if they stalled on their very first working rep (e.g., failed bench press at TOP). Now defers only when workingReps == 0 AND no rep is pending, so a pending first rep correctly triggers stall detection. Tests updated to simulate the first-rep stall scenario directly instead of bypassing it via a completed first rep. https://claude.ai/code/session_017rGcHJxzHsrPfr6sJLLruP
|
@9thLevelSoftware I've opened a new pull request, #261, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Fixes stall detection so failures during the first working rep can be detected (e.g., stalled at TOP), while still avoiding false positives during brief pauses.
Changes:
- Update standard-set stall detection deferral to consider whether a rep is currently pending.
- Clarify intended failure scenario behavior in stall guard documentation.
- Adjust tests to simulate first-working-rep failure scenarios without completing the first rep.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/DWSMWorkoutLifecycleTest.kt | Updates test setup/counters to keep first working rep incomplete when simulating stall/failure. |
| shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt | Tightens stall deferral logic to only defer when no working reps are confirmed and no rep is pending. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * - Defer stall detection only when no working reps are confirmed AND no rep is pending. | ||
| * | ||
| * Issue #256: Removed hasPendingRep guard. Previously, stall detection was deferred | ||
| * while a rep was pending at TOP (to avoid false triggers during brief lockout pauses). | ||
| * However, a stalled pending rep IS the failure scenario (e.g., failed bench press). | ||
| * The velocity hysteresis band (STALL_VELOCITY_LOW=2.5, STALL_VELOCITY_HIGH=10.0 mm/s) | ||
| * already provides adequate protection against false triggers during brief pauses. | ||
| * Issue #256: Removed hasPendingRep guard from deferral. A stalled pending rep IS the | ||
| * failure scenario (e.g., failed bench press at TOP of first rep). The velocity hysteresis | ||
| * band (STALL_VELOCITY_LOW=2.5, STALL_VELOCITY_HIGH=10.0 mm/s) already provides adequate | ||
| * protection against false triggers during brief pauses. When workingReps == 0 but | ||
| * hasPendingRep is true, the user is mid-first-rep and must be protected. | ||
| */ | ||
| private fun shouldDeferStandardSetStall(params: WorkoutParameters, repCount: RepCount): Boolean { | ||
| val isStandardSet = !params.isJustLift && !params.isAMRAP && !coordinator.isCurrentTimedCableExercise | ||
| if (!isStandardSet) return false | ||
| return repCount.workingReps == 0 | ||
| return repCount.workingReps == 0 && !repCount.hasPendingRep | ||
| } |
There was a problem hiding this comment.
The KDoc contradicts the implementation: it states the hasPendingRep guard was removed from deferral, but the updated logic explicitly uses !repCount.hasPendingRep to decide deferral. Please rewrite the Issue #256 note to reflect the actual change (i.e., deferral was narrowed to only when no rep is pending), and clarify what 'must be protected' means here (protected from deferral vs. protected from false positives) to avoid confusion for future maintainers.
| // Simulate pending first rep (stalled mid-concentric) | ||
| harness.fakeBleRepo.emitRepNotification( | ||
| RepNotification( | ||
| topCounter = 5, | ||
| completeCounter = 4, | ||
| topCounter = 4, | ||
| completeCounter = 3, | ||
| repsRomCount = 3, | ||
| repsRomTotal = 3, | ||
| repsSetCount = 1, | ||
| repsSetCount = 0, |
There was a problem hiding this comment.
The test comment says 'stalled mid-concentric' but the event fields use topCounter/completeCounter values that appear to represent a rep that has reached TOP but is not completed (pending at TOP). Please align the comment with what the counters represent in this test scenario (or adjust the counters to reflect a true mid-concentric stall if that’s the intent) so the test remains self-explanatory.
Summary
This PR fixes stall detection logic to properly detect failures (e.g., failed bench press) that occur during the first working rep, while still protecting against false positives during brief lockout pauses.
Key Changes
shouldDeferStandardSetStall()to only defer stall detection whenworkingReps == 0 AND !hasPendingRep. Previously, it deferred wheneverworkingReps == 0, which prevented failure detection when a rep was pending at the top of the first rep.completeFirstWorkingRep()and adjusted test expectations to reflect that the first working rep should remain incomplete when simulating failure scenarios.Implementation Details
hasPendingRepguard unnecessary for deferral.workingReps == 0buthasPendingRep == true, the user is mid-first-rep and must be protected from stall detection deferral to allow proper failure detection.https://claude.ai/code/session_017rGcHJxzHsrPfr6sJLLruP