From a1879b2da08561243982b2798adda52379c7ed66 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:23:25 +0200 Subject: [PATCH 1/5] refactor(chat): extract the optimistic action pattern into one helper Four call sites had each grown their own copy of "apply locally, retry once on a transient failure, revert when it finally fails, revert when cancelled", with the retry predicate spelled out twice and one of them shipping a guard bug. optimisticAction() now carries that shape, including the case where the server refuses in the payload rather than in the status code, and the case where an answer means the change was already applied - which must not revert anything. No behaviour change; the repository tests of reactions, deletions, edits, pinning and dismissals carry the migration. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../network/OfflineFirstChatRepository.kt | 123 ++++++------------ .../reactions/ReactionsRepositoryImpl.kt | 110 ++++------------ .../nextcloud/talk/utils/OptimisticAction.kt | 69 ++++++++++ 3 files changed, 131 insertions(+), 171 deletions(-) create mode 100644 app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index deed6e0dbc..f3f0173ebd 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -32,8 +32,7 @@ import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.participants.Participant import com.nextcloud.talk.utils.bundle.BundleKeys import com.nextcloud.talk.utils.message.SendMessageUtils -import com.nextcloud.talk.utils.revertOnCancellation -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow @@ -802,31 +801,24 @@ class OfflineFirstChatRepository @Inject constructor( messageId: Long, text: String ): Result { - val restore = applyLocalEdit(messageId, text) + val result = optimisticAction( + apply = { applyLocalEdit(messageId, text) }, + isConfirmed = ::editAccepted, + request = { network.editChatMessage(credentials, url, text) } + ) - return try { - val response = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - network.editChatMessage(credentials, url, text) - } - } - val statusCode = response.ocs?.meta?.statusCode - if (statusCode != null && statusCode != HTTP_OK) { - // the server refused the edit, e.g. because the message is too old - restore?.invoke() - } else { - persistEditedMessage(messageId, response) - } - Result.success(response) - } catch (e: HttpException) { - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - restore?.invoke() - Result.failure(e) - } + result.getOrNull()?.takeIf(::editAccepted)?.let { persistEditedMessage(messageId, it) } + + return result } + /** + * A status code other than 200 means the server refused the edit, for instance because the + * message is too old, and the optimistic edit has to be taken back. + */ + private fun editAccepted(response: ChatOverallSingleMessage): Boolean = + response.ocs?.meta?.statusCode.let { it == null || it == HTTP_OK } + /** * Writes [text] into the cached message together with the edit metadata the bubble shows, and * returns the action that restores the previous version. The revert only applies while the message @@ -884,29 +876,19 @@ class OfflineFirstChatRepository @Inject constructor( url: String, messageId: Long, deletedPlaceholder: String - ): Result { - val restore = applyLocalDeletion(messageId, deletedPlaceholder) - - return try { - val response = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { + ): Result = + optimisticAction( + apply = { applyLocalDeletion(messageId, deletedPlaceholder) }, + request = { + try { network.deleteChatMessage(credentials, url) + } catch (e: HttpException) { + // the server does not know the message any more, so it is gone either way and the + // local deletion stands + if (e.code() == HTTP_NOT_FOUND) null else throw e } } - Result.success(response) - } catch (e: HttpException) { - if (e.code() == HTTP_NOT_FOUND) { - // the server does not know the message any more, so it is gone either way - Result.success(null) - } else { - restore?.invoke() - Result.failure(e) - } - } catch (e: IOException) { - restore?.invoke() - Result.failure(e) - } - } + ) /** * Renders the message as deleted in the local database and returns the action that puts it back, @@ -1006,12 +988,6 @@ class OfflineFirstChatRepository @Inject constructor( network.unPinMessage(credentials, url) } - private fun isRetryable(error: Exception): Boolean = - when (error) { - is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR - else -> error is IOException - } - private suspend fun withLocalPinnedMessage( messageId: Long, pinned: Boolean, @@ -1025,23 +1001,14 @@ class OfflineFirstChatRepository @Inject constructor( null } - return try { - val overall = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { request() } + return optimisticAction(apply = { restore }, request = request) + .onSuccess { + // the room refresh that follows is computed after this change reached the server, so + // the guard has done its job and must not outlive the request + conversationListUpdater.clearPendingPinnedMessage(internalConversationId) } - // the room refresh that follows is computed after this change reached the server, so the - // guard has done its job and must not outlive the request - conversationListUpdater.clearPendingPinnedMessage(internalConversationId) - Result.success(overall.ocs?.data?.toDomainModel()) - } catch (e: HttpException) { - Log.e(TAG, "Error while pinning or unpinning a message: $e") - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - Log.e(TAG, "Error while pinning or unpinning a message: $e") - restore?.invoke() - Result.failure(e) - } + .onFailure { Log.e(TAG, "Error while pinning or unpinning a message: $it") } + .map { it.ocs?.data?.toDomainModel() } } private fun isChatDataInitialized(): Boolean = @@ -1055,23 +1022,10 @@ class OfflineFirstChatRepository @Inject constructor( null } - return try { - revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - network.hidePinnedMessage(credentials, url) - } - } - conversationListUpdater.clearPendingHiddenPinnedMessage(internalConversationId) - Result.success(true) - } catch (e: HttpException) { - Log.e(TAG, "Error while hiding the pinned message: $e") - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - Log.e(TAG, "Error while hiding the pinned message: $e") - restore?.invoke() - Result.failure(e) - } + return optimisticAction(apply = { restore }, request = { network.hidePinnedMessage(credentials, url) }) + .onSuccess { conversationListUpdater.clearPendingHiddenPinnedMessage(internalConversationId) } + .onFailure { Log.e(TAG, "Error while hiding the pinned message: $it") } + .map { true } } override suspend fun onSignalingChatMessageReceived(chatMessages: List) { @@ -1286,8 +1240,5 @@ class OfflineFirstChatRepository @Inject constructor( private const val MESSAGE_TYPE_DELETED = "comment_deleted" private const val HTTP_OK = 200 private const val HTTP_NOT_FOUND = 404 - private const val HTTP_TOO_MANY_REQUESTS = 429 - private const val HTTP_INTERNAL_SERVER_ERROR = 500 - private const val RETRY_DELAY_MS = 500L } } diff --git a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt index 46c683df54..9a01933cc3 100644 --- a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt +++ b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt @@ -12,12 +12,10 @@ import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.data.database.dao.ChatMessagesDao import com.nextcloud.talk.models.domain.ReactionAddedModel import com.nextcloud.talk.models.domain.ReactionDeletedModel -import com.nextcloud.talk.utils.revertOnCancellation -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import retrofit2.HttpException -import java.io.IOException import javax.inject.Inject /** @@ -45,23 +43,14 @@ class ReactionsRepositoryImpl @Inject constructor( val internalConversationId = "$userId@$roomToken" val messageId = message.jsonMessageId.toLong() - val applied = applyLocalAdd(internalConversationId, messageId, emoji) - val revert: suspend () -> Unit = { - if (applied) applyLocalRemove(internalConversationId, messageId, emoji) - } - - val confirmed = revertOnCancellation(revert) { - requestSucceeds( - successStatusCodes = ADD_SUCCESS_CODES, - alreadyAppliedHttpCodes = emptySet() - ) { - ncApiCoroutines.sendReaction(credentials, url, emoji).ocs?.meta?.statusCode - } - } - - if (!confirmed) { - revert() - } + val confirmed = optimisticAction( + apply = { + val applied = applyLocalAdd(internalConversationId, messageId, emoji) + revertWith(applied) { applyLocalRemove(internalConversationId, messageId, emoji) } + }, + isConfirmed = { statusCode -> statusCode in ADD_SUCCESS_CODES }, + request = { ncApiCoroutines.sendReaction(credentials, url, emoji).ocs?.meta?.statusCode } + ).logFailure().getOrNull() in ADD_SUCCESS_CODES return ReactionAddedModel(message, emoji, confirmed) } @@ -77,77 +66,32 @@ class ReactionsRepositoryImpl @Inject constructor( val internalConversationId = "$userId@$roomToken" val messageId = message.jsonMessageId.toLong() - val applied = applyLocalRemove(internalConversationId, messageId, emoji) - val revert: suspend () -> Unit = { - if (applied) applyLocalAdd(internalConversationId, messageId, emoji) - } - - val confirmed = revertOnCancellation(revert) { - requestSucceeds( - successStatusCodes = DELETE_SUCCESS_CODES, - alreadyAppliedHttpCodes = DELETE_ALREADY_APPLIED_CODES - ) { - ncApiCoroutines.deleteReaction(credentials, url, emoji).ocs?.meta?.statusCode - } - } - - if (!confirmed) { - revert() - } + val confirmed = optimisticAction( + apply = { + val applied = applyLocalRemove(internalConversationId, messageId, emoji) + revertWith(applied) { applyLocalAdd(internalConversationId, messageId, emoji) } + }, + isConfirmed = { statusCode -> statusCode in DELETE_SUCCESS_CODES }, + request = { deleteReactionStatusCode(credentials, url, emoji) } + ).logFailure().getOrNull() in DELETE_SUCCESS_CODES return ReactionDeletedModel(message, emoji, confirmed) } - private suspend fun requestSucceeds( - successStatusCodes: Set, - alreadyAppliedHttpCodes: Set, - call: suspend () -> Int? - ): Boolean = - try { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - attemptRequest(successStatusCodes, alreadyAppliedHttpCodes, call) - } - } catch (e: IOException) { - Log.w(TAG, "Reaction request failed, the retry failed as well: $e") - false - } catch (e: HttpException) { - Log.w(TAG, "Reaction request failed with HTTP ${e.code()}, the retry failed as well: $e") - false - } - /** - * Returns whether the server applied the reaction, and throws for a failure that is worth another - * attempt: a connection problem, a rate limit or a server error. + * A reaction the server no longer knows is deleted as far as this call is concerned. */ - private suspend fun attemptRequest( - successStatusCodes: Set, - alreadyAppliedHttpCodes: Set, - call: suspend () -> Int? - ): Boolean = + private suspend fun deleteReactionStatusCode(credentials: String?, url: String, emoji: String): Int? = try { - val statusCode = call() - if (statusCode in successStatusCodes) { - true - } else { - Log.w(TAG, "Reaction request answered with unexpected status code $statusCode") - false - } + ncApiCoroutines.deleteReaction(credentials, url, emoji).ocs?.meta?.statusCode } catch (e: HttpException) { - when { - e.code() in alreadyAppliedHttpCodes -> true - isRetryable(e) -> throw e - else -> { - Log.w(TAG, "Reaction request rejected with HTTP ${e.code()}: $e") - false - } - } + if (e.code() == HTTP_NOT_FOUND) HTTP_OK else throw e } - private fun isRetryable(error: Exception): Boolean = - when (error) { - is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR - else -> error is IOException - } + private fun revertWith(applied: Boolean, revert: suspend () -> Unit): (suspend () -> Unit)? = + revert.takeIf { applied } + + private fun Result.logFailure(): Result = onFailure { Log.w(TAG, "Reaction request failed: $it") } /** * Adds the reaction to the cached message and reports whether that changed anything. A reaction the @@ -191,12 +135,8 @@ class ReactionsRepositoryImpl @Inject constructor( private const val HTTP_OK: Int = 200 private const val HTTP_CREATED: Int = 201 private const val HTTP_NOT_FOUND: Int = 404 - private const val HTTP_TOO_MANY_REQUESTS: Int = 429 - private const val HTTP_INTERNAL_SERVER_ERROR: Int = 500 - private const val RETRY_DELAY_MS: Long = 500 private val ADD_SUCCESS_CODES = setOf(HTTP_OK, HTTP_CREATED) private val DELETE_SUCCESS_CODES = setOf(HTTP_OK) - private val DELETE_ALREADY_APPLIED_CODES = setOf(HTTP_NOT_FOUND) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt new file mode 100644 index 0000000000..856d93d77f --- /dev/null +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -0,0 +1,69 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.utils + +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext +import retrofit2.HttpException +import java.io.IOException + +private const val HTTP_TOO_MANY_REQUESTS = 429 +private const val HTTP_INTERNAL_SERVER_ERROR = 500 +private const val RETRY_DELAY_MS = 500L + +/** + * Whether a failed request is worth another attempt: a connection problem, a rate limit or a server + * error. An answer the server actually gave - a refusal, a permission error - is not. + */ +fun isTransientFailure(error: Exception): Boolean = + when (error) { + is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR + else -> error is IOException + } + +/** + * Applies a change locally, has the server confirm it and undoes it when that fails, which is how + * every user action that the client can predict should behave: the screen reacts to the tap, the + * request follows, and only a request that finally fails takes the change back. + * + * [apply] writes the local state and returns the action that undoes it again, or null when there was + * nothing to change - in which case nothing is undone later either. [request] is retried once for a + * transient failure. [isConfirmed] decides whether an answer that did not throw still counts as a + * refusal, for endpoints that report one in the payload rather than in the status code. + * + * Cancellation is part of the contract: closing the screen while the request is in flight undoes the + * change as well, instead of leaving it applied although the server may never have heard of it. + */ +suspend fun optimisticAction( + apply: suspend () -> (suspend () -> Unit)?, + isConfirmed: (T) -> Boolean = { true }, + request: suspend () -> T +): Result { + val revert = apply() + // a revert that is itself cancelled would leave the change applied, which is the very thing it + // is there to prevent + val revertUninterruptibly: suspend () -> Unit = { withContext(NonCancellable) { revert?.invoke() } } + + return try { + val answer = revertOnCancellation(revertUninterruptibly) { + withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isTransientFailure) { + request() + } + } + if (!isConfirmed(answer)) { + revertUninterruptibly() + } + Result.success(answer) + } catch (e: HttpException) { + revertUninterruptibly() + Result.failure(e) + } catch (e: IOException) { + revertUninterruptibly() + Result.failure(e) + } +} From 9cc4d581f6160f9f65834e251232910d31069fdf Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:55:21 +0200 Subject: [PATCH 2/5] perf(conversations): keep list and info actions correct when they fail Marking a conversation read or unread, favoriting, archiving and the important and sensitive toggles now go through the shared optimistic helper, so every one of them retries a connection problem once and takes its change back when the screen is left mid-request. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../viewmodel/ConversationInfoViewModel.kt | 47 +++-- .../viewmodels/ConversationsListViewModel.kt | 177 +++++++++--------- .../nextcloud/talk/utils/OptimisticAction.kt | 12 +- .../ConversationInfoViewModelTest.kt | 28 +++ .../viewmodel/FakeConversationsRepository.kt | 14 +- 5 files changed, 169 insertions(+), 109 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index 33c23580c0..e75f1d8aef 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -50,6 +50,7 @@ import com.nextcloud.talk.utils.DateConstants import com.nextcloud.talk.utils.DisplayUtils import com.nextcloud.talk.utils.ParticipantRoleUtils import com.nextcloud.talk.utils.SpreedFeatures +import com.nextcloud.talk.utils.optimisticAction import com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule import io.reactivex.Observer import io.reactivex.android.schedulers.AndroidSchedulers @@ -857,18 +858,23 @@ class ConversationInfoViewModel @Inject constructor( fun toggleImportantConversation(credentials: String, baseUrl: String, roomToken: String) { val previousValue = _uiState.value.importantConversation val newValue = !previousValue - _uiState.update { it.copy(importantConversation = newValue) } + viewModelScope.launch { - try { - if (newValue) { - conversationsRepository.markConversationAsImportant(credentials, baseUrl, roomToken) - } else { - conversationsRepository.markConversationAsUnImportant(credentials, baseUrl, roomToken) + optimisticAction( + apply = { + _uiState.update { it.copy(importantConversation = newValue) } + suspend { _uiState.update { it.copy(importantConversation = previousValue) } } + }, + request = { + if (newValue) { + conversationsRepository.markConversationAsImportant(credentials, baseUrl, roomToken) + } else { + conversationsRepository.markConversationAsUnImportant(credentials, baseUrl, roomToken) + } } - } catch (exception: Exception) { - _uiState.update { it.copy(importantConversation = previousValue) } + ).onFailure { throwable -> _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) - Log.e(TAG, "failed to toggle important conversation state", exception) + Log.e(TAG, "failed to toggle important conversation state", throwable) } } } @@ -877,18 +883,23 @@ class ConversationInfoViewModel @Inject constructor( fun toggleSensitiveConversation(credentials: String, baseUrl: String, roomToken: String) { val previousValue = _uiState.value.sensitiveConversation val newValue = !previousValue - _uiState.update { it.copy(sensitiveConversation = newValue) } + viewModelScope.launch { - try { - if (newValue) { - conversationsRepository.markConversationAsSensitive(credentials, baseUrl, roomToken) - } else { - conversationsRepository.markConversationAsInsensitive(credentials, baseUrl, roomToken) + optimisticAction( + apply = { + _uiState.update { it.copy(sensitiveConversation = newValue) } + suspend { _uiState.update { it.copy(sensitiveConversation = previousValue) } } + }, + request = { + if (newValue) { + conversationsRepository.markConversationAsSensitive(credentials, baseUrl, roomToken) + } else { + conversationsRepository.markConversationAsInsensitive(credentials, baseUrl, roomToken) + } } - } catch (exception: Exception) { - _uiState.update { it.copy(sensitiveConversation = previousValue) } + ).onFailure { throwable -> _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) - Log.e(TAG, "failed to toggle sensitive conversation state", exception) + Log.e(TAG, "failed to toggle sensitive conversation state", throwable) } } } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 31547455b0..784889af1f 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -42,7 +42,7 @@ import com.nextcloud.talk.utils.CapabilitiesUtil.hasSpreedFeatureCapability import com.nextcloud.talk.utils.SpreedFeatures import com.nextcloud.talk.utils.UserIdUtils import com.nextcloud.talk.utils.database.user.CurrentUserProviderOld -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import io.reactivex.Observer import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.disposables.Disposable @@ -782,22 +782,21 @@ class ConversationsListViewModel @Inject constructor( ) val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - messageId?.let { conversationListUpdater.markPendingReadMarker(conversation.internalId, it) } - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.markConversationAsRead(credentials, url, messageId) } - } - _readUnreadState.value = ConversationReadUnreadUiState.Success - } catch (e: Exception) { - messageId?.let { conversationListUpdater.clearPendingReadMarker(conversation.internalId, it) } - withContext(Dispatchers.IO) { - repository.updateConversation(original) - } - _readUnreadState.value = ConversationReadUnreadUiState.Error - } + val result = optimisticAction( + apply = { + messageId?.let { conversationListUpdater.markPendingReadMarker(conversation.internalId, it) } + applyLocally(optimistic) + revertTo(original) { + messageId?.let { conversationListUpdater.clearPendingReadMarker(conversation.internalId, it) } + } + }, + request = { conversationsRepository.markConversationAsRead(credentials, url, messageId) } + ) + + _readUnreadState.value = result.fold( + onSuccess = { ConversationReadUnreadUiState.Success }, + onFailure = { ConversationReadUnreadUiState.Error } + ) } } @@ -811,25 +810,36 @@ class ConversationsListViewModel @Inject constructor( ) val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingUnread(conversation.internalId) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.markConversationAsUnread(credentials, url) } - } - _readUnreadState.value = ConversationReadUnreadUiState.Success - } catch (e: Exception) { - conversationListUpdater.clearPendingUnread(conversation.internalId) - withContext(Dispatchers.IO) { - repository.updateConversation(original) - } - _readUnreadState.value = ConversationReadUnreadUiState.Error - } + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingUnread(conversation.internalId) + applyLocally(optimistic) + revertTo(original) { conversationListUpdater.clearPendingUnread(conversation.internalId) } + }, + request = { conversationsRepository.markConversationAsUnread(credentials, url) } + ) + + _readUnreadState.value = result.fold( + onSuccess = { ConversationReadUnreadUiState.Success }, + onFailure = { ConversationReadUnreadUiState.Error } + ) } } + private suspend fun applyLocally(conversation: ConversationModel) { + withContext(Dispatchers.IO) { repository.updateConversation(conversation) } + } + + /** + * The action that puts [original] back and releases whatever guard the caller registered, for + * [optimisticAction] to run when the request finally fails or the screen is left mid-request. + */ + private fun revertTo(original: ConversationModel, releaseGuard: () -> Unit): suspend () -> Unit = + { + releaseGuard() + applyLocally(original) + } + fun resetFavoriteState() { _favoriteState.value = FavoriteUiState.None } @@ -846,28 +856,27 @@ class ConversationsListViewModel @Inject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForArchive(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingArchived(conversation.internalId, desiredArchived) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { - if (desiredArchived) { - conversationsRepository.archiveConversation(credentials, url) - } else { - conversationsRepository.unarchiveConversation(credentials, url) - } + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingArchived(conversation.internalId, desiredArchived) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingArchived(conversation.internalId, desiredArchived) + } + }, + request = { + if (desiredArchived) { + conversationsRepository.archiveConversation(credentials, url) + } else { + conversationsRepository.unarchiveConversation(credentials, url) } } - _archiveState.value = ArchiveUiState.Success(desiredArchived, conversation.displayName) - } catch (e: Exception) { - conversationListUpdater.clearPendingArchived(conversation.internalId, desiredArchived) - withContext(Dispatchers.IO) { - repository.updateConversation(original) - } - _archiveState.value = ArchiveUiState.Error - } + ) + + _archiveState.value = result.fold( + onSuccess = { ArchiveUiState.Success(desiredArchived, conversation.displayName) }, + onFailure = { ArchiveUiState.Error } + ) } } @@ -878,22 +887,21 @@ class ConversationsListViewModel @Inject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = true) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.addConversationToFavorites(credentials, url) } - } - _favoriteState.value = FavoriteUiState.Success - } catch (e: Exception) { - conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = true) - withContext(Dispatchers.IO) { - repository.updateConversation(original) - } - _favoriteState.value = FavoriteUiState.Error - } + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = true) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = true) + } + }, + request = { conversationsRepository.addConversationToFavorites(credentials, url) } + ) + + _favoriteState.value = result.fold( + onSuccess = { FavoriteUiState.Success }, + onFailure = { FavoriteUiState.Error } + ) } } @@ -904,22 +912,21 @@ class ConversationsListViewModel @Inject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = false) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.removeConversationFromFavorites(credentials, url) } - } - _favoriteState.value = FavoriteUiState.Success - } catch (e: Exception) { - conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = false) - withContext(Dispatchers.IO) { - repository.updateConversation(original) - } - _favoriteState.value = FavoriteUiState.Error - } + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = false) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = false) + } + }, + request = { conversationsRepository.removeConversationFromFavorites(credentials, url) } + ) + + _favoriteState.value = result.fold( + onSuccess = { FavoriteUiState.Success }, + onFailure = { FavoriteUiState.Error } + ) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt index 856d93d77f..6637c22313 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -7,6 +7,7 @@ package com.nextcloud.talk.utils +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.withContext import retrofit2.HttpException @@ -34,11 +35,13 @@ fun isTransientFailure(error: Exception): Boolean = * [apply] writes the local state and returns the action that undoes it again, or null when there was * nothing to change - in which case nothing is undone later either. [request] is retried once for a * transient failure. [isConfirmed] decides whether an answer that did not throw still counts as a - * refusal, for endpoints that report one in the payload rather than in the status code. + * refusal, for endpoints that report one in the payload rather than in the status code. A request + * that fails for any other reason is not retried, but the change is still taken back. * * Cancellation is part of the contract: closing the screen while the request is in flight undoes the * change as well, instead of leaving it applied although the server may never have heard of it. */ +@Suppress("TooGenericExceptionCaught") suspend fun optimisticAction( apply: suspend () -> (suspend () -> Unit)?, isConfirmed: (T) -> Boolean = { true }, @@ -59,10 +62,9 @@ suspend fun optimisticAction( revertUninterruptibly() } Result.success(answer) - } catch (e: HttpException) { - revertUninterruptibly() - Result.failure(e) - } catch (e: IOException) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { revertUninterruptibly() Result.failure(e) } diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt index 991a9be850..459db74ffd 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt @@ -108,6 +108,34 @@ class ConversationInfoViewModelTest { assertFalse(model.uiState.value.guestsAllowed) } + @Test + fun `an important toggle that the server never accepts goes back to what it was`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingImportantRequests = Int.MAX_VALUE } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + assertEquals(true, model.uiState.value.importantConversation) + + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.importantConversation) + } + + @Test + fun `an important toggle survives a single connection problem`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingImportantRequests = 1 } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertEquals(2, repository.importantRequests) + assertEquals(true, model.uiState.value.importantConversation) + } + @Test fun `createConversationNameByParticipants should combine names correctly`() { val original = listOf("Dave", null, "Charlie") diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt index 189e7f6be1..4d41bcc755 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt @@ -14,6 +14,7 @@ import com.nextcloud.talk.models.json.participants.TalkBan import com.nextcloud.talk.models.json.profile.Profile import com.nextcloud.talk.repositories.conversations.ConversationsRepository import io.reactivex.Observable +import java.io.IOException /** * Answers with what a test needs of [ConversationsRepository.allowGuests], and fails everything @@ -24,6 +25,10 @@ class FakeConversationsRepository : ConversationsRepository { var failAllowGuests = false var lastAllowGuestsPassword: String? = null + /** How many of the next important-conversation requests fail with a connection problem. */ + var failingImportantRequests = 0 + var importantRequests = 0 + override suspend fun allowGuests( user: User, url: String, @@ -92,7 +97,14 @@ class FakeConversationsRepository : ConversationsRepository { credentials: String, baseUrl: String, roomToken: String - ): GenericOverall = throw UnsupportedOperationException() + ): GenericOverall { + importantRequests++ + if (failingImportantRequests > 0) { + failingImportantRequests-- + throw IOException("no connection") + } + return GenericOverall() + } override suspend fun markConversationAsUnImportant( credentials: String, From cad049cdeadec9d9a44a71443c60d5d0df2b6921 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:58:17 +0200 Subject: [PATCH 3/5] fix(conversation info): tell the user when a setting did not save The notification level, the message expiration and the call notification switch logged their failure and left the screen showing a setting the server never accepted. They now go back to the previous value and say so. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../viewmodel/ConversationInfoViewModel.kt | 55 ++++++--- .../DatabaseStorageModule.kt | 110 +++++++----------- 2 files changed, 86 insertions(+), 79 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index e75f1d8aef..976f4886be 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -807,10 +807,17 @@ class ConversationInfoViewModel @Inject constructor( } fun toggleCallNotifications() { - val newEnabled = !_uiState.value.callNotificationsEnabled - _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } + val previousEnabled = _uiState.value.callNotificationsEnabled + val newEnabled = !previousEnabled + viewModelScope.launch { - databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) + optimisticAction( + apply = { + _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } + suspend { _uiState.update { it.copy(callNotificationsEnabled = previousEnabled) } } + }, + request = { databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) } + ).onFailure { throwable -> reportSettingFailure("call notifications", throwable) } } } @@ -818,11 +825,22 @@ class ConversationInfoViewModel @Inject constructor( val res = NextcloudTalkApplication.sharedApplication!!.resources val values = res.getStringArray(R.array.message_notification_levels_entry_values) val descriptions = res.getStringArray(R.array.message_notification_levels) - if (position in values.indices && position in descriptions.indices) { - _uiState.update { it.copy(notificationLevel = descriptions[position]) } - viewModelScope.launch { - databaseStorageModule?.saveString("conversation_info_message_notifications_dropdown", values[position]) - } + if (position !in values.indices || position !in descriptions.indices) return + + val previousLevel = _uiState.value.notificationLevel + viewModelScope.launch { + optimisticAction( + apply = { + _uiState.update { it.copy(notificationLevel = descriptions[position]) } + suspend { _uiState.update { it.copy(notificationLevel = previousLevel) } } + }, + request = { + databaseStorageModule?.saveString( + "conversation_info_message_notifications_dropdown", + values[position] + ) + } + ).onFailure { throwable -> reportSettingFailure("the notification level", throwable) } } } @@ -830,14 +848,25 @@ class ConversationInfoViewModel @Inject constructor( val res = NextcloudTalkApplication.sharedApplication!!.resources val values = res.getStringArray(R.array.message_expiring_values) val descriptions = res.getStringArray(R.array.message_expiring_descriptions) - if (position in values.indices && position in descriptions.indices) { - _uiState.update { it.copy(messageExpirationLabel = descriptions[position]) } - viewModelScope.launch { - databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) - } + if (position !in values.indices || position !in descriptions.indices) return + + val previousLabel = _uiState.value.messageExpirationLabel + viewModelScope.launch { + optimisticAction( + apply = { + _uiState.update { it.copy(messageExpirationLabel = descriptions[position]) } + suspend { _uiState.update { it.copy(messageExpirationLabel = previousLabel) } } + }, + request = { databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) } + ).onFailure { throwable -> reportSettingFailure("the message expiration", throwable) } } } + private suspend fun reportSettingFailure(setting: String, throwable: Throwable) { + _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) + Log.e(TAG, "failed to save $setting", throwable) + } + fun setUpcomingEvent(summary: String?, time: String?) { _uiState.update { it.copy(upcomingEventSummary = summary, upcomingEventTime = time) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt index 449858e123..cd515aed4e 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt @@ -8,8 +8,6 @@ */ package com.nextcloud.talk.utils.preferences.preferencestorage -import android.text.TextUtils -import android.util.Log import autodagger.AutoInjector import com.nextcloud.talk.api.NcApi import com.nextcloud.talk.api.NcApiCoroutines @@ -63,7 +61,6 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { this.conversationToken = conversationToken } - @Suppress("Detekt.TooGenericExceptionCaught") suspend fun saveBoolean(key: String, value: Boolean) { if ("call_notifications_switch" == key) { val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4)) @@ -71,12 +68,7 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { val credentials = getCredentials(conversationUser.username, conversationUser.token) val notificationLevel = if (value) 1 else 0 withContext(Dispatchers.IO) { - try { - ncApiCoroutines!!.notificationCalls(credentials!!, url, notificationLevel) - Log.d(TAG, "Toggled notification calls") - } catch (e: Exception) { - Log.e(TAG, "Error when trying to toggle notification calls", e) - } + ncApiCoroutines!!.notificationCalls(credentials!!, url, notificationLevel) } } if ("lobby_switch" != key) { @@ -91,64 +83,51 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { } } - @Suppress("Detekt.TooGenericExceptionCaught") suspend fun saveString(key: String, value: String) { when (key) { - "conversation_settings_dropdown" -> { - try { - val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(API_VERSION_4)) - val trimmedValue = value.replace("expire_", "") - val valueInt = trimmedValue.toInt() - withContext(Dispatchers.IO) { - ncApiCoroutines!!.setMessageExpiration( - getCredentials(conversationUser.username, conversationUser.token)!!, - getUrlForMessageExpiration(apiVersion, conversationUser.baseUrl, conversationToken), - valueInt - ) - messageExpiration = valueInt - } - } catch (exception: Exception) { - Log.e(TAG, "Error when trying to set message expiration", exception) - } - } - "conversation_info_message_notifications_dropdown" -> { - try { - if (hasSpreedFeatureCapability( - conversationUser.capabilities!!.spreedCapability!!, - SpreedFeatures.NOTIFICATION_LEVELS - ) - ) { - if (TextUtils.isEmpty(messageNotificationLevel) || messageNotificationLevel != value) { - val intValue = when (value) { - "never" -> NOTIFICATION_NEVER - "mention" -> NOTIFICATION_MENTION - "always" -> NOTIFICATION_ALWAYS - else -> 0 - } - val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4, 1)) - withContext(Dispatchers.IO) { - ncApiCoroutines!!.setNotificationLevel( - getCredentials(conversationUser.username, conversationUser.token)!!, - getUrlForRoomNotificationLevel( - apiVersion, - conversationUser.baseUrl, - conversationToken - ), - intValue - ) - messageNotificationLevel = value - } - } else { - messageNotificationLevel = value - } - } - } catch (exception: Exception) { - Log.e(TAG, "Error trying to set notification level", exception) - } - } - else -> { - arbitraryStorageManager!!.storeStorageSetting(accountIdentifier, key, value, conversationToken) - } + "conversation_settings_dropdown" -> saveMessageExpiration(value) + "conversation_info_message_notifications_dropdown" -> saveNotificationLevel(value) + else -> arbitraryStorageManager!!.storeStorageSetting(accountIdentifier, key, value, conversationToken) + } + } + + private suspend fun saveMessageExpiration(value: String) { + val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(API_VERSION_4)) + val valueInt = value.replace("expire_", "").toInt() + withContext(Dispatchers.IO) { + ncApiCoroutines!!.setMessageExpiration( + getCredentials(conversationUser.username, conversationUser.token)!!, + getUrlForMessageExpiration(apiVersion, conversationUser.baseUrl, conversationToken), + valueInt + ) + messageExpiration = valueInt + } + } + + private suspend fun saveNotificationLevel(value: String) { + val spreedCapability = conversationUser.capabilities?.spreedCapability ?: return + if (!hasSpreedFeatureCapability(spreedCapability, SpreedFeatures.NOTIFICATION_LEVELS)) { + messageNotificationLevel = value + return + } + if (messageNotificationLevel == value) { + return + } + + val intValue = when (value) { + "never" -> NOTIFICATION_NEVER + "mention" -> NOTIFICATION_MENTION + "always" -> NOTIFICATION_ALWAYS + else -> 0 + } + val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4, 1)) + withContext(Dispatchers.IO) { + ncApiCoroutines!!.setNotificationLevel( + getCredentials(conversationUser.username, conversationUser.token)!!, + getUrlForRoomNotificationLevel(apiVersion, conversationUser.baseUrl, conversationToken), + intValue + ) + messageNotificationLevel = value } } @@ -186,7 +165,6 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { } companion object { - private const val TAG = "DatabaseStorageModule" private const val EXPIRE_1_HOUR = 3600 private const val EXPIRE_8_HOURS = 28800 private const val EXPIRE_1_DAY = 86400 From 55800e7262d7f4e09159c36ae31e6c1a32a8609c Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 01:00:36 +0200 Subject: [PATCH 4/5] perf(conversation list): drop a deleted conversation from the list at once Deleting left the row in place until the worker had finished and the list had been fetched again. It now disappears on confirmation, reusing the guard that hides a conversation being left, and comes back if the delete fails. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../ConversationsListActivity.kt | 51 ++++++++++--------- .../viewmodels/ConversationsListViewModel.kt | 18 +++---- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt index 1b41c8082c..ae3c5dbd05 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt @@ -1241,7 +1241,7 @@ class ConversationsListActivity : BaseActivity() { @SuppressLint("StringFormatInvalid") private fun showLeaveConversationSnackbar(conversation: ConversationModel) { val token = conversation.token ?: return - conversationsListViewModel.markConversationPendingLeave(token) + conversationsListViewModel.markConversationPendingRemoval(token) lifecycleScope.launch { val result = snackbarHostState.showSnackbar( message = String.format(resources.getString(R.string.left_conversation), conversation.displayName), @@ -1249,7 +1249,7 @@ class ConversationsListActivity : BaseActivity() { duration = SnackbarDuration.Long ) when (result) { - SnackbarResult.ActionPerformed -> conversationsListViewModel.clearConversationPendingLeave(token) + SnackbarResult.ActionPerformed -> conversationsListViewModel.clearConversationPendingRemoval(token) SnackbarResult.Dismissed -> leaveConversation(conversation) } } @@ -1277,11 +1277,11 @@ class ConversationsListActivity : BaseActivity() { resources.getString(R.string.nc_shortcut_conversation_deleted) ) } - conversationsListViewModel.clearConversationPendingLeave(token) + conversationsListViewModel.clearConversationPendingRemoval(token) fetchRooms() } WorkInfo.State.FAILED -> { - conversationsListViewModel.clearConversationPendingLeave(token) + conversationsListViewModel.clearConversationPendingRemoval(token) showSnackbar(resources.getString(R.string.nc_common_error_sorry)) } else -> {} @@ -1298,6 +1298,7 @@ class ConversationsListActivity : BaseActivity() { .setTitle(R.string.nc_delete_call) .setMessage(R.string.nc_delete_conversation_more) .setPositiveButton(R.string.nc_delete) { _, _ -> + conversation.token?.let { conversationsListViewModel.markConversationPendingRemoval(it) } deleteConversation(conversation) } .setNegativeButton(R.string.nc_cancel) { _, _ -> @@ -1515,31 +1516,31 @@ class ConversationsListActivity : BaseActivity() { WorkManager.getInstance(context).getWorkInfoByIdLiveData(deleteConversationWorker.id) .observeForever { workInfo: WorkInfo? -> - if (workInfo != null) { - when (workInfo.state) { - WorkInfo.State.SUCCEEDED -> { - currentUser?.id?.let { userId -> - ShortcutManagerHelper.disableConversationShortcut( - context, - conversation.token, - userId, - context.resources.getString(R.string.nc_shortcut_conversation_deleted) - ) - } - showSnackbar( - String.format( - context.resources.getString(R.string.deleted_conversation), - conversation.displayName - ) + when (workInfo?.state) { + WorkInfo.State.SUCCEEDED -> { + currentUser?.id?.let { userId -> + ShortcutManagerHelper.disableConversationShortcut( + context, + conversation.token, + userId, + context.resources.getString(R.string.nc_shortcut_conversation_deleted) ) } + conversation.token?.let { conversationsListViewModel.clearConversationPendingRemoval(it) } + showSnackbar( + String.format( + context.resources.getString(R.string.deleted_conversation), + conversation.displayName + ) + ) + } - WorkInfo.State.FAILED -> { - showSnackbar(context.resources.getString(R.string.nc_common_error_sorry)) - } + WorkInfo.State.FAILED -> { + conversation.token?.let { conversationsListViewModel.clearConversationPendingRemoval(it) } + showSnackbar(context.resources.getString(R.string.nc_common_error_sorry)) + } - else -> { - } + else -> { } } } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 784889af1f..55aeb31156 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -242,10 +242,10 @@ class ConversationsListViewModel @Inject constructor( private val hideRoomToken = MutableStateFlow(null) - /** Tokens of rooms being left; hidden optimistically while the leave-undo snackbar is showing. */ - private val pendingLeaveTokens = MutableStateFlow>(emptySet()) + /** Tokens of rooms being left or deleted; hidden optimistically until the request finished. */ + private val pendingRemovalTokens = MutableStateFlow>(emptySet()) - private val excludedRoomTokens = combine(hideRoomToken, pendingLeaveTokens) { hideToken, pendingTokens -> + private val excludedRoomTokens = combine(hideRoomToken, pendingRemovalTokens) { hideToken, pendingTokens -> if (hideToken != null) pendingTokens + hideToken else pendingTokens } @@ -387,14 +387,14 @@ class ConversationsListViewModel @Inject constructor( hideRoomToken.value = token } - /** Optimistically hide a room while its leave-undo snackbar is showing. */ - fun markConversationPendingLeave(token: String) { - pendingLeaveTokens.value = pendingLeaveTokens.value + token + /** Optimistically hide a room that is being left or deleted. */ + fun markConversationPendingRemoval(token: String) { + pendingRemovalTokens.value = pendingRemovalTokens.value + token } - /** Un-hide a room, either because the leave was undone or because it finished/failed. */ - fun clearConversationPendingLeave(token: String) { - pendingLeaveTokens.value = pendingLeaveTokens.value - token + /** Un-hide a room, either because the removal was undone or because it finished or failed. */ + fun clearConversationPendingRemoval(token: String) { + pendingRemovalTokens.value = pendingRemovalTokens.value - token } fun getFederationInvitations() { From 5099081afd31248429481dda7cc339dd4d5af9cb Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 09:20:16 +0200 Subject: [PATCH 5/5] test(utils): cover the optimistic action contract directly The helper was only exercised through the repositories that use it, so the rules every optimistic action depends on - applied before the answer, taken back on a refusal or a cancellation, kept after one hiccup - had no test of their own. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../talk/utils/OptimisticActionTest.kt | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt diff --git a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt new file mode 100644 index 0000000000..6e48a2e0e3 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt @@ -0,0 +1,139 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.utils + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import retrofit2.HttpException +import retrofit2.Response +import java.io.IOException + +/** + * The contract every optimistic action in the app relies on: the change is visible before the answer + * arrives, it survives a hiccup, and it is taken back whenever the request does not finally succeed - + * including when the screen is left while the request is still in flight. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class OptimisticActionTest { + + private var state = "before" + + private val applyChange: suspend () -> (suspend () -> Unit)? = { + val previous = state + state = "after" + { state = previous } + } + + @Test + fun `a change the server accepts stays applied`() = + runTest { + val result = optimisticAction(apply = applyChange, request = { "ok" }) + + assertEquals("ok", result.getOrNull()) + assertEquals("after", state) + } + + @Test + fun `a change the server refuses is taken back`() = + runTest { + val result = optimisticAction(apply = applyChange, request = { throw forbidden() }) + + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `a connection problem is retried once before the change is taken back`() = + runTest { + var attempts = 0 + + val result = optimisticAction( + apply = applyChange, + request = { + attempts++ + throw IOException("no connection") + } + ) + + assertEquals(2, attempts) + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `a single connection problem does not cost the change`() = + runTest { + var attempts = 0 + + optimisticAction( + apply = applyChange, + request = { + attempts++ + if (attempts == 1) throw IOException("no connection") else "ok" + } + ) + + assertEquals("after", state) + } + + @Test + fun `a refusal the server reports in the payload is taken back as well`() = + runTest { + val result = optimisticAction( + apply = applyChange, + isConfirmed = { answer: String -> answer == "ok" }, + request = { "refused" } + ) + + // the request itself did not fail, so the caller still sees the answer it got + assertEquals("refused", result.getOrNull()) + assertEquals("before", state) + } + + @Test + fun `nothing is taken back when nothing was applied`() = + runTest { + val result = optimisticAction(apply = { null }, request = { throw forbidden() }) + + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `leaving the screen while the request is in flight takes the change back`() = + runTest { + val job = launch { + optimisticAction(apply = applyChange, request = { awaitForever() }) + } + runCurrent() + assertEquals("after", state) + + job.cancel() + advanceUntilIdle() + + assertEquals("before", state) + } + + private suspend fun awaitForever(): String = suspendCancellableCoroutine { } + + private fun forbidden(): HttpException = + HttpException(Response.error(HTTP_FORBIDDEN, "".toResponseBody("text/plain".toMediaType()))) + + private companion object { + const val HTTP_FORBIDDEN = 403 + } +}