Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Raw JSON Form Field Input #9078

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { FormFullNameFieldInput } from '@/object-record/record-field/form-types/
import { FormLinksFieldInput } from '@/object-record/record-field/form-types/components/FormLinksFieldInput';
import { FormMultiSelectFieldInput } from '@/object-record/record-field/form-types/components/FormMultiSelectFieldInput';
import { FormNumberFieldInput } from '@/object-record/record-field/form-types/components/FormNumberFieldInput';
import { FormRawJsonFieldInput } from '@/object-record/record-field/form-types/components/FormRawJsonFieldInput';
import { FormSelectFieldInput } from '@/object-record/record-field/form-types/components/FormSelectFieldInput';
import { FormTextFieldInput } from '@/object-record/record-field/form-types/components/FormTextFieldInput';
import { VariablePickerComponent } from '@/object-record/record-field/form-types/types/VariablePickerComponent';
Expand All @@ -26,6 +27,7 @@ import { isFieldFullName } from '@/object-record/record-field/types/guards/isFie
import { isFieldLinks } from '@/object-record/record-field/types/guards/isFieldLinks';
import { isFieldMultiSelect } from '@/object-record/record-field/types/guards/isFieldMultiSelect';
import { isFieldNumber } from '@/object-record/record-field/types/guards/isFieldNumber';
import { isFieldRawJson } from '@/object-record/record-field/types/guards/isFieldRawJson';
import { isFieldSelect } from '@/object-record/record-field/types/guards/isFieldSelect';
import { isFieldText } from '@/object-record/record-field/types/guards/isFieldText';
import { JsonValue } from 'type-fest';
Expand Down Expand Up @@ -118,5 +120,13 @@ export const FormFieldInput = ({
VariablePicker={VariablePicker}
options={field.metadata.options}
/>
) : isFieldRawJson(field) ? (
<FormRawJsonFieldInput
label={field.label}
defaultValue={defaultValue as string | undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Type casting to string | undefined may be insufficient for raw JSON - consider using JsonValue type instead to properly handle nested objects and arrays

onPersist={onPersist}
placeholder={field.label}
VariablePicker={VariablePicker}
/>
) : null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { FormFieldInputContainer } from '@/object-record/record-field/form-types/components/FormFieldInputContainer';
import { FormFieldInputInputContainer } from '@/object-record/record-field/form-types/components/FormFieldInputInputContainer';
import { FormFieldInputRowContainer } from '@/object-record/record-field/form-types/components/FormFieldInputRowContainer';
import { TextVariableEditor } from '@/object-record/record-field/form-types/components/TextVariableEditor';
import { useTextVariableEditor } from '@/object-record/record-field/form-types/hooks/useTextVariableEditor';
import { VariablePickerComponent } from '@/object-record/record-field/form-types/types/VariablePickerComponent';
import { InputLabel } from '@/ui/input/components/InputLabel';
import { CAPTURE_VARIABLE_TAG_REGEX } from '@/workflow/search-variables/utils/initializeEditorContent';
import { useId } from 'react';
import { isDefined } from 'twenty-ui';
import { turnIntoEmptyStringIfWhitespacesOnly } from '~/utils/string/turnIntoEmptyStringIfWhitespacesOnly';

type FormRawJsonFieldInputProps = {
label?: string;
defaultValue: string | null | undefined;
placeholder: string;
onPersist: (value: string | null) => void;
readonly?: boolean;
VariablePicker?: VariablePickerComponent;
};

export const FormRawJsonFieldInput = ({
label,
defaultValue,
placeholder,
onPersist,
readonly,
VariablePicker,
}: FormRawJsonFieldInputProps) => {
const inputId = useId();

const editor = useTextVariableEditor({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: onUpdate callback is recreated on every render. Consider memoizing with useCallback to prevent unnecessary re-renders.

placeholder,
multiline: true,
readonly,
defaultValue: defaultValue ?? undefined,
onUpdate: (editor) => {
const text = turnIntoEmptyStringIfWhitespacesOnly(editor.getText());

if (text === '') {
onPersist(null);

return;
}

const textWithReplacedVariables = text.replaceAll(
new RegExp(CAPTURE_VARIABLE_TAG_REGEX, 'g'),
'0',
);
Comment on lines +46 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Replacing variables with '0' could validate successfully but produce invalid JSON when actual variable values are longer/shorter. Consider using a placeholder that matches expected value length.


let isValidJson;
try {
JSON.parse(textWithReplacedVariables);

isValidJson = true;
} catch {
isValidJson = false;
}
Comment on lines +51 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Empty catch block silently fails JSON validation. Consider providing feedback to user about invalid JSON format.


if (!isValidJson) {
return;
}

onPersist(text);
},
});

const handleVariableTagInsert = (variableName: string) => {
if (!isDefined(editor)) {
throw new Error(
'Expected the editor to be defined when a variable is selected',
);
}

editor.commands.insertVariableTag(variableName);
};
Comment on lines +68 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: handleVariableTagInsert throws if editor undefined, but component renders null in that case. This error should never occur - consider removing the check or logging a warning instead.


if (!isDefined(editor)) {
return null;
}

return (
<FormFieldInputContainer>
{label ? <InputLabel>{label}</InputLabel> : null}

<FormFieldInputRowContainer multiline>
<FormFieldInputInputContainer
hasRightElement={isDefined(VariablePicker)}
multiline
>
<TextVariableEditor editor={editor} multiline readonly={readonly} />
</FormFieldInputInputContainer>

{VariablePicker ? (
<VariablePicker
inputId={inputId}
multiline
onVariableSelect={handleVariableTagInsert}
/>
) : null}
</FormFieldInputRowContainer>
</FormFieldInputContainer>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,246 @@
import { expect } from '@storybook/jest';
import { Meta, StoryObj } from '@storybook/react';
import { fn, userEvent, waitFor, within } from '@storybook/test';
import { FormRawJsonFieldInput } from '../FormRawJsonFieldInput';

const meta: Meta<typeof FormRawJsonFieldInput> = {
title: 'UI/Data/Field/Form/Input/FormRawJsonFieldInput',
component: FormRawJsonFieldInput,
args: {},
argTypes: {},
};

export default meta;

type Story = StoryObj<typeof FormRawJsonFieldInput>;

export const Default: Story = {
args: {
label: 'JSON field',
placeholder: 'Enter valid json',
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);

await canvas.findByText('JSON field');
},
};

export const Readonly: Story = {
args: {
label: 'JSON field',
placeholder: 'Enter valid json',
readonly: true,
onPersist: fn(),
},
play: async ({ canvasElement, args }) => {
const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await userEvent.type(editor, '{{ "a": {{ "b" : "d" } }');

await waitFor(() => {
const allParagraphs = canvasElement.querySelectorAll('.ProseMirror > p');
expect(allParagraphs).toHaveLength(1);
expect(allParagraphs[0]).toHaveTextContent('');
});

expect(args.onPersist).not.toHaveBeenCalled();
},
};

export const SaveValidJson: Story = {
args: {
placeholder: 'Enter valid json',
onPersist: fn(),
},
play: async ({ canvasElement, args }) => {
const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await Promise.all([
userEvent.type(editor, '{{ "a": {{ "b" : "d" } }'),

waitFor(() => {
expect(args.onPersist).toHaveBeenCalledWith('{ "a": { "b" : "d" } }');
}),
Comment on lines +61 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using Promise.all here is unnecessary since there's only one waitFor condition. This could lead to race conditions if more promises are added later.

]);
},
};

export const IgnoreInvalidJson: Story = {
args: {
placeholder: 'Enter valid json',
onPersist: fn(),
},
play: async ({ canvasElement, args }) => {
const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await userEvent.type(editor, 'lol');

await userEvent.click(canvasElement);

expect(args.onPersist).not.toHaveBeenCalled();
},
};

export const DisplayDefaultValueWithVariablesProperly: Story = {
args: {
placeholder: 'Enter valid json',
defaultValue: '{ "a": { "b" : {{var.test}} } }',
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);

await canvas.findByText(/{ "a": { "b" : /);

await waitFor(() => {
const variableTag = canvasElement.querySelector(
'[data-type="variableTag"]',
);

expect(variableTag).toBeVisible();
expect(variableTag).toHaveTextContent('test');
});

await canvas.findByText(/ } }/);
},
};

export const InsertVariableInTheMiddleOnTextInput: Story = {
args: {
placeholder: 'Enter valid json',
VariablePicker: ({ onVariableSelect }) => {
return (
<button
onClick={() => {
onVariableSelect('{{test}}');
}}
>
Add variable
</button>
);
},
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);

const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

const addVariableButton = await canvas.findByRole('button', {
name: 'Add variable',
});

await userEvent.type(editor, '{{ "a": {{ "b" : ');

await userEvent.click(addVariableButton);

await userEvent.type(editor, ' } }');

await Promise.all([
waitFor(() => {
expect(args.onPersist).toHaveBeenCalledWith(
'{ "a": { "b" : {{test}} } }',
);
}),
]);
},
};

export const CanUseVariableAsObjectProperty: Story = {
args: {
placeholder: 'Enter valid json',
VariablePicker: ({ onVariableSelect }) => {
return (
<button
onClick={() => {
onVariableSelect('{{test}}');
}}
>
Add variable
</button>
);
},
},
play: async ({ canvasElement, args }) => {
const canvas = within(canvasElement);

const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

const addVariableButton = await canvas.findByRole('button', {
name: 'Add variable',
});

await userEvent.type(editor, '{{ "');

await userEvent.click(addVariableButton);

await userEvent.type(editor, '": 2 }');

await waitFor(() => {
expect(args.onPersist).toHaveBeenCalledWith('{ "{{test}}": 2 }');
});
},
};

export const ClearField: Story = {
args: {
placeholder: 'Enter valid json',
defaultValue: '{ "a": 2 }',
},
play: async ({ canvasElement, args }) => {
const defaultValueStringLength = args.defaultValue!.length;

const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await Promise.all([
userEvent.type(editor, `{Backspace>${defaultValueStringLength}}`),

waitFor(() => {
expect(args.onPersist).toHaveBeenCalledWith(null);
}),
Comment on lines +200 to +205
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using Promise.all with a single waitFor condition is unnecessary and could mask timing issues. Consider removing Promise.all wrapper.

]);
},
};

/**
* Line breaks are not authorized in JSON strings. Users should instead put newlines characters themselves.
* See https://stackoverflow.com/a/42073.
*/
export const BreaksWhenUserInsertsNewlineInJsonString: Story = {
args: {
placeholder: 'Enter valid json',
onPersist: fn(),
},
play: async ({ canvasElement, args }) => {
const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await userEvent.type(editor, '"a{Enter}b"');

await userEvent.click(canvasElement);

expect(args.onPersist).not.toHaveBeenCalled();
},
};

export const AcceptsJsonEncodedNewline: Story = {
args: {
placeholder: 'Enter valid json',
onPersist: fn(),
},
play: async ({ canvasElement, args }) => {
const editor = canvasElement.querySelector('.ProseMirror > p');
expect(editor).toBeVisible();

await userEvent.type(editor, '"a\\nb"');

await userEvent.click(canvasElement);

expect(args.onPersist).toHaveBeenCalledWith('"a\\nb"');
},
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { isDefined } from 'twenty-ui';

const CAPTURE_ALL_VARIABLE_TAG_INNER_REGEX = /{{([^{}]+)}}/g;
export const CAPTURE_ALL_VARIABLE_TAG_INNER_REGEX = /{{([^{}]+)}}/g;

export const extractVariableLabel = (rawVariableName: string) => {
const variableWithoutBrackets = rawVariableName.replace(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { isNonEmptyString } from '@sniptt/guards';
import { Editor } from '@tiptap/react';

const CAPTURE_VARIABLE_TAG_REGEX = /({{[^{}]+}})/;
export const CAPTURE_VARIABLE_TAG_REGEX = /({{[^{}]+}})/;

export const initializeEditorContent = (editor: Editor, content: string) => {
const lines = content.split(/\n/);
Expand Down
Loading
Loading