Copilot commented on code in PR #322:
URL:
https://github.com/apache/kvrocks-controller/pull/322#discussion_r2174092078
##########
webui/src/app/ui/formDialog.tsx:
##########
@@ -101,7 +103,9 @@ const FormDialog: React.FC<FormDialogProps> = ({
return (
<>
- {position === "card" ? (
+ {children ? (
+ <div onClick={openDialog}>{children}</div>
Review Comment:
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.
```suggestion
<button type="button" onClick={openDialog} style={{ all:
"unset", cursor: "pointer" }}>
{children}
</button>
```
##########
webui/src/app/namespaces/[namespace]/page.tsx:
##########
@@ -19,10 +19,23 @@
"use client";
-import { Box, Typography, Chip } from "@mui/material";
+import {
Review Comment:
The import 'listShards' is not used anywhere in the file. Removing unused
imports can improve code maintainability.
##########
webui/src/app/namespaces/[namespace]/page.tsx:
##########
@@ -49,15 +125,95 @@ export default function Namespace({ params }: { params: {
namespace: string } })
}
const clusters = await fetchClusters(params.namespace);
+ let totalShards = 0;
+ let totalNodes = 0;
+
const data = await Promise.all(
- clusters.map((cluster) =>
- fetchCluster(params.namespace, cluster).catch((error)
=> {
+ clusters.map(async (cluster) => {
+ try {
+ const clusterInfo = await
fetchCluster(params.namespace, cluster);
+ if (
+ clusterInfo &&
+ typeof clusterInfo === "object" &&
+ "shards" in clusterInfo
+ ) {
+ const shards = (clusterInfo as any).shards ||
[];
+
+ let clusterNodeCount = 0;
+ 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
+ );
+ }
+ }
Review Comment:
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.
```suggestion
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
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
