fix(notes): drop expired notes and match geohash tags case-insensitively - #896
fix(notes): drop expired notes and match geohash tags case-insensitively#896Chessing234 wants to merge 3 commits into
Conversation
Two ways the Android note handler disagrees with the iOS one, on the same event: - NIP-40. Relays are not required to drop expired events, so iOS checks the `expiration` tag client-side "so 24h dead drops actually vanish". Android had no such check, so an expired note kept showing until the list was cleared - the opposite of what a dead drop promises. - Case. Tag names and geohashes are case-insensitive, and iOS lowercases both before matching. Android compared them exactly, so a note tagged ["G", "U4PRUYD"] was visible on iOS and invisible on Android. Both now follow the iOS handler.
![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: 293a751596
ℹ️ 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".
| val geohashTag = event.tags.firstOrNull { | ||
| it.size >= 2 && it[0].lowercase() == "g" && validGeohashes.contains(it[1].lowercase()) | ||
| } |
There was a problem hiding this comment.
For relay-delivered events with an uppercase G tag or uppercase geohash, this handler still will not run: LocationNotesInitializer subscribes with the original NostrFilter, and NostrRelayManager.handleMessage calls subInfo.filter.matches(response.event) before dispatch; NostrFilter.matches still compares tag names and values exactly (it[0] == tagName, eventValues.contains(requiredValue)). Because the lowercasing happens only here, those events are dropped in the relay layer and Android still diverges from the intended case-insensitive behavior unless the filter match/subscription path is normalized too.
AGENTS.md reference: AGENTS.md:L90-L91
Useful? React with 👍 / 👎.
| // NIP-40: relays are not required to drop expired events, so enforce it | ||
| // here - otherwise a 24h dead drop stays visible past its expiry. | ||
| val expiresAt = expirationSeconds(event) | ||
| if (expiresAt != null && expiresAt * 1000L <= System.currentTimeMillis()) { |
There was a problem hiding this comment.
When a note arrives before its NIP-40 expiration time, this check admits it into _notes, but the manager does not store expiresAt or schedule/poll any later removal. In a long-lived nearby-notes session, a 24h dead-drop received shortly before expiry therefore remains visible in the sheet/header until the user refreshes, moves cells, or stops the subscription, so the new expiration enforcement still misses the common “expires while displayed” case.
AGENTS.md reference: AGENTS.md:L90-L91
Useful? React with 👍 / 👎.
NostrRelayManager runs every incoming event through NostrFilter.matches before any handler sees it, and that comparison was exact. So the case-insensitive handling in the previous commit was unreachable over the relay path: a note tagged ["G", "U4PRUYD"] was dropped at the gate. iOS has no such client-side filter and lowercases the tag name and geohash where it reads them, so the note is visible there. The values this app filters on are hex ids or geohashes, both of which encode the same value in either case.
A dead drop can cross its NIP-40 expiry while it is on screen. Filtering at ingest alone kept it visible until the subscription was recreated, so the note outlived its expiry exactly in the session where someone is reading it. Notes now carry their expiry, and a 60s job started with the subscription (and cancelled with it) drops the ones that have passed -- the same interval and behaviour as the iOS manager. Ids stay in noteIDs so a relay replay cannot resurrect a dropped note.
|
both right, thanks — checked the relay path and the filter gate does run first (NostrRelayManager.handleMessage -> subInfo.filter.matches before dispatch), so the lowercasing in the handler was unreachable over a live subscription. 26100f4: NostrFilter.matches now compares tag names and values case-insensitively. the values this app filters on are hex ids (e, p) or geohashes (g) and both encode the same value in either case, and iOS has no client-side filter at all, so this is the behaviour android was supposed to have. added a test that a filter for u4pruyd accepts ["G", "U4PRUYD"] and still rejects a different geohash — it fails with the old exact comparison and passes now. 15af9bf: notes carry their expiry, and a 60s job started with the subscription and cancelled with it drops the ones that have passed — same interval as the iOS manager's prune timer. ids stay in noteIDs so a replay can't resurrect a dropped note. the pruning itself is a pure function so the boundary case (expiry exactly now) is covered in the tests. full :app:testDebugUnitTest and :app:lintDebug green. |
1 participant
Two ways
LocationNotesManager.handleEventdisagrees with the iOS handler on the same event.NIP-40 expiry. iOS drops expired notes client-side, with the reason in the code:
Android had no expiration check at all, so an expired note kept showing until the list was cleared or trimmed — the opposite of what a dead drop promises, and the kind of thing a user relies on rather than notices.
Tag case. Tag names and geohashes are case-insensitive; iOS lowercases both before matching:
Android compared them exactly (
it[0] == "g",subscribedGeohashes.contains(eventGeohash)), so a note tagged["G", "U4PRUYD"]appeared on iOS and was silently dropped on Android — two devices in the same cell showing different note lists.Both now follow the iOS handler. The expiration helper mirrors
LocationNotesManager.expirationDateon that side../gradlew :app:testDebugUnitTest :app:lintDebugpasses locally (rc=0). The handler needs relay and Android dependencies to exercise end to end, so there is no unit test here; the change is a direct transcription of the iOS conditions.