Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ upload_certificate.pem
/.jjconflict-base-*
*.o
/ios/build

.DS_Store
151 changes: 148 additions & 3 deletions android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.bitcoinppl.cove.sidebar
import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.clickable
import androidx.compose.foundation.gestures.detectDragGesturesAfterLongPress
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
Expand All @@ -18,7 +19,8 @@ import androidx.compose.foundation.layout.safeDrawing
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.itemsIndexed
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.icons.Icons
Expand All @@ -30,14 +32,28 @@ import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableFloatStateOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.graphicsLayer
import androidx.compose.ui.hapticfeedback.HapticFeedbackType
import androidx.compose.ui.input.pointer.pointerInput
import androidx.compose.ui.platform.LocalHapticFeedback
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import org.bitcoinppl.cove.AppManager
import org.bitcoinppl.cove.R
import org.bitcoinppl.cove.ui.theme.CoveColor
Expand All @@ -47,12 +63,29 @@ import org.bitcoinppl.cove_core.RouteFactory
import org.bitcoinppl.cove_core.SettingsRoute
import org.bitcoinppl.cove_core.WalletColor
import org.bitcoinppl.cove_core.WalletMetadata
import android.util.Log
Comment thread
geeekyvishal marked this conversation as resolved.
import androidx.compose.foundation.Image

@Composable
fun SidebarView(
app: AppManager,
modifier: Modifier = Modifier,
) {
var walletList by remember { mutableStateOf(app.wallets) }
var draggedWalletId by remember { mutableStateOf<String?>(null) }
var draggedDistance by remember { mutableFloatStateOf(0f) }
var dragStartCenterY by remember { mutableFloatStateOf(0f) }
val listState = rememberLazyListState()
val haptic = LocalHapticFeedback.current
val scope = rememberCoroutineScope()

LaunchedEffect(app.wallets, draggedWalletId) {
// Keep local list in sync with source-of-truth while not actively dragging.
if (draggedWalletId == null) {
walletList = app.wallets
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Column(
modifier =
modifier
Expand Down Expand Up @@ -116,12 +149,111 @@ fun SidebarView(
// wallet list
LazyColumn(
modifier = Modifier.weight(1f),
state = listState,
verticalArrangement = Arrangement.spacedBy(12.dp),
) {
items(app.wallets) { wallet ->
itemsIndexed(
items = walletList,
key = { _, wallet -> wallet.id.toString() },
) { _, wallet ->
val isDragged = wallet.id == draggedWalletId
WalletItem(
wallet = wallet,
modifier =
Modifier
.graphicsLayer {
translationY = if (isDragged) draggedDistance else 0f
}.pointerInput(wallet.id) {
detectDragGesturesAfterLongPress(
onDragStart = {
draggedWalletId = wallet.id
draggedDistance = 0f
val itemInfo =
listState.layoutInfo.visibleItemsInfo.firstOrNull {
it.key == wallet.id.toString()
}
dragStartCenterY =
if (itemInfo != null) {
itemInfo.offset + (itemInfo.size / 2f)
} else {
0f
}
haptic.performHapticFeedback(HapticFeedbackType.LongPress)
},
onDrag = { change, dragAmount ->
change.consume()
val draggedId = draggedWalletId ?: return@detectDragGesturesAfterLongPress
draggedDistance += dragAmount.y
val fromIndex = walletList.indexOfFirst { it.id == draggedId }
if (fromIndex == -1) return@detectDragGesturesAfterLongPress

val currentCenterY = dragStartCenterY + draggedDistance
val targetInfo =
listState.layoutInfo.visibleItemsInfo.firstOrNull { info ->
currentCenterY >= info.offset &&
currentCenterY <= info.offset + info.size
}
?: return@detectDragGesturesAfterLongPress

val toIndex = targetInfo.index
if (toIndex == fromIndex) return@detectDragGesturesAfterLongPress
if (toIndex !in walletList.indices) return@detectDragGesturesAfterLongPress

walletList = walletList.move(fromIndex, toIndex)
val refreshedInfo =
listState.layoutInfo.visibleItemsInfo.firstOrNull { it.index == toIndex }
if (refreshedInfo != null) {
dragStartCenterY = refreshedInfo.offset + (refreshedInfo.size / 2f)
draggedDistance = currentCenterY - dragStartCenterY
} else {
draggedDistance = 0f
}
},
onDragEnd = {
val draggedId = draggedWalletId
if (draggedId != null) {
val appOrder = app.wallets.map { it.id }
val localOrder = walletList.map { it.id }
if (localOrder != appOrder) {
scope.launch(Dispatchers.IO) {
runCatching {
app.database.wallets().reorderWallets(orderedIds = localOrder)
}.onSuccess {
withContext(Dispatchers.Main) {
draggedWalletId = null
draggedDistance = 0f
dragStartCenterY = 0f
haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove)
}
}.onFailure {
Log.e("SidebarView", "Failed to reorder wallets", it)
withContext(Dispatchers.Main) {
walletList = app.wallets
draggedWalletId = null
draggedDistance = 0f
dragStartCenterY = 0f
haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove)
}
}
}
return@detectDragGesturesAfterLongPress
}
Comment on lines +212 to +240
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Blocking disk I/O on the UI thread

app.database.wallets().reorderWallets(...) is a synchronous Rust FFI call that writes to the redb database. Calling it directly in onDragEnd (which runs on the composition/main thread) can cause dropped frames or jank on slower devices, especially if the database write takes more than a single frame (~16 ms).

Consider launching the call in a coroutine scope (e.g., rememberCoroutineScope()) so it is dispatched off the main thread:

coroutineScope.launch(Dispatchers.IO) {
    runCatching {
        app.database.wallets().reorderWallets(orderedIds = localOrder)
    }.onFailure {
        Log.e("SidebarView", "Failed to reorder wallets", it)
        withContext(Dispatchers.Main) { walletList = app.wallets }
    }
}

The iOS persistWalletOrder() has the same pattern and would benefit from an equivalent Task { await ... } wrapper.

}
Comment on lines +215 to +241
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't dispatch the Rust FFI reorder call to Dispatchers.IO.

Per the repository FFI/Threading policy, Rust FFI calls are synchronous and fast (microseconds), Tokio is handled inside the core, and redb does not block the UI thread. scope.launch(Dispatchers.IO) { ... reorderWallets(...) } should only be used with profiling evidence showing >16ms UI blocking. Running on Main also removes the race described in the neighboring comment on the LaunchedEffect: app.wallets will be updated (via WalletsChanged) before draggedWalletId is cleared below, so walletList won't flicker back to the old order. The withContext(Dispatchers.Main) { walletList = app.wallets } inside onFailure is also redundant — clearing draggedWalletId a few lines later will re-trigger the LaunchedEffect and resync walletList from app.wallets on its own.

🛠️ Suggested simplification
-                                        if (localOrder != appOrder) {
-                                            scope.launch(Dispatchers.IO) {
-                                                runCatching {
-                                                    app.database.wallets().reorderWallets(orderedIds = localOrder)
-                                                }.onFailure {
-                                                    Log.e("SidebarView", "Failed to reorder wallets", it)
-                                                    withContext(Dispatchers.Main) {
-                                                        walletList = app.wallets
-                                                    }
-                                                }
-                                            }
-                                        }
+                                        if (localOrder != appOrder) {
+                                            runCatching {
+                                                app.database.wallets().reorderWallets(orderedIds = localOrder)
+                                            }.onFailure {
+                                                Log.e("SidebarView", "Failed to reorder wallets", it)
+                                                walletList = app.wallets
+                                            }
+                                        }

With this change, scope/rememberCoroutineScope and the kotlinx.coroutines.Dispatchers/launch/withContext imports on lines 39, 54–56 are no longer needed in this function.

As per coding guidelines: "NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))" and "ONLY suggest Dispatchers.IO with profiling evidence showing >16ms UI blocking".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@android/app/src/main/java/org/bitcoinppl/cove/sidebar/SidebarView.kt` around
lines 215 - 229, The reorderWallets FFI call is incorrectly dispatched to
Dispatchers.IO causing unnecessary threading and potential race; change the
block that currently does scope.launch(Dispatchers.IO) { runCatching {
app.database.wallets().reorderWallets(orderedIds = localOrder) } ... } to invoke
app.database.wallets().reorderWallets(orderedIds = localOrder) directly on the
current (Main) thread inside the existing LaunchedEffect so it runs
synchronously; remove the surrounding scope.launch(Dispatchers.IO) and the
withContext(Dispatchers.Main) fallback (the onFailure should just log the
error), and then remove any now-unused coroutine imports and the
rememberCoroutineScope usage since they’re no longer needed; ensure walletList
and draggedWalletId behavior remains driven by the existing
LaunchedEffect/WalletsChanged flow.

draggedWalletId = null
draggedDistance = 0f
dragStartCenterY = 0f
haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove)
},
onDragCancel = {
draggedWalletId = null
draggedDistance = 0f
dragStartCenterY = 0f
walletList = app.wallets
},
)
},
onClick = {
if (draggedWalletId != null) return@WalletItem
app.closeSidebarAndNavigate {
app.rust.selectWallet(wallet.id)
}
Expand Down Expand Up @@ -202,11 +334,12 @@ fun SidebarView(
@Composable
private fun WalletItem(
wallet: WalletMetadata,
modifier: Modifier = Modifier,
onClick: () -> Unit,
) {
Row(
modifier =
Modifier
modifier
.fillMaxWidth()
.clip(RoundedCornerShape(10.dp))
.background(CoveColor.coveLightGray.copy(alpha = 0.06f))
Expand Down Expand Up @@ -235,6 +368,18 @@ private fun WalletItem(
}
}

private fun List<WalletMetadata>.move(
fromIndex: Int,
toIndex: Int,
): List<WalletMetadata> {
if (fromIndex == toIndex) return this
val mutable = toMutableList()
val item = mutable.removeAt(fromIndex)
val safeTarget = toIndex.coerceIn(0, mutable.size)
mutable.add(safeTarget, item)
return mutable.toList()
}

// convert wallet color to compose color
private fun WalletColor.toComposeColor(): Color =
when (this) {
Expand Down
Loading
Loading