This repository was archived by the owner on Mar 2, 2026. It is now read-only.
1164-task-refactor-all-the-nft-screens-and-move-to-data-cache#1203
Draft
tombeckenham wants to merge 4 commits into
Draft
1164-task-refactor-all-the-nft-screens-and-move-to-data-cache#1203tombeckenham wants to merge 4 commits into
tombeckenham wants to merge 4 commits into
Conversation
- Introduced `triggerNftCollectionRefresh` function to refresh NFT collections. - Added `useSingleCollection` hook for fetching single NFT collections. - Removed unused `EditNFTAddress` component files. - Adjusted styles in `CollectionCard` and `NFTTab` components for better layout. - Updated `CollectionDetail` to utilize new refresh functionality. Closes #1164
PR SummaryRefactored NFT-related components and services to improve data caching and UI components. Introduced new collection card component, unified NFT tab view, and enhanced data fetching with pagination support. Improved type definitions and network state handling across components. Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (3)
- cae9c65: Supported loading the network
Closes #1164
-
f249e7e: Add NFT collection refresh functionality and clean up UI components
-
Introduced
triggerNftCollectionRefreshfunction to refresh NFT collections. -
Added
useSingleCollectionhook for fetching single NFT collections. -
Removed unused
EditNFTAddresscomponent files. -
Adjusted styles in
CollectionCardandNFTTabcomponents for better layout. -
Updated
CollectionDetailto utilize new refresh functionality.
Closes #1164
- d025d5f: Refactored Cadence NFT List view
Closes #1164
Files Processed (30)
- apps/extension/src/background/controller/provider/rpcFlow.ts (1 hunk)
- apps/extension/src/background/controller/wallet.ts (4 hunks)
- apps/extension/src/core/service/nft-evm.ts (3 hunks)
- apps/extension/src/core/service/nft.ts (5 hunks)
- apps/extension/src/core/service/openapi.ts (2 hunks)
- apps/extension/src/data-model/cache-data-keys.ts (5 hunks)
- apps/extension/src/data-model/user-data-keys.ts (1 hunk)
- apps/extension/src/ui/components/NFTs/EmptyStatus.tsx (from apps/extension/src/ui/views/EmptyStatus.tsx) (0 hunks)
- apps/extension/src/ui/components/NFTs/collection-card.tsx (1 hunk)
- apps/extension/src/ui/components/NetworkIndicator.tsx (1 hunk)
- apps/extension/src/ui/components/StorageUsageCard.tsx (1 hunk)
- apps/extension/src/ui/components/TokenLists/AddCustomEvmToken.tsx (1 hunk)
- apps/extension/src/ui/components/account-menu/MenuDrawer.tsx (2 hunks)
- apps/extension/src/ui/components/account/stories/account-listing.stories.tsx (2 hunks)
- apps/extension/src/ui/components/header/index.tsx (1 hunk)
- apps/extension/src/ui/components/news/NewsView.tsx (1 hunk)
- apps/extension/src/ui/hooks/use-account-hooks.ts (2 hunks)
- apps/extension/src/ui/hooks/use-coin-hooks.ts (2 hunks)
- apps/extension/src/ui/hooks/useCoinHook.ts (1 hunk)
- apps/extension/src/ui/hooks/useNetworkHook.ts (1 hunk)
- apps/extension/src/ui/hooks/useNftHook.ts (4 hunks)
- apps/extension/src/ui/views/Approval/components/Confirmation.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/Connect.tsx (2 hunks)
- apps/extension/src/ui/views/Approval/components/EthApproval/EthConfirm/DefaultBlock.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/EthApproval/EthSuggest/index.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/SignMessage.tsx (1 hunk)
- apps/extension/src/ui/views/Dashboard/dashboard-total.tsx (2 hunks)
- apps/extension/src/ui/views/Dashboard/wallet-tab.tsx (2 hunks)
- apps/extension/src/ui/views/Deposit/deposit.stories.tsx (3 hunks)
- apps/extension/src/ui/views/NFT/CollectionDetail.tsx (3 hunks)
Actionable Comments (1)
-
apps/extension/src/ui/views/Approval/components/EthApproval/EthConfirm/DefaultBlock.tsx [38-46]
possible bug: "Potential bug in UTF-8 text decoding logic"
Skipped Comments (2)
-
apps/extension/src/ui/views/NFT/CollectionDetail.tsx [79-88]
best practice: "Missing error handling in collection refresh operation"
-
apps/extension/src/ui/components/NFTs/collection-card.tsx [28-30]
possible issue: "Potential state mutation in navigation"
Comment on lines
38
to
46
| const decoder = new TextDecoder('utf-8', { fatal: true }); | ||
| let decodedText = ''; | ||
| try { | ||
| return decoder.decode(bytes); | ||
| decodedText = decoder.decode(bytes); | ||
| } catch (e) { | ||
| return hexString; | ||
| } | ||
| return decodedText; | ||
| }; |
There was a problem hiding this comment.
The UTF-8 decoding logic has a potential issue where decodedText is returned even if the decoding fails. The try-catch block should either return hexString immediately after successful decoding or move the return statement inside the try block. Current implementation could return an uninitialized decodedText variable.
Closes #1164
tombeckenham
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Files Processed (28)
- .cursor/rules/use-git-mv-for-moves.mdc (1 hunk)
- apps/extension/src/background/controller/provider/rpcFlow.ts (1 hunk)
- apps/extension/src/background/controller/wallet.ts (6 hunks)
- apps/extension/src/core/service/nft-evm.ts (2 hunks)
- apps/extension/src/core/service/nft.ts (8 hunks)
- apps/extension/src/core/service/openapi.ts (4 hunks)
- apps/extension/src/data-model/cache-data-keys.ts (4 hunks)
- apps/extension/src/data-model/user-data-keys.ts (1 hunk)
- apps/extension/src/ui/components/NFTs/collection-card.tsx (1 hunk)
- apps/extension/src/ui/components/NetworkIndicator.tsx (1 hunk)
- apps/extension/src/ui/components/StorageUsageCard.tsx (1 hunk)
- apps/extension/src/ui/components/TokenLists/AddCustomEvmToken.tsx (1 hunk)
- apps/extension/src/ui/components/account-menu/MenuDrawer.tsx (2 hunks)
- apps/extension/src/ui/components/account/stories/account-listing.stories.tsx (2 hunks)
- apps/extension/src/ui/components/header/index.tsx (2 hunks)
- apps/extension/src/ui/hooks/use-account-hooks.ts (2 hunks)
- apps/extension/src/ui/hooks/use-coin-hooks.ts (2 hunks)
- apps/extension/src/ui/hooks/use-data.ts (1 hunk)
- apps/extension/src/ui/hooks/useCoinHook.ts (1 hunk)
- apps/extension/src/ui/hooks/useNetworkHook.ts (1 hunk)
- apps/extension/src/ui/hooks/useNftHook.ts (12 hunks)
- apps/extension/src/ui/views/Approval/components/Confirmation.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/Connect.tsx (2 hunks)
- apps/extension/src/ui/views/Approval/components/EthApproval/EthConfirm/DefaultBlock.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/EthApproval/EthSuggest/index.tsx (1 hunk)
- apps/extension/src/ui/views/Approval/components/SignMessage.tsx (1 hunk)
- apps/extension/src/ui/views/Dashboard/dashboard-total.tsx (2 hunks)
- apps/extension/src/ui/views/Dashboard/wallet-tab.tsx (2 hunks)
Actionable Comments (1)
-
apps/extension/src/core/service/nft-evm.ts [98-108]
possible issue: "Potential infinite loop in NFT collection loading"
Skipped Comments (3)
-
apps/extension/src/ui/hooks/useNftHook.ts [205-208]
possible issue: "Loading state could get stuck in edge cases"
-
apps/extension/src/ui/hooks/useNftHook.ts [213-219]
best practice: "Missing error handling and loading state cleanup"
-
apps/extension/src/core/service/nft.ts [181-184]
maintainability: "Missing return type annotation in function signature"
Comment on lines
+98
to
+108
| // We can't run this in parallel as we don't know the total number of pages | ||
| // So we need to run it sequentially | ||
| let entireEvmCollectionNftItemList: EntireEvmCollectionNftItemsStore | undefined = undefined; | ||
| let offset = ''; | ||
| do { | ||
| const evmNftCollectionListPage = await this.loadEvmCollectionNftItemListPage( | ||
| network, | ||
| address, | ||
| collectionIdentifier, | ||
| offset | ||
| ); |
There was a problem hiding this comment.
Consider adding a timeout or maximum retry limit for the do-while loop. If the API keeps returning valid offsets but no actual NFTs, this could potentially run indefinitely.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Closes #1164
Related Issue
Closes #
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)