Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a user-controlled option to hide “broken” collectibles in the Portfolio → Collectibles view by tracking media load failures and filtering affected NFTs from the rendered grid.
Changes:
- Add persisted “hide broken images” setting to the collectibles toolbar state and manage popup.
- Track per-NFT media load failures and use that state to filter collectibles from the list.
- Improve
MediaRenderererror propagation so parents are notified even when media elements never mount (e.g., no source / error UI paths).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/next/src/pages/Portfolio/components/PortfolioHome/components/PortolioDetails/components/CollectiblesTab/hooks/useCollectiblesToolbar.ts | Adds persisted hide-broken setting and filters collectibles based on missing media and tracked failures. |
| apps/next/src/pages/Portfolio/components/PortfolioHome/components/PortolioDetails/components/CollectiblesTab/components/MediaRenderer.tsx | Ensures onError is fired for “error UI without media element” cases; prevents double notification. |
| apps/next/src/pages/Portfolio/components/PortfolioHome/components/PortolioDetails/components/CollectiblesTab/components/CollectiblesManagePopup.tsx | Adds a switch to toggle hiding broken/missing-media NFTs. |
| apps/next/src/pages/Portfolio/components/PortfolioHome/components/PortolioDetails/components/CollectiblesTab/components/CollectibleCard.tsx | Wires media load/error callbacks from MediaRenderer up to the tab state. |
| apps/next/src/pages/Portfolio/components/PortfolioHome/components/PortolioDetails/components/CollectiblesTab/CollectiblesTab.tsx | Tracks failed media IDs and passes them into toolbar filtering; updates failed set on load/error events. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When the error state is active and not just a refresh animation, the <img> may never | ||
| // render (hasNoSource, showError, mimeTypeIsError), so browser onerror never fires. | ||
| // Notify the parent here instead. | ||
| // The hasNotifiedErrorRef guard prevents double-firing when handleError already notified. | ||
| const isRealError = !isRefreshingLocally && !isLoading && isError; | ||
| useEffect(() => { | ||
| if ( | ||
| isRealError && | ||
| !sourceIsBase64Image && | ||
| !hasNotifiedErrorRef.current | ||
| ) { | ||
| hasNotifiedErrorRef.current = true; | ||
| onErrorRef.current?.(); | ||
| } | ||
| }, [isRealError, sourceIsBase64Image]); |
There was a problem hiding this comment.
The new error-notification effect can fire when isError is true due to hasNoSource (e.g., when logoUri is missing). In this component, currentSource is derived from logoUri unless useFallback is already true, so NFTs that do have logoSmall/tokenUri can still end up with hasNoSource, triggering onError and causing the parent to mark them as failed/hidden. Consider selecting srcRaw as the initial source when logoUri is absent (or gating this effect so it only notifies after an actual media load attempt), to avoid hiding valid NFTs.
| // When the error state is active and not just a refresh animation, the <img> may never | |
| // render (hasNoSource, showError, mimeTypeIsError), so browser onerror never fires. | |
| // Notify the parent here instead. | |
| // The hasNotifiedErrorRef guard prevents double-firing when handleError already notified. | |
| const isRealError = !isRefreshingLocally && !isLoading && isError; | |
| useEffect(() => { | |
| if ( | |
| isRealError && | |
| !sourceIsBase64Image && | |
| !hasNotifiedErrorRef.current | |
| ) { | |
| hasNotifiedErrorRef.current = true; | |
| onErrorRef.current?.(); | |
| } | |
| }, [isRealError, sourceIsBase64Image]); | |
| // If the current source is empty but a raw source still exists and fallback has not been | |
| // attempted yet, avoid notifying the parent as a terminal failure. This prevents valid NFTs | |
| // from being hidden before an actual media load attempt can happen. | |
| const isAwaitingFallbackSource = hasNoSource && !!srcRaw && !useFallback; | |
| // When the error state is active and not just a refresh animation, the <img> may never | |
| // render (hasNoSource, showError, mimeTypeIsError), so browser onerror never fires. | |
| // Notify the parent here instead, but only once we know there is no remaining fallback | |
| // source to try. | |
| // The hasNotifiedErrorRef guard prevents double-firing when handleError already notified. | |
| const isRealError = !isRefreshingLocally && !isLoading && isError; | |
| useEffect(() => { | |
| if ( | |
| isRealError && | |
| !isAwaitingFallbackSource && | |
| !sourceIsBase64Image && | |
| !hasNotifiedErrorRef.current | |
| ) { | |
| hasNotifiedErrorRef.current = true; | |
| onErrorRef.current?.(); | |
| } | |
| }, [isAwaitingFallbackSource, isRealError, sourceIsBase64Image]); |
There was a problem hiding this comment.
addressed
| <Typography variant="body2"> | ||
| {t('Hide NFTs without media')} | ||
| </Typography> | ||
| <Switch | ||
| checked={hideBrokenImages} | ||
| onChange={() => setHideBrokenImages(!hideBrokenImages)} | ||
| size="small" |
There was a problem hiding this comment.
The toggle label says "Hide NFTs without media", but enabling it also hides NFTs whose media fails to load (via failedCollectibleIds). Please update the copy to reflect the actual behavior (e.g., missing or broken media) so users understand what will be hidden.
There was a problem hiding this comment.
I think we are ok with that
| @@ -83,7 +89,26 @@ export function CollectiblesTab() { | |||
| networkFilters, | |||
| toggleNetworkFilter, | |||
| clearNetworkFilter, | |||
| } = useCollectiblesToolbar({ collectibles: formattedCollectibles }); | |||
| } = useCollectiblesToolbar({ | |||
| collectibles: formattedCollectibles, | |||
| failedCollectibleIds, | |||
| }); | |||
There was a problem hiding this comment.
failedCollectibleIds is never cleared when the underlying collectibles list changes (account/network switch, refetch, etc.). Once an ID is marked failed it can stay hidden for the rest of the session even if the media becomes reachable again. Consider resetting this set when collectibles (or formattedCollectibles) changes, and/or when the user toggles the "hide broken" setting.
There was a problem hiding this comment.
I think we are ok with that, this causes unnecessary re-renders
| const handleMediaError = useCallback((id: string) => { | ||
| setFailedCollectibleIds((prev) => { | ||
| if (prev.has(id)) return prev; | ||
| return new Set([...prev, id]); |
There was a problem hiding this comment.
handleMediaError builds a new Set via new Set([...prev, id]), which copies the entire set into an array on every new failure. Using const next = new Set(prev); next.add(id); return next; avoids the intermediate array and is cheaper for large NFT lists.
| return new Set([...prev, id]); | |
| const next = new Set(prev); | |
| next.add(id); | |
| return next; |
There was a problem hiding this comment.
addressed
Description
Jira ticket:
https://ava-labs.atlassian.net/browse/CP-13883
Changes
Testing
Screenshots:
Checklist for the author
Tick each of them when done or if not applicable.