From 2b5223097fa97ca35bac0f01406b3606aedb6e7b Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Fri, 29 May 2026 16:42:31 +0200 Subject: [PATCH] feat: filter notifications to warnings/errors/ready only and add clear-all button Notification filtering: - lifecycle_hooks.py: only instance.error and instance.health_changed with status=running generate notifications. All other lifecycle events (created, started, stopped, restarted, deleted) are filtered out. - health_monitor.py: only error and unhealthy states generate notifications. Running/recovered state no longer creates info notifications. - _derive_title now maps instance.health_changed to "Container ready". Clear-all button: - Added dismiss_all() to NotificationService - Added DELETE /notifications endpoint for bulk dismiss - Frontend: clearAllNotifications API, clearAll in notification context, "Clear all" button in notification drawer alongside "Mark all as read" - Added CSS for .notification-clear-all with danger hover state - Updated notification-center tests Quality gates: pytest (21 passed), vitest (11 passed) --- apps/api/src/api/notifications.py | 16 +++++- apps/api/src/services/health_monitor.py | 14 +++--- apps/api/src/services/lifecycle_hooks.py | 31 +++++++++--- apps/api/src/services/notification_service.py | 26 ++++++++++ apps/api/tests/unit/test_lifecycle_hooks.py | 49 +++++++++++++++++++ .../tests/unit/test_notification_service.py | 49 +++++++++++++++++++ apps/web/src/api/notifications.ts | 9 ++++ .../components/notification-center.test.tsx | 21 ++++++++ .../src/components/notification-center.tsx | 10 ++++ apps/web/src/state/notifications.tsx | 21 ++++++++ apps/web/src/styles.css | 23 ++++++++- 11 files changed, 253 insertions(+), 16 deletions(-) create mode 100644 apps/api/tests/unit/test_lifecycle_hooks.py diff --git a/apps/api/src/api/notifications.py b/apps/api/src/api/notifications.py index bf5259e..adaacb2 100644 --- a/apps/api/src/api/notifications.py +++ b/apps/api/src/api/notifications.py @@ -47,6 +47,10 @@ class MarkAllReadResponse(BaseModel): marked_count: int +class ClearAllResponse(BaseModel): + cleared_count: int + + async def _get_mute_categories( session: AsyncSession, user_id: uuid.UUID, @@ -137,7 +141,7 @@ async def dismiss_notification( user: User = Depends(get_current_user), session: AsyncSession = Depends(get_db_session), ) -> None: - """Soft-delete (dismiss) a notification.""" + """Soft-delete (dismiss) a single notification.""" try: await notification_service.dismiss(session, notification_id, user.id) except ValueError as exc: @@ -145,3 +149,13 @@ async def dismiss_notification( status_code=status.HTTP_404_NOT_FOUND, detail="Notification not found", ) from exc + + +@router.delete("", status_code=status.HTTP_200_OK) +async def clear_all_notifications( + user: User = Depends(get_current_user), + session: AsyncSession = Depends(get_db_session), +) -> ClearAllResponse: + """Dismiss all notifications for the authenticated user.""" + cleared = await notification_service.dismiss_all(session, user.id) + return ClearAllResponse(cleared_count=cleared) diff --git a/apps/api/src/services/health_monitor.py b/apps/api/src/services/health_monitor.py index eec060f..9940abd 100644 --- a/apps/api/src/services/health_monitor.py +++ b/apps/api/src/services/health_monitor.py @@ -220,18 +220,18 @@ class HealthMonitor: await self._event_bus.publish(event_type, payload) # Create notification for instance owner (fire-and-forget) + # Only send warnings and errors; skip "recovered" info notifications. if new_status == "error": category = "instance" severity = "error" title = "Container failed" - else: + elif new_status == "unhealthy": category = "health" - if new_status == "unhealthy": - severity = "warning" - title = "Container unhealthy" - else: - severity = "info" - title = "Container recovered" + severity = "warning" + title = "Container unhealthy" + else: + # Running/recovered — do not notify + return try: await notification_service.create_notification( diff --git a/apps/api/src/services/lifecycle_hooks.py b/apps/api/src/services/lifecycle_hooks.py index 17c07e8..c3e802e 100644 --- a/apps/api/src/services/lifecycle_hooks.py +++ b/apps/api/src/services/lifecycle_hooks.py @@ -24,6 +24,7 @@ def _derive_title(event_type: str) -> str: "instance.restarted": "Container restarted", "instance.deleted": "Container deleted", "instance.error": "Container error", + "instance.health_changed": "Container ready", } return mapping.get( event_type, @@ -31,6 +32,21 @@ def _derive_title(event_type: str) -> str: ) +def _should_notify(event_type: str, status: str | None) -> bool: + """Determine whether a lifecycle event should generate a notification. + + Only warnings, errors, and "container is ready" (health_changed running) + are sent to users. + """ + if event_type == "instance.error": + return True + if event_type == "instance.health_changed" and status == "running": + return True + # Filter out: created, started, stopped, restarted, deleted, and any + # health_changed that is not "running" (unhealthy is handled by health_monitor) + return False + + def _build_payload( event_type: str, instance: ToolInstance, @@ -118,15 +134,16 @@ async def publish_lifecycle_event( await event_bus.publish(event_type, payload) # Create notification for instance owner (fire-and-forget) - # Skip intermediate "starting" notifications — only notify on terminal states - # (failed or successful attempts) - _is_starting_intermediate = event_type == "instance.started" and ( - status or instance.status - ) == "starting" - if _is_starting_intermediate: + # Only send warnings, errors, and "container is ready" notifications. + effective_status = status or instance.status + if not _should_notify(event_type, effective_status): return - severity = "error" if event_type == "instance.error" else "info" + severity = ( + "error" + if event_type == "instance.error" + else "success" + ) title = _derive_title(event_type) try: diff --git a/apps/api/src/services/notification_service.py b/apps/api/src/services/notification_service.py index e02d7cf..a6f4264 100644 --- a/apps/api/src/services/notification_service.py +++ b/apps/api/src/services/notification_service.py @@ -195,6 +195,32 @@ class NotificationService: await session.commit() return result.rowcount or 0 + async def dismiss_all( + self, + session: AsyncSession, + user_id: uuid.UUID, + ) -> int: + """Soft-delete all non-dismissed notifications for a user. + + Args: + session: Database session. + user_id: Owner of the notifications. + + Returns: + Number of rows updated. + """ + stmt = ( + update(Notification) + .where( + Notification.user_id == user_id, + Notification.dismissed_at.is_(None), + ) + .values(dismissed_at=datetime.now(timezone.utc)) + ) + result: CursorResult[Any] = await session.execute(stmt) # type: ignore[assignment] + await session.commit() + return result.rowcount or 0 + async def dismiss( self, session: AsyncSession, diff --git a/apps/api/tests/unit/test_lifecycle_hooks.py b/apps/api/tests/unit/test_lifecycle_hooks.py new file mode 100644 index 0000000..af5676e --- /dev/null +++ b/apps/api/tests/unit/test_lifecycle_hooks.py @@ -0,0 +1,49 @@ +"""Unit tests for lifecycle hook helpers.""" + +import pytest + +from src.services.lifecycle_hooks import _derive_title, _should_notify + + +class TestDeriveTitle: + """Tests for _derive_title.""" + + def test_known_event_types(self) -> None: + assert _derive_title("instance.created") == "Container created" + assert _derive_title("instance.started") == "Container started" + assert _derive_title("instance.stopped") == "Container stopped" + assert _derive_title("instance.restarted") == "Container restarted" + assert _derive_title("instance.deleted") == "Container deleted" + assert _derive_title("instance.error") == "Container error" + assert _derive_title("instance.health_changed") == "Container ready" + + def test_unknown_event_type(self) -> None: + assert _derive_title("instance.custom_event") == "Custom Event" + + +class TestShouldNotify: + """Tests for _should_notify filtering.""" + + def test_error_events_are_notified(self) -> None: + assert _should_notify("instance.error", "error") is True + assert _should_notify("instance.error", None) is True + + def test_health_changed_running_is_notified(self) -> None: + assert _should_notify("instance.health_changed", "running") is True + + def test_created_started_stopped_restarted_deleted_filtered(self) -> None: + for event in [ + "instance.created", + "instance.started", + "instance.stopped", + "instance.restarted", + "instance.deleted", + ]: + assert _should_notify(event, "pending") is False + assert _should_notify(event, "running") is False + assert _should_notify(event, None) is False + + def test_health_changed_non_running_filtered(self) -> None: + assert _should_notify("instance.health_changed", "unhealthy") is False + assert _should_notify("instance.health_changed", "starting") is False + assert _should_notify("instance.health_changed", None) is False diff --git a/apps/api/tests/unit/test_notification_service.py b/apps/api/tests/unit/test_notification_service.py index 4088b45..d619e5e 100644 --- a/apps/api/tests/unit/test_notification_service.py +++ b/apps/api/tests/unit/test_notification_service.py @@ -308,6 +308,55 @@ async def test_get_unread_count_excludes_dismissed( assert count == 0 +@pytest.mark.unit +@pytest.mark.asyncio +async def test_dismiss_all_affects_all_non_dismissed( + db_session: AsyncSession, + notification_service: NotificationService, + user_a: User, +) -> None: + for i in range(4): + await notification_service.create_notification( + db_session, + user_a.id, + category="instance", + severity="info", + title=f"Notification {i}", + ) + + cleared = await notification_service.dismiss_all(db_session, user_a.id) + + assert cleared == 4 + items, total = await notification_service.list_notifications(db_session, user_a.id) + assert total == 0 + + +@pytest.mark.unit +@pytest.mark.asyncio +async def test_dismiss_all_affects_only_caller( + db_session: AsyncSession, + notification_service: NotificationService, + user_a: User, + user_b: User, +) -> None: + for i in range(3): + await notification_service.create_notification( + db_session, user_a.id, category="instance", severity="info", title=f"A-{i}" + ) + for i in range(2): + await notification_service.create_notification( + db_session, user_b.id, category="instance", severity="info", title=f"B-{i}" + ) + + cleared = await notification_service.dismiss_all(db_session, user_a.id) + + assert cleared == 3 + items_a, total_a = await notification_service.list_notifications(db_session, user_a.id) + items_b, total_b = await notification_service.list_notifications(db_session, user_b.id) + assert total_a == 0 + assert total_b == 2 + + @pytest.mark.unit @pytest.mark.asyncio async def test_mark_all_read_affects_only_caller( diff --git a/apps/web/src/api/notifications.ts b/apps/web/src/api/notifications.ts index 5b0189f..9865b25 100644 --- a/apps/web/src/api/notifications.ts +++ b/apps/web/src/api/notifications.ts @@ -30,6 +30,10 @@ export interface MarkAllReadResponse { marked_count: number; } +export interface ClearAllResponse { + cleared_count: number; +} + export const getNotifications = async (): Promise => { const response = await apiClient.get("/notifications"); @@ -62,3 +66,8 @@ export const markAllNotificationsRead = async (): Promise => { export const dismissNotification = async (id: string): Promise => { await apiClient.delete(`/notifications/${id}`); }; + +export const clearAllNotifications = async (): Promise => { + const response = await apiClient.delete("/notifications"); + return response.data.cleared_count; +}; diff --git a/apps/web/src/components/notification-center.test.tsx b/apps/web/src/components/notification-center.test.tsx index 9ef712c..780e7b8 100644 --- a/apps/web/src/components/notification-center.test.tsx +++ b/apps/web/src/components/notification-center.test.tsx @@ -9,6 +9,7 @@ vi.mock("../api/notifications", () => ({ markNotificationRead: vi.fn(), markAllNotificationsRead: vi.fn(), dismissNotification: vi.fn(), + clearAllNotifications: vi.fn(), })); import { getNotifications, getUnreadCount } from "../api/notifications"; @@ -146,6 +147,26 @@ describe("NotificationCenter", () => { expect(vi.mocked(mockMarkAll)).toHaveBeenCalled(); }); + it("calls clearAll on clear-all button click", async () => { + mockedGetNotifications.mockResolvedValue({ + items: [makeNotification("1")], + total: 1, + limit: 20, + offset: 0, + }); + + render(, { wrapper }); + fireEvent.click(screen.getByRole("button", { name: /notifications/i })); + + await vi.advanceTimersByTimeAsync(100); + fireEvent.click(screen.getByRole("button", { name: /clear all/i })); + + const { clearAllNotifications: mockClearAll } = await import( + "../api/notifications" + ); + expect(vi.mocked(mockClearAll)).toHaveBeenCalled(); + }); + it("refreshes list immediately on open", async () => { render(, { wrapper }); fireEvent.click(screen.getByRole("button", { name: /notifications/i })); diff --git a/apps/web/src/components/notification-center.tsx b/apps/web/src/components/notification-center.tsx index dda5667..dc5f431 100644 --- a/apps/web/src/components/notification-center.tsx +++ b/apps/web/src/components/notification-center.tsx @@ -15,6 +15,7 @@ export function NotificationCenter({ unreadCount, markRead, markAllRead, + clearAll, dismiss, refreshList, isDropdownOpen, @@ -115,6 +116,15 @@ export function NotificationCenter({ > Mark all as read + )} diff --git a/apps/web/src/state/notifications.tsx b/apps/web/src/state/notifications.tsx index c8a76b0..1bb17cb 100644 --- a/apps/web/src/state/notifications.tsx +++ b/apps/web/src/state/notifications.tsx @@ -11,6 +11,7 @@ import { markNotificationRead, markAllNotificationsRead, dismissNotification, + clearAllNotifications, } from "../api/notifications"; import type { NotificationItem } from "../api/notifications"; @@ -21,6 +22,7 @@ export interface NotificationContextValue { error: Error | null; markRead: (id: string) => Promise; markAllRead: () => Promise; + clearAll: () => Promise; dismiss: (id: string) => Promise; refreshList: () => Promise; isDropdownOpen: boolean; @@ -265,6 +267,24 @@ export function NotificationProvider({ await fetchList(); }, [fetchList]); + const clearAll = useCallback(async () => { + const { notifications: currentNotifications } = stateRef.current; + const unreadInList = currentNotifications.filter( + (n) => n.read_at === null, + ).length; + + setNotifications([]); + setUnreadCount((c) => Math.max(0, c - unreadInList)); + setError(null); + + try { + await clearAllNotifications(); + } catch (err) { + setNotifications(currentNotifications); + setError(err as Error); + } + }, []); + const value: NotificationContextValue = { notifications, unreadCount, @@ -272,6 +292,7 @@ export function NotificationProvider({ error, markRead, markAllRead, + clearAll, dismiss, refreshList, isDropdownOpen, diff --git a/apps/web/src/styles.css b/apps/web/src/styles.css index f6a512f..ff600d1 100644 --- a/apps/web/src/styles.css +++ b/apps/web/src/styles.css @@ -4602,10 +4602,12 @@ a:active, padding: 0.75rem 1rem; border-top: 1px solid var(--border); flex-shrink: 0; + display: flex; + gap: 0.5rem; } .notification-mark-all { - width: 100%; + flex: 1; padding: 0.5rem 0.75rem; background: transparent; border: 1px solid var(--border); @@ -4623,6 +4625,25 @@ a:active, border-color: var(--brand); } +.notification-clear-all { + flex: 1; + padding: 0.5rem 0.75rem; + background: transparent; + border: 1px solid var(--border); + border-radius: 8px; + color: var(--muted); + font: inherit; + font-size: 0.85rem; + cursor: pointer; + transition: all 0.15s ease; +} + +.notification-clear-all:hover { + background: var(--bg); + color: var(--danger); + border-color: var(--danger); +} + /* Notification Item */ .notification-item { display: flex;