From df9b7de43d9bc55a3496eed2d1ea0491bdf8ca5f Mon Sep 17 00:00:00 2001 From: Taksh Date: Thu, 13 Aug 2026 06:36:32 +0530 Subject: [PATCH 1/3] Drop bookmark name resolution that lands after the bookmark is gone resolveNameIfNeeded launches a reverse geocode on Dispatchers.IO and never cancels it. If the bookmark is removed, or wiped by clearAll, while that lookup is in flight, the coroutine still writes the resolved place name into _bookmarkNames and persists it to SharedPreferences. clearAll is the panic path (ChatViewModel clears bookmarks so "panic should remove everything"), and its "clear any in-flight resolutions to avoid repopulating" comment does not hold: emptying the resolving set does not stop the coroutine that is already running. The result is a geocoded location name left on disk after a panic wipe, visible again on the next launch. Commit the resolved name through commitResolvedName, which drops it unless the geohash is still bookmarked. membership is now read from the IO dispatcher, so guard the accessors that touch it and move the resolving bookkeeping behind the same lock. --- .../android/geohash/GeohashBookmarksStore.kt | 55 ++++++++++++++++--- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt index 92e237cb..4cc89492 100644 --- a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt +++ b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt @@ -20,7 +20,10 @@ import java.util.Locale * - Persistence: SharedPreferences (JSON string array) * - Semantics: geohashes are normalized to lowercase base32 and de-duplicated */ -class GeohashBookmarksStore private constructor(private val context: Context) { +class GeohashBookmarksStore private constructor( + private val context: Context, + private val resolveNames: Boolean +) { companion object { private const val TAG = "GeohashBookmarksStore" @@ -30,10 +33,18 @@ class GeohashBookmarksStore private constructor(private val context: Context) { @Volatile private var INSTANCE: GeohashBookmarksStore? = null fun getInstance(context: Context): GeohashBookmarksStore { return INSTANCE ?: synchronized(this) { - INSTANCE ?: GeohashBookmarksStore(context.applicationContext).also { INSTANCE = it } + INSTANCE ?: GeohashBookmarksStore(context.applicationContext, resolveNames = true) + .also { INSTANCE = it } } } + /** + * Isolated store for unit tests. Reverse geocoding is disabled so a test never + * reaches the network; the persistence and bookkeeping paths are unchanged. + */ + internal fun createForTest(context: Context): GeohashBookmarksStore = + GeohashBookmarksStore(context.applicationContext, resolveNames = false) + private val allowedChars = "0123456789bcdefghjkmnpqrstuvwxyz".toSet() fun normalize(raw: String): String { return raw.trim().lowercase(Locale.US) @@ -58,13 +69,16 @@ class GeohashBookmarksStore private constructor(private val context: Context) { init { load() } + @Synchronized fun isBookmarked(geohash: String): Boolean = membership.contains(normalize(geohash)) + @Synchronized fun toggle(geohash: String) { val gh = normalize(geohash) if (membership.contains(gh)) remove(gh) else add(gh) } + @Synchronized fun add(geohash: String) { val gh = normalize(geohash) if (gh.isEmpty() || membership.contains(gh)) return @@ -76,6 +90,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) { resolveNameIfNeeded(gh) } + @Synchronized fun remove(geohash: String) { val gh = normalize(geohash) if (!membership.contains(gh)) return @@ -142,6 +157,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) { // MARK: - Destructive Reset + @Synchronized fun clearAll() { try { membership.clear() @@ -162,7 +178,9 @@ class GeohashBookmarksStore private constructor(private val context: Context) { // MARK: - Friendly Name Resolution + @Synchronized fun resolveNameIfNeeded(geohash: String) { + if (!resolveNames) return val gh = normalize(geohash) if (gh.isEmpty()) return if (_bookmarkNames.value?.containsKey(gh) == true) return @@ -206,20 +224,39 @@ class GeohashBookmarksStore private constructor(private val context: Context) { pickNameForLength(gh.length, a) } - if (!name.isNullOrEmpty()) { - val current = _bookmarkNames.value.toMutableMap() - current[gh] = name - _bookmarkNames.value = current - persistNames(current) - } + commitResolvedName(gh, name) } catch (e: Exception) { Log.w(TAG, "Bookmark name resolution failed") } finally { - resolving.remove(gh) + finishResolving(gh) } } } + @Synchronized + private fun finishResolving(geohash: String) { + resolving.remove(geohash) + } + + /** + * Stores a resolved friendly name, but only while the geohash is still bookmarked. + * + * A reverse geocode runs on the IO dispatcher and can outlive the bookmark it was + * started for. Without this check a lookup in flight during [remove] or [clearAll] + * writes the place name back to SharedPreferences after the wipe, so a panic clear + * leaves a resolved location name on disk for the next launch. + */ + @Synchronized + internal fun commitResolvedName(geohash: String, name: String?) { + val gh = normalize(geohash) + if (gh.isEmpty() || name.isNullOrEmpty()) return + if (!membership.contains(gh)) return + val current = _bookmarkNames.value.toMutableMap() + current[gh] = name + _bookmarkNames.value = current + persistNames(current) + } + private fun pickNameForLength(len: Int, address: android.location.Address?): String? { if (address == null) return null return when (len) { From d89bfecd8c353bd0ed431997ea0655e3ca781b64 Mon Sep 17 00:00:00 2001 From: Taksh Date: Thu, 13 Aug 2026 06:36:39 +0530 Subject: [PATCH 2/3] Cover the bookmark name commit rules Pins the three cases the commit seam has to get right: a name resolved for a live bookmark is stored, and a name that lands after remove or after a panic clearAll is dropped, including across a reload from SharedPreferences. createForTest builds an isolated store with reverse geocoding disabled so the tests never reach the network. --- .../geohash/GeohashBookmarkNameCommitTest.kt | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt diff --git a/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt new file mode 100644 index 00000000..b6c96e8b --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt @@ -0,0 +1,82 @@ +package com.bitchat.android.geohash + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * A reverse geocode started for a bookmark runs on the IO dispatcher and can finish after + * the bookmark is gone. These tests pin the commit step: a resolved name is stored only + * while the geohash is still bookmarked. + */ +@RunWith(RobolectricTestRunner::class) +class GeohashBookmarkNameCommitTest { + + private lateinit var store: GeohashBookmarksStore + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + store = GeohashBookmarksStore.createForTest(context) + store.clearAll() + } + + @Test + fun `resolved name is stored for a bookmark that is still present`() { + store.add("u4pruy") + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertEquals("Copenhagen", store.bookmarkNames.value["u4pruy"]) + } + + @Test + fun `resolved name is dropped when the bookmark was removed while in flight`() { + store.add("u4pruy") + store.remove("u4pruy") + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertNull(store.bookmarkNames.value["u4pruy"]) + } + + @Test + fun `resolved name is dropped when a panic clear happened while in flight`() { + store.add("u4pruy") + store.clearAll() + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertNull(store.bookmarkNames.value["u4pruy"]) + assertEquals(emptyMap(), store.bookmarkNames.value) + } + + @Test + fun `a panic clear survives a restart when a lookup lands after the wipe`() { + store.add("u4pruy") + store.clearAll() + store.commitResolvedName("u4pruy", "Copenhagen") + + // A fresh store reads back what was persisted, which is what the next launch sees. + val reloaded = GeohashBookmarksStore.createForTest( + ApplicationProvider.getApplicationContext() + ) + assertEquals(emptyList(), reloaded.bookmarks.value) + assertEquals(emptyMap(), reloaded.bookmarkNames.value) + } + + @Test + fun `an empty or blank name is never stored`() { + store.add("u4pruy") + + store.commitResolvedName("u4pruy", null) + store.commitResolvedName("u4pruy", "") + + assertNull(store.bookmarkNames.value["u4pruy"]) + } +}