mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-10-23 00:02:28 +00:00
android, desktop: fix chat preview and unread counter updating wrong chat (#7072)
* android, desktop: fix chat preview and unread counter updating wrong chat While "Chat with admins", a member support chat or the reports view was open, ChatsContext.addChatItem in the secondary context looked up the chat's index in its own list but read and wrote the main chat list at that index (chatsContext.chats[i]), so an unrelated chat in the main list got the message as its preview and its unread count went up. Use the context's own list at all four sites again. The main-list qualifier had also made a pending invitee's sent support messages reach the group's preview in the main list (at a secondary-list index), so send that update to the main list explicitly: addSentChatItem passes a sent item to the main chat list context as well when it is sent from a secondary view and belongs in the main list (inMainChatList, the same rule addChatItem uses for previews). Count unread stats only in the main chat list context, so the revert does not make the open secondary view count main-list messages in its own list. * plans: wrong-chat preview and unread fix
This commit is contained in:
1 parent
87e3f8c3f6
commit
cc3f49f794
4 files changed
+176
-9
No files matched your search
+16
-6
@@ -414,6 +414,13 @@ object ChatModel {
|
||||
}
|
||||
}
|
||||
|
||||
suspend fun addSentChatItem(activeCtx: ChatsContext, rhId: Long?, cInfo: ChatInfo, cItem: ChatItem) {
|
||||
activeCtx.addChatItem(rhId, cInfo, cItem)
|
||||
if (activeCtx.secondaryContextFilter != null && cInfo.inMainChatList) {
|
||||
chatsContext.addChatItem(rhId, cInfo, cItem)
|
||||
}
|
||||
}
|
||||
|
||||
// Spec: spec/state.md#ChatsContext
|
||||
class ChatsContext(val secondaryContextFilter: SecondaryContextFilter?) {
|
||||
val chats = mutableStateOf(SnapshotStateList<Chat>())
|
||||
@@ -584,9 +591,9 @@ object ChatModel {
|
||||
val i = getChatIndex(rhId, cInfo.id)
|
||||
val chat: Chat
|
||||
if (i >= 0) {
|
||||
chat = chatsContext.chats[i]
|
||||
chat = chats[i]
|
||||
// update preview (for chat from main scope to show new items for invitee in pending status)
|
||||
if (cInfo.groupChatScope() == null || cInfo.groupInfo_?.membership?.memberPending == true) {
|
||||
if (cInfo.inMainChatList) {
|
||||
val newPreviewItem = when (cInfo) {
|
||||
is ChatInfo.Group -> {
|
||||
val currentPreviewItem = chat.chatItems.firstOrNull()
|
||||
@@ -604,20 +611,20 @@ object ChatModel {
|
||||
else -> cItem
|
||||
}
|
||||
val wasUnread = chat.unreadTag
|
||||
chatsContext.chats[i] = chat.copy(
|
||||
chats[i] = chat.copy(
|
||||
chatItems = arrayListOf(newPreviewItem),
|
||||
chatStats =
|
||||
if (cItem.meta.itemStatus is CIStatus.RcvNew) {
|
||||
if (cItem.meta.itemStatus is CIStatus.RcvNew && secondaryContextFilter == null) {
|
||||
increaseUnreadCounter(rhId, currentUser.value!!)
|
||||
chat.chatStats.copy(unreadCount = chat.chatStats.unreadCount + 1, unreadMentions = if (cItem.meta.userMention) chat.chatStats.unreadMentions + 1 else chat.chatStats.unreadMentions)
|
||||
} else
|
||||
chat.chatStats
|
||||
)
|
||||
updateChatTagReadInPrimaryContext(chatsContext.chats[i], wasUnread)
|
||||
updateChatTagReadInPrimaryContext(chats[i], wasUnread)
|
||||
}
|
||||
// pop chat
|
||||
if (appPlatform.isDesktop && cItem.chatDir.sent) {
|
||||
reorderChat(chatsContext.chats[i], 0)
|
||||
reorderChat(chats[i], 0)
|
||||
} else {
|
||||
popChatCollector.throttlePopChat(chat.remoteHostId, chat.id, currentPosition = i)
|
||||
}
|
||||
@@ -1872,6 +1879,9 @@ sealed class ChatInfo: SomeChat, NamedChat {
|
||||
|
||||
val isChannel: Boolean
|
||||
get() = groupInfo_?.useRelays == true
|
||||
|
||||
val inMainChatList: Boolean
|
||||
get() = groupChatScope() == null || groupInfo_?.membership?.memberPending == true
|
||||
}
|
||||
|
||||
@Serializable
|
||||
|
||||
+2
-2
@@ -599,7 +599,7 @@ fun ComposeView(
|
||||
if (!chatItems.isNullOrEmpty()) {
|
||||
chatItems.forEach { aChatItem ->
|
||||
withContext(Dispatchers.Main) {
|
||||
chatsCtx.addChatItem(chat.remoteHostId, aChatItem.chatInfo, aChatItem.chatItem)
|
||||
chatModel.addSentChatItem(chatsCtx, chat.remoteHostId, aChatItem.chatInfo, aChatItem.chatItem)
|
||||
}
|
||||
}
|
||||
return chatItems.first().chatItem
|
||||
@@ -741,7 +741,7 @@ fun ComposeView(
|
||||
|
||||
withContext(Dispatchers.Main) {
|
||||
chatItems?.forEach { chatItem ->
|
||||
chatsCtx.addChatItem(rhId, chat.chatInfo, chatItem)
|
||||
chatModel.addSentChatItem(chatsCtx, rhId, chat.chatInfo, chatItem)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+1
-1
@@ -494,7 +494,7 @@ fun sendCommandMsg(chatsCtx: ChatModel.ChatsContext, chat: Chat, msg: String) {
|
||||
if (!chatItems.isNullOrEmpty()) {
|
||||
chatItems.forEach { aChatItem ->
|
||||
withContext(Dispatchers.Main) {
|
||||
chatsCtx.addChatItem(chat.remoteHostId, aChatItem.chatInfo, aChatItem.chatItem)
|
||||
chatModel.addSentChatItem(chatsCtx, chat.remoteHostId, aChatItem.chatInfo, aChatItem.chatItem)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,157 @@
|
||||
# Chat list: message preview and unread counter applied to the wrong chat
|
||||
|
||||
**PR:** #7072 · **Files:** `ChatModel.kt` (`ChatsContext.addChatItem`, `ChatModel.addSentChatItem`, `ChatInfo.inMainChatList`), `ComposeView.kt`, `FramedItemView.kt`
|
||||
|
||||
## Symptom
|
||||
|
||||
While a secondary chat view is open — "Chat with admins", "chat with member", or a
|
||||
group's reports view — an incoming message could land on an unrelated chat in the
|
||||
main list: that chat took the message as its last-message preview and its unread
|
||||
badge incremented. The wrong preview stays until that chat's next message or until the chat list is reloaded; the wrong
|
||||
unread count until the chat is opened (opening reloads its stats).
|
||||
|
||||
## Root cause
|
||||
|
||||
`ChatsContext` is a nested class with two live instances: the primary chat list
|
||||
(`ChatModel.chatsContext`, shown in the main list) and a secondary context created
|
||||
per reports / member-support view (set up in `ChatListNavLinkView.kt`, `MemberSupportChatView.kt`,
|
||||
`GroupReportsView.kt`). `SimpleXAPI` dispatches every incoming item to **both**.
|
||||
|
||||
Inside `addChatItem`:
|
||||
|
||||
```kotlin
|
||||
val i = getChatIndex(rhId, cInfo.id) // searches THIS context's own list
|
||||
...
|
||||
chat = chatsContext.chats[i] // but reads the PRIMARY singleton's list
|
||||
chatsContext.chats[i] = chat.copy(...) // ...and writes it
|
||||
```
|
||||
|
||||
`getChatIndex` is a member of `ChatsContext`, so `chats` resolves to the receiver
|
||||
context. `chatsContext` (unqualified) is a member of the enclosing `object ChatModel`
|
||||
and always resolves to the **primary** instance. On the secondary context the two
|
||||
lists hold different chats in a different order, so `i` — a secondary-list index —
|
||||
points at an unrelated chat in the primary list, which then gets clobbered.
|
||||
|
||||
### Why the indices diverge
|
||||
|
||||
The secondary context's chat list starts empty (only the primary is seeded on load) and
|
||||
grows by `addChat` at index 0 as chats receive messages. The two lists fall out of step
|
||||
when a change reaches only one of them, for example:
|
||||
- a message sent in "Chat with admins" creates the group's entry only in the secondary
|
||||
list;
|
||||
- a chat added to or removed from the primary list only (a group invitation, a contact
|
||||
request);
|
||||
- a burst of messages within 2 s: the primary list's pop is throttled, while the
|
||||
secondary `addChat` inserts at index 0 at once.
|
||||
|
||||
Until #7404 is merged, the secondary pop collector rebuilds the secondary list from the primary
|
||||
one, which re-aligns them now and then, so the bug is intermittent.
|
||||
|
||||
### How it was introduced
|
||||
|
||||
Commit `b97e1e0f1` (#5909, "show new messages from support scope in main chat preview
|
||||
when invitee is pending") changed `chats[i]` → `chatsContext.chats[i]` at four sites
|
||||
while broadening the preview guard with `|| memberPending`. The guard change was the
|
||||
feature for received items; the `chatsContext.` qualifier also made the invitee's sent
|
||||
support messages reach the main list, but at an index from the secondary list, which
|
||||
on the secondary context updates the wrong chat.
|
||||
|
||||
## The fix
|
||||
|
||||
Use the receiver context's own list at all four sites, keeping the broadened guard:
|
||||
|
||||
```diff
|
||||
- chat = chatsContext.chats[i]
|
||||
+ chat = chats[i]
|
||||
- chatsContext.chats[i] = chat.copy(
|
||||
+ chats[i] = chat.copy(
|
||||
- updateChatTagReadInPrimaryContext(chatsContext.chats[i], wasUnread)
|
||||
+ updateChatTagReadInPrimaryContext(chats[i], wasUnread)
|
||||
- reorderChat(chatsContext.chats[i], 0)
|
||||
+ reorderChat(chats[i], 0)
|
||||
```
|
||||
|
||||
This is a byte-for-byte revert of the qualifier from `b97e1e0f1`, with the feature's
|
||||
guard preserved. `chats[i]` get/set is the idiom used elsewhere in the class.
|
||||
|
||||
## The revert alone would break #5909
|
||||
|
||||
A pending invitee's own **sent** support message reaches only the **active** context —
|
||||
`ComposeView` send/forward and `FramedItemView` command-send all call
|
||||
`chatsCtx.addChatItem`, and in a member-support chat `chatsCtx` is the *secondary*
|
||||
context. Before this change that call wrote `chatsContext.chats[i]`, i.e. the primary
|
||||
list, so the group's main-list preview did update — but only as a side effect of the
|
||||
bug, at whatever index happened to line up.
|
||||
|
||||
Reverting the four sites therefore removes it: the secondary updates its own list and
|
||||
the primary is never touched, so the invitee's sent support messages stop appearing as
|
||||
the group's preview. Confirmed by testing during review.
|
||||
|
||||
So the revert is paired with an explicit dispatch, which does the same thing on purpose
|
||||
and at the right index:
|
||||
|
||||
```kotlin
|
||||
suspend fun addSentChatItem(activeCtx: ChatsContext, rhId: Long?, cInfo: ChatInfo, cItem: ChatItem) {
|
||||
activeCtx.addChatItem(rhId, cInfo, cItem)
|
||||
if (activeCtx.secondaryContextFilter != null && cInfo.inMainChatList) {
|
||||
chatsContext.addChatItem(rhId, cInfo, cItem)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`ChatInfo.inMainChatList` (`groupChatScope() == null || membership.memberPending`) is
|
||||
the rule `addChatItem` applies to decide whether an item updates the preview of a chat
|
||||
already in the list; both places use the property.
|
||||
|
||||
Cross-context dispatch stays with the **caller**, matching how `processReceivedMsg`
|
||||
hands received items to both contexts and how `ChatView` handles deletes. No
|
||||
`ChatsContext` method writes the other context — the invariant this fix restores
|
||||
(the pop collector still reads the primary list until #7404).
|
||||
|
||||
Passing the scoped `cInfo` to the primary is safe: when the group is in the main list
|
||||
`updateChatInfo` strips the scope before storing it (when it isn't, `addChat` stores it
|
||||
as received items already do), and `chatItemBelongsToScope` returns false in the primary context
|
||||
for a scoped `cInfo`, so the item never enters the primary item list.
|
||||
|
||||
## Why it is safe
|
||||
|
||||
- The four-site revert changes nothing for the **primary** context:
|
||||
`chats === chatsContext.chats` there, so both forms resolve to the same list. The
|
||||
secondary context stops touching the wrong one. The primary context additionally
|
||||
receives items sent from a secondary view that belong in the main list (the pending
|
||||
invitee's support messages; also a bot command tapped in the reports view, which the
|
||||
main chat now shows).
|
||||
- `updateChatTagReadInPrimaryContext` and `increaseUnreadCounter` (through
|
||||
`changeUnreadCounterInPrimaryContext`) already return early in a secondary context,
|
||||
so they stay primary-only either way.
|
||||
- `popChatCollector` is per-context (`PopChatCollector(this)`), so the pop/reorder
|
||||
calls write the context they run in (until #7404 the collector reads the primary
|
||||
list; see "A related defect" below).
|
||||
- The unread stats increment is limited to the primary context
|
||||
(`secondaryContextFilter == null`). The secondary context's own entry for the open
|
||||
group is what `ChatView` reads for the unread button inside the open view; without
|
||||
the limit it would count main-scope group messages that the view doesn't show. The
|
||||
limit keeps increments out of that entry, as #7559 does for chats added to the list.
|
||||
- iOS is unaffected — `ItemsModel` holds only chat items and the chat list is a single
|
||||
`chats` array on `ChatModel`, so the mis-indexing cannot occur there.
|
||||
|
||||
## Alternatives considered
|
||||
|
||||
- **Guard the whole block to `secondaryContextFilter == null`.** Looked smaller, but
|
||||
the secondary list would then stop getting the preview and order updates that
|
||||
received items give it, and the block would branch on the context type.
|
||||
- **Compute `i` against the primary list and keep the primary writes.** Would make the
|
||||
secondary call double-update the primary preview and unread, and never update the
|
||||
secondary list at all.
|
||||
|
||||
## A related defect, fixed separately
|
||||
|
||||
`PopChatCollector` is nested but not `inner`, so the unqualified `getChat` / `chats`
|
||||
inside `popCollectedChats()` also resolve to `ChatModel` — the primary list — while
|
||||
the result is written to `chatsCtx.chats`. Same class of defect, different symptom
|
||||
(it corrupts the secondary context's own list, not the main one). Fixed in #7404.
|
||||
|
||||
## Verification
|
||||
|
||||
Desktop, master vs this PR, with "Chat with admins" open in a group the user does not
|
||||
own: see the PR description for the reproduction and the before/after screenshots.
|
||||
Reference in new issue
Block a user