Codex/stu experiment#436
Conversation
|
@codex resolve issues: gemini-code-assist Bot reviewed 1 minute ago ...monMain/kotlin/com/devil/phoenixproject/presentation/manager/DefaultWorkoutSessionManager.kt @gemini-code-assist The new post-set tagging flow for Just Lift sessions does not trigger a Personal Record (PR) check. Previously, auto-detection would assign an exerciseId before the session was saved, allowing the PR logic in ActiveSessionEngine to run. Now, the exerciseId is null at save time, and tagging the exercise later via tagJustLiftSessionExercise creates a CompletedSet but never invokes personalRecordRepository.checkAndSavePR. This results in Just Lift workouts failing to contribute to the user's PR history or trigger celebrations. @9thLevelSoftware Reply... The ensureCompletedSetForTaggedJustLift implementation returns the existing CompletedSet without updating it if one already exists for the session. This creates a data consistency issue during retagging. If a user changes the exercise tag or AMRAP status, the existing CompletedSet record in the database will retain its original setType and other properties. While the exercise link is updated in the WorkoutSession table, the CompletedSet row should also be synchronized to reflect the latest state (e.g., updating setType if isAmrap changed). |
|
@codex Resolve merge conflicts This branch has conflicts that must be resolved shared/src/commonMain/kotlin/com/devil/phoenixproject/util/BlePacketFactory.kt |
![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 removes the exercise auto-detection system and introduces a manual tagging feature for Just Lift sessions, allowing users to associate exercises with their sets post-workout. It includes the addition of a new exercise picker dialog and updates to the session summary UI. Furthermore, the BLE packet construction logic was refactored to ensure that target weight and soft-max values are anchored to the selected force rather than being adjusted by per-rep progression. Reviewers identified several instances of hardcoded strings in the new UI components that should be moved to string resources to support localization.
| "Tag exercise", | ||
| style = MaterialTheme.typography.headlineSmall, | ||
| fontWeight = FontWeight.Bold, | ||
| ) | ||
| }, | ||
| text = { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .heightIn(max = 520.dp), | ||
| ) { | ||
| OutlinedTextField( | ||
| value = searchQuery, | ||
| onValueChange = { searchQuery = it }, | ||
| label = { Text(stringResource(Res.string.search_exercises_short)) }, | ||
| leadingIcon = { Icon(Icons.Default.Search, contentDescription = null) }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| singleLine = true, | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(Spacing.small)) | ||
|
|
||
| if (filteredExercises.isEmpty()) { | ||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(180.dp), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| Text( | ||
| "No exercises found", |
There was a problem hiding this comment.
Several strings in this component are hardcoded. To support localization and maintain consistency with the rest of the project, please move these to string resources (e.g., Res.string.tag_exercise_title, Res.string.no_exercises_found).
| text = taggedExerciseName ?: "Tag exercise", | ||
| style = MaterialTheme.typography.titleSmall, | ||
| fontWeight = FontWeight.Medium, | ||
| color = MaterialTheme.colorScheme.onSurface, | ||
| ) | ||
| taggedExerciseName?.let { | ||
| Text( | ||
| text = "Exercise tagged", | ||
| style = MaterialTheme.typography.bodySmall, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| TextButton(onClick = onClick) { | ||
| Text(if (taggedExerciseName == null) "Select" else "Change") |
There was a problem hiding this comment.
The labels in the ExerciseTagSection are hardcoded. Please use string resources for "Tag exercise", "Exercise tagged", "Select", and "Change" to ensure the UI can be localized.
|
@codex resolve issues gemini-code-assist Bot reviewed 2 minutes ago ...mmonMain/kotlin/com/devil/phoenixproject/presentation/components/MiniExercisePickerDialog.kt @gemini-code-assist Several strings in this component are hardcoded. To support localization and maintain consistency with the rest of the project, please move these to string resources (e.g., Res.string.tag_exercise_title, Res.string.no_exercises_found). @9thLevelSoftware Reply... The labels in the ExerciseTagSection are hardcoded. Please use string resources for "Tag exercise", "Exercise tagged", "Select", and "Change" to ensure the UI can be localized. |
![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: d8f75eadc5
ℹ️ 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".
| workoutRepository.updateSessionExerciseTag(sessionId, exerciseId, exercise.name) | ||
| val taggedSession = session.copy(exerciseId = exerciseId, exerciseName = exercise.name) | ||
| completedSetRepository.ensureCompletedSetForTaggedJustLift(taggedSession, isAmrap) | ||
|
|
There was a problem hiding this comment.
This tagging path only updates WorkoutSession.exerciseId and inserts a fallback CompletedSet, then immediately syncs, but it never runs PR/gamification evaluation for the newly attributed exercise. In the current flow, PR detection is only executed during saveWorkoutSession() using params.selectedExerciseId; Just Lift sessions are saved with that value unset, so post-set tagging cannot produce new PRs or mark the created set as PR even when the weight/reps beat prior records.
Useful? React with 👍 / 👎.
Summary
Testing
|
Summary
Testing
|
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24687c0fbe
ℹ️ 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".
| workoutRepository.updateSessionExerciseTag(sessionId, exerciseId, exercise.name) | ||
| val taggedSession = session.copy(exerciseId = exerciseId, exerciseName = exercise.name) | ||
| val completedSet = completedSetRepository.ensureCompletedSetForTaggedJustLift(taggedSession, isAmrap) | ||
|
|
||
| if (completedSet != null && completedSet.actualReps > 0) { |
There was a problem hiding this comment.
Retagging a Just Lift session can permanently attribute the same lift to multiple exercises because this path always updates PRs for the newly selected exercise without compensating for an existing prior tag on the same session. In the common flow where a user tags exercise A, then taps “Change” and tags exercise B, updatePRsIfBetter can write PR rows for both A and B from one set, which corrupts PR history and leaderboards over time. The tagging path needs to detect retags (session.exerciseId already set and different) and avoid or explicitly reconcile PR side effects before applying the new tag.
Useful? React with 👍 / 👎.
Code Review SummaryStatus: No Issues Found (beyond existing comments) | Recommendation: Merge This PR is a substantial refactoring that removes the exercise auto-detection feature and replaces it with manual exercise tagging for Just Lift sessions. Key Changes:
Existing Comments Verified:
No new issues identified beyond the existing comments. Files Reviewed (31 files)
Reviewed by laguna-m.1:free · 0 tokens |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec6c4516d9
ℹ️ 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".
| @@ -510,6 +505,10 @@ fun WorkoutTab( | |||
| } | |||
|
|
|||
| is WorkoutState.SetSummary -> { | |||
There was a problem hiding this comment.
This new tagging flow is only reachable inside the WorkoutState.SetSummary branch, but ActiveSessionEngine.handleSetCompletion still skips SetSummary and goes straight to Idle when summaryCountdownSeconds < 0 ("Summary OFF"). In that configuration, Just Lift sessions now have no UI path to tag an exercise at all (auto-detection was removed in this commit), so sessions remain untagged and never receive post-tag PR/gamification processing. Please provide a non-summary tagging entry point (or prevent skipping summary until tagging is done) for Just Lift.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f93561f54
ℹ️ 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".
| personalRecordRepository.updatePRsIfBetter( | ||
| exerciseId = exerciseId, | ||
| weightPRWeightPerCableKg = taggedSession.weightPerCableKg, | ||
| volumePRWeightPerCableKg = taggedSession.weightPerCableKg, | ||
| reps = completedSet.actualReps, | ||
| workoutMode = taggedSession.mode, | ||
| timestamp = currentTimeMillis(), | ||
| profileId = taggedSession.profileId, | ||
| cableCount = taggedSession.displayMultiplier, | ||
| ).onFailure { error -> | ||
| Logger.e(error) { "Failed to update PRs while tagging Just Lift session $sessionId" } | ||
| } |
There was a problem hiding this comment.
When a Just Lift session is tagged, this path now invokes updatePRsIfBetter, but it ignores the success payload and never calls markAsPr for the associated CompletedSet. Fresh evidence in this commit is that PR updates are attempted at this call site, yet the set row remains is_pr = false even when a new PR is created, which leaves session history and any CompletedSet.isPr-based UI/reporting inconsistent with the PR table.
Useful? React with 👍 / 👎.
* Refresh Smart Insights now timestamp on fetch (#412) * Refresh time-sensitive Smart Insights and recompute on clock updates (#418) * Align Smart Insights nowMs refresh with fetch and timer * Stabilize Smart Insights timer effect lifecycle * Remove periodic SmartInsights timer refresh loop * Harden SmartInsights loading state during refresh * Avoid mutating compose state from IO dispatcher * Align SmartInsights query and weekly-volume computation to shared nowMs (Option A) (#413) * Align insights query and weekly volume with shared nowMs * Harden insights anchor timestamp lifecycle * Fix SmartInsights loading guard for null anchor timestamp * Reset insights anchor before each profile refresh * Expand SmartSuggestions muscle-group taxonomy and add unknown-group fallback tracking (#415) * Expand muscle-group aliases and track unknown fallback usage * Refine unknown muscle-group fallback tracking * Keep SmartSuggestions fallback tracking stateless * Resolve review nits in muscle-group fallback docs/tests * Standardize inclusive cutoff policy (>=) in ReadinessEngine and add boundary tests (#416) * Define inclusive readiness cutoff policy and add boundary tests * Refine readiness boundary tests and add exclusion coverage * Address review cleanup in readiness boundary tests * Centralize inclusive readiness cutoff check * Unify insights into Snapshot→Trends→Diagnostics→Actions and add per-card metadata (#417) * Refactor analytics insights into unified hierarchy * Address review feedback on insight timeframe badges * Resolve review feedback on insights cards and cleanup * Resolve follow-up comments on readiness insight presentation * Use average intensity for Time-of-Day insights with confidence gating (#414) * Refine time-of-day insight to average intensity with confidence gating * Align time insight session-threshold copy with confidence gating * Add time-of-day tests for distinct-day gating and intensity scoring * Generalize time-window confidence copy across locales * Codex/fix high priority bug in insightstab.kt (#420) * Refactor analytics insights into unified hierarchy * Address review feedback on insight timeframe badges * Resolve review feedback on insights cards and cleanup * Resolve follow-up comments on readiness insight presentation * Fix missing RoundedCornerShape import in InsightsTab * Refresh Smart Insights now timestamp on fetch (#419) * Refresh Smart Insights now timestamp on fetch * Fix Compose state mutation in IO dispatcher and reset loading state on profile change Agent-Logs-Url: https://github.com/9thLevelSoftware/Project-Phoenix-MP/sessions/23e4c8a0-93e7-428f-8d00-b84a01429a58 Co-authored-by: 9thLevelSoftware <69057727+9thLevelSoftware@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> * fix(rep-counter): ignore firmware warmup sync for variable warmup sets (#422) * Request transient duck audio focus for Android workout cues (#423) * Fix Android cue audio precedence over background music * Fix audio focus release timing for workout cues * Align audio focus attributes with playback usage * Add focusChangeListener as identity token in requestTransientDuckAudioFocus Agent-Logs-Url: https://github.com/9thLevelSoftware/Project-Phoenix-MP/sessions/1d2923b8-cdaf-4080-bc22-8839a6940e94 Co-authored-by: 9thLevelSoftware <69057727+9thLevelSoftware@users.noreply.github.com> * Add focusHoldDurationMs helper and use event-specific delay in abandonAfterDelay Agent-Logs-Url: https://github.com/9thLevelSoftware/Project-Phoenix-MP/sessions/cd565f28-356a-4acd-9102-45b2602e7e02 Co-authored-by: 9thLevelSoftware <69057727+9thLevelSoftware@users.noreply.github.com> * Tie ducking focus hold to cue duration metadata --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> * build: update dependencies and add device metadata to Health Connect Update Health Connect to the stable 1.1.0 release and include device attribution in synced exercise records. - **Health**: Update `androidx-health-connect` to `1.1.0` stable and remove alpha-pinning workarounds in `shared/build.gradle.kts`. - **Health**: Implement `Metadata.activelyRecorded` for `ExerciseSessionRecord` and `TotalCaloriesBurnedRecord` to identify the "Vitruvian Trainer" device. - **Versions**: Upgrade Kotlin to `2.3.21`, AGP to `9.2.1`, and Gradle to `9.5.0`. - **Versions**: Bump Compose BOM to `2026.05.00`, Compose Navigation to `2.9.8`, and Ktor to `3.4.3`. * Revert audio focus duck attempt (#423) — restore audio output for #421 (#428) * Revert "Request transient duck audio focus for Android workout cues (#423)" This reverts commit 6371274. * fix(audio): release MediaPlayer on exception, dedupe sound name lists Address gemini-code-assist review on PR #428: - playWithMediaPlayer: hoist `mediaPlayer` declaration out of the `try` block and release it from `catch`. Previous structure leaked the native player if setVolume/setOnCompletionListener/start threw. - Extract BADGE_SOUND_NAMES and PR_SOUND_NAMES to private top-level vals. Eliminates duplication between the SoundPool-loading `remember` block and getRandomBadgeSound/getRandomPRSound, and removes per-call list allocations on the random helpers. --------- Co-authored-by: Claude <noreply@anthropic.com> * Restore Android workout cue audio routing and ignore code-review artifacts (#429) * Address PR #429 review feedback: route workout cues to STREAM_MUSIC (#430) * Address PR #429 review feedback: route cues to STREAM_MUSIC P1 (chatgpt-codex-connector / gemini-code-assist): the previous change to USAGE_ASSISTANCE_SONIFICATION routed cues to STREAM_SYSTEM, breaking the existing volumeControlStream = STREAM_MUSIC contract. Hardware volume buttons would no longer adjust workout cue loudness, leaving users with silent or unbalanced cues. - Switch STANDARD_ANDROID_CUE_USAGE to USAGE_MEDIA so SoundPool and MediaPlayer both route through STREAM_MUSIC, matching the activity's volumeControlStream and the existing media-volume warning logic. - Update warnIfMediaVolumeMuted comment/message to reflect USAGE_MEDIA. - Reword MainActivity volumeControlStream comment to reflect the actual scope (entire activity lifetime, not just the workout UI). - Update structural test assertions to match USAGE_MEDIA and add a guard preventing accidental reintroduction of USAGE_ASSISTANCE_SONIFICATION. - Replace silent fallbacks in HapticFeedbackAudioRoutingGuardTest with explicit checks so a missing project root, source file, or raw resource directory fails with an actionable message instead of an opaque FileNotFoundException or "every cue missing" report. * Address PR #430 review feedback: harden cue source parser - gemini-code-assist (high) / chatgpt-codex-connector (P2): the multi-line branch in soundNamesInConstantList searched for `\n)`, which only matches a closing paren at column 0. Replace with a simple `indexOf(")", start)`. Sound names only contain `[a-z0-9_]` so the first `)` after `start` is reliably the listOf(...) closer regardless of single-line, multi-line, or indented formatting. - gemini-code-assist (medium): extract the rep-count range (`(1..N).mapNotNull`) from the source instead of duplicating `1..25`, so the guard stays in sync if the upper bound changes. Skipped: the suggestion to skip SoundPool initialization on Fire OS (pre-existing behavior, out of scope for this PR's audio-routing focus). --------- Co-authored-by: Claude <noreply@anthropic.com> * Fix top-level superset reorder persistence and restore audio routing * Fix Android cue packaging and audio routing (#431) * Harden Compose layouts for iOS accessibility scaling (#432) * Harden iOS accessibility layouts for issue 389 * Fix cue audio resource lookup in release builds * Fix per-cable weight display and softMax handling (#433) * fix: preserve stall detection in portal sync * chore: remove stale generated artifacts * Codex/stu experiment (#436) * Fix per-cable weight display and softMax handling * Fix BLE force target to keep progression separate * Replace Just Lift auto-detection with post-set exercise tagging (#435) * Fix Just Lift retag PR tracking and completed set sync * fix: prevent Just Lift retag PR duplication * test: align AMRAP target weight expectation --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
No description provided.