From d381652e01263140cb7c98ad0da54d12743c9179 Mon Sep 17 00:00:00 2001 From: Devil Date: Mon, 17 Aug 2026 08:56:05 -0400 Subject: [PATCH 1/3] fix: flush _repCount before WORKOUT_COMPLETE handles set completion (#703) When the Nth rep notification triggers WORKOUT_COMPLETE, handleSetCompletion reads coordinator._repCount.value which was still stale (N-1) because the StateFlow write at handleRepNotification line 1334 occurs AFTER repCounter.process() returns. The fix flushes _repCount.value inside the WORKOUT_COMPLETE handler before calling handleSetCompletion, ensuring captureExitSnapshot sees the correct rep count. Also adds a targeted test exercising the full repNotification -> handleRepNotification -> WORKOUT_COMPLETE -> handleSetCompletion -> captureExitSnapshot -> persistence path with reps=7 warmupReps=3. Fixes #703 --- .../manager/ActiveSessionEngine.kt | 6 ++ .../manager/WorkoutExitPersistenceTest.kt | 88 +++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt index 9f0e35058..f072c1a00 100644 --- a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt +++ b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt @@ -530,6 +530,12 @@ class ActiveSessionEngine( // Playing both was causing multiple sounds to fire at once (sound stacking bug). // Issue #182: Trigger set completion immediately on WORKOUT_COMPLETE event. if (executionGuard.isCurrent(eventLease) && coordinator._workoutState.value is WorkoutState.Active) { + // Issue #703: Flush _repCount.value BEFORE handleSetCompletion reads it. + // repCounter.process() has already updated its internal workingReps, + // but coordinator._repCount.value is still stale (updated later at line 1334 + // in handleRepNotification). handleSetCompletion -> captureExitSnapshot reads + // _repCount.value, so we must ensure it reflects the final rep count. + coordinator._repCount.value = repCounter.getRepCount() Logger.d("WORKOUT_COMPLETE event received - triggering immediate set completion") handleSetCompletion(eventLease) } diff --git a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt index cadd3dadd..acea4c459 100644 --- a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt +++ b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt @@ -917,4 +917,92 @@ class WorkoutExitPersistenceTest { velocityB = 130.0, ), ) + + /** + * Issue #703: Rep count off-by-one when ROM reps precede working set. + * + * When the Nth rep notification triggers WORKOUT_COMPLETE, handleSetCompletion + * reads coordinator._repCount.value which was still stale (N-1) because the + * StateFlow write at handleRepNotification line 1334 occurs AFTER repCounter.process() + * returns. The fix flushes _repCount.value inside the WORKOUT_COMPLETE handler + * before calling handleSetCompletion. + */ + @Test + fun `issue 703 - working reps persist correctly when ROM reps precede working set`() = runTest { + val harness = DWSMTestHarness(this) + try { + // Setup: reps=7, warmupReps=3 (matching reporter's scenario) + harness.fakeExerciseRepo.addExercise(TestFixtures.benchPress) + harness.fakeBleRepo.simulateConnect("Vee_Test") + harness.dwsm.updateWorkoutParameters( + WorkoutParameters( + programMode = ProgramMode.OldSchool, + reps = 7, + warmupReps = 3, + weightPerCableKg = 25f, + selectedExerciseId = TestFixtures.benchPress.id, + ), + ) + harness.dwsm.startWorkout(skipCountdown = true) + harness.testScope.testScheduler.advanceUntilIdle() + + val lease = harness.activeSessionEngine.currentExecutionLeaseForTest() + + // Simulate 3 ROM/warmup reps + repeat(3) { romRep -> + harness.fakeBleRepo.emitRepNotification( + harness.repNotification( + repsSetCount = romRep, + timestamp = nowMs() + romRep * 1000L, + repsRomCount = romRep + 1, + repsRomTotal = 3, + ), + ) + harness.testScope.testScheduler.advanceUntilIdle() + } + + // Simulate 7 working reps (repsSetCount 0..6, repsRomCount stays at 3) + repeat(7) { workingRep -> + harness.fakeBleRepo.emitRepNotification( + harness.repNotification( + repsSetCount = workingRep + 1, + timestamp = nowMs() + (3 + workingRep) * 1000L, + repsRomCount = 3, + repsRomTotal = 3, + ), + ) + harness.testScope.testScheduler.advanceUntilIdle() + } + + // Verify the persisted session has workingReps=7, not 6 + val savedSessions = harness.fakeWorkoutRepo.saveSessionAttempts + assertTrue(savedSessions.isNotEmpty(), "Expected at least one saved session") + val lastSession = savedSessions.last() + assertEquals( + 7, + lastSession.workingReps, + "Issue #703: workingReps should be 7, not ${lastSession.workingReps}. " + + "The StateFlow was stale when WORKOUT_COMPLETE fired.", + ) + assertEquals( + 7, + lastSession.totalReps, + "Issue #703: totalReps should be 7", + ) + + // Verify completedSet also has correct rep count + val savedCompletedSets = harness.fakeCompletedSetRepo.saved + assertTrue(savedCompletedSets.isNotEmpty(), "Expected at least one completed set") + val lastCompletedSet = savedCompletedSets.last() + assertEquals( + 7, + lastCompletedSet.actualReps, + "Issue #703: CompletedSet.actualReps should be 7, not ${lastCompletedSet.actualReps}", + ) + } finally { + harness.cleanup() + } + } + + private fun nowMs() = DWSMTestHarness.TEST_WALL_CLOCK_EPOCH_MS } From f77aaeece279e422e893cf4676a0a1e3b2e08063 Mon Sep 17 00:00:00 2001 From: Devil Date: Mon, 17 Aug 2026 09:04:04 -0400 Subject: [PATCH 2/3] fix: use modernRepPacket instead of repNotification in test (#703) --- .../presentation/manager/WorkoutExitPersistenceTest.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt index acea4c459..180b50872 100644 --- a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt +++ b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt @@ -951,8 +951,9 @@ class WorkoutExitPersistenceTest { // Simulate 3 ROM/warmup reps repeat(3) { romRep -> harness.fakeBleRepo.emitRepNotification( - harness.repNotification( + harness.modernRepPacket( repsSetCount = romRep, + repsSetTotal = 7, timestamp = nowMs() + romRep * 1000L, repsRomCount = romRep + 1, repsRomTotal = 3, @@ -964,8 +965,9 @@ class WorkoutExitPersistenceTest { // Simulate 7 working reps (repsSetCount 0..6, repsRomCount stays at 3) repeat(7) { workingRep -> harness.fakeBleRepo.emitRepNotification( - harness.repNotification( + harness.modernRepPacket( repsSetCount = workingRep + 1, + repsSetTotal = 7, timestamp = nowMs() + (3 + workingRep) * 1000L, repsRomCount = 3, repsRomTotal = 3, From 5893a27406bc3a3e8362e86c100ee9e4fc73f70e Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Mon, 17 Aug 2026 09:19:45 -0400 Subject: [PATCH 3/3] fix: address Kilo review suggestions on PR #704 - Drop brittle line-1334 reference from WORKOUT_COMPLETE flush comment in ActiveSessionEngine.kt (the actual write is at line 1340; line numbers rot). - Drop same wrong line reference from the WorkoutExitPersistenceTest KDoc. - Replace the duplicate private nowMs() helper with the existing harness.nowMs from DWSMTestHarness (which advances with the testScheduler), so future tests that care about scheduler-relative timing remain correct. --- .../presentation/manager/ActiveSessionEngine.kt | 7 ++++--- .../presentation/manager/WorkoutExitPersistenceTest.kt | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt index f072c1a00..b61eac138 100644 --- a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt +++ b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt @@ -532,9 +532,10 @@ class ActiveSessionEngine( if (executionGuard.isCurrent(eventLease) && coordinator._workoutState.value is WorkoutState.Active) { // Issue #703: Flush _repCount.value BEFORE handleSetCompletion reads it. // repCounter.process() has already updated its internal workingReps, - // but coordinator._repCount.value is still stale (updated later at line 1334 - // in handleRepNotification). handleSetCompletion -> captureExitSnapshot reads - // _repCount.value, so we must ensure it reflects the final rep count. + // but coordinator._repCount.value is still stale (the StateFlow write in + // handleRepNotification happens after this callback returns). handleSetCompletion + // -> captureExitSnapshot reads _repCount.value, so we must ensure it reflects + // the final rep count before the snapshot is captured. coordinator._repCount.value = repCounter.getRepCount() Logger.d("WORKOUT_COMPLETE event received - triggering immediate set completion") handleSetCompletion(eventLease) diff --git a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt index 180b50872..57e70cfdc 100644 --- a/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt +++ b/shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitPersistenceTest.kt @@ -923,7 +923,7 @@ class WorkoutExitPersistenceTest { * * When the Nth rep notification triggers WORKOUT_COMPLETE, handleSetCompletion * reads coordinator._repCount.value which was still stale (N-1) because the - * StateFlow write at handleRepNotification line 1334 occurs AFTER repCounter.process() + * StateFlow write in handleRepNotification occurs AFTER repCounter.process() * returns. The fix flushes _repCount.value inside the WORKOUT_COMPLETE handler * before calling handleSetCompletion. */ @@ -954,7 +954,7 @@ class WorkoutExitPersistenceTest { harness.modernRepPacket( repsSetCount = romRep, repsSetTotal = 7, - timestamp = nowMs() + romRep * 1000L, + timestamp = harness.nowMs + romRep * 1000L, repsRomCount = romRep + 1, repsRomTotal = 3, ), @@ -968,7 +968,7 @@ class WorkoutExitPersistenceTest { harness.modernRepPacket( repsSetCount = workingRep + 1, repsSetTotal = 7, - timestamp = nowMs() + (3 + workingRep) * 1000L, + timestamp = harness.nowMs + (3 + workingRep) * 1000L, repsRomCount = 3, repsRomTotal = 3, ), @@ -1005,6 +1005,4 @@ class WorkoutExitPersistenceTest { harness.cleanup() } } - - private fun nowMs() = DWSMTestHarness.TEST_WALL_CLOCK_EPOCH_MS }