Skip to content

feat(notices): notices button in the header - #2668

Open
camielvs wants to merge 4 commits into
banners-04-stripfrom
banners-05-inbox
Open

feat(notices): notices button in the header#2668
camielvs wants to merge 4 commits into
banners-04-stripfrom
banners-05-inbox

Conversation

@camielvs

@camielvs camielvs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

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 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown

🎩 Preview

A preview build has been created at: banners-05-inbox/c5ba846

Comment thread src/components/shared/Banners/BannerInbox.tsx Outdated
Comment thread src/components/shared/Notices/NoticeInbox.tsx Outdated
Comment thread src/components/shared/Notices/NoticeInbox.tsx
Comment thread src/components/shared/Banners/BannerInbox.test.tsx Outdated
Comment thread src/components/shared/Banners/BannerInbox.tsx Outdated
Comment thread src/components/shared/Banners/BannerInbox.tsx Outdated
@camielvs
camielvs force-pushed the banners-05-inbox branch 2 times, most recently from 9e7df61 to 0769ace Compare August 27, 2026 00:05
@camielvs
camielvs force-pushed the banners-05-inbox branch 2 times, most recently from af61eb7 to b02bd10 Compare August 27, 2026 01:18
@camielvs camielvs mentioned this pull request Aug 27, 2026
8 tasks
@camielvs
camielvs marked this pull request as ready for review August 28, 2026 00:02
@camielvs
camielvs requested a review from a team as a code owner August 28, 2026 00:02

Copy link
Copy Markdown
Collaborator

I think you forgot to push local changes
image.png

image.png

It looks like the latest code wasn’t pushed, because the branch appears to be missing part of the intended implementation.

I vibe-coded a quick solution locally so I could test the layout with six more realistic notifications of varying lengths. Based on the attached screenshot, I don’t think the two-column layout works well. Longer notifications make the columns uneven, the reading order is harder to follow, and the page can quickly become visually overwhelming.

My recommendation is to use a single stacked list instead. It would be easier to scan, handle notifications of different lengths more naturally, and scale better when several are active at once.

Copy link
Copy Markdown
Collaborator

image.png

Here's an example of what it looked like stacked. I understand that it takes up a lot of room, but I think it's easier to read. and realistically we probably only have one to three banners like this

image.png

here it is in the 2 column view:

image.png

@camielvs
camielvs force-pushed the banners-05-inbox branch 2 times, most recently from 1d161de to f563d37 Compare August 28, 2026 23:51
camielvs and others added 4 commits August 28, 2026 17:02
Adds a Megaphone button to the top bar that opens the full list of notices,
with an unread count badge. Opening the list marks everything in it as read;
notices the host marked dismissible can be removed one at a time from here.

The list is also the way back once the strip has been hidden, which until now
was a one-way door for the rest of the session. Its toggle mirrors the strip's,
and the strip's own controls stay out of the way while the list is open.
…oggle

The notices button now stays in the header whether or not there is
anything to read, and opening it with nothing to show gives a "No notices"
empty state. Previously the whole affordance unmounted when the list
emptied, which also meant dismissing the last notice pulled the open
popover out from under the reader.

Cap the unread badge at 9+ so a host-supplied count cannot clip inside the
badge; the trigger's label still carries the exact number.

The banner toggle follows hasHiddenNotices, so it offers to bring the
banners back as soon as one notice is hidden rather than only when all of
them are.

Move the viewport-fit sizing onto the PopoverContent primitive so no caller
has to name Radix custom properties, and let the header row use the fill
prop rather than a width class.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown
Collaborator Author

Weird. I definitely pushed the rework, as a three-column view.

The question of 1 v 2 v 3 columns is tricky - 1 column is bad for short notices, whilst the others are bad for long notices. It's easy to change though, so once we dream up a solution we can iterate.

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.

2 participants