Skip to content

feat(notices): let a reader hide a banner without retiring it - #2681

Merged
camielvs merged 1 commit into
masterfrom
banners-06-hide
Sep 4, 2026
Merged

feat(notices): let a reader hide a banner without retiring it#2681
camielvs merged 1 commit into
masterfrom
banners-06-hide

Conversation

@camielvs

@camielvs camielvs commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

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:

  • A mandatory notice (no dismissible) sits on the home page with no way to clear it, even for someone who has read it and wants their dashboard back.
  • Clearing a dismissible one is a one-way door: there was no way to see it again short of clearing 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:

  • Dismissible notice → hidden for good, persisted under hidden-notices.
  • Mandatory notice → hidden for this session only, back on the next load.

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

useHiddenNotices is a separate store from useNotices, keyed on hidden-notices, and the banners compose the two. Hidden state deliberately doesn't live in useNotices — 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 showAll can restore everything: session hides clear on their own, and hidden-notices is emptied.

One shared primitive changes: InfoBox gains dismissIcon and dismissLabel, both defaulting to today's values (X / "Dismiss"), so every existing InfoBox renders 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 on InfoBox would be worse.

How to test

Install a source with one dismissible and one mandatory notice (see src/config/NOTICES.md):

  • Both banners on the dashboard home now show an eye-off, not an X.
  • Hide the dismissible one → gone, and still gone after a reload. It's still listed in the header popover.
  • Hide the mandatory one → gone, but back after a reload.
  • Popover header shows Show notices once anything is hidden; clicking it brings both banners back.
  • Hide notices clears the whole home region in one click.
  • The X in the popover still permanently retires a dismissible notice.

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
5 #2681 hide a banner without retiring it (you are here)

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.

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown

🎩 Preview

A preview build has been created at: banners-06-hide/10818c0

camielvs commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

@camielvs
camielvs marked this pull request as ready for review August 29, 2026 00:14
@camielvs
camielvs requested a review from a team as a code owner August 29, 2026 00:14
@camielvs
camielvs force-pushed the banners-06-hide branch 2 times, most recently from 39f4679 to 1f10f45 Compare September 3, 2026 18:50

@morgan-wowk morgan-wowk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

@camielvs

camielvs commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Your id-reuse nit is now documented, @morgan-wowkNOTICES.md on #2666 (9db2da9) rather than here, since it is a property of the contract that mints the ids rather than of the hide layer:

An id belongs to a notice's content, not to its slot. Whether a reader has read, dismissed or hidden a notice is stored against its id in their browser, so editing a title or body while keeping the id will not resurface it for anyone who has already cleared it. Publish changed content under a new id.

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.

camielvs commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Merge activity

  • Sep 4, 6:33 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 4, 6:54 PM UTC: Graphite rebased this pull request as part of a merge.
  • Sep 4, 6:58 PM UTC: @camielvs merged this pull request with Graphite.

camielvs added a commit that referenced this pull request Sep 4, 2026
## 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.
camielvs added a commit that referenced this pull request Sep 4, 2026
## 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 |
camielvs added a commit that referenced this pull request Sep 4, 2026
## 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.
@camielvs
camielvs changed the base branch from banners-05-inbox to graphite-base/2681 September 4, 2026 18:47
camielvs added a commit that referenced this pull request Sep 4, 2026
## 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.
@camielvs
camielvs changed the base branch from graphite-base/2681 to master September 4, 2026 18:52
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.
Comment on lines +73 to +77
new StorageEvent("storage", {
key: HIDDEN_KEY,
newValue: JSON.stringify(["a"]),
storageArea: localStorage,
}),
@camielvs
camielvs merged commit 845ecdc into master Sep 4, 2026
17 checks passed
@camielvs
camielvs deleted the banners-06-hide branch September 4, 2026 18:58

@morgan-wowk morgan-wowk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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. useHiddenNotices only touches localStorage["hidden-notices"] and a module-level in-memory hiddenForThisSession set — 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. markHidden persists to hidden-notices only when notice.dismissible, else session-only (hiddenForThisSession.add, gone on reload). The three stores — hidden-notices / dismissed-notices / read-notices — are distinct keys.
  • Storage-failure resilient. typedStorage swallows a throwing localStorage; 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 synthetic StorageEvent (null storageArea) is ignored and handled by the explicit publish(); cross-tab real events re-publish.
  • InfoBox defaults preserved. New dismissIcon/dismissLabel default to "X"/"Dismiss" — the previously hardcoded values — and NoticeCard is the only one of 60+ InfoBox usages that passes onDismiss. 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.

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.

3 participants