[FEAT] Wallet connect ux improvements #183
Conversation
|
@jimenezz22 is attempting to deploy a commit to the ZeroXBridge Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds wallet connection UX components, a unified connection hook, detection and error utilities, and integrates them into Ethereum and Starknet modals with connection-state propagation. Updates global styles (scrollbars, animations). Includes a developer guide (CLAUDE.md). Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant M as Connect Modal (Eth/Starknet)
participant D as walletDetection.ts
participant H as useWalletConnection
participant P as Chain Provider<br/>(wagmi / starknet-react)
participant E as walletErrors.ts
U->>M: Open Connect Wallet
M->>D: getAvailable/Installed/Missing wallets
D-->>M: Wallet lists
U->>M: Click a wallet to connect
M->>M: Disable back/close, mark connecting
M->>H: connectWallet(connector)
par Race: connect vs timeout
H->>P: initiate connection
H->>H: start 30s timeout
end
alt Success
P-->>H: connected (address, wallet)
H-->>M: step=success -> connected
M->>U: Show success animation + address
else Failure/Reject/Unsupported
P-->>H: error
H->>E: parseWalletError(error)
E-->>H: WalletError
H-->>M: step=failed, error
M->>U: Show error banner + Retry/Install
end
M->>M: Re-enable actions when not connecting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/connectWallet.tsx (1)
3-3: Add modal a11y (role, labeling), ESC handling, and close button labelImproves accessibility and UX, and ensures ESC won’t close while connecting.
-import React, { useState } from 'react'; +import React, { useEffect, useState, useId } from 'react'; @@ const ConnectModal: React.FC<ConnectModalProps> = ({ isModalOpen, setIsModalOpen }) => { const [activeChain, setActiveChain] = useState<'ethereum' | 'starknet' | null>(null); const [isAnyWalletConnecting, setIsAnyWalletConnecting] = useState(false); + const titleId = useId(); @@ - const handleModalClick = (e: React.MouseEvent) => { + const handleModalClick = (e: React.MouseEvent) => { e.stopPropagation(); }; + + // Close on ESC when not connecting + useEffect(() => { + if (!isModalOpen) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape' && !isAnyWalletConnecting) { + setIsModalOpen(false); + } + }; + document.addEventListener('keydown', onKeyDown); + return () => document.removeEventListener('keydown', onKeyDown); + }, [isModalOpen, isAnyWalletConnecting, setIsModalOpen]); @@ - <button + <button className={` absolute top-4 right-4 transition-colors z-20 ${isAnyWalletConnecting ? 'text-gray-600 cursor-not-allowed' : 'text-white hover:text-gray-300' } `} onClick={() => !isAnyWalletConnecting && setIsModalOpen(false)} disabled={isAnyWalletConnecting} + aria-label="Close" > <X size={24} /> </button> @@ - <h2 className="text-white text-xl font-semibold text-center"> + <h2 id={titleId} className="text-white text-xl font-semibold text-center"> @@ - <div + <div className={`fixed inset-0 bg-black/40 z-50 flex items-center justify-center p-4 ${ isModalOpen ? "visible" : "hidden" }`} onClick={handleOverlayClick} + role="dialog" + aria-modal="true" + aria-labelledby={titleId} >Also applies to: 15-21, 59-71, 73-82, 41-46
♻️ Duplicate comments (2)
app/components/ethereumWallet.tsx (2)
40-41: Consider avoiding immediate state initialization with a function call.Similar to the StarkNet component, the
walletDetectionstate is initialized immediately withcheckWalletInstallation(). Consider lazy initialization for better SSR compatibility.
55-59: Avoid using window.location.reload() for state updates.This has the same issue as in the StarkNet component - using
window.location.reload()causes poor UX by reloading the entire application.
🧹 Nitpick comments (18)
CLAUDE.md (1)
29-31: Add TypeScript syntax highlighting to the code blockThe fenced code block should specify a language for proper syntax highlighting.
-``` +```typescript WagmiProvider → QueryClientProvider → StarknetProvider → ThemeProvider → LayoutContent</blockquote></details> <details> <summary>app/utils/walletErrors.ts (4)</summary><blockquote> `10-15`: **originalError is accepted but never used; plumb it through for observability** You take an `originalError` param in `createWalletError` but drop it. That hampers debugging/telemetry and confuses consumers. Add an optional `cause` (or `originalError`) field to `WalletError` and populate it from the factory. ```diff export interface WalletError { type: WalletErrorType; message: string; suggestedAction?: string; retryable: boolean; + // Preserve the originating error for logging/telemetry + cause?: unknown; } export const createWalletError = (type: WalletErrorType, originalError?: any): WalletError => { const errorMap: Record<WalletErrorType, Omit<WalletError, 'type'>> = { @@ }; return { type, - ...errorMap[type] + ...errorMap[type], + cause: originalError }; };Also applies to: 17-55
19-49: Tighten retryability semantics; avoid encouraging futile retriesRight now every error type is
retryable: true. Consider setting non-recoverable cases tofalseto align with the “retry only for recoverable” objective.[WalletErrorType.WALLET_NOT_INSTALLED]: { message: "Wallet extension not detected", suggestedAction: "Please install the wallet extension and refresh the page", - retryable: true + retryable: false }, @@ [WalletErrorType.UNSUPPORTED_NETWORK]: { message: "Unsupported network", suggestedAction: "Please switch to a supported network in your wallet", - retryable: true + retryable: false }, @@ [WalletErrorType.UNKNOWN_ERROR]: { message: "An unexpected error occurred", suggestedAction: "Please refresh the page and try again", - retryable: true + retryable: false }
87-96: Unify WalletConnect install domain and keep links consistent across the codebaseThis file uses
walletconnect.org, whileapp/utils/walletDetection.tsuseswalletconnect.com. Prefer the current official domain and keep both modules aligned.const installUrls: Record<string, string> = { @@ - 'walletconnect': 'https://walletconnect.org' + 'walletconnect': 'https://walletconnect.com' };If you want me to verify/download URLs with the latest official sources, I can run a quick web check.
17-49: Minor: avoid re-creating errorMap on every callHoist
errorMapto module scope to cut per-call allocations. Low impact but easy win.-export const createWalletError = (type: WalletErrorType, originalError?: any): WalletError => { - const errorMap: Record<WalletErrorType, Omit<WalletError, 'type'>> = { +const errorMap: Record<WalletErrorType, Omit<WalletError, 'type'>> = { [WalletErrorType.WALLET_NOT_INSTALLED]: { @@ } }; - +export const createWalletError = (type: WalletErrorType, originalError?: any): WalletError => { return { type, ...errorMap[type], cause: originalError }; };app/components/WalletTooltip.tsx (1)
73-93: Consider rendering in a portal to avoid clipping within overflow:hidden parentsTooltips inside scrollable/overflow containers can be clipped even with z-50. A portal (e.g., to document.body) avoids this.
I can provide a minimal portal wrapper if you want to adopt it project-wide.
app/utils/walletDetection.ts (3)
74-86: WalletConnect “always available” may mask “no wallet installed” empty statesTreating WalletConnect as “installed” means
getInstalledWallets('ethereum')is never empty, so fallback guidance (“no wallet detected”) won’t appear for ETH. If the UX intends empty states when no browser extensions are present, either:
- mark WalletConnect as
isInstalled: false, or- add a
requiresInstallationflag and filter accordingly in UI, or- gate empty-state logic on “installed browser extensions,” not all connectors.
I can adjust the helpers to support
requiresInstallationwithout breaking current consumers. Want a patch?Also applies to: 97-116
56-66: Icon and link consistency
- ArgentX/Braavos use a generic
/wallet.svg, while others point to wallet-specific icons. If assets exist, prefer dedicated icons for clarity.- WalletConnect link here is
walletconnect.combutgetWalletInstallUrlreturnswalletconnect.org. Align both to the same canonical domain.If you confirm the preferred assets/URLs, I can send a follow-up diff.
Also applies to: 63-70
1-9: Type safety: declare ambient Window typings for injected providersReplace repeated
(window as any)with an ambient declaration to improve safety and editor hints.// e.g., in a global.d.ts interface EthereumLikeProvider { isMetaMask?: boolean; providers?: EthereumLikeProvider[]; } interface StarknetLikeProvider { isArgentX?: boolean; isBraavos?: boolean; providers?: StarknetLikeProvider[]; } interface Window { ethereum?: EthereumLikeProvider; starknet?: StarknetLikeProvider; starknet_argentX?: unknown; starknet_braavos?: unknown; }Also applies to: 11-13
app/components/connectWallet.tsx (1)
19-21: Auto-close the modal when a wallet connects successfullyWhen
ethereumAddressorstarknetAddressbecomes truthy, you can close the modal and clearactiveChainto simplify the happy path.const { address: starknetAddress } = useStarknetAccount(); const { address: ethereumAddress } = useEthereumAccount(); + React.useEffect(() => { + if ((ethereumAddress || starknetAddress) && isModalOpen && !isAnyWalletConnecting) { + setActiveChain(null); + setIsModalOpen(false); + } + }, [ethereumAddress, starknetAddress, isAnyWalletConnecting, isModalOpen, setIsModalOpen]);Also applies to: 140-151
app/components/WalletError.tsx (1)
3-3: Type import path consistencyYou import from
@/app/utils/walletErrors. Ensure your tsconfigpathsresolves@to the repo root; otherwise prefer a relative import to avoid build-time path issues.app/components/WalletConnectionProgress.tsx (2)
16-28: Consider memoizing the helper functions to avoid recreations on each render.The helper functions
getStepIcon,getStepMessage,getStepColor, andgetProgressWidthare recreated on every render. Since they only depend on thestepandwalletNameprops, consider memoizing them withuseMemofor better performance.+import React, { useMemo } from 'react'; import { ConnectionStep } from '@/app/hooks/useWalletConnection'; import { Loader2, CheckCircle, AlertCircle, Wallet } from 'lucide-react';- const getStepIcon = () => { + const stepIcon = useMemo(() => { switch (step) { case 'connecting': return <Loader2 className="w-5 h-5 text-[#A26DFF] animate-spin" />; case 'success': case 'connected': return <CheckCircle className="w-5 h-5 text-green-400" />; case 'failed': return <AlertCircle className="w-5 h-5 text-red-400" />; default: return <Wallet className="w-5 h-5 text-gray-400" />; } - }; + }, [step]);Apply similar changes to the other helper functions, then update the render to use
stepIconinstead of{getStepIcon()}.
30-43: Consider defensive handling of undefined walletName.While the function handles the fallback gracefully, consider adding a check to ensure consistent messaging when
walletNameis explicitly undefined vs. when it's an empty string.const getStepMessage = () => { + const displayName = walletName?.trim() || 'wallet'; switch (step) { case 'connecting': - return `Connecting to ${walletName || 'wallet'}...`; + return `Connecting to ${displayName}...`; case 'success': - return `Successfully connected to ${walletName || 'wallet'}!`; + return `Successfully connected to ${displayName}!`; case 'connected': - return `Connected to ${walletName || 'wallet'}`; + return `Connected to ${displayName}`; case 'failed': - return `Failed to connect to ${walletName || 'wallet'}`; + return `Failed to connect to ${displayName}`; default: return 'Ready to connect'; } };app/components/WalletRecommendations.tsx (1)
68-81: Consider adding aria-label for better accessibility.The Install button should have an aria-label to provide context for screen readers about which wallet is being installed.
<a href={wallet.downloadUrl} target="_blank" rel="noopener noreferrer" + aria-label={`Install ${wallet.name} wallet`} className=" inline-flex items-center gap-1 px-2 py-1 bg-[#A26DFF] hover:bg-[#A26DFF]/90 text-white text-xs rounded transition-colors duration-200 " >app/components/WalletEmptyState.tsx (1)
114-114: Validate external URLs for security.The component constructs URLs dynamically based on the category. While these are currently hardcoded, it's good practice to validate or use a URL mapping for external links.
Add a URL mapping at the top of the file:
const WALLET_GUIDE_URLS = { ethereum: 'https://ethereum.org/wallets', starknet: 'https://starknet.io/wallets' } as const;Then use it in the component:
- href={category === 'ethereum' ? 'https://ethereum.org/wallets' : 'https://starknet.io/wallets'} + href={WALLET_GUIDE_URLS[category]}app/components/starknetWallet.tsx (2)
48-49: Consider avoiding immediate state initialization with a function call.The
walletDetectionstate is initialized immediately withcheckWalletInstallation(), which may not be necessary if the component renders on the server side. Consider lazy initialization or using useEffect.- const [walletDetection, setWalletDetection] = useState(() => checkWalletInstallation()); + const [walletDetection, setWalletDetection] = useState<ReturnType<typeof checkWalletInstallation> | null>(null); + + useEffect(() => { + setWalletDetection(checkWalletInstallation()); + }, []);
220-238: Extract wallet icon rendering logic to reduce duplication.The wallet icon rendering logic with type checking and fallback is duplicated across components. Consider extracting it to a utility function.
Create a utility function:
// In a utils file or at the top of the component const getWalletIconSrc = (icon: string | { dark: string; light?: string }): string => { return typeof icon === "object" ? icon.dark : icon; };Then use it:
- src={ - typeof wallet.icon === "object" - ? wallet.icon.dark - : wallet.icon - } + src={getWalletIconSrc(wallet.icon)}app/components/ethereumWallet.tsx (1)
219-222: Add error handling for missing wallet icons.The image source might fail to load if the icon file is missing. Consider adding an onError handler to provide a fallback.
<img src={walletIcons[connector.id.toLowerCase()] || '/wallet.svg'} alt={connector.name} className="w-8 h-8" + onError={(e) => { + (e.target as HTMLImageElement).src = '/wallet.svg'; + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
CLAUDE.md(1 hunks)app/components/WalletConnectionProgress.tsx(1 hunks)app/components/WalletEmptyState.tsx(1 hunks)app/components/WalletError.tsx(1 hunks)app/components/WalletLoader.tsx(1 hunks)app/components/WalletRecommendations.tsx(1 hunks)app/components/WalletSuccess.tsx(1 hunks)app/components/WalletTooltip.tsx(1 hunks)app/components/connectWallet.tsx(2 hunks)app/components/ethereumWallet.tsx(3 hunks)app/components/starknetWallet.tsx(1 hunks)app/globals.css(3 hunks)app/hooks/useWalletConnection.ts(1 hunks)app/utils/walletDetection.ts(1 hunks)app/utils/walletErrors.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
app/components/WalletError.tsx (1)
app/utils/walletErrors.ts (2)
WalletError(10-15)getWalletInstallUrl(87-96)
app/components/ethereumWallet.tsx (3)
app/hooks/useWalletConnection.ts (1)
useWalletConnection(21-163)app/utils/walletDetection.ts (5)
checkWalletInstallation(12-28)getAvailableWallets(75-77)getInstalledWallets(80-86)getMissingWallets(89-95)hasAnyWalletsInstalled(98-100)app/components/WalletTooltip.tsx (1)
InfoTooltip(98-105)
app/components/WalletEmptyState.tsx (1)
app/utils/walletDetection.ts (1)
WalletInfo(1-9)
app/hooks/useWalletConnection.ts (1)
app/utils/walletErrors.ts (3)
WalletError(10-15)createWalletError(17-55)parseWalletError(57-85)
app/components/WalletRecommendations.tsx (1)
app/utils/walletDetection.ts (1)
WalletInfo(1-9)
app/components/WalletConnectionProgress.tsx (1)
app/hooks/useWalletConnection.ts (1)
ConnectionStep(7-7)
app/components/starknetWallet.tsx (3)
app/hooks/useWalletConnection.ts (1)
useWalletConnection(21-163)app/utils/walletDetection.ts (5)
checkWalletInstallation(12-28)getAvailableWallets(75-77)getInstalledWallets(80-86)getMissingWallets(89-95)hasAnyWalletsInstalled(98-100)app/components/WalletTooltip.tsx (1)
InfoTooltip(98-105)
🪛 LanguageTool
CLAUDE.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...m run dev(uses Next.js with Turbopack) - **Build for production**:npm run build` ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ... Turbopack) - Build for production: npm run build - Start production server: npm start -...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...n build- **Start production server**:npm start- **Lint code**:npm run lint` The project...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...view ### Multi-Chain Wallet Integration The application supports dual blockchain...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...n supports dual blockchain connectivity: - Ethereum: Via Wagmi with connectors fo...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...rs for MetaMask, WalletConnect, and Safe - Starknet: Via @starknet-react/core wit...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... simultaneously. ### Provider Hierarchy The app uses a nested provider structure...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...er → LayoutContent ``` ### Theme System - Uses a custom React Context (`ThemeConte...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...nsive breakpoints ### Routing Structure - Landing pages: Root (/) and about (`...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...pages**: Root (/) and about (/about) - Dashboard: Main dashboard (`/dashboard...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...board (/dashboard) with nested routes: - Analytics (/dashboard/analytics) - S...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...: - Analytics (/dashboard/analytics) - Swap (/dashboard/swap) - Lock Liqui...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...analytics) - Swap (/dashboard/swap) - Lock Liquidity (/dashboard/lock-liquidi...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ... Liquidity (/dashboard/lock-liquidity) - Claim & Burn (/dashboard/claim-burn) -...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ...- Claim & Burn (/dashboard/claim-burn) - Governance: Voting proposals (`/govern...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...-proposals`) ### Component Organization - UI Components: Reusable components in ...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...UI Components**: Reusable components in app/components/ui/ - Feature Components: Domain-specific co...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...or wallet connection, trading, analytics - Layout Components: Sidebar, Navbar, mo...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...r, mobile navigation ### Styling System - Tailwind CSS with extensive custom confi...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ... CSS with extensive custom configuration - Custom breakpoints for various laptop si...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ... (MacBook, Windows laptops, 4K displays) - Custom animations and color schemes - CS...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...s) - Custom animations and color schemes - CSS variables for theming ### Database ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...es for theming ### Database Integration - Uses Vercel Postgres (@vercel/postgres...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...ses Vercel Postgres (@vercel/postgres) - Schema defined in app/db/schema.sql - ...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...@vercel/postgres) - Schema defined in app/db/schema.sql - API routes in app/api/ ## Key Technic...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...echnical Patterns ### Wallet Management The useWalletState hook provides unifi...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...ality. ### Conditional Layout Rendering The sidebar and navigation are condition...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...ard pages. ### TypeScript Configuration - Uses path mapping with @/* for root-le...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
app/components/WalletLoader.tsx (1)
1-44: Clean and well-structured loading component!The component implementation is correct and follows React best practices. The size-based styling approach with the
sizeClassesmapping provides good flexibility.app/globals.css (1)
132-161: Well-implemented custom scrollbar with good mobile optimization!The scrollbar styling effectively uses the purple theme color (#A26DFF) with appropriate opacity states. The mobile-specific hiding (lines 163-173) is a thoughtful UX improvement for touch devices.
CLAUDE.md (1)
1-82: Valuable developer documentation for Claude AI integration!This comprehensive documentation file will significantly help Claude AI understand the codebase structure when assisting with development. The architectural overview and technical patterns are well-documented.
app/components/WalletSuccess.tsx (1)
1-110: Excellent wallet success component with smooth animations!The component is well-implemented with proper cleanup of timers, smooth transition states, and a clean UI. The auto-close functionality with manual override option provides good UX flexibility.
app/hooks/useWalletConnection.ts (1)
1-163: Robust wallet connection hook with excellent error handling!The implementation includes proper debouncing, timeout handling, connection state management, and comprehensive error handling. The cleanup patterns and state transitions are well thought out.
app/components/connectWallet.tsx (1)
141-149: Child modals properly propagate connection stateI’ve verified that both EthereumWalletModal and StarknetWalletModal use a
useEffecthook to callonConnectionStateChange(isConnecting)whenever the internalisConnectingflag changes. The shareduseWalletConnectionhook setsisConnectingtotrueat the start of every connection attempt and tofalseon both success (including after the brief “success” UI step) and all error paths (including connection timeouts). No additional changes are needed.
app/components/ethereumWallet.tsx
Outdated
| {connectors.map((connector: any) => { | ||
| const isCurrentlyConnecting = isConnecting && selectedConnector?.id === connector.id; | ||
|
|
||
| return ( | ||
| <button | ||
| key={connector.id} | ||
| onClick={() => handleWalletConnect(connector)} | ||
| disabled={isConnecting} | ||
| className={` | ||
| w-full flex items-center justify-between p-4 rounded-lg | ||
| transition-colors disabled:opacity-50 disabled:cursor-not-allowed | ||
| ${isCurrentlyConnecting | ||
| ? 'bg-[#A26DFF]/20 border border-[#A26DFF]/30' | ||
| : 'bg-[#291A43] hover:bg-[#342251]' | ||
| } | ||
| `} | ||
| > | ||
| <div className="flex items-center gap-3"> | ||
| {isCurrentlyConnecting ? ( | ||
| <div className="w-8 h-8 flex items-center justify-center"> | ||
| <WalletLoader size="sm" message="" /> | ||
| </div> | ||
| ) : ( | ||
| <img | ||
| src={walletIcons[connector.id.toLowerCase()] || '/wallet.svg'} | ||
| alt={connector.name} | ||
| className="w-8 h-8" | ||
| /> | ||
| )} | ||
| <div className="text-left"> | ||
| <p className="font-medium text-white"> | ||
| {connector.name} | ||
| </p> | ||
| <p className="text-sm text-gray-400"> | ||
| {isCurrentlyConnecting | ||
| ? 'Connecting...' | ||
| : connector.ready ? 'Available' : 'Not installed' | ||
| } | ||
| </p> | ||
| </div> | ||
| </div> | ||
| {isCurrentlyConnecting && ( | ||
| <span className="text-xs text-[#A26DFF]">Please approve</span> | ||
| )} | ||
| </button> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing key prop validation and potential performance issue.
The connectors.map() is using connector.id as the key, but there's no validation that these IDs are unique. Additionally, the connector object comparison in isCurrentlyConnecting might cause unnecessary re-renders.
- const isCurrentlyConnecting = isConnecting && selectedConnector?.id === connector.id;
+ const isCurrentlyConnecting = isConnecting && selectedConnector?.id === connector.id;
+ const connectorKey = `${connector.id}-${connector.name || 'unknown'}`;
return (
<button
- key={connector.id}
+ key={connectorKey}
onClick={() => handleWalletConnect(connector)}Also consider memoizing the connector list if it changes frequently:
const memoizedConnectors = useMemo(() => connectors, [connectors]);🤖 Prompt for AI Agents
In app/components/ethereumWallet.tsx around lines 195-241, the map uses
connector.id as a key without validating uniqueness and the current comparison
may cause unnecessary re-renders; ensure each mapped element has a stable,
unique key (fall back to a composite key like `${connector.id}-${index}` or a
unique identifier from the connector, and avoid using raw objects as keys),
change any connector comparisons to compare primitive ids (e.g.
selectedConnector?.id === connector.id) instead of whole objects, and to reduce
re-renders either memoize the connectors array with useMemo([connectors]) or
extract the connector button into a separate component wrapped with React.memo
so rendering is stable.
Ugo-X
left a comment
Ugo-X
left a comment
There was a problem hiding this comment.
Great work so far.
However you were to fork from app main and work is already there and send a PR back to app main not main
Ugo-X
left a comment
Ugo-X
left a comment
There was a problem hiding this comment.
Also after making the necessary changes, please address the code rabbit reviews
|
@jimenezz22 Please what is the update on this? |
Thanks for the clarification! I didn’t know about this, when I forked the repo, GitHub automatically used main since that’s set as the default branch on your side. That’s why my PR ended up pointing to main. To avoid this kind of confusion in future contributions, it might be helpful to set app main as the default branch in the repository settings. I’ll rebase my changes onto app main and update the PR accordingly 👍 |
|
@jimenezz22 Gm, Please what is the update on this? |
Resolved conflicts: - Removed deleted wallet component files (connectWallet.tsx, ethereumWallet.tsx, starknetWallet.tsx) - Preserved custom wallet animation styles in globals.css - Will regenerate pnpm-lock.yaml after merge 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix button styling in WalletEmptyState with proper flex layout - Add ARIA attributes and dynamic error titles to WalletError - Improve WalletTooltip accessibility with keyboard support and ARIA roles - Use connectAsync for proper promise-based wallet connections - Enhance wallet detection to support multiple providers - Improve error parsing with EIP-1193 error codes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
ready |
Resolved conflicts in globals.css - kept wallet animation styles 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
2 participants
🎯 Overview
This PR implements comprehensive UX improvements for the wallet connection
flow, addressing all edge cases and providing clear user feedback throughout
the connection process.
closes: #171
✨ What's Changed
Phase 1: Enhanced Error Handling
scenarios
Phase 2: Loading States & Duplicate Prevention
Phase 3: Fallback UI & Wallet Detection
Braavos)
found
Phase 4: Enhanced UX Components
Bug Fixes
🧪 Testing
Tested Wallets
Tested Scenarios
📸 Visual Changes
🎯 Acceptance Criteria Met
✅ Error messages are user-friendly and context-specific
✅ Visible loading spinner appears while connecting
✅ Duplicate actions prevented during loading state
✅ Fallback UI displayed when no wallet installed or connection fails
✅ UX tested across ArgentX, Braavos, and MetaMask
🚀 Impact
Users now experience:
Summary by CodeRabbit