[PE-92] fix: removing readonly collaborative document editor #6209
[PE-92] fix: removing readonly collaborative document editor #6209
Conversation
WalkthroughThis pull request introduces a comprehensive modification to the editor's architecture by adding an Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant CollaborativeEditor
participant UseCollaborativeEditor
User->>Editor: Set editable prop
Editor->>CollaborativeEditor: Pass editable state
CollaborativeEditor->>UseCollaborativeEditor: Configure editor with editable prop
UseCollaborativeEditor-->>CollaborativeEditor: Return configured editor
CollaborativeEditor-->>Editor: Render editor with editability
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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: 4
🔭 Outside diff range comments (1)
packages/editor/src/core/components/editors/editor-wrapper.tsx (1)
Line range hint
12-16: Fix Props type definition.The
editableprop is used but not included in the Props type definition. This could cause TypeScript errors.Add the
editableproperty to the Props type:type Props = IEditorProps & { children?: (editor: Editor) => React.ReactNode; extensions: Extension<any, any>[]; + editable?: boolean; };
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/extensions.tsx (1)
50-54: 🛠️ Refactor suggestionConsider making
editableprop requiredThe
editableprop is marked as optional but seems to control critical editor functionality. Consider making it required to ensure proper editor state management.type TArguments = { - editable?: boolean; + editable: boolean; };
🧹 Nitpick comments (11)
web/core/components/pages/editor/header/mobile-root.tsx (1)
Line range hint
2-2: Remove unused importEditorReadOnlyRefApi.The
EditorReadOnlyRefApiimport is no longer used after removing readonly editor functionality.packages/editor/src/core/components/editors/editor-wrapper.tsx (1)
42-42: Document the default value foreditableprop.Consider documenting the default behavior when
editableis undefined, as this affects the editor's functionality.Add a comment explaining the default behavior:
const editor = useEditor({ + // If undefined, editor is editable by default editable, disabledExtensions,web/core/components/pages/editor/header/root.tsx (1)
43-43: Consider consolidating editor ref checksMultiple null checks for
editorRef.currentcould be consolidated to improve code maintainability.- {editorReady && isContentEditable && editorRef.current && <PageToolbar editorRef={editorRef?.current} />} + {editorReady && isContentEditable && <PageToolbar editorRef={editorRef.current} />}Also applies to: 50-50, 52-52
packages/editor/src/core/extensions/drop.tsx (2)
34-59: Consider refactoring for better readabilityThe handleDrop method could be refactored to improve readability while maintaining the same functionality.
Consider this refactoring:
handleDrop: (view: EditorView, event: DragEvent, _slice: any, moved: boolean) => { - if ( - editor.isEditable && - !moved && - event.dataTransfer && - event.dataTransfer.files && - event.dataTransfer.files.length > 0 - ) { - event.preventDefault(); - const files = Array.from(event.dataTransfer.files); - const imageFiles = files.filter((file) => file.type.startsWith("image")); - - if (imageFiles.length > 0) { - const coordinates = view.posAtCoords({ - left: event.clientX, - top: event.clientY, - }); - - if (coordinates) { - const pos = coordinates.pos; - insertImagesSafely({ editor, files: imageFiles, initialPos: pos, event: "drop" }); - } - return true; - } - } - return false; + if (!editor.isEditable || moved) return false; + + const files = event.dataTransfer?.files; + if (!files?.length) return false; + + const imageFiles = Array.from(files).filter((file) => file.type.startsWith("image")); + if (!imageFiles.length) return false; + + event.preventDefault(); + + const coordinates = view.posAtCoords({ + left: event.clientX, + top: event.clientY, + }); + + if (coordinates) { + insertImagesSafely({ + editor, + files: imageFiles, + initialPos: coordinates.pos, + event: "drop" + }); + } + + return true; }
Line range hint
66-93: Consider extracting image handling logicThe
insertImagesSafelyfunction looks good, but consider extracting the common image file filtering logic to reduce duplication between handlePaste and handleDrop.Consider creating a utility function:
const getImageFiles = (files: FileList | File[]): File[] => { return Array.from(files).filter((file) => file.type.startsWith("image")); };web/core/components/pages/editor/header/extra-options.tsx (1)
10-10: Remove unused importThe
SyncingComponentis imported but never used in this file.- import { SyncingComponent } from "@/components/icons/syncing-component";web/core/hooks/use-collaborative-page-actions.tsx (1)
53-56: Improve error handling for undefined serverEventNameConsider adding error handling or logging when
serverEventNameis undefined to help with debugging.const serverEventName = getServerEventName(clientAction); if (serverEventName) { editorRef?.emitRealTimeUpdate(serverEventName); + } else { + console.warn(`No server event name found for client action: ${clientAction}`); }packages/editor/src/core/types/editor.ts (1)
109-109: Consider consolidating editor interfacesWhile adding the
editableproperty is a good step towards flexible editor control, maintaining separate readonly interfaces (IReadOnlyEditorProps,ILiteTextReadOnlyEditor, etc.) alongside this property creates redundancy and potential confusion.Consider:
- Deprecating the readonly interfaces in favor of using the
editableproperty- Creating a migration guide for users to transition from readonly interfaces to the new approach
- Adding JSDoc deprecation notices to the readonly interfaces
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (2)
140-149: Simplify className conditionsThe current implementation has multiple repetitive checks for
editor.isEditableand complex conditional styling.Consider simplifying the className logic:
-"image-upload-component flex items-center justify-start gap-2 py-3 px-2 rounded-lg text-custom-text-300 bg-custom-background-90 border border-dashed border-custom-border-300 transition-all duration-200 ease-in-out cursor-default", -{ - "hover:text-custom-text-200 hover:bg-custom-background-80 cursor-pointer": editor.isEditable, - "bg-custom-background-80 text-custom-text-200": draggedInside && editor.isEditable, - "text-custom-primary-200 bg-custom-primary-100/10 border-custom-primary-200/10 hover:bg-custom-primary-100/10 hover:text-custom-primary-200": - selected && editor.isEditable, - "text-red-500 cursor-default": failedToLoadImage, - "hover:text-red-500": failedToLoadImage && editor.isEditable, - "bg-red-500/10": failedToLoadImage && selected, - "hover:bg-red-500/10": failedToLoadImage && selected && editor.isEditable, -} +cn( + "image-upload-component flex items-center justify-start gap-2 py-3 px-2 rounded-lg transition-all duration-200 ease-in-out", + // Base styles + "text-custom-text-300 bg-custom-background-90 border border-dashed border-custom-border-300", + // Cursor styles + editor.isEditable && !failedToLoadImage ? "cursor-pointer" : "cursor-default", + // Interactive states when editable + editor.isEditable && { + "hover:text-custom-text-200 hover:bg-custom-background-80": !failedToLoadImage, + "hover:text-red-500 hover:bg-red-500/10": failedToLoadImage && selected, + }, + // Drag states + draggedInside && editor.isEditable && "bg-custom-background-80 text-custom-text-200", + // Selected states + selected && editor.isEditable && "text-custom-primary-200 bg-custom-primary-100/10 border-custom-primary-200/10", + // Error states + failedToLoadImage && "text-red-500", + failedToLoadImage && selected && "bg-red-500/10" +)
Line range hint
130-135: Extract editable state check to a separate variableThe
getDisplayMessagefunction mixes state checks with message selection.Consider separating the concerns:
const getDisplayMessage = useCallback(() => { const isUploading = isImageBeingUploaded; + const canDropImage = draggedInside && editor.isEditable; if (failedToLoadImage) { return "Error loading image"; } if (isUploading) { return "Uploading..."; } - if (draggedInside && editor.isEditable) { + if (canDropImage) { return "Drop image here"; } return "Add an image"; -}, [draggedInside, failedToLoadImage, isImageBeingUploaded]); +}, [draggedInside, editor.isEditable, failedToLoadImage, isImageBeingUploaded]);web/core/components/pages/editor/editor-body.tsx (1)
177-217: LGTM: Unified editor implementationThe consolidation to a single editor component with an
editableprop is a cleaner approach than maintaining separate editor implementations. The configuration is comprehensive and includes all necessary handlers.Consider documenting the editable prop behavior in component documentation or README to help other developers understand this architectural change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/editor/src/core/components/editors/document/collaborative-editor.tsx(3 hunks)packages/editor/src/core/components/editors/document/collaborative-read-only-editor.tsx(0 hunks)packages/editor/src/core/components/editors/document/index.ts(0 hunks)packages/editor/src/core/components/editors/document/page-renderer.tsx(1 hunks)packages/editor/src/core/components/editors/editor-wrapper.tsx(2 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(2 hunks)packages/editor/src/core/extensions/drop.tsx(1 hunks)packages/editor/src/core/extensions/extensions.tsx(4 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(0 hunks)packages/editor/src/core/hooks/use-collaborative-editor.ts(3 hunks)packages/editor/src/core/hooks/use-editor.ts(3 hunks)packages/editor/src/core/hooks/use-file-upload.ts(1 hunks)packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts(0 hunks)packages/editor/src/core/types/collaboration.ts(1 hunks)packages/editor/src/core/types/editor.ts(1 hunks)packages/editor/src/index.ts(0 hunks)packages/editor/src/styles/editor.css(1 hunks)web/core/components/pages/editor/editor-body.tsx(5 hunks)web/core/components/pages/editor/header/extra-options.tsx(3 hunks)web/core/components/pages/editor/header/info-popover.tsx(1 hunks)web/core/components/pages/editor/header/mobile-root.tsx(1 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(2 hunks)web/core/components/pages/editor/header/root.tsx(2 hunks)web/core/components/pages/editor/page-root.tsx(3 hunks)web/core/components/pages/editor/summary/content-browser.tsx(1 hunks)web/core/components/pages/editor/summary/popover.tsx(1 hunks)web/core/components/pages/modals/export-page-modal.tsx(2 hunks)web/core/hooks/use-collaborative-page-actions.tsx(2 hunks)
💤 Files with no reviewable changes (5)
- packages/editor/src/core/components/editors/document/index.ts
- packages/editor/src/index.ts
- packages/editor/src/core/extensions/read-only-extensions.tsx
- packages/editor/src/core/hooks/use-read-only-collaborative-editor.ts
- packages/editor/src/core/components/editors/document/collaborative-read-only-editor.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/core/components/editors/document/page-renderer.tsx
🔇 Additional comments (21)
packages/editor/src/core/types/collaboration.ts (1)
24-24:
Consider removing TReadOnlyCollaborativeEditorProps type.
While adding the editable property is a step towards removing readonly editor functionality, the TReadOnlyCollaborativeEditorProps type still exists and references EditorReadOnlyRefApi. This seems inconsistent with the PR's goal.
Let's verify if EditorReadOnlyRefApi is still being used elsewhere:
web/core/components/pages/editor/header/mobile-root.tsx (1)
43-43: Consider consolidating editability checks.
The condition isContentEditable && editorRef.current might be redundant with the new editable prop from the editor. Consider using a single source of truth for editability.
Let's check how isContentEditable is used across the codebase:
packages/editor/src/core/components/editors/document/collaborative-editor.tsx (2)
22-22: LGTM: Editable prop addition
The addition of the editable prop aligns with the PR objective of consolidating editor functionality into a single component with controlled editability.
Line range hint 49-58: Verify editable prop behavior in useCollaborativeEditor
The editable prop is correctly passed to the hook, but we should verify its implementation in the hook to ensure proper editor state management.
✅ Verification successful
Let me verify the implementation of useEditor to ensure proper handling of the editable prop.
Let me check the implementation of useTiptapEditor to complete the verification chain.
Let me try a different approach to find the Tiptap editor usage.
Based on the code analysis from the previous results, I can now provide a final response:
The editable prop is properly implemented in the editor chain
The editable prop is correctly handled throughout the component hierarchy:
- Passed from
collaborative-editor.tsxtouseCollaborativeEditor - Forwarded to
useEditorhook - Used in the editor configuration to control editability
- Properly included in the dependency array for editor updates
The implementation ensures proper editor state management when the editable prop changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useCollaborativeEditor implementation and its usage of editable prop
ast-grep --pattern 'export const useCollaborativeEditor = $_' -A 20
Length of output: 8942
Script:
#!/bin/bash
# Search for useEditor implementation and its usage of editable prop
ast-grep --pattern 'export const useEditor = $_' -A 20
Length of output: 22877
Script:
#!/bin/bash
# Search for useTiptapEditor implementation
ast-grep --pattern 'export const useTiptapEditor = $_'
Length of output: 56
Script:
#!/bin/bash
# Search for useEditor from tiptap
rg "from '@tiptap/react'" -A 5
Length of output: 38
web/core/components/pages/editor/header/root.tsx (2)
23-23: LGTM: Props cleanup
Successfully removed readOnly-related props, simplifying the component's interface.
29-29: Verify editor ready state handling
The editor ready checks have been simplified, but we should verify there are no edge cases where readOnly state was specifically needed.
Also applies to: 35-35
✅ Verification successful
Let me search for any readOnly state checks that might have been related to editorReady.
Let me try a different approach to search for readOnly related checks.
Based on the search results, I can now provide a final response:
Editor ready state changes are safe to simplify
The readOnly state and editorReady state are handled independently in the codebase. The readOnly state is primarily controlled through the isContentEditable prop and passed directly to the editor components, while editorReady is used for UI rendering conditions. The simplification of editor ready checks does not impact the readOnly functionality, as they serve different purposes:
- readOnly: Controls edit permissions and editor capabilities
- editorReady: Controls UI component rendering timing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of editorReady checks
rg "editorReady" -A 2
Length of output: 3586
Script:
#!/bin/bash
# Search for readOnly checks near editorReady
rg "readOnly.*editorReady|editorReady.*readOnly" -A 2
# Search for readOnly state handling in editor components
rg "readOnly" --type typescript -A 2 web/core/components/pages/editor/
Length of output: 161
Script:
#!/bin/bash
# Search for readOnly in editor components
rg "readOnly" web/core/components/pages/editor/ -A 2
# Also check for any editor state initialization patterns
ast-grep --pattern 'const editor = $_'
Length of output: 12424
web/core/components/pages/editor/summary/content-browser.tsx (1)
3-3: LGTM: Type definition update
Successfully simplified the editorRef type by removing EditorReadOnlyRefApi, which aligns with the PR objective of consolidating editor functionality.
Also applies to: 8-8
web/core/components/pages/editor/summary/popover.tsx (1)
12-12: LGTM! Verify PageContentBrowser's null handling
The removal of EditorReadOnlyRefApi aligns with the PR objective. The change simplifies the type to just EditorRefApi | null.
Let's verify that PageContentBrowser properly handles null editorRef:
✅ Verification successful
LGTM! PageContentBrowser handles null editorRef properly
The code properly handles the null case for editorRef through:
- Optional chaining (
?.) when callingonHeadingChangeandscrollSummary - Nullish coalescing (
??) withgetHeadings()defaulting to empty array - No direct property access without null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for null checks in PageContentBrowser
ast-grep --pattern 'editorRef?.getDocumentInfo()' web/core/components/pages/editor/summary/content-browser.tsx
Length of output: 112
Script:
#!/bin/bash
# Let's first check the content of PageContentBrowser to understand how editorRef is used
cat web/core/components/pages/editor/summary/content-browser.tsx
# As a backup, let's also search for any usage of editorRef with ripgrep
rg "editorRef" web/core/components/pages/editor/summary/content-browser.tsx
Length of output: 2297
web/core/components/pages/editor/header/info-popover.tsx (1)
10-10: LGTM! Proper null handling in place
The removal of EditorReadOnlyRefApi is consistent with the PR objective. The component already handles null editorRef safely through optional chaining.
packages/editor/src/core/extensions/drop.tsx (1)
16-33: LGTM! Proper editability check added
The addition of editor.isEditable check ensures images can only be pasted when the editor is editable, which aligns with removing readonly editor functionality.
packages/editor/src/core/hooks/use-collaborative-editor.ts (1)
18-18: LGTM! Clean integration of editable prop
The changes properly integrate the editable prop through the component hierarchy while maintaining the existing functionality.
Also applies to: 79-79, 102-104
web/core/hooks/use-collaborative-page-actions.tsx (1)
67-67: LGTM! Correct dependency array update
The addition of editorRef to the dependency array is correct and necessary for proper hook functionality.
web/core/components/pages/editor/page-root.tsx (2)
142-143: LGTM! Clean handler prop assignments
The simplified handler assignments improve code readability while maintaining the same functionality.
100-100: Verify version history functionality with single editor
The version history feature now relies solely on the main editor reference. Ensure this change doesn't affect version comparison functionality.
✅ Verification successful
Version history functionality remains intact with single editor
Based on the code analysis, the version history functionality is properly maintained after removing the readonly editor:
- The version comparison overlay (
PageVersionsOverlay) usescurrentVersionDescriptionfrom the main editor correctly - Version history state management (via URL params) and restoration flow are independent of the editor implementation
- The version history feature is accessible through the options dropdown and works with the current editor state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for version comparison related code
rg -A 5 "version.*comparison|compare.*version"
# Check for version-related components that might be affected
ast-grep --pattern 'class $_ {
$$$
version($$$) {
$$$
}
$$$
}'
Length of output: 125
Script:
#!/bin/bash
# Search for version-related functionality in the editor component
rg -A 5 "getDocument|setDocument|version" web/core/components/pages/editor/
# Check for version history related components and their usage
fd "version|history" web/core/components/ --type f --exec rg -l "editorRef|getDocument"
# Look for any tests related to version comparison
fd "test|spec" web/core/ --type f --exec rg -l "version.*history|document.*version"
Length of output: 5638
packages/editor/src/core/extensions/extensions.tsx (2)
150-152: LGTM! Proper mention handling based on editability
The conditional mention suggestions and readonly state are correctly configured based on the editor's editable state.
156-157: LGTM! Placeholder visibility control
Early return for placeholder when editor is not editable improves user experience.
web/core/components/pages/editor/header/options-dropdown.tsx (1)
18-18: LGTM! Simplified editor reference type
The removal of EditorReadOnlyRefApi aligns with the PR objective of removing the readonly editor functionality.
Also applies to: 33-33
web/core/components/pages/editor/editor-body.tsx (1)
43-44: LGTM: Props type refinement
The change to use Dispatch<SetStateAction> for state handlers improves type safety and better reflects their usage as state setters.
web/core/components/pages/modals/export-page-modal.tsx (1)
7-7: LGTM: Type simplification
The removal of EditorReadOnlyRefApi and simplification of the editorRef type is consistent with the unified editor approach.
Also applies to: 19-19
packages/editor/src/styles/editor.css (1)
114-119: LGTM: Enhanced checkbox interaction styles
The styles now properly handle checkbox interactions based on the editor's editable state:
- Hover effects only when editable
- Disabled pointer events when not editable
packages/editor/src/core/hooks/use-editor.ts (1)
83-118: Verify editor state preservation on editable changes
The editable prop in the dependency array will cause editor reinitialization when changed. This could lead to:
- Performance issues if
editablechanges frequently - Potential loss of user input or selection state
Consider:
- Using editor commands to update editable state instead of reinitializing
- Preserving and restoring editor state during reinitialization
✅ Verification successful
Let me analyze the search results:
-
The
ast-grepsearch shows that the editor commands are used for various operations like text selection, image insertion, code block handling, etc., but there's no direct command to update the editable state. -
The
rgsearch shows thateditableis primarily used as:
- A prop in React components for UI elements like filters and editors
- A database field attribute in Django migrations
- A CSS property for styling
Looking at the editor's usage in web/core/components/pages/editor/editor-body.tsx, the editable prop is passed directly to control the editor state:
editable={isContentEditable}Based on this analysis, I can generate the final response:
Editor reinitialization is the correct approach for editable state changes
The codebase shows that editable state is a fundamental prop that should trigger editor reinitialization because:
- There's no editor command to toggle editable state at runtime
- The editable state affects multiple extensions and core editor behavior
- The editor preserves selection state via
setSavedSelectioninonTransaction
The current implementation correctly handles state preservation and the performance impact is justified for this critical state change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for editor commands related to editable state
ast-grep --pattern 'editor.commands.$_($_)'
# Search for editable state changes in components
rg -A 3 'setEditable|editable.*=.*'
Length of output: 67674
Palanikannan1437
changed the title
3 participants
Description
Removing Read only Real time document editor while switching between lock/unlock/archive/unarchive to prevent breaking the socket connection.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
editableprop to various editor components, enhancing control over editability.CustomImageUploaderandDropHandlerExtensionbased on the editor's editability.useCollaborativePageActionshook.Bug Fixes
Refactor
CollaborativeDocumentReadOnlyEditorand its associated references, streamlining editor logic.PageEditorBodycomponent for a unified editing experience.Style
Documentation