From 577f3fbd714d2ddb261efe9b3ea9690dbe0e4fee Mon Sep 17 00:00:00 2001 From: Harry Randall Date: Sun, 20 Sep 2026 20:03:41 +1000 Subject: [PATCH 1/2] feat: tell an administrator when their import run has finished A run could take twenty minutes and the only way to learn how it ended was to sit on the imports page and watch. This adds an own-row inbox and hangs the first producer off the one write every ending shares. Four paths end a run, two of them SQL with no application code to hook, so the producer is a trigger on the run row rather than a call in the worker: private.refresh_catalogue_import_run() is the sole writer of catalogue_import_runs.status, so a trigger there catches the worker finishing, a cancellation, a released target and the stale-lease sweep without each caller having to remember. The dedupe key means a run that leaves and re-enters a terminal status is still reported once. The table is gated on identity, not on a permission. The catalogue write hole came from a helper that accepted a read permission every sign-up holds, so there is no permission to get wrong here: authenticated gets select and nothing else, the only reader of a row is the user named on it, and both the producer and the read marker are definer routines no client role can call or aim at somebody else. --- apps/web/types/database.ts | 40 ++ .../20260920400000_notifications.sql | 218 ++++++++++ supabase/tests/database/notifications.sql | 379 ++++++++++++++++++ 3 files changed, 637 insertions(+) create mode 100644 supabase/migrations/20260920400000_notifications.sql create mode 100644 supabase/tests/database/notifications.sql diff --git a/apps/web/types/database.ts b/apps/web/types/database.ts index 318e46f4..8c0df089 100644 --- a/apps/web/types/database.ts +++ b/apps/web/types/database.ts @@ -2453,6 +2453,42 @@ export type Database = { } Relationships: [] } + notifications: { + Row: { + body: string | null + created_at: string + dedupe_key: string | null + href: string | null + id: string + kind: string + read_at: string | null + title: string + user_id: string + } + Insert: { + body?: string | null + created_at?: string + dedupe_key?: string | null + href?: string | null + id?: string + kind: string + read_at?: string | null + title: string + user_id: string + } + Update: { + body?: string | null + created_at?: string + dedupe_key?: string | null + href?: string | null + id?: string + kind?: string + read_at?: string | null + title?: string + user_id?: string + } + Relationships: [] + } offering_sessions: { Row: { academic_period_code: string @@ -3605,6 +3641,10 @@ export type Database = { Args: { p_item_year_id: number } Returns: undefined } + mark_notifications_read: { + Args: { p_notification_ids?: string[] } + Returns: number + } move_current_user_plan_item: { Args: { p_before_plan_item_id?: string diff --git a/supabase/migrations/20260920400000_notifications.sql b/supabase/migrations/20260920400000_notifications.sql new file mode 100644 index 00000000..aae45b74 --- /dev/null +++ b/supabase/migrations/20260920400000_notifications.sql @@ -0,0 +1,218 @@ +begin; + +-- An in-app inbox, one row per person per thing worth telling them. +-- +-- Own-row means own-row. The recent catalogue write hole came from a helper +-- that accepted a permission every sign-up already holds, so nothing here is +-- gated on a permission at all: the only reader of a row is the user named on +-- it, and the only writer is a routine that runs with definer rights inside +-- the database. `authenticated` gets select and nothing else, so a client +-- cannot invent a notification for somebody else even by accident. + +create table public.notifications ( + id uuid primary key default gen_random_uuid(), + user_id uuid not null, + kind text not null, + title text not null, + body text, + href text, + dedupe_key text, + read_at timestamptz, + created_at timestamptz not null default now(), + constraint notifications_user_fkey + foreign key (user_id) references auth.users (id) on delete cascade, + constraint notifications_kind_check check ( + kind in ('import_run', 'key_date', 'plan_risk', 'published_change') + ), + constraint notifications_title_check check (btrim(title) <> ''), + constraint notifications_href_check check (href is null or href like '/%'), + constraint notifications_dedupe_key_check check ( + dedupe_key is null or btrim(dedupe_key) <> '' + ) +); + +-- The bell reads one page of a person's rows, newest first. +create index notifications_user_recent_idx + on public.notifications (user_id, created_at desc); + +-- The unread count is the only number on screen before the menu opens, so it +-- gets its own partial index rather than a scan over a growing history. +create index notifications_user_unread_idx + on public.notifications (user_id, created_at desc) + where read_at is null; + +-- The dedupe constraint, made real. A producer that runs again for the same +-- subject names the same key and the second insert is dropped rather than +-- stacked. Rows without a key are one-offs and are never deduplicated, so the +-- uniqueness is partial rather than a nullable column pretending to be unique. +create unique index notifications_dedupe_idx + on public.notifications (user_id, dedupe_key) + where dedupe_key is not null; + +alter table public.notifications enable row level security; + +create policy notifications_owner_select +on public.notifications +for select +to authenticated +using ((select auth.uid()) = user_id); + +-- No insert, update or delete policy and no write grant: every producer runs +-- through private.record_notification() and reading is marked through +-- public.mark_notifications_read(). A client has no reason to write this table +-- and therefore no way to. +revoke all on table public.notifications from anon, authenticated; +grant select on table public.notifications to authenticated; +grant select, insert, update, delete on table public.notifications to service_role; + +comment on table public.notifications is + 'In-app inbox. A row belongs to exactly one user, who is its only reader.'; +comment on column public.notifications.dedupe_key is + 'Stable identity of the subject a repeating producer reports, unique per user.'; + +-- Producers ------------------------------------------------------------------------------ + +-- The single insert path. Callers are database routines; it is revoked from +-- every client role so a dedupe key cannot be forged from the browser. +create or replace function private.record_notification( + p_user_id uuid, + p_kind text, + p_title text, + p_body text default null, + p_href text default null, + p_dedupe_key text default null +) +returns uuid +language plpgsql +set search_path = '' +as $function$ +declare + recorded uuid; +begin + -- A run whose requester was deleted has nobody to tell. + if p_user_id is null then + return null; + end if; + + insert into public.notifications (user_id, kind, title, body, href, dedupe_key) + values (p_user_id, p_kind, p_title, p_body, p_href, p_dedupe_key) + on conflict (user_id, dedupe_key) where dedupe_key is not null do nothing + returning id into recorded; + + return recorded; +end; +$function$; + +revoke all on function private.record_notification(uuid, text, text, text, text, text) +from public, anon, authenticated; + +-- Import run completion. Four paths end a run: the worker finishing its last +-- target, cancel_catalogue_import(), release_catalogue_import_target() and the +-- stale-lease sweep. All four write the run row through +-- private.refresh_catalogue_import_run(), so hanging the producer off that +-- write is the one place that catches every path without each caller having to +-- remember. Two of the four are SQL only and have no application code to hook. +create or replace function private.notify_catalogue_import_run_finished() +returns trigger +language plpgsql +security definer +set search_path = '' +as $function$ +declare + kind_label text; + record_word text; + summary text; +begin + kind_label := case new.kind + when 'course' then 'courses' + when 'programme' then 'programmes' + when 'major' then 'majors' + when 'minor' then 'minors' + else 'specialisations' + end; + record_word := case when new.target_count = 1 then 'record' else 'records' end; + + summary := case new.status + when 'completed' then + case + when new.failed_count > 0 then format( + '%s of %s %s imported, %s failed.', + new.completed_count, new.target_count, record_word, new.failed_count + ) + else format('%s %s ready to review.', new.completed_count, record_word) + end + when 'failed' then format('All %s %s failed.', new.target_count, record_word) + else format('Stopped with %s of %s %s finished.', + new.completed_count, new.target_count, record_word) + end; + + perform private.record_notification( + new.requested_by, + 'import_run', + format('Import run #%s %s', new.run_number, new.status), + summary, + format('/admin/%s/imports?run=%s', kind_label, new.id), + format('import_run:%s', new.id) + ); + + return null; +end; +$function$; + +revoke all on function private.notify_catalogue_import_run_finished() +from public, anon, authenticated; + +-- refresh_catalogue_import_run() rewrites the run row on every target result, +-- so the trigger insists on an actual change into a terminal status. The +-- dedupe key is a second guard, not the first one. +create trigger catalogue_import_runs_notify_finished +after update of status on public.catalogue_import_runs +for each row +when ( + old.status is distinct from new.status + and new.status in ('completed', 'failed', 'cancelled') +) +execute function private.notify_catalogue_import_run_finished(); + +-- Reading -------------------------------------------------------------------------------- + +-- Marks the caller's unread rows read. It takes no user, so there is nothing +-- to tamper with: the filter is auth.uid() and a caller passing another +-- person's notification ids marks nothing. +create or replace function public.mark_notifications_read( + p_notification_ids uuid[] default null +) +returns integer +language plpgsql +security definer +set search_path = '' +as $function$ +declare + reader uuid := (select auth.uid()); + marked integer; +begin + if reader is null then + raise exception using errcode = '28000', message = 'Authentication is required.'; + end if; + + with changed as ( + update public.notifications + set read_at = now() + where user_id = reader + and read_at is null + and (p_notification_ids is null or id = any (p_notification_ids)) + returning id + ) + select count(*) into marked from changed; + + return marked; +end; +$function$; + +revoke all on function public.mark_notifications_read(uuid[]) from public, anon; +grant execute on function public.mark_notifications_read(uuid[]) to authenticated; + +comment on function public.mark_notifications_read(uuid[]) is + 'Marks the calling user''s unread notifications read. Null marks all of them.'; + +commit; diff --git a/supabase/tests/database/notifications.sql b/supabase/tests/database/notifications.sql new file mode 100644 index 00000000..fcc163ff --- /dev/null +++ b/supabase/tests/database/notifications.sql @@ -0,0 +1,379 @@ +-- The inbox is own-row, and nothing but the row's owner sees it. +-- +-- The catalogue write hole came from a permission helper that quietly let every +-- signed-in account through. The notifications table is gated on identity +-- rather than permission, so what has to hold is narrower and testable: one +-- user reads their own rows and no others, no client role can write the table +-- at all, mark_notifications_read() only ever touches the caller's rows, and a +-- producer that runs twice for the same subject leaves one row behind. + +begin; + +create extension if not exists pgtap with schema extensions; + +select extensions.plan(22); + +insert into auth.users ( + instance_id, id, aud, role, email, + raw_app_meta_data, raw_user_meta_data, created_at, updated_at +) +values + ( + '00000000-0000-0000-0000-000000000000', + '12000000-0000-4000-8000-000000000001', + 'authenticated', 'authenticated', 'inbox-one@example.test', + '{"provider":"email","providers":["email"]}'::jsonb, '{}'::jsonb, now(), now() + ), + ( + '00000000-0000-0000-0000-000000000000', + '12000000-0000-4000-8000-000000000002', + 'authenticated', 'authenticated', 'inbox-two@example.test', + '{"provider":"email","providers":["email"]}'::jsonb, '{}'::jsonb, now(), now() + ), + ( + '00000000-0000-0000-0000-000000000000', + '12000000-0000-4000-8000-000000000003', + 'authenticated', 'authenticated', 'inbox-importer@example.test', + '{"provider":"email","providers":["email"]}'::jsonb, '{}'::jsonb, now(), now() + ); + +-- Shape ----------------------------------------------------------------------- + +select extensions.ok( + ( + select relrowsecurity + from pg_class + where oid = 'public.notifications'::regclass + ), + 'row level security is enabled on notifications' +); + +select extensions.is( + ( + select count(*)::int + from information_schema.role_table_grants + where table_schema = 'public' + and table_name = 'notifications' + and grantee in ('anon', 'authenticated') + and privilege_type <> 'SELECT' + ), + 0, + 'no client role may write a notification' +); + +select extensions.is( + ( + select count(*)::int + from information_schema.role_table_grants + where table_schema = 'public' + and table_name = 'notifications' + and grantee = 'anon' + ), + 0, + 'a signed-out visitor has no access at all' +); + +-- The producer is the one insert path, and it is reachable only from inside the +-- database. A client role that could call it could forge a notification into +-- anybody's inbox and name its dedupe key, which is exactly the shape of the +-- catalogue write hole: a routine that looked private while every signed-in +-- account could reach it. +select extensions.is( + ( + select count(*)::int + from pg_proc as routines + join pg_namespace as schemas on schemas.oid = routines.pronamespace + cross join unnest(array['anon', 'authenticated', 'service_role']) as client(role) + where schemas.nspname = 'private' + and routines.proname = 'record_notification' + and has_function_privilege(client.role, routines.oid, 'execute') + ), + 0, + 'no client role may call the notification producer' +); + +select extensions.ok( + not has_function_privilege( + 'anon', 'public.mark_notifications_read(uuid[])', 'execute' + ), + 'a signed-out visitor cannot mark anything read' +); + +-- Producers write through the definer routine, as the application does. + +do $$ +begin + perform private.record_notification( + '12000000-0000-4000-8000-000000000001', + 'import_run', + 'Import run #1 completed', + 'Three records ready to review.', + '/admin/courses/imports?run=1', + 'inbox-test:one' + ); + perform private.record_notification( + '12000000-0000-4000-8000-000000000001', + 'plan_risk', + 'A planned course lost its offering', + null, null, + 'inbox-test:two' + ); + perform private.record_notification( + '12000000-0000-4000-8000-000000000002', + 'import_run', + 'Import run #2 failed', + 'All two records failed.', + '/admin/majors/imports?run=2', + 'inbox-test:three' + ); +end; +$$; + +-- Dedupe ---------------------------------------------------------------------- + +select extensions.ok( + private.record_notification( + '12000000-0000-4000-8000-000000000001', + 'import_run', + 'Import run #1 completed', + 'Three records ready to review.', + '/admin/courses/imports?run=1', + 'inbox-test:one' + ) is null, + 'a producer repeating the same dedupe key records nothing the second time' +); + +select extensions.is( + ( + select count(*)::int + from public.notifications + where dedupe_key = 'inbox-test:one' + ), + 1, + 'the repeated subject left exactly one row' +); + +-- The same key belongs to a different person's inbox independently. +select extensions.ok( + private.record_notification( + '12000000-0000-4000-8000-000000000002', + 'import_run', + 'Import run #1 completed', + null, null, + 'inbox-test:one' + ) is not null, + 'a dedupe key is scoped to one user, not shared across the table' +); + +select extensions.throws_ok( + $$ + insert into public.notifications (user_id, kind, title, dedupe_key) + values ( + '12000000-0000-4000-8000-000000000001', 'import_run', 'Forged', 'inbox-test:one' + ) + $$, + '23505', + null, + 'the dedupe constraint is enforced by the database, not only by the producer' +); + +-- Rows without a key are one-offs and are not deduplicated. +select extensions.ok( + private.record_notification( + '12000000-0000-4000-8000-000000000001', 'key_date', 'Census date', null, null, null + ) is not null, + 'a notification without a dedupe key is always recorded' +); + + +-- The import run producer ------------------------------------------------------ +-- +-- Every path that ends a run writes the run row, so the producer hangs off that +-- write rather than off any one caller. The run rows below are written directly +-- because what is under test is the transition, not the worker. + +insert into public.catalogue_import_runs ( + id, academic_year_id, kind, status, requested_model, + parser_version, prompt_version, schema_version, requested_by, + target_count, completed_count, failed_count +) +values + ( + '12000000-0000-4000-8000-00000000aaaa', + (select id from public.academic_years order by year desc limit 1), + 'course', 'running', + (select id from public.import_models order by id limit 1), + 'test', 'test', 'test', + '12000000-0000-4000-8000-000000000003', + 3, 3, 0 + ), + ( + '12000000-0000-4000-8000-00000000bbbb', + (select id from public.academic_years order by year desc limit 1), + 'major', 'running', + (select id from public.import_models order by id limit 1), + 'test', 'test', 'test', + null, + 1, 0, 1 + ); + +update public.catalogue_import_runs +set status = 'completed' +where id = '12000000-0000-4000-8000-00000000aaaa'; + +select extensions.is( + ( + select body + from public.notifications + where user_id = '12000000-0000-4000-8000-000000000003' + ), + '3 records ready to review.', + 'a finished run tells the administrator who asked for it what it produced' +); + +select extensions.is( + ( + select href + from public.notifications + where user_id = '12000000-0000-4000-8000-000000000003' + ), + format( + '/admin/courses/imports?run=%s', + '12000000-0000-4000-8000-00000000aaaa' + ), + 'the notification opens the run it is about' +); + +-- A recovered target can move a run out of a terminal status and back again. +update public.catalogue_import_runs +set status = 'running' +where id = '12000000-0000-4000-8000-00000000aaaa'; + +update public.catalogue_import_runs +set status = 'failed' +where id = '12000000-0000-4000-8000-00000000aaaa'; + +select extensions.is( + ( + select count(*)::int + from public.notifications + where user_id = '12000000-0000-4000-8000-000000000003' + ), + 1, + 'a run that finishes more than once is reported once' +); + +update public.catalogue_import_runs +set status = 'failed' +where id = '12000000-0000-4000-8000-00000000bbbb'; + +select extensions.is( + ( + select count(*)::int + from public.notifications + where kind = 'import_run' + and href like '/admin/majors/imports?run=12000000-0000-4000-8000-00000000bbbb' + ), + 0, + 'a run whose requester is gone notifies nobody' +); + +-- The second inbox's real notification ids, captured while they are still +-- visible. Passing null to mark_notifications_read() means "all of mine", so +-- the test has to hand it ids that exist and belong to somebody else. +select set_config( + 'tests.other_inbox_ids', + ( + select string_agg(id::text, ',') + from public.notifications + where user_id = '12000000-0000-4000-8000-000000000002' + ), + false +); + +-- The first user --------------------------------------------------------------- + +set local role authenticated; +select set_config( + 'request.jwt.claims', + '{"sub":"12000000-0000-4000-8000-000000000001","role":"authenticated"}', + true +); + +select extensions.is( + (select count(*)::int from public.notifications), + 3, + 'a user reads their own notifications' +); + +select extensions.is( + ( + select count(*)::int + from public.notifications + where user_id <> '12000000-0000-4000-8000-000000000001' + ), + 0, + 'a user cannot read another inbox' +); + +select extensions.throws_ok( + $$ + insert into public.notifications (user_id, kind, title) + values ('12000000-0000-4000-8000-000000000002', 'import_run', 'Forged') + $$, + '42501', + null, + 'a user cannot write into another inbox' +); + +select extensions.throws_ok( + $$ + insert into public.notifications (user_id, kind, title) + values ('12000000-0000-4000-8000-000000000001', 'import_run', 'Self-made') + $$, + '42501', + null, + 'a user cannot write into their own inbox either' +); + +-- The second user's rows are invisible, so aiming mark_notifications_read() at +-- them is the interesting case: the ids are real and the caller is not. +select extensions.is( + public.mark_notifications_read( + string_to_array(current_setting('tests.other_inbox_ids'), ',')::uuid[] + ), + 0, + 'marking another user''s notification ids read changes nothing' +); + +select extensions.is( + public.mark_notifications_read(), + 3, + 'marking everything read covers only the caller''s unread rows' +); + +select extensions.is( + (select count(*)::int from public.notifications where read_at is null), + 0, + 'the caller has nothing unread left' +); + +-- The second user --------------------------------------------------------------- + +select set_config( + 'request.jwt.claims', + '{"sub":"12000000-0000-4000-8000-000000000002","role":"authenticated"}', + true +); + +select extensions.is( + (select count(*)::int from public.notifications where read_at is null), + 2, + 'the other inbox was untouched by the first user marking theirs read' +); + +reset role; + +select * from extensions.finish(); + +rollback; From 218ec37f24455427d964fa9f55d3b5d7d79090b3 Mon Sep 17 00:00:00 2001 From: Harry Randall Date: Sun, 20 Sep 2026 20:03:57 +1000 Subject: [PATCH 2/2] feat: give the bell a real inbox instead of three invented rows The notifications menu shipped with a hard-coded sampleNotifications array, so the badge was decoration and reading a row did nothing. It now reads the signed-in user's rows, marks them read through the database function that scopes the update to the caller, and says so when it cannot. An empty inbox is what most people have most of the time, so it gets the finished treatment rather than the broken one: no retry, no warning colour and nothing for the reader to do. The failure state is the only one that offers another attempt. Rows group by age, not by read state, because grouping on read state would make a row jump to another heading the instant it was read and lose the reader's place. Marking read is optimistic and a failed write is corrected by the next read rather than by an error the reader can do nothing about. Reading goes through getAuthViewer(), so a signed-out visitor and a deployment with no Supabase configured both get an empty inbox rather than a thrown configuration error: the bell sits in the shell on every page and has to stay quiet where there is nobody to notify. --- apps/web/lib/coursemap/notifications.ts | 89 +++++ apps/web/tests/notifications-menu.test.tsx | 139 +++++++ apps/web/ui/shell/notifications-menu.tsx | 443 +++++++++++++++------ 3 files changed, 557 insertions(+), 114 deletions(-) create mode 100644 apps/web/lib/coursemap/notifications.ts create mode 100644 apps/web/tests/notifications-menu.test.tsx diff --git a/apps/web/lib/coursemap/notifications.ts b/apps/web/lib/coursemap/notifications.ts new file mode 100644 index 00000000..060ac1d7 --- /dev/null +++ b/apps/web/lib/coursemap/notifications.ts @@ -0,0 +1,89 @@ +"use server"; + +import { getAuthViewer } from "@/lib/auth/viewer"; +import { createClient } from "@/lib/supabase/server"; + +export type Notification = { + id: string; + /** + * The database constraint decides which kinds exist. The bell draws the ones + * it recognises with their own icon and anything newer with the bell itself, + * so a row added by a later migration is never invisible to an older client. + */ + kind: string; + title: string; + body: string | null; + href: string | null; + readAt: string | null; + createdAt: string; +}; + +export type NotificationInbox = { + notifications: Notification[]; + /** Unread rows across the whole inbox, not only the page that was read. */ + unreadCount: number; +}; + +/** One screenful. The bell is a glance, not a history. */ +const INBOX_LIMIT = 20; + +/** + * A signed-out visitor, and a deployment with no Supabase configured, both have + * an empty inbox rather than a failure. The bell sits in the shell on every + * page, so it must stay quiet where there is nobody to notify. + */ +const EMPTY_INBOX: NotificationInbox = { notifications: [], unreadCount: 0 }; + +/** + * The newest notifications belonging to the signed-in user. Own-row policies do + * the filtering, so this never names a user; a request without a session simply + * reads nothing. + */ +export async function loadNotifications(): Promise { + if (!(await getAuthViewer())) return EMPTY_INBOX; + const supabase = await createClient(); + + const [{ data, error }, { count, error: countError }] = await Promise.all([ + supabase + .from("notifications") + .select("id,kind,title,body,href,read_at,created_at") + .order("created_at", { ascending: false }) + .limit(INBOX_LIMIT), + supabase + .from("notifications") + .select("id", { count: "exact", head: true }) + .is("read_at", null), + ]); + if (error) throw error; + if (countError) throw countError; + + return { + notifications: (data ?? []).map((row) => ({ + id: row.id, + kind: row.kind, + title: row.title, + body: row.body, + href: row.href, + readAt: row.read_at, + createdAt: row.created_at, + })), + unreadCount: count ?? 0, + }; +} + +/** + * Marks notifications read. Omitting the ids marks every unread row. The + * database function scopes the update to the caller, so the ids are a + * narrowing rather than a permission. + */ +export async function markNotificationsRead( + notificationIds?: string[], +): Promise { + if (!(await getAuthViewer())) return 0; + const supabase = await createClient(); + const { data, error } = await supabase.rpc("mark_notifications_read", { + p_notification_ids: notificationIds, + }); + if (error) throw error; + return data ?? 0; +} diff --git a/apps/web/tests/notifications-menu.test.tsx b/apps/web/tests/notifications-menu.test.tsx new file mode 100644 index 00000000..b907393d --- /dev/null +++ b/apps/web/tests/notifications-menu.test.tsx @@ -0,0 +1,139 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { beforeEach, expect, test, vi } from "vitest"; +import { NotificationsMenu } from "../ui/shell/notifications-menu"; + +const inbox = vi.hoisted(() => ({ + loadNotifications: vi.fn(), + markNotificationsRead: vi.fn(), +})); +vi.mock("@/lib/coursemap/notifications", () => inbox); + +function notification(overrides: Record = {}) { + return { + id: "run-1", + kind: "import_run", + title: "Import run #7 completed", + body: "3 records ready to review.", + href: "/admin/courses/imports?run=7", + readAt: null, + createdAt: new Date().toISOString(), + ...overrides, + }; +} + +beforeEach(() => { + vi.resetAllMocks(); + inbox.markNotificationsRead.mockResolvedValue(1); +}); + +test("an empty inbox reads as finished, not as a failure", async () => { + inbox.loadNotifications.mockResolvedValue({ + notifications: [], + unreadCount: 0, + }); + const user = userEvent.setup(); + render(); + + const bell = screen.getByRole("button", { name: "Notifications" }); + await user.click(bell); + + expect(await screen.findByText("You are all caught up")).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /try again/i })).toBeNull(); + expect( + screen.queryByRole("button", { name: /mark all as read/i }), + ).toBeNull(); +}); + +test("the bell counts unread rows and opening does not mark them read", async () => { + inbox.loadNotifications.mockResolvedValue({ + notifications: [ + notification(), + notification({ + id: "risk-1", + kind: "plan_risk", + title: "COMP2400 lost its Semester 2 offering", + body: null, + href: null, + readAt: new Date().toISOString(), + }), + ], + unreadCount: 1, + }); + const user = userEvent.setup(); + render(); + + const bell = await screen.findByRole("button", { + name: "Notifications, 1 unread", + }); + await user.click(bell); + + expect( + await screen.findByText("Import run #7 completed"), + ).toBeInTheDocument(); + expect( + screen.getByText("COMP2400 lost its Semester 2 offering"), + ).toBeInTheDocument(); + expect(screen.getAllByText("Unread")).toHaveLength(1); + expect(inbox.markNotificationsRead).not.toHaveBeenCalled(); +}); + +test("opening a notification marks that row read and clears the count", async () => { + inbox.loadNotifications.mockResolvedValue({ + notifications: [notification()], + unreadCount: 1, + }); + const user = userEvent.setup(); + render(); + + await user.click( + await screen.findByRole("button", { name: "Notifications, 1 unread" }), + ); + await user.click( + await screen.findByRole("link", { name: /Import run #7 completed/ }), + ); + + expect(inbox.markNotificationsRead).toHaveBeenCalledWith(["run-1"]); + await waitFor(() => + expect( + screen.getByRole("button", { name: "Notifications" }), + ).toBeInTheDocument(), + ); +}); + +test("mark all as read empties the count in one action", async () => { + inbox.loadNotifications.mockResolvedValue({ + notifications: [notification(), notification({ id: "run-2" })], + unreadCount: 2, + }); + const user = userEvent.setup(); + render(); + + await user.click( + await screen.findByRole("button", { name: "Notifications, 2 unread" }), + ); + await user.click( + await screen.findByRole("button", { name: "Mark all as read" }), + ); + + expect(inbox.markNotificationsRead).toHaveBeenCalledWith(undefined); + expect(screen.queryByText("Unread")).toBeNull(); +}); + +test("an unreachable inbox says so and offers another attempt", async () => { + inbox.loadNotifications.mockRejectedValue(new Error("offline")); + const user = userEvent.setup(); + render(); + + await user.click(screen.getByRole("button", { name: "Notifications" })); + + expect( + await screen.findByText("Notifications are unavailable"), + ).toBeInTheDocument(); + inbox.loadNotifications.mockResolvedValue({ + notifications: [], + unreadCount: 0, + }); + await user.click(screen.getByRole("button", { name: "Try again" })); + expect(await screen.findByText("You are all caught up")).toBeInTheDocument(); +}); diff --git a/apps/web/ui/shell/notifications-menu.tsx b/apps/web/ui/shell/notifications-menu.tsx index 37b12356..347f498b 100644 --- a/apps/web/ui/shell/notifications-menu.tsx +++ b/apps/web/ui/shell/notifications-menu.tsx @@ -1,153 +1,368 @@ "use client"; -import { useState } from "react"; +import Link from "next/link"; +import { useCallback, useEffect, useState, useTransition } from "react"; import { + AlertCircle, Bell, BookOpen, CalendarDays, CheckCheck, - Sparkles, + Inbox, + PackageCheck, + TriangleAlert, + type LucideIcon, } from "lucide-react"; import { Badge } from "@coursemap/ui/components/badge"; import { Button } from "@coursemap/ui/primitives/button"; import { - DropdownMenu, - DropdownMenuContent, - DropdownMenuGroup, - DropdownMenuItem, - DropdownMenuLabel, - DropdownMenuSeparator, - DropdownMenuTrigger, -} from "@coursemap/ui/primitives/dropdown-menu"; + Empty, + EmptyDescription, + EmptyHeader, + EmptyMedia, + EmptyTitle, +} from "@coursemap/ui/primitives/empty"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "@coursemap/ui/primitives/popover"; +import { ScrollArea } from "@coursemap/ui/primitives/scroll-area"; +import { Skeleton } from "@coursemap/ui/primitives/skeleton"; +import { + loadNotifications, + markNotificationsRead, + type Notification, +} from "@/lib/coursemap/notifications"; +import { cn } from "@/lib/cn"; + +/** + * A row carrying a kind this build has not heard of still draws, with the bell + * itself, rather than dropping out of the inbox. + */ +const kindIcons: Record = { + import_run: PackageCheck, + key_date: CalendarDays, + plan_risk: TriangleAlert, + published_change: BookOpen, +}; + +const relative = new Intl.RelativeTimeFormat("en-AU", { numeric: "auto" }); +const MINUTE = 60_000; +const HOUR = 60 * MINUTE; +const DAY = 24 * HOUR; + +/** + * Coarse and forgiving. The exact minute never matters in an inbox, and the + * value is recomputed on every render rather than ticking, so an open menu + * cannot disagree with itself. + */ +function timeAgo(iso: string) { + const elapsed = Date.now() - new Date(iso).getTime(); + if (!Number.isFinite(elapsed)) return ""; + if (elapsed < MINUTE) return "Just now"; + if (elapsed < HOUR) + return relative.format(-Math.round(elapsed / MINUTE), "minute"); + if (elapsed < DAY) + return relative.format(-Math.round(elapsed / HOUR), "hour"); + if (elapsed < 7 * DAY) + return relative.format(-Math.round(elapsed / DAY), "day"); + return new Intl.DateTimeFormat("en-AU", { + day: "numeric", + month: "short", + }).format(new Date(iso)); +} + +/** + * Today and Earlier, not New and Read. Grouping on read state would make a row + * jump to another heading the moment it was read, which loses the reader's + * place; grouping on age keeps every row where it was put. + */ +function groupOf(iso: string) { + return Date.now() - new Date(iso).getTime() < DAY ? "Today" : "Earlier"; +} + +function InboxSkeleton() { + return ( +
+ {Array.from({ length: 3 }).map((_, index) => ( +
+ +
+ + +
+
+ ))} + Loading notifications... +
+ ); +} + +function NotificationRow({ + notification, + onOpen, +}: { + notification: Notification; + onOpen: () => void; +}) { + const Icon = kindIcons[notification.kind] ?? Bell; + const unread = notification.readAt === null; + const content = ( + <> + + + + + {notification.title} + + {notification.body && ( + + {notification.body} + + )} + + {timeAgo(notification.createdAt)} + + + {unread ? ( + <> +