fix(playlists): make the playlist-track replace atomic
android / Build + lint + test (push) Successful in 4m4s
android / Build + lint + test (push) Successful in 4m4s
`refreshDetail` did an un-transacted `deleteByPlaylist` + `upsertAll` — the same shape as the Home index write that #2327 just fixed. Room's InvalidationTracker fires after the DELETE, so an observer of `observeByPlaylist` would see `emptyList()` before the new rows land, which is exactly what made every Home row visibly collapse to empty and refill. Nothing consumes `observeByPlaylist` today, so this is not a live defect — it's a landmine. Making playlist detail cache-first later would have silently reintroduced the flicker, and the reason would have been three layers away from the symptom. One `@Transaction` now costs nothing and removes that. `deleteByPlaylist` is left in place as the building block but is no longer called from outside the DAO. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+25
-1
@@ -4,6 +4,7 @@ import androidx.room.Dao
|
|||||||
import androidx.room.Insert
|
import androidx.room.Insert
|
||||||
import androidx.room.OnConflictStrategy
|
import androidx.room.OnConflictStrategy
|
||||||
import androidx.room.Query
|
import androidx.room.Query
|
||||||
|
import androidx.room.Transaction
|
||||||
import com.fabledsword.minstrel.cache.db.entities.CachedPlaylistTrackEntity
|
import com.fabledsword.minstrel.cache.db.entities.CachedPlaylistTrackEntity
|
||||||
import kotlinx.coroutines.flow.Flow
|
import kotlinx.coroutines.flow.Flow
|
||||||
|
|
||||||
@@ -35,10 +36,33 @@ interface CachedPlaylistTrackDao {
|
|||||||
@Query("SELECT MAX(position) FROM cached_playlist_tracks WHERE playlistId = :playlistId")
|
@Query("SELECT MAX(position) FROM cached_playlist_tracks WHERE playlistId = :playlistId")
|
||||||
suspend fun maxPosition(playlistId: String): Int?
|
suspend fun maxPosition(playlistId: String): Int?
|
||||||
|
|
||||||
/** Replace-all pattern for a playlist; called after a sync delta lands. */
|
|
||||||
@Query("DELETE FROM cached_playlist_tracks WHERE playlistId = :playlistId")
|
@Query("DELETE FROM cached_playlist_tracks WHERE playlistId = :playlistId")
|
||||||
suspend fun deleteByPlaylist(playlistId: String)
|
suspend fun deleteByPlaylist(playlistId: String)
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Replaces a playlist's whole membership in ONE transaction; called
|
||||||
|
* after a refresh or a sync delta lands.
|
||||||
|
*
|
||||||
|
* Atomic on purpose. Room's InvalidationTracker only notifies observers
|
||||||
|
* after the transaction commits, so [observeByPlaylist] never sees the
|
||||||
|
* empty gap between the delete and the re-insert. Un-transacted, that
|
||||||
|
* gap is a real observed state — it's what made every Home row visibly
|
||||||
|
* collapse to empty and refill before issue #2327 fixed the equivalent
|
||||||
|
* write in `CachedHomeIndexDao`.
|
||||||
|
*
|
||||||
|
* Nothing observes [observeByPlaylist] live today, so this is
|
||||||
|
* pre-emptive: it means making playlist detail cache-first later can't
|
||||||
|
* silently reintroduce that flicker.
|
||||||
|
*/
|
||||||
|
@Transaction
|
||||||
|
suspend fun replacePlaylistTracks(
|
||||||
|
playlistId: String,
|
||||||
|
rows: List<CachedPlaylistTrackEntity>,
|
||||||
|
) {
|
||||||
|
deleteByPlaylist(playlistId)
|
||||||
|
if (rows.isNotEmpty()) upsertAll(rows)
|
||||||
|
}
|
||||||
|
|
||||||
@Query(
|
@Query(
|
||||||
"DELETE FROM cached_playlist_tracks " +
|
"DELETE FROM cached_playlist_tracks " +
|
||||||
"WHERE playlistId = :playlistId AND trackId IN (:trackIds)",
|
"WHERE playlistId = :playlistId AND trackId IN (:trackIds)",
|
||||||
|
|||||||
+3
-3
@@ -119,9 +119,9 @@ class PlaylistsRepository @Inject constructor(
|
|||||||
throw e
|
throw e
|
||||||
}
|
}
|
||||||
playlistDao.upsertAll(listOf(wire.toPlaylistEntity()))
|
playlistDao.upsertAll(listOf(wire.toPlaylistEntity()))
|
||||||
playlistTrackDao.deleteByPlaylist(id)
|
playlistTrackDao.replacePlaylistTracks(
|
||||||
playlistTrackDao.upsertAll(
|
playlistId = id,
|
||||||
wire.tracks.mapNotNull { row ->
|
rows = wire.tracks.mapNotNull { row ->
|
||||||
row.trackId?.let { trackId ->
|
row.trackId?.let { trackId ->
|
||||||
CachedPlaylistTrackEntity(
|
CachedPlaylistTrackEntity(
|
||||||
playlistId = id,
|
playlistId = id,
|
||||||
|
|||||||
Reference in New Issue
Block a user