[SDK] Fix: Rabby mobile connections, and a click-dead WalletConnect QR overlay - #8969
Draft
bassem-abdelazim-trilitech wants to merge 3 commits into
Draft
bassem-abdelazim-trilitech wants to merge 3 commits into
bassem-abdelazim-trilitech wants to merge 3 commits into
Conversation
The generated entry for io.rabby recorded no iOS app, no Android app and no deep link, while the WalletConnect registry publishes all three. That left Rabby with no mobile hand-off and excluded it from WCSupportedWalletIds, so it fell through to the generic external-connect path on desktop instead of the in-modal WalletConnect QR screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
createQROverlay is appended to document.body and declared no pointer-events, so it inherited `pointer-events: none` from modal libraries that disable the body while a dialog is open - leaving the overlay visible but click-dead. It now sets pointer-events explicitly. Each connect attempt also only tracked its own overlay, so abandoned attempts and expired pairings stacked overlays. Overlays are now tagged and stale ones removed before a new one is appended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5391bd1 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 |
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
@bassem-abdelazim-trilitech is attempting to deploy a commit to the thirdweb Team on Vercel. A member of the Team first needs to authorize it. |
tsconfig.base.json declares lib ["ES2022", "DOM"] without DOM.Iterable, so `for...of` over querySelectorAll fails to typecheck with TS2488. Use NodeListOf.forEach, which lib.dom declares directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Rabby's generated registry entry is missing the mobile data the WalletConnect registry publishes for it, which both blocks mobile connections and pushes desktop onto a fallback QR overlay that turns out to be unusable inside a host modal.
Reported symptoms: on mobile, choosing Rabby never opens the Rabby app; on desktop, choosing it opens a QR overlay that ignores clicks on × and Copy URI, and stacks a second overlay once the pairing expires. Happy to open a tracking issue and link it here if you'd prefer one.
Changes
io.rabbyentry with its current registry data (app.ios,app.android,mobile.native: "rabby://")io.rabbytoWCSupportedWalletIdsand sethasMobileSupport: trueinwallet-infos.ts, which follow from that datapointer-events: autoon the QR overlay root so it cannot inheritnonefrom the host pageqr-overlay.test.tscovering both overlay behavioursNotes for the reviewer
On hand-editing generated files.
io.rabby/index.tsis auto-generated, so I checked what a regeneration would produce rather than writing the values by hand. I reproducedscripts/wallets/generate.ts's transform against the live explorer API and diffed the result forio.metamaskas a control — byte-identical to what's committed — then applied the same transform's output forio.rabby. So this file matches whatpnpm build:generate-walletswould emit today.I did not run a full regeneration because it would sweep in unrelated registry churn. Comparing every committed entry against the API, exactly two have drifted in a way that matters:
io.rabbyandxyz.talisman(also missingtalisman://). I left Talisman out because itsimage_idand display name have changed too, so it needs an image regeneration rather than a data edit — happy to add it, or to just run the generator, if you'd prefer either.On the
null→""churn. The explorer API now returns empty strings where it used to returnnull, so the regenerated entry carries""for the platforms Rabby doesn't ship (mac,windows,firefox, …). That's what the generator emits today. I checked every consumer of these fields —create-wallet.ts,wallet-connect/controller.ts,AnyWalletConnectUI.tsx,GetStartedScreen.tsx,WalletConnectConnection.tsx— and all of them test truthiness (||,&&), with no=== nullor??, so""behaves exactly asnulldid.Knock-on effect in React Native.
hasMobileSupportfilters the searchable list inExternalWalletsList.tsx, so Rabby now appears there too. That follows from the corrected data rather than being a separate change, but worth flagging since it's user-visible.The overlay fixes are independent of the registry fix. Correcting Rabby's entry routes it to
WalletConnectConnection(the in-modal QR screen) via themobile.native || mobile.universalbranch inAnyWalletConnectUI.tsx, so Rabby stops reachingcreateQROverlayat all. But any wallet without a deep link still lands there, so the overlay is worth fixing on its own. They're split across two commits if you'd rather take only one.How to test
cd packages/thirdweb pnpm test:dev src/wallets/wallet-connect/qr-overlay.test.tsBoth tests fail on
mainfor the reasons they describe —expected '' to be 'auto', andto have a length of 1 but got 2— and pass with this change.For the registry half, in an app whose connect UI is inside its own modal (any library that sets
pointer-events: noneonbodywhile open), choose Rabby without the extension installed. Before: a QR overlay that ignores clicks on × and Copy URI, with a second one stacking after the pairing expires. After: the in-modal WalletConnect QR screen. On a phone, choosing Rabby now hasrabby://to hand off to.🤖 Generated with Claude Code