Add Raw JSON Form Field Input #9078
Conversation
Devessier
force-pushed
the
add-uuid-form-field-input
branch
2 times, most recently
from
480eb2e to
34e959b
Compare
…ngine will do on the backend Users have to put double quotes where they want to create strings.
Devessier
force-pushed
the
add-uuid-form-field-input
branch
from
34e959b to
90e7dc2
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implemented a new FormRawJsonFieldInput component for handling JSON validation and variable tag insertion in form fields, with support for variable tags in JSON content.
- Added
FormRawJsonFieldInput.tsxwith JSON validation that replaces variable tags with '0' for validation and converts empty strings to null - Added comprehensive Storybook tests in
FormRawJsonFieldInput.stories.tsxcovering JSON validation, variable insertion, and edge cases - Added
renderTextmethod tovariableTag.tsto return raw variable attributes (e.g., '{{test}}') - Exported
CAPTURE_VARIABLE_TAG_REGEXandCAPTURE_ALL_VARIABLE_TAG_INNER_REGEXconstants for reuse in variable tag parsing - Modified
FormFieldInput.tsxto support the new RawJson field type
6 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| ) : isFieldRawJson(field) ? ( | ||
| <FormRawJsonFieldInput | ||
| label={field.label} | ||
| defaultValue={defaultValue as string | undefined} |
There was a problem hiding this comment.
logic: Type casting to string | undefined may be insufficient for raw JSON - consider using JsonValue type instead to properly handle nested objects and arrays
| await Promise.all([ | ||
| userEvent.type(editor, '{{ "a": {{ "b" : "d" } }'), | ||
|
|
||
| waitFor(() => { | ||
| expect(args.onPersist).toHaveBeenCalledWith('{ "a": { "b" : "d" } }'); | ||
| }), |
There was a problem hiding this comment.
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.
| await Promise.all([ | ||
| userEvent.type(editor, `{Backspace>${defaultValueStringLength}}`), | ||
|
|
||
| waitFor(() => { | ||
| expect(args.onPersist).toHaveBeenCalledWith(null); | ||
| }), |
There was a problem hiding this comment.
logic: Using Promise.all with a single waitFor condition is unnecessary and could mask timing issues. Consider removing Promise.all wrapper.
| let isValidJson; | ||
| try { | ||
| JSON.parse(textWithReplacedVariables); | ||
|
|
||
| isValidJson = true; | ||
| } catch { | ||
| isValidJson = false; | ||
| } |
There was a problem hiding this comment.
logic: Empty catch block silently fails JSON validation. Consider providing feedback to user about invalid JSON format.
| const textWithReplacedVariables = text.replaceAll( | ||
| new RegExp(CAPTURE_VARIABLE_TAG_REGEX, 'g'), | ||
| '0', | ||
| ); |
There was a problem hiding this comment.
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.
| 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); | ||
| }; |
There was a problem hiding this comment.
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.
| }: FormRawJsonFieldInputProps) => { | ||
| const inputId = useId(); | ||
|
|
||
| const editor = useTextVariableEditor({ |
There was a problem hiding this comment.
style: onUpdate callback is recreated on every render. Consider memoizing with useCallback to prevent unnecessary re-renders.
| onPersist: (value: string | null) => void; | ||
| readonly?: boolean; | ||
| VariablePicker?: VariablePickerComponent; | ||
| }; |
There was a problem hiding this comment.
not for this PR, but I think we will build a base props interface for those components
There was a problem hiding this comment.
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.
packages/twenty-front/src/modules/workflow/search-variables/utils/extractVariableLabel.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if (!isValidJson) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Thanks @Devessier for your contribution! |
renderTextmethod forVariableTag. This method returns thevariableattribute of the node, i.e.{{test}}.editor.getText()function to simply get the textual representation of the field without relying oneditor.getJSON()andparseEditorContent.TextVariableEditorcomponent.RawJsonFieldInputfield, especially the JSON validation.Closes https://github.com/twentyhq/private-issues/issues/180