address review: cancellation-safe star, one star handler, focus logic tested

- setStar no longer treats cancellation as a rejection: rethrow CancellationException
  instead of toasting/rolling back, and roll back under NonCancellable on a real failure
- serialise setStar so overlapping toggles cannot clobber each other's rollback
- the menu star item reverts its label when the request fails (setStar now returns the
  result), so the open menu cannot drift from the row
- ContextMenuHost.dismiss clears the actions so a popped screen's ViewModel is released
- one shared setStar from the app shell instead of five per-ViewModel copies
- extract the nav-focus decision into focusPops and unit-test it

app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt

@@ -17,8 +17,13 @@ import com.subtracks.data.prefs.PlaylistSort
 import com.subtracks.data.prefs.SongSort
 import com.subtracks.data.prefs.StarredFilter
 import com.subtracks.data.source.StarType
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
+import kotlinx.coroutines.withContext
 
 class LibraryRepository(
     private val db: SubtracksDatabase,
@@ -242,22 +247,32 @@ class LibraryRepository(
         playlistId: String,
     ): Flow<PagingData<SongListItem>> = pager(60) { db.libraryDao().playlistSongs(sourceId, playlistId) }
 
+    private val starLock = Mutex()
+
     suspend fun setStar(
         type: StarType,
         id: String,
         starred: Boolean,
-    ): Result<Unit> {
-        val sourceId = sourceRepository.activeSourceIdOnce()
-        val source = sourceRepository.activeMusicSource()
-        if (sourceId == null || source == null) return Result.failure(IllegalStateException("No active server"))
-        val previous = starredValue(sourceId, type, id)
-        updateStarred(sourceId, type, id, if (starred) System.currentTimeMillis() else null)
-        return runCatching { source.setStar(type, id, starred) }
-            .onFailure {
-                updateStarred(sourceId, type, id, previous)
-                showMessage("Could not update star")
+    ): Result<Unit> =
+        starLock.withLock {
+            val sourceId = sourceRepository.activeSourceIdOnce()
+            val source = sourceRepository.activeMusicSource()
+            if (sourceId == null || source == null) {
+                return@withLock Result.failure(IllegalStateException("No active server"))
             }
-    }
+            val previous = starredValue(sourceId, type, id)
+            updateStarred(sourceId, type, id, if (starred) System.currentTimeMillis() else null)
+            try {
+                source.setStar(type, id, starred)
+                Result.success(Unit)
+            } catch (cancellation: CancellationException) {
+                throw cancellation
+            } catch (failure: Exception) {
+                withContext(NonCancellable) { updateStarred(sourceId, type, id, previous) }
+                showMessage("Could not update star")
+                Result.failure(failure)
+            }
+        }
 
     private suspend fun starredValue(
         sourceId: Long,

app/src/main/kotlin/com/subtracks/ui/AppRoot.kt

@@ -52,13 +52,13 @@ import androidx.lifecycle.ViewModelStoreOwner
 import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import androidx.lifecycle.viewModelScope
 import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
-import androidx.navigation.NavBackStackEntry
 import androidx.navigation.NavType
 import androidx.navigation.compose.NavHost
 import androidx.navigation.compose.composable
 import androidx.navigation.compose.currentBackStackEntryAsState
 import androidx.navigation.compose.rememberNavController
 import androidx.navigation.navArgument
+import com.subtracks.data.repo.LibraryRepository
 import com.subtracks.data.repo.SourceRepository
 import com.subtracks.playback.PlaybackController
 import com.subtracks.ui.components.ContextMenuHost
@@ -123,6 +123,20 @@ private object Routes {
     fun playlist(id: String) = "playlist/${Uri.encode(id)}"
 }
 
+internal data class BackStackKey(
+    val route: String?,
+    val argument: String?,
+)
+
+internal fun focusPops(
+    stack: List<BackStackKey>,
+    pattern: String,
+    value: String,
+): Int? {
+    val index = stack.indexOfLast { it.route == pattern && it.argument == value }
+    return if (index < 0) null else stack.size - 1 - index
+}
+
 @Composable
 fun SubtracksRoot(root: RootViewModel = koinViewModel()) {
     when (val hasSource = root.hasSource.collectAsStateWithLifecycle().value) {
@@ -144,6 +158,7 @@ private fun MainNavigation() {
             ?.route
     val tabBarVisible = currentRoute == Routes.LIBRARY
     val playbackController = koinInject<PlaybackController>()
+    val libraryRepository = koinInject<LibraryRepository>()
     val playback by playbackController.state.collectAsStateWithLifecycle()
     var showingQueue by rememberSaveable { mutableStateOf(false) }
     var nowPlayingOpen by rememberSaveable { mutableStateOf(false) }
@@ -188,17 +203,14 @@ private fun MainNavigation() {
         value: String,
         route: String,
     ) {
-        val matches: (NavBackStackEntry?) -> Boolean = { entry ->
-            entry?.destination?.route == pattern && entry.arguments?.getString(argument) == value
-        }
-        if (matches(navController.currentBackStackEntry)) return
-        if (navController.currentBackStack.value.none(matches)) {
-            navController.navigate(route)
-            return
-        }
-        var guard = navController.currentBackStack.value.size
-        while (guard-- > 0 && !matches(navController.currentBackStackEntry) && navController.popBackStack()) {
-            // pop until the matching entry is back on top
+        val stack =
+            navController.currentBackStack.value.map {
+                BackStackKey(it.destination.route, it.arguments?.getString(argument))
+            }
+        when (val pops = focusPops(stack, pattern, value)) {
+            null -> navController.navigate(route)
+            0 -> Unit
+            else -> repeat(pops) { navController.popBackStack() }
         }
     }
     LaunchedEffect(Unit) { playbackController.connect() }
@@ -273,6 +285,7 @@ private fun MainNavigation() {
                                 navigateDetail(Routes.ARTIST_DETAIL, "artistId", artistId, Routes.artist(artistId))
                             },
                             contextMenuHost = contextMenuHost,
+                            setStar = libraryRepository::setStar,
                             bottomInset = bottomInset,
                         )
                     }
@@ -305,6 +318,7 @@ private fun MainNavigation() {
                                 navigateDetail(Routes.ARTIST_DETAIL, "artistId", artistId, Routes.artist(artistId))
                             },
                             contextMenuHost = contextMenuHost,
+                            setStar = libraryRepository::setStar,
                         )
                     }
                     composable(
@@ -331,6 +345,7 @@ private fun MainNavigation() {
                             },
                             onViewAlbum = { albumId -> navigateDetail(Routes.ALBUM_DETAIL, "albumId", albumId, Routes.album(albumId)) },
                             contextMenuHost = contextMenuHost,
+                            setStar = libraryRepository::setStar,
                         )
                     }
                     composable(
@@ -345,6 +360,7 @@ private fun MainNavigation() {
                                 navigateDetail(Routes.ARTIST_DETAIL, "artistId", artistId, Routes.artist(artistId))
                             },
                             contextMenuHost = contextMenuHost,
+                            setStar = libraryRepository::setStar,
                         )
                     }
                     composable(Routes.ADD_SERVER) {
@@ -416,6 +432,7 @@ private fun MainNavigation() {
                             fadeNowPlaying()
                         },
                         contextMenuHost = contextMenuHost,
+                        setStar = libraryRepository::setStar,
                         modifier = Modifier.fillMaxSize(),
                     )
                 }

app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt

@@ -95,7 +95,7 @@ class ItemActions(
     val shuffleAlbum: (AlbumModel) -> Unit = {},
     val playPlaylist: (PlaylistModel) -> Unit = {},
     val shufflePlaylist: (PlaylistModel) -> Unit = {},
-    val setStar: (StarType, String, Boolean) -> Unit = { _, _, _ -> },
+    val setStar: suspend (StarType, String, Boolean) -> Result<Unit> = { _, _, _ -> Result.success(Unit) },
     val viewAlbum: ((String) -> Unit)? = null,
     val viewArtist: ((String) -> Unit)? = null,
 )
@@ -116,6 +116,7 @@ class ContextMenuHost {
 
     fun dismiss() {
         target = null
+        actions = ItemActions()
     }
 }
 
@@ -227,13 +228,17 @@ private fun MenuHeader(target: MenuTarget) {
 @Composable
 private fun StarItem(
     current: Long?,
-    onToggle: (Boolean) -> Unit,
+    onSet: suspend (Boolean) -> Result<Unit>,
 ) {
     var starred by remember(current) { mutableStateOf(current) }
+    val scope = rememberCoroutineScope()
     MenuItem(starIcon(starred), starLabel(starred)) {
-        val starring = starred == null
+        val previous = starred
+        val starring = previous == null
         starred = if (starring) System.currentTimeMillis() else null
-        onToggle(starring)
+        scope.launch {
+            if (onSet(starring).isFailure) starred = previous
+        }
     }
 }
 

app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt

@@ -25,6 +25,7 @@ import com.subtracks.data.model.CoverArtRef
 import com.subtracks.data.model.Disc
 import com.subtracks.data.model.QueueKind
 import com.subtracks.data.model.Song
+import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import com.subtracks.ui.components.ContextMenuHost
 import com.subtracks.ui.components.HeroDetailScaffold
@@ -47,6 +48,7 @@ fun AlbumDetailRoute(
     onBack: () -> Unit,
     onViewArtist: (String) -> Unit,
     contextMenuHost: ContextMenuHost? = null,
+    setStar: suspend (StarType, String, Boolean) -> Result<Unit>,
     viewModel: AlbumDetailViewModel = koinViewModel(key = albumId) { parametersOf(albumId) },
     playbackController: PlaybackController = koinInject(),
 ) {
@@ -61,7 +63,7 @@ fun AlbumDetailRoute(
             playSong = { song -> songs.indexOfFirst { it.id == song.id }.takeIf { it >= 0 }?.let(viewModel::play) },
             playAlbum = { viewModel.playAll() },
             shuffleAlbum = { viewModel.shuffle() },
-            setStar = viewModel::setStar,
+            setStar = setStar,
             viewArtist = onViewArtist,
         )
     AlbumDetailScreen(

app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailViewModel.kt

@@ -8,7 +8,6 @@ import com.subtracks.data.model.Disc
 import com.subtracks.data.model.Song
 import com.subtracks.data.repo.LibraryRepository
 import com.subtracks.data.repo.SourceRepository
-import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.Flow
@@ -54,12 +53,4 @@ class AlbumDetailViewModel(
             playbackController.shuffleAlbum(sourceId.first(), albumId)
         }
     }
-
-    fun setStar(
-        type: StarType,
-        id: String,
-        starred: Boolean,
-    ) {
-        viewModelScope.launch { libraryRepository.setStar(type, id, starred) }
-    }
 }

app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt

@@ -41,9 +41,7 @@ import androidx.compose.runtime.Composable
 import androidx.compose.runtime.CompositionLocalProvider
 import androidx.compose.runtime.derivedStateOf
 import androidx.compose.runtime.getValue
-import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
-import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.alpha
@@ -61,6 +59,7 @@ import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import com.subtracks.data.model.Album
 import com.subtracks.data.model.Artist
 import com.subtracks.data.model.CoverArtRef
+import com.subtracks.data.source.StarType
 import com.subtracks.ui.components.ContextMenuHost
 import com.subtracks.ui.components.CoverArt
 import com.subtracks.ui.components.ItemActions
@@ -92,6 +91,7 @@ fun ArtistDetailRoute(
     onAlbumClick: (Album) -> Unit,
     onViewAlbum: (String) -> Unit,
     contextMenuHost: ContextMenuHost? = null,
+    setStar: suspend (StarType, String, Boolean) -> Result<Unit>,
     viewModel: ArtistDetailViewModel = koinViewModel(key = artistId) { parametersOf(artistId) },
 ) {
     val artist by viewModel.artist.collectAsStateWithLifecycle(initialValue = null)
@@ -103,7 +103,7 @@ fun ArtistDetailRoute(
         ItemActions(
             playAlbum = { viewModel.playAlbum(it.id) },
             shuffleAlbum = { viewModel.shuffleAlbum(it.id) },
-            setStar = viewModel::setStar,
+            setStar = setStar,
             viewAlbum = onViewAlbum,
         )
     ArtistDetailScreen(

app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailViewModel.kt

@@ -7,7 +7,6 @@ import com.subtracks.data.model.Artist
 import com.subtracks.data.model.CoverArtRef
 import com.subtracks.data.repo.LibraryRepository
 import com.subtracks.data.repo.SourceRepository
-import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.delay
@@ -68,14 +67,6 @@ class ArtistDetailViewModel(
         thumbnail: Boolean,
     ): CoverArtRef? = sourceRepository.coverArt(coverArt, thumbnail)
 
-    fun setStar(
-        type: StarType,
-        id: String,
-        starred: Boolean,
-    ) {
-        viewModelScope.launch { libraryRepository.setStar(type, id, starred) }
-    }
-
     fun playAlbum(albumId: String) {
         viewModelScope.launch {
             playbackController.playAlbum(sourceId.first(), albumId, 0)

app/src/main/kotlin/com/subtracks/ui/library/LibraryScreen.kt

@@ -113,6 +113,7 @@ import com.subtracks.data.prefs.ListQuery
 import com.subtracks.data.prefs.PlaylistSort
 import com.subtracks.data.prefs.SongSort
 import com.subtracks.data.prefs.StarredFilter
+import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import com.subtracks.ui.components.ContextMenuHost
 import com.subtracks.ui.components.ItemActions
@@ -195,6 +196,7 @@ fun LibraryRoute(
     onViewAlbum: (String) -> Unit,
     onViewArtist: (String) -> Unit,
     contextMenuHost: ContextMenuHost? = null,
+    setStar: suspend (StarType, String, Boolean) -> Result<Unit>,
     bottomInset: Dp,
     viewModel: LibraryViewModel = koinViewModel(),
     playbackController: PlaybackController = koinInject(),
@@ -227,7 +229,7 @@ fun LibraryRoute(
             shuffleAlbum = { viewModel.shuffleAlbum(it.id) },
             playPlaylist = { viewModel.playPlaylist(it.id) },
             shufflePlaylist = { viewModel.shufflePlaylist(it.id) },
-            setStar = viewModel::setStar,
+            setStar = setStar,
             viewAlbum = onViewAlbum,
             viewArtist = onViewArtist,
         )

app/src/main/kotlin/com/subtracks/ui/library/LibraryViewModel.kt

@@ -20,7 +20,6 @@ import com.subtracks.data.prefs.UserPreferences
 import com.subtracks.data.repo.LibraryRepository
 import com.subtracks.data.repo.QueueSongQuery
 import com.subtracks.data.repo.SourceRepository
-import com.subtracks.data.source.StarType
 import com.subtracks.data.sync.SyncManager
 import com.subtracks.data.sync.SyncStatus
 import com.subtracks.playback.PlaybackController
@@ -163,14 +162,6 @@ class LibraryViewModel(
         }
     }
 
-    fun setStar(
-        type: StarType,
-        id: String,
-        starred: Boolean,
-    ) {
-        viewModelScope.launch { libraryRepository.setStar(type, id, starred) }
-    }
-
     fun sync() = syncManager.requestSync()
 }
 

app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt

@@ -15,6 +15,7 @@ import com.subtracks.data.model.CoverArtRef
 import com.subtracks.data.model.Playlist
 import com.subtracks.data.model.QueueKind
 import com.subtracks.data.model.SongListItem
+import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import com.subtracks.ui.components.ContextMenuHost
 import com.subtracks.ui.components.HeroDetailScaffold
@@ -37,6 +38,7 @@ fun PlaylistDetailRoute(
     onViewAlbum: (String) -> Unit,
     onViewArtist: (String) -> Unit,
     contextMenuHost: ContextMenuHost? = null,
+    setStar: suspend (StarType, String, Boolean) -> Result<Unit>,
     viewModel: PlaylistDetailViewModel = koinViewModel(key = playlistId) { parametersOf(playlistId) },
     playbackController: PlaybackController = koinInject(),
 ) {
@@ -47,7 +49,7 @@ fun PlaylistDetailRoute(
         ItemActions(
             playPlaylist = { viewModel.playAll() },
             shufflePlaylist = { viewModel.shuffle() },
-            setStar = viewModel::setStar,
+            setStar = setStar,
             viewAlbum = onViewAlbum,
             viewArtist = onViewArtist,
         )

app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailViewModel.kt

@@ -9,7 +9,6 @@ import com.subtracks.data.model.Playlist
 import com.subtracks.data.model.SongListItem
 import com.subtracks.data.repo.LibraryRepository
 import com.subtracks.data.repo.SourceRepository
-import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.Flow
@@ -56,12 +55,4 @@ class PlaylistDetailViewModel(
             playbackController.shufflePlaylist(sourceId.first(), playlistId)
         }
     }
-
-    fun setStar(
-        type: StarType,
-        id: String,
-        starred: Boolean,
-    ) {
-        viewModelScope.launch { libraryRepository.setStar(type, id, starred) }
-    }
 }

app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt

@@ -45,7 +45,6 @@ import androidx.compose.runtime.mutableFloatStateOf
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.produceState
 import androidx.compose.runtime.remember
-import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
@@ -66,6 +65,7 @@ import com.subtracks.data.model.CoverArtRef
 import com.subtracks.data.model.QueueKind
 import com.subtracks.data.model.Song
 import com.subtracks.data.repo.LibraryRepository
+import com.subtracks.data.source.StarType
 import com.subtracks.playback.PlaybackController
 import com.subtracks.playback.PlaybackState
 import com.subtracks.playback.RepeatMode
@@ -78,7 +78,6 @@ import com.subtracks.ui.theme.ArtworkSeedCache
 import com.subtracks.ui.theme.ArtworkTheme
 import com.subtracks.ui.theme.HeroGradient
 import com.subtracks.ui.theme.rememberArtworkColors
-import kotlinx.coroutines.launch
 import org.koin.compose.koinInject
 
 @Composable
@@ -88,6 +87,7 @@ fun NowPlayingRoute(
     onViewAlbum: (String) -> Unit,
     onViewArtist: (String) -> Unit,
     contextMenuHost: ContextMenuHost? = null,
+    setStar: suspend (StarType, String, Boolean) -> Result<Unit>,
     modifier: Modifier = Modifier,
     controller: PlaybackController = koinInject(),
     libraryRepository: LibraryRepository = koinInject(),
@@ -103,10 +103,9 @@ fun NowPlayingRoute(
             }
         }
     val context = LocalPlatformContext.current
-    val scope = rememberCoroutineScope()
     val actions =
         ItemActions(
-            setStar = { type, id, starred -> scope.launch { libraryRepository.setStar(type, id, starred) } },
+            setStar = setStar,
             viewAlbum = onViewAlbum,
             viewArtist = onViewArtist,
         )

app/src/test/kotlin/com/subtracks/ui/FocusPopsTest.kt

@@ -0,0 +1,39 @@
+package com.subtracks.ui
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Test
+
+class FocusPopsTest {
+    private val library = BackStackKey("library", null)
+    private val artistA = BackStackKey("artist/{artistId}?coverArt={coverArt}", "ar1")
+    private val albumA = BackStackKey("album/{albumId}?coverArt={coverArt}", "al1")
+    private val albumB = BackStackKey("album/{albumId}?coverArt={coverArt}", "al2")
+
+    @Test
+    fun anAbsentTargetNavigates() {
+        assertNull(focusPops(listOf(library, albumA), "artist/{artistId}?coverArt={coverArt}", "ar1"))
+    }
+
+    @Test
+    fun anEmptyStackNavigates() {
+        assertNull(focusPops(emptyList(), "album/{albumId}?coverArt={coverArt}", "al1"))
+    }
+
+    @Test
+    fun aTargetAlreadyOnTopNeedsNoPops() {
+        assertEquals(0, focusPops(listOf(library, artistA), "artist/{artistId}?coverArt={coverArt}", "ar1"))
+    }
+
+    @Test
+    fun aTargetUnderTheTopPopsDownToIt() {
+        assertEquals(1, focusPops(listOf(library, artistA, albumA), "artist/{artistId}?coverArt={coverArt}", "ar1"))
+    }
+
+    @Test
+    fun theMatchingEntryIsChosenNotJustTheDestination() {
+        val stack = listOf(library, albumA, artistA, albumB)
+        assertEquals(2, focusPops(stack, "album/{albumId}?coverArt={coverArt}", "al1"))
+        assertEquals(0, focusPops(stack, "album/{albumId}?coverArt={coverArt}", "al2"))
+    }
+}