[WEB-2730] chore: core/editor updates to support mobile editor#5910
[WEB-2730] chore: core/editor updates to support mobile editor#5910
Conversation
WalkthroughThe changes in this pull request introduce various enhancements to the editor components by adding new optional properties such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (18)
packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
Line range hint
40-41: Update forwardRef type definitionThe forwardRef wrapper also needs the updated type definition to match the main component.
Update the type definition:
-const RichTextEditorWithRef = forwardRef<EditorRefApi, IRichTextEditor>((props, ref) => +const RichTextEditorWithRef = forwardRef<EditorRefApi, IRichTextEditorProps>((props, ref) =>packages/editor/src/core/types/collaboration.ts (1)
39-39: Add JSDoc documentation for the new property.The addition of
slashCommandEnabledis well-placed and follows TypeScript conventions. Consider adding JSDoc documentation to explain its purpose and default behavior.+ /** Flag to enable/disable slash commands in the editor. Defaults to false. */ slashCommandEnabled?: boolean;packages/editor/src/core/extensions/mentions/mention-node-view.tsx (3)
27-31: Enhance click handler with accessibility improvements.While the click handler works correctly, consider adding:
- ARIA attributes for better accessibility
- Visual feedback when clicking non-redirecting mentions
Consider these improvements:
<a href={props.node.attrs.redirect_uri || "#"} target="_blank" + rel="noopener noreferrer" + aria-disabled={!props.node.attrs.redirect_uri} + title={props.node.attrs.redirect_uri ? "Click to open profile" : "No profile link available"} onClick={handleClick}
Line range hint
36-43: Add security attributes for external links.Opening external links with
target="_blank"withoutrel="noopener noreferrer"can expose the application to security vulnerabilities.Apply this fix:
<a href={props.node.attrs.redirect_uri || "#"} target="_blank" + rel="noopener noreferrer" 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 ? highlightsState.includes(props.node.attrs.entity_identifier) : false, "cursor-pointer": !props.extension.options.readonly, } )}
Let me analyze the results and generate a final response.
Remove global TypeScript and ESLint disabling directives
The scan results show that
mention-node-view.tsxis the only file in the entire codebase that has TypeScript checking completely disabled with@ts-nocheck. While ESLint disabling directives are used in other files, they are typically more targeted and specific to certain rules or lines.This makes
mention-node-view.tsxan outlier in terms of TypeScript practices. The complete disabling of type checking goes against the codebase's conventions and reduces code quality and type safety. Consider:
- Properly typing the component and its props instead of disabling TypeScript
- Using more targeted ESLint disable directives for specific rules if absolutely necessary
- Creating a tracking issue for the TODO comment to ensure these technical debt items are addressed
🔗 Analysis chain
Line range hint
1-5: TypeScript and ESLint should not be disabled globally.The file currently disables TypeScript checking and multiple ESLint rules globally, which can lead to runtime errors and maintenance issues. Consider:
- Properly typing the component props and state
- Addressing ESLint warnings instead of disabling them
- Adding more context to the TODO comment or creating a tracking issue
Let's check if other files follow proper TypeScript practices:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TypeScript and ESLint disable patterns in editor files # Look for files with similar patterns of disabled checks # Search for @ts-nocheck usage echo "Files with disabled TypeScript checking:" rg "@ts-nocheck" -l # Search for eslint-disable usage echo "\nFiles with disabled ESLint rules:" rg "eslint-disable" -lLength of output: 5572
packages/editor/src/core/helpers/scroll-to-node.ts (1)
35-35: Add JSDoc documentation for the new function.Please add documentation to describe the function's purpose, parameters, and behavior.
+/** + * Scrolls to a specific node position in the editor using DOM coordinates + * @param editor - The editor instance + * @param pos - The position in the document to scroll to + * @param behavior - Optional scroll behavior ('auto' | 'smooth') + */ export function scrollToNodeViaDOMCoordinates(editor: Editor, pos: number, behavior?: ScrollBehavior): void {packages/editor/src/core/components/editors/editor-wrapper.tsx (1)
Line range hint
10-14: Consider adding specific TypeScript types for new props.The
onTransactioncallback would benefit from using Tiptap's Transaction type instead of implicitly usingany.type Props = IEditorProps & { children?: (editor: Editor) => React.ReactNode; extensions: Extension<any, any>[]; + onTransaction?: (transaction: Transaction) => void; + handleEditorReady?: () => void; + autofocus?: boolean; };Don't forget to import the Transaction type:
import { Editor, Extension, Transaction } from "@tiptap/core";packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
Line range hint
13-13: Consider adding JSDoc documentation for the new prop.Adding documentation for the
slashCommandEnabledprop in theICollaborativeDocumentEditorinterface would improve maintainability and developer experience.Example documentation:
/** * @property {boolean} [slashCommandEnabled=true] - Controls whether slash commands are enabled in the editor */packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
66-66: Consider mobile-specific image optimizationsSince this update supports mobile, consider adding responsive image handling.
Suggestions for mobile optimization:
- Add srcset/sizes attributes for responsive images
- Consider lazy loading for images outside the viewport
- Implement progressive image loading for better perceived performance
Would you like me to provide a detailed implementation for these optimizations?
packages/editor/src/core/types/editor.ts (2)
38-40: Add JSDoc comments for the new API methods.The new methods enhance editor control capabilities, particularly useful for mobile support. However, they would benefit from documentation explaining:
- The return type of
getCurrentCursorPosition- The expected format of the
positionparameter inscrollToNodeViaDOMCoordinates- The behavior when these methods are called on an unfocused editor
97-99: Maintain consistent naming convention for feature flags.The feature flags follow different naming patterns:
slashCommandEnabledbubbleMenuEnableddragDropEnabledConsider standardizing all feature flags to follow the same pattern, preferably with an
isEnabledprefix for boolean flags.interface IRichTextEditor extends IEditorProps { extensions?: IEditorExtension[]; isSlashCommandEnabled?: boolean; isBubbleMenuEnabled?: boolean; isDragDropEnabled?: boolean; }packages/editor/src/core/components/menus/menu-items.ts (2)
213-222: Consider renaming the key for clarity.While the implementation is solid, consider using "horizontal-rule" instead of "divider" as the key to maintain consistency with the command name and make the purpose more explicit.
- key: "divider", + key: "horizontal-rule",
213-222: Add JSDoc documentation.Consider adding JSDoc documentation to describe the component's purpose and parameters, especially the optional
rangeparameter.+/** + * Creates a menu item for inserting horizontal rules in the editor. + * @param editor The editor instance + * @returns An EditorMenuItem for horizontal rule insertion + */ export const HorizontalRuleItem = (editor: Editor) =>packages/editor/src/core/helpers/editor-commands.ts (1)
184-187: LGTM! Consider adding JSDoc documentation.The implementation follows the established pattern consistently with other editor commands. The function correctly handles both cases - with and without range selection.
Consider adding JSDoc documentation for consistency:
+/** + * Inserts a horizontal rule at the current cursor position + * @param editor The editor instance + * @param range Optional range to replace with horizontal rule + */ export const insertHorizontalRule = (editor: Editor, range?: Range) => { if (range) editor.chain().focus().deleteRange(range).setHorizontalRule().run(); else editor.chain().focus().setHorizontalRule().run(); }web/helpers/editor.helper.ts (1)
Line range hint
46-58: Consider proper async implementation for asset source retrieval.If async is required for future mobile support or remote asset source retrieval:
- Ensure the async requirement is documented in the
TFileHandlertype- Implement proper async operations for asset source retrieval
- Add error handling for async operations
Example implementation if async is needed:
getAssetSrc: async (path) => { if (!path) return ""; try { if (path?.startsWith("http")) { // Add async validation/retrieval for remote URLs if needed const response = await validateRemoteAsset(path); return response.url; } else { const assetSrc = getEditorAssetSrc({ assetId: path, projectId, workspaceSlug, }); // Add async validation/retrieval for local assets if needed const response = await validateLocalAsset(assetSrc); return response.url ?? ""; } } catch (error) { console.error("Failed to retrieve asset source:", error); return ""; } }Also applies to: 97-109
packages/editor/src/styles/editor.css (1)
Line range hint
4-450: Document style precedence and add CSS module safeguards.The mix of Tailwind and custom styles could lead to maintenance challenges. Consider:
- Adding comments to document style precedence
- Using CSS modules or a similar solution to scope custom styles
- Creating a style guide for the editor components
Example documentation format:
/* Style Precedence: * 1. ProseMirror core styles * 2. Custom editor styles * 3. Tailwind utilities */packages/editor/src/core/hooks/use-editor.ts (2)
95-98: Consider adding error handling for onTransaction callback.While the transaction handling logic is correct, the custom callback should be wrapped in a try-catch block to prevent potential errors from affecting the editor's state updates.
onTransaction: ({ editor }) => { setSavedSelection(editor.state.selection); - onTransaction?.(); + try { + onTransaction?.(); + } catch (error) { + console.error('Error in onTransaction callback:', error); + } },
131-137: LGTM: New methods are well-implemented with proper null checks.The new methods are implemented correctly with appropriate null checks and error handling. Consider adding explicit return type annotations for better type safety:
- blur: () => editorRef.current?.commands.blur(), + blur: (): void => editorRef.current?.commands.blur(), - scrollToNodeViaDOMCoordinates(behavior?: ScrollBehavior, pos?: number) { + scrollToNodeViaDOMCoordinates(behavior?: ScrollBehavior, pos?: number): void { - getCurrentCursorPosition: () => savedSelection?.from, + getCurrentCursorPosition: (): number | undefined => savedSelection?.from,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
- packages/editor/src/ce/extensions/document-extensions.tsx (1 hunks)
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx (2 hunks)
- packages/editor/src/core/components/editors/editor-wrapper.tsx (2 hunks)
- packages/editor/src/core/components/editors/lite-text/editor.tsx (1 hunks)
- packages/editor/src/core/components/editors/rich-text/editor.tsx (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (4 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (4 hunks)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (2 hunks)
- packages/editor/src/core/extensions/mentions/mention-node-view.tsx (1 hunks)
- packages/editor/src/core/helpers/editor-commands.ts (1 hunks)
- packages/editor/src/core/helpers/scroll-to-node.ts (1 hunks)
- packages/editor/src/core/hooks/use-collaborative-editor.ts (2 hunks)
- packages/editor/src/core/hooks/use-editor.ts (6 hunks)
- packages/editor/src/core/types/collaboration.ts (1 hunks)
- packages/editor/src/core/types/config.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (2 hunks)
- packages/editor/src/index.ts (1 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- web/helpers/editor.helper.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
Learnt from: SatishGandham PR: makeplane/plane#5864 File: packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts:60-60 Timestamp: 2024-10-22T08:03:04.373Z Learning: In `packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts`, the `getImageSource` command returns a string directly, not a function.
🔇 Additional comments (23)
packages/editor/src/ce/extensions/document-extensions.tsx (1)
18-19: LGTM! Clean implementation of conditional extension.The implementation elegantly handles the conditional inclusion of the SlashCommands extension based on the flag.
packages/editor/src/core/components/editors/lite-text/editor.tsx (1)
Line range hint
1-24: Verify mobile-specific requirements.While the changes enhance editor configurability, let's ensure all mobile-specific requirements are met:
- Touch event handling
- Mobile keyboard behavior
- Performance on mobile devices
Let's check for any mobile-specific configurations or tests:
✅ Verification successful
Mobile support is already well-handled in the editor component
The editor component already has good mobile support through:
- Touch event handling in the image block extension (
packages/editor/src/core/extensions/custom-image/components/image-block.tsx)- Proper mobile-specific header components and layouts throughout the application
- Configurable extensions system that allows platform-specific customization
The changes in the PR enhance this by:
- Making the enter key behavior configurable, which is important for mobile keyboard interactions
- Allowing external extensions to be injected, enabling platform-specific extensions when needed
- Keeping the editor lightweight and flexible for different platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mobile-related configurations and tests echo "Searching for mobile-related files and configurations..." rg -l "mobile|touch|responsive" --type ts --type tsx echo "Checking for mobile-specific test cases..." rg -l "mobile|touch|responsive" --glob "*test*" --glob "*spec*"Length of output: 366
Script:
#!/bin/bash # Search for mobile-related configurations and tests with correct file extensions echo "Searching for mobile-related files and configurations..." rg -l "mobile|touch|responsive" -g "*.ts" -g "*.tsx" echo "\nChecking for touch event handlers..." ast-grep --pattern 'onTouch$_($$$) { $$$ }' echo "\nChecking for mobile-specific imports and configurations..." rg "(import|require).*mobile" -g "*.ts" -g "*.tsx" echo "\nChecking for viewport or media query related code..." rg "(useMediaQuery|@media|viewport)" -g "*.ts" -g "*.tsx"Length of output: 3614
Script:
#!/bin/bash # Let's check the editor-specific mobile handling echo "Checking editor-specific mobile/touch handling..." rg "touch|mobile|responsive" -g "*.ts" -g "*.tsx" --glob "*editor*" -C 2 echo "\nChecking for any mobile-specific editor extensions..." rg "extension.*mobile|mobile.*extension" -g "*.ts" -g "*.tsx" -C 2 echo "\nChecking editor configuration interfaces for mobile properties..." ast-grep --pattern 'interface $_Editor { $$$ }'Length of output: 22840
packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
19-28: Verify external extensions integrationThe order of extensions might affect their behavior. Placing
externalExtensionsat the start means they could be overridden by internal extensions.Let's check if there are any extension ordering dependencies:
Also, consider adding a comment documenting the extension order significance:
const getExtensions = useCallback(() => { const extensions = [ + // External extensions are added first to allow internal extensions to take precedence ...externalExtensions, SideMenuExtension({packages/editor/src/core/helpers/scroll-to-node.ts (1)
35-55: Verify mobile scroll behavior compatibility.Since this PR is related to mobile updates, we should verify that the scrolling behavior works correctly across different mobile browsers and contexts.
Let's check for any mobile-specific scroll handling in the codebase:
✅ Verification successful
Let me search for general scroll behavior implementations to better understand the context.
Based on the search results, I can now provide a comprehensive response about the mobile scroll behavior compatibility:
Mobile scroll behavior is properly handled with appropriate fallbacks and device detection
The codebase shows robust handling of mobile scroll behavior through multiple mechanisms:
- Device/platform detection is implemented via
usePlatformOShook that checks for mobile devices using user agent.- The scroll implementation in
scrollToNodeViaDOMCoordinatesuses standard web APIs (window.scrollTo) with proper fallbacks.- Mobile-specific scroll behavior is handled through:
- Responsive scroll containers with mobile-specific classes
- Touch-friendly scrolling with
scrollbar-smfor mobile viewports- Proper viewport meta tags for mobile scaling
- The scroll behavior accepts an optional
ScrollBehaviorparameter allowing smooth scrolling where supported.The implementation follows best practices for cross-platform scroll behavior and should work reliably across mobile browsers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mobile-specific scroll handling rg -i "(mobile.*scroll|scroll.*mobile)" --type tsLength of output: 51
Script:
#!/bin/bash # Search for scroll-related functionality rg -i "scroll" --type ts -A 3 # Look for viewport or window-related handling that might affect mobile rg -i "(window|viewport).*(scroll|height|width)" --type ts -A 3 # Search for any browser detection or device-specific handling rg -i "(browser|device|platform|userAgent)" --type ts -A 3Length of output: 240283
packages/editor/src/core/components/editors/editor-wrapper.tsx (2)
31-33: LGTM!The new props are correctly destructured and maintain the alphabetical ordering convention.
49-51: Please verify mobile-specific behavior.Since this PR is intended to support mobile updates, please ensure:
- The
autofocusbehavior works as expected on mobile devices (some mobile browsers have restrictions on autofocus)- The
onTransactionhandler performs well with mobile input methods- The
handleEditorReadycallback timing is consistent across different mobile browsersLet's check for any mobile-specific handling in the codebase:
✅ Verification successful
Mobile behavior appears to be properly handled
Based on the codebase analysis:
- The
autofocusprop is properly typed and passed through the component chain without any mobile-specific restrictions, which is correct as modern mobile browsers handle autofocus well.- The
onTransactionhandler is implemented as a simple callback without any device-specific logic, which is appropriate as it should work consistently across all platforms.- Mobile touch events are properly handled in the codebase, particularly in the image component, showing awareness of mobile interactions.
- No mobile-specific issues or restrictions were found in the editor implementation that would affect these props.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for mobile-specific handling in editor files rg -i "mobile|touch|android|ios" --type typescript --type tsx packages/editor/Length of output: 115
Script:
#!/bin/bash # Look for mobile-specific handling in editor files with correct file extensions rg -i "mobile|touch|android|ios" "packages/editor/.*\.(ts|tsx)" # Also check for any mobile-specific configurations or utilities rg -i "mobile|touch|android|ios" "packages/editor/.*\.(json|js)" # Search for autofocus handling rg "autofocus" "packages/editor/.*\.(ts|tsx)" # Look for transaction handlers that might have mobile-specific logic rg "onTransaction" "packages/editor/.*\.(ts|tsx)"Length of output: 505
Script:
#!/bin/bash # First, let's find the correct paths using fd fd -e ts -e tsx . packages/editor/ # Then use these patterns with correct paths rg -i "mobile|touch|android|ios" packages/editor/src/ # Check for autofocus handling rg "autofocus" packages/editor/src/ # Look for transaction handlers rg "onTransaction" packages/editor/src/Length of output: 12010
packages/editor/src/core/components/editors/document/collaborative-editor.tsx (2)
17-17: LGTM! Props destructuring is well-implemented.The new
slashCommandEnabledprop with a default value oftruemaintains backward compatibility and follows proper naming conventions.
47-47: Verify the hook implementation for slashCommandEnabled.The prop is correctly passed to the hook, but let's verify that the
useCollaborativeEditorhook properly handles this configuration.✅ Verification successful
Slash command configuration is properly implemented
The
slashCommandEnabledprop is correctly handled throughout the implementation:
- It's properly passed through the
useCollaborativeEditorhook- In
document-extensions.tsx, it conditionally adds theSlashCommandsextension to the editor when enabled- The implementation follows a clean pattern for feature toggling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hook implementation and its connection to slash commands # Check the hook implementation echo "Checking useCollaborativeEditor hook implementation..." ast-grep --pattern 'useCollaborativeEditor = ($params) => { $$$ slashCommandEnabled $$$ }' # Check for slash command related extensions echo "Checking slash command extensions..." rg -l "SlashCommands.*extension" --type typescriptLength of output: 388
Script:
#!/bin/bash # Let's try a different approach to find the hook implementation and slash command usage # Search for useCollaborativeEditor hook definition echo "Searching for useCollaborativeEditor hook..." rg -A 10 "export const useCollaborativeEditor" # Search for slash command related code echo "Searching for slash command related code..." rg -l "slashCommand" # Find TypeScript/JavaScript files containing the hook echo "Finding files with useCollaborativeEditor..." fd -e ts -e tsx -e js -e jsx | xargs rg "useCollaborativeEditor"Length of output: 2248
Script:
#!/bin/bash # Let's examine how slashCommandEnabled is used in the hook implementation # Check the full implementation of use-collaborative-editor.ts echo "Checking full hook implementation..." cat packages/editor/src/core/hooks/use-collaborative-editor.ts # Check document-extensions for slash command setup echo "Checking document extensions..." cat packages/editor/src/ce/extensions/document-extensions.tsxLength of output: 4096
packages/editor/src/core/hooks/use-collaborative-editor.ts (3)
16-16: LGTM: Props destructuring is clean and follows conventions.The
slashCommandEnabledprop is correctly added to the destructuring pattern.
Line range hint
93-98: Verify collaborative behavior with mixed slash command settings.Since this is a collaborative editor, please ensure that the editor behaves correctly when different users have different
slashCommandEnabledsettings. Consider:
- How slash commands are rendered for users who have them disabled
- How the document state is maintained when some users can't use slash commands
Let's check for any existing handling of feature flags in collaborative context:
#!/bin/bash # Description: Check how feature flags are handled in collaborative context # Look for patterns related to handling different user capabilities rg -A 5 "provider.*document" --type ts # Check for any existing feature flag handling in collaborative context rg -A 5 "enabled.*provider" --type ts
93-93: Verify slash command extension integration.The
slashCommandEnabledflag is passed toDocumentEditorAdditionalExtensions. Please ensure:
- The editor's performance isn't significantly impacted when slash commands are enabled
- The extension order doesn't create conflicts with other extensions
Let's verify the implementation:
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
Line range hint
6-14: LGTM: Improved type definitionsThe consolidation of types by extending NodeViewProps improves type safety and maintainability.
packages/editor/src/core/types/editor.ts (1)
Line range hint
38-105: Verify implementation of new types across the codebase.Let's ensure all components properly implement these interface changes.
✅ Verification successful
Interface changes are properly implemented across the codebase
The verification shows that:
- All new props (
slashCommandEnabled,handleEditorReady,autofocus) are consistently used across the codebase in relevant components and hooks- The interfaces (
ICollaborativeDocumentEditor,IRichTextEditor,ILiteTextEditor) are properly implemented and extended in their respective components- Props are correctly passed through component hierarchies
- None of the TODOs found are related to these interface changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify implementation of new interface properties # Check for proper implementation of new EditorRefApi methods echo "Checking EditorRefApi implementations..." ast-grep --pattern 'class $_ implements EditorRefApi { $$$ blur() { $$$ } $$$ }' # Check for proper usage of new props echo "Checking usage of new props..." rg -l "slashCommandEnabled|handleEditorReady|autofocus" --type typescript # Check for any TODO or FIXME comments related to these changes echo "Checking for related TODOs..." rg "TODO|FIXME" --type typescript -C 2 -g "**/{editor,types}/**"Length of output: 538
Script:
#!/bin/bash # Let's try a different approach with correct file type flags # Check for EditorRefApi implementations echo "Checking EditorRefApi implementations..." rg "implements EditorRefApi" -A 10 --type ts # Check for usage of new props echo "Checking usage of new props..." rg "slashCommandEnabled|handleEditorReady|autofocus" --type ts # Check for proper interface usage echo "Checking interface usage..." rg "ICollaborativeDocumentEditor|IRichTextEditor|ILiteTextEditor" --type ts -A 2 # Check for any related TODOs echo "Checking for related TODOs..." rg "TODO|FIXME" --type ts -g "**/editor/**" -g "**/types/**"Length of output: 11750
packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
27-27: Verify consistency with read-only-custom-image.ts implementation.The signature change to return
Promise<string>is inconsistent withread-only-custom-image.tswhich returns a string directly. This inconsistency needs to be addressed.Let's verify the implementation in read-only-custom-image.ts:
packages/editor/src/core/components/menus/menu-items.ts (3)
2-2: LGTM: Import statements are properly organized.The new imports are correctly placed and support the horizontal rule functionality.
Also applies to: 23-23, 28-28
262-262: LGTM: Proper integration into menu items.The HorizontalRuleItem is logically placed in the menu items array, grouped with other content insertion items.
213-222: Verify mobile compatibility and command implementation.Since this is part of a mobile update, please ensure:
- The insertHorizontalRule command is properly implemented
- The UI/UX works well on mobile devices (touch interactions, proper spacing)
✅ Verification successful
Based on the results, I can now provide a final response:
Implementation is properly handled without mobile-specific concerns
The
insertHorizontalRulecommand is correctly implemented inpackages/editor/src/core/helpers/editor-commands.tsand works by using the editor's chain commands to set a horizontal rule. Since this is a basic editor operation that works through standard click/touch events, no special mobile handling is required. The implementation:
- Uses the editor's built-in
setHorizontalRulecommand- Properly handles both range-based and normal insertions
- Works consistently across desktop and mobile through standard interaction events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if insertHorizontalRule command is properly implemented echo "Checking for insertHorizontalRule implementation..." rg -p "insertHorizontalRule" --type ts # Check for mobile-specific considerations echo "Checking for mobile-specific handling..." rg -p "mobile.*horizontal.*rule|horizontal.*rule.*mobile" --type tsLength of output: 572
Script:
#!/bin/bash # Let's check the implementation of insertHorizontalRule echo "Checking insertHorizontalRule implementation details:" rg -p -B 2 -A 5 "export const insertHorizontalRule" --type ts # Let's check if there are any mobile-specific components or handlers echo "Checking mobile-specific components and handlers:" rg -p "isMobile|onTouchStart|onTouchEnd|TouchEvent" --type ts # Let's verify if horizontal rule has any specific styling or layout considerations echo "Checking horizontal rule styling:" rg -p "horizontalRule.*style|\.hr|horizontal-rule" --type css,scss,tsLength of output: 29515
web/helpers/editor.helper.ts (2)
Line range hint
97-109: Duplicate unnecessary async implementation.Similar to the previous comment, this
getAssetSrcimplementation is also marked as async but performs only synchronous operations.If async isn't required, apply the same fix here:
- getAssetSrc: async (path) => { + getAssetSrc: (path) => {
Line range hint
46-58: Unnecessary async keyword detected.The
getAssetSrcfunction is marked as async but doesn't perform any asynchronous operations. All operations are synchronous string manipulations.Let's verify if async is required by the
TFileHandlertype:If async isn't required by the type definition, consider this fix:
- getAssetSrc: async (path) => { + getAssetSrc: (path) => {✅ Verification successful
The async keyword is required for
getAssetSrcAfter analyzing the codebase, the async implementation is correct and necessary because:
- The
getAssetSrcis used in the editor's command system where it's explicitly called as an async function:getImageSource: (path: string) => async () => await getAssetSrc(path)
- The type
TFileHandleris used in the CustomImageExtension wheregetAssetSrcis awaited, indicating it's designed to be asynchronous by contract.The original review comment suggesting to remove the async keyword was incorrect. The current implementation properly aligns with the editor's requirements and type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if TFileHandler requires async getAssetSrc # Expected: Find the type definition to confirm if async is required ast-grep --pattern 'type TFileHandler = { $$$ getAssetSrc: $type $$$ }'Length of output: 79
Script:
#!/bin/bash # Let's try a different approach to find the type definition # First, let's find files that might contain the TFileHandler type rg "TFileHandler" -l # Then let's look for any imports of TFileHandler in the current file rg "import.*TFileHandler" web/helpers/editor.helper.ts # And search for async getAssetSrc pattern in the codebase to understand the usage rg "getAssetSrc.*async" -A 2Length of output: 1267
Script:
#!/bin/bash # Let's check the type definition in the editor package rg "TFileHandler.*=.*{" -A 10 packages/editor/src/core/types/editor.ts # Also check if there are any async implementations of getAssetSrc in the editor package rg "getAssetSrc.*=.*async" packages/editor/src/core/Length of output: 227
Script:
#!/bin/bash # Fix the regex syntax and try again to find the type definition rg "TFileHandler" -A 10 packages/editor/src/core/types/editor.ts # Also check the actual content of the file to see how getAssetSrc is used in the editor cat packages/editor/src/core/extensions/custom-image/custom-image.tsLength of output: 7173
packages/editor/src/core/hooks/use-editor.ts (4)
36-37: LGTM: Interface additions are well-typed and backward compatible.The new optional properties
onTransactionandautofocusare appropriately typed and maintain backward compatibility.
59-59: LGTM: Props destructuring is clean and maintains defaults.The new props are correctly destructured with appropriate defaults.
Also applies to: 64-64
73-73: LGTM: Editor configuration properly includes autofocus.The autofocus property is correctly passed to the underlying Tiptap editor.
Line range hint
1-289: Verify mobile-specific edge cases.Since this PR aims to support mobile updates, we should verify the following aspects:
- Touch event handling in the editor
- Mobile keyboard interaction with autofocus
- Scroll behavior on mobile devices
packages/editor/src/core/components/editors/lite-text/editor.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/components/editors/lite-text/editor.tsx
Outdated
Show resolved
Hide resolved
| const { | ||
| dragDropEnabled, | ||
| bubbleMenuEnabled = true, | ||
| slashCommandEnabled = true, | ||
| extensions: externalExtensions = [], | ||
| } = props; |
There was a problem hiding this comment.
Type safety: Props interface mismatch
The component accepts props of type IRichTextEditor but destructures additional properties (bubbleMenuEnabled, slashCommandEnabled, extensions) that aren't defined in the interface. This could lead to type-safety issues.
Update the component's props type to include these new properties:
interface IRichTextEditorProps extends IRichTextEditor {
bubbleMenuEnabled?: boolean;
slashCommandEnabled?: boolean;
extensions?: any[];
}Then update the component signature:
const RichTextEditor = (props: IRichTextEditorProps) => {| const fileUrl = await upload(file); | ||
| return fileUrl; | ||
| }, | ||
| getImageSource: (path: string) => async () => await getAssetSrc(path), |
There was a problem hiding this comment.
🛠️ 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 getAssetSrc fails.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| } | |
| }, |
Palanikannan1437
force-pushed
the
chore/editor-updates
branch
2 times, most recently
from
c301469 to
3f11629
Compare
Palanikannan1437
force-pushed
the
chore/editor-updates
branch
from
3f11629 to
3a6733c
Compare
…lane into chore/editor-updates
Palanikannan1437
force-pushed
the
chore/editor-updates
branch
from
a41d71f to
0776169
Compare
1akhanBaheti
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
packages/editor/src/core/types/collaboration.ts (1)
39-40: Add JSDoc comments for the new properties.
The new properties are well-typed, but adding documentation would help other developers understand their purpose and usage.
+ /** Flag to enable/disable slash commands in the editor. Useful for mobile optimization. */
slashCommandEnabled?: boolean;
+ /** Callback fired when editor transactions occur. */
onTransaction?: () => void;space/helpers/editor.helper.ts (1)
Line range hint 73-80: Consider extracting shared asset resolution logic.
The getAssetSrc implementation is identical in both handlers, which introduces maintenance overhead. Consider extracting this into a shared utility function.
Here's a suggested refactor:
+const resolveAssetSrc = async (path: string, anchor: string): Promise<string> => {
+ if (!path) return "";
+ if (path?.startsWith("http")) {
+ return path;
+ } else {
+ return getEditorAssetSrc(anchor, path) ?? "";
+ }
+};
export const getEditorFileHandlers = (args: TArgs): TFileHandler => {
const { anchor, uploadFile, workspaceId } = args;
return {
- getAssetSrc: async (path) => {
- if (!path) return "";
- if (path?.startsWith("http")) {
- return path;
- } else {
- return getEditorAssetSrc(anchor, path) ?? "";
- }
- },
+ getAssetSrc: (path) => resolveAssetSrc(path, anchor),
// ... rest of the handlers
};
};
export const getReadOnlyEditorFileHandlers = (
args: Pick<TArgs, "anchor">
): { getAssetSrc: TFileHandler["getAssetSrc"] } => {
const { anchor } = args;
return {
- getAssetSrc: async (path) => {
- if (!path) return "";
- if (path?.startsWith("http")) {
- return path;
- } else {
- return getEditorAssetSrc(anchor, path) ?? "";
- }
- },
+ getAssetSrc: (path) => resolveAssetSrc(path, anchor),
};
};packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
6-16: Fix typo in type name: "Custo" should be "Custom"
The type name CustoBaseImageNodeViewProps appears to have a typo. Consider renaming it to CustomBaseImageNodeViewProps for consistency and clarity.
-export type CustoBaseImageNodeViewProps = {
+export type CustomBaseImageNodeViewProps = {
getPos: () => number;
editor: Editor;
node: NodeViewProps["node"] & {
attrs: ImageAttributes;
};
updateAttributes: (attrs: Partial<ImageAttributes>) => void;
selected: boolean;
};
-export type CustomImageNodeProps = NodeViewProps & CustoBaseImageNodeViewProps;
+export type CustomImageNodeProps = NodeViewProps & CustomBaseImageNodeViewProps;packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
Line range hint 10-157: Consider enhancing mobile-specific interaction patterns
While the component handles basic touch interactions well, consider these mobile-specific enhancements:
- Add explicit touch event handlers alongside drag events
- Consider implementing a mobile-specific upload UI variant
- Add proper touch feedback states beyond hover effects
These would improve the mobile experience while maintaining desktop functionality.
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (2)
4-4: Fix inconsistent type naming convention.
The type name CustoBaseImageNodeViewProps appears to have a typo. Consider renaming it to CustomBaseImageNodeViewProps to maintain consistency with other "Custom" prefixed types in the codebase.
Also applies to: 40-40
Line range hint 244-260: Consider simplifying error handling logic.
The current error handling uses multiple boolean flags which increases complexity. Consider refactoring to use a single state enum:
type ImageLoadState = 'initial' | 'loading' | 'error' | 'restored' | 'failed';
// Then replace multiple flags with a single state:
const [imageLoadState, setImageLoadState] = useState<ImageLoadState>('initial');
// Usage in error handler:
onError={async (e) => {
if (!editor?.commands.restoreImage || imageLoadState === 'failed') {
setImageLoadState('failed');
setFailedToLoadImage(true);
return;
}
try {
setImageLoadState('loading');
await editor?.commands.restoreImage?.(imgNodeSrc);
imageRef.current.src = resolvedImageSrc;
setImageLoadState('restored');
} catch {
setImageLoadState('failed');
setFailedToLoadImage(true);
console.error("Error while loading image", e);
}
}}This would make the error handling flow more explicit and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- packages/editor/src/ce/extensions/document-extensions.tsx (1 hunks)
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx (2 hunks)
- packages/editor/src/core/components/editors/lite-text/editor.tsx (2 hunks)
- packages/editor/src/core/components/editors/rich-text/editor.tsx (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-block.tsx (7 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (3 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1 hunks)
- packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1 hunks)
- packages/editor/src/core/extensions/image/extension.tsx (1 hunks)
- packages/editor/src/core/extensions/image/read-only-image.tsx (1 hunks)
- packages/editor/src/core/hooks/use-collaborative-editor.ts (3 hunks)
- packages/editor/src/core/types/collaboration.ts (1 hunks)
- space/helpers/editor.helper.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/editor/src/ce/extensions/document-extensions.tsx
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx
- packages/editor/src/core/components/editors/lite-text/editor.tsx
- packages/editor/src/core/hooks/use-collaborative-editor.ts
🧰 Additional context used
📓 Learnings (2)
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1)
Learnt from: SatishGandham
PR: makeplane/plane#5864
File: packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts:60-60
Timestamp: 2024-10-22T08:03:04.373Z
Learning: In `packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts`, the `getImageSource` command returns a string directly, not a function.
packages/editor/src/core/extensions/image/read-only-image.tsx (1)
Learnt from: SatishGandham
PR: makeplane/plane#5864
File: packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts:60-60
Timestamp: 2024-10-22T08:03:04.373Z
Learning: In `packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts`, the `getImageSource` command returns a string directly, not a function.
🔇 Additional comments (17)
packages/editor/src/core/extensions/image/read-only-image.tsx (1)
29-29:
Review the async implementation of getImageSource.
The current implementation has several concerns:
- The double function wrapping (
async () => await) seems unnecessary and could be simplified - This change might break compatibility with other parts of the editor expecting synchronous behavior
- This contradicts the implementation in PR #5864 where
getImageSourcereturned a string directly
Consider simplifying the implementation:
-getImageSource: (path: string) => async () => await getAssetSrc(path),
+getImageSource: (path: string) => getAssetSrc(path),Let's verify the usage of this command in the codebase:
packages/editor/src/core/components/editors/rich-text/editor.tsx (2)
11-16: LGTM: Props destructuring with sensible defaults
The default values for bubbleMenuEnabled and slashCommandEnabled are appropriately set to true, maintaining backward compatibility.
Note: The type safety issue mentioned in the previous review is still applicable.
19-31: LGTM: Extensions setup with proper dependency tracking
The useCallback hook correctly includes all dependencies (dragDropEnabled, slashCommandEnabled, externalExtensions) in its dependency array, ensuring proper memoization.
Note: The concern about hardcoded aiEnabled: false from the previous review is still valid.
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1)
71-71:
Fix the getImageSource command implementation.
The current implementation has several issues:
- Based on previous feedback,
getImageSourceshould return a string directly, not a function - The async/await wrapper is unnecessary since it's already returning a function
- This change could break mobile functionality if the calling code expects synchronous behavior
Apply this fix to align with the expected behavior:
- getImageSource: (path: string) => async () => await getAssetSrc(path),
+ getImageSource: (path: string) => getAssetSrc(path),Let's verify the usage of getImageSource in the codebase:
space/helpers/editor.helper.ts (1)
Line range hint 34-41: Verify the necessity of async transformation.
The function is marked as async but contains no await operations. While this maintains interface compatibility with the updated type definition, consider:
- If there are plans to add async operations in the future, add a TODO comment explaining the rationale.
- If no async operations are planned, consider keeping it synchronous to avoid unnecessary Promise wrapping.
Let's check if other implementations of this interface use async operations:
✅ Verification successful
The async transformation is necessary and correctly implemented
The search results confirm that getAssetSrc is used by image extensions in the editor that perform asynchronous operations. Specifically:
- The
TFileHandlerinterface inpackages/editor/src/core/types/config.tsexplicitly definesgetAssetSrcas returning aPromise<string>. - Multiple image extensions (
ReadOnlyImageExtension,CustomImageExtension,CustomReadOnlyImageExtension) usegetAssetSrcin theirgetImageSourcecommands withawait.
The async implementation is consistent with the type definition and its usage across the codebase, particularly in image handling functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of getAssetSrc to understand the async requirement
ast-grep --pattern 'getAssetSrc: async'
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search for getAssetSrc implementations and related patterns
rg "getAssetSrc" -A 3 -B 3
# Also search for the interface or type definition that might require this async
rg "interface.*Editor.*Handler" -A 10 -B 1
Length of output: 15705
packages/editor/src/core/extensions/image/extension.tsx (1)
79-79: LGTM! Verify consistency of async image source retrieval.
The change to make getImageSource async is correct and properly implemented. This aligns with the broader changes to support asynchronous asset source retrieval.
Let's verify that all image source retrievals are consistently handled as async operations:
✅ Verification successful
All image source retrievals are consistently async across the codebase
The verification confirms that:
- All
getAssetSrcimplementations are properly async, returning Promises - All
getImageSourceimplementations consistently use async/await pattern - The async pattern is used in all image extensions:
- ImageExtension
- ReadOnlyImageExtension
- CustomImageExtension
- CustomReadOnlyImageExtension
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent async handling of getAssetSrc across the codebase
# Test 1: Look for any remaining synchronous getAssetSrc calls
echo "Checking for potentially synchronous getAssetSrc calls..."
rg "getAssetSrc\([^)]*\)(?!\s*\.(then|catch|finally|\s*await))" -g "!*.md"
# Test 2: Verify all getImageSource implementations are async
echo "Verifying getImageSource implementations..."
ast-grep --pattern 'getImageSource: $_ => async () => await $_'
Length of output: 646
Script:
#!/bin/bash
# Let's try a different approach with simpler patterns
# Check for getAssetSrc usage patterns
echo "Checking getAssetSrc usage patterns..."
rg "getAssetSrc" -A 2 -B 2
# Check for getImageSource implementations
echo "Checking getImageSource implementations..."
rg "getImageSource" -A 2 -B 2
Length of output: 14541
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (3)
18-26: LGTM: Clean state management
The component's state management is well-structured with clear separation between different image states (upload status, resolved source, file system source, and error state).
Line range hint 60-89: LGTM: Clean conditional rendering
The component's rendering logic is well-structured with clear conditions for switching between image block and uploader components. Props are properly passed down to child components.
51-58:
Add cleanup for async operation
The async operation needs a cleanup function to prevent state updates if the component unmounts during the image source resolution.
useEffect(() => {
+ let mounted = true;
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);
+ if (mounted) setResolvedSrc(url as string);
};
getImageSource();
+ return () => {
+ mounted = false;
+ };
}, [imageFromFileSystem, node.attrs.src]);Note: The previous review comments about type safety and error handling are still applicable here.
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (2)
8-8: LGTM: Import statement is well-structured
The import statement correctly brings in the necessary types and utilities, following TypeScript best practices.
Line range hint 10-16: Fix typo in type name and verify mobile requirements
The type name CustoBaseImageNodeViewProps appears to have a typo ("Custo" instead of "Custom"). Additionally, since this PR aims to support mobile updates, we should verify if any mobile-specific props are needed.
Let's check for consistency in naming and related mobile-specific props:
packages/editor/src/core/components/menus/menu-items.ts (4)
23-23: LGTM: Icon import for horizontal rule.
The MinusSquare icon is appropriately imported and well-suited for representing a horizontal rule in the menu.
28-28: LGTM: Helper function import.
The insertHorizontalRule import follows the established pattern and is correctly placed.
262-262: LGTM: Menu item integration.
The HorizontalRuleItem is appropriately integrated into the menu items array, with logical placement among other content insertion items.
213-222: Verify horizontal rule implementation dependencies.
Let's verify that all required dependencies for the horizontal rule functionality are in place.
Also applies to: 262-262
✅ Verification successful
Let me try a different search strategy to verify the horizontal rule implementation.
Horizontal rule implementation is properly integrated
The verification shows that the horizontal rule functionality is fully implemented with all required dependencies in place:
- The
CustomHorizontalRuleextension is properly registered in multiple extension configurations - The
insertHorizontalRulecommand is implemented ineditor-commands.ts - The node type is properly configured with styles and drag-handle support
- The functionality is accessible through both menu items and slash commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify horizontal rule implementation dependencies
# Test 1: Check if horizontalRule extension is registered
echo "Checking for horizontalRule extension registration..."
rg -l "horizontalRule.*?extension" packages/editor/src/
# Test 2: Verify insertHorizontalRule command implementation
echo "Checking insertHorizontalRule command implementation..."
ast-grep --pattern 'export const insertHorizontalRule = (editor: Editor) => {
$$$
}'
Length of output: 384
Script:
#!/bin/bash
# Search for horizontalRule related code with more context
echo "Searching for horizontalRule node type and commands..."
rg "horizontalRule" packages/editor/src/ -B 2 -A 2
# Search for extension registration patterns
echo "Searching for extension registration patterns..."
ast-grep --pattern 'Schema.extend({
$$$
horizontalRule: $_
$$$
})'
# Look for any horizontal rule related imports or configurations
echo "Searching for horizontal rule related imports..."
rg "horizontalRule|HorizontalRule" packages/editor/src/ --type ts
Length of output: 6132
packages/editor/src/core/extensions/custom-image/components/image-block.tsx (2)
59-59: LGTM! Improved naming and image source handling.
The rename from remoteImageSrc to resolvedImageSrc better reflects the variable's purpose, as the image source could be resolved from various sources, not just remote ones. The error handling logic has also been improved to use the correct source for image restoration.
Also applies to: 213-213, 215-215, 217-217, 251-252
267-267: LGTM! Enhanced visual feedback for image states.
Good improvements to the UI/UX:
- The blur state is now properly tied to the image resolution state
- Added clear visual feedback for selected images with a highlight overlay
- The selection highlight is correctly shown only when using the resolved image
Also applies to: 287-289
| <EditorWrapper {...props} extensions={getExtensions()}> | ||
| {(editor) => <>{editor && <EditorBubbleMenu editor={editor} />}</>} | ||
| {(editor) => <>{editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />}</>} | ||
| </EditorWrapper> |
There was a problem hiding this comment.
💡 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:
EditorBubbleMenuPropsis correctly typed asOmit<BubbleMenuProps, "children">- However, the component implementation uses
props: anywhich bypasses type checking - The editor prop from TipTap's BubbleMenu is being passed through without proper typing
- Consider removing the
: anytype annotation inEditorBubbleMenucomponent to leverage proper type inference - Update the component to properly type the props parameter:
export const EditorBubbleMenu: FC<EditorBubbleMenuProps> = (props) => {
🔗 Analysis chain
Verify editor prop type safety
While the conditional rendering works, let's ensure the editor prop type is correctly handled.
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 executed
The 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
|
|
||
| export type TCollaborativeEditorProps = TCollaborativeEditorHookProps & { | ||
| slashCommandEnabled?: boolean; | ||
| onTransaction?: () => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the transaction handler with parameters.
The onTransaction callback might benefit from parameters to provide transaction details, which could be useful for debugging or handling specific transaction types.
- onTransaction?: () => void;
+ onTransaction?: (transaction: Transaction) => void;Don't forget to add the corresponding import:
import { Transaction } from '@tiptap/pm/state';| if (resolvedSrc) { | ||
| setIsUploaded(true); | ||
| setImageFromFileSystem(undefined); | ||
| } else { | ||
| setIsUploaded(false); | ||
| } | ||
| }, [remoteImageSrc]); | ||
| }, [resolvedSrc]); |
There was a problem hiding this comment.
🛠️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (resolvedSrc) { | |
| setIsUploaded(true); | |
| setImageFromFileSystem(undefined); | |
| } else { | |
| setIsUploaded(false); | |
| } | |
| }, [remoteImageSrc]); | |
| }, [resolvedSrc]); | |
| setIsUploaded(Boolean(resolvedSrc)); | |
| if (resolvedSrc) setImageFromFileSystem(undefined); | |
| }, [resolvedSrc]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/editor/src/core/components/editors/lite-text/editor.tsx (1)
12-18: Well-implemented extension management with performance optimization.
The implementation effectively addresses performance and immutability concerns:
- Uses
useMemoto prevent unnecessary recreation of the extensions array - Properly handles extension composition without mutating props
- Correctly specifies all dependencies in the dependency array
Consider documenting the disabledExtensions format and available options in the component's JSDoc for better maintainability.
To improve maintainability, consider:
- Creating a constant for extension identifiers (e.g.,
EXTENSION_IDS.ENTER_KEYinstead of the string literal "enter-key") - Adding validation for the disabledExtensions array to catch typos early
packages/editor/src/core/types/editor.ts (2)
38-40: LGTM! Consider adding JSDoc comments.
The new methods have clear purposes and proper type definitions. Adding JSDoc comments would improve developer experience by providing usage examples and parameter descriptions.
/** Removes focus from the editor */
blur: () => void;
/**
* Scrolls to a specific node position with optional scroll behavior
* @param behavior - Optional ScrollBehavior ('auto' | 'smooth')
* @param position - Target position in the document
*/
scrollToNodeViaDOMCoordinates: (behavior?: ScrollBehavior, position?: number) => void;
/** Returns the current cursor position in the document */
getCurrentCursorPosition: () => number | undefined;98-98: Add documentation for bubbleMenuEnabled.
The purpose and behavior of the bubble menu should be documented for clarity.
/**
* Enables/disables the bubble menu that appears when text is selected
* @default false
*/
bubbleMenuEnabled?: boolean;packages/editor/src/core/components/menus/menu-items.ts (1)
213-220: Consider adding JSDoc documentation.
To maintain consistency and improve maintainability, consider adding JSDoc documentation for the HorizontalRuleItem function, describing its purpose and parameters.
Example:
+/**
+ * Creates a menu item for inserting horizontal rules in the editor.
+ * @param editor - The editor instance to attach the command to
+ * @returns An EditorMenuItem configuration for horizontal rules
+ */
export const HorizontalRuleItem = (editor: Editor) =>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/editor/src/ce/extensions/document-extensions.tsx (1 hunks)
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx (2 hunks)
- packages/editor/src/core/components/editors/lite-text/editor.tsx (2 hunks)
- packages/editor/src/core/components/editors/rich-text/editor.tsx (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 hunks)
- packages/editor/src/core/hooks/use-collaborative-editor.ts (2 hunks)
- packages/editor/src/core/types/collaboration.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (3 hunks)
- packages/editor/src/core/types/extensions.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/editor/src/ce/extensions/document-extensions.tsx
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx
- packages/editor/src/core/hooks/use-collaborative-editor.ts
- packages/editor/src/core/types/collaboration.ts
🔇 Additional comments (13)
packages/editor/src/core/types/extensions.ts (1)
1-1: LGTM! Verify extension handling in dependent components.
The new extension types follow the existing naming convention and align with the mobile editor support objective.
Let's verify how these new extensions are handled across the codebase:
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about how the new extensions are being handled across the codebase.
✅ Extensions are properly handled in dependent components
The new extension types are correctly integrated across the codebase:
- "slash-commands" is properly handled in both document extensions and rich text editor with conditional inclusion
- "enter-key" is correctly implemented in the lite text editor with conditional inclusion
- All components using TExtensions are properly checking for disabled extensions before applying them
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for components that use TExtensions and might need updates
# Search for files importing or using TExtensions
echo "Files potentially affected by TExtensions changes:"
rg -l "TExtensions"
# Check specific usage of new extensions
echo -e "\nUsage of new extension types:"
rg -A 3 "(slash-commands|enter-key)"
Length of output: 2735
packages/editor/src/core/components/editors/lite-text/editor.tsx (2)
1-1: LGTM! Appropriate import for performance optimization.
The addition of useMemo import is correct and aligns with its usage for memoizing the extensions array.
10-10: Verify type definitions for new props.
The component uses disabledExtensions and extensions props, but we need to ensure these are properly typed in the ILiteTextEditor interface.
Let's verify the type definitions:
✅ Verification successful
Let me check the IEditorProps interface since ILiteTextEditor extends it and we need to verify the disabledExtensions prop.
Props are properly typed in the interfaces
The type definitions are correctly set up:
extensionsis defined inILiteTextEditorinterface asextensions?: any[]disabledExtensionsis defined inIEditorPropsinterface (whichILiteTextEditorextends) asdisabledExtensions?: TExtensions[]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ILiteTextEditor interface definition
ast-grep --pattern 'interface ILiteTextEditor {
$$$
}'
# Backup: Search using ripgrep if ast-grep doesn't find it
rg -A 10 'interface ILiteTextEditor'
Length of output: 905
Script:
#!/bin/bash
# Search for IEditorProps interface
ast-grep --pattern 'interface IEditorProps {
$$$
}'
# Backup: Search using ripgrep
rg -A 15 'interface IEditorProps'
Length of output: 3006
packages/editor/src/core/components/editors/rich-text/editor.tsx (6)
11-16: Type safety issue with props interface still exists.
The component is still destructuring properties that aren't defined in the IRichTextEditor interface.
19-24: Hardcoding aiEnabled: false prevents proper configuration.
The aiEnabled flag should be configurable through disabledExtensions.
25-31: LGTM! Clean extensions handling implementation.
The conditional extension addition based on disabledExtensions is well implemented with proper dependency tracking.
34-36: Nested conditional rendering could be simplified.
The current implementation using nested conditions could be more readable.
Line range hint 40-46: LGTM! Proper ref handling implementation.
The component correctly implements ref forwarding with proper TypeScript types and follows React best practices by setting the displayName.
Line range hint 1-46: Verify mobile editor support implementation.
While the changes add flexibility to the editor, it's not immediately clear how they specifically support mobile editor functionality. Let's verify the mobile-specific implementations:
packages/editor/src/core/types/editor.ts (1)
91-91: LGTM! Explicit null handling improves type safety.
The addition of null to the value type properly handles cases where no content is present.
packages/editor/src/core/components/menus/menu-items.ts (3)
23-23: LGTM! Import statements are properly organized.
The new imports for MinusSquare icon and insertHorizontalRule helper are correctly placed in their respective sections.
Also applies to: 28-28
213-220: Previous review suggestions are still applicable.
The implementation is functionally correct but could benefit from the improvements suggested in the previous review.
260-260: LGTM! Menu item is properly integrated.
The HorizontalRuleItem is correctly added to the menu items array with appropriate placement in the formatting options group.
| suggestions?: () => Promise<IMentionSuggestion[]>; | ||
| }; | ||
| onChange?: (json: object, html: string) => void; | ||
| onTransaction?: () => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing onTransaction type definition.
The onTransaction callback could be more useful with transaction details as parameters.
import { Transaction } from '@tiptap/core';
onTransaction?: (transaction: Transaction) => void;
1akhanBaheti
changed the title
5 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Miscellaneous
attached issue: WEB-2730