Update removedAt for already deleted nodes during range deletions #909
Conversation
- Add logic to update `removedAt` for nodes with outdated deletion times - Ensure that garbage collection accounts for the latest `removedAt` updates - Maintain existing behavior for local operations while enhancing remote operation consistency
|
|
WalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Is there a way to check the removedAt field in the e2e tests when a remote operation try in? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/test/helper/helper.ts (1)
289-294: LGTM: New helper function added.The
getLTTfunction provides a convenient way to access the last time ticket from thedummyContext, which can be useful in testing scenarios. It's consistent with other helper functions in the file.Consider whether this function is necessary, as
dummyContext.getLastTimeTicket()could be called directly where needed. If it's used frequently in tests, it might be worth keeping for consistency and ease of use.packages/sdk/test/unit/document/crdt/tree_test.ts (1)
235-235: Consider updating the test name for consistencyFor consistency with other test cases in the
CRDTTree.Editsuite, consider starting the test name with "Can", for example:-it('Update removedAt for already deleted nodes during range deletions', function () { +it('Can update removedAt for already deleted nodes during range deletions', function () {This aligns with the naming convention used in other test cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/test/helper/helper.ts (2 hunks)
- packages/sdk/test/unit/document/crdt/tree_test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/sdk/test/helper/helper.ts (2)
190-190: LGTM: Minor formatting improvement.The addition of a space after the arrow function declaration enhances code readability by following a consistent style.
Line range hint
1-294: Overall assessment: Minor improvements to testing utilities.The changes in this file are minor and focused on improving the testing utilities. They don't introduce any new risks or complexities. The new
getLTTfunction and the formatting improvement inassertThrowsAsyncare both acceptable changes that maintain the file's purpose and structure.packages/sdk/test/unit/document/crdt/tree_test.ts (1)
235-263: Test case accurately verifies 'removedAt' updatesThe test is well-structured and effectively verifies that
removedAttimestamps are updated correctly for already deleted nodes during range deletions. The assertions confirm that both the parent node (p) and the text node (txt) have theirremovedAtfields properly set.
|
I've added a test case for quick progress. Please review the test code. |
It seems that the txt node has already been deleted from the t tree in the previous edit step. Is it intentional that in assert.isTrue(txt.getRemovedAt()?.equals(getLTT())), the delimiter values are expected to be the same? Given that the txt node is already deleted, should its removedAt timestamp still match the getLTT() value in this context? |
What this PR does / why we need it:
This PR addresses the issue where
removedAttimestamps were not being updated for nodes that had already been deleted during range deletion operations. The changes ensure that theremovedAtfield reflects the most recent deletion time, which is crucial for maintaining operational consistency across remote clients, particularly in scenarios that may involve future undo/redo functionalities.Any background context you want to provide?
Previously, nodes that were already marked as removed did not have their removedAt timestamp updated during subsequent deletion operations.
This led to inconsistencies when undoing or redoing changes, especially in cases where multiple clients performed overlapping deletion operations.
This PR ensures that the removedAt field is always reflective of the most recent deletion, thereby maintaining the integrity of the document's operational history and facilitating accurate undo/redo operations.
What are the relevant tickets?
Fixes #821
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes