Skip to content

Commit 90f85fc

Browse files
committed
fix: prevent detail fetch from restoring notifications after mark-as-read
A concurrent detail fetch could snapshot storage before any mark-as-read operation removed entries, then write the stale snapshot back (restoring already-read notifications). Fixed by: - Bumping notificationFetchVersion in markAsRead, markAllAsRead, and markRepoAsRead after the API call completes. - Adding a second version check after the async storage.getNotifications() read in both priority and background detail-save paths, so a mutation that happened during the read causes the write to be discarded.
1 parent ead1924 commit 90f85fc

1 file changed

Lines changed: 30 additions & 10 deletions

File tree

src/background/service-worker.js

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -422,11 +422,15 @@ async function checkNotifications() {
422422
if (currentFetchVersion >= notificationFetchVersion) {
423423
// Merge with current storage to avoid overwriting user deletions
424424
const currentStored = await storage.getNotifications();
425-
const safeDetailed = filterToCurrentlyStored(detailedNotifications, currentStored);
426-
427-
// Save priority notifications immediately
428-
await storage.setNotifications(safeDetailed);
429-
console.log(`Fetch #${currentFetchVersion} saved ${priorityNotifications.length} priority notifications`);
425+
// Re-check version after async read: a user mark-as-read may have bumped
426+
// notificationFetchVersion between the read and write, making our snapshot stale.
427+
if (currentFetchVersion >= notificationFetchVersion) {
428+
const safeDetailed = filterToCurrentlyStored(detailedNotifications, currentStored);
429+
430+
// Save priority notifications immediately
431+
await storage.setNotifications(safeDetailed);
432+
console.log(`Fetch #${currentFetchVersion} saved ${priorityNotifications.length} priority notifications`);
433+
}
430434
}
431435
}
432436

@@ -464,11 +468,15 @@ async function checkNotifications() {
464468
if (currentFetchVersion >= notificationFetchVersion) {
465469
// Merge with current storage to avoid overwriting user deletions
466470
const currentStoredNotifications = await storage.getNotifications();
467-
const safeDetailed = filterToCurrentlyStored(detailedNotifications, currentStoredNotifications);
468-
469-
// Update storage with all completed details
470-
await storage.setNotifications(safeDetailed);
471-
console.log(`Fetch #${currentFetchVersion} updated storage with detailed notifications`);
471+
// Re-check version after async read: a user mark-as-read may have bumped
472+
// notificationFetchVersion between the read and write, making our snapshot stale.
473+
if (currentFetchVersion >= notificationFetchVersion) {
474+
const safeDetailed = filterToCurrentlyStored(detailedNotifications, currentStoredNotifications);
475+
476+
// Update storage with all completed details
477+
await storage.setNotifications(safeDetailed);
478+
console.log(`Fetch #${currentFetchVersion} updated storage with detailed notifications`);
479+
}
472480
} else {
473481
console.log(`Fetch #${currentFetchVersion} superseded before final save, skipping storage update`);
474482
}
@@ -709,6 +717,9 @@ async function markAsRead(notificationId) {
709717
try {
710718
await github.markAsRead(notificationId);
711719

720+
// Invalidate in-progress detail fetches so they don't restore this notification.
721+
notificationFetchVersion++;
722+
712723
// Update local storage
713724
const notifications = await storage.getNotifications();
714725
const updated = notifications.filter((n) => n.id !== notificationId);
@@ -727,6 +738,9 @@ async function markAllAsRead() {
727738
try {
728739
await github.markAllAsRead();
729740

741+
// Invalidate in-progress detail fetches so they don't restore notifications after the clear.
742+
notificationFetchVersion++;
743+
730744
// Clear local storage
731745
hasMoreNotifications = false;
732746
await storage.setNotifications([]);
@@ -743,6 +757,9 @@ async function markRepoAsRead(owner, repo) {
743757
try {
744758
await github.markRepoAsRead(owner, repo);
745759

760+
// Invalidate in-progress detail fetches so they don't restore repo notifications.
761+
notificationFetchVersion++;
762+
746763
// Filter out notifications from this repository
747764
const notifications = await storage.getNotifications();
748765
const updated = notifications.filter((n) => n.repository.full_name !== `${owner}/${repo}`);
@@ -913,6 +930,9 @@ notifications.onClicked.addListener(async (notificationId) => {
913930
const githubNotifId = notificationId.slice(NOTIFICATION_ID_PREFIX.length);
914931

915932
// Get all notifications to find the one that was clicked
933+
// Invalidate in-progress fetches before the async chain so any fetch that
934+
// passes its version check during our read won't restore the removed notification.
935+
notificationFetchVersion++;
916936
const notificationsList = await storage.getNotifications();
917937
const notification = notificationsList.find((n) => n.id === githubNotifId);
918938

0 commit comments

Comments
 (0)