Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
9 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 9323651 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
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 |
Visual Regression Test Results ✅ PassedChromatic Build: https://www.chromatic.com/build?appId=6493191bf4b10fed8ca7353f&number=779 👉 Please review the visual changes in Chromatic and accept or reject them. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where mobile WalletConnect connections that fail would not properly set the wcError flag, leaving the application in an inconsistent state. The fix adds a catch handler to capture connection failures and update the error state accordingly.
Changes:
- Added error handling to
connectWalletConnectmethod in ConnectionController to setwcError,wcFetchingUri, andstatusflags when connection fails - Bumped package version from 1.8.17 to 1.8.18
- Added changeset documenting the fix
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/controllers/src/controllers/ConnectionController.ts | Added catch handler to set error state flags when WalletConnect connection fails |
| packages/appkit/exports/constants.ts | Version bump to 1.8.18 |
| .changeset/quiet-hands-raise.md | Changeset documenting the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .catch(() => { | ||
| state.wcError = true | ||
| state.wcFetchingUri = false | ||
| state.status = 'disconnected' | ||
|
|
||
| return undefined | ||
| }) |
There was a problem hiding this comment.
Consider adding a test case that verifies the error handling behavior when connectWalletConnect fails. The test should check that wcError, wcFetchingUri, and status are set correctly when the promise rejects. This would ensure the bug fix is properly covered and prevent regressions. See existing test at line 293 of ConnectionController.test.ts for a similar pattern.
| .catch(() => { | ||
| state.wcError = true | ||
| state.wcFetchingUri = false | ||
| state.status = 'disconnected' | ||
|
|
||
| return undefined | ||
| }) |
There was a problem hiding this comment.
There is a logic error in the error handling flow. When the promise rejects and the catch handler sets status to 'disconnected' (line 252), the promise continues execution. After awaiting the promise (line 257), the code unconditionally sets status to 'connected' (line 260), which overwrites the 'disconnected' status from the error handler. This means that even when an error occurs, the status will end up as 'connected'. Consider returning early from the function after the await if an error occurred, or check if wcError is true before setting the status to 'connected'.
| state.wcError = true | ||
| state.wcFetchingUri = false | ||
| state.status = 'disconnected' |
There was a problem hiding this comment.
The error handling here is inconsistent with the existing setWcError method (line 456-460). The setWcError method also sets state.buffering to false when handling errors. For consistency, state.buffering should also be set to false here, or consider using ConnectionController.setWcError(true) to ensure all related state is properly reset.
| state.wcError = true | |
| state.wcFetchingUri = false | |
| state.status = 'disconnected' | |
| ConnectionController.setWcError(true) |
📦 Bundle Size Check✅ All bundles are within size limits 📊 View detailed bundle sizes> @reown/appkit-monorepo@1.7.1 size /home/runner/work/appkit/appkit > size-limit |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Refactors .catch() to try/catch so status correctly stays 'disconnected' on error instead of being overwritten to 'connected'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added tests and a small refactor for the error handling: Refactor: Replaced Tests (3 new):
|
| } else { | ||
| await ConnectionController._getClient()?.connectWalletConnect?.() | ||
| } |
There was a problem hiding this comment.
do we want to also add a catch here as well?
…atch Lifts try/catch to wrap both cached and non-cached branches so error state (wcError, wcFetchingUri, status) is set consistently. Re-throws so callers like w3m-connecting-wc-view can still handle errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Description
wcErrorflagType of change