feat: Chat Bubbles - Show reactions - WPB-19515 #3466
Conversation
Test Results4 371 tests 4 344 ✅ 6m 51s ⏱️ Results for commit cc83a63. ♻️ This comment has been updated with latest results. |
| senderNameObserverProvider: SenderNameObserverProvider?, | ||
| reactionsObserverProvider: ReactionsObserverProvider? |
There was a problem hiding this comment.
question: I'm not sure why these are optional but maybe I'm missing something?
| public import Combine | ||
|
|
||
| public protocol ReactionsObserverProtocol { | ||
| var reactionsPublisher: AnyPublisher<ReactionsModel, Never>? { get } |
There was a problem hiding this comment.
question: Same question here. Why is the result optional?
| // also performs mapping of domain model which is just raw string | ||
| // to UI model which is Attributed string | ||
| package struct AnySenderNameObserverProvider: @unchecked Sendable { | ||
| package struct AnyObserverProvider: @unchecked Sendable { |
There was a problem hiding this comment.
suggestion: Rename ObserverProviderbecause:
- I think the fact that it is type erasing is kind of an implementation detail.
- I imagine
Anytype erasers to be very generic objects but this isn't.
| import Foundation | ||
| import WireMessagingDomain | ||
|
|
||
| class ReactionsViewModel: ObservableObject { |
| case exists(ReactionsModel) | ||
| } | ||
|
|
||
| @Published var state: State |
There was a problem hiding this comment.
nitpick: This could be @Published var state: ReactionsModel. It saves some boilerplate although if there are going to be more states the current version is certainly better.
| return | ||
| } | ||
|
|
||
| self?.reactionsPublisher = NSManagedObject.publisher(for: message, in: viewContext) |
There was a problem hiding this comment.
suggestion: Return the publisher and set self.reactionsPublisher outside the closure. That way reactionsPublisher can be a let.
|
|
||
| final class ReactionsObserver: ReactionsObserverProtocol { | ||
|
|
||
| var reactionsPublisher: AnyPublisher<ReactionsModel, Never>? |
There was a problem hiding this comment.
suggestion: I personally really don't like that this is optional as it this optionality then propagates everywhere. How about, if we can't create it because the message doesn't exist (which I guess is a bug), we assign a type erased https://developer.apple.com/documentation/combine/empty publisher. That way we can get rid of the optional.
|
This PR is stale because it has been open 30 days with no activity. Please update it or close it in case is not relevant anymore. |
3 participants
Issue
Added logic to show live changes in messages reactions
Testing
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-08-18.at.15.55.37.mp4