Skip to content

feat(announcements): user feed, acknowledgements, and What's New panel (PR-2) - #969

Merged
philmerrell merged 1 commit into
developfrom
feature/announcements-user-surface
Sep 5, 2026
Merged

philmerrell merged 1 commit into
developfrom
feature/announcements-user-surface

Conversation

@philmerrell

Copy link
Copy Markdown
Contributor

PR-2 of docs/specs/feature-announcements.md, following #966. This is the durable surface: users can see and acknowledge announcements, and nothing interrupts anyone.

The banner (PR-4) and modal (PR-5) are still unbuilt, but GET /announcements already returns their slots — so those PRs are pure frontend work against a contract that exists today.

What's here

Backend

  • visibility.py — the entire filter chain as one pure function. No DynamoDB, no FastAPI, no clock of its own. The server computes visibility and the client renders what it is handed (§D5), so the rules live in one language instead of drifting across two — and they're table-testable without moto.
  • GET /announcements → panel list + capped banner/modal slots + unread count. POST /announcements/{id}/ack → 204. Cookie session auth on both, per CLAUDE.md's app_api rule.
  • UserAnnouncement is deliberately not a subset alias of the admin model. It omits state, targetRoles, showToNewUsers and createdBy, so adding an admin field later can't widen the user payload by accident.

Frontend

  • announcements.service.ts — signal-based, loads on first read from the user dropdown (which the topnav renders only after the session resolves, so the request carries the roles the server needs for targeting).
  • What's-New panel in the user menu, with the unread dot on the avatar and a count badge on the row.

Three things worth a close read

1. I diverged from the spec's filter chain, on purpose. §D5 lists the ack check as step 5, before the caps. But D1 and D2 are explicit that dismissing a loud surface leaves the entry in the panel — so applied literally, step 5 deletes the durable record the whole design is built around. Eligibility (steps 1–4) produces the panel; ack suppression applies only when choosing the banner and the modal. Commented at the call site and pinned by a test.

2. Browser verification caught a bug that every unit test passed. The panel's markdown rendered lists without bullets and tables unstyled. The cause: the prose classes copied from user-menu-link-modal are inert — the Tailwind typography plugin isn't installed — so Tailwind's preflight list-style: none wins. The app's real markdown stylesheet is scoped under .message-block in styles.css. Switching to it fixed lists, tables, code and links at once, and it's what §D10 asks for anyway ("styling matches assistant messages").

⚠️ user-menu-link-modal has this same latent bug today. Any admin-authored modal link containing a list or table renders unstyled. Left alone here as out of scope, but it's a one-line fix worth doing.

3. Fail-open dismissal (§D7). A rejected ack still hides the item for the tab session and resolves rather than throwing, so no caller has to remember to catch. A user trapped under an undismissable banner by a transient 500 is a worse outcome than one that reappears tomorrow. localStorage is not used at all — server state is the truth (§D3).

Also: the ack endpoint returns 404, not 403, for an id this user can't see. 403 would confirm that an announcement targeted at another role exists.

Verification

Beyond the unit tests, I ran this against real dev data on a local stack (app-api on :8010, SPA on :4300, so nothing touched the running services on :4200/:8000) and confirmed the full loop:

  • New / Updated / already-read pills all render correctly side by side.
  • Revision bump lapses suppression — acked at R1, bumped to R2, the entry returns as Updated rather than plain New, exactly as §D4 intends.
  • Acks persist: opening the panel cleared the dot, wrote the rows, and unread_count was 0 on the next fetch.
  • Markdown (ordered lists, nested bullets, tables, inline code, links) renders identically to assistant messages.

The three seeded announcements and four ack rows were deleted afterwards — the dev table is back to zero rows. Temporary launch/env config and the dev server's regenerated favicons were all reverted.

Test results

  • New backend tests: 44 visibility cases + 22 route cases, covering state/date/role/new-user filtering, ack suppression scoped to loud surfaces, the caps and their ordering (requiresAck first, then severity, then oldest), the 404-not-403 path, unread/updated derivation, and the flag-off 404.
  • New SPA tests: 26, using DI-token overrides rather than vi.mock per house convention — service caps, fail-open dismissal, unread clearing without waiting on the server, and the pill snapshot.
  • SPA suite: 2348 of 2349 pass. The single failure is shared-view.page.spec.ts > should create the component, a load-sensitive timeout — I confirmed it fails on clean develop too (where this machine produced 9 failures across 5 files), so it is pre-existing and not from this change.
  • npx tsc --noEmit: clean.
  • The full backend suite was still running when this PR was opened; every announcements-related test and the architecture/rbac suites pass. I'll confirm the full run in a comment.

No infrastructure change in this PR — the table and IAM grant shipped with #966, so this deploys through backend.yml + frontend-deploy.yml alone.

🤖 Generated with Claude Code

…ew panel

PR-2 of docs/specs/feature-announcements.md. The durable surface: users can
now see and acknowledge announcements. Nothing interrupts anyone — the banner
(PR-4) and modal (PR-5) are still unbuilt, though the API already returns
their slots so those PRs are pure frontend.

Backend
- `apis/shared/announcements/visibility.py` — the whole filter chain as one
  pure function: no DynamoDB, no FastAPI, no clock of its own. The server
  computes visibility and the client renders what it is handed (§D5), so the
  rules exist in one language instead of drifting across two.
- `GET /announcements` returns the panel list plus the capped banner/modal
  slots and an unread count; `POST /announcements/{id}/ack` records seen /
  dismissed / acknowledged. Cookie session auth on both, per CLAUDE.md.
- `UserAnnouncement` is deliberately NOT a subset alias of the admin model.
  It omits `state`, `targetRoles`, `showToNewUsers` and `createdBy`, so
  adding an admin field can never widen the user payload by accident.

**Divergence from the spec's filter chain, on purpose.** §D5 lists the ack
check as step 5, before the caps. D1 and D2 are explicit that dismissing a
loud surface leaves the entry in the panel — so applied literally, step 5
would delete the durable record the design is built around. Eligibility
(steps 1-4) produces the panel; ack suppression applies only when choosing
the banner and the modal.

The ack endpoint 404s (not 403s) on an id this user cannot see: 403 would
confirm that an announcement targeted at another role exists.

Frontend
- `announcements.service.ts` — signal-based, loads on first read from the user
  dropdown, which the topnav renders only after the session resolves (so the
  request carries the roles the server needs for targeting). Fail-open
  dismissal per §D7: a rejected ack still hides the item for the tab and
  resolves rather than throwing.
- What's-New panel in the user menu, with the unread dot and count badge. The
  count is in the aria-label, not the colour alone. Opening the panel marks
  everything seen; the pills snapshot at open so they do not vanish as the
  user reads them.

Markdown renders through `message-block`, the app's existing markdown
stylesheet, NOT the `prose` classes: the Tailwind typography plugin is not
installed, so those are inert and preflight strips list markers. Browser
verification caught this — lists rendered without bullets and tables
unstyled, and every unit test passed. Reusing `message-block` is also what
§D10 asks for, since it is what assistant messages use.

Verified end to end against dev data: New / Updated / already-read pills,
the revision bump lapsing suppression, and acks persisting across a reload.
The seeded rows were removed afterwards.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@philmerrell
philmerrell merged commit e3ed36d into develop Sep 5, 2026
4 checks passed
@philmerrell
philmerrell deleted the feature/announcements-user-surface branch September 5, 2026 21:28
@philmerrell

Copy link
Copy Markdown
Contributor Author

Full backend suite: 7602 passed, 3 skipped, 5 failed — all five failures are pre-existing or load flakes, none related to this change. Detail, since "5 failed" deserves more than an assertion that it's fine:

The run took 1:18:23 (normally ~7 min) because it was competing with an Angular build on the same machine. That contention is the direct cause of four of the five.

Failure Verdict
test_harness_grants.py ×3 Load flake — all pass in isolation
test_pbt_kb_status_fail_closed.py Load flake — passes in isolation
test_pbt_auth_sweep.py::TestNonAdminRoleRejection::test_non_admin_roles_get_403 Pre-existing — reproduces on clean develop

The auth-sweep one is worth naming precisely, because a failing test with "non-admin" and "403" in the name is exactly the kind you should not wave through when a PR adds routes:

  • It is a hypothesis DeadlineExceeded, not an authorization failure: "took 313.04ms, which exceeds the deadline of 200.00ms", then 9.28ms on the retry. Hypothesis classifies it itself as FlakyFailure — falsified on the first call, not on the next.
  • Every actual assertion passed. The captured log shows 403 Forbidden for every generated role set.
  • It exercises GET /admin/managed-models, not any route added here.
  • It reproduces on clean develop (b4a69b82) in 17s on an idle machine, with this branch checked out or not.

So: pre-existing timing sensitivity in that property test's 200ms deadline, unrelated to announcements. Worth a deadline=None on its own, but not this PR's business.

Everything owned by this change is green:

  • test_announcements_visibility.py — 44 passed
  • test_announcements_user_routes.py — 22 passed
  • test_announcements.py + test_announcements_routes.py (PR-1) — still green
  • tests/architecture/ + tests/rbac/ — green

Also confirmed the branch still merges cleanly into current develop (which has moved to b4a69b82 since this branched) — 0 conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant