Apply schema to model #1
Conversation
WalkthroughThe changes involve significant restructuring and enhancements across multiple files in the codebase. The Changes
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: 11
🧹 Outside diff range and nitpick comments (36)
src/model/range.ts (2)
2-4: Consider enhancing the function documentation.While the current documentation is clear, it could be more informative. Consider adding details about the parameter and return value.
Here's a suggested improvement:
/** * Checks if the given range is collapsed. * @param range The Range object to check. * @returns {boolean} True if the range is collapsed, false otherwise. */
1-8: Overall, good implementation with room for minor improvements.The
isCollapsedfunction is well-implemented and efficient. Consider the suggested improvements for documentation and edge case handling. Additionally, it would be beneficial to add unit tests for this function to ensure it behaves correctly for various input scenarios.Would you like assistance in generating unit tests for the
isCollapsedfunction?src/main.ts (1)
10-14: Well-structured schema introductionThe introduction of the
schemaobject is a significant improvement. It defines a clear structure for the document, allowing for better content organization and potentially richer text formatting. The schema aligns well with common HTML structures, which is good for compatibility and familiarity.Consider expanding the schema in the future to support more complex document structures, such as headings, lists, or inline formatting elements (bold, italic, etc.). This would allow for even richer content creation capabilities.
src/model/operations.ts (2)
9-17: LGTM with a minor suggestion: Update JSDoc comment for consistency.The
EditOperationtype is well-structured and appropriate for representing an edit operation in the editor. However, there's a small inconsistency in the JSDoc comment.Consider updating the JSDoc comment to use "EditOperation" instead of "ReplaceOperation" for consistency:
/** - * `ReplaceOperation` represents a operation that replaces a range of nodes with + * `EditOperation` represents an operation that replaces a range of nodes with * another range of nodes. */
19-27: LGTM with a minor suggestion: Correct grammar in JSDoc comment.The
MoveOperationtype is well-defined and appropriate for representing a move operation in the editor. The structure and naming are clear and consistent.There's a minor grammatical issue in the JSDoc comment. Consider updating it as follows:
/** - * `MoveOperation` represents a operation that moves a range of nodes to another + * `MoveOperation` represents an operation that moves a range of nodes to another * position. */test/model/helper.ts (1)
14-26: LGTM: tokenToXML function is well-implemented. Consider enhancing the JSDoc.The
tokenToXMLfunction is logically sound and handles all cases of theTagTypeenum. The implementation is concise and effective.Consider enhancing the JSDoc comment to include information about the parameters and return value. For example:
/** * Converts a node and tag type to an XML string. * @param node - The node to convert. * @param type - The type of tag to generate ('open', 'close', or 'text'). * @returns The XML string representation of the node. */test/model/model.test.ts (2)
6-10: LGTM: Good test for model creation, consider adding more assertions.This test case effectively verifies that a
Modelcan be created with a schema and initial value. The use of a template literal for the XML improves readability.Consider adding assertions to check other properties of the created model, such as its schema, to ensure it's fully initialized as expected.
1-23: Overall assessment: Good start on Model tests, room for expansionThis new test file for the
Modelclass provides a solid foundation for testing core functionalities. The tests are well-structured and follow best practices. However, there are opportunities for improvement:
- Expand test coverage to include more scenarios for text editing.
- Address the TODO for testing inverse operations.
- Consider adding tests for edge cases and potential error conditions.
These additions will enhance the robustness of the test suite and ensure the
Modelclass behaves correctly under various conditions.As the
Modelclass evolves, remember to update and expand these tests accordingly to maintain comprehensive coverage.src/model/nodepos.ts (3)
4-27: LGTM with a minor suggestion: Enhance JSDoc comment.The
isOutOfRangefunction is well-implemented with correct logic and good use of early returns. However, the JSDoc comment could be more descriptive.Consider expanding the JSDoc comment to provide more details about the function's parameters and return value. For example:
/** * Checks if the given node is out of the range defined by start and end positions. * @param node - The node to check. * @param start - The start position of the range. * @param end - The end position of the range. * @returns {boolean} True if the node is out of range, false otherwise. */
29-34: LGTM with a minor suggestion: Enhance JSDoc comment.The
isLeftMostfunction is correctly implemented. However, the JSDoc comment could be more informative.Consider expanding the JSDoc comment to provide more details about the function's parameter and return value. For example:
/** * Checks if the given node position is at the leftmost position of the node. * @param pos - The node position to check. * @returns {boolean} True if the position is at the leftmost (offset 0), false otherwise. */
36-41: LGTM with a minor suggestion: Enhance JSDoc comment.The
isRightMostfunction is correctly implemented, using thelengthOffunction to determine if the position is at the rightmost edge of the node. However, the JSDoc comment could be more informative.Consider expanding the JSDoc comment to provide more details about the function's parameter and return value. For example:
/** * Checks if the given node position is at the rightmost position of the node. * @param pos - The node position to check. * @returns {boolean} True if the position is at the rightmost (offset equals node length), false otherwise. */test/model/schema.test.ts (2)
52-65: LGTM: Comprehensive test forfromXMLmethod.The new test case effectively covers the functionality of the
fromXMLmethod, including both successful conversion and error handling. It aligns well with the existing test structure and provides good coverage.Consider adding more test scenarios to cover edge cases:
- Test with nested elements.
- Test with attributes in the XML.
- Test with empty elements.
Example:
it('should handle complex XML structures', () => { const schema = new Schema({ root: { children: 'p*' }, p: { children: '(text|span)*', attrs: { id: {} } }, span: { children: 'text*' }, text: {}, }); const val = /*html*/ `<p id="test"><span>Hello</span>, <span>world!</span></p>`; expect(toXML(schema.fromXML(val))).toEqual(val); const emptyVal = /*html*/ `<p></p>`; expect(toXML(schema.fromXML(emptyVal))).toEqual(emptyVal); });
Line range hint
1-65: Overall assessment: Positive addition to the test suite.The new test case for
fromXMLmethod enhances the coverage of theSchemaclass functionality. It maintains consistency with existing tests and introduces valuable checks for XML conversion. The changes are well-integrated and improve the overall quality of the test suite.As the
Schemaclass evolves, consider organizing tests into describe blocks for each method (e.g.,describe('fromJSON'),describe('fromXML')). This structure will improve readability and make it easier to add more specific test cases in the future.src/model/types.ts (3)
6-14: LGTM: Element type definition is comprehensive. Consider using a non-optional children array.The
Elementtype definition is well-structured and covers all necessary properties for an element node. However, consider making thechildrenproperty non-optional with a default empty array. This can simplify operations on elements by ensuring thatchildrenis always an array.Consider updating the
childrenproperty as follows:children: Array<Node> = [];This change would ensure that
childrenis always initialized as an empty array, potentially simplifying operations on elements.
36-42: LGTM: Range type definition is concise. Consider adding more detailed documentation.The
Rangetype effectively represents a range in the editor content usingPathfor both start and end. This ensures consistency with thePathtype.Consider expanding the comment to include an example, similar to the
Pathtype documentation. This would further clarify howRangeis used in practice.
44-51: LGTM: NodePos type definition is clear. Consider adding a conversion method.The
NodePostype effectively represents a node's position in the model. The comment suggests that it can be converted toPath, which is a useful feature.Consider adding a utility function or method to convert
NodePostoPath. This would make the conversion process explicit and easily accessible. For example:export function nodePosToPatch(nodePos: NodePos): Path { // Implementation details }test/util/array.test.ts (2)
4-32: LGTM: Comprehensive tests for groupBy, with a suggestion for improvement.The test cases for
groupByare well-structured and cover various scenarios, including edge cases. They effectively test the function's behavior for different inputs and grouping conditions.Consider adding a test case for non-primitive types or custom comparison functions to further improve coverage. For example:
it('should group objects by a custom comparison function', () => { const objects = [ { id: 1, category: 'A' }, { id: 2, category: 'A' }, { id: 3, category: 'B' }, { id: 4, category: 'B' }, ]; const result = groupBy(objects, (prev, curr) => prev.category === curr.category); expect(result).toEqual([ [{ id: 1, category: 'A' }, { id: 2, category: 'A' }], [{ id: 3, category: 'B' }, { id: 4, category: 'B' }], ]); });
34-52: LGTM: Well-structured tests for takeWhile, with a suggestion for improvement.The test cases for
takeWhileare clear and cover the main scenarios, including edge cases like empty input and always true predicate.Consider adding a test case where the first element doesn't satisfy the condition, resulting in an empty array. This would ensure the function correctly handles immediate termination. For example:
it('should return an empty array when the first element does not satisfy the condition', () => { const array = [10, 1, 2, 3, 4, 5]; const result = takeWhile(array, (v) => v < 5); expect(result).toEqual([]); });src/utils/array.ts (4)
17-22: LGTM! Consider enhancing the JSDoc comment.The
initialOffunction is correctly implemented and type-safe. The logic to return all elements except the last one is accurate.Consider enhancing the JSDoc comment to mention the behavior for empty arrays:
/** * `initialOf` returns all elements of an array except the last one. * If the array is empty, it returns an empty array. */
24-29: LGTM! Consider enhancing the JSDoc comment.The
tailOffunction is correctly implemented and type-safe. The logic to return all elements except the first one is accurate.Consider enhancing the JSDoc comment to mention the behavior for empty arrays, similar to the suggestion for
initialOf:/** * `tailOf` returns all elements of an array except the first one. * If the array is empty, it returns an empty array. */
31-55: LGTM! Consider enhancing documentation and simplifying implementation.The
groupByfunction is correctly implemented and type-safe. The logic to group adjacent elements is accurate.
- Enhance the JSDoc comment to provide more details about the function's behavior:
/** * `groupBy` groups adjacent elements of an array according to a given function. * @param array - The input array to be grouped. * @param fn - A function that takes two adjacent elements and returns true if they should be in the same group. * @returns An array of arrays, where each inner array is a group of adjacent elements. * @example * groupBy([1, 2, 2, 3, 4, 4, 5], (a, b) => a === b) * // Returns [[1], [2, 2], [3], [4, 4], [5]] */
- Consider simplifying the implementation for better readability:
export function groupBy<T>( array: Array<T>, fn: (prev: T, curr: T) => boolean, ): Array<Array<T>> { return array.reduce((groups: Array<Array<T>>, current: T) => { const lastGroup = groups[groups.length - 1]; if (lastGroup && fn(lastGroup[lastGroup.length - 1], current)) { lastGroup.push(current); } else { groups.push([current]); } return groups; }, []); }This implementation eliminates the need for separate handling of the first element and simplifies the logic.
57-73: LGTM! Complete the JSDoc comment.The
takeWhilefunction is correctly implemented and type-safe. The logic to take elements while the predicate function returns true is accurate and efficient.Complete the JSDoc comment to fully describe the function's behavior:
/** * `takeWhile` takes elements from the beginning of an array while the given * predicate function returns true. * @param array - The input array. * @param fn - A predicate function that takes an element and returns a boolean. * @returns A new array containing elements from the beginning of the input array * until the predicate function returns false. */test/view/selection.test.ts (4)
13-13: Good use of template literal for HTML content.The change to use a template literal with the
htmlcomment improves readability and enables better syntax highlighting in IDEs. This is a good practice.Consider applying this change consistently to all HTML strings in the file for uniformity.
29-58: Well-structured test suite forpathOffunction.The new test suite for
pathOfis comprehensive and mirrors the structure of the existingoffsetOftests. It covers various scenarios including nested elements and error cases, providing good coverage for the new function.Consider adding a test case for an empty element (e.g.,
<br>) to ensure the function handles all possible DOM structures correctly.
30-30: Consider improving test descriptions for clarity.While the consistency in test descriptions between
offsetOfandpathOfis good, they could be more specific to each function's purpose. For example:
- "should convert DOM position to offset" for
offsetOf- "should convert DOM position to path" for
pathOfThis would make the purpose of each test suite clearer at a glance.
Also applies to: 36-36, 51-51
Line range hint
1-58: Good overall test structure and coverage.The parallel structure between
offsetOfandpathOftests is excellent for consistency and readability. The test coverage appears comprehensive, including error cases for both functions.Consider adding the following test cases to further improve coverage:
- Test with a deeply nested DOM structure to ensure correct handling of complex paths.
- Test with text nodes that contain special characters or emojis.
- Test the behavior when dealing with comment nodes.
src/view/view.ts (2)
53-54: Improved input handling implementation.The changes to
handleBeforeInputsimplify the code and align well with the new command structure. This is a good improvement.Consider adding error handling or logging for unexpected scenarios. For example:
if (CommonInputEventTypes.includes(event.inputType)) { const range = toRange(event.getTargetRanges()[0], this.container); const text = this.getTextFromEvent(event); try { this.notify(insertText(range, text)); } catch (error) { console.error('Error in handleBeforeInput:', error); // Optionally, add more specific error handling here } }This would help with debugging and make the code more robust.
Also applies to: 59-60
80-82: New getSelection method added.The new
getSelectionmethod is a good addition, providing a way to retrieve the current selection from the container.Consider adding a brief JSDoc comment to describe the method's purpose and return value. For example:
/** * Retrieves the current selection from the container. * @returns {Range | undefined} The current selection range, or undefined if no selection exists. */ getSelection(): Range | undefined { return getSelection(this.container); }This would improve the code's self-documentation and make it easier for other developers to understand and use the method.
test/model/nodes.test.ts (2)
33-70: LGTM: 'Nodes.Between' tests are thorough and well-implemented.The 'Nodes.Between' describe block contains three comprehensive test cases that cover important scenarios:
- Iterating over nodes between two positions
- Iterating over a text node
- Handling the case when start and end positions are the same
All tests use clear XML examples and appropriate assertions to verify the expected behavior.
Consider extracting the common logic for converting nodes to XML into a helper function to improve code reusability. For example:
function nodesToXML(container: Element, start: Pos, end: Pos): string { return Array.from(nodesBetween(container, start, end)) .map(([node, type]) => tokenToXML(node, type)) .join(''); }This function could then be used in all three test cases, reducing duplication and improving maintainability.
72-100: LGTM: 'Nodes' tests cover essential node manipulation operations.The 'Nodes' describe block contains three well-implemented test cases that cover important node manipulation operations:
- Serializing to XML
- Splitting a text node
- Inserting a node after another and removing a node
All tests use clear XML examples and appropriate assertions to verify the expected behavior.
Consider splitting the third test case (lines 87-99) into two separate test cases for better test isolation:
- A test case for inserting a node after another
- A test case for removing a node
This separation will make the tests more focused and easier to maintain. For example:
it('should insert a node after another', () => { const val = /*html*/ `<p>Hello</p>`; const container = plainTextSchema.fromXML(val) as Element; const node = insertAfter(container.children![0], { type: 'text', text: ', world!', })[0]; expect(toXML(node!)).toEqual(', world!'); expect(toXML(container)).toEqual(/*html*/ `<p>Hello, world!</p>`); }); it('should remove a node', () => { const val = /*html*/ `<p>Hello, world!</p>`; const container = plainTextSchema.fromXML(val) as Element; removeNode(container.children![0]); expect(toXML(container)).toEqual(/*html*/ `<p></p>`); });This approach will make it easier to identify which specific operation might be failing if a test breaks in the future.
src/commands/commands.ts (2)
38-38: Specify the type of theopsarray for clarityFor better type safety and code readability, consider specifying the type of the
opsarray when initializing it. This makes the code more explicit and helps prevent potential type-related issues.Apply this diff:
- const ops = []; + const ops: Operation[] = [];
45-47: Use object property shorthand foropsSince the property name and the variable name are the same, you can use the object property shorthand syntax for brevity.
Apply this diff:
return { - ops: ops, + ops, };src/view/selection.ts (1)
118-121: Clarify the Purpose oftoIndexRangeFunctionThe
toIndexRangefunction converts anAbstractRangeto anIndexRangeusing theoffsetOffunction. Since there are bothtoRangeandtoIndexRangefunctions, please ensure that their purposes are clearly differentiated.Consider enhancing the documentation comments for both functions to explain their distinct roles and how they should be used, preventing potential confusion for future maintainers.
src/model/model.ts (1)
95-95: Rename 'withSplitText' parameter for clarityThe parameter name
withSplitTextcould be more concise. Renaming it tosplitTextimproves readability and better reflects its purpose.Apply this diff to rename the parameter:
- nodePosOf(path: Array<number>, withSplitText: boolean = true): NodePos { + nodePosOf(path: Array<number>, splitText: boolean = true): NodePos {And update the reference inside the method:
- if (!withSplitText) { + if (!splitText) {Also applies to: 97-97
src/model/nodes.ts (2)
114-131: Handle return value insplitTextfunctionThe
splitTextfunction returns a newTextnode when a split occurs, but it doesn't handle the case where no split is performed (i.e., when it returnsundefined). Ensure that the calling code properly handles theundefinedcase to prevent potential runtime errors.
172-185: Ensure proper cleanup inremoveNodeAfter removing the node from
parent.children, thenode.parentreference still points to the old parent. Consider settingnode.parenttoundefinedto fully detach the node.Apply this diff to update the function:
children.splice(index, 1); + node.parent = undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- src/commands.ts (0 hunks)
- src/commands/commands.ts (1 hunks)
- src/editor.ts (6 hunks)
- src/main.ts (1 hunks)
- src/model/model.ts (1 hunks)
- src/model/node.ts (0 hunks)
- src/model/nodepos.ts (1 hunks)
- src/model/nodes.ts (1 hunks)
- src/model/operations.ts (1 hunks)
- src/model/range.ts (1 hunks)
- src/model/schema.ts (4 hunks)
- src/model/types.ts (1 hunks)
- src/plugins/devtools.ts (1 hunks)
- src/range.ts (0 hunks)
- src/utils/array.ts (1 hunks)
- src/view/selection.ts (2 hunks)
- src/view/view.ts (3 hunks)
- test/model/helper.ts (1 hunks)
- test/model/model.test.ts (1 hunks)
- test/model/nodes.test.ts (1 hunks)
- test/model/schema.test.ts (2 hunks)
- test/util/array.test.ts (1 hunks)
- test/view/selection.test.ts (3 hunks)
💤 Files with no reviewable changes (3)
- src/commands.ts
- src/model/node.ts
- src/range.ts
🧰 Additional context used
🪛 Biome
src/model/schema.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (47)
src/model/range.ts (2)
1-1: LGTM: Import statement is correct.The import statement correctly imports the
Rangetype from a local types file, which is a good practice for type management and code organization.
5-8: LGTM: Function implementation is correct and efficient.The
isCollapsedfunction correctly checks if a range is collapsed by comparing the length and elements of the start and end arrays. The use ofArray.everymethod is an efficient way to compare array elements.src/main.ts (2)
9-9: Improved initial value structureThe change from a plain string to an HTML string (
'<p>Hello,</p>') for theinitialValueis a good improvement. This structured format aligns well with the newly introduced schema and provides better semantic meaning to the initial content.
9-14: Comprehensive update enhancing editor structureThe changes in this file, particularly the introduction of structured initial content and a schema, represent a significant step towards a more robust and flexible editor. These modifications align well with the PR objective of applying a schema to the model.
To ensure consistency across the codebase, let's verify the usage of the updated
Editor.createmethod:This will help us confirm that all calls to
Editor.createhave been updated to include the newschemaparameter.✅ Verification successful
Verified:
Editor.createusage is consistent across the codebaseAll instances of
Editor.createhave been updated to include theschemaparameter, ensuring consistency and alignment with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Editor.create method across the codebase # Test: Search for Editor.create usage rg --type typescript "Editor\.create" -A 5Length of output: 79
Script:
#!/bin/bash # Description: Verify the usage of Editor.create method across the codebase using correct file type # Test: Search for Editor.create usage in TypeScript files rg --type ts "Editor\.create" -A 5Length of output: 277
src/model/operations.ts (3)
1-1: LGTM: Imports are concise and specific.The import statement is well-structured, importing only the necessary types (
NodeandRange) from the localtypesmodule. This approach promotes clean and efficient code organization.
3-7: LGTM: Well-definedOperationtype with clear documentation.The
Operationtype is well-defined as a union ofEditOperationandMoveOperation. The accompanying JSDoc comment clearly explains its purpose, which is crucial for maintaining readable and self-documenting code.
1-27: LGTM: Well-structured and consistent type definitions.The overall structure of this file is excellent. It follows good TypeScript practices with clear, well-documented type definitions. The use of union types for
Operationand specific types forEditOperationandMoveOperationallows for flexible and type-safe operations in the editor.The file maintains a single focus on defining operation types, which is good for modularity and separation of concerns. This structure will make it easier to maintain and extend the operations in the future.
test/model/helper.ts (4)
1-3: LGTM: Import statements are appropriate.The import statements are concise and relevant to the file's purpose. They correctly import the necessary types and classes from both the source and the current directory.
5-12: LGTM: PlainTextSpec is well-defined and documented.The
PlainTextSpecconstant provides a clear and logical schema for a plain text document. The structure with a root element containing paragraphs, which in turn contain text nodes, is appropriate. The JSDoc comment enhances code readability.
28-28: LGTM: plainTextSchema is correctly instantiated.The
plainTextSchemaconstant is properly created using theSchemaclass and the previously definedPlainTextSpec. This provides a convenient, ready-to-use schema instance for plain text documents in tests.
1-28: Overall, excellent implementation of the test helper file.This new file,
test/model/helper.ts, is well-structured and serves its purpose effectively. It introduces a clear schema for plain text documents and provides useful utility functions for testing. The code is clean, follows good practices, and is appropriately documented. The exported entities are properly defined and will be valuable for testing the model implementation.src/plugins/devtools.ts (1)
24-24: Confirm the intention behind changing to XML representationThe modification from
getValue()totoXML()changes the format of the editor model's content from plain text to XML in the Devtools plugin. This change aligns with the PR objective of applying a schema to the model, as XML is commonly used for structured data representation.However, please consider the following points:
- Ensure that this change is intentional and consistent with the overall direction of the project.
- Verify that all consumers of the Devtools plugin output are updated to handle XML instead of plain text.
- Consider adding a comment explaining the rationale behind using XML representation for debugging purposes.
To ensure consistency across the codebase, let's check for other occurrences of
getValue()that might need similar updates:✅ Verification successful
To address the issue with the unrecognized file type, please run the following scripts to search for other occurrences of
getValue()in TypeScript files:
Verified: No Additional Usages of
getValue()FoundAfter searching the codebase, no other usages of
getValue()were found beyond the method declaration insrc/view/view.ts. This confirms that the change insrc/plugins/devtools.tsis isolated and does not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of getValue() in the codebase rg --type typescript 'getValue\(\)'Length of output: 72
Script:
#!/bin/bash # Search for getValue() in TypeScript files using the correct type identifier rg --type ts 'getValue\(\)'Length of output: 69
Script:
#!/bin/bash # Alternative search for getValue() using a glob pattern for .ts files rg 'getValue\(\)' -g '*.ts'Length of output: 69
test/model/model.test.ts (2)
1-3: LGTM: Imports are appropriate and well-structured.The import statements are concise and import only the necessary components. The use of a custom
PlainTextSpecfrom a helper file is a good practice for test organization.
5-5: LGTM: Well-structured test suite.The test suite is properly organized using a
describeblock for the 'model', which provides a clear context for the contained tests.Also applies to: 23-23
src/model/nodepos.ts (2)
1-2: LGTM: Import statements are appropriate.The import statements correctly bring in the necessary types and functions used in this file.
1-41: Overall assessment: Well-implemented utility functions with minor documentation improvements suggested.The
src/model/nodepos.tsfile introduces three well-implemented utility functions for working with node positions and ranges. The logic in each function is correct and efficient. The only suggestion is to enhance the JSDoc comments for better documentation.Great job on implementing these utility functions! They will likely prove very useful for working with node positions and ranges in the model.
test/model/schema.test.ts (1)
3-3: LGTM: New import for XML conversion.The import of
toXMLfunction is correctly added and is necessary for the new test case.src/model/types.ts (5)
1-4: LGTM: Node type definition is clear and flexible.The
Nodetype as a union ofText | Elementprovides a good foundation for representing different node types in the model. This approach allows for easy extension if new node types are needed in the future.
16-23: LGTM: Text type definition is clear and consistent.The
Texttype definition is well-structured and consistent with theElementtype. The fixedtypeproperty helps in easily identifying text nodes, and the optionalparentproperty allows for flexibility in the node structure.
25-34: LGTM: Path type definition is simple and well-documented.The
Pathtype as an array of numbers is an elegant solution for representing positions in the editor content without direct node references. The detailed comment with examples greatly enhances the understanding of this concept.
53-65: LGTM: IndexRange type definition is well-documented and clear.The
IndexRangetype effectively represents a range using start and end indexes in the XML-like content. The detailed comment with a visual example greatly enhances the understanding of this concept.
1-65: Overall, excellent type definitions for the editor model.This file provides a comprehensive and well-structured set of types for the editor model. The use of descriptive comments, especially with examples, greatly enhances the understanding of each type. The types are designed to work together cohesively, providing a solid foundation for the model implementation.
A few minor suggestions were made to further improve the type definitions:
- Consider making the
childrenproperty inElementnon-optional with a default empty array.- Expand the documentation for the
Rangetype to include an example.- Add a utility function to convert
NodePostoPath.These changes would further enhance the usability and clarity of the type system.
test/util/array.test.ts (2)
1-2: LGTM: Imports are correct and properly structured.The import statements are appropriate for the test file, importing necessary testing functions from 'vitest' and the utility functions being tested.
1-52: Overall, excellent test coverage with minor suggestions for improvement.This new test file provides comprehensive coverage for the
groupByandtakeWhileutility functions. The tests are well-structured, clear, and cover various scenarios including edge cases. The suggestions for additional test cases (non-primitive types forgroupByand immediate termination fortakeWhile) would further enhance the robustness of the test suite.Great job on writing these tests! They will significantly contribute to the reliability of the utility functions.
src/utils/array.ts (3)
1-7: LGTM! Well-documented and type-safe implementation.The
firstOffunction is correctly implemented, with proper type safety using generics. The JSDoc comment accurately describes the function's behavior, including the edge case of an empty array.
9-15: LGTM! Consistent withfirstOfand well-documented.The
lastOffunction is correctly implemented, maintaining consistency with thefirstOffunction in terms of type safety and documentation. The logic to access the last element is correct.
1-73: Overall, well-implemented array utility functions with good type safety.This file provides a comprehensive set of array utility functions that are well-implemented and type-safe. The use of generics ensures flexibility and type correctness across different use cases. The suggested improvements are minor and focus on enhancing documentation and simplifying the
groupByfunction for better readability.Great job on creating these utility functions! They will likely prove very useful throughout the project.
test/view/selection.test.ts (1)
2-2: LGTM: Import statement updated correctly.The import statement has been properly updated to include the new
pathOffunction alongsideoffsetOf. This change aligns with the introduction of the newpathOftest suite.src/view/view.ts (2)
1-1: LGTM: Import statements updated correctly.The new imports align well with the changes made in the file, particularly for the
handleBeforeInputmethod and the newgetSelectionmethod.Also applies to: 4-5
Line range hint
1-82: Verify integration with the rest of the codebase.The changes in this file look good and align well with the PR objectives. However, given that this is part of a larger refactoring effort to apply a schema to the model, it would be beneficial to verify how these changes integrate with other parts of the codebase.
To ensure smooth integration, please run the following script to check for any potential issues:
This script will help identify any inconsistencies or potential issues related to the recent changes in the View class.
test/model/nodes.test.ts (3)
1-12: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary testing utilities and functions required for the tests. There are no unused imports visible.
14-31: LGTM: 'Nodes.Pos' tests are comprehensive and well-structured.The 'Nodes.Pos' describe block contains two well-written test cases that cover important functionality:
- Converting a Path to a Pos
- Converting a node to a path
Both tests use clear XML examples and appropriate assertions to verify the expected behavior.
1-100: Overall, excellent test coverage and implementation.This new test file
test/model/nodes.test.tsprovides comprehensive coverage for node manipulation functionalities. The tests are well-structured, use clear examples, and cover important edge cases. The use of describe blocks helps organize the tests logically.Key strengths:
- Clear and concise test cases
- Good coverage of different node manipulation scenarios
- Appropriate use of assertions and setup
Minor suggestions for improvement have been made to enhance code reusability and test isolation. These changes would further improve the maintainability and clarity of the tests.
Great job on implementing these tests! They will significantly contribute to the reliability of the node manipulation functions in the codebase.
src/commands/commands.ts (2)
5-7:Commandtype definition is appropriateThe
Commandtype correctly defines a structure with an array ofOperationobjects, which will be used to represent a sequence of operations in a command.
9-19:insertTextfunction correctly creates a text insert commandThe
insertTextfunction properly constructs a command that inserts text at a given range. It encapsulates the operation details accurately, ensuring that text insertion is handled correctly.src/editor.ts (4)
33-36: DefaultEditorOptionsensure a schema is always providedThe
createmethod now defaults theoptsparameter to{ schema: BasicSchema }, which guarantees that a schema is supplied even if none is provided by the user. This change ensures that the editor has a schema to work with by default, and the optionalinitialValueandpluginsare handled appropriately.
46-46: Model initialization updated to include schemaThe
Model.createmethod now takesopts.schemaas the first argument. This ensures that the model is initialized with the correct schema, aligning the model with the provided or default schema specification.
58-58: Initialize the view with the model's XML representationUpdating
this.view.setValueto usethis.model.toXML()ensures that the view is initialized with the correct XML representation of the model's content. This change aligns the view's content with the model's structure.
135-140:insertTextmethod correctly handles text insertionThe updated
insertTextmethod now retrieves the current selection from the view and defaults to the end of the model's content if no selection exists. This logic ensures that text insertion operates correctly in all scenarios. The use ofthis.execute(insertText(range, text))effectively applies the insertion command to the model.src/view/selection.ts (4)
1-2: Imports Are CorrectThe import statements correctly include the necessary types and utility functions, ensuring that all dependencies are accounted for.
8-8:PositionType DefinitionThe
Positiontype is appropriately defined as a tuple of aNodeand anumber, representing a position within the DOM. This aligns with its usage in the code and ensures type safety.
11-19: Implementation ofgetSelectionFunctionThe
getSelectionfunction accurately retrieves the current selection from the window and converts it to aRangeusing thetoRangefunction. The check for an empty selection is correctly handled, returningundefinedwhen there is no selection.
37-73:pathOfFunction LogicThe
pathOffunction effectively computes the path of a given position within the container by:
- Traversing up the DOM tree to the container.
- Grouping adjacent text nodes to treat them as a single node.
- Calculating offsets within text node groups.
This approach ensures accurate path calculations within complex DOM structures.
src/model/schema.ts (2)
1-1: Imports correctly updated to use NoteNode and NoteElementThe imports are appropriately updated to reflect the new type definitions.
88-90:⚠️ Potential issueRename inner function to prevent name shadowing
The
fromDOMmethod contains an inner function also namedfromDOM, which can cause confusion and potential scope issues. Rename the inner function to avoid shadowing the outer method.Apply this diff to rename the inner function:
- function fromDOM(domNode: Node): NoteNode { + function parseDOMNode(domNode: Node): NoteNode {Ensure all references to the inner function are updated accordingly.
Likely invalid or redundant comment.
src/model/nodes.ts (2)
154-167: Check for negative index ininsertAfterSimilarly, in the
insertAfterfunction, verify thatprevis always found inparent.childrento prevent unexpected behavior whenindexOfreturns-1.Run the following script to check the validity:
#!/bin/bash # Description: Verify that 'prev' is always found in 'parent.children' # Test: Search for all calls to 'insertAfter' and ensure 'prev' is a child of 'parent' rg --type ts 'insertAfter\((\w+),' -A 5
136-149: Check for negative index ininsertBeforeIn the
insertBeforefunction, the index ofnextinparent.childrenmight be-1ifnextis not found. Although this is unlikely ifnextis a valid child, adding a check can prevent unexpected behavior.Run the following script to verify that
nextis always a child ofparent:✅ Verification successful
Verification Successful: No instances of
insertBeforewherenextis not a child ofparentwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'next' is always found in 'parent.children' # Test: Search for all calls to 'insertBefore' and ensure 'next' is a child of 'parent' rg --type ts 'insertBefore\((\w+),' -A 5Length of output: 349
Script:
#!/bin/bash # Description: Verify that 'next' is always found in 'parent.children' # Test: Search for all calls to 'insertBefore' and ensure 'next' is a child of 'parent' rg --type ts 'insertBefore\((\w+),' -A 5Length of output: 42
Script:
#!/bin/bash # Description: Verify that 'next' is always found in 'parent.children' # Test 1: Refined rg command to search for all calls to 'insertBefore' with improved regex rg --type ts 'insertBefore\s*\(\s*(\w+)\s*,' -A 5 # Test 2: Use ast-grep to find all 'insertBefore' function calls and extract the first argument ast-grep --lang typescript --pattern 'call_expression:insertBefore($_, ...)' --json .Length of output: 142
src/model/range.ts
Outdated
| export function isCollapsed(range: Range): boolean { | ||
| return range.s.length === range.e.length && | ||
| range.s.every((s, i) => s === range.e[i]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling edge cases.
While the current implementation works correctly for valid Range objects, it doesn't handle potential edge cases such as null or undefined values. Consider adding a null check at the beginning of the function.
Here's a suggested improvement:
export function isCollapsed(range: Range): boolean {
if (!range || !range.s || !range.e) {
return false; // or throw an error, depending on your error handling strategy
}
return range.s.length === range.e.length &&
range.s.every((s, i) => s === range.e[i]);
}| const index = | ||
| takeWhile(groups[i], (c) => c !== node).reduce( | ||
| (acc, c) => acc + (c.textContent || '').length, | ||
| 0, | ||
| ) + offset; | ||
| path.unshift(index); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Index Calculation in pathOf Function
The index calculation reduces over the text nodes to accumulate their lengths:
const index =
takeWhile(groups[i], (c) => c !== node).reduce(
(acc, c) => acc + (c.textContent || '').length,
0,
) + offset;Consider optimizing this calculation for better performance, especially when dealing with a large number of text nodes. Possible improvements include:
- Caching text content lengths if the same calculations occur frequently.
- Minimizing the use of high-order functions within performance-critical loops.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (13)
src/model/operations.ts (2)
9-17: LGTM: Well-definedEditOperationtype, but consider updating comments.The
EditOperationtype is well-structured for editing operations. However, there's a comment about aReplaceOperationthat isn't implemented.Consider either removing the
ReplaceOperationcomment or updating it to clarify its future implementation status. For example:/** * `EditOperation` represents an operation that edits a range of nodes. * * Note: A future enhancement may include a `ReplaceOperation` to replace * a range of nodes with another range of nodes. */
1-27: LGTM: Well-structured type definitions aligning with PR objectives.This file introduces clear and well-structured type definitions for editor operations, which aligns well with the PR objective of applying schema to the model. The
Operation,EditOperation, andMoveOperationtypes provide a solid foundation for implementing and managing editor operations.The exported types will be useful for other parts of the codebase that need to work with these operations. Overall, this is a good addition to the project's type system.
As the project evolves, consider the following:
- Implement the mentioned
ReplaceOperationif it becomes necessary.- Ensure that these types are used consistently across the codebase when dealing with editor operations.
- Consider adding utility functions or methods to work with these types, such as type guards or operation creators, in a separate file to keep this file focused on type definitions.
src/editor.ts (3)
8-8: LGTM: Schema support addedThe import of
SchemaSpecandBasicSchemaindicates the addition of schema support to the editor. This is a valuable feature for content structure and validation.Consider adding documentation comments to explain the purpose and usage of these schema-related imports, especially for developers who might be new to the concept.
15-15: LGTM: Schema option added with good defaultThe addition of the
schemaproperty toEditorOptionsand the use ofBasicSchemaas a default in thecreatemethod is a good approach. It allows for custom schemas while maintaining backwards compatibility.Consider adding a brief comment above the
schemaproperty inEditorOptionsto explain its purpose and any requirements for custom schemas.Also applies to: 33-36
135-140: LGTM: Improved insertText methodThe refactored
insertTextmethod now correctly handles the case when there's no current selection by using the model's content end range. This is a good improvement in the text insertion logic.Consider adding a brief comment explaining the fallback behavior when there's no selection, to make the code more self-documenting.
src/view/selection.ts (2)
21-32: LGTM:toRangefunction is well-implemented, with a minor suggestion.The
toRangefunction correctly converts anAbstractRangeto aRange, handling both collapsed and non-collapsed ranges appropriately. It makes good use of thepathOffunction.Consider adding a type annotation for the return value to improve code readability:
export function toRange(range: AbstractRange, container: Node): Range { // ... existing implementation ... }
34-73: LGTM:pathOffunction is well-implemented, with suggestions for improvement.The
pathOffunction correctly computes the path of a node within a container, handling the complexities of DOM traversal and text node grouping.Consider the following improvements:
- Extract the text node handling logic into a separate function to improve readability:
function calculateTextNodeIndex(group: Node[], node: Node, offset: number): number { return takeWhile(group, (c) => c !== node).reduce( (acc, c) => acc + (c.textContent || '').length, 0 ) + offset; }
- Use early returns to reduce nesting:
if (node.nodeType === Node.TEXT_NODE) { const index = calculateTextNodeIndex(groups[i], node as Node, offset); path.unshift(index); } path.unshift(i); node = parent;These changes would make the function more modular and easier to understand.
src/model/schema.ts (2)
11-18: LGTM: Useful addition of BasicSchemaThe introduction of
BasicSchemais a valuable addition, providing a default schema for a simple text editor. This serves as a good example for users and can be a helpful starting point.Consider adding a brief comment for each node type in the schema to explain their purpose, e.g.:
export const BasicSchema: SchemaSpec = { root: { children: 'p*' }, // Root node, can contain multiple paragraphs p: { children: 'text*' }, // Paragraph node, can contain multiple text nodes text: {}, // Text node, represents plain text content };This would further enhance the documentation and make the schema even more self-explanatory.
88-136: LGTM: Valuable addition of fromDOM and fromXML methodsThe new
fromDOMandfromXMLmethods are excellent additions to theSchemaclass, significantly enhancing its capabilities for working with different data formats. The implementation is thorough and handles various scenarios well.A few suggestions for further improvement:
- Consider adding input validation for the
fromXMLmethod to ensure the input is a non-empty string.- In the
fromDOMmethod, you might want to handle potential errors fromDOMParserwhen parsing invalid XML.- For better type safety, consider using a type guard for the
domElem as Elementcast in thefromDOMmethod.Example for point 3:
if (domNode instanceof Element) { const attrs: Record<string, string> = {}; for (const { name, value } of domNode.attributes) { attrs[name] = value; } if (Object.keys(attrs).length > 0) { node.attrs = attrs; } }These changes would make the methods more robust and type-safe.
src/model/nodes.ts (4)
11-24: LGTM: toXML function with suggestion for enhancementThe
toXMLfunction correctly converts nodes to their XML string representation, handling both text and element nodes appropriately. The recursive approach for element nodes is well-implemented.Consider enhancing the function to handle element attributes if they are part of your
Elementtype. This would provide a more complete XML representation.
26-37: LGTM: toNodePos function with suggestion for error handlingThe
toNodePosfunction effectively converts a path to a node position. The use ofinitialOfandlastOfutility functions is appropriate.Consider adding error handling for cases where the path is invalid or intermediate nodes are missing. This would make the function more robust and prevent potential runtime errors.
112-132: LGTM: splitText function with minor optimization suggestionThe
splitTextfunction correctly splits a text node at a given position, handling edge cases appropriately. The implementation is correct and efficient.Consider a minor optimization: you can combine the creation and insertion of the new node into a single step:
return insertAfter(text, { type: 'text', text: next })[0] as Text;This eliminates the need for the intermediate
newNodevariable.
152-168: LGTM: insertAfter function with refactoring suggestionThe
insertAfterfunction correctly inserts new nodes after a given node. It properly handles multiple nodes and updates parent references.Consider refactoring
insertBeforeandinsertAfterto reduce code duplication. You could create a private helper function that both methods use:function insertNodes(sibling: Node, newNodes: Array<Node>, after: boolean): Array<Node> { const parent = sibling.parent; if (!parent) { throw new Error('node does not have a parent'); } const index = parent.children!.indexOf(sibling) + (after ? 1 : 0); parent.children!.splice(index, 0, ...newNodes); for (const node of newNodes) { node.parent = parent; } return newNodes; } export function insertBefore(next: Node, ...newNodes: Array<Node>): Array<Node> { return insertNodes(next, newNodes, false); } export function insertAfter(prev: Node, ...newNodes: Array<Node>): Array<Node> { return insertNodes(prev, newNodes, true); }This approach would reduce code duplication and make future maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- src/commands.ts (0 hunks)
- src/commands/commands.ts (1 hunks)
- src/editor.ts (6 hunks)
- src/main.ts (1 hunks)
- src/model/model.ts (1 hunks)
- src/model/node.ts (0 hunks)
- src/model/nodepos.ts (1 hunks)
- src/model/nodes.ts (1 hunks)
- src/model/operations.ts (1 hunks)
- src/model/range.ts (1 hunks)
- src/model/schema.ts (4 hunks)
- src/model/types.ts (1 hunks)
- src/plugins/devtools.ts (1 hunks)
- src/range.ts (0 hunks)
- src/utils/array.ts (1 hunks)
- src/view/selection.ts (2 hunks)
- src/view/view.ts (3 hunks)
- test/model/helper.ts (1 hunks)
- test/model/model.test.ts (1 hunks)
- test/model/nodes.test.ts (1 hunks)
- test/model/schema.test.ts (2 hunks)
- test/util/array.test.ts (1 hunks)
- test/view/selection.test.ts (3 hunks)
💤 Files with no reviewable changes (3)
- src/commands.ts
- src/model/node.ts
- src/range.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/commands/commands.ts
- src/main.ts
- src/model/range.ts
- src/model/types.ts
- src/plugins/devtools.ts
- src/utils/array.ts
- src/view/view.ts
- test/model/helper.ts
- test/model/model.test.ts
- test/model/nodes.test.ts
- test/model/schema.test.ts
- test/util/array.test.ts
- test/view/selection.test.ts
🧰 Additional context used
🔇 Additional comments (36)
src/model/operations.ts (3)
1-1: LGTM: Imports are appropriate.The imports of
NodeandRangefrom './types' are suitable for the operations being defined in this file.
3-7: LGTM: Well-definedOperationtype.The
Operationtype is well-defined as a union ofEditOperationandMoveOperation. The comment provides clear documentation, and the naming follows TypeScript conventions.
19-27: LGTM: Well-definedMoveOperationtype.The
MoveOperationtype is well-structured for move operations. The comment provides clear documentation, and the use ofRangefor bothsourceandtargetproperties is appropriate for specifying the original and new positions of the nodes.src/model/nodepos.ts (5)
1-2: LGTM: Import statements are appropriate.The import statements are concise and import the necessary types and functions for this module.
4-9: LGTM:isLeftMostfunction is well-implemented and documented.The function correctly checks if a node position is at the leftmost position by verifying if the offset is 0. The JSDoc comment accurately describes the function's purpose.
11-16: LGTM:isRightMostfunction is well-implemented and documented.The function correctly checks if a node position is at the rightmost position by comparing the offset to the node's length (obtained using the
lengthOffunction). The JSDoc comment accurately describes the function's purpose.
18-23: LGTM:equalsfunction is well-implemented and documented.The function correctly checks if two node positions are equal by comparing both their
nodeandoffsetproperties. The JSDoc comment accurately describes the function's purpose.
1-23: Excellent implementation of node position utility functions.This file provides a set of well-implemented and clearly documented utility functions for working with node positions. The code is clean, concise, and follows good practices. Each function serves a specific purpose and is appropriately exported for use in other modules.
src/editor.ts (4)
7-7: LGTM: Good separation of concernsThe import of
Command,execute, andinsertTextfrom a separatecommandsmodule improves the code's modularity and organization.
46-46: LGTM: Model creation updated to include schemaThe
Model.createcall now includes theopts.schemaparameter, ensuring that the model is created with the specified or default schema. This is a good integration of the new schema functionality.
58-58: LGTM: Updated model content representationThe change from
getValue()totoXML()suggests an improvement in how the model's content is represented. This likely provides better structure or formatting for the view.
71-78: Implement the model-to-view update in the model's subscribe callbackThe subscription to the model changes currently contains a TODO to update the view, but the implementation is missing. To ensure the view reflects changes in the model, you should update the view within this callback.
Would you like assistance in implementing the update logic for the view? I can help provide code to synchronize the view with the model when the model changes.
src/view/selection.ts (3)
1-2: LGTM: Imports and type definition look good.The new imports and the
Positiontype definition enhance the module's functionality and improve code readability. ThePositiontype, representing a DOM node and an offset, is well-documented and will be useful for the new functions.Also applies to: 4-8
11-19: LGTM:getSelectionfunction is well-implemented.The
getSelectionfunction correctly retrieves the current selection and handles the case when no selection is present. It appropriately uses thetoRangefunction to convert the selection to aRange.
60-65: Address previous optimization suggestion forpathOffunction.A previous review comment suggested optimizing the index calculation in the
pathOffunction. This optimization has not been implemented in the current changes.Consider revisiting this optimization suggestion:
- Cache text content lengths if the same calculations occur frequently.
- Minimize the use of high-order functions within performance-critical loops.
These optimizations could improve performance, especially when dealing with a large number of text nodes.
src/model/schema.ts (5)
1-1: LGTM: Improved import namingThe change to import
NodeasNoteNodeandElementasNoteElementimproves clarity and helps avoid potential naming conflicts. This is a good practice, especially in larger codebases.
3-6: LGTM: Improved type definition and documentationThe explicit export of
SchemaSpecand the added documentation comment enhance the code's usability and maintainability. This change promotes better type checking across modules and improves developer understanding of the type's purpose.
9-9: LGTM: Improved type safety for TextNodeSpecThe change from
{}toRecord<string, never>forTextNodeSpecimproves type safety and follows best practices. This addresses the concern raised in the previous review comment about avoiding{}as a type. Good job on implementing this improvement.
22-22: LGTM: Improved class documentationThe updated documentation for the
Schemaclass now accurately reflects its expanded capabilities, including building models from DOM, JSON, or XML. This change provides users with a clear understanding of the class's functionality and is a valuable improvement to the codebase.
32-42: Extend support for non-text nodes in create methodThe
createmethod now includes basic functionality for creating text nodes and validates the node type. This is a good start, but as mentioned in the previous review, consider extending this method to support additional node types defined in the schema.For non-text nodes, you could create an element node with an empty children array:
if (type !== 'text') { return { type, attrs: {}, children: [] }; }This would provide a more complete implementation and avoid throwing a 'not implemented' error.
src/model/model.ts (10)
2-19: LGTM: Import statements and class declaration updatesThe new imports and the change in the
Modelclass declaration accurately reflect the transition to an operation-based model. These changes align well with the overall restructuring of theModelclass.
23-27: LGTM: New staticcreatemethodThe
createstatic method is a well-designed addition. It encapsulates the logic for creating a newModelinstance, including schema creation and initial XML parsing. This approach promotes cleaner instantiation of theModelclass.
29-37: LGTM: Updated constructor and newcreateTextmethodThe constructor changes appropriately reflect the new model structure. The addition of the
createTextmethod provides a consistent and schema-aware way to create text nodes, which is a good practice for maintaining data integrity.
39-41: LGTM:toXMLmethod implementationThe
toXMLmethod provides a clean way to convert the root node back to XML format. This is essential for serialization and maintaining compatibility with XML-based systems.
43-52: Refine error message inapplymethodThe error message in the default case of the switch statement includes the entire
opobject, which could potentially expose sensitive information or clutter logs.As previously suggested, consider modifying the error message to include only the operation type:
- throw new Error(`invalid operation type: ${op}`); + throw new Error(`invalid operation type: ${op.type}`);
54-90: LGTM: Well-implementededitmethodThe
editmethod is well-structured and handles node removal and insertion effectively. It correctly manages edge cases like collapsed ranges and uses appropriate utility functions for node manipulation. The return of an operation object aligns well with the new operation-based model structure.
95-110: LGTM: Well-implementednodePosOfmethodThe
nodePosOfmethod effectively retrieves node positions from paths and includes a useful text node splitting functionality. This implementation provides precise positioning capabilities, which is crucial for accurate text manipulation operations.
116-137: LGTM: Well-implementedgetContentEndRangemethodThe
getContentEndRangemethod effectively determines the end range of content within the model. It correctly handles both text and element nodes, and the traversal logic is clear and well-implemented. This method will be useful for operations that need to target the end of the document.
140-141: Implement the 'move' methodThe
movemethod is still not implemented, which will cause runtime errors when applying 'move' operations. This issue was previously flagged and remains unaddressed.As mentioned in the previous review, please implement the
movemethod to handle 'move' operations correctly. If you need assistance with the implementation, please let me know.
1-142: Overall assessment: Significant improvements with one remaining taskThe changes to the
Modelclass represent a substantial improvement in structure and functionality. The transition to an operation-based model is well-implemented, with clear and effective methods for handling various operations. The new structure should provide better flexibility and maintainability.However, there is one remaining task:
- Implement the
movemethod to handle 'move' operations.Once this is addressed, the
Modelclass will be fully functional and aligned with the new operation-based approach.src/model/nodes.ts (6)
1-9: LGTM: Imports and TagType definitionThe imports are appropriate for the file's functionality, and the
TagTypedefinition is clear and well-documented.
39-56: LGTM: pathOf functionThe
pathOffunction effectively computes the path of a node from the container. The implementation is correct and efficient, using a while loop to traverse up the tree andunshiftto build the path. The error handling for cases where the node is not in the container is appropriate.
103-110: LGTM: lengthOf functionThe
lengthOffunction correctly calculates the length of a node, handling both text and element nodes appropriately. For text nodes, it returns the length of the text, and for element nodes, it returns the number of children. The implementation is concise and effective.
134-150: LGTM: insertBefore functionThe
insertBeforefunction correctly inserts new nodes before a given node. It efficiently handles multiple nodes using the spread operator and properly updates the parent references. The error handling for the case where the node has no parent is appropriate.
170-186: LGTM: removeNode functionThe
removeNodefunction correctly removes a node from its parent. It includes appropriate error handling for cases where the node has no parent or is not in the parent's children array. The use ofsplicefor removal is efficient and correct.
1-186: Overall assessment: Well-implemented node management utilities with room for minor improvementsThe
src/model/nodes.tsfile provides a comprehensive set of utility functions for managing a tree structure of nodes. The implementation is generally solid, with clear and efficient functions for XML conversion, path management, and node manipulation.Key points:
- Most functions are well-implemented and appropriately handle error cases.
- The
nodesBetweenfunction requires fixing to address theinRangevariable issue and improve handling of same-node cases.- There are opportunities for minor optimizations in
splitTextand refactoring to reduce code duplication betweeninsertBeforeandinsertAfter.Addressing these points will further improve the robustness and maintainability of the code.
To ensure that the changes suggested for the
nodesBetweenfunction don't introduce regressions, please run the following verification script:This script will help verify the implementation of the
nodesBetweenfunction and its usage across the codebase, as well as identify any existing test cases that may need to be updated after applying the suggested changes.✅ Verification successful
Verification Complete:
nodesBetweenFunction is Implemented and TestedThe
nodesBetweenfunction is present insrc/model/nodes.tsand is utilized withintest/model/nodes.test.ts, indicating that it is implemented and has associated test coverage.
Implementation Found:
src/model/nodes.tscontains thenodesBetweenfunction definition.Usage in Tests:
test/model/nodes.test.tsincludes usages ofnodesBetween, suggesting existing tests cover its functionality.To ensure comprehensive coverage, please review the existing tests in
test/model/nodes.test.tsto verify that all edge cases, particularly regarding theinRangevariable and same-node scenarios, are adequately tested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the nodesBetween function implementation and its usage # Test: Search for nodesBetween function implementation echo "Searching for nodesBetween function implementation:" rg --type typescript -A 5 'function\*?\s+nodesBetween' # Test: Search for nodesBetween function usage echo "Searching for nodesBetween function usage:" rg --type typescript 'nodesBetween\(' # Test: Search for test cases related to nodesBetween echo "Searching for test cases related to nodesBetween:" rg --type typescript 'test.*nodesBetween'Length of output: 562
Script:
#!/bin/bash # Description: Verify the nodesBetween function implementation and its usage # Test: Search for nodesBetween function implementation echo "Searching for nodesBetween function implementation:" rg 'function\*?\s+nodesBetween' --glob '*.ts' --glob '*.tsx' -A 5 # Test: Search for nodesBetween function usage echo "Searching for nodesBetween function usage:" rg 'nodesBetween\(' --glob '*.ts' --glob '*.tsx' # Test: Search for test cases related to nodesBetween echo "Searching for test cases related to nodesBetween:" rg 'test.*nodesBetween' --glob '*.ts' --glob '*.tsx'Length of output: 1218
src/model/nodes.ts
Outdated
| /** | ||
| * `nodesBetween` iterates over the nodes between the start and end positions. | ||
| */ | ||
| export function* nodesBetween( | ||
| container: Element, | ||
| start: NodePos, | ||
| end: NodePos, | ||
| ): Generator<[Node, TagType]> { | ||
| let inRange = false; | ||
| function* traverse(node: Node): Generator<[Node, TagType]> { | ||
| if (node === start.node) { | ||
| inRange = true; | ||
| } | ||
|
|
||
| if (node.type === 'text') { | ||
| if (inRange) { | ||
| yield [node, 'text']; | ||
| } | ||
| } else { | ||
| const elem = node as Element; | ||
| for (const child of elem.children || []) { | ||
| if (child.type !== 'text' && inRange) { | ||
| yield [child, 'open']; | ||
| } | ||
|
|
||
| yield* traverse(child); | ||
|
|
||
| if (child.type !== 'text' && inRange) { | ||
| yield [child, 'close']; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (node === end.node) { | ||
| inRange = false; | ||
| } | ||
| } | ||
|
|
||
| if (equals(start, end)) { | ||
| return; | ||
| } | ||
|
|
||
| yield* traverse(container); | ||
| } |
There was a problem hiding this comment.
Fix the incorrect use of the inRange variable and improve same node handling
The nodesBetween function has two issues that need to be addressed:
-
The
inRangevariable is still being used incorrectly, as mentioned in the past review comment. This can lead to incorrect behavior due to shared state across recursion levels. -
The function doesn't handle the case where start and end nodes are the same but with different offsets.
To fix these issues, apply the following changes:
- Pass
inRangeas a parameter to thetraversefunction:
- let inRange = false;
- function* traverse(node: Node): Generator<[Node, TagType]> {
+ function* traverse(node: Node, inRange: boolean): Generator<[Node, TagType]> {
if (node === start.node) {
- inRange = true;
+ inRange = true;
}
// ... rest of the function ...
if (node === end.node) {
- inRange = false;
+ inRange = false;
}
}
- yield* traverse(container);
+ yield* traverse(container, false);- Update the equality check to handle same node with different offsets:
- if (equals(start, end)) {
+ if (start.node === end.node && start.offset === end.offset) {
return;
}These changes will ensure correct traversal and handling of all cases, including when start and end positions are in the same node.
📝 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.
| /** | |
| * `nodesBetween` iterates over the nodes between the start and end positions. | |
| */ | |
| export function* nodesBetween( | |
| container: Element, | |
| start: NodePos, | |
| end: NodePos, | |
| ): Generator<[Node, TagType]> { | |
| let inRange = false; | |
| function* traverse(node: Node): Generator<[Node, TagType]> { | |
| if (node === start.node) { | |
| inRange = true; | |
| } | |
| if (node.type === 'text') { | |
| if (inRange) { | |
| yield [node, 'text']; | |
| } | |
| } else { | |
| const elem = node as Element; | |
| for (const child of elem.children || []) { | |
| if (child.type !== 'text' && inRange) { | |
| yield [child, 'open']; | |
| } | |
| yield* traverse(child); | |
| if (child.type !== 'text' && inRange) { | |
| yield [child, 'close']; | |
| } | |
| } | |
| } | |
| if (node === end.node) { | |
| inRange = false; | |
| } | |
| } | |
| if (equals(start, end)) { | |
| return; | |
| } | |
| yield* traverse(container); | |
| } | |
| /** | |
| * `nodesBetween` iterates over the nodes between the start and end positions. | |
| */ | |
| export function* nodesBetween( | |
| container: Element, | |
| start: NodePos, | |
| end: NodePos, | |
| ): Generator<[Node, TagType]> { | |
| function* traverse(node: Node, inRange: boolean): Generator<[Node, TagType]> { | |
| if (node === start.node) { | |
| inRange = true; | |
| } | |
| if (node.type === 'text') { | |
| if (inRange) { | |
| yield [node, 'text']; | |
| } | |
| } else { | |
| const elem = node as Element; | |
| for (const child of elem.children || []) { | |
| if (child.type !== 'text' && inRange) { | |
| yield [child, 'open']; | |
| } | |
| yield* traverse(child, inRange); | |
| if (child.type !== 'text' && inRange) { | |
| yield [child, 'close']; | |
| } | |
| } | |
| } | |
| if (node === end.node) { | |
| inRange = false; | |
| } | |
| } | |
| if (start.node === end.node && start.offset === end.offset) { | |
| return; | |
| } | |
| yield* traverse(container, false); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (19)
src/utils/array.ts (3)
17-22: LGTM with a minor suggestion: Well-implementedinitialOffunctionThe
initialOffunction is correctly implemented, with an appropriate type signature and accurate JSDoc comment. It efficiently returns all elements of the array except the last one.Consider handling the case of an empty array explicitly to improve robustness:
export function initialOf<T>(array: Array<T>): Array<T> { return array.length > 0 ? array.slice(0, -1) : []; }This change also simplifies the slice operation by using a negative index.
31-55: LGTM with suggestions: Well-implementedgroupByfunctionThe
groupByfunction is correctly implemented, with an appropriate type signature and accurate JSDoc comment. It efficiently groups adjacent elements of an array according to the given function.Consider the following suggestions to improve readability:
- Use destructuring to simplify access to the last group and its last element.
- Use
pushinstead of direct index assignment for clarity.- Consider using
if-elseinstead of ternary operator for the initial group creation.Here's a refactored version:
export function groupBy<T>( array: Array<T>, fn: (prev: T, curr: T) => boolean, ): Array<Array<T>> { if (array.length === 0) { return []; } return array.slice(1).reduce( (groups: Array<Array<T>>, current: T): Array<Array<T>> => { const [lastGroup] = groups.slice(-1); const [lastElement] = lastGroup.slice(-1); if (fn(lastElement, current)) { lastGroup.push(current); } else { groups.push([current]); } return groups; }, [[array[0]]], ); }This refactored version maintains the same functionality while improving readability.
57-73: LGTM with a minor suggestion: Well-implementedtakeWhilefunctionThe
takeWhilefunction is correctly implemented, with an appropriate type signature and efficient logic. It accurately takes elements from the beginning of an array while the given predicate function returns true.The JSDoc comment has a minor grammatical issue. Consider updating it to:
/** * `takeWhile` takes elements from the beginning of an array while the given * predicate function returns true. */This small change improves the clarity of the comment.
src/model/nodepos.ts (3)
8-16: LGTM:toNodePosfunction is well-implemented.The function correctly converts a path to a node position by traversing the container's children. It handles the last offset appropriately in the returned NodePos.
Consider adding error handling for cases where the path might be invalid or a node in the path doesn't exist. This could prevent potential runtime errors.
21-61: LGTM:nodesBetweenfunction is well-implemented with efficient traversal.The generator function correctly iterates over nodes between start and end positions, handling both text nodes and elements appropriately. The use of a generator is efficient for potentially large structures.
Consider extracting the
traversefunction to improve readability. This would make the main function body more concise and easier to understand at a glance. Here's a suggested refactor:function* traverse(node: Node, start: NodePos, end: NodePos): Generator<[Node, TagType]> { // ... existing traverse logic ... } export function* nodesBetween( container: Element, start: NodePos, end: NodePos, ): Generator<[Node, TagType]> { if (equals(start, end)) { return; } yield* traverse(container, start, end); }
73-75: LGTM:isRightMostfunction is correctly implemented.The function accurately determines if a node position is at the rightmost position by comparing the offset to the length of the node using the
lengthOffunction.Consider caching the result of
lengthOf(pos.node)if it's an expensive operation and this function is called frequently. This could potentially improve performance in scenarios where the node length doesn't change often.export function isRightMost(pos: NodePos): boolean { const nodeLength = lengthOf(pos.node); return pos.offset === nodeLength; }test/view/selection.test.ts (3)
13-13: Good use of template literal for HTML content.The change to use a template literal with the
htmlcomment improves readability and enables better syntax highlighting in IDEs. This is a good practice.Consider applying this change consistently to all HTML strings in the file for uniformity.
29-58: Well-structured test suite forpathOffunction.The new test suite for
pathOfis comprehensive and mirrors the structure of theoffsetOfsuite, which is excellent for consistency. It covers various scenarios including simple conversion, nested elements, and error handling.For improved readability, consider adding a brief comment before each test case explaining what aspect of
pathOfit's testing. For example:// Test conversion of root-level text node it('should convert to the given position', () => { // ... }); // Test conversion in nested elements it('should convert to the given position in element', () => { // ... }); // Test error handling for invalid input it('should return error if the range is not in the container', () => { // ... });
Line range hint
4-58: Consider refactoring to reduce duplication between test suites.The
offsetOfandpathOftest suites have very similar structures and use identical HTML content for testing. While this consistency is good, there's an opportunity to reduce duplication and improve maintainability.Consider refactoring the common setup code into a shared function or using
beforeEach. For example:describe('Selection functions', () => { let container: HTMLDivElement; beforeEach(() => { container = document.createElement('div'); container.innerHTML = /*html*/ `<b>Hello</b> <i>World</i>!`; }); describe('offsetOf', () => { // offsetOf tests using shared container }); describe('pathOf', () => { // pathOf tests using shared container }); });This approach would make it easier to maintain and update both test suites simultaneously if the HTML structure needs to change in the future.
src/view/view.ts (1)
55-58: Consider creating issues for TODO items.There are several TODO comments in the
handleBeforeInputmethod that highlight areas needing further attention:
- Aggregating consecutive input events for composition text.
- Handling more input types created by the enter key.
- Handling the dummy br element created by the enter key.
Consider creating separate GitHub issues for each of these items to ensure they are tracked and addressed in future updates.
Would you like me to create GitHub issues for these TODO items?
src/model/nodes.ts (4)
12-22: LGTM with a suggestion: Consider handling element attributes in toXML function.The
toXMLfunction correctly handles both text and element nodes, using recursion for element children. However, it doesn't currently process element attributes.Consider extending the function to handle element attributes if they are part of your
Elementtype. This could be implemented as follows:export function toXML(node: Node): string { if (node.type === 'text') { return (node as Text).text; } const elem = node as Element; const attributes = elem.attributes ? Object.entries(elem.attributes) .map(([key, value]) => ` ${key}="${value}"`) .join('') : ''; const children = elem.children?.map(toXML).join('') || ''; return `<${elem.type}${attributes}>${children}</${elem.type}>`; }This assumes that
Elementhas an optionalattributesproperty of typeRecord<string, string>.
55-72: LGTM with a suggestion: Consider returning both nodes in splitText function.The
splitTextfunction correctly splits a text node at the given position and handles edge cases well. However, it modifies the original node in place, which might be unexpected.Consider returning both the original (modified) node and the new node. This makes the function's behavior more explicit and allows the caller to decide how to use both resulting nodes. Here's a suggested modification:
export function splitText(pos: NodePos): [Text, Text] | undefined { if (pos.node.type !== 'text') { return undefined; } const text = pos.node as Text; if (pos.offset === 0 || pos.offset === text.text.length) { return undefined; } const prevText = text.text.slice(0, pos.offset); const nextText = text.text.slice(pos.offset); text.text = prevText; const newNode = { type: 'text', text: nextText } as Text; insertAfter(text, newNode); return [text, newNode]; }This approach makes it clear that the original node is modified and a new node is created.
98-111: LGTM with a suggestion: Consider refactoring insertBefore and insertAfter.The
insertAfterfunction correctly inserts new nodes after a given node and is well-implemented. However, it's very similar toinsertBefore.Consider refactoring
insertBeforeandinsertAfterto reduce code duplication. You could create a private helper function that both can use:function insertNodes( referenceNode: Node, newNodes: Array<Node>, insertBefore: boolean ): Array<Node> { const parent = referenceNode.parent; if (!parent) { throw new Error('node does not have a parent'); } const index = parent.children!.indexOf(referenceNode); const insertionIndex = insertBefore ? index : index + 1; parent.children!.splice(insertionIndex, 0, ...newNodes); for (const node of newNodes) { node.parent = parent; } return newNodes; } export function insertBefore(next: Node, ...newNodes: Array<Node>): Array<Node> { return insertNodes(next, newNodes, true); } export function insertAfter(prev: Node, ...newNodes: Array<Node>): Array<Node> { return insertNodes(prev, newNodes, false); }This approach reduces code duplication and makes it easier to maintain both functions.
116-129: LGTM with a suggestion: Update removed node's parent reference in removeNode.The
removeNodefunction correctly removes a node from its parent's children array and includes appropriate error handling.Consider updating the removed node's parent reference to null to avoid potential issues with dangling references. Here's a suggested modification:
export function removeNode(node: Node) { const parent = node.parent; if (!parent) { throw new Error('node does not have a parent'); } const children = parent.children!; const index = children.indexOf(node); if (index === -1) { throw new Error('node is not in the parent'); } children.splice(index, 1); node.parent = null; // Update the removed node's parent reference }This ensures that the removed node no longer references its former parent, which can help prevent bugs and make garbage collection more efficient.
src/view/selection.ts (2)
11-19: LGTM:getSelectionfunction is well-implemented.The
getSelectionfunction correctly retrieves the current selection and converts it to aRange. It properly handles the case when no selection is present.Consider adding a type annotation for the
selectionvariable to improve code readability:const selection: Selection | null = window.getSelection();
21-32: LGTM:toRangefunction is well-implemented.The
toRangefunction correctly converts anAbstractRangeto aRange, handling both collapsed and non-collapsed ranges appropriately.For improved clarity, consider adding a comment explaining the difference between
AbstractRangeandRange:/** * `toRange` converts the abstract range to the path range. * AbstractRange is a browser-provided type, while Range is our custom type using paths. */src/model/model.ts (3)
22-32: LGTM: Staticcreatemethod and constructor updated appropriately.The new
createmethod and the updated constructor align well with the new structure of theModelclass. However, consider adding a comment to explain the purpose of thecreatemethod, especially its relationship to the constructor.Consider adding a comment above the
createmethod:/** * Creates a new Model instance from a schema specification and initial XML. * This method handles the creation of the Schema and parsing of the initial value. */ static create(spec: SchemaSpec, initialValue: string): Model { // ... existing code ... }
58-91: LGTM:editmethod implemented correctly with a suggestion for improvement.The
editmethod is well-structured and handles various scenarios correctly. The comment explaining the need to split the text node at the end of the range is helpful.Consider adding type annotations to the
for...ofloop for better readability:for (const [node, type] of nodesBetween(this.root, start, end) as [Node, string][]) { // ... existing code ... }This makes it clear what types
nodeandtypeare expected to be.
96-107: Current implementation ofrangeOfis functional but needs improvement.The
rangeOfmethod works for the current implementation, but as noted in the TODO comment, it assumes the first and last nodes are always text nodes. This limitation could lead to issues in more complex scenarios.To ensure this limitation is addressed in the future:
Would you like me to create a GitHub issue to track the implementation of handling element nodes in the
rangeOfmethod?Consider adding a more detailed comment explaining the current limitations and the expected behavior for element nodes. This will help future developers understand the context of the TODO.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
- src/commands.ts (0 hunks)
- src/commands/commands.ts (1 hunks)
- src/editor.ts (6 hunks)
- src/main.ts (1 hunks)
- src/model/model.ts (1 hunks)
- src/model/node.ts (0 hunks)
- src/model/nodepos.ts (1 hunks)
- src/model/nodes.ts (1 hunks)
- src/model/operations.ts (1 hunks)
- src/model/range.ts (1 hunks)
- src/model/schema.ts (4 hunks)
- src/model/types.ts (1 hunks)
- src/plugins/devtools.ts (1 hunks)
- src/range.ts (0 hunks)
- src/utils/array.ts (1 hunks)
- src/view/selection.ts (2 hunks)
- src/view/view.ts (3 hunks)
- test/model/helper.ts (1 hunks)
- test/model/model.test.ts (1 hunks)
- test/model/nodepos.test.ts (1 hunks)
- test/model/nodes.test.ts (1 hunks)
- test/model/schema.test.ts (2 hunks)
- test/util/array.test.ts (1 hunks)
- test/view/selection.test.ts (3 hunks)
💤 Files with no reviewable changes (3)
- src/commands.ts
- src/model/node.ts
- src/range.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/commands/commands.ts
- src/editor.ts
- src/main.ts
- src/model/operations.ts
- src/model/range.ts
- src/model/schema.ts
- src/model/types.ts
- src/plugins/devtools.ts
- test/model/helper.ts
- test/model/model.test.ts
- test/model/nodes.test.ts
- test/model/schema.test.ts
- test/util/array.test.ts
🧰 Additional context used
🔇 Additional comments (32)
src/utils/array.ts (4)
1-7: LGTM: Well-implementedfirstOffunctionThe
firstOffunction is correctly implemented, with an appropriate type signature and accurate JSDoc comment. It efficiently returns the first element of the array orundefinedfor empty arrays.
9-15: LGTM: Well-implementedlastOffunctionThe
lastOffunction is correctly implemented, with an appropriate type signature and accurate JSDoc comment. It efficiently returns the last element of the array orundefinedfor empty arrays.
24-29: LGTM: Well-implementedtailOffunctionThe
tailOffunction is correctly implemented, with an appropriate type signature and accurate JSDoc comment. It efficiently returns all elements of the array except the first one, and handles empty arrays correctly.
1-73: Overall assessment: Well-implemented array utility functionsThis file introduces a set of useful array utility functions that are generally well-implemented. The functions are concise, efficient, and come with appropriate type signatures and JSDoc comments. Minor suggestions have been made for some functions to improve readability or robustness, but these are not critical issues. The utility functions provided here will likely be valuable for array manipulations throughout the project.
src/model/nodepos.ts (4)
1-3: LGTM: Import statements are appropriate.The import statements are concise and relevant to the functionality implemented in this file. They import necessary types, utility functions, and constants from various modules.
66-68: LGTM:isLeftMostfunction is correctly implemented.The function provides a simple and accurate check for determining if a node position is at the leftmost position by verifying if the offset is 0.
80-82: LGTM:equalsfunction is correctly implemented.The function provides a simple and accurate comparison of two node positions by checking both the node and offset equality. This implementation ensures that two positions are considered equal only if they refer to the same node and the same offset within that node.
1-82: Overall, the implementation insrc/model/nodepos.tsis well-structured and robust.The file introduces several utility functions for working with node positions, which appear to be crucial for the schema application to the model. The functions are generally well-implemented, with clear purposes and efficient algorithms.
A few minor suggestions were made for potential improvements:
- Consider adding error handling in
toNodePosfor invalid paths.- The
nodesBetweenfunction could be refactored for improved readability.- Performance optimization in
isRightMostmight be beneficial if it's frequently called.These suggestions are minor and the current implementation is already of high quality. Great job on this new module!
test/view/selection.test.ts (1)
2-2: LGTM: Import statement updated correctly.The import statement has been properly updated to include the new
pathOffunction alongsideoffsetOf. This change aligns with the introduction of the newpathOftest suite.src/view/view.ts (3)
1-1: LGTM: Import statements updated correctly.The new imports align with the changes in the
handleBeforeInputmethod and the addition of thegetSelectionmethod. This ensures that all necessary functions and types are available for use in theViewclass.Also applies to: 4-5
53-54: LGTM: Simplified input handling logic.The
handleBeforeInputmethod has been refactored to directly callinsertTextandsplitBlockcommands, which simplifies the code and improves maintainability. The use ofthis.notify()ensures that these commands are properly propagated through the Observable pattern.Also applies to: 59-59
80-82: LGTM: NewgetSelectionmethod added.The new
getSelectionmethod enhances theViewclass by providing a way to retrieve the current selection from the container. This addition is well-aligned with the class's responsibilities and improves its overall functionality.test/model/nodepos.test.ts (7)
1-5: LGTM: Import statements are well-organized and relevant.The import statements are logically organized, with external imports (vitest) followed by local imports. All imported items appear to be relevant to the tests in this file.
7-16: LGTM: Path to Pos conversion test is well-structured.This test effectively verifies the
toNodePosfunction by converting a sample XML structure and asserting the correct node and offset. The use of a multi-level XML structure ensures the function works correctly with nested elements.
18-23: LGTM: Node to path conversion test is concise and effective.This test verifies the
pathOffunction by converting a text node back to its path representation. The test uses a simpler XML structure, which is appropriate for this specific test case.
26-37: LGTM: Comprehensive test for iterating over nodes between positions.This test effectively verifies the
nodesBetweenfunction by iterating over multiple paragraphs and asserting the correct XML output. The use oftokenToXMLfor comparison is a good approach.
39-51: LGTM: Test for iterating over a single text node is well-implemented.This test verifies the behavior of
nodesBetweenwhen operating on a single text node with specific start and end offsets. The assertion correctly checks for the expected substring.
53-62: LGTM: Edge case test for identical start and end positions.This test correctly verifies that
nodesBetweenreturns an empty result when the start and end positions are the same. This is an important edge case to cover.
1-63: Excellent test coverage and structure for node position functionality.This test file provides comprehensive coverage for the node position and iteration functionality. The tests are well-structured, covering various scenarios including conversions between paths and positions, iterating over multiple nodes, handling single text nodes, and edge cases. The use of sample XML structures and the
tokenToXMLfunction for comparisons is effective and enhances the readability of the tests.Some points of note:
- Good separation of concerns with two distinct test suites.
- Effective use of the Vitest testing framework.
- Clear and descriptive test names.
- Appropriate use of helper functions (e.g.,
plainTextSchema.fromXML,tokenToXML).Overall, this test file significantly contributes to the reliability of the node position and iteration functionality in the codebase.
src/model/nodes.ts (5)
1-7: LGTM: Import and TagType definition are clear and appropriate.The import statement includes necessary types, and the
TagTypedefinition is well-documented and correctly defined as a union type.
27-41: LGTM: pathOf function is well-implemented.The
pathOffunction efficiently builds the path from a node to its container. It includes appropriate error handling and uses an efficient method (unshift) to construct the path.
43-50: LGTM: lengthOf function is concise and correct.The
lengthOffunction correctly handles both text and element nodes, providing an appropriate length measure for each type.
77-93: LGTM: insertBefore function is well-implemented.The
insertBeforefunction correctly inserts new nodes before a given node, efficiently handling multiple node insertion. It includes appropriate error handling and updates the parent of the inserted nodes.
1-129: Overall: Well-implemented node manipulation utilities with minor improvement opportunities.The
src/model/nodes.tsfile provides a comprehensive set of utilities for working with a tree-like node structure. The implementation is generally solid, with appropriate error handling and efficient algorithms.Key strengths:
- Clear type definitions and well-documented functions.
- Efficient implementations for node traversal and manipulation.
- Appropriate error handling in most functions.
Suggestions for improvement:
- Consider handling element attributes in the
toXMLfunction.- Modify the
splitTextfunction to return both resulting nodes.- Refactor
insertBeforeandinsertAfterto reduce code duplication.- Update the removed node's parent reference in the
removeNodefunction.These minor improvements would enhance the robustness and maintainability of the code. Overall, this is a well-structured and implemented set of utilities.
src/view/selection.ts (3)
1-2: LGTM: Import statements and type definition are appropriate.The updated import statements and the new
Positiontype definition are well-structured and properly documented. These changes align with the new functionality being introduced in the file.Also applies to: 4-8
34-73: LGTM:pathOffunction is well-implemented, but consider optimizations.The
pathOffunction correctly calculates the path of a node within a container, handling the grouping of adjacent text nodes and traversing the DOM tree upwards.Consider the following optimizations:
- Cache the result of
Array.from(parent.childNodes)if it's used multiple times.- The offset calculation for text nodes (lines 60-65) could potentially be optimized. Consider using a more efficient algorithm if dealing with a large number of text nodes.
Example of caching
childNodes:const childNodesArray = Array.from(parent.childNodes); const groups = groupBy( childNodesArray, (p, c) => p.nodeType === Node.TEXT_NODE && c.nodeType === Node.TEXT_NODE, );
Line range hint
1-121: LGTM: Overall file structure and consistency are excellent.The file structure is logical and consistent, with imports at the top, followed by type definitions and well-documented functions. The new additions integrate seamlessly with the existing code, maintaining a high level of code quality and readability.
src/model/model.ts (5)
2-18: LGTM: Import statements and class declaration updated appropriately.The new imports and the change in the
Observablegeneric type align well with the transition to an operation-based model structure. This provides a solid foundation for the enhanced functionality of theModelclass.
34-40: LGTM:createTextandtoXMLmethods implemented correctly.These methods provide essential functionality for working with the new model structure. The
createTextmethod ensures that text nodes are created according to the schema, whiletoXMLallows for easy conversion of the model back to XML format.
112-154: LGTM:nodePosOfandgetContentEndRangemethods implemented correctly.Both methods are well-implemented and provide essential functionality for working with the model structure:
nodePosOfcorrectly handles the conversion of a path to a node position, with the option to split text nodes.getContentEndRangeefficiently traverses the tree to find the end of the content.The implementation is clear and should work well with the new model structure.
157-158: Implement the 'move' method as previously noted.As mentioned in a previous review comment, the
movemethod is not implemented. This will cause runtime errors when applying 'move' operations.Would you like assistance in implementing the
movemethod? I can provide a basic structure or pseudocode to help you get started.
42-56:⚠️ Potential issueImplement 'move' operation and refine error handling.
The
applymethod structure is good, allowing for easy addition of new operation types. However, there are two issues to address:
The 'move' operation is not implemented. This could lead to unexpected behavior when applying 'move' operations.
The error message in the default case exposes the entire
opobject, which could potentially leak sensitive information.To address these issues:
Implement the 'move' operation or add a TODO comment if it's planned for future implementation.
Modify the error message to only include the operation type:
- throw new Error(`invalid operation type: ${op}`); + throw new Error(`Invalid operation type: ${op.type}`);Note: The error message issue was previously flagged in an earlier review. Please address this in your next revision.
| export function toIndexRange( | ||
| range: AbstractRange, | ||
| container: Node, | ||
| ): IndexRange { |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: toRange function is still in use.
Multiple instances of toRange were found in the codebase:
src/view/view.tssrc/view/selection.ts
Please update all remaining usages of toRange to toIndexRange.
🔗 Analysis chain
LGTM: toIndexRange function renamed and updated correctly.
The renaming of toRange to toIndexRange and the change in return type to IndexRange accurately reflect the function's purpose.
Please ensure that all usages of the former toRange function returning an IndexRange are updated to use toIndexRange. Run the following script to check for any remaining usages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all occurrences of `toRange` in TypeScript files.
rg --type typescript --word-regexp '\btoRange\b'
Length of output: 85
Script:
#!/bin/bash
# Description: Find all occurrences of `toRange` in TypeScript files.
rg --glob "*.ts*" --word-regexp '\btoRange\b'
Length of output: 608
Apply schema to model
Summary by CodeRabbit
Release Notes
New Features
Editorclass to support schema management and improved text insertion logic.Bug Fixes
Devtoolsplugin to display the model's value in XML format instead of plain text.Tests
Model,Nodes, and utility functions to ensure reliability and correctness.pathOffunction and enhanced existing tests for theoffsetOffunction.