feat: Chat bubbles datasource - WPB-19058 #3430

dmitrysimkin · 2025-08-06T08:54:41Z

WPB-19058 [iOS] Setup ConversationView new datasource

Issue

Created a skeleton for new conversation screen datasource, for now with mock data.

Key points:

Created ConversationMessagesViewModel as MainActor to serve the view on main thread
Created ConversationMessagesDataSource as actor so it automatically work on background thread as we need all messages calculations be offloaded from main thread
ConversationMessagesDataSource emits updates with NSDiffableDataSourceSnapshot in order to do as much as possible on background thread and give UI ready snapshot to just apply
Added initial models MessageModel and UserModel in domain layer
Created ChatBubbles preview app for easier development

Testing

Run ChatBubbles preview app (new scheme created)

Checklist

Title contains a reference JIRA issue number like [WPB-XXX].
Description is filled and free of optional paragraphs.
Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

Make sure you use the API for UI elements that support large fonts.
All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
New UI elements have Accessibility strings for VoiceOver.

github-actions · 2025-08-06T09:03:54Z

Test Results

1 951 tests 1 924 ✅ 2m 24s ⏱️
321 suites 27 💤
2 files 0 ❌

Results for commit 748c81a.

♻️ This comment has been updated with latest results.

samwyndham

Nice work @dmitrysimkin! Right now I've added a few comments to think about.

samwyndham · 2025-08-07T07:49:16Z

WireMessaging/Sources/WireMessagingDomain/Conversation/MessageModel.swift

+        case text(TextMessageModel)
+    }
+
+    public let sender: UserModel


question: Do all messages have a sender? For example a system message?

True, it's optional in ZMMessage, will update in next PR

thought: We could consider that even system messages have a sender (the system), but i guess if we are using UserModel as a sender that's not possible (and out of scope). It will make MessageModel a bit more standard and reusable tho. Anyway, i was just a thought that cross my mind when reviewing this.

I think the model here shouldn't be concerned with messages, but rather cells. Cells are mostly for messages, but they can also be for other things like time dividers and even a loading cell at the top of the history. Perhaps we can do something like:

enum CellModel { case systemMessage(SystemMessageModel) case error(ErrorModel) case userMessage(UserMessageModel) case timeDivider(TimeDividerModel) ... }

In Domain it should be about business models. For example if in real work for a generic message can have no sender it should be optional. But for a presentation layer for a specific message type such as text message for instance it should be a sender, so in UI layer it would non-optional

samwyndham · 2025-08-07T07:59:01Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+import WireMessagingDomain
+
+public enum MessagesSection: Sendable {
+    // one section for now, later we'd have probably one section for a day


thought: I like the idea that eventually sections will be per day. I was wondering how to do this in a lazy efficient way using NSFetchedResultsController. We can't just group by serverTimestamp. I wonder it this could be done with a transient property 🤔. Anyway this is just me thinking and is out of scope for now

Or perhaps when we set the timestamp we can compute its day and store to make it easier for the fetched results controller to group.

samwyndham · 2025-08-07T08:05:08Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+    private var updatesStreamContinuation: AsyncStream<MessagesUpdate>.Continuation?
+    public func updatesStream() async -> AsyncStream<MessagesUpdate> {
+        AsyncStream { continuation in
+            self.updatesStreamContinuation = continuation


suggestion: Does this creates a strong reference cycle? Anyway there is now a better way (in my opinion) of creating streams without closures:

let (stream, continuation) = AsyncStream.makeStream(of:MessagesUpdate.self) updatesStreamContinuation = continuation return stream

I'll check, tnx for pointing it out. and I'll improve without closure

It was indeed a retain cycle, fixed in #3439

samwyndham · 2025-08-07T08:09:08Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+/// Does all calculations in background
+public actor ConversationMessagesDataSource: @preconcurrency ConversationMessagesDataSourceProtocol {
+
+    // AsyncStream because Combine's AnyPublisher is not Sendable


thought: One thing to be careful of with AsyncStream is that it each update can only be read once - this is fine if there is only a single consumer of updatesStream() but if there are multiple they will miss updates. Personally I think it might be better to use a publisher internally. I believe these can easily be converted to streams via the publishers values property. However behavior here needs to be confirmed.

I prefer publisher as well but it does not have Sendable conformance out of the box. I'll monitor its behaviour while i'd be working on it in next PRs and change/fix is there would be issues. Thanks for the concern

samwyndham · 2025-08-07T08:09:36Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+
+/// Actor to synchronise access to all that needed to conversation screen
+/// Does all calculations in background
+public actor ConversationMessagesDataSource: @preconcurrency ConversationMessagesDataSourceProtocol {


praise: actor is a good choice.

samwyndham · 2025-08-07T08:54:39Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+    func loadMessages(
+        near message: MessageModel,
+        forceRecalculate: Bool = false,
+    ) async -> IndexPath {
+        .init()
+    }
+
+    // load older messages
+    func loadOlderMessages() {}
+
+    // load newer messages
+    func loadNewerMessages() {}


thought: I'm wondering if paging is the right way to go. A properly configured NSFetchedResultsController doesn't need it and will do things lazily and efficiently. One reason where paging can be awkward can be if we want to jump around in history - e.g. a jump to start of conversation feature. This is of course doable but might be a bit more complex to implement. NSFetchedResultsController also does not require diffing but still emits NSDiffableDataSourceSnapshot

These two approaches will be require quite a different API though. The NSFetchedResultsController approach would be more similar to a classic UITableViewDataSource.

yeah Interesting to think about it. i just made same interface as we have in old datasource. Let me try find more effecient way working with NSFRC at later stages of implementation

@samwyndham are you suggesting to let the fetched results controller load all messages of the conversation? How does it do that efficiently if we only really will look at the last 50 unless we scroll back in time?

samwyndham · 2025-08-07T08:59:33Z

WireMessaging/Sources/WireMessagingUI/Conversation/MessageCollectionViewCell.swift

+import SwiftUI
+import UIKit
+
+class MessageCollectionViewCell: UICollectionViewCell {


suggestion: No reason to subclass. Use a regular UICollectionViewCell and configure it withe help methods. That way we also don't need the casting after dequeuing the cell.

Got rid of MessageCollectionViewCell in #3439

samwyndham · 2025-08-07T09:01:25Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesViewController.swift

+                    withReuseIdentifier: MessageCollectionViewCell.reuseIdentifier,
+                    for: indexPath
+                ) as? MessageCollectionViewCell else {
+                    return UICollectionViewCell()


suggestion: Crashing is preferable if we get a random cell type as we can't do anything with it.

No need to crash after using UICollectionviewCell

johnxnguyen · 2025-08-07T13:30:11Z

WireMessaging/Sources/WireMessagingDomain/Conversation/MessageModel.swift

+        case text(TextMessageModel)
+    }
+
+    public let sender: UserModel


I think the model here shouldn't be concerned with messages, but rather cells. Cells are mostly for messages, but they can also be for other things like time dividers and even a loading cell at the top of the history. Perhaps we can do something like:

enum CellModel { case systemMessage(SystemMessageModel) case error(ErrorModel) case userMessage(UserMessageModel) case timeDivider(TimeDividerModel) ... }

johnxnguyen · 2025-08-07T13:32:37Z

...ources/WireMessagingUI/Conversation/Message Views/TextMessageView/TextMessageViewModel.swift

+import SwiftUI
+public import Foundation
+
+public class TextMessageViewModel: ObservableObject, Identifiable, Hashable, @unchecked Sendable {


question: is Identifiable and Hashable necessary for diffable datasource?

Hashable constraint was added by UICollectioViewDiffableDataSource, but Identifiable looks like an artefact from my POC tries, I'll remove it

Removed Identifiable in #3439

johnxnguyen · 2025-08-07T13:34:16Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+import WireMessagingDomain
+
+public enum MessagesSection: Sendable {
+    // one section for now, later we'd have probably one section for a day


Or perhaps when we set the timestamp we can compute its day and store to make it easier for the fetched results controller to group.

johnxnguyen · 2025-08-07T13:38:01Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+public typealias MessagesSnapshot = NSDiffableDataSourceSnapshot<MessagesSection, MessageType>
+
+public protocol ConversationMessagesDataSourceProtocol: Sendable {
+    func updatesStream() async -> AsyncStream<MessagesUpdate>


suggestion: streamMessageUpdates

why stream at the beginning? usually it's prefix like ...Publisher or ...Subject and it's not singe message updates but plural messages updates. it feels to me still updatesStream might be better name, wdyt @johnxnguyen ?

johnxnguyen · 2025-08-07T13:41:14Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesDataSource.swift

+    func loadMessages(
+        near message: MessageModel,
+        forceRecalculate: Bool = false,
+    ) async -> IndexPath {
+        .init()
+    }
+
+    // load older messages
+    func loadOlderMessages() {}
+
+    // load newer messages
+    func loadNewerMessages() {}


@samwyndham are you suggesting to let the fetched results controller load all messages of the conversation? How does it do that efficiently if we only really will look at the last 50 unless we scroll back in time?

johnxnguyen · 2025-08-07T13:43:30Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesViewModel.swift

+
+@MainActor
+public protocol ConversationMessagesViewModelProtocol {
+    func updatesStream() async -> AsyncStream<MessagesUpdate>


suggestion: streamMessageUpdates

johnxnguyen · 2025-08-07T13:46:45Z

WireMessaging/Sources/WireMessagingUI/Conversation/MessageUpdateType.swift

+    // later to be added more updates like:
+    // loaded new messages, new or older
+    // re-sent failed message
+    // to be added other updates that happens to a conversation view


question: can you elaborate a bit more on this? For example, how was a failedMessage update be handled?

Similar to message added, if failed message resent successfully it will update internal data in datasource and will emit new snapshot, where new message will be moved from it's original place to the end of the conversation

For now I added different cases and they all (2) have just snapshot, so probably specific cases might not be need in the future but I'd think in UI we'd might need to introduce some side effect based on specific update case

johnxnguyen · 2025-08-07T13:49:51Z

WireMessaging/Sources/WireMessagingUI/Conversation/ConversationMessagesViewController.swift

+        let stream = await viewModel.updatesStream()
+        for await update in stream {
+            switch update {
+            case let .initiallyLoaded(snapshot):
+                await dataSource.apply(snapshot)
+            case let .messageAdded(snapshot):
+                await dataSource.apply(snapshot)
+            }
+        }


question: how would you envision this working with a SwiftUI list? Just wondering when the time comes, what changes we would make to connect the datasource.

We already have list of view models inside snapshot and in case of SwiftUI we'd just use list of view models. Or we can map steam to some other @published property of a state or smth like this

dmitrysimkin added 10 commits August 5, 2025 09:18

Added initial classes

3993646

Add preview app

fbbae89

Make VM be main actor

7564174

Added model and datasource

d5e5677

Code style

22b78bb

Added way to add messages

7563f13

Code style

45ce965

Generate with domain models and map to VMs

3f3270d

Format

a9aa4ae

Add more comments and improve code

a9cdbba

dmitrysimkin requested review from samwyndham, SyamalaHari, johnxnguyen, WilhelmOks, OlivellaO and a team August 6, 2025 08:54

github-actions bot assigned dmitrysimkin Aug 6, 2025

Add some more comments

378a48f

Fixed build error

748c81a

samwyndham reviewed Aug 7, 2025

View reviewed changes

johnxnguyen reviewed Aug 7, 2025

View reviewed changes

feat: Chat bubbles datasource - WPB-19058 #3430

Are you sure you want to change the base?

feat: Chat bubbles datasource - WPB-19058 #3430

Uh oh!

Conversation

dmitrysimkin commented Aug 6, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Testing

Checklist

UI accessibility checklist

Uh oh!

github-actions bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

samwyndham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrysimkin Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmitrysimkin commented Aug 6, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Aug 6, 2025 •

edited

Loading

dmitrysimkin Aug 7, 2025 •

edited

Loading