From fceafe731d4e2c209620972fc362bd8c6f00fce8 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 16 Sep 2026 15:56:37 -0400 Subject: [PATCH 1/4] fix(notifications): attribute a chat push to the user who sent it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applyChatStyle` resolved the sender with `getOtherMember`, which returns the first member that isn't you. That is the sender in a DM and an arbitrary participant anywhere else, so every group push carried the wrong name and photo. `PushChatMetadata.sendingUserId` was already decoded and never read. `planSenderLookup` now picks the identity: a linked device contact where one exists, otherwise the sending user's profile, and nobody for a group system message rather than a guessed participant. The profile is read from `user_profiles` first — `ChatMemberDataSource` already writes every synced member's profile there — and only falls through to `ProfileController` for someone the device has not seen. `MessagingStyle` also keyed every `Person` by `groupKey`, which is per-chat, so all senders in a group collapsed into one person and the first speaker's name and icon were stamped on everyone's messages. The key is now the sender's user id. --- .../app/notifications/NotificationService.kt | 77 ++++++++++--- .../notifications/SenderAttributionPlanner.kt | 52 +++++++++ .../SenderAttributionPlannerTest.kt | 107 ++++++++++++++++++ 3 files changed, 221 insertions(+), 15 deletions(-) create mode 100644 apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/SenderAttributionPlanner.kt create mode 100644 apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/SenderAttributionPlannerTest.kt diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt index 371f6ab4a5..8a0f56f664 100644 --- a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt @@ -47,6 +47,7 @@ import com.flipcash.services.models.chat.ChatType import com.flipcash.services.user.UserManager import com.flipcash.shared.notifications.R import com.getcode.utils.TraceType +import com.getcode.utils.hexEncodedString import com.getcode.utils.trace import com.google.firebase.messaging.FirebaseMessagingService import com.google.firebase.messaging.RemoteMessage @@ -262,7 +263,7 @@ class NotificationService : FirebaseMessagingService(), } val notificationId = if (chatId != null) { - builder.applyChatStyle(chatId, groupKey, title, body) + builder.applyChatStyle(chatId, groupKey, title, body, payload?.chatMetadata) } else { builder.setContentTitle(title).setContentText(body) SecureRandom().nextInt(Int.MAX_VALUE) @@ -288,40 +289,59 @@ class NotificationService : FirebaseMessagingService(), groupKey: String?, title: String?, body: String?, + metadata: PushChatMetadata?, ): Int { val notificationId = chatId.hashCode() // Prefer the device-contact identity (CONTACT_DM, or a counterparty saved - // in the address book): the user's own name + photo for them. Only when - // that's absent do we fetch the chat member and fall back to their - // server-side profile — which is the only identity a TIP_DM has. - val contactE164 = contactCoordinator.lookupContactByDmChatId(chatId.toString())?.e164 - val member = if (contactE164 == null) chatCoordinator.getOtherMember(chatId) else null - val e164 = contactE164 ?: member?.userProfile?.verifiedPhoneNumber + // in the address book): the user's own name + photo for them. The row is + // keyed by DM chat id, so a group can never match it — don't ask. + val contactE164 = if (metadata?.chatType != ChatType.GROUP) { + contactCoordinator.lookupContactByDmChatId(chatId.toString())?.e164 + } else { + null + } + + // Otherwise fall back to the sender's server-side profile — which is the only + // identity a TIP_DM has, and the only one a group participant has at all. + val lookup = planSenderLookup( + chatType = metadata?.chatType, + sendingUserId = metadata?.sendingUserId, + hasDeviceContact = contactE164 != null, + ) + val sender = when (lookup) { + is SenderLookup.ByUserId -> + resolveSenderProfile(lookup.userId)?.let { Sender(lookup.userId, it) } + SenderLookup.OtherMember -> + chatCoordinator.getOtherMember(chatId)?.let { Sender(it.userId, it.userProfile) } + SenderLookup.None -> null + } + val e164 = contactE164 ?: sender?.profile?.verifiedPhoneNumber val senderName = e164?.let { contactResolver.resolveName(it) } - ?: member?.userProfile?.displayName?.takeIf { it.isNotBlank() } - ?: member?.userProfile?.socialHandle() + ?: sender?.profile?.displayName?.takeIf { it.isNotBlank() } + ?: sender?.profile?.socialHandle() ?: title ?: "" // Device-contact photo (local, synchronous) first; otherwise the profile // picture URL loaded through the app's shared Coil loader (cache-first, - // network-bounded). Works for CONTACT_DM and TIP_DM alike. + // network-bounded). Works for every chat type: a group sender who is in the + // address book resolves through their profile's phone number. // // Size the rendition to the platform's large-icon dimension (density-scaled) rather than // grabbing the smallest THUMBNAIL — the server ships several thumbnail/display sizes and // the tiny 32px one looks grainy on the notification's person icon. val avatarPx = resources.getDimensionPixelSize(android.R.dimen.notification_large_icon_width) val avatar = e164?.let { resolveContactPhoto(it) } - ?: member?.userProfile?.profilePicture?.let { picture -> + ?: sender?.profile?.profilePicture?.let { picture -> // Through the resolver, not `picture.urlForSize` — the stored download URL expires // and the profile it came from may have been persisted days ago. mediaUrlResolver.urlForSize( media = picture, targetLongestSidePx = avatarPx, - // The counterparty's picture, so their profile is what authorizes a re-mint. - access = BlobAccessContext.profile(member?.userId), + // The sender's picture, so their profile is what authorizes a re-mint. + access = BlobAccessContext.profile(sender.userId), )?.let { url -> loadRemoteAvatar(url, picture.cacheKeyForSize(avatarPx)) } @@ -329,15 +349,21 @@ class NotificationService : FirebaseMessagingService(), trace( tag = "NotificationService", - message = "applyChatStyle: chatId=$chatId, groupKey=$groupKey, e164=$e164, hasMember=${member != null}, hasAvatar=${avatar != null}, authenticated=${userManager.accountCluster != null}", + message = "applyChatStyle: chatId=$chatId, groupKey=$groupKey, chatType=${metadata?.chatType}, lookup=${lookup::class.simpleName}, e164=$e164, hasSender=${sender != null}, hasAvatar=${avatar != null}, authenticated=${userManager.accountCluster != null}", type = TraceType.Log, ) val selfPerson = buildSelfPerson(this@NotificationService, userManager.profile, contactResolver) + // Key the sender by their user id. MessagingStyle tells participants apart by + // Person key, and groupKey is per-chat — under it every sender in a group + // collapses into one person, so the name and icon of whoever spoke first get + // stamped on all of their messages. + val senderKey = metadata?.sendingUserId?.hexEncodedString() ?: groupKey ?: "unknown" + val senderPerson = Person.Builder() .setName(senderName) - .setKey(groupKey ?: "unknown") + .setKey(senderKey) .apply { if (avatar != null) setIcon(IconCompat.createWithBitmap(avatar.toCircularBitmap())) } @@ -359,6 +385,27 @@ class NotificationService : FirebaseMessagingService(), return notificationId } + /** Who a chat push is attributed to: their id (for blob access) and their profile. */ + private data class Sender(val userId: ID, val profile: UserProfile) + + /** + * The sender's profile, cache-first. + * + * `user_profiles` is a superset of the chat roster — + * [com.flipcash.app.persistence.sources.ChatMemberDataSource] writes every synced member's + * profile into it — so this is a local read for anyone the app has already seen, and only + * reaches the network for a group participant it hasn't. The result is written back so the + * next push from them doesn't. + * + * The transcript's [com.flipcash.shared.chat.ChatCoordinator.requestSenderProfile] resolves + * the same profiles, but it is fire-and-forget by design (it must not stall Paging), and a + * notification has to render now. + */ + private suspend fun resolveSenderProfile(userId: ID): UserProfile? = + userProfileDataSource.getCachedProfile(userId) + ?: profileController.getProfileForUser(userId).getOrNull() + ?.also { userProfileDataSource.store(userId, it) } + /** First social handle (e.g. an X username) to render as a display name, if any. */ private fun UserProfile.socialHandle(): String? = socialAccounts.filterIsInstance() diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/SenderAttributionPlanner.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/SenderAttributionPlanner.kt new file mode 100644 index 0000000000..47455a11dd --- /dev/null +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/SenderAttributionPlanner.kt @@ -0,0 +1,52 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType +import com.getcode.opencode.model.core.ID + +/** + * Which member of a chat a push should be attributed to. + * + * Pure by construction, for the same reason [planPushHandling] is: the rule is + * what's worth testing, and it doesn't need a database or a live service to + * state. + */ +sealed interface SenderLookup { + /** Fetch this specific member — the user the server says sent the message. */ + data class ByUserId(val userId: ID) : SenderLookup + + /** Fetch "the member who isn't me". Only identifies a sender in a DM. */ + data object OtherMember : SenderLookup + + /** No member to fetch; render the sender from the push's own title. */ + data object None : SenderLookup +} + +/** + * Decides how to identify the sender of a chat push. + * + * [sendingUserId] is the answer whenever the payload carries one, and it is the + * only answer that works in a group: [SenderLookup.OtherMember] returns the first + * member that isn't you, which is the sender in a DM and an arbitrary participant + * anywhere else. + * + * @param chatType the chat's type, null when the payload carries no metadata + * @param sendingUserId the sender per the payload, null for a system message + * @param hasDeviceContact whether the chat is already linked to an address-book + * contact, which identifies a DM's counterparty without any fetch + */ +fun planSenderLookup( + chatType: ChatType?, + sendingUserId: ID?, + hasDeviceContact: Boolean, +): SenderLookup = when { + // A device-contact link is keyed by DM chat id, so the contact it resolves to is + // that DM's counterparty — the sender of anything we're notified about there, and + // a better identity than the server profile because it's the name and photo the + // user chose for them. Worth short-circuiting the member fetch for. + hasDeviceContact -> SenderLookup.None + sendingUserId != null -> SenderLookup.ByUserId(sendingUserId) + // A system message in a group has no sender. Guessing one would put a + // participant's name and face on something they didn't say. + chatType == ChatType.GROUP -> SenderLookup.None + else -> SenderLookup.OtherMember +} diff --git a/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/SenderAttributionPlannerTest.kt b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/SenderAttributionPlannerTest.kt new file mode 100644 index 0000000000..8396cbf109 --- /dev/null +++ b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/SenderAttributionPlannerTest.kt @@ -0,0 +1,107 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType +import com.getcode.opencode.model.core.ID +import kotlin.test.Test +import kotlin.test.assertEquals + +class SenderAttributionPlannerTest { + + private val alice: ID = listOf(0x0a, 0x0b, 0x0c) + + // region Group chats: the sender is whoever the payload names, or nobody + + @Test + fun `group push is attributed to the sending user`() { + assertEquals( + SenderLookup.ByUserId(alice), + planSenderLookup( + chatType = ChatType.GROUP, + sendingUserId = alice, + hasDeviceContact = false, + ), + ) + } + + @Test + fun `group push without a sender attributes to nobody rather than a member`() { + assertEquals( + SenderLookup.None, + planSenderLookup( + chatType = ChatType.GROUP, + sendingUserId = null, + hasDeviceContact = false, + ), + ) + } + + // endregion + + // region DMs: unchanged behaviour + + @Test + fun `dm push prefers the device contact over any fetch`() { + assertEquals( + SenderLookup.None, + planSenderLookup( + chatType = ChatType.CONTACT_DM, + sendingUserId = alice, + hasDeviceContact = true, + ), + ) + } + + @Test + fun `dm push without a device contact uses the sending user`() { + assertEquals( + SenderLookup.ByUserId(alice), + planSenderLookup( + chatType = ChatType.TIP_DM, + sendingUserId = alice, + hasDeviceContact = false, + ), + ) + } + + @Test + fun `dm push without a sending user falls back to the other member`() { + assertEquals( + SenderLookup.OtherMember, + planSenderLookup( + chatType = ChatType.TIP_DM, + sendingUserId = null, + hasDeviceContact = false, + ), + ) + } + + // endregion + + // region Payloads with no chat metadata at all + + @Test + fun `unknown chat type without a sending user falls back to the other member`() { + assertEquals( + SenderLookup.OtherMember, + planSenderLookup( + chatType = null, + sendingUserId = null, + hasDeviceContact = false, + ), + ) + } + + @Test + fun `unknown chat type still honours a sending user`() { + assertEquals( + SenderLookup.ByUserId(alice), + planSenderLookup( + chatType = null, + sendingUserId = alice, + hasDeviceContact = false, + ), + ) + } + + // endregion +} From f10523a8354169561fe454722a865e73656b56c9 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 16 Sep 2026 15:56:47 -0400 Subject: [PATCH 2/4] fix(chat): stop SenderResolver re-asking for senders it cannot resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `request` released a user id on any failure, and the transcript re-asks on every emission of its `combine` — each page load, each pending mutation, each write to `user_profiles`. One `NOT_FOUND` sender in a group therefore cost a `GetProfile` per emission for the life of the process. `NOT_FOUND` is an answer, not a failure to get one, so those ids are now held permanently. Other failures are held for 30s rather than released immediately. The next emission of the transcript is not evidence the network came back, so retrying on it just fails again, and being offline while scrolling was a burst of attempts that could not succeed. Opening a group whose loaded pages carry senders past the roster page asked for all of them in one frame. The in-flight set collapses duplicates but says nothing about the fan-out across distinct ids, so a semaphore caps concurrent fetches at six, held for the call alone. `clear` now cancels outstanding work. A fetch in flight was authorized by the account that just went away, and a sleeping backoff would otherwise wake to release an id the next account may already be fetching. --- .../shared/chat/internal/SenderResolver.kt | 96 ++++++++++++++++-- .../shared/chat/SenderResolverTest.kt | 97 ++++++++++++++++--- 2 files changed, 168 insertions(+), 25 deletions(-) diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/SenderResolver.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/SenderResolver.kt index 105d34ec3e..d955e6bbdb 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/SenderResolver.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/SenderResolver.kt @@ -3,6 +3,7 @@ package com.flipcash.shared.chat.internal import com.flipcash.app.persistence.sources.UserProfileDataSource import com.flipcash.libs.coroutines.DispatcherProvider import com.flipcash.services.controllers.ProfileController +import com.flipcash.services.models.GetUserProfileError import com.flipcash.services.models.UserProfile import com.getcode.opencode.model.core.ID import com.getcode.utils.TraceType @@ -10,10 +11,16 @@ import com.getcode.utils.hexEncodedString import com.getcode.utils.trace import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancelChildren +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withPermit import javax.inject.Inject import javax.inject.Singleton +import kotlin.time.Duration +import kotlin.time.Duration.Companion.seconds /** * Resolves the profiles of message senders the roster does not cover. @@ -29,6 +36,18 @@ import javax.inject.Singleton * fills in. That is why there is no suspend `resolve(userId)` here: the transcript is a Paging * stream, and a per-item suspend call would stall page composition on the network. * + * `GetProfile` takes one user id — there is no batch RPC, and no chat RPC that lists members past + * the page `GetChat` returns — so one call per unknown sender is the floor. What this class owns is + * making sure it is also the ceiling: the caller re-asks on every emission of the transcript's + * `combine`, which is once per page load, per pending mutation, and per write to `user_profiles`. + * Three things keep that from turning into a call per emission: + * + * - **A resolved sender stays marked.** Their profile is in `user_profiles`, so the transcript + * reads it from [profiles] and never reaches [request] again. + * - **An unresolvable sender stays marked.** See [unresolvable]. + * - **A sender whose fetch failed for any other reason is held for [RETRY_BACKOFF].** See + * [inFlight]. + * * Owns its own scope because [com.flipcash.shared.chat.internal.delegates.MessagingDelegate] — its * only caller — has no `initialize(scope)` and giving it one would churn `RealChatCoordinator` and * every construction of it in tests. @@ -41,51 +60,108 @@ class SenderResolver @Inject constructor( ) { private val scope = CoroutineScope(dispatchers.IO + SupervisorJob()) + /** Guards [inFlight] and [unresolvable], which are read and written together in [request]. */ + private val lock = Any() + /** - * User-id hexes with a fetch in flight. Marked *before* the launch, so two requests arriving - * in the same frame collapse on the calling thread instead of racing to the network. + * User-id hexes with a fetch in flight, or cooling down after a failure. Marked *before* the + * launch, so two requests arriving in the same frame collapse on the calling thread instead of + * racing to the network. + * + * A transient failure — being offline is the common one — keeps the id marked for + * [RETRY_BACKOFF] rather than releasing it immediately. Releasing it immediately makes the + * retry cadence "however often the transcript happens to re-emit", which is neither a policy + * nor bounded: a scroll while offline is a burst of retries that cannot succeed. */ private val inFlight = mutableSetOf() + /** + * User-id hexes the server has no profile for, which are never asked for again. + * + * `NOT_FOUND` is an answer, not a failure to get one — the account is gone, or was never + * visible to this viewer. Treating it as transient is what made a single deleted sender in a + * group cost a `GetProfile` on every re-emission for the life of the process. + */ + private val unresolvable = mutableSetOf() + + /** + * Caps how many profile fetches are on the network at once. + * + * Opening a group whose loaded pages carry senders past the roster page asks for all of them in + * one frame. [inFlight] collapses duplicates but says nothing about the fan-out across distinct + * ids, and none of these are urgent — a bubble renders without its name either way. The permit + * is held for the call alone, so a backing-off id occupies nothing. + */ + private val fetchLimit = Semaphore(MAX_CONCURRENT_FETCHES) + /** Every profile this device holds, keyed by user-id hex. The transcript indexes into it. */ val profiles: Flow> = userProfileDataSource.observeProfiles() /** * Asks for [userId]'s profile if nothing has asked already. Returns immediately. - * - * A failure releases the id rather than caching the miss: the common cause is being offline, - * and a permanently poisoned id would leave the sender nameless for the life of the process. */ fun request(userId: ID) { val hex = userId.hexEncodedString() - synchronized(inFlight) { + synchronized(lock) { + if (hex in unresolvable) return if (!inFlight.add(hex)) return } scope.launch { - profileController.getProfileForUser(userId) + fetchLimit.withPermit { profileController.getProfileForUser(userId) } .onSuccess { profile -> userProfileDataSource.store(userId, profile) // Kept marked on success: the profile is in `user_profiles` now, so the // transcript reads it from [profiles] and never asks again. } .onFailure { error -> - synchronized(inFlight) { inFlight.remove(hex) } + if (error is GetUserProfileError.NotFound) { + synchronized(lock) { unresolvable.add(hex) } + trace( + tag = TAG, + message = "no profile exists for sender $hex; will not ask again", + type = TraceType.Silent, + ) + return@onFailure + } + trace( tag = TAG, message = "failed to resolve sender $hex", error = error, type = TraceType.Error, ) + // Held, not released: the next emission of the transcript is not a signal that + // the network came back, and asking again on it would just fail again. + delay(RETRY_BACKOFF) + synchronized(lock) { inFlight.remove(hex) } } } } - /** Drops what this process has asked for, so a re-login re-resolves against the new account. */ + /** + * Drops what this process has asked for, so a re-login re-resolves against the new account. + * + * Outstanding work is cancelled rather than left to finish. A fetch in flight was authorized by + * the account that just went away, and a backoff still sleeping would wake to release an id the + * next account may already be fetching. + */ fun clear() { - synchronized(inFlight) { inFlight.clear() } + synchronized(lock) { + inFlight.clear() + unresolvable.clear() + } + scope.coroutineContext.cancelChildren() } private companion object { const val TAG = "SenderResolver" + + /** + * Enough to fill a screen of unfamiliar senders without opening a connection per bubble. + */ + const val MAX_CONCURRENT_FETCHES = 6 + + /** How long a sender whose fetch failed is left alone before anything asks again. */ + val RETRY_BACKOFF: Duration = 30.seconds } } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/SenderResolverTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/SenderResolverTest.kt index 9423f75a02..ab14321ec2 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/SenderResolverTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/SenderResolverTest.kt @@ -4,29 +4,37 @@ import app.cash.turbine.test import com.flipcash.app.core.dispatchers.TestDispatchers import com.flipcash.app.persistence.sources.UserProfileDataSource import com.flipcash.services.controllers.ProfileController +import com.flipcash.services.models.GetUserProfileError import com.flipcash.services.models.UserProfile import com.flipcash.shared.chat.internal.SenderResolver import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.mockk +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds /** * `GetProfile` is single-user, so every distinct unknown sender in a group costs one call. The - * guarantee this test exists for is that it costs no more than one: the transcript asks for a - * sender on every recomposition that sees a bubble from them, and an unguarded resolver would - * fan that out into a call per frame. + * guarantee these tests exist for is that it costs no more than one: the transcript asks for a + * sender on every emission that sees a bubble from them — a page load, a pending mutation, any + * write to `user_profiles` — and an unguarded resolver would fan that out into a call per emission. * * The dedupe marks a user id as in-flight *before* launching the fetch, so the second request * is dropped on the calling thread. That makes the behaviour deterministic under * `StandardTestDispatcher` rather than dependent on when the coroutine happens to be scheduled. */ +@OptIn(ExperimentalCoroutinesApi::class) class SenderResolverTest { private val scheduler = TestCoroutineScheduler() @@ -74,21 +82,80 @@ class SenderResolverTest { resolver.clear() } + // region Failures + @Test - fun `a failed fetch releases the id so a later request retries`() = runTest(dispatchers.dispatcher) { - coEvery { profileController.getProfileForUser(userId) } returns - Result.failure(RuntimeException("offline")) - val resolver = subject() + fun `a transient failure is held for the backoff window, then retried`() = + runTest(dispatchers.dispatcher) { + coEvery { profileController.getProfileForUser(userId) } returns + Result.failure(RuntimeException("offline")) + val resolver = subject() - resolver.request(userId) - advanceUntilIdle() - resolver.request(userId) - advanceUntilIdle() + resolver.request(userId) + runCurrent() - coVerify(exactly = 2) { profileController.getProfileForUser(userId) } - coVerify(exactly = 0) { userProfileDataSource.store(any(), any()) } - resolver.clear() - } + // A re-emission inside the window is not evidence the network came back, so it does + // not buy another attempt. + resolver.request(userId) + runCurrent() + coVerify(exactly = 1) { profileController.getProfileForUser(userId) } + + advanceTimeBy(31.seconds) + runCurrent() + resolver.request(userId) + runCurrent() + + coVerify(exactly = 2) { profileController.getProfileForUser(userId) } + coVerify(exactly = 0) { userProfileDataSource.store(any(), any()) } + resolver.clear() + } + + @Test + fun `a sender the server has no profile for is never asked about again`() = + runTest(dispatchers.dispatcher) { + coEvery { profileController.getProfileForUser(userId) } returns + Result.failure(GetUserProfileError.NotFound()) + val resolver = subject() + + resolver.request(userId) + advanceUntilIdle() + + // NOT_FOUND is an answer, not a failure to get one: waiting out a backoff and asking + // again would get the same answer for as long as the transcript keeps showing the + // bubble. + advanceTimeBy(5.minutes) + resolver.request(userId) + advanceUntilIdle() + + coVerify(exactly = 1) { profileController.getProfileForUser(userId) } + resolver.clear() + } + + // endregion + + @Test + fun `fetches are capped so opening a group does not fan out per bubble`() = + runTest(dispatchers.dispatcher) { + val gate = CompletableDeferred() + var started = 0 + coEvery { profileController.getProfileForUser(any()) } coAnswers { + started++ + gate.await() + Result.success(profile) + } + val resolver = subject() + + repeat(20) { index -> resolver.request(ByteArray(16) { index.toByte() }.toList()) } + advanceUntilIdle() + + assertEquals(6, started) + + gate.complete(Unit) + advanceUntilIdle() + + assertEquals(20, started) + resolver.clear() + } @Test fun `resolved profiles are observed off the shared user_profiles table`() = From 32ffbb1b56af0ff2fc63021e9d07e7c8be0be7e9 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 16 Sep 2026 17:12:42 -0400 Subject: [PATCH 3/4] fix(notifications): render a group chat push as a group conversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applyChatStyle` builds a `MessagingStyle` but never calls `setGroupConversation` or `setConversationTitle`, so every chat push posts as a one-to-one. The platform reads that as "the notification title already says who this is" and drops the sender's name from the message line, and there is no group name anywhere on the notification. Set both, and set them after `extractMessagingStyleFromNotification`: a re-post rebuilds the style from the notification already on screen, which carries the previous flag and title back with it. The title comes from `chat_metadata.title` — the same row the conversation list is named from — through a new single-column DAO read. It is applied only when non-null, so a push that lands before the group's row syncs leaves whatever an earlier push resolved rather than blanking it. Resolving the type moved into `planConversationStyling`, payload first and stored row second, because `chat_metadata` is optional on the wire (`ProtobufToLocal.asPayload`). A group push that omits it was passing for a DM, which also sent `planSenderLookup` down its `OtherMember` branch and put an arbitrary participant's name on the notification. Both the styling and the sender lookup now key off the one resolved value. A group this device has never synced still posts untitled — the push's own `title` field is the sender-name fallback on this path, so reading it as a conversation title would be a guess about what the server puts there. --- .../ConversationStylingPlanner.kt | 58 ++++++++ .../app/notifications/NotificationService.kt | 23 ++- .../ConversationStylingPlannerTest.kt | 133 ++++++++++++++++++ .../app/persistence/dao/ChatMetadataDao.kt | 9 ++ .../sources/ChatMetadataDataSource.kt | 4 + 5 files changed, 224 insertions(+), 3 deletions(-) create mode 100644 apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ConversationStylingPlanner.kt create mode 100644 apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ConversationStylingPlannerTest.kt diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ConversationStylingPlanner.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ConversationStylingPlanner.kt new file mode 100644 index 0000000000..c965a32c28 --- /dev/null +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ConversationStylingPlanner.kt @@ -0,0 +1,58 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType + +/** + * How a chat push should render as a conversation. + * + * Pure for the same reason [planSenderLookup] is: the rule is what's worth testing, and stating + * it needs neither a database nor a live service. + */ +data class ConversationStyling( + /** The chat's type, whichever source could answer. Also what the sender lookup keys off. */ + val chatType: ChatType, + /** + * Whether `MessagingStyle` should treat this as a group. + * + * Without it the platform renders every message as a one-to-one: no conversation title, and + * the sender's name dropped from the line because in a DM the notification's own title + * already says who it is. + */ + val isGroupConversation: Boolean, + /** + * What to call the group, or null to leave whatever is already on screen. + * + * Null rather than a placeholder because a re-posted notification carries the previous + * title forward, and a group whose row hasn't synced yet is better untitled than renamed. + */ + val conversationTitle: String?, +) + +/** + * Decides whether a chat push is a group conversation and what to call it. + * + * The type is taken from [payloadChatType] first and [storedChatType] second. `chat_metadata` is + * optional on the wire, and a push that omits it would otherwise pass for a DM — which is how a + * group ends up rendered as one, with an arbitrary participant's name on it. The device already + * knows the type of any chat it has synced, so it only falls through to [ChatType.UNKNOWN] for a + * group this device has never seen. + * + * @param payloadChatType the type the push declares, null when it carries no chat metadata + * @param storedChatType the type in `chat_metadata`, [ChatType.UNKNOWN] when the chat isn't stored + * @param storedTitle the group's name in `chat_metadata`, null for a DM or an unstored chat + */ +fun planConversationStyling( + payloadChatType: ChatType?, + storedChatType: ChatType, + storedTitle: String?, +): ConversationStyling { + val chatType = payloadChatType?.takeUnless { it == ChatType.UNKNOWN } ?: storedChatType + val isGroup = chatType == ChatType.GROUP + return ConversationStyling( + chatType = chatType, + isGroupConversation = isGroup, + // A DM is named by its counterparty, so a title on one would be the wrong name in a + // place the user can't correct. Blank is the same as absent. + conversationTitle = storedTitle?.takeIf { isGroup && it.isNotBlank() }, + ) +} diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt index 8a0f56f664..f783f60f3e 100644 --- a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt @@ -29,6 +29,7 @@ import com.flipcash.app.contacts.ContactResolver import com.flipcash.app.core.util.Linkify import com.flipcash.shared.chat.ChatCoordinator import com.flipcash.app.tokens.TokenCoordinator +import com.flipcash.app.persistence.sources.ChatMetadataDataSource import com.flipcash.app.persistence.sources.UserProfileDataSource import com.flipcash.services.controllers.ProfileController import com.flipcash.services.controllers.PushController @@ -115,6 +116,9 @@ class NotificationService : FirebaseMessagingService(), @Inject lateinit var userProfileDataSource: UserProfileDataSource + @Inject + lateinit var chatMetadataDataSource: ChatMetadataDataSource + // TODO(firebase-messaging): 25.1.0 deprecated onNewToken in favor of FID-based onRegistered(). // Migrate once Firebase ships a stable guide and the backend accepts FID registration. // Tracking: https://github.com/firebase/firebase-android-sdk/issues/8087 @@ -293,10 +297,16 @@ class NotificationService : FirebaseMessagingService(), ): Int { val notificationId = chatId.hashCode() + val styling = planConversationStyling( + payloadChatType = metadata?.chatType, + storedChatType = chatMetadataDataSource.getChatType(chatId), + storedTitle = chatMetadataDataSource.getTitle(chatId), + ) + // Prefer the device-contact identity (CONTACT_DM, or a counterparty saved // in the address book): the user's own name + photo for them. The row is // keyed by DM chat id, so a group can never match it — don't ask. - val contactE164 = if (metadata?.chatType != ChatType.GROUP) { + val contactE164 = if (!styling.isGroupConversation) { contactCoordinator.lookupContactByDmChatId(chatId.toString())?.e164 } else { null @@ -305,7 +315,7 @@ class NotificationService : FirebaseMessagingService(), // Otherwise fall back to the sender's server-side profile — which is the only // identity a TIP_DM has, and the only one a group participant has at all. val lookup = planSenderLookup( - chatType = metadata?.chatType, + chatType = styling.chatType, sendingUserId = metadata?.sendingUserId, hasDeviceContact = contactE164 != null, ) @@ -349,7 +359,7 @@ class NotificationService : FirebaseMessagingService(), trace( tag = "NotificationService", - message = "applyChatStyle: chatId=$chatId, groupKey=$groupKey, chatType=${metadata?.chatType}, lookup=${lookup::class.simpleName}, e164=$e164, hasSender=${sender != null}, hasAvatar=${avatar != null}, authenticated=${userManager.accountCluster != null}", + message = "applyChatStyle: chatId=$chatId, groupKey=$groupKey, chatType=${styling.chatType}, isGroup=${styling.isGroupConversation}, hasTitle=${styling.conversationTitle != null}, lookup=${lookup::class.simpleName}, e164=$e164, hasSender=${sender != null}, hasAvatar=${avatar != null}, authenticated=${userManager.accountCluster != null}", type = TraceType.Log, ) @@ -377,6 +387,13 @@ class NotificationService : FirebaseMessagingService(), style.addMessage(body.orEmpty(), System.currentTimeMillis(), senderPerson) + // After the extract above, not before: a re-post rebuilds the style from the notification + // already on screen, which carries the old flag and title back with it. + style.setGroupConversation(styling.isGroupConversation) + // Only when there is one to set, so a push that lands before the group's row syncs leaves + // the title a previous push managed to resolve rather than blanking it. + styling.conversationTitle?.let { style.setConversationTitle(it) } + setStyle(style) .setGroupAlertBehavior(NotificationCompat.GROUP_ALERT_CHILDREN) .addAction(buildReplyAction(chatId, notificationId, groupKey)) diff --git a/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ConversationStylingPlannerTest.kt b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ConversationStylingPlannerTest.kt new file mode 100644 index 0000000000..b0e458cde5 --- /dev/null +++ b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ConversationStylingPlannerTest.kt @@ -0,0 +1,133 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class ConversationStylingPlannerTest { + + // region Groups + + @Test + fun `group push is a group conversation named by the stored row`() { + val styling = planConversationStyling( + payloadChatType = ChatType.GROUP, + storedChatType = ChatType.GROUP, + storedTitle = "Lunch crew", + ) + + assertTrue(styling.isGroupConversation) + assertEquals("Lunch crew", styling.conversationTitle) + } + + @Test + fun `group this device has not stored is still a group conversation`() { + // The first push from a group joined elsewhere arrives before its feed row does. + val styling = planConversationStyling( + payloadChatType = ChatType.GROUP, + storedChatType = ChatType.UNKNOWN, + storedTitle = null, + ) + + assertTrue(styling.isGroupConversation) + assertNull(styling.conversationTitle) + } + + @Test + fun `a blank title is not a title`() { + val styling = planConversationStyling( + payloadChatType = ChatType.GROUP, + storedChatType = ChatType.GROUP, + storedTitle = " ", + ) + + assertNull(styling.conversationTitle) + } + + // endregion + + // region DMs + + @Test + fun `dm push is not a group conversation`() { + val styling = planConversationStyling( + payloadChatType = ChatType.CONTACT_DM, + storedChatType = ChatType.CONTACT_DM, + storedTitle = null, + ) + + assertFalse(styling.isGroupConversation) + assertNull(styling.conversationTitle) + } + + @Test + fun `a title stored against a dm is not used to name it`() { + // A DM is named by its counterparty. A title on one is a row that shouldn't have it, + // and honouring it would put a name on the notification the user cannot correct. + val styling = planConversationStyling( + payloadChatType = ChatType.TIP_DM, + storedChatType = ChatType.TIP_DM, + storedTitle = "Lunch crew", + ) + + assertFalse(styling.isGroupConversation) + assertNull(styling.conversationTitle) + } + + // endregion + + // region Payloads that don't say what the chat is + + @Test + fun `push without chat metadata falls back to the stored type`() { + val styling = planConversationStyling( + payloadChatType = null, + storedChatType = ChatType.GROUP, + storedTitle = "Lunch crew", + ) + + assertEquals(ChatType.GROUP, styling.chatType) + assertTrue(styling.isGroupConversation) + assertEquals("Lunch crew", styling.conversationTitle) + } + + @Test + fun `an unknown type on the wire falls back to the stored type`() { + val styling = planConversationStyling( + payloadChatType = ChatType.UNKNOWN, + storedChatType = ChatType.GROUP, + storedTitle = null, + ) + + assertEquals(ChatType.GROUP, styling.chatType) + assertTrue(styling.isGroupConversation) + } + + @Test + fun `neither source knowing the type renders as a one-to-one`() { + val styling = planConversationStyling( + payloadChatType = null, + storedChatType = ChatType.UNKNOWN, + storedTitle = null, + ) + + assertEquals(ChatType.UNKNOWN, styling.chatType) + assertFalse(styling.isGroupConversation) + } + + @Test + fun `the payload wins over a stale stored type`() { + val styling = planConversationStyling( + payloadChatType = ChatType.GROUP, + storedChatType = ChatType.TIP_DM, + storedTitle = null, + ) + + assertEquals(ChatType.GROUP, styling.chatType) + } + + // endregion +} diff --git a/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/dao/ChatMetadataDao.kt b/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/dao/ChatMetadataDao.kt index 683078121d..b680193d06 100644 --- a/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/dao/ChatMetadataDao.kt +++ b/apps/flipcash/shared/persistence/db/src/main/kotlin/com/flipcash/app/persistence/dao/ChatMetadataDao.kt @@ -171,6 +171,15 @@ interface ChatMetadataDao { @Query("SELECT chat_type FROM chat_metadata WHERE chat_id_hex = :chatIdHex") suspend fun getChatType(chatIdHex: String): String? + /** + * A group's name, or null when the chat is not stored or is a DM. + * + * The two nulls read the same here on purpose: a DM has no title to return, so a caller that + * wants one is asking about a group either way. + */ + @Query("SELECT title FROM chat_metadata WHERE chat_id_hex = :chatIdHex") + suspend fun getTitle(chatIdHex: String): String? + @Query("SELECT analytics_counted_through FROM chat_metadata WHERE chat_id_hex = :chatIdHex") suspend fun getAnalyticsCountedThrough(chatIdHex: String): Long? diff --git a/apps/flipcash/shared/persistence/sources/src/main/kotlin/com/flipcash/app/persistence/sources/ChatMetadataDataSource.kt b/apps/flipcash/shared/persistence/sources/src/main/kotlin/com/flipcash/app/persistence/sources/ChatMetadataDataSource.kt index 262b53de12..ffe0ff3fcf 100644 --- a/apps/flipcash/shared/persistence/sources/src/main/kotlin/com/flipcash/app/persistence/sources/ChatMetadataDataSource.kt +++ b/apps/flipcash/shared/persistence/sources/src/main/kotlin/com/flipcash/app/persistence/sources/ChatMetadataDataSource.kt @@ -102,6 +102,10 @@ class ChatMetadataDataSource @Inject constructor( return ChatType.entries.firstOrNull { it.name == stored } ?: ChatType.UNKNOWN } + /** [chatId]'s group name, or null when the chat is a DM or is not stored on this device. */ + suspend fun getTitle(chatId: ChatId): String? = + db?.chatMetadataDao()?.getTitle(mapper.chatIdHex(chatId)) + suspend fun getAnalyticsCountedThrough(chatId: ChatId): Long = db?.chatMetadataDao()?.getAnalyticsCountedThrough(mapper.chatIdHex(chatId)) ?: 0L From 73e947c6c2566d2c92cd6ccdcf94e56eb56af16a Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 16 Sep 2026 17:22:06 -0400 Subject: [PATCH 4/4] fix(notifications): open the conversation from a group chat push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildContentIntent` deep-linked `ChatType.TIP_DM` alone, so tapping a group push launched the app on the camera and left the user to find the chat. The `/tip/chat/{id}` link is named for where it was first used, but `AppRouter` resolves it to the Chats tab and the conversation by id — which is the destination for a group too, since that tab lists both types (`TipFlowViewModel`: `feed(TIP_DM, GROUP)`). `planChatTapTarget` now decides from the resolved chat type, and only a `CONTACT_DM` still falls back to the launcher: the Send tab that opened one was removed and `AppRouter` routes nothing for it. `planConversationStyling` moved up into `postNotification` so the tap target and the message style share one resolved type. The content intent was reading `payload.chatMetadata.chatType` directly and had the same blind spot the style did — `chat_metadata` is optional on the wire, and a group push that omits it would still have landed on the camera. Every content intent was also built under request code 99. `FLAG_UPDATE_CURRENT` rewrites the extras of whichever PendingIntent is held under a code, and `filterEquals` ignores extras, so with two chats notified both notifications pointed at whichever was posted last. The code now travels with the target. --- .../app/notifications/ChatTapTargetPlanner.kt | 36 +++++++++ .../app/notifications/NotificationService.kt | 76 +++++++++++++------ .../notifications/ChatTapTargetPlannerTest.kt | 38 ++++++++++ 3 files changed, 125 insertions(+), 25 deletions(-) create mode 100644 apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ChatTapTargetPlanner.kt create mode 100644 apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ChatTapTargetPlannerTest.kt diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ChatTapTargetPlanner.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ChatTapTargetPlanner.kt new file mode 100644 index 0000000000..9158a11376 --- /dev/null +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/ChatTapTargetPlanner.kt @@ -0,0 +1,36 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType + +/** Where tapping a chat notification lands. */ +enum class ChatTapTarget { + /** The conversation itself, through the app's chat-by-id link. */ + Conversation, + + /** + * A plain launch, which opens the app on the camera. + * + * The fallback for a chat with no screen to open, rather than firing a link nothing routes: + * an unrouted link still leaves the app on whatever it was showing, having taken the tap. + */ + AppLauncher, +} + +/** + * Whether a chat push can open its conversation. + * + * The Chats tab lists tip DMs and groups (`TipFlowViewModel`: `feed(TIP_DM, GROUP)`), and both + * open by chat id from there, so both are reachable through the `/tip/chat/{id}` link the router + * resolves to that tab. A `CONTACT_DM` is not: the Send tab and direct-send flow that used to + * open one were removed, and `AppRouter` deliberately routes nothing for them. + * + * [chatType] is the type resolved by [planConversationStyling], not what the payload declared. + * `chat_metadata` is optional on the wire, and a group push that omits it would otherwise land on + * the camera. Null and [ChatType.UNKNOWN] read the same: neither the push nor the device could + * say what the chat is, and a chat screen opened for a type the app has no surface for would + * dead-end. + */ +fun planChatTapTarget(chatType: ChatType?): ChatTapTarget = when (chatType) { + ChatType.TIP_DM, ChatType.GROUP -> ChatTapTarget.Conversation + ChatType.CONTACT_DM, ChatType.UNKNOWN, null -> ChatTapTarget.AppLauncher +} diff --git a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt index f783f60f3e..f128db3721 100644 --- a/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt +++ b/apps/flipcash/shared/notifications/src/main/kotlin/com/flipcash/app/notifications/NotificationService.kt @@ -70,6 +70,13 @@ class NotificationService : FirebaseMessagingService(), private const val KEY_BODY = "push_notification_body" private const val KEY_PAYLOAD = "flipcash_payload" + /** + * Request code for the content intent of any notification that only launches the app. + * Arbitrary, and shared on purpose — every such intent is identical, so it does not + * matter which notification's the system keeps. + */ + private const val LAUNCH_REQUEST_CODE = 99 + // Correlation id for measurement runs. scripts/spike/ sends a known // sequence number with every push so a missing delivery and a late one // can be told apart in the log rather than reading as the same silence. @@ -253,6 +260,16 @@ class NotificationService : FirebaseMessagingService(), if (chatId != null && chatCoordinator.isActiveChat(chatId)) return + // Resolved once, here, because the tap target and the message style both turn on what + // kind of chat this is and neither can take the payload's word for it. + val styling = chatId?.let { + planConversationStyling( + payloadChatType = payload?.chatMetadata?.chatType, + storedChatType = chatMetadataDataSource.getChatType(it), + storedTitle = chatMetadataDataSource.getTitle(it), + ) + } + val groupKey = payload?.groupKey?.takeIf { it.isNotEmpty() } val builder = NotificationCompat.Builder(this, channel.id) @@ -261,13 +278,13 @@ class NotificationService : FirebaseMessagingService(), .setSmallIcon(R.drawable.flipcash_logo) .setColor(getColor(R.color.notification_color)) .setAutoCancel(true) - .setContentIntent(buildContentIntent(payload?.chatMetadata, payload?.navigation)) + .setContentIntent(buildContentIntent(styling?.chatType, payload?.navigation)) .apply { if (groupKey != null) setGroup(groupKey) } - val notificationId = if (chatId != null) { - builder.applyChatStyle(chatId, groupKey, title, body, payload?.chatMetadata) + val notificationId = if (chatId != null && styling != null) { + builder.applyChatStyle(chatId, styling, groupKey, title, body, payload?.chatMetadata) } else { builder.setContentTitle(title).setContentText(body) SecureRandom().nextInt(Int.MAX_VALUE) @@ -290,6 +307,7 @@ class NotificationService : FirebaseMessagingService(), private suspend fun NotificationCompat.Builder.applyChatStyle( chatId: ChatId, + styling: ConversationStyling, groupKey: String?, title: String?, body: String?, @@ -297,12 +315,6 @@ class NotificationService : FirebaseMessagingService(), ): Int { val notificationId = chatId.hashCode() - val styling = planConversationStyling( - payloadChatType = metadata?.chatType, - storedChatType = chatMetadataDataSource.getChatType(chatId), - storedTitle = chatMetadataDataSource.getTitle(chatId), - ) - // Prefer the device-contact identity (CONTACT_DM, or a counterparty saved // in the address book): the user's own name + photo for them. The row is // keyed by DM chat id, so a group can never match it — don't ask. @@ -560,33 +572,47 @@ class NotificationService : FirebaseMessagingService(), ).build() } + /** + * Where this notification's tap goes. + * + * @param chatType the type resolved by [planConversationStyling], null for a non-chat push + */ internal fun Context.buildContentIntent( - metadata: PushChatMetadata?, + chatType: ChatType?, navigation: NavigationTrigger?, ): PendingIntent { - val target = when (navigation) { - is NavigationTrigger.CurrencyInfo -> Intent(Intent.ACTION_VIEW).apply { - data = Linkify.tokenInfo(navigation.mint).toUri() - } - - // Only tip DMs deep-link (via /tip/chat/…). Non-tip chat notifications — and all - // contact/phone-addressed chats — no longer have an in-app entry point (the Send - // tab / direct-send flow was removed), so fall through to a plain launch that opens - // the app on the camera instead of firing a now-unhandled /chat/ deeplink. - is NavigationTrigger.Chat.ById -> if (metadata?.chatType == ChatType.TIP_DM) { + // The request code travels with the target: `FLAG_UPDATE_CURRENT` rewrites the extras of + // whichever PendingIntent is already held under it, and `filterEquals` — which decides + // what "already held" means — ignores extras. Under one shared code every posted + // notification ends up pointing at whatever was notified last. + val (target, requestCode) = when (navigation) { + is NavigationTrigger.CurrencyInfo -> Intent(Intent.ACTION_VIEW).apply { - data = Linkify.tipChatById(navigation.chatId).toUri() + data = Linkify.tokenInfo(navigation.mint).toUri() + } to navigation.mint.hashCode() + + is NavigationTrigger.Chat.ById -> { + // /tip/chat/… is named for where it was first used, but it resolves to the Chats + // tab and the conversation by id, which is the destination for a group too. + val intent = when (planChatTapTarget(chatType)) { + ChatTapTarget.Conversation -> Intent(Intent.ACTION_VIEW).apply { + data = Linkify.tipChatById(navigation.chatId).toUri() + } + + ChatTapTarget.AppLauncher -> packageManager.getLaunchIntentForPackage(packageName) } - } else { - packageManager.getLaunchIntentForPackage(packageName) + // The same code the reply action uses, which is not a collision: a PendingIntent + // is identified by its type too, and that one is a broadcast. + intent to navigation.chatId.hashCode() } - else -> packageManager.getLaunchIntentForPackage(packageName) + // Every one of these is the same launcher intent, so they can share a code. + else -> packageManager.getLaunchIntentForPackage(packageName) to LAUNCH_REQUEST_CODE } return PendingIntent.getActivity( this, - 99, + requestCode, target, PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE ) diff --git a/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ChatTapTargetPlannerTest.kt b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ChatTapTargetPlannerTest.kt new file mode 100644 index 0000000000..9c03060c61 --- /dev/null +++ b/apps/flipcash/shared/notifications/src/test/kotlin/com/flipcash/app/notifications/ChatTapTargetPlannerTest.kt @@ -0,0 +1,38 @@ +package com.flipcash.app.notifications + +import com.flipcash.services.models.chat.ChatType +import kotlin.test.Test +import kotlin.test.assertEquals + +class ChatTapTargetPlannerTest { + + // region Chats the app can open + + @Test + fun `a group push opens the conversation`() { + assertEquals(ChatTapTarget.Conversation, planChatTapTarget(ChatType.GROUP)) + } + + @Test + fun `a tip dm push opens the conversation`() { + assertEquals(ChatTapTarget.Conversation, planChatTapTarget(ChatType.TIP_DM)) + } + + // endregion + + // region Chats the app has no screen for + + @Test + fun `a contact dm push launches the app`() { + // The Send tab it used to open was removed, and AppRouter routes nothing for it. + assertEquals(ChatTapTarget.AppLauncher, planChatTapTarget(ChatType.CONTACT_DM)) + } + + @Test + fun `an unresolved type launches the app`() { + assertEquals(ChatTapTarget.AppLauncher, planChatTapTarget(ChatType.UNKNOWN)) + assertEquals(ChatTapTarget.AppLauncher, planChatTapTarget(null)) + } + + // endregion +}