fix(notifications): paginate and index the notification list endpoints#1
fix(notifications): paginate and index the notification list endpoints#1tagpro wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wrZPxJDip5x3mzjWnpwos
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughNotification retrieval now supports paginated read/unread queries, total-count headers, indexed ordering, bulk read updates, repository tests, generated API types, and dashboard load-more behavior. ChangesNotification pagination and read state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard as Notifications dashboard
participant Router as Notification router
participant Service as NotificationService
participant Repository as NotificationRepository
participant Database as Notification database
Dashboard->>Router: Request paginated notifications
Router->>Service: Count matching notifications
Service->>Repository: Count by read filter
Repository->>Database: Execute count query
Router->>Service: Fetch page with limit and offset
Service->>Repository: Query ordered notification page
Repository->>Database: Execute filtered paginated query
Router-->>Dashboard: Notifications plus X-Total-Count
sequenceDiagram
participant Dashboard as Notifications dashboard
participant Router as Notification router
participant Service as NotificationService
participant Repository as NotificationRepository
participant Database as Notification database
Dashboard->>Router: PATCH mark all as read
Router->>Service: Mark all notifications as read
Service->>Repository: Update unread notifications
Repository->>Database: Execute bulk read update
Router-->>Dashboard: 204 No Content
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superseded: opening a fork-specific branch that merges cleanly with the fork's master (the branch here is based on upstream/master and conflicts with the fork's existing tests/conftest.py). |
Fork-side copy of maxdorninger#564, so the change can be tested on our own instance.
The branch is based on upstream/master, not this fork's master, so it does not carry the fork-specific commits (CI/Quay, MissingGreenlet fix). Merging it here brings the pagination work onto the fork's master alongside those.
Problem
Both notification list endpoints return the entire notification table:
Every visit to the tab (and every 30s poll) fetches every row, pydantic-validates each one, serialises the whole list to JSON and ships it to the browser. On our instance the
notificationtable had ~437,000 rows.There is also no index beyond the primary key on
id, so theORDER BY timestamp DESCsorts the whole table on every load and the unread query is a full scan.Changes
GET /notificationandGET /notification/unreadtakelimit(default 50, max 200) andoffset; total count comes back in anX-Total-Countheader (exposed via CORS).GET /notificationtakes an optionalreadfilter — the UI used to build its "read" list by fetching everything and filtering client-side.b3d51c7fa9e2(offe60ae827ed98): index ontimestamp DESC, partial indexWHERE read = false.PATCH /notification/readbulk-marks all unread as read in oneUPDATE; the old "Mark All as Read" looped a PATCH per notification and would otherwise have marked only the loaded page.tests/, async SQLite) plus repository tests for limit/offset/ordering/unread/bulk-read.Testing locally
Note the migration creates two indexes on a ~437k-row table — quick, but it does take a lock briefly.
The notification flood (identical failure notifications written every import cycle) is a separate root cause and is not addressed here; this only makes the endpoint bounded and indexed.
🤖 Generated with Claude Code
https://claude.ai/code/session_019wrZPxJDip5x3mzjWnpwos
Summary by CodeRabbit
New Features
Bug Fixes
Tests