Fix/add new wallet#570
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntegrates DAppKit wallet modal into the add-wallet flow, adds a 200ms wallet-polling hash to detect external wallet changes, emits a Changes
Sequence DiagramsequenceDiagram
actor User
participant SelectWallet as SelectWalletContent
participant DAppKit as DAppKit Modal
participant Polling as Wallet Polling (200ms)
participant Storage as useWalletStorage
participant Hook as useWallet
participant App as App State
User->>SelectWallet: Click "Add Wallet"
SelectWallet->>DAppKit: openDappKitModal()
Note over DAppKit: User approves/connects wallet in modal
DAppKit-->>SelectWallet: Modal closes (wallet added)
rect `#E6F2FF`
Polling->>Polling: every 200ms compute simpleHash(storedWallets)
alt Hash changed (new wallet)
Polling->>SelectWallet: trigger refreshWallets()
SelectWallet->>Storage: getStoredWallets()
Storage-->>SelectWallet: return updated list
SelectWallet->>Hook: update active/other wallets
end
end
rect `#FFF0E6`
User->>App: Remove wallet
App->>Storage: removeWallet(address)
Storage-->>Storage: update storage
Storage->>Hook: dispatch `wallet_removed` CustomEvent
Hook->>Hook: add to recentlyRemovedWalletsRef (5s cooldown)
end
rect `#E8FFE6`
Hook->>Hook: on new connection compare connected vs getStoredWallets()
alt New wallet detected
Hook->>Storage: setActiveWalletStorage(...)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
113-119: Redundant browser check can be removed.The
isBrowser()check on line 115 is redundant since line 94 already returns early when not in a browser environment. The event dispatch can be simplified.🔎 Proposed simplification
- // Dispatch event to notify that a wallet was removed - // This prevents useWallet from setting it as active again if it's still connected - if (isBrowser()) { - window.dispatchEvent( - new CustomEvent('wallet_removed', { detail: { address } }), - ); - } + // Dispatch event to notify that a wallet was removed + // This prevents useWallet from setting it as active again if it's still connected + window.dispatchEvent( + new CustomEvent('wallet_removed', { detail: { address } }), + );packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (1)
87-114: Consider event-driven updates instead of polling.The 200ms polling mechanism works but could be optimized by using a custom event (e.g.,
wallet_added) dispatched when a new wallet is added, similar to the existingwallet_removedandwallet_switchedevents. This would eliminate continuous polling and improve efficiency.💡 Event-driven alternative
Instead of polling, dispatch a
wallet_addedevent inuseWalletStorage.tswhensaveWalletadds a new wallet:In
useWalletStorage.ts:if (existingIndex < 0) { wallets.push(walletToSave); // Dispatch event for new wallet if (isBrowser()) { window.dispatchEvent( new CustomEvent('wallet_added', { detail: { address } }) ); } }Then replace the polling with an event listener in this file, similar to the existing
wallet_switchedlistener on lines 68-85.
📜 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 (5)
packages/vechain-kit/package.jsonpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/hooks/api/wallet/useWallet.tspackages/vechain-kit/src/hooks/api/wallet/useWalletStorage.tspackages/vechain-kit/src/providers/VeChainKitProvider.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/hooks/api/wallet/useWalletStorage.tspackages/vechain-kit/src/providers/VeChainKitProvider.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/hooks/api/wallet/useWallet.ts
🧠 Learnings (2)
📓 Common learnings
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
📚 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/providers/VeChainKitProvider.tsx
🧬 Code graph analysis (3)
packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts (1)
packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)
packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (1)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
useWallet(62-493)
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (1)
packages/vechain-kit/src/utils/ssrUtils.ts (1)
isBrowser(75-77)
⏰ 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: build-size-check
- GitHub Check: deploy
- GitHub Check: Lint & Build
- GitHub Check: e2e-test
🔇 Additional comments (6)
packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
694-694: LGTM! Aligns with PR objectives.Setting
alwaysShowConnect={true}ensures the DAppKit connect UI is always visible, supporting the new wallet modal flow where the DAppKit modal is opened directly for adding wallets.packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx (1)
19-19: LGTM! Modal flow simplified.The change to directly open the DAppKit modal via
useDAppKitWalletModalandopenDappKitModal()simplifies the flow and aligns with the PR objective to bypass the custom ConnectModal. This addresses the issue with closing the modal after login.Also applies to: 46-46, 271-273
packages/vechain-kit/src/hooks/api/wallet/useWallet.ts (3)
277-308: LGTM! Removal tracking prevents unwanted re-activation.The
recentlyRemovedWalletsReftracking with a 5-second grace period effectively prevents the useWallet hook from re-activating wallets that were just removed by the user. The event listener correctly responds to thewallet_removedevent dispatched fromuseWalletStorage.ts.
320-336: LGTM! Correct new wallet detection.Checking
isNewWalletagainst the stored wallets list before saving ensures accurate detection of newly added wallets. The guard against recently removed wallets prevents the user experience issue mentioned in the PR.
236-249: LGTM! Improved wallet activation logic.The changes correctly handle wallet activation across multiple scenarios:
- New wallets are automatically activated for better UX
- New connections (no stored active wallet) are activated
- Reconnections with the stored active wallet maintain state
- Wallet switches in progress are not overridden
Reading
currentStoredActiveWalletdirectly from storage on each render avoids race conditions with state updates.Also applies to: 352-362
packages/vechain-kit/package.json (1)
67-67: This is a release candidate, not a stable release—confirm the team intends to use pre-release code.The version
2.1.0-rc.5exists on npm and is not deprecated, but it's currently the latest published version with no stable2.1.0release available yet. The 2.1.0 series consists only of release candidates (rc.1–rc.5), with rc.5 published on 2025-12-23. Before merging, confirm the team is aware this introduces a pre-release dependency and review the release notes for any known issues or breaking changes.
|
Size Change: +14.4 kB (+0.25%) Total Size: 5.76 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Pull request overview
This PR enhances multi-wallet functionality by upgrading to a newer version of dapp-kit-react that improves modal visibility, streamlining the "add wallet" flow, and implementing safeguards to prevent recently removed wallets from being automatically re-activated.
Key Changes
- Updated @vechain/dapp-kit-react from rc.4 to rc.5 and enabled
alwaysShowConnectprop for better modal visibility - Simplified "add wallet" flow to directly open DAppKit modal instead of custom ConnectModal
- Added event-based tracking system with 5-second buffer to prevent removed wallets from being re-activated
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vechain-kit/package.json | Updated @vechain/dapp-kit-react dependency to rc.5 in both dependencies and peerDependencies |
| packages/vechain-kit/src/providers/VeChainKitProvider.tsx | Added alwaysShowConnect prop to DAppKitProvider for improved modal visibility |
| packages/vechain-kit/src/hooks/api/wallet/useWalletStorage.ts | Added wallet_removed custom event dispatch when removing wallets |
| packages/vechain-kit/src/hooks/api/wallet/useWallet.ts | Implemented recently-removed-wallets tracking, updated wallet activation logic to prioritize new wallets, and enhanced storage consistency checks |
| packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsx | Simplified add wallet handler to directly open DAppKit modal and added 200ms polling to detect wallet changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| refreshWallets(); | ||
| } | ||
| }, 200); // Check every 200ms |
There was a problem hiding this comment.
The 200ms polling interval is aggressive and could impact performance with unnecessary re-renders. Consider increasing the interval to 500-1000ms, or implementing a more event-driven approach using the existing custom events like 'wallet_removed' to trigger updates only when needed.
| }, 200); // Check every 200ms | |
| }, 800); // Check every 800ms |
| recentlyRemovedWalletsRef.current.delete( | ||
| event.detail.address.toLowerCase(), | ||
| ); | ||
| }, 5000); |
There was a problem hiding this comment.
The 5-second buffer timeout is a magic number without clear justification. Consider extracting this as a named constant (e.g., WALLET_REMOVAL_BUFFER_MS) with a comment explaining why this specific duration was chosen.
There was a problem hiding this comment.
Actionable comments posted: 0
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/AccountModal/Contents/Profile/ProfileContent.tsx (1)
41-56: Dead code: the in-app browser branch is unreachable.The
handleSwitchWalletfunction includes logic to handle in-app browser scenarios (lines 42-43), but this function is only called from the second Switch button condition (line 161) which explicitly checks!connection.isInAppBrowser. This means the in-app browser branch will never execute.Consider refactoring to either:
- Remove the in-app browser logic from
handleSwitchWalletif it's meant only for desktop- Use
handleSwitchWalletconsistently for both Switch button conditions (lines 141 and 161) to eliminate code duplication🔎 Option 1: Remove dead code and clarify function purpose
const handleSwitchWallet = () => { - if (isInAppBrowser) { - switchWallet(); - } else { - // Desktop: navigate to select wallet screen - setCurrentContent({ - type: 'select-wallet', - props: { - setCurrentContent, - onClose: () => {}, - returnTo: 'profile', - onLogoutSuccess, - }, - }); - } + // Desktop: navigate to select wallet screen + setCurrentContent({ + type: 'select-wallet', + props: { + setCurrentContent, + onClose: () => {}, + returnTo: 'profile', + onLogoutSuccess, + }, + }); };Option 2: Use handleSwitchWallet for both conditions and consolidate logic
Update
handleSwitchWalletto use theconnection.isInAppBrowserand add theisSwitchWalletEnabledcheck:const handleSwitchWallet = () => { - if (isInAppBrowser) { - switchWallet(); + if (connection.isInAppBrowser && isSwitchWalletEnabled) { + await switchWallet(); } else { // Desktop: navigate to select wallet screen setCurrentContent({Then use it for the first Switch button as well:
- onClick={async () => { - await switchWallet(); - // For VeWorld in-app browser, the wallet_switched event will be dispatched - // by the dapp-kit when the wallet actually changes - }} + onClick={handleSwitchWallet}Note: This would require making
handleSwitchWalletasync if you choose option 2.
🧹 Nitpick comments (1)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (1)
127-129: Consider extracting complex condition to improve readability.The boolean condition spanning lines 127-129 determines when to show the switch wallet button versus the copy address button. While correct, the logic could be more maintainable if extracted to a descriptive variable.
🔎 Suggested refactor
+ const shouldShowSwitchWallet = + (connection.isInAppBrowser && isSwitchWalletEnabled) || + (!connection.isInAppBrowser && connection.isConnectedWithDappKit); + return ( <HStack mt={mt} w={'full'} {...style} justifyContent={'flex-start'} alignItems={'center'} > {/* ... Button component ... */} - {(connection.isInAppBrowser && isSwitchWalletEnabled) || - (!connection.isInAppBrowser && - connection.isConnectedWithDappKit) ? ( + {shouldShowSwitchWallet ? ( <IconButton aria-label="Switch wallet"
📜 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
⏰ 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: build-size-check
- GitHub Check: Lint & Build
- GitHub Check: deploy
- GitHub Check: e2e-test
🔇 Additional comments (3)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (1)
48-50: No issue found:isInAppBrowservalues are already synchronized.The
isInAppBrowserexported byuseSwitchWallet()is directly derived fromconnection.isInAppBrowserreturned byuseWallet()(see useSwitchWallet.ts line 21:const isInAppBrowser = connection.isInAppBrowser). Both the rendering condition on line 127 and the handler on line 66 reference the same source value—one uses the direct property and the other uses the exported alias. No inconsistency exists and no changes are needed.Likely an incorrect or invalid review comment.
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (2)
152-153: Condition change correctly separates desktop from in-app browser scenarios.The updated condition properly ensures the desktop Switch button only appears when NOT in the in-app browser and connected with DappKit. This prevents overlap with the in-app browser Switch button condition and correctly implements the three-way branching logic described in the comment.
133-165: No action needed. The code is correct as written.The in-app and desktop Switch buttons intentionally use different conditions:
- In-app (line 133):
connection.isInAppBrowser && isSwitchWalletEnabledgates on feature availability- Desktop (lines 152-153):
!connection.isInAppBrowser && connection.isConnectedWithDappKitgates on connection typeThis difference is intentional, reflecting different capability requirements for each environment.
Additionally,
connection.isInAppBrowserandisInAppBrowserfromuseSwitchWalletare consistent—the latter derives directly from the former (line 30 of useSwitchWallet.ts), so they are guaranteed to match.
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 (4)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.tsxpackages/vechain-kit/src/utils/stringUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx
- packages/vechain-kit/src/components/AccountModal/Contents/SelectWallet/SelectWalletContent.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/utils/stringUtils.tspackages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx
⏰ 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 (2)
packages/vechain-kit/src/components/AccountModal/Components/AccountSelector.tsx (2)
77-77: LGTM!Adding the
onLogoutSuccesscallback ensures the modal closes after a successful logout from the select-wallet screen.
128-130: Verify the conditional logic change aligns with intended behavior.The switch wallet button rendering condition has been made more restrictive. For in-app browsers with a DappKit connection, the
isSwitchWalletEnabledflag from the DappKit library must now also be true, whereas on desktop, onlyisConnectedWithDappKitis required. This same pattern is applied consistently across both AccountSelector.tsx and ProfileContent.tsx, which suggests it is intentional. However, sinceisSwitchWalletEnabledoriginates from the external@vechain/dapp-kit-reactlibrary, the exact semantics and implications of this flag require verification against the DappKit library behavior and the PR description.
| export const simpleHash = (input: string): string => { | ||
| let hash = 0; | ||
| for (let i = 0; i < input.length; i++) { | ||
| const char = input.charCodeAt(i); | ||
| hash = (hash << 5) - hash + char; | ||
| hash = hash & hash; // Convert to 32bit integer | ||
| } | ||
| return hash.toString(36); | ||
| }; |
There was a problem hiding this comment.
Fix incorrect 32-bit integer coercion.
Line 24 uses hash & hash, which is an identity operation and does not convert the value to a 32-bit integer. This defeats the purpose of the comment and may lead to incorrect hash values or overflow issues.
🔎 Proposed fix
export const simpleHash = (input: string): string => {
let hash = 0;
for (let i = 0; i < input.length; i++) {
const char = input.charCodeAt(i);
hash = (hash << 5) - hash + char;
- hash = hash & hash; // Convert to 32bit integer
+ hash = hash | 0; // Convert to 32bit integer
}
return hash.toString(36);
};📝 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.
| export const simpleHash = (input: string): string => { | |
| let hash = 0; | |
| for (let i = 0; i < input.length; i++) { | |
| const char = input.charCodeAt(i); | |
| hash = (hash << 5) - hash + char; | |
| hash = hash & hash; // Convert to 32bit integer | |
| } | |
| return hash.toString(36); | |
| }; | |
| export const simpleHash = (input: string): string => { | |
| let hash = 0; | |
| for (let i = 0; i < input.length; i++) { | |
| const char = input.charCodeAt(i); | |
| hash = (hash << 5) - hash + char; | |
| hash = hash | 0; // Convert to 32bit integer | |
| } | |
| return hash.toString(36); | |
| }; |
🤖 Prompt for AI Agents
In packages/vechain-kit/src/utils/stringUtils.ts around lines 19 to 27, the
current line "hash = hash & hash" is an identity operation and does not coerce
the value to a 32-bit integer; replace it with a proper 32-bit coercion (for
example "hash = hash | 0" or "hash |= 0") so the value is truncated to a signed
32-bit integer as intended, keep the rest of the loop and return unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (1)
109-112: Consider consolidating duplicate disconnect-confirm flows.The disconnect-confirm navigation pattern appears in multiple locations:
- ProfileContent footer Logout button (lines 175-186)
- AddressDisplay Logout button (packages/vechain-kit/src/components/common/AddressDisplay.tsx, lines 182-189)
- ProfileCard's onLogout callback (lines 109-112) differs slightly by directly calling disconnect
Both the AddressDisplay and ProfileContent footer buttons trigger nearly identical disconnect-confirm flows. Consider extracting this into a shared helper function to reduce duplication and improve maintainability.
💡 Suggested refactoring approach
Create a shared helper function:
const handleLogoutConfirm = ( setCurrentContent: React.Dispatch<React.SetStateAction<AccountModalContentTypes>>, onDisconnect: () => void, returnTo: AccountModalContentTypes = 'profile' ) => { setCurrentContent({ type: 'disconnect-confirm', props: { onDisconnect, onBack: () => setCurrentContent(returnTo), }, }); };Then use it in both locations:
// In ProfileContent onClick={() => handleLogoutConfirm(setCurrentContent, () => { disconnect(); onLogoutSuccess?.(); })} // In AddressDisplay (via prop or context) onClick={() => handleLogoutConfirm(setCurrentContent, () => onLogout?.())}Also applies to: 175-186
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/Components/ProfileCard/ProfileCard.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/common/AddressDisplay.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/Contents/Profile/ProfileContent.tsxpackages/vechain-kit/src/components/common/AddressDisplay.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/Components/ProfileCard/ProfileCard.tsx
🧬 Code graph analysis (1)
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (2)
packages/vechain-kit/src/hooks/api/wallet/useTotalBalance.ts (1)
useTotalBalance(13-44)packages/vechain-kit/src/components/WalletButton/AssetIcons.tsx (1)
AssetIcons(27-129)
⏰ 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: deploy
- GitHub Check: build-size-check
🔇 Additional comments (6)
packages/vechain-kit/src/components/common/AddressDisplay.tsx (1)
20-20: LGTM: onLogout prop addition.The addition of the optional
onLogoutcallback prop is implemented correctly and follows TypeScript best practices.Also applies to: 36-36, 45-45
packages/vechain-kit/src/components/AccountModal/Contents/Profile/Components/ProfileCard/ProfileCard.tsx (1)
21-21: LGTM: Clean prop threading.The
onLogoutprop is correctly threaded fromProfileCardtoAddressDisplay. The implementation is straightforward and maintains type safety.Also applies to: 39-39, 197-197
packages/vechain-kit/src/components/AccountModal/Contents/Profile/ProfileContent.tsx (4)
10-10: LGTM: Import additions.All new imports are appropriately used within the component for the balance display and wallet switching functionality.
Also applies to: 12-17, 28-28
47-49: LGTM: Balance hook integration.The
useTotalBalancehook is properly integrated to support the balance-aware wallet button display.
51-66: LGTM: Simplified switch-wallet logic.The unified
handleSwitchWalletfunction cleanly handles both in-app browser and desktop flows, improving code clarity.
123-142: LGTM: Balance-aware wallet button.The conditional rendering of asset icons and formatted balance enhances the wallet button's informativeness when the user has assets.
| <IconButton | ||
| icon={<LuLogOut />} | ||
| onClick={() => | ||
| setCurrentContent?.({ | ||
| type: 'disconnect-confirm', | ||
| props: { | ||
| onDisconnect: () => onLogout?.(), | ||
| onBack: () => | ||
| setCurrentContent?.('profile'), | ||
| }, | ||
| }) | ||
| } | ||
| variant="vechainKitSecondary" | ||
| height="30px" | ||
| w="30px" | ||
| borderRadius="5px" | ||
| aria-label="Copy address" | ||
| /> |
There was a problem hiding this comment.
Fix the incorrect aria-label on the Logout button.
The aria-label is set to "Copy address" but this button triggers logout functionality. This is an accessibility issue that will confuse screen reader users.
🔎 Proposed fix
<IconButton
icon={<LuLogOut />}
onClick={() =>
setCurrentContent?.({
type: 'disconnect-confirm',
props: {
onDisconnect: () => onLogout?.(),
onBack: () =>
setCurrentContent?.('profile'),
},
})
}
variant="vechainKitSecondary"
height="30px"
w="30px"
borderRadius="5px"
- aria-label="Copy address"
+ aria-label="Logout"
/>📝 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.
| <IconButton | |
| icon={<LuLogOut />} | |
| onClick={() => | |
| setCurrentContent?.({ | |
| type: 'disconnect-confirm', | |
| props: { | |
| onDisconnect: () => onLogout?.(), | |
| onBack: () => | |
| setCurrentContent?.('profile'), | |
| }, | |
| }) | |
| } | |
| variant="vechainKitSecondary" | |
| height="30px" | |
| w="30px" | |
| borderRadius="5px" | |
| aria-label="Copy address" | |
| /> | |
| <IconButton | |
| icon={<LuLogOut />} | |
| onClick={() => | |
| setCurrentContent?.({ | |
| type: 'disconnect-confirm', | |
| props: { | |
| onDisconnect: () => onLogout?.(), | |
| onBack: () => | |
| setCurrentContent?.('profile'), | |
| }, | |
| }) | |
| } | |
| variant="vechainKitSecondary" | |
| height="30px" | |
| w="30px" | |
| borderRadius="5px" | |
| aria-label="Logout" | |
| /> |
🤖 Prompt for AI Agents
In packages/vechain-kit/src/components/common/AddressDisplay.tsx around lines
179 to 196, the IconButton used to open the logout confirmation has an incorrect
aria-label "Copy address"; change it to a descriptive logout label such as
"Logout" or "Sign out" to accurately reflect the action and improve
accessibility for screen readers, leaving all other props and behavior
unchanged.
Using latest version of dappkit so the connect modal is visible.
When clicking "add wallet" we display immediatly dappkit instead of our own ConnectModal, because I have difficulties in closing that modal after the login is completed. But there is no point to connect to a social login account, since those are not supported for multiwallets, so I think it actually even has more sense and saves some click to the user.
Applied some fixes to how we store new connections and set wallets active.
Registrazione.schermo.2025-12-23.alle.10.52.47.mov
I have only one issue I was not able to solve, but it's a minor I would like to address later:
It is not possible to remove the lastly connected wallet.
Scenario:
I have 1 wallet, add a new one, switch back to wallet 1, and try to remove wallet 2 (the lastly added): instead of removing it it's set as active.
I add another wallet 3, now I can remove wallet 2, but cannot remove wallet 3.
The weird stuff is that it always is set as active.
Another issue I noticed:
When doing login after disconnecting, the behaviour is not consistent in setting the wallet the user has in VeWorld as new.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.