fix(editor): Fix save keybind in expression editor and unfocused node details view (#13640)

cypress/e2e/9-expression-editor-modal.cy.ts

@@ -1,4 +1,6 @@
+import { META_KEY } from '../constants';
 import { NDV } from '../pages/ndv';
+import { successToast } from '../pages/notifications';
 import { WorkflowPage as WorkflowPageClass } from '../pages/workflow';
 
 const WorkflowPage = new WorkflowPageClass();
@@ -11,6 +13,23 @@ describe('Expression editor modal', () => {
 		cy.on('uncaught:exception', (error) => error.name !== 'ExpressionError');
 	});
 
+	describe('Keybinds', () => {
+		beforeEach(() => {
+			WorkflowPage.actions.addNodeToCanvas('Hacker News');
+			WorkflowPage.actions.zoomToFit();
+			WorkflowPage.actions.openNode('Hacker News');
+			WorkflowPage.actions.openExpressionEditorModal();
+		});
+
+		it('should save the workflow with save keybind', () => {
+			WorkflowPage.getters.expressionModalInput().clear();
+			WorkflowPage.getters.expressionModalInput().click().type('{{ "hello"');
+			WorkflowPage.getters.expressionModalOutput().contains('hello');
+			WorkflowPage.getters.expressionModalInput().click().type(`{${META_KEY}+s}`);
+			successToast().should('be.visible');
+		});
+	});
+
 	describe('Static data', () => {
 		beforeEach(() => {
 			WorkflowPage.actions.addNodeToCanvas('Hacker News');

packages/frontend/editor-ui/src/components/NodeDetailsView.test.ts

@@ -1,5 +1,5 @@
 import { createPinia, setActivePinia } from 'pinia';
-import { waitFor } from '@testing-library/vue';
+import { waitFor, waitForElementToBeRemoved, fireEvent } from '@testing-library/vue';
 import { mock } from 'vitest-mock-extended';
 
 import NodeDetailsView from '@/components/NodeDetailsView.vue';
@@ -24,7 +24,7 @@ vi.mock('vue-router', () => {
 	};
 });
 
-async function createPiniaWithActiveNode() {
+async function createPiniaStore(isActiveNode: boolean) {
 	const node = mockNodes[0];
 	const workflow = mock<IWorkflowDb>({
 		connections: {},
@@ -42,12 +42,19 @@ async function createPiniaWithActiveNode() {
 	nodeTypesStore.setNodeTypes(defaultNodeDescriptions);
 	workflowsStore.workflow = workflow;
 	workflowsStore.nodeMetadata[node.name] = { pristine: true };
-	ndvStore.activeNodeName = node.name;
+
+	if (isActiveNode) {
+		ndvStore.activeNodeName = node.name;
+	}
 
 	await useSettingsStore().getSettings();
 	await useUsersStore().loginWithCookie();
 
-	return { pinia, currentWorkflow: workflowsStore.getCurrentWorkflow() };
+	return {
+		pinia,
+		currentWorkflow: workflowsStore.getCurrentWorkflow(),
+		nodeName: node.name,
+	};
 }
 
 describe('NodeDetailsView', () => {
@@ -71,7 +78,7 @@ describe('NodeDetailsView', () => {
 	});
 
 	it('should render correctly', async () => {
-		const { pinia, currentWorkflow } = await createPiniaWithActiveNode();
+		const { pinia, currentWorkflow } = await createPiniaStore(true);
 
 		const renderComponent = createComponentRenderer(NodeDetailsView, {
 			props: {
@@ -94,4 +101,134 @@ describe('NodeDetailsView', () => {
 
 		await waitFor(() => expect(getByTestId('ndv')).toBeInTheDocument());
 	});
+
+	describe('keyboard listener', () => {
+		test('should register and unregister keydown listener based on modal open state', async () => {
+			const { pinia, currentWorkflow, nodeName } = await createPiniaStore(false);
+			const ndvStore = useNDVStore();
+
+			const renderComponent = createComponentRenderer(NodeDetailsView, {
+				props: {
+					teleported: false,
+					appendToBody: false,
+					workflowObject: currentWorkflow,
+				},
+				global: {
+					mocks: {
+						$route: {
+							name: VIEWS.WORKFLOW,
+						},
+					},
+				},
+			});
+
+			const { getByTestId, queryByTestId } = renderComponent({
+				pinia,
+			});
+
+			const addEventListenerSpy = vi.spyOn(document, 'addEventListener');
+			const removeEventListenerSpy = vi.spyOn(document, 'removeEventListener');
+
+			ndvStore.activeNodeName = nodeName;
+
+			await waitFor(() => expect(getByTestId('ndv')).toBeInTheDocument());
+			await waitFor(() => expect(queryByTestId('ndv-modal')).toBeInTheDocument());
+
+			expect(addEventListenerSpy).toHaveBeenCalledWith('keydown', expect.any(Function), true);
+			expect(removeEventListenerSpy).not.toHaveBeenCalledWith(
+				'keydown',
+				expect.any(Function),
+				true,
+			);
+
+			ndvStore.activeNodeName = null;
+
+			await waitForElementToBeRemoved(queryByTestId('ndv-modal'));
+
+			expect(removeEventListenerSpy).toHaveBeenCalledWith('keydown', expect.any(Function), true);
+
+			addEventListenerSpy.mockRestore();
+			removeEventListenerSpy.mockRestore();
+		});
+
+		test('should unregister keydown listener on unmount', async () => {
+			const { pinia, currentWorkflow, nodeName } = await createPiniaStore(false);
+			const ndvStore = useNDVStore();
+
+			const renderComponent = createComponentRenderer(NodeDetailsView, {
+				props: {
+					teleported: false,
+					appendToBody: false,
+					workflowObject: currentWorkflow,
+				},
+				global: {
+					mocks: {
+						$route: {
+							name: VIEWS.WORKFLOW,
+						},
+					},
+				},
+			});
+
+			const { getByTestId, queryByTestId, unmount } = renderComponent({
+				pinia,
+			});
+
+			ndvStore.activeNodeName = nodeName;
+
+			await waitFor(() => expect(getByTestId('ndv')).toBeInTheDocument());
+			await waitFor(() => expect(queryByTestId('ndv-modal')).toBeInTheDocument());
+
+			const removeEventListenerSpy = vi.spyOn(document, 'removeEventListener');
+			expect(removeEventListenerSpy).not.toHaveBeenCalledWith(
+				'keydown',
+				expect.any(Function),
+				true,
+			);
+
+			unmount();
+
+			expect(removeEventListenerSpy).toHaveBeenCalledWith('keydown', expect.any(Function), true);
+
+			removeEventListenerSpy.mockRestore();
+		});
+
+		test("should emit 'saveKeyboardShortcut' when save shortcut keybind is pressed", async () => {
+			const { pinia, currentWorkflow, nodeName } = await createPiniaStore(false);
+			const ndvStore = useNDVStore();
+
+			const renderComponent = createComponentRenderer(NodeDetailsView, {
+				props: {
+					teleported: false,
+					appendToBody: false,
+					workflowObject: currentWorkflow,
+				},
+				global: {
+					mocks: {
+						$route: {
+							name: VIEWS.WORKFLOW,
+						},
+					},
+				},
+			});
+
+			const { getByTestId, queryByTestId, emitted } = renderComponent({
+				pinia,
+			});
+
+			ndvStore.activeNodeName = nodeName;
+
+			await waitFor(() => expect(getByTestId('ndv')).toBeInTheDocument());
+			await waitFor(() => expect(queryByTestId('ndv-modal')).toBeInTheDocument());
+
+			await fireEvent.keyDown(getByTestId('ndv'), {
+				key: 's',
+				ctrlKey: true,
+				bubbles: true,
+				cancelable: true,
+			});
+
+			expect(emitted().saveKeyboardShortcut).toBeTruthy();
+		});
+	});
 });

packages/frontend/editor-ui/src/components/NodeDetailsView.vue

@@ -352,15 +352,19 @@ const setIsTooltipVisible = ({ isTooltipVisible }: DataPinningDiscoveryEvent) =>
 
 const onKeyDown = (e: KeyboardEvent) => {
 	if (e.key === 's' && deviceSupport.isCtrlKeyPressed(e)) {
-		e.stopPropagation();
+		onSaveWorkflow(e);
-		e.preventDefault();
-
-		if (props.readOnly) return;
-
-		emit('saveKeyboardShortcut', e);
 	}
 };
 
+const onSaveWorkflow = (e: KeyboardEvent) => {
+	e.stopPropagation();
+	e.preventDefault();
+
+	if (props.readOnly) return;
+
+	emit('saveKeyboardShortcut', e);
+};
+
 const onInputItemHover = (e: { itemIndex: number; outputIndex: number } | null) => {
 	if (e === null || !inputNodeName.value || !isPairedItemHoveringEnabled.value) {
 		ndvStore.setHoveringItem(null);
@@ -597,11 +601,25 @@ const onSearch = (search: string) => {
 	isPairedItemHoveringEnabled.value = !search;
 };
 
+const registerKeyboardListener = () => {
+	document.addEventListener('keydown', onKeyDown, true);
+};
+
+const unregisterKeyboardListener = () => {
+	document.removeEventListener('keydown', onKeyDown, true);
+};
+
 //watchers
 
 watch(
 	activeNode,
 	(node, oldNode) => {
+		if (node && !oldNode) {
+			registerKeyboardListener();
+		} else if (!node) {
+			unregisterKeyboardListener();
+		}
+
 		if (node && node.name !== oldNode?.name && !isActiveStickyNode.value) {
 			runInputIndex.value = -1;
 			runOutputIndex.value = -1;
@@ -655,6 +673,7 @@ watch(
 	},
 	{ immediate: true },
 );
+
 watch(maxOutputRun, () => {
 	runOutputIndex.value = -1;
 });
@@ -681,6 +700,7 @@ onMounted(() => {
 
 onBeforeUnmount(() => {
 	dataPinningEventBus.off('data-pinning-discovery', setIsTooltipVisible);
+	unregisterKeyboardListener();
 });
 </script>
 
@@ -719,8 +739,8 @@ onBeforeUnmount(() => {
 			v-if="activeNode"
 			ref="container"
 			class="data-display"
+			data-test-id="ndv-modal"
 			tabindex="0"
-			@keydown.capture="onKeyDown"
 		>
 			<div :class="$style.modalBackground" @click="close"></div>
 			<NDVDraggablePanels