fix(notifications): paginate and index the notification list endpoints - #564
Open
tagpro wants to merge 2 commits into
Open
fix(notifications): paginate and index the notification list endpoints#564tagpro wants to merge 2 commits into
tagpro 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNotifications now support paginated, filtered retrieval with total-count headers, indexed queries, bulk read updates, repository tests, and a dashboard UI with separate unread/read pagination. ChangesNotification pagination and bulk read state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant NotificationRouter
participant NotificationService
participant NotificationRepository
Browser->>NotificationRouter: Request notification page
NotificationRouter->>NotificationService: Request count and page
NotificationService->>NotificationRepository: Query count and paginated results
NotificationRepository-->>NotificationService: Count and notifications
NotificationService-->>NotificationRouter: Notification page
NotificationRouter-->>Browser: Page with X-Total-Count
Browser->>NotificationRouter: PATCH mark all as read
NotificationRouter->>NotificationService: Bulk mark unread notifications as read
NotificationService->>NotificationRepository: Execute bulk update
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
This was referenced Jul 11, 2026
tagpro
marked this pull request as ready for review
July 13, 2026 02:36
![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 (3)
web/src/routes/dashboard/notifications/+page.svelte (2)
279-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "Load More" markup for unread and read sections.
The two blocks are structurally identical (spinner, button, "Showing X of Y" text), differing only in which state variables they reference. Consider extracting a Svelte 5
{#snippet}(or a small reusable component) taking{ items, total, loading, onLoadMore }to avoid keeping two copies in sync.Also applies to: 373-391
🤖 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 279 - 297, Extract the duplicated Load More UI used by the unread and read notification sections into a Svelte 5 snippet or reusable component that accepts items, total, loading, and onLoadMore inputs. Replace both inline blocks with the shared implementation while preserving each section’s existing state references and behavior.
54-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling around the paginated fetches.
loadUnread/loadRead/loadMoreUnread/loadMoreReadhave no try/catch, unlikemarkAllAsRead(lines 142-162) which logs and resets its loading flag on failure. A rejectedclient.GET(network error) here becomes an unhandled promise rejection withloadingMoreUnread/loadingMoreReadcorrectly reset viafinally, but no feedback to the user that the page failed to load.🤖 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 54 - 92, Add error handling to the paginated notification loading flow: wrap the GET operations used by loadUnread and loadRead with catches that log the failure and provide the existing user-facing failure feedback, matching markAllAsRead. Preserve the loadingMoreUnread and loadingMoreRead finally-based resets while ensuring rejected requests from loadMoreUnread and loadMoreRead are handled rather than becoming unhandled promise rejections.alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py (1)
20-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-concurrent index creation will lock the notification table during deploy.
CREATE INDEX(andCREATE INDEX ... postgresql_where=...) withoutCONCURRENTLYtakes a lock that blocks writes for the duration of the build. Given the test suite explicitly seeds against a table that had ~437k rows in production, building two indexes on this table during migration could stall writes noticeably at deploy time.🔧 Suggested fix using Alembic's autocommit block + CONCURRENTLY
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'), - ) + with op.get_context().autocommit_block(): + op.create_index( + 'ix_notification_timestamp_desc', + 'notification', + [sa.text('timestamp DESC')], + unique=False, + postgresql_concurrently=True, + ) + op.create_index( + 'ix_notification_unread_timestamp_desc', + 'notification', + [sa.text('timestamp DESC')], + unique=False, + postgresql_where=sa.text('read = false'), + postgresql_concurrently=True, + ) 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') + with op.get_context().autocommit_block(): + op.drop_index( + 'ix_notification_unread_timestamp_desc', + table_name='notification', + postgresql_concurrently=True, + ) + op.drop_index( + 'ix_notification_timestamp_desc', + table_name='notification', + postgresql_concurrently=True, + )Note:
CREATE INDEX CONCURRENTLYcannot run inside a transaction, hence theautocommit_block()wrapper.🤖 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 `@alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py` around lines 20 - 38, Update upgrade() to create both notification indexes concurrently using Alembic’s autocommit_block() context, ensuring each op.create_index call sets PostgreSQL concurrent creation. Keep the existing index names, descending timestamp expressions, and partial read = false predicate unchanged.
🤖 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 164-176: Update the polling guard in the onMount interval to also
return while loadingMoreUnread is true, preventing the timer’s reset loadUnread
call from overlapping loadMoreUnread. Preserve the existing loading,
markingAllAsRead, and page-length checks; do not alter the broader request
coordination unless required by the existing flow.
- Around line 30-33: Update totalCountOf to check whether the X-Total-Count
header value is missing before converting it with Number. Return fallback for a
null or invalid header value, while preserving valid numeric totals.
---
Nitpick comments:
In `@alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py`:
- Around line 20-38: Update upgrade() to create both notification indexes
concurrently using Alembic’s autocommit_block() context, ensuring each
op.create_index call sets PostgreSQL concurrent creation. Keep the existing
index names, descending timestamp expressions, and partial read = false
predicate unchanged.
In `@web/src/routes/dashboard/notifications/`+page.svelte:
- Around line 279-297: Extract the duplicated Load More UI used by the unread
and read notification sections into a Svelte 5 snippet or reusable component
that accepts items, total, loading, and onLoadMore inputs. Replace both inline
blocks with the shared implementation while preserving each section’s existing
state references and behavior.
- Around line 54-92: Add error handling to the paginated notification loading
flow: wrap the GET operations used by loadUnread and loadRead with catches that
log the failure and provide the existing user-facing failure feedback, matching
markAllAsRead. Preserve the loadingMoreUnread and loadingMoreRead finally-based
resets while ensuring rejected requests from loadMoreUnread and loadMoreRead are
handled rather than becoming unhandled promise rejections.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d148e8c-4abe-41a1-a32e-f549393ea92f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
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.pypyproject.tomltests/__init__.pytests/conftest.pytests/test_notification_repository.pyweb/src/lib/api/api.d.tsweb/src/routes/dashboard/notifications/+page.svelte
📜 Review details
🔇 Additional comments (12)
media_manager/notification/models.py (1)
3-3: LGTM!Also applies to: 16-29
media_manager/notification/repository.py (1)
3-10: LGTM!Also applies to: 24-29, 43-60, 62-84, 86-96, 127-137
media_manager/notification/service.py (1)
17-32: LGTM!Also applies to: 43-45
media_manager/notification/router.py (1)
1-23: LGTM!Also applies to: 34-54, 61-77, 101-117
media_manager/main.py (1)
144-144: LGTM!alembic/versions/b3d51c7fa9e2_index_notification_timestamp_and_read.py (1)
1-17: LGTM!pyproject.toml (1)
47-57: LGTM!tests/conftest.py (1)
1-44: LGTM!tests/test_notification_repository.py (1)
1-258: LGTM!web/src/lib/api/api.d.ts (1)
936-1028: LGTM!Also applies to: 3724-3885
web/src/routes/dashboard/notifications/+page.svelte (2)
1-29: LGTM!Also applies to: 39-53, 94-163
205-278: LGTM!Also applies to: 301-372, 393-396
…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
tagpro
added a commit
to tagpro/MediaManager
that referenced
this pull request
Jul 13, 2026
…ad fetches (#4) 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. Addresses review feedback on maxdorninger#564. Claude-Session: https://claude.ai/code/session_01SC2BDd7wA7hPa7k2h2yAv1 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
The notifications tab is very slow to load. Both list endpoints return the entire notification table:
So every visit to the tab fetches every row, pydantic-validates each one, serialises the whole list to JSON and ships it to the browser. On the instance where I hit this, the
notificationtable had ~437,000 rows — the tab was loading all of them on every page load and on every 30s poll.The table also has 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
Backend
GET /notificationandGET /notification/unreadnow takelimit(default 50,le=200) andoffset(ge=0), threaded through the service into.limit()/.offset()in the SQL. Ordering is unchanged (timestamp descending).X-Total-Countresponse header (and exposed via CORS) so the UI knows how many pages there are.GET /notificationtakes an optionalreadfilter. The UI previously built its "read" list by fetching everything and filtering client-side, which is not possible once the endpoint is paginated.b3d51c7fa9e2, chained offe60ae827ed98) adds an index ontimestamp DESCfor the ORDER BY, and a partial index (WHERE read = false) for the unread query.PATCH /notification/readmarks every unread notification as read in a singleUPDATE. "Mark All as Read" used to work by looping a PATCH over every notification the client held — that only worked because the client held all of them, and would silently mark just the current page now.Frontend
Tests
limitcaps the number returned,offsetskips correctly, ordering is timestamp descending, the unread query returns only unread rows and respects limit/offset, and the bulk mark-as-read reaches past the loaded page. Seeded with more rows than the page size so pagination is actually exercised.tests/, async SQLite viaaiosqlite, plus atestdependency group), since there wasn't one yet.Not in this PR
The reason that instance had 437k notifications is a separate root cause: identical failure notifications are written every import cycle, so the table floods. That's worth fixing on its own; this PR is limited to making the endpoint bounded and indexed, which is what makes the tab slow regardless of how the rows got there.
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_019wrZPxJDip5x3mzjWnpwos
Summary by CodeRabbit
limit/offset, plus an optional read-status filter.X-Total-Counttotal counts on notification responses to support “Load More”.