Skip to content

Put every optimistic action on one helper and fix the ones that failed silently #6715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
AndyScherzinger wants to merge 5 commits into
base: feat/noid/optimisticReactions
Choose a base branch
from
Draft

Put every optimistic action on one helper and fix the ones that failed silently #6715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1879b2
refactor(chat): extract the optimistic action pattern into one helper
AndyScherzinger Sep 17, 2026
9cc4d58
perf(conversations): keep list and info actions correct when they fail
AndyScherzinger Sep 17, 2026
cad049c
fix(conversation info): tell the user when a setting did not save
AndyScherzinger Sep 17, 2026
55800e7
perf(conversation list): drop a deleted conversation from the list at…
AndyScherzinger Sep 17, 2026
5099081
test(utils): cover the optimistic action contract directly
AndyScherzinger Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 37 additions & 86 deletions app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -802,31 +801,24 @@ class OfflineFirstChatRepository @Inject constructor(
messageId: Long,
text: String
): Result<ChatOverallSingleMessage> {
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
Expand Down Expand Up @@ -884,29 +876,19 @@ class OfflineFirstChatRepository @Inject constructor(
url: String,
messageId: Long,
deletedPlaceholder: String
): Result<ChatOverallSingleMessage?> {
val restore = applyLocalDeletion(messageId, deletedPlaceholder)

return try {
val response = revertOnCancellation({ restore?.invoke() }) {
withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) {
): Result<ChatOverallSingleMessage?> =
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,
Expand Down Expand Up @@ -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,
Expand All @@ -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 =
Expand All @@ -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<ChatMessageJson>) {
Expand Down Expand Up @@ -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
}
}
102 changes: 71 additions & 31 deletions app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -806,37 +807,66 @@ 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) }
}
}

fun saveNotificationLevel(position: Int) {
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) }
}
}

fun saveMessageExpiration(position: Int) {
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) }
}
Expand All @@ -857,18 +887,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)
}
}
}
Expand All @@ -877,18 +912,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)
}
}
}
Expand Down
Loading