-
Notifications
You must be signed in to change notification settings - Fork 425
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
FE-1281 | Move pools router into local tRPC router #4005
Conversation
Moves pools router into local tRPC router for following methods: - getUserPools - getPools - getPool
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/web/components/complex/add-conc-liquidity.tsxOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces modifications across several components and hooks to change the data source for pool information from an edge API to a local API. This adjustment affects components such as 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 (
|
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: 2
🧹 Outside diff range and nitpick comments (3)
packages/web/components/complex/portfolio/user-positions.tsx (1)
Line range hint
99-112
: LGTM! Consider adding error handling for the query.The migration from edge to local router is correct. However, the query lacks error handling which could be important for user feedback.
Consider adding error handling:
const { data: allMyPoolDetails, isLoading: isLoadingMyPoolDetails } = api.local.pools.getUserPools.useQuery( { userOsmoAddress: address ?? "", }, { enabled: Boolean(address), + onError: (error) => { + console.error('Failed to fetch user pools:', error); + // Add user feedback handling here + }, trpc: { context: { skipBatch: true, }, }, } );packages/web/pages/pool/[id].tsx (1)
29-29
: LGTM! Consider enhancing error handling for better UX.The migration to local router is correct. The error handling through
isError
is good, but could be enhanced for better user experience.Consider enhancing error handling:
-const { data: pool, isError } = api.local.pools.getPool.useQuery({ poolId }); +const { data: pool, isError, error } = api.local.pools.getPool.useQuery( + { poolId }, + { + onError: (error) => { + console.error('Failed to fetch pool:', error); + // Consider showing a user-friendly error message + }, + } +);packages/web/hooks/ui-config/use-add-concentrated-liquidity-config.ts (1)
Line range hint
64-69
: Consider adding error handling for the pool query.The switch to local API looks good. However, consider adding error handling to gracefully handle cases where the pool data fetch fails.
const { data: pool, isFetched: isPoolFetched } = api.local.pools.getPool.useQuery( { poolId }, { refetchInterval: 5_000, // 5 seconds + retry: 3, + onError: (error) => { + console.error('Failed to fetch pool data:', error); + // Handle error appropriately + } } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/web/components/complex/add-conc-liquidity.tsx
(1 hunks)packages/web/components/complex/pools-table.tsx
(1 hunks)packages/web/components/complex/portfolio/user-positions.tsx
(1 hunks)packages/web/hooks/ui-config/use-add-concentrated-liquidity-config.ts
(1 hunks)packages/web/hooks/ui-config/use-historical-and-depth-data.ts
(1 hunks)packages/web/modals/add-liquidity.tsx
(1 hunks)packages/web/pages/pool/[id].tsx
(1 hunks)packages/web/server/api/local-router.ts
(2 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- packages/web/modals/add-liquidity.tsx
- packages/web/hooks/ui-config/use-historical-and-depth-data.ts
- packages/web/components/complex/pools-table.tsx
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
What is the purpose of the change:
Moves pools router into local tRPC router for following methods:
getPools
getPool
Linear Task
https://linear.app/osmosis/issue/FE-1281/move-pools-router-into-local-trpc-router
Brief Changelog
getPools
,getPool
instead of egdeTesting and Verifying
This change has been tested locally by rebuilding the website and verified content and links are expected