Add RHTNode removal to converter for consistency #842
Conversation
|
Warning Rate limit exceeded@hackerwins has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 33 minutes and 22 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce significant enhancements to the conversion functionalities between internal models and Protobuf formats. Key updates include the addition of 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
==========================================
+ Coverage 80.66% 80.68% +0.01%
==========================================
Files 59 59
Lines 4577 4586 +9
Branches 931 932 +1
==========================================
+ Hits 3692 3700 +8
- Misses 616 617 +1
Partials 269 269 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
src/api/yorkie/v1/resources_pb.js (1)
Line range hint
20-21: Remove redundant 'use strict' directive.- "use strict";src/api/converter.ts (3)
Line range hint
142-142: Replace unnecessary template literals with string literals for better performance and clarity.- throw new YorkieError(Code.Unsupported, `unsupported type: ${valueType}`); + throw new YorkieError(Code.Unsupported, 'unsupported type: ' + valueType);Also applies to: 267-267, 763-763
Line range hint
376-376: Consider replacing non-null assertions with explicit null checks to prevent potential runtime errors.- pbOperation.body.value = pbSetOperation; + if (pbSetOperation) { + pbOperation.body.value = pbSetOperation; + } else { + throw new Error('Expected pbSetOperation to be non-null'); + }Also applies to: 396-396, 422-422, 430-430, 449-449, 497-497, 832-832, 911-911, 916-916, 922-922, 925-925, 939-939, 949-949, 960-960
Line range hint
817-819: Consider replacingforEachwithfor...offor better performance and clarity in loops.- pbTreeNodes.forEach((node) => { + for (const node of pbTreeNodes) {Also applies to: 854-856
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/api/converter.ts (4 hunks)
- src/api/yorkie/v1/resources.proto (1 hunks)
- src/api/yorkie/v1/resources_pb.d.ts (1 hunks)
- src/api/yorkie/v1/resources_pb.js (1 hunks)
- src/document/crdt/rht.ts (3 hunks)
- test/unit/api/converter_test.ts (2 hunks)
- test/unit/document/crdt/rht_test.ts (1 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.
Unsafe fix: Use a literal key instead.
[error] 28-28: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
[error] 29-29: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
[error] 24-84: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 84-91: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 91-133: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 23-133: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.src/document/crdt/rht.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
[error] 231-231: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
[error] 300-300: Use === instead of ==.
== is only allowed when comparing againstnull== is only allowed when comparing against null
Using == may be unsafe if you are relying on type coercion
Unsafe fix: Use ===
[error] 300-300: Use !== instead of !=.
!= is only allowed when comparing againstnull!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==test/unit/document/crdt/rht_test.ts
[error] 24-41: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 43-59: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 61-71: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 73-86: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 88-105: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 107-123: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 125-133: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 23-134: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 137-137: This enum declaration contains members that are implicitly initialized.
This enum member should be explicitly initialized.
This enum member should be explicitly initialized.
This enum member should be explicitly initialized.
Allowing implicit initializations for enum members can cause bugs if enum declarations are modified over time.
Safe fix: Initialize all enum members.
[error] 192-192: Useless rename.
Safe fix: Remove the renaming.
src/api/yorkie/v1/resources_pb.js
[error] 20-21: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.src/api/converter.ts
[error] 142-142: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 267-267: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[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] 697-697: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
[error] 763-763: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 817-819: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 832-832: Forbidden non-null assertion.
[error] 854-856: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
[error] 911-911: Forbidden non-null assertion.
[error] 916-916: Forbidden non-null assertion.
[error] 922-922: Forbidden non-null assertion.
[error] 925-925: Forbidden non-null assertion.
[error] 939-939: Forbidden non-null assertion.
[error] 949-949: Forbidden non-null assertion.
[error] 960-960: Forbidden non-null assertion.
Additional comments not posted (9)
test/unit/api/converter_test.ts (1)
107-112: Changes to the tree styling and removal are correctly implemented and tested.src/document/crdt/rht.ts (1)
124-126: The addition ofgetNodeMapByKeyprovides a clear way to access the internal node map.src/api/yorkie/v1/resources.proto (1)
229-229: The addition of theis_removedfield toNodeAttris crucial for accurately tracking the removal state of node attributes.test/unit/document/crdt/rht_test.ts (1)
125-133: The new test case for deep copying an RHT instance is well-implemented and ensures the correctness of thedeepcopymethod.Tools
Biome
[error] 125-133: This function expression can be turned into an arrow function.
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.src/api/yorkie/v1/resources_pb.js (1)
439-439: Field addition looks correct.Please verify that existing serialized data handles the new
is_removedfield gracefully during deserialization.src/api/converter.ts (3)
595-609: The implementation oftoRHTcorrectly handles the newisRemovedattribute. Well done on maintaining clarity and consistency in the conversion logic.
1068-1082: The implementation offromRHTcorrectly handles the newisRemovedattribute, ensuring that the attribute is properly converted from the Protobuf format to the model.
Line range hint
697-697: Specify a more appropriate type instead of usinganyto enhance type safety and code maintainability.
[ISSUE]
- function toText(text: CRDTText<Record<string, any>>): PbJSONElement { + function toText(text: CRDTText<Record<string, string>>): PbJSONElement {src/api/yorkie/v1/resources_pb.d.ts (1)
1231-1235: Addition ofis_removedfield toNodeAttrclass aligns with the updated Protobuf definition.
hackerwins
changed the title
What this PR does / why we need it?
Add RHTNode removal to converter for consistency
This commit addresses the missing
isRemovedencoding in the RHT.Similar to other CRDTs like ElementRHT, including tombstone nodes like
isRemovedduring encoding is crucial. However, the RHT did notinclude tombstone nodes in its encoding, leading to inconsistencies in
snapshots.
Any background context you want to provide?
What are the relevant tickets?
Checklist
Summary by CodeRabbit
New Features
is_removedto enhance data structure functionality.Tests
deepcopymethod functionality.