fix(general): harden media action execution and make notification favorite state episode-aware and remove latency#3583
Conversation
Await custom command handlers before returning SessionResult. Return structured SessionError values for unsupported commands and runtime failures. Switch callback command hooks to suspend handlers to align with async execution.
Serialize like/library/radio/target-playlist actions with dedicated mutexes to keep rapid taps deterministic. Make library toggles optimistic locally and run remote sync in-order to remove perceived delay. Move target-playlist reads/writes to IO and prevent duplicate inserts under repeated actions. Refactor notification command internals to suspend handlers and improve failure reporting behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverted media session callbacks to suspending handlers, moved custom-command dispatch to coroutine futures with explicit error mapping, and added mutex-guarded suspend implementations in MusicService that perform optimistic DB updates, external syncs, immediate UI refreshes, and structured exception handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Client/Controller
participant Session as MediaLibrarySessionCallback
participant Service as MusicService
participant DB as Database
participant YouTube as YouTube API
participant UI as Notification/Widget
Controller->>Session: send customAction (e.g., "TOGGLE_LIKE")
Session->>Service: scope.future { invoke suspending handler }
Service->>Service: withLock (acquire Mutex)
Service->>DB: optimistic update (suspend)
alt external sync required
Service->>YouTube: sync like/library/playlist (suspend)
YouTube-->>Service: result / error
end
Service->>DB: refresh/read merged state (suspend)
Service->>Service: mergeSongStateIntoCurrentMetadata
Service->>UI: updateNotification() / updateWidgetUI()
Service-->>Session: complete Future with SessionResult (success or mapped error)
Session-->>Controller: future completed with result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1967-1971: Inconsistent error handling compared to the other public wrappers.
toggleLike(),toggleLibrary(), andaddToTargetPlaylist()now log viaTimber.tag(TAG).e(...)andreportException(e)on failure. With the newtoggleStartRadioInternal()throwingIllegalStateExceptionwhen no recommendations are applied, this wrapper will silently swallow the failure viaSilentHandler, leaving no trace in logs/crash reports for user-triggered radio starts.♻️ Align with the other wrappers
fun toggleStartRadio() { scope.launch(SilentHandler) { - toggleStartRadioInternal() + try { + toggleStartRadioInternal() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Timber.tag(TAG).e(e, "Failed to start radio") + reportException(e) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1967 - 1971, The public wrapper toggleStartRadio should mirror other wrappers (toggleLike, toggleLibrary, addToTargetPlaylist) by catching exceptions from toggleStartRadioInternal and recording them instead of silently swallowing them; update toggleStartRadio to launch the coroutine, wrap the call to toggleStartRadioInternal() in a try/catch, and in the catch block call Timber.tag(TAG).e(e, "Failed to start radio") and invoke reportException(e) so IllegalStateException and other errors are logged and reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1220-1228: The current favorite computation can be incorrectly
overridden by stale metadata; update the logic so that when
currentSong.value?.song is non-null its DB-backed fields take precedence:
determine isFavorite by first checking if song != null — for episodes use
(song.inLibrary != null) and for non-episodes use (song.liked == true); only
when song == null fall back to currentMediaMetadata (metadata?.inLibrary != null
for episodes, metadata?.liked == true for non-episodes). Use the existing
identifiers currentSong, currentMediaMetadata, isEpisode, isFavorite,
song.inLibrary, song.liked, metadata.inLibrary, and metadata.liked when
implementing this precedence change.
- Around line 1951-1960: The current update uses the player's current metadata
(baseMetadata) to set inLibrary/isEpisode which can overwrite metadata for a
newly playing track; in MusicService around the currentMediaMetadata update
(using baseMetadata, player.currentMetadata and songEntity), first verify the
metadata belongs to the same track before mutating UI state: compare the unique
ID of songEntity (or the DB row id used in the toggle) with baseMetadata?.id (or
player.currentMetadata?.id) and only perform currentMediaMetadata.value =
baseMetadata?.copy(...) when the IDs match; if they don't match, skip updating
currentMediaMetadata (but still call updateNotification()/updateWidgetUI as
appropriate) so you don't attribute the toggle to a different track.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1967-1971: The public wrapper toggleStartRadio should mirror other
wrappers (toggleLike, toggleLibrary, addToTargetPlaylist) by catching exceptions
from toggleStartRadioInternal and recording them instead of silently swallowing
them; update toggleStartRadio to launch the coroutine, wrap the call to
toggleStartRadioInternal() in a try/catch, and in the catch block call
Timber.tag(TAG).e(e, "Failed to start radio") and invoke reportException(e) so
IllegalStateException and other errors are logged and reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ff92f0c-0c20-4956-9a28-0d7ec1dd57e4
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
- Prefer DB-backed song state over possibly stale metadata when deriving notification favorite state. - Preserve coroutine cancellation in start-radio fallback path instead of swallowing CancellationException. - Move currentSong capture inside library/like mutexes to avoid stale concurrent toggles. - Re-read persisted song rows after optimistic DB updates and merge liked/likedDate/inLibrary/isEpisode into current metadata before notification/widget refresh. - Add rollback path for toggleLibrary remote failures and refresh UI from rolled-back state for consistency. - Guard episode metadata writeback to current track ID to avoid cross-track attribution during transitions. Validation: ./gradlew :app:compileFossDebugKotlin --no-daemon --console=plain (BUILD SUCCESSFUL).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1436-1442: Stale log tag in the new internal method.The warning on Line 1440 still reads
"startRadioSeamlessly called before player initialization", buttoggleStartRadioInternalis now also reachable viatoggleStartRadio()(Line 2015) and from the session callback wiring at Line 510. The message will misattribute the caller when the other entry points are used.- Timber.tag(TAG).w("startRadioSeamlessly called before player initialization") + Timber.tag(TAG).w("toggleStartRadioInternal called before player initialization")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1436 - 1442, The warning message in toggleStartRadioInternal still says "startRadioSeamlessly called before player initialization" which misattributes callers; update the Timber.tag(TAG).w(...) message inside the suspend function toggleStartRadioInternal to a neutral or correct message (e.g., "toggleStartRadioInternal called before player initialization" or "start radio called before player initialization") so it accurately reflects this method can be reached from toggleStartRadio and session callback wiring; modify only the string literal in toggleStartRadioInternal's early-return log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1861-1920: toggleLikeInternal currently does an optimistic DB
update then calls syncUtils.likeSong(...) and
DownloadService.sendAddDownload(...) without rollback on failure; mirror
toggleLibraryInternal: wrap the remote calls (syncUtils.likeSong and the
DownloadService call) in a try/catch, and on exception perform an IO re-read and
rollback update to restore the previous song state (use the original songEntity
or the DB row read back), call mergeSongStateIntoCurrentMetadata(rollbackSong),
updateNotification(), updateWidgetUI(player.isPlaying) while still inside the
lock, then rethrow the exception so outer toggleLike() can log/report it.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1436-1442: The warning message in toggleStartRadioInternal still
says "startRadioSeamlessly called before player initialization" which
misattributes callers; update the Timber.tag(TAG).w(...) message inside the
suspend function toggleStartRadioInternal to a neutral or correct message (e.g.,
"toggleStartRadioInternal called before player initialization" or "start radio
called before player initialization") so it accurately reflects this method can
be reached from toggleStartRadio and session callback wiring; modify only the
string literal in toggleStartRadioInternal's early-return log.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 000743fc-074a-432b-8955-07fc49c05c1c
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
- Wrap toggleLike remote sync + auto-download calls in try/catch after optimistic DB updates.\n- Roll back to the pre-toggle song state on non-cancellation failures, then merge metadata and refresh notification/widget UI before rethrowing for centralized error reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1459-1536:⚠️ Potential issue | 🟠 MajorOuter catch in
toggleStartRadioInternalstill swallowsCancellationException.The inner fallback catch at Line 1532 correctly rethrows
CancellationException, but the outercatch (e: Exception)at Line 1494 does not. In Kotlin,CancellationExceptionextendsException, so a cancellation raised by the primarywithContext(Dispatchers.IO) { ... }(Lines 1461‑1466) is caught here, the fallback network calls then execute against an already-cancelled coroutine, and the original cancellation is lost if the fallback completes (or its inner catch doesn't see a newCancellationException). This contradicts the PR's "Preserve coroutine cancellation" commit — the fix needs to apply at both levels.🛡️ Suggested fix
} catch (e: Exception) { + if (e is CancellationException) throw e // Fallback: try with related endpoint try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1459 - 1536, The outer catch in toggleStartRadioInternal around the primary withContext block swallows CancellationException; update that catch (e: Exception) to rethrow CancellationException (e.g., if (e is CancellationException) throw e) before handling/logging fallback so coroutine cancellation is preserved, similar to the inner fallback catch, and ensure the same pattern is applied to any other broad Exception catches in toggleStartRadioInternal that wrap suspend calls like withContext(Dispatchers.IO) { ... } and radioQueue.getInitialStatus().
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1844-1846: Nit: stray trailing brace formatting.The
} }on Line 1845 is syntactically fine but visually confusing. Consider splitting onto separate lines to match the style oftoggleLikeInternal/addToTargetPlaylistInternal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1844 - 1846, The closing braces after the catch rethrow in MusicService.kt are squashed ("} }") and should be split onto separate lines to match the project's brace style; locate the block that contains the `throw e` (within the same method that parallels `toggleLikeInternal`/`addToTargetPlaylistInternal`) and place each closing brace on its own line so the method and enclosing block are clearly separated and visually consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1874-1878: The episode "save for later" path needs the same
rollback parity as other toggles: in toggleEpisodeSaveForLater (which
toggleLikeInternal delegates to), capture the previous inLibrary/save-for-later
state before performing the optimistic DB update and mutating
currentMediaMetadata, then wrap the remote call to syncUtils.saveEpisode(...) in
a try/catch; on non-cancellation failures restore the DB row to the previous
state, re-merge the previous metadata into currentMediaMetadata, trigger the
notification/widget refresh/update functions (same ones used by
toggleLibraryInternal/toggleLikeInternal rollback), and rethrow the exception;
apply this pattern to the other occurrence of toggleEpisodeSaveForLater logic
around lines 1998-2031 as well so both paths have symmetric rollback behavior.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1459-1536: The outer catch in toggleStartRadioInternal around the
primary withContext block swallows CancellationException; update that catch (e:
Exception) to rethrow CancellationException (e.g., if (e is
CancellationException) throw e) before handling/logging fallback so coroutine
cancellation is preserved, similar to the inner fallback catch, and ensure the
same pattern is applied to any other broad Exception catches in
toggleStartRadioInternal that wrap suspend calls like
withContext(Dispatchers.IO) { ... } and radioQueue.getInitialStatus().
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1844-1846: The closing braces after the catch rethrow in
MusicService.kt are squashed ("} }") and should be split onto separate
lines to match the project's brace style; locate the block that contains the
`throw e` (within the same method that parallels
`toggleLikeInternal`/`addToTargetPlaylistInternal`) and place each closing brace
on its own line so the method and enclosing block are clearly separated and
visually consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acc71a80-a5f2-4f72-8837-c2673a9e6492
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…ter failures Mirror CodeRabbit rollback handling in toggleEpisodeSaveForLater by restoring the previous DB row when remote saveEpisode sync fails, re-merging rolled-back metadata, refreshing notification and widget state, and rethrowing non-cancellation errors for centralized reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (3)
1444-1444: Avoid shadowing thecurrentMediaMetadatastate flow with a local.The class-level
currentMediaMetadata: MutableStateFlow<MediaMetadata?>(Line 305) is shadowed by a local val of the same name, which is easy to misread in a 100-line method that also touches the real flow elsewhere. Rename the local (e.g.playerMetadata) for clarity.- val currentMediaMetadata = player.currentMetadata ?: return - - val currentIndex = player.currentMediaItemIndex - val currentMediaId = currentMediaMetadata.id + val playerMetadata = player.currentMetadata ?: return + val currentIndex = player.currentMediaItemIndex + val currentMediaId = playerMetadata.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` at line 1444, The method creates a local val named currentMediaMetadata that shadows the class-level MutableStateFlow currentMediaMetadata; rename the local to something like playerMetadata to avoid confusion. Specifically, replace the local declaration that assigns player.currentMetadata (e.g., val currentMediaMetadata = player.currentMetadata ?: return) with a distinct name (e.g., val playerMetadata = player.currentMetadata ?: return) and update all subsequent references in the method to use playerMetadata so the class property currentMediaMetadata remains unshadowed.
2058-2062: Inconsistent error handling vs. the other public toggles.
toggleLike/toggleLibrary/addToTargetPlaylistall wrap the internal suspend call intry { … } catch (CancellationException) { throw } catch (Exception) { log + reportException }.toggleStartRadiojust launches the internal underSilentHandler, so any non-cancellation failure (including the newIllegalStateException("No radio recommendations…")at Line 1539) is silently swallowed with no log. Either mirror the pattern used by the other toggles, or just delegate tostartRadioSeamlessly()which already has the wrapper.♻️ Consolidate on the existing wrapper
fun toggleStartRadio() { - scope.launch(SilentHandler) { - toggleStartRadioInternal() - } + startRadioSeamlessly() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 2058 - 2062, toggleStartRadio currently launches toggleStartRadioInternal under SilentHandler and thus swallows non-cancellation exceptions; update it to follow the same error-handling pattern used by toggleLike/toggleLibrary/addToTargetPlaylist by wrapping the suspend call in try { ... } catch (CancellationException) { throw } catch (Exception e) { log the error and call reportException(e) }, or simply delegate from toggleStartRadio to startRadioSeamlessly() which already implements that wrapper; ensure you reference toggleStartRadio, toggleStartRadioInternal, and startRadioSeamlessly when making the change so the IllegalStateException("No radio recommendations…") and similar errors are properly logged and reported.
1786-1792: Dead-code fallback:currentSong.first()on a StateFlow is equivalent tocurrentSong.value.
currentSongis aStateFlow<Song?>(declared at Lines 306-310), andStateFlow.first()returns the current.valueimmediately — it does not wait for a non-null emission. SocurrentSong.value ?: currentSong.first() ?: return@withLockcollapses tocurrentSong.value ?: return@withLock. If the intent was to actually wait for a non-null song (useful right after session init), it needs a predicate.Same pattern repeats at Line 1863 (
toggleLikeInternal) and Line 1970 (addToTargetPlaylistInternal).♻️ Pick one of the two intents
If the fallback was never meant to wait, drop it:
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock + val songToToggle = currentSong.value ?: return@withLockIf the intent is to wait for a non-null song (e.g., toggle fired before the DB flow emitted), be explicit:
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock + val songToToggle = currentSong.value ?: currentSong.first { it != null } ?: return@withLockApply the same change at Lines 1863 and 1970.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1786 - 1792, The code uses currentSong.value ?: currentSong.first() which is redundant because StateFlow.first() returns the current value; decide one intent and update all three functions (toggleLibraryInternal, toggleLikeInternal, addToTargetPlaylistInternal): if you do not want to wait, remove the .first() fallback and use currentSong.value ?: return; if you want to wait for a non-null song, replace currentSong.first() with currentSong.first { it != null } and then unwrap the non-null (e.g., val songToToggle = currentSong.value ?: currentSong.first { it != null }!!.also { /* or safe cast */ }), making the same change in the corresponding lines for toggleLikeInternal and addToTargetPlaylistInternal so you either consistently use immediate .value or explicit first { it != null } to await a non-null emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1423-1434: The current startRadioSeamlessly wrapper catches all
Exceptions and forwards them to reportException, which causes benign
IllegalStateException("No radio recommendations available for current track")
thrown by toggleStartRadioInternal to be reported; update startRadioSeamlessly
to special-case that expected failure by adding a specific catch for
IllegalStateException (or inspect the exception message) and handle it quietly
(e.g., debug/log + return) while preserving the existing catch for other
Exceptions that calls reportException; refer to startRadioSeamlessly,
toggleStartRadioInternal, and reportException when making the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 1444: The method creates a local val named currentMediaMetadata that
shadows the class-level MutableStateFlow currentMediaMetadata; rename the local
to something like playerMetadata to avoid confusion. Specifically, replace the
local declaration that assigns player.currentMetadata (e.g., val
currentMediaMetadata = player.currentMetadata ?: return) with a distinct name
(e.g., val playerMetadata = player.currentMetadata ?: return) and update all
subsequent references in the method to use playerMetadata so the class property
currentMediaMetadata remains unshadowed.
- Around line 2058-2062: toggleStartRadio currently launches
toggleStartRadioInternal under SilentHandler and thus swallows non-cancellation
exceptions; update it to follow the same error-handling pattern used by
toggleLike/toggleLibrary/addToTargetPlaylist by wrapping the suspend call in try
{ ... } catch (CancellationException) { throw } catch (Exception e) { log the
error and call reportException(e) }, or simply delegate from toggleStartRadio to
startRadioSeamlessly() which already implements that wrapper; ensure you
reference toggleStartRadio, toggleStartRadioInternal, and startRadioSeamlessly
when making the change so the IllegalStateException("No radio recommendations…")
and similar errors are properly logged and reported.
- Around line 1786-1792: The code uses currentSong.value ?: currentSong.first()
which is redundant because StateFlow.first() returns the current value; decide
one intent and update all three functions (toggleLibraryInternal,
toggleLikeInternal, addToTargetPlaylistInternal): if you do not want to wait,
remove the .first() fallback and use currentSong.value ?: return; if you want to
wait for a non-null song, replace currentSong.first() with currentSong.first {
it != null } and then unwrap the non-null (e.g., val songToToggle =
currentSong.value ?: currentSong.first { it != null }!!.also { /* or safe cast
*/ }), making the same change in the corresponding lines for toggleLikeInternal
and addToTargetPlaylistInternal so you either consistently use immediate .value
or explicit first { it != null } to await a non-null emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da5bd14a-4bdc-4152-aace-f2242505fd26
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…or reporting Address CodeRabbit feedback by treating the expected no radio recommendations IllegalStateException in startRadioSeamlessly as a benign outcome, logging it at debug level instead of reporting it, while preserving reportException for unexpected IllegalStateException cases and other failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (5)
2005-2063: Episode save-for-later now has rollback parity — matches the pattern used intoggleLibraryInternal/toggleLikeInternal.Previous-state capture, optimistic DB write, id-guarded in-memory metadata merge, immediate notification/widget refresh, and try/catch around
syncUtils.saveEpisodewith DB rollback + metadata re-merge on failure.CancellationExceptionis rethrown before rollback runs. Good.Very minor nit: Line 2013 uses fully-qualified
java.time.LocalDateTime.now()while the rest of the file (e.g. Lines 1803, 1887) uses the importedLocalDateTime.now(). Worth normalizing for consistency.✏️ Suggested diff
- val updatedInLibrary = if (isCurrentlySaved) null else java.time.LocalDateTime.now() + val updatedInLibrary = if (isCurrentlySaved) null else LocalDateTime.now()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 2005 - 2063, The code in toggleEpisodeSaveForLater uses the fully-qualified java.time.LocalDateTime.now() (creating an inconsistency with the rest of the file); replace that usage with the imported LocalDateTime.now() to match existing style (ensure there's an import for java.time.LocalDateTime at the top if missing) so the timestamp assignment for updatedInLibrary follows the same import pattern as other uses like in toggleLibraryInternal / toggleLikeInternal.
1793-1853: Library toggle rollback pattern looks solid.Optimistic DB write → remote sync → DB re-read → id-guarded metadata merge → notification/widget refresh, with a symmetric rollback that re-reads the row after attempting to restore and rethrows non-cancellation exceptions. Cancellation is correctly rethrown before the rollback path as well. Nice.
Minor formatting nit on Line 1852 — the trailing
} }has collapsed braces with stray whitespace between them; not functional, just ugly when scanning diffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1793 - 1853, Trailing collapsed braces and stray whitespace at the end of toggleLibraryInternal make the diff noisy; fix by normalizing the closing braces for the function and surrounding blocks (function toggleLibraryInternal and its inner withLock scope) so each closing brace is on its own line with standard indentation and no extra spaces between them.
1423-1549: Radio path: serialization + sentinel-based no-recommendations surfacing is clean.
toggleStartRadioInternalbeing gated bystartRadioMutexprevents concurrent queue mutation between the session-command path andstartRadioSeamlessly(),hasAppliedRadioItemscorrectly covers both the primary and fallback branches, and throwingIllegalStateExceptionwhen neither applies lets callers differentiate benign empty-results from real errors.startRadioSeamlesslynow narrows that case to a debug log (Lines 1429‑1435), matching the commit message's intent.One thing to watch: the sentinel is matched by message string (
"No radio recommendations available for current track"). If you touch this again, consider a dedicated private exception (e.g.private object NoRadioRecommendationsException : IllegalStateException(...)) to avoid the string-compare; purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1423 - 1549, The code currently signals "no recommendations" by throwing IllegalStateException with a specific message and startRadioSeamlessly checks the message string; change this to a dedicated exception type to avoid fragile string comparisons: define a private exception (e.g. private class NoRadioRecommendationsException : IllegalStateException(... ) or private object) and throw that from toggleStartRadioInternal instead of IllegalStateException("No radio recommendations available for current track"); then update startRadioSeamlessly to catch NoRadioRecommendationsException explicitly in its catch clauses (replacing the message equality check) while preserving the existing logging behavior and other exception handling.
1975-2003: Duplicate-insert guard and IO offload look correct; one tiny polish.Serializing under
addToTargetPlaylistMutex, readingAndroidAutoTargetPlaylistKeyon IO, the early-return for theTARGET_PLAYLIST_AUTOsentinel, and thecheckInPlaylist > 0guard beforeaddSongsToPlaylisttogether address rapid-tap duplicate inserts cleanly.Optional nit: the toast posted via
Handler(Looper.getMainLooper()).post { ... }at Lines 1984‑1991 could use the existingscopewithDispatchers.Main(orwithContext(Dispatchers.Main)) for consistency with the rest of the file, avoiding a one-offHandlerallocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1975 - 2003, In MusicService.addToTargetPlaylistInternal, replace the Handler(Looper.getMainLooper()).post { Toast.makeText(...).show() } block with a coroutine Main dispatcher call (e.g. use withContext(Dispatchers.Main) { Toast.makeText(this@MusicService, getString(R.string.android_auto_target_playlist_not_set), Toast.LENGTH_SHORT).show() } or scope.launch(Dispatchers.Main) { ... }) so the toast runs on the main thread without allocating a one-off Handler; keep the exact message and target (this@MusicService, getString(...), Toast.LENGTH_SHORT) and leave the surrounding early-return logic unchanged.
681-696:distinctUntilChangedByover the relevant song fields is a nice replacement for the 1s debounce.Using
listOf(id, liked, inLibrary, title, thumbnailUrl, artists joined)as the dedupe key removes latency on favorite toggles while still avoiding spurious refreshes when unrelated fields change. Two tiny notes — neither a blocker:
- The
joinToString("|")key is O(n) per emission and theoretically collides if an artist name literally contains|. Realistically fine; if you ever want to be safe, aList<String>of artist names (orartists.map { it.name }) compares element-wise without a separator.- A small
data class NotificationKey(...)would read more self-documenting than an anonymouslistOf(...)and make future additions (e.g.isEpisode) harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 681 - 696, The current distinctUntilChangedBy key uses listOf(..., artists.joinToString("|")...) which is O(n) per emission and can collide if an artist name contains "|" — change the key to use a structural List of artist names (e.g. artists.map { it.name }) or, better, introduce a small data class (e.g. NotificationKey(id: String, liked: Boolean, inLibrary: Boolean, title: String?, thumbnailUrl: String?, artists: List<String>)) and use that as the dedupe key in currentSong.distinctUntilChangedBy; update usages in MusicService (the currentSong distinctUntilChangedBy block and the updateNotification/updateWidgetUI flow) to construct and return NotificationKey instead of the anonymous list to make comparisons element-wise and more self-documenting for future fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2065-2069: toggleStartRadio() currently launches
toggleStartRadioInternal() under SilentHandler and swallows all exceptions;
change it to mirror startRadioSeamlessly(): call toggleStartRadioInternal()
inside a try/catch when launched from toggleStartRadio() (still using
SilentHandler), catch IllegalStateException with message "No radio
recommendations available for current track" and log it at debug/verbose, and
forward any other Throwable to reportException so real failures are reported;
optionally extract the common try/catch wrapper used by startRadioSeamlessly()
and toggleStartRadio() into a private helper (e.g. runStartRadioSafely) to avoid
duplicating the sentinel handling.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2005-2063: The code in toggleEpisodeSaveForLater uses the
fully-qualified java.time.LocalDateTime.now() (creating an inconsistency with
the rest of the file); replace that usage with the imported LocalDateTime.now()
to match existing style (ensure there's an import for java.time.LocalDateTime at
the top if missing) so the timestamp assignment for updatedInLibrary follows the
same import pattern as other uses like in toggleLibraryInternal /
toggleLikeInternal.
- Around line 1793-1853: Trailing collapsed braces and stray whitespace at the
end of toggleLibraryInternal make the diff noisy; fix by normalizing the closing
braces for the function and surrounding blocks (function toggleLibraryInternal
and its inner withLock scope) so each closing brace is on its own line with
standard indentation and no extra spaces between them.
- Around line 1423-1549: The code currently signals "no recommendations" by
throwing IllegalStateException with a specific message and startRadioSeamlessly
checks the message string; change this to a dedicated exception type to avoid
fragile string comparisons: define a private exception (e.g. private class
NoRadioRecommendationsException : IllegalStateException(... ) or private object)
and throw that from toggleStartRadioInternal instead of
IllegalStateException("No radio recommendations available for current track");
then update startRadioSeamlessly to catch NoRadioRecommendationsException
explicitly in its catch clauses (replacing the message equality check) while
preserving the existing logging behavior and other exception handling.
- Around line 1975-2003: In MusicService.addToTargetPlaylistInternal, replace
the Handler(Looper.getMainLooper()).post { Toast.makeText(...).show() } block
with a coroutine Main dispatcher call (e.g. use withContext(Dispatchers.Main) {
Toast.makeText(this@MusicService,
getString(R.string.android_auto_target_playlist_not_set),
Toast.LENGTH_SHORT).show() } or scope.launch(Dispatchers.Main) { ... }) so the
toast runs on the main thread without allocating a one-off Handler; keep the
exact message and target (this@MusicService, getString(...), Toast.LENGTH_SHORT)
and leave the surrounding early-return logic unchanged.
- Around line 681-696: The current distinctUntilChangedBy key uses listOf(...,
artists.joinToString("|")...) which is O(n) per emission and can collide if an
artist name contains "|" — change the key to use a structural List of artist
names (e.g. artists.map { it.name }) or, better, introduce a small data class
(e.g. NotificationKey(id: String, liked: Boolean, inLibrary: Boolean, title:
String?, thumbnailUrl: String?, artists: List<String>)) and use that as the
dedupe key in currentSong.distinctUntilChangedBy; update usages in MusicService
(the currentSong distinctUntilChangedBy block and the
updateNotification/updateWidgetUI flow) to construct and return NotificationKey
instead of the anonymous list to make comparisons element-wise and more
self-documenting for future fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db516202-9890-47ba-9144-b519a73468ee
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…ntry points Address CodeRabbit feedback by routing toggleStartRadio through the same guarded wrapper as startRadioSeamlessly, treating the expected no radio recommendations IllegalStateException as a debug-only outcome, and reporting unexpected failures through reportException while reusing a shared sentinel message constant.
…ng with consistent failure reporting and typed no-recommendation handling
…vent fallback on cancellation
1 participant
Problem
Media notification behaviour was inconsistent and could feel laggy due to the 1 second debounce window. Podcast episodes also could not be visually liked from notification (heart state did not reliably reflect saved state). Notification action flows could further become inconsistent under rapid user interaction.
Cause
Custom media session commands were previously executed in a fire-and-forget style and returned success immediately. Playback actions could run concurrently without per-action serialization, which increased race risk for like, library, radio, and quick-add actions. Notification refresh depended on a debounce-based update path, and favourite state logic did not consistently apply episode semantics or immediate metadata refresh after episode save toggles.
Solution
inLibraryand song state toliked.Testing
./gradlew :app:assembleFossDebugRelated Issues
N/A
Summary by CodeRabbit
Bug Fixes
Refactor