From 2ee3d26657dd3575aa90a7ea40f4f3095ffb0b95 Mon Sep 17 00:00:00 2001 From: Csaba Tuncsik Date: Thu, 16 Oct 2025 12:32:29 +0200 Subject: [PATCH] fix(editor): Enhance error handling and toast notifications in WorkflowDiffModal (#20812) --- .../components/SourceControlPushModal.test.ts | 178 +++++++++++-- .../components/SourceControlPushModal.vue | 6 +- .../workflow-diff/WorkflowDiffModal.test.ts | 242 +++++++++++++++++- .../workflow-diff/WorkflowDiffModal.vue | 28 +- 4 files changed, 411 insertions(+), 43 deletions(-) diff --git a/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.test.ts b/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.test.ts index 0cd2c60c7cf..7b8fceeeb56 100644 --- a/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.test.ts +++ b/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.test.ts @@ -146,14 +146,15 @@ describe('SourceControlPushModal', () => { // Setup store with default mock to prevent automatic data loading pinia = createTestingPinia(); sourceControlStore = mockedStore(useSourceControlStore); - sourceControlStore.getAggregatedStatus = vi.fn().mockResolvedValue([]); - sourceControlStore.pushWorkfolder = vi.fn().mockResolvedValue([]); + sourceControlStore.getAggregatedStatus.mockResolvedValue([]); settingsStore = mockedStore(useSettingsStore); settingsStore.settings.enterprise = defaultSettings.enterprise; }); - it('mounts', () => { + it('mounts', async () => { + sourceControlStore.getAggregatedStatus.mockResolvedValue([]); + const { getByText } = renderModal({ pinia, props: { @@ -163,7 +164,10 @@ describe('SourceControlPushModal', () => { }, }, }); - expect(getByText('Commit and push changes')).toBeInTheDocument(); + + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); }); it('should toggle checkboxes', async () => { @@ -190,7 +194,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -199,8 +206,17 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible (v-if="!isLoading" check passes) + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const files = getAllByTestId('source-control-push-modal-file-checkbox'); + expect(files).toHaveLength(2); + }); + const files = getAllByTestId('source-control-push-modal-file-checkbox'); - expect(files).toHaveLength(2); await userEvent.click(files[0]); expect(within(files[0]).getByRole('checkbox')).toBeChecked(); @@ -299,7 +315,9 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getByRole } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getByRole, getByText } = renderModal({ pinia, props: { data: { @@ -309,9 +327,18 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const submitButton = getByTestId('source-control-push-modal-submit'); + expect(submitButton).toBeDisabled(); + }); + const submitButton = getByTestId('source-control-push-modal-submit'); const commitMessage = 'commit message'; - expect(submitButton).toBeDisabled(); expect(getByRole('alert').textContent).toContain( [ @@ -360,10 +387,12 @@ describe('SourceControlPushModal', () => { }, ]; + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + mockRoute.name = VIEWS.WORKFLOW; mockRoute.params = { name: 'gTbbBkkYTnNyX1jD' }; - const { getByTestId, getAllByTestId } = renderModal({ + const { getByTestId, getAllByTestId, getByText } = renderModal({ pinia, props: { data: { @@ -373,8 +402,17 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const files = getAllByTestId('source-control-push-modal-file-checkbox'); + expect(files).toHaveLength(2); + }); + const files = getAllByTestId('source-control-push-modal-file-checkbox'); - expect(files).toHaveLength(2); // The current workflow should be auto-selected now that we fixed the regression expect(within(files[0]).getByRole('checkbox')).toBeChecked(); @@ -419,7 +457,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -428,8 +469,15 @@ describe('SourceControlPushModal', () => { }, }); - const workflows = getAllByTestId('source-control-push-modal-file-checkbox'); - expect(workflows).toHaveLength(2); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const workflows = getAllByTestId('source-control-push-modal-file-checkbox'); + expect(workflows).toHaveLength(2); + }); const tab = getAllByTestId('source-control-push-modal-tab').filter(({ textContent }) => textContent?.includes('Credentials'), @@ -467,7 +515,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -476,7 +527,14 @@ describe('SourceControlPushModal', () => { }, }); - expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + }); await userEvent.type(getByTestId('source-control-push-search'), '1'); await waitFor(() => { @@ -511,7 +569,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -520,7 +581,14 @@ describe('SourceControlPushModal', () => { }, }); - expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + }); await userEvent.click(getByTestId('source-control-filter-dropdown')); @@ -588,7 +656,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -597,13 +668,27 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const tab = getAllByTestId('source-control-push-modal-tab').filter(({ textContent }) => + textContent?.includes(name), + ); + expect(tab.length).toBeGreaterThan(0); + }); + const tab = getAllByTestId('source-control-push-modal-tab').filter(({ textContent }) => textContent?.includes(name), ); await userEvent.click(tab[0]); - expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + await waitFor(() => { + expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(2); + }); await userEvent.click(getByTestId('source-control-filter-dropdown')); @@ -635,7 +720,10 @@ describe('SourceControlPushModal', () => { }, ]; - const { getByTestId, getAllByTestId, queryAllByTestId } = renderModal({ + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); + + const { getByTestId, getAllByTestId, queryAllByTestId, getByText } = renderModal({ + pinia, props: { data: { eventBus, @@ -644,7 +732,14 @@ describe('SourceControlPushModal', () => { }, }); - expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(1); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(getAllByTestId('source-control-push-modal-file-checkbox')).toHaveLength(1); + }); await userEvent.click(getByTestId('source-control-filter-dropdown')); @@ -689,9 +784,9 @@ describe('SourceControlPushModal', () => { }, ]; - // Use the existing store instance from beforeEach + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); - const { getByTestId } = renderModal({ + const { getByTestId, getByText } = renderModal({ pinia, props: { data: { @@ -701,6 +796,15 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(getByTestId('source-control-push-modal-commit')).toBeInTheDocument(); + }); + const commitInput = getByTestId('source-control-push-modal-commit'); const commitMessage = 'Test commit message'; @@ -732,9 +836,9 @@ describe('SourceControlPushModal', () => { }, ]; - // Use the existing store instance from beforeEach + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); - const { getByTestId } = renderModal({ + const { getByTestId, getByText } = renderModal({ pinia, props: { data: { @@ -744,6 +848,15 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(getByTestId('source-control-push-modal-commit')).toBeInTheDocument(); + }); + const commitInput = getByTestId('source-control-push-modal-commit'); expect(getByTestId('source-control-push-modal-submit')).toBeDisabled(); @@ -767,12 +880,12 @@ describe('SourceControlPushModal', () => { }, ]; - // Use the existing store instance from beforeEach + sourceControlStore.getAggregatedStatus.mockResolvedValue(status); mockRoute.name = 'SOME_OTHER_VIEW'; mockRoute.params = { name: 'differentId' }; - const { getByTestId, getAllByTestId } = renderModal({ + const { getByTestId, getAllByTestId, getByText } = renderModal({ pinia, props: { data: { @@ -782,11 +895,20 @@ describe('SourceControlPushModal', () => { }, }); + // Wait for modal content to be visible + await waitFor(() => { + expect(getByText('Commit and push changes')).toBeInTheDocument(); + }); + + await waitFor(() => { + const files = getAllByTestId('source-control-push-modal-file-checkbox'); + expect(files).toHaveLength(1); + }); + const commitInput = getByTestId('source-control-push-modal-commit'); const commitMessage = 'Test commit message'; const files = getAllByTestId('source-control-push-modal-file-checkbox'); - expect(files).toHaveLength(1); expect(within(files[0]).getByRole('checkbox')).not.toBeChecked(); await userEvent.type(commitInput, commitMessage); diff --git a/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.vue b/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.vue index 89e23b4b54e..b23ac9a55a0 100644 --- a/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.vue +++ b/packages/frontend/editor-ui/src/features/sourceControl.ee/components/SourceControlPushModal.vue @@ -659,10 +659,8 @@ watchEffect(() => { // Load data when modal opens onMounted(async () => { - // Only load fresh data if we don't have any initial data - if (!props.data.status || props.data.status.length === 0) { - await loadSourceControlStatus(); - } + // Always load fresh data to ensure workflow names are populated + await loadSourceControlStatus(); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.test.ts b/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.test.ts index cbb2ff0bbad..b387d73d118 100644 --- a/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.test.ts @@ -17,12 +17,26 @@ const mockRoute = reactive({ name: '', params: {}, fullPath: '', + query: {}, }); +const mockRouterBack = vi.fn(); +const mockRouterReplace = vi.fn(); +const mockShowError = vi.fn(); + +vi.mock('@/composables/useToast', () => ({ + useToast: vi.fn(() => ({ + showError: mockShowError, + })), +})); + vi.mock('vue-router', () => ({ useRoute: () => mockRoute, RouterLink: vi.fn(), - useRouter: vi.fn(), + useRouter: () => ({ + back: mockRouterBack, + replace: mockRouterReplace, + }), })); vi.mock('@/features/workflow-diff/useViewportSync', () => ({ @@ -106,6 +120,17 @@ describe('WorkflowDiffModal', () => { beforeEach(() => { vi.clearAllMocks(); + mockShowError.mockClear(); + mockRouterBack.mockClear(); + mockRouterReplace.mockClear(); + + // Mock window.history.length to simulate having navigation history + Object.defineProperty(window.history, 'length', { + writable: true, + configurable: true, + value: 2, + }); + createTestingPinia(); nodeTypesStore = mockedStore(useNodeTypesStore); sourceControlStore = mockedStore(useSourceControlStore); @@ -451,4 +476,219 @@ describe('WorkflowDiffModal', () => { }); }); }); + + describe('Error Handling - Remote Workflow', () => { + it('should show toast error and close modal when remote workflow fails to load', async () => { + sourceControlStore.getRemoteWorkflow.mockRejectedValue( + new Error('Remote API error') as never, + ); + + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(mockShowError).toHaveBeenCalledTimes(1); + expect(mockRouterBack).toHaveBeenCalledTimes(1); + }); + }); + + it('should continue loading local workflow when remote fails', async () => { + sourceControlStore.getRemoteWorkflow.mockRejectedValue( + new Error('Remote API error') as never, + ); + + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(workflowsStore.fetchWorkflow).toHaveBeenCalledWith('test-workflow-id'); + }); + }); + }); + + describe('Error Handling - Local Workflow', () => { + it('should show toast error and close modal when local workflow fails to load', async () => { + workflowsStore.fetchWorkflow.mockRejectedValue(new Error('Local API error') as never); + + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(mockShowError).toHaveBeenCalledTimes(1); + expect(mockRouterBack).toHaveBeenCalledTimes(1); + }); + }); + + it('should continue loading remote workflow when local fails', async () => { + workflowsStore.fetchWorkflow.mockRejectedValue(new Error('Local API error') as never); + + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(sourceControlStore.getRemoteWorkflow).toHaveBeenCalledWith('test-workflow-id'); + }); + }); + }); + + describe('Error Handling - Both Workflows', () => { + it('should show both toast errors but close modal only once when both workflows fail', async () => { + sourceControlStore.getRemoteWorkflow.mockRejectedValue( + new Error('Remote API error') as never, + ); + workflowsStore.fetchWorkflow.mockRejectedValue(new Error('Local API error') as never); + + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(mockShowError).toHaveBeenCalledTimes(2); + expect(mockRouterBack).toHaveBeenCalledTimes(1); + }); + }); + }); + + describe('Successful Loading', () => { + it('should load both remote and local workflows successfully', async () => { + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(sourceControlStore.getRemoteWorkflow).toHaveBeenCalledWith('test-workflow-id'); + expect(workflowsStore.fetchWorkflow).toHaveBeenCalledWith('test-workflow-id'); + }); + }); + + it('should load node types before fetching workflows', async () => { + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(nodeTypesStore.loadNodeTypesIfNotLoaded).toHaveBeenCalled(); + }); + }); + + it('should not show toast errors on successful load', async () => { + renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + await vi.waitFor(() => { + expect(sourceControlStore.getRemoteWorkflow).toHaveBeenCalled(); + expect(workflowsStore.fetchWorkflow).toHaveBeenCalled(); + }); + + expect(mockShowError).not.toHaveBeenCalled(); + }); + }); + + describe('handleBeforeClose behavior', () => { + it('should call router.back when history length > 1', async () => { + Object.defineProperty(window.history, 'length', { + writable: true, + configurable: true, + value: 2, + }); + + const { container } = renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + const backButton = container.querySelector('[data-icon="arrow-left"]'); + await userEvent.click(backButton!); + + expect(mockRouterBack).toHaveBeenCalledTimes(1); + expect(mockRouterReplace).not.toHaveBeenCalled(); + }); + + it('should call router.replace when history length = 1', async () => { + Object.defineProperty(window.history, 'length', { + writable: true, + configurable: true, + value: 1, + }); + + mockRoute.query = { diff: 'workflow-id', direction: 'push', other: 'param' }; + + const { container } = renderModal({ + props: { + data: { + eventBus, + workflowId: 'test-workflow-id', + direction: 'push', + }, + }, + }); + + const backButton = container.querySelector('[data-icon="arrow-left"]'); + await userEvent.click(backButton!); + + expect(mockRouterBack).not.toHaveBeenCalled(); + expect(mockRouterReplace).toHaveBeenCalledTimes(1); + expect(mockRouterReplace).toHaveBeenCalledWith({ + query: { other: 'param' }, + }); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.vue b/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.vue index aac1b839558..69b1cdb5665 100644 --- a/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.vue +++ b/packages/frontend/editor-ui/src/features/workflow-diff/WorkflowDiffModal.vue @@ -3,6 +3,7 @@ import Node from '@/features/canvas/components/elements/nodes/CanvasNode.vue'; import Modal from '@/components/Modal.vue'; import NodeIcon from '@/components/NodeIcon.vue'; import { useTelemetry } from '@/composables/useTelemetry'; +import { useToast } from '@/composables/useToast'; import { STICKY_NODE_TYPE, WORKFLOW_DIFF_MODAL_KEY } from '@/constants'; import DiffBadge from '@/features/workflow-diff/DiffBadge.vue'; import NodeDiff from '@/features/workflow-diff/NodeDiff.vue'; @@ -41,6 +42,7 @@ const props = defineProps<{ const { selectedDetailId, onNodeClick, syncIsEnabled } = useProvideViewportSync(); const telemetry = useTelemetry(); +const toast = useToast(); const $style = useCssModule(); const nodeTypesStore = useNodeTypesStore(); const sourceControlStore = useSourceControlStore(); @@ -56,13 +58,17 @@ const manualAsyncConfiguration = { immediate: false, } as const; +const isClosed = ref(false); + const remote = useAsyncState<{ workflow?: IWorkflowDb; remote: boolean } | undefined, [], false>( async () => { try { const { workflowId } = props.data; const { content: workflow } = await sourceControlStore.getRemoteWorkflow(workflowId); return { workflow, remote: true }; - } catch { + } catch (error) { + toast.showError(error, i18n.baseText('generic.error')); + handleBeforeClose(); return { workflow: undefined, remote: true }; } }, @@ -76,7 +82,9 @@ const local = useAsyncState<{ workflow?: IWorkflowDb; remote: boolean } | undefi const { workflowId } = props.data; const workflow = await workflowsStore.fetchWorkflow(workflowId); return { workflow, remote: false }; - } catch { + } catch (error) { + toast.showError(error, i18n.baseText('generic.error')); + handleBeforeClose(); return { workflow: undefined, remote: false }; } }, @@ -84,12 +92,6 @@ const local = useAsyncState<{ workflow?: IWorkflowDb; remote: boolean } | undefi manualAsyncConfiguration, ); -useAsyncState(async () => { - await Promise.all([nodeTypesStore.loadNodeTypesIfNotLoaded()]); - await Promise.all([remote.execute(), local.execute()]); - return true; -}, false); - const sourceWorkFlow = computed(() => (props.data.direction === 'push' ? remote : local)); const targetWorkFlow = computed(() => (props.data.direction === 'push' ? local : remote)); @@ -301,6 +303,9 @@ const nodeDiffs = computed(() => { }); function handleBeforeClose() { + if (isClosed.value) return; + isClosed.value = true; + selectedDetailId.value = undefined; // Check if we have history to go back to avoid empty navigation issues @@ -325,8 +330,11 @@ function handleEscapeKey(event: KeyboardEvent) { } } -onMounted(() => { +onMounted(async () => { document.addEventListener('keydown', handleEscapeKey, true); + await nodeTypesStore.loadNodeTypesIfNotLoaded(); + void remote.execute(); + void local.execute(); }); onUnmounted(() => { @@ -348,7 +356,7 @@ const isSourceWorkflowNew = computed(() => { return !sourceExists && targetExists; }); -onNodeClick((nodeId) => { +onNodeClick((nodeId: string) => { const node = nodesDiff.value.get(nodeId); if (!node) { return;