Change sites from List to Map #5531
Conversation
fix #4879 Auditors: @bbondy, @bsclifton, @cezaraugusto Test Plan: Performance: 1. Import bulk bookmarks (4000+) from other browsers (Don't merge into toolbar) 2. The process should finish instantly 3. Delete Import from XXX folder 4. The process should finish instantly Migration: 1. Make sure session-store-1 contains the sites data before 0.12.10 2. Lauch Brave and Close 3. sites of session-store-1 should change from [] tp {}
darkdh
force-pushed
the
sites-refactor
branch
from
b5349f6
to
20034b2
Compare
| }) | ||
| data.sites = sites | ||
| } | ||
|
|
There was a problem hiding this comment.
Great job with the above 😄 It's super important to have migrations for the session data
| @@ -34,7 +34,8 @@ AppStore | |||
| } | |||
| } | |||
| }, | |||
| sites: [{ | |||
| sites: { | |||
| [siteKey]: { // Calculated by siteUtil.getSiteKey() | |||
There was a problem hiding this comment.
could you just put the format of the key? like folder specifies folderId, URLs use location/partition/folderId?
| } | ||
| if (isBookmarkFolder(tags)) { | ||
| return sites.findIndex((site) => isBookmarkFolder(site.get('tags')) && site.get('folderId') === siteDetail.get('folderId')) | ||
| const folderId = siteDetail.get('folderId') |
There was a problem hiding this comment.
with this refactor, the tags field really isn't needed (which is nice). Because you can determine if it's a folder or not by the folderId. It might clean things up to remove it
| }) | ||
|
|
||
| if (oldSiteDetail && oldSiteDetail.get('order') !== undefined) { | ||
| site = site.set('order', oldSiteDetail.get('order')) |
There was a problem hiding this comment.
At some point (doesn't have to be now, something for all of us to keep in mind 😄), I think it might be worth creating util methods to help with type conversion / default values. Something like this:
const safeNumber = (value, default) => {
const num = Number(value)
return isNaN(num) ? default : num
}
| let destinationSite = sites.get(destinationKey) | ||
| sites = sites.delete(sourceKey) | ||
| sites = sites.map((site) => { | ||
| const siteOrder = site.get('order') |
There was a problem hiding this comment.
Would it make sense to pull this re-ordering out to a common method?
| if (!action.siteDetail.get('folderId')) { | ||
| action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites)) | ||
| } | ||
| appState = appState.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag)) | ||
| } |
There was a problem hiding this comment.
Much cleaner + easier to read 😄
|
Looks good! 👍 I left some comments with feedback. This is a great step in making the code better 😄 |
|
Thanks for the review effort @bsclifton. It has been addressed in d8d2b66 |
- Updated tests to be using/expecting a map (not a list) - Updated siteUtil to return empty map where appropriate (instead of empty array) - Removed unused method in siteUtil - Extra error handling in siteUtil.updateSiteFavicon - Added back some of the missing tests for updateSiteFavicon - Update newtab > getUnpinned() to use slice (since it's a map) Auditors: @darkdh, @cezaraugusto
|
@darkdh only issue I could found was fixed by @bsclifton (accessed sites breaking on refresh) @bsclifton edits fixed the above but made a regression where topSites are no longer collecting favicons instantly. Otherwise LGTM. Awesome team work! |
|
Also I noticed the navigation bar star icon no longer works after this. It seems to create 3 site entries for a single website. And only the third has a bookmark tag. Sites should always be unique. Since this can cause some session store corruption I'm going to revert this. |
git rebase -ito squash commits (if needed).Test Plan:
fix #4879
Auditors: @bbondy, @bsclifton, @cezaraugusto
Test Plan:
Performance:
Migration: