From 4d8bcf7e781c267085c380523ecbb97ab9b284b0 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Tue, 24 Dec 2024 10:57:20 +0100 Subject: [PATCH 1/3] fix(Default#getDiffs): Don't produce UPDATEs for tabs, ever Signed-off-by: Marcel Klehr --- src/lib/strategies/Default.ts | 38 +++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/lib/strategies/Default.ts b/src/lib/strategies/Default.ts index 3ef4bfd9f0..0c429da39a 100644 --- a/src/lib/strategies/Default.ts +++ b/src/lib/strategies/Default.ts @@ -27,7 +27,6 @@ import { CancelledSyncError, FailsafeError } from '../../errors/Error' import NextcloudBookmarksAdapter from '../adapters/NextcloudBookmarks' import CachingAdapter from '../adapters/Caching' -import LocalTabs from '../LocalTabs' const ACTION_CONCURRENCY = 12 @@ -437,8 +436,23 @@ export default class SyncProcess { this.cacheTreeRoot, this.localTreeRoot, // We also allow canMergeWith for folders here, because Window IDs are not stable - (oldItem, newItem) => - (oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)) || (oldItem.type === 'folder' && oldItem.canMergeWith(newItem)), + (oldItem, newItem) => { + if (oldItem.type !== newItem.type) { + return false + } + if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') { + return oldItem.url === newItem.url + } + if (oldItem.type === 'folder') { + if (String(oldItem.id) === String(newItem.id)) { + return true + } + if (oldItem.canMergeWith(newItem)) { + return true + } + } + return false + }, this.preserveOrder, ) serverScanner = new Scanner( @@ -448,9 +462,21 @@ export default class SyncProcess { // (for bookmarks, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is ";") // (for folders because Window IDs are not stable) (oldItem, newItem) => { - if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.canMergeWith(newItem))) { - newMappings.push([oldItem, newItem]) - return true + if (oldItem.type !== newItem.type) { + return false + } + if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') { + return oldItem.url === newItem.url + } + if (oldItem.type === 'folder') { + if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) { + newMappings.push([oldItem, newItem]) + return true + } + if (oldItem.canMergeWith(newItem)) { + newMappings.push([oldItem, newItem]) + return true + } } return false }, From e3e6b930706f2821988416b5098fe64e0fc16918 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Tue, 24 Dec 2024 12:29:40 +0100 Subject: [PATCH 2/3] test: update the server on local changes of url collisions Signed-off-by: Marcel Klehr --- src/test/test.js | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/test/test.js b/src/test/test.js index 0917d04f39..28cc3e8b6a 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -543,6 +543,67 @@ describe('Floccus', function() { false ) }) + it('should update the server on local changes of url collisions', async function() { + if (ACCOUNT_DATA.noCache) { + return this.skip() + } + + const localRoot = account.getData().localRoot + const fooFolder = await browser.bookmarks.create({ + title: 'foo', + parentId: localRoot + }) + const barFolder = await browser.bookmarks.create({ + title: 'bar', + parentId: fooFolder.id + }) + const bookmark1 = await browser.bookmarks.create({ + title: 'ur1l', + url: 'http://ur1.l/', + parentId: fooFolder.id + }) + const bookmark2 = await browser.bookmarks.create({ + title: 'url', + url: 'http://ur.l/', + parentId: barFolder.id + }) + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + const newData = { url: 'http://ur.l/' } + await browser.bookmarks.update(bookmark1.id, newData) + await account.sync() // update on server + expect(account.getData().error).to.not.be.ok + + const tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + new Folder({ + title: 'foo', + children: [ + new Folder({ + title: 'bar', + children: [ + new Bookmark({ + title: bookmark2.title, + url: bookmark2.url + }) + ] + }), + new Bookmark({ + title: ACCOUNT_DATA.type === 'nextcloud-bookmarks' ? bookmark2.title : bookmark1.title, + url: newData.url + }), + ] + }) + ] + }), + false + ) + }) it('should update the server on local removals', async function() { if (ACCOUNT_DATA.noCache) { return this.skip() From d244f6d44f2fb2713382cd564c179ce49fa978f1 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Tue, 24 Dec 2024 13:43:25 +0100 Subject: [PATCH 3/3] fix(SyncProcess): Fix URL collisions on NC Bookmarks Signed-off-by: Marcel Klehr --- src/lib/strategies/Default.ts | 40 +++++++++++++++++++++++++++++------ src/test/test.js | 2 +- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/lib/strategies/Default.ts b/src/lib/strategies/Default.ts index 0c429da39a..3b93f0374e 100644 --- a/src/lib/strategies/Default.ts +++ b/src/lib/strategies/Default.ts @@ -487,8 +487,22 @@ export default class SyncProcess { localScanner = new Scanner( this.cacheTreeRoot, this.localTreeRoot, - (oldItem, newItem) => - (oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)), + (oldItem, newItem) => { + if (oldItem.type !== newItem.type) { + return false + } + if (oldItem.type === 'folder') { + if (String(oldItem.id) === String(newItem.id)) { + return true + } + } + if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') { + if (String(oldItem.id) === String(newItem.id) && oldItem.url === newItem.url) { + return true + } + } + return false + }, this.preserveOrder ) serverScanner = new Scanner( @@ -496,9 +510,24 @@ export default class SyncProcess { this.serverTreeRoot, // We also allow canMergeWith here, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is ";") (oldItem, newItem) => { - if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.type === 'bookmark' && oldItem.canMergeWith(newItem))) { - newMappings.push([oldItem, newItem]) - return true + if (oldItem.type !== newItem.type) { + return false + } + if (oldItem.type === 'folder') { + if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) { + newMappings.push([oldItem, newItem]) + return true + } + } + if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') { + if (Mappings.mappable(mappingsSnapshot, oldItem, newItem) && oldItem.url === newItem.url) { + newMappings.push([oldItem, newItem]) + return true + } + if (oldItem.canMergeWith(newItem)) { + newMappings.push([oldItem, newItem]) + return true + } } return false }, @@ -882,7 +911,6 @@ export default class SyncProcess { } if (action.payload instanceof Folder && action.payload.children.length && action.oldItem instanceof Folder) { - // Fix for Unidirectional reverted REMOVEs, for all other strategies this should be a noop action.payload.children.forEach((item) => { item.parentId = id diff --git a/src/test/test.js b/src/test/test.js index 28cc3e8b6a..8fbc85daf6 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -5284,8 +5284,8 @@ describe('Floccus', function() { new Folder({ title: 'Window 0', children: [ - new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }), new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test2' }), + new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }), new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test4' }), ] })