Merge pull request #12470 from brave/topsites-revisited

hidden top site fixes you wouldn't believe existed

app/browser/api/topSites.js

@@ -20,7 +20,12 @@ let minAccessOfTopSites
 const staticData = Immutable.fromJS(newTabData.topSites)
 
 const isPinned = (state, siteKey) => {
-  return aboutNewTabState.getPinnedTopSites(state).find(site => site.get('key') === siteKey)
+  return aboutNewTabState.getPinnedTopSites(state).some(site => {
+    if (!site || !site.get) {
+      return false
+    }
+    return site.get('key') === siteKey
+  })
 }
 
 const isIgnored = (state, siteKey) => {
@@ -116,6 +121,7 @@ const getTopSiteData = () => {
 
   if (sites.size < 18) {
     const preDefined = staticData
+      // TODO: this doesn't work properly
       .filter((site) => {
         return !isPinned(state, site.get('key')) && !isIgnored(state, site.get('key'))
       })
@@ -126,7 +132,27 @@ const getTopSiteData = () => {
     sites = sites.concat(preDefined)
   }
 
-  appActions.topSiteDataAvailable(sites)
+  let gridSites = aboutNewTabState.getPinnedTopSites(state).map(pinned => {
+    // do not allow duplicates
+    if (pinned) {
+      sites = sites.filter(site => site.get('key') !== pinned.get('key'))
+    }
+    // topsites are populated once user visit a new site.
+    // pinning a site to a given index is a user decision
+    // and should be taken as priority. If there's an empty
+    // space we just fill it with visited sites. Otherwise
+    // fallback to the pinned item.
+    if (!pinned) {
+      const firstSite = sites.first()
+      sites = sites.shift()
+      return firstSite
+    }
+    return pinned
+  })
+
+  gridSites = gridSites.filter(site => site != null)
+
+  appActions.topSiteDataAvailable(gridSites)
 }
 
 /**

app/common/state/aboutNewTabState.js

@@ -4,19 +4,27 @@
 
 const Immutable = require('immutable')
 const {makeImmutable} = require('./immutableUtil')
-
+const topSites = require('../../browser/api/topSites')
+const newTabData = require('../../../js/data/newTabData')
 /**
  * topSites are defined by users. Pinned sites are attached to their positions
  * in the grid, and the non pinned indexes are populated with newly accessed sites
  */
-
+const defaultPinnedSite = Immutable.fromJS(newTabData.pinnedTopSites)
 const aboutNewTabState = {
   getSites: (state) => {
     return state.getIn(['about', 'newtab', 'sites'])
   },
 
   getPinnedTopSites: (state) => {
-    return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List())
+    // add same number as fallback to avoid race condition on startup
+    const maxEntries = topSites.aboutNewTabMaxEntries || 100
+
+    // we need null spaces in order to proper pin a topSite in the right position.
+    // so let's set it to the same number as max new tab entries.
+    return state
+      .getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List())
+      .setSize(maxEntries)
   },
 
   getIgnoredTopSites: (state) => {
@@ -28,7 +36,11 @@ const aboutNewTabState = {
     if (!props) {
       return state
     }
-
+    // list is only empty if there's no pinning interaction.
+    // in this case we include the default pinned top sites list
+    if (state.getIn(['about', 'newtab', 'pinnedTopSites']).isEmpty()) {
+      state = state.setIn(['about', 'newtab', 'pinnedTopSites'], defaultPinnedSite)
+    }
     state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
     return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
   },

app/sessionStore.js

@@ -32,7 +32,6 @@ const windowState = require('./common/state/windowState')
 
 // Utils
 const locale = require('./locale')
-const {pinnedTopSites} = require('../js/data/newTabData')
 const {defaultSiteSettingsList} = require('../js/data/siteSettingsList')
 const filtering = require('./filtering')
 const autofill = require('./autofill')
@@ -1019,7 +1018,7 @@ module.exports.defaultAppState = () => {
         gridLayoutSize: 'small',
         sites: [],
         ignoredTopSites: [],
-        pinnedTopSites: pinnedTopSites
+        pinnedTopSites: []
       },
       welcome: {
         showOnLoad: !['test', 'development'].includes(process.env.NODE_ENV)

js/about/newtab.js

@@ -97,7 +97,7 @@ class NewTabPage extends React.Component {
   }
 
   get pinnedTopSites () {
-    return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List())
+    return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List()).setSize(100)
   }
 
   get ignoredTopSites () {
@@ -109,20 +109,18 @@ class NewTabPage extends React.Component {
   }
 
   isPinned (siteKey) {
-    return this.pinnedTopSites.find(site => site.get('key') === siteKey)
+    return this.pinnedTopSites.some(site => {
+      if (!site || !site.get) {
+        return false
+      }
+      return site.get('key') === siteKey
+    })
   }
 
   get gridLayout () {
     const sizeToCount = {large: 18, medium: 12, small: 6}
     const count = sizeToCount[this.gridLayoutSize]
-
-    let sites = this.pinnedTopSites.take(count)
-
-    if (sites.size < count) {
-      sites = sites.concat(this.topSites.take(count - sites.size))
-    }
-
-    return sites
+    return this.topSites.take(count)
   }
 
   showNotification () {
@@ -178,16 +176,26 @@ class NewTabPage extends React.Component {
   }
 
   onPinnedTopSite (siteKey) {
+    let sites = this.topSites
     let pinnedTopSites = this.pinnedTopSites
-    const siteProps = this.topSites.find(site => site.get('key') === siteKey)
 
-    if (this.isPinned(siteKey)) {
-      pinnedTopSites = pinnedTopSites.filter(site => site.get('key') !== siteKey)
+    const siteProps = sites.find(site => site.get('key') === siteKey)
+
+    const currentSiteIndex = sites.findIndex(site => site.get('key') === siteKey)
+    const currentPinnedSiteIndex = pinnedTopSites
+      .findIndex(site => site && site.get('key') === siteKey)
+
+    // ensure pinned sites are pinned in the right order when pinned
+    // if not pinned, pin and attach it to its position
+    if (!this.isPinned(siteKey)) {
+      pinnedTopSites = pinnedTopSites.splice(currentSiteIndex, 1, siteProps)
     } else {
-      pinnedTopSites = pinnedTopSites.push(siteProps)
+      pinnedTopSites = pinnedTopSites.splice(currentPinnedSiteIndex, 1, null)
+      sites = sites.splice(currentPinnedSiteIndex, 1, siteProps)
+      aboutActions.setNewTabDetail({sites}, true)
     }
 
-    aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites}, true)
+    aboutActions.setNewTabDetail({pinnedTopSites}, true)
   }
 
   onIgnoredTopSite (siteKey) {

test/unit/app/browser/api/topSitesTest.js

@@ -154,38 +154,28 @@ describe('topSites api', function () {
     ])
 
     it('respects position of pinned items when populating results', function () {
-      const allPinned = Immutable.fromJS([site1, site4])
-      const stateWithPinnedSites = defaultAppState
+      const allPinned = Immutable.fromJS([null, site2, null, site4])
+      let stateWithPinnedSites = defaultAppState
         .set(STATE_SITES.HISTORY_SITES, generateMap(site1, site2, site3, site4))
         .setIn(['about', 'newtab', 'pinnedTopSites'], allPinned)
       this.topSites.calculateTopSites(stateWithPinnedSites)
       // checks:
-      // - pinned item are in their expected order
+      // - pinned item are in their expected order (site 2 at i-1 and site4 at i-3)
       // - unpinned items fill the rest of the spots (starting w/ highest # visits first)
+      this.topSites.calculateTopSites(stateWithPinnedSites)
       getStateValue = stateWithPinnedSites
       this.clock.tick(calculateTopSitesClockTime)
       assert.equal(this.appActions.topSiteDataAvailable.callCount, 1)
       const newSitesData = this.appActions.topSiteDataAvailable.getCall(0).args[0]
-      const expectedSites = Immutable.fromJS([
-        {
-          location: 'https://example3.com/',
-          title: 'sample 3',
-          parentFolderId: 0,
-          count: 23,
-          lastAccessedTime: 123,
-          bookmarked: false,
-          key: 'https://example3.com/|0'
-        },
-        {
-          location: 'https://example2.com/',
-          title: 'sample 2',
-          parentFolderId: 0,
-          count: 5,
-          bookmarked: false,
-          key: 'https://example2.com/|0'
-        }
-      ])
-      assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS())
+
+      // assert that first site is populated
+      assert.deepEqual(newSitesData.get(0).isEmpty(), false)
+      // assert that site 2 is at i-1 as planned
+      assert.deepEqual(newSitesData.get(1), site2)
+      // assert that second site is populated
+      assert.deepEqual(newSitesData.get(2).isEmpty(), false)
+      // assert that site 4 is at i-3 as planned
+      assert.deepEqual(newSitesData.get(3), site4)
     })
 
     it('only includes one result for a domain (the one with the highest count)', function () {
@@ -294,7 +284,7 @@ describe('topSites api', function () {
         assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS())
       })
 
-      it('only returns the last `maxSites` results', function () {
+      it('only returns the last maxSites results', function () {
         const maxSites = this.topSites.aboutNewTabMaxEntries
         let tooManySites = Immutable.Map()
         for (let i = 0; i < maxSites + 1; i++) {

test/unit/app/browser/reducers/aboutNewTabsReducerTest.js

@@ -50,7 +50,7 @@ describe('aboutNewTabReducerTest', function () {
     const initialState = Immutable.fromJS({
       settings: { },
       about: {
-        newtab: { }
+        newtab: { pinnedTopSites: [] }
       }
     })
     it('gets a value from default settings when nothing is set', () => {