Notifications system wiring + preferences, UI, and tests#61
Open
Ananya44444 wants to merge 3 commits into
Open
Notifications system wiring + preferences, UI, and tests#61Ananya44444 wants to merge 3 commits into
Ananya44444 wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a preference-aware notification pipeline: DB schema and seeding, encryption/decryption helpers, event-driven notification creation, paginated/decrypted listing and preference CRUD APIs, static cache headers, frontend notification list and preferences pages with polling/caching, navbar/layout wiring, and tests covering triggers, APIs, and idempotency. ChangesNotification System with Preferences and Real-Time UI
Sequence DiagramsequenceDiagram
participant User as User
participant Navbar as Navbar UI
participant Layout as layout.js
participant NotifPage as Notifications Page
participant API as Backend API
participant DB as Database
User->>Navbar: Click notification bell
Navbar->>NotifPage: Navigate to /notifications.html
NotifPage->>Layout: await initLayout (optional)
NotifPage->>API: GET /api/notifications?limit=&offset=
API->>DB: SELECT notifications, unread_count
DB-->>API: encrypted rows + unread_count
API-->>NotifPage: {notifications, unread_count, limit, offset}
NotifPage->>NotifPage: decrypt & render, write cache
Layout->>API: (periodic) GET /api/notifications/unread-count via refreshUnreadBadge
API-->>Layout: {unread_count}
Layout->>Navbar: applyUnreadBadgeCount(unread_count)
NotifPage->>API: POST /api/notifications/{id}/read
API->>DB: UPDATE notification read state
DB-->>API: success
API-->>NotifPage: {success}
NotifPage->>NotifPage: refresh list (loadNotifications)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds in-app notifications with per-user preferences, including new API endpoints, schema, UI pages, an event-dispatch system, and tests. The list endpoint is rewritten to use AES-GCM decryption with a cached CryptoKey and to support pagination, and the navbar gains an unread badge with polling.
Changes:
- New
notification_preferencestable + GET/PATCH endpoints, and integration of preferences into_create_notification. - New event-handler registry (
emit_event+@_event_handler) wired into register/join/create_activity/create_session/add_activity_tags;api_joinmade idempotent. - New
notifications.htmlandnotification-preferences.htmlpages, navbar unread badge + polling inlayout.js, plus tests for the new API surface.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/worker.py | Adds preferences table, event dispatcher, notification handlers, preference-aware _create_notification, idempotent join, pagination on list, AES-GCM decrypt with cached key, PATCH method in CORS, static cache-control. |
| schema.sql | Adds notification_preferences table. |
| public/notifications.html | New notifications page with polling, caching, filters, mark-read. |
| public/notification-preferences.html | New preferences page. |
| public/partials/navbar.html | Bell badge + Notifications link (replaces Settings link). |
| public/js/layout.js | Unread badge polling, exposes initLayout/refreshUnreadBadge. |
| tests/test_notifications_api.py | API tests for list/unread/mark/mark-all. |
| tests/test_notification_preferences.py | Tests for preference GET/PATCH. |
| tests/test_notification_triggers.py | Tests for notification emission on join/create_session. |
| tests/test_join_idempotent.py | Test that re-join does not re-notify. |
| .gitignore | Ignore _django_ref/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@public/js/layout.js`:
- Around line 231-247: initLayout() can run twice because it’s invoked
immediately on DOM ready and also awaited elsewhere via window.initLayout; make
it idempotent by storing and returning a shared promise. Add a module-scoped or
global symbol (e.g., window.__initLayoutPromise) and in initLayout wrapper check
if that promise exists—if so, return it; otherwise set it to the async init task
(the current initializeDarkMode/inject/update calls) and return it. Ensure
window.initLayout is assigned to a function that returns that shared promise so
callers and the DOMContentLoaded path use the same bootstrap result.
In `@public/notification-preferences.html`:
- Line 26: The status container div with id="status" is not announced to screen
readers; update the <div id="status" ...> element to include ARIA live-region
attributes (e.g., add role="status" and aria-live="polite") so dynamic
success/error messages are announced, or use role="alert" when rendering
immediate error states; ensure the element with id "status" is the one modified
so assistive tech receives updates.
- Around line 96-117: The submit handler attached to
document.getElementById('prefs-form') doesn't catch fetch() rejections so
network failures produce unhandled rejections and no user feedback; wrap the
fetch/response handling in a try/catch (or add a .catch) around the
fetch(`${API}/api/notification-preferences`, ...) call, and in the catch call
showStatus with a clear error message (e.g. 'Network error: failed to save') and
return to stop further success handling; ensure you still parse non-OK responses
as before and reference the same payload and showStatus usage.
In `@public/notifications.html`:
- Around line 267-276: The pollUnreadCount function fetches unread_count and
then calls window.refreshUnreadBadge which currently triggers another fetch;
modify pollUnreadCount to pass the already-fetched count to the badge updater
(e.g., call window.refreshUnreadBadge(unread)) and update the refreshUnreadBadge
implementation to accept an optional count parameter and use it when provided
instead of performing its own fetch; keep existing behavior when count is
undefined so other callers are unaffected.
- Around line 90-91: The notifications cache key CACHE_KEY ('notif_cache_v1') is
global and can leak one user's notifications to another; change the code to
scope the cache to the signed-in user by composing the key with a stable
per-user identifier (for example: `${CACHE_KEY}:${user.id}` or
`${CACHE_KEY}:${user.email}`) wherever the cache is read/written (the places
using CACHE_KEY and CACHE_TTL_MS, e.g., the notification fetch/cache logic
referenced around the affected blocks). Ensure you obtain the current user's
unique id from the page's user/session object (or a provided auth token) and use
that derived key for both setItem and getItem calls so cached entries are
user-scoped and expire with the same TTL.
- Around line 42-45: The two filter buttons (ids "filter-all" and
"filter-unread") only convey selection via color; add aria-pressed="false" (or
"true" when active) to both elements in the HTML and update their aria-pressed
state inside the setFilter(isUnread) function so screen readers receive the
current selection; also apply the same aria-pressed addition and update logic
for the duplicate button instances around the other block referenced (the second
pair at lines 114-123) so both sets stay in sync with the filter boolean.
- Around line 339-341: The bug is that loadMore() increments the shared offset
before triggering loadNotifications(), causing skipped pages on fetch failure;
change the flow so offset is advanced only after a successful load: either have
loadNotifications return a Promise and call loadNotifications(false).then(() =>
{ offset += limit; }) or add a success callback/handler inside loadNotifications
to increment offset there (referencing loadMore(), loadNotifications(), offset
and limit) so the increment happens only on successful fetch.
In `@public/partials/navbar.html`:
- Around line 182-184: The icon-only notification link with id "notif-bell-link"
lacks a reliable accessible name; add an explicit aria-label="Notifications" to
the <a id="notif-bell-link"> element and, if JavaScript updates the unread count
in the span with id "notif-unread-badge", update the link's aria-label
dynamically to include the unread count (e.g., "Notifications, 3 unread")
whenever the badge changes so screen readers receive the count.
In `@src/worker.py`:
- Around line 2377-2386: Replace the dynamic column selection using pref_col and
inline SQL in the notification preference check with a fixed-column fetch: query
the notification_preferences row for the user_id selecting the known fixed
columns (e.g., columns referenced in _NOTIF_PREF_MAP) via
env.DB.prepare(...).bind(user_id).first(), then in Python inspect the
appropriate key (lookup _NOTIF_PREF_MAP[category]) on the returned record to
decide whether to return; keep the existing try/except behavior (catch Exception
and treat as default enabled) and continue to use the internal _NOTIF_PREF_MAP
and the same callsite that currently references pref_col to locate the change in
src/worker.py.
In `@tests/test_join_idempotent.py`:
- Around line 27-36: The test currently asserts the MockDB internal cursor
(env.DB._idx == 2) to detect no side effects; instead, stop coupling to MockDB
internals and assert the queued statements (e.g., stmt_act, stmt_existing or
whatever queued enrollment/notification statement objects created by MockDB) had
their .run() methods never called after calling worker.api_join(req, env).
Update tests/test_join_idempotent.py to remove the env.DB._idx assertion and
replace it with explicit assertions that the queued statements' .run() (or
equivalent execution sentinel on those mock statement objects) was not invoked,
while keeping the rest of the test (env = make_env(...), req, resp
status/message and call to worker.api_join) unchanged.
In `@tests/test_notification_preferences.py`:
- Around line 21-35: Add a new test in TestNotificationPreferences that checks
unauthenticated GET behavior: create an env (make_env with MockDB) and a
MockRequest for method="GET" to "/api/notification-preferences" but omit the
Authorization header, call worker.api_get_notification_preferences(req, env),
and assert the response status is 401 and the parsed body contains the expected
error shape/message; reference existing test_get_defaults for structure and use
_parse to inspect the response.
- Around line 37-54: The test test_patch_partial_update only asserts the HTTP
response but not that the DB upsert was called with the merged preferences;
update the test to also verify the MockDB/stmt_upsert received the expected
merged payload (enrollment_notify=True, session_notify=True,
system_notify=True). Locate the MockDB instance constructed via
make_env(db=MockDB([stmt_select, stmt_upsert])) and assert the upsert call
recorded by MockDB (or the args on stmt_upsert) contain those three boolean
values after calling worker.api_patch_notification_preferences; keep the
existing response assertions and add a single assertion that the persisted
upsert parameters equal the merged preference dict.
In `@tests/test_notification_triggers.py`:
- Around line 18-49: The test_join_creates_notifications currently only checks
that stmt_notif_insert_1.bind().run() and stmt_notif_insert_2.bind().run() were
called; update it to assert the actual bind arguments or intercepted payloads
contain the expected recipient ids and payload fields (e.g., ensure
stmt_notif_insert_1.bind was called with recipient_id == host-1 and payload
contains the participant name "v1:" and that stmt_notif_insert_2.bind was called
with recipient_id == user-1 and the correct join notification payload); locate
these in the test function test_join_creates_notifications and in the analogous
assertions around lines 51-86 and replace the loose .called checks with explicit
assertions on stmt_notif_insert_1.bind.call_args /
stmt_notif_insert_2.bind.call_args (or the MockDB interception mechanism) to
validate recipient ids and payload contents propagated by worker.api_join.
In `@tests/test_notifications_api.py`:
- Around line 78-89: The test test_mark_one_read currently only asserts HTTP
200; update it to verify the update mutation actually executed by asserting that
the mocked update statement (stmt_update) had its .run() called with the
expected bindings (notification id "n1" and the test user id from
_auth_header()) on the MockDB instance used to create env; locate where
stmt_update and MockDB are created in the test and add an assertion like
stmt_update.run.assert_called_once_with(expected_bindings) after the call to
worker.api_mark_notification_read to ensure the is_read update was invoked.
- Around line 29-52: The test currently only covers default listing for
api_list_notifications; add at least one additional request that supplies query
parameters (e.g., unread_only=1, limit and offset) via MockRequest to exercise
the new pagination/filter contract and assert the returned metadata
(body["data"]["total"], body["data"]["limit"], body["data"]["offset"], and
unread_count) and that notifications are filtered correctly; update
tests/test_notifications_api.py by creating a MockDB/stmt that represents
multiple rows (some read/unread), call worker.api_list_notifications with a URL
including ?unread_only=1&limit=2&offset=1, and add assertions for the pagination
fields and that returned notifications match the expected filtered slice.
🪄 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: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e39411ab-ad80-4fbb-a418-c2fc6b910dfc
📒 Files selected for processing (11)
.gitignorepublic/js/layout.jspublic/notification-preferences.htmlpublic/notifications.htmlpublic/partials/navbar.htmlschema.sqlsrc/worker.pytests/test_join_idempotent.pytests/test_notification_preferences.pytests/test_notification_triggers.pytests/test_notifications_api.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_notification_triggers.py (1)
55-92: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd an opt-out case for trigger delivery.
stmt_pref_batchonly exercises the default-enabled path, so this suite still misses the core “recipient disabled notifications” contract. A case wheresession_notify=0foruser-2andstmt_notif_insertnever runs would catch regressions in the new preference gate quickly.As per coding guidelines,
**/tests/**: "Ensure tests are descriptive, cover edge cases, and follow the project's existing test patterns."🤖 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 `@tests/test_notification_triggers.py` around lines 55 - 92, Add a new test (or extend the existing test_create_session_creates_notifications) to exercise the opt-out path by mocking stmt_pref_batch to return a preference row with session_notify=0 for user "user-2" and asserting that stmt_notif_insert.bind(...).run is not called; locate the test using symbols test_create_session_creates_notifications, stmt_pref_batch, stmt_notif_insert, and MockDB/make_env, set stmt_pref_batch to include a preference result for user-2 with session_notify=0, call worker.api_create_session with the same request, then assert stmt_notif_insert.bind.return_value.run was not invoked to verify the preference gate.
♻️ Duplicate comments (1)
public/notifications.html (1)
90-91: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding username as a secondary fallback for cache scoping.
The cache is now user-scoped with
user.id, which resolves the privacy concern. However, ifuser.idhappens to beundefinedornull(but the user object exists), multiple users could still share the'anon'key. Adding the username as a secondary fallback provides extra defense.♻️ Optional improvement
- const CACHE_KEY = `notif_cache_v1_${user.id || 'anon'}`; + const CACHE_KEY = `notif_cache_v1_${user.id || user.username || 'anon'}`;🤖 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 `@public/notifications.html` around lines 90 - 91, Update the cache key construction so it falls back to the user's username when user.id is missing: modify the CACHE_KEY expression (currently using CACHE_KEY = `notif_cache_v1_${user.id || 'anon'}`) to include a secondary fallback like user.username (e.g. use user.id || user.username || 'anon') so anonymous IDs don't collide across distinct users who lack an id but have different usernames.
🤖 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 `@src/worker.py`:
- Around line 2388-2396: The handler for activity tag updates is checking the
wrong preference and category: it calls _get_pref_map(env, recipient_ids,
"enrollment_notify") and creates notifications with category="enrollment";
change the preference bucket to the system preference (e.g., "system_notify")
and set category="system" when calling _create_notification so the handler
consults system preferences instead of enrollment preferences (adjust both the
_get_pref_map call and the category argument in the _create_notification
invocation).
---
Outside diff comments:
In `@tests/test_notification_triggers.py`:
- Around line 55-92: Add a new test (or extend the existing
test_create_session_creates_notifications) to exercise the opt-out path by
mocking stmt_pref_batch to return a preference row with session_notify=0 for
user "user-2" and asserting that stmt_notif_insert.bind(...).run is not called;
locate the test using symbols test_create_session_creates_notifications,
stmt_pref_batch, stmt_notif_insert, and MockDB/make_env, set stmt_pref_batch to
include a preference result for user-2 with session_notify=0, call
worker.api_create_session with the same request, then assert
stmt_notif_insert.bind.return_value.run was not invoked to verify the preference
gate.
---
Duplicate comments:
In `@public/notifications.html`:
- Around line 90-91: Update the cache key construction so it falls back to the
user's username when user.id is missing: modify the CACHE_KEY expression
(currently using CACHE_KEY = `notif_cache_v1_${user.id || 'anon'}`) to include a
secondary fallback like user.username (e.g. use user.id || user.username ||
'anon') so anonymous IDs don't collide across distinct users who lack an id but
have different usernames.
🪄 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: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e9df1c3-da76-430e-89ba-e9e85f26a7c5
📒 Files selected for processing (9)
public/js/layout.jspublic/notification-preferences.htmlpublic/notifications.htmlpublic/partials/navbar.htmlsrc/worker.pytests/test_join_idempotent.pytests/test_notification_preferences.pytests/test_notification_triggers.pytests/test_notifications_api.py
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
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.
-Add notification preferences and API endpoints.
-Wire notifications to enrollments, sessions, activities, and tag updates.
-Improve notifications UI with polling, unread badge, and preferences page.
-Add tests for notifications API, preferences, triggers, and idempotent join.
Changes
Backend
-Add notification_preferences table and DDL.
-Add GET/PATCH /api/notification-preferences.
-Add events for enrollment, session creation, activity creation, and tag updates.
-Guard against duplicate enrollment notifications.
-Include participant name in host enrollment notifications.
-Pagination with [limit]/[offset]and decrypt titles/messages on read.
Frontend
-Notifications page with unread filter, mark‑read/all, polling refresh.
-Preferences page with three toggles (enrollment/session/system).
-Navbar bell link and unread badge.
Tests
-Notifications API coverage (list, unread count, mark one/all, auth/404).
-Preferences API coverage (GET defaults, PATCH partial, validation).
-Trigger coverage (join + session create).
-Idempotent join test.
Purpose
Implements a full notifications system: backend schema, event wiring, APIs, encrypted storage/decryption, frontend UI (notifications + preferences + navbar integration), polling/caching UX, and tests to cover API behavior and event triggers.
Key modifications
Backend
Frontend
Tests
Impact