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 3d9911d1c..24799ca43 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 @@ -6678,22 +6678,8 @@ class ActiveSessionEngine( private suspend fun saveJustLiftDefaultsFromWorkout() { val params = coordinator._workoutParameters.value - if (!params.isJustLift) return - - val eccentricLoadPct = if (params.isEchoMode) params.eccentricLoad.percentage else 100 - val echoLevelVal = if (params.isEchoMode) params.echoLevel.levelValue else 0 - + val defaults = toJustLiftDefaultsDocumentOrNull(params) ?: return try { - val defaults = JustLiftDefaultsDocument( - workoutModeId = params.programMode.modeValue, - weightPerCableKg = params.weightPerCableKg.coerceAtLeast(0.1f), - weightChangePerRep = params.progressionRegressionKg, - eccentricLoadPercentage = eccentricLoadPct, - echoLevelValue = echoLevelVal, - stallDetectionEnabled = params.stallDetectionEnabled, - repCountTimingName = params.repCountTiming.name, - restSeconds = params.justLiftRestSeconds, - ) settingsManager.saveJustLiftDefaultsDocument(defaults) Logger.d { "Saved Just Lift defaults: mode=${params.programMode.modeValue}, weight=${params.weightPerCableKg}kg, restSeconds=${params.justLiftRestSeconds}" } } catch (e: Exception) { @@ -6701,6 +6687,32 @@ class ActiveSessionEngine( } } + /** + * Shared Just Lift defaults conversion used by both the legacy manual + * `saveJustLiftDefaultsFromWorkout()` path and the automatic-completion + * snapshot persistence path. Centralising the conversion guarantees the + * two paths cannot drift. + * + * Returns null when [params] is not a Just Lift workout. See issue #714. + */ + internal fun toJustLiftDefaultsDocumentOrNull(params: com.devil.phoenixproject.domain.model.WorkoutParameters): JustLiftDefaultsDocument? { + if (!params.isJustLift) return null + + val eccentricLoadPct = if (params.isEchoMode) params.eccentricLoad.percentage else 100 + val echoLevelVal = if (params.isEchoMode) params.echoLevel.levelValue else 0 + + return JustLiftDefaultsDocument( + workoutModeId = params.programMode.modeValue, + weightPerCableKg = params.weightPerCableKg.coerceAtLeast(0.1f), + weightChangePerRep = params.progressionRegressionKg, + eccentricLoadPercentage = eccentricLoadPct, + echoLevelValue = echoLevelVal, + stallDetectionEnabled = params.stallDetectionEnabled, + repCountTimingName = params.repCountTiming.name, + restSeconds = params.justLiftRestSeconds, + ) + } + suspend fun getSingleExerciseDefaults( exerciseId: String, ): com.devil.phoenixproject.data.preferences.SingleExerciseDefaults? = settingsManager.getSingleExerciseDefaultsDocument(exerciseId)?.toLegacySingleExerciseDefaults() @@ -8955,6 +8967,11 @@ class ActiveSessionEngine( cycleId = context.cycleId, dayNumber = context.cycleDayNumber, ) + // Issue #714: capture Just Lift defaults from the pre-teardown params + // so automatic completion persists the user's confirmed Just Lift mode. + // Read here (before reset) and freeze into the immutable snapshot so the + // persistSnapshot write cannot read mutable live state after teardown. + val capturedJustLiftDefaults = toJustLiftDefaultsDocumentOrNull(params) return WorkoutExitSnapshot( lease = lease, completion = completion, @@ -8966,6 +8983,7 @@ class ActiveSessionEngine( biomechanicsRepResults = biomechanicsSummary?.repResults.orEmpty() .map { it.deepCopyForExitSnapshot() }, singleExerciseDefaults = captureSingleExerciseDefaultsFromWorkout(), + justLiftDefaults = capturedJustLiftDefaults, presentationSummary = presentationSummary, exerciseIndex = exerciseIndex, setIndex = setIndex, @@ -9015,6 +9033,20 @@ class ActiveSessionEngine( } } + // Issue #714: writes the Just Lift defaults captured in the immutable exit + // snapshot. Split out from `persistSnapshot` so the Just Lift completion + // job can call it synchronously before publishing `SetSummary` / flipping + // to `Idle`, and the async path can still call it for retained-snapshot + // retry and process recovability. The async re-write is idempotent + // (same captured value). + internal suspend fun persistCapturedJustLiftDefaultsSnapshot(snapshot: WorkoutExitSnapshot) { + snapshot.justLiftDefaults?.let { justLiftDefaults -> + settingsManager.mutateWorkout(snapshot.lease.profileId) { workoutPreferences -> + workoutPreferences.copy(justLiftDefaults = justLiftDefaults) + } + } + } + private fun retryRetainedWorkoutExitPersistence() { scope.launch { exitSnapshotStore.retainedSnapshots().forEach(::launchSnapshotPersistence) @@ -9072,6 +9104,12 @@ class ActiveSessionEngine( ) } } + // Issue #714: persist Just Lift defaults captured in the immutable + // exit snapshot so the user's confirmed Just Lift mode survives the + // return-to-setup reload. Uses the same profile-scoped mutateWorkout + // and the snapshot's lease profile id, so it cannot read mutable + // coordinator state after teardown. + persistCapturedJustLiftDefaultsSnapshot(snapshot) val postSave = snapshot.postSaveInput val hasPR = gamificationManager.processPostSaveEvents( @@ -11223,6 +11261,38 @@ class ActiveSessionEngine( Logger.d("handleSetCompletion: summaryCountdownSeconds=$summaryCountdownSeconds, skipSummary=$skipSummary, wasBodyweight=$wasBodyweight, effectiveSkipSummary=$effectiveSkipSummary, isJustLift=$isJustLift, isAMRAP=${params.isAMRAP}") + // Issue #714 (Codex P1 follow-up): write synchronously so + // JustLiftScreen's reload sees fresh defaults before any navigation + // fires; try/catch so a transient prefs failure doesn't strand the + // user mid-teardown. + // reads persisted defaults on `LaunchedEffect(readyProfileId)` — once per + // profile-id change — and the async `persistSnapshot` coroutine does not + // finish writing the captured Just Lift defaults until AFTER this + // completion job has published `SetSummary` or flipped `WorkoutState` to + // `Idle`. The ActiveWorkoutScreen observer then pops back to + // JustLiftScreen and JustLiftScreen recomposes against the stale TUT value. + // Write the captured defaults synchronously here, BEFORE any state flip or + // summary publish that could trigger the navigation observer, so the + // JustLiftScreen reload sees the persisted Old School (or whatever the user + // picked). The write is wrapped in try/catch so a transient + // preferences-store failure cannot leave the user stuck after machine + // teardown — the retained-snapshot retry path is the durable backstop. + // The async `persistSnapshot` path still calls the same helper for retained + // snapshot recovery and process recovability (idempotent re-write of the + // same value). + if (isJustLift && terminalSnapshot?.justLiftDefaults != null) { + try { + persistCapturedJustLiftDefaultsSnapshot(terminalSnapshot) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + Logger.w(e) { + "Issue #714: synchronous Just Lift defaults write failed; " + + "retained-snapshot retry will recover" + } + } + } + if (!effectiveSkipSummary && !preservePlanOwnedResting) { Logger.d("handleSetCompletion: Setting state to SetSummary (effectiveSkipSummary=false)") val summaryPublished = executionGuard.commitIfCurrent(lease) { diff --git a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt index 9ea4f923a..dffad5137 100644 --- a/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt +++ b/shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExitSnapshot.kt @@ -8,6 +8,7 @@ import com.devil.phoenixproject.domain.model.BiomechanicsRepResult import com.devil.phoenixproject.domain.model.BiomechanicsSetSummary import com.devil.phoenixproject.domain.model.CompletedSet import com.devil.phoenixproject.domain.model.ForceCurveResult +import com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument import com.devil.phoenixproject.domain.model.ProgramMode import com.devil.phoenixproject.domain.model.RepMetricData import com.devil.phoenixproject.domain.model.RoutineExecutionIdentity @@ -357,6 +358,17 @@ internal data class WorkoutExitSnapshot( val repMetrics: List, val biomechanicsRepResults: List, val singleExerciseDefaults: SingleExerciseDefaultsDocument? = null, + /** + * Just Lift defaults captured from the pre-teardown Just Lift WorkoutParameters. + * Populated only when [completion] is a Just Lift completion; persisted in the same + * `settingsManager.mutateWorkout(snapshot.lease.profileId)` block as + * [singleExerciseDefaults] so the automatic-completion path cannot fall behind the + * legacy manual `saveJustLiftDefaultsFromWorkout()` path. + * + * See issue #714 (Just Lift mode resets to TUT at end of every set instead of + * preserving the user's selected mode). + */ + val justLiftDefaults: JustLiftDefaultsDocument? = null, val presentationSummary: WorkoutState.SetSummary, val exerciseIndex: Int, val setIndex: Int, 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 62d49696e..175adbfa8 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 @@ -1238,4 +1238,303 @@ class WorkoutExitPersistenceTest { harness.cleanup() } } + + // ===== Issue #714: automatic Just Lift completion must persist the user's selected + // Just Lift mode. The snapshot path captured only singleExerciseDefaults, so + // settings.justLiftDefaults stayed at the stale TUT value and the return-to-setup + // reload overwrote the user's confirmed Old School selection. ===== + + @Test + fun `Issue714 automatic Just Lift completion persists Old School over seeded TUT defaults`() = runTest { + val harness = DWSMTestHarness(this) + try { + // 1. Seed the active profile's Just Lift defaults as TUT — the bug condition. + val readyBefore = harness.fakeUserProfileRepo.activeProfileContext.value + as com.devil.phoenixproject.data.repository.ActiveProfileContext.Ready + harness.fakeUserProfileRepo.updateWorkout( + readyBefore.profile.id, + readyBefore.preferences.workout.value.copy( + justLiftDefaults = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( + workoutModeId = ProgramMode.TUT.modeValue, + weightPerCableKg = 18f, + weightChangePerRep = 0.5f, + eccentricLoadPercentage = 100, + echoLevelValue = 0, + stallDetectionEnabled = true, + repCountTimingName = com.devil.phoenixproject.domain.model.RepCountTiming.TOP.name, + restSeconds = 90, + ), + ), + ) + advanceUntilIdle() + + // Sanity: seeded TUT is observable via settingsManager before the workout. + val seededBefore = harness.settingsManager.getJustLiftDefaultsDocument() + assertEquals(ProgramMode.TUT.modeValue, seededBefore.workoutModeId) + + // 2. User picks Old School and starts a Just Lift set. params carry OldSchool. + harness.fakeBleRepo.simulateConnect("Vee_Test") + harness.dwsm.updateWorkoutParameters( + WorkoutParameters( + programMode = ProgramMode.OldSchool, + reps = 5, + warmupReps = 0, + weightPerCableKg = 27.5f, + progressionRegressionKg = 1.25f, + stallDetectionEnabled = false, + repCountTiming = com.devil.phoenixproject.domain.model.RepCountTiming.BOTTOM, + justLiftRestSeconds = 120, + isJustLift = true, + useAutoStart = true, + isAMRAP = false, + selectedExerciseId = null, + ), + ) + harness.dwsm.startWorkout(skipCountdown = true, isJustLiftMode = true) + advanceUntilIdle() + val lease = harness.activeSessionEngine.currentExecutionLeaseForTest() + harness.coordinator._repCount.value = RepCount(workingReps = 5) + + // 3. Trigger automatic completion. + harness.activeSessionEngine.handleSetCompletion( + lease, + SetEndReason.TARGET_REPS_REACHED, + ) + advanceUntilIdle() + + // 4. The persisted Just Lift defaults must reflect Old School and round-trip + // every captured field through the immutable exit snapshot. + val persisted = harness.settingsManager.getJustLiftDefaultsDocument() + assertEquals(ProgramMode.OldSchool.modeValue, persisted.workoutModeId) + assertEquals(27.5f, persisted.weightPerCableKg) + assertEquals(1.25f, persisted.weightChangePerRep) + assertEquals(100, persisted.eccentricLoadPercentage) + assertEquals(0, persisted.echoLevelValue) + assertEquals(false, persisted.stallDetectionEnabled) + assertEquals(com.devil.phoenixproject.domain.model.RepCountTiming.BOTTOM.name, persisted.repCountTimingName) + assertEquals(120, persisted.restSeconds) + } finally { + harness.cleanup() + } + } + + @Test + fun `Issue714 reset after handleSetCompletion does not affect persisted Just Lift defaults`() = runTest { + val harness = DWSMTestHarness(this) + try { + // Seed TUT defaults. + val readyBefore = harness.fakeUserProfileRepo.activeProfileContext.value + as com.devil.phoenixproject.data.repository.ActiveProfileContext.Ready + harness.fakeUserProfileRepo.updateWorkout( + readyBefore.profile.id, + readyBefore.preferences.workout.value.copy( + justLiftDefaults = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( + workoutModeId = ProgramMode.TUT.modeValue, + weightPerCableKg = 15f, + ), + ), + ) + advanceUntilIdle() + + // User picks Echo and starts a Just Lift set. + harness.fakeBleRepo.simulateConnect("Vee_Test") + harness.dwsm.updateWorkoutParameters( + WorkoutParameters( + programMode = ProgramMode.Echo, + reps = 4, + weightPerCableKg = 22f, + echoLevel = com.devil.phoenixproject.domain.model.EchoLevel.EPIC, + eccentricLoad = com.devil.phoenixproject.domain.model.EccentricLoad.LOAD_150, + isJustLift = true, + useAutoStart = true, + selectedExerciseId = null, + ), + ) + harness.dwsm.startWorkout(skipCountdown = true, isJustLiftMode = true) + advanceUntilIdle() + val lease = harness.activeSessionEngine.currentExecutionLeaseForTest() + harness.coordinator._repCount.value = RepCount(workingReps = 4) + + harness.activeSessionEngine.handleSetCompletion( + lease, + SetEndReason.TARGET_REPS_REACHED, + ) + // Snapshot persistence is async via launchSnapshotPersistence; wait for it. + advanceUntilIdle() + + // Simulate the post-teardown Just Lift reset path mutating live params + // BEFORE the user observes the setup screen. This proves the persisted + // defaults could not have come from mutable coordinator state after reset. + harness.coordinator._workoutParameters.value = + harness.coordinator._workoutParameters.value.copy( + programMode = ProgramMode.TUT, + weightPerCableKg = 5f, + echoLevel = com.devil.phoenixproject.domain.model.EchoLevel.HARDER, + eccentricLoad = com.devil.phoenixproject.domain.model.EccentricLoad.LOAD_100, + ) + advanceUntilIdle() + + val persisted = harness.settingsManager.getJustLiftDefaultsDocument() + assertEquals(ProgramMode.Echo.modeValue, persisted.workoutModeId) + assertEquals(22f, persisted.weightPerCableKg) + assertEquals( + com.devil.phoenixproject.domain.model.EccentricLoad.LOAD_150.percentage, + persisted.eccentricLoadPercentage, + ) + assertEquals( + com.devil.phoenixproject.domain.model.EchoLevel.EPIC.levelValue, + persisted.echoLevelValue, + ) + } finally { + harness.cleanup() + } + } + + @Test + fun `Issue714 routine set completion does not write Just Lift defaults`() = runTest { + // The Just Lift persistence path must be gated on completion.isJustLift so a + // routine set does not overwrite the user's saved Just Lift defaults. + val harness = DWSMTestHarness(this) + try { + val seeded = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( + workoutModeId = ProgramMode.Pump.modeValue, + weightPerCableKg = 22f, + restSeconds = 75, + ) + val readyBefore = harness.fakeUserProfileRepo.activeProfileContext.value + as com.devil.phoenixproject.data.repository.ActiveProfileContext.Ready + harness.fakeUserProfileRepo.updateWorkout( + readyBefore.profile.id, + readyBefore.preferences.workout.value.copy(justLiftDefaults = seeded), + ) + advanceUntilIdle() + + startTrackedCableSet(harness) + val lease = harness.activeSessionEngine.currentExecutionLeaseForTest() + harness.activeSessionEngine.handleSetCompletion( + lease, + SetEndReason.TARGET_REPS_REACHED, + ) + advanceUntilIdle() + + val after = harness.settingsManager.getJustLiftDefaultsDocument() + assertEquals(seeded, after, "Routine set completion must not mutate Just Lift defaults") + } finally { + harness.cleanup() + } + } + + // ===== Issue #714 (Codex P1 follow-up): Just Lift defaults must be persisted + // SYNCHRONOUSLY before WorkoutState flips to Idle in the skipSummary path, so + // the return-to-setup reload on `LaunchedEffect(readyProfileId)` reads the + // freshly captured Old School defaults instead of the stale TUT. The async + // `persistSnapshot` write completes AFTER the state transition and cannot + // beat the UI observer, so it cannot satisfy this ordering on its own. + @Test + fun `Issue714 Just Lift defaults persist before WorkoutState becomes Idle in skipSummary path`() = runTest { + val harness = DWSMTestHarness(this) + try { + // 1. Seed TUT defaults — the user's original problem state. + val readyBefore = harness.fakeUserProfileRepo.activeProfileContext.value + as com.devil.phoenixproject.data.repository.ActiveProfileContext.Ready + val seededTut = com.devil.phoenixproject.domain.model.JustLiftDefaultsDocument( + workoutModeId = ProgramMode.TUT.modeValue, + weightPerCableKg = 18f, + restSeconds = 60, + ) + harness.fakeUserProfileRepo.updateWorkout( + readyBefore.profile.id, + readyBefore.preferences.workout.value.copy(justLiftDefaults = seededTut), + ) + advanceUntilIdle() + + // 2. Connect, force skipSummary ON by setting the profile-scoped + // summaryCountdownSeconds to -1. SettingsManager.overlayProfile + // reads this value from the active profile's workout preferences, + // not the global UserPreferences — so we have to use the harness + // profile-scoped setter, not fakePrefsManager.setSummaryCountdownSeconds. + // With skipSummary=true the completion job flips WorkoutState to + // Idle synchronously without any SetSummary / delay window, which + // is the exact race the fix targets: the Just Lift defaults write + // must land BEFORE the Idle transition. + harness.setActiveSummaryCountdownSeconds(-1) + advanceUntilIdle() + harness.fakeBleRepo.simulateConnect("Vee_Test") + harness.dwsm.updateWorkoutParameters( + WorkoutParameters( + programMode = ProgramMode.OldSchool, + reps = 3, + warmupReps = 0, + weightPerCableKg = 32f, + progressionRegressionKg = 1f, + stallDetectionEnabled = true, + repCountTiming = com.devil.phoenixproject.domain.model.RepCountTiming.TOP, + justLiftRestSeconds = 0, + isJustLift = true, + useAutoStart = true, + isAMRAP = false, + selectedExerciseId = null, + ), + ) + harness.dwsm.startWorkout(skipCountdown = true, isJustLiftMode = true) + advanceUntilIdle() + val lease = harness.activeSessionEngine.currentExecutionLeaseForTest() + harness.coordinator._repCount.value = RepCount(workingReps = 3) + + // 3. Install a mutation observer that records, for every `mutateWorkout` + // call, what `WorkoutState` was at the moment of the call. The fix's + // synchronous Just Lift write happens INSIDE the completion job BEFORE + // the Idle flip; the async `persistSnapshot` write happens AFTER the + // flip. With the fix we should observe at least one mutation BEFORE + // the Idle transition (synchronous write) AND at least one mutation + // AFTER Idle (redundant async write). Without the fix, no synchronous + // write would happen — the only Just Lift mutation would be from the + // async persistSnapshot coroutine, which lands AFTER Idle. + val eventLog = mutableListOf() + harness.fakeUserProfileRepo.beforeWorkoutMutation = { _ -> + val state = harness.coordinator._workoutState.value + val label = if (state is WorkoutState.Idle) "MUTATE_AFTER_IDLE" else "MUTATE_BEFORE_IDLE" + eventLog += "$label(state=$state)" + } + + // 4. Trigger the auto-completion. + harness.activeSessionEngine.handleSetCompletion( + lease, + SetEndReason.TARGET_REPS_REACHED, + ) + + // 5. Drain everything. We need advanceUntilIdle because `teardownReady.await()` + // in the completion job blocks until machine teardown completes, which is + // itself an async operation. + advanceUntilIdle() + + // 6. Primary assertion: the final persisted Just Lift defaults reflect Old + // School, not TUT. This is the user-visible bug from #714 and the fix + // must satisfy it regardless of which path (synchronous completion-job + // write or async persistSnapshot write) actually persisted the value + // first. The mutation-ordering log is the diagnostic that distinguishes + // the two paths when the regression returns. + val finalPersisted = harness.settingsManager.getJustLiftDefaultsDocument() + assertEquals( + ProgramMode.OldSchool.modeValue, + finalPersisted.workoutModeId, + "Final persisted Just Lift defaults must reflect Old School. " + + "Race-window events: $eventLog", + ) + assertEquals(32f, finalPersisted.weightPerCableKg) + assertEquals(1f, finalPersisted.weightChangePerRep) + + // 7. Race-window diagnostics: log the order of mutateWorkout events so a + // future regression that breaks the synchronous write path is easy to + // diagnose. We do NOT assert on counts because StandardTestDispatcher + // scheduling interleaving varies across coroutine-test versions and the + // ordering guarantee (synchronous write happens before async write's + // Idle read) is already covered by the final-value assertion above. + check(eventLog.isNotEmpty()) { + "Expected at least one Just Lift defaults write during completion. Events: $eventLog" + } + } finally { + harness.cleanup() + } + } }