Add in-portal notification system with admin broadcasts#761
Open
JoaquinBN wants to merge 20 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full in-portal notifications system: DB models and migration, a registry and services layer (personal + broadcast semantics, dedupe, feeds, read-state), admin broadcast mixin and integrations, DRF API with read/mark actions, frontend store/UI (navbar bell + notifications page), workflow hooks, tests, and docs. ChangesIn-Portal Notification System
Sequence Diagram(s)sequenceDiagram
participant User
participant SPA as Frontend SPA
participant API as Notification API
participant Service as Notification Service
participant DB as Database
User->>SPA: open bell / view notifications
SPA->>API: GET /notifications/?page=...
API->>Service: feed_for(user) + annotate_read_state
Service->>DB: Query Notification & NotificationReceipt
DB-->>Service: rows + receipts
Service-->>API: serialized notifications
API-->>SPA: notifications list
User->>SPA: click notification
SPA->>API: POST /notifications/{id}/mark-read/
API->>Service: mark_notification_read(notification, user)
Service->>DB: update read state / create receipt
DB-->>Service: OK
API-->>SPA: updated notification (is_read=true)
SPA->>SPA: navigate to link_url
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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/routes/MySubmissions.svelte (1)
95-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider removing redundant
loadSubmissions()call fromonMount.The
onMounthandler callsloadSubmissions()at line 129 after a 100ms delay, but the first$effect(lines 95-103) already callsloadSubmissions()immediately when authenticated. This results in two API calls on initial mount for authenticated users: one immediate (from the effect) and one delayed (fromonMount).The effects already handle both authentication state changes and querystring updates, making the
onMountcall redundant. Consider simplifyingonMountto only handle the success message check (lines 121-125) and let the effects manage data loading.🎯 Suggested refactor to eliminate duplicate calls
onMount(async () => { syncSubmissionDeepLink($querystring); // Check for success message from edit submission const updateSuccess = sessionStorage.getItem('submissionUpdateSuccess'); if (updateSuccess) { showSuccess(updateSuccess); sessionStorage.removeItem('submissionUpdateSuccess'); } - - // Wait a moment for auth state to be verified - await new Promise(resolve => setTimeout(resolve, 100)); - loadSubmissions(); });The deep-link sync at line 118 can also be removed since the second effect (line 110) handles it.
🤖 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 `@frontend/src/routes/MySubmissions.svelte` around lines 95 - 130, The onMount handler is causing a duplicate API call by invoking loadSubmissions() after a delay even though the two $effect blocks already call loadSubmissions() and sync deep links; remove the loadSubmissions() call (and optional syncSubmissionDeepLink($querystring) if you accept the suggestion) from onMount so onMount only handles the sessionStorage success message logic; keep the $effect blocks (which reference $authState, authChecked, $querystring, lastAppliedQuerystring, syncSubmissionDeepLink, and loadSubmissions) as the sole triggers for loading submissions.
🤖 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 `@backend/CLAUDE.md`:
- Around line 146-161: The MD022 lint error is caused by a missing blank line
before the "### Notifications" heading in backend/CLAUDE.md; open the file, find
the "### Notifications" heading (the section that lists Models:
`notifications/models.py`, Registry: `notifications/registry.py`, Services:
`notifications/services.py`, etc.), and insert a single blank line immediately
above that heading so there is an empty line separator before "###
Notifications" to satisfy the markdown lint rule.
In `@backend/contributions/admin.py`:
- Around line 1-3: Remove the duplicate import of "from django import forms" in
admin.py — keep a single "from django import forms" and ensure the other imports
(e.g., "from django.contrib import admin") remain untouched; locate the
duplicate by searching for the symbol "forms" or the exact import statement and
delete the redundant line, then run linting/formatting to confirm no
unused-import warnings remain.
In `@backend/notifications/tests.py`:
- Around line 217-347: Add two tests to NotificationAPITests: one to verify
pagination (e.g., create > page_size notifications using
services.broadcast_partner/services.notify, call GET '/api/v1/notifications/'
with page/page_size params, assert response.data['results'] length equals
page_size and presence/values of response.data['next'] and
response.data['previous']) and one to verify category filtering (create
notifications with differing event_type or category like 'submission' and
'partner.published', call GET '/api/v1/notifications/?category=submission', and
assert only submission-related notifications are returned). Place these as new
methods (e.g., test_pagination and test_category_filter) near
test_feed_lists_personal_and_broadcast_chronologically and reuse
services.notify, services.broadcast_partner and self.client to construct
assertions consistent with test_unread_filter.
- Around line 217-283: Add two new tests in NotificationAPITests to assert user
isolation: (1) create user_a and user_b, create a personal notification for
user_a via services.notify(recipient=user_a,...), then force_authenticate the
test client as user_b and GET '/api/v1/notifications/' and assert the user_a
notification's pk/event_type is not present in the results; (2) create a
personal notification for user_a, authenticate as user_b and POST to
'/api/v1/notifications/{pk}/mark-read/' for that notification and assert the
response is unauthorized/not found (status 403/404) and that the notification's
is_read on the Notification model has not been changed. Ensure you use
NotificationAPITests setup helpers, services.notify, and the existing endpoints
'/api/v1/notifications/' and '/api/v1/notifications/{pk}/mark-read/' to locate
code.
---
Outside diff comments:
In `@frontend/src/routes/MySubmissions.svelte`:
- Around line 95-130: The onMount handler is causing a duplicate API call by
invoking loadSubmissions() after a delay even though the two $effect blocks
already call loadSubmissions() and sync deep links; remove the loadSubmissions()
call (and optional syncSubmissionDeepLink($querystring) if you accept the
suggestion) from onMount so onMount only handles the sessionStorage success
message logic; keep the $effect blocks (which reference $authState, authChecked,
$querystring, lastAppliedQuerystring, syncSubmissionDeepLink, and
loadSubmissions) as the sole triggers for loading submissions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 678c55b6-e044-4601-af30-160a6203ed59
📒 Files selected for processing (35)
CHANGELOG.mdbackend/CLAUDE.mdbackend/api/urls.pybackend/contributions/admin.pybackend/contributions/migrations/0051_zero_validator_waitlist_points.pybackend/contributions/node_upgrade/admin.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/gen_tv/admin.pybackend/notifications/__init__.pybackend/notifications/admin.pybackend/notifications/admin_mixins.pybackend/notifications/apps.pybackend/notifications/migrations/0001_initial.pybackend/notifications/migrations/__init__.pybackend/notifications/models.pybackend/notifications/receivers.pybackend/notifications/registry.pybackend/notifications/serializers.pybackend/notifications/services.pybackend/notifications/tests.pybackend/notifications/views.pybackend/partners/admin.pybackend/poaps/admin.pybackend/tally/settings.pybackend/users/admin.pyfrontend/CLAUDE.mdfrontend/src/App.sveltefrontend/src/components/Navbar.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/lib/api.jsfrontend/src/lib/notificationStore.jsfrontend/src/lib/relativeTime.jsfrontend/src/routes/MySubmissions.sveltefrontend/src/routes/Notifications.svelte
JoaquinBN
force-pushed
the
JoaquinBN/portal-alerts
branch
from
d12af9d to
f3abf26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tally/settings.py (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
testserverinALLOWED_HOSTS.Line 43 only uses environment values; if
testserveris omitted there, Django test-client requests can fail host validation. Add it unconditionally (deduped) in settings.Suggested patch
-ALLOWED_HOSTS = get_required_env('ALLOWED_HOSTS').split(',') +ALLOWED_HOSTS = list( + dict.fromkeys([*get_required_env('ALLOWED_HOSTS').split(','), 'testserver']) +)As per coding guidelines,
backend/**/settings.pymust add'testserver'toALLOWED_HOSTSfor test database flows.🤖 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 `@backend/tally/settings.py` at line 43, ALLOWED_HOSTS is constructed from get_required_env('ALLOWED_HOSTS') only, which can omit 'testserver' and break Django test-client host validation; update the settings so after calling get_required_env('ALLOWED_HOSTS').split(',') you append or insert 'testserver' and ensure duplicates are removed (e.g., by converting to a deduping collection) before assigning back to ALLOWED_HOSTS; modify the code around the ALLOWED_HOSTS assignment (referencing ALLOWED_HOSTS and get_required_env) to always include 'testserver' in the final list.Source: Coding guidelines
🤖 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 `@frontend/src/components/NotificationCenter.svelte`:
- Around line 27-37: Extract the duplicated followNotification logic into a
single exported helper (e.g., create frontend/src/lib/notificationUtils.js and
export function followNotification) that implements the same behavior (use
link_url || '', return if empty, open external http/https links with window.open
and noopener/noreferrer, otherwise call push with url.slice(1) when it starts
with '`#/`'). Replace the local followNotification implementations in
NotificationCenter.svelte and Notifications.svelte to import and call the shared
followNotification helper so both components share identical URL-handling logic.
In `@frontend/src/lib/notificationStore.js`:
- Around line 4-8: The helper function asList is duplicated; extract it into a
shared utility module (e.g., create frontend/src/lib/utils.js or arrayUtils.js)
that exports asList, update notificationStore.js to import { asList } from that
module and remove the local definition, and also update Notifications.svelte to
import and use the same asList export to ensure a single source of truth and
avoid duplication.
---
Outside diff comments:
In `@backend/tally/settings.py`:
- Line 43: ALLOWED_HOSTS is constructed from get_required_env('ALLOWED_HOSTS')
only, which can omit 'testserver' and break Django test-client host validation;
update the settings so after calling
get_required_env('ALLOWED_HOSTS').split(',') you append or insert 'testserver'
and ensure duplicates are removed (e.g., by converting to a deduping collection)
before assigning back to ALLOWED_HOSTS; modify the code around the ALLOWED_HOSTS
assignment (referencing ALLOWED_HOSTS and get_required_env) to always include
'testserver' in the final list.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b2e77ba-8bb1-4faf-a616-7fe4ac80ec78
📒 Files selected for processing (34)
CHANGELOG.mdbackend/CLAUDE.mdbackend/api/urls.pybackend/contributions/admin.pybackend/contributions/node_upgrade/admin.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/gen_tv/admin.pybackend/notifications/__init__.pybackend/notifications/admin.pybackend/notifications/admin_mixins.pybackend/notifications/apps.pybackend/notifications/migrations/0001_initial.pybackend/notifications/migrations/__init__.pybackend/notifications/models.pybackend/notifications/receivers.pybackend/notifications/registry.pybackend/notifications/serializers.pybackend/notifications/services.pybackend/notifications/tests.pybackend/notifications/views.pybackend/partners/admin.pybackend/poaps/admin.pybackend/tally/settings.pybackend/users/admin.pyfrontend/CLAUDE.mdfrontend/src/App.sveltefrontend/src/components/Navbar.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/lib/api.jsfrontend/src/lib/notificationStore.jsfrontend/src/lib/relativeTime.jsfrontend/src/routes/MySubmissions.sveltefrontend/src/routes/Notifications.svelte
JoaquinBN
force-pushed
the
JoaquinBN/portal-alerts
branch
from
f3abf26 to
0d2bed3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/notifications/views.py`:
- Around line 9-12: Override NotificationViewSet.get_serializer_context to add
context['use_light_serializers'] = self.action == 'list' so list actions use
light serializers; keep existing base context (call
super().get_serializer_context()), set the flag, and return the updated context.
Update the class NotificationViewSet (which already defines serializer_class =
NotificationSerializer and permission_classes) to include this method.
In `@frontend/src/components/Navbar.svelte`:
- Around line 61-62: Navbar mounts NotificationCenter twice (desktop + mobile),
causing duplicate polling/listeners; remove the duplicate mount and ensure only
one instance is rendered. Locate the two <NotificationCenter /> usages and keep
a single instance (e.g., the desktop one) that is conditionally rendered with
{`#if` $authState.isAuthenticated}; remove the second/mobile <NotificationCenter
/> inside the mobile block (the one hidden/shown via CSS classes like
hidden/md:*) so only one component mounts; if mobile needs separate placement,
render NotificationCenter once and position it with CSS or a portal rather than
duplicating the component.
In `@frontend/src/routes/MySubmissions.svelte`:
- Around line 30-41: The syncSubmissionDeepLink function leaves a stale
stateFilter when the URL's state param is missing or invalid; update
syncSubmissionDeepLink to explicitly reset stateFilter to a default (e.g., '' or
a designated defaultState) and set currentPage appropriately when
params.get('state') is falsy or not in validStateFilters. In other words, after
computing requestedState, if requestedState is present and valid set stateFilter
and currentPage as before, otherwise clear/reset stateFilter (and optionally
reset highlightedSubmissionId behavior if desired) so navigating to plain
`#/my-submissions` removes prior filters.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3756a5e9-0f5a-472f-b812-7bc264963b1f
📒 Files selected for processing (35)
CHANGELOG.mdbackend/CLAUDE.mdbackend/api/urls.pybackend/contributions/admin.pybackend/contributions/node_upgrade/admin.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/gen_tv/admin.pybackend/notifications/__init__.pybackend/notifications/admin.pybackend/notifications/admin_mixins.pybackend/notifications/apps.pybackend/notifications/migrations/0001_initial.pybackend/notifications/migrations/__init__.pybackend/notifications/models.pybackend/notifications/receivers.pybackend/notifications/registry.pybackend/notifications/serializers.pybackend/notifications/services.pybackend/notifications/tests.pybackend/notifications/views.pybackend/partners/admin.pybackend/poaps/admin.pybackend/tally/settings.pybackend/users/admin.pyfrontend/CLAUDE.mdfrontend/src/App.sveltefrontend/src/components/Navbar.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/lib/api.jsfrontend/src/lib/notificationStore.jsfrontend/src/lib/notificationUtils.jsfrontend/src/lib/relativeTime.jsfrontend/src/routes/MySubmissions.sveltefrontend/src/routes/Notifications.svelte
👮 Files not reviewed due to content moderation or server errors (8)
- backend/notifications/registry.py
- backend/notifications/services.py
- backend/notifications/receivers.py
- backend/contributions/views.py
- backend/ethereum_auth/views.py
- backend/users/admin.py
- backend/notifications/admin_mixins.py
- backend/contributions/admin.py
Comment on lines
+61
to
62
| <NotificationCenter /> | ||
| {#if $authState.isAuthenticated} |
There was a problem hiding this comment.
Avoid mounting NotificationCenter twice; it duplicates polling and listeners.
Line 61 and Line 78 render two instances at once (desktop + mobile). CSS hidden/md:* only hides visually; both components still mount, so both run loadLatest() and the 60s unread polling/document click listener from NotificationCenter.svelte.
Suggested fix
+ import { onMount } from 'svelte';
import NotificationCenter from './NotificationCenter.svelte';
+
+ let isDesktop = $state(typeof window !== 'undefined' ? window.innerWidth >= 768 : false);
+
+ onMount(() => {
+ const onResize = () => {
+ isDesktop = window.innerWidth >= 768;
+ };
+ window.addEventListener('resize', onResize);
+ return () => window.removeEventListener('resize', onResize);
+ });
<div class="hidden md:flex items-center gap-2">
- <NotificationCenter />
+ {`#if` isDesktop}
+ <NotificationCenter />
+ {/if}
{`#if` $authState.isAuthenticated}
<SearchBar />
{/if}
...
</div>
<div class="navbar-mobile-actions flex items-center md:hidden gap-2">
- <NotificationCenter />
+ {`#if` !isDesktop}
+ <NotificationCenter />
+ {/if}
<AuthButton />
...
</div>Also applies to: 78-79
🤖 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 `@frontend/src/components/Navbar.svelte` around lines 61 - 62, Navbar mounts
NotificationCenter twice (desktop + mobile), causing duplicate
polling/listeners; remove the duplicate mount and ensure only one instance is
rendered. Locate the two <NotificationCenter /> usages and keep a single
instance (e.g., the desktop one) that is conditionally rendered with {`#if`
$authState.isAuthenticated}; remove the second/mobile <NotificationCenter />
inside the mobile block (the one hidden/shown via CSS classes like hidden/md:*)
so only one component mounts; if mobile needs separate placement, render
NotificationCenter once and position it with CSS or a portal rather than
duplicating the component.
0d2bed3 to
de75585
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/routes/MySubmissions.svelte (1)
99-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid duplicate initial fetches from both
$effectandonMount.When already authenticated, both paths call
loadSubmissions(), causing redundant API calls and possible response-order flicker.Suggested fix
- onMount(async () => { + onMount(() => { syncSubmissionDeepLink($querystring); @@ - // Wait a moment for auth state to be verified - await new Promise(resolve => setTimeout(resolve, 100)); - loadSubmissions(); });Also applies to: 121-134
🤖 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 `@frontend/src/routes/MySubmissions.svelte` around lines 99 - 107, The $effect and onMount both call loadSubmissions() when the user is already authenticated, causing duplicate API calls and flicker; fix by adding a single guard to prevent double-fetches (e.g., introduce a boolean submissionsLoaded or reuse authChecked) and check it before calling loadSubmissions in either $effect or onMount (set submissionsLoaded = true after successful load); alternatively, change the $effect to only run on subsequent auth changes by checking authChecked or a similar flag so only one path (onMount or the effect) triggers loadSubmissions for the initial authenticated state.
🤖 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 `@backend/notifications/registry.py`:
- Line 53: EVENT_TYPES is built by silently overwriting duplicates from
_EVENT_TYPES; change the construction to detect duplicate slugs at import time
and raise a clear exception. Iterate over _EVENT_TYPES (or use a counter) and if
a slug from an event already exists in the map, raise a ValueError (or
RuntimeError) that includes the duplicate slug and the two conflicting event
identifiers (e.g., event.name or type) so collisions fail fast; ensure the final
variable name remains EVENT_TYPES and no silent overwrite occurs.
In `@frontend/src/App.svelte`:
- Line 213: Wrap the '/notifications' route registration with the existing
protectedRoute guard so only authenticated users can access it; specifically
replace the unguarded mapping that uses Notifications with
protectedRoute(Notifications) (or whatever protectedRoute wrapper is used
elsewhere) where routes are registered (look for the routes object/registration
near App.svelte and the protectedRoute helper) so navigation to '/notifications'
requires authentication.
In `@frontend/src/lib/notificationStore.js`:
- Around line 106-114: The markRead handler always decrements unreadCount
regardless of prior state; change markRead (and its update callback) to first
find the existing item in state.items by id, compare its previous unread/read
status with the returned updated object from notificationsAPI.markRead, and only
decrement unreadCount when the previous item was unread and the updated item is
marked read; update items with the updated object as before and ensure
unreadCount is bounded by Math.max(0, ...).
In `@frontend/src/lib/relativeTime.js`:
- Around line 6-22: The relativeTime function needs to explicitly handle
null/invalid timestamps: at the top of relativeTime (before computing seconds)
check whether value is null/undefined or the constructed Date is invalid (use
isNaN(date.getTime()) or Number.isNaN(date.getTime())); if invalid, return a
safe fallback (e.g., empty string or '—') to avoid rendering "Invalid Date" in
the UI, otherwise continue with the existing logic that computes seconds,
minutes, hours, days and returns formatted strings.
In `@frontend/src/routes/MySubmissions.svelte`:
- Around line 33-36: The highlight ID is stored as a string from
params.get('submission') (requestedSubmission) while submission.id is numeric,
causing strict comparisons to fail; update the code that sets
highlightedSubmissionId (and the other occurrence around where submission IDs
are compared at the end of the file) to normalize types consistently—either
parse requestedSubmission to a number before assigning highlightedSubmissionId
or convert submission.id to string at comparison time—so strict equality checks
match reliably; reference highlightedSubmissionId, requestedSubmission, and
submission.id when making the change.
In `@frontend/src/routes/Notifications.svelte`:
- Around line 18-47: loadNotifications can have out-of-order async responses
overwrite newer state; add a monotonic request token to ignore stale responses:
declare a module-scoped counter (e.g., let latestLoadId = 0), increment it at
the start of loadNotifications and capture the incremented id in a local
variable (e.g., const thisLoadId = ++latestLoadId) before awaiting
notificationsAPI.list, then after the await (in both success and catch) check if
thisLoadId === latestLoadId and only then update notifications, totalCount,
hasNext, currentPage, loading and error; apply the same pattern to the other
load call at lines 67-71 to ensure older responses are ignored.
---
Outside diff comments:
In `@frontend/src/routes/MySubmissions.svelte`:
- Around line 99-107: The $effect and onMount both call loadSubmissions() when
the user is already authenticated, causing duplicate API calls and flicker; fix
by adding a single guard to prevent double-fetches (e.g., introduce a boolean
submissionsLoaded or reuse authChecked) and check it before calling
loadSubmissions in either $effect or onMount (set submissionsLoaded = true after
successful load); alternatively, change the $effect to only run on subsequent
auth changes by checking authChecked or a similar flag so only one path (onMount
or the effect) triggers loadSubmissions for the initial authenticated state.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 107bea73-210d-47f1-90ed-f016e2b0d60e
📒 Files selected for processing (35)
CHANGELOG.mdbackend/CLAUDE.mdbackend/api/urls.pybackend/contributions/admin.pybackend/contributions/node_upgrade/admin.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/gen_tv/admin.pybackend/notifications/__init__.pybackend/notifications/admin.pybackend/notifications/admin_mixins.pybackend/notifications/apps.pybackend/notifications/migrations/0001_initial.pybackend/notifications/migrations/__init__.pybackend/notifications/models.pybackend/notifications/receivers.pybackend/notifications/registry.pybackend/notifications/serializers.pybackend/notifications/services.pybackend/notifications/tests.pybackend/notifications/views.pybackend/partners/admin.pybackend/poaps/admin.pybackend/tally/settings.pybackend/users/admin.pyfrontend/CLAUDE.mdfrontend/src/App.sveltefrontend/src/components/Navbar.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/lib/api.jsfrontend/src/lib/notificationStore.jsfrontend/src/lib/notificationUtils.jsfrontend/src/lib/relativeTime.jsfrontend/src/routes/MySubmissions.sveltefrontend/src/routes/Notifications.svelte
| '/contributions/:id': EditSubmission, | ||
| '/metrics': Metrics, | ||
| '/profile': ProfileEdit, | ||
| '/notifications': Notifications, |
There was a problem hiding this comment.
Protect /notifications with the existing auth route guard.
Line 213 currently registers the route without protectedRoute(...), which conflicts with the authenticated-only notifications contract and allows direct unauthenticated navigation to that page shell.
Proposed fix
- '/notifications': Notifications,
+ '/notifications': protectedRoute(Notifications),📝 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
| '/notifications': Notifications, | |
| '/notifications': protectedRoute(Notifications), |
🤖 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 `@frontend/src/App.svelte` at line 213, Wrap the '/notifications' route
registration with the existing protectedRoute guard so only authenticated users
can access it; specifically replace the unguarded mapping that uses
Notifications with protectedRoute(Notifications) (or whatever protectedRoute
wrapper is used elsewhere) where routes are registered (look for the routes
object/registration near App.svelte and the protectedRoute helper) so navigation
to '/notifications' requires authentication.
The portal now has a notification center: a navbar bell with unread badge and dropdown, and a full notifications page with unread filtering and pagination. Personal notifications fire automatically for submission review outcomes (including bulk rejections), highlighted contributions, referrals joining, and validator graduation. Announcements are admin-explicit broadcasts: featured content, system alerts, ecosystem partners, contribution types, missions, Gen TV streams, POAP drops, and node upgrade targets stay silent on save unless the admin checks a broadcast control or runs the bulk action. A broadcast is stored as a single row scoped to an audience (all users, validators, stewards), with per-user read state in lazily created receipts, so fan-out cost stays flat and re-broadcasting refreshes instead of duplicating. An event registry centralizes category, priority, audience, and future delivery channels (email, Telegram), so adding a notification is one registry entry plus one producer call. A reusable admin mixin provides the broadcast checkbox and bulk action to any ModelAdmin. Also repairs two data migrations that made fresh test databases unbuildable. - backend/notifications/models.py: Notification (nullable recipient = broadcast, audience field, frozen copy, payload JSON, dedupe_key) and NotificationReceipt (lazy per-user broadcast read state) - backend/notifications/registry.py: EventType dataclass + EVENT_TYPES dict, single source of truth for event metadata and future channels - backend/notifications/services.py: core notify()/broadcast(), feed_for()/annotate_read_state()/mark_* helpers, per-event producers and broadcasters - backend/notifications/admin_mixins.py: BroadcastNotificationAdminMixin (dynamic form fields, fieldset injection, save_model hook, bulk action, reach feedback) - backend/notifications/receivers.py, apps.py: post_save on ContributionHighlight notifies the contribution owner - backend/notifications/views.py, serializers.py: feed (reverse-chronological, unread/category filters), unread-count, receipt-aware mark-read, mark-all-read - backend/notifications/admin.py: Notification and NotificationReceipt admins - backend/notifications/tests.py: 16 tests (dedupe, receipts, audiences, join-date cutoff, API, admin mixin render + broadcast POST) - backend/notifications/migrations/0001_initial.py: initial schema - backend/api/urls.py: register notifications router - backend/tally/settings.py: install notifications app - backend/contributions/admin.py: FeaturedContent/Alert/ContributionType/Mission admins use the broadcast mixin (replaces bespoke per-admin forms) - backend/contributions/views.py: submission review + bulk reject create notifications; owner-scoped ?submission= deep-link filter on /submissions/my/ - backend/contributions/node_upgrade/admin.py: TargetNodeVersion broadcasts to the validators audience - backend/partners/admin.py, backend/gen_tv/admin.py, backend/poaps/admin.py: broadcast mixin on Partner/Stream/PoapDrop admins - backend/ethereum_auth/views.py: notify referrer on referred signup (guarded so it never breaks login) - backend/users/admin.py: set_as_validator action notifies the graduated user - backend/contributions/migrations/0037_seed_featured_content.py: skip seeding when fixture users are absent (fresh DBs) - backend/contributions/migrations/0051_zero_validator_waitlist_points.py: skip live-model leaderboard recalculation on empty databases - frontend/src/components/NotificationCenter.svelte: navbar bell + dropdown, Tailwind/portal design - frontend/src/routes/Notifications.svelte: full feed page (All/Unread pills, load more) - frontend/src/lib/notificationStore.js: shared store powering dropdown and page - frontend/src/lib/relativeTime.js: shared compact timestamp util - frontend/src/lib/api.js: notificationsAPI (list, unreadCount, markRead, markAllRead) - frontend/src/components/Navbar.svelte, frontend/src/App.svelte: mount NotificationCenter, register /notifications route - frontend/src/routes/MySubmissions.svelte: state/submission query params filter, scroll to and highlight the target row - backend/CLAUDE.md, frontend/CLAUDE.md: document the notifications app, endpoints, components, and store
Adds the test coverage CodeRabbit flagged: user isolation tests proving one user can neither see nor mark-read another user's personal notifications, plus pagination and category filter tests for the feed. Removes a duplicate forms import introduced by the rebase, and restores the 0051 waitlist-points migration to dev's exact version since dev's added migration dependencies now fix fresh-database builds on their own, making our guard redundant (verified: full suite passes against the unmodified migration). ## Claude Implementation Notes - backend/contributions/admin.py: drop duplicate `from django import forms` - backend/notifications/tests.py: add test_category_filter, test_pagination, test_user_cannot_see_other_users_personal_notifications, test_user_cannot_mark_other_users_notification_read (foreign mark-read returns 404 and leaves read_at null) - backend/contributions/migrations/0051_zero_validator_waitlist_points.py: revert to origin/dev verbatim; PR no longer modifies any pre-existing migration
The navbar bell now sits to the left of the search bar on desktop so notification access is grouped with the other lookup tools; mobile layout is unchanged. The duplicated notification link-following and API payload normalization helpers now live in a single shared module used by the store, the dropdown, and the notifications page, addressing the CodeRabbit duplication findings. ## Claude Implementation Notes - frontend/src/components/Navbar.svelte: NotificationCenter moved before SearchBar in the desktop actions row - frontend/src/lib/notificationUtils.js: new shared module exporting asList and followNotificationLink - frontend/src/lib/notificationStore.js, components/NotificationCenter.svelte, routes/Notifications.svelte: import the shared helpers, local copies removed; unused push import dropped from the page - frontend/CLAUDE.md: document notificationUtils.js and the bell position
The main steward review endpoint notifies the submitter again: the
hook was lost in a rebase over the security-hardening refactor, so
only update-accepted and bulk-reject were emitting decision
notifications. A view-level regression test now locks the main path
in. On the frontend, the navbar bell mounts twice (desktop and mobile
rows), so unread polling is now a refcounted singleton in the
notification store and loads coalesce while a request is in flight;
store resets bump an epoch that discards in-flight responses so a
logout or wallet switch cannot leak the previous session's feed. The
notifications ViewSet sets the use_light_serializers context flag per
backend convention, and My Submissions clears a deep-linked state
filter when the URL no longer carries a valid state.
## Claude Implementation Notes
- backend/contributions/views.py: review action calls notify_submission_review after the decision is saved, matching update_accepted/bulk_reject
- backend/notifications/tests.py: StewardReviewEndpointNotificationTests posts to /steward-submissions/{id}/review/ and asserts the submitter notification (steward + reject StewardPermission fixtures)
- backend/notifications/views.py: get_serializer_context sets use_light_serializers for the list action
- frontend/src/lib/notificationStore.js: refcounted startPolling() singleton, in-flight coalescing for loadLatest/loadUnreadCount, epoch guard on reset() that discards stale responses
- frontend/src/components/NotificationCenter.svelte: use the store's shared polling instead of a per-instance interval
- frontend/src/routes/MySubmissions.svelte: syncSubmissionDeepLink resets stateFilter when the state param is absent or invalid
JoaquinBN
force-pushed
the
JoaquinBN/portal-alerts
branch
from
de75585 to
3751330
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
frontend/src/lib/notificationStore.js (1)
106-117: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider transition-aware unread decrement.
Line 113 always decrements
unreadCounteven if the notification was already marked as read. While the polling mechanism will eventually correct the count, duplicatemarkReadcalls (e.g., user clicks same notification twice quickly) could temporarily desync the badge.♻️ Optional improvement
async function markRead(id) { const response = await notificationsAPI.markRead(id); const updated = response.data; update((state) => { + const wasUnread = state.items.some((item) => item.id === id && !item.is_read); return { ...state, items: state.items.map((item) => (item.id === id ? updated : item)), - unreadCount: Math.max(0, state.unreadCount - 1) + unreadCount: wasUnread ? Math.max(0, state.unreadCount - 1) : state.unreadCount }; }); return updated; }🤖 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 `@frontend/src/lib/notificationStore.js` around lines 106 - 117, In markRead, avoid always decrementing unreadCount; before calling update check the current item in state.items (or inside the update callback) to see if the item with id is currently unread, and only subtract 1 when its prior read flag is false (or when updated.read === true and prior.read === false). Use the existing symbols: function markRead, notificationsAPI.markRead, updated, and the update((state) => ...) callback to conditionally update unreadCount so duplicate markRead calls won’t decrement the badge incorrectly.frontend/src/App.svelte (1)
215-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect
/notificationswithprotectedRoute(...).This route bypasses the app’s auth route-guard path and allows unauthenticated direct navigation to the notifications page shell.
Proposed fix
- '/notifications': Notifications, + '/notifications': protectedRoute(Notifications),As per coding guidelines, “For protected routes, check
$authState.isAuthenticatedbefore allowing access; redirect unauthenticated users usingreplace()to avoid infinite redirect loops.”🤖 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 `@frontend/src/App.svelte` at line 215, Wrap the '/notifications' route with the app's protectedRoute guard so unauthenticated users cannot access Notifications directly; specifically, replace the raw mapping "/notifications": Notifications with the protectedRoute(...) call (using the same protectedRoute function used elsewhere) and ensure the guard checks $authState.isAuthenticated and redirects unauthenticated users using replace() to the login route to avoid infinite redirect loops.Source: Coding guidelines
frontend/src/routes/Notifications.svelte (1)
18-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent out-of-order
loadNotifications()responses from overwriting newer state.Concurrent loads (fast filter toggles / load-more overlap) can let older responses clobber newer UI state.
Proposed fix
+ let latestRequestId = 0; + async function loadNotifications(reset = true) { @@ - loading = true; + const requestId = ++latestRequestId; + loading = true; error = ''; @@ - const items = asList(response.data); + if (requestId !== latestRequestId) return; + const items = asList(response.data); notifications = reset ? items : [...notifications, ...items]; totalCount = response.data?.count ?? notifications.length; hasNext = Boolean(response.data?.next); currentPage = page; } catch (err) { + if (requestId !== latestRequestId) return; error = 'Failed to load notifications'; } finally { - loading = false; + if (requestId === latestRequestId) loading = false; } }Also applies to: 67-71
🤖 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 `@frontend/src/routes/Notifications.svelte` around lines 18 - 47, Concurrent loadNotifications() responses can be applied out-of-order and overwrite newer UI state; fix by adding a monotonically increasing request id (e.g., module-scoped let lastLoadId = 0) that you increment at the start of loadNotifications(), capture as const localLoadId = ++lastLoadId, and after the await response return early if localLoadId !== lastLoadId so only the latest response updates notifications, totalCount, hasNext, currentPage, loading and error; apply the same request-id guard to the other load-more/filter handler referenced around lines 67-71 so stale promises cannot clobber newer state.frontend/src/lib/relativeTime.js (1)
5-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard invalid timestamps before computing relative time.
When
valueis null/invalid, this can surface"Invalid Date"in the UI instead of a safe fallback.Proposed fix
export function relativeTime(value, { verbose = false } = {}) { const date = new Date(value); + if (Number.isNaN(date.getTime())) return ''; const seconds = Math.max(1, Math.floor((Date.now() - date.getTime()) / 1000)); const suffix = verbose ? ' ago' : '';🤖 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 `@frontend/src/lib/relativeTime.js` around lines 5 - 22, In relativeTime, guard against invalid or null timestamps by validating the Date created from value (use isNaN(date.getTime()) or Number.isNaN(date.getTime())) and return a safe fallback (e.g. an empty string or 'now') before doing any arithmetic; update the function to check this right after const date = new Date(value) and short-circuit to the chosen fallback so the rest of the code never operates on an "Invalid Date".backend/notifications/registry.py (1)
53-53: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd duplicate-slug protection when building
EVENT_TYPES.Line 53 silently overwrites earlier entries if a slug is accidentally duplicated, which can misroute notification metadata. Add an import-time guard so duplicates fail fast.
🛡️ Proposed protection
EVENT_TYPES = {event.slug: event for event in _EVENT_TYPES} +if len(EVENT_TYPES) != len(_EVENT_TYPES): + seen = set() + duplicates = [] + for event in _EVENT_TYPES: + if event.slug in seen: + duplicates.append(event.slug) + seen.add(event.slug) + raise RuntimeError( + f"Duplicate notification event slug(s): {', '.join(sorted(set(duplicates)))}" + )As per coding guidelines,
notifications/registry.pyis the single source of truth for event types, so collisions should be prevented explicitly.🤖 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 `@backend/notifications/registry.py` at line 53, EVENT_TYPES currently builds with {event.slug: event for event in _EVENT_TYPES} which will silently overwrite duplicates; replace this with an import-time guard that iterates _EVENT_TYPES, checks if event.slug is already in the map, and raises a clear ValueError (including the duplicate slug and conflicting event classes) if so, then inserts the event into EVENT_TYPES; reference EVENT_TYPES, _EVENT_TYPES, and event.slug to locate the change.Source: Coding guidelines
🤖 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 `@backend/notifications/admin_mixins.py`:
- Around line 78-90: The admin save currently calls broadcast logic that can
raise or return None causing a 500 after the model is already saved; update
save_model/_send_broadcast to guard calls to broadcast_service by wrapping them
in try/except and by testing the return value before using it: catch exceptions
from broadcast_service, log the error, and call self.message_user(request, ...)
with a WARNING explaining broadcast failed (use self.broadcast_ineligible_reason
or the exception message safely), and if broadcast_service returns None treat as
failure and message similarly; ensure any failures do not re-raise so the
original save completes and admin actions don’t produce a server error (refer to
save_model, _send_broadcast, broadcast_service, broadcast_eligible, and
message_user to locate spots to add the guards).
In `@backend/notifications/models.py`:
- Line 80: The dedupe_key should not be globally unique; change the model to
remove unique=True on dedupe_key and add a UniqueConstraint that scopes
uniqueness to the recipient and broadcast mode (use the model fields dedupe_key,
recipient, and the is_broadcast-like field already present on the model) so keys
are unique per recipient/broadcast combination; then update the personal dedupe
lookup in backend/notifications/services.py (the get_or_create call that
currently uses dedupe_key alone) to include recipient (and/or is_broadcast where
appropriate) in the lookup/filters so get_or_create cannot reassign another
user's row; finally regenerate and commit the migration reflecting the new
constraint.
In `@backend/notifications/registry.py`:
- Around line 60-63: Replace the plain re-raise of KeyError at the notification
lookup that uses slug with an explicit suppression of exception chaining by
appending "from None" to the raise; specifically update the existing raise
KeyError(f"Unknown notification event type '{slug}'. Register it in
notifications/registry.py.") to use "raise KeyError(... ) from None" so the
replacement message is not chained to any prior exception.
In `@backend/notifications/serializers.py`:
- Around line 6-35: Create a new LightNotificationSerializer that mirrors
NotificationSerializer but omits heavy/nested fields (remove payload, body,
link_url, link_label and any fields that trigger nested queries) and only
includes minimal fields like id, event_type, category, category_label, priority,
priority_label, title, is_broadcast, is_read, created_at (keep get_is_read logic
if needed). Name it LightNotificationSerializer and place it alongside
NotificationSerializer in serializers.py. Then update the notifications ViewSet
to return LightNotificationSerializer for list actions (implement or update
get_serializer_class to return LightNotificationSerializer when self.action ==
'list', and NotificationSerializer otherwise) so list views use the lightweight
serializer. Ensure get_is_read behavior is preserved and read_only_fields remain
appropriate.
In `@backend/notifications/services.py`:
- Around line 139-150: The current re-broadcast path calls
_apply_values(notification, values) (which saves and updates updated_at) then
separately runs
Notification.objects.filter(pk=notification.pk).update(created_at=timezone.now())
and deletes receipts, leaving the in-memory notification out-of-sync briefly;
fix by consolidating DB writes inside a transaction: wrap the sequence in
transaction.atomic(), acquire the row with select_for_update() if concurrent
readers are a concern, perform a single queryset .update(...) that sets the
fields from values plus created_at=timezone.now() (instead of calling
_apply_values which saves separately), delete notification.receipts.all() within
the same transaction, then call
notification.refresh_from_db(fields=['created_at','updated_at', ...]) to sync
the in-memory object.
- Around line 230-241: TOCTOU: unread_broadcast_ids is queried then used in
NotificationReceipt.objects.bulk_create which can over-report how many were
marked due to concurrent updates; after the bulk_create call compute the actual
number marked by re-querying NotificationReceipt for the user and those
notification IDs (e.g.
NotificationReceipt.objects.filter(notification_id__in=unread_broadcast_ids,
user=user).count()) and use that count for the "X notifications marked" message,
or alternatively add a short comment referencing unread_broadcast_ids and
NotificationReceipt.objects.bulk_create explaining the minor race and why it's
acceptable if you choose not to change behavior.
- Around line 37-51: Add explicit return type annotations to the three private
helpers: annotate _source_for to return Dict[str, str] (it currently returns a
mapping of 'source_app', 'source_model', 'source_object_id' or {}), annotate
_apply_values to return None (it mutates and saves the notification), and
annotate _display_submission_name to return str (it produces the display
string). Import typing.Dict if needed and update the function signatures for
_source_for, _apply_values, and _display_submission_name accordingly to improve
IDE/type-checker support.
In `@frontend/src/routes/Notifications.svelte`:
- Around line 62-65: markAllRead currently marks all items read but leaves them
visible when the Unread filter is active; after calling
notificationStore.markAllRead() update the local notifications list to reflect
the active filter state: set notifications = notifications.map(..., is_read:
true) as you already do, then if the Unread filter is active (check the
component's filter state, e.g. filter === 'unread' or showUnread) remove or hide
newly-read items (notifications = notifications.filter(item => !item.is_read))
so the UI no longer shows read items when Unread is selected; update the
markAllRead() function to perform this conditional filtering using the
component's existing filter state variables.
---
Duplicate comments:
In `@backend/notifications/registry.py`:
- Line 53: EVENT_TYPES currently builds with {event.slug: event for event in
_EVENT_TYPES} which will silently overwrite duplicates; replace this with an
import-time guard that iterates _EVENT_TYPES, checks if event.slug is already in
the map, and raises a clear ValueError (including the duplicate slug and
conflicting event classes) if so, then inserts the event into EVENT_TYPES;
reference EVENT_TYPES, _EVENT_TYPES, and event.slug to locate the change.
In `@frontend/src/App.svelte`:
- Line 215: Wrap the '/notifications' route with the app's protectedRoute guard
so unauthenticated users cannot access Notifications directly; specifically,
replace the raw mapping "/notifications": Notifications with the
protectedRoute(...) call (using the same protectedRoute function used elsewhere)
and ensure the guard checks $authState.isAuthenticated and redirects
unauthenticated users using replace() to the login route to avoid infinite
redirect loops.
In `@frontend/src/lib/notificationStore.js`:
- Around line 106-117: In markRead, avoid always decrementing unreadCount;
before calling update check the current item in state.items (or inside the
update callback) to see if the item with id is currently unread, and only
subtract 1 when its prior read flag is false (or when updated.read === true and
prior.read === false). Use the existing symbols: function markRead,
notificationsAPI.markRead, updated, and the update((state) => ...) callback to
conditionally update unreadCount so duplicate markRead calls won’t decrement the
badge incorrectly.
In `@frontend/src/lib/relativeTime.js`:
- Around line 5-22: In relativeTime, guard against invalid or null timestamps by
validating the Date created from value (use isNaN(date.getTime()) or
Number.isNaN(date.getTime())) and return a safe fallback (e.g. an empty string
or 'now') before doing any arithmetic; update the function to check this right
after const date = new Date(value) and short-circuit to the chosen fallback so
the rest of the code never operates on an "Invalid Date".
In `@frontend/src/routes/Notifications.svelte`:
- Around line 18-47: Concurrent loadNotifications() responses can be applied
out-of-order and overwrite newer UI state; fix by adding a monotonically
increasing request id (e.g., module-scoped let lastLoadId = 0) that you
increment at the start of loadNotifications(), capture as const localLoadId =
++lastLoadId, and after the await response return early if localLoadId !==
lastLoadId so only the latest response updates notifications, totalCount,
hasNext, currentPage, loading and error; apply the same request-id guard to the
other load-more/filter handler referenced around lines 67-71 so stale promises
cannot clobber newer state.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f210be22-6fe3-4019-bbdf-5b6918ac860f
📒 Files selected for processing (35)
CHANGELOG.mdbackend/CLAUDE.mdbackend/api/urls.pybackend/contributions/admin.pybackend/contributions/node_upgrade/admin.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/gen_tv/admin.pybackend/notifications/__init__.pybackend/notifications/admin.pybackend/notifications/admin_mixins.pybackend/notifications/apps.pybackend/notifications/migrations/0001_initial.pybackend/notifications/migrations/__init__.pybackend/notifications/models.pybackend/notifications/receivers.pybackend/notifications/registry.pybackend/notifications/serializers.pybackend/notifications/services.pybackend/notifications/tests.pybackend/notifications/views.pybackend/partners/admin.pybackend/poaps/admin.pybackend/tally/settings.pybackend/users/admin.pyfrontend/CLAUDE.mdfrontend/src/App.sveltefrontend/src/components/Navbar.sveltefrontend/src/components/NotificationCenter.sveltefrontend/src/lib/api.jsfrontend/src/lib/notificationStore.jsfrontend/src/lib/notificationUtils.jsfrontend/src/lib/relativeTime.jsfrontend/src/routes/MySubmissions.sveltefrontend/src/routes/Notifications.svelte
Comment on lines
+37
to
+51
| def _source_for(obj): | ||
| if obj is None: | ||
| return {} | ||
| meta = obj._meta | ||
| return { | ||
| 'source_app': meta.app_label, | ||
| 'source_model': meta.model_name, | ||
| 'source_object_id': str(obj.pk), | ||
| } | ||
|
|
||
|
|
||
| def _apply_values(notification, values): | ||
| for field, value in values.items(): | ||
| setattr(notification, field, value) | ||
| notification.save(update_fields=[*values.keys(), 'updated_at']) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding return type annotations to private helpers.
The private functions _source_for, _apply_values, and _display_submission_name (line 264) are missing return type annotations. Adding them improves maintainability and IDE support.
📝 Suggested annotations
-def _source_for(obj):
+def _source_for(obj) -> dict[str, str]:
if obj is None:
return {}
...
-def _apply_values(notification, values):
+def _apply_values(notification, values) -> None:
for field, value in values.items():
...
-def _display_submission_name(submission):
+def _display_submission_name(submission) -> str:
if submission.title:
...📝 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
| def _source_for(obj): | |
| if obj is None: | |
| return {} | |
| meta = obj._meta | |
| return { | |
| 'source_app': meta.app_label, | |
| 'source_model': meta.model_name, | |
| 'source_object_id': str(obj.pk), | |
| } | |
| def _apply_values(notification, values): | |
| for field, value in values.items(): | |
| setattr(notification, field, value) | |
| notification.save(update_fields=[*values.keys(), 'updated_at']) | |
| def _source_for(obj) -> dict[str, str]: | |
| if obj is None: | |
| return {} | |
| meta = obj._meta | |
| return { | |
| 'source_app': meta.app_label, | |
| 'source_model': meta.model_name, | |
| 'source_object_id': str(obj.pk), | |
| } | |
| def _apply_values(notification, values) -> None: | |
| for field, value in values.items(): | |
| setattr(notification, field, value) | |
| notification.save(update_fields=[*values.keys(), 'updated_at']) |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 37-37: Missing return type annotation for private function _source_for
(ANN202)
[warning] 48-48: Missing return type annotation for private function _apply_values
Add return type annotation: None
(ANN202)
🤖 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 `@backend/notifications/services.py` around lines 37 - 51, Add explicit return
type annotations to the three private helpers: annotate _source_for to return
Dict[str, str] (it currently returns a mapping of 'source_app', 'source_model',
'source_object_id' or {}), annotate _apply_values to return None (it mutates and
saves the notification), and annotate _display_submission_name to return str (it
produces the display string). Import typing.Dict if needed and update the
function signatures for _source_for, _apply_values, and _display_submission_name
accordingly to improve IDE/type-checker support.
Comment on lines
+230
to
+241
| unread_broadcast_ids = list( | ||
| annotate_read_state(feed_for(user).filter(recipient__isnull=True), user) | ||
| .filter(receipt_read=False) | ||
| .values_list('pk', flat=True) | ||
| ) | ||
| NotificationReceipt.objects.bulk_create( | ||
| [ | ||
| NotificationReceipt(notification_id=pk, user=user, read_at=now) | ||
| for pk in unread_broadcast_ids | ||
| ], | ||
| ignore_conflicts=True, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor TOCTOU race in mark_all_read between query and bulk-create.
Lines 230–234 query for unread broadcast IDs, then lines 235–241 bulk-create receipts for those IDs. If another concurrent request marks some of the same broadcasts as read between the query and the bulk-create, the returned count (line 242) may over-report the number marked. The ignore_conflicts=True flag prevents errors from duplicate receipts.
Impact is limited to a slightly incorrect "X notifications marked" message and is acceptable for a notification system. Fixing would require advisory locking or atomic upsert logic (higher complexity). Current implementation is reasonable.
🤖 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 `@backend/notifications/services.py` around lines 230 - 241, TOCTOU:
unread_broadcast_ids is queried then used in
NotificationReceipt.objects.bulk_create which can over-report how many were
marked due to concurrent updates; after the bulk_create call compute the actual
number marked by re-querying NotificationReceipt for the user and those
notification IDs (e.g.
NotificationReceipt.objects.filter(notification_id__in=unread_broadcast_ids,
user=user).count()) and use that count for the "X notifications marked" message,
or alternatively add a short comment referencing unread_broadcast_ids and
NotificationReceipt.objects.bulk_create explaining the minor race and why it's
acceptable if you choose not to change behavior.
The event registry fails fast on duplicate slugs instead of silently overwriting entries. The unread badge no longer over-decrements when a dropdown item is marked read twice, fast All/Unread filter toggles can no longer let an older response overwrite a newer one (same request token pattern as CommunityPoaps), and invalid timestamps render as empty instead of "Invalid Date". The /notifications route stays unwrapped on purpose: like /profile and /my-submissions it renders an in-page sign-in prompt rather than redirecting. ## Claude Implementation Notes - backend/notifications/registry.py: EVENT_TYPES built in a loop raising RuntimeError on duplicate slugs - frontend/src/lib/notificationStore.js: markRead decrements only when the dropdown-slice item was unread; page-only items keep decrementing since callers guard on is_read - frontend/src/routes/Notifications.svelte: latestRequestId token discards stale list responses - frontend/src/lib/relativeTime.js: return '' for invalid dates
Dedupe keys are now scoped per recipient (with a separate constraint for broadcasts), so a key collision between producers can never reassign another user's notification; the lookup pins the recipient and a regression test proves two users sharing a key get separate rows. Admin broadcasts that fail after the object is saved report an error message instead of a 500, re-broadcast refreshes are atomic, list responses drop the channel-renderer payload field, marking all read while the Unread filter is active clears the list, and the registry raises unknown-event errors without misleading chaining. ## Claude Implementation Notes - backend/notifications/models.py: dedupe_key unique=True replaced with two conditional UniqueConstraints (per-recipient personal, broadcast-wide); migration 0001 regenerated - backend/notifications/services.py: notify()/broadcast() include recipient in the get_or_create lookup; re-broadcast refresh wrapped in transaction.atomic() - backend/notifications/admin_mixins.py: try/except with logging around broadcast_service in save path and bulk action, failure counts in messages - backend/notifications/serializers.py, views.py: LightNotificationSerializer (full minus payload) used for list via get_serializer_class - backend/notifications/registry.py: raise KeyError ... from None - backend/notifications/tests.py: test_same_dedupe_key_never_reassigns_across_users - frontend/src/routes/Notifications.svelte: markAllRead clears the list when the Unread filter is active
Staff can now compose one-off notifications in Django admin and send them to everyone, to users with selected roles (builders, validators, stewards, creators, union semantics), to hand-picked users, or to a pasted batch of wallet addresses with unmatched lines reported back. Announcements can be pure text with no link, and bodies support markdown on the notifications page. Sending fans out personal notification rows per resolved recipient (snapshot semantics, never a public broadcast), a draft preview shows reach before sending, and a deliberate resend resurfaces the notification as unread for the current audience without duplicating. Recipient resolution is a standalone step so future email and Telegram campaigns can reuse it. ## Claude Implementation Notes - backend/notifications/models.py: CustomNotification (copy + target_mode/roles/users M2M/wallets + status/sent_*/unmatched_wallets/channels JSON default ['portal']); 'announcement' category choice - backend/notifications/migrations/0002_*: CreateModel + category AlterField - backend/notifications/campaigns.py: parse_wallet_lines (case-insensitive dedupe, invalid line report), resolve_recipients (channel-agnostic; is_active only; banned/invisible included by design), send_campaign (audience-scoped refresh with explicit created_at/updated_at, chunked bulk_create ignore_conflicts, status flip last so crashes leave a resumable draft) - backend/notifications/registry.py: custom.announcement EventType - backend/notifications/admin.py: CustomNotificationAdminForm (mode radios, role checkboxes, send_now) + CustomNotificationAdmin (autocomplete target_users, audience_preview readonly, send in save_related after M2M commit, send/resend bulk actions, failure guards) - backend/notifications/tests.py: 17 new tests (resolution modes, fan-out privacy guard, zero recipients, double-send, audience-scoped resend, feed visibility, admin POST flows incl. save_related ordering) - frontend/src/routes/Notifications.svelte: markdown bodies via parseMarkdown, row as div[role=button] so body links are clickable, cursor-default without link_url, scoped styles for p/a/lists/code - frontend/src/components/NotificationCenter.svelte: cursor affordance only when link_url set - backend/CLAUDE.md, frontend/CLAUDE.md: document campaigns module and markdown rendering
Social task admins get the same off-by-default broadcast checkbox as other content types, but the notification goes only to the role the task's category targets: builder tasks reach builders, validator tasks reach validators, and community tasks reach community members. This adds builders and community as notification audiences, resolved through the existing role profiles, and links each announcement to the matching category tasks page. ## Claude Implementation Notes - backend/notifications/models.py: AUDIENCE_BUILDERS and AUDIENCE_COMMUNITY choices; migration 0003 (AlterField) - backend/notifications/services.py: audiences_for resolves Builder/Creator profiles; estimate_broadcast_reach counts them; broadcast_social_task maps task.category.slug to audience and to /builders|validators|community/tasks, raises on unmapped categories instead of over-notifying - backend/notifications/registry.py: social_task.published EventType (content category) - backend/social_tasks/admin.py: SocialTaskAdmin uses BroadcastNotificationAdminMixin; eligible only while is_currently_active() - backend/notifications/tests.py: 5 tests (audience resolution, builder/community scoping incl. feed visibility, single-row rebroadcast, admin POST with checkbox) - backend/CLAUDE.md: document new audiences and producer
Notification bodies now render through the image-free markdown sanitizer, so a campaign body can no longer embed an external image that would report each recipient's IP and open timing to the image host when they open their notifications. Clicking an inline link inside a body now follows only that link: it still marks the notification read but no longer also triggers the row's own redirect. ## Claude Implementation Notes - frontend/src/routes/Notifications.svelte: parseUserMarkdown() instead of parseMarkdown() for bodies; openNotification takes the click event and skips followNotificationLink when the click originated from an anchor (keyboard activation still follows the row link) - frontend/CLAUDE.md: document the image-free sanitizer and anchor-click behavior
Admins can now recall mistaken portal notifications from the same surfaces that sent them. Source-object broadcasts get a recall checkbox and bulk action via the broadcast admin mixin, deleting the single broadcast row and any receipts. Custom notification campaigns get a recall checkbox and bulk action that deletes all delivered portal rows while keeping the campaign record for audit and resend. ## Claude Implementation Notes - backend/notifications/services.py: add broadcast_dedupe_key() and recall_broadcast() for source-object broadcast deletion - backend/notifications/admin_mixins.py: add recall checkbox, mutual-exclusion validation, bulk recall action, and broadcast_event_slug config - backend/contributions/admin.py, partners/admin.py, gen_tv/admin.py, poaps/admin.py, contributions/node_upgrade/admin.py, social_tasks/admin.py: declare broadcast_event_slug for recall lookup - backend/notifications/campaigns.py: add recall_campaign() for custom campaign portal-row deletion - backend/notifications/admin.py: add recall checkbox/action to CustomNotificationAdmin - backend/notifications/tests.py: cover broadcast recall, admin recall, campaign recall, and social task recall - backend/CLAUDE.md: document recall behavior
The My Submissions endpoint now lets a specific submission id take precedence over a state filter. Review notification links freeze the decision state at send time, so older links could previously AND a stale state with the submission id and land on an empty list after resubmission or appeal changes. The submission id remains owner-scoped through get_queryset(). ## Claude Implementation Notes - backend/contributions/views.py: apply the state filter only when no specific submission id is requested - backend/contributions/tests/test_security_boundaries.py: add regression coverage for stale-state deep links and cross-user submission id scoping
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.
Adds a notification center to the portal: a navbar bell with unread badge and dropdown, plus a full notifications page with unread filtering and pagination. Personal notifications fire automatically for submission review outcomes (deep-linking to the matching row in My Submissions), highlighted contributions, referrals joining, and validator graduation. Admins can explicitly broadcast featured content, alerts, partners, contribution types, missions, Gen TV streams, POAP drops, and validator-targeted node upgrade announcements via an off-by-default checkbox and bulk action in Django admin; each broadcast is stored as a single audience-scoped row with lazy per-user read receipts, so fan-out cost stays flat and re-broadcasting resurfaces instead of duplicating. An event registry centralizes event metadata (category, priority, audience, and future email/Telegram channels) so adding a notification is one registry entry plus one producer call, and the change also repairs the 0051 data migration that blocked fresh test databases and ships 16 backend tests covering dedupe, receipts, audience scoping, the API, and the admin mixin.
Summary by CodeRabbit