Cache Sync bookmarks and history by objectId

app/sessionStore.js

@@ -590,7 +590,8 @@ module.exports.defaultAppState = () => {
   return {
     firstRunTimestamp: new Date().getTime(),
     sync: {
-      lastFetchTimestamp: 0
+      lastFetchTimestamp: 0,
+      objectsById: {}
     },
     sites: getTopSiteMap(),
     tabs: [],

app/sync.js

@@ -232,6 +232,9 @@ module.exports.onSyncReady = (isFirstRun, e) => {
         return syncUtil.createSiteSettingsData(item, siteSettings[item])
       }))
   }
+
+  appActions.createSyncCache()
+
   // Periodically poll for new records
   let startAt = appState.getIn(['sync', 'lastFetchTimestamp']) || 0
   const poll = () => {

docs/state.md

@@ -270,6 +270,9 @@ AppStore
     lastFetchTimestamp: integer // the last time new sync records were fetched in seconds
     deviceId: Array.<number>,
     objectId: Array.<number>, // objectId for this sync device
+    objectsById: {
+      [string of objectId joined by pipes |]: Array.<string> // array key path within appState, so we can do appState.getIn({key path})
+    },
     seed: Array.<number>,
     seedQr: string, // data URL of QR code representing the seed
   },

js/actions/appActions.js

@@ -894,6 +894,15 @@ const appActions = {
     })
   },
 
+  /**
+   * Dispatch to populate the sync object id -> appState key path mapping cache
+   */
+  createSyncCache: function () {
+    AppDispatcher.dispatch({
+      actionType: appConstants.APP_CREATE_SYNC_CACHE
+    })
+  },
+
   /**
    * Dispatches a message to delete sync data.
    */

js/constants/appConstants.js

@@ -91,6 +91,7 @@ const appConstants = {
   APP_TAB_TOGGLE_DEV_TOOLS: _,
   APP_TAB_CLONED: _,
   APP_SET_OBJECT_ID: _,
+  APP_CREATE_SYNC_CACHE: _,
   APP_SAVE_SYNC_INIT_DATA: _,
   APP_RESET_SYNC_DATA: _,
   APP_ADD_NOSCRIPT_EXCEPTIONS: _,

js/state/syncUtil.js

@@ -234,6 +234,7 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
   const existingObject = module.exports.getObjectById(objectId, categoryName, appState)
   if (!existingObject) { return null }
 
+  const existingObjectKeyPath = existingObject[0]
   const existingObjectData = existingObject[1].toJS()
   let item
   switch (categoryName) {
@@ -242,7 +243,7 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
       item = module.exports.createSiteData(existingObjectData, appState)
       break
     case 'PREFERENCES':
-      const hostPattern = existingObject[0]
+      const hostPattern = existingObjectKeyPath[existingObjectKeyPath.length - 1]
       item = module.exports.createSiteSettingsData(hostPattern, existingObjectData)
       break
     default:
@@ -260,6 +261,46 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
   }
 }
 
+/**
+ * Cache sync objects' key paths by objectIds.
+ * NOTE: Deletes current cache entries.
+ * XXX: Currently only caches sites (history and bookmarks).
+ * @param {Immutable.Map} appState application state
+ * @returns {Immutable.Map} new app state
+ */
+module.exports.createSiteCache = (appState) => {
+  const objectsById = new Immutable.Map().withMutations(objectsById => {
+    appState.get('sites').forEach((site, siteKey) => {
+      const objectId = site.get('objectId')
+      if (!objectId) { return true }
+      const cacheKey = objectId.toJS().join('|')
+      objectsById = objectsById.set(cacheKey, ['sites', siteKey])
+    })
+  })
+  return appState.setIn(['sync', 'objectsById'], objectsById)
+}
+
+/**
+ * Cache a sync object's key path by objectId.
+ * XXX: Currently only caches sites (history and bookmarks).
+ * @param {Immutable.Map} appState application state
+ * @param {Immutable.Map} siteDetail
+ * @returns {Immutable.Map} new app state
+ */
+module.exports.updateSiteCache = (appState, siteDetail) => {
+  if (!siteDetail) { return appState }
+  const siteKey = siteUtil.getSiteKey(siteDetail)
+  const object = appState.getIn(['sites', siteKey])
+  const objectId = (object && object.get('objectId')) || siteDetail.get('objectId')
+  if (!objectId) { return appState }
+  const cacheKey = ['sync', 'objectsById', objectId.toJS().join('|')]
+  if (object) {
+    return appState.setIn(cacheKey, ['sites', siteKey])
+  } else {
+    return appState.deleteIn(cacheKey)
+  }
+}
+
 /**
  * Given an objectId and category, return the matching browser object.
  * @param {Immutable.List} objectId
@@ -278,17 +319,14 @@ module.exports.getObjectById = (objectId, category, appState) => {
   }
   switch (category) {
     case 'BOOKMARKS':
-      return appState.get('sites').findEntry((site, index) => {
-        const itemObjectId = site.get('objectId')
-        const isBookmark = siteUtil.isFolder(site) || siteUtil.isBookmark(site)
-        return (isBookmark && itemObjectId && itemObjectId.equals(objectId))
-      })
     case 'HISTORY_SITES':
-      return appState.get('sites').findEntry((site, index) => {
-        const itemObjectId = site.get('objectId')
-        const isBookmark = siteUtil.isFolder(site) || siteUtil.isBookmark(site)
-        return (!isBookmark && itemObjectId && itemObjectId.equals(objectId))
-      })
+      const objectKey = appState.getIn(['sync', 'objectsById', objectId.toJS().join('|')])
+      const object = objectKey && appState.getIn(objectKey)
+      if (!object) {
+        return null
+      } else {
+        return [objectKey, object]
+      }
     case 'PREFERENCES':
       return appState.get('siteSettings').findEntry((siteSetting, hostPattern) => {
         const itemObjectId = siteSetting.get('objectId')

js/stores/appStore.js

@@ -9,6 +9,7 @@ const ExtensionConstants = require('../../app/common/constants/extensionConstant
 const AppDispatcher = require('../dispatcher/appDispatcher')
 const appConfig = require('../constants/appConfig')
 const settings = require('../constants/settings')
+const siteTags = require('../constants/siteTags')
 const writeActions = require('../constants/sync/proto').actions
 const siteUtil = require('../state/siteUtil')
 const syncUtil = require('../state/syncUtil')
@@ -339,6 +340,10 @@ function handleChangeSettingAction (settingKey, settingValue) {
   }
 }
 
+const syncEnabled = () => {
+  return getSetting(settings.SYNC_ENABLED) === true
+}
+
 const applyReducers = (state, action) => [
   require('../../app/browser/reducers/downloadsReducer'),
   require('../../app/browser/reducers/flashReducer'),
@@ -439,7 +444,8 @@ const handleAppAction = (action) => {
         let siteDetail = siteData.siteDetail
         const sites = appState.get('sites')
         if (record.action !== writeActions.DELETE &&
-          !siteDetail.get('folderId') && siteUtil.isFolder(siteDetail)) {
+          tag === siteTags.BOOKMARK_FOLDER &&
+          !siteDetail.get('folderId')) {
           siteDetail = siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
         }
         switch (record.action) {
@@ -456,6 +462,7 @@ const handleAppAction = (action) => {
               siteUtil.removeSite(sites, siteDetail, tag))
             break
         }
+        appState = syncUtil.updateSiteCache(appState, siteDetail)
       })
       appState = aboutNewTabState.setSites(appState)
       appState = aboutHistoryState.setHistory(appState)
@@ -482,12 +489,18 @@ const handleAppAction = (action) => {
       if (oldSiteSize !== appState.get('sites').size) {
         filterOutNonRecents()
       }
+      if (syncEnabled()) {
+        appState = syncUtil.updateSiteCache(appState, action.destinationDetail || action.siteDetail)
+      }
       appState = aboutNewTabState.setSites(appState, action)
       appState = aboutHistoryState.setHistory(appState, action)
       break
     case appConstants.APP_REMOVE_SITE:
       const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
       appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag, true, removeSiteSyncCallback))
+      if (syncEnabled()) {
+        appState = syncUtil.updateSiteCache(appState, action.siteDetail)
+      }
       appState = aboutNewTabState.setSites(appState, action)
       appState = aboutHistoryState.setHistory(appState, action)
       break
@@ -496,6 +509,9 @@ const handleAppAction = (action) => {
         appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
           action.sourceDetail, action.destinationDetail, action.prepend,
           action.destinationIsParent, false, syncActions.updateSite))
+        if (syncEnabled()) {
+          appState = syncUtil.updateSiteCache(appState, action.destinationDetail)
+        }
         break
       }
     case appConstants.APP_CLEAR_HISTORY:
@@ -875,6 +891,9 @@ const handleAppAction = (action) => {
         appState = appState.setIn(['sync', 'seedQr'], action.seedQr)
       }
       break
+    case appConstants.APP_CREATE_SYNC_CACHE:
+      appState = syncUtil.createSiteCache(appState)
+      break
     case appConstants.APP_RESET_SYNC_DATA:
       const sessionStore = require('../../app/sessionStore')
       const syncDefault = Immutable.fromJS(sessionStore.defaultAppState().sync)
@@ -889,6 +908,7 @@ const handleAppAction = (action) => {
           appState = appState.setIn(['sites', key, 'originalSeed'], originalSeed)
         }
       })
+      appState.setIn(['sync', 'objectsById'], {})
       break
     case appConstants.APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION:
       appState = appState.set('deleteConfirmationVisible', true)

test/misc-components/syncTest.js

@@ -392,7 +392,7 @@ describe('Syncing bookmarks', function () {
     const pageNthChild = 1
     const folderTitle = this.folder1Title
     const pageTitle = this.folder1Page1Title
-    const folder = `.bookmarkToolbarButton[title="${folderTitle}"]`
+    const folder = `[data-test-id="bookmarkToolbarButton"][title="${folderTitle}"]`
     yield Brave.app.client
       .waitForVisible(folder)
       .click(folder)
@@ -404,7 +404,7 @@ describe('Syncing bookmarks', function () {
     const pageNthChild = 2
     const folderTitle = this.folder1Title
     const pageTitle = this.folder1Page2Title
-    const folder = `.bookmarkToolbarButton[title="${folderTitle}"]`
+    const folder = `[data-test-id="bookmarkToolbarButton"][title="${folderTitle}"]`
     yield Brave.app.client
       .waitForVisible(folder)
       .click(folder)
@@ -428,9 +428,18 @@ describe('Syncing bookmarks', function () {
     const folder1Title = this.folder1Title
 
     yield Brave.app.client
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(1) .bookmarkText', pageTitle)
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(2) .bookmarkText', updatedTitle)
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(3) .bookmarkText', folder1Title)
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(1) [data-test-id="bookmarkText"]')
+          .then((title) => title === pageTitle)
+      })
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(2) [data-test-id="bookmarkText"]')
+          .then((title) => title === updatedTitle)
+      })
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(3) [data-test-id="bookmarkText"]')
+          .then((title) => title === folder1Title)
+      })
   })
 })
 
@@ -533,7 +542,7 @@ describe('Syncing bookmarks from an existing profile', function () {
     const pageNthChild = 1
     const folderTitle = this.folder1Title
     const pageTitle = this.folder1Page1Title
-    const folder = `.bookmarkToolbarButton[title="${folderTitle}"]`
+    const folder = `[data-test-id="bookmarkToolbarButton"][title="${folderTitle}"]`
     yield Brave.app.client
       .waitForVisible(folder)
       .click(folder)
@@ -545,7 +554,7 @@ describe('Syncing bookmarks from an existing profile', function () {
     const pageNthChild = 2
     const folderTitle = this.folder1Title
     const pageTitle = this.folder1Page2Title
-    const folder = `.bookmarkToolbarButton[title="${folderTitle}"]`
+    const folder = `[data-test-id="bookmarkToolbarButton"][title="${folderTitle}"]`
     yield Brave.app.client
       .waitForVisible(folder)
       .click(folder)
@@ -559,9 +568,18 @@ describe('Syncing bookmarks from an existing profile', function () {
     const folder1Title = this.folder1Title
 
     yield Brave.app.client
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(1) .bookmarkText', pageTitle)
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(2) .bookmarkText', updatedTitle)
-      .waitForTextValue('.bookmarkToolbarButton:nth-child(3) .bookmarkText', folder1Title)
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(1) [data-test-id="bookmarkText"]')
+          .then((title) => title === pageTitle)
+      })
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(2) [data-test-id="bookmarkText"]')
+          .then((title) => title === updatedTitle)
+      })
+      .waitUntil(function () {
+        return this.getText('[data-test-id="bookmarkToolbarButton"]:nth-child(3) [data-test-id="bookmarkText"]')
+          .then((title) => title === folder1Title)
+      })
   })
 })