fix: ledger bitcoin query #5571
Conversation
WalkthroughThe recent updates primarily involve standardizing variable names for Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| import { useCurrentAccountNativeSegwitIndexZeroSignerNullable } from '@app/store/accounts/blockchain/bitcoin/native-segwit-account.hooks'; | ||
| import { useCurrentStacksAccountAddress } from '@app/store/accounts/blockchain/stacks/stacks-account.hooks'; | ||
|
|
||
| function SendButtonSuspense() { | ||
| const navigate = useNavigate(); | ||
| const { whenWallet } = useWalletType(); | ||
| const btcAddress = useCurrentAccountNativeSegwitIndexZeroSignerNullable()?.address; | ||
| const { balance: btcBalance } = useBtcCryptoAssetBalanceNativeSegwit(btcAddress ?? ''); |
There was a problem hiding this comment.
This was the bug in the linked issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/app/components/loaders/bitcoin-account-loader.tsx (1 hunks)
- src/app/components/loaders/btc-balance-loader.tsx (1 hunks)
- src/app/components/loaders/sip10-tokens-loader.tsx (1 hunks)
- src/app/components/loaders/stx-balance-loader.tsx (2 hunks)
- src/app/features/activity-list/activity-list.tsx (4 hunks)
- src/app/features/asset-list/asset-list.tsx (4 hunks)
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (1 hunks)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx (2 hunks)
- src/app/pages/home/components/send-button.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx
Additional context used
GitHub Check: typecheck
src/app/components/loaders/sip10-tokens-loader.tsx
[failure] 13-13:
Property 'isLoading' does not exist on type '{ isInitialLoading: boolean; tokens: { balance: CryptoAssetBalance; info: Sip10CryptoAssetInfo; }[]; }'.
Additional comments not posted (7)
src/app/components/loaders/btc-balance-loader.tsx (1)
12-12: Good alignment with new naming convention.The change from
isInitialLoadingtoisLoadingin both the interface and the function implementation is consistent and aligns with the new naming convention.Also applies to: 19-19
src/app/components/loaders/stx-balance-loader.tsx (1)
15-15: Good alignment with new naming convention.The change from
isInitialLoadingtoisLoadingin both the interface and the function implementation is consistent and aligns with the new naming convention.Also applies to: 31-32
src/app/components/loaders/bitcoin-account-loader.tsx (1)
36-36: Improved fallback handling.The explicit handling of the
fallbackcondition enhances robustness by ensuring anullreturn whenfallbackis falsy.src/app/pages/home/components/send-button.tsx (1)
25-25: Aligns with PR objectives to simplify button disabling logic.Removing the dependency on
btcBalancefor disabling the send button simplifies the logic and aligns with the PR objectives.src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (1)
52-52: Renaming of loading state variable approved.The change from
isInitialLoadingtoisLoadingis consistent with the PR’s aim to standardize loading state naming conventions across the codebase.src/app/features/activity-list/activity-list.tsx (1)
Line range hint
53-79: Renaming of loading state variables approved.The systematic replacement of
isInitialLoadingwithisLoadingacross various hooks and conditional checks inActivityListenhances clarity and maintains consistency with the rest of the codebase.src/app/features/asset-list/asset-list.tsx (1)
Line range hint
58-153: Renaming of loading state variables approved across various loaders.The systematic replacement of
isInitialLoadingwithisLoadingin various loaders withinAssetListenhances clarity and maintains consistency with the rest of the codebase.
[APROVED]
| @@ -10,6 +10,6 @@ | |||
| children(isLoading: boolean, tokens: Sip10TokenAssetDetails[]): React.ReactNode; | |||
| } | |||
| export function Sip10TokensLoader({ address, filter, children }: Sip10TokensLoaderProps) { | |||
| const { isInitialLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter }); | |||
| return children(isInitialLoading, tokens); | |||
| const { isLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter }); | |||
There was a problem hiding this comment.
Type mismatch needs correction.
The isLoading property does not exist on the returned object from useFilteredSip10Tokens. This needs to be corrected to align with expected property names.
- const { isLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter });
+ const { isInitialLoading: isLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter });Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { isLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter }); | |
| const { isInitialLoading: isLoading, tokens = [] } = useFilteredSip10Tokens({ address, filter }); |
Tools
GitHub Check: typecheck
[failure] 13-13:
Property 'isLoading' does not exist on type '{ isInitialLoading: boolean; tokens: { balance: CryptoAssetBalance; info: Sip10CryptoAssetInfo; }[]; }'.
|
Is there a test we can write for this too? |
fbwoolf
force-pushed
the
fix/ledger-bitcoin-query
branch
2 times, most recently
from
5be65b9 to
da558d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (10)
- package.json (1 hunks)
- src/app/components/loaders/btc-balance-loader.tsx (1 hunks)
- src/app/components/loaders/sip10-tokens-loader.tsx (1 hunks)
- src/app/components/loaders/stacks-account-loader.tsx (1 hunks)
- src/app/components/loaders/stx-balance-loader.tsx (2 hunks)
- src/app/features/activity-list/activity-list.tsx (4 hunks)
- src/app/features/asset-list/asset-list.tsx (4 hunks)
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (1 hunks)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx (2 hunks)
- src/app/pages/home/components/send-button.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- src/app/components/loaders/stacks-account-loader.tsx
Files skipped from review as they are similar to previous changes (7)
- src/app/components/loaders/sip10-tokens-loader.tsx
- src/app/components/loaders/stx-balance-loader.tsx
- src/app/features/activity-list/activity-list.tsx
- src/app/features/asset-list/asset-list.tsx
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx
- src/app/pages/home/components/send-button.tsx
Additional context used
GitHub Check: typecheck
src/app/components/loaders/btc-balance-loader.tsx
[failure] 16-16:
Argument of type '{ isLoading: boolean; data: UtxoResponseItem[]; error: Error; isError: true; isPending: false; isLoadingError: false; isRefetchError: true; isSuccess: false; status: "error"; ... 14 more ...; fetchStatus: FetchStatus; } | { ...; } | { ...; } | { ...; } | { ...; }' is not assignable to parameter of type 'UseQueryResult<UtxoResponseItem[] | undefined, Error | null>'.
[failure] 17-17:
Argument of type '{ isLoading: boolean; data: UtxoResponseItem[]; error: Error; isError: true; isPending: false; isLoadingError: false; isRefetchError: true; isSuccess: false; status: "error"; ... 14 more ...; fetchStatus: FetchStatus; } | { ...; } | { ...; } | { ...; } | { ...; }' is not assignable to parameter of type 'UseQueryResult<UtxoResponseItem[] | undefined, Error | null>'.
Additional comments not posted (1)
src/app/components/loaders/btc-balance-loader.tsx (1)
12-12: Approve parameter renaming for consistency.Renaming
isInitialLoadingtoisLoadingin the function signature enhances consistency across the application, aligning with similar changes in other components.
| } | ||
| export function BtcBalanceLoader({ address, children }: BtcBalanceLoaderProps) { | ||
| const { balance, query: result } = useBtcCryptoAssetBalanceNativeSegwit(address); | ||
| if (isInitializingData(result)) return <CryptoAssetItemPlaceholder />; | ||
| if (!isFetchedWithSuccess(result)) | ||
| return <CryptoAssetItemError caption="BTC" icon={<BtcAvatarIcon />} title="Bitcoin" />; | ||
| return children(balance, result.isInitialLoading); | ||
| return children(balance, result.isLoading); |
There was a problem hiding this comment.
Fix type mismatches in the return statement.
There seems to be a type error in the way result.isLoading is being used. Ensure that the types align correctly with the expected types in the children function call.
- return children(balance, result.isLoading);
+ return children(balance, result.isLoading as boolean); // Assuming `isLoading` should indeed be a boolean, cast it explicitly if necessary.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return children(balance, result.isLoading); | |
| return children(balance, result.isLoading as boolean); // Assuming `isLoading` should indeed be a boolean, cast it explicitly if necessary. |
fbwoolf
force-pushed
the
fix/ledger-bitcoin-query
branch
from
da558d8 to
640e400
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (12)
- package.json (1 hunks)
- src/app/common/hooks/balance/use-total-balance.tsx (2 hunks)
- src/app/components/loaders/btc-balance-loader.tsx (2 hunks)
- src/app/components/loaders/sip10-tokens-loader.tsx (1 hunks)
- src/app/components/loaders/stacks-account-loader.tsx (1 hunks)
- src/app/components/loaders/stx-balance-loader.tsx (2 hunks)
- src/app/features/activity-list/activity-list.tsx (4 hunks)
- src/app/features/asset-list/asset-list.tsx (4 hunks)
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (2 hunks)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx (2 hunks)
- src/app/pages/home/components/send-button.tsx (1 hunks)
- src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- src/app/common/hooks/balance/use-total-balance.tsx
Files skipped from review as they are similar to previous changes (10)
- package.json
- src/app/components/loaders/btc-balance-loader.tsx
- src/app/components/loaders/sip10-tokens-loader.tsx
- src/app/components/loaders/stacks-account-loader.tsx
- src/app/components/loaders/stx-balance-loader.tsx
- src/app/features/activity-list/activity-list.tsx
- src/app/features/asset-list/asset-list.tsx
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx
- src/app/pages/home/components/send-button.tsx
Additional comments not posted (1)
src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts (1)
39-39: Refactoring of the return statement for clarity and maintainability.The restructuring of the return statement to spread the
queryobject before appending thebalanceensures that all properties ofqueryare directly accessible, enhancing maintainability and reducing the need for additional property accesses.
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding error handling for nativeSegwitSigner.
The function directly uses nativeSegwitSigner.address without null checks. Adding error handling or null checks can prevent runtime errors if nativeSegwitSigner is undefined.
+ if (!nativeSegwitSigner) return null;
const nativeSegwitSigner = useCurrentAccountNativeSegwitIndexZeroSigner();
return useBtcCryptoAssetBalanceNativeSegwit(nativeSegwitSigner.address);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }; | |
| } | |
| if (!nativeSegwitSigner) return null; | |
| const nativeSegwitSigner = useCurrentAccountNativeSegwitIndexZeroSigner(); | |
| return useBtcCryptoAssetBalanceNativeSegwit(nativeSegwitSigner.address); |
fbwoolf
force-pushed
the
fix/ledger-bitcoin-query
branch
from
640e400 to
1acc40e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (12)
- package.json (1 hunks)
- src/app/common/hooks/balance/use-total-balance.tsx (2 hunks)
- src/app/components/loaders/btc-balance-loader.tsx (2 hunks)
- src/app/components/loaders/sip10-tokens-loader.tsx (1 hunks)
- src/app/components/loaders/stacks-account-loader.tsx (1 hunks)
- src/app/components/loaders/stx-balance-loader.tsx (2 hunks)
- src/app/features/activity-list/activity-list.tsx (4 hunks)
- src/app/features/asset-list/asset-list.tsx (4 hunks)
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (2 hunks)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx (2 hunks)
- src/app/pages/home/components/send-button.tsx (1 hunks)
- src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts (1 hunks)
Files skipped from review as they are similar to previous changes (12)
- package.json
- src/app/common/hooks/balance/use-total-balance.tsx
- src/app/components/loaders/btc-balance-loader.tsx
- src/app/components/loaders/sip10-tokens-loader.tsx
- src/app/components/loaders/stacks-account-loader.tsx
- src/app/components/loaders/stx-balance-loader.tsx
- src/app/features/activity-list/activity-list.tsx
- src/app/features/asset-list/asset-list.tsx
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx
- src/app/pages/home/components/send-button.tsx
- src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts
fbwoolf
force-pushed
the
fix/ledger-bitcoin-query
branch
from
1acc40e to
03eb692
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (13)
- package.json (1 hunks)
- src/app/common/hooks/balance/use-total-balance.tsx (2 hunks)
- src/app/components/loaders/btc-balance-loader.tsx (2 hunks)
- src/app/components/loaders/sip10-tokens-loader.tsx (1 hunks)
- src/app/components/loaders/stacks-account-loader.tsx (1 hunks)
- src/app/components/loaders/stx-balance-loader.tsx (2 hunks)
- src/app/features/activity-list/activity-list.tsx (4 hunks)
- src/app/features/asset-list/asset-list.tsx (4 hunks)
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx (2 hunks)
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx (2 hunks)
- src/app/pages/home/components/send-button.tsx (1 hunks)
- src/app/pages/home/home.tsx (2 hunks)
- src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts (1 hunks)
Files skipped from review as they are similar to previous changes (12)
- package.json
- src/app/common/hooks/balance/use-total-balance.tsx
- src/app/components/loaders/btc-balance-loader.tsx
- src/app/components/loaders/sip10-tokens-loader.tsx
- src/app/components/loaders/stacks-account-loader.tsx
- src/app/components/loaders/stx-balance-loader.tsx
- src/app/features/activity-list/activity-list.tsx
- src/app/features/asset-list/asset-list.tsx
- src/app/features/asset-list/bitcoin/brc20-token-asset-list/brc20-token-asset-list.tsx
- src/app/pages/fund/choose-asset-to-fund/choose-asset-to-fund.tsx
- src/app/pages/home/components/send-button.tsx
- src/app/query/bitcoin/balance/btc-balance-native-segwit.hooks.ts
Additional comments not posted (2)
src/app/pages/home/home.tsx (2)
15-15: Approved new import foruseCurrentAccountIndex.This import enhances clarity by providing direct access to the current account index, aligning with the PR's goal of improving code readability and maintainability.
30-30: Refactor: Use ofcurrentAccountIndexinuseAccountDisplayName.The direct use of
currentAccountIndexinstead ofaccount?.indexsimplifies the code and potentially avoids a null reference issue ifaccountis undefined. This change should be verified across the application to ensure it doesn't affect existing functionalities.Also applies to: 34-34
|
Created this issue as a follow up here after discussing with team in standup: #5574 |
The bug here took a bit to track down, but it was a change made to use the btc balance to disable the home page send button.
Hoping to install this change to the query pkg before merging this: leather-io/mono#236
Summary by CodeRabbit
isLoadingfor consistency.CurrentStacksAccountLoadercomponent.SendButtonSuspensecomponent.These changes enhance code clarity and consistency, ensuring a more maintainable codebase and uniform loading state handling across the application.