Skip to content

Conversation

@ignaciosantise
Copy link
Collaborator

@ignaciosantise ignaciosantise commented Nov 13, 2025

This pull request introduces improved error handling and event tracking for user-initiated rejection and proposal expiry during wallet connection flows. The changes add new utility methods for error identification, update event types to distinguish between general connection errors and user rejections, and enhance user feedback in the UI.

Error handling improvements:

  • Added new methods to ErrorUtil (isRpcProviderError, isUserRejectedMessage, isUserRejectedRequestError, isProposalExpiredError) to more accurately identify user rejection and proposal expiry errors. Also defined new RPC error codes for user rejection scenarios. [1] [2]

UI and feedback enhancements:

  • Updated the ConnectingView component to use the new error utility methods for distinguishing between user rejection and other errors. Now shows a specific error message ("User rejected the request") when appropriate, and only displays generic errors for other cases. [1] [2]

Event tracking updates:

  • Added a new event type 'USER_REJECTED' to the EventName union and corresponding event object to the Event type, allowing the system to track user-initiated rejection separately from general connection errors. [1] [2]

Note

Renews WalletConnect URI on failures, adds user-rejection error utilities and event, updates connecting views to use them, and tweaks text truncation in UI.

  • Connection flow:
    • Renew WC URI on errors; await connect promise and auto-retry for QR with timestamp guard.
    • Use ErrorUtil to distinguish user rejection vs other/proposal-expired errors; show specific snackbars and map to USER_REJECTED vs CONNECT_ERROR events.
  • Common utils:
    • Add ErrorUtil RPC codes and helpers: isRpcProviderError, isUserRejectedMessage, isUserRejectedRequestError, isProposalExpiredError, isWalletNotFoundError, categorizeConnectionError.
  • Events/types:
    • Extend EventName and Event with USER_REJECTED tracking.
  • UI:
    • Make ConnectingBody title centered and single-line; add flexShrink to header/store texts to prevent truncation issues.

Written by Cursor Bugbot for commit d8c49ea. This will update automatically on new commits. Configure here.

Copilot AI review requested due to automatic review settings November 13, 2025 20:12
@changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2025

🦋 Changeset detected

Latest commit: d8c49ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@reown/appkit-react-native Patch
@reown/appkit-common-react-native Patch
@reown/appkit-bitcoin-react-native Patch
@reown/appkit-coinbase-react-native Patch
@reown/appkit-core-react-native Patch
@reown/appkit-ethers-react-native Patch
@reown/appkit-solana-react-native Patch
@reown/appkit-ui-react-native Patch
@reown/appkit-wagmi-react-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link

claude bot commented Nov 13, 2025

Claude finished @ignaciosantise's task —— View job


Code Review Complete ✓

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Review ErrorUtil changes
  • Review event types changes
  • Review ConnectingView component changes
  • Analyze code quality, security, and best practices
  • Post comprehensive review feedback

Review Findings

1. Missing Changeset ⚠️

The PR is missing a changeset file. This is required for version management. Please add a changeset using the link provided by the bot.

2. Inconsistent Error Handling Pattern

File: packages/appkit/src/views/w3m-connecting-external-view/index.tsx:99-100

The ConnectingExternalView component uses regex pattern matching to detect user rejection:

} else if (/(rejected)/i.test((error as Error).message)) {
  setErrorType('declined');
}

However, it doesn't use the new ErrorUtil.isUserRejectedRequestError() method introduced in this PR. For consistency and maintainability, this file should be updated to use the centralized error detection utility.

Recommendation: Update ConnectingExternalView to use ErrorUtil.isUserRejectedRequestError() and potentially also track USER_REJECTED events similar to the ConnectingView component.

3. Weak Proposal Expiry Detection

File: packages/common/src/utils/ErrorUtil.ts:82-84

isProposalExpiredError(error: any) {
  return error?.message?.includes('expired');
}

Issues:

  • Uses loose string matching that could produce false positives (e.g., "certificate expired", "session expired")
  • Doesn't check if the error is an RPC provider error first
  • No type safety for the error parameter

Recommendation:

isProposalExpiredError(error: any) {
  if (ErrorUtil.isRpcProviderError(error)) {
    return error.message?.toLowerCase().includes('proposal expired');
  }
  
  if (error instanceof Error) {
    return error.message?.toLowerCase().includes('proposal expired');
  }
  
  return false;
}

4. Missing Test Coverage

Impact: Medium

No test files exist for:

  • ErrorUtil utility methods (critical for error handling reliability)
  • ConnectingView component (integration tests for error scenarios)

Recommendation: Add unit tests for the new ErrorUtil methods, especially:

  • isUserRejectedRequestError() with various error codes and message formats
  • isProposalExpiredError() to prevent false positives
  • Edge cases (null, undefined, non-error objects)

5. Type Safety Concern

File: packages/common/src/utils/ErrorUtil.ts:40-54

The isRpcProviderError() method uses any type and runtime checks. While this is reasonable for error handling, consider adding a TypeScript type guard return type:

isRpcProviderError(error: any): error is { message: string; code: number } {
  // ... existing implementation
}

This would provide better type narrowing when using the method.

6. Event Type Missing Additional Context

File: packages/common/src/types/api/events.ts:157-164

The new USER_REJECTED event type has the same structure as CONNECT_ERROR. Consider adding additional context:

| {
    type: 'track';
    address?: string;
    event: 'USER_REJECTED';
    properties: {
      message: string;
      errorCode?: number;  // Add RPC error code if available
      wallet?: string;     // Add wallet name/id if available
    };
  }

This would provide richer analytics for understanding rejection patterns.

7. Missing JSDoc Documentation

File: packages/common/src/utils/ErrorUtil.ts

The new methods lack JSDoc comments. According to project guidelines, public APIs should have JSDoc documentation. Add documentation for:

  • isRpcProviderError()
  • isUserRejectedMessage()
  • isUserRejectedRequestError()
  • isProposalExpiredError()

Example:

/**
 * Checks if an error is an RPC provider error with code and message properties
 * @param error - The error object to check
 * @returns True if the error is an RPC provider error
 */
isRpcProviderError(error: any): error is { message: string; code: number } {
  // ...
}

Summary

Strengths:

  • Good separation of concerns with dedicated error utility methods
  • Improved user experience with specific error messages
  • New event type for better analytics tracking

Required Actions:

  1. Add changeset file (blocking for merge)
  2. Fix weak proposal expiry detection logic
  3. Update ConnectingExternalView to use centralized error utilities

Recommended Actions:

  1. Add test coverage for new error utility methods
  2. Add JSDoc documentation
  3. Enhance event type with additional context
  4. Consider adding type guards for better type safety

Note: I cannot run linters, type checkers, or tests in this environment. Please ensure you run:

  • npm run lint (or equivalent)
  • npm run typecheck (or equivalent)
  • npm test (or equivalent)
  • Build the project to verify no runtime issues

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances error handling for wallet connection flows by introducing more granular error identification and tracking. The changes improve user experience by distinguishing between user-initiated rejections, proposal expiry, and general connection errors, providing more appropriate feedback and analytics tracking for each case.

Key Changes

  • Added utility methods in ErrorUtil to identify RPC provider errors, user rejection errors, and proposal expiry errors
  • Introduced a new USER_REJECTED event type for tracking user-initiated rejections separately from general connection errors
  • Updated ConnectingView to use the new error utilities and provide context-specific error messages to users

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
packages/common/src/utils/ErrorUtil.ts Added RPC error code constants and four new error detection methods to identify and categorize different types of connection errors
packages/common/src/types/api/events.ts Added USER_REJECTED event type and corresponding event structure for tracking user rejection events
packages/appkit/src/views/w3m-connecting-view/index.tsx Updated error handling to use new error utilities, show context-appropriate error messages, and track user rejections separately

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ignaciosantise
Copy link
Collaborator Author

@claude review

@claude
Copy link

claude bot commented Nov 13, 2025

Claude finished @ignaciosantise's task —— View job


🔄 Incremental Review: Code Review Complete

Todo List:

  • Read repository guidelines (CLAUDE.md not present)
  • Review all changed files
  • Validate previously reported issues
  • Identify new issues
  • Post comprehensive review feedback

Validation of Previous Review Comments

✅ Resolved Issues

  1. Missing Changeset (reported by previous Claude review)

    • Status: ✅ Resolved
    • Changeset file now exists at .changeset/cool-parks-slide.md
  2. Unused RPC_ERROR_CODE.USER_REJECTED constant (reported by Copilot)

    • Status: ❌ Confirmed still present
    • Line ErrorUtil.ts:2 defines USER_REJECTED: 5000 but it's never used in isUserRejectedRequestError
    • Only checks for codes 4001 and 5002
  3. Redundant optional chaining (reported by Copilot)

    • Status: ❌ Confirmed still present
    • ErrorUtil.ts:66 uses error?.code when isRpcProviderError already confirms error.code exists
    • Should be error.code without optional chaining
  4. Weak proposal expiry detection (reported by Cursor & previous Claude review)

    • Status: ✅ Improved but still has issues
    • Now checks for RPC provider error first (lines 82-84)
    • However, still searches for generic "proposal expired" string that could match unrelated errors
    • The fix in lines 86-91 adds type checking, which is good
  5. Premature error display during retry (reported by Cursor)

    • Status: ✅ Working as intended (confirmed by author)
    • Author confirmed the QR retry behavior is intentional

New Issues Found

1. Type Safety: Inconsistent Error Type Narrowing

File: packages/common/src/utils/ErrorUtil.ts:66
Severity: Low

const isUserRejectedMethodsCode =
  error?.code === ErrorUtil.RPC_ERROR_CODE.USER_REJECTED_METHODS;

After isRpcProviderError(error) returns true on line 63, TypeScript knows error is { message: string; code: number }. The optional chaining error?.code is redundant and contradicts the type guard.

Recommendation:

const isUserRejectedMethodsCode =
  error.code === ErrorUtil.RPC_ERROR_CODE.USER_REJECTED_METHODS;

2. Unused Constant: USER_REJECTED (5000)

File: packages/common/src/utils/ErrorUtil.ts:2-5
Severity: Low

The constant USER_REJECTED: 5000 is defined but never used. Only codes 4001 and 5002 are checked in the error detection logic.

Options:

  • Remove the unused constant if 5000 is not a valid error code
  • Add it to the checks in isUserRejectedRequestError if it should be detected

3. Overly Broad String Matching

File: packages/common/src/utils/ErrorUtil.ts:83, 89
Severity: Medium

The proposal expiry check uses a broad substring match:

error.message?.toLowerCase().includes('proposal expired')

While better than just 'expired', this could still match variations like:

  • "The proposal expired yesterday"
  • "Another proposal expired"

For WalletConnect, proposal expiry errors likely have a specific format. Consider using a more precise check or regex pattern if the exact error message format is known.


4. Missing JSDoc Documentation

File: packages/common/src/utils/ErrorUtil.ts:39-113
Severity: Low

The new utility methods lack JSDoc documentation. According to React Native best practices, public APIs should be documented with JSDoc comments explaining:

  • Purpose of each method
  • Parameter types and meanings
  • Return values
  • Example usage (optional)

Example:

/**
 * Checks if an error is an RPC provider error with required code and message properties
 * @param error - The error object to check
 * @returns True if the error has both `code` (number) and `message` (string) properties
 */
isRpcProviderError(error: any): error is { message: string; code: number } {
  // ...
}

5. Event Type Missing Context Properties

File: packages/common/src/types/api/events.ts:157-164
Severity: Low (Enhancement)

The new USER_REJECTED event has the same structure as CONNECT_ERROR:

| {
    type: 'track';
    address?: string;
    event: 'USER_REJECTED';
    properties: {
      message: string;
    };
  }

Recommendation: Add optional context properties for richer analytics:

properties: {
  message: string;
  errorCode?: number;     // RPC error code if available
  walletId?: string;      // Wallet identifier
  reconnect?: boolean;    // Was this a reconnection attempt?
}

This would help track patterns like:

  • Which wallets have higher rejection rates?
  • Which error codes are most common?
  • Are rejections more common on initial connections or reconnections?

6. Potential Race Condition in Retry Logic

File: packages/appkit/src/views/w3m-connecting-view/index.tsx:56-64
Severity: Medium

The retry logic in the catch block has a potential issue:

catch (error) {
  LogController.sendError(error, 'ConnectingView.tsx', 'initializeConnection');
  WcController.setWcError(true);
  WcController.clearUri();

  if (isQr && CoreHelperUtil.isAllowedRetry(lastRetry)) {
    setLastRetry(Date.now());
    initializeConnection(true);  // Recursive call
  }

  const isUserRejected = ErrorUtil.isUserRejectedRequestError(error);
  // ... error handling continues

Issues:

  • When initializeConnection(true) is called recursively on line 63, the code continues executing
  • Lines 66-80 still run, showing error messages and sending events
  • If the retry succeeds, the user still sees the error from the first attempt

Recommendation:

if (isQr && CoreHelperUtil.isAllowedRetry(lastRetry)) {
  setLastRetry(Date.now());
  initializeConnection(true);
  return; // Don't show errors if we're retrying
}

This ensures errors are only shown when no retry is attempted.


Summary

Still Present from Previous Reviews:

  • ❌ Unused USER_REJECTED: 5000 constant
  • ❌ Redundant optional chaining on line 66
  • ⚠️ Broad "proposal expired" string matching (partially improved)

New Issues:

  • Missing JSDoc documentation for public API methods
  • Event type could include more context for analytics
  • Potential race condition in retry logic causing premature error display

Positive Observations:

  • Good separation of concerns with ErrorUtil
  • Improved error detection with specific RPC error codes
  • Better UX with user-friendly error messages
  • Proper event tracking for analytics
  • Changeset file now present

Recommended Actions

High Priority:

  1. Fix potential race condition in retry logic (add return after retry call)
  2. Remove unused constant or add it to checks

Medium Priority:
3. Remove redundant optional chaining for consistency
4. Consider more specific "proposal expired" detection

Low Priority:
5. Add JSDoc documentation for public API methods
6. Enhance event type with additional context properties


Note: I cannot run linters, type checkers, or tests in this environment. Please ensure you run:

  • npm run lint (or equivalent)
  • npm run typecheck (or equivalent)
  • npm test (or equivalent)

to validate the changes before merging.


View job

@sonarqubecloud
Copy link

@ignaciosantise ignaciosantise merged commit 2cdbf19 into develop Nov 14, 2025
15 checks passed
@ignaciosantise ignaciosantise deleted the fix/renew-uri branch November 14, 2025 14:28
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants