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`() = 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/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 371f6ab4a5..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 @@ -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 @@ -47,6 +48,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 @@ -68,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. @@ -114,6 +123,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 @@ -248,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) @@ -256,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) + 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) @@ -285,43 +307,63 @@ class NotificationService : FirebaseMessagingService(), private suspend fun NotificationCompat.Builder.applyChatStyle( chatId: ChatId, + styling: ConversationStyling, 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 (!styling.isGroupConversation) { + 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 = styling.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 +371,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=${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, ) 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())) } @@ -351,6 +399,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)) @@ -359,6 +414,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() @@ -496,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/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/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 +} 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/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 +} 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