refactor sync to use appStore change listener

and only trigger sync when important site fields are modified.
fix #8415
may also fix brave/sync#31

test plan:
0. automated sync tests should pass
1. enable sync and go to https://example.com
2. bookmark it. you should see a message in the console indicating a record was sent.
3. close the page and visit it again. you should not see a message in the console.
4. unbookmark it. you should see a message in the console indicating a record was sent.

app/browser/reducers/sitesReducer.js

@@ -13,6 +13,7 @@ const Immutable = require('immutable')
 const {makeImmutable} = require('../../common/state/immutableUtil')
 const settings = require('../../../js/constants/settings')
 const {getSetting} = require('../../../js/settings')
+const writeActions = require('../../../js/constants/sync/proto').actions
 
 const syncEnabled = () => {
   return getSetting(settings.SYNC_ENABLED) === true
@@ -21,17 +22,16 @@ const syncEnabled = () => {
 const sitesReducer = (state, action, emitChanges) => {
   switch (action.actionType) {
     case appConstants.APP_ADD_SITE:
-      const addSiteSyncCallback = action.skipSync ? undefined : syncActions.updateSite
       if (action.siteDetail.constructor === Immutable.List) {
         action.siteDetail.forEach((s) => {
-          state = state.set('sites', siteUtil.addSite(state.get('sites'), s, action.tag, undefined, addSiteSyncCallback))
+          state = state.set('sites', siteUtil.addSite(state.get('sites'), s, action.tag, undefined, action.skipSync))
         })
       } else {
         let sites = state.get('sites')
         if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {
           action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
         }
-        state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, addSiteSyncCallback))
+        state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync))
       }
       if (action.destinationDetail) {
         state = state.set('sites', siteUtil.moveSite(state.get('sites'),
@@ -51,11 +51,46 @@ const sitesReducer = (state, action, emitChanges) => {
     case appConstants.APP_MOVE_SITE:
       state = state.set('sites', siteUtil.moveSite(state.get('sites'),
         action.sourceDetail, action.destinationDetail, action.prepend,
-        action.destinationIsParent, false, syncActions.updateSite))
+        action.destinationIsParent, false))
       if (syncEnabled()) {
         state = syncUtil.updateSiteCache(state, action.destinationDetail)
       }
       break
+    case appConstants.APP_APPLY_SITE_RECORDS:
+      let nextFolderId = siteUtil.getNextFolderId(state.get('sites'))
+      // Ensure that all folders are assigned folderIds
+      action.records.forEach((record, i) => {
+        if (record.action !== writeActions.DELETE &&
+          record.bookmark && record.bookmark.isFolder &&
+          record.bookmark.site &&
+          typeof record.bookmark.site.folderId !== 'number') {
+          record.bookmark.site.folderId = nextFolderId
+          action.records.set(i, record)
+          nextFolderId = nextFolderId + 1
+        }
+      })
+      action.records.forEach((record) => {
+        const siteData = syncUtil.getSiteDataFromRecord(record, state, action.records)
+        const tag = siteData.tag
+        let siteDetail = siteData.siteDetail
+        const sites = state.get('sites')
+        switch (record.action) {
+          case writeActions.CREATE:
+            state = state.set('sites',
+              siteUtil.addSite(sites, siteDetail, tag, undefined, true))
+            break
+          case writeActions.UPDATE:
+            state = state.set('sites',
+              siteUtil.addSite(sites, siteDetail, tag, siteData.existingObjectData, true))
+            break
+          case writeActions.DELETE:
+            state = state.set('sites',
+              siteUtil.removeSite(sites, siteDetail, tag))
+            break
+        }
+        state = syncUtil.updateSiteCache(state, siteDetail)
+      })
+      break
     case appConstants.APP_TAB_PINNED:
       const tab = state.get('tabs').find((tab) => tab.get('tabId') === action.tabId)
       if (!tab) {

app/sync.js

@@ -26,7 +26,8 @@ const CATEGORY_MAP = syncUtil.CATEGORY_MAP
 const CATEGORY_NAMES = Object.keys(categories)
 const SYNC_ACTIONS = Object.values(syncConstants)
 
-let dispatcherCallback = null
+// The sync background script message sender
+let backgroundSender = null
 
 const log = (message) => {
   if (!config.debug) { return }
@@ -43,6 +44,65 @@ let pollIntervalId = null
 let deviceIdSent = false
 let bookmarksToolbarShown = false
 
+// Determines what to sync
+const appStoreChangeCallback = function (diffs) {
+  if (!backgroundSender) {
+    return
+  }
+
+  // Fields that should trigger a sync SEND when changed
+  const syncFields = {
+    sites: ['location', 'tags', 'customTitle', 'folderId', 'parentFolderId'],
+    siteSettings: Object.keys(syncUtil.siteSettingDefaults)
+  }
+  diffs.forEach((diff) => {
+    if (!diff || !diff.path) {
+      return
+    }
+    const path = diff.path.split('/')
+    if (path.length < 3) {
+      // We are looking for paths like ['', 'sites', 'https://brave.com/', 'title']
+      return
+    }
+    const type = path[1]
+    const fieldsToPick = syncFields[type]
+    if (!fieldsToPick) {
+      return
+    }
+
+    const isInsert = diff.op === 'add' && path.length === 3
+    const isUpdate = fieldsToPick.includes(path[3]) // Ignore insignicant updates
+
+    // DELETES are handled in appState because the old object is no longer
+    // available by the time emitChanges is received
+    if (isInsert || isUpdate) {
+      // Get the item's path and entry in appStore
+      const statePath = path.slice(1, 3).map((item) => item.replace(/~1/g, '/'))
+      const entry = AppStore.getState().getIn(statePath)
+      if (!entry || !entry.toJS) {
+        return
+      }
+
+      let action = null
+
+      if (isInsert && !entry.get('skipSync')) {
+        action = writeActions.CREATE
+      } else if (isUpdate) {
+        action = writeActions.UPDATE
+      }
+
+      if (action !== null) {
+        // Set the object ID if there is not already one
+        const entryJS = entry.toJS()
+        entryJS.objectId = entryJS.objectId || syncUtil.newObjectId(statePath)
+
+        sendSyncRecords(backgroundSender, action,
+          [type === 'sites' ? syncUtil.createSiteData(entryJS) : syncUtil.createSiteSettingsData(statePath[1], entryJS)])
+      }
+    }
+  })
+}
+
 /**
  * Sends sync records of the same category to the sync server.
  * @param {event.sender} sender
@@ -81,8 +141,7 @@ const sendSyncRecords = (sender, action, data) => {
 const validateAction = (action) => {
   const SYNC_ACTIONS_WITHOUT_ITEMS = [
     syncConstants.SYNC_CLEAR_HISTORY,
-    syncConstants.SYNC_CLEAR_SITE_SETTINGS,
-    syncConstants.SYNC_DELETE_USER
+    syncConstants.SYNC_CLEAR_SITE_SETTINGS
   ]
   if (SYNC_ACTIONS.includes(action.actionType) !== true) {
     return false
@@ -103,53 +162,30 @@ const validateAction = (action) => {
   return true
 }
 
-const doAction = (sender, action) => {
+const dispatcherCallback = (action) => {
+  if (!backgroundSender) {
+    return
+  }
+
   if (action.key === settings.SYNC_ENABLED) {
     if (action.value === false) {
       module.exports.stop()
     }
   }
   // If sync is not enabled, the following actions should be ignored.
-  if (!syncEnabled() || validateAction(action) !== true || sender.isDestroyed()) {
+  if (!syncEnabled() || validateAction(action) !== true || backgroundSender.isDestroyed()) {
     return
   }
   switch (action.actionType) {
-    case syncConstants.SYNC_ADD_SITE:
-      sendSyncRecords(sender, writeActions.CREATE,
-        [syncUtil.createSiteData(action.item.toJS())])
-      break
-    case syncConstants.SYNC_UPDATE_SITE:
-      sendSyncRecords(sender, writeActions.UPDATE,
-        [syncUtil.createSiteData(action.item.toJS())])
-      break
     case syncConstants.SYNC_REMOVE_SITE:
-      sendSyncRecords(sender, writeActions.DELETE,
+      sendSyncRecords(backgroundSender, writeActions.DELETE,
         [syncUtil.createSiteData(action.item.toJS())])
       break
     case syncConstants.SYNC_CLEAR_HISTORY:
-      sender.send(messages.DELETE_SYNC_CATEGORY, CATEGORY_MAP.historySite.categoryName)
-      break
-    case syncConstants.SYNC_ADD_SITE_SETTING:
-      if (syncUtil.isSyncable('siteSetting', action.item)) {
-        sendSyncRecords(sender, writeActions.CREATE,
-          [syncUtil.createSiteSettingsData(action.hostPattern, action.item.toJS())])
-      }
-      break
-    case syncConstants.SYNC_UPDATE_SITE_SETTING:
-      if (syncUtil.isSyncable('siteSetting', action.item)) {
-        sendSyncRecords(sender, writeActions.UPDATE,
-          [syncUtil.createSiteSettingsData(action.hostPattern, action.item.toJS())])
-      }
-      break
-    case syncConstants.SYNC_REMOVE_SITE_SETTING:
-      sendSyncRecords(sender, writeActions.DELETE,
-        [syncUtil.createSiteSettingsData(action.hostPattern, action.item.toJS())])
+      backgroundSender.send(messages.DELETE_SYNC_CATEGORY, CATEGORY_MAP.historySite.categoryName)
       break
     case syncConstants.SYNC_CLEAR_SITE_SETTINGS:
-      sender.send(messages.DELETE_SYNC_SITE_SETTINGS)
-      break
-    case syncConstants.SYNC_DELETE_USER:
-      sender.send(messages.DELETE_SYNC_USER)
+      backgroundSender.send(messages.DELETE_SYNC_SITE_SETTINGS)
       break
     default:
   }
@@ -165,6 +201,8 @@ module.exports.onSyncReady = (isFirstRun, e) => {
   if (!syncEnabled()) {
     return
   }
+  AppStore.addChangeListener(appStoreChangeCallback)
+
   if (!deviceIdSent && isFirstRun) {
     // Sync the device id for this device
     sendSyncRecords(e.sender, writeActions.CREATE, [{
@@ -283,9 +321,9 @@ module.exports.init = function (appState) {
   })
   // sent by about:preferences when resetting sync
   ipcMain.on(RESET_SYNC, (e) => {
-    if (dispatcherCallback) {
+    if (backgroundSender) {
       // send DELETE_SYNC_USER to sync client. it replies with DELETED_SYNC_USER
-      dispatcherCallback({actionType: syncConstants.SYNC_DELETE_USER})
+      backgroundSender.send(messages.DELETE_SYNC_USER)
     } else {
       reset()
     }
@@ -295,14 +333,15 @@ module.exports.init = function (appState) {
   })
   // GET_INIT_DATA is the first message sent by the sync-client when it starts
   ipcMain.on(messages.GET_INIT_DATA, (e) => {
+    // Set the message sender
+    backgroundSender = e.sender
     // Clear any old errors
     appActions.setSyncSetupError(null)
     // Unregister the previous dispatcher cb
     if (dispatcherCallback) {
       appDispatcher.unregister(dispatcherCallback)
     }
     // Register the dispatcher callback now that we have a valid sender
-    dispatcherCallback = doAction.bind(null, e.sender)
     appDispatcher.register(dispatcherCallback)
     // Send the initial data
     if (syncEnabled()) {
@@ -413,4 +452,5 @@ module.exports.stop = function () {
     clearInterval(pollIntervalId)
   }
   appActions.setSyncSetupError(null)
+  AppStore.removeChangeListener(appStoreChangeCallback)
 }

docs/state.md

@@ -233,6 +233,7 @@ AppStore
       originalSeed: Array.<number>, // only set for bookmarks that have been synced before a sync profile reset
       parentFolderId: number, // set for bookmarks and bookmark folders only
       partitionNumber: number, // optionally specifies a specific session
+      skipSync: boolean, // Set for objects FETCHed by sync
       tags: [string], // empty, 'bookmark', 'bookmark-folder', 'default', or 'reader'
       themeColor: string, // CSS compatible color string
       title: string
@@ -259,6 +260,7 @@ AppStore
       openExternalPermission: boolean,
       pointerLockPermission: boolean,
       protocolRegistrationPermission: boolean,
+      skipSync: boolean, // Set for objects FETCHed by sync
       runInsecureContent: boolean, // allow active mixed content
       safeBrowsing: boolean,
       savePasswords: boolean, // only false or undefined/null

js/actions/syncActions.js

@@ -7,20 +7,6 @@ const AppDispatcher = require('../dispatcher/appDispatcher')
 const syncConstants = require('../constants/syncConstants')
 
 const syncActions = {
-  addSite: function (item) {
-    AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_ADD_SITE,
-      item
-    })
-  },
-
-  updateSite: function (item) {
-    AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_UPDATE_SITE,
-      item
-    })
-  },
-
   removeSite: function (item) {
     AppDispatcher.dispatch({
       actionType: syncConstants.SYNC_REMOVE_SITE,
@@ -38,30 +24,6 @@ const syncActions = {
     AppDispatcher.dispatch({
       actionType: syncConstants.SYNC_CLEAR_SITE_SETTINGS
     })
-  },
-
-  addSiteSetting: function (hostPattern, item) {
-    AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_ADD_SITE_SETTING,
-      item,
-      hostPattern
-    })
-  },
-
-  updateSiteSetting: function (hostPattern, item) {
-    AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_UPDATE_SITE_SETTING,
-      item,
-      hostPattern
-    })
-  },
-
-  removeSiteSetting: function (hostPattern, item) {
-    AppDispatcher.dispatch({
-      actionType: syncConstants.SYNC_REMOVE_SITE_SETTING,
-      item,
-      hostPattern
-    })
   }
 }
 

js/constants/syncConstants.js

@@ -6,15 +6,9 @@ const mapValuesByKeys = require('../lib/functional').mapValuesByKeys
 
 const _ = null
 const syncConstants = {
-  SYNC_ADD_SITE: _, /** @param {Immutable.Map} item */
-  SYNC_UPDATE_SITE: _, /** @param {Immutable.Map} item */
   SYNC_REMOVE_SITE: _,  /** @param {Immutable.Map} item */
   SYNC_CLEAR_HISTORY: _,
-  SYNC_CLEAR_SITE_SETTINGS: _,
-  SYNC_DELETE_USER: _,
-  SYNC_ADD_SITE_SETTING: _, /** @param {string} hostPattern, @param {Immutable.Map} item */
-  SYNC_UPDATE_SITE_SETTING: _, /** @param {string} hostPattern, @param {Immutable.Map} item */
-  SYNC_REMOVE_SITE_SETTING: _  /** @param {string} hostPattern, @param {Immutable.Map} item */
+  SYNC_CLEAR_SITE_SETTINGS: _
 }
 
 module.exports = mapValuesByKeys(syncConstants)