From 1032891d262365f68831d043136e047d08b38077 Mon Sep 17 00:00:00 2001 From: sdarbinyan Date: Sat, 15 Aug 2026 04:18:20 +0400 Subject: [PATCH] fix: address final review findings (order-notification watcher robustness) Co-Authored-By: Claude Sonnet 5 --- .../admin-settings-page.component.spec.ts | 29 ++++++++++- .../pages/admin-settings-page.component.ts | 9 ++-- .../admin/shell/admin-layout.component.scss | 2 + .../shell/admin-layout.component.spec.ts | 9 ++++ .../admin/shell/admin-layout.component.ts | 1 + .../admin-order-watcher.service.spec.ts | 51 +++++++++++++++++-- .../services/admin-order-watcher.service.ts | 37 +++++++++++--- .../floating-notifications.component.scss | 4 ++ 8 files changed, 127 insertions(+), 15 deletions(-) diff --git a/src/app/features/admin/settings/pages/admin-settings-page.component.spec.ts b/src/app/features/admin/settings/pages/admin-settings-page.component.spec.ts index 32859cb..4865ae9 100644 --- a/src/app/features/admin/settings/pages/admin-settings-page.component.spec.ts +++ b/src/app/features/admin/settings/pages/admin-settings-page.component.spec.ts @@ -13,7 +13,7 @@ describe('AdminSettingsPageComponent notification interval', () => { beforeEach(() => { watcherStub = { intervalMs: signal(15000), - setIntervalSeconds: jasmine.createSpy('setIntervalSeconds'), + setIntervalSeconds: jasmine.createSpy('setIntervalSeconds').and.returnValue(true), }; TestBed.configureTestingModule({ @@ -39,4 +39,31 @@ describe('AdminSettingsPageComponent notification interval', () => { expect(watcherStub.setIntervalSeconds).toHaveBeenCalledWith(30); }); + + it('shows the saved message and keeps the draft when the interval is applied', () => { + watcherStub.setIntervalSeconds.and.callFake((seconds: number) => { + watcherStub.intervalMs.set(seconds * 1000); + return true; + }); + const fixture = TestBed.createComponent(AdminSettingsPageComponent); + const component = fixture.componentInstance; + + component.notificationIntervalSecondsDraft.set(30); + component.saveNotificationInterval(); + + expect(component.showNotificationIntervalSaved()).toBe(true); + expect(component.notificationIntervalSecondsDraft()).toBe(30); + }); + + it('does not show the saved message and reverts the draft when the interval is rejected', () => { + watcherStub.setIntervalSeconds.and.returnValue(false); + const fixture = TestBed.createComponent(AdminSettingsPageComponent); + const component = fixture.componentInstance; + + component.notificationIntervalSecondsDraft.set(0); + component.saveNotificationInterval(); + + expect(component.showNotificationIntervalSaved()).toBe(false); + expect(component.notificationIntervalSecondsDraft()).toBe(15); + }); }); diff --git a/src/app/features/admin/settings/pages/admin-settings-page.component.ts b/src/app/features/admin/settings/pages/admin-settings-page.component.ts index b32a6a4..223b155 100644 --- a/src/app/features/admin/settings/pages/admin-settings-page.component.ts +++ b/src/app/features/admin/settings/pages/admin-settings-page.component.ts @@ -31,9 +31,12 @@ export class AdminSettingsPageComponent { readonly showNotificationIntervalSaved = signal(false); saveNotificationInterval(): void { - this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft()); - this.showNotificationIntervalSaved.set(true); - setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS); + const applied = this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft()); + this.notificationIntervalSecondsDraft.set(Math.round(this.orderWatcher.intervalMs() / 1000)); + if (applied) { + this.showNotificationIntervalSaved.set(true); + setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS); + } } onCompactToggle(compact: boolean): void { diff --git a/src/app/features/admin/shell/admin-layout.component.scss b/src/app/features/admin/shell/admin-layout.component.scss index 1bd2613..16a75e2 100644 --- a/src/app/features/admin/shell/admin-layout.component.scss +++ b/src/app/features/admin/shell/admin-layout.component.scss @@ -367,6 +367,8 @@ right: 0; top: calc(100% + 8px); width: 240px; + max-height: 60vh; + overflow-y: auto; padding: var(--space-md); background: var(--bg-primary); border: 1px solid var(--border-color); diff --git a/src/app/features/admin/shell/admin-layout.component.spec.ts b/src/app/features/admin/shell/admin-layout.component.spec.ts index 3adb993..5423c1a 100644 --- a/src/app/features/admin/shell/admin-layout.component.spec.ts +++ b/src/app/features/admin/shell/admin-layout.component.spec.ts @@ -30,6 +30,7 @@ describe('AdminLayoutComponent notifications bell', () => { recentOrders: ReturnType>; unreadCount: ReturnType>; start: jasmine.Spy; + stop: jasmine.Spy; markAllSeen: jasmine.Spy; }; @@ -38,6 +39,7 @@ describe('AdminLayoutComponent notifications bell', () => { recentOrders: signal([makeOrder('o1', '1001')]), unreadCount: signal(1), start: jasmine.createSpy('start'), + stop: jasmine.createSpy('stop'), markAllSeen: jasmine.createSpy('markAllSeen'), }; @@ -73,4 +75,11 @@ describe('AdminLayoutComponent notifications bell', () => { expect(component.notificationsOpen()).toBe(false); expect(watcherStub.markAllSeen).toHaveBeenCalledTimes(1); }); + + it('stops the watcher when the component is destroyed', () => { + const fixture = TestBed.createComponent(AdminLayoutComponent); + fixture.detectChanges(); + fixture.destroy(); + expect(watcherStub.stop).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/app/features/admin/shell/admin-layout.component.ts b/src/app/features/admin/shell/admin-layout.component.ts index db783e8..c1649f4 100644 --- a/src/app/features/admin/shell/admin-layout.component.ts +++ b/src/app/features/admin/shell/admin-layout.component.ts @@ -93,6 +93,7 @@ export class AdminLayoutComponent { constructor() { this.readRouteData(); this.orderWatcher.start(); + this.destroyRef.onDestroy(() => this.orderWatcher.stop()); this.router.events .pipe( filter(event => event instanceof NavigationEnd), diff --git a/src/app/features/admin/shell/services/admin-order-watcher.service.spec.ts b/src/app/features/admin/shell/services/admin-order-watcher.service.spec.ts index 4873f5c..6220b18 100644 --- a/src/app/features/admin/shell/services/admin-order-watcher.service.spec.ts +++ b/src/app/features/admin/shell/services/admin-order-watcher.service.spec.ts @@ -48,7 +48,10 @@ describe('AdminOrderWatcherService', () => { [makeOrder('o2', '1002', '2026-08-15T10:00:00.000Z'), makeOrder('o1', '1001', '2026-08-15T09:00:00.000Z')], ]; - localStorage.clear(); + localStorage.removeItem('adminOrderWatcher.lastNotifiedOrderId.v1'); + localStorage.removeItem('adminOrderWatcher.lastNotifiedOrderCreatedAt.v1'); + localStorage.removeItem('adminOrderWatcher.lastAcknowledgedOrderId.v1'); + localStorage.removeItem('adminOrderWatcher.pollIntervalMs.v1'); TestBed.configureTestingModule({ providers: [ @@ -110,13 +113,53 @@ describe('AdminOrderWatcherService', () => { })); it('setIntervalSeconds updates intervalMs and rejects invalid values', () => { - service.setIntervalSeconds(30); + expect(service.setIntervalSeconds(30)).toBe(true); expect(service.intervalMs()).toBe(30000); - service.setIntervalSeconds(0); + expect(service.setIntervalSeconds(0)).toBe(false); expect(service.intervalMs()).toBe(30000); - service.setIntervalSeconds(-5); + expect(service.setIntervalSeconds(-5)).toBe(false); expect(service.intervalMs()).toBe(30000); }); + + it('falls back to a timestamp comparison (not the whole page) when the last-notified order id disappears', fakeAsync(() => { + service.start(); + tick(0); + + // Second poll: 'o2' (the last-notified id) is gone. Page contains orders + // both older and newer than o2's createdAt (2026-08-15T10:00:00.000Z). + pollIndex = 1; + ordersByPoll.push([ + makeOrder('o5', '1005', '2026-08-15T12:00:00.000Z'), + makeOrder('o4', '1004', '2026-08-15T11:00:00.000Z'), + makeOrder('o3', '1003', '2026-08-15T10:30:00.000Z'), + makeOrder('o0', '1000', '2026-08-15T08:00:00.000Z'), + ]); + + tick(service.intervalMs()); + + // Only o5, o4, o3 are newer than o2's createdAt - o0 must not toast. + expect(notifications.notifications().length).toBe(3); + const messages = notifications.notifications().map(n => n.message); + expect(messages.some(m => m.includes('1005'))).toBe(true); + expect(messages.some(m => m.includes('1004'))).toBe(true); + expect(messages.some(m => m.includes('1003'))).toBe(true); + expect(messages.some(m => m.includes('1000'))).toBe(false); + })); + + it('stop() clears the interval so no further polls occur', fakeAsync(() => { + const gateway = TestBed.inject(AdminOrdersLocalGateway); + const loadOrdersSpy = spyOn(gateway, 'loadOrders').and.callThrough(); + + service.start(); + tick(0); + const callsAfterStart = loadOrdersSpy.calls.count(); + + tick(service.intervalMs() / 2); + service.stop(); + tick(service.intervalMs() * 2); + + expect(loadOrdersSpy.calls.count()).toBe(callsAfterStart); + })); }); diff --git a/src/app/features/admin/shell/services/admin-order-watcher.service.ts b/src/app/features/admin/shell/services/admin-order-watcher.service.ts index 6a55841..7e5efe0 100644 --- a/src/app/features/admin/shell/services/admin-order-watcher.service.ts +++ b/src/app/features/admin/shell/services/admin-order-watcher.service.ts @@ -7,6 +7,7 @@ import { LanguageService } from '../../../../services/language.service'; import { TranslateService } from '../../../../i18n/translate.service'; const LAST_NOTIFIED_KEY = 'adminOrderWatcher.lastNotifiedOrderId.v1'; +const LAST_NOTIFIED_AT_KEY = 'adminOrderWatcher.lastNotifiedOrderCreatedAt.v1'; const LAST_ACKNOWLEDGED_KEY = 'adminOrderWatcher.lastAcknowledgedOrderId.v1'; const POLL_INTERVAL_KEY = 'adminOrderWatcher.pollIntervalMs.v1'; export const DEFAULT_POLL_INTERVAL_MS = 15000; @@ -44,6 +45,7 @@ export class AdminOrderWatcherService { readonly intervalMs: Signal = this.intervalMsSignal.asReadonly(); private lastNotifiedOrderId: string | null = this.storage.getItem(LAST_NOTIFIED_KEY); + private lastNotifiedOrderCreatedAt: string | null = this.storage.getItem(LAST_NOTIFIED_AT_KEY); private timerId: ReturnType | null = null; private started = false; @@ -56,9 +58,17 @@ export class AdminOrderWatcherService { this.scheduleNext(); } - setIntervalSeconds(seconds: number): void { + stop(): void { + if (this.timerId !== null) { + clearInterval(this.timerId); + this.timerId = null; + } + this.started = false; + } + + setIntervalSeconds(seconds: number): boolean { if (!Number.isFinite(seconds) || seconds < MIN_POLL_INTERVAL_MS / 1000) { - return; + return false; } const ms = Math.round(seconds * 1000); this.intervalMsSignal.set(ms); @@ -66,14 +76,16 @@ export class AdminOrderWatcherService { if (this.started) { this.scheduleNext(); } + return true; } markAllSeen(): void { - const newestId = this.recentOrdersSignal()[0]?.id ?? null; - this.lastAcknowledgedOrderIdSignal.set(newestId); - if (newestId) { - this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId); + const newestId = this.recentOrdersSignal()[0]?.id; + if (!newestId) { + return; } + this.lastAcknowledgedOrderIdSignal.set(newestId); + this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId); } private scheduleNext(): void { @@ -99,10 +111,21 @@ export class AdminOrderWatcherService { const isFirstPoll = this.lastNotifiedOrderId === null; const notifyIndex = isFirstPoll ? -1 : items.findIndex(order => order.id === this.lastNotifiedOrderId); - const newOrders = isFirstPoll ? [] : (notifyIndex === -1 ? items : items.slice(0, notifyIndex)); + // When the last-notified order id is no longer present in the current page + // (deleted, or more than RECENT_ORDERS_LIMIT orders arrived since the last + // poll), fall back to a timestamp comparison instead of treating the whole + // page as new - otherwise a single missing id could fire a burst of up to + // RECENT_ORDERS_LIMIT toasts. + const newOrders = isFirstPoll + ? [] + : notifyIndex !== -1 + ? items.slice(0, notifyIndex) + : items.filter(order => this.lastNotifiedOrderCreatedAt !== null && order.createdAt > this.lastNotifiedOrderCreatedAt); this.lastNotifiedOrderId = items[0].id; + this.lastNotifiedOrderCreatedAt = items[0].createdAt; this.storage.setItem(LAST_NOTIFIED_KEY, this.lastNotifiedOrderId); + this.storage.setItem(LAST_NOTIFIED_AT_KEY, this.lastNotifiedOrderCreatedAt); if (isFirstPoll) { // Nothing existed to compare against yet - treat current orders as already diff --git a/src/app/features/website/user-experience/components/floating-notifications/floating-notifications.component.scss b/src/app/features/website/user-experience/components/floating-notifications/floating-notifications.component.scss index 124d69d..02c1a5d 100644 --- a/src/app/features/website/user-experience/components/floating-notifications/floating-notifications.component.scss +++ b/src/app/features/website/user-experience/components/floating-notifications/floating-notifications.component.scss @@ -49,6 +49,10 @@ border-color: color-mix(in srgb, var(--primary-color) 45%, white); } +.floating-note-clickable { + cursor: pointer; +} + @keyframes note-in { from { opacity: 0;