From 434bfed97788548aa0be2f7d05be3bd21a9867a3 Mon Sep 17 00:00:00 2001 From: Jaspreet Singh <6873201+tagpro@users.noreply.github.com> Date: Sat, 11 Jul 2026 16:23:52 +1000 Subject: [PATCH 1/2] fix(notifications): paginate and index the notification list endpoints The notification tab loaded slowly because both list endpoints returned the entire notification table. GET /notification and GET /notification/unread ran `select(Notification).order_by(timestamp desc)` with no limit, so every page load fetched every row, pydantic-validated each one, serialised the lot to JSON and shipped it to the browser. On the instance this was found on the table had ~437,000 rows. The table also had no index beyond the primary key on `id`, so the ORDER BY sorted the whole table on every load and the unread filter was a full scan. Backend: - Both list endpoints now take `limit` (default 50, max 200) and `offset` (>= 0), threaded through the service into `.limit()/.offset()` in the SQL. Ordering stays timestamp descending. The number of matching notifications is returned in the `X-Total-Count` response header (exposed via CORS) so the UI can paginate. - GET /notification takes an optional `read` filter, so the UI can page through read notifications rather than fetching everything and filtering client-side. - New migration indexes the table for these queries: an index on `timestamp` descending for the ORDER BY, and a partial index for the unread query. - New PATCH /notification/read marks every unread notification read in a single statement. The UI used to do this by looping a PATCH over every notification it held, which only worked because it held all of them. Frontend: - The notifications page now loads a page of 50 with "load more", and does not fetch read notifications at all until that section is expanded. Tests: - Repository tests covering the limit cap, offset, timestamp-descending ordering, the unread query, and the bulk mark-as-read, seeded with more rows than the page size so pagination is actually exercised. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019wrZPxJDip5x3mzjWnpwos --- ...2_index_notification_timestamp_and_read.py | 44 +++ media_manager/main.py | 1 + media_manager/notification/models.py | 16 +- media_manager/notification/repository.py | 59 +++- media_manager/notification/router.py | 66 ++++- media_manager/notification/service.py | 24 +- pyproject.toml | 11 + tests/__init__.py | 0 tests/conftest.py | 43 +++ tests/test_notification_repository.py | 257 ++++++++++++++++++ uv.lock | 31 +++ web/src/lib/api/api.d.ts | 85 +++++- .../dashboard/notifications/+page.svelte | 228 ++++++++++++---- 13 files changed, 795 insertions(+), 70 deletions(-) create mode 100644 alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py create mode 100644 tests/__init__.py create mode 100644 tests/conftest.py create mode 100644 tests/test_notification_repository.py diff --git a/alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py b/alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py new file mode 100644 index 00000000..af5808e9 --- /dev/null +++ b/alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py @@ -0,0 +1,44 @@ +"""index notification timestamp and read + +Revision ID: b3d51c7fa9e2 +Revises: e60ae827ed98 +Create Date: 2026-07-11 10:12:38.114905 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision: str = 'b3d51c7fa9e2' +down_revision: Union[str, None] = 'e60ae827ed98' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + # The notification list is ordered by timestamp descending and paginated, + # so the index matches that order to keep a page from sorting the table. + op.create_index( + 'ix_notification_timestamp_desc', + 'notification', + [sa.text('timestamp DESC')], + unique=False, + ) + # Unread notifications are a small fraction of the table, so a partial + # index keeps the unread query off a full table scan. + op.create_index( + 'ix_notification_unread_timestamp_desc', + 'notification', + [sa.text('timestamp DESC')], + unique=False, + postgresql_where=sa.text('read = false'), + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_index('ix_notification_unread_timestamp_desc', table_name='notification') + op.drop_index('ix_notification_timestamp_desc', table_name='notification') diff --git a/media_manager/main.py b/media_manager/main.py index a36b4be9..f8a00300 100644 --- a/media_manager/main.py +++ b/media_manager/main.py @@ -141,6 +141,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator: allow_origins=origins, allow_credentials=True, allow_methods=["GET", "PUT", "POST", "DELETE", "PATCH", "HEAD", "OPTIONS"], + expose_headers=["X-Total-Count"], ) app.add_middleware(CorrelationIdMiddleware, header_name="X-Correlation-ID") api_app = APIRouter(prefix="/api/v1") diff --git a/media_manager/notification/models.py b/media_manager/notification/models.py index f7772867..fabd3a5b 100644 --- a/media_manager/notification/models.py +++ b/media_manager/notification/models.py @@ -1,6 +1,6 @@ from uuid import UUID -from sqlalchemy import DateTime +from sqlalchemy import DateTime, Index, text from sqlalchemy.orm import Mapped, mapped_column from media_manager.database import Base @@ -13,3 +13,17 @@ class Notification(Base): message: Mapped[str] read: Mapped[bool] timestamp = mapped_column(DateTime, nullable=False) + + __table_args__ = ( + # Notifications are always listed newest first and are paginated, so the + # index matches that order to keep a page from sorting the whole table. + Index("ix_notification_timestamp_desc", text("timestamp DESC")), + # Unread notifications are a small fraction of the table, so a partial + # index keeps the unread query off a full table scan. + Index( + "ix_notification_unread_timestamp_desc", + text("timestamp DESC"), + postgresql_where=text("read = false"), + sqlite_where=text("read = 0"), + ), + ) diff --git a/media_manager/notification/repository.py b/media_manager/notification/repository.py index 1ab9b7c9..696d5405 100644 --- a/media_manager/notification/repository.py +++ b/media_manager/notification/repository.py @@ -1,12 +1,13 @@ import logging -from sqlalchemy import delete, select, update +from sqlalchemy import delete, func, select, update from sqlalchemy.exc import ( IntegrityError, SQLAlchemyError, ) from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy.sql.expression import false +from sqlalchemy.sql import Select +from sqlalchemy.sql.expression import false, true from media_manager.exceptions import ConflictError, NotFoundError from media_manager.notification.models import Notification @@ -20,6 +21,12 @@ log = logging.getLogger(__name__) +def _filtered_by_read(stmt: Select, read: bool | None) -> Select: + if read is None: + return stmt + return stmt.where(Notification.read == (true() if read else false())) + + class NotificationRepository: def __init__(self, db: AsyncSession) -> None: self.db = db @@ -33,12 +40,15 @@ async def get_notification(self, nid: NotificationId) -> NotificationSchema: return NotificationSchema.model_validate(result) - async def get_unread_notifications(self) -> list[NotificationSchema]: + async def get_unread_notifications( + self, limit: int, offset: int = 0 + ) -> list[NotificationSchema]: try: stmt = ( - select(Notification) - .where(Notification.read == false()) + _filtered_by_read(select(Notification), read=False) .order_by(Notification.timestamp.desc()) + .limit(limit) + .offset(offset) ) results = (await self.db.execute(stmt)).scalars().all() return [ @@ -49,9 +59,21 @@ async def get_unread_notifications(self) -> list[NotificationSchema]: log.exception("Database error while retrieving unread notifications") raise - async def get_all_notifications(self) -> list[NotificationSchema]: + async def get_all_notifications( + self, limit: int, offset: int = 0, read: bool | None = None + ) -> list[NotificationSchema]: + """Return one page of notifications, newest first. + + ``read`` optionally restricts the page to read or unread notifications; + ``None`` returns both. + """ try: - stmt = select(Notification).order_by(Notification.timestamp.desc()) + stmt = ( + _filtered_by_read(select(Notification), read=read) + .order_by(Notification.timestamp.desc()) + .limit(limit) + .offset(offset) + ) results = (await self.db.execute(stmt)).scalars().all() return [ NotificationSchema.model_validate(notification) @@ -61,6 +83,17 @@ async def get_all_notifications(self) -> list[NotificationSchema]: log.exception("Database error while retrieving notifications") raise + async def count_notifications(self, read: bool | None = None) -> int: + """Count the notifications a matching page would be taken from.""" + try: + stmt = _filtered_by_read( + select(func.count()).select_from(Notification), read=read + ) + return (await self.db.execute(stmt)).scalar_one() + except SQLAlchemyError: + log.exception("Database error while counting notifications") + raise + async def save_notification(self, notification: NotificationSchema) -> None: try: self.db.add( @@ -91,6 +124,18 @@ async def mark_notification_as_unread(self, nid: NotificationId) -> None: await self.db.execute(stmt) return + async def mark_all_notifications_as_read(self) -> None: + """Mark every unread notification as read in a single statement. + + The UI only holds a page of notifications now, so it can no longer mark + them all read by looping over the ones it has. + """ + stmt = ( + update(Notification).where(Notification.read == false()).values(read=True) + ) + await self.db.execute(stmt) + return + async def delete_notification(self, nid: NotificationId) -> None: stmt = delete(Notification).where(Notification.id == nid) result = await self.db.execute(stmt) diff --git a/media_manager/notification/router.py b/media_manager/notification/router.py index b40640d4..03202d06 100644 --- a/media_manager/notification/router.py +++ b/media_manager/notification/router.py @@ -1,4 +1,6 @@ -from fastapi import APIRouter, Depends, status +from typing import Annotated + +from fastapi import APIRouter, Depends, Query, Response, status from media_manager.auth.users import current_active_user from media_manager.notification.dependencies import notification_service_dep @@ -6,6 +8,19 @@ router = APIRouter() +DEFAULT_PAGE_SIZE = 50 +MAX_PAGE_SIZE = 200 + +TOTAL_COUNT_HEADER = "X-Total-Count" + +limit_dep = Annotated[ + int, + Query( + ge=1, le=MAX_PAGE_SIZE, description="Maximum number of notifications to return" + ), +] +offset_dep = Annotated[int, Query(ge=0, description="Number of notifications to skip")] + # -------------------------------- # GET NOTIFICATIONS @@ -18,11 +33,25 @@ ) async def get_all_notifications( notification_service: notification_service_dep, + response: Response, + limit: limit_dep = DEFAULT_PAGE_SIZE, + offset: offset_dep = 0, + read: Annotated[ + bool | None, + Query(description="Only return read (true) or unread (false) notifications"), + ] = None, ) -> list[Notification]: """ - Get all notifications. + Get a page of notifications, newest first. + + The total number of matching notifications is returned in the + `X-Total-Count` response header. """ - return await notification_service.get_all_notifications() + total = await notification_service.count_notifications(read=read) + response.headers[TOTAL_COUNT_HEADER] = str(total) + return await notification_service.get_all_notifications( + limit=limit, offset=offset, read=read + ) @router.get( @@ -31,11 +60,21 @@ async def get_all_notifications( ) async def get_unread_notifications( notification_service: notification_service_dep, + response: Response, + limit: limit_dep = DEFAULT_PAGE_SIZE, + offset: offset_dep = 0, ) -> list[Notification]: """ - Get all unread notifications. + Get a page of unread notifications, newest first. + + The total number of unread notifications is returned in the `X-Total-Count` + response header. """ - return await notification_service.get_unread_notifications() + total = await notification_service.count_notifications(read=False) + response.headers[TOTAL_COUNT_HEADER] = str(total) + return await notification_service.get_unread_notifications( + limit=limit, offset=offset + ) @router.get( @@ -59,6 +98,23 @@ async def get_notification( # -------------------------------- +@router.patch( + "/read", + status_code=status.HTTP_204_NO_CONTENT, + dependencies=[Depends(current_active_user)], +) +async def mark_all_notifications_as_read( + notification_service: notification_service_dep, +) -> None: + """ + Mark every unread notification as read. + + The client only holds a page of notifications, so it cannot mark them all + read by iterating over the ones it has. + """ + await notification_service.mark_all_notifications_as_read() + + @router.patch( "/{notification_id}/read", status_code=status.HTTP_204_NO_CONTENT, diff --git a/media_manager/notification/service.py b/media_manager/notification/service.py index ee59af01..980fc6be 100644 --- a/media_manager/notification/service.py +++ b/media_manager/notification/service.py @@ -14,11 +14,22 @@ def __init__( async def get_notification(self, nid: NotificationId) -> Notification: return await self.notification_repository.get_notification(nid=nid) - async def get_unread_notifications(self) -> list[Notification]: - return await self.notification_repository.get_unread_notifications() - - async def get_all_notifications(self) -> list[Notification]: - return await self.notification_repository.get_all_notifications() + async def get_unread_notifications( + self, limit: int, offset: int = 0 + ) -> list[Notification]: + return await self.notification_repository.get_unread_notifications( + limit=limit, offset=offset + ) + + async def get_all_notifications( + self, limit: int, offset: int = 0, read: bool | None = None + ) -> list[Notification]: + return await self.notification_repository.get_all_notifications( + limit=limit, offset=offset, read=read + ) + + async def count_notifications(self, read: bool | None = None) -> int: + return await self.notification_repository.count_notifications(read=read) async def save_notification(self, notification: Notification) -> None: return await self.notification_repository.save_notification(notification) @@ -29,6 +40,9 @@ async def mark_notification_as_read(self, nid: NotificationId) -> None: async def mark_notification_as_unread(self, nid: NotificationId) -> None: return await self.notification_repository.mark_notification_as_unread(nid=nid) + async def mark_all_notifications_as_read(self) -> None: + return await self.notification_repository.mark_all_notifications_as_read() + async def delete_notification(self, nid: NotificationId) -> None: return await self.notification_repository.delete_notification(nid=nid) diff --git a/pyproject.toml b/pyproject.toml index 3775d049..fa31b71a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,6 +44,17 @@ dev = [ "ruff", "ty>=0.0.33", ] +test = [ + "pytest>=8.4.0", + "pytest-asyncio>=1.0.0", + "aiosqlite>=0.21.0", +] + +[tool.pytest.ini_options] +asyncio_mode = "auto" +asyncio_default_fixture_loop_scope = "function" +testpaths = ["tests"] +pythonpath = ["."] [tool.setuptools.packages.find] include = ["media_manager*"] diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..59fb1196 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,43 @@ +"""Shared fixtures for the repository test-suite. + +The tests run against an in-memory SQLite database driven by ``aiosqlite``, so +the suite needs no running PostgreSQL. Note that the indexer models are +deliberately not imported: they use a PostgreSQL-only ARRAY column. +""" + +from collections.abc import AsyncIterator + +import pytest +from sqlalchemy.ext.asyncio import ( + AsyncEngine, + AsyncSession, + async_sessionmaker, + create_async_engine, +) +from sqlalchemy.pool import StaticPool + +from media_manager.database import Base +from media_manager.notification import models as notification_models + +# Importing the model modules is what registers their tables on +# ``Base.metadata``; binding them here keeps the imports from looking unused. +_MODEL_MODULES = (notification_models,) + + +@pytest.fixture +async def engine() -> AsyncIterator[AsyncEngine]: + engine = create_async_engine( + "sqlite+aiosqlite://", + poolclass=StaticPool, + connect_args={"check_same_thread": False}, + ) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + yield engine + await engine.dispose() + + +@pytest.fixture +def session_factory(engine: AsyncEngine) -> async_sessionmaker[AsyncSession]: + """Mirrors the production sessionmaker, notably ``expire_on_commit=False``.""" + return async_sessionmaker(engine, expire_on_commit=False) diff --git a/tests/test_notification_repository.py b/tests/test_notification_repository.py new file mode 100644 index 00000000..21d77158 --- /dev/null +++ b/tests/test_notification_repository.py @@ -0,0 +1,257 @@ +"""Tests for the pagination of the notification repository. + +The notification list endpoints used to select the *entire* notification table +(a live instance had ~437k rows), sort it, validate every row and ship the lot +to the browser. The repository now takes a ``limit``/``offset`` page, so these +tests pin the page down: it is bounded, it skips correctly, it stays ordered +newest first, and the unread page never leaks read notifications. + +Every assertion is made against a page smaller than the seeded row count, so a +regression to an unbounded query fails the suite rather than passing by luck. +""" + +import uuid +from datetime import datetime, timedelta + +import pytest +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from media_manager.notification.repository import NotificationRepository +from media_manager.notification.schemas import Notification as NotificationSchema +from media_manager.notification.schemas import NotificationId + +# Deliberately larger than any ``limit`` used below, so pagination is exercised. +SEEDED_NOTIFICATIONS = 120 + +BASE_TIME = datetime(2026, 1, 1, 12, 0, 0) + + +def build_notification(index: int, *, read: bool) -> NotificationSchema: + """Build notification ``index``, one minute newer than notification ``index - 1``.""" + return NotificationSchema( + id=NotificationId(uuid.uuid4()), + message=f"Notification {index}", + read=read, + timestamp=BASE_TIME + timedelta(minutes=index), + ) + + +@pytest.fixture +async def seeded_notifications( + session_factory: async_sessionmaker[AsyncSession], +) -> list[NotificationSchema]: + """Seed alternating read/unread notifications, returned newest first. + + Inserted in oldest-first order so that a query which forgets to order by + timestamp cannot accidentally return the expected order. + """ + notifications = [ + build_notification(index, read=index % 2 == 0) + for index in range(SEEDED_NOTIFICATIONS) + ] + async with session_factory() as session: + repository = NotificationRepository(session) + for notification in notifications: + await repository.save_notification(notification) + return sorted(notifications, key=lambda n: n.timestamp, reverse=True) + + +def ids(notifications: list[NotificationSchema]) -> list[uuid.UUID]: + return [notification.id for notification in notifications] + + +class TestGetAllNotifications: + async def test_limit_caps_the_number_returned( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_all_notifications(limit=50) + + assert len(page) == 50 + assert ids(page) == ids(seeded_notifications[:50]) + + async def test_offset_skips_the_preceding_notifications( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_all_notifications( + limit=50, offset=50 + ) + + assert ids(page) == ids(seeded_notifications[50:100]) + + async def test_consecutive_pages_cover_every_notification_exactly_once( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + collected: list[NotificationSchema] = [] + async with session_factory() as session: + repository = NotificationRepository(session) + for offset in range(0, SEEDED_NOTIFICATIONS, 50): + collected += await repository.get_all_notifications( + limit=50, offset=offset + ) + + assert ids(collected) == ids(seeded_notifications) + + async def test_offset_past_the_end_returns_an_empty_page( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_all_notifications( + limit=50, offset=SEEDED_NOTIFICATIONS + ) + + assert page == [] + + async def test_page_is_ordered_by_timestamp_descending( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_all_notifications(limit=50) + + timestamps = [notification.timestamp for notification in page] + assert timestamps == sorted(timestamps, reverse=True) + # The newest seeded notification has to be the first row of the first page. + assert page[0].timestamp == seeded_notifications[0].timestamp + + async def test_read_filter_returns_only_read_notifications( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + expected = [n for n in seeded_notifications if n.read] + async with session_factory() as session: + page = await NotificationRepository(session).get_all_notifications( + limit=20, offset=10, read=True + ) + + assert all(notification.read for notification in page) + assert ids(page) == ids(expected[10:30]) + + +class TestGetUnreadNotifications: + async def test_returns_only_unread_notifications( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_unread_notifications( + limit=50 + ) + + assert page != [] + assert not any(notification.read for notification in page) + + async def test_limit_caps_the_number_returned( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + unread = [n for n in seeded_notifications if not n.read] + assert len(unread) > 20, "the seed must exceed the limit under test" + + async with session_factory() as session: + page = await NotificationRepository(session).get_unread_notifications( + limit=20 + ) + + assert len(page) == 20 + assert ids(page) == ids(unread[:20]) + + async def test_offset_skips_the_preceding_unread_notifications( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + unread = [n for n in seeded_notifications if not n.read] + async with session_factory() as session: + page = await NotificationRepository(session).get_unread_notifications( + limit=20, offset=20 + ) + + assert ids(page) == ids(unread[20:40]) + + async def test_page_is_ordered_by_timestamp_descending( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + page = await NotificationRepository(session).get_unread_notifications( + limit=50 + ) + + timestamps = [notification.timestamp for notification in page] + assert timestamps == sorted(timestamps, reverse=True) + + +class TestMarkAllNotificationsAsRead: + async def test_marks_every_unread_notification_read( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + """A single statement has to reach past the page the client holds.""" + async with session_factory() as session: + repository = NotificationRepository(session) + await repository.mark_all_notifications_as_read() + await session.commit() + + async with session_factory() as session: + repository = NotificationRepository(session) + unread = await repository.count_notifications(read=False) + read = await repository.count_notifications(read=True) + + assert unread == 0 + assert read == SEEDED_NOTIFICATIONS + + +class TestCountNotifications: + async def test_counts_every_notification_by_default( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + total = await NotificationRepository(session).count_notifications() + + assert total == SEEDED_NOTIFICATIONS + + async def test_counts_unread_and_read_separately( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + async with session_factory() as session: + repository = NotificationRepository(session) + unread = await repository.count_notifications(read=False) + read = await repository.count_notifications(read=True) + + assert unread == len([n for n in seeded_notifications if not n.read]) + assert read == len([n for n in seeded_notifications if n.read]) + assert unread + read == SEEDED_NOTIFICATIONS + + async def test_count_is_independent_of_the_page_size( + self, + session_factory: async_sessionmaker[AsyncSession], + seeded_notifications: list[NotificationSchema], + ) -> None: + """The UI paginates on this count, so it must describe the whole table.""" + async with session_factory() as session: + repository = NotificationRepository(session) + page = await repository.get_all_notifications(limit=10) + total = await repository.count_notifications() + + assert len(page) == 10 + assert total == SEEDED_NOTIFICATIONS diff --git a/uv.lock b/uv.lock index 8a6c2832..046dfbe4 100644 --- a/uv.lock +++ b/uv.lock @@ -95,6 +95,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/fb/76/641ae371508676492379f16e2fa48f4e2c11741bd63c48be4b12a6b09cba/aiosignal-1.4.0-py3-none-any.whl", hash = "sha256:053243f8b92b990551949e63930a839ff0cf0b0ebbe0597b0f3fb19e1a0fe82e", size = 7490, upload-time = "2025-07-03T22:54:42.156Z" }, ] +[[package]] +name = "aiosqlite" +version = "0.22.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/4e/8a/64761f4005f17809769d23e518d915db74e6310474e733e3593cfc854ef1/aiosqlite-0.22.1.tar.gz", hash = "sha256:043e0bd78d32888c0a9ca90fc788b38796843360c855a7262a532813133a0650", size = 14821, upload-time = "2025-12-23T19:25:43.997Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/00/b7/e3bf5133d697a08128598c8d0abc5e16377b51465a33756de24fa7dee953/aiosqlite-0.22.1-py3-none-any.whl", hash = "sha256:21c002eb13823fad740196c5a2e9d8e62f6243bd9e7e4a1f87fb5e44ecb4fceb", size = 17405, upload-time = "2025-12-23T19:25:42.139Z" }, +] + [[package]] name = "alembic" version = "1.18.4" @@ -1068,6 +1077,11 @@ dev = [ { name = "ruff" }, { name = "ty" }, ] +test = [ + { name = "aiosqlite" }, + { name = "pytest" }, + { name = "pytest-asyncio" }, +] [package.metadata] requires-dist = [ @@ -1111,6 +1125,11 @@ dev = [ { name = "ruff" }, { name = "ty", specifier = ">=0.0.33" }, ] +test = [ + { name = "aiosqlite", specifier = ">=0.21.0" }, + { name = "pytest", specifier = ">=8.4.0" }, + { name = "pytest-asyncio", specifier = ">=1.0.0" }, +] [[package]] name = "multidict" @@ -1619,6 +1638,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/3b/ab/b3226f0bd7cdcf710fbede2b3548584366da3b19b5021e74f5bde2a8fa3f/pytest-9.0.2-py3-none-any.whl", hash = "sha256:711ffd45bf766d5264d487b917733b453d917afd2b0ad65223959f59089f875b", size = 374801, upload-time = "2025-12-06T21:30:49.154Z" }, ] +[[package]] +name = "pytest-asyncio" +version = "1.4.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "pytest" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/43/7c/d36d04db312ecf4298932ef77e6e4a9e8ad017906e24e34f0b0c361a2473/pytest_asyncio-1.4.0.tar.gz", hash = "sha256:c6c0d2259945122819f171a32ecea2c349ead889ee28176caaf492143424be42", size = 58514, upload-time = "2026-05-26T09:56:04.083Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/03/e2/08a497ef684b88559c9cc5f4ad53a37e7b99e727094a86d6ea32536d5d3c/pytest_asyncio-1.4.0-py3-none-any.whl", hash = "sha256:933ca923a23075a87fb7070c0ec272a6848489824d887c85c812670932835aa1", size = 16930, upload-time = "2026-05-26T09:56:02.576Z" }, +] + [[package]] name = "python-dotenv" version = "1.2.2" diff --git a/web/src/lib/api/api.d.ts b/web/src/lib/api/api.d.ts index 08e74f2c..9c8605a0 100644 --- a/web/src/lib/api/api.d.ts +++ b/web/src/lib/api/api.d.ts @@ -942,7 +942,10 @@ export interface paths { }; /** * Get All Notifications - * @description Get all notifications. + * @description Get a page of notifications, newest first. + * + * The total number of matching notifications is returned in the + * `X-Total-Count` response header. */ get: operations['get_all_notifications_api_v1_notification_get']; put?: never; @@ -962,7 +965,10 @@ export interface paths { }; /** * Get Unread Notifications - * @description Get all unread notifications. + * @description Get a page of unread notifications, newest first. + * + * The total number of unread notifications is returned in the `X-Total-Count` + * response header. */ get: operations['get_unread_notifications_api_v1_notification_unread_get']; put?: never; @@ -997,6 +1003,29 @@ export interface paths { patch?: never; trace?: never; }; + '/api/v1/notification/read': { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + /** + * Mark All Notifications As Read + * @description Mark every unread notification as read. + * + * The client only holds a page of notifications, so it cannot mark them all + * read by iterating over the ones it has. + */ + patch: operations['mark_all_notifications_as_read_api_v1_notification_read_patch']; + trace?: never; + }; '/api/v1/notification/{notification_id}/read': { parameters: { query?: never; @@ -3694,7 +3723,14 @@ export interface operations { }; get_all_notifications_api_v1_notification_get: { parameters: { - query?: never; + query?: { + /** @description Maximum number of notifications to return */ + limit?: number; + /** @description Number of notifications to skip */ + offset?: number; + /** @description Only return read (true) or unread (false) notifications */ + read?: boolean | null; + }; header?: never; path?: never; cookie?: never; @@ -3710,11 +3746,25 @@ export interface operations { 'application/json': components['schemas']['Notification'][]; }; }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + 'application/json': components['schemas']['HTTPValidationError']; + }; + }; }; }; get_unread_notifications_api_v1_notification_unread_get: { parameters: { - query?: never; + query?: { + /** @description Maximum number of notifications to return */ + limit?: number; + /** @description Number of notifications to skip */ + offset?: number; + }; header?: never; path?: never; cookie?: never; @@ -3730,6 +3780,15 @@ export interface operations { 'application/json': components['schemas']['Notification'][]; }; }; + /** @description Validation Error */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + 'application/json': components['schemas']['HTTPValidationError']; + }; + }; }; }; get_notification_api_v1_notification__notification_id__get: { @@ -3806,6 +3865,24 @@ export interface operations { }; }; }; + mark_all_notifications_as_read_api_v1_notification_read_patch: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 204: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; mark_notification_as_read_api_v1_notification__notification_id__read_patch: { parameters: { query?: never; diff --git a/web/src/routes/dashboard/notifications/+page.svelte b/web/src/routes/dashboard/notifications/+page.svelte index 1f92d6d9..83b12b67 100644 --- a/web/src/routes/dashboard/notifications/+page.svelte +++ b/web/src/routes/dashboard/notifications/+page.svelte @@ -9,33 +9,117 @@ import type { Notification } from '$lib/api/api'; import { resolve } from '$app/paths'; + // The notification table grows without bound (a live instance had ~437k rows), + // so the page only ever holds the pages it has explicitly loaded. + const PAGE_SIZE = 50; + let unreadNotifications: Notification[] = []; let readNotifications: Notification[] = []; + // Totals come from the X-Total-Count header, so "load more" knows when to stop. + let unreadTotal = 0; + let readTotal = 0; let loading = true; + let loadingMoreUnread = false; + let loadingMoreRead = false; let showRead = false; let markingAllAsRead = false; - async function fetchNotifications() { - loading = true; - const unread = await client.GET('/api/v1/notification/unread'); - const all = await client.GET('/api/v1/notification'); - unreadNotifications = unread.data!; - readNotifications = all.data!.filter((n) => n.read); - loading = false; + $: hasMoreUnread = unreadNotifications.length < unreadTotal; + $: hasMoreRead = readNotifications.length < readTotal; + + function totalCountOf(response: Response, fallback: number): number { + const total = Number(response.headers.get('X-Total-Count')); + return Number.isNaN(total) ? fallback : total; + } + + function timeOf(notification: Notification): number { + return new Date(notification.timestamp ?? 0).getTime(); + } + + /** Insert into a newest-first list, keeping it ordered. */ + function insertNewestFirst( + notifications: Notification[], + notification: Notification, + hasMore: boolean + ): Notification[] { + const index = notifications.findIndex((n) => timeOf(n) < timeOf(notification)); + if (index === -1) { + // Older than everything loaded: if further pages exist it belongs to one + // of them, so leave it to be fetched in order rather than tacking it on. + return hasMore ? notifications : [...notifications, notification]; + } + return [...notifications.slice(0, index), notification, ...notifications.slice(index)]; + } + + async function loadUnread({ reset = false } = {}) { + const offset = reset ? 0 : unreadNotifications.length; + const { data, response } = await client.GET('/api/v1/notification/unread', { + params: { query: { limit: PAGE_SIZE, offset } } + }); + if (!data) return; + + unreadNotifications = reset ? data : [...unreadNotifications, ...data]; + unreadTotal = totalCountOf(response, unreadNotifications.length); + } + + async function loadRead({ reset = false } = {}) { + const offset = reset ? 0 : readNotifications.length; + const { data, response } = await client.GET('/api/v1/notification', { + params: { query: { limit: PAGE_SIZE, offset, read: true } } + }); + if (!data) return; + + readNotifications = reset ? data : [...readNotifications, ...data]; + readTotal = totalCountOf(response, readNotifications.length); + } + + async function loadMoreUnread() { + loadingMoreUnread = true; + try { + await loadUnread(); + } finally { + loadingMoreUnread = false; + } + } + + async function loadMoreRead() { + loadingMoreRead = true; + try { + await loadRead(); + } finally { + loadingMoreRead = false; + } + } + + async function toggleShowRead() { + showRead = !showRead; + // Read notifications are the bulk of the table, so they are not fetched at + // all until the section is opened. + if (showRead && readNotifications.length === 0) { + loadingMoreRead = true; + try { + await loadRead({ reset: true }); + } finally { + loadingMoreRead = false; + } + } } async function markAsRead(notificationId: string) { const { response } = await client.PATCH('/api/v1/notification/{notification_id}/read', { params: { path: { notification_id: notificationId } } }); + if (!response.ok) return; - if (response.ok) { - const notification = unreadNotifications.find((n) => n.id === notificationId); - if (notification) { - notification.read = true; - readNotifications = [notification, ...readNotifications]; - unreadNotifications = unreadNotifications.filter((n) => n.id !== notificationId); - } + const notification = unreadNotifications.find((n) => n.id === notificationId); + if (!notification) return; + + notification.read = true; + unreadNotifications = unreadNotifications.filter((n) => n.id !== notificationId); + unreadTotal = Math.max(0, unreadTotal - 1); + readTotal += 1; + if (showRead) { + readNotifications = insertNewestFirst(readNotifications, notification, hasMoreRead); } } @@ -43,36 +127,33 @@ const { response } = await client.PATCH('/api/v1/notification/{notification_id}/unread', { params: { path: { notification_id: notificationId } } }); + if (!response.ok) return; - if (response.ok) { - const notification = readNotifications.find((n) => n.id === notificationId); - if (notification) { - notification.read = false; - unreadNotifications = [notification, ...unreadNotifications]; - readNotifications = readNotifications.filter((n) => n.id !== notificationId); - } - } + const notification = readNotifications.find((n) => n.id === notificationId); + if (!notification) return; + + notification.read = false; + readNotifications = readNotifications.filter((n) => n.id !== notificationId); + readTotal = Math.max(0, readTotal - 1); + unreadTotal += 1; + unreadNotifications = insertNewestFirst(unreadNotifications, notification, hasMoreUnread); } async function markAllAsRead() { - if (unreadNotifications.length === 0) return; + if (unreadTotal === 0) return; try { markingAllAsRead = true; - const promises = unreadNotifications.map((notification) => - client.PATCH('/api/v1/notification/{notification_id}/read', { - params: { path: { notification_id: notification.id! } } - }) - ); - - await Promise.all(promises); - - // Move all unread to read - readNotifications = [ - ...unreadNotifications.map((n) => ({ ...n, read: true })), - ...readNotifications - ]; + // One request: the client no longer holds every notification, so it cannot + // mark them all read by looping over the ones it happens to have. + const { response } = await client.PATCH('/api/v1/notification/read'); + if (!response.ok) return; + unreadNotifications = []; + unreadTotal = 0; + readNotifications = []; + readTotal = 0; + if (showRead) await loadRead({ reset: true }); } catch (error) { console.error('Failed to mark all notifications as read:', error); } finally { @@ -81,9 +162,16 @@ } onMount(() => { - fetchNotifications(); + loading = true; + loadUnread({ reset: true }).finally(() => (loading = false)); - const interval = setInterval(fetchNotifications, 30000); + const interval = setInterval(() => { + if (loading || markingAllAsRead) return; + // Only refresh while a single page is shown, so polling cannot discard + // pages the user has loaded. + if (unreadNotifications.length > PAGE_SIZE) return; + loadUnread({ reset: true }); + }, 30000); return () => clearInterval(interval); }); @@ -117,7 +205,7 @@

Notifications

- {#if unreadNotifications.length > 0} + {#if unreadTotal > 0}
{/each} + + {#if hasMoreUnread} +
+ +

+ Showing {unreadNotifications.length} of {unreadTotal} +

+
+ {/if} {/if}
@@ -214,11 +321,17 @@ {#if showRead}
{#if readNotifications.length === 0} -
-

No read notifications

-
+ {#if loadingMoreRead} +
+
+
+ {:else} +
+

No read notifications

+
+ {/if} {:else}
{#each readNotifications as notification (notification.id)} @@ -257,6 +370,25 @@
{/each}
+ + {#if hasMoreRead} +
+ +

+ Showing {readNotifications.length} of {readTotal} +

+
+ {/if} {/if} {/if} From 5b28452cc92bf7ac1c2bedf4cdcd99744a2e58a3 Mon Sep 17 00:00:00 2001 From: Jaspreet Singh <6873201+tagpro@users.noreply.github.com> Date: Mon, 13 Jul 2026 14:32:34 +1000 Subject: [PATCH 2/2] fix(notifications): handle a missing X-Total-Count and serialise unread fetches Number(null) is 0, so an absent X-Total-Count header collapsed unreadTotal and readTotal to zero rather than falling back to the loaded count, hiding the unread count and the "Mark All as Read" button while rows were on screen. The 30s poll and a "Load More" click both wrote unreadNotifications and could overlap, letting whichever response landed last win: a poll issued before a "Load More" but resolving after it truncates the list back to the first page, dropping the page the user just asked for. Queue the unread fetches so each reads its offset only once the previous one has settled, and skip the poll while a "Load More" is pending so it cannot discard a loaded page. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SC2BDd7wA7hPa7k2h2yAv1 --- .../dashboard/notifications/+page.svelte | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/web/src/routes/dashboard/notifications/+page.svelte b/web/src/routes/dashboard/notifications/+page.svelte index 83b12b67..0b6f3e09 100644 --- a/web/src/routes/dashboard/notifications/+page.svelte +++ b/web/src/routes/dashboard/notifications/+page.svelte @@ -28,8 +28,10 @@ $: hasMoreRead = readNotifications.length < readTotal; function totalCountOf(response: Response, fallback: number): number { - const total = Number(response.headers.get('X-Total-Count')); - return Number.isNaN(total) ? fallback : total; + const header = response.headers.get('X-Total-Count'); + if (!header) return fallback; + const total = Number(header); + return Number.isFinite(total) ? total : fallback; } function timeOf(notification: Notification): number { @@ -51,15 +53,26 @@ return [...notifications.slice(0, index), notification, ...notifications.slice(index)]; } - async function loadUnread({ reset = false } = {}) { - const offset = reset ? 0 : unreadNotifications.length; - const { data, response } = await client.GET('/api/v1/notification/unread', { - params: { query: { limit: PAGE_SIZE, offset } } - }); - if (!data) return; - - unreadNotifications = reset ? data : [...unreadNotifications, ...data]; - unreadTotal = totalCountOf(response, unreadNotifications.length); + // The 30s poll and a "Load More" click both write unreadNotifications, so they + // are queued rather than run concurrently: overlapping, whichever landed last + // would win, dropping a page or repeating an id in the keyed {#each}. Queuing + // also means each request reads its offset only once the one before it is done. + let unreadQueue: Promise = Promise.resolve(); + + function loadUnread({ reset = false } = {}): Promise { + const fetchPage = async () => { + const offset = reset ? 0 : unreadNotifications.length; + const { data, response } = await client.GET('/api/v1/notification/unread', { + params: { query: { limit: PAGE_SIZE, offset } } + }); + if (!data) return; + + unreadNotifications = reset ? data : [...unreadNotifications, ...data]; + unreadTotal = totalCountOf(response, unreadNotifications.length); + }; + + unreadQueue = unreadQueue.then(fetchPage, fetchPage); + return unreadQueue; } async function loadRead({ reset = false } = {}) { @@ -166,7 +179,7 @@ loadUnread({ reset: true }).finally(() => (loading = false)); const interval = setInterval(() => { - if (loading || markingAllAsRead) return; + if (loading || markingAllAsRead || loadingMoreUnread) return; // Only refresh while a single page is shown, so polling cannot discard // pages the user has loaded. if (unreadNotifications.length > PAGE_SIZE) return;