From 030b7d5ef300fdf79fbc8fe2d35183739cc1ecda Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 12:50:23 +0100 Subject: [PATCH 1/6] enh(tabs): Make merge strategy work with tabs reset cache upon browser start and change mergable scanner argument for tabs Signed-off-by: Marcel Klehr --- src/lib/Account.ts | 12 ++--- src/lib/browser/BrowserController.js | 10 +++- src/lib/strategies/Default.ts | 71 ++++++++++++++++++++-------- 3 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/lib/Account.ts b/src/lib/Account.ts index cb72687745..db7c22c594 100644 --- a/src/lib/Account.ts +++ b/src/lib/Account.ts @@ -208,7 +208,7 @@ export default class Account { // main sync steps: mappings = await this.storage.getMappings() - const cacheTree = localResource.constructor.name !== 'LocalTabs' ? await this.storage.getCache() : new Folder({title: '', id: 'tabs', location: ItemLocation.LOCAL}) + const cacheTree = await this.storage.getCache() let continuation = await this.storage.getCurrentContinuation() @@ -280,12 +280,10 @@ export default class Account { await this.setData({ ...this.getData(), scheduled: false, syncing: 1 }) // update cache - if (localResource.constructor.name !== 'LocalTabs') { - const cache = (await localResource.getBookmarksTree()).clone(false) - this.syncProcess.filterOutUnacceptedBookmarks(cache) - this.syncProcess.filterOutUnmappedItems(cache, await mappings.getSnapshot()) - await this.storage.setCache(cache) - } + const cache = (await localResource.getBookmarksTree()).clone(false) + this.syncProcess.filterOutUnacceptedBookmarks(cache) + this.syncProcess.filterOutUnmappedItems(cache, await mappings.getSnapshot()) + await this.storage.setCache(cache) if (this.server.onSyncComplete) { await this.server.onSyncComplete() diff --git a/src/lib/browser/BrowserController.js b/src/lib/browser/BrowserController.js index 2f1498ee3e..b2d02459d5 100644 --- a/src/lib/browser/BrowserController.js +++ b/src/lib/browser/BrowserController.js @@ -167,6 +167,9 @@ export default class BrowserController { } } }) + + // Run some things on browser startup + browser.runtime.onStartup.addListener(this.onStartup) } async _receiveEvent(data, sendResponse) { @@ -412,7 +415,7 @@ export default class BrowserController { } } - async onLoad() { + async onStartup() { const accounts = await Account.getAllAccounts() await Promise.all( accounts.map(async acc => { @@ -423,9 +426,14 @@ export default class BrowserController { scheduled: acc.getData().enabled, }) } + if (acc.getData().localRoot === 'tabs') { + await acc.init() + } }) ) + } + async onLoad() { browser.storage.local.get('telemetryEnabled').then(async d => { if (!d.telemetryEnabled) { return diff --git a/src/lib/strategies/Default.ts b/src/lib/strategies/Default.ts index dd70fe72d0..0dc5972aa6 100644 --- a/src/lib/strategies/Default.ts +++ b/src/lib/strategies/Default.ts @@ -27,6 +27,7 @@ import { CancelledSyncError, FailsafeError } from '../../errors/Error' import NextcloudBookmarksAdapter from '../adapters/NextcloudBookmarks' import CachingAdapter from '../adapters/Caching' +import LocalTabs from '../LocalTabs' const ACTION_CONCURRENCY = 12 @@ -424,27 +425,55 @@ export default class SyncProcess { const newMappings = [] - // if we have the cache available, Diff cache and both trees - const localScanner = new Scanner( - this.cacheTreeRoot, - this.localTreeRoot, - (oldItem, newItem) => - (oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)), - this.preserveOrder - ) - const serverScanner = new Scanner( - this.cacheTreeRoot, - 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 - } - return false - }, - this.preserveOrder - ) + let localScanner, serverScanner + if (this.localTree instanceof LocalTabs) { + // if we have the cache available, Diff cache and both trees + localScanner = new Scanner( + 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)), + this.preserveOrder, + ) + serverScanner = new Scanner( + this.cacheTreeRoot, + this.serverTreeRoot, + // We also allow canMergeWith here + // (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 + } + return false + }, + this.preserveOrder, + ) + } else { + // if we have the cache available, Diff cache and both trees + localScanner = new Scanner( + this.cacheTreeRoot, + this.localTreeRoot, + (oldItem, newItem) => + (oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)), + this.preserveOrder + ) + serverScanner = new Scanner( + this.cacheTreeRoot, + 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 + } + return false + }, + this.preserveOrder + ) + } Logger.log('Calculating diffs for local and server trees relative to cache tree') const localScanResult = await localScanner.run() const serverScanResult = await serverScanner.run() From 763f46472e4a3d99a1097ae3feffe49c0062a559 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 14:11:43 +0100 Subject: [PATCH 2/6] fix(tests) Signed-off-by: Marcel Klehr --- src/test/test.js | 88 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 3 deletions(-) diff --git a/src/test/test.js b/src/test/test.js index e191ad6f27..d95b22bd3c 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -4904,10 +4904,10 @@ describe('Floccus', function() { windowType: 'normal' // no devtools or panels or popups }) await browser.tabs.remove(tabs.filter(tab => tab.url.startsWith('http')).map(tab => tab.id)) - await awaitTabsUpdated() } catch (e) { console.error(e) } + await awaitTabsUpdated() if (ACCOUNT_DATA.type === 'git') { await account.server.clearServer() } else if (ACCOUNT_DATA.type !== 'fake') { @@ -5115,14 +5115,14 @@ describe('Floccus', function() { serverMark2 = { title: 'Example Domain', url: 'https://example.org/#test3', - parentId: windowFolderId, + parentId: tree.children[0].id, location: ItemLocation.SERVER } await adapter.createBookmark( new Bookmark(serverMark2) ) - await adapter.updateBookmark({ ...serverMark, id: serverMarkId, url: 'https://example.org/#test2', title: 'Example Domain', parentId: windowFolderId }) + await adapter.updateBookmark({ ...serverMark, id: serverMarkId, url: 'https://example.org/#test2', title: 'Example Domain', parentId: tree.children[0].id }) }) await account.setData({...account.getData(), strategy: 'slave'}) @@ -5148,6 +5148,88 @@ describe('Floccus', function() { false ) }) + it('should sync tabs correctly when merging server and local changes', async function() { + const adapter = account.server + const serverTree = await getAllBookmarks(account) + let windowFolderId, serverMark, serverMarkId + await withSyncConnection(account, async() => { + windowFolderId = await adapter.createFolder(new Folder({ + parentId: serverTree.id, + title: 'Window 0', + location: ItemLocation.SERVER + })) + serverMark = { + title: 'Example Domain', + url: 'https://example.org/#test1', + parentId: windowFolderId, + location: ItemLocation.SERVER + } + + serverMarkId = await adapter.createBookmark( + new Bookmark(serverMark) + ) + }) + + await account.sync() + expect(account.getData().error).to.not.be.ok + + let tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + new Folder({ + title: 'Window 0', + children: [ + new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test1' }), + ] + }) + ] + }), + false + ) + + let serverMark2 + await withSyncConnection(account, async() => { + serverMark2 = { + title: 'Example Domain', + url: 'https://example.org/#test3', + parentId: tree.children[0].id, + location: ItemLocation.SERVER + } + await adapter.createBookmark( + new Bookmark(serverMark2) + ) + + await adapter.updateBookmark({ ...serverMark, id: serverMarkId, url: 'https://example.org/#test2', title: 'Example Domain', parentId: tree.children[0].id }) + }) + + await browser.tabs.create({url: 'https://example.org/#test4'}) + await awaitTabsUpdated() + + await account.sync() + expect(account.getData().error).to.not.be.ok + + tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + 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/#test4' }), + ] + }) + ] + }), + false + ) + }) }) }) }) From 32c02851df216652e4f5363f6c6dde0358751daf Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 16:28:14 +0100 Subject: [PATCH 3/6] fix(LocalTabs): implement OrderFolderResource Signed-off-by: Marcel Klehr --- src/lib/LocalTabs.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/LocalTabs.ts b/src/lib/LocalTabs.ts index dbca1acefb..f8c862392b 100644 --- a/src/lib/LocalTabs.ts +++ b/src/lib/LocalTabs.ts @@ -1,12 +1,12 @@ import browser from './browser-api' import Logger from './Logger' -import { IResource } from './interfaces/Resource' +import { OrderFolderResource } from './interfaces/Resource' import PQueue from 'p-queue' import { Bookmark, Folder, ItemLocation } from './Tree' import Ordering from './interfaces/Ordering' import uniq from 'lodash/uniq' -export default class LocalTabs implements IResource { +export default class LocalTabs implements OrderFolderResource { private queue: PQueue<{ concurrency: 10 }> private storage: unknown From 7211af615a44680c7da766464dc3bd6abff23449 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 16:34:25 +0100 Subject: [PATCH 4/6] fix(tests) Signed-off-by: Marcel Klehr --- src/test/test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/test.js b/src/test/test.js index d95b22bd3c..e141b2ec92 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -5227,7 +5227,8 @@ describe('Floccus', function() { }) ] }), - false + false, + false, // We're merging which doesn't guarantee an order ) }) }) From 1711129f0533fff61a9911a16b098c7ba01ebcd2 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 16:54:28 +0100 Subject: [PATCH 5/6] fix(LocalTabs): Wait until tab update is finished before we yield to sync process Signed-off-by: Marcel Klehr --- src/lib/Account.ts | 2 +- src/lib/LocalTabs.ts | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/lib/Account.ts b/src/lib/Account.ts index db7c22c594..a215633335 100644 --- a/src/lib/Account.ts +++ b/src/lib/Account.ts @@ -1,6 +1,6 @@ import AdapterFactory from './AdapterFactory' import Logger from './Logger' -import { Folder, ItemLocation, TItemLocation } from './Tree' +import { ItemLocation, TItemLocation } from './Tree' import UnidirectionalSyncProcess from './strategies/Unidirectional' import MergeSyncProcess from './strategies/Merge' import DefaultSyncProcess from './strategies/Default' diff --git a/src/lib/LocalTabs.ts b/src/lib/LocalTabs.ts index f8c862392b..28b6a822ea 100644 --- a/src/lib/LocalTabs.ts +++ b/src/lib/LocalTabs.ts @@ -65,6 +65,7 @@ export default class LocalTabs implements OrderFolderResource): Promise { @@ -95,6 +97,7 @@ export default class LocalTabs implements OrderFolderResource browser.tabs.remove(bookmarkId)) + await awaitTabsUpdated() } async createFolder(folder:Folder): Promise { @@ -131,6 +134,7 @@ export default class LocalTabs implements OrderFolderResource):Promise { @@ -140,7 +144,7 @@ export default class LocalTabs implements OrderFolderResource):Promise { const id = folder.id Logger.log('(tabs)REMOVEFOLDER', id) - await this.queue.add(() => browser.tabs.remove(id)) + await this.queue.add(() => browser.window.remove(id)) } async isAvailable(): Promise { @@ -150,3 +154,15 @@ export default class LocalTabs implements OrderFolderResource(resolve => { + browser.tabs.onUpdated.addListener(() => { + browser.tabs.onUpdated.removeListener(resolve) + setTimeout(() => resolve(), 1000) + }) + }), + new Promise(resolve => setTimeout(resolve, 1100)) + ]) +} From eb31b8360f17554046d57809d1df712eb1522e27 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 27 Nov 2024 17:11:26 +0100 Subject: [PATCH 6/6] fix(tests) Signed-off-by: Marcel Klehr --- src/test/test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/test.js b/src/test/test.js index e141b2ec92..0917d04f39 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -5149,6 +5149,9 @@ describe('Floccus', function() { ) }) it('should sync tabs correctly when merging server and local changes', async function() { + if (ACCOUNT_DATA.noCache) { + return this.skip() + } const adapter = account.server const serverTree = await getAllBookmarks(account) let windowFolderId, serverMark, serverMarkId