[TASK-6955] feat: populate token selector with squid supported token #539
Conversation
Per flow we only want to show tokens that make sense, for example if we are paying we dont want to show tokens that are not supported by squid.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new file for fetching and caching squid chains and tokens, utilizing the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
src/components/Global/TokenSelector/TokenSelector.consts.ts (1)
10-10: Add JSDoc documentation for the new property.The new property is well-placed and aligns with the PR objectives. However, it would benefit from documentation to explain its purpose and behavior.
Consider adding JSDoc documentation:
export interface TokenSelectorProps { classNameButton?: string shouldBeConnected?: boolean + /** + * When true, displays only tokens that are supported by Squid for cross-chain operations. + * @since 1.0.0 + */ showOnlySquidSupported?: boolean onReset?: () => void }src/app/actions/squid.ts (1)
48-71: Remove commented out code.The commented-out version of the function is no longer needed and should be removed to maintain code cleanliness.
-/* -export async function getSquidChainsAndTokens(): Promise<Record<string, interfaces.ISquidChain & {tokens: interfaces.ISquidToken[]}>> { - console.log('======= Fetching squid chains and tokens =======') - const chains = await getSquidChainsCache() - const tokens = await getSquidTokensCache() - - const chainsById = chains.filter(chain => 'evm' === chain.chainType) - .reduce((acc, chain) => { - acc[chain.chainId] = chain - return acc - }, {} as Record<string, any>) - - tokens.forEach(token => { - if (token.chainId in chainsById) { - if (!chainsById[token.chainId].tokens) { - chainsById[token.chainId].tokens = [] - } - chainsById[token.chainId].tokens.push(token) - } - }) - - return chainsById -} -*/src/context/tokenSelector.context.tsx (1)
58-60: Consider adding loading state management.While the state initialization is correct, consider adding a loading state to handle the initial data fetch of Squid chains and tokens. This would help prevent potential UI flickers and provide better user feedback.
+ const [isLoadingSquidData, setIsLoadingSquidData] = useState(true) const [supportedSquidChainsAndTokens, setSupportedSquidChainsAndTokens] = useState< Record<string, interfaces.ISquidChain & { tokens: interfaces.ISquidToken[] }> >({})src/components/Claim/Claim.tsx (5)
Line range hint
47-50: Consider memoizing context valuesTo optimize performance, consider memoizing the context values using
useMemoif they're being passed down to multiple child components.-const { setSelectedChainID, setSelectedTokenAddress } = useContext(context.tokenSelectorContext) +const memoizedContextValue = useMemo(() => { + const { setSelectedChainID, setSelectedTokenAddress } = useContext(context.tokenSelectorContext) + return { setSelectedChainID, setSelectedTokenAddress } +}, [])
Line range hint
52-73: Add error boundaries for screen navigationThe screen navigation handlers are well-implemented, but consider adding error boundaries to gracefully handle any potential state corruption during navigation.
const handleOnCustom = (screen: _consts.ClaimScreens) => { + try { setStep(() => ({ screen: screen, idx: _consts.CLAIM_SCREEN_FLOW.indexOf(screen), })) + } catch (error) { + console.error('Navigation failed:', error) + // Reset to initial state or show error UI + } }
Line range hint
74-116: Refactor checkLink for better separation of concernsThe
checkLinkfunction handles multiple responsibilities. Consider breaking it down into smaller, focused functions for better maintainability and testing.+const setupTokenPrice = async (tokenAddress: string, chainId: number) => { + const tokenPrice = await utils.fetchTokenPrice(tokenAddress.toLowerCase(), chainId) + return tokenPrice?.price || 0 +} +const setupEstimatedPoints = async (params: { + address: string + chainId: number + amountUSD: number +}) => { + return estimatePoints({ + address: params.address, + chainId: params.chainId, + amountUSD: params.amountUSD, + actionType: ActionType.CLAIM, + }) +} const checkLink = async (link: string) => { try { const linkDetails: interfaces.ILinkDetails = await peanut.getLinkDetails({ link, }) - const attachmentInfo = await getAttachmentInfo(linkDetails.link) - setAttachment({ - message: attachmentInfo?.message, - attachmentUrl: attachmentInfo?.fileUrl, - }) + await setupAttachment(linkDetails.link) setClaimLinkData(linkDetails) setSelectedChainID(linkDetails.chainId) setSelectedTokenAddress(linkDetails.tokenAddress) if (linkDetails.claimed) { setLinkState(_consts.claimLinkStateType.ALREADY_CLAIMED) return } - const tokenPrice = await utils.fetchTokenPrice(linkDetails.tokenAddress.toLowerCase(), linkDetails.chainId) - tokenPrice && setTokenPrice(tokenPrice?.price) + const price = await setupTokenPrice(linkDetails.tokenAddress, linkDetails.chainId) + setTokenPrice(price) if (address) { setRecipient({ name: '', address }) - const estimatedPoints = await estimatePoints({ - address: address ?? '', - chainId: linkDetails.chainId, - amountUSD: Number(linkDetails.tokenAmount) * (tokenPrice?.price ?? 0), - actionType: ActionType.CLAIM, - }) + const estimatedPoints = await setupEstimatedPoints({ + address, + chainId: linkDetails.chainId, + amountUSD: Number(linkDetails.tokenAmount) * price, + }) setEstimatedPoints(estimatedPoints) }
Line range hint
118-123: Improve useEffect implementationThe useEffect hook should include proper cleanup and dependencies.
-useEffect(() => { +useEffect(() => { + let mounted = true const pageUrl = typeof window !== 'undefined' ? window.location.href : '' if (pageUrl) { - checkLink(pageUrl) + checkLink(pageUrl).catch(error => { + if (mounted) { + console.error('Failed to check link:', error) + setLinkState(_consts.claimLinkStateType.NOT_FOUND) + } + }) } -}, []) + return () => { + mounted = false + } +}, [checkLink])
Line range hint
125-199: Extract loading state into separate componentConsider extracting the loading state into a separate component for better reusability and maintainability.
+const LoadingSpinner = () => ( + <div className="relative flex w-full items-center justify-center"> + <div className="animate-spin"> + <img src={assets.PEANUTMAN_LOGO.src} alt="logo" className="h-6 sm:h-10" /> + <span className="sr-only">Loading...</span> + </div> + </div> +) return ( <div className="card"> - {linkState === _consts.claimLinkStateType.LOADING && ( - <div className="relative flex w-full items-center justify-center"> - <div className="animate-spin"> - <img src={assets.PEANUTMAN_LOGO.src} alt="logo" className="h-6 sm:h-10" /> - <span className="sr-only">Loading...</span> - </div> - </div> - )} + {linkState === _consts.claimLinkStateType.LOADING && <LoadingSpinner />}src/components/Claim/Link/Onchain/Confirm.view.tsx (1)
Line range hint
89-108: Enhance error handling for cross-chain scenariosThe current error handling uses a generic error handler. Consider adding specific error messages for cross-chain scenarios to improve the user experience.
Example enhancement:
} catch (error) { + let errorString + if (selectedRoute) { + errorString = error instanceof Error && error.message.includes('bridge') + ? 'Cross-chain transfer failed. Please try again.' + : utils.ErrorHandler(error) + } else { + errorString = utils.ErrorHandler(error) + } setErrorState({ showError: true, - errorMessage: errorString, + errorMessage: errorString }) }src/components/Request/Pay/Views/Initial.view.tsx (2)
129-135: Consider adding type safety improvements.The
tokenSupportsXChainimplementation is logically correct, but could benefit from explicit typing:const tokenSupportsXChain = useMemo(() => { + type SquidToken = { address: string } return ( - supportedSquidChainsAndTokens[requestLinkData.chainId]?.tokens.some((token) => + supportedSquidChainsAndTokens[requestLinkData.chainId]?.tokens.some((token: SquidToken) => areTokenAddressesEqual(token.address, requestLinkData.tokenAddress) ) ?? false ) }, [requestLinkData.tokenAddress, requestLinkData.chainId, supportedSquidChainsAndTokens])
377-389: Document the new TokenSelector prop.The conditional rendering and messaging look good. However, the new prop
showOnlySquidSupportedon TokenSelector should be documented to explain its purpose and impact.Consider adding a comment above the TokenSelector to explain the prop:
+ {/* Only show tokens that are supported by Squid for cross-chain transfers */} {tokenSupportsXChain && <TokenSelector onReset={resetTokenAndChain} showOnlySquidSupported />}src/components/Claim/Link/Initial.view.tsx (2)
144-150: Consider caching optimization for token support checkThe
tokenSupportsXChainmemo's dependency array includes the entiresupportedSquidChainsAndTokensobject, which could trigger unnecessary recalculations.Consider extracting only the required chain's tokens:
const tokenSupportsXChain = useMemo(() => { + const chainTokens = supportedSquidChainsAndTokens[claimLinkData.chainId]?.tokens return ( - supportedSquidChainsAndTokens[claimLinkData.chainId]?.tokens.some((token) => + chainTokens?.some((token) => areTokenAddressesEqual(token.address, claimLinkData.tokenAddress) ) ?? false ) - }, [claimLinkData.tokenAddress, claimLinkData.chainId, supportedSquidChainsAndTokens]) + }, [claimLinkData.tokenAddress, claimLinkData.chainId, supportedSquidChainsAndTokens[claimLinkData.chainId]?.tokens])
Line range hint
179-196: Improve error handling for unsupported tokensThe error handling for unsupported tokens could be more informative to users.
Consider providing more specific error messages:
if (!tokenSupportsXChain) { setErrorState({ showError: true, - errorMessage: 'offramp unavailable', + errorMessage: `This token (${claimLinkData.tokenSymbol}) on ${chainName || 'this chain'} does not support cross-chain transfers.`, }) return }src/components/Global/TokenSelector/TokenSelector.tsx (3)
115-125: Use Next.jsImagecomponent for consistent image handlingAt line 115~, you're using a standard
<img>tag to display the chain icon. To maintain consistency and leverage Next.js image optimization features, consider replacing it with theImagecomponent fromnext/image.Apply this refactor to use the
Imagecomponent:- <img - src={supportedSquidChainsAndTokens[balance.chainId]?.chainIconURI} - className="absolute -top-1 left-3 h-4 w-4 rounded-full" - alt="logo" - onError={(_e) => { - setChainPlaceholders((prev) => ({ - ...prev, - [`${balance.address}_${balance.chainId}`]: true, - })) - }} - /> + <Image + src={supportedSquidChainsAndTokens[balance.chainId]?.chainIconURI || ''} + className="absolute -top-1 left-3 h-4 w-4 rounded-full" + alt="Chain logo" + width={16} + height={16} + unoptimized + onError={() => { + setChainPlaceholders((prev) => ({ + ...prev, + [`${balance.address}_${balance.chainId}`]: true, + })) + }} + />This ensures consistent image handling across your application and benefits from Next.js optimizations.
Line range hint
238-246: Memoize the filter function to prevent unnecessary re-rendersWhile the calculation in
byChainAndTextis simple, memoizing the function withuseCallbackcan optimize performance by preventing unnecessary re-renders of the filtered list.Apply this refactor to memoize the
byChainAndTextfunction:- const byChainAndText = ({ name, symbol, chainId }: IUserBalance): boolean => + const byChainAndText = useCallback(({ name, symbol, chainId }: IUserBalance): boolean => { ( (!hasUserChangedChain || selectedChainID === chainId) && (filterValue.length === 0 || name.toLowerCase().includes(filterValue.toLowerCase()) || symbol.toLowerCase().includes(filterValue.toLowerCase())) ) + }, [hasUserChangedChain, selectedChainID, filterValue])This can improve performance, especially with larger datasets.
30-59: Consider using a virtualization library for listsThe custom virtual scrolling implementation may not handle all edge cases efficiently. Using a proven library like
react-windowcan improve performance and reliability when displaying large lists.Install
react-window:npm install react-windowRefactor
TokenListto useFixedSizeListfromreact-window:import { FixedSizeList as List } from 'react-window'; const TokenList = ({ balances, setToken }) => { // ... return ( <List height={400} itemCount={balances.length} itemSize={56} width="100%" > {({ index, style }) => { const balance = balances[index]; return ( <div style={style} onClick={() => setToken(balance)} // ... > {/* Render token item */} </div> ); }} </List> ); };This enhances performance and user experience with minimal effort.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
src/app/actions/squid.ts(1 hunks)src/components/Claim/Claim.consts.ts(0 hunks)src/components/Claim/Claim.tsx(1 hunks)src/components/Claim/Link/Initial.view.tsx(7 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(3 hunks)src/components/Global/TokenSelector/TokenSelector.consts.ts(1 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(6 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)src/context/tokenSelector.context.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/components/Claim/Claim.consts.ts
🧰 Additional context used
📓 Learnings (2)
src/components/Global/TokenSelector/TokenSelector.tsx (2)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
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#424
File: src/components/Global/TokenSelector/TokenSelector.tsx:197-211
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Global/TokenSelector/TokenSelector.tsx`, when the calculation within functions like `byChainAndText` is not computationally expensive, it's acceptable to avoid using `useCallback` for memoization.
src/context/tokenSelector.context.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
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.
🔇 Additional comments (14)
src/app/actions/squid.ts (2)
1-7: LGTM! Imports and server directive are properly configured.
The server-side directive and imports are correctly set up for the intended functionality.
8-13: Consider the implications of using unstable_cache API.
While the caching implementation is good, there are two points to consider:
- The
unstable_cacheAPI from Next.js is experimental and might change in future releases. - The cache keys ('getSquidChains', 'getSquidTokens') could be more specific to prevent potential collisions.
Consider making these changes:
-const getSquidChainsCache = unstable_cache(async () => await getSquidChains({ isTestnet: false }), ['getSquidChains'], {
+const getSquidChainsCache = unstable_cache(async () => await getSquidChains({ isTestnet: false }), ['squid:chains:v1'], {
revalidate: 3600 * 12,
})
-const getSquidTokensCache = unstable_cache(async () => await getSquidTokens({ isTestnet: false }), ['getSquidTokens'], {
+const getSquidTokensCache = unstable_cache(async () => await getSquidTokens({ isTestnet: false }), ['squid:tokens:v1'], {
revalidate: 3600 * 12,
})✅ Verification successful
Cache key naming is safe, but consider documenting unstable API usage
The cache keys are actually well-isolated and used consistently:
getSquidChainsandgetSquidTokensare only used as cache keys insquid.ts- A separate cache key
getSquidChainsAndTokensis used for the combined data - The caching implementation forms a proper hierarchy where base cached data is used by the combined cache
While the current cache key naming is safe from collisions, the use of unstable_cache should be documented as it's an experimental Next.js API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other uses of these cache keys
rg -l "getSquidChains|getSquidTokens" --type ts
Length of output: 112
Script:
#!/bin/bash
# Check the usage context of these functions in both files
rg "getSquidChains|getSquidTokens" -C 3 --type ts
Length of output: 3249
src/context/tokenSelector.context.tsx (4)
7-8: LGTM! Imports are correctly structured.
The new imports for Squid interfaces and actions align with the PR objectives to enhance token selector functionality.
30-30: LGTM! Context interface is properly extended.
The new supportedSquidChainsAndTokens property is well-typed and initialized appropriately.
158-158: LGTM! Context value is properly updated.
The new supportedSquidChainsAndTokens value is correctly added to the provider.
8-8: Verify caching implementation in getSquidChainsAndTokens.
The PR objectives mention caching Squid tokens for 12 hours. Let's verify the implementation:
✅ Verification successful
Caching is correctly implemented with 12-hour revalidation
The verification confirms that getSquidChainsAndTokens is wrapped with unstable_cache and configured with a 12-hour revalidation period (revalidate: 3600 * 12), which aligns with the PR objectives for caching Squid tokens.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if getSquidChainsAndTokens implements 12-hour caching
# Search for caching implementation
rg -A 10 "getSquidChainsAndTokens" src/app/actions/squid.ts
# Look for cache-related code
rg -l "cache|persist|store" src/app/actions/
Length of output: 1201
src/components/Claim/Claim.tsx (1)
Line range hint 17-45: Well-structured state management
The state declarations are well-organized and the removal of crossChainDetails aligns with the PR's objective of simplifying token selection and management.
src/components/Claim/Link/Onchain/Confirm.view.tsx (2)
32-34: LGTM: Context usage is well-structured
The addition of supportedSquidChainsAndTokens from the token selector context aligns with the PR objectives and follows React best practices.
Line range hint 109-124: Review commented file type detection logic
There's commented-out code for file type detection that might affect attachment display functionality. Please verify if this code should be removed or implemented.
src/components/Request/Pay/Views/Initial.view.tsx (1)
Line range hint 141-171: Verify error handling for Squid-specific scenarios.
The error handling in estimateTxFee looks good for general cross-chain cases, but let's verify it handles Squid-specific edge cases.
✅ Verification successful
Let me analyze the error handling patterns further by checking for specific error constants and error handling patterns in the codebase.
Based on the analysis of the error handling patterns in the codebase, I can now generate the final response.
Error handling for Squid integration is robust and properly implemented.
The error handling in the code follows consistent patterns and covers Squid-specific scenarios:
- Proper error handling for route finding with
ERR_NO_ROUTEconstant - Comprehensive error state management with
setErrorState - Appropriate cleanup of state variables on error (slippage, unsigned transactions, fees)
- Graceful fallback behavior with user-friendly error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Squid-specific error handling patterns in the codebase
# Look for Squid-related error handling patterns
rg -A 3 "squid|Squid" --type ts
Length of output: 22470
Script:
#!/bin/bash
# Look for error constants and error handling patterns in the codebase
rg -A 3 "ERR_NO_ROUTE|setErrorState|catch.*error" --type ts
Length of output: 77556
src/components/Claim/Link/Initial.view.tsx (2)
81-81: LGTM: Context usage aligns with PR objectives
The addition of supportedSquidChainsAndTokens from context aligns with the PR's goal of utilizing Squid's supported token list.
436-436: Verify chain name consistency
The code uses different sources for chain names: supportedPeanutChains for source chain and supportedSquidChainsAndTokens for destination chain.
Let's verify the chain name consistency:
Also applies to: 481-495
✅ Verification successful
Let me gather more information about how these chain names are used and defined.
Let me check one more thing to understand how these chain names are mapped or validated between Peanut and Squid.
Based on the gathered information, I can now provide a final response:
Chain name handling is properly implemented and consistent
The code correctly handles chain names between Peanut and Squid implementations:
- For source chains, it uses
supportedPeanutChains.find(chain => chain.chainId === chainId)?.namewhich ensures chain ID-based lookup - For destination chains, it uses
supportedSquidChainsAndTokens[chainId]?.axelarChainNamewhich is appropriate for Squid/Axelar integration - The code includes proper validation and mapping through
getBridgeChainNamefunction - Chain IDs are consistently used as the source of truth for lookups in both cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential chain name mismatches between Peanut and Squid implementations
ast-grep --pattern 'supportedPeanutChains.find((chain) => chain.chainId === $_.chainId)?.name'
# Check Squid chain name usage
ast-grep --pattern 'supportedSquidChainsAndTokens[$_]?.axelarChainName'
Length of output: 1617
Script:
#!/bin/bash
# Check the definition and usage of supportedPeanutChains
ast-grep --pattern 'const supportedPeanutChains = $_'
# Check the definition and usage of supportedSquidChainsAndTokens
ast-grep --pattern 'const supportedSquidChainsAndTokens = $_'
# Check imports of these constants
rg "import.*supportedPeanutChains|import.*supportedSquidChainsAndTokens" -A 2
Length of output: 1071
Script:
#!/bin/bash
# Check for any chain name mapping or validation logic
rg "chainName|chainId" src/components/Claim/Link/Initial.view.tsx -B 2 -A 2
# Check the constants file for chain definitions
fd "constants.ts" --exec cat {}
Length of output: 3391
src/components/Global/TokenSelector/TokenSelector.tsx (2)
350-356: Verify TypeScript types for chainsToDisplay
At lines 350~ to 356~, ensure that the chainsToDisplay prop passed to ChainSelector has the correct type, especially after filtering supportedPeanutChains.
Confirm that the filtered supportedPeanutChains array matches the expected type for ChainSelector. If necessary, adjust the types or provide type assertions to prevent type errors.
170-175: 🛠️ Refactor suggestion
Avoid setTimeout for state synchronization
Using setTimeout to synchronize state after closing the modal can lead to timing issues and race conditions. Instead, utilize the useEffect hook to reset state based on the visible state.
Apply this refactor to reset state when the modal visibility changes:
function setToken(balance: IUserBalance): void {
setSelectedChainID(balance.chainId)
setSelectedTokenAddress(balance.address)
setVisible(false)
- setTimeout(() => {
- setFilterValue('')
- setUserChangedChain(false)
- }, 200) // the modal takes 200ms to close (duration-200 on leave)
}
+ useEffect(() => {
+ if (!visible) {
+ setFilterValue('')
+ setUserChangedChain(false)
+ }
+ }, [visible])This approach ensures state is reset immediately after the modal closes, improving reliability.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/app/actions/squid.ts (1)
19-40: Add input validation and improve cache key naming.The concurrent API calls and type-safe data processing are well implemented. However, there are a few improvements that could make the function more robust.
Consider these enhancements:
export const getSquidChainsAndTokens = unstable_cache( async (): Promise<Record<string, interfaces.ISquidChain & { tokens: interfaces.ISquidToken[] }>> => { + if (process.env.NODE_ENV === 'development') { + console.debug('Fetching Squid chains and tokens...') + } + const [chains, tokens] = await Promise.all([getSquidChainsCache(), getSquidTokensCache()]) + if (!chains?.length || !tokens?.length) { + throw new Error('Failed to fetch Squid data: Empty chains or tokens list') + } + const chainsById = chains .filter(supportedByPeanut) .reduce<Record<string, interfaces.ISquidChain & { tokens: interfaces.ISquidToken[] }>>((acc, chain) => { acc[chain.chainId] = { ...chain, tokens: [] } return acc }, {}) + if (Object.keys(chainsById).length === 0) { + console.warn('No supported chains found in Squid data') + } + tokens.forEach((token) => { if (token.active && token.chainId in chainsById) { chainsById[token.chainId].tokens.push(token) } }) return chainsById }, - ['getSquidChainsAndTokens'], + ['squid:chains-and-tokens:v1'], { revalidate: 3600 * 12 } )src/components/Claim/Link/Onchain/Confirm.view.tsx (1)
210-218: Consider optimizing chain information lookupThe same chain information is looked up multiple times. Consider storing the chain name in a variable to avoid repeated object property access.
+ const destinationChainName = supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName; // ... - {supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName} + {destinationChainName} <MoreInfo text={`You are bridging ${claimLinkData.tokenSymbol.toLowerCase()} on ${ consts.supportedPeanutChains.find( (chain) => chain.chainId === selectedRoute.route.params.fromChain )?.name - } to ${selectedRoute.route.estimate.toToken.symbol.toLowerCase()} on ${ - supportedSquidChainsAndTokens[selectedRoute.route.params.toChain] - ?.axelarChainName + } to ${selectedRoute.route.estimate.toToken.symbol.toLowerCase()} on ${ + destinationChainName }.`} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/actions/squid.ts(1 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(3 hunks)
🔇 Additional comments (4)
src/app/actions/squid.ts (2)
15-18: LGTM! Well-structured helper function.
The supportedByPeanut function is well-implemented with clear logic and efficient chain filtering.
1-40: Verify implementation against PR objectives.
The implementation aligns well with the PR objectives, particularly:
- Fetching Squid-supported tokens
- Implementing 12-hour caching
- Supporting lazy loading of token data
Let's verify the token selector integration:
✅ Verification successful
Implementation successfully integrates with token selector and meets objectives
The verification confirms that:
- The
getSquidChainsAndTokensfunction is properly integrated into the token selector context (tokenSelector.context.tsx) - The implementation includes proper state management for chains and tokens through
supportedSquidChainsAndTokens - The data is fetched on component mount and cached for 12 hours as required
- The context provides the necessary interface for token selection and chain management
- Token data is lazily loaded through the context's useEffect hook, avoiding unnecessary fetches
The code structure effectively supports the PR objectives of populating the token selector with Squid-supported tokens while maintaining efficient caching and lazy loading patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token selector component integration
# Check if the token selector component imports this function
rg -l "getSquidChainsAndTokens"
# Look for lazy loading implementation in token selector
rg "loading=('lazy'|\"lazy\")" --type tsx
Length of output: 170
Script:
#!/bin/bash
# Check token selector context implementation
cat src/context/tokenSelector.context.tsx
# Look for lazy loading with correct file extension
rg "loading=('lazy'|\"lazy\")" -t ts -t tsx
# Check if there are any other files using the token selector
rg -l "useTokenSelector"
Length of output: 7464
src/components/Claim/Link/Onchain/Confirm.view.tsx (2)
32-34: LGTM: Context usage aligns with PR objectives
The addition of supportedSquidChainsAndTokens from the token selector context is consistent with the PR's goal of utilizing Squid-supported tokens for the selector functionality.
174-174: Previous review comment still applies
The optional chaining usage for accessing axelarChainName is consistent with previous review recommendations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sentry.edge.config.ts (2)
15-21: Optimize header removal while maintaining securityGood security practice to remove sensitive headers. However, the current implementation uses the
deleteoperator which can impact performance.Consider using undefined assignment instead:
beforeSend(event) { if (event.request?.headers) { - delete event.request.headers['Authorization'] - delete event.request.headers['api-key'] + event.request.headers['Authorization'] = undefined + event.request.headers['api-key'] = undefined } return event },🧰 Tools
🪛 Biome
[error] 17-17: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
14-22: Consider adding specific error monitoring for token selectorSince this PR introduces new token selector functionality with Squid integration and caching, consider adding specific error monitoring.
Suggestions for monitoring:
- Track failed Squid token fetching attempts
- Monitor cache hits/misses for token list
- Track token selector rendering errors
- Monitor cross-chain support validation errors
Would you like assistance in implementing these monitoring points?
🧰 Tools
🪛 Biome
[error] 17-17: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sentry.edge.config.ts(1 hunks)
🧰 Additional context used
🪛 Biome
sentry.edge.config.ts
[error] 17-17: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
|
Added fix for balance not showing https://www.notion.so/peanutprotocol/Bug-Can-get-into-bad-balance-state-on-tokenSelector-12e83811757980febfdbf6a9b62b46ba?pvs=4 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Global/TokenSelector/TokenSelector.tsx (4)
30-49: Consider enhancing virtual scrolling implementation.The virtual scrolling implementation is good, but could be improved:
- Define
ITEM_HEIGHTas a named constant outside the component- Consider making buffer size configurable
- Add ResizeObserver to handle container size changes
+ const TOKEN_LIST_ITEM_HEIGHT = 56; + const BUFFER_SIZE = 3; const TokenList = ({ balances, setToken }: { balances: IUserBalance[]; setToken: (address: IUserBalance) => void }) => { - const ITEM_HEIGHT = 56 + const ITEM_HEIGHT = TOKEN_LIST_ITEM_HEIGHT // ... rest of the code const handleScroll = () => { if (!containerRef.current) return const element = containerRef.current const scrollTop = element.scrollTop const viewportHeight = element.clientHeight const start = Math.floor(scrollTop / ITEM_HEIGHT) const visibleCount = Math.ceil(viewportHeight / ITEM_HEIGHT) - const end = Math.min(start + visibleCount + 3, balances.length) + const end = Math.min(start + visibleCount + BUFFER_SIZE, balances.length) setVisibleRange({ - start: Math.max(0, start - 3), + start: Math.max(0, start - BUFFER_SIZE), end, }) }
115-125: Use Next.js Image component for chain icons.For consistency and better performance, use the Next.js Image component for chain icons as well.
- <img - src={supportedSquidChainsAndTokens[balance.chainId]?.chainIconURI} - className="absolute -top-1 left-3 h-4 w-4 rounded-full" - alt="logo" - onError={(_e) => { - setChainPlaceholders((prev) => ({ - ...prev, - [`${balance.address}_${balance.chainId}`]: true, - })) - }} - /> + <Image + src={supportedSquidChainsAndTokens[balance.chainId]?.chainIconURI} + className="absolute -top-1 left-3 h-4 w-4 rounded-full" + alt={`${supportedSquidChainsAndTokens[balance.chainId]?.axelarChainName ?? 'Chain'} logo`} + width={16} + height={16} + unoptimized + onError={(_e) => { + setChainPlaceholders((prev) => ({ + ...prev, + [`${balance.address}_${balance.chainId}`]: true, + })) + }} + />
Line range hint
266-279: Simplify and improve selectedBalance update logic.The current implementation could be more robust and easier to maintain.
useEffect(() => { - if (selectedBalance) return - - if (_balancesToDisplay.length > 0) { - setSelectedBalance( - _balancesToDisplay.find( - (balance) => - areTokenAddressesEqual(balance.address, selectedTokenAddress) && - balance.chainId === selectedChainID - ) - ) - } else { - setSelectedBalance(undefined) - } + const matchingBalance = _balancesToDisplay.find( + (balance) => + areTokenAddressesEqual(balance.address, selectedTokenAddress) && + balance.chainId === selectedChainID + ); + + if (!selectedBalance || + !areTokenAddressesEqual(selectedBalance.address, selectedTokenAddress) || + selectedBalance.chainId !== selectedChainID) { + setSelectedBalance(matchingBalance ?? undefined); + } }, [_balancesToDisplay, selectedTokenAddress, selectedChainID])This change:
- Removes the early return which could prevent necessary updates
- Simplifies the logic by combining conditions
- Updates selectedBalance when chain or token address changes
353-359: Simplify chain filtering logic.The nested ternary and filter conditions can be simplified for better readability.
- chainsToDisplay={ - showOnlySquidSupported - ? supportedPeanutChains.filter( - (chain) => !!supportedSquidChainsAndTokens[chain.chainId] - ) - : supportedPeanutChains - } + chainsToDisplay={supportedPeanutChains.filter( + (chain) => !showOnlySquidSupported || !!supportedSquidChainsAndTokens[chain.chainId] + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Global/TokenSelector/TokenSelector.tsx(7 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Global/TokenSelector/TokenSelector.tsx (2)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#424
File: src/components/Global/TokenSelector/TokenSelector.tsx:197-211
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Global/TokenSelector/TokenSelector.tsx`, when the calculation within functions like `byChainAndText` is not computationally expensive, it's acceptable to avoid using `useCallback` for memoization.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
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.
| @@ -0,0 +1,40 @@ | |||
| 'use server' | |||
|
|
|||
| import { getSquidChains, getSquidTokens } from '@squirrel-labs/peanut-sdk' | |||
There was a problem hiding this comment.
Thought: we might want to integrate across.to or a tron bridge in the future. Might want to add an abstraction layer here (ie not SquidChains, but X-chain supported chains)
| import { IToken, IUserBalance } from '@/interfaces' | ||
| import { IUserBalance, IToken } from '@/interfaces' |
There was a problem hiding this comment.
lol? how did this happen? Automated formatting?
Smoother transition, make search bar sticky
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
src/components/Global/ConfirmDetails/Index.tsx (1)
4-9: Props interface changes improve component reusabilityThe updated interface with direct token and chain information makes the component more modular and reduces coupling to external token/chain lookup logic.
This change follows good architectural principles by:
- Reducing component coupling
- Making the component more reusable
- Simplifying the data flow
src/components/Global/ChainSelector/index.tsx (2)
102-115: Improve code readability and performanceThe chain items implementation could be improved in two ways:
- Simplify the nested valuePerChain logic
- Consider memoizing the chain items to prevent unnecessary re-renders
+const getChainValue = (chain: Chain, valuePerChain: any[], showValue: boolean) => { + if (!showValue) return undefined; + return valuePerChain.find((value) => value.chainId === chain.chainId)?.valuePerChain; +}; +const memoizedChainItems = useMemo(() => + _chainsToDisplay.map( (chain) => chain.mainnet && chainItem({ chain, setChain: () => setChain(chain.chainId), - valuePerChain: !chainsToDisplay - ? valuePerChain.find((value) => value.chainId === chain.chainId) - ?.valuePerChain - : undefined, + valuePerChain: getChainValue(chain, valuePerChain, !chainsToDisplay), }) - ) + ), + [_chainsToDisplay, chainsToDisplay, valuePerChain] +);
Line range hint
136-144: Fix key prop placementThe key prop should be placed before other props to avoid React warnings and ensure proper component reconciliation.
<Menu.Item + key={chain.name} as="button" onClick={setChain} className="flex h-12 w-full items-center justify-between gap-2 px-4 text-sm font-bold transition-colors last:mb-0 hover:bg-n-3/10 dark:hover:bg-white/20" - key={chain.name} >src/components/Global/TokenSelector/Components/AdvancedButton.tsx (1)
76-88: Consider adding request cancellation for token symbol fetches.While the mounted flag prevents state updates after unmount, rapid changes to
selectedTokenAddressorselectedChainIDcould lead to out-of-order responses. Consider using AbortController to cancel pending requests.useEffect(() => { let isMounted = true + const controller = new AbortController() if (!tokenSymbol) { - fetchTokenSymbol(selectedTokenAddress, selectedChainID).then((symbol) => { + fetchTokenSymbol(selectedTokenAddress, selectedChainID, controller.signal).then((symbol) => { if (isMounted) { _setTokenSymbol(symbol) } }) } return () => { isMounted = false + controller.abort() } }, [tokenSymbol, selectedTokenAddress, selectedChainID])src/hooks/useBalance.tsx (2)
24-31: Add cleanup and optimize dependenciesThe disconnection handling is well-implemented, but could be enhanced with:
- A cleanup function to cancel pending operations
- Adding
refetchBalancesto the dependency arrayConsider updating the effect:
useEffect(() => { if (!isConnected) { setBalances([]) } else { refetchBalances() } + return () => { + // Cleanup any pending operations + } -}, [isConnected]) +}, [isConnected, refetchBalances])
Line range hint
120-127: Consider externalizing chain ID mappingsThe hardcoded chain ID mappings could be moved to a configuration file for better maintainability and reusability.
Consider:
- Moving the chain ID mappings to a separate configuration file
- Adding TypeScript types for the chain IDs
- Implementing a mapping utility function
Example structure:
// chainConfig.ts export const CHAIN_ID_MAPPINGS = { '8508132': '534352', // Base Sepolia -> Base '81032': '81457', // Shibuya -> Astar '59160': '59144' // Linea testnet -> Linea } as const; // utils/chains.ts export const mapChainId = (chainId: string): string => CHAIN_ID_MAPPINGS[chainId] || chainId;src/components/Create/Link/Confirm.view.tsx (1)
207-208: Add error handling for sharing operations.The email and SMS sharing operations should include error handling to manage potential failures gracefully.
- if (createType === 'email_link') shareToEmail(recipient.name ?? '', link[0], usdValue) - if (createType === 'sms_link') shareToSms(recipient.name ?? '', link[0], usdValue) + try { + if (createType === 'email_link') await shareToEmail(recipient.name ?? '', link[0], usdValue) + if (createType === 'sms_link') await shareToSms(recipient.name ?? '', link[0], usdValue) + } catch (error) { + console.error('Failed to share link:', error) + // Consider showing a user-friendly notification that sharing failed + // but the link was still created successfully + }src/components/Global/TokenSelector/TokenSelector.tsx (4)
30-49: Enhance virtual scrolling implementationThe virtual scrolling implementation is good but could be improved for better maintainability and flexibility.
Consider these improvements:
+const TOKEN_LIST_ITEM_HEIGHT = 56 +const BUFFER_SIZE = 3 + const TokenList = ({ balances, setToken }: { balances: IUserBalance[]; setToken: (address: IUserBalance) => void }) => { const containerRef = useRef<HTMLDivElement>(null) const [visibleRange, setVisibleRange] = useState({ start: 0, end: 10 }) - const ITEM_HEIGHT = 56 const handleScroll = () => { if (!containerRef.current) return const element = containerRef.current const scrollTop = element.scrollTop const viewportHeight = element.clientHeight - const start = Math.floor(scrollTop / ITEM_HEIGHT) - const visibleCount = Math.ceil(viewportHeight / ITEM_HEIGHT) - const end = Math.min(start + visibleCount + 3, balances.length) + const start = Math.floor(scrollTop / TOKEN_LIST_ITEM_HEIGHT) + const visibleCount = Math.ceil(viewportHeight / TOKEN_LIST_ITEM_HEIGHT) + const end = Math.min(start + visibleCount + BUFFER_SIZE, balances.length) setVisibleRange({ - start: Math.max(0, start - 3), + start: Math.max(0, start - BUFFER_SIZE), end, }) }Also consider adding a resize observer to handle viewport changes:
useEffect(() => { const element = containerRef.current if (element) { const resizeObserver = new ResizeObserver(handleScroll) resizeObserver.observe(element) return () => resizeObserver.disconnect() } }, [])
116-120: Simplify chain icon URL resolutionThe current nested ternary operation for resolving chain icons is hard to read and maintain.
Consider extracting the logic into a helper function:
+const getChainIconUrl = (chainId: number, supportedSquidChainsAndTokens: any, supportedPeanutChains: any) => { + const squidChain = supportedSquidChainsAndTokens[chainId] + if (squidChain?.chainIconURI) return squidChain.chainIconURI + + const peanutChain = supportedPeanutChains.find((c) => c.chainId === chainId) + return peanutChain?.icon.url +} + -src={ - supportedSquidChainsAndTokens[balance.chainId]?.chainIconURI ?? - supportedPeanutChains.find((c) => c.chainId === balance.chainId)?.icon - .url -} +src={getChainIconUrl(balance.chainId, supportedSquidChainsAndTokens, supportedPeanutChains)}
292-309: Simplify chain display logicThe chain resolution logic is duplicated and could be simplified using a common interface.
Consider this refactor:
+interface DisplayChain { + name: string; + iconURI: string; + chainId: number; +} + +const getDisplayedChain = ( + chainId: number | undefined, + supportedSquidChainsAndTokens: Record<number, any>, + supportedPeanutChains: any[] +): DisplayChain | undefined => { + if (!chainId) return undefined + + const squidChain = supportedSquidChainsAndTokens[chainId] + if (squidChain) { + return { + name: squidChain.axelarChainName, + iconURI: squidChain.chainIconURI, + chainId: squidChain.chainId, + } + } + + const peanutChain = supportedPeanutChains.find((chain) => chain.chainId === chainId) + return peanutChain ? { + name: peanutChain.name, + iconURI: peanutChain.icon.url, + chainId: peanutChain.chainId, + } : undefined +} + -const displayedChain = useMemo(() => { - if (!selectedChainID) return undefined - if (supportedSquidChainsAndTokens[selectedChainID]) { - const chain = supportedSquidChainsAndTokens[selectedChainID] - return { - name: chain.axelarChainName, - iconURI: chain.chainIconURI, - chainId: chain.chainId, - } - } else { - const chain = supportedPeanutChains.find((chain) => chain.chainId === selectedChainID) - return { - name: chain?.name, - iconURI: chain?.icon.url, - chainId: chain?.chainId, - } - } -}, [selectedChainID, supportedSquidChainsAndTokens]) +const displayedChain = useMemo( + () => getDisplayedChain(selectedChainID, supportedSquidChainsAndTokens, supportedPeanutChains), + [selectedChainID, supportedSquidChainsAndTokens] +)
378-384: Simplify chain filtering logicThe chain filtering logic could be more readable and maintainable.
Consider extracting the filtering logic:
+const getDisplayableChains = ( + showOnlySquidSupported: boolean, + supportedPeanutChains: any[], + supportedSquidChainsAndTokens: Record<number, any> +) => { + if (!showOnlySquidSupported) return supportedPeanutChains + return supportedPeanutChains.filter((chain) => !!supportedSquidChainsAndTokens[chain.chainId]) +} + -chainsToDisplay={ - showOnlySquidSupported - ? supportedPeanutChains.filter( - (chain) => !!supportedSquidChainsAndTokens[chain.chainId] - ) - : supportedPeanutChains -} +chainsToDisplay={getDisplayableChains( + showOnlySquidSupported, + supportedPeanutChains, + supportedSquidChainsAndTokens +)}src/components/Claim/Link/Initial.view.tsx (2)
434-454: Consider adding loading state for token support checkThe token selector implementation looks good, but there's no loading state while checking token support. This could lead to a flash of content if the
supportedSquidChainsAndTokensdata is still loading.Consider adding a loading state:
{recipientType !== 'iban' && recipientType !== 'us' ? ( + supportedSquidChainsAndTokens[claimLinkData.chainId] ? ( tokenSupportsXChain ? ( <TokenSelector shouldBeConnected={false} showOnlySquidSupported onReset={() => { setSelectedChainID(claimLinkData.chainId) setSelectedTokenAddress(claimLinkData.tokenAddress) }} /> ) : ( <label className="text-h7 font-light"> This token does not support cross-chain claims... </label> ) + ) : ( + <div className="text-h7 font-light">Loading token support...</div> + ) ) : null}
620-621: Consider adding error handling for reset operationThe reset operation in the error state looks good, but it might benefit from error handling to ensure the chain ID and token address are valid.
Add validation:
-setSelectedChainID(claimLinkData.chainId) -setSelectedTokenAddress(claimLinkData.tokenAddress) +if (claimLinkData?.chainId && claimLinkData?.tokenAddress) { + setSelectedChainID(claimLinkData.chainId) + setSelectedTokenAddress(claimLinkData.tokenAddress) +} else { + console.error('Invalid claim link data for reset') + setErrorState({ + showError: true, + errorMessage: 'Unable to reset token selection', + }) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/Claim/Generic/AlreadyClaimed.view.tsx(0 hunks)src/components/Claim/Link/Initial.view.tsx(8 hunks)src/components/Create/Link/Confirm.view.tsx(10 hunks)src/components/Global/ChainSelector/index.tsx(3 hunks)src/components/Global/ConfirmDetails/Index.tsx(2 hunks)src/components/Global/TokenSelector/Components/AdvancedButton.tsx(5 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(7 hunks)src/hooks/useBalance.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/Claim/Generic/AlreadyClaimed.view.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Global/TokenSelector/TokenSelector.tsx (2)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
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#424
File: src/components/Global/TokenSelector/TokenSelector.tsx:197-211
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Global/TokenSelector/TokenSelector.tsx`, when the calculation within functions like `byChainAndText` is not computationally expensive, it's acceptable to avoid using `useCallback` for memoization.
🔇 Additional comments (13)
src/components/Global/ConfirmDetails/Index.tsx (1)
Line range hint 1-53: Overall implementation looks good
The component is well-structured, type-safe, and implements the required functionality cleanly. The changes align well with the PR objectives of improving token selector functionality.
src/components/Global/ChainSelector/index.tsx (1)
82-89: LGTM! Well-implemented transition effects
The transition implementation provides smooth animation with appropriate timing for both enter and leave states.
src/components/Global/TokenSelector/Components/AdvancedButton.tsx (3)
43-46: LGTM! Good handling of wallet connection state.
The addition of isConnected from useAccount hook and proper state initialization improves the component's ability to handle disconnected wallet states.
65-74: LGTM! Proper handling of balance updates.
The effect hook correctly manages balance states:
- Sets balance to undefined when disconnected
- Only fetches balance when necessary
- Includes all required dependencies
128-128: LGTM! Clean UI state handling.
The UI updates properly handle all states:
- Uses managed token symbol state
- Correctly displays loading state when fetching balance
- Properly handles disconnected state
Also applies to: 132-140
src/hooks/useBalance.tsx (1)
14-14: LGTM: Clean addition of connection status tracking
The addition of isConnected from useAccount hook is well-integrated and follows React hooks best practices.
src/components/Create/Link/Confirm.view.tsx (4)
7-17: LGTM! Well-organized utility imports.
The new utility imports are properly structured and align with the component's responsibilities for token handling, storage management, and error handling.
Line range hint 157-189: LGTM! Well-structured storage operations.
The local storage operations are properly implemented with comprehensive data storage for both direct sends and created links. The peanut preferences update maintains user settings for future use.
Also applies to: 211-215
Line range hint 220-224: LGTM! Proper error handling implementation.
The error handling using the ErrorHandler utility is well-implemented with clear error state management.
236-237: LGTM! Clean and consistent UI updates.
The UI changes for displaying addresses, token/chain information, and transaction costs are well-implemented and maintain consistency throughout the component.
Also applies to: 250-253, 307-307, 311-311
src/components/Global/TokenSelector/TokenSelector.tsx (1)
Line range hint 1-400: Overall implementation looks good!
The TokenSelector component successfully implements the required functionality with proper error handling, virtual scrolling, and cross-chain support. The suggested improvements focus on code organization and maintainability rather than functional issues.
src/components/Claim/Link/Initial.view.tsx (2)
145-151: Good implementation of token support check using useMemo
The memoization of tokenSupportsXChain is a good performance optimization since it only recomputes when the dependencies change.
Line range hint 493-507: Verify chain name fallback handling
The code assumes supportedSquidChainsAndTokens[chainId]?.axelarChainName will always be available. Consider adding a fallback chain name.
Let's verify the chain name availability:
Add a fallback:
-supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName
+supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName ?? 'Unknown Chain'| {chainIconUri ? ( | ||
| <img src={chainIconUri} className="h-6 w-6" /> | ||
| ) : ( | ||
| <Icon name="chain_placeholder" className="h-6 w-6" fill="#999" /> | ||
| )} | ||
| <label className="text-sm font-bold text-gray-1">{chainName}</label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and lazy loading to chain image
The chain icon image needs the same improvements as the token icon.
Apply these improvements:
-<img src={chainIconUri} className="h-6 w-6" />
+<img
+ src={chainIconUri}
+ className="h-6 w-6"
+ alt={`${chainName} chain icon`}
+ loading="lazy"
+/>📝 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.
| {chainIconUri ? ( | |
| <img src={chainIconUri} className="h-6 w-6" /> | |
| ) : ( | |
| <Icon name="chain_placeholder" className="h-6 w-6" fill="#999" /> | |
| )} | |
| <label className="text-sm font-bold text-gray-1">{chainName}</label> | |
| {chainIconUri ? ( | |
| <img | |
| src={chainIconUri} | |
| className="h-6 w-6" | |
| alt={`${chainName} chain icon`} | |
| loading="lazy" | |
| /> | |
| ) : ( | |
| <Icon name="chain_placeholder" className="h-6 w-6" fill="#999" /> | |
| )} | |
| <label className="text-sm font-bold text-gray-1">{chainName}</label> |
| {tokenIconUri ? ( | ||
| <img src={tokenIconUri} className="h-6 w-6" /> | ||
| ) : ( | ||
| <Icon name="token_placeholder" className="h-6 w-6" fill="#999" /> | ||
| )} | ||
| <label className="text-h5 sm:text-h3"> | ||
| {formatTokenAmount(Number(tokenAmount))}{' '} | ||
| {data | ||
| ? data | ||
| .find((chain: any) => chain.chainId === selectedChainID) | ||
| ?.tokens.find((token: any) => | ||
| areTokenAddressesEqual(token.address, selectedTokenAddress) | ||
| )?.symbol | ||
| : getTokenSymbol(selectedTokenAddress, selectedChainID)} | ||
| {formatTokenAmount(Number(tokenAmount))} {tokenSymbol} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and lazy loading to image
While the fallback logic is good, the image tag needs accessibility improvements and lazy loading as mentioned in the PR objectives.
Apply these improvements:
-<img src={tokenIconUri} className="h-6 w-6" />
+<img
+ src={tokenIconUri}
+ className="h-6 w-6"
+ alt={`${tokenSymbol} token icon`}
+ loading="lazy"
+/>📝 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.
| {tokenIconUri ? ( | |
| <img src={tokenIconUri} className="h-6 w-6" /> | |
| ) : ( | |
| <Icon name="token_placeholder" className="h-6 w-6" fill="#999" /> | |
| )} | |
| <label className="text-h5 sm:text-h3"> | |
| {formatTokenAmount(Number(tokenAmount))}{' '} | |
| {data | |
| ? data | |
| .find((chain: any) => chain.chainId === selectedChainID) | |
| ?.tokens.find((token: any) => | |
| areTokenAddressesEqual(token.address, selectedTokenAddress) | |
| )?.symbol | |
| : getTokenSymbol(selectedTokenAddress, selectedChainID)} | |
| {formatTokenAmount(Number(tokenAmount))} {tokenSymbol} | |
| {tokenIconUri ? ( | |
| <img | |
| src={tokenIconUri} | |
| className="h-6 w-6" | |
| alt={`${tokenSymbol} token icon`} | |
| loading="lazy" | |
| /> | |
| ) : ( | |
| <Icon name="token_placeholder" className="h-6 w-6" fill="#999" /> | |
| )} | |
| <label className="text-h5 sm:text-h3"> | |
| {formatTokenAmount(Number(tokenAmount))} {tokenSymbol} |
| src={_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.icon.url} | ||
| alt={''} | ||
| className="h-6 w-6" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add descriptive alt text for accessibility
The chain icon's alt text is empty, which impacts screen reader accessibility. Consider adding a descriptive alt text that includes the chain name.
<img
src={_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.icon.url}
- alt={''}
+ alt={`${_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.name} chain icon`}
className="h-6 w-6"
/>📝 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.
| src={_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.icon.url} | |
| alt={''} | |
| className="h-6 w-6" | |
| /> | |
| src={_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.icon.url} | |
| alt={`${_chainsToDisplay.find((chain) => chain.chainId === selectedChainID)?.name} chain icon`} | |
| className="h-6 w-6" | |
| /> |
| <div className="sticky top-0 bg-white p-2 dark:bg-n-1"> | ||
| <Search | ||
| className="px-1" | ||
| placeholder="Search by chain name" | ||
| value={filterValue} | ||
| onChange={(e: any) => setFilterValue(e.target.value)} | ||
| onSubmit={() => {}} | ||
| medium | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Fix type safety and remove unused prop
- The Search component accepts
onChangeandonSubmitprops withanytype, butonSubmitis never used in the implementation - The
onChangehandler should use the proper React input event type - Since
onSubmitis unused, it should be removed from the component props
type SearchProps = {
className?: string
placeholder: string
value: string
- onChange: any
- onSubmit: any
+ onChange: (e: React.ChangeEvent<HTMLInputElement>) => void
large?: boolean
medium?: boolean
border?: boolean
}
-const Search = ({ className, placeholder, value, onChange, onSubmit, large, medium, border }: SearchProps) => {
+const Search = ({ className, placeholder, value, onChange, large, medium, border }: SearchProps) => {🔗 Analysis chain
Improve type safety and verify empty handler
Two potential improvements in the Search component implementation:
- The event type is set to 'any' which bypasses TypeScript's type checking
- There's an empty onSubmit handler which might need implementation
<Search
className="px-1"
placeholder="Search by chain name"
value={filterValue}
- onChange={(e: any) => setFilterValue(e.target.value)}
+ onChange={(e: React.ChangeEvent<HTMLInputElement>) => setFilterValue(e.target.value)}
onSubmit={() => {}}
medium
/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Search component's onSubmit is used elsewhere
rg "onSubmit.*=.*{.*}" --type typescript
Length of output: 156
Script:
#!/bin/bash
# Check if Search component's onSubmit is used elsewhere with proper escaping
rg "onSubmit\s*=\s*\{" --type typescript
# Also check the Search component's implementation
ast-grep --pattern 'interface $_ {
$$$
onSubmit$_
$$$
}'
Length of output: 142
Script:
#!/bin/bash
# Check if Search component's onSubmit is used elsewhere with proper escaping
rg "onSubmit\s*=\s*\{" -t ts -t tsx
# Find Search component definition
fd "Search.(ts|tsx)" --exec cat {}
Length of output: 104
Script:
#!/bin/bash
# Search for files containing Search component
fd -e ts -e tsx | xargs rg "onSubmit.*=" -l
# Find Search component definition
find . -type f -name "Search.ts" -o -name "Search.tsx" -o -name "search.ts" -o -name "search.tsx" | xargs cat
Length of output: 800
Script:
#!/bin/bash
# Check the Search component implementation
cat src/components/Global/Search/index.tsx
# Check how onSubmit is used in other components for context
rg "onSubmit.*=" -A 2 src/components/Global/Search/index.tsx
Length of output: 2071
| const selectedChain = useMemo(() => { | ||
| if (supportedSquidChainsAndTokens[selectedChainID]) { | ||
| const chain = supportedSquidChainsAndTokens[selectedChainID] | ||
| return { | ||
| name: chain.axelarChainName, | ||
| iconUri: chain.chainIconURI, | ||
| } | ||
| } else { | ||
| const chain = supportedPeanutChains.find((chain) => chain.chainId === selectedChainID) | ||
| return { | ||
| name: chain?.name, | ||
| iconUri: chain?.icon.url, | ||
| } | ||
| } | ||
| }, [supportedSquidChainsAndTokens, selectedChainID]) | ||
|
|
||
| const selectedToken = useMemo(() => { | ||
| if (supportedSquidChainsAndTokens[selectedChainID]) { | ||
| const chain = supportedSquidChainsAndTokens[selectedChainID] | ||
| const token = chain.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | ||
| return { | ||
| symbol: token?.symbol, | ||
| iconUri: token?.logoURI, | ||
| } | ||
| } else { | ||
| const token = peanutTokenDetails | ||
| .find((tokenDetails) => tokenDetails.chainId === selectedChainID) | ||
| ?.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | ||
| return { | ||
| symbol: token?.symbol, | ||
| iconUri: token?.logoURI, | ||
| } | ||
| } | ||
| }, [selectedChainID, selectedTokenAddress, supportedSquidChainsAndTokens]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and type safety to prevent potential runtime errors.
The memoization logic for selectedChain and selectedToken could encounter undefined values in edge cases. Consider adding proper null checks and type safety:
const selectedChain = useMemo(() => {
if (supportedSquidChainsAndTokens[selectedChainID]) {
const chain = supportedSquidChainsAndTokens[selectedChainID]
return {
- name: chain.axelarChainName,
- iconUri: chain.chainIconURI,
+ name: chain?.axelarChainName ?? 'Unknown Chain',
+ iconUri: chain?.chainIconURI ?? '',
}
} else {
const chain = supportedPeanutChains.find((chain) => chain.chainId === selectedChainID)
return {
- name: chain?.name,
- iconUri: chain?.icon.url,
+ name: chain?.name ?? 'Unknown Chain',
+ iconUri: chain?.icon?.url ?? '',
}
}
}, [supportedSquidChainsAndTokens, selectedChainID])
const selectedToken = useMemo(() => {
+ const defaultToken = {
+ symbol: 'Unknown Token',
+ iconUri: '',
+ }
if (supportedSquidChainsAndTokens[selectedChainID]) {
const chain = supportedSquidChainsAndTokens[selectedChainID]
const token = chain.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress))
- return {
- symbol: token?.symbol,
- iconUri: token?.logoURI,
- }
+ return token ? {
+ symbol: token.symbol ?? defaultToken.symbol,
+ iconUri: token.logoURI ?? defaultToken.iconUri,
+ } : defaultToken
} else {
const token = peanutTokenDetails
.find((tokenDetails) => tokenDetails.chainId === selectedChainID)
?.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress))
- return {
- symbol: token?.symbol,
- iconUri: token?.logoURI,
- }
+ return token ? {
+ symbol: token.symbol ?? defaultToken.symbol,
+ iconUri: token.logoURI ?? defaultToken.iconUri,
+ } : defaultToken
}
}, [selectedChainID, selectedTokenAddress, supportedSquidChainsAndTokens])📝 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 selectedChain = useMemo(() => { | |
| if (supportedSquidChainsAndTokens[selectedChainID]) { | |
| const chain = supportedSquidChainsAndTokens[selectedChainID] | |
| return { | |
| name: chain.axelarChainName, | |
| iconUri: chain.chainIconURI, | |
| } | |
| } else { | |
| const chain = supportedPeanutChains.find((chain) => chain.chainId === selectedChainID) | |
| return { | |
| name: chain?.name, | |
| iconUri: chain?.icon.url, | |
| } | |
| } | |
| }, [supportedSquidChainsAndTokens, selectedChainID]) | |
| const selectedToken = useMemo(() => { | |
| if (supportedSquidChainsAndTokens[selectedChainID]) { | |
| const chain = supportedSquidChainsAndTokens[selectedChainID] | |
| const token = chain.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | |
| return { | |
| symbol: token?.symbol, | |
| iconUri: token?.logoURI, | |
| } | |
| } else { | |
| const token = peanutTokenDetails | |
| .find((tokenDetails) => tokenDetails.chainId === selectedChainID) | |
| ?.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | |
| return { | |
| symbol: token?.symbol, | |
| iconUri: token?.logoURI, | |
| } | |
| } | |
| }, [selectedChainID, selectedTokenAddress, supportedSquidChainsAndTokens]) | |
| const selectedChain = useMemo(() => { | |
| if (supportedSquidChainsAndTokens[selectedChainID]) { | |
| const chain = supportedSquidChainsAndTokens[selectedChainID] | |
| return { | |
| name: chain?.axelarChainName ?? 'Unknown Chain', | |
| iconUri: chain?.chainIconURI ?? '', | |
| } | |
| } else { | |
| const chain = supportedPeanutChains.find((chain) => chain.chainId === selectedChainID) | |
| return { | |
| name: chain?.name ?? 'Unknown Chain', | |
| iconUri: chain?.icon?.url ?? '', | |
| } | |
| } | |
| }, [supportedSquidChainsAndTokens, selectedChainID]) | |
| const selectedToken = useMemo(() => { | |
| const defaultToken = { | |
| symbol: 'Unknown Token', | |
| iconUri: '', | |
| } | |
| if (supportedSquidChainsAndTokens[selectedChainID]) { | |
| const chain = supportedSquidChainsAndTokens[selectedChainID] | |
| const token = chain.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | |
| return token ? { | |
| symbol: token.symbol ?? defaultToken.symbol, | |
| iconUri: token.logoURI ?? defaultToken.iconUri, | |
| } : defaultToken | |
| } else { | |
| const token = peanutTokenDetails | |
| .find((tokenDetails) => tokenDetails.chainId === selectedChainID) | |
| ?.tokens.find((token) => areTokenAddressesEqual(token.address, selectedTokenAddress)) | |
| return token ? { | |
| symbol: token.symbol ?? defaultToken.symbol, | |
| iconUri: token.logoURI ?? defaultToken.iconUri, | |
| } : defaultToken | |
| } | |
| }, [selectedChainID, selectedTokenAddress, supportedSquidChainsAndTokens]) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
129-135: Consider adding error boundary for token support checkThe token support check logic is well-implemented with proper memoization and null safety. However, consider adding error handling for malformed token addresses to prevent potential runtime errors.
const tokenSupportsXChain = useMemo(() => { + try { return ( supportedSquidChainsAndTokens[requestLinkData.chainId]?.tokens.some((token) => areTokenAddressesEqual(token.address, requestLinkData.tokenAddress) ) ?? false ) + } catch (error) { + console.error('Error checking token support:', error) + return false + } }, [requestLinkData.tokenAddress, requestLinkData.chainId, supportedSquidChainsAndTokens])
377-387: Improve JSX indentation for better readabilityThe conditional messaging logic is clear and user-friendly, but the JSX indentation could be improved.
- {tokenSupportsXChain ? ( - <label className="text-h9 font-light"> - You can fulfill this payment request with any token on any chain. Pick the token and chain that - you want to fulfill this request with. - </label> - ) : ( - <label className="text-h9 font-light"> - This token does not support cross-chain transfers. You can only fulfill this payment request - with the selected token on the selected chain. - </label> - )} + {tokenSupportsXChain ? ( + <label className="text-h9 font-light"> + You can fulfill this payment request with any token on any chain. + Pick the token and chain that you want to fulfill this request with. + </label> + ) : ( + <label className="text-h9 font-light"> + This token does not support cross-chain transfers. + You can only fulfill this payment request with the selected token + on the selected chain. + </label> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Global/TokenSelector/Components/AdvancedButton.tsx(5 hunks)src/components/Request/Pay/Views/Initial.view.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Global/TokenSelector/Components/AdvancedButton.tsx
🔇 Additional comments (3)
src/components/Request/Pay/Views/Initial.view.tsx (3)
83-83: LGTM: Context integration looks good
The addition of supportedSquidChainsAndTokens from context aligns with the PR's objective of integrating Squid-supported tokens.
283-284: LGTM: Network switching logic is well-implemented
The chain validation and switching logic is correctly implemented with proper error handling through the parent function's try-catch block.
389-389: LGTM: TokenSelector integration is well-implemented
The conditional rendering of TokenSelector with the new showOnlySquidSupported prop aligns perfectly with the PR's objective of utilizing Squid-supported tokens.
Main focus
Instead of relaying only on our sdk list of supported chain and tokens, the token selector is now populated mainly from the supported tokens from squid, in the flows of link and request creation we also use the sdk list because we want users to be able to create those even if those wont be able to be fulfilled crosschain
Other changes
Should also fix