[WIKI-585] refactor: editor core without props #7556
Conversation
WalkthroughRemoved variant-specific document conversion helpers and work-item embed extensions; consolidated document conversion into a single parser-kit/schema and switched callers to externalized conversion helpers. Updated imports and function signatures across server, web, and editor packages; deleted several extension files and simplified editor extension composition. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant EditorLib
Client->>Server: POST /convert-document { description_html }
Server->>EditorLib: getAllDocumentFormatsFromHTMLString({ descriptionHTML })
EditorLib-->>Server: { description, description_binary, description_html? }
Server-->>Client: JSON response with description and description_binary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/live/src/server.ts (1)
67-74: Consider removing unused variant parameter.The
variantparameter is still being destructured and validated but is no longer used in the conversion function call. Consider removing this parameter from the request validation to avoid confusion.- const { description_html, variant } = req.body as TConvertDocumentRequestBody; + const { description_html } = req.body as TConvertDocumentRequestBody; try { - if (description_html === undefined || variant === undefined) { + if (description_html === undefined) { res.status(400).send({ message: "Missing required fields", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/live/src/core/helpers/convert-document.ts(0 hunks)apps/live/src/core/helpers/page.ts(0 hunks)apps/live/src/core/lib/page.ts(3 hunks)apps/live/src/server.ts(2 hunks)apps/web/core/hooks/use-page-fallback.ts(2 hunks)packages/editor/src/ce/extensions/core/without-props.ts(0 hunks)packages/editor/src/ce/extensions/parser-kit.ts(1 hunks)packages/editor/src/core/components/editors/document/collaborative-editor.tsx(2 hunks)packages/editor/src/core/components/editors/document/editor.tsx(1 hunks)packages/editor/src/core/extensions/index.ts(0 hunks)packages/editor/src/core/extensions/parser-kit.ts(3 hunks)packages/editor/src/core/extensions/work-item-embed/extension-config.ts(0 hunks)packages/editor/src/core/extensions/work-item-embed/extension.tsx(0 hunks)packages/editor/src/core/extensions/work-item-embed/index.ts(0 hunks)packages/editor/src/core/helpers/editor-commands.ts(2 hunks)packages/editor/src/core/helpers/parser.ts(3 hunks)packages/editor/src/core/helpers/yjs-utils.ts(3 hunks)packages/editor/src/lib.ts(0 hunks)
💤 Files with no reviewable changes (8)
- packages/editor/src/lib.ts
- packages/editor/src/core/extensions/work-item-embed/index.ts
- apps/live/src/core/helpers/convert-document.ts
- packages/editor/src/core/extensions/work-item-embed/extension-config.ts
- packages/editor/src/core/extensions/index.ts
- apps/live/src/core/helpers/page.ts
- packages/editor/src/ce/extensions/core/without-props.ts
- packages/editor/src/core/extensions/work-item-embed/extension.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in the plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making i...
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/web/core/hooks/use-page-fallback.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (11)
packages/editor/src/core/helpers/editor-commands.ts (1)
2-2: LGTM! Improved type consistency with tiptap ecosystem.Using the official
Leveltype from@tiptap/extension-headinginstead of manually defining the union type (1 | 2 | 3 | 4 | 5 | 6) improves maintainability and ensures consistency with the tiptap extension ecosystem.Also applies to: 16-16
packages/editor/src/ce/extensions/parser-kit.ts (1)
1-4: LGTM! Good foundation for the unified parser kit approach.This establishes the base for consolidating editor extensions into a single parser kit, replacing the previous variant-specific approach. The empty array serves as an extensible foundation.
packages/editor/src/core/components/editors/document/editor.tsx (1)
10-10: LGTM! Clean removal of WorkItemEmbedExtension.The removal is consistent with the broader cleanup of the WorkItemEmbedExtension across the codebase. The extensions array construction is simplified while still passing
embedConfigtoDocumentEditorAdditionalExtensions, suggesting embed functionality may be handled elsewhere.Also applies to: 40-62
apps/web/core/hooks/use-page-fallback.ts (1)
3-3: LGTM! Improved function signature alignment with unified approach.The change from
getBinaryDataFromDocumentEditorHTMLStringtogetBinaryDataFromHTMLStringwith a structured object parameter aligns with the broader refactoring to consolidate variant-specific functions. The new signature is more extensible and consistent.Also applies to: 30-32
apps/live/src/server.ts (1)
12-12: LGTM! Good consolidation of conversion logic.The change to use
getAllDocumentFormatsFromHTMLStringfrom the external package consolidates the document conversion logic effectively. The structured parameter approach is more maintainable.Also applies to: 75-77
packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
21-21: No changes needed for undefined extensionsThe
useCollaborativeEditorhook already assigns a default value (extensions = []) when destructuring its props, so passingundefinedwon’t cause runtime errors.packages/editor/src/core/extensions/parser-kit.ts (1)
9-9: LGTM! Clean refactoring to parser kit namingThe renaming from
CoreEditorExtensionsWithoutPropstoPARSER_KITand the corresponding import updates align well with the PR objective of consolidating the editor core under a unified parser kit approach.Also applies to: 25-25, 101-101
apps/live/src/core/lib/page.ts (1)
22-24: Good refactoring to use named parametersThe migration to object parameters with named properties improves code readability and maintainability. The changes from positional arguments to
{ descriptionBinary }and{ descriptionHTML }make the API more explicit.Note that
getBinaryDataFromHTMLStringnow returns the binary data directly rather than wrapped in an object, which is a cleaner API design.Also applies to: 49-51
packages/editor/src/core/helpers/parser.ts (1)
9-9: Clean removal of variant-based logicThe refactoring successfully removes the
variantparameter and consolidates the conversion logic undergetAllDocumentFormatsFromHTMLString. This simplification aligns with the PR objective of unifying editor utilities.Also applies to: 67-67, 88-90
packages/editor/src/core/helpers/yjs-utils.ts (2)
7-10: Excellent consolidation to single parser schemaThe unification to a single
PARSER_KITandparserSchemaeliminates code duplication and simplifies the codebase. This is a significant improvement over the previous variant-based approach.
45-56: Well-structured API with consistent object parametersThe refactoring of all conversion functions to use object parameters with descriptive property names (
descriptionHTML,descriptionBinary) creates a consistent and maintainable API. The return types are also properly aligned withTDocumentPayload.Also applies to: 62-86, 92-109
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/live/src/server.ts (1)
66-77: Remove unusedvariantvalidation in/convert-documenthandler
In apps/live/src/server.ts (lines 66–77),variantis destructured and required by the request body type but never used ingetAllDocumentFormatsFromHTMLString, causing valid requests without it to return 400. Update the handler to only extract and validatedescription_html, for example:- this.router.post("/convert-document", (req: Request, res: Response) => { - const { description_html, variant } = req.body as TConvertDocumentRequestBody; + this.router.post("/convert-document", (req: Request, res: Response) => { + const { description_html } = req.body as Partial<TConvertDocumentRequestBody>; try { - if (description_html === undefined || variant === undefined) { - res.status(400).send({ message: "Missing required fields" }); + if (typeof description_html !== "string" || description_html.trim() === "") { + res.status(400).send({ message: "description_html is required" }); return; }
🧹 Nitpick comments (1)
apps/live/src/server.ts (1)
12-12: Fix import order to satisfy linter (external before alias/internal)Move
@plane/editor/libabove@/core/...imports.import helmet from "helmet"; +import { getAllDocumentFormatsFromHTMLString } from "@plane/editor/lib"; // hocuspocus server // helpers import { logger, manualLogger } from "@/core/helpers/logger.js"; import { getHocusPocusServer } from "@/core/hocuspocus-server.js"; -// types -import { TConvertDocumentRequestBody } from "@/core/types/common.js"; -import { getAllDocumentFormatsFromHTMLString } from "@plane/editor/lib"; +// types +import { TConvertDocumentRequestBody } from "@/core/types/common.js";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/live/src/server.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/live/src/server.ts (1)
packages/editor/src/core/helpers/yjs-utils.ts (1)
getAllDocumentFormatsFromHTMLString(92-109)
🪛 GitHub Check: Build and lint web apps
apps/live/src/server.ts
[warning] 12-12:
@plane/editor/lib import should occur before import of @/core/helpers/logger.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/live/src/server.ts (1)
75-77: Helper switch LGTMUsing
getAllDocumentFormatsFromHTMLString({ descriptionHTML })matches the new parser-kit API; returned fields align with response.
Description
This PR refactors the editor core without props by-
Type of Change
Summary by CodeRabbit
Refactor
Bug Fixes
Chores