From 39e45d8b929d474f1e7587329b003fe15b61636d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milorad=20FIlipovi=C4=87?= Date: Fri, 22 Dec 2023 08:42:53 +0100 Subject: [PATCH] fix(editor): Prevent canvas undo/redo when NDV is open (#8118) ## Summary Preventing canvas undo/redo while NDV or any modal is open. We already had a NDV open check in place but looks like it was broken by unreactive ref inside `useHistoryHelper` composable. This PR fixes this by using store getter directly inside the helper method and adds modal open check. ## Related tickets and issues Fixes ADO-657 ## Review / Merge checklist - [ ] PR title and summary are descriptive. **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** ([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md)) - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [ ] Tests included. > A bug is not considered fixed, unless a test is added to prevent it from happening again. > A feature is not complete without tests. --- cypress/e2e/10-undo-redo.cy.ts | 34 +++++++++++++ .../editor-ui/src/components/AboutModal.vue | 7 ++- .../__tests__/useHistoryHelper.test.ts | 8 ++- .../src/composables/useHistoryHelper.ts | 49 ++++++++++++------- packages/editor-ui/src/stores/ndv.store.ts | 3 ++ packages/editor-ui/src/stores/ui.store.ts | 3 ++ 6 files changed, 84 insertions(+), 20 deletions(-) diff --git a/cypress/e2e/10-undo-redo.cy.ts b/cypress/e2e/10-undo-redo.cy.ts index e904d891b1..4182c75507 100644 --- a/cypress/e2e/10-undo-redo.cy.ts +++ b/cypress/e2e/10-undo-redo.cy.ts @@ -1,12 +1,14 @@ import { CODE_NODE_NAME, SET_NODE_NAME, EDIT_FIELDS_SET_NODE_NAME } from './../constants'; import { SCHEDULE_TRIGGER_NODE_NAME } from '../constants'; import { WorkflowPage as WorkflowPageClass } from '../pages/workflow'; +import { MessageBox as MessageBoxClass } from '../pages/modals/message-box'; import { NDV } from '../pages/ndv'; // Suite-specific constants const CODE_NODE_NEW_NAME = 'Something else'; const WorkflowPage = new WorkflowPageClass(); +const messageBox = new MessageBoxClass(); const ndv = new NDV(); describe('Undo/Redo', () => { @@ -354,4 +356,36 @@ describe('Undo/Redo', () => { .should('have.css', 'left', `637px`) .should('have.css', 'top', `501px`); }); + + it('should not undo/redo when NDV or a modal is open', () => { + WorkflowPage.actions.addInitialNodeToCanvas(SCHEDULE_TRIGGER_NODE_NAME, { keepNdvOpen: true }); + // Try while NDV is open + WorkflowPage.actions.hitUndo(); + WorkflowPage.getters.canvasNodes().should('have.have.length', 1); + ndv.getters.backToCanvas().click(); + // Try while modal is open + cy.getByTestId('menu-item').contains('About n8n').click({ force: true }); + cy.getByTestId('about-modal').should('be.visible'); + WorkflowPage.actions.hitUndo(); + WorkflowPage.getters.canvasNodes().should('have.have.length', 1); + cy.getByTestId('close-about-modal-button').click(); + // Should work now + WorkflowPage.actions.hitUndo(); + WorkflowPage.getters.canvasNodes().should('have.have.length', 0); + }); + + it('should not undo/redo when NDV or a prompt is open', () => { + WorkflowPage.actions.addInitialNodeToCanvas(SCHEDULE_TRIGGER_NODE_NAME, { keepNdvOpen: false }); + WorkflowPage.getters.workflowMenu().click(); + WorkflowPage.getters.workflowMenuItemImportFromURLItem().should('be.visible'); + WorkflowPage.getters.workflowMenuItemImportFromURLItem().click(); + // Try while prompt is open + messageBox.getters.header().click(); + WorkflowPage.actions.hitUndo(); + WorkflowPage.getters.canvasNodes().should('have.have.length', 1); + // Close prompt and try again + messageBox.actions.cancel(); + WorkflowPage.actions.hitUndo(); + WorkflowPage.getters.canvasNodes().should('have.have.length', 0); + }); }); diff --git a/packages/editor-ui/src/components/AboutModal.vue b/packages/editor-ui/src/components/AboutModal.vue index 2be4c4faa6..4893e4e62e 100644 --- a/packages/editor-ui/src/components/AboutModal.vue +++ b/packages/editor-ui/src/components/AboutModal.vue @@ -47,7 +47,12 @@ diff --git a/packages/editor-ui/src/composables/__tests__/useHistoryHelper.test.ts b/packages/editor-ui/src/composables/__tests__/useHistoryHelper.test.ts index 9176fd9d2e..a035654b1f 100644 --- a/packages/editor-ui/src/composables/__tests__/useHistoryHelper.test.ts +++ b/packages/editor-ui/src/composables/__tests__/useHistoryHelper.test.ts @@ -22,7 +22,13 @@ vi.mock('@/stores/history.store', () => { }), }; }); -vi.mock('@/stores/ui.store'); +vi.mock('@/stores/ui.store', () => { + return { + useUIStore: () => ({ + isAnyModalOpen: false, + }), + }; +}); vi.mock('vue-router', () => ({ useRoute: () => ({}), })); diff --git a/packages/editor-ui/src/composables/useHistoryHelper.ts b/packages/editor-ui/src/composables/useHistoryHelper.ts index a221586654..599154f3f9 100644 --- a/packages/editor-ui/src/composables/useHistoryHelper.ts +++ b/packages/editor-ui/src/composables/useHistoryHelper.ts @@ -5,13 +5,14 @@ import { BulkCommand, Command } from '@/models/history'; import { useHistoryStore } from '@/stores/history.store'; import { useUIStore } from '@/stores/ui.store'; -import { ref, onMounted, onUnmounted, nextTick, getCurrentInstance } from 'vue'; +import { onMounted, onUnmounted, nextTick, getCurrentInstance } from 'vue'; import { useDebounceHelper } from './useDebounce'; import { useDeviceSupport } from 'n8n-design-system/composables/useDeviceSupport'; import { getNodeViewTab } from '@/utils/canvasUtils'; import type { Route } from 'vue-router'; const UNDO_REDO_DEBOUNCE_INTERVAL = 100; +const ELEMENT_UI_OVERLAY_SELECTOR = '.el-overlay'; export function useHistoryHelper(activeRoute: Route) { const instance = getCurrentInstance(); @@ -24,8 +25,6 @@ export function useHistoryHelper(activeRoute: Route) { const { callDebounced } = useDebounceHelper(); const { isCtrlKeyPressed } = useDeviceSupport(); - const isNDVOpen = ref(ndvStore.activeNodeName !== null); - const undo = async () => callDebounced( async () => { @@ -95,29 +94,43 @@ export function useHistoryHelper(activeRoute: Route) { } } - function trackUndoAttempt(event: KeyboardEvent) { - if (isNDVOpen.value && !event.shiftKey) { - const activeNode = ndvStore.activeNode; - if (activeNode) { - telemetry?.track('User hit undo in NDV', { node_type: activeNode.type }); - } + function trackUndoAttempt() { + const activeNode = ndvStore.activeNode; + if (activeNode) { + telemetry?.track('User hit undo in NDV', { node_type: activeNode.type }); } } + /** + * Checks if there is a Element UI dialog open by querying + * for the visible overlay element. + */ + function isMessageDialogOpen(): boolean { + return ( + document.querySelector(`${ELEMENT_UI_OVERLAY_SELECTOR}:not([style*="display: none"])`) !== + null + ); + } + function handleKeyDown(event: KeyboardEvent) { const currentNodeViewTab = getNodeViewTab(activeRoute); + const isNDVOpen = ndvStore.isNDVOpen; + const isAnyModalOpen = uiStore.isAnyModalOpen || isMessageDialogOpen(); + const undoKeysPressed = isCtrlKeyPressed(event) && event.key.toLowerCase() === 'z'; if (event.repeat || currentNodeViewTab !== MAIN_HEADER_TABS.WORKFLOW) return; - if (isCtrlKeyPressed(event) && event.key.toLowerCase() === 'z') { + if (isNDVOpen || isAnyModalOpen) { + if (isNDVOpen && undoKeysPressed && !event.shiftKey) { + trackUndoAttempt(); + } + return; + } + if (undoKeysPressed) { event.preventDefault(); - if (!isNDVOpen.value) { - if (event.shiftKey) { - void redo(); - } else { - void undo(); - } - } else if (!event.shiftKey) { - trackUndoAttempt(event); + if (event.shiftKey) { + void redo(); + } else { + void undo(); } } } diff --git a/packages/editor-ui/src/stores/ndv.store.ts b/packages/editor-ui/src/stores/ndv.store.ts index 5ca6dca6d8..062cf05ac6 100644 --- a/packages/editor-ui/src/stores/ndv.store.ts +++ b/packages/editor-ui/src/stores/ndv.store.ts @@ -139,6 +139,9 @@ export const useNDVStore = defineStore(STORES.NDV, { return null; }, + isNDVOpen(): boolean { + return this.activeNodeName !== null; + }, }, actions: { setActiveNodeName(nodeName: string | null): void { diff --git a/packages/editor-ui/src/stores/ui.store.ts b/packages/editor-ui/src/stores/ui.store.ts index 6eeb2f1a5b..9667f80e5c 100644 --- a/packages/editor-ui/src/stores/ui.store.ts +++ b/packages/editor-ui/src/stores/ui.store.ts @@ -410,6 +410,9 @@ export const useUIStore = defineStore(STORES.UI, { const style = getComputedStyle(document.body); return Number(style.getPropertyValue('--header-height')); }, + isAnyModalOpen(): boolean { + return this.modalStack.length > 0; + }, }, actions: { setTheme(theme: ThemeOption): void {