fix(coins): unauthenticated coin list and chart issues#3138
Conversation
Co-authored-by: charl <charl@vanstaden.info>
|
Cursor Agent can help with this pull request. Just |
|
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements SDK-driven lazy initialization of price chart coins, updates coins_repo to select assets from activated or all SDK assets, adds 24h change fetching and uppercase symbol caching, and adjusts a UI text style to use bodyMedium with onSurface color. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as PriceChart UI
participant B as PriceChartBloc
participant SDK as KomodoDefiSdk
participant C as AppConfig
UI->>B: Request availableCoins
alt availableCoins empty
Note over B: Lazy init
B->>SDK: getAllAssets()
SDK-->>B: assets[]
B->>C: get excluded/testnet lists
B-->>B: Filter assets, map to CoinPriceInfo
B-->>UI: availableCoins (initialized)
else not empty
B-->>UI: availableCoins (cached)
end
sequenceDiagram
autonumber
participant U as Caller
participant R as CoinsRepo.fetchCurrentPrices
participant SDK as KomodoDefiSdk
participant PX as PriceProvider
participant Log as Logger
participant Cache as PriceCache
U->>R: fetchCurrentPrices()
R->>SDK: getActivatedAssets()
alt none or unavailable
R->>SDK: getAllAssets()
SDK-->>R: assets (all)
else available
SDK-->>R: assets (activated)
end
R-->>R: Filter exclusions + testnets -> validAssets
loop for each asset in validAssets
R->>PX: maybeFiatPrice(asset)
PX-->>R: price? (nullable)
opt if price available
R->>PX: priceChange24h(asset)
alt success
PX-->>R: change24h
else error
PX-->>R: error
R->>Log: warn(24h retrieval failed)
R-->>R: change24h = null
end
R-->>Cache: set(symbol.toUpperCase(), CexPrice{price, change24h, ...})
end
end
R-->>U: Completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with unauthenticated user experience by ensuring coin prices and chart functionality work properly. The changes address three main problems: missing price data in coin lists, empty chart selectors, and poor text visibility in dropdowns.
- Modified price fetching to include all available assets when no activated assets exist for unauthenticated users
- Enhanced chart selector to populate available coins from SDK assets when empty
- Fixed coin dropdown text legibility by using proper theme colors
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/bloc/coins_bloc/coins_repo.dart | Updates price fetching logic to include all SDK assets for unauthenticated users and normalizes cache keys |
| lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart | Populates chart selector with available coins from SDK when empty |
| lib/shared/widgets/coin_select_item_widget.dart | Fixes text color to use proper theme color for better legibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Visit the preview URL for this PR (updated for commit cc89eef): https://walletrc--pull-3138-merge-bfumakdv.web.app (expires Sun, 21 Sep 2025 16:05:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/bloc/coins_bloc/coins_repo.dart (1)
571-577: Bug: cache key case change will break existing reads.Prices are now written with an uppercase key, but multiple readers still fetch by the original-case configSymbol (e.g., _assetToCoinWithoutAddress at Line 206). This will make usdPrice null across the app.
Minimal fix: write both keys to preserve backwards compatibility.
- final symbolKey = asset.id.symbol.configSymbol.toUpperCase(); - _pricesCache[symbolKey] = CexPrice( - assetId: asset.id, - price: fiatPrice, - lastUpdated: DateTime.now(), - change24h: change24h, - ); + final symbol = asset.id.symbol.configSymbol; + final entry = CexPrice( + assetId: asset.id, + price: fiatPrice, + lastUpdated: DateTime.now(), + change24h: change24h, + ); + _pricesCache[symbol.toUpperCase()] = entry; // new canonical form + _pricesCache[symbol] = entry; // backward-compatFollow-up (optional): centralize a _symbolKey() helper and migrate all call sites in a dedicated cleanup.
♻️ Duplicate comments (1)
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart (1)
41-45: Avoid using 0.0 as a “data not loaded” sentinel for percentage.Hardcoding 0.0 can be mistaken for “no change.” Prefer a dedicated placeholder strategy (e.g., a constant like kPercentageNotAvailable = 0.0 with UI hinting) or make the field nullable if feasible. This mirrors a prior note on this topic.
🧹 Nitpick comments (5)
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart (1)
30-49: Populate availableCoins immediately (even on fetch failures).Currently availableCoins is only set in the success emit. If OHLC fetch fails, the selector can remain empty for unauthenticated users. Emit the populated map right after building it.
Apply:
try { - // Populate available coins for the selector from SDK assets if empty + // Populate available coins for the selector from SDK assets if empty Map<AssetId, CoinPriceInfo> fetchedCexCoins = state.availableCoins; if (fetchedCexCoins.isEmpty) { final Map<AssetId, Asset> allAssets = _sdk.assets.available; final entries = allAssets.values .where((asset) => !excludedAssetList.contains(asset.id.id)) .where((asset) => !asset.protocol.isTestnet) .map( (asset) => MapEntry( asset.id, CoinPriceInfo( ticker: asset.id.symbol.assetConfigId, selectedPeriodIncreasePercentage: 0.0, id: asset.id.id, name: asset.id.name, ), ), ); fetchedCexCoins = Map<AssetId, CoinPriceInfo>.fromEntries(entries); + // Ensure the selector is populated right away + emit(state.copyWith(availableCoins: fetchedCexCoins)); }lib/bloc/coins_bloc/coins_repo.dart (4)
128-131: Filtering excluded assets looks good.This aligns getKnownCoins with app-config exclusions.
Consider also filtering out testnet assets here for consistency with chart filtering.
139-141: Consistent exclusion in map helper.Mirrors the list variant correctly.
Same optional note about excluding testnets for parity with chart behavior.
556-582: Sequential per-asset requests will be slow; add bounded parallelism.Fetching prices and 24h change one-by-one will not scale when unauthenticated (all assets). Extract a helper and process in chunks (e.g., 8–16 concurrency) to reduce total latency without overloading providers.
Example (outline):
Future<void> _fetchAndCache(Asset asset) async { final symbol = asset.id.symbol.configSymbol; final now = DateTime.now(); final results = await Future.wait<Decimal?>([ _kdfSdk.marketData.maybeFiatPrice(asset.id), _kdfSdk.marketData.priceChange24h(asset.id).catchError((_) => null), ], eagerError: false); final fiatPrice = results[0]; final change24h = results[1]; if (fiatPrice == null) return; final entry = CexPrice(assetId: asset.id, price: fiatPrice, lastUpdated: now, change24h: change24h); _pricesCache[symbol.toUpperCase()] = entry; _pricesCache[symbol] = entry; // backward-compat } final assets = validAssets.toList(); const batch = 12; for (var i = 0; i < assets.length; i += batch) { final chunk = assets.sublist(i, (i + batch).clamp(0, assets.length)); await Future.wait(chunk.map(_fetchAndCache), eagerError: false); }
540-585: Return type can be non-nullable.fetchCurrentPrices always returns _pricesCache; consider Future<Map<String, CexPrice>> to simplify callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart(2 hunks)lib/bloc/coins_bloc/coins_repo.dart(2 hunks)lib/shared/widgets/coin_select_item_widget.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#3109
File: lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart:50-54
Timestamp: 2025-08-27T22:24:30.511Z
Learning: During SDK migration for CEX market data in Komodo Wallet, functionality like coin filtering for profit/loss charts was moved to the SDK. The app-level filtering was kept as a safety net until SDK behavior could be confirmed, particularly for test coin filtering.
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: The `excludedAssetList` from `app_config.dart` is used in the `_filterExcludedAssets` method in `coins_state.dart`. Since `coins_state.dart` is a part file of `coins_bloc.dart`, the import needs to be in the parent file even though it's not directly used there. In Dart, part files share the namespace and imports of their parent files.
Applied to files:
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: In the Komodo Wallet project, part files share imports with their parent files. The import for `app_config.dart` in `coins_bloc.dart` is necessary because the part file `coins_state.dart` uses `excludedAssetList` from that package.
Applied to files:
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart
📚 Learning: 2025-08-31T19:24:53.317Z
Learnt from: CR
PR: KomodoPlatform/komodo-wallet#0
File: AGENTS.md:0-0
Timestamp: 2025-08-31T19:24:53.317Z
Learning: For Komodo DeFi Flutter SDK repository, reference `/KDF_API_DOCUMENTATION.md` and applicable RPCs, data structures, and notes when implementing RPC-related features
Applied to files:
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart
📚 Learning: 2025-05-01T21:00:36.970Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:2-3
Timestamp: 2025-05-01T21:00:36.970Z
Learning: It's acceptable to use unconditional `dart:io` imports in the Komodo wallet codebase when the usage is guarded by `!kIsWeb` conditional checks that prevent the platform-specific code from being executed in web environments.
Applied to files:
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart
⏰ 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). (6)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: validate_code_guidelines
🔇 Additional comments (4)
lib/shared/widgets/coin_select_item_widget.dart (1)
94-96: Good fix for legibility with onSurface.Using bodyMedium + onSurface makes the dropdown text readable in both themes. No further changes needed here.
lib/bloc/cex_market_data/price_chart/price_chart_bloc.dart (2)
32-37: Verify exclusion key type.excludedAssetList is checked against asset.id.id. Confirm excludedAssetList indeed contains AssetId.id values (not configId/ticker) to avoid false negatives.
59-66: Potential off-by-one in time buckets.List.generate uses floor division and excludes endAt. If the API expects an end-inclusive series, you may miss the last bucket. Consider ceil and/or explicitly appending endAt.
lib/bloc/coins_bloc/coins_repo.dart (1)
541-549: Asset target selection logic: sensible defaulting.Preferring activated assets when signed in reduces load; falling back to all assets enables unauthenticated prices.
If all-assets is large, confirm provider rate limits are respected and UX remains acceptable.
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, please upgrade to Bugbot Pro by visiting the Cursor dashboard. Your first 14 days will be free!
| ); | ||
| final change24h = price?.change24h ?? Decimal.zero; | ||
| final change24h = | ||
| await _sdk.marketData.priceChange24h(coin.id) ?? Decimal.zero; |
There was a problem hiding this comment.
Bug: Direct SDK Call Without Error Handling
The _calculateTotalChange24h method now calls _sdk.marketData.priceChange24h directly within a loop. This call lacks exception handling, meaning if the SDK method throws an exception for any coin, the entire portfolio change calculation will fail. This is less resilient than the previous cached approach and inconsistent with error handling for the same SDK call elsewhere in the codebase.
takenagain
left a comment
takenagain
left a comment
There was a problem hiding this comment.
LGTM aside from two SDK-related issues that are out of scope for this PR,
- VIA -> USDT Binance request error (coin not supported).
- XRP and USDT activation failing with local debug builds
[1] KDF: 13 12:00:09, mm2_main::rpc::dispatcher:128] ERROR RPC error response: platform_coin_with_tokens:455] prelude:116] eth_with_token_activation:106] Unexpected platform protocol {"type":"ERC20","protocol_data":{"platform":"ETH","contract_address":"0xdAC17F958D2ee523a2206206994597C13D831ec7"}} for USDT-ERC20 js_primitives.dart:28 [1] KDF: 13 12:00:09, mm2_main::rpc::dispatcher:128] ERROR RPC error response: platform_coin_with_tokens:455] prelude:116] eth_with_token_activation:106] Unexpected platform protocol {"type":"ERC20","protocol_data":{"platform":"ETH","contract_address":"0x39fBBABf11738317a448031930706cd3e612e1B9"}} for XRP-ERC20 js_primitives.dart:28
same issue as reported in #3129 (comment) |
|
Confirmed prices and 24h change available in https://walletrc--pull-3138-merge-bfumakdv.web.app/wallet before login. Price info appears to arrive much faster than before after login for auto-enabled coins 🎉, though adding extras still had some lag, including the case below where DGB/DGB-segwit were toggled to be activated at the same time, and despite sharing the same price, only one is displayed This may be related After second login, the above error was not shown (it seems to occur when entering a coin page before its ready, putting the coin in a broken state), though both LTC and DGB were a little slower to show pricing info than their segwit equivalents. For context, both non-segwit variants had zero balance, and the segwit variants were funded. |
CharlVS
changed the title
Co-authored-by: charl <charl@vanstaden.info>
CharlVS
deleted the
cursor/fix-unauthenticated-coin-list-and-chart-issues-a7d0
branch
Fix unauthenticated coins list not showing prices and empty chart selector, and improve coin dropdown text legibility.
The unauthenticated coins list was not displaying prices or 24h changes because
CoinsRepo.fetchCurrentPricesonly fetched prices for activated assets. The charts' coin selector was empty when unauthenticated becausePriceChartBloc.availableCoinswas not being populated. The coin dropdown text was illegible due to an incorrect text color.Summary by CodeRabbit
New Features
Bug Fixes
Style