fix(notifications): paginate and index the notification list endpoints - #3
Closed
tagpro wants to merge 4 commits into
Closed
fix(notifications): paginate and index the notification list endpoints#3tagpro wants to merge 4 commits into
tagpro wants to merge 4 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
…ad 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SC2BDd7wA7hPa7k2h2yAv1
📝 WalkthroughWalkthroughNotifications now support indexed pagination, read filtering, total-count headers, bulk mark-as-read operations, generated API typings, repository tests, and incremental dashboard loading for unread and read sections. ChangesNotification pagination and read management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant NotificationAPI
participant NotificationService
participant NotificationRepository
participant Database
Dashboard->>NotificationAPI: GET notification page
NotificationAPI->>NotificationService: Count and fetch page
NotificationService->>NotificationRepository: Apply filters and pagination
NotificationRepository->>Database: Execute indexed query
Database-->>NotificationRepository: Rows and count
NotificationRepository-->>NotificationService: Results
NotificationService-->>NotificationAPI: Results and total
NotificationAPI-->>Dashboard: Notifications with X-Total-Count
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
media_manager/notification/repository.py (1)
86-95: 🗄️ Data Integrity & Integration | 🔵 TrivialCount and page queries are issued separately — consider the race window.
The router calls
count_notificationsandget_all_notificationsas independent queries. If a notification is inserted or deleted between the two calls, theX-Total-Countheader can disagree with the actual page contents. For a notification list this is typically acceptable (the user simply sees a slightly stale count), but if strict consistency is needed, consider counting within the same transaction snapshot or using a single query with window functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@media_manager/notification/repository.py` around lines 86 - 95, Review the notification-list flow around count_notifications and get_all_notifications and avoid issuing them against separate transaction snapshots when strict consistency is required. Use one shared transaction snapshot for both queries, or combine the count and page retrieval into a single window-function query, while preserving the existing read filter and pagination behavior.web/src/routes/dashboard/notifications/+page.svelte (1)
316-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent event-binding syntax.
This button uses the legacy
on:clickdirective while other buttons in this same file (e.g., Line 222, Line 296, Line 390) use the Svelte 5onclick={...}attribute form. Both work, but mixing styles in one component is inconsistent.- <button - on:click={() => toggleShowRead()} + <button + onclick={() => toggleShowRead()}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/routes/dashboard/notifications/`+page.svelte around lines 316 - 317, Update the button invoking toggleShowRead to use Svelte 5’s onclick attribute syntax, matching the other buttons in this component, and remove the legacy on:click directive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/routes/dashboard/notifications/`+page.svelte:
- Around line 177-189: Handle the promise returned by the polling call to
loadUnread({ reset: true }) inside the setInterval callback by attaching a
rejection handler, preventing fetch failures from becoming unhandled rejections
while preserving the existing polling guards and loading behavior.
- Around line 78-87: Update loadRead to mirror the serialized request flow used
by loadUnread: add a read-notification queue and move offset calculation into
the queued fetch operation so it uses the latest readNotifications state. Route
reset and append loads through this queue, preserving reset semantics and
ensuring overlapping calls from loadMoreRead, toggleShowRead, and markAllAsRead
cannot apply stale results.
---
Nitpick comments:
In `@media_manager/notification/repository.py`:
- Around line 86-95: Review the notification-list flow around
count_notifications and get_all_notifications and avoid issuing them against
separate transaction snapshots when strict consistency is required. Use one
shared transaction snapshot for both queries, or combine the count and page
retrieval into a single window-function query, while preserving the existing
read filter and pagination behavior.
In `@web/src/routes/dashboard/notifications/`+page.svelte:
- Around line 316-317: Update the button invoking toggleShowRead to use Svelte
5’s onclick attribute syntax, matching the other buttons in this component, and
remove the legacy on:click directive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6910fc63-f47a-45ac-9780-fa72e0905358
📒 Files selected for processing (10)
alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.pymedia_manager/main.pymedia_manager/notification/models.pymedia_manager/notification/repository.pymedia_manager/notification/router.pymedia_manager/notification/service.pytests/conftest.pytests/test_notification_repository.pyweb/src/lib/api/api.d.tsweb/src/routes/dashboard/notifications/+page.svelte
Comment on lines
+78
to
+87
| 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); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
loadRead lacks the same write-serialization loadUnread has, reintroducing the race the PR set out to fix.
Unlike loadUnread, which computes offset lazily inside fetchPage and serializes calls through unreadQueue, loadRead computes offset = reset ? 0 : readNotifications.length synchronously at call time and has no queue. This is invoked from three places that can genuinely overlap:
loadMoreRead(Line 98) — a "Load More" click, guarded only by disabling its own button.toggleShowRead(Line 107) — opens the section and does an initialreset: trueload.markAllAsRead(Line 155) — resetsreadNotifications/readTotalto empty, then (ifshowRead) callsloadRead({ reset: true }).
If a user opens the "Read Notifications" section, clicks "Load More", and then clicks "Mark All as Read" before the load-more request resolves, both loadRead calls run concurrently. Whichever resolves last wins: the stale, pre-reset "load more" response (computed with the old, larger offset) can overwrite the freshly reset list, or duplicate/drop items in the keyed {#each readNotifications as notification (notification.id)} block — exactly the class of bug the PR's unreadQueue was built to prevent, just not applied to loadRead.
🔒️ Proposed fix: mirror `unreadQueue` for read notifications
+ let readQueue: Promise<void> = Promise.resolve();
+
- 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);
- }
+ function loadRead({ reset = false } = {}): Promise<void> {
+ const fetchPage = async () => {
+ 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);
+ };
+
+ readQueue = readQueue.then(fetchPage, fetchPage);
+ return readQueue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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); | |
| } | |
| let readQueue: Promise<void> = Promise.resolve(); | |
| function loadRead({ reset = false } = {}): Promise<void> { | |
| const fetchPage = async () => { | |
| 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); | |
| }; | |
| readQueue = readQueue.then(fetchPage, fetchPage); | |
| return readQueue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/routes/dashboard/notifications/`+page.svelte around lines 78 - 87,
Update loadRead to mirror the serialized request flow used by loadUnread: add a
read-notification queue and move offset calculation into the queued fetch
operation so it uses the latest readNotifications state. Route reset and append
loads through this queue, preserving reset semantics and ensuring overlapping
calls from loadMoreRead, toggleShowRead, and markAllAsRead cannot apply stale
results.
Comment on lines
177
to
189
| onMount(() => { | ||
| fetchNotifications(); | ||
| loading = true; | ||
| loadUnread({ reset: true }).finally(() => (loading = false)); | ||
|
|
||
| const interval = setInterval(fetchNotifications, 30000); | ||
| const interval = setInterval(() => { | ||
| 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; | ||
| loadUnread({ reset: true }); | ||
| }, 30000); | ||
| return () => clearInterval(interval); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unhandled rejection risk from unguarded polling call.
loadUnread({ reset: true }) inside setInterval (Line 186) is fire-and-forget with no .catch. Since unreadQueue = unreadQueue.then(fetchPage, fetchPage) will re-throw if fetchPage itself fails, a persistent network error will produce an unhandled promise rejection every 30 seconds indefinitely.
🔧 Proposed fix
- loadUnread({ reset: true });
+ loadUnread({ reset: true }).catch((error) => console.error('Failed to refresh unread notifications:', error));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onMount(() => { | |
| fetchNotifications(); | |
| loading = true; | |
| loadUnread({ reset: true }).finally(() => (loading = false)); | |
| const interval = setInterval(fetchNotifications, 30000); | |
| const interval = setInterval(() => { | |
| 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; | |
| loadUnread({ reset: true }); | |
| }, 30000); | |
| return () => clearInterval(interval); | |
| }); | |
| onMount(() => { | |
| loading = true; | |
| loadUnread({ reset: true }).finally(() => (loading = false)); | |
| const interval = setInterval(() => { | |
| 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; | |
| loadUnread({ reset: true }).catch((error) => console.error('Failed to refresh unread notifications:', error)); | |
| }, 30000); | |
| return () => clearInterval(interval); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/routes/dashboard/notifications/`+page.svelte around lines 177 - 189,
Handle the promise returned by the polling call to loadUnread({ reset: true })
inside the setInterval callback by attaching a rejection handler, preventing
fetch failures from becoming unhandled rejections while preserving the existing
polling guards and loading behavior.
Owner
Author
|
Superseded — this branch re-applied the pagination commits that are already on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fork-side copy of upstream maxdorninger/MediaManager#564, including the review fixes.
Problem
Both notification list endpoints returned the entire table — no
LIMIT/OFFSET. Every visit to the notifications tab, and every 30s poll, fetched every row, pydantic-validated each one and shipped the lot to the browser. On the instance that surfaced this the table had ~437,000 rows. There was also no index beyond the primary key, soORDER BY timestamp DESCsorted the whole table each load and the unread query (WHERE read = false) was a full scan.Changes
Backend
GET /notificationandGET /notification/unreadtakelimit(default 50,le=200) andoffset(ge=0), threaded through the service into.limit()/.offset(). Ordering unchanged (timestamp descending).X-Total-Countheader (CORS-exposed) so the UI knows how many pages exist.GET /notificationtakes an optionalreadfilter — the UI used to build its "read" list by fetching everything and filtering client-side, which a paginated endpoint can't support.b3d51c7fa9e2adds an index ontimestamp DESCand a partial index (WHERE read = false) for the unread query.PATCH /notification/readmarks every unread notification read in oneUPDATE. "Mark All as Read" previously looped a PATCH over the notifications the client held, which only worked because it held all of them.Frontend
Review fixes (on top of the upstream branch)
X-Total-Countno longer collapses the totals to zero:Number(null)is0, notNaN, so theisNaNfallback never ran, which could hide the unread count and the "Mark All as Read" button while rows were still on screen.Tests
limitcaps the result,offsetskips correctly, ordering is timestamp descending, the unread query returns only unread rows and respects limit/offset, and bulk mark-as-read reaches past the loaded page. Seeded with more rows than the page size so pagination is actually exercised. Adds a small pytest harness (async SQLite viaaiosqlite, plus atestdependency group), since there wasn't one.Not in this PR
The reason that instance had 437k notifications is a separate root cause — identical failure notifications are written every import cycle. Worth fixing on its own; this PR is limited to making the endpoints bounded and indexed.
Verification
uv run pytest— 14 passeduv run ruff check ./media_manager— cleannpm run lint(inweb/) — cleanX-Total-Count,limit=500is rejected with 422, offset pages don't overlap, the unread page never contains read rows, andPATCH /notification/readempties the unread list.alembic upgrade e60ae827ed98:b3d51c7fa9e2 --sqlrenders the expected DDL, including the partial index.🤖 Generated with Claude Code
https://claude.ai/code/session_01SC2BDd7wA7hPa7k2h2yAv1
Summary by CodeRabbit
New Features
Bug Fixes