Add confirmation UI for deleting inactive hosts - #144
Open
samuelxanda wants to merge 2 commits into
Open
Conversation
Admins should confirm before removing a host the API marks deletable, and viewers should never see that action. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Adds a frontend-only “Delete host” action for inactive hosts, gated by the server-provided canDelete flag and admin role, with a confirmation dialog and React Query invalidation to refresh host/inventory data after deletion.
Changes:
- Introduces
canDelete?: booleanonHost, plus acanShowDeleteHosthelper and unit test. - Adds
DELETEsupport to the API client, adeleteHostservice, and auseDeleteHostmutation hook (with tests) that invalidates['hosts']-prefixed queries. - Wires delete action + confirmation modal into
HostCard, and passes admin/delete handlers from Dashboard and Hosts pages; adds UI tests.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Locks new test dependencies (Testing Library, user-event, jsdom) pulled in for UI/hook tests. |
| apps/web/vite.config.ts | Adds Vitest type reference and a default test environment configuration. |
| apps/web/src/types/api.ts | Extends Host with optional canDelete flag from the API. |
| apps/web/src/services/hosts.ts | Adds deleteHost() service call. |
| apps/web/src/services/api.ts | Extends request method union to include DELETE and exposes apiClient.delete. |
| apps/web/src/lib/hosts.ts | Adds canShowDeleteHost() gating helper. |
| apps/web/src/lib/hosts.test.ts | Unit test for canShowDeleteHost(). |
| apps/web/src/hooks/useDeleteHost.ts | Adds mutation hook that deletes hosts, shows toasts, and invalidates hosts queries. |
| apps/web/src/hooks/useDeleteHost.test.tsx | Tests invalidation and error toast behavior for the delete hook. |
| apps/web/src/features/hosts/HostsPage.tsx | Passes canManage (admin) and onDelete into HostCard. |
| apps/web/src/features/dashboard/DashboardPage.tsx | Passes canManage (admin) and onDelete into HostCard on the dashboard view. |
| apps/web/src/components/HostCard.tsx | Adds “Delete host” menu item (when eligible) and confirmation dialog flow. |
| apps/web/src/components/HostCard.test.tsx | UI tests for eligibility gating, cancel, and confirm delete flows. |
| apps/web/src/components/DeleteHostDialog.tsx | New confirmation dialog component rendered via portal. |
| apps/web/package.json | Adds Testing Library, user-event, and jsdom devDependencies for web tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
31
to
34
| "@types/react-dom": "^19.2.3", | ||
| "@vitejs/plugin-react": "^6.0.3", | ||
| "jsdom": "^30.0.1", | ||
| "oxlint": "^1.71.0", |
Comment on lines
+19
to
+42
| useEffect(() => { | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape' && !pending) { | ||
| onCancel() | ||
| } | ||
| } | ||
| document.addEventListener('keydown', onKeyDown) | ||
| return () => document.removeEventListener('keydown', onKeyDown) | ||
| }, [onCancel, pending]) | ||
|
|
||
| return createPortal( | ||
| <div className="fixed inset-0 z-50 flex items-center justify-center p-4"> | ||
| <div | ||
| className="animate-overlay-in absolute inset-0 bg-slate-950/40 backdrop-blur-[2px]" | ||
| onClick={pending ? undefined : onCancel} | ||
| aria-hidden | ||
| /> | ||
| <div | ||
| role="alertdialog" | ||
| aria-modal="true" | ||
| aria-labelledby="delete-host-title" | ||
| aria-describedby="delete-host-description" | ||
| className="animate-pop-in relative w-full max-w-md rounded-lg border border-border bg-card p-5 shadow-2xl" | ||
| > |
jsdom 30 cannot run in CI, and the confirm dialog needs focus plus a scroll lock so keyboard users can actually use it. Co-authored-by: Cursor <cursoragent@cursor.com>
samuelxanda
marked this pull request as ready for review
August 24, 2026 21:33
rodriguecyber
requested changes
Aug 25, 2026
rodriguecyber
left a comment
rodriguecyber
left a comment
Collaborator
There was a problem hiding this comment.
The PR description mention Backend related issue but you worked on frontend part please update you r work description so it can be merged
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
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.
Summary
canDelete: true.hostsqueries (inventory keys share that prefix) so the dashboard refreshes.This PR does not include the backend deletion endpoint (#125). Until that lands,
canDeleteis absent and the action stays hidden.Test plan
canDelete: true): menu shows Delete host, confirm removes it from the dashboard.cd apps/web && npm test(52 passing locally).Closes #126