From b25b9deacd899d50736f6f822e33ef84b5ce0af0 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sat, 3 Oct 2026 09:12:32 +0000 Subject: [PATCH] search: address review findings Prefix result keys per type so ids shared across songs, albums, artists and playlists cannot collide and crash the list; add a loading state so a query in flight no longer reads as no results; gate on code points to match SQLite's length(); bind the three-character floor to one constant; label the search field for TalkBack; and drop the vacuous trigram-plan test. Cancel the ViewModel scope in its test teardown so it cannot leak into later Robolectric tests. --- .../com/subtracks/data/db/LibraryDao.kt | 10 +++++--- .../subtracks/data/repo/LibraryRepository.kt | 2 -- .../com/subtracks/ui/search/SearchScreen.kt | 25 +++++++++++++------ .../subtracks/ui/search/SearchViewModel.kt | 10 +++++--- .../com/subtracks/data/db/LibraryDaoTest.kt | 14 ----------- .../subtracks/ui/search/SearchScreenTest.kt | 24 ++++++++++++++++++ .../ui/search/SearchViewModelTest.kt | 12 +++++++++ 7 files changed, 67 insertions(+), 30 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt index 31af298e..0c408c11 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -130,20 +130,22 @@ internal const val PLAYLIST_ORDER_BY_UPDATED_REVERSED = "changed ASC, name COLLA // Cross-type search only runs from three characters, since a shorter term cannot use the trigram // index and scanning the whole library for it is not worth it. +const val SEARCH_MIN_LENGTH = 3 + internal const val ALBUM_SEARCH_FILTER = - "(length(:search) >= 3 AND rowid IN (SELECT rowid FROM album_search " + + "(length(:search) >= $SEARCH_MIN_LENGTH AND rowid IN (SELECT rowid FROM album_search " + "WHERE album_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))" internal const val ARTIST_SEARCH_FILTER = - "(length(:search) >= 3 AND rowid IN (SELECT rowid FROM artist_search " + + "(length(:search) >= $SEARCH_MIN_LENGTH AND rowid IN (SELECT rowid FROM artist_search " + "WHERE artist_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))" internal const val PLAYLIST_SEARCH_FILTER = - "(length(:search) >= 3 AND rowid IN (SELECT rowid FROM playlist_search " + + "(length(:search) >= $SEARCH_MIN_LENGTH AND rowid IN (SELECT rowid FROM playlist_search " + "WHERE playlist_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))" internal const val SONG_SEARCH_FILTER = - "(length(:search) >= 3 AND songs.rowid IN (SELECT rowid FROM song_search " + + "(length(:search) >= $SEARCH_MIN_LENGTH AND songs.rowid IN (SELECT rowid FROM song_search " + "WHERE song_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))" @Dao diff --git a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt index a9b1f3bf..694ceaa0 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt @@ -510,6 +510,4 @@ const val REDISCOVER_WINDOW_MS = 60L * 24 * 60 * 60 * 1000 const val SEARCH_RESULT_LIMIT = 50 -const val SEARCH_MIN_LENGTH = 3 - fun rediscoverCutoff(): Long = System.currentTimeMillis() - REDISCOVER_WINDOW_MS diff --git a/app/src/main/kotlin/com/subtracks/ui/search/SearchScreen.kt b/app/src/main/kotlin/com/subtracks/ui/search/SearchScreen.kt index ac77a2e2..190443cd 100644 --- a/app/src/main/kotlin/com/subtracks/ui/search/SearchScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/search/SearchScreen.kt @@ -29,20 +29,23 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.focus.FocusRequester import androidx.compose.ui.focus.focusRequester import androidx.compose.ui.graphics.Color +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.semantics import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.subtracks.data.db.SEARCH_MIN_LENGTH import com.subtracks.data.model.Album import com.subtracks.data.model.AlbumSongItem import com.subtracks.data.model.Artist import com.subtracks.data.model.CoverArtRef import com.subtracks.data.model.Playlist import com.subtracks.data.model.Song -import com.subtracks.data.repo.SEARCH_MIN_LENGTH import com.subtracks.data.source.StarType import com.subtracks.playback.PlaybackController import com.subtracks.ui.components.ContextMenuHost import com.subtracks.ui.components.EmptyState import com.subtracks.ui.components.ItemActions +import com.subtracks.ui.components.LoadingState import com.subtracks.ui.components.MenuTarget import com.subtracks.ui.home.AlbumListRow import com.subtracks.ui.home.ArtistListRow @@ -138,20 +141,28 @@ fun SearchScreen( focusedIndicatorColor = Color.Transparent, unfocusedIndicatorColor = Color.Transparent, ), - modifier = Modifier.fillMaxWidth().focusRequester(focusRequester), + modifier = + Modifier + .fillMaxWidth() + .focusRequester(focusRequester) + .semantics { contentDescription = "Search your library" }, ) }, ) }, ) { padding -> when { - query.length < SEARCH_MIN_LENGTH -> { + !searchReady(query) -> { EmptyState( text = "Type at least $SEARCH_MIN_LENGTH characters to search.", modifier = Modifier.padding(padding), ) } + results.loading -> { + LoadingState(Modifier.padding(padding)) + } + results.isEmpty -> { EmptyState( text = "No results for \u201C$query\u201D.", @@ -194,7 +205,7 @@ private fun LazyListScope.songResults( ) { if (songs.isEmpty()) return resultHeader("Songs") - items(songs, key = { it.song.id }) { item -> + items(songs, key = { "song-${it.song.id}" }) { item -> SongRow( song = item.song, coverArtId = item.coverArt, @@ -218,7 +229,7 @@ private fun LazyListScope.albumResults( ) { if (albums.isEmpty()) return resultHeader("Albums") - items(albums, key = { it.id }) { album -> + items(albums, key = { "album-${it.id}" }) { album -> AlbumListRow( album = album, coverArt = coverArt, @@ -237,7 +248,7 @@ private fun LazyListScope.artistResults( ) { if (artists.isEmpty()) return resultHeader("Artists") - items(artists, key = { it.id }) { artist -> + items(artists, key = { "artist-${it.id}" }) { artist -> ArtistListRow( artist = artist, coverArt = coverArt, @@ -256,7 +267,7 @@ private fun LazyListScope.playlistResults( ) { if (playlists.isEmpty()) return resultHeader("Playlists") - items(playlists, key = { it.id }) { playlist -> + items(playlists, key = { "playlist-${it.id}" }) { playlist -> PlaylistRow( playlist = playlist, coverArt = coverArt, diff --git a/app/src/main/kotlin/com/subtracks/ui/search/SearchViewModel.kt b/app/src/main/kotlin/com/subtracks/ui/search/SearchViewModel.kt index 2ac73795..ddb91a70 100644 --- a/app/src/main/kotlin/com/subtracks/ui/search/SearchViewModel.kt +++ b/app/src/main/kotlin/com/subtracks/ui/search/SearchViewModel.kt @@ -2,6 +2,7 @@ package com.subtracks.ui.search import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import com.subtracks.data.db.SEARCH_MIN_LENGTH import com.subtracks.data.model.Album import com.subtracks.data.model.AlbumSongItem import com.subtracks.data.model.Artist @@ -9,7 +10,6 @@ import com.subtracks.data.model.CoverArtRef import com.subtracks.data.model.Playlist import com.subtracks.data.model.Song import com.subtracks.data.repo.LibraryRepository -import com.subtracks.data.repo.SEARCH_MIN_LENGTH import com.subtracks.data.repo.SourceRepository import com.subtracks.playback.PlaybackController import com.subtracks.ui.home.playSongInContext @@ -21,6 +21,7 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.stateIn data class SearchResults( @@ -28,11 +29,14 @@ data class SearchResults( val artists: List = emptyList(), val playlists: List = emptyList(), val songs: List = emptyList(), + val loading: Boolean = false, ) { val isEmpty: Boolean get() = albums.isEmpty() && artists.isEmpty() && playlists.isEmpty() && songs.isEmpty() } +internal fun searchReady(query: String): Boolean = query.codePointCount(0, query.length) >= SEARCH_MIN_LENGTH + @OptIn(ExperimentalCoroutinesApi::class) class SearchViewModel( private val libraryRepository: LibraryRepository, @@ -46,7 +50,7 @@ class SearchViewModel( libraryRepository.activeSourceId .flatMapLatest { sourceId -> _query.flatMapLatest { text -> - if (sourceId == null || text.length < SEARCH_MIN_LENGTH) { + if (sourceId == null || !searchReady(text)) { flowOf(SearchResults()) } else { combine( @@ -56,7 +60,7 @@ class SearchViewModel( libraryRepository.searchSongs(sourceId, text), ) { albums, artists, playlists, songs -> SearchResults(albums, artists, playlists, songs) - } + }.onStart { emit(SearchResults(loading = true)) } } } }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), SearchResults()) diff --git a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt index 2b5beda3..db9c5a22 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -762,20 +762,6 @@ class LibraryDaoTest { assertEquals(2, dao.searchAlbums(sourceId, "oad", 2).first().size) } - @Test - fun songSearchRunsThroughTheTrigramIndex() = - runTest { - val plan = - plan( - "SELECT songs.*, albums.coverArt AS coverArt FROM songs " + - "LEFT JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + - "WHERE songs.sourceId = 1 AND ${SONG_SEARCH_FILTER.replace(":search", "'abc'")} " + - "ORDER BY songs.title COLLATE NOCASE, songs.id LIMIT 20 OFFSET 0", - ) - - assertTrue(plan, plan.contains("song_search")) - } - @Test fun crossTypeSearchIgnoresQueriesShorterThanThreeCharacters() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/ui/search/SearchScreenTest.kt b/app/src/test/kotlin/com/subtracks/ui/search/SearchScreenTest.kt index 81d59ca5..fdbf3f37 100644 --- a/app/src/test/kotlin/com/subtracks/ui/search/SearchScreenTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/search/SearchScreenTest.kt @@ -83,6 +83,30 @@ class SearchScreenTest { composeRule.onNodeWithText("No results", substring = true).assertIsDisplayed() } + @Test + fun idsSharedAcrossTypesDoNotCollide() { + composeRule.setContent { + SubtracksTheme { + SearchScreen( + query = "road", + onQueryChange = {}, + results = + SearchResults( + albums = listOf(album("1", "Road Album")), + artists = listOf(artist("1", "Road Artist")), + playlists = listOf(playlist("1", "Road Trip")), + songs = listOf(AlbumSongItem(song("1", "Road Song"), coverArt = null)), + ), + ) + } + } + + composeRule.onNodeWithText("Road Song").assertIsDisplayed() + composeRule.onNodeWithText("Road Album").assertIsDisplayed() + composeRule.onNodeWithText("Road Artist").assertIsDisplayed() + composeRule.onNodeWithText("Road Trip").assertIsDisplayed() + } + private fun album( id: String, name: String, diff --git a/app/src/test/kotlin/com/subtracks/ui/search/SearchViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/search/SearchViewModelTest.kt index a6aba47b..1c9d7793 100644 --- a/app/src/test/kotlin/com/subtracks/ui/search/SearchViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/search/SearchViewModelTest.kt @@ -1,6 +1,7 @@ package com.subtracks.ui.search import android.content.Context +import androidx.lifecycle.viewModelScope import androidx.room3.Room import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider @@ -27,8 +28,10 @@ import com.subtracks.playback.PlaybackController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.asCoroutineDispatcher +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.resetMain @@ -37,6 +40,7 @@ import kotlinx.coroutines.withTimeout import okhttp3.OkHttpClient import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -94,6 +98,7 @@ class SearchViewModelTest { @After fun tearDown() { + runBlocking { viewModel.viewModelScope.coroutineContext[Job]?.cancelAndJoin() } cancelAndJoinBlocking(controllerScope, repoScope) db.close() Dispatchers.resetMain() @@ -129,6 +134,13 @@ class SearchViewModelTest { assertTrue(blank.isEmpty) } + @Test + fun searchReadyCountsCodePointsRatherThanUtf16Units() { + assertTrue(searchReady("road")) + assertFalse(searchReady("ro")) + assertFalse(searchReady("a\uD83D\uDE00")) + } + @Test fun playingASearchResultPlaysItsAlbumInsteadOfASearchQueue() { val sourceId =