Fix: Fix handling of residual actions from subordinate scanner runs

.github/workflows/tests.yml

@@ -89,6 +89,7 @@ jobs:
         npm-version: [10.x]
         server-version: ['32']
         app-version: ['stable']
+        seed: ["${{ github.sha }}"]
         floccus-adapter:
           - fake
           - nextcloud-bookmarks
@@ -117,43 +118,89 @@ jobs:
             browsers: firefox
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
           - app-version: master
             server-version: 32
             floccus-adapter: nextcloud-bookmarks
             test-name: benchmark root
             browsers: firefox
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
           - app-version: master
             server-version: 32
             floccus-adapter: nextcloud-bookmarks
             test-name: benchmark root
             browsers: chrome
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
           - app-version: stable
             server-version: 32
             floccus-adapter: fake-noCache
             test-name: test
             browsers: firefox
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
           - app-version: master
             server-version: 32
             floccus-adapter: fake
             test-name: benchmark root
             browsers: firefox
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
           - app-version: master
             server-version: 32
             floccus-adapter: fake
             test-name: benchmark root
             browsers: chrome
             node-version: 14.x
             npm-version: 7.x
+            seed: ${{ github.sha }}
+          - app-version: master
+            server-version: 32
+            floccus-adapter: fake
+            test-name: benchmark root
+            browsers: chrome
+            node-version: 14.x
+            npm-version: 7.x
+            seed: "3"
+          - app-version: master
+            server-version: 32
+            floccus-adapter: fake
+            test-name: benchmark root
+            browsers: chrome
+            node-version: 14.x
+            npm-version: 7.x
+            seed: "6"
+          - app-version: master
+            server-version: 32
+            floccus-adapter: fake
+            test-name: benchmark root
+            browsers: chrome
+            node-version: 14.x
+            npm-version: 7.x
+            seed: "0.5202964461460992"
+          - app-version: master
+            server-version: 32
+            floccus-adapter: fake
+            test-name: benchmark root
+            browsers: chrome
+            node-version: 14.x
+            npm-version: 7.x
+            seed: "12dd05da349df3d71c9e48e764f5d50ec2bbc88d"
+          - app-version: master
+            server-version: 32
+            floccus-adapter: fake
+            test-name: benchmark root
+            browsers: chrome
+            node-version: 14.x
+            npm-version: 7.x
+            seed: "cfc00c4b69afec232e3b26244c2bc98ed6d547b6"
 
-    name: ${{ matrix.browsers == 'firefox' && '🦊' || '🔵' }} ${{matrix.floccus-adapter}}:${{ matrix.test-name}} ⭐${{ matrix.app-version }}
+    name: ${{ matrix.browsers == 'firefox' && '🦊' || '🔵' }} ${{matrix.floccus-adapter}}:${{ matrix.test-name}} ⭐${{ matrix.app-version }} ${{ matrix.test-name == 'benchmark root' && matrix.seed != github.sha && '🌱' || ''}}${{ matrix.test-name == 'benchmark root' && matrix.seed != github.sha && matrix.seed || ''}}
 
     services:
       hub:
@@ -272,7 +319,7 @@ jobs:
         env:
           SELENIUM_BROWSER: ${{ matrix.browsers }}
           FLOCCUS_TEST: ${{matrix.floccus-adapter}} ${{ matrix.test-name}}
-          FLOCCUS_TEST_SEED: ${{ github.sha }}
+          FLOCCUS_TEST_SEED: ${{matrix.seed}}
           TEST_HOST: 172.17.0.1:8174
         run: |
           npm run test
@@ -283,7 +330,7 @@ jobs:
         env:
           SELENIUM_BROWSER: ${{ matrix.browsers }}
           FLOCCUS_TEST: ${{matrix.floccus-adapter}} ${{ matrix.test-name}}
-          FLOCCUS_TEST_SEED: ${{ github.sha }}
+          FLOCCUS_TEST_SEED: ${{matrix.seed}}
           GIST_TOKEN: ${{ secrets.GIST_TOKEN }}
           GOOGLE_API_REFRESH_TOKEN: ${{ secrets.GOOGLE_API_REFRESH_TOKEN }}
           DROPBOX_API_REFRESH_TOKEN: ${{ secrets.DROPBOX_API_REFRESH_TOKEN }}

src/lib/strategies/Default.ts

@@ -776,12 +776,17 @@ export default class SyncProcess {
           this.hashSettings,
           false
         )
-        await subScanner.run()
+        const scanResult = await subScanner.run()
         newMappings.push([concurrentCreation.payload, action.payload.id])
         await Parallel.each(newMappings, async([oldItem, newId]) => {
           await this.addMapping(action.payload.location === ItemLocation.LOCAL ? this.localTree : this.server, oldItem, newId)
         },1)
-        // TODO: subScanner may contain residual CREATE/REMOVE actions that need to be added to mappings
+
+        // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here
+        // We do not act on REMOVEs, only on CREATEs as we merge the two sides
+        scanResult.CREATE.getActions().forEach(action =>
+          targetPlan.CREATE.commit({type: action.type, payload: action.payload})
+        )
         return
       }
 

src/lib/strategies/Merge.ts

@@ -152,11 +152,11 @@ export default class MergeSyncProcess extends DefaultSyncProcess {
             this.hashSettings,
             false
           )
-          await subScanner.run()
+          const scanResult = await subScanner.run()
           newMappings.push([concurrentCreation.payload, action.payload.id])
           await Parallel.each(
             newMappings,
-            async([oldItem, newId]) => {
+            async ([oldItem, newId]) => {
               await this.addMapping(
                 action.payload.location === ItemLocation.LOCAL
                   ? this.localTree
@@ -167,7 +167,16 @@ export default class MergeSyncProcess extends DefaultSyncProcess {
             },
             1
           )
-          // TODO: subScanner may contain residual CREATE/REMOVE actions that need to be added to mappings
+
+          // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here
+          // We do not act on REMOVEs, only on CREATEs as we merge the two sides
+          scanResult.CREATE.getActions().forEach((action) =>
+            targetPlan.CREATE.commit({
+              type: action.type,
+              payload: action.payload,
+            })
+          )
+
           return
         }
 

src/test/test.js

@@ -4476,6 +4476,77 @@ describe('Floccus', function() {
               false
             )
           })
+          it('should keep residual creates when merging concurrently created folders', async function() {
+            const localRoot1 = account1.getData().localRoot
+            const localRoot2 = account2.getData().localRoot
+
+            await browser.bookmarks.create({
+              title: 'unrelated',
+              parentId: localRoot1
+            })
+
+            await account1.sync()
+            expect(account1.getData().error).to.not.be.ok
+            await account2.sync()
+            expect(account2.getData().error).to.not.be.ok
+
+            const folder1 = await browser.bookmarks.create({
+              title: 'shared',
+              parentId: localRoot1
+            })
+            await browser.bookmarks.create({
+              title: 'from account 1',
+              url: 'https://account1.example/',
+              parentId: folder1.id
+            })
+
+            const folder2 = await browser.bookmarks.create({
+              title: 'shared',
+              parentId: localRoot2
+            })
+            await browser.bookmarks.create({
+              title: 'from account 2',
+              url: 'https://account2.example/',
+              parentId: folder2.id
+            })
+
+            await account1.sync()
+            expect(account1.getData().error).to.not.be.ok
+            await account2.sync()
+            expect(account2.getData().error).to.not.be.ok
+            await account1.sync()
+            expect(account1.getData().error).to.not.be.ok
+
+            const serverTree = await getAllBookmarks(account1)
+            expectTreeEqual(
+              serverTree,
+              new Folder({
+                title: serverTree.title,
+                children: [
+                  new Folder({
+                    title: 'unrelated',
+                    children: [],
+                  }),
+                  new Folder({
+                    title: 'shared',
+                    children: [
+                      new Bookmark({ title: 'from account 1', url: 'https://account1.example/' }),
+                      new Bookmark({ title: 'from account 2', url: 'https://account2.example/' }),
+                    ]
+                  })
+                ]
+              }),
+              false,
+              false
+            )
+
+            const tree1 = await account1.localTree.getBookmarksTree(true)
+            const tree2 = await account2.localTree.getBookmarksTree(true)
+            tree1.title = serverTree.title
+            tree2.title = serverTree.title
+            expectTreeEqual(tree1, serverTree, false, false)
+            expectTreeEqual(tree2, serverTree, false, false)
+          })
           it('should handle concurrent hierarchy reversals', async function() {
             const localRoot = account1.getData().localRoot
             const aFolder = await browser.bookmarks.create({