Handle local changes correctly when receiving snapshot #868
Conversation
WalkthroughThe recent changes improve how the Changes
Poem
Tip You can customize the tone of the comments in your PRsSpecify the tone of the comments in your PRs by configuring the 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 (
|
hackerwins
force-pushed
the
snapshot-with-localchanges
branch
from
a854344 to
6466e1e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
=======================================
Coverage 80.80% 80.81%
=======================================
Files 60 60
Lines 4637 4639 +2
Branches 942 943 +1
=======================================
+ Hits 3747 3749 +2
Misses 621 621
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/document/document.ts (1 hunks)
- test/integration/client_test.ts (1 hunks)
- test/integration/snapshot_test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- test/integration/snapshot_test.ts
Additional context used
Biome
test/integration/client_test.ts
[error] 847-847: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (2)
test/integration/client_test.ts (1)
842-865: LGTM! Ensure comprehensive test coverage.The new test case for handling local changes when receiving a snapshot is comprehensive and well-structured.
Ensure that this test is run in various scenarios to validate its robustness.
Tools
Biome
[error] 847-847: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/document/document.ts (1)
1075-1081: Ensure local changes are applied as remote changes when a snapshot is present.The new code correctly ensures that local changes are applied as remote changes when a snapshot is present. This logic is crucial for maintaining the integrity of the document's state.
| await c1.sync(); | ||
|
|
||
| // 02. c2 receives the snapshot and increases the counter simultaneously. | ||
| c2.sync(); |
There was a problem hiding this comment.
@hackerwins
Would you explain why this line doesn't use await?
What this PR does / why we need it?
Handle local changes correctly when receiving snapshot
This commit addresses the issue where unsent local changes were
missing from
Document.Rootwhen receiving a snapshot from the server.To resolve this problem, the local changes are now applied to
Document.Rootafter applying the snapshot.Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
Bug Fixes
Tests