frontend: optimize recent cluster lookup with Map-based lookup#5686
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DashratRajpurohit 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 |
|
|
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
Can you please have a look at the git commits to see if they meet the contribution guidelines? We use a Linux kernel style of git commits. See the contributing guide for general context, and please see previous git commits with git log for examples.
Commits that need attention
⚡ Performance: Optimize cluster lookup in Recent Clusters and Chooser— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
It looks like the Contributor License Agreement (CLA) has not been signed. Kubernetes requires all contributors to sign the CLA before a PR can be merged. Could you please take a look at the EasyCLA bot comment and sign it?
About the CLA
The Kubernetes project uses the Linux Foundation EasyCLA system. Signing takes only a minute through the link in the EasyCLA bot comment. Individual contributors sign electronically; corporate contributors may need their employer to sign a Corporate CLA.
There was a problem hiding this comment.
Pull request overview
This PR aims to speed up “recent cluster” resolution in the cluster chooser and Home page by replacing repeated linear searches (Array.find) with a name→cluster lookup Map built via useMemo.
Changes:
- Build a
clustersByNamelookup (Map) from the clusters list usingReact.useMemo. - Replace
clusters.find(...)lookups withclustersByName.get(name)in recent-cluster selection logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
frontend/src/components/cluster/Chooser.tsx |
Introduces a memoized Map for cluster lookup and switches recent-cluster matching to Map.get(). |
frontend/src/components/App/Home/RecentClusters.tsx |
Mirrors the same Map-based recent-cluster lookup optimization on the Home page widget. |
Comments suppressed due to low confidence (2)
frontend/src/components/cluster/Chooser.tsx:200
React.useMemo(..., [clusters])won’t effectively memoize here if theclustersprop is a freshly created array each render (e.g. the parent builds it viaObject.values(...)). In that case the Map will still be rebuilt every render; consider memoizing the clusters array at the call site or changing the memo dependency to something stable to actually avoid recomputation.
const clustersByName = React.useMemo(
() => new Map(clusters.map(cluster => [cluster.name, cluster])),
[clusters]
);
frontend/src/components/App/Home/RecentClusters.tsx:96
useMemo(..., [clusters])will rebuild the Map whenever theclustersarray identity changes;Home/index.tsxcurrently passesObject.values(customNameClusters)inline, which creates a new array each render. To get the intended perf benefit, memoize the array at the parent or pass a stable structure so the memo can be effective.
const clustersByName = React.useMemo(
() => new Map(clusters.map(cluster => [cluster.name, cluster])),
[clusters]
);
| // If we have more than the maximum number of recent clusters allowed, we show the most | ||
| // recent ones. Otherwise, just show the clusters in the order they are received. | ||
| const clustersByName = React.useMemo( | ||
| () => new Map(clusters.map(cluster => [cluster.name, cluster])), |
| // If we have more than the maximum number of recent clusters allowed, we show the most | ||
| // recent ones. Otherwise, just show the clusters in the order they are received. | ||
| const clustersByName = React.useMemo( | ||
| () => new Map(clusters.map(cluster => [cluster.name, cluster])), |
cef2dab to
b54f8a0
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
frontend: optimize cluster lookup with Map and useMemo— Description must start with a capital letter — e.g.frontend: HomeButton: Fix the buttonnotfrontend: HomeButton: fix the button.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
frontend/src/components/cluster/Chooser.tsx:199
- In this file,
ClusterListreceivesclustersfromObject.values(clustersConf)in the parentChoosercomponent, which creates a new array on each render. That means thisuseMemowill typically recompute every render anyway (since[clusters]changes by reference), limiting the intended memoization benefit. Consider memoizing theclusterListarray in the parent, or removinguseMemohere if the input can’t be made stable.
const clustersByName = React.useMemo(
() => new Map<string, Cluster>(clusters.map(cluster => [cluster.name, cluster] as const)),
[clusters]
);
| const clustersByName = React.useMemo( | ||
| () => new Map<string, Cluster>(clusters.map(cluster => [cluster.name, cluster] as const)), | ||
| [clusters] | ||
| ); | ||
|
|
| const clustersByName = React.useMemo( | ||
| () => new Map<string, Cluster>(clusters.map(cluster => [cluster.name, cluster] as const)), | ||
| [clusters] | ||
| ); | ||
|
|
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.
3b86602 to
54f476f
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
can you please rebase against main to remove the merge main commit?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
Replace O(N*M) nested array iterations with an O(N+M) Map-based lookup for recent clusters. This significantly improves UI responsiveness in environments with many clusters. - Implement React.useMemo for clusters lookup Map. - Construct the Map conditionally only if clusters.length exceeds the maxRecentClusters threshold to avoid redundant work. - Add explicit TypeScript types for Map entries. - Use 'as const' to prevent tuple widening. Signed-off-by: DashratRajpurohit <dashrat.rajpurohit.cg@gmail.com>
1959c96 to
54422d9
Compare
What
Optimized the recent cluster lookup logic by replacing nested array iterations ($O(N \cdot M)$) with a Map-based lookup ($O(N + M)$).
Why
In environments with a large number of clusters, the current nested
.find()inside.map()can lead to performance degradation during re-renders. Using aMapandReact.useMemoensures the lookup table is built once per cluster list update and provides constant-time lookups.Changes
clustersByNameMap usinguseMemoinChooser.tsxandRecentClusters.tsx.clusters.find()withclustersByName.get().Testing
Verified that recent clusters are still correctly identified and filtered.