[SDK] fix: ignore EIP-1193 code 1013 transient disconnect in injected…#8807
Merged
0xFirekeeper merged 6 commits intoJun 15, 2026
Merged
Conversation
… wallets MetaMask fires a provider disconnect event with error code 1013 during transient states such as chain changes or RPC hiccups. The handler was ignoring the error argument, treating code 1013 as a permanent disconnect and triggering spurious app logouts. Now filters code 1013 and forwards the error payload to subscribers. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 2628e3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR suppresses spurious wallet disconnect events caused by EIP-1193 transient reconnect signals (error code ChangesInjected wallet transient disconnect fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/injected/index.ts (1)
459-469: ⚡ Quick winUse the shared disconnect error contract and add an explicit return type on
onDisconnect.Line 459 duplicates the payload shape inline and omits an explicit return type. Reuse
WalletDisconnectErrorfrompackages/thirdweb/src/wallets/wallet-emitter.tsand annotatePromise<void>to keep the contract single-sourced and guideline-compliant.♻️ Suggested change
-import type { WalletEmitter } from "../wallet-emitter.js"; +import type { WalletDisconnectError, WalletEmitter } from "../wallet-emitter.js"; @@ - async function onDisconnect(error?: { code?: number; message?: string }) { + async function onDisconnect( + error?: WalletDisconnectError, + ): Promise<void> {As per coding guidelines,
**/*.{ts,tsx}should “Write idiomatic TypeScript with explicit function declarations and return types” and “Re-use shared types from@/typesor localtypes.tsbarrel exports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/thirdweb/src/wallets/injected/index.ts` around lines 459 - 469, The onDisconnect function duplicates the error parameter type inline and lacks an explicit return type annotation. Import the shared WalletDisconnectError type from the wallet-emitter module and use it to replace the inline parameter type definition on the onDisconnect function signature. Then add an explicit Promise<void> return type annotation to comply with TypeScript guidelines and maintain a single source of truth for the disconnect error contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/thirdweb/src/wallets/injected/index.ts`:
- Around line 459-469: The onDisconnect function duplicates the error parameter
type inline and lacks an explicit return type annotation. Import the shared
WalletDisconnectError type from the wallet-emitter module and use it to replace
the inline parameter type definition on the onDisconnect function signature.
Then add an explicit Promise<void> return type annotation to comply with
TypeScript guidelines and maintain a single source of truth for the disconnect
error contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92aa556e-1f04-449a-8121-eddf44e4393a
📒 Files selected for processing (3)
.changeset/fix-injected-wallet-transient-disconnect.mdpackages/thirdweb/src/wallets/injected/index.tspackages/thirdweb/src/wallets/wallet-emitter.ts
Contributor
size-limit report 📦
|
…onnect - Import WalletDisconnectError in injected/index.ts instead of inline type - Add explicit Promise<void> return type to onDisconnect - Export WalletDisconnectError from wallets.ts and wallets.native.ts to fix knip unused-type lint error Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Bump NODE_OPTIONS max-old-space-size in packages/thirdweb/package.json test script from 8192 to 12288 to provide more heap for vitest runs (coverage-heavy tests) and avoid OOM failures.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8807 +/- ##
==========================================
- Coverage 52.78% 52.69% -0.09%
==========================================
Files 934 934
Lines 62976 62979 +3
Branches 4137 4143 +6
==========================================
- Hits 33241 33187 -54
- Misses 29635 29692 +57
Partials 100 100
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/injected/index.test.ts (1)
13-18: ⚡ Quick winTighten helper typing to avoid
any/unknownand add an explicit return type.Line 13 (
any[]), Line 36 (as unknown as EIP1193Provider), and Line 15 (missing explicit return type) reduce type-safety in this test helper.Suggested patch
- // biome-ignore lint/suspicious/noExplicitAny: test listener registry - type Listener = (...args: any[]) => void; + type DisconnectListener = ( + error?: WalletDisconnectError, + ) => void | Promise<void>; - async function connectAndGetDisconnectHandler() { + async function connectAndGetDisconnectHandler(): Promise<{ + disconnectHandler: DisconnectListener; + onDisconnect: ReturnType<typeof vi.fn>; + provider: EIP1193Provider; + }> { let disconnectHandler: - | ((error?: WalletDisconnectError) => void) + | DisconnectListener | undefined; const provider = { - on: vi.fn((event: string, listener: Listener) => { + on: vi.fn((event: string, listener: DisconnectListener) => { if (event === "disconnect") { disconnectHandler = listener; } }), removeListener: vi.fn(), request: vi.fn(async ({ method }: { method: string }) => { if (method === "eth_accounts") { return [TEST_ACCOUNT_A.address]; } if (method === "eth_chainId") { return "0x1"; } return undefined; }), - } as unknown as EIP1193Provider; + } satisfies EIP1193Provider;As per coding guidelines, “Write idiomatic TypeScript with explicit function declarations and return types” and “Avoid
anyandunknownin TypeScript unless unavoidable.”Also applies to: 20-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/thirdweb/src/wallets/injected/index.test.ts` around lines 13 - 18, The test helper has three type-safety issues: the Listener type on line 13 uses any[] instead of a specific type signature, the connectAndGetDisconnectHandler function on line 15 lacks an explicit return type declaration, and within the function body (around lines 20-37) there is an unsafe as unknown as EIP1193Provider type cast. Replace the any[] in the Listener type with a specific function signature that matches the expected parameters, add an explicit return type to the connectAndGetDisconnectHandler function (likely a Promise wrapping the disconnect handler and provider), and remove the unsafe unknown intermediate cast by using proper type annotations or imports to correctly type the EIP1193Provider creation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/thirdweb/src/extensions/modules/MintableERC721/mintableERC721.test.ts`:
- Line 24: The test suite for ModularTokenERC721 in the mintableERC721.test.ts
file is disabled with describe.skip on line 24, which removes all test coverage
and can hide regressions. Remove the .skip modifier from the describe call to
re-enable the entire test suite. If only certain tests within the suite need to
be skipped, apply .skip to those individual test cases instead of disabling the
entire suite.
---
Nitpick comments:
In `@packages/thirdweb/src/wallets/injected/index.test.ts`:
- Around line 13-18: The test helper has three type-safety issues: the Listener
type on line 13 uses any[] instead of a specific type signature, the
connectAndGetDisconnectHandler function on line 15 lacks an explicit return type
declaration, and within the function body (around lines 20-37) there is an
unsafe as unknown as EIP1193Provider type cast. Replace the any[] in the
Listener type with a specific function signature that matches the expected
parameters, add an explicit return type to the connectAndGetDisconnectHandler
function (likely a Promise wrapping the disconnect handler and provider), and
remove the unsafe unknown intermediate cast by using proper type annotations or
imports to correctly type the EIP1193Provider creation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bca3047-614a-4d49-b624-758f05a2f2ee
📒 Files selected for processing (2)
packages/thirdweb/src/extensions/modules/MintableERC721/mintableERC721.test.tspackages/thirdweb/src/wallets/injected/index.test.ts
0xFirekeeper
deleted the
sdk/fix-injected-wallet-1013-transient-disconnect
branch
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.
… wallets
MetaMask fires a provider disconnect event with error code 1013 during transient states such as chain changes or RPC hiccups. The handler was ignoring the error argument, treating code 1013 as a permanent disconnect and triggering spurious app logouts. Now filters code 1013 and forwards the error payload to subscribers.
PR-Codex overview
This PR focuses on improving the handling of disconnect events in wallet integrations, particularly for transient errors from injected wallets like MetaMask. It ensures that temporary disconnects do not trigger spurious logouts and updates the relevant types and event handling.
Detailed summary
WalletDisconnectErrortype to handle disconnect errors with code and message.WalletEmitterEventsto changedisconnectfromnevertoWalletDisconnectError | undefined.onDisconnectfunction to ignore transient disconnect errors (code 1013).testscript inpackage.jsonto adjust memory limits.describein tests to skip certain tests instead of running them.Summary by CodeRabbit
1013), preventing spuriousdisconnectevents during reconnect scenarios.WalletDisconnectError, and updated the publicdisconnectevent contract to deliverWalletDisconnectError | undefined.onDisconnecttransient vs non-transient behavior.MintableERC721test suite.