-
Notifications
You must be signed in to change notification settings - Fork 12
fixed - supported token not loading #41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fixed - supported token not loading #41
Conversation
📝 WalkthroughWalkthroughTwo frontend token-selection files are modified: TokenPicker.jsx adds contextual error messages by parsing error strings and referencing chain names; useTokenList.js ensures consistent UI state by clearing error and loading flags when returning cached tokens. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
@kumawatkaran523 please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/TokenPicker.jsx (1)
318-348: Consider using error types instead of string matching.The current approach uses
.includes()to parse error message strings, which tightly couples the UI to the exact wording of error messages fromuseTokenList. This makes the code fragile and harder to maintain.Consider refactoring to use structured error objects or error codes:
// In useTokenList.js, return structured errors: setError({ type: 'TESTNET_NOT_SUPPORTED', message: '...', chainId }); setError({ type: 'CHAIN_NOT_SUPPORTED', message: '...', chainId }); // In TokenPicker.jsx, check error type: {tokensError.type === 'TESTNET_NOT_SUPPORTED' ? ( // testnet UI ) : tokensError.type === 'CHAIN_NOT_SUPPORTED' ? ( // unsupported chain UI ) : ( // generic error UI )}This approach is more maintainable, supports i18n better, and decouples UI logic from error message text.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/TokenPicker.jsxfrontend/src/hooks/useTokenList.js
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/TokenPicker.jsx (1)
frontend/src/hooks/useTokenList.js (2)
ChainIdToName(12-16)ChainIdToName(12-16)
🔇 Additional comments (1)
frontend/src/hooks/useTokenList.js (1)
28-29: Good fix for UI state consistency.Explicitly clearing the error and loading states when returning cached tokens prevents stale UI states from persisting. This ensures the component consuming this hook receives consistent state regardless of whether data comes from cache or a fresh fetch.
|
@Arnav-Nehra, What I think that opening the token search only on Mainnet will be a good idea. It reduces confusion for users who are connected to testnets or other chains, where the token list is often empty or not relevant. What I Recommend :
Pros
What you think ?? |
|
Yes, I think this is a better approach.
|
Tokens were already loading on Mainnet but they were not working on Testnet so Added proper error logging for better UX
fixes #31
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.