fix(notifications): honor category prefs before push delivery - #1943
Conversation
Load notification_preferences for recipients and skip push when the mapped category's inApp channel is off. Drop email toggles from Settings since there is no email delivery path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@nyxsky404 is attempting to deploy a commit to the durdana3105's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR adds notification preference helpers and applies category-based push suppression to direct and scheduled delivery. Settings now expose push preferences only. Upload route documentation and profile-photo upload tests were updated for user-scoped storage paths and a 2 MB limit. ChangesNotification preference enforcement
Upload documentation and test updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NotificationRequest
participant NotificationController
participant Profiles
participant WebPush
NotificationRequest->>NotificationController: submit type and category
NotificationController->>Profiles: load recipient preferences
Profiles-->>NotificationController: return notification preferences
NotificationController->>NotificationController: resolve category and check inApp
NotificationController->>WebPush: deliver allowed push
NotificationController-->>NotificationRequest: return delivery or preference_disabled response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Update profile-photo assertions for Supabase storage paths and the 2MB limit, and document /api/upload plus /api/users/upload-photo so docs completeness checks pass. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@nyxsky404 The CI failures on this PR are fixed. Cause: backend tests still expected the old local Fix: aligned The required @durdana3105 Ready for review and merge when you are. |
nyxsky404
left a comment
nyxsky404
left a comment
There was a problem hiding this comment.
Looks good: push dispatch and send-push now honor category inApp prefs before delivery; Settings UI matches the push-only path.
CI test is green after the upload test/docs alignment commit. LGTM on the feature change — needs a maintainer approval/merge (@durdana3105).
![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
🤖 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/controllers/cronController.js`:
- Around line 135-143: Update the notification state handling in the cron
controller: in the prefsError rollback path, inspect the result of the
notifications update that clears push_claimed_at and handle or report rollback
failures instead of ignoring them. In the opt-out path, check the push_sent_at
update result and ensure failures do not mark the notification as processed or
allow it to be reclaimed for repeated evaluation.
In `@backend/tests/uploadPhoto.test.js`:
- Around line 169-171: Update the upload test around storageUploadMock to assert
that its first call received a path containing TEST_USER_ID followed by the
expected filename or path structure. Keep the existing fileUrl and
upload-invocation assertions, but make the user-scoping verification depend
directly on the .upload() argument rather than the generated public URL.
In `@docs/api.md`:
- Line 165: Update the profile-photo validation bullet to claim only that the
image type is checked against the allowed MIME types; do not claim magic-byte
content matching unless the corresponding comparison is added in the user upload
route.
In `@src/pages/Settings.tsx`:
- Around line 154-157: Update the ToggleSwitch component to expose its checked
state with aria-pressed={checked}, and add category-specific accessible labels
at each Settings call site for messages/Push, sessions, and friend requests.
Ensure each label identifies the notification category and Push channel while
preserving the existing checked and onChange behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f38b810-148d-438e-b9ec-4cb3e06bec88
📒 Files selected for processing (8)
backend/controllers/cronController.jsbackend/controllers/notificationController.jsbackend/tests/dispatchPushNotifications.test.jsbackend/tests/notificationPreferences.test.jsbackend/tests/uploadPhoto.test.jsbackend/utils/notificationPreferences.jsdocs/api.mdsrc/pages/Settings.tsx
| if (prefsError) { | ||
| const notificationIds = notifications.map((n) => n.id); | ||
| if (notificationIds.length > 0) { | ||
| await supabase | ||
| .from("notifications") | ||
| .update({ push_claimed_at: null }) | ||
| .in("id", notificationIds); | ||
| } | ||
| return res.status(500).json({ error: prefsError.message }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle failures when changing preference-related notification state.
If the profile query fails, check the claim rollback result. A rollback failure leaves claimed notifications unavailable until the claim TTL expires.
If the opt-out push_sent_at update fails, do not report the notification as processed. The row can otherwise be reclaimed and repeatedly evaluated.
Proposed fix
- await supabase
+ const { error: rollbackError } = await supabase
.from("notifications")
.update({ push_claimed_at: null })
.in("id", notificationIds);
+ if (rollbackError) {
+ return res.status(500).json({
+ error: `Preference fetch failed, and rollback failed: ${rollbackError.message}`,
+ });
+ }
- await supabase
+ const { error: skipError } = await supabase
.from("notifications")
.update({ push_sent_at: new Date().toISOString() })
.eq("id", notification.id);
+ if (skipError) throw skipError;Also applies to: 165-168
🤖 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/controllers/cronController.js` around lines 135 - 143, Update the
notification state handling in the cron controller: in the prefsError rollback
path, inspect the result of the notifications update that clears push_claimed_at
and handle or report rollback failures instead of ignoring them. In the opt-out
path, check the push_sent_at update result and ensure failures do not mark the
notification as processed or allow it to be reclaimed for repeated evaluation.
| // Supabase storage path is scoped to the authenticated user's ID | ||
| expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`); | ||
| expect(storageUploadMock).toHaveBeenCalled(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the storage path directly.
toHaveBeenCalled() proves only that an upload occurred. The URL assertion proves user scoping only if the getPublicUrl mock derives the URL from the upload path. Assert the first .upload() argument directly.
Suggested assertion
expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`);
expect(storageUploadMock).toHaveBeenCalled();
+ expect(storageUploadMock.mock.calls[0][0]).toMatch(
+ new RegExp(`^${TEST_USER_ID}/`),
+ );📝 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.
| // Supabase storage path is scoped to the authenticated user's ID | |
| expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`); | |
| expect(storageUploadMock).toHaveBeenCalled(); | |
| // Supabase storage path is scoped to the authenticated user's ID | |
| expect(res.body.fileUrl).toContain(`${TEST_USER_ID}/`); | |
| expect(storageUploadMock).toHaveBeenCalled(); | |
| expect(storageUploadMock.mock.calls[0][0]).toMatch( | |
| new RegExp(`^${TEST_USER_ID}/`), | |
| ); |
🤖 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/tests/uploadPhoto.test.js` around lines 169 - 171, Update the upload
test around storageUploadMock to assert that its first call received a path
containing TEST_USER_ID followed by the expected filename or path structure.
Keep the existing fileUrl and upload-invocation assertions, but make the
user-scoping verification depend directly on the .upload() argument rather than
the generated public URL.
| **Validation**: | ||
| - 2MB size limit | ||
| - Strict image MIME allow-list | ||
| - Magic byte verification that file content matches the declared image type |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Correct the profile-photo MIME validation claim.
backend/routes/users.js checks the declared MIME and detected MIME against the allow-list separately. It does not verify that both values match. Therefore, an allowed JPEG declaration with PNG content is accepted.
Update this bullet to describe verification of an allowed image type, or add an explicit comparison in the route.
🤖 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 `@docs/api.md` at line 165, Update the profile-photo validation bullet to claim
only that the image type is checked against the allowed MIME types; do not claim
magic-byte content matching unless the corresponding comparison is added in the
user upload route.
| <ToggleSwitch | ||
| checked={preferences.messages.inApp} | ||
| onChange={() => handleToggle("messages", "inApp")} | ||
| onChange={() => handleToggle("messages")} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give each Push toggle an accessible name and state.
ToggleSwitch renders a button without text, aria-label, or aria-pressed. Screen-reader users cannot identify the notification category or determine whether Push is enabled.
Pass a category-specific label at each call site. Add aria-pressed={checked} to ToggleSwitch.
Proposed fix
<ToggleSwitch
checked={preferences.messages.inApp}
onChange={() => handleToggle("messages")}
+ label="Push notifications for new messages"
/>
-const ToggleSwitch = ({ checked, onChange }: { checked: boolean; onChange: () => void }) => {
+const ToggleSwitch = ({ checked, onChange, label }: {
+ checked: boolean;
+ onChange: () => void;
+ label: string;
+}) => {
return (
<button
type="button"
onClick={onChange}
+ aria-label={label}
+ aria-pressed={checked}Apply equivalent labels for sessions and friend requests.
Also applies to: 167-170, 180-182
🤖 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 `@src/pages/Settings.tsx` around lines 154 - 157, Update the ToggleSwitch
component to expose its checked state with aria-pressed={checked}, and add
category-specific accessible labels at each Settings call site for
messages/Push, sessions, and friend requests. Ensure each label identifies the
notification category and Push channel while preserving the existing checked and
onChange behavior.
|
Hi @durdana3105 — gentle nudge. This PR is ready for review from my side. I've rebased/kept it current where possible. If CI is red due to unrelated upstream/main issues or deploy previews, the code change itself should still be reviewable. Please review and merge when you can — and add Thank you! |
2 participants
Summary
Push delivery was sending to every subscription for a user without reading
notification_preferences. Settings also showed email switches with no email path behind them.sendPushNotificationand crondispatchPushNotificationsload prefs and skip when the category'sinAppchannel is disabledpush_sent_atso they don't retry foreverTesting
npx vitest run --project backend tests/dispatchPushNotifications.test.js tests/notificationPreferences.test.jsFixes #1900
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Documentation