fix(admin): address announcement review findings

Author: Programcode

e2e/admin-announcements.spec.ts

@@ -125,7 +125,7 @@ test('admin creates a targeted announcement and deletes it from history', async
     targetUserIds: [TARGET_USER_ID],
   });
 
-  await page.getByRole('button', { name: 'Delete' }).click();
+  await page.getByRole('button', { name: 'Delete announcement: Targeted note' }).click();
   await page.getByRole('button', { name: 'Delete announcement' }).click();
 
   await expect(page.getByRole('heading', { name: 'Targeted note' })).toHaveCount(0);

src/components/admin/AdminAnnouncements.tsx

@@ -335,6 +335,7 @@ export function AdminAnnouncements({ users }: AdminAnnouncementsProps) {
                       variant="ghost"
                       size="sm"
                       className="self-start text-destructive hover:text-destructive"
+                      aria-label={`Delete announcement: ${announcement.title}`}
                       onClick={() => setDeleteTarget(announcement)}
                     >
                       <Trash2 className="mr-2 h-4 w-4" aria-hidden="true" />

src/components/layout/AppLayout.tsx

@@ -50,7 +50,7 @@ export function AppLayout() {
   const [mobileMenuOpen, setMobileMenuOpen] = useState(false);
   const [announcements, setAnnouncements] = useState<UserAnnouncement[]>([]);
   const [dismissingId, setDismissingId] = useState<string | null>(null);
-  const [dismissError, setDismissError] = useState<string | null>(null);
+  const [dismissError, setDismissError] = useState<{ announcementId: string; message: string } | null>(null);
   const { t } = useTranslation();
   const location = useLocation();
 
@@ -79,7 +79,10 @@ export function AppLayout() {
       await dismissAnnouncement(announcementId);
       setAnnouncements((current) => current.filter((announcement) => announcement.id !== announcementId));
     } catch (error) {
-      setDismissError(error instanceof Error ? error.message : 'Could not dismiss this announcement.');
+      setDismissError({
+        announcementId,
+        message: error instanceof Error ? error.message : 'Could not dismiss this announcement.',
+      });
     } finally {
       setDismissingId(null);
     }
@@ -175,7 +178,7 @@ export function AppLayout() {
           position={1}
           total={announcements.length}
           dismissing={dismissingId === announcements[0].id}
-          error={dismissError}
+          error={dismissError?.announcementId === announcements[0].id ? dismissError.message : null}
           onDismiss={() => handleDismissAnnouncement(announcements[0].id)}
         />
       )}

src/lib/announcements.ts

@@ -124,7 +124,9 @@ function normalizeAdminAnnouncement(row: AdminAnnouncementRow): AdminAnnouncemen
 }
 
 function toIsoOrUndefined(value: string | undefined) {
-  return value ? new Date(value).toISOString() : undefined;
+  if (!value) return undefined;
+  const date = new Date(value);
+  return Number.isNaN(date.getTime()) ? value : date.toISOString();
 }
 
 export async function listActiveAnnouncements(signal?: AbortSignal): Promise<UserAnnouncement[]> {

src/test/announcements.test.ts

@@ -0,0 +1,55 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { createAnnouncement } from '@/lib/announcements';
+
+const { mockSupabase } = vi.hoisted(() => ({
+  mockSupabase: {
+    auth: {
+      getSession: vi.fn(),
+    },
+  },
+}));
+
+vi.mock('@/lib/supabase/client', () => ({
+  supabase: mockSupabase,
+  SUPABASE_URL: 'http://localhost:54321',
+}));
+
+describe('announcement API client', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockSupabase.auth.getSession.mockResolvedValue({
+      data: { session: { access_token: 'test-token' } },
+    });
+  });
+
+  it('passes invalid non-empty dates through for server-side validation', async () => {
+    let requestBody: Record<string, unknown> = {};
+    vi.spyOn(globalThis, 'fetch').mockImplementation(async (_input, init) => {
+      requestBody = JSON.parse(String(init?.body ?? '{}'));
+      return new Response(JSON.stringify({
+        announcement: {
+          id: 'aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee',
+          title: 'Date validation',
+          body: 'The server should reject invalid dates.',
+          target_type: 'all',
+          target_user_ids: null,
+          starts_at: '2026-08-31T12:00:00+00:00',
+          expires_at: null,
+          created_by: '99999999-9999-4999-8999-999999999999',
+          created_at: '2026-08-31T12:00:00+00:00',
+        },
+      }), { status: 201, headers: { 'Content-Type': 'application/json' } });
+    });
+
+    await createAnnouncement({
+      title: 'Date validation',
+      body: 'The server should reject invalid dates.',
+      targetType: 'all',
+      startsAt: 'not-a-date',
+      expiresAt: 'also-not-a-date',
+    });
+
+    expect(requestBody.startsAt).toBe('not-a-date');
+    expect(requestBody.expiresAt).toBe('also-not-a-date');
+  });
+});

src/test/components/AppLayout.test.tsx

@@ -67,6 +67,34 @@ describe('AppLayout announcements', () => {
     await waitFor(() => expect(mockListActiveAnnouncements).toHaveBeenCalledTimes(2));
   });
 
+  it('does not show a stale dismissal error under the next announcement', async () => {
+    const nextAnnouncement: UserAnnouncement = {
+      ...announcement,
+      id: 'bbbbbbbb-cccc-4ddd-8eee-ffffffffffff',
+      title: 'New announcement',
+      body: 'This message replaced the previous one.',
+    };
+    let rejectDismissal: (reason: Error) => void = () => undefined;
+    const pendingDismissal = new Promise<void>((_resolve, reject) => {
+      rejectDismissal = reject;
+    });
+    mockListActiveAnnouncements
+      .mockResolvedValueOnce([announcement])
+      .mockResolvedValueOnce([nextAnnouncement]);
+    mockDismissAnnouncement.mockReturnValue(pendingDismissal);
+
+    renderLayout();
+    await screen.findByText('Scheduled maintenance');
+    fireEvent.click(screen.getByRole('button', { name: 'Dismiss announcement: Scheduled maintenance' }));
+    fireEvent.click(screen.getByRole('link', { name: 'Open next page' }));
+    await screen.findByText('New announcement');
+
+    rejectDismissal(new Error('Could not dismiss the old announcement'));
+
+    await waitFor(() => expect(mockDismissAnnouncement).toHaveBeenCalledWith(announcement.id));
+    expect(screen.queryByText('Could not dismiss the old announcement')).not.toBeInTheDocument();
+  });
+
   it('keeps the app usable when announcements fail to load', async () => {
     mockListActiveAnnouncements.mockRejectedValue(new Error('Network unavailable'));
     renderLayout();

src/test/pages/AdminPage.test.tsx

@@ -505,7 +505,7 @@ describe('AdminPage Integration', () => {
         await waitFor(() => expect(screen.getByText('Admin Dashboard')).toBeInTheDocument());
         fireEvent.mouseDown(screen.getByRole('tab', { name: 'Announcements' }), { button: 0 });
         await screen.findByText('Remove me');
-        fireEvent.click(screen.getByRole('button', { name: 'Delete' }));
+        fireEvent.click(screen.getByRole('button', { name: 'Delete announcement: Remove me' }));
         fireEvent.click(await screen.findByRole('button', { name: 'Delete announcement' }));
 
         await waitFor(() => expect(deleteBody.announcementId).toBe(announcementId));

supabase/migrations/20260831152911_admin_announcements.sql

@@ -13,7 +13,7 @@ CREATE TABLE public.admin_announcements (
   CONSTRAINT admin_announcements_target_shape CHECK (
     (target_type = 'all' AND target_user_ids IS NULL)
     OR
-    (target_type = 'users' AND cardinality(target_user_ids) > 0)
+    (target_type = 'users' AND target_user_ids IS NOT NULL AND cardinality(target_user_ids) > 0)
   ),
   CONSTRAINT admin_announcements_valid_window CHECK (
     expires_at IS NULL OR expires_at > starts_at

supabase/tests/admin_announcements_rls_test.sql

@@ -1,5 +1,5 @@
 BEGIN;
-SELECT plan(13);
+SELECT plan(14);
 
 INSERT INTO auth.users (id)
 VALUES
@@ -23,6 +23,14 @@ VALUES
   ('aaaaaaaa-0000-4000-8000-000000000004', 'Future', 'Not active yet.', 'all', NULL, now() + interval '1 day', NULL, '99999999-9999-4999-8999-999999999999'),
   ('aaaaaaaa-0000-4000-8000-000000000005', 'Expired', 'No longer active.', 'all', NULL, now() - interval '2 days', now() - interval '1 day', '99999999-9999-4999-8999-999999999999');
 
+SELECT throws_ok(
+  $$INSERT INTO public.admin_announcements (title, body, target_type, target_user_ids, created_by)
+    VALUES ('Missing recipients', 'This must fail.', 'users', NULL, '99999999-9999-4999-8999-999999999999')$$,
+  '23514',
+  NULL,
+  'targeted announcements require a non-null recipient array'
+);
+
 SELECT ok(
   has_column_privilege('authenticated', 'public.admin_announcements', 'title', 'SELECT'),
   'authenticated users can read public announcement fields'