Skip to content

Commit faa68a1

Browse files
committed
fix(chat): chat-screen core correctness + states (plans/15 C)
- guard deep-jump against empty list; robust APPEND wait; reliable not-found snackbar; once-guarded re-resolve - mark-read only on resume/at-bottom with a frozen read-marker snapshot, restoring the unread divider and FAB badge - LazyColumn item keys (no scroll jumps); reactions aggregated in the VM by buffer (no SQL IN-clause overflow, no blank-frame churn) - loadState footer (loading/error/beginning) + empty-buffer state; imePadding - retry removes the failed row and re-sends (ACTION preserved) + delete; collapse JOIN/PART/QUIT runs into one pill; pending 'sending' indicator - member-count plural, localized typing subtitle + day separators
1 parent ddf6d6b commit faa68a1

12 files changed

Lines changed: 568 additions & 106 deletions

File tree

app/src/main/kotlin/io/github/trevarj/motd/data/db/Daos.kt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,11 @@ interface MessageDao {
135135
@Query("SELECT * FROM messages WHERE bufferId = :bufferId AND msgid = :msgid LIMIT 1")
136136
suspend fun byMsgid(bufferId: Long, msgid: String): MessageEntity?
137137

138+
// Delete a single row by primary key. Used to drop a failed local-echo row on retry/delete so
139+
// the resend does not leave a permanent duplicate "failed" bubble (plans/15 #10).
140+
@Query("DELETE FROM messages WHERE id = :id")
141+
suspend fun deleteById(id: Long)
142+
138143
/** 0-based reverse-list index: strict complement of pagingSource ORDER BY serverTime DESC, id DESC. */
139144
@Query(
140145
"""SELECT COUNT(*) FROM messages WHERE bufferId = :bufferId
@@ -192,6 +197,13 @@ interface ReactionDao {
192197
@Query("SELECT * FROM reactions WHERE bufferId = :bufferId AND targetMsgid IN (:msgids)")
193198
fun observeFor(bufferId: Long, msgids: List<String>): Flow<List<ReactionEntity>>
194199

200+
// Buffer-scoped observe with no per-msgid IN(...) list. Scrolling back accumulates >999 loaded
201+
// msgids, which would overflow SQLite's bind-variable limit in observeFor and crash; scoping by
202+
// bufferId keeps one stable query and the repository filters to the visible window in memory
203+
// (plans/15 #5). A buffer's reaction table is small relative to its message history.
204+
@Query("SELECT * FROM reactions WHERE bufferId = :bufferId")
205+
fun observeForBuffer(bufferId: Long): Flow<List<ReactionEntity>>
206+
195207
@Insert(onConflict = OnConflictStrategy.REPLACE)
196208
suspend fun upsert(r: ReactionEntity)
197209
}

app/src/main/kotlin/io/github/trevarj/motd/data/repo/MessageRepositoryImpl.kt

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,21 @@ class MessageRepositoryImpl @Inject constructor(
3131
pagingSourceFactory = { messageDao.pagingSource(bufferId) },
3232
).flow
3333

34+
// Kept for the frozen contract; scopes to a small, fixed msgid set (safe under 999 vars).
3435
override fun reactions(bufferId: Long, msgids: List<String>): Flow<List<ReactionEntity>> =
3536
reactionDao.observeFor(bufferId, msgids)
3637

38+
// Buffer-scoped observe: one stable query regardless of how far the user scrolls back, so the
39+
// per-msgid IN(...) list can never exceed SQLite's ~999 bind-variable cap (plans/15 #5). The
40+
// screen aggregates only the visible msgids from this stream.
41+
override fun reactionsForBuffer(bufferId: Long): Flow<List<ReactionEntity>> =
42+
reactionDao.observeForBuffer(bufferId)
43+
3744
override suspend fun byMsgid(bufferId: Long, msgid: String): MessageEntity? =
3845
messageDao.byMsgid(bufferId, msgid)
3946

4047
override suspend fun countNewerThan(bufferId: Long, serverTime: Long, id: Long): Int =
4148
messageDao.countNewerThan(bufferId, serverTime, id)
49+
50+
override suspend fun deleteMessage(id: Long) = messageDao.deleteById(id)
4251
}

app/src/main/kotlin/io/github/trevarj/motd/data/repo/Repositories.kt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,13 @@ interface MessageRepository {
3232
/** Paging 3 stream wired to ChatHistoryRemoteMediator (WP5 supplies mediator via factory). */
3333
fun messages(bufferId: Long): Flow<PagingData<MessageEntity>>
3434
fun reactions(bufferId: Long, msgids: List<String>): Flow<List<ReactionEntity>>
35+
/** Buffer-scoped reactions (no per-msgid IN list); callers filter to the visible window
36+
* in memory to avoid SQLite's bind-variable overflow on large windows (plans/15 #5, #18). */
37+
fun reactionsForBuffer(bufferId: Long): Flow<List<ReactionEntity>>
3538
suspend fun byMsgid(bufferId: Long, msgid: String): MessageEntity?
3639
suspend fun countNewerThan(bufferId: Long, serverTime: Long, id: Long): Int
40+
/** Delete a locally-stored row by id (failed-echo cleanup on retry/delete, plans/15 #10). */
41+
suspend fun deleteMessage(id: Long)
3742
}
3843

3944
/** WP4 injects this to build its Pager; WP1 stub-binds a no-op (immediate endOfPagination),

app/src/main/kotlin/io/github/trevarj/motd/ui/chat/ChatScreen.kt

Lines changed: 115 additions & 43 deletions
Large diffs are not rendered by default.

app/src/main/kotlin/io/github/trevarj/motd/ui/chat/ChatViewModel.kt

Lines changed: 97 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import dagger.hilt.android.lifecycle.HiltViewModel
99
import io.github.trevarj.motd.data.db.BufferEntity
1010
import io.github.trevarj.motd.data.db.MemberEntity
1111
import io.github.trevarj.motd.data.db.MessageEntity
12-
import io.github.trevarj.motd.data.db.ReactionEntity
1312
import io.github.trevarj.motd.data.repo.BufferRepository
1413
import io.github.trevarj.motd.data.repo.LinkPreview
1514
import io.github.trevarj.motd.data.repo.LinkPreviewRepository
@@ -25,17 +24,15 @@ import io.github.trevarj.motd.service.TypingTracker
2524
import io.github.trevarj.motd.ui.nav.ChatRoute
2625
import androidx.navigation.toRoute
2726
import kotlinx.coroutines.flow.Flow
28-
import kotlinx.coroutines.flow.MutableSharedFlow
2927
import kotlinx.coroutines.flow.MutableStateFlow
30-
import kotlinx.coroutines.flow.SharedFlow
3128
import kotlinx.coroutines.flow.SharingStarted
3229
import kotlinx.coroutines.flow.StateFlow
33-
import kotlinx.coroutines.flow.asSharedFlow
3430
import kotlinx.coroutines.flow.asStateFlow
3531
import kotlinx.coroutines.flow.combine
3632
import kotlinx.coroutines.flow.firstOrNull
3733
import kotlinx.coroutines.flow.stateIn
3834
import kotlinx.coroutines.launch
35+
import io.github.trevarj.motd.ui.components.ReactionChip
3936
import java.time.Instant
4037
import javax.inject.Inject
4138

@@ -51,6 +48,14 @@ data class ChatState(
5148
val connState: IrcClientState = IrcClientState.Disconnected,
5249
)
5350

51+
/**
52+
* Wire text for resending a failed row. An ACTION is stored with its `/me ` prefix stripped, so
53+
* re-prefix it; the manager rewrites `/me ` back into a CTCP ACTION. Non-ACTION kinds resend
54+
* verbatim (plans/15 #10).
55+
*/
56+
fun resendText(kind: io.github.trevarj.motd.data.db.MessageKind, text: String): String =
57+
if (kind == io.github.trevarj.motd.data.db.MessageKind.ACTION) "/me $text" else text
58+
5459
@HiltViewModel
5560
class ChatViewModel @Inject constructor(
5661
private val savedStateHandle: SavedStateHandle,
@@ -95,9 +100,38 @@ class ChatViewModel @Inject constructor(
95100
)
96101
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), ChatState())
97102

98-
/** Reactions for the currently visible msgids; the screen supplies the id set. */
99-
fun reactions(msgids: List<String>): Flow<List<ReactionEntity>> =
100-
messageRepository.reactions(bufferId, msgids)
103+
// --- reactions aggregation (plans/15 #5, #18) ---
104+
105+
/** Msgids currently visible in the paging window; the screen keeps this current. */
106+
private val visibleMsgids = MutableStateFlow<List<String>>(emptyList())
107+
108+
fun setVisibleMsgids(ids: List<String>) {
109+
if (visibleMsgids.value != ids) visibleMsgids.value = ids
110+
}
111+
112+
/**
113+
* Reaction chips keyed by msgid, aggregated in the VM so the value survives across message
114+
* arrivals (no blank frame from an emptyList re-seed) and picks up echo-confirm msgid swaps as
115+
* soon as [visibleMsgids] changes. Buffer-scoped reactions avoid the SQLite IN(...) overflow
116+
* (plans/15 #5); we filter to the visible window here.
117+
*/
118+
val reactionChips: StateFlow<Map<String, List<ReactionChip>>> = combine(
119+
messageRepository.reactionsForBuffer(bufferId),
120+
visibleMsgids,
121+
connState,
122+
) { all, visible, conn ->
123+
val visibleSet = visible.toHashSet()
124+
val relevant = all.filter { it.targetMsgid in visibleSet }
125+
val myNick = (conn as? IrcClientState.Ready)?.nick
126+
aggregateReactions(relevant, myNick, nickNormalizer())
127+
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyMap())
128+
129+
// --- read marker snapshot (plans/15 #2) ---
130+
131+
// Frozen on buffer entry so the "— New messages —" divider and the FAB unread badge keep a
132+
// stable boundary instead of flashing/vanishing as markRead advances the live marker.
133+
private val _readMarkerSnapshot = MutableStateFlow<Long?>(null)
134+
val readMarkerSnapshot: StateFlow<Long?> = _readMarkerSnapshot.asStateFlow()
101135

102136
// --- lifecycle: foreground tracker + mark-read (plans/07) ---
103137

@@ -133,8 +167,19 @@ class ChatViewModel @Inject constructor(
133167
connectionManager.sendReact(bufferId, msgid, emoji)
134168
}
135169

170+
/**
171+
* Retry a failed message: drop the old failed row first (no permanent duplicate), then resend.
172+
* An ACTION is stored with its display text stripped of the `/me ` prefix, so re-prefix it to
173+
* resend as an ACTION rather than a plain PRIVMSG (plans/15 #10).
174+
*/
136175
fun retry(message: MessageEntity) = viewModelScope.launch {
137-
connectionManager.sendMessage(bufferId, message.text, message.replyToMsgid)
176+
messageRepository.deleteMessage(message.id)
177+
connectionManager.sendMessage(bufferId, resendText(message.kind, message.text), message.replyToMsgid)
178+
}
179+
180+
/** Delete a failed local row without resending (action-sheet delete affordance, plans/15 #10). */
181+
fun deleteFailed(message: MessageEntity) = viewModelScope.launch {
182+
messageRepository.deleteMessage(message.id)
138183
}
139184

140185
suspend fun linkPreview(url: String): LinkPreview? = linkPreviewRepository.preview(url)
@@ -214,24 +259,47 @@ class ChatViewModel @Inject constructor(
214259
/** Resolved jump target (index + optional highlight msgid); null when nothing to jump to. */
215260
val jumpTarget: StateFlow<ChatJumpResolver.Result.Target?> = _jumpTarget.asStateFlow()
216261

217-
private val _jumpFailed = MutableSharedFlow<Unit>(extraBufferCapacity = 1)
218-
/** Emits when a jump could not be resolved (cap miss / not loaded) → snackbar. */
219-
val jumpFailed: SharedFlow<Unit> = _jumpFailed.asSharedFlow()
262+
// Nullable-event StateFlow instead of a replay-less SharedFlow so a NotFound resolved in init
263+
// (before the screen subscribes) is not dropped; the UI clears it via [onJumpFailedShown]
264+
// (plans/15 #13).
265+
private val _jumpFailed = MutableStateFlow(false)
266+
val jumpFailed: StateFlow<Boolean> = _jumpFailed.asStateFlow()
267+
268+
// Re-resolve is allowed exactly once per navigation; a second index shift falls through to the
269+
// not-loaded snackbar rather than looping (plans/15 #12).
270+
private var reresolveUsed = false
220271

221272
init {
222273
if (jumpTime > 0 && savedStateHandle.get<Boolean>(JUMP_CONSUMED_KEY) != true) {
223274
savedStateHandle[JUMP_CONSUMED_KEY] = true
224275
resolveJump()
225276
}
277+
// Freeze the read-marker once, on the first buffer emission, so the unread divider/badge
278+
// stay put instead of collapsing as markRead advances the live marker (plans/15 #2).
279+
viewModelScope.launch {
280+
_readMarkerSnapshot.value = bufferRepository.observeBuffer(bufferId).firstOrNull()?.readMarkerTime
281+
}
226282
}
227283

228284
private fun resolveJump() = viewModelScope.launch {
229285
// The buffer name (chathistory target) may not be in `state` yet on first composition;
230286
// read it directly from the repo so the AROUND fallback has a target.
231287
val name = bufferRepository.observeBuffer(bufferId).firstOrNull()?.name
288+
publishResolve(name)
289+
}
290+
291+
private suspend fun publishResolve(name: String?) {
232292
when (val r = resolver.resolve(bufferId, jumpMsgid, jumpTime, name)) {
233-
is ChatJumpResolver.Result.Target -> _jumpTarget.value = r
234-
ChatJumpResolver.Result.NotFound -> _jumpFailed.tryEmit(Unit)
293+
is ChatJumpResolver.Result.Target -> {
294+
// Force a distinct emission so the screen's LaunchedEffect(jumpTarget) always
295+
// re-runs, even when the re-resolved index equals the previous one (plans/15 #12).
296+
_jumpTarget.value = null
297+
_jumpTarget.value = r
298+
}
299+
ChatJumpResolver.Result.NotFound -> {
300+
_jumpTarget.value = null
301+
_jumpFailed.value = true
302+
}
235303
}
236304
}
237305

@@ -240,16 +308,24 @@ class ChatViewModel @Inject constructor(
240308
_jumpTarget.value = null
241309
}
242310

243-
/** Re-resolve the same target once when a live message shifted indices mid-jump. */
311+
/** Screen calls this after showing the not-loaded snackbar so it does not re-fire. */
312+
fun onJumpFailedShown() {
313+
_jumpFailed.value = false
314+
}
315+
316+
/**
317+
* Re-resolve the same target once when a live message shifted indices mid-jump. The single-shot
318+
* guard means the screen can always call [onJumpHandled] after it; a repeat request just clears
319+
* the target so the not-loaded path takes over (plans/15 #12).
320+
*/
244321
fun reresolveJumpOnce() = viewModelScope.launch {
245-
val name = state.value.buffer?.name
246-
when (val r = resolver.resolve(bufferId, jumpMsgid, jumpTime, name)) {
247-
is ChatJumpResolver.Result.Target -> _jumpTarget.value = r
248-
ChatJumpResolver.Result.NotFound -> {
249-
_jumpTarget.value = null
250-
_jumpFailed.tryEmit(Unit)
251-
}
322+
if (reresolveUsed) {
323+
_jumpTarget.value = null
324+
_jumpFailed.value = true
325+
return@launch
252326
}
327+
reresolveUsed = true
328+
publishResolve(state.value.buffer?.name)
253329
}
254330

255331
/**

0 commit comments

Comments
 (0)