[SDK] Fix Coinbase wallet transaction desktop popup for mobile QR login #8253
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: fb875c6 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 |
joaquim-verges
changed the title
WalkthroughImplements conditional Coinbase popup display based on signer type from localStorage during transaction requests, forwards 402 response error details through payment requirement selection to improve error messaging, and adds a changeset noting a patch release for the CB wallet desktop popup fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DApp
participant ThirdwebSDK as SDK (Coinbase)
participant LocalStorage as localStorage
participant Coinbase as Coinbase Provider
User->>DApp: Initiate transaction
DApp->>ThirdwebSDK: onTransactionRequested()
ThirdwebSDK->>LocalStorage: Read "-CBWSDK:SignerConfigurator:SignerType"
alt signerType == "scw"
ThirdwebSDK->>Coinbase: showCoinbasePopup(provider)
Note right of Coinbase: Popup only for "scw"
else signerType != "scw" or unavailable
Note over ThirdwebSDK: Skip popup
end
ThirdwebSDK->>Coinbase: Preprocess & send transaction
Coinbase-->>ThirdwebSDK: Result
ThirdwebSDK-->>DApp: Resolve/Reject
sequenceDiagram
autonumber
participant Client as Client Code
participant Wrapper as wrapFetchWithPayment
participant Server as API Server
Client->>Wrapper: fetch(request)
Wrapper->>Server: send request
Server-->>Wrapper: 402 Payment Required + error text
Wrapper->>Wrapper: selectPaymentRequirements(..., error)
alt No valid requirements
Wrapper-->>Client: Error("No valid payment requirements... " + error)
Note over Wrapper: Error text propagated from 402
else Valid requirements
Wrapper->>Wrapper: validate maxAmount / maybe switch chain
Wrapper->>Server: retry with payment header
Server-->>Wrapper: response
Wrapper-->>Client: response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
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
force-pushed
the
Fix_Coinbase_wallet_transaction_desktop_popup_for_mobile_QR_login
branch
from
26692e0 to
fb875c6
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
🧹 Nitpick comments (1)
packages/thirdweb/src/x402/fetchWithPayment.ts (1)
141-150: Handle undefined error parameter gracefully.The error messages on lines 90 and 149 concatenate the
errorparameter directly, which will append the string "undefined" when the error parameter is not provided. This results in less professional error messages.Apply this diff to handle undefined gracefully:
function defaultPaymentRequirementsSelector( paymentRequirements: RequestedPaymentRequirements[], chainId: number, scheme: "exact", error?: string, ) { if (!paymentRequirements.length) { throw new Error( - `No valid payment requirements found in server 402 response. ${error}`, + `No valid payment requirements found in server 402 response.${error ? ` ${error}` : ""}`, ); }Also apply a similar fix on line 90:
if (!selectedPaymentRequirements) { throw new Error( - `No suitable payment requirements found for chain ${chain.id}. ${error}`, + `No suitable payment requirements found for chain ${chain.id}.${error ? ` ${error}` : ""}`, ); }
📜 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 (3)
.changeset/sixty-bobcats-add.md(1 hunks)packages/thirdweb/src/wallets/coinbase/coinbase-web.ts(1 hunks)packages/thirdweb/src/x402/fetchWithPayment.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/thirdweb/src/x402/fetchWithPayment.tspackages/thirdweb/src/wallets/coinbase/coinbase-web.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/x402/fetchWithPayment.tspackages/thirdweb/src/wallets/coinbase/coinbase-web.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/x402/fetchWithPayment.tspackages/thirdweb/src/wallets/coinbase/coinbase-web.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/sixty-bobcats-add.md
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/coinbase/coinbase-web.ts
⏰ 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: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
.changeset/sixty-bobcats-add.md (1)
1-5: LGTM!The changeset follows the correct format with an appropriate patch version bump for this bug fix, and the description clearly explains the change.
packages/thirdweb/src/x402/fetchWithPayment.ts (1)
85-85: LGTM!Forwarding the server-provided error to the selector improves error messaging for debugging payment requirement issues.
| if (window.localStorage) { | ||
| // this is the local storage key for the signer type in the cb web sdk | ||
| // value can be "scw" (web) or "walletlink" (mobile wallet) | ||
| const signerType = window.localStorage.getItem( | ||
| "-CBWSDK:SignerConfigurator:SignerType", | ||
| ); | ||
| if (signerType === "scw") { | ||
| await showCoinbasePopup(provider); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the stability of the internal localStorage key.
The code relies on the hardcoded localStorage key "-CBWSDK:SignerConfigurator:SignerType", which appears to be an internal implementation detail of the Coinbase Web SDK. If Coinbase changes this key in a future SDK version, the conditional popup logic will silently fail.
Consider:
- Wrapping the localStorage access in a try-catch to handle potential exceptions (e.g., in private browsing mode)
- Defining the key and valid values as constants for better maintainability
Run the following script to check if there's an official SDK API or documentation for determining signer type:
Additionally, search the web for official Coinbase Web SDK documentation:
🏁 Script executed:
#!/bin/bash
# Search for any official API or types related to signer type in the codebase
rg -nP --type=ts -C3 'SignerType|signerType|signer.*type' --glob '!node_modules/**'Length of output: 27989
🌐 Web query:
Coinbase Web SDK SignerConfigurator SignerType official API documentation
💡 Result:
I searched Coinbase's official developer docs and repositories but could not find any official API documentation referencing "SignerConfigurator" or "SignerType" in a Coinbase Web SDK.
Sources I checked:
- Coinbase developer docs (CDP / Wallet SDK pages). [1]
- Coinbase Node/JS SDK documentation (Coinbase SDK GitHub/pages). [2]
If you have a link, code snippet, or the package name (npm/GitHub) where you saw those identifiers, share it and I’ll search that source and return the exact official docs or types.
References:
[1] Coinbase Developer Documentation (wallet / CDP docs).
[2] @coinbase/coinbase-sdk Node.js docs (Coinbase GitHub pages).
Extract the localStorage key and values into constants and wrap access in try-catch
The hardcoded key "-CBWSDK:SignerConfigurator:SignerType" is an internal SDK detail and may change. Define constants for the key and its valid values (“scw”, “walletlink”), and wrap window.localStorage.getItem in a try-catch to handle errors (e.g., private-browsing) and avoid silent failures.
🤖 Prompt for AI Agents
In packages/thirdweb/src/wallets/coinbase/coinbase-web.ts around lines 192 to
200, extract the hardcoded localStorage key
"-CBWSDK:SignerConfigurator:SignerType" and the valid values "scw" and
"walletlink" into top-level constants, then replace the inline string uses with
those constants; wrap the window.localStorage.getItem call in a try-catch (and
guard for window.localStorage presence) so any errors (e.g. private browsing)
are caught and handled by falling back to a safe default (treat as unknown / do
not call showCoinbasePopup), and ensure the signerType comparison uses the
defined constant for "scw".
size-limit report 📦
|
joaquim-verges
deleted the
Fix_Coinbase_wallet_transaction_desktop_popup_for_mobile_QR_login
branch
1 participant
PR-Codex overview
This PR focuses on improving error handling in the
defaultPaymentRequirementsSelectorfunction and ensuring the Coinbase popup displays correctly based on the wallet type.Detailed summary
defaultPaymentRequirementsSelectorto include an optionalerrorstring.scw(web) incoinbase-web.ts.Summary by CodeRabbit
Bug Fixes
Improvements
Chores