feat(#646): label set types in workout UI#647
Conversation
- Pure-Kotlin setTypeLabel() helper returns sealed Calibration | Warmup(n) | Working off existing repCount.warmupReps/isWarmupComplete + coordinator.currentWarmupSetIndex - WorkoutHud ExecutionPage renders label inline under the existing 'Set X / Y' text; rides the existing !isJustLift guard - SetReadyScreen header renders warmup/working label inline with the existing mode picker; calibration label suppressed - 6 locale strings.xml: set_type_calibration | set_type_warmup | set_type_working - WorkoutTab forwards currentWarmupSetIndex from state through to WorkoutHud - 4 pure-Kotlin helper tests; existing 2431-test suite still green Fixes #646
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| // ponytail: 3 mirrors Constants.DEFAULT_WARMUP_REPS; revisit if firmware | ||
| // calibration buffer size ever changes. | ||
| private const val CALIBRATION_BUFFER_REPS = 3 |
There was a problem hiding this comment.
🔥 The Roast: You wrote CALIBRATION_BUFFER_REPS = 3 and then helpfully left a ponytail: comment telling future-you it mirrors Constants.DEFAULT_WARMUP_REPS. That's a Post-it note on a landmine. The engine uses Constants.DEFAULT_WARMUP_REPS in ~10 call sites; the day someone bumps that constant for a firmware change, this helper silently disagrees with the engine and the UI cheerfully labels live ROM reps as "Working Set."
🩹 The Fix: Delete CALIBRATION_BUFFER_REPS and import com.devil.phoenixproject.util.Constants, then reference Constants.DEFAULT_WARMUP_REPS directly in the when clause. One source of truth, one landmine defused. Add a regression test that exercises a non-3 buffer size if you want belt-and-suspenders.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| val setTypeText = when (setType) { | ||
| is SetTypeLabel.Warmup -> stringResource(Res.string.set_type_warmup, setType.setNumber) | ||
| SetTypeLabel.Working -> stringResource(Res.string.set_type_working) | ||
| SetTypeLabel.Calibration -> "" // unreachable; showCalibrationLabel=false |
There was a problem hiding this comment.
🔥 The Roast: Calibration -> "" annotated // unreachable, wrapped in if (setTypeText.isNotEmpty()). The compiler can prove the branch is dead; the comment admits it; the runtime guard defends against it. Three layers of "I know this is awkward" stacked on a helper that knows too much.
🩹 The Fix: Split the helper. Keep setTypeLabel(repCount, currentWarmupSetIndex) returning Calibration | Warmup(n) | Working for the active set (ExecutionPage). Add a sibling readySetTypeLabel(repCount, currentWarmupSetIndex) returning just Warmup(n) | Working for SetReady. The unreachable branch and the isNotEmpty() guard both vanish; the call sites read more honestly.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
Correctness / Safety Findings
Ponytail Review
Ponytail net: -8 lines. Suggested Minimal Patch
That's two files, one behavior-preserving change, one drift fix. No new dependencies, no tests removed. 🏆 Best part: The pure-Kotlin + sealed-type helper. Trivially testable, no Compose runtime, no string-resource coupling at the decision site. That's the right instinct. 💀 Worst part: Hardcoding a value the rest of the engine reads from a shared constant, then writing a 📊 Overall: A tight, well-scoped PR that earns its keep on the helper design — but trips on the one constant it shouldn't have defined. Fix the Final Merge GuidanceCan merge after the warning finding ( Fix these issues in Kilo Cloud Files Reviewed (11 files)
Reviewed by minimax-m3 · Input: 33.8K · Output: 9.6K · Cached: 294.5K |
![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 introduces a set-type badge (Calibration Reps, Warm-up Set, or Working Set) to the workout screens based on the current set state. The review feedback highlights a logic issue where warm-up sets are incorrectly labeled as calibration reps for the first three reps, suggesting a reordering of conditions and an accompanying unit test. Additionally, the reviewer recommends removing redundant, untranslated English string resources from non-English locale files, as they automatically fall back to the default strings file.
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.
| ): SetTypeLabel = when { | ||
| showCalibrationLabel && repCount.warmupReps < CALIBRATION_BUFFER_REPS && !repCount.isWarmupComplete -> | ||
| SetTypeLabel.Calibration | ||
| currentWarmupSetIndex >= 0 -> | ||
| SetTypeLabel.Warmup(currentWarmupSetIndex + 1) | ||
| else -> | ||
| SetTypeLabel.Working | ||
| } |
There was a problem hiding this comment.
During the active set of a warm-up set, the first 3 reps are unweighted calibration reps. However, labeling them as Calibration Reps instead of Warm-up Set N defeats the core purpose of this feature (which is to remind the user that they are in a warm-up set). If a warm-up set is configured with 3 or fewer reps, the user will never see the warm-up label during the active set.
By reordering the conditions so that currentWarmupSetIndex >= 0 takes precedence, we ensure that warm-up sets are always clearly identified as such from the very first rep, while working sets still correctly display Calibration Reps for the first 3 reps.
| ): SetTypeLabel = when { | |
| showCalibrationLabel && repCount.warmupReps < CALIBRATION_BUFFER_REPS && !repCount.isWarmupComplete -> | |
| SetTypeLabel.Calibration | |
| currentWarmupSetIndex >= 0 -> | |
| SetTypeLabel.Warmup(currentWarmupSetIndex + 1) | |
| else -> | |
| SetTypeLabel.Working | |
| } | |
| ): SetTypeLabel = when { | |
| currentWarmupSetIndex >= 0 -> | |
| SetTypeLabel.Warmup(currentWarmupSetIndex + 1) | |
| showCalibrationLabel && repCount.warmupReps < CALIBRATION_BUFFER_REPS && !repCount.isWarmupComplete -> | |
| SetTypeLabel.Calibration | |
| else -> | |
| SetTypeLabel.Working | |
| } |
|
|
||
| @Test | ||
| fun working_when_no_warmup_index_at_setready() { | ||
| val rc = RepCount(warmupReps = 0, isWarmupComplete = false) | ||
| val label = setTypeLabel(rc, currentWarmupSetIndex = -1, showCalibrationLabel = false) | ||
| assertEquals(SetTypeLabel.Working, label) | ||
| } |
There was a problem hiding this comment.
Add a unit test to verify and pin the precedence behavior where warm-up sets take precedence over calibration reps during the active set.
| @Test | |
| fun working_when_no_warmup_index_at_setready() { | |
| val rc = RepCount(warmupReps = 0, isWarmupComplete = false) | |
| val label = setTypeLabel(rc, currentWarmupSetIndex = -1, showCalibrationLabel = false) | |
| assertEquals(SetTypeLabel.Working, label) | |
| } | |
| @Test | |
| fun working_when_no_warmup_index_at_setready() { | |
| val rc = RepCount(warmupReps = 0, isWarmupComplete = false) | |
| val label = setTypeLabel(rc, currentWarmupSetIndex = -1, showCalibrationLabel = false) | |
| assertEquals(SetTypeLabel.Working, label) | |
| } | |
| @Test | |
| fun warmup_takes_precedence_over_calibration_during_active_set() { | |
| val rc = RepCount(warmupReps = 0, isWarmupComplete = false) | |
| val label = setTypeLabel(rc, currentWarmupSetIndex = 0, showCalibrationLabel = true) | |
| assertEquals(SetTypeLabel.Warmup(setNumber = 1), label) | |
| } | |
| } |
| <!-- ==================== Set type labels (Issue #646) ==================== --> | ||
| <string name="set_type_calibration">Calibration Reps</string> | ||
| <string name="set_type_warmup">Warm-up Set %1$d</string> | ||
| <string name="set_type_working">Working Set</string> |
There was a problem hiding this comment.
In Android and Compose Multiplatform, string resources automatically fall back to the default values/strings.xml if they are missing from locale-specific files. Explicitly adding untranslated English strings to non-English locale files is redundant, pollutes the translation files, and makes it harder for translation tools/translators to identify untranslated keys. These should be removed and only added when actual German translations are available.
| <!-- ==================== Set type labels (Issue #646) ==================== --> | ||
| <string name="set_type_calibration">Calibration Reps</string> | ||
| <string name="set_type_warmup">Warm-up Set %1$d</string> | ||
| <string name="set_type_working">Working Set</string> |
There was a problem hiding this comment.
In Android and Compose Multiplatform, string resources automatically fall back to the default values/strings.xml if they are missing from locale-specific files. Explicitly adding untranslated English strings to non-English locale files is redundant, pollutes the translation files, and makes it harder for translation tools/translators to identify untranslated keys. These should be removed and only added when actual Spanish translations are available.
| <!-- ==================== Set type labels (Issue #646) ==================== --> | ||
| <string name="set_type_calibration">Calibration Reps</string> | ||
| <string name="set_type_warmup">Warm-up Set %1$d</string> | ||
| <string name="set_type_working">Working Set</string> |
There was a problem hiding this comment.
In Android and Compose Multiplatform, string resources automatically fall back to the default values/strings.xml if they are missing from locale-specific files. Explicitly adding untranslated English strings to non-English locale files is redundant, pollutes the translation files, and makes it harder for translation tools/translators to identify untranslated keys. These should be removed and only added when actual French translations are available.
| <!-- ==================== Set type labels (Issue #646) ==================== --> | ||
| <string name="set_type_calibration">Calibration Reps</string> | ||
| <string name="set_type_warmup">Warm-up Set %1$d</string> | ||
| <string name="set_type_working">Working Set</string> |
There was a problem hiding this comment.
In Android and Compose Multiplatform, string resources automatically fall back to the default values/strings.xml if they are missing from locale-specific files. Explicitly adding untranslated English strings to non-English locale files is redundant, pollutes the translation files, and makes it harder for translation tools/translators to identify untranslated keys. These should be removed and only added when actual Italian translations are available.
| <!-- ==================== Set type labels (Issue #646) ==================== --> | ||
| <string name="set_type_calibration">Calibration Reps</string> | ||
| <string name="set_type_warmup">Warm-up Set %1$d</string> | ||
| <string name="set_type_working">Working Set</string> |
There was a problem hiding this comment.
In Android and Compose Multiplatform, string resources automatically fall back to the default values/strings.xml if they are missing from locale-specific files. Explicitly adding untranslated English strings to non-English locale files is redundant, pollutes the translation files, and makes it harder for translation tools/translators to identify untranslated keys. These should be removed and only added when actual Dutch translations are available.
![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: 6955e3afeb
ℹ️ 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".
| } | ||
|
|
||
| // Issue #646: set-type badge (Calibration Reps / Warm-up Set N / Working Set) | ||
| val setType = setTypeLabel(repCount, currentWarmupSetIndex, showCalibrationLabel = true) |
There was a problem hiding this comment.
When a routine reaches a bodyweight exercise while connected, ActiveSessionEngine.startWorkout configures warmupTarget = 0 and stores a fresh RepCount() with isWarmupComplete = false, so this unconditional showCalibrationLabel = true makes setTypeLabel return Calibration before the bodyweight timer branch renders. The HUD then labels a bodyweight set as “Calibration Reps” even though no firmware calibration reps run for bodyweight exercises; pass the exercise type through here or disable the calibration label for isCurrentExerciseBodyweight.
Useful? React with 👍 / 👎.
CI + build statusAll 10 CI checks pass on this PR — Build Android, Unit Tests, Shared Module Tests, Android App Unit Tests, iOS Test Target Compile, iOS Schema Sync, Lint, Security Hygiene, GitGuardian, Kilo Code Review. Local verification:
Visual evidence blocker (heads-up)The original spec asks for 7 PNGs of the implemented UI on the AVD ( I considered adding a debug mock-state path that pre-populates The Available evidence right now:
To unblock final-visual capture the reporter or a tester with access to a real Vitruvian can:
Or: if the team wants me to add a debug-only mock-state path for visual capture, give the green light and I'll wire it as a separate Scope reminderPer the implementation spec, the bounded diff is:
Forbidden scope explicitly NOT touched: |
1 participant
What
Labels each set in the workout UI as Calibration Reps (first 3 ROM/calibration reps), Warm-up Set N (variable warm-up phase), or Working Set (routine default). Visible on the active workout HUD below the existing "Set X / Y" text and inline in the Set Ready header. Just Lift is unchanged.
Why
Issue #646 reporter frequently forgets they have Warm-up Sets enabled for some exercises because the UI doesn't currently distinguish the firmware 3-rep ROM buffer (labeled "REP" / "WARMUP" in the rep counter) from variable warm-up sets and from working sets.
Scope (bounded)
setTypeLabel(repCount, currentWarmupSetIndex, showCalibrationLabel)returning sealedCalibration | Warmup(n) | Working— no Compose runtime, no string resources, trivially testable.ExecutionPage's existing!isJustLift && exerciseName != nullblock (reuses existing weight, color, and spacing).SetReadyScreenheader next to the existing set picker (reuses existing mode picker typography).currentWarmupSetIndex: Int = -1plumbedWorkoutTab→WorkoutHud→ExecutionPage.SetReadyScreencollectsviewModel.currentWarmupSetIndexdirectly.showCalibrationLabel = falseflag at the SetReady call site (calibration reps run during the active set, not at SetReady).Not in scope (unchanged)
ActiveSessionEngine,RoutineFlowManager,WorkoutCoordinatoruntouchedCompatibility
Risks
currentWarmupSetIndexandrepCountfields the engine already uses for runtime behavior.showCalibrationLabel = falseflag at that call site.3in the helper is tied toConstants.DEFAULT_WARMUP_REPS;ponytail:comment in code documents the coupling.Tests
SetTypeLabelTest.kt— 4 pure-Kotlin helper tests, all green.:shared:testAndroidHostTestsuite (2431 tests) — 0 failures.Visual Evidence
To be added as a PR comment once emulator captures are complete (
final-visuals/android/*.png).References
phoenix-enhancements/issue-646-1525152789725188258/architecture.mdphoenix-enhancements/issue-646-1525152789725188258/integration-model.mdphoenix-enhancements/issue-646-1525152789725188258/implementation-spec.mdphoenix-enhancements/issue-646-1525152789725188258/signoff.mdFixes #646