Fix sync-after-import-bookmark hierarchy

Fix #8892 Fix #9405 Fix #9404
brave · Jun 12, 2017 · 3ea1f8f · 3ea1f8f
1 parent ba4d7d4
commit 3ea1f8f
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 23 deletions.
diff --git a/app/sessionStore.js b/app/sessionStore.js
@@ -296,6 +296,10 @@ module.exports.cleanAppData = (data, isShutdown) => {
   if (data.dragData) {
     delete data.dragData
   }
+  if (data.sync) {
+    // clear sync site cache
+    data.sync.objectsById = {}
+  }
   const clearSiteSettings = isShutdown && getSetting(settings.SHUTDOWN_CLEAR_SITE_SETTINGS) === true
   if (clearSiteSettings) {
     data.siteSettings = {}

diff --git a/app/sync.js b/app/sync.js
@@ -95,6 +95,11 @@ const appStoreChangeCallback = function (diffs) {
     const entry = state.getIn(statePath)
     const isSite = type === 'sites'
 
+    if (action === writeActions.CREATE && entry && entry.get('skipSync')) {
+      // Don't re-create objects that were fetched by sync
+      return
+    }
+
     if (isSite && action === writeActions.DELETE && !entry) {
       // If we deleted the site, it is no longer availble in appState.
       // Find the corresponding objectId using the sync cache

diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js
@@ -65,7 +65,13 @@ const deviceIdString = (deviceId) => {
 module.exports.deviceIdString = deviceIdString
 
 // Cache of bookmark folder object IDs mapped to folder IDs
-let folderIdMap = new Immutable.Map()
+// Used when receiving folder records
+let objectToFolderMap = new Immutable.Map()
+// Cache of folder IDs mapped to object IDs to prevent double-assigning object IDs to
+// folders due to delay in appState propagation. See
+// https://github.com/brave/browser-laptop/issues/8892#issuecomment-307954974
+// Used when sending folder records
+const folderToObjectMap = {}
 
 /**
  * Converts sync records into a form that can be consumed by AppStore.
@@ -373,8 +379,8 @@ const getObjectById = (objectId, category, appState) => {
  * @returns {number|undefined}
  */
 const getFolderIdByObjectId = (objectId, appState, records) => {
-  if (folderIdMap.has(objectId)) {
-    return folderIdMap.get(objectId)
+  if (objectToFolderMap.has(objectId)) {
+    return objectToFolderMap.get(objectId)
   }
   let folderId
   const entry = getObjectById(objectId, 'BOOKMARKS', appState)
@@ -391,7 +397,7 @@ const getFolderIdByObjectId = (objectId, appState, records) => {
     }
   }
   if (folderId) {
-    folderIdMap = folderIdMap.set(objectId, folderId)
+    objectToFolderMap = objectToFolderMap.set(objectId, folderId)
   }
   return folderId
 }
@@ -444,10 +450,6 @@ module.exports.newObjectId = (objectPath) => {
  */
 const findOrCreateFolderObjectId = (folderId, appState) => {
   if (typeof folderId !== 'number' || folderId < 0) { return undefined }
-  if (!appState) {
-    const AppStore = require('../stores/appStore')
-    appState = AppStore.getState()
-  }
   const folder = appState.getIn(['sites', folderId.toString()])
   if (!folder) { return undefined }
   const objectId = folder.get('objectId')
@@ -483,25 +485,40 @@ module.exports.createSiteData = (site, appState) => {
   if (siteKey === null) {
     throw new Error('Sync could not create siteKey')
   }
+  let name
+  let objectId
+  let parentFolderObjectId
+  let value
   if (module.exports.isSyncable('bookmark', immutableSite)) {
-    const objectId = site.objectId || module.exports.newObjectId(['sites', siteKey])
-    const parentFolderObjectId = site.parentFolderObjectId ||
-      (site.parentFolderId && findOrCreateFolderObjectId(site.parentFolderId, appState))
-    return {
-      name: 'bookmark',
-      objectId,
-      value: {
-        site: siteData,
-        isFolder: siteUtil.isFolder(immutableSite),
-        hideInToolbar: site.parentFolderId === -1,
-        parentFolderObjectId
-      }
+    name = 'bookmark'
+    objectId = site.objectId ||
+      folderToObjectMap[site.folderId] ||
+      module.exports.newObjectId(['sites', siteKey])
+    parentFolderObjectId = site.parentFolderObjectId ||
+      folderToObjectMap[site.parentFolderId] ||
+      findOrCreateFolderObjectId(site.parentFolderId, appState)
+    value = {
+      site: siteData,
+      isFolder: siteUtil.isFolder(immutableSite),
+      hideInToolbar: site.parentFolderId === -1,
+      parentFolderObjectId
     }
   } else if (siteUtil.isHistoryEntry(immutableSite)) {
+    objectId = site.objectId || module.exports.newObjectId(['sites', siteKey])
+    name = 'historySite'
+    value = siteData
+  }
+  if (objectId) {
+    if (typeof site.folderId === 'number') {
+      folderToObjectMap[site.folderId] = objectId
+    }
+    if (typeof site.parentFolderId === 'number' && parentFolderObjectId) {
+      folderToObjectMap[site.parentFolderId] = parentFolderObjectId
+    }
     return {
-      name: 'historySite',
-      objectId: site.objectId || module.exports.newObjectId(['sites', siteKey]),
-      value: siteData
+      name,
+      objectId,
+      value
     }
   }
   console.log(`Ignoring entry because we can't create site data: ${JSON.stringify(site)}`)