Improve the mark messages as read logic #1185

Andrii-Horishnii-Glia · 2025-01-17T20:15:04Z

What was solved?
New messages should be marked as read when:

We have new messages when the history loaded after a visitor opened the chat (transcript) screen or a visitor gets a new SC or Live Engagement message from an Operator; and
(Not ready) It is an ongoing secure conversation or an ongoing engagement on_end equals retain; and
The visitor has stayed on the chat (transcript) screen for at least 6 seconds after the new message; and
The leave current SC dialog has not shown for at least 6 seconds after the new message.

Release notes:

Feature
Ignore
Release notes (Is it clear from the description here?)
Migration guide (If changes are needed for integrator already using the SDK - what needs to be communicated? Add underneath please)

Additional info:

Is the feature sufficiently tested? All tests fixed? Necessary unit, acceptance, snapshots added? Check that at least new public classes & methods are covered with unit tests
Did you add logging beneficial for troubleshooting of customer issues?
Did you add new logging? We would like the logging between platforms to be similar. Refer to Logging from iOS SDKs → Things to consider for newly added logs in Confluence for more information.

Screenshots:

igorkravchenko

Nice! Left some comments and suggestions.
The problem I see at the moment is lack of injected Combine schedulers, this forces expectations to be used in unit tests. This is undesirable because this makes tests slow and flaky.

igorkravchenko · 2025-01-30T11:05:45Z

GliaWidgets/SecureConversations/ChatTranscript/SecureConversations.TranscriptModel.swift

+            .store(in: &markMessagesAsReadCancellables)
+    }
+
+    fileprivate func doMarkMessagesAsRead() {


Perhaps there's a slightly better name for the method:

Suggested change

fileprivate func doMarkMessagesAsRead() {

fileprivate func performMarkMessagesAsReadRequest() {

igorkravchenko · 2025-01-30T11:07:32Z

GliaWidgets/SecureConversations/ChatTranscript/SecureConversations.TranscriptModel.swift

@@ -801,6 +767,50 @@ extension SecureConversations.TranscriptModel {
    }
 }

+// MARK: Mark messages as read
+extension SecureConversations.TranscriptModel: ApplicationVisibilityTracker {
+    func markMessagesAsRead(with predicate: Bool = true) {


It would be nice to have this method documented to understand what it does and what parameter means.

igorkravchenko · 2025-01-30T11:09:33Z

GliaWidgets/SecureConversations/ChatTranscript/SecureConversations.TranscriptModel.swift

+
+            switch result {
+            case .success:
+                self.historySection.removeAll(where: {


Nowadays swift allows omitting self. if it was properly unwrapped (in this case by guard self) after being put to capture list as weak. This is also applicable to other places in code with similar pattern.

Suggested change

self.historySection.removeAll(where: {

historySection.removeAll(where: {

igorkravchenko · 2025-01-30T11:11:42Z

GliaWidgets/SecureConversations/ChatTranscript/SecureConversations.TranscriptModel.swift

+
+    fileprivate func doMarkMessagesAsRead() {
+        _ = environment.secureMarkMessagesAsRead { [weak self] result in
+            guard let self = self else { return }


Suggested change

guard let self = self else { return }

guard let self else { return }

igorkravchenko · 2025-01-30T11:15:38Z

GliaWidgets/Sources/Observable/ObservableValue.swift

@@ -24,6 +25,26 @@ class ObservableValue<T: Any> {
        observers.removeAll(where: { $0().0 === observer })
    }

+    func eraseToAnyPublisher() -> AnyPublisher<T, Never> {


Since eraseToAnyPublisher is method defined on Combine publishers, I would suggest to name this method differently to avoid ambiguity:

Suggested change

func eraseToAnyPublisher() -> AnyPublisher<T, Never> {

func toCombinePublisher() -> AnyPublisher<T, Never> {

igorkravchenko · 2025-01-30T11:47:02Z

GliaWidgets/Sources/ViewController/Common/State/ApplicationVisibilityTracker.swift

+                .map { _ in false }
+        )
+            .combineLatest(isViewControllerAppearPublisher)
+            .map { $0 && $1 }


Does it make sense to name these parameters for clarity?

igorkravchenko · 2025-01-30T11:49:08Z

GliaWidgets/Sources/ViewController/Common/State/ApplicationVisibilityTracker.swift

+            .map {
+                if $0 {
+                    return Just(true)
+                        .delay(for: resumeToForegroundDelay, scheduler: DispatchQueue.global())


Having hardcoded scheduler will likely affect unit testing. Is it possible to pass scheduler as dependency (or parameter)?

igorkravchenko · 2025-01-30T11:54:03Z

GliaWidgets/Sources/ViewController/Common/State/ApplicationVisibilityTracker.swift

+            .filter { $0 }
+            .first()
+            .map { _ in }
+            .receive(on: RunLoop.main)


Is is possible to avoid hardcoding how to receive value, by passing it as parameter? Also why RunLoop is used here, while on line 54 DispatchQueue is used?

igorkravchenko · 2025-01-30T12:00:20Z

GliaWidgets/Sources/ViewModel/Chat/ChatViewModel.swift

@@ -976,6 +983,26 @@ extension ChatViewModel {
    }
 }

+// MARK: Mark messages as read
+extension ChatViewModel: ApplicationVisibilityTracker {
+    func markMessagesAsRead(with predicate: Bool = true) {


I think comments for this method would be helpful.

igorkravchenko · 2025-01-30T12:05:23Z

GliaWidgetsTests/SecureConversations/ChatTranscript/TranscriptModelTests.swift

@@ -793,14 +799,21 @@ final class SecureConversationsTranscriptModelTests: XCTestCase {
        modelEnv.createEntryWidget = { _ in .mock() }
        let scheduler = CoreSdkClient.ReactiveSwift.TestScheduler()
        modelEnv.messagesWithUnreadCountLoaderScheduler = scheduler
+        modelEnv.uiApplication.applicationState = { .active }
+        let expectation = expectation(description: "Message marked as read")


It is probably not very obvious, but it our unit tests we try as hard as possible to avoid using expectations, because of their flakiness.
I understand that for this case, this necessity is caused by Combine code (a thread hop), but this has to be solved via Combine schedulers injection.

ykyivskyi-gl

Good job. Left some comments.

ykyivskyi-gl · 2025-01-30T11:48:08Z

GliaWidgets/SecureConversations/ChatTranscript/SecureConversations.TranscriptModel.swift

+            switch result {
+            case .success:
+                self.historySection.removeAll(where: {
+                    if case .unreadMessageDivider = $0.kind {


Could be just $0.kind == . unreadMessageDivider ?

ykyivskyi-gl · 2025-01-30T12:01:24Z

GliaWidgets/Sources/ViewController/Common/State/ApplicationVisibilityTracker.swift

+                }
+            }
+            .switchToLatest()
+            .filter { $0 }


Does it make sense to filter after .map { $0 && $1 } then next map may handle true only.

Pay attention, .map is not .filter.
.map { $0 && $1 } returns false in lots cases.

We need delay for true and no delay for false. Also, we need to cancel the delay if the new condition is false. So, the .filter fight after the .map will not work.

ykyivskyi-gl · 2025-01-30T12:12:28Z

GliaWidgets/Sources/ViewModel/Chat/ChatViewModel.swift

@@ -976,6 +983,26 @@ extension ChatViewModel {
    }
 }

+// MARK: Mark messages as read
+extension ChatViewModel: ApplicationVisibilityTracker {
+    func markMessagesAsRead(with predicate: Bool = true) {


May we add this method to shared protocol of ChatViewModel and SecureConversations.TranscriptModel and reuse it between them? I see that they do different thing on .sink so this action may be passed in closure.

The logic of those methods is not the same. A similar logic already moved to the ApplicationVisibilityTracker shared protocol.

Check if a visitor is on the chat screen before marking MOB-3953 fixup! Mark messages as read after 6 seconds

Andrii-Horishnii-Glia changed the base branch from master to feature/entry-widget-and-secure-conversations-v2 January 17, 2025 20:15

EgorovEI force-pushed the feature/entry-widget-and-secure-conversations-v2 branch from 86ce9ea to 1fa66b0 Compare January 24, 2025 09:40

EgorovEI requested a review from a team as a code owner January 24, 2025 09:40

EgorovEI force-pushed the MOB-3953_mark_as_read branch from e096124 to be42ee1 Compare January 24, 2025 10:20

Andrii-Horishnii-Glia marked this pull request as draft January 29, 2025 08:55

Andrii-Horishnii-Glia force-pushed the MOB-3953_mark_as_read branch from be42ee1 to 3549d06 Compare January 29, 2025 22:09

Andrii-Horishnii-Glia requested review from igorkravchenko, EgorovEI, rasmustautsglia and ykyivskyi-gl January 30, 2025 06:26

Andrii-Horishnii-Glia marked this pull request as ready for review January 30, 2025 06:27

Andrii-Horishnii-Glia force-pushed the MOB-3953_mark_as_read branch 2 times, most recently from 02fdc8e to 31e1085 Compare January 30, 2025 12:06

igorkravchenko reviewed Jan 30, 2025

View reviewed changes

ykyivskyi-gl reviewed Jan 30, 2025

View reviewed changes

Andrii-Horishnii-Glia added 2 commits January 31, 2025 13:18

Mark messages as read after 6 seconds

7b23030

Check if a visitor is on the chat screen before marking MOB-3953 fixup! Mark messages as read after 6 seconds

fixup! Mark messages as read after 6 seconds

6712da9

Andrii-Horishnii-Glia force-pushed the MOB-3953_mark_as_read branch from b413f9d to 6712da9 Compare January 31, 2025 11:19

fixup! Mark messages as read after 6 seconds

3af045a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the mark messages as read logic #1185

Improve the mark messages as read logic #1185

Andrii-Horishnii-Glia commented Jan 17, 2025

igorkravchenko left a comment

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

Andrii-Horishnii-Glia Jan 30, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

Andrii-Horishnii-Glia Jan 31, 2025

igorkravchenko Jan 30, 2025

igorkravchenko Jan 30, 2025

ykyivskyi-gl left a comment

ykyivskyi-gl Jan 30, 2025

ykyivskyi-gl Jan 30, 2025

Andrii-Horishnii-Glia Jan 31, 2025

Andrii-Horishnii-Glia Jan 31, 2025

ykyivskyi-gl Jan 30, 2025

Andrii-Horishnii-Glia Jan 31, 2025

	fileprivate func doMarkMessagesAsRead() {
	fileprivate func performMarkMessagesAsReadRequest() {

	self.historySection.removeAll(where: {
	historySection.removeAll(where: {

	guard let self = self else { return }
	guard let self else { return }

	func eraseToAnyPublisher() -> AnyPublisher<T, Never> {
	func toCombinePublisher() -> AnyPublisher<T, Never> {

Improve the mark messages as read logic #1185

Are you sure you want to change the base?

Improve the mark messages as read logic #1185

Conversation

Andrii-Horishnii-Glia commented Jan 17, 2025

igorkravchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykyivskyi-gl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment