fix(systemaddon): Dedupe Topsites #2987
Conversation
There was a problem hiding this comment.
Code looks great, I'm a bit worried about a particular case though; if we end up with <12 items due to deduping, won't that result in a lot of extra querying because the check for if we have enough sites will return false (and always return false until the sites change). Can we do something to prevent that from happening?
system-addon/lib/TopSitesFeed.jsm
Outdated
| // Parse site url and extract hostname. | ||
| pinned.forEach(site => { site.hostname = shortURL(site); }); | ||
|
|
||
| return this.dedupe.collection(pinned).slice(0, TOP_SITES_SHOWMORE_LENGTH); |
There was a problem hiding this comment.
Would this always favour the pinned item? Or is there a possibility it would get removed?
There was a problem hiding this comment.
Currently no, but Dedupe can be called with a compare function that prefers pinned items.
system-addon/common/Dedupe.jsm
Outdated
| } | ||
|
|
||
| /** | ||
| * Removes duplicates in a list based on createKey. |
There was a problem hiding this comment.
This should probably say it picks the first item that it finds. So re: @k88hudson's question about pinned, it will remove the pinned if a non-pinned duplicate key item appeared first.
There was a problem hiding this comment.
Actually, nevermind. it depends on the compare.
|
@k88hudson If I'm reading this right we are already calling |
|
We could also, after getting this result, compare the length of the result with the length of |
piatra
force-pushed
the
bug-2933-dedupe-topsites
branch
from
14969df
to
5fad94b
Compare
system-addon/lib/TopSitesFeed.jsm
Outdated
| return site.hostname; | ||
| } | ||
| _dedupeCompare(storedValue, newValue) { | ||
| return newValue.isPinned; |
There was a problem hiding this comment.
Not sure if we want to somehow indicate if both are pinned and neither deduped?
There was a problem hiding this comment.
That's what I'm currently working on, realized just as I wanted to request review.
piatra
force-pushed
the
bug-2933-dedupe-topsites
branch
from
5fad94b
to
1defd7a
Compare
system-addon/common/Dedupe.jsm
Outdated
| const result = []; | ||
| groups.forEach(values => { | ||
| const valueMap = new Map(); | ||
| values.forEach(value => { |
There was a problem hiding this comment.
firefox style usually prefers for (const value of values) because there is slightly less overhead, but feel free to do that as a follow-up
system-addon/lib/TopSitesFeed.jsm
Outdated
| this.dedupe = new Dedupe(this._dedupeKey); | ||
| } | ||
| _dedupeKey(site) { | ||
| return site.hostname; |
There was a problem hiding this comment.
This is throwing an error and crashing the feed when the pinned group contains null items, so maybe check site && site.hostname and/or filter out null values from pinned before running dedupe
system-addon/lib/TopSitesFeed.jsm
Outdated
| return insertPinned([...frecent, ...DEFAULT_TOP_SITES], pinned).slice(0, TOP_SITES_SHOWMORE_LENGTH); | ||
| // Group together websites that require deduping. | ||
| const topsitesGroup = [pinned, frecent, DEFAULT_TOP_SITES]; | ||
| topsitesGroup.forEach(group => group.forEach(site => { |
There was a problem hiding this comment.
Maybe do this to filter out the null items and prevent accidental mutation?
for (const group in topsitesGroup) {
group
.filter(site => site)
.map(site => Object.assign({}, site, {hostName: shortURL(site)});
}
|
🔥 🔥 🔥 |
Fixes #2933
Dedupe happens in the Feeds. I've added the hostname key to the site objects because it is displayed in the cards. Previously the value returned by
shortURLwas named title (especially in the tests) it but made it confusing since websites already have a title.