mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-08-14 13:40:32 +00:00
android, desktop: fix draft appearing in another chat when switching chats while sending (#7308)
* ios, android, desktop: fix message being sent leaking into another chat Compose state is shared between the chats opened in the same view, and the send is launched in a scope that outlives the chat, so a send that was still in flight when the chat was switched put its message (with the reply context) into the compose state and then the draft of another chat, and a late success cleared whatever was typed in the meantime. The message being sent is no longer kept in the compose state when leaving the chat, and the compose state is only cleared or restored after sending if it still holds the message that was sent - the same check is used by the other senders that show progress in the compose. A message that failed to send is restored in the chat it was composed in, or kept as its draft when another chat is open (iOS has no failed message restore, there the message is dropped as before). * android, desktop: keep only the chat switch fix Revert the iOS changes and the same check in the three senders that connect a prepared chat, leaving the fix for the compose state shared between the chats opened in one view. * plan: document what the narrowed change leaves to the connect senders * android, desktop: use the same check where the sending flag is shared The senders that connect a prepared chat set the same inProgress flag, so a connect completing after the chat was switched cleared it for a send started in the chat opened next, and that sent message was then left in the compose. * android, desktop: keep the live message clauses inside the open chat check live and cs.liveMessage != null were alternatives to chatIsOpen, which holds only while a live message is always sent to the chat that is open. #7323 removes that: the live message committed by a chat switch is sent to the chat it was composed in, while this view already shows another one, so an unguarded clause here clears that chat's compose state - the leak this fix exists to prevent. liveSend stays an alternative to inProgress inside the guard, so a live send behaves exactly as before while its chat is open, and it is excluded from the restore/draft branch, which would otherwise write a draft on every failing keystroke send once that chat is no longer open. On this branch the only behaviour change is a live send completing after its chat was left, which now leaves the opened chat alone. Also load the draft of the chat opened next when the compose state is cleared on switching away from a send in flight: clearState() returns before the branch that loads a draft, so that draft was never shown, and being in the slot but in no compose state it was then dropped by clearPrevDraft on the next chat switch. * plan: explain why live sends are guarded by the open chat check Records that the exemption in "Deliberately unchanged" is from a guard based on inProgress, not from chatIsOpen, and why the earlier form broke once #7323 sends a live message to a chat other than the one open.
This commit is contained in:
Narasimha-sc
+54
-20
@@ -597,6 +597,10 @@ fun ComposeView(
|
||||
composeState.value = composeState.value.copy(inProgress = true)
|
||||
}
|
||||
|
||||
// composeState and its inProgress flag are shared between the chats opened in this view, and sending is not cancelled
|
||||
// when the chat is switched - a send may only clear or reset the state while it still holds the message that was sent
|
||||
fun composeHasSentMessage(): Boolean = chatModel.chatId.value == chat.id && composeState.value.inProgress
|
||||
|
||||
suspend fun sendMemberContactInvitation() {
|
||||
val mc = checkLinkPreview()
|
||||
sending()
|
||||
@@ -604,10 +608,10 @@ fun ComposeView(
|
||||
if (contact != null) {
|
||||
withContext(Dispatchers.Main) {
|
||||
chatsCtx.updateContact(chat.remoteHostId, contact)
|
||||
clearState()
|
||||
if (composeHasSentMessage()) clearState()
|
||||
}
|
||||
} else {
|
||||
composeState.value = composeState.value.copy(inProgress = false)
|
||||
} else withContext(Dispatchers.Main) {
|
||||
if (composeHasSentMessage()) composeState.value = composeState.value.copy(inProgress = false)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -624,10 +628,10 @@ fun ComposeView(
|
||||
if (contact != null) {
|
||||
withContext(Dispatchers.Main) {
|
||||
chatsCtx.updateContact(chat.remoteHostId, contact)
|
||||
clearState()
|
||||
if (composeHasSentMessage()) clearState()
|
||||
}
|
||||
} else {
|
||||
composeState.value = composeState.value.copy(inProgress = false)
|
||||
} else withContext(Dispatchers.Main) {
|
||||
if (composeHasSentMessage()) composeState.value = composeState.value.copy(inProgress = false)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -669,10 +673,10 @@ fun ComposeView(
|
||||
chatModel.channelRelayHostnames.remove(groupInfo.groupId)
|
||||
chatModel.groupMembers.value = relayResults.map { it.relayMember }
|
||||
chatModel.populateGroupMembersIndexes()
|
||||
clearState()
|
||||
if (composeHasSentMessage()) clearState()
|
||||
}
|
||||
} else {
|
||||
composeState.value = composeState.value.copy(inProgress = false)
|
||||
} else withContext(Dispatchers.Main) {
|
||||
if (composeHasSentMessage()) composeState.value = composeState.value.copy(inProgress = false)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -932,16 +936,38 @@ fun ComposeView(
|
||||
val wasForwarding = cs.forwarding
|
||||
val forwardingFromChatId = (cs.contextItem as? ComposeContextItem.ForwardingItems)?.fromChatInfo?.id
|
||||
val lastFailed = lastMessageFailedToSend
|
||||
if (lastFailed == null) {
|
||||
clearState(live)
|
||||
} else {
|
||||
composeState.value = lastFailed
|
||||
}
|
||||
val draft = chatModel.draft.value
|
||||
if (wasForwarding && chatModel.draftChatId.value == draftChatId(chat.chatInfo.id, chatScope) && forwardingFromChatId != chat.chatInfo.id && draft != null) {
|
||||
composeState.value = draft
|
||||
} else {
|
||||
clearCurrentDraft()
|
||||
// composeState is shared between the chats opened in this view, and this runs after the send API call, so the user
|
||||
// could have switched chats or typed another message in the meantime - only the message that was sent may be
|
||||
// cleared or restored. On Main, so that these checks and changes are not interleaved with the user switching
|
||||
// chats or typing.
|
||||
withContext(Dispatchers.Main) {
|
||||
val chatIsOpen = chatModel.chatId.value == chat.id
|
||||
// a live message is held in the compose state of the chat it is sent to, but only while that chat is the one open
|
||||
val liveSend = live || cs.liveMessage != null
|
||||
val sentMessageInCompose = chatIsOpen && (liveSend || composeState.value.inProgress)
|
||||
if (sentMessageInCompose) {
|
||||
if (lastFailed == null) {
|
||||
clearState(live)
|
||||
} else {
|
||||
composeState.value = lastFailed
|
||||
}
|
||||
}
|
||||
val draft = chatModel.draft.value
|
||||
if (wasForwarding && chatModel.draftChatId.value == draftChatId(chat.chatInfo.id, chatScope) && forwardingFromChatId != chat.chatInfo.id && draft != null) {
|
||||
if (sentMessageInCompose) composeState.value = draft
|
||||
} else {
|
||||
clearCurrentDraft()
|
||||
// liveSend excluded: a failing keystroke send would otherwise write a draft on every attempt
|
||||
if (!sentMessageInCompose && !liveSend && lastFailed != null) {
|
||||
// the message was not sent, so it is restored in the chat it was composed in, or kept as its draft if another chat is open
|
||||
if (chatIsOpen && composeState.value.empty) {
|
||||
composeState.value = lastFailed
|
||||
} else if (saveLastDraft) {
|
||||
chatModel.draft.value = lastFailed
|
||||
chatModel.draftChatId.value = draftChatId(chat.id, chatScope)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return sent
|
||||
}
|
||||
@@ -1319,7 +1345,15 @@ fun ComposeView(
|
||||
deleteUnusedFiles()
|
||||
} else if (cs.inProgress) {
|
||||
clearPrevDraft(prevChatId)
|
||||
composeState.value = cs.copy(inProgress = false, progressByTimeout = false)
|
||||
// the message being sent must not be kept in the compose state, it is shared with the chat opened next;
|
||||
// if it fails to send it is restored in this chat or saved as its draft
|
||||
clearState()
|
||||
// clearState() does not load the draft of the chat opened next, and without this it is never shown and is
|
||||
// dropped when that chat is left
|
||||
val draft = chatModel.draft.value
|
||||
if (draft != null && chatModel.draftChatId.value == draftChatId(chatModel.chatId.value, chatScope)) {
|
||||
composeState.value = draft
|
||||
}
|
||||
} else if (!cs.empty) {
|
||||
if (cs.preview is ComposePreview.VoicePreview && !cs.preview.finished) {
|
||||
recState.value = RecordingState.NotStarted
|
||||
|
||||
@@ -0,0 +1,244 @@
|
||||
# Fix: message being sent leaks into another chat's compose/draft, and erases what is typed there
|
||||
|
||||
Branch: `nd/fix-inflight-send-writes-other-chat-compose` (off `origin/stable`)
|
||||
Date: 2026-07-25
|
||||
PR: #7308
|
||||
|
||||
Line references are against `origin/stable` at `8dc387cb5`, with this fix
|
||||
applied. Android and desktop only
|
||||
(`multiplatform/.../views/chat/ComposeView.kt`); iOS has the same defect
|
||||
but is not addressed here.
|
||||
|
||||
## Problem
|
||||
|
||||
Two reported symptoms, one cause. Both need a send that is still in
|
||||
flight when the chat is switched (slow network, large file, or the send
|
||||
just hanging with the progress circle showing):
|
||||
|
||||
1. **The message ends up in another chat's draft.** Reply to a message
|
||||
(or just type), press send, switch to another chat while it is
|
||||
sending: the text *and the reply context* appear in that chat's input,
|
||||
and leaving it saves them as that chat's draft. No forwarding
|
||||
involved.
|
||||
2. **A late send erases what you typed.** Press send, the progress circle
|
||||
keeps spinning, switch to another chat and back, type a new message —
|
||||
when the original send finally succeeds, the newly typed message is
|
||||
erased.
|
||||
|
||||
## Cause
|
||||
|
||||
The compose state is shared, and the send outlives the chat:
|
||||
|
||||
- `ChatView.kt:134` — one `MutableState<ComposeState>` per `ChatView`
|
||||
instance, `rememberSaveable` with no keys, reused for every chat that
|
||||
the view displays.
|
||||
- `Utils.kt:43-46` — `withLongRunningApi` launches on
|
||||
`CoroutineScope(Dispatchers.Default)`, a standalone scope with no tie
|
||||
to the composition or to the chat, and `sendMessage`
|
||||
(`ComposeView.kt:972-976`) uses it. Leaving the chat never cancels an
|
||||
in-flight send.
|
||||
|
||||
Two writes then act on the wrong chat:
|
||||
|
||||
- **On the chat switch** — `ComposeView.kt:1343-1347`: the `cs.inProgress`
|
||||
branch used to keep the message in the shared compose state
|
||||
(`composeState.value = cs.copy(inProgress = false, progressByTimeout = false)`)
|
||||
and only cleared the *previous* chat's saved draft. The text and the
|
||||
quote were therefore sitting in the input of the chat opened next, and
|
||||
`ComposeView.kt:1348-1358` (`!cs.empty`) then saved them as *that*
|
||||
chat's draft on the next switch. Symptom 1.
|
||||
- **When the send completes** — `ComposeView.kt:943-968`, running in the
|
||||
detached coroutine after the switch: `clearState(live)` on success, or
|
||||
`composeState.value = lastFailed` on failure, where `lastFailed =
|
||||
cs.copy(inProgress = false, preview = preview)`
|
||||
(`ComposeView.kt:729`) **keeps `contextItem`, i.e. the reply**. On
|
||||
success this wipes whatever is in the input now — including a message
|
||||
typed after coming back (symptom 2); on failure it dumps the old
|
||||
message into whichever chat is open (symptom 1 again).
|
||||
|
||||
The same function was already inconsistent about which chat it acts on:
|
||||
its draft bookkeeping (`clearCurrentDraft()`, and the forwarding
|
||||
condition) uses the **captured** `chat` — the chat the message was
|
||||
composed in — while its `composeState` writes hit whatever chat is
|
||||
displayed at that moment.
|
||||
|
||||
## Fix
|
||||
|
||||
Two changes, both in `ComposeView.kt`.
|
||||
|
||||
**1. Do not keep the message being sent in the shared compose state**
|
||||
(`ComposeView.kt:1343-1347`). On switching away with a send in flight the
|
||||
compose state is cleared, so nothing leaks into the chat opened next:
|
||||
|
||||
```kotlin
|
||||
} else if (cs.inProgress) {
|
||||
clearPrevDraft(prevChatId)
|
||||
// the message being sent must not be kept in the compose state, it is shared with the chat opened next;
|
||||
// if it fails to send it is restored in this chat or saved as its draft
|
||||
clearState()
|
||||
}
|
||||
```
|
||||
|
||||
`clearState()` is used rather than assigning an empty `ComposeState` so that
|
||||
the link preview state is reset too (`pendingLinkUrl` still points at the
|
||||
sent message's link, and its fetch would otherwise set a preview on the
|
||||
input of the chat opened next), and so that the attachment size limit is
|
||||
carried over the same way as everywhere else.
|
||||
|
||||
In-flight content is deliberately **not** saved as a draft here: the
|
||||
message has been submitted and will most likely be sent, and a draft is
|
||||
for messages that are not sent yet.
|
||||
|
||||
`clearState()` alone would leave the chat opened next with an empty input
|
||||
even when it has a draft: this branch, like the live message one above it,
|
||||
returns before the branch that loads a draft
|
||||
(`chatModel.draftChatId.value == draftChatId(chatModel.chatId.value, chatScope)`),
|
||||
so that draft was never shown - and, being still in the slot but not in
|
||||
any compose state, it was then dropped by `clearPrevDraft` on the next
|
||||
chat switch. It is loaded here instead. This is not the one-slot
|
||||
limitation below: nothing else is competing for the slot, the draft is
|
||||
simply lost.
|
||||
|
||||
**2. Only touch the compose state if it still holds the message that was
|
||||
sent** (`ComposeView.kt:936-968`):
|
||||
|
||||
```kotlin
|
||||
withContext(Dispatchers.Main) {
|
||||
val chatIsOpen = chatModel.chatId.value == chat.id
|
||||
val liveSend = live || cs.liveMessage != null
|
||||
val sentMessageInCompose = chatIsOpen && (liveSend || composeState.value.inProgress)
|
||||
if (sentMessageInCompose) {
|
||||
if (lastFailed == null) {
|
||||
clearState(live)
|
||||
} else {
|
||||
composeState.value = lastFailed
|
||||
}
|
||||
}
|
||||
val draft = chatModel.draft.value
|
||||
if (wasForwarding && chatModel.draftChatId.value == draftChatId(chat.chatInfo.id, chatScope) && forwardingFromChatId != chat.chatInfo.id && draft != null) {
|
||||
if (sentMessageInCompose) composeState.value = draft
|
||||
} else {
|
||||
clearCurrentDraft()
|
||||
if (!sentMessageInCompose && lastFailed != null) {
|
||||
// the message was not sent, so it is restored in the chat it was composed in, or kept as its draft if another chat is open
|
||||
if (chatIsOpen && composeState.value.empty) {
|
||||
composeState.value = lastFailed
|
||||
} else if (saveLastDraft) {
|
||||
chatModel.draft.value = lastFailed
|
||||
chatModel.draftChatId.value = draftChatId(chat.id, chatScope)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Both the checks and the changes run on `Dispatchers.Main` (the block has
|
||||
no suspension points), so they cannot be interleaved with the user
|
||||
switching chats or typing - `KeyChangeEffect`, which does change 1, runs
|
||||
there too.
|
||||
|
||||
`inProgress` is the marker that the compose state is still the submitted
|
||||
message: it is set by `sending()` (`ComposeView.kt:596-598`), preserved by
|
||||
`copy` while sending (the only other write during a send is
|
||||
`progressByTimeout` at `ComposeView.kt:1610-1617`), reset when switching
|
||||
away (change 1), and never set by typing a new message. So a chat switch
|
||||
*or* newly typed text both make the guard false.
|
||||
|
||||
A **failed** send is different from an in-flight one - the message was not
|
||||
sent, so it is an unsent message. It is put back into the input if that
|
||||
chat is open and nothing else is being composed there, and kept as that
|
||||
chat's draft otherwise, so it never appears in another chat (see the
|
||||
limitations below for when it is still dropped). Staying in the chat is
|
||||
unaffected: the guard is true there
|
||||
and the failed message is restored into the input as before, keeping
|
||||
"preserving long message when failed to send" (`e61babdc8`) working.
|
||||
|
||||
Deliberately unchanged:
|
||||
|
||||
- The condition of the forwarding branch. Gating the whole branch would
|
||||
send a forward that completed after the user left to `clearCurrentDraft()`
|
||||
instead, **deleting** the destination chat's draft that the branch
|
||||
exists to preserve - only the compose write inside it is gated.
|
||||
- Live message sends (`live`, or `cs.liveMessage != null` for the send
|
||||
that finalises a live message when leaving the chat, `ComposeView.kt:1338-1342`),
|
||||
as long as their chat is the one open. They never call `sending()`, so a
|
||||
guard based on `inProgress` would change their behaviour: failed live
|
||||
sends would stop restoring and would write a draft on every failing
|
||||
keystroke send. That is why `liveSend` is an alternative to `inProgress`
|
||||
inside the guard, and why it is excluded from the restore/draft branch -
|
||||
not gating it there would produce exactly that draft-per-keystroke.
|
||||
|
||||
What they are **not** exempt from is `chatIsOpen`. An earlier revision
|
||||
had `live || cs.liveMessage != null` outside it, which holds only while
|
||||
a live message is always sent to the chat that is open. #7323 removes
|
||||
that: the live message committed by a chat switch is sent to the chat it
|
||||
was composed in, while this view already shows another one, so an
|
||||
unguarded clause here would clear *that* chat's compose state - the leak
|
||||
this fix exists to prevent. Standalone this changes nothing except a
|
||||
live send that completes after its chat was left, which now leaves the
|
||||
opened chat alone. `sendMessageAsync` reads `composeState` inside the
|
||||
coroutine (`ComposeView.kt:684`), so that branch cannot clear the state
|
||||
itself without racing the send; #7323 adds the `composed` parameter that
|
||||
makes the captured state explicit.
|
||||
|
||||
**3. The same check where the flag is shared** (`ComposeView.kt:600-602`
|
||||
and the three senders that connect a prepared chat). They call the same
|
||||
`sending()`, so an unguarded `clearState()` or `inProgress` reset from
|
||||
one of them corrupts the state of a send started in the chat opened next.
|
||||
|
||||
## Behaviour after the fix
|
||||
|
||||
| situation | before | after |
|
||||
| --- | --- | --- |
|
||||
| send, stay in chat, succeeds | input cleared | input cleared (unchanged) |
|
||||
| send, stay in chat, fails | message restored in input | message restored in input (unchanged) |
|
||||
| send, switch chats, succeeds | message left in the other chat's input, saved as its draft | other chat untouched |
|
||||
| send, switch chats, fails | message dumped into the other chat's input | message restored in the chat it was composed in, or kept as its draft |
|
||||
| send hangs, switch away and back, type, then it succeeds | typed message erased | typed message kept |
|
||||
| forward send, still in destination chat | destination chat's draft restored | unchanged |
|
||||
| live message sent on leaving the chat | compose state cleared by the send | unchanged |
|
||||
|
||||
## Limitations
|
||||
|
||||
Kept deliberately, to not grow the change:
|
||||
|
||||
- A message that failed to send is dropped, rather than kept, when the
|
||||
"Message draft" privacy setting is off, when the destination chat of a
|
||||
failed forward already has a draft (its own draft is preserved
|
||||
instead), and when the single draft slot is later taken by another
|
||||
chat - drafts are one global slot, so the last write wins.
|
||||
- The three senders that connect a prepared chat share the same
|
||||
`sending()` flag, so they use the same check (`ComposeView.kt:604-616`,
|
||||
`618-640`, `659-685`). Without it a connect completing after the chat
|
||||
was switched would clear `inProgress` for a send started in the chat
|
||||
opened next, and that sent message would then stay in the input. They
|
||||
have no failed-message restore, so their typed message is dropped when
|
||||
the chat is switched instead of being carried into the next chat.
|
||||
- Typing in the same chat while its own send is in flight is still
|
||||
cleared when the send completes: `inProgress` is preserved by `copy`,
|
||||
so the guard stays true. Unchanged from before, and different from the
|
||||
reported symptom, which needs the chat to be switched.
|
||||
## Verification
|
||||
|
||||
- `./gradlew :common:compileKotlinDesktop` — passes.
|
||||
- Manual (needs a slow or failing send — e.g. airplane mode, or a large
|
||||
file). On desktop any chat switch exercises it; on Android only an
|
||||
in-place switch does (member info → open chat), because leaving to the
|
||||
chat list destroys the view:
|
||||
1. Reply + type in A, send, switch to B while sending. B's input must
|
||||
stay empty; leaving B must not create a draft in B. If the send
|
||||
failed, A must hold the message (with the reply) as its draft.
|
||||
2. Send in A with the network off so the circle keeps spinning, switch
|
||||
to B and back to A, type a new message, restore the network. The
|
||||
typed message must survive the old send completing.
|
||||
3. Regression: ordinary send in A (input clears), failed send while
|
||||
staying in A (message comes back in the input), forward into a chat
|
||||
that has a draft (draft restored after sending).
|
||||
|
||||
Rebased onto the scope-aware draft ids introduced by #7309: the draft
|
||||
written here for a message that failed to send uses
|
||||
`draftChatId(chat.id, chatScope)`, like every other draft write.
|
||||
|
||||
Related: `plans/2026-07-25-fix-forward-moves-draft-to-target-chat.md`
|
||||
(PR #7307) — different cause (stale `chat` captured by the desktop
|
||||
`onDispose`), same shared-compose-state design.
|
||||
Reference in New Issue
Block a user