fix(notifications): paginate and index the notification list endpoints (fork)#2
fix(notifications): paginate and index the notification list endpoints (fork)#2tagpro wants to merge 2 commits 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
Brings the notification pagination work (based on upstream/master, PR maxdorninger#564) onto the fork's master, which carries the fork-specific CI and MissingGreenlet commits. The only conflict was tests/conftest.py, which both sides add: the fork's version (movie/tv fixtures for the eager-loading tests) is kept, with the notification models registered on Base.metadata alongside them so the notification repository tests can create their table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wrZPxJDip5x3mzjWnpwos
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Fork-side copy of the upstream PR maxdorninger/MediaManager#564, so the change can be run against our own instance.
The upstream branch (
fix/notifications-pagination) is based on upstream/master and conflicts with this fork's master, which already carriestests/conftest.pyfrom the MissingGreenlet commit. This branch is that work merged into the fork's master, with the conflict resolved: the fork's conftest (movie/tv fixtures) is kept, and the notification models are registered onBase.metadataalongside them.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 (WHERE read = false) is a full scan.Changes
GET /notificationandGET /notification/unreadtakelimit(default 50, max 200) andoffset; the 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, which cannot survive pagination.b3d51c7fa9e2(offe60ae827ed98): index ontimestamp DESCfor the ORDER BY, partial indexWHERE read = falsefor the unread query.PATCH /notification/readbulk-marks all unread as read in oneUPDATE. "Mark All as Read" used to loop a PATCH per notification and would otherwise have silently marked only the loaded page.Testing it locally
The migration builds two indexes on the ~437k-row table — fast, but it does take a brief lock.
Worth eyeballing on the running instance: the tab should render immediately with 50 unread; "Load more" appends the next 50 and the "Showing N of M" count matches; expanding "Read Notifications" issues its first request only then; "Mark All as Read" clears everything in one request rather than 437k.
Not fixed here
The reason the table reached 437k rows is a separate root cause — identical failure notifications are written every import cycle. This PR only makes the endpoint bounded and indexed, which is what makes the tab slow regardless of how the rows got there.
🤖 Generated with Claude Code
https://claude.ai/code/session_019wrZPxJDip5x3mzjWnpwos