Mobile, Desktop, Cli: Resolves #16020: Automatically merge non-overlapping note changes on sync conflicts - #16023
Conversation
|
There's a test failure: [@joplin/lib]: Summary of all failing tests |
|
Please review and trim down the LLM generated comments |
|
Thanks for the review, I have removed unwanted comments and shortened the necessary ones |
11772ae to
5d9d1c5
Compare
Thanks for finding it, that was the problem with Base cleared on every sync run with file system as sync target, which is why automerge was not performed.
I fixed it slight differently. Instead of adding parameters and updating every caller as you suggested , I made the insert to carry the existing base values over from row it is replacing, so nothing lost here whichever sync step call it. I also carried base_conflict_note_id, which was losing the same way On 2nd one, I think it's not required because base is recorded only on upload step from the local note and not on download, so it's always plaintext and it never needs decrypting [you can see in below video about E2EE on a file system target] Screen.Recording.2026-08-12.at.6.mp4Testing info: Base was recorded in left profile before recording the video |
It's not technically required to record the base on download (and was not in the original proposal), but I realise now that it is advisable to record it on download as well, to get the least amount of conflicts. Take this example:
A conflict must be created because it could not be fully resolved automatically. If the base version was stored on download as well, then the conflict would have been able to be automatically resolved completely. It's worth noting as well, if E2EE is not enabled it should also update the base version on download as well: If encryption is not applied update it via the Synchronizer, if encryption is applied, that would be skipped, and it will update it via the BaseItem.decrypt flow instead |
| { | ||
| sql: 'INSERT INTO sync_items (sync_target, item_type, item_id, item_location, sync_time, remote_item_updated_time, sync_disabled, sync_disabled_reason) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', | ||
| params: [syncTarget, itemType, itemId, itemLocation, syncTime, remoteItemUpdatedTime, syncDisabled ? 1 : 0, `${syncDisabledReason}`], | ||
| sql: 'DELETE FROM sync_items WHERE sync_target = ? AND item_type = ? AND item_id = ? AND id != (SELECT MAX(id) FROM sync_items WHERE sync_target = ? AND item_type = ? AND item_id = ?)', |
There was a problem hiding this comment.
You can't do this. Ids in Joplin are UUIDs, so using MAX will not work. Also in the insert, you're doing many sub queries vs 1 additional select which I proposed. From a performance perspective it's probably better to follow my suggested approach instead (plus in include the base_conflict_note_id column as well which I forgot about)
There was a problem hiding this comment.
sync_items.id is an INTEGER PRIMARY KEY so max do work but I think it's fair considering performance and to avoid subqueries, so I followed your approach. I added base_conflict_note_id as well. Verified it manually.
Screen.Recording.2026-08-12.at.9.mp4
|
Fixed a bug in commit: Issue:Screen.Recording.2026-08-12.at.7.mp4FixScreen.Recording.2026-08-12.at.7.1.mp4 |
@varunkumar-22 What are your thoughts about this message? Additionally, without saving base version on download, when a user does the full initial sync for a new device, the base version won't be populated for any note initially. Also, what do you think about altering the description of the setting, to highlight the possibility of duplication and changes to formatting when enabled? |
Agreed, this makes sense and I initially discussed to record base for downloads as well, but it was ruled out considering that it's not safe to do it. It will avoid unnecessary conflicts when automerge is enabled. Yet, I feel it's safe to write base for downloads as
But I still would like to confirm it whether it is safe to do or not.
I discussed about this with Caleb, and we agree to go with a disclaimer as it can take a lot of time working on this issue with no guarantee of finding solution. I will update the setting in upcoming commit and will give an update after it is done. |
This code comment is what indicates you can never have a conflict in the delta step (implicitly stated that it is due to the upload step always running before delta), and therefore you can rely on any version saved locally in the delta step as being a common base. If the delta step would ever be moved before the upload, then the sync algorithm would become fundementally broken. joplin/packages/lib/Synchronizer.ts Lines 894 to 895 in 2654b33 My initial pushback was because I thought it was best to change the Synchronizer code as little as possible, but I see now that there is a safe way to do this, even with E2EE, and it does provide a notable benefit. |
|
With commit, now the base is recorded on downloads as well. When E2EE is enabled, the base is skipped to avoid recording encrypted content and it is recorded once the decryption worker ran and the content ends up as plain text
Implemented as you described, but with just one difference. Instead of adding a new function I reused existing Without E2EE:Screen.Recording.2026-08-14.at.3.mp4With E2EE:Screen.Recording.2026-08-14.at.2.mp4Testing info: file system is used as sync target |
In commit I added a code comment for white-space only edit loss and also duplication when adjacent identical lines are edited. This also covers disclaimer about this limitations. Desktop:
Mobile:
|
I think the user facing note below the setting is a bit too technical and long to be honest. How about this?: |
| // the sync steps that do so would otherwise lose it on every run | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- item is an entity slice with id/type_ — tests pass loose objects with `id` as number | ||
| public static updateSyncTimeQueries(syncTarget: number, item: any, syncTime: number, remoteItemUpdatedTime = 0, syncDisabled = false, syncDisabledReason = '', itemLocation: number = null) { | ||
| public static updateSyncTimeQueries(syncTarget: number, item: any, syncTime: number, base: SyncItemBaseVersion, remoteItemUpdatedTime = 0, syncDisabled = false, syncDisabledReason = '', itemLocation: number = null) { |
There was a problem hiding this comment.
My apologies, I think actually making an extra select before calling this query is less efficient than your previous solution. I didn't realise on a sqlite db that making extra round trips to the db is still much more expensive than running multiple queries in a single transaction. While a single select in it's own transaction is very cheap (maybe 1-10 ms depending on the device), considering that the delta step may iterate over 100s of thousands of items, the overall impact of adding this additional select may not be insignificant.
You were correct that the id of sync_items is indeed an integer, and it would work to match the latest row using MAX. So I think it would be better to restore your previous change in commit 359654b, but AI suggested a more optimised query for the INSERT query:
INSERT INTO sync_items (
sync_target,
item_type,
item_id,
item_location,
sync_time,
remote_item_updated_time,
sync_disabled,
sync_disabled_reason,
base_body,
base_title,
base_conflict_note_id
)
SELECT
?,
?,
?,
?,
?,
?,
?,
?,
COALESCE(existing.base_body, ''),
COALESCE(existing.base_title, ''),
COALESCE(existing.base_conflict_note_id, '')
FROM (SELECT 1) AS seed
LEFT JOIN sync_items AS existing
ON existing.id = (
SELECT MAX(candidate.id)
FROM sync_items AS candidate
WHERE candidate.sync_target = ?
AND candidate.item_type = ?
AND candidate.item_id = ?
);


Fixes #16020
PR Summary:
As described in issue #16020 :
When the sync loop detects a note conflict, this new code runs inside
handleConflictAction, just after the existing mustHandleConflict check and before the conflict note is created. So, the automatic conflict resolution is performed after the conflict detection and just before the note creation. Read-only notes, encrypted notes , and locked notes fall back to the existing conflict note behavior.The three versions
A three-way merge needs base, local, and remote, and all three already exist:
If there's no base (a note only ever downloaded ), it falls back to the plain conflict note
How the merge works (diffNotes.ts):
autoMerge(base, local, remote):
Turning that into two notes (autoMergeNote.ts)
From those sections it builds two versions that are same everywhere except where a real conflict is happened:
What sync does with this:
conflict_note_states, along with the remote note's updated_time so the resolution UI can tell whether the original note has changed since the conflict was created. Theremote_titleandremote_bodyare kept for future partial resolution purpose.E2EE support
Initially, automerge not used to run when E2EE is enabled as we can read encrypted remote note content until it is decrypted and normal conflict behaviour is continued. So, now this case is handled by decrypting the notes which are found to be conflicts, E2E encrypted and local is not encrypted. the encrypted remote note is decrypted in memory and it is used for running automerge. If everything is merged cleanly, then the merged note is saved directly over the local note and If incase a real conflict exists, decrypted remote (along with some auto-merged changes) is used to update the original note , which helps upcoming conflict resolution UI to read the actual remote content from it instead of encrypted or any empty data
The local note is not decrypted; it is expected to already be decrypted during a normal sync, and if incase it is still encrypted, or if the remote note cannot be decrypted (for ex, the master key has not been unlocked), then the behaviour safely falls back to the existing conflict note behavior
added additional tests in
Synchronizer.autoMerge.test.tsfor cases : merging an encrypted remote, keeping the decrypted remote on the original note when partially merged, and the fallback when decryption is not availableSettings
A new setting controls this:
sync.autoMergeConflicts- a normal public setting under the Synchronisation, named as "Automatically merge non-conflicting note changes". It is enabled by default, so automatic merging is available immediately, while users can still disable it at any time.The setting is read through a static helper (isAutoMergeEnabled()) instead of checking the setting name in different places
Testing:
Tests are added:
diffNotes.test.ts- the merge engine: clean merges, real conflicts, line level merges, trailing whitespace, no-base fallbackautoMergeNote.test.ts- title rules and the two resolved versionsSynchronizer.autoMerge.test.ts- two real synced devices: clean merge makes no conflict note and reaches the other device, partial merge keeps shared changes into both, locked notes skippedTested the automatic merge implementation across various Markdown edge cases, including images, links, tables, code blocks, lists, headings, blockquotes, checklists, nested formatting, emojis, whitespace-only changes, insertions, deletions, and mixed content to verify that merges behave correctly without breaking the document structure and all are working correctly.
limitation:
remote_bodyandremote_titlecolumns inconflict_note_statesare left in the schema but no longer written at sync time [ comment ]. As at that point they only duplicate what original note alreadyhas, and so they will be invalid as soon as any changes are made to note. They are kept for storing a partial resolution later.AI Assistance Disclosure:
AI was used in researching & learning about the library implementation, in writing some tests and also for data while testing various edgecases