Add timed-set countdown cues and stronger rep sound#505
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 countdown beeps during the final 10 seconds of timed sets (for both bodyweight and cable exercises), with the playback rate increasing as the timer approaches zero. It introduces ExerciseCountdownCuePolicy to manage tick emission, updates user preferences and settings UI, and adds corresponding unit tests. The reviewer identified potential issues in ActiveSessionEngine.kt, including skipped initial countdown ticks for sets lasting 10 seconds or less, and potential race conditions from non-atomic timer decrements, recommending the use of MutableStateFlow.update for thread safety.
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 lastObservedRemaining = effectiveDuration | ||
| var lastTickedSecond = -1 |
There was a problem hiding this comment.
For timed sets with a duration of 10 seconds or less (e.g., exactly 10 seconds or 5 seconds), the first countdown beep is skipped because the timer decrements before the first tick check is performed.
To fix this, initialize lastTickedSecond by checking and emitting a tick for the initial duration before entering the loop.
var lastObservedRemaining = effectiveDuration
var lastTickedSecond = emitExerciseCountdownTickIfNeeded(
remainingSeconds = effectiveDuration,
lastTickedSecond = -1,
)| val current = coordinator._timedExerciseRemainingSeconds.value ?: 0 | ||
| coordinator._timedExerciseRemainingSeconds.value = (current - 1).coerceAtLeast(0) | ||
| val remaining = (current - 1).coerceAtLeast(0) | ||
| coordinator._timedExerciseRemainingSeconds.value = remaining |
There was a problem hiding this comment.
There is a potential race condition where a concurrent write to _timedExerciseRemainingSeconds (such as a timer reset or manual adjustment) can be overwritten by the non-atomic read-modify-write decrement operation.
Using MutableStateFlow.update ensures the decrement is atomic and prevents concurrent updates from being lost.
var remaining = 0
coordinator._timedExerciseRemainingSeconds.update { current ->
remaining = ((current ?: 0) - 1).coerceAtLeast(0)
remaining
}| var lastObservedRemaining = timedExerciseDuration | ||
| var lastTickedSecond = -1 |
There was a problem hiding this comment.
For timed sets with a duration of 10 seconds or less, the first countdown beep is skipped because the timer decrements before the first tick check is performed.
Initialize lastTickedSecond by checking and emitting a tick for the initial duration before entering the loop.
var lastObservedRemaining = timedExerciseDuration
var lastTickedSecond = emitExerciseCountdownTickIfNeeded(
remainingSeconds = timedExerciseDuration,
lastTickedSecond = -1,
)| val current = coordinator._timedExerciseRemainingSeconds.value ?: 0 | ||
| coordinator._timedExerciseRemainingSeconds.value = (current - 1).coerceAtLeast(0) | ||
| val remaining = (current - 1).coerceAtLeast(0) | ||
| coordinator._timedExerciseRemainingSeconds.value = remaining |
There was a problem hiding this comment.
There is a potential race condition where a concurrent write to _timedExerciseRemainingSeconds (such as a timer reset or manual adjustment) can be overwritten by the non-atomic read-modify-write decrement operation.
Using MutableStateFlow.update ensures the decrement is atomic and prevents concurrent updates from being lost.
var remaining = 0
coordinator._timedExerciseRemainingSeconds.update { current ->
remaining = ((current ?: 0) - 1).coerceAtLeast(0)
remaining
}
![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: 7d4c33c16a
ℹ️ 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 (prefs.beepsEnabled) { | ||
| coordinator._hapticEvents.emit(HapticEvent.REST_ENDING) |
There was a problem hiding this comment.
When a user leaves general beeps on but disables countdown beeps, this new five-seconds-remaining branch still emits REST_ENDING, which maps to the rest-over audio/haptic cue. Since the settings copy now says countdown beeps cover the last 10 seconds of rest timers and timed sets, those users will still hear/get a cue during the disabled countdown window; gate this branch with countdownBeepsEnabled too, or keep REST_ENDING at actual rest completion.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| try { | ||
| val playbackRate = if (event is HapticEvent.COUNTDOWN_TICK) { |
There was a problem hiding this comment.
🔥 The Roast: You added playback rate acceleration for countdown ticks in the SoundPool path, but completely forgot that playWithMediaPlayer exists as a fallback. On devices where SoundPool fails (looking at you, Fire OS), countdown ticks will play at normal speed like they're taking a leisurely stroll instead of racing to zero.
🩹 The Fix: Add playback rate support to playWithMediaPlayer when the event is a COUNTDOWN_TICK, matching what you did for SoundPool and iOS. Something like mediaPlayer.setPlaybackParams(PlaybackParams().setSpeed(ExerciseCountdownCuePolicy.playbackRate(event.secondsRemaining))) before mediaPlayer.start().
📏 Severity: warning
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. The MediaPlayer fallback playback rate gap has been plugged. The peasants of Fire OS can now enjoy turbo-charged countdown beeps just like the SoundPool VIPs. It's like democracy for audio cues. 📊 Overall: Like finding a unicorn in production — the MediaPlayer fix actually landed. Someone pinch me. Files Reviewed (8 files)
Reviewed by laguna-m.1-20260312:free · 1,031,029 tokens |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73262bf80a
ℹ️ 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 (!coordinator._isRestPaused.value && | ||
| remainingSeconds in 1..10 && | ||
| remainingSeconds in 6..10 && | ||
| remainingSeconds != lastTickedSecond |
There was a problem hiding this comment.
For routine rest timers with countdown beeps enabled, changing this range to 6..10 drops the existing ticks for 5, 4, 3, 2, and 1 seconds; before this change the rest loop emitted COUNTDOWN_TICK for the full last 10 seconds, and the Issue 490 plan explicitly says existing rest-timer countdown behavior should remain unchanged while adding timed-set cues. Users now only get 10–6 plus the separate rest-ending cue at 5 seconds, so the final five-second rest countdown no longer matches the setting copy or prior behavior.
Useful? React with 👍 / 👎.
Resolved conflict in ActiveSessionEngine.kt where the PR branch reordered kotlin.* imports while main added kotlinx.serialization imports — kept all four. https://claude.ai/code/session_01F3GxXaEqPWv4ig8EmVCSMP
Resolve merge conflicts with weight recommendation (PR #499), timed-set countdown cues (PR #505), and Health Connect export (PR #516) by keeping both deload modifiers and the new main features. https://claude.ai/code/session_01MBWwVaUrnXjzZxYKKnn33i
Summary
Testing