-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Save Markdown and BlockNote in RICH_TEXT fields (#7613) #9279
base: main
Are you sure you want to change the base?
Save Markdown and BlockNote in RICH_TEXT fields (#7613) #9279
Conversation
TODOs/FIXMEs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR transforms RICH_TEXT fields into composite fields that store both BlockNote and Markdown formats, enabling dual-format storage and format choice during CSV imports.
- Adds new
rich-text.composite-type.ts
defining the composite structure with nullableblocknote
andmarkdown
TEXT fields - Modifies search functionality in task/note entities to use
bodyBlocknote
instead ofbody
, with a TODO to verify search behavior - Moves RICH_TEXT handling from
basicColumnActionFactory
tocompositeColumnActionFactory
for database migrations - Updates OpenAPI schema and Zapier integration to handle RICH_TEXT as a composite field with both formats
- Needs proper example values in
SettingsCompositeFieldTypeConfigs.ts
(currently marked as TODO)
22 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
export const richTextSchema: z.ZodType<FieldRichTextValue> = z.object({ | ||
blocknote: z.string().nullable(), | ||
markdown: z.string().nullable(), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider adding validation to ensure at least one of blocknote or markdown is non-null to prevent empty objects
blocknote: 'TODO', // TODO | ||
markdown: 'TODO', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: example values are marked as TODO and need to be properly defined with realistic sample data
label: 'Rich Text', | ||
Icon: IllustrationIconText, | ||
subFields: ['blocknote', 'markdown'], | ||
filterableSubFields: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider adding 'markdown' to filterableSubFields to enable searching/filtering by content
@@ -21,7 +21,7 @@ export class ActivityQueryResultGetterHandler | |||
return activity; | |||
} | |||
|
|||
const body: RichTextBody = JSON.parse(activity.body); | |||
const body: RichTextBody = JSON.parse(activity.body.blocknote); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: add try/catch around JSON.parse to handle invalid JSON gracefully
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: missing return type for undefined case - could cause runtime issues if field type is not handled
export type RichTextMetadata = { | ||
blocknote: string; | ||
markdown: string; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: both fields are defined as required in the type but marked as not required in the composite type definition above. This inconsistency could cause runtime issues
export class FieldMetadataDefaultValueRichText { | ||
@ValidateIf((_object, value) => value !== null) | ||
@IsString() | ||
value: string | null; | ||
@ValidateIf((object, value) => value !== null) | ||
@IsQuotedString() | ||
blocknote: string | null; | ||
|
||
@ValidateIf((object, value) => value !== null) | ||
@IsQuotedString() | ||
markdown: string | null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Both blocknote and markdown fields can be null simultaneously. Consider adding validation to ensure at least one format is provided.
@@ -37,7 +38,7 @@ const BODY_FIELD_NAME = 'body'; | |||
|
|||
export const SEARCH_FIELDS_FOR_TASK: FieldTypeAndNameMetadata[] = [ | |||
{ name: TITLE_FIELD_NAME, type: FieldMetadataType.TEXT }, | |||
{ name: BODY_FIELD_NAME, type: FieldMetadataType.RICH_TEXT }, | |||
{ name: `${BODY_FIELD_NAME}Blocknote`, type: FieldMetadataType.RICH_TEXT }, // TODO: Check later if and how this works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: search field using 'bodyBlocknote' may break full-text search for existing tasks since the field name changed from 'body' to 'bodyBlocknote'
@@ -35,7 +36,7 @@ const BODY_FIELD_NAME = 'body'; | |||
|
|||
export const SEARCH_FIELDS_FOR_NOTES: FieldTypeAndNameMetadata[] = [ | |||
{ name: TITLE_FIELD_NAME, type: FieldMetadataType.TEXT }, | |||
{ name: BODY_FIELD_NAME, type: FieldMetadataType.RICH_TEXT }, | |||
{ name: `${BODY_FIELD_NAME}Blocknote`, type: FieldMetadataType.RICH_TEXT }, // TODO: Check later if and how this works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: search field name change from 'body' to 'bodyBlocknote' may break existing search functionality if not properly migrated
name: 'blocknote', | ||
label: 'Blocknote', | ||
description: 'Blocknote', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: inconsistent casing between 'blocknote' and 'BlockNote' in label/description
@eliasylonen Could you link the original issue ? |
@lucasbordeau I edited the description to add the related issues. I think we should create a separate issue for the input in table views as this PR is going to be quite big already (it's still wip). Would you mind creating it if you can point to the right direction? I'm not sure what has been done/is left to be done |
Could we use deprecated instead of old ? |
Shouldn't we add the type RICH_TEXT next to the RICH_TEXT_OLD/DEPRECATED ? |
@lucasbordeau Yes 👍 will do! |
RICH_TEXT
field composite: instead of saving only BlockNote also save a lossy copy of the value in Markdown format.Fixes #7613
Fixes #7444