feat: Improvement of wallet connection functionality#17
feat: Improvement of wallet connection functionality#17keyneszeng wants to merge 1 commit intogenlayerlabs:mainfrom
Conversation
- 娣诲姞缃戠粶涓€閿垏鎹㈠姛鑳?- 瀹炵幇璐︽埛鍒囨崲鍚庢暟鎹嚜鍔ㄥ埛鏂?- 娣诲姞杩炴帴瓒呮椂澶勭悊鏈哄埗 - 鏀硅繘缃戠粶鐘舵€佸彉鍖栨彁绀?- 浼樺寲閿欒澶勭悊鍜岀敤鎴峰弽棣?- 娣诲姞缃戠粶璀﹀憡妯箙鏄剧ず
📝 WalkthroughWalkthroughThis PR introduces comprehensive wallet connectivity enhancements, including network switching functionality integrated into UI components, automatic cache invalidation upon wallet account changes via custom events, timeout protection for MetaMask connections, and network status warnings in the navigation interface. Changes
Sequence DiagramssequenceDiagram
actor User
participant UI as AccountPanel/Navbar
participant WalletProvider
participant MetaMask
participant GenLayer as GenLayer Network
User->>UI: Clicks "Switch to GenLayer Network"
UI->>WalletProvider: switchToCorrectNetwork()
WalletProvider->>MetaMask: eth_chainId
alt Not on GenLayer
MetaMask-->>WalletProvider: Current Chain ID
WalletProvider->>MetaMask: wallet_switchEthereumChain
MetaMask->>GenLayer: Switch Network
GenLayer-->>MetaMask: Success
MetaMask-->>WalletProvider: Switched
WalletProvider->>WalletProvider: Update isOnCorrectNetwork
WalletProvider-->>UI: Success Toast
else Already on GenLayer
MetaMask-->>WalletProvider: GenLayer Chain ID
WalletProvider-->>UI: Already Connected Toast
end
UI-->>User: Update UI / Clear Banner
sequenceDiagram
participant MetaMask
participant WalletProvider
participant Window as Window Events
participant Hooks as useFootballBets Hooks
participant QueryClient
MetaMask->>WalletProvider: User switches account
WalletProvider->>WalletProvider: Update wallet state
WalletProvider->>Window: Dispatch walletAccountChanged
Window->>Hooks: Event listener triggered
Hooks->>QueryClient: invalidateQueries(['bets'])
Hooks->>QueryClient: invalidateQueries(['playerPoints', address])
Hooks->>QueryClient: invalidateQueries(['leaderboard'])
QueryClient->>Hooks: Refetch data
Hooks-->>Hooks: Update UI with fresh data
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/lib/hooks/useFootballBets.ts (1)
57-71: Refactor: Reduce coupling between data domains.The
useBetshook invalidates queries forplayerPointsandleaderboard, which creates tight coupling between different data domains. Each hook should ideally manage only its own cache invalidation to maintain better separation of concerns and reduce unexpected side effects.♻️ Proposed refactor
Remove the extra invalidations from
useBetsand let each hook manage its own data:useEffect(() => { const handleAccountChange = () => { queryClient.invalidateQueries({ queryKey: ["bets"] }); - queryClient.invalidateQueries({ queryKey: ["playerPoints"] }); - queryClient.invalidateQueries({ queryKey: ["leaderboard"] }); }; if (typeof window !== "undefined") { window.addEventListener("walletAccountChanged", handleAccountChange); return () => { window.removeEventListener("walletAccountChanged", handleAccountChange); }; } }, [queryClient]);Since
usePlayerPointsanduseLeaderboardalready have their own event listeners (lines 97-108 and 134-145), they will handle their own cache invalidation.frontend/components/Navbar.tsx (2)
52-64: Consider adding a network switch action to the banner.The warning banner informs users they're on the wrong network but doesn't provide a way to switch directly from the banner. While the
AccountPanelincludes a switch button, adding an action button to the banner would improve discoverability and reduce friction.💡 Optional enhancement
Add a switch button to the banner:
+ import { Button } from "./ui/button"; const { isConnected, isOnCorrectNetwork } = useWallet(); + const { switchToCorrectNetwork } = useWallet(); // ... in JSX: <Alert variant="default" className="bg-transparent border-0 p-0"> <AlertCircle className="h-4 w-4 text-yellow-500" /> <AlertDescription className="text-sm text-yellow-400"> You're not on the GenLayer network. Please switch networks to continue. </AlertDescription> + <Button + variant="outline" + size="sm" + onClick={switchToCorrectNetwork} + className="ml-auto" + > + Switch Network + </Button> </Alert>
65-71: Consider extracting banner height to a constant.The
marginTopvalue of'48px'is hardcoded and may not match the actual banner height if styling changes. Using a shared constant would ensure consistency.♻️ Proposed refactor
Extract the banner height to a constant:
+ const NETWORK_BANNER_HEIGHT = 48; // px return ( <> {isConnected && !isOnCorrectNetwork && ( - <div className="fixed top-0 left-0 right-0 z-[60] bg-yellow-500/10 border-b border-yellow-500/20 backdrop-blur-sm"> + <div + className="fixed top-0 left-0 right-0 z-[60] bg-yellow-500/10 border-b border-yellow-500/20 backdrop-blur-sm" + style={{ height: `${NETWORK_BANNER_HEIGHT}px` }} + > {/* ... */} </div> )} <header className="fixed top-0 left-0 right-0 z-50 transition-all duration-500 ease-out" style={{ paddingTop: `${paddingTop}px`, - marginTop: isConnected && !isOnCorrectNetwork ? '48px' : '0' + marginTop: isConnected && !isOnCorrectNetwork ? `${NETWORK_BANNER_HEIGHT}px` : '0' }} >WALLET_IMPROVEMENTS.md (1)
1-153: Consider providing English documentation or bilingual version.The documentation is comprehensive and well-structured, but it's entirely in Chinese. For a project that appears to use English in code comments and variable names, providing an English version or bilingual documentation would improve accessibility for international contributors.
The technical content is accurate and matches the implementation. The code examples correctly demonstrate:
- Custom event system (lines 86-104)
- Timeout mechanism (lines 108-116)
- Usage patterns (lines 120-136)
Consider creating
WALLET_IMPROVEMENTS.en.mdor combining both languages in one file with clear sections.frontend/lib/genlayer/WalletProvider.tsx (1)
349-356: Consider extracting state update logic to reduce duplication.The state update pattern (get accounts, get chainId, check network, update state) is duplicated across
connectWallet,switchWalletAccount, andswitchToCorrectNetwork. Extracting this to a helper function would improve maintainability.♻️ Proposed refactor
Extract the common state update logic:
const updateWalletState = async () => { const accounts = await getAccounts(); const chainId = await getCurrentChainId(); const correctNetwork = await isOnGenLayerNetwork(); setState({ address: accounts[0] || null, chainId, isConnected: accounts.length > 0, isLoading: false, isMetaMaskInstalled: true, isOnCorrectNetwork: correctNetwork, }); return { accounts, chainId, correctNetwork }; };Then use it in all three functions:
const { accounts } = await updateWalletState(); return accounts[0];frontend/components/AccountPanel.tsx (1)
263-289: Strong implementation of network switch UI.The loading states, disabled conditions, and error handling are well-implemented. The user flow is clear with proper visual feedback.
One minor refinement: The ExternalLink icon (line 284) typically signals opening an external URL, which might not be the clearest metaphor for an in-wallet network switch operation.
🎨 Optional icon alternatives
Consider semantically clearer icons from lucide-react:
-import { User, LogOut, AlertCircle, ExternalLink, Loader2 } from "lucide-react"; +import { User, LogOut, AlertCircle, RefreshCw, Loader2 } from "lucide-react";) : ( <> - <ExternalLink className="w-4 h-4 mr-2" /> + <RefreshCw className="w-4 h-4 mr-2" /> Switch to GenLayer Network </> )}Alternatives:
Network,Zap, orArrowRightLeftcould also work depending on the desired visual metaphor.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
WALLET_IMPROVEMENTS.mdfrontend/components/AccountPanel.tsxfrontend/components/Navbar.tsxfrontend/lib/genlayer/WalletProvider.tsxfrontend/lib/genlayer/client.tsfrontend/lib/hooks/useFootballBets.ts
🧰 Additional context used
📓 Path-based instructions (4)
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend should be implemented with Next.js 15, React 19, TypeScript, Tailwind CSS, TanStack Query, and Wagmi/Viem for wallet integration
Files:
frontend/lib/hooks/useFootballBets.tsfrontend/components/Navbar.tsxfrontend/lib/genlayer/WalletProvider.tsxfrontend/components/AccountPanel.tsxfrontend/lib/genlayer/client.ts
frontend/lib/hooks/use*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Store React hooks for contract interactions in
frontend/lib/hooks/directory with naming patternuse<ContractName>.ts
Files:
frontend/lib/hooks/useFootballBets.ts
frontend/lib/genlayer/WalletProvider.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain wallet context provider in
frontend/lib/genlayer/WalletProvider.tsx
Files:
frontend/lib/genlayer/WalletProvider.tsx
frontend/lib/genlayer/client.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain GenLayer client initialization in
frontend/lib/genlayer/client.ts
Files:
frontend/lib/genlayer/client.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Applies to frontend/lib/genlayer/WalletProvider.tsx : Maintain wallet context provider in `frontend/lib/genlayer/WalletProvider.tsx`
📚 Learning: 2025-12-10T12:30:13.640Z
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Applies to frontend/lib/genlayer/WalletProvider.tsx : Maintain wallet context provider in `frontend/lib/genlayer/WalletProvider.tsx`
Applied to files:
WALLET_IMPROVEMENTS.mdfrontend/lib/hooks/useFootballBets.tsfrontend/components/Navbar.tsxfrontend/lib/genlayer/WalletProvider.tsxfrontend/components/AccountPanel.tsxfrontend/lib/genlayer/client.ts
📚 Learning: 2025-12-10T12:30:13.640Z
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Applies to frontend/lib/hooks/use*.{ts,tsx} : Store React hooks for contract interactions in `frontend/lib/hooks/` directory with naming pattern `use<ContractName>.ts`
Applied to files:
frontend/lib/hooks/useFootballBets.ts
📚 Learning: 2025-12-10T12:30:13.640Z
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Applies to frontend/**/*.{ts,tsx} : Frontend should be implemented with Next.js 15, React 19, TypeScript, Tailwind CSS, TanStack Query, and Wagmi/Viem for wallet integration
Applied to files:
frontend/lib/hooks/useFootballBets.tsfrontend/components/Navbar.tsxfrontend/components/AccountPanel.tsx
📚 Learning: 2025-12-10T12:30:13.640Z
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Applies to frontend/lib/genlayer/client.ts : Maintain GenLayer client initialization in `frontend/lib/genlayer/client.ts`
Applied to files:
frontend/lib/genlayer/WalletProvider.tsxfrontend/lib/genlayer/client.ts
📚 Learning: 2025-12-10T12:30:13.640Z
Learnt from: CR
Repo: genlayerlabs/genlayer-project-boilerplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T12:30:13.640Z
Learning: Follow the development workflow: ensure GenLayer Studio is running, select network with `genlayer network`, deploy contract with `npm run deploy`, update frontend environment variable, and run frontend with `cd frontend && bun dev`
Applied to files:
frontend/lib/genlayer/WalletProvider.tsx
🧬 Code graph analysis (1)
frontend/lib/genlayer/WalletProvider.tsx (2)
frontend/lib/utils/toast.ts (5)
success(20-32)warning(50-62)info(65-71)userRejected(121-131)error(35-47)frontend/lib/genlayer/client.ts (4)
switchToGenLayerNetwork(167-189)isOnGenLayerNetwork(194-204)getCurrentChainId(123-139)getAccounts(102-118)
🔇 Additional comments (12)
frontend/lib/hooks/useFootballBets.ts (2)
96-108: LGTM! Clean event listener implementation.The event listener properly invalidates only the player points cache, includes the address dependency, and handles cleanup correctly.
133-145: LGTM! Event listener implementation is correct.The leaderboard hook properly manages its own cache invalidation with appropriate cleanup.
frontend/lib/genlayer/client.ts (2)
211-242: Verify timeout behavior doesn't leave MetaMask in inconsistent state.The timeout implementation uses
Promise.race()which returns when the first promise settles, but it doesn't cancel the losing promise. If the timeout fires first, the connection attempt (including MetaMask dialogs) continues running in the background, which could confuse users.This is likely acceptable behavior since:
- MetaMask operations cannot be programmatically cancelled once initiated
- Users can close MetaMask dialogs manually
- The timeout prevents indefinite waiting on the application side
However, verify that subsequent operations handle the case where:
- Timeout occurs and user sees error
- User then approves the pending MetaMask dialog
- Application state may not reflect the now-connected wallet
Consider testing this edge case to ensure the application state syncs properly if a delayed MetaMask approval occurs after timeout.
232-240: LGTM! Network validation and switching logic is correct.The function properly validates the network after obtaining accounts and switches to GenLayer if needed. This ensures users are always on the correct network after connection.
frontend/lib/genlayer/WalletProvider.tsx (4)
155-164: LGTM! Network change notifications enhance user experience.The toast notifications provide clear, immediate feedback when the network changes, helping users understand their current state.
297-307: LGTM! Account switch notification and event dispatch work well together.The success toast provides user feedback, and the custom event enables automatic data refresh across all components that listen for
walletAccountChanged.
342-342: Review the hardcoded 1-second delay necessity.The implementation includes a 1-second delay after calling
switchToGenLayerNetwork(). While this may help ensure the network switch completes before verification, it's a magic number that could be problematic if network switching is faster or slower than expected.Consider:
- Is this delay necessary, or does
switchToGenLayerNetwork()already wait for completion?- Could polling
isOnGenLayerNetwork()with a retry mechanism be more robust than a fixed delay?- If the delay is needed, extract it to a named constant (e.g.,
NETWORK_SWITCH_DELAY_MS)Verify that the delay is required by testing network switching behavior, as relying on fixed delays can lead to race conditions or unnecessary waiting.
367-380: LGTM! Comprehensive error handling with user-friendly messages.The error handling distinguishes between user cancellation and system errors, providing appropriate feedback for each case.
frontend/components/AccountPanel.tsx (4)
4-4: LGTM: Loading spinner icon added.The Loader2 import is properly used for the network switching loading state.
32-32: LGTM: Network switching capability integrated.The switchToCorrectNetwork method is properly destructured and aligns with the wallet context provider pattern.
41-41: LGTM: Loading state properly declared.The isSwitchingNetwork state follows the existing pattern and is correctly used to manage UI feedback.
97-112: No action required—error notifications are already handled internally.The error handling is not inconsistent.
switchToCorrectNetwork()already shows toast notifications for all error cases (including error toasts for non-rejected failures and userRejected toasts for cancelled requests) before throwing the error. ThehandleSwitchNetworkcatch block receives the thrown error and displays it in the inline Alert viasetConnectionError. Adding a toast notification here would result in duplicate notifications to the user.Likely an incorrect or invalid review comment.
|
best |
Overview
This submission has comprehensively optimized the wallet connection function, enhancing the user experience, system stability, and operational convenience. The main improvements include enhanced network switching, automatic data refreshing during account switching, and handling of connection timeouts.
Main Changes
Enhanced network switching function
A new method named "switchToCorrectNetwork()" has been added, enabling one-click switching to the GenLayer network.
A "Switch to GenLayer Network" button has been added to the AccountPanel component.
Warnings and operation instructions will be displayed when the user is on an incorrect network.
The data will be automatically refreshed after the account switch.
When the account is switched, all related data will be automatically refreshed.
Use the walletAccountChanged event to notify the components for updates.
Affected hooks: useBets, usePlayerPoints, useLeaderboard
Handling connection timeout
Add a 30-second timeout mechanism to the
connectMetaMask()functionImplement timeout control using
Promise.race()Provide clear error messages for timeout situations
Changed files
WALLET_IMPROVEMENTS.md - New documentation for wallet improvements
frontend/components/AccountPanel.tsx - Add UI for network switching
frontend/components/Navbar.tsx - Update navigation bar (possibly related adjustments)
frontend/lib/genlayer/WalletProvider.tsx - Improve core wallet logic
frontend/lib/genlayer/client.ts - Handle connection timeouts
Test suggestions
Test MetaMask connection and network switching functionality
Verify that data automatically refreshes after account switching
Check error handling in timeout scenarios
These improvements aim to provide a smoother wallet interaction experience and reduce the number of user operations.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.