Skip to content

fix: replace exercise catalogue with free-exercise-db - #706

Merged
9thLevelSoftware merged 21 commits into
mainfrom
fix/vitruvian-catalog-replacement
Aug 21, 2026
Merged

fix: replace exercise catalogue with free-exercise-db#706
9thLevelSoftware merged 21 commits into
mainfrom
fix/vitruvian-catalog-replacement

Conversation

@9thLevelSoftware

Copy link
Copy Markdown
Owner

Remove the bundled dump, streamed mux/jwplayer demos, and docs. Seed the library from openly licensed free-exercise-db stills, archive legacy catalogue rows so workout history still resolves, and add an optional wger refresh behind Settings.

Remove the bundled dump, streamed mux/jwplayer demos, and decompilation
docs. Seed the library from openly licensed free-exercise-db stills,
archive legacy catalogue rows so workout history still resolves, and
add an optional wger refresh behind Settings.
Copilot AI lite review requested due to automatic review settings August 20, 2026 23:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

Comment thread shared/src/commonMain/composeResources/values/strings.xml
Comment thread shared/src/commonMain/composeResources/values/strings.xml
@kilo-code-bot

kilo-code-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Code Review Roast 🔥

Verdict: 1 Suggestion | Recommendation: Merge — Ponytail nit only

Oh wait, this incremental is almost clean. I had my flamethrower warmed up and everything, but the new mergeExerciseMvtCollisions does a proper weighted average, the resolvePersonalRecordCollisions rewrite uses compareBy from stdlib, and the six new tests cover every branch. The only smudge is a hand-rolled English stemmer that nameAliases already covers.

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 1
🤏 nitpick 0
🩹 minor 0
Issue Details (click to expand)
File Line Roast
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/LegacyCatalogueIdMap.kt 149 stemKey is a hand-rolled English stemmer (~20 lines of ies/ches/shes/sses/xes/s suffix rules). For every known singular/plural rename, nameAliases already provides explicit, reviewable mapping. The stemmer is a fallback for a fallback that never fires.

🏆 Best part: mergeExerciseMvtCollisions does a proper weighted average ((old * oldCount + new * newCount) / totalCount), correctly handles the edge case where both sides have 0 samples (picks the newer updatedAt), and deletes the old row before reassignExerciseMvtExerciseId runs so the bulk UPDATE is a no-op for already-merged profiles. The maxOf(updatedAt) preserves the most recent timestamp. This is the right shape for merging personal MVT data and the second-pass test proves it's idempotent.

💀 Worst part: stemKey is a hand-rolled English stemmer with enough suffix rules to make a linguist wince. It's a "just in case" fallback that never fires because nameAliases covers all known renames. The activeByStem construction and lookup add 6 lines that the stemmer's own callers don't need.

📊 Overall: Like the third pancake — the batter is right, the flip is clean, and you didn't overcook it. The incremental is small, correct, and well-tested. The previous round's consumeLegacyExerciseUserFields + compareBy rewrite laid the groundwork; this round adds the last-mile coverage (duplicate IDs, live/tombstone, MAX_VOLUME, MVT merge, renamed rack pull).

Files Reviewed (4 files)
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt — no new findings (mergeExerciseMvtCollisions is correct, compareBy rewrite is clean, mappedTargetByName handles duplicate explicit mappings correctly)
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/LegacyCatalogueIdMap.kt — 1 Ponytail finding (stemKey is YAGNI; nameAliases covers all known renames)
  • shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq — no new findings (selectExerciseMvtByExerciseId is needed for the merge; deleteConflictingExerciseMvt removal is correct)
  • shared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightExerciseRepositoryTest.kt — no new findings (6 new tests cover duplicate ID, live/tombstone, renamed, heavier-collision-both-directions, MAX_VOLUME, and MVT merge; insertPr helper is clean and reusable)

Fix these issues in Kilo Cloud

Previous Review Summaries (7 snapshots, latest commit 4cc665b)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 4cc665b)

Verdict: 0 Critical Issues | Recommendation: Merge — Ponytail nits only

The previous round's timesPerformed-on-every-open bonfire, the user-archive revival, and the unreachable defensive check are all genuinely fixed. consumeLegacyExerciseUserFields zeros the legacy row's user fields after merging, selectPersonalRecordsByExerciseId + resolvePersonalRecordCollisions replaces the lossy SQL JOIN with metric-aware arbitration, and the second-pass test proves the whole thing is idempotent. The new code is small, correct, and does what the title promises.

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 1
🤏 nitpick 1
🩹 minor 1
Issue Details (click to expand)
File Line Roast
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt 398 Eight lines of ||-chained tiebreakers where the third clause's first two conjuncts are redundant — compareBy already knows how to chain.
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt 406 Five-line if/else whose only job is picking which id to delete. Kotlin ternary is right there.
shared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightExerciseRepositoryTest.kt 448 New idempotency test covers user-field merge but leaves resolvePersonalRecordCollisions (four branches) on vibes.

🏆 Best part: consumeLegacyExerciseUserFields is the correct fix shape — merge once, zero the source, then reassign. The previous "additive merge every time" was a landmine, and this lands the disarm without bloating mergeLegacyExerciseUserFields itself. Plus the order of operations (merge → consume → reassign) is exactly the order that keeps the transaction honest.

💀 Worst part: The new resolvePersonalRecordCollisions rolls its own metric comparison by hand when compareBy does the same thing in three lines. It's not wrong, it's just a missed opportunity for the Kotlin stdlib to do the typing.

📊 Overall: Like the second pancake — same batter as the first, but you flipped it before it burned. The consumeLegacyExerciseUserFields + second-pass test combo is the kind of fix that ages well.

Files Reviewed (3 files)
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt — 2 findings (Ponytail shrink on tiebreaker + if/else)
  • shared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightExerciseRepositoryTest.kt — 1 finding (test gap for new function)
  • shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq — no new findings (removed archived = 0 and deleteConflictingPersonalRecords cleanly; added consumeLegacyExerciseUserFields and selectPersonalRecordsByExerciseId are well-shaped)

Fix these issues in Kilo Cloud

Previous review (commit ba0b84c)

Verdict: 1 Issue Found | Recommendation: Address before merge

The remap pipeline is the right shape — explicit id map + name fallback + per-table reassign + user-field merge + duplicate-PR dedupe — but it's wired up to fire on every importExercises() call, and one branch of the merge isn't idempotent. Translation: a user with archived legacy rows will see their timesPerformed stat climb like a fitness app that ran out of digits.

Overview

Severity Count
🚨 critical 0
⚠️ warning 1
💡 suggestion 1
🤏 nitpick 2
💁 ponytail 2
Issue Details (click to expand)
File Line Roast
SqlDelightExerciseRepository.kt 160 remapLegacyCatalogueIds() runs every call, timesPerformed merge is additive → stat inflates on every picker open
VitruvianDatabase.sq 618 archived = 0 in catalog update silently revives user-archived stock rows
ExerciseImporter.kt 354 explicit != row.id defensive check against a state the data never enters
ExerciseImporter.kt 374 Twelve hand-typed queries.reassign…ExerciseId lines begging for a list iteration

🏆 Best part: The user-field merge logic in mergeLegacyExerciseUserFields is genuinely clever — MAX(new, old) semantics for lastPerformed and one_rep_max_kg, COALESCE for mvtOverrideMs, and a guard so the legacy row's isFavorite = 1 wins if either side is favourited. That's the kind of "what does the user actually want?" SQL you usually don't see.

💀 Worst part: The same merge that gets lastPerformed and one_rep_max_kg right uses additive timesPerformed = new + old semantics — and then it's called on every importExercises(). The defensive coding instinct was strong elsewhere; it just took the day off for the only field where it mattered.

📊 Overall: Like installing a beautiful new boiler that also happens to drip into the basement every time you turn on a tap. Fix the wiring, leave the boiler.

Correctness / Safety Findings

  • warning: SqlDelightExerciseRepository.kt:160remapLegacyCatalogueIds() is unconditional and mergeLegacyExerciseUserFields.timesPerformed is additive, so every picker / routine / single-exercise open inflates legacy timesPerformed by the archived row's count. Required fix: gate the call on first-import (move inside the if (currentSource != BUNDLED_CATALOG_SOURCE) branch) or make the merge idempotent.
  • suggestion: VitruvianDatabase.sq:618archived = 0 in updateCatalogExercise re-activates user-archived stock rows on every reimport. Drop the archived = 0 clause; insertExerciseIfAbsent already covers fresh rows with the desired default.
  • minor: ExerciseImporter.kt:354explicit != row.id guard is unreachable; LegacyCatalogueIdMap.explicit keys are by construction different from their values.
  • minor: ExerciseImporter.kt:374 — twelve near-identical reassign…ExerciseId calls could iterate over a constant list of queries.

Ponytail Review

shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq:L668: shrink — mergeLegacyExerciseUserFields is a 20-line UPDATE with five correlated subqueries on the same :oldId. If the repo is ever asked to merge more than a handful of legacy rows, those subqueries add up. For the current 29-entry map it's fine; flagging as a "if this map grows, push it into Kotlin" marker rather than a fix-now.

shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt:L342: yagni — remapLegacyCatalogueIds is public on the importer but has exactly one caller (this PR's SqlDelightExerciseRepository.importExercises). Mark internal or private until a second use appears. Optional, but the visibility is wider than the call graph.

Ponytail net: -3 lines.

Suggested Minimal Patch

  1. In SqlDelightExerciseRepository.kt:149-159, move exerciseImporter.remapLegacyCatalogueIds() inside the if (currentSource != ExerciseImporter.BUNDLED_CATALOG_SOURCE) block, immediately after preferencesManager.setExerciseCatalogSource(...). One-line move, zero behaviour change for fresh installs, no more compounding timesPerformed.
  2. In VitruvianDatabase.sq:618, delete the archived = 0 line from updateCatalogExercise. The bundled import already sets archived = 0L in insertExerciseIfAbsent, so fresh rows still get the right value.
  3. In ExerciseImporter.kt:354, drop the && explicit != row.id clause.
  4. (Optional) Collapse the twelve reassign calls into a list iteration.

Final Merge Guidance

Can merge after the timesPerformed doubling fix. Everything else is hygiene. The archived = 0 reversal is a separate UX call — leaving it as-is keeps the old behaviour; removing it makes the new code honest about preserving user archive choices.

Fix these issues in Kilo Cloud

Previous review (commit e562d9b)

Verdict: No Issues Found | Recommendation: Merge

Five surgical commits, each one responding to a different raised hand from the last round. The orphan-key relapse is in remission, the legacy catalogue IDs get the same canonical treatment as fresh slugs, and a foam roll no longer pretends to need a cable machine. The PR is finally behaving like a person who reads review comments.

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 0
🤏 nitpick 0
💁 ponytail 1
What changed since the last review (click to expand)
  • Resolved: FiveThreeOneRoutineDetector.mainLiftId now routes both replacement slugs and the four legacy catalogue IDs (ZZ92N8QsBdp6HCh3, 0040d53f-…, UjIGHxCav-lS9B2I, e64c7837-…) through the same canonical set, so existing 5/3/1 cycles regenerate again. RegenerateFiveThreeOneRoutinesUseCase carries the stored row's ID alongside the canonical ID so the TM bump lands on the legacy row that's actually present.
  • Resolved: CycleTemplates.GLUTE_KICKBACKS now points at One-Legged_Cable_Kickback (cable equipment) instead of the body-only Glute_Kickback. The Upper/Lower TUT percentage lift stays a machine movement.
  • Resolved: ExerciseImporter writes isBodyweight=1L only for the BODYWEIGHT token, 0L when the equipment matches Exercise.CABLE_ACCESSORIES, and NULL for everything else. Foam rolls, exercise balls, and other unclassifiable gear keep Exercise.isBodyweight's !hasCableAccessory derivation.
  • Resolved: ExercisePicker now offers a Cable chip mapped to CABLE, and ExerciseFilterShelf renders it. Face Pull and other generic cable rows are selectable.

🏆 Best part: The behaviour change in storedIsBodyweightFlag is the right one — storing NULL instead of 0L for non-cable, non-bodyweight equipment lets the consumer derive isBodyweight from hasCableAccessory consistently with custom exercises. The schema comment in VitruvianDatabase.sq ("NULL = derive from equipment; 0 = cable; 1 = bodyweight") is now actually honoured.

💀 Worst part: The new test import leaves non-cable equipment bodyweight derivation unset only asserts the boolean view. If Exercise.hasCableAccessory ever drifted (say, somebody added "FOAM ROLL" to CABLE_ACCESSORIES because they read "roll" and panicked), this test would silently flip isBodyweight and you'd have no warning. A direct read of the stored column would catch it.

📊 Overall: Five commits, five clean fixes, zero regressions. The pony is loose, well-fed, and finally doing the trick.

Ponytail note (click to expand)
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/FiveThreeOneRoutineDetector.kt:17-26: shrinkcanonicalLiftById lists BENCH_ID → BENCH_ID, SHOULDER_PRESS_ID → SHOULDER_PRESS_ID, SQUAT_ID → SQUAT_ID, DEADLIFT_ID → DEADLIFT_ID as well as the four legacy mappings. The first four are reachable by exerciseId in MAIN_LIFT_IDS. A legacy-only map plus a MAIN_LIFT_IDS fallback in canonicalMainLiftId expresses the same behaviour with materially less code:
private val legacyToCanonical = mapOf(
    LEGACY_BENCH_ID to BENCH_ID,
    LEGACY_SHOULDER_PRESS_ID to SHOULDER_PRESS_ID,
    LEGACY_SQUAT_ID to SQUAT_ID,
    LEGACY_DEADLIFT_ID to DEADLIFT_ID,
)

fun canonicalMainLiftId(exerciseId: String?): String? = exerciseId?.let {
    legacyToCanonical[it] ?: if (it in MAIN_LIFT_IDS) it else null
}

Optional — only worth doing if you anticipate more legacy IDs or want one source of truth for the canonical set.

Files Reviewed (11 files)
  • shared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightExerciseRepositoryTest.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/CycleTemplates.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/Exercise.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/FiveThreeOneRoutineDetector.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/domain/usecase/RegenerateFiveThreeOneRoutinesUseCase.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/components/ExercisePicker.kt — 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/components/exercisepicker/ExerciseFilterShelf.kt — 0 issues
  • shared/src/commonTest/kotlin/com/devil/phoenixproject/domain/usecase/RegenerateFiveThreeOneRoutinesUseCaseTest.kt — 0 issues
  • shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/components/exercisepicker/ExercisePickerFiltersTest.kt — 0 issues
  • shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngineIntegrationTest.kt — 0 issues

Previous review (commit c43b083)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually clean. I had my flamethrower warmed up for a fifth orphan localization key and the author just… shipped the missing <string> to all five locales like a person who reads code review comments.

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 0
🤏 nitpick 0
What changed since the last review (click to expand)
  • Resolved: settings_exercise_demos_disabled orphan key (was critical at WorkoutHud.kt:771). Now defined in values/strings.xml plus values-de, values-es, values-fr, values-nl.
  • Resolved: WorkoutHudVideoWiringTest now asserts against the resource key (settings_exercise_demos_disabled) instead of the stale English literal.
  • Resolved: ExercisePicker now honors enableVideoPlayback end-to-end. GroupedExerciseList accepts the flag, LaunchedEffect short-circuits the getImages() fetch when off, and onThumbnailClick is nulled so the picker thumbnail / ExerciseImageDialog cannot open when demos are disabled. ExercisePickerContent's dialog render guard and the inner onShowVideo lambda both gate on the flag.

🏆 Best part: The fix is actually localized this time. Five locale files, one consistent message, no more decorative <string> tags. The resource discipline finally matches the file count.

💀 Worst part: The redundant if (enableVideoPlayback) inside the onShowVideo lambda in ExercisePicker.kt is dead defensive code — the row's onThumbnailClick is already null when the flag is off, so the lambda can't be invoked. Harmless, but a four-line "just in case" that the type system already prevents.

📊 Overall: Like a recovery arc I didn't expect to see. The PR kept shipping orphan keys like it was a subscription box, and now it ships five matching translations in one commit. The pony is loose.

Files Reviewed (8 files)
  • shared/src/commonMain/composeResources/values/strings.xml - 0 issues
  • shared/src/commonMain/composeResources/values-de/strings.xml - 0 issues
  • shared/src/commonMain/composeResources/values-es/strings.xml - 0 issues
  • shared/src/commonMain/composeResources/values-fr/strings.xml - 0 issues
  • shared/src/commonMain/composeResources/values-nl/strings.xml - 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/components/ExercisePicker.kt - 0 issues
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/components/exercisepicker/GroupedExerciseList.kt - 0 issues
  • shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/WorkoutHudVideoWiringTest.kt - 0 issues

Previous review (commit 0bceb5f)

Verdict: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
🚨 critical 1
⚠️ warning 0
💡 suggestion 0
🤏 nitpick 0
Issue Details (click to expand)
File Line Roast
shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/screen/WorkoutHud.kt 771 Half-applied localization fix: settings_exercise_demos_disabled is referenced via stringResource(...) but never defined in any strings.xml. Build-time resource lookup will fail on Android, iOS, and host tests.

🏆 Best part: The intent is finally right — the literal is gone and the codebase wants to be localized.

💀 Worst part: This is the fourth orphan key this PR has shipped across its own commits, after previously being roasted for exactly that. The pattern isn't a bug anymore; it's a tradition.

📊 Overall: A PR that's one missing <string> tag away from shipping, and the PR description claims this is the final commit. Add the resource, run the build, then ship.

Files Reviewed (1 file)
  • shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/screen/WorkoutHud.kt - 1 issue

Fix these issues in Kilo Cloud

Previous review (commit 48fc8d1)

Verdict: 1 Issue Found | Recommendation: Merge (optional cleanup)

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 1
🤏 nitpick 0
Issue Details (click to expand)
File Line Roast
shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/screen/WorkoutHud.kt 771 Three commits ago this PR roped itself for hardcoding "Refreshing…" next to localized siblings and adopted settings_refresh_wger_catalog_in_progress. Two commits later, on the same screen, it ships "Video Playback Disabled" as an English literal right next to a localized settings_show_exercise_videos_description. The cure is contagious — the patient keeps relapsing.

Verdict

Approve — the heavy lifting is correct: cableMetadataForEquipment plumbs bilateral/DOUBLE through both the bundled and wger import paths, ExercisePickerFilters and the canonical BODYWEIGHT token keep the bodyweight filter honest, the enableVideoPlayback setting is finally honored again on the active-workout HUD, and the SqlDelightExerciseRepositoryTest assertions pin both the dual-cable intent and displayMultiplier == 2 so this regression can't sneak back. Nothing here is a correctness, security, or data-loss risk.

Correctness / Safety Findings

  • minor: WorkoutHud.kt:771"Video Playback Disabled" is a hardcoded English UI literal. Same anti-pattern this PR just fixed on the wger refresh path. Add a settings_exercise_demos_disabled resource and replace with stringResource(...).

Ponytail Review

  • shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/ExerciseImporter.kt:336reuse: cableMetadataForEquipment reimplements the exact split/uppercase/match for {BARBELL, E-Z CURL BAR, BAR, BELT} that already exists in Exercise.usesUnifiedAttachment (Exercise.kt:75). Extract a shared UNIFIED_ATTACHMENTS set (or call usesUnifiedAttachment on a synthetic Exercise) and reuse it in both places.

Ponytail net: 0 lines (refactor only).

Suggested Minimal Patch

Add settings_exercise_demos_disabled to strings.xml, then in WorkoutHud.kt:771:

stringResource(Res.string.settings_exercise_demos_disabled)

Optional cleanup: move the unified-attachment token set into a single constant referenced from both ExerciseImporter.cableMetadataForEquipment and Exercise.usesUnifiedAttachment.

Final Merge Guidance

Can merge as-is. The hardcoded-string finding is a localization nit, not a correctness blocker; the duplicate logic in cableMetadataForEquipment / usesUnifiedAttachment is an optional refactor.

Fix these issues in Kilo Cloud

Previous review (commit b7533c7)

Verdict: 1 Issue Found | Recommendation: Merge (optional cleanup)

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 1
🤏 nitpick 0
Issue Details (click to expand)
File Line Roast
ExerciseImporter.kt 229 canonicalEquipmentLabel was added specifically to collapse empty/body only/bodyweight to BODYWEIGHT_EQUIPMENT, then the very next line kept `

Resolved From Previous Review ✅

The 5 issues from the previous review at 05693853 are all closed in this incremental commit:

  • SettingsTab.kt:761-763 hardcoded literals → now getString(Res.string.settings_refresh_wger_catalog_…)
  • "Refreshing…" hardcoded → now stringResource(Res.string.settings_refresh_wger_catalog_in_progress)
  • settings_refresh_wger_catalog_success / _error orphan keys → now adopted
  • SchemaManifestTest expected-total count → now includes manifestDroppedTables
  • ExerciseImporter + ExercisePickerFilters bodyweight path → canonicalEquipmentLabel + isBodyweight short-circuit; new ExercisePickerFiltersTest.bodyweightFilterMatchesExplicitClassificationNotJustToken covers it
  • SqlDelight schema bumped 43 → 44 so the on-open migration actually runs on existing installs

🏆 Best part: You took the previous review's "stop hardcoding what you just localized" note, wrote a canonicalEquipmentLabel helper so the wger + free-exercise-db paths share one truth, and added a regression test that pins the filter behavior. That's not a fix, that's a response.

💀 Worst part: The helper you wrote is so good it makes two of the lines right after it unreachable, and you left them there anyway. Future-you will read that || contains("bodyweight") and wonder if wger is sending a string the helper doesn't recognize — it isn't.

📊 Overall: Like a kitchen where the chef finally cleans the fridge, swaps the menu, and bumps the health-department permit — but leaves one expired yogurt in the back. The food is safe. The yogurt is theoretical.

Files Reviewed (9 files in incremental diff)
  • shared/build.gradle.kts - schema version 43 → 44
  • shared/src/androidHostTest/.../SchemaManifestTest.kt - expected total includes dropped tables
  • shared/src/androidHostTest/.../SqlDelightExerciseRepositoryTest.kt - asserts canonical BODYWEIGHT token

[Snapshot truncated.]

Additional previous summary content was truncated to keep this comment within platform limits.


Reviewed by minimax-m3 · Input: 52.3K · Output: 25.5K · Cached: 330.9K

Review guidance: REVIEW.md from base branch main

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 05693853c9

ℹ️ 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".

codex added 3 commits August 20, 2026 19:46
Count dropped-table heals in SchemaManifestTest and pin SetReady layout to ExerciseDemoImage after mux video removal.
43.sqm migrates 43 -> 44; keep Gradle version aligned so existing installs archive stock rows and create ExerciseImage.
Wire existing translation keys, add in-progress copy, store BODYWEIGHT, and match the picker Bodyweight filter to the explicit flag.
@9thLevelSoftware

Copy link
Copy Markdown
Owner Author

Unit Tests was red on 0569385 (2 failures in Shared Module Test Results).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7533c73ec

ℹ️ 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".

Honor the HUD demo preference, mark barbell/E-Z curl bar imports as dual-cable unified attachments, and drop unreachable wger bodyweight checks.
@9thLevelSoftware

Copy link
Copy Markdown
Owner Author

Resolved all review threads.

…tion/screen/WorkoutHud.kt

Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bceb5f13f

ℹ️ 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".

codex added 3 commits August 20, 2026 20:19
Define settings_exercise_demos_disabled so WorkoutHud's localized disabled-demo label compiles.
Pin WorkoutHud disabled-demo copy to settings_exercise_demos_disabled instead of the removed English literal.
Skip remote thumbnail fetches, dialogs, and thumbnail clicks when enableVideoPlayback is false.
@9thLevelSoftware

Copy link
Copy Markdown
Owner Author

Lint Check and Unit Tests both failed on 0bceb5f1 with Unresolved reference 'settings_exercise_demos_disabled'.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c43b0833d6

ℹ️ 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".

codex added 3 commits August 20, 2026 20:38
Existing cycles still store ZZ92/UjIG/e64c/0040d53f IDs. Normalize them to the four replacement slugs so week and training-max advancement keep working after migration 43.
Point GLUTE_KICKBACKS at One-Legged_Cable_Kickback so the TUT percentage lift stays a machine exercise instead of body-only Glute_Kickback.
Store isBodyweight=1 only for BODYWEIGHT, 0 for cable accessories, and NULL otherwise so foam-roll/ball work uses !hasCableAccessory instead of requiring the machine.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3fcd217aa

ℹ️ 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".

codex added 2 commits August 20, 2026 20:48
Replace JVM-only MutableMap.putIfAbsent with a common contains-key write so compileKotlinIosArm64 can resolve 5/3/1 TM bump tracking.
Filter shelf and getEquipmentDatabaseValues now map Cable to CABLE so Face Pull and other free-exercise-db cable rows are selectable.
@9thLevelSoftware

Copy link
Copy Markdown
Owner Author

Automated fix for iOS Test Target Compile.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e562d9b68e

ℹ️ 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".

codex added 3 commits August 20, 2026 21:06
INSERT OR IGNORE plus an in-place catalogue UPDATE keeps favorites, 1RM, usage, MVT, and custom rows instead of INSERT OR REPLACE deleting through FKs. Zero of N imports now fail so the source marker is not set.
SqlDelightExerciseRepository only records BUNDLED_CATALOG_SOURCE when the importer reports a positive count, so a 0-of-N or failed import can retry.
…lbacks

Reimport keeps favorite/1RM/usage; colliding custom IDs fail the import; name lookups prefer archived=0 over legacy rows.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 217c7e2efd

ℹ️ 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".

After import, rewrite sessions, PRs, routines, assessments, and related
rows from archived Vitruvian IDs onto free-exercise-db slugs so upgrade
history and 1RMs show on the active picker entries.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba0b84ccc3

ℹ️ 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".

Zero consumed legacy user fields after merge so timesPerformed cannot
compound on later picker opens. When both IDs already have a PR, keep
the heavier/higher-volume record. Do not un-archive stock rows on
catalogue refresh.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4cc665b26a

ℹ️ 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".

Exact-name remap missed plural renames such as Rack Pull vs Rack Pulls.
Add explicit IDs, a reviewed alias table, and unique last-word stemming.
Merge personal MVT running means instead of deleting the legacy aggregate.
Simplify PR collision to compareBy and cover weight/volume winners in tests.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ffb116198

ℹ️ 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".

Add the bar-based Bench Press ID and other same-name dump duplicates
so history is not left on archived rows. Collision resolution now
prefers live PRs over heavier tombstones before comparing metrics.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15070d69c0

ℹ️ 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".

Comment on lines +207 to +211
if (exercises.isNotEmpty() && importedCount == 0) {
Logger.e { "Imported 0 of ${exercises.size} free-exercise-db rows" }
return@withContext Result.failure(
Exception("Imported 0 of ${exercises.size} exercises"),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject partial catalogue imports before saving the marker

Fresh evidence after the stated zero-row fix is that this guard fails only when importedCount == 0: each row exception is still swallowed at lines 201-203, and the counter is incremented before image replacement at line 185. A transient failure affecting one row—or even every image write after its exercise was counted—therefore returns success, causing SqlDelightExerciseRepository.importExercises() to persist the source marker and skip all future repair attempts. Propagate any row/image failure, or verify catalogue completeness before marking the source imported.

Useful? React with 👍 / 👎.

@9thLevelSoftware
9thLevelSoftware merged commit 6167d15 into main Aug 21, 2026
10 checks passed
@9thLevelSoftware
9thLevelSoftware deleted the fix/vitruvian-catalog-replacement branch August 21, 2026 02:22
* (`Rack Pull` / `Rack Pulls` → `rack pull`). Applied only when that key
* identifies a single active catalogue row.
*/
fun stemKey(name: String): String {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 The Roast: You wrote a hand-rolled English stemmer. Twenty lines of "ies drops to y, ches/shes/sses/xes drop two, generic s drops one" — enough suffix rules to make a linguist wince and a future maintainer weep. And nameAliases already covers every single one of these cases with explicit, reviewable entries (rack pull → rack pulls, concentration curl → concentration curls, mountain climber → mountain climbers, etc.). The stemmer is a fallback for a fallback that never fires.

🩹 The Fix: Delete stemKey (lines 149-166), the activeByStem construction in ExerciseImporter.kt:352-355, and the activeByStem[...] branch in ExerciseImporter.kt:378. Rely on nameAliases as the single source of truth — if a new rename is needed, add it to the map rather than hoping the stemmer guesses right. Net: ~25 fewer lines, zero behavior change for known cases.

📏 Severity: suggestion


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

9thLevelSoftware added a commit that referenced this pull request Aug 21, 2026
* fix: remove remaining Vitruvian IP outliers after catalogue swap

E2E audit follow-up to #706/#708 for the Vitruvian takedown request.
HEAD was already clean of the exercise dataset, the mux/jwplayer
streaming stack and the decompilation docs; this removes what survived:

- Drop stale "572 exercises with video" copy (TESTFLIGHT_NOTES,
  docs/index.html), add non-affiliation line to TestFlight notes.
- Delete final-visuals/ QA screenshots that render the old catalogue
  (three show a live stream.mux.com playback URL). Unreferenced by code.
- Migration 39: replace the six legacy catalogue IDs (+ catalogue-name
  comments) with the equivalent name-based heal on non-custom rows;
  drop the redundant RoutineExercise ID block. Idempotent, data-only;
  Kotlin mirror and SchemaParityTest updated to synthetic IDs.
- Reword ~70 comments/identifiers that cited the official app or
  decompiled internals as the source of behaviour (BLE/protocol,
  diagnostics, UI, tests, third-party Kable patch, almanac).
  createOfficialStopPacket() -> createSoftStopPacket();
  DiagnosticFaultCategory.VITRUVIAN("Vee") -> CONTROLLER("Controller");
  fault labels re-authored in Phoenix wording.
- Delete HardwareValidationTest scaffold (described the official app's
  Sample struct); update BlePacketCapture comments.
- Remove dead media3/HLS version-catalog aliases and unused video
  strings; "video" wording -> demo images in en/de/es/fr/nl.
- Neutralise safe branding strings (backup filenames, export headers,
  install guides, bug template, disconnect prompt); persisted names
  (vitruvian.db, vitruvian_preferences, video_playback key) untouched.
- LegacyCatalogueIdMap KDoc now states keys are migration-only opaque IDs.

Verified: :shared:testAndroidHostTest + :androidApp:testDebugUnitTest +
verifyCommonMainVitruvianDatabaseMigration -> 3,724 tests, 0 failures.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RpYk6iyjaMzC4DjiMEBwvK

* Bump app version to 1.0.0

---------

Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants