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.
This commit is contained in:
austinried
2026-10-04 00:30:07 +00:00
parent c1081b4ae7
commit b25b9deacd
7 changed files with 67 additions and 30 deletions
@@ -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 // 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. // index and scanning the whole library for it is not worth it.
const val SEARCH_MIN_LENGTH = 3
internal const val ALBUM_SEARCH_FILTER = 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, '\"', '\"\"') || '\"'))" "WHERE album_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))"
internal const val ARTIST_SEARCH_FILTER = 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, '\"', '\"\"') || '\"'))" "WHERE artist_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))"
internal const val PLAYLIST_SEARCH_FILTER = 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, '\"', '\"\"') || '\"'))" "WHERE playlist_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))"
internal const val SONG_SEARCH_FILTER = 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, '\"', '\"\"') || '\"'))" "WHERE song_search MATCH '\"' || replace(:search, '\"', '\"\"') || '\"'))"
@Dao @Dao
@@ -510,6 +510,4 @@ const val REDISCOVER_WINDOW_MS = 60L * 24 * 60 * 60 * 1000
const val SEARCH_RESULT_LIMIT = 50 const val SEARCH_RESULT_LIMIT = 50
const val SEARCH_MIN_LENGTH = 3
fun rediscoverCutoff(): Long = System.currentTimeMillis() - REDISCOVER_WINDOW_MS fun rediscoverCutoff(): Long = System.currentTimeMillis() - REDISCOVER_WINDOW_MS
@@ -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.focus.focusRequester import androidx.compose.ui.focus.focusRequester
import androidx.compose.ui.graphics.Color 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.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.subtracks.data.db.SEARCH_MIN_LENGTH
import com.subtracks.data.model.Album import com.subtracks.data.model.Album
import com.subtracks.data.model.AlbumSongItem import com.subtracks.data.model.AlbumSongItem
import com.subtracks.data.model.Artist import com.subtracks.data.model.Artist
import com.subtracks.data.model.CoverArtRef import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.Playlist import com.subtracks.data.model.Playlist
import com.subtracks.data.model.Song import com.subtracks.data.model.Song
import com.subtracks.data.repo.SEARCH_MIN_LENGTH
import com.subtracks.data.source.StarType import com.subtracks.data.source.StarType
import com.subtracks.playback.PlaybackController import com.subtracks.playback.PlaybackController
import com.subtracks.ui.components.ContextMenuHost import com.subtracks.ui.components.ContextMenuHost
import com.subtracks.ui.components.EmptyState import com.subtracks.ui.components.EmptyState
import com.subtracks.ui.components.ItemActions import com.subtracks.ui.components.ItemActions
import com.subtracks.ui.components.LoadingState
import com.subtracks.ui.components.MenuTarget import com.subtracks.ui.components.MenuTarget
import com.subtracks.ui.home.AlbumListRow import com.subtracks.ui.home.AlbumListRow
import com.subtracks.ui.home.ArtistListRow import com.subtracks.ui.home.ArtistListRow
@@ -138,20 +141,28 @@ fun SearchScreen(
focusedIndicatorColor = Color.Transparent, focusedIndicatorColor = Color.Transparent,
unfocusedIndicatorColor = Color.Transparent, unfocusedIndicatorColor = Color.Transparent,
), ),
modifier = Modifier.fillMaxWidth().focusRequester(focusRequester), modifier =
Modifier
.fillMaxWidth()
.focusRequester(focusRequester)
.semantics { contentDescription = "Search your library" },
) )
}, },
) )
}, },
) { padding -> ) { padding ->
when { when {
query.length < SEARCH_MIN_LENGTH -> { !searchReady(query) -> {
EmptyState( EmptyState(
text = "Type at least $SEARCH_MIN_LENGTH characters to search.", text = "Type at least $SEARCH_MIN_LENGTH characters to search.",
modifier = Modifier.padding(padding), modifier = Modifier.padding(padding),
) )
} }
results.loading -> {
LoadingState(Modifier.padding(padding))
}
results.isEmpty -> { results.isEmpty -> {
EmptyState( EmptyState(
text = "No results for \u201C$query\u201D.", text = "No results for \u201C$query\u201D.",
@@ -194,7 +205,7 @@ private fun LazyListScope.songResults(
) { ) {
if (songs.isEmpty()) return if (songs.isEmpty()) return
resultHeader("Songs") resultHeader("Songs")
items(songs, key = { it.song.id }) { item -> items(songs, key = { "song-${it.song.id}" }) { item ->
SongRow( SongRow(
song = item.song, song = item.song,
coverArtId = item.coverArt, coverArtId = item.coverArt,
@@ -218,7 +229,7 @@ private fun LazyListScope.albumResults(
) { ) {
if (albums.isEmpty()) return if (albums.isEmpty()) return
resultHeader("Albums") resultHeader("Albums")
items(albums, key = { it.id }) { album -> items(albums, key = { "album-${it.id}" }) { album ->
AlbumListRow( AlbumListRow(
album = album, album = album,
coverArt = coverArt, coverArt = coverArt,
@@ -237,7 +248,7 @@ private fun LazyListScope.artistResults(
) { ) {
if (artists.isEmpty()) return if (artists.isEmpty()) return
resultHeader("Artists") resultHeader("Artists")
items(artists, key = { it.id }) { artist -> items(artists, key = { "artist-${it.id}" }) { artist ->
ArtistListRow( ArtistListRow(
artist = artist, artist = artist,
coverArt = coverArt, coverArt = coverArt,
@@ -256,7 +267,7 @@ private fun LazyListScope.playlistResults(
) { ) {
if (playlists.isEmpty()) return if (playlists.isEmpty()) return
resultHeader("Playlists") resultHeader("Playlists")
items(playlists, key = { it.id }) { playlist -> items(playlists, key = { "playlist-${it.id}" }) { playlist ->
PlaylistRow( PlaylistRow(
playlist = playlist, playlist = playlist,
coverArt = coverArt, coverArt = coverArt,
@@ -2,6 +2,7 @@ package com.subtracks.ui.search
import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope import androidx.lifecycle.viewModelScope
import com.subtracks.data.db.SEARCH_MIN_LENGTH
import com.subtracks.data.model.Album import com.subtracks.data.model.Album
import com.subtracks.data.model.AlbumSongItem import com.subtracks.data.model.AlbumSongItem
import com.subtracks.data.model.Artist 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.Playlist
import com.subtracks.data.model.Song import com.subtracks.data.model.Song
import com.subtracks.data.repo.LibraryRepository import com.subtracks.data.repo.LibraryRepository
import com.subtracks.data.repo.SEARCH_MIN_LENGTH
import com.subtracks.data.repo.SourceRepository import com.subtracks.data.repo.SourceRepository
import com.subtracks.playback.PlaybackController import com.subtracks.playback.PlaybackController
import com.subtracks.ui.home.playSongInContext import com.subtracks.ui.home.playSongInContext
@@ -21,6 +21,7 @@ import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.onStart
import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.stateIn
data class SearchResults( data class SearchResults(
@@ -28,11 +29,14 @@ data class SearchResults(
val artists: List<Artist> = emptyList(), val artists: List<Artist> = emptyList(),
val playlists: List<Playlist> = emptyList(), val playlists: List<Playlist> = emptyList(),
val songs: List<AlbumSongItem> = emptyList(), val songs: List<AlbumSongItem> = emptyList(),
val loading: Boolean = false,
) { ) {
val isEmpty: Boolean val isEmpty: Boolean
get() = albums.isEmpty() && artists.isEmpty() && playlists.isEmpty() && songs.isEmpty() 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) @OptIn(ExperimentalCoroutinesApi::class)
class SearchViewModel( class SearchViewModel(
private val libraryRepository: LibraryRepository, private val libraryRepository: LibraryRepository,
@@ -46,7 +50,7 @@ class SearchViewModel(
libraryRepository.activeSourceId libraryRepository.activeSourceId
.flatMapLatest { sourceId -> .flatMapLatest { sourceId ->
_query.flatMapLatest { text -> _query.flatMapLatest { text ->
if (sourceId == null || text.length < SEARCH_MIN_LENGTH) { if (sourceId == null || !searchReady(text)) {
flowOf(SearchResults()) flowOf(SearchResults())
} else { } else {
combine( combine(
@@ -56,7 +60,7 @@ class SearchViewModel(
libraryRepository.searchSongs(sourceId, text), libraryRepository.searchSongs(sourceId, text),
) { albums, artists, playlists, songs -> ) { albums, artists, playlists, songs ->
SearchResults(albums, artists, playlists, songs) SearchResults(albums, artists, playlists, songs)
} }.onStart { emit(SearchResults(loading = true)) }
} }
} }
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), SearchResults()) }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), SearchResults())
@@ -762,20 +762,6 @@ class LibraryDaoTest {
assertEquals(2, dao.searchAlbums(sourceId, "oad", 2).first().size) 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 @Test
fun crossTypeSearchIgnoresQueriesShorterThanThreeCharacters() = fun crossTypeSearchIgnoresQueriesShorterThanThreeCharacters() =
runTest { runTest {
@@ -83,6 +83,30 @@ class SearchScreenTest {
composeRule.onNodeWithText("No results", substring = true).assertIsDisplayed() 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( private fun album(
id: String, id: String,
name: String, name: String,
@@ -1,6 +1,7 @@
package com.subtracks.ui.search package com.subtracks.ui.search
import android.content.Context import android.content.Context
import androidx.lifecycle.viewModelScope
import androidx.room3.Room import androidx.room3.Room
import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.sqlite.driver.bundled.BundledSQLiteDriver
import androidx.test.core.app.ApplicationProvider import androidx.test.core.app.ApplicationProvider
@@ -27,8 +28,10 @@ import com.subtracks.playback.PlaybackController
import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.resetMain
@@ -37,6 +40,7 @@ import kotlinx.coroutines.withTimeout
import okhttp3.OkHttpClient import okhttp3.OkHttpClient
import org.junit.After import org.junit.After
import org.junit.Assert.assertEquals import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue import org.junit.Assert.assertTrue
import org.junit.Before import org.junit.Before
import org.junit.Test import org.junit.Test
@@ -94,6 +98,7 @@ class SearchViewModelTest {
@After @After
fun tearDown() { fun tearDown() {
runBlocking { viewModel.viewModelScope.coroutineContext[Job]?.cancelAndJoin() }
cancelAndJoinBlocking(controllerScope, repoScope) cancelAndJoinBlocking(controllerScope, repoScope)
db.close() db.close()
Dispatchers.resetMain() Dispatchers.resetMain()
@@ -129,6 +134,13 @@ class SearchViewModelTest {
assertTrue(blank.isEmpty) assertTrue(blank.isEmpty)
} }
@Test
fun searchReadyCountsCodePointsRatherThanUtf16Units() {
assertTrue(searchReady("road"))
assertFalse(searchReady("ro"))
assertFalse(searchReady("a\uD83D\uDE00"))
}
@Test @Test
fun playingASearchResultPlaysItsAlbumInsteadOfASearchQueue() { fun playingASearchResultPlaysItsAlbumInsteadOfASearchQueue() {
val sourceId = val sourceId =