Fix incorrect tree snapshot encoding/decoding #836
Conversation
WalkthroughThe recent update enhances the tree traversal and size management functionalities across various modules. Notably, the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Converter
participant CRDTTree
participant Document
User->>Converter: Call traverseAll()
Converter->>CRDTTree: traverseAll()
CRDTTree-->>Converter: Return traversal result
User->>Document: Create new Tree
Document->>CRDTTree: Get Node Size
CRDTTree-->>Document: Return size
Document-->>User: Return Tree with size
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #836 +/- ##
==========================================
+ Coverage 80.45% 80.65% +0.20%
==========================================
Files 59 59
Lines 4543 4554 +11
Branches 921 922 +1
==========================================
+ Hits 3655 3673 +18
+ Misses 619 614 -5
+ Partials 269 267 -2 ☔ View full report in Codecov by Sentry. |
hackerwins
force-pushed
the
snapshot-test
branch
2 times, most recently
from
1e9f047
to
e4a157a
Compare
hackerwins
changed the title
hackerwins
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
src/api/converter.ts (4)
Line range hint
142-142: Avoid using template literals where not necessary.- `unsupported type: ${valueType}` + 'unsupported type: ' + valueTypeAlso applies to: 267-267, 752-752
Line range hint
376-376: Remove non-null assertions to ensure code robustness. Consider adding null checks or handling potential null values gracefully.- pbTextNodePos.createdAt! + pbTextNodePos.createdAt ? pbTextNodePos.createdAt : defaultTimeTicketAlso applies to: 396-396, 422-422, 430-430, 449-449, 497-497, 821-821, 900-900, 905-905, 911-911, 914-914, 928-928, 938-938, 949-949
Line range hint
686-686: Specify a more explicit type instead of usingany.- function toText(text: CRDTText<Record<string, any>>): PbJSONElement { + function toText(text: CRDTText<Record<string, string>>): PbJSONElement {
Line range hint
806-808: Prefer usingfor...ofloops overforEachfor better performance and readability.- Object.entries(pbPresences).forEach(([actorID, pbPresence]) => { + for (const [actorID, pbPresence] of Object.entries(pbPresences)) {Also applies to: 843-845
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/api/converter.ts (3 hunks)
- src/document/crdt/tree.ts (2 hunks)
- src/document/json/tree.ts (1 hunks)
- src/util/index_tree.ts (4 hunks)
- test/unit/api/converter_test.ts (2 hunks)
Additional context used
Biome
test/unit/api/converter_test.ts
[error] 27-27: The computed expression can be simplified without the use of a string literal.
[error] 28-28: The computed expression can be simplified without the use of a string literal.
[error] 29-29: The computed expression can be simplified without the use of a string literal.
[error] 24-84: This function expression can be turned into an arrow function.
[error] 100-101: Do not use template literals if interpolation and special-character handling are not needed.
[error] 108-108: Unexpected any. Specify a different type.
[error] 113-113: Unexpected any. Specify a different type.
[error] 84-117: This function expression can be turned into an arrow function.
[error] 117-124: This function expression can be turned into an arrow function.
[error] 23-124: This function expression can be turned into an arrow function.
src/document/json/tree.ts
[error] 86-86: This variable implicitly has the any type.
[error] 118-118: This variable implicitly has the any type.
[error] 125-125: This variable implicitly has the any type.
[error] 217-217: Forbidden non-null assertion.
[error] 281-281: Unexpected any. Specify a different type.
[error] 293-293: Forbidden non-null assertion.
[error] 317-317: Unexpected any. Specify a different type.
[error] 332-332: Forbidden non-null assertion.
[error] 339-339: Forbidden non-null assertion.
[error] 374-374: Forbidden non-null assertion.
[error] 381-381: Forbidden non-null assertion.
[error] 411-411: Forbidden non-null assertion.
[error] 422-422: Forbidden non-null assertion.
[error] 429-429: Forbidden non-null assertion.
[error] 433-433: Forbidden non-null assertion.
[error] 440-440: Forbidden non-null assertion.
[error] 444-444: Forbidden non-null assertion.
[error] 447-447: Forbidden non-null assertion.
[error] 449-449: Forbidden non-null assertion.
[error] 16-17: All these imports are only used as types.
src/util/index_tree.ts
[error] 190-190: Unexpected any. Specify a different type.
[error] 197-197: Forbidden non-null assertion.
[error] 197-197: Unexpected any. Specify a different type.
[error] 198-198: Forbidden non-null assertion.
[error] 252-252: Forbidden non-null assertion.
[error] 252-252: Unexpected any. Specify a different type.
[error] 294-294: Unexpected any. Specify a different type.
[error] 310-310: Unexpected any. Specify a different type.
[error] 393-393: Forbidden non-null assertion.
[error] 393-393: Unexpected any. Specify a different type.
[error] 442-442: Unexpected any. Specify a different type.
[error] 818-818: Forbidden non-null assertion.
[error] 824-824: Forbidden non-null assertion.
[error] 828-828: Forbidden non-null assertion.
[error] 833-833: Forbidden non-null assertion.
[error] 948-948: Forbidden non-null assertion.
[error] 956-956: Forbidden non-null assertion.
[error] 16-17: All these imports are only used as types.
[error] 516-516: Reassigning a function parameter is confusing.
[error] 759-759: Reassigning a function parameter is confusing.
src/document/crdt/tree.ts
[error] 151-151: This variable implicitly has the any type.
[error] 154-154: Forbidden non-null assertion.
[error] 155-155: Forbidden non-null assertion.
[error] 160-160: Forbidden non-null assertion.
[error] 242-242: Forbidden non-null assertion.
[error] 326-328: This else clause can be omitted because previous branches break early.
[error] 452-452: The assignment should not be in an expression.
[error] 547-547: Forbidden non-null assertion.
[error] 550-550: Forbidden non-null assertion.
[error] 591-591: Forbidden non-null assertion.
[error] 596-596: Forbidden non-null assertion.
[error] 722-722: Change to an optional chain.
[error] 724-735: Template literals are preferred over string concatenation.
[error] 877-877: Unexpected any. Specify a different type.
[error] 890-890: Forbidden non-null assertion.
[error] 891-891: Forbidden non-null assertion.
[error] 891-891: Forbidden non-null assertion.
[error] 896-896: Forbidden non-null assertion.
[error] 974-974: Forbidden non-null assertion.
[error] 1039-1039: Forbidden non-null assertion.
src/api/converter.ts
[error] 142-142: Do not use template literals if interpolation and special-character handling are not needed.
[error] 267-267: Do not use template literals if interpolation and special-character handling are not needed.
[error] 376-376: Forbidden non-null assertion.
[error] 396-396: Forbidden non-null assertion.
[error] 422-422: Forbidden non-null assertion.
[error] 430-430: Forbidden non-null assertion.
[error] 449-449: Forbidden non-null assertion.
[error] 497-497: Forbidden non-null assertion.
[error] 686-686: Unexpected any. Specify a different type.
[error] 752-752: Do not use template literals if interpolation and special-character handling are not needed.
[error] 806-808: Prefer for...of instead of forEach.
[error] 821-821: Forbidden non-null assertion.
[error] 843-845: Prefer for...of instead of forEach.
[error] 900-900: Forbidden non-null assertion.
[error] 905-905: Forbidden non-null assertion.
[error] 911-911: Forbidden non-null assertion.
[error] 914-914: Forbidden non-null assertion.
[error] 928-928: Forbidden non-null assertion.
[error] 938-938: Forbidden non-null assertion.
[error] 949-949: Forbidden non-null assertion.
Additional comments not posted (10)
test/unit/api/converter_test.ts (1)
20-20: Updated import statement to includeTreeclass aligns with the new functionality for handling tree structures.src/document/json/tree.ts (1)
256-265: The new methodgetNodeSizecorrectly checks initialization before proceeding, which is good practice for error handling. Ensure that the tree is properly initialized in all use cases to avoid runtime errors.src/util/index_tree.ts (3)
137-138: The methodupdateAncestorsSizehas been updated to correctly reflect changes in node sizes up the tree, which is crucial for maintaining the integrity of tree size calculations.
150-168: The methodupdateDescendantsSizeeffectively recalculates the size of all descendant nodes, ensuring accurate size tracking within the tree.
300-301: The new methodprependis specifically used for creating nodes from snapshots, which aligns with the intended enhancements for handling tree structures more effectively.src/document/crdt/tree.ts (2)
1280-1282: The methodgetNodeSizecorrectly returns the size of thenodeMapByIDLLRBTree.
777-777: Ensure proper handling of node registration intraverseAll.Verification successful
The
nodeMapByID.putoperations are present in multiple contexts within thesrc/document/crdt/tree.tsfile. The code appears to handle node registration in various methods, includingtraverseAll,registerNode, and within other operations.
traverseAllmethod: Registers nodes during traversal.registerNodemethod: Explicitly registers a given node.- Another context within a loop: Registers nodes conditionally.
These instances suggest that node registration is handled in different parts of the code, potentially ensuring that nodes are registered correctly without duplicates.
Lines around
traverseAll:this.indexTree.traverseAll((node) => { this.nodeMapByID.put(node.id, node); });Lines around
registerNode:public registerNode(node: CRDTTreeNode): void { this.nodeMapByID.put(node.id, node); }Another context:
this.nodeMapByID.put(node.id, node);Given the evidence, it appears that the node registration is handled properly in the code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all nodes are correctly registered without duplicates or omissions. # Test: Search for duplicate node registrations. Expect: No duplicates. rg --type python $'this.nodeMapByID.put(node.id, node)' src/document/crdt/tree.tsLength of output: 113
Script:
#!/bin/bash # Description: Verify that all nodes are correctly registered without duplicates or omissions. # Test: Search for duplicate node registrations. Expect: No duplicates. rg --type typescript $'this.nodeMapByID.put(node.id, node)' src/document/crdt/tree.tsLength of output: 121
Script:
#!/bin/bash # Description: Verify that all nodes are correctly registered without duplicates or omissions. # Test: Search for duplicate node registrations. Expect: No duplicates. rg 'this.nodeMapByID.put(node.id, node)' src/document/crdt/tree.tsLength of output: 68
Script:
#!/bin/bash # Description: Verify the presence and usage of the `nodeMapByID` variable in the file. # Test: Search for the `nodeMapByID` variable declaration and its related operations. rg 'nodeMapByID' src/document/crdt/tree.tsLength of output: 433
Script:
#!/bin/bash # Description: Verify the context around `put` operations to ensure proper handling of node registration. # Test: Extract lines around `put` operations for detailed inspection. rg -C 5 'nodeMapByID.put' src/document/crdt/tree.tsLength of output: 931
src/api/converter.ts (3)
108-108: Renamed functiontraversetotraverseAllto better reflect its functionality.
604-604: Refactored tree traversal logic to usetraverseAllfor improved clarity and maintainability.
1050-1050: Added call toupdateDescendantsSizeto ensure tree size is correctly updated, addressing the PR's objective to fix size updates during encoding/decoding.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (12)
test/unit/api/converter_test.ts (1)
Line range hint
91-124: Refactor test function to use arrow function for consistency.- it('should encode and decode tree properly', function () { + it('should encode and decode tree properly', () => {Tools
Biome
[error] 107-108: Do not use template literals if interpolation and special-character handling are not needed.
src/document/crdt/tree.ts (7)
Line range hint
151-151: Please specify a type for this variable to avoid implicit 'any' type.- let variable; + let variable: SpecificType; // Replace 'SpecificType' with the actual type
Line range hint
154-154: Avoid using non-null assertions. Consider adding proper checks or handling potential null values gracefully.- value!; + value ? value : defaultValue; // Replace 'defaultValue' with an appropriate fallbackAlso applies to: 155-155, 160-160, 242-242, 547-547, 550-550, 591-591, 596-596, 890-890, 891-891, 896-896, 974-974, 1039-1039
Line range hint
326-328: Thiselseclause is redundant as previous branches break early. Simplifying this can enhance readability.- else { - // code - }
Line range hint
452-452: Avoid assignments within expressions for clearer and more predictable code behavior.- if ((variable = value)) { + variable = value; + if (variable) {
Line range hint
722-722: Consider using optional chaining to safely access properties.- object.property.subProperty + object?.property?.subProperty
Line range hint
724-735: Prefer template literals over string concatenation for better readability and performance.- "string1" + variable + "string2" + `string1${variable}string2`
Line range hint
877-877: Specify a type instead of using 'any' to ensure type safety.- let variable: any; + let variable: SpecificType; // Replace 'SpecificType' with the actual typesrc/api/converter.ts (4)
Line range hint
142-142: Avoid using template literals when not necessary.- throw new YorkieError(Code.Unsupported, `unsupported type: ${valueType}`); + throw new YorkieError(Code.Unsupported, 'unsupported type: ' + valueType);Also applies to: 267-267, 752-752
Line range hint
376-376: Avoid using non-null assertions. Consider adding null checks or using optional chaining.- pbOperation.body.value!; + pbOperation.body.value ?? throw new Error('Operation value is undefined');Also applies to: 396-396, 422-422, 430-430, 449-449, 497-497, 821-821, 900-900, 905-905, 911-911, 914-914, 928-928, 938-938, 949-949
Line range hint
686-686: Specify a type instead of usingany.- function toText(text: CRDTText<Record<string, any>>): PbJSONElement { + function toText(text: CRDTText<Record<string, string>>): PbJSONElement {
Line range hint
806-808: Prefer usingfor...ofinstead offorEachfor better performance and readability.- Object.entries(pbPresences).forEach(([actorID, pbPresence]) => { + for (const [actorID, pbPresence] of Object.entries(pbPresences)) {Also applies to: 843-845
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/api/converter.ts (3 hunks)
- src/document/crdt/tree.ts (2 hunks)
- src/document/json/tree.ts (1 hunks)
- src/util/index_tree.ts (4 hunks)
- test/unit/api/converter_test.ts (2 hunks)
Additional context used
Biome
test/unit/api/converter_test.ts
[error] 27-27: The computed expression can be simplified without the use of a string literal.
[error] 28-28: The computed expression can be simplified without the use of a string literal.
[error] 29-29: The computed expression can be simplified without the use of a string literal.
[error] 24-84: This function expression can be turned into an arrow function.
[error] 84-91: This function expression can be turned into an arrow function.
[error] 107-108: Do not use template literals if interpolation and special-character handling are not needed.
[error] 91-124: This function expression can be turned into an arrow function.
[error] 23-124: This function expression can be turned into an arrow function.
src/document/json/tree.ts
[error] 86-86: This variable implicitly has the any type.
[error] 118-118: This variable implicitly has the any type.
[error] 125-125: This variable implicitly has the any type.
[error] 217-217: Forbidden non-null assertion.
[error] 281-281: Unexpected any. Specify a different type.
[error] 293-293: Forbidden non-null assertion.
[error] 317-317: Unexpected any. Specify a different type.
[error] 332-332: Forbidden non-null assertion.
[error] 339-339: Forbidden non-null assertion.
[error] 374-374: Forbidden non-null assertion.
[error] 381-381: Forbidden non-null assertion.
[error] 411-411: Forbidden non-null assertion.
[error] 422-422: Forbidden non-null assertion.
[error] 429-429: Forbidden non-null assertion.
[error] 433-433: Forbidden non-null assertion.
[error] 440-440: Forbidden non-null assertion.
[error] 444-444: Forbidden non-null assertion.
[error] 447-447: Forbidden non-null assertion.
[error] 449-449: Forbidden non-null assertion.
[error] 16-17: All these imports are only used as types.
src/util/index_tree.ts
[error] 190-190: Unexpected any. Specify a different type.
[error] 197-197: Forbidden non-null assertion.
[error] 197-197: Unexpected any. Specify a different type.
[error] 198-198: Forbidden non-null assertion.
[error] 252-252: Forbidden non-null assertion.
[error] 252-252: Unexpected any. Specify a different type.
[error] 294-294: Unexpected any. Specify a different type.
[error] 310-310: Unexpected any. Specify a different type.
[error] 393-393: Forbidden non-null assertion.
[error] 393-393: Unexpected any. Specify a different type.
[error] 442-442: Unexpected any. Specify a different type.
[error] 818-818: Forbidden non-null assertion.
[error] 824-824: Forbidden non-null assertion.
[error] 828-828: Forbidden non-null assertion.
[error] 833-833: Forbidden non-null assertion.
[error] 948-948: Forbidden non-null assertion.
[error] 956-956: Forbidden non-null assertion.
[error] 16-17: All these imports are only used as types.
[error] 516-516: Reassigning a function parameter is confusing.
[error] 759-759: Reassigning a function parameter is confusing.
src/document/crdt/tree.ts
[error] 151-151: This variable implicitly has the any type.
[error] 154-154: Forbidden non-null assertion.
[error] 155-155: Forbidden non-null assertion.
[error] 160-160: Forbidden non-null assertion.
[error] 242-242: Forbidden non-null assertion.
[error] 326-328: This else clause can be omitted because previous branches break early.
[error] 452-452: The assignment should not be in an expression.
[error] 547-547: Forbidden non-null assertion.
[error] 550-550: Forbidden non-null assertion.
[error] 591-591: Forbidden non-null assertion.
[error] 596-596: Forbidden non-null assertion.
[error] 722-722: Change to an optional chain.
[error] 724-735: Template literals are preferred over string concatenation.
[error] 877-877: Unexpected any. Specify a different type.
[error] 890-890: Forbidden non-null assertion.
[error] 891-891: Forbidden non-null assertion.
[error] 891-891: Forbidden non-null assertion.
[error] 896-896: Forbidden non-null assertion.
[error] 974-974: Forbidden non-null assertion.
[error] 1039-1039: Forbidden non-null assertion.
src/api/converter.ts
[error] 142-142: Do not use template literals if interpolation and special-character handling are not needed.
[error] 267-267: Do not use template literals if interpolation and special-character handling are not needed.
[error] 376-376: Forbidden non-null assertion.
[error] 396-396: Forbidden non-null assertion.
[error] 422-422: Forbidden non-null assertion.
[error] 430-430: Forbidden non-null assertion.
[error] 449-449: Forbidden non-null assertion.
[error] 497-497: Forbidden non-null assertion.
[error] 686-686: Unexpected any. Specify a different type.
[error] 752-752: Do not use template literals if interpolation and special-character handling are not needed.
[error] 806-808: Prefer for...of instead of forEach.
[error] 821-821: Forbidden non-null assertion.
[error] 843-845: Prefer for...of instead of forEach.
[error] 900-900: Forbidden non-null assertion.
[error] 905-905: Forbidden non-null assertion.
[error] 911-911: Forbidden non-null assertion.
[error] 914-914: Forbidden non-null assertion.
[error] 928-928: Forbidden non-null assertion.
[error] 938-938: Forbidden non-null assertion.
[error] 949-949: Forbidden non-null assertion.
Additional comments not posted (8)
test/unit/api/converter_test.ts (1)
108-108: Avoid using template literals when not necessary.- assert.equal(doc.getRoot().tree.toXML(), /*html*/ `<r><p>14</p></r>`); + assert.equal(doc.getRoot().tree.toXML(), '<r><p>14</p></r>');Likely invalid or redundant comment.
Tools
Biome
[error] 107-108: Do not use template literals if interpolation and special-character handling are not needed.
src/document/json/tree.ts (1)
259-265: Ensure proper error handling for uninitialized context or tree ingetNodeSize.src/util/index_tree.ts (1)
151-168: Ensure thatupdateDescendantsSizecorrectly updates the size of descendants.src/document/crdt/tree.ts (2)
777-777: The methodtraverseAllis used to populatenodeMapByID. Ensure that all nodes are correctly handled during traversal.
1280-1282: ThegetNodeSizemethod provides the size of thenodeMapByIDLLRBTree, which is crucial for understanding the scale of the tree structure at any given point.src/api/converter.ts (3)
108-108: Renamed functiontraversetotraverseAllto better reflect its functionality.
604-604: Correctly updated the function call totraverseAllto align with the renaming.
1050-1050: Added call toupdateDescendantsSizeto ensure the size of descendants is correctly updated, aligning with PR objectives.
What this PR does / why we need it?
Fix incorrect tree snapshot encoding/decoding
The following issues exist when encoding and ecoding snapshots of Tree:
This commit fixes the incorrect tree snapshot encoding/decoding logic.
Any background context you want to provide?
What are the relevant tickets?
Address yorkie-team/yorkie#880
Checklist
Summary by CodeRabbit
New Features
getNodeSizemethod to theTreeclass for retrieving the size of the tree node.Enhancements
Tests
Documentobject.