fix: drop geohash bookmark name resolution that lands after a remove or panic clear - #875
Conversation
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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d89bfecd8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| store.add("u4pruy") | ||
|
|
||
| store.commitResolvedName("u4pruy", "Copenhagen") |
There was a problem hiding this comment.
For these geohash persistence tests, the fixture starts with a specific geohash and then stores a real city name (Copenhagen), and the same real-looking location fixture is repeated through the class. The repository privacy rules for geohash/location work require test data to be clearly synthetic, so please replace this with neutral documented fixture values before committing location-related test data.
AGENTS.md reference: AGENTS.md:L35-L45
Useful? React with 👍 / 👎.
1 participant
What
GeohashBookmarksStore.resolveNameIfNeededlaunches a reverse geocode onDispatchers.IOand holds no handle to cancel it. If the bookmark disappears while the lookup is in flight, the coroutine still runs to completion and writes the resolved place name into_bookmarkNames, then persists it to SharedPreferences.Two ways that happens today:
remove(gh)deletes the stored name, then the lookup lands and puts it back.clearAll()is the panic path.ChatViewModelcalls it with the comment "panic should remove everything", andclearAllitself says// Clear any in-flight resolutions to avoid repopulating. That comment does not hold — emptying theresolvingset does not stop a coroutine that is already running, so a geocoded location name can be written to disk after the wipe and is read back on the next launch.membershipwas also being mutated on the main thread and is now read from the IO dispatcher, so the accessors that touch it are guarded and theresolvingbookkeeping moved behind the same lock.How
Resolved names now go through
commitResolvedName, which drops the result unless the geohash is still bookmarked.resolveNameIfNeededis only ever called for entries of the bookmarks list (LocationChannelsSheet), so nothing legitimate is dropped.Tests
GeohashBookmarkNameCommitTestpins the three rules: a name resolved for a live bookmark is stored; a name landing afterremoveis dropped; a name landing after a panicclearAllis dropped, including across a reload from SharedPreferences.createForTestbuilds an isolated store with reverse geocoding disabled, so the tests never reach the network —add()would otherwise start a real Nominatim request under Robolectric.What I ran
./gradlew testDebugUnitTest lintDebug— green (596 app unit tests, 0 failures;:wear:lintDebugand:app:lintDebugboth pass).membershipguard fails 3 of the 5.