Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a cache-first, paging-backed transaction display pipeline with new Room entities/DAOs and migration (16→17), in-memory prebuilt paging and adapter-swap to live PagingData, extensive TxDisplayCacheService, schema snapshots (17–21), UI/layout/strings updates, a performance test, small util/model tweaks, and a build version bump. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as WalletTransactionsFragment
participant VM as MainViewModel
participant CacheSvc as TxDisplayCacheService
participant CacheDAO as TxDisplayCacheDao
participant GroupDAO as TxGroupCacheDao
participant Paging as Room/Paging
participant Wallet as Wallet Loader
UI->>VM: onViewCreated()
VM->>CacheSvc: request cachedRows()
CacheSvc->>CacheDAO: load cached TxDisplayCacheEntry list
CacheDAO-->>CacheSvc: cached entries
CacheSvc-->>VM: emit cachedRows StateFlow
VM-->>UI: submit cachedRows to CacheTransactionAdapter (immediate render)
VM->>CacheSvc: subscribe to transactions Flow
CacheSvc->>Wallet: observe wallet/metadata/contact flows (background)
Wallet-->>CacheSvc: live transaction updates
CacheSvc->>Paging: build Pager (Room) -> PagingData<HistoryRowView>
Paging-->>CacheSvc: PagingData stream
CacheSvc-->>VM: emit transactions Flow
VM-->>UI: submitData to TransactionAdapter -> swap to live adapter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
docs/proposals/transaction-display-performance.md (2)
23-56: Add a language identifier to the fenced code block.The code block starting at line 23 lacks a language specifier, which prevents syntax highlighting and can trigger linting warnings.
📝 Suggested fix
-``` +```text Data Sources ├── walletData.observeTransactions() → List<Transaction> (filtered, batched 500ms)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/transaction-display-performance.md` around lines 23 - 56, The fenced code block containing the architecture diagram (starting with "Data Sources" and including symbols like walletData.observeTransactions(), refreshTransactions(), refreshTransactionBatch(), updateContactsAndMetadata(), and StateFlow<Map<LocalDate, List<TransactionRowView>>> ) needs a language identifier to satisfy linting and enable highlighting; update the opening fence from ``` to ```text (or another appropriate language) so the block becomes ```text and leave the rest unchanged.
327-327: Normalize the horizontal rule style.The horizontal rule on this line has trailing spaces which is inconsistent with others in the document (e.g., line 7). Consider using a consistent
---format throughout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/transaction-display-performance.md` at line 327, The horizontal rule on the indicated line contains trailing spaces; normalize it to match the other rules by replacing the current line with a clean three-dash rule ("---") with no trailing whitespace so all horizontal rules in transaction-display-performance.md use the same `---` format.wallet/build.gradle (1)
124-125: Consider extracting the paging version to an ext variable for consistency.Other dependencies in this project use version variables defined in the root
build.gradle(e.g.,$roomVersion,$lifecycleVersion). Hardcoding3.1.1here could lead to version drift if paging is used elsewhere or updated later.♻️ Suggested improvement
In root
build.gradle, add:pagingVersion = '3.1.1'Then update this line:
- implementation "androidx.paging:paging-runtime-ktx:3.1.1" + implementation "androidx.paging:paging-runtime-ktx:$pagingVersion"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/build.gradle` around lines 124 - 125, Extract the hardcoded paging version by adding a pagingVersion property in the root build.gradle (matching how roomVersion and lifecycleVersion are defined), then replace the literal version in the module's dependency declaration (implementation "androidx.paging:paging-runtime-ktx:3.1.1") with the variable reference (e.g., using pagingVersion) so the dependency becomes consistent with other version-managed libs; update any documentation or README if you track dependency variables centrally.wallet/src/de/schildbach/wallet/WalletApplication.java (1)
488-539: Consider usingStopwatchfor timing consistency.The file already uses
Stopwatch(from Guava) for timing inloadWalletFromProtobuf()andprotobufSerializeWallet(). Using the same pattern here would be more consistent and readable than manualSystem.currentTimeMillis()arithmetic with the inline reassignment pattern.♻️ Suggested approach
public void finalizeInitialization() { - long _t = System.currentTimeMillis(); + Stopwatch watch = Stopwatch.createStarted(); // ... security guard code ... - log.info("STARTUP finalizeInit: securityGuard done in {}ms", System.currentTimeMillis() - _t); _t = System.currentTimeMillis(); + log.info("STARTUP finalizeInit: securityGuard done in {}", watch); watch.reset().start(); dashSystemService.getSystem().initDash(true, true, Constants.SYNC_FLAGS, Constants.VERIFY_FLAGS); - log.info("STARTUP finalizeInit: initDash done in {}ms", System.currentTimeMillis() - _t); _t = System.currentTimeMillis(); + log.info("STARTUP finalizeInit: initDash done in {}", watch); watch.reset().start(); // ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/WalletApplication.java` around lines 488 - 539, Replace the manual System.currentTimeMillis() timing pattern in WalletApplication (the long _t variable and its reassignment) with a Guava Stopwatch for consistency: create a Stopwatch (Stopwatch.createStarted()) before the securityGuard block, use stopwatch.elapsed(TimeUnit.MILLISECONDS) in the log statements after the securityGuard ensureMnemonicFallbacks/ensurePinFallback sequence, after dashSystemService.getSystem().initDash(...), after afterLoadWallet(), after initPlatform()/createNotificationChannels(), and after the integrations initUphold(), initCoinbase(), initDashSpend(); between sections call stopwatch.reset(); stopwatch.start() to restart timing. Add the necessary import for com.google.common.base.Stopwatch and java.util.concurrent.TimeUnit and reference the existing methods (ensureMnemonicFallbacks, ensurePinFallback, initDash, afterLoadWallet, cleanupFiles, createNotificationChannels, initPlatform, initUphold, initCoinbase, initDashSpend) when replacing the corresponding timing/log lines.wallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.kt (3)
162-164: Remove unusederrorfunction.The
errorhelper function is defined but never called in this test class.🧹 Proposed cleanup
- fun error(format: String, vararg args: Any?) { - log("ERROR", format, *args) - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.kt` around lines 162 - 164, The helper function error(format: String, vararg args: Any?) in WalletLoadPerformanceTest.kt is unused and should be removed to clean up dead code; locate the function named error and delete its declaration (and any associated unused overloads like the log("ERROR", ...) call path) so no unused helper remains in the test class.
44-48: Remove unused constants.
MNEMONICandEXPECTED_SAVE_TIME_MSare declared but never used in this test class.🧹 Proposed cleanup
companion object { private val log = LoggerFactory.getLogger(WalletLoadPerformanceTest::class.java) - private const val MNEMONIC = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about" - private const val EXPECTED_SAVE_TIME_MS = 5000L // Expected maximum save time }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.kt` around lines 44 - 48, Remove the unused constants MNEMONIC and EXPECTED_SAVE_TIME_MS from the companion object in WalletLoadPerformanceTest; locate the companion object containing LoggerFactory.getLogger(WalletLoadPerformanceTest::class.java) and delete the declarations for MNEMONIC and EXPECTED_SAVE_TIME_MS so the class no longer contains unused fields.
199-206: Use explicit locale for String.format.Using implicit default locale can produce inconsistent output across devices/regions. For test logging, this is minor but worth fixing for consistency.
🔧 Proposed fix
+import java.util.Locale + // Inside formatMessage function: - replacement = if (decimals == 1) { - String.format("%.1f", value) - } else if (decimals == 2) { - String.format("%.2f", value) - } else { - String.format("%.2f", value) // default - } + replacement = if (decimals == 1) { + String.format(Locale.US, "%.1f", value) + } else if (decimals == 2) { + String.format(Locale.US, "%.2f", value) + } else { + String.format(Locale.US, "%.2f", value) // default + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.kt` around lines 199 - 206, The String.format calls in WalletLoadPerformanceTest.kt use the implicit default Locale which can vary across devices; update the formatting to use an explicit Locale (e.g., Locale.US or Locale.ROOT) for the three String.format invocations that produce replacement (the branches when decimals == 1, decimals == 2, and the default) so that String.format is called with a Locale parameter and the same format string, ensuring consistent numeric output for the replacement variable computed from arg.toDouble().wallet/src/de/schildbach/wallet/ui/main/TransactionAdapter.kt (1)
274-280: Avoid creating newHistoryRowDiffCallbackinstances on each comparison.The
DiffCallbackinner class instantiates a newHistoryRowDiffCallbackfor everyareItemsTheSameandareContentsTheSamecall. This creates unnecessary object allocations during list diffing.♻️ Proposed fix
+ // Keep for backwards compatibility (TransactionGroupDetailsFragment uses it) class DiffCallback : DiffUtil.ItemCallback<HistoryRowView>() { - override fun areItemsTheSame(oldItem: HistoryRowView, newItem: HistoryRowView) = - HistoryRowDiffCallback().areItemsTheSame(oldItem, newItem) - override fun areContentsTheSame(oldItem: HistoryRowView, newItem: HistoryRowView) = - HistoryRowDiffCallback().areContentsTheSame(oldItem, newItem) + private val delegate = HistoryRowDiffCallback() + override fun areItemsTheSame(oldItem: HistoryRowView, newItem: HistoryRowView) = + delegate.areItemsTheSame(oldItem, newItem) + override fun areContentsTheSame(oldItem: HistoryRowView, newItem: HistoryRowView) = + delegate.areContentsTheSame(oldItem, newItem) }🤖 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/TransactionAdapter.kt` around lines 274 - 280, The DiffCallback inner class currently creates a new HistoryRowDiffCallback on every call to areItemsTheSame and areContentsTheSame; change DiffCallback to reuse a single HistoryRowDiffCallback instance (e.g., private val historyDiff = HistoryRowDiffCallback()) and delegate both methods to that instance instead of constructing a new one each time, updating the implementation of areItemsTheSame and areContentsTheSame to call historyDiff.areItemsTheSame(...) and historyDiff.areContentsTheSame(...).wallet/src/de/schildbach/wallet/ui/main/MainActivity.kt (1)
197-203: Update comment to reflect StateFlow usage.The code now observes
isAbleToCreateIdentity(aStateFlow), but the comment still references "LiveData". Consider updating the comment for accuracy.📝 Proposed fix
fun initViewModel() { viewModel.isAbleToCreateIdentity.observe(this) { // empty observer just to trigger data loading // viewModel is shared with some fragments keeping the observer active - // inside the parent Activity will avoid recreation of relatively complex - // isAbleToCreateIdentityData LiveData + // inside the parent Activity will avoid recreation of relatively complex + // isAbleToCreateIdentity StateFlow }🤖 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/MainActivity.kt` around lines 197 - 203, Update the outdated comment in initViewModel to reference StateFlow instead of LiveData: clarify that viewModel.isAbleToCreateIdentity is a StateFlow and that attaching an empty observer in the Activity keeps the shared ViewModel's StateFlow active across fragments to avoid recreation of the relatively complex isAbleToCreateIdentity data; mention that the observer is intentionally empty and is used to trigger data loading and retain the StateFlow subscription.wallet/src/de/schildbach/wallet/database/AppDatabase.kt (1)
47-67: Drop the unreachable 20/21 schema exports or bump the DB version consistently.
AppDatabasestill declares version 19, but this PR also addswallet/schemas/de.schildbach.wallet.database.AppDatabase/20.jsonand21.json. Those snapshots already diverge from the 19 schema (tx_group_cacheandfilterFlagsdisappear again), so keeping them makes future Room migration baselines misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/database/AppDatabase.kt` around lines 47 - 67, AppDatabase currently declares version = 19 while new schema snapshot files wallet/schemas/de.schildbach.wallet.database.AppDatabase/20.json and 21.json exist and diverge (e.g., tx_group_cache and filterFlags differences); either delete the unreachable 20.json and 21.json schema exports or bump AppDatabase.version to match the latest snapshot (e.g., 21) and ensure the entities list in AppDatabase (ExchangeRate, BlockchainState, TransactionMetadata, AddressMetadata, IconBitmap, GiftCard, DashPayProfile, DashPayContactRequest, UserAlert, Invitation, TransactionMetadataCacheItem, TransactionMetadataDocument, UsernameRequest, UsernameVote, ImportedMasternodeKey, TopUp, TxDisplayCacheEntry, TxGroupCacheEntry) and the DB schema reflect the fields present in the 20/21 snapshots (restore or remove tx_group_cache and filterFlags consistently) so Room migration baselines remain accurate.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (3)
1043-1049:tryIncludealways returnstruebut the interface semantics suggest conditional inclusion.The
TransactionWrapper.tryInclude(t: Transaction)method typically returns whether the transaction was successfully included in the wrapper. For single-tx wrappers, returningtrueunconditionally is semantically misleading and could cause issues if callers rely on the return value.Consider returning
falseor throwing since single-tx wrappers shouldn't accept additional transactions.♻️ Suggested fix
private fun createSingleTxWrapper(tx: Transaction): TransactionWrapper = object : TransactionWrapper { override val id = tx.txId.toStringBase58() override val transactions = hashMapOf(tx.txId to tx) override val groupDate = tx.updateTime.toInstant().atZone(ZoneId.systemDefault()).toLocalDate() - override fun tryInclude(t: Transaction) = true + override fun tryInclude(t: Transaction) = false // Single-tx wrappers don't accept additional txs override fun getValue(bag: TransactionBag) = tx.getValue(bag) }🤖 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/MainViewModel.kt` around lines 1043 - 1049, The createSingleTxWrapper implementation currently has TransactionWrapper.tryInclude(t: Transaction) always returning true; change it so single-tx wrappers do not claim to accept additional transactions—have tryInclude return false (and do not modify transactions or state) for the single-entry wrapper created by createSingleTxWrapper, so callers relying on the boolean will correctly see that inclusion failed; update the anonymous TransactionWrapper in createSingleTxWrapper (the tryInclude override) accordingly.
170-205: ViewModel uses multiple separate flows instead of a singleUIStatedata class.Per coding guidelines, ViewModels should consolidate state into a single
UIStatedata class with a private mutable_uiStateand public immutableuiStateviaasStateFlow(). Currently, transaction state is spread across_txDataSource,_transactionsLoaded,cachedRows, andtransactions.This is a significant refactor, so consider addressing it in a follow-up PR to reduce the scope of this change.
As per coding guidelines: "ViewModels should use a single
UIStatedata class rather than multiple separate flows" and "UseStateFlow(notLiveData) for asynchronously updated fields in ViewModels".🤖 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/MainViewModel.kt` around lines 170 - 205, State is currently split across _txDataSource, _transactionsLoaded, cachedRows and transactions; consolidate these into a single UIState data class and expose it as an immutable StateFlow (private MutableStateFlow _uiState, public uiState = _uiState.asStateFlow()) so the ViewModel follows the guideline of one StateFlow for UI state. Create UIState fields for txDataSource (or a simpler enum), transactionsLoaded, cachedRows and any other visible transaction-related properties; update places currently observing cachedRows/_txDataSource/_transactionsLoaded to read from uiState instead and update _uiState atomically when any of those underlying values change (e.g., in rebuildWrappedList and relevant setters). Make this a contained follow-up refactor so behavior is preserved while moving the four separate flows into the single _uiState/ uiState pattern.
248-258: Mix ofLiveDataandStateFlowfor async state.
_isBlockchainSynced,_blockchainSyncPercentage,_exchangeRate, etc. useMutableLiveData, while_transactionsLoaded,_rateStale,_txDataSourceuseMutableStateFlow. Per coding guidelines, preferStateFlowfor asynchronously updated fields.This inconsistency may be inherited from existing code, but consider migrating to
StateFlowuniformly in a follow-up.As per coding guidelines: "Use
StateFlow(notLiveData) for asynchronously updated fields in ViewModels".Also applies to: 266-268
🤖 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/MainViewModel.kt` around lines 248 - 258, The ViewModel mixes LiveData and StateFlow for async state; migrate the async LiveData fields to StateFlow to match the rest of the class: replace _isBlockchainSynced / isBlockchainSynced, _isBlockchainSyncFailed / isBlockchainSyncFailed and _blockchainSyncPercentage / blockchainSyncPercentage (and the similar fields referenced at 266-268) to use MutableStateFlow<T>(initialValue) for the backing property and expose them as StateFlow<T> (e.g., StateFlow<Boolean> or StateFlow<Int>) so callers can collect/observe via Kotlin Flow APIs; update any callers (UI code) to use collect/collectAsState instead of LiveData observation. Ensure you preserve types and provide sensible initial values (false/0) when creating the MutableStateFlow instances.wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
291-293:distinctUntilChanged()onloadStateFlowmay not behave as expected.
CombinedLoadStatescontains multiple nestedLoadStateobjects. UsingdistinctUntilChanged()compares the entire object by equality, which may still emit frequently due to internal state changes (e.g.,endOfPaginationReachedtoggling). This won't cause bugs but may result in more emissions than intended.Consider removing it or using a more targeted comparison if you observe excessive recomposition.
🤖 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/WalletTransactionsFragment.kt` around lines 291 - 293, The current chain in WalletTransactionsFragment using liveAdapter.loadStateFlow.distinctUntilChanged().collectLatest { ... } is comparing entire CombinedLoadStates (which contains nested LoadState fields like endOfPaginationReached) leading to extra emissions; change this to either remove distinctUntilChanged() or map to the specific fields you care about (e.g., map { it.refresh to it.append to it.prepend or a boolean like it.append.endOfPaginationReached }) and then apply distinctUntilChanged() on that mapped value before collectLatest, so the collector only reacts to the targeted state changes.wallet/src/de/schildbach/wallet/database/dao/TxDisplayCacheDao.kt (1)
49-51: Consider SQLite'sINclause limit for large transaction histories.Room/SQLite has a default limit of ~999 parameters for
INclauses. IfrowIdsexceeds this limit (e.g., wallets with thousands of transactions), the query will fail. This is unlikely for targeted metadata updates but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/database/dao/TxDisplayCacheDao.kt` around lines 49 - 51, The current DAO method getEntriesByIds in TxDisplayCacheDao risks exceeding SQLite's ~999 parameter limit for large rowIds lists; fix by adding a batched wrapper (e.g., getEntriesByIdsBatched) in the DAO or repository that splits the incoming rowIds into chunks under the safe limit (use 900 params per chunk), calls the existing getEntriesByIds for each chunk inside a single suspend/transactional flow, aggregates/merges the results, and returns the combined List<TxDisplayCacheEntry>; reference getEntriesByIds and TxDisplayCacheDao when locating where to add this batching logic.
🤖 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/schemas/de.schildbach.wallet.database.AppDatabase/18.json`:
- Around line 991-1145: The tx_display_cache table definition is missing the
filterFlags column used by TxDisplayCacheDao.pagingSource(); update the
tx_display_cache "createSql" and its "fields" array to add a filterFlags INTEGER
(notNull false or as appropriate) and include it in the schema/primary key block
as needed so the bitwise WHERE (:filterFlag = 0 OR (filterFlags & :filterFlag)
!= 0) can run; additionally add an index entry for tx_display_cache on time DESC
to support pagingSource() and getAll(), and add an index for tx_group_cache on
(groupDate DESC, sortOrder ASC) to support getAll() and getActiveGroups() by
updating the "indices" arrays for those table objects.
In `@wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt`:
- Around line 127-153: migration16to17 is wrongly declared as Migration(16, 21)
which incorrectly spans beyond the intended 16→17 change; update its version
tuple to Migration(16, 17) so Room will apply this migration during the 16→19
upgrade path. Locate the object named migration16to17 and replace the
Migration(16, 21) constructor call with Migration(16, 17) without changing the
migration body that creates tx_display_cache.
In `@wallet/src/de/schildbach/wallet/database/entity/TxDisplayCacheEntry.kt`:
- Around line 170-177: The reconstructed DashPayProfile in
TxDisplayCacheEntry.kt incorrectly sets userId = rowId (transaction hex/group
id); change reconstruction in resolvedContact so it uses a cached contactUserId
instead of rowId—add a new cached field (e.g., contactUserId) to
TxDisplayCacheEntry, persist it when storing contact info, and use that value
when creating DashPayProfile(userId = contactUserId); alternatively, if you
don't want to persist IDs, set the reconstructed profile to a display-only form
(e.g., userId = null/empty) and document that DashPayProfile from
contactUsername/contactDisplayName/contactAvatarUrl must not be used for
identity lookups.
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 397-412: Within the _transactionsDirection.flatMapLatest block the
suspend call txDisplayCacheDao.getCount() is being invoked on the upstream flow
context (likely main/UI) which can block the UI; change the logic so that any
database calls (txDisplayCacheDao.getCount() and any other DAO/suspend ops used
in this branch) are executed on Dispatchers.IO (e.g. wrap them with
withContext(Dispatchers.IO) or move to a coroutine launched on IO) before
mutating state; ensure you still set _txDataSource.value =
TxDataSource.RoomLive(...), _transactionsLoaded.value = true, and call
initializeFactoriesFromCache() or rebuildWrappedList(filter) from the collected
flow after the IO call completes so behavior remains identical.
- Around line 499-510: The nested viewModelScope.launch(Dispatchers.IO) creates
a detached coroutine so cache deletions may run concurrently with the wrapper
resets and paging invalidation; replace the nested launch with an awaited IO
context (e.g. withContext(Dispatchers.IO) { txDisplayCacheDao.deleteAll();
txGroupCacheDao.deleteAll() }) or capture and join the Job, so deletions
complete before resetting coinJoinWrapperFactory/crowdNodeWrapperFactory and
calling currentPagingSource?.invalidate(); reference the existing
viewModelScope.launch(Dispatchers.IO), txDisplayCacheDao.deleteAll(),
txGroupCacheDao.deleteAll(), coinJoinWrapperFactory, crowdNodeWrapperFactory,
and currentPagingSource?.invalidate() when making the change.
In `@wallet/src/de/schildbach/wallet/ui/main/PrebuiltRowsPagingSource.kt`:
- Around line 50-53: The prevKey currently returns the current offset which
causes prepend to reload the same page; update the prevKey logic in the
LoadResult.Page creation (where LoadResult.Page is constructed using data =
slice, prevKey = ..., nextKey = ...) so that prevKey points to the previous page
start, e.g. null when offset == 0, otherwise max(0, offset - params.loadSize)
(use params.loadSize as the page size) so the paging library will request the
correct preceding offset instead of reloading the current page.
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 124-133: The coroutine may call requireActivity() after the
fragment is detached causing IllegalStateException; instead, after obtaining the
wrapper from viewModel.loadGroupWrapper, guard against a destroyed/detached
fragment by checking fragment state or the nullable activity and only show the
details if safe (e.g., use activity?.let { ... } or check isAdded()/isDetached()
before calling TransactionGroupDetailsFragment(wrapper).show(...)); keep the
null-return comment behavior when the fragment is not in a valid state and
ensure viewModel.logEvent(AnalyticsConstants.Home.TRANSACTION_DETAILS) is only
called when you actually proceed to show the fragment.
---
Nitpick comments:
In `@docs/proposals/transaction-display-performance.md`:
- Around line 23-56: The fenced code block containing the architecture diagram
(starting with "Data Sources" and including symbols like
walletData.observeTransactions(), refreshTransactions(),
refreshTransactionBatch(), updateContactsAndMetadata(), and
StateFlow<Map<LocalDate, List<TransactionRowView>>> ) needs a language
identifier to satisfy linting and enable highlighting; update the opening fence
from ``` to ```text (or another appropriate language) so the block becomes
```text and leave the rest unchanged.
- Line 327: The horizontal rule on the indicated line contains trailing spaces;
normalize it to match the other rules by replacing the current line with a clean
three-dash rule ("---") with no trailing whitespace so all horizontal rules in
transaction-display-performance.md use the same `---` format.
In
`@wallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.kt`:
- Around line 162-164: The helper function error(format: String, vararg args:
Any?) in WalletLoadPerformanceTest.kt is unused and should be removed to clean
up dead code; locate the function named error and delete its declaration (and
any associated unused overloads like the log("ERROR", ...) call path) so no
unused helper remains in the test class.
- Around line 44-48: Remove the unused constants MNEMONIC and
EXPECTED_SAVE_TIME_MS from the companion object in WalletLoadPerformanceTest;
locate the companion object containing
LoggerFactory.getLogger(WalletLoadPerformanceTest::class.java) and delete the
declarations for MNEMONIC and EXPECTED_SAVE_TIME_MS so the class no longer
contains unused fields.
- Around line 199-206: The String.format calls in WalletLoadPerformanceTest.kt
use the implicit default Locale which can vary across devices; update the
formatting to use an explicit Locale (e.g., Locale.US or Locale.ROOT) for the
three String.format invocations that produce replacement (the branches when
decimals == 1, decimals == 2, and the default) so that String.format is called
with a Locale parameter and the same format string, ensuring consistent numeric
output for the replacement variable computed from arg.toDouble().
In `@wallet/build.gradle`:
- Around line 124-125: Extract the hardcoded paging version by adding a
pagingVersion property in the root build.gradle (matching how roomVersion and
lifecycleVersion are defined), then replace the literal version in the module's
dependency declaration (implementation
"androidx.paging:paging-runtime-ktx:3.1.1") with the variable reference (e.g.,
using pagingVersion) so the dependency becomes consistent with other
version-managed libs; update any documentation or README if you track dependency
variables centrally.
In `@wallet/src/de/schildbach/wallet/database/AppDatabase.kt`:
- Around line 47-67: AppDatabase currently declares version = 19 while new
schema snapshot files
wallet/schemas/de.schildbach.wallet.database.AppDatabase/20.json and 21.json
exist and diverge (e.g., tx_group_cache and filterFlags differences); either
delete the unreachable 20.json and 21.json schema exports or bump
AppDatabase.version to match the latest snapshot (e.g., 21) and ensure the
entities list in AppDatabase (ExchangeRate, BlockchainState,
TransactionMetadata, AddressMetadata, IconBitmap, GiftCard, DashPayProfile,
DashPayContactRequest, UserAlert, Invitation, TransactionMetadataCacheItem,
TransactionMetadataDocument, UsernameRequest, UsernameVote,
ImportedMasternodeKey, TopUp, TxDisplayCacheEntry, TxGroupCacheEntry) and the DB
schema reflect the fields present in the 20/21 snapshots (restore or remove
tx_group_cache and filterFlags consistently) so Room migration baselines remain
accurate.
In `@wallet/src/de/schildbach/wallet/database/dao/TxDisplayCacheDao.kt`:
- Around line 49-51: The current DAO method getEntriesByIds in TxDisplayCacheDao
risks exceeding SQLite's ~999 parameter limit for large rowIds lists; fix by
adding a batched wrapper (e.g., getEntriesByIdsBatched) in the DAO or repository
that splits the incoming rowIds into chunks under the safe limit (use 900 params
per chunk), calls the existing getEntriesByIds for each chunk inside a single
suspend/transactional flow, aggregates/merges the results, and returns the
combined List<TxDisplayCacheEntry>; reference getEntriesByIds and
TxDisplayCacheDao when locating where to add this batching logic.
In `@wallet/src/de/schildbach/wallet/ui/main/MainActivity.kt`:
- Around line 197-203: Update the outdated comment in initViewModel to reference
StateFlow instead of LiveData: clarify that viewModel.isAbleToCreateIdentity is
a StateFlow and that attaching an empty observer in the Activity keeps the
shared ViewModel's StateFlow active across fragments to avoid recreation of the
relatively complex isAbleToCreateIdentity data; mention that the observer is
intentionally empty and is used to trigger data loading and retain the StateFlow
subscription.
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 1043-1049: The createSingleTxWrapper implementation currently has
TransactionWrapper.tryInclude(t: Transaction) always returning true; change it
so single-tx wrappers do not claim to accept additional transactions—have
tryInclude return false (and do not modify transactions or state) for the
single-entry wrapper created by createSingleTxWrapper, so callers relying on the
boolean will correctly see that inclusion failed; update the anonymous
TransactionWrapper in createSingleTxWrapper (the tryInclude override)
accordingly.
- Around line 170-205: State is currently split across _txDataSource,
_transactionsLoaded, cachedRows and transactions; consolidate these into a
single UIState data class and expose it as an immutable StateFlow (private
MutableStateFlow _uiState, public uiState = _uiState.asStateFlow()) so the
ViewModel follows the guideline of one StateFlow for UI state. Create UIState
fields for txDataSource (or a simpler enum), transactionsLoaded, cachedRows and
any other visible transaction-related properties; update places currently
observing cachedRows/_txDataSource/_transactionsLoaded to read from uiState
instead and update _uiState atomically when any of those underlying values
change (e.g., in rebuildWrappedList and relevant setters). Make this a contained
follow-up refactor so behavior is preserved while moving the four separate flows
into the single _uiState/ uiState pattern.
- Around line 248-258: The ViewModel mixes LiveData and StateFlow for async
state; migrate the async LiveData fields to StateFlow to match the rest of the
class: replace _isBlockchainSynced / isBlockchainSynced, _isBlockchainSyncFailed
/ isBlockchainSyncFailed and _blockchainSyncPercentage /
blockchainSyncPercentage (and the similar fields referenced at 266-268) to use
MutableStateFlow<T>(initialValue) for the backing property and expose them as
StateFlow<T> (e.g., StateFlow<Boolean> or StateFlow<Int>) so callers can
collect/observe via Kotlin Flow APIs; update any callers (UI code) to use
collect/collectAsState instead of LiveData observation. Ensure you preserve
types and provide sensible initial values (false/0) when creating the
MutableStateFlow instances.
In `@wallet/src/de/schildbach/wallet/ui/main/TransactionAdapter.kt`:
- Around line 274-280: The DiffCallback inner class currently creates a new
HistoryRowDiffCallback on every call to areItemsTheSame and areContentsTheSame;
change DiffCallback to reuse a single HistoryRowDiffCallback instance (e.g.,
private val historyDiff = HistoryRowDiffCallback()) and delegate both methods to
that instance instead of constructing a new one each time, updating the
implementation of areItemsTheSame and areContentsTheSame to call
historyDiff.areItemsTheSame(...) and historyDiff.areContentsTheSame(...).
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 291-293: The current chain in WalletTransactionsFragment using
liveAdapter.loadStateFlow.distinctUntilChanged().collectLatest { ... } is
comparing entire CombinedLoadStates (which contains nested LoadState fields like
endOfPaginationReached) leading to extra emissions; change this to either remove
distinctUntilChanged() or map to the specific fields you care about (e.g., map {
it.refresh to it.append to it.prepend or a boolean like
it.append.endOfPaginationReached }) and then apply distinctUntilChanged() on
that mapped value before collectLatest, so the collector only reacts to the
targeted state changes.
In `@wallet/src/de/schildbach/wallet/WalletApplication.java`:
- Around line 488-539: Replace the manual System.currentTimeMillis() timing
pattern in WalletApplication (the long _t variable and its reassignment) with a
Guava Stopwatch for consistency: create a Stopwatch (Stopwatch.createStarted())
before the securityGuard block, use stopwatch.elapsed(TimeUnit.MILLISECONDS) in
the log statements after the securityGuard
ensureMnemonicFallbacks/ensurePinFallback sequence, after
dashSystemService.getSystem().initDash(...), after afterLoadWallet(), after
initPlatform()/createNotificationChannels(), and after the integrations
initUphold(), initCoinbase(), initDashSpend(); between sections call
stopwatch.reset(); stopwatch.start() to restart timing. Add the necessary import
for com.google.common.base.Stopwatch and java.util.concurrent.TimeUnit and
reference the existing methods (ensureMnemonicFallbacks, ensurePinFallback,
initDash, afterLoadWallet, cleanupFiles, createNotificationChannels,
initPlatform, initUphold, initCoinbase, initDashSpend) when replacing the
corresponding timing/log lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f59eace6-a71a-4bde-b889-a56ef7700e5e
📒 Files selected for processing (33)
build.gradlecommon/src/main/java/org/dash/wallet/common/services/RateRetrievalState.ktcommon/src/main/java/org/dash/wallet/common/util/ResourceString.ktdocs/proposals/async-wallet-loading.mddocs/proposals/transaction-display-performance.mdwallet/androidTest/de/schildbach/wallet/performance/WalletLoadPerformanceTest.ktwallet/build.gradlewallet/res/values/strings-extra.xmlwallet/schemas/de.schildbach.wallet.database.AppDatabase/17.jsonwallet/schemas/de.schildbach.wallet.database.AppDatabase/18.jsonwallet/schemas/de.schildbach.wallet.database.AppDatabase/19.jsonwallet/schemas/de.schildbach.wallet.database.AppDatabase/20.jsonwallet/schemas/de.schildbach.wallet.database.AppDatabase/21.jsonwallet/src/de/schildbach/wallet/WalletApplication.javawallet/src/de/schildbach/wallet/database/AppDatabase.ktwallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.ktwallet/src/de/schildbach/wallet/database/dao/TxDisplayCacheDao.ktwallet/src/de/schildbach/wallet/database/dao/TxGroupCacheDao.ktwallet/src/de/schildbach/wallet/database/entity/TxDisplayCacheEntry.ktwallet/src/de/schildbach/wallet/database/entity/TxGroupCacheEntry.ktwallet/src/de/schildbach/wallet/di/DatabaseModule.ktwallet/src/de/schildbach/wallet/ui/dashpay/UpgradeToEvolutionFragment.ktwallet/src/de/schildbach/wallet/ui/dashpay/UserAlert.ktwallet/src/de/schildbach/wallet/ui/main/MainActivity.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/main/PrebuiltRowsPagingSource.ktwallet/src/de/schildbach/wallet/ui/main/TransactionAdapter.ktwallet/src/de/schildbach/wallet/ui/main/WalletFragment.ktwallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.ktwallet/src/de/schildbach/wallet/ui/more/MoreFragment.ktwallet/src/de/schildbach/wallet/ui/transactions/TransactionGroupDetailsFragment.ktwallet/src/de/schildbach/wallet/ui/transactions/TransactionRowView.ktwallet/test/de/schildbach/wallet/ui/more/tools/TransactionExportTest.kt
wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/database/entity/TxDisplayCacheEntry.kt
Outdated
Show resolved
Hide resolved
HashEngineering
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/transactions/WalletBalanceObserver.kt (1)
70-80:⚠️ Potential issue | 🟠 MajorWallet reset handling is incomplete for
observe()subscribersLine 80 adds reset handling for
_totalBalance/_mixedBalance, butobserve()listeners (custom balance flow) still don’t subscribe to reset events. After a wallet reset, those subscribers can stay stale until another tx/change event occurs.🔧 Proposed fix (add reset listener inside
observe())val walletChangeListener = object : ThrottlingWalletChangeListener() { override fun onThrottledWalletChanged() { emitBalance() } } + val walletResetListener = object : WalletResetEventListener { + override fun onWalletReset(wallet: Wallet?) { + emitBalance() + } + } wallet.addChangeEventListener(Threading.SAME_THREAD, walletChangeListener) wallet.addCoinsSentEventListener(Threading.SAME_THREAD, walletChangeListener) wallet.addCoinsReceivedEventListener(Threading.SAME_THREAD, walletChangeListener) + wallet.addResetEventListener(Threading.SAME_THREAD, walletResetListener) emitBalance() awaitClose { wallet.removeChangeEventListener(walletChangeListener) wallet.removeCoinsSentEventListener(walletChangeListener) wallet.removeCoinsReceivedEventListener(walletChangeListener) + wallet.removeResetEventListener(walletResetListener) walletChangeListener.removeCallbacks() emitterJob.cancel() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt` around lines 70 - 80, The custom observe() flow doesn't subscribe to wallet reset events, so subscribers can remain stale after a Wallet reset; update the observe() implementation to also register walletResetListener via wallet.addResetEventListener(Threading.SAME_THREAD, walletResetListener) when a collector starts and ensure you remove/unregister that listener on cancellation (mirroring how walletChangeListener is added/removed) so emitBalances()/the custom balance emission runs on reset for observe() subscribers.
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
518-521:⚠️ Potential issue | 🟠 MajorAwait cache deletion during wallet reset to avoid race windows.
Line 518–521 launches deletion in a detached coroutine, but Line 526 invalidates paging immediately. This can interleave reset, invalidation, and incoming updates nondeterministically.
🔧 Proposed fix
+import kotlinx.coroutines.withContext ... walletData.observeWalletReset() .onEach { wrappedTransactionList = emptyList() contactsByTxId = mapOf() - viewModelScope.launch(Dispatchers.IO) { + withContext(Dispatchers.IO) { txDisplayCacheDao.deleteAll() txGroupCacheDao.deleteAll() } walletData.wallet?.let { wallet -> coinJoinWrapperFactory = CoinJoinTxWrapperFactory(walletData.networkParameters, wallet as WalletEx) crowdNodeWrapperFactory = FullCrowdNodeSignUpTxSetFactory(walletData.networkParameters, wallet) } currentPagingSource?.invalidate() }Also applies to: 526-527
🤖 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/MainViewModel.kt` around lines 518 - 521, The cache deletions are launched in a detached IO coroutine (viewModelScope.launch(Dispatchers.IO) { txDisplayCacheDao.deleteAll(); txGroupCacheDao.deleteAll() }) which can run after you immediately invalidate paging, causing race windows; change this to perform the deletions synchronously from the caller coroutine so you await completion before invalidating: e.g. make the surrounding function suspend or use viewModelScope.launch { withContext(Dispatchers.IO) { txDisplayCacheDao.deleteAll(); txGroupCacheDao.deleteAll() } ; /* then call the existing paging invalidation code */ } so the deleteAll() calls finish before you call the invalidation logic referenced around the current lines.
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
338-347: Consolidate ViewModel state into a singleuiStateto avoid split-state drift.This adds another independently exposed async state while
isAbleToCreateIdentityLiveDatastill exists, increasing duplicate state surfaces and update paths.As per coding guidelines,
**/*ViewModel.kt: “ViewModels should use a singleUIStatedata class rather than multiple separate flows”; “UseStateFlow(notLiveData) for asynchronously updated fields in ViewModels”; and “Use private mutable_uiStatewith public immutableuiStateviaasStateFlow()in ViewModels`.🤖 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/MainViewModel.kt` around lines 338 - 347, The ViewModel exposes a separate StateFlow isAbleToCreateIdentity (and still has isAbleToCreateIdentityLiveData), causing split-state drift; replace these by consolidating into a single UI state: create a private MutableStateFlow<_uiState> and public uiState = _uiState.asStateFlow(), define a UIState data class including isAbleToCreateIdentity and any other fields (e.g., blockchainIdentity, isPlatformAvailable), compute isAbleToCreateIdentity by combining isPlatformAvailable and blockchainIdentity (use the existing combine/ combineLatestData logic) and update _uiState inside viewModelScope, and remove the independent isAbleToCreateIdentity StateFlow/LiveData exposures so all async updates flow through the single uiState.
🤖 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/schemas/de.schildbach.wallet.database.AppDatabase/17.json`:
- Line 993: The Room schema shows a mismatch for the filterFlags column: the
created table SQL in AppDatabase schema JSON declares "filterFlags INTEGER NOT
NULL" while the migration SQL in AppDatabaseMigrations (16→17) adds it with
"DEFAULT 0", and the Kotlin entity has a Kotlin default but lacks an explicit
Room default; fix by either adding `@ColumnInfo`(defaultValue = "0") to the
entity's filterFlags property and regenerating the schema (ensuring the entity
name and property are the one named filterFlags), or remove the "DEFAULT 0"
clause from the ALTER TABLE / ADD COLUMN SQL in AppDatabaseMigrations.kt (the
migration that targets version 17) so both migration and entity/schema stay
consistent.
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 1060-1063: The single-tx wrapper uses Base58 for the id which is
inconsistent with the rest of the flow that keys by hex; update the
createSingleTxWrapper implementation so TransactionWrapper.id uses the hex
string (tx.txId.toString()) instead of tx.txId.toStringBase58(), keeping
transactions and groupDate as-is to preserve the existing map key and date
behavior (refer to createSingleTxWrapper, TransactionWrapper, and the id
property).
---
Outside diff comments:
In `@wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt`:
- Around line 70-80: The custom observe() flow doesn't subscribe to wallet reset
events, so subscribers can remain stale after a Wallet reset; update the
observe() implementation to also register walletResetListener via
wallet.addResetEventListener(Threading.SAME_THREAD, walletResetListener) when a
collector starts and ensure you remove/unregister that listener on cancellation
(mirroring how walletChangeListener is added/removed) so emitBalances()/the
custom balance emission runs on reset for observe() subscribers.
---
Duplicate comments:
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 518-521: The cache deletions are launched in a detached IO
coroutine (viewModelScope.launch(Dispatchers.IO) {
txDisplayCacheDao.deleteAll(); txGroupCacheDao.deleteAll() }) which can run
after you immediately invalidate paging, causing race windows; change this to
perform the deletions synchronously from the caller coroutine so you await
completion before invalidating: e.g. make the surrounding function suspend or
use viewModelScope.launch { withContext(Dispatchers.IO) {
txDisplayCacheDao.deleteAll(); txGroupCacheDao.deleteAll() } ; /* then call the
existing paging invalidation code */ } so the deleteAll() calls finish before
you call the invalidation logic referenced around the current lines.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 338-347: The ViewModel exposes a separate StateFlow
isAbleToCreateIdentity (and still has isAbleToCreateIdentityLiveData), causing
split-state drift; replace these by consolidating into a single UI state: create
a private MutableStateFlow<_uiState> and public uiState =
_uiState.asStateFlow(), define a UIState data class including
isAbleToCreateIdentity and any other fields (e.g., blockchainIdentity,
isPlatformAvailable), compute isAbleToCreateIdentity by combining
isPlatformAvailable and blockchainIdentity (use the existing combine/
combineLatestData logic) and update _uiState inside viewModelScope, and remove
the independent isAbleToCreateIdentity StateFlow/LiveData exposures so all async
updates flow through the single uiState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5ecf195-7146-4c22-b6d6-257b43eee6ae
📒 Files selected for processing (8)
wallet/schemas/de.schildbach.wallet.database.AppDatabase/17.jsonwallet/schemas/de.schildbach.wallet.database.AppDatabase/20.jsonwallet/src/de/schildbach/wallet/database/AppDatabase.ktwallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.ktwallet/src/de/schildbach/wallet/database/entity/TxDisplayCacheEntry.ktwallet/src/de/schildbach/wallet/di/DatabaseModule.ktwallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt
- wallet/schemas/de.schildbach.wallet.database.AppDatabase/20.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
524-538:⚠️ Potential issue | 🟠 MajorAwait cache deletes during wallet reset; current detached launch can race with updates.
At Line 528,
viewModelScope.launch(Dispatchers.IO)is detached from the reset flow. Deletes can run concurrently with wrapper resets and paging invalidation, causing inconsistent cache state.🛠️ Suggested synchronization fix
walletData.observeWalletReset() .onEach { wrappedTransactionList = emptyList() contactsByTxId = mapOf() - viewModelScope.launch(Dispatchers.IO) { - txDisplayCacheDao.deleteAll() - txGroupCacheDao.deleteAll() - } + txDisplayCacheDao.deleteAll() + txGroupCacheDao.deleteAll() walletData.wallet?.let { wallet -> coinJoinWrapperFactory = CoinJoinTxWrapperFactory(walletData.networkParameters, wallet as WalletEx) crowdNodeWrapperFactory = FullCrowdNodeSignUpTxSetFactory(walletData.networkParameters, wallet) } currentPagingSource?.invalidate() } - .launchIn(viewModelScope) + .launchIn(viewModelWorkerScope)🤖 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/MainViewModel.kt` around lines 524 - 538, The wallet reset handler launches cache deletes in a detached viewModelScope coroutine which can race with wrapper resets and paging invalidation; instead perform the deletes synchronously in the reset flow by awaiting them (e.g. call txDisplayCacheDao.deleteAll() and txGroupCacheDao.deleteAll() from the current coroutine using withContext(Dispatchers.IO) or make the onEach lambda suspend) before reassigning wrappedTransactionList/contactsByTxId, recreating coinJoinWrapperFactory and crowdNodeWrapperFactory, and calling currentPagingSource?.invalidate(), so the deletes complete before the rest of the reset proceeds.
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
316-324: Use a single loading-visibility render path.
binding.loading.isVisibleis currently written by both load-state handling andisBuildingCacheobservation. Unifying this into one computed path will prevent UI flicker and state overrides.🤖 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/WalletTransactionsFragment.kt` around lines 316 - 324, The fragment currently sets binding.loading.isVisible in two places causing race/flicker: the load-state handling block and the viewModel.isBuildingCache observer; consolidate to a single computed render path inside WalletTransactionsFragment by deriving a single Boolean (e.g., val loadingVisible = isLoading || viewModel.isBuildingCache.value) and use that to set binding.loading.isVisible only once where load-state is processed (keep the existing showEmptyView()/showTransactionList() logic intact), and remove the separate viewModel.isBuildingCache.observe that sets binding.loading.isVisible to avoid conflicting updates.
🤖 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/database/AppDatabaseMigrations.kt`:
- Around line 156-165: Add a non-unique index on tx_group_cache.txId to optimize
queries that start from txId (used by getGroupsForTxIds). In the migration that
creates the tx_group_cache table (in AppDatabaseMigrations.kt), execute an
additional SQL statement like CREATE INDEX IF NOT EXISTS idx_tx_group_cache_txId
ON tx_group_cache(txId) immediately after the CREATE TABLE to ensure the index
is present for existing and new databases. Ensure the index name is unique and
matches the project's naming conventions.
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 172-213: The ViewModel currently exposes multiple parallel flows
(transactionsLoaded, _isBuildingCache/isBuildingCache, cachedRows derived from
_txDataSource, etc.); refactor to a single MainUiState data class and one
private MutableStateFlow _uiState with public uiState via asStateFlow(),
populate fields (transactionsLoaded, isBuildingCache, cachedRows,
canCreateIdentity, ...) and update all places that read
transactionsLoaded/isBuildingCache/cachedRows to read from uiState instead;
ensure you derive cachedRows from the existing _txDataSource when building the
initial ui state and update _uiState whenever those underlying sources change
(e.g., when toggling _isBuildingCache or when _txDataSource emits a
PrebuiltCache) so consumers of transactions/transactionsLoaded refer to uiState
properties rather than separate flows.
In `@wallet/src/de/schildbach/wallet/ui/main/TransactionAdapter.kt`:
- Around line 157-163: In setPrimaryStatus(txView: TransactionRowView) ensure
you clear the recycled view when neither txView.contact nor txView.title is
present: after the existing branches add a fallback that resets
binding.primaryStatus.text (e.g., to an empty string) so previous row text
cannot "bleed" into this holder; reference the setPrimaryStatus function,
txView.contact, txView.title and binding.primaryStatus when making the change.
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 288-305: The problem is that cacheHasItems uses
cacheAdapter.currentList which remains non-empty after cacheAdapter is removed,
hiding loading/empty states; change cacheHasItems to reflect whether the cache
adapter is actually attached to the UI (e.g., check
concatAdapter.adapters.contains(cacheAdapter) and/or
cacheAdapter.isAttachedToRecyclerView) and itemCount, e.g., compute
cacheHasItems = (concatAdapter.adapters.contains(cacheAdapter) &&
cacheAdapter.currentList.isNotEmpty()) or use cacheAdapter.itemCount when
attached; update any logic that relies on cacheHasItems (isLoading/isEmpty) so
it only considers cache content when the cacheAdapter is currently part of
concatAdapter.
---
Duplicate comments:
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 524-538: The wallet reset handler launches cache deletes in a
detached viewModelScope coroutine which can race with wrapper resets and paging
invalidation; instead perform the deletes synchronously in the reset flow by
awaiting them (e.g. call txDisplayCacheDao.deleteAll() and
txGroupCacheDao.deleteAll() from the current coroutine using
withContext(Dispatchers.IO) or make the onEach lambda suspend) before
reassigning wrappedTransactionList/contactsByTxId, recreating
coinJoinWrapperFactory and crowdNodeWrapperFactory, and calling
currentPagingSource?.invalidate(), so the deletes complete before the rest of
the reset proceeds.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 316-324: The fragment currently sets binding.loading.isVisible in
two places causing race/flicker: the load-state handling block and the
viewModel.isBuildingCache observer; consolidate to a single computed render path
inside WalletTransactionsFragment by deriving a single Boolean (e.g., val
loadingVisible = isLoading || viewModel.isBuildingCache.value) and use that to
set binding.loading.isVisible only once where load-state is processed (keep the
existing showEmptyView()/showTransactionList() logic intact), and remove the
separate viewModel.isBuildingCache.observe that sets binding.loading.isVisible
to avoid conflicting updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 649d27f1-1afd-4690-8355-688d3bc6a3f5
📒 Files selected for processing (8)
wallet/res/layout/wallet_transactions_fragment.xmlwallet/res/values/strings-extra.xmlwallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.ktwallet/src/de/schildbach/wallet/database/entity/TxDisplayCacheEntry.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/main/PrebuiltRowsPagingSource.ktwallet/src/de/schildbach/wallet/ui/main/TransactionAdapter.ktwallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt
✅ Files skipped from review due to trivial changes (1)
- wallet/res/layout/wallet_transactions_fragment.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- wallet/res/values/strings-extra.xml
- wallet/src/de/schildbach/wallet/ui/main/PrebuiltRowsPagingSource.kt
Comment on lines
+156
to
+165
| CREATE TABLE IF NOT EXISTS `tx_group_cache` ( | ||
| `groupId` TEXT NOT NULL, | ||
| `txId` TEXT NOT NULL, | ||
| `wrapperType` TEXT NOT NULL, | ||
| `groupDate` TEXT NOT NULL, | ||
| `sortOrder` INTEGER NOT NULL, | ||
| PRIMARY KEY(`groupId`, `txId`) | ||
| ) | ||
| """ | ||
| ) |
Contributor
There was a problem hiding this comment.
Add an index for tx_group_cache.txId to avoid scan-heavy group lookups.
PRIMARY KEY(groupId, txId) does not optimize queries that start from txId. The lazy-loading paths that call getGroupsForTxIds(...) will degrade as cache size grows.
⚡ Suggested migration update
database.execSQL(
"""
CREATE TABLE IF NOT EXISTS `tx_group_cache` (
`groupId` TEXT NOT NULL,
`txId` TEXT NOT NULL,
`wrapperType` TEXT NOT NULL,
`groupDate` TEXT NOT NULL,
`sortOrder` INTEGER NOT NULL,
PRIMARY KEY(`groupId`, `txId`)
)
"""
)
+ database.execSQL(
+ "CREATE INDEX IF NOT EXISTS `index_tx_group_cache_txId` ON `tx_group_cache`(`txId`)"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE IF NOT EXISTS `tx_group_cache` ( | |
| `groupId` TEXT NOT NULL, | |
| `txId` TEXT NOT NULL, | |
| `wrapperType` TEXT NOT NULL, | |
| `groupDate` TEXT NOT NULL, | |
| `sortOrder` INTEGER NOT NULL, | |
| PRIMARY KEY(`groupId`, `txId`) | |
| ) | |
| """ | |
| ) | |
| database.execSQL( | |
| """ | |
| CREATE TABLE IF NOT EXISTS `tx_group_cache` ( | |
| `groupId` TEXT NOT NULL, | |
| `txId` TEXT NOT NULL, | |
| `wrapperType` TEXT NOT NULL, | |
| `groupDate` TEXT NOT NULL, | |
| `sortOrder` INTEGER NOT NULL, | |
| PRIMARY KEY(`groupId`, `txId`) | |
| ) | |
| """ | |
| ) | |
| database.execSQL( | |
| "CREATE INDEX IF NOT EXISTS `index_tx_group_cache_txId` ON `tx_group_cache`(`txId`)" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt` around
lines 156 - 165, Add a non-unique index on tx_group_cache.txId to optimize
queries that start from txId (used by getGroupsForTxIds). In the migration that
creates the tx_group_cache table (in AppDatabaseMigrations.kt), execute an
additional SQL statement like CREATE INDEX IF NOT EXISTS idx_tx_group_cache_txId
ON tx_group_cache(txId) immediately after the CREATE TABLE to ensure the index
is present for existing and new databases. Ensure the index name is unique and
matches the project's naming conventions.
Comment on lines
+172
to
+213
| // In-memory sorted wrapped list — rebuilt when any source changes | ||
| private var wrappedTransactionList: List<TransactionWrapper> = emptyList() | ||
|
|
||
| // Simple flag / flow for WalletFragment and WalletTransactionsFragment | ||
| private val _transactionsLoaded = MutableStateFlow(false) | ||
| val transactionsLoaded: StateFlow<Boolean> = _transactionsLoaded | ||
|
|
||
| /** True while a full cache rebuild is in progress (first run or user-initiated refresh). */ | ||
| private val _isBuildingCache = MutableStateFlow(false) | ||
| val isBuildingCache: StateFlow<Boolean> = _isBuildingCache | ||
|
|
||
| private val pagingConfig = PagingConfig(pageSize = 50, prefetchDistance = 20, enablePlaceholders = false) | ||
| // The current active PagingSource — kept so contact changes can trigger manual invalidate(). | ||
| // Only used for the RoomLive paging source; PrebuiltCache uses a separate in-memory source. | ||
| private var currentPagingSource: PagingSource<Int, TxDisplayCacheEntry>? = null | ||
|
|
||
| /** | ||
| * Two-phase data source for the transaction list: | ||
| * - [TxDataSource.Empty]: no data yet (ViewModel just created, cache not loaded) | ||
| * - [TxDataSource.PrebuiltCache]: initial fast display — rows already include date-header | ||
| * entries interleaved, so no [insertSeparators] transform is needed on this path | ||
| * - [TxDataSource.RoomLive]: live Room-backed paging after [rebuildWrappedList] writes | ||
| * fresh data; Room's [InvalidationTracker] auto-invalidates on metadata/tx changes | ||
| */ | ||
| private sealed class TxDataSource { | ||
| object Empty : TxDataSource() | ||
| class PrebuiltCache(val rows: List<HistoryRowView>) : TxDataSource() | ||
| data class RoomLive(val filterFlag: Int) : TxDataSource() | ||
| } | ||
| private val _txDataSource = MutableStateFlow<TxDataSource>(TxDataSource.Empty) | ||
|
|
||
| /** | ||
| * Cache rows for the fast startup phase, exposed so [WalletTransactionsFragment] can call | ||
| * [CacheTransactionAdapter.submitList] directly — a single background DiffUtil + one | ||
| * main-thread handler post — instead of going through [PagingDataAdapter.submitData]'s | ||
| * multi-dispatch coroutine chain which is slow when the main looper is congested at startup. | ||
| */ | ||
| val cachedRows: StateFlow<List<HistoryRowView>> = _txDataSource | ||
| .map { source -> if (source is TxDataSource.PrebuiltCache) source.rows else emptyList() } | ||
| .stateIn(viewModelScope, SharingStarted.Eagerly, emptyList()) | ||
|
|
||
| val transactions: Flow<PagingData<HistoryRowView>> = _txDataSource |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate the newly added ViewModel state into a single UIState flow.
This change adds more parallel state channels instead of a unified uiState, which makes consistency and rendering transitions harder to reason about.
♻️ Direction for alignment
data class MainUiState(
val transactionsLoaded: Boolean = false,
val isBuildingCache: Boolean = false,
val cachedRows: List<HistoryRowView> = emptyList(),
val canCreateIdentity: Boolean = false,
// ...
)
private val _uiState = MutableStateFlow(MainUiState())
val uiState = _uiState.asStateFlow()As per coding guidelines, "ViewModels should use a single UIState data class rather than multiple separate flows" and "Use private mutable _uiState with public immutable uiState via asStateFlow() in ViewModels".
Also applies to: 344-353
🤖 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/MainViewModel.kt` around lines 172 -
213, The ViewModel currently exposes multiple parallel flows
(transactionsLoaded, _isBuildingCache/isBuildingCache, cachedRows derived from
_txDataSource, etc.); refactor to a single MainUiState data class and one
private MutableStateFlow _uiState with public uiState via asStateFlow(),
populate fields (transactionsLoaded, isBuildingCache, cachedRows,
canCreateIdentity, ...) and update all places that read
transactionsLoaded/isBuildingCache/cachedRows to read from uiState instead;
ensure you derive cachedRows from the existing _txDataSource when building the
initial ui state and update _uiState whenever those underlying sources change
(e.g., when toggling _isBuildingCache or when _txDataSource emits a
PrebuiltCache) so consumers of transactions/transactionsLoaded refer to uiState
properties rather than separate flows.
Comment on lines
+157
to
163
| private fun setPrimaryStatus(txView: TransactionRowView) { | ||
| if (txView.contact != null) { | ||
| val name = txView.contact.displayName.ifEmpty { txView.contact.username } | ||
| binding.primaryStatus.text = name | ||
| } else if (txView.title != null) { | ||
| binding.primaryStatus.text = txView.title!!.format(resources) | ||
| } |
Contributor
There was a problem hiding this comment.
Reset primaryStatus when no contact/title is available to avoid recycled text bleed.
If a recycled holder reaches this branch with both fields absent, prior row text can remain visible.
✅ Minimal safe fix
private fun setPrimaryStatus(txView: TransactionRowView) {
if (txView.contact != null) {
val name = txView.contact.displayName.ifEmpty { txView.contact.username }
binding.primaryStatus.text = name
} else if (txView.title != null) {
binding.primaryStatus.text = txView.title!!.format(resources)
+ } else {
+ binding.primaryStatus.text = null
}
binding.primaryStatus.setTextColor(contentColor)
}🤖 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/TransactionAdapter.kt` around lines
157 - 163, In setPrimaryStatus(txView: TransactionRowView) ensure you clear the
recycled view when neither txView.contact nor txView.title is present: after the
existing branches add a fallback that resets binding.primaryStatus.text (e.g.,
to an empty string) so previous row text cannot "bleed" into this holder;
reference the setPrimaryStatus function, txView.contact, txView.title and
binding.primaryStatus when making the change.
Comment on lines
+288
to
+305
| val cacheHasItems = cacheAdapter.currentList.isNotEmpty() | ||
| // Only show the loading spinner when there's nothing to display yet. | ||
| val isLoading = isRefreshing && liveAdapter.itemCount == 0 && !cacheHasItems | ||
| val isEmpty = loadStates.refresh is LoadState.NotLoading && | ||
| liveAdapter.itemCount == 0 && !cacheHasItems | ||
|
|
||
| // Swap cacheAdapter → liveAdapter inside the same ConcatAdapter once live | ||
| // items are ready. Keeping one ConcatAdapter instance avoids the | ||
| // "cannot find wrapper" crash from ConcatAdapterController. | ||
| if (loadStates.refresh is LoadState.NotLoading && liveAdapter.itemCount > 0 && | ||
| concatAdapter.adapters.contains(cacheAdapter)) { | ||
| log.info("STARTUP swapping to live adapter: {} items at {}", | ||
| liveAdapter.itemCount, System.currentTimeMillis()) | ||
| val lm = binding.walletTransactionsList.layoutManager as LinearLayoutManager | ||
| val scrollState = lm.onSaveInstanceState() | ||
| concatAdapter.removeAdapter(cacheAdapter) | ||
| concatAdapter.addAdapter(liveAdapter) | ||
| lm.onRestoreInstanceState(scrollState) |
Contributor
There was a problem hiding this comment.
cacheHasItems should be tied to adapter attachment, not just currentList.
After cacheAdapter is removed, currentList is still non-empty. That keeps cacheHasItems=true, which can hide valid empty/loading states for the live list.
🧩 Suggested fix
- val cacheHasItems = cacheAdapter.currentList.isNotEmpty()
+ val cacheAttached = concatAdapter.adapters.contains(cacheAdapter)
+ val cacheHasItems = cacheAttached && cacheAdapter.currentList.isNotEmpty()
@@
if (loadStates.refresh is LoadState.NotLoading && liveAdapter.itemCount > 0 &&
concatAdapter.adapters.contains(cacheAdapter)) {
@@
concatAdapter.removeAdapter(cacheAdapter)
+ cacheAdapter.submitList(emptyList())
concatAdapter.addAdapter(liveAdapter)
lm.onRestoreInstanceState(scrollState)
}🤖 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/WalletTransactionsFragment.kt` around
lines 288 - 305, The problem is that cacheHasItems uses cacheAdapter.currentList
which remains non-empty after cacheAdapter is removed, hiding loading/empty
states; change cacheHasItems to reflect whether the cache adapter is actually
attached to the UI (e.g., check concatAdapter.adapters.contains(cacheAdapter)
and/or cacheAdapter.isAttachedToRecyclerView) and itemCount, e.g., compute
cacheHasItems = (concatAdapter.adapters.contains(cacheAdapter) &&
cacheAdapter.currentList.isNotEmpty()) or use cacheAdapter.itemCount when
attached; update any logic that relies on cacheHasItems (isLoading/isEmpty) so
it only considers cache content when the cacheAdapter is currently part of
concatAdapter.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
238-247:⚠️ Potential issue | 🟠 MajorDo not let
cacheAdapter.currentListoutlive the cache phase.
filterNot { it.isEmpty() }meanssubmitList(emptyList())never runs, socacheAdapter.currentListstays populated afterTxDataSource.Emptyor after the adapter is removed. That keepscacheHasItemstrue even when the cache adapter is off-screen, which hides real loading/empty states and can leave stale cache rows visible during a force rebuild. Submit empty lists too, and basecacheHasItemson adapter attachment + item count.Suggested change
viewLifecycleOwner.lifecycleScope.launch { viewModel.cachedRows - .filterNot { it.isEmpty() } .collect { rows -> log.info("STARTUP cache submitList: {} rows at {}", rows.size, System.currentTimeMillis()) if (firstPageLoadStartTime == 0L) { firstPageLoadStartTime = System.currentTimeMillis() } cacheAdapter.submitList(rows) - showTransactionList() + if (rows.isNotEmpty()) { + showTransactionList() + } } } @@ - val cacheHasItems = cacheAdapter.currentList.isNotEmpty() + val cacheHasItems = + concatAdapter.adapters.contains(cacheAdapter) && cacheAdapter.itemCount > 0 @@ val lm = binding.walletTransactionsList.layoutManager as LinearLayoutManager val scrollState = lm.onSaveInstanceState() concatAdapter.removeAdapter(cacheAdapter) + cacheAdapter.submitList(emptyList()) concatAdapter.addAdapter(liveAdapter) lm.onRestoreInstanceState(scrollState) }Also applies to: 295-313
🤖 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/WalletTransactionsFragment.kt` around lines 238 - 247, The fragment filters out empty cache emissions so cacheAdapter.submitList(emptyList()) never runs, letting cacheAdapter.currentList persist and causing cacheHasItems to be stale; change the cachedRows collector (the block using viewModel.cachedRows, filterNot { it.isEmpty() }, and cacheAdapter.submitList(rows)) to submit the empty list as well (remove or alter the filterNot) and update any logic that computes cacheHasItems to consider both adapter.isAttachedToRecyclerView (or adapter attachment state) AND adapter.itemCount > 0; apply the same fix to the other cachedRows collector block that mirrors this behavior (the later block using cacheAdapter and cacheHasItems) so TxDataSource.Empty truly clears the cache display.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
539-554:⚠️ Potential issue | 🟠 MajorSerialize wallet reset on
viewModelWorkerScope.This reset still mutates
wrappedTransactionList/contactsByTxIdonviewModelScope, then starts cache deletion in a detachedlaunch(Dispatchers.IO). A reset that lands duringupdateWrappedListForTransactions()or the metadata/contact upserts can repopulate tables after they've been "cleared". Run the whole block on the worker scope and await the deletes before invalidating the paging source.Suggested change
walletData.observeWalletReset() .onEach { wrappedTransactionList = emptyList() contactsByTxId = mapOf() - viewModelScope.launch(Dispatchers.IO) { - txDisplayCacheDao.deleteAll() - txGroupCacheDao.deleteAll() - } + txDisplayCacheDao.deleteAll() + txGroupCacheDao.deleteAll() walletData.wallet?.let { wallet -> coinJoinWrapperFactory = CoinJoinTxWrapperFactory(walletData.networkParameters, wallet as WalletEx) crowdNodeWrapperFactory = FullCrowdNodeSignUpTxSetFactory(walletData.networkParameters, wallet) } _currentPagingSource.value?.invalidate() } .catch { e -> log.error("wallet reset flow error", e) } - .launchIn(viewModelScope) + .launchIn(viewModelWorkerScope)🤖 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/MainViewModel.kt` around lines 539 - 554, The wallet reset mutates wrappedTransactionList and contactsByTxId on viewModelScope then launches detached deletes with launch(Dispatchers.IO), which can race with updateWrappedListForTransactions and metadata upserts; move the entire observeWalletReset handler to run inside viewModelWorkerScope, perform txDisplayCacheDao.deleteAll() and txGroupCacheDao.deleteAll() as suspend calls (or with withContext(Dispatchers.IO)) and await their completion before invalidating _currentPagingSource.value, and ensure coinJoinWrapperFactory / crowdNodeWrapperFactory assignment and the wrappedTransactionList/contactsByTxId clears occur within that serialized worker-scope block so no repopulation races occur.
🤖 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/ui/main/MainViewModel.kt`:
- Around line 923-927: The lazy wrapper loading path (loadWrapperOnDemand)
mutates the shared wrappedTransactionList from callers using
lifecycleScope.launch (e.g., WalletTransactionsFragment), creating a race with
the viewModelWorkerScope processing pipeline; ensure the lookup-and-append runs
on the same worker scope or is synchronized: move the wrappedTransactionList
none/{wrappedTransactionList = ...} update into viewModelWorkerScope (use
viewModelWorkerScope.launch { ... }) or protect wrappedTransactionList with a
mutex/state holder (e.g., a Mutex around the check+append or an
Atomic/MutableState wrapper) so loadWrapperOnDemand and the worker pipeline
cannot interleave and lose updates.
- Around line 387-409: The startup path loads all rows via
txDisplayCacheDao.getAll() and promotes them to TxDataSource.PrebuiltCache
without applying the restored transactionsDirection filter, which can show the
wrong tab; modify the coroutine that builds historyRows (use
transactionsDirection/current direction variable restored around Line 380) to
filter cachedRows before mapping (e.g., only include entries whose direction
matches the current transactionsDirection or map to TransactionRowView then
filter by its direction) so that when you assign _txDataSource.value =
TxDataSource.PrebuiltCache(historyRows) and set _transactionsLoaded.value = true
the PrebuiltCache only contains rows matching the active direction; use
contactsByTxId and existing toTransactionRowView conversion but apply the
direction check prior to adding to historyRows.
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 324-332: The loading overlay is driven from two places which
conflicts: the inline assignment using isLoading ||
viewModel.isBuildingCache.value and the later observer on
viewModel.isBuildingCache that overwrites it; remove the second observer and
drive binding.loading.isVisible from a single combined state instead (e.g.
create a combined Boolean in the fragment or a MediatorLiveData/LiveData in the
ViewModel like isLoadingCombined that merges isLoading and
viewModel.isBuildingCache, then observe that single source and set
binding.loading.isVisible accordingly), ensuring references to
binding.loading.isVisible, viewModel.isBuildingCache, the local isLoading flag,
and the existing showEmptyView()/showTransactionList() logic remain consistent.
---
Duplicate comments:
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 539-554: The wallet reset mutates wrappedTransactionList and
contactsByTxId on viewModelScope then launches detached deletes with
launch(Dispatchers.IO), which can race with updateWrappedListForTransactions and
metadata upserts; move the entire observeWalletReset handler to run inside
viewModelWorkerScope, perform txDisplayCacheDao.deleteAll() and
txGroupCacheDao.deleteAll() as suspend calls (or with
withContext(Dispatchers.IO)) and await their completion before invalidating
_currentPagingSource.value, and ensure coinJoinWrapperFactory /
crowdNodeWrapperFactory assignment and the wrappedTransactionList/contactsByTxId
clears occur within that serialized worker-scope block so no repopulation races
occur.
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 238-247: The fragment filters out empty cache emissions so
cacheAdapter.submitList(emptyList()) never runs, letting
cacheAdapter.currentList persist and causing cacheHasItems to be stale; change
the cachedRows collector (the block using viewModel.cachedRows, filterNot {
it.isEmpty() }, and cacheAdapter.submitList(rows)) to submit the empty list as
well (remove or alter the filterNot) and update any logic that computes
cacheHasItems to consider both adapter.isAttachedToRecyclerView (or adapter
attachment state) AND adapter.itemCount > 0; apply the same fix to the other
cachedRows collector block that mirrors this behavior (the later block using
cacheAdapter and cacheHasItems) so TxDataSource.Empty truly clears the cache
display.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b9f235-ae6b-4b32-aa90-3cb8d38038a2
📒 Files selected for processing (2)
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt
Comment on lines
+324
to
+332
| binding.loading.isVisible = isLoading || viewModel.isBuildingCache.value | ||
| if (isEmpty && header.isEmpty()) showEmptyView() else showTransactionList() | ||
| } | ||
| } | ||
|
|
||
| // Show the "determining transaction history" overlay while the cache is being built. | ||
| viewModel.isBuildingCache.observe(viewLifecycleOwner) { building -> | ||
| binding.loading.isVisible = building | ||
| } |
Contributor
There was a problem hiding this comment.
Use one source of truth for the loading overlay.
Lines 324-325 already compute isLoading || viewModel.isBuildingCache.value, but the separate observer on Lines 329-332 later overwrites that with just building. When a rebuild flips building to false before the first live page arrives, this second collector can hide the spinner while the list is still empty and loading. Keep binding.loading.isVisible driven from a single combined state.
🤖 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/WalletTransactionsFragment.kt` around
lines 324 - 332, The loading overlay is driven from two places which conflicts:
the inline assignment using isLoading || viewModel.isBuildingCache.value and the
later observer on viewModel.isBuildingCache that overwrites it; remove the
second observer and drive binding.loading.isVisible from a single combined state
instead (e.g. create a combined Boolean in the fragment or a
MediatorLiveData/LiveData in the ViewModel like isLoadingCombined that merges
isLoading and viewModel.isBuildingCache, then observe that single source and set
binding.loading.isVisible accordingly), ensuring references to
binding.loading.isVisible, viewModel.isBuildingCache, the local isLoading flag,
and the existing showEmptyView()/showTransactionList() logic remain consistent.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (2)
326-344:⚠️ Potential issue | 🟡 MinorLoading overlay is controlled from two places.
binding.loading.isVisibleis set both in theloadStateFlowcollector (line 328) and in theisBuildingCacheobserver (line 339). These can conflict: theisBuildingCacheobserver runs independently and may overwrite the computed value from the load state collector.Consider combining these into a single source of truth for the loading state.
🔧 Suggested approach: single combined observer
Remove the separate
isBuildingCache.observeand incorporate its logic into theloadStateFlowcollector wherebuildingFromScratchis already computed. This ensures one code path controls the loading visibility:// In loadStateFlow collector (already has buildingFromScratch): binding.loading.isVisible = isLoading || buildingFromScratch // Handle the toast separately: viewModel.isBuildingCache.observe(viewLifecycleOwner) { building -> if (!building && pendingManualRefresh) { pendingManualRefresh = false Toast.makeText(requireContext(), R.string.history_refresh_complete, Toast.LENGTH_SHORT).show() } }🤖 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/WalletTransactionsFragment.kt` around lines 326 - 344, The loading overlay is being set in two places causing conflicts; remove the separate viewModel.isBuildingCache.observe block that assigns binding.loading.isVisible and instead compute visibility only inside the loadStateFlow collector using the existing buildingFromScratch and isLoading values (keep binding.loading.isVisible = isLoading || buildingFromScratch there), then preserve a minimal observer for viewModel.isBuildingCache only to handle the pendingManualRefresh/Toast logic (when !building && pendingManualRefresh reset flag and show history_refresh_complete). Ensure you reference the same symbols: loadStateFlow collector, buildingFromScratch, binding.loading.isVisible, viewModel.isBuildingCache.observe, pendingManualRefresh, and the Toast call.
296-302:⚠️ Potential issue | 🟡 Minor
cacheHasItemsmay still produce incorrect state after adapter swap.After
cacheAdapteris removed fromconcatAdapter(line 313),cacheAdapter.currentListremains non-empty becausesubmitList(emptyList())is never called. This meanscacheHasItemscan incorrectly remaintrue, potentially hiding valid empty states.🔧 Suggested fix: clear cache list after swap
if (loadStates.refresh is LoadState.NotLoading && liveAdapter.itemCount > 0 && concatAdapter.adapters.contains(cacheAdapter)) { log.info("STARTUP swapping to live adapter: {} items at {}", liveAdapter.itemCount, System.currentTimeMillis()) val lm = binding.walletTransactionsList.layoutManager as LinearLayoutManager val scrollState = lm.onSaveInstanceState() concatAdapter.removeAdapter(cacheAdapter) + cacheAdapter.submitList(emptyList()) // Clear to fix cacheHasItems check concatAdapter.addAdapter(liveAdapter) lm.onRestoreInstanceState(scrollState) }🤖 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/WalletTransactionsFragment.kt` around lines 296 - 302, The cacheAdapter's currentList can remain non-empty after you remove it from the concat adapter, causing cacheHasItems to be stale; after the code path that removes or swaps out cacheAdapter from concatAdapter (where concatAdapter.removeAdapter(cacheAdapter) or the adapter swap happens), explicitly clear its data by calling cacheAdapter.submitList(emptyList()) so cacheAdapter.currentList becomes empty and isEmpty/isLoading are computed correctly.
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt (1)
678-695: Parallel contact resolution may overwhelm the platform.
resolveAllContactslaunches parallelasynccoroutines onDispatchers.IOfor every transaction needing contact resolution (line 681). With a large transaction history, this could spawn hundreds of concurrent network requests to the platform.Consider limiting concurrency or batching requests.
♻️ Suggested approach using chunking
// Process in batches of 10 to limit concurrent platform calls val resolved = coroutineScope { txsToResolve .chunked(10) .flatMap { chunk -> chunk.map { tx -> async(Dispatchers.IO) { // ... existing resolution logic } }.awaitAll() } .filterNotNull() .toMap() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt` around lines 678 - 695, The current implementation launches an async(Dispatchers.IO) for every element in txsToResolve which can spawn too many concurrent platform calls; modify resolveAllContacts to limit concurrency by processing txsToResolve in bounded batches (e.g., chunked(10)) or by using a CoroutineDispatcher/Semaphore to cap parallelism, then for each batch map to async { platformRepo.blockchainIdentity.getContactForTransaction(tx) ... } .awaitAll(), collect non-null pairs and merge to a map as before; reference txsToResolve, platformRepo.blockchainIdentity.getContactForTransaction, and Dispatchers.IO 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/TxDisplayCacheService.kt`:
- Around line 314-329: The nested serviceScope.launch causes cache deletions to
run asynchronously and can race with subsequent resets; remove the inner launch
and call txDisplayCacheDao.deleteAll() and txGroupCacheDao.deleteAll() directly
(they are suspend functions) inside the onEach block so deletions complete
before resetting wrappedTransactionList, setting
coinJoinWrapperFactory/crowdNodeWrapperFactory, and calling
_currentPagingSource.value?.invalidate(); this ensures synchronous cache
clearing when walletData.observeWalletReset() fires.
- Around line 212-234: The force-unwrap walletData.wallet!! inside the
flatMapLatest (where TxDirectionFilter(direction, walletData.wallet!!) is
created) can throw and terminate the flow; change to a null-safe pattern: inside
the flatMapLatest retrieve walletData.wallet into a local val (e.g., val w =
walletData.wallet) and if it's null return an empty/idle flow (or skip by using
return@flatMapLatest emptyFlow()) so the branch uses a non-null wallet only when
available; update subsequent calls that reference the wallet (TxDirectionFilter
and walletData.observeTransactions) to use that local non-null wallet and avoid
the !! operator so the flow remains alive during shutdown/initialization.
- Around line 368-377: The forceRebuildTransactionCache function calls
walletData.wallet!! which can NPE; change it to defensively handle a null wallet
by checking walletData.wallet first and returning early (or setting
TxDataSource.Empty) if null, then only create the TxDirectionFilter and call
rebuildWrappedList when wallet is non-null; locate this in
forceRebuildTransactionCache and replace the force-unwrap usage with a safe
check (e.g., let/if-null-return) before constructing TxDirectionFilter and
invoking rebuildWrappedList.
---
Duplicate comments:
In `@wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt`:
- Around line 326-344: The loading overlay is being set in two places causing
conflicts; remove the separate viewModel.isBuildingCache.observe block that
assigns binding.loading.isVisible and instead compute visibility only inside the
loadStateFlow collector using the existing buildingFromScratch and isLoading
values (keep binding.loading.isVisible = isLoading || buildingFromScratch
there), then preserve a minimal observer for viewModel.isBuildingCache only to
handle the pendingManualRefresh/Toast logic (when !building &&
pendingManualRefresh reset flag and show history_refresh_complete). Ensure you
reference the same symbols: loadStateFlow collector, buildingFromScratch,
binding.loading.isVisible, viewModel.isBuildingCache.observe,
pendingManualRefresh, and the Toast call.
- Around line 296-302: The cacheAdapter's currentList can remain non-empty after
you remove it from the concat adapter, causing cacheHasItems to be stale; after
the code path that removes or swaps out cacheAdapter from concatAdapter (where
concatAdapter.removeAdapter(cacheAdapter) or the adapter swap happens),
explicitly clear its data by calling cacheAdapter.submitList(emptyList()) so
cacheAdapter.currentList becomes empty and isEmpty/isLoading are computed
correctly.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt`:
- Around line 678-695: The current implementation launches an
async(Dispatchers.IO) for every element in txsToResolve which can spawn too many
concurrent platform calls; modify resolveAllContacts to limit concurrency by
processing txsToResolve in bounded batches (e.g., chunked(10)) or by using a
CoroutineDispatcher/Semaphore to cap parallelism, then for each batch map to
async { platformRepo.blockchainIdentity.getContactForTransaction(tx) ... }
.awaitAll(), collect non-null pairs and merge to a map as before; reference
txsToResolve, platformRepo.blockchainIdentity.getContactForTransaction, and
Dispatchers.IO when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ea7c356-4991-49e9-b030-c89f5a882099
📒 Files selected for processing (5)
wallet/res/values/strings-extra.xmlwallet/src/de/schildbach/wallet/WalletApplication.javawallet/src/de/schildbach/wallet/service/TxDisplayCacheService.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- wallet/res/values/strings-extra.xml
- wallet/src/de/schildbach/wallet/WalletApplication.java
wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+314
to
+329
| walletData.observeWalletReset() | ||
| .onEach { | ||
| wrappedTransactionList = emptyList() | ||
| contactsByTxId = mapOf() | ||
| serviceScope.launch { | ||
| txDisplayCacheDao.deleteAll() | ||
| txGroupCacheDao.deleteAll() | ||
| } | ||
| walletData.wallet?.let { wallet -> | ||
| coinJoinWrapperFactory = CoinJoinTxWrapperFactory(walletData.networkParameters, wallet as WalletEx) | ||
| crowdNodeWrapperFactory = FullCrowdNodeSignUpTxSetFactory(walletData.networkParameters, wallet) | ||
| } | ||
| _currentPagingSource.value?.invalidate() | ||
| } | ||
| .catch { e -> log.error("wallet reset flow error", e) } | ||
| .launchIn(serviceScope) |
Contributor
There was a problem hiding this comment.
Nested coroutine launch during wallet reset may cause race condition.
At line 318, a nested serviceScope.launch is used for cache deletion. Since the outer onEach already runs on serviceScope, the deletion happens asynchronously while the wrapper factory reset (lines 322-325) and paging source invalidation (line 326) proceed immediately.
If a transaction update arrives during deletion, it could write to a cache being cleared. Consider using synchronous execution.
🔧 Suggested fix: make deletion synchronous within the flow
walletData.observeWalletReset()
.onEach {
wrappedTransactionList = emptyList()
contactsByTxId = mapOf()
- serviceScope.launch {
- txDisplayCacheDao.deleteAll()
- txGroupCacheDao.deleteAll()
- }
+ // Delete synchronously to prevent races with incoming tx updates
+ txDisplayCacheDao.deleteAll()
+ txGroupCacheDao.deleteAll()
walletData.wallet?.let { wallet ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/service/TxDisplayCacheService.kt` around
lines 314 - 329, The nested serviceScope.launch causes cache deletions to run
asynchronously and can race with subsequent resets; remove the inner launch and
call txDisplayCacheDao.deleteAll() and txGroupCacheDao.deleteAll() directly
(they are suspend functions) inside the onEach block so deletions complete
before resetting wrappedTransactionList, setting
coinJoinWrapperFactory/crowdNodeWrapperFactory, and calling
_currentPagingSource.value?.invalidate(); this ensures synchronous cache
clearing when walletData.observeWalletReset() fires.
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
1 participant
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
Improvements
Documentation
Tests
Chores