From cc20142cc3180526af7216c0a8f3fe16948ba540 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Jan 2026 16:53:39 +0000 Subject: [PATCH] fix(wallet): address code review issues and simplify user menu - Fix memory leak: track retry timeout and clear on disconnect - Add explicit WalletSupport type for support observable - Add comments explaining balance refresh error handling behavior - Add comment about restoreWallet not being awaited intentionally - User menu now uses connectionStatus observable (shows connecting/error states) - Remove wallet name display from user menu (simplifies UI) - Remove unused walletServiceProfile hook and getWalletName function https://claude.ai/code/session_01CnJgjFMvZHZWs2ujAiWAiQ --- src/components/nostr/user-menu.tsx | 74 +++++++++--------------------- src/hooks/useWallet.ts | 10 +++- src/services/nwc.ts | 24 ++++++++-- 3 files changed, 50 insertions(+), 58 deletions(-) diff --git a/src/components/nostr/user-menu.tsx b/src/components/nostr/user-menu.tsx index c40ee55..dbd724d 100644 --- a/src/components/nostr/user-menu.tsx +++ b/src/components/nostr/user-menu.tsx @@ -118,18 +118,12 @@ export default function UserMenu() { return sats.toString(); } - // Get wallet service profile for display name, using wallet relays as hints - const walletServiceProfile = useProfile( - nwcConnection?.service, - nwcConnection?.relays, - ); - - // Use wallet hook for real-time balance and methods + // Use wallet hook for real-time balance and connection status const { disconnect: disconnectWallet, refreshBalance, balance, - wallet, + connectionStatus, } = useWallet(); function openProfile() { @@ -185,26 +179,6 @@ export default function UserMenu() { } } - function getWalletName(): string { - if (!nwcConnection) return ""; - // Use service pubkey profile name, fallback to alias, then pubkey slice - return ( - getDisplayName(nwcConnection.service, walletServiceProfile) || - nwcConnection.info?.alias || - nwcConnection.service.slice(0, 8) - ); - } - - function openWalletServiceProfile() { - if (!nwcConnection?.service) return; - addWindow( - "profile", - { pubkey: nwcConnection.service }, - `Profile ${nwcConnection.service.slice(0, 8)}...`, - ); - setShowWalletInfo(false); - } - return ( <> @@ -262,28 +236,23 @@ export default function UserMenu() { )} - {/* Wallet Name */} -
- Wallet: - -
- {/* Connection Status */}
Status:
- - {wallet ? "Connected" : "Disconnected"} + + {connectionStatus}
@@ -409,16 +378,17 @@ export default function UserMenu() { )} -
- - - {getWalletName()} - -
+ ) : ( wallet?.support$, [wallet]); + // Wallet support from library's support$ observable (cached by library for 60s) + const support: WalletSupport | null | undefined = use$( + () => wallet?.support$, + [wallet], + ); // Wallet methods - combines reactive support$ with cached info fallback // The support$ waits for kind 13194 events which some wallets don't publish @@ -63,6 +67,8 @@ export function useWallet() { }, [support?.methods, state.nwcConnection?.info?.methods]); // Restore wallet on mount if connection exists + // Note: Not awaited intentionally - wallet is available synchronously from wallet$ + // before validation completes. Any async errors are handled within restoreWallet. useEffect(() => { if (nwcConnection && !wallet && !restoreAttemptedRef.current) { restoreAttemptedRef.current = true; diff --git a/src/services/nwc.ts b/src/services/nwc.ts index 0d6f195..386a552 100644 --- a/src/services/nwc.ts +++ b/src/services/nwc.ts @@ -25,6 +25,7 @@ WalletConnect.pool = pool; // Internal state let notificationSubscription: Subscription | null = null; +let notificationRetryTimeout: ReturnType | null = null; /** * Connection status for the NWC wallet @@ -75,9 +76,13 @@ function hexToBytes(hex: string): Uint8Array { * Notifications trigger balance refresh for real-time updates. */ function subscribeToNotifications(wallet: WalletConnect) { - // Clean up existing subscription + // Clean up existing subscription and pending retry notificationSubscription?.unsubscribe(); notificationSubscription = null; + if (notificationRetryTimeout) { + clearTimeout(notificationRetryTimeout); + notificationRetryTimeout = null; + } let retryCount = 0; const maxRetries = 5; @@ -109,7 +114,7 @@ function subscribeToNotifications(wallet: WalletConnect) { const delay = baseDelay * Math.pow(2, retryCount); retryCount++; connectionStatus$.next("connecting"); - setTimeout(subscribe, delay); + notificationRetryTimeout = setTimeout(subscribe, delay); } else { connectionStatus$.next("error"); lastError$.next( @@ -124,7 +129,7 @@ function subscribeToNotifications(wallet: WalletConnect) { if (wallet$.value && retryCount < maxRetries) { const delay = baseDelay * Math.pow(2, retryCount); retryCount++; - setTimeout(subscribe, delay); + notificationRetryTimeout = setTimeout(subscribe, delay); } }, }); @@ -209,8 +214,13 @@ export async function restoreWallet( * Disconnects and clears the wallet. */ export function clearWallet(): void { + // Clean up subscription and pending retry notificationSubscription?.unsubscribe(); notificationSubscription = null; + if (notificationRetryTimeout) { + clearTimeout(notificationRetryTimeout); + notificationRetryTimeout = null; + } wallet$.next(null); balance$.next(undefined); @@ -221,7 +231,12 @@ export function clearWallet(): void { /** * Refreshes the balance from the wallet. - * Includes retry logic for reliability. + * Includes retry logic with exponential backoff for reliability. + * + * Note: If we're already connected and a balance fetch fails after retries, + * we don't set error state. This prevents UI flapping - the notification + * subscription is the primary health indicator. A transient balance fetch + * failure shouldn't mark an otherwise working connection as errored. */ export async function refreshBalance(): Promise { const wallet = wallet$.value; @@ -253,6 +268,7 @@ export async function refreshBalance(): Promise { setTimeout(r, baseDelay * Math.pow(2, attempt)), ); } else if (connectionStatus$.value !== "connected") { + // Only set error state if not already connected (e.g., during initial validation) connectionStatus$.next("error"); lastError$.next( error instanceof Error ? error : new Error("Failed to get balance"),