feat(ui): async display host status and database list breadcrumb skeleton #257
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Suspense component requires a fallback (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 4 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| import { fetchData, getClickHouseHosts } from '@/lib/clickhouse' | ||
| import { getHost } from '@/lib/utils' | ||
| import { Suspense } from 'react' |
There was a problem hiding this comment.
suggestion (performance): Consider lazy loading HostStatus component
Since you are using Suspense, it might be beneficial to lazy load the HostStatus component to improve initial load performance.
| import { Suspense } from 'react' | |
| import { Suspense, lazy } from 'react' | |
| const HostStatus = lazy(() => import('@/components/HostStatus')) |
| <span className="absolute inline-flex size-full animate-ping rounded-full bg-sky-400 opacity-75"></span> | ||
| <span className="relative inline-flex size-2 rounded-full bg-sky-500"></span> | ||
| </span> | ||
| const Online = ({ title }: { title: string }) => ( |
There was a problem hiding this comment.
suggestion (performance): Consider memoizing Online component
If the Online component is rendered frequently, consider memoizing it using React.memo to avoid unnecessary re-renders.
| const Online = ({ title }: { title: string }) => ( | |
| import React, { memo } from 'react'; | |
| const Online = memo(({ title }: { title: string }) => ( |
| <span className="relative flex size-2"> | ||
| <span className="absolute inline-flex size-full rounded-full bg-red-400"></span> | ||
| </span> | ||
| const Offline = ({ title }: { title: string }) => ( |
There was a problem hiding this comment.
suggestion (performance): Consider memoizing Offline component
If the Offline component is rendered frequently, consider memoizing it using React.memo to avoid unnecessary re-renders.
| const Offline = ({ title }: { title: string }) => ( | |
| import React, { memo } from 'react'; | |
| const Offline = memo(({ title }: { title: string }) => ( | |
| <TooltipProvider delayDuration={0}> | |
| <Tooltip> |
| @@ -55,7 +76,10 @@ export async function ClickHouseHost() { | |||
| if (hosts.length === 1) { | |||
| return ( | |||
| <div className="flex flex-row items-center gap-2"> | |||
| <HostStatus host={hosts[0]} /> | |||
| {getHost(hosts[0])} | |||
| <Suspense> | |||
There was a problem hiding this comment.
issue: Suspense component requires a fallback
The Suspense component should have a fallback prop to display a loading indicator or message while the HostStatus component is being loaded.
components/menu/menu.tsx
Outdated
| <MenuNavigationStyle className="hidden md:flex" items={items} /> | ||
| <MenuDropdownStyle className="flex md:hidden" items={items} /> | ||
| <MenuNavigationStyle | ||
| className="hidden transition md:flex" |
There was a problem hiding this comment.
suggestion: Consider specifying transition properties
The 'transition' class is added, but no specific transition properties are defined. Consider specifying which properties should transition for better clarity and control.
| className="hidden transition md:flex" | |
| className="hidden transition-all duration-300 md:flex" |
No description provided.