Per-exercise 'end at the top' toggle applies to all exercises in routine

[9thLevelSoftware]

Bug Report

Reporter: cptnawesome7088
Type: Bug
Repo: Mobile (project-phoenix-mp)
Platform: iOS (latest iPhone version)

Summary

When a user creates a routine with multiple exercises and configures the "end at the top" toggle for only some exercises, the toggle setting bleeds to all exercises during the routine. Only the exercise where the toggle was enabled should end at the top, but instead every exercise in the routine ends at the top.

Steps to Reproduce

1. Create a new routine with 5 different exercises
2. Enable the "end at the top" toggle for only the first exercise
3. Ensure the "end at the top" toggle is OFF for the remaining 4 exercises
4. Start and run the routine

Expected Behavior

Only the first exercise (where "end at the top" was toggled ON) should end at the top. The other 4 exercises should follow their default end behavior (toggle OFF).

Actual Behavior

Every exercise throughout the routine ended at the top, not just the one exercise where the toggle was targeted. The "end at the top" setting appears to apply globally to the routine rather than per-exercise.

Impact

Users cannot customize per-exercise "end at the top" behavior within a routine. The toggle effectively becomes an all-or-nothing setting, breaking routine customization for users who want mixed behavior across exercises.

Original Discord Report

<@1502654170031849644> I am new to both discord and your app. I love what your are doing. So sorry if I do this incorrectly. I made a routine with 5 different exercises and had my first exercise end at the top and the rest with that togglde off. Instead every exercise ended at the top throughout the routine and not just the one exercise that I targeted. I am using the latest iphone version

Attachments

No attachments provided.

[9thLevelSoftware]

In-App UI Preview\n\nThe per-exercise Stop at Top clarification rendered in the actual Phoenix app running in the iOS simulator.\n\n\n\nScreen: Exercise configuration bottom sheet\nPrototype scope: Temporary hardcoded Chest Press fixture; no production data or behavior changes.\nUI direction: Keep the setting visibly scoped to the selected exercise so mixed routines are understandable.

[9thLevelSoftware]

Preview image correction\n\nThe uploaded simulator screenshot is available here:\n\n

[9thLevelSoftware]

Root Cause Analysis

Evidence used

• The original iPhone report describes a five-exercise routine with End at Top enabled only for exercise 1 and disabled for exercises 2–5; all exercises then ended at the top.
• The recreation packet is REPRODUCED (code-path) on main commit 075cc337957a71386aef1df9bb459cdfeb9fffae. It validates that the per-exercise value is correctly saved in the UI/model/database, but leaks during workout transitions.
• The pre-RCA research gate found no clarification gap. The report already defines the mixed per-exercise inputs and the failed result; no reporter clarification was used.

Data path and divergence

RoutineExercise is the per-exercise source of truth for stopAtTop and repCountTiming (shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/Routine.kt:79-107). Routine load correctly seeds both values from exercise 1 into WorkoutParameters (RoutineFlowManager.kt:836-851), and the manual Set Ready path correctly reseeds both values for the selected exercise (RoutineFlowManager.kt:1099-1113).

The active workout path then reads WorkoutParameters to configure the live exercise/BLE behavior (ActiveSessionEngine.kt:2588-2790); stopAtTop is passed to the rep counter and logged as part of the cable-workout parameters. WorkoutParameters is a Kotlin data class (Models.kt:346-375), so copy() retains any field that a caller does not explicitly replace.

Three transition copies replace weight/reps/mode/exercise ID but omit both per-exercise settings:

1. DefaultWorkoutSessionManager.proceedFromSummary (DefaultWorkoutSessionManager.kt:930-940)
2. ActiveSessionEngine.startRestTimer (ActiveSessionEngine.kt:4436-4447)
3. ActiveSessionEngine.startNextSetOrExercise (ActiveSessionEngine.kt:4842-4853)

On an autoplay exercise boundary, startRestTimer selects the next exercise but keeps the previous stopAtTop; startNextSetOrExercise again keeps that stale value and then starts the next workout. As a result, exercise 1's stopAtTop=true is sent/configured for later exercises even when their stored value is false. That is the deterministic cause of the report.

proceedFromSummary has the same stale-copy defect. Its normal non-autoplay branch subsequently calls enterSetReady, which reseeds the correct values, so it is not the primary long-lived path behind the reported autoplay symptom. It must still be fixed to keep the transition state invariant and prevent stale values from being observed before Set Ready completes. repCountTiming has the identical omission and can likewise leak across exercises.

Binding fix direction

In each of the three WorkoutParameters.copy() calls above, explicitly assign the settings from the exercise selected for the next step:

• stopAtTop = nextExercise.stopAtTop (or exerciseForNextSet.stopAtTop in startRestTimer)
• repCountTiming = nextExercise.repCountTiming (or exerciseForNextSet.repCountTiming in startRestTimer)

Do not preserve either value from the prior WorkoutParameters across an exercise boundary. This is a narrow shared-Kotlin state-propagation fix; no UI, database schema, sync protocol, or persisted routine migration is needed.

Acceptance criteria and regression coverage

1. Add an autoplay transition regression in shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/DWSMEquipmentRackTest.kt, alongside the existing autoplay startNextSet ... does not leak ... test. Configure exercise 1 with stopAtTop=true / repCountTiming=BOTTOM and exercise 2 with stopAtTop=false / repCountTiming=TOP; force WorkoutState.Resting, call public dwsm.startNextSet(), and assert the resulting parameters match exercise 2. This must fail before the startNextSetOrExercise fix.
2. Cover the rest-transition parameter update with ActiveSessionEngine.startRestTimer() (the existing lifecycle test class already invokes it) and assert its next-step parameters are sourced from the upcoming exercise.
3. Verify all three copy sites explicitly carry both fields, then run ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true (or the project’s focused test invocation plus the full host-test task before merge).
4. Validate mixed configurations in both directions: true→false must not retain End at Top, and false→true must apply it to the later exercise; likewise verify repCountTiming changes in both directions.

Non-goals

• Do not change the routine-edit UI or the persisted RoutineExercise/SQLDelight schema; the recreation evidence shows those layers already preserve the per-exercise value.
• Do not globally reset WorkoutParameters or alter same-exercise behavior beyond sourcing the selected next exercise’s two per-exercise settings.
• Do not treat this as an iOS-only persistence or device-firmware defect; the failing state transition is in shared Kotlin logic.

[9thLevelSoftware]

Architecture Review — APPROVED

Stage: arch_review for issue #689 (ticket 1533078986291155165)
Reviewer: phoenixarchitect (GPT-5.6 Terra xhigh)
Source commit verified: 075cc337957a71386aef1df9bb459cdfeb9fffae

RCA verification

The RCA root cause is confirmed against source. Three routine-transition WorkoutParameters.copy() calls omit stopAtTop and repCountTiming, so Kotlin data-class copy retains the prior exercise's values across exercise boundaries:

1. DefaultWorkoutSessionManager.proceedFromSummary (line 930-940) — missing both fields
2. ActiveSessionEngine.startRestTimer (line 4436-4447) — missing both fields
3. ActiveSessionEngine.startNextSetOrExercise (line 4842-4853) — missing both fields

The stale value is actively consumed: ActiveSessionEngine.kt:2627 reads params.stopAtTop to configure the live BLE rep-counter/cable behavior. The correct seeding path exists in RoutineFlowManager.kt (lines 845/850, 1107-1108, 1182-1183, 1427-1428), confirming the per-exercise source of truth is available at every transition site.

Architectural risk assessment

• Breaking changes / migration: None. The fix adds two explicitly-assigned fields to three existing copy() calls. No schema, persistence, sync, or API contract change.
• Codebase alignment: Strong. The fix follows the exact pattern already used in RoutineFlowManager.kt seeding paths (stopAtTop = exercise.stopAtTop, repCountTiming = exercise.repCountTiming). No new abstraction or pattern introduced.
• Cross-system risk: None. This is shared-Kotlin state propagation only. No portal, Supabase schema, Edge Function, or firmware interaction. repCountTiming/stopAtTop are not persisted or synced via WorkoutParameters — they are transient session state seeded from RoutineExercise.
• Scope: Minimal and focused. Three copy sites, two fields each. The RCA non-goals correctly exclude UI, schema, sync, and global reset changes. No risk of broadening into unrelated refactors.
• Regression coverage: Feasible. DWSMEquipmentRackTest.kt already has an autoplay startNextSet harness (line 281) that calls harness.dwsm.startNextSet() — the exact transition path needed. The proposed mixed stopAtTop/repCountTiming assertions can be added alongside the existing rack-leak test.

Verdict

APPROVED. The fix direction is technically sound, minimal, and follows existing codebase conventions. The implementation worker should:

1. Add stopAtTop and repCountTiming from the selected next exercise to all three copy() calls.
2. Add the regression tests specified in the RCA acceptance criteria.
3. Run host tests before merge.

No architectural changes requested.