[SDK] Fix onDisconnect callback not being invoked in React Native #7850
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 83c9ac4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
WalkthroughAdds a changeset for a patch release documenting a React Native fix. Updates ConnectedModal in React Native to accept an optional onDisconnect callback and invokes it after the existing disconnect flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ConnectedModal
participant Wallet as WalletManager
participant SIWE as SIWEAuth
participant App as App(Consumer)
User->>ConnectedModal: Trigger Disconnect
ConnectedModal->>ConnectedModal: onClose()
ConnectedModal->>Wallet: disconnect(wallet)
alt SIWE session exists
ConnectedModal->>SIWE: doLogout()
end
opt onDisconnect provided
ConnectedModal->>App: onDisconnect({ wallet, account })
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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. |
joaquim-verges
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx (1)
49-55: onDisconnect prop missing from ConnectedModalProps type.You destructure
onDisconnectlater butConnectedModalPropsdoesn’t declare it, which will cause a TypeScript error.Add the optional callback to the prop type:
type ConnectedModalProps = ConnectButtonProps & { theme: Theme; wallet: Wallet; account: Account; onClose?: () => void; containerType: ContainerType; + onDisconnect?: (payload: { wallet: Wallet; account: Account }) => void; };
🧹 Nitpick comments (2)
.changeset/many-tips-fail.md (1)
5-5: Polish changeset copy for clarity and capitalization.Recommend clarifying the scope and capitalizing “React Native”.
Apply this diff:
-Fix onDisconnect not being invoked in react native +Fix onDisconnect callback not being invoked in React Native ConnectedModalpackages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx (1)
301-311: Ensure onDisconnect fires after disconnect/logout completes; guard against callback errors.Current flow calls
onDisconnectimmediately (and potentially before async disconnect/logout complete). Consider running the sequence asynchronously, awaiting disconnect/logout, and invokingonDisconnectin afinallyto guarantee it runs. Also guard against exceptions in user callbacks to avoid crashing the UI.Apply this diff:
- onPress={() => { - onClose?.(); - disconnect(wallet); - if (siweAuth.isLoggedIn) { - siweAuth.doLogout(); - } - onDisconnect?.({ - wallet, - account, - }); - }} + onPress={() => { + onClose?.(); + void (async () => { + try { + await disconnect(wallet); + if (siweAuth.isLoggedIn) { + await siweAuth.doLogout(); + } + } finally { + try { + onDisconnect?.({ wallet, account }); + } catch { + // Swallow user callback errors to avoid crashing the UI + } + } + })(); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/many-tips-fail.md(1 hunks)packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/native/ui/connect/ConnectedModal.tsx
**/*.{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/native/ui/connect/ConnectedModal.tsx
⏰ 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). (8)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx (2)
296-296: Destructuring onDisconnect here is correct once prop typing is updated.After adding
onDisconnecttoConnectedModalProps, this destructuring is good.
301-304: No action needed:onClosebeforeonDisconnectis deliberate and consistent
We verified that in the React NativeConnectedModaland in the webConnectedWalletDetailsmodal, the modal is closed (viaonClose/closeModal) before invoking the consumer’sonDisconnectcallback. This matches the established pattern across platforms, so the current ordering can remain as is.
size-limit report 📦
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7850 +/- ##
=======================================
Coverage 56.34% 56.34%
=======================================
Files 905 905
Lines 58834 58834
Branches 4150 4152 +2
=======================================
Hits 33151 33151
Misses 25577 25577
Partials 106 106
🚀 New features to boost your workflow:
|
joaquim-verges
deleted the
_React_Native_Fix_onDisconnect_callback_not_being_invoked
branch
Fixes #7830
PR-Codex overview
This PR addresses an issue where the
onDisconnectfunction was not being invoked in theDisconnectWalletcomponent of the React Native application. It modifies the component to include this functionality, ensuring proper disconnection handling.Detailed summary
DisconnectWalletcomponent to includeonDisconnectin props.onDisconnectwithwalletandaccountparameters when the disconnect process is triggered.Summary by CodeRabbit
New Features
Bug Fixes
Chores