[SDK] Add all connected wallets in onConnect callbacks #8403
Conversation
🦋 Changeset detectedLatest commit: e3edd1b 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.
|
WalkthroughUpdated onConnect callbacks to receive both the active wallet and the array of all connected wallets; introduced exported Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectUI as Connect Modal / Embed
participant ConnectionManager
participant WalletManager
User->>ConnectUI: trigger connect
ConnectUI->>ConnectionManager: request connect(wallet)
ConnectionManager->>WalletManager: perform connection(s)
WalletManager->>WalletManager: determine activeWallet\nand connectedWallets list
WalletManager->>ConnectionManager: return connected state
ConnectionManager->>ConnectUI: invoke onConnect(activeWallet, allConnectedWallets)
note right of ConnectUI: Consumer receives both\nactive wallet and the full list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
github-actions
bot
added
Playground
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. |
size-limit report 📦
|
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
40f0412 to
1822370
Compare
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
1822370 to
66d2806
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx (1)
72-76: Callback invoked with incorrect number of arguments.The
onConnectcallback now expects two arguments(activeWallet: Wallet, allConnectedWallets: Wallet[])perOnConnectCallback, but it's being invoked with only one argument. The second parameter will beundefined.You need to access
connectedWalletsfrom the connection manager and pass it as the second argument:+import { useConnectionManager } from "../../../../core/providers/connection-manager.js"; + export function useConnectModal() { const setRootEl = useContext(SetRootElementContext); const [isConnecting, setIsConnecting] = useState(false); + const connectionManager = useConnectionManager(); const connect = useCallback( (props: UseConnectModalOptions) => { // ... existing code ... setRootEl( <Modal {...props} + connectionManager={connectionManager} connectLocale={locale}Then in the Modal component, use it to invoke the callback:
function Modal( props: UseConnectModalOptions & { onConnect: OnConnectCallback; onClose: () => void; connectLocale: ConnectLocale; + connectionManager: ReturnType<typeof useConnectionManager>; }, ) { // ... existing code ... onConnect={(w) => { if (props.auth) return; - resolve(w); + props.onConnect(w, props.connectionManager.connectedWallets.getValue()); + resolve(w); cleanup(); }}
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/connection/types.ts (1)
104-111: Consider aligning parameter name with OnConnectCallback.The second parameter is named
otherWalletshere butallConnectedWalletsin theOnConnectCallbacktype definition. While both are valid, using the same name would improve consistency across the codebase.Optional diff for consistency:
* onConnect={(activeWallet, otherWallets) => { * console.log("auto connected to", activeWallet) - * console.log("other wallets that were also connected", otherWallets) + * console.log("all connected wallets", otherWallets) * }} * /> * ``` */ - onConnect?: (activeWallet: Wallet, otherWallets: Wallet[]) => void; + onConnect?: (activeWallet: Wallet, allConnectedWallets: Wallet[]) => void;
📜 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 (14)
.changeset/fair-mails-divide.md(1 hunks)packages/thirdweb/src/exports/react.native.ts(1 hunks)packages/thirdweb/src/exports/react.ts(1 hunks)packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts(2 hunks)packages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.ts(2 hunks)packages/thirdweb/src/react/core/hooks/connection/types.ts(1 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx(2 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsx(1 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx(3 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx(2 hunks)packages/thirdweb/src/wallets/connection/autoConnect.ts(1 hunks)packages/thirdweb/src/wallets/connection/autoConnectCore.ts(2 hunks)packages/thirdweb/src/wallets/connection/types.ts(1 hunks)packages/thirdweb/src/wallets/manager/index.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
Files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsxpackages/thirdweb/src/exports/react.tspackages/thirdweb/src/react/core/hooks/connection/types.tspackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsxpackages/thirdweb/src/wallets/connection/autoConnect.tspackages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.tspackages/thirdweb/src/exports/react.native.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/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsxpackages/thirdweb/src/exports/react.tspackages/thirdweb/src/react/core/hooks/connection/types.tspackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsxpackages/thirdweb/src/wallets/connection/autoConnect.tspackages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.tspackages/thirdweb/src/exports/react.native.ts
packages/thirdweb/src/exports/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/exports/**: Export everything viaexports/directory, grouped by feature in the SDK public API
Every public symbol must have comprehensive TSDoc with at least one@exampleblock that compiles and custom annotation tags (@beta,@internal,@experimental)
Files:
packages/thirdweb/src/exports/react.tspackages/thirdweb/src/exports/react.native.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnect.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified `Wallet` and `Account` interfaces in wallet architecture
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
📚 Learning: 2025-09-23T19:56:43.668Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 8106
File: packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx:482-485
Timestamp: 2025-09-23T19:56:43.668Z
Learning: In packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx, the EmbedContainer uses width: "100vw" intentionally rather than "100%" - this is by design for the bridge widget embedding use case.
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified `Wallet` and `Account` interfaces in wallet architecture
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/core/hooks/connection/types.ts.changeset/fair-mails-divide.mdpackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnect.ts
📚 Learning: 2025-07-18T19:20:32.530Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: .cursor/rules/dashboard.mdc:0-0
Timestamp: 2025-07-18T19:20:32.530Z
Learning: Applies to dashboard/**/*client.tsx : Interactive UI that relies on hooks (`useState`, `useEffect`, React Query, wallet hooks).
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsxpackages/thirdweb/src/react/core/hooks/connection/types.tspackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsxpackages/thirdweb/src/wallets/connection/autoConnect.tspackages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.tspackages/thirdweb/src/exports/react.native.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to apps/{dashboard,playground-web}/**/*.{ts,tsx} : Import UI primitives from `@/components/ui/*` (Button, Input, Select, Tabs, Card, Sidebar, Badge, Separator) in dashboard and playground apps
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx
📚 Learning: 2025-07-18T19:20:32.530Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: .cursor/rules/dashboard.mdc:0-0
Timestamp: 2025-07-18T19:20:32.530Z
Learning: Applies to dashboard/**/*.{tsx,jsx} : Always import from the central UI library under `@/components/ui/*` – e.g. `import { Button } from "@/components/ui/button"`.
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx
📚 Learning: 2025-07-18T19:20:32.530Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: .cursor/rules/dashboard.mdc:0-0
Timestamp: 2025-07-18T19:20:32.530Z
Learning: Applies to dashboard/**/*.{tsx,jsx} : Reuse core UI primitives; avoid re-implementing buttons, cards, modals.
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx
📚 Learning: 2025-05-30T17:14:25.332Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7227
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/modules/components/OpenEditionMetadata.tsx:26-26
Timestamp: 2025-05-30T17:14:25.332Z
Learning: The ModuleCardUIProps interface already includes a client prop of type ThirdwebClient, so when components use `Omit<ModuleCardUIProps, "children" | "updateButton">`, they inherit the client prop without needing to add it explicitly.
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to src/exports/react.native.ts : React Native specific exports are in `src/exports/react.native.ts`
Applied to files:
packages/thirdweb/src/exports/react.tspackages/thirdweb/src/exports/react.native.ts
📚 Learning: 2025-06-17T18:30:52.976Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7356
File: apps/nebula/src/app/not-found.tsx:1-1
Timestamp: 2025-06-17T18:30:52.976Z
Learning: In the thirdweb/js project, the React namespace is available for type annotations (like React.FC) without needing to explicitly import React. This is project-specific configuration that differs from typical TypeScript/React setups.
Applied to files:
packages/thirdweb/src/exports/react.tspackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/exports/react.native.ts
📚 Learning: 2025-07-18T19:20:32.530Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: .cursor/rules/dashboard.mdc:0-0
Timestamp: 2025-07-18T19:20:32.530Z
Learning: Applies to dashboard/**/*client.tsx : Anything that consumes hooks from `tanstack/react-query` or thirdweb SDKs.
Applied to files:
packages/thirdweb/src/exports/react.tspackages/thirdweb/src/wallets/manager/index.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Applied to files:
.changeset/fair-mails-divide.mdpackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnect.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Surface breaking changes prominently in PR descriptions
Applied to files:
.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
Applied to files:
.changeset/fair-mails-divide.mdpackages/thirdweb/src/wallets/manager/index.tspackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/wallets/connection/types.ts
📚 Learning: 2025-06-06T23:46:08.795Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7298
File: apps/dashboard/src/app/nebula-app/move-funds/move-funds.tsx:424-424
Timestamp: 2025-06-06T23:46:08.795Z
Learning: The thirdweb project has an ESLint rule that restricts direct usage of `defineChain`. When it's necessary to use `defineChain` directly, it's acceptable to disable the rule with `// eslint-disable-next-line no-restricted-syntax`.
Applied to files:
packages/thirdweb/src/wallets/manager/index.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/exports/** : Export everything via `exports/` directory, grouped by feature in the SDK public API
Applied to files:
packages/thirdweb/src/exports/react.native.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/react-native-adapter/** : Mobile platform shims are in `packages/react-native-adapter/`
Applied to files:
packages/thirdweb/src/exports/react.native.ts
🧬 Code graph analysis (8)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx (1)
packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.ts (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsx (1)
packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/core/hooks/connection/types.ts (2)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)
packages/thirdweb/src/wallets/manager/index.ts (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-3)
⏰ 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). (7)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
packages/thirdweb/src/exports/react.ts (1)
26-26: LGTM!The public export of
OnConnectCallbackis correctly placed and follows the established pattern.packages/thirdweb/src/wallets/connection/autoConnect.ts (1)
20-23: LGTM!Documentation example accurately reflects the new two-argument callback signature.
packages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.ts (2)
11-11: LGTM!Correct import of the shared
OnConnectCallbacktype for consistency across the codebase.
217-225: LGTM!Documentation example and type definition correctly reflect the new two-argument callback signature.
packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx (2)
19-19: LGTM!Correct import of the shared
OnConnectCallbacktype.
94-94: LGTM!Type definition correctly uses
OnConnectCallbackfor consistency.packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx (3)
31-31: LGTM!Correct import of the shared
OnConnectCallbacktype.
47-47: LGTM!Type definition correctly uses
OnConnectCallback.
96-98: LGTM!Callback is correctly invoked with both arguments: the active wallet and all connected wallets from the connection manager.
packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
3-3: Add comprehensive TSDoc with example.Per coding guidelines for
packages/thirdweb/src/exports/**, every public symbol must have comprehensive TSDoc with at least one@exampleblock. SinceOnConnectCallbackis exported publicly via the React exports barrel, it needs documentation.As per coding guidelines
Apply this diff:
+/** + * Callback invoked when a wallet successfully connects. + * + * @param activeWallet - The wallet that was just connected and set as active + * @param allConnectedWallets - Array of all currently connected wallets including the active wallet + * + * @example + * ```tsx + * import { ConnectButton } from "thirdweb/react"; + * + * function App() { + * return ( + * <ConnectButton + * client={client} + * onConnect={(activeWallet, allConnectedWallets) => { + * console.log("Active wallet:", activeWallet.id); + * console.log("Total connected:", allConnectedWallets.length); + * }} + * /> + * ); + * } + * ``` + */ export type OnConnectCallback = (activeWallet: Wallet, allConnectedWallets: Wallet[]) => void;⛔ Skipped due to learnings
Learnt from: CR Repo: thirdweb-dev/js PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-07-18T19:19:55.613Z Learning: Applies to packages/thirdweb/src/exports/** : Every public symbol must have comprehensive TSDoc with at least one `example` block that compiles and custom annotation tags (`beta`, `internal`, `experimental`)Learnt from: CR Repo: thirdweb-dev/js PR: 0 File: .cursor/rules/dashboard.mdc:0-0 Timestamp: 2025-07-18T19:20:32.530Z Learning: Applies to dashboard/**/*client.tsx : Interactive UI that relies on hooks (`useState`, `useEffect`, React Query, wallet hooks).packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx (1)
43-43: LGTM: Type updated to use centralized OnConnectCallback.The import and type update align with the new two-argument callback signature used across the connection flow.
Also applies to: 358-358
packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts (1)
28-28: LGTM: Public API updated with clear documentation.The onConnect callback signature is updated to accept both the active wallet and all connected wallets. The example code clearly demonstrates the new usage pattern.
Also applies to: 941-948
packages/thirdweb/src/wallets/manager/index.ts (1)
4-4: LGTM: Manager correctly implements new callback signature.The connection manager now invokes onConnect with both the active wallet and all connected wallets at the appropriate lifecycle points (connect and accountChanged events).
Also applies to: 33-33, 162-162, 171-171
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsx (1)
22-22: LGTM: Type updated to use OnConnectCallback.The modal's onConnect prop type is updated to align with the new callback signature.
Also applies to: 30-30
packages/thirdweb/src/wallets/connection/autoConnectCore.ts (1)
210-221: LGTM: Consolidated onConnect invocation with error handling.The onConnect callback is now invoked once after all auto-connect attempts complete, passing both the active wallet and all connected wallets. The added error handling ensures callback errors are logged without breaking the connection flow.
packages/thirdweb/src/exports/react.native.ts (1)
20-27: LGTM: Public API surface expanded with new types.The exports now include OnConnectCallback and related connection/payment types, making them available for external use. This aligns with the SDK's public API organization guidelines.
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
66d2806 to
8e7917b
Compare
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
8e7917b to
460011a
Compare
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
460011a to
1e004fa
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8403 +/- ##
=======================================
Coverage 54.82% 54.82%
=======================================
Files 919 919
Lines 60840 60843 +3
Branches 4135 4135
=======================================
+ Hits 33356 33359 +3
Misses 27383 27383
Partials 101 101
🚀 New features to boost your workflow:
|
joaquim-verges
force-pushed
the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
from
1e004fa to
e3edd1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/connection/types.ts (1)
104-111: Clarify parameter naming and semantics.The parameter name
otherWalletsis inconsistent with the canonicalOnConnectCallbacktype definition (which usesallConnectedWallets) and potentially misleading. The comment "other wallets that were also connected" suggests this array excludes the active wallet, but based on the implementation in ConnectModalContent and autoConnectCore, the second parameter receivesconnectionManager.connectedWallets.getValue()which typically includes all connected wallets.Please clarify:
- Does the second parameter include the activeWallet or only other wallets?
- Update the parameter name to match
OnConnectCallback(allConnectedWallets) for consistency- Update the JSDoc comment to accurately describe whether the array includes or excludes the activeWallet
Apply this diff if the array includes all wallets (including active):
/** - * Callback to be called on successful auto-connection of last active wallet. The callback is called with the connected wallet + * Callback to be called on successful auto-connection of last active wallet. The callback is called with the active wallet and all connected wallets. * * ```tsx * <AutoConnect - * onConnect={(activeWallet, otherWallets) => { + * onConnect={(activeWallet, allConnectedWallets) => { * console.log("auto connected to", activeWallet) - * console.log("other wallets that were also connected", otherWallets) + * console.log("all connected wallets (including active)", allConnectedWallets) * }} * /> * ``` */ - onConnect?: (activeWallet: Wallet, otherWallets: Wallet[]) => void; + onConnect?: (activeWallet: Wallet, allConnectedWallets: Wallet[]) => void;
📜 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 (17)
.changeset/fair-mails-divide.md(1 hunks)apps/playground-web/src/components/in-app-wallet/ecosystem.tsx(1 hunks)packages/thirdweb/src/exports/react.native.ts(1 hunks)packages/thirdweb/src/exports/react.ts(1 hunks)packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts(2 hunks)packages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.ts(2 hunks)packages/thirdweb/src/react/core/hooks/connection/types.ts(1 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx(2 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsx(2 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx(3 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx(2 hunks)packages/thirdweb/src/wallets/connection/autoConnect.ts(1 hunks)packages/thirdweb/src/wallets/connection/autoConnectCore.test.ts(4 hunks)packages/thirdweb/src/wallets/connection/autoConnectCore.ts(2 hunks)packages/thirdweb/src/wallets/connection/types.ts(1 hunks)packages/thirdweb/src/wallets/manager/connection-manager.test.ts(1 hunks)packages/thirdweb/src/wallets/manager/index.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/src/wallets/connection/autoConnect.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/thirdweb/src/react/web/ui/ConnectWallet/useConnectModal.tsx
- packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts
- packages/thirdweb/src/react/core/hooks/connection/ConnectEmbedProps.ts
- packages/thirdweb/src/wallets/manager/index.ts
- packages/thirdweb/src/wallets/connection/autoConnectCore.test.ts
- apps/playground-web/src/components/in-app-wallet/ecosystem.tsx
- packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModal.tsx
- packages/thirdweb/src/react/core/hooks/connection/types.ts
- packages/thirdweb/src/exports/react.native.ts
- packages/thirdweb/src/wallets/manager/connection-manager.test.ts
🧰 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
Files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/exports/react.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/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsxpackages/thirdweb/src/wallets/connection/autoConnectCore.tspackages/thirdweb/src/exports/react.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnectCore.ts
packages/thirdweb/src/exports/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/exports/**: Export everything viaexports/directory, grouped by feature in the SDK public API
Every public symbol must have comprehensive TSDoc with at least one@exampleblock that compiles and custom annotation tags (@beta,@internal,@experimental)
Files:
packages/thirdweb/src/exports/react.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
📚 Learning: 2025-09-23T19:56:43.668Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 8106
File: packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx:482-485
Timestamp: 2025-09-23T19:56:43.668Z
Learning: In packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx, the EmbedContainer uses width: "100vw" intentionally rather than "100%" - this is by design for the bridge widget embedding use case.
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx
📚 Learning: 2025-07-18T19:20:32.530Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: .cursor/rules/dashboard.mdc:0-0
Timestamp: 2025-07-18T19:20:32.530Z
Learning: Applies to dashboard/**/*client.tsx : Interactive UI that relies on hooks (`useState`, `useEffect`, React Query, wallet hooks).
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsxpackages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified `Wallet` and `Account` interfaces in wallet architecture
Applied to files:
packages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnectCore.ts.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Applied to files:
packages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnectCore.ts.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
Applied to files:
packages/thirdweb/src/wallets/connection/types.tspackages/thirdweb/src/wallets/connection/autoConnectCore.ts.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
Applied to files:
packages/thirdweb/src/wallets/connection/types.ts.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Surface breaking changes prominently in PR descriptions
Applied to files:
.changeset/fair-mails-divide.md
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to src/exports/react.native.ts : React Native specific exports are in `src/exports/react.native.ts`
Applied to files:
packages/thirdweb/src/exports/react.ts
📚 Learning: 2025-06-17T18:30:52.976Z
Learnt from: MananTank
Repo: thirdweb-dev/js PR: 7356
File: apps/nebula/src/app/not-found.tsx:1-1
Timestamp: 2025-06-17T18:30:52.976Z
Learning: In the thirdweb/js project, the React namespace is available for type annotations (like React.FC) without needing to explicitly import React. This is project-specific configuration that differs from typical TypeScript/React setups.
Applied to files:
packages/thirdweb/src/exports/react.ts
🧬 Code graph analysis (2)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx (1)
packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-6)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx (3)
packages/thirdweb/src/exports/react.native.ts (1)
OnConnectCallback(27-27)packages/thirdweb/src/exports/react.ts (1)
OnConnectCallback(26-26)packages/thirdweb/src/react/core/hooks/connection/types.ts (1)
OnConnectCallback(3-6)
⏰ 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). (3)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
.changeset/fair-mails-divide.md (1)
2-2: Breaking change already flagged in previous review.A past review comment correctly identified that this callback signature change is breaking and should be marked as
major, notminor. Refer to the existing comment for details and the suggested fix.packages/thirdweb/src/exports/react.ts (1)
26-26: LGTM!The OnConnectCallback type export is correctly structured and properly placed in the public API surface.
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectEmbed.tsx (2)
19-19: LGTM!The OnConnectCallback type import is correctly added and used to update the prop type.
358-358: LGTM!The onConnect prop type is correctly updated to use OnConnectCallback, maintaining consistency with the new callback signature across the codebase.
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx (1)
96-98: Verify callback parameter semantics and timing.The invocation passes
wallet(the newly connected wallet) as the first argument, but theOnConnectCallbacktype names this parameteractiveWallet. This creates ambiguity whenshouldSetActiveis false (line 92-94): the newly connected wallet is NOT the active wallet, yet it's passed as the "activeWallet" parameter.Additionally, verify that
connectionManager.connectedWallets.getValue()returns the updated list immediately aftersetActiveWalletoraddConnectedWalletis called. If these operations are asynchronous or defer state updates, the callback might receive stale data.Consider these options:
If the first parameter should always be the active wallet (not just the connecting wallet), fetch it explicitly:
const currentActiveWallet = connectionManager.activeWalletStore.getValue(); if (currentActiveWallet) { props.onConnect(currentActiveWallet, connectionManager.connectedWallets.getValue()); }If the first parameter represents the wallet that triggered the connection (regardless of whether it's active), update the parameter name in
OnConnectCallbackfromactiveWallettoconnectedWalletfor clarity.Document the expected behavior: does
onConnectonly fire when a wallet becomes active, or does it fire for any connected wallet?packages/thirdweb/src/wallets/connection/autoConnectCore.ts (2)
209-222: Well-structured callback invocation.The finalization logic correctly retrieves the active wallet and all connected wallets from the manager stores after all connection attempts complete. This ensures the callback receives accurate state rather than assuming connection results. The try-catch around
onConnectprevents callback errors from disrupting the connection flow.Note: This implementation is more robust than the approach in ConnectModalContent.tsx (line 96-98), which passes the just-connected wallet rather than explicitly fetching the active wallet from the store. Consider aligning ConnectModalContent to use a similar pattern for consistency.
155-160: LGTM!The connection logic correctly awaits the result and handles errors appropriately. Removing the direct wallet assignment in favor of retrieving the final state from manager stores (lines 210-211) is a cleaner approach.
joaquim-verges
deleted the
_SDK_Add_all_connected_wallets_in_onConnect_callbacks
branch
PR-Codex overview
This PR focuses on enhancing the wallet connection functionality by modifying the
onConnectcallbacks to include all connected wallets, improving the user experience during wallet connections.Detailed summary
OnConnectCallbacktype to acceptactiveWalletandallConnectedWallets.onConnectimplementations across various components to log both active and connected wallets.onConnectcalls.autoConnectlogic to handle multiple connected wallets.Summary by CodeRabbit
New Features
Breaking Changes
Behavioral Fixes
Tests