Feat/switch desktop wallet#568
Conversation
WalkthroughAdds multi‑wallet management and persistence: new SelectWallet and RemoveWalletConfirm flows, WalletCard UI, local wallet storage hook and APIs, storage-aware switching (desktop vs in‑app), modal preventAutoClose control, wallet-switch feedback UI, translations, and types/theme updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Profile as ProfileContent
participant Selector as AccountSelector
participant Select as SelectWalletContent
participant Storage as useWalletStorage
participant SwitchHook as useSwitchWallet
participant WalletHook as useWallet
User->>Profile: Click "Switch wallet"
Profile->>Selector: request switch (desktop path)
Selector->>Select: setCurrentContent('select-wallet')
Select->>Storage: getStoredWallets()
Storage-->>Select: stored wallets
User->>Select: Click wallet "Select"
Select->>Storage: setActiveWallet(address)
Storage-->>Select: confirm active
Select->>SwitchHook: switchWallet() or emit wallet_switched (desktop)
alt In-app browser
SwitchHook->>WalletHook: call dappKitSwitchWallet()
WalletHook-->>Select: connection updated
else Desktop
SwitchHook->>WalletHook: dispatch wallet_switched event
WalletHook-->>Select: effective address updated
end
Select-->>User: update UI / close or return
sequenceDiagram
actor User
participant Select as SelectWalletContent
participant Confirm as RemoveWalletConfirmContent
participant Storage as useWalletStorage
participant WalletHook as useWallet
User->>Select: Click "Remove" on wallet
Select->>Confirm: navigate to remove-wallet-confirm (props)
Confirm-->>User: show confirmation
User->>Confirm: Confirm remove
Confirm->>Storage: removeWallet(address)
Storage-->>Confirm: removed
alt No wallets left
Confirm->>WalletHook: trigger disconnect/logout
Confirm-->>User: modal closes
else Other wallets present
Storage->>Storage: setActiveWallet(newAddress) if needed
Confirm-->>Select: navigate back
Select-->>User: refreshed list & active wallet
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Size Change: +158 kB (+2.82%) Total Size: 5.74 MB
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vechain-kit/src/components/ConnectModal/ConnectModal.tsx (1)
131-136: MissingpreventAutoCloseprop in fallback content.The fallback
MainContentrendered whenrenderContent()returns null does not receive thepreventAutoCloseprop, which could cause inconsistent auto-close behavior.🔎 Proposed fix
const fallbackContent = ( <MainContent setCurrentContent={setCurrentContent} onClose={onClose} + preventAutoClose={preventAutoClose} /> );
🧹 Nitpick comments (13)
packages/vechain-kit/src/languages/zh.json (1)
22-22: Clarify duplicate translation keys with different casing.The translations for wallet management UI look appropriate. However, there are two similar keys with different casing:
- Line 279:
"Remove Wallet"→"移除钱包"- Line 281:
"Remove wallet"→"移除钱包"If these are intentional (e.g., title case for headings vs. sentence case for buttons), consider adding comments or using more distinctive key names to clarify their different contexts. If they serve the same purpose, consolidate them to avoid confusion.
Also applies to: 24-24, 44-44, 210-210, 247-247, 279-281, 294-294
packages/vechain-kit/src/languages/es.json (1)
22-22: Clarify duplicate translation keys with different casing.The Spanish translations for wallet management UI look appropriate. However, the same duplicate key pattern appears here:
- Line 279:
"Remove Wallet"→"Eliminar Billetera"- Line 281:
"Remove wallet"→"Eliminar billetera"This pattern is consistent across all language files. If these keys serve different UI contexts (e.g., modal titles vs. button labels), consider using more distinctive key names (e.g.,
"Remove Wallet Title"vs"Remove Wallet Button") to clarify intent and improve maintainability.Also applies to: 24-24, 44-44, 210-210, 247-247, 279-281, 294-294
packages/vechain-kit/src/languages/ja.json (1)
22-22: Japanese translations look good; duplicate key pattern noted.The Japanese translations for wallet management UI are contextually appropriate. The duplicate key pattern (
"Remove Wallet"vs"Remove wallet") is consistent across all language files, suggesting it may be intentional. However, the same recommendation applies: if these serve different contexts, use more distinctive key names to improve clarity.Also applies to: 24-24, 44-44, 210-210, 247-247, 279-281, 294-294
packages/vechain-kit/src/languages/de.json (1)
22-22: German translations look good; duplicate key pattern consistent.The German translations for wallet management UI are contextually appropriate. The duplicate key pattern is present here as well:
- Line 279:
"Remove Wallet"→"Brieftasche entfernen"- Line 281:
"Remove wallet"→"Brieftasche entfernen"Since this pattern appears in all language files, it's likely intentional. Still, for long-term maintainability, consider consolidating or using more explicit key names to distinguish their usage contexts.
Also applies to: 24-24, 44-44, 210-210, 247-247, 279-281, 294-294
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
120-150: Duplicatedata-testidon different buttons.Both the in-app browser switch button (line 134) and the DappKit switch button (line 147) have
data-testid="switch-wallet-button". This will make it difficult to distinguish between them in tests.Consider using distinct test IDs like
"switch-wallet-inapp-button"and"switch-wallet-desktop-button".🔎 Suggested fix
isLoading={isSwitching} isDisabled={isSwitching} - data-testid="switch-wallet-button" + data-testid="switch-wallet-inapp-button" > {t('Switch')} </Button> ) : connection.isConnectedWithDappKit ? ( <Button size="md" width="full" height="40px" variant="vechainKitSecondary" leftIcon={<Icon as={LuArrowLeftRight} />} colorScheme="red" onClick={handleSwitchWallet} - data-testid="switch-wallet-button" + data-testid="switch-wallet-desktop-button" >packages/vechain-kit/src/languages/en.json (1)
279-281: Potential duplicate localization keys with inconsistent casing.Both
"Remove Wallet"(line 279) and"Remove wallet"(line 281) exist. This could lead to inconsistent capitalization in the UI depending on which key is used. Consider consolidating to a single key and using CSS text-transform if different casing is needed in different contexts.packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (2)
19-25: Consider removing the unusedwalletDomainprop or documenting deprecation.The
walletDomainprop is received but intentionally unused (prefixed as_walletDomain), with a comment indicating it's "kept for backward compatibility." If this is a new component, there's no backward compatibility concern. Consider either removing the prop or adding a@deprecatedJSDoc annotation if it's needed for API stability.Also applies to: 29-29
53-58: Consider using a theme token instead of hardcoded color.The trash icon uses a hardcoded color
'#ef4444'(line 55). For consistency with the rest of the theming system, consider using a semantic color token (e.g.,vechain-kit-dangeror similar if available).🔎 Suggested approach
<Icon as={LuTrash2} - color={'#ef4444'} + color="red.500" fontSize={'60px'} opacity={0.5} />Or use
useTokenif a semantic token exists for danger/error colors.packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (3)
186-211: Magic timeout value may cause race conditions.The 1000ms timeout (Line 196-198) to reset
isWalletSwitchInProgressis arbitrary. If wallet connection takes longer, it could cause the active wallet to be overwritten unexpectedly.🔎 Consider using a more deterministic approach
Instead of a fixed timeout, consider resetting the flag when the connection state actually stabilizes, or use a ref to track the intended switch target:
- // Reset the flag after a short delay to allow the connection to update - setTimeout(() => { - setIsWalletSwitchInProgress(false); - }, 1000); + // Flag will be reset when connection state matches the switched walletThen reset in the effect that handles connection changes when the effective address matches the switched address.
248-293: Effect dependency array includes unstable derived values.Using
activeAccountMetadata?.domain,activeAccountMetadata?.image, andactiveAccountMetadata?.isLoading(Lines 286-288) as separate dependencies can cause unnecessary re-runs. TheactiveAccountMetadataobject identity should be sufficient, or use theisLoadingflag alone since that's what the condition checks.🔎 Simplify dependencies
}, [ isConnectedWithDappKit, isInAppBrowser, connectedWalletAddress, - activeAccountMetadata?.domain, - activeAccountMetadata?.image, activeAccountMetadata?.isLoading, initializeCurrentWallet, saveWallet, setActiveWalletStorage, storedActiveWalletAddress, + isWalletSwitchInProgress, ]);Note:
isWalletSwitchInProgressis used in the effect but missing from dependencies.
295-314: Redundant wallet save operation.This effect saves the wallet when
storedActiveWalletAddressmatcheseffectiveConnectedWalletAddress, but the previous effect (Lines 248-293) already callssaveWallet(connectedWalletAddress). This may result in duplicate localStorage writes.Consider consolidating the save logic into a single effect to avoid redundant operations.
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (2)
67-85: Consider a more robust synchronization approach.Using
setTimeoutwith a hardcoded 100ms delay for storage synchronization is fragile and could lead to race conditions on slower devices or under heavy load. Consider having the event carry the updated wallet data directly, or using a shared state management solution.That said, if the
wallet_switchedevent is dispatched after storage is already updated, the delay may be unnecessary:const handleWalletSwitch = () => { - // Small delay to ensure storage is updated - setTimeout(() => { - refreshWallets(); - }, 100); + refreshWallets(); };
113-145: Remove unusedaccountfrom dependency array.
accountis listed in the dependency array but is not used in the callback body. This causes unnecessary re-creation of the callback whenaccountchanges.🔎 Proposed fix
}, [ activeWalletAddress, activeWallet, - account, setActiveWallet, refresh, setCurrentContent, + returnTo, refreshWallets, saveWallet, ]);Note:
returnToshould also be added since it's used in the callback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
examples/homepage/src/app/components/features/LoginMethodsSection/index.tspackages/vechain-kit/src/components/AccountModal/AccountModal.tsxpackages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/Components/WalletCard.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/index.tspackages/vechain-kit/src/components/AccountModal/Contents/index.tspackages/vechain-kit/src/components/AccountModal/Types/Types.tspackages/vechain-kit/src/components/ConnectModal/Components/DappKitButton.tsxpackages/vechain-kit/src/components/ConnectModal/ConnectModal.tsxpackages/vechain-kit/src/components/ConnectModal/Contents/MainContent.tsxpackages/vechain-kit/src/components/common/AccountAvatar.tsxpackages/vechain-kit/src/hooks/api/wallet/index.tspackages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.tspackages/vechain-kit/src/hooks/api/wallet/useWallet.tspackages/vechain-kit/src/hooks/api/wallet/useWalletStorage.tspackages/vechain-kit/src/hooks/signing/useSignMessage.tspackages/vechain-kit/src/hooks/thor/transactions/useSendTransaction.tspackages/vechain-kit/src/languages/de.jsonpackages/vechain-kit/src/languages/en.jsonpackages/vechain-kit/src/languages/es.jsonpackages/vechain-kit/src/languages/fr.jsonpackages/vechain-kit/src/languages/it.jsonpackages/vechain-kit/src/languages/ja.jsonpackages/vechain-kit/src/languages/zh.jsonpackages/vechain-kit/src/providers/ModalProvider.tsxpackages/vechain-kit/src/theme/card.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/hooks/signing/useSignMessage.tspackages/vechain-kit/src/components/AccountModal/Contents/index.tspackages/vechain-kit/src/hooks/api/wallet/index.tspackages/vechain-kit/src/hooks/thor/transactions/useSendTransaction.tspackages/vechain-kit/src/components/ConnectModal/Contents/MainContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/index.tspackages/vechain-kit/src/theme/card.tspackages/vechain-kit/src/components/ConnectModal/ConnectModal.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/Components/WalletCard.tsxpackages/vechain-kit/src/hooks/api/wallet/useWallet.tspackages/vechain-kit/src/components/AccountModal/Types/Types.tspackages/vechain-kit/src/components/common/AccountAvatar.tsxpackages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.tspackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsxpackages/vechain-kit/src/providers/ModalProvider.tsxpackages/vechain-kit/src/components/AccountModal/AccountModal.tsxpackages/vechain-kit/src/hooks/api/wallet/useWalletStorage.tspackages/vechain-kit/src/components/ConnectModal/Components/DappKitButton.tsxexamples/homepage/src/app/components/features/LoginMethodsSection/index.tspackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : In VeChain Kit Version 2, use `useThor` instead of `useConnex` for contract interactions
Applied to files:
packages/vechain-kit/src/hooks/api/wallet/index.tspackages/vechain-kit/src/hooks/thor/transactions/useSendTransaction.tspackages/vechain-kit/src/hooks/api/wallet/useWallet.tspackages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.tspackages/vechain-kit/src/components/ConnectModal/Components/DappKitButton.tsx
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : For transaction building and sending, use the `useBuildTransaction` hook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Applied to files:
packages/vechain-kit/src/hooks/thor/transactions/useSendTransaction.ts
🧬 Code graph analysis (11)
packages/vechain-kit/src/components/ConnectModal/ConnectModal.tsx (1)
packages/vechain-kit/src/components/ConnectModal/Contents/MainContent.tsx (1)
MainContent(28-110)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/Components/WalletCard.tsx (5)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
StoredWallet(11-15)packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
useVeChainKitConfig(204-210)packages/vechain-kit/src/hooks/api/wallet/useWalletMetadata.ts (1)
useWalletMetadata(10-34)packages/vechain-kit/src/components/common/AccountAvatar.tsx (1)
AccountAvatar(10-53)packages/vechain-kit/src/utils/formattingUtils.tsx (2)
humanDomain(11-22)humanAddress(5-9)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (4)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
useWalletStorage(17-138)packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)packages/vechain-kit/src/hooks/thor/smartAccounts/useSmartAccount.ts (1)
useSmartAccount(51-60)packages/vechain-kit/src/hooks/api/wallet/useWalletMetadata.ts (1)
useWalletMetadata(10-34)
packages/vechain-kit/src/components/AccountModal/Types/Types.ts (1)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (1)
RemoveWalletConfirmContentProps(19-25)
packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (3)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (2)
StoredWallet(11-15)useWalletStorage(17-138)packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
useWallet(62-410)packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (5)
packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
useVeChainKitConfig(204-210)packages/vechain-kit/src/hooks/api/wallet/useWalletMetadata.ts (1)
useWalletMetadata(10-34)packages/vechain-kit/src/utils/formattingUtils.tsx (2)
humanDomain(11-22)humanAddress(5-9)packages/vechain-kit/src/components/common/StickyHeaderContainer.tsx (1)
StickyHeaderContainer(16-140)packages/vechain-kit/src/components/common/ModalBackButton.tsx (1)
ModalBackButton(8-23)
packages/vechain-kit/src/providers/ModalProvider.tsx (1)
packages/vechain-kit/src/components/ConnectModal/ConnectModal.tsx (1)
ConnectModalContentsTypes(17-43)
packages/vechain-kit/src/components/AccountModal/AccountModal.tsx (2)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (1)
RemoveWalletConfirmContent(27-99)packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (1)
SelectWalletContent(37-322)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (3)
packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
useVeChainKitConfig(204-210)packages/vechain-kit/src/config/network.ts (1)
NETWORK_TYPE(6-6)packages/vechain-kit/src/utils/ssrUtils.ts (4)
isBrowser(75-77)getLocalStorageItem(8-18)setLocalStorageItem(23-32)removeLocalStorageItem(37-46)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (9)
packages/vechain-kit/src/components/AccountModal/Types/Types.ts (1)
AccountModalContentTypes(20-119)packages/vechain-kit/src/hooks/modals/useAccountModalOptions.tsx (1)
useAccountModalOptions(3-10)packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
useWallet(62-410)packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (1)
useSwitchWallet(20-75)packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (2)
useWalletStorage(17-138)StoredWallet(11-15)packages/vechain-kit/src/providers/ModalProvider.tsx (1)
useModal(73-79)packages/vechain-kit/src/hooks/api/wallet/useRefreshBalances.ts (1)
useRefreshBalances(4-28)packages/vechain-kit/src/components/common/StickyHeaderContainer.tsx (1)
StickyHeaderContainer(16-140)packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/Components/WalletCard.tsx (1)
WalletCard(28-128)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (3)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
useWallet(62-410)packages/vechain-kit/src/hooks/index.ts (1)
useWallet(17-17)packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (1)
useSwitchWallet(20-75)
🔇 Additional comments (49)
packages/vechain-kit/src/hooks/signing/useSignMessage.ts (1)
72-72: LGTM! Dependency optimization aligned with wallet switching.The change from
accounttoaccount?.addressin the dependency array is a sound optimization that ensures thesignMessagecallback recreates specifically when the wallet address changes—critical for the wallet-switching feature introduced in this PR—while avoiding unnecessary recreations when the account object reference changes but the address remains constant.packages/vechain-kit/src/theme/card.ts (1)
31-39: LGTM! New wallet card variant is well-structured.The
vechainKitWalletCardvariant provides appropriate styling for clickable wallet cards with pointer cursor and relative positioning for potential child elements (badges, icons). The structure follows the existing pattern and aligns with the wallet selection UI introduced in this PR.examples/homepage/src/app/components/features/LoginMethodsSection/index.ts (1)
1-1: LGTM! Formatting cleanup.Trivial formatting change removing extra blank lines at EOF. No functional impact.
packages/vechain-kit/src/components/ConnectModal/Contents/MainContent.tsx (2)
25-25: LGTM! Prop addition enables flexible modal behavior.The
preventAutoCloseprop with default valuefalsepreserves existing auto-close behavior while allowing callers to keep the modal open after connection when needed.Also applies to: 28-28
48-51: LGTM! Correct conditional auto-close logic.The effect correctly guards
onClosewith thepreventAutoCloseflag and includes it in the dependency array, ensuring the modal can remain open after connection when required by the caller.packages/vechain-kit/src/components/common/AccountAvatar.tsx (1)
12-22: LGTM! Wallet address tracking ensures correct avatar on wallet switch.The addition of
walletAddressRefand the effect to resetpreviousImageRefwhen the wallet address changes is essential for the wallet switching feature. This prevents stale avatar images from being displayed when users switch between wallets, ensuring the UI remains consistent with the active wallet.The implementation correctly:
- Tracks the current wallet address with a ref
- Resets the cached image reference when the address changes
- Preserves the existing image caching behavior during metadata loading
packages/vechain-kit/src/hooks/api/wallet/index.ts (1)
22-22: LGTM!The new
useWalletStorageexport is correctly added and aligns with the barrel file pattern used throughout the codebase.packages/vechain-kit/src/components/AccountModal/Contents/index.ts (1)
14-14: LGTM!The
SelectWalletmodule export correctly exposes the new wallet selection components.packages/vechain-kit/src/components/ConnectModal/Components/DappKitButton.tsx (1)
29-44: LGTM - dependency array fix and documentation.Good addition of
sourceto the dependency array since it's referenced in the callback. The comments helpfully document that wallet activation is now centralized inuseWallet.ts.packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/index.ts (1)
1-2: LGTM!Clean barrel file structure exposing the new wallet content components.
packages/vechain-kit/src/components/AccountModal/AccountModal.tsx (3)
21-22: LGTM!Imports for the new wallet content components are correctly added.
100-103: LGTM!The
remove-wallet-confirmcase correctly rendersRemoveWalletConfirmContentby spreading the props fromcurrentContent.props, which aligns with the component's expected interface.
135-145: LGTM!The
select-walletcase properly passes the required props toSelectWalletContent. The prop mapping (setCurrentContent,onClose,returnTo,onLogoutSuccess) matches the component's expected interface from the relevant code snippets.packages/vechain-kit/src/hooks/thor/transactions/useSendTransaction.ts (2)
169-178: Signer resolution logic for desktop DappKit looks correct.The conditional logic ensures that on desktop (non-in-app browser) with DappKit, the user's selected/stored wallet (
signerAccountAddress) is used as the signer, while in-app browser flows continue using the extension's current wallet (signer.address). This aligns with the PR's goal of enabling desktop wallet switching.Please verify that in-app browser transaction flows still function correctly after this change, particularly that
signer.addressis used as intended whenisInAppBrowseris true.
201-202: LGTM - dependency array updated correctly.The new connection properties are correctly added to the dependency array since they're used in the signer resolution logic.
packages/vechain-kit/src/components/ConnectModal/ConnectModal.tsx (2)
14-14: LGTM!The
preventAutoCloseprop is correctly typed as optional and properly defaulted tofalse.Also applies to: 49-49
64-70: LGTM!The
preventAutoCloseprop is correctly propagated toMainContentin both the null-check fallback and the 'main' case.Also applies to: 79-79
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (3)
24-24: LGTM!The new hook imports and destructuring correctly provide the
isInAppBrowserflag andgetAvailableMethodsfunction needed for the branching logic.Also applies to: 49-50
127-129: LGTM - visibility condition correctly updated.The switch wallet button now shows when either:
- In-app browser with
switchWalletmethod available, OR- Connected with DappKit (for desktop wallet selection)
This enables the new desktop wallet switching flow while preserving the in-app browser behavior.
66-80: No issues found. The props structure is correct and consistent with the type definitions. ThehandleSwitchWalletfunction properly nestssetCurrentContentwithinpropswhen creating the state object (matching the'select-wallet'type definition in Types.ts), andAccountModal.tsxcorrectly unpacks these nested props when renderingSelectWalletContent. This is the expected pattern: the state object uses nested props, while the component itself receives individual props extracted from that object.packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
60-65: LGTM!The settings button is now consistently available, which aligns with the wallet management flow improvements.
packages/vechain-kit/src/components/AccountModal/Types/Types.ts (2)
36-46: LGTM!The
select-walletcontent type is well-structured with appropriate props. ThereturnTooptional property with a literal union type provides good type safety for navigation flows.
104-107: LGTM!The
remove-wallet-confirmcontent type correctly references theRemoveWalletConfirmContentPropstype, maintaining consistency with the component definition.packages/vechain-kit/src/languages/en.json (1)
22-24: LGTM!The new wallet-related localization strings are well-organized and follow the existing alphabetical ordering convention.
Also applies to: 44-44, 210-210, 247-247, 279-281, 294-294
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (2)
39-41: Verify thehumanDomainparameters are intentional.
humanDomain(domain, 20, 0)useslengthAfter=0, meaning the domain will be truncated to show only the first 20 characters with no suffix. Based on the utility signature informattingUtils.tsx, this would displayfirst20chars•••without any ending portion. Confirm this is the intended display behavior.
27-98: LGTM overall!The component structure is clean, follows established patterns (StickyHeaderContainer, ModalBackButton), and properly integrates with the translation system. The dynamic domain resolution via
useWalletMetadatais a good approach.packages/vechain-kit/src/providers/ModalProvider.tsx (3)
86-87: LGTM!The
connectModalPreventAutoClosestate is properly managed: initialized tofalse, set when opening the modal, and reset tofalsewhen closing. This ensures clean state between modal sessions.Also applies to: 105-105, 115-115
177-211: LGTM!The context provider correctly exposes the new
connectModalPreventAutoClosestate and setter, and properly passes the flag to theConnectModalcomponent.
89-110: Missing dependencies inuseCallback.The
openConnectModalcallback usessetSourceandconnectV2fromuseDAppKitWallet()but the dependency array is empty. While React state setters are stable and don't need to be included,setSourceandconnectV2come from an external hook and could potentially change.Run the following script to verify if these functions are stable:
#!/bin/bash # Check the useDAppKitWallet hook implementation for memoization of setSource and connectV2 ast-grep --pattern $'export const useDAppKitWallet = () => { $$$ }'packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/Components/WalletCard.tsx (2)
28-50: LGTM! Clean component structure with proper hook usage.The component correctly:
- Integrates wallet metadata and balance hooks
- Uses theming tokens for consistent styling
- Combines loading states appropriately
52-127: Well-implemented card with proper event handling.Good use of
stopPropagationon the remove button (Line 119) to prevent triggeringonSelectwhen removing. The conditional rendering for active indicator vs remove button is clean.packages/vechain-kit/src/languages/it.json (1)
22-24: Translations look correct.The new Italian translations for wallet management features are properly added. Note that both "Remove Wallet" (Line 279) and "Remove wallet" (Line 281) are intentional for different UI contexts (title vs button label).
Also applies to: 44-44, 210-210, 247-247, 279-279, 281-281, 294-294
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
161-170: Verify initial state synchronization on mount.The initial state for
storedActiveWalletAddress(Lines 168-170) is computed once during render. IfisConnectedWithDappKitorisInAppBrowserchange after the initial render, the state won't reflect the correct value until the effect on Lines 173-180 runs. This should work correctly due to the subsequent effect, but verify behavior during rapid connection state changes.packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (2)
20-52: LGTM! Clean separation of in-app browser vs desktop flows.The hook correctly:
- Delegates to dapp-kit's
switchWalletfor in-app browser environments- Provides storage utilities for desktop UI-based switching
- Handles the async nature appropriately with loading state
54-74: Good event-based communication pattern.The
setActiveWalletwrapper properly persists to storage and dispatches thewallet_switchedcustom event for cross-component communication. This decoupled approach allowsuseWalletto react to switches initiated from anywhere.packages/vechain-kit/src/languages/fr.json (1)
22-24: French translations are correct.The new wallet management translations are properly localized. The translation quality looks good with appropriate French phrasing.
Also applies to: 44-44, 210-210, 247-247, 279-279, 281-281, 294-294
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (7)
1-15: LGTM!Imports are correctly structured using SSR-safe utilities, and the
StoredWallettype is well-defined with the necessary fields for wallet tracking.
20-25: LGTM!The storage key generation is correctly scoped by network type, ensuring wallet data is isolated per network.
36-41: LGTM!Simple and safe implementation with proper SSR guard.
43-71: LGTM!The save logic correctly handles both new and existing wallets with case-insensitive address matching. Setting
isActive: falseon save is appropriate sincesetActiveWallethandles activation separately.
73-90: LGTM!Clean implementation with proper case-insensitive matching and dual storage for the active wallet state.
92-114: LGTM!Proper cleanup logic that correctly handles the case when the removed wallet was the active one.
116-128: LGTM!Safe initialization that only acts when no wallets are stored, preventing accidental overwrites.
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (6)
42-54: LGTM!Hooks are properly initialized with appropriate initial state from storage.
56-65: LGTM!Clean refresh logic that properly syncs state when the account changes.
87-111: LGTM!Well-structured memoized computations with proper fallback logic for the active wallet address.
147-230: LGTM!The removal flow is comprehensive, correctly handling edge cases like removing the active wallet and removing the last wallet. The navigation to confirmation screen with appropriate callbacks is well-implemented.
255-321: LGTM!Clean UI structure with proper conditional rendering, localization, and appropriate handling of the active wallet's no-op
onSelect.
28-41: TheonCloseprop is declared in the Props type but not used within the component body. However, it is required by the parent component's type contract. The parent component (AccountModal.tsx) explicitly passesonClosewhen instantiating SelectWalletContent, and the type definition in Types.ts declares it as a required prop. Removing it would break the parent component's interface. Consider documenting this prop or refactoring the parent component if it shouldn't be required.Likely an incorrect or invalid review comment.
| const handleSwitchWallet = () => { | ||
| if (isInAppBrowser) { | ||
| switchWallet(); | ||
| } else { | ||
| // Desktop: navigate to select wallet screen | ||
| setCurrentContent({ | ||
| type: 'select-wallet', | ||
| props: { | ||
| setCurrentContent, | ||
| onClose: () => {}, | ||
| returnTo: 'profile', | ||
| onLogoutSuccess, | ||
| }, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The onClose callback is a no-op, which may cause issues.
The handleSwitchWallet function passes onClose: () => {} as a prop to the select-wallet content. This empty callback might prevent proper modal closure behavior when the user completes or cancels the wallet selection flow.
Consider whether this should invoke closeAccountModal or another appropriate handler.
🔎 Suggested fix
const handleSwitchWallet = () => {
if (isInAppBrowser) {
switchWallet();
} else {
// Desktop: navigate to select wallet screen
setCurrentContent({
type: 'select-wallet',
props: {
setCurrentContent,
- onClose: () => {},
+ onClose: () => setCurrentContent('profile'),
returnTo: 'profile',
onLogoutSuccess,
},
});
}
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx
around lines 36 to 51, the handleSwitchWallet function passes a no-op onClose
(()=>{}) into the 'select-wallet' content which prevents the modal from closing
after wallet selection/cancellation; replace the no-op with the proper modal
close handler (e.g. closeAccountModal) or forward the component's existing
onClose prop so the select-wallet flow can close the modal and perform cleanup,
ensuring you pass any required params (or wrap to call additional callbacks like
onLogoutSuccess if needed).
| const handleAddNewWallet = useCallback(async () => { | ||
| // Disconnect from dappkit first if connected | ||
| if (connection.isConnectedWithDappKit && !connection.isInAppBrowser) { | ||
| try { | ||
| dappKitDisconnect(); | ||
| } catch (error) { | ||
| console.error('Error disconnecting from dappkit:', error); | ||
| } | ||
| } | ||
|
|
||
| // Small delay to ensure disconnect is processed | ||
| setTimeout(() => { | ||
| // Open ConnectModal without preventAutoClose so it closes automatically | ||
| // when a new wallet is connected after disconnect | ||
| openConnectModal('main', false); | ||
| }, 100); | ||
| }, [ | ||
| openConnectModal, | ||
| connection.isConnectedWithDappKit, | ||
| connection.isInAppBrowser, | ||
| dappKitDisconnect, | ||
| ]); |
There was a problem hiding this comment.
Inconsistent async handling of dappKitDisconnect.
dappKitDisconnect() is awaited in handleRemoveWallet (line 173) but not here. This inconsistency could cause the connect modal to open before disconnect completes, leading to unexpected behavior.
🔎 Proposed fix
const handleAddNewWallet = useCallback(async () => {
// Disconnect from dappkit first if connected
if (connection.isConnectedWithDappKit && !connection.isInAppBrowser) {
try {
- dappKitDisconnect();
+ await dappKitDisconnect();
} catch (error) {
console.error('Error disconnecting from dappkit:', error);
}
}
- // Small delay to ensure disconnect is processed
- setTimeout(() => {
- // Open ConnectModal without preventAutoClose so it closes automatically
- // when a new wallet is connected after disconnect
- openConnectModal('main', false);
- }, 100);
+ // Open ConnectModal without preventAutoClose so it closes automatically
+ // when a new wallet is connected after disconnect
+ openConnectModal('main', false);
}, [📝 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 handleAddNewWallet = useCallback(async () => { | |
| // Disconnect from dappkit first if connected | |
| if (connection.isConnectedWithDappKit && !connection.isInAppBrowser) { | |
| try { | |
| dappKitDisconnect(); | |
| } catch (error) { | |
| console.error('Error disconnecting from dappkit:', error); | |
| } | |
| } | |
| // Small delay to ensure disconnect is processed | |
| setTimeout(() => { | |
| // Open ConnectModal without preventAutoClose so it closes automatically | |
| // when a new wallet is connected after disconnect | |
| openConnectModal('main', false); | |
| }, 100); | |
| }, [ | |
| openConnectModal, | |
| connection.isConnectedWithDappKit, | |
| connection.isInAppBrowser, | |
| dappKitDisconnect, | |
| ]); | |
| const handleAddNewWallet = useCallback(async () => { | |
| // Disconnect from dappkit first if connected | |
| if (connection.isConnectedWithDappKit && !connection.isInAppBrowser) { | |
| try { | |
| await dappKitDisconnect(); | |
| } catch (error) { | |
| console.error('Error disconnecting from dappkit:', error); | |
| } | |
| } | |
| // Open ConnectModal without preventAutoClose so it closes automatically | |
| // when a new wallet is connected after disconnect | |
| openConnectModal('main', false); | |
| }, [ | |
| openConnectModal, | |
| connection.isConnectedWithDappKit, | |
| connection.isInAppBrowser, | |
| dappKitDisconnect, | |
| ]); |
🤖 Prompt for AI Agents
In
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx
around lines 232-253, the handler calls dappKitDisconnect() without awaiting it
which can let openConnectModal run before disconnect finishes; update the code
to await dappKitDisconnect inside the existing try/catch (i.e., await
dappKitDisconnect();), then call openConnectModal only after the awaited
disconnect completes; remove the current setTimeout or, if a brief delay is
still required, use an awaited Promise-based sleep (await new Promise(r =>
setTimeout(r, 100))) before calling openConnectModal, and keep error logging in
the catch.
| const getStoredWallets = useCallback((): StoredWallet[] => { | ||
| if (!isBrowser()) return []; | ||
|
|
||
| const keys = getStorageKeys(network.type); | ||
| const cached = getLocalStorageItem(keys.wallets); | ||
| if (!cached) return []; | ||
| return JSON.parse(cached) as StoredWallet[]; | ||
| }, [network.type, getStorageKeys]); |
There was a problem hiding this comment.
Add error handling for JSON.parse to prevent crashes from corrupted localStorage.
If localStorage contains invalid JSON (e.g., corrupted data or manual tampering), JSON.parse will throw and crash the application.
🔎 Proposed fix
const getStoredWallets = useCallback((): StoredWallet[] => {
if (!isBrowser()) return [];
const keys = getStorageKeys(network.type);
const cached = getLocalStorageItem(keys.wallets);
if (!cached) return [];
- return JSON.parse(cached) as StoredWallet[];
+ try {
+ return JSON.parse(cached) as StoredWallet[];
+ } catch (error) {
+ console.warn('Error parsing stored wallets:', error);
+ return [];
+ }
}, [network.type, getStorageKeys]);🤖 Prompt for AI Agents
In packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts around lines 27
to 34, the call to JSON.parse on cached localStorage data can throw for
invalid/corrupted JSON; wrap the parse in a try/catch, and on parse failure
return an empty array (and optionally remove the corrupted storage key and log
the error) to prevent the app from crashing and to recover gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
36-51: The no-oponClosecallback still prevents proper modal closure.As flagged in a previous review, the empty
onClose: () => {}callback at line 45 prevents the select-wallet screen from properly handling close or cancel actions. Users have no way to navigate back from the wallet selection flow.Replace the no-op with a handler that returns to the profile view or closes the modal appropriately.
🔎 Suggested fix
} else { // Desktop: navigate to select wallet screen setCurrentContent({ type: 'select-wallet', props: { setCurrentContent, - onClose: () => {}, + onClose: () => setCurrentContent('profile'), returnTo: 'profile', onLogoutSuccess, }, }); }
🧹 Nitpick comments (2)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
120-149: Consider adding clarifying comments for the multi-branch wallet action logic.The three-way conditional rendering of wallet actions (in-app switch, desktop switch, logout) is correct but complex. Each branch handles a distinct environment and capability scenario:
- Branch A (lines 120–136): In-app browser with native wallet switching
- Branch B (lines 137–149): Desktop DappKit requiring UI-based wallet selection
- Branch C (lines 150–175): Fallback logout for other connection types
Adding brief inline comments explaining when each branch is reached would improve maintainability and make the branching logic clearer to future developers.
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (1)
66-80: Remove unnecessary optional chaining and non-null assertion.Since
setCurrentContentis now a required prop, the optional chaining on line 71 and the non-null assertion on line 74 are unnecessary. The logic correctly branches between in-app browser (direct switch) and desktop (modal navigation).🔎 Proposed cleanup
const handleSwitchWallet = () => { if (isInAppBrowser) { switchWallet(); } else { // Desktop: navigate to select wallet screen - setCurrentContent?.({ + setCurrentContent({ type: 'select-wallet', props: { - setCurrentContent: setCurrentContent!, + setCurrentContent, onClose, returnTo: 'main', }, }); } };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx
🧬 Code graph analysis (1)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (2)
packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (1)
useSwitchWallet(20-75)packages/vechain-kit/src/components/common/ModalSettingsButton.tsx (1)
ModalSettingsButton(10-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: deploy
- GitHub Check: Lint & Build
- GitHub Check: build-size-check
- GitHub Check: e2e-test
🔇 Additional comments (5)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (5)
24-24: LGTM! New hooks properly imported.The addition of
useSwitchWalletanduseDAppKitWallethooks supports the new environment-aware wallet switching functionality.
49-50: LGTM! Hooks properly integrated.The destructured values from
useSwitchWalletanduseDAppKitWalletare correctly used throughout the component to implement environment-aware wallet switching.
127-128: LGTM! Button visibility correctly handles both environments.The condition properly shows the switch wallet button for:
- In-app browser when switching is enabled
- Desktop when connected via DAppKit
This aligns with the multi-wallet UI requirements.
132-132: LGTM! onClick correctly routed through environment-aware handler.The button now invokes
handleSwitchWallet, which properly branches between direct wallet switching (in-app browser) and modal navigation (desktop).
40-43: All callers properly provide the required props.Verification confirms that
setCurrentContentandonCloseare required props forAccountSelector. The only caller,AccountMainContent, correctly passes both required props on lines 81-82. No breaking changes or missing prop issues found.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
41-56: The no-oponClosecallback remains unresolved.As flagged in a previous review, the
onClose: () => {}at line 50 prevents proper navigation when the user cancels wallet selection. When the select-wallet flow invokesonClose, nothing happens, which could leave the user in an unexpected state.🔎 Suggested fix
} else { // Desktop: navigate to select wallet screen setCurrentContent({ type: 'select-wallet', props: { setCurrentContent, - onClose: () => {}, + onClose: () => setCurrentContent('profile'), returnTo: 'profile', onLogoutSuccess, }, }); }
🧹 Nitpick comments (5)
packages/vechain-kit/src/languages/it.json (1)
279-279: Verify the intentional use of duplicate keys with different capitalization.Two similar translation keys exist with only capitalization differences:
- Line 279:
"Remove Wallet": "Rimuovi Portafoglio"- Line 281:
"Remove wallet": "Rimuovi portafoglio"While both translations are correct, this pattern is uncommon in i18n files. Please confirm whether these are intentionally maintained for different UI contexts (e.g., button labels vs menu items) or if one should be consolidated.
Also applies to: 281-281
packages/vechain-kit/src/providers/FeedbackProvider.tsx (2)
11-20: Consider exporting FeedbackContextType if needed elsewhere.The AI summary indicates
FeedbackContextTypeshould be an exported entity, but it's currently not exported. If consuming components need to reference this type directly (e.g., for advanced patterns or testing), consider addingexportto the type declaration.🔎 Proposed addition
-type FeedbackContextType = { +export type FeedbackContextType = { showFeedback: (message?: string) => void; shouldShowFeedback: boolean; message: string | null; resetFeedback: () => void; };
30-37: Consider exporting Props type and using a more specific name.The AI summary lists
Propsas an exported entity, but it's not currently exported. Additionally, the generic namePropscould be more descriptive.🔎 Proposed refinement
-type Props = { +export type FeedbackProviderProps = { children: Readonly<ReactNode>; /** * Optional callback to reset feedback when modal closes * If provided, feedback will be reset when the modal closes */ resetOnModalClose?: boolean; }; -export const FeedbackProvider = ({ +export const FeedbackProvider = ({ children, resetOnModalClose = false, -}: Props) => { +}: FeedbackProviderProps) => {packages/vechain-kit/src/components/common/WalletSwitchFeedback.tsx (1)
13-17: Documentation mentions unimplemented behavior.The JSDoc states the component "Handles both desktop (via props) and VeWorld in-app browser (via address change detection)", but only prop-based handling is implemented. Consider updating the comment to accurately reflect current behavior.
🔎 Proposed fix
/** * Component that displays inline feedback when a wallet switch occurs. - * Handles both desktop (via props) and VeWorld in-app browser (via address change detection). + * Handles wallet switch feedback on desktop via props. * Simply add this component where you want the feedback to appear. */packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
128-159: UseisInAppBrowserfromuseSwitchWalletfor consistency.At line 128, the condition uses
connection.isInAppBrowser, but the component destructuresisInAppBrowserfromuseSwitchWalletat line 38 and uses it inhandleSwitchWalletat line 42. For consistency and to match the hook-based pattern, use the destructured value throughout the component.🔎 Suggested fix
- {connection.isInAppBrowser && isSwitchWalletEnabled ? ( + {isInAppBrowser && isSwitchWalletEnabled ? ( <Button size="md" width="full"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/vechain-kit/src/components/AccountModal/AccountModal.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Account/AccountMainContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/components/AccountModal/Types/Types.tspackages/vechain-kit/src/components/common/InlineFeedback.tsxpackages/vechain-kit/src/components/common/WalletSwitchFeedback.tsxpackages/vechain-kit/src/components/common/index.tspackages/vechain-kit/src/languages/de.jsonpackages/vechain-kit/src/languages/en.jsonpackages/vechain-kit/src/languages/es.jsonpackages/vechain-kit/src/languages/fr.jsonpackages/vechain-kit/src/languages/it.jsonpackages/vechain-kit/src/languages/ja.jsonpackages/vechain-kit/src/languages/zh.jsonpackages/vechain-kit/src/providers/FeedbackProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/vechain-kit/src/languages/ja.json
- packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx
- packages/vechain-kit/src/languages/de.json
- packages/vechain-kit/src/languages/fr.json
- packages/vechain-kit/src/languages/zh.json
- packages/vechain-kit/src/languages/en.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/components/AccountModal/Contents/Account/AccountMainContent.tsxpackages/vechain-kit/src/providers/FeedbackProvider.tsxpackages/vechain-kit/src/components/common/InlineFeedback.tsxpackages/vechain-kit/src/components/common/WalletSwitchFeedback.tsxpackages/vechain-kit/src/components/AccountModal/Types/Types.tspackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/common/index.tspackages/vechain-kit/src/components/AccountModal/AccountModal.tsx
🧬 Code graph analysis (5)
packages/vechain-kit/src/providers/FeedbackProvider.tsx (1)
packages/vechain-kit/src/providers/ModalProvider.tsx (1)
useModal(73-79)
packages/vechain-kit/src/components/common/WalletSwitchFeedback.tsx (1)
packages/vechain-kit/src/components/common/InlineFeedback.tsx (1)
InlineFeedback(41-100)
packages/vechain-kit/src/components/AccountModal/Types/Types.ts (1)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/RemoveWalletConfirmContent.tsx (1)
RemoveWalletConfirmContentProps(19-25)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (5)
packages/vechain-kit/src/components/AccountModal/Types/Types.ts (1)
AccountModalContentTypes(24-135)packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
useWallet(62-410)packages/vechain-kit/src/hooks/api/wallet/useSwitchWallet.ts (1)
useSwitchWallet(20-75)packages/vechain-kit/src/components/common/ModalSettingsButton.tsx (1)
ModalSettingsButton(10-67)packages/vechain-kit/src/components/common/WalletSwitchFeedback.tsx (1)
WalletSwitchFeedback(18-45)
packages/vechain-kit/src/components/AccountModal/AccountModal.tsx (3)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (1)
SelectWalletContent(37-332)packages/vechain-kit/src/components/AccountModal/Contents/Account/AccountMainContent.tsx (1)
AccountMainContent(35-109)packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
ProfileContent(31-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint & Build
- GitHub Check: e2e-test
- GitHub Check: build-size-check
- GitHub Check: deploy
🔇 Additional comments (19)
packages/vechain-kit/src/languages/es.json (1)
22-22: LGTM! Spanish translations are accurate and consistent.All new wallet-related Spanish translations are grammatically correct, contextually appropriate, and maintain consistency with existing translations in the file. The capitalization patterns match the English source keys, and the informal "tú" form is used consistently with the rest of the file.
Also applies to: 24-24, 44-44, 210-210, 247-247, 279-279, 281-281, 294-294, 327-327
packages/vechain-kit/src/languages/it.json (2)
22-22: LGTM! Italian translations are accurate.The wallet-related translations are grammatically correct and appropriately capitalized for Italian UI labels. "Portafoglio" (wallet) is correctly used in singular and plural forms.
Also applies to: 24-24, 247-247, 294-294
44-44: LGTM! Status and confirmation messages are correctly translated.The Italian translations are accurate:
- The confirmation message uses appropriate formal phrasing
- "Caricamento..." is the standard loading indicator
- "Account cambiato" correctly uses the past participle agreeing with the masculine noun
Also applies to: 210-210, 327-327
packages/vechain-kit/src/providers/FeedbackProvider.tsx (3)
1-9: LGTM!The imports are clean and all necessary dependencies are included. The pattern of importing
useModalfrom the sibling provider is consistent with the existing codebase structure.
22-28: LGTM!The hook implementation follows the exact same pattern as
useModalfrom the codebase, ensuring consistency and proper error handling when used outside the provider context.
39-76: LGTM!The provider implementation follows React best practices:
- Proper use of
useCallbackwith stable dependencies- Correct
useEffectdependency array (includingresetFeedbackis appropriate despite it being stable)- Clean state management with separate state variables for clarity
- The conditional reset logic on modal close is well-implemented
packages/vechain-kit/src/components/common/InlineFeedback.tsx (1)
41-50: Clean component structure with proper theming.The component correctly uses Chakra UI's theming utilities (
useToken,useColorModeValue) for color mode awareness. The animation keyframes are well-defined.packages/vechain-kit/src/components/common/index.ts (1)
16-17: LGTM!New component exports follow the existing barrel export pattern.
packages/vechain-kit/src/components/AccountModal/AccountModal.tsx (2)
100-103: LGTM!The new
remove-wallet-confirmcase correctly spreads props to the content component.
135-164: Well-structured content routing with feedback support.The implementation correctly handles both object-form content types (with
switchFeedbackprops) and maintains the existing string-form cases for backward compatibility. ThereturnToandonLogoutSuccessprops are properly forwarded.packages/vechain-kit/src/components/common/WalletSwitchFeedback.tsx (1)
18-44: Implementation is correct for the prop-based feedback flow.The local state allows the feedback to be dismissed via
onCloseindependently of when the parent updates theshowFeedbackprop, which is the intended UX.packages/vechain-kit/src/components/AccountModal/Contents/Account/AccountMainContent.tsx (1)
82-84: LGTM!Clean integration of
WalletSwitchFeedback. The optional chaining correctly handles the case whenswitchFeedbackis undefined.packages/vechain-kit/src/components/AccountModal/Types/Types.ts (2)
20-22: LGTM!Clean type definition for
SwitchFeedbackthat's properly used across the modal content variants.
52-62: ThesetCurrentContentprop is not redundant and should remain in the type definition. SelectWalletContent actively uses this prop multiple times throughout the component to navigate between different modal states—including closing the modal, navigating to wallet removal confirmation, and returning to previous screens. This is a necessary prop for the component's navigation functionality, not a redundant pass-through.Likely an incorrect or invalid review comment.
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (5)
11-11: LGTM! Imports align with new wallet switching functionality.The new imports for wallet switching hooks and feedback components are properly used throughout the component.
Also applies to: 14-17
28-28: LGTM! Prop definition and usage are correct.The
switchFeedbackprop is properly typed and integrated into the component signature.Also applies to: 34-34
38-39: LGTM! Hook usage is correct.The hooks are properly called to access wallet switching functionality and state.
65-70: LGTM! Settings button is properly rendered.The unconditional rendering of the settings button is appropriate, with internal state management handled by the component itself.
77-79: LGTM! Feedback component integration is correct.The
WalletSwitchFeedbackcomponent properly receives theshowFeedbackprop with safe optional chaining, and handles undefined/false values gracefully.
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| setIsVisible(false); | ||
| setTimeout(() => { | ||
| onClose?.(); | ||
| }, 300); // Wait for animation to complete | ||
| }, duration); | ||
|
|
||
| return () => clearTimeout(timer); | ||
| }, [duration, onClose]); |
There was a problem hiding this comment.
Nested timeout may leak and invoke stale callback after unmount.
The inner setTimeout (line 55-57) is not cleaned up if the component unmounts after the outer timer fires but before the inner one completes. This can cause onClose to be invoked on an unmounted component.
🔎 Proposed fix
export const InlineFeedback = ({
message,
duration = 2000,
onClose,
}: Props) => {
const [isVisible, setIsVisible] = useState(true);
const textPrimary = useToken('colors', 'vechain-kit-text-primary');
const bgColor = useColorModeValue('#f0f9ff', '#0000009e');
const borderColor = useColorModeValue('#bfdbfe', '#3b3b3b');
const iconColor = useToken('colors', 'vechain-kit-primary');
useEffect(() => {
+ let animationTimer: ReturnType<typeof setTimeout>;
const timer = setTimeout(() => {
setIsVisible(false);
- setTimeout(() => {
+ animationTimer = setTimeout(() => {
onClose?.();
}, 300); // Wait for animation to complete
}, duration);
- return () => clearTimeout(timer);
+ return () => {
+ clearTimeout(timer);
+ clearTimeout(animationTimer);
+ };
}, [duration, onClose]);🤖 Prompt for AI Agents
In packages/vechain-kit/src/components/common/InlineFeedback.tsx around lines 52
to 61, the inner setTimeout created to delay calling onClose after the animation
can leak and invoke a stale callback if the component unmounts after the outer
timer fires; capture both timeout IDs (outer and inner) in local variables (or
refs) and clear both in the effect cleanup, and ensure you call the latest
onClose by storing it in a ref (update the ref in render) before using it in the
inner timeout so the cleanup reliably prevents invoking onClose after unmount.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
317-334: Consider consolidating or clarifying the purpose of this effect.This effect appears to handle a specific edge case for saving the stored active wallet, but its relationship to the previous effect (lines 268-313) is not immediately clear. Both effects call
saveWallet, which may lead to confusion about when and why each effect runs.Consider:
- Adding a more detailed comment explaining when this effect runs that the previous effect doesn't cover, OR
- Consolidating the logic if possible to reduce duplication
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : In VeChain Kit Version 2, use `useThor` instead of `useConnex` for contract interactions
Applied to files:
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧬 Code graph analysis (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (3)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
useWalletStorage(17-138)packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)packages/vechain-kit/src/hooks/api/wallet/useWalletMetadata.ts (1)
useWalletMetadata(10-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint & Build
- GitHub Check: e2e-test
- GitHub Check: build-size-check
- GitHub Check: deploy
🔇 Additional comments (5)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (5)
25-26: LGTM!The new imports are properly used throughout the file to support wallet storage and SSR-safe browser detection.
165-168: LGTM!The in-app browser detection properly uses optional chaining and provides a safe fallback, preventing errors in non-VeChain environments.
170-186: LGTM!The stored active wallet state management correctly handles initialization and updates. The function dependency
getActiveWalletis acceptable since it's memoized withuseCallbackin theuseWalletStoragehook, ensuring it only changes when the network type changes.
241-262: LGTM!The smart account and metadata queries correctly use
effectiveConnectedWalletAddressto respect the stored active wallet selection. The conditional enabling is already handled within theuseSmartAccounthook.
422-422: LGTM!Good refactor to use the pre-computed
isInAppBrowservariable instead of computing it inline. This ensures consistency throughout the hook and improves performance.
| // Track if a wallet switch is in progress to prevent overriding the user's selection | ||
| const [isWalletSwitchInProgress, setIsWalletSwitchInProgress] = | ||
| useState(false); | ||
|
|
||
| // Listen for wallet switch events | ||
| useEffect(() => { | ||
| if (!isBrowser() || !isConnectedWithDappKit || isInAppBrowser) return; | ||
|
|
||
| const handleWalletSwitch = ( | ||
| event: CustomEvent<{ address: string }>, | ||
| ) => { | ||
| setIsWalletSwitchInProgress(true); | ||
| setStoredActiveWalletAddress(event.detail.address); | ||
| // Reset the flag after a short delay to allow the connection to update | ||
| setTimeout(() => { | ||
| setIsWalletSwitchInProgress(false); | ||
| }, 1000); | ||
| }; | ||
|
|
||
| window.addEventListener( | ||
| 'wallet_switched', | ||
| handleWalletSwitch as EventListener, | ||
| ); | ||
| return () => { | ||
| window.removeEventListener( | ||
| 'wallet_switched', | ||
| handleWalletSwitch as EventListener, | ||
| ); | ||
| }; | ||
| }, [isConnectedWithDappKit, isInAppBrowser]); |
There was a problem hiding this comment.
Clean up the setTimeout to prevent memory leaks and state updates on unmounted components.
The timeout set on line 202 is not cleaned up when the component unmounts or when the effect re-runs. This can cause state updates on an unmounted component.
🔎 Proposed fix
// Listen for wallet switch events
useEffect(() => {
if (!isBrowser() || !isConnectedWithDappKit || isInAppBrowser) return;
+ let timeoutId: NodeJS.Timeout | null = null;
+
const handleWalletSwitch = (
event: CustomEvent<{ address: string }>,
) => {
setIsWalletSwitchInProgress(true);
setStoredActiveWalletAddress(event.detail.address);
// Reset the flag after a short delay to allow the connection to update
- setTimeout(() => {
+ timeoutId = setTimeout(() => {
setIsWalletSwitchInProgress(false);
+ timeoutId = null;
}, 1000);
};
window.addEventListener(
'wallet_switched',
handleWalletSwitch as EventListener,
);
return () => {
+ if (timeoutId) {
+ clearTimeout(timeoutId);
+ }
window.removeEventListener(
'wallet_switched',
handleWalletSwitch as EventListener,
);
};
}, [isConnectedWithDappKit, isInAppBrowser]);📝 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.
| // Track if a wallet switch is in progress to prevent overriding the user's selection | |
| const [isWalletSwitchInProgress, setIsWalletSwitchInProgress] = | |
| useState(false); | |
| // Listen for wallet switch events | |
| useEffect(() => { | |
| if (!isBrowser() || !isConnectedWithDappKit || isInAppBrowser) return; | |
| const handleWalletSwitch = ( | |
| event: CustomEvent<{ address: string }>, | |
| ) => { | |
| setIsWalletSwitchInProgress(true); | |
| setStoredActiveWalletAddress(event.detail.address); | |
| // Reset the flag after a short delay to allow the connection to update | |
| setTimeout(() => { | |
| setIsWalletSwitchInProgress(false); | |
| }, 1000); | |
| }; | |
| window.addEventListener( | |
| 'wallet_switched', | |
| handleWalletSwitch as EventListener, | |
| ); | |
| return () => { | |
| window.removeEventListener( | |
| 'wallet_switched', | |
| handleWalletSwitch as EventListener, | |
| ); | |
| }; | |
| }, [isConnectedWithDappKit, isInAppBrowser]); | |
| // Track if a wallet switch is in progress to prevent overriding the user's selection | |
| const [isWalletSwitchInProgress, setIsWalletSwitchInProgress] = | |
| useState(false); | |
| // Listen for wallet switch events | |
| useEffect(() => { | |
| if (!isBrowser() || !isConnectedWithDappKit || isInAppBrowser) return; | |
| let timeoutId: NodeJS.Timeout | null = null; | |
| const handleWalletSwitch = ( | |
| event: CustomEvent<{ address: string }>, | |
| ) => { | |
| setIsWalletSwitchInProgress(true); | |
| setStoredActiveWalletAddress(event.detail.address); | |
| // Reset the flag after a short delay to allow the connection to update | |
| timeoutId = setTimeout(() => { | |
| setIsWalletSwitchInProgress(false); | |
| timeoutId = null; | |
| }, 1000); | |
| }; | |
| window.addEventListener( | |
| 'wallet_switched', | |
| handleWalletSwitch as EventListener, | |
| ); | |
| return () => { | |
| if (timeoutId) { | |
| clearTimeout(timeoutId); | |
| } | |
| window.removeEventListener( | |
| 'wallet_switched', | |
| handleWalletSwitch as EventListener, | |
| ); | |
| }; | |
| }, [isConnectedWithDappKit, isInAppBrowser]); |
🤖 Prompt for AI Agents
In packages/vechain-kit/src/hooks/api/wallet/useWallet.ts around lines 188 to
217, the setTimeout used to reset isWalletSwitchInProgress is not cleaned up
which can cause state updates after unmount; store the timeout ID (e.g., in a
ref or local variable) when calling setTimeout, clear that timeout in the effect
cleanup (and before setting a new one when handler runs again), and guard the
state update by ensuring the component is still mounted (or simply rely on
clearing the timeout) so no setState runs after unmount.
| // Always prioritize the stored active wallet from cache when switching | ||
| // Use connected wallet when: | ||
| // 1. No stored active wallet exists (new connection) | ||
| // 2. Connected wallet is not in stored wallets list (new wallet after disconnect) | ||
| // 3. A switch is NOT in progress AND connected wallet differs from stored (reconnection with different wallet) | ||
| const storedWallets = getStoredWallets(); | ||
| const isConnectedWalletInStoredList = storedWallets.some( | ||
| (w) => | ||
| w.address.toLowerCase() === connectedWalletAddress?.toLowerCase(), | ||
| ); | ||
|
|
||
| const effectiveConnectedWalletAddress = | ||
| // If switch is in progress, always use stored active wallet | ||
| isWalletSwitchInProgress && storedActiveWalletAddress | ||
| ? storedActiveWalletAddress | ||
| : // If stored active wallet exists and connected wallet is in stored list, use stored (switch scenario) | ||
| storedActiveWalletAddress && isConnectedWalletInStoredList | ||
| ? storedActiveWalletAddress | ||
| : // Otherwise use connected wallet (new connection or reconnection with different wallet) | ||
| connectedWalletAddress; |
There was a problem hiding this comment.
Memoize getStoredWallets() to avoid reading localStorage on every render.
Line 224 calls getStoredWallets() on every render, which reads from localStorage. This is a performance concern, especially as the component re-renders frequently due to wallet state changes.
🔎 Proposed fix using useMemo
+// Memoize stored wallets to avoid reading from localStorage on every render
+const storedWallets = useMemo(() => {
+ if (isConnectedWithDappKit && !isInAppBrowser) {
+ return getStoredWallets();
+ }
+ return [];
+}, [isConnectedWithDappKit, isInAppBrowser, getStoredWallets, connectedWalletAddress]);
+
-const storedWallets = getStoredWallets();
const isConnectedWalletInStoredList = storedWallets.some(
(w) =>
w.address.toLowerCase() === connectedWalletAddress?.toLowerCase(),
);Note: Include connectedWalletAddress as a dependency to refresh when the wallet changes, ensuring the list stays current.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/vechain-kit/src/hooks/api/wallet/useWallet.ts around lines 219 to
238, getStoredWallets() is called on every render which repeatedly reads
localStorage; wrap the call in a useMemo (e.g., const storedWallets = useMemo(()
=> getStoredWallets(), [connectedWalletAddress]) ) so the stored wallets list is
computed only when connectedWalletAddress changes (or other relevant deps), and
then use the memoized storedWallets in the subsequent logic to avoid unnecessary
localStorage reads.
| useEffect(() => { | ||
| if ( | ||
| isConnectedWithDappKit && | ||
| !isInAppBrowser && | ||
| connectedWalletAddress && | ||
| activeAccountMetadata && | ||
| !activeAccountMetadata.isLoading | ||
| ) { | ||
| // First try to initialize (only saves if no wallets exist and sets as active) | ||
| initializeCurrentWallet(connectedWalletAddress); | ||
| // Always save/update the wallet (in case it already exists or is a new connection) | ||
| saveWallet(connectedWalletAddress); | ||
|
|
||
| // Check if this is a new connection or a switch | ||
| // When switching, storedActiveWalletAddress is updated immediately via wallet_switched event | ||
| // and isWalletSwitchInProgress is set to true | ||
| // We should NOT override the stored active wallet when switching | ||
| const isNewConnection = !storedActiveWalletAddress; | ||
| const isSameAsStoredActive = | ||
| storedActiveWalletAddress && | ||
| storedActiveWalletAddress.toLowerCase() === | ||
| connectedWalletAddress.toLowerCase(); | ||
|
|
||
| // Only set as active if: | ||
| // 1. It's a new connection (no stored active wallet), OR | ||
| // 2. The connected wallet matches the stored active wallet (same wallet, just ensuring it's saved), AND | ||
| // 3. A wallet switch is NOT in progress (to prevent overriding user's selection during switch) | ||
| if ( | ||
| (isNewConnection || isSameAsStoredActive) && | ||
| !isWalletSwitchInProgress | ||
| ) { | ||
| setActiveWalletStorage(connectedWalletAddress); | ||
| } | ||
| } | ||
| }, [ | ||
| isConnectedWithDappKit, | ||
| isInAppBrowser, | ||
| connectedWalletAddress, | ||
| activeAccountMetadata?.domain, | ||
| activeAccountMetadata?.image, | ||
| activeAccountMetadata?.isLoading, | ||
| initializeCurrentWallet, | ||
| saveWallet, | ||
| setActiveWalletStorage, | ||
| storedActiveWalletAddress, | ||
| ]); |
There was a problem hiding this comment.
Add missing dependency and fix granular dependency issues.
Two issues with this effect:
-
Critical:
isWalletSwitchInProgressis used on line 297 but is missing from the dependency array. This causes a stale closure bug where the effect always sees the initial value of the flag. -
Major: The dependency array includes granular properties (
activeAccountMetadata?.domain,activeAccountMetadata?.image,activeAccountMetadata?.isLoading). This is fragile and can cause the effect to run unnecessarily or miss important updates. React best practice is to depend on the whole object or use a more stable dependency.
🔎 Proposed fix
}, [
isConnectedWithDappKit,
isInAppBrowser,
connectedWalletAddress,
- activeAccountMetadata?.domain,
- activeAccountMetadata?.image,
- activeAccountMetadata?.isLoading,
+ activeAccountMetadata.isLoading,
initializeCurrentWallet,
saveWallet,
setActiveWalletStorage,
storedActiveWalletAddress,
+ isWalletSwitchInProgress,
]);Alternatively, you could add a condition inside the effect to check isLoading before proceeding:
useEffect(() => {
if (
isConnectedWithDappKit &&
!isInAppBrowser &&
connectedWalletAddress &&
- activeAccountMetadata &&
- !activeAccountMetadata.isLoading
+ !activeAccountMetadata.isLoading
) {
// ... rest of logic
}
}, [
isConnectedWithDappKit,
isInAppBrowser,
connectedWalletAddress,
+ activeAccountMetadata.isLoading,
initializeCurrentWallet,
saveWallet,
setActiveWalletStorage,
storedActiveWalletAddress,
+ isWalletSwitchInProgress,
]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/vechain-kit/src/hooks/api/wallet/useWallet.ts around lines 268 to
313, the effect is missing isWalletSwitchInProgress in its dependency array
(causing a stale-closure bug) and uses fragile granular deps
activeAccountMetadata?.domain, activeAccountMetadata?.image,
activeAccountMetadata?.isLoading; update the dependency array to include
isWalletSwitchInProgress and replace the granular activeAccountMetadata
properties with the full activeAccountMetadata object (i.e., use
activeAccountMetadata) so the effect responds correctly to metadata changes and
the wallet-switch flag.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (3)
198-223: Clean up the setTimeout to prevent memory leaks and state updates on unmounted components.The timeout set on line 208 is not cleaned up when the component unmounts or when the effect re-runs. This can cause state updates on an unmounted component.
🔎 Proposed fix
// Listen for wallet switch events useEffect(() => { if (!isBrowser() || !isConnectedWithDappKit || isInAppBrowser) return; + let timeoutId: NodeJS.Timeout | null = null; + const handleWalletSwitch = ( event: CustomEvent<{ address: string }>, ) => { setIsWalletSwitchInProgress(true); setStoredActiveWalletAddress(event.detail.address); // Reset the flag after a short delay to allow the connection to update - setTimeout(() => { + timeoutId = setTimeout(() => { setIsWalletSwitchInProgress(false); + timeoutId = null; }, 1000); }; window.addEventListener( 'wallet_switched', handleWalletSwitch as EventListener, ); return () => { + if (timeoutId) { + clearTimeout(timeoutId); + } window.removeEventListener( 'wallet_switched', handleWalletSwitch as EventListener, ); }; }, [isConnectedWithDappKit, isInAppBrowser]);
230-230: Memoize getStoredWallets() to avoid reading localStorage on every render.Line 230 calls
getStoredWallets()on every render, which reads from localStorage. This is a performance concern, especially as the component re-renders frequently due to wallet state changes.🔎 Proposed fix using useMemo
+// Memoize stored wallets to avoid reading from localStorage on every render +const storedWallets = useMemo(() => { + if (isConnectedWithDappKit && !isInAppBrowser) { + return getStoredWallets(); + } + return []; +}, [isConnectedWithDappKit, isInAppBrowser, getStoredWallets, connectedWalletAddress]); + -const storedWallets = getStoredWallets(); const isConnectedWalletInStoredList = storedWallets.some( (w) => w.address.toLowerCase() === connectedWalletAddress?.toLowerCase(), );Note: Include
connectedWalletAddressas a dependency to refresh when the wallet changes.
274-319: Add missing dependency and fix granular dependency issues.Two issues with this effect:
Critical:
isWalletSwitchInProgressis used on line 303 but is missing from the dependency array (lines 308-319). This causes a stale closure bug where the effect always sees the initial value of the flag.Major: The dependency array includes granular properties (
activeAccountMetadata?.domain,activeAccountMetadata?.image,activeAccountMetadata?.isLoading) on lines 312-314. This is fragile and can cause the effect to run unnecessarily or miss important updates.🔎 Proposed fix
}, [ isConnectedWithDappKit, isInAppBrowser, connectedWalletAddress, - activeAccountMetadata?.domain, - activeAccountMetadata?.image, - activeAccountMetadata?.isLoading, + activeAccountMetadata.isLoading, initializeCurrentWallet, saveWallet, setActiveWalletStorage, storedActiveWalletAddress, + isWalletSwitchInProgress, ]);
🧹 Nitpick comments (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
321-340: Consider consolidating wallet save logic to avoid redundant storage writes.This effect saves the wallet when
storedActiveWalletAddresschanges and matcheseffectiveConnectedWalletAddress. The previous effect (lines 274-319) already handles saving wallets on connection. This might lead to redundantlocalStoragewrites.Consider whether this second effect is necessary or if the logic can be consolidated into the first effect.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧠 Learnings (1)
📚 Learning: 2025-12-01T13:01:33.771Z
Learnt from: CR
Repo: vechain/vechain-kit PR: 0
File: .cursor/rules/migration-guide-to-v2.mdc:0-0
Timestamp: 2025-12-01T13:01:33.771Z
Learning: Applies to **/*.{ts,tsx} : In VeChain Kit Version 2, use `useThor` instead of `useConnex` for contract interactions
Applied to files:
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧬 Code graph analysis (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (5)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
useWalletStorage(17-138)packages/vechain-kit/src/hooks/utils/useGetNodeUrl.ts (1)
useGetNodeUrl(4-8)packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)packages/vechain-kit/src/hooks/thor/smartAccounts/useSmartAccount.ts (1)
useSmartAccount(51-60)packages/vechain-kit/src/hooks/api/wallet/useWalletMetadata.ts (1)
useWalletMetadata(10-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint & Build
- GitHub Check: deploy
- GitHub Check: build-size-check
- GitHub Check: e2e-test
🔇 Additional comments (4)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (4)
79-84: LGTM!Clean integration of the wallet storage hook. The destructured functions are appropriately used throughout the component for wallet persistence.
88-92: LGTM!The in-app browser detection is well-implemented with SSR safety via
isBrowser()guard and appropriate memoization.
247-249: LGTM!The usage of
effectiveConnectedWalletAddressis consistent and properly integrated with smart account retrieval and metadata fetching.Also applies to: 253-253, 262-263
428-428: LGTM!Exposing
isInAppBrowseras part of the connection state is consistent with the pattern and provides useful context to consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/homepage/i18n.tsexamples/homepage/src/app/languages/en.jsonpackages/vechain-kit/src/providers/VeChainKitProvider.tsx
💤 Files with no reviewable changes (1)
- packages/vechain-kit/src/providers/VeChainKitProvider.tsx
✅ Files skipped from review due to trivial changes (1)
- examples/homepage/src/app/languages/en.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
examples/homepage/i18n.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-test
- GitHub Check: Lint & Build
- GitHub Check: deploy
- GitHub Check: build-size-check
| if (typeof window === 'undefined') return 'en'; | ||
|
|
||
| if (stored && supportedLanguages.includes(stored)) return stored; | ||
| if (browser && supportedLanguages.includes(browser)) return browser; | ||
| } | ||
| const stored = localStorage.getItem('i18nextLng'); | ||
| const browser = navigator.language?.split('-')[0]; | ||
|
|
||
| if (stored && supportedLanguages.includes(stored)) return stored; | ||
| if (browser && supportedLanguages.includes(browser)) return browser; | ||
|
|
||
| return 'en'; // fallback |
There was a problem hiding this comment.
Wrap localStorage access in try-catch to prevent initialization failures.
The localStorage.getItem() call on line 44 can throw exceptions in privacy mode, when storage is disabled, or in restricted security contexts. Since this function runs during i18n initialization, an unhandled exception would break the entire language detection system.
🔎 Proposed fix to add exception handling
if (typeof window === 'undefined') return 'en';
-const stored = localStorage.getItem('i18nextLng');
+let stored: string | null = null;
+try {
+ stored = localStorage.getItem('i18nextLng');
+} catch {
+ // Storage access blocked - fall through to browser detection
+}
const browser = navigator.language?.split('-')[0];
if (stored && supportedLanguages.includes(stored)) return stored;
if (browser && supportedLanguages.includes(browser)) return browser;Note: The SSR guard on line 42 and optional chaining on line 45 for navigator.language are well-handled.
📝 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 (typeof window === 'undefined') return 'en'; | |
| if (stored && supportedLanguages.includes(stored)) return stored; | |
| if (browser && supportedLanguages.includes(browser)) return browser; | |
| } | |
| const stored = localStorage.getItem('i18nextLng'); | |
| const browser = navigator.language?.split('-')[0]; | |
| if (stored && supportedLanguages.includes(stored)) return stored; | |
| if (browser && supportedLanguages.includes(browser)) return browser; | |
| return 'en'; // fallback | |
| if (typeof window === 'undefined') return 'en'; | |
| let stored: string | null = null; | |
| try { | |
| stored = localStorage.getItem('i18nextLng'); | |
| } catch { | |
| // Storage access blocked - fall through to browser detection | |
| } | |
| const browser = navigator.language?.split('-')[0]; | |
| if (stored && supportedLanguages.includes(stored)) return stored; | |
| if (browser && supportedLanguages.includes(browser)) return browser; | |
| return 'en'; // fallback |
🤖 Prompt for AI Agents
In examples/homepage/i18n.ts around lines 42 to 50,
localStorage.getItem('i18nextLng') can throw (e.g., in privacy mode), so wrap
the access in a try-catch: attempt to read localStorage inside the try, assign
to a local variable only if read succeeds, and on any error silently ignore and
continue to the browser/‘en’ fallbacks; ensure you do not change the existing
SSR guard or navigator optional chaining and preserve supportedLanguages checks.
2 participants
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.