fix(nft): add IPFS gateway resolution, retry, and fallback to improve NFT image loading#3020
fix(nft): add IPFS gateway resolution, retry, and fallback to improve NFT image loading#3020
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
circumvent "NoSuchCoin" error when re-enabling the coin in the same session.
…activate-parent-coins-for-nfts
…activate-parent-coins-for-nfts
|
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 update introduces a robust IPFS gateway management and fallback mechanism for NFT image and video loading. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as NftImage Widget
participant Bloc as NftImageBloc
participant Gateway as IpfsGatewayManager
UI->>Bloc: NftImageLoadRequested(imageUrl)
Bloc->>Gateway: _generateAllUrls(imageUrl)
Gateway-->>Bloc: [url1, url2, ...]
Bloc->>Gateway: findWorkingUrl([url1, url2, ...])
Gateway->>Gateway: Test url1, url2, ... (skip failed)
Gateway-->>Bloc: workingUrl or null
alt workingUrl found
Bloc->>UI: emit Loaded(workingUrl)
else no workingUrl
Bloc->>UI: emit Exhausted(error)
end
UI->>Bloc: NftImageLoadFailed(failedUrl)
Bloc->>Gateway: logGatewayAttempt(failedUrl, false)
Bloc->>Gateway: find next working URL
Gateway-->>Bloc: nextWorkingUrl or null
alt nextWorkingUrl found
Bloc->>UI: emit Retrying(nextWorkingUrl)
else exhausted
Bloc->>UI: emit Exhausted(error)
end
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 (
|
|
Visit the preview URL for this PR (updated for commit 7db078c): https://walletrc--pull-3020-merge-92xu8slr.web.app (expires Mon, 11 Aug 2025 13:29:17 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
gateway pattern matches subdomain due to loose alphanumeric requirement
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive IPFS gateway resolution, retry, and fallback mechanisms to improve NFT image loading reliability. The changes implement a sophisticated system for handling multiple IPFS gateways, circuit breaker patterns, and seamless URL fallbacks when NFT images fail to load.
Key changes:
- New
IpfsGatewayManagerutility for IPFS URL resolution and gateway management with circuit breaker pattern NftImageBlocfor state management of image loading with automatic fallback between multiple gateways- Enhanced
NftImagewidget with automatic retry and fallback capabilities for both images and videos - Comprehensive test coverage for the new IPFS gateway management functionality
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
lib/shared/utils/ipfs_gateway_manager.dart |
Core IPFS gateway management utility with URL parsing, fallback logic, and circuit breaker pattern |
lib/shared/constants/ipfs_constants.dart |
Configuration constants for IPFS gateways and timeout settings |
lib/bloc/nft_image/nft_image_bloc.dart |
BLoC for managing NFT image loading state with automatic fallbacks |
lib/bloc/nft_image/nft_image_state.dart |
State definitions for image loading with fallback mechanisms |
lib/bloc/nft_image/nft_image_event.dart |
Events for image loading lifecycle management |
lib/views/nfts/common/widgets/nft_image.dart |
Enhanced NFT image widget with automatic retry and fallback support |
lib/model/nft.dart |
Simplified to return raw URLs, delegating normalization to the bloc |
lib/bloc/app_bloc_root.dart |
Added IpfsGatewayManager as a repository provider |
test_units/tests/utils/ipfs_gateway_manager_test.dart |
Comprehensive test suite for IPFS gateway manager functionality |
test_units/main.dart |
Added test registration for IPFS gateway manager tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/views/nfts/nft_transactions/common/widgets/nft_txn_media.dart (1)
29-32: Rename the local field for consistencyThe widget property is still called
imagePath, yet it’s now forwarded asimageUrl. Renaming keeps the API self-documenting and avoids future confusion.- final String? imagePath; + final String? imageUrl; … - required this.imagePath, + required this.imageUrl, … - child: NftImage(imageUrl: imagePath), + child: NftImage(imageUrl: imageUrl),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/bloc/app_bloc_root.dart(2 hunks)lib/bloc/nft_image/nft_image_bloc.dart(1 hunks)lib/bloc/nft_image/nft_image_event.dart(1 hunks)lib/bloc/nft_image/nft_image_state.dart(1 hunks)lib/model/nft.dart(1 hunks)lib/shared/constants/ipfs_constants.dart(1 hunks)lib/shared/utils/ipfs_gateway_manager.dart(1 hunks)lib/views/nfts/common/widgets/nft_image.dart(2 hunks)lib/views/nfts/details_page/desktop/nft_details_page_desktop.dart(1 hunks)lib/views/nfts/details_page/mobile/nft_details_page_mobile.dart(2 hunks)lib/views/nfts/details_page/withdraw/nft_withdraw_success.dart(1 hunks)lib/views/nfts/nft_list/nft_list_item.dart(1 hunks)lib/views/nfts/nft_transactions/common/widgets/nft_txn_media.dart(1 hunks)test_units/main.dart(2 hunks)test_units/tests/utils/ipfs_gateway_manager_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
test_units/main.dart (5)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/bloc/app_bloc_root.dart (2)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
test_units/tests/utils/ipfs_gateway_manager_test.dart (3)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/bloc/nft_image/nft_image_state.dart (1)
Learnt from: takenagain
PR: #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.
lib/shared/utils/ipfs_gateway_manager.dart (1)
Learnt from: takenagain
PR: #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.
lib/views/nfts/common/widgets/nft_image.dart (3)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/bloc/nft_image/nft_image_bloc.dart (2)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
⏰ 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). (4)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
🔇 Additional comments (27)
lib/shared/constants/ipfs_constants.dart (1)
7-13: Verify gateway style alignment withIpfsGatewayManagerURL builderAll listed gateways are path-style (
…/ipfs/). IfIpfsGatewayManagerever switches between path- and sub-domain-style resolution, keeping a mixed list will break URL generation. Please double-check that the normalization logic expects path-style gateways only, or split the constants into two explicit lists.lib/views/nfts/details_page/desktop/nft_details_page_desktop.dart (1)
36-37: Renamed parameter looks correct
NftImage(imageUrl: …)matches the new constructor. No further action required.lib/views/nfts/details_page/withdraw/nft_withdraw_success.dart (1)
71-76: Parameter update confirmed
NftImage(imageUrl: nft.imageUrl)aligns with the refactored widget API.lib/views/nfts/nft_list/nft_list_item.dart (1)
73-76: Parameter update confirmed
NftImage(imageUrl: widget.nft.imageUrl)aligns with the new constructor. No issues spotted.test_units/main.dart (1)
36-36: LGTM! Proper test integration following established patterns.The new IPFS gateway test is correctly integrated into the centralized test runner, following the project's established pattern of defining tests in individual files but executing them through the main test entry point.
Also applies to: 76-76
lib/views/nfts/details_page/mobile/nft_details_page_mobile.dart (1)
102-102: LGTM! Parameter name updates maintain consistency.The parameter renames from
imagePathtoimageUrlcorrectly align with the refactoredNftImagewidget signature and support the new bloc-based image loading approach.Also applies to: 156-156
lib/bloc/app_bloc_root.dart (2)
69-69: LGTM! Clean import addition.The import for
IpfsGatewayManageris properly placed with other utility imports.
156-158: Excellent dependency injection setup with clear documentation.The
IpfsGatewayManageris correctly positioned near the root of the widget tree to enable app-wide access to the shared instance. The explanatory comment clearly documents the rationale for keeping the in-memory cache of failing URLs at this level, which is crucial for the circuit breaker functionality.lib/model/nft.dart (1)
78-78: Excellent architectural improvement with clear separation of concerns.Simplifying the
imageUrlgetter to return raw data while delegating URL normalization and fallback handling to the bloc layer is a sound design decision. This enables theIpfsGatewayManagerandNftImageBlocto implement sophisticated retry and fallback mechanisms while keeping the model focused on data representation.test_units/tests/utils/ipfs_gateway_manager_test.dart (13)
6-8: LGTM! Proper test structure following project conventions.The main function correctly calls the test function, following the established pattern in the Komodo Wallet project where individual test files are executed through a central test runner.
10-16: Excellent test setup with proper initialization.The test group structure and setUp method provide clean initialization for each test case, ensuring test isolation.
18-61: Comprehensive constructor and configuration testing.The tests thoroughly cover default gateway selection, custom gateway configuration, and failure cooldown settings. The platform-specific testing with
kIsWebensures correct behavior across different environments.
63-120: Thorough IPFS URL detection coverage.The test suite covers all IPFS URL formats including protocol URLs (
ipfs://), gateway format URLs, subdomain format URLs, and properly handles edge cases like null/empty URLs and non-IPFS URLs.
122-191: Excellent CID extraction testing with comprehensive format coverage.Tests properly validate CID and path extraction from various IPFS URL formats including protocol, gateway, and subdomain formats. The case-insensitive testing is particularly valuable for real-world robustness.
193-225: Solid gateway URL generation testing.Tests ensure proper URL generation for both IPFS and non-IPFS content, with appropriate handling of edge cases and primary gateway selection.
227-251: Good URL normalization testing.Tests validate that different IPFS URL formats are properly normalized to the preferred gateway format while preserving paths.
253-279: Well-designed fallback mechanism testing.Tests ensure proper fallback behavior between gateways and appropriate handling of edge cases when no more fallbacks are available.
281-328: Excellent circuit breaker and failure tracking tests.The comprehensive testing of failure tracking, cooldown periods, and reliable URL filtering ensures the circuit breaker functionality works as expected. The async test for cooldown expiration is particularly well-implemented.
330-346: Good gateway support validation testing.Tests ensure proper validation of gateway support for different platforms.
348-396: Comprehensive edge case and error handling testing.The extensive testing of malformed URLs, long URLs, and special characters ensures robust error handling. The graceful handling validation is crucial for production stability.
398-475: Valuable real-world example testing.Testing with actual NFT metadata URLs, popular IPFS services, and various URL types provides confidence that the implementation will work with real-world data. The comprehensive coverage of both valid and invalid URLs is excellent.
477-509: Good performance and logging test coverage.Tests ensure that logging functionality works correctly for both successful and failed gateway attempts without throwing exceptions.
lib/bloc/nft_image/nft_image_event.dart (1)
1-63: Well-structured event classes for NFT image loading.The event hierarchy is clean and follows Bloc best practices. All events properly extend Equatable with correct props implementation for value equality.
lib/views/nfts/common/widgets/nft_image.dart (1)
1-287: Excellent refactoring to Bloc pattern for robust image loading.The implementation properly integrates the NftImageBloc for fallback support and maintains clean separation of concerns between image and video handling widgets.
lib/bloc/nft_image/nft_image_bloc.dart (2)
32-39: Consider fallback for servers that don't support HEAD requests.Some servers or IPFS gateways might not properly support HEAD requests or return different status codes.
Consider adding a fallback to GET requests with minimal data fetching if HEAD fails, or document which gateways are known to support HEAD requests properly.
11-253: Well-architected BLoC with comprehensive fallback logic.The implementation properly manages state transitions, handles timer cleanup, and integrates circuit breaker pattern through IpfsGatewayManager. The retry logic with configurable attempts and timeouts provides good resilience.
lib/shared/utils/ipfs_gateway_manager.dart (1)
4-197: Comprehensive IPFS gateway management with circuit breaker pattern.The implementation provides robust URL normalization, platform-specific gateway selection, and failure tracking with cooldown. The regex patterns correctly handle various IPFS URL formats.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test_units/tests/utils/ipfs_gateway_manager_test.dart (1)
395-395: The base64 data URL is very long and makes the test code hard to read.Consider extracting it to a constant at the top of the file for better readability.
+ // At the top of testIpfsGatewayManager function + const base64ImageUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/5+hHgAHggJ/PchI7wAAAABJRU5ErkJggg=='; + // In the test - 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/5+hHgAHggJ/PchI7wAAAABJRU5ErkJggg==', + base64ImageUrl,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/bloc/nft_image/nft_image_bloc.dart(1 hunks)lib/shared/utils/ipfs_gateway_manager.dart(1 hunks)lib/views/nfts/common/widgets/nft_image.dart(2 hunks)test_units/tests/utils/ipfs_gateway_manager_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
test_units/tests/utils/ipfs_gateway_manager_test.dart (3)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/views/nfts/common/widgets/nft_image.dart (3)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/shared/utils/ipfs_gateway_manager.dart (3)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #2608
File: lib/views/fiat/webview_dialog.dart:50-55
Timestamp: 2025-05-01T21:00:56.962Z
Learning: kIsWasm is a valid constant in Flutter 3.22+ (released May 2024) that detects if an application is running on WebAssembly. It complements kIsWeb for more precise platform detection.
Learnt from: takenagain
PR: #2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:208-225
Timestamp: 2025-05-01T20:56:36.180Z
Learning: The kIsWasm constant is available in Flutter version 3.22+ (released May 2024) and is used to detect if an application is running on the WebAssembly backend. When writing platform-specific code, use both kIsWeb and kIsWasm checks to properly handle both JavaScript and WebAssembly compilation targets.
lib/bloc/nft_image/nft_image_bloc.dart (2)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
⏰ 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). (3)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
- GitHub Check: Build Mobile (Android)
🔇 Additional comments (5)
test_units/tests/utils/ipfs_gateway_manager_test.dart (1)
10-465: Excellent test coverage!The test suite comprehensively covers all aspects of the
IpfsGatewayManagerincluding initialization, URL detection, extraction, normalization, failure tracking with cooldown, platform-specific behavior, edge cases, and real-world scenarios. The tests are well-organized and thorough.lib/views/nfts/common/widgets/nft_image.dart (1)
176-270: Well-implemented video player with proper lifecycle management!The deferred initialization of
VideoPlayerControlleruntil a valid URL is available properly addresses the previous concern about empty URI initialization. The implementation correctly handles URL changes, disposes resources, and manages state transitions.lib/bloc/nft_image/nft_image_bloc.dart (1)
10-231: Robust Bloc implementation with excellent error handling!The
NftImageBlocis well-structured with:
- Proper dependency injection of
IpfsGatewayManager- Comprehensive error handling and retry logic with configurable limits
- Clean state transitions and timer management
- Appropriate resource cleanup in the
close()methodThe fallback mechanism through multiple URLs with circuit breaker pattern is a great approach for improving NFT media loading reliability.
lib/shared/utils/ipfs_gateway_manager.dart (2)
79-110: Excellent IPFS URL extraction logic!The
_extractContentIdmethod comprehensively handles all IPFS URL formats:
- Protocol format (ipfs://)
- Gateway format with proper pattern precedence
- Subdomain format with path preservation
- Generic /ipfs/ path detection
- Case-insensitive matching throughout
The implementation correctly prioritizes gateway pattern matching before subdomain to avoid conflicts.
157-190: Well-implemented circuit breaker pattern!The failure tracking mechanism effectively implements a circuit breaker pattern with:
- Thread-safe operations using mutex protection
- Configurable cooldown periods for failed URLs
- Automatic cleanup of expired failures
- Efficient filtering of reliable gateways
This approach prevents repeated attempts on failing gateways while allowing recovery after the cooldown period.
…nto bugfix/nft-image-loading
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
Errors which prompted issue for PR no longer seen in app, but seems now that though image may load initially, it falls away once you start to navigate.
vokoscreenNG-2025-07-31_04-43-00.mp4
* fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The issue with the disappearing image should be resolved. @smk762 could you give it another spin to confirm that everything looks good? |
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
LGTM thanks. Disappearing image issue resolved 🍻
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix impossible nested condition logic * handle uninitialized logger exceptions gratefully * caches `typeName` to avoid unneccesary `getCoinTypeName` calls * adds caching for `isParent` * set parents as native; use cached coin values * special case to cover ARB * chore: roll SDK (#3036) Roll SDK for critical KMD API pricing fix * chore: remove FEEDBACK_TEST_URL references * fix(ui): close dropdown on window resize * Fix null safety issues in UiDropdown widget Co-authored-by: charl <charl@vanstaden.info> * fix(build): correct env vars passing to Docker and Dart via --dart-define (#3037) * refactor: simplify Docker env vars and build command logic - Remove unnecessary environment variables from Docker container - Keep only GITHUB_API_PUBLIC_READONLY_TOKEN as env var - Replace hardcoded build logic with conditional credential handling - Add proper validation for Trello and Cloudflare feedback services - Use dart-define flags instead of environment variables for build configs - Add warning messages for incomplete credential sets * fix executable rights on build.sh * remove the redundant FEEDBACK_TEST_URL, as it will no longer be used in the GUI * swap BUILD_COMMAND and BUILD_CMD (important) BUILD_CMD, which is built dynamically with --dart-define= arguments, should be called last (not during the assets-fetch stage) to ensure the app is actually built with the required defines and that all String.fromEnvironment(...) statements in the Dart code work as expected. * fix(nft): add IPFS gateway resolution, retry, and fallback to improve NFT image loading (#3020) * feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function * feat(nft): add nft image bloc for url resolution, fallbacks and retry * fix(ipfs-gateway-manager): move gateway match above subdomain gateway pattern matches subdomain due to loose alphanumeric requirement * test: add unit tests for ipfs url normaliser and fix case sensitivity * refactor(nft-image): remove dead code & move url testing out of bloc * fix(nft): delegate IpfsGatewayManager disposal to root provider (#3047) * fix: delegate IpfsGatewayManager disposal to root provider * refactor: remove unnecessary line break Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(nft): reset NFT image on url change (#3046) * fix: reset NFT image on url change * refactor(nft-image): use ValueKey instead of Key Using Key(imageUrl!) creates a new Key object on every build. Consider using ValueKey(imageUrl!) instead, which is more semantically appropriate for value-based keys and may have better performance characteristics. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-image): use ValueKey instead of Key Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * style: apply updated dart formatting to modified files * fix(coins): filter excluded and testnet coins out of price updates * fix(nft-image): reduce the number of success events fired --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix(build): add --no-web-resources-cdn to the web build in build.sh (#3055) * Remove .dgph build artifacts from iOS project (#3058) * remove dgph binaries * Ignore .dgph build artifacts in iOS * fix arb parent ticker * include qtum parent * chore(sdk): bump submodule to 170aab4 for parent display name suffix Pulls in AssetId.displayName and CoinSubClass.tokenStandardSuffix * feat(ui): disambiguate parent chain names via SDK displayName - Use AssetId.displayName for top-level assets (SDK)\n- Update UI to show e.g. Ethereum (ARB20) for parents\n- Keep protocol pill consistent using coin.typeName (NATIVE for parents)\n- Search compares against displayName\n\nRefs: #2988 (comment) --------- Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: charl <charl@vanstaden.info> Co-authored-by: DeckerSU <deckersu@protonmail.com> Co-authored-by: Francois <takenagain@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests