feat(openchoreo): add row-level delete to catalog and namespace listings#675
Open
isuruRuhu wants to merge 3 commits into
Open
feat(openchoreo): add row-level delete to catalog and namespace listings#675isuruRuhu wants to merge 3 commits into
isuruRuhu wants to merge 3 commits into
Conversation
Signed-off-by: isuruRuhu <isuru.ruhu@gmail.com>
Signed-off-by: isuruRuhu <isuru.ruhu@gmail.com>
Signed-off-by: isuruRuhu <isuru.ruhu@gmail.com>
isuruRuhu
requested review from
Mirage20,
kaviththiranga,
sameerajayasoma and
stefinie123
as code owners
📝 WalkthroughWalkthroughThis PR generalizes listing-row delete behavior across the Backstage plugin: a new ChangesFrontend row-delete generalization
System refresh on component delete
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Row as RowDeleteButton
participant Dialog as useDeleteEntityDialog
participant Client as OpenChoreoClientApi
participant Overlay as usePendingDeletionOverlay
Row->>Dialog: requestDelete(entity)
Dialog->>Client: performEntityDelete(entity)
Client-->>Dialog: success
Dialog->>Overlay: markDeleted(entity)
Overlay-->>Row: overlay(entities) shows deletion badge
sequenceDiagram
participant Event as EventDeltaApplier
participant Catalog as CatalogService
Event->>Catalog: getEntityByRef(component)
Catalog-->>Event: parent System ref (partOf or spec.system)
Event->>Catalog: remove Component entity
Event->>Catalog: refreshEntity(System ref)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/openchoreo/src/components/Projects/ProjectContentsCard/ProjectContentsCard.tsx (1)
89-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating pending-deletion tracking with
usePendingDeletionOverlay.
ProjectContentsCardmanually manages apendingDeletionsSet and applies optimistic marks viawithOptimisticDeletion, which overlaps with whatusePendingDeletionOverlayprovides (markDeleted+overlay). The duplication is currently justified because the overlay here operates onProjectContentItem[](notEntity[]) and the card also needs arefreshTokenbump that the hook doesn't support. If this pattern appears in more listing components, consider extracting a lower-levelusePendingDeletionSethook (exposingmarkDeleted,isPending, and the raw Set) that bothusePendingDeletionOverlayand this card could share.Also applies to: 216-225
🤖 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 `@plugins/openchoreo/src/components/Projects/ProjectContentsCard/ProjectContentsCard.tsx` around lines 89 - 95, `ProjectContentsCard` is duplicating pending-deletion state management that overlaps with `usePendingDeletionOverlay`; refactor the shared logic so the card can reuse a lower-level pending-deletion primitive instead of owning its own `pendingDeletions` Set and `withOptimisticDeletion` flow. Keep the card-specific `refreshToken` behavior, but move the shared delete-tracking pieces into a reusable hook (for example a `usePendingDeletionSet`-style helper) that exposes `markDeleted`, `isPending`, and the raw Set, then have both `ProjectContentsCard` and `usePendingDeletionOverlay` consume it.plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityDialog.test.tsx (1)
84-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the in-flight "deleting" state.
The suite covers open/close, success, and error paths but doesn't verify the UI guards that prevent closing the dialog while a delete is in progress (disabled buttons,
handleCloseearly-return, spinner). A never-resolving promise can hold the hook in thedeletingstate to assert these guards.✅ Suggested test
it('closes on cancel without calling the API', async () => { // ... existing test ... }); + + it('disables actions and keeps the dialog open while deletion is in flight', async () => { + const user = userEvent.setup(); + // Never-resolving promise keeps the hook in the deleting state + mockClient.deleteComponent.mockReturnValue(new Promise(() => {})); + + renderHarness([makeComponent('my-service')]); + + await user.click(screen.getByTestId('open-my-service')); + await user.click(screen.getByRole('button', { name: /^delete$/i })); + + await waitFor(() => { + expect(screen.getByRole('button', { name: /cancel/i })).toBeDisabled(); + expect(screen.getByRole('button', { name: /deleting/i })).toBeDisabled(); + }); + + // Dialog remains open + expect( + screen.getByRole('heading', { name: /delete component/i }), + ).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 `@plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityDialog.test.tsx` around lines 84 - 263, Add a test in useDeleteEntityDialog.test.tsx for the in-flight deleting state: trigger delete through the existing renderHarness/useDeleteEntityDialog flow with a never-resolving mockClient.deleteComponent promise, then assert the dialog enters the deleting state and UI guards are active (cancel/delete disabled, spinner shown, and close attempts are ignored by handleClose). Use the existing makeComponent, renderHarness, and delete button flow to locate the right hook behavior without changing the success/error tests.
🤖 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 @.changeset/component-delete-from-project-listing.md:
- Around line 5-10: The changeset summary is inconsistent with the implemented
behavior: RowActionsCell and RowDeleteButton support deletion for both component
and resource rows, and the related listing-row-delete-everywhere.md changeset
confirms that scope. Update the wording in the changeset to remove the “hidden
for resources” claim and describe the delete action as applying to both
components and resources, so the changelog matches the final behavior.
In
`@plugins/openchoreo/src/components/DeleteEntity/hooks/deleteEntityDispatch.tsx`:
- Around line 110-128: The cascade note in getEntityDeleteCascadeNote is using
Typography variant="h5", which makes the warning read like a heading inside the
delete confirmation dialog. Update the returned Typography for both the system
and domain branches to use a body-level variant such as body2 or caption, and
keep the note visually distinct with the existing warning styling instead of
relying on heading semantics.
---
Nitpick comments:
In
`@plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityDialog.test.tsx`:
- Around line 84-263: Add a test in useDeleteEntityDialog.test.tsx for the
in-flight deleting state: trigger delete through the existing
renderHarness/useDeleteEntityDialog flow with a never-resolving
mockClient.deleteComponent promise, then assert the dialog enters the deleting
state and UI guards are active (cancel/delete disabled, spinner shown, and close
attempts are ignored by handleClose). Use the existing makeComponent,
renderHarness, and delete button flow to locate the right hook behavior without
changing the success/error tests.
In
`@plugins/openchoreo/src/components/Projects/ProjectContentsCard/ProjectContentsCard.tsx`:
- Around line 89-95: `ProjectContentsCard` is duplicating pending-deletion state
management that overlaps with `usePendingDeletionOverlay`; refactor the shared
logic so the card can reuse a lower-level pending-deletion primitive instead of
owning its own `pendingDeletions` Set and `withOptimisticDeletion` flow. Keep
the card-specific `refreshToken` behavior, but move the shared delete-tracking
pieces into a reusable hook (for example a `usePendingDeletionSet`-style helper)
that exposes `markDeleted`, `isPending`, and the raw Set, then have both
`ProjectContentsCard` and `usePendingDeletionOverlay` consume it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e3e82b-e732-45b7-820f-d25520ec739a
📒 Files selected for processing (29)
.changeset/component-delete-from-project-listing.md.changeset/listing-row-delete-everywhere.md.changeset/refresh-system-on-component-delete.mdpackages/app/src/components/catalog/CatalogCardList.tsxpackages/app/src/components/catalog/styles.tsplugins/catalog-backend-module-openchoreo/src/provider/EventDeltaApplier.test.tsplugins/catalog-backend-module-openchoreo/src/provider/EventDeltaApplier.tsplugins/openchoreo/src/components/DeleteEntity/components/DeleteEntityDialog.tsxplugins/openchoreo/src/components/DeleteEntity/components/RowDeleteButton.test.tsxplugins/openchoreo/src/components/DeleteEntity/components/RowDeleteButton.tsxplugins/openchoreo/src/components/DeleteEntity/components/index.tsplugins/openchoreo/src/components/DeleteEntity/hooks/deleteEntityDispatch.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/index.tsplugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityDialog.test.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityDialog.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/usePendingDeletionOverlay.tsplugins/openchoreo/src/components/DeleteEntity/index.tsplugins/openchoreo/src/components/DeleteEntity/utils/deletionUtils.tsplugins/openchoreo/src/components/DeleteEntity/utils/index.tsplugins/openchoreo/src/components/Namespaces/NamespaceProjectsCard/NamespaceProjectsCard.tsxplugins/openchoreo/src/components/Namespaces/NamespaceResourcesCard/NamespaceResourcesCard.tsxplugins/openchoreo/src/components/Projects/ProjectContentsCard/ProjectContentsCard.test.tsxplugins/openchoreo/src/components/Projects/ProjectContentsCard/ProjectContentsCard.tsxplugins/openchoreo/src/components/Projects/ProjectContentsCard/RowActionsCell.test.tsxplugins/openchoreo/src/components/Projects/ProjectContentsCard/RowActionsCell.tsxplugins/openchoreo/src/components/Projects/ProjectContentsCard/columns.tsxplugins/openchoreo/src/components/Projects/hooks/useProjectContentsPage.tsplugins/openchoreo/src/index.ts
Comment on lines
+5
to
+10
| Add a per-row "Delete" action to the Project Contents table so a component | ||
| can be deleted directly from the project listing without opening the | ||
| component page first. The action surfaces in a row "more actions" kebab for | ||
| component rows (hidden for resources and for rows already marked for | ||
| deletion), reuses the shared delete-confirmation dialog, and refreshes the | ||
| listing once the component is marked for deletion. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Changeset says "hidden for resources" but code supports resource deletion.
RowActionsCell renders RowDeleteButton for both component and resource items, and RowActionsCell.test.tsx confirms the delete button appears for resource rows. The companion changeset listing-row-delete-everywhere.md also states it "extends the Project Contents row action to resource rows." Since both changesets ship together, the "hidden for resources" clause is inaccurate for the final state and will confuse changelog readers.
📝 Suggested changeset revision
Add a per-row "Delete" action to the Project Contents table so a component
can be deleted directly from the project listing without opening the
-component page first. The action surfaces in a row "more actions" kebab for
-component rows (hidden for resources and for rows already marked for
-deletion), reuses the shared delete-confirmation dialog, and refreshes
-the listing once the component is marked for deletion.
+component page first. The action surfaces as a per-row delete button for
+both component and resource rows (hidden for rows already marked for
+deletion), reuses the shared delete-confirmation dialog, and optimistically
+marks the row for deletion until the next catalog sync drops it.📝 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.
Suggested change
| Add a per-row "Delete" action to the Project Contents table so a component | |
| can be deleted directly from the project listing without opening the | |
| component page first. The action surfaces in a row "more actions" kebab for | |
| component rows (hidden for resources and for rows already marked for | |
| deletion), reuses the shared delete-confirmation dialog, and refreshes the | |
| listing once the component is marked for deletion. | |
| Add a per-row "Delete" action to the Project Contents table so a component | |
| can be deleted directly from the project listing without opening the | |
| component page first. The action surfaces as a per-row delete button for | |
| both component and resource rows (hidden for rows already marked for | |
| deletion), reuses the shared delete-confirmation dialog, and optimistically | |
| marks the row for deletion until the next catalog sync drops it. |
🤖 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 @.changeset/component-delete-from-project-listing.md around lines 5 - 10, The
changeset summary is inconsistent with the implemented behavior: RowActionsCell
and RowDeleteButton support deletion for both component and resource rows, and
the related listing-row-delete-everywhere.md changeset confirms that scope.
Update the wording in the changeset to remove the “hidden for resources” claim
and describe the delete action as applying to both components and resources, so
the changelog matches the final behavior.
Comment on lines
+110
to
+128
| export function getEntityDeleteCascadeNote(kind: string): ReactNode { | ||
| const kindLower = kind.toLowerCase(); | ||
| if (kindLower === 'system') { | ||
| return ( | ||
| <Typography variant="h5"> | ||
| Note: All components within this project will also be deleted. | ||
| </Typography> | ||
| ); | ||
| } | ||
| if (kindLower === 'domain') { | ||
| return ( | ||
| <Typography variant="h5"> | ||
| Note: All projects and components within this namespace will also be | ||
| deleted. | ||
| </Typography> | ||
| ); | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use body2 or caption variant for cascade note text instead of h5.
Typography variant="h5" renders as a heading, which is semantically incorrect for a warning note inside a confirmation dialog. Screen readers will announce it as a heading, and visually it will be disproportionately large relative to the dialog's other text. Use variant="body2" (or caption) with a warning color instead.
🎨 Proposed fix
export function getEntityDeleteCascadeNote(kind: string): ReactNode {
const kindLower = kind.toLowerCase();
if (kindLower === 'system') {
return (
- <Typography variant="h5">
+ <Typography variant="body2" color="error">
Note: All components within this project will also be deleted.
</Typography>
);
}
if (kindLower === 'domain') {
return (
- <Typography variant="h5">
+ <Typography variant="body2" color="error">
Note: All projects and components within this namespace will also be
deleted.
</Typography>
);
}
return undefined;
}📝 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.
Suggested change
| export function getEntityDeleteCascadeNote(kind: string): ReactNode { | |
| const kindLower = kind.toLowerCase(); | |
| if (kindLower === 'system') { | |
| return ( | |
| <Typography variant="h5"> | |
| Note: All components within this project will also be deleted. | |
| </Typography> | |
| ); | |
| } | |
| if (kindLower === 'domain') { | |
| return ( | |
| <Typography variant="h5"> | |
| Note: All projects and components within this namespace will also be | |
| deleted. | |
| </Typography> | |
| ); | |
| } | |
| return undefined; | |
| } | |
| export function getEntityDeleteCascadeNote(kind: string): ReactNode { | |
| const kindLower = kind.toLowerCase(); | |
| if (kindLower === 'system') { | |
| return ( | |
| <Typography variant="body2" color="error"> | |
| Note: All components within this project will also be deleted. | |
| </Typography> | |
| ); | |
| } | |
| if (kindLower === 'domain') { | |
| return ( | |
| <Typography variant="body2" color="error"> | |
| Note: All projects and components within this namespace will also be | |
| deleted. | |
| </Typography> | |
| ); | |
| } | |
| return undefined; | |
| } |
🤖 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
`@plugins/openchoreo/src/components/DeleteEntity/hooks/deleteEntityDispatch.tsx`
around lines 110 - 128, The cascade note in getEntityDeleteCascadeNote is using
Typography variant="h5", which makes the warning read like a heading inside the
delete confirmation dialog. Update the returned Typography for both the system
and domain branches to use a body-level variant such as body2 or caption, and
keep the note visually distinct with the existing warning styling instead of
relying on heading semantics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
To let users delete entities directly from the console listing pages, without opening each entity page first. A delete action is added to the rows of the catalog "All ..." pages, the namespace page's projects/resources cards, and the project's Project Contents table (components and resources).
Goals
Approach
A shared
useDeleteEntityDialoghook dispatches the delete per entity kind, reusing the same kind mapping and confirmation dialog as the entity-page context menu. A reusableRowDeleteButtonrenders the row action only for kinds the API can delete and hides it for rows already marked for deletion. Deleted rows are overlaid with the "marked for deletion" badge until the catalog sync catches up.User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Summary by CodeRabbit
New Features
Bug Fixes
Style