Add next-set weight recommendation suggestions#499
Conversation
![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 conservative, suggestion-only next-set weight recommendation feature (Issue #424). It adds the RecommendWeightAdjustmentUseCase to compute weight adjustments based on completed-set quality and biomechanics data, alongside a WeightRecommendationCard UI component on the SetReadyScreen to apply or dismiss suggestions. The feature is integrated into the session management flow and includes a toggle in the settings. Feedback on the changes points out a logical gap where missing target reps by a large margin with high quality still recommends maintaining weight, a missing edge case check for zero completed reps, and several instances of hardcoded user-facing strings that violate internationalization (i18n) standards.
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.
| return maintain( | ||
| input = input, | ||
| reasonCode = "QUALITY_MIXED_MAINTAIN", | ||
| explanation = "Keep this weight - rep quality was mixed.", | ||
| confidence = if (completedTarget) RecommendationConfidence.MEDIUM else RecommendationConfidence.LOW, | ||
| ) |
There was a problem hiding this comment.
There is a logical gap when a user fails to complete their target reps by a significant margin (e.g., completing only 1 or 2 reps out of a 10-rep target), but those few reps are of very high quality (e.g., averageScore >= MIXED_QUALITY_MIN).
Currently, this scenario falls through to line 73 and recommends MAINTAIN with QUALITY_MIXED_MAINTAIN (with LOW confidence). Recommending to keep the same weight when the user missed their target reps by 80-90% is counter-intuitive and could lead to injury or overtraining.
Suggestion:
Consider adding a check to ensure that if the target was missed by a significant margin (e.g., actualReps < targetReps * 0.5 or a similar threshold), the engine either recommends a DECREASE or suppresses the recommendation entirely (return null).
| if (input.isBodyweight || !input.hasNextSetTarget) return null | ||
| if (input.currentWeightKgPerCable < Constants.MIN_WEIGHT_KG) return null | ||
| if (input.weightIncrementKg <= 0f) return null | ||
| if (input.qualitySummary == null && input.biomechanicsSummary == null) return null |
There was a problem hiding this comment.
If the user completes 0 reps (e.g., they started and immediately stopped the set), we do not have any actual performance data to base a recommendation on. Currently, if qualitySummary or biomechanicsSummary are non-null but empty, the code might still proceed.
Suggestion:
Add an early return check for input.actualReps <= 0 to safely ignore sets with no completed reps.
| if (input.isBodyweight || !input.hasNextSetTarget) return null | |
| if (input.currentWeightKgPerCable < Constants.MIN_WEIGHT_KG) return null | |
| if (input.weightIncrementKg <= 0f) return null | |
| if (input.qualitySummary == null && input.biomechanicsSummary == null) return null | |
| if (input.isBodyweight || !input.hasNextSetTarget || input.actualReps <= 0) return null | |
| if (input.currentWeightKgPerCable < Constants.MIN_WEIGHT_KG) return null | |
| if (input.weightIncrementKg <= 0f) return null | |
| if (input.qualitySummary == null && input.biomechanicsSummary == null) return null |
| val headline = when (recommendation.direction) { | ||
| WeightAdjustmentDirection.INCREASE -> "Increase next set" | ||
| WeightAdjustmentDirection.DECREASE -> "Reduce next set" | ||
| WeightAdjustmentDirection.MAINTAIN -> "Keep next set steady" | ||
| } |
There was a problem hiding this comment.
Hardcoded user-facing strings violate the internationalization (i18n) standards of the project. The codebase already utilizes stringResource(Res.string....) for localization (as seen in SetReadyScreen.kt).
Suggestion:
Externalize these strings to the project's resource files and load them dynamically.
(Note: Also apply this to other hardcoded strings in this file, such as "per cable" on line 93, "Dismiss" on line 115, and "Apply" on line 118).
| val headline = when (recommendation.direction) { | |
| WeightAdjustmentDirection.INCREASE -> "Increase next set" | |
| WeightAdjustmentDirection.DECREASE -> "Reduce next set" | |
| WeightAdjustmentDirection.MAINTAIN -> "Keep next set steady" | |
| } | |
| val headline = when (recommendation.direction) { | |
| WeightAdjustmentDirection.INCREASE -> stringResource(Res.string.weight_recommendation_increase) | |
| WeightAdjustmentDirection.DECREASE -> stringResource(Res.string.weight_recommendation_decrease) | |
| WeightAdjustmentDirection.MAINTAIN -> stringResource(Res.string.weight_recommendation_maintain) | |
| } |
| Text( | ||
| "Weight Suggestions", | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| fontWeight = FontWeight.Medium, | ||
| color = MaterialTheme.colorScheme.onSurface, | ||
| ) | ||
| Spacer(modifier = Modifier.height(4.dp)) | ||
| Text( | ||
| "Show Apply or Dismiss recommendations after completed sets", | ||
| style = MaterialTheme.typography.bodySmall, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) |
There was a problem hiding this comment.
The newly added settings labels are hardcoded, which violates the project's internationalization (i18n) standards.
Suggestion:
Use stringResource to load these strings from the localized resources.
| Text( | |
| "Weight Suggestions", | |
| style = MaterialTheme.typography.bodyLarge, | |
| fontWeight = FontWeight.Medium, | |
| color = MaterialTheme.colorScheme.onSurface, | |
| ) | |
| Spacer(modifier = Modifier.height(4.dp)) | |
| Text( | |
| "Show Apply or Dismiss recommendations after completed sets", | |
| style = MaterialTheme.typography.bodySmall, | |
| color = MaterialTheme.colorScheme.onSurfaceVariant, | |
| ) | |
| Text( | |
| stringResource(Res.string.settings_weight_suggestions_title), | |
| style = MaterialTheme.typography.bodyLarge, | |
| fontWeight = FontWeight.Medium, | |
| color = MaterialTheme.colorScheme.onSurface, | |
| ) | |
| Spacer(modifier = Modifier.height(4.dp)) | |
| Text( | |
| stringResource(Res.string.settings_weight_suggestions_subtitle), | |
| style = MaterialTheme.typography.bodySmall, | |
| color = MaterialTheme.colorScheme.onSurfaceVariant, | |
| ) |
![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: 4af4412a22
ℹ️ 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".
| targetSetIndex = nextStep.second, | ||
| targetReps = params.reps, | ||
| actualReps = completedReps, | ||
| currentWeightKgPerCable = params.weightPerCableKg, |
There was a problem hiding this comment.
When a routine programs different weights per set, this builds the recommendation from the completed set's params.weightPerCableKg even though the recommendation targets nextStep. The rest/manual transition code loads the next set's planned setWeightsPerCableKg, so applying a high-quality set-1 recommendation can overwrite a planned heavier set 2 (for example 20kg -> planned 30kg becomes 22.5kg). Please base the suggestion on the target set's planned weight or suppress it when the next set already has a distinct programmed weight.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| operator fun invoke(input: WeightAdjustmentInput): WeightAdjustmentRecommendation? { | ||
| if (input.isBodyweight || !input.hasNextSetTarget) return null | ||
| if (input.currentWeightKgPerCable < Constants.MIN_WEIGHT_KG) return null |
There was a problem hiding this comment.
🔥 The Roast: This early return check < Constants.MIN_WEIGHT_KG is a masterclass in writing code that looks defensive but does absolutely nothing. With MIN_WEIGHT_KG = 0f, this condition weight < 0 will never be true for any legitimate cable exercise weight. It's like a bouncer checking if you're under -5 feet tall.
🩹 The Fix: Either remove this redundant check, or if you want to catch unconfigured/edge-case weights, change it to <= 0f to catch actual problematic values. The constant should probably be MIN_PRACTICAL_WEIGHT_KG = 0.5f (machine's minimum increment) rather than 0.
📏 Severity: suggestion
| } | ||
|
|
||
| if (quality != null) { | ||
| val finalRepScore = quality.repScores.maxByOrNull { it.repNumber }?.composite |
There was a problem hiding this comment.
🔥 The Roast: maxByOrNull { it.repNumber } to find the "final rep" is clever but reads like you're finding the rep with the maximum number (which happens to be the last one if they're sequential). Future maintainers will need a treasure map to understand this logic.
🩹 The Fix: Add a comment clarifying this finds the last/highest-numbered rep, or use repScores.maxByOrNull { it.repNumber } with an explanatory comment: // Last rep has highest repNumber.
📏 Severity: nitpick
|
|
||
| // Bodyweight = no cable accessories (handles, bar, rope, etc.) in equipment list | ||
| val isBodyweight = !currentExercise.exercise.hasCableAccessory | ||
| val matchingWeightRecommendation = weightRecommendation?.takeIf { recommendation -> |
There was a problem hiding this comment.
🔥 The Roast: The takeIf predicate is checking !isBodyweight but you already have that context from line 120. While not technically wrong, this redundant check adds mental overhead - like checking if a fish has lungs before giving it water.
🩹 The Fix: The check is defensive and ensures the recommendation is appropriate even if state changes. It's acceptable, but consider removing !isBodyweight since isBodyweight is already computed and the recommendation shouldn't target bodyweight exercises anyway (filtered in updateWeightRecommendationForCompletedSet).
📏 Severity: nitpick
Code Review Roast 🔥Verdict: No New Issues | Recommendation: Merge when ready (address carried-forward issues) Overview
🏆 Best part: This feature addition is impressively thorough - new use case, state management, UI component, settings integration, and comprehensive test coverage including integration tests. The 💀 Previous issues still outstanding (carry-forward from initial review):
📊 Overall: Like a well-planned workout program - the progression is logical, the form is solid, and someone already pointed out where the previous reps were sloppy. Fix the carried-forward issues when you get a chance, but this PR is merge-worthy. Files Reviewed (12 files)
Reviewed by laguna-m.1-20260312:free · 1,106,893 tokens |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 651ce3eb11
ℹ️ 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".
| exerciseName = targetExercise?.exercise?.name ?: currentExercise?.exercise?.name, | ||
| targetExerciseId = targetExerciseId, | ||
| targetSetIndex = nextStep.second, | ||
| targetReps = params.reps, |
There was a problem hiding this comment.
When a routine programs different reps per set, this still evaluates the recommendation against the completed set's params.reps instead of the target set's configured reps. For example, after completing a high-quality 5-rep set before a planned 10-rep set, the card can recommend increasing the next set's weight even though the next target is much harder; if the next set is AMRAP (setReps[...] == null), it can still show a target-based suggestion. Please read the target set reps from targetExercise.setReps.getOrNull(nextStep.second) and suppress recommendations when that target is absent.
Useful? React with 👍 / 👎.
Both branches independently added new dependencies to the same constructor parameter lists (RecommendWeightAdjustmentUseCase from the PR branch, EquipmentRackRepository + ApplyEquipmentRackLoadUseCase from main). Resolution keeps both features intact across all 11 conflicted files. https://claude.ai/code/session_018ubmbkr2vYycJueywZnoC2
Resolve merge conflicts with weight recommendation (PR #499), timed-set countdown cues (PR #505), and Health Connect export (PR #516) by keeping both deload modifiers and the new main features. https://claude.ai/code/session_01MBWwVaUrnXjzZxYKKnn33i
Summary
Testing