diff --git a/frontend/ai.client/src/app/services/announcements/announcements.service.spec.ts b/frontend/ai.client/src/app/services/announcements/announcements.service.spec.ts index e36d84447..fe506dfe1 100644 --- a/frontend/ai.client/src/app/services/announcements/announcements.service.spec.ts +++ b/frontend/ai.client/src/app/services/announcements/announcements.service.spec.ts @@ -6,7 +6,8 @@ import { } from '@angular/common/http/testing'; import { provideHttpClient } from '@angular/common/http'; import { signal } from '@angular/core'; -import { AnnouncementsService } from './announcements.service'; +import { ACK_RETRY_DELAY_MS, AnnouncementsService } from './announcements.service'; +import { SUPPRESS_ERROR_TOAST } from '../../auth/error.interceptor'; import { Announcement, AnnouncementFeed } from './announcement.model'; import { ConfigService } from '../config.service'; @@ -124,6 +125,16 @@ describe('AnnouncementsService', () => { expect(service.unreadCount()).toBe(0); expect(service.hasUnread()).toBe(false); }); + + it('opts out of the global error toast', async () => { + service.feedResource.reload(); + service.panelItems(); + await vi.waitFor(() => { + const req = httpMock.expectOne(FEED_URL); + expect(req.request.context.get(SUPPRESS_ERROR_TOAST)).toBe(true); + req.flush(makeFeed()); + }); + }); }); describe('unread count', () => { @@ -229,6 +240,87 @@ describe('AnnouncementsService', () => { await expect(done).resolves.toBe(false); }); + it('opts out of the global error toast', async () => { + // A status-0 network drop on mobile surfaced as "An error occurred" + // even though the failure is swallowed here by design (§D7). + await loadFeed(); + + const done = service.ack('a1', 'acknowledged', 'modal'); + await vi.waitFor(() => { + const req = httpMock.expectOne(`${API}/announcements/a1/ack`); + expect(req.request.context.get(SUPPRESS_ERROR_TOAST)).toBe(true); + req.flush('nope', { status: 500, statusText: 'Server Error' }); + }); + + expect(await done).toBe(false); + }); + + describe('retry', () => { + const ACK_URL = `${API}/announcements/a1/ack`; + const networkDrop = (req: ReturnType) => + req.error(new ProgressEvent('error'), { status: 0, statusText: 'Unknown Error' }); + + afterEach(() => vi.useRealTimers()); + + it('retries once after a network drop and reports success', async () => { + await loadFeed(); + vi.useFakeTimers(); + + const done = service.ack('a1', 'acknowledged', 'modal'); + networkDrop(httpMock.expectOne(ACK_URL)); + + // Nothing is resent until the delay has elapsed. + httpMock.expectNone(ACK_URL); + await vi.advanceTimersByTimeAsync(ACK_RETRY_DELAY_MS); + + const retried = httpMock.expectOne(ACK_URL); + expect(retried.request.body).toEqual({ action: 'acknowledged', surface: 'modal' }); + retried.flush(null); + + expect(await done).toBe(true); + }); + + it('retries a gateway 503', async () => { + await loadFeed(); + vi.useFakeTimers(); + + const done = service.ack('a1', 'acknowledged', 'modal'); + httpMock + .expectOne(ACK_URL) + .flush('nope', { status: 503, statusText: 'Service Unavailable' }); + await vi.advanceTimersByTimeAsync(ACK_RETRY_DELAY_MS); + httpMock.expectOne(ACK_URL).flush(null); + + expect(await done).toBe(true); + }); + + it('gives up after the one retry', async () => { + await loadFeed(); + vi.useFakeTimers(); + + const done = service.ack('a1', 'acknowledged', 'modal'); + networkDrop(httpMock.expectOne(ACK_URL)); + await vi.advanceTimersByTimeAsync(ACK_RETRY_DELAY_MS); + networkDrop(httpMock.expectOne(ACK_URL)); + await vi.advanceTimersByTimeAsync(ACK_RETRY_DELAY_MS * 5); + + httpMock.expectNone(ACK_URL); + expect(await done).toBe(false); + }); + + it('does not retry a 4xx', async () => { + await loadFeed(); + vi.useFakeTimers(); + + const done = service.ack('a1', 'acknowledged', 'modal'); + httpMock.expectOne(ACK_URL).flush('nope', { status: 404, statusText: 'Not Found' }); + await vi.advanceTimersByTimeAsync(ACK_RETRY_DELAY_MS * 5); + + httpMock.expectNone(ACK_URL); + expect(await done).toBe(false); + }); + }); + it('a local dismissal hides the modal too', async () => { await loadFeed( makeFeed({ diff --git a/frontend/ai.client/src/app/services/announcements/announcements.service.ts b/frontend/ai.client/src/app/services/announcements/announcements.service.ts index 1499cee85..f856cc8f8 100644 --- a/frontend/ai.client/src/app/services/announcements/announcements.service.ts +++ b/frontend/ai.client/src/app/services/announcements/announcements.service.ts @@ -1,6 +1,7 @@ import { Injectable, computed, inject, resource, signal } from '@angular/core'; -import { HttpClient } from '@angular/common/http'; -import { firstValueFrom } from 'rxjs'; +import { HttpClient, HttpContext, HttpErrorResponse } from '@angular/common/http'; +import { firstValueFrom, retry, throwError, timer } from 'rxjs'; +import { SUPPRESS_ERROR_TOAST } from '../../auth/error.interceptor'; import { ConfigService } from '../config.service'; import { Announcement, @@ -10,6 +11,22 @@ import { AnnouncementSurface, } from './announcement.model'; +/** How long to wait before the single ack retry. */ +export const ACK_RETRY_DELAY_MS = 1000; + +/** + * Failures worth one more attempt: the request never landed (status 0 — a + * mobile network blip, a backgrounded tab) or a gateway in front of app-api + * gave up. A 4xx is the server's considered answer and retrying won't change + * it. + */ +function isTransient(error: unknown): boolean { + return ( + error instanceof HttpErrorResponse && + (error.status === 0 || error.status === 502 || error.status === 503 || error.status === 504) + ); +} + const EMPTY_FEED: AnnouncementFeed = { panel: [], banner: null, @@ -46,6 +63,16 @@ export class AnnouncementsService { () => `${this.config.appApiUrl()}/announcements`, ); + /** + * Every request here fails open and swallows its own error, so the global + * error toast must not fire either — otherwise a dropped ack (status 0 on a + * flaky mobile connection, a transient 5xx) still puts "An error occurred" + * in front of the user for something they cannot act on. + */ + private readonly options = { + context: new HttpContext().set(SUPPRESS_ERROR_TOAST, true), + }; + /** * Loads on first read. The topnav only renders the user dropdown once the * session bootstrap has resolved, so the loader fires post-auth with the @@ -95,7 +122,12 @@ export class AnnouncementsService { * * Fails open (§D7): a rejected POST still hides the item locally and * resolves rather than throwing, so no caller has to remember to catch. - * Returns whether the server accepted it, for tests and for a future retry. + * Returns whether the server accepted it. + * + * A transient failure is retried once. The endpoint is idempotent and + * monotonic (§D2), so a duplicate that did land server-side is a no-op — + * and without the retry, a dropped `acknowledged` resurfaces the modal on + * the next load. */ async ack( announcementId: string, @@ -110,7 +142,17 @@ export class AnnouncementsService { const body: AnnouncementAckRequest = { action, surface }; try { await firstValueFrom( - this.http.post(`${this.baseUrl()}/${announcementId}/ack`, body), + this.http.post( + `${this.baseUrl()}/${announcementId}/ack`, + body, + this.options, + ).pipe( + retry({ + count: 1, + delay: error => + isTransient(error) ? timer(ACK_RETRY_DELAY_MS) : throwError(() => error), + }), + ), ); return true; } catch { @@ -145,7 +187,7 @@ export class AnnouncementsService { private async fetchFeed(): Promise { try { return await firstValueFrom( - this.http.get(`${this.baseUrl()}/`), + this.http.get(`${this.baseUrl()}/`, this.options), ); } catch { // The surface is kill-switched off (404) or the backend is unhappy.