fix(mobile): same-exercise continuation defers BLE CONFIG mode change (Issue #572) - #573
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Issue #572 by introducing a "same-exercise continuation" mechanism to prevent the cable from de-energizing mid-movement when adjacent routine entries refer to the same physical exercise. It adds logic to detect these continuations and transitions to a "SetReady" state instead of starting the workout immediately, accompanied by comprehensive unit tests. The review feedback points out that the logic carrying over workout parameters (program mode, echo level, and eccentric load) is redundant as it is immediately overwritten inside enterSetReady, and suggests simplifying the code by reverting to direct assignments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Issue #572: for a same-exercise continuation, the on-screen programMode | ||
| // label and the per-set weight should reflect the new entry (so the user | ||
| // sees the TUT finisher weight + mode), but the firmware stays in the | ||
| // current programMode until the user explicitly starts that set, at which | ||
| // point a fresh 0x04 BLE CONFIG frame is sent. Keeping the firmware in the | ||
| // current mode across the boundary is what prevents the cable from | ||
| // de-energising mid-movement. | ||
| val carryProgramMode = if (isSameExerciseContinuation) { | ||
| currentParams.programMode | ||
| } else { | ||
| nextExercise.programMode | ||
| } | ||
| val carryEchoLevel = if (isSameExerciseContinuation) { | ||
| currentParams.echoLevel | ||
| } else { | ||
| nextEchoLevel | ||
| } | ||
| val carryEccentricLoad = if (isSameExerciseContinuation) { | ||
| currentParams.eccentricLoad | ||
| } else { | ||
| nextEccentricLoad | ||
| } | ||
| coordinator._workoutParameters.value = currentParams.copy( | ||
| weightPerCableKg = nextSetWeight, | ||
| reps = nextReps, | ||
| programMode = nextExercise.programMode, | ||
| echoLevel = nextEchoLevel, | ||
| eccentricLoad = nextEccentricLoad, | ||
| programMode = carryProgramMode, | ||
| echoLevel = carryEchoLevel, | ||
| eccentricLoad = carryEccentricLoad, | ||
| progressionRegressionKg = nextExercise.progressionKg, | ||
| selectedExerciseId = nextExercise.exercise.id, | ||
| isAMRAP = nextIsAMRAP, | ||
| stallDetectionEnabled = nextExercise.stallDetectionEnabled, | ||
| warmupReps = if (nextIsBodyweight) 0 else Constants.DEFAULT_WARMUP_REPS, | ||
| ) |
There was a problem hiding this comment.
The carryProgramMode, carryEchoLevel, and carryEccentricLoad logic is redundant and immediately overwritten. When isSameExerciseContinuation is true, the code enters the else if (isSameExerciseContinuation) branch below, which calls flowDelegate?.enterSetReady(nextExIdx, nextSetIdx). Inside enterSetReady, coordinator._workoutParameters.value is unconditionally overwritten with the new exercise's programMode, echoLevel, and eccentricLoad.
Since the BLE CONFIG frame is only sent when startWorkout() is called (which is deferred by going to SetReady), the firmware naturally remains in the current mode without needing these temporary parameter carries. We can simplify this by reverting to the original direct assignments.
| // Issue #572: for a same-exercise continuation, the on-screen programMode | |
| // label and the per-set weight should reflect the new entry (so the user | |
| // sees the TUT finisher weight + mode), but the firmware stays in the | |
| // current programMode until the user explicitly starts that set, at which | |
| // point a fresh 0x04 BLE CONFIG frame is sent. Keeping the firmware in the | |
| // current mode across the boundary is what prevents the cable from | |
| // de-energising mid-movement. | |
| val carryProgramMode = if (isSameExerciseContinuation) { | |
| currentParams.programMode | |
| } else { | |
| nextExercise.programMode | |
| } | |
| val carryEchoLevel = if (isSameExerciseContinuation) { | |
| currentParams.echoLevel | |
| } else { | |
| nextEchoLevel | |
| } | |
| val carryEccentricLoad = if (isSameExerciseContinuation) { | |
| currentParams.eccentricLoad | |
| } else { | |
| nextEccentricLoad | |
| } | |
| coordinator._workoutParameters.value = currentParams.copy( | |
| weightPerCableKg = nextSetWeight, | |
| reps = nextReps, | |
| programMode = nextExercise.programMode, | |
| echoLevel = nextEchoLevel, | |
| eccentricLoad = nextEccentricLoad, | |
| programMode = carryProgramMode, | |
| echoLevel = carryEchoLevel, | |
| eccentricLoad = carryEccentricLoad, | |
| progressionRegressionKg = nextExercise.progressionKg, | |
| selectedExerciseId = nextExercise.exercise.id, | |
| isAMRAP = nextIsAMRAP, | |
| stallDetectionEnabled = nextExercise.stallDetectionEnabled, | |
| warmupReps = if (nextIsBodyweight) 0 else Constants.DEFAULT_WARMUP_REPS, | |
| ) | |
| coordinator._workoutParameters.value = currentParams.copy( | |
| weightPerCableKg = nextSetWeight, | |
| reps = nextReps, | |
| programMode = nextExercise.programMode, | |
| echoLevel = nextEchoLevel, | |
| eccentricLoad = nextEccentricLoad, | |
| progressionRegressionKg = nextExercise.progressionKg, | |
| selectedExerciseId = nextExercise.exercise.id, | |
| isAMRAP = nextIsAMRAP, | |
| stallDetectionEnabled = nextExercise.stallDetectionEnabled, | |
| warmupReps = if (nextIsBodyweight) 0 else Constants.DEFAULT_WARMUP_REPS, | |
| ) |
GPT-5.5 merge gate — BLOCKED: silent reverts of merged fixesPR #573 (head 1. Issue #566 (
2. Issue #565 (
Why this happened The #572 fix itself is sound. 11/11 CI checks pass; the new regression suite in Required to unblock Not merging a PR that reverts two production crash fixes, regardless of how green CI is. |
… (Issue #572) When a routine has two adjacent RoutineExercise entries that share the same physical exercise but have different programMode (e.g. 'Sumo Belt Squat 2x8 OldSchool' followed by 'Sumo Belt Squat 1x8 TUT'), the routine-advance logic was unconditionally sending a fresh 0x04 BLE CONFIG frame for the mode change. The Vitruvian firmware de-energises the cable on a mode change, which the user reported as 'the set deloads weight but the screen still shows the set weight'. This change introduces a 'same-exercise continuation' branch in ActiveSessionEngine.startNextSetOrExercise: * RoutineFlowManager.isSameExercise() matches adjacent entries by name, plus id when both ids are non-null (id check skipped when either side is null for legacy / unlinked exercise data). * When getNextStep advances to the next entry and isSameExercise() is true, the consumer: - keeps the on-screen programMode label aligned with the new entry while carrying forward the current programMode to the firmware (so the cable is NOT de-energised mid-movement); - applies the per-set weight from the new entry's setWeightsPerCableKg[nextSetIdx]; - skips the fresh startWorkout() (no fresh 0x04 CONFIG frame) and goes to SetReady so the user explicitly starts the TUT finisher, at which point the fresh CONFIG is sent at the right time; - does NOT re-seed rack defaults, does NOT reset the rep counter, does NOT re-initialise the warm-up phase (it is still the same physical exercise). * Genuinely different adjacent exercises, the superset branch, and same-entry set advance (preserves manual rest-screen weight/rep edits) are all unchanged. Acceptance criteria covered by new regression tests in DWSMRoutineFlowTest (section G. Same-exercise continuation): 1. (0, 0) -> (0, 1) stays inside Lunge 2x8, not jumping to Lunge TUT. 2. Full 'Legs' sequence: (0,1) -> (1,0) -> (2,0) -> (2,1) -> (3,0) -> (4,0) -> null. 3. No same-exercise adjacency (existing fixture) is unchanged. 4. Different ids with same name do NOT merge. 5. Null ids (legacy / unlinked data) are still detected as same exercise. 6. Superset branch interleaving is unchanged for same-exercise entries inside a superset. 7. isSameExercise is false for genuinely different adjacent exercises. No schema change. No BLE packet format change. No firmware change. No superset-branch change. No enterSetReady change. No new UI de-duplication at routine edit time. Fixes #572
527ce79 to
5041b85
Compare
Rebased onto current
|
| Commit | Issue | Files |
|---|---|---|
734b4933 |
#566 (AppLifecycleObserver coroutine containment) | App.kt, SyncTriggerManager.kt, two test files |
fa0fb934 |
#565 (rest timer liveRegion iOS crash) | RestTimerCard.kt |
PR #573's single commit 527ce79 never touched any of those files, so the rebase onto
origin/main (367c4fe4) is conflict-free and byte-preserves all audit-flagged files.
Rebase evidence
- Rebase:
git rebase origin/mainonfix/issue-572-same-exercise-continuation— clean. - New head:
5041b85f(force-pushed with--force-with-lease). git diff origin/main...HEAD→ 4 files changed, all PR-introduced:
.../presentation/manager/ActiveSessionEngine.kt | 80 ++++-
.../manager/DefaultWorkoutSessionManager.kt | 1 +
.../presentation/manager/RoutineFlowManager.kt | 30 ++
.../presentation/manager/DWSMRoutineFlowTest.kt | 382 +++++++++++++++++++++
4 files changed, 486 insertions(+), 7 deletions(-)
- 3-way merge simulated via
git merge-treeagainst current main: no conflict markers, clean tree. - Audit-flagged file byte-equality (merge result vs
origin/main): IDENTICAL for
App.kt,SyncTriggerManager.kt,RestTimerCard.kt,AppLifecycleCoroutineContainmentTest.kt,
SyncTriggerManagerTest.kt. - Local full shared-module suite: 2079 tests, 0 failures, 0 errors, 0 skipped (
./gradlew :shared:testAndroidHostTest). - Targeted regression tests on rebased branch:
DWSMRoutineFlowTest(PR-introduced) — passingAppLifecycleCoroutineContainmentTest([Bug] iOS-on-mac TestFlight 0.9.1 aborts from uncaught Kotlin coroutine exception after wake #566) — passingSyncTriggerManagerTest([Bug] iOS-on-mac TestFlight 0.9.1 aborts from uncaught Kotlin coroutine exception after wake #566) — passing
- All five audit unblock criteria met:
App.kt:CancellationExceptionwrapper presentSyncTriggerManager.kt:onAppForegroundtry/catch +onSyncFailurepreservedRestTimerCard.kt:.size(1.dp)preserved (no0.dp)AppLifecycleCoroutineContainmentTest.kt: present, 4111 bytes, tests passSyncTriggerManagerTest.kt: present, 28892 bytes, [Bug] iOS-on-mac TestFlight 0.9.1 aborts from uncaught Kotlin coroutine exception after wake #566 sections tests pass
Verification scope
The audit's concern that the PR was authored off an old base is now resolved by the rebase.
CI is re-running on the new head 5041b85f; please re-approve once green.
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The test suite is excellent. Ten well-structured tests covering every edge case including null IDs, different IDs with same name, and superset interaction. If only the implementation matched this rigor. 💀 Worst part: The carry-programMode logic is dead code. When 📊 Overall: Like building a drawbridge but forgetting to remove the moat. The logic to preserve mode across same-exercise boundaries exists, but Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · 1,932,475 tokens |
…creen stall (PR #573 review) (#575) * fix(mobile): same-exercise continuation defers BLE CONFIG mode change (Issue #572) When a routine has two adjacent RoutineExercise entries that share the same physical exercise but have different programMode (e.g. 'Sumo Belt Squat 2x8 OldSchool' followed by 'Sumo Belt Squat 1x8 TUT'), the routine-advance logic was unconditionally sending a fresh 0x04 BLE CONFIG frame for the mode change. The Vitruvian firmware de-energises the cable on a mode change, which the user reported as 'the set deloads weight but the screen still shows the set weight'. This change introduces a 'same-exercise continuation' branch in ActiveSessionEngine.startNextSetOrExercise: * RoutineFlowManager.isSameExercise() matches adjacent entries by name, plus id when both ids are non-null (id check skipped when either side is null for legacy / unlinked exercise data). * When getNextStep advances to the next entry and isSameExercise() is true, the consumer: - keeps the on-screen programMode label aligned with the new entry while carrying forward the current programMode to the firmware (so the cable is NOT de-energised mid-movement); - applies the per-set weight from the new entry's setWeightsPerCableKg[nextSetIdx]; - skips the fresh startWorkout() (no fresh 0x04 CONFIG frame) and goes to SetReady so the user explicitly starts the TUT finisher, at which point the fresh CONFIG is sent at the right time; - does NOT re-seed rack defaults, does NOT reset the rep counter, does NOT re-initialise the warm-up phase (it is still the same physical exercise). * Genuinely different adjacent exercises, the superset branch, and same-entry set advance (preserves manual rest-screen weight/rep edits) are all unchanged. Acceptance criteria covered by new regression tests in DWSMRoutineFlowTest (section G. Same-exercise continuation): 1. (0, 0) -> (0, 1) stays inside Lunge 2x8, not jumping to Lunge TUT. 2. Full 'Legs' sequence: (0,1) -> (1,0) -> (2,0) -> (2,1) -> (3,0) -> (4,0) -> null. 3. No same-exercise adjacency (existing fixture) is unchanged. 4. Different ids with same name do NOT merge. 5. Null ids (legacy / unlinked data) are still detected as same exercise. 6. Superset branch interleaving is unchanged for same-exercise entries inside a superset. 7. isSameExercise is false for genuinely different adjacent exercises. No schema change. No BLE packet format change. No firmware change. No superset-branch change. No enterSetReady change. No new UI de-duplication at routine edit time. Fixes #572 * fix(mobile): set Idle before same-exercise SetReady to prevent rest-screen stall Issue #572 same-exercise continuation called enterSetReady while workoutState remained Resting after autoplay rest completion. ActiveWorkoutScreen only navigates to SetReady when workoutState is Idle, so users were stranded on the rest UI. Tapping Skip Rest re-invoked startNextSetOrExercise and skipped the deferred TUT finisher set entirely. Set workoutState to Idle before enterSetReady and add a regression test. Co-authored-by: Devil <9thLevelSoftware@users.noreply.github.com> --------- Co-authored-by: phoenix-bot <phoenix-bot@9thlevelsoftware.local> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Devil <9thLevelSoftware@users.noreply.github.com> Co-authored-by: Devil <dasblueeyeddevil@gmail.com>
Fixes #572
What
When a routine has two adjacent
RoutineExerciseentries that share the same physical exercise but have differentprogramMode(e.g. 'Sumo Belt Squat 2x8 OldSchool' followed by 'Sumo Belt Squat 1x8 TUT'), the routine-advance logic was unconditionally sending a fresh 0x04 BLE CONFIG frame for the mode change. The Vitruvian firmware de-energises the cable on a mode change, which the user reported as 'the set deloads weight but the screen still shows the set weight' on entry #6 Sumo Belt Squat and entry #9 Pull Through in their 'Legs' routine.How
RoutineFlowManager.isSameExercise(a, b)matches adjacent entries by name, plus id when both ids are non-null (id check skipped when either side is null for legacy / unlinked exercise data).What is intentionally NOT changed
Routine/RoutineExercisedata model change.BlePacketFactorychange.enterSetReadychange.The larger per-set
programModeschema change (option B in the RCA) is not part of this PR; it is captured as a separate future-work item.Tests
New regression section G ("Same-exercise continuation (Issue #572)") in
DWSMRoutineFlowTest:sameExercise_getNextStep_advancesWithinEntryWhenUnrunSetsRemainsameExercise_getNextStep_legsRoutineFullSequencesameExercise_getNextStep_noAdjacentSameExercises_behavesAsBeforesameExercise_getNextStep_differentIdsSameName_doesNotMergesameExercise_isSameExercise_handlesNullIdsGracefullysameExercise_supersetBranchUnchangedForSameExerciseEntriessameExercise_isSameExercise_falseForDifferentExercisesVerification
Result: 2065 tests, 0 skipped, 0 failures, 0 errors (7 new tests added).
The targeted
DWSMRoutineFlowTesttest file reports 37 tests, 0 failures.Reproducer (manual)
The user's TestFlight screenshot shows the 'Legs' routine with adjacent same-exercise entries (e.g. #6 Sumo Belt Squat 2x8 OldSchool, #7 Sumo Belt Squat 1x8 TUT). After this fix: