diff --git a/api/controllers/console/datasets/datasets_document.py b/api/controllers/console/datasets/datasets_document.py index 5887b2ec24..ab207bf65c 100644 --- a/api/controllers/console/datasets/datasets_document.py +++ b/api/controllers/console/datasets/datasets_document.py @@ -41,7 +41,7 @@ from fields.document_fields import ( from libs.datetime_utils import naive_utc_now from libs.login import current_account_with_tenant, login_required from models import DatasetProcessRule, Document, DocumentSegment, UploadFile -from models.dataset import DocumentPipelineExecutionLog +from models.dataset import DocumentPipelineExecutionLog, DocumentSegmentSummary from services.dataset_service import DatasetService, DocumentService from services.entities.knowledge_entities.knowledge_entities import KnowledgeConfig, ProcessRule, RetrievalModel from services.file_service import FileService diff --git a/api/controllers/console/datasets/datasets_segments.py b/api/controllers/console/datasets/datasets_segments.py index 23a668112d..7eb88e0ff3 100644 --- a/api/controllers/console/datasets/datasets_segments.py +++ b/api/controllers/console/datasets/datasets_segments.py @@ -32,7 +32,7 @@ from extensions.ext_redis import redis_client from fields.segment_fields import child_chunk_fields, segment_fields from libs.helper import escape_like_pattern from libs.login import current_account_with_tenant, login_required -from models.dataset import ChildChunk, DocumentSegment +from models.dataset import ChildChunk, DocumentSegment, DocumentSegmentSummary from models.model import UploadFile from services.dataset_service import DatasetService, DocumentService, SegmentService from services.entities.knowledge_entities.knowledge_entities import ChildChunkUpdateArgs, SegmentUpdateArgs diff --git a/api/core/workflow/nodes/document_extractor/node.py b/api/core/workflow/nodes/document_extractor/node.py index 14ebd1f9ae..25dd98f48a 100644 --- a/api/core/workflow/nodes/document_extractor/node.py +++ b/api/core/workflow/nodes/document_extractor/node.py @@ -62,6 +62,21 @@ class DocumentExtractorNode(Node[DocumentExtractorNodeData]): inputs = {"variable_selector": variable_selector} process_data = {"documents": value if isinstance(value, list) else [value]} + # Ensure storage_key is loaded for File objects + files_to_check = value if isinstance(value, list) else [value] + files_needing_storage_key = [ + f for f in files_to_check if isinstance(f, File) and not f.storage_key and f.related_id + ] + if files_needing_storage_key: + from sqlalchemy.orm import Session + + from extensions.ext_database import db + from factories.file_factory import StorageKeyLoader + + with Session(bind=db.engine) as session: + storage_key_loader = StorageKeyLoader(session, tenant_id=self.tenant_id) + storage_key_loader.load_storage_keys(files_needing_storage_key) + try: if isinstance(value, list): extracted_text_list = list(map(_extract_text_from_file, value)) @@ -415,6 +430,16 @@ def _download_file_content(file: File) -> bytes: response.raise_for_status() return response.content else: + # Check if storage_key is set + if not file.storage_key: + raise FileDownloadError(f"File storage_key is missing for file: {file.filename}") + + # Check if file exists before downloading + from extensions.ext_storage import storage + + if not storage.exists(file.storage_key): + raise FileDownloadError(f"File not found in storage: {file.storage_key}") + return file_manager.download(file) except Exception as e: raise FileDownloadError(f"Error downloading file: {str(e)}") from e diff --git a/api/migrations/versions/2026_01_12_1358-562dcce7d77c_add_summaryindex_feature.py b/api/migrations/versions/2026_01_12_1358-562dcce7d77c_add_summaryindex_feature.py new file mode 100644 index 0000000000..40fe419ef6 --- /dev/null +++ b/api/migrations/versions/2026_01_12_1358-562dcce7d77c_add_summaryindex_feature.py @@ -0,0 +1,69 @@ +"""add SummaryIndex feature + +Revision ID: 562dcce7d77c +Revises: 03ea244985ce +Create Date: 2026-01-12 13:58:40.584802 + +""" +from alembic import op +import models as models +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '562dcce7d77c' +down_revision = '03ea244985ce' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('document_segment_summary', + sa.Column('id', models.types.StringUUID(), nullable=False), + sa.Column('dataset_id', models.types.StringUUID(), nullable=False), + sa.Column('document_id', models.types.StringUUID(), nullable=False), + sa.Column('chunk_id', models.types.StringUUID(), nullable=False), + sa.Column('summary_content', models.types.LongText(), nullable=True), + sa.Column('summary_index_node_id', sa.String(length=255), nullable=True), + sa.Column('summary_index_node_hash', sa.String(length=255), nullable=True), + sa.Column('status', sa.String(length=32), server_default=sa.text("'generating'"), nullable=False), + sa.Column('error', models.types.LongText(), nullable=True), + sa.Column('enabled', sa.Boolean(), server_default=sa.text('true'), nullable=False), + sa.Column('disabled_at', sa.DateTime(), nullable=True), + sa.Column('disabled_by', models.types.StringUUID(), nullable=True), + sa.Column('created_at', sa.DateTime(), server_default=sa.text('CURRENT_TIMESTAMP'), nullable=False), + sa.Column('updated_at', sa.DateTime(), server_default=sa.text('CURRENT_TIMESTAMP'), nullable=False), + sa.PrimaryKeyConstraint('id', name='document_segment_summary_pkey') + ) + with op.batch_alter_table('document_segment_summary', schema=None) as batch_op: + batch_op.create_index('document_segment_summary_chunk_id_idx', ['chunk_id'], unique=False) + batch_op.create_index('document_segment_summary_dataset_id_idx', ['dataset_id'], unique=False) + batch_op.create_index('document_segment_summary_document_id_idx', ['document_id'], unique=False) + batch_op.create_index('document_segment_summary_status_idx', ['status'], unique=False) + + with op.batch_alter_table('datasets', schema=None) as batch_op: + batch_op.add_column(sa.Column('summary_index_setting', models.types.AdjustedJSON(), nullable=True)) + + with op.batch_alter_table('documents', schema=None) as batch_op: + batch_op.add_column(sa.Column('need_summary', sa.Boolean(), server_default=sa.text('false'), nullable=True)) + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('documents', schema=None) as batch_op: + batch_op.drop_column('need_summary') + + with op.batch_alter_table('datasets', schema=None) as batch_op: + batch_op.drop_column('summary_index_setting') + + with op.batch_alter_table('document_segment_summary', schema=None) as batch_op: + batch_op.drop_index('document_segment_summary_status_idx') + batch_op.drop_index('document_segment_summary_document_id_idx') + batch_op.drop_index('document_segment_summary_dataset_id_idx') + batch_op.drop_index('document_segment_summary_chunk_id_idx') + + op.drop_table('document_segment_summary') + # ### end Alembic commands ### diff --git a/web/app/components/rag-pipeline/components/chunk-card-list/index.spec.tsx b/web/app/components/rag-pipeline/components/chunk-card-list/index.spec.tsx index ca5fae25c7..1864ad0c6e 100644 --- a/web/app/components/rag-pipeline/components/chunk-card-list/index.spec.tsx +++ b/web/app/components/rag-pipeline/components/chunk-card-list/index.spec.tsx @@ -398,7 +398,7 @@ describe('ChunkCard', () => { const { rerender } = render( , @@ -411,7 +411,7 @@ describe('ChunkCard', () => { rerender( , @@ -428,7 +428,7 @@ describe('ChunkCard', () => { const { rerender } = render( , @@ -500,7 +500,7 @@ describe('ChunkCard', () => { render( , 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 diff --git a/web/tsconfig.json b/web/tsconfig.json index 6df4f56bf1..45d7fa7871 100644 --- a/web/tsconfig.json +++ b/web/tsconfig.json @@ -2,7 +2,7 @@ "compilerOptions": { "incremental": true, "target": "es2022", - "jsx": "react-jsx", + "jsx": "preserve", "lib": [ "dom", "dom.iterable",