From e3d3cebdd454bf9e0a16d4f147bcaf8a0155d1a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 20 Jun 2026 20:08:08 +0000 Subject: [PATCH] refactor(notifications): tighten public-chat reply walk and its docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit follow-ups, no behaviour change: - isNotifiablePublicChatReply bails before allocating the HashSet/ArrayDeque scratch when the channel message has no parents (top-level posts — the common case), and builds the deque straight from the parent list. - Correct the docstring: a kind-42 replyTo holds only the immediate parent (the channel root is filtered out), so the chain is walked hop-by-hop through each cached ancestor — the previous wording implied replyTo already held the ancestors. - Trim the over-long inline comment on the acceptableEvent gate. - Make the multi-hop test prove what it claims: assert the immediate parent is not me, so the walk only passes by climbing to the grandparent. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PxDVCSWe1RwZ51vABBwqbG --- .../dal/NotificationFeedFilter.kt | 41 +++++++++---------- .../dal/NotificationPublicChatReplyTest.kt | 6 ++- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt index 210ea42ac4..76e2799c96 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationFeedFilter.kt @@ -148,35 +148,37 @@ class NotificationFeedFilter( ) + ADDRESSABLE_KINDS // How deep to walk a public chat reply chain looking for one of the - // user's own messages. Bounds the cost on very long threads and the + // user's own messages. Bounds the cost on very long threads; the // visited-set guards against malformed cyclic replyTo links. private const val PUBLIC_CHAT_ANCESTOR_SCAN_LIMIT = 30 /** * Public chats (NIP-28, kind 42) routinely reply to a user without * adding a `p` tag, so the normal mention gate ([Event.isTaggedUser]) - * misses them. Treat a channel message as "for me" when one of my own + * misses them. Treats a channel message as "for me" when one of my own * messages appears in its reply chain — a direct reply to my message - * (the common case the user described as "the previous message was - * mine") or a later message in a thread I'm already part of ("an - * active thread"). Reactions/zaps/reposts target via `replyTo` and are - * handled by the generic rules below; this is scoped to channel - * messages only. + * (the common case: "the previous message was mine") or a later message + * in a thread I'm already part of (an "active thread"). A kind-42 + * `replyTo` holds only the immediate parent, so the chain is walked + * hop-by-hop through each cached ancestor. * * Cache-only (reads [Note.replyTo] + author, never the account), so the - * push dispatcher and the in-app feed can both call it to relax the - * p-tag gate without loading the account or decrypting anything. + * push dispatcher and the in-app feed can both relax their p-tag gate + * with it without loading the account or decrypting anything. */ fun isNotifiablePublicChatReply( note: Note, authorHex: HexKey, ): Boolean { if (note.event !is ChannelMessageEvent) return false + // Top-level channel posts have no parent to reply to — bail before + // allocating the walk's scratch structures (the common case). + val parents = note.replyTo + if (parents.isNullOrEmpty()) return false var scanned = 0 val seen = HashSet() - val toVisit = ArrayDeque() - note.replyTo?.let { toVisit.addAll(it) } + val toVisit = ArrayDeque(parents) while (toVisit.isNotEmpty() && scanned < PUBLIC_CHAT_ANCESTOR_SCAN_LIMIT) { val ancestor = toVisit.removeFirst() @@ -403,16 +405,13 @@ class NotificationFeedFilter( // follow/list modes) also applies the per-kind relevance heuristics. val isRawGlobal = followList() is TopFilter.Global - // Channel messages may reply to one of my messages without a p-tag - // (common in NIP-28 clients), so the p-tag gate is OR'd with the - // public-chat reply check. Kept inline (not a pre-computed val) so the - // cheap `kind in NOTIFICATION_KINDS` check short-circuits ahead of it — - // otherwise the tag scan + reply-chain walk would run on every note in - // the cache. Within the OR, the cheaper tag scan runs before the - // reply-chain walk. tagsAnEventByUser below (also gated for - // Selected/follow modes) returns true for exactly the same events, so - // unrelated channel chatter never leaks through — even in Global mode, - // where it is the only relevance check. + // The p-tag gate is OR'd with isNotifiablePublicChatReply so channel + // replies into my messages still notify without a p-tag. Kept inline + // (not a pre-computed val) so the cheap kind check short-circuits ahead + // of the tag scan + reply walk, which is also why the cheaper tag scan + // is ordered first within the OR. In Global mode this is the only + // relevance check (tagsAnEventByUser is skipped below); it still scopes + // to genuine replies, so unrelated channel chatter never leaks through. return noteEvent?.kind in NOTIFICATION_KINDS && (noteEvent is LnZapEvent || notifAuthor != loggedInUserHex) && (isChessEvent || filterParams.isGlobal() || notifAuthor == null || filterParams.isAuthorInFollows(notifAuthor)) && diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationPublicChatReplyTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationPublicChatReplyTest.kt index 16681183b9..92022ae414 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationPublicChatReplyTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/notifications/dal/NotificationPublicChatReplyTest.kt @@ -78,13 +78,17 @@ class NotificationPublicChatReplyTest { } @Test - fun `reply deeper in a thread i posted in is notifiable`() { + fun `reply two hops above me in a thread i posted in is notifiable`() { // root(other) <- myMessage(me) <- someoneElse(other) <- newReply(other) + // newReply's immediate parent is `other`, so a single-hop check would + // miss me — this only passes if the walk climbs to the grandparent. val root = channelMessage("d".repeat(64), other, emptyList()) val myMessage = channelMessage("e".repeat(64), me, listOf(root)) val someoneElse = channelMessage("9".repeat(64), other, listOf(myMessage)) val newReply = channelMessage("8".repeat(64), other, listOf(someoneElse)) + // Sanity: the immediate parent is not me, so this is a true multi-hop hit. + assertFalse(newReply.replyTo?.any { it.author?.pubkeyHex == me } == true) assertTrue(NotificationFeedFilter.isNotifiablePublicChatReply(newReply, me)) }