-
Notifications
You must be signed in to change notification settings - Fork 615
[wagmi-adapter] Add onConnect callback #8131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 4c0641d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Connector as wagmi-adapter Connector
participant Wallet as InApp Wallet
rect rgba(200,220,255,0.20)
note right of App: App initiates connect (initial or reconnect) with optional onConnect
App->>Connector: connect({ ..., onConnect })
alt Reconnecting path
Connector->>Wallet: autoConnect({ ..., onConnect })
Wallet-->>Connector: wallet instance
else Provider init path
Connector->>Wallet: autoConnect({ ..., onConnect })
Wallet-->>Connector: wallet instance
end
Connector->>Connector: args.onConnect?.(wallet)
Note right of Connector: onConnect callback executed with connected wallet
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8131 +/- ##
=======================================
Coverage 56.28% 56.28%
=======================================
Files 906 906
Lines 59193 59193
Branches 4172 4172
=======================================
Hits 33316 33316
Misses 25772 25772
Partials 105 105
🚀 New features to boost your workflow:
|
d727bad to
4c0641d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/wagmi-adapter/src/connector.ts (3)
111-112: Forward only defined callbacks toautoConnect.Passing
onConnect: undefinedcreates noisy runtime payloads and obscures intent. Wrap the property so it’s sent only when provided.- await autoConnect({ - chain: defineChain(chainId), - client, - wallets: [wallet], - onConnect: args.onConnect, - }); + await autoConnect({ + chain: defineChain(chainId), + client, + wallets: [wallet], + ...(args.onConnect && { onConnect: args.onConnect }), + });
146-147: Guard callback for smart-account wrappers as well.When
smartAccountis enabled,walletbecomes anInAppWalletWithSmartAccount, which won't type-match(wallet: InAppWallet) => voidand could cause runtime surprises for consumers expecting the base wallet. Consider normalizing to the underlying wallet instance before invoking the callback.You can normalize like this:
- args.onConnect?.(wallet); + if (args.onConnect) { + const baseWallet = "getPersonalWallet" in wallet + ? wallet.getPersonalWallet() + : wallet; + args.onConnect(baseWallet); + }
170-174: Same optional spread nit as above.Keep the payload clean by omitting the
onConnectkey when it’s not supplied.await autoConnect({ chain, client, wallets: [wallet], - onConnect: args.onConnect, + ...(args.onConnect && { onConnect: args.onConnect }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/famous-places-hug.md(1 hunks)packages/wagmi-adapter/src/connector.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/wagmi-adapter/src/connector.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/wagmi-adapter/src/connector.ts
packages/wagmi-adapter/**
📄 CodeRabbit inference engine (CLAUDE.md)
Wagmi ecosystem integration is in
packages/wagmi-adapter/
Files:
packages/wagmi-adapter/src/connector.ts
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/*.md: Each change inpackages/*must include a changeset for the appropriate package
Version bump rules: patch for non‑API changes; minor for new/modified public API
Files:
.changeset/famous-places-hug.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Size
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
.changeset/famous-places-hug.md (1)
1-5: Looks good — changeset formatted correctly.The package bump level and summary align with the surfaced API change. 👍
| onConnect?: (wallet: Wallet) => void; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type mismatch: callback should receive InAppWallet type.
ecosystemWallet / thirdwebInAppWallet both return an InAppWallet instance (or InAppWalletWithSmartAccount), not the broader Wallet union. Typing onConnect as (wallet: Wallet) => void forces downstream consumers to handle wallet shapes we never pass here and can break inference in existing code that narrows to in-app wallets. Please type the callback to the concrete in-app wallet type we actually hand back.
Apply this diff to align the type:
-import {
- EIP1193,
- ecosystemWallet,
- type InAppWalletConnectionOptions,
- type InAppWalletCreationOptions,
- type MultiStepAuthArgsType,
- type SingleStepAuthArgsType,
- inAppWallet as thirdwebInAppWallet,
- type Wallet,
-} from "thirdweb/wallets";
+import {
+ EIP1193,
+ ecosystemWallet,
+ type InAppWallet,
+ type InAppWalletConnectionOptions,
+ type InAppWalletCreationOptions,
+ type MultiStepAuthArgsType,
+ type SingleStepAuthArgsType,
+ inAppWallet as thirdwebInAppWallet,
+} from "thirdweb/wallets";
@@
- ecosystemId?: `ecosystem.${string}`;
- onConnect?: (wallet: Wallet) => void;
+ ecosystemId?: `ecosystem.${string}`;
+ onConnect?: (wallet: InAppWallet) => void;🤖 Prompt for AI Agents
In packages/wagmi-adapter/src/connector.ts around lines 19-20, the onConnect
callback is currently typed as (wallet: Wallet) => void but callers only ever
pass InAppWallet (or InAppWalletWithSmartAccount); change the onConnect
parameter type to the concrete in-app wallet type (e.g. InAppWallet |
InAppWalletWithSmartAccount or the shared InAppWallet type used in the
codebase), and add the necessary import(s) at the top of the file; update any
related exports/usages to reflect the new signature so downstream consumers get
the correct narrowed type.
size-limit report 📦
|

PR-Codex overview
This PR introduces an
onConnectcallback to theInAppWalletParameterstype and updates theinAppWalletConnectorfunction to support this new feature, allowing developers to execute custom logic when a wallet is connected.Detailed summary
onConnectoptional callback toInAppWalletParameters.inAppWalletConnectorto accept and handleonConnect.args.onConnect(wallet)upon successful wallet connection.Summary by CodeRabbit