-
Notifications
You must be signed in to change notification settings - Fork 14
feat(Clusters): allow add, edit and delete cluster funcs #2369
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
84c3bb1
to
380ec0f
Compare
380ec0f
to
c225fe0
Compare
c225fe0
to
0466072
Compare
@astandrik @Raubzeug ping |
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
This PR implements create/edit/delete cluster functionality in the UI by adding new capability endpoints, hooks, header actions, and table menu items, while removing the old cluster statistics view.
- Introduce
/meta/create_cluster
,/meta/update_cluster
,/meta/delete_cluster
capabilities and corresponding hooks (useAdd/Edit/DeleteClusterFeatureAvailable
) - Extend the header: add “Add Cluster” button on the Clusters page and update breadcrumb logic to include a root “All clusters” link
- Update the Clusters page: remove statistics component, add a dropdown menu for edit/delete actions in the table, and adjust layout
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/types/api/capabilities.ts | Add new cluster CRUD capability strings |
src/store/reducers/capabilities/hooks.ts | Add hooks to check feature availability |
src/containers/Header/breadcrumbs.tsx | Add clusters breadcrumb and adapt getters |
src/containers/Header/Header.tsx | Insert Add Cluster button and update header controls |
src/containers/Clusters/columns.tsx | Add edit/delete actions to the clusters table column |
src/containers/Clusters/Clusters.tsx | Remove statistics, wire up hooks, and adjust layout |
src/containers/Clusters/i18n/ru.json | (Removed) Russian translations for clusters page |
src/containers/Clusters/i18n/index.ts | Removed RU import from keyset registration |
Comments suppressed due to low confidence (5)
src/containers/Clusters/i18n/ru.json:1
- Russian translations for the clusters page were removed; re-add the
ru.json
file or merge these translations back to maintain Russian localization support.
{
src/containers/Clusters/i18n/index.ts:3
- Removed the RU keyset import; this breaksover Russian localization for the clusters page. Consider restoring the
ru
keyset registration.
import ru from './ru.json';
src/store/reducers/capabilities/hooks.ts:127
- New feature hook
useAddClusterFeatureAvailable
lacks unit tests verifying its behavior; consider adding tests to cover feature-version lookup logic.
export const useAddClusterFeatureAvailable = () => {
src/containers/Header/Header.tsx:61
- Rendering logic for the “Add Cluster” button is new UI behavior and should be covered by component tests to catch potential regressions.
if (isClustersPage && isAddClusterAvailable) {
src/containers/Clusters/i18n/en.json:21
- [nitpick] The UI label uses “Remove Cluster” but the API hook is named
onDeleteCluster
. Consider renaming the key and label to “delete-cluster” for consistency.
"remove-cluster": "Remove Cluster"
src/containers/Header/Header.scss
Outdated
@@ -4,7 +4,7 @@ | |||
justify-content: space-between; | |||
align-items: center; | |||
|
|||
padding: 0 20px 0 12px; | |||
padding: 0 20px; |
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.
--g-spacing-5
@@ -44,6 +48,15 @@ export function Clusters() { | |||
|
|||
const dispatch = useTypedDispatch(); | |||
|
|||
React.useEffect(() => { | |||
dispatch(setHeaderBreadcrumbs('clusters', {})); |
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.
Is it possible to set it as default value in store instead of useEffect here?
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.
- It is not a default value, there is no clusters page in single-cluster version
useEffect
is needed here anyway to reset current breadcrumbs state. So even if it will be a default value, some function call will be needed
src/containers/Clusters/columns.tsx
Outdated
const renderActions = () => { | ||
const menuItems: (DropdownMenuItem | DropdownMenuItem[])[] = []; | ||
|
||
if (isEditClusterAvailable) { |
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.
maybe if onEditCluster is undefined - we also shouldnt push this to menuItmes
currently its kinda unobvious how onEditCluster and isEditClusterAvailable are connected or related to each other
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.
We check it when we define these vars
const isEditClusterAvailable =
useEditClusterFeatureAvailable() && uiFactory.onEditCluster !== undefined;
const isDeleteClusterAvailable =
useDeleteClusterFeatureAvailable() && uiFactory.onDeleteCluster !== undefined;
uiFactory.onEditCluster?.()
here is because we do not check type here
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.
Fixed, it should look better now
Part of #2333
onAddCluster
,onEditCluster
,onDeleteCluster
options touiFactory
Clusters page is not fully redesigned according to design, I only make changes that allow to add buttons
Design: https://nda.ya.ru/t/-uEByWGb7F6QxM
Example how it will work: https://nda.ya.ru/t/EyvbNXsb7F7iRc
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary 🗑️2
🗑️ Deleted Tests (2)
Bundle Size: ✅
Current: 83.77 MB | Main: 83.76 MB
Diff: +0.54 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information