[PE-193] feat: flat lists #6316
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the list functionality in the Plane editor. It adds a new flat list extension with advanced features like indentation, dedenting, splitting, and various list types (bullet, ordered, task, and toggle). The changes span multiple files, introducing new commands, utilities, and plugins to manage list interactions more effectively. The implementation focuses on improving the user experience by providing more intuitive list management and better handling of list-related operations. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant ListExtension
participant ListCommands
User->>Editor: Interact with list
Editor->>ListExtension: Detect list interaction
ListExtension->>ListCommands: Trigger appropriate command
alt Indent List
ListCommands->>Editor: Increase list item depth
else Dedent List
ListCommands->>Editor: Decrease list item depth
else Toggle List Type
ListCommands->>Editor: Change list type
else Split List
ListCommands->>Editor: Split current list item
end
ListExtension->>Editor: Update document state
Poem
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: 9
🧹 Nitpick comments (83)
packages/editor/src/core/extensions/flat-list/core/utils/is-node-selection.ts (1)
3-5: Consider a more robust implementation for type checking.While the current implementation works, it could be made more robust to prevent false positives. The
NodeSelectiontype from ProseMirror has additional properties that could be checked.Consider this alternative implementation that performs more thorough type checking:
-export function isNodeSelection(selection: Selection): selection is NodeSelection { - return Boolean((selection as NodeSelection).node); +export function isNodeSelection(selection: Selection): selection is NodeSelection { + if (!selection) return false; + // Check multiple properties that are specific to NodeSelection + return ( + 'node' in selection && + 'from' in selection && + 'to' in selection && + selection.from === selection.to - 1 + ); +}This implementation:
- Safely handles null/undefined inputs
- Uses
inoperator for type narrowing instead of type assertion- Checks additional properties specific to NodeSelection
- Validates the selection range (NodeSelection always spans exactly one node)
packages/editor/src/core/extensions/table/table/table.ts (1)
119-119: Remove debug console statement.
Leaving aconsole.login production code can clutter logs and potentially leak implementation details. Recommend removing or using a more controlled logging approach if needed for debugging.- console.log("ran");packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts (3)
14-26: Minor duplication betweendiv[data-list-kind]anddiv[data-list].
Both rules exhibit near-identical logic for extracting attributes. You could unify them if you want to minimize repetition. For example, factor out a helper function to extract these attributes, then call it in each rule.+function extractListAttributes(element: Element, defaultKind: ListKind = "bullet"): ListAttributes { + return { + kind: (element.getAttribute("data-list-kind") || defaultKind) as ListKind, + order: parseInteger(element.getAttribute("data-list-order")), + checked: element.hasAttribute("data-list-checked"), + collapsed: element.hasAttribute("data-list-collapsed"), + }; +} /* Then in the existing rules: */ -{ - tag: "div[data-list-kind]", - getAttrs: (element): ListAttributes => { - ... - return { ... }; - }, -}, +{ + tag: "div[data-list-kind]", + getAttrs: (element): ListAttributes => + typeof element === "string" ? {} : extractListAttributes(element), +},
44-93: Cautious iteration over child nodes.
The loop at lines 49–54 checks up to three nested child nodes before deciding if we have a checkbox input. This can be brittle if an unexpected DOM structure or additional nesting is introduced. Consider a more robust traversal or a single function that safely finds the<input>element.-for (let i = 0; i < 3 && checkbox; i++) { - if (["INPUT", "UL", "OL", "LI"].includes(checkbox.nodeName)) { - break; - } - checkbox = checkbox.firstChild as HTMLElement | null; -} +function findCheckboxInput(root: HTMLElement | null, maxDepth = 3): HTMLInputElement | null { + let current = root; + let depth = 0; + while (current && depth < maxDepth) { + if (current.nodeName === "INPUT" && current.getAttribute("type") === "checkbox") { + return current as HTMLInputElement; + } + if (["UL", "OL", "LI"].includes(current.nodeName)) { + break; + } + current = current.firstChild as HTMLElement | null; + depth++; + } + return null; +}
77-86: Regex-based task detection.
Stripping the"[x]"or"[ ]"tokens from text content is convenient, but it could misfire if text is localized or if the bracket pattern appears incidentally. If your use case expands or if i18n is introduced, consider a more structured approach to identify tasks.packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts (1)
10-13: Consider adding boundary checks forstartIndexandendIndex.
If the provided indices exceed the content bounds, this function might generate unexpected results or errors. As a good-to-have, add checks to fail gracefully or return an empty string if the range is bad.Here's a sample snippet that returns an empty string if the indices are out of range:
export function rangeToString(range: NodeRange): string { const { parent, startIndex, endIndex } = range; + if (startIndex < 0 || endIndex > parent.content.size || startIndex >= endIndex) { + return ""; + } return cutByIndex(parent.content, startIndex, endIndex).toString(); }packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts (2)
69-79: Clarify theindex === 1condition.
It’s not immediately obvious why splitting is targeted only whenindex === 1. Consider documenting the rationale behind this condition.
81-101: Consider multiple passes for comprehensive fixes.
In complex documents, a single pass of joins and splits may not fix all list inconsistencies. You could repeatedly callfixListuntil no further changes apply or combine it into one iterative loop.packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts (1)
61-61: Rewrite assignment in loop condition for clarity.
This code snippet uses assignment within thewhilecondition, which can be confusing. Refactor the pattern to avoid multiple side effects in a single expression.-while (((next = child.nextElementSibling), next?.tagName === child.tagName)) { - child.append(...Array.from(next.children)); - next.remove(); -} +let next = child.nextElementSibling; +while (next && next.tagName === child.tagName) { + child.append(...Array.from(next.children)); + next.remove(); + next = child.nextElementSibling; +}🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts (1)
70-93: Optional fallback for missingattrs.
InisTargetListandisTargetListsRange, the code checks(node.attrs as ListAttributes). Ifattrsis unexpectedly undefined, type coercion might fail. Adding a default or a safe check can make the code more robust.- return (node.attrs as ListAttributes).kind === kind + return (node.attrs as ListAttributes)?.kind === kindpackages/editor/src/core/extensions/flat-list/core/migrate.ts (1)
3-44: Consider logging or handling unrecognized list types.
Currently, only known list types (bullet_list,ordered_list,task_list, etc.) are explicitly handled. Any unknown or custom list types are simply passed through. Logging or handling them might make migrations more transparent.packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts (3)
30-36: Check for invalid block ranges.The early return on lines 34-36 is a good safeguard if no valid block range is found. Consider adding an inline comment to clarify why we bail out at this point, especially for future maintainers unfamiliar with ProseMirror’s range mechanics.
57-63: Optimize multiple passes.When iterating from the end index downward, each child is handled individually (list or non-list). For large selections, consider grouping consecutive nodes in a single pass to minimize transformations and possibly enhance performance.
93-101: Early exit for inline checks.
rangeAllowInlineContentscans all child nodes in the range. If performance becomes a concern with very large blocks, consider returning promptly upon finding the first node with inline content.packages/editor/src/core/extensions/flat-list/heading-list-extension.ts (1)
50-97: Graceful error handling in commands.Wrapping the creation logic in a
try/catchwithincreateHeadedListis helpful. Consider providing user-facing feedback (e.g., toast notifications) rather than only logging to the console, especially if errors might be triggered by user actions within an editor UI.packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts (2)
105-130: Check the split index boundaries.
splitPosis derived fromsplitIndexat line 112. Particularly in edge cases (e.g.,splitIndexis 0 or equal to endIndex), confirm you’re not accidentally referencing invalid positions when creatingrange1orrange2.Shall I write a test snippet for boundary conditions to verify correctness?
135-185: Consider a fallback if merging fails.When moving the selected nodes into the previous list node fails or is invalid (e.g., mismatched node types, or the parent is locked), the code returns
false. Consider adding a fallback approach to wrap or re-insert the nodes to maintain a valid document structure. This can help avoid dropping user content inadvertently.packages/editor/src/core/extensions/extensions.tsx (4)
60-62: Review disabled StarterKit list features.
bulletList,orderedList, andlistItemare set tofalse. Ensure that all commands reliant on these default configurations are replaced or supplemented by the newly added list extensions (BulletList, OrderedList, ListItem, and FlatListExtension).
85-93: Remove redundant input rules if not using them.
BulletList.extend(...)has an emptyaddInputRules()method returning no rules. If these additions are unneeded, consider removing or documenting their future purpose.
94-102: Remove redundant input rules in OrderedList.Similar to BulletList, the
addInputRules()method is returning an empty array. Consider removing or replacing placeholders with meaningful list-related rules.
112-120: Check TaskList’s space-y-2 spacing.
TaskListis configured with specificHTMLAttributes. Ensure consistent spacing with other list types and confirm that tasks remain distinctly styled from bullet/ordered lists.packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts (4)
5-16: Use consistent naming for utility imports.
withAutoFixListand the block-boundary utilities are introduced here. Confirm these match naming conventions in other modules likesplit-list.tsto avoid confusion.
22-36: Clarify usage of DedentListOptions.
from?: numberandto?: numberare optional. Document the typical scenario for dedenting partial ranges vs. entire selection to help future maintainers.
150-157: Review safeLiftRange pre-processing.
moveRangeSiblingsmodifies the transaction, then updates$fromand$to. Check for race conditions if other code modifies the transaction concurrently.
159-209: Move sibling logic correctness.The code merges trailing siblings into the last child. Ensure that empty or partially empty nodes aren’t accidentally merged.
packages/editor/src/core/hooks/use-editor.ts (1)
116-145: Avoid losing user selection mid-migration.Your code attempts to preserve
savedSelection. Edge cases (e.g., selection outside doc size) might cause unexpected scroll or selection states. Consider fallback logic if the old selection is invalid.packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts (3)
239-239: Use optional chaining for safer property access.Per static analysis, you can simplify
node && node.type.spec.disableDropCursorusing optional chaining.- const disableDropCursor = node && node.type.spec.disableDropCursor; + const disableDropCursor = node?.type?.spec?.disableDropCursor;🧰 Tools
🪛 Biome (1.9.4)
[error] 239-239: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
254-254: Use optional chaining forthis.editorView.dragging?.slice.Eliminates the double check
if (this.editorView.dragging && this.editorView.dragging.slice).- if (this.editorView.dragging && this.editorView.dragging.slice) { + if (this.editorView.dragging?.slice) {🧰 Tools
🪛 Biome (1.9.4)
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
106-113: Document the disableDropCursor NodeSpec feature.Developers may overlook or misuse the
disableDropCursorproperty. Provide inline doc comments or examples on how to override it for specific nodes.packages/editor/src/core/extensions/drop-cursor-working.ts (4)
47-48: Consider removing debug logs.You have
console.log("aaya")in production code. To avoid cluttering the console and potentially revealing internal implementation details, consider removing or wrapping it with a debug flag or logging utility.
119-119: Avoid usingthis as anyif possible.Casting
thistoanycan bypass type safety and lead to runtime issues. Consider giving the class a clear type for the event handlers to avoid losing type information.
222-222: Use optional chaining for clarity.Static analysis suggested using an optional chain. If
listElementmight be null, consider usinglistElement?.nextElementSiblingor an equivalent pattern for more concise and robust code.🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
234-239: Remove leftover debug logs.The
console.log("asfd")andconsole.log("asdff")calls seem to be leftover debug logs. Removing them or using a proper logger is preferable in production code.packages/editor/src/core/extensions/drop-cursor-weird.ts (4)
48-48: Remove console log in production code.The
console.log("aaya")statement might unintentionally leak debug information in production. Replacing it with a logging utility or removing it can help keep the console clean.
54-74: Consider more robust error handling for drop events.Currently, if
posis missing or invalid, you simply returnfalse. Consider providing fallback logic or error handling for exceptional cases to prevent partial or inconsistent updates to the editor state.
212-212: Optional chaining suggestion.Static analysis recommends using optional chaining for safer property access in
node?.type?.spec.disableDropCursor. This reduces repetitive null checks and ensures consistent behavior ifnodeis null or undefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
226-226: Evaluate optional chaining usage here.As with line 212, consider
this.editorView.dragging?.sliceto shorten your logic and handle undefineddraggingstates gracefully.🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts (3)
48-48: Avoid production console logging.Using
console.log("asdff")in production can clutter the logs. Remove or wrap this statement for debugging only.
58-58: Remove extraneous debug statements.The
console.log("asdff")line is similarly redundant. Prefer a logging framework or remove debug output for production readiness.
265-295: Check optional chain feasibility.Static analysis suggests optional chaining for lines like
this.editorView.dragging?.slice. Confirm that references are resilient to null or undefined to avoid potential runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: expected
,but instead found:Remove :
(parse)
[error] 268-268: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 273-273: expected
,but instead found:Remove :
(parse)
[error] 273-273: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 281-281: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 268-271: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
packages/editor/src/core/extensions/drop-cursor.ts (2)
266-266: Consider optional chaining.For improved readability and safety, using optional chaining can make the code more concise where
nodeornode.typemight be null. For example,node?.type?.spec.disableDropCursor.🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
272-272: Optional chaining forthis.editorView.dragging.Consistently apply optional chaining to
this.editorView.dragging?.sliceto avoid potential null dereferences and reduce nested conditionals.🧰 Tools
🪛 Biome (1.9.4)
[error] 272-272: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor.optimized.ts (2)
271-271: Optional chaining in property access.Use
node?.type?.spec.disableDropCursorto handle the case wherenodeornode.typemight be undefined. This prevents possible runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
277-277: Optional chaining onthis.editorView.dragging?.slice.Following the same rationale, optional chaining can help clarify null vs. defined states without redundant branching logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 277-277: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/flat-list/core/utils/cut-by-index.ts (1)
1-6: Caution with internal API usage.
Relying onfragment.cutByIndexmay break if it changes in future Tiptap releases. Keep an eye on release notes or consider an alternative approach if available.packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts (1)
5-7: Guard against no-op patches.If
patchmakes no changes to the transaction, the flow still re-dispatches the original transaction. This is correct behavior, but it might be helpful to explicitly check whether any meaningful updates were made, especially if performance or clarity is a concern.packages/editor/src/core/extensions/flat-list/core/commands/enter-without-lift.ts (1)
1-10: Consider adding an optionalliftEmptyBlockstep.Currently,
liftEmptyBlockis omitted by design. If there's a scenario where partial or conditional lifting is beneficial, consider implementing a custom command or an optional parameter. This ensures flexibility without straying from the core vision of “enter without lift.”packages/editor/src/core/extensions/flat-list/core/utils/map-pos.ts (1)
3-16: Avoid mutating theposvariable externally to the closure.
mapPosmutatesposwithin thegetPosfunction, which is fine for an enclosed closure. However, keep in mind that multiple calls togetPosre-map the samepos. If you need independent position tracking, clone or store each position in a separate variable to prevent unintended side effects.packages/editor/src/core/extensions/flat-list/core/utils/browser.ts (2)
4-4: Optional chaining recommendation.To enhance clarity and reduce logical complexity, consider using optional chaining and nullish coalescing:
-const agent = (nav && nav.userAgent) || ""; +const agent = nav?.userAgent ?? "";🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
12-12: Straightforward Safari detection.The check accurately identifies Safari by vendor while excluding IE/Edge. Keep in mind that browser detection can be fragile over time; consider feature detection where feasible.
packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-end.ts (1)
6-11: Implementation logic is sound.Like
atTextblockStart, this function correctly checks if the cursor is at the end of the text block. Consider adding tests for boundary conditions (e.g., empty paragraphs or multi-node paragraphs) to ensure robust coverage.packages/editor/src/core/extensions/flat-list/core/types.ts (1)
1-2: Consider narrowingkindinListAttributestoListKind.
Currently,ListAttributesdefines thekindproperty as a genericstring, whereasListKindis a union of specific string literals. Aligning them would help prevent inconsistencies between supported list kinds and assigned attribute values.-export interface ListAttributes { - kind?: string; +export interface ListAttributes { + kind?: ListKind;packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts (1)
14-20: Comprehensive coverage of lifting operations.
safeLiftFromToensures only valid block ranges are lifted. Consider expanding error handling or logging to capture scenarios where a lift is not possible.packages/editor/src/core/extensions/flat-list/core/plugins/index.ts (1)
9-29: Comprehensive plugin array creation.
ThecreateListPluginsfunction neatly aggregates related plugins, making it easy to extend or selectively include them. The JSDoc is informative.Consider providing a config object letting consumers selectively enable or disable specific plugins.
packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts (1)
3-3: Consider caching repeated checks.
isCollapsedListNodemay be invoked repeatedly for each node withinnodesBetween. Though this is likely negligible, you could cache or otherwise reduce repeated calls if performance becomes critical.packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts (1)
24-33: Improve clarity of step size check.
step.slice.size >= afterPos - beforePoscan be more readable if separated into simpler or named checks to clarify the purpose of the comparison.packages/editor/src/core/extensions/flat-list/core/node-view.ts (1)
27-34: Update logic might skip subtle style changes.
Theupdatefunction checkssameMarkup, but if the node has changed style-based attributes that do not affect markup directly, you may need a more granular check to handle them.packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts (1)
20-24: Clarify use of group naming.
flatListGroupis descriptive but ensure it doesn't conflict with other list groups or naming inside the schema.packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts (1)
30-53: Consider clarifying toggle logic in comments.
While the command usage is mostly self-explanatory, it could be beneficial to include a brief inline comment or docstring highlighting how thecollapsedparameter overrides the toggling behavior.+/** + * If `collapsed` is provided, it sets that value. Otherwise, it toggles + * the current `collapsed` attribute. + */ export function createToggleCollapsedCommand({ collapsed = undefined, ...packages/editor/src/core/extensions/flat-list/core/dom-events.ts (2)
10-30: Good pattern for custom event handling.
Preventing default behavior and translating the event to a ProseMirror position is a solid approach. Watch for potential offset inaccuracies(-10, -10)across different browsers.-const pos = view.posAtDOM(target, -10, -10); +// Consider verifying the values or making them configurable +const pos = view.posAtDOM(target, -10, -10);
32-50: Refactor the early-return path for clarity.
Consider simplifying the block that checksif (!isListNode(list)) return false;for readability, especially if more conditions are added later.packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts (1)
53-70:setVisibleSelectionensures user clarity.
Expanding the collapsed node on selection is a user-friendly approach. Consider if this action should be optional or dependent on user preference.packages/editor/src/core/extensions/flat-list/core/index.ts (2)
10-19: Consider adding consistent naming patternsFor clarity and discoverability, ensure that your command factory functions follow consistent naming conventions (e.g.,
createSomethingCommand). While it is consistent in many places, consider confirming that all newly introduced commands strictly follow this pattern to help new contributors find them quickly.
20-38: Comprehensive list utility exportsExporting these plugs and event handlers (e.g.,
createListPlugins,createListEventPlugin, etc.) in one place greatly simplifies the import process. However, consider documenting in the code or a README which plugins should typically be used together, to assist users who might be uncertain about how to combine them for common functionality.packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts (2)
49-60: Potential for code duplication
deleteCommandis very similar tobackspaceCommand. If the logic between the two grows further, you might consider refactoring common steps into a shared utility, differentiating them only where behavior actually diverges.
62-83: Helpful keymap objectCentralizing keybindings in
listKeymapis a good design pattern. This consolidates the user experience in one place, making future modifications more straightforward. Consider adding support for platform-specific shortcuts (e.g., Mac vs. Windows) if your editor usage spans multiple systems, or at least comment about potential differences.packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts (1)
76-88:defaultMarkerGetteris straightforwardThe function effectively handles task and toggle list markers. If you extend more list types in the future, consider a fallback approach (e.g., a standard bullet). Currently, returning
nullor an empty array is a reasonable approach but might need handling if additional list kinds arise.packages/editor/src/core/extensions/read-only-extensions.tsx (1)
142-142: Appending the extension at the endAdding
FlatListExtensionto the read-only editor extensions ensures it’s available as part of the final returned array, preserving the original order of other extensions. If the initialization order for extensions could matter, consider providing a quick comment or docs on why it appears last.packages/editor/src/core/extensions/side-menu.tsx (2)
125-128: Ensure offset constants are well-documented and validated.Here, the logic modifies
rect.leftandrect.topconditionally for.prosemirror-flat-list. While this helps tweak the side menu positioning, consider defining constants or enumerations for these numeric offsets (e.g., 5, 6) to improve maintainability, reduce magic numbers, and easily adjust them as needed.
134-136: Consider merging duplicate offset logic across branches.Lines 134-136 also apply offsets if
node.classList.contains("prosemirror-flat-list"). This mirrors the logic in lines 125-128 (though with slightly different offset values). Consider consolidating these conditions into a shared helper or applying them in a single block (if feasible) to avoid inconsistencies and duplication.packages/editor/src/core/components/menus/menu-items.ts (4)
25-25: Consider consistent naming for imported icons.
ListCollapseis imported here among many icons. The name is meaningful, but ensure that the naming stays consistent with the rest of the list-related icons (ListIcon,ListOrderedIcon) so it's immediately recognizable during refactoring or UI changes.
179-185: New ToggleListItem introduced.This new menu item for toggle lists is coherent with the bullet and ordered lists approach. Make sure to double-check styling or UI states so that toggling appears consistent with other list types.
186-186: Ensure spacing or separation after new list items.Having an additional blank line (line 186) can be removed or clarified. If it’s intentional for code readability, that’s fine; if not, consider removing to keep consistent formatting in the file.
270-270: Menu ordering for ToggleListItem.Placing
ToggleListItemnearBulletListItemandNumberedListItemmight boost discoverability for users. Consider reordering if a logical grouping of list items helps the UI/UX.packages/editor/src/core/helpers/editor-commands.ts (5)
66-66: Check for consistency with surrounding functions.Line 66 is empty; remove if not needed or keep if it logically separates functions for clarity.
79-79: Maintain consistent spacing or remove redundant line.Similar to line 66, consider removing if not necessary for readability.
92-92: Consider removing redundant blank line.If this spacing is not purposeful, removing it would maintain consistency.
105-105: Consider grouping blank lines consistently.
Similar spacing feedback as above lines.
177-178: Removal of clearNodes() before table insertion is beneficial for preserving user content.The revised approach avoids unnecessarily clearing nodes. Ensure that this doesn’t cause unexpected interactions if the user tries inserting a table inside existing container nodes. Possibly add tests for partial table insertions to confirm correct behavior.
packages/editor/src/core/extensions/typography/index.ts (1)
83-85: Remove or document deprecated rule.
Commenting outraquosuggests it is deprecated or unused. Consider removing it altogether if no longer needed, or provide a comment explaining any future re-enablement.packages/editor/src/core/extensions/flat-list/core/style.css (3)
7-7: Remove duplicate margin-bottom declaration.The
margin-bottomproperty is declared twice (lines 5 and 7).margin-top: 0; margin-bottom: 0; margin-left: 32px; - margin-bottom: 0; position: relative;
17-60: Well-implemented ordered list counter management.The implementation handles ordered list counters effectively:
- Uses
contain: styleto prevent counter leakage- Supports custom order numbers with feature detection
- Handles Safari compatibility for older versions
Consider adding a CSS custom property for the counter name (e.g.,
--list-counter-name) to make it more maintainable and reusable across different list types.
102-104: Consider transition for collapsed state.Adding a transition effect when collapsing/expanding toggle lists would improve the user experience.
&[data-list-collapsable][data-list-collapsed] > .list-content > *:nth-child(n + 2) { display: none; + transition: height 0.2s ease-out; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (82)
packages/editor/package.json(2 hunks)packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx(2 hunks)packages/editor/src/core/components/menus/menu-items.ts(4 hunks)packages/editor/src/core/extensions/custom-list-keymap/list-keymap.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-huh.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-weird.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-working.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor.optimized.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/enter-without-lift.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-collapsed-backward.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/move-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/toggle-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/dom-events.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/input-rule.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/migrate.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/node-view.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/clipboard.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/event.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/rendering.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/safari-workaround.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/style.css(1 hunks)packages/editor/src/core/extensions/flat-list/core/types.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-end.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-start.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/block-boundary.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/browser.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/create-and-fill.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/cut-by-index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/get-list-type.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/in-collapsed-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-block-node-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-collapsed-list-node.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-list-node.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-list-type.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-node-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-text-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/list-range.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/map-pos.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/max-open.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/parse-integer.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/set-list-attributes.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/set-node-attributes.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/unwrap-list-slice.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/zoom-in-range.ts(1 hunks)packages/editor/src/core/extensions/flat-list/heading-list-extension.ts(1 hunks)packages/editor/src/core/extensions/flat-list/list-extension.ts(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(2 hunks)packages/editor/src/core/extensions/side-menu.tsx(1 hunks)packages/editor/src/core/extensions/table/table/table.ts(1 hunks)packages/editor/src/core/extensions/typography/index.ts(1 hunks)packages/editor/src/core/helpers/editor-commands.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)packages/editor/src/core/plugins/drag-handle.ts(2 hunks)packages/editor/src/core/types/editor.ts(1 hunks)packages/editor/src/index.ts(1 hunks)packages/editor/src/styles/drag-drop.css(1 hunks)packages/editor/src/styles/editor.css(1 hunks)
⛔ Files not processed due to max files limit (2)
- packages/editor/src/styles/lists.css
- web/core/constants/editor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/flat-list/core/utils/browser.ts
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-weird.ts
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-working.ts
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor.ts
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 272-272: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts
[error] 189-189: expected , but instead found :
Remove :
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 196-196: expected , but instead found :
Remove :
(parse)
[error] 196-196: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 207-207: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 268-268: expected , but instead found :
Remove :
(parse)
[error] 268-268: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 273-273: expected , but instead found :
Remove :
(parse)
[error] 273-273: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 305-305: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 309-309: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 313-313: expected , but instead found :
Remove :
(parse)
[error] 313-313: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 320-320: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 320-320: expected , but instead found :
Remove :
(parse)
[error] 320-320: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 326-326: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 326-326: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 330-331: expected a semicolon to end the class property, but found none
(parse)
[error] 331-331: expected a semicolon to end the class property, but found none
(parse)
[error] 345-346: expected a semicolon to end the class property, but found none
(parse)
[error] 355-356: expected a semicolon to end the class property, but found none
(parse)
[error] 364-364: expected } but instead the file ends
the file ends here
(parse)
[error] 281-281: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 355-356: Duplicate class member name "function"
(lint/suspicious/noDuplicateClassMembers)
[error] 189-194: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 196-205: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 268-271: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 305-307: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 309-311: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 326-328: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts
[error] 18-18: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 20-20: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 27-27: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 29-29: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/editor/src/core/extensions/drop-cursor.optimized.ts
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 277-277: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts
[error] 31-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 32-32: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 33-33: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts
[error] 239-239: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (134)
packages/editor/src/core/extensions/table/table/table.ts (4)
114-114: Good insertion node creation.
ThecreateTableutility usage is straightforward and aligns well with the schema. No issues observed here.
116-117: Check insertion position correctness.
Calculatingpositionusingselection.$from.before(selection.$from.depth)is typically correct for many scenarios. However, ensure that if the selection is nested (e.g., within a list), this logic still places the table where intended.
126-127: Insertion of the table is appropriate.
Inserting the created table at the computed position is consistent with typical ProseMirror patterns. No immediate concerns.
129-135: Validate cursor positioning after table insertion.
Setting the selection to(cellPos + 1)may work for basic scenarios, but ensure thorough testing for correct cursor placement in different editor states (e.g., nested nodes, multiple tables).packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts (2)
1-2: Imports look good.
Everything here is necessary for defining and exporting the parse rules for lists, with no apparent issues.
109-119: Hard-coding nested lists tobulletkind.
By defaulting nested lists to bullet, you risk losing the original context if the source was anolor a toggle list. Verify that this behavior is desired for converting Dropbox Paper invalid HTML. Otherwise, consider allowing a more graceful fallback that inspects the node and adjusts accordingly.✅ Verification successful
Let me gather more information about the list handling in the codebase.
Let me gather one final piece of information to understand how list kinds are handled in the codebase.
Defaulting nested lists to
bulletis intentional for Dropbox Paper compatibilityThe code explicitly handles invalid HTML structure from Dropbox Paper where nested lists can appear directly under
ulorolelements. The decision to default these to bullet lists appears to be a deliberate choice for compatibility, as:
- The comment in the code clearly states this is specifically for handling Dropbox Paper's invalid HTML structure
- The codebase has proper handling for other list types (ordered, task, toggle) in valid HTML scenarios
- There's no evidence of lost functionality, as regular nested lists are handled through proper DOM parsing rules elsewhere in the codebase
The current implementation provides a consistent way to handle this edge case while maintaining compatibility with content pasted from Dropbox Paper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for places that transform nested lists to bullet # to confirm it's the only desired behavior across the codebase. rg -A 4 'nested lists'Length of output: 973
Script:
#!/bin/bash # Check for list-related code and attributes ast-grep --pattern 'kind: "bullet"' # Look for other list parsing or transformation logic rg -A 4 'ListAttributes' # Check for Dropbox Paper related code rg -B 2 -A 2 'dropbox|paper'Length of output: 32055
Script:
#!/bin/bash # Check how list kinds are determined from HTML elements ast-grep --pattern 'tag: ":is(ul, ol)"' # Look for list kind determination logic rg -A 4 'kind: "ordered"'Length of output: 3497
packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts (3)
1-2: Imports look good here.
The imports correctly reference the necessary types from@tiptap/pm/model.
3-4: EnsurecutByIndexhas the required test coverage.
SincerangeToStringdepends oncutByIndex, please verify thatcutByIndexcorrectly handles out-of-bounds indices and other edge cases.
5-9: Documentation is clear and concise.
Your JSDoc comment is well structured, providing a clear explanation of the utility’s purpose and internal scope.packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts (4)
9-26: Potential infinite loop in generator.
This generator runs in an infinitewhile (true)loop, which may lead to unintended repeated yields if called more times than expected. Confirm that consumers of this generator are designed to iterate a limited number of times and handle any leftover mapping operations.Would you like me to generate a script to scan for all call sites of
getTransactionRangesto ensure it’s used in a controlled manner?
29-63: Validate descending boundary sort logic.
The boundaries are sorted in descending order to ensure correct join application from the bottom up. Make sure this order is intentional and verified by tests.
65-67: List join condition looks appropriate.
UsingisListNodechecks ensures that only adjacent list nodes (plus the child) are joined.
103-104: Integration withpatchCommandis well-structured.
Good use of a wrapper to seamlessly integrate the auto-fix functionality into the command ecosystem.packages/editor/src/core/extensions/custom-list-keymap/list-keymap.ts (2)
46-46: Validate default Tab behavior outside List context.
Returningfalsehere allows Tab to bubble up, enabling standard focus navigation when the editor is not in alistItemortaskItem. Verify that this behavior is consistent with the desired user experience, particularly if the rest of your app relies on Tab for a different workflow.
54-54: Confirm Shift-Tab logic aligns with desired user experience.
Similarly, returningfalseforShift-Taboutside list contexts will permit the browser to handle backward focus traversal. Ensure this is intended and doesn't conflict with handling indentation/unindentation in other parts of the code.packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts (1)
29-62: Verify correctness for partial or empty list selections.
The main unwrapping logic checks for valid node or range selections but might behave unexpectedly if the selected list node has no content or is partially formed. Consider adding tests to ensure stability in edge cases.Would you like assistance generating a suite of edge case unit tests for this scenario?
packages/editor/src/core/extensions/flat-list/core/commands/move-list.ts (1)
28-86: Ensure thorough testing of boundary conditions.
The logic for moving list items up and down is complex and index-based. Confirm that boundary indexes (startIndex = 0orendIndex = parent.childCount) are handled gracefully.Automated tests covering corner cases can prevent off-by-one or out-of-bounds errors. Would you like a shell script to scan for existing tests related to list movement?
packages/editor/src/core/extensions/flat-list/core/migrate.ts (1)
72-82: Add robust tests for list migrations.
While the recursive approach covers bullet, ordered, and task lists, confirm it behaves correctly for nested or partial lists, and for edge cases like empty nodes.I can help create a script to search for existing migration tests or generate new test coverage for these scenarios if desired.
packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts (1)
37-45: Verify deeper parent node logic.By shifting the
range.depthby one level if the parent is a list node, the command ensures wrapping at the correct level. However, consider verifying corner cases (e.g., wrapping nested lists or deeply nested nodes) to confirm no unexpected range adjustments occur.packages/editor/src/core/extensions/flat-list/core/input-rule.ts (2)
38-59: Confirm match index usability.When retrieving old attributes and computing
newAttrs, ensure the index usage onmatchremains valid in all cases (e.g., empty or partial regex matches). An out-of-bounds reference can lead to undefined behavior.
82-103: Avoid conflicting input rules.The predefined
listInputRulesmight compete with other custom or default Tiptap input rules (e.g., smart quotes, code block triggers). Double-check that your patterns do not conflict with existing input rules, especially if multiple rules share overlapping conditions.packages/editor/src/core/extensions/flat-list/list-extension.ts (3)
50-77: Commands naming consistency.The commands
createList,indentList,dedentList, andsplitListconsistently map to the underlyingcreateWrapInListCommand,createIndentListCommand, etc. This is clean and aligns nicely with your extension's aim. Good practice to keep them in a single place for clarity.
78-107: Potential keymapping collisions.The shortcut handlers (
Tab,Shift-Tab,Enter) override default behaviors if the list extension is active. Ensure that this doesn't inadvertently remove necessary functionality for other node types or cause collisions with higher-level keymaps. Possibly document or confirm other node types won't need these keys.
108-111: Composing plugins with caution.The array returned by
addProseMirrorPluginsmerges your custom plugin set withcreateListPlugins(...),listKeymapPlugin, andlistInputRulePlugin. This approach is excellent. Just watch out for potential plugin ordering pitfalls if other third-party plugins rely on matching or transforming list nodes.packages/editor/src/core/extensions/flat-list/heading-list-extension.ts (1)
99-148: Keybinding overrides.Your heading list extension uses a similar approach to the list extension to override
Tab,Shift-Tab,Enter, etc. If the heading list node is active, standard text indentation or new line behavior is replaced. Validate that these overrides do not block normal heading operations or accessibility-based shortcuts.packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts (1)
57-63: Return consistency after indentation.When
indentRangesucceeds on line 58, you dispatch and return early. This is good, but ensure the calling command also handles any partial indentation. In more complex scenarios (e.g., partial success near the edges), a direct single return could miss further indentation steps. Confirm that partial ranges are correctly updated before returning.packages/editor/src/core/extensions/extensions.tsx (6)
6-11: Verify consistent naming across list-related imports.All list extensions (ListItem, BulletList, OrderedList) have consistent naming and are properly imported. No issues detected here.
40-43: Ensure separation of concerns for DropCursorExtension and FlatListExtension.The newly imported
DropCursorExtensionandFlatListExtensionare related but serve different purposes. Confirm they don’t overlap in functionality or create conflicts when used together, especially in drag/drop scenarios involving new flat list features.
77-80: Assess disabling the dropcursor in StarterKit.Previously, dropcursor was customized via CSS classes. Now it’s disabled (
dropcursor: false) here. Verify whether the newDropCursorExtensionfully replaces that functionality to avoid conflicting or redundant dropcursor behaviors.
83-84: Validate the combined usage of DropCursorExtension and FlatListExtension.You’re integrating both extensions into the same editor. Double-check collisions in event handling, especially with list-specific drag/drop gestures in
FlatListExtension.
121-133: Review nested TaskItem usage.
TaskItemis configured withnested: true. Verify the editor’s behavior for deeply nested tasks (e.g., a TaskItem within a TaskItem) and whether it’s intended or can lead to undesired complexities.
207-212: Check Markdown integration for new list extensions.Markdown extension is enabled with
html: true. Verify that bullet/ordered/task lists parse correctly from and into Markdown, especially after adopting the new “flat list” approach.packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts (4)
22-24: Confirm correct chaining of the split commands.
createSplitListCommandcomposessplitBlockNodeSelectionInListCommandandsplitListCommandwithwithAutoFixList. Ensure no edge cases where both commands might conflict or produce unintended splits.
31-65: Validate behavior of splitBlockNodeSelectionInListCommand.The command only applies if:
- The selection is a block node selection
- The parent is a list
- The parent has exactly one child
- That single child is not a list node
Double-check corner cases, e.g., when a list has only a single nested child but that child also contains nested content.
67-130: Ensure logical splitting rules for nested lists.
splitListCommanddedents or splits based on cursor position (first child vs. subsequent child). Validate that these heuristics operate correctly for advanced nesting scenarios.
132-185: Accurately handle new list creation within doSplitList.
doSplitListdetermines whether to create an additional block type or a new list node. Review correctness for boundary conditions (split at start/end of a list) and howattrs.collapsedinfluences it.packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts (5)
1-4: Confirm safety of ReplaceAroundStep usage.
ReplaceAroundStepcan be tricky for transformations. Ensure that surrounding content and indexes remain valid after dedenting multiple nested lists in large documents.
43-63: Ensure single-step or multi-step dedent logic.
dedentListCommandfinds the lists range and callsdedentRange. Test multi-nested lists thoroughly to confirm that repeated calls or partial dedenting doesn’t produce malformed structures.
109-148: Confirm dedentNodeRange logic.
dedentNodeRangecallssafeLiftRangeordedentOutOfList. Evaluate ifsafeLiftRangemight incorrectly lift a node that was partially dedented, especially with nested list items.
211-233: Double-check boundary fix logic.
fixEndBoundarymerges siblings by callingmoveRangeSiblings. Confirm that multi-layer merges still respect user expectations of separate list blocks.
235-279: Validate dedentOutOfList merges.Looping over child nodes merges them into one “big list node,” then uses
ReplaceAroundStep. This might produce surprising results if partial merges were intended. Thoroughly test.packages/editor/src/core/hooks/use-editor.ts (2)
27-27: Check for potential conflicts in your migration logic.
migrateDocJSONis newly imported. Ensure the data structure post-migration stays compatible with other features (like indentation commands).
114-115: Prevent repeated migrations.
[hasMigrated, setHasMigrated]ensures we only migrate once. This is good, but watch out for scenarios where re-migration might be needed if the doc changes drastically (e.g., loaded from a stale source).packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts (2)
125-142: Ensure dynamic event listeners are cleaned up.
DropCursorViewattaches drag events in the constructor, removing them indestroy(). This is correct, but confirm no memory leaks if the extension re-initializes multiple times.
268-302: Validate custom drop logic for list items.When a user drops a node between lists, the code re-inserts that node via
tr.delete+tr.insert. Thoroughly test to confirm that merges or splits of lists happen as intended, especially if dropping multiple items.packages/editor/src/core/extensions/drop-cursor-working.ts (1)
214-225: Verify the sibling detection logic.When determining the next sibling or parent node in the DOM, ensure that edge cases (e.g.,
nullor missing elements) are gracefully handled to prevent runtime errors or incorrect discount logic for the drop position.🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts (1)
76-77: Ensure offset usage is correct.Subtracting
1frompositionor applying offsets manually can lead to off-by-one errors. Double-check that these numeric adjustments reflect the intended final cursor position.packages/editor/src/core/extensions/drop-cursor.ts (1)
50-52: Confirm return type from the helper function.Your destructuring implies
rawIsBetweenFlatListsFnmay return an object with specific properties, but you also allow|| {}. Validate that the fallback prevents undefined references during object destructuring.packages/editor/src/core/extensions/drop-cursor.optimized.ts (1)
76-76: Double-check the final drop position offset.The code subtracts
2from the computeddropPosByDropCursorPosbefore insertion. Ensure this aligns with your content structure and does not cause off-by-one insertion.packages/editor/src/core/extensions/flat-list/core/utils/is-text-selection.ts (2)
1-2: No issues with import statement.
3-5: Implementation looks good.
This type guard correctly checks if the value is aTextSelection. It is a precise and concise solution.packages/editor/src/core/extensions/flat-list/core/utils/is-list-type.ts (3)
1-2: No issues with initial import.
3-4: No issues with secondary import.
5-8: Verify multiple list type scenarios.
If the schema contains multiple list node types,getListType(type.schema) === typemight fail for some. Confirm there is only one list node type or handle multiple.packages/editor/src/core/extensions/flat-list/core/utils/parse-integer.ts (1)
1-7: Logic is sound.
The function correctly handles null and undefined cases, and cleanly returns an integer or null.packages/editor/src/core/extensions/flat-list/core/utils/is-list-node.ts (1)
6-9: This utility function looks clean and concise.The implementation is straightforward and checks for the truthiness of
nodebefore callingisListType. This is both safe and purposeful. No changes needed at this time.packages/editor/src/core/extensions/flat-list/core/utils/is-block-node-selection.ts (1)
5-7: Implementation is appropriate for selection checks.Using
isNodeSelection(selection)before verifyingselection.node.type.isBlockis a clear approach. This guarded check ensures no runtime error for undefined node types.packages/editor/src/core/extensions/flat-list/core/utils/is-collapsed-list-node.ts (1)
8-9: Logic is aligned with intended functionality.The function checks both the node type and the
collapsedattribute, making it easy to detect collapsed list nodes. No issues found.packages/editor/src/core/extensions/flat-list/core/plugins/rendering.ts (1)
10-18: Logical and modular plugin creation.Wrapping the node view logic in a separate plugin (
createListRenderingPlugin) is a clean approach, ensuring the rendering concerns stay modular. This fosters future extensibility for different list node types or advanced behaviors.packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts (1)
3-11: Clarify assumptions for thepatchfunction.This utility assumes that the
patchfunction always returns a transaction fully compatible with thedispatchcallback. Consider validating or documenting preconditions (e.g., ensuring the returned transaction is properly shaped) to avoid subtle runtime errors ifpatchis malformed.packages/editor/src/core/extensions/flat-list/core/plugins/event.ts (1)
10-18: Verify mouse event behavior for different node types.The plugin handles
mousedownevents on list markers but might also intercept interactions with other child nodes. Verify thathandleListMarkerMouseDowngracefully ignores or properly handles clicks outside the expected context (e.g., nested nodes or embedded media).packages/editor/src/core/extensions/flat-list/core/utils/set-node-attributes.ts (1)
4-13: Well-structured utility function for updating node attributes.This function cleanly updates node attributes only when necessary, returning a
booleanindicating whether any changes were made. This approach helps avoid unnecessary transactions and keeps performance overhead low.packages/editor/src/core/extensions/flat-list/core/plugins/safari-workaround.ts (1)
12-14: Safari IME plugin looks solid.The plugin elegantly wraps
imeSpanand can be cleanly integrated into your editor plugin stack. Ensure you document usage in your setup to inform developers of its role in managing Safari’s IME quirks.packages/editor/src/core/extensions/flat-list/core/utils/create-and-fill.ts (1)
3-15: Effective use of ProseMirror’screateAndFill.This function robustly handles node creation and ensures validity via
node.check(). The explicitRangeErrormessage for failed creation is helpful for debugging.packages/editor/src/core/extensions/flat-list/core/utils/in-collapsed-list.ts (1)
7-18: Implementation looks good.This function correctly checks each ancestor node for the
collapsedattribute. The loop structure is efficient for typical ProseMirror documents, as the maximum depth is generally small. No critical issues or off-by-one errors are apparent.packages/editor/src/core/extensions/flat-list/core/utils/zoom-in-range.ts (1)
6-19: Consider verifying deeper edge cases in complex nested structures.This utility returns a deeper block range if one exists. Be mindful of extreme nesting scenarios (e.g., deeply nested lists or tables). Though rare, they can occur in rich text editors, so add tests to cover extreme nested levels to ensure correctness.
packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-start.ts (1)
6-10: Implementation is correct and concise.The method properly checks whether the cursor is at the start of the text block. No improvements required; the logic and fallback to
nullfor non-text cursor selections are appropriate.packages/editor/src/core/extensions/flat-list/core/types.ts (3)
8-8: LeverageListKindfor consistency.
Good job using a union type for meaningful enumeration of valid list kinds:"bullet" | "ordered" | "task" | "toggle". This helps ensure type safety across your list-related utilities.
19-25: Validate theProsemirrorNodeJSONinterface.
This interface seems well-aligned with ProseMirror node definitions. Ensure that any custom attributes outside ofAttrsare carefully handled in your node transformations to avoid unexpected runtime errors.
27-27: Re-exportingNodeasProsemirrorNodeis helpful for clarity.
This re-export clarifies that we're dealing with ProseMirror’sNode, which can reduce confusion in your codebase. Nicely done.packages/editor/src/core/extensions/flat-list/core/plugins/clipboard.ts (2)
1-2: Confirm that the schema is fully aligned with list node specs.
The plugin relies on schema specs for list nodes. Ensure all relevant schemas (e.g., bullet, ordered, task, toggle) are well-defined so the clipboard plugin serializes/deserializes them accurately.
13-20: Excellent approach to customizing clipboard handling for lists.
Using a specialized serializer (ListDOMSerializer) andunwrapListSlicefosters consistent copy-paste behavior for various list types. This design encapsulates list-clipboard logic neatly in one plugin.packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts (1)
5-12: Safeguard edge cases when lifting.
safeLiftcorrectly checks for a validliftTargetbefore performing the lift, returningfalseif none is found. This approach prevents unsafe transformations and helps avoid runtime errors.packages/editor/src/core/extensions/flat-list/core/utils/set-list-attributes.ts (1)
8-22: Consider partial merges for extended attributes.
The current spread merging logic is straightforward, combining all existing attributes with the new ones. If you introduce nested attribute structures in the future, double-check deep merges so no data is inadvertently overwritten.Validate node type before merging attributes.
TheisListNodecheck is essential, but if you expand list node types beyond bullet/ordered/task/toggle, ensureisListNodehandles them uniformly.packages/editor/src/core/extensions/flat-list/core/utils/unwrap-list-slice.ts (1)
12-22: Implementation appears solid.The function effectively reduces open depth for slices containing a single list node, preventing unwanted deep nesting when pasting content. The loop operates on clear conditions and exits when they are no longer satisfied. This looks correct and efficient for most list-related use cases.
packages/editor/src/core/extensions/flat-list/core/utils/block-boundary.ts (2)
3-15: Validate the usage constraints.The loop uses
d <= $pos.depth, which is correct for typical scenarios. However, if an invalid depth is passed in (e.g., ifdepthis larger than$pos.depth), it could result in unexpected logic flow. Recommend documenting valid depth ranges to ensure consistency.
17-29: Logic is clear and aligns with block boundary checks.Similar to the start boundary check, this approach for checking the end boundary is well-structured, iterating up the node hierarchy without redundancy. The readability is good, and the function’s name matches its behavior.
packages/editor/src/core/extensions/flat-list/core/commands/toggle-list.ts (1)
16-27: Toggling logic looks coherent.Chaining
unwrapListfollowed bywrapInListis a common pattern for toggling. However, verify that lists of the same kind are correctly re-wrapped or unwrapped without unexpected side effects. Test thoroughly on multiple list kinds (e.g., bullet, ordered) to confirm correct toggling behavior.packages/editor/src/core/extensions/flat-list/core/utils/max-open.ts (3)
1-2: Imports look good.
No issues with these import statements; they correctly pull in the required ProseMirror types.
3-17: Reference to original ProseMirror code is commendable.
Copying relevant snippets from ProseMirror ensures consistent behavior. The logic for incrementally increasingopenStartlooks correct, and the optionalopenIsolatingflag is well-handled.
19-33: Symmetrical approach for maxOpenEnd is consistent with maxOpenStart.
Mirroring the logic inmaxOpenStartensures that both ends of the fragment are handled similarly. The code is clear, succinct, and follows best practices.packages/editor/src/core/extensions/flat-list/core/utils/list-range.ts (3)
1-4: Helper imports are well-organized.
TheNodeRange,ResolvedPos, andisListNodeimports are concise and relevant, aligning with the utility's purpose.
5-31: Robust approach for finding list ranges.
ThefindListsRangefunction carefully re-checks ranges with adjusted depths, ensuring that multiple sibling list nodes are captured. The recursive fallback of$to<$fromis succinct and effective.
33-44: Efficient validation of list nodes.
TheisListsRangefunction loops through children in the specified range to confirm they are lists. This explicit check prevents mistakenly treating non-list nodes as lists.packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts (1)
1-9: Overall structure is clear.
The function commentary is thorough and explains the rationale behind splitting logic and boundary avoidance.packages/editor/src/core/extensions/flat-list/core/plugins/index.ts (3)
1-3: Appropriate imports for plugin creation.
These imports align with the modular plugin architecture.
4-8: Plugins are well-organized by feature.
Clipboard, event, rendering, and Safari workarounds are separated, enhancing clarity.
31-36: Additional exports for plugin creation.
Exporting individual plugin builders promotes flexibility and reusability for advanced consumers.packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts (1)
19-43: Check for multi-range selections.
This logic currently considers single continuous ranges. If multi-range selections or disjoint ranges become possible, you might need additional handling.Generate a script to search for multi-range usage:
packages/editor/src/core/extensions/flat-list/core/node-view.ts (1)
23-25: Ensure toggling logic is robust across browsers.
Adding an empty<span>is a clever workaround for iOS Safari, but watch for potential quirks on other platforms. Consider adding browser checks, if new anomalies arise.packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts (1)
39-52: Validate attributes for complex use cases.
collapsed,checked, etc. can be interdependent attributes. Ensure there's a plan for thoroughly testing combinations (e.g., toggled and collapsed).packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts (3)
1-5: Imports look good.
All imported modules appear essential; no issues identified.
8-23: Appropriate interface design.
TheToggleCollapsedOptionsinterface is well-structured, with clear documentation for optional fields.
56-64: Well-defined default toggleability check.
ThedefaultIsToggleablefunction correctly ensures toggling only applies to toggle lists that contain children.packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts (5)
1-7: Imports are standard and concise.
No redundant imports detected.
8-18: Adequate documentation.
The docstring thoroughly conveys the command's behavior under different conditions.
19-47: Check boundary cases injoinListUp.
While the logic for index checks (indexInList === 0orindexInList === listNode.childCount - 1) is correct, consider a unit test for edge cases, such as a list with a single child, or scenarios where the view might not supply a valid cursor.
49-64:liftListContentutility is well-structured.
Clear separation of concerns and usage ofNodeRangewithsafeLift.
66-76:liftParentfunction matches intended use.
Logic is straightforward and reusessafeLift; no concerns here.packages/editor/src/core/extensions/flat-list/core/dom-events.ts (2)
1-9: Imports are well-structured.
No concerns regarding import statements.
52-65: Handler logic is straightforward.
defaultListClickHandlertogglescheckedorcollapsedbased onkind. This is a concise approach, and the expansions for other list kinds remain flexible.packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts (5)
1-11: Imports are appropriate.
The modules and functions imported here align well with the file’s purpose.
12-24: Selection handling logic is well-thought-out.
moveOutOfCollapsediterates through depths to ensure the user never gets stuck in a collapsed node. This is beneficial for user experience.
26-41:setSafeSelectionapproach is coherent.
Combining the checks for$fromand$toensures the final selection is outside of collapsed nodes.
42-42: Convenient command wrapper export.
withSafeSelectionnicely follows the existing pattern of patching commands for safer operations.
44-51:getCollapsedPositionnaming is clear.
This function effectively returns the position before the collapsed node. No issues found.packages/editor/src/core/extensions/flat-list/core/index.ts (1)
1-9: Export structure looks goodThe grouping of commands, DOM event handlers, and utilities into separate modules helps maintain a logical separation of responsibilities. This approach will make your code more discoverable and easier to maintain.
packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts (3)
1-8: Organized import structureThese ProseMirror command imports are well-organized. Keeping them together at the top provides clarity on which default commands are leveraged and extended.
17-26: Inline documentation is clearThe docstring explaining the
enterCommandis well-structured and accurately references the commands it composes. Good job making it easy for other developers to understand how these commands work together.
27-47: Complex command chaining logicThe
backspaceCommandwraps several commands for robust handling of list scenarios. While this is powerful, observe that further additions or special cases can make the chain unwieldy. Consider writing comprehensive tests covering partial deletions, empty lines, nested lists, etc., to ensure all branches behave correctly.Would you like me to generate some example test scaffolding or shell scripts to confirm coverage of these edge cases?
packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts (3)
1-31: Clear interface definition
ListToDOMOptionsis well-defined with descriptive docstrings. This ensures clarity for anyone working with your DOM conversion logic.
33-74: Conditional logic for native vs. custom list renderingThe
listToDOMfunction elegantly handles both native lists and custom<div>containers. However, be mindful that advanced manipulations (like stylized markers or toggles) in a native list might cause unexpected browser behavior in some edge cases. Testing in multiple browsers (Safari, IE, etc.) is recommended to ensure consistent rendering.
90-106: Data attributes for styling and togglingThe
defaultAttributesGetterensures that key data attributes (data-list-kind,data-list-collapsed, etc.) are attached for styling or DOM-based logic. Be mindful of potential naming collisions if other components or frameworks read or modify the same attributes.packages/editor/src/core/extensions/read-only-extensions.tsx (1)
34-34: ImportingFlatListExtensionThanks for keeping the import at a consistent location alongside other extension imports. This helps maintain a clear import hierarchy.
packages/editor/src/core/types/editor.ts (1)
38-38: Addition of "toggle-list" command looks good.This addition appropriately extends the editor commands to support toggling list functionality. Ensure all references to
"toggle-list"are thoroughly tested and invoked appropriately throughout the codebase.packages/editor/src/core/components/menus/menu-items.ts (4)
38-41: List toggling methods aligned with "flat" approach.The newly imported
toggleFlatBulletList,toggleFlatOrderedList,toggleFlatTaskList, andtoggleFlatToggleListunify the list creation approach. This consistency is a good improvement. Verify that any references to the older toggles (toggleBulletList, etc.) are removed to avoid unused methods.
158-159: Use of isActive("list", { kind: "bullet" }) in BulletListItem.Switching from
isActive("bulletList")toisActive("list", { kind: "bullet" })is a clear, extensible approach that aligns with the new list architecture. This looks good.
166-167: Use of isActive("list", { kind: "ordered" }) in NumberedListItem.Similar to bullet lists, adopting the new
listscope for ordered lists is consistent and follows the architectural change for "flat" lists.
174-175: Refactoring to use isActive("list", { kind: "task" }) in TodoListItem.Good alignment with the new "flat" list approach.
packages/editor/src/core/helpers/editor-commands.ts (4)
54-65: New toggleFlatOrderedList logic.This function introduces a clear approach for ordered lists via
createList. Confirm any existing calls totoggleOrderedListare now migrated, if the goal is to fully adopt this new approach.
67-78: New toggleFlatBulletList logic.Implementation is parallel to toggleFlatOrderedList. This is consistent with the new "flat" approach. Ensure range-based deletion performs correctly with multiline selections.
80-91: New toggleFlatTaskList logic.Again, consistent with the bullet/ordered approach. Great for uniform behavior across different list types.
93-104: New toggleFlatToggleList logic.Implementation mirrors the other "flat" expansions. Ensure extended toggles (like sub-items or nested toggles) handle range deletions properly if those are part of any roadmap capabilities.
packages/editor/src/core/plugins/drag-handle.ts (2)
23-23: New selector for flat-list is well-defined.
Including".prosemirror-flat-list"in the selectors is a clear addition for handling the new flat-list style blocks. Ensure this does not collide with any other existing CSS selectors that might unintentionally match.
382-383: Consider edge cases in position adjustment for flat-list.
Subtracting one from thedraggedNodePoswhen the node’s class name includes"prosemirror-flat-list"might cause off-by-one errors if combined with other list transformations (e.g., nested lists, toggling between list types). Please verify that the updated position remains accurate across all editor states.packages/editor/src/styles/drag-drop.css (1)
63-69: Confirm the styling offset for flat-list.
Applying a 25px horizontal offset and making the::afterborder transparent differentiates.prosemirror-flat-listfrom other selected nodes. This seems consistent with the new list type. Please verify that the increased offset does not introduce UI overlap in more complex layouts (e.g., nested lists, small viewport widths).packages/editor/package.json (4)
45-45: New bullet-list extension dependency.
Adding"@tiptap/extension-bullet-list"at version^2.10.3aligns with the new list functionality. Confirm that it does not conflict with any other versions in the project.
49-49: Update of list-item extension.
Upgrading"@tiptap/extension-list-item"from^2.1.13to^2.10.3is a significant jump. Verify any backward-incompatible changes in the extension and ensure they're accounted for in the new editor logic.
51-51: New ordered-list extension added.
Introducing"@tiptap/extension-ordered-list"is consistent with the new list enhancements. Validate that it integrates correctly with the bullet-list and list-item changes for a stable user experience.
69-69: Implement Safari IME fix.
The addition of"prosemirror-safari-ime-span"addresses known quirks in Safari’s IME handling. Check for any newly identified platform issues that may require additional patches.packages/editor/src/core/extensions/flat-list/core/style.css (2)
1-11: LGTM! Base list styles are well-structured.The base styles provide a good foundation with proper spacing and positioning. The use of logical properties (
margin-left) ensures better RTL support.
68-69: Consider browser support for logical height unit.The
1lhlogical height unit has limited browser support. Keep the fallback1.5emfor better compatibility.✅ Verification successful
The fallback for
1lhis already in placeLooking at the search results, I can see that there's already a fallback declaration
width: 1.5em;before thewidth: 1lh;in the CSS file. This follows the CSS cascade where browsers that don't support thelhunit will use theemunit fallback, while modern browsers that supportlhwill use that value. This is the recommended progressive enhancement approach.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check caniuse data for logical height unit support curl -s "https://raw.githubusercontent.com/Fyrd/caniuse/main/features-json/logical-height-units.json" | jq '.stats'Length of output: 177
Script:
#!/bin/bash # Search for any fallback declarations for lh unit in CSS files rg -B1 -A1 "width:\s*\d+lh" --type css # Search for any other usage of lh unit in CSS files rg "\d+lh" --type cssLength of output: 674
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
21-21: LGTM! Clean integration of ToggleListItem.The
ToggleListItemis well-integrated into the existing menu structure, maintaining consistency with other list items.Also applies to: 46-46
packages/editor/src/styles/editor.css (1)
411-414: LGTM! Consistent paragraph spacing.The simplified padding approach provides more consistent spacing between paragraphs.
| // Delete any existing content at the current position if it's an empty paragraph | ||
| const nodeAfter = tr.doc.nodeAt(position); | ||
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | ||
| tr.delete(position, position + 2); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the hardcoded magic number "2" when deleting the empty paragraph.
Using a hardcoded length of "2" might break if the paragraph node structure changes. Consider using nodeAfter.nodeSize or a similar approach to ensure robust paragraph deletion.
- tr.delete(position, position + 2);
+ tr.delete(position, position + nodeAfter.nodeSize);📝 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.
| // Delete any existing content at the current position if it's an empty paragraph | |
| const nodeAfter = tr.doc.nodeAt(position); | |
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | |
| tr.delete(position, position + 2); | |
| } | |
| // Delete any existing content at the current position if it's an empty paragraph | |
| const nodeAfter = tr.doc.nodeAt(position); | |
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | |
| tr.delete(position, position + nodeAfter.nodeSize); | |
| } |
packages/editor/src/core/extensions/flat-list/core/commands/join-collapsed-backward.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/get-list-type.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts
Show resolved
Hide resolved
Palanikannan1437
force-pushed
the
feat/flat-lists
branch
from
fec5cc2 to
7536a78
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/editor/src/core/extensions/drop-cursor.ts (4)
264-268: Use optional chaining for safer access
Static analysis suggests using optional chaining to eliminate potential undefined access in lines involvingnodeandnode.type. Specifically, you might rewrite:-const disableDropCursor = node && node.type.spec.disableDropCursor; +const disableDropCursor = node?.type?.spec?.disableDropCursor;🧰 Tools
🪛 Biome (1.9.4)
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
270-274: Add null check or optional chaining for drag slice
Static analysis warns of potential undefined access when referencingthis.editorView.dragging.slice. Consider using optional chaining to safely guard this access:-const point = dropPoint(this.editorView.state.doc, target, this.editorView.dragging.slice); +const point = dropPoint(this.editorView.state.doc, target, this.editorView.dragging?.slice);🧰 Tools
🪛 Biome (1.9.4)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
142-152: Refine arguments forsetCursor
The function signaturesetCursor(pos: number | null, isBetweenFlatLists?: boolean)carries a loosely typed second parameter. A more descriptive name or explicit interface for the cursor state might help maintain clarity, especially when adding new states or flags in the future.
154-194: Improve maintainability of overlay logic
TheupdateOverlaymethod is fairly large and has multiple branching conditions. Consider splitting out distinct responsibilities (e.g., computing rect positions, toggling classes, updating transform properties) into smaller helper functions for better readability.packages/editor/src/core/extensions/extensions.tsx (5)
60-62: Explicitly disabling built-in list configurationsBy setting
bulletList,orderedList, andlistItemtofalse, you are effectively deferring to custom or extended versions of these. This is a clean approach, although keep an eye on backward compatibility if existing features depended on Tiptap’s default lists.
85-93: CustomBulletListextension with no input rulesRemoving or overriding default input rules is common when implementing a more controlled list experience. If your project requires auto-list creation upon typing
-, consider adding minimal input rules or confirm your UI addresses this need.
94-102: CustomOrderedListextension with no input rulesSimilar to
BulletList, reintroduce or adapt input rules if users expect auto-incrementing numbered lists upon typing1.or similar patterns.
112-120: TaskList extension overrideFully custom
TaskListlogic might require more robust input or transformation rules if you want automatic creation of new tasks. Ensure user experience is intuitive without them.
121-133: CustomizingTaskItemwith keyboard shortcutsProviding an empty
addKeyboardShortcutsmight be an interim approach. If your usage scenario requires toggling tasks via keystrokes or pressing enter to automatically create new tasks, consider implementing these shortcuts for a smoother experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/src/core/extensions/drop-cursor.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/hooks/use-editor.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/drop-cursor.ts
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
packages/editor/src/core/extensions/drop-cursor.ts (3)
39-40: Validate data extraction prior torawIsBetweenFlatListsFncall
321-412: Confirm edge cases inrawIsBetweenFlatListsFn
Ensure the function properly handles scenarios where the element under drag is not part of a.prosemirror-flat-list, or if list logic is extended in the future (e.g., multiple nested toggles or tasks). You may want more robust checks to manage other edge cases, such as empty lists or partially rendered nodes.
430-452: Throttle logic looks good
The throttling for re-checking drag position appears reasonable. Confirm if the default threshold of 8px suits typical user interactions, otherwise consider making it configurable.packages/editor/src/core/extensions/extensions.tsx (8)
6-6: Good addition ofListItemimportImporting the
ListItemextension sets the foundation for advanced list handling. No immediate issues.
10-11: Proper bullet / ordered list importsUsing these explicit imports will help ensure clarity in the codebase and maintain separation of concerns for each list type. Nicely done.
40-41: Cleanly separating typesExplicitly importing
TExtensions,TFileHandler, andTMentionHandlerhelps maintain code clarity and type safety across the editor’s setup.
42-43: DropCursorExtension & FlatListExtension importsThese imports demonstrate a custom approach to drop-cursor behavior and advanced list features. Ensure there are no conflicts with the built-in Tiptap functionality or any duplicated logic.
77-80: Swapping the defaultdropcursorwithdropcursor: falseCommenting out the existing dropcursor class-based setup and defaulting to
dropcursor: falseis fine if the customDropCursorExtensionis fully tested. Monitoring user feedback on performance and drag-and-drop behavior is advisable.
83-84: Ensuring correct order of extension loadingPlacing
DropCursorExtensionandFlatListExtensionbefore the custom bullet/ordered/task list ensures the new drop-cursor behavior is applied early. Verify no unintended overrides occur between them.
103-111: Overriding defaultListItemextensionYou’re disabling default input rules. If certain behaviors (like pressing Enter to continue or break a list) are desired, ensure they remain accessible in your custom list or the
FlatListExtension.
207-212: Markdown extension with HTML transformationsEnabling
html: trueandtransformCopiedText / transformPastedTextcan make content editing more flexible, but also raises potential security and formatting concerns. Verify that any pasted content is sanitized or that your environment is trusted.
Palanikannan1437
force-pushed
the
feat/flat-lists
branch
from
96e9f15 to
8487bb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/editor/src/core/extensions/extensions.tsx (1)
85-120: Consider consolidating duplicate code in list extensions.The BulletList, OrderedList, and ListItem extensions have identical empty parseHTML and addInputRules methods. Consider creating a base extension or utility function to reduce code duplication.
+ const createEmptyListExtension = (extension: any) => + extension.extend({ + parseHTML() { + return []; + }, + addInputRules() { + return []; + }, + }); - BulletList.extend({ - parseHTML() { - return []; - }, - addInputRules() { - return []; - }, - }) + createEmptyListExtension(BulletList)packages/editor/src/core/extensions/index.ts (1)
26-26: Consider selective exports for better tree-shaking.
Exporting everything from"./flat-list"might introduce unwanted payload in the final bundle if only some parts of"./flat-list"are used.- export * from "./flat-list"; + export { + FlatListExtension, + // export additional symbols only as needed + } from "./flat-list";packages/editor/src/core/extensions/flat-list/list-extension.ts (2)
18-27: Ensure naming conventions remain consistent.
The interface is declared asflatListComponent, while the extension name is"list". Confirm that the naming aligns with the rest of the codebase for clarity and maintainability.
78-111: Add key bindings for backspace dedent/smart handling if needed.
Currently, Tab, Shift-Tab, and Enter are handled. Consider implementing a specialized backspace handler to remove indentation or merge items in certain cases, if that aligns with your design goals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/extensions/flat-list/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/list-extension.ts(1 hunks)packages/editor/src/core/extensions/index.ts(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(4 hunks)packages/editor/src/core/extensions/table/table/table.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/core/extensions/flat-list/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/extensions/read-only-extensions.tsx
- packages/editor/src/core/extensions/table/table/table.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-admin
- GitHub Check: build-space
🔇 Additional comments (7)
packages/editor/src/core/extensions/extensions.tsx (4)
2-5: LGTM! Imports are well-organized.The new imports are properly structured and align with the PR objectives for implementing flat lists.
Also applies to: 35-35
83-84: LGTM! New extensions are properly registered.The DropCursorExtension and FlatListExtension are correctly added to replace the disabled default extensions.
222-227: LGTM! Markdown configuration is well-configured.The Markdown extension is properly configured with HTML support and text transformation enabled.
60-62: Verify the impact of disabling dropcursor.While disabling the built-in list extensions is correct for implementing flat lists, disabling the dropcursor extension might affect the visual feedback when dragging content. Please ensure this doesn't impact the drag-and-drop user experience.
Also applies to: 77-80
✅ Verification successful
Disabling dropcursor is correct and intentional
The codebase implements its own comprehensive drag-and-drop system using @atlaskit/pragmatic-drag-and-drop with custom visual indicators, making the default ProseMirror dropcursor extension unnecessary and potentially conflicting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any custom drag-and-drop handling that might replace dropcursor functionality rg -l "onDragStart|onDragEnd|onDrop" --type ts --type tsxLength of output: 87
Script:
#!/bin/bash # Search for drag-and-drop handlers in both .ts and .tsx files echo "=== Searching for drag-and-drop handlers ===" rg -l "onDragStart|onDragEnd|onDrop" --type ts rg -l "onDragStart|onDragEnd|onDrop" -g "*.tsx" echo -e "\n=== Searching for dropcursor references ===" # Search for dropcursor-related code and configuration rg -i "dropcursor" --type ts rg -i "dropcursor" -g "*.tsx" echo -e "\n=== Checking for ProseMirror drag-drop plugins ===" # Look for any custom drag-drop implementations rg -i "drag|drop.*plugin" --type ts rg -i "drag|drop.*plugin" -g "*.tsx"Length of output: 67234
packages/editor/src/core/extensions/flat-list/list-extension.ts (3)
33-49: Check for potential read-only usage.
ThisNodeextension setsselectable: trueanddraggable: true. If the editor includes a read-only mode, confirm that the list extension respects read-only constraints appropriately.
50-77: Commands look well-structured.
This modular approach, splitting core list functionality into separate commands (create, indent, dedent, split), enhances maintainability. Good job!
1-16: Validate that all imported modules are in use.
Review the imports from"./core"to ensure no unused imports are dragged in. This helps maintain clarity and possibly reduce bundle size.✅ Verification successful
All imports from "./core" are actively used in the code
Each imported symbol is utilized within the file for type definitions, plugin creation, or command handling. No unused imports detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references of each imported symbol to verify usage rg -A 2 "ListAttributes|IndentListOptions|DedentListOptions|createListSpec|listKeymap|listInputRules|createWrapInListCommand|createIndentListCommand|createDedentListCommand|createSplitListCommand|createListPlugins"Length of output: 37777
| TaskItem.extend({ | ||
| parseHTML() { | ||
| return []; | ||
| }, | ||
| addInputRules() { | ||
| return []; | ||
| }, | ||
| addKeyboardShortcuts() { | ||
| return {}; | ||
| }, | ||
| }).configure({ | ||
| HTMLAttributes: { | ||
| class: "relative", | ||
| }, | ||
| nested: true, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement keyboard shortcuts for TaskItem.
The TaskItem extension has empty keyboard shortcuts configuration. Consider implementing keyboard shortcuts for better user experience, such as toggling task completion status.
addKeyboardShortcuts() {
- return {};
+ return {
+ 'Mod-Enter': () => this.editor.commands.toggleTaskCheck(),
+ 'Mod-Space': () => this.editor.commands.toggleTaskCheck(),
+ };
},📝 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.
| TaskItem.extend({ | |
| parseHTML() { | |
| return []; | |
| }, | |
| addInputRules() { | |
| return []; | |
| }, | |
| addKeyboardShortcuts() { | |
| return {}; | |
| }, | |
| }).configure({ | |
| HTMLAttributes: { | |
| class: "relative", | |
| }, | |
| nested: true, | |
| }), | |
| TaskItem.extend({ | |
| parseHTML() { | |
| return []; | |
| }, | |
| addInputRules() { | |
| return []; | |
| }, | |
| addKeyboardShortcuts() { | |
| return { | |
| 'Mod-Enter': () => this.editor.commands.toggleTaskCheck(), | |
| 'Mod-Space': () => this.editor.commands.toggleTaskCheck(), | |
| }; | |
| }, | |
| }).configure({ | |
| HTMLAttributes: { | |
| class: "relative", | |
| }, | |
| nested: true, | |
| }), |
Description
This PR adds flat lists to our schema, a migration script that aims to safely convert old lists to new ones and a ton of new improvements to lists
Detailed description
TODO
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Performance