Conversation
ralyodio
approved these changes
Mar 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition where deleting a bookmark from the toolbar gets reverted by auto-sync. The root cause was that performSync() read tombstones at the start of the sync, but if a user deleted a bookmark mid-sync, the onRemoved handler wrote a new tombstone to storage that would be overwritten when performSync later called storeTombstones() with its stale merge result.
Changes:
- Re-reads current tombstones from storage before writing merged tombstones in
performSync(), preserving any concurrent tombstones written byonRemovedduring the sync. - Adds comprehensive unit tests (
toolbar-delete-revert.test.js) covering tombstone filtering, race condition scenarios, and cross-browser edge cases using copied functions. - Adds integration tests (
toolbar-delete-race-integration.test.js) that call the realperformSynccode with mocked browser APIs and simulates the exact race condition.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
apps/extension/src/background/index.js |
Re-reads tombstones before writing to avoid overwriting concurrent tombstones; updates all downstream references to use safeMergedTombstones; adds __test__ exports guarded by VITEST for integration testing |
apps/extension/__tests__/toolbar-delete-revert.test.js |
Unit tests for tombstone filtering, full sync flow simulation, race condition demonstration, server-side push, and edge cases using copied helper functions |
apps/extension/__tests__/toolbar-delete-race-integration.test.js |
Integration tests calling real performSync with mocked browser/fetch APIs, simulating the race condition by blocking the cloud GET and firing onRemoved mid-sync |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Race condition: when a user deletes a bookmark while a sync is running, performSync's storeTombstones() overwrites the tombstone created by the onRemoved listener, causing the follow-up sync to re-add the bookmark from cloud. Fix: re-read current tombstones from storage before writing to preserve any tombstones added concurrently during the sync.
…er APIs Add __test__ exports (VITEST-only, tree-shaken in production) to background/index.js so integration tests can import and call the real performSync, addTombstone, categorizeCloudBookmarks, etc. New test file: toolbar-delete-race-integration.test.js (10 tests) - Calls the REAL performSync with fully mocked browser.* and fetch APIs - Simulates the tombstone race condition: delays cloud GET, fires onRemoved mid-sync, verifies tombstone is preserved after sync - Verifies deleted bookmark is NOT re-added from cloud - Tests tombstone merge, categorization filtering, concurrent sync guard, and state management - All 10 tests pass; 527 total passing (was 517)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
f416513 to
e31f363
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a race condition where deleting a bookmark from the toolbar gets reverted when auto-sync runs.
Root Cause
A tombstone overwrite race condition in
performSync():performSyncreadslocalTombstones = []at sync startonRemovedfires →addTombstone()writes new tombstone to storageperformSynccomputesmergedTombstonesfrom the stale tombstones read in step 1performSynccallsstoreTombstones(mergedTombstones)→ overwrites the new tombstoneFix
Before writing merged tombstones, re-read current tombstones from storage to preserve any tombstones written concurrently by
onRemoved:Tests
Unit tests (19 tests) —
toolbar-delete-revert.test.jsIntegration tests (10 tests) —
toolbar-delete-race-integration.test.jsperformSynccode (not copied/reimplemented functions) with fully mockedbrowser.*andfetchAPIsonRemovedhandler mid-sync, verifies the tombstone is preserved and the bookmark isn't re-addedTest-only exports (
__test__inbackground/index.js)import.meta.env?.VITEST— tree-shaken in production buildsperformSync,addTombstone,categorizeCloudBookmarks,resetState, etc.Results
store.test.js(unrelatedstorage.setItemissue)