Skip to content

Commit 8cef7fb

Browse files
committed
fix: pull-only sync should not push back to cloud — prevents ordering revert
The receiving browser (e.g. Chromium) was pushing bookmarks back to cloud after pulling updates from the source browser (e.g. Firefox), overwriting the source browser's ordering changes. This caused a revert cycle: 1. Firefox reorders → syncs to cloud ✅ 2. Chromium syncs → pulls new order → but also pushes back with old indices 3. Firefox syncs → pulls Chromium's overwritten order → reverts Root cause: bookmarksToUpdate (cloud→local updates) was incorrectly counted as 'local changes to push'. And Step 9 always pushed regardless of whether there were actual local changes. Fix: - Remove bookmarksToUpdate from hasLocalChangesToPush (they're cloud→local) - Gate Step 9 push behind hasLocalChangesToPush check - Store cloud checksum on pull-only syncs for future comparison v0.8.19
1 parent 5219316 commit 8cef7fb

File tree

5 files changed

+82
-14
lines changed

5 files changed

+82
-14
lines changed

apps/extension/__tests__/cross-browser-sync.test.js

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,3 +613,59 @@ describe('Bookmark Update Detection', () => {
613613
expect(bookmarkNeedsUpdate(cloud, local)).toBe(false);
614614
});
615615
});
616+
617+
describe('Pull-only sync should not push back to cloud', () => {
618+
it('should not treat cloud→local updates as local changes to push', () => {
619+
// bookmarksToUpdate are updates FROM cloud, not local changes
620+
// hasLocalChangesToPush should NOT include bookmarksToUpdate.length
621+
const localAdditions = []; // no new local bookmarks
622+
const locallyModifiedBookmarkIds = new Set(); // no locally modified bookmarks
623+
const localTombstones = [];
624+
const cloudTombstones = [];
625+
const deletedLocally = 0;
626+
const bookmarksToUpdate = [{ cloud: { url: 'a', index: 1 }, local: { url: 'a', index: 0 } }];
627+
628+
// This is the fixed logic — bookmarksToUpdate should NOT be included
629+
const hasLocalChangesToPush =
630+
localAdditions.length > 0 ||
631+
locallyModifiedBookmarkIds.size > 0 ||
632+
localTombstones.length > cloudTombstones.length ||
633+
deletedLocally > 0;
634+
635+
expect(hasLocalChangesToPush).toBe(false);
636+
// Even though there are bookmarks to update from cloud, we should NOT push back
637+
expect(bookmarksToUpdate.length).toBe(1);
638+
});
639+
640+
it('should push when there are actual local additions', () => {
641+
const localAdditions = [{ url: 'https://new-local.com' }];
642+
const locallyModifiedBookmarkIds = new Set();
643+
const localTombstones = [];
644+
const cloudTombstones = [];
645+
const deletedLocally = 0;
646+
647+
const hasLocalChangesToPush =
648+
localAdditions.length > 0 ||
649+
locallyModifiedBookmarkIds.size > 0 ||
650+
localTombstones.length > cloudTombstones.length ||
651+
deletedLocally > 0;
652+
653+
expect(hasLocalChangesToPush).toBe(true);
654+
});
655+
656+
it('should push when there are locally modified bookmarks', () => {
657+
const localAdditions = [];
658+
const locallyModifiedBookmarkIds = new Set(['bookmark-123']);
659+
const localTombstones = [];
660+
const cloudTombstones = [];
661+
const deletedLocally = 0;
662+
663+
const hasLocalChangesToPush =
664+
localAdditions.length > 0 ||
665+
locallyModifiedBookmarkIds.size > 0 ||
666+
localTombstones.length > cloudTombstones.length ||
667+
deletedLocally > 0;
668+
669+
expect(hasLocalChangesToPush).toBe(true);
670+
});
671+
});

apps/extension/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@marksyncr/extension",
3-
"version": "0.8.18",
3+
"version": "0.8.19",
44
"private": true,
55
"type": "module",
66
"scripts": {

apps/extension/src/background/index.js

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1726,13 +1726,12 @@ async function performSync(sourceId) {
17261726
console.log(`[MarkSyncr] Local checksum: ${localChecksum}`);
17271727
console.log(`[MarkSyncr] Cloud checksum: ${cloudChecksum}`);
17281728

1729-
// Determine if we have local changes to push to cloud
1730-
// Local changes = bookmarks that exist locally but not in cloud (before this sync)
1731-
// OR bookmarks that differ from cloud (local-first model: local changes override cloud)
1732-
// OR tombstones that need to be synced
1729+
// Determine if we have LOCAL (user-initiated) changes to push to cloud
1730+
// NOTE: bookmarksToUpdate are cloud→local updates (received FROM cloud), NOT local changes.
1731+
// Including them here was causing the receiving browser to re-push cloud data back,
1732+
// overwriting the original browser's ordering changes.
17331733
const hasLocalChangesToPush =
17341734
localAdditions.length > 0 ||
1735-
bookmarksToUpdate.length > 0 ||
17361735
locallyModifiedBookmarkIds.size > 0 ||
17371736
localTombstones.length > cloudTombstones.length ||
17381737
deletedLocally > 0;
@@ -1785,15 +1784,28 @@ async function performSync(sourceId) {
17851784
}
17861785

17871786
// Step 9: Push merged bookmarks and tombstones back to cloud
1788-
console.log(
1789-
`[MarkSyncr] Pushing ${mergedFlat.length} merged bookmarks and ${mergedTombstones.length} tombstones to cloud...`
1790-
);
1791-
const syncResult = await syncBookmarksToCloud(mergedFlat, detectBrowser(), mergedTombstones);
1792-
console.log('[MarkSyncr] Cloud sync result:', syncResult);
1787+
// ONLY push if we have actual local changes. If this was a pull-only sync
1788+
// (we just received updates from cloud), don't push back — that would overwrite
1789+
// the source browser's changes with our potentially different index ordering.
1790+
let syncResult = null;
1791+
if (hasLocalChangesToPush) {
1792+
console.log(
1793+
`[MarkSyncr] Pushing ${mergedFlat.length} merged bookmarks and ${mergedTombstones.length} tombstones to cloud...`
1794+
);
1795+
syncResult = await syncBookmarksToCloud(mergedFlat, detectBrowser(), mergedTombstones);
1796+
console.log('[MarkSyncr] Cloud sync result:', syncResult);
1797+
} else {
1798+
console.log(
1799+
'[MarkSyncr] Pull-only sync — skipping push to cloud (no local changes to push)'
1800+
);
1801+
}
17931802

17941803
// Store the new checksum
1795-
if (syncResult.checksum) {
1804+
if (syncResult?.checksum) {
17961805
await storeLastCloudChecksum(syncResult.checksum);
1806+
} else if (cloudChecksum) {
1807+
// Pull-only: store the cloud checksum so next sync knows we're in sync
1808+
await storeLastCloudChecksum(cloudChecksum);
17971809
}
17981810

17991811
// Step 10: Save version history ONLY when we have local changes being pushed to cloud

apps/web/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@marksyncr/web",
3-
"version": "0.8.18",
3+
"version": "0.8.19",
44
"private": true,
55
"type": "module",
66
"scripts": {

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "marksyncr",
3-
"version": "0.8.18",
3+
"version": "0.8.19",
44
"private": true,
55
"description": "Cross-browser bookmark sync extension with cloud storage",
66
"type": "module",

0 commit comments

Comments
 (0)