From 0ed0a31ed6705edf61b6550bcf68df6086b0f594 Mon Sep 17 00:00:00 2001 From: zxhlyh Date: Mon, 26 Jan 2026 14:41:12 +0800 Subject: [PATCH] fix: workflow sync draft --- .../hooks/use-nodes-sync-draft.spec.ts | 705 ++++++++++++++++++ .../hooks/use-nodes-sync-draft.ts | 2 +- .../hooks/use-workflow-refresh-draft.spec.ts | 556 ++++++++++++++ .../hooks/use-workflow-refresh-draft.ts | 14 +- 4 files changed, 1270 insertions(+), 7 deletions(-) create mode 100644 web/app/components/workflow-app/hooks/use-nodes-sync-draft.spec.ts create mode 100644 web/app/components/workflow-app/hooks/use-workflow-refresh-draft.spec.ts diff --git a/web/app/components/workflow-app/hooks/use-nodes-sync-draft.spec.ts b/web/app/components/workflow-app/hooks/use-nodes-sync-draft.spec.ts new file mode 100644 index 0000000000..626cbb4fc4 --- /dev/null +++ b/web/app/components/workflow-app/hooks/use-nodes-sync-draft.spec.ts @@ -0,0 +1,705 @@ +/** + * Test Suite for useNodesSyncDraft Hook + * + * PURPOSE: + * This hook handles syncing workflow draft to the server. The key fix being tested + * is the error handling behavior when `draft_workflow_not_sync` error occurs. + * + * MULTI-TAB PROBLEM SCENARIO: + * 1. User opens the same workflow in Tab A and Tab B (both have hash: v1) + * 2. Tab A saves successfully, server returns new hash: v2 + * 3. Tab B tries to save with old hash: v1, server returns 400 error with code + * 'draft_workflow_not_sync' + * 4. BEFORE FIX: handleRefreshWorkflowDraft() was called without args, which fetched + * draft AND overwrote canvas - user lost unsaved changes in Tab B + * 5. AFTER FIX: handleRefreshWorkflowDraft(true) is called, which fetches draft but + * only updates hash (notUpdateCanvas=true), preserving user's canvas changes + * + * TESTING STRATEGY: + * We don't simulate actual tab switching UI behavior. Instead, we mock the API to + * return `draft_workflow_not_sync` error and verify: + * - The hook calls handleRefreshWorkflowDraft(true) - not handleRefreshWorkflowDraft() + * - This ensures canvas data is preserved while hash is updated for retry + * + * This is behavior-driven testing - we verify "what the code does when receiving + * specific API errors" rather than simulating complete user interaction flows. + * True multi-tab integration testing would require E2E frameworks like Playwright. + */ + +import { act, renderHook, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useNodesSyncDraft } from './use-nodes-sync-draft' + +// Mock reactflow store +const mockGetNodes = vi.fn() + +type MockEdge = { + id: string + source: string + target: string + data: Record +} + +const mockStoreState: { + getNodes: ReturnType + edges: MockEdge[] + transform: number[] +} = { + getNodes: mockGetNodes, + edges: [], + transform: [0, 0, 1], +} +vi.mock('reactflow', () => ({ + useStoreApi: () => ({ + getState: () => mockStoreState, + }), +})) + +// Mock features store +const mockFeaturesState = { + features: { + opening: { enabled: false, opening_statement: '', suggested_questions: [] }, + suggested: {}, + text2speech: {}, + speech2text: {}, + citation: {}, + moderation: {}, + file: {}, + }, +} +vi.mock('@/app/components/base/features/hooks', () => ({ + useFeaturesStore: () => ({ + getState: () => mockFeaturesState, + }), +})) + +// Mock workflow service +const mockSyncWorkflowDraft = vi.fn() +vi.mock('@/service/workflow', () => ({ + syncWorkflowDraft: (...args: unknown[]) => mockSyncWorkflowDraft(...args), +})) + +// Mock useNodesReadOnly +const mockGetNodesReadOnly = vi.fn() +vi.mock('@/app/components/workflow/hooks/use-workflow', () => ({ + useNodesReadOnly: () => ({ + getNodesReadOnly: mockGetNodesReadOnly, + }), +})) + +// Mock useSerialAsyncCallback - pass through the callback +vi.mock('@/app/components/workflow/hooks/use-serial-async-callback', () => ({ + useSerialAsyncCallback: (callback: (...args: unknown[]) => unknown) => callback, +})) + +// Mock workflow store +const mockSetSyncWorkflowDraftHash = vi.fn() +const mockSetDraftUpdatedAt = vi.fn() + +const createMockWorkflowStoreState = (overrides = {}) => ({ + appId: 'test-app-id', + conversationVariables: [], + environmentVariables: [], + syncWorkflowDraftHash: 'current-hash-123', + isWorkflowDataLoaded: true, + setSyncWorkflowDraftHash: mockSetSyncWorkflowDraftHash, + setDraftUpdatedAt: mockSetDraftUpdatedAt, + ...overrides, +}) + +const mockWorkflowStoreGetState = vi.fn() +vi.mock('@/app/components/workflow/store', () => ({ + useWorkflowStore: () => ({ + getState: mockWorkflowStoreGetState, + }), +})) + +// Mock useWorkflowRefreshDraft (THE KEY DEPENDENCY FOR THIS TEST) +const mockHandleRefreshWorkflowDraft = vi.fn() +vi.mock('.', () => ({ + useWorkflowRefreshDraft: () => ({ + handleRefreshWorkflowDraft: mockHandleRefreshWorkflowDraft, + }), +})) + +// Mock API_PREFIX +vi.mock('@/config', () => ({ + API_PREFIX: '/api', +})) + +// Create a mock error response that mimics the actual API error +const createMockErrorResponse = (code: string) => { + const errorBody = { code, message: 'Draft not in sync' } + let bodyUsed = false + + return { + json: vi.fn().mockImplementation(() => { + bodyUsed = true + return Promise.resolve(errorBody) + }), + get bodyUsed() { + return bodyUsed + }, + } +} + +describe('useNodesSyncDraft', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetNodesReadOnly.mockReturnValue(false) + mockGetNodes.mockReturnValue([ + { id: 'node-1', type: 'start', data: { type: 'start' } }, + { id: 'node-2', type: 'llm', data: { type: 'llm' } }, + ]) + mockStoreState.edges = [ + { id: 'edge-1', source: 'node-1', target: 'node-2', data: {} }, + ] + mockWorkflowStoreGetState.mockReturnValue(createMockWorkflowStoreState()) + mockSyncWorkflowDraft.mockResolvedValue({ + hash: 'new-hash-456', + updated_at: Date.now(), + }) + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + describe('doSyncWorkflowDraft function', () => { + it('should return doSyncWorkflowDraft function', () => { + const { result } = renderHook(() => useNodesSyncDraft()) + + expect(result.current.doSyncWorkflowDraft).toBeDefined() + expect(typeof result.current.doSyncWorkflowDraft).toBe('function') + }) + + it('should return syncWorkflowDraftWhenPageClose function', () => { + const { result } = renderHook(() => useNodesSyncDraft()) + + expect(result.current.syncWorkflowDraftWhenPageClose).toBeDefined() + expect(typeof result.current.syncWorkflowDraftWhenPageClose).toBe('function') + }) + }) + + describe('successful sync', () => { + it('should call syncWorkflowDraft service on successful sync', async () => { + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).toHaveBeenCalledWith({ + url: '/apps/test-app-id/workflows/draft', + params: expect.objectContaining({ + hash: 'current-hash-123', + graph: expect.objectContaining({ + nodes: expect.any(Array), + edges: expect.any(Array), + viewport: expect.any(Object), + }), + }), + }) + }) + + it('should update syncWorkflowDraftHash on success', async () => { + mockSyncWorkflowDraft.mockResolvedValue({ + hash: 'new-hash-789', + updated_at: 1234567890, + }) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-789') + }) + + it('should update draftUpdatedAt on success', async () => { + const updatedAt = 1234567890 + mockSyncWorkflowDraft.mockResolvedValue({ + hash: 'new-hash', + updated_at: updatedAt, + }) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSetDraftUpdatedAt).toHaveBeenCalledWith(updatedAt) + }) + + it('should call onSuccess callback on success', async () => { + const onSuccess = vi.fn() + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSuccess }) + }) + + expect(onSuccess).toHaveBeenCalled() + }) + + it('should call onSettled callback after success', async () => { + const onSettled = vi.fn() + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSettled }) + }) + + expect(onSettled).toHaveBeenCalled() + }) + }) + + describe('sync error handling - draft_workflow_not_sync (THE KEY FIX)', () => { + /** + * This is THE KEY TEST for the bug fix. + * + * SCENARIO: Multi-tab editing + * 1. User opens workflow in Tab A and Tab B + * 2. Tab A saves draft successfully, gets new hash + * 3. Tab B tries to save with old hash + * 4. Server returns 400 with code 'draft_workflow_not_sync' + * + * BEFORE FIX: + * - handleRefreshWorkflowDraft() was called without arguments + * - This would fetch draft AND overwrite the canvas + * - User loses their unsaved changes in Tab B + * + * AFTER FIX: + * - handleRefreshWorkflowDraft(true) is called + * - This fetches draft but DOES NOT overwrite canvas + * - Only hash is updated for the next sync attempt + * - User's unsaved changes are preserved + */ + it('should call handleRefreshWorkflowDraft with notUpdateCanvas=true when draft_workflow_not_sync error occurs', async () => { + const mockError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // THE KEY ASSERTION: handleRefreshWorkflowDraft must be called with true + await waitFor(() => { + expect(mockHandleRefreshWorkflowDraft).toHaveBeenCalledWith(true) + }) + }) + + it('should NOT call handleRefreshWorkflowDraft when notRefreshWhenSyncError is true', async () => { + const mockError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + // First parameter is notRefreshWhenSyncError + await result.current.doSyncWorkflowDraft(true) + }) + + // Wait a bit for async operations + await new Promise(resolve => setTimeout(resolve, 100)) + + expect(mockHandleRefreshWorkflowDraft).not.toHaveBeenCalled() + }) + + it('should call onError callback when draft_workflow_not_sync error occurs', async () => { + const mockError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + const onError = vi.fn() + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onError }) + }) + + expect(onError).toHaveBeenCalled() + }) + + it('should call onSettled callback after error', async () => { + const mockError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + const onSettled = vi.fn() + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSettled }) + }) + + expect(onSettled).toHaveBeenCalled() + }) + }) + + describe('other error handling', () => { + it('should NOT call handleRefreshWorkflowDraft for non-draft_workflow_not_sync errors', async () => { + const mockError = createMockErrorResponse('some_other_error') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // Wait a bit for async operations + await new Promise(resolve => setTimeout(resolve, 100)) + + expect(mockHandleRefreshWorkflowDraft).not.toHaveBeenCalled() + }) + + it('should handle error without json method', async () => { + const mockError = new Error('Network error') + mockSyncWorkflowDraft.mockRejectedValue(mockError) + + const { result } = renderHook(() => useNodesSyncDraft()) + const onError = vi.fn() + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onError }) + }) + + expect(onError).toHaveBeenCalled() + expect(mockHandleRefreshWorkflowDraft).not.toHaveBeenCalled() + }) + + it('should handle error with bodyUsed already true', async () => { + const mockError = { + json: vi.fn(), + bodyUsed: true, + } + mockSyncWorkflowDraft.mockRejectedValue(mockError) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // Should not call json() when bodyUsed is true + expect(mockError.json).not.toHaveBeenCalled() + expect(mockHandleRefreshWorkflowDraft).not.toHaveBeenCalled() + }) + }) + + describe('read-only mode', () => { + it('should not sync when nodes are read-only', async () => { + mockGetNodesReadOnly.mockReturnValue(true) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).not.toHaveBeenCalled() + }) + + it('should not sync on page close when nodes are read-only', () => { + mockGetNodesReadOnly.mockReturnValue(true) + + // Mock sendBeacon + const mockSendBeacon = vi.fn() + Object.defineProperty(navigator, 'sendBeacon', { + value: mockSendBeacon, + writable: true, + }) + + const { result } = renderHook(() => useNodesSyncDraft()) + + act(() => { + result.current.syncWorkflowDraftWhenPageClose() + }) + + expect(mockSendBeacon).not.toHaveBeenCalled() + }) + }) + + describe('workflow data not loaded', () => { + it('should not sync when workflow data is not loaded', async () => { + mockWorkflowStoreGetState.mockReturnValue( + createMockWorkflowStoreState({ isWorkflowDataLoaded: false }), + ) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).not.toHaveBeenCalled() + }) + }) + + describe('no appId', () => { + it('should not sync when appId is not set', async () => { + mockWorkflowStoreGetState.mockReturnValue( + createMockWorkflowStoreState({ appId: null }), + ) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).not.toHaveBeenCalled() + }) + }) + + describe('node filtering', () => { + it('should filter out temp nodes', async () => { + mockGetNodes.mockReturnValue([ + { id: 'node-1', type: 'start', data: { type: 'start' } }, + { id: 'node-temp', type: 'custom', data: { type: 'custom', _isTempNode: true } }, + { id: 'node-2', type: 'llm', data: { type: 'llm' } }, + ]) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ + graph: expect.objectContaining({ + nodes: expect.not.arrayContaining([ + expect.objectContaining({ id: 'node-temp' }), + ]), + }), + }), + }), + ) + }) + + it('should remove internal underscore properties from nodes', async () => { + mockGetNodes.mockReturnValue([ + { + id: 'node-1', + type: 'start', + data: { + type: 'start', + _internalProp: 'should be removed', + _anotherInternal: true, + publicProp: 'should remain', + }, + }, + ]) + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + const callArgs = mockSyncWorkflowDraft.mock.calls[0][0] + const sentNode = callArgs.params.graph.nodes[0] + + expect(sentNode.data).not.toHaveProperty('_internalProp') + expect(sentNode.data).not.toHaveProperty('_anotherInternal') + expect(sentNode.data).toHaveProperty('publicProp', 'should remain') + }) + }) + + describe('edge filtering', () => { + it('should filter out temp edges', async () => { + mockStoreState.edges = [ + { id: 'edge-1', source: 'node-1', target: 'node-2', data: {} }, + { id: 'edge-temp', source: 'node-1', target: 'node-3', data: { _isTemp: true } }, + ] + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + const callArgs = mockSyncWorkflowDraft.mock.calls[0][0] + const sentEdges = callArgs.params.graph.edges + + expect(sentEdges).toHaveLength(1) + expect(sentEdges[0].id).toBe('edge-1') + }) + + it('should remove internal underscore properties from edges', async () => { + mockStoreState.edges = [ + { + id: 'edge-1', + source: 'node-1', + target: 'node-2', + data: { + _internalEdgeProp: 'should be removed', + publicEdgeProp: 'should remain', + }, + }, + ] + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + const callArgs = mockSyncWorkflowDraft.mock.calls[0][0] + const sentEdge = callArgs.params.graph.edges[0] + + expect(sentEdge.data).not.toHaveProperty('_internalEdgeProp') + expect(sentEdge.data).toHaveProperty('publicEdgeProp', 'should remain') + }) + }) + + describe('viewport handling', () => { + it('should send current viewport from transform', async () => { + mockStoreState.transform = [100, 200, 1.5] + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + expect(mockSyncWorkflowDraft).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ + graph: expect.objectContaining({ + viewport: { x: 100, y: 200, zoom: 1.5 }, + }), + }), + }), + ) + }) + }) + + describe('multi-tab concurrent editing scenario (END-TO-END TEST)', () => { + /** + * Simulates the complete multi-tab scenario to verify the fix works correctly. + * + * Scenario: + * 1. Tab A and Tab B both have the workflow open with hash 'hash-v1' + * 2. Tab A saves successfully, server returns 'hash-v2' + * 3. Tab B tries to save with 'hash-v1', gets 'draft_workflow_not_sync' error + * 4. Tab B should only update hash to 'hash-v2', not overwrite canvas + * 5. Tab B can now retry save with correct hash + */ + it('should preserve canvas data during hash conflict resolution', async () => { + // Initial state: both tabs have hash-v1 + mockWorkflowStoreGetState.mockReturnValue( + createMockWorkflowStoreState({ syncWorkflowDraftHash: 'hash-v1' }), + ) + + // Tab B tries to save with old hash, server returns error + const syncError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(syncError) + + const { result } = renderHook(() => useNodesSyncDraft()) + + // Tab B attempts to sync + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // Verify the sync was attempted with old hash + expect(mockSyncWorkflowDraft).toHaveBeenCalledWith( + expect.objectContaining({ + params: expect.objectContaining({ + hash: 'hash-v1', + }), + }), + ) + + // Verify handleRefreshWorkflowDraft was called with true (not overwrite canvas) + await waitFor(() => { + expect(mockHandleRefreshWorkflowDraft).toHaveBeenCalledWith(true) + }) + + // The key assertion: only one argument (true) was passed + expect(mockHandleRefreshWorkflowDraft).toHaveBeenCalledTimes(1) + expect(mockHandleRefreshWorkflowDraft.mock.calls[0]).toEqual([true]) + }) + + it('should handle multiple consecutive sync failures gracefully', async () => { + // Create fresh error for each call to avoid bodyUsed issue + mockSyncWorkflowDraft + .mockRejectedValueOnce(createMockErrorResponse('draft_workflow_not_sync')) + .mockRejectedValueOnce(createMockErrorResponse('draft_workflow_not_sync')) + + const { result } = renderHook(() => useNodesSyncDraft()) + + // First sync attempt + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // Wait for first refresh call + await waitFor(() => { + expect(mockHandleRefreshWorkflowDraft).toHaveBeenCalledTimes(1) + }) + + // Second sync attempt + await act(async () => { + await result.current.doSyncWorkflowDraft() + }) + + // Both should call handleRefreshWorkflowDraft with true + await waitFor(() => { + expect(mockHandleRefreshWorkflowDraft).toHaveBeenCalledTimes(2) + }) + + mockHandleRefreshWorkflowDraft.mock.calls.forEach((call) => { + expect(call).toEqual([true]) + }) + }) + }) + + describe('callbacks behavior', () => { + it('should not call onSuccess when sync fails', async () => { + const syncError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(syncError) + const onSuccess = vi.fn() + const onError = vi.fn() + + const { result } = renderHook(() => useNodesSyncDraft()) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSuccess, onError }) + }) + + expect(onSuccess).not.toHaveBeenCalled() + expect(onError).toHaveBeenCalled() + }) + + it('should always call onSettled regardless of success or failure', async () => { + const onSettled = vi.fn() + + const { result } = renderHook(() => useNodesSyncDraft()) + + // Test success case + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSettled }) + }) + expect(onSettled).toHaveBeenCalledTimes(1) + + // Reset + onSettled.mockClear() + + // Test failure case + const syncError = createMockErrorResponse('draft_workflow_not_sync') + mockSyncWorkflowDraft.mockRejectedValue(syncError) + + await act(async () => { + await result.current.doSyncWorkflowDraft(false, { onSettled }) + }) + expect(onSettled).toHaveBeenCalledTimes(1) + }) + }) +}) diff --git a/web/app/components/workflow-app/hooks/use-nodes-sync-draft.ts b/web/app/components/workflow-app/hooks/use-nodes-sync-draft.ts index 5d394bab1e..5673d5edf1 100644 --- a/web/app/components/workflow-app/hooks/use-nodes-sync-draft.ts +++ b/web/app/components/workflow-app/hooks/use-nodes-sync-draft.ts @@ -115,7 +115,7 @@ export const useNodesSyncDraft = () => { if (error && error.json && !error.bodyUsed) { error.json().then((err: any) => { if (err.code === 'draft_workflow_not_sync' && !notRefreshWhenSyncError) - handleRefreshWorkflowDraft() + handleRefreshWorkflowDraft(true) }) } callback?.onError?.() diff --git a/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.spec.ts b/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.spec.ts new file mode 100644 index 0000000000..702d8787f0 --- /dev/null +++ b/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.spec.ts @@ -0,0 +1,556 @@ +/** + * Test Suite for useWorkflowRefreshDraft Hook + * + * PURPOSE: + * This hook is responsible for refreshing workflow draft data from the server. + * The key fix being tested is the `notUpdateCanvas` parameter behavior. + * + * MULTI-TAB PROBLEM SCENARIO: + * 1. User opens the same workflow in Tab A and Tab B (both have hash: v1) + * 2. Tab A saves successfully, server returns new hash: v2 + * 3. Tab B tries to save with old hash: v1, server returns 400 error (draft_workflow_not_sync) + * 4. BEFORE FIX: handleRefreshWorkflowDraft() was called without args, which fetched + * draft AND overwrote canvas - user lost unsaved changes in Tab B + * 5. AFTER FIX: handleRefreshWorkflowDraft(true) is called, which fetches draft but + * only updates hash, preserving user's canvas changes + * + * TESTING STRATEGY: + * We don't simulate actual tab switching UI behavior. Instead, we test the hook's + * response to specific inputs: + * - When notUpdateCanvas=true: should NOT call handleUpdateWorkflowCanvas + * - When notUpdateCanvas=false/undefined: should call handleUpdateWorkflowCanvas + * + * This is behavior-driven testing - we verify "what the code does when given specific + * inputs" rather than simulating complete user interaction flows. + */ + +import { act, renderHook, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useWorkflowRefreshDraft } from './use-workflow-refresh-draft' + +// Mock the workflow service +const mockFetchWorkflowDraft = vi.fn() +vi.mock('@/service/workflow', () => ({ + fetchWorkflowDraft: (...args: unknown[]) => mockFetchWorkflowDraft(...args), +})) + +// Mock the workflow update hook +const mockHandleUpdateWorkflowCanvas = vi.fn() +vi.mock('@/app/components/workflow/hooks', () => ({ + useWorkflowUpdate: () => ({ + handleUpdateWorkflowCanvas: mockHandleUpdateWorkflowCanvas, + }), +})) + +// Mock store state +const mockSetSyncWorkflowDraftHash = vi.fn() +const mockSetIsSyncingWorkflowDraft = vi.fn() +const mockSetEnvironmentVariables = vi.fn() +const mockSetEnvSecrets = vi.fn() +const mockSetConversationVariables = vi.fn() +const mockSetIsWorkflowDataLoaded = vi.fn() +const mockCancelDebouncedSync = vi.fn() + +const createMockStoreState = (overrides = {}) => ({ + appId: 'test-app-id', + setSyncWorkflowDraftHash: mockSetSyncWorkflowDraftHash, + setIsSyncingWorkflowDraft: mockSetIsSyncingWorkflowDraft, + setEnvironmentVariables: mockSetEnvironmentVariables, + setEnvSecrets: mockSetEnvSecrets, + setConversationVariables: mockSetConversationVariables, + setIsWorkflowDataLoaded: mockSetIsWorkflowDataLoaded, + isWorkflowDataLoaded: true, + debouncedSyncWorkflowDraft: { + cancel: mockCancelDebouncedSync, + }, + ...overrides, +}) + +const mockWorkflowStoreGetState = vi.fn() +vi.mock('@/app/components/workflow/store', () => ({ + useWorkflowStore: () => ({ + getState: mockWorkflowStoreGetState, + }), +})) + +// Default mock response from fetchWorkflowDraft +const createMockDraftResponse = (overrides = {}) => ({ + hash: 'new-hash-12345', + graph: { + nodes: [{ id: 'node-1', type: 'start', data: {} }], + edges: [{ id: 'edge-1', source: 'node-1', target: 'node-2' }], + viewport: { x: 100, y: 200, zoom: 1.5 }, + }, + environment_variables: [ + { id: 'env-1', name: 'API_KEY', value: 'secret-key', value_type: 'secret' }, + { id: 'env-2', name: 'BASE_URL', value: 'https://api.example.com', value_type: 'string' }, + ], + conversation_variables: [ + { id: 'conv-1', name: 'user_input', value: 'test' }, + ], + ...overrides, +}) + +describe('useWorkflowRefreshDraft', () => { + beforeEach(() => { + vi.clearAllMocks() + mockWorkflowStoreGetState.mockReturnValue(createMockStoreState()) + mockFetchWorkflowDraft.mockResolvedValue(createMockDraftResponse()) + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + describe('handleRefreshWorkflowDraft function', () => { + it('should return handleRefreshWorkflowDraft function', () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + expect(result.current.handleRefreshWorkflowDraft).toBeDefined() + expect(typeof result.current.handleRefreshWorkflowDraft).toBe('function') + }) + }) + + describe('notUpdateCanvas parameter behavior (THE KEY FIX)', () => { + it('should NOT call handleUpdateWorkflowCanvas when notUpdateCanvas is true', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockFetchWorkflowDraft).toHaveBeenCalledWith('/apps/test-app-id/workflows/draft') + }) + + await waitFor(() => { + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-12345') + }) + + // THE KEY ASSERTION: Canvas should NOT be updated when notUpdateCanvas is true + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + }) + + it('should call handleUpdateWorkflowCanvas when notUpdateCanvas is false', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(false) + }) + + await waitFor(() => { + expect(mockFetchWorkflowDraft).toHaveBeenCalledWith('/apps/test-app-id/workflows/draft') + }) + + await waitFor(() => { + // Canvas SHOULD be updated when notUpdateCanvas is false + expect(mockHandleUpdateWorkflowCanvas).toHaveBeenCalledWith({ + nodes: [{ id: 'node-1', type: 'start', data: {} }], + edges: [{ id: 'edge-1', source: 'node-1', target: 'node-2' }], + viewport: { x: 100, y: 200, zoom: 1.5 }, + }) + }) + + await waitFor(() => { + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-12345') + }) + }) + + it('should call handleUpdateWorkflowCanvas when notUpdateCanvas is undefined (default)', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + await waitFor(() => { + expect(mockFetchWorkflowDraft).toHaveBeenCalled() + }) + + await waitFor(() => { + // Canvas SHOULD be updated when notUpdateCanvas is undefined + expect(mockHandleUpdateWorkflowCanvas).toHaveBeenCalled() + }) + }) + + it('should still update hash even when notUpdateCanvas is true', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-12345') + }) + + // Verify canvas was NOT updated + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + }) + + it('should still update environment variables when notUpdateCanvas is true', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetEnvironmentVariables).toHaveBeenCalledWith([ + { id: 'env-1', name: 'API_KEY', value: '[__HIDDEN__]', value_type: 'secret' }, + { id: 'env-2', name: 'BASE_URL', value: 'https://api.example.com', value_type: 'string' }, + ]) + }) + + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + }) + + it('should still update env secrets when notUpdateCanvas is true', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetEnvSecrets).toHaveBeenCalledWith({ + 'env-1': 'secret-key', + }) + }) + + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + }) + + it('should still update conversation variables when notUpdateCanvas is true', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetConversationVariables).toHaveBeenCalledWith([ + { id: 'conv-1', name: 'user_input', value: 'test' }, + ]) + }) + + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + }) + }) + + describe('syncing state management', () => { + it('should set isSyncingWorkflowDraft to true before fetch', () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + expect(mockSetIsSyncingWorkflowDraft).toHaveBeenCalledWith(true) + }) + + it('should set isSyncingWorkflowDraft to false after fetch completes', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + await waitFor(() => { + expect(mockSetIsSyncingWorkflowDraft).toHaveBeenCalledWith(false) + }) + }) + + it('should set isSyncingWorkflowDraft to false even when fetch fails', async () => { + mockFetchWorkflowDraft.mockRejectedValue(new Error('Network error')) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + await waitFor(() => { + expect(mockSetIsSyncingWorkflowDraft).toHaveBeenCalledWith(false) + }) + }) + }) + + describe('isWorkflowDataLoaded flag management', () => { + it('should set isWorkflowDataLoaded to false before fetch when it was true', () => { + mockWorkflowStoreGetState.mockReturnValue( + createMockStoreState({ isWorkflowDataLoaded: true }), + ) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + expect(mockSetIsWorkflowDataLoaded).toHaveBeenCalledWith(false) + }) + + it('should set isWorkflowDataLoaded to true after fetch succeeds', async () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + await waitFor(() => { + expect(mockSetIsWorkflowDataLoaded).toHaveBeenCalledWith(true) + }) + }) + + it('should restore isWorkflowDataLoaded when fetch fails and it was previously loaded', async () => { + mockWorkflowStoreGetState.mockReturnValue( + createMockStoreState({ isWorkflowDataLoaded: true }), + ) + mockFetchWorkflowDraft.mockRejectedValue(new Error('Network error')) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + await waitFor(() => { + // Should restore to true because wasLoaded was true + expect(mockSetIsWorkflowDataLoaded).toHaveBeenLastCalledWith(true) + }) + }) + }) + + describe('debounced sync cancellation', () => { + it('should cancel debounced sync before fetching draft', () => { + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + + expect(mockCancelDebouncedSync).toHaveBeenCalled() + }) + + it('should handle case when debouncedSyncWorkflowDraft has no cancel method', () => { + mockWorkflowStoreGetState.mockReturnValue( + createMockStoreState({ debouncedSyncWorkflowDraft: {} }), + ) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + // Should not throw + expect(() => { + act(() => { + result.current.handleRefreshWorkflowDraft() + }) + }).not.toThrow() + }) + }) + + describe('edge cases', () => { + it('should handle empty graph in response', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-empty', + graph: null, + environment_variables: [], + conversation_variables: [], + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(false) + }) + + await waitFor(() => { + expect(mockHandleUpdateWorkflowCanvas).toHaveBeenCalledWith({ + nodes: [], + edges: [], + viewport: { x: 0, y: 0, zoom: 1 }, + }) + }) + }) + + it('should handle missing viewport in response', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-no-viewport', + graph: { + nodes: [{ id: 'node-1' }], + edges: [], + viewport: null, + }, + environment_variables: [], + conversation_variables: [], + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(false) + }) + + await waitFor(() => { + expect(mockHandleUpdateWorkflowCanvas).toHaveBeenCalledWith({ + nodes: [{ id: 'node-1' }], + edges: [], + viewport: { x: 0, y: 0, zoom: 1 }, + }) + }) + }) + + it('should handle missing environment_variables in response', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-no-env', + graph: { nodes: [], edges: [], viewport: { x: 0, y: 0, zoom: 1 } }, + environment_variables: undefined, + conversation_variables: [], + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetEnvironmentVariables).toHaveBeenCalledWith([]) + expect(mockSetEnvSecrets).toHaveBeenCalledWith({}) + }) + }) + + it('should handle missing conversation_variables in response', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-no-conv', + graph: { nodes: [], edges: [], viewport: { x: 0, y: 0, zoom: 1 } }, + environment_variables: [], + conversation_variables: undefined, + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetConversationVariables).toHaveBeenCalledWith([]) + }) + }) + + it('should filter only secret type for envSecrets', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-mixed-env', + graph: { nodes: [], edges: [], viewport: { x: 0, y: 0, zoom: 1 } }, + environment_variables: [ + { id: 'env-1', name: 'SECRET_KEY', value: 'secret-value', value_type: 'secret' }, + { id: 'env-2', name: 'PUBLIC_URL', value: 'https://example.com', value_type: 'string' }, + { id: 'env-3', name: 'ANOTHER_SECRET', value: 'another-secret', value_type: 'secret' }, + ], + conversation_variables: [], + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetEnvSecrets).toHaveBeenCalledWith({ + 'env-1': 'secret-value', + 'env-3': 'another-secret', + }) + }) + }) + + it('should hide secret values in environment variables', async () => { + mockFetchWorkflowDraft.mockResolvedValue({ + hash: 'hash-secrets', + graph: { nodes: [], edges: [], viewport: { x: 0, y: 0, zoom: 1 } }, + environment_variables: [ + { id: 'env-1', name: 'SECRET_KEY', value: 'super-secret', value_type: 'secret' }, + { id: 'env-2', name: 'PUBLIC_URL', value: 'https://example.com', value_type: 'string' }, + ], + conversation_variables: [], + }) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockSetEnvironmentVariables).toHaveBeenCalledWith([ + { id: 'env-1', name: 'SECRET_KEY', value: '[__HIDDEN__]', value_type: 'secret' }, + { id: 'env-2', name: 'PUBLIC_URL', value: 'https://example.com', value_type: 'string' }, + ]) + }) + }) + }) + + describe('multi-tab scenario simulation (THE BUG FIX VERIFICATION)', () => { + /** + * This test verifies the fix for the multi-tab scenario: + * 1. User opens workflow in Tab A and Tab B + * 2. Tab A saves draft successfully + * 3. Tab B tries to save but gets 'draft_workflow_not_sync' error (hash mismatch) + * 4. BEFORE FIX: Tab B would fetch draft and overwrite canvas with old data + * 5. AFTER FIX: Tab B only updates hash, preserving user's canvas changes + */ + it('should only update hash when called with notUpdateCanvas=true (simulating sync error recovery)', async () => { + const mockResponse = createMockDraftResponse() + mockFetchWorkflowDraft.mockResolvedValue(mockResponse) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + // Simulate the sync error recovery scenario where notUpdateCanvas is true + act(() => { + result.current.handleRefreshWorkflowDraft(true) + }) + + await waitFor(() => { + expect(mockFetchWorkflowDraft).toHaveBeenCalled() + }) + + await waitFor(() => { + // Hash should be updated for next sync attempt + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-12345') + }) + + // Canvas should NOT be updated - user's changes are preserved + expect(mockHandleUpdateWorkflowCanvas).not.toHaveBeenCalled() + + // Other states should still be updated + expect(mockSetEnvironmentVariables).toHaveBeenCalled() + expect(mockSetConversationVariables).toHaveBeenCalled() + }) + + it('should update canvas when called with notUpdateCanvas=false (normal refresh)', async () => { + const mockResponse = createMockDraftResponse() + mockFetchWorkflowDraft.mockResolvedValue(mockResponse) + + const { result } = renderHook(() => useWorkflowRefreshDraft()) + + // Simulate normal refresh scenario + act(() => { + result.current.handleRefreshWorkflowDraft(false) + }) + + await waitFor(() => { + expect(mockFetchWorkflowDraft).toHaveBeenCalled() + }) + + await waitFor(() => { + expect(mockSetSyncWorkflowDraftHash).toHaveBeenCalledWith('new-hash-12345') + }) + + // Canvas SHOULD be updated in normal refresh + await waitFor(() => { + expect(mockHandleUpdateWorkflowCanvas).toHaveBeenCalled() + }) + }) + }) +}) diff --git a/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.ts b/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.ts index fa4a44d894..a7283c0078 100644 --- a/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.ts +++ b/web/app/components/workflow-app/hooks/use-workflow-refresh-draft.ts @@ -8,7 +8,7 @@ export const useWorkflowRefreshDraft = () => { const workflowStore = useWorkflowStore() const { handleUpdateWorkflowCanvas } = useWorkflowUpdate() - const handleRefreshWorkflowDraft = useCallback(() => { + const handleRefreshWorkflowDraft = useCallback((notUpdateCanvas?: boolean) => { const { appId, setSyncWorkflowDraftHash, @@ -31,12 +31,14 @@ export const useWorkflowRefreshDraft = () => { fetchWorkflowDraft(`/apps/${appId}/workflows/draft`) .then((response) => { // Ensure we have a valid workflow structure with viewport - const workflowData: WorkflowDataUpdater = { - nodes: response.graph?.nodes || [], - edges: response.graph?.edges || [], - viewport: response.graph?.viewport || { x: 0, y: 0, zoom: 1 }, + if (!notUpdateCanvas) { + const workflowData: WorkflowDataUpdater = { + nodes: response.graph?.nodes || [], + edges: response.graph?.edges || [], + viewport: response.graph?.viewport || { x: 0, y: 0, zoom: 1 }, + } + handleUpdateWorkflowCanvas(workflowData) } - handleUpdateWorkflowCanvas(workflowData) setSyncWorkflowDraftHash(response.hash) setEnvSecrets((response.environment_variables || []).filter(env => env.value_type === 'secret').reduce((acc, env) => { acc[env.id] = env.value