Update feat branch - #578
Merged
Merged
Conversation
…eground sync (#566) (#569) Fixes #566 Squash-merging the foreground coroutine exception containment fix. Verification (GPT-5.5 xhigh merge gate): - 11/11 CI checks pass (Security Hygiene, iOS Schema Sync, Lint, Unit Tests, Build Android, iOS Test Target Compile, Cursor Automation, Android App Unit Test, Shared Module Test, GitGuardian, Kilo Code Review) - mergeable=MERGEABLE, mergeStateStatus=CLEAN - Reviews: Copilot COMMENTED (quota-limited, no content), Gemini Code Assist COMMENTED with 4 advisory suggestions (not REQUEST_CHANGES) - Gemini suggestions evaluated and intentionally NOT applied — they conflict with the RCA's defense-in-depth crash-prevention approach (Throwable catch is required on Kotlin/Native to prevent SIGABRT; Exception would miss Error subclasses that can still reach the uncaught handler) - Diff: +194 / -8 across 4 files (App.kt, SyncTriggerManager.kt, AppLifecycleCoroutineContainmentTest.kt, SyncTriggerManagerTest.kt) - Fix matches RCA: rethrow CancellationException to preserve cancellation semantics, log+swallow other throwables to prevent them reaching propagateExceptionFinalResort - Closing keyword Fixes #566 verified
…#567) Squash-merged via GPT-5.5 xhigh merge gate for issue #565 (iOS TestFlight EXC_BAD_ACCESS in Compose Multiplatform accessibility bounds during rest between sets). Final commit e1b0c35 changes the liveRegion announcement Box from 0.dp to 1.dp (non-zero) — eliminates the zero-size AX node that raced iOS UIAccessibility polling while preserving the separate hidden-node announcement pattern (so the visible timer Text reads its actual time when focused by VoiceOver/TalkBack, fixing the stale-focus regression Gemini Code Assist caught in the first commit). Local :shared:testAndroidHostTest passes (184 suites, 2058 tests). 10/10 CI checks green.
…n iPhone portrait (#571) (#574) * fix(just-lift): cap wheel drag hot zone + side-by-side weight cards on iPhone portrait (#571) Issue #571 reporter (iPhone 16 Pro, Just Lift, LB) saw the Weight Change Per Rep slider produce 29/30/44-49 instead of 1 lb increments. RCA proved the slider is correctly bound to valueRange = -10..+10 with 1 lb steps and cannot emit those values; the visible jumps were the iOS CompactNumberPicker (LazyColumn wheel, range 1..242 LB) claiming vertical drags intended for the slider and snapping to neighbour weights. This commit applies the three-part fix from the GPT-5.5 RCA: 1. WindowSizeClass.kt + JustLiftScreen.kt — split the side-by-side weight card gate from the broader useCompactAccessibility. New shouldStackWeightCards() returns true only when the screen is also short (height Compact), so iPhone portrait (402x874dp) with default Dynamic Type / Bold Text keeps the two weight cards side-by-side. iPhone landscape and short-height tablets still stack. 2. JustLiftScreen.kt — add an explicit Spacer(Spacing.small) between the two weight cards when stacked, belt-and-braces separation. 3. CompactNumberPicker.ios.kt — add a pointerInput on the wheel's BoxWithConstraints that consumes vertical drags landing outside the centred 36dp selected band, so the inner LazyColumn does not claim them. In edit mode the wheel defers entirely to the BasicTextField. Adds WindowSizeClassTest cases for shouldStackWeightCards and a new JustLiftScreenWeightSliderWiringTest that pins the ProgressionSlider's valueRange and weightChangePerRep binding against future regressions. Refs: #571 (Fixes) RCA: rca_owner=gpt-5.5-xhigh, fix_driver=gpt55_rca * fix(just-lift): address Gemini review feedback on #574 Three valid findings from @gemini-code-assist[bot] on PR #574 (#571), plus a documentation comment for the fourth: 1. **Dead-zone fix (HIGH):** the previous commit's pointerInput consumed every pointer event outside the wheel's centred 36dp band, which prevented the outer verticalScroll from scrolling while the user's finger was on those top/bottom areas of the wheel. Replaced the consume-events approach with a 'dragStartedInCenterBand' state flag that gates LazyColumn.userScrollEnabled. The inner LazyColumn no longer claims the gesture when the drag starts outside the centred row, so the outer verticalScroll / sibling ProgressionSlider can take over naturally. No event consumption, no dead zone. 2. **Spacing fix (MEDIUM):** the explicit Spacer(modifier = Modifier.height( Spacing.small)) added between the two stacked cards was being double-spaced by the outer Column's Arrangement.spacedBy(Spacing.medium) — total gap was 40dp, not the 24dp the PR description claimed. Moved the additional separation to bottom padding on the first card (only when stackWeightCards is true), so the outer arrangement no longer doubles up: 16dp (outer) + 8dp (this padding) = 24dp. 3. **Redundant logic (MEDIUM):** shouldStackWeightCards was defined as 'shouldUseCompactAccessibilityLayout(...) && heightSizeClass == Compact', but shouldUseCompactAccessibilityLayout already returns true for compact heights (its first OR-clause), so the entire LHS was redundant. Reduced the helper to 'heightSizeClass == Compact'. fontScale, boldTextEnabled, and fontScaleThreshold parameters are now gone from the public signature. Updated WindowSizeClassTest cases to match the simplified contract; added a new exhaustive test pinning height as the sole determinant. 4. **Fragile test (deferred with rationale):** the static-analysis approach in JustLiftScreenWeightSliderWiringTest is a known anti-pattern. Documented in the file's KDoc why we keep static analysis for this PR (the bug class is 'widget wired wrong' — a UI test would still pass if onValueChange were silently rebinded) and when it should be migrated to a Compose UI test. Local gates (all green): - :shared:testAndroidHostTest → BUILD SUCCESSFUL - :shared:lintAnalyzeAndroidHostTest → BUILD SUCCESSFUL - :shared:compileKotlinIosArm64 → BUILD SUCCESSFUL * fix(test): make JustLiftScreenWeightSliderWiringTest KMP-compatible The 'iOS Test Target Compile' CI job failed because the static-analysis test introduced in #574 uses java.io.File, which is JVM-only and not available under the iOS test target. The Android test target compiles fine because it links the JVM stdlib. Move the file-reading logic into a KMP expect/actual pair: - commonTest/testutil/SourceFileReader.kt → expect fun readProjectFile - androidHostTest/testutil/SourceFileReader.android.kt → actual via java.io.File - iosTest/testutil/SourceFileReader.ios.kt → actual via NSFileManager + NSString.stringWithContentsOfFile The test file itself now just calls readProjectFile(relativePath) and asserts non-null. All the up-walk logic is contained in the actuals, so the iOS implementation can be re-tuned independently if Xcode's test-runner cwd differs from the gradle one. Also drops the unused 'assertEquals' import that was added in the previous commit (assertEquals is still used in WindowSizeClassTest.kt; that file keeps its import). Local gates (all green): - :shared:compileTestKotlinIosArm64 → BUILD SUCCESSFUL - :shared:compileKotlinIosArm64 → BUILD SUCCESSFUL - :shared:testAndroidHostTest → BUILD SUCCESSFUL - :shared:lintAnalyzeAndroidHostTest → BUILD SUCCESSFUL --------- Co-authored-by: phoenix-bot <bot@phoenix.local> Co-authored-by: 9thLevelSoftware <noreply@9thlevelsoftware.com>
… (Issue #572) (#573) 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 Co-authored-by: phoenix-bot <phoenix-bot@9thlevelsoftware.local>
…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>
* 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>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several bug fixes and layout improvements, including robust exception handling during foreground sync to prevent crashes, same-exercise continuation logic to avoid de-energizing cables during workout mode changes, and layout adjustments to prevent gesture conflicts on tall portrait devices. Additionally, it addresses zero-size accessibility node crashes on iOS and refines rep counting in unlimited modes. The reviewer provided valuable feedback pointing out that using raw absolute values for directional counters in RepCounterFromMachine assumes they start at zero, which could prematurely skip the warmup phase if they do not reset. The reviewer also suggested using isNullOrBlank() when comparing exercise IDs to safely handle blank IDs in legacy data.
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.
Comment on lines
+481
to
+484
| else if (repsSetCount == 0 && repsRomCount == 0 && warmupReps < warmupTarget) { | ||
| val directionalWarmupReps = maxOf(up, down).coerceAtMost(warmupTarget) | ||
| if (directionalWarmupReps > warmupReps) { | ||
| warmupReps = directionalWarmupReps |
Contributor
There was a problem hiding this comment.
Using raw absolute values of up and down directly (e.g., maxOf(up, down)) introduces a fragile assumption that these machine counters always start at exactly 0 at the beginning of every set.
If the machine's counters do not reset (for example, due to a missed BLE CONFIG frame, reconnection mid-set, or firmware-specific behavior), maxOf(up, down) will immediately exceed warmupTarget, causing the warmup phase to be instantly skipped. Similarly, directionalWorkingReps at line 565 will immediately jump to an incorrect value.
Recommendation:
To make this mode-agnostic and robust against arbitrary starting counter values, track the initial up and down values (e.g., initialUp and initialDown) when the set is configured or upon receiving the first packet, and use the relative delta from those baselines:
val relativeUp = up - initialUp
val relativeDown = down - initialDown
val directionalWarmupReps = maxOf(relativeUp, relativeDown).coerceAtMost(warmupTarget)
Comment on lines
+432
to
+436
| val aid = a.exercise.id | ||
| val bid = b.exercise.id | ||
| // When both ids are present, they must agree. When either is null, the name match | ||
| // is the only signal we have, and that is sufficient. | ||
| return aid == null || bid == null || aid == bid |
Contributor
There was a problem hiding this comment.
In legacy or unlinked exercise data, exercise.id might be a blank string (e.g., "") rather than null.
Currently, isSameExercise only checks for null values (aid == null || bid == null). If one of the IDs is blank (e.g., "") and the other is a valid ID, the check aid == bid will fail, and the method will incorrectly return false even though the names match and one ID is effectively absent.
Recommendation:
Use isNullOrBlank() to safely treat blank IDs as absent/null.
Suggested change
| val aid = a.exercise.id | |
| val bid = b.exercise.id | |
| // When both ids are present, they must agree. When either is null, the name match | |
| // is the only signal we have, and that is sufficient. | |
| return aid == null || bid == null || aid == bid | |
| val aid = a.exercise.id | |
| val bid = b.exercise.id | |
| // When both ids are present, they must agree. When either is null or blank, the name match | |
| // is the only signal we have, and that is sufficient. | |
| return aid.isNullOrBlank() || bid.isNullOrBlank() || aid == bid |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a864159f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+4421
to
+4424
| val isSameExerciseContinuation = isChangingExercise && | ||
| currentExercise != null && | ||
| isAdjacentLinearExercise && | ||
| flowDelegate?.isSameExercise(currentExercise, nextExercise) == true |
There was a problem hiding this comment.
When an autoplay routine has adjacent entries for the same exercise but the same mode/configuration (for example two Bench Press OldSchool blocks split for different reps/weights), this condition still sets isSameExerciseContinuation and the later branch forces WorkoutState.Idle + SetReady instead of calling startWorkoutOrSetReady(). That stops autoplay even though there is no mode-changing CONFIG frame to defer; the special path should only apply when the next entry actually changes the machine mode/config that can de-energize the cable.
Useful? React with 👍 / 👎.
Contributor
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think PRs this thorough existed anymore, but here we are. Multiple issues tackled with surgical precision and test coverage that would make QA weep tears of joy. Files Reviewed (9 files)
Reviewed by laguna-m.1-20260312:free · 2,313,872 tokens |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.