* 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 <hy4ri@users.noreply.github.com>
This commit is contained in:
1 parent
34ba93901b
commit
def122ab58
5 files changed
+294
-16
No files matched your search
@@ -1,4 +1,5 @@
|
|||||||
package com.m57.hermescontrol.data.model
|
package com.m57.hermescontrol.data.model
|
||||||
|
|
||||||
import kotlinx.serialization.Serializable
|
import kotlinx.serialization.Serializable
|
||||||
import kotlinx.serialization.json.JsonElement
|
import kotlinx.serialization.json.JsonElement
|
||||||
import kotlinx.serialization.json.JsonPrimitive
|
import kotlinx.serialization.json.JsonPrimitive
|
||||||
@@ -6,15 +7,25 @@ import kotlinx.serialization.json.JsonPrimitive
|
|||||||
@Serializable
|
@Serializable
|
||||||
data class SessionMessagesResponse(
|
data class SessionMessagesResponse(
|
||||||
val messages: List<SessionMessage>,
|
val messages: List<SessionMessage>,
|
||||||
|
val offset: Int? = null,
|
||||||
|
val total: Int? = null,
|
||||||
)
|
)
|
||||||
|
|
||||||
@Serializable
|
@Serializable
|
||||||
data class SessionMessage(
|
data class SessionMessage(
|
||||||
val role: String? = null,
|
val role: String? = null,
|
||||||
val content: String? = null,
|
val content: JsonElement? = null,
|
||||||
val timestamp: JsonElement? = null,
|
val timestamp: JsonElement? = null,
|
||||||
val type: String? = null,
|
val type: String? = null,
|
||||||
) {
|
) {
|
||||||
val timestampText: String?
|
val timestampText: String?
|
||||||
get() = (timestamp as? JsonPrimitive)?.content
|
get() = (timestamp as? JsonPrimitive)?.content
|
||||||
|
|
||||||
|
val contentText: String
|
||||||
|
get() =
|
||||||
|
when (content) {
|
||||||
|
is JsonPrimitive -> content.content
|
||||||
|
null -> ""
|
||||||
|
else -> content.toString()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -143,6 +143,7 @@ interface HermesApiService {
|
|||||||
@Path("id", encoded = true) sessionId: String,
|
@Path("id", encoded = true) sessionId: String,
|
||||||
@Query("limit") limit: Int? = null,
|
@Query("limit") limit: Int? = null,
|
||||||
@Query("offset") offset: Int = 0,
|
@Query("offset") offset: Int = 0,
|
||||||
|
@Query("include_compacted") includeCompacted: Boolean? = true,
|
||||||
): Response<SessionMessagesResponse>
|
): Response<SessionMessagesResponse>
|
||||||
|
|
||||||
@GET("api/sessions/stats")
|
@GET("api/sessions/stats")
|
||||||
|
|||||||
@@ -1408,8 +1408,9 @@ class ChatViewModel(
|
|||||||
val result = fetchMessagePage(sessionId, offset, MESSAGE_PAGE_SIZE)
|
val result = fetchMessagePage(sessionId, offset, MESSAGE_PAGE_SIZE)
|
||||||
when (result) {
|
when (result) {
|
||||||
is NetworkResult.Success -> {
|
is NetworkResult.Success -> {
|
||||||
val chatMessages = mapServerMessages(sessionId, result.data.messages.orEmpty(), offset)
|
val serverOffset = result.data.offset ?: offset
|
||||||
loadedMessageOffset = offset
|
val chatMessages = mapServerMessages(sessionId, result.data.messages.orEmpty(), serverOffset)
|
||||||
|
loadedMessageOffset = serverOffset
|
||||||
withContext(Dispatchers.IO) {
|
withContext(Dispatchers.IO) {
|
||||||
repo.persistMessages(chatMessages, sessionId)
|
repo.persistMessages(chatMessages, sessionId)
|
||||||
}
|
}
|
||||||
@@ -1418,7 +1419,7 @@ class ChatViewModel(
|
|||||||
state.copy(
|
state.copy(
|
||||||
messages = chatMessages,
|
messages = chatMessages,
|
||||||
isLoading = false,
|
isLoading = false,
|
||||||
hasOlderMessages = offset > 0 && chatMessages.isNotEmpty(),
|
hasOlderMessages = serverOffset > 0 && chatMessages.isNotEmpty(),
|
||||||
isLoadingOlder = false,
|
isLoadingOlder = false,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -1449,15 +1450,16 @@ class ChatViewModel(
|
|||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
when (val result = fetchMessagePage(sessionId, newOffset, limit)) {
|
when (val result = fetchMessagePage(sessionId, newOffset, limit)) {
|
||||||
is NetworkResult.Success -> {
|
is NetworkResult.Success -> {
|
||||||
val older = mapServerMessages(sessionId, result.data.messages.orEmpty(), newOffset)
|
val returnedOffset = result.data.offset ?: newOffset
|
||||||
loadedMessageOffset = newOffset
|
val older = mapServerMessages(sessionId, result.data.messages.orEmpty(), returnedOffset)
|
||||||
|
loadedMessageOffset = returnedOffset
|
||||||
withContext(Dispatchers.IO) { repo.persistMessages(older, sessionId) }
|
withContext(Dispatchers.IO) { repo.persistMessages(older, sessionId) }
|
||||||
_uiState.update { current ->
|
_uiState.update { current ->
|
||||||
if (current.currentSessionId != sessionId) return@update current
|
if (current.currentSessionId != sessionId) return@update current
|
||||||
current.copy(
|
current.copy(
|
||||||
messages = (older + current.messages).distinctBy { it.id },
|
messages = (older + current.messages).distinctBy { it.id },
|
||||||
isLoadingOlder = false,
|
isLoadingOlder = false,
|
||||||
hasOlderMessages = newOffset > 0,
|
hasOlderMessages = returnedOffset < oldOffset && older.isNotEmpty() && returnedOffset > 0,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1559,7 +1561,14 @@ class ChatViewModel(
|
|||||||
offset: Int,
|
offset: Int,
|
||||||
limit: Int,
|
limit: Int,
|
||||||
) = withContext(Dispatchers.IO) {
|
) = 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(
|
private fun mapServerMessages(
|
||||||
@@ -1585,7 +1594,7 @@ class ChatViewModel(
|
|||||||
ChatMessage(
|
ChatMessage(
|
||||||
id = "rest-$sessionId-$globalIndex",
|
id = "rest-$sessionId-$globalIndex",
|
||||||
role = role,
|
role = role,
|
||||||
content = msg.content.orEmpty(),
|
content = msg.contentText,
|
||||||
timestamp = timestamp,
|
timestamp = timestamp,
|
||||||
isStreaming = false,
|
isStreaming = false,
|
||||||
)
|
)
|
||||||
|
|||||||
+62
-5
@@ -247,11 +247,11 @@ class HermesApiServiceMockWebServerTest {
|
|||||||
assertEquals(3, messages.size)
|
assertEquals(3, messages.size)
|
||||||
|
|
||||||
assertEquals("user", messages[0].role)
|
assertEquals("user", messages[0].role)
|
||||||
assertEquals("Hello Hermes", messages[0].content)
|
assertEquals("Hello Hermes", messages[0].contentText)
|
||||||
assertEquals("1718000000", messages[0].timestampText)
|
assertEquals("1718000000", messages[0].timestampText)
|
||||||
|
|
||||||
assertEquals("assistant", messages[1].role)
|
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", messages[2].role)
|
||||||
assertEquals("tool_execution", messages[2].type)
|
assertEquals("tool_execution", messages[2].type)
|
||||||
@@ -270,11 +270,68 @@ class HermesApiServiceMockWebServerTest {
|
|||||||
|
|
||||||
assertTrue(response.isSuccessful)
|
assertTrue(response.isSuccessful)
|
||||||
assertEquals(
|
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,
|
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
|
@Test
|
||||||
fun getSessionMessages_withNullRole_doesNotCrash() =
|
fun getSessionMessages_withNullRole_doesNotCrash() =
|
||||||
runBlocking {
|
runBlocking {
|
||||||
@@ -303,7 +360,7 @@ class HermesApiServiceMockWebServerTest {
|
|||||||
assertNotNull(body)
|
assertNotNull(body)
|
||||||
assertEquals(1, body!!.messages.size)
|
assertEquals(1, body!!.messages.size)
|
||||||
assertNull(body.messages[0].role)
|
assertNull(body.messages[0].role)
|
||||||
assertEquals("Plain content", body.messages[0].content)
|
assertEquals("Plain content", body.messages[0].contentText)
|
||||||
assertNull(body.messages[0].timestamp)
|
assertNull(body.messages[0].timestamp)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -383,7 +440,7 @@ class HermesApiServiceMockWebServerTest {
|
|||||||
val request = mockServer.takeRequest()
|
val request = mockServer.takeRequest()
|
||||||
assertEquals(
|
assertEquals(
|
||||||
"path should contain the session ID with slashes preserved",
|
"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,
|
request.path,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -32,6 +32,9 @@ import kotlinx.coroutines.test.advanceUntilIdle
|
|||||||
import kotlinx.coroutines.test.resetMain
|
import kotlinx.coroutines.test.resetMain
|
||||||
import kotlinx.coroutines.test.runTest
|
import kotlinx.coroutines.test.runTest
|
||||||
import kotlinx.coroutines.test.setMain
|
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.After
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
import org.junit.Assert.assertFalse
|
import org.junit.Assert.assertFalse
|
||||||
@@ -1677,7 +1680,7 @@ class ChatViewModelTest {
|
|||||||
verify { HermesWsClient.disconnect() }
|
verify { HermesWsClient.disconnect() }
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── History pagination guard (issue #674) ───────────────────────────────
|
// ── History pagination guard (issue #674 & #686) ───────────────────────────────
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* When the initial REST page returns empty messages, hasOlderMessages
|
* When the initial REST page returns empty messages, hasOlderMessages
|
||||||
@@ -1707,7 +1710,7 @@ class ChatViewModelTest {
|
|||||||
),
|
),
|
||||||
)
|
)
|
||||||
coEvery {
|
coEvery {
|
||||||
mockApi.getSessionMessages("session-456", any(), any())
|
mockApi.getSessionMessages("session-456", any(), any(), any())
|
||||||
} returns
|
} returns
|
||||||
retrofit2.Response.success(
|
retrofit2.Response.success(
|
||||||
com.m57.hermescontrol.data.model.SessionMessagesResponse(
|
com.m57.hermescontrol.data.model.SessionMessagesResponse(
|
||||||
@@ -1725,4 +1728,201 @@ class ChatViewModelTest {
|
|||||||
assertTrue(viewModel.uiState.value.messages.isEmpty())
|
assertTrue(viewModel.uiState.value.messages.isEmpty())
|
||||||
assertFalse(viewModel.uiState.value.isLoading)
|
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)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user