code review fixes:

Deduplicate chess notify() into shared notifyChessEvent() helper using BaseChessEvent Remove addCompletedGameDirectly, extend moveToCompleted with optional liveState param Hoist currentUser lookup to top of ChessLobbyContent, remove 4 duplicate remembers Add missing imports in desktop ChessScreen, replace all FQN usages with short names Remove redundant distinctBy in completed games UI (state already prevents duplicates)
2026-03-23 20:37:57 +01:00
parent a6205a29c4
commit 6c8021f219
5 changed files with 76 additions and 149 deletions
@@ -48,6 +48,7 @@ import com.vitorpamplona.quartz.nip57Zaps.LnZapEvent
 import com.vitorpamplona.quartz.nip57Zaps.LnZapRequestEvent
 import com.vitorpamplona.quartz.nip59Giftwrap.seals.SealedRumorEvent
 import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent
+import com.vitorpamplona.quartz.nip64Chess.baseEvent.BaseChessEvent
 import com.vitorpamplona.quartz.nip64Chess.challenge.accept.LiveChessGameAcceptEvent
 import com.vitorpamplona.quartz.nip64Chess.move.LiveChessMoveEvent
 import com.vitorpamplona.quartz.utils.Log
@@ -107,12 +108,29 @@ class EventNotificationConsumer(
                Log.d(TAG, "Unwrapped consume ${innerEvent.javaClass.simpleName}")

                when (innerEvent) {
-                    is PrivateDmEvent -> notify(innerEvent, account)
-                    is LnZapEvent -> notify(innerEvent, account)
-                    is ChatMessageEvent -> notify(innerEvent, account)
-                    is ChatMessageEncryptedFileHeaderEvent -> notify(innerEvent, account)
-                    is LiveChessGameAcceptEvent -> notify(innerEvent, account)
-                    is LiveChessMoveEvent -> notify(innerEvent, account)
+                    is PrivateDmEvent -> {
+                        notify(innerEvent, account)
+                    }
+
+                    is LnZapEvent -> {
+                        notify(innerEvent, account)
+                    }
+
+                    is ChatMessageEvent -> {
+                        notify(innerEvent, account)
+                    }
+
+                    is ChatMessageEncryptedFileHeaderEvent -> {
+                        notify(innerEvent, account)
+                    }
+
+                    is LiveChessGameAcceptEvent -> {
+                        notifyChessEvent(innerEvent, account, R.string.app_notification_chess_challenge_accepted)
+                    }
+
+                    is LiveChessMoveEvent -> {
+                        notifyChessEvent(innerEvent, account, R.string.app_notification_chess_your_turn)
+                    }
                }
            }
        }
@@ -486,11 +504,11 @@ class EventNotificationConsumer(
        }
    }

-    private suspend fun notify(
-        event: LiveChessGameAcceptEvent,
+    private suspend fun notifyChessEvent(
+        event: BaseChessEvent,
        account: Account,
+        contentStringRes: Int,
    ) {
-        Log.d(TAG, "New Chess Challenge Accept to Notify")
        if (
            event.createdAt > TimeUtils.fifteenMinutesAgo() &&
            event.pubKey != account.signer.pubKey
@@ -499,40 +517,7 @@ class EventNotificationConsumer(
            val user = author.toBestDisplayName()
            val userPicture = author.profilePicture()
            val title = stringRes(applicationContext, R.string.app_notification_chess_channel_name)
-            val content = stringRes(applicationContext, R.string.app_notification_chess_challenge_accepted, user)
-            val noteUri =
-                "notifications$ACCOUNT_QUERY_PARAM" +
-                    account.signer.pubKey
-                        .hexToByteArray()
-                        .toNpub()
-
-            notificationManager()
-                .sendChessNotification(
-                    event.id,
-                    content,
-                    title,
-                    event.createdAt,
-                    userPicture,
-                    noteUri,
-                    applicationContext,
-                )
-        }
-    }
-
-    private suspend fun notify(
-        event: LiveChessMoveEvent,
-        account: Account,
-    ) {
-        Log.d(TAG, "New Chess Move to Notify")
-        if (
-            event.createdAt > TimeUtils.fifteenMinutesAgo() &&
-            event.pubKey != account.signer.pubKey
-        ) {
-            val author = LocalCache.getOrCreateUser(event.pubKey)
-            val user = author.toBestDisplayName()
-            val userPicture = author.profilePicture()
-            val title = stringRes(applicationContext, R.string.app_notification_chess_channel_name)
-            val content = stringRes(applicationContext, R.string.app_notification_chess_your_turn, user)
+            val content = stringRes(applicationContext, contentStringRes, user)
            val noteUri =
                "notifications$ACCOUNT_QUERY_PARAM" +
                    account.signer.pubKey
@@ -298,6 +298,10 @@ fun ChessLobbyContent(
    val challenges by chessViewModel.challenges.collectAsState()
    val completedGames by chessViewModel.completedGames.collectAsState()
    val userPubkey = accountViewModel.account.userProfile().pubkeyHex
+    val currentUser =
+        remember(userPubkey) {
+            accountViewModel.checkGetOrCreateUser(userPubkey)
+        }

    val hasContent =
        activeGames.isNotEmpty() ||
@@ -368,10 +372,6 @@ fun ChessLobbyContent(
                        accountViewModel.checkGetOrCreateUser(state.opponentPubkey)
                    }
                val displayName = opponent?.toBestDisplayName() ?: state.opponentPubkey.take(8)
-                val playerUser =
-                    remember(state.playerPubkey) {
-                        accountViewModel.checkGetOrCreateUser(state.playerPubkey)
-                    }
                ActiveGameCard(
                    gameId = gameId,
                    opponentName = displayName,
@@ -381,7 +381,7 @@ fun ChessLobbyContent(
                        OverlappingAvatars(
                            avatar1Hex = state.playerPubkey,
                            avatar2Hex = state.opponentPubkey,
-                            avatar1Url = playerUser?.profilePicture(),
+                            avatar1Url = currentUser?.profilePicture(),
                            avatar2Url = opponent?.profilePicture(),
                        )
                    },
@@ -410,10 +410,6 @@ fun ChessLobbyContent(
                val opponentName =
                    opponentUser?.toBestDisplayName()
                        ?: challenge.opponentPubkey?.take(8)
-                val currentUser =
-                    remember(userPubkey) {
-                        accountViewModel.checkGetOrCreateUser(userPubkey)
-                    }
                OutgoingChallengeCard(
                    opponentName = opponentName,
                    userPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
@@ -477,10 +473,6 @@ fun ChessLobbyContent(
                            ?: accountViewModel.checkGetOrCreateUser(challenge.challengerPubkey)?.toBestDisplayName()
                            ?: challenge.challengerPubkey.take(8)
                    }
-                val currentUser =
-                    remember(userPubkey) {
-                        accountViewModel.checkGetOrCreateUser(userPubkey)
-                    }
                ChallengeCard(
                    challengerName = displayName,
                    challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
@@ -518,10 +510,6 @@ fun ChessLobbyContent(
                            ?: accountViewModel.checkGetOrCreateUser(challenge.challengerPubkey)?.toBestDisplayName()
                            ?: challenge.challengerPubkey.take(8)
                    }
-                val currentUser =
-                    remember(userPubkey) {
-                        accountViewModel.checkGetOrCreateUser(userPubkey)
-                    }
                ChallengeCard(
                    challengerName = displayName,
                    challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
@@ -576,7 +564,7 @@ fun ChessLobbyContent(
            }

            items(
-                completedGames.distinctBy { it.gameId }.take(10),
+                completedGames.take(10),
                key = { "completed-${it.gameId}-${it.completedAt}" },
            ) { game ->
                val opponentPubkey =
@@ -601,10 +589,7 @@ fun ChessLobbyContent(
                        OverlappingAvatars(
                            avatar1Hex = userPubkey,
                            avatar2Hex = opponentPubkey,
-                            avatar1Url =
-                                remember(userPubkey) {
-                                    accountViewModel.checkGetOrCreateUser(userPubkey)
-                                }?.profilePicture(),
+                            avatar1Url = currentUser?.profilePicture(),
                            avatar2Url = opponentUser?.profilePicture(),
                        )
                    },
@@ -778,8 +778,7 @@ class ChessLobbyLogic(
                    // If the discovered game is already finished, send it straight to completed
                    val gameStatus = result.liveState.gameStatus.value
                    if (gameStatus is GameStatus.Finished) {
-                        // Use the direct helper to avoid the addActiveGame → moveToCompleted flicker
-                        state.addCompletedGameDirectly(startEventId, result.liveState, gameStatus.result.notation, null)
+                        state.moveToCompleted(startEventId, gameStatus.result.notation, null, liveState = result.liveState)
                    } else if (!result.liveState.isSpectator) {
                        state.addActiveGame(startEventId, result.liveState)
                        pollingDelegate.addGameId(startEventId)
@@ -394,31 +394,24 @@ class ChessLobbyState(
    }

    /**
-     * Move a game from active/spectating to completed.
-     * Display names are optional; UI can look them up later if needed.
+     * Move a game to completed. Looks up the game from active/spectating maps,
+     * or uses the provided [liveState] if the game isn't in either map yet
+     * (e.g. a newly discovered game that is already finished).
     */
    fun moveToCompleted(
        gameId: String,
        result: String,
        termination: String?,
+        liveState: LiveChessGameState? = null,
        whiteDisplayName: String? = null,
        blackDisplayName: String? = null,
    ) {
-        val existingState = _activeGames.value[gameId] ?: _spectatingGames.value[gameId]
-        existingState?.let { gameState ->
-            // Derive white/black pubkeys from player color
+        val gameState = _activeGames.value[gameId] ?: _spectatingGames.value[gameId] ?: liveState ?: return
+
        val whitePubkey =
-                if (gameState.playerColor == Color.WHITE) {
-                    gameState.playerPubkey
-                } else {
-                    gameState.opponentPubkey
-                }
+            if (gameState.playerColor == Color.WHITE) gameState.playerPubkey else gameState.opponentPubkey
        val blackPubkey =
-                if (gameState.playerColor == Color.BLACK) {
-                    gameState.playerPubkey
-                } else {
-                    gameState.opponentPubkey
-                }
+            if (gameState.playerColor == Color.BLACK) gameState.playerPubkey else gameState.opponentPubkey

        val completed =
            CompletedGame(
@@ -436,12 +429,7 @@ class ChessLobbyState(
            )

        _completedGames.update { current ->
-                // Avoid duplicates
-                if (current.any { it.gameId == gameId }) {
-                    current
-                } else {
-                    listOf(completed) + current
-                }
+            if (current.any { it.gameId == gameId }) current else listOf(completed) + current
        }

        // Remove from active/spectating
@@ -453,43 +441,6 @@ class ChessLobbyState(
            _selectedGameId.value = null
        }
    }
-    }
-
-    /**
-     * Build and record a CompletedGame directly from a LiveChessGameState without requiring the
-     * game to be inserted into activeGames first. Use this when a newly discovered game is already
-     * finished so we avoid the intermediate addActiveGame → moveToCompleted flicker.
-     */
-    fun addCompletedGameDirectly(
-        gameId: String,
-        liveState: LiveChessGameState,
-        result: String,
-        termination: String?,
-    ) {
-        val whitePubkey =
-            if (liveState.playerColor == Color.WHITE) liveState.playerPubkey else liveState.opponentPubkey
-        val blackPubkey =
-            if (liveState.playerColor == Color.BLACK) liveState.playerPubkey else liveState.opponentPubkey
-
-        val completed =
-            CompletedGame(
-                gameId = gameId,
-                whitePubkey = whitePubkey,
-                whiteDisplayName = null,
-                blackPubkey = blackPubkey,
-                blackDisplayName = null,
-                result = result,
-                termination = termination,
-                moveCount = liveState.moveHistory.value.size,
-                completedAt =
-                    com.vitorpamplona.quartz.utils.TimeUtils
-                        .now(),
-            )
-
-        _completedGames.update { current ->
-            if (current.any { it.gameId == gameId }) current else listOf(completed) + current
-        }
-    }

    fun addSpectatingGame(
        gameId: String,
@@ -43,7 +43,6 @@ import androidx.compose.material.icons.automirrored.filled.ArrowBack
 import androidx.compose.material.icons.automirrored.filled.KeyboardArrowLeft
 import androidx.compose.material.icons.automirrored.filled.KeyboardArrowRight
 import androidx.compose.material.icons.filled.Add
-import androidx.compose.material.icons.filled.ArrowBack
 import androidx.compose.material.icons.filled.Refresh
 import androidx.compose.material.icons.filled.Visibility
 import androidx.compose.material3.Button
@@ -67,14 +66,22 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.text.font.FontWeight
 import androidx.compose.ui.unit.dp
 import androidx.compose.ui.unit.min
+import com.vitorpamplona.amethyst.commons.chess.ActiveGameCard
+import com.vitorpamplona.amethyst.commons.chess.ChallengeCard
 import com.vitorpamplona.amethyst.commons.chess.ChessBroadcastBanner
 import com.vitorpamplona.amethyst.commons.chess.ChessChallenge
 import com.vitorpamplona.amethyst.commons.chess.ChessConfig
+import com.vitorpamplona.amethyst.commons.chess.ChessPlayerVsHeader
 import com.vitorpamplona.amethyst.commons.chess.ChessSyncBanner
 import com.vitorpamplona.amethyst.commons.chess.CompletedGame
+import com.vitorpamplona.amethyst.commons.chess.CompletedGameCard
 import com.vitorpamplona.amethyst.commons.chess.InteractiveChessBoard
 import com.vitorpamplona.amethyst.commons.chess.NewChessGameDialog
+import com.vitorpamplona.amethyst.commons.chess.OutgoingChallengeCard
+import com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars
 import com.vitorpamplona.amethyst.commons.chess.PublicGame
+import com.vitorpamplona.amethyst.commons.chess.PublicGameCard
+import com.vitorpamplona.amethyst.commons.chess.SpectatingGameCard
 import com.vitorpamplona.amethyst.commons.data.UserMetadataCache
 import com.vitorpamplona.amethyst.commons.ui.components.UserAvatar
 import com.vitorpamplona.amethyst.desktop.account.AccountState
@@ -415,13 +422,13 @@ private fun ChessLobby(
            }

            items(activeGames.entries.toList(), key = { "active-${it.key}" }) { (gameId, state) ->
-                com.vitorpamplona.amethyst.commons.chess.ActiveGameCard(
+                ActiveGameCard(
                    gameId = gameId,
                    opponentName = metadataCache.getDisplayName(state.opponentPubkey),
                    isYourTurn = state.isPlayerTurn(),
                    onClick = { onSelectGame(gameId) },
                    avatar = {
-                        com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars(
+                        OverlappingAvatars(
                            avatar1Hex = state.playerPubkey,
                            avatar2Hex = state.opponentPubkey,
                            avatar1Url = metadataCache.getPictureUrl(state.playerPubkey),
@@ -446,14 +453,14 @@ private fun ChessLobby(
            }

            items(outgoingChallenges, key = { "outgoing-${it.eventId}" }) { challenge ->
-                com.vitorpamplona.amethyst.commons.chess.OutgoingChallengeCard(
+                OutgoingChallengeCard(
                    opponentName = challenge.opponentPubkey?.let { metadataCache.getDisplayName(it) },
                    userPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
                    onClick = { onOpenOwnChallenge(challenge) },
                    avatar =
                        challenge.opponentPubkey?.let { pubkey ->
                            {
-                                com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars(
+                                OverlappingAvatars(
                                    avatar1Hex = userPubkey,
                                    avatar2Hex = pubkey,
                                    avatar1Url = metadataCache.getPictureUrl(userPubkey),
@@ -479,7 +486,7 @@ private fun ChessLobby(

            items(spectatingGames.entries.toList(), key = { "spectating-${it.key}" }) { (gameId, state) ->
                val moveCount by state.moveHistory.collectAsState()
-                com.vitorpamplona.amethyst.commons.chess.SpectatingGameCard(
+                SpectatingGameCard(
                    moveCount = moveCount.size,
                    onClick = { onSelectGame(gameId) },
                )
@@ -500,13 +507,13 @@ private fun ChessLobby(
            }

            items(incomingChallenges, key = { "incoming-${it.eventId}" }) { challenge ->
-                com.vitorpamplona.amethyst.commons.chess.ChallengeCard(
+                ChallengeCard(
                    challengerName = challenge.challengerDisplayName ?: metadataCache.getDisplayName(challenge.challengerPubkey),
                    challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
                    isIncoming = true,
                    onAccept = { onAcceptChallenge(challenge) },
                    avatar = {
-                        com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars(
+                        OverlappingAvatars(
                            avatar1Hex = challenge.challengerPubkey,
                            avatar2Hex = userPubkey,
                            avatar1Url = challenge.challengerAvatarUrl ?: metadataCache.getPictureUrl(challenge.challengerPubkey),
@@ -531,13 +538,13 @@ private fun ChessLobby(
            }

            items(openChallenges, key = { "open-${it.eventId}" }) { challenge ->
-                com.vitorpamplona.amethyst.commons.chess.ChallengeCard(
+                ChallengeCard(
                    challengerName = challenge.challengerDisplayName ?: metadataCache.getDisplayName(challenge.challengerPubkey),
                    challengerPlaysWhite = challenge.challengerColor == ChessColor.WHITE,
                    isIncoming = false,
                    onAccept = { onAcceptChallenge(challenge) },
                    avatar = {
-                        com.vitorpamplona.amethyst.commons.chess.OverlappingAvatars(
+                        OverlappingAvatars(
                            avatar1Hex = challenge.challengerPubkey,
                            avatar2Hex = userPubkey,
                            avatar1Url = challenge.challengerAvatarUrl ?: metadataCache.getPictureUrl(challenge.challengerPubkey),
@@ -561,7 +568,7 @@ private fun ChessLobby(
            }

            items(publicGames, key = { "public-${it.gameId}" }) { game ->
-                com.vitorpamplona.amethyst.commons.chess.PublicGameCard(
+                PublicGameCard(
                    whiteName = game.whiteDisplayName ?: metadataCache.getDisplayName(game.whitePubkey),
                    blackName = game.blackDisplayName ?: metadataCache.getDisplayName(game.blackPubkey),
                    moveCount = game.moveCount,
@@ -583,13 +590,13 @@ private fun ChessLobby(
            }

            items(
-                completedGames.distinctBy { it.gameId }.take(10),
+                completedGames.take(10),
                key = { "completed-${it.gameId}-${it.completedAt}" },
            ) { game ->
                // Derive opponent pubkey based on who the user is
                val opponentPubkey =
                    if (game.whitePubkey == userPubkey) game.blackPubkey else game.whitePubkey
-                com.vitorpamplona.amethyst.commons.chess.CompletedGameCard(
+                CompletedGameCard(
                    opponentName = game.blackDisplayName ?: game.whiteDisplayName ?: metadataCache.getDisplayName(opponentPubkey),
                    result = game.result,
                    didUserWin = game.didUserWin(userPubkey),
@@ -674,7 +681,7 @@ private fun DesktopChessGameLayout(
                ) {
                    // Player vs Player header above board
                    if (whiteHex.isNotEmpty() || blackHex.isNotEmpty()) {
-                        com.vitorpamplona.amethyst.commons.chess.ChessPlayerVsHeader(
+                        ChessPlayerVsHeader(
                            whiteName = whiteName,
                            whiteHex = whiteHex,
                            whiteAvatarUrl = whiteAvatarUrl,