Conversation
📝 WalkthroughWalkthroughNormalizes AmountView input to guard non-digit and negative values; adds lifecycle guards in LockScreenActivity; makes SEND/RECEIVE shortcut navigation single-top without popping wallet; introduces flow-backed blockchain identity and updateIdentity(), hooks identity updates into sync and MainViewModel; adds scan close button and handler; removes a platform init call; updates masternode strings. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt (1)
427-451:⚠️ Potential issue | 🟠 MajorTab switching will not work when PaymentsFragment is reused via
launchSingleTop.The
launchSingleTop(true)option correctly prevents back stack overload, but it introduces a functional regression: whenPaymentsFragmentis already on screen and the user taps a different shortcut (e.g., SEND then RECEIVE), theARG_ACTIVE_TABargument will be ignored.
PaymentsFragment.activateTab()(lines 126-136) reads the tab argument only inonViewCreated(), which does not execute when the fragment instance is reused. The fragment has no mechanism to detect and apply argument changes on reuse—there is noonResume()override,savedStateHandleobservation, orcurrentBackStackEntrylistener.Consider one of these approaches:
- Observe
findNavController().currentBackStackEntry?.savedStateHandleinPaymentsFragmentto detect tab changes- Use a shared ViewModel to communicate the desired tab
- Remove
launchSingleTopif tab switching is a priority (accepting some back stack growth)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt` around lines 427 - 451, When reusing PaymentsFragment with launchSingleTop the fragment instance doesn't re-run onViewCreated so ARG_ACTIVE_TAB passed in the navigate bundle is ignored; update PaymentsFragment to observe navigation state and react to tab changes (e.g. in onViewCreated subscribe to findNavController().currentBackStackEntry?.savedStateHandle or the fragment's savedStateHandle for PaymentsFragment.ARG_ACTIVE_TAB) and call PaymentsFragment.activateTab() when that value changes, or alternatively stop using NavOptions.Builder().setLaunchSingleTop(true) from the shortcut handlers if you prefer to force a new fragment instance; reference PaymentsFragment.activateTab(), ARG_ACTIVE_TAB and the findNavController().currentBackStackEntry?.savedStateHandle API to implement the observer and apply the tab switch on reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt`:
- Around line 427-451: When reusing PaymentsFragment with launchSingleTop the
fragment instance doesn't re-run onViewCreated so ARG_ACTIVE_TAB passed in the
navigate bundle is ignored; update PaymentsFragment to observe navigation state
and react to tab changes (e.g. in onViewCreated subscribe to
findNavController().currentBackStackEntry?.savedStateHandle or the fragment's
savedStateHandle for PaymentsFragment.ARG_ACTIVE_TAB) and call
PaymentsFragment.activateTab() when that value changes, or alternatively stop
using NavOptions.Builder().setLaunchSingleTop(true) from the shortcut handlers
if you prefer to force a new fragment instance; reference
PaymentsFragment.activateTab(), ARG_ACTIVE_TAB and the
findNavController().currentBackStackEntry?.savedStateHandle API to implement the
observer and apply the tab switch on reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18f6db1b-4eb6-44e1-bb99-d55892fe97d2
📒 Files selected for processing (3)
common/src/main/java/org/dash/wallet/common/ui/enter_amount/AmountView.ktwallet/src/de/schildbach/wallet/ui/LockScreenActivity.ktwallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
common/src/main/java/org/dash/wallet/common/ui/enter_amount/AmountView.kt (1)
60-65: Silent conversion of empty/invalid input to "0" may mask caller bugs.The defensive check is appropriate for preventing crashes from negative values. However, the condition
value.none { it.isDigit() }will also convert empty strings to "0". As noted inConfirmTransactionDialog.kt(line 165), callers use fallback patterns likegetString(ARG_AMOUNT) ?: ""— this will now silently become "0" rather than propagating as-is, potentially masking cases where a required amount argument is missing.Consider whether logging or explicitly handling the empty-string case separately would aid debugging:
💡 Suggested refinement
var input: String get() = _input set(value) { - _input = if (value.none { it.isDigit() } || value.trimStart().startsWith("-")) "0" else value + _input = when { + value.trimStart().startsWith("-") -> "0" + value.none { it.isDigit() } -> { + if (value.isNotEmpty()) { + Log.w(TAG, "AmountView input has no digits: '$value', defaulting to 0") + } + "0" + } + else -> value + } updateAmount() }🤖 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/enter_amount/AmountView.kt` around lines 60 - 65, The input setter in AmountView (the var input { get/set }) currently replaces any value with no digits (including "") with "0", which can silently mask missing-argument bugs; change the setter to treat empty string separately: if value.isEmpty() then preserve it (set _input = value) or emit a debug/log warning before preserving, while still converting only truly invalid/negative inputs (e.g., value.none { it.isDigit() } && value.isNotEmpty(), or value.trimStart().startsWith("-")) to "0"; keep calling updateAmount() after setting _input so UI updates; refer to the input setter, _input field, and updateAmount() when making the change.
🤖 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/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`:
- Line 109: updateIdentity() mutates the existing _blockchainIdentity instance
in place so blockchainIdentityFlow observers using distinctUntilChanged() don't
see a change; after calling _blockchainIdentity?.updateIdentity() (and any
in-place mutation), reassign the flow with a new object to force emission (e.g.
create a new BlockchainIdentity instance or call a copy/clone and set the
backing MutableStateFlow/_blockchainIdentityFlow.value to that new instance).
Update the code in IdentityRepository around updateIdentity(),
_blockchainIdentity and blockchainIdentityFlow so the flow is set to a new
instance after mutation so MainViewModel (distinctUntilChanged()) will detect
the change and trigger refreshContactsForAllTransactions().
---
Nitpick comments:
In `@common/src/main/java/org/dash/wallet/common/ui/enter_amount/AmountView.kt`:
- Around line 60-65: The input setter in AmountView (the var input { get/set })
currently replaces any value with no digits (including "") with "0", which can
silently mask missing-argument bugs; change the setter to treat empty string
separately: if value.isEmpty() then preserve it (set _input = value) or emit a
debug/log warning before preserving, while still converting only truly
invalid/negative inputs (e.g., value.none { it.isDigit() } &&
value.isNotEmpty(), or value.trimStart().startsWith("-")) to "0"; keep calling
updateAmount() after setting _input so UI updates; refer to the input setter,
_input field, and updateAmount() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b15c442f-4e6b-4e95-825e-947900dc60fd
📒 Files selected for processing (7)
common/src/main/java/org/dash/wallet/common/ui/enter_amount/AmountView.ktwallet/res/layout/scan_activity.xmlwallet/src/de/schildbach/wallet/WalletApplication.javawallet/src/de/schildbach/wallet/service/platform/IdentityRepository.ktwallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/scan/ScanActivity.java
💤 Files with no reviewable changes (1)
- wallet/src/de/schildbach/wallet/WalletApplication.java
| suspend fun shouldShowAlert(): Boolean | ||
| fun getNextContactAddress(userId: String, accountReference: Int): Address? | ||
| suspend fun clearDatabase(includeInvitations: Boolean) | ||
| fun updateIdentity() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that updateIdentity() mutates identity without publishing a guaranteed new flow emission.
# Expected finding:
# - updateIdentity() does not re-publish a semantically new flow value
# - MainViewModel relies on blockchainIdentityFlow emissions for refresh
rg -n -C3 --type=kt 'override fun updateIdentity\(\)|_blockchainIdentityFlow\.value\s*=' wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt
rg -n -C3 --type=kt 'blockchainIdentityFlow|distinctUntilChanged\(\)' wallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktRepository: dashpay/dash-wallet
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find and read the IdentityRepository.kt file
fd "IdentityRepository.kt" wallet/srcRepository: dashpay/dash-wallet
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Read the IdentityRepository.kt file to understand updateIdentity() and blockchainIdentityFlow
cat -n wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.ktRepository: dashpay/dash-wallet
Length of output: 48829
🏁 Script executed:
#!/bin/bash
# Search for updateIdentity and blockchainIdentityFlow in IdentityRepository
rg -n -C5 'updateIdentity|blockchainIdentityFlow' wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.ktRepository: dashpay/dash-wallet
Length of output: 3668
🏁 Script executed:
#!/bin/bash
# Check MainViewModel.kt for blockchainIdentityFlow and distinctUntilChanged usage
rg -n -C5 'blockchainIdentityFlow|distinctUntilChanged' wallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktRepository: dashpay/dash-wallet
Length of output: 3419
updateIdentity() mutates identity without notifying blockchainIdentityFlow observers.
At lines 827-829, updateIdentity() calls _blockchainIdentity?.updateIdentity(), which mutates the existing BlockchainIdentity instance in place. Because the flow reference remains unchanged, MainViewModel's subscription (line 339-343) using distinctUntilChanged() will not detect the update, causing refreshContactsForAllTransactions() to be skipped.
Force a new emission by reassigning the flow value:
Fix pattern
override fun updateIdentity() {
- _blockchainIdentity?.updateIdentity()
+ _blockchainIdentity?.let { identity ->
+ identity.updateIdentity()
+ _blockchainIdentityFlow.value = identity
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt` at
line 109, updateIdentity() mutates the existing _blockchainIdentity instance in
place so blockchainIdentityFlow observers using distinctUntilChanged() don't see
a change; after calling _blockchainIdentity?.updateIdentity() (and any in-place
mutation), reassign the flow with a new object to force emission (e.g. create a
new BlockchainIdentity instance or call a copy/clone and set the backing
MutableStateFlow/_blockchainIdentityFlow.value to that new instance). Update the
code in IdentityRepository around updateIdentity(), _blockchainIdentity and
blockchainIdentityFlow so the flow is set to a new instance after mutation so
MainViewModel (distinctUntilChanged()) will detect the change and trigger
refreshContactsForAllTransactions().
1 participant
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features
Localization
Improvements