From dc213ca76cd672958a60d687f04c6b0cff5a8036 Mon Sep 17 00:00:00 2001 From: yyh Date: Fri, 6 Feb 2026 15:34:27 +0800 Subject: [PATCH] refactor(skill)!: add file node view-state flow and mode-based file data hook - introduce resolving/ready/missing node view-state to avoid unsupported flicker - switch useSkillFileData to explicit mode: none/content/download - add hook tests for view-state transitions and mode query gating BREAKING CHANGE: useSkillFileData signature changed from (appId, nodeId, isEditable) to (appId, nodeId, mode). --- .../workflow/skill/file-content-panel.tsx | 44 ++++++- .../hooks/use-file-node-view-state.spec.tsx | 115 ++++++++++++++++++ .../skill/hooks/use-file-node-view-state.ts | 73 +++++++++++ .../skill/hooks/use-skill-file-data.spec.tsx | 86 +++++++++++++ .../skill/hooks/use-skill-file-data.ts | 25 +++- 5 files changed, 334 insertions(+), 9 deletions(-) create mode 100644 web/app/components/workflow/skill/hooks/use-file-node-view-state.spec.tsx create mode 100644 web/app/components/workflow/skill/hooks/use-file-node-view-state.ts create mode 100644 web/app/components/workflow/skill/hooks/use-skill-file-data.spec.tsx diff --git a/web/app/components/workflow/skill/file-content-panel.tsx b/web/app/components/workflow/skill/file-content-panel.tsx index 60ed079486..56f25c65f3 100644 --- a/web/app/components/workflow/skill/file-content-panel.tsx +++ b/web/app/components/workflow/skill/file-content-panel.tsx @@ -1,6 +1,7 @@ 'use client' import type { OnMount } from '@monaco-editor/react' +import type { SkillFileDataMode } from './hooks/use-skill-file-data' import type { AppAssetTreeView } from '@/types/app-asset' import { loader } from '@monaco-editor/react' import isDeepEqual from 'fast-deep-equal' @@ -20,6 +21,7 @@ import { START_TAB_ID } from './constants' import CodeFileEditor from './editor/code-file-editor' import MarkdownFileEditor from './editor/markdown-file-editor' import { useSkillSaveManager } from './hooks/skill-save-context' +import { useFileNodeViewState } from './hooks/use-file-node-view-state' import { useFileTypeInfo } from './hooks/use-file-type-info' import { useSkillAssetNodeMap } from './hooks/use-skill-asset-tree' import { useSkillFileData } from './hooks/use-skill-file-data' @@ -69,7 +71,12 @@ const FileContentPanel = () => { const activeTabId = useStore(s => s.activeTabId) const editorAutoFocusFileId = useStore(s => s.editorAutoFocusFileId) const storeApi = useWorkflowStore() - const { data: nodeMap } = useSkillAssetNodeMap() + const { + data: nodeMap, + isLoading: isNodeMapLoading, + isFetching: isNodeMapFetching, + isFetched: isNodeMapFetched, + } = useSkillAssetNodeMap() const isStartTab = activeTabId === START_TAB_ID const fileTabId = isStartTab ? null : activeTabId @@ -80,10 +87,23 @@ const FileContentPanel = () => { const currentFileNode = fileTabId ? nodeMap?.get(fileTabId) : undefined const shouldAutoFocusEditor = Boolean(fileTabId && editorAutoFocusFileId === fileTabId) + const fileNodeViewState = useFileNodeViewState({ + fileTabId, + hasCurrentFileNode: Boolean(currentFileNode), + isNodeMapLoading, + isNodeMapFetching, + isNodeMapFetched, + }) + const isNodeReady = fileNodeViewState === 'ready' - const { isMarkdown, isCodeOrText, isImage, isVideo, isPdf, isSQLite, isEditable, isPreviewable } = useFileTypeInfo(currentFileNode) + const { isMarkdown, isCodeOrText, isImage, isVideo, isPdf, isSQLite, isEditable, isPreviewable } = useFileTypeInfo(isNodeReady ? currentFileNode : undefined) + const fileDataMode: SkillFileDataMode = !fileTabId || !isNodeReady + ? 'none' + : isEditable + ? 'content' + : 'download' - const { fileContent, downloadUrlData, isLoading, error } = useSkillFileData(appId, fileTabId, isEditable) + const { fileContent, downloadUrlData, isLoading, error } = useSkillFileData(appId, fileTabId, fileDataMode) const originalContent = fileContent?.content ?? '' const currentContent = draftContent !== undefined ? draftContent : originalContent @@ -246,6 +266,24 @@ const FileContentPanel = () => { ) } + if (fileNodeViewState === 'resolving') { + return ( +
+ +
+ ) + } + + if (fileNodeViewState === 'missing') { + return ( +
+ + {t('skillSidebar.loadError')} + +
+ ) + } + if (isLoading) { return (
diff --git a/web/app/components/workflow/skill/hooks/use-file-node-view-state.spec.tsx b/web/app/components/workflow/skill/hooks/use-file-node-view-state.spec.tsx new file mode 100644 index 0000000000..4be17f2008 --- /dev/null +++ b/web/app/components/workflow/skill/hooks/use-file-node-view-state.spec.tsx @@ -0,0 +1,115 @@ +import { renderHook, waitFor } from '@testing-library/react' +import { useFileNodeViewState } from './use-file-node-view-state' + +type HookProps = { + fileTabId: string | null + hasCurrentFileNode: boolean + isNodeMapLoading: boolean + isNodeMapFetching: boolean + isNodeMapFetched: boolean +} + +const createProps = (overrides: Partial = {}): HookProps => ({ + fileTabId: 'file-1', + hasCurrentFileNode: false, + isNodeMapLoading: true, + isNodeMapFetching: true, + isNodeMapFetched: false, + ...overrides, +}) + +describe('useFileNodeViewState', () => { + describe('resolution lifecycle', () => { + it('should return ready when there is no active file tab', () => { + const { result } = renderHook(() => useFileNodeViewState(createProps({ + fileTabId: null, + }))) + + expect(result.current).toBe('ready') + }) + + it('should return resolving during initial node resolution', () => { + const { result } = renderHook(() => useFileNodeViewState(createProps())) + + expect(result.current).toBe('resolving') + }) + + it('should return missing when query settles without a matching node', () => { + const { result, rerender } = renderHook( + (props: HookProps) => useFileNodeViewState(props), + { initialProps: createProps() }, + ) + + rerender(createProps({ + isNodeMapLoading: false, + isNodeMapFetching: false, + isNodeMapFetched: true, + })) + + expect(result.current).toBe('missing') + }) + + it('should stay missing during background refetch after missing is resolved', async () => { + const { result, rerender } = renderHook( + (props: HookProps) => useFileNodeViewState(props), + { initialProps: createProps() }, + ) + + rerender(createProps({ + isNodeMapLoading: false, + isNodeMapFetching: false, + isNodeMapFetched: true, + })) + + await waitFor(() => { + expect(result.current).toBe('missing') + }) + + rerender(createProps({ + isNodeMapLoading: false, + isNodeMapFetching: true, + isNodeMapFetched: true, + })) + + expect(result.current).toBe('missing') + }) + + it('should become ready once the target node appears', () => { + const { result, rerender } = renderHook( + (props: HookProps) => useFileNodeViewState(props), + { initialProps: createProps() }, + ) + + rerender(createProps({ + hasCurrentFileNode: true, + isNodeMapLoading: false, + isNodeMapFetching: false, + isNodeMapFetched: true, + })) + + expect(result.current).toBe('ready') + }) + + it('should reset to resolving when switching to another file tab', () => { + const { result, rerender } = renderHook( + (props: HookProps) => useFileNodeViewState(props), + { initialProps: createProps({ + isNodeMapLoading: false, + isNodeMapFetching: false, + isNodeMapFetched: true, + }) }, + ) + + expect(result.current).toBe('missing') + + rerender(createProps({ + fileTabId: 'file-2', + isNodeMapLoading: false, + isNodeMapFetching: true, + isNodeMapFetched: true, + })) + + expect(result.current).toBe('resolving') + }) + }) +}) diff --git a/web/app/components/workflow/skill/hooks/use-file-node-view-state.ts b/web/app/components/workflow/skill/hooks/use-file-node-view-state.ts new file mode 100644 index 0000000000..4c64ca2a48 --- /dev/null +++ b/web/app/components/workflow/skill/hooks/use-file-node-view-state.ts @@ -0,0 +1,73 @@ +import { useRef } from 'react' + +export type FileNodeViewState = 'resolving' | 'ready' | 'missing' + +type ResolveFileNodeViewStateParams = { + hasFileTabId: boolean + hasCurrentFileNode: boolean + isNodeMapLoading: boolean + isNodeMapFetching: boolean + isNodeMapFetched: boolean + isNodeResolutionPending: boolean +} + +type UseFileNodeViewStateParams = { + fileTabId: string | null + hasCurrentFileNode: boolean + isNodeMapLoading: boolean + isNodeMapFetching: boolean + isNodeMapFetched: boolean +} + +export const resolveFileNodeViewState = ({ + hasFileTabId, + hasCurrentFileNode, + isNodeMapLoading, + isNodeMapFetching, + isNodeMapFetched, + isNodeResolutionPending, +}: ResolveFileNodeViewStateParams): FileNodeViewState => { + if (!hasFileTabId || hasCurrentFileNode) + return 'ready' + + if (isNodeResolutionPending && (!isNodeMapFetched || isNodeMapLoading || isNodeMapFetching)) + return 'resolving' + + return 'missing' +} + +export function useFileNodeViewState({ + fileTabId, + hasCurrentFileNode, + isNodeMapLoading, + isNodeMapFetching, + isNodeMapFetched, +}: UseFileNodeViewStateParams): FileNodeViewState { + const hasFileTabId = Boolean(fileTabId) + const resolutionRef = useRef<{ tabId: string | null, pending: boolean }>({ + tabId: fileTabId, + pending: hasFileTabId, + }) + + if (resolutionRef.current.tabId !== fileTabId) { + resolutionRef.current = { + tabId: fileTabId, + pending: hasFileTabId, + } + } + + if (fileTabId && resolutionRef.current.pending) { + const isQuerySettled = isNodeMapFetched && !isNodeMapLoading && !isNodeMapFetching + if (hasCurrentFileNode || isQuerySettled) + resolutionRef.current.pending = false + } + + return resolveFileNodeViewState({ + hasFileTabId, + hasCurrentFileNode, + isNodeMapLoading, + isNodeMapFetching, + isNodeMapFetched, + isNodeResolutionPending: resolutionRef.current.pending, + }) +} diff --git a/web/app/components/workflow/skill/hooks/use-skill-file-data.spec.tsx b/web/app/components/workflow/skill/hooks/use-skill-file-data.spec.tsx new file mode 100644 index 0000000000..ee562908b8 --- /dev/null +++ b/web/app/components/workflow/skill/hooks/use-skill-file-data.spec.tsx @@ -0,0 +1,86 @@ +import { renderHook } from '@testing-library/react' +import { useSkillFileData } from './use-skill-file-data' + +const { + mockUseGetAppAssetFileContent, + mockUseGetAppAssetFileDownloadUrl, +} = vi.hoisted(() => ({ + mockUseGetAppAssetFileContent: vi.fn(), + mockUseGetAppAssetFileDownloadUrl: vi.fn(), +})) + +vi.mock('@/service/use-app-asset', () => ({ + useGetAppAssetFileContent: (...args: unknown[]) => mockUseGetAppAssetFileContent(...args), + useGetAppAssetFileDownloadUrl: (...args: unknown[]) => mockUseGetAppAssetFileDownloadUrl(...args), +})) + +describe('useSkillFileData', () => { + beforeEach(() => { + vi.clearAllMocks() + mockUseGetAppAssetFileContent.mockReturnValue({ + data: undefined, + isLoading: false, + error: null, + }) + mockUseGetAppAssetFileDownloadUrl.mockReturnValue({ + data: undefined, + isLoading: false, + error: null, + }) + }) + + describe('mode control', () => { + it('should disable both queries when mode is none', () => { + const { result } = renderHook(() => useSkillFileData('app-1', 'node-1', 'none')) + + expect(mockUseGetAppAssetFileContent).toHaveBeenCalledWith('app-1', 'node-1', { enabled: false }) + expect(mockUseGetAppAssetFileDownloadUrl).toHaveBeenCalledWith('app-1', 'node-1', { enabled: false }) + expect(result.current.isLoading).toBe(false) + expect(result.current.error).toBeNull() + }) + + it('should fetch content data when mode is content', () => { + const contentError = new Error('content-error') + mockUseGetAppAssetFileContent.mockReturnValue({ + data: { content: 'hello' }, + isLoading: true, + error: contentError, + }) + mockUseGetAppAssetFileDownloadUrl.mockReturnValue({ + data: { download_url: 'https://example.com/file' }, + isLoading: true, + error: new Error('download-error'), + }) + + const { result } = renderHook(() => useSkillFileData('app-1', 'node-1', 'content')) + + expect(mockUseGetAppAssetFileContent).toHaveBeenCalledWith('app-1', 'node-1', { enabled: true }) + expect(mockUseGetAppAssetFileDownloadUrl).toHaveBeenCalledWith('app-1', 'node-1', { enabled: false }) + expect(result.current.fileContent).toEqual({ content: 'hello' }) + expect(result.current.isLoading).toBe(true) + expect(result.current.error).toBe(contentError) + }) + + it('should fetch download URL data when mode is download', () => { + const downloadError = new Error('download-error') + mockUseGetAppAssetFileContent.mockReturnValue({ + data: { content: 'hello' }, + isLoading: true, + error: new Error('content-error'), + }) + mockUseGetAppAssetFileDownloadUrl.mockReturnValue({ + data: { download_url: 'https://example.com/file' }, + isLoading: true, + error: downloadError, + }) + + const { result } = renderHook(() => useSkillFileData('app-1', 'node-1', 'download')) + + expect(mockUseGetAppAssetFileContent).toHaveBeenCalledWith('app-1', 'node-1', { enabled: false }) + expect(mockUseGetAppAssetFileDownloadUrl).toHaveBeenCalledWith('app-1', 'node-1', { enabled: true }) + expect(result.current.downloadUrlData).toEqual({ download_url: 'https://example.com/file' }) + expect(result.current.isLoading).toBe(true) + expect(result.current.error).toBe(downloadError) + }) + }) +}) diff --git a/web/app/components/workflow/skill/hooks/use-skill-file-data.ts b/web/app/components/workflow/skill/hooks/use-skill-file-data.ts index 979ad01ffd..34ea7d46b9 100644 --- a/web/app/components/workflow/skill/hooks/use-skill-file-data.ts +++ b/web/app/components/workflow/skill/hooks/use-skill-file-data.ts @@ -1,5 +1,7 @@ import { useGetAppAssetFileContent, useGetAppAssetFileDownloadUrl } from '@/service/use-app-asset' +export type SkillFileDataMode = 'none' | 'content' | 'download' + export type SkillFileDataResult = { fileContent: ReturnType['data'] downloadUrlData: ReturnType['data'] @@ -9,19 +11,22 @@ export type SkillFileDataResult = { /** * Hook to fetch file data for skill documents. - * Fetches content for editable files and download URL for non-editable files. + * Uses explicit mode to control data fetching: + * - 'content': fetch editable file content + * - 'download': fetch non-editable file download URL + * - 'none': skip file-related requests while node metadata is unresolved */ export function useSkillFileData( appId: string, nodeId: string | null | undefined, - isEditable: boolean, + mode: SkillFileDataMode, ): SkillFileDataResult { const { data: fileContent, isLoading: isContentLoading, error: contentError, } = useGetAppAssetFileContent(appId, nodeId || '', { - enabled: isEditable, + enabled: mode === 'content', }) const { @@ -29,11 +34,19 @@ export function useSkillFileData( isLoading: isDownloadUrlLoading, error: downloadUrlError, } = useGetAppAssetFileDownloadUrl(appId, nodeId || '', { - enabled: !isEditable && !!nodeId, + enabled: mode === 'download' && !!nodeId, }) - const isLoading = isEditable ? isContentLoading : isDownloadUrlLoading - const error = isEditable ? contentError : downloadUrlError + const isLoading = mode === 'content' + ? isContentLoading + : mode === 'download' + ? isDownloadUrlLoading + : false + const error = mode === 'content' + ? contentError + : mode === 'download' + ? downloadUrlError + : null return { fileContent,