fix: eliminate crash issues when fragments/dialogs recreated and address network error #1407
Conversation
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
…haseGiftCardFragment
WalkthroughThese changes add saved state management to ViewModels for amount entry and merchant data, enhance null safety and error handling in merchant and gift card purchase flows, extend data source and DAO interfaces for merchant retrieval by ID, and refactor UI components to better handle nullable merchant states and process death scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PurchaseGiftCardFragment
participant CTXSpendViewModel
participant MerchantDao
participant EnterAmountViewModel
User->>PurchaseGiftCardFragment: Navigates to purchase gift card
PurchaseGiftCardFragment->>CTXSpendViewModel: Get saved merchant ID
alt Merchant ID found
PurchaseGiftCardFragment->>CTXSpendViewModel: getMerchantById(merchantId)
CTXSpendViewModel->>MerchantDao: getMerchantById(merchantId)
MerchantDao-->>CTXSpendViewModel: Merchant?
CTXSpendViewModel-->>PurchaseGiftCardFragment: Merchant?
alt Merchant found
PurchaseGiftCardFragment->>CTXSpendViewModel: Setup merchant
else Merchant not found
PurchaseGiftCardFragment->>User: Navigate back
end
else No merchant ID
PurchaseGiftCardFragment->>User: Navigate back
end
sequenceDiagram
participant User
participant PurchaseGiftCardConfirmDialog
participant EnterAmountViewModel
participant CTXSpendViewModel
User->>PurchaseGiftCardConfirmDialog: Clicks confirm purchase
PurchaseGiftCardConfirmDialog->>CTXSpendViewModel: Check giftCardMerchant
alt Merchant is null
PurchaseGiftCardConfirmDialog->>User: Dismiss dialog
else Merchant is valid
PurchaseGiftCardConfirmDialog->>CTXSpendViewModel: Purchase gift card
alt Success
PurchaseGiftCardConfirmDialog->>EnterAmountViewModel: clearSavedState()
PurchaseGiftCardConfirmDialog->>User: Show gift card details
else CTXSpendException
alt isNetworkError
PurchaseGiftCardConfirmDialog->>User: Show network error dialog
else
PurchaseGiftCardConfirmDialog->>User: Show error dialog
end
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountViewModel.kt (1)
139-147: Consider lifecycle-aware observers to prevent memory leaks.Using
observeForevercan lead to memory leaks if the ViewModel outlives the observers. Since SavedStateHandle persists across configuration changes, this might be acceptable, but consider documenting whyobserveForeveris safe here.Add a comment explaining the safety of using
observeForever:+ // Using observeForever is safe here because: + // 1. ViewModel and SavedStateHandle have the same lifecycle + // 2. These observers are needed for the entire ViewModel lifetime + // 3. They will be cleaned up when ViewModel is cleared // Save amount changes to SavedStateHandle _amount.observeForever { coin -> savedStateHandle[KEY_AMOUNT] = coin?.value }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt(2 hunks)common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountViewModel.kt(5 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(10 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt(8 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt(7 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: common/src/main/java/org/dash/wallet/common/ui/segmented_picker/SegmentedPicker.kt:70-83
Timestamp: 2025-05-12T09:17:22.548Z
Learning: The SegmentedPicker component in the Dash Wallet app intentionally uses a mixed state pattern (both internal state and external parameter) to handle animations internally while still respecting external state changes. This design allows the component to manage animation timing independently without requiring the parent component to handle animation logic.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt (1)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt:106-115
Timestamp: 2025-05-07T14:17:15.912Z
Learning: In the Dash Wallet app, MerchantDao interface methods use an "excludeType" parameter (e.g., `MerchantType.ONLINE`) to exclude merchants of a specific type rather than to include them. For example, in `observePhysicalMerchants()`, passing `MerchantType.ONLINE` as the exclude type parameter filters out online-only merchants, effectively returning physical merchants.
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt (5)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt (1)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt:106-115
Timestamp: 2025-05-07T14:17:15.912Z
Learning: In the Dash Wallet app, MerchantDao interface methods use an "excludeType" parameter (e.g., `MerchantType.ONLINE`) to exclude merchants of a specific type rather than to include them. For example, in `observePhysicalMerchants()`, passing `MerchantType.ONLINE` as the exclude type parameter filters out online-only merchants, effectively returning physical merchants.
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountViewModel.kt (5)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (7)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (5)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (6)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt:106-115
Timestamp: 2025-05-07T14:17:15.912Z
Learning: In the Dash Wallet app, MerchantDao interface methods use an "excludeType" parameter (e.g., `MerchantType.ONLINE`) to exclude merchants of a specific type rather than to include them. For example, in `observePhysicalMerchants()`, passing `MerchantType.ONLINE` as the exclude type parameter filters out online-only merchants, effectively returning physical merchants.
🪛 detekt (1.23.8)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
[warning] 159-159: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (19)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt (1)
493-494: LGTM! Clean implementation following established patterns.The new
getMerchantByIdmethod properly implements merchant lookup by ID with appropriate nullable return type and correct SQL syntax.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt (2)
84-84: Interface extension looks good.The new
getMerchantByIdmethod signature is consistent with the repository pattern and matches the DAO implementation.
514-516: Proper delegation to DAO layer.The implementation correctly delegates to the
merchantDao.getMerchantById()method, maintaining clean separation of concerns.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (3)
37-37: Appropriate import for network error detection.Adding
SSLHandshakeExceptionimport supports the new network error detection capability.
42-44: Enhanced exception constructor improves error chaining.Adding the optional
causeparameter enables proper exception chaining while maintaining backward compatibility with existing constructors.
70-71: Useful network error detection property.The
isNetworkErrorproperty provides a clean way to identify SSL handshake failures, enabling more specific error handling in the UI layer.wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.kt (1)
75-84: Good defensive programming to prevent NPE.The null check for
cftx.assetLockPublicKeyIdbefore accessing itsbytesproperty prevents potential crashes and provides appropriate fallback behavior.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (5)
28-28: Good integration of EnterAmountViewModel for state management.Adding activity-scoped
EnterAmountViewModelenables proper state persistence and cleanup across the purchase flow.Also applies to: 43-43, 67-67
80-84: Essential null safety check prevents dialog crashes.The early null check for
giftCardMerchantwith proper logging and dialog dismissal prevents crashes when merchant data is unavailable during fragment recreation.
108-113: Defensive double-check prevents race conditions.Re-checking merchant availability before proceeding with confirmation guards against race conditions where merchant data might become null during async operations.
126-133: Enhanced network error handling improves UX.The specific handling of network errors using
ex.isNetworkErrorprovides users with more meaningful error messages for SSL handshake failures and other network issues.
180-180: Proper state cleanup after successful purchase.Clearing the saved state in
EnterAmountViewModelafter successful purchase prevents stale input data from affecting subsequent transactions.common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt (1)
79-82: Good practice: Clearing saved state on navigation back.Clearing the saved state when navigating back prevents stale data from persisting and improves the user experience.
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountViewModel.kt (1)
84-92: Robust error handling for saved state restoration.Good defensive programming by catching parsing exceptions when restoring fiat amount from saved state. This prevents crashes from corrupted or invalid saved data.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (2)
90-102: Well-structured state restoration logic.The merchant setup logic properly handles state restoration by checking saved merchant ID first, then falling back to the currently selected merchant. The error handling with navigation back ensures the UI doesn't get stuck in an invalid state.
214-219: Good null safety handling for merchant access.The refactored code properly handles the nullable merchant with safe calls and early returns, preventing potential null pointer exceptions.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (3)
86-90: Comprehensive state persistence implementation.Good implementation of SavedStateHandle for both balance and merchant ID persistence. The observers ensure state is automatically saved when values change.
Also applies to: 142-145
116-121: Smart implementation of nullable merchant with state preservation.Converting from lateinit to nullable with a custom setter that saves the merchant ID is a robust approach that eliminates potential UninitializedPropertyAccessException while maintaining state persistence.
373-375: Good practice using IO dispatcher for database operations.Properly using
withContext(Dispatchers.IO)for the database query ensures the operation doesn't block the main thread.
common/src/main/java/org/dash/wallet/common/ui/enter_amount/EnterAmountFragment.kt
Outdated
Show resolved
Hide resolved
...h/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt
Outdated
Show resolved
Hide resolved
...loredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
Outdated
Show resolved
Hide resolved
…not on the backstack
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…-wallet into fix/ctx-crash-issue
|
@coderabbitai can you review again in light of the PR description |
|
Tip For best results, initiate chat on the files or code changes. @HashEngineering Thank you for the clarification! Reviewing the changes in light of the PR description makes the implementation strategy much clearer. The changes are indeed well-focused on the two stated objectives: ✅ Crash Prevention During Fragment/Dialog RecreationThe changes systematically address recreation crashes through: Saved State Management:
Null Safety Improvements:
Process Death Scenario Handling:
✅ Network Error HandlingEnhanced error management through: Network Error Detection:
Improved User Experience:
The implementation is comprehensive and addresses the root causes mentioned in your PR description. The combination of state persistence, null safety, and proper error handling should significantly improve app stability during fragment recreation and network-related scenarios. |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
A user had this crash that is reported 2 seconds after the app starts.The user was trying to purchase a gift card, but was getting SSL Handshake errors with purchaseGiftCard. Logs report many other connection problems.7 minutes later, the app is restarted by the user. 2 seconds later the crash happenedI am wondering if this PurchaseGiftCardConfirmDialog is being recreated by Android, but this dialog was created from the right place in the app, so the giftCardMerchant value is not initialized. My idea is based on the call stack and that 2 seconds had passed, so it would seem impossible to make an attempt to purchase a card.or is there another explanation?
The stack trace from this dialog when the user clicks on the "Continue" button on the gift card enter amount screen:
And then dismiss the dialog before going futher.This somewhat defeats the purpose of lateinit (we might as well be using a nullable here). A proper solution would be to dismiss any dialogs when the app goes into background, similar to how we do it for the lockscreen. But this might be a bit of work to get everything correct.
The problem, of course, is that even if we dismiss the dialog, the user will end up on an empty Buy screen with uninitialized merchant, this can lead to other issues if he starts pressing buttons.The REAL proper solution would be to save the state of the viewModel with SavedStateHandle and restore the state correctly when the app is restored from background. We do this in the Send screen iirk.
Related PR's and Dependencies
none
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores