From def122ab587ef7239d3c7f4ac57b571fff78c824 Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Thu, 23 Jul 2026 00:30:48 +0300 Subject: [PATCH] fix(#686): implement robust history pagination (#687) * fix(#686): implement robust history pagination Honor server-returned offset, degrade gracefully on older servers, and parse JSON tool results. * fix(ci): repair unit-test compilation and pagination test mocks - Remove trailing blank line in SessionMessagesResponse.kt (ktlint) - Replace unresolved io.mockk.eq/coAnswers usage in ChatViewModelTest with an ordered-call counter inside coAnswers, which compiles and runs on MockK 1.14.11 (eq is a matcher-scope member, not a top-level import) - Align HermesApiServiceMockWebServerTest with the PR's JsonElement content model (contentText) and the new include_compacted=true default query param All unit tests pass locally with the ByteBuddy agent preload. --------- Co-authored-by: M57 --- .../data/model/SessionMessagesResponse.kt | 13 +- .../data/remote/HermesApiService.kt | 1 + .../hermescontrol/ui/chat/ChatViewModel.kt | 25 ++- .../HermesApiServiceMockWebServerTest.kt | 67 +++++- .../ui/chat/ChatViewModelTest.kt | 204 +++++++++++++++++- 5 files changed, 294 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt b/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt index 735bef87..c8fc82f6 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt @@ -1,4 +1,5 @@ package com.m57.hermescontrol.data.model + import kotlinx.serialization.Serializable import kotlinx.serialization.json.JsonElement import kotlinx.serialization.json.JsonPrimitive @@ -6,15 +7,25 @@ import kotlinx.serialization.json.JsonPrimitive @Serializable data class SessionMessagesResponse( val messages: List, + val offset: Int? = null, + val total: Int? = null, ) @Serializable data class SessionMessage( val role: String? = null, - val content: String? = null, + val content: JsonElement? = null, val timestamp: JsonElement? = null, val type: String? = null, ) { val timestampText: String? get() = (timestamp as? JsonPrimitive)?.content + + val contentText: String + get() = + when (content) { + is JsonPrimitive -> content.content + null -> "" + else -> content.toString() + } } diff --git a/app/src/main/java/com/m57/hermescontrol/data/remote/HermesApiService.kt b/app/src/main/java/com/m57/hermescontrol/data/remote/HermesApiService.kt index 7b254659..f6a6cd21 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/remote/HermesApiService.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/remote/HermesApiService.kt @@ -143,6 +143,7 @@ interface HermesApiService { @Path("id", encoded = true) sessionId: String, @Query("limit") limit: Int? = null, @Query("offset") offset: Int = 0, + @Query("include_compacted") includeCompacted: Boolean? = true, ): Response @GET("api/sessions/stats") diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt index 019b7baf..efad2482 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt @@ -1408,8 +1408,9 @@ class ChatViewModel( val result = fetchMessagePage(sessionId, offset, MESSAGE_PAGE_SIZE) when (result) { is NetworkResult.Success -> { - val chatMessages = mapServerMessages(sessionId, result.data.messages.orEmpty(), offset) - loadedMessageOffset = offset + val serverOffset = result.data.offset ?: offset + val chatMessages = mapServerMessages(sessionId, result.data.messages.orEmpty(), serverOffset) + loadedMessageOffset = serverOffset withContext(Dispatchers.IO) { repo.persistMessages(chatMessages, sessionId) } @@ -1418,7 +1419,7 @@ class ChatViewModel( state.copy( messages = chatMessages, isLoading = false, - hasOlderMessages = offset > 0 && chatMessages.isNotEmpty(), + hasOlderMessages = serverOffset > 0 && chatMessages.isNotEmpty(), isLoadingOlder = false, ) } @@ -1449,15 +1450,16 @@ class ChatViewModel( viewModelScope.launch { when (val result = fetchMessagePage(sessionId, newOffset, limit)) { is NetworkResult.Success -> { - val older = mapServerMessages(sessionId, result.data.messages.orEmpty(), newOffset) - loadedMessageOffset = newOffset + val returnedOffset = result.data.offset ?: newOffset + val older = mapServerMessages(sessionId, result.data.messages.orEmpty(), returnedOffset) + loadedMessageOffset = returnedOffset withContext(Dispatchers.IO) { repo.persistMessages(older, sessionId) } _uiState.update { current -> if (current.currentSessionId != sessionId) return@update current current.copy( messages = (older + current.messages).distinctBy { it.id }, isLoadingOlder = false, - hasOlderMessages = newOffset > 0, + hasOlderMessages = returnedOffset < oldOffset && older.isNotEmpty() && returnedOffset > 0, ) } } @@ -1559,7 +1561,14 @@ class ChatViewModel( offset: Int, limit: Int, ) = withContext(Dispatchers.IO) { - safeApiCall { ApiClient.hermesApi.getSessionMessages(sessionId, limit = limit, offset = offset) } + safeApiCall { + ApiClient.hermesApi.getSessionMessages( + sessionId = sessionId, + limit = limit, + offset = offset, + includeCompacted = true, + ) + } } private fun mapServerMessages( @@ -1585,7 +1594,7 @@ class ChatViewModel( ChatMessage( id = "rest-$sessionId-$globalIndex", role = role, - content = msg.content.orEmpty(), + content = msg.contentText, timestamp = timestamp, isStreaming = false, ) diff --git a/app/src/test/java/com/m57/hermescontrol/data/remote/HermesApiServiceMockWebServerTest.kt b/app/src/test/java/com/m57/hermescontrol/data/remote/HermesApiServiceMockWebServerTest.kt index c084ba58..590ff0ae 100644 --- a/app/src/test/java/com/m57/hermescontrol/data/remote/HermesApiServiceMockWebServerTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/data/remote/HermesApiServiceMockWebServerTest.kt @@ -247,11 +247,11 @@ class HermesApiServiceMockWebServerTest { assertEquals(3, messages.size) assertEquals("user", messages[0].role) - assertEquals("Hello Hermes", messages[0].content) + assertEquals("Hello Hermes", messages[0].contentText) assertEquals("1718000000", messages[0].timestampText) assertEquals("assistant", messages[1].role) - assertEquals("Hi! How can I help?", messages[1].content) + assertEquals("Hi! How can I help?", messages[1].contentText) assertEquals("tool", messages[2].role) assertEquals("tool_execution", messages[2].type) @@ -270,11 +270,68 @@ class HermesApiServiceMockWebServerTest { assertTrue(response.isSuccessful) assertEquals( - "/api/sessions/desktop/session/messages?limit=150&offset=300", + "/api/sessions/desktop/session/messages?limit=150&offset=300&include_compacted=true", mockServer.takeRequest().path, ) } + @Test + fun getSessionMessages_withJsonObjectToolResult_deserializesSuccessfully() = + runBlocking { + mockServer.enqueue( + MockResponse() + .setResponseCode(200) + .setBody( + """ + { + "messages": [ + { + "role": "tool", + "content": {"status": "ok", "items": [1, 2]}, + "type": "tool_result" + } + ] + } + """.trimIndent(), + ), + ) + + val response = api.getSessionMessages("test-session-id") + assertTrue(response.isSuccessful) + + val body = response.body() + assertNotNull(body) + assertEquals(1, body!!.messages.size) + assertEquals("tool", body.messages[0].role) + assertEquals("{\"status\":\"ok\",\"items\":[1,2]}", body.messages[0].contentText) + } + + @Test + fun getSessionMessages_withOffsetAndTotal_deserializesPaginationFields() = + runBlocking { + mockServer.enqueue( + MockResponse() + .setResponseCode(200) + .setBody( + """ + { + "messages": [], + "offset": 45, + "total": 120 + } + """.trimIndent(), + ), + ) + + val response = api.getSessionMessages("test-session-id") + assertTrue(response.isSuccessful) + + val body = response.body() + assertNotNull(body) + assertEquals(45, body!!.offset) + assertEquals(120, body.total) + } + @Test fun getSessionMessages_withNullRole_doesNotCrash() = runBlocking { @@ -303,7 +360,7 @@ class HermesApiServiceMockWebServerTest { assertNotNull(body) assertEquals(1, body!!.messages.size) assertNull(body.messages[0].role) - assertEquals("Plain content", body.messages[0].content) + assertEquals("Plain content", body.messages[0].contentText) assertNull(body.messages[0].timestamp) } @@ -383,7 +440,7 @@ class HermesApiServiceMockWebServerTest { val request = mockServer.takeRequest() assertEquals( "path should contain the session ID with slashes preserved", - "/api/sessions/session/with/slashes/messages?offset=0", + "/api/sessions/session/with/slashes/messages?offset=0&include_compacted=true", request.path, ) } diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt index f43c5499..bfa4caa9 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt @@ -32,6 +32,9 @@ import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.buildJsonObject +import kotlinx.serialization.json.put import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -1677,7 +1680,7 @@ class ChatViewModelTest { verify { HermesWsClient.disconnect() } } - // ── History pagination guard (issue #674) ─────────────────────────────── + // ── History pagination guard (issue #674 & #686) ─────────────────────────────── /** * When the initial REST page returns empty messages, hasOlderMessages @@ -1707,7 +1710,7 @@ class ChatViewModelTest { ), ) coEvery { - mockApi.getSessionMessages("session-456", any(), any()) + mockApi.getSessionMessages("session-456", any(), any(), any()) } returns retrofit2.Response.success( com.m57.hermescontrol.data.model.SessionMessagesResponse( @@ -1725,4 +1728,201 @@ class ChatViewModelTest { assertTrue(viewModel.uiState.value.messages.isEmpty()) assertFalse(viewModel.uiState.value.isLoading) } + + @Test + fun testLoadOlderMessages_honorsServerReturnedOffset() = + runTest { + val (viewModel, _) = createViewModelWithSession() + + val mockApi = ApiClient.hermesApi + coEvery { + mockApi.getSessions(any(), any(), any()) + } returns + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionListResponse( + sessions = + listOf( + com.m57.hermescontrol.data.model.SessionInfo( + id = "session-456", + title = "Test", + message_count = 100, + ), + ), + total = 1, + ), + ) + var messagesCallCount = 0 + coEvery { + mockApi.getSessionMessages("session-456", any(), any(), any()) + } coAnswers { + messagesCallCount += 1 + if (messagesCallCount == 1) { + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionMessagesResponse( + messages = + listOf( + com.m57.hermescontrol.data.model.SessionMessage( + role = "assistant", + content = JsonPrimitive("Msg 50"), + ), + ), + offset = 50, + total = 100, + ), + ) + } else { + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionMessagesResponse( + messages = + listOf( + com.m57.hermescontrol.data.model.SessionMessage( + role = "user", + content = JsonPrimitive("Older Msg 20"), + ), + ), + offset = 20, + total = 100, + ), + ) + } + } + + viewModel.switchSession("session-456") + advanceUntilIdle() + + assertTrue(viewModel.uiState.value.hasOlderMessages) + + // Server returns effective offset 20 (different from requested offset 0) + viewModel.loadOlderMessages() + advanceUntilIdle() + + assertTrue(viewModel.uiState.value.hasOlderMessages) + val messages = viewModel.uiState.value.messages + assertEquals(2, messages.size) + assertEquals("rest-session-456-20", messages[0].id) + assertEquals("Older Msg 20", messages[0].content) + } + + @Test + fun testLoadOlderMessages_oldServerFallback_stopsPaginationWhenOffsetDoesNotDecrease() = + runTest { + val (viewModel, _) = createViewModelWithSession() + + val mockApi = ApiClient.hermesApi + coEvery { + mockApi.getSessions(any(), any(), any()) + } returns + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionListResponse( + sessions = + listOf( + com.m57.hermescontrol.data.model.SessionInfo( + id = "session-456", + title = "Test", + message_count = 100, + ), + ), + total = 1, + ), + ) + var messagesCallCount = 0 + coEvery { + mockApi.getSessionMessages("session-456", any(), any(), any()) + } coAnswers { + messagesCallCount += 1 + if (messagesCallCount == 1) { + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionMessagesResponse( + messages = + listOf( + com.m57.hermescontrol.data.model.SessionMessage( + role = "assistant", + content = JsonPrimitive("Msg 50"), + ), + ), + offset = 50, + total = 100, + ), + ) + } else { + // Older server ignores query params and returns offset = 50 + // (same as oldOffset, offset did not decrease) + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionMessagesResponse( + messages = + listOf( + com.m57.hermescontrol.data.model.SessionMessage( + role = "assistant", + content = JsonPrimitive("Msg 50"), + ), + ), + offset = 50, + total = 100, + ), + ) + } + } + + viewModel.switchSession("session-456") + advanceUntilIdle() + + viewModel.loadOlderMessages() + advanceUntilIdle() + + assertFalse( + "hasOlderMessages must be false when returned offset does not decrease", + viewModel.uiState.value.hasOlderMessages, + ) + } + + @Test + fun testLoadMessages_handlesJsonObjectToolResult() = + runTest { + val (viewModel, _) = createViewModelWithSession() + + val mockApi = ApiClient.hermesApi + coEvery { + mockApi.getSessions(any(), any(), any()) + } returns + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionListResponse( + sessions = + listOf( + com.m57.hermescontrol.data.model.SessionInfo( + id = "session-456", + title = "Test", + message_count = 1, + ), + ), + total = 1, + ), + ) + val jsonObjectContent = + buildJsonObject { + put("status", JsonPrimitive("ok")) + } + coEvery { + mockApi.getSessionMessages("session-456", any(), any(), any()) + } returns + retrofit2.Response.success( + com.m57.hermescontrol.data.model.SessionMessagesResponse( + messages = + listOf( + com.m57.hermescontrol.data.model.SessionMessage( + role = "tool", + content = jsonObjectContent, + ), + ), + offset = 0, + total = 1, + ), + ) + + viewModel.switchSession("session-456") + advanceUntilIdle() + + val messages = viewModel.uiState.value.messages + assertEquals(1, messages.size) + assertEquals("{\"status\":\"ok\"}", messages[0].content) + } }