Skip to content

Commit 802c6b0

Browse files
authored
give image cluster priority over the emoji for group conversations (#1140)
* give image cluster priority over the emoji for group conversations * make sure all clusters update in sync on profile updates, efficiently
1 parent 68761a5 commit 802c6b0

4 files changed

Lines changed: 239 additions & 7 deletions

File tree

Convos/Conversations List/View Controller/ConversationsViewController.swift

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,19 @@ final class ConversationsViewController: UIViewController {
171171
changed.insert(id)
172172
continue
173173
}
174+
// `avatarType` encodes the rendered avatar (the clustered member
175+
// profiles and their avatar URLs), so comparing it reconfigures the
176+
// cell when a member changes their photo. Without this, a member
177+
// avatar update re-emits a fresh conversation but no tracked field
178+
// here changes, so every group showing that member keeps a stale
179+
// cluster until something else about the row changes.
174180
if oldConvo.isMuted != newConvo.isMuted ||
175181
oldConvo.isUnread != newConvo.isUnread ||
176182
oldConvo.isPinned != newConvo.isPinned ||
177183
oldConvo.scheduledExplosionDate != newConvo.scheduledExplosionDate ||
178184
oldConvo.displayName != newConvo.displayName ||
179-
oldConvo.lastMessage != newConvo.lastMessage {
185+
oldConvo.lastMessage != newConvo.lastMessage ||
186+
oldConvo.avatarType != newConvo.avatarType {
180187
changed.insert(id)
181188
}
182189
}

ConvosCore/Sources/ConvosCore/Storage/Models/Conversation.swift

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -245,14 +245,16 @@ public extension Conversation {
245245
if otherMembers.count == 1, let member = otherMembers.first {
246246
return .profile(member.profile, member.agentVerification)
247247
}
248-
if let conversationEmoji, !conversationEmoji.isEmpty {
249-
return .emoji(conversationEmoji)
250-
}
248+
// A group whose members have avatars renders the combined cluster of
249+
// their photos. This is checked before the conversation emoji because
250+
// that emoji is only ever an auto-seeded fallback (there is no UI to
251+
// choose it), so it should show only when there is nothing better - not
252+
// suppress the member cluster.
251253
let otherProfiles = otherMembers.map(\.profile)
252-
if otherProfiles.isEmpty || !otherProfiles.hasAnyAvatar {
253-
return .emoji(defaultEmoji)
254+
if !otherProfiles.isEmpty, otherProfiles.hasAnyAvatar {
255+
return .clustered(Array(otherProfiles.sortedForCluster().prefix(7)))
254256
}
255-
return .clustered(Array(otherProfiles.sortedForCluster().prefix(7)))
257+
return .emoji(defaultEmoji)
256258
}
257259

258260
var memberNamesString: String {
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
@testable import ConvosCore
2+
import Combine
3+
import Foundation
4+
import GRDB
5+
import Testing
6+
7+
/// Determines whether the conversation-list `ValueObservation` re-fires when a
8+
/// member's canonical `profileAvatar` changes.
9+
///
10+
/// The list joins each member's avatar through the `profileAvatarLatest` *view*
11+
/// (`DBConversationMember.avatarSlot`), fetched via an `.including(all:)`
12+
/// prefetch. Whether GRDB's region tracking reaches the underlying `profileAvatar`
13+
/// table through that view is the open question behind the "stale cluster until
14+
/// you open the conversation" bug:
15+
///
16+
/// - If `reEmitsOnMemberProfileAvatarWrite` passes, the data layer is already
17+
/// reactive and the stale-cluster bug is a UI cell-reload problem (fix the
18+
/// list/diffable-data-source, not the observation).
19+
/// - If it fails while the `reEmitsOnConversationChange` control passes, the
20+
/// observation is not tracking `profileAvatar` through the view and needs an
21+
/// explicit tracked region.
22+
@Suite("Conversation list observation - profileAvatar reactivity", .serialized)
23+
struct ConversationsRepositoryAvatarObservationTests {
24+
private let salt = Data(repeating: 1, count: 32)
25+
private let nonce = Data(repeating: 2, count: 12)
26+
private let key = Data(repeating: 3, count: 32)
27+
28+
private func seedGroup(_ db: Database) throws {
29+
try DBMember(inboxId: "me").save(db, onConflict: .ignore)
30+
try DBInbox(inboxId: "me", clientId: "client-me", createdAt: Date()).save(db, onConflict: .ignore)
31+
try DBMember(inboxId: "alice").save(db, onConflict: .ignore)
32+
33+
try DBConversation(
34+
id: "c1",
35+
clientConversationId: "client-c1",
36+
inviteTag: "tag-c1",
37+
creatorId: "me",
38+
kind: .group,
39+
consent: .allowed,
40+
createdAt: Date(),
41+
name: nil,
42+
description: nil,
43+
imageURLString: nil,
44+
publicImageURLString: nil,
45+
includeInfoInPublicPreview: false,
46+
expiresAt: nil,
47+
debugInfo: .empty,
48+
isLocked: false,
49+
imageSalt: nil,
50+
imageNonce: nil,
51+
imageEncryptionKey: nil,
52+
conversationEmoji: nil,
53+
imageLastRenewed: nil,
54+
isUnused: false,
55+
hasHadVerifiedAgent: false
56+
).insert(db)
57+
58+
try ConversationLocalState(
59+
conversationId: "c1",
60+
isPinned: false,
61+
isUnread: false,
62+
isUnreadUpdatedAt: Date(),
63+
isMuted: false,
64+
pinnedOrder: nil,
65+
hidesInviteCard: false,
66+
leftHostedInviteSession: false,
67+
wasRemoved: false,
68+
hasHadOtherMembers: false,
69+
hasSharedInvite: false
70+
).insert(db)
71+
72+
for (inboxId, role) in [("me", MemberRole.superAdmin), ("alice", MemberRole.member)] {
73+
try DBConversationMember(
74+
conversationId: "c1",
75+
inboxId: inboxId,
76+
role: role,
77+
consent: .allowed,
78+
createdAt: Date(),
79+
invitedByInboxId: nil
80+
).insert(db)
81+
}
82+
try DBProfile(
83+
inboxId: "alice", name: "Alice", profileSource: .profileUpdate,
84+
updatedAt: Date(timeIntervalSince1970: 1)
85+
).save(db)
86+
}
87+
88+
@Test("re-emits when a member's profileAvatar is written (through the profileAvatarLatest view)")
89+
func reEmitsOnMemberProfileAvatarWrite() async throws {
90+
let dbManager = MockDatabaseManager.makeTestDatabase()
91+
let writer = dbManager.dbWriter
92+
try await writer.write { db in try self.seedGroup(db) }
93+
94+
let counter = EmissionCounter()
95+
let repo = ConversationsRepository(dbReader: writer, consent: [.allowed])
96+
let cancellable = repo.conversationsPublisher.sink { _ in counter.increment() }
97+
defer { cancellable.cancel() }
98+
99+
try await Task.sleep(nanoseconds: 300_000_000)
100+
let afterInitial = counter.count
101+
#expect(afterInitial >= 1, "Observation should deliver an initial value")
102+
103+
// Simulate Alice updating her photo: a new canonical avatar row.
104+
try await writer.write { db in
105+
try DBProfileAvatar(
106+
inboxId: "alice", conversationId: "c1",
107+
url: "https://example.com/alice-new.bin",
108+
salt: self.salt, nonce: self.nonce, encryptionKey: self.key,
109+
profileSource: .profileUpdate, updatedAt: Date()
110+
).save(db)
111+
}
112+
113+
try await Task.sleep(nanoseconds: 800_000_000)
114+
#expect(
115+
counter.count > afterInitial,
116+
"Expected the conversation-list observation to re-emit when a member's profileAvatar changed"
117+
)
118+
}
119+
120+
@Test("control: re-emits when the conversation row itself changes")
121+
func reEmitsOnConversationChange() async throws {
122+
let dbManager = MockDatabaseManager.makeTestDatabase()
123+
let writer = dbManager.dbWriter
124+
try await writer.write { db in try self.seedGroup(db) }
125+
126+
let counter = EmissionCounter()
127+
let repo = ConversationsRepository(dbReader: writer, consent: [.allowed])
128+
let cancellable = repo.conversationsPublisher.sink { _ in counter.increment() }
129+
defer { cancellable.cancel() }
130+
131+
try await Task.sleep(nanoseconds: 300_000_000)
132+
let afterInitial = counter.count
133+
#expect(afterInitial >= 1, "Observation should deliver an initial value")
134+
135+
try await writer.write { db in
136+
guard let conversation = try DBConversation.fetchOne(db, key: "c1") else { return }
137+
try conversation.with(name: "Renamed").save(db)
138+
}
139+
140+
try await Task.sleep(nanoseconds: 800_000_000)
141+
#expect(
142+
counter.count > afterInitial,
143+
"Control failed: the observation did not re-emit even for a direct conversation change"
144+
)
145+
}
146+
}
147+
148+
private final class EmissionCounter: @unchecked Sendable {
149+
private let lock = NSLock()
150+
private var value = 0
151+
func increment() {
152+
lock.lock()
153+
value += 1
154+
lock.unlock()
155+
}
156+
var count: Int {
157+
lock.lock()
158+
defer { lock.unlock() }
159+
return value
160+
}
161+
}

ConvosCore/Tests/ConvosCoreTests/DefaultConversationDisplayTests.swift

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,68 @@ struct DefaultConversationDisplayTests {
547547
}
548548
}
549549

550+
@Test("avatarType clusters a group with member avatars even when a conversation emoji is seeded")
551+
func avatarTypeClustersOverSeededConversationEmoji() {
552+
// The conversation emoji is only ever an auto-seeded fallback (there is
553+
// no UI to choose it), so it must not suppress the member cluster - the
554+
// #686 introduced checking the emoji before the cluster, but then this decision
555+
// was reversed.
556+
let members = [
557+
ConversationMember.mock(isCurrentUser: true, name: "You"),
558+
ConversationMember(
559+
profile: Profile.mock(inboxId: "other1", name: "Alice"),
560+
role: .member,
561+
isCurrentUser: false
562+
),
563+
ConversationMember(
564+
profile: Profile.mock(inboxId: "other2", name: "Bob"),
565+
role: .member,
566+
isCurrentUser: false
567+
)
568+
]
569+
let conversation = Conversation(
570+
id: "test",
571+
clientConversationId: "client-test",
572+
creator: .mock(isCurrentUser: true),
573+
createdAt: Date(),
574+
consent: .allowed,
575+
kind: .group,
576+
name: nil,
577+
description: nil,
578+
members: members,
579+
otherMember: nil,
580+
messages: [],
581+
isPinned: false,
582+
isUnread: false,
583+
isMuted: false,
584+
pinnedOrder: nil,
585+
hidesInviteCard: false,
586+
leftHostedInviteSession: false,
587+
wasRemoved: false,
588+
lastMessage: nil,
589+
imageURL: nil,
590+
imageSalt: nil,
591+
imageNonce: nil,
592+
imageEncryptionKey: nil,
593+
conversationEmoji: "🦊",
594+
includeInfoInPublicPreview: false,
595+
isDraft: false,
596+
invite: nil,
597+
expiresAt: nil,
598+
debugInfo: .empty,
599+
isLocked: false,
600+
agentJoinStatus: nil,
601+
hasHadVerifiedAgent: false,
602+
wasCreatedFromAgentBuilder: false
603+
)
604+
605+
if case .clustered(let profiles) = conversation.avatarType {
606+
#expect(profiles.count == 2)
607+
} else {
608+
#expect(Bool(false), "Expected clustered avatar type - a seeded conversation emoji must not suppress the member cluster")
609+
}
610+
}
611+
550612
// MARK: - Member Name Limiting Tests
551613

552614
@Test("Four named profiles shows three and 1 other")

0 commit comments

Comments
 (0)