fix(coins-bloc): coins re-activating after being disabled by the user#2584
fix(coins-bloc): coins re-activating after being disabled by the user#2584
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant CB as CoinsBloc
participant KDF as _kdfSdk
participant Wallet as CurrentWallet
CB->>KDF: Fetch current wallet
KDF-->>CB: Return current wallet data
CB->>CB: Process coin operations based on wallet configuration
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/bloc/coins_bloc/coins_bloc.dart (2)
120-121: Consider short-circuiting when there's no wallet.The
currentWalletmay benull, yet the code continues into the switch block undercase null:. This can work as intended, but it might be clearer to explicitly short-circuit ifcurrentWalletis absent for maintainability.
267-269: Double-check for coin presence before logging.Similar to the previous block, you are force-unwrapping
currentWalletCoins[assetId]!. While likely safe in practice, a guard check or error message if the coin is missing can improve robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/bloc/coins_bloc/coins_bloc.dart(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Test web-app-macos
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: Build Desktop (macos)
- GitHub Check: unit_tests
- GitHub Check: validate_code_guidelines
🔇 Additional comments (12)
lib/bloc/coins_bloc/coins_bloc.dart (12)
233-235: Handle potential null wallet scenario.Currently, if
currentWalletisnull, the block to sync coins would be skipped. Consider confirming that you never expect to activate coins without a wallet. Otherwise, you may want to explicitly bail out.
251-257: Good early return approach.Returning immediately when there are no wallet coins to disable is a clean approach that avoids unnecessary processing.
258-266: Prevent possible runtime error from forced null unwrap.You’re force-unwrapping
currentWalletCoins[assetId]!. IfassetIdis ever missing (e.g., concurrency or missed edge case), a runtime error would occur. Consider a safety check or null-safe approach.
278-278: Improved logging message.Using
assetIdin the error log helps with triaging.
387-387: Asynchronous state emission looks correct.Emitting the result from
_prePopulateListWithActivatingCoinsensures that the state accurately reflects newly activating coins.
416-418: Correct method signature update.Changing
_prePopulateListWithActivatingCoinsto return aFuturealigns with the asynchronous wallet retrieval.
426-426: Enabling coins explicitly for the current wallet type.Assigning
enabledType: currentWallet?.config.typeensures the coin's enabled type matches the latest wallet context.
447-448: Straightforward login check.Retrieving the current wallet and checking if it's non-null is a clear way to determine if the user is logged in before activating a coin.
494-494: Fetching current wallet upon login.Grabbing
currentWalletat login time is consistent with the rest of the updated logic to ensure up-to-date wallet data.
516-516: Nicely combined suspended and unactivated coins.Adding the result of
_getUnactivatedWalletCoins()ensures you capture all coins that need activation in one pass.
523-524: Appropriate async retrieval.Shifting
_getUnactivatedWalletCoinsto aFutureis consistent with the rest of the asynchronous design.
526-526: Useful warning log.Logging a warning when there's no wallet gives clear feedback in case of unexpected states.
4855fe5 to
c3f522e
Compare
|
Visit the preview URL for this PR (updated for commit c3f522e): https://walletrc--pull-2584-merge-j9r0zc0d.web.app (expires Mon, 07 Apr 2025 11:22:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
AndrewDelaney
left a comment
There was a problem hiding this comment.
The updates are working well. Coin management is seemless, and I did not run into any issues enabling/disabling assets. Coins were added and removed from the lists as soon as I gave the action.
|
The Enable/Disable feature for coins is working as expected. I did not encounter any issues during testing. After disabling specific coins and logging back into the wallet, I observed that the disabled coins no longer appear on the Wallet page or under the graph, which is the expected behavior. However, please note that this also applies to the KMD coin. Once KMD is disabled and the user logs in again, it does not reappear on the Wallet page unless re-enabled. Additionally, although it falls outside the current testing scope, I noticed that the test coins (Doc & Marty) were not visible even when the 'Enable Test Coin' toggle was turned on in the settings. |
Fixes coins re-appearing in the coins list after being disabled (after a delay), as seen in #2526
Summary by CodeRabbit