Remove sync conflict threshold#9972
Conversation
|
Cloudflare Pages Preview https://6f7a5974.notesnook-app.pages.dev Commit: c9e66bb |
|
Desktop Previews Commit: c9e66bb |
| } | ||
| return; | ||
| if (isHTMLEqual(localItem.data, remoteItem.data)) { | ||
| return remoteItem.dateEdited > localItem.dateEdited ? "merge" : undefined; |
There was a problem hiding this comment.
If you're going to remove the conflict threshold, wouldn't it be safer to implement a change id of some sort to detect conflicts instead of relying on timestamps? If there are even small amounts of local time drift between devices, there's a possibility that the conflict detection wont work correctly for changes made close together, if a user has the app open on 2 devices at the same time.
I worked on an implementation like this for Joplin last year, which falls back to a timestamp based check when the new column is not yet populated. The PR for the change was closed because a spec had not been discussed at the time and there was some related work in the pipeline which it should align with. But the implementation works and is conceptually sound. You can see the code for that here laurent22/joplin#13617
There was a problem hiding this comment.
there's a possibility that the conflict detection wont work correctly for changes made close together
That should be covered by the isHTMLEqual check.
2 participants
Description
This should fix all the cases where a conflicting edit from another device resulted in content getting unintentially and wrongly replaced. A side effect of this change is that users will experience a lot more conflicts but I think that is better than silently keeping one or the other copy. To trigger a conflict when there's an actual change, I have also improved the HTML equality check. Since
QA Scope
Needs to be tested on web & mobile. Specifically test the case where edits are being made on 2 devices simultaneously.
Type of Change
Visuals
Testing
If tests were not added, explain why
Platform
Sign-off