fix: prevent bookmark additions/deletions from reverting on next sync (#7)

Preshy · web-flow · commit 392294eb683e · 2026-03-17T09:44:36.000-07:00
Four interrelated bugs caused local changes (adds/deletes) to be
reverted by the next auto-sync after the first successful sync:

1. applyTombstonesToLocal did not set isSyncDrivenChange, causing
   cloud-driven deletions to be tracked as locally modified and
   triggering unnecessary follow-up syncs via pendingSyncNeeded.

2. forcePush/forcePull did not update lastSyncTime (tombstone
   safeguard), lastCloudChecksum (baseline for next sync), or clear
   locallyModifiedBookmarkIds. If the first sync was a force
   push/pull, all subsequent syncs had stale metadata. forcePush
   also did not send local tombstones to the cloud.

3. Server-side dateAdded normalization fell back to Date.now() for
   falsy values, making bookmarks appear brand-new and causing
   the tombstone filter (bookmarkDate &gt; tombstoneDate) to
   incorrectly keep bookmarks that should have been deleted.

4. Server-side replace mode redundantly re-applied tombstone
   filtering on data the extension had already filtered. Combined
   with the Date.now() fallback, this could silently strip or
   preserve bookmarks incorrectly.
diff --git a/apps/extension/src/background/index.js b/apps/extension/src/background/index.js
@@ -2046,27 +2046,36 @@ async function applyTombstonesToLocal(tombstones, localBookmarks) {
   }
   console.log(`[MarkSyncr] Found ${matchCount} local bookmarks that match tombstones`);
 
-  for (const bookmark of localBookmarks) {
-    if (tombstonedUrls.has(bookmark.url)) {
-      // Skip if this bookmark was locally modified (e.g., user re-added it after deletion)
-      // The user's intent to have this bookmark takes priority over the cloud tombstone
-      if (locallyModifiedBookmarkIds.has(bookmark.id)) {
-        console.log(
-          `[MarkSyncr] 🏠 Skipping tombstone for locally modified bookmark: ${bookmark.url}`
-        );
-        continue;
-      }
+  // Set isSyncDrivenChange so that onRemoved doesn't track these cloud-driven
+  // deletions as locally modified. Without this flag, each tombstone-driven
+  // removal was incorrectly added to locallyModifiedBookmarkIds and triggered
+  // an unnecessary follow-up sync via pendingSyncNeeded.
+  isSyncDrivenChange = true;
+  try {
+    for (const bookmark of localBookmarks) {
+      if (tombstonedUrls.has(bookmark.url)) {
+        // Skip if this bookmark was locally modified (e.g., user re-added it after deletion)
+        // The user's intent to have this bookmark takes priority over the cloud tombstone
+        if (locallyModifiedBookmarkIds.has(bookmark.id)) {
+          console.log(
+            `[MarkSyncr] 🏠 Skipping tombstone for locally modified bookmark: ${bookmark.url}`
+          );
+          continue;
+        }
 
-      console.log(`[MarkSyncr] Tombstone match found for: ${bookmark.url}`);
+        console.log(`[MarkSyncr] Tombstone match found for: ${bookmark.url}`);
 
-      try {
-        await browser.bookmarks.remove(bookmark.id);
-        deletedCount++;
-        console.log(`[MarkSyncr] ✓ Deleted local bookmark (tombstoned): ${bookmark.url}`);
-      } catch (err) {
-        console.warn(`[MarkSyncr] ✗ Failed to delete tombstoned bookmark: ${bookmark.url}`, err);
+        try {
+          await browser.bookmarks.remove(bookmark.id);
+          deletedCount++;
+          console.log(`[MarkSyncr] ✓ Deleted local bookmark (tombstoned): ${bookmark.url}`);
+        } catch (err) {
+          console.warn(`[MarkSyncr] ✗ Failed to delete tombstoned bookmark: ${bookmark.url}`, err);
+        }
       }
     }
+  } finally {
+    isSyncDrivenChange = false;
   }
 
   console.log(`[MarkSyncr] applyTombstonesToLocal complete: deleted ${deletedCount} bookmarks`);
@@ -2911,9 +2920,13 @@ async function forcePush() {
     // Pass the bookmark count as synced count since we're pushing all to cloud
     const stats = countBookmarks(bookmarkTree, flatBookmarks.filter((b) => b.url).length);
 
+    // Get current local tombstones to push alongside bookmarks
+    // Force push should include tombstones so the cloud knows about deletions
+    const localTombstones = await getTombstones();
+
     // Force push to cloud (overwrite)
     console.log(`[MarkSyncr] Force pushing ${flatBookmarks.length} bookmarks to cloud...`);
-    const syncResult = await syncBookmarksToCloud(flatBookmarks, detectBrowser());
+    const syncResult = await syncBookmarksToCloud(flatBookmarks, detectBrowser(), localTombstones);
     console.log('[MarkSyncr] Force push sync result:', syncResult);
 
     // Save version with force push marker (non-blocking)
@@ -2930,10 +2943,25 @@ async function forcePush() {
       console.warn('[MarkSyncr] Force push version save failed (continuing):', versionErr.message);
     }
 
-    // Update last sync time
+    // Update last sync time (both ISO string for display and timestamp for tombstone safeguard)
+    const syncTimestamp = Date.now();
     await browser.storage.local.set({
-      lastSync: new Date().toISOString(),
+      lastSync: new Date(syncTimestamp).toISOString(),
     });
+    await storeLastSyncTime(syncTimestamp);
+
+    // Store the cloud checksum so the next performSync knows the baseline
+    if (syncResult?.checksum) {
+      await storeLastCloudChecksum(syncResult.checksum);
+    }
+
+    // Clear locally modified tracking — force push overwrites cloud, so everything is in sync
+    if (saveModifiedIdsTimeout) {
+      clearTimeout(saveModifiedIdsTimeout);
+      saveModifiedIdsTimeout = null;
+    }
+    locallyModifiedBookmarkIds.clear();
+    await saveLocallyModifiedIds();
 
     return { success: true, stats, message: 'Successfully force pushed bookmarks to cloud' };
   } catch (err) {
@@ -3096,17 +3124,27 @@ async function forcePull() {
         `[MarkSyncr] Force Pull: Created ${importedCount} bookmarks, ${foldersCreated} folders`
       );
 
-      // Update last sync time
+      // Update last sync time (both ISO string for display and timestamp for tombstone safeguard)
+      const syncTimestamp = Date.now();
       await browser.storage.local.set({
-        lastSync: new Date().toISOString(),
+        lastSync: new Date(syncTimestamp).toISOString(),
       });
+      await storeLastSyncTime(syncTimestamp);
 
       const stats = { total: importedCount, folders: foldersCreated, synced: importedCount };
 
       console.log(
         `[MarkSyncr] Force Pull complete: ${importedCount} bookmarks, ${foldersCreated} folders`
       );
 
+      // Clear locally modified tracking — force pull overwrites local, so everything is in sync
+      if (saveModifiedIdsTimeout) {
+        clearTimeout(saveModifiedIdsTimeout);
+        saveModifiedIdsTimeout = null;
+      }
+      locallyModifiedBookmarkIds.clear();
+      await saveLocallyModifiedIds();
+
       // CRITICAL: After Force Pull, sync the pulled bookmarks back to cloud_bookmarks
       // This ensures the next regular sync sees matching data and doesn't revert changes.
       // Without this, the next sync would compare local bookmarks (from version history)
diff --git a/apps/web/__tests__/bookmarks.test.js b/apps/web/__tests__/bookmarks.test.js
@@ -3023,9 +3023,36 @@ describe('Server-Side Tombstone Cleanup', () => {
 
       // Existing cloud data has a DIFFERENT order
       const existingCloudBookmarks = [
-        { id: '1', url: 'https://first.com', title: 'First', index: 0, type: 'bookmark', folderPath: '', dateAdded: 1000, source: 'browser' },
-        { id: '2', url: 'https://second.com', title: 'Second', index: 1, type: 'bookmark', folderPath: '', dateAdded: 1000, source: 'browser' },
-        { id: '3', url: 'https://third.com', title: 'Third', index: 2, type: 'bookmark', folderPath: '', dateAdded: 1000, source: 'browser' },
+        {
+          id: '1',
+          url: 'https://first.com',
+          title: 'First',
+          index: 0,
+          type: 'bookmark',
+          folderPath: '',
+          dateAdded: 1000,
+          source: 'browser',
+        },
+        {
+          id: '2',
+          url: 'https://second.com',
+          title: 'Second',
+          index: 1,
+          type: 'bookmark',
+          folderPath: '',
+          dateAdded: 1000,
+          source: 'browser',
+        },
+        {
+          id: '3',
+          url: 'https://third.com',
+          title: 'Third',
+          index: 2,
+          type: 'bookmark',
+          folderPath: '',
+          dateAdded: 1000,
+          source: 'browser',
+        },
       ];
 
       let upsertedData = null;
@@ -3084,16 +3111,16 @@ describe('Server-Side Tombstone Cleanup', () => {
       expect(storedBookmarks[2].url).toBe('https://second.com');
     });
 
-    it('should still apply tombstone filtering when replace=true', async () => {
+    it('should trust incoming bookmarks as-is when replace=true (extension handles tombstone filtering)', async () => {
       const now = Date.now();
+      // In replace mode, the extension has already performed tombstone filtering
+      // client-side. The server should store whatever the extension sends.
       const bookmarksToSync = [
         { id: '1', url: 'https://keep.com', title: 'Keep', dateAdded: now - 10000 },
-        { id: '2', url: 'https://deleted.com', title: 'Deleted', dateAdded: now - 10000 },
+        { id: '2', url: 'https://also-keep.com', title: 'Also Keep', dateAdded: now - 10000 },
       ];
 
-      const incomingTombstones = [
-        { url: 'https://deleted.com', deletedAt: now - 5000 },
-      ];
+      const incomingTombstones = [{ url: 'https://already-deleted.com', deletedAt: now - 5000 }];
 
       let upsertedData = null;
       const upsertMock = vi.fn().mockImplementation((data) => {
@@ -3139,10 +3166,13 @@ describe('Server-Side Tombstone Cleanup', () => {
       const response = await POST(request);
       expect(response.status).toBe(200);
 
-      // Only the non-tombstoned bookmark should be stored
+      // Both bookmarks should be stored — server trusts the extension in replace mode
       const storedBookmarks = upsertedData.bookmark_data;
-      expect(storedBookmarks).toHaveLength(1);
-      expect(storedBookmarks[0].url).toBe('https://keep.com');
+      expect(storedBookmarks).toHaveLength(2);
+      expect(storedBookmarks.map((b) => b.url).sort()).toEqual([
+        'https://also-keep.com',
+        'https://keep.com',
+      ]);
     });
 
     it('should use legacy merge behavior when replace is not set', async () => {
@@ -3151,7 +3181,16 @@ describe('Server-Side Tombstone Cleanup', () => {
       ];
 
       const existingCloudBookmarks = [
-        { id: '1', url: 'https://existing.com', title: 'Existing', index: 0, type: 'bookmark', folderPath: '', dateAdded: 1000, source: 'browser' },
+        {
+          id: '1',
+          url: 'https://existing.com',
+          title: 'Existing',
+          index: 0,
+          type: 'bookmark',
+          folderPath: '',
+          dateAdded: 1000,
+          source: 'browser',
+        },
       ];
 
       let upsertedData = null;
diff --git a/apps/web/app/api/bookmarks/route.js b/apps/web/app/api/bookmarks/route.js
@@ -673,7 +673,9 @@ async function tryRefreshDropboxToken(supabase, source) {
   const clientSecret = process.env.DROPBOX_CLIENT_SECRET;
 
   if (!clientId || !clientSecret) {
-    console.error('[Dropbox Sync] Cannot refresh token: DROPBOX_CLIENT_ID or DROPBOX_CLIENT_SECRET not configured');
+    console.error(
+      '[Dropbox Sync] Cannot refresh token: DROPBOX_CLIENT_ID or DROPBOX_CLIENT_SECRET not configured'
+    );
     return null;
   }
 
@@ -754,7 +756,8 @@ async function syncToDropbox(supabase, source, bookmarks, tombstones, checksum)
     });
   } catch (error) {
     // If the error indicates an expired token, try refreshing and retrying once
-    const isExpiredToken = error.message?.includes('expired_access_token') ||
+    const isExpiredToken =
+      error.message?.includes('expired_access_token') ||
       error.message?.includes('invalid_access_token');
 
     if (isExpiredToken) {
@@ -829,13 +832,23 @@ export async function POST(request) {
     // Normalize bookmarks structure
     // Preserve empty titles - don't replace with URL
     // IMPORTANT: Preserve type and index fields for proper checksum comparison
+    //
+    // NOTE on dateAdded: We preserve the original value (or 0 if falsy) instead
+    // of falling back to Date.now(). The old Date.now() fallback was dangerous:
+    // when dateAdded was falsy, the server would assign the current timestamp,
+    // making the bookmark appear "brand new." This caused the tombstone filter
+    // (bookmarkDate > tombstoneDate) to incorrectly keep bookmarks that should
+    // have been deleted — the bookmark looked newer than its tombstone.
     const normalizedBookmarks = bookmarks.map((bookmark) => ({
       id: bookmark.id,
       type: bookmark.type || 'bookmark', // Preserve type (bookmark or folder)
       url: bookmark.url,
       title: bookmark.title ?? '',
       folderPath: bookmark.folderPath || bookmark.folder_path || '',
-      dateAdded: (typeof bookmark.dateAdded === 'string' ? new Date(bookmark.dateAdded).getTime() : bookmark.dateAdded) || Date.now(),
+      dateAdded:
+        (typeof bookmark.dateAdded === 'string'
+          ? new Date(bookmark.dateAdded).getTime()
+          : bookmark.dateAdded) || 0,
       index: bookmark.index ?? 0, // Preserve index for ordering
       source,
     }));
@@ -903,17 +916,20 @@ export async function POST(request) {
     let deleted = 0;
 
     if (replace) {
-      console.log(`[Bookmarks API] Replace mode: using incoming bookmarks directly (skipping merge)`);
-      // Apply tombstones to filter deleted bookmarks from incoming data
-      const tombstoneMap = new Map(mergedTombstones.map((t) => [t.url, t.deletedAt || 0]));
-      finalBookmarks = normalizedBookmarks.filter((b) => {
-        if (!b.url) return true; // Keep folders
-        const tombstoneDate = tombstoneMap.get(b.url);
-        if (tombstoneDate === undefined) return true; // No tombstone
-        const bookmarkDate = typeof b.dateAdded === 'string' ? new Date(b.dateAdded).getTime() : (b.dateAdded || 0);
-        return bookmarkDate > tombstoneDate;
-      });
-      deleted = normalizedBookmarks.length - finalBookmarks.length;
+      console.log(
+        `[Bookmarks API] Replace mode: using incoming bookmarks directly (skipping merge and tombstone filter)`
+      );
+      // In replace mode, the extension has already performed a full two-way merge
+      // INCLUDING tombstone filtering on the client side. The incoming bookmarks
+      // are the authoritative state — we trust them as-is.
+      //
+      // IMPORTANT: We no longer re-apply tombstone filtering here because:
+      // 1. The extension already filtered tombstoned bookmarks before pushing
+      // 2. Re-filtering with dateAdded comparisons was causing bookmarks with
+      //    falsy/stale dateAdded to be incorrectly removed from the cloud
+      // 3. The extension is the source of truth in replace mode
+      finalBookmarks = normalizedBookmarks;
+      deleted = 0;
       added = finalBookmarks.length;
     } else {
       // Legacy merge behavior
@@ -928,7 +944,8 @@ export async function POST(request) {
         if (!b.url) return true; // Keep folders
         const tombstoneDate = tombstoneMap.get(b.url);
         if (tombstoneDate === undefined) return true; // No tombstone
-        const bookmarkDate = typeof b.dateAdded === 'string' ? new Date(b.dateAdded).getTime() : (b.dateAdded || 0);
+        const bookmarkDate =
+          typeof b.dateAdded === 'string' ? new Date(b.dateAdded).getTime() : b.dateAdded || 0;
         return bookmarkDate > tombstoneDate;
       });
       deleted = merged.length - finalBookmarks.length;
@@ -937,9 +954,7 @@ export async function POST(request) {
     console.log(
       `[Bookmarks API] After ${replace ? 'replace' : 'merge'}: ${finalBookmarks.length + deleted} total, ${added} added, ${updated} updated, ${deleted} removed by tombstones`
     );
-    console.log(
-      `[Bookmarks API] Tombstones: ${mergedTombstones.length} stored`
-    );
+    console.log(`[Bookmarks API] Tombstones: ${mergedTombstones.length} stored`);
 
     // Validate total bookmark count after merge to prevent data explosion
     if (finalBookmarks.length > MAX_BOOKMARKS_PER_USER) {
