From cdcd9fd1a2f789d6b1224802ed3837a383576b99 Mon Sep 17 00:00:00 2001 From: yyh Date: Sun, 25 Jan 2026 21:17:25 +0800 Subject: [PATCH] refactor(skill-editor): lift Ctrl+S handler to Provider and remove redundant hook Move global keyboard shortcut handling from component-level hook to SkillSaveProvider, eliminating duplicate event listener registrations and race conditions. Delete use-skill-file-save hook as its logic is now consolidated in the provider with direct store access. --- .../workflow/skill/file-content-panel.tsx | 17 +- .../skill/hooks/use-skill-file-save.spec.tsx | 209 ------------------ .../skill/hooks/use-skill-file-save.ts | 63 ------ .../skill/hooks/use-skill-save-manager.tsx | 29 +++ 4 files changed, 33 insertions(+), 285 deletions(-) delete mode 100644 web/app/components/workflow/skill/hooks/use-skill-file-save.spec.tsx delete mode 100644 web/app/components/workflow/skill/hooks/use-skill-file-save.ts diff --git a/web/app/components/workflow/skill/file-content-panel.tsx b/web/app/components/workflow/skill/file-content-panel.tsx index 944d4a8f45..ad87d7ae80 100644 --- a/web/app/components/workflow/skill/file-content-panel.tsx +++ b/web/app/components/workflow/skill/file-content-panel.tsx @@ -19,7 +19,6 @@ import MarkdownFileEditor from './editor/markdown-file-editor' import { useFileTypeInfo } from './hooks/use-file-type-info' import { useSkillAssetNodeMap } from './hooks/use-skill-asset-tree' import { useSkillFileData } from './hooks/use-skill-file-data' -import { useSkillFileSave } from './hooks/use-skill-file-save' import { useSkillSaveManager } from './hooks/use-skill-save-manager' import StartTabContent from './start-tab' import { getFileLanguage } from './utils/file-utils' @@ -100,25 +99,17 @@ const FileContentPanel: FC = () => { storeApi.getState().pinTab(fileTabId) }, [fileTabId, isEditable, originalContent, storeApi]) - useSkillFileSave({ - appId, - activeTabId: fileTabId, - isEditable, - originalContent, - currentMetadata, - t, - }) - const { saveFile, registerFallback, unregisterFallback } = useSkillSaveManager() const fallbackRef = useRef({ content: originalContent, metadata: currentMetadata }) - fallbackRef.current = { content: originalContent, metadata: currentMetadata } useEffect(() => { if (!fileTabId || fileContent?.content === undefined) return - registerFallback(fileTabId, { content: originalContent, metadata: currentMetadata }) + const fallback = { content: originalContent, metadata: currentMetadata } + fallbackRef.current = fallback + registerFallback(fileTabId, fallback) return () => { unregisterFallback(fileTabId) @@ -129,8 +120,8 @@ const FileContentPanel: FC = () => { if (!fileTabId || !isEditable) return - const { content: fallbackContent, metadata: fallbackMetadata } = fallbackRef.current return () => { + const { content: fallbackContent, metadata: fallbackMetadata } = fallbackRef.current void saveFile(fileTabId, { fallbackContent, fallbackMetadata, diff --git a/web/app/components/workflow/skill/hooks/use-skill-file-save.spec.tsx b/web/app/components/workflow/skill/hooks/use-skill-file-save.spec.tsx deleted file mode 100644 index ed6d992c2f..0000000000 --- a/web/app/components/workflow/skill/hooks/use-skill-file-save.spec.tsx +++ /dev/null @@ -1,209 +0,0 @@ -import type { SaveFileOptions, SaveResult } from './use-skill-save-manager' -import { act, renderHook, waitFor } from '@testing-library/react' -import Toast from '@/app/components/base/toast' -import { useSkillFileSave } from './use-skill-file-save' - -const { mockSaveFile, mockToastNotify } = vi.hoisted(() => ({ - mockSaveFile: vi.fn<(fileId: string, options?: SaveFileOptions) => Promise>(), - mockToastNotify: vi.fn(), -})) - -vi.mock('./use-skill-save-manager', () => ({ - useSkillSaveManager: () => ({ - saveFile: mockSaveFile, - }), -})) - -vi.mock('@/app/components/base/toast', () => ({ - default: { - notify: mockToastNotify, - }, -})) - -const createParams = (overrides: Partial[0]> = {}) => ({ - appId: 'app-1', - activeTabId: 'file-1' as string | null, - isEditable: true, - originalContent: 'original content', - currentMetadata: { version: 1 } as Record, - t: vi.fn(() => 'saved-message') as unknown as Parameters[0]['t'], - ...overrides, -}) - -// Scenario: save behavior and shortcut handling for skill file saves. -describe('useSkillFileSave', () => { - beforeEach(() => { - vi.clearAllMocks() - mockSaveFile.mockResolvedValue({ saved: false }) - }) - - // Scenario: guard clauses prevent invalid saves. - describe('Guards', () => { - it('should return early when active tab id is missing', async () => { - // Arrange - const params = createParams({ activeTabId: null }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(mockSaveFile).not.toHaveBeenCalled() - expect(Toast.notify).not.toHaveBeenCalled() - }) - - it('should return early when app id is empty', async () => { - // Arrange - const params = createParams({ appId: '' }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(mockSaveFile).not.toHaveBeenCalled() - expect(Toast.notify).not.toHaveBeenCalled() - }) - - it('should return early when not editable', async () => { - // Arrange - const params = createParams({ isEditable: false }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(mockSaveFile).not.toHaveBeenCalled() - expect(Toast.notify).not.toHaveBeenCalled() - }) - }) - - // Scenario: save results surface as toast notifications. - describe('Save Results', () => { - it('should call saveFile with fallback data when valid', async () => { - // Arrange - const params = createParams({ - originalContent: 'fallback content', - currentMetadata: { tag: 'v1' }, - }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(mockSaveFile).toHaveBeenCalledWith('file-1', { - fallbackContent: 'fallback content', - fallbackMetadata: { tag: 'v1' }, - }) - expect(Toast.notify).not.toHaveBeenCalled() - }) - - it('should show error toast when save fails', async () => { - // Arrange - const params = createParams() - mockSaveFile.mockResolvedValueOnce({ saved: false, error: new Error('boom') }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(Toast.notify).toHaveBeenCalledWith({ - type: 'error', - message: 'Error: boom', - }) - expect(params.t).not.toHaveBeenCalled() - }) - - it('should show success toast when save succeeds', async () => { - // Arrange - const params = createParams() - mockSaveFile.mockResolvedValueOnce({ saved: true }) - const { result } = renderHook(() => useSkillFileSave(params)) - - // Act - await act(async () => { - await result.current() - }) - - // Assert - expect(params.t).toHaveBeenCalledWith('api.saved', { ns: 'common' }) - expect(Toast.notify).toHaveBeenCalledWith({ - type: 'success', - message: 'saved-message', - }) - }) - }) - - // Scenario: Ctrl/Cmd+S triggers save and suppresses default behavior. - describe('Keyboard Shortcut', () => { - it('should trigger save on Ctrl+S', async () => { - // Arrange - const params = createParams() - renderHook(() => useSkillFileSave(params)) - const event = new KeyboardEvent('keydown', { key: 's', ctrlKey: true, cancelable: true }) - const preventDefault = vi.fn() - Object.defineProperty(event, 'preventDefault', { value: preventDefault }) - - // Act - act(() => { - window.dispatchEvent(event) - }) - - // Assert - await waitFor(() => { - expect(mockSaveFile).toHaveBeenCalled() - }) - expect(preventDefault).toHaveBeenCalled() - }) - - it('should trigger save on Cmd+S', async () => { - // Arrange - const params = createParams() - renderHook(() => useSkillFileSave(params)) - const event = new KeyboardEvent('keydown', { key: 's', metaKey: true, cancelable: true }) - const preventDefault = vi.fn() - Object.defineProperty(event, 'preventDefault', { value: preventDefault }) - - // Act - act(() => { - window.dispatchEvent(event) - }) - - // Assert - await waitFor(() => { - expect(mockSaveFile).toHaveBeenCalled() - }) - expect(preventDefault).toHaveBeenCalled() - }) - - it('should not trigger save when key is not s', async () => { - // Arrange - const params = createParams() - renderHook(() => useSkillFileSave(params)) - const event = new KeyboardEvent('keydown', { key: 'x', ctrlKey: true, cancelable: true }) - - // Act - act(() => { - window.dispatchEvent(event) - }) - - // Assert - await waitFor(() => { - expect(mockSaveFile).not.toHaveBeenCalled() - }) - }) - }) -}) diff --git a/web/app/components/workflow/skill/hooks/use-skill-file-save.ts b/web/app/components/workflow/skill/hooks/use-skill-file-save.ts deleted file mode 100644 index e3295b2b50..0000000000 --- a/web/app/components/workflow/skill/hooks/use-skill-file-save.ts +++ /dev/null @@ -1,63 +0,0 @@ -import type { TFunction } from 'i18next' -import { useEventListener } from 'ahooks' -import { useCallback } from 'react' -import Toast from '@/app/components/base/toast' -import { useSkillSaveManager } from './use-skill-save-manager' - -type UseSkillFileSaveParams = { - appId: string - activeTabId: string | null - isEditable: boolean - originalContent: string - currentMetadata: Record | undefined - t: TFunction<'workflow'> -} - -/** - * Hook to handle file save logic and Ctrl+S keyboard shortcut. - * Returns the save handler function. - */ -export function useSkillFileSave({ - appId, - activeTabId, - isEditable, - originalContent, - currentMetadata, - t, -}: UseSkillFileSaveParams): () => Promise { - const { saveFile } = useSkillSaveManager() - - const handleSave = useCallback(async () => { - if (!activeTabId || !appId || !isEditable) - return - - const result = await saveFile(activeTabId, { - fallbackContent: originalContent, - fallbackMetadata: currentMetadata, - }) - - if (result.error) { - Toast.notify({ - type: 'error', - message: String(result.error), - }) - return - } - - if (result.saved) { - Toast.notify({ - type: 'success', - message: t('api.saved', { ns: 'common' }), - }) - } - }, [activeTabId, appId, currentMetadata, isEditable, originalContent, saveFile, t]) - - useEventListener('keydown', (e: KeyboardEvent) => { - if ((e.ctrlKey || e.metaKey) && e.key === 's') { - e.preventDefault() - handleSave() - } - }, { target: window }) - - return handleSave -} diff --git a/web/app/components/workflow/skill/hooks/use-skill-save-manager.tsx b/web/app/components/workflow/skill/hooks/use-skill-save-manager.tsx index d7eb433f0a..9f7a6217e3 100644 --- a/web/app/components/workflow/skill/hooks/use-skill-save-manager.tsx +++ b/web/app/components/workflow/skill/hooks/use-skill-save-manager.tsx @@ -1,7 +1,10 @@ import { useQueryClient } from '@tanstack/react-query' +import { useEventListener } from 'ahooks' import isDeepEqual from 'fast-deep-equal' import * as React from 'react' import { useCallback, useMemo, useRef } from 'react' +import { useTranslation } from 'react-i18next' +import Toast from '@/app/components/base/toast' import { useWorkflowStore } from '@/app/components/workflow/store' import { consoleQuery } from '@/service/client' import { useUpdateAppAssetFileContent } from '@/service/use-app-asset' @@ -53,6 +56,7 @@ export const SkillSaveProvider = ({ appId, children, }: SkillSaveProviderProps) => { + const { t } = useTranslation() const storeApi = useWorkflowStore() const queryClient = useQueryClient() const updateContent = useUpdateAppAssetFileContent() @@ -225,6 +229,31 @@ export const SkillSaveProvider = ({ fallbackRegistryRef.current.delete(fileId) }, []) + useEventListener('keydown', (e: KeyboardEvent) => { + if ((e.ctrlKey || e.metaKey) && e.key === 's') { + e.preventDefault() + const { activeTabId } = storeApi.getState() + if (!activeTabId || activeTabId === START_TAB_ID) + return + + const fallback = fallbackRegistryRef.current.get(activeTabId) + void saveFile(activeTabId, { + fallbackContent: fallback?.content, + fallbackMetadata: fallback?.metadata, + }).then((result) => { + if (result.error) { + const errorMessage = result.error instanceof Error + ? result.error.message + : String(result.error) + Toast.notify({ type: 'error', message: errorMessage }) + } + else if (result.saved) { + Toast.notify({ type: 'success', message: t('api.saved', { ns: 'common' }) }) + } + }) + } + }, { target: typeof window !== 'undefined' ? window : undefined }) + const value = useMemo(() => ({ saveFile, saveAllDirty,