Improve ergonomics of api calls

persistence/src/commonMain/kotlin/com/quran/shared/persistence/repository/bookmark/repository/BookmarksRepositoryImpl.kt

@@ -161,17 +161,19 @@ class BookmarksRepositoryImpl(
 
     override suspend fun deleteBookmark(sura: Int, ayah: Int): Boolean {
         logger.i { "Deleting ayah bookmark for $sura:$ayah" }
-        withContext(Dispatchers.IO) {
+        return withContext(Dispatchers.IO) {
+            var deleted = false
             database.transaction {
                 val bookmark = bookmarkQueries.value
                     .getBookmarkForAyah(sura.toLong(), ayah.toLong())
                     .executeAsOneOrNull()
-                if (bookmark != null) {
+                if (bookmark?.hasSavedBookmarkMembership() == true) {
                     deleteSavedBookmarkByLocalIdInTransaction(bookmark.local_id, null)
+                    deleted = true
                 }
             }
+            deleted
         }
-        return true
     }
 
     override suspend fun deleteBookmark(bookmark: AyahBookmark): Boolean {
@@ -185,14 +187,25 @@ class BookmarksRepositoryImpl(
     }
 
     private suspend fun deleteBookmarkWithLocalId(localId: String): Boolean {
-        withContext(Dispatchers.IO) {
+        return withContext(Dispatchers.IO) {
+            var deleted = false
             database.transaction {
-                deleteSavedBookmarkByLocalIdInTransaction(localId.toLong(), null)
+                val id = localId.toLong()
+                val bookmark = bookmarkQueries.value.getBookmarkByLocalId(id).executeAsOneOrNull()
+                if (bookmark?.hasSavedBookmarkMembership() == true) {
+                    deleteSavedBookmarkByLocalIdInTransaction(id, null)
+                    deleted = true
+                }
             }
+            deleted
         }
-        return true
     }
 
+    private fun DatabaseBookmark.hasSavedBookmarkMembership(): Boolean =
+        deleted == 0L &&
+            (is_in_default_collection == 1L ||
+                bookmarkCollectionQueries.value.countActiveForBookmark(local_id).executeAsOne() > 0)
+
     private fun deleteSavedBookmarkByLocalIdInTransaction(localId: Long, timestampMillis: Long?) {
         bookmarkQueries.value.clearDefaultCollection(
             local_id = localId,

persistence/src/commonMain/kotlin/com/quran/shared/persistence/repository/collection/repository/CollectionsRepositoryImpl.kt

@@ -123,15 +123,22 @@ class CollectionsRepositoryImpl(
 
     override suspend fun deleteCollection(localId: String): Boolean {
         logger.i { "Deleting collection localId=$localId" }
-        withContext(Dispatchers.IO) {
+        return withContext(Dispatchers.IO) {
+            var deleted = false
             database.transaction {
+                val id = localId.toLong()
+                val collection = collectionQueries.value.getCollectionByLocalId(id).executeAsOneOrNull()
+                if (collection?.deleted != 0L) {
+                    return@transaction
+                }
                 collectionQueries.value.deleteCollection(
-                    id = localId.toLong()
+                    id = id
                 )
                 reconciler.reconcile()
+                deleted = true
             }
+            deleted
         }
-        return true
     }
 
     override suspend fun fetchMutatedCollections(): List<LocalModelMutation<Collection>> {

persistence/src/commonTest/kotlin/com/quran/shared/persistence/repository/BookmarkSyncArchitectureTest.kt

@@ -29,6 +29,7 @@ import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
@@ -61,6 +62,60 @@ class BookmarkSyncArchitectureTest {
         assertEquals(0L, database.bookmark_collectionsQueries.countAll().executeAsOne())
     }
 
+    @Test
+    fun `deleteBookmark by ayah returns false for missing bookmark without mutation`() = runTest {
+        val deleted = bookmarksRepository.deleteBookmark(2, 255)
+
+        assertFalse(deleted)
+        assertEquals(emptyList(), bookmarksRepository.fetchMutatedBookmarks())
+    }
+
+    @Test
+    fun `deleteBookmark by local id returns false for missing bookmark without mutation`() = runTest {
+        val deleted = bookmarksRepository.deleteBookmark("999")
+
+        assertFalse(deleted)
+        assertEquals(emptyList(), bookmarksRepository.fetchMutatedBookmarks())
+    }
+
+    @Test
+    fun `deleteBookmark by ayah returns false for reading-only bookmark without saved mutation`() = runTest {
+        readingRepository.addAyahReadingBookmark(2, 255, at(100))
+        val before = database.bookmarksQueries.getBookmarkForAyah(2L, 255L).executeAsOne()
+        val mutationsBefore = bookmarksRepository.fetchMutatedBookmarks()
+
+        val deleted = bookmarksRepository.deleteBookmark(2, 255)
+
+        val after = database.bookmarksQueries.getBookmarkForAyah(2L, 255L).executeAsOne()
+        val mutationsAfter = bookmarksRepository.fetchMutatedBookmarks()
+        assertFalse(deleted)
+        assertEquals(before, after)
+        assertEquals(mutationsBefore.size, mutationsAfter.size)
+        assertEquals(mutationsBefore.single().localID, mutationsAfter.single().localID)
+        assertEquals(mutationsBefore.single().mutation, mutationsAfter.single().mutation)
+        assertEquals(mutationsBefore.single().ack, mutationsAfter.single().ack)
+        assertEquals(0L, database.bookmark_collectionsQueries.countAll().executeAsOne())
+    }
+
+    @Test
+    fun `deleteBookmark by local id returns false for reading-only bookmark without saved mutation`() = runTest {
+        val readingBookmark = readingRepository.addAyahReadingBookmark(2, 255, at(100))
+        val before = database.bookmarksQueries.getBookmarkByLocalId(readingBookmark.localId.toLong()).executeAsOne()
+        val mutationsBefore = bookmarksRepository.fetchMutatedBookmarks()
+
+        val deleted = bookmarksRepository.deleteBookmark(readingBookmark.localId)
+
+        val after = database.bookmarksQueries.getBookmarkByLocalId(readingBookmark.localId.toLong()).executeAsOne()
+        val mutationsAfter = bookmarksRepository.fetchMutatedBookmarks()
+        assertFalse(deleted)
+        assertEquals(before, after)
+        assertEquals(mutationsBefore.size, mutationsAfter.size)
+        assertEquals(mutationsBefore.single().localID, mutationsAfter.single().localID)
+        assertEquals(mutationsBefore.single().mutation, mutationsAfter.single().mutation)
+        assertEquals(mutationsBefore.single().ack, mutationsAfter.single().ack)
+        assertEquals(0L, database.bookmark_collectionsQueries.countAll().executeAsOne())
+    }
+
     @Test
     fun `applyRemoteChanges checks write boundary before bookmark transaction`() = runTest {
         assertFailsWith<IllegalStateException> {

persistence/src/commonTest/kotlin/com/quran/shared/persistence/repository/CollectionsRepositoryTest.kt

@@ -18,6 +18,7 @@ import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
+import kotlin.test.assertFalse
 import kotlin.test.assertNull
 import kotlin.time.Instant
 
@@ -124,6 +125,41 @@ class CollectionsRepositoryTest {
         assertEquals(1000L, record.modified_at)
     }
 
+    @Test
+    fun `deleteCollection returns false for missing collection without mutation`() = runTest {
+        val deleted = repository.deleteCollection("999")
+
+        assertFalse(deleted)
+        assertEquals(emptyList(), repository.fetchMutatedCollections())
+    }
+
+    @Test
+    fun `deleteCollection returns false for retained deleted collection without advancing mutation`() = runTest {
+        database.collectionsQueries.persistRemoteCollection(
+            remote_id = "remote-collection-id",
+            name = "Favorites",
+            created_at = 1000L,
+            modified_at = 1000L
+        )
+        val collection = database.collectionsQueries.getCollectionByRemoteId("remote-collection-id").executeAsOne()
+        assertEquals(true, repository.deleteCollection(collection.local_id.toString()))
+        val firstTombstone = database.collectionsQueries.getCollectionByRemoteId("remote-collection-id").executeAsOne()
+        val firstMutation = repository.fetchMutatedCollections().single()
+
+        val deletedAgain = repository.deleteCollection(collection.local_id.toString())
+
+        val secondTombstone = database.collectionsQueries.getCollectionByRemoteId("remote-collection-id").executeAsOne()
+        assertFalse(deletedAgain)
+        assertEquals(firstTombstone.pending_version, secondTombstone.pending_version)
+        assertEquals(firstTombstone.modified_at, secondTombstone.modified_at)
+        val secondMutation = repository.fetchMutatedCollections().single()
+        assertEquals(firstMutation.model, secondMutation.model)
+        assertEquals(firstMutation.remoteID, secondMutation.remoteID)
+        assertEquals(firstMutation.localID, secondMutation.localID)
+        assertEquals(firstMutation.mutation, secondMutation.mutation)
+        assertEquals(firstMutation.ack, secondMutation.ack)
+    }
+
     @Test
     fun `applyRemoteChanges clears collection ACK when pending version still matches`() = runTest {
         database.collectionsQueries.persistRemoteCollection(

sync-pipelines/src/commonMain/kotlin/com/quran/shared/pipeline/QuranDataService.kt

@@ -497,22 +497,49 @@ class QuranDataService internal constructor(
     }
 
     @NativeCoroutines
-    suspend fun deleteBookmark(bookmark: AyahBookmark) {
-        mutatingCall("Failed to delete bookmark") {
+    suspend fun deleteBookmark(bookmark: AyahBookmark): Boolean {
+        return deleteBookmarkResult("Failed to delete bookmark") {
             bookmarksRepository.deleteBookmark(bookmark)
         }
     }
 
     @NativeCoroutines
-    suspend fun addCollection(name: String) {
-        mutatingCall("Failed to add collection") {
+    suspend fun deleteBookmark(localId: String): Boolean {
+        return deleteBookmarkResult("Failed to delete bookmark") {
+            bookmarksRepository.deleteBookmark(localId)
+        }
+    }
+
+    @NativeCoroutines
+    suspend fun deleteBookmark(sura: Int, ayah: Int): Boolean {
+        return deleteBookmarkResult("Failed to delete bookmark") {
+            bookmarksRepository.deleteBookmark(sura, ayah)
+        }
+    }
+
+    private suspend fun deleteBookmarkResult(
+        errorMessage: String,
+        block: suspend () -> Boolean
+    ): Boolean {
+        return mutatingCall(errorMessage, triggerAfter = false) {
+            val deleted = block()
+            if (deleted) {
+                triggerSync()
+            }
+            deleted
+        }
+    }
+
+    @NativeCoroutines
+    suspend fun addCollection(name: String): Collection {
+        return mutatingCall("Failed to add collection") {
             collectionsRepository.addCollection(name)
         }
     }
 
     @NativeCoroutines
-    suspend fun addCollection(name: String, timestamp: PlatformDateTime) {
-        mutatingCall("Failed to add collection") {
+    suspend fun addCollection(name: String, timestamp: PlatformDateTime): Collection {
+        return mutatingCall("Failed to add collection") {
             collectionsRepository.addCollection(name, timestamp)
         }
     }
@@ -549,9 +576,13 @@ class QuranDataService internal constructor(
     }
 
     @NativeCoroutines
-    suspend fun deleteCollection(localId: String) {
-        mutatingCall("Failed to delete collection") {
-            collectionsRepository.deleteCollection(localId)
+    suspend fun deleteCollection(localId: String): Boolean {
+        return mutatingCall("Failed to delete collection", triggerAfter = false) {
+            val deleted = collectionsRepository.deleteCollection(localId)
+            if (deleted) {
+                triggerSync()
+            }
+            deleted
         }
     }