From 12ddef09a0994e2620b07417fea7eafe4ae6cdbf Mon Sep 17 00:00:00 2001 From: m57 <41645758+Hy4ri@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:26:13 +0300 Subject: [PATCH] refactor(chat): single owner for display kinds and REST row ids (#1364) --- .../data/local/ChatMessageEntity.kt | 4 +- .../ui/chat/ChatClarifyDelegate.kt | 2 +- .../hermescontrol/ui/chat/ChatListEntries.kt | 2 +- .../m57/hermescontrol/ui/chat/ChatMessage.kt | 2 +- .../ui/chat/ChatMessageIdentity.kt | 40 +++++++++++++++++++ .../ui/chat/ChatServerMessageMapper.kt | 6 +-- .../ui/chat/ChatTranscriptMerge.kt | 18 ++++----- .../hermescontrol/ui/chat/ChatViewModel.kt | 6 +-- .../ui/chat/fullbleed/FullBleedTurns.kt | 9 ++--- .../ui/chat/fullbleed/TimelineMarkerChip.kt | 17 ++++---- .../ui/chat/ChatMessageIdentityTest.kt | 34 ++++++++++++++++ 11 files changed, 107 insertions(+), 33 deletions(-) create mode 100644 app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentity.kt create mode 100644 app/src/test/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentityTest.kt diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt index 9b645e7a..5ea8ee9f 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt @@ -66,7 +66,9 @@ internal fun canonicalMessageOrder( id: String, sessionId: String, ): Long? { - val prefix = "rest-$sessionId-" + val prefix = + com.m57.hermescontrol.ui.chat.RestMessageId + .sessionPrefix(sessionId) if (!id.startsWith(prefix)) return null val suffix = id.removePrefix(prefix) return suffix.takeIf { it.isNotEmpty() && it.all { char -> char in '0'..'9' } }?.toLongOrNull() diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatClarifyDelegate.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatClarifyDelegate.kt index 92bce3f6..aa031ec6 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatClarifyDelegate.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatClarifyDelegate.kt @@ -84,7 +84,7 @@ class ChatClarifyDelegate( ChatMessage( role = MessageRole.USER, content = displayContent, - displayKind = "clarify_response", + displayKind = DisplayKind.CLARIFY_RESPONSE, ) uiState.update { state -> diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatListEntries.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatListEntries.kt index 53d45508..69732b47 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatListEntries.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatListEntries.kt @@ -36,7 +36,7 @@ private const val MAX_ITERATIONS_SYSTEM_MARKER = */ internal fun ChatMessage.isUserTurnBoundary(): Boolean = role == MessageRole.USER && - (displayKind == null || displayKind == "steer" || displayKind == "clarify_response") && + (displayKind == null || displayKind == DisplayKind.STEER || displayKind == DisplayKind.CLARIFY_RESPONSE) && !content.startsWith(MAX_ITERATIONS_SYSTEM_MARKER) /** diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt index badd3eb2..78fc4eaa 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt @@ -117,7 +117,7 @@ data class ChatMessage( /** Cached REST rows already carry their canonical identity in the persisted primary key. */ internal val ChatMessage.canonicalRestId: String? - get() = restId ?: id.takeIf { it.startsWith("rest-") } + get() = restId ?: id.takeIf(RestMessageId::isRest) /** A persisted RUNNING snapshot is not evidence of current tool activity. */ internal val ChatMessage.isToolRunning: Boolean diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentity.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentity.kt new file mode 100644 index 00000000..a4d782c3 --- /dev/null +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentity.kt @@ -0,0 +1,40 @@ +package com.m57.hermescontrol.ui.chat + +/** + * Known `display_kind` wire values (#1336). [ChatMessage.displayKind] stays a raw + * string because the backend can add kinds; these names keep the ones the app + * interprets in one place. + */ +internal object DisplayKind { + const val STEER = "steer" + const val CLARIFY_RESPONSE = "clarify_response" + const val LOCAL_FEEDBACK = "local_feedback" + const val MODEL_SWITCH = "model_switch" + const val PERSONALITY_SWITCH = "personality_switch" + const val AUTO_CONTINUE = "auto_continue" + const val ASYNC_DELEGATION_COMPLETE = "async_delegation_complete" + const val SKILL_INVOCATION = "skill_invocation" + const val INTERNAL_NOTIFICATION = "internal_notification" + const val MAX_ITERATIONS_REACHED = "max_iterations_reached" + + /** Kinds that tag real user or local rows rather than timeline markers. */ + val nonMarkerKinds = setOf(STEER, CLARIFY_RESPONSE, LOCAL_FEEDBACK) +} + +/** + * The single owner of the REST transcript row-id format (#1336): + * `rest--`, where rowKey is the server row id under newest-anchored + * paging or an absolute position under legacy paging. + */ +internal object RestMessageId { + private const val PREFIX = "rest-" + + fun of( + sessionId: String, + rowKey: Any, + ): String = "$PREFIX$sessionId-$rowKey" + + fun sessionPrefix(sessionId: String): String = "$PREFIX$sessionId-" + + fun isRest(id: String): Boolean = id.startsWith(PREFIX) +} diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt index 56040ff8..53bfd0d8 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt @@ -51,9 +51,9 @@ internal fun mapServerMessages( fun restIdAt(index: Int): String = if (stableRowIds) { - "rest-$sessionId-${requireNotNull(messages[index].id) { "Latest transcript row has no stable id" }}" + RestMessageId.of(sessionId, requireNotNull(messages[index].id) { "Latest transcript row has no stable id" }) } else { - "rest-$sessionId-${offset + index}" + RestMessageId.of(sessionId, offset + index) } val wsCompletionIdByRestIndex = mutableMapOf() @@ -278,7 +278,7 @@ internal fun mapServerMessages( } // REST echoes must not reserve a match before the richer WS copy of that tool. - val liveTools = liveMessages.filter { it.role == MessageRole.TOOL && !it.id.startsWith("rest-") } + val liveTools = liveMessages.filter { it.role == MessageRole.TOOL && !RestMessageId.isRest(it.id) } val mappedTools = mapped.filter { it.role == MessageRole.TOOL } val matches = matchTranscriptMessages(mappedTools, liveTools) val toolsById = mappedTools.indices.associate { index -> mappedTools[index].id to matches[index] } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatTranscriptMerge.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatTranscriptMerge.kt index afe8769e..9ebae364 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatTranscriptMerge.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatTranscriptMerge.kt @@ -174,12 +174,12 @@ internal class TranscriptComparison( // Rows generated only by Mobile never have a canonical REST counterpart. // Do not let coincidentally identical server text claim their identity. if (a.role == MessageRole.ASSISTANT && - (a.displayKind == "local_feedback" || b.displayKind == "local_feedback") + (a.displayKind == DisplayKind.LOCAL_FEEDBACK || b.displayKind == DisplayKind.LOCAL_FEEDBACK) ) { return false } if (a.role == MessageRole.USER && - (a.displayKind == "clarify_response" || b.displayKind == "clarify_response") + (a.displayKind == DisplayKind.CLARIFY_RESPONSE || b.displayKind == DisplayKind.CLARIFY_RESPONSE) ) { return false } @@ -323,8 +323,8 @@ internal fun dedupeCachedMessages( confirmedOnly: Boolean = false, ): List { val unique = messages.dedupeById() - val rest = unique.filter { it.id.startsWith("rest-") } - val live = unique.filterNot { it.id.startsWith("rest-") } + val rest = unique.filter { RestMessageId.isRest(it.id) } + val live = unique.filterNot { RestMessageId.isRest(it.id) } if (rest.isEmpty() || live.isEmpty()) return unique val matches = matchTranscriptMessages(rest, live).mapIndexed { index, match -> @@ -405,7 +405,7 @@ internal fun mergeCachedTranscriptPage( ) } - match.id.startsWith("rest-") && !message.id.startsWith("rest-") -> { + RestMessageId.isRest(match.id) && !RestMessageId.isRest(message.id) -> { message } @@ -463,7 +463,7 @@ internal fun mergeTranscriptWithLive( ) } - preserveLiveIds && match != null && !match.id.startsWith("rest-") -> { + preserveLiveIds && match != null && !RestMessageId.isRest(match.id) -> { val mergedContent = if (match.role == MessageRole.ASSISTANT && ChatVerifierFooter.split(match.content) != null && @@ -586,8 +586,8 @@ private fun List.inTranscriptOrder( internal fun ChatMessage.isPermanentlyLocal(): Boolean = role == MessageRole.SYSTEM || - (role == MessageRole.USER && (content.startsWith("/") || displayKind == "clarify_response")) || - (role == MessageRole.ASSISTANT && displayKind == "local_feedback") + (role == MessageRole.USER && (content.startsWith("/") || displayKind == DisplayKind.CLARIFY_RESPONSE)) || + (role == MessageRole.ASSISTANT && displayKind == DisplayKind.LOCAL_FEEDBACK) internal fun ChatMessage.isSessionStartMarker(): Boolean = role == MessageRole.SYSTEM && (content == "Session created" || content == "Session branched") @@ -635,6 +635,6 @@ internal fun mergeIncrementalTranscriptPage( internal fun serverMessageIndex( id: String, sessionId: String, -): Int? = id.removePrefix("rest-$sessionId-").takeIf { it != id }?.toIntOrNull() +): Int? = id.removePrefix(RestMessageId.sessionPrefix(sessionId)).takeIf { it != id }?.toIntOrNull() internal fun List.dedupeById(): List = associateBy { it.id }.values.toList() 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 59284f8b..01ba89f1 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 @@ -4136,7 +4136,7 @@ class ChatViewModel( ChatMessage( role = MessageRole.ASSISTANT, content = text, - displayKind = "local_feedback", + displayKind = DisplayKind.LOCAL_FEEDBACK, ) _uiState.update { it.copy(messages = it.messages + msg) } @@ -4748,7 +4748,7 @@ class ChatViewModel( ) } if (!valid()) return@launch - val targetId = "rest-$sessionId-$rowId" + val targetId = RestMessageId.of(sessionId, rowId) if (page.none { it.id == targetId || it.canonicalRestId == targetId }) { _timelineState.update { it.copy(windowErrorMessage = "The selected prompt is no longer available.") @@ -5106,7 +5106,7 @@ class ChatViewModel( // A mapped page can reuse a live WS message. Never overwrite its newer persisted // version with the snapshot used for mapping; WS owns persistence of those IDs. val pageIds = page.mapNotNull { it.canonicalRestId }.toSet() - val aliases = _uiState.value.messages.filter { it.restId in pageIds && !it.id.startsWith("rest-") } + val aliases = _uiState.value.messages.filter { it.restId in pageIds && !RestMessageId.isRest(it.id) } withContext(historyDispatcher) { repo.persistMessages( page.mapNotNull { message -> message.canonicalRestId?.let { message.copy(id = it, restId = null) } }, diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedTurns.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedTurns.kt index cf5a8057..74c96e32 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedTurns.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedTurns.kt @@ -1,6 +1,7 @@ package com.m57.hermescontrol.ui.chat.fullbleed import com.m57.hermescontrol.ui.chat.ChatMessage +import com.m57.hermescontrol.ui.chat.DisplayKind import com.m57.hermescontrol.ui.chat.MessageRole import com.m57.hermescontrol.ui.chat.SearchMatch import com.m57.hermescontrol.ui.chat.SearchTarget @@ -59,11 +60,7 @@ internal fun ChatMessage.isSyntheticSystemRow(): Boolean = displayKind == null && content.startsWith(MAX_ITERATIONS_SYSTEM_MARKER) -internal fun ChatMessage.isTimelineMarker(): Boolean = - displayKind != null && - displayKind != "steer" && - displayKind != "clarify_response" && - displayKind != "local_feedback" +internal fun ChatMessage.isTimelineMarker(): Boolean = displayKind != null && displayKind !in DisplayKind.nonMarkerKinds /** * Split a flat message list into turns for the full-bleed renderer. @@ -99,7 +96,7 @@ fun groupIntoTurns(messages: List): List { message.isSyntheticSystemRow() -> { agentEntries += - AgentEntry.SystemEvent(message.copy(displayKind = "max_iterations_reached")) + AgentEntry.SystemEvent(message.copy(displayKind = DisplayKind.MAX_ITERATIONS_REACHED)) } message.role == MessageRole.USER -> { diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/TimelineMarkerChip.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/TimelineMarkerChip.kt index 88c2d033..f2168a95 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/TimelineMarkerChip.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/TimelineMarkerChip.kt @@ -15,6 +15,7 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import com.m57.hermescontrol.R import com.m57.hermescontrol.ui.chat.ChatMessage +import com.m57.hermescontrol.ui.chat.DisplayKind /** * Friendly label resource for a `display_kind` timeline marker (issue #904). @@ -24,13 +25,13 @@ import com.m57.hermescontrol.ui.chat.ChatMessage */ internal fun timelineMarkerLabelRes(kind: String?): Int? = when (kind) { - "model_switch" -> R.string.timeline_marker_model_switch - "personality_switch" -> R.string.timeline_marker_personality_switch - "auto_continue" -> R.string.timeline_marker_auto_continue - "async_delegation_complete" -> R.string.timeline_marker_delegation_complete - "skill_invocation" -> R.string.timeline_marker_skill_invocation - "internal_notification" -> R.string.timeline_marker_internal_notification - "max_iterations_reached" -> R.string.timeline_marker_max_iterations + DisplayKind.MODEL_SWITCH -> R.string.timeline_marker_model_switch + DisplayKind.PERSONALITY_SWITCH -> R.string.timeline_marker_personality_switch + DisplayKind.AUTO_CONTINUE -> R.string.timeline_marker_auto_continue + DisplayKind.ASYNC_DELEGATION_COMPLETE -> R.string.timeline_marker_delegation_complete + DisplayKind.SKILL_INVOCATION -> R.string.timeline_marker_skill_invocation + DisplayKind.INTERNAL_NOTIFICATION -> R.string.timeline_marker_internal_notification + DisplayKind.MAX_ITERATIONS_REACHED -> R.string.timeline_marker_max_iterations else -> null } @@ -63,7 +64,7 @@ internal fun TimelineMarkerChip( ) { val labelRes = timelineMarkerLabelRes(message.displayKind) ?: return val text = - if (message.displayKind == "model_switch") { + if (message.displayKind == DisplayKind.MODEL_SWITCH) { val model = markerModelFromContent(message.content) if (model != null) { stringResource(R.string.timeline_marker_model_switch_to, model) diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentityTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentityTest.kt new file mode 100644 index 00000000..a612d9a7 --- /dev/null +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatMessageIdentityTest.kt @@ -0,0 +1,34 @@ +package com.m57.hermescontrol.ui.chat + +import com.m57.hermescontrol.data.local.canonicalMessageOrder +import com.m57.hermescontrol.ui.chat.fullbleed.isTimelineMarker +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** Issue #1336: one owner for the REST row-id format and the known display kinds. */ +class ChatMessageIdentityTest { + @Test + fun restIdsRoundTripThroughTheSingleFormatOwner() { + val id = RestMessageId.of("s-1", 42) + + assertEquals("rest-s-1-42", id) + assertTrue(RestMessageId.isRest(id)) + assertFalse(RestMessageId.isRest("ws-42")) + assertEquals(42L, canonicalMessageOrder(id, "s-1")) + assertNull(canonicalMessageOrder(RestMessageId.of("other", 42), "s-1")) + assertEquals(id, ChatMessage(id = id, role = MessageRole.USER, content = "").canonicalRestId) + } + + @Test + fun onlyUnknownOrMarkerKindsAreTimelineMarkers() { + fun kind(value: String?) = ChatMessage(role = MessageRole.USER, content = "x", displayKind = value) + + DisplayKind.nonMarkerKinds.forEach { assertFalse(it, kind(it).isTimelineMarker()) } + assertTrue(kind(DisplayKind.MODEL_SWITCH).isTimelineMarker()) + assertTrue(kind("future_backend_kind").isTimelineMarker()) + assertFalse(kind(null).isTimelineMarker()) + } +}