feat: masternode keys redesign with components#1467
Merged
HashEngineering merged 16 commits intomasterfrom Mar 8, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ListItem and ListEmptyState UI components, introduces named NavBar variants around TopNavBase, migrates masternode keys screens from XML/RecyclerView to Jetpack Compose, introduces StateFlow-backed masternode UI state and viewmodel logic, and removes legacy XML layouts and RecyclerView adapters/view-holders. Changes
Sequence DiagramsequenceDiagram
participant Frag as MasternodeFragment
participant VM as MasternodeKeysViewModel
participant SF as StateFlow(MasternodeKeysUIState)
participant Screen as ComposeScreen
Frag->>Frag: onCreateView() returns ComposeView
Frag->>VM: subscribe / request init
VM->>SF: emit MasternodeKeysUIState
Screen->>SF: collect()
SF-->>Screen: UI state emitted
Screen->>Frag: render list (compose)
Frag->>VM: user selects key type (onKeyTypeClick)
VM->>SF: emit MasternodeKeyChainUIState
Screen->>Screen: navigate to KeyChainScreen and render items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/DEVELOPMENT-PATTERNS.md:
- Around line 84-86: Several markdown code fences in
.claude/agents/DEVELOPMENT-PATTERNS.md are missing language identifiers
(triggering MD040); update each fence around the examples that show file paths
and the ASCII layout to include a language tag such as "text". Specifically,
change the triple-backtick fences for the blocks containing
"common/src/main/java/org/dash/wallet/common/ui/components/TopNavBase.kt",
"common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt", and the
ASCII layout block ("[topLabel ...] Row { ... } [bottomLabel]") to use ```text
so markdownlint MD040 is satisfied and the content remains unchanged.
In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt`:
- Around line 186-192: The info Icon in ListItem.kt (the Icon composable shown
when showInfoIcon is true) currently sets contentDescription = null which hides
a useful accessibility cue; update the Icon to supply a meaningful
contentDescription (e.g., a string resource like R.string.info_icon_description
or a passed-in parameter such as infoIconDescription) and use that value for
contentDescription so screen readers announce the indicator; if the icon
triggers an action, ensure the description reflects the action (e.g., "show
details"), otherwise use a concise label like "info"; update both occurrences
around showInfoIcon to use the same accessible description.
- Around line 122-123: The trailingActionText is rendered like an actionable
blue label but has no dedicated click behavior when onClick is null; update the
ListItem composable so the trailingActionText element (and trailingLabel if
applicable) only receives a Modifier.clickable tied to the provided onClick
handler (use Modifier.clickable { onClick() } or .then(Modifier) conditional)
and when onClick is null render it with non-action styling/semantics (e.g.,
default text color and no clickable semantics) so it doesn't appear tappable;
locate references to trailingActionText, trailingLabel and onClick in the
ListItem implementation and apply this conditional clickable modifier and style
change.
In `@common/src/main/java/org/dash/wallet/common/ui/components/TopNavBase.kt`:
- Around line 216-343: Several icon-only NavBar composables (e.g., NavBarBack,
NavBarBackPlus, NavBarClose and the icon-using variants NavBarBackTitleInfo,
NavBarBackTitlePlus, NavBarTitleClose) currently pass no content descriptions
into TopNavBase, leaving controls unlabeled for assistive tech; add optional
parameters for leadingContentDescription and trailingContentDescription (with
sensible defaults or nullable) to those composable signatures and forward them
into the TopNavBase call (which should accept/forward the same props), ensuring
the leadingIcon/trailingIcon are rendered with the provided content
descriptions.
- Around line 157-180: The bare trailing Icon currently attaches the click
handler directly to the 22.dp Icon (inside the Box), which yields a too-small
touch target; instead, move the clickable modifier to the parent Box (the Box
with .align(Alignment.CenterEnd)) when onTrailingClick is non-null and ensure
that Box's minimum size is at least 48.dp (e.g., use .size(48.dp) or
.requiredSizeIn(minWidth = 48.dp, minHeight = 48.dp) while keeping the visual
Icon at 22.dp), and for the trailingIconCircle branch preserve the current
Template click behavior but ensure the Box click handling covers the non-circle
case; reference Box, trailingIconCircle, Template, Icon, and onTrailingClick.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt`:
- Around line 67-71: The onCopy lambda in MasternodeKeyChainFragment is logging
the copied text (log.info("text copied to clipboard: {}", text)), which can leak
private keys; remove or replace that logging with a non-sensitive message that
does not include the text (e.g., log.info("masternode key copied to clipboard")
or omit logging entirely). Locate the onCopy handler that calls
viewModel.copyToClipboard(text) and Toast(...).toast(R.string.copied) and delete
or redact the log.info call so no copied value is written to logs.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt`:
- Line 225: Replace the realistic-looking address literal used for preview in
MasternodeKeyChainScreen (the KeyFieldEntry call with KeyFieldType.ADDRESS) with
an explicit non-realistic placeholder string (e.g. "ADDRESS_PLACEHOLDER" or
"example-address-0000") so secret scanners aren’t triggered; update the
KeyFieldEntry instantiation in MasternodeKeyChainScreen.kt to use that dummy
placeholder instead of "XuuRQMVEK9fQMsoAegE32Bdc1XvHhAiWa9".
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt`:
- Around line 312-314: The ViewModel has multiple async state holders
(_keyChainUIState/keyChainUIState and LiveData newKeysFound) instead of a single
UIState; consolidate MasternodeKeyChainUIState into the existing uiState data
class and remove the separate _keyChainUIState/keyChainUIState and newKeysFound
LiveData: merge the fields from MasternodeKeyChainUIState into the uiState
model, update any producers/consumers to read/write via the single
MutableStateFlow uiState, and replace emissions of newKeysFound with updates on
uiState so all fragments observe the single StateFlow (refer to symbols
_keyChainUIState, keyChainUIState, uiState, newKeysFound, and
MasternodeKeyChainUIState when making changes).
- Around line 317-324: initKeyChainScreen mutates shared collections
(masternodeKeyChainInfoMap / masternodeKeyInfoList) from Dispatchers.IO which
can race with other UI-thread mutations; change the coroutine so only the CPU/IO
work (getDecryptedKey) runs on IO, then switch to the Main dispatcher to perform
the assignment to keyChainInfo.masternodeKeyInfoList[position] and call
rebuildKeyChainState, or alternatively protect mutations with a Mutex around
masternodeKeyChainInfoMap/masternodeKeyInfoList; update the block in the
viewModelScope.launch(Dispatchers.IO) where getDecryptedKey(...) is called so
mutation and rebuildKeyChainState(...) happen on Dispatchers.Main (or under the
Mutex) and apply the same pattern for the similar section at lines 329-352.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.claude/agents/DEVELOPMENT-PATTERNS.mdcommon/src/main/java/org/dash/wallet/common/ui/components/ListItem.ktcommon/src/main/java/org/dash/wallet/common/ui/components/TopNavBase.ktcommon/src/main/res/drawable/ic_list_chevron_right.xmlwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysFragment.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysScreen.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt (1)
236-236:⚠️ Potential issue | 🟡 MinorReplace realistic preview address with an explicit dummy placeholder.
Line 236 still uses a realistic address-like literal, which is triggering secret scanning noise. Please switch it to a clearly fake placeholder string.
🔧 Suggested patch
- KeyFieldEntry(KeyFieldType.ADDRESS, "XuuRQMVEK9fQMsoAegE32Bdc1XvHhAiWa9"), + KeyFieldEntry(KeyFieldType.ADDRESS, "example-address-0000"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt` at line 236, Replace the realistic-looking address literal used in the preview KeyFieldEntry to avoid secret-scanning noise: locate the instantiation of KeyFieldEntry with KeyFieldType.ADDRESS (in MasternodeKeyChainScreen / the preview list) and change the string "XuuRQMVEK9fQMsoAegE32Bdc1XvHhAiWa9" to a clearly fake placeholder such as "DUMMY_ADDRESS_PLACEHOLDER" (or similar) so it no longer resembles a real address.
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysScreen.kt (1)
54-54: UsecollectAsStateWithLifecycle()for lifecycle-aware flow collection.Line 54 uses
collectAsState(). For Android Composable screens,collectAsStateWithLifecycle()is the recommended approach to avoid collecting state outside active lifecycle states. Theandroidx.lifecycle:lifecycle-runtime-composedependency is already available in the project, and this pattern is already adopted in other parts of the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysScreen.kt` at line 54, Replace the lifecycle-unsafe flow collection in MasternodeKeysScreen.kt: change the uiState assignment that uses uiStateFlow.collectAsState() to use collectAsStateWithLifecycle() instead (i.e., update the expression assigning uiState in the composable), and add the corresponding import for the lifecycle-aware extension if missing (collectAsStateWithLifecycle from androidx.lifecycle.compose). Ensure the symbol referenced is the uiStateFlow in the MasternodeKeysScreen composable and that the resulting uiState variable remains a by delegated state (val uiState by ...).wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt (1)
62-62: Consider lifecycle-aware StateFlow collection in screen composables.The
lifecycle-runtime-composedependency is available in the wallet module. While line 62 currently usescollectAsState(), the recommended best practice iscollectAsStateWithLifecycle()to avoid collecting while the UI is not in an active lifecycle state. This would require coordinated refactoring across the wallet module, as all screen composables currently use the non-lifecycle-aware approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt` at line 62, Replace the non-lifecycle-aware collection of uiStateFlow in MasternodeKeyChainScreen (the line declaring "val uiState by uiStateFlow.collectAsState()") with the lifecycle-aware API: use collectAsStateWithLifecycle() on uiStateFlow, add the import for androidx.lifecycle.compose.collectAsStateWithLifecycle, and ensure the module has lifecycle-runtime-compose on the classpath; repeat the same change for other screen composables that currently call collectAsState() so collection only occurs while the UI is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt`:
- Around line 281-288: The trailing help icon currently uses Icon(...
contentDescription = null) which hides it from screen readers; update the
ListItem composable to provide an accessible description by using a non-null
contentDescription (e.g., derive from trailingHelpText or add a new parameter
trailingHelpIconContentDescription: String? = null) and pass that into the Icon
call instead of null (refer to trailingHelpIcon, trailingHelpText and the Icon
invocation in ListItem.kt); ensure the description is optional but when present
is used so the icon conveys its contextual meaning to accessibility services.
---
Duplicate comments:
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt`:
- Line 236: Replace the realistic-looking address literal used in the preview
KeyFieldEntry to avoid secret-scanning noise: locate the instantiation of
KeyFieldEntry with KeyFieldType.ADDRESS (in MasternodeKeyChainScreen / the
preview list) and change the string "XuuRQMVEK9fQMsoAegE32Bdc1XvHhAiWa9" to a
clearly fake placeholder such as "DUMMY_ADDRESS_PLACEHOLDER" (or similar) so it
no longer resembles a real address.
---
Nitpick comments:
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt`:
- Line 62: Replace the non-lifecycle-aware collection of uiStateFlow in
MasternodeKeyChainScreen (the line declaring "val uiState by
uiStateFlow.collectAsState()") with the lifecycle-aware API: use
collectAsStateWithLifecycle() on uiStateFlow, add the import for
androidx.lifecycle.compose.collectAsStateWithLifecycle, and ensure the module
has lifecycle-runtime-compose on the classpath; repeat the same change for other
screen composables that currently call collectAsState() so collection only
occurs while the UI is active.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysScreen.kt`:
- Line 54: Replace the lifecycle-unsafe flow collection in
MasternodeKeysScreen.kt: change the uiState assignment that uses
uiStateFlow.collectAsState() to use collectAsStateWithLifecycle() instead (i.e.,
update the expression assigning uiState in the composable), and add the
corresponding import for the lifecycle-aware extension if missing
(collectAsStateWithLifecycle from androidx.lifecycle.compose). Ensure the symbol
referenced is the uiStateFlow in the MasternodeKeysScreen composable and that
the resulting uiState variable remains a by delegated state (val uiState by
...).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
common/src/main/java/org/dash/wallet/common/ui/components/ListItem.ktwallet/res/layout/fragment_masternode_key_chain.xmlwallet/res/layout/fragment_masternode_key_types.xmlwallet/res/layout/masternode_key_row.xmlwallet/res/layout/masternode_key_type_row.xmlwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainAdapter.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyTypeAdapter.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyTypeViewHolder.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyViewHolder.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysScreen.kt
💤 Files with no reviewable changes (8)
- wallet/res/layout/fragment_masternode_key_chain.xml
- wallet/res/layout/fragment_masternode_key_types.xml
- wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyViewHolder.kt
- wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyTypeAdapter.kt
- wallet/res/layout/masternode_key_row.xml
- wallet/res/layout/masternode_key_type_row.xml
- wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainAdapter.kt
- wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyTypeViewHolder.kt
Replace the separate _keyChainUIState/keyChainUIState StateFlow and (keyChainState, newKeysFound). Update MasternodeKeyChainScreen to accept StateFlow<MasternodeKeysUIState> and MasternodeKeyChainFragment to observe uiState via repeatOnLifecycle instead of LiveData.observe.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt (1)
70-74:⚠️ Potential issue | 🔴 CriticalDo not log copied key values.
Line 73 logs the raw copied text, which can include private key material.
Suggested fix
onCopy = { text -> viewModel.copyToClipboard(text) Toast(requireContext()).toast(R.string.copied) - log.info("text copied to clipboard: {}", text) + log.info("masternode key copied to clipboard") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt` around lines 70 - 74, The onCopy lambda in MasternodeKeyChainFragment currently logs the raw copied text (log.info("text copied to clipboard: {}", text)), which may include private key material; remove the sensitive value from logs by deleting the text argument or replacing the statement with a non-sensitive message (e.g., log.info("text copied to clipboard") ) and keep calls to viewModel.copyToClipboard(text) and Toast as-is; also scan for other usages of log.info/log.debug that reference the copied key to ensure no private key material is logged.wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt (1)
236-236:⚠️ Potential issue | 🟡 MinorReplace preview key/address samples with explicit placeholders.
Line 236 uses a realistic-looking address literal and is already tripping secret scanning.
Suggested fix
- KeyFieldEntry(KeyFieldType.ADDRESS, "XuuRQMVEK9fQMsoAegE32Bdc1XvHhAiWa9"), + KeyFieldEntry(KeyFieldType.ADDRESS, "ADDRESS_PLACEHOLDER"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt` at line 236, Replace the realistic-looking address literal used as a preview in MasternodeKeyChainScreen (the KeyFieldEntry call with KeyFieldType.ADDRESS) with a non-sensitive explicit placeholder string (e.g. "ADDRESS_PLACEHOLDER" or "<address>") or a named constant (e.g. SAMPLE_ADDRESS) to avoid triggering secret scanners; update the KeyFieldEntry instantiation in the MasternodeKeyChainScreen.kt preview accordingly and ensure any related preview/test code uses the same placeholder constant instead of real-looking addresses.common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt (3)
299-309:⚠️ Potential issue | 🟠 MajorDon’t style
trailingActionTextas a link when it isn’t clickable.Line 303 always uses
dashBlueeven whenonTrailingActionClickis null, which looks actionable but does nothing.Suggested fix
trailingActionText?.let { + val actionEnabled = onTrailingActionClick != null Text( text = it, style = MyTheme.Typography.LabelLarge, - color = MyTheme.Colors.dashBlue, - modifier = if (onTrailingActionClick != null) { + color = if (actionEnabled) MyTheme.Colors.dashBlue else MyTheme.Colors.textTertiary, + modifier = if (actionEnabled) { Modifier.clickable { onTrailingActionClick() } } else { Modifier } ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt` around lines 299 - 309, The trailingActionText is always styled with MyTheme.Colors.dashBlue making it look clickable even when onTrailingActionClick is null; update the Text styling in ListItem.kt so the color is conditional: use MyTheme.Colors.dashBlue only when onTrailingActionClick != null, otherwise use the non-action/label color (e.g., MyTheme.Colors.onBackground or the existing LabelLarge default). Keep the clickable Modifier only when onTrailingActionClick is present and ensure color and modifier choices are paired to avoid presenting non-clickable text as a link.
282-288:⚠️ Potential issue | 🟡 MinorExpose an accessibility description for
trailingHelpIcon.Line 285 hides the icon from accessibility services with
contentDescription = null.Suggested fix
fun ListItem( @@ trailingHelpText: String? = null, `@DrawableRes` trailingHelpIcon: Int? = null, + trailingHelpIconContentDescription: String? = null, @@ trailingHelpIcon?.let { iconRes -> Icon( painter = painterResource(iconRes), - contentDescription = null, + contentDescription = trailingHelpIconContentDescription ?: helpText, tint = MyTheme.Colors.textTertiary, modifier = Modifier.size(12.dp) ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt` around lines 282 - 288, The trailing help icon is hidden from accessibility by always using contentDescription = null; update the ListItem composable to accept an optional accessibility string (e.g., trailingHelpIconContentDescription: String? = null) and pass that into the Icon's contentDescription (replace the hardcoded null in the Icon call inside ListItem.kt), so screen readers can announce the icon when provided; ensure callers of ListItem are updated where appropriate to supply a localized description (or leave null to preserve current behavior).
188-194:⚠️ Potential issue | 🟠 MajorAdd accessible descriptions for
showInfoIconindicators.Line 191 and Line 227 set
contentDescription = null, so screen readers won’t announce a meaningful indicator.Suggested fix
fun ListItem( @@ showInfoIcon: Boolean = false, + infoIconContentDescription: String = "Info", @@ if (showInfoIcon) { Icon( painter = painterResource(android.R.drawable.ic_dialog_info), - contentDescription = null, + contentDescription = infoIconContentDescription, tint = MyTheme.Colors.textTertiary, modifier = Modifier.size(14.dp) ) } @@ if (showInfoIcon) { Icon( painter = painterResource(android.R.drawable.ic_dialog_info), - contentDescription = null, + contentDescription = infoIconContentDescription, tint = MyTheme.Colors.textTertiary, modifier = Modifier.size(14.dp) ) }Also applies to: 224-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt` around lines 188 - 194, The info-icons rendered when showInfoIcon is true currently set contentDescription = null, which makes them inaccessible; update both Icon usages in ListItem.kt to provide a meaningful contentDescription (e.g., use stringResource(R.string.info_icon_description) or accept a new parameter like infoIconDescription: String?) instead of null, so screen readers announce the indicator; keep contentDescription = null only if the icon is purely decorative, otherwise pass the descriptive string to the Icon composable (or wire a parameter through the ListItem composable) so accessibility is preserved.wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt (1)
308-319:⚠️ Potential issue | 🟠 MajorGuard keychain list mutations to avoid cross-coroutine races.
initKeyChainScreen()mutatesmasternodeKeyInfoListonDispatchers.IOand rebuilds UI state while other paths can mutate/read the same structures. This can produce inconsistent state snapshots.Suggested direction
+import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock @@ + private val keyChainStateMutex = Mutex() @@ fun initKeyChainScreen(type: MasternodeKeyType) { rebuildKeyChainState(type) viewModelScope.launch(Dispatchers.IO) { - val keyChainInfo = masternodeKeyChainInfoMap[type] ?: return@launch - keyChainInfo.masternodeKeyInfoList.forEachIndexed { position, keyInfo -> - if (keyInfo.privateKeyHex == null) { - val decrypted = getDecryptedKey(keyInfo.masternodeKey) - keyChainInfo.masternodeKeyInfoList[position] = decrypted - rebuildKeyChainState(type) - } - } + keyChainStateMutex.withLock { + val keyChainInfo = masternodeKeyChainInfoMap[type] ?: return@withLock + keyChainInfo.masternodeKeyInfoList.forEachIndexed { position, keyInfo -> + if (keyInfo.privateKeyHex == null) { + keyChainInfo.masternodeKeyInfoList[position] = getDecryptedKey(keyInfo.masternodeKey) + } + } + rebuildKeyChainState(type) + } } }Also applies to: 322-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt` around lines 308 - 319, initKeyChainScreen currently launches on Dispatchers.IO and mutates masternodeKeyChainInfoMap[type].masternodeKeyInfoList and calls rebuildKeyChainState from a background coroutine, causing race conditions; protect those mutations by serializing access (e.g., add a shared Mutex or synchronized lock for masternodeKeyChainInfoMap entries) or perform list updates on the main thread: fetch/decrypt with getDecryptedKey on IO, then acquire the Mutex (or switch to Dispatchers.Main) before assigning to masternodeKeyInfoList and calling rebuildKeyChainState; apply the same guarding pattern to the related code block around lines 322-345 to avoid cross-coroutine races.
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt (1)
62-62: Prefer lifecycle-aware flow collection in Compose screens.Use
collectAsStateWithLifecycle()instead ofcollectAsState()for Fragment-hosted UI state collection. Thelifecycle-runtime-composedependency is already available in the wallet module.Suggested fix
+import androidx.lifecycle.compose.collectAsStateWithLifecycle @@ - val uiState by uiStateFlow.collectAsState() + val uiState by uiStateFlow.collectAsStateWithLifecycle()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt` at line 62, The screen collects uiState using uiStateFlow.collectAsState() which is not lifecycle-aware for Fragment-hosted Compose; update the collection to use collectAsStateWithLifecycle() on uiStateFlow (e.g., the val uiState by uiStateFlow.collectAsState() in MasternodeKeyChainScreen) and add the appropriate import for androidx.lifecycle.compose.collectAsStateWithLifecycle so the UI state is lifecycle-aware.wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt (1)
59-77: AddViewCompositionStrategyto properly manage Fragment-hostedComposeViewlifecycle.The
ComposeViewshould dispose its composition when the Fragment view is destroyed to prevent composition state leaks across view recreation cycles.Suggested fix
import androidx.compose.ui.platform.ComposeView +import androidx.compose.ui.platform.ViewCompositionStrategy @@ return ComposeView(requireContext()).apply { + setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) setContent { MasternodeKeyChainScreen(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt` around lines 59 - 77, The ComposeView created in MasternodeKeyChainFragment currently doesn't set a ViewCompositionStrategy, causing compositions to survive Fragment view destruction; update the ComposeView instantiation to call setViewCompositionStrategy with ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed (or equivalent) so the composition is disposed when the Fragment view is destroyed—locate the ComposeView creation in MasternodeKeyChainFragment and add the setViewCompositionStrategy(...) call before setContent to ensure proper lifecycle management.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/res/values/strings-extra.xml`:
- Line 451: The string resource masternode_key_type_platform uses inconsistent
capitalization ("EvoNode operator keys"); update its value to "EvoNode Operator
Keys" to match the Title Case style used by sibling resources (e.g.,
masternode_key_type_owner, masternode_key_type_voting).
---
Duplicate comments:
In `@common/src/main/java/org/dash/wallet/common/ui/components/ListItem.kt`:
- Around line 299-309: The trailingActionText is always styled with
MyTheme.Colors.dashBlue making it look clickable even when onTrailingActionClick
is null; update the Text styling in ListItem.kt so the color is conditional: use
MyTheme.Colors.dashBlue only when onTrailingActionClick != null, otherwise use
the non-action/label color (e.g., MyTheme.Colors.onBackground or the existing
LabelLarge default). Keep the clickable Modifier only when onTrailingActionClick
is present and ensure color and modifier choices are paired to avoid presenting
non-clickable text as a link.
- Around line 282-288: The trailing help icon is hidden from accessibility by
always using contentDescription = null; update the ListItem composable to accept
an optional accessibility string (e.g., trailingHelpIconContentDescription:
String? = null) and pass that into the Icon's contentDescription (replace the
hardcoded null in the Icon call inside ListItem.kt), so screen readers can
announce the icon when provided; ensure callers of ListItem are updated where
appropriate to supply a localized description (or leave null to preserve current
behavior).
- Around line 188-194: The info-icons rendered when showInfoIcon is true
currently set contentDescription = null, which makes them inaccessible; update
both Icon usages in ListItem.kt to provide a meaningful contentDescription
(e.g., use stringResource(R.string.info_icon_description) or accept a new
parameter like infoIconDescription: String?) instead of null, so screen readers
announce the indicator; keep contentDescription = null only if the icon is
purely decorative, otherwise pass the descriptive string to the Icon composable
(or wire a parameter through the ListItem composable) so accessibility is
preserved.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt`:
- Around line 70-74: The onCopy lambda in MasternodeKeyChainFragment currently
logs the raw copied text (log.info("text copied to clipboard: {}", text)), which
may include private key material; remove the sensitive value from logs by
deleting the text argument or replacing the statement with a non-sensitive
message (e.g., log.info("text copied to clipboard") ) and keep calls to
viewModel.copyToClipboard(text) and Toast as-is; also scan for other usages of
log.info/log.debug that reference the copied key to ensure no private key
material is logged.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt`:
- Line 236: Replace the realistic-looking address literal used as a preview in
MasternodeKeyChainScreen (the KeyFieldEntry call with KeyFieldType.ADDRESS) with
a non-sensitive explicit placeholder string (e.g. "ADDRESS_PLACEHOLDER" or
"<address>") or a named constant (e.g. SAMPLE_ADDRESS) to avoid triggering
secret scanners; update the KeyFieldEntry instantiation in the
MasternodeKeyChainScreen.kt preview accordingly and ensure any related
preview/test code uses the same placeholder constant instead of real-looking
addresses.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt`:
- Around line 308-319: initKeyChainScreen currently launches on Dispatchers.IO
and mutates masternodeKeyChainInfoMap[type].masternodeKeyInfoList and calls
rebuildKeyChainState from a background coroutine, causing race conditions;
protect those mutations by serializing access (e.g., add a shared Mutex or
synchronized lock for masternodeKeyChainInfoMap entries) or perform list updates
on the main thread: fetch/decrypt with getDecryptedKey on IO, then acquire the
Mutex (or switch to Dispatchers.Main) before assigning to masternodeKeyInfoList
and calling rebuildKeyChainState; apply the same guarding pattern to the related
code block around lines 322-345 to avoid cross-coroutine races.
---
Nitpick comments:
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.kt`:
- Around line 59-77: The ComposeView created in MasternodeKeyChainFragment
currently doesn't set a ViewCompositionStrategy, causing compositions to survive
Fragment view destruction; update the ComposeView instantiation to call
setViewCompositionStrategy with
ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed (or equivalent) so
the composition is disposed when the Fragment view is destroyed—locate the
ComposeView creation in MasternodeKeyChainFragment and add the
setViewCompositionStrategy(...) call before setContent to ensure proper
lifecycle management.
In
`@wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.kt`:
- Line 62: The screen collects uiState using uiStateFlow.collectAsState() which
is not lifecycle-aware for Fragment-hosted Compose; update the collection to use
collectAsStateWithLifecycle() on uiStateFlow (e.g., the val uiState by
uiStateFlow.collectAsState() in MasternodeKeyChainScreen) and add the
appropriate import for androidx.lifecycle.compose.collectAsStateWithLifecycle so
the UI state is lifecycle-aware.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/src/main/java/org/dash/wallet/common/ui/components/ListItem.ktwallet/res/values/strings-extra.xmlwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainFragment.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeyChainScreen.ktwallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysViewModel.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Refactor
Documentation