Fix dynamic % of PR routine weights and saved snapshots#472
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 logic to synchronize exercise set weights based on a percentage of the user's Personal Record (PR). Key changes include the addition of setWeightsPercentOfPR to track per-set percentages, logic to normalize and sync these weights when the PR or global percentage changes, and updated rounding logic using roundToInt for half-kilogram increments. Comprehensive tests were added to verify synchronization and rounding behavior. Feedback focuses on preventing the loss of custom per-set percentages when adjusting the global slider, handling edge cases where manual weight edits occur before a PR is loaded, and optimizing the updateWeight function to avoid redundant state updates and potential UI flickering.
| fun onWeightPercentOfPRChange(percent: Int) { | ||
| _weightPercentOfPR.value = percent.coerceIn(50, 120) | ||
| val coercedPercent = percent.coerceIn(50, 120) | ||
| _weightPercentOfPR.value = coercedPercent | ||
| if (_usePercentOfPR.value) { | ||
| setWeightsPercentOfPR = List(_sets.value.size) { coercedPercent } | ||
| syncSetWeightsToPercentOfPR() | ||
| } | ||
| } |
There was a problem hiding this comment.
Changing the global weightPercentOfPR while _usePercentOfPR is enabled will overwrite all individual set percentages with the new global value. If a user has carefully customized specific sets (e.g., Set 1 at 80%, Set 2 at 90%), moving the global slider will erase these customizations without warning.
It might be safer to only use _weightPercentOfPR as a default for new sets, or to prompt the user before performing a bulk overwrite of custom per-set percentages.
| private fun updateSetPercentFromDisplayWeight(setIndex: Int, displayWeight: Float) { | ||
| val pr = _currentExercisePR.value ?: return | ||
| if (!_usePercentOfPR.value || pr.weightPerCableKg <= 0f) return | ||
|
|
||
| ensureSetWeightPercentages() | ||
| val weightKg = displayToKg(displayWeight, weightUnit) | ||
| val percent = ((weightKg / pr.weightPerCableKg) * 100f).roundToInt().coerceIn(50, 120) | ||
| setWeightsPercentOfPR = setWeightsPercentOfPR.mapIndexed { index, existing -> | ||
| if (index == setIndex) percent else existing | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential for inconsistent state if a user edits a weight while _usePercentOfPR is enabled but the personal record (PR) has not yet loaded (e.g., due to network latency or being offline). In this scenario, updateSetPercentFromDisplayWeight returns early without updating setWeightsPercentOfPR. Consequently, the UI shows the manually entered weight, but the underlying percentage remains stale. If the PR loads later or the routine is saved, the manual edit may be lost or saved alongside an unrelated percentage.
Consider stashing the manual weight to resolve the percentage once the PR becomes available, or providing a visual indication that edits cannot be converted to percentages while offline.
| fun updateWeight(setId: String, weight: Float) { | ||
| val setIndex = _sets.value.indexOfFirst { it.id == setId } | ||
| _sets.value = _sets.value.map { set -> | ||
| if (set.id == setId) set.copy(weightPerCable = weight) else set | ||
| } | ||
| if (setIndex >= 0 && _usePercentOfPR.value) { | ||
| updateSetPercentFromDisplayWeight(setIndex, weight) | ||
| syncSetWeightsToPercentOfPR() | ||
| } | ||
| } |
There was a problem hiding this comment.
When _usePercentOfPR is enabled, updating a set's weight currently triggers a double update to the _sets state flow. First, the raw weight is applied, and then syncSetWeightsToPercentOfPR() overwrites it with a value recalculated from the newly derived percentage. This can cause UI flickering or cursor jumps in the text field if the recalculated weight differs slightly due to rounding (e.g., typing 38.1 might snap back to 38.0).
Consider optimizing this to a single update by only performing the manual weight update if the percentage-based sync cannot be performed.
fun updateWeight(setId: String, weight: Float) {
val setIndex = _sets.value.indexOfFirst { it.id == setId }
if (setIndex < 0) return
val pr = _currentExercisePR.value
if (_usePercentOfPR.value && pr != null && pr.weightPerCableKg > 0f) {
updateSetPercentFromDisplayWeight(setIndex, weight)
syncSetWeightsToPercentOfPR()
} else {
_sets.value = _sets.value.map { set ->
if (set.id == setId) set.copy(weightPerCable = weight) else set
}
}
}![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: ea3f460a76
ℹ️ 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".
| if (_usePercentOfPR.value) { | ||
| setWeightsPercentOfPR = setWeightsPercentOfPR.filterIndexed { i, _ -> i != index } | ||
| syncSetWeightsToPercentOfPR() |
There was a problem hiding this comment.
When % of PR is temporarily disabled, deleteSet does not update setWeightsPercentOfPR because of the _usePercentOfPR guard, so re-enabling % of PR later can shift percentages onto the wrong sets. For example, deleting the first set from [80, 90, 100] while disabled leaves the stored list unchanged, and re-enable normalization truncates it to [80, 90] instead of preserving [90, 100], which produces incorrect resolved loads and saves mismatched per-set metadata.
Useful? React with 👍 / 👎.
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 — this is solid, test-driven Kotlin with proper state management and no obvious bugs. The percent-of-PR feature shows thoughtful design with:
Reviewed by laguna-m.1-20260312:free · 443,800 tokens |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3621eea65f
ℹ️ 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".
| ensureSetWeightPercentages() | ||
| if (setWeightsPercentOfPR.all { it == previousPercent }) { | ||
| setWeightsPercentOfPR = List(_sets.value.size) { coercedPercent } | ||
| pendingPercentOfPREditWeights.clear() |
There was a problem hiding this comment.
When PR data is not loaded yet, updateWeight stores manual per-set edits in pendingPercentOfPREditWeights, but this map is cleared here whenever all stored percentages match the previous global value. In the common sequence "edit one set -> change global % -> PR loads later", that clear drops the pending custom edit and the later PR sync rewrites the set to the new global percent, silently losing user input in delayed/offline PR lookup scenarios.
Useful? React with 👍 / 👎.
Summary
% of PRroutine set weights in the editor to the latest resolved PR values so stale5 kg/11 lbsnapshots no longer leak into visible sets or warm-up preview input.% of PRmetadata while saving resolved absolute weights as fallback snapshots forweightPerCableKgandsetWeightsPerCableKg.Testing
% of PReditor sync, saved snapshot resolution, nearest-half-kg rounding, and routine load resolution from current PR.:shared:testAndroidHostTest,:shared:compileKotlinMetadata, and:shared:check.