fix(chat): tell the user when a reminder or a share to notes failed

A reminder the server refused was only logged, and the chat says "Message
sent" the moment a share to notes starts, before the answer arrives - so a
share that failed was reported as a success. Both now show what happened.

Assisted-by: Claude Code:claude-opus-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Author: AndyScherzinger

app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt

@@ -641,13 +641,15 @@ interface NcApiCoroutines {
         @Field("timestamp") timestamp: Int
     ): ReminderOverall
 
+    @Suppress("LongParameterList")
     @FormUrlEncoded
     @POST
     suspend fun sendLocation(
         @Header("Authorization") authorization: String,
         @Url url: String,
         @Field("objectType") objectType: String,
         @Field("objectId") objectId: String,
-        @Field("metaData") metaData: String
+        @Field("metaData") metaData: String,
+        @Field("referenceId") referenceId: String
     ): GenericOverall
 }

app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt

@@ -1532,6 +1532,12 @@ class ChatActivity :
             }
         }
 
+        lifecycleScope.launch {
+            chatViewModel.actionFailures.collect { message ->
+                Snackbar.make(binding.root, message, Snackbar.LENGTH_LONG).show()
+            }
+        }
+
         lifecycleScope.launch {
             chatViewModel.noMoreSearchResults.collect {
                 val inSearchMode = chatViewModel.chatMode.value == ChatViewModel.ChatMode.SEARCH_MODE

app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt

@@ -9,7 +9,7 @@ package com.nextcloud.talk.chat.data.network
 import com.nextcloud.talk.data.user.model.User
 import com.nextcloud.talk.models.domain.ConversationModel
 import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto
-import com.nextcloud.talk.models.json.chat.ChatMessageJson
+import com.nextcloud.talk.models.json.chat.ChatMessageDto
 import com.nextcloud.talk.models.json.chat.ChatOverall
 import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage
 import com.nextcloud.talk.models.json.conversations.RoomOverall
@@ -36,21 +36,26 @@ interface ChatNetworkDataSource {
 
     suspend fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): ReminderDto
     suspend fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): GenericOverall
+
+    @Suppress("LongParameterList")
     suspend fun shareToNotes(
         credentials: String,
         url: String,
         message: String,
-        displayName: String
+        displayName: String,
+        referenceId: String
     ): ChatOverallSingleMessage
 
     suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall
 
+    @Suppress("LongParameterList")
     suspend fun shareLocationToNotes(
         credentials: String,
         url: String,
         objectType: String,
         objectId: String,
-        metadata: String
+        metadata: String,
+        referenceId: String
     ): GenericOverall
 
     suspend fun leaveRoom(credentials: String, url: String): GenericOverall
@@ -84,7 +89,7 @@ interface ChatNetworkDataSource {
         messageId: String,
         limit: Int,
         threadId: Int?
-    ): List<ChatMessageJson>
+    ): List<ChatMessageDto>
     suspend fun getOpenGraph(credentials: String, baseUrl: String, extractedLinkToPreview: String): ReferenceDto?
     suspend fun unbindRoom(credentials: String, baseUrl: String, roomToken: String): GenericOverall
 

app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt

@@ -11,7 +11,7 @@ import com.nextcloud.talk.api.NcApiCoroutines
 import com.nextcloud.talk.data.user.model.User
 import com.nextcloud.talk.models.domain.ConversationModel
 import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto
-import com.nextcloud.talk.models.json.chat.ChatMessageJson
+import com.nextcloud.talk.models.json.chat.ChatMessageDto
 import com.nextcloud.talk.models.json.chat.ChatOverall
 import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage
 import com.nextcloud.talk.models.json.conversations.RoomOverall
@@ -21,7 +21,6 @@ import com.nextcloud.talk.models.json.reminder.ReminderDto
 import com.nextcloud.talk.models.json.upcomingEvents.UpcomingEventsOverall
 import com.nextcloud.talk.models.json.userAbsence.UserAbsenceOverall
 import com.nextcloud.talk.utils.ApiUtils
-import com.nextcloud.talk.utils.message.SendMessageUtils
 import io.reactivex.Observable
 import retrofit2.Response
 
@@ -98,18 +97,10 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines:
         credentials: String,
         url: String,
         message: String,
-        displayName: String
+        displayName: String,
+        referenceId: String
     ): ChatOverallSingleMessage =
-        ncApiCoroutines.sendChatMessage(
-            credentials,
-            url,
-            message,
-            displayName,
-            0,
-            false,
-            SendMessageUtils().generateReferenceId(),
-            null
-        )
+        ncApiCoroutines.sendChatMessage(credentials, url, message, displayName, 0, false, referenceId, null)
 
     override suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall =
         ncApiCoroutines.getNoteToSelfRoom(credentials, url)
@@ -119,8 +110,9 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines:
         url: String,
         objectType: String,
         objectId: String,
-        metadata: String
-    ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata)
+        metadata: String,
+        referenceId: String
+    ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata, referenceId)
 
     override suspend fun leaveRoom(credentials: String, url: String): GenericOverall =
         ncApiCoroutines.leaveRoom(credentials, url)
@@ -196,7 +188,7 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines:
         messageId: String,
         limit: Int,
         threadId: Int?
-    ): List<ChatMessageJson> {
+    ): List<ChatMessageDto> {
         val url = ApiUtils.getUrlForChatMessageContext(baseUrl, token, messageId)
         return ncApiCoroutines.getContextOfChatMessage(credentials, url, limit, threadId).ocs?.data ?: listOf()
     }

app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt

@@ -12,6 +12,8 @@ import android.net.Uri
 import android.os.Bundle
 import android.provider.OpenableColumns
 import android.util.Log
+import androidx.annotation.StringRes
+import androidx.annotation.VisibleForTesting
 import androidx.lifecycle.DefaultLifecycleObserver
 import androidx.lifecycle.LifecycleOwner
 import androidx.lifecycle.LiveData
@@ -56,7 +58,7 @@ import com.nextcloud.talk.models.MessageDraft
 import com.nextcloud.talk.models.domain.ConversationModel
 import com.nextcloud.talk.models.domain.SearchMessageEntry
 import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto
-import com.nextcloud.talk.models.json.chat.ChatMessageJson
+import com.nextcloud.talk.models.json.chat.ChatMessageDto
 import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage
 import com.nextcloud.talk.models.json.conversations.ConversationEnums
 import com.nextcloud.talk.models.json.conversations.RoomOverall
@@ -81,6 +83,7 @@ import com.nextcloud.talk.utils.SpreedFeatures
 import com.nextcloud.talk.utils.UserIdUtils
 import com.nextcloud.talk.utils.throttleLatest
 import com.nextcloud.talk.utils.bundle.BundleKeys
+import com.nextcloud.talk.utils.isTransientFailure
 import com.nextcloud.talk.utils.message.SendMessageUtils
 import com.nextcloud.talk.utils.message.groupHashOf
 import com.nextcloud.talk.utils.preferences.AppPreferences
@@ -497,7 +500,7 @@ class ChatViewModel @AssistedInject constructor(
         mediaPlayerManager.handleOnStop()
     }
 
-    fun onSignalingChatMessageReceived(chatMessages: List<ChatMessageJson>) {
+    fun onSignalingChatMessageReceived(chatMessages: List<ChatMessageDto>) {
         viewModelScope.launch {
             chatRepository.onSignalingChatMessageReceived(chatMessages)
         }
@@ -655,6 +658,10 @@ class ChatViewModel @AssistedInject constructor(
     private val _reactionFailures = MutableSharedFlow<ReactionOperation>(extraBufferCapacity = 1)
     val reactionFailures: SharedFlow<ReactionOperation> = _reactionFailures
 
+    /** A reminder or a share to notes that the server refused, for the chat to report. */
+    private val _actionFailures = MutableSharedFlow<Int>(extraBufferCapacity = 1)
+    val actionFailures: SharedFlow<Int> = _actionFailures
+
     private val reactionLocks = mutableMapOf<Int, Mutex>()
 
     @Volatile private var firstUnreadMessageId: Int? = null
@@ -1859,15 +1866,9 @@ class ChatViewModel @AssistedInject constructor(
             ?.subscribe(JoinRoomObserver())
     }
 
-    @Suppress("Detekt.TooGenericExceptionCaught")
     fun setReminder(user: User, roomToken: String, messageId: String, timestamp: Int, chatApiVersion: Int) {
-        viewModelScope.launch {
-            try {
-                chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion)
-                Log.d(TAG, "reminder set successfully")
-            } catch (e: Exception) {
-                Log.e(TAG, "Error when sending reminder", e)
-            }
+        requestWithFeedback("set the reminder", R.string.nc_common_error_sorry) {
+            chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion)
         }
     }
 
@@ -1887,15 +1888,10 @@ class ChatViewModel @AssistedInject constructor(
         _getReminderExistState.value = GetReminderStateSet
     }
 
-    @Suppress("Detekt.TooGenericExceptionCaught")
     fun deleteReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) {
-        viewModelScope.launch {
-            try {
-                chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion)
-                _getReminderExistState.value = GetReminderStartState
-            } catch (e: Exception) {
-                Log.d(TAG, "Error when deleting reminder", e)
-            }
+        requestWithFeedback("delete the reminder", R.string.nc_common_error_sorry) {
+            chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion)
+            _getReminderExistState.value = GetReminderStartState
         }
     }
 
@@ -2153,14 +2149,9 @@ class ChatViewModel @AssistedInject constructor(
         )
     }
 
-    @Suppress("Detekt.TooGenericExceptionCaught")
     fun shareToNotes(credentials: String, url: String, message: String, displayName: String) {
-        viewModelScope.launch {
-            try {
-                chatNetworkDataSource.shareToNotes(credentials, url, message, displayName)
-            } catch (e: Exception) {
-                Log.d(TAG, "Error when sharing to notes", e)
-            }
+        shareToNotes("share the message to notes", credentials, url) { referenceId ->
+            chatNetworkDataSource.shareToNotes(credentials, url, message, displayName, referenceId)
         }
     }
 
@@ -2183,13 +2174,97 @@ class ChatViewModel @AssistedInject constructor(
             null
         }
 
-    @Suppress("Detekt.TooGenericExceptionCaught")
     fun shareLocationToNotes(credentials: String, url: String, objectType: String, objectId: String, metadata: String) {
+        shareToNotes("share the location to notes", credentials, url) { referenceId ->
+            chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata, referenceId)
+        }
+    }
+
+    /**
+     * Shares to the note to self and, when the share failed for a transient reason, asks the note to
+     * self whether the message arrived anyway before sending it a second time. The server stores the
+     * reference id of a message without ever looking at it again, so it deduplicates nothing: a blind
+     * second attempt after a timeout the server did accept posts the note twice.
+     */
+    private fun shareToNotes(
+        description: String,
+        credentials: String,
+        notesUrl: String,
+        share: suspend (String) -> Unit
+    ) {
+        val referenceId = SendMessageUtils().generateReferenceId()
+
+        viewModelScope.launch {
+            val failure = attemptShare(share, referenceId) ?: return@launch
+
+            if (!isTransientFailure(failure)) {
+                reportShareFailure(description, failure)
+                return@launch
+            }
+
+            if (!alreadyShared(credentials, notesUrl, referenceId)) {
+                attemptShare(share, referenceId)?.let { reportShareFailure(description, it) }
+            }
+        }
+    }
+
+    /** Runs the share once and reports what went wrong, or null when it worked. */
+    @Suppress("Detekt.TooGenericExceptionCaught")
+    private suspend fun attemptShare(share: suspend (String) -> Unit, referenceId: String): Exception? =
+        try {
+            share(referenceId)
+            null
+        } catch (e: CancellationException) {
+            throw e
+        } catch (e: Exception) {
+            e
+        }
+
+    /**
+     * Whether the note to self already holds the message of this share. A lookup that cannot be
+     * answered counts as "it might", because posting a second copy is worse than not retrying.
+     */
+    @Suppress("Detekt.TooGenericExceptionCaught")
+    private suspend fun alreadyShared(credentials: String, notesUrl: String, referenceId: String): Boolean =
+        try {
+            val fieldMap = hashMapOf(
+                "lookIntoFuture" to 0,
+                "includeLastKnown" to 1,
+                "setReadMarker" to 0,
+                "markNotificationsAsRead" to 0,
+                "timeout" to 0,
+                "limit" to SHARE_LOOKUP_LIMIT
+            )
+            val recent = chatNetworkDataSource.pullChatMessages(credentials, notesUrl, fieldMap)
+            holdsShare(recent.body()?.ocs?.data, referenceId)
+        } catch (e: CancellationException) {
+            throw e
+        } catch (e: Exception) {
+            logger.w(TAG, "Could not check whether the share arrived, so it is not sent again", e)
+            true
+        }
+
+    private fun reportShareFailure(description: String, throwable: Throwable) {
+        logger.e(TAG, "Failed to $description", throwable)
+        _actionFailures.tryEmit(R.string.nc_message_not_added_to_notes)
+    }
+
+    /**
+     * Runs an action whose only visible outcome is whether it worked. The reminder dialog is already
+     * dismissed and the chat has already said the message was sent by the time the answer arrives, so
+     * a failure that is not reported here is never reported at all.
+     *
+     * Deliberately without a retry: sharing to notes posts a message with a reference id generated per
+     * call, so a second attempt after a timeout the server did accept would post it twice.
+     */
+    @Suppress("Detekt.TooGenericExceptionCaught")
+    private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) {
         viewModelScope.launch {
             try {
-                chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata)
+                request()
             } catch (e: Exception) {
-                Log.e(TAG, "Error when sharing location to notes", e)
+                logger.e(TAG, "Failed to $description", e)
+                _actionFailures.tryEmit(failureMessage)
             }
         }
     }
@@ -2645,6 +2720,18 @@ class ChatViewModel @AssistedInject constructor(
     companion object {
         private val TAG = ChatViewModel::class.java.simpleName
 
+        /** Enough of the note to self to cover what a share of ours could have landed behind. */
+        private const val SHARE_LOOKUP_LIMIT = 20
+
+        /**
+         * Whether [messages] already hold the share that was sent with [referenceId]. Messages the
+         * server did not return - because the lookup failed or answered "not modified" - are read as
+         * "it may well be there", since sending a second copy is worse than not sending one at all.
+         */
+        @VisibleForTesting
+        fun holdsShare(messages: List<ChatMessageDto>?, referenceId: String): Boolean =
+            messages?.any { it.referenceId == referenceId } ?: true
+
         /**
          * Ids of the messages a delete of [messageId] applies to: every already-uploaded message of the
          * media group the message is shown in, or just the message itself.

app/src/main/res/values/strings.xml

@@ -559,6 +559,7 @@ How to translate with transifex:
     <string name="nc_scheduled_thread">Scheduled thread</string>
     <string name="nc_reply_to_thread">Reply to thread: %1$s</string>
     <string name="nc_message_added_to_notes">Message added to notes</string>
+    <string name="nc_message_not_added_to_notes">Could not add the message to notes</string>
     <string name="nc_message_failed">Failed to send message</string>
     <string name="nc_message_failed_to_send">Failed to send message:</string>
     <string name="nc_add_attachment">Add attachment</string>