Code review:

- invalidate zaps flow when removeAllChildNotes clears onchainZaps
- simplify on-chain zap gallery after review
This commit is contained in:
davotoula
2026-05-19 12:58:22 +02:00
parent 91ded74636
commit cf44c092dd
3 changed files with 41 additions and 75 deletions
@@ -20,7 +20,6 @@
*/
package com.vitorpamplona.amethyst.ui.note
import androidx.compose.foundation.background
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
@@ -29,20 +28,13 @@ import androidx.compose.foundation.layout.FlowRow
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.alpha
import androidx.compose.ui.draw.clip
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import com.vitorpamplona.amethyst.commons.model.OnchainZapEntry
import com.vitorpamplona.amethyst.model.Note
import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNoteZaps
@@ -51,11 +43,8 @@ import com.vitorpamplona.amethyst.ui.navigation.routes.routeFor
import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
import com.vitorpamplona.amethyst.ui.theme.Size25dp
import com.vitorpamplona.amethyst.ui.theme.Size35Modifier
import com.vitorpamplona.amethyst.ui.theme.Size35dp
import com.vitorpamplona.amethyst.ui.theme.StdStartPadding
import com.vitorpamplona.amethyst.ui.theme.WidthAuthorPictureModifier
import com.vitorpamplona.amethyst.ui.theme.bitcoinColor
import com.vitorpamplona.amethyst.ui.theme.overPictureBackground
import kotlinx.collections.immutable.ImmutableList
import kotlinx.collections.immutable.persistentListOf
import kotlinx.collections.immutable.toImmutableList
@@ -76,14 +65,17 @@ internal fun WatchOnchainZapsAndRenderGallery(
) {
// Reuse the same flow the lightning gallery subscribes to. Note.addOnchainZap
// invalidates flowSet.zaps, so this composable refreshes when on-chain zaps
// arrive or upgrade pending → confirmed.
// arrive or upgrade pending → confirmed. The flow also fires for lightning
// zap arrivals on the same note, so memoize the list snapshot.
val zapsState by observeNoteZaps(baseNote, accountViewModel)
val entries =
remember(zapsState) {
zapsState
?.note
?.onchainZaps
?.values
?.toImmutableList() ?: persistentListOf()
}
if (entries.isNotEmpty()) {
RenderOnchainZapGallery(entries, nav, accountViewModel)
@@ -136,13 +128,8 @@ private fun OnchainZapEntryRow(
}
val avatarAlpha = if (entry.confirmed) 1f else 0.6f
Row(
modifier =
Modifier.clickable { onOnchainZapEntryClick(entry, nav) },
verticalAlignment = Alignment.CenterVertically,
) {
Box(
modifier = Size35Modifier,
modifier = Size35Modifier.clickable { onOnchainZapEntryClick(entry, nav) },
contentAlignment = Alignment.BottomCenter,
) {
// Only the avatar dims for pending entries. The amount overlay and clock
@@ -154,39 +141,15 @@ private fun OnchainZapEntryRow(
)
}
// Amount overlay — same look as the lightning row's CrossfadeToDisplayAmount.
Box(
modifier =
Modifier
.size(Size35dp)
.clip(CircleShape),
contentAlignment = Alignment.BottomCenter,
) {
val overlayBg = MaterialTheme.colorScheme.overPictureBackground
Box(
modifier = remember(overlayBg) { Modifier.width(Size35dp).background(overlayBg) },
contentAlignment = Alignment.BottomCenter,
) {
Text(
text = amountText,
fontWeight = FontWeight.Bold,
color = MaterialTheme.colorScheme.bitcoinColor,
fontSize = 12.sp,
modifier = bottomPadding1dp,
)
}
}
CrossfadeToDisplayAmount(amountText)
if (!entry.confirmed) {
// TopStart so the badge doesn't collide with the FollowingIcon
// that WatchUserMetadataAndFollowsAndRenderUserProfilePicture
// paints at TopEnd for followed users.
Box(
modifier = Modifier.align(Alignment.TopStart),
) {
Box(modifier = Modifier.align(Alignment.TopStart)) {
PendingClockBadge(modifier = Modifier.size(14.dp))
}
}
}
}
}
@@ -310,7 +310,7 @@ open class Note(
fun removeAllChildNotes(): List<Note> {
val repliesChanged = replies.isNotEmpty()
val reactionsChanged = reactions.isNotEmpty()
val zapsChanged = zaps.isNotEmpty() || zapPayments.isNotEmpty()
val zapsChanged = zaps.isNotEmpty() || zapPayments.isNotEmpty() || onchainZaps.isNotEmpty()
val boostsChanged = boosts.isNotEmpty()
val reportsChanged = reports.isNotEmpty()
@@ -435,15 +435,14 @@ open class Note(
entry: OnchainZapEntry,
): Boolean {
val existing = onchainZaps[txid]
if (existing == null) {
onchainZaps = onchainZaps + Pair(txid, entry)
return true
}
if (existing != null) {
// Same-state duplicate: keep the first source and amount we got.
if (existing.confirmed == entry.confirmed) return false
// Downgrade confirmed → pending: never accept.
if (existing.confirmed && !entry.confirmed) return false
// Upgrade pending → confirmed: replace, so the upgrading event's source wins.
// Downgrade confirmed → pending: never accept. States differ here,
// so existing.confirmed alone is sufficient to identify the downgrade.
if (existing.confirmed) return false
// Else: existing pending + incoming confirmed — fall through to upgrade.
}
onchainZaps = onchainZaps + Pair(txid, entry)
return true
}
@@ -94,9 +94,13 @@ class NoteOnchainZapTest {
val secondSrc = sourceNote("2".repeat(64))
target.addOnchainZap(firstSrc, "tx1", 100L, confirmed = true)
target.addOnchainZap(secondSrc, "tx1", 100L, confirmed = true)
// Mismatched verifiedSats so we can detect a regression that silently
// overwrites the first entry with the second.
target.addOnchainZap(secondSrc, "tx1", 999L, confirmed = true)
assertSame(firstSrc, target.onchainZaps["tx1"]?.source)
val entry = target.onchainZaps["tx1"]
assertSame(firstSrc, entry?.source)
assertEquals(100L, entry?.verifiedSats)
assertEquals(BigDecimal.valueOf(100L), target.zapsAmount)
}