feat: implement wallet connection state UX hardening#371
Merged
OlufunbiIK merged 1 commit intoOlufunbiIK:mainfrom Mar 29, 2026
Merged
Conversation
- Define consistent wallet states (connecting, connected, rejected, locked, not_installed, error, disconnected) - Create WalletStateManager utility for centralized state management - Add enhanced wallet hooks with proper error handling and retry affordances - Implement normalized UI states across all wallet components - Add clear retry affordances for failed connections with actionable guidance - Create comprehensive wallet state transition tests - Add detailed wallet UX documentation and migration guide - Implement state-specific components with consistent visual indicators - Provide backward compatibility with existing wallet hooks Fixes OlufunbiIK#248
|
@patrickNwafo is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements wallet connection state UX hardening by introducing standardized connection states (disconnected, connecting, connected, rejected, locked, not_installed, error), a centralized WalletStateManager utility, enhanced hooks exposing connection state and actions, a new state-aware EnhancedWalletConnect component, and comprehensive tests and documentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant EnhancedWalletConnect as EnhancedWalletConnect<br/>(Component)
participant useEnhancedWallet as useEnhancedWallet<br/>(Hook)
participant WalletContext as WalletContext<br/>(Provider)
participant WalletStateManager as WalletStateManager<br/>(Utility)
User->>EnhancedWalletConnect: Render / Interact
EnhancedWalletConnect->>useEnhancedWallet: Call hooks for state
useEnhancedWallet->>WalletContext: Consume context (connectionState, error)
WalletStateManager->>WalletStateManager: Derive state from<br/>isConnecting, isConnected, error
WalletStateManager-->>WalletContext: Update connectionStatus
WalletContext-->>useEnhancedWallet: Return enhanced wallet + state
useEnhancedWallet-->>EnhancedWalletConnect: Return connectionState,<br/>message, icon, canRetry
EnhancedWalletConnect->>EnhancedWalletConnect: Switch on connectionState<br/>Render subcomponent
alt User Action (Connect/Retry)
User->>EnhancedWalletConnect: Click button
EnhancedWalletConnect->>useEnhancedWallet: Call retryConnection / connect
useEnhancedWallet->>WalletContext: Execute connect action
WalletContext->>WalletStateManager: Recompute connectionStatus
WalletStateManager-->>WalletContext: Update state
WalletContext-->>useEnhancedWallet: Return updated state
useEnhancedWallet-->>EnhancedWalletConnect: Return new connectionState
EnhancedWalletConnect-->>User: Re-render with updated UI
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🧹 Nitpick comments (4)
frontend/src/hooks/useEnhancedWallet.ts (1)
98-98: CallingcanRetry()during render may cause inconsistent behavior.
getRetryAction: wallet.canRetry() ? wallet.retryConnection : undefinedevaluatescanRetry()at render time, butcanRetry(the function reference) is also returned. If state changes between accesses, they could disagree.♻️ Proposed fix for consistency
return { getConnectionState: () => wallet.connectionState, getConnectionMessage: wallet.getConnectionMessage, canRetry: wallet.canRetry, - getRetryAction: wallet.canRetry() ? wallet.retryConnection : undefined, + getRetryAction: () => wallet.canRetry() ? wallet.retryConnection : undefined, getStateIcon: wallet.getStateIcon,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useEnhancedWallet.ts` at line 98, The current assignment evaluates wallet.canRetry() at render time causing potential mismatch between the boolean and the returned function; instead assign getRetryAction a stable function/closure that checks canRetry at invocation time and then calls retryConnection if allowed (i.e., return a function that when called checks wallet.canRetry() and then invokes wallet.retryConnection(), or returns undefined), updating useEnhancedWallet's return shape where needed so callers get a consistent callable retry action (refer to wallet.canRetry and wallet.retryConnection in the hook).frontend/src/utils/walletState.ts (2)
140-141: Redundant ternary always evaluates toundefined.
retryAction: canRetry ? undefined : undefinedis effectively justretryAction: undefined. The comment explains intent, but the code is misleading.♻️ Proposed clarification
return { state, error: error || actionMessage, canRetry, - retryAction: canRetry ? undefined : undefined, // Will be set by context + // retryAction is set by the context provider };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/walletState.ts` around lines 140 - 141, The property assignment retryAction: canRetry ? undefined : undefined is redundant and confusing; change it to a single explicit assignment retryAction: undefined and keep (or move) the explanatory comment that it will be set by context so intent remains clear; locate this in walletState.ts where the object containing retryAction and canRetry is created to update the property.
33-37: Overly broad heuristic:message.includes("freighter")may cause false positives.Any error message mentioning "freighter" (e.g., "Freighter connection failed") would be classified as
not_installed, which may not be accurate.♻️ Suggested refinement
if ( message.includes("not installed") || - message.includes("freighter") + message.includes("install freighter") || + message.includes("freighter is not installed") ) { return "not_installed"; }frontend/src/components/wallet/EnhancedWalletConnect.tsx (1)
21-21: Duplicate hook call causes unnecessary re-computation.
useEnhancedWallet()is called twice—once at line 21 and again at line 39. Consolidate into a single call and destructure all needed values together.♻️ Proposed fix
- const { publicKey, balance, network } = useEnhancedWallet(); + const { publicKey, balance, network, connectionState } = useEnhancedWallet(); const message = getConnectionMessage(); const solutions = getErrorSolutions(); const handleRetry = async () => { await retryConnection(); }; const handleConnect = async () => { await connect(); }; const handleDisconnect = async () => { await disconnect(); }; - // Get connection state from enhanced wallet - const { connectionState } = useEnhancedWallet(); - // Render different UI based on connection stateAlso applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/wallet/EnhancedWalletConnect.tsx` at line 21, The component EnhancedWalletConnect is calling the hook useEnhancedWallet() twice (once at the top and again later); replace both calls with a single call and destructure all required values (publicKey, balance, network and any other props used later) from that single const expression so all usages reference the same variables and avoid re-running the hook; remove the duplicate call site and update any references to use the variables returned from the consolidated const.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/contexts/WalletContext.tsx`:
- Around line 482-492: The context value references undefined functions
(retryConnection, clearError, getConnectionStatus) and relies on a missing
setLastError setter; add definitions for these symbols and wire them into the
value object or remove them if unnecessary. Implement retryConnection as a
function that attempts to reconnect (e.g., reuses the existing connect function
and updates state), implement clearError to clear the current error by calling
setLastError(null) or dispatching the appropriate action, and implement
getConnectionStatus to return the current status from state (e.g., state.status
or derive it from other state fields); ensure setLastError exists (as a state
setter or dispatch helper) and is used consistently where referenced in the file
(e.g., in signTransactionHandler and other handlers), then include the newly
defined retryConnection, clearError, and getConnectionStatus in the value object
so the build succeeds.
- Around line 118-128: The try block inside the async function
checkFreighterInstalled is left unclosed causing a syntax error; close the try
block and add a corresponding catch (or finally) that handles errors and returns
a boolean (e.g., return false on failure) so the function always resolves to a
boolean; specifically update the checkFreighterInstalled function surrounding
the try { ... await isConnected(); return true; } to include the missing closing
brace and a catch that returns false (or logs the error) to ensure the function
and its callers (the useEffect that initializes the wallet) compile and behave
correctly.
- Around line 59-83: Define the missing retryConnection and clearError functions
before the useEffect and export them in the context value; implement
retryConnection as a stable useCallback that clears any transient error and
triggers a reconnect attempt (e.g., setState to set isConnecting=true and call
the existing connect logic or WalletStateManager connect function) and implement
clearError as a useCallback that clears state.error/lastError via setState, then
reference these callbacks in the useEffect dependency array and in the context
value so WalletStateManager.getConnectionStatus can assign retryAction
correctly; ensure both functions are memoized (useCallback) so they do not cause
unnecessary effect reruns.
In `@frontend/src/hooks/useEnhancedWallet.ts`:
- Around line 67-73: The clearError implementation in useEnhancedWallet.ts is a
no-op (clearError currently only contains comments and an empty conditional on
originalWallet.error), so implement actual error clearing by wiring through the
context's clearError: update useWallet.ts to return EnhancedWalletContextType
(including clearError) instead of WalletContextType, then in
useEnhancedWallet.ts call the context-provided clearError (or, if you prefer a
local fallback, invoke originalWallet.clearError when present or call
originalWallet.connect/disconnect sequence only as a last resort) to ensure
errors are actually cleared; reference functions/types: clearError,
originalWallet, useEnhancedWallet.ts, useWallet.ts, EnhancedWalletContextType,
WalletContextType.
- Around line 1-4: The hook type is wrong: useWallet (imported as
useOriginalWallet) is typed to return WalletContextType but the actual context
uses EnhancedWalletContextType (which includes clearError, retryConnection,
getConnectionStatus), causing useEnhancedWallet to reimplement incomplete
methods. Fix by updating the return type of useWallet (or its cast) to
EnhancedWalletContextType so callers get the full interface; then remove the
duplicated/placeholder implementations in useEnhancedWallet (clearError no-op,
retryConnection logic) and instead call through to the context-provided
clearError, retryConnection and getConnectionStatus methods; ensure any usage of
WalletStateManager or connection helpers inside retryConnection delegates to the
context method rather than duplicating logic.
In `@frontend/src/utils/__tests__/walletState.test.ts`:
- Around line 226-228: The failing tests are due to mock state persisting across
tests because beforeEach only calls jest.clearAllMocks() but does not
reinitialize mockOriginalWallet; update the beforeEach to reset or recreate
mockOriginalWallet (or explicitly set its properties like isConnecting = false,
isInstalled = <initial>, etc.) so each test starts with a fresh, known wallet
state; locate the mockOriginalWallet declaration used by the tests and ensure
the beforeEach resets that object rather than relying solely on
jest.clearAllMocks().
---
Nitpick comments:
In `@frontend/src/components/wallet/EnhancedWalletConnect.tsx`:
- Line 21: The component EnhancedWalletConnect is calling the hook
useEnhancedWallet() twice (once at the top and again later); replace both calls
with a single call and destructure all required values (publicKey, balance,
network and any other props used later) from that single const expression so all
usages reference the same variables and avoid re-running the hook; remove the
duplicate call site and update any references to use the variables returned from
the consolidated const.
In `@frontend/src/hooks/useEnhancedWallet.ts`:
- Line 98: The current assignment evaluates wallet.canRetry() at render time
causing potential mismatch between the boolean and the returned function;
instead assign getRetryAction a stable function/closure that checks canRetry at
invocation time and then calls retryConnection if allowed (i.e., return a
function that when called checks wallet.canRetry() and then invokes
wallet.retryConnection(), or returns undefined), updating useEnhancedWallet's
return shape where needed so callers get a consistent callable retry action
(refer to wallet.canRetry and wallet.retryConnection in the hook).
In `@frontend/src/utils/walletState.ts`:
- Around line 140-141: The property assignment retryAction: canRetry ? undefined
: undefined is redundant and confusing; change it to a single explicit
assignment retryAction: undefined and keep (or move) the explanatory comment
that it will be set by context so intent remains clear; locate this in
walletState.ts where the object containing retryAction and canRetry is created
to update the property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceddd195-0a3f-487a-b138-4fcf9e85ecdd
📒 Files selected for processing (7)
frontend/WALLET_UX_GUIDE.mdfrontend/src/components/wallet/EnhancedWalletConnect.tsxfrontend/src/contexts/WalletContext.tsxfrontend/src/hooks/useEnhancedWallet.tsfrontend/src/types/wallet.tsfrontend/src/utils/__tests__/walletState.test.tsfrontend/src/utils/walletState.ts
Comment on lines
+59
to
+83
| // Update connection status whenever relevant state changes | ||
| useEffect(() => { | ||
| const connectionStatus = WalletStateManager.getConnectionStatus( | ||
| state.isConnected, | ||
| state.isConnecting, | ||
| state.error, | ||
| state.lastError, | ||
| ); | ||
|
|
||
| if (xlmBalance) { | ||
| setState((prev) => ({ | ||
| ...prev, | ||
| balance: { | ||
| asset: 'XLM', | ||
| balance: formatStellarAmount(xlmBalance.balance), | ||
| }, | ||
| error: null, | ||
| ...prev, | ||
| connectionStatus: { | ||
| ...connectionStatus, | ||
| retryAction: connectionStatus.canRetry | ||
| ? retryConnection | ||
| : undefined, | ||
| }, | ||
| })); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching balance:', error); | ||
| setState((prev) => ({ | ||
| ...prev, | ||
| balance: null, | ||
| error: 'Failed to fetch balance', | ||
| })); | ||
| } | ||
| }, []); | ||
|
|
||
| // Connect wallet | ||
| const connect = useCallback(async () => { | ||
| try { | ||
| setState((prev) => ({ ...prev, isConnecting: true, error: null })); | ||
|
|
||
| // Check if Freighter is installed | ||
| const installed = await checkFreighterInstalled(); | ||
| if (!installed) { | ||
| throw new WalletError( | ||
| 'NOT_INSTALLED' as WalletErrorCode, | ||
| 'Freighter wallet is not installed. Please install it from https://freighter.app' | ||
| ); | ||
| } | ||
|
|
||
| // Request connection | ||
| const allowed = await setAllowed(); | ||
| if (!allowed) { | ||
| throw new WalletError( | ||
| 'USER_REJECTED' as WalletErrorCode, | ||
| 'Connection request was rejected' | ||
| ); | ||
| } | ||
|
|
||
| // Get public key | ||
| const { address } = await getAddress(); | ||
| const publicKey = address; | ||
| if (!publicKey) { | ||
| throw new WalletError( | ||
| 'UNKNOWN_ERROR' as WalletErrorCode, | ||
| 'Failed to get public key from wallet' | ||
| ); | ||
| } | ||
|
|
||
| // Get network | ||
| let network: Network = defaultNetwork; | ||
| try { | ||
| const { network: freighterNetwork } = await getNetwork(); | ||
| if (freighterNetwork === 'PUBLIC') { | ||
| network = 'mainnet'; | ||
| } else if (freighterNetwork === 'TESTNET') { | ||
| network = 'testnet'; | ||
| } | ||
| } catch { | ||
| // Use default if getNetwork fails | ||
| network = defaultNetwork; | ||
| } | ||
|
|
||
| setState((prev) => ({ | ||
| ...prev, | ||
| isConnected: true, | ||
| isConnecting: false, | ||
| publicKey, | ||
| network, | ||
| error: null, | ||
| })); | ||
|
|
||
| // Fetch balance | ||
| await fetchBalance(publicKey, network); | ||
| } catch (error) { | ||
| let walletError: WalletError; | ||
|
|
||
| if (error instanceof WalletError) { | ||
| walletError = error; | ||
| } else if (error && typeof error === 'object' && 'message' in error) { | ||
| const message = String(error.message); | ||
| if (message.includes('locked') || message.includes('Locked')) { | ||
| walletError = new WalletError( | ||
| 'LOCKED' as WalletErrorCode, | ||
| 'Wallet is locked. Please unlock Freighter and try again.' | ||
| ); | ||
| } else if (message.includes('reject') || message.includes('Reject')) { | ||
| walletError = new WalletError( | ||
| 'USER_REJECTED' as WalletErrorCode, | ||
| 'Request was rejected by user' | ||
| ); | ||
| }, [ | ||
| state.isConnected, | ||
| state.isConnecting, | ||
| state.error, | ||
| state.lastError, | ||
| retryConnection, | ||
| ]); |
There was a problem hiding this comment.
Missing definitions for retryConnection and clearError referenced in effect and context value.
The useEffect at line 60 references retryConnection in its dependency array (line 82), and the context value (lines 489-490) includes both retryConnection and clearError. However, these functions are not defined in the provided code segment.
🔧 Functions that need to be defined before the effect
+ // Clear error state
+ const clearError = useCallback(() => {
+ setState((prev) => ({
+ ...prev,
+ error: null,
+ lastError: undefined,
+ }));
+ }, []);
+
+ // Retry connection
+ const retryConnection = useCallback(async () => {
+ if (state.connectionStatus.canRetry) {
+ await connect();
+ }
+ }, [state.connectionStatus.canRetry, connect]);
+
+ // Helper to set last error
+ const setLastError = useCallback((error: WalletError) => {
+ setState((prev) => ({
+ ...prev,
+ lastError: {
+ code: error.code,
+ message: error.message,
+ timestamp: Date.now(),
+ },
+ }));
+ }, []);
+
+ // Get connection status helper
+ const getConnectionStatus = useCallback(() => {
+ return state.connectionStatus;
+ }, [state.connectionStatus]);
+
// Update connection status whenever relevant state changes
useEffect(() => {📝 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.
Suggested change
| // Update connection status whenever relevant state changes | |
| useEffect(() => { | |
| const connectionStatus = WalletStateManager.getConnectionStatus( | |
| state.isConnected, | |
| state.isConnecting, | |
| state.error, | |
| state.lastError, | |
| ); | |
| if (xlmBalance) { | |
| setState((prev) => ({ | |
| ...prev, | |
| balance: { | |
| asset: 'XLM', | |
| balance: formatStellarAmount(xlmBalance.balance), | |
| }, | |
| error: null, | |
| ...prev, | |
| connectionStatus: { | |
| ...connectionStatus, | |
| retryAction: connectionStatus.canRetry | |
| ? retryConnection | |
| : undefined, | |
| }, | |
| })); | |
| } | |
| } catch (error) { | |
| console.error('Error fetching balance:', error); | |
| setState((prev) => ({ | |
| ...prev, | |
| balance: null, | |
| error: 'Failed to fetch balance', | |
| })); | |
| } | |
| }, []); | |
| // Connect wallet | |
| const connect = useCallback(async () => { | |
| try { | |
| setState((prev) => ({ ...prev, isConnecting: true, error: null })); | |
| // Check if Freighter is installed | |
| const installed = await checkFreighterInstalled(); | |
| if (!installed) { | |
| throw new WalletError( | |
| 'NOT_INSTALLED' as WalletErrorCode, | |
| 'Freighter wallet is not installed. Please install it from https://freighter.app' | |
| ); | |
| } | |
| // Request connection | |
| const allowed = await setAllowed(); | |
| if (!allowed) { | |
| throw new WalletError( | |
| 'USER_REJECTED' as WalletErrorCode, | |
| 'Connection request was rejected' | |
| ); | |
| } | |
| // Get public key | |
| const { address } = await getAddress(); | |
| const publicKey = address; | |
| if (!publicKey) { | |
| throw new WalletError( | |
| 'UNKNOWN_ERROR' as WalletErrorCode, | |
| 'Failed to get public key from wallet' | |
| ); | |
| } | |
| // Get network | |
| let network: Network = defaultNetwork; | |
| try { | |
| const { network: freighterNetwork } = await getNetwork(); | |
| if (freighterNetwork === 'PUBLIC') { | |
| network = 'mainnet'; | |
| } else if (freighterNetwork === 'TESTNET') { | |
| network = 'testnet'; | |
| } | |
| } catch { | |
| // Use default if getNetwork fails | |
| network = defaultNetwork; | |
| } | |
| setState((prev) => ({ | |
| ...prev, | |
| isConnected: true, | |
| isConnecting: false, | |
| publicKey, | |
| network, | |
| error: null, | |
| })); | |
| // Fetch balance | |
| await fetchBalance(publicKey, network); | |
| } catch (error) { | |
| let walletError: WalletError; | |
| if (error instanceof WalletError) { | |
| walletError = error; | |
| } else if (error && typeof error === 'object' && 'message' in error) { | |
| const message = String(error.message); | |
| if (message.includes('locked') || message.includes('Locked')) { | |
| walletError = new WalletError( | |
| 'LOCKED' as WalletErrorCode, | |
| 'Wallet is locked. Please unlock Freighter and try again.' | |
| ); | |
| } else if (message.includes('reject') || message.includes('Reject')) { | |
| walletError = new WalletError( | |
| 'USER_REJECTED' as WalletErrorCode, | |
| 'Request was rejected by user' | |
| ); | |
| }, [ | |
| state.isConnected, | |
| state.isConnecting, | |
| state.error, | |
| state.lastError, | |
| retryConnection, | |
| ]); | |
| // Clear error state | |
| const clearError = useCallback(() => { | |
| setState((prev) => ({ | |
| ...prev, | |
| error: null, | |
| lastError: undefined, | |
| })); | |
| }, []); | |
| // Retry connection | |
| const retryConnection = useCallback(async () => { | |
| if (state.connectionStatus.canRetry) { | |
| await connect(); | |
| } | |
| }, [state.connectionStatus.canRetry, connect]); | |
| // Helper to set last error | |
| const setLastError = useCallback((error: WalletError) => { | |
| setState((prev) => ({ | |
| ...prev, | |
| lastError: { | |
| code: error.code, | |
| message: error.message, | |
| timestamp: Date.now(), | |
| }, | |
| })); | |
| }, []); | |
| // Get connection status helper | |
| const getConnectionStatus = useCallback(() => { | |
| return state.connectionStatus; | |
| }, [state.connectionStatus]); | |
| // Update connection status whenever relevant state changes | |
| useEffect(() => { | |
| const connectionStatus = WalletStateManager.getConnectionStatus( | |
| state.isConnected, | |
| state.isConnecting, | |
| state.error, | |
| state.lastError, | |
| ); | |
| setState((prev) => ({ | |
| ...prev, | |
| connectionStatus: { | |
| ...connectionStatus, | |
| retryAction: connectionStatus.canRetry | |
| ? retryConnection | |
| : undefined, | |
| }, | |
| })); | |
| }, [ | |
| state.isConnected, | |
| state.isConnecting, | |
| state.error, | |
| state.lastError, | |
| retryConnection, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/contexts/WalletContext.tsx` around lines 59 - 83, Define the
missing retryConnection and clearError functions before the useEffect and export
them in the context value; implement retryConnection as a stable useCallback
that clears any transient error and triggers a reconnect attempt (e.g., setState
to set isConnecting=true and call the existing connect logic or
WalletStateManager connect function) and implement clearError as a useCallback
that clears state.error/lastError via setState, then reference these callbacks
in the useEffect dependency array and in the context value so
WalletStateManager.getConnectionStatus can assign retryAction correctly; ensure
both functions are memoized (useCallback) so they do not cause unnecessary
effect reruns.
Comment on lines
+118
to
+128
| // Check if Freighter is installed | ||
| const checkFreighterInstalled = useCallback(async (): Promise<boolean> => { | ||
| try { | ||
| // Try to access Freighter API | ||
| if (typeof window !== "undefined" && (window as any).freighterApi) { | ||
| return true; | ||
| } | ||
| // Fallback: try to call isConnected | ||
| await isConnected(); | ||
| return true; | ||
| // Initialize wallet connection on mount |
There was a problem hiding this comment.
Critical syntax error: checkFreighterInstalled has unclosed try block causing build failure.
The function opens a try block at line 120 but the code jumps directly to a new useEffect at line 129 without a catch/finally clause or closing brace. This causes the pipeline error: "Expected 'finally' but found ';'".
🐛 Proposed fix to complete the function
// Check if Freighter is installed
const checkFreighterInstalled = useCallback(async (): Promise<boolean> => {
try {
// Try to access Freighter API
if (typeof window !== "undefined" && (window as any).freighterApi) {
return true;
}
// Fallback: try to call isConnected
await isConnected();
return true;
+ } catch {
+ return false;
+ }
+ }, []);
+
// Initialize wallet connection on mount
useEffect(() => {📝 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.
Suggested change
| // Check if Freighter is installed | |
| const checkFreighterInstalled = useCallback(async (): Promise<boolean> => { | |
| try { | |
| // Try to access Freighter API | |
| if (typeof window !== "undefined" && (window as any).freighterApi) { | |
| return true; | |
| } | |
| // Fallback: try to call isConnected | |
| await isConnected(); | |
| return true; | |
| // Initialize wallet connection on mount | |
| // Check if Freighter is installed | |
| const checkFreighterInstalled = useCallback(async (): Promise<boolean> => { | |
| try { | |
| // Try to access Freighter API | |
| if (typeof window !== "undefined" && (window as any).freighterApi) { | |
| return true; | |
| } | |
| // Fallback: try to call isConnected | |
| await isConnected(); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }, []); | |
| // Initialize wallet connection on mount | |
| useEffect(() => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/contexts/WalletContext.tsx` around lines 118 - 128, The try
block inside the async function checkFreighterInstalled is left unclosed causing
a syntax error; close the try block and add a corresponding catch (or finally)
that handles errors and returns a boolean (e.g., return false on failure) so the
function always resolves to a boolean; specifically update the
checkFreighterInstalled function surrounding the try { ... await isConnected();
return true; } to include the missing closing brace and a catch that returns
false (or logs the error) to ensure the function and its callers (the useEffect
that initializes the wallet) compile and behave correctly.
Comment on lines
+482
to
+492
| const value: EnhancedWalletContextType = { | ||
| ...state, | ||
| connect, | ||
| disconnect, | ||
| switchNetwork, | ||
| refreshBalance, | ||
| signTransaction: signTransactionHandler, | ||
| retryConnection, | ||
| clearError, | ||
| getConnectionStatus, | ||
| }; |
There was a problem hiding this comment.
Context value references undefined functions.
The context value at lines 482-492 includes retryConnection, clearError, and getConnectionStatus, but none of these are defined in the provided code. Along with setLastError used throughout, these missing definitions will cause the build to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/contexts/WalletContext.tsx` around lines 482 - 492, The context
value references undefined functions (retryConnection, clearError,
getConnectionStatus) and relies on a missing setLastError setter; add
definitions for these symbols and wire them into the value object or remove them
if unnecessary. Implement retryConnection as a function that attempts to
reconnect (e.g., reuses the existing connect function and updates state),
implement clearError to clear the current error by calling setLastError(null) or
dispatching the appropriate action, and implement getConnectionStatus to return
the current status from state (e.g., state.status or derive it from other state
fields); ensure setLastError exists (as a state setter or dispatch helper) and
is used consistently where referenced in the file (e.g., in
signTransactionHandler and other handlers), then include the newly defined
retryConnection, clearError, and getConnectionStatus in the value object so the
build succeeds.
Comment on lines
+1
to
+4
| import { useWallet as useOriginalWallet } from "./useWallet"; | ||
| import type { WalletConnectionState } from "../types/wallet"; | ||
| import { WalletStateManager } from "../utils/walletState"; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the type mismatch in useWallet.ts
cat frontend/src/hooks/useWallet.tsRepository: OlufunbiIK/tip-tune
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Check the WalletContext definition
if [ -f "frontend/src/contexts/WalletContext.ts" ]; then
cat -n "frontend/src/contexts/WalletContext.ts"
else
find . -name "WalletContext*" -type f | head -5
fiRepository: OlufunbiIK/tip-tune
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check the wallet type definitions
if [ -f "frontend/src/types/wallet.ts" ]; then
cat -n "frontend/src/types/wallet.ts"
else
find . -name "wallet.ts" -o -name "wallet.tsx" | grep types
fiRepository: OlufunbiIK/tip-tune
Length of output: 3278
🏁 Script executed:
#!/bin/bash
# Check the WalletContext.tsx definition
cat -n "frontend/src/contexts/WalletContext.tsx"Repository: OlufunbiIK/tip-tune
Length of output: 20033
🏁 Script executed:
#!/bin/bash
# Check useEnhancedWallet to see what it reimplements
cat -n "frontend/src/hooks/useEnhancedWallet.ts"Repository: OlufunbiIK/tip-tune
Length of output: 5349
Type mismatch: useWallet returns WalletContextType but context provides EnhancedWalletContextType.
WalletContext is created with EnhancedWalletContextType | null (includes clearError, retryConnection, getConnectionStatus), but useWallet.ts casts the return to WalletContextType (which lacks these fields). This prevents access to methods the context already implements, forcing useEnhancedWallet to reimplement incomplete versions—clearError is a no-op with a TODO comment, and retryConnection unnecessarily duplicates the connection logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useEnhancedWallet.ts` around lines 1 - 4, The hook type is
wrong: useWallet (imported as useOriginalWallet) is typed to return
WalletContextType but the actual context uses EnhancedWalletContextType (which
includes clearError, retryConnection, getConnectionStatus), causing
useEnhancedWallet to reimplement incomplete methods. Fix by updating the return
type of useWallet (or its cast) to EnhancedWalletContextType so callers get the
full interface; then remove the duplicated/placeholder implementations in
useEnhancedWallet (clearError no-op, retryConnection logic) and instead call
through to the context-provided clearError, retryConnection and
getConnectionStatus methods; ensure any usage of WalletStateManager or
connection helpers inside retryConnection delegates to the context method rather
than duplicating logic.
Comment on lines
+67
to
+73
| const clearError = () => { | ||
| // This would need to be added to original wallet context | ||
| // For now, we'll just reconnect to clear the error | ||
| if (originalWallet.error && !originalWallet.isConnected) { | ||
| // Error will be cleared on next connection attempt | ||
| } | ||
| }; |
There was a problem hiding this comment.
clearError is a no-op and won't actually clear errors.
The function body only contains comments and a conditional with no action. Users clicking "Cancel" on error states will see no effect.
🐛 Proposed fix leveraging context's clearError
Either update useWallet.ts to return EnhancedWalletContextType (which includes clearError from context), or implement local error clearing:
const clearError = () => {
- // This would need to be added to original wallet context
- // For now, we'll just reconnect to clear the error
- if (originalWallet.error && !originalWallet.isConnected) {
- // Error will be cleared on next connection attempt
- }
+ // Note: This requires useWallet to expose clearError from context
+ // or updating useWallet.ts to return EnhancedWalletContextType
+ if ('clearError' in originalWallet && typeof originalWallet.clearError === 'function') {
+ (originalWallet as any).clearError();
+ }
};The proper fix is to update frontend/src/hooks/useWallet.ts to return EnhancedWalletContextType instead of WalletContextType.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useEnhancedWallet.ts` around lines 67 - 73, The clearError
implementation in useEnhancedWallet.ts is a no-op (clearError currently only
contains comments and an empty conditional on originalWallet.error), so
implement actual error clearing by wiring through the context's clearError:
update useWallet.ts to return EnhancedWalletContextType (including clearError)
instead of WalletContextType, then in useEnhancedWallet.ts call the
context-provided clearError (or, if you prefer a local fallback, invoke
originalWallet.clearError when present or call originalWallet.connect/disconnect
sequence only as a last resort) to ensure errors are actually cleared; reference
functions/types: clearError, originalWallet, useEnhancedWallet.ts, useWallet.ts,
EnhancedWalletContextType, WalletContextType.
Comment on lines
+226
to
+228
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Test pollution: mock object state persists across tests, causing assertion failure.
beforeEach calls jest.clearAllMocks() but doesn't reset mockOriginalWallet properties. The test at line 233 sets isConnecting = true, which persists to the test at line 252 where isConnecting is expected to be false initially—causing the pipeline failure.
🐛 Proposed fix to reset mock state
beforeEach(() => {
jest.clearAllMocks();
+ // Reset mock object to initial state
+ mockOriginalWallet.isConnected = false;
+ mockOriginalWallet.isConnecting = false;
+ mockOriginalWallet.publicKey = null;
+ mockOriginalWallet.network = "testnet";
+ mockOriginalWallet.balance = null;
+ mockOriginalWallet.error = null;
});📝 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.
Suggested change
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| }); | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| // Reset mock object to initial state | |
| mockOriginalWallet.isConnected = false; | |
| mockOriginalWallet.isConnecting = false; | |
| mockOriginalWallet.publicKey = null; | |
| mockOriginalWallet.network = "testnet"; | |
| mockOriginalWallet.balance = null; | |
| mockOriginalWallet.error = null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/__tests__/walletState.test.ts` around lines 226 - 228, The
failing tests are due to mock state persisting across tests because beforeEach
only calls jest.clearAllMocks() but does not reinitialize mockOriginalWallet;
update the beforeEach to reset or recreate mockOriginalWallet (or explicitly set
its properties like isConnecting = false, isInstalled = <initial>, etc.) so each
test starts with a fresh, known wallet state; locate the mockOriginalWallet
declaration used by the tests and ensure the beforeEach resets that object
rather than relying solely on jest.clearAllMocks().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Wallet Connection State UX Hardening
📋 Summary
This PR implements comprehensive wallet connection state UX hardening that normalizes UI states and retry affordances across all wallet connection flows. The solution provides consistent status indicators, actionable error messages, and clear next actions for every connection scenario.
🎯 Problem Addressed
Before: Wallet states (connecting, rejected, disconnected) were not consistently communicated across components, leading to confusing user experiences with unclear error messages and missing retry affordances.
After: Every connection outcome shows clear status and next action with normalized UI states and comprehensive retry affordances.
🔧 Key Changes
🎨 Enhanced State Management
disconnected,connecting,connected,rejected,locked,not_installed,error🧩 Enhanced Type System