Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't produce UPDATE when URL changes #1807

Merged
merged 3 commits into from
Dec 24, 2024
Merged

fix: Don't produce UPDATE when URL changes #1807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d8bcf7
fix(Default#getDiffs): Don't produce UPDATEs for tabs, ever
marcelklehr Dec 24, 2024
e3e6b93
test: update the server on local changes of url collisions
marcelklehr Dec 24, 2024
d244f6d
fix(SyncProcess): Fix URL collisions on NC Bookmarks
marcelklehr Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 66 additions & 12 deletions src/lib/strategies/Default.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { CancelledSyncError, FailsafeError } from '../../errors/Error'

import NextcloudBookmarksAdapter from '../adapters/NextcloudBookmarks'
import CachingAdapter from '../adapters/Caching'
import LocalTabs from '../LocalTabs'

const ACTION_CONCURRENCY = 12

Expand Down Expand Up @@ -437,8 +436,23 @@ export default class SyncProcess {
this.cacheTreeRoot,
this.localTreeRoot,
// We also allow canMergeWith for folders here, because Window IDs are not stable
(oldItem, newItem) =>
(oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)) || (oldItem.type === 'folder' && oldItem.canMergeWith(newItem)),
(oldItem, newItem) => {
if (oldItem.type !== newItem.type) {
return false
}
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
return oldItem.url === newItem.url
}
if (oldItem.type === 'folder') {
if (String(oldItem.id) === String(newItem.id)) {
return true
}
if (oldItem.canMergeWith(newItem)) {
return true
}
}
return false
},
this.preserveOrder,
)
serverScanner = new Scanner(
Expand All @@ -448,9 +462,21 @@ export default class SyncProcess {
// (for bookmarks, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is "<bookmarkID>;<folderId>")
// (for folders because Window IDs are not stable)
(oldItem, newItem) => {
if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.canMergeWith(newItem))) {
newMappings.push([oldItem, newItem])
return true
if (oldItem.type !== newItem.type) {
return false
}
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
return oldItem.url === newItem.url
}
if (oldItem.type === 'folder') {
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) {
newMappings.push([oldItem, newItem])
return true
}
if (oldItem.canMergeWith(newItem)) {
newMappings.push([oldItem, newItem])
return true
}
}
return false
},
Expand All @@ -461,18 +487,47 @@ export default class SyncProcess {
localScanner = new Scanner(
this.cacheTreeRoot,
this.localTreeRoot,
(oldItem, newItem) =>
(oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)),
(oldItem, newItem) => {
if (oldItem.type !== newItem.type) {
return false
}
if (oldItem.type === 'folder') {
if (String(oldItem.id) === String(newItem.id)) {
return true
}
}
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
if (String(oldItem.id) === String(newItem.id) && oldItem.url === newItem.url) {
return true
}
}
return false
},
this.preserveOrder
)
serverScanner = new Scanner(
this.cacheTreeRoot,
this.serverTreeRoot,
// We also allow canMergeWith here, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is "<bookmarkID>;<folderId>")
(oldItem, newItem) => {
if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.type === 'bookmark' && oldItem.canMergeWith(newItem))) {
newMappings.push([oldItem, newItem])
return true
if (oldItem.type !== newItem.type) {
return false
}
if (oldItem.type === 'folder') {
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) {
newMappings.push([oldItem, newItem])
return true
}
}
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem) && oldItem.url === newItem.url) {
newMappings.push([oldItem, newItem])
return true
}
if (oldItem.canMergeWith(newItem)) {
newMappings.push([oldItem, newItem])
return true
}
}
return false
},
Expand Down Expand Up @@ -856,7 +911,6 @@ export default class SyncProcess {
}

if (action.payload instanceof Folder && action.payload.children.length && action.oldItem instanceof Folder) {

// Fix for Unidirectional reverted REMOVEs, for all other strategies this should be a noop
action.payload.children.forEach((item) => {
item.parentId = id
Expand Down
63 changes: 62 additions & 1 deletion src/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,67 @@ describe('Floccus', function() {
false
)
})
it('should update the server on local changes of url collisions', async function() {
if (ACCOUNT_DATA.noCache) {
return this.skip()
}

const localRoot = account.getData().localRoot
const fooFolder = await browser.bookmarks.create({
title: 'foo',
parentId: localRoot
})
const barFolder = await browser.bookmarks.create({
title: 'bar',
parentId: fooFolder.id
})
const bookmark1 = await browser.bookmarks.create({
title: 'ur1l',
url: 'http://ur1.l/',
parentId: fooFolder.id
})
const bookmark2 = await browser.bookmarks.create({
title: 'url',
url: 'http://ur.l/',
parentId: barFolder.id
})
await account.sync() // propagate to server
expect(account.getData().error).to.not.be.ok

const newData = { url: 'http://ur.l/' }
await browser.bookmarks.update(bookmark1.id, newData)
await account.sync() // update on server
expect(account.getData().error).to.not.be.ok

const tree = await getAllBookmarks(account)
expectTreeEqual(
tree,
new Folder({
title: tree.title,
children: [
new Folder({
title: 'foo',
children: [
new Folder({
title: 'bar',
children: [
new Bookmark({
title: bookmark2.title,
url: bookmark2.url
})
]
}),
new Bookmark({
title: ACCOUNT_DATA.type === 'nextcloud-bookmarks' ? bookmark2.title : bookmark1.title,
url: newData.url
}),
]
})
]
}),
false
)
})
it('should update the server on local removals', async function() {
if (ACCOUNT_DATA.noCache) {
return this.skip()
Expand Down Expand Up @@ -5223,8 +5284,8 @@ describe('Floccus', function() {
new Folder({
title: 'Window 0',
children: [
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }),
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test2' }),
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }),
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test4' }),
]
})
Expand Down
Loading