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.
This commit is contained in:
Milorad FIlipović 2023-12-22 08:42:53 +01:00 committed by GitHub
parent 711fa2b925
commit 39e45d8b92
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 84 additions and 20 deletions

View file

@ -1,12 +1,14 @@
import { CODE_NODE_NAME, SET_NODE_NAME, EDIT_FIELDS_SET_NODE_NAME } from './../constants'; import { CODE_NODE_NAME, SET_NODE_NAME, EDIT_FIELDS_SET_NODE_NAME } from './../constants';
import { SCHEDULE_TRIGGER_NODE_NAME } from '../constants'; import { SCHEDULE_TRIGGER_NODE_NAME } from '../constants';
import { WorkflowPage as WorkflowPageClass } from '../pages/workflow'; import { WorkflowPage as WorkflowPageClass } from '../pages/workflow';
import { MessageBox as MessageBoxClass } from '../pages/modals/message-box';
import { NDV } from '../pages/ndv'; import { NDV } from '../pages/ndv';
// Suite-specific constants // Suite-specific constants
const CODE_NODE_NEW_NAME = 'Something else'; const CODE_NODE_NEW_NAME = 'Something else';
const WorkflowPage = new WorkflowPageClass(); const WorkflowPage = new WorkflowPageClass();
const messageBox = new MessageBoxClass();
const ndv = new NDV(); const ndv = new NDV();
describe('Undo/Redo', () => { describe('Undo/Redo', () => {
@ -354,4 +356,36 @@ describe('Undo/Redo', () => {
.should('have.css', 'left', `637px`) .should('have.css', 'left', `637px`)
.should('have.css', 'top', `501px`); .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);
});
}); });

View file

@ -47,7 +47,12 @@
<template #footer> <template #footer>
<div class="action-buttons"> <div class="action-buttons">
<n8n-button @click="closeDialog" float="right" :label="$locale.baseText('about.close')" /> <n8n-button
@click="closeDialog"
float="right"
:label="$locale.baseText('about.close')"
data-test-id="close-about-modal-button"
/>
</div> </div>
</template> </template>
</Modal> </Modal>

View file

@ -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', () => ({ vi.mock('vue-router', () => ({
useRoute: () => ({}), useRoute: () => ({}),
})); }));

View file

@ -5,13 +5,14 @@ import { BulkCommand, Command } from '@/models/history';
import { useHistoryStore } from '@/stores/history.store'; import { useHistoryStore } from '@/stores/history.store';
import { useUIStore } from '@/stores/ui.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 { useDebounceHelper } from './useDebounce';
import { useDeviceSupport } from 'n8n-design-system/composables/useDeviceSupport'; import { useDeviceSupport } from 'n8n-design-system/composables/useDeviceSupport';
import { getNodeViewTab } from '@/utils/canvasUtils'; import { getNodeViewTab } from '@/utils/canvasUtils';
import type { Route } from 'vue-router'; import type { Route } from 'vue-router';
const UNDO_REDO_DEBOUNCE_INTERVAL = 100; const UNDO_REDO_DEBOUNCE_INTERVAL = 100;
const ELEMENT_UI_OVERLAY_SELECTOR = '.el-overlay';
export function useHistoryHelper(activeRoute: Route) { export function useHistoryHelper(activeRoute: Route) {
const instance = getCurrentInstance(); const instance = getCurrentInstance();
@ -24,8 +25,6 @@ export function useHistoryHelper(activeRoute: Route) {
const { callDebounced } = useDebounceHelper(); const { callDebounced } = useDebounceHelper();
const { isCtrlKeyPressed } = useDeviceSupport(); const { isCtrlKeyPressed } = useDeviceSupport();
const isNDVOpen = ref<boolean>(ndvStore.activeNodeName !== null);
const undo = async () => const undo = async () =>
callDebounced( callDebounced(
async () => { async () => {
@ -95,29 +94,43 @@ export function useHistoryHelper(activeRoute: Route) {
} }
} }
function trackUndoAttempt(event: KeyboardEvent) { function trackUndoAttempt() {
if (isNDVOpen.value && !event.shiftKey) { const activeNode = ndvStore.activeNode;
const activeNode = ndvStore.activeNode; if (activeNode) {
if (activeNode) { telemetry?.track('User hit undo in NDV', { node_type: activeNode.type });
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) { function handleKeyDown(event: KeyboardEvent) {
const currentNodeViewTab = getNodeViewTab(activeRoute); 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 (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(); event.preventDefault();
if (!isNDVOpen.value) { if (event.shiftKey) {
if (event.shiftKey) { void redo();
void redo(); } else {
} else { void undo();
void undo();
}
} else if (!event.shiftKey) {
trackUndoAttempt(event);
} }
} }
} }

View file

@ -139,6 +139,9 @@ export const useNDVStore = defineStore(STORES.NDV, {
return null; return null;
}, },
isNDVOpen(): boolean {
return this.activeNodeName !== null;
},
}, },
actions: { actions: {
setActiveNodeName(nodeName: string | null): void { setActiveNodeName(nodeName: string | null): void {

View file

@ -410,6 +410,9 @@ export const useUIStore = defineStore(STORES.UI, {
const style = getComputedStyle(document.body); const style = getComputedStyle(document.body);
return Number(style.getPropertyValue('--header-height')); return Number(style.getPropertyValue('--header-height'));
}, },
isAnyModalOpen(): boolean {
return this.modalStack.length > 0;
},
}, },
actions: { actions: {
setTheme(theme: ThemeOption): void { setTheme(theme: ThemeOption): void {