From 453253a78892e6b897166d9bc624f1a43385b9cb Mon Sep 17 00:00:00 2001 From: Yan Wang Date: Thu, 3 Sep 2026 02:45:49 -0700 Subject: [PATCH 1/2] feat: optimistic send UI + bounded REST timeout for bad networks On a bad network the send button appeared dead: Android had no optimistic message insertion (iOS does) and the shared OkHttp client used readTimeout(0), so a half-open connection could hang for minutes with zero feedback. - Split the network client: REST calls get a bounded 60s read timeout so a stuck request fails fast; the SSE stream keeps an infinite read timeout on its own client (it can be silent for minutes while the agent works). - Insert an optimistic user row (deterministic msg_ id) immediately on send, clear the composer, and show a spinner on the send button. - Reconcile in loadMessages/loadMoreMessages by id membership: keep pending rows the server has not echoed yet, prune confirmed ids. On failure, drop the row and restore the composer text/attachments. - Pass the client-chosen messageID through PromptRequest so the server echoes the same id back (pure id-membership reconciliation). --- .../opencode_client/data/api/OpenCodeApi.kt | 1 + .../data/repository/OpenCodeRepository.kt | 38 ++++-- .../yage/opencode_client/ui/MainViewModel.kt | 32 ++++- .../ui/MainViewModelSessionActions.kt | 88 ++++++++++++- .../opencode_client/ui/chat/ChatInputBar.kt | 30 +++-- .../opencode_client/ui/chat/ChatScreen.kt | 1 + .../yage/opencode_client/MainViewModelTest.kt | 99 +++++++++++++-- .../opencode_client/NfcQuickPromptTest.kt | 6 +- .../opencode_client/OptimisticSendTest.kt | 119 ++++++++++++++++++ 9 files changed, 371 insertions(+), 43 deletions(-) create mode 100644 app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt diff --git a/app/src/main/java/com/yage/opencode_client/data/api/OpenCodeApi.kt b/app/src/main/java/com/yage/opencode_client/data/api/OpenCodeApi.kt index c2a3ea99..0b736dd1 100644 --- a/app/src/main/java/com/yage/opencode_client/data/api/OpenCodeApi.kt +++ b/app/src/main/java/com/yage/opencode_client/data/api/OpenCodeApi.kt @@ -123,6 +123,7 @@ data class UpdateSessionTimeRequest( @kotlinx.serialization.Serializable data class PromptRequest( + @kotlinx.serialization.SerialName("messageID") val messageId: String? = null, val parts: List, val agent: String = "build", val model: ModelInput? = null diff --git a/app/src/main/java/com/yage/opencode_client/data/repository/OpenCodeRepository.kt b/app/src/main/java/com/yage/opencode_client/data/repository/OpenCodeRepository.kt index a2e6447e..072b8c35 100644 --- a/app/src/main/java/com/yage/opencode_client/data/repository/OpenCodeRepository.kt +++ b/app/src/main/java/com/yage/opencode_client/data/repository/OpenCodeRepository.kt @@ -28,12 +28,13 @@ class OpenCodeRepository @Inject constructor() { encodeDefaults = true // Include type in parts - server needs discriminator } - private var okHttpClient: OkHttpClient = buildOkHttpClient() - private var retrofit: Retrofit = buildRetrofit() + private var restHttpClient: OkHttpClient = buildHttpClient(REST_READ_TIMEOUT_SECONDS) + private var sseHttpClient: OkHttpClient = buildHttpClient(SSE_READ_TIMEOUT_SECONDS) + private var retrofit: Retrofit = buildRetrofit(restHttpClient) private var api: OpenCodeApi = retrofit.create(OpenCodeApi::class.java) - private var sseClient: SSEClient = SSEClient(okHttpClient) + private var sseClient: SSEClient = SSEClient(sseHttpClient) - private fun buildOkHttpClient(): OkHttpClient { + private fun buildHttpClient(readTimeoutSeconds: Long): OkHttpClient { return OkHttpClient.Builder() .addInterceptor(HttpLoggingInterceptor().apply { level = HttpLoggingInterceptor.Level.BASIC @@ -52,26 +53,28 @@ class OpenCodeRepository @Inject constructor() { .build() chain.proceed(request) } - .connectTimeout(15, java.util.concurrent.TimeUnit.SECONDS) - .readTimeout(0, java.util.concurrent.TimeUnit.SECONDS) + .connectTimeout(CONNECT_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS) + .writeTimeout(WRITE_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS) + .readTimeout(readTimeoutSeconds, java.util.concurrent.TimeUnit.SECONDS) .build() } - private fun buildRetrofit(): Retrofit { + private fun buildRetrofit(client: OkHttpClient): Retrofit { val url = if (baseUrl.startsWith("http")) baseUrl else "http://$baseUrl" return Retrofit.Builder() .baseUrl(url.trimEnd('/') + "/") - .client(okHttpClient) + .client(client) .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) .build() } @Synchronized private fun rebuildClients() { - okHttpClient = buildOkHttpClient() - retrofit = buildRetrofit() + restHttpClient = buildHttpClient(REST_READ_TIMEOUT_SECONDS) + sseHttpClient = buildHttpClient(SSE_READ_TIMEOUT_SECONDS) + retrofit = buildRetrofit(restHttpClient) api = retrofit.create(OpenCodeApi::class.java) - sseClient = SSEClient(okHttpClient) + sseClient = SSEClient(sseHttpClient) } @Synchronized @@ -126,7 +129,8 @@ class OpenCodeRepository @Inject constructor() { text: String, agent: String = "build", model: Message.ModelInfo? = null, - attachments: List = emptyList() + attachments: List = emptyList(), + messageId: String? = null ): Result = apiCall { val parts = buildList { if (text.isNotBlank()) add(PromptRequest.PartInput(type = "text", text = text)) @@ -142,6 +146,7 @@ class OpenCodeRepository @Inject constructor() { } } val request = PromptRequest( + messageId = messageId, parts = parts, agent = agent, model = model?.let { PromptRequest.ModelInput(it.providerId, it.modelId) } @@ -229,5 +234,14 @@ class OpenCodeRepository @Inject constructor() { companion object { const val DEFAULT_SERVER = "http://localhost:4096" + + // REST calls (prompt_async, getMessages, ...) get a bounded read timeout so a + // half-open connection fails fast instead of hanging forever. SSE is a + // long-lived stream that can be silent for minutes while the agent works, so + // it keeps an infinite read timeout on its own client. + private const val CONNECT_TIMEOUT_SECONDS = 15L + private const val WRITE_TIMEOUT_SECONDS = 60L + private const val REST_READ_TIMEOUT_SECONDS = 60L + private const val SSE_READ_TIMEOUT_SECONDS = 0L } } diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt index b5a9fb63..f7a74a3c 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt @@ -99,6 +99,7 @@ data class AppState( val sessionTodos: Map> = emptyMap(), val sendingSessionIds: Set = emptySet(), val sessionSendTimestamps: Map = emptyMap(), + val pendingOptimisticMessageIds: Set = emptySet(), val imageAttachments: List = emptyList(), val hostProfiles: List = emptyList(), val currentHostProfileId: String? = null, @@ -1584,8 +1585,24 @@ class MainViewModel @Inject constructor( val attachments = _state.value.imageAttachments if (text.isEmpty() && attachments.isEmpty()) return + // Insert the optimistic user row immediately so the send feels instant, then + // clear the composer. The row carries the same deterministic msg_ id we send + // to the server, so reconciliation in loadMessages is pure id membership. + val messageId = makeServerId("msg") + val optimistic = buildOptimisticMessage( + sessionId = sessionId, + text = text, + attachments = attachments, + messageId = messageId, + parentMessageId = _state.value.messages.lastOrNull()?.info?.id + ) + _state.update { state -> state.copy( + messages = state.messages + optimistic, + pendingOptimisticMessageIds = state.pendingOptimisticMessageIds + messageId, + inputText = "", + imageAttachments = emptyList(), sendingSessionIds = state.sendingSessionIds + sessionId, sessionSendTimestamps = state.sessionSendTimestamps + (sessionId to System.currentTimeMillis()) ) @@ -1605,11 +1622,11 @@ class MainViewModel @Inject constructor( attachments = attachments, agent = agent, model = model, + messageId = messageId, onRefreshMessages = ::loadMessagesWithRetry, onRefreshSessions = ::loadSessions, onSuccess = { settingsManager.setDraftText(sessionId, "") - _state.update { it.copy(imageAttachments = emptyList()) } }, onComplete = { _state.update { state -> @@ -1632,7 +1649,18 @@ class MainViewModel @Inject constructor( dispatchSend() } .onFailure { error -> - _state.update { it.copy(error = "Failed to restore session: ${errorMessageOrFallback(error, "unknown error")}") } + // Restoring the archived session failed; roll back the optimistic row. + _state.update { state -> + state.copy( + messages = state.messages.filter { m -> m.info.id != messageId }, + pendingOptimisticMessageIds = state.pendingOptimisticMessageIds - messageId, + inputText = text, + imageAttachments = attachments, + sendingSessionIds = state.sendingSessionIds - sessionId, + sessionSendTimestamps = state.sessionSendTimestamps - sessionId, + error = "Failed to restore session: ${errorMessageOrFallback(error, "unknown error")}" + ) + } } } return diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt index b010ac21..f6c0a297 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt @@ -2,6 +2,8 @@ package com.yage.opencode_client.ui import com.yage.opencode_client.data.model.ComposerImageAttachment import com.yage.opencode_client.data.model.Message +import com.yage.opencode_client.data.model.MessageWithParts +import com.yage.opencode_client.data.model.Part import com.yage.opencode_client.data.repository.OpenCodeRepository import com.yage.opencode_client.util.SettingsManager import kotlinx.coroutines.CoroutineScope @@ -9,6 +11,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch +import java.util.UUID internal fun launchLoadSessions( scope: CoroutineScope, @@ -164,6 +167,7 @@ internal fun selectSessionState( it.copy( currentSessionId = sessionId, messages = emptyList(), + pendingOptimisticMessageIds = emptySet(), streamingPartTexts = emptyMap(), streamingReasoningPart = null, messageLimit = 30, @@ -172,6 +176,19 @@ internal fun selectSessionState( } } +internal fun mergePendingOptimisticMessages( + serverMessages: List, + currentState: AppState +): Pair, Set> { + val loadedIds = serverMessages.map { it.info.id }.toSet() + val pendingRows = currentState.messages.filter { m -> + currentState.pendingOptimisticMessageIds.contains(m.info.id) && m.info.id !in loadedIds + } + val merged = serverMessages + pendingRows + val prunedPending = currentState.pendingOptimisticMessageIds - loadedIds + return merged to prunedPending +} + internal fun launchLoadMessages( scope: CoroutineScope, repository: OpenCodeRepository, @@ -197,8 +214,10 @@ internal fun launchLoadMessages( val modelIndex = settingsManager?.getModelForSession(sessionId) ?: inferredModelIndex val agentName = settingsManager?.getAgentForSession(sessionId) ?: inferredAgentName state.update { + val (mergedMessages, prunedPending) = mergePendingOptimisticMessages(messages, it) it.copy( - messages = messages, + messages = mergedMessages, + pendingOptimisticMessageIds = prunedPending, messageLimit = limit, isLoadingMessages = false, selectedModelIndex = modelIndex ?: it.selectedModelIndex, @@ -263,8 +282,10 @@ internal fun launchLoadMoreMessages( .onSuccess { messages -> if (sessionId == state.value.currentSessionId) { state.update { + val (mergedMessages, prunedPending) = mergePendingOptimisticMessages(messages, it) it.copy( - messages = messages, + messages = mergedMessages, + pendingOptimisticMessageIds = prunedPending, messageLimit = newLimit, isLoadingMessages = false ) @@ -439,17 +460,17 @@ internal fun launchSendMessage( attachments: List = emptyList(), agent: String, model: Message.ModelInfo?, + messageId: String, onRefreshMessages: (String, Boolean) -> Unit, onRefreshSessions: () -> Unit, onSuccess: (() -> Unit)? = null, onComplete: (() -> Unit)? = null ) { scope.launch { - repository.sendMessage(sessionId, text, agent, model, attachments = attachments) + repository.sendMessage(sessionId, text, agent, model, attachments = attachments, messageId = messageId) .onSuccess { state.update { it.copy( - inputText = "", error = null, sessions = bumpSessionUpdated(it.sessions, sessionId, System.currentTimeMillis()), sessionStatuses = it.sessionStatuses + (sessionId to com.yage.opencode_client.data.model.SessionStatus(type = "busy")) @@ -465,8 +486,65 @@ internal fun launchSendMessage( } } .onFailure { error -> - state.update { it.copy(error = errorMessageOrFallback(error, "Failed to send message")) } + // The optimistic row was inserted before dispatch. On failure, drop it + // and hand the text/attachments back to the composer so the user can retry. + state.update { + it.copy( + messages = it.messages.filter { m -> m.info.id != messageId }, + pendingOptimisticMessageIds = it.pendingOptimisticMessageIds - messageId, + inputText = text, + imageAttachments = attachments, + error = errorMessageOrFallback(error, "Failed to send message") + ) + } } onComplete?.invoke() } } + +internal fun makeServerId(prefix: String): String = + "${prefix}_${UUID.randomUUID().toString().replace("-", "")}" + +internal fun buildOptimisticMessage( + sessionId: String, + text: String, + attachments: List, + messageId: String, + parentMessageId: String? +): MessageWithParts { + val now = System.currentTimeMillis() + val message = Message( + id = messageId, + sessionId = sessionId, + role = "user", + parentId = parentMessageId, + time = Message.TimeInfo(created = now, completed = now) + ) + val parts = buildList { + if (text.isNotBlank()) { + add( + Part( + id = "temp-part-$messageId", + messageId = messageId, + sessionId = sessionId, + type = "text", + text = text + ) + ) + } + attachments.forEach { attachment -> + add( + Part( + id = "temp-file-${attachment.id}", + messageId = messageId, + sessionId = sessionId, + type = "file", + mime = attachment.mime, + filename = attachment.filename, + url = attachment.dataUrl + ) + ) + } + } + return MessageWithParts(info = message, parts = parts) +} diff --git a/app/src/main/java/com/yage/opencode_client/ui/chat/ChatInputBar.kt b/app/src/main/java/com/yage/opencode_client/ui/chat/ChatInputBar.kt index 0319e2e5..ade10402 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/chat/ChatInputBar.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/chat/ChatInputBar.kt @@ -36,6 +36,7 @@ import androidx.compose.material.icons.filled.Mic import androidx.compose.material.icons.filled.MoreHoriz import androidx.compose.material.icons.filled.Refresh import androidx.compose.material.icons.filled.Stop +import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.DropdownMenu import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.Icon @@ -83,6 +84,7 @@ import kotlin.math.sin internal fun ChatInputBar( text: String, isBusy: Boolean, + isSending: Boolean = false, isRecording: Boolean, isTranscribing: Boolean, hasPreservedSpeechAudio: Boolean, @@ -201,12 +203,13 @@ internal fun ChatInputBar( ChatPrimaryActionButton( onClick = onSend, - enabled = canSend, + enabled = canSend && !isSending, containerColor = MaterialTheme.colorScheme.primary, contentColor = Color.White, dimWhenDisabled = true, icon = Icons.AutoMirrored.Filled.Send, - contentDescription = stringResource(R.string.chat_send) + contentDescription = stringResource(R.string.chat_send), + progress = isSending ) } @@ -528,7 +531,8 @@ private fun ChatPrimaryActionButton( contentColor: Color, dimWhenDisabled: Boolean, icon: ImageVector, - contentDescription: String + contentDescription: String, + progress: Boolean = false ) { val effectiveAlpha = if (!enabled && dimWhenDisabled) 0.35f else 1f val interaction = remember { MutableInteractionSource() } @@ -549,12 +553,20 @@ private fun ChatPrimaryActionButton( }, contentAlignment = Alignment.Center ) { - Icon( - icon, - contentDescription = null, - tint = contentColor, - modifier = Modifier.size(20.dp) - ) + if (progress) { + CircularProgressIndicator( + modifier = Modifier.size(20.dp), + strokeWidth = 2.dp, + color = contentColor + ) + } else { + Icon( + icon, + contentDescription = null, + tint = contentColor, + modifier = Modifier.size(20.dp) + ) + } } } diff --git a/app/src/main/java/com/yage/opencode_client/ui/chat/ChatScreen.kt b/app/src/main/java/com/yage/opencode_client/ui/chat/ChatScreen.kt index 3cdc6bf7..25703fd0 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/chat/ChatScreen.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/chat/ChatScreen.kt @@ -271,6 +271,7 @@ fun ChatScreen( ChatInputBar( text = state.inputText, isBusy = currentSessionIsRunning, + isSending = state.currentSessionId?.let { it in state.sendingSessionIds } == true, isRecording = state.isRecording, isTranscribing = state.isTranscribing, hasPreservedSpeechAudio = state.hasPreservedSpeechAudio, diff --git a/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt b/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt index 5bb7feac..a59f2f01 100644 --- a/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt +++ b/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt @@ -61,6 +61,7 @@ import kotlinx.serialization.json.buildJsonArray import kotlinx.serialization.json.buildJsonObject import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before @@ -376,7 +377,7 @@ class MainViewModelTest { @Test fun `sendMessage success clears input and uses selected preset model`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) coEvery { repository.getSessions(400) } returns Result.success( listOf(com.yage.opencode_client.data.model.Session(id = "session-1", directory = "/tmp/project")) ) @@ -397,7 +398,9 @@ class MainViewModelTest { "session-1", "hello world", "review", - Message.ModelInfo(selected.providerId, selected.modelId) + Message.ModelInfo(selected.providerId, selected.modelId), + any(), + any() ) } assertEquals("", viewModel.state.value.inputText) @@ -406,7 +409,7 @@ class MainViewModelTest { @Test fun `sendMessage ignores duplicate sends while request is in flight`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any(), any()) } coAnswers { + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } coAnswers { delay(100) Result.success(Unit) } @@ -421,13 +424,13 @@ class MainViewModelTest { advanceUntilIdle() - coVerify(exactly = 1) { repository.sendMessage(any(), any(), any(), any(), any()) } + coVerify(exactly = 1) { repository.sendMessage(any(), any(), any(), any(), any(), any()) } assertFalse(viewModel.state.value.sendingSessionIds.contains("session-1")) } @Test fun `sendMessage success refreshes sessions`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) coEvery { repository.getSessions(400) } returns Result.success( listOf(com.yage.opencode_client.data.model.Session(id = "session-1", directory = "/tmp/project", title = "Updated")) ) @@ -458,7 +461,7 @@ class MainViewModelTest { title = "Previous Top", time = com.yage.opencode_client.data.model.Session.TimeInfo(updated = 2_000) ) - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) coEvery { repository.getSessions(400) } returns Result.success(listOf(previousTop, current)) val viewModel = createViewModel() @@ -478,7 +481,7 @@ class MainViewModelTest { @Test fun `sendMessage failure keeps input and exposes error`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.failure(IllegalStateException("send failed")) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.failure(IllegalStateException("send failed")) val viewModel = createViewModel() viewModel.selectSession("session-1") @@ -494,7 +497,7 @@ class MainViewModelTest { @Test fun `sendMessage still queues prompt when current session is busy`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) val viewModel = createViewModel() viewModel.selectSession("session-1") @@ -514,6 +517,8 @@ class MainViewModelTest { "session-1", "queue this next", any(), + any(), + any(), any() ) } @@ -534,10 +539,80 @@ class MainViewModelTest { viewModel.sendMessage() advanceUntilIdle() - coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any(), any()) } + coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any(), any(), any()) } assertEquals("do not send yet", viewModel.state.value.inputText) } + @Test + fun `sendMessage inserts optimistic user row and clears input immediately`() = runTest { + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } coAnswers { + delay(100) + Result.success(Unit) + } + val viewModel = createViewModel() + viewModel.selectSession("session-1") + advanceUntilIdle() + viewModel.setInputText("hello") + + viewModel.sendMessage() + + // The optimistic row is inserted synchronously, before the network call resolves. + val state = viewModel.state.value + assertEquals("", state.inputText) + assertTrue(state.sendingSessionIds.contains("session-1")) + val optimistic = state.messages.lastOrNull() + assertNotNull(optimistic) + assertEquals("user", optimistic!!.info.role) + assertEquals("hello", optimistic.parts.first { it.isText }.text) + assertTrue(state.pendingOptimisticMessageIds.contains(optimistic.info.id)) + } + + @Test + fun `sendMessage failure removes optimistic row and restores input`() = runTest { + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns + Result.failure(IllegalStateException("send failed")) + val viewModel = createViewModel() + viewModel.selectSession("session-1") + advanceUntilIdle() + viewModel.setInputText("hello") + + viewModel.sendMessage() + // Optimistic row is present immediately after the send is dispatched. + assertTrue(viewModel.state.value.messages.isNotEmpty()) + advanceUntilIdle() + + // After the failure, the optimistic row is dropped and the input restored. + val state = viewModel.state.value + assertEquals("hello", state.inputText) + assertEquals("send failed", state.error) + assertTrue(state.messages.isEmpty()) + assertTrue(state.pendingOptimisticMessageIds.isEmpty()) + } + + @Test + fun `sendMessage success keeps optimistic row until server confirms it`() = runTest { + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.getMessages(any(), any()) } returns Result.success(emptyList()) + coEvery { repository.getSessions(400) } returns Result.success( + listOf(com.yage.opencode_client.data.model.Session(id = "session-1", directory = "/tmp/project")) + ) + + val viewModel = createViewModel() + viewModel.selectSession("session-1") + advanceUntilIdle() + viewModel.setInputText("hello") + + viewModel.sendMessage() + advanceUntilIdle() + + // The server has not echoed the message yet, so the optimistic row stays visible. + val state = viewModel.state.value + val optimistic = state.messages.lastOrNull() + assertNotNull(optimistic) + assertEquals("user", optimistic!!.info.role) + assertTrue(state.pendingOptimisticMessageIds.contains(optimistic.info.id)) + } + @Test fun `sendMessage ignores blank input`() = runTest { val viewModel = createViewModel() @@ -548,7 +623,7 @@ class MainViewModelTest { viewModel.sendMessage() advanceUntilIdle() - coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any()) } + coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any(), any(), any()) } assertEquals(" ", viewModel.state.value.inputText) } @@ -560,7 +635,7 @@ class MainViewModelTest { viewModel.sendMessage() advanceUntilIdle() - coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any()) } + coVerify(exactly = 0) { repository.sendMessage(any(), any(), any(), any(), any(), any()) } assertEquals("hello", viewModel.state.value.inputText) } @@ -1827,7 +1902,7 @@ class MainViewModelTest { @Test fun `sendMessage on success clears draft for current session`() = runTest { - coEvery { repository.sendMessage(any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) val viewModel = createViewModel() viewModel.selectSession("s1") diff --git a/app/src/test/java/com/yage/opencode_client/NfcQuickPromptTest.kt b/app/src/test/java/com/yage/opencode_client/NfcQuickPromptTest.kt index bcc24ebf..80eb7c32 100644 --- a/app/src/test/java/com/yage/opencode_client/NfcQuickPromptTest.kt +++ b/app/src/test/java/com/yage/opencode_client/NfcQuickPromptTest.kt @@ -107,7 +107,7 @@ class NfcQuickPromptTest { coEvery { repository.getSessions(any()) } returns Result.success(emptyList()) coEvery { repository.getAgents() } returns Result.success(emptyList()) coEvery { repository.getProviders() } returns Result.success(ProvidersResponse()) - coEvery { repository.sendMessage(any(), any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) } @After @@ -165,7 +165,7 @@ class NfcQuickPromptTest { Session(id = "s1", directory = "/tmp") ) coEvery { repository.getMessages(any(), any()) } returns Result.success(emptyList()) - coEvery { repository.sendMessage(any(), any(), any(), any(), any()) } returns Result.success(Unit) + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } returns Result.success(Unit) val vm = createViewModel() vm.handleNfcPrompt("test prompt", autoSend = true) @@ -174,7 +174,7 @@ class NfcQuickPromptTest { // autoSend=true: sendMessage clears inputText on success assertNull(vm.state.value.pendingNfcAction) // Verify sendMessage was called - coVerify { repository.sendMessage("s1", "test prompt", any(), any(), any()) } + coVerify { repository.sendMessage("s1", "test prompt", any(), any(), any(), any()) } } @Test diff --git a/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt b/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt new file mode 100644 index 00000000..31b0b4c1 --- /dev/null +++ b/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt @@ -0,0 +1,119 @@ +package com.yage.opencode_client + +import com.yage.opencode_client.data.model.ComposerImageAttachment +import com.yage.opencode_client.data.model.Message +import com.yage.opencode_client.data.model.MessageWithParts +import com.yage.opencode_client.ui.AppState +import com.yage.opencode_client.ui.buildOptimisticMessage +import com.yage.opencode_client.ui.makeServerId +import com.yage.opencode_client.ui.mergePendingOptimisticMessages +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +class OptimisticSendTest { + + @Test + fun `makeServerId produces a prefixed dashless uuid`() { + val id = makeServerId("msg") + assertTrue(id.startsWith("msg_")) + assertEquals(36, id.length) + assertTrue(id.substring(4).all { it.isDigit() || it in 'a'..'f' }) + } + + @Test + fun `makeServerId is unique across calls`() { + val a = makeServerId("msg") + val b = makeServerId("msg") + assertTrue(a != b) + } + + @Test + fun `buildOptimisticMessage builds a user row with text and file parts`() { + val attachment = ComposerImageAttachment( + id = "att-1", + filename = "a.png", + mime = "image/png", + dataUrl = "data:image/png;base64,xxx", + thumbnailData = byteArrayOf(1, 2, 3), + byteSize = 3 + ) + val row = buildOptimisticMessage( + sessionId = "s1", + text = "hello", + attachments = listOf(attachment), + messageId = "msg_abc", + parentMessageId = "msg_parent" + ) + assertEquals("msg_abc", row.info.id) + assertEquals("user", row.info.role) + assertEquals("s1", row.info.sessionId) + assertEquals("msg_parent", row.info.parentId) + assertEquals(2, row.parts.size) + assertEquals("text", row.parts[0].type) + assertEquals("hello", row.parts[0].text) + assertEquals("file", row.parts[1].type) + assertEquals("a.png", row.parts[1].filename) + assertEquals("data:image/png;base64,xxx", row.parts[1].url) + } + + @Test + fun `buildOptimisticMessage with blank text and no attachments has no parts`() { + val row = buildOptimisticMessage( + sessionId = "s1", + text = " ", + attachments = emptyList(), + messageId = "msg_abc", + parentMessageId = null + ) + assertEquals(0, row.parts.size) + assertEquals("user", row.info.role) + } + + @Test + fun `merge keeps unconfirmed pending rows and prunes confirmed ids`() { + val pendingRow = MessageWithParts( + info = Message(id = "msg_pending", sessionId = "s1", role = "user"), + parts = emptyList() + ) + val confirmedRow = MessageWithParts( + info = Message(id = "msg_confirmed", sessionId = "s1", role = "assistant"), + parts = emptyList() + ) + val currentState = AppState( + messages = listOf(pendingRow, confirmedRow), + pendingOptimisticMessageIds = setOf("msg_pending", "msg_confirmed") + ) + // Server returns the confirmed message; the pending one is still in flight. + val (merged, pruned) = mergePendingOptimisticMessages(listOf(confirmedRow), currentState) + assertEquals(listOf("msg_confirmed", "msg_pending"), merged.map { it.info.id }) + assertEquals(setOf("msg_pending"), pruned) + } + + @Test + fun `merge drops the pending row once the server confirms it`() { + val confirmedRow = MessageWithParts( + info = Message(id = "msg_x", sessionId = "s1", role = "user"), + parts = emptyList() + ) + val currentState = AppState( + messages = listOf(confirmedRow), + pendingOptimisticMessageIds = setOf("msg_x") + ) + val (merged, pruned) = mergePendingOptimisticMessages(listOf(confirmedRow), currentState) + assertEquals(listOf("msg_x"), merged.map { it.info.id }) + assertEquals(emptySet(), pruned) + } + + @Test + fun `merge is a no-op when there are no pending rows`() { + val server = MessageWithParts( + info = Message(id = "msg_a", sessionId = "s1", role = "assistant"), + parts = emptyList() + ) + val currentState = AppState(messages = listOf(server), pendingOptimisticMessageIds = emptySet()) + val (merged, pruned) = mergePendingOptimisticMessages(listOf(server), currentState) + assertEquals(listOf("msg_a"), merged.map { it.info.id }) + assertEquals(emptySet(), pruned) + } +} \ No newline at end of file From 86b05bee6ff657a7dc2dde44633b7eb559d1d202 Mon Sep 17 00:00:00 2001 From: Yan Wang Date: Thu, 3 Sep 2026 04:50:17 -0700 Subject: [PATCH 2/2] fix: handle async send failures and session-scoped optimistic rollback Address review findings on the optimistic send UI: - Handle the session.error SSE event. prompt_async acknowledges with 204 before the turn runs, so a failure that happens before the user message is persisted (agent missing, session deleted, crash) only surfaces through this event. Previously it was dropped, leaving a permanent ghost optimistic row and silently losing the user's text. Now, for the current session, drop the pending optimistic rows, recover their text into the composer, and set an error. Add parseSessionErrorReason to build a bounded display reason. - Guard the send-failure composer rollback by session ownership so a failure that lands after the user switched sessions no longer clobbers the other session's draft. - Reset pendingOptimisticMessageIds in resetRuntimeForHostSwitch so stale ids do not leak across a host switch. Adds tests for the session.error handling (drop+recover, and ignore for a different session), the cross-session rollback guard, the host-switch reset, and the parseSessionErrorReason edge cases. --- .../yage/opencode_client/ui/MainViewModel.kt | 1 + .../ui/MainViewModelSessionActions.kt | 6 +- .../ui/MainViewModelSupport.kt | 23 +++ .../ui/MainViewModelSyncActions.kt | 29 ++++ .../yage/opencode_client/MainViewModelTest.kt | 152 ++++++++++++++++++ .../opencode_client/OptimisticSendTest.kt | 68 ++++++++ 6 files changed, 277 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt index f7a74a3c..4ba4b109 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModel.kt @@ -1503,6 +1503,7 @@ class MainViewModel @Inject constructor( sessionTodos = emptyMap(), sendingSessionIds = emptySet(), sessionSendTimestamps = emptyMap(), + pendingOptimisticMessageIds = emptySet(), agents = emptyList(), providers = null, filePathToShowInFiles = null, diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt index f6c0a297..87f36a9b 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSessionActions.kt @@ -488,12 +488,14 @@ internal fun launchSendMessage( .onFailure { error -> // The optimistic row was inserted before dispatch. On failure, drop it // and hand the text/attachments back to the composer so the user can retry. + // Only restore the composer if the user is still on the session that sent + // this message; otherwise we'd clobber another session's draft. state.update { it.copy( messages = it.messages.filter { m -> m.info.id != messageId }, pendingOptimisticMessageIds = it.pendingOptimisticMessageIds - messageId, - inputText = text, - imageAttachments = attachments, + inputText = if (it.currentSessionId == sessionId) text else it.inputText, + imageAttachments = if (it.currentSessionId == sessionId) attachments else it.imageAttachments, error = errorMessageOrFallback(error, "Failed to send message") ) } diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSupport.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSupport.kt index 8773d8bb..7710b4af 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSupport.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSupport.kt @@ -7,6 +7,7 @@ import com.yage.opencode_client.data.model.SSEEvent import com.yage.opencode_client.data.model.Session import com.yage.opencode_client.data.model.SessionStatus import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonObject import kotlinx.serialization.json.JsonPrimitive import java.security.MessageDigest @@ -169,6 +170,28 @@ internal fun parseQuestionAskedEvent(event: SSEEvent): QuestionRequest? { }.getOrNull() } +/** + * Builds a short display reason from a `session.error` payload + * (`error: {name, data: {message}}`). Server causes can be long multi-line + * dumps; keep the first meaningful line, bounded. + */ +internal fun parseSessionErrorReason(event: SSEEvent): String { + val errorObj = event.payload.getJsonObject("error") ?: return "unknown error" + val name = (errorObj["name"] as? JsonPrimitive)?.content + val data = errorObj["data"] as? JsonObject + val message = (data?.get("message") as? JsonPrimitive)?.content + ?: (data?.get("error") as? JsonPrimitive)?.content + val text = when { + !message.isNullOrBlank() -> { + val firstLine = message.lineSequence().map { it.trim() }.firstOrNull { it.isNotEmpty() } ?: message + if (!name.isNullOrBlank()) "$name: $firstLine" else firstLine + } + !name.isNullOrBlank() -> name + else -> "unknown error" + } + return text.take(300) +} + internal fun reasoningPartOrNull(partType: String, partId: String, messageId: String, sessionId: String): Part? { return if (partType == "reasoning") { Part(id = partId, messageId = messageId, sessionId = sessionId, type = "reasoning") diff --git a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSyncActions.kt b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSyncActions.kt index 1ceba24e..02e109ef 100644 --- a/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSyncActions.kt +++ b/app/src/main/java/com/yage/opencode_client/ui/MainViewModelSyncActions.kt @@ -184,5 +184,34 @@ internal fun handleIncomingSseEvent( } state.update { it.copy(sessionTodos = it.sessionTodos + (sessionId to todos)) } } + "session.error" -> { + val sessionId = event.payload.getString("sessionID") + if (sessionId != null && sessionId == state.value.currentSessionId) { + // prompt_async acknowledges with 204 before the turn runs, so a failure + // that happens before the user message is persisted only surfaces here. + // The server will never echo our id, so drop the pending optimistic rows + // and hand their text back to the composer so the user can retry. + val reason = parseSessionErrorReason(event) + state.update { state -> + if (state.pendingOptimisticMessageIds.isEmpty()) { + state.copy(error = "Send failed: $reason") + } else { + val pendingIds = state.pendingOptimisticMessageIds + val recoveredText = state.messages + .filter { m -> m.info.id in pendingIds } + .mapNotNull { row -> row.parts.firstOrNull { p -> p.isText }?.text } + .filter { text -> text.isNotBlank() } + .joinToString("\n") + state.copy( + messages = state.messages.filter { m -> m.info.id !in pendingIds }, + pendingOptimisticMessageIds = emptySet(), + inputText = recoveredText.ifBlank { state.inputText }, + error = "Send failed: $reason" + ) + } + } + onRefreshMessages(sessionId, false) + } + } } } diff --git a/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt b/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt index a59f2f01..6e91ccba 100644 --- a/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt +++ b/app/src/test/java/com/yage/opencode_client/MainViewModelTest.kt @@ -613,6 +613,158 @@ class MainViewModelTest { assertTrue(state.pendingOptimisticMessageIds.contains(optimistic.info.id)) } + @Test + fun `sendMessage failure does not clobber another session draft after switching`() = runTest { + coEvery { repository.sendMessage(any(), any(), any(), any(), any(), any()) } coAnswers { + delay(100) + Result.failure(IllegalStateException("send failed")) + } + every { settingsManager.getDraftText("session-2") } returns "session-2 draft" + + val viewModel = createViewModel() + viewModel.selectSession("session-1") + advanceUntilIdle() + viewModel.setInputText("hello") + + viewModel.sendMessage() + // Switch to another session while session-1's send is still in flight. + viewModel.selectSession("session-2") + advanceUntilIdle() + + // session-1's send failed, but session-2's draft must be preserved. + val state = viewModel.state.value + assertEquals("session-2", state.currentSessionId) + assertEquals("session-2 draft", state.inputText) + assertEquals("send failed", state.error) + assertTrue(state.pendingOptimisticMessageIds.isEmpty()) + } + + @Test + fun `session_error SSE removes pending optimistic row and recovers text`() = runTest { + coEvery { repository.getMessages(any(), any()) } returns Result.success(emptyList()) + val viewModel = createViewModel() + updateState(viewModel) { + it.copy( + currentSessionId = "session-1", + messages = listOf( + MessageWithParts( + info = Message(id = "msg_pending", sessionId = "session-1", role = "user"), + parts = listOf( + Part(id = "p1", messageId = "msg_pending", sessionId = "session-1", type = "text", text = "hello") + ) + ) + ), + pendingOptimisticMessageIds = setOf("msg_pending") + ) + } + + handleSse( + viewModel, + SSEEvent( + payload = SSEPayload( + type = "session.error", + properties = buildJsonObject { + put("sessionID", JsonPrimitive("session-1")) + put( + "error", + buildJsonObject { + put("name", JsonPrimitive("ProviderAuthError")) + put("data", buildJsonObject { put("message", JsonPrimitive("nope")) }) + } + ) + } + ) + ) + ) + advanceUntilIdle() + + val state = viewModel.state.value + assertTrue(state.messages.isEmpty()) + assertTrue(state.pendingOptimisticMessageIds.isEmpty()) + assertEquals("hello", state.inputText) + assertEquals("Send failed: ProviderAuthError: nope", state.error) + } + + @Test + fun `session_error SSE for another session is ignored`() = runTest { + coEvery { repository.getMessages(any(), any()) } returns Result.success(emptyList()) + val viewModel = createViewModel() + updateState(viewModel) { + it.copy( + currentSessionId = "session-1", + messages = listOf( + MessageWithParts( + info = Message(id = "msg_pending", sessionId = "session-1", role = "user"), + parts = listOf( + Part(id = "p1", messageId = "msg_pending", sessionId = "session-1", type = "text", text = "hello") + ) + ) + ), + pendingOptimisticMessageIds = setOf("msg_pending") + ) + } + + handleSse( + viewModel, + SSEEvent( + payload = SSEPayload( + type = "session.error", + properties = buildJsonObject { + put("sessionID", JsonPrimitive("session-2")) + put("error", buildJsonObject { put("name", JsonPrimitive("X")) }) + } + ) + ) + ) + advanceUntilIdle() + + // The error belongs to a different session, so the pending row is untouched. + assertEquals(1, viewModel.state.value.messages.size) + assertTrue(viewModel.state.value.pendingOptimisticMessageIds.contains("msg_pending")) + assertNull(viewModel.state.value.error) + } + + @Test + fun `host switch clears pending optimistic message ids`() = runTest { + val first = HostProfile( + id = "host-1", + name = "First", + transport = HostTransport.DIRECT, + serverUrl = "http://first.test" + ) + val second = HostProfile( + id = "host-2", + name = "Second", + transport = HostTransport.DIRECT, + serverUrl = "http://second.test" + ) + var currentProfile = first + every { hostProfileStore.currentProfile() } answers { currentProfile } + every { hostProfileStore.profiles() } returns listOf(first, second) + every { hostProfileStore.select(second.id) } answers { + currentProfile = second + second + } + coEvery { repository.checkHealth() } returns Result.failure(IllegalStateException("offline")) + + val viewModel = createViewModel() + updateState(viewModel) { + it.copy( + isConnected = true, + currentSessionId = "session-1", + messages = listOf(MessageWithParts(Message(id = "msg_pending", sessionId = "session-1", role = "user"))), + pendingOptimisticMessageIds = setOf("msg_pending") + ) + } + + viewModel.selectHostProfile(second.id) + advanceUntilIdle() + + assertTrue(viewModel.state.value.pendingOptimisticMessageIds.isEmpty()) + assertNull(viewModel.state.value.currentSessionId) + assertTrue(viewModel.state.value.messages.isEmpty()) + } + @Test fun `sendMessage ignores blank input`() = runTest { val viewModel = createViewModel() diff --git a/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt b/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt index 31b0b4c1..6f4537d0 100644 --- a/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt +++ b/app/src/test/java/com/yage/opencode_client/OptimisticSendTest.kt @@ -3,10 +3,15 @@ package com.yage.opencode_client import com.yage.opencode_client.data.model.ComposerImageAttachment import com.yage.opencode_client.data.model.Message import com.yage.opencode_client.data.model.MessageWithParts +import com.yage.opencode_client.data.model.SSEEvent +import com.yage.opencode_client.data.model.SSEPayload import com.yage.opencode_client.ui.AppState import com.yage.opencode_client.ui.buildOptimisticMessage import com.yage.opencode_client.ui.makeServerId import com.yage.opencode_client.ui.mergePendingOptimisticMessages +import com.yage.opencode_client.ui.parseSessionErrorReason +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.buildJsonObject import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test @@ -116,4 +121,67 @@ class OptimisticSendTest { assertEquals(listOf("msg_a"), merged.map { it.info.id }) assertEquals(emptySet(), pruned) } + + @Test + fun `parseSessionErrorReason combines name and first message line`() { + val event = SSEEvent( + payload = SSEPayload( + type = "session.error", + properties = buildJsonObject { + put("sessionID", JsonPrimitive("s1")) + put( + "error", + buildJsonObject { + put("name", JsonPrimitive("ProviderAuthError")) + put("data", buildJsonObject { put("message", JsonPrimitive("boom")) }) + } + ) + } + ) + ) + assertEquals("ProviderAuthError: boom", parseSessionErrorReason(event)) + } + + @Test + fun `parseSessionErrorReason keeps only the first meaningful line of a multi-line message`() { + val event = SSEEvent( + payload = SSEPayload( + type = "session.error", + properties = buildJsonObject { + put( + "error", + buildJsonObject { + put("name", JsonPrimitive("APIError")) + put( + "data", + buildJsonObject { + put("message", JsonPrimitive("first line\nsecond line\nthird")) + } + ) + } + ) + } + ) + ) + assertEquals("APIError: first line", parseSessionErrorReason(event)) + } + + @Test + fun `parseSessionErrorReason falls back to name when there is no message`() { + val event = SSEEvent( + payload = SSEPayload( + type = "session.error", + properties = buildJsonObject { + put("error", buildJsonObject { put("name", JsonPrimitive("MessageAbortedError")) }) + } + ) + ) + assertEquals("MessageAbortedError", parseSessionErrorReason(event)) + } + + @Test + fun `parseSessionErrorReason returns fallback when the error object is missing`() { + val event = SSEEvent(payload = SSEPayload(type = "session.error")) + assertEquals("unknown error", parseSessionErrorReason(event)) + } } \ No newline at end of file