From ee8c097012ddd2cb030ec62697e868517803b44f Mon Sep 17 00:00:00 2001 From: Grace Kind Date: Sat, 1 Aug 2026 00:21:28 -0500 Subject: [PATCH] Verify updates before changing notification count --- package.json | 2 +- src/js/notificationService.js | 29 ++- tests/unit/specs/notificationService.test.js | 241 +++++++++++++++---- 3 files changed, 217 insertions(+), 55 deletions(-) diff --git a/package.json b/package.json index da40f0c7..bd253e93 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "impro", - "version": "0.18.138", + "version": "0.18.139", "type": "module", "scripts": { "start": "rm -rf \"${BUILD_DIR:-build}\" && NODE_ENV=development eleventy --serve", diff --git a/src/js/notificationService.js b/src/js/notificationService.js index f892e545..317010e4 100644 --- a/src/js/notificationService.js +++ b/src/js/notificationService.js @@ -2,12 +2,15 @@ import { wait } from "/js/utils.js"; import { Signal } from "/js/signals.js"; const POLLING_INTERVAL_SECONDS = 10; +const VERIFY_MAX_TRIES = 3; +const VERIFY_RETRY_MS = 1000; export class NotificationService { constructor(api) { this.api = api; this.$numNotifications = new Signal.State(0); this.$numNotifications.__debugName = "$numNotifications"; + this._lastVerifiedTopUri = null; } snooze(timeoutMinutes = 120) { @@ -34,9 +37,31 @@ export class NotificationService { } async fetchNumNotifications() { const numNotifications = await this.api.getNumNotifications(); - if (numNotifications !== this.$numNotifications.get()) { - this.$numNotifications.set(numNotifications); + const currentCount = this.$numNotifications.get(); + if (numNotifications === currentCount) return; + // The count endpoint updates before listNotifications, so wait for + // the endpoint to return new notifications before updating + if (numNotifications > currentCount) { + if (!(await this._verifyListHasNewItems())) return; } + this.$numNotifications.set(numNotifications); + } + + async _verifyListHasNewItems() { + for (let attempt = 0; attempt < VERIFY_MAX_TRIES; attempt++) { + try { + const res = await this.api.getNotifications({ limit: 1 }); + const topUri = res.notifications[0]?.uri ?? null; + if (topUri && topUri !== this._lastVerifiedTopUri) { + this._lastVerifiedTopUri = topUri; + return true; + } + } catch (error) { + console.warn(error); + } + if (attempt < VERIFY_MAX_TRIES - 1) await wait(VERIFY_RETRY_MS); + } + return false; } async markNotificationsAsRead() { // optimistic update diff --git a/tests/unit/specs/notificationService.test.js b/tests/unit/specs/notificationService.test.js index 7457f0e5..1fe9c145 100644 --- a/tests/unit/specs/notificationService.test.js +++ b/tests/unit/specs/notificationService.test.js @@ -1,85 +1,222 @@ -import { describe, it, mock } from "node:test"; +import { describe, it, mock, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; import { NotificationService } from "/js/notificationService.js"; -// Mock API -function createMockApi({ numNotifications = 0, markAsReadFn = null } = {}) { +function createMockApi({ + numNotifications = 0, + markAsReadFn = null, + getNotificationsFn = null, + topUri = "at://did:example/app.bsky.feed.post/first", +} = {}) { return { getNumNotifications: async () => numNotifications, markNotificationsAsRead: markAsReadFn || (async () => {}), + getNotifications: + getNotificationsFn || + (async () => ({ notifications: topUri ? [{ uri: topUri }] : [] })), }; } -describe("constructor", () => { - it("should initialize with zero notifications", () => { - const api = createMockApi(); - const service = new NotificationService(api); - assert.deepEqual(service.$numNotifications.get(), 0); +describe("NotificationService", () => { + const originalSetTimeout = globalThis.setTimeout; + beforeEach(() => { + globalThis.setTimeout = (fn) => originalSetTimeout(fn, 0); + }); + afterEach(() => { + globalThis.setTimeout = originalSetTimeout; }); -}); -describe("fetchNumNotifications", () => { - it("should update notification count from API", async () => { - const api = createMockApi({ numNotifications: 5 }); - const service = new NotificationService(api); + describe("constructor", () => { + it("should initialize with zero notifications", () => { + const api = createMockApi(); + const service = new NotificationService(api); + assert.deepEqual(service.$numNotifications.get(), 0); + }); + }); - await service.fetchNumNotifications(); + describe("fetchNumNotifications", () => { + it("should update notification count from API", async () => { + const api = createMockApi({ numNotifications: 5 }); + const service = new NotificationService(api); - assert.deepEqual(service.$numNotifications.get(), 5); - }); + await service.fetchNumNotifications(); - it("should update $numNotifications signal when count changes", async () => { - const api = createMockApi({ numNotifications: 3 }); - const service = new NotificationService(api); + assert.deepEqual(service.$numNotifications.get(), 5); + }); - assert.deepEqual(service.$numNotifications.get(), 0); + it("should update $numNotifications signal when count changes", async () => { + const api = createMockApi({ numNotifications: 3 }); + const service = new NotificationService(api); - await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 0); - assert.deepEqual(service.$numNotifications.get(), 3); - }); -}); + await service.fetchNumNotifications(); -describe("$numNotifications", () => { - it("should reflect current notification count", async () => { - const api = createMockApi({ numNotifications: 7 }); - const service = new NotificationService(api); + assert.deepEqual(service.$numNotifications.get(), 3); + }); - assert.deepEqual(service.$numNotifications.get(), 0); + it("commits the increased count once the list reflects a new top item", async () => { + let topUri = "at://did:example/app.bsky.feed.post/first"; + const getNotificationsFn = mock.fn(async () => ({ + notifications: [{ uri: topUri }], + })); + const api = createMockApi({ + numNotifications: 1, + getNotificationsFn, + }); + const service = new NotificationService(api); + + // First tick: 0 -> 1, list returns "first" (baseline), commits. + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 1); + + // Server bumps count but list is still stale. + api.getNumNotifications = async () => 2; + await service.fetchNumNotifications(); + assert.deepEqual( + service.$numNotifications.get(), + 1, + "count should stay 1 while list is stale", + ); + + // Now the list catches up. + topUri = "at://did:example/app.bsky.feed.post/second"; + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 2); + }); - await service.fetchNumNotifications(); + it("retries the list probe when the top URI hasn't changed yet", async () => { + let topUri = "at://did:example/app.bsky.feed.post/first"; + const getNotificationsFn = mock.fn(async () => ({ + notifications: [{ uri: topUri }], + })); + const api = createMockApi({ + numNotifications: 1, + getNotificationsFn, + }); + const service = new NotificationService(api); + + // Establish baseline. + await service.fetchNumNotifications(); + assert.deepEqual(getNotificationsFn.mock.callCount(), 1); + + // Count bumps; make the list return the fresh URI only on the 3rd probe. + api.getNumNotifications = async () => 2; + let probe = 0; + api.getNotifications = async () => { + probe++; + return { + notifications: [ + { + uri: + probe < 3 + ? topUri + : "at://did:example/app.bsky.feed.post/second", + }, + ], + }; + }; + + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 2); + assert.deepEqual(probe, 3); + }); - assert.deepEqual(service.$numNotifications.get(), 7); - }); -}); + it("gives up after the max retries and keeps the old count", async () => { + const api = createMockApi({ + numNotifications: 1, + topUri: "at://did:example/app.bsky.feed.post/first", + }); + const service = new NotificationService(api); + await service.fetchNumNotifications(); + + api.getNumNotifications = async () => 5; + // Top URI never changes. + const probe = mock.fn(async () => ({ + notifications: [{ uri: "at://did:example/app.bsky.feed.post/first" }], + })); + api.getNotifications = probe; + + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 1); + assert.deepEqual(probe.mock.callCount(), 3); + }); + + it("commits decreases immediately without probing the list", async () => { + const api = createMockApi({ + numNotifications: 5, + topUri: "at://did:example/app.bsky.feed.post/first", + }); + const service = new NotificationService(api); + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 5); + + api.getNumNotifications = async () => 2; + const probe = mock.fn(async () => ({ notifications: [] })); + api.getNotifications = probe; + + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 2); + assert.deepEqual(probe.mock.callCount(), 0); + }); -describe("markNotificationsAsRead", () => { - it("should optimistically set count to zero", async () => { - const api = createMockApi({ numNotifications: 5 }); - const service = new NotificationService(api); + it("keeps the old count when the probe throws", async () => { + const api = createMockApi({ + numNotifications: 1, + topUri: "at://did:example/app.bsky.feed.post/first", + }); + const service = new NotificationService(api); + await service.fetchNumNotifications(); + + api.getNumNotifications = async () => 2; + api.getNotifications = async () => { + throw new Error("appview down"); + }; + + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 1); + }); + }); - await service.fetchNumNotifications(); - assert.deepEqual(service.$numNotifications.get(), 5); + describe("$numNotifications", () => { + it("should reflect current notification count", async () => { + const api = createMockApi({ numNotifications: 7 }); + const service = new NotificationService(api); - // Start marking as read (don't await) - const markPromise = service.markNotificationsAsRead(); + assert.deepEqual(service.$numNotifications.get(), 0); - // Count should immediately be zero - assert.deepEqual(service.$numNotifications.get(), 0); + await service.fetchNumNotifications(); - await markPromise; + assert.deepEqual(service.$numNotifications.get(), 7); + }); }); - it("should call api.markNotificationsAsRead", async () => { - const markAsReadFn = mock.fn(); - const api = createMockApi({ - numNotifications: 5, - markAsReadFn, + describe("markNotificationsAsRead", () => { + it("should optimistically set count to zero", async () => { + const api = createMockApi({ numNotifications: 5 }); + const service = new NotificationService(api); + + await service.fetchNumNotifications(); + assert.deepEqual(service.$numNotifications.get(), 5); + + const markPromise = service.markNotificationsAsRead(); + + assert.deepEqual(service.$numNotifications.get(), 0); + + await markPromise; }); - const service = new NotificationService(api); - await service.markNotificationsAsRead(); + it("should call api.markNotificationsAsRead", async () => { + const markAsReadFn = mock.fn(); + const api = createMockApi({ + numNotifications: 5, + markAsReadFn, + }); + const service = new NotificationService(api); + + await service.markNotificationsAsRead(); - assert.deepEqual(markAsReadFn.mock.callCount(), 1); + assert.deepEqual(markAsReadFn.mock.callCount(), 1); + }); }); }); -- 2.51.2