From 8788e2a35bed261e13da5c92ee31bbb414d019a4 Mon Sep 17 00:00:00 2001
From: Elias Meire <elias@meire.dev>
Date: Fri, 29 Mar 2024 16:01:32 +0100
Subject: [PATCH] fix(editor): UI enhancements and fixes for expression inputs
 (#8996)

---
 packages/editor-ui/src/__tests__/setup.ts     |  7 ++++++
 .../AssignmentCollection/Assignment.vue       |  2 --
 .../components/FilterConditions/Condition.vue |  2 --
 .../InlineExpressionEditorOutput.vue          | 25 +++++++++++--------
 .../src/components/ParameterInputFull.vue     |  7 +++++-
 .../ExpressionEditorModalInput.test.ts        | 15 -----------
 .../components/__tests__/HtmlEditor.test.ts   | 17 -------------
 .../useAutocompleteTelemetry.test.ts          | 17 -------------
 .../__tests__/useExpressionEditor.test.ts     | 17 -------------
 .../src/composables/useExpressionEditor.ts    |  5 ++--
 .../src/Extensions/StringExtensions.ts        |  5 +++-
 .../StringExtensions.test.ts                  |  8 +++++-
 12 files changed, 41 insertions(+), 86 deletions(-)

diff --git a/packages/editor-ui/src/__tests__/setup.ts b/packages/editor-ui/src/__tests__/setup.ts
index 9316e30a30..4c75c226e6 100644
--- a/packages/editor-ui/src/__tests__/setup.ts
+++ b/packages/editor-ui/src/__tests__/setup.ts
@@ -12,3 +12,10 @@ window.ResizeObserver =
 	}));
 
 Element.prototype.scrollIntoView = vi.fn();
+
+Range.prototype.getBoundingClientRect = vi.fn();
+Range.prototype.getClientRects = vi.fn(() => ({
+	item: vi.fn(),
+	length: 0,
+	[Symbol.iterator]: vi.fn(),
+}));
diff --git a/packages/editor-ui/src/components/AssignmentCollection/Assignment.vue b/packages/editor-ui/src/components/AssignmentCollection/Assignment.vue
index c1ee16976b..f18b5d5c81 100644
--- a/packages/editor-ui/src/components/AssignmentCollection/Assignment.vue
+++ b/packages/editor-ui/src/components/AssignmentCollection/Assignment.vue
@@ -169,7 +169,6 @@ const onBlur = (): void => {
 						display-options
 						hide-label
 						hide-hint
-						:rows="3"
 						:is-read-only="isReadOnly"
 						:parameter="nameParameter"
 						:value="assignment.name"
@@ -196,7 +195,6 @@ const onBlur = (): void => {
 							hide-label
 							hide-issues
 							hide-hint
-							:rows="3"
 							is-assignment
 							:is-read-only="isReadOnly"
 							:options-position="breakpoint === 'default' ? 'top' : 'bottom'"
diff --git a/packages/editor-ui/src/components/FilterConditions/Condition.vue b/packages/editor-ui/src/components/FilterConditions/Condition.vue
index 18eb28b034..e3b1be0e18 100644
--- a/packages/editor-ui/src/components/FilterConditions/Condition.vue
+++ b/packages/editor-ui/src/components/FilterConditions/Condition.vue
@@ -156,7 +156,6 @@ const onBlur = (): void => {
 					hide-label
 					hide-hint
 					hide-issues
-					:rows="3"
 					:is-read-only="readOnly"
 					:parameter="leftParameter"
 					:value="condition.leftValue"
@@ -181,7 +180,6 @@ const onBlur = (): void => {
 					hide-label
 					hide-hint
 					hide-issues
-					:rows="3"
 					:is-read-only="readOnly"
 					:options-position="breakpoint === 'default' ? 'top' : 'bottom'"
 					:parameter="rightParameter"
diff --git a/packages/editor-ui/src/components/InlineExpressionEditor/InlineExpressionEditorOutput.vue b/packages/editor-ui/src/components/InlineExpressionEditor/InlineExpressionEditorOutput.vue
index 65102dabbe..4c2bda3195 100644
--- a/packages/editor-ui/src/components/InlineExpressionEditor/InlineExpressionEditorOutput.vue
+++ b/packages/editor-ui/src/components/InlineExpressionEditor/InlineExpressionEditorOutput.vue
@@ -45,21 +45,24 @@ const resolvedExpression = computed(() => {
 });
 
 const plaintextSegments = computed<Plaintext[]>(() => {
-	if (props.segments.length === 0) {
-		return [
-			{
-				from: 0,
-				to: resolvedExpression.value.length - 1,
-				plaintext: resolvedExpression.value,
-				kind: 'plaintext',
-			},
-		];
-	}
-
 	return props.segments.filter((s): s is Plaintext => s.kind === 'plaintext');
 });
 
 const resolvedSegments = computed<Resolved[]>(() => {
+	if (props.segments.length === 0) {
+		const emptyExpression = resolvedExpression.value;
+		const emptySegment: Resolved = {
+			from: 0,
+			to: emptyExpression.length,
+			kind: 'resolvable',
+			error: null,
+			resolvable: '',
+			resolved: emptyExpression,
+			state: 'pending',
+		};
+		return [emptySegment];
+	}
+
 	let cursor = 0;
 
 	return props.segments
diff --git a/packages/editor-ui/src/components/ParameterInputFull.vue b/packages/editor-ui/src/components/ParameterInputFull.vue
index ceda6d05d7..20c7a81d5d 100644
--- a/packages/editor-ui/src/components/ParameterInputFull.vue
+++ b/packages/editor-ui/src/components/ParameterInputFull.vue
@@ -1,6 +1,6 @@
 <template>
 	<n8n-input-label
-		:class="$style.wrapper"
+		:class="[$style.wrapper, { [$style.tipVisible]: showDragnDropTip }]"
 		:label="hideLabel ? '' : i18n.nodeText().inputLabelDisplayName(parameter, path)"
 		:tooltip-text="hideLabel ? '' : i18n.nodeText().inputLabelDescription(parameter, path)"
 		:show-tooltip="focused"
@@ -357,6 +357,11 @@ export default defineComponent({
 	}
 }
 
+.tipVisible {
+	--input-border-bottom-left-radius: 0;
+	--input-border-bottom-right-radius: 0;
+}
+
 .tip {
 	position: absolute;
 	z-index: 2;
diff --git a/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts b/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts
index c285f62c9d..0aa1e7d0fe 100644
--- a/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts
+++ b/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts
@@ -6,8 +6,6 @@ import { setActivePinia } from 'pinia';
 import { waitFor } from '@testing-library/vue';
 
 describe('ExpressionParameterInput', () => {
-	const originalRangeGetBoundingClientRect = Range.prototype.getBoundingClientRect;
-	const originalRangeGetClientRects = Range.prototype.getClientRects;
 	const renderComponent = createComponentRenderer(ExpressionEditorModalInput);
 	let pinia: TestingPinia;
 
@@ -16,19 +14,6 @@ describe('ExpressionParameterInput', () => {
 		setActivePinia(pinia);
 	});
 
-	beforeAll(() => {
-		Range.prototype.getBoundingClientRect = vi.fn();
-		Range.prototype.getClientRects = () => ({
-			item: vi.fn(),
-			length: 0,
-			[Symbol.iterator]: vi.fn(),
-		});
-	});
-
-	afterAll(() => {
-		Range.prototype.getBoundingClientRect = originalRangeGetBoundingClientRect;
-		Range.prototype.getClientRects = originalRangeGetClientRects;
-	});
 	test.each([
 		['not be editable', 'readonly', true, ''],
 		['be editable', 'not readonly', false, 'test'],
diff --git a/packages/editor-ui/src/components/__tests__/HtmlEditor.test.ts b/packages/editor-ui/src/components/__tests__/HtmlEditor.test.ts
index f7eadcbc63..7d5f80d428 100644
--- a/packages/editor-ui/src/components/__tests__/HtmlEditor.test.ts
+++ b/packages/editor-ui/src/components/__tests__/HtmlEditor.test.ts
@@ -17,9 +17,6 @@ const DEFAULT_SETUP = {
 };
 
 describe('HtmlEditor.vue', () => {
-	const originalRangeGetBoundingClientRect = Range.prototype.getBoundingClientRect;
-	const originalRangeGetClientRects = Range.prototype.getClientRects;
-
 	const pinia = createTestingPinia({
 		initialState: {
 			[STORES.SETTINGS]: {
@@ -29,20 +26,6 @@ describe('HtmlEditor.vue', () => {
 	});
 	setActivePinia(pinia);
 
-	beforeAll(() => {
-		Range.prototype.getBoundingClientRect = vi.fn();
-		Range.prototype.getClientRects = () => ({
-			item: vi.fn(),
-			length: 0,
-			[Symbol.iterator]: vi.fn(),
-		});
-	});
-
-	afterAll(() => {
-		Range.prototype.getBoundingClientRect = originalRangeGetBoundingClientRect;
-		Range.prototype.getClientRects = originalRangeGetClientRects;
-	});
-
 	afterAll(() => {
 		vi.clearAllMocks();
 	});
diff --git a/packages/editor-ui/src/composables/__tests__/useAutocompleteTelemetry.test.ts b/packages/editor-ui/src/composables/__tests__/useAutocompleteTelemetry.test.ts
index 54c105d124..13df44088d 100644
--- a/packages/editor-ui/src/composables/__tests__/useAutocompleteTelemetry.test.ts
+++ b/packages/editor-ui/src/composables/__tests__/useAutocompleteTelemetry.test.ts
@@ -28,9 +28,6 @@ vi.mock('@/stores/n8nRoot.store', () => ({
 }));
 
 describe('useAutocompleteTelemetry', () => {
-	const originalRangeGetBoundingClientRect = Range.prototype.getBoundingClientRect;
-	const originalRangeGetClientRects = Range.prototype.getClientRects;
-
 	beforeEach(() => {
 		setActivePinia(createTestingPinia());
 	});
@@ -39,20 +36,6 @@ describe('useAutocompleteTelemetry', () => {
 		vi.clearAllMocks();
 	});
 
-	beforeAll(() => {
-		Range.prototype.getBoundingClientRect = vi.fn();
-		Range.prototype.getClientRects = () => ({
-			item: vi.fn(),
-			length: 0,
-			[Symbol.iterator]: vi.fn(),
-		});
-	});
-
-	afterAll(() => {
-		Range.prototype.getBoundingClientRect = originalRangeGetBoundingClientRect;
-		Range.prototype.getClientRects = originalRangeGetClientRects;
-	});
-
 	const getEditor = (defaultDoc = '') => {
 		const extensionCompartment = new Compartment();
 		const state = EditorState.create({
diff --git a/packages/editor-ui/src/composables/__tests__/useExpressionEditor.test.ts b/packages/editor-ui/src/composables/__tests__/useExpressionEditor.test.ts
index dac86b8ca8..916b809525 100644
--- a/packages/editor-ui/src/composables/__tests__/useExpressionEditor.test.ts
+++ b/packages/editor-ui/src/composables/__tests__/useExpressionEditor.test.ts
@@ -21,9 +21,6 @@ vi.mock('@/stores/ndv.store', () => ({
 }));
 
 describe('useExpressionEditor', () => {
-	const originalRangeGetBoundingClientRect = Range.prototype.getBoundingClientRect;
-	const originalRangeGetClientRects = Range.prototype.getClientRects;
-
 	const mockResolveExpression = () => {
 		const mock = vi.fn();
 		vi.spyOn(workflowHelpers, 'useWorkflowHelpers').mockReturnValueOnce({
@@ -42,20 +39,6 @@ describe('useExpressionEditor', () => {
 		vi.clearAllMocks();
 	});
 
-	beforeAll(() => {
-		Range.prototype.getBoundingClientRect = vi.fn();
-		Range.prototype.getClientRects = () => ({
-			item: vi.fn(),
-			length: 0,
-			[Symbol.iterator]: vi.fn(),
-		});
-	});
-
-	afterAll(() => {
-		Range.prototype.getBoundingClientRect = originalRangeGetBoundingClientRect;
-		Range.prototype.getClientRects = originalRangeGetClientRects;
-	});
-
 	test('should create an editor', async () => {
 		const root = ref<HTMLElement>();
 		const { editor } = useExpressionEditor({
diff --git a/packages/editor-ui/src/composables/useExpressionEditor.ts b/packages/editor-ui/src/composables/useExpressionEditor.ts
index 029a18689a..a4a423f512 100644
--- a/packages/editor-ui/src/composables/useExpressionEditor.ts
+++ b/packages/editor-ui/src/composables/useExpressionEditor.ts
@@ -185,7 +185,7 @@ export const useExpressionEditor = ({
 		if (editor.value) {
 			editor.value.destroy();
 		}
-		editor.value = new EditorView({ parent, state });
+		editor.value = new EditorView({ parent, state, scrollTo: EditorView.scrollIntoView(0) });
 		debouncedUpdateSegments();
 	});
 
@@ -385,7 +385,8 @@ export const useExpressionEditor = ({
 	function setCursorPosition(pos: number | 'lastExpression' | 'end'): void {
 		if (pos === 'lastExpression') {
 			const END_OF_EXPRESSION = ' }}';
-			pos = Math.max(readEditorValue().lastIndexOf(END_OF_EXPRESSION), 0);
+			const endOfLastExpression = readEditorValue().lastIndexOf(END_OF_EXPRESSION);
+			pos = endOfLastExpression !== -1 ? endOfLastExpression : editor.value?.state.doc.length ?? 0;
 		} else if (pos === 'end') {
 			pos = editor.value?.state.doc.length ?? 0;
 		}
diff --git a/packages/workflow/src/Extensions/StringExtensions.ts b/packages/workflow/src/Extensions/StringExtensions.ts
index 88728fc400..15b0f32f27 100644
--- a/packages/workflow/src/Extensions/StringExtensions.ts
+++ b/packages/workflow/src/Extensions/StringExtensions.ts
@@ -382,7 +382,10 @@ function parseJson(value: string): unknown {
 	try {
 		return JSON.parse(value);
 	} catch (error) {
-		return undefined;
+		if (value.includes("'")) {
+			throw new ExpressionExtensionError("Parsing failed. Check you're using double quotes");
+		}
+		throw new ExpressionExtensionError('Parsing failed');
 	}
 }
 
diff --git a/packages/workflow/test/ExpressionExtensions/StringExtensions.test.ts b/packages/workflow/test/ExpressionExtensions/StringExtensions.test.ts
index cdff80759e..7f437953c0 100644
--- a/packages/workflow/test/ExpressionExtensions/StringExtensions.test.ts
+++ b/packages/workflow/test/ExpressionExtensions/StringExtensions.test.ts
@@ -270,7 +270,13 @@ describe('Data Transformation Functions', () => {
 				test1: 1,
 				test2: '2',
 			});
-			expect(evaluate('={{ "hi".parseJson() }}')).toBeUndefined();
+		});
+
+		test('.parseJson should throw on invalid JSON', () => {
+			expect(() => evaluate("={{ \"{'test1':1,'test2':'2'}\".parseJson() }}")).toThrowError(
+				"Parsing failed. Check you're using double quotes",
+			);
+			expect(() => evaluate('={{ "No JSON here".parseJson() }}')).toThrowError('Parsing failed');
 		});
 
 		test('.toBoolean should work on a string', () => {