From 7dbee64360acc7c19908e472f594ebafa7e1114a Mon Sep 17 00:00:00 2001
From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Date: Fri, 16 Oct 2026 11:18:03 +0800
Subject: [PATCH] fix(web): report skill deletion errors once

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---
 web/src/pages/skill-detail.tsx                | 29 +++----
 .../shared/hooks/use-delete-skill.test.tsx    | 86 +++++++++++++++++++
 2 files changed, 98 insertions(+), 17 deletions(-)
 create mode 100644 web/src/shared/hooks/use-delete-skill.test.tsx

diff --git a/web/src/pages/skill-detail.tsx b/web/src/pages/skill-detail.tsx
index aa6e15352..18481f8e3 100644
--- a/web/src/pages/skill-detail.tsx
+++ b/web/src/pages/skill-detail.tsx
@@ -611,23 +611,18 @@ export function SkillDetailPage() {
     if (!skill || !isDeleteSlugConfirmationValid(skill.slug, deleteSkillInput)) {
       return
     }
-    try {
-      await deleteSkillMutation.mutateAsync({ namespace, slug, ownerId: skill.ownerId })
-      setSkillDeleted(true)
-      toast.success(
-        t('skillDetail.deleteSkillSuccessTitle'),
-        t('skillDetail.deleteSkillSuccessDescription', { skill: skill.displayName }),
-      )
-      setDeleteSkillInputOpen(false)
-      navigateAfterOverlays(() => {
-        navigate({ to: resolveDeletedSkillReturnTo(search.returnTo) })
-      })
-      queryClient.removeQueries({ queryKey: ['skills', namespace, slug] })
-      queryClient.invalidateQueries({ queryKey: ['skills', 'my'] })
-    } catch (error) {
-      toast.error(t('skillDetail.deleteSkillErrorTitle'), error instanceof Error ? error.message : '')
-      throw error
-    }
+    await deleteSkillMutation.mutateAsync({ namespace, slug, ownerId: skill.ownerId })
+    setSkillDeleted(true)
+    toast.success(
+      t('skillDetail.deleteSkillSuccessTitle'),
+      t('skillDetail.deleteSkillSuccessDescription', { skill: skill.displayName }),
+    )
+    setDeleteSkillInputOpen(false)
+    navigateAfterOverlays(() => {
+      navigate({ to: resolveDeletedSkillReturnTo(search.returnTo) })
+    })
+    queryClient.removeQueries({ queryKey: ['skills', namespace, slug] })
+    queryClient.invalidateQueries({ queryKey: ['skills', 'my'] })
   }
 
   const handleDeleteVersion = async () => {
diff --git a/web/src/shared/hooks/use-delete-skill.test.tsx b/web/src/shared/hooks/use-delete-skill.test.tsx
new file mode 100644
index 000000000..43fc24e40
--- /dev/null
+++ b/web/src/shared/hooks/use-delete-skill.test.tsx
@@ -0,0 +1,86 @@
+// @vitest-environment jsdom
+import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'
+import { useMutation } from '@tanstack/react-query'
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import { App } from '@/app/providers'
+import { skillLifecycleApi } from '@/api/client'
+import { ApiError } from '@/shared/lib/api-error'
+import { toast } from '@/shared/lib/toast'
+import { useDeleteSkill } from './use-skill-queries'
+
+vi.mock('@/app/router', () => ({ router: {} }))
+vi.mock('@tanstack/react-router', () => ({ RouterProvider: () => <DeleteHarness /> }))
+vi.mock('@/shared/components/toaster', () => ({ Toaster: () => null }))
+vi.mock('@/shared/lib/toast', () => ({ toast: { error: vi.fn(), success: vi.fn() } }))
+
+const blockedError = new ApiError('revoke global publication first', 409, 'revoke global publication first')
+
+// Exercise the real application MutationCache. Like skill-detail, the caller
+// reports success only; the global handler owns errors and authentication redirects.
+function DeleteHarness() {
+  const deletion = useDeleteSkill()
+  const unrelated = useMutation({ mutationFn: async () => { throw blockedError } })
+  const deleteSkill = async () => {
+    try {
+      await deletion.mutateAsync({ namespace: 'team', slug: 'promoted', ownerId: 'owner' })
+      toast.success('deleted')
+    } catch {
+      // The click boundary consumes rejection; MutationCache already handled it.
+    }
+  }
+  return <>
+    <button onClick={() => { void deleteSkill() }} disabled={deletion.isPending}>Delete</button>
+    <button onClick={() => { void unrelated.mutateAsync().catch(() => undefined) }}>Other request</button>
+  </>
+}
+
+afterEach(() => {
+  cleanup()
+  vi.restoreAllMocks()
+  vi.clearAllMocks()
+  vi.unstubAllGlobals()
+})
+
+describe('skill deletion feedback ownership', () => {
+  it('shows one actionable error per failed deletion, including a new retry', async () => {
+    const request = vi.spyOn(skillLifecycleApi, 'deleteSkill').mockRejectedValue(blockedError)
+    render(<App />)
+    fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
+    await waitFor(() => expect(toast.error).toHaveBeenCalledTimes(1))
+    expect(toast.error).toHaveBeenLastCalledWith(blockedError.serverMessage)
+    await waitFor(() => expect(screen.getByRole('button', { name: 'Delete' }).hasAttribute('disabled')).toBe(false))
+    fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
+    await waitFor(() => expect(request).toHaveBeenCalledTimes(2))
+    await waitFor(() => expect(toast.error).toHaveBeenCalledTimes(2))
+    expect(toast.success).not.toHaveBeenCalled()
+  })
+
+  it('reports a successful deletion once without an error notification', async () => {
+    vi.spyOn(skillLifecycleApi, 'deleteSkill').mockResolvedValue({ skillId: 123 })
+    render(<App />)
+    fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
+    await waitFor(() => expect(toast.success).toHaveBeenCalledTimes(1))
+    expect(toast.error).not.toHaveBeenCalled()
+  })
+
+
+  it('keeps the existing login redirect and one notification when the session expires', async () => {
+    vi.spyOn(skillLifecycleApi, 'deleteSkill').mockRejectedValue(new ApiError('session expired', 401))
+    render(<App />)
+    const location = { href: '' }
+    // Override only location on a delegated window; React still sees the real DOM.
+    const testWindow = Object.create(window)
+    Object.defineProperty(testWindow, 'location', { value: location })
+    vi.stubGlobal('window', testWindow)
+    fireEvent.click(screen.getByRole('button', { name: 'Delete' }))
+    await waitFor(() => expect(location.href).toBe('/login'))
+    expect(toast.error).toHaveBeenCalledTimes(1)
+    expect(toast.success).not.toHaveBeenCalled()
+  })
+
+  it('retains global error feedback for mutations without local ownership', async () => {
+    render(<App />)
+    fireEvent.click(screen.getByRole('button', { name: 'Other request' }))
+    await waitFor(() => expect(toast.error).toHaveBeenCalledExactlyOnceWith(blockedError.serverMessage))
+  })
+})