-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[WEB-2730] chore: core/editor updates to support mobile editor #5910
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
Changes from all commits
42db46c
b38f0ec
dfc5b1f
3a6733c
4a517ed
3206c97
d909153
560b8a3
db05603
0a5d690
f27f594
8eb9f03
0776169
3bd29e8
c76228b
f36841c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,24 +8,31 @@ import { SideMenuExtension, SlashCommands } from "@/extensions"; | |||||||||
| import { EditorRefApi, IRichTextEditor } from "@/types"; | ||||||||||
|
|
||||||||||
| const RichTextEditor = (props: IRichTextEditor) => { | ||||||||||
| const { dragDropEnabled } = props; | ||||||||||
| const { | ||||||||||
| disabledExtensions, | ||||||||||
| dragDropEnabled, | ||||||||||
| bubbleMenuEnabled = true, | ||||||||||
| extensions: externalExtensions = [], | ||||||||||
| } = props; | ||||||||||
|
|
||||||||||
| const getExtensions = useCallback(() => { | ||||||||||
| const extensions = [SlashCommands()]; | ||||||||||
|
|
||||||||||
| extensions.push( | ||||||||||
| const extensions = [ | ||||||||||
| ...externalExtensions, | ||||||||||
| SideMenuExtension({ | ||||||||||
| aiEnabled: false, | ||||||||||
| dragDropEnabled: !!dragDropEnabled, | ||||||||||
| }) | ||||||||||
| ); | ||||||||||
| }), | ||||||||||
| ]; | ||||||||||
| if (!disabledExtensions?.includes("slash-commands")) { | ||||||||||
| extensions.push(SlashCommands()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return extensions; | ||||||||||
| }, [dragDropEnabled]); | ||||||||||
| }, [dragDropEnabled, disabledExtensions, externalExtensions]); | ||||||||||
|
|
||||||||||
| return ( | ||||||||||
| <EditorWrapper {...props} extensions={getExtensions()}> | ||||||||||
| {(editor) => <>{editor && <EditorBubbleMenu editor={editor} />}</>} | ||||||||||
| {(editor) => <>{editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />}</>} | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify conditional rendering The nested ternary with multiple conditions can be simplified. Consider this more readable alternative: -{(editor) => <>{editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />}</>}
+{(editor) => (
+ bubbleMenuEnabled && editor ? <EditorBubbleMenu editor={editor} /> : null
+)}📝 Committable suggestion
Suggested change
|
||||||||||
| </EditorWrapper> | ||||||||||
|
Comment on lines
34
to
36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Based on the gathered information, I can now provide a final response: Type safety concerns identified in EditorBubbleMenu The component has a potential type safety issue:
🔗 Analysis chainVerify editor prop type safety While the conditional rendering works, let's ensure the Let me gather more information about the type definitions and usage to ensure type safety. Let me check one more thing to understand how the editor prop is typed in the @tiptap/react package. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check EditorBubbleMenu component's prop types
rg -A 5 'EditorBubbleMenu.*Props|interface.*EditorBubbleMenu' packages/editor/
Length of output: 1568 Script: #!/bin/bash
# Check BubbleMenuProps type definition and editor prop usage
rg -A 5 'BubbleMenuProps|type.*BubbleMenu' packages/editor/
# Check EditorWrapper component's type definitions
ast-grep --pattern 'interface EditorWrapperProps {
$$$
}'
Length of output: 2158 Script: #!/bin/bash
# Check EditorWrapper implementation and type usage
ast-grep --pattern 'const EditorWrapper = ({ $$$'
# Check if there are any type errors related to editor prop
rg -B2 -A2 'editor: ' packages/editor/src/core/components/editors/rich-text/
Length of output: 130 |
||||||||||
| ); | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,23 +3,24 @@ import { Editor, NodeViewProps, NodeViewWrapper } from "@tiptap/react"; | |||||||||||||||||||||||
| // extensions | ||||||||||||||||||||||||
| import { CustomImageBlock, CustomImageUploader, ImageAttributes } from "@/extensions/custom-image"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export type CustomImageComponentProps = { | ||||||||||||||||||||||||
| export type CustoBaseImageNodeViewProps = { | ||||||||||||||||||||||||
| getPos: () => number; | ||||||||||||||||||||||||
| editor: Editor; | ||||||||||||||||||||||||
| node: NodeViewProps["node"] & { | ||||||||||||||||||||||||
| attrs: ImageAttributes; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| updateAttributes: (attrs: ImageAttributes) => void; | ||||||||||||||||||||||||
| updateAttributes: (attrs: Partial<ImageAttributes>) => void; | ||||||||||||||||||||||||
| selected: boolean; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export type CustomImageNodeViewProps = NodeViewProps & CustomImageComponentProps; | ||||||||||||||||||||||||
| export type CustomImageNodeProps = NodeViewProps & CustoBaseImageNodeViewProps; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export const CustomImageNode = (props: CustomImageNodeViewProps) => { | ||||||||||||||||||||||||
| export const CustomImageNode = (props: CustomImageNodeProps) => { | ||||||||||||||||||||||||
| const { getPos, editor, node, updateAttributes, selected } = props; | ||||||||||||||||||||||||
| const { src: remoteImageSrc } = node.attrs; | ||||||||||||||||||||||||
| const { src: imgNodeSrc } = node.attrs; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const [isUploaded, setIsUploaded] = useState(false); | ||||||||||||||||||||||||
| const [resolvedSrc, setResolvedSrc] = useState<string | undefined>(undefined); | ||||||||||||||||||||||||
| const [imageFromFileSystem, setImageFromFileSystem] = useState<string | undefined>(undefined); | ||||||||||||||||||||||||
| const [failedToLoadImage, setFailedToLoadImage] = useState(false); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -39,13 +40,22 @@ export const CustomImageNode = (props: CustomImageNodeViewProps) => { | |||||||||||||||||||||||
| // the image is already uploaded if the image-component node has src attribute | ||||||||||||||||||||||||
| // and we need to remove the blob from our file system | ||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||
| if (remoteImageSrc) { | ||||||||||||||||||||||||
| if (resolvedSrc) { | ||||||||||||||||||||||||
| setIsUploaded(true); | ||||||||||||||||||||||||
| setImageFromFileSystem(undefined); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| setIsUploaded(false); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }, [remoteImageSrc]); | ||||||||||||||||||||||||
| }, [resolvedSrc]); | ||||||||||||||||||||||||
|
Comment on lines
+43
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider simplifying the upload status logic The upload status logic can be more concise by directly using the boolean expression. -if (resolvedSrc) {
- setIsUploaded(true);
- setImageFromFileSystem(undefined);
-} else {
- setIsUploaded(false);
-}
+setIsUploaded(Boolean(resolvedSrc));
+if (resolvedSrc) setImageFromFileSystem(undefined);📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||
| const getImageSource = async () => { | ||||||||||||||||||||||||
| // @ts-expect-error function not expected here, but will still work and don't remove await | ||||||||||||||||||||||||
| const url: string = await editor?.commands?.getImageSource?.(imgNodeSrc); | ||||||||||||||||||||||||
| setResolvedSrc(url as string); | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| getImageSource(); | ||||||||||||||||||||||||
| }, [imageFromFileSystem, node.attrs.src]); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||
| <NodeViewWrapper> | ||||||||||||||||||||||||
|
|
@@ -55,8 +65,7 @@ export const CustomImageNode = (props: CustomImageNodeViewProps) => { | |||||||||||||||||||||||
| imageFromFileSystem={imageFromFileSystem} | ||||||||||||||||||||||||
| editorContainer={editorContainer} | ||||||||||||||||||||||||
| editor={editor} | ||||||||||||||||||||||||
| // @ts-expect-error function not expected here, but will still work | ||||||||||||||||||||||||
| src={editor?.commands?.getImageSource?.(remoteImageSrc)} | ||||||||||||||||||||||||
| src={resolvedSrc} | ||||||||||||||||||||||||
| getPos={getPos} | ||||||||||||||||||||||||
| node={node} | ||||||||||||||||||||||||
| setEditorContainer={setEditorContainer} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,8 +24,8 @@ declare module "@tiptap/core" { | |||||||||||||||||||
| imageComponent: { | ||||||||||||||||||||
| insertImageComponent: ({ file, pos, event }: InsertImageComponentProps) => ReturnType; | ||||||||||||||||||||
| uploadImage: (file: File) => () => Promise<string> | undefined; | ||||||||||||||||||||
| getImageSource?: (path: string) => () => Promise<string>; | ||||||||||||||||||||
| restoreImage: (src: string) => () => Promise<void>; | ||||||||||||||||||||
| getImageSource?: (path: string) => () => string; | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
@@ -193,10 +193,10 @@ export const CustomImageExtension = (props: TFileHandler) => { | |||||||||||||||||||
| const fileUrl = await upload(file); | ||||||||||||||||||||
| return fileUrl; | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| getImageSource: (path: string) => async () => await getAssetSrc(path), | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling for getAssetSrc failures. While the async implementation is good for mobile performance, consider adding error handling to gracefully handle cases where Consider updating the implementation: - getImageSource: (path: string) => async () => await getAssetSrc(path),
+ getImageSource: (path: string) => async () => {
+ try {
+ return await getAssetSrc(path);
+ } catch (error) {
+ console.error('Failed to get asset source:', error);
+ throw error;
+ }
+ },📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
| restoreImage: (src: string) => async () => { | ||||||||||||||||||||
| await restoreImageFn(src); | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| getImageSource: (path: string) => () => getAssetSrc(path), | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| }, | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,16 +18,22 @@ export const MentionNodeView = (props) => { | |||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||
| if (!props.extension.options.mentionHighlights) return; | ||||||||||||||||||||||||||||
| const hightlights = async () => { | ||||||||||||||||||||||||||||
| const userId = await props.extension.options.mentionHighlights(); | ||||||||||||||||||||||||||||
| const userId = await props.extension.options.mentionHighlights?.(); | ||||||||||||||||||||||||||||
| setHighlightsState(userId); | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| hightlights(); | ||||||||||||||||||||||||||||
|
Comment on lines
+21
to
24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve error handling and variable naming in mentionHighlights. The current implementation has several issues:
Consider this improvement: - const userId = await props.extension.options.mentionHighlights?.();
- setHighlightsState(userId);
+ try {
+ const highlights = await props.extension.options.mentionHighlights?.();
+ setHighlightsState(highlights);
+ } catch (error) {
+ console.error('Failed to fetch mention highlights:', error);
+ setHighlightsState([]);
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
| }, [props.extension.options]); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { | ||||||||||||||||||||||||||||
| if (!props.node.attrs.redirect_uri) { | ||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||
| <NodeViewWrapper className="mention-component inline w-fit"> | ||||||||||||||||||||||||||||
| <a | ||||||||||||||||||||||||||||
| href={props.node.attrs.redirect_uri} | ||||||||||||||||||||||||||||
| href={props.node.attrs.redirect_uri || "#"} | ||||||||||||||||||||||||||||
| target="_blank" | ||||||||||||||||||||||||||||
| className={cn("mention rounded bg-custom-primary-100/20 px-1 py-0.5 font-medium text-custom-primary-100", { | ||||||||||||||||||||||||||||
| "bg-yellow-500/20 text-yellow-500": highlightsState | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
💡 Codebase verification
Hardcoding
aiEnabled: falseis incorrect and should be configurableThe codebase shows that AI functionality is meant to be configurable through the
disabledExtensionsprop. Inuse-collaborative-editor.ts, theaiEnabledflag is dynamically set based on whether "ai" is included indisabledExtensions. Hardcoding it tofalsein the rich-text editor prevents this configuration from working as intended.packages/editor/src/core/hooks/use-collaborative-editor.ts: UsesaiEnabled: !disabledExtensions?.includes("ai")packages/editor/src/core/components/editors/rich-text/editor.tsx: Incorrectly hardcodesaiEnabled: false🔗 Analysis chain
Verify aiEnabled flag usage
The
aiEnabled: falseis hardcoded in SideMenuExtension. This might conflict with AI-related features mentioned in other components.Let's check for AI-related configurations:
Let me check the implementation details of the AI feature to understand if hardcoding
aiEnabled: falseis correct or not.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 504
Script:
Length of output: 8214