Merge pull request #1807 from floccusaddon/fix/nc-bookmarks-update

fix: Don't produce UPDATE when URL changes
floccusaddon · Dec 24, 2024 · da69650 · da69650
2 parents 58197a9 + d244f6d
commit da69650
Show file tree

Hide file tree

Showing 2 changed files with 128 additions and 13 deletions.
diff --git 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 "<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
+          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
         },
@@ -461,18 +487,47 @@ 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(
         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
+          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
         },
@@ -856,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
@@ -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()
@@ -5223,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' }),
                     ]
                   })