ACM-37999 Edit Cluster Description via Modal#6527
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oksanabaza The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughManaged cluster descriptions are rendered from annotations as Markdown and can be edited through a new modal. The editor supports Markdown formatting, persists trimmed values or clears them with ChangesManaged cluster description
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterOverview
participant EditDescription
participant patchResource
ClusterOverview->>EditDescription: Open with cluster identity and annotations
EditDescription->>patchResource: Save description annotation
patchResource-->>EditDescription: Return success or error
EditDescription-->>ClusterOverview: Close after success or cancel
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsx (3)
87-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
UPPER_SNAKE_CASEfor constant values and move outside the component.As per the coding guidelines, constants that are truly constant values must use
UPPER_SNAKE_CASE. Moving it outside the component also prevents unnecessary re-evaluations during renders.♻️ Proposed refactor
Move this definition outside of
ClusterOverviewPageContent:+const CLUSTER_DESCRIPTION_ANNOTATION = 'console.open-cluster-management.io/description' + export function ClusterOverviewPageContent() {And remove it from within the component:
- const clusterDescriptionAnnotation = 'console.open-cluster-management.io/description' const [showEditLabels, setShowEditLabels] = useState<boolean>(false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsx` around lines 87 - 89, Move the immutable cluster description annotation value outside the ClusterOverviewPageContent component and rename it to UPPER_SNAKE_CASE, then update all references from clusterDescriptionAnnotation to the new constant.Source: Coding guidelines
519-530: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid inline object creation in JSX.
As per coding guidelines, avoid inline object creation for the
resourceprop to prevent unnecessary re-renders of theEditDescriptioncomponent. Consider usinguseMemofor the resource payload anduseCallbackfor theclosehandler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsx` around lines 519 - 530, Update ClusterOverview’s EditDescription props to avoid creating the resource object and close callback inline during render. Use useMemo for the conditional resource payload and useCallback for the setShowEditDescription(false) handler, preserving the existing values and undefined behavior when editing is hidden.Source: Coding guidelines
344-361: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid inline object/array and function creation in JSX.
As per coding guidelines, avoid inline functions and inline array creation (e.g.,
rbac={[...]},onClick={() => ...}) in JSX to prevent unnecessary re-renders.Consider extracting the
rbacarray and theonClickhandler. Also, if you apply the constant renaming suggested above, ensure you updateclusterDescriptionAnnotationtoCLUSTER_DESCRIPTION_ANNOTATION.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsx` around lines 344 - 361, In the ClusterOverview component, extract the inline rbac array and edit-description onClick callback from the description keyAction JSX into stable variables or callbacks, then reference them in RbacButton to avoid recreating them during render. If the annotation constant was renamed, update all references from clusterDescriptionAnnotation to CLUSTER_DESCRIPTION_ANNOTATION.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx`:
- Around line 111-160: Extract the inline save handler from AcmSubmit into a
named handleSave function, preserving alert clearing, patchResource success, and
error handling. Change the patch from a JSON Patch array to a merge-patch object
whose metadata.annotations entry sets the trimmed description or null for
deletion, allowing existing annotations to merge safely. Update AcmSubmit’s
onClick to reference handleSave.
---
Nitpick comments:
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsx`:
- Around line 87-89: Move the immutable cluster description annotation value
outside the ClusterOverviewPageContent component and rename it to
UPPER_SNAKE_CASE, then update all references from clusterDescriptionAnnotation
to the new constant.
- Around line 519-530: Update ClusterOverview’s EditDescription props to avoid
creating the resource object and close callback inline during render. Use
useMemo for the conditional resource payload and useCallback for the
setShowEditDescription(false) handler, preserving the existing values and
undefined behavior when editing is hidden.
- Around line 344-361: In the ClusterOverview component, extract the inline rbac
array and edit-description onClick callback from the description keyAction JSX
into stable variables or callbacks, then reference them in RbacButton to avoid
recreating them during render. If the annotation constant was renamed, update
all references from clusterDescriptionAnnotation to
CLUSTER_DESCRIPTION_ANNOTATION.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d9bfd23a-9364-4f5d-945d-b857ebdd069b
📒 Files selected for processing (3)
frontend/src/resources/utils/get-cluster.tsfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/ClusterDetails/ClusterOverview/ClusterOverview.tsxfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx (1)
4-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
IAlertContext.
IAlertContextis only used in a type position. Move it toimport typeto comply with the frontend TypeScript import rule.Proposed fix
import { AcmAlertContext, AcmAlertGroup, AcmForm, AcmModal, AcmSubmit, - IAlertContext, } from '../../../../../ui-components' +import type { IAlertContext } from '../../../../../ui-components'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx` around lines 4 - 11, Update the imports used by EditDescription so IAlertContext is imported with a type-only import, while AcmAlertContext, AcmAlertGroup, AcmForm, AcmModal, and AcmSubmit remain value imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx`:
- Around line 4-11: Update the imports used by EditDescription so IAlertContext
is imported with a type-only import, while AcmAlertContext, AcmAlertGroup,
AcmForm, AcmModal, and AcmSubmit remain value imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c15c6b8-c673-4d0d-afd7-f0f550f0acc3
📒 Files selected for processing (1)
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx (3)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
userEvent.clickfor save actions.Native
.click()bypasses the interaction model used by the rest of this suite.
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L54-L54: replace.click()withuserEvent.click(...).frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L75-L75: replace.click()withuserEvent.click(...).frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L115-L115: replace.click()withuserEvent.click(...).As per coding guidelines, “Use
@testing-library/user-eventfor user interactions instead offireEvent.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx` at line 54, Replace the native save-button click calls in EditDescription.test.tsx at lines 54, 75, and 115 with userEvent.click interactions, ensuring userEvent is available through the suite’s existing testing setup.Source: Coding guidelines
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse a
closemock and assert modal lifecycle behavior.Inline no-op callbacks prevent verifying that successful saves close the modal and failed saves do not.
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L27-L27: pass a named mock instead of an inline callback.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L32-L32: pass a named mock instead of an inline callback.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L37-L37: assert the mock is called after a successful update.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L59-L59: assert the mock is called after clearing succeeds.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L80-L82: pass a named mock instead of an inline callback.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L97-L97: assert the mock is not called after the corrected failure response.frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx#L127-L127: pass a named mock instead of an inline callback.As per coding guidelines, “avoid inline functions ... in JSX.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx` at line 27, In EditDescription.test.tsx, create and reuse a named close mock across all EditDescription renders at lines 27, 32, 80-82, and 127 instead of inline callbacks; assert it is called after successful update and clearing at lines 37 and 59, and not called after the corrected failure response at line 97.Source: Coding guidelines
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse aliases and a type-only import.
IResourceis used only as a type, and the added relative imports should use~/paths.Proposed change
-import { IResource, ManagedClusterApiVersion, ManagedClusterKind } from '../../../../../resources' +import { ManagedClusterApiVersion, ManagedClusterKind } from '~/resources' +import type { IResource } from '~/resources' import { render, waitFor } from '`@testing-library/react`' import userEvent from '`@testing-library/user-event`' -import { mockBadRequestStatus, nockIgnoreApiPaths, nockPatch } from '../../../../../lib/nock-util' -import { EditDescription } from './EditDescription' +import { mockBadRequestStatus, nockIgnoreApiPaths, nockPatch } from '~/lib/nock-util' +import { EditDescription } from '~/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription'As per coding guidelines, “When adding imports, use the
~/shorthand” and “Useimport typefor type-only imports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx` around lines 3 - 7, Update the imports in EditDescription.test.tsx to use the ~/ alias for the added project-relative imports, and split IResource into an import type statement while keeping ManagedClusterApiVersion and ManagedClusterKind as value imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx`:
- Around line 103-116: Update the nockPatch setup in the EditDescription test to
pass mockBadRequestStatus as the response status argument rather than the
response body, then assert that the expected error alert is displayed after
clicking the Save button. Keep the existing request-completion assertion and
ensure the test exercises the failed update path.
---
Nitpick comments:
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx`:
- Line 54: Replace the native save-button click calls in
EditDescription.test.tsx at lines 54, 75, and 115 with userEvent.click
interactions, ensuring userEvent is available through the suite’s existing
testing setup.
- Line 27: In EditDescription.test.tsx, create and reuse a named close mock
across all EditDescription renders at lines 27, 32, 80-82, and 127 instead of
inline callbacks; assert it is called after successful update and clearing at
lines 37 and 59, and not called after the corrected failure response at line 97.
- Around line 3-7: Update the imports in EditDescription.test.tsx to use the ~/
alias for the added project-relative imports, and split IResource into an import
type statement while keeping ManagedClusterApiVersion and ManagedClusterKind as
value imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3f409aa-7d8b-4429-89e6-ae9d5ac6be61
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (1)
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx
| const nockScope = nockPatch( | ||
| { apiVersion: resource.apiVersion, kind: resource.kind, metadata: { name: resource.metadata!.name } }, | ||
| { | ||
| metadata: { | ||
| annotations: { | ||
| [CLUSTER_DESCRIPTION_ANNOTATION]: 'New description', | ||
| }, | ||
| }, | ||
| }, | ||
| mockBadRequestStatus | ||
| ) | ||
|
|
||
| getByRole('button', { name: /save/i }).click() | ||
| await waitFor(() => expect(nockScope.isDone()).toBeTruthy()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return an actual failure status and assert the alert.
nockPatch treats mockBadRequestStatus as the response body; without a fourth argument, it returns 204. This test currently exercises the success path and never verifies the error UI.
Proposed change
},
- mockBadRequestStatus
+ mockBadRequestStatus,
+ 400
)
getByRole('button', { name: /save/i }).click()
await waitFor(() => expect(nockScope.isDone()).toBeTruthy())
+ expect(getByText('Bad request.')).toBeInTheDocument()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nockScope = nockPatch( | |
| { apiVersion: resource.apiVersion, kind: resource.kind, metadata: { name: resource.metadata!.name } }, | |
| { | |
| metadata: { | |
| annotations: { | |
| [CLUSTER_DESCRIPTION_ANNOTATION]: 'New description', | |
| }, | |
| }, | |
| }, | |
| mockBadRequestStatus | |
| ) | |
| getByRole('button', { name: /save/i }).click() | |
| await waitFor(() => expect(nockScope.isDone()).toBeTruthy()) | |
| const nockScope = nockPatch( | |
| { apiVersion: resource.apiVersion, kind: resource.kind, metadata: { name: resource.metadata!.name } }, | |
| { | |
| metadata: { | |
| annotations: { | |
| [CLUSTER_DESCRIPTION_ANNOTATION]: 'New description', | |
| }, | |
| }, | |
| }, | |
| mockBadRequestStatus, | |
| 400 | |
| ) | |
| getByRole('button', { name: /save/i }).click() | |
| await waitFor(() => expect(nockScope.isDone()).toBeTruthy()) | |
| expect(getByText('Bad request.')).toBeInTheDocument() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/EditDescription.test.tsx`
around lines 103 - 116, Update the nockPatch setup in the EditDescription test
to pass mockBadRequestStatus as the response status argument rather than the
response body, then assert that the expected error alert is displayed after
clicking the Save button. Keep the existing request-completion assertion and
ensure the test exercises the failed update path.
|
/retest |
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
Signed-off-by: Oksana Bazylieva <obazylie@redhat.com>
|
1 participant
📝 Summary
Ticket Summary (Title):
ACM-37999 Edit Cluster Description via Modal
Ticket Link:
https://redhat.atlassian.net/browse/ACM-37999
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Commands used in demo:
oc get managedcluster weekly -o jsonpath='{.metadata.annotations.console\.open-cluster-management\.io/description}'oc annotate managedcluster weekly 'console.open-cluster-management.io/description=**Updated Description** - _test cli_' --overwriteoc annotate managedcluster weekly 'console.open-cluster-management.io/description=**Production Cluster** - This is an extremely long description designed to stress test the UI rendering capabilities and ensure that the markdown component can handle very long single-line strings without breaking the layout or causing performance issues. The cluster runs critical workloads including [microservices](https://kubernetes.io/docs/concepts/services-networking/), [databases](https://www.postgresql.org/), and [message queues](https://kafka.apache.org/) across multiple availability zones. It features auto-scaling with HPA configured for CPU and memory metrics, network policies for security segmentation, service mesh integration with Istio for traffic management and observability, persistent storage backed by Ceph RBD and CephFS storage classes, monitoring stack including Prometheus, Grafana, and AlertManager, centralized logging with Elasticsearch and Fluentd, backup solutions with Velero, disaster recovery procedures documented in the runbook, regular security scanning with Trivy and Falco, compliance checks against CIS benchmarks, automated certificate rotation via cert-manager, ingress controllers with rate limiting and WAF rules, GitOps deployment pipelines using ArgoCD and Tekton, chaos engineering tests with Chaos Mesh, cost optimization policies, resource quotas per namespace, multi-tenancy isolation, federated identity management, audit logging enabled, encryption at rest and in transit, regular vulnerability assessments, incident response procedures, on-call rotation schedules, SLO definitions and SLI monitoring, capacity planning reports, performance benchmarking results, network latency measurements between zones, database replication lag monitoring, cache hit ratio tracking, API response time percentiles, error rate thresholds, saturation metrics for CPU memory disk and network, and comprehensive documentation for all operational procedures including deployment rollback strategies upgrade procedures node maintenance workflows and troubleshooting guides.' --overwriteoc annotate managedcluster weekly 'console.open-cluster-management.io/description-'Demo:
Demo.mov
Summary by CodeRabbit