Skip to content

Commit f51c90c

Browse files
AndyScherzingerAndyScherzinger
committed
fix(conversations): guard optimistic list actions against stale syncs
The conversation list actions - mark as read/unread (menu and swipe), add to or remove from favorites, and tag assignment - write the local conversation entry optimistically and revert on server failure, but had no protection against the same race the read marker had: a room list sync (or single-room refresh) whose response was computed before the action reached the server reverts the entry to the old state until the next sync, flipping badges and jumping favorite-sorted rows. Generalize the pending read marker mechanism in ConversationListUpdater to these actions: each optimistic write registers a pending change, and while the server response provably doesn't reflect it yet, the local state is kept in the merge. Once the server confirms the change it is released and the server state applies unchanged - so changes made on other devices keep flowing - and a failed action releases its pending change when reverting, restoring the server's authority. Mark-as-read reuses the pending read marker itself; mark-as-unread, favorites and tags get their own pending state with value-based confirmation. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
1 parent 3f71b0d commit f51c90c

5 files changed

Lines changed: 204 additions & 31 deletions

File tree

‎app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt‎

Lines changed: 122 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import javax.inject.Inject
2727
* responses from reverting that read state. The room list sync remains the authority and
2828
* self-healing safeguard for everything else.
2929
*/
30+
@Suppress("TooManyFunctions")
3031
class ConversationListUpdater @Inject constructor(
3132
private val chatDao: ChatMessagesDao,
3233
private val chatBlocksDao: ChatBlocksDao,
@@ -45,11 +46,59 @@ class ConversationListUpdater @Inject constructor(
4546
*/
4647
private val pendingReadMarkers = ConcurrentHashMap<String, Int>()
4748

49+
/**
50+
* Favorite flags applied locally but not yet confirmed by a server response; guarded in the
51+
* merge like the pending read markers.
52+
*/
53+
private val pendingFavorites = ConcurrentHashMap<String, Boolean>()
54+
55+
/**
56+
* Rooms marked as unread from the conversation list whose server state doesn't reflect it yet.
57+
*/
58+
private val pendingUnreadFlags = ConcurrentHashMap<String, Boolean>()
59+
60+
/**
61+
* Tag assignments applied locally but not yet confirmed by a server response.
62+
*/
63+
private val pendingTagIds = ConcurrentHashMap<String, List<String>>()
64+
4865
/**
4966
* The locally written but not yet server-confirmed read marker of the conversation, or null.
5067
*/
5168
fun pendingReadMarker(internalConversationId: String): Int? = pendingReadMarkers[internalConversationId]
5269

70+
/**
71+
* Registers a read marker sent from the conversation list (mark as read), so the merge guards
72+
* it exactly like a marker sent from an open chat.
73+
*/
74+
fun markPendingReadMarker(internalConversationId: String, lastReadMessage: Int) {
75+
pendingReadMarkers[internalConversationId] = lastReadMessage
76+
}
77+
78+
fun markPendingFavorite(internalConversationId: String, favorite: Boolean) {
79+
pendingFavorites[internalConversationId] = favorite
80+
}
81+
82+
fun clearPendingFavorite(internalConversationId: String, favorite: Boolean) {
83+
pendingFavorites.remove(internalConversationId, favorite)
84+
}
85+
86+
fun markPendingUnread(internalConversationId: String) {
87+
pendingUnreadFlags[internalConversationId] = true
88+
}
89+
90+
fun clearPendingUnread(internalConversationId: String) {
91+
pendingUnreadFlags.remove(internalConversationId)
92+
}
93+
94+
fun markPendingTags(internalConversationId: String, tagIds: List<String>) {
95+
pendingTagIds[internalConversationId] = tagIds
96+
}
97+
98+
fun clearPendingTags(internalConversationId: String, tagIds: List<String>) {
99+
pendingTagIds.remove(internalConversationId, tagIds)
100+
}
101+
53102
/**
54103
* Releases a pending read marker: called when a room list sync confirmed it or when sending it
55104
* ultimately failed. Only removes [lastReadMessage] itself, so a newer marker written in the
@@ -80,40 +129,88 @@ class ConversationListUpdater @Inject constructor(
80129
}
81130

82131
/**
83-
* Keeps provably stale read states out of the merge of server responses (the room list sync
84-
* and the single-room refresh): a response computed before a concurrently sent read marker
85-
* reached the server still reports the room as unread, and applying it would revert the
86-
* conversation entry until the next sync. While a marker for the room is pending and the
87-
* server's read state is still behind it, the local read state is kept; once the server has
88-
* caught up (or moved past it, e.g. read further on another device) the marker is released
89-
* and the server state applies unchanged — including a lower one, so marking as unread from
90-
* another device keeps working.
132+
* Keeps provably stale local-action state out of the merge of server responses (the room
133+
* list sync and the single-room refresh): a response computed before a concurrently sent
134+
* change — read marker, mark as unread, favorite flag, tag assignment — reached the server
135+
* still reports the old state, and applying it would revert the conversation entry until the
136+
* next sync. While a change for the room is pending and the server hasn't reflected it yet,
137+
* the local state is kept; once the server confirms it (or moved past it, e.g. read further
138+
* or unfavorited on another device after confirmation) the pending change is released and
139+
* the server state applies unchanged, so the server stays the authority.
91140
*/
92-
fun preserveReadStateOfPendingMarkers(
141+
fun preservePendingLocalState(
93142
previousConversations: Map<String, ConversationEntity>,
94143
conversationsFromServer: List<ConversationEntity>
95144
): List<ConversationEntity> =
96145
conversationsFromServer.map { serverItem ->
97-
val pendingMarker = pendingReadMarker(serverItem.internalId)
98-
?: return@map serverItem
99146
val previous = previousConversations[serverItem.internalId]
100147
?: return@map serverItem
148+
var guarded = guardPendingReadMarker(serverItem, previous)
149+
guarded = guardPendingUnread(guarded, previous)
150+
guarded = guardPendingFavorite(guarded, previous)
151+
guarded = guardPendingTags(guarded, previous)
152+
guarded
153+
}
154+
155+
private fun guardPendingReadMarker(
156+
serverItem: ConversationEntity,
157+
previous: ConversationEntity
158+
): ConversationEntity {
159+
val pendingMarker = pendingReadMarker(serverItem.internalId) ?: return serverItem
160+
161+
return if (serverItem.lastReadMessage < pendingMarker) {
162+
Log.d(
163+
TAG,
164+
"Keeping local read state for room ${serverItem.token}: server lastRead=" +
165+
"${serverItem.lastReadMessage} is behind the pending read marker $pendingMarker"
166+
)
167+
serverItem.copy(
168+
lastReadMessage = previous.lastReadMessage,
169+
unreadMessages = previous.unreadMessages
170+
)
171+
} else {
172+
clearPendingReadMarker(serverItem.internalId, pendingMarker)
173+
serverItem
174+
}
175+
}
176+
177+
private fun guardPendingUnread(serverItem: ConversationEntity, previous: ConversationEntity): ConversationEntity {
178+
if (!pendingUnreadFlags.containsKey(serverItem.internalId)) {
179+
return serverItem
180+
}
181+
182+
return if (serverItem.unreadMessages > 0) {
183+
clearPendingUnread(serverItem.internalId)
184+
serverItem
185+
} else {
186+
serverItem.copy(
187+
lastReadMessage = previous.lastReadMessage,
188+
unreadMessages = previous.unreadMessages
189+
)
190+
}
191+
}
192+
193+
private fun guardPendingFavorite(serverItem: ConversationEntity, previous: ConversationEntity): ConversationEntity {
194+
val desired = pendingFavorites[serverItem.internalId] ?: return serverItem
195+
196+
return if (serverItem.favorite == desired) {
197+
clearPendingFavorite(serverItem.internalId, desired)
198+
serverItem
199+
} else {
200+
serverItem.copy(favorite = previous.favorite)
201+
}
202+
}
101203

102-
if (serverItem.lastReadMessage < pendingMarker) {
103-
Log.d(
104-
TAG,
105-
"Keeping local read state for room ${serverItem.token}: server lastRead=" +
106-
"${serverItem.lastReadMessage} is behind the pending read marker $pendingMarker"
107-
)
108-
serverItem.copy(
109-
lastReadMessage = previous.lastReadMessage,
110-
unreadMessages = previous.unreadMessages
111-
)
112-
} else {
113-
clearPendingReadMarker(serverItem.internalId, pendingMarker)
114-
serverItem
115-
}
204+
private fun guardPendingTags(serverItem: ConversationEntity, previous: ConversationEntity): ConversationEntity {
205+
val desired = pendingTagIds[serverItem.internalId] ?: return serverItem
206+
207+
return if (serverItem.tagIds == desired) {
208+
clearPendingTags(serverItem.internalId, desired)
209+
serverItem
210+
} else {
211+
serverItem.copy(tagIds = previous.tagIds)
116212
}
213+
}
117214

118215
/**
119216
* Reflects a background catch-up in the conversation list: the room's cached conversation

‎app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ class OfflineFirstConversationsRepository @Inject constructor(
144144
model.hiddenUpcomingEvent = existingEntity?.hiddenUpcomingEvent
145145
_conversationFlow.emit(model)
146146
val previous = existingEntity?.let { mapOf(it.internalId to it) }.orEmpty()
147-
val entityList = conversationListUpdater.preserveReadStateOfPendingMarkers(
147+
val entityList = conversationListUpdater.preservePendingLocalState(
148148
previous,
149149
listOf(model.asEntity())
150150
)
@@ -190,7 +190,7 @@ class OfflineFirstConversationsRepository @Inject constructor(
190190

191191
dao.syncConversationsForUser(
192192
accountId = user.id!!,
193-
serverItems = conversationListUpdater.preserveReadStateOfPendingMarkers(
193+
serverItems = conversationListUpdater.preservePendingLocalState(
194194
previousConversations,
195195
conversationsFromSync
196196
),

‎app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import com.nextcloud.talk.R
1616
import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager
1717
import com.nextcloud.talk.contacts.ContactsRepository
1818
import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository
19+
import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater
1920
import com.nextcloud.talk.conversationlist.ui.ConversationListEntry
2021
import com.nextcloud.talk.data.user.model.User
2122
import com.nextcloud.talk.invitation.data.InvitationsModel
@@ -77,7 +78,8 @@ class ConversationsListViewModel @Inject constructor(
7778
private val invitationsRepository: InvitationsRepository,
7879
private val arbitraryStorageManager: ArbitraryStorageManager,
7980
var userManager: UserManager,
80-
private val conversationsRepository: ConversationsRepository
81+
private val conversationsRepository: ConversationsRepository,
82+
private val conversationListUpdater: ConversationListUpdater
8183
) : ViewModel() {
8284

8385
private val _currentUser = currentUserProvider.currentUser.blockingGet()
@@ -714,6 +716,7 @@ class ConversationsListViewModel @Inject constructor(
714716
)
715717
val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token)
716718
viewModelScope.launch {
719+
messageId?.let { conversationListUpdater.markPendingReadMarker(conversation.internalId, it) }
717720
withContext(Dispatchers.IO) {
718721
repository.updateConversation(optimistic)
719722
}
@@ -723,6 +726,7 @@ class ConversationsListViewModel @Inject constructor(
723726
}
724727
_readUnreadState.value = ConversationReadUnreadUiState.Success
725728
} catch (e: Exception) {
729+
messageId?.let { conversationListUpdater.clearPendingReadMarker(conversation.internalId, it) }
726730
withContext(Dispatchers.IO) {
727731
repository.updateConversation(original)
728732
}
@@ -741,6 +745,7 @@ class ConversationsListViewModel @Inject constructor(
741745
)
742746
val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token)
743747
viewModelScope.launch {
748+
conversationListUpdater.markPendingUnread(conversation.internalId)
744749
withContext(Dispatchers.IO) {
745750
repository.updateConversation(optimistic)
746751
}
@@ -750,6 +755,7 @@ class ConversationsListViewModel @Inject constructor(
750755
}
751756
_readUnreadState.value = ConversationReadUnreadUiState.Success
752757
} catch (e: Exception) {
758+
conversationListUpdater.clearPendingUnread(conversation.internalId)
753759
withContext(Dispatchers.IO) {
754760
repository.updateConversation(original)
755761
}
@@ -769,6 +775,7 @@ class ConversationsListViewModel @Inject constructor(
769775
val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1))
770776
val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token)
771777
viewModelScope.launch {
778+
conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = true)
772779
withContext(Dispatchers.IO) {
773780
repository.updateConversation(optimistic)
774781
}
@@ -778,6 +785,7 @@ class ConversationsListViewModel @Inject constructor(
778785
}
779786
_favoriteState.value = FavoriteUiState.Success
780787
} catch (e: Exception) {
788+
conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = true)
781789
withContext(Dispatchers.IO) {
782790
repository.updateConversation(original)
783791
}
@@ -793,6 +801,7 @@ class ConversationsListViewModel @Inject constructor(
793801
val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1))
794802
val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token)
795803
viewModelScope.launch {
804+
conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = false)
796805
withContext(Dispatchers.IO) {
797806
repository.updateConversation(optimistic)
798807
}
@@ -802,6 +811,7 @@ class ConversationsListViewModel @Inject constructor(
802811
}
803812
_favoriteState.value = FavoriteUiState.Success
804813
} catch (e: Exception) {
814+
conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = false)
805815
withContext(Dispatchers.IO) {
806816
repository.updateConversation(original)
807817
}

‎app/src/main/java/com/nextcloud/talk/conversationtags/viewmodels/ConversationTagsViewModel.kt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import androidx.lifecycle.ViewModel
1212
import androidx.lifecycle.viewModelScope
1313
import com.bluelinelabs.logansquare.LoganSquare
1414
import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository
15+
import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater
1516
import com.nextcloud.talk.conversationtags.data.ConversationTagsRepository
1617
import com.nextcloud.talk.data.user.model.User
1718
import com.nextcloud.talk.models.domain.ConversationModel
@@ -34,7 +35,8 @@ import javax.inject.Inject
3435
class ConversationTagsViewModel @Inject constructor(
3536
private val conversationTagsRepository: ConversationTagsRepository,
3637
private val repository: OfflineConversationsRepository,
37-
private val currentUserProvider: CurrentUserProviderOld
38+
private val currentUserProvider: CurrentUserProviderOld,
39+
private val conversationListUpdater: ConversationListUpdater
3840
) : ViewModel() {
3941

4042
private val currentUser: User = currentUserProvider.currentUser.blockingGet()
@@ -164,6 +166,7 @@ class ConversationTagsViewModel @Inject constructor(
164166
val optimistic = conversation.copy(tagIds = tagIds)
165167
replaceConversationForTagAssignment(conversation.token, optimistic)
166168
viewModelScope.launch {
169+
conversationListUpdater.markPendingTags(conversation.internalId, tagIds)
167170
withContext(Dispatchers.IO) {
168171
repository.updateConversation(optimistic)
169172
}
@@ -177,6 +180,7 @@ class ConversationTagsViewModel @Inject constructor(
177180
)
178181
}
179182
} catch (e: Exception) {
183+
conversationListUpdater.clearPendingTags(conversation.internalId, tagIds)
180184
replaceConversationForTagAssignment(conversation.token, original)
181185
withContext(Dispatchers.IO) {
182186
repository.updateConversation(original)

0 commit comments

Comments
 (0)