Merge pull request #5219 from brave/pr5173_ca-9008_1.8.x

NTP: Prevent top sites interface from handling duplicated site entries (uplift to 1.8.x)

components/brave_new_tab_ui/helpers/newTabUtils.ts

@@ -135,3 +135,29 @@ export function filterFromExcludedSites (
         .every((removedSite: NewTab.Site) => removedSite.url !== site.url)
     })
 }
+
+export function equalsSite (
+  site: NewTab.Site,
+  siteToCompare: NewTab.Site
+): boolean {
+  return (
+    // Ensure there are no duplicated URLs
+    site.url === siteToCompare.url ||
+    // Ensure there are no duplicated IDs
+    site.id === siteToCompare.id
+  )
+}
+
+export function filterDuplicatedSitesbyIndexOrUrl (
+  sitesData: NewTab.Site[]
+): NewTab.Site[] {
+  const newGridSites: NewTab.Site[] = []
+
+  for (const site of sitesData) {
+    const isDuplicate = newGridSites.some(item => equalsSite(item, site))
+    if (!isDuplicate) {
+      newGridSites.push(site)
+    }
+  }
+  return newGridSites
+}

components/brave_new_tab_ui/state/gridSitesState.ts

@@ -10,7 +10,8 @@ import {
   getGridSitesWhitelist,
   isGridSitePinned,
   isGridSiteBookmarked,
-  filterFromExcludedSites
+  filterFromExcludedSites,
+  filterDuplicatedSitesbyIndexOrUrl
 } from '../helpers/newTabUtils'
 
 export function gridSitesReducerSetFirstRenderDataFromLegacy (
@@ -239,7 +240,15 @@ export function gridSitesReducerAddSiteOrSites (
     ...sitesToAdd,
     ...state.gridSites
   ]
-  state = gridSitesReducerDataUpdated(state, currentGridSitesWithNewItems)
+
+  // We have reports of users having duplicated entries
+  // after updating to the new storage. This is a special
+  // case that breaks all top sites mechanism. Ensure all
+  // entries defined visually in the grid are not duplicated.
+  const updatedGridSites =
+    filterDuplicatedSitesbyIndexOrUrl(currentGridSitesWithNewItems)
+
+  state = gridSitesReducerDataUpdated(state, updatedGridSites)
   return state
 }
 

components/test/brave_new_tab_ui/helpers/newTabUtils_test.ts

@@ -276,4 +276,34 @@ describe('new tab util files tests', () => {
       expect(assertion).toHaveLength(1)
     })
   })
+  describe('filterDuplicatedSitesbyIndexOrUrl', () => {
+    const sitesData: NewTab.Site[] = [{
+      id: 'topsite-000',
+      url: 'https://brave.com',
+      title: 'brave',
+      favicon: '',
+      letter: '',
+      pinnedIndex: undefined,
+      bookmarkInfo: undefined
+    }]
+    it('filter sites already included in the sites list', () => {
+      const duplicatedSitesData = [ ...sitesData, ...sitesData ]
+      const assertion = newTabUtils.filterDuplicatedSitesbyIndexOrUrl(duplicatedSitesData)
+      expect(assertion).toHaveLength(1)
+    })
+    it('does not filter sites not included in the sites list', () => {
+      const otherSitesData: NewTab.Site[] = [{
+        id: 'topsite-111',
+        url: 'https://new_site.com',
+        title: 'new_site',
+        favicon: '',
+        letter: '',
+        pinnedIndex: undefined,
+        bookmarkInfo: undefined
+      }]
+      const newSitesData: NewTab.Site[] = [ ...sitesData, ...otherSitesData ]
+      const assertion = newTabUtils.filterDuplicatedSitesbyIndexOrUrl(newSitesData)
+      expect(assertion).toHaveLength(2)
+    })
+  })
 })

components/test/brave_new_tab_ui/reducers/grid_sites_reducer_test.ts

@@ -10,7 +10,7 @@ import * as gridSitesState from '../../../brave_new_tab_ui/state/gridSitesState'
 
 const bookmarkInfo: chrome.bookmarks.BookmarkTreeNode = {
   dateAdded: 123123,
-  id: '',
+  id: 'topsite-0',
   index: 1337,
   parentId: '',
   title: 'brave',
@@ -80,6 +80,53 @@ describe('gridSitesReducer', () => {
 
       expect(assertion.gridSites).toHaveLength(2)
     })
+    it('populate state.gridSites list without duplicates', () => {
+      const brandNewSite: NewTab.Site = {
+        id: 'topsite-000',
+        url: 'https://.com',
+        title: 'g. clooney free propaganda',
+        pinnedIndex: 0,
+        favicon: '',
+        letter: '',
+        bookmarkInfo: undefined
+      }
+      const veryRepetitiveSite: NewTab.Site = {
+        id: 'topsite-111',
+        url: 'https://serg-loves-pokemon-4ever.com',
+        title: 'pokemon fan page',
+        pinnedIndex: 0,
+        favicon: '',
+        letter: '',
+        bookmarkInfo: undefined
+      }
+
+      const veryRepetitiveSiteList: NewTab.Site[] = [
+        veryRepetitiveSite,
+        veryRepetitiveSite,
+        veryRepetitiveSite,
+        veryRepetitiveSite,
+        veryRepetitiveSite
+      ]
+
+      // Add repetitiveSiteList everywhere. Dupe party.
+      const veryRepetitiveInitialGridSitesState: NewTab.GridSitesState = {
+        gridSites: veryRepetitiveSiteList,
+        removedSites: veryRepetitiveSiteList,
+        shouldShowSiteRemovedNotification: false,
+        legacy: {
+          pinnedTopSites: veryRepetitiveSiteList,
+          ignoredTopSites: veryRepetitiveSiteList
+        }
+      }
+
+      const assertion = gridSitesReducer(veryRepetitiveInitialGridSitesState, {
+        type: types.GRID_SITES_SET_FIRST_RENDER_DATA,
+        payload: { topSites: [ brandNewSite ] }
+      })
+      // We should see just one repetitive site and the new
+      // one added on the payload above
+      expect(assertion.gridSites).toHaveLength(2)
+    })
   })
   describe('GRID_SITES_DATA_UPDATED', () => {
     let gridSitesReducerDataUpdatedStub: jest.SpyInstance
@@ -224,7 +271,8 @@ describe('gridSitesReducer', () => {
     it('push an item from state.removedSites list back to state.gridSites list', () => {
       const removedSite: NewTab.Site = {
         ...gridSites[1],
-        url: 'https://example.com'
+        url: 'https://example.com',
+        id: 'topsite-999999'
       }
       const newStateWithGridSites: NewTab.State = {
         ...storage.initialGridSitesState,
@@ -277,10 +325,12 @@ describe('gridSitesReducer', () => {
     it('push all items from state.removedSites list back to state.gridSites list', () => {
       const removedSites: NewTab.Site[] = [{
         ...gridSites[0],
-        url: 'https://example.com'
+        url: 'https://example.com',
+        id: 'topsite-999999'
       }, {
         ...gridSites[1],
-        url: 'https://another-example.com'
+        url: 'https://another-example.com',
+        id: 'topsite-999998'
       }]
       const newStateWithGridSites: NewTab.State = {
         ...storage.initialGridSitesState,
@@ -440,7 +490,8 @@ describe('gridSitesReducer', () => {
     it('add sites to state.gridSites list', () => {
       const newSite: NewTab.Site = {
         ...gridSites[0],
-        url: 'https://example.com'
+        url: 'https://example.com',
+        id: 'topsite-99999'
       }
       const newStateWithGridSites: NewTab.State = {
         ...storage.initialGridSitesState,

components/test/brave_new_tab_ui/state/gridSitesState_test.ts

@@ -200,7 +200,7 @@ describe('gridSitesState', () => {
   })
   describe('gridSitesReducerUndoRemoveSite', () => {
     it('push an item from the state.removedSites list back to state.gridSites list', () => {
-      const removedSite: NewTab.Site = { ...gridSites[1], url: 'https://example.com' }
+      const removedSite: NewTab.Site = { ...gridSites[1], id: 'topsite-000', url: 'https://example.com' }
       const newState: NewTab.State = {
         ...storage.initialGridSitesState,
         gridSites,
@@ -230,10 +230,12 @@ describe('gridSitesState', () => {
     it('push all items from state.removedSites list back to state.gridSites list', () => {
       const removedSites: NewTab.Site[] = [{
         ...gridSites[0],
-        url: 'https://example.com'
+        url: 'https://example.com',
+        id: 'topsite-000'
       }, {
         ...gridSites[1],
-        url: 'https://another-example.com'
+        url: 'https://another-example.com',
+        id: 'topsite-111'
       }]
 
       const newState: NewTab.State = {
@@ -300,14 +302,31 @@ describe('gridSitesState', () => {
   })
   describe('gridSitesReducerAddSiteOrSites', () => {
     it('add sites to state.gridSites list', () => {
-      const newSite: NewTab.Site = { ...gridSites[0], url: 'https://example.com' }
+      const newSite: NewTab.Site = { ...gridSites[0], id: 'topsite-111', url: 'https://example.com' }
       const newState: NewTab.State = { ...storage.initialGridSitesState, gridSites }
 
       const assertion = gridSitesState
         .gridSitesReducerAddSiteOrSites(newState, newSite)
 
       expect(assertion.gridSites).toHaveLength(3)
     })
+    it('do not allow duplicated sites to be added in state.gridSites list', () => {
+      const duplicatedGridSites: NewTab.Site[] = [
+        ...gridSites,
+        ...gridSites
+      ]
+
+      expect(duplicatedGridSites).toHaveLength(4)
+
+      const assertion = gridSitesState
+      .gridSitesReducerAddSiteOrSites(
+        storage.initialGridSitesState,
+        duplicatedGridSites
+        )
+
+      // Array length should reduce since it should filter dupes
+      expect(assertion.gridSites).toHaveLength(2)
+    })
   })
   describe('gridSitesReducerShowSiteRemovedNotification', () => {
     it('update state with the specified payload value', () => {