feat: add interactive Volcano dashboard experience#240
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive migration of the Volcano Dashboard from a React/Vite/Express setup to a Next.js application. Key changes include the implementation of a new dashboard layout using DashboardShell, the migration of API routes to Next.js route handlers, and the introduction of authentication guards and SSO support. Additionally, the PR adds sample Kubernetes manifests for cluster validation and updates the Docker configuration. My review highlights several areas for improvement, primarily regarding the incorrect use of await on non-promise objects in API routes, the need for better type safety in API requests, and recommendations to refactor legacy code and improve user notification patterns by replacing alert() with non-blocking UI components.
| export const runtime = "nodejs"; | ||
|
|
||
| export const GET = withRead(async (request, context) => { | ||
| const { namespace, name } = await context.params; |
| export const runtime = "nodejs"; | ||
|
|
||
| export const GET = withRead(async (request, context) => { | ||
| const { namespace, name } = await context.params; |
| export const runtime = "nodejs"; | ||
|
|
||
| export const GET = withRead(async (request, context) => { | ||
| const { name } = await context.params; |
| }); | ||
|
|
||
| export const DELETE = withWrite(async (request, context) => { | ||
| const { name } = await context.params; |
| }); | ||
|
|
||
| export const PATCH = withWrite(async (request, context) => { | ||
| const { name } = await context.params; |
| const handleCreateJob = async (newJob) => { | ||
| try { | ||
| await createJob(newJob); | ||
| alert("Job created successfully!"); |
| mb: 2, | ||
| }} | ||
| > | ||
| <Box sx={{ flex: 1 }}> |
| const fetchQueues = useCallback(async () => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const params = { | ||
| search: searchText, | ||
| state: "All", | ||
| queue: filters.queue, | ||
| page: pagination.page, | ||
| limit: pagination.rowsPerPage, | ||
| }; | ||
|
|
||
| const response = await axios.get(`${API_BASE}/queues`, { params }); | ||
| setQueues(response.data.items || []); | ||
| setTotalQueues(response.data.totalCount || 0); | ||
| } catch (err) { | ||
| setError("Failed to fetch queues: " + err.message); | ||
| setQueues([]); | ||
| setTotalQueues(0); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [filters.queue, pagination.page, pagination.rowsPerPage, searchText]); |
There was a problem hiding this comment.
This component uses axios directly within a useCallback and useEffect for data fetching. This is inconsistent with other new components in the PR that leverage @tanstack/react-query. Refactoring this to use useQuery would provide benefits like caching, automatic refetching, and a consistent data-fetching pattern across the application.
| import React, { useState } from "react"; | ||
| import { Link, Outlet, useLocation } from "react-router-dom"; | ||
| import { | ||
| AppBar, | ||
| Box, | ||
| Drawer, | ||
| IconButton, | ||
| List, | ||
| ListItem, | ||
| ListItemButton, | ||
| ListItemIcon, | ||
| ListItemText, | ||
| Toolbar, | ||
| Typography, | ||
| Tooltip, | ||
| } from "@mui/material"; | ||
| import AccountCircleOutlinedIcon from "@mui/icons-material/AccountCircleOutlined"; | ||
| import DnsOutlinedIcon from "@mui/icons-material/DnsOutlined"; | ||
| import DescriptionOutlinedIcon from "@mui/icons-material/DescriptionOutlined"; | ||
| import DeviceHubOutlinedIcon from "@mui/icons-material/DeviceHubOutlined"; | ||
| import ExpandLessIcon from "@mui/icons-material/ExpandLess"; | ||
| import ExpandMoreIcon from "@mui/icons-material/ExpandMore"; | ||
| import EventRepeatOutlinedIcon from "@mui/icons-material/EventRepeatOutlined"; | ||
| import HomeOutlinedIcon from "@mui/icons-material/HomeOutlined"; | ||
| import Inventory2OutlinedIcon from "@mui/icons-material/Inventory2Outlined"; | ||
| import LogoutOutlinedIcon from "@mui/icons-material/LogoutOutlined"; | ||
| import MailOutlineIcon from "@mui/icons-material/MailOutline"; | ||
| import MenuIcon from "@mui/icons-material/Menu"; | ||
| import TuneOutlinedIcon from "@mui/icons-material/TuneOutlined"; | ||
| import WorkOutlineOutlinedIcon from "@mui/icons-material/WorkOutlineOutlined"; | ||
|
|
||
| const Layout = ({ onLogout = () => {} }) => { | ||
| // Hooks must be used inside component functions | ||
| const location = useLocation(); | ||
| const [open, setOpen] = useState(true); | ||
| const [adminOpen, setAdminOpen] = useState(false); | ||
|
|
||
| // constants can be kept outside the component | ||
| const headerGrey = "#424242"; // dark gray top stripe | ||
| const drawerWidth = 280; | ||
| const collapsedDrawerWidth = 60; | ||
| const iconProps = { sx: { fontSize: 18 } }; | ||
|
|
||
| const handleDrawerToggle = () => { | ||
| setOpen(!open); | ||
| }; | ||
|
|
||
| const handleAdminToggle = () => { | ||
| setAdminOpen((previous) => !previous); | ||
| }; | ||
|
|
||
| const menuSections = [ | ||
| { | ||
| items: [ | ||
| { | ||
| text: "Overview", | ||
| icon: <HomeOutlinedIcon {...iconProps} />, | ||
| path: "/dashboard", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| title: "Scheduling", | ||
| items: [ | ||
| { | ||
| text: "Queues", | ||
| icon: <DeviceHubOutlinedIcon {...iconProps} />, | ||
| path: "/scheduling/queues", | ||
| }, | ||
| { | ||
| text: "Jobs", | ||
| icon: <WorkOutlineOutlinedIcon {...iconProps} />, | ||
| path: "/scheduling/jobs", | ||
| }, | ||
| { | ||
| text: "CronJob", | ||
| icon: <EventRepeatOutlinedIcon {...iconProps} />, | ||
| path: "/scheduling/cronjobs", | ||
| }, | ||
| { | ||
| text: "Pod Groups", | ||
| icon: <DeviceHubOutlinedIcon {...iconProps} />, | ||
| path: "/scheduling/podgroups", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| title: "Workloads", | ||
| items: [ | ||
| { | ||
| text: "Pods", | ||
| icon: <Inventory2OutlinedIcon {...iconProps} />, | ||
| path: "/workload/pods", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| title: "System", | ||
| items: [ | ||
| { | ||
| text: "Configuration", | ||
| icon: <TuneOutlinedIcon {...iconProps} />, | ||
| path: "/system/configuration", | ||
| }, | ||
| { | ||
| text: "Cluster Information", | ||
| icon: <DnsOutlinedIcon {...iconProps} />, | ||
| path: "/system/cluster-information", | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| const footerItems = [ | ||
| { | ||
| text: "Documentation", | ||
| icon: <DescriptionOutlinedIcon {...iconProps} />, | ||
| path: "/documentation", | ||
| }, | ||
| ]; | ||
|
|
||
| const renderMenuItem = (item) => { | ||
| const active = | ||
| item.path && | ||
| (location.pathname === item.path || | ||
| location.pathname.startsWith(`${item.path}/`)); | ||
| const itemStyles = { | ||
| minHeight: 38, | ||
| mx: open ? 1.25 : 0.75, | ||
| my: 0.25, | ||
| px: open ? 1.25 : 1, | ||
| borderRadius: "6px", | ||
| color: item.disabled ? "text.disabled" : "text.primary", | ||
| justifyContent: open ? "flex-start" : "center", | ||
| "&.active": { | ||
| bgcolor: "rgba(0, 0, 0, 0.08)", | ||
| "& .MuiListItemIcon-root": { | ||
| color: "text.primary", | ||
| }, | ||
| "& .MuiListItemText-primary": { | ||
| color: "text.primary", | ||
| fontWeight: 600, | ||
| }, | ||
| }, | ||
| "&:hover": { | ||
| backgroundColor: item.disabled | ||
| ? "transparent" | ||
| : "rgba(0, 0, 0, 0.08)", | ||
| }, | ||
| }; | ||
|
|
||
| const content = item.disabled ? ( | ||
| <ListItem | ||
| key={item.text} | ||
| aria-disabled="true" | ||
| sx={{ | ||
| ...itemStyles, | ||
| cursor: "not-allowed", | ||
| opacity: 0.58, | ||
| }} | ||
| > | ||
| <ListItemIcon | ||
| sx={{ | ||
| minWidth: open ? 34 : 0, | ||
| color: "inherit", | ||
| justifyContent: "center", | ||
| }} | ||
| > | ||
| {item.icon} | ||
| </ListItemIcon> | ||
| {open && ( | ||
| <ListItemText | ||
| primary={item.text} | ||
| primaryTypographyProps={{ fontSize: 13 }} | ||
| /> | ||
| )} | ||
| </ListItem> | ||
| ) : ( | ||
| <ListItemButton | ||
| key={item.text} | ||
| component={Link} | ||
| to={item.path} | ||
| className={active ? "active" : ""} | ||
| sx={itemStyles} | ||
| > | ||
| <ListItemIcon | ||
| sx={{ | ||
| minWidth: open ? 34 : 0, | ||
| color: "inherit", | ||
| justifyContent: "center", | ||
| }} | ||
| > | ||
| {item.icon} | ||
| </ListItemIcon> | ||
| {open && ( | ||
| <ListItemText | ||
| primary={item.text} | ||
| primaryTypographyProps={{ fontSize: 13 }} | ||
| /> | ||
| )} | ||
| </ListItemButton> | ||
| ); | ||
|
|
||
| return !open ? ( | ||
| <Tooltip key={item.text} title={item.text} placement="right"> | ||
| {content} | ||
| </Tooltip> | ||
| ) : ( | ||
| <React.Fragment key={item.text}>{content}</React.Fragment> | ||
| ); | ||
| }; | ||
|
|
||
| return ( | ||
| <Box sx={{ display: "flex", minHeight: "100vh" }}> | ||
| <AppBar | ||
| position="fixed" | ||
| sx={{ | ||
| zIndex: (theme) => theme.zIndex.drawer + 1, | ||
| bgcolor: headerGrey, | ||
| }} | ||
| > | ||
| <Toolbar> | ||
| <IconButton | ||
| color="inherit" | ||
| aria-label="toggle drawer" | ||
| onClick={handleDrawerToggle} | ||
| edge="start" | ||
| sx={{ mr: 2, color: "white" }} | ||
| > | ||
| <MenuIcon sx={{ fontSize: 20 }} /> | ||
| </IconButton> | ||
| <Typography | ||
| variant="h6" | ||
| noWrap | ||
| component="div" | ||
| sx={{ | ||
| color: "#ffffff", | ||
| fontWeight: 500, | ||
| }} | ||
| > | ||
| Volcano Dashboard | ||
| </Typography> | ||
| </Toolbar> | ||
| </AppBar> | ||
|
|
||
| <Drawer | ||
| data-testid="sidebar-drawer" | ||
| variant="permanent" | ||
| sx={{ | ||
| width: open ? drawerWidth : collapsedDrawerWidth, | ||
| flexShrink: 0, | ||
| [`& .MuiDrawer-paper`]: { | ||
| width: open ? drawerWidth : collapsedDrawerWidth, | ||
| boxSizing: "border-box", | ||
| backgroundColor: "#f5f5f5", | ||
| transition: "width 0.2s", | ||
| overflowX: "hidden", | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| }, | ||
| }} | ||
| > | ||
| <Toolbar /> | ||
| <Box sx={{ overflow: "hidden auto", flexGrow: 1 }}> | ||
| {menuSections.map((section, sectionIndex) => ( | ||
| <Box | ||
| key={section.title || "primary"} | ||
| sx={{ | ||
| borderTop: | ||
| sectionIndex === 0 | ||
| ? "none" | ||
| : "1px solid rgba(0, 0, 0, 0.08)", | ||
| pt: sectionIndex === 0 ? 1 : 1.25, | ||
| mt: sectionIndex === 0 ? 0 : 0.75, | ||
| }} | ||
| > | ||
| {open && section.title && ( | ||
| <Typography | ||
| variant="caption" | ||
| sx={{ | ||
| color: "text.secondary", | ||
| display: "block", | ||
| fontSize: 11, | ||
| fontWeight: 600, | ||
| letterSpacing: "0.03em", | ||
| px: 2, | ||
| pb: 0.5, | ||
| textTransform: "uppercase", | ||
| }} | ||
| > | ||
| {section.title} | ||
| </Typography> | ||
| )} | ||
| <List dense disablePadding> | ||
| {section.items.map(renderMenuItem)} | ||
| </List> | ||
| </Box> | ||
| ))} | ||
| </Box> | ||
| <Box | ||
| sx={{ | ||
| borderTop: "1px solid rgba(0, 0, 0, 0.08)", | ||
| px: 0, | ||
| py: 1, | ||
| display: "flex", | ||
| flexDirection: "column", | ||
| gap: 1, | ||
| mt: "auto", | ||
| }} | ||
| > | ||
| <List dense disablePadding> | ||
| {footerItems.map(renderMenuItem)} | ||
| </List> | ||
| <ListItemButton | ||
| aria-expanded={adminOpen} | ||
| aria-label="Toggle admin menu" | ||
| onClick={handleAdminToggle} | ||
| sx={{ | ||
| alignItems: "center", | ||
| borderRadius: "6px", | ||
| display: "flex", | ||
| gap: 1.25, | ||
| justifyContent: open ? "space-between" : "center", | ||
| minHeight: 34, | ||
| mx: open ? 1.25 : 0.75, | ||
| px: open ? 1.25 : 1, | ||
| "&:hover": { | ||
| backgroundColor: "rgba(0, 0, 0, 0.08)", | ||
| }, | ||
| }} | ||
| > | ||
| <Box | ||
| sx={{ | ||
| alignItems: "center", | ||
| display: "flex", | ||
| gap: 1, | ||
| }} | ||
| > | ||
| <AccountCircleOutlinedIcon {...iconProps} /> | ||
| {open && ( | ||
| <Typography sx={{ fontSize: 13 }}> | ||
| admin | ||
| </Typography> | ||
| )} | ||
| </Box> | ||
| {open && | ||
| (adminOpen ? ( | ||
| <ExpandLessIcon {...iconProps} /> | ||
| ) : ( | ||
| <ExpandMoreIcon {...iconProps} /> | ||
| ))} | ||
| </ListItemButton> | ||
| {open && adminOpen && ( | ||
| <Box | ||
| sx={{ | ||
| bgcolor: "#ffffff", | ||
| border: "1px solid rgba(0, 0, 0, 0.12)", | ||
| borderRadius: 1, | ||
| boxShadow: "0 4px 12px rgba(0, 0, 0, 0.12)", | ||
| mx: 0.5, | ||
| overflow: "hidden", | ||
| }} | ||
| > | ||
| <Box | ||
| sx={{ | ||
| alignItems: "center", | ||
| borderBottom: | ||
| "1px solid rgba(0, 0, 0, 0.08)", | ||
| display: "flex", | ||
| gap: 1, | ||
| px: 1.5, | ||
| py: 1.25, | ||
| }} | ||
| > | ||
| <MailOutlineIcon {...iconProps} /> | ||
| <Box> | ||
| <Typography sx={{ fontSize: 13 }}> | ||
| admin@example.com | ||
| </Typography> | ||
| <Typography | ||
| color="text.secondary" | ||
| sx={{ fontSize: 11 }} | ||
| > | ||
| Cluster administrator | ||
| </Typography> | ||
| </Box> | ||
| </Box> | ||
| <ListItemButton | ||
| aria-label="logout" | ||
| onClick={onLogout} | ||
| sx={{ | ||
| gap: 1, | ||
| minHeight: 40, | ||
| px: 1.5, | ||
| }} | ||
| > | ||
| <LogoutOutlinedIcon {...iconProps} /> | ||
| <Typography sx={{ fontSize: 13 }}> | ||
| Logout | ||
| </Typography> | ||
| </ListItemButton> | ||
| </Box> | ||
| )} | ||
| <img | ||
| src="/volcano-icon-color.svg" | ||
| alt="Volcano Logo" | ||
| style={{ | ||
| alignSelf: "center", | ||
| maxWidth: open ? "120px" : "36px", | ||
| height: "auto", | ||
| transition: "max-width 0.2s", | ||
| }} | ||
| /> | ||
| </Box> | ||
| </Drawer> | ||
| <Box | ||
| component="main" | ||
| sx={{ | ||
| flexGrow: 1, | ||
| p: 3, | ||
| backgroundColor: "#ffffff", | ||
| minWidth: 0, | ||
| }} | ||
| > | ||
| <Toolbar /> | ||
| <Outlet /> | ||
| </Box> | ||
| </Box> | ||
| ); | ||
| }; | ||
|
|
||
| export default Layout; |
There was a problem hiding this comment.
This Layout component appears to be legacy code from a previous setup using react-router-dom. The new application architecture is based on Next.js and uses its built-in layout system (e.g., app/layout.tsx, DashboardShell.tsx). This file is likely unused and should be removed to avoid confusion and code rot.
| alert("Failed to create queue: " + response.statusText); | ||
| return; | ||
| } | ||
|
|
||
| setCreateDialogOpen(false); | ||
| fetchQueues(); | ||
| } catch (err) { | ||
| alert( | ||
| "Network error: " + (err?.response?.data?.error || err.message), | ||
| ); |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
5c2c1c8 to
09043a5
Compare
|
Thank you @Sn0rt Could you show it on the weeeky community meeting to help us understand the changes better |
Great! I'll explain the core changes in this PR at the weekly community meeting focus on the following topic:
|
de6p
left a comment
There was a problem hiding this comment.
We already have Next.js implemented, so why would we need to implement it again? Also, please review the latest dashboard—we've added several enhancements, including tRPC and Next.js improvements.
Please create a separate issue for authentication. Could you also explain why authentication is needed?
|
|
@de6p Hi deep, @Sn0rt shares his dashboard on the meeting, which is great. I think we can continue with your style choices, but for functionality, there are some good ideas we can add, such as displaying resource usage for each pod, hierarchical queues, login, etc. I think we could differentiate this PR from Shivam and work on both in parallel. I've also asked @Sn0rt to revise the style based on your code. |
de6p
left a comment
There was a problem hiding this comment.
@Sn0rt , the changes you've made look good and promising. Could you first rebase this branch with the latest codebase?
It would also be helpful if you could open issues for the changes you plan to make to the existing resource pods, queues, etc. Tracking those changes through issues will make them easier to manage and follow.
Additionally, I'd appreciate it if you could create separate PRs for these changes. That would make the review process much easier for me.
Those are the only requests from my side. Thanks!
Thanks @de6p, that makes sense. Considering the current size and coupling of this PR, my preferred approach is:
I understand that separate PRs would make review easier. However, splitting this PR now would require extracting and removing a lot of already-coupled functionality, which would be a significant amount of extra work. So I think the practical trade-off is between these two options:
Given the current workload, I prefer the first option. I’ll start by rebasing on the latest |
0cf6612 to
5075eee
Compare
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
5075eee to
4a7890c
Compare
|
/ok-to-test |
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
Signed-off-by: Sn0rt <wangguohao.2009@gmail.com>
8d238a2 to
1f3c899
Compare
Summary
This PR submits a major update to Volcano Dashboard for review.
It modernizes the dashboard into a unified Next.js + TypeScript application, replaces the previous tRPC/monorepo direction with explicit REST APIs and a root app layout, and significantly improves the Volcano-specific scheduling, workload, queue, authentication, and deployment experience.
Try it
A preview image can be deployed with the Helm chart for testing:
Breaking Changes
A breaking change means existing code, deployment scripts, CI workflows, image references, or developer workflows may need to be updated after this PR is merged.
This PR includes the following breaking changes:
Remove the monorepo layout
apps/webandpackages/*workspace structure.Remove tRPC
packages/trpcAPI layer./api/v1/*REST routes.Replace the old deployment manifest with Helm
helm/volcano-dashboard.Update dashboard routes
/dashboard/scheduling/queues/scheduling/jobs/scheduling/podgroups/scheduling/cronjobs/workload/pods/system/configuration/system/cluster-information/documentationEnhancements
This PR also adds several functional and user-experience improvements:
Improved dashboard shell and navigation
Queue hierarchy experience
REST API coverage
Resource detail workflow
Authentication and access control
Workload and scheduling management
Deployment and release
Tests and validation
Validation
The following checks are expected to pass:
npm run routes:auditnpm run buildnpm testVolcano.Dashboard.May.2.9.37.PM.webm