feat: shake-to-revive after the sleep timer stops (#1618) - #1696
Conversation
Builds on #1595 (ShakeDetector + 12 m/s² threshold, reused as-is) and #1609 (main-marshalled pause/resume). After the sleep timer fires and pauses, the accelerometer keeps listening for a ~60s grace window; a qualifying shake resumes from the paused position and re-arms a fresh timer at the SAME mode that just elapsed. After the window with no shake, the sensor is released so a stopped timer never drains battery overnight. - SleepTimer: new `timerFired: SharedFlow<SleepTimerMode>` (replay=0 event; cancel() emits nothing) carrying the fired mode — solves fire-vs-cancel AND "same duration as last" in one signal. Emitted at the single fire point (fadeAndPause). Concrete class → no fakes affected. - DefaultPlaybackController: concrete-only `timerFired` val delegating to SleepTimer — the PlaybackController interface is untouched, so its fakes need no change. - StoryvoxPlaybackService: one accelerometer owner (`refreshShakeListening`) armed for fade-tail OR grace window; `shakeGraceJob` opens/closes the grace window on `timerFired`; onShake revives (resume + re-arm mode) when in-window. - SleepTimerDecisions: pure `shouldListenInGraceWindow` / `shouldReviveOnShake` + JVM tests in SleepTimerAutoArmTest (fire→listen, boundary, expiry, no-fire). CI proves compile + the pure decisions; sensor-listening + resume + re-arm are runtime → device-verify by JP (shake is physical). green+OPEN for JP. Device-verify (JP): timer stops → shake within 60s resumes + re-arms same duration; no shake within 60s → accelerometer released (no drain); user- cancelled timer opens no window. Closes #1618 Refs #1595 #1609 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 post-stop shake-to-revive grace window (Issue #1618), allowing users to shake their device within 60 seconds after the sleep timer stops to resume playback and re-arm the same timer mode. It consolidates accelerometer management under a single refreshShakeListening function and adds corresponding unit tests. The review comments identify two critical issues with this implementation: first, the accelerometer state flow (shakeJob) does not monitor isPlaying, meaning the sensor might remain active even after manual playback resumes; second, the grace window state is not cleared when the user manually resumes playback or starts a new timer, which could lead to accidental shake gestures overwriting manual actions. Code suggestions are provided to address both issues.
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.
| shakeJob = scope.launch { | ||
| controller.state | ||
| .map { it.sleepTimerRemainingMs to it.shakeToExtendEnabled } | ||
| .distinctUntilChanged() | ||
| .collect { (remaining, enabled) -> | ||
| // Issue #1595 — pure [shouldListenForShake] gate: listen | ||
| // only while the user opted in AND an armed timer is in | ||
| // its 10s fade tail. | ||
| val inFadeWindow = shouldListenForShake(remaining, enabled, SHAKE_FADE_WINDOW_MS) | ||
| if (inFadeWindow && !shakeListening) { | ||
| val started = shakeDetector?.start() ?: false | ||
| shakeListening = true | ||
| Log.i( | ||
| TAG, | ||
| "Shake-to-extend: fade tail entered (remaining=${remaining}ms) — " + | ||
| "accelerometer ${if (started) "registered" else "FAILED to register"}", | ||
| ) | ||
| } else if (!inFadeWindow && shakeListening) { | ||
| shakeDetector?.stop() | ||
| shakeListening = false | ||
| Log.d(TAG, "Shake-to-extend: fade tail exited — accelerometer released") | ||
| .collect { refreshShakeListening() } | ||
| } |
There was a problem hiding this comment.
The shakeJob flow mapping only monitors sleepTimerRemainingMs and shakeToExtendEnabled. When a user manually resumes playback or starts a new timer, isPlaying changes, but this flow does not emit. As a result, refreshShakeListening() is never called, and the accelerometer remains active and listening for shakes even while the app is actively playing.
This can lead to accidental shake-to-revive triggers that overwrite the active playback state or newly started sleep timer if the user shakes the phone within 60 seconds of the last timer stop.
Including isPlaying in the shakeJob flow mapping ensures that any change in playback state correctly triggers refreshShakeListening() to release the accelerometer.
| shakeJob = scope.launch { | |
| controller.state | |
| .map { it.sleepTimerRemainingMs to it.shakeToExtendEnabled } | |
| .distinctUntilChanged() | |
| .collect { (remaining, enabled) -> | |
| // Issue #1595 — pure [shouldListenForShake] gate: listen | |
| // only while the user opted in AND an armed timer is in | |
| // its 10s fade tail. | |
| val inFadeWindow = shouldListenForShake(remaining, enabled, SHAKE_FADE_WINDOW_MS) | |
| if (inFadeWindow && !shakeListening) { | |
| val started = shakeDetector?.start() ?: false | |
| shakeListening = true | |
| Log.i( | |
| TAG, | |
| "Shake-to-extend: fade tail entered (remaining=${remaining}ms) — " + | |
| "accelerometer ${if (started) "registered" else "FAILED to register"}", | |
| ) | |
| } else if (!inFadeWindow && shakeListening) { | |
| shakeDetector?.stop() | |
| shakeListening = false | |
| Log.d(TAG, "Shake-to-extend: fade tail exited — accelerometer released") | |
| .collect { refreshShakeListening() } | |
| } | |
| shakeJob = scope.launch { | |
| controller.state | |
| .map { Triple(it.sleepTimerRemainingMs, it.shakeToExtendEnabled, it.isPlaying) } | |
| .distinctUntilChanged() | |
| .collect { refreshShakeListening() } | |
| } |
| private fun refreshShakeListening() { | ||
| val s = controller.state.value | ||
| val enabled = s.shakeToExtendEnabled |
There was a problem hiding this comment.
The post-stop grace window (sleepTimerFiredAtMs and lastFiredSleepMode) is never cleared when the user manually resumes playback or starts a new sleep timer.
If the user manually resumes playback or starts a new timer, the grace window remains active. A subsequent shake within the 60-second window will still trigger onShake, which will call controller.resume() and controller.startSleepTimer(mode), overwriting the user's manual actions (e.g., overwriting a newly started 30-minute timer with the old EndOfChapter mode).
Clearing the grace window state inside refreshShakeListening() if the app is currently playing or if a new sleep timer is already running prevents this issue.
| private fun refreshShakeListening() { | |
| val s = controller.state.value | |
| val enabled = s.shakeToExtendEnabled | |
| private fun refreshShakeListening() { | |
| val s = controller.state.value | |
| if (s.isPlaying || s.sleepTimerRemainingMs != null) { | |
| sleepTimerFiredAtMs = null | |
| lastFiredSleepMode = null | |
| } | |
| val enabled = s.shakeToExtendEnabled |
When the sleep timer fires and pauses, a quick shake within a short grace window should bring playback back — instead of forcing the user to unlock, find the app, and hit play. Builds on #1595 (the
ShakeDetector+ 12 m/s² threshold, reused as-is) and #1609 (main-marshalled pause/resume).Behavior (per the issue)
SHAKE_GRACE_WINDOW_MS, tunable const).Design
SleepTimer.timerFired: SharedFlow<SleepTimerMode>—replay = 0(an event, not state) so a late subscriber can't open a spurious window; emitted at the single fire point (fadeAndPause), carrying the fired mode.cancel()emits nothing. One signal solves fire-vs-cancel and "same duration as last".DefaultPlaybackController.timerFired— a concrete-only val delegating toSleepTimer. ThePlaybackControllerinterface is deliberately untouched, so its fakes need no change.StoryvoxPlaybackService— one accelerometer owner (refreshShakeListening()) armed while in the fade tail OR the grace window;shakeGraceJobopens the window ontimerFiredand releases the sensor after it;onShakerevives (resume()on the Main scope per fix(sleep): marshal timer-fire pause to Main — stop Player-wrong-thread crash (#1606) #1609 +startSleepTimer(firedMode)).CI proves (compile + unit)
Pure decisions
shouldListenInGraceWindow/shouldReviveOnShakeinSleepTimerDecisions, with JVM cases inSleepTimerAutoArmTest(fire→listen, partway, end-boundary, past-boundary, no-fire).📱 Device-verify — for JP (sensor + resume are runtime; do NOT merge until checked)
Closes #1618
Refs #1595 #1609
🤖 Generated with Claude Code