Skip to content

Commit

Permalink
move more code and change backslash/quote escaping
Browse files Browse the repository at this point in the history
  • Loading branch information
@CharlieKolb
CharlieKolb committed Feb 4, 2025
1 parent 883d016 commit ee7f39a
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 46 deletions.
30 changes: 15 additions & 15 deletions packages/editor-ui/src/components/ParameterInputFull.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import { N8nInputLabel } from 'n8n-design-system';
import {
buildValueFromOverride,
type FromAIOverride,
isOverrideValue,
isFromAIOverrideValue,
makeOverrideValue,
updateExtraPropValues,
} from './ParameterInputOverrides/fromAIOverrideUtils';
updateFromAIOverrideValues,
} from '../utils/fromAIOverrideUtils';
import { useNodeTypesStore } from '@/stores/nodeTypes.store';
import { useTelemetry } from '@/composables/useTelemetry';
Expand Down Expand Up @@ -73,7 +73,7 @@ const nodeTypesStore = useNodeTypesStore();
const telemetry = useTelemetry();
const node = computed(() => ndvStore.activeNode);
const parameterOverrides = ref<FromAIOverride | null>(
const fromAIOverride = ref<FromAIOverride | null>(
makeOverrideValue(
props,
node.value && nodeTypesStore.getNodeType(node.value.type, node.value.typeVersion),
Expand All @@ -84,11 +84,11 @@ const canBeContentOverride = computed(() => {
// The resourceLocator handles overrides separately
if (!node.value || isResourceLocator.value) return false;
return parameterOverrides.value !== null;
return fromAIOverride.value !== null;
});
const isContentOverride = computed(
() => canBeContentOverride.value && !!isOverrideValue(props.value?.toString() ?? ''),
() => canBeContentOverride.value && !!isFromAIOverrideValue(props.value?.toString() ?? ''),
);
const hint = computed(() => i18n.nodeText().hint(props.parameter, props.path));
Expand All @@ -110,7 +110,7 @@ const showExpressionSelector = computed(() => {
// infer whether it's overridden and we should hide the toggle
const value =
props.value && typeof props.value === 'object' && 'value' in props.value && props.value.value;
if (value && isOverrideValue(String(value))) {
if (value && isFromAIOverrideValue(String(value))) {
return false;
}
Expand Down Expand Up @@ -260,7 +260,7 @@ const isSingleLineInput: ComputedRef<boolean> = computed(
);
function applyOverride() {
if (!parameterOverrides.value) return;
if (!fromAIOverride.value) return;
telemetry.track(
'User turned on fromAI override',
Expand All @@ -270,8 +270,8 @@ function applyOverride() {
},
{ withPostHog: true },
);
updateExtraPropValues(parameterOverrides.value, String(props.value));
const value = buildValueFromOverride(parameterOverrides.value, props, true);
updateFromAIOverrideValues(fromAIOverride.value, String(props.value));
const value = buildValueFromOverride(fromAIOverride.value, props, true);
valueChanged({
node: node.value?.name,
name: props.path,
Expand All @@ -280,7 +280,7 @@ function applyOverride() {
}
function removeOverride(clearField = false) {
if (!parameterOverrides.value) return;
if (!fromAIOverride.value) return;
telemetry.track(
'User turned off fromAI override',
Expand All @@ -295,7 +295,7 @@ function removeOverride(clearField = false) {
name: props.path,
value: clearField
? props.parameter.default
: buildValueFromOverride(parameterOverrides.value, props, false),
: buildValueFromOverride(fromAIOverride.value, props, false),
});
void setTimeout(async () => {
await parameterInputWrapper.value?.focusInput();
Expand Down Expand Up @@ -346,7 +346,7 @@ function removeOverride(clearField = false) {
>
<template #default="{ droppable, activeDrop }">
<FromAiOverrideField
v-if="parameterOverrides && isContentOverride"
v-if="fromAIOverride && isContentOverride"
:is-read-only="isReadOnly"
@close="removeOverride"
/>
Expand Down Expand Up @@ -401,8 +401,8 @@ function removeOverride(clearField = false) {
/>
</div>
<ParameterOverrideSelectableList
v-if="isContentOverride && parameterOverrides"
v-model="parameterOverrides"
v-if="isContentOverride && fromAIOverride"
v-model="fromAIOverride"
:parameter="parameter"
:path="path"
:is-read-only="isReadOnly"
Expand Down
2 changes: 1 addition & 1 deletion ...ages/editor-ui/src/components/ParameterInputOverrides/ParameterOverrideSelectableList.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import type { IUpdateInformation } from '@/Interface';
import { type INodeProperties } from 'n8n-workflow';
import { buildValueFromOverride, type FromAIOverride } from './fromAIOverrideUtils';
import { buildValueFromOverride, type FromAIOverride } from '../../utils/fromAIOverrideUtils';
import { computed } from 'vue';
import { N8nSelectableList } from 'n8n-design-system';
Expand Down
30 changes: 16 additions & 14 deletions packages/editor-ui/src/components/ResourceLocator/ResourceLocator.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ import { useTelemetry } from '@/composables/useTelemetry';
import { onClickOutside, type VueInstance } from '@vueuse/core';
import {
buildValueFromOverride,
isOverrideValue,
isFromAIOverrideValue,
makeOverrideValue,
updateExtraPropValues,
updateFromAIOverrideValues,
type FromAIOverride,
} from '../ParameterInputOverrides/fromAIOverrideUtils';
} from '../../utils/fromAIOverrideUtils';
interface IResourceLocatorQuery {
results: INodeListSearchItems[];
Expand Down Expand Up @@ -289,7 +289,7 @@ const requiresSearchFilter = computed(
() => !!getPropertyArgument(currentMode.value, 'searchFilterRequired'),
);
const parameterOverrides = ref<FromAIOverride | null>(
const fromAIOverride = ref<FromAIOverride | null>(
makeOverrideValue(
{
value: props.modelValue?.value ?? '',
Expand All @@ -302,11 +302,13 @@ const parameterOverrides = ref<FromAIOverride | null>(
const canBeContentOverride = computed(() => {
if (!props.node) return false;
return parameterOverrides.value !== null;
return fromAIOverride.value !== null;
});
const isContentOverride = computed(
() => canBeContentOverride.value && !!isOverrideValue(props.modelValue?.value?.toString() ?? ''),
() =>
canBeContentOverride.value &&
!!isFromAIOverrideValue(props.modelValue?.value?.toString() ?? ''),
);
const showOverrideButton = computed(
Expand Down Expand Up @@ -711,7 +713,7 @@ function onInputBlur(event: FocusEvent) {
}
function applyOverride() {
if (!props.node || !parameterOverrides.value) return;
if (!props.node || !fromAIOverride.value) return;
telemetry.track(
'User turned on fromAI override',
Expand All @@ -721,16 +723,16 @@ function applyOverride() {
},
{ withPostHog: true },
);
updateExtraPropValues(parameterOverrides.value, props.modelValue.value?.toString() ?? '');
updateFromAIOverrideValues(fromAIOverride.value, props.modelValue.value?.toString() ?? '');
emit('update:modelValue', {
...props.modelValue,
value: buildValueFromOverride(parameterOverrides.value, props, true),
value: buildValueFromOverride(fromAIOverride.value, props, true),
});
}
function removeOverride() {
if (!props.node || !parameterOverrides.value) return;
if (!props.node || !fromAIOverride.value) return;
telemetry.track(
'User turned off fromAI override',
Expand All @@ -742,7 +744,7 @@ function removeOverride() {
);
emit('update:modelValue', {
...props.modelValue,
value: buildValueFromOverride(parameterOverrides.value, props, false),
value: buildValueFromOverride(fromAIOverride.value, props, false),
});
void setTimeout(() => {
inputRef.value?.focus();
Expand Down Expand Up @@ -853,7 +855,7 @@ function removeOverride() {
@keydown.stop="onKeyDown"
>
<FromAiOverrideField
v-if="parameterOverrides && isContentOverride"
v-if="fromAIOverride && isContentOverride"
:class="[$style.inputField, $style.fromAiOverrideField]"
:is-read-only="isReadOnly"
@close="removeOverride"
Expand Down Expand Up @@ -921,8 +923,8 @@ function removeOverride() {
</div>
</ResourceLocatorDropdown>
<ParameterOverrideSelectableList
v-if="isContentOverride && parameterOverrides"
v-model="parameterOverrides"
v-if="isContentOverride && fromAIOverride"
v-model="fromAIOverride"
:parameter="parameter"
:path="path"
:is-read-only="isReadOnly"
Expand Down
19 changes: 12 additions & 7 deletions ...nputOverrides/fromAIOverrideUtils.test.ts → ...-ui/src/utils/fromAIOverrideUtils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { FromAIOverride, OverrideContext } from './fromAIOverrideUtils';
import {
buildValueFromOverride,
fromAIExtraProps,
isOverrideValue,
isFromAIOverrideValue,
makeOverrideValue,
parseOverrides,
} from './fromAIOverrideUtils';
Expand Down Expand Up @@ -104,8 +104,10 @@ describe('makeOverrideValue', () => {

describe('FromAiOverride', () => {
it('correctly identifies override values', () => {
expect(isOverrideValue('={{ $fromAI() }}')).toBe(false);
expect(isOverrideValue('={{ /*n8n-auto-generated-fromAI-override*/ $fromAI() }}')).toBe(true);
expect(isFromAIOverrideValue('={{ $fromAI() }}')).toBe(false);
expect(isFromAIOverrideValue('={{ /*n8n-auto-generated-fromAI-override*/ $fromAI() }}')).toBe(
true,
);
});

it('should parseOverrides as expected', () => {
Expand All @@ -129,10 +131,13 @@ describe('FromAiOverride', () => {
});

test.each<[string, string, string]>([
['normal case', '$fromAI("a", `b`)', 'b'],
['working', '$fromAI("a", `a \\` \\\\\\``)', 'a ` \\`'],
// ['failing', '$fromAI("a", `\\``)', '`'],
])('should handle backtick escaping %s', (_name, value, expected) => {
['none', '$fromAI("a", `b`)', 'b'],
['a simple case of', '$fromAI("a", `\\``)', '`'],
// this is a bug in the current implementation
// see related comments in the main file
// We try to use different quote characters where possible
['a complex case of', '$fromAI("a", `a \\` \\\\\\``)', 'a ` `'],
])('should handle %s backtick escaping ', (_name, value, expected) => {
expect(parseOverrides(value)).toEqual({ description: expected });
});

Expand Down
35 changes: 26 additions & 9 deletions ...eterInputOverrides/fromAIOverrideUtils.ts → ...ditor-ui/src/utils/fromAIOverrideUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function isExtraPropKey(
return extraProps.hasOwnProperty(key);
}

export function updateExtraPropValues(override: FromAIOverride, expr: string) {
export function updateFromAIOverrideValues(override: FromAIOverride, expr: string) {
const { extraProps, extraPropValues } = override;
const overrides = parseOverrides(expr);
if (overrides) {
Expand All @@ -96,10 +96,18 @@ function fieldTypeToFromAiType(propType: NodePropertyTypes) {
}
}

export function isOverrideValue(s: string) {
export function isFromAIOverrideValue(s: string) {
return s.startsWith(`={{ ${FROM_AI_AUTO_GENERATED_MARKER} $fromAI(`);
}

function getBestQuoteChar(description: string) {
if (description.includes('\n')) return '`';

if (!description.includes('`')) return '`';
if (!description.includes('"')) return '"';
return "'";
}

export function buildValueFromOverride(
override: FromAIOverride,
props: Pick<OverrideContext, 'parameter'>,
Expand All @@ -110,24 +118,33 @@ export function buildValueFromOverride(
const key = sanitizeFromAiParameterName(props.parameter.displayName);
const description =
extraPropValues?.description?.toString() ?? extraProps.description.initialValue;

// We want to escape these characters here as the generated formula needs to be valid JS code, and we risk
// closing the string prematurely without it.
// If we don't escape \ then 'my \` description' as user input would end up as 'my \\` description'
// in the generated code, causing an unescaped backtick to close the string.
const sanitizedDescription = description.replace(/\\/g, '\\\\').replace(/`/g, '\\`');
// If we don't escape \ then <my \' description> as would end up as 'my \\' description' in
// the generated code, causing an unescaped quoteChar to close the string.
// We try to minimize this by looking for an unused quote char first
const quoteChar = getBestQuoteChar(description);
const sanitizedDescription = description
.replaceAll(/\\/g, '\\\\')
.replaceAll(quoteChar, `\\${quoteChar}`);
const type = fieldTypeToFromAiType(props.parameter.type);

return `={{ ${marker}$fromAI('${key}', \`${sanitizedDescription}\`, '${type}') }}`;
return `={{ ${marker}$fromAI('${key}', ${quoteChar}${sanitizedDescription}${quoteChar}, '${type}') }}`;
}

export function parseOverrides(
s: string,
expression: string,
): Record<keyof FromAIOverride['extraProps'], string | undefined> | null {
try {
// `extractFromAICalls` has different escape semantics from JS strings
// Specifically it makes \ escape any following character.
// So we need to escape our \ so we don't drop them accidentally
const calls = extractFromAICalls(s.replaceAll(/\\/g, '\\\\'));
// However ` used in the description cause \` to appear here, which would break
// So we take the hit and expose the bug for backticks only, turning \` into `.
const preparedExpression = expression.replace(/\\[^`]/g, '\\\\');

const calls = extractFromAICalls(preparedExpression);
if (calls.length === 1) {
return {
description: calls[0].description,
Expand Down Expand Up @@ -165,7 +182,7 @@ export function makeOverrideValue(
extraProps: fromAIExtraProps,
extraPropValues: {},
};
updateExtraPropValues(fromAiOverride, context.value?.toString() ?? '');
updateFromAIOverrideValues(fromAiOverride, context.value?.toString() ?? '');
return fromAiOverride;
}

Expand Down

0 comments on commit ee7f39a

Please sign in to comment.