From 84005bd25b84fe1a77407cb789eb406dd8feafb6 Mon Sep 17 00:00:00 2001 From: yyh Date: Tue, 24 Mar 2026 23:21:24 +0800 Subject: [PATCH] Align skill tree menu behaviors --- .../tree/node-delete-confirm-dialog.tsx | 70 ++++++ .../file-tree/tree/tree-context-menu.spec.tsx | 21 +- .../file-tree/tree/tree-context-menu.tsx | 65 ++---- .../skill/file-tree/tree/tree-node.spec.tsx | 18 ++ .../skill/file-tree/tree/tree-node.tsx | 200 ++++++++++-------- .../operations/use-file-operations.ts | 9 +- .../operations/use-modify-operations.ts | 6 +- .../operations/use-paste-operation.spec.tsx | 27 +++ .../operations/use-paste-operation.ts | 14 +- .../skill-body/sidebar-search-add.spec.tsx | 16 ++ .../skill/skill-body/sidebar-search-add.tsx | 25 ++- 11 files changed, 313 insertions(+), 158 deletions(-) create mode 100644 web/app/components/workflow/skill/file-tree/tree/node-delete-confirm-dialog.tsx diff --git a/web/app/components/workflow/skill/file-tree/tree/node-delete-confirm-dialog.tsx b/web/app/components/workflow/skill/file-tree/tree/node-delete-confirm-dialog.tsx new file mode 100644 index 0000000000..5ff302139e --- /dev/null +++ b/web/app/components/workflow/skill/file-tree/tree/node-delete-confirm-dialog.tsx @@ -0,0 +1,70 @@ +'use client' + +import * as React from 'react' +import { useTranslation } from 'react-i18next' +import { + AlertDialog, + AlertDialogActions, + AlertDialogCancelButton, + AlertDialogConfirmButton, + AlertDialogContent, + AlertDialogDescription, + AlertDialogTitle, +} from '@/app/components/base/ui/alert-dialog' + +type NodeDeleteConfirmDialogProps = { + nodeType: 'file' | 'folder' + open: boolean + isDeleting: boolean + onConfirm: () => void + onCancel: () => void +} + +const NodeDeleteConfirmDialog = ({ + nodeType, + open, + isDeleting, + onConfirm, + onCancel, +}: NodeDeleteConfirmDialogProps) => { + const { t } = useTranslation('workflow') + const isFolder = nodeType === 'folder' + + return ( + { + if (!nextOpen) + onCancel() + }} + > + +
+ + {isFolder + ? t('skillSidebar.menu.deleteConfirmTitle') + : t('skillSidebar.menu.fileDeleteConfirmTitle')} + + + {isFolder + ? t('skillSidebar.menu.deleteConfirmContent') + : t('skillSidebar.menu.fileDeleteConfirmContent')} + +
+ + + {t('operation.cancel', { ns: 'common' })} + + + {t('operation.confirm', { ns: 'common' })} + + +
+
+ ) +} + +export default React.memo(NodeDeleteConfirmDialog) diff --git a/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.spec.tsx b/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.spec.tsx index a645180445..241be57ee4 100644 --- a/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.spec.tsx +++ b/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.spec.tsx @@ -4,9 +4,11 @@ import TreeContextMenu from './tree-context-menu' const mocks = vi.hoisted(() => ({ clearSelection: vi.fn(), + setSelectedNodeIds: vi.fn(), deselectAll: vi.fn(), getNode: vi.fn(), selectNode: vi.fn(), + useFileOperations: vi.fn(), fileOperations: { fileInputRef: { current: null }, folderInputRef: { current: null }, @@ -29,6 +31,7 @@ vi.mock('@/app/components/workflow/store', () => ({ useWorkflowStore: () => ({ getState: () => ({ clearSelection: mocks.clearSelection, + setSelectedNodeIds: mocks.setSelectedNodeIds, }), }), })) @@ -53,7 +56,10 @@ vi.mock('next/dynamic', () => ({ })) vi.mock('../../hooks/file-tree/operations/use-file-operations', () => ({ - useFileOperations: () => mocks.fileOperations, + useFileOperations: (...args: unknown[]) => { + mocks.useFileOperations(...args) + return mocks.fileOperations + }, })) vi.mock('./node-menu', () => ({ @@ -112,7 +118,10 @@ describe('TreeContextMenu', () => { }) it('should switch to item menu when a tree node is right clicked', () => { - mocks.getNode.mockReturnValue({ select: mocks.selectNode }) + mocks.getNode.mockReturnValue({ + select: mocks.selectNode, + data: { name: 'readme.md' }, + }) render( @@ -128,11 +137,17 @@ describe('TreeContextMenu', () => { fireEvent.contextMenu(screen.getByRole('treeitem')) expect(mocks.getNode).toHaveBeenCalledWith('file-1') + expect(mocks.deselectAll).toHaveBeenCalledTimes(1) expect(mocks.selectNode).toHaveBeenCalledTimes(1) + expect(mocks.setSelectedNodeIds).toHaveBeenCalledWith(['file-1']) expect(mocks.clearSelection).not.toHaveBeenCalled() - expect(mocks.deselectAll).not.toHaveBeenCalled() expect(screen.getByTestId('node-menu-context')).toHaveAttribute('data-type', 'file') expect(screen.getByTestId('node-menu-context')).toHaveAttribute('data-node-id', 'file-1') + expect(mocks.useFileOperations).toHaveBeenLastCalledWith(expect.objectContaining({ + nodeId: 'file-1', + nodeType: 'file', + fileName: 'readme.md', + })) }) it('should keep import modal mounted after root menu requests it', () => { diff --git a/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.tsx b/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.tsx index 263e6c3565..105e60950d 100644 --- a/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.tsx +++ b/web/app/components/workflow/skill/file-tree/tree/tree-context-menu.tsx @@ -4,16 +4,6 @@ import type { TreeApi } from 'react-arborist' import type { TreeNodeData } from '../../type' import * as React from 'react' import { useState } from 'react' -import { useTranslation } from 'react-i18next' -import { - AlertDialog, - AlertDialogActions, - AlertDialogCancelButton, - AlertDialogConfirmButton, - AlertDialogContent, - AlertDialogDescription, - AlertDialogTitle, -} from '@/app/components/base/ui/alert-dialog' import { ContextMenu, ContextMenuContent, @@ -23,6 +13,7 @@ import { useWorkflowStore } from '@/app/components/workflow/store' import dynamic from '@/next/dynamic' import { NODE_MENU_TYPE, ROOT_ID } from '../../constants' import { useFileOperations } from '../../hooks/file-tree/operations/use-file-operations' +import NodeDeleteConfirmDialog from './node-delete-confirm-dialog' import NodeMenu from './node-menu' const ImportSkillModal = dynamic(() => import('../../start-tab/import-skill-modal'), { @@ -41,6 +32,7 @@ type TreeContextMenuProps = Omit< type MenuTarget = { nodeId: string type: typeof NODE_MENU_TYPE.ROOT | typeof NODE_MENU_TYPE.FOLDER | typeof NODE_MENU_TYPE.FILE + fileName?: string } const defaultMenuTarget: MenuTarget = { @@ -54,7 +46,6 @@ const TreeContextMenu = ({ children, ...props }: TreeContextMenuProps) => { - const { t } = useTranslation('workflow') const storeApi = useWorkflowStore() const [menuTarget, setMenuTarget] = useState(defaultMenuTarget) const [isImportModalOpen, setIsImportModalOpen] = useState(false) @@ -76,10 +67,14 @@ const TreeContextMenu = ({ if (!nodeId || (nodeType !== NODE_MENU_TYPE.FILE && nodeType !== NODE_MENU_TYPE.FOLDER)) return - treeRef.current?.get(nodeId)?.select() + const targetNode = treeRef.current?.get(nodeId) + treeRef.current?.deselectAll?.() + targetNode?.select() + storeApi.getState().setSelectedNodeIds([nodeId]) setMenuTarget({ nodeId, type: nodeType, + fileName: targetNode?.data.name, }) }, [storeApi, treeRef]) @@ -88,18 +83,14 @@ const TreeContextMenu = ({ nodeId: menuTarget.nodeId, treeRef, onClose: handleMenuClose, + nodeType: menuTarget.type === NODE_MENU_TYPE.ROOT ? undefined : menuTarget.type, + fileName: menuTarget.fileName, }) const handleOpenImportSkills = React.useCallback(() => { setIsImportModalOpen(true) }, []) const isRootTarget = menuTarget.type === NODE_MENU_TYPE.ROOT - const isFolderTarget = menuTarget.type === NODE_MENU_TYPE.FOLDER - const deleteConfirmTitle = isFolderTarget - ? t('skillSidebar.menu.deleteConfirmTitle') - : t('skillSidebar.menu.fileDeleteConfirmTitle') - const deleteConfirmContent = isFolderTarget - ? t('skillSidebar.menu.deleteConfirmContent') - : t('skillSidebar.menu.fileDeleteConfirmContent') + const deleteNodeType = menuTarget.type === NODE_MENU_TYPE.FOLDER ? 'folder' : 'file' return ( <> @@ -132,37 +123,15 @@ const TreeContextMenu = ({ {!isRootTarget && ( - { - if (!open) - fileOperations.handleDeleteCancel() + isDeleting={fileOperations.isDeleting} + onConfirm={() => { + void fileOperations.handleDeleteConfirm() }} - > - -
- - {deleteConfirmTitle} - - - {deleteConfirmContent} - -
- - - {t('operation.cancel', { ns: 'common' })} - - { - void fileOperations.handleDeleteConfirm() - }} - > - {t('operation.confirm', { ns: 'common' })} - - -
-
+ onCancel={fileOperations.handleDeleteCancel} + /> )} { expect(storeActions.setDragOverFolderId).toHaveBeenCalledWith(null) }) }) + + describe('Dialogs', () => { + it('should keep delete confirmation dialog mounted when requested by dropdown actions', () => { + fileOperationMocks.showDeleteConfirm = true + const props = buildProps({ id: 'file-1', name: 'readme.md', nodeType: 'file' }) + + render() + + expect(screen.getByText('workflow.skillSidebar.menu.fileDeleteConfirmTitle')).toBeInTheDocument() + expect(screen.getByText('workflow.skillSidebar.menu.fileDeleteConfirmContent')).toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: /common\.operation\.confirm/i })) + fireEvent.click(screen.getByRole('button', { name: /common\.operation\.cancel/i })) + + expect(fileOperationMocks.handleDeleteConfirm).toHaveBeenCalledTimes(1) + expect(fileOperationMocks.handleDeleteCancel).toHaveBeenCalledTimes(1) + }) + }) }) diff --git a/web/app/components/workflow/skill/file-tree/tree/tree-node.tsx b/web/app/components/workflow/skill/file-tree/tree/tree-node.tsx index 834a05a2f7..119b1b3586 100644 --- a/web/app/components/workflow/skill/file-tree/tree/tree-node.tsx +++ b/web/app/components/workflow/skill/file-tree/tree/tree-node.tsx @@ -16,6 +16,7 @@ import { NODE_MENU_TYPE } from '../../constants' import { useFolderFileDrop } from '../../hooks/file-tree/dnd/use-folder-file-drop' import { useTreeNodeHandlers } from '../../hooks/file-tree/interaction/use-tree-node-handlers' import { useFileOperations } from '../../hooks/file-tree/operations/use-file-operations' +import NodeDeleteConfirmDialog from './node-delete-confirm-dialog' import NodeMenu from './node-menu' import TreeEditInput from './tree-edit-input' import TreeGuideLines from './tree-guide-lines' @@ -90,106 +91,117 @@ const TreeNode = ({ node, style, dragHandle, treeChildren }: TreeNodeProps) => { }) return ( -
- + <>
-
- + +
+
+ +
+ + {node.isEditing + ? ( + + ) + : ( + + {node.data.name} + + )}
- {node.isEditing - ? ( - - ) - : ( - - {node.data.name} - + + + + + + +
- - - - - - - - -
+ { + void fileOperations.handleDeleteConfirm() + }} + onCancel={fileOperations.handleDeleteCancel} + /> + ) } diff --git a/web/app/components/workflow/skill/hooks/file-tree/operations/use-file-operations.ts b/web/app/components/workflow/skill/hooks/file-tree/operations/use-file-operations.ts index 3cf5f73197..03e1b2f22f 100644 --- a/web/app/components/workflow/skill/hooks/file-tree/operations/use-file-operations.ts +++ b/web/app/components/workflow/skill/hooks/file-tree/operations/use-file-operations.ts @@ -18,6 +18,8 @@ type UseFileOperationsOptions = { onClose: () => void treeRef?: React.RefObject | null> node?: NodeApi + nodeType?: TreeNodeData['node_type'] + fileName?: string } export function useFileOperations({ @@ -25,8 +27,12 @@ export function useFileOperations({ onClose, treeRef, node, + nodeType: explicitNodeType, + fileName: explicitFileName, }: UseFileOperationsOptions) { const nodeId = node?.data.id ?? explicitNodeId ?? '' + const nodeType = node?.data.node_type ?? explicitNodeType + const fileName = node?.data.name ?? explicitFileName const appDetail = useAppStore(s => s.appDetail) const appId = appDetail?.id || '' @@ -46,6 +52,7 @@ export function useFileOperations({ nodeId, node, treeRef, + nodeType, appId, storeApi, treeData, @@ -55,7 +62,7 @@ export function useFileOperations({ const downloadOps = useDownloadOperation({ appId, nodeId, - fileName: node?.data.name, + fileName, onClose, }) diff --git a/web/app/components/workflow/skill/hooks/file-tree/operations/use-modify-operations.ts b/web/app/components/workflow/skill/hooks/file-tree/operations/use-modify-operations.ts index 5ffd6926b5..ad07a18c3d 100644 --- a/web/app/components/workflow/skill/hooks/file-tree/operations/use-modify-operations.ts +++ b/web/app/components/workflow/skill/hooks/file-tree/operations/use-modify-operations.ts @@ -18,6 +18,7 @@ type UseModifyOperationsOptions = { nodeId: string node?: NodeApi treeRef?: React.RefObject | null> + nodeType?: TreeNodeData['node_type'] appId: string storeApi: StoreApi treeData?: AppAssetTreeResponse @@ -28,6 +29,7 @@ export function useModifyOperations({ nodeId, node, treeRef, + nodeType, appId, storeApi, treeData, @@ -54,7 +56,7 @@ export function useModifyOperations({ }, []) const handleDeleteConfirm = useCallback(async () => { - const isFolder = node?.data?.node_type === 'folder' + const isFolder = (node?.data?.node_type ?? nodeType) === 'folder' try { const descendantFileIds = treeData?.children ? getAllDescendantFileIds(nodeId, treeData.children) @@ -91,7 +93,7 @@ export function useModifyOperations({ setShowDeleteConfirm(false) onClose() } - }, [appId, nodeId, node?.data?.node_type, deleteNodeAsync, storeApi, treeData?.children, onClose, t, emitTreeUpdate]) + }, [appId, nodeId, node?.data?.node_type, nodeType, deleteNodeAsync, storeApi, treeData?.children, onClose, t, emitTreeUpdate]) const handleDeleteCancel = useCallback(() => { setShowDeleteConfirm(false) diff --git a/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.spec.tsx b/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.spec.tsx index 15fa155430..e2f2f8f33f 100644 --- a/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.spec.tsx +++ b/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.spec.tsx @@ -61,6 +61,7 @@ const mocks = vi.hoisted(() => ({ getTargetFolderIdFromSelection: vi.fn<(selectedId: string | null, nodes: TreeNodeData[]) => string>(), toApiParentId: vi.fn<(folderId: string | null | undefined) => string | null>(), findNodeById: vi.fn<(nodes: TreeNodeData[], nodeId: string) => TreeNodeData | null>(), + isDescendantOf: vi.fn<(potentialDescendantId: string | null | undefined, ancestorId: string | null | undefined, nodes: TreeNodeData[]) => boolean>(), })) vi.mock('@/app/components/workflow/store', () => ({ @@ -88,6 +89,7 @@ vi.mock('../../../utils/tree-utils', () => ({ getTargetFolderIdFromSelection: mocks.getTargetFolderIdFromSelection, toApiParentId: mocks.toApiParentId, findNodeById: mocks.findNodeById, + isDescendantOf: mocks.isDescendantOf, })) vi.mock('@/app/components/base/ui/toast', () => ({ @@ -127,6 +129,7 @@ describe('usePasteOperation', () => { mocks.getTargetFolderIdFromSelection.mockReturnValue('target-folder') mocks.toApiParentId.mockReturnValue('target-parent') mocks.findNodeById.mockReturnValue(null) + mocks.isDescendantOf.mockReturnValue(false) }) // Scenario: isPasting output should reflect mutation pending state. @@ -193,6 +196,30 @@ describe('usePasteOperation', () => { expect(mocks.moveMutateAsync).not.toHaveBeenCalled() expect(mocks.toastError).toHaveBeenCalledWith('workflow.skillSidebar.menu.cannotMoveToSelf') }) + + it('should reject moving folder into its descendant and show error toast', async () => { + mocks.workflowState.clipboard = { + operation: 'cut', + nodeIds: new Set(['folder-1']), + } + mocks.getTargetFolderIdFromSelection.mockReturnValueOnce('child-folder') + mocks.findNodeById + .mockReturnValueOnce(createTreeNode('folder-1', 'folder')) + .mockReturnValueOnce(createTreeNode('folder-1', 'folder')) + mocks.isDescendantOf.mockReturnValueOnce(true) + const treeRef = createTreeRef('child-folder') + const treeData: AppAssetTreeResponse = { + children: [createTreeNode('folder-1', 'folder')], + } + const { result } = renderHook(() => usePasteOperation({ treeRef, treeData })) + + await act(async () => { + await result.current.handlePaste() + }) + + expect(mocks.moveMutateAsync).not.toHaveBeenCalled() + expect(mocks.toastError).toHaveBeenCalledWith('workflow.skillSidebar.menu.cannotMoveToDescendant') + }) }) // Scenario: successful cut-paste should move all nodes and clear clipboard. diff --git a/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.ts b/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.ts index 3e6bb855ac..0c2099ac0e 100644 --- a/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.ts +++ b/web/app/components/workflow/skill/hooks/file-tree/operations/use-paste-operation.ts @@ -10,7 +10,7 @@ import { useStore as useAppStore } from '@/app/components/app/store' import { toast } from '@/app/components/base/ui/toast' import { useWorkflowStore } from '@/app/components/workflow/store' import { useMoveAppAssetNode } from '@/service/use-app-asset' -import { findNodeById, getTargetFolderIdFromSelection, toApiParentId } from '../../../utils/tree-utils' +import { findNodeById, getTargetFolderIdFromSelection, isDescendantOf, toApiParentId } from '../../../utils/tree-utils' import { useSkillTreeUpdateEmitter } from '../data/use-skill-tree-collaboration' type UsePasteOperationOptions = { @@ -64,11 +64,23 @@ export function usePasteOperation({ return false }) + const isMovingToDescendant = nodeIdsArray.some((nodeId) => { + const node = findNodeById(treeChildren, nodeId) + if (!node || node.node_type !== 'folder') + return false + return isDescendantOf(targetFolderId, nodeId, treeChildren) + }) + if (isMovingToSelf) { toast.error(t('skillSidebar.menu.cannotMoveToSelf')) return } + if (isMovingToDescendant) { + toast.error(t('skillSidebar.menu.cannotMoveToDescendant')) + return + } + isPastingRef.current = true try { diff --git a/web/app/components/workflow/skill/skill-body/sidebar-search-add.spec.tsx b/web/app/components/workflow/skill/skill-body/sidebar-search-add.spec.tsx index 856130dcbc..759a8f4a36 100644 --- a/web/app/components/workflow/skill/skill-body/sidebar-search-add.spec.tsx +++ b/web/app/components/workflow/skill/skill-body/sidebar-search-add.spec.tsx @@ -176,6 +176,22 @@ describe('SidebarSearchAdd', () => { // Assert expect(screen.queryByTestId('import-skill-modal')).not.toBeInTheDocument() }) + + it('should hide import skills action when target folder is not root', () => { + mocks.storeState.selectedTreeNodeId = 'folder-1' + mocks.treeData = { + children: [ + createNode({ + id: 'folder-1', + }), + ], + } + + render() + fireEvent.click(screen.getByRole('button', { name: /common\.operation\.add/i })) + + expect(screen.queryByRole('menuitem', { name: /workflow\.skillSidebar\.menu\.importSkills/i })).not.toBeInTheDocument() + }) }) describe('Data flow', () => { diff --git a/web/app/components/workflow/skill/skill-body/sidebar-search-add.tsx b/web/app/components/workflow/skill/skill-body/sidebar-search-add.tsx index 516aba130a..624e3c97af 100644 --- a/web/app/components/workflow/skill/skill-body/sidebar-search-add.tsx +++ b/web/app/components/workflow/skill/skill-body/sidebar-search-add.tsx @@ -41,6 +41,7 @@ const SidebarSearchAdd = () => { return ROOT_ID return getTargetFolderIdFromSelection(selectedTreeNodeId, treeChildren) }, [selectedTreeNodeId, treeChildren]) + const isRootTarget = targetFolderId === ROOT_ID const handleMenuClose = useCallback(() => {}, []) const { @@ -128,16 +129,22 @@ const SidebarSearchAdd = () => { disabled={isLoading} /> - + {isRootTarget + ? ( + <> + - setIsImportModalOpen(true)} - disabled={isLoading} - tooltip={t('skill.startTab.importSkillDesc')} - /> + setIsImportModalOpen(true)} + disabled={isLoading} + tooltip={t('skill.startTab.importSkillDesc')} + /> + + ) + : null}