From 649d4ab45902e83d8b993d0d1c02bd234d36fcfb Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Fri, 25 Sep 2026 14:42:44 -0400 Subject: [PATCH] fix(cordn): say who could not be added, not their pubkey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every admin failure on the group info screen was rendered as `e.message`. Those messages are written for a log — they name pubkeys and gids in full, which is exactly what you want when reading one and never what you want in a chat. So somebody who searched by name, saw a face and tapped add was told "the coordinator holds no KeyPackage for 74ee1b23…" and left to work out which of those hex characters was the person. That is the same thing the group info refactor spent a commit removing from this page, and it slipped through because it arrives from a commons exception rather than a string resource. `CordnGroupException` gains a `Reason`, so the screen can branch on a value rather than search for substrings in English that is free to be reworded. Four reasons carry wording of their own — no key package, a key package belonging to somebody else, no Welcome, not a member — and the default stays OTHER so every other throw site keeps compiling and keeps showing its message. The screen adds three more cases the enum cannot cover: self-removal, which is an IllegalArgumentException from a `require`; a coordinator that does not answer, which is the commonest failure here and says nothing useful in its own words; and anything else, which still falls back to the message rather than to silence. The display name comes from the row that was tapped, because the exception only ever knows the pubkey. Every failure is also logged in full now. The banner is for the person; logcat is for whoever has to find out why. Verified on the tablet against a freshly generated key that belongs to nobody and has published nothing: "npub1wnhpkgc7…js6ksmev has not published a key package to this coordinator, so they cannot be added yet. Ask them to open cordn on this coordinator first." Membership stayed at 2 — `invite` throws before it commits anything. Co-Authored-By: Claude Opus 5 (1M context) --- .../chats/cordnGroup/CordnGroupInfoScreen.kt | 76 ++++++++++++++++--- amethyst/src/main/res/values/strings.xml | 7 ++ .../commons/cordn/CordnGroupManager.kt | 48 ++++++++++-- .../cordn/CordnGroupExceptionReasonTest.kt | 61 +++++++++++++++ 4 files changed, 176 insertions(+), 16 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupExceptionReasonTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt index 668fc153c4..6aefdb19a5 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/cordnGroup/CordnGroupInfoScreen.kt @@ -20,6 +20,8 @@ */ package com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.cordnGroup +import android.content.Context +import android.util.Log import androidx.compose.animation.AnimatedVisibility import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Arrangement @@ -62,6 +64,7 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R +import com.vitorpamplona.amethyst.commons.cordn.CordnGroupException import com.vitorpamplona.amethyst.commons.cordn.GroupExposure import com.vitorpamplona.amethyst.commons.cordn.ui.CordnExposureCard import com.vitorpamplona.amethyst.commons.icons.symbols.Icon @@ -90,6 +93,7 @@ import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.quartz.cordn.spec00Coordinator.JoinRequest import com.vitorpamplona.quartz.cordn.spec01GroupMetadata.CordnGroupMetadata import com.vitorpamplona.quartz.nip01Core.core.HexKey +import kotlinx.coroutines.TimeoutCancellationException import kotlinx.coroutines.launch /** @@ -224,7 +228,12 @@ private fun CordnGroupInfo( * roster, and a rename stayed the old name. The runtime's own methods pair * each commit with that refresh. */ - fun runAdmin(block: suspend (CordnRuntime) -> Unit) { + val context = LocalContext.current + + fun runAdmin( + who: String? = null, + block: suspend (CordnRuntime) -> Unit, + ) { val target = runtime if (target == null || manager == null) { adminError = noSession @@ -236,7 +245,11 @@ private fun CordnGroupInfo( try { block(target) } catch (e: Exception) { - adminError = e.message ?: failed + // Not e.message. Those are written for a log and name pubkeys + // and gids in full, so the screen that had just shown a face + // answered with 64 hex characters. + Log.w("CordnGroupInfo", "admin action failed in ${room.gid}: ${e.message}", e) + adminError = adminFailureText(context, e, who, failed) } finally { busy = false } @@ -359,7 +372,7 @@ private fun CordnGroupInfo( onInvite = { user -> memberSearch = "" userSuggestions.reset() - runAdmin { it.invite(coordinatorPubKey, room.gid, user.pubkeyHex) } + runAdmin(user.toBestDisplayName()) { it.invite(coordinatorPubKey, room.gid, user.pubkeyHex) } }, ) } @@ -395,6 +408,8 @@ private fun CordnGroupInfo( } removing?.let { target -> + val removingName = observeUserNameByHex(target, accountViewModel) + // A plain confirm rather than the shared quick-action dialog: that // one offers "don't ask again", which for an irreversible removal // would be a setting nobody should be nudged into. @@ -402,17 +417,12 @@ private fun CordnGroupInfo( onDismissRequest = { removing = null }, title = { Text(stringRes(R.string.cordn_info_remove_confirm_title)) }, text = { - Text( - stringRes( - R.string.cordn_info_remove_confirm_body, - observeUserNameByHex(target, accountViewModel), - ), - ) + Text(stringRes(R.string.cordn_info_remove_confirm_body, removingName)) }, confirmButton = { TextButton(onClick = { removing = null - runAdmin { it.removeMember(coordinatorPubKey, room.gid, target) } + runAdmin(removingName) { it.removeMember(coordinatorPubKey, room.gid, target) } }) { Text( text = stringRes(R.string.cordn_info_remove_member), @@ -567,6 +577,52 @@ private fun CordnAddMember( } } +/** + * What to tell somebody when an admin action failed. + * + * `CordnGroupException` carries a [CordnGroupException.Reason] precisely so this + * can be a `when` rather than a search for substrings in English. [who] is the + * display name already on screen; the exception only knows the pubkey, and a + * person who just tapped a face should not be answered with 64 hex characters. + * + * Anything with no better wording falls back to the message, which is still + * better than silence — and the caller has logged the throwable in full either + * way. + */ +private fun adminFailureText( + context: Context, + e: Throwable, + who: String?, + fallback: String, +): String { + val name = who ?: stringRes(context, R.string.cordn_admin_someone) + return when (e) { + is CordnGroupException -> + when (e.reason) { + CordnGroupException.Reason.NO_KEY_PACKAGE -> stringRes(context, R.string.cordn_admin_no_key_package, name) + CordnGroupException.Reason.WRONG_KEY_PACKAGE_OWNER -> stringRes(context, R.string.cordn_admin_wrong_key_package, name) + CordnGroupException.Reason.NO_WELCOME -> stringRes(context, R.string.cordn_admin_no_welcome, name) + CordnGroupException.Reason.NOT_A_MEMBER -> stringRes(context, R.string.cordn_admin_not_a_member, name) + CordnGroupException.Reason.OTHER -> e.message ?: fallback + } + + // The UI hides Remove against yourself, so reaching this means the + // roster and the button disagreed rather than that anyone tried. + is IllegalArgumentException -> + if (e.message?.contains("self-removal") == true) { + stringRes(context, R.string.cordn_admin_no_self_remove) + } else { + e.message ?: fallback + } + + // A coordinator that does not answer is the single commonest failure + // here and says nothing useful in its own words. + is TimeoutCancellationException -> stringRes(context, R.string.cordn_admin_coordinator_silent) + + else -> e.message ?: fallback + } +} + /** * The three values that only matter when something is wrong. * diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index ba7dd859e0..a186a7d3f1 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -396,6 +396,13 @@ Add someone Name, npub or name@domain They can only be added if they published a key package to this coordinator. + That person + %1$s has not published a key package to this coordinator, so they cannot be added yet. Ask them to open cordn on this coordinator first. + This coordinator served a key package that belongs to someone else, so %1$s was not added. Nothing was changed. + %1$s could not be given a way into the group, so the add was abandoned. + %1$s is not in this group. + You cannot remove yourself from a cordn group. + The coordinator did not answer, so nothing was changed. Try again in a moment. Nobody found by that name. Admin +%1$d more diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupManager.kt index 48e41dee3f..4bfb7ab963 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupManager.kt @@ -235,14 +235,20 @@ class CordnGroupManager( // theirs would spend a package they meant for someone else. val taken = call { coordinator.takeKeyPackage(keyPackageRef ?: targetPubKey) } - ?: throw CordnGroupException("the coordinator holds no KeyPackage for $targetPubKey") + ?: throw CordnGroupException( + "the coordinator holds no KeyPackage for $targetPubKey", + CordnGroupException.Reason.NO_KEY_PACKAGE, + ) val verified = KeyPackagePublication.verify(taken.publicationEvent) if (verified.pubKey != targetPubKey) { - throw CordnGroupException("the KeyPackage served for $targetPubKey belongs to ${verified.pubKey}") + throw CordnGroupException( + "the KeyPackage served for $targetPubKey belongs to ${verified.pubKey}", + CordnGroupException.Reason.WRONG_KEY_PACKAGE_OWNER, + ) } val result = group.addMember(verified.bytes) - val welcome = result.welcomeBytes ?: throw CordnGroupException("adding a member produced no Welcome") + val welcome = result.welcomeBytes ?: throw CordnGroupException("adding a member produced no Welcome", CordnGroupException.Reason.NO_WELCOME) // framedCommitBytes, not commitBytes: the latter is the bare RFC 9420 // Commit struct with no MLSMessage around it, which no receiver can @@ -297,7 +303,10 @@ class CordnGroupManager( .entries .firstOrNull { it.value == targetPubKey } ?.key - ?: throw CordnGroupException("$targetPubKey is not a member of $gid") + ?: throw CordnGroupException( + "$targetPubKey is not a member of $gid", + CordnGroupException.Reason.NOT_A_MEMBER, + ) val result = group.removeMember(leafIndex) val posted = @@ -946,10 +955,37 @@ class CordnGroupManager( } } -/** Something went wrong that is this manager's to explain, not MLS's. */ +/** + * Something went wrong that is this manager's to explain, not MLS's. + * + * [reason] exists so a screen can say what happened in its own words. The + * [message] is written for a log — it names pubkeys and gids in full, which is + * what you want when reading one and never what you want in a chat — so a UI + * that rendered `e.message` showed a person who had just tapped a face a + * 64-character hex string. Branching on the reason lets it name the person + * instead, without matching on English that is free to change. + */ class CordnGroupException( message: String, -) : IllegalStateException(message) + val reason: Reason = Reason.OTHER, +) : IllegalStateException(message) { + enum class Reason { + /** Nobody has published a KeyPackage for this person to this coordinator. */ + NO_KEY_PACKAGE, + + /** The coordinator served a KeyPackage belonging to somebody else. */ + WRONG_KEY_PACKAGE_OWNER, + + /** The add produced no Welcome, so the invitee could never open the group. */ + NO_WELCOME, + + /** The person named is not in this group. */ + NOT_A_MEMBER, + + /** Anything with no better wording than the message itself. */ + OTHER, + } +} /** * A Welcome that has been opened but not joined. diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupExceptionReasonTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupExceptionReasonTest.kt new file mode 100644 index 0000000000..ccfa485e68 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/cordn/CordnGroupExceptionReasonTest.kt @@ -0,0 +1,61 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.cordn + +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * The reason a screen branches on. + * + * `CordnGroupException`'s message is written for a log: it names pubkeys and + * gids in full, which is what you want when reading one and never what you + * want in a chat. A UI that rendered it answered somebody who had just tapped + * a face with 64 hex characters. [CordnGroupException.Reason] is how the + * screen says it in its own words instead, so it has to survive as a value + * rather than as English anyone is free to reword. + */ +class CordnGroupExceptionReasonTest { + @Test + fun `the reason travels with the exception`() { + val e = CordnGroupException("the coordinator holds no KeyPackage for abc", CordnGroupException.Reason.NO_KEY_PACKAGE) + + assertEquals(CordnGroupException.Reason.NO_KEY_PACKAGE, e.reason) + } + + @Test + fun `an exception raised without one is OTHER, not a crash`() { + // Most throw sites have no better wording than their own message, and + // must keep compiling and keep rendering that message. + val e = CordnGroupException("something else went wrong") + + assertEquals(CordnGroupException.Reason.OTHER, e.reason) + } + + @Test + fun `the message is left alone for the log`() { + // The reason is additive. Whatever a maintainer reads in logcat must + // still be the precise thing, pubkey and all. + val e = CordnGroupException("abc is not a member of gid-1", CordnGroupException.Reason.NOT_A_MEMBER) + + assertEquals("abc is not a member of gid-1", e.message) + } +}