[SDK] Fix hiddenWallets prop not applying to all wallets screen #8354
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 326255c 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
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. |
WalkthroughPropagates hidden-wallet IDs through the connect UI and applies filtering so specified wallets are excluded from "All wallets" on web and native; also adds a changelog entry and skips several indexer-related event tests (marked TODO). Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ConnectButton UI
participant Modal as ConnectModalContent
participant All as AllWalletsUI
participant Native as ConnectModal (native)
rect rgba(0,128,96,0.08)
UI->>Modal: open modal (includes hiddenWallets)
Note right of Modal: showAll screen forwards\nwalletIdsToHide to AllWalletsUI
Modal->>All: render(allWallets, walletIdsToHide)
All->>All: filter wallets\nexclude NON_SEARCHABLE & walletIdsToHide
All-->>Modal: render filtered list
end
rect rgba(0,64,128,0.06)
UI->>Native: open native modal (includes hiddenWallets)
Native->>Native: filter external wallets\nexclude in-app + hiddenWallets
Native-->>UI: render filtered list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
size-limit report 📦
|
joaquim-verges
force-pushed
the
_thirdweb_Fix_hiddenWallets_prop_not_applying_to_all_wallets_screen
branch
from
df82a14 to
326255c
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: 0
🧹 Nitpick comments (2)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsx (2)
30-39: Add explicit return type per coding guidelines.The function declaration should include an explicit return type.
As per coding guidelines
Apply this diff:
-function AllWalletsUI(props: { +function AllWalletsUI(props: { onBack: () => void; onSelect: (wallet: Wallet) => void; size: "compact" | "wide"; client: ThirdwebClient; recommendedWallets: Wallet[] | undefined; connectLocale: ConnectLocale; disableSelectionDataReset?: boolean; walletIdsToHide?: WalletId[]; -}) { +}): JSX.Element {
43-47: Filtering logic is correct and handles the fix properly.The implementation correctly excludes wallets specified in
walletIdsToHidewhile maintaining the existingNON_SEARCHABLE_WALLETSfilter. The optional chaining and dependency array are correct.For better performance if
walletIdsToHidecould contain many items, consider using a Set for O(1) lookup:const walletList = useMemo(() => { + const hideSet = props.walletIdsToHide ? new Set(props.walletIdsToHide) : null; - return walletInfos - .filter((info) => !NON_SEARCHABLE_WALLETS.includes(info.id)) - .filter((info) => !props.walletIdsToHide?.includes(info.id)); + return walletInfos.filter( + (info) => + !NON_SEARCHABLE_WALLETS.includes(info.id) && + (!hideSet || !hideSet.has(info.id)) + ); }, [props.walletIdsToHide]);However, for typical use cases (hiding a handful of wallets), the current implementation is perfectly adequate.
📜 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 (5)
.changeset/dark-ravens-end.md(1 hunks)packages/thirdweb/src/event/actions/get-events.test.ts(3 hunks)packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx(1 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsx(2 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx
- packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/ConnectModalContent.tsx
- .changeset/dark-ravens-end.md
🧰 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/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsxpackages/thirdweb/src/event/actions/get-events.test.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/AllWalletsUI.tsxpackages/thirdweb/src/event/actions/get-events.test.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/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsxpackages/thirdweb/src/event/actions/get-events.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
**/*.test.{ts,tsx}: Co‑locate tests asfoo.test.ts(x)next to the implementation
Use real function invocations with stub data; avoid brittle mocks
Use MSW to intercept HTTP calls for network interactions; mock only hard‑to‑reproduce scenarios
Keep tests deterministic and side‑effect free; use Vitest
Files:
packages/thirdweb/src/event/actions/get-events.test.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: joaquim-verges
Repo: thirdweb-dev/js PR: 7268
File: packages/thirdweb/src/wallets/in-app/core/wallet/in-app-core.ts:210-216
Timestamp: 2025-06-03T23:44:40.243Z
Learning: EIP7702 wallets do not need special handling for switching chains, unlike EIP4337 wallets which require reconnection when switching chains. In the switchChain method condition, EIP7702 should be intentionally excluded from the reconnection logic.
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/** : Smart wallets with account abstraction
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-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/AllWalletsUI.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/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.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/AllWalletsUI.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/** : Support for in-app wallets (social/email login)
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.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/** : Smart wallets with account abstraction
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsx
📚 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/AllWalletsUI.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 test/src/test-wallets.ts : Predefined test accounts are in `test/src/test-wallets.ts`
Applied to files:
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.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 **/*.test.{ts,tsx} : Keep tests deterministic and side-effect free
Applied to files:
packages/thirdweb/src/event/actions/get-events.test.ts
📚 Learning: 2025-08-29T15:37:38.513Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T15:37:38.513Z
Learning: Applies to **/*.test.{ts,tsx} : Keep tests deterministic and side‑effect free; use Vitest
Applied to files:
packages/thirdweb/src/event/actions/get-events.test.ts
🧬 Code graph analysis (1)
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsx (1)
packages/thirdweb/src/wallets/constants.ts (1)
NON_SEARCHABLE_WALLETS(10-15)
⏰ 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: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/thirdweb/src/event/actions/get-events.test.ts (1)
174-244: Cannot fully verify scope mismatch claim without PR metadata.The review comment makes specific claims about the PR's objective (fixing
hiddenWalletsprop), but I only have access to the code snippet showing the skipped tests. To verify whether these changes are truly unrelated to the stated PR objective, I would need:
- PR description and title — to confirm the actual scope
- Git history — to determine when/why these tests were skipped and by whom
- Related issues/tickets — to confirm if the indexer problem is tracked separately
- Commit message context — to understand the intent behind these changes
The review comment's concerns about test skipping and lack of issue tracking are valid if these tests were intentionally disabled for this PR. However, without access to PR metadata and commit history, I cannot confirm whether:
- These changes belong in this PR or were accidentally included
- The indexer tests were already skipped before this PR
- There's a related issue already tracking the indexer problem
packages/thirdweb/src/react/web/ui/ConnectWallet/Modal/AllWalletsUI.tsx (1)
11-11: LGTM! Type import and prop addition are correct.The
WalletIdtype import and the new optionalwalletIdsToHideprop are properly typed and support the feature requirement.Also applies to: 38-38
joaquim-verges
deleted the
_thirdweb_Fix_hiddenWallets_prop_not_applying_to_all_wallets_screen
branch
Fixes INC-149
Summary by CodeRabbit
Bug Fixes
Chores
Tests
PR-Codex overview
This PR focuses on improving the handling of hidden wallets in the wallet connection modals and updating test cases related to event fetching.
Detailed summary
walletIdsToHideprop toConnectModalContent.externalWalletsfiltering to include hidden wallets inConnectModal.AllWalletsUIto filter out hidden wallets based onwalletIdsToHide.