fix: default image when nft to send has no image #580
Conversation
WalkthroughThe pull request introduces error handling improvements for NFT image loading across two Vue components. In Changes
Sequence DiagramsequenceDiagram
participant Component as NFT Select Component
participant Image as Image Resource
participant ErrorHandler as imageLoadError Method
Component->>Image: Attempt to load image
alt Image loads successfully
Image-->>Component: Render image
else Image fails to load
Image->>ErrorHandler: Trigger @error event
ErrorHandler-->>Component: Handle image loading failure
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/extension/src/providers/common/ui/send-transaction/nft-select-list/components/nft-select-list-item.vue (1)
4-4: Consider adding fallback image src and loading stateWhile the error handling is good, consider these improvements for better user experience:
- Add a default/fallback src attribute
- Add loading state indication
- Add alt text for accessibility
- <img :src="item.image" @error="imageLoadError" /> + <img + :src="item.image" + :alt="item.name" + @error="imageLoadError" + loading="lazy" + />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension/src/providers/common/ui/send-transaction/nft-select-list/components/nft-select-list-item.vue(2 hunks)packages/extension/src/providers/common/ui/send-transaction/send-nft-select.vue(2 hunks)
🔇 Additional comments (2)
packages/extension/src/providers/common/ui/send-transaction/send-nft-select.vue (1)
5-5:
Default image implementation is missing
The PR's objective is to "fix default image when nft to send has no image", but the implementation doesn't specify a default image source. The imageLoadError handler should set a fallback image when the NFT image fails to load.
Let's verify if a default image exists in the codebase:
Please ensure that the imageLoadError utility function sets an appropriate default image source. If you need help implementing this, I can provide guidance on adding a default image asset and updating the error handler.
packages/extension/src/providers/common/ui/send-transaction/nft-select-list/components/nft-select-list-item.vue (1)
29-29: Verify imageLoadError utility implementation
Let's ensure the utility properly handles the image error case.
✅ Verification successful
imageLoadError utility is properly implemented and widely used
The utility is correctly implemented in packages/extension/src/ui/action/utils/misc.ts and sets a fallback image when image loading fails. This is consistently used across multiple components in the codebase for handling failed image loads, including NFT images, token logos, and other assets. The implementation in the reviewed file follows the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of imageLoadError utility
rg -A 5 "export.*imageLoadError.*=" src/
Length of output: 87
Script:
#!/bin/bash
# Search for imageLoadError implementation in the entire repository
rg -A 5 "export.*imageLoadError.*=" packages/
# Also search for any usage of imageLoadError
rg "imageLoadError" packages/
Length of output: 3209
| @@ -2,7 +2,7 @@ | |||
| <div class="send-nft-select__wrap"> | |||
| <a class="send-nft-select" @click="$emit('toggleSelect', item.id !== '')"> | |||
| <div class="send-nft-select__image"> | |||
| <img v-if="item.image !== ''" :src="item.image" alt="" /> | |||
| <img :src="item.image" alt="" @error="imageLoadError" /> | |||
There was a problem hiding this comment.
Inconsistent image error handling detected
There are two <img> tags in the template with different approaches to handling missing images:
- The first image uses
@error="imageLoadError" - The second image uses
v-if="item.image !== ''"
This inconsistency could lead to different behaviors. Consider applying the same error handling approach to both images.
Apply this diff to make the handling consistent:
<img :src="item.image" alt="" @error="imageLoadError" />
...
<img
- v-if="item.image !== ''"
class="send-nft-select__view"
:src="item.image"
alt=""
+ @error="imageLoadError"
/>Also applies to: 31-35
| @@ -26,6 +26,7 @@ | |||
| <script setup lang="ts"> | |||
| import { PropType } from 'vue'; | |||
| import { NFTItemWithCollectionName } from '@/types/nft'; | |||
| import { imageLoadError } from '@/ui/action/utils/misc'; | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Prop validation can be improved but type definition is correct
Based on the codebase analysis, the NFTItemWithCollectionName interface extends NFTItem which has the required image property as a string. The type definition is correct and ensures type safety. However, the default empty object prop could be improved.
defineProps({
item: {
type: Object as PropType<NFTItemWithCollectionName>,
- default: () => ({}),
+ required: true
},
});The imageLoadError utility is correctly imported and used with the properly typed NFT interface.
🔗 Analysis chain
Verify NFT type definition and add prop validation
The default empty object for the item prop could lead to undefined behavior. Let's verify the type definition and add validation.
Consider adding required validation:
defineProps({
item: {
type: Object as PropType<NFTItemWithCollectionName>,
- default: () => ({}),
+ required: true,
+ validator: (value: NFTItemWithCollectionName) => {
+ return typeof value.image === 'string';
+ }
},
});Also applies to: 33-39
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NFT type definition for image property
ast-grep --pattern 'interface NFTItemWithCollectionName {
$$$
image: $_
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Let's try to find the type definition using ripgrep
rg "NFTItemWithCollectionName" -A 5 -B 5
# Also search for any similar interface definitions
rg "interface.*NFT.*Item" -A 5 -B 5
Length of output: 30415
Summary by CodeRabbit
New Features
Bug Fixes