Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Correctly converts key from pre-split sites to post-split pinned sites
Browse files Browse the repository at this point in the history
Fix #12580
Prevents duplicated pinned sites
Migration was attempting to insert pinned site with key format [location]|[partition]|0 when expected format is [location]|[partition]
Also ensures pinned site order stays consistent across migrations, windows, and app starts
  • Loading branch information
petemill committed Jan 10, 2018
1 parent 82ff43b commit 3c11c38
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 3 deletions.
3 changes: 2 additions & 1 deletion app/browser/windows.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ const updatePinnedTabs = (win, appState) => {
// sites are instructions of what should be pinned
// tabs are sites our window already has pinned
// for each site which should be pinned, find if it's already pinned
for (const site of statePinnedSites.values()) {
const statePinnedSitesOrdered = statePinnedSites.sort((a, b) => a.get('order') - b.get('order'))
for (const site of statePinnedSitesOrdered.values()) {
const existingPinnedTabIdx = pinnedWindowTabs.findIndex(tab => siteMatchesTab(site, tab))
if (existingPinnedTabIdx !== -1) {
// if it's already pinned we don't need to consider the tab in further searches
Expand Down
4 changes: 4 additions & 0 deletions app/common/state/pinnedSitesState.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ const pinnedSiteState = {
return state
}

if (state.hasIn([STATE_SITES.PINNED_SITES, key])) {
return state
}

state = state.setIn([STATE_SITES.PINNED_SITES, key], site)
return state
},
Expand Down
4 changes: 3 additions & 1 deletion app/sessionStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,9 @@ module.exports.runPreMigrations = (data) => {
const site = data.sites[key]
if (site.tags && site.tags.includes('pinned')) {
delete site.tags
data.pinnedSites[key] = site
// matches `getKey` from pinnedSitesUtil
const pinnedSiteKey = `${site.location}|${site.partitionNumber}`
data.pinnedSites[pinnedSiteKey] = site
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/unit/app/sessionStoreTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,7 @@ describe('sessionStore unit tests', function () {

before(function () {
oldValue = data.getIn(['sites', 'https://pinned-tab.com|0|0'])
newValue = runPreMigrations.pinnedSites['https://pinned-tab.com|0|0']
newValue = runPreMigrations.pinnedSites['https://pinned-tab.com|0']
})

it('copies lastAccessedTime', function () {
Expand Down

0 comments on commit 3c11c38

Please sign in to comment.