laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-05 19:28:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(cordn): five defects from auditing the chat work

Browse Source 
An audit of everything since the bubble refactor. Three of these are
real user-facing defects in code added over the last four commits.

**Crash: sending an emptied gallery.** The upload dialog lets you delete
what you picked, and deleting the last item leaves an *empty*
multiOrchestrator, not a null one. `canPost()` only checked non-null, so
Send stayed live with nothing to send; cordn's uploader then indexed
item 0 and threw IndexOutOfBounds. canPost() now checks the size, which
fixes the live button for every caller — Marmot and the rest silently
posted nothing in the same state — and cordn checks again where it
indexes.

**Sending the same file twice.** cordn never started the upload
tracker, and the dialog's Send button reads canPost(), which is only
false while that tracker says an upload is running. So the button
stayed live for the whole upload and a second tap encrypted, uploaded
and posted the file a second time. Started now, and finished in a
`finally` — not only on success, or a failed upload would leave the
dialog's button dead forever.

**Plaintext attachments left in the cache.** Compression and metadata
stripping each write a new file, and both hold the attachment in the
clear. UploadOrchestrator deletes them in a `finally`; the cordn path
never did, so every attachment left an unencrypted copy of an
end-to-end encrypted message on disk. Deleted now, through the
orchestrator's own helper (made internal rather than copied), which
no-ops on the user's own file.

**Plaintext audio left in the cache.** Same defect, other path: the
recorder writes to cacheDir and sendVoiceNote deletes the file after
sending, but a note recorded and then abandoned — screen closed, room
switched — never reached that. A DisposableEffect deletes it now.
This one arrived with the voice preview, which is what made a recording
outlive the tap that made it.

**Stale suggestion list.** onTextChanged only fires for typing, so a
list left open by a half-typed "@na" survived the field being cleared
on send. Reset alongside every external write to the draft.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012BfD4txdnsaPRXmNXbup9n
This commit is contained in:
Claude
2026-09-24 14:14:05 +00:00
parent 50184ed6fe
commit a0182504cb
4 changed files with 102 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/service/uploads/UploadOrchestrator.kt
+1 -1
View File
@@ -432,7 +432,7 @@ class UploadOrchestrator {
* Deletes a temporary file created during the upload pipeline if its URI
* differs from the original (meaning it's an intermediate temp file, not the user's content).
*/
private fun deleteTempUri(
internal fun deleteTempUri(
tempUri: Uri,
originalUri: Uri,
) {
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnComposer.kt
+20 -17
View File
@@ -99,23 +99,6 @@ internal fun CordnComposer(
// inequality, so neither direction can bounce off the other.
val draftState = remember(room.gid) { TextFieldState(room.draft.value) }
LaunchedEffect(draftState, room) {
snapshotFlow { draftState.text.toString() }.collect {
if (room.draft.value != it) room.draft.value = it
}
}
LaunchedEffect(draftState, room) {
room.draft.collect { external ->
// The screen writes the draft from outside on three paths: clearing it on
// send, restoring it when a send fails, and loading a message's text into
// it to edit. Cursor to the end, as editFromDraft does elsewhere.
if (external != draftState.text.toString()) {
if (external.isEmpty()) draftState.clearText() else draftState.setTextAndPlaceCursorAtEnd(external)
}
}
}
val suggestions =
remember(room.gid, accountViewModel) {
UserSuggestionState(
@@ -131,6 +114,26 @@ internal fun CordnComposer(
onDispose { suggestions.reset() }
}
LaunchedEffect(draftState, room) {
snapshotFlow { draftState.text.toString() }.collect {
if (room.draft.value != it) room.draft.value = it
}
}
LaunchedEffect(draftState, room) {
room.draft.collect { external ->
// The screen writes the draft from outside on three paths: clearing it on
// send, restoring it when a send fails, and loading a message's text into
// it to edit. Cursor to the end, as editFromDraft does elsewhere.
if (external != draftState.text.toString()) {
if (external.isEmpty()) draftState.clearText() else draftState.setTextAndPlaceCursorAtEnd(external)
// onTextChanged only fires for typing, so a list left open by a
// half-typed "@na" survived the field being cleared on send.
suggestions.reset()
}
}
}
// A recorded voice note is something to send even with nothing typed; the text
// beside it rides along as its caption.
val canPost by remember(pendingVoice) { derivedStateOf { draftState.text.isNotBlank() || pendingVoice != null } }
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupChatScreen.kt
+75 -39
View File
@@ -180,6 +180,16 @@ private fun CordnGroupChat(
)
}
var pendingVoice by remember { mutableStateOf<RecordingResult?>(null) }
DisposableEffect(room.gid) {
onDispose {
// The recorder writes plaintext audio to cacheDir and sendVoiceNote deletes
// it after sending. A note recorded and then abandoned — the screen closed,
// the room switched — never reached that, so it stayed on disk: an
// unencrypted copy of a message that was never even sent.
pendingVoice?.file?.delete()
}
}
var attachError by remember { mutableStateOf<String?>(null) }
// Why sending says anything at all when it fails: `manager()` is null-safe
@@ -768,51 +778,77 @@ private suspend fun sendAttachment(
session.manager.group(room.gid)
?: throw CordnAttachmentException(stringRes(context, R.string.cordn_send_no_session))
val orchestrator = state.multiOrchestrator ?: return
// The gallery inside the dialog can delete what was picked, which leaves an empty
// orchestrator rather than a null one. canPost() now refuses that, but indexing it
// blindly here crashed, so it is checked where the index happens too.
val orchestrator = state.multiOrchestrator
if (orchestrator == null || orchestrator.size() == 0) return
val item = orchestrator.get(0)
val uri = item.media.uri
val declaredMime = item.media.mimeType ?: context.contentResolver.getType(uri) ?: CordnBlobUpload.OPAQUE
// The media-quality slider.
val compressed =
item.orchestrator.compressIfNeeded(
uri = uri,
mimeType = declaredMime,
compressionQuality = MediaCompressor.intToCompressorQuality(state.mediaQualitySlider),
context = context,
)
val mime = compressed.contentType ?: declaredMime
// Marks the dialog busy: its Send button reads canPost(), which is false while a
// tracker says an upload is running. Without this the button stayed live for the
// whole upload and a second tap encrypted, uploaded and posted the file twice.
state.mediaUploadTracker.startUpload(orchestrator.hasNonMedia())
// The strip-metadata switch. A file type the stripper does not handle comes back
// untouched and says so, which is not a failure — there was nothing to strip.
val finalUri =
if (state.stripMetadata) {
withContext(Dispatchers.IO) { MetadataStripper.strip(compressed.uri, mime, context) }.uri
} else {
compressed.uri
try {
// The media-quality slider.
val compressed =
item.orchestrator.compressIfNeeded(
uri = uri,
mimeType = declaredMime,
compressionQuality = MediaCompressor.intToCompressorQuality(state.mediaQualitySlider),
context = context,
)
val mime = compressed.contentType ?: declaredMime
// The strip-metadata switch. A file type the stripper does not handle comes back
// untouched and says so, which is not a failure — there was nothing to strip.
val finalUri =
if (state.stripMetadata) {
withContext(Dispatchers.IO) { MetadataStripper.strip(compressed.uri, mime, context) }.uri
} else {
compressed.uri
}
try {
// Name comes from OpenableColumns: `uri.lastPathSegment` is a document id
// on a content:// URI, not a filename.
val name = resolveDisplayName(context, uri)
val bytes =
withContext(Dispatchers.IO) { context.contentResolver.openInputStream(finalUri)?.use { it.readBytes() } }
?: throw CordnAttachmentException(stringRes(context, R.string.cordn_media_unreadable))
// Null means the chosen host has no base URL, which is a setting the person can
// change — the one failure here that is entirely actionable.
val tag =
CordnMediaService(accountViewModel.account)
.upload(bytes, mime, name, context, state.selectedServer.baseUrl)
?: throw CordnAttachmentException(stringRes(context, R.string.cordn_media_no_server))
// Into the room as well, for the same reason every other send is: an
// attachment of your own echoes back as an Echo and would otherwise be
// invisible to the person who sent it.
//
// The dialog's description is the message's own content, so an attachment with
// something written about it is one message rather than two.
room.add(session.manager.send(room.gid, content = state.caption.trim(), tags = arrayOf(tag)))
} finally {
// Compressing and stripping each write a new file, and both hold the
// attachment in the clear. Leaving them in the cache would keep plaintext
// copies of a message that is end-to-end encrypted everywhere else — the
// same reason the voice path deletes its recording. Both calls no-op on the
// user's own file.
item.orchestrator.deleteTempUri(finalUri, uri)
item.orchestrator.deleteTempUri(compressed.uri, uri)
}
// Name and type come from OpenableColumns: `uri.lastPathSegment` is a document id
// on a content:// URI, not a filename.
val name = resolveDisplayName(context, uri)
val bytes =
withContext(Dispatchers.IO) { context.contentResolver.openInputStream(finalUri)?.use { it.readBytes() } }
?: throw CordnAttachmentException(stringRes(context, R.string.cordn_media_unreadable))
// Null means the chosen host has no base URL, which is a setting the person can
// change — the one failure here that is entirely actionable.
val tag =
CordnMediaService(accountViewModel.account)
.upload(bytes, mime, name, context, state.selectedServer.baseUrl)
?: throw CordnAttachmentException(stringRes(context, R.string.cordn_media_no_server))
// Into the room as well, for the same reason every other send is: an
// attachment of your own echoes back as an Echo and would otherwise be
// invisible to the person who sent it.
//
// The dialog's description is the message's own content, so an attachment with
// something written about it is one message rather than two.
room.add(session.manager.send(room.gid, content = state.caption.trim(), tags = arrayOf(tag)))
} finally {
// Always, not just on success: a failure leaves the dialog up to retry from,
// and a tracker stuck "uploading" would keep its Send button dead forever.
state.mediaUploadTracker.finishUpload()
}
}
/**
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/utils/ChatFileUploadState.kt
+6 -1
View File
@@ -81,7 +81,12 @@ class ChatFileUploadState(
multiOrchestrator?.remove(selected)
}
fun canPost(): Boolean = !mediaUploadTracker.isUploading && multiOrchestrator != null
/**
* Deleting the last picked item leaves an *empty* orchestrator, not a null one, so
* a non-null check alone kept Send live with nothing to send. Callers that loop
* over the items then post nothing; one that indexes item 0 crashes.
*/
fun canPost(): Boolean = !mediaUploadTracker.isUploading && (multiOrchestrator?.size() ?: 0) > 0
fun hasPickedMedia() = multiOrchestrator != null