Fix a queue bug that deleted episodes, and delete dead DAO methods

Review found a data-losing bug. advance() drops the head of the queue, but
play() never put the played episode there. Play something from a show list,
let it finish, and advance dropped whatever was actually at the head -- a
different episode, removed from the queue having never been played.

The fix is an invariant rather than a special case: anything played becomes
the head, so advance always drops the episode that just finished. playNow()
does that, play() calls it, and the tests name the scenario.

Two DAO methods had no caller -- getQueue, superseded by observeQueue, and
observeUnplayed, written for a screen that was never built. Deleted. Room
generates an implementation for each, so they were not free.

The reachability guard now covers DAO methods too, which is the shape that
bit FeeDreader: three superseded methods sat undetected until a manual audit.
Interfaces need looser matching than objects, since a call's receiver varies
by site and a @Transaction default method calls its siblings unqualified, so
it matches both forms and errs toward "called" -- a false alarm costs more
attention than a missed one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B

8 files changed, 94 insertions, 14 deletions

diff --git a/app/src/main/java/org/terst/listen/data/Daos.kt b/app/src/main/java/org/terst/listen/data/Daos.kt
index 2376644..deb350a 100644
--- a/app/src/main/java/org/terst/listen/data/Daos.kt
+++ b/app/src/main/java/org/terst/listen/data/Daos.kt
@@ -84,9 +84,6 @@ interface EpisodeDao {
         before: Long,
         downloaded: DownloadState = DownloadState.DOWNLOADED,
     ): List<EpisodeEntity>
-
-    @Query("SELECT * FROM episodes WHERE playedAtMs IS NULL ORDER BY publishedAtMs DESC LIMIT :limit")
-    fun observeUnplayed(limit: Int): Flow<List<EpisodeEntity>>
 }
 
 @Dao
@@ -98,13 +95,6 @@ interface QueueDao {
     )
     fun observeQueue(): Flow<List<EpisodeEntity>>
 
-    @Query(
-        """SELECT e.* FROM episodes e
-           INNER JOIN queue q ON q.episodeId = e.id
-           ORDER BY q.position"""
-    )
-    suspend fun getQueue(): List<EpisodeEntity>
-
     @Query("SELECT * FROM queue ORDER BY position")
     suspend fun getEntries(): List<QueueEntity>
 
diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt
index fb869d8..60d1590 100644
--- a/app/src/main/java/org/terst/listen/data/Repositories.kt
+++ b/app/src/main/java/org/terst/listen/data/Repositories.kt
@@ -76,6 +76,9 @@ class QueueRepository(private val dao: QueueDao) {
 
     suspend fun playNext(episodeId: String) = rewrite { QueueOrdering.playNext(it, episodeId) }
 
+    /** Promotes an episode to the head, where "what is playing" lives. */
+    suspend fun playNow(episodeId: String) = rewrite { QueueOrdering.playNow(it, episodeId) }
+
     suspend fun remove(episodeId: String) = rewrite { QueueOrdering.remove(it, episodeId) }
 
     suspend fun move(from: Int, to: Int) = rewrite { QueueOrdering.move(it, from, to) }
diff --git a/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt b/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt
index 4cc6543..d657477 100644
--- a/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt
+++ b/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt
@@ -26,6 +26,15 @@ object QueueOrdering {
         return listOf(rest.first(), episodeId) + rest.drop(1)
     }
 
+    /**
+     * Makes an episode the head, which is what is playing.
+     *
+     * The invariant this exists to hold: [advance] drops the head, so if the playing episode
+     * isn't there, finishing it deletes a different episode from the queue unplayed.
+     */
+    fun playNow(queue: List<String>, episodeId: String): List<String> =
+        listOf(episodeId) + queue.filterNot { it == episodeId }
+
     fun remove(queue: List<String>, episodeId: String): List<String> =
         queue.filterNot { it == episodeId }
 
diff --git a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt
index fa501a4..aef40ac 100644
--- a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt
+++ b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt
@@ -58,6 +58,11 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) {
     /** Starts an episode, resuming where the listener left off and at the show's speed. */
     fun play(episode: EpisodeEntity) {
         viewModelScope.launch {
+            // Anything played becomes the head of the queue. Without that, finishing an
+            // episode started from a show list would advance past -- and silently discard --
+            // whatever was actually queued.
+            graph.queue.playNow(episode.id)
+
             val show = graph.subscriptions.getById(episode.subscriptionId)
             val speed = show?.playbackSpeed ?: DEFAULT_SPEED
             connection.play(
diff --git a/app/src/test/java/org/terst/listen/ReachabilityTest.kt b/app/src/test/java/org/terst/listen/ReachabilityTest.kt
index 4eaed9e..d129756 100644
--- a/app/src/test/java/org/terst/listen/ReachabilityTest.kt
+++ b/app/src/test/java/org/terst/listen/ReachabilityTest.kt
@@ -70,6 +70,35 @@ class ReachabilityTest {
             .toList()
     }
 
+    /**
+     * Room DAOs are the other place dead code hides -- FeeDreader accumulated three superseded
+     * methods that an audit, not a test, eventually found. Matching is on `.name(` across all
+     * of main with declarations stripped, because a DAO call's receiver varies by site and a
+     * @Transaction default method legitimately calls its siblings in the same file.
+     */
+    @Test
+    fun `every dao method has a caller`() {
+        val sources = mainDir.walkTopDown().filter { it.extension == "kt" }.toList()
+        val declaration = Regex("""(?:suspend )?fun (\w+)\(""")
+        val corpus = sources.joinToString("\n") { declaration.replace(it.readText(), "DECL(") }
+
+        val daoFile = File(mainDir, "data/Daos.kt")
+        assertTrue("expected ${daoFile.path}", daoFile.isFile)
+
+        val dead = Regex("""^    (?:suspend )?fun (\w+)""", RegexOption.MULTILINE)
+            .findAll(daoFile.readText())
+            .map { it.groupValues[1] }
+            .distinct()
+            // Both a qualified call and a bare one: a @Transaction default method calls its
+            // siblings unqualified. Heuristic on purpose -- it errs toward saying "called",
+            // since a false alarm here costs more attention than a missed one.
+            .filterNot { corpus.contains(".$it(") || Regex("""\b$it\(""").containsMatchIn(corpus) }
+            .toList()
+
+        assertTrue("No caller for DAO method(s): ${dead.sorted()}. Wire them up or delete them.",
+            dead.isEmpty())
+    }
+
     @Test
     fun `every helper object function has a caller`() {
         val sources = mainDir.walkTopDown().filter { it.extension == "kt" }.toList()
diff --git a/app/src/test/java/org/terst/listen/data/FakeDaos.kt b/app/src/test/java/org/terst/listen/data/FakeDaos.kt
index 821e1b2..f313687 100644
--- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt
+++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt
@@ -103,9 +103,6 @@ class FakeEpisodeDao : EpisodeDao {
             it.downloadState == downloaded && it.playedAtMs != null && it.playedAtMs!! < before
         }
 
-    override fun observeUnplayed(limit: Int): Flow<List<EpisodeEntity>> =
-        flowOf(rows.filter { it.playedAtMs == null }.take(limit))
-
     private fun replace(id: String, transform: (EpisodeEntity) -> EpisodeEntity) {
         val index = rows.indexOfFirst { it.id == id }
         if (index >= 0) {
@@ -125,7 +122,6 @@ class FakeQueueDao : QueueDao {
     }
 
     override fun observeQueue(): Flow<List<EpisodeEntity>> = flowOf(ordered())
-    override suspend fun getQueue() = ordered()
     override suspend fun getEntries() = entries.sortedBy { it.position }
 
     override suspend fun upsertAll(entries: List<QueueEntity>) {
diff --git a/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt b/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt
index a3ed294..dde5bdb 100644
--- a/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt
+++ b/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt
@@ -81,6 +81,29 @@ class QueueRepositoryTest {
         assertContiguous()
     }
 
+    /**
+     * The bug this prevents: play an episode that isn't in the queue, let it finish, and the
+     * old advance dropped whatever was at the head -- a different episode, removed unplayed.
+     */
+    @Test
+    fun `playing an unqueued episode does not consume a queued one`() = runTest {
+        listOf("a", "b").forEach { repo.append(it) }
+        repo.playNow("d")
+        assertEquals(listOf("d", "a", "b"), ids())
+
+        assertEquals("a", repo.advance())
+        assertEquals("nothing may be dropped except the episode that finished", listOf("a", "b"), ids())
+        assertContiguous()
+    }
+
+    @Test
+    fun `play now promotes to the head`() = runTest {
+        listOf("a", "b", "c").forEach { repo.append(it) }
+        repo.playNow("c")
+        assertEquals(listOf("c", "a", "b"), ids())
+        assertContiguous()
+    }
+
     @Test
     fun `advance drops the finished episode and reports the next`() = runTest {
         listOf("a", "b", "c").forEach { repo.append(it) }
diff --git a/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt b/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt
index 556351f..93ee0a0 100644
--- a/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt
+++ b/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt
@@ -69,6 +69,31 @@ class QueueOrderingTest {
         assertEquals(queue, QueueOrdering.move(queue, from = 99, to = 0))
     }
 
+    /**
+     * Playing an episode makes it the head, which is what keeps advance honest: advance drops
+     * the head, so unless the playing episode is there, finishing it silently deletes somebody
+     * else's place in the queue.
+     */
+    @Test
+    fun `play now puts the episode at the head`() {
+        assertEquals(listOf("d", "a", "b", "c"), QueueOrdering.playNow(queue, "d"))
+    }
+
+    @Test
+    fun `play now on a queued episode moves it to the head without duplicating`() {
+        assertEquals(listOf("c", "a", "b"), QueueOrdering.playNow(queue, "c"))
+    }
+
+    @Test
+    fun `play now on the current head changes nothing`() {
+        assertEquals(listOf("a", "b", "c"), QueueOrdering.playNow(queue, "a"))
+    }
+
+    @Test
+    fun `play now on an empty queue starts it`() {
+        assertEquals(listOf("d"), QueueOrdering.playNow(emptyList(), "d"))
+    }
+
     @Test
     fun `advancing drops the finished episode and returns the next`() {
         val (next, remaining) = QueueOrdering.advance(queue)