feat(notices): let a reader hide a banner without retiring it - #2681
Conversation
🎩 PreviewA preview build has been created at: |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c5ba846 to
f3e7cec
Compare
39f4679 to
1f10f45
Compare
f3e7cec to
685017f
Compare
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 Automated review
Approving. "Hide" is genuinely reader-local — useHiddenNotices writes only to localStorage/an in-memory session set and never mutates the host source or the shared snapshot, so a hidden notice stays "active" and still shows in the inbox. The two-tier rule (dismissible → persisted, mandatory → session-only) is correct and tested, and the hidden-notices/read-notices/dismissed-notices keys don't collide. The InfoBox dismissIcon/dismissLabel defaults keep every existing InfoBox visually identical.
Nit (spans the id-keyed persistence layers here and in #2668): hide/dismiss/read are all keyed on the stable notice.id, so if a host reuses an id for new content, a reader who hid or dismissed the old one silently never sees the new one. That's a documented consequence of the #2666 id contract, not a bug — worth one line in NOTICES.md telling hosts to mint a fresh id for new content.
1f10f45 to
37ab29b
Compare
685017f to
fad48b0
Compare
|
Your id-reuse nit is now documented, @morgan-wowk —
Worth noting it stopped being hypothetical while this stack sat in review: the host side (Shopify/oasis-frontend#694) now serves notices from an oasis-backend table, so "edit the existing row" is a thing an operator will reasonably do, and this stack gives them three separate reader-local reasons it will not show up. |
37ab29b to
eb3bd2c
Compare
fad48b0 to
015d06d
Compare
## Why `AiChat` had built a full markdown renderer for itself. The notices feature later in this stack needs to render markdown too — and from a source outside the app, so it needs stricter handling. Rather than keep a second copy, this lifts AiChat's renderer into a shared component. ## What you get Two entry points: - **`Markdown`** — for text the app authored. The same styling AiChat had, now in one place. - **`UntrustedMarkdown`** — for text the app did *not* author. Raw HTML never renders, links are narrowed to absolute `http(s)` URLs (so nothing can point into our own routes or run `javascript:`), and images degrade to their alt text instead of fetching a remote file. AiChat keeps only its own overrides: entity and component chips, and syntax-highlighted fenced code blocks. Links render through the `Link` primitive rather than a raw anchor, so an internal href stays in the same tab and only absolute `http(s)` gets `target="_blank"` and the external icon. That needed one fix in the primitive: it wrapped its children in a `div`, which is invalid inside a `<p>` and which React flagged on every render — AiChat already renders `Link` inside markdown today. A `span` fixes it with no visual change. ## Reviewer notes **This is the one PR in the stack that changes something you can already see.** AiChat's markdown picks up the shared styling, which differs slightly from what it had: | element | before | after | | --- | --- | --- | | `h1` | medium | large, slightly tighter top margin | | `h2` | small bold | medium semibold | | `h3` | small semibold | marginally more space below | | `h4` | small semibold | small semibold, dimmed | | `h5` `h6` | unstyled | small, dimmed | | bullet / number markers | default colour | dimmed | | task lists (`- [ ]`) | rendered with a bullet | bullet removed, checkbox spaced | | table cells | vertically centred | top-aligned | | images | unstyled | rounded, capped at container width | These are the shared component's choices; worth a glance to confirm they're an improvement and not a surprise. An earlier revision of this PR also dropped `last:border-b-0` from table rows — that was an accident in the extraction and is restored. Tests: both entry points, with the untrusted path's escapes (script tags, `javascript:` links, relative URLs, remote images) asserted directly, plus unit tests for the URL guard. ## Where this sits | | PR | | | --- | --- | --- | | **1** | **#2664** | **shared Markdown renderer** *(you are here)* | | 2 | #2666 | validated host contract | | 3 | #2667 | notice banners on the dashboard home — announcement parity | | 4 | #2668 | notices button in the header | | 5 | #2681 | hide a banner without retiring it | This one stands alone: it is useful as a de-duplication whether or not anything above it lands.
## Why The announcements this stack replaces are **already host-installed**: `AnnouncementBanners` reads `window.__TANGLE_ANNOUNCEMENTS__` directly, and `src/config/announcements.ts` is nothing but the `declare global` for it. So the mechanism isn't new here — what's new is that it's validated, typed at the boundary as `unknown`, and can update after boot. The existing global has four problems, all of them things a host page can trigger today: - **No validation.** `variant: announcement.variant ?? "info"` passes any string through to `variantStyles[variant].container` — a typo'd variant is a `TypeError` in render, and an entry with no `title` renders an empty banner. - **Read during render, with `new Date()`.** Both a mutable global read and a clock read happen inside the component body, which is why `AnnouncementBanners.tsx` can't be enabled for the React Compiler. - **No live update.** `useState(getDismissedIds)` snapshots once, and the global is read with no subscription — a host that installs or replaces its data after the app mounts is invisible until a reload. There's no event, so it's a race the host can't win. - **Body is plain text.** No links, no emphasis, no lists. ## What you get **No behaviour change.** This PR adds `src/config/notices.ts` and its documentation; nothing imports it yet. `__TANGLE_ANNOUNCEMENTS__` and its banners are still the only thing on screen — they're replaced in #2667. ```js window.__TANGLE_NOTICE_SOURCE__ = { version: 1, getSnapshot: () => notices, subscribe: (listener) => { listeners.add(listener); return () => listeners.delete(listener); }, refresh: () => fetchNotices(), // optional }; window.dispatchEvent(new CustomEvent("tangle:notice-source")); ``` With nothing installed the app shows nothing, exactly as it does today with the global unset. `src/config/NOTICES.md` is the reference for host authors. ## Reviewer notes **This is a trust boundary, which is where the code goes.** The global is typed `unknown` and everything is proven before it reaches a component: one type guard for the source, then per-field reads. The rules, all tested: - `version` must be exactly `1` — a source declaring `2` is ignored wholesale, so a future contract can't half-render through this one. - `id` required (a finite number is coerced); `title` required and non-blank. An entry failing either is dropped rather than rendered empty. - `variant` falls back to `info` for anything unrecognised — this is the fix for the `variantStyles[variant]` crash above. - `body` is Markdown and renders through `UntrustedMarkdown` from #2664. Whitespace is preserved because it's significant. - `action.url` must be absolute `http(s)`; `javascript:` and relative URLs drop the action and keep the notice. - Duplicate ids collapse to the first; at most 20 entries are read per snapshot. - A `getSnapshot` that throws is treated as "no notices" rather than propagating. Two things worth a look because they're subtle rather than long: **Snapshot identity.** `getSnapshot` is called on every render, and a host that maps/spreads its array returns a new reference each time — which would loop `useSyncExternalStore`. The store compares serialised content and returns the previous frozen array when it's unchanged. Tested with a rebuild-per-read source. **The required event.** The app subscribes when it mounts, which may be before the host has installed anything, so `subscribe` alone would race. `tangle:notice-source` is what tells the store to (re)bind — documented as required, tested for late installation, and safe to dispatch again after replacing the global. Without it the options are a silent race (today's behaviour) or blocking app boot on the host's fetch. **Field-level differences from `Announcement`:** `expiresAt` is gone — a host that can update its snapshot expires a notice by dropping it, rather than shipping an expiry the app has to evaluate with a clock read in render. `action` is added. `id`/`title`/`body`/`variant`/`dismissible` carry over. ## Where this sits | | PR | | | --- | --- | --- | | 1 | #2664 | shared Markdown renderer | | **2** | **#2666** | **validated host contract** *(you are here)* | | 3 | #2667 | notice banners on the dashboard home — announcement parity | | 4 | #2668 | notices button in the header | | 5 | #2681 | hide a banner without retiring it |
## Why This is the PR that swaps the announcements system for the notices one. Everything below it was preparation; everything above it is optional. ## What you get `AnnouncementBanners` and `src/config/announcements.ts` are deleted. `NoticeBanners` takes the exact slot `<AnnouncementBanners />` occupied — first child of `DashboardHomeView`'s `BlockStack`, dashboard home only, nowhere else in the app. **Scope of this PR is announcement parity, deliberately.** A reader can see banners and dismiss the ones the host marked dismissible. That's it. The notice inbox is #2668 and hiding is #2681, so each can be judged — or dropped — on its own. What parity buys over the deleted version, all of it a consequence of #2666 rather than new surface here: - an unrecognised `variant` no longer crashes the render - a `body` renders as Markdown, so a notice can link and emphasise - an optional action button - notices appear when the host publishes them, without a reload, and a dismissal in one tab applies in the others - `NoticeBanners` and `useNotices` are React Compiler-enabled; `AnnouncementBanners` could not be, because it read a mutable global and called `new Date()` in render Two visible differences from the deleted banners, both intentional: - **Severity order.** `error` → `warning` → `success` → `info`, rather than host array order, so an outage isn't below a nice-to-know. - **Three-column grid** matching the dashboard's other rows, rather than full-width stacked. A long body scrolls inside its own card with the action pinned below it, so one verbose notice can't push the rest of the page down. ## Reviewer notes `src/hooks/useNotices.ts` is where the non-obvious code is: **Dismissal is two-tier, and this is the bit worth reading.** `dismissible: true` from the host means "the reader may retire this permanently" → `localStorage`. A notice *without* `dismissible` is mandatory, so it can be cleared for the current session only and returns on the next load. In this PR the banners only ever offer the X on dismissible notices, so the session tier has no UI yet — it's what #2668's inbox and #2681's hide build on, and it is tested directly. **`refresh` is optional and this is the only caller.** One `visibilitychange` listener, throttled to at most once per 30s, calling `source.refresh?.()`. Nothing in this feature polls on a timer — no `setInterval` is added anywhere. If the host doesn't implement `refresh`, this is a no-op and notices simply update whenever the host publishes. **Store rather than context.** Module-level state + `useSyncExternalStore`, because two unrelated subtrees read it (the banners here, the header button in #2668) and neither owns the other. It also gives a stable snapshot to the React Compiler and picks up the cross-tab `storage` event for free. Also here: `notices.ts`'s `TangleNoticeAction`/`TangleNoticeSource` stop being exported now that nothing outside the module names them, and `knip.json` drops both `src/config/announcements.ts` and `src/config/notices.ts` from its ignore list — the first because the file is gone, the second because it now has real consumers. ## How to test Install a source in the console and dispatch the event (see `src/config/NOTICES.md`): ```js window.__TANGLE_NOTICE_SOURCE__ = { version: 1, getSnapshot: () => [ { id: "1", title: "Scheduled maintenance", body: "Submissions paused **09:00–11:00 UTC**.", variant: "warning", dismissible: true }, { id: "2", title: "Read-only mode", body: "", variant: "error" }, ], subscribe: () => () => {}, }; window.dispatchEvent(new CustomEvent("tangle:notice-source")); ``` - Dashboard home shows both, error first, no reload needed. - The warning has an X and stays gone after a reload; the error has none. - No banners on any other route. - With the global unset, the home page is byte-for-byte what it is on `master` with no announcements installed. ## Where this sits | | PR | | | --- | --- | --- | | 1 | #2664 | shared Markdown renderer | | 2 | #2666 | validated host contract | | **3** | **#2667** | **notice banners on the dashboard home — announcement parity** *(you are here)* | | 4 | #2668 | notices button in the header | | 5 | #2681 | hide a banner without retiring it | **This is the merge point.** Stopping here is a complete, shippable replacement for the announcements system. #2668 and #2681 add surface on top and are independently droppable.
## Why
Banners live on the dashboard home only — same as the announcements they replace. Someone who opens a run page from the CLI and stays there never sees them. And there was no way to look at a notice again after clearing it off the page.
This adds a megaphone to the header: notices are reachable from any route, with an unread badge.
## What you get
A `Popover` on a `TooltipButton` in both top bars (v1 `AppMenu`, v2 `AppMenuActions`), listing every notice in full — unclamped, so a long body isn't cut off the way it is in a banner. Dismissible notices can be retired from here too. Empty state is "No notices"; the button stays in the header either way so it doesn't shift its neighbours around.
**Unread**, and only unread, is what this PR adds to the state model: a notice is unread until the reader opens the popover, tracked by id in `localStorage` under `read-notices`. Badge caps at `9+` visually while the trigger's `aria-label` keeps the exact count ("Notices, 12 unread"). Reading is separate from dismissal, so opening the list doesn't clear anything from the banners.
## Reviewer notes
`useNoticeInbox` wraps `useNotices` rather than duplicating it — it adds read-tracking and open state and forwards `notices`/`dismiss` through. Same module-store + `useSyncExternalStore` shape as #2667, for the same reason: the badge and the list are in different subtrees.
Open state is deliberately in the store rather than `useState`, because it has to survive the trigger unmounting when the route swaps between the v1 and v2 top bars. The `useEffect(() => closeNoticeInbox, [])` closes it on unmount so it can't come back open on a different page.
**One shared primitive changes: `popover.tsx`, one line.** `PopoverContent` had a fixed `w-72` and no height ceiling, so a tall popover ran off the bottom of the viewport with no way to reach the rest. It now caps to Radix's available width/height and scrolls. This affects every popover in the app — worth confirming, though the change only ever *removes* overflow.
Accessibility: the trigger's name carries the count, dismissal is a labelled button, and the popover keeps focus and stays reachable after its last notice goes (tested).
⚠️ The screenshots on this PR predate the restructure and still show a **Show/Hide notices** toggle in the popover header — that control now belongs to #2681. The list itself is otherwise as shown.
## How to test
Install a source (see #2667 or `src/config/NOTICES.md`), then:
- Megaphone appears in the header on every route, including the ones with no banners.
- Badge shows the unread count; opening the popover clears it, and it stays clear after a reload.
- Each notice renders in full, with its action if it has one.
- Dismiss a dismissible notice from the popover — it goes from both the popover and the home banners.
- With no source installed, the button is present and says "No notices".
## Where this sits
| | PR | |
| --- | --- | --- |
| 1 | #2664 | shared Markdown renderer |
| 2 | #2666 | validated host contract |
| 3 | #2667 | notice banners on the dashboard home — announcement parity |
| **4** | **#2668** | **notices button in the header** *(you are here)* |
| 5 | #2681 | hide a banner without retiring it |
Additive on top of the #2667 merge point — the banners work without this.
A notice the host marks mandatory cannot be dismissed, so without this there is no way to clear one off the dashboard home. Hiding is per-reader and reversible: the notice stays in the header inbox, which offers to bring the banners back. A hide persists only for notices the host already allows to be dismissed; hiding a mandatory notice lasts for the session.
eb3bd2c to
10818c0
Compare
| new StorageEvent("storage", { | ||
| key: HIDDEN_KEY, | ||
| newValue: JSON.stringify(["a"]), | ||
| storageArea: localStorage, | ||
| }), |
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 Re-review — approving the reader-local hide
Re-verified at the current head now that hide is consolidated here (moved out of #2667):
- Hide is genuinely reader-local.
useHiddenNoticesonly toucheslocalStorage["hidden-notices"]and a module-level in-memoryhiddenForThisSessionset — it never calls the notice source or mutates the frozen snapshot. Banners filter locally (notices.filter((n) => !hiddenIds.has(n.id))) while the inbox renders the unfiltered list, so a hidden notice stays "active" and still shows in the inbox (tested: "keeps listing a hidden notice in full"); retire/dismiss remains a separate inbox action. - Two-tier rule, no key collision.
markHiddenpersists tohidden-noticesonly whennotice.dismissible, else session-only (hiddenForThisSession.add, gone on reload). The three stores —hidden-notices/dismissed-notices/read-notices— are distinct keys. - Storage-failure resilient.
typedStorageswallows a throwinglocalStorage; the write fails silently,readHiddenIds()returns[], and the id falls through to the in-memory set — degrades to session-only rather than crashing;publish()runs independently so the UI still updates. Same-tab syntheticStorageEvent(nullstorageArea) is ignored and handled by the explicitpublish(); cross-tab real events re-publish. - InfoBox defaults preserved. New
dismissIcon/dismissLabeldefault to"X"/"Dismiss"— the previously hardcoded values — andNoticeCardis the only one of 60+InfoBoxusages that passesonDismiss. No visual change to any other InfoBox. - Hide is keyed on the stable
notice.id, consistent with the id-stability contract now documented in #2666's NOTICES.md.
LGTM.

Why
With #2668 merged, dismissing a notice is the only way to clear it off the dashboard home — and dismissal is permanent and only offered on notices the host marked
dismissible. That leaves two gaps:dismissible) sits on the home page with no way to clear it, even for someone who has read it and wants their dashboard back.localStorage.Now that the notices are also in the header popover, taking a banner off the home page doesn't lose it — so hide becomes safe to offer, on every notice.
What you get
The banner's X becomes an eye-off, "Hide notice", present on every card:
hidden-notices.The reader always still has it in the header popover either way. The popover header gains one toggle that flips on state: Hide notices when any are showing, Show notices when any are hidden — so hiding is reversible without touching storage by hand.
Dismissal is unchanged and still lives in the popover: the X there still retires a dismissible notice permanently.
Reviewer notes
useHiddenNoticesis a separate store fromuseNotices, keyed onhidden-notices, and the banners compose the two. Hidden state deliberately doesn't live inuseNotices— a hidden notice is still an active notice, and the popover has to keep listing it.The persist-if-dismissible / session-only-if-mandatory rule is the same two-tier shape #2667 established for dismissal, and it's the reason
showAllcan restore everything: session hides clear on their own, andhidden-noticesis emptied.One shared primitive changes:
InfoBoxgainsdismissIconanddismissLabel, both defaulting to today's values (X/ "Dismiss"), so every existingInfoBoxrenders identically. This is the only change to a design-system primitive in the notices stack, and it exists because a notice card needs the same affordance in the same slot with a different meaning — an eye-off in the banner, an X in the popover — and a second dismiss slot onInfoBoxwould be worse.How to test
Install a source with one dismissible and one mandatory notice (see
src/config/NOTICES.md):Where this sits
Top of the stack, and the most droppable PR in it: #2667 alone is a complete announcements replacement, and #2668 adds the header surface. This is purely a reader affordance and depends on #2668 existing — hiding is only reasonable because the popover keeps the notice reachable.