NTP: Prevent top sites interface from handling duplicated site entries #5173

cezaraugusto · 2020-04-07T14:43:22Z

Submitter Checklist:

Test Plan:

Covered by automated tests npm run test-unit

(Manual) Test Plan

Check you have an existing profile with some top site. If you do not, navigate some websites until
typing chrome.topSites.get(site => console.log(site)) in your NTP console outputs more than one result (this bug only happens in existing profiles w/ top sites).
Copy/paste the code below in devtools on your NTP console tab. The code is corrupted storage with two top sites repeated and pinned at indexes 1 and 2, respectively.
Refresh the page
You should see Google Translate and GitHub as top sites, pinned, at indexes 1 and 2, respectively. If you had any top site before this check, they should be there too, but pinned indexes should be respected.

👉👉👉👉👉👉👉Code you need to copy 👈👈👈👈👈👈👈

localStorage.setItem('grid-sites-data-v1', JSON.stringify({
  gridSites: [
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    },
    {
      title: "Google Translate",
      url: "https://translate.google.com/",
      id: "topsite-3742-1585908285880",
      letter: "G",
      favicon: "chrome://favicon/size/64@1x/https://translate.google.com/",
      pinnedIndex: 1
    },
    {
      title: "GitHub",
      url: "https://github.com/congard",
      id: "topsite-3742-1585908285881",
      letter: "C",
      favicon: "chrome://favicon/size/64@1x/https://github.com/congard",
      bookmarkInfo: {},
      pinnedIndex: 2
    }
  ],
  removedSites: [],
  shouldShowSiteRemovedNotification: false,
  legacy: {
    pinnedTopSites: [],
    ignoredTopSites: []
  }
}
))

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

Fix brave/brave-browser#9008

rebron · 2020-04-07T20:05:10Z

I had a broken top sites profile and tested the macOS version of PR build is working properly now for my top sites, no duplicate top sites and no overflow of top sites into a second or third row. Removing top sites and undoing is also working.

simonhong

LGTM

simonhong · 2020-04-08T02:43:12Z

components/brave_new_tab_ui/helpers/newTabUtils.ts

@@ -115,3 +115,29 @@ export function filterFromExcludedSites (
        .every((removedSite: NewTab.Site) => removedSite.url !== site.url)
    })
 }
+
+export function equalsSite (


nit: Should this method has export because it is only used in this file?

bsclifton · 2020-04-10T16:02:42Z

~~@cezaraugusto can you please create an uplift to 1.8 as #5097 was merged to 1.8? thanks~~ 😄 👍 Great job on the tests here ❤️

Just created uplift with #5219 😄

srirambv · 2020-04-22T11:39:33Z

@cezaraugusto ran throught the PR on

Brave	1.9.37 Chromium: 81.0.4044.113 (Official Build) nightly (64-bit)
Revision	cf9d66371ea608e227eed56ccba3abc2701bd23d-refs/branch-heads/4044@{#936}
OS	Windows 10 OS Version 1909 (Build 18363.778)

On existing profile with top tiles running chrome.topSites.get(site => console.log(site)) just showed 1 entry as per below screenshot

Running the corrupt local storage code and then reloading retains the existing top tile but expected pinned tiles are not in position 1 and 2 respectively

Verification PASSED on macOS 10.15.4 x64 using the following build:

Brave | 1.10.11 Chromium: 81.0.4044.122 (Official Build) nightly (64-bit)
-- | --
Revision | 44f4233f08910d83b146130c1938256a2e05b136-refs/branch-heads/4044@{#963}
OS | macOS Version 10.15.4 (Build 19E287)

Downloaded 1.9.15 CR: 81.0.4044.92 and populated the NTP tiles as per the following:

Upgraded to 1.10.11 CR: 81.0.4044.122 and ensured no data loss occurred in terms of websites being removed from the NTP grid. Used the STR from #5173 (comment) and imported the data. Ensured that the pinned tabs were in the correct position:

srirambv · 2020-04-27T17:01:12Z

Rechecked again on 1.8.24 ->1.10.11
Before upgrade

After upgrade

NTP: Prevent top sites interface from handling duplicated site entries

33bed65

Fix brave/brave-browser#9008

cezaraugusto requested review from bsclifton, rebron and simonhong April 7, 2020 14:43

cezaraugusto self-assigned this Apr 7, 2020

simonhong approved these changes Apr 8, 2020

View reviewed changes

bsclifton added this to the 1.9.x - Nightly milestone Apr 8, 2020

cezaraugusto merged commit d601f8a into master Apr 10, 2020

cezaraugusto deleted the ca-9008 branch April 10, 2020 15:26

brave-builds pushed a commit that referenced this pull request Apr 10, 2020

Uplift of #5173 (squashed) to beta

3154641

brave-builds mentioned this pull request Apr 10, 2020

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

Merged

4 tasks

bsclifton pushed a commit that referenced this pull request Apr 21, 2020

Uplift of #5173 (squashed) to beta

7975d35

GeetaSarvadnya mentioned this pull request Apr 28, 2020

Duplicated top site entries after update brave/brave-browser#9008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NTP: Prevent top sites interface from handling duplicated site entries #5173

NTP: Prevent top sites interface from handling duplicated site entries #5173

cezaraugusto commented Apr 7, 2020

rebron commented Apr 7, 2020

simonhong left a comment

simonhong Apr 8, 2020

bsclifton commented Apr 10, 2020 •

edited

Loading

srirambv commented Apr 22, 2020 •

edited by kjozwiak

Loading

srirambv commented Apr 27, 2020

NTP: Prevent top sites interface from handling duplicated site entries #5173

NTP: Prevent top sites interface from handling duplicated site entries #5173

Conversation

cezaraugusto commented Apr 7, 2020

Submitter Checklist:

Test Plan:

(Manual) Test Plan

Reviewer Checklist:

After-merge Checklist:

rebron commented Apr 7, 2020

simonhong left a comment

Choose a reason for hiding this comment

simonhong Apr 8, 2020

Choose a reason for hiding this comment

bsclifton commented Apr 10, 2020 • edited Loading

srirambv commented Apr 22, 2020 • edited by kjozwiak Loading

srirambv commented Apr 27, 2020

bsclifton commented Apr 10, 2020 •

edited

Loading

srirambv commented Apr 22, 2020 •

edited by kjozwiak

Loading