From bc323491b7432a627f1199768075c8e3fc4de247 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 6 Sep 2026 14:20:56 -0600 Subject: [PATCH] feat(announcements): open the full announcement from the banner text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The banner pill renders no body, so its only affordances were a ✕ and an optional external CTA — which trains people to dismiss unread, with What's New (buried in the user menu) as the only other route to the content. The text is now a button that opens the announcement's own dialog, and the dialog owns the ack from there: any of its exits writes `dismissed` (or `acknowledged`) and retires the pill durably. Having read the body is a stronger signal of consumption than clicking ✕ on a one-line strip, and leaving the pill up afterwards would only ask the user to dismiss something they have already dealt with. It opens the single-announcement dialog rather than the What's New list deliberately: the pill named one thing, and handing back a list to search through is a worse answer than the thing itself. Three supporting changes: - `AnnouncementModalData.sourceSurface` (default `modal`) attributes every ack to where the gesture happened, so reach stats can tell a banner that earned a read from an interruption nobody asked for. No backend change: `AnnouncementSurface` is a plain literal and the ack route never cross-checks it against the announcement's own `surfaces`. - `AnnouncementModalService.openFor()` is the one entry point for a user-initiated open. It skips the §D8 turn-safety gate — a click is not an interruption — but still respects `openRef`, and marks the item `shown` so the auto-modal cannot re-interrupt with something already read. - A `requiresAck` announcement gets no ✕ on the pill. Suppression is rank-based and covers the banner *and* modal slots alike, so a ✕ there let a user retire a compliance notice before the blocking modal ever fired, leaving no `acknowledged` record anywhere. The text button uses an `sr-only` prefix rather than an `aria-label`: the visible line may be the summary, so a title-derived label would leave the visible words outside the accessible name (WCAG 2.5.3, Label in Name). Co-Authored-By: Claude Opus 5 --- docs/specs/feature-announcements.md | 24 ++++ .../announcement-banner.component.spec.ts | 109 +++++++++++++++++- .../announcement-banner.component.ts | 71 ++++++++++-- .../announcement-modal.component.spec.ts | 42 ++++++- .../announcement-modal.component.ts | 25 +++- .../announcement-modal.service.spec.ts | 58 ++++++++++ .../announcement-modal.service.ts | 63 +++++++--- 7 files changed, 354 insertions(+), 38 deletions(-) diff --git a/docs/specs/feature-announcements.md b/docs/specs/feature-announcements.md index bd199615..716fb9b9 100644 --- a/docs/specs/feature-announcements.md +++ b/docs/specs/feature-announcements.md @@ -88,6 +88,25 @@ An announcement carries `surfaces: list[Literal["panel", "banner", "modal"]]`. rendered by a sibling of `quota-warning-banner`. One line plus an optional CTA and a ✕. + **Its text is a button that opens the announcement's own dialog.** The pill + renders no body, so without a way in, its only affordances are ✕ and an + optional external CTA — which trains people to dismiss unread, with What's + New (buried in the user menu) as the only other route to the content. It + opens the single-announcement dialog rather than the What's New list + deliberately: the pill named one thing, and handing back a list to search + through is a worse answer than the thing itself. The dialog owns the ack + from that point (`sourceSurface: "banner"`, so reach stats can tell a + banner-driven read from an interruption), and any of its exits retires the + pill durably — having read the body is a stronger signal of consumption + than clicking ✕ on a one-line strip. + + **A `requiresAck` announcement gets no ✕ on the pill.** Dismissal + suppression is rank-based and covers the banner *and* modal slots alike + (§D7), so a ✕ here would let a user retire a compliance notice from the + strip before the blocking modal ever fired — leaving no `acknowledged` + record anywhere. On those, the only way out is to open it and press the + button. + *Revised after PR-4 shipped.* It was first built as a full-bleed strip below the top nav. Two things moved it. Dismissing a strip that occupied layout reflowed the whole view, so it became an overlay; and what a banner @@ -128,6 +147,11 @@ bust. **Announcements never touch the model call path** (D12). | `dismissed` | User clicks ✕ or "Got it" | Suppresses banner and modal. Entry stays in the panel. | | `acknowledged` | User clicks the confirm button on a `requiresAck` modal | As `dismissed`, plus it is a durable record an admin can report on. | +The ack's `surface` records **where the gesture happened**, not which surface +owns the announcement: a dialog the user opened by clicking the banner's text +writes `banner` for every action it records, so reach stats can distinguish a +banner that earned a read from a modal the user never asked for. + They are ranked (`seen=1 < dismissed=2 < acknowledged=3`) and the stored rank **only ever increases**. The write is a conditional `UpdateExpression`: diff --git a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts index 5c5872e0..254faeb1 100644 --- a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts +++ b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { TestBed } from '@angular/core/testing'; import { signal } from '@angular/core'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; +import { AnnouncementModalService } from '../../services/announcements/announcement-modal.service'; import { Announcement } from '../../services/announcements/announcement.model'; import { AnnouncementBannerComponent } from './announcement-banner.component'; @@ -28,16 +29,19 @@ function makeAnnouncement(overrides: Partial = {}): Announcement { describe('AnnouncementBannerComponent', () => { let bannerItem: ReturnType>; let ack: ReturnType; + let openFor: ReturnType; beforeEach(() => { TestBed.resetTestingModule(); bannerItem = signal(null); ack = vi.fn(async () => true); + openFor = vi.fn(); // DI-token override rather than vi.mock, per house convention. TestBed.configureTestingModule({ providers: [ { provide: AnnouncementsService, useValue: { bannerItem, ack } }, + { provide: AnnouncementModalService, useValue: { openFor } }, ], }); }); @@ -127,12 +131,11 @@ describe('AnnouncementBannerComponent', () => { const fixture = create(); ack.mockClear(); + // By label, not "the first button" — the text is a button too now. const dismiss = (fixture.nativeElement as HTMLElement).querySelector( - 'button', + 'button[aria-label="Dismiss announcement: Skills are here"]', ) as HTMLButtonElement; - expect(dismiss.getAttribute('aria-label')).toBe( - 'Dismiss announcement: Skills are here', - ); + expect(dismiss).not.toBeNull(); dismiss.click(); expect(ack).toHaveBeenCalledWith('a1', 'dismissed', 'banner'); @@ -173,6 +176,7 @@ describe('AnnouncementBannerComponent', () => { TestBed.configureTestingModule({ providers: [ { provide: AnnouncementsService, useValue: { bannerItem, ack } }, + { provide: AnnouncementModalService, useValue: { openFor } }, ], }); bannerItem.set( @@ -184,6 +188,103 @@ describe('AnnouncementBannerComponent', () => { expect(link.getAttribute('rel')).toBe('noopener noreferrer'); }); + describe('the text opens the full announcement', () => { + function readMore(fixture: ReturnType) { + // By `title`, which carries the untruncated headline — the ✕ is the one + // with an aria-label. + return (fixture.nativeElement as HTMLElement).querySelector( + 'button[title="Skills are here"]', + ) as HTMLButtonElement | null; + } + + it('hands the announcement to the modal service, attributed to the banner', () => { + // The pill renders no body, so without this the only affordances on it + // are ✕ and an optional CTA — which trains people to dismiss unread. + bannerItem.set(makeAnnouncement()); + const fixture = create(); + + readMore(fixture)!.click(); + + expect(openFor).toHaveBeenCalledWith( + expect.objectContaining({ announcement_id: 'a1' }), + 'banner', + ); + }); + + it('writes no ack of its own — the dialog owns that', () => { + // Acking `dismissed` here as well would be a second, racing write for + // the same gesture, and would retire the pill before the user has read + // a word of the body. + bannerItem.set(makeAnnouncement()); + const fixture = create(); + ack.mockClear(); + + readMore(fixture)!.click(); + + expect(ack).not.toHaveBeenCalled(); + }); + + it('keeps the visible text inside the accessible name (WCAG 2.5.3)', () => { + // `bannerText()` may be the summary, so an aria-label built from the + // title would leave a voice-control user saying a phrase that is not + // the button's name. The visible words have to survive. + bannerItem.set(makeAnnouncement({ summary: 'Short version' })); + const fixture = create(); + const button = readMore(fixture)!; + + expect(button.getAttribute('aria-label')).toBeNull(); + expect(button.textContent).toContain('Short version'); + expect(button.textContent).toContain('Read more'); + // The untruncated headline is still reachable on hover. + expect(button.getAttribute('title')).toBe('Skills are here'); + }); + }); + + describe('a requiresAck announcement cannot be retired from the strip', () => { + it('offers no ✕', () => { + // `dismissed` and `acknowledged` both sit at or above SUPPRESSING_RANK, + // and suppression covers the banner AND modal slots — so a ✕ here would + // let a user kill a compliance notice before the blocking modal ever + // fired, leaving no `acknowledged` record anywhere. + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + + expect( + (fixture.nativeElement as HTMLElement).querySelector( + 'button[aria-label^="Dismiss announcement"]', + ), + ).toBeNull(); + }); + + it('still opens on click — that is the only way out', () => { + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + + ( + (fixture.nativeElement as HTMLElement).querySelector( + 'button[title="Skills are here"]', + ) as HTMLButtonElement + ).click(); + + expect(openFor).toHaveBeenCalledWith( + expect.objectContaining({ requires_ack: true }), + 'banner', + ); + }); + + it('writes no `dismissed` even if onDismiss is reached some other way', () => { + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + ack.mockClear(); + + ( + fixture.componentInstance as unknown as { onDismiss(): void } + ).onDismiss(); + + expect(ack).not.toHaveBeenCalled(); + }); + }); + describe('overlays rather than occupying space', () => { it('positions the host absolutely, so dismissing it cannot reflow the page', () => { // The regression: the banner used to be a flex child of the shell's diff --git a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts index e2882259..3620d110 100644 --- a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts +++ b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts @@ -14,6 +14,7 @@ import { heroXMark, } from '@ng-icons/heroicons/outline'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; +import { AnnouncementModalService } from '../../services/announcements/announcement-modal.service'; import { Announcement, AnnouncementSeverity, @@ -80,6 +81,22 @@ import { * Body markdown is deliberately *not* rendered here: this surface is one line. * The full body lives in What's New, which is why `panel` is forced onto every * announcement server-side. + * + * **The text is a button, and clicking it opens the full announcement.** + * Without it the only affordances on the pill are ✕ and an optional CTA, which + * trains people to dismiss unread — and What's New, the only other way to the + * body, is buried in the user menu. It opens the single-announcement dialog + * rather than the What's New list on purpose: the pill named one thing, so + * handing back a list to search is a worse answer than the thing itself. From + * there the dialog owns the ack, and any of its exits retires the pill + * durably (see `onOpenDetail`). + * + * **A `requiresAck` announcement gets no ✕ here.** `dismissed` and + * `acknowledged` are both at or above `SUPPRESSING_RANK`, and suppression + * applies to the banner *and* modal slots alike — so with a ✕ on the strip, a + * user could retire a compliance notice from the pill and the blocking modal + * would never fire, leaving no `acknowledged` record anywhere. On those, the + * only way out is to open it and press the button. */ @Component({ selector: 'app-announcement-banner', @@ -117,9 +134,19 @@ import { aria-hidden="true" /> -

- {{ bannerText() }} -

+ @if (item.cta_url && item.cta_label) { } - + @if (!item.requires_ack) { + + } } `, }) export class AnnouncementBannerComponent { private readonly announcements = inject(AnnouncementsService); + private readonly modals = inject(AnnouncementModalService); /** * Which side of the composer to take. `'above'` suits a bottom-pinned @@ -221,7 +251,24 @@ export class AnnouncementBannerComponent { protected onDismiss(): void { const item = this.announcement(); - if (!item) return; + if (!item || item.requires_ack) return; void this.announcements.ack(item.announcement_id, 'dismissed', 'banner'); } + + /** + * Open the full announcement, attributing whatever the user does there to + * the banner. + * + * The dialog owns the ack from here: any of its exits writes `dismissed` + * (or `acknowledged`), which outranks the `seen` this strip already wrote + * and retires the pill on every device. That is the intent — having read + * the body is a stronger signal of consumption than clicking ✕ on a + * one-line strip, and leaving the pill up afterwards would just ask the + * user to dismiss something they have already dealt with. + */ + protected onOpenDetail(): void { + const item = this.announcement(); + if (!item) return; + this.modals.openFor(item, 'banner'); + } } diff --git a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts index 00db445b..3c1e562f 100644 --- a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts +++ b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts @@ -3,7 +3,10 @@ import { TestBed } from '@angular/core/testing'; import { DIALOG_DATA, DialogRef } from '@angular/cdk/dialog'; import { provideMarkdown } from 'ngx-markdown'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; -import { Announcement } from '../../services/announcements/announcement.model'; +import { + Announcement, + AnnouncementSurface, +} from '../../services/announcements/announcement.model'; import { AnnouncementModalComponent, AnnouncementModalData, @@ -33,7 +36,10 @@ describe('AnnouncementModalComponent', () => { let ack: ReturnType; let close: ReturnType; - function setup(announcement: Announcement) { + function setup( + announcement: Announcement, + sourceSurface?: AnnouncementSurface, + ) { TestBed.resetTestingModule(); ack = vi.fn(async () => true); close = vi.fn(); @@ -45,7 +51,10 @@ describe('AnnouncementModalComponent', () => { { provide: DialogRef, useValue: { close, closed: { subscribe: vi.fn() } } }, { provide: DIALOG_DATA, - useValue: { announcement } satisfies AnnouncementModalData, + useValue: { + announcement, + sourceSurface, + } satisfies AnnouncementModalData, }, ], }); @@ -172,6 +181,33 @@ describe('AnnouncementModalComponent', () => { expect(link.getAttribute('rel')).toBe('noopener noreferrer'); }); + describe('ack attribution', () => { + it('defaults to the `modal` surface — the §D8 interruption', () => { + setup(makeAnnouncement()); + expect(ack).toHaveBeenCalledWith('a1', 'seen', 'modal'); + }); + + it('attributes every ack to the surface the user came from', () => { + // Opened by clicking the banner text. The ack row's `surface` is the + // only record of what drove the dismissal, so it must say `banner` — + // otherwise banner engagement is indistinguishable from an interruption + // the user never asked for. + const fixture = setup(makeAnnouncement(), 'banner'); + expect(ack).toHaveBeenCalledWith('a1', 'seen', 'banner'); + + ack.mockClear(); + confirmButton(fixture).click(); + expect(ack).toHaveBeenCalledWith('a1', 'dismissed', 'banner'); + }); + + it('carries the surface through an `acknowledged` too', () => { + const fixture = setup(makeAnnouncement({ requires_ack: true }), 'banner'); + ack.mockClear(); + confirmButton(fixture).click(); + expect(ack).toHaveBeenCalledWith('a1', 'acknowledged', 'banner'); + }); + }); + it('is a labelled modal dialog', () => { const fixture = setup(makeAnnouncement()); const panel = el(fixture).querySelector('[role="dialog"]')!; diff --git a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts index b93ab17c..8202bc39 100644 --- a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts +++ b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts @@ -10,10 +10,24 @@ import { NgIcon, provideIcons } from '@ng-icons/core'; import { heroXMark } from '@ng-icons/heroicons/outline'; import { DialogDismissDirective } from '../dialog/dialog-dismiss.directive'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; -import { Announcement } from '../../services/announcements/announcement.model'; +import { + Announcement, + AnnouncementSurface, +} from '../../services/announcements/announcement.model'; export interface AnnouncementModalData { announcement: Announcement; + /** + * Which surface the user came from, for ack attribution. + * + * Defaults to `'modal'` — the §D8 interruption, where the dialog *is* the + * surface. The banner passes `'banner'` when the user clicks its text to + * read the body, because the ack row's `surface` is the only record of what + * actually drove the dismissal, and "the banner earned a read" is the one + * number this interaction exists to produce. The funnel counters key on + * action alone, so attribution never distorts them. + */ + sourceSurface?: AnnouncementSurface; } /** @@ -147,6 +161,9 @@ export class AnnouncementModalComponent { protected readonly titleId = `announcement-modal-title-${crypto.randomUUID()}`; protected readonly announcement = this.data.announcement; + /** Where every ack from this dialog is attributed. See `AnnouncementModalData`. */ + private readonly surface: AnnouncementSurface = this.data.sourceSurface ?? 'modal'; + protected readonly requiresAck = computed( () => this.announcement.requires_ack, ); @@ -168,7 +185,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, 'seen', - 'modal', + this.surface, ); } @@ -177,7 +194,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, this.requiresAck() ? 'acknowledged' : 'dismissed', - 'modal', + this.surface, ); this.dialogRef.close(); } @@ -187,7 +204,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, 'dismissed', - 'modal', + this.surface, ); this.dialogRef.close(); } diff --git a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts index 0784bf83..da958a6e 100644 --- a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts +++ b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts @@ -233,6 +233,64 @@ describe('AnnouncementModalService (§D8 turn-safety gate)', () => { expect(open).not.toHaveBeenCalled(); }); + describe('openFor — the user asked for it', () => { + it('opens even when every §D8 gate would refuse an interruption', () => { + // A click is not an interruption. The gate exists to stop us throwing a + // dialog at someone mid-thought; here the user is the one asking. + isLoadingSession.set('session-1'); + toolApprovalPending.set(true); + focusComposer('half a thought'); + const service = start(); + + service.openFor(makeAnnouncement(), 'banner'); + + expect(open).toHaveBeenCalledTimes(1); + const [, config] = open.mock.calls[0]; + expect(config.data.announcement.announcement_id).toBe('a1'); + expect(config.data.sourceSurface).toBe('banner'); + }); + + it('defaults the surface to `modal` when no source is given', () => { + const service = start(); + service.openFor(makeAnnouncement()); + expect(open.mock.calls[0][1].data.sourceSurface).toBe('modal'); + }); + + it('keeps disableClose on a requiresAck announcement', () => { + // Opening it yourself is not a way around the acknowledgement. + const service = start(); + service.openFor(makeAnnouncement({ requires_ack: true }), 'banner'); + expect(open.mock.calls[0][1].disableClose).toBe(true); + }); + + it('refuses to stack a second dialog on an open one', () => { + const service = start(); + service.openFor(makeAnnouncement(), 'banner'); + service.openFor(makeAnnouncement({ announcement_id: 'a2' }), 'banner'); + expect(open).toHaveBeenCalledTimes(1); + }); + + it('stops the §D8 effect re-opening what the user already read', () => { + // Same announcement in both slots — without marking it shown, the user + // would read it from the banner and then be interrupted by it on the + // next navigation. + const service = start(); + service.openFor(makeAnnouncement(), 'banner'); + expect(open).toHaveBeenCalledTimes(1); + + // Let the dialog close, so `openRef` is not what is holding it back. + const onClosed = open.mock.results[0].value.closed.subscribe.mock + .calls[0][0] as () => void; + onClosed(); + + modalItem.set(makeAnnouncement()); + TestBed.tick(); + navigate(); + + expect(open).toHaveBeenCalledTimes(1); + }); + }); + it('opens on the next settled navigation after a failed gate', () => { isLoadingSession.set('session-1'); start(); diff --git a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts index 082c3eb7..f0365c5f 100644 --- a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts +++ b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts @@ -9,6 +9,7 @@ import { AnnouncementModalComponent, AnnouncementModalData, } from '../../components/announcement-modal/announcement-modal.component'; +import { Announcement, AnnouncementSurface } from './announcement.model'; import { SessionService } from '../../auth/session.service'; import { MessageMapService } from '../../session/services/session/message-map.service'; import { ToolApprovalService } from '../tool-approval/tool-approval.service'; @@ -95,25 +96,57 @@ export class AnnouncementModalService { untracked(() => { if (this.shown.has(item.announcement_id)) return; if (!this.canInterrupt()) return; - - this.shown.add(item.announcement_id); - this.openRef = this.dialog.open( - AnnouncementModalComponent, - { - data: { announcement: item }, - hasBackdrop: false, // the dialog component owns its own backdrop - // The only exit from a `requiresAck` announcement is its button. - disableClose: item.requires_ack, - panelClass: 'announcement-modal', - }, - ); - this.openRef.closed.subscribe(() => { - this.openRef = null; - }); + this.open(item, 'modal'); }); }); } + /** + * Open the detail dialog for one announcement **because the user asked**. + * + * The banner calls this when its text is clicked: the pill is one line and + * renders no body, so without a way in, the only affordances on it are ✕ and + * an optional CTA — which is how you train people to dismiss unread. + * + * Two things it deliberately does *not* do. It does not consult + * `canInterrupt()`: that gate exists to stop us throwing a dialog at someone + * mid-thought, and a click is not an interruption — the user is the one + * asking. It *does* respect `openRef`, because two stacked dialogs is a bug + * whoever opened them. + * + * It also marks the announcement `shown`, so the §D8 effect cannot re-open + * the same item as an interruption after the user has already read it here. + */ + openFor( + announcement: Announcement, + sourceSurface: AnnouncementSurface = 'modal', + ): void { + if (this.openRef !== null) return; + this.open(announcement, sourceSurface); + } + + /** The single place a dialog is constructed, so `openRef` cannot drift. */ + private open( + announcement: Announcement, + sourceSurface: AnnouncementSurface, + ): void { + this.shown.add(announcement.announcement_id); + this.openRef = this.dialog.open( + AnnouncementModalComponent, + { + data: { announcement, sourceSurface }, + hasBackdrop: false, // the dialog component owns its own backdrop + // The only exit from a `requiresAck` announcement is its button — + // including when the user opened it themselves from the banner. + disableClose: announcement.requires_ack, + panelClass: 'announcement-modal', + }, + ); + this.openRef.closed.subscribe(() => { + this.openRef = null; + }); + } + /** * The §D8 gate. Every read here is a snapshot — see the class comment for * why this must not be reactive.