[scanner] fix: add user feedback for silent actions#21291
Conversation
Add toast notifications after delete operations that previously gave no user feedback: - TeamManagement: show success/error toast after team deletion - LocalClustersSection: show success/error toast after local cluster deletion - CardFactoryModal: show success toast after custom card deletion - Add i18n keys for all new toast messages in en/common.json Fixes #21288 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: GitHub Copilot <223556219+Copilot@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
Bot
added
the
size/M
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
✅ Test Coverage CheckAll new source files in this PR have corresponding test files. Checked |
♿ Accessibility Audit (WCAG 2.1 AA)✅ No WCAG 2.1 AA violations detected in audited routes. Powered by axe-core. Target: WCAG 2.1 AA compliance. |
There was a problem hiding this comment.
Pull request overview
Adds user-visible toast notifications for destructive delete actions that previously completed silently, aligning UI feedback with the existing useToast utility and i18n conventions.
Changes:
- Team deletion now shows success/error toast based on
deleteTeam()outcome. - Local cluster deletion now shows success/error toast based on
deleteCluster()outcome (including unexpected-throw fallback). - Custom card deletion now shows a toast after deletion, with new i18n keys added to
en/common.json.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/src/pages/TeamManagement.tsx | Adds useToast + t() usage to provide success/error feedback after team deletion. |
| web/src/components/settings/sections/LocalClustersSection.tsx | Adds useToast + translated success/error toasts for local cluster deletion results. |
| web/src/components/dashboard/CardFactoryModal.tsx | Shows a toast after custom card deletion (but currently doesn’t validate deletion success). |
| web/src/locales/en/common.json | Adds new i18n strings for the delete-operation toasts. |
| const handleDelete = (id: string) => { | ||
| deleteDynamicCard(id) | ||
| setExistingCards(getAllDynamicCards()) | ||
| showToast(t('dashboard.cardFactory.deleteSuccess'), 'success') | ||
| } |
| "invalidJsonData": "Invalid JSON data.", | ||
| "deleteSuccess": "Card deleted" |
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
[quality] Review — return-type contracts verified; three real concerns (comment mode; self-approve blocked)
Good direction — the three silent delete paths get user-visible feedback. I verified the two success branches against the current hooks:
useTeams.deleteTeamreturnsPromise<boolean>—trueon 200 or demo mode,falseon catch.if (success)is correct.useLocalClusterTools.deleteClusterreturnsPromise<boolean>—trueon 200/demo,falsewhen disconnected.if (success)is correct.
So the core wiring is sound. Concerns:
1. CardFactoryModal.handleDelete — no failure path (blocking-ish)
const handleDelete = (id: string) => {
deleteDynamicCard(id)
setExistingCards(getAllDynamicCards())
showToast(t('dashboard.cardFactory.deleteSuccess'), 'success')
}Unlike the other two, this synchronously assumes success. deleteDynamicCard presumably wraps IDB / localStorage writes, either of which can throw (QuotaExceededError, IDB AbortError, private-browsing SecurityError). If it throws, the exception propagates unhandled and the user sees no toast at all. Wrap:
const handleDelete = (id: string) => {
try {
deleteDynamicCard(id)
setExistingCards(getAllDynamicCards())
showToast(t('dashboard.cardFactory.deleteSuccess'), 'success')
} catch (e) {
showToast(t('dashboard.cardFactory.deleteError'), 'error')
logger.error('[CardFactory] Failed to delete card:', e)
}
}Add matching deleteError i18n key (currently only deleteSuccess is added).
2. useToast requires provider — verify wiring for TeamManagementPage
The useToast() hook typically requires a <ToastProvider> ancestor in the tree. If TeamManagementPage is rendered by a route that isn't wrapped, useToast() throws at mount, breaking the entire page. LocalClustersSection (Settings) and CardFactoryModal (dashboard) probably work because those trees already have provider wiring, but TeamManagement is a page-level route.
Verify: rg -l 'ToastProvider' web/src and confirm the app root/route wrapper mounts it above <TeamManagementPage />. If not, this PR introduces a mount-time crash on the teams page.
3. i18n key placement scattered across en/common.json
The new keys land in three sections: settings.localClusters.* (line ~969), dashboard.cardFactory.* (line ~1604), and teams.* (line ~5515). All are correctly nested by feature — that's good. But the dashboard.cardFactory section only adds deleteSuccess, no deleteError (see concern #1). And there's no en-locale-only PR review of the diff: pluralization variants (_one/_other) aren't needed here since neither string has count interpolation, so that's fine.
Non-blocking observations
- No tests added. Toast-showing is often skipped for unit testing because it's UI wiring, but a simple
render(<TeamManagementPage />)+ spy onuseToastmock would lock the success/error branches. Follow-up. LocalClustersSectiontry/catch is defensive. Good — the innerif (success)catchesdeleteCluster's handled errors (returns false), the outercatchcatches unexpected throws (which the old comment said don't happen but you're defending against anyway). Belt-and-suspenders.- Message text:
"Failed to delete team"doesn't include the team name, unlike the cluster message"Failed to delete cluster \"{{name}}\"". Minor UX inconsistency — worth passing the team name too.
Bead filed. Not merging.
Filed by quality agent (ACMM L4/L6 — full mode)
Wrap direct hook calls in renderHook() from @testing-library/react and access results via result.current to fix 'Invalid hook call' vitest failure in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Scanner <scanner@kubestellar.io>
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
⏹️ Post-Merge Verification: cancelledCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Fixes #21288
Add toast notifications to delete operations that previously gave no user feedback:
en/common.jsonAll messages use
t()from react-i18next with new keys in theen/common.jsonlocale file. The existinguseToast/showToastutility already present in the codebase is used — no new libraries introduced.Locations not changed (already have feedback or are not user-facing):
GPUReservationsTabprops (parentGPUReservations.tsxalready callsshowToast)ChunkErrorBoundary(sessionStorage.removeItem— internal, not a user action)handleDeleteVCluster(covered by existingVClusterActionBannerinline feedback)TeamDetail.test.tsx(test-only, no production change needed)