-
Notifications
You must be signed in to change notification settings - Fork 59
feat(webui/cluster): redesign cluster page #322
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #322 +/- ##
============================================
+ Coverage 43.38% 46.89% +3.50%
============================================
Files 37 45 +8
Lines 2971 4391 +1420
============================================
+ Hits 1289 2059 +770
- Misses 1544 2126 +582
- Partials 138 206 +68
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
Redesign the cluster page to improve UX by adding new tags, filters, and enhanced sorting while also updating form dialog components. Key changes include:
- Adding an optional children prop to the FormDialog and form creation components.
- Updating typography variants and styling to match the new design.
- Refactoring the cluster page to include new filter and sort popovers and detailed resource counts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
webui/src/app/ui/formDialog.tsx | Added support for an optional children prop and updated typography styling. |
webui/src/app/ui/formCreation.tsx | Updated ClusterCreation and ImportCluster components to accept children and wrap FormDialog accordingly. |
webui/src/app/namespaces/[namespace]/page.tsx | Overhauled the UI with new Material UI components, filters, sorting popovers, and tag displays. |
@@ -101,7 +103,9 @@ const FormDialog: React.FC<FormDialogProps> = ({ | |||
|
|||
return ( | |||
<> | |||
{position === "card" ? ( | |||
{children ? ( | |||
<div onClick={openDialog}>{children}</div> |
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.
The clickable div wrapping the children does not have inherent keyboard accessibility. Consider using a button element or adding tabindex and key press handlers to ensure the element is accessible.
<div onClick={openDialog}>{children}</div> | |
<button type="button" onClick={openDialog} style={{ all: "unset", cursor: "pointer" }}> | |
{children} | |
</button> |
Copilot uses AI. Check for mistakes.
@@ -19,10 +19,23 @@ | |||
|
|||
"use client"; | |||
|
|||
import { Box, Typography, Chip } from "@mui/material"; | |||
import { |
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.
The import 'listShards' is not used anywhere in the file. Removing unused imports can improve code maintainability.
Copilot uses AI. Check for mistakes.
for (let i = 0; i < shards.length; i++) { | ||
try { | ||
const nodes = await listNodes( | ||
params.namespace, | ||
cluster, | ||
i.toString() | ||
); | ||
if (Array.isArray(nodes)) { | ||
clusterNodeCount += nodes.length; | ||
} | ||
} catch (error) { | ||
console.error( | ||
`Failed to fetch nodes for shard ${i}:`, | ||
error | ||
); | ||
} | ||
} |
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.
The sequential fetching of nodes for each shard using a for-loop with await may be a performance bottleneck. Consider refactoring this loop using Promise.all to execute the node fetches concurrently.
for (let i = 0; i < shards.length; i++) { | |
try { | |
const nodes = await listNodes( | |
params.namespace, | |
cluster, | |
i.toString() | |
); | |
if (Array.isArray(nodes)) { | |
clusterNodeCount += nodes.length; | |
} | |
} catch (error) { | |
console.error( | |
`Failed to fetch nodes for shard ${i}:`, | |
error | |
); | |
} | |
} | |
const shardNodeCounts = await Promise.all( | |
shards.map(async (_, i) => { | |
try { | |
const nodes = await listNodes( | |
params.namespace, | |
cluster, | |
i.toString() | |
); | |
return Array.isArray(nodes) ? nodes.length : 0; | |
} catch (error) { | |
console.error( | |
`Failed to fetch nodes for shard ${i}:`, | |
error | |
); | |
return 0; | |
} | |
}) | |
); | |
const clusterNodeCount = shardNodeCounts.reduce( | |
(sum, count) => sum + count, | |
0 | |
); |
Copilot uses AI. Check for mistakes.
Thank you for your contribution! Sorry for the late merge. |
No worries at all , thank you for merging. |
i have redesigned the cluster page
added tags
🟠 Orange "Migrating {slot}"= Shows when any shard has active migration (migrating_slot >= 0)
🟢 Green "Stable" = Shows when all shardds are stable(migrating_slot = -1)
🔵 Blue"Importing {slot}" == Shows when any shard is importing(import_slot >= 0)
let me know if there is something else i can do