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)
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
@@ -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(
|
||||
|
||||
@@ -30,6 +30,10 @@ export interface MarkAllReadResponse {
|
||||
marked_count: number;
|
||||
}
|
||||
|
||||
export interface ClearAllResponse {
|
||||
cleared_count: number;
|
||||
}
|
||||
|
||||
export const getNotifications = async (): Promise<NotificationListResponse> => {
|
||||
const response =
|
||||
await apiClient.get<NotificationListResponse>("/notifications");
|
||||
@@ -62,3 +66,8 @@ export const markAllNotificationsRead = async (): Promise<number> => {
|
||||
export const dismissNotification = async (id: string): Promise<void> => {
|
||||
await apiClient.delete(`/notifications/${id}`);
|
||||
};
|
||||
|
||||
export const clearAllNotifications = async (): Promise<number> => {
|
||||
const response = await apiClient.delete<ClearAllResponse>("/notifications");
|
||||
return response.data.cleared_count;
|
||||
};
|
||||
|
||||
@@ -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(<NotificationCenter />, { 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(<NotificationCenter />, { wrapper });
|
||||
fireEvent.click(screen.getByRole("button", { name: /notifications/i }));
|
||||
|
||||
@@ -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
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
className="notification-clear-all"
|
||||
onClick={() => {
|
||||
void clearAll();
|
||||
}}
|
||||
>
|
||||
Clear all
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -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<void>;
|
||||
markAllRead: () => Promise<void>;
|
||||
clearAll: () => Promise<void>;
|
||||
dismiss: (id: string) => Promise<void>;
|
||||
refreshList: () => Promise<void>;
|
||||
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,
|
||||
|
||||
+22
-1
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user