[TASK-6438] feat: avoid loading tokens that wont be used#814
[TASK-6438] feat: avoid loading tokens that wont be used#814kushagrasarathe merged 2 commits intopeanut-wallet-devfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
## Walkthrough
The changes focus on optimizing the `TokenSelector` component in the codebase. The `TokenList` subcomponent is now wrapped with React's `memo` to prevent unnecessary re-renders. The `setToken` handler is refactored to use `useCallback` for stable reference identity. Logic for clearing filters and user chain change state is shifted to a `useEffect` that triggers after the modal closes, synchronized with the modal's close animation. Token and chain selector rendering is now conditional based on the `shouldBeConnected` state, and the memoization logic for the token list is updated accordingly. Additionally, the `AdvancedTokenSelectorButton` component was enhanced to manage its token logo URI dynamically via state and effect hooks.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------|
| src/components/Global/TokenSelector/TokenSelector.tsx | - Wrapped `TokenList` with `memo` for render optimization.<br>- Refactored `setToken` to use `useCallback`.<br>- Moved filter and state clearing to a `useEffect` triggered by modal visibility.<br>- Updated token list memoization and rendering logic to consider `shouldBeConnected`.<br>- Made `ChainSelector` rendering conditional.<br>- Simplified `onClose` handler. |
| src/components/Global/TokenSelector/Components/AdvancedButton.tsx | - Managed token logo URI as state `_tokenLogoUri` initialized from prop.<br>- Added `useEffect` to update `_tokenLogoUri` dynamically by calling `getTokenLogo`.<br>- Updated rendering to use `_tokenLogoUri` state instead of prop for token logo image. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant TokenSelector
participant Modal
participant TokenList
User->>TokenSelector: Open modal
TokenSelector->>Modal: Show modal
Modal->>TokenSelector: User selects token
TokenSelector->>TokenList: Pass stable setToken handler
TokenList->>TokenSelector: setToken called (memoized)
TokenSelector->>Modal: Close modal
Modal->>TokenSelector: Modal visibility changes to false
TokenSelector->>TokenSelector: useEffect clears filter and resets state after 200msPossibly related PRs
Suggested labels
Suggested reviewers
Poem
Learnt from: jjramirezn Learnt from: jjramirezn Learnt from: jjramirezn |
jjramirezn
force-pushed
the
feat/not-needed-extra-tokens
branch
from
bd1cea3 to
52ab99e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/components/Global/TokenSelector/TokenSelector.tsx (1)
4-426:⚠️ Potential issueMissing implementation for showOnlyBalanceTokens property
The
showOnlyBalanceTokensproperty added to theTokenSelectorPropsinterface in the constants file is not being used in this component. This seems to be a critical oversight given the PR objective.To properly implement this functionality, you need to:
- Add the property to the destructured props in the component signature:
const TokenSelector = ({ classNameButton, shouldBeConnected = true, showOnlySquidSupported = false, + showOnlyBalanceTokens = false, onReset, }: TokenSelectorProps) => {
- Use this property to filter tokens with balances in the
filteredBalancesuseMemo:const filteredBalances = useMemo(() => { // initially show all balances and the tokens on the current chain if (filterValue.length === 0 && !hasUserChangedChain) return _balancesToDisplay + // Filter tokens to only show those with balances if needed + const balancesFiltered = showOnlyBalanceTokens + ? _balancesToDisplay.filter(balance => parseFloat(balance.amount) > 0) + : _balancesToDisplay; const byChainAndText = ({ name, symbol, chainId }: IUserBalance): boolean => { // existing filtering logic } - return _balancesToDisplay.filter(byChainAndText) + return balancesFiltered.filter(byChainAndText) }, [_balancesToDisplay, filterValue, selectedChainID, hasUserChangedChain, showOnlyBalanceTokens])
- Update the dependency array to include the new property.
🛑 Comments failed to post (3)
src/components/Global/TokenSelector/TokenSelector.consts.ts (1)
11-11:
⚠️ Potential issueNew property added without implementation
The
showOnlyBalanceTokensproperty has been added to the interface, but I don't see it being used in the implementation (TokenSelector.tsx). While the property aligns with the PR objective of "avoiding loading tokens that won't be used," there's no filtering logic based on this flag.Consider implementing the filtering logic using this property, for example in the
filteredBalancesuseMemo:const filteredBalances = useMemo(() => { // initially show all balances and the tokens on the current chain if (filterValue.length === 0 && !hasUserChangedChain) return _balancesToDisplay + // Filter by token balance if needed + const balancesWithFilter = showOnlyBalanceTokens + ? _balancesToDisplay.filter(balance => parseFloat(balance.amount) > 0) + : _balancesToDisplay; const byChainAndText = ({ name, symbol, chainId }: IUserBalance): boolean => // rest of your filtering logic - return _balancesToDisplay.filter(byChainAndText) + return balancesWithFilter.filter(byChainAndText) }, [_balancesToDisplay, filterValue, selectedChainID, hasUserChangedChain, showOnlyBalanceTokens])📝 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 filteredBalances = useMemo(() => { // initially show all balances and the tokens on the current chain if (filterValue.length === 0 && !hasUserChangedChain) return _balancesToDisplay // Filter by token balance if needed const balancesWithFilter = showOnlyBalanceTokens ? _balancesToDisplay.filter(balance => parseFloat(balance.amount) > 0) : _balancesToDisplay const byChainAndText = ({ name, symbol, chainId }: IUserBalance): boolean => // rest of your filtering logic return balancesWithFilter.filter(byChainAndText) }, [ _balancesToDisplay, filterValue, selectedChainID, hasUserChangedChain, showOnlyBalanceTokens ])src/components/Global/TokenSelector/TokenSelector.tsx (2)
395-408: 💡 Verification agent
🧩 Analysis chain
Conditional ChainSelector rendering needs verification
Conditionally rendering the
ChainSelectorbased on!shouldBeConnectedmeans users won't be able to select a chain when they're expected to be connected. This might limit functionality for connected users who want to switch chains.Let's check if there are other places in the codebase where chain selection is handled for connected users:
Consider documenting this design decision with a more explicit comment to clarify the intent:
{!shouldBeConnected && ( + // Chain selector is hidden for connected users because they can only + // use tokens from chains where they have balances <ChainSelector // rest of the code /> )}
🏁 Script executed:
#!/bin/bash # Check for other chain selection mechanisms for connected users # Look for references to ChainSelector component usage rg -A 5 -B 5 "ChainSelector" src/components/ --type ts --type tsx # Look for chain selection logic conditions rg -A 5 "setSelectedChainID|setChainID" src/components/ --type ts --type tsxLength of output: 200
This will help us determine if connected users have an alternate chain-selection path elsewhere.
🏁 Script executed:
#!/bin/bash # Retry searching for ChainSelector usages without relying on --type flags rg -n 'ChainSelector' --glob '*.tsx' src # Search for any chain-switching logic or handlers in .tsx files rg -n 'setSelectedChainID|setChainID|switchChain|onChange.*chain' --glob '*.tsx' srcLength of output: 4899
Clarify why ChainSelector is hidden for connected users
The
ChainSelectoris only rendered when!shouldBeConnectedbecause once a user connects their wallet, the active chain is locked to their wallet’s network. Please add a comment to explain this design decision.• File: src/components/Global/TokenSelector/TokenSelector.tsx
• Lines: 395–408{!shouldBeConnected && ( + // Only show chain selector when disconnected. + // For connected users, the active chain is determined by their wallet network. <ChainSelector chainsToDisplay={ showOnlySquidSupported ? supportedPeanutChains.filter( (chain) => !!supportedSquidChainsAndTokens[chain.chainId] ) : supportedPeanutChains } onChange={(_chainId) => { setUserChangedChain(true) }} /> )}📝 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.{!shouldBeConnected && ( + // Only show chain selector when disconnected. + // For connected users, the active chain is determined by their wallet network. <ChainSelector chainsToDisplay={ showOnlySquidSupported ? supportedPeanutChains.filter( (chain) => !!supportedSquidChainsAndTokens[chain.chainId] ) : supportedPeanutChains } onChange={(_chainId) => { setUserChangedChain(true) }} /> )}
191-194: 💡 Verification agent
🧩 Analysis chain
Clarify early return behavior in selectedChainTokens
While the comment explains the intent, the implementation returns an empty array when
shouldBeConnectedis true, which seems counterintuitive. If the goal is to show only tokens with balances for connected users, returning an empty array might not be the right approach.Consider refining this logic to ensure proper token filtering:
// If we expect the user to be connected, then we just want to show the // tokens where the user has a balance - if (shouldBeConnected) return [] + // No need to load tokens without balances when user is connected + // The user's actual balances will be displayed from _balancesToDisplay + if (shouldBeConnected) return []Let's verify this behavior with the existing logic:
🏁 Script executed:
#!/bin/bash # Check how _balancesToDisplay is handling token display logic rg -A 10 "_balancesToDisplay\s*=" src/components/Global/TokenSelector/Length of output: 1091
I've requested more context around both the
selectedChainTokenscomputation and the early-return to ensure the intended logic matches what’s implemented.
🏁 Script executed:
# Show the definition and surrounding logic of selectedChainTokens rg -n "selectedChainTokens" -A10 -B10 src/components/Global/TokenSelector/TokenSelector.tsx # Show the early return with shouldBeConnected in context rg -n "if (shouldBeConnected)" -A10 -B10 src/components/Global/TokenSelector/TokenSelector.tsxLength of output: 2510
🏁 Script executed:
#!/bin/bash # Locate the definition and usages of shouldBeConnected in this file rg -n shouldBeConnected -A10 -B10 src/components/Global/TokenSelector/TokenSelector.tsxLength of output: 4955
Clarify why
selectedChainTokensreturns an empty list whenshouldBeConnectedis trueThe early return in the
selectedChainTokensmemo is intentional: when a connection is required, we don’t want to append any “chain‐only” tokens because the user’s actual balances (including any tokens they hold on that chain) are rendered via_balancesToDisplay. To make this explicit, please update the inline comment as follows:--- a/src/components/Global/TokenSelector/TokenSelector.tsx +++ b/src/components/Global/TokenSelector/TokenSelector.tsx @@ -191,4 +191,6 - // If we expect the user to be connected, then we just want to show the - // tokens where the user has a balance + // When a wallet connection is required, skip adding chain-only tokens here— + // the user’s token balances are shown through _balancesToDisplay. if (shouldBeConnected) return []This makes it clear that returning
[]is correct behavior in this context.📝 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.// When a wallet connection is required, skip adding chain-only tokens here— // the user’s token balances are shown through _balancesToDisplay. if (shouldBeConnected) return []
kushagrasarathe
left a comment
kushagrasarathe
left a comment
There was a problem hiding this comment.
@jjramirezn reviewed, one blocking qn, when connected with a wallet where i dont have any tokens, the token selector trigger is shown like this, imo should show default token/chain logos (token logo is missing in this case), and when opened it shows No balances to display! which is good, but then in send flow it also lets me move forward to confirm screen (see second screenshot)
For flows where the user needs to be connected (most of them), only show the tokens that the user has a balance in. This will improve user experience and also reduce the amount of tokens (and their logos) that will be loaded
jjramirezn
force-pushed
the
feat/not-needed-extra-tokens
branch
from
52ab99e to
9394d8b
Compare
|
After going back and forward with eliminating external wallets already, came into too many issues for the scope of this so implemented the easy fix. About the confirm, this is an existing issue I think its fixed when send flow changes |
2 participants
For flows where the user needs to be connected (most of them), only show the tokens that the user has a balance in. This will improve user experience and also reduce the amount of tokens (and their logos) that will be loaded
Summary by CodeRabbit