From b19c6217436ff123917457269f320924276fbe52 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:18:59 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20drift=20audit=20batch=203a=20=E2=80=94?= =?UTF-8?q?=20Android=20offline=20correctness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the 2026-06-02 drift audit (Scribe parent #552): - **#577 (Android)** RequestsViewModel.cancel() called refresh() on BOTH Synced and Queued outcomes. Synced is fine (re-fetch the canonical list); Queued is offline by definition — the optimistic removal at the top of cancel() is already correct, and refresh() on Queued either (a) gets the not-yet-delivered cancelled row back from the server and snaps it into the list (confusing), or (b) fails with a transport error and flips the screen to UiState.Error so the user thinks the cancel failed even though it's queued. Gate refresh() on outcome == Synced; the mutation replayer reconciles when connectivity returns. - **#570 (Android)** LikesRepository.refreshIds() pulled the server's likes list and INSERTed it into cached_likes — but never DELETEd local rows the server no longer surfaces. A cross-device unlike (user likes on web, then unlikes on web) left the entry visible on Android's Liked tab indefinitely with no way to clear short of wiping app data. Add CachedLikeDao.clearForUser + a @Transaction replaceAllForUser that atomically wipes-then-inserts the user's set; refreshIds() uses replaceAllForUser so the local cache is exactly what the server reports. The LOCAL_USER_ID hardcode is its own drift (#576) and stays for now — fixing it needs threading AuthStore.userId through the repo. --- .../minstrel/cache/db/dao/CachedLikeDao.kt | 18 +++++++++++++++ .../minstrel/likes/data/LikesRepository.kt | 19 ++++++++++----- .../minstrel/requests/ui/RequestsViewModel.kt | 23 +++++++++++++------ 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt index 21b3d56c..a958ba15 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt @@ -4,6 +4,7 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import com.fabledsword.minstrel.cache.db.entities.CachedLikeEntity import kotlinx.coroutines.flow.Flow @@ -36,4 +37,21 @@ interface CachedLikeDao { "WHERE userId = :userId AND entityType = :entityType AND entityId = :entityId", ) suspend fun delete(userId: String, entityType: String, entityId: String) + + @Query("DELETE FROM cached_likes WHERE userId = :userId") + suspend fun clearForUser(userId: String) + + /** + * Atomically replaces the user's entire cached_likes set with + * [rows]. Used by [com.fabledsword.minstrel.likes.data.LikesRepository.refreshIds] + * so cross-device unlikes (a row that the server no longer + * surfaces) get removed from the local cache — drift #570 + * caught the missing delete pass that left stale Liked tab + * entries pointing at tracks the user had unliked elsewhere. + */ + @Transaction + suspend fun replaceAllForUser(userId: String, rows: List) { + clearForUser(userId) + upsertAll(rows) + } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt index 1e5087ed..0151a506 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt @@ -111,11 +111,18 @@ class LikesRepository @Inject constructor( } /** - * Pulls `GET /api/likes/ids` and reconciles `cached_likes`. Called - * by the LikedTab ViewModel on init (and by the SyncController in - * Phase 12). Adds rows for IDs the server has but we don't; the - * delete-of-stale-rows pass lives in the SyncController where the - * full reconciliation runs. + * Pulls `GET /api/likes/ids` and atomically replaces the user's + * cached_likes set with the server's canonical view. Called by + * the LikedTab ViewModel on init (and by the SyncController in + * Phase 12). + * + * Drift #570 fix: previously this called `upsertAll` only, which + * added rows for IDs the server had but never removed rows the + * server no longer surfaced — so a cross-device unlike (user + * likes on web, then unlikes on web) left the Liked tab on + * Android showing the now-unliked entry forever. The atomic + * replaceAllForUser DAO method runs a transactional delete-then- + * insert so the local set is exactly what the server reports. */ suspend fun refreshIds() { val wire = api.ids() @@ -123,7 +130,7 @@ class LikesRepository @Inject constructor( rows += wire.artistIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_ARTIST, it) } rows += wire.albumIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_ALBUM, it) } rows += wire.trackIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_TRACK, it) } - likeDao.upsertAll(rows) + likeDao.replaceAllForUser(LOCAL_USER_ID, rows) } companion object { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt index 1b938506..7759a78e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt @@ -5,6 +5,7 @@ import androidx.lifecycle.viewModelScope import com.fabledsword.minstrel.api.ErrorCopy import com.fabledsword.minstrel.events.EventsStream import com.fabledsword.minstrel.models.RequestRef +import com.fabledsword.minstrel.requests.data.CancelOutcome import com.fabledsword.minstrel.requests.data.RequestsRepository import com.fabledsword.minstrel.shared.UiState import dagger.hilt.android.lifecycle.HiltViewModel @@ -71,7 +72,7 @@ class RequestsViewModel @Inject constructor( } viewModelScope.launch { try { - val (_, updated) = repository.cancel(id) + val (outcome, updated) = repository.cancel(id) if (updated != null) { internal.update { state -> if (state !is UiState.Success) { @@ -87,12 +88,20 @@ class RequestsViewModel @Inject constructor( } } } - // Refresh fetches the canonical list whether the cancel - // went through live (Synced) or got enqueued (Queued). - // The Queued case shows the optimistic removal until the - // replayer drains; the next list fetch surfaces the - // server's canonical view. - refresh() + // Drift #577: only refetch on the Synced outcome. The + // Queued path is offline by definition — the optimistic + // removal at the top of cancel() is correct, and a + // refresh() call here would either (a) get the + // not-yet-delivered cancelled row back from the server + // and snap it into the list (confusing), or (b) fail + // with a transport error and flip the whole screen to + // UiState.Error, making the user think the cancel + // failed. The mutation queue will replay the cancel + // when connectivity returns; the next on-screen refresh + // (pull-to-refresh, navigation back) reconciles. + if (outcome == CancelOutcome.Synced) { + refresh() + } } catch ( @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, ) {