Fix syncing of bookmark imports

Fix brave/sync#158

Test Plan:
1. Sync Pyramid 0 to Pyramid 1.
2. In Pyramid 0 import a bookmarks file (100s of bookmarks with nested folders)
3. Observe Pyramid 1 for bookmarks to sync over.
4. Compare bookmarks between pyramids. They should be the same.

app/common/lib/bookmarkFoldersUtil.js

@@ -58,7 +58,8 @@ const isFolder = (folder) => {
     return false
   }
 
-  return folder.get('type') === siteTags.BOOKMARK_FOLDER
+  return (folder.get('type') === siteTags.BOOKMARK_FOLDER) ||
+    (typeof folder.get('folderId') === 'number')
 }
 
 const getKey = (folderDetails) => {

app/common/state/bookmarksState.js

@@ -184,7 +184,7 @@ const bookmarksState = {
     }
 
     if (getSetting(settings.SYNC_ENABLED) === true) {
-      syncActions.removeSite(bookmark)
+      syncActions.removeSites([bookmark.toJS()])
     }
 
     state = bookmarkLocationCache.removeCacheKey(state, bookmark.get('location'), bookmarkKey)
@@ -207,17 +207,21 @@ const bookmarksState = {
     }
 
     const syncEnabled = getSetting(settings.SYNC_ENABLED) === true
+    const removedBookmarks = []
     const bookmarks = bookmarksState.getBookmarks(state)
       .filter(bookmark => {
         if (bookmark.get('parentFolderId') !== ~~parentFolderId) {
           return true
         }
         if (syncEnabled) {
-          syncActions.removeSite(bookmark)
+          removedBookmarks.push(bookmark.toJS())
         }
         return false
       })
 
+    if (syncEnabled && removedBookmarks.length) {
+      syncActions.removeSites(removedBookmarks)
+    }
     return state.set(STATE_SITES.BOOKMARKS, bookmarks)
   },
 

app/importer.js

@@ -25,9 +25,11 @@ const settings = require('../js/constants/settings')
 
 // Actions
 const appActions = require('../js/actions/appActions')
+const syncActions = require('../js/actions/syncActions')
 
 // Utils
 const {getSetting} = require('../js/settings')
+const {syncEnabled} = require('../js/state/syncUtil')
 const locale = require('./locale')
 const tabMessageBox = require('./browser/tabMessageBox')
 const {makeImmutable} = require('./common/state/immutableUtil')
@@ -104,11 +106,12 @@ const getParentFolderId = (path, pathMap, addFolderFunction, topLevelFolderId, n
   if (parentFolderId === undefined) {
     parentFolderId = nextFolderIdObject.id++
     pathMap[parentFolder] = parentFolderId
-    addFolderFunction({
+    const folder = {
       title: parentFolder,
       folderId: parentFolderId,
       parentFolderId: getParentFolderId(path, pathMap, addFolderFunction, topLevelFolderId, nextFolderIdObject)
-    })
+    }
+    addFolderFunction(folder)
   }
   return parentFolderId
 }
@@ -122,8 +125,22 @@ importer.on('add-bookmarks', (e, importedBookmarks, topLevelFolder) => {
   let folders = []
   let bookmarks = []
   let topLevelFolderId = nextFolderIdObject.id++
+  const isSyncEnabled = syncEnabled()
+  const syncRecords = []
   const functionBuffer = new FunctionBuffer((args) => makeImmutable(args), this)
+  const bufferedAddBookmark = (bookmark) => {
+    if (isSyncEnabled) {
+      bookmark.skipSync = true
+      syncRecords.push(bookmark)
+    }
+    functionBuffer.buffer(appActions.addBookmark, bookmark)
+    bookmarks.push(bookmark)
+  }
   const bufferedAddFolder = (folder) => {
+    if (isSyncEnabled) {
+      folder.skipSync = true
+      syncRecords.push(folder)
+    }
     functionBuffer.buffer(appActions.addBookmarkFolder, folder)
     folders.push(folder)
   }
@@ -148,21 +165,22 @@ importer.on('add-bookmarks', (e, importedBookmarks, topLevelFolder) => {
         folderId,
         parentFolderId
       }
-      functionBuffer.buffer(appActions.addBookmarkFolder, folder)
-      folders.push(folder)
+      bufferedAddFolder(folder)
     } else {
       const location = importedBookmark.url
       const bookmark = {
         title,
         location,
         parentFolderId
       }
-      functionBuffer.buffer(appActions.addBookmark, bookmark)
-      bookmarks.push(bookmark)
+      bufferedAddBookmark(bookmark)
     }
   }
   functionBuffer.flush()
   bookmarkList = makeImmutable(bookmarks)
+  if (isSyncEnabled && syncRecords.length) {
+    syncActions.addSites(syncRecords)
+  }
 })
 
 importer.on('add-favicons', (e, detail) => {

app/sync.js

@@ -52,10 +52,6 @@ const log = (message) => {
   console.log(`sync ${new Date().getTime()}:`, message)
 }
 
-const syncEnabled = () => {
-  return getSetting(settings.SYNC_ENABLED) === true
-}
-
 let deviceId = null /** @type {Array|null} */
 let pollIntervalId = null
 
@@ -189,15 +185,10 @@ const validateAction = (action) => {
     return false
   }
 
-  // If the action requires an item, validate the item.
+  // If the action requires items, validate the items.
   if (SYNC_ACTIONS_WITHOUT_ITEMS.includes(action.actionType) !== true) {
-    if (!action.item || !action.item.toJS) {
-      log('Missing item!')
-      return false
-    }
-    // Only accept items who have an objectId set already
-    if (!action.item.get('objectId')) {
-      log(`Missing object ID! ${action.item.toJS()}`)
+    if (!action.items || !action.items.length) {
+      log('Missing items!')
       return false
     }
   }
@@ -215,14 +206,29 @@ const dispatcherCallback = (action) => {
     }
   }
   // If sync is not enabled, the following actions should be ignored.
-  if (!syncEnabled() || validateAction(action) !== true || backgroundSender.isDestroyed()) {
+  if (!syncUtil.syncEnabled() || validateAction(action) !== true || backgroundSender.isDestroyed()) {
     return
   }
   switch (action.actionType) {
+    // NOTE: Most sites are actually added via the AppStore change listener.
+    // The bookmarks importer uses this because the it creates jumbled site
+    // diffs.
+    case syncConstants.SYNC_ADD_SITES:
+      sendSyncRecords(backgroundSender, writeActions.CREATE,
+        action.items.map(item => syncUtil.createBookmarkData(item)))
+      break
     // Currently triggered only by bookmarksState and bookmarkFoldersState.
-    case syncConstants.SYNC_REMOVE_SITE:
+    case syncConstants.SYNC_REMOVE_SITES:
+      // Only accept items who have an objectId set already
+      const validItems = action.items.filter(item => {
+        if (item.objectId) {
+          return true
+        }
+        log(`Missing object ID! ${JSON.stringify(item)}`)
+        return false
+      })
       sendSyncRecords(backgroundSender, writeActions.DELETE,
-        [syncUtil.createBookmarkData(action.item.toJS())])
+        validItems.map(item => syncUtil.createBookmarkData(item)))
       break
     case syncConstants.SYNC_CLEAR_HISTORY:
       backgroundSender.send(syncMessages.DELETE_SYNC_CATEGORY, CATEGORY_MAP.historySite.categoryName)
@@ -241,7 +247,7 @@ const dispatcherCallback = (action) => {
  */
 module.exports.onSyncReady = (isFirstRun, e) => {
   appActions.setSyncSetupError(null)
-  if (!syncEnabled()) {
+  if (!syncUtil.syncEnabled()) {
     return
   }
   AppStore.addChangeListener(appStoreChangeCallback)
@@ -423,7 +429,7 @@ module.exports.init = function (appState) {
     // Register the dispatcher callback now that we have a valid sender
     appDispatcher.register(dispatcherCallback)
     // Send the initial data
-    if (syncEnabled()) {
+    if (syncUtil.syncEnabled()) {
       const appState = AppStore.getState().get('sync')
       const seed = appState.get('seed') ? Array.from(appState.get('seed')) : null
       deviceId = appState.get('deviceId') ? Array.from(appState.get('deviceId')) : null
@@ -485,7 +491,7 @@ module.exports.init = function (appState) {
     if (records.length > 0) {
       appActions.pendingSyncRecordsRemoved(records)
     }
-    if (!syncEnabled()) {
+    if (!syncUtil.syncEnabled()) {
       return
     }
     let devices = {}

js/actions/syncActions.js

@@ -7,10 +7,17 @@ const AppDispatcher = require('../dispatcher/appDispatcher')
 const syncConstants = require('../constants/syncConstants')
 
 const syncActions = {
-  removeSite: function (item) {
+  addSites: function (items) {
     AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_REMOVE_SITE,
-      item
+      actionType: syncConstants.SYNC_ADD_SITES,
+      items
+    })
+  },
+
+  removeSites: function (items) {
+    AppDispatcher.dispatch({
+      actionType: syncConstants.SYNC_REMOVE_SITES,
+      items
     })
   },
 

js/constants/syncConstants.js

@@ -6,7 +6,8 @@ const mapValuesByKeys = require('../lib/functional').mapValuesByKeys
 
 const _ = null
 const syncConstants = {
-  SYNC_REMOVE_SITE: _,  /** @param {Immutable.Map} item */
+  SYNC_ADD_SITES: _,  /** @param {Array.<Object>} items */
+  SYNC_REMOVE_SITES: _,  /** @param {Array.<Object>} items */
   SYNC_CLEAR_HISTORY: _,
   SYNC_CLEAR_SITE_SETTINGS: _
 }

js/state/syncUtil.js

@@ -572,6 +572,8 @@ module.exports.newObjectId = (objectPath) => {
  */
 const findOrCreateFolderObjectId = (folderId, appState) => {
   if (typeof folderId !== 'number' || folderId < 0) { return undefined }
+  const cachedObjectId = folderToObjectMap[folderId]
+  if (cachedObjectId) { return cachedObjectId }
   if (!appState) {
     const AppStore = require('../stores/appStore')
     appState = AppStore.getState()
@@ -582,7 +584,9 @@ const findOrCreateFolderObjectId = (folderId, appState) => {
   if (objectId) {
     return objectId.toJS()
   } else {
-    return module.exports.newObjectId([STATE_SITES.BOOKMARK_FOLDERS, folder.get('key')])
+    const newObjectId = module.exports.newObjectId([STATE_SITES.BOOKMARK_FOLDERS, folder.get('key')])
+    folderToObjectMap[folderId] = newObjectId
+    return newObjectId
   }
 }
 
@@ -624,14 +628,13 @@ module.exports.createBookmarkData = (site, appState) => {
 
   const sitesCollection = isFolder ? STATE_SITES.BOOKMARK_FOLDERS : STATE_SITES.BOOKMARKS
   const objectId = site.objectId ||
-    folderToObjectMap[site.folderId] ||
-    module.exports.newObjectId([sitesCollection, siteKey])
+      findOrCreateFolderObjectId(site.folderId, appState) ||
+      module.exports.newObjectId([sitesCollection, siteKey])
   if (!objectId) {
     console.log(`Warning: createBookmarkData can't create site data: ${JSON.stringify(site)}`)
   }
 
   const parentFolderObjectId = site.parentFolderObjectId ||
-    folderToObjectMap[site.parentFolderId] ||
     findOrCreateFolderObjectId(site.parentFolderId, appState)
   const value = {
     site: siteData,