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

Merged
merged 7 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, but I think we will build a base props interface for those components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could! It would be great to harmonize the props every component takes. For instance, only some fields take a readonly prop. We might want to add this behavior to every component.


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',
);
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;
}
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a big fan of this. We will want to valid the field indeed. But we do not want to avoid persisting without letting the user notice. This is why we do not have any validation on links and email yet, we leave the user save whatever he wants.

Next step would be to discuss with Thomas on a common behavior for field content validation. Also maybe not let the workflow being activated if a field is invalid so we know the workflow would fail.

I would:

  • remove that step
  • add a fast follow to discuss validation

Wdyt?

Copy link
Contributor Author

@Devessier Devessier Dec 18, 2024

Choose a reason for hiding this comment

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

I agree that we still need to talk about validation. I already added validation to the Date field. The input doesn't allow you to submit invalid dates.
I can remove the validation step for the Raw JSON field for now.


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 +49 to +57
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" } }');
}),
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 +198 to +203
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;
Devessier marked this conversation as resolved.
Show resolved Hide resolved

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