Merge pull request #1781 from floccusaddon/enh/make-tabsync-work

Make merge strategy work with tabs

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'
@@ -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()

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<typeof ItemLocation.LOCAL> {
+export default class LocalTabs implements OrderFolderResource<typeof ItemLocation.LOCAL> {
   private queue: PQueue<{ concurrency: 10 }>
   private storage: unknown
 
@@ -65,6 +65,7 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
         active: false,
       })
     )
+    await awaitTabsUpdated()
     return node.id
   }
 
@@ -85,6 +86,7 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
         index: -1 // last
       })
     )
+    await awaitTabsUpdated()
   }
 
   async removeBookmark(bookmark:Bookmark<typeof ItemLocation.LOCAL>): Promise<void> {
@@ -95,6 +97,7 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
       return
     }
     await this.queue.add(() => browser.tabs.remove(bookmarkId))
+    await awaitTabsUpdated()
   }
 
   async createFolder(folder:Folder<typeof ItemLocation.LOCAL>): Promise<number> {
@@ -131,6 +134,7 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
         throw new Error('Failed to reorder folder ' + id + ': ' + e.message)
       }
     }
+    await awaitTabsUpdated()
   }
 
   async updateFolder(folder:Folder<typeof ItemLocation.LOCAL>):Promise<void> {
@@ -140,7 +144,7 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
   async removeFolder(folder:Folder<typeof ItemLocation.LOCAL>):Promise<void> {
     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<boolean> {
@@ -150,3 +154,15 @@ export default class LocalTabs implements IResource<typeof ItemLocation.LOCAL> {
     return Boolean(tabs.length)
   }
 }
+
+function awaitTabsUpdated() {
+  return Promise.race([
+    new Promise<void>(resolve => {
+      browser.tabs.onUpdated.addListener(() => {
+        browser.tabs.onUpdated.removeListener(resolve)
+        setTimeout(() => resolve(), 1000)
+      })
+    }),
+    new Promise(resolve => setTimeout(resolve, 1100))
+  ])
+}

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

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 "<bookmarkID>;<folderId>")
-      (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 "<bookmarkID>;<folderId>")
+        // (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 "<bookmarkID>;<folderId>")
+        (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()

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,92 @@ describe('Floccus', function() {
               false
             )
           })
+          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
+            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,
+              false, // We're merging which doesn't guarantee an order
+            )
+          })
         })
       })
   })