dispose of scrollbars thoughtfully #3008
Conversation
|
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. 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 WalkthroughThis set of changes refactors several wallet UI components, replacing and removing coin list item widgets, and modularizing fiat price and change displays. Some widgets are converted from stateless to stateful to manage scroll controllers. Error handling for 24h price change retrieval is improved, and a new extension method provides cached 24h change access. The code also introduces more robust state management and modularization in coin details display. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Bloc
participant SDK
UI->>Bloc: Request 24h price change (lastKnown24hChange)
Bloc->>Bloc: Retrieve cached 24h change for coin
Bloc-->>UI: Return 24h change value
UI->>SDK: Request current USD price
SDK-->>UI: Return USD price
UI->>Bloc: Listen for CoinsBloc state updates
Bloc-->>UI: Notify on 24h change update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
* feat(ui): show loading skeleton for coin addresses * Reduce address placeholder items from 3 to 1
There was a problem hiding this comment.
Pull Request Overview
This PR addresses memory leaks by properly managing ScrollController lifecycle in Flutter widgets. It converts StatelessWidget classes to StatefulWidget to enable proper disposal of ScrollController instances that were previously being created inline without disposal.
- Convert StatelessWidget classes to StatefulWidget to manage ScrollController lifecycle
- Add proper initialization and disposal of ScrollController instances
- Refactor inline children creation to avoid repeated computation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| expandable_coin_list_item.dart | Refactors children creation to avoid repeated computation in build method |
| market_maker_bot_form.dart | Converts layout widgets to StatefulWidget and adds proper ScrollController disposal |
| maker_form_layout.dart | Converts layout widgets to StatefulWidget and adds proper ScrollController disposal |
| ), | ||
| ) | ||
| .toList() | ||
| : [SkeletonListTile()]; |
There was a problem hiding this comment.
Creating a new list with SkeletonListTile() on every build can impact performance. Consider making this a static constant or caching it as a final variable to avoid repeated object creation.
| : [SkeletonListTile()]; | |
| : [_skeletonListTile]; |
There was a problem hiding this comment.
For readers (CC @smk762): This isn't applicable. Flutter's widget tree handles it all automagically as long as you are not using widget helper methods. If you really want to be extra safe, you can specify the key parameter, but this is usually only necessary when you have multiple sibling widgets of the same type.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
♻️ Duplicate comments (1)
lib/views/wallet/wallet_page/common/expandable_coin_list_item.dart (1)
79-91: UX improvement but performance concern remains.Good UX improvement by showing a skeleton when addresses aren't available. However, creating
SkeletonListTile()on every build impacts performance.Apply this diff to cache the skeleton widget:
+ static const _skeletonListTile = SkeletonListTile(); + @override Widget build(BuildContext context) { final hasAddresses = widget.pubkeys?.keys.isNotEmpty ?? false; final sortedAddresses = hasAddresses ? (List.of(widget.pubkeys!.keys) ..sort((a, b) => b.balance.spendable.compareTo(a.balance.spendable))) : null; final children = sortedAddresses != null ? sortedAddresses .map( (pubkey) => _AddressRow( pubkey: pubkey, coin: widget.coin, isSwapAddress: pubkey == sortedAddresses.first, onTap: widget.onTap, onCopy: () => copyToClipBoard(context, pubkey.address), ), ) .toList() - : [SkeletonListTile()]; + : [_skeletonListTile];
🧹 Nitpick comments (1)
lib/shared/utils/extensions/legacy_coin_migration_extensions.dart (1)
64-69: Consider adding error handling for missing CoinsBlocThe method assumes
CoinsBlocis always available in the context. Consider wrapping in a try-catch or checking if the bloc exists to prevent runtime errors./// Get cached 24hr change from CoinsBloc state /// This bridges the gap until SDK provides cached 24hr data double? lastKnown24hChange(BuildContext context) { - return context.read<CoinsBloc>().state.get24hChangeForAsset(id); + try { + return context.read<CoinsBloc>().state.get24hChangeForAsset(id); + } catch (e) { + // CoinsBloc not found in context + return null; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/bloc/coins_bloc/coins_repo.dart(1 hunks)lib/shared/utils/extensions/legacy_coin_migration_extensions.dart(2 hunks)lib/shared/widgets/coin_fiat_change.dart(0 hunks)lib/shared/widgets/coin_fiat_price.dart(0 hunks)lib/views/dex/simple/form/maker/maker_form_layout.dart(4 hunks)lib/views/market_maker_bot/market_maker_bot_form.dart(4 hunks)lib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart(3 hunks)lib/views/wallet/wallet_page/common/coin_list_item.dart(0 hunks)lib/views/wallet/wallet_page/common/coin_list_item_desktop.dart(0 hunks)lib/views/wallet/wallet_page/common/coin_list_item_mobile.dart(0 hunks)lib/views/wallet/wallet_page/common/expandable_coin_list_item.dart(2 hunks)
💤 Files with no reviewable changes (5)
- lib/shared/widgets/coin_fiat_price.dart
- lib/shared/widgets/coin_fiat_change.dart
- lib/views/wallet/wallet_page/common/coin_list_item.dart
- lib/views/wallet/wallet_page/common/coin_list_item_desktop.dart
- lib/views/wallet/wallet_page/common/coin_list_item_mobile.dart
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2976
File: lib/bloc/coins_manager/coins_manager_bloc.dart:82-83
Timestamp: 2025-07-23T09:31:17.738Z
Learning: In the CoinsManagerBloc, `state.selectedCoins` is used separately from `state.coins` to indicate whether a coin is enabled or not in the UI. The order of Set deduplication when merging coin lists doesn't affect UI behavior because selection state is tracked independently from the coin list used for filtering and sorting.
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.
📚 Learning: in the komodo wallet project, part files share imports with their parent files. the import for `app_...
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/views/wallet/wallet_page/common/expandable_coin_list_item.dartlib/shared/utils/extensions/legacy_coin_migration_extensions.dartlib/bloc/coins_bloc/coins_repo.dartlib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart
📚 Learning: the `excludedassetlist` from `app_config.dart` is used in the `_filterexcludedassets` method in `coi...
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/views/wallet/wallet_page/common/expandable_coin_list_item.dartlib/shared/utils/extensions/legacy_coin_migration_extensions.dartlib/bloc/coins_bloc/coins_repo.dartlib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart
📚 Learning: in the coinsmanagerbloc, `state.selectedcoins` is used separately from `state.coins` to indicate whe...
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2976
File: lib/bloc/coins_manager/coins_manager_bloc.dart:82-83
Timestamp: 2025-07-23T09:31:17.738Z
Learning: In the CoinsManagerBloc, `state.selectedCoins` is used separately from `state.coins` to indicate whether a coin is enabled or not in the UI. The order of Set deduplication when merging coin lists doesn't affect UI behavior because selection state is tracked independently from the coin list used for filtering and sorting.
Applied to files:
lib/views/wallet/wallet_page/common/expandable_coin_list_item.dartlib/shared/utils/extensions/legacy_coin_migration_extensions.dartlib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart
📚 Learning: it's acceptable to use unconditional `dart:io` imports in the komodo wallet codebase when the usage ...
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/shared/utils/extensions/legacy_coin_migration_extensions.dartlib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart
📚 Learning: in the komodo wallet project, test functions are defined in individual files under `test_units/tests...
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2611
File: test_units/tests/system_health/http_head_time_provider_test.dart:8-24
Timestamp: 2025-05-08T19:05:13.083Z
Learning: In the Komodo Wallet project, test functions are defined in individual files under `test_units/tests/` but are executed through the central entry point in `test_units/main.dart`, so individual test files don't need their own `main()` function.
Applied to files:
lib/shared/utils/extensions/legacy_coin_migration_extensions.dart
📚 Learning: in the komodo wallet project, test files are structured to define test functions that are called fro...
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2611
File: test_units/tests/system_health/system_clock_repository_test.dart:7-8
Timestamp: 2025-05-08T19:07:13.442Z
Learning: In the Komodo Wallet project, test files are structured to define test functions that are called from a central test runner in test_units/main.dart, rather than each test file having its own main() function.
Applied to files:
lib/shared/utils/extensions/legacy_coin_migration_extensions.dart
🔇 Additional comments (12)
lib/views/market_maker_bot/market_maker_bot_form.dart (4)
58-82: LGTM! Proper ScrollController lifecycle management implemented.The conversion from StatelessWidget to StatefulWidget correctly addresses memory leak prevention by managing ScrollController lifecycle. The implementation follows Flutter best practices with late final fields, proper initialization in
initState(), and disposal indispose().
97-101: LGTM! ScrollControllers correctly integrated.The persistent ScrollControllers are properly integrated into the scrollable widgets. The shared controller between DexScrollbar and SingleChildScrollView ensures synchronized scrolling behavior.
Also applies to: 125-125
135-156: LGTM! Consistent ScrollController lifecycle management.The mobile layout conversion mirrors the desktop implementation with proper ScrollController lifecycle management. The pattern is consistent and correctly prevents memory leaks.
161-161: LGTM! ScrollController properly integrated.The persistent ScrollController is correctly integrated into the mobile layout's SingleChildScrollView.
lib/views/dex/simple/form/maker/maker_form_layout.dart (4)
104-128: LGTM! Consistent implementation across codebase.The ScrollController lifecycle management is identical to the market maker bot form, maintaining consistency across the codebase. This uniform approach ensures reliable memory leak prevention.
143-147: LGTM! ScrollControllers properly integrated.The ScrollControllers are correctly integrated following the same pattern as the market maker bot form, ensuring consistent scroll behavior across components.
Also applies to: 168-168
178-199: LGTM! Consistent mobile layout implementation.The mobile layout ScrollController management follows the same pattern as the market maker bot form, maintaining consistency and proper memory leak prevention across the codebase.
204-204: LGTM! ScrollController properly integrated.The ScrollController is correctly integrated into the mobile layout's SingleChildScrollView, consistent with the established pattern.
lib/bloc/coins_bloc/coins_repo.dart (1)
530-544: Good error handling improvementWrapping the 24h change retrieval in a try-catch block prevents a single coin's price change failure from disrupting the entire price update process. This is a good defensive programming practice.
lib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart (3)
47-93: Good modularization of UI componentsBreaking down the UI into separate widget classes improves code organization and maintainability. The widgets are properly stateless and focused on single responsibilities.
95-146: Efficient use of BlocBuilder with buildWhenThe
buildWhencondition ensures the widget only rebuilds when the 24h change for the specific coin changes, avoiding unnecessary rebuilds. This is a good performance optimization.
1-184: Clarify ScrollController disposal location
This file (coin_details_info_fiat.dart) doesn’t use anyScrollControllerorScrollbar, so no disposal is needed here. The PR’s “dispose of scrollbars thoughtfully” goal likely applies to widgets that create controllers inline and never dispose them. For example:
- lib/views/wallet/coin_details/rewards/kmd_rewards_info.dart
- lib/views/wallet/coin_details/transactions/transaction_details.dart
- lib/views/wallet/coin_details/transactions/transaction_table.dart
Were the intended scrollbar‐cleanup changes meant for those (or similar) files—by refactoring them into
StatefulWidgets with properdispose()—or was the focus solely on updating theDexScrollbarimplementation inkomodo_ui_kit? Please clarify where the disposal adjustments should land.
…into fix/scroll-controller-conflicts # Conflicts: # lib/bloc/coins_bloc/coins_repo.dart # lib/views/wallet/coin_details/coin_details_info/coin_details_info_fiat.dart
![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 3bcf3b5): https://walletrc--pull-3008-merge-3qu164sz.web.app (expires Wed, 13 Aug 2025 15:55:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
closing as stale |
* fix missing price and change data on coin page, sync with main wallet page * chore: remove unreferenced coin list and fiat widgets * refactor(coin-details-info-fiat): move widget builder functions to widgets https://stackoverflow.com/a/53234826 * adds Individual error handling for price and 24hr data sourcing * cache 24hr data from CoinsBlock until SDK offers it * fix layout issue (renderbox constraint violations) * dispose of scrollbars thoughtfully * Show skeleton for assets list addresses (#2990) * feat(ui): show loading skeleton for coin addresses * Reduce address placeholder items from 3 to 1 * fix: apply Cursor suggestions --------- Co-authored-by: Francois <[email protected]> Co-authored-by: Charl (Nitride) <[email protected]>
Resolves task 2 in #3006
This should avoid some memleaking
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Removed