-
Notifications
You must be signed in to change notification settings - Fork 277
Jon/search update #5861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Jon/search update #5861
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const startsWithMatch = | ||
| normalizeForSearch(currencyCode).startsWith(term) || | ||
| normalizeForSearch(displayName).startsWith(term) || | ||
| normalizeForSearch(assetDisplayName).startsWith(term) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard optional asset display names in wallet search
In searchWalletList the search predicate unconditionally normalizes assetDisplayName; many currency infos still omit this new field, so entering any search text for those wallets will throw at runtime (normalizeForSearch expects a string), crashing the wallet list instead of filtering. A null check or fallback is needed before normalizing optional fields.
Useful? React with 👍 / 👎.
191b22a to
6de28c5
Compare
Split search queries by spaces so users can search multiple terms like 'base eth' to find Ethereum on Base network. All terms must match at least one searchable field (AND logic). Add chainDisplayName as a searchable field in searchWalletList to enable searching by network/chain name.
Include assetDisplayName from currencyInfo as a searchable field for wallet and create-wallet searches. This allows users to search by the asset's display name (e.g., 'Ethereum') in addition to other existing searchable fields.
6de28c5 to
1a6d44b
Compare
Change search matching for currencyCode, displayName, and assetDisplayName from includes() to startsWith(). This prevents partial substring matches that crowd search results. For example, searching 'eth' now shows Ethereum assets but not Tether (which contains 'eth' in the middle). Context fields like chainDisplayName, wallet name, and contractAddress still use includes() for broader discovery.
Test coverage for: - Multi-word search with space delimiter (AND logic) - startsWith matching for currencyCode, displayName, assetDisplayName - includes matching for chainDisplayName, wallet name, contractAddress - assetDisplayName and chainDisplayName only searched for mainnet assets - Regression tests for original issues (#1: 'base eth', #3: 'eth' vs Tether)
1a6d44b to
9798ffe
Compare
| normalizeForSearch(assetDisplayName).startsWith(term) | ||
| ) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing mainnet guard for assetDisplayName in filter function
The assetDisplayName search in filterWalletCreateItemListBySearchText lacks the walletType != null guard that the adjacent pluginId search has. The comment says "for mainnet create items" but the code allows tokens with assetDisplayName to match. While tokens currently don't have assetDisplayName set, this inconsistency between the documented behavior and implementation could cause unexpected matches if token items gain this field later. The searchWalletList function correctly uses token == null for this restriction.
Overview
The wallet search supports:
Field Matching Rules
startsWithFields (Asset Identification)These fields only match if the search term appears at the beginning of the field value.
currencyCodedisplayNameassetDisplayNameRationale: Prevents partial matches like "eth" matching "Tether"
includesFields (Discovery/Context)These fields match if the search term appears anywhere in the field value.
chainDisplayNamewallet namecontractAddressRationale: Allows broader discovery by network name, wallet name, or contract address
Multi-Word Search (AND Logic)
When multiple words are entered (space-separated), ALL terms must match at least one field.
Examples
base etheth basebtc savingsbase btcMainnet vs Token Behavior
Some fields are only searched for mainnet assets (not tokens):
assetDisplayNamechainDisplayNamecurrencyCodedisplayNamewallet namecontractAddressWhy This Matters
Without this restriction, searching "eth" would match:
This would defeat the purpose of the
startsWithmatching for asset identification.Wallet Name Behavior
Wallet name search uses
includesand applies to all items (mainnet and tokens).Important Implication
If a wallet is named "My Ethereum", searching "eth" will match:
This is intentional - searching by wallet name should find everything in that wallet.
Example
Case Studies
Case 1: Finding ETH on Base Network
Goal: Find Ethereum on Base L2
Case 2: "eth" Should NOT Match Tether
Goal: Searching "eth" should find Ethereum, not Tether
Case 3: "teth" SHOULD Match Tether
Goal: Partial match at start of display name
Case 4: "steth" Should NOT Match WSTETH
Goal: Middle-of-word matches should not work for asset fields
Case 5: Finding by Contract Address
Goal: Search by partial contract address
Edge Cases
Empty/Whitespace Search
""(empty)" "(whitespace only)Multiple Spaces Between Terms
Special Characters
Contract addresses with
0xprefix work normally:No Matches
When no items match all search terms:
Summary Table
ethtethbase ethbitcoinwstethstethsavings0xdac17fCHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Adds case-insensitive, multi-word AND search with startsWith/includes rules across wallet items and create-wallet items, including mainnet-only field handling and contract address matching, plus comprehensive tests.
currencyCode/displayName/assetDisplayNameusestartsWith;chainDisplayName(mainnet only), wallet name, andcontractAddressuseincludes.assetDisplayNameandchainDisplayNameonly applied to mainnet; filters outloadingitems.assetDisplayNameto create items and applies same multi-term rules.pluginId(mainnet only) andnetworkLocation(e.g.,contractAddress) inincludesmatching.walletSearch.test.tscovering single/multi-word, case-insensitivity, mainnet vs token behavior, contract address, and regressions.assetDisplayNamein create list item.Written by Cursor Bugbot for commit 9798ffe. This will update automatically on new commits. Configure here.