fix: address final review findings (order-notification watcher robustness)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
sdarbinyan
2026-08-15 04:18:20 +04:00
parent 9ccd807a55
commit 1032891d26
8 changed files with 127 additions and 15 deletions

View File

@@ -13,7 +13,7 @@ describe('AdminSettingsPageComponent notification interval', () => {
beforeEach(() => { beforeEach(() => {
watcherStub = { watcherStub = {
intervalMs: signal(15000), intervalMs: signal(15000),
setIntervalSeconds: jasmine.createSpy('setIntervalSeconds'), setIntervalSeconds: jasmine.createSpy('setIntervalSeconds').and.returnValue(true),
}; };
TestBed.configureTestingModule({ TestBed.configureTestingModule({
@@ -39,4 +39,31 @@ describe('AdminSettingsPageComponent notification interval', () => {
expect(watcherStub.setIntervalSeconds).toHaveBeenCalledWith(30); 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);
});
}); });

View File

@@ -31,10 +31,13 @@ export class AdminSettingsPageComponent {
readonly showNotificationIntervalSaved = signal(false); readonly showNotificationIntervalSaved = signal(false);
saveNotificationInterval(): void { saveNotificationInterval(): void {
this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft()); const applied = this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft());
this.notificationIntervalSecondsDraft.set(Math.round(this.orderWatcher.intervalMs() / 1000));
if (applied) {
this.showNotificationIntervalSaved.set(true); this.showNotificationIntervalSaved.set(true);
setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS); setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS);
} }
}
onCompactToggle(compact: boolean): void { onCompactToggle(compact: boolean): void {
this.preferences.setDensity(compact ? 'compact' : 'comfortable'); this.preferences.setDensity(compact ? 'compact' : 'comfortable');

View File

@@ -367,6 +367,8 @@
right: 0; right: 0;
top: calc(100% + 8px); top: calc(100% + 8px);
width: 240px; width: 240px;
max-height: 60vh;
overflow-y: auto;
padding: var(--space-md); padding: var(--space-md);
background: var(--bg-primary); background: var(--bg-primary);
border: 1px solid var(--border-color); border: 1px solid var(--border-color);

View File

@@ -30,6 +30,7 @@ describe('AdminLayoutComponent notifications bell', () => {
recentOrders: ReturnType<typeof signal<AdminOrder[]>>; recentOrders: ReturnType<typeof signal<AdminOrder[]>>;
unreadCount: ReturnType<typeof signal<number>>; unreadCount: ReturnType<typeof signal<number>>;
start: jasmine.Spy; start: jasmine.Spy;
stop: jasmine.Spy;
markAllSeen: jasmine.Spy; markAllSeen: jasmine.Spy;
}; };
@@ -38,6 +39,7 @@ describe('AdminLayoutComponent notifications bell', () => {
recentOrders: signal<AdminOrder[]>([makeOrder('o1', '1001')]), recentOrders: signal<AdminOrder[]>([makeOrder('o1', '1001')]),
unreadCount: signal(1), unreadCount: signal(1),
start: jasmine.createSpy('start'), start: jasmine.createSpy('start'),
stop: jasmine.createSpy('stop'),
markAllSeen: jasmine.createSpy('markAllSeen'), markAllSeen: jasmine.createSpy('markAllSeen'),
}; };
@@ -73,4 +75,11 @@ describe('AdminLayoutComponent notifications bell', () => {
expect(component.notificationsOpen()).toBe(false); expect(component.notificationsOpen()).toBe(false);
expect(watcherStub.markAllSeen).toHaveBeenCalledTimes(1); 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);
});
}); });

View File

@@ -93,6 +93,7 @@ export class AdminLayoutComponent {
constructor() { constructor() {
this.readRouteData(); this.readRouteData();
this.orderWatcher.start(); this.orderWatcher.start();
this.destroyRef.onDestroy(() => this.orderWatcher.stop());
this.router.events this.router.events
.pipe( .pipe(
filter(event => event instanceof NavigationEnd), filter(event => event instanceof NavigationEnd),

View File

@@ -48,7 +48,10 @@ describe('AdminOrderWatcherService', () => {
[makeOrder('o2', '1002', '2026-08-15T10:00:00.000Z'), makeOrder('o1', '1001', '2026-08-15T09:00:00.000Z')], [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({ TestBed.configureTestingModule({
providers: [ providers: [
@@ -110,13 +113,53 @@ describe('AdminOrderWatcherService', () => {
})); }));
it('setIntervalSeconds updates intervalMs and rejects invalid values', () => { it('setIntervalSeconds updates intervalMs and rejects invalid values', () => {
service.setIntervalSeconds(30); expect(service.setIntervalSeconds(30)).toBe(true);
expect(service.intervalMs()).toBe(30000); expect(service.intervalMs()).toBe(30000);
service.setIntervalSeconds(0); expect(service.setIntervalSeconds(0)).toBe(false);
expect(service.intervalMs()).toBe(30000); expect(service.intervalMs()).toBe(30000);
service.setIntervalSeconds(-5); expect(service.setIntervalSeconds(-5)).toBe(false);
expect(service.intervalMs()).toBe(30000); 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);
}));
}); });

View File

@@ -7,6 +7,7 @@ import { LanguageService } from '../../../../services/language.service';
import { TranslateService } from '../../../../i18n/translate.service'; import { TranslateService } from '../../../../i18n/translate.service';
const LAST_NOTIFIED_KEY = 'adminOrderWatcher.lastNotifiedOrderId.v1'; const LAST_NOTIFIED_KEY = 'adminOrderWatcher.lastNotifiedOrderId.v1';
const LAST_NOTIFIED_AT_KEY = 'adminOrderWatcher.lastNotifiedOrderCreatedAt.v1';
const LAST_ACKNOWLEDGED_KEY = 'adminOrderWatcher.lastAcknowledgedOrderId.v1'; const LAST_ACKNOWLEDGED_KEY = 'adminOrderWatcher.lastAcknowledgedOrderId.v1';
const POLL_INTERVAL_KEY = 'adminOrderWatcher.pollIntervalMs.v1'; const POLL_INTERVAL_KEY = 'adminOrderWatcher.pollIntervalMs.v1';
export const DEFAULT_POLL_INTERVAL_MS = 15000; export const DEFAULT_POLL_INTERVAL_MS = 15000;
@@ -44,6 +45,7 @@ export class AdminOrderWatcherService {
readonly intervalMs: Signal<number> = this.intervalMsSignal.asReadonly(); readonly intervalMs: Signal<number> = this.intervalMsSignal.asReadonly();
private lastNotifiedOrderId: string | null = this.storage.getItem(LAST_NOTIFIED_KEY); private lastNotifiedOrderId: string | null = this.storage.getItem(LAST_NOTIFIED_KEY);
private lastNotifiedOrderCreatedAt: string | null = this.storage.getItem(LAST_NOTIFIED_AT_KEY);
private timerId: ReturnType<typeof setInterval> | null = null; private timerId: ReturnType<typeof setInterval> | null = null;
private started = false; private started = false;
@@ -56,9 +58,17 @@ export class AdminOrderWatcherService {
this.scheduleNext(); 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) { if (!Number.isFinite(seconds) || seconds < MIN_POLL_INTERVAL_MS / 1000) {
return; return false;
} }
const ms = Math.round(seconds * 1000); const ms = Math.round(seconds * 1000);
this.intervalMsSignal.set(ms); this.intervalMsSignal.set(ms);
@@ -66,14 +76,16 @@ export class AdminOrderWatcherService {
if (this.started) { if (this.started) {
this.scheduleNext(); this.scheduleNext();
} }
return true;
} }
markAllSeen(): void { markAllSeen(): void {
const newestId = this.recentOrdersSignal()[0]?.id ?? null; const newestId = this.recentOrdersSignal()[0]?.id;
this.lastAcknowledgedOrderIdSignal.set(newestId); if (!newestId) {
if (newestId) { return;
this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId);
} }
this.lastAcknowledgedOrderIdSignal.set(newestId);
this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId);
} }
private scheduleNext(): void { private scheduleNext(): void {
@@ -99,10 +111,21 @@ export class AdminOrderWatcherService {
const isFirstPoll = this.lastNotifiedOrderId === null; const isFirstPoll = this.lastNotifiedOrderId === null;
const notifyIndex = isFirstPoll ? -1 : items.findIndex(order => order.id === this.lastNotifiedOrderId); 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.lastNotifiedOrderId = items[0].id;
this.lastNotifiedOrderCreatedAt = items[0].createdAt;
this.storage.setItem(LAST_NOTIFIED_KEY, this.lastNotifiedOrderId); this.storage.setItem(LAST_NOTIFIED_KEY, this.lastNotifiedOrderId);
this.storage.setItem(LAST_NOTIFIED_AT_KEY, this.lastNotifiedOrderCreatedAt);
if (isFirstPoll) { if (isFirstPoll) {
// Nothing existed to compare against yet - treat current orders as already // Nothing existed to compare against yet - treat current orders as already

View File

@@ -49,6 +49,10 @@
border-color: color-mix(in srgb, var(--primary-color) 45%, white); border-color: color-mix(in srgb, var(--primary-color) 45%, white);
} }
.floating-note-clickable {
cursor: pointer;
}
@keyframes note-in { @keyframes note-in {
from { from {
opacity: 0; opacity: 0;