Fix throwing SharedToggleItem on item without label #3486
Conversation
| @@ -25,7 +25,7 @@ export default function SharedToggleItem({ | |||
| <div className="text_wrap"> | |||
| <div className="thumbnail" role="img" /> | |||
| <label className="label ellipsis"> | |||
| {isProbablyEVMAddress(label) ? truncateAddress(label) : label} | |||
| {isProbablyEVMAddress(label) ? truncateAddress(label) : label ?? ""} | |||
There was a problem hiding this comment.
I think what we're missing here is updating the type of label to include the possibility of undefined (and same for isProbablyEVMAddress).
| @@ -8,7 +8,7 @@ import SharedToggleButton from "./SharedToggleButton" | |||
| export const STARS_GREY_URL = "./images/stars_grey.svg" | |||
|
|
|||
| type SharedToggleItemProps = { | |||
| label: string | |||
| label?: string | |||
There was a problem hiding this comment.
Non-blocking comment
I wonder if this is the correct place for the fix. 🤔 My point is that the problem is not with the SharedToggleItem component but with the undefined name value for collection. Therefore, I don't know whether we shouldn't fix this issue here. Because now the changes apply to the shared component what is a bit unclear is why the label can be optional. What I mean is that the thumbnailURL is already optional and with the optional label it reduces this component to a simple SharedToggleButton. Maybe we could add a comment for this?
There was a problem hiding this comment.
Tbh I'm now hesitant to even merging this fix, as I was not able to reproduce it and in theory the name should be replaced with empty string if needed while the collection is fetched from SH. Unless we are able to find a way to reproduce it 🤔
cc @michalinacienciala can you try to reproduce that problem with NFTs filters?
There was a problem hiding this comment.
I was able to reproduce it. A bit different steps than before: I reproduced it in Playwright tests when switching networks quickly (Ethereum -> Optymism -> Polygon -> Arbitrum) and then going straight away to NFTs filtering.
There was a problem hiding this comment.
Do we have this test already in the codebase? if not can you create a draft PR with it?
There was a problem hiding this comment.
Added temporary test in #3512.
|
Ok, I've found where the problem was, it is not actually because of NFTs, it happens because the account is not fully loaded and it has no name yet but we are not providing a fallback to address. Will close this PR and open a new one. |
### What Follow up for #3486 Add fallback from name to address in NFT filters. To avoid NFT filters throwing error when the account is not yet loaded let's add a fallback from name to address to put in the SharedToggle label. ### Testing - [x] install extension and throttle network so it is super slow 🐌 - [x] add account and while it is loading quickly go to NFTs and try to click on the filters panel as soon as it is unblocked - [x] filters panel should not throw Latest build: [extension-builds-3513](https://github.com/tahowallet/extension/suites/14022443935/artifacts/782867040) (as of Mon, 03 Jul 2023 10:15:53 GMT).
What
Solve throwing NFTs filters page when some collection got
undefinedlabel. Seems like it makesSharedToggleItemthrow an unexpected error. As we don't know yet how it happened that the collection had anundefinedlabel let's make a fix inisProbablyEVMAddressLatest build: extension-builds-3486 (as of Mon, 19 Jun 2023 10:13:43 GMT).