fix: preserve locally modified bookmark titles, folders, and ordering during sync (#9)

* fix: preserve locally modified bookmark titles and folders during sync

Previously, locallyModifiedBookmarkIds protection only skipped index-only
changes from cloud. Title and folder changes were allowed through, which
caused local edits (e.g., renaming a bookmark label) to be overwritten by
stale cloud data during the pull phase. By the time the push phase ran,
the local change had already been reverted, so nothing got pushed.

Now ALL cloud-driven updates are skipped for locally modified bookmarks,
allowing local title, folder, and index changes to survive the pull phase
and get pushed to cloud correctly.

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix: skip ALL cloud updates for locally modified bookmarks (not just index-only)

Update categorizeCloudBookmarks, updateLocalBookmarksFromCloud, and tests
to ensure local wins for title, folder, AND index changes when a bookmark
is in locallyModifiedBookmarkIds. Fixes the bug where renaming a bookmark
label locally (e.g. 'incident' -> 's') gets reverted on next sync.

Also reverts Copilot autofix commits that partially restored the old
broken behavior.

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: Preshy

apps/extension/__tests__/local-reorder-preserved.test.js

@@ -5,13 +5,13 @@
  * the sync pulls the cloud's old order and applies it locally, reverting the
  * user's changes. The cloud's stale indices overwrite local modifications.
  *
- * Fix: Skip cloud-to-local index updates and reordering for bookmarks that are
- * in the locallyModifiedBookmarkIds set. The local order takes priority and
- * gets pushed to cloud instead.
+ * Fix: Skip ALL cloud-to-local updates (title, folder, index) for bookmarks
+ * that are in the locallyModifiedBookmarkIds set. The local state takes
+ * priority and gets pushed to cloud instead.
  *
  * Three fix points tested:
- * 1. categorizeCloudBookmarks() - skips index-only updates for locally modified bookmarks
- * 2. updateLocalBookmarksFromCloud() - skips index-only moves for locally modified bookmarks
+ * 1. categorizeCloudBookmarks() - skips ALL cloud updates for locally modified bookmarks
+ * 2. updateLocalBookmarksFromCloud() - skips ALL cloud updates for locally modified bookmarks
  * 3. reorderLocalToMatchCloud() - skips reorder for folders with locally modified children
  */
 
@@ -46,7 +46,8 @@ function bookmarkNeedsUpdate(cloudBm, localBm) {
 }
 
 /**
- * categorizeCloudBookmarks WITH the fix: skip index-only updates for locally modified bookmarks
+ * categorizeCloudBookmarks WITH the fix: skip ALL cloud updates for locally modified bookmarks.
+ * Local state takes priority and will be pushed to cloud in the push phase.
  */
 function categorizeCloudBookmarks(
   cloudBookmarks,
@@ -80,17 +81,11 @@ function categorizeCloudBookmarks(
     if (!localBm) {
       toAdd.push(cloudBm);
     } else if (bookmarkNeedsUpdate(cloudBm, localBm)) {
-      // FIX: Skip index-only updates for locally modified bookmarks
+      // FIX: Skip ALL cloud updates for locally modified bookmarks.
+      // Local wins for title, folder, AND index changes.
       if (locallyModifiedBookmarkIds.has(localBm.id)) {
-        const cloudFolder = normalizeFolderPath(cloudBm.folderPath);
-        const localFolder = normalizeFolderPath(localBm.folderPath);
-        const titleChanged = (cloudBm.title ?? '') !== (localBm.title ?? '');
-        const folderChanged = cloudFolder !== localFolder;
-
-        if (!titleChanged && !folderChanged) {
-          skippedByLocalModification.push(cloudBm.url);
-          continue;
-        }
+        skippedByLocalModification.push(cloudBm.url);
+        continue;
       }
       toUpdate.push({ cloud: cloudBm, local: localBm });
     } else {
@@ -178,7 +173,7 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
       expect(result.skippedByLocalModification.length).toBe(5);
     });
 
-    it('should still allow title changes from cloud even for locally modified bookmarks', () => {
+    it('should skip cloud title changes for locally modified bookmarks (local wins)', () => {
       const cloudBookmarks = [
         {
           url: 'https://github.com',
@@ -207,12 +202,13 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
         locallyModifiedBookmarkIds
       );
 
-      // Title changed in cloud - should still be updated
-      expect(result.toUpdate.length).toBe(1);
-      expect(result.skippedByLocalModification.length).toBe(0);
+      // Locally modified bookmark — local wins, cloud title change is skipped.
+      // The local state will be pushed to cloud in the push phase.
+      expect(result.toUpdate.length).toBe(0);
+      expect(result.skippedByLocalModification.length).toBe(1);
     });
 
-    it('should still allow folder changes from cloud even for locally modified bookmarks', () => {
+    it('should skip cloud folder changes for locally modified bookmarks (local wins)', () => {
       const cloudBookmarks = [
         { url: 'https://github.com', title: 'GitHub', folderPath: 'Bookmarks Bar/Work', index: 0 },
       ];
@@ -236,9 +232,10 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
         locallyModifiedBookmarkIds
       );
 
-      // Folder changed in cloud - should still be updated
-      expect(result.toUpdate.length).toBe(1);
-      expect(result.skippedByLocalModification.length).toBe(0);
+      // Locally modified bookmark — local wins, cloud folder change is skipped.
+      // The local state will be pushed to cloud in the push phase.
+      expect(result.toUpdate.length).toBe(0);
+      expect(result.skippedByLocalModification.length).toBe(1);
     });
 
     it('should update bookmarks normally when they are NOT locally modified', () => {
@@ -303,17 +300,17 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
         locallyModifiedBookmarkIds
       );
 
-      // bm-1: locally modified, index-only change -> skip
-      // bm-2: locally modified, index-only change -> skip
-      // bm-3: NOT locally modified, title changed -> update
+      // bm-1: locally modified -> skip (local wins)
+      // bm-2: locally modified -> skip (local wins)
+      // bm-3: NOT locally modified, title changed -> update from cloud
       expect(result.skippedByLocalModification.length).toBe(2);
       expect(result.toUpdate.length).toBe(1);
       expect(result.toUpdate[0].cloud.url).toBe('https://c.com');
     });
   });
 
   describe('updateLocalBookmarksFromCloud - skip index-only moves for locally modified', () => {
-    it('should skip index-only moves for locally modified bookmarks', () => {
+    it('should skip ALL cloud updates for locally modified bookmarks', () => {
       const locallyModifiedBookmarkIds = new Set(['bm-1', 'bm-2', 'bm-3']);
 
       const bookmarksToUpdate = [
@@ -344,32 +341,21 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
       const updated = [];
 
       for (const { cloud, local } of bookmarksToUpdate) {
-        const titleChanged = (cloud.title ?? '') !== (local.title ?? '');
-        const folderChanged =
-          normalizeFolderPath(cloud.folderPath) !== normalizeFolderPath(local.folderPath);
-        const indexChanged =
-          cloud.index !== undefined && local.index !== undefined && cloud.index !== local.index;
-
-        // FIX: skip index-only moves for locally modified bookmarks
-        if (
-          locallyModifiedBookmarkIds.has(local.id) &&
-          !titleChanged &&
-          !folderChanged &&
-          indexChanged
-        ) {
+        // FIX: skip ALL cloud updates for locally modified bookmarks
+        if (locallyModifiedBookmarkIds.has(local.id)) {
           skipped.push(cloud.url);
           continue;
         }
 
         updated.push(cloud.url);
       }
 
-      // Both bookmarks have index-only changes and are locally modified
+      // Both bookmarks are locally modified — local wins
       expect(skipped).toEqual(['https://a.com', 'https://b.com']);
       expect(updated).toEqual([]);
     });
 
-    it('should still update title even for locally modified bookmarks', () => {
+    it('should skip ALL cloud updates for locally modified bookmarks (including title)', () => {
       const locallyModifiedBookmarkIds = new Set(['bm-1']);
 
       const bookmarksToUpdate = [
@@ -394,28 +380,18 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
       const updated = [];
 
       for (const { cloud, local } of bookmarksToUpdate) {
-        const titleChanged = (cloud.title ?? '') !== (local.title ?? '');
-        const folderChanged =
-          normalizeFolderPath(cloud.folderPath) !== normalizeFolderPath(local.folderPath);
-        const indexChanged =
-          cloud.index !== undefined && local.index !== undefined && cloud.index !== local.index;
-
-        if (
-          locallyModifiedBookmarkIds.has(local.id) &&
-          !titleChanged &&
-          !folderChanged &&
-          indexChanged
-        ) {
+        // FIX: skip ALL cloud updates for locally modified bookmarks
+        if (locallyModifiedBookmarkIds.has(local.id)) {
           skipped.push(cloud.url);
           continue;
         }
 
         updated.push(cloud.url);
       }
 
-      // Title changed - should NOT be skipped
-      expect(skipped).toEqual([]);
-      expect(updated).toEqual(['https://a.com']);
+      // Locally modified — local wins, cloud title change is skipped
+      expect(skipped).toEqual(['https://a.com']);
+      expect(updated).toEqual([]);
     });
   });
 
@@ -697,8 +673,8 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
       expect(result.skippedByLocalModification.length).toBe(1);
     });
 
-    it('should detect actual folder changes even after normalization', () => {
-      // Cloud moved bookmark to a subfolder - this is a real change
+    it('should skip cloud folder changes for locally modified bookmarks even after normalization', () => {
+      // Cloud moved bookmark to a subfolder - but bookmark is locally modified, so local wins
       const cloudBookmarks = [
         { url: 'https://a.com', title: 'A', folderPath: 'Bookmarks Bar/Work', index: 0 },
       ];
@@ -716,9 +692,9 @@ describe('Bug Fix: Bookmark reorder should NOT reset on Sync Now', () => {
         locallyModifiedBookmarkIds
       );
 
-      // Folder changed (toolbar/ vs toolbar/Work) - should be updated
-      expect(result.toUpdate.length).toBe(1);
-      expect(result.skippedByLocalModification.length).toBe(0);
+      // Locally modified — local wins, cloud folder change is skipped
+      expect(result.toUpdate.length).toBe(0);
+      expect(result.skippedByLocalModification.length).toBe(1);
     });
   });
 });

apps/extension/src/background/index.js

@@ -492,26 +492,14 @@ function categorizeCloudBookmarks(cloudBookmarks, localBookmarks, tombstones) {
     if (!localBm) {
       toAdd.push(cloudBm);
     } else if (bookmarkNeedsUpdate(cloudBm, localBm)) {
-      // If this bookmark was locally modified (e.g., user reordered it), skip
-      // cloud-driven index/position updates. The local state takes priority and
-      // will be pushed to cloud. This prevents the "reorder then Sync Now resets
-      // order" bug where cloud's stale indices overwrite local changes.
-      // We still allow title and folder changes from cloud (meaningful edits),
-      // but skip index-only changes for locally modified bookmarks.
+      // If this bookmark was locally modified (e.g., user renamed or reordered
+      // it), skip ALL cloud-driven updates. The local state takes priority and
+      // will be pushed to cloud in the push phase. Previously we only skipped
+      // index-only changes, which caused local title/folder edits to be
+      // overwritten by stale cloud data.
       if (locallyModifiedBookmarkIds.has(localBm.id)) {
-        const cloudFolder = normalizeFolderPath(cloudBm.folderPath);
-        const localFolder = normalizeFolderPath(localBm.folderPath);
-        const titleChanged = (cloudBm.title ?? '') !== (localBm.title ?? '');
-        const folderChanged = cloudFolder !== localFolder;
-
-        if (!titleChanged && !folderChanged) {
-          // Only the index differs — this is a reorder conflict.
-          // Local wins because the user just rearranged these bookmarks.
-          skippedByLocalModification.push(cloudBm.url);
-          continue;
-        }
-        // Title or folder changed in cloud — allow the update even for locally
-        // modified bookmarks (meaningful edit from another browser).
+        skippedByLocalModification.push(cloudBm.url);
+        continue;
       }
       toUpdate.push({ cloud: cloudBm, local: localBm });
     } else {
@@ -2347,12 +2335,11 @@ async function addCloudBookmarksToLocal(cloudItems) {
 
         if (item.type === 'folder') {
           // It's a folder entry - create the folder WITHOUT specifying index.
-          // Index-based positioning is handled by reorderLocalToMatchCloud after
-          // all items are created. Setting index here causes cascading shifts:
-          // each insertion at index N shifts all items at N+ to the right, so
-          // subsequent inserts land at the wrong position. When the cloud index
-          // exceeds the local folder's item count, the browser clamps it to the
-          // end — the main cause of "bookmarks end up at the END of toolbar."
+          // Contract: creation only establishes parent/identity; index/ordering
+          // are applied later by reorderLocalToMatchCloud once all items exist.
+          // Setting index during creation can cause cascading shifts (each insert
+          // at index N pushes items at N+ to the right) and lead to incorrect
+          // positions when the cloud index exceeds the current local item count.
           const children = await browser.bookmarks.getChildren(targetFolderId);
           const existingFolder = children.find((c) => !c.url && c.title === item.title);
 
@@ -2513,14 +2500,19 @@ async function reorderLocalToMatchCloud(cloudBookmarks, userModifiedIds = null)
 
     if (children.length < 2) continue;
 
-    // Skip reordering only when the user manually rearranged bookmarks
-    // before this sync started. We use `userModifiedIds` — a snapshot of
-    // locallyModifiedBookmarkIds taken at the START of performSync, before
-    // any sync-driven creates/moves/deletes. This ensures that sync-driven
-    // changes (adds, folder moves, tombstone deletions) don't accidentally
+    // Skip auto-reordering when a folder appears to have multiple
+    // *locally modified* children before this sync started. This is a
+    // conservative heuristic that treats "more than one modified child"
+    // as a signal that the user may have intentionally rearranged or
+    // otherwise edited that folder and we should not override it.
+    //
+    // We use `userModifiedIds` — a snapshot of locallyModifiedBookmarkIds
+    // taken at the START of performSync, before any sync-driven
+    // creates/moves/deletes. This ensures that sync-driven changes
+    // (adds, folder moves, tombstone deletions) don't accidentally
     // trigger the skip, which was the root cause of "bookmarks end up at
-    // the end of the toolbar" — the reorder pass was being skipped because
-    // sync-driven onMoved events polluted locallyModifiedBookmarkIds.
+    // the end of the toolbar" — the reorder pass was being skipped
+    // because sync-driven onMoved events polluted locallyModifiedBookmarkIds.
     const idsToCheck = userModifiedIds || locallyModifiedBookmarkIds;
     const modifiedChildrenCount = children.reduce(
       (count, child) => (idsToCheck.has(child.id) ? count + 1 : count),
@@ -2687,17 +2679,13 @@ async function updateLocalBookmarksFromCloud(bookmarksToUpdate) {
       const indexChanged =
         cloud.index !== undefined && local.index !== undefined && cloud.index !== local.index;
 
-      // Skip index-only moves for locally modified bookmarks.
-      // This prevents the "reorder then Sync Now resets order" bug where
-      // cloud's stale indices overwrite local changes the user just made.
-      if (
-        locallyModifiedBookmarkIds.has(local.id) &&
-        !titleChanged &&
-        !folderChanged &&
-        indexChanged
-      ) {
+      // Skip ALL cloud-driven updates for locally modified bookmarks.
+      // Local state takes priority and will be pushed to cloud in the push phase.
+      // This prevents stale cloud data (title, folder, index) from overwriting
+      // local changes the user just made.
+      if (locallyModifiedBookmarkIds.has(local.id)) {
         console.log(
-          `[MarkSyncr] 🏠 Skipping index-only update for locally modified bookmark: ${cloud.url} (local index ${local.index} vs cloud index ${cloud.index})`
+          `[MarkSyncr] 🏠 Skipping cloud update for locally modified bookmark: ${cloud.url} (title: "${local.title}" vs cloud "${cloud.title}", index: ${local.index} vs cloud ${cloud.index})`
         );
         continue;
       }