Codex/profile readiness fixtures#651
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive profile-based training settings system, including a new Profile tab, migration of training preferences to a profile-scoped SQLDelight aggregate, and a robust QA seeding mechanism for local testing. My feedback highlights that the rackSnapshotJson function currently uses brittle hardcoded JSON strings, which should be replaced with the project's JSON serializer to ensure consistency and maintainability.
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.
| private fun rackSnapshotJson(profileKey: String, isProfileA: Boolean): String { | ||
| val id = rackItemId(profileKey) | ||
| return if (isProfileA) { | ||
| "[{\"id\":\"$id\",\"name\":\"QA Weighted Vest A\",\"category\":\"WEIGHTED_VEST\",\"weightKg\":10.0,\"behavior\":\"ADDED_RESISTANCE\",\"enabled\":true,\"sortOrder\":1,\"createdAt\":1700000001000,\"updatedAt\":1700000001100}]" | ||
| } else { | ||
| "[{\"id\":\"$id\",\"name\":\"QA Assistance B\",\"category\":\"ASSISTANCE\",\"weightKg\":7.5,\"behavior\":\"COUNTERWEIGHT\",\"enabled\":true,\"sortOrder\":2,\"createdAt\":1700000002000,\"updatedAt\":1700000002100}]" | ||
| } |
There was a problem hiding this comment.
The rackSnapshotJson function uses hardcoded JSON strings to create the rack snapshot. This is brittle and can easily break if the RackItem serialization changes. To improve maintainability and ensure consistency with the rest of the app, consider using the project's JSON serializer to generate this string from RackItem data classes.
This would involve creating the RackItem instances and then using a Json instance to encode them into a string, which ensures that the generated JSON is always in sync with the RackItem data class definition and its serialization rules.
private fun rackSnapshotJson(profileKey: String, isProfileA: Boolean): String {
val json = kotlinx.serialization.json.Json { encodeDefaults = true }
val rackItem = if (isProfileA) {
RackItem(
id = rackItemId(profileKey),
name = "QA Weighted Vest A",
category = RackItemCategory.WEIGHTED_VEST,
weightKg = 10.0f,
behavior = RackItemBehavior.ADDED_RESISTANCE,
enabled = true,
sortOrder = 1,
createdAt = 1_700_000_001_000L,
updatedAt = 1_700_000_001_100L,
)
} else {
RackItem(
id = rackItemId(profileKey),
name = "QA Assistance B",
category = RackItemCategory.ASSISTANCE,
weightKg = 7.5f,
behavior = RackItemBehavior.COUNTERWEIGHT,
enabled = true,
sortOrder = 2,
createdAt = 1_700_000_002_000L,
updatedAt = 1_700_000_002_100L,
)
}
return json.encodeToString(listOf(rackItem))
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff8470a88
ℹ️ 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".
| ), | ||
| selected = core.weightUnit, | ||
| enabled = enabled, | ||
| onSelected = { onCoreChange(core.copy(weightUnit = it)) }, |
There was a problem hiding this comment.
When a profile switches between KG and LB here, the existing weightIncrement is carried over even though UserPreferences.effectiveWeightIncrement interprets the stored value in the newly selected display unit. For example, a user with a 5 kg step who switches to LB now silently gets a 5 lb step (and a 1 lb step becomes 1 kg when switching back), which changes workout weight adjustments/recommendations; reset to the unit default (-1f) or convert the increment when changing weightUnit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efac83b241
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (verbalEvent != null) { | ||
| coordinator._hapticEvents.emit(verbalEvent) | ||
| Logger.i { "VBT: VERBAL_ENCOURAGEMENT emitted (tier=${verbalEvent.vulgarTier}, dominatrix=${verbalEvent.dominatrixMode}, vulgar=${verbalEvent.vulgarMode})" } |
There was a problem hiding this comment.
When a velocity-loss alert fires with verbal encouragement enabled, this new if (verbalEvent != null) block is not closed before the legacy val prefs = ... emission below, so the code emits a second VERBAL_ENCOURAGEMENT using raw vulgarModeEnabled/dominatrixModeActive values. In the inspected workout path, a profile with persisted vulgar/dominatrix intent but adultsOnlyConfirmed == false first emits the neutralized event and then immediately emits an adult-routed event, bypassing the local adult confirmation gate; close this block before the legacy code or remove the legacy emission.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Someone deleted a feature AND updated the tests AND updated the spec. I'm supposed to be cynical about PRs but this one is just… competent. I don't have a joke for this. Let me look at my notes — no, still nothing. Moving on. Overview
No new inline comments this pass. Re-verified the open review threads against HEAD ( Existing thread status
Incremental scope (524f968 → 655075d)9 files, +12/-74, one commit (
Correctness / Safety FindingsNo correctness or safety findings on changed lines. The Ponytail Review
Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge. This commit is the cleanest possible execution of a UI removal: the unused code is gone, the strings are gone from every locale that had them, the spec doc was updated to match the new contract, and the contract tests now actively assert the absence so re-regression is caught. No new dependencies, no new abstractions, no new tests (the existing ones were tightened), no behavior change for any path that still legitimately needs the stored fields. 🏆 Best part: 💀 Worst part: The spec doc still describes the workout-preferences JSON shape at 📊 Overall: A 62-line net-deletion commit that retires a UI control cleanly. No issues, no new abstractions, no new tests, no behavior change for any caller that still needs the underlying field. The data-model residue is intentional and spec-mandated. This is what "remove a feature" looks like when the engineer actually thought about it for ten minutes instead of just hiding the view. Files Reviewed (9 files)
Previous Review Summaries (3 snapshots, latest commit 524f968)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 524f968)Verdict: No New Issues Found | Recommendation: Merge Oh look — the engineer actually read the previous review and fixed it. I had to put down my coffee for this. Overview
Existing thread status
Incremental scope (2a01e2f → 524f968)7 files, +172/-19, one commit (
Correctness / Safety FindingsNo correctness or safety findings on changed lines. Ponytail Review
Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Both prior findings are properly addressed. Final Merge GuidanceCan merge. The P2 backup-adoption finding and the medium hardcoded-JSON finding are both resolved with tests that prevent regression. The remaining P1 finding on 🏆 Best part: 💀 Worst part: The test rename + value flip for the "absent explicit owner" tests is a quiet behavior change that may bite downstream snapshot tests or other callers that depended on the old 📊 Overall: Two review findings, two clean fixes, two regression tests. The kind of incremental that makes a code reviewer suspicious that something is hiding — but I've checked, and it isn't. Ship it. Files Reviewed (7 files)
Previous review (commit 2a01e2f)Verdict: No Issues Found | Recommendation: Merge Oh wait, this incremental is actually clean. I had my flamethrower warmed up and everything. Overview
No new inline comments this pass. Re-verified all four existing threads against HEAD ( Existing thread status
Incremental scopeThe PR is a UI/UX cleanup of the Profile preferences screen: removes global per-profile knobs that don't belong there ( The behavior shifts are codified in the design doc (
No correctness, security, data-loss, race, or regression issues found in the changed lines. 🏆 Best part: The five extracted 💀 Worst part: The 7-tap-on-title unlock is now genuinely undiscoverable — both VBT (Dominatrix) and LED (Disco) titles render as plain bold text with no ripple, no "Tap header 7× to unlock" supporting line, and no role hint. The unlock popup fires correctly when triggered, and the design doc explicitly documents the design, but a first-time user has a snowball's chance of finding either easter egg without external documentation. Minor discoverability regression. 📊 Overall: 17 files, +630/-198, one commit, two stale threads finally closed, no new fires. The architecture is doing what architecture does — moving policy out of the view layer and into helpers the tests can name. Ship it. Files Reviewed (17 files)
Reply with Previous review (commit 23f4ede)Verdict: Comment Only | Recommendation: Can merge; previously-blocking P1/P2 are now resolved, leaving only the two pre-existing review threads. Overview
No new inline comments this pass. I re-verified all four existing threads against HEAD ( Existing thread status
Stall-detection / Echo / Issue-#656 fixes (incremental scope)The PR carries out the full F1–F9 stall-detection audit and the #656 Summary-nav fix. Highlights:
Ponytail Review
Ponytail net: ~40 lines removable (all optional cleanup; none gate correctness). Suggested Minimal PatchNo patch from this review. Existing threads cover correctne [Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by minimax-m3 · Input: 51.4K · Output: 7.9K · Cached: 556.4K |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e15c7953a4
ℹ️ 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".
| @@ -466,7 +559,7 @@ abstract class BaseDataBackupManager( | |||
| dominantSide = session.dominantSide, | |||
| strengthProfile = session.strengthProfile, | |||
| formScore = session.formScore, | |||
| profile_id = session.profileId ?: "default", | |||
| profile_id = session.profileId ?: activeProfileId, | |||
There was a problem hiding this comment.
When importing a per-session/per-routine backup from another install or from a deleted profile, the backup has no userProfiles entry (see exportSession/exportRoutine) but mapSessionToBackup still serializes the source profileId. This assignment preserves that non-existent profile id instead of falling back to the active profile, so the imported workout history is written under an orphan profile and disappears from the user's active profile; resolve session.profileId against the imported/existing profile set, and mirror the fix in the streaming import path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 524f968291
ℹ️ 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".
| @@ -1842,20 +1857,21 @@ class ActiveSessionEngine( | |||
| repCountTimingName = params.repCountTiming.name, | |||
| restSeconds = params.justLiftRestSeconds, | |||
| ) | |||
| preferencesManager.saveJustLiftDefaults(defaults) | |||
| settingsManager.saveJustLiftDefaultsDocument(defaults) | |||
There was a problem hiding this comment.
When a Just Lift workout is saved in a non-Echo mode, echoLevelVal is 0 (EchoLevel.HARD). This new direct profile-preference write bypasses the old SettingsPreferencesManager.saveJustLiftDefaults normalization that converted non-Echo HARD placeholders to HARDER, so after saving any OldSchool/Pump Just Lift workout the next Echo Just Lift default reopens at the stricter pre-issue-553 HARD level. Normalize the document before persisting, or preserve the prior Echo difficulty when the saved workout is not Echo.
Useful? React with 👍 / 👎.
| @@ -1901,7 +1917,7 @@ class ActiveSessionEngine( | |||
| perSetRestTime = currentExercise.perSetRestTime, | |||
| defaultRackItemIds = currentExercise.defaultRackItemIds.filter { it.isNotBlank() }.distinct(), | |||
| ) | |||
| preferencesManager.saveSingleExerciseDefaults(defaults) | |||
| settingsManager.saveSingleExerciseDefaultsDocument(defaults.toDocument()) | |||
There was a problem hiding this comment.
For temporary single-exercise routines in non-Echo modes, echoLevelVal is also 0, and this new direct document save bypasses the old saveSingleExerciseDefaults normalization for non-Echo HARD placeholders. Completing a non-Echo single-exercise workout now overwrites that exercise's saved Echo difficulty to HARD, so the next Echo session for that exercise starts with the pre-issue-553 strict default; apply the same normalization before saving.
Useful? React with 👍 / 👎.
1 participant
No description provided.