-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: problem with endless loading of balances and names #5296
Conversation
e4d2c67
to
3dd7d70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFetching
fires more often than isLoading
right, as fetching is always new regardless of if it has data.
So doesn't fetching show the loader more often?
@@ -18,6 +19,7 @@ export function useNativeSegwitBalance(address: string) { | |||
btcBalance: wrappedBalance, | |||
isInitialLoading, | |||
isLoading, | |||
isFetching, | |||
}; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could also be a single query being returned with a select
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it couldn't... actually not that easy. the query useGetUtxosByAddressQuery
is wrapped several times before this exact hook
Do we show the shimmer or the skeleton? |
hm, wonder if it's critical to show shimmer animation every time we fetching? otherwise we need to add something like |
for this change we show shimmer |
can you explain this a little more? Why is part of the request not enabled? Questioning this just because the changes don't map to my understanding of these props. isLoading = initial load only. isFetching = all refetches. So, if we have a problem with it showing loading state too often, how does changing it to a property that is true more often, fix this problem? |
e.g. in ledger mode, when we don't have stx address => we set |
This is actually surprising to me ...shouldn't that disable the query entirely, so isLoading would be false? |
1d6680e
to
3f625ed
Compare
3f625ed
to
f69c12d
Compare
export function useAccountDisplayName({ address, index }: { index: number; address: string }) { | ||
const { data: names = [], isLoading } = useGetAccountNamesByAddressQuery(address); | ||
return useMemo(() => { | ||
const name = names[0] || getAutogeneratedAccountDisplayName(index); | ||
return { | ||
name, | ||
isLoading, | ||
}; | ||
}, [names, index, isLoading]); | ||
const query = useGetBnsNamesOwnedByAddress(address, { | ||
select: resp => { | ||
const names = resp.names ?? []; | ||
return names[0] || getAutogeneratedAccountDisplayName(index); | ||
}, | ||
}); | ||
|
||
return { | ||
...query, | ||
data: query.data || getAutogeneratedAccountDisplayName(index), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need one of these two techniques right? select or replacing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem here is again in ledger mode, when query is not enabled, select doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if you do it on the data prop, it works for both cases, and the select isn't needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean remove select and
data: (query.data?.names ?? [])[0] || getAutogeneratedAccountDisplayName(index)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seems strange to me you're repeating the logic twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can try it in ledger bitcoin only mode
This pr fixes "endless" loading of balances and names in only stacks/btc mode, since part of requests are not enabled and useQuery always returns
isLoading: true
, I changed it toisFetching