Skip to content

Conversation

@ignaciosantise
Copy link
Collaborator

@ignaciosantise ignaciosantise commented Oct 6, 2025

This pull request improves the resilience of provider access in the AppKit library by handling cases where the provider may not be initialized, such as during session restoration or app restarts. The main changes add error handling and clarify return values to prevent crashes and improve stability.

Error handling and stability improvements:

  • Updated the getProvider method in AppKit to return null if the provider is not available or not yet initialized, and added a try/catch block to handle errors gracefully and log them during session restoration scenarios. [1] [2]
  • Updated the useProvider hook to wrap provider access in a try/catch block, returning undefined for the provider and provider type if the provider is not initialized, preventing errors during session restoration.

Note

Wrap provider access in try/catch, return null/undefined when uninitialized, and log errors in AppKit.getProvider and useProvider.

  • Provider access resilience
    • packages/appkit/src/AppKit.ts
      • getProvider: return type clarified to T | null; now guards missing connection/adapter and wraps connector.getProvider() in try/catch, returning null and logging on errors.
    • packages/appkit/src/hooks/useProvider.ts
      • useProvider: adds adapter presence check; wraps adapter.getProvider() in try/catch, returning { provider: undefined, providerType: undefined } on failure and logging the error.

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

@ignaciosantise ignaciosantise requested a review from Copilot October 6, 2025 18:06
@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2025

⚠️ No Changeset found

Latest commit: 9c9885f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

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

Click here to learn what changesets are, and how to add one.

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

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 in the AppKit library to prevent crashes when provider access fails during session restoration or app restarts. The changes add try-catch blocks around provider access methods to gracefully handle cases where providers are not yet initialized.

  • Added try-catch error handling to getProvider method in AppKit class
  • Added try-catch error handling to useProvider hook
  • Updated documentation to clarify return values when providers are unavailable

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/appkit/src/AppKit.ts Added try-catch block to getProvider method with error logging and updated JSDoc
packages/appkit/src/hooks/useProvider.ts Added try-catch block to useProvider hook to handle provider initialization errors

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@claude
Copy link

claude bot commented Oct 6, 2025

Claude finished @ignaciosantise's task —— View job


Code Review Complete

Tasks

  • Read modified files (AppKit.ts and useProvider.ts)
  • Analyze error handling implementation
  • Check type safety and return value consistency
  • Verify alignment with project conventions
  • Provide review feedback

Review Findings

1. Silent Error Swallowing in useProvider Hook - packages/appkit/src/hooks/useProvider.ts:50-52

The try-catch block in useProvider swallows errors without logging them, which differs from the error handling pattern used throughout the codebase (where LogController.sendError is consistently used).

} catch (error) {
  // Provider not initialized yet during session restoration
  return { provider: undefined, providerType: undefined };
}

Issue: This makes debugging difficult. If provider access fails for reasons other than initialization timing (e.g., adapter corruption, unexpected null reference), there's no trace of the error.

Recommendation: Add error logging consistent with the pattern used in AppKit.getProvider:

} catch (error) {
  // Provider not initialized yet during session restoration
  LogController.sendError(error, 'useProvider.ts', 'useProvider');
  return { provider: undefined, providerType: undefined };
}

2. Missing Changeset

The changeset bot indicates no changeset was added. Since this is a bug fix that affects the public API behavior (changing return values and adding error handling), a changeset should be added to track this change in the release notes.

Recommendation: Add a changeset for this PR by running:

npx changeset add

Select "patch" for affected packages and describe the fix.


3. Inconsistent Null Guards - packages/appkit/src/hooks/useProvider.ts:45-48

The hook checks for !connection but doesn't verify that connection.adapter exists before calling getProvider(). While the try-catch provides safety, an explicit null check would be more defensive and clearer:

if (!connection || !connection.adapter) {
  return { provider: undefined, providerType: undefined };
}

try {
  return {
    provider: connection.adapter.getProvider(),
    providerType: connection.adapter.getSupportedNamespace()
  };

This matches the defensive pattern used in AppKit.getProvider (lines 254-257).


4. Type Safety Consideration - packages/appkit/src/AppKit.ts:260

The getProvider method returns T | null, but the type assertion as T assumes getProvider() returns a type compatible with T. If the provider type doesn't match the expected generic type, this could cause runtime issues.

Current code:

return connection.adapter.connector.getProvider() as T;

Consider: Document in JSDoc that callers must ensure the generic type matches the actual provider type, or consider adding runtime type validation for critical scenarios.


Summary

The error handling improvements are good and align with the project's goal of preventing crashes during session restoration. However:

  1. Critical: Add error logging in useProvider hook for consistency and debuggability
  2. Required: Add a changeset for this PR
  3. Recommended: Add explicit null checks for connection.adapter before accessing it
  4. Nice-to-have: Consider enhancing JSDoc for the generic type parameter in getProvider

The changes follow React Native and TypeScript best practices, and the try-catch approach is appropriate for handling async initialization timing issues.


@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2025

@ignaciosantise
Copy link
Collaborator Author

@claude review

@claude
Copy link

claude bot commented Oct 6, 2025

Claude finished @ignaciosantise's task —— View job


🔄 Incremental Review:

Tasks

  • Read modified files to understand current state
  • Validate previously reported issues
  • Check for new issues
  • Provide review feedback

Review Findings

✅ Resolved Issues from Previous Review

  1. Error logging in useProvider hook - packages/appkit/src/hooks/useProvider.ts:53

    • Status:RESOLVED
    • The hook now correctly logs errors using LogController.sendError(error, 'useProvider', 'useProvider') which matches the project's error handling pattern.
  2. Null guard improvements - packages/appkit/src/hooks/useProvider.ts:43-44

    • Status:RESOLVED
    • The hook now checks both !connection and !connection.adapter before attempting to access the provider, providing defensive programming consistent with the codebase.

🔴 Issues Still Present

  1. Missing Changeset - CRITICAL

    • The changeset bot is still flagging this PR as missing a changeset
    • Since this is a bug fix affecting public API behavior (return values and error handling), a changeset is required
    • Action Required: Run npx changeset add and select "patch" for affected packages
  2. Type Safety Consideration - packages/appkit/src/AppKit.ts:260

    • The getProvider method returns T | null but uses as T | null type assertion without runtime validation
    • While this is acceptable for flexibility, it assumes callers ensure the generic type matches the actual provider type
    • Recommendation: Consider adding JSDoc documentation noting that callers must ensure type compatibility

📝 Summary

The latest commits successfully addressed the two main code quality issues:

  • ✅ Added error logging in useProvider hook (line 53)
  • ✅ Added explicit null check for connection.adapter (line 43)

The error handling implementation now follows project conventions consistently. The remaining action item is to add a changeset for proper version tracking.


No new issues found in the latest commits. The code improvements are solid and align with React Native and TypeScript best practices.


@ignaciosantise ignaciosantise merged commit 09233b6 into feat/multichain Oct 6, 2025
8 checks passed
@ignaciosantise ignaciosantise deleted the fix/provider-hook branch October 6, 2025 18:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 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