feat: new token selector #825
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis set of changes introduces a substantial refactor and redesign of the token and network selection UI, particularly the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TokenSelector
participant BottomDrawer
participant NetworkListView
participant TokenListItem
participant TokenContext
participant Wallet
User->>TokenSelector: Clicks token selector button
TokenSelector->>BottomDrawer: Opens drawer UI
BottomDrawer->>User: Shows sections (networks, tokens, search)
User->>BottomDrawer: Selects "Change Network"
BottomDrawer->>NetworkListView: Shows network list
User->>NetworkListView: Selects network
NetworkListView->>TokenContext: Updates selected network
NetworkListView->>BottomDrawer: Returns to token list
User->>BottomDrawer: Searches token
BottomDrawer->>TokenListItem: Filters and displays tokens
User->>TokenListItem: Selects token
TokenListItem->>TokenContext: Updates selected token
TokenListItem->>BottomDrawer: Closes drawer
TokenContext->>Wallet: (Optional) Fetches wallet balances
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
src/app/(mobile-ui)/home/page.tsx
Outdated
| </ActionButtonGroup> | ||
| </div> | ||
|
|
||
| <NewTokenSelector /> |
There was a problem hiding this comment.
temporary for testing purpose, to be removed @kushagrasarathe
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/context/tokenSelector.context.tsx (1)
225-233: 🛠️ Refactor suggestionTwo setters doing different things causes API confusion
tokenSelectorContextnow exposes bothsetSelectedChainID(raw state setter) andupdateSelectedChainID(helper that also clears the token address). Consumers must remember which one to call in which scenario, which is error-prone.Consider:
- Export only one function (
setSelectedChainID) whose signature accepts an options object, or- Rename helpers clearly, e.g.
resetTokenAndSetChainID.Doing so prevents accidental misuse and keeps the public API small.
🧹 Nitpick comments (7)
src/app/(mobile-ui)/home/page.tsx (1)
98-99: Good integration of the NewTokenSelector component.The new token selector is appropriately positioned between the action buttons and rewards card in the UI hierarchy. Consider adding a comment explaining its purpose and behavior since it's being used without props.
- <NewTokenSelector /> + {/* Token and network selector for wallet-connected environments */} + <NewTokenSelector />src/components/Global/EmptyStates/EmptyState.tsx (2)
5-10: ExposedescriptionasReact.ReactNodefor greater flexibility
descriptionis currently typed asstring, while the neighbouringtitleandctaprops acceptReactNode. Aligningdescriptionwith the other props would allow callers to embed links, bold text, or line-breaks without string interpolation.- description?: string + description?: React.ReactNode
16-23: Minor accessibility & semantics touch-up
- The icon is purely decorative, so screen-readers will announce an empty “image” unless explicitly hidden.
- Wrapping the CTA in its own container allows you to apply consistent spacing without influencing the
title/descriptionblock.- <div className="rounded-full bg-primary-1 p-2"> - <Icon name={icon} size={16} /> + <div className="rounded-full bg-primary-1 p-2"> + <Icon name={icon} size={16} aria-hidden /> </div> - <div className="text-center"> + <div className="flex flex-col items-center text-center"> <div className="font-medium">{title}</div> {description && <div className="text-sm text-grey-1">{description}</div>} - {cta && cta} + </div> + {cta && <div>{cta}</div>}src/context/tokenSelector.context.tsx (2)
98-102: MemoiseupdateSelectedChainIDto avoid re-rendersBecause
updateSelectedChainIDis recreated on every render, any component that uses it from context will re-render even when nothing else changed. Wrapping the function inuseCallback(or hoisting it intouseMemowith the state-setters in its deps) keeps the reference stable.- const updateSelectedChainID = (chainID: string) => { + const updateSelectedChainID = React.useCallback((chainID: string) => { setSelectedTokenAddress('0x0000000000000000000000000000000000000000') setSelectedChainID(chainID) - } + }, [])
121-215: Effect re-fires on every change ofsupportedSquidChainsAndTokensobject
getSquidChainsAndTokens().then(setSupportedSquidChainsAndTokens)creates a new object instance on every successful fetch, which will retrigger thisuseEffect➜fetchAndSetTokenPricecycle even when data has not logically changed.If the fetch runs periodically, wrap the setter in a functional update that freezes / deep-compares, or memoise
supportedSquidChainsAndTokenswithuseMemobefore setting state.This saves an unnecessary price fetch & state churn.
src/components/Global/TokenSelector/NewTokenSelector.tsx (2)
404-409: Duplicated fallback makes intent unclearreturn network?.axelarChainName || network?.axelarChainName || `Chain ${selectedChainID}`The same property is checked twice; the second branch will never be reached. Replace the second occurrence with
network?.chainName(or remove altogether) for clarity.- return network?.axelarChainName || network?.axelarChainName || `Chain ${selectedChainID}` + return network?.axelarChainName || network?.chainName || `Chain ${selectedChainID}`
488-500:popularChainIdscan live outside the componentThe constant list is recreated on every render despite never changing. Moving it to module scope (or using
useMemowithout deps) saves allocations and keeps the render function lean.- const popularChainIds = useMemo( - () => [ +const POPULAR_CHAIN_IDS = [ { chainId: '42161', name: 'ARB' }, { chainId: '1', name: 'ETH' }, { chainId: '10', name: 'OP' }, { chainId: '8453', name: 'BASE'}, - ], - [] - ) +]Then replace
popularChainIdswithPOPULAR_CHAIN_IDSfurther below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/(mobile-ui)/home/page.tsx(2 hunks)src/components/0_Bruddle/Divider.tsx(1 hunks)src/components/Global/EmptyStates/EmptyState.tsx(1 hunks)src/components/Global/TokenSelector/NewTokenSelector.tsx(1 hunks)src/components/SearchUsers/SearchResults.tsx(2 hunks)src/context/tokenSelector.context.tsx(3 hunks)tailwind.config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/context/tokenSelector.context.tsx (2)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#631
File: src/components/Create/Create.tsx:108-112
Timestamp: 2025-01-16T13:14:40.363Z
Learning: In the Peanut UI codebase, the `resetTokenContextProvider` function from `tokenSelectorContext` is a stable function reference that doesn't change, so it doesn't need to be included in useEffect dependencies.
🧬 Code Graph Analysis (1)
src/components/SearchUsers/SearchResults.tsx (1)
src/components/Global/EmptyStates/EmptyState.tsx (1)
EmptyState(12-27)
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/NewTokenSelector.tsx
[error] 336-336: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 337-337: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (7)
tailwind.config.js (1)
441-444: Great addition of the.shadow-2utility class!This new shadow class with a smaller offset (0.125rem) complements the existing shadow classes and provides a more subtle option for UI elements. The implementation correctly follows the established pattern of having both light and dark mode variants.
src/app/(mobile-ui)/home/page.tsx (1)
11-11: Clean import of the new token selector component.The import is correctly placed with other component imports.
src/components/SearchUsers/SearchResults.tsx (2)
4-4: Good import of the reusable EmptyState component.This is a clean addition that will help with using the new component.
104-109: Excellent refactoring to use the EmptyState component.Replacing the manually constructed empty state UI with the reusable EmptyState component improves code maintainability and UI consistency. The props passed to the component match its expected interface.
src/components/0_Bruddle/Divider.tsx (3)
5-5: Good addition of the dividerClassname prop.This optional prop enhances the component's flexibility by allowing custom styling specifically for the divider lines.
8-8: Properly updated function parameter list.The parameter list now includes the new dividerClassname prop.
11-11: Correct implementation of the dividerClassname prop.The dividerClassname is properly applied to both divider lines using twMerge to combine with the default styles.
Also applies to: 13-13
| </div> | ||
| ) : ( | ||
| <> | ||
| <span>Select Token</span> | ||
| <Icon | ||
| name="chevron-up" | ||
| className={`h-4 w-4 transition-transform ${!isDrawerOpen ? 'rotate-180' : ''}`} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
Optional chaining prevents a potential crash
supportedSquidChainsAndTokens[selectedChainID] may be undefined until networks are fetched. Accessing .axelarChainName directly risks Cannot read property errors during the first render.
- const network = supportedSquidChainsAndTokens[selectedChainID]
+ const network = supportedSquidChainsAndTokens?.[selectedChainID]This also resolves the Biome useOptionalChain lint finding.
- added supported chains based on discord thread discussion - added coming soon chains - abstracted the new token selector component into separate files for better code readibility
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/components/Global/TokenSelector/NewTokenSelector.tsx (2)
65-69:⚠️ Potential issueFix memory leak in closeDrawer function
The
closeDrawerfunction returns a cleanup function for the timeout, but this cleanup is never used since the function is called directly and not as an effect. This creates a small memory leak.- const closeDrawer = useCallback(() => { - setIsDrawerOpen(false) - const timer = setTimeout(() => setSearchValue(''), 200) - return () => clearTimeout(timer) - }, []) + const closeDrawer = useCallback(() => { + setIsDrawerOpen(false) + setTimeout(() => setSearchValue(''), 200) + }, [])
165-165:⚠️ Potential issueAdd optional chaining to prevent potential null reference errors
The
supportedSquidChainsAndTokens[selectedChainID]access could be undefined if the network data isn't loaded yet or the selectedChainID doesn't exist in the data.- const network = supportedSquidChainsAndTokens[selectedChainID] + const network = supportedSquidChainsAndTokens?.[selectedChainID]
🧹 Nitpick comments (12)
src/components/Global/TokenSelector/NetworkListView.tsx (2)
1-30: Props interface is well-defined but could use additional documentationThe component props are comprehensive and typed appropriately.
Consider adding JSDoc comments to explain the purpose and requirements of each prop, especially for complex props like
chainsandallowedChainIds.
84-104: Consider performance optimization for large listsThe component renders all filtered chains without virtualization, which could cause performance issues with large network lists.
Consider implementing a virtualized list (e.g., using
react-windoworreact-virtualized) if you expect to support a large number of networks, which would significantly improve rendering performance.src/components/Global/TokenSelector/NetworkButton.tsx (1)
46-48: Improve name truncation logic for better readabilityThe current truncation logic might cut network names in awkward places. Consider a more readable approach.
<span className="text-sm font-medium"> - {isSearch ? 'more' : chainName.length > 6 ? chainName.substring(0, 6) : chainName} + {isSearch ? 'more' : chainName.length > 6 ? `${chainName.substring(0, 5)}…` : chainName} </span>src/components/Global/TokenSelector/NetworkListItem.tsx (3)
28-29: Consider improving the selection state logic for coming-soon networksThe current implementation only applies the selection highlight when
isSelected && !isComingSoon, but then handles click prevention separately. This can be confusing as a "coming soon" network appears differently than other selected networks.Consider consistent styling for selection states regardless of availability, and only disable the click handler:
- <div - key={chainId} - className={twMerge('cursor-pointer rounded-sm shadow-sm', isSelected && !isComingSoon && 'bg-primary-3')} - onClick={isComingSoon ? undefined : onClick} - > + <div + key={chainId} + className={twMerge( + 'rounded-sm shadow-sm', + isSelected && 'bg-primary-3', + isComingSoon ? 'cursor-not-allowed' : 'cursor-pointer' + )} + onClick={isComingSoon ? undefined : onClick} + >
27-27: Remove redundant key propThe
keyprop is used to help React identify items in lists, but this component doesn't directly render in an array context.- <div - key={chainId} - className={twMerge('cursor-pointer rounded-sm shadow-sm', isSelected && !isComingSoon && 'bg-primary-3')} - onClick={isComingSoon ? undefined : onClick} - > + <div + className={twMerge('cursor-pointer rounded-sm shadow-sm', isSelected && !isComingSoon && 'bg-primary-3')} + onClick={isComingSoon ? undefined : onClick} + >
52-54: Improve the network name fallback displayUsing the first two letters of a network name can be ambiguous for networks with similar names (e.g., "Arbitrum" and "Avalanche" would both show "AR").
Consider a more distinctive fallback that handles similar network names better:
- <div className="flex h-8 w-8 items-center justify-center rounded-full bg-gray-300 text-black"> - {name?.substring(0, 2)?.toUpperCase() || 'CH'} - </div> + <div className="flex h-8 w-8 items-center justify-center rounded-full bg-gray-300 text-black"> + {name?.substring(0, 1)?.toUpperCase() || 'C'} + </div>Or consider using a generic network icon for all networks without an icon.
src/components/Global/TokenSelector/TokenListItem.tsx (2)
33-38: Enhance decimal value handling for token balancesThe current implementation has a potential issue with
Math.minifbalance.decimalsis not a valid number. Additionally, thedisplayDecimalscalculation could be simplified.- const formattedBalance = useMemo(() => { - if (!balance.amount || !balance.decimals) return null - const displayDecimals = Math.min(balance.decimals ?? 6, 6) - const formatted = balance.amount.toFixed(displayDecimals) - return formatAmount(formatted) - }, [balance.amount, balance.decimals]) + const formattedBalance = useMemo(() => { + if (!balance.amount || typeof balance.decimals !== 'number') return null + const displayDecimals = Math.min(balance.decimals, 6) + const formatted = balance.amount.toFixed(displayDecimals) + return formatAmount(formatted) + }, [balance.amount, balance.decimals])
55-67: Protect against memory leaks in image error handlingThe
onErrorhandler sets a state value, but this could lead to a memory leak if the component unmounts while the image is still loading and then encounters an error.Consider using a ref to track component mounted state:
import React, { useContext, useMemo, useState, useRef, useEffect } from 'react' // In the component: + const isMounted = useRef(true); + + useEffect(() => { + return () => { + isMounted.current = false; + }; + }, []); // In the Image component: - onError={() => setTokenPlaceholder(true)} + onError={() => { + if (isMounted.current) { + setTokenPlaceholder(true); + } + }}src/components/Global/TokenSelector/NewTokenSelector.tsx (4)
216-216: Remove console.log statement from production codeThis console.log statement appears to be used for debugging and should be removed in production code.
- } else { - console.log('Render: No selected token/chain in context for button display.') - } + }
309-317: Consider dynamic drawer height based on contentThe drawer heights (expanded, half, collapsed) are hardcoded, which might not adapt well to different screen sizes or content amounts. This could lead to UI issues on devices with smaller screens.
Consider implementing a more responsive approach to drawer height:
- Use viewport units with min/max constraints
- Calculate height based on content and screen size
- Add overflow handling for varying content amounts
For example:
- expandedHeight={95} - halfHeight={60} - collapsedHeight={10} + expandedHeight="calc(min(95vh, 800px))" + halfHeight="calc(min(60vh, 500px))" + collapsedHeight={10}Or consider implementing a more dynamic solution using refs to measure content height.
45-45: Improve type safety foronNetworkSelectpropThe
onNetworkSelectprop is typed as(network: any) => void, which lacks type safety. This could lead to runtime errors if the wrong type of data is passed.Define a proper interface for the network object or use existing types from your codebase:
+ import { interfaces } from '@/interfaces' interface NewTokenSelectorProps { classNameButton?: string onTokenSelect?: (token: IUserBalance) => void - onNetworkSelect?: (network: any) => void + onNetworkSelect?: (network: interfaces.ISquidChain) => void }
71-103: Optimize token filtering logic for better performanceThe
displayTokensuseMemo calculation performs multiple operations that could be optimized for better performance, especially with large token lists.
- Use a single filter operation instead of chaining them
- Precompute lowercase search terms once outside the filter loop
- Early return empty array if no balances or wallet connection
const displayTokens = useMemo(() => { if (!isConnected) { return [] } if (!selectedWallet?.balances) { return [] } const lowerSearchValue = searchValue.toLowerCase() - let filteredByChain = selectedWallet.balances - if (selectedChainID) { - filteredByChain = selectedWallet.balances.filter((balance) => { - const balanceChainId = String(balance.chainId) - const isMatch = balanceChainId === selectedChainID - - return isMatch - }) - } - - const filteredBalances = filteredByChain.filter((balance) => { - const hasSymbol = !!balance.symbol - const symbolMatch = hasSymbol && balance.symbol.toLowerCase().includes(lowerSearchValue) - const nameMatch = balance?.name && balance.name.toLowerCase().includes(lowerSearchValue) - const addressMatch = balance?.address && balance.address.toLowerCase().includes(lowerSearchValue) - - const shouldInclude = hasSymbol && (symbolMatch || nameMatch || addressMatch) - - return shouldInclude - }) - - return filteredBalances + return selectedWallet.balances.filter(balance => { + // Filter by chain if needed + if (selectedChainID && String(balance.chainId) !== selectedChainID) { + return false; + } + + // Must have a symbol + if (!balance.symbol) { + return false; + } + + // If no search, include all tokens matching the chain filter + if (!lowerSearchValue) { + return true; + } + + // Search filters + const symbolMatch = balance.symbol.toLowerCase().includes(lowerSearchValue); + const nameMatch = balance?.name && balance.name.toLowerCase().includes(lowerSearchValue); + const addressMatch = balance?.address && balance.address.toLowerCase().includes(lowerSearchValue); + + return symbolMatch || nameMatch || addressMatch; + }); }, [isConnected, selectedWallet?.balances, searchValue, selectedChainID])🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/chains/solana.svgis excluded by!**/*.svgsrc/assets/chains/tron.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
src/assets/chains/index.ts(1 hunks)src/components/Global/TokenSelector/NetworkButton.tsx(1 hunks)src/components/Global/TokenSelector/NetworkListItem.tsx(1 hunks)src/components/Global/TokenSelector/NetworkListView.tsx(1 hunks)src/components/Global/TokenSelector/NewTokenSelector.tsx(1 hunks)src/components/Global/TokenSelector/TokenListItem.tsx(1 hunks)src/components/Global/TokenSelector/TokenSelector.consts.ts(2 hunks)src/config/wagmi.config.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/config/wagmi.config.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
🧬 Code Graph Analysis (5)
src/components/Global/TokenSelector/TokenSelector.consts.ts (1)
src/config/wagmi.config.tsx (1)
networks(23-26)
src/components/Global/TokenSelector/TokenListItem.tsx (5)
src/interfaces/interfaces.ts (1)
IUserBalance(19-30)src/components/Global/Card/index.tsx (1)
CardPosition(4-4)src/context/tokenSelector.context.tsx (1)
tokenSelectorContext(24-44)src/utils/general.utils.ts (1)
formatAmount(259-299)src/components/Global/Icons/Icon.tsx (1)
Icon(99-108)
src/components/Global/TokenSelector/NetworkButton.tsx (2)
src/components/0_Bruddle/Button.tsx (1)
Button(63-96)src/components/Global/Icons/Icon.tsx (1)
Icon(99-108)
src/components/Global/TokenSelector/NetworkListItem.tsx (2)
src/components/0_Bruddle/Card.tsx (1)
Card(72-72)src/components/Global/Icons/Icon.tsx (1)
Icon(99-108)
src/components/Global/TokenSelector/NewTokenSelector.tsx (8)
src/interfaces/interfaces.ts (1)
IUserBalance(19-30)src/hooks/wallet/useWallet.ts (1)
useWallet(89-402)src/context/tokenSelector.context.tsx (1)
tokenSelectorContext(24-44)src/constants/zerodev.consts.ts (3)
PEANUT_WALLET_TOKEN(20-20)PEANUT_WALLET_CHAIN(18-18)PEANUT_WALLET_TOKEN_SYMBOL(21-21)src/utils/general.utils.ts (1)
formatAmount(259-299)src/components/Global/TokenSelector/TokenSelector.consts.ts (3)
TOKEN_SELECTOR_POPULAR_NETWORK_IDS(53-70)TOKEN_SELECTOR_SUPPORTED_NETWORK_IDS(73-73)TOKEN_SELECTOR_COMING_SOON_NETWORKS(39-50)src/components/Global/Icons/Icon.tsx (1)
Icon(99-108)src/components/Global/EmptyStates/EmptyState.tsx (1)
EmptyState(12-27)
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/NewTokenSelector.tsx
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
src/components/Global/TokenSelector/TokenSelector.consts.ts (1)
30-73: Well-structured network configuration data with proper typingThe constants and interface definitions for network configurations are well-organized and will provide consistent network metadata throughout the token selector component. Good use of mapping over the existing
networksarray to derive supported network IDs.Note: The icons for Solana and Tron will be displayed incorrectly due to the swapped imports in
src/assets/chains/index.ts. Make sure to fix that issue to ensure correct visual representation of these networks.
| const filteredChains = useMemo(() => { | ||
| const lowerSearchValue = searchValue.toLowerCase() | ||
|
|
||
| const activeChains = Object.values(chains) | ||
| .filter((chain) => allowedChainIds.has(chain.chainId)) | ||
| .filter( | ||
| (chain) => | ||
| chain.axelarChainName?.toLowerCase().includes(lowerSearchValue) || | ||
| chain.chainName?.toLowerCase().includes(lowerSearchValue) | ||
| ) | ||
| .map((chain) => ({ | ||
| chainId: chain.chainId, | ||
| name: chain.axelarChainName || chain.chainName, | ||
| iconUrl: chain.chainIconURI, | ||
| isComingSoon: false, | ||
| })) | ||
|
|
||
| const filteredComingSoon = comingSoonNetworks | ||
| .filter((network) => network.name.toLowerCase().includes(lowerSearchValue)) | ||
| .map((network) => ({ | ||
| chainId: network.chainId, | ||
| name: network.name, | ||
| iconUrl: network.iconUrl, | ||
| isComingSoon: true, | ||
| })) | ||
|
|
||
| return [...activeChains, ...filteredComingSoon] | ||
| }, [chains, searchValue, allowedChainIds, comingSoonNetworks]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive programming for data filtering
The chain filtering logic is well-implemented using useMemo for performance optimization. However, it lacks defensive programming to handle potential edge cases.
const filteredChains = useMemo(() => {
const lowerSearchValue = searchValue.toLowerCase()
const activeChains = Object.values(chains)
+ .filter(chain => chain && typeof chain === 'object')
.filter((chain) => allowedChainIds.has(chain.chainId))
.filter(
(chain) =>
chain.axelarChainName?.toLowerCase().includes(lowerSearchValue) ||
chain.chainName?.toLowerCase().includes(lowerSearchValue)
)
.map((chain) => ({
chainId: chain.chainId,
name: chain.axelarChainName || chain.chainName,
iconUrl: chain.chainIconURI,
isComingSoon: false,
}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const filteredChains = useMemo(() => { | |
| const lowerSearchValue = searchValue.toLowerCase() | |
| const activeChains = Object.values(chains) | |
| .filter((chain) => allowedChainIds.has(chain.chainId)) | |
| .filter( | |
| (chain) => | |
| chain.axelarChainName?.toLowerCase().includes(lowerSearchValue) || | |
| chain.chainName?.toLowerCase().includes(lowerSearchValue) | |
| ) | |
| .map((chain) => ({ | |
| chainId: chain.chainId, | |
| name: chain.axelarChainName || chain.chainName, | |
| iconUrl: chain.chainIconURI, | |
| isComingSoon: false, | |
| })) | |
| const filteredComingSoon = comingSoonNetworks | |
| .filter((network) => network.name.toLowerCase().includes(lowerSearchValue)) | |
| .map((network) => ({ | |
| chainId: network.chainId, | |
| name: network.name, | |
| iconUrl: network.iconUrl, | |
| isComingSoon: true, | |
| })) | |
| return [...activeChains, ...filteredComingSoon] | |
| }, [chains, searchValue, allowedChainIds, comingSoonNetworks]) | |
| const filteredChains = useMemo(() => { | |
| const lowerSearchValue = searchValue.toLowerCase() | |
| const activeChains = Object.values(chains) | |
| .filter(chain => chain && typeof chain === 'object') | |
| .filter((chain) => allowedChainIds.has(chain.chainId)) | |
| .filter( | |
| (chain) => | |
| chain.axelarChainName?.toLowerCase().includes(lowerSearchValue) || | |
| chain.chainName?.toLowerCase().includes(lowerSearchValue) | |
| ) | |
| .map((chain) => ({ | |
| chainId: chain.chainId, | |
| name: chain.axelarChainName || chain.chainName, | |
| iconUrl: chain.chainIconURI, | |
| isComingSoon: false, | |
| })) | |
| const filteredComingSoon = comingSoonNetworks | |
| .filter((network) => network.name.toLowerCase().includes(lowerSearchValue)) | |
| .map((network) => ({ | |
| chainId: network.chainId, | |
| name: network.name, | |
| iconUrl: network.iconUrl, | |
| isComingSoon: true, | |
| })) | |
| return [...activeChains, ...filteredComingSoon] | |
| }, [chains, searchValue, allowedChainIds, comingSoonNetworks]) |
| {isSearch ? ( | ||
| <Icon name="cancel" size={12} className="size-4 rotate-45" /> | ||
| ) : chainIconURI ? ( | ||
| <Image src={chainIconURI} alt={chainName} width={24} height={24} className="h-6 w-6 rounded-full" /> | ||
| ) : ( | ||
| <div className="flex h-6 w-6 items-center justify-center rounded-full bg-gray-300 text-xs"> | ||
| {chainName.substring(0, 3).toUpperCase()} | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for image loading
The component uses Next.js Image but doesn't handle image loading failures.
{isSearch ? (
<Icon name="cancel" size={12} className="size-4 rotate-45" />
) : chainIconURI ? (
- <Image src={chainIconURI} alt={chainName} width={24} height={24} className="h-6 w-6 rounded-full" />
+ <Image
+ src={chainIconURI}
+ alt={chainName}
+ width={24}
+ height={24}
+ className="h-6 w-6 rounded-full"
+ onError={(e) => {
+ e.currentTarget.onerror = null;
+ e.currentTarget.src = ''; // Could set to a fallback image path
+ e.currentTarget.parentElement.innerHTML = chainName.substring(0, 3).toUpperCase();
+ }}
+ />
) : (
<div className="flex h-6 w-6 items-center justify-center rounded-full bg-gray-300 text-xs">
{chainName.substring(0, 3).toUpperCase()}
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {isSearch ? ( | |
| <Icon name="cancel" size={12} className="size-4 rotate-45" /> | |
| ) : chainIconURI ? ( | |
| <Image src={chainIconURI} alt={chainName} width={24} height={24} className="h-6 w-6 rounded-full" /> | |
| ) : ( | |
| <div className="flex h-6 w-6 items-center justify-center rounded-full bg-gray-300 text-xs"> | |
| {chainName.substring(0, 3).toUpperCase()} | |
| </div> | |
| )} | |
| </div> | |
| {isSearch ? ( | |
| <Icon name="cancel" size={12} className="size-4 rotate-45" /> | |
| ) : chainIconURI ? ( | |
| <Image | |
| src={chainIconURI} | |
| alt={chainName} | |
| width={24} | |
| height={24} | |
| className="h-6 w-6 rounded-full" | |
| onError={(e) => { | |
| // Prevent infinite loop if fallback also errors | |
| e.currentTarget.onerror = null; | |
| // Optionally set a fallback image URL: | |
| e.currentTarget.src = ''; | |
| // Replace the image with the chain initials | |
| e.currentTarget.parentElement!.innerHTML = | |
| chainName.substring(0, 3).toUpperCase(); | |
| }} | |
| /> | |
| ) : ( | |
| <div className="flex h-6 w-6 items-center justify-center rounded-full bg-gray-300 text-xs"> | |
| {chainName.substring(0, 3).toUpperCase()} | |
| </div> | |
| )} | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/components/Global/TokenSelector/NewTokenSelector.tsx (5)
302-310: Enhance accessibility for the token selector buttonAdd appropriate ARIA attributes to improve accessibility for screen readers.
<Button variant="stroke" onClick={openDrawer} + aria-expanded={isDrawerOpen} + aria-haspopup="dialog" + aria-label={buttonSymbol && buttonChainName ? `Selected token: ${buttonSymbol} on ${buttonChainName}` : "Select Token"} className={twMerge( 'flex min-h-16 w-full items-center justify-between bg-white p-4 hover:bg-white hover:text-black', classNameButton )} shadowSize="4" >
70-75:⚠️ Potential issueFix memory leak in
closeDrawerThe
closeDrawerfunction returns a cleanup function that's never used by its callers, resulting in a small memory leak when the component mounts/unmounts frequently.const closeDrawer = useCallback(() => { setIsDrawerOpen(false) - const timer = setTimeout(() => setSearchValue(''), 200) - return () => clearTimeout(timer) + setTimeout(() => setSearchValue(''), 200) }, [])
131-132:⚠️ Potential issueApply optional chaining to prevent potential null reference errors
Properties are accessed without checking if they exist first, which could lead to runtime errors.
- const nameMatch = balance.name && balance.name.toLowerCase().includes(lowerSearchValue) - const addressMatch = balance.address && balance.address.toLowerCase().includes(lowerSearchValue) + const nameMatch = balance?.name && balance.name.toLowerCase().includes(lowerSearchValue) + const addressMatch = balance?.address && balance.address.toLowerCase().includes(lowerSearchValue)🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
216-216:⚠️ Potential issueFix duplicated chain name fallback
There's a duplicate reference to
network?.axelarChainNamein the fallback chain.- return network?.axelarChainName || network?.axelarChainName || `Chain ${selectedChainID}` + return network?.axelarChainName || `Chain ${selectedChainID}`
347-347:⚠️ Potential issueOptional chaining prevents a potential crash
supportedSquidChainsAndTokens[selectedChainID]may beundefineduntil networks are fetched. Accessing.axelarChainNamedirectly risksCannot read propertyerrors during the first render.- const network = supportedSquidChainsAndTokens[selectedChainID] + const network = supportedSquidChainsAndTokens?.[selectedChainID]
🧹 Nitpick comments (4)
src/components/Global/TokenSelector/NewTokenSelector.tsx (4)
45-45: Specify a proper type foronNetworkSelectparameterUsing
anytype reduces type safety. Consider using a more specific type that matches the network object structure.- onNetworkSelect?: (network: any) => void + onNetworkSelect?: (network: { + chainId: string; + axelarChainName?: string; + chainIconURI?: string; + [key: string]: any; + }) => void
104-111: Remove console logs before production deploymentConsole statements should be removed from production code as they can impact performance and expose implementation details.
- console.log('Using manually fetched external balances.') sourceBalances = externalBalances } else if (isConnected && selectedWallet?.balances) { - console.log("Using balances from useWallet's selectedWallet.") sourceBalances = selectedWallet.balances } else { - console.log('No balances available.') sourceBalances = [] }
266-267: Remove console log statementAnother console log statement that should be removed before production.
- console.log('Render: No selected token/chain in context for button display.')
160-170: Refactor duplicate code betweenhandleNetworkSelectandhandleChainSelectBoth functions perform similar operations but with slightly different implementations. Consider extracting the common logic to a shared helper function.
+ const updateSelectedNetwork = useCallback( + (chainId: string, network?: any) => { + setSelectedChainID(chainId) + setSelectedTokenAddress('0x0000000000000000000000000000000000000000') + + if (onNetworkSelect && network) { + onNetworkSelect(network) + } + }, + [setSelectedChainID, setSelectedTokenAddress, onNetworkSelect] + ) const handleNetworkSelect = useCallback( (chain: { chainId: string; name: string; iconURI: string }) => { - setSelectedChainID(chain.chainId) - setSelectedTokenAddress('0x0000000000000000000000000000000000000000') - - if (onNetworkSelect) { - onNetworkSelect(supportedSquidChainsAndTokens[chain.chainId]) - } + updateSelectedNetwork(chain.chainId, supportedSquidChainsAndTokens[chain.chainId]) }, - [setSelectedChainID, setSelectedTokenAddress, onNetworkSelect, supportedSquidChainsAndTokens] + [updateSelectedNetwork, supportedSquidChainsAndTokens] ) const handleChainSelect = useCallback( (chainId: string) => { - setSelectedChainID(chainId) - setSelectedTokenAddress('0x0000000000000000000000000000000000000000') setShowNetworkList(false) const selectedNetwork = Object.values(supportedSquidChainsAndTokens).find( (chain) => chain.chainId === chainId ) - if (selectedNetwork && onNetworkSelect) { - onNetworkSelect(selectedNetwork) - } + updateSelectedNetwork(chainId, selectedNetwork) }, - [setSelectedChainID, setSelectedTokenAddress, onNetworkSelect, supportedSquidChainsAndTokens] + [updateSelectedNetwork, supportedSquidChainsAndTokens, setShowNetworkList] )Also applies to: 184-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/NewTokenSelector.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (3)
src/components/Global/TokenSelector/NewTokenSelector.tsx (3)
440-460: LGTM: Well-implemented search input with clear buttonThe search input implementation is clean and user-friendly, with a clear button that appears only when there's text to clear.
507-520: LGTM: Good use of EmptyState componentThe EmptyState component is effectively used to guide users when they need to connect their wallet, with a clear call to action.
359-367: LGTM: Well-structured BottomDrawer configurationThe BottomDrawer is properly configured with appropriate height settings for different positions, enhancing the user experience on mobile.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/components/Global/TokenSelector/NewTokenSelector.tsx (7)
70-75:closeDrawerreturns a function that is never usedThe
useCallbackreturns theclearTimeoutcleanup function, but callers invokecloseDrawer()without using its return value, so the timer can never be cleared – a small memory leak on pages where the component mounts/unmounts often.- const closeDrawer = useCallback(() => { - setIsDrawerOpen(false) - const timer = setTimeout(() => setSearchValue(''), 200) - return () => clearTimeout(timer) - }, []) + const closeDrawer = useCallback(() => { + setIsDrawerOpen(false) + setTimeout(() => setSearchValue(''), 200) + }, [])
160-170: Reset token data when chain changes to avoid stale dataWhen changing the chain, it's important to reset token-related state to prevent stale data issues. You're correctly setting the token address to a default value, but should consider resetting any other token-related data.
216-216: Fix duplicated chain name fallbackSimilar to the issue in TokenListItem, there's a duplicate reference to
network?.axelarChainNamein the fallback chain.- return network?.axelarChainName || network?.axelarChainName || `Chain ${selectedChainID}` + return network?.axelarChainName || `Chain ${selectedChainID}`
302-357: Enhance accessibility for the token selector buttonThe button that toggles the token selector drawer lacks proper accessibility attributes to indicate its expanded state and purpose to screen readers.
Add appropriate ARIA attributes to improve accessibility:
<Button variant="stroke" onClick={openDrawer} + aria-expanded={isDrawerOpen} + aria-haspopup="dialog" + aria-label={buttonSymbol && buttonChainName ? `Selected token: ${buttonSymbol} on ${buttonChainName}` : "Select Token"} className={twMerge( 'flex min-h-16 w-full items-center justify-between bg-white p-4 hover:bg-white hover:text-black', classNameButton )} shadowSize="4" >
131-132:⚠️ Potential issueApply optional chaining to prevent potential null reference errors
The
balance.nameandbalance.addressproperties are accessed without checking if they exist first, which could lead to errors if these properties are undefined.- const nameMatch = balance.name && balance.name.toLowerCase().includes(lowerSearchValue) - const addressMatch = balance.address && balance.address.toLowerCase().includes(lowerSearchValue) + const nameMatch = balance?.name && balance.name.toLowerCase().includes(lowerSearchValue) + const addressMatch = balance?.address && balance.address.toLowerCase().includes(lowerSearchValue)🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
77-98: 🛠️ Refactor suggestionAdd timer cleanup in the wallet balances fetch effect
The effect lacks cleanup for asynchronous operations, which could cause issues if the component unmounts during the fetch operation.
useEffect(() => { + let isMounted = true; if (isExternalWalletConnected && !prevIsExternalConnected.current && externalWalletAddress) { setIsLoadingExternalBalances(true) setExternalBalances(null) fetchWalletBalances(externalWalletAddress) .then((balances) => { + if (isMounted) { setExternalBalances(balances.balances || []) + } }) .catch((error) => { console.error('Manual balance fetch failed:', error) + if (isMounted) { setExternalBalances([]) + } }) .finally(() => { + if (isMounted) { setIsLoadingExternalBalances(false) + } }) } else if (!isExternalWalletConnected && prevIsExternalConnected.current) { setExternalBalances(null) } prevIsExternalConnected.current = isExternalWalletConnected + + return () => { + isMounted = false; + } }, [isExternalWalletConnected, externalWalletAddress])
166-166:⚠️ Potential issueAdd optional chaining to prevent potential null reference errors
When accessing
supportedSquidChainsAndTokens[chain.chainId], optional chaining should be used to prevent null reference errors if the networks haven't been loaded yet.- onNetworkSelect(supportedSquidChainsAndTokens[chain.chainId]) + onNetworkSelect(supportedSquidChainsAndTokens?.[chain.chainId])
🧹 Nitpick comments (3)
src/components/Global/TokenSelector/NewTokenSelector.tsx (3)
100-144: Remove console.log statements before productionThere are several console.log statements in the component that should be removed before pushing to production to avoid cluttering the browser console.
- console.log('Using manually fetched external balances.') + // Using manually fetched external balances- console.log("Using balances from useWallet's selectedWallet.") + // Using balances from useWallet's selectedWallet- console.log('No balances available.') + // No balances availableAlso remove the console.log on line 266:
- console.log('Render: No selected token/chain in context for button display.') + // No selected token/chain in context for button display🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
184-197: Consolidate network selection handlers
handleChainSelectandhandleNetworkSelect(lines 160-170) perform similar functions with different implementations. Consider refactoring to consolidate these into a single handler to improve maintainability.// Replace both handlers with a single implementation const handleNetworkSelect = useCallback( (chainId: string, networkData?: { name?: string; iconURI?: string }) => { setSelectedChainID(chainId) setSelectedTokenAddress('0x0000000000000000000000000000000000000000') if (networkData?.name === undefined) { setShowNetworkList(false) } if (onNetworkSelect) { const networkToPass = supportedSquidChainsAndTokens?.[chainId] || Object.values(supportedSquidChainsAndTokens).find( (chain) => chain.chainId === chainId ); if (networkToPass) { onNetworkSelect(networkToPass) } } }, [setSelectedChainID, setSelectedTokenAddress, onNetworkSelect, supportedSquidChainsAndTokens] )
45-45: Use a more specific type foronNetworkSelectpropThe
onNetworkSelectprop is typed as(network: any) => void, which isn't type-safe. Consider creating a more specific interface for the network object.- onNetworkSelect?: (network: any) => void + onNetworkSelect?: (network: { + chainId: string; + axelarChainName?: string; + chainIconURI?: string; + [key: string]: any; + }) => void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Global/Icons/Icon.tsx(3 hunks)src/components/Global/Icons/wallet-cancel.tsx(1 hunks)src/components/Global/Icons/wallet-outline.tsx(1 hunks)src/components/Global/TokenSelector/NewTokenSelector.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/components/Global/Icons/wallet-outline.tsx
- src/components/Global/Icons/wallet-cancel.tsx
- src/components/Global/Icons/Icon.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/NewTokenSelector.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/components/Global/TokenSelector/TokenListItem.tsx (1)
30-33: Duplicate expression in fallback chain nameThe fallback uses
chain?.axelarChainNametwice, which is redundant and was already pointed out in a previous review.return chain?.axelarChainName || chain?.axelarChainName || `Chain ${balance.chainId}`Replace the second occurrence with the intended fallback.
- return chain?.axelarChainName || chain?.axelarChainName || `Chain ${balance.chainId}` + return chain?.axelarChainName || `Chain ${balance.chainId}`src/components/Global/TokenSelector/NewTokenSelector.tsx (2)
79-100: Missing cleanup in balance-fetch effect can update unmounted componentIf the component unmounts while the asynchronous
fetchWalletBalancescall is in flight, theset…calls in the promise chain will try to update state on an unmounted component, leading to memory-leak warnings.Re-introduce the “isMounted” guard suggested previously:
useEffect(() => { - if (isExternalWalletConnected && !prevIsExternalConnected.current && externalWalletAddress) { + let isMounted = true + if (isExternalWalletConnected && !prevIsExternalConnected.current && externalWalletAddress) { setIsLoadingExternalBalances(true) setExternalBalances(null) fetchWalletBalances(externalWalletAddress) - .then((balances) => { - setExternalBalances(balances.balances || []) + .then((balances) => { + if (isMounted) setExternalBalances(balances.balances || []) }) .catch((error) => { console.error('Manual balance fetch failed:', error) - setExternalBalances([]) + if (isMounted) setExternalBalances([]) }) .finally(() => { - setIsLoadingExternalBalances(false) + if (isMounted) setIsLoadingExternalBalances(false) }) ... -}, [isExternalWalletConnected, externalWalletAddress]) + return () => { + isMounted = false + } +}, [isExternalWalletConnected, externalWalletAddress])
210-214: Duplicate expression in network name fallbackSame duplication as earlier:
network?.axelarChainNameis checked twice.- return network?.axelarChainName || network?.axelarChainName || `Chain ${selectedChainID}` + return network?.axelarChainName || `Chain ${selectedChainID}`
🧹 Nitpick comments (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx (1)
129-133: Optional-chain to avoid null reference & satisfy linterStatic analysis (Biome) flags direct property access that could crash when
balance.name/balance.addressareundefined.- const nameMatch = balance.name && balance.name.toLowerCase().includes(lowerSearchValue) - const addressMatch = balance.address && balance.address.toLowerCase().includes(lowerSearchValue) + const nameMatch = balance?.name?.toLowerCase().includes(lowerSearchValue) + const addressMatch = balance?.address?.toLowerCase().includes(lowerSearchValue)This keeps behaviour identical while eliminating the edge-case crash and silencing the linter.
🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Global/TokenSelector/NetworkButton.tsx(1 hunks)src/components/Global/TokenSelector/NetworkListView.tsx(1 hunks)src/components/Global/TokenSelector/NewTokenSelector.tsx(1 hunks)src/components/Global/TokenSelector/TokenListItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Global/TokenSelector/NetworkButton.tsx
- src/components/Global/TokenSelector/NetworkListView.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/Global/TokenSelector/NewTokenSelector.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/NewTokenSelector.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
src/components/Global/TokenSelector/TokenListItem.tsx (1)
35-40:✅ Verification successful
balance.amountmay not support.toFixed– verify numeric type
balance.amountis assumed to be a native number. If it is aBigNumber,bigint,string, orDecimal.jsinstance, calling.toFixed()will throw at runtime.Consider normalising the value first (e.g. via
Number()or a library helper) or delegating the formatting to a utility that is aware of the underlying type.
🏁 Script executed:
#!/bin/bash # Locate component definition and props rg -n "TokenListItem" -C3 src/components/Global/TokenSelector/TokenListItem.tsx # Inspect usage of `balance` and its type import rg -n "balance" -C5 src/components/Global/TokenSelector/TokenListItem.tsx # Search for the `TokenBalance` or related type definitions rg -n "interface .*Balance" -C3 srcLength of output: 5404
🏁 Script executed:
#!/bin/bash # Display the IUserBalance interface definition with context sed -n '1,50p' src/interfaces/interfaces.ts | sed -n '1,200!d'Length of output: 62
🏁 Script executed:
#!/bin/bash # Show the IUserBalance interface definition for context rg -n "export interface IUserBalance" -C5 src/interfaces/interfaces.ts # Show the declaration of the `amount` field within that interface rg -n "amount" -C5 src/interfaces/interfaces.tsLength of output: 1890
Safe to use
.toFixed()onbalance.amount
IUserBalance.amountis declared as a nativenumber, so callingbalance.amount.toFixed(...)is safe at runtime. No changes required.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
src/components/Global/TokenSelector/Components/NetworkButton.tsx (2)
39-49: Consider adding alt text for accessibility when using Icon componentThe Icon component with
name="cancel"should have an appropriate aria-label for screen readers. This helps ensure the component is accessible to all users.- <Icon name="cancel" size={12} className="size-4 rotate-45" /> + <Icon name="cancel" size={12} className="size-4 rotate-45" aria-label="Clear search" />
25-32: Consider adding ARIA attributes for selected stateSince this component represents a selectable button that can be in a selected state, adding appropriate ARIA attributes would improve accessibility.
<Button variant="stroke" className={twMerge( 'shadow-2 flex h-fit min-w-14 flex-1 flex-col items-center justify-center gap-1 p-3 text-center text-black hover:text-black', isSelected ? 'bg-primary-3 hover:bg-primary-3' : 'bg-white hover:bg-white' )} onClick={onClick} + aria-pressed={isSelected} >src/components/Global/TokenSelector/Components/NetworkListItem.tsx (1)
42-50: Fallback avatar shows raw uppercase initials for multi-byte names
name.substring(0, 2)will slice the UTF-16 string, breaking on emojis / multibyte chars (e.g. “🚀Sol”).
Consider usingArray.from(name)[0]to respect user-perceived characters or using a dedicated lib likegrapheme-splitter.src/components/Global/TokenSelector/Components/NetworkListView.tsx (2)
68-76: Add accessible label to the search inputA labelled control is required for screen-reader users.
-<BaseInput +<BaseInput + aria-label="Search networks"
90-101: Click handler still passed to “coming soon” items
onClick={() => onSelectChain(chain.chainId)}is evaluated for all entries; the inner component just drops it.
Passingundefinedavoids needless re-renders and accidental propagation.-onClick={() => onSelectChain(chain.chainId)} +onClick={chain.isComingSoon ? undefined : () => onSelectChain(chain.chainId)}src/components/Global/TokenSelector/Components/TokenListItem.tsx (1)
43-46: Use<button>instead of<div>for accessibilitySame reasoning as in
NetworkListItem.src/components/Global/TokenSelector/TokenSelector.tsx (2)
170-176: Hard-coded constants reduce flexibility
PEANUT_WALLET_TOKEN&PEANUT_WALLET_CHAINare inlined here.
Consider injecting them via props/ctx so this component can be reused in white-label builds.
295-306: Possible performance hit when searching large balance arrays repeatedly
displayTokensrecalculates on every keystroke.
DebouncesetSearchValueor memoiselowerSearchValuewithuseDeferredValue(React 18) to keep the UI responsive for wallets with hundreds of assets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/assets/chains/index.ts(1 hunks)src/components/Cashout/Components/Initial.view.tsx(2 hunks)src/components/Claim/Link/Initial.view.tsx(1 hunks)src/components/Global/TokenSelector/Components/AdvancedButton.tsx(0 hunks)src/components/Global/TokenSelector/Components/NetworkButton.tsx(1 hunks)src/components/Global/TokenSelector/Components/NetworkListItem.tsx(1 hunks)src/components/Global/TokenSelector/Components/NetworkListView.tsx(1 hunks)src/components/Global/TokenSelector/Components/SimpleTokenDisplay.tsx(0 hunks)src/components/Global/TokenSelector/Components/TokenDisplay.tsx(0 hunks)src/components/Global/TokenSelector/Components/TokenListItem.tsx(1 hunks)src/components/Global/TokenSelector/Components/index.ts(0 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(1 hunks)src/components/Payment/PaymentForm/index.tsx(1 hunks)src/components/Send/link/views/Initial.link.send.view.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- src/components/Global/TokenSelector/Components/index.ts
- src/components/Global/TokenSelector/Components/SimpleTokenDisplay.tsx
- src/components/Global/TokenSelector/Components/TokenDisplay.tsx
- src/components/Global/TokenSelector/Components/AdvancedButton.tsx
✅ Files skipped from review due to trivial changes (2)
- src/components/Cashout/Components/Initial.view.tsx
- src/assets/chains/index.ts
🧰 Additional context used
🧠 Learnings (1)
src/components/Global/TokenSelector/TokenSelector.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-10-08T20:13:42.967Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
🧬 Code Graph Analysis (3)
src/components/Global/TokenSelector/Components/NetworkButton.tsx (3)
src/components/0_Bruddle/Button.tsx (1)
Button(63-96)src/components/Global/Icons/Icon.tsx (1)
Icon(105-114)src/utils/general.utils.ts (1)
getInitialsFromName(1106-1113)
src/components/Global/TokenSelector/Components/NetworkListItem.tsx (2)
src/components/0_Bruddle/Card.tsx (1)
Card(72-72)src/components/Global/Icons/Icon.tsx (1)
Icon(105-114)
src/components/Global/TokenSelector/Components/NetworkListView.tsx (4)
src/components/Global/TokenSelector/TokenSelector.consts.ts (1)
NetworkConfig(32-36)src/components/Global/Icons/Icon.tsx (1)
Icon(105-114)src/components/0_Bruddle/Button.tsx (1)
Button(63-96)src/components/Global/EmptyStates/EmptyState.tsx (1)
EmptyState(12-27)
🪛 Biome (1.9.4)
src/components/Global/TokenSelector/TokenSelector.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/components/Payment/PaymentForm/index.tsx (1)
559-559: TokenSelector prop removal aligns with component refactoringThe props
onReset={resetTokenAndChain}andshowOnlySquidSupportedhave been removed from the TokenSelector component. This aligns with the PR's goal of refactoring the token selector to be more self-contained with integrated external wallet support.src/components/Send/link/views/Initial.link.send.view.tsx (1)
170-171: Simplified useWallet hook destructuringThe
isExternalWalletproperty has been removed from the useWallet hook destructuring. This is consistent with the removal of TokenSelector integration that previously depended on this property.src/components/Claim/Link/Initial.view.tsx (1)
589-589: TokenSelector prop removal follows new component patternSimilar to other files, the props
onResetandshowOnlySquidSupportedhave been removed from the TokenSelector component. This is part of the consistent pattern of simplifying the TokenSelector interface across the application.src/components/Global/TokenSelector/Components/NetworkButton.tsx (1)
1-56: Well-structured network button component with flexible display optionsThe new NetworkButton component provides a clean, reusable UI element for network selection with appropriate conditional rendering based on state (search/selected). It properly handles cases where no chain icon is available by using initials as a fallback.
src/components/Global/TokenSelector/Components/NetworkListItem.tsx
Outdated
Show resolved
Hide resolved
kushagrasarathe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/0_Bruddle/Button.tsx (1)
74-75: Button hover behavior has been updated with dynamic transition effects.The transition style has been changed from what was likely a
transition-transformtotransition-all, and the hover effect now includes shadow removal and position shifting based on the shadowSize prop. This creates a "push" effect when hovering rather than on active press.While this provides immediate visual feedback on hover, be aware that:
- Using
transition-allmight be slightly less performant than transitioning specific properties- The dynamic CSS injection with
${shadowSize}pxworks but ensure shadowSize is always properly validated- Testing should verify this change maintains consistent UX across all button usage scenarios
Consider being more explicit about which properties transition for improved performance:
-`btn w-full flex items-center gap-2 transform transition-all hover:shadow-none hover:translate-x-[3px] hover:translate-y-[${shadowSize}px] notranslate`, +`btn w-full flex items-center gap-2 transform transition-[transform,box-shadow] hover:shadow-none hover:translate-x-[3px] hover:translate-y-[${shadowSize}px] notranslate`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/0_Bruddle/Button.tsx(1 hunks)src/components/Global/TokenSelector/Components/NetworkListView.tsx(1 hunks)src/components/Global/TokenSelector/Components/TokenListItem.tsx(1 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Global/TokenSelector/Components/NetworkListView.tsx
- src/components/Global/TokenSelector/Components/TokenListItem.tsx
- src/components/Global/TokenSelector/TokenSelector.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
| {isExternalWallet && ( | ||
| <> | ||
| <TokenSelector classNameButton="max-w-[100%]" /> | ||
| <TokenSelector /> |
| }} | ||
| /> | ||
| )} | ||
| !isPeanutClaimOnlyMode() && <TokenSelector />} |
There was a problem hiding this comment.
-
wtf is an empty state? what does this file do, why do we need it? - pls comment on file
-
why doe sit have so much inline styling?
There was a problem hiding this comment.
- noted, will add a comment
- it's a custom component from design, that we are now reusing and will be using more throughout the application when we dont have any data to show, example, you search hugo, but hugo doesnt exist (🤣 ), so we show this component
src/components/Global/Icons/Icon.tsx
Outdated
| import { WalletCancelIcon } from './wallet-cancel' | ||
| import { WalletOutlineIcon } from './wallet-outline' | ||
|
|
||
| // allowed icon names |
There was a problem hiding this comment.
mininit: is allowed the right word here?
There was a problem hiding this comment.
these are the only icons we support, ig available icons would be better
| const popularSymbolsToFind = ['USDC', 'USDT'] | ||
| const nativeTokenAddress = '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' // note: native token address in squid data is 0xe.. and not 0x00.. |
There was a problem hiding this comment.
blocker: why another hardcoded place? pls justify
There was a problem hiding this comment.
We have SQUID_ETH_ADDRESS
| const nativeTokenAddress = '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' // note: native token address in squid data is 0xe.. and not 0x00.. | ||
|
|
||
| // creates a placeholder balance popular tokens, note we dont show the balance amount for these tokens | ||
| const createPlaceholderBalance = (token: IToken, chainId: string): IUserBalance => ({ |
There was a problem hiding this comment.
this feels like bad practice
There was a problem hiding this comment.
hmm, using this to show popular token in the ui, where we dont need balance, following this approach to reuse the existing component which hides balance when its a popular token, renaming this to be more clear
| const popularSymbolsToFind = ['USDC', 'USDT'] | ||
| const nativeTokenAddress = '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' // note: native token address in squid data is 0xe.. and not 0x00.. |
There was a problem hiding this comment.
We have SQUID_ETH_ADDRESS
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Global/TokenSelector/Components/SearchInput.tsx (1)
1-53: 💡 Verification agent🧩 Analysis chain
Consider potential naming conflicts with existing SearchInput component.
Based on the relevant code snippets, there appears to be another
SearchInputcomponent insrc/components/SearchUsers/SearchInput.tsx. Having two components with the same name but different implementations could lead to confusion.
🏁 Script executed:
#!/bin/bash # Check for other SearchInput components in the codebase rg "export (const|default) SearchInput" --type typescriptLength of output: 94
🏁 Script executed:
#!/bin/bash # List all files named SearchInput.tsx in the repository echo "Files named SearchInput.tsx:" find . -type f -name "SearchInput.tsx" # For each file found, check for SearchInput exports echo echo "Export declarations in each SearchInput.tsx:" for file in $(find . -type f -name "SearchInput.tsx"); do echo ">> $file" grep -nE "export\s+(const|default)\s+SearchInput" "$file" || echo " No export SearchInput found" doneLength of output: 1073
Disambiguate duplicate SearchInput components
There are two distinct SearchInput components in the repo:
- src/components/Global/TokenSelector/Components/SearchInput.tsx (export default SearchInput)
- src/components/SearchUsers/SearchInput.tsx (export const SearchInput)
Having both named identically can lead to import conflicts and developer confusion. Please rename one (or both) of the components (and their files/exports) to clearly reflect their purpose—for example:
// src/components/Global/TokenSelector/Components/SearchInput.tsx -export default SearchInput +export default TokenSearchInputand update all corresponding imports:
-import SearchInput from '@/components/Global/TokenSelector/Components/SearchInput' +import TokenSearchInput from '@/components/Global/TokenSelector/Components/TokenSearchInput'– Ensure barrel files or index exports are updated accordingly.
🧹 Nitpick comments (6)
src/components/Global/TokenSelector/Components/ScrollableList.tsx (2)
10-16: Component implementation is clean and follows React best practices.The component has a clear, focused purpose and makes good use of default props. The tailwind classes are organized logically with
twMergeto handle class combinations properly.Consider adding ARIA attributes for better accessibility:
- <div className={twMerge('overflow-y-auto pb-2 pr-1', maxHeight, className)}> + <div + className={twMerge('overflow-y-auto pb-2 pr-1', maxHeight, className)} + role="region" + aria-label="Scrollable content" + >
1-19: Consider adding JSDoc documentation and virtualization for large lists.This component seems well-implemented for its purpose, but could benefit from two enhancements:
- Add JSDoc documentation to explain the component's purpose and usage
- For performance with large lists, consider adding virtualization support or a note about when to use this versus a virtualized alternative
Add JSDoc documentation to improve developer experience:
+ /** + * ScrollableList - A reusable component for creating scrollable content areas + * + * @param {ReactNode} children - Content to be displayed in the scrollable area + * @param {string} [className] - Additional CSS classes to apply to the container + * @param {string} [maxHeight='max-h-[50vh]'] - Tailwind CSS class for maximum height + * + * @example + * <ScrollableList maxHeight="max-h-[30vh]"> + * <ItemOne /> + * <ItemTwo /> + * </ScrollableList> + */ const ScrollableList: React.FC<ScrollableListProps> = ({ children, className, maxHeight = 'max-h-[50vh]' }) => {Do you have any test coverage for this component? If not, consider adding tests to verify it renders correctly with different prop combinations.
src/hooks/ui/useDynamicHeight.ts (2)
26-51: Consider adding window resize event handling.While the ResizeObserver catches changes to the element's size, window resize events might also affect the viewport height calculation. Consider adding a window resize listener to recalculate when the viewport dimensions change.
useEffect(() => { const element = ref.current if (!element) return let initialMeasureDone = false // flag to track initial measurement const observer = new ResizeObserver((entries) => { window.requestAnimationFrame(() => { for (const entry of entries) { const elementHeight = entry.contentRect.height // ensure we don't calculate height based on a 0-height element initially if (elementHeight <= 0 && !initialMeasureDone) continue const calculatedVh = (elementHeight / window.innerHeight) * 100 + extraVhOffset const constrainedVh = Math.max(minHeightVh, Math.min(maxHeightVh, calculatedVh)) setHeightVh(constrainedVh) initialMeasureDone = true // mark initial measurement as done } }) }) + const handleWindowResize = () => { + if (!element || !initialMeasureDone) return + + window.requestAnimationFrame(() => { + const elementHeight = element.offsetHeight + const calculatedVh = (elementHeight / window.innerHeight) * 100 + extraVhOffset + const constrainedVh = Math.max(minHeightVh, Math.min(maxHeightVh, calculatedVh)) + + setHeightVh(constrainedVh) + }) + } observer.observe(element) + window.addEventListener('resize', handleWindowResize) return () => { observer.disconnect() + window.removeEventListener('resize', handleWindowResize) } }, [ref, maxHeightVh, minHeightVh, extraVhOffset])
1-54: Consider implementing a debounce mechanism.For frequently resizing elements or in performance-critical applications, implementing a debounce mechanism could optimize performance by limiting how often calculations and re-renders occur.
import { RefObject, useEffect, useState } from 'react' + import { debounce } from 'lodash' // Or implement your own debounce function interface DynamicHeightOptions { maxHeightVh?: number minHeightVh?: number extraVhOffset?: number // extra vh to add to the calculated height + debounceMs?: number // Optional debounce time in milliseconds } /** * custom hook to calculate the dynamic height of a referenced element * as a percentage of the viewport height (vh), with constraints. * * @param ref RefObject pointing to the HTML element whose height needs to be measured. * @param options Configuration options for max/min height and offset. * @returns The calculated height in vh units, or null initially. */ export function useDynamicHeight(ref: RefObject<HTMLElement>, options: DynamicHeightOptions = {}): number | null { const { maxHeightVh = 90, // Default max height 90vh minHeightVh = 10, // Default min height 10vh extraVhOffset = 5, // Default extra 5vh offset + debounceMs = 0, // Default no debounce } = options const [heightVh, setHeightVh] = useState<number | null>(null) // initialize with null useEffect(() => { const element = ref.current if (!element) return let initialMeasureDone = false // flag to track initial measurement + const updateHeight = (elementHeight: number) => { + // ensure we don't calculate height based on a 0-height element initially + if (elementHeight <= 0 && !initialMeasureDone) return + + const calculatedVh = (elementHeight / window.innerHeight) * 100 + extraVhOffset + const constrainedVh = Math.max(minHeightVh, Math.min(maxHeightVh, calculatedVh)) + + setHeightVh(constrainedVh) + initialMeasureDone = true // mark initial measurement as done + } + + const debouncedUpdateHeight = debounceMs > 0 + ? debounce(updateHeight, debounceMs) + : updateHeight const observer = new ResizeObserver((entries) => { window.requestAnimationFrame(() => { for (const entry of entries) { - const elementHeight = entry.contentRect.height - // ensure we don't calculate height based on a 0-height element initially - if (elementHeight <= 0 && !initialMeasureDone) continue - - const calculatedVh = (elementHeight / window.innerHeight) * 100 + extraVhOffset - const constrainedVh = Math.max(minHeightVh, Math.min(maxHeightVh, calculatedVh)) - - setHeightVh(constrainedVh) - initialMeasureDone = true // mark initial measurement as done + debouncedUpdateHeight(entry.contentRect.height) } }) }) observer.observe(element) - return () => observer.disconnect() + return () => { + observer.disconnect() + if (debounceMs > 0 && typeof debouncedUpdateHeight.cancel === 'function') { + debouncedUpdateHeight.cancel() + } + } - }, [ref, maxHeightVh, minHeightVh, extraVhOffset]) + }, [ref, maxHeightVh, minHeightVh, extraVhOffset, debounceMs]) return heightVh }src/components/Global/TokenSelector/Components/SearchInput.tsx (2)
35-47: Consider enhancing accessibility for the search icon.While the clear button has an aria-label, the search icon doesn't have any accessibility attributes. This could make the purpose of the input less clear to screen reader users.
Consider adding an
aria-labelto the input element itself to indicate it's a search field:<BaseInput variant="md" className={twMerge('h-10 w-full border border-black px-10 text-sm font-normal', inputClassName)} placeholder={placeholder} value={value} onChange={(e) => onChange(e.target.value)} + aria-label="Search input" {...rest} />
36-47: Consider focus management after clearing the input.When the user clicks the clear button, it would be helpful to return focus to the input field to allow for continued typing.
import { Button } from '@/components/0_Bruddle' import BaseInput from '@/components/0_Bruddle/BaseInput' import { Icon } from '@/components/Global/Icons/Icon' -import React from 'react' +import React, { useRef } from 'react' import { twMerge } from 'tailwind-merge' // ... const SearchInput: React.FC<SearchInputProps> = ({ value, onChange, onClear, placeholder = 'Search...', className, inputClassName, ...rest }) => { + const inputRef = useRef<HTMLInputElement>(null) + + const handleClear = () => { + onClear() + inputRef.current?.focus() + } return ( <div className={twMerge('relative', className)}> <BaseInput variant="md" + ref={inputRef} className={twMerge('h-10 w-full border border-black px-10 text-sm font-normal', inputClassName)} placeholder={placeholder} value={value} onChange={(e) => onChange(e.target.value)} {...rest} /> <Icon name="search" className="absolute left-3 top-1/2 h-5 w-5 -translate-y-1/2 text-grey-1" /> {value && ( <Button variant="transparent" - onClick={onClear} + onClick={handleClear} className="absolute right-2 top-1/2 w-fit -translate-y-1/2 p-0" aria-label="Clear search" > <div className="flex size-6 items-center justify-center"> <Icon name="cancel" className="h-5 w-5 text-grey-1" /> </div> </Button> )} </div> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/Global/EmptyStates/EmptyState.tsx(1 hunks)src/components/Global/Icons/Icon.tsx(4 hunks)src/components/Global/Icons/star.tsx(1 hunks)src/components/Global/TokenSelector/Components/NetworkListItem.tsx(1 hunks)src/components/Global/TokenSelector/Components/NetworkListView.tsx(1 hunks)src/components/Global/TokenSelector/Components/ScrollableList.tsx(1 hunks)src/components/Global/TokenSelector/Components/SearchInput.tsx(1 hunks)src/components/Global/TokenSelector/Components/TokenListItem.tsx(1 hunks)src/components/Global/TokenSelector/TokenSelector.consts.ts(2 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(1 hunks)src/hooks/ui/useDynamicHeight.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Global/Icons/star.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/Global/Icons/Icon.tsx
- src/components/Global/TokenSelector/TokenSelector.consts.ts
- src/components/Global/EmptyStates/EmptyState.tsx
- src/components/Global/TokenSelector/Components/NetworkListItem.tsx
- src/components/Global/TokenSelector/Components/TokenListItem.tsx
- src/components/Global/TokenSelector/Components/NetworkListView.tsx
- src/components/Global/TokenSelector/TokenSelector.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Global/TokenSelector/Components/SearchInput.tsx (3)
src/components/SearchUsers/SearchInput.tsx (1)
SearchInput(13-38)src/components/Global/Icons/Icon.tsx (1)
Icon(108-117)src/components/0_Bruddle/Button.tsx (1)
Button(63-96)
🔇 Additional comments (12)
src/components/Global/TokenSelector/Components/ScrollableList.tsx (3)
1-3: Good imports and dependencies setup.The component imports only what it needs from React and uses tailwind-merge effectively for handling class conflicts.
4-8: Well-defined props interface with helpful comment.The props interface clearly defines the component's API with appropriate types. The comment on
maxHeightis helpful for explaining the expected format.
18-19: Appropriate export pattern.Using default export makes sense for this component.
src/hooks/ui/useDynamicHeight.ts (5)
3-7: Well-structured interface with clear purpose.The DynamicHeightOptions interface is clearly defined with appropriate optional parameters, making it flexible and reusable. The comment on extraVhOffset nicely explains its purpose.
9-16: Good documentation practice.The JSDoc comments are thorough and provide clear information about the hook's purpose, parameters, and return value, which improves code maintainability and makes it easier for other developers to use.
17-23: Good default values and parameter handling.The hook provides sensible default values for all configuration options while allowing customization. The parameter structure is clean and follows React conventions.
24-44: Well-implemented resize observation with performance considerations.The implementation uses ResizeObserver with requestAnimationFrame, which is the modern approach for efficiently tracking element size changes. The initial measurement flag is a good practice to avoid calculations on elements that aren't fully rendered yet.
50-51: Proper cleanup in useEffect.The hook correctly cleans up the ResizeObserver when the component unmounts, preventing memory leaks.
src/components/Global/TokenSelector/Components/SearchInput.tsx (4)
1-5: Import statements look good.All necessary dependencies are imported correctly, including
Button,BaseInput,Icon,React, andtwMerge.
7-14: Well-structured Props interface.The props interface correctly extends
BaseInputprops while providing custom typing foronChangeto handle string values directly. Required and optional props are properly defined.
16-24: Component declaration looks good.Props are correctly destructured with a sensible default for the placeholder text.
25-34: Input element implementation is clean.Good use of
twMergefor combining class names and proper event handling for the onChange event.
contributes to TASK-10626
description
todo:
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Chores